fix(memory,mcp): add migration 111 + real Zod schemas for substrate dispatch tools - #198
KooshaPari wants to merge 1 commit into
Conversation
Post-merge type/export skew across 6 subsystems (typecheck:core now clean): - executors: re-export mergeAbortSignals from base.ts barrel so ./antigravity keeps resolving it (extracted leaf lost its barrel re-export in the merge). - translator: add missing GEMINI_CLI member to FORMATS (responseTranslator branches on it). - memory: restore TV6 access telemetry dropped by the merge — access_count / last_accessed_at columns (migration 111), accessCount/lastAccessedAt on the Memory rows, createMemory Omit, and the recordMemoryAccess export retrieval.ts depends on. - combos: add first-class ComboModelStep.allowedConnectionIds (diegosouzapw#3266) parsed in normalizeComboStep; let normalizeStoredCombo return the narrowed ComboRecord shape so getComboById/getComboByName type-check. - mcp dispatch: type substrate dispatchTools inputSchema as real Zod schemas so server.registerTool accepts them and .parse() is valid. - types/validation: drop stale re-exports with no backing member (ProviderConnection/ProviderNode, ValidationResult).
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning Review limit reached
Next review available in: 1 second Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
Note
|
|
Superseded by #196 which merged the same typecheck fixes. Main is now at 0 typecheck errors. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2e777bce9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| @@ -0,0 +1,10 @@ | |||
| -- TV6 — Typed memory decay: track access frequency so decay can grant access-based immunity. | |||
There was a problem hiding this comment.
Adding this migration as 111 while the highest existing migration before it is 107 introduces new unexplained gaps 108, 109, and 110. I checked scripts/check/check-migration-numbering.mjs: its frozen gap allowlist only permits 026 and 055, so this new file adds fresh migration-numbering anomalies that the repo's migration-numbering test/gate will report. Please either use the next contiguous slot or add documented compatibility/allowlist handling for the skipped versions.
Useful? React with 👍 / 👎.
KooshaPari
changed the title
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| try { | ||
| const db = getDbInstance(); | ||
| const placeholders = unique.map(() => "?").join(", "); | ||
| const stmt = db.prepare( |
There was a problem hiding this comment.
SUGGESTION: Unbounded IN clause may hit SQLite parameter limit
recordMemoryAccess builds WHERE id IN (...) with one placeholder per id. SQLite defaults to SQLITE_MAX_VARIABLE_NUMBER = 999, so arrays larger than ~990 IDs cause prepare() to throw "too many SQL variables". The catch swallows it, but the entire batch fails silently. The current call site (retrieval.ts:237) stays well under the limit, but this is an exported helper so future callers could exceed it. Consider batching or capping the array size.
| -- | ||
| -- `access_count` increments each time a memory is injected into a prompt; `last_accessed_at` | ||
| -- records the most recent injection (and re-bases the decay clock so recently-used memories | ||
| -- survive). Both default to a never-accessed baseline, so every pre-existing row behaves as |
There was a problem hiding this comment.
SUGGESTION: Comment says "Both default" but last_accessed_at has no DEFAULT clause
Line 5-8 states both columns "default to a never-accessed baseline", but migration line 10 adds last_accessed_at as plain TEXT with no DEFAULT. Existing rows receive NULL, not a concrete default value. The code tolerates NULL (mapped to lastAccessedAt: null in rowToMemory), so runtime behavior is correct, but the comment overstates the column definition.
Code Review SummaryStatus: 2 Issues Found | Recommendation: Notes Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (10 files)
Reply with Reviewed by step-3.7-flash-20260528 · Input: 373.2K · Output: 31.5K · Cached: 3.3M |
1 participant
Rebased onto current origin/main. PR #196 already resolved the shared 19 typecheck errors; this PR is now reduced to the two changes that #196 left as latent runtime bugs:
src/lib/db/migrations/111_memory_typed_decay.sql— main'ssrc/lib/memory/store.tsreads/writesaccess_countandlast_accessed_at(9 references) but no migration on main creates those columns (max migration was 107). Without this, memory access-tracking SQL throwsno such columnat runtime. Adds the two columns (TV6 typed-decay telemetry, default-off).open-sse/mcp-server/tools/dispatchTools.ts— the 3 substrate tools declaredinputSchemaas a plain JSON-schema object literal, butserver.tscallsinputSchema.parse(). fix(typecheck): resolve 20 remaining type errors #196 silenced the type error withas unknown as z.ZodTypeAny, but a plain object has no.parse()method, so the tools would crash on first invocation. Converts them to realz.object({...})schemas (satisfies Hard Rule chore(OmniRoute): hygiene cleanup #7: validate inputs with Zod) so.parse()works at runtime.Verified
npm run typecheck:core= 0 errors on the rebased branch.