From c9551398f47d16c0a9a09aa8de00abc889b0ea79 Mon Sep 17 00:00:00 2001 From: Thiago Alves Date: Thu, 28 May 2026 15:02:50 -0400 Subject: [PATCH] fix(pipeline): plumb xml2st extra-args array through the shared port contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit User-reported bug on openplc-web staging: declaring a STRUCT data type broke the build with `Undefined type 'MY_QUEUE'` out of strucpp. The same project compiled fine on the desktop editor. Root cause: xml2st flag drift. Editor's `handleTranspileXMLtoST` hardcoded `--keep-structs`; the web's compile-service `/generate-st` endpoint hardcoded the unflagged invocation, so structs got rewritten to FUNCTION_BLOCKs and strucpp rejected them. Fix: the shared port contract carries an array of raw xml2st CLI tokens (`TranspileXmlToStArgs.xml2stArgs: readonly string[]`) — not a typed boolean per flag — so future xml2st options are a one-line append in `runCompilePipeline` with no per-platform changes needed. The shared pipeline sets `['--keep-structs']` for strucpp targets. - Editor adapter passes the array through verbatim to the local xml2st binary (trusted) via `handleTranspileXMLtoST`'s new third positional arg. - Web adapter (companion PR on openplc-web) filters the array against its `KNOWN_XML2ST_FLAGS` allowlist, dropping + warning on anything unknown before sending to the service. - compiler-service (separate PR on its feat/strucpp-support branch) accepts `xml2st_args: list[str]` and validates against an identical server-side allowlist (defence in depth). Internal editor callers (`compileForDebug`, `compileLibrary`) now pass `['--keep-structs']` explicitly — same effective behaviour as the old hardcode. Regression guards: - `pipeline.test.ts`: asserts `port.transpileXmlToSt` receives `xml2stArgs: ['--keep-structs']`. Future refactors that drop the flag from the pipeline break here. - `editor-compiler-platform-port.test.ts`: two new cases verify the editor adapter forwards both a populated array and an empty array verbatim to `handleTranspileXMLtoST` (no silent defaults). Co-Authored-By: Claude Opus 4.7 (1M context) --- .../editor-compiler-platform-port.test.ts | 48 +++++++++++++++++++ .../editor/compiler/compiler-module.ts | 46 +++++++++++------- .../compiler/editor-compiler-platform-port.ts | 12 +++-- .../shared/compile/__tests__/pipeline.test.ts | 9 ++++ src/backend/shared/compile/pipeline.ts | 14 +++++- .../shared/ports/compiler-platform-port.ts | 24 +++++++++- 6 files changed, 130 insertions(+), 23 deletions(-) diff --git a/src/backend/editor/compiler/__tests__/editor-compiler-platform-port.test.ts b/src/backend/editor/compiler/__tests__/editor-compiler-platform-port.test.ts index ebeb776c4..5d90a0511 100644 --- a/src/backend/editor/compiler/__tests__/editor-compiler-platform-port.test.ts +++ b/src/backend/editor/compiler/__tests__/editor-compiler-platform-port.test.ts @@ -210,6 +210,54 @@ describe('createEditorCompilerPlatformPort', () => { expect(log).toHaveBeenCalledWith(expect.stringContaining('lib install failed'), 'error') }) + // ---- transpileXmlToSt — xml2stArgs forwarding (STRUCT drift regression) ---- + + it('transpileXmlToSt forwards args.xml2stArgs to handleTranspileXMLtoST verbatim', async () => { + // Regression guard for the editor/web STRUCT drift bug: the + // shared pipeline owns the xml2st flag set as an array of CLI + // tokens, and the editor adapter must thread that array into + // handleTranspileXMLtoST as the third positional arg — the + // handler then splices it straight into the spawned xml2st argv. + // Editor's local xml2st is trusted, so the adapter passes the + // array through verbatim (no filtering). + const handleTranspileXMLtoST = jest.fn(async () => undefined) + const tmp = mkdtempSync(join(tmpdir(), 'xml2st-args-')) + try { + const port = createEditorCompilerPlatformPort( + makeHandlers({ handleTranspileXMLtoST }), + makeContext({ sourceTargetFolderPath: tmp }), + ) + // The handler stub never produces a program.st, so the readFile + // after the spawn-equivalent step throws — that's fine, we only + // care about the xml2stArgs argument forwarded to the handler. + await port.transpileXmlToSt({ xml: '', xml2stArgs: ['--keep-structs'] }, () => undefined) + expect(handleTranspileXMLtoST).toHaveBeenCalledTimes(1) + const callArgs = handleTranspileXMLtoST.mock.calls[0]! + expect(callArgs[2]).toEqual(['--keep-structs']) + } finally { + rmSync(tmp, { recursive: true, force: true }) + } + }) + + it('transpileXmlToSt forwards an empty xml2stArgs array verbatim', async () => { + // The adapter must not "helpfully" inject defaults when the + // pipeline asked for nothing — that would be the exact kind of + // silent drift the shared port contract exists to prevent. + const handleTranspileXMLtoST = jest.fn(async () => undefined) + const tmp = mkdtempSync(join(tmpdir(), 'xml2st-empty-args-')) + try { + const port = createEditorCompilerPlatformPort( + makeHandlers({ handleTranspileXMLtoST }), + makeContext({ sourceTargetFolderPath: tmp }), + ) + await port.transpileXmlToSt({ xml: '', xml2stArgs: [] }, () => undefined) + const callArgs = handleTranspileXMLtoST.mock.calls[0]! + expect(callArgs[2]).toEqual([]) + } finally { + rmSync(tmp, { recursive: true, force: true }) + } + }) + // ---- uploadArduinoBoard — port wiring (regression for issue #5) ---- it('uploadArduinoBoard forwards args.port to the handler as communicationPort', async () => { diff --git a/src/backend/editor/compiler/compiler-module.ts b/src/backend/editor/compiler/compiler-module.ts index 4155f5f92..186917ffd 100644 --- a/src/backend/editor/compiler/compiler-module.ts +++ b/src/backend/editor/compiler/compiler-module.ts @@ -720,15 +720,19 @@ class CompilerModule { async handleTranspileXMLtoST( generatedXMLFilePath: string, handleOutputData: (chunk: Buffer | string, logLevel?: 'info' | 'error') => void, + extraXml2stArgs: readonly string[], ) { return new Promise>((resolve, reject) => { - // `--keep-structs` tells xml2st to emit user-defined STRUCT data - // types as native `TYPE name : STRUCT … END_STRUCT;` declarations - // instead of rewriting them as FUNCTION_BLOCKs (matiec's legacy - // workaround). Strucpp parses STRUCT natively and rejects the FB - // rewrite as a type-vs-instance mismatch — every program build in - // the editor targets strucpp now, so we always set the flag. - const executeCommand = this.#executeXml2st(['--generate-st', generatedXMLFilePath, '--keep-structs']) + // `extraXml2stArgs` comes from the shared pipeline's + // `TranspileXmlToStArgs.xml2stArgs` — the single source of truth + // for xml2st flag semantics across editor and web. Editor passes + // them through verbatim (trusted local binary); web's adapter + // filters against its known-args allowlist before sending to the + // compile-service. Strucpp targets currently pass + // `['--keep-structs']` (native STRUCT declarations vs matiec's + // legacy struct→FB rewrite); future flags appear here as the + // pipeline opts into them. + const executeCommand = this.#executeXml2st(['--generate-st', generatedXMLFilePath, ...extraXml2stArgs]) let stderrData = '' @@ -2225,9 +2229,13 @@ class CompilerModule { const generatedXMLFilePath = join(sourceTargetFolderPath, 'plc.xml') try { - await this.handleTranspileXMLtoST(generatedXMLFilePath, (data, logLevel) => { - _mainProcessPort.postMessage({ logLevel, message: data }) - }) + await this.handleTranspileXMLtoST( + generatedXMLFilePath, + (data, logLevel) => { + _mainProcessPort.postMessage({ logLevel, message: data }) + }, + ['--keep-structs'], + ) } catch (error) { _mainProcessPort.postMessage({ logLevel: 'error', @@ -2471,13 +2479,17 @@ class CompilerModule { // Stage 2: xml2st spawn (shared with the program-build path). try { - await this.handleTranspileXMLtoST(xmlPath, (data, logLevel) => { - // xml2st's stdout doubles as progress + error stream; surface - // it verbatim so the user sees the same diagnostics the - // program-build path produces. - const message = typeof data === 'string' ? data : data.toString() - post(message, logLevel ?? 'info') - }) + await this.handleTranspileXMLtoST( + xmlPath, + (data, logLevel) => { + // xml2st's stdout doubles as progress + error stream; surface + // it verbatim so the user sees the same diagnostics the + // program-build path produces. + const message = typeof data === 'string' ? data : data.toString() + post(message, logLevel ?? 'info') + }, + ['--keep-structs'], + ) } catch (error) { bail(`xml2st failed: ${getErrorMessage(error)}`) return diff --git a/src/backend/editor/compiler/editor-compiler-platform-port.ts b/src/backend/editor/compiler/editor-compiler-platform-port.ts index 0c5a88369..b469d2e52 100644 --- a/src/backend/editor/compiler/editor-compiler-platform-port.ts +++ b/src/backend/editor/compiler/editor-compiler-platform-port.ts @@ -164,10 +164,14 @@ export function createEditorCompilerPlatformPort( await fs.mkdir(dirname(xmlPath), { recursive: true }) await fs.writeFile(xmlPath, args.xml, 'utf-8') - await handlers.handleTranspileXMLtoST(xmlPath, (chunk, level) => { - const message = typeof chunk === 'string' ? chunk : chunk.toString() - log(message, level ?? 'info') - }) + await handlers.handleTranspileXMLtoST( + xmlPath, + (chunk, level) => { + const message = typeof chunk === 'string' ? chunk : chunk.toString() + log(message, level ?? 'info') + }, + args.xml2stArgs, + ) const programStPath = join(context.sourceTargetFolderPath, 'program.st') const programSt = await fs.readFile(programStPath, 'utf-8') diff --git a/src/backend/shared/compile/__tests__/pipeline.test.ts b/src/backend/shared/compile/__tests__/pipeline.test.ts index efda4f0eb..9ce0c2b4a 100644 --- a/src/backend/shared/compile/__tests__/pipeline.test.ts +++ b/src/backend/shared/compile/__tests__/pipeline.test.ts @@ -167,6 +167,15 @@ describe('runCompilePipeline — simulator path', () => { expect(result.binary).toBeInstanceOf(Uint8Array) expect(result.uploaded).toBe(false) expect(port.transpileXmlToSt).toHaveBeenCalledTimes(1) + // The pipeline owns xml2st flag semantics: every strucpp target + // gets `xml2stArgs: ['--keep-structs']`. Regression guard for + // the editor/web STRUCT drift bug — see compiler-platform-port.ts + // comment. Future flags get added to this array, not to a + // typed boolean on the port (the port stays format-agnostic). + expect(port.transpileXmlToSt).toHaveBeenCalledWith( + expect.objectContaining({ xml2stArgs: ['--keep-structs'] }), + expect.any(Function), + ) expect(port.compileArduino).toHaveBeenCalledTimes(1) expect(port.uploadRuntimeV4).not.toHaveBeenCalled() expect(port.uploadArduinoBoard).not.toHaveBeenCalled() diff --git a/src/backend/shared/compile/pipeline.ts b/src/backend/shared/compile/pipeline.ts index e5ada7289..1c2642443 100644 --- a/src/backend/shared/compile/pipeline.ts +++ b/src/backend/shared/compile/pipeline.ts @@ -326,7 +326,19 @@ async function runCompilePipelineInner( // on editor, HTTP /generate-st on web). // --------------------------------------------------------------------- emit({ stage: 'st', message: 'Generating Structured Text...', level: 'info' }) - const stResult = await port.transpileXmlToSt({ xml: plcXml }, makePlatformLog(emit, 'st')) + // `['--keep-structs']` — strucpp parses native `STRUCT` declarations + // and rejects matiec's legacy struct→FB rewrite as a type-vs-instance + // mismatch. Editor's local xml2st always passed `--keep-structs`; + // pre-pipeline this was hardcoded inside its compiler-module while + // the web's compile-service `/generate-st` endpoint ran without it, + // causing structs to compile on the desktop and fail on the web. + // The flag set is now part of the port contract so both adapters + // observe the same tokens — future xml2st flags get appended here + // at this single call site. + const stResult = await port.transpileXmlToSt( + { xml: plcXml, xml2stArgs: ['--keep-structs'] }, + makePlatformLog(emit, 'st'), + ) if (!stResult.ok || !stResult.programSt) { if (stResult.errors && stResult.errors.length > 0) { emitCompileErrorEvents( diff --git a/src/middleware/shared/ports/compiler-platform-port.ts b/src/middleware/shared/ports/compiler-platform-port.ts index d5f4d6fdf..382996f72 100644 --- a/src/middleware/shared/ports/compiler-platform-port.ts +++ b/src/middleware/shared/ports/compiler-platform-port.ts @@ -81,9 +81,31 @@ export type PlatformDeviceContext = // --------------------------------------------------------------------------- /** `xml2st` input: a single XML string (the IEC 61131-3 PLC XML the - * shared `XmlGenerator` produces). Same input on both platforms. */ + * shared `XmlGenerator` produces) plus an array of extra CLI tokens + * to append to the xml2st invocation. Defined here (not on either + * adapter) because xml2st flag drift was the root cause of the + * initial cross-platform STRUCT bug — editor's local xml2st passed + * `--keep-structs` but the web's compile-service `/generate-st` + * endpoint hardcoded an unflagged invocation, so structs declared + * in the project compiled fine on the desktop and blew up on the + * web with `Undefined type 'MY_STRUCT'` errors out of strucpp. + * Pushing the flag set into a single shared field means any future + * xml2st option is a one-line pipeline change with no per-platform + * drift possible. + * + * Editor's adapter passes `xml2stArgs` verbatim to the local + * binary (trusted). Web's adapter filters against its own + * known-args allowlist before forwarding to the compile-service + * `/generate-st` endpoint, logging a warning for anything it + * doesn't recognise (defence in depth — service has its own + * allowlist too). */ export interface TranspileXmlToStArgs { xml: string + /** Extra CLI tokens to append after `--generate-st ` in the + * xml2st invocation. For strucpp targets the shared pipeline + * sets `['--keep-structs']`; future flags get added here at the + * single call site in `runCompilePipeline`. */ + xml2stArgs: readonly string[] } export interface TranspileXmlToStResult {