fix(goals): stop the generic memory table from re-surfacing phantom goals in the VFS#225
Conversation
… in the VFS VFS↔goals-table version skew: pre-routing hook versions (<=0.7.4) wrote goals as plain files into the generic memory table. The VFS bootstrap loaded every memory path, so those goal-shaped rows reappeared under /goal/<owner>/<status>/ even though they were never in hivemind_goals — visible in `ls /goal/...` but absent from `hivemind goal list` (which reads only the structured table), and vice versa. memoryBootstrap now skips goal/kpi-shaped paths when the dedicated table is configured, so that namespace is sourced solely from hivemind_goals/_kpis and the VFS and CLI agree. Falls back to keeping the rows when routing is off (no dedicated table = those rows are the only copy). Adds regression tests.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR prevents "phantom" goal/KPI entries from polluting the goal namespace by filtering goal/KPI-shaped paths from the memory table during bootstrap when dedicated structured tables are configured. Implementation adds path classification logic with regression tests covering both enabled and disabled routing scenarios. ChangesGoal/KPI Namespace Isolation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 |
Coverage ReportScope: files changed in this PR. Enforced threshold: 90% per metric (per file via
File Coverage — 2 files changed
Generated for commit a9d8b7a. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/claude-code/deeplake-fs.test.ts (1)
253-281: ⚡ Quick winWell-structured regression tests verify namespace isolation.
Both test cases correctly validate the fix: legacy goal-shaped memory rows are filtered when dedicated tables are configured, and preserved when routing is disabled. The assertions directly address the phantom-goal bug described in the PR objectives.
Optional: Consider adding KPI coverage
Since the filtering logic is symmetric for goals and KPIs, you could add a parallel test case for KPI paths to ensure completeness:
+ it("excludes legacy kpi-shaped rows from the memory table when kpisTable is set", async () => { + const client = makeSkewClient({ + memoryPaths: ["/kpi/goal-uuid/legacy-kpi.md", "/notes/hello.md"], + goals: [], + }); + const fs = await DeeplakeFs.create(client as never, "memory", "/", "sessions", { + goalsTable: "goals", + kpisTable: "kpis", + }); + expect(await fs.readdir("/notes")).toContain("hello.md"); + const kpiDir = await fs.readdir("/kpi"); + expect(kpiDir).not.toContain("goal-uuid"); // legacy KPI path hidden + });However, this is optional since the implementation uses identical logic for both cases.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/claude-code/deeplake-fs.test.ts` around lines 253 - 281, Add a parallel KPI-focused test mirroring the two goal tests to ensure namespace isolation is symmetric: create a test near "DeeplakeFs goal/kpi namespace isolation" that uses makeSkewClient with memoryPaths like "/kpi/alice/active/legacy.md" and a kpis array with a structured KPI (e.g., kpi_id "real") and assert DeeplakeFs.create(..., { kpisTable: "kpis", goalsTable: "goals" }) hides the legacy memory-row while the structured KPI surfaces, and another test that omits kpisTable to assert the legacy memory-row is preserved; use the same helpers (makeSkewClient, DeeplakeFs.create) and mirror the existing assertions for consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/claude-code/deeplake-fs.test.ts`:
- Around line 253-281: Add a parallel KPI-focused test mirroring the two goal
tests to ensure namespace isolation is symmetric: create a test near "DeeplakeFs
goal/kpi namespace isolation" that uses makeSkewClient with memoryPaths like
"/kpi/alice/active/legacy.md" and a kpis array with a structured KPI (e.g.,
kpi_id "real") and assert DeeplakeFs.create(..., { kpisTable: "kpis",
goalsTable: "goals" }) hides the legacy memory-row while the structured KPI
surfaces, and another test that omits kpisTable to assert the legacy memory-row
is preserved; use the same helpers (makeSkewClient, DeeplakeFs.create) and
mirror the existing assertions for consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 50ca9d00-9a88-4655-b0c6-c2849f5ad860
📒 Files selected for processing (2)
src/shell/deeplake-fs.tstests/claude-code/deeplake-fs.test.ts
…meout Coverage: src/shell/deeplake-fs.ts was unenforced and surfaced as a red PR coverage comment (~82% lines / 72% branches) because the file as a whole was graded against 90% even though the goals-skew change touched only a few lines. Add deeplake-fs-coverage.test.ts exercising the previously-untested paths against a stateful mock DeeplakeApi: goal/kpi structured-table routing (bootstrap, upsert INSERT/UPDATE, rm soft-close, mv status-transition + guards), the /graph/* VFS bridge, session-backed reads, the embeddings-disabled flush path, flush re-queue, and the virtual-index sessions section. File now at 97/91/97/99; pin a 90/90/90/90 per-file threshold in vitest.config.ts so regressions fail the gate. resume-brief: raise QUERY_TIMEOUT_MS from 1.5s to 3s so a cold backend (~1.9s) no longer silently loses the race and degrades the pickup to a plain welcome; still inside the 5s SessionStart hook budget.
| // session concat path, and the rm/mv goal flows against a stateful | ||
| // mock DeeplakeApi, bringing it to 97/91/97/99. Floor set at 90 to | ||
| // catch regressions on these paths going forward. | ||
| "src/shell/deeplake-fs.ts": { statements: 90, branches: 90, functions: 90, lines: 90 }, |
There was a problem hiding this comment.
@efenocchi This entry was added for two reasons:
-
deeplake-fs.tspreviously had no enforced coverage floor. The file sat at roughly 82% lines / 72% branches, which showed up as a red coverage comment on PRs but was never actually enforced — so regressions could slip through unnoticed. -
This PR adds substantial new tests.
deeplake-fs-coverage.test.tsexercises the structured-table dispatch,/graph/*bridge, session concat path, andrm/mvgoal flows against a stateful mockDeeplakeApi. That pushed measured coverage to ~97% statements / 91% branches / 97% functions / 99% lines.
By registering the threshold here at 90/90/90/90 now, any future change that regresses coverage on this file will fail CI immediately — enforcing the quality bar going forward rather than letting it silently erode again.
The inline comment above the entry in the diff spells this out:
"Previously unenforced: the file sat at ~82% lines / ~72% branches and only surfaced as a red PR-coverage comment … Floor set at 90 to catch regressions on these paths going forward."
| const kind = classifyPath(p); | ||
| if ((kind === "goal" && fs.goalsTable) || (kind === "kpi" && fs.kpisTable)) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
@efenocchi, this code change fixes a "phantom goal" bug in the VFS bootstrap. Here's what it does:
Background: The VFS (DeeplakeFs) has two storage layers for goals:
- A generic memory table (old approach) — older hook versions (≤0.7.4) wrote goal entries here as plain files.
- A dedicated
hivemind_goalstable (new approach) — the structured source of truth that the CLI (hivemind goal list) reads from.
The bug: When DeeplakeFs.create() bootstrapped the generic memory table, it loaded all rows indiscriminately — including any goal-shaped paths (e.g. /goal/<owner>/opened/<uuid>.md) that old hook versions had written there. This caused those goals to reappear in the VFS (ls /goal/... showed them) even though the structured table had no record of them → "phantom" goals.
The fix (the added lines):
const kind = classifyPath(p);
if ((kind === "goal" && fs.goalsTable) || (kind === "kpi" && fs.kpisTable)) {
continue;
}During bootstrap, for each row fetched from the generic memory table:
- It classifies the path using
classifyPath()to check if it looks like a goal or KPI path. - If it does and the corresponding dedicated table (
goalsTable/kpisTable) is configured, it skips loading that row into the VFS cache. - The goal/KPI data is then sourced exclusively from the dedicated structured tables (bootstrapped separately in
goalsBootstrap/kpisBootstrap). - If no dedicated table is configured (legacy/routing-off mode), the memory rows are kept as-is — they're the only copy.
This ensures the VFS and the CLI always show the same set of goals with no phantom duplicates.
2 participants
Problem
VFS↔goals-table version skew. Pre-routing hook versions (<=0.7.4) wrote goals as plain files into the generic
memorytable. The VFS bootstrap (memoryBootstrapindeeplake-fs.ts) loads every memory path, so those goal-shaped rows reappear under/goal/<owner>/<status>/...even though they were never inhivemind_goals.Result: a goal is visible in
ls /goal/...but absent fromhivemind goal list(the CLI reads only the structured table) — and vice-versa. Confirmed live: a phantom goal surfaced in the VFS thathivemind goal getreported as not found.Fix
memoryBootstrapnow skips goal/kpi-shaped paths when the dedicated table is configured, so that namespace is sourced solely fromhivemind_goals/hivemind_kpisand the VFS and CLI agree. When routing is off (no dedicated table) the rows are the only copy, so they are kept.Verification (against rebuilt bundle)
hivemind_goals✅ls✅mvopened→closed → table status updates, no dual-listing ✅deeplake-fssuite (74) green;tsc --noEmitcleanFixes the open goal "Fix Hivemind VFS to goals-table version skew".
Summary by CodeRabbit