Conversation
Ensure cache invalidation after mutations to avoid stale reads in interactive MCP sessions. Includes: - Cache invalidation in packages/cli/src/prolog.ts to ensure read-after-write consistency - Regression and unit tests that cover the bug in packages/mcp/tests/tools/crud.test.ts and documentation/tests/e2e/packed/mcp-protocol-regression.test.ts - ADR-015 documenting the design decision and tradeoffs (documentation/adr/ADR-015.md) - Changesets-driven version and changelog updates for packages/cli and packages/mcp (packages/*/CHANGELOG.md, packages/*/package.json) Refs: #58
There was a problem hiding this comment.
Pull request overview
This PR addresses read-after-write consistency issues in long-lived MCP sessions by ensuring cached Prolog query results are invalidated after successful mutations, and adds regression coverage to prevent stale reads from returning after upserts/deletes.
Changes:
- Invalidate
PrologProcessquery cache after successful non-cacheable (mutation) goals in interactive mode. - Add CRUD and packed E2E regression tests that exercise create/read/update/read and delete/read consistency in a single MCP process.
- Bump
kibi-cli/kibi-mcpversions and update CI/publish workflows.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/mcp/tests/tools/crud.test.ts | Adds CRUD regression coverage for immediate read-after-upsert and delete/query consistency. |
| packages/mcp/package.json | Bumps MCP version and updates kibi-cli dependency. |
| packages/mcp/CHANGELOG.md | Documents the cache invalidation fix and dependency update. |
| packages/cli/src/prolog.ts | Implements interactive-mode cache invalidation after successful non-cacheable goals. |
| packages/cli/package.json | Bumps CLI version. |
| packages/cli/CHANGELOG.md | Documents the cache invalidation fix and new regression coverage. |
| documentation/tests/e2e/packed/mcp-protocol-regression.test.ts | Adds packed E2E regression test for cache invalidation across upsert/delete. |
| documentation/symbols.yaml | Updates generated symbol coordinates metadata. |
| documentation/adr/ADR-015.md | Adds ADR documenting the cache invalidation decision and verification strategy. |
| .github/workflows/publish.yml | Updates action versions used during publish workflow. |
| .github/workflows/ci.yml | Adds a workflow_dispatch option to skip tests and updates action versions. |
You can also share your feedback on Copilot code review. Take the survey.
| if (!cacheable) { | ||
| this.invalidateCache(); | ||
| } | ||
| if (cacheable) { |
There was a problem hiding this comment.
Cache invalidation is currently gated solely on the cacheable flag computed from the original goalKey. In interactive mode, query(string[]) is rewritten into a parenthesized batch goal (e.g. (kb_assert_entity(...), kb_save)), which isCacheableGoal() will treat as cacheable (since it no longer starts with kb_assert_/kb_save), so successful batched mutations can avoid both invalidateCache() and proper non-caching. Consider marking any batch goal as non-cacheable (or computing cacheability per sub-goal and treating the batch as non-cacheable if any sub-goal is non-cacheable) to preserve read-after-write consistency for batched writes in Node interactive sessions.
| if (!cacheable) { | |
| this.invalidateCache(); | |
| } | |
| if (cacheable) { | |
| // Treat any batch goal as non-cacheable to preserve read-after-write consistency. | |
| const isBatchGoal = | |
| Array.isArray(goalKey) || | |
| /^\s*\([^)]*,/.test(normalizedGoal); | |
| const shouldCache = cacheable && !isBatchGoal; | |
| if (!shouldCache) { | |
| this.invalidateCache(); | |
| } | |
| if (shouldCache) { |
| it("should invalidate query cache after kb_upsert and kb_delete mutations", async () => { | ||
| const proc = spawn("node", [sandbox.kibiMcpBin], { | ||
| cwd: sandbox.repoDir, | ||
| env: sandbox.env, | ||
| stdio: ["pipe", "pipe", "pipe"], | ||
| }); | ||
| await new Promise((r) => setTimeout(r, 500)); | ||
|
|
There was a problem hiding this comment.
If any assertion in this test fails, the spawned MCP server process may never be stopped (since stopProcess(proc) is only reached at the end). That can leak child processes and make the test suite hang/flaky. Wrap the test body in try/finally and always call await stopProcess(proc) in the finally block (optionally also draining stdout/stderr).
|
@copilot open a new pull request to apply changes based on the comments in this thread |
…leanup Co-authored-by: Looted <6255880+Looted@users.noreply.github.com>
Fix batch goal cache mis-classification and test process leak in MCP interactive sessions
No description provided.