forked from molecule-ai/molecule-core
fix(memory): self-review on PR #2956 — drop speculative field, tighten 503 match
Two issues caught in five-axis self-review of #2956: ## 1. Drop speculative source_workspace_id rendering The panel rendered a "from peer" badge based on `propagation.source_workspace_id`, claiming it surfaced cross- workspace propagation. But the OpenAPI spec at docs/api-protocol/memory-plugin-v1.yaml documents `propagation` as "Opaque metadata the plugin stores and returns. Reserved for future cross-namespace propagation semantics" — and a grep across workspace-server/internal/memory/ confirms NO writer in the codebase populates that key. The badge would never render against real data. Violates "don't design for hypothetical future requirements" from the project conventions. Drop the field from MemoryV2, the row badge, the test fixtures, and the JSDoc. When propagation gains a concrete shape, re-add backed by an actual writer. ## 2. Tighten 503 detection — match the literal contract string Pre-fix detection: `msg.includes('503') || msg.toLowerCase().includes('plugin is not configured')` False-positives on any unrelated 503 + on any error mentioning "plugin" + "configured" in any order. Post-fix: `msg.includes('MEMORY_PLUGIN_URL')` — the env var name is a hard-coded literal in workspace-server/internal/handlers/memories_v2.go's available() error, so this is a pinned cross-layer contract. Drift between the Go error message and the canvas detection now fails loud (TestMemoriesV2_PluginUnwired_All503 asserts the env var name in the response body; the canvas test asserts the same). Extracted as a named export `isPluginUnavailableError` so the detection is unit-testable and reusable. Added 4 direct tests: contract-string match, generic-503 false-negative, 401 false- negative, non-Error inputs. ## Test results - 30 component tests pass (was 26; +4 for isPluginUnavailableError) - Coverage on MemoryInspectorPanel.tsx: 100% lines, 100% functions (branch coverage up to 85.9% from 84.7% — speculative-field branches no longer count) - Full canvas suite: 1277/1277 pass across 91 files
This commit is contained in:
Hongming Wang
parent
7ee696ec9a
commit
c4d3c9a451
@ -57,8 +57,12 @@ export interface MemoryV2 {
|
||||
created_at: string;
|
||||
/** 0..1 plugin similarity score; only present when ?q= is set. */
|
||||
score?: number | null;
|
||||
/** workspace_id of the peer that originated this memory if propagation is in play. */
|
||||
source_workspace_id?: string;
|
||||
// Note: an earlier iteration of this type carried a `source_workspace_id`
|
||||
// field rendered as a "from peer" badge. The propagation contract that
|
||||
// would have populated it ("Reserved for future cross-namespace
|
||||
// propagation semantics" in memory-plugin-v1.yaml) is unimplemented —
|
||||
// nothing in the codebase writes that key. Removed in self-review.
|
||||
// Re-add when propagation gains a concrete shape.
|
||||
}
|
||||
|
||||
interface MemoriesResponse {
|
||||
@ -83,6 +87,24 @@ function sanitizeId(id: string): string {
|
||||
return id.replace(/[^a-zA-Z0-9]/g, '-');
|
||||
}
|
||||
|
||||
/**
|
||||
* Detect a memory-plugin-503 error from the api wrapper's stringified
|
||||
* Error message. Matches on the literal env-var name rather than the
|
||||
* status code, because the api shim renders status codes inside a
|
||||
* larger formatted message and a future status-code reformat would
|
||||
* silently break the detection.
|
||||
*
|
||||
* The substring `MEMORY_PLUGIN_URL` is hard-coded in the handler at
|
||||
* `workspace-server/internal/handlers/memories_v2.go:available()`,
|
||||
* so this is a pinned cross-layer contract — drift is caught by both
|
||||
* the Go test (TestMemoriesV2_PluginUnwired_All503) and the canvas
|
||||
* test (TestMemoryInspectorPanel — plugin unavailable).
|
||||
*/
|
||||
export function isPluginUnavailableError(err: unknown): boolean {
|
||||
const msg = err instanceof Error ? err.message : '';
|
||||
return msg.includes('MEMORY_PLUGIN_URL');
|
||||
}
|
||||
|
||||
function formatRelativeTime(iso: string): string {
|
||||
const diff = Date.now() - new Date(iso).getTime();
|
||||
if (diff < 60_000) return `${Math.floor(diff / 1000)}s`;
|
||||
@ -169,11 +191,10 @@ export function MemoryInspectorPanel({ workspaceId }: Props) {
|
||||
setNamespaces(data);
|
||||
setPluginUnavailable(false);
|
||||
} catch (e) {
|
||||
// 503 indicates the plugin isn't wired. Surface it specially —
|
||||
// anything else stays as a generic load failure that the
|
||||
// Plugin-unavailable (503) indicates MEMORY_PLUGIN_URL isn't set.
|
||||
// Anything else stays as a generic load failure that the
|
||||
// entries-load path will also flag.
|
||||
const msg = e instanceof Error ? e.message : '';
|
||||
if (msg.includes('503') || msg.toLowerCase().includes('plugin is not configured')) {
|
||||
if (isPluginUnavailableError(e)) {
|
||||
setPluginUnavailable(true);
|
||||
}
|
||||
setNamespaces({ readable: [], writable: [] });
|
||||
@ -205,12 +226,11 @@ export function MemoryInspectorPanel({ workspaceId }: Props) {
|
||||
: data.memories;
|
||||
setEntries(sorted);
|
||||
} catch (e) {
|
||||
const msg = e instanceof Error ? e.message : 'Failed to load memories';
|
||||
if (msg.includes('503') || msg.toLowerCase().includes('plugin is not configured')) {
|
||||
if (isPluginUnavailableError(e)) {
|
||||
setPluginUnavailable(true);
|
||||
setError(null); // surfaced via banner, not row error
|
||||
} else {
|
||||
setError(msg);
|
||||
setError(e instanceof Error ? e.message : 'Failed to load memories');
|
||||
}
|
||||
setEntries([]);
|
||||
} finally {
|
||||
@ -578,16 +598,6 @@ function MemoryEntryRow({ entry, onDelete }: MemoryEntryRowProps) {
|
||||
</span>
|
||||
)}
|
||||
|
||||
{/* Source workspace badge (propagated memory) */}
|
||||
{entry.source_workspace_id && (
|
||||
<span
|
||||
className="text-[9px] shrink-0 font-mono text-violet-400"
|
||||
title={`From: ${entry.source_workspace_id}`}
|
||||
data-testid="source-workspace-badge"
|
||||
>
|
||||
⇡{entry.source_workspace_id.slice(0, 6)}
|
||||
</span>
|
||||
)}
|
||||
|
||||
<span className="text-[9px] text-ink-soft shrink-0">
|
||||
{formatRelativeTime(entry.created_at)}
|
||||
|
||||
@ -13,7 +13,7 @@
|
||||
* - Search results sort by score descending
|
||||
* - Empty-state copy differs by query / plugin-state / no-data
|
||||
* - Per-row badges render (kind / source / pin / TTL / score /
|
||||
* source_workspace_id) and TTL countdown handles past/future/null
|
||||
* score) and TTL countdown handles past/future/null
|
||||
* - Delete (Forget) flow: optimistic removal, confirmation dialog,
|
||||
* server failure rolls back via reload
|
||||
* - formatTTL helper covers s/m/h/d/expired/null/invalid branches
|
||||
@ -61,6 +61,7 @@ import { api } from '@/lib/api';
|
||||
import {
|
||||
MemoryInspectorPanel,
|
||||
formatTTL,
|
||||
isPluginUnavailableError,
|
||||
type MemoryV2,
|
||||
type NamespacesResponse,
|
||||
} from '../MemoryInspectorPanel';
|
||||
@ -99,14 +100,13 @@ const MEM_PINNED: MemoryV2 = {
|
||||
created_at: '2026-04-17T12:00:00.000Z',
|
||||
};
|
||||
|
||||
const MEM_PROPAGATED: MemoryV2 = {
|
||||
id: 'mem-from-peer',
|
||||
const MEM_RUNTIME_CHECKPOINT: MemoryV2 = {
|
||||
id: 'mem-checkpoint',
|
||||
namespace: 'team:t-1',
|
||||
content: 'Cross-workspace fact',
|
||||
content: 'Runtime checkpoint',
|
||||
kind: 'checkpoint',
|
||||
source: 'runtime',
|
||||
pin: false,
|
||||
source_workspace_id: 'ws-peer-99',
|
||||
created_at: '2026-04-17T12:00:00.000Z',
|
||||
};
|
||||
|
||||
@ -142,6 +142,33 @@ function stubFetch(memories: MemoryV2[], namespaces: NamespacesResponse = NS_RES
|
||||
}) as typeof api.get);
|
||||
}
|
||||
|
||||
// ── isPluginUnavailableError helper ─────────────────────────────────────────
|
||||
|
||||
describe('isPluginUnavailableError', () => {
|
||||
it('matches the literal env var contract from the server handler', () => {
|
||||
expect(
|
||||
isPluginUnavailableError(
|
||||
new Error('API GET /workspaces/x/v2/memories: 503 {"error":"memory plugin is not configured (set MEMORY_PLUGIN_URL)"}'),
|
||||
),
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
it('does not false-match on generic 503 errors that don\'t mention the env var', () => {
|
||||
expect(isPluginUnavailableError(new Error('API GET /foo: 503 something else'))).toBe(false);
|
||||
});
|
||||
|
||||
it('does not false-match on plain 4xx errors', () => {
|
||||
expect(isPluginUnavailableError(new Error('API GET /foo: 401 unauthorized'))).toBe(false);
|
||||
});
|
||||
|
||||
it('returns false for non-Error inputs', () => {
|
||||
expect(isPluginUnavailableError(null)).toBe(false);
|
||||
expect(isPluginUnavailableError(undefined)).toBe(false);
|
||||
expect(isPluginUnavailableError('a string')).toBe(false);
|
||||
expect(isPluginUnavailableError({ message: 'MEMORY_PLUGIN_URL' })).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
// ── formatTTL helper ─────────────────────────────────────────────────────────
|
||||
|
||||
describe('formatTTL', () => {
|
||||
@ -242,7 +269,7 @@ describe('MemoryInspectorPanel — plugin unavailable', () => {
|
||||
});
|
||||
|
||||
it('shows the empty-state explaining plugin disabled', async () => {
|
||||
mockGet.mockRejectedValue(new Error('HTTP 503'));
|
||||
mockGet.mockRejectedValue(new Error('API GET /workspaces/x/v2/memories: 503 {"error":"memory plugin is not configured (set MEMORY_PLUGIN_URL)"}'));
|
||||
render(<MemoryInspectorPanel workspaceId="ws-1" />);
|
||||
await waitFor(() => screen.getByText(/Memory plugin disabled/i));
|
||||
});
|
||||
@ -346,8 +373,8 @@ describe('MemoryInspectorPanel — search', () => {
|
||||
// ── Per-row badges ───────────────────────────────────────────────────────────
|
||||
|
||||
describe('MemoryInspectorPanel — row badges', () => {
|
||||
it('renders kind, source, pin, TTL, source-workspace badges per shape', async () => {
|
||||
stubFetch([MEM_PINNED, MEM_PROPAGATED]);
|
||||
it('renders kind, source, pin, TTL badges per shape', async () => {
|
||||
stubFetch([MEM_PINNED, MEM_RUNTIME_CHECKPOINT]);
|
||||
render(<MemoryInspectorPanel workspaceId="ws-1" />);
|
||||
|
||||
await waitFor(() => {
|
||||
@ -357,15 +384,13 @@ describe('MemoryInspectorPanel — row badges', () => {
|
||||
expect(pinnedRow.querySelector('[data-testid="source-badge"]')?.textContent).toBe('user');
|
||||
expect(pinnedRow.querySelector('[data-testid="pin-badge"]')).toBeTruthy();
|
||||
expect(pinnedRow.querySelector('[data-testid="ttl-badge"]')?.textContent).toMatch(/^⌛\d+[hd]$/);
|
||||
expect(pinnedRow.querySelector('[data-testid="source-workspace-badge"]')).toBeNull();
|
||||
|
||||
// Propagated memory: kind=checkpoint, source=runtime, no pin, no TTL, source_workspace
|
||||
const propRow = screen.getByTestId('memory-row-mem-from-peer');
|
||||
// Checkpoint memory: kind=checkpoint, source=runtime, no pin, no TTL
|
||||
const propRow = screen.getByTestId('memory-row-mem-checkpoint');
|
||||
expect(propRow.querySelector('[data-testid="kind-badge"]')?.textContent).toBe('C');
|
||||
expect(propRow.querySelector('[data-testid="source-badge"]')?.textContent).toBe('runtime');
|
||||
expect(propRow.querySelector('[data-testid="pin-badge"]')).toBeNull();
|
||||
expect(propRow.querySelector('[data-testid="ttl-badge"]')).toBeNull();
|
||||
expect(propRow.querySelector('[data-testid="source-workspace-badge"]')?.textContent).toMatch(/^⇡ws-pee/);
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
Loading…
Reference in New Issue
Block a user