From e052d5ee4598332a6491b653d70fc82a08642d61 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Wed, 22 Jul 2026 11:35:57 +0000 Subject: [PATCH] fix(copilot): only warn on saved write_flow; add regression tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review: writeFlowDraft reports conflicts/persistence errors as {success:false} rather than throwing, so the empty-body warning must be folded into the JSON result only on a successful save — otherwise the model is told to set_flow_module_code on a flow that was never saved (stale or nonexistent draft). Add core.test.ts coverage for the empty-body warning (top-level, nested, preprocessor, failure; populated suppressed), the no-warning-on-failed-save path, and the malformed-JSON escaping hint. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../copilot/chat/global/core.test.ts | 86 +++++++++++++++++++ .../components/copilot/chat/global/core.ts | 38 ++++++-- 2 files changed, 115 insertions(+), 9 deletions(-) diff --git a/frontend/src/lib/components/copilot/chat/global/core.test.ts b/frontend/src/lib/components/copilot/chat/global/core.test.ts index 78da313953..1866adf294 100644 --- a/frontend/src/lib/components/copilot/chat/global/core.test.ts +++ b/frontend/src/lib/components/copilot/chat/global/core.test.ts @@ -2857,6 +2857,92 @@ describe('global AI tools', () => { ).resolves.toBe(code) }) + it('warns about every empty rawscript body (top-level, nested, preprocessor, failure) and skips populated ones', async () => { + const result = JSON.parse( + await callGlobalTool('write_flow', { + path: 'f/flows/empty-bodies', + modules: JSON.stringify([ + { + id: 'empty_top', + value: { type: 'rawscript', language: 'bun', content: '', input_transforms: {} } + }, + { + id: 'filled', + value: { + type: 'rawscript', + language: 'bun', + content: 'export async function main() { return 1 }', + input_transforms: {} + } + }, + { + id: 'loop', + value: { + type: 'forloopflow', + iterator: { type: 'javascript', expr: 'results.filled' }, + skip_failures: false, + modules: [ + { + id: 'empty_nested', + value: { + type: 'rawscript', + language: 'bun', + content: '', + input_transforms: {} + } + } + ] + } + } + ]), + preprocessor_module: JSON.stringify({ + id: 'preprocessor', + value: { type: 'rawscript', language: 'bun', content: '', input_transforms: {} } + }), + failure_module: JSON.stringify({ + id: 'failure', + value: { type: 'rawscript', language: 'bun', content: '', input_transforms: {} } + }) + }) + ) + + expect(result.success).toBe(true) + expect(result.message).toContain('set_flow_module_code') + for (const id of ['empty_top', 'empty_nested', 'preprocessor', 'failure']) { + expect(result.message).toContain(`"${id}"`) + } + expect(result.message).not.toContain('"filled"') + }) + + it('does not append the empty-body warning when the flow was not saved', async () => { + const path = 'f/flows/write-fails' + failingWrites.add(`flow:${path}`) + + const result = JSON.parse( + await callGlobalTool('write_flow', { + path, + modules: JSON.stringify([ + { + id: 'empty_step', + value: { type: 'rawscript', language: 'bun', content: '', input_transforms: {} } + } + ]) + }) + ) + + expect(result.success).toBe(false) + expect(JSON.stringify(result)).not.toContain('set_flow_module_code') + }) + + it('hints at inline-code escaping when the modules JSON fails to parse', async () => { + await expect( + callGlobalTool('write_flow', { + path: 'f/flows/bad-json', + modules: '[{"id":"a","value":{"type":"rawscript","content":"oops"}]' + }) + ).rejects.toThrow(/Invalid JSON for modules.*set_flow_module_code/s) + }) + it('writes flows with flow-mode arguments and reads compact flow value', async () => { const writeResult = JSON.parse( await callGlobalTool('write_flow', { diff --git a/frontend/src/lib/components/copilot/chat/global/core.ts b/frontend/src/lib/components/copilot/chat/global/core.ts index b628cfbdb4..cafe2ea76f 100644 --- a/frontend/src/lib/components/copilot/chat/global/core.ts +++ b/frontend/src/lib/components/copilot/chat/global/core.ts @@ -469,11 +469,11 @@ function parseOptionalJsonArg(value: unknown, field: string): unknown { * Rawscript bodies are fragile to embed inside the `modules` JSON string: the * code's quotes and newlines have to survive three levels of escaping (tool-call * arguments -> modules string -> content string) and the model routinely mangles - * them. When a rawscript module is left with empty or placeholder content, steer - * the model to fill the body out-of-band with `set_flow_module_code` instead of - * re-sending the whole flow. + * them. So a module may be saved with empty (or `inline_script.` placeholder) + * content; return the ids that still need a body filled out-of-band with + * `set_flow_module_code`. */ -function formatEmptyInlineScriptWarning(editable: EditableFlowJson): string { +function emptyInlineScriptModuleIds(editable: EditableFlowJson): string[] { const value: FlowValue = { modules: editable.modules, preprocessor_module: editable.preprocessor_module ?? undefined, @@ -481,14 +481,34 @@ function formatEmptyInlineScriptWarning(editable: EditableFlowJson): string { } const session = createInlineScriptSession() buildEditableFlowJson({ value, schema: editable.schema }, session) - const emptyIds = Object.entries(session.getAll()) + return Object.entries(session.getAll()) .filter(([, content]) => content.trim() === '' || /^inline_script\./.test(content)) .map(([id]) => id) +} + +/** + * Fold the empty-body warning into `write_flow`'s JSON result — but only when the + * save actually succeeded. `writeFlowDraft` reports conflicts/persistence errors + * as `{ success: false }` rather than throwing; telling the model to fill code on + * a flow that was never saved would send it after a stale or nonexistent draft. + */ +function appendEmptyInlineScriptWarning(result: string, editable: EditableFlowJson): string { + const emptyIds = emptyInlineScriptModuleIds(editable) if (emptyIds.length === 0) { - return '' + return result + } + let parsed: { success?: unknown; message?: unknown } + try { + parsed = JSON.parse(result) + } catch { + return result + } + if (parsed.success !== true || typeof parsed.message !== 'string') { + return result } const list = emptyIds.map((id) => `"${id}"`).join(', ') - return `\n\nWarning: inline scripts ${list} have no code yet. Fill each one with set_flow_module_code(path, module_id, code) — do not re-send the whole flow.` + parsed.message += `\n\nWarning: inline scripts ${list} have no code yet. Fill each one with set_flow_module_code(path, module_id, code) — do not re-send the whole flow.` + return JSON.stringify(parsed, null, 2) } function editableFlowToDraftValue(editable: EditableFlowJson): FlowDraftValue { @@ -2827,7 +2847,7 @@ export const globalTools: Tool<{}>[] = [ groups: parseOptionalJsonArg(parsed.groups, 'groups'), notes: parseOptionalJsonArg(parsed.notes, 'notes') }) - const message = await writeFlowDraft( + const result = await writeFlowDraft( { path: parsed.path, summary: parsed.summary, @@ -2836,7 +2856,7 @@ export const globalTools: Tool<{}>[] = [ }, ctx ) - return message + formatEmptyInlineScriptWarning(editable) + return appendEmptyInlineScriptWarning(result, editable) } }, {