Skip to content

Commit 43a53c5

Browse files
Copilotintel352
Copilot
and
intel352
authored
Preserve unknown top-level YAML keys and fix metadata loss through import/export cycle (#8)
* Initial plan * Fix YAML reformatting: preserve unknown top-level keys, fix triggers injection, fix parseYamlSafe consistency Agent-Logs-Url: https://github.com/GoCodeAlone/workflow-editor/sessions/fdecd5e6-5e99-4497-be49-ba1ed4033a96 Co-authored-by: intel352 <77607+intel352@users.noreply.github.com> * Address code review: explicit extraTopLevelKeys null check, explicit default key ordering, simplified passthrough extraction Agent-Logs-Url: https://github.com/GoCodeAlone/workflow-editor/sessions/fdecd5e6-5e99-4497-be49-ba1ed4033a96 Co-authored-by: intel352 <77607+intel352@users.noreply.github.com> * Address code review: move KNOWN_TOP_LEVEL_KEYS before configToYaml, use clearer destructuring names Agent-Logs-Url: https://github.com/GoCodeAlone/workflow-editor/sessions/fdecd5e6-5e99-4497-be49-ba1ed4033a96 Co-authored-by: intel352 <77607+intel352@users.noreply.github.com> * Fix review feedback: clearCanvas resets importedPassthrough, exclude pipelines from passthrough type, fix buildMainFileContent for multi-file key ordering and unknown keys Agent-Logs-Url: https://github.com/GoCodeAlone/workflow-editor/sessions/cbe50c21-b5cc-4238-ab7b-36cb53bc6e40 Co-authored-by: intel352 <77607+intel352@users.noreply.github.com> * Refactor nested ternary in buildMainFileContent to if-else for clarity Agent-Logs-Url: https://github.com/GoCodeAlone/workflow-editor/sessions/cbe50c21-b5cc-4238-ab7b-36cb53bc6e40 Co-authored-by: intel352 <77607+intel352@users.noreply.github.com> * Style: use adjustedOriginalKeys and conditional spread for imports in buildMainFileContent Agent-Logs-Url: https://github.com/GoCodeAlone/workflow-editor/sessions/cbe50c21-b5cc-4238-ab7b-36cb53bc6e40 Co-authored-by: intel352 <77607+intel352@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
1 parent 666ea8c commit 43a53c5

File tree

4 files changed

+318
-37
lines changed

4 files changed

+318
-37
lines changed

‎src/stores/workflowStore.ts‎

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,15 @@ interface WorkflowStore {
105105
importedTriggers: Record<string, unknown>;
106106
importedPipelines: Record<string, unknown>;
107107

108+
/**
109+
* Passthrough metadata from the most recently imported WorkflowConfig.
110+
* Stores non-visual top-level fields (name, version, _originalKeys, _extraTopLevelKeys, imports,
111+
* requires, platform, infrastructure, sidecars) so that exportToConfig() can reconstruct
112+
* a complete WorkflowConfig without losing unknown keys or reformatting structure.
113+
* Pipelines are excluded here because they are tracked separately via importedPipelines.
114+
*/
115+
importedPassthrough: Omit<WorkflowConfig, 'modules' | 'workflows' | 'triggers' | 'pipelines'> | null;
116+
108117
// Multi-file resolution: maps module name → source file path
109118
sourceMap: Map<string, string>;
110119
setSourceMap: (sourceMap: Map<string, string>) => void;
@@ -218,6 +227,7 @@ const useWorkflowStore = create<WorkflowStore>()(
218227
importedWorkflows: {},
219228
importedTriggers: {},
220229
importedPipelines: {},
230+
importedPassthrough: null,
221231

222232
// Multi-file resolution
223233
sourceMap: new Map<string, string>(),
@@ -453,9 +463,15 @@ const useWorkflowStore = create<WorkflowStore>()(
453463
},
454464

455465
exportToConfig: () => {
456-
const { nodes, edges, importedWorkflows, importedTriggers, importedPipelines } = get();
466+
const { nodes, edges, importedWorkflows, importedTriggers, importedPipelines, importedPassthrough } = get();
457467
const moduleTypeMap = useModuleSchemaStore.getState().moduleTypeMap;
458-
const config = nodesToConfig(nodes, edges, moduleTypeMap);
468+
// Pass importedPassthrough as originalConfig so that nodesToConfig can restore
469+
// non-visual fields: name, version, _originalKeys, _extraTopLevelKeys, imports,
470+
// requires, platform, infrastructure, sidecars.
471+
const originalConfig: import('../types/workflow.ts').WorkflowConfig | undefined = importedPassthrough
472+
? { modules: [], workflows: {}, triggers: {}, ...importedPassthrough }
473+
: undefined;
474+
const config = nodesToConfig(nodes, edges, moduleTypeMap, originalConfig);
459475
if (Object.keys(config.workflows).length === 0 && Object.keys(importedWorkflows).length > 0) {
460476
config.workflows = importedWorkflows;
461477
}
@@ -484,13 +500,17 @@ const useWorkflowStore = create<WorkflowStore>()(
484500
get().pushHistory();
485501
const moduleTypeMap = useModuleSchemaStore.getState().moduleTypeMap;
486502
const { nodes, edges } = configToNodes(config, moduleTypeMap, sourceMap);
503+
// Extract passthrough metadata (everything except the visual module/workflow/trigger data)
504+
const { modules: _modules, workflows: _workflows, triggers: _triggers, pipelines: _pipelines, ...passthroughFields } = config;
505+
const importedPassthrough: WorkflowStore['importedPassthrough'] = passthroughFields;
487506
const updates: Partial<WorkflowStore> = {
488507
nodes,
489508
edges,
490509
selectedNodeId: null,
491510
importedWorkflows: config.workflows ?? {},
492511
importedTriggers: config.triggers ?? {},
493512
importedPipelines: config.pipelines ?? {},
513+
importedPassthrough,
494514
};
495515
if (sourceMap) {
496516
updates.sourceMap = sourceMap;
@@ -501,7 +521,7 @@ const useWorkflowStore = create<WorkflowStore>()(
501521

502522
clearCanvas: () => {
503523
get().pushHistory();
504-
set({ nodes: [], edges: [], selectedNodeId: null, selectedEdgeId: null, nodeCounter: 0, importedWorkflows: {}, importedTriggers: {}, importedPipelines: {} });
524+
set({ nodes: [], edges: [], selectedNodeId: null, selectedEdgeId: null, nodeCounter: 0, importedWorkflows: {}, importedTriggers: {}, importedPipelines: {}, importedPassthrough: null });
505525
},
506526

507527
exportLayout: (): LayoutData => {

‎src/types/workflow.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,8 @@ export interface WorkflowConfig {
2323
infrastructure?: Record<string, unknown>;
2424
sidecars?: unknown[];
2525
_originalKeys?: string[];
26+
/** Preserves unknown top-level keys that are not part of the known schema (e.g. engine:, custom config blocks). */
27+
_extraTopLevelKeys?: Record<string, unknown>;
2628
}
2729

2830
// Workflow section types for edge extraction

‎src/utils/serialization-bugs.test.ts‎

Lines changed: 159 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { describe, it, expect } from 'vitest';
2-
import { configToNodes, nodesToConfig, configToYaml, parseYamlSafe } from './serialization.ts';
2+
import { configToNodes, nodesToConfig, configToYaml, parseYamlSafe, parseYaml } from './serialization.ts';
33
import { MODULE_TYPE_MAP } from '../types/workflow.ts';
44

55
describe('Bug 1: name and version round-trip', () => {
@@ -136,3 +136,161 @@ triggers: {}
136136
expect(isPartial).toBe(false);
137137
});
138138
});
139+
140+
describe('Bug 4: unknown top-level keys (e.g. engine:) must not be dropped', () => {
141+
it('parseYaml captures unknown top-level keys in _extraTopLevelKeys', () => {
142+
const yamlText = `
143+
name: my-service
144+
engine:
145+
validation:
146+
templateRefs: warn
147+
modules: []
148+
workflows: {}
149+
`;
150+
const config = parseYaml(yamlText);
151+
expect(config._extraTopLevelKeys).toBeDefined();
152+
expect(config._extraTopLevelKeys!['engine']).toEqual({ validation: { templateRefs: 'warn' } });
153+
});
154+
155+
it('parseYamlSafe captures unknown top-level keys in _extraTopLevelKeys', () => {
156+
const yamlText = `
157+
name: my-service
158+
engine:
159+
validation:
160+
templateRefs: warn
161+
modules: []
162+
`;
163+
const { config } = parseYamlSafe(yamlText);
164+
expect(config._extraTopLevelKeys).toBeDefined();
165+
expect(config._extraTopLevelKeys!['engine']).toEqual({ validation: { templateRefs: 'warn' } });
166+
});
167+
168+
it('configToYaml emits unknown top-level keys', () => {
169+
const yamlText = `
170+
name: my-service
171+
engine:
172+
validation:
173+
templateRefs: warn
174+
modules: []
175+
workflows: {}
176+
`;
177+
const config = parseYaml(yamlText);
178+
const out = configToYaml(config);
179+
expect(out).toContain('engine:');
180+
expect(out).toContain('templateRefs: warn');
181+
});
182+
183+
it('nodesToConfig passes through _extraTopLevelKeys from originalConfig', () => {
184+
const yamlText = `
185+
name: my-service
186+
engine:
187+
validation:
188+
templateRefs: warn
189+
modules:
190+
- name: server
191+
type: http.server
192+
config:
193+
address: ":8080"
194+
workflows: {}
195+
`;
196+
const config = parseYaml(yamlText);
197+
const { nodes, edges } = configToNodes(config, MODULE_TYPE_MAP);
198+
const exported = nodesToConfig(nodes, edges, MODULE_TYPE_MAP, config);
199+
expect(exported._extraTopLevelKeys).toBeDefined();
200+
expect(exported._extraTopLevelKeys!['engine']).toEqual({ validation: { templateRefs: 'warn' } });
201+
const out = configToYaml(exported);
202+
expect(out).toContain('engine:');
203+
});
204+
205+
it('full round-trip preserves engine block and original key ordering', () => {
206+
const yamlText = `name: my-service
207+
version: "1.0"
208+
engine:
209+
validation:
210+
templateRefs: warn
211+
modules:
212+
- name: server
213+
type: http.server
214+
config:
215+
address: ':8080'
216+
pipelines:
217+
health:
218+
trigger:
219+
type: http
220+
method: GET
221+
path: /healthz
222+
`;
223+
const config = parseYaml(yamlText);
224+
const { nodes, edges } = configToNodes(config, MODULE_TYPE_MAP);
225+
const exported = nodesToConfig(nodes, edges, MODULE_TYPE_MAP, config);
226+
const out = configToYaml(exported);
227+
228+
// engine block must be present
229+
expect(out).toContain('engine:');
230+
expect(out).toContain('templateRefs: warn');
231+
232+
// Key ordering: name comes before engine comes before modules
233+
const nameIdx = out.indexOf('name:');
234+
const engineIdx = out.indexOf('engine:');
235+
const modulesIdx = out.indexOf('modules:');
236+
expect(nameIdx).toBeGreaterThanOrEqual(0);
237+
expect(engineIdx).toBeGreaterThan(nameIdx);
238+
expect(modulesIdx).toBeGreaterThan(engineIdx);
239+
});
240+
});
241+
242+
describe('Bug 5: triggers: {} must not be injected when not in original', () => {
243+
it('does not add triggers: {} when original YAML had no triggers key', () => {
244+
const yamlText = `
245+
name: my-service
246+
modules:
247+
- name: server
248+
type: http.server
249+
config:
250+
address: ":8080"
251+
pipelines:
252+
health:
253+
trigger:
254+
type: http
255+
method: GET
256+
path: /healthz
257+
`;
258+
const config = parseYaml(yamlText);
259+
// triggers not in original keys
260+
expect(config._originalKeys).not.toContain('triggers');
261+
262+
const { nodes, edges } = configToNodes(config, MODULE_TYPE_MAP);
263+
const exported = nodesToConfig(nodes, edges, MODULE_TYPE_MAP, config);
264+
const out = configToYaml(exported);
265+
266+
// triggers: {} must NOT appear in output
267+
expect(out).not.toMatch(/^triggers:/m);
268+
});
269+
});
270+
271+
describe('Bug 6: parseYamlSafe is consistent with parseYaml for all fields', () => {
272+
it('parseYamlSafe preserves imports, requires, platform, infrastructure, sidecars', () => {
273+
const yamlText = `
274+
name: my-service
275+
imports:
276+
- other.yaml
277+
requires:
278+
some-service: ">=1.0"
279+
platform:
280+
target: kubernetes
281+
infrastructure:
282+
database:
283+
type: postgres
284+
sidecars:
285+
- name: proxy
286+
image: envoy:latest
287+
modules: []
288+
`;
289+
const { config } = parseYamlSafe(yamlText);
290+
expect(config.imports).toEqual(['other.yaml']);
291+
expect(config.requires).toEqual({ 'some-service': '>=1.0' });
292+
expect(config.platform).toEqual({ target: 'kubernetes' });
293+
expect(config.infrastructure).toEqual({ database: { type: 'postgres' } });
294+
expect(config.sidecars).toHaveLength(1);
295+
});
296+
});

0 commit comments

Comments
 (0)