Quality-first architecture refactor + strict contracts + docs/CI hardening#10
Conversation
📝 WalkthroughWalkthroughCentralizes tool contracts and renames memory tools to mem-*, removes the external compatibility pipeline, adds readiness/diagnostics/queue HTTP endpoints, introduces search strategy registry and AI provider fallback policies, refactors mode resolution with inheritance, and adds CI/quality-gate tooling and governance docs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTP as HTTP Server
participant Readiness as DefaultReadinessService
participant Runtime as MemoryEngine
participant Config as Config
Client->>HTTP: GET /v1/readiness
HTTP->>Readiness: evaluate({config, adapterStatuses, runtime})
Readiness->>Config: validate compression/provider settings
Readiness->>Runtime: query runtime.status and runtime.queue
Readiness-->>HTTP: {ready, status, reasons}
HTTP-->>Client: 200/503 readiness envelope
sequenceDiagram
participant Search as SearchOrchestrator
participant Registry as StrategyRegistry
participant Executor as StrategyExecutor
participant Repo as ObservationRepository
Search->>Registry: get(strategyId)
Registry-->>Search: executor
Search->>Executor: execute(deps, query, options, limit)
Executor->>Repo: search / searchByConcept / searchByFile
Repo-->>Executor: observations[]
Executor-->>Search: SearchResult[]
Search-->>Client: ranked results
sequenceDiagram
participant CLI as Mode Loader
participant Resolver as ModeResolverV2
participant FS as FileSystem
participant Map as RawModesMap
CLI->>Resolver: loadAllRaw()
Resolver->>FS: read mode JSON files
FS-->>Resolver: file contents
Resolver-->>Map: Map<id, ModeConfigSource>
CLI->>Resolver: resolveById(id, Map)
alt has parent
Resolver->>Resolver: resolveById(parent, Map)
Resolver-->>CLI: merged ModeConfig
else cycle or missing
Resolver-->>CLI: DEFAULT_MODE
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
Greptile SummaryThis PR migrates to a quality-first architecture by refactoring search into a pluggable strategy registry, adding mode inheritance via
Confidence Score: 3/5
Important Files Changed
Flowchartflowchart TD
A[SearchOrchestrator.search] --> B[normalizeOptions]
B --> C{Strategy Registry Lookup}
C -->|filter-only| D[executeFilterOnlyStrategy]
C -->|semantic| E[executeSemanticStrategy]
C -->|hybrid| F[executeHybridStrategy]
D --> G[ObservationRepository.search / searchByConcept / searchByFile]
E -->|no embedding model| D
E -->|has embedding model| H[generateEmbedding + vector search]
H -->|native vec extension| I[getVecEmbeddingMatches]
H -->|JS fallback| J[getWithEmbeddings + cosineSimilarity]
F --> K[hybridSearch: FTS5 + RRF merge]
K --> L[safelyRunFts + runVectorSearch]
G --> M[Label as project source]
I --> M
J --> M
L --> M
M --> N{entityRepo available?}
N -->|yes| O[graphAugmentedSearch]
N -->|no| P{userObservationRepo?}
O --> P
P -->|yes| Q[searchUserMemory + mergeResults]
P -->|no| R{reranker?}
Q --> R
R -->|yes| S[reranker.rerank]
R -->|no| T[Return results]
S --> T
Last reviewed commit: 5df3b50 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
package.json (1)
90-90:⚠️ Potential issue | 🟠 MajorRemove self-dependency from package.json.
Line 90 lists
"open-mem": "^0.9.0"as a dependency, but the package is named"open-mem"(line 2). This was already documented as removed in CHANGELOG.md but wasn't removed from package.json. The codebase does not import "open-mem" as a package;src/index.tsonly uses path references to locate the plugin's own dist folder. Remove this entry from dependencies.docs/architecture.md (1)
138-142:⚠️ Potential issue | 🟡 MinorStale
memory.*references remain in the External Surfaces section.Lines 138 and 142 still reference
memory.*tools/namespace while the rest of the document (and broader docs) now usesmem-*. These should be updated for consistency.-- **Tools**: All 9 `memory.*` tools +- **Tools**: All 9 `mem-*` tools -- Same 9 tools over stdin/stdout JSON-RPC (`memory.*` namespace) +- Same 9 tools over stdin/stdout JSON-RPC (`mem-*` namespace)docs/api.md (1)
268-318:⚠️ Potential issue | 🟡 MinorHealth Check response example is now visually orphaned from its endpoint.
The new endpoint sections (Readiness, Diagnostics, Tools Guide, Queue Status, Trigger Queue Processing) were inserted between the Health Check description (line 266) and its example
Response:block (lines 308–318). As a result, the JSON response showing"status": "healthy"withdatabase/queue/providerfields now appears to belong to the "Trigger Queue Processing" endpoint.Move the Health Check response example to immediately after line 266 (before the new Readiness section), or add dedicated response examples for the new endpoints as well.
🤖 Fix all issues with AI agents
In @.github/workflows/nightly-matrix.yml:
- Around line 21-30: The "Run matrix checks" job is running identical structural
checks across a 6-job matrix and exporting unused env vars (OPEN_MEM_PROVIDER,
OPEN_MEM_ENTITY_EXTRACTION); collapse the matrix to a single job (remove
matrix.provider and matrix.vector_mode usage) and drop those unused environment
variables, then remove or adjust the redundant "bun run quality:gate" invocation
(which re-runs check:contracts and check:architecture) so the job only runs the
unique steps: bun run typecheck, bun run check:boundaries, bun run
check:contracts, bun run check:architecture (or have quality:gate run in a
separate job that excludes duplicate checks).
In `@docs/tools.md`:
- Around line 26-28: The list repeats the verb "Use" for mem-find, mem-history,
and mem-get which is stylistically repetitive; update one or two items to vary
sentence starts while keeping meaning and the command names intact (e.g., change
the first to "Run mem-find to locate candidate observations," the second to
"Inspect mem-history to understand timeline context," and keep the third as "Use
mem-get to pull complete details when needed"), ensuring each line still
references mem-find, mem-history, and mem-get exactly.
In `@scripts/check-architecture-fitness.ts`:
- Around line 19-22: The code calls walk(join(process.cwd(), "src/core")) /
"src/search" / "src/modes" unguarded so readdirSync inside walk will throw
ENOENT if a directory is missing; update the code that builds coreFiles,
searchFiles, and modeFiles to first check directory existence (e.g.,
fs.existsSync or fs.statSync(...).isDirectory()) or wrap walk(...) in a
try/catch and return an empty array when the dir is absent, then combine into
criticalFiles as before (references: walk, coreFiles, searchFiles, modeFiles,
criticalFiles).
In `@src/core/contracts.ts`:
- Around line 139-157: The file defines duplicate interfaces ReadinessService
and SetupDiagnosticsService that conflict with the service implementations;
resolve by consolidating to a single canonical contract that matches the
implementations used by DefaultReadinessService and
DefaultSetupDiagnosticsService: update or remove the ReadinessService in
src/core/contracts.ts so its signature matches the implementation (use
evaluate(input: ReadinessInput): ReadinessResult) or import and re-export the
service-side types (ReadinessInput, ReadinessResult) instead, and likewise
update SetupDiagnosticsService to run(config: OpenMemConfig):
SetupDiagnosticsResult or re-export the types from
src/services/setup-diagnostics.ts; ensure all imports across the codebase
reference the unified interface/name to avoid duplicate symbols.
In `@src/modes/resolver.ts`:
- Around line 114-116: getDefaultModeConfig currently returns the shared
DEFAULT_MODE reference (and resolveById returns DEFAULT_MODE on cycle/miss),
which allows callers to mutate the singleton; change getDefaultModeConfig to
return a copy (e.g., return { ...DEFAULT_MODE, observationTypes:
[...DEFAULT_MODE.observationTypes], promptTemplates:
[...(DEFAULT_MODE.promptTemplates || [])], /* clone any array properties */ } or
simply use structuredClone(DEFAULT_MODE)) and update resolveById to return the
same cloned copy instead of DEFAULT_MODE; alternatively, freeze DEFAULT_MODE
with Object.freeze(DEFAULT_MODE) if immutability is preferred.
In `@src/search/orchestrator.ts`:
- Around line 110-112: The code mutates objects returned by executor (results)
which may be shared/cached (notably from executeFilterOnlyStrategy in
filter-only.ts that returns deps.observations.search(...)); avoid in-place
mutation by returning new objects with a source field instead—either update each
strategy (e.g., executeFilterOnlyStrategy) to construct results that already
include source="project" or, at this call site where executor(options, { query,
limit }) is used, map the results to new objects (e.g., map each result to a
shallow copy with source set) before further use so you never modify the
original returned objects.
In `@src/search/strategies/hybrid.ts`:
- Around line 4-22: executeHybridStrategy currently drops singular options
(options.file, options.concept) because it only forwards plural fields to
hybridSearch; update this function to normalize singular→plural before calling
hybridSearch (if options.file is set, set files = [options.file] merged with
options.files; likewise for options.concept -> concepts = [options.concept]
merged with options.concepts) and then pass the normalized files/concepts to
hybridSearch (reference executeHybridStrategy, StrategyOptions, hybridSearch,
HybridSearchOptions); alternatively, if you prefer behavior consistent with
executeFilterOnlyStrategy, you can instead detect singular options and return
early with the same handling, but do not silently drop singular fields.
In `@src/search/strategies/semantic.ts`:
- Around line 46-48: The catch block in the vector-extension path currently
swallows errors and returns an empty array, causing no results when the vector
subsystem fails; change this to call and return the existing fallback function
executeFilterOnlyStrategy (the same fallback used when embedding generation
fails) so the function returns filter-only results instead of []. Locate the
try/catch around the vector extension call in semantic.ts and replace the silent
return [] in the catch with a call to executeFilterOnlyStrategy with the same
arguments/context so behavior is consistent when vectors are unavailable.
In `@src/services/readiness.ts`:
- Around line 20-22: There are two conflicting ReadinessService interface
definitions; remove the unused one in core/contracts.ts (the variant declaring
getStatus(): ReadinessStatus) and consolidate on the service interface used by
DefaultReadinessService: keep the ReadinessService that declares evaluate(input:
ReadinessInput): ReadinessResult (from services/readiness.ts), update any
imports that referenced the removed interface to import the services version,
and ensure DefaultReadinessService and any callers reference the single exported
ReadinessService type and ReadinessInput/ReadinessResult symbols.
In `@src/services/setup-diagnostics.ts`:
- Around line 17-19: There are two conflicting SetupDiagnosticsService
interfaces (one requires run(config: OpenMemConfig): SetupDiagnosticsResult
while the other declares run(): SetupDiagnosticsReport), so reconcile them by
making the contract match the actual implementation: change the contract's
SetupDiagnosticsService signature to run(config: OpenMemConfig):
SetupDiagnosticsResult and ensure the exported types reference
SetupDiagnosticsResult and OpenMemConfig (or alternatively change
DefaultSetupDiagnosticsService to implement the no-arg run returning
SetupDiagnosticsReport if you prefer that API); remove the duplicate interface
declaration so only one SetupDiagnosticsService exists, update any affected
imports/exports, and ensure DefaultSetupDiagnosticsService implements the
unified interface name exactly.
🧹 Nitpick comments (34)
src/ai/fallback-policy.ts (2)
7-13: Nit:unknown | nullis redundant.
unknownalready includesnullin TypeScript, sounknown | nullsimplifies tounknown. Consider using justunknownfor clarity, or if the intent is to signal thatnullmeans "no error" (as used inonAttempt), a more explicit union likeError | nullor a dedicated field would convey that better.
30-36: Unsafe cast onerrorto extractstatus.Line 31 casts
input.errortoRecord<string, unknown>without a guard. If the error is a primitive (e.g., a thrown string) ornull, the optional chaining prevents a crash, but the cast is misleading and would silently produce"unknown"for well-structured errors that store status differently.Consider a small type guard or duck-type check:
Suggested approach
onFailover(input: FallbackDecisionInput): void { - const status = (input.error as Record<string, unknown>)?.status ?? "unknown"; + const err = input.error; + const status = + (err != null && typeof err === "object" && "status" in err ? (err as Record<string, unknown>).status : undefined) ?? "unknown"; if (!input.nextProvider) return;src/ai/fallback.ts (2)
76-81: HardcodedisConfigErrorcheck constrains custom policy behavior.Both
doGenerate(line 79) anddoStream(line 125) short-circuit withthrowon config errors before consulting the policy. This means a customProviderFallbackPolicycan never override the config-error behavior —shouldFailoveris never called for these errors.If this is intentional defense-in-depth (config errors should always halt), that's reasonable, but worth a brief inline comment explaining why the policy is bypassed here. Otherwise, if the goal is full policy pluggability, this check should be removed and left to the policy.
Also applies to: 122-127
62-102:doGenerateanddoStreamare near-identical — consider extracting the retry loop.The two methods differ only in the inner call (
doGeneratevsdoStreamon the provider model). The surrounding loop, policy hook calls, error handling, and decision construction are duplicated line-for-line. A shared private helper would eliminate ~40 lines of duplication.Sketch
+private async executeWithFallback( + options: unknown, + method: "doGenerate" | "doStream", +): Promise<unknown> { + let lastError: unknown; + for (let i = 0; i < this.providers.length; i++) { + const provider = this.providers[i]; + try { + this.policy.onAttempt?.({ + error: null, + provider: provider.name, + nextProvider: this.providers[i + 1]?.name, + attemptIndex: i, + totalProviders: this.providers.length, + }); + return await (provider.model[method] as (opts: unknown) => Promise<unknown>)(options); + } catch (error: unknown) { + lastError = error; + if (isConfigError(error)) throw error; + const decision = { + error, + provider: provider.name, + nextProvider: this.providers[i + 1]?.name, + attemptIndex: i, + totalProviders: this.providers.length, + }; + if (this.policy.shouldFailover(decision)) { + this.policy.onFailover(decision); + continue; + } + this.policy.onFinalFailure?.(decision); + throw error; + } + } + throw lastError; +} + async doGenerate(options: unknown): Promise<unknown> { - // ... duplicated loop ... + return this.executeWithFallback(options, "doGenerate"); } + async doStream(options: unknown): Promise<unknown> { - // ... duplicated loop ... + return this.executeWithFallback(options, "doStream"); }Also applies to: 108-148
docs/engineering/deprecation-policy.md (1)
1-14: Governance doc is a good addition but quite sparse.Consider adding concrete examples for each lifecycle stage (e.g., what a deprecation warning looks like in code or logs) and clarifying what "explicit" means in rule 3 — does it require a changelog entry, a migration guide, or both? Linking to the ADR template for structural deprecation decisions would also tie this into the broader governance framework.
.github/CODEOWNERS (1)
1-15: Single-owner CODEOWNERS creates a bus-factor-of-1 bottleneck.Every PR touching these paths will be blocked if
@clopcais unavailable. Consider adding a team (e.g.,@org/core-team) as a secondary owner, or at minimum a catch-all rule (*@clopca``) so files outside the listed paths also get review coverage..github/workflows/release-gate.yml (2)
20-25: Early failure in multi-command step hides downstream check results.The
|block runs withset -eby default, so ifcheck:boundariesfails,check:contracts,check:architecture, andcheck:migrationare skipped. If you want all checks to report independently, either split them into separate steps or useset +ewith an explicit exit code accumulator.Split into separate steps
- name: Architecture and Contract Checks run: | bun run check:boundaries bun run check:contracts bun run check:architecture bun run check:migration + # Alternative: split into individual steps so all checks run independently + # - name: Boundary Check + # run: bun run check:boundaries + # - name: Contract Check + # run: bun run check:contracts + # - name: Architecture Check + # run: bun run check:architecture + # - name: Migration Check + # run: bun run check:migration
13-13: Consider pinning the Bun version and enabling dependency caching.
oven-sh/setup-bun@v2without abun-versioninput will install the latest, which may introduce non-determinism in your release gate. Also, this action supports caching viacache: trueor a separateactions/cachestep, which would speed up the gate.Pin version and enable cache
- uses: oven-sh/setup-bun@v2 + with: + bun-version: "1.2.x" + cache: truescripts/check-migration-compat.ts (2)
12-17: Exits on first missing file — consider reporting all missing files before exiting.Currently the loop exits on the first missing file, so if multiple files are missing, only the first is reported. Accumulate all errors and exit once at the end.
Report all missing files
+let missing = false; for (const file of required) { if (!existsSync(file)) { console.error(`[migration-compat] required file missing: ${file}`); - process.exit(1); + missing = true; } } +if (missing) { + process.exit(1); +}
6-19: Script name overpromises — this only checks file existence, not migration compatibility.A "migration compatibility check" typically validates schema changes are backward-compatible, checks for destructive operations, or verifies migration ordering. This script only verifies that three files exist. Consider either renaming it to
check-migration-files-exist.tsor adding meaningful validation (e.g., ensuring the schema file exports expected symbols, checking for breaking DDL changes).scripts/check-contract-drift.ts (1)
27-32: Consider reporting all description failures before exiting.Currently the script exits on the first tool with a short/missing description, hiding any subsequent failures. Collecting all errors and reporting them together would speed up fixing.
♻️ Suggested change
+let descErrors = 0; for (const tool of TOOL_CONTRACTS) { if (!tool.description || tool.description.trim().length < 10) { console.error(`[contract-drift] Missing or too-short description for ${tool.name}`); - process.exit(1); + descErrors++; } } +if (descErrors > 0) process.exit(1);scripts/check-architecture-fitness.ts (1)
34-37: Adapter-import detection may produce false positives on comments or string literals.
content.includes("/adapters/")matches anywhere in the file — comments, string literals, or documentation — not just import statements. Since this is a CI gate that blocks merges, consider narrowing to actual import/require statements.♻️ Suggested: match only import/require lines
- if (content.includes("../adapters/") || content.includes("/adapters/")) { - console.error(`[architecture-fitness] adapter import leaked into core/search/modes: ${file}`); - process.exit(1); - } + const lines = content.split("\n"); + for (const line of lines) { + if (/^\s*(import|export)\s/.test(line) || /require\s*\(/.test(line)) { + if (line.includes("/adapters/")) { + console.error(`[architecture-fitness] adapter import leaked into core/search/modes: ${file}`); + process.exit(1); + } + } + }.github/workflows/architecture-health.yml (1)
8-22: Consider adding explicitpermissionsfor least-privilege.The workflow only needs to read the repo and upload an artifact. Without an explicit
permissionsblock, it inherits the repository's default token permissions, which may be broader than necessary.♻️ Suggested addition
jobs: report: runs-on: ubuntu-latest + permissions: + contents: read steps:src/adapters/opencode/tools.ts (1)
27-29: Unsafe lookup:descriptions[key]silently returnsundefinedfor missing contracts.
Object.fromEntriesproducesRecord<string, string>, sodescriptions["mem-find"]type-checks even if the key is absent at runtime. If a contract entry is ever renamed or removed fromTOOL_CONTRACTS, the corresponding tool silently getsdescription: undefined.Consider using the existing
getToolContractByNamehelper (fromschemas.ts) with a runtime assertion, or at minimum add a guard:♻️ Suggested safer lookup
- const descriptions = Object.fromEntries( - TOOL_CONTRACTS.map((tool) => [tool.name, tool.description]), - ); + const desc = (name: string): string => { + const contract = TOOL_CONTRACTS.find((t) => t.name === name); + if (!contract) throw new Error(`Missing tool contract: ${name}`); + return contract.description; + };Then replace
descriptions["mem-find"]withdesc("mem-find"), etc.docs/architecture/adr/ADR-0001-architecture-principles.md (1)
1-24: ADR is missing the "Alternatives Considered" section from the template.The ADR template (
ADR-TEMPLATE.md) includes an "Alternatives Considered" section. Adding it here (even if brief) would improve consistency and document why other approaches were rejected.src/services/readiness.ts (1)
48-56:"initializing"status is misleading for configuration errors.When adapters are missing or an API key isn't configured, the status is set to
"initializing", which implies a transient startup state. These are persistent configuration problems that won't self-resolve. Consider adding a"misconfigured"status or mapping config-related failures differently from runtime issues.src/contracts/schemas.ts (1)
96-156: Consider deriving tool names as a const union for type safety.The
namefield inToolContractMetadatais typed asstring, which means typos in contract names (or in consumer lookups likedescriptions["mem-find"]) won't be caught at compile time. A narrow type would improve safety:♻️ Optional: derive tool name type
export const TOOL_NAMES = [ "mem-find", "mem-history", "mem-get", "mem-create", "mem-revise", "mem-remove", "mem-export", "mem-import", "mem-maintenance", "mem-help", ] as const; export type ToolName = (typeof TOOL_NAMES)[number]; export interface ToolContractMetadata { name: ToolName; // ...rest }package.json (1)
30-38: Build scripts have heavy duplication of--externalflags.Every
build:*script repeats the same 8--externalflags verbatim. This is fragile — adding a new external dependency requires updating 7+ lines.Consider extracting these into a shared build script or a variable. Not blocking, but worth a follow-up.
src/adapters/mcp/server.ts (1)
201-218:schemaMapduplicatestoolSchemas— consider using it directly.The
schemaMapobject manually mirrors every key fromtoolSchemas. You could indextoolSchemasdirectly with a type assertion ontool.schema, eliminating the duplicated mapping and reducing the risk of the two falling out of sync when a new tool is added.♻️ Proposed simplification
private getToolDefinitions(): McpToolDefinition[] { - const schemaMap = { - find: toolSchemas.find, - history: toolSchemas.history, - get: toolSchemas.get, - create: toolSchemas.create, - revise: toolSchemas.revise, - remove: toolSchemas.remove, - transferExport: toolSchemas.transferExport, - transferImport: toolSchemas.transferImport, - maintenance: toolSchemas.maintenance, - help: toolSchemas.help, - } as const; - - return TOOL_CONTRACTS.map((tool) => ({ - name: tool.name, - description: tool.description, - inputSchema: toInputSchema(schemaMap[tool.schema]), - })); + return TOOL_CONTRACTS.map((tool) => ({ + name: tool.name, + description: tool.description, + inputSchema: toInputSchema(toolSchemas[tool.schema as keyof typeof toolSchemas]), + })); }If
ToolContractMetadata.schemawere typed askeyof typeof toolSchemasinstead ofstring, the assertion could be dropped entirely and the compiler would catch any mismatch at the contract definition site.src/services/setup-diagnostics.ts (1)
97-117: Asymmetric default-on logic foropencodeadapter.
platformOpenCodeEnableduses!== false(default-on), whileclaude-codeandcursoruse=== true(default-off). This is likely intentional since those config fields are optional booleans, but worth a brief inline comment for future readers..github/workflows/ci.yml (1)
22-29:check:contractsandcheck:architecturerun twice per CI run.Lines 23 and 25 execute these checks as standalone steps, and line 29 (
quality:gate) runs them again internally (seescripts/quality-gate.tslines 6-7). This doubles the execution time for those checks without added value.Either remove the standalone steps here and rely on
quality:gate, or strip the duplicates from the quality-gate script to keep the CI as the single orchestrator.docs/engineering/quality-gates.md (1)
5-11: Inconsistent command format between gate entries.Items 1–4 use the full
bun run …invocation, while items 5–7 switch to a descriptive label with the script name in parentheses. Keeping a uniform format (e.g., all asbun run check:contracts) makes it easier to copy-paste commands and reduces ambiguity.📝 Suggested consistency fix
-5. Contract checks (`check-contract-drift`) -6. Architecture fitness checks (`check-architecture-fitness`) -7. Migration compatibility checks (`check-migration-compat`) for storage/contract changes +5. `bun run check:contracts` +6. `bun run check:architecture` +7. `bun run check:migration` (for storage/contract changes)src/doctor.ts (1)
8-15:strict: falsesilently ignores unknown flags.Typos like
--projecTor--josnwill be silently swallowed, falling back to defaults without any user feedback. Consider usingstrict: trueand lettingparseArgssurface unrecognised flags early, or at minimum log a warning for unexpected arguments.docs/search.md (1)
100-103: Fenced code blocks missing language specifier (flagged by markdownlint).The code blocks at lines 100 and 109 lack a language tag. Adding a language identifier (e.g.,
textorplain) would satisfy the MD040 lint rule.Also applies to: 109-112
docs/openapi.v1.json (1)
27-31: New endpoint entries are consistent with docs and tests.The five new paths align with
docs/api.mddocumentation andtests/servers/http-server.test.tscoverage. The spec remains summary-only (no request/response schemas), which is a pre-existing pattern — consider fleshing out schemas as part of a future documentation pass to make the 1.0.0 contract more consumable by API clients and code generators.src/search/strategies/types.ts (1)
25-30: Note:StrategyExecutoris a distinct type fromSearchStrategyExecutorin the registry.This file's
StrategyExecutortakes(deps, query, options, limit)whileSearchStrategyExecutor<TOptions>insrc/search/registry.tstakes(options, context). These serve different purposes (direct execution vs. registry-based dispatch), but the similar naming could confuse contributors. A brief doc comment clarifying the distinction would help.src/modes/loader.ts (2)
17-19: RedundantModeResolverV2instantiation —loadModecreates a second resolver needlessly.
loadAllModes()at line 12 already creates aModeResolverV2(MODES_DIR). SinceloadModecallsloadAllModes()on line 19, a second resolver is created here for no additional benefit. Consider extracting a shared resolver instance or reusing the one fromloadAllModes.♻️ Proposed fix — share a single resolver
const MODES_DIR = join(import.meta.dir, "."); let modeCache: Map<string, ModeConfig> | null = null; +const resolver = new ModeResolverV2(MODES_DIR); function loadAllModes(): Map<string, ModeConfig> { if (modeCache) return modeCache; - - const resolver = new ModeResolverV2(MODES_DIR); modeCache = resolver.loadAllRaw(); return modeCache; } export function loadMode(modeId: string): ModeConfig { - const resolver = new ModeResolverV2(MODES_DIR); return resolver.resolveById(modeId, loadAllModes()); }
27-29:?? getDefaultModeConfig()fallback is unreachable.
resolveById(persrc/modes/resolver.tsline 92–110) always returns aModeConfig— it falls back toDEFAULT_MODEinternally on missing IDs or cycle detection, never returningnull/undefined. The??guard won't trigger. This is harmless as a defensive measure, but worth a comment if intentional.tests/modes/resolver-v2.test.ts (1)
60-93: Cycle-fallback assertion relies on a magic string"code"forDEFAULT_MODE.id.If
DEFAULT_MODEinresolver.tsis ever renamed, this test silently breaks. Consider importinggetDefaultModeConfigand asserting against its.idto keep the test resilient.Proposed improvement
import { ModeResolverV2 } from "../../src/modes/resolver"; +import { getDefaultModeConfig } from "../../src/modes/resolver"; ... const resolved = resolver.resolveById("a", raw); - expect(resolved.id).toBe("code"); + expect(resolved.id).toBe(getDefaultModeConfig().id);src/search/strategies/semantic.ts (1)
51-52: Embedding fallback fetches up tolimit * 10rows with full embeddings into memory.For large observation stores this could be expensive. The vector-extension path only fetches
limit * 3candidates. Consider documenting the tradeoff or capping the multiplier.src/adapters/http/server.ts (2)
289-307: Runtime fallback object is duplicated four times across endpoints.The same
RuntimeStatusSnapshotfallback literal appears in/v1/health(line 256),/v1/readiness(line 291),/v1/metrics(line 382), and/v1/queue(line 346). Extract it into a helper to reduce drift risk.Proposed helper extraction
+function getRuntime( + runtimeStatusProvider: (() => RuntimeStatusSnapshot) | undefined, + health: { status: string; timestamp: string }, +): RuntimeStatusSnapshot { + return ( + runtimeStatusProvider?.() ?? { + status: health.status, + timestamp: health.timestamp, + 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, + } + ); +}Then replace each occurrence with:
const runtime = getRuntime(runtimeStatusProvider, memoryEngine.getHealth());
309-309:DefaultReadinessServiceis instantiated on every request.While stateless, constructing it per-request is unnecessary. Consider creating a single instance at
createDashboardAppscope. Same applies toDefaultSetupDiagnosticsServiceon line 325.src/search/orchestrator.ts (1)
244-249:computeLineageInfoonly resolves one level of revision depth.If revisions can chain (A → B → C), this will always report
depth: 1androotIdas the immediate parent rather than the true root. If deeper lineage tracking is planned, this will need recursive traversal.docs/tools.md (1)
175-177: Clarifyactiontype with explicit union for consistency.
Other parameters use literal unions; consider listingactionas a union (e.g., `folderContextDryRun | folderContextClean | ...`) instead of “enum” to match the rest of the table.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (10)
src/modes/resolver.ts (3)
51-65: Optional fields (extends,locale,promptOverrides) are not validated.A JSON file with
"extends": 123or"promptOverrides": "oops"would passisValidModeand could cause subtle misbehaviour downstream (e.g.rawModes.get(123)silently misses,{ ...base.promptOverrides, ...override.promptOverrides }spreads a string's characters as keys).Consider adding lightweight optional-field checks:
Proposed tighter validation
return ( typeof v.id === "string" && typeof v.name === "string" && typeof v.description === "string" && isStringArray(v.observationTypes) && isStringArray(v.conceptVocabulary) && isStringArray(v.entityTypes) && - isStringArray(v.relationshipTypes) + isStringArray(v.relationshipTypes) && + (v.extends === undefined || typeof v.extends === "string") && + (v.locale === undefined || typeof v.locale === "string") && + (v.promptOverrides === undefined || + (typeof v.promptOverrides === "object" && + v.promptOverrides !== null && + !Array.isArray(v.promptOverrides))) );
67-80: Resolved modes retain the child'sextendsfield.After
mergeMode(parent, override), the result still carriesoverride.extends(e.g."code"). Downstream code or serialisation that inspectsextendscould mistakenly believe the mode still needs resolution. Consider stripping it:Strip `extends` after merge
function mergeMode(base: ModeConfig, override: ModeConfig): ModeConfig { - return { + const merged: ModeConfig = { ...base, ...override, observationTypes: override.observationTypes ?? base.observationTypes, conceptVocabulary: override.conceptVocabulary ?? base.conceptVocabulary, entityTypes: override.entityTypes ?? base.entityTypes, relationshipTypes: override.relationshipTypes ?? base.relationshipTypes, promptOverrides: { ...(base.promptOverrides ?? {}), ...(override.promptOverrides ?? {}), }, }; + delete merged.extends; + return merged; }
85-102: Duplicateidvalues across different JSON files are silently overwritten.If two files declare the same
id, the last one read wins, andreaddirSyncorder is filesystem-dependent. A warning log (or collecting duplicates and throwing) would make misconfiguration easier to diagnose.scripts/check-architecture-fitness.ts (1)
37-51: Early exit reports only the first violation — consider collecting all errors.The script calls
process.exit(1)on the first TODO/FIXME or adapter-leak hit, so a CI run surfaces only one problem at a time. Collecting all violations and reporting them together would save developers multiple fix-and-rerun cycles.♻️ Suggested approach
+const errors: string[] = []; for (const file of criticalFiles) { const content = readFileSync(file, "utf-8"); for (const regex of forbidden) { if (regex.test(content)) { - console.error(`[architecture-fitness] anonymous TODO/FIXME found in ${file}`); - process.exit(1); + errors.push(`[architecture-fitness] anonymous TODO/FIXME found in ${file}`); } } if (hasAdapterImportLeak(content)) { - console.error(`[architecture-fitness] adapter import leaked into core/search/modes: ${file}`); - process.exit(1); + errors.push(`[architecture-fitness] adapter import leaked into core/search/modes: ${file}`); } } + +if (errors.length > 0) { + for (const e of errors) console.error(e); + process.exit(1); +}src/adapters/http/server.ts (2)
307-340: DuplicatedRuntimeStatusSnapshotfallback object across multiple endpoints.The same fallback object for
runtimeStatusProvider?.() ?? { ... }is copy-pasted in/v1/health(line 274),/v1/readiness(line 309),/v1/queue(line 364), and/v1/metrics(line 400). Extract a helper to reduce duplication and risk of drift.♻️ Suggested helper
+function getRuntimeSnapshot( + health: ReturnType<MemoryEngine["getHealth"]>, + runtimeStatusProvider?: () => RuntimeStatusSnapshot, +): RuntimeStatusSnapshot { + return runtimeStatusProvider?.() ?? { + status: health.status, + timestamp: health.timestamp, + 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, + }; +}Then replace the duplicated blocks with:
const runtime = getRuntimeSnapshot(memoryEngine.getHealth(), runtimeStatusProvider);Also applies to: 361-395
327-328: Readiness and diagnostics services instantiated per request.
new DefaultReadinessService()andnew DefaultSetupDiagnosticsService()are created on every request. If these are stateless, consider creating them once increateDashboardAppand reusing them.Also applies to: 343-343
src/search/strategies/semantic.ts (1)
51-52: Empty-candidate paths return[]without fallback — verify this is intentional.When
getWithEmbeddingsreturns no candidates (line 52) orgetVecEmbeddingMatchesreturns none (line 25), the function returns[]rather than falling back to filter-only search. This differs from the error-handling paths which do fall back. If the intent is "no embeddings stored yet → no semantic results," this is fine; if users should still get FTS results, consider a fallback here too.src/adapters/mcp/server.ts (1)
374-377: Fragile error detection via string matching on serialized JSON.Line 376 uses
payload.includes('"error": {')to determineisError. This is brittle — it depends onJSON.stringifyformatting (e.g., the 2-space indent) and could false-positive on user content containing that substring. Sincetext()already returns results fromok()andfail()helpers, consider returning a structured{ payload, isError }tuple from the inner function instead of post-hoc string inspection.src/search/orchestrator.ts (2)
137-150:normalizeOptionspreserves singular fields via spread — verify this is intentional.The spread
...optionson line 146 means the returned object still carriesconceptandfilealongside the mergedconceptsandfiles. This works correctly becauseexecuteFilterOnlyStrategychecksoptions.conceptfor its early-return path (filter-only.ts line 7). However, this creates a subtle contract: strategies may see both singular and plural forms simultaneously.If the intent is for strategies to only consume plural forms, consider also unsetting the singular fields. If the current behavior is deliberate (filter-only needs them), a brief comment would help future readers.
152-170: SilentcatchinsearchUserMemoryswallows all errors.Line 167's bare
catchblock returns[]for any failure. While this prevents user-memory errors from breaking the main search flow, it also hides potentially actionable errors (e.g., DB corruption, schema mismatch). Consider logging atwarnlevel so issues are observable.
|
@cursor review |
|
@greptile review |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (6)
src/search/strategies/filter-only.ts (2)
20-47: Concept filters shadow file filters when both are provided.If a caller passes both
concept/conceptsandfile/files, only the concept branch executes and the file filters are silently ignored due to the early return on line 46. This is fine if intentional (and matches the prioritized-filtering design), but consider documenting this precedence or logging a warning so callers aren't surprised.
30-46: Each concept term queries with the fulllimit, potentially over-fetching.
searchByConceptis called per concept withlimit, so with N concepts the DB performs N queries returning up to N × limit rows before dedup. Same pattern on the file branch (line 54). This is acceptable for small N but worth keeping in mind if the concepts/files arrays grow large.src/modes/resolver.ts (2)
99-115: Redundant spread before explicit field assignments inmergeMode.
...baseand...overrideon lines 101–102 set all fields, but lines 103–109 immediately overwrite every documented field with explicit??fallback logic. The only fields actually contributed by the spreads that survive areextendsandlocale. This isn't a bug, but it's easy to misread.♻️ Optional: make the intent explicit
function mergeMode(base: ModeConfig, override: ModeConfigSource): ModeConfig { return { - ...base, - ...override, id: override.id, + extends: override.extends ?? base.extends, + locale: override.locale ?? base.locale, name: override.name ?? base.name, description: override.description ?? base.description, observationTypes: override.observationTypes ?? base.observationTypes, conceptVocabulary: override.conceptVocabulary ?? base.conceptVocabulary, entityTypes: override.entityTypes ?? base.entityTypes, relationshipTypes: override.relationshipTypes ?? base.relationshipTypes, promptOverrides: { ...(base.promptOverrides ?? {}), ...(override.promptOverrides ?? {}), }, }; }
120-137: Duplicateidacross files is silently last-writer-wins.If two JSON files in the modes directory declare the same
id,modes.set(parsed.id, parsed)overwrites without warning. The order depends onreaddirSync, which is filesystem-dependent. This could cause subtle, hard-to-debug behavior differences across environments.Consider logging a warning when a duplicate
idis encountered.src/modes/loader.ts (1)
17-18:loadModere-resolves inheritance on every call.Each call to
loadModerunsresolveById(including the recursive inheritance walk) against the cached raw sources. This is fine for typical config-loading frequency, but worth noting if this ever gets called in a hot path. A resolved-mode cache keyed bymodeIdcould be added if needed later.src/adapters/http/server.ts (1)
310-327: Consider reusing service instances instead of allocating per request.
new DefaultReadinessService()andnew DefaultSetupDiagnosticsService()(line 330) are instantiated on every request. If these are stateless, hoist them to module or factory scope to avoid unnecessary allocations on the hot path.♻️ Proposed refactor
export function createDashboardApp(deps: DashboardDeps): Hono { + const readinessService = new DefaultReadinessService(); + const diagnosticsService = new DefaultSetupDiagnosticsService(); const { projectPath, memoryEngine, ...Then use
readinessService.evaluate(...)anddiagnosticsService.run(...)in the handlers.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
1 participant
Summary
This PR applies the quality-first architecture migration end-to-end and removes compatibility-maintenance overhead that no longer aligns with project goals.
What changed
SearchStrategyRegistryand thin orchestrator composition.ModeResolverV2) with inheritance support and validation.ProviderFallbackPolicy.src/contracts/schemas.ts) and reused across adapters.mem-*tool naming.Validation
bun run lintpassesbun run typecheckpassesbun run test:repopasses (878 pass, 0 fail)bun run docs:buildpassesNotes
Note
Medium Risk
Moderate risk because it refactors core search execution, tightens MCP initialization behavior (drops legacy mode), and adds new operator HTTP endpoints/CLI that affect runtime and release workflows.
Overview
Governance + CI hardening: introduces
CODEOWNERS, a PR template, ADR/policy docs, and new CI workflows/steps (typecheck,lint,check:contracts,check:architecture,check:migration, plus scheduled architecture/nightly checks) while removing theexternal-compatworkflow and its supporting scripts/docs/artifacts.Contracts + ops surfaces: centralizes tool schemas/metadata into
src/contracts/schemas.ts(with acheck-contract-driftgate) and wires contract metadata through MCP/OpenCode/HTTP; adds HTTP endpoints forreadiness,diagnostics,tools/guide, and local-only queue operator routes (/v1/queue,/v1/queue/process) plus a newopen-mem-doctorCLI.Core behavior changes: refactors search into a pluggable strategy registry with extracted strategy implementations, replaces mode loading with
ModeResolverV2(supportsextends+ safer fallback copies), makes provider failover policy-driven, removesOPEN_MEM_MCP_COMPAT_MODE/legacy MCP mode (strict init required), and tightens a concept-search FTS query by quoting/escaping.Written by Cursor Bugbot for commit 514a31a. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Documentation
Refactor
Chores