Skip to content

feat: external compatibility verification + GA release gates#5

Merged
clopca merged 2 commits into
mainfrom
codex/external-compat-ga
Feb 8, 2026
Merged

feat: external compatibility verification + GA release gates#5
clopca merged 2 commits into
mainfrom
codex/external-compat-ga

Conversation

@clopca
Copy link
Copy Markdown
Owner

@clopca clopca commented Feb 8, 2026

Summary

  • hardens MCP compatibility lifecycle + transcript-based determinism checks
  • adds platform adapter foundation for OpenCode, Claude Code, and Cursor
  • introduces worker bridge protocol (stdio + optional HTTP) and lifecycle smoke checks
  • adds external compatibility evidence pipeline with machine-readable report schema
  • adds matrix renderer + release gate enforcement (freshness/status/matrix alignment)
  • adds CI workflows:
    • nightly/manual external verification on self-hosted macOS
    • release-blocking compatibility gate
  • adds ops/dashboard/runtime metrics and documentation/runbook updates

Key External-Compat Additions

  • report schema: docs/schemas/external-compat-report.schema.json
  • latest/history reports: docs/compatibility/external-compat-latest.json, docs/compatibility/external-compat-history/*.json
  • verification scripts:
    • scripts/verify-external-clients.ts
    • scripts/smoke-platform-workers.ts
    • scripts/render-compat-matrix.ts
    • scripts/check-external-compat-gate.ts
    • scripts/detect-client-versions.ts
    • scripts/verify-external-clients-auto.ts
  • workflows:
    • .github/workflows/external-compat.yml
    • .github/workflows/release-gate.yml
  • fixtures/tests:
    • tests/fixtures/external-clients/*
    • tests/scripts/external-compat-scripts.test.ts

Verification Performed

  • bun run build
  • bun test tests/scripts/external-compat-scripts.test.ts
  • bun test tests/servers/mcp-transcripts.test.ts tests/integration/platform-parity.test.ts tests/integration/platform-worker.test.ts tests/adapters/platform-bridge-client.test.ts
  • bun run compat:verify --artifacts-dir .artifacts/external-compat
  • bun run compat:verify:auto --artifacts-dir .artifacts/external-compat
  • bun run compat:matrix
  • bun run compat:smoke --artifacts-dir .artifacts/external-compat
  • bun run compat:gate

Notes

  • .artifacts/ outputs were intentionally not committed.
  • compatibility claims are scoped to macOS verification per matrix policy.

Summary by CodeRabbit

Release Notes

  • New Features

    • External compatibility verification for Claude Code and Cursor platform adapters
    • Platform adapter workers normalizing IDE-specific events
    • MCP server lifecycle with protocol version negotiation (initialize/notifications)
    • Runtime health and metrics endpoints for observability
    • Search result explanations showing matched strategies and relevance scores
    • Observation lineage tracking for revision history
    • Dashboard Ops page monitoring queue and platform adapters
  • Documentation

    • Platform adapter runbook and operational procedures
    • MCP compatibility matrix and protocol details
    • Getting started guide updates with adapter setup

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Feb 8, 2026

📝 Walkthrough

Walkthrough

Introduces a comprehensive platform adapter system for Claude Code and Cursor with event normalization, worker runtime, and end-to-end external compatibility testing. Adds MCP protocol negotiation, observation lineage tracking, search result explainability, dashboard observability, and supporting scripts and workflows for verification and release gating.

Changes

Cohort / File(s) Summary
Platform Adapter System
src/adapters/platform/*
New platform adapter framework with types, normalization utilities, bridge client, and concrete adapters for OpenCode, Claude Code, and Cursor. Includes descriptor definitions and barrel exports.
Platform Worker Runtime
src/platform-worker.ts
Main worker entry point supporting stdio and optional HTTP bridge modes. Handles event ingestion, session lifecycle, AI components integration, and graceful shutdown.
MCP Server Enhancements
src/adapters/mcp/server.ts
Adds protocol negotiation, initialization handling, strict lifecycle mode, and zod-based validation. Supports configurable protocol versions and compatibility modes.
Dashboard & HTTP API
src/adapters/http/server.ts, dashboard/src/pages/Ops.tsx, dashboard/src/components/...
New Ops runtime status page, /v1/health, /v1/metrics, /v1/platforms endpoints. Added navigation and runtime status provider integration.
Observation Lineage & Search Explain
src/db/observations.ts, src/store/ports.ts, src/search/*.ts, dashboard/src/components/TimelineItem.tsx
Adds getLineage() and getByIdIncludingArchived() queries, tracks revisionOf/supersededBy/deletedAt fields. Propagates explain metadata across FTS, vector, and hybrid search paths with strategy and matching signals.
Queue & Metrics Infrastructure
src/queue/processor.ts, src/runtime/metrics.ts
Introduces QueueObserver pattern with hooks for enqueue/batch lifecycle. RuntimeMetricsCollector tracks enqueue count, batch statistics, and uptime.
Session Lifecycle & Tool/Chat Capture
src/hooks/session-events.ts, src/hooks/tool-capture.ts, src/hooks/chat-capture.ts
Extracts session lifecycle handling with strict/legacy modes. Exports persistChatMessage and enqueueToolCapture for platform adapters.
External Compatibility Testing
scripts/verify-external-clients.ts, scripts/check-external-compat-gate.ts, scripts/render-compat-matrix.ts, scripts/detect-client-versions.ts, scripts/external-compat.ts, scripts/benchmark-*.ts, scripts/smoke-platform-workers.ts
Comprehensive test orchestration: MCP scenario execution, client version detection, compatibility report generation, matrix rendering, smoke tests, and release gate validation.
Configuration Extensions
src/config.ts, src/config/store.ts, src/types.ts
Adds platform adapter toggles (platformOpenCodeEnabled, platformClaudeCodeEnabled, platformCursorEnabled) and MCP compatibility settings (mcpCompatibilityMode, mcpProtocolVersion, mcpSupportedProtocolVersions).
GitHub Actions Workflows
.github/workflows/external-compat.yml, .github/workflows/release-gate.yml
New scheduled/manual workflows for external client compatibility verification and release gate execution with artifact packaging.
Test Fixtures & Suites
tests/fixtures/external-clients/*, tests/fixtures/mcp-transcripts/*, tests/adapters/platform-*.test.ts, tests/integration/platform-*.test.ts, tests/servers/http-server.test.ts, tests/servers/mcp-transcripts.test.ts, tests/scripts/external-compat-scripts.test.ts
Comprehensive test coverage: platform adapter normalization, bridge client protocol, MCP transcripts, platform worker integration, parity testing, and external compat script validation.
Documentation & Schemas
docs/compatibility/external-compat-latest.json, docs/schemas/external-compat-report.schema.json, docs/mcp-compatibility-matrix.md, docs/platform-adapter-runbook.md, docs/getting-started.md, docs/architecture.md, docs/openapi.v1.json
Compatibility report examples, JSON schemas, operational runbooks, matrix documentation, and API spec updates.
Supporting Changes
package.json, biome.json, CHANGELOG.md, README.md, src/core/contracts.ts, src/core/memory-engine.ts, src/index.ts
Package script additions for compat/build targets, schema config update, exports expansion for platform APIs, and contract method signatures.

Sequence Diagram(s)

sequenceDiagram
    actor User
    participant StdIO as Worker<br/>Stdin/Stdout
    participant Runtime as PlatformIngestion<br/>Runtime
    participant Adapter as Platform<br/>Adapter
    participant Queue as QueueProcessor
    participant DB as Database
    participant AI as AI Components

    User->>StdIO: Send event envelope
    StdIO->>Runtime: Parse BridgeEnvelope
    Runtime->>Adapter: normalize(rawEvent)
    Adapter-->>Runtime: NormalizedPlatformEvent
    
    alt Session Event
        Runtime->>Queue: handleSessionLifecycleEvent()
        Queue->>DB: Update session state
    else Tool Event
        Runtime->>Queue: enqueueToolCapture()
        Queue->>DB: Create pending item
        Queue->>AI: Compress output
        AI->>DB: Store observation
    else Chat Event
        Runtime->>DB: persistChatMessage()
        DB->>AI: Extract entities & concepts
        AI->>DB: Store observation
    else Idle/Flush
        Runtime->>Queue: processBatch()
        Queue->>AI: Summarize session
        AI->>DB: Store summary
    end
    
    Queue-->>StdIO: BridgeResponse { ok, code, message }
    StdIO-->>User: JSON response on stdout
Loading
sequenceDiagram
    actor Client as MCP Client
    participant Server as MCP Server
    participant Handler as RequestHandler
    participant MemoryEngine as MemoryEngine
    participant Validator as Schema<br/>Validator

    Client->>Server: initialize request
    Server->>Handler: handleInitialize()
    Handler->>Validator: Validate protocolVersion
    
    alt Unsupported Version
        Validator-->>Server: Error (-32602)
        Server-->>Client: Error response
    else Supported
        Validator-->>Handler: Valid
        Handler->>Server: Set initialized state
        Server-->>Client: Initialize response + capabilities
    end
    
    Client->>Server: tools/list (before initialize in strict mode)
    Server->>Handler: Check strictMode && !initialized
    
    alt Strict Mode Not Initialized
        Handler-->>Server: Error (-32002) "Not initialized"
        Server-->>Client: Error response
    else Legacy or Initialized
        Handler->>MemoryEngine: getToolDefinitions()
        MemoryEngine-->>Handler: Tool list
        Handler-->>Server: tools/list response
        Server-->>Client: Tools array
    end
    
    Client->>Server: tools/call {name, arguments}
    Server->>Handler: handleToolCall()
    Handler->>Validator: Validate arguments
    Validator-->>Handler: Errors or validated input
    Handler->>MemoryEngine: Execute tool
    MemoryEngine-->>Handler: Result or error
    Handler-->>Server: Structured response
    Server-->>Client: JSON-RPC result or error
Loading
sequenceDiagram
    participant GH as GitHub Actions
    participant Build as Build & Test
    participant External as External<br/>Verification
    participant Matrix as Matrix<br/>Renderer
    participant Report as Report<br/>Generation
    participant Gate as Release<br/>Gate

    GH->>Build: bun install && bun build
    Build->>External: Run external client verification
    External->>External: Load claude-code, cursor fixtures
    External->>External: Execute MCP scenarios
    External->>External: Run worker smoke tests
    External->>Report: Collect results, metrics, errors
    Report->>Report: Generate ExternalCompatReport
    Report->>Report: Write JSON with SLO checks
    Report->>Matrix: renderCompatMatrix()
    Matrix->>Matrix: Update docs/mcp-compatibility-matrix.md
    Report->>GH: Upload artifacts (external-compat-*.zip)
    
    GH->>Gate: Run release-gate workflow
    Gate->>Gate: Load external-compat-report.json
    Gate->>Gate: checkExternalCompatGate()
    Gate->>Gate: Validate report freshness, client status, scenarios
    Gate-->>GH: Exit 0 (pass) or 1 (fail)
Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Possibly related PRs

Poem

🐰 Hops through adapters, Claude and Cursor too,
Workers ingest events with lifecycle anew,
MCP negotiates with protocol grace,
Lineage blooms, explain lights the race.
Dashboard now glows with ops in sight,
Tests guard the gates, all systems tight!

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 7.95% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat: external compatibility verification + GA release gates' accurately reflects the main changes: external compatibility verification infrastructure and release gate enforcement, with clear references to both core features.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch codex/external-compat-ga

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@clopca
Copy link
Copy Markdown
Owner Author

clopca commented Feb 8, 2026

@greptile review

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Feb 8, 2026

Greptile Overview

Greptile Summary

This PR introduces a production-grade external compatibility verification system with release gates to ensure Claude Code and Cursor MCP integration stability. The implementation follows ports-and-adapters architecture with clean separation between platform-specific adapters and core memory engine logic.

Key Additions

  • Verification Infrastructure: Comprehensive MCP protocol compliance testing via transcript-based harness (scripts/verify-external-clients.ts) that validates JSON-RPC lifecycles, tool call semantics, and assertion-based output validation across both platforms
  • Worker Bridge Protocol: Platform worker processes (src/platform-worker.ts) supporting dual transports (stdio + optional HTTP) with event/flush/health/shutdown command protocol, enabling isolated testing and observability
  • Release Gate Enforcement: Automated compatibility gate (scripts/check-external-compat-gate.ts) blocking releases when evidence is stale (>7 days), clients fail required scenarios, or matrix documentation drifts from verification report
  • Platform Adapters: Event normalization layer (src/adapters/platform/normalize.ts) converting OpenCode/Claude Code/Cursor-specific schemas into unified internal representation, with integration tests (tests/integration/platform-parity.test.ts) proving behavioral equivalence
  • CI/CD Workflows: Nightly external compatibility verification on self-hosted macOS runners with artifact bundling, plus release-time gate enforcement
  • Schema + Documentation: JSON Schema validation for compatibility reports, operational runbook covering startup/troubleshooting/failure taxonomy, and machine-readable compatibility matrix

Architecture Quality

The implementation demonstrates strong engineering practices:

  • Deterministic Testing: Transcript fixtures with assertion-based validation enable reproducible protocol compliance checks
  • SLO Enforcement: P95 latency targets (MCP tool calls <250ms, worker ingest <100ms) with automated failure rate monitoring (<1%)
  • Failure Taxonomy: Categorized error codes (CLIENT_PROTOCOL_DRIFT, WORKER_BRIDGE_REGRESSION, ENVIRONMENT_DEPENDENCY, NON_DETERMINISTIC_OUTPUT) with documented remediation paths
  • Type Safety: Zod schema validation for MCP parameters, strict TypeScript typing throughout
  • Observability: Runtime metrics exposed via HTTP dashboard, comprehensive transcript/log artifact collection

Test Coverage

Extensive test coverage across all layers:

  • 5 new MCP transcript fixtures validating protocol edge cases
  • Platform parity integration tests proving adapter equivalence
  • Worker bridge smoke tests for stdio/HTTP transports
  • Script-level unit tests for verification/gate/matrix utilities
  • Comprehensive benchmark suite for platform normalization

Verification Performed

The PR description documents thorough pre-merge verification including builds, targeted test suites, compatibility verification with artifact collection, matrix rendering, smoke checks, and gate validation.

Confidence Score: 5/5

  • This PR is safe to merge with comprehensive testing and well-architected external compatibility infrastructure
  • Score reflects extensive test coverage (99 files changed), robust CI/CD integration with release gates, deterministic transcript-based verification, comprehensive smoke testing for both stdio and HTTP bridges, schema validation, and thorough documentation including operational runbook
  • No files require special attention

Important Files Changed

Filename Overview
scripts/verify-external-clients.ts Core verification harness for MCP protocol compliance testing across Claude Code and Cursor clients with comprehensive transcript recording and SLO validation
scripts/check-external-compat-gate.ts Release gate enforcement checking report freshness, client support status, and matrix alignment to block releases with stale/failed compatibility evidence
scripts/smoke-platform-workers.ts Worker lifecycle validation testing stdio/HTTP bridges, invalid JSON recovery, and command protocol correctness for both platforms
src/platform-worker.ts Platform worker process implementation with stdio + optional HTTP bridge supporting event/flush/health/shutdown commands
src/adapters/platform/normalize.ts Event normalization layer converting platform-specific event schemas (OpenCode/Claude Code/Cursor) into unified internal format
src/adapters/mcp/server.ts Enhanced MCP server with strict compatibility mode, parameter validation via Zod, and deterministic transcript support
.github/workflows/external-compat.yml Nightly/manual CI workflow for external compatibility verification on self-hosted macOS runner with artifact bundling
.github/workflows/release-gate.yml Release gate workflow blocking deployments when external compatibility evidence is stale or failing
docs/schemas/external-compat-report.schema.json JSON Schema v2020-12 defining strict structure for compatibility reports with SLO targets and failure taxonomy
tests/servers/mcp-transcripts.test.ts Transcript-based test harness loading JSON fixtures to validate MCP server behavior deterministically
tests/integration/platform-parity.test.ts Integration test validating OpenCode/Claude Code/Cursor adapters produce identical memory outcomes from equivalent event sequences
docs/platform-adapter-runbook.md Operational runbook documenting startup procedures, verification workflow, failure taxonomy remediation, and 7-day soak requirements

Sequence Diagram

sequenceDiagram
    participant CI as GitHub Actions
    participant Verify as verify-external-clients.ts
    participant MCP as MCP Server (dist/mcp.js)
    participant Worker as Platform Worker
    participant Smoke as smoke-platform-workers.ts
    participant Gate as check-external-compat-gate.ts
    participant Matrix as render-compat-matrix.ts

    Note over CI,Matrix: External Compatibility Verification Pipeline

    CI->>+Verify: Run compatibility verification
    
    loop For each client (claude-code, cursor)
        Verify->>Verify: Load MCP fixture from tests/fixtures/external-clients/{client}-mcp.json
        
        loop For each scenario
            Verify->>+MCP: Spawn MCP server process (stdio)
            Verify->>MCP: Send JSON-RPC messages
            MCP-->>-Verify: Return JSON-RPC responses
            Verify->>Verify: Validate assertions (responseId, contains checks)
            Verify->>Verify: Record pass/fail + durationMs
            Verify->>Verify: Write transcript to artifacts/transcripts/{client}/{scenario}.jsonl
        end
        
        Verify->>Verify: Calculate p95 latency for tool calls
        Verify->>Verify: Determine status (supported/expected-supported/failed)
    end
    
    Verify->>+Smoke: Run worker bridge smoke checks
    
    loop For each client (claude-code, cursor)
        Smoke->>Smoke: Load worker fixture from tests/fixtures/external-clients/{client}-worker.json
        
        Smoke->>+Worker: Spawn worker process (stdio mode)
        Smoke->>Worker: Send invalid JSON (recovery test)
        Worker-->>Smoke: Return INVALID_JSON error
        Smoke->>Worker: Send health command
        Worker-->>Smoke: Return status
        Smoke->>Worker: Send events
        Worker-->>Smoke: Return OK responses
        Smoke->>Worker: Send flush command
        Worker-->>Smoke: Return processed count
        Smoke->>Worker: Send shutdown command
        Worker-->>-Smoke: Return OK + graceful shutdown
        
        Smoke->>+Worker: Spawn worker process (HTTP mode)
        Smoke->>Worker: GET /v1/health
        Worker-->>Smoke: Return 200 + status
        Smoke->>Worker: POST /v1/events (with events)
        Worker-->>Smoke: Return 200 + OK
        Smoke->>Worker: POST /v1/events (flush)
        Worker-->>Smoke: Return 200 + processed count
        Smoke->>Worker: POST /v1/events (shutdown)
        Worker-->>-Smoke: Return 200 + shutdown confirmation
        
        Smoke->>Smoke: Calculate p95 latency for worker events
        Smoke->>Smoke: Write transcripts and error logs
    end
    
    Smoke-->>-Verify: Return smoke results
    
    Verify->>Verify: Aggregate all results + calculate SLO metrics
    Verify->>Verify: Validate report against JSON schema
    Verify->>Verify: Write external-compat-report.json
    Verify->>Verify: Write summary.md and checksums.txt
    Verify->>Verify: Copy report to docs/compatibility/external-compat-latest.json
    Verify-->>-CI: Return report (exit 1 if any failures)
    
    CI->>+Matrix: Render compatibility matrix
    Matrix->>Matrix: Read external-compat-report.json
    Matrix->>Matrix: Generate markdown table from report
    Matrix->>Matrix: Update docs/mcp-compatibility-matrix.md
    Matrix-->>-CI: Updated matrix file
    
    Note over CI,Gate: Release Gate (on release events)
    
    CI->>+Gate: Check external compatibility gate
    Gate->>Gate: Read docs/compatibility/external-compat-latest.json
    Gate->>Gate: Validate report schema
    Gate->>Gate: Check report freshness (< 7 days)
    Gate->>Gate: Check all clients have status=supported
    Gate->>Gate: Check all required scenarios passed
    Gate->>Gate: Read docs/mcp-compatibility-matrix.md
    Gate->>Gate: Verify matrix alignment with report
    Gate-->>-CI: Return OK or error list (exit 1 if errors)
    
    Note over CI: On failure, block release deployment
Loading

@clopca clopca merged commit 66d97b6 into main Feb 8, 2026
1 of 3 checks passed
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

Note

Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
src/config/store.ts (1)

302-325: ⚠️ Potential issue | 🟡 Minor

entityExtractionEnabled and userMemoryMaxContextTokens are in FIELD_SCHEMA but missing from ENV_BY_KEY.

Both fields have FIELD_SCHEMA entries (lines 266-281 and 290-299) but no corresponding environment variable mappings. This means:

  • getEffectiveConfig won't detect env overrides for these fields
  • Users can't toggle them via environment variables, unlike the other new fields

If this is intentional (e.g., file/programmatic-only config), consider adding a comment. Otherwise:

Proposed fix
 	userMemoryEnabled: ["OPEN_MEM_USER_MEMORY"],
+	userMemoryMaxContextTokens: ["OPEN_MEM_USER_MEMORY_MAX_CONTEXT_TOKENS"],
+	entityExtractionEnabled: ["OPEN_MEM_ENTITY_EXTRACTION"],
 };
README.md (1)

407-434: ⚠️ Potential issue | 🟡 Minor

Programmatic Configuration Reference table is missing the new platform/MCP fields.

The env var table (lines 400-405) documents OPEN_MEM_PLATFORM_OPENCODE, OPEN_MEM_PLATFORM_CLAUDE_CODE, OPEN_MEM_PLATFORM_CURSOR, OPEN_MEM_MCP_COMPAT_MODE, OPEN_MEM_MCP_PROTOCOL_VERSION, and OPEN_MEM_MCP_SUPPORTED_PROTOCOLS, but the corresponding programmatic options (platformOpenCodeEnabled, platformClaudeCodeEnabled, platformCursorEnabled, mcpCompatibilityMode, mcpProtocolVersion, mcpSupportedProtocolVersions) are absent from the programmatic reference table. Users who configure programmatically won't discover these options here.

🤖 Fix all issues with AI agents
In `@biome.json`:
- Line 2: Update the $schema URL in biome.json to a valid published schema
(change "https://biomejs.dev/schemas/2.3.14/schema.json" to
"https://biomejs.dev/schemas/2.3.11/schema.json" or point to the local schema
"./node_modules/@biomejs/biome/configuration_schema.json"); then confirm Biome
v2 migration completed by ensuring any organizeImports config was moved under
assist.actions.source.organizeImports (or run `biome migrate --write` to do it
automatically) and that any files.ignore rules were converted into negated
patterns in files.includes (e.g., include "**" plus negations like "!dist")
while remembering node_modules is ignored by default.

In `@scripts/check-external-compat-gate.ts`:
- Around line 60-72: Replace the duplicated status-to-display logic by importing
and using the existing formatStatus function from render-compat-matrix (or move
it to shared external-compat module) inside the loop that iterates
report.clients; call formatStatus(client.status) to produce expectedStatus
instead of the ternary chain, and likewise reuse the label derivation used by
renderGeneratedBlock (or extract a small helper for client label) so the
rowMustContain string uses the shared label and formatStatus outputs; update
imports accordingly and remove the duplicated conditional logic.

In `@src/adapters/http/server.ts`:
- Around line 219-228: The import endpoint currently maps only "replace" and
treats both "skip" and "merge" as "skip-duplicates", silently dropping merge
semantics; update the handler for app.post("/v1/memory/import") to explicitly
handle body.mode values: map "replace" -> "overwrite", "skip" ->
"skip-duplicates", and "merge" -> the correct merge behavior (e.g., pass mode:
"merge" to memoryEngine.import or perform merging logic inside the handler), and
if an unknown mode is provided return c.json(fail("VALIDATION_ERROR",
"Unsupported import mode"), 400); use the body.mode and memoryEngine.import
identifiers to locate and change the mapping logic.

In `@src/adapters/mcp/server.ts`:
- Around line 414-417: The current isError detection uses brittle string
matching on the serialized payload; instead modify the text() helper to return a
discriminated/structured result (e.g., { kind: "text", raw: string, parsed?:
unknown, isError?: boolean } or similar) so the caller can inspect parsed JSON
rather than searching substrings; update the try block that calls text() to use
the returned discriminant/parsed value to compute isError (e.g., check
parsed.error !== undefined && parsed.error !== null) and return the structured
content accordingly, referencing the text() function and the isError variable so
the downstream logic no longer relies on payload.includes(...) string matching.
- Around line 185-188: handleInitialize currently sets this.initialized inside
the initialize flow, which allows accepting tools/list and tools/call before the
client sends notifications/initialized; remove the premature this.initialized =
true from handleInitialize (and keep sending the initialize response via
this.send as-is) and ensure only the notifications/initialized handler sets
this.initialized (the existing handler that processes
"notifications/initialized" should flip the flag and mark the server ready), so
strict mode enforces the full MCP lifecycle.

In `@src/adapters/platform/bridge-client.ts`:
- Around line 107-124: getBridgeHealth currently calls parseBridgeResponse
without protection and can throw on non-JSON responses; wrap the
parseBridgeResponse(text) call in a try-catch (similar to sendBridgeHttpEvent)
and on parse failure return a BridgeResponse with ok: false, code indicating
invalid/non-JSON response (e.g., "INVALID_JSON" or "HTTP_ERROR"), include
resp.status and the raw text (or a truncated message) in message, and still
preserve final HTTP-error handling logic so getBridgeHealth returns a
well-formed BridgeResponse instead of propagating an exception.

In `@src/platform-worker.ts`:
- Around line 273-279: The code currently calls parseEnvelope(body) twice;
capture its result once in a local variable (e.g., const envelope =
parseEnvelope(body)) and pass that envelope to handleEnvelope(envelope) and use
envelope.command (with the same nullish/default logic) for the "shutdown" check;
update the block where handleEnvelope and the shutdown setTimeout are invoked so
they use the single envelope variable to avoid double-parsing and potential side
effects.

In `@tests/integration/platform-worker.test.ts`:
- Around line 23-24: The test hardcodes a local absolute cwd
("/Users/clopca/...") when calling Bun.spawn which breaks CI; change the
Bun.spawn invocation that creates proc to use a dynamic working directory (e.g.
process.cwd() or path.resolve(__dirname, "../../") / repo root) instead of the
hardcoded string so the tests run in the repository/workspace where the runner
executes; update the proc creation (Bun.spawn([...], { cwd: ... })) to reference
the dynamic value and ensure related variables entry and project still supply
the correct args.
🟡 Minor comments (13)
src/adapters/mcp/server.ts-306-312 (1)

306-312: ⚠️ Potential issue | 🟡 Minor

Only the first type filter is passed to search.

parsed.types?.[0] silently discards all but the first element of the types array. If a client sends types: ["decision", "bugfix"], only "decision" is used. Either pass the full array (if MemoryEngine.search can accept it) or document the limitation.

scripts/smoke-platform-workers.ts-186-192 (1)

186-192: ⚠️ Potential issue | 🟡 Minor

Read proc.stderr concurrently with awaiting process exit, not sequentially.

The current code reads proc.stderr only after await proc.exited (line 188-190). Bun's documentation recommends reading stderr and awaiting exit concurrently to ensure the stream is properly drained. The runStdioSmoke function (lines 97-99) already implements this pattern correctly—apply the same approach here:

proc.stdin.end();
const [stderr, exit] = await Promise.all([
  new Response(proc.stderr).text(),
  proc.exited,
]);
if (exit !== 0) {
  errors.push(`WORKER_HTTP_EXIT_${exit}:${stderr}`);
}
dashboard/src/components/Layout.tsx-9-10 (1)

9-10: ⚠️ Potential issue | 🟡 Minor

Ops and Settings share the same gear icon (⚙) — users won't be able to distinguish them visually.

\u{2699} (Ops) and \u2699\uFE0F (Settings) render identically or near-identically on most platforms. Consider picking a distinct icon for Ops (e.g., \u{1F6E0} 🛠 or \u{1F4E1} 📡) to avoid sidebar confusion.

src/adapters/opencode/tools.ts-129-143 (1)

129-143: ⚠️ Potential issue | 🟡 Minor

"merge" mode is accepted by the schema but unsupported by the engine—it silently degrades to "skip-duplicates".

Lines 134–135 map args.mode values to MemoryImportOptions.mode, which only accepts "skip-duplicates" | "overwrite". The schema (src/contracts/api.ts:92) defines mode: z.enum(["skip", "merge", "replace"]), but the underlying engine (src/core/memory-engine.ts:277–288) implements only "skip-duplicates" and "overwrite". The mapping args.mode === "replace" ? "overwrite" : "skip-duplicates" causes both "skip" and "merge" to collapse to the same behavior with no distinction. Either remove "merge" from the schema or add explicit engine support for it.

.github/workflows/external-compat.yml-28-31 (1)

28-31: ⚠️ Potential issue | 🟡 Minor

Scheduled runs will pass empty strings, not "unknown", for version env vars.

inputs.claude_code_version / inputs.cursor_version only receive the default: "unknown" when the workflow is triggered via workflow_dispatch. On schedule triggers, inputs is empty, so these env vars resolve to "".

If the verify script distinguishes empty from "unknown", this will produce unexpected behavior on nightly runs.

Proposed fix
       env:
-          OPEN_MEM_CLAUDE_CODE_VERSION: ${{ inputs.claude_code_version }}
-          OPEN_MEM_CURSOR_VERSION: ${{ inputs.cursor_version }}
+          OPEN_MEM_CLAUDE_CODE_VERSION: ${{ inputs.claude_code_version || 'unknown' }}
+          OPEN_MEM_CURSOR_VERSION: ${{ inputs.cursor_version || 'unknown' }}
docs/schemas/external-compat-report.schema.json-153-174 (1)

153-174: ⚠️ Potential issue | 🟡 Minor

failureTaxonomy requires exactly 4 entries (minItems: 4), forcing every report to include all failure categories.

Since the enum has exactly 4 codes, this effectively mandates that every report contains an entry for every failure category — even when a category doesn't apply. If a report only has one relevant failure (or none), it must still pad with 3+ placeholder entries to pass validation.

The TypeScript ExternalCompatReport interface defines failureTaxonomy as a plain Array<{...}> without a minimum. Consider relaxing or removing this constraint so reports only include relevant taxonomy entries.

🔧 Proposed fix
     "failureTaxonomy": {
       "type": "array",
-      "minItems": 4,
       "items": {
scripts/check-external-compat-gate.ts-11-22 (1)

11-22: ⚠️ Potential issue | 🟡 Minor

--freshness-days with non-numeric input silently produces NaN, bypassing the staleness check.

Number("abc") yields NaN, and ageMs > NaN is always false, so a malformed flag silently skips freshness validation. Add a guard.

🛡️ Proposed fix
 	return {
 		reportPath: pick("--report", "docs/compatibility/external-compat-latest.json"),
 		matrixPath: pick("--matrix", "docs/mcp-compatibility-matrix.md"),
-		freshnessDays: Number(pick("--freshness-days", "7")),
+		freshnessDays: (() => {
+			const n = Number(pick("--freshness-days", "7"));
+			if (Number.isNaN(n) || n <= 0) throw new Error("--freshness-days must be a positive number");
+			return n;
+		})(),
 	};
dashboard/src/components/TimelineItem.tsx-248-288 (1)

248-288: ⚠️ Potential issue | 🟡 Minor

Empty lineage renders a bare "Lineage" heading with no content.

When lineage is an empty array, neither the loading indicator, the error message, nor the items list renders — leaving a dangling section header. Consider showing a "No lineage" message or hiding the section entirely.

💡 Proposed addition for the empty state
 {lineage && lineage.length > 0 && (
   <div className="space-y-1.5">
     {/* ... existing items ... */}
   </div>
 )}
+{lineage && lineage.length === 0 && (
+  <p className="text-xs text-stone-400">No lineage history</p>
+)}
dashboard/src/components/TimelineItem.tsx-72-96 (1)

72-96: ⚠️ Potential issue | 🟡 Minor

No retry path after a lineage fetch error.

Once lineageError is set, the guard on Line 73 (lineage || lineageError) prevents any subsequent fetch, even if the user collapses and re-expands the item. The user is stuck with the error message.

Consider resetting lineageError when expanded transitions to false, or providing a retry button.

🔧 Minimal fix — reset error on collapse to allow retry
 useEffect(() => {
+	if (!expanded) {
+		setLineageError(null);
+		return;
+	}
-	if (!expanded || lineage || lineageError) return;
+	if (lineage) return;
 	let cancelled = false;
dashboard/src/types.ts-24-27 (1)

24-27: ⚠️ Potential issue | 🟡 Minor

Dashboard Observation is missing supersededAt compared to the backend type.

The backend Observation in src/types.ts (line 39) includes supersededAt?: string | null, but the dashboard type only adds revisionOf, deletedAt, and supersededBy. If the lineage UI (e.g., TimelineItem.tsx) needs to display when an observation was superseded, this field will be absent from the frontend type.

Proposed fix
 	revisionOf?: string | null;
 	deletedAt?: string | null;
 	supersededBy?: string | null;
+	supersededAt?: string | null;
 }
src/index.ts-258-274 (1)

258-274: ⚠️ Potential issue | 🟡 Minor

"degraded" status is sticky — any historical error permanently marks status as degraded.

snapshot.queue.lastError is never cleared once set in RuntimeMetricsCollector (see src/runtime/metrics.ts line 54). After the first failed item, lastError persists for the entire process lifetime, so the runtime status will read "degraded" indefinitely even if the system has fully recovered.

Consider either:

  • Clearing lastError after a configurable number of successful batches, or
  • Basing degraded status on a time window (e.g., error within the last N minutes), or
  • Tracking a separate errorCount and comparing against a threshold.
src/adapters/platform/normalize.ts-162-176 (1)

162-176: ⚠️ Potential issue | 🟡 Minor

Fallback callId uses Date.now(), which can collide under concurrent ingestion.

Line 170 generates ${platform}-${Date.now()} when callId is missing. Under high throughput, multiple events in the same millisecond would share the same ID. Consider using crypto.randomUUID() or appending a random suffix.

♻️ Proposed fix
+import { randomUUID } from "node:crypto";
+
 // ...
-		callId: asString(obj.callId) ?? `${platform}-${Date.now()}`,
+		callId: asString(obj.callId) ?? `${platform}-${randomUUID()}`,
src/platform-worker.ts-98-170 (1)

98-170: ⚠️ Potential issue | 🟡 Minor

No cleanup on initialization failure — database left open.

If any step after createDatabase throws (e.g., initializeSchema, adapter creation), the opened database handle leaks. Consider wrapping initialization in a try/catch that closes the DB on failure.

♻️ Proposed fix
 function initialize(platform: PlatformName, projectDir: string): WorkerState {
 	const projectPath = getCanonicalProjectPath(projectDir);
 	const config = resolveConfig(projectPath);
 	assertAdapterEnabled(platform, config);
 
 	Database.enableExtensionSupport();
 	const db = createDatabase(config.dbPath);
-	initializeSchema(db, {
-		hasVectorExtension: db.hasVectorExtension,
-		embeddingDimension: config.embeddingDimension,
-	});
+	try {
+		initializeSchema(db, {
+			hasVectorExtension: db.hasVectorExtension,
+			embeddingDimension: config.embeddingDimension,
+		});
 	// ... rest of initialization ...
+	} catch (err) {
+		db.close();
+		throw err;
+	}
🧹 Nitpick comments (48)
src/store/sqlite/adapters.ts (1)

19-21: Consider consistent formatting across all adapter functions.

The multi-line formatting here differs from the other three adapter functions in this file (lines 7, 11, 15), which use single-line parameter declarations. For consistency, consider either applying multi-line formatting to all four functions or keeping them all single-line.

♻️ Optional: Apply consistent single-line formatting
-export function createUserObservationStore(
-	repo: UserObservationRepository | null,
-): UserObservationStore | null {
+export function createUserObservationStore(repo: UserObservationRepository | null): UserObservationStore | null {
scripts/benchmark-platform-normalization.ts (2)

7-19: Consider asserting non-null results to catch silent regressions.

If the normalization functions' contracts change and they start returning null for these inputs, the benchmark would silently measure no-op iterations without surfacing the problem.

💡 Suggested improvement
 for (let i = 0; i < count; i++) {
-  normalizeOpenCodeEvent({
+  const a = normalizeOpenCodeEvent({
     eventType: "tool.execute.after",
     payload: { sessionID: `s-${i}`, callID: `c-${i}`, tool: "Read" },
     output: { output: "content", title: "Read" },
   });
-  normalizeExternalEvent("cursor", {
+  const b = normalizeExternalEvent("cursor", {
     kind: "chat.message",
     sessionId: `s-${i}`,
     text: `message-${i}`,
     role: "user",
   });
+  if (!a || !b) throw new Error(`Normalization returned null at iteration ${i}`);
 }

23-23: Long line reduces readability — consider breaking up the output.

💡 Suggested improvement
-console.log(JSON.stringify({ events: count * 2, totalMs: Number(elapsed.toFixed(2)), avgUs: Number(((elapsed * 1000) / (count * 2)).toFixed(2)) }, null, 2));
+const totalEvents = count * 2;
+console.log(JSON.stringify({
+  totalEvents,
+  totalMs: Number(elapsed.toFixed(2)),
+  avgUs: Number(((elapsed * 1000) / totalEvents).toFixed(2)),
+}, null, 2));
scripts/benchmark-search.ts (2)

14-14: Unused SummaryRepository instance.

This is instantiated but never assigned or referenced. Remove it to avoid confusion.

Proposed fix
-new SummaryRepository(db);

39-52: p95 over 4 samples is just the max — consider running more iterations or dropping the metric.

With only 4 queries, Math.floor(4 * 0.95) = index 3, which is simply the maximum value. The "p95" label is misleading. Either run each query multiple times (e.g., 50+ iterations) to get meaningful percentiles, or report min/max alongside the average instead.

scripts/detect-client-versions.ts (2)

19-26: Consider adding a timeout to Bun.spawnSync calls.

If a probed command hangs (e.g., cursor --version launching a GUI and waiting for input), spawnSync blocks the entire process indefinitely. Bun supports a timeout option on spawnSync.

🛡️ Suggested hardening
-function tryCommand(command: string[], source: string): { version: string; source: string } | null {
-	const proc = Bun.spawnSync(command, { stdout: "pipe", stderr: "pipe" });
+function tryCommand(command: string[], source: string): { version: string; source: string } | null {
+	const proc = Bun.spawnSync(command, { stdout: "pipe", stderr: "pipe", timeout: 10_000 });

80-85: main() is declared async but performs no asynchronous work.

detectClientVersions() is fully synchronous, so the async qualifier on main and the .catch() guard at line 88 are superfluous. Not harmful, but slightly misleading.

scripts/verify-external-clients-auto.ts (1)

7-17: parseArgs silently accepts malformed flags.

If --artifacts-dir is passed without a value (as the last argument), args[index + 1] is undefined and the fallback kicks in — which is fine. However, if a flag like --artifacts-dir --project ./foo is passed (missing the dir value), the artifacts directory would be set to "--project". Consider adding a guard.

scripts/smoke-platform-workers.ts (2)

158-158: Hardcoded 250ms sleep is fragile for HTTP readiness.

If the worker takes longer than 250ms to bind its HTTP port (e.g., under CI load or slow disk I/O for fixture loading), the first fetch at line 173 will fail with a connection error, producing a misleading WORKER_HTTP_HEALTH_FAILED. Consider polling the health endpoint with a short retry loop instead.

🔧 Suggested retry-based readiness check
-	await Bun.sleep(250);
+	const maxRetries = 20;
+	for (let i = 0; i < maxRetries; i++) {
+		try {
+			const res = await fetch(`${baseUrl}/v1/health`);
+			if (res.ok) break;
+		} catch {
+			// not ready yet
+		}
+		await Bun.sleep(100);
+	}

Note: if you adopt this, you'd also need to adjust the health check at line 173–174 to avoid double-checking health, or simply keep it as a validation assertion after the readiness loop.


89-94: Stdio durations measure write latency, not round-trip time.

The performance.now() delta only captures how long proc.stdin.write() takes to return, not how long the worker takes to process and respond. This is acknowledged by the comment at line 235, but the p95Ms value returned from runStdioSmoke could be confusing to consumers. Consider renaming the field (e.g., writeP95Ms) or documenting it in the SmokeResult interface.

scripts/external-compat.ts (2)

129-132: readJsonFile uses an unsafe as T cast without runtime validation.

The caller gets no guarantee that the parsed JSON matches T. For internal tooling this is acceptable, but if these utilities are used to read untrusted or externally-authored files (e.g., test fixtures from contributors), a malformed file would produce confusing errors downstream rather than at the parse site.


158-174: Validation covers structural completeness but not semantic correctness.

The validator checks field presence and minimum counts but does not validate:

  • generatedAt as a valid ISO 8601 string
  • schemaVersion format
  • summary or slo field consistency (e.g., failedScenarios <= scenarioCount)

This is fine for a first pass, but consider tightening validation if this becomes a contract boundary (e.g., consumed by CI gates).

.github/workflows/release-gate.yml (1)

12-15: Consider pinning Bun version and using --frozen-lockfile.

For a release-blocking gate, reproducibility matters. The Bun version isn't pinned (risking behavior drift), and bun install without --frozen-lockfile could silently update the lockfile instead of failing on mismatch.

Proposed fix
       - uses: actions/checkout@v4
-      - uses: oven-sh/setup-bun@v2
+      - uses: oven-sh/setup-bun@v2
+        with:
+          bun-version: "1.2.x"
       - name: Install
-        run: bun install
+        run: bun install --frozen-lockfile
docs/mcp-compatibility-matrix.md (1)

17-23: Minor: Protocol Version column uses right-alignment for date strings.

Line 36 of scripts/render-compat-matrix.ts uses ---: (right-align) for the "Protocol Version" column, but the values are date strings (2024-11-05), not numeric. Change to --- (center) to match the semantic nature of the data.

tests/adapters/platform-bridge-client.test.ts (1)

46-59: fakeFetch doesn't validate the outgoing request.

The fake ignores its arguments, so the test only verifies response parsing — not that sendBridgeHttpEvent sends the correct URL, method, headers, or body. Consider asserting on the request parameters if you want to verify the full contract.

tests/servers/http-server.test.ts (1)

245-268: Hardcoded platform count may be brittle.

expect(payload.data.platforms.length).toBe(3) will break if a new platform adapter is added. This is acceptable as an intentional contract test, but consider adding a comment noting the expected platforms to make future updates easier.

src/hooks/chat-capture.ts (1)

136-150: Duplicate agent guard is intentional but worth a brief comment.

The agent check on line 139 short-circuits before text extraction (good for performance), while persistChatMessage re-checks on line 74 for safety when called directly. Consider adding a short inline comment on line 139 noting it's an early-exit optimization, since the public function also validates.

.github/workflows/external-compat.yml (1)

22-22: Consider pinning Bun version in setup-bun.

Using oven-sh/setup-bun@v2 without a bun-version input means CI will use whatever the latest is. A breaking Bun upgrade could cause spurious failures in the compatibility workflow. Pinning (e.g., bun-version: "1.3.0") keeps results reproducible.

docs/compatibility/external-compat-latest.json (1)

1-14: Committed report will go stale — nightly workflow doesn't auto-update it.

The external-compat.yml workflow uploads the report as a CI artifact but doesn't commit the updated external-compat-latest.json back to the repo. With freshnessDays: 7, the release gate (compat:gate) will start rejecting this committed snapshot within a week unless there's a separate mechanism to refresh it.

Consider either having the nightly workflow commit the updated report or documenting that external-compat-latest.json must be manually refreshed before releases.

tests/fixtures/external-clients/cursor-mcp.json (1)

1-55: Fixture is identical to claude-code-mcp.json except for "client": "cursor".

If both clients are expected to behave identically for MCP protocol compliance, this duplication is acceptable. However, if Cursor has any client-specific quirks or different error message formats, consider adding differentiated scenarios. The memory.help / memory.find concern noted in the Claude Code fixture review applies here as well (Line 12 vs Line 18).

scripts/render-compat-matrix.ts (3)

13-22: formatStatus default branch silently absorbs unknown statuses.

Any unrecognized status string (e.g., a future "degraded" value or a typo) silently maps to "Expected Supported". Consider making the "expected-supported" case explicit and logging/throwing on truly unknown values.

🔧 Proposed fix
 function formatStatus(status: string): string {
 	switch (status) {
 		case "supported":
 			return "Supported";
+		case "expected-supported":
+			return "Expected Supported";
 		case "failed":
 			return "Failed";
 		default:
-			return "Expected Supported";
+			return status;
 	}
 }

33-40: Markdown table alignment specifier for "Protocol Version" column is right-aligned (---:).

Line 36 uses ---: for the Protocol Version column, which right-aligns its content. Protocol versions are date-like strings (e.g., 2024-11-05), not numeric values — left-alignment (---) would be more conventional.

📝 Proposed fix
-		"|---|---|---:|---|---|---|---|---|",
+		"|---|---|---|---|---|---|---|---|",

24-41: Redundant lookup: report.clients.find(...) duplicates data already in summarizeMatrixClients output.

renderGeneratedBlock calls summarizeMatrixClients(report) to get client summaries, then on Line 29 re-searches report.clients to find protocolVersion. Consider either adding protocolVersion to MatrixClientSummary upstream or passing it alongside the summary, to avoid the duplicated lookup.

scripts/check-external-compat-gate.ts (1)

24-75: Gate function doesn't validate readJsonFile / readFile failures gracefully.

If the report file or matrix file is missing, the function throws an unhandled ENOENT error. Since this is a release gate that CI depends on, wrapping these I/O calls and pushing a descriptive error (e.g., REPORT_MISSING / MATRIX_MISSING) into the errors array — instead of crashing — would produce more actionable CI output.

tests/servers/mcp-server.test.ts (1)

29-33: Minor robustness concern: proc.kill() called without awaiting process exit.

callServer reads stdout then immediately kills the process. If the subprocess has already exited, kill() on a dead process is a no-op in Bun, so this is safe in practice. However, awaiting proc.exited before killing would be cleaner and avoids potential resource leaks in edge cases.

🔧 Proposed fix
 	const output = await new Response(proc.stdout).text();
-	proc.kill();
+	await proc.exited;
dashboard/src/pages/Ops.tsx (1)

25-34: Error and loading states can render simultaneously.

The error and loading conditionals are independent, so if one API call fails while another is still in-flight, both the error banner and the loading indicator will appear at the same time. Consider showing the error only after loading completes, or suppressing the loading indicator once an error is present.

🔧 Proposed fix
-      {error && (
+      {error && !loading && (
         <div className="mb-6 rounded-xl border border-red-200 bg-red-50 px-5 py-4">
tests/servers/mcp-transcripts.test.ts (2)

91-116: Fixtures with no assertion fields pass vacuously.

If a fixture defines only name and messages but omits expected, expectedContains, and outputContains, the test body executes callServer and then succeeds without checking anything. Consider adding a guard to flag or fail such fixtures.

🛡️ Proposed guard
 	for (const fixture of fixtures) {
 		test(fixture.name, async () => {
+			const hasAssertions = fixture.expected || fixture.expectedContains || fixture.outputContains;
+			expect(hasAssertions).toBeTruthy();
+
 			const responses = await callServer(fixture.messages);

29-49: callServer helper mirrors mcp-server.test.ts — consider extracting a shared utility.

Both tests/servers/mcp-server.test.ts and this file define a nearly identical callServer function that spawns a helper, streams stdin, and collects stdout lines. Extracting this to a shared test helper (e.g., tests/servers/helpers.ts) would reduce duplication and ensure consistent behavior across test suites.

src/config.ts (2)

141-151: Asymmetric env var parsing prevents re-affirming the default.

platformOpenCodeEnabled can only be set to false, while platformClaudeCodeEnabled / platformCursorEnabled can only be set to true. This means a user cannot explicitly set OPEN_MEM_PLATFORM_OPENCODE=true or OPEN_MEM_PLATFORM_CLAUDE_CODE=false via environment to match the default — which is harmless today but inconsistent with how a user might expect boolean env overrides to work. Consider parsing both "true" and "false" for all three.

♻️ Proposed fix for symmetric boolean parsing
-	if (process.env.OPEN_MEM_PLATFORM_OPENCODE === "false") env.platformOpenCodeEnabled = false;
-	if (process.env.OPEN_MEM_PLATFORM_CLAUDE_CODE === "true") env.platformClaudeCodeEnabled = true;
-	if (process.env.OPEN_MEM_PLATFORM_CURSOR === "true") env.platformCursorEnabled = true;
+	if (process.env.OPEN_MEM_PLATFORM_OPENCODE)
+		env.platformOpenCodeEnabled = process.env.OPEN_MEM_PLATFORM_OPENCODE === "true";
+	if (process.env.OPEN_MEM_PLATFORM_CLAUDE_CODE)
+		env.platformClaudeCodeEnabled = process.env.OPEN_MEM_PLATFORM_CLAUDE_CODE === "true";
+	if (process.env.OPEN_MEM_PLATFORM_CURSOR)
+		env.platformCursorEnabled = process.env.OPEN_MEM_PLATFORM_CURSOR === "true";

144-145: mcpCompatibilityMode accepts any string without validation.

The as "strict" | "legacy" cast silently accepts invalid values (e.g., OPEN_MEM_MCP_COMPAT_MODE=foo). Consider validating and falling back to the default.

🛡️ Proposed validation
-	if (process.env.OPEN_MEM_MCP_COMPAT_MODE)
-		env.mcpCompatibilityMode = process.env.OPEN_MEM_MCP_COMPAT_MODE as "strict" | "legacy";
+	if (process.env.OPEN_MEM_MCP_COMPAT_MODE) {
+		const mode = process.env.OPEN_MEM_MCP_COMPAT_MODE;
+		if (mode === "strict" || mode === "legacy") {
+			env.mcpCompatibilityMode = mode;
+		}
+	}
dashboard/src/components/TimelineItem.tsx (1)

76-78: Unnecessary Content-Type header on a GET request.

The fetch to the lineage endpoint doesn't include a request body, so the Content-Type: application/json header is superfluous. While harmless, it can be removed for clarity. You may want Accept: application/json instead if the server performs content negotiation.

src/search/hybrid.ts (2)

250-261: Redundant spread + explicit field re-declaration in the final map.

The spread ...(result.explain ?? ...) is immediately overwritten by every individual field below it, making the spread effectively a no-op. This works correctly but is slightly misleading — a reader might expect the spread to contribute additional fields.

Not a functional issue; leaving it is fine for forward-compatibility if new explain fields are added later.

♻️ Simplified version without redundant spread
 .map(({ score, result }) => ({
 	...result,
 	explain: {
-		...(result.explain ?? { strategy: "hybrid", matchedBy: [] }),
 		strategy: "hybrid" as const,
 		matchedBy: result.explain?.matchedBy ?? [],
 		rrfScore: score,
 		ftsRank: result.explain?.ftsRank,
 		vectorDistance: result.explain?.vectorDistance,
 		vectorSimilarity: result.explain?.vectorSimilarity,
 	},
 }));

199-213: FTS results' original explain metadata (if any) is discarded.

When FTS results enter mergeWithRRF, any pre-existing explain on r is replaced with a new object containing only ftsRank. If upstream FTS enrichment ever populates explain (e.g., from the orchestrator's concept-filter or file-filter paths), that data would be lost here.

Currently this is fine since safelyRunFts returns raw FTS results without explain, but worth keeping in mind.

tests/integration/platform-parity.test.ts (1)

26-26: Module-level resources array couples cleanup to a single describe block.

If additional describe blocks are added to this file in the future, the afterEach in the current block won't clean up databases created by other blocks (or vice versa). Consider scoping resources inside the describe, or using beforeEach/afterEach at the file level.

This is fine for now since there's only one describe block.

src/adapters/platform/bridge-client.ts (2)

61-64: Shallow validation of BridgeStatus — cast without verifying internal shape.

Line 63 casts obj.status to BridgeStatus after only checking it's a non-null, non-array object. If the server returns {"status": {"unexpected": true}}, the consumer gets a BridgeStatus with undefined for platform, projectPath, and queue. This could cause subtle downstream errors.

Consider validating at least the platform and queue fields, or documenting that consumers should handle partial data.


75-105: Consider adding a timeout to sendBridgeHttpEvent and getBridgeHealth.

Neither HTTP function sets a timeout or AbortSignal. If the bridge server is unresponsive, callers will hang indefinitely. This is especially relevant for health checks, which are often called periodically.

Example using AbortSignal.timeout
 export async function sendBridgeHttpEvent(
 	baseUrl: string,
 	envelope: BridgeEnvelope,
 	fetchImpl: typeof fetch = fetch,
+	timeoutMs = 10_000,
 ): Promise<BridgeResponse> {
 	const url = `${baseUrl.replace(/\/$/, "")}/v1/events`;
 	const resp = await fetchImpl(url, {
 		method: "POST",
 		headers: { "Content-Type": "application/json" },
 		body: JSON.stringify(envelope),
+		signal: AbortSignal.timeout(timeoutMs),
 	});
package.json (1)

24-31: Consider extracting the repeated --external flags into a shared variable or script.

Every build:* command repeats the same set of --external flags. This is a pre-existing pattern, but with two more targets added, the maintenance surface grows. A helper script or a shared variable (e.g., in a bunfig.toml or a small build orchestrator) would reduce duplication and the risk of forgetting an external when adding new targets.

tests/integration/platform-worker.test.ts (1)

39-44: Potential deadlock: stderr is not consumed concurrently with stdout.

stdout is fully read on line 39 before stderr is read (conditionally) on line 42. If the child process writes enough data to stderr to fill the OS pipe buffer while stdout is still being consumed, the child can block, causing a deadlock. While unlikely with small test outputs, it's a latent risk.

Read both streams concurrently:

🔧 Suggested fix
-	const stdout = await new Response(proc.stdout).text();
-	const code = await proc.exited;
-	if (code !== 0) {
-		const stderr = await new Response(proc.stderr).text();
-		throw new Error(`worker failed (${entry}): ${stderr}`);
-	}
+	const [stdout, stderr, code] = await Promise.all([
+		new Response(proc.stdout).text(),
+		new Response(proc.stderr).text(),
+		proc.exited,
+	]);
+	if (code !== 0) {
+		throw new Error(`worker failed (${entry}): ${stderr}`);
+	}
src/adapters/platform/runtime.ts (1)

58-93: Missing exhaustive check — unknown event.kind values are silently ignored.

The switch on event.kind has no default case. If NormalizedPlatformEvent gains new kind values in the future, they will be silently dropped without any indication. Consider adding a default clause that logs an unhandled event kind or using a TypeScript exhaustiveness check:

🔧 Suggested exhaustiveness guard
 		case "chat.message":
 			persistChatMessage({
 				observations: this.observations,
 				sessions: this.sessions,
 				projectPath: this.projectPath,
 				sessionId: event.sessionId,
 				text: event.text,
 				agent: event.role === "user" ? "user" : event.role,
 				sensitivePatterns: this.config.sensitivePatterns,
 			});
 			return;
+		default: {
+			const _exhaustive: never = event;
+			console.warn(`[open-mem] Unhandled platform event kind: ${(event as any).kind}`);
+		}
 	}
scripts/verify-external-clients.ts (1)

71-77: Same sequential stream reading pattern — potential deadlock risk.

Like in the test file, stdout is read first (line 74), then stderr (line 75), then proc.exited (line 76). If the child process fills its stderr pipe buffer before stdout is fully consumed, it will block waiting on the pipe, creating a deadlock.

🔧 Suggested fix
-	proc.stdin.write(input);
-	proc.stdin.end();
-
-	const stdout = await new Response(proc.stdout).text();
-	const stderr = await new Response(proc.stderr).text();
-	const exit = await proc.exited;
+	proc.stdin.write(input);
+	proc.stdin.end();
+
+	const [stdout, stderr, exit] = await Promise.all([
+		new Response(proc.stdout).text(),
+		new Response(proc.stderr).text(),
+		proc.exited,
+	]);
src/platform-worker.ts (3)

254-297: new URL(req.url) is constructed twice for the same request.

Lines 260 and 263 each create a new URL(req.url) to extract the pathname. Parse it once and reuse.

♻️ Proposed fix
 		fetch: async (req) => {
-			if (req.method === "GET" && new URL(req.url).pathname === "/v1/health") {
-				return Response.json(healthResponse(state));
-			}
-			if (req.method === "POST" && new URL(req.url).pathname === "/v1/events") {
+			const pathname = new URL(req.url).pathname;
+			if (req.method === "GET" && pathname === "/v1/health") {
+				return Response.json(healthResponse(state));
+			}
+			if (req.method === "POST" && pathname === "/v1/events") {

69-84: Number.parseInt is called twice for rawHttpPort.

Lines 80 and 81 both parse the same string. Minor, but easy to clean up.

♻️ Proposed fix
 	const rawHttpPort = values["http-port"];
-	const httpPort =
-		typeof rawHttpPort === "string" && Number.parseInt(rawHttpPort, 10) > 0
-			? Number.parseInt(rawHttpPort, 10)
-			: undefined;
+	const parsedPort = typeof rawHttpPort === "string" ? Number.parseInt(rawHttpPort, 10) : NaN;
+	const httpPort = parsedPort > 0 ? parsedPort : undefined;

221-230: Swallowed error in processBatch during shutdown may hide data loss.

Line 226 silently catches any error from state.queue.processBatch(). If the final batch fails, the worker exits successfully without any indication that data may have been lost. Consider at least logging the error before exiting.

♻️ Proposed fix
 	const shutdown = async () => {
 		if (shuttingDown) return;
 		shuttingDown = true;
 		try {
 			await state.queue.processBatch();
-		} catch {}
+		} catch (err) {
+			console.error("[open-mem] Final batch processing failed during shutdown:", err);
+		}
 		state.queue.stop();
 		state.db.close();
 		process.exit(0);
 	};
src/adapters/platform/normalize.ts (1)

81-119: Collapse duplicate branches for session.started/session.created and session.ended/session.completed.

Lines 81–99 and 101–119 contain two pairs of identical blocks that differ only in the eventType string. Combine them to reduce duplication.

♻️ Proposed fix
-	if (eventType === "session.started") {
-		return {
-			kind: "session.start",
-			platform: "opencode",
-			sessionId,
-			occurredAt: nowIso(),
-			metadata: props ?? undefined,
-		};
-	}
-
-	if (eventType === "session.created") {
-		return {
-			kind: "session.start",
-			platform: "opencode",
-			sessionId,
-			occurredAt: nowIso(),
-			metadata: props ?? undefined,
-		};
-	}
-
-	if (eventType === "session.ended") {
-		return {
-			kind: "session.end",
-			platform: "opencode",
-			sessionId,
-			occurredAt: nowIso(),
-			metadata: props ?? undefined,
-		};
-	}
-
-	if (eventType === "session.completed") {
-		return {
-			kind: "session.end",
-			platform: "opencode",
-			sessionId,
-			occurredAt: nowIso(),
-			metadata: props ?? undefined,
-		};
-	}
+	if (eventType === "session.started" || eventType === "session.created") {
+		return {
+			kind: "session.start",
+			platform: "opencode",
+			sessionId,
+			occurredAt: nowIso(),
+			metadata: props ?? undefined,
+		};
+	}
+
+	if (eventType === "session.ended" || eventType === "session.completed") {
+		return {
+			kind: "session.end",
+			platform: "opencode",
+			sessionId,
+			occurredAt: nowIso(),
+			metadata: props ?? undefined,
+		};
+	}
src/adapters/http/server.ts (3)

234-287: Duplicated runtime status fallback object in /v1/health and /v1/metrics.

The identical 12-line default object is repeated on lines 236–252 and 269–285. Extract it into a shared helper or constant to keep the two endpoints consistent and reduce maintenance burden.

♻️ Proposed fix
+function defaultRuntimeStatus() {
+	return {
+		status: "ok" as const,
+		timestamp: new Date().toISOString(),
+		uptimeMs: process.uptime() * 1000,
+		queue: {
+			mode: "in-process",
+			running: false,
+			processing: false,
+			pending: 0,
+			lastBatchDurationMs: 0,
+			lastProcessedAt: null,
+			lastFailedAt: null,
+			lastError: null,
+		},
+		batches: { total: 0, processedItems: 0, failedItems: 0, avgDurationMs: 0 },
+		enqueueCount: 0,
+	};
+}
+
 app.get("/v1/health", (c) => {
 	const stats = memoryEngine.stats();
-	const runtime = runtimeStatusProvider?.() ?? {
-		status: "ok" as const,
-		// ... 12 lines ...
-	};
+	const runtime = runtimeStatusProvider?.() ?? defaultRuntimeStatus();
 	// ...
 });
 
 app.get("/v1/metrics", (c) => {
-	const runtime = runtimeStatusProvider?.() ?? {
-		status: "ok" as const,
-		// ... 12 lines ...
-	};
+	const runtime = runtimeStatusProvider?.() ?? defaultRuntimeStatus();
 	// ...
 });

70-80: redactConfig uses a broad heuristic that may over-redact.

Any string field whose key contains "key" or "api" (case-insensitive) is redacted. This would also redact legitimate non-sensitive fields if their names happen to match (e.g., a hypothetical "hotkey" or "apiVersion"). Consider using an explicit allowlist of sensitive field names instead.


101-122: Request body is cast with as without runtime validation.

Throughout the endpoints (e.g., lines 103–112, 146–152, 198, 221, 322, 338), request bodies are cast to expected types using as with no schema validation. Malformed input that structurally passes JSON parsing but has wrong types (e.g., importance: "high" instead of a number) will flow through unchecked. Consider using Zod schemas from your contracts layer for runtime validation on at least the write endpoints.

src/queue/processor.ts (1)

439-444: Add a getCount() method to avoid fetching 1000 rows just to count pending items.

The getStats() method calls getPending(1000).length, which fetches and deserializes all columns for up to 1000 rows only to discard them. This wastes I/O and memory. Add a getCount() method to PendingMessageRepository that executes SELECT COUNT(*) FROM pending_messages WHERE status = 'pending' instead.

Comment thread biome.json
"organizeImports": {
"enabled": true
},
"$schema": "https://biomejs.dev/schemas/2.3.14/schema.json",
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🌐 Web query:

Biome v2.0 migration guide organizeImports breaking changes

💡 Result:

Biome v2.0 migration: organizeImports breaking changes (and what to do)

1) Config location changed (breaking)

  • In Biome v1, import organizing was configured under a top-level organizeImports field.
  • In Biome v2, that configuration moved to an assist code action: assist.actions.source.organizeImports. The biome migrate --write command rewrites your config accordingly. [1]

Example migration (what biome migrate does):

// v1
{
  "organizeImports": { "enabled": true }
}

// v2
{
  "assist": { "actions": { "source": { "organizeImports": "on" } } }
}

[1]


2) Import sorting behavior changed (breaking)
Biome v2 ships a new import organizer, which can change your diffs because it now: supports customizable sorting, organizes export statements too, ignores blank lines between imports, and can merge import/export ordering more consistently. [1][2]

A specific behavior change called out in the v2 upgrade guide:

  • Node.js modules without the node: prefix are no longer forced ahead of other package imports under the default ordering. [1]

So a v1 order like:

import fs from "node:fs";
import path from "path";
import pkg from "@a/package";

may become (v2 default):

import fs from "node:fs";
import pkg from "@a/package";
import path from "path";

[1]


3) How to keep (roughly) the old grouping
Biome v2 lets you define a custom group order under:
assist.actions.source.organizeImports.options.groups (the upgrade guide provides an example, including :NODE: to group Node built-ins). Note Biome explicitly warns you can’t reproduce v1 behavior exactly, but you can get close via custom groups. [1][2]


References

  • [1] Biome “Upgrade to Biome v2” guide (breaking changes + organizeImports migration + Node import ordering note)
  • [2] Biome assist action docs: organizeImports (options like groups, identifier ordering, chunking imports/exports)

🌐 Web query:

Biome v2 files.ignore default configuration

💡 Result:

In Biome v2, there is no files.ignore option. File exclusion is done via negated patterns inside files.includes (and “force-ignore” with !!). For example: ["**", "!**/*.test.js", "!!**/dist"]. [1]

So the “default files.ignore” is effectively “nothing configured” (because the setting doesn’t exist in v2). By default, Biome still always ignores node_modules/ regardless of files.includes. [1]

Also, Biome has some always-ignored “protected files” (no diagnostics are emitted for them): composer.lock, npm-shrinkwrap.json, package-lock.json, yarn.lock. [2]

Sources:
[1] Biome v2 configuration reference (files.includes, negation/force-ignore, node_modules note) (biomejs.dev)
[2] Biome “Configure Biome” guide (protected files list) (biomejs.dev)

Citations:


🌐 Web query:

Biome 2.3.14 schema release version

💡 Result:

For Biome CLI 2.3.14, the latest schema version published on biomejs.dev (i.e., the version used in https://biomejs.dev/schemas/<version>/schema.json) is 2.3.11—that’s the version currently shown in Biome’s official configuration docs. [1]

If you want to avoid the hosted-schema version mismatch entirely, point $schema to the local schema shipped with the npm package instead: ./node_modules/@biomejs/biome/configuration_schema.json. [1]

Sources:
[1] Biome docs – Configuration ($schema example shows https://biomejs.dev/schemas/2.3.11/schema.json)
[2] Arch Linux package entry confirming Biome 2.3.14 exists as a released package


Fix schema version mismatch and verify Biome v2 config migration is complete.

The $schema URL references version 2.3.14, but the latest published schema is 2.3.11 — this will cause schema validation failures. Update to https://biomejs.dev/schemas/2.3.11/schema.json or use the local schema: ./node_modules/@biomejs/biome/configuration_schema.json.

Additionally, removal of organizeImports and files.ignore blocks is a breaking change in Biome v2. Verify these were properly migrated:

  • organizeImports → moved to assist.actions.source.organizeImports (use biome migrate --write if not already done)
  • files.ignore → replaced with negated patterns in files.includes (e.g., ["**", "!dist", "!node_modules"]). Note: node_modules is ignored by default in v2.
🤖 Prompt for AI Agents
In `@biome.json` at line 2, Update the $schema URL in biome.json to a valid
published schema (change "https://biomejs.dev/schemas/2.3.14/schema.json" to
"https://biomejs.dev/schemas/2.3.11/schema.json" or point to the local schema
"./node_modules/@biomejs/biome/configuration_schema.json"); then confirm Biome
v2 migration completed by ensuring any organizeImports config was moved under
assist.actions.source.organizeImports (or run `biome migrate --write` to do it
automatically) and that any files.ignore rules were converted into negated
patterns in files.includes (e.g., include "**" plus negations like "!dist")
while remembering node_modules is ignored by default.

Comment on lines +60 to +72
for (const client of report.clients) {
const label = client.name === "claude-code" ? "Claude Code MCP integration" : "Cursor MCP integration";
const expectedStatus =
client.status === "supported"
? "Supported"
: client.status === "failed"
? "Failed"
: "Expected Supported";
const rowMustContain = `${label} | stdio | 2024-11-05 | ${client.version.detected} | ${expectedStatus}`;
if (!matrix.includes(rowMustContain)) {
errors.push(`MATRIX_MISMATCH:${client.name}: expected row fragment '${rowMustContain}'`);
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Duplicated status-formatting logic — will silently diverge from render-compat-matrix.ts.

Lines 62-67 re-implement the same status → display label mapping that formatStatus performs in render-compat-matrix.ts. If either copy is updated independently, the gate will report false MATRIX_MISMATCH errors. Import and reuse formatStatus instead.

♻️ Proposed fix

Import formatStatus from render-compat-matrix.ts (or move it to the shared external-compat.ts module):

 import {
 	readJsonFile,
 	toDate,
 	validateExternalCompatReport,
 	type ExternalCompatReport,
 } from "./external-compat";
+import { formatStatus } from "./render-compat-matrix";

Then simplify:

 	for (const client of report.clients) {
 		const label = client.name === "claude-code" ? "Claude Code MCP integration" : "Cursor MCP integration";
-		const expectedStatus =
-			client.status === "supported"
-				? "Supported"
-				: client.status === "failed"
-					? "Failed"
-					: "Expected Supported";
+		const expectedStatus = formatStatus(client.status);
 		const rowMustContain = `${label} | stdio | 2024-11-05 | ${client.version.detected} | ${expectedStatus}`;

The label derivation (Lines 61) is also duplicated from renderGeneratedBlock in render-compat-matrix.ts — consider extracting that as well.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
for (const client of report.clients) {
const label = client.name === "claude-code" ? "Claude Code MCP integration" : "Cursor MCP integration";
const expectedStatus =
client.status === "supported"
? "Supported"
: client.status === "failed"
? "Failed"
: "Expected Supported";
const rowMustContain = `${label} | stdio | 2024-11-05 | ${client.version.detected} | ${expectedStatus}`;
if (!matrix.includes(rowMustContain)) {
errors.push(`MATRIX_MISMATCH:${client.name}: expected row fragment '${rowMustContain}'`);
}
}
import {
readJsonFile,
toDate,
validateExternalCompatReport,
type ExternalCompatReport,
} from "./external-compat";
import { formatStatus } from "./render-compat-matrix";
// ... other code ...
for (const client of report.clients) {
const label = client.name === "claude-code" ? "Claude Code MCP integration" : "Cursor MCP integration";
const expectedStatus = formatStatus(client.status);
const rowMustContain = `${label} | stdio | 2024-11-05 | ${client.version.detected} | ${expectedStatus}`;
if (!matrix.includes(rowMustContain)) {
errors.push(`MATRIX_MISMATCH:${client.name}: expected row fragment '${rowMustContain}'`);
}
}
🤖 Prompt for AI Agents
In `@scripts/check-external-compat-gate.ts` around lines 60 - 72, Replace the
duplicated status-to-display logic by importing and using the existing
formatStatus function from render-compat-matrix (or move it to shared
external-compat module) inside the loop that iterates report.clients; call
formatStatus(client.status) to produce expectedStatus instead of the ternary
chain, and likewise reuse the label derivation used by renderGeneratedBlock (or
extract a small helper for client label) so the rowMustContain string uses the
shared label and formatStatus outputs; update imports accordingly and remove the
duplicated conditional logic.

Comment on lines +219 to +228
app.post("/v1/memory/import", async (c) => {
try {
const body = (await c.req.json()) as { payload: string; mode?: "skip" | "merge" | "replace" };
const mode = body.mode === "replace" ? "overwrite" : "skip-duplicates";
const result = await memoryEngine.import(body.payload, { mode });
return c.json(ok(result));
} catch {
return c.json(fail("VALIDATION_ERROR", "Invalid import payload"), 400);
}
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Import "merge" mode is silently treated as "skip-duplicates".

The API type declares three modes ("skip" | "merge" | "replace"), but only "replace" is explicitly mapped. Both "skip" and "merge" fall through to "skip-duplicates", which means a caller requesting "merge" gets silent skip behavior instead. Either implement merge semantics, reject unsupported modes with a 400, or document the limitation.

🤖 Prompt for AI Agents
In `@src/adapters/http/server.ts` around lines 219 - 228, The import endpoint
currently maps only "replace" and treats both "skip" and "merge" as
"skip-duplicates", silently dropping merge semantics; update the handler for
app.post("/v1/memory/import") to explicitly handle body.mode values: map
"replace" -> "overwrite", "skip" -> "skip-duplicates", and "merge" -> the
correct merge behavior (e.g., pass mode: "merge" to memoryEngine.import or
perform merging logic inside the handler), and if an unknown mode is provided
return c.json(fail("VALIDATION_ERROR", "Unsupported import mode"), 400); use the
body.mode and memoryEngine.import identifiers to locate and change the mapping
logic.

Comment on lines +185 to +188
}

this.initialized = true;
this.send({
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

In strict mode, initialized is set prematurely during initialize.

Per the MCP protocol, the full handshake is: client sends initialize → server responds → client sends notifications/initialized → server is ready. Setting this.initialized = true at line 187 (inside handleInitialize) means the server accepts tools/list and tools/call requests before the client sends notifications/initialized, undermining the "strict" compatibility mode guarantee.

If strict mode is meant to enforce the full MCP lifecycle, this.initialized should only be set by the notifications/initialized handler at line 124.

🐛 Proposed fix
 	private handleInitialize(msg: JsonRpcRequest): void {
 		const requested =
 			typeof msg.params?.protocolVersion === "string"
 				? msg.params.protocolVersion
 				: this.protocolVersion;

 		if (!this.supportedProtocolVersions.includes(requested)) {
 			this.send({
 				jsonrpc: "2.0",
 				id: msg.id ?? null,
 				error: {
 					code: -32602,
 					message: `Unsupported protocol version: ${requested}`,
 					data: { supported: this.supportedProtocolVersions },
 				},
 			});
 			return;
 		}

-		this.initialized = true;
+		if (this.compatibilityMode === "legacy") {
+			this.initialized = true;
+		}
 		this.send({
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
}
this.initialized = true;
this.send({
}
if (this.compatibilityMode === "legacy") {
this.initialized = true;
}
this.send({
🤖 Prompt for AI Agents
In `@src/adapters/mcp/server.ts` around lines 185 - 188, handleInitialize
currently sets this.initialized inside the initialize flow, which allows
accepting tools/list and tools/call before the client sends
notifications/initialized; remove the premature this.initialized = true from
handleInitialize (and keep sending the initialize response via this.send as-is)
and ensure only the notifications/initialized handler sets this.initialized (the
existing handler that processes "notifications/initialized" should flip the flag
and mark the server ready), so strict mode enforces the full MCP lifecycle.

Comment on lines +414 to +417
try {
const payload = await text();
const isError = payload.includes('"error": {') && !payload.includes('"error": null');
return { content: [{ type: "text", text: payload }], isError };
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

isError detection via string matching on serialized JSON is fragile and incorrect.

payload.includes('"error": {') && !payload.includes('"error": null') depends on JSON.stringify(…, null, 2) formatting and will break if indentation changes. Worse, a tool response whose data happens to contain the substring "error": { (e.g., an observation narrative) would be falsely flagged as an error.

Restructure text() to return a discriminated result so isError is determined from structured data rather than string inspection.

🐛 Proposed fix
-	private async executeTool(name: string, args: Record<string, unknown>): Promise<McpToolResult> {
-		const text = async () => {
+	private async executeTool(name: string, args: Record<string, unknown>): Promise<McpToolResult> {
+		const execute = async (): Promise<{ payload: string; isError: boolean }> => {
 			switch (name) {
 				case "memory.find": {
 					const parsed = toolSchemas.find.parse(args);
 					const results = await this.memoryEngine.search(parsed.query, {
 						limit: parsed.limit,
 						type: parsed.types?.[0],
 					});
-					return JSON.stringify(ok({ results }), null, 2);
+					return { payload: JSON.stringify(ok({ results }), null, 2), isError: false };
 				}
 				// ... similar changes for other cases, returning { payload, isError: false } for success
 				// and { payload: JSON.stringify(fail(...), ...), isError: true } for failure paths
 				default:
-					return JSON.stringify(fail("NOT_FOUND", `Unknown tool: ${name}`), null, 2);
+					return { payload: JSON.stringify(fail("NOT_FOUND", `Unknown tool: ${name}`), null, 2), isError: true };
 			}
 		};
 
 		try {
-			const payload = await text();
-			const isError = payload.includes('"error": {') && !payload.includes('"error": null');
-			return { content: [{ type: "text", text: payload }], isError };
+			const { payload, isError } = await execute();
+			return { content: [{ type: "text", text: payload }], isError };
 		} catch (error) {
🤖 Prompt for AI Agents
In `@src/adapters/mcp/server.ts` around lines 414 - 417, The current isError
detection uses brittle string matching on the serialized payload; instead modify
the text() helper to return a discriminated/structured result (e.g., { kind:
"text", raw: string, parsed?: unknown, isError?: boolean } or similar) so the
caller can inspect parsed JSON rather than searching substrings; update the try
block that calls text() to use the returned discriminant/parsed value to compute
isError (e.g., check parsed.error !== undefined && parsed.error !== null) and
return the structured content accordingly, referencing the text() function and
the isError variable so the downstream logic no longer relies on
payload.includes(...) string matching.

Comment on lines +107 to +124
export async function getBridgeHealth(
baseUrl: string,
fetchImpl: typeof fetch = fetch,
): Promise<BridgeResponse> {
const url = `${baseUrl.replace(/\/$/, "")}/v1/health`;
const resp = await fetchImpl(url, { method: "GET" });
const text = await resp.text();
const parsed = parseBridgeResponse(text);
if (!resp.ok && parsed.ok) {
return {
...parsed,
ok: false,
code: "HTTP_ERROR",
message: `HTTP status ${resp.status}`,
};
}
return parsed;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

getBridgeHealth lacks error handling around parseBridgeResponse, unlike sendBridgeHttpEvent.

sendBridgeHttpEvent (lines 89-93) wraps parseBridgeResponse in a try-catch to handle non-JSON responses, but getBridgeHealth calls it unprotected on line 114. Health endpoints are especially likely to return non-JSON when the service is down (e.g., a reverse proxy returning an HTML 502 page), which will cause an unhandled exception to propagate.

Proposed fix
 export async function getBridgeHealth(
 	baseUrl: string,
 	fetchImpl: typeof fetch = fetch,
 ): Promise<BridgeResponse> {
 	const url = `${baseUrl.replace(/\/$/, "")}/v1/health`;
 	const resp = await fetchImpl(url, { method: "GET" });
 	const text = await resp.text();
-	const parsed = parseBridgeResponse(text);
+	let parsed: BridgeResponse;
+	try {
+		parsed = parseBridgeResponse(text);
+	} catch {
+		throw new Error(`Bridge health response was not parseable JSON (status ${resp.status})`);
+	}
 	if (!resp.ok && parsed.ok) {
 		return {
 			...parsed,
 			ok: false,
 			code: "HTTP_ERROR",
 			message: `HTTP status ${resp.status}`,
 		};
 	}
 	return parsed;
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export async function getBridgeHealth(
baseUrl: string,
fetchImpl: typeof fetch = fetch,
): Promise<BridgeResponse> {
const url = `${baseUrl.replace(/\/$/, "")}/v1/health`;
const resp = await fetchImpl(url, { method: "GET" });
const text = await resp.text();
const parsed = parseBridgeResponse(text);
if (!resp.ok && parsed.ok) {
return {
...parsed,
ok: false,
code: "HTTP_ERROR",
message: `HTTP status ${resp.status}`,
};
}
return parsed;
}
export async function getBridgeHealth(
baseUrl: string,
fetchImpl: typeof fetch = fetch,
): Promise<BridgeResponse> {
const url = `${baseUrl.replace(/\/$/, "")}/v1/health`;
const resp = await fetchImpl(url, { method: "GET" });
const text = await resp.text();
let parsed: BridgeResponse;
try {
parsed = parseBridgeResponse(text);
} catch {
throw new Error(`Bridge health response was not parseable JSON (status ${resp.status})`);
}
if (!resp.ok && parsed.ok) {
return {
...parsed,
ok: false,
code: "HTTP_ERROR",
message: `HTTP status ${resp.status}`,
};
}
return parsed;
}
🤖 Prompt for AI Agents
In `@src/adapters/platform/bridge-client.ts` around lines 107 - 124,
getBridgeHealth currently calls parseBridgeResponse without protection and can
throw on non-JSON responses; wrap the parseBridgeResponse(text) call in a
try-catch (similar to sendBridgeHttpEvent) and on parse failure return a
BridgeResponse with ok: false, code indicating invalid/non-JSON response (e.g.,
"INVALID_JSON" or "HTTP_ERROR"), include resp.status and the raw text (or a
truncated message) in message, and still preserve final HTTP-error handling
logic so getBridgeHealth returns a well-formed BridgeResponse instead of
propagating an exception.

Comment thread src/platform-worker.ts
Comment on lines +273 to +279
try {
const response = await handleEnvelope(parseEnvelope(body));
if ((parseEnvelope(body).command ?? "event") === "shutdown") {
setTimeout(() => {
void shutdown();
}, 0);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

parseEnvelope(body) is called twice — store it once.

On line 274 the body is parsed into an envelope for handleEnvelope, then on line 275 it's re-parsed just to check the command. This is wasteful and fragile if the function ever gains side effects. Store the result in a local variable.

♻️ Proposed fix
 				try {
-					const response = await handleEnvelope(parseEnvelope(body));
-					if ((parseEnvelope(body).command ?? "event") === "shutdown") {
+					const envelope = parseEnvelope(body);
+					const response = await handleEnvelope(envelope);
+					if ((envelope.command ?? "event") === "shutdown") {
 						setTimeout(() => {
 							void shutdown();
 						}, 0);
 					}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
try {
const response = await handleEnvelope(parseEnvelope(body));
if ((parseEnvelope(body).command ?? "event") === "shutdown") {
setTimeout(() => {
void shutdown();
}, 0);
}
try {
const envelope = parseEnvelope(body);
const response = await handleEnvelope(envelope);
if ((envelope.command ?? "event") === "shutdown") {
setTimeout(() => {
void shutdown();
}, 0);
}
🤖 Prompt for AI Agents
In `@src/platform-worker.ts` around lines 273 - 279, The code currently calls
parseEnvelope(body) twice; capture its result once in a local variable (e.g.,
const envelope = parseEnvelope(body)) and pass that envelope to
handleEnvelope(envelope) and use envelope.command (with the same nullish/default
logic) for the "shutdown" check; update the block where handleEnvelope and the
shutdown setTimeout are invoked so they use the single envelope variable to
avoid double-parsing and potential side effects.

Comment on lines +23 to +24
const proc = Bun.spawn(["bun", "run", `src/${entry}.ts`, "--project", project], {
cwd: "/Users/clopca/dev/github/open-mem",
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Hardcoded absolute path breaks CI and other environments — all 3 tests fail.

Line 24 uses /Users/clopca/dev/github/open-mem as cwd, which is a local developer path. This is the direct cause of all three ENOENT failures in the CI pipeline. The path does not exist on the GitHub Actions runner (or any other machine).

Use process.cwd() or resolve the repository root dynamically:

🐛 Proposed fix
 const proc = Bun.spawn(["bun", "run", `src/${entry}.ts`, "--project", project], {
-	cwd: "/Users/clopca/dev/github/open-mem",
+	cwd: process.cwd(),
 	stdin: "pipe",
 	stdout: "pipe",
 	stderr: "pipe",
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const proc = Bun.spawn(["bun", "run", `src/${entry}.ts`, "--project", project], {
cwd: "/Users/clopca/dev/github/open-mem",
const proc = Bun.spawn(["bun", "run", `src/${entry}.ts`, "--project", project], {
cwd: process.cwd(),
stdin: "pipe",
stdout: "pipe",
stderr: "pipe",
🧰 Tools
🪛 GitHub Actions: ci

[error] 23-23: ENOENT: no such file or directory, posix_spawn 'bun'. The code attempted to spawn the command: bun run src/${entry}.ts --project ${project}.

🪛 GitHub Check: test

[failure] 23-23: error: ENOENT: no such file or directory

  at runWorker (/home/runner/work/open-mem/open-mem/tests/integration/platform-worker.test.ts:23:19)
  at <anonymous> (/home/runner/work/open-mem/open-mem/tests/integration/platform-worker.test.ts:129:27)

[failure] 23-23: error: ENOENT: no such file or directory

  at runWorker (/home/runner/work/open-mem/open-mem/tests/integration/platform-worker.test.ts:23:19)
  at <anonymous> (/home/runner/work/open-mem/open-mem/tests/integration/platform-worker.test.ts:100:27)

[failure] 23-23: error: ENOENT: no such file or directory

  at runWorker (/home/runner/work/open-mem/open-mem/tests/integration/platform-worker.test.ts:23:19)
  at <anonymous> (/home/runner/work/open-mem/open-mem/tests/integration/platform-worker.test.ts:71:27)
🤖 Prompt for AI Agents
In `@tests/integration/platform-worker.test.ts` around lines 23 - 24, The test
hardcodes a local absolute cwd ("/Users/clopca/...") when calling Bun.spawn
which breaks CI; change the Bun.spawn invocation that creates proc to use a
dynamic working directory (e.g. process.cwd() or path.resolve(__dirname,
"../../") / repo root) instead of the hardcoded string so the tests run in the
repository/workspace where the runner executes; update the proc creation
(Bun.spawn([...], { cwd: ... })) to reference the dynamic value and ensure
related variables entry and project still supply the correct args.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant