From 35abc8cb522e42117183a3eb5cf28a18531e6012 Mon Sep 17 00:00:00 2001 From: Karstein Phobic Nyvold Kvistad Date: Sun, 26 Apr 2026 18:41:48 +0200 Subject: [PATCH] fix(set_pou_code): omitted declaration/impl no longer wipes the POU Bug: calling set_pou_code with implementationCode only (declarationCode omitted) would wipe the POU's PROGRAM/VAR...END_VAR block in the binary. After such a call mirror_export classified the POU as 'UNKNOWN' (no PROGRAM/FUNCTION_BLOCK keyword in the empty declaration), and the var block disappeared from the .st mirror file. Root cause: the TS wrapper substituted '' (empty string) into the Python template when declarationCode was undefined, giving DECLARATION_CONTENT = "". The Python script then took the truthy-ish branch (empty string is not None) and called decl_obj.replace('') -- wiping textual_decl. Fix: pass explicit SET_DECLARATION / SET_IMPLEMENTATION boolean flags from the TS wrapper, gate the replace() calls on those flags. Empty string remains a valid intentional value (caller wants to wipe). - Reproduced on MCPTest2: PLC_PRG declaration block was wiped between v1.3.0.0 and v1.3.1.0 by exactly this code path. - Regression test added in tests/integration/e2e.test.ts covering the omitted-declarationCode path. - Existing set_pou_code test extended to assert SET_DECLARATION / SET_IMPLEMENTATION are emitted in the rendered script. --- src/scripts/set_pou_code.py | 16 +++++++++------- src/server.ts | 7 +++++++ tests/integration/e2e.test.ts | 29 +++++++++++++++++++++++++++++ 3 files changed, 45 insertions(+), 7 deletions(-) diff --git a/src/scripts/set_pou_code.py b/src/scripts/set_pou_code.py index ddeda46..d804d42 100644 --- a/src/scripts/set_pou_code.py +++ b/src/scripts/set_pou_code.py @@ -3,6 +3,11 @@ import sys, scriptengine as script_engine, os, traceback POU_FULL_PATH = "{POU_FULL_PATH}" # Expecting format like "Application/MyPOU" or "Folder/SubFolder/MyPOU" DECLARATION_CONTENT = """{DECLARATION_CONTENT}""" IMPLEMENTATION_CONTENT = """{IMPLEMENTATION_CONTENT}""" +# Boolean flags from the TS wrapper. "True" if the caller passed the field, +# "False" if they omitted it. Empty string is a valid intentional value +# (e.g. "wipe declaration") and must not be conflated with "not provided". +SET_DECLARATION = {SET_DECLARATION} +SET_IMPLEMENTATION = {SET_IMPLEMENTATION} try: print("DEBUG: set_pou_code script: POU_FULL_PATH='%s', Project='%s'" % (POU_FULL_PATH, PROJECT_FILE_PATH)) @@ -18,9 +23,7 @@ try: # --- Set Declaration Part --- declaration_updated = False - # Check if the content is actually provided (might be None/empty if only impl is set) - has_declaration_content = 'DECLARATION_CONTENT' in locals() or 'DECLARATION_CONTENT' in globals() - if has_declaration_content and DECLARATION_CONTENT is not None: # Check not None + if SET_DECLARATION: if hasattr(target_object, 'textual_declaration'): decl_obj = target_object.textual_declaration if decl_obj and hasattr(decl_obj, 'replace'): @@ -37,13 +40,12 @@ try: else: print("WARN: Target '%s' does not have textual_declaration attribute. Skipping declaration update." % target_name) else: - print("DEBUG: Declaration content not provided or is None. Skipping declaration update.") + print("DEBUG: Declaration not provided by caller (SET_DECLARATION=False). Skipping declaration update.") # --- Set Implementation Part --- implementation_updated = False - has_implementation_content = 'IMPLEMENTATION_CONTENT' in locals() or 'IMPLEMENTATION_CONTENT' in globals() - if has_implementation_content and IMPLEMENTATION_CONTENT is not None: # Check not None + if SET_IMPLEMENTATION: if hasattr(target_object, 'textual_implementation'): impl_obj = target_object.textual_implementation if impl_obj and hasattr(impl_obj, 'replace'): @@ -60,7 +62,7 @@ try: else: print("WARN: Target '%s' does not have textual_implementation attribute. Skipping implementation update." % target_name) else: - print("DEBUG: Implementation content not provided or is None. Skipping implementation update.") + print("DEBUG: Implementation not provided by caller (SET_IMPLEMENTATION=False). Skipping implementation update.") # --- SAVE THE PROJECT TO PERSIST THE CODE CHANGE --- diff --git a/src/server.ts b/src/server.ts index f28eb5a..f10bab8 100644 --- a/src/server.ts +++ b/src/server.ts @@ -843,6 +843,11 @@ export async function startMcpServer(config: ServerConfig): Promise { // Escape for triple-quoted Python strings const sanDecl = (args.declarationCode ?? '').replace(/\\/g, '\\\\').replace(/"""/g, '\\"\\"\\"'); const sanImpl = (args.implementationCode ?? '').replace(/\\/g, '\\\\').replace(/"""/g, '\\"\\"\\"'); + // Distinguish "argument provided" from "argument is empty string". An + // omitted declaration must NOT reach decl_obj.replace('') -- doing so + // wipes the POU's PROGRAM/VAR...END_VAR block, leaving an UNKNOWN POU. + const setDecl = args.declarationCode !== undefined ? 'True' : 'False'; + const setImpl = args.implementationCode !== undefined ? 'True' : 'False'; const script = scriptManager.prepareScriptWithHelpers( 'set_pou_code', { @@ -850,6 +855,8 @@ export async function startMcpServer(config: ServerConfig): Promise { POU_FULL_PATH: sanPouPath, DECLARATION_CONTENT: sanDecl, IMPLEMENTATION_CONTENT: sanImpl, + SET_DECLARATION: setDecl, + SET_IMPLEMENTATION: setImpl, }, ['ensure_project_open', 'find_object_by_path'] ); diff --git a/tests/integration/e2e.test.ts b/tests/integration/e2e.test.ts index f22c960..6ec224d 100644 --- a/tests/integration/e2e.test.ts +++ b/tests/integration/e2e.test.ts @@ -58,11 +58,40 @@ describe('E2E Script Preparation', () => { POU_FULL_PATH: 'Application/MyPOU', DECLARATION_CONTENT: sanDecl, IMPLEMENTATION_CONTENT: sanImpl, + SET_DECLARATION: 'True', + SET_IMPLEMENTATION: 'True', }, ['ensure_project_open', 'find_object_by_path'] ); expect(script).toContain('Application/MyPOU'); expect(script).toContain('x := 42;'); + expect(script).toContain('SET_DECLARATION = True'); + expect(script).toContain('SET_IMPLEMENTATION = True'); + }); + + it('set_pou_code with omitted declarationCode gates the replace() call', () => { + // Regression: when caller omits declarationCode, the script must NOT + // call decl_obj.replace('') -- doing so wipes the POU's + // PROGRAM/VAR...END_VAR block (binary becomes UNKNOWN POU). + // server.ts passes SET_DECLARATION='False' in that case. + const script = mgr.prepareScriptWithHelpers( + 'set_pou_code', + { + PROJECT_FILE_PATH: 'C:\\test.project', + POU_FULL_PATH: 'Application/PLC_PRG', + DECLARATION_CONTENT: '', + IMPLEMENTATION_CONTENT: 'x := 1;', + SET_DECLARATION: 'False', + SET_IMPLEMENTATION: 'True', + }, + ['ensure_project_open', 'find_object_by_path'] + ); + expect(script).toContain('SET_DECLARATION = False'); + expect(script).toContain('SET_IMPLEMENTATION = True'); + // The skip branch must be reachable + expect(script).toContain('SET_DECLARATION=False'); + // No leftover {PLACEHOLDER} unsubstituted + expect(script).not.toMatch(/\{[A-Z_]+\}/); }); it('check_status script has no placeholders after load', () => {