mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-08-20 00:02:19 +00:00
fix(copilot): only warn on saved write_flow; add regression tests
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) <noreply@anthropic.com>
This commit is contained in:
@@ -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', {
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
},
|
||||
{
|
||||
|
||||
Reference in New Issue
Block a user