feat(prompt): optional CRUD onboarding + RAG-only via catalog slots #219
Conversation
…log slots; add CRUD onboarding guidance item; add decision tool; support RAG-only resources (Issue #184)
✅ Deploy Preview for fireproof-ai-builder ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
- Retrieval header handling ignores the
useflag and includes a localStorage fallback, risking accidental leakage of stale refs and violating the “conservative by default” goal. It should only attach whenuse === 'on'and members exist. - Global module state for retrieval can bleed across sessions/concurrent operations; thread state through function parameters instead.
- The tag extraction regex is brittle (strict newlines/line endings), and
resolveCatalogSlothardcodes tag mappings instead of using catalogslotsmetadata. preloadLlmsTextindexes the cache withllm.llmsTxtUrlwithout guarding for undefined, potentially polluting the cache under the "undefined" key. Also, the decision tool can be conditionally skipped when unnecessary to save cost/latency.
Additional notes (1)
- Performance |
app/prompts.ts:281-284
The decision tool is executed unconditionally even when no slots are configured forautoinclusion and retrieval is not inautomode. This imposes unnecessary latency/cost. You can preserve the parallelism while skipping the tool when it’s not needed.
Summary of changes
- Added a new guidance catalog item: app/llms/crud-onboarding.json with tagged content in app/llms/crud-onboarding.txt, including ragResources metadata.
- Updated prompt assembly (app/prompts.ts):
- Excluded guidance-only items from module selection/imports.
- Introduced decideCrudLookFeel tool run in parallel with module selection.
- Implemented slot resolution for instructionalText and demoDataGuidance (catalog or inline) with inclusion modes.
- Injected resolved slot content into the Guidelines section; removed previous hard-coded bullets.
- Added RAG-only retrieval configuration and surfaced refs via a header instead of concatenating text.
- Extended UserSettings schema (app/types/settings.ts) to include config.prompt.slots and config.prompt.retrieval.
- Added RAG header handling to streamAI (app/utils/streamHandler.ts).
- Added docs (docs/prompt-slots-and-retrieval.md) and comprehensive tests for slots, retrieval, and import generation sorting.
- Minor sorting/filters adjustments in generateImportStatements and chosen LLMs handling.
| // Active retrieval configuration for stream calls (RAG-only) | ||
| let activeRetrievalConfig: { use: 'auto' | 'on' | 'off'; members: Array<{ ref: string }> } = { | ||
| use: 'off', | ||
| members: [], | ||
| }; | ||
|
|
||
| export function getActiveRetrievalConfig() { | ||
| return activeRetrievalConfig; | ||
| } | ||
|
|
||
| function setActiveRetrievalConfig(cfg: { | ||
| use?: 'auto' | 'on' | 'off'; | ||
| members?: Array<{ ref: string }>; | ||
| }) { | ||
| activeRetrievalConfig = { | ||
| use: cfg.use ?? 'off', | ||
| members: Array.isArray(cfg.members) ? cfg.members : [], | ||
| }; | ||
| } | ||
|
|
||
| // Test-only helper | ||
| export function __setActiveRetrievalConfigForTests(cfg: { | ||
| use?: 'auto' | 'on' | 'off'; | ||
| members?: Array<{ ref: string }>; | ||
| }) { | ||
| setActiveRetrievalConfig(cfg); | ||
| } | ||
|
|
There was a problem hiding this comment.
Global, mutable module state for retrieval (activeRetrievalConfig) risks cross-request/session leakage and race conditions. If two prompts are built concurrently for different sessions, the later call to setActiveRetrievalConfig could affect the earlier streamAI invocation. This also makes testing and reasoning harder. Consider threading retrieval configuration through function parameters instead of global state.
Suggestion
Refactor to avoid module-global state. For example, return the resolved retrieval members alongside the prompt and pass them into streamAI explicitly:
- Change makeBaseSystemPrompt to return
{ prompt: string, ragMembers: Array<{ref:string}> }. - Change streamAI to accept an optional
ragMembersargument and set the header based on it, removing the getActiveRetrievalConfig call.
Example:
// prompts.ts
export async function makeBaseSystemPrompt(model: string, sessionDoc?: any): Promise<{ prompt: string, ragMembers: Array<{ref:string}> }> {
// ...
const ragMembers = retrievalMembers.map(m => ({ ref: m.ref }));
return { prompt: `...`, ragMembers };
}
// streamHandler.ts
export async function streamAI( /* existing args */, extra?: { ragMembers?: Array<{ref:string}> }) {
// ...
if (extra?.ragMembers?.length) {
options.headers["X-RAG-Refs"] = encodeURIComponent(JSON.stringify(extra.ragMembers));
}
// ...
}
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this refactor.
| }); | ||
| } |
There was a problem hiding this comment.
preloadLlmsText writes to and reads from llmsTextCache[llm.llmsTxtUrl] without guarding for undefined. Guidance items likely do not define llmsTxtUrl, causing a pollution of the cache under the "undefined" key and potentially short-circuiting subsequent loads unpredictably. This is a subtle correctness issue introduced by adding guidance items.
Suggestion
Guard all reads/writes keyed by llm.llmsTxtUrl:
export async function preloadLlmsText(): Promise<void> {
llmsList.forEach((llm: any) => {
const byNameCached = !!llmsTextCache[llm.name];
const byUrlCached = llm.llmsTxtUrl ? !!llmsTextCache[llm.llmsTxtUrl] : false;
if (byNameCached || byUrlCached) return;
const text = loadLlmsTextByName(llm.name);
if (text) {
llmsTextCache[llm.name] = text;
if (llm.llmsTxtUrl) llmsTextCache[llm.llmsTxtUrl] = text;
}
});
}
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this fix.
| function extractTaggedSection(txt: string, tag: string): string | undefined { | ||
| const re = new RegExp(`<${tag}>\\n([\\s\\S]*?)\\n<\\/${tag}>`); | ||
| const m = txt.match(re); | ||
| return m ? m[1].trim() : undefined; | ||
| } |
There was a problem hiding this comment.
The regex in extractTaggedSection requires hard newlines immediately after the opening tag and before the closing tag and assumes LF-only line endings. This is brittle and will fail with different whitespace/CRLF or if tags are formatted slightly differently. It’s easy to make the extraction robust without sacrificing simplicity.
Suggestion
Relax the regex to tolerate optional whitespace and CRLF around boundaries:
function extractTaggedSection(txt: string, tag: string): string | undefined {
const re = new RegExp(`<${tag}>[\r\n]*([\s\S]*?)[\r\n]*<\/${tag}>`);
const m = txt.match(re);
return m ? m[1].trim() : undefined;
}
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this change.
| function resolveCatalogSlot(refOrUndefined: string | undefined, key: string | undefined) { | ||
| const ref = refOrUndefined || DEFAULT_CATALOG_REF; // name@version | ||
| const [name] = ref.split('@'); | ||
| const text = llmsTextCache[name] || llmsTextContent[name]; | ||
| if (!text) return undefined; | ||
| let tag = ''; | ||
| if (name === 'crud-onboarding') { | ||
| tag = key === 'demoDataGuidance' ? 'demo-data-guidance' : 'instructional-guidance'; | ||
| } else { | ||
| tag = key || ''; | ||
| } | ||
| if (!tag) return undefined; | ||
| return extractTaggedSection(text, tag); | ||
| } |
There was a problem hiding this comment.
resolveCatalogSlot hardcodes tag names for crud-onboarding instead of using the slots mapping that already exists in the catalog JSON. This duplicates knowledge, makes future catalog items harder to support, and risks drift if the mapping changes. Prefer deriving the tag from the catalog item metadata.
Suggestion
Look up the catalog item's slots mapping to resolve the correct tag dynamically:
function resolveCatalogSlot(refOrUndefined: string | undefined, key: string | undefined) {
const ref = refOrUndefined || DEFAULT_CATALOG_REF;
const [name] = ref.split('@');
const text = llmsTextCache[name] || llmsTextContent[name];
if (!text || !key) return undefined;
// Find the JSON entry for this catalog item
const item: any = llmsList.find((l: any) => l.name === name);
const tagName = item?.slots?.[key] || (name === 'crud-onboarding' && (key === 'demoDataGuidance' ? 'demo-data-guidance' : 'instructional-guidance'));
if (!tagName) return undefined;
return extractTaggedSection(text, tagName);
}
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this improvement.
| // Provide retrieval-only members to backend without concatenating to prompt text | ||
| try { | ||
| const rag = getActiveRetrievalConfig(); | ||
| if (rag && Array.isArray(rag.members) && rag.members.length > 0) { | ||
| (options.headers as any)['X-RAG-Refs'] = encodeURIComponent(JSON.stringify(rag.members)); | ||
| } else { | ||
| const ls = | ||
| typeof localStorage !== 'undefined' ? localStorage.getItem('vibes-rag-refs') : null; | ||
| if (ls) (options.headers as any)['X-RAG-Refs'] = encodeURIComponent(ls); | ||
| } | ||
| } catch (_err) { | ||
| // Non-fatal: retrieval config not available | ||
| } | ||
|
|
There was a problem hiding this comment.
RAG header injection ignores the configured use flag and falls back to localStorage when no active members are present. This violates the conservative default principle and can accidentally leak stale retrieval refs into unrelated sessions. The header should only be attached when use === 'on' and members are present; consider removing the localStorage fallback entirely.
Suggestion
Honor the use switch and drop the fallback to localStorage to avoid stale state bleed:
try {
const rag = getActiveRetrievalConfig();
if (rag?.use === 'on' && Array.isArray(rag.members) && rag.members.length > 0) {
(options.headers as any)['X-RAG-Refs'] = encodeURIComponent(JSON.stringify(rag.members));
}
} catch (_) {
// Non-fatal
}
If you still need a fallback, gate it behind an explicit opt-in. Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this fix.
| // RAG-only retrieval config (not concatenated into text) | ||
| const retrievalUse = retrieval?.use || 'off'; | ||
| const retrievalMembers = Array.isArray(retrieval?.members) ? retrieval.members : []; | ||
| setActiveRetrievalConfig({ | ||
| use: retrievalUse === 'auto' ? (autoDemo ? 'on' : 'off') : retrievalUse, | ||
| members: retrievalMembers | ||
| .map((m: any) => ({ ref: (m as any).ref })) | ||
| .filter((m: { ref?: string }) => !!m.ref), | ||
| }); |
There was a problem hiding this comment.
retrieval.use: 'auto' is currently tied to autoDemo (includeDemoData) rather than a dedicated retrieval signal. This coupling is surprising and can enable retrieval based on demo-data guidance heuristics instead of actual retrieval intent. Consider deriving retrieval auto decision from a more appropriate indicator (e.g., appType === 'crud' plus presence of configured members) or keeping it disabled unless explicitly enabled.
Suggestion
Decouple retrieval auto logic from demo-data guidance. For example:
const hasMembers = retrievalMembers.length > 0;
const autoRetrieval = decision?.appType === 'crud' && hasMembers; // or another explicit rule
setActiveRetrievalConfig({
use: retrievalUse === 'auto' ? (autoRetrieval ? 'on' : 'off') : retrievalUse,
members: retrievalMembers.map((m: any) => ({ ref: String(m.ref) })).filter(m => !!m.ref),
});
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this change.
|
close for #229 |
Summary: Make Demo Data and Instructional Text guidance optional and catalog-driven, add a decision tool for CRUD vs custom look-and-feel, and support retrieval-only (RAG) resources that are not concatenated into the LLM text.
Context
Issue #184 calls out that Demo Data (and Instructional Text) are too bullish. This PR moves both into a library catalog item with slots, adds config to control inclusion, and introduces a decision tool so CRUD-style apps get onboarding by default while custom/look-and-feel apps do not. Retrieval-only assets (RAG) can be enabled without being pasted into the prompt text.
Changes
Acceptance criteria mapping
Open questions
Verification
pnpm check # results: 67 test files passed; 373 tests (369 passed, 4 skipped); typecheck OK; prettier formattedCloses #184