← Back to PRs

#18756: fix the memory manager class hierarchy declared at the wrong level

by leoh open 2026-02-17 01:37 View on GitHub →
stale size: XS
## Summary - **Problem:** The memory manager class hierarchy (`MemoryManagerSyncOps` → `MemoryManagerEmbeddingOps` → `MemoryIndexManager`) had properties and methods declared at the wrong inheritance level — batch-failure fields lived in the leaf class but were accessed in the middle class, two methods were `private` instead of `protected`, and `MemoryManagerEmbeddingOps` was concrete despite never being instantiated directly. - **Why it matters:** `pnpm build` fails with ~15 TypeScript errors, which blocks the Docker image build and therefore `docker-setup.sh` entirely. - **What changed:** Hoisted four batch-failure properties (`batchFailureCount`, `batchFailureLastError`, `batchFailureLastProvider`, `batchFailureLock`) to `MemoryManagerSyncOps`; widened `ensureSessionListener`/`runSync` from `private` to `protected`; marked `MemoryManagerEmbeddingOps` as `abstract`; fixed `buildEmbeddingBatchRunnerOptions` return-type signature to match its callees (`agentId: string`, `data?` optional). - **What did NOT change (scope boundary):** No runtime logic, no new features, no test changes — strictly visibility/declaration fixes to restore a green build. ## Change Type (select all) - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Docs - [ ] Security hardening - [ ] Chore/infra ## Scope (select all touched areas) - [ ] Gateway / orchestration - [ ] Skills / tool execution - [ ] Auth / tokens - [x] Memory / storage - [ ] Integrations - [ ] API / contracts - [ ] UI / DX - [ ] CI/CD / infra ## Linked Issue/PR - Closes # - Related # ## User-visible / Behavior Changes None ## Security Impact (required) - New permissions/capabilities? (`Yes/No`) No - Secrets/tokens handling changed? (`Yes/No`) No - New/changed network calls? (`Yes/No`) No - Command/tool execution surface changed? (`Yes/No`) No - Data access scope changed? (`Yes/No`) No - If any `Yes`, explain risk + mitigation: ## Repro + Verification ### Environment - OS: macOS (Darwin 25.2.0) - Runtime/container: Docker Desktop, Node 22-bookworm, pnpm 10.23.0 - Model/provider: N/A - Integration/channel (if any): N/A - Relevant config (redacted): Default `docker-setup.sh` settings ### Steps 1. `git checkout main` (at commit `dcdbbd8b3`) 2. Run `bash docker-setup.sh` 3. Docker build fails at `RUN pnpm build` with ~15 TS errors in `manager-embedding-ops.ts` and `manager.ts` ### Expected - Docker image builds successfully; gateway starts. ### Actual - Build exits with `exit code 1` due to TypeScript compilation errors (`TS2551`, `TS2339`, `TS2341`, `TS2322`). ## Evidence - [x] Trace/log snippets **Before:** `pnpm build` produces errors: ``` src/memory/manager-embedding-ops.ts(582,23): error TS2551: Property 'batchFailureLock' does not exist on type 'MemoryManagerEmbeddingOps'. src/memory/manager-embedding-ops.ts(596,16): error TS2551: Property 'batchFailureCount' does not exist on type 'MemoryManagerEmbeddingOps'. src/memory/manager.ts(180,10): error TS2341: Property 'ensureSessionListener' is private and only accessible within class 'MemoryManagerSyncOps'. src/memory/manager.ts(384,25): error TS2341: Property 'runSync' is private and only accessible within class 'MemoryManagerSyncOps'. ``` **After:** `tsc --noEmit` passes with zero errors; `docker-setup.sh` completes and gateway container starts. ## Human Verification (required) - **Verified scenarios:** Full `docker-setup.sh` run end-to-end — image builds, onboarding completes, gateway container starts. - **Edge cases checked:** `tsc --noEmit` confirms zero errors across the entire codebase, not just the touched files. - **What you did not verify:** Runtime memory indexing / embedding batch flows; unit/integration test suite (no `node_modules` were present locally until midway through the fix). ## Compatibility / Migration - Backward compatible? (`Yes/No`) Yes - Config/env changes? (`Yes/No`) No - Migration needed? (`Yes/No`) No - If yes, exact upgrade steps: ## Failure Recovery (if this breaks) - **How to disable/revert this change quickly:** `git revert <commit>` — the three files are self-contained. - **Files/config to restore:** `src/memory/manager-sync-ops.ts`, `src/memory/manager-embedding-ops.ts`, `src/memory/manager.ts` - **Known bad symptoms reviewers should watch for:** Any new TS errors referencing these three files; runtime `TypeError` on `this.batchFailureCount` being `undefined` (would indicate the property was not inherited correctly). ## Risks and Mitigations - **Risk:** Widening `ensureSessionListener`/`runSync` from `private` to `protected` exposes them to any future subclass, not just `MemoryIndexManager`. - **Mitigation:** Both methods are internal implementation details with no public API surface; the class hierarchy is small and co-located in `src/memory/`.

Most Similar PRs