From 32e612000d3c3f00812c8b54153cc137117fe574 Mon Sep 17 00:00:00 2001 From: Karstein Kvistad Date: Sun, 26 Apr 2026 17:16:44 +0200 Subject: [PATCH] fix(create_folder): try project.create_folder(name, SV_POU) first; drop ScriptManager cache create_folder v2 (positional foldername) returned None silently against the SP22 Application object -- no exception raised, no folder created. Investigation showed: - ScriptObject.create_folder(foldername) is documented to "create a folder in the structured view of the parent node", but on Application specifically it's a silent no-op (the structured view isn't pinned to POU view there). - ScriptProject.create_folder(foldername, structured_view=None) on the project itself with explicit SV_POU GUID ({21AF5390-2942-461a-BF89-951AAF6999F1}) is the documented and reliable pathway -- the resulting folder appears under Application in the IDE tree because that's where SV_POU lives. v3 fix: try strategies in order until one returns non-None: (1) primary_project.create_folder(name, SV_POU_GUID) -- new, primary (2) parent.create_folder(name) positional -- pre-SP21 path (3) parent.create_folder(foldername=name) -- alt keyword (4) primary_project.create_folder(name) -- default view (5) parent.create_object(typeUuid='85d1215e-...') -- alt factory (6) parent.add(script_engine.types.IecFolder, name=name) -- legacy Each strategy guards on hasattr + return-value-not-None, so a silent no-op falls through instead of being mistaken for success. ScriptManager: dropped the in-memory template cache. Each loadTemplate call now reads the .py from disk fresh. Cost: ~1ms per call vs ~1.5s of CODESYS execution time -- invisible. Win: edits to dist/scripts/ take effect without an MCP restart, which makes iterating on script- side fixes (like this very create_folder loop) much faster. Existing "cache hit" unit test rewritten as "two loads return equal content". tests/test-fixes.mjs: standalone harness that drives a single persistent CODESYS through HeadlessExecutor + CodesysLauncher to verify the four broken-tool fixes end-to-end. Useful for regression-testing without needing a vsc reboot loop. Currently only smoke-tests create_folder + compile + cross-project; expand as more fixes need verification. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/script-manager.ts | 20 ++-- src/scripts/create_folder.py | 87 ++++++++++++++---- tests/test-fixes.mjs | 146 ++++++++++++++++++++++++++++++ tests/unit/script-manager.test.ts | 4 +- 4 files changed, 224 insertions(+), 33 deletions(-) create mode 100644 tests/test-fixes.mjs diff --git a/src/script-manager.ts b/src/script-manager.ts index 13303f9..8a87487 100644 --- a/src/script-manager.ts +++ b/src/script-manager.ts @@ -1,6 +1,11 @@ /** * Python script template loading and interpolation. - * Loads .py templates from src/scripts/, caches them, and performs {PARAM} replacement. + * Loads .py templates from src/scripts/ (or dist/scripts/) and performs + * {PARAM} replacement. No caching: a tool call is ~1.5 s of CODESYS time, + * so the few-ms cost of re-reading a small .py file each call is invisible + * AND it means edits to dist/scripts/ are picked up live without an MCP + * restart. This makes iterating on script-side fixes much faster + * (relevant for the SP21+ scripting-engine drift bugs we hit on this fork). */ import * as fs from 'fs'; @@ -9,28 +14,19 @@ import { ScriptParams } from './types'; export class ScriptManager { private scriptsDir: string; - private cache: Map = new Map(); constructor(scriptsDir?: string) { this.scriptsDir = scriptsDir ?? path.join(__dirname, 'scripts'); } - /** Synchronously load a template file and cache it */ + /** Synchronously read a template file. Re-reads on every call -- no cache. */ loadTemplate(name: string): string { const fileName = name.endsWith('.py') ? name : `${name}.py`; - const cached = this.cache.get(fileName); - if (cached !== undefined) { - return cached; - } - const filePath = path.join(this.scriptsDir, fileName); if (!fs.existsSync(filePath)) { throw new Error(`Script template not found: ${filePath}`); } - - const content = fs.readFileSync(filePath, 'utf-8'); - this.cache.set(fileName, content); - return content; + return fs.readFileSync(filePath, 'utf-8'); } /** diff --git a/src/scripts/create_folder.py b/src/scripts/create_folder.py index b5792b1..41ccefc 100644 --- a/src/scripts/create_folder.py +++ b/src/scripts/create_folder.py @@ -46,38 +46,84 @@ try: parent_name = getattr(parent_object, 'get_name', lambda: str(parent_object))() print("DEBUG: Using parent object: %s" % parent_name) - # Create the folder. The keyword changed between docs and stubs: - # Per the SP22 stub Stubs/scriptengine/ScriptObject.pyi, the signature is - # def create_folder(self, foldername): ... - # NOT `name=...` (which the original fork code used and got - # "create_folder() got an unexpected keyword argument 'name'" against - # SP22). Use the positional form first to be agnostic to the keyword - # name across SP releases. Fall through to alternate factories if - # create_folder is unavailable on the parent at all. + # Create the folder. Per the SP22 stubs: + # ScriptObject.create_folder(foldername) -- on POUs / sub-objects + # ScriptProject.create_folder(foldername, structured_view=None) + # -- on the project itself, + # accepts an explicit view GUID + # On SP22 specifically, calling .create_folder('X') on an Application object + # returns None silently (no exception, no folder created). The reliable + # pathway is to call create_folder on the PROJECT with an explicit + # structured_view GUID -- the SV_POU view is where Application's children + # live, so a folder there will appear under Application in the IDE tree. + # + # SV_POU GUID = {21AF5390-2942-461a-BF89-951AAF6999F1}. (Documented in the + # ScriptProject.pyi stub; constant since SP3.5.2.0.) + SV_POU_GUID_STR = '21AF5390-2942-461a-BF89-951AAF6999F1' + sv_pou_guid = None + try: + from System import Guid + sv_pou_guid = Guid(SV_POU_GUID_STR) + except Exception as guid_e: + print("WARN: Could not construct System.Guid for SV_POU: %s" % guid_e) + new_folder = None - if hasattr(parent_object, 'create_folder'): + + # Strategy 1: project-level create_folder with explicit POU view. This is + # the only call shape that reliably works on SP22 Application children. + if hasattr(primary_project, 'create_folder') and sv_pou_guid is not None: try: - print("DEBUG: Calling parent.create_folder('%s') [positional]" % FOLDER_NAME) - new_folder = parent_object.create_folder(FOLDER_NAME) + print("DEBUG: Trying primary_project.create_folder('%s', SV_POU)" % FOLDER_NAME) + new_folder = primary_project.create_folder(FOLDER_NAME, sv_pou_guid) + if new_folder is not None: + print("DEBUG: project.create_folder(SV_POU) succeeded.") except Exception as e: - print("WARN: parent.create_folder('%s') raised: %s -- trying foldername= kwarg." % (FOLDER_NAME, e)) + print("WARN: primary_project.create_folder('%s', SV_POU) raised: %s" % (FOLDER_NAME, e)) + new_folder = None + + # Strategy 2: parent.create_folder (positional). Works pre-SP21 and on + # parents whose factories haven't been pinned to project-level. + if new_folder is None and hasattr(parent_object, 'create_folder'): + try: + print("DEBUG: Trying parent.create_folder('%s') [positional]" % FOLDER_NAME) + new_folder = parent_object.create_folder(FOLDER_NAME) + if new_folder is not None: + print("DEBUG: parent.create_folder() positional succeeded.") + except Exception as e: + print("WARN: parent.create_folder('%s') raised: %s" % (FOLDER_NAME, e)) + new_folder = None + if new_folder is None: try: new_folder = parent_object.create_folder(foldername=FOLDER_NAME) + if new_folder is not None: + print("DEBUG: parent.create_folder(foldername=) succeeded.") except Exception as e2: - print("WARN: parent.create_folder(foldername='%s') raised: %s -- trying alternate factories." % (FOLDER_NAME, e2)) + print("WARN: parent.create_folder(foldername='%s') raised: %s" % (FOLDER_NAME, e2)) new_folder = None + # Strategy 3: project-level create_folder default view (POU). + if new_folder is None and hasattr(primary_project, 'create_folder'): + try: + print("DEBUG: Trying primary_project.create_folder('%s') [default view]" % FOLDER_NAME) + new_folder = primary_project.create_folder(FOLDER_NAME) + if new_folder is not None: + print("DEBUG: project.create_folder() default-view succeeded.") + except Exception as e: + print("WARN: primary_project.create_folder('%s') raised: %s" % (FOLDER_NAME, e)) + new_folder = None + + # Strategy 4: generic create_object with the folder type UUID. Last-ditch + # for non-standard parent types. if new_folder is None and hasattr(parent_object, 'create_object'): - # CODESYS folder type UUID -- documented "generic IEC folder" type. - # Tried as a fallback for parents that don't expose create_folder. FOLDER_TYPE_UUID = '85d1215e-6520-4983-9a55-2d39d1f24cb4' try: - print("DEBUG: parent.create_folder() unavailable. Trying parent.create_object(typeUuid=%s, name='%s')" % (FOLDER_TYPE_UUID, FOLDER_NAME)) + print("DEBUG: Trying parent.create_object(typeUuid=%s, name='%s')" % (FOLDER_TYPE_UUID, FOLDER_NAME)) new_folder = parent_object.create_object(typeUuid=FOLDER_TYPE_UUID, name=FOLDER_NAME) except Exception as e: print("WARN: parent.create_object(typeUuid=%s) raised: %s" % (FOLDER_TYPE_UUID, e)) new_folder = None + # Strategy 5: types.IecFolder + parent.add (very old API). if new_folder is None and hasattr(script_engine, 'types') and hasattr(script_engine.types, 'IecFolder') and hasattr(parent_object, 'add'): try: print("DEBUG: Trying parent.add(script_engine.types.IecFolder, name='%s')" % FOLDER_NAME) @@ -88,9 +134,12 @@ try: if new_folder is None: raise TypeError( - "Parent object '%s' of type %s does not support any known folder-creation factory: " - "tried create_folder() positional, create_folder(foldername=...), " - "create_object(typeUuid=...), and add(script_engine.types.IecFolder)." % ( + "Parent object '%s' of type %s -- folder creation failed for all known strategies: " + "(1) primary_project.create_folder(name, SV_POU), " + "(2) parent.create_folder(name), " + "(3) primary_project.create_folder(name) default view, " + "(4) parent.create_object(typeUuid='85d1215e-...'), " + "(5) parent.add(script_engine.types.IecFolder)." % ( parent_name, type(parent_object).__name__)) if new_folder: diff --git a/tests/test-fixes.mjs b/tests/test-fixes.mjs new file mode 100644 index 0000000..0197457 --- /dev/null +++ b/tests/test-fixes.mjs @@ -0,0 +1,146 @@ +#!/usr/bin/env node +// Verifies the four broken-tool fixes (commit 2607063) end-to-end. +// Drives a single persistent CODESYS instance, runs each test against +// a *copy* of MCPTest2 so the source binary isn't mutated, and reports +// pass/fail with the relevant slice of script output for inspection. + +import * as fs from 'fs'; +import * as path from 'path'; +import * as os from 'os'; +import { fileURLToPath } from 'url'; +import { CodesysLauncher } from '../dist/launcher.js'; +import { ScriptManager } from '../dist/script-manager.js'; + +const __filename = fileURLToPath(import.meta.url); +const __dirname = path.dirname(__filename); +const repoRoot = path.resolve(__dirname, '..'); + +const MCPTEST2 = '\\\\files\\karstein.kvistad\\Documents\\Claude\\PLC\\MCPTest2\\MCPTest2.project'; +const MARINER = '\\\\files\\karstein.kvistad\\Documents\\Claude\\PLC\\mariner40206\\MRCodesysMarinerMK2.6_012.project'; + +const config = { + codesysPath: 'C:\\Program Files\\CODESYS 3.5.22.10\\CODESYS\\Common\\CODESYS.exe', + profileName: 'CODESYS V3.5 SP22 Patch 1', +}; + +// Set up a working copy of MCPTest2. +const workDir = fs.mkdtempSync(path.join(os.tmpdir(), 'codesys-mcp-testfixes-')); +const workMCP = path.join(workDir, 'MCPTest2.project'); +fs.copyFileSync(MCPTEST2, workMCP); +console.log(`copy: ${workMCP}`); + +const sm = new ScriptManager(path.join(repoRoot, 'src', 'scripts')); +const launcher = new CodesysLauncher(config); +console.log('launching persistent CODESYS...'); +await launcher.launch(); +console.log(' ready.'); + +const results = []; + +async function run(name, scriptArgs, helpers, projectPath) { + const params = { PROJECT_FILE_PATH: projectPath, ...scriptArgs }; + const script = sm.prepareScriptWithHelpers(name, params, helpers); + const t0 = process.hrtime.bigint(); + const r = await launcher.executeScript(script); + const ms = Number(process.hrtime.bigint() - t0) / 1e6; + const ok = r?.success && (r.output || '').includes('SCRIPT_SUCCESS'); + // ScriptManager caches templates after first load, so re-instantiate to + // force fresh reads if we mutate src/scripts/. Not needed here -- we + // only load each name once per test. + return { ok, ms, output: r?.output || '', error: r?.error || '' }; +} + +// --- TEST 1: create_folder fix --- +console.log('\n[1/3] create_folder fix...'); +{ + const r = await run('create_folder', + { FOLDER_NAME: 'Test_Bench_Folder', PARENT_PATH: 'PLCWinNT/Plc Logic/Application' }, + ['ensure_project_open', 'find_object_by_path'], workMCP); + console.log(` ${r.ms.toFixed(0)} ms ${r.ok ? 'PASS' : 'FAIL'}`); + if (!r.ok) { + console.log(' ---output tail---'); + console.log(r.output.slice(-1500).split('\n').map(l => ' ' + l).join('\n')); + } + // Cleanup: delete the folder so MCPTest2 stays unchanged for next runs. + if (r.ok) { + const cleanup = await run('delete_object', + { OBJECT_PATH: 'PLCWinNT/Plc Logic/Application/Test_Bench_Folder' }, + ['ensure_project_open', 'find_object_by_path'], workMCP); + console.log(` cleanup delete_object: ${cleanup.ok ? 'OK' : 'FAIL'}`); + } + results.push({ name: 'create_folder', ok: r.ok, ms: r.ms }); +} + +// --- TEST 2: compile_project + get_compile_messages fix --- +console.log('\n[2/3] compile_project + get_compile_messages fix...'); +{ + const r = await run('compile_project', {}, ['ensure_project_open'], workMCP); + console.log(` compile_project: ${r.ms.toFixed(0)} ms ${r.ok ? 'PASS' : 'FAIL'}`); + if (!r.ok) { + console.log(' ---output tail---'); + console.log(r.output.slice(-1500).split('\n').map(l => ' ' + l).join('\n')); + } else { + // Verify the JSON markers + parseable JSON in the output (this is + // exactly what would have failed pre-fix). + const m = r.output.match(/### COMPILE_MESSAGES_START ###\n([\s\S]*?)\n### COMPILE_MESSAGES_END ###/); + if (!m) { + console.log(' WARN: no markers found in output (still passed -- but JSON emit may be missing)'); + } else { + try { + const parsed = JSON.parse(m[1]); + console.log(` json parsed OK: ${parsed.length} messages`); + if (parsed.length > 0) { + console.log(' first message:', JSON.stringify(parsed[0])); + } + } catch (e) { + console.log(` FAIL: json parse error: ${e.message}`); + } + } + } + results.push({ name: 'compile_project', ok: r.ok, ms: r.ms }); + + const r2 = await run('get_compile_messages', {}, ['ensure_project_open'], workMCP); + console.log(` get_compile_messages: ${r2.ms.toFixed(0)} ms ${r2.ok ? 'PASS' : 'FAIL'}`); + if (!r2.ok) { + console.log(' ---output tail---'); + console.log(r2.output.slice(-1500).split('\n').map(l => ' ' + l).join('\n')); + } + results.push({ name: 'get_compile_messages', ok: r2.ok, ms: r2.ms }); +} + +// --- TEST 3: ensure_project_open cross-project switch --- +console.log('\n[3/3] ensure_project_open cross-project switch fix...'); +{ + // Currently MCPTest2 working copy is primary. Switch to mariner40206. + // Pre-fix: this would either fail or leave MCPTest2 still primary. + const r = await run('open_project', {}, ['ensure_project_open'], MARINER); + console.log(` open_project(mariner40206): ${r.ms.toFixed(0)} ms ${r.ok ? 'PASS' : 'FAIL'}`); + if (!r.ok) { + console.log(' ---output tail---'); + console.log(r.output.slice(-1500).split('\n').map(l => ' ' + l).join('\n')); + } + // Verify: list_project_libraries should show mariner's libs (~64), not MCPTest2's (5). + if (r.ok) { + const verify = await run('list_project_libraries', {}, ['ensure_project_open'], MARINER); + const libCount = (verify.output.match(/(\d+) library reference\(s\)/) || [])[1]; + console.log(` verify list_project_libraries: lib count = ${libCount} (expected ~64 for mariner40206)`); + const switchOk = libCount && parseInt(libCount, 10) > 50; + console.log(` cross-project switch verified: ${switchOk ? 'PASS' : 'FAIL'}`); + results.push({ name: 'cross_project_switch', ok: r.ok && switchOk, ms: r.ms }); + } else { + results.push({ name: 'cross_project_switch', ok: false, ms: r.ms }); + } +} + +console.log('\n=== summary ==='); +for (const t of results) { + console.log(` ${t.ok ? '✓' : '✗'} ${t.name} (${t.ms.toFixed(0)} ms)`); +} + +console.log('\nshutting down...'); +await launcher.shutdown(); +try { fs.rmSync(workDir, { recursive: true, force: true }); } catch {} +console.log('done.'); + +const allOk = results.every(r => r.ok); +process.exit(allOk ? 0 : 1); diff --git a/tests/unit/script-manager.test.ts b/tests/unit/script-manager.test.ts index 3017869..d74ef02 100644 --- a/tests/unit/script-manager.test.ts +++ b/tests/unit/script-manager.test.ts @@ -41,10 +41,10 @@ describe('ScriptManager', () => { expect(result).toBe('x and y'); }); - it('cache hit - second load returns same content', () => { + it('two loads return identical content (no cache, fresh file read each call)', () => { const first = mgr.loadTemplate('check_status'); const second = mgr.loadTemplate('check_status'); - expect(first).toBe(second); // Same reference from cache + expect(first).toEqual(second); }); it('combineScripts concatenates with double newlines', () => {