Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
/**
* Behavior tests for the `memory` node config form (issue #5226). Covers the
* per-operation progressive disclosure (`recall`/`search` vs `flavour` vs
* `remember`/`forget`) and the hard UI rule mirroring the engine invariant:
* a `remember`/`forget` node's `scope` control must never offer `user`.
* `useT()` falls back to the bundled English map with no provider mounted
* (same convention as the sibling `nodeConfigForms.test.tsx`).
*/
import { fireEvent, render, screen, within } from '@testing-library/react';
import { describe, expect, it, vi } from 'vitest';

import { MemoryForm } from '../memoryFields';

function renderMemoryForm(config: Record<string, unknown> = {}) {
const onChange = vi.fn();
render(<MemoryForm config={config} onChange={onChange} />);
return { onChange };
}

describe('MemoryForm', () => {
it('defaults to recall: shows scope (including user) and query, no key/value/flavour', () => {
renderMemoryForm();
const scope = screen.getByTestId('node-config-memory-scope');
const options = within(scope).getAllByRole('option');
expect(options.map(o => o.getAttribute('value'))).toEqual(['user', 'flow', 'flows']);
expect(screen.getByTestId('node-config-memory-query')).toBeInTheDocument();
expect(screen.queryByTestId('node-config-memory-flavour')).not.toBeInTheDocument();
expect(screen.queryByTestId('node-config-memory-key')).not.toBeInTheDocument();
expect(screen.queryByTestId('node-config-memory-value')).not.toBeInTheDocument();
});

it('search behaves like recall: scope includes user, and query is shown', () => {
renderMemoryForm({ operation: 'search' });
const scope = screen.getByTestId('node-config-memory-scope');
const options = within(scope).getAllByRole('option');
expect(options.map(o => o.getAttribute('value'))).toContain('user');
expect(screen.getByTestId('node-config-memory-query')).toBeInTheDocument();
});

it('flavour shows only the flavour slug field, no scope or query', () => {
renderMemoryForm({ operation: 'flavour' });
expect(screen.getByTestId('node-config-memory-flavour')).toBeInTheDocument();
expect(screen.queryByTestId('node-config-memory-scope')).not.toBeInTheDocument();
expect(screen.queryByTestId('node-config-memory-query')).not.toBeInTheDocument();
});

it('remember hides the user scope option (only flow) and shows key + value', () => {
renderMemoryForm({ operation: 'remember', scope: 'flow' });
const scope = screen.getByTestId('node-config-memory-scope');
const options = within(scope).getAllByRole('option');
expect(options.map(o => o.getAttribute('value'))).toEqual(['flow']);
expect(within(scope).queryByRole('option', { name: /read-only/i })).not.toBeInTheDocument();
expect(screen.getByTestId('node-config-memory-key')).toBeInTheDocument();
expect(screen.getByTestId('node-config-memory-value')).toBeInTheDocument();
// Not a read/search operation, so no query field.
expect(screen.queryByTestId('node-config-memory-query')).not.toBeInTheDocument();
});

it('forget hides the user scope option and shows key but not value', () => {
renderMemoryForm({ operation: 'forget', scope: 'flow' });
const scope = screen.getByTestId('node-config-memory-scope');
const options = within(scope).getAllByRole('option');
expect(options.map(o => o.getAttribute('value'))).toEqual(['flow']);
expect(screen.getByTestId('node-config-memory-key')).toBeInTheDocument();
expect(screen.queryByTestId('node-config-memory-value')).not.toBeInTheDocument();
});

it('clamps scope to flow when switching from recall(user) to remember', () => {
const { onChange } = renderMemoryForm({ operation: 'recall', scope: 'user' });
fireEvent.change(screen.getByTestId('node-config-memory-operation'), {
target: { value: 'remember' },
});
expect(onChange).toHaveBeenLastCalledWith({ operation: 'remember', scope: 'flow' });
});

it('self-heals a pre-existing invalid remember+user config on mount', () => {
const { onChange } = renderMemoryForm({ operation: 'remember', scope: 'user' });
expect(onChange).toHaveBeenCalledWith({ scope: 'flow' });
});

it('emits a query patch as the query is typed for recall', () => {
const { onChange } = renderMemoryForm({ operation: 'recall' });
fireEvent.change(screen.getByTestId('node-config-memory-query'), {
target: { value: '=item.title' },
});
expect(onChange).toHaveBeenLastCalledWith({ query: '=item.title' });
});

it('emits limit as a number as it is typed', () => {
const { onChange } = renderMemoryForm({ operation: 'recall' });
fireEvent.change(screen.getByTestId('node-config-memory-limit'), { target: { value: '5' } });
expect(onChange).toHaveBeenLastCalledWith({ limit: 5 });
});

it('emits undefined for limit when the field is cleared back to empty', () => {
// Start from an already-populated `limit` so clearing it is a genuine
// DOM value change (a fresh controlled input already renders blank).
const { onChange } = renderMemoryForm({ operation: 'recall', limit: 5 });
fireEvent.change(screen.getByTestId('node-config-memory-limit'), { target: { value: '' } });
expect(onChange).toHaveBeenLastCalledWith({ limit: undefined });
});

it('emits min_score as a number as it is typed', () => {
const { onChange } = renderMemoryForm({ operation: 'recall' });
fireEvent.change(screen.getByTestId('node-config-memory-min-score'), {
target: { value: '0.5' },
});
expect(onChange).toHaveBeenLastCalledWith({ min_score: 0.5 });
});

it('emits undefined for min_score when the field is cleared back to empty', () => {
// Start from an already-populated `min_score` so clearing it is a genuine
// DOM value change (a fresh controlled input already renders blank).
const { onChange } = renderMemoryForm({ operation: 'recall', min_score: 0.5 });
fireEvent.change(screen.getByTestId('node-config-memory-min-score'), { target: { value: '' } });
expect(onChange).toHaveBeenLastCalledWith({ min_score: undefined });
});
});
Comment thread
greptile-apps[bot] marked this conversation as resolved.
235 changes: 235 additions & 0 deletions app/src/components/flows/canvas/nodeConfig/memoryFields.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,235 @@
/**
* `memory` node config form (issue #5226 — 13th tinyflows `NodeKind`).
* Declarative, in-graph memory access: `recall`/`search`/`flavour`/`people`
* read; `remember`/`forget` write. Spec: `my_docs/memory_access_in_workflows/08-memory-node.md`.
*
* Field keys mirror what the engine actually reads at runtime (per the
* design doc's `MemoryProvider` trait + node config table):
* - `operation` — always. `recall` · `search` · `flavour` · `people` ·
* `remember` · `forget`.
* - `scope` — for `recall`/`search`/`remember`/`forget`. `user` and
* `flows` are read-only scopes (only offered for the read operations);
* `remember`/`forget` may only ever target `flow`.
* - `query` — `=`-bindable, for `recall`/`search` (and optionally
* `people`, whose `people(query: Option<&str>)` signature takes one too).
* - `flavour` — a plain slug (not an expression), for `flavour`.
* - `key` — `=`-bindable, for `remember`/`forget`.
* - `value` — `=`-bindable, for `remember` only.
* - `limit` / `min_score` — optional numeric caps on the read operations.
*
* Progressive disclosure mirrors the other multi-field forms in
* `nodeConfigForms.tsx` (e.g. `TriggerForm`'s `trigger_kind` switch): only
* the fields relevant to the selected `operation` render.
*/
import createDebug from 'debug';
import { useEffect } from 'react';

import { useT } from '../../../../lib/i18n/I18nContext';
import {
configNumber,
configString,
ExpressionField,
NumberField,
SelectField,
} from './nodeConfigFields';
import type { UpstreamExpressionOption } from './upstreamOptions';

const log = createDebug('app:flows:nodeConfig:memory');

const MEMORY_OPERATIONS = ['recall', 'search', 'flavour', 'people', 'remember', 'forget'] as const;
type MemoryOperation = (typeof MEMORY_OPERATIONS)[number];

/**
* The seven persona facets the `flavour` operation's `memory_flavour` engine
* reader (`src/openhuman/memory/tools/flavour.rs`) accepts — any other slug
* returns "Unknown flavour". Rendered as a dropdown rather than free text so
* an author can't type an invalid slug (e.g. `email-tone`) in the first place.
*/
const MEMORY_FLAVOURS = [
'communication',
'coding_style',
'stack',
'workflow',
'environment',
'directives',
'anti_preferences',
] as const;

/** Operations that read/write at a `scope`. `flavour` and `people` don't take one. */
const SCOPED_OPERATIONS = new Set<MemoryOperation>(['recall', 'search', 'remember', 'forget']);

/**
* `remember`/`forget` are flow-only writes — the engine rejects a
* `user`-scoped write at `validate_all` time (the hard security invariant
* from `05-security.md`/`08-memory-node.md`). The UI must never let an
* author construct that pair, so these operations get a `scope` control
* offering only `flow`, never `user` (or the other read-only scope, `flows`).
*/
const WRITE_OPERATIONS = new Set<MemoryOperation>(['remember', 'forget']);

/** Operations that take a free-text `query`. */
const QUERY_OPERATIONS = new Set<MemoryOperation>(['recall', 'search', 'people']);

/** Operations whose result set can be capped/thresholded. */
const RESULT_OPERATIONS = new Set<MemoryOperation>(['recall', 'search', 'people']);

/**
* Deliberately narrower than `NodeConfigFormProps` (`nodeConfigForms.tsx`) —
* this form needs no `connections` (memory nodes don't use the credential
* picker), so it isn't imported here just to keep the shape identical. A
* component typed against this subset is still assignable into
* `NODE_CONFIG_FORMS`'s `NodeConfigForm` slot (the fuller prop type has
* every property this one requires).
*/
export interface MemoryFormProps {
config: Record<string, unknown>;
onChange: (patch: Record<string, unknown>) => void;
upstreamOptions?: UpstreamExpressionOption[];
}

export function MemoryForm({ config, onChange, upstreamOptions }: MemoryFormProps) {
const { t } = useT();
const operation = (configString(config, 'operation') || 'recall') as MemoryOperation;
const scope = configString(config, 'scope');
const isWrite = WRITE_OPERATIONS.has(operation);

// Self-heal: if this node arrives with an already-invalid `remember`/
// `forget` + non-`flow` scope (a raw-JSON edit, an older draft, a
// workflow-builder proposal), correct it as soon as the form mounts/updates
// rather than only guarding the operation-change handler below — the
// invariant must hold no matter how the bad pair got here.
useEffect(() => {
if (isWrite && scope !== 'flow') {
log(
'self-heal: clamping scope=%s to flow for write operation=%s',
scope || '(empty)',
operation
);
onChange({ scope: 'flow' });
}
}, [isWrite, scope, operation, onChange]);

const handleOperationChange = (value: string) => {
const next = value as MemoryOperation;
const patch: Record<string, unknown> = { operation: next };
// Hard UI rule mirroring the engine invariant: switching straight into a
// write operation while a read-only scope (`user`/`flows`) is selected
// must clamp to `flow` in the same patch, not rely on the effect above
// (which would otherwise author one extra, momentarily-invalid patch).
if (WRITE_OPERATIONS.has(next) && scope !== 'flow') {
patch.scope = 'flow';
}
log('operation change: %s -> %s (scope patch=%s)', operation, next, patch.scope ?? 'none');
onChange(patch);
};

const scopeOptions = isWrite
? [{ value: 'flow', label: t('flows.nodeConfig.memory.scope_flow') }]
: [
{ value: 'user', label: t('flows.nodeConfig.memory.scope_user') },
{ value: 'flow', label: t('flows.nodeConfig.memory.scope_flow') },
{ value: 'flows', label: t('flows.nodeConfig.memory.scope_flows') },
];

return (
<div className="space-y-3">
<SelectField
label={t('flows.nodeConfig.memory.operationLabel')}
value={operation}
onChange={handleOperationChange}
testId="node-config-memory-operation"
options={MEMORY_OPERATIONS.map(op => ({
value: op,
label: t(`flows.nodeConfig.memory.operation_${op}`),
}))}
/>

{SCOPED_OPERATIONS.has(operation) && (
<SelectField
label={t('flows.nodeConfig.memory.scopeLabel')}
hint={
isWrite
? t('flows.nodeConfig.memory.scopeWriteHint')
: t('flows.nodeConfig.memory.scopeHint')
}
value={isWrite ? 'flow' : scope || 'user'}
onChange={v => onChange({ scope: v })}
testId="node-config-memory-scope"
options={scopeOptions}
/>
)}

{QUERY_OPERATIONS.has(operation) && (
<ExpressionField
label={t('flows.nodeConfig.memory.queryLabel')}
hint={operation === 'people' ? t('flows.nodeConfig.memory.queryOptionalHint') : undefined}
value={configString(config, 'query')}
onChange={v => onChange({ query: v })}
placeholder="=item.title"
upstreamOptions={upstreamOptions}
testId="node-config-memory-query"
/>
)}

{operation === 'flavour' && (
<SelectField
label={t('flows.nodeConfig.memory.flavourLabel')}
hint={t('flows.nodeConfig.memory.flavourHint')}
value={configString(config, 'flavour') || MEMORY_FLAVOURS[0]}
onChange={v => onChange({ flavour: v })}
testId="node-config-memory-flavour"
options={MEMORY_FLAVOURS.map(facet => ({
value: facet,
label: t(`flows.nodeConfig.memory.flavour_${facet}`),
}))}
/>
)}

{(operation === 'remember' || operation === 'forget') && (
<ExpressionField
label={t('flows.nodeConfig.memory.keyLabel')}
value={configString(config, 'key')}
onChange={v => onChange({ key: v })}
placeholder="=item.id"
upstreamOptions={upstreamOptions}
testId="node-config-memory-key"
/>
)}

{operation === 'remember' && (
<ExpressionField
label={t('flows.nodeConfig.memory.valueLabel')}
value={configString(config, 'value')}
onChange={v => onChange({ value: v })}
placeholder="=item"
upstreamOptions={upstreamOptions}
testId="node-config-memory-value"
/>
)}

{RESULT_OPERATIONS.has(operation) && (
<div className="grid grid-cols-2 gap-3">
<NumberField
label={t('flows.nodeConfig.memory.limitLabel')}
hint={t('flows.nodeConfig.memory.limitHint')}
value={configNumber(config, 'limit')}
onChange={v => onChange({ limit: v })}
min={1}
step={1}
testId="node-config-memory-limit"
/>
<NumberField
label={t('flows.nodeConfig.memory.minScoreLabel')}
hint={t('flows.nodeConfig.memory.minScoreHint')}
value={configNumber(config, 'min_score')}
onChange={v => onChange({ min_score: v })}
min={0}
max={1}
step={0.05}
testId="node-config-memory-min-score"
/>
Comment on lines +212 to +230

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Enforce numeric bounds before persisting.

min/max/step do not block interactive changes, so this can save limit: 0, fractional limits, or min_score outside 0..1. Normalize or reject values in these callbacks and cover the boundaries.

Proposed fix
+const normalizeLimit = (value: number | undefined) =>
+  value === undefined || !Number.isFinite(value) ? undefined : Math.max(1, Math.floor(value));
+
+const normalizeMinScore = (value: number | undefined) =>
+  value === undefined || !Number.isFinite(value) ? undefined : Math.min(1, Math.max(0, value));
+
-            onChange={v => onChange({ limit: v })}
+            onChange={v => onChange({ limit: normalizeLimit(v) })}
...
-            onChange={v => onChange({ min_score: v })}
+            onChange={v => onChange({ min_score: normalizeMinScore(v) })}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<NumberField
label={t('flows.nodeConfig.memory.limitLabel')}
hint={t('flows.nodeConfig.memory.limitHint')}
value={configNumber(config, 'limit')}
onChange={v => onChange({ limit: v })}
min={1}
step={1}
testId="node-config-memory-limit"
/>
<NumberField
label={t('flows.nodeConfig.memory.minScoreLabel')}
hint={t('flows.nodeConfig.memory.minScoreHint')}
value={configNumber(config, 'min_score')}
onChange={v => onChange({ min_score: v })}
min={0}
max={1}
step={0.05}
testId="node-config-memory-min-score"
/>
const normalizeLimit = (value: number | undefined) =>
value === undefined || !Number.isFinite(value) ? undefined : Math.max(1, Math.floor(value));
const normalizeMinScore = (value: number | undefined) =>
value === undefined || !Number.isFinite(value) ? undefined : Math.min(1, Math.max(0, value));
<NumberField
label={t('flows.nodeConfig.memory.limitLabel')}
hint={t('flows.nodeConfig.memory.limitHint')}
value={configNumber(config, 'limit')}
onChange={v => onChange({ limit: normalizeLimit(v) })}
min={1}
step={1}
testId="node-config-memory-limit"
/>
<NumberField
label={t('flows.nodeConfig.memory.minScoreLabel')}
hint={t('flows.nodeConfig.memory.minScoreHint')}
value={configNumber(config, 'min_score')}
onChange={v => onChange({ min_score: normalizeMinScore(v) })}
min={0}
max={1}
step={0.05}
testId="node-config-memory-min-score"
/>
🤖 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 `@app/src/components/flows/canvas/nodeConfig/memoryFields.tsx` around lines 194
- 212, Enforce valid numeric values in the onChange callbacks for the memory
limit and min_score NumberFields before calling onChange: normalize or reject
limit values below 1 or fractional values, and min_score values outside 0..1.
Preserve valid inputs and cover the boundary cases, rather than relying on the
visual min/max/step props.

</div>
)}
</div>
);
}
Loading
Loading