fix(add_library): dedup pre-check + default to add_placeholder, opt-in direct/force
Empirical failure: add_library('Standard') on a project that already had Standard, * (System) silently created a SECOND direct Standard reference, pulling in unresolved transitive deps (e.g. yellow-warning IoStandard 3.1.3.1).
Root cause: script always called add_library() without checking lm.references first; never called add_placeholder() so the result was a direct (non-* (System)) reference.
Fix: (1) walk lm.references for an existing entry by bare name and no-op with a confirmation message unless force=true; (2) default to add_placeholder() so transitive deps resolve at compile (matches the modern '<Name>, * (System)' convention); (3) keep add_library() reachable via direct=true; (4) on miss, dump dir(lm) so unknown SPs surface the actual API.
Docs: https://content.helpme-codesys.com/en/ScriptingEngine/ScriptLibManObject.html
Mildly breaking for callers: previous behaviour was always direct
add_library(); pass direct=true to restore. The dedup default also flips
'add second copy' to 'no-op' -- pass force=true to restore.
Test updated: tests/integration/e2e.test.ts now passes USE_DIRECT='0' and
FORCE_DUP='0' alongside LIBRARY_NAME, asserts add_placeholder + dedup
strings appear in the rendered script.
### Manual smoke test
1. mcp__codesys__add_library libraryName=Standard against a project that
already has Standard listed: expect SCRIPT_SUCCESS with body 'Library
Already Present: Standard' and NO second entry in the Library Manager.
2. mcp__codesys__add_library libraryName=Util on a project without Util:
expect a new entry rendered as 'Util, * (System)' (placeholder, not
direct).
3. mcp__codesys__add_library libraryName=Standard direct=true: expect a
direct (non-* (System)) reference even on dedup hit if also force=true.
4. mcp__codesys__list_project_libraries should reflect each result.
This commit is contained in:
Karstein Phobic Nyvold Kvistad
parent
19d6cc3d8b
commit
fc49e7ff8b
3 changed files with 90 additions and 19 deletions
|
|
@ -41,6 +41,8 @@ import sys, scriptengine as script_engine, os, traceback
|
|||
# bricking the next open.
|
||||
|
||||
LIBRARY_NAME = "{LIBRARY_NAME}"
|
||||
USE_DIRECT = "{USE_DIRECT}" == "1"
|
||||
FORCE_DUP = "{FORCE_DUP}" == "1"
|
||||
|
||||
|
||||
def _resolve_in_repo(name):
|
||||
|
|
@ -190,6 +192,26 @@ try:
|
|||
|
||||
print("DEBUG: Library Manager found: %s" % getattr(lib_manager, 'get_name', lambda: '?')())
|
||||
|
||||
# Step 0 (NEW per OPEN-BUGS-CROSS-REFERENCE Bug 4): dedup pre-check.
|
||||
# If a reference with the same bare name already exists (whether
|
||||
# placeholder or managed), no-op with a confirmation message rather
|
||||
# than silently creating a duplicate. Bypass with FORCE_DUP=1.
|
||||
existing_ref = _find_added_reference(lib_manager, LIBRARY_NAME)
|
||||
if existing_ref is not None and not FORCE_DUP:
|
||||
existing_name = getattr(existing_ref, 'name', '?')
|
||||
is_ph = bool(getattr(existing_ref, 'is_placeholder', False))
|
||||
kind = "placeholder" if is_ph else "managed"
|
||||
msg = ("Library '%s' is already referenced (%s, name=%r). "
|
||||
"No-op (use force=true to add another reference)."
|
||||
% (LIBRARY_NAME, kind, existing_name))
|
||||
print(msg)
|
||||
print("Library Already Present: %s" % LIBRARY_NAME)
|
||||
print("Project: %s" % project_name)
|
||||
print("SCRIPT_SUCCESS: %s" % msg)
|
||||
sys.exit(0)
|
||||
if existing_ref is not None and FORCE_DUP:
|
||||
print("DEBUG: dedup pre-check found existing reference for '%s' but FORCE_DUP=1 -- adding duplicate." % LIBRARY_NAME)
|
||||
|
||||
# Step 1: pre-resolve the library name against the installed repository.
|
||||
# If found, we will pass the ManagedLib to add_library() to get a
|
||||
# MANAGED reference instead of a placeholder reference.
|
||||
|
|
@ -203,32 +225,70 @@ try:
|
|||
else:
|
||||
print("DEBUG: Pre-resolve via library_manager.find_library returned no hit for '%s'." % LIBRARY_NAME)
|
||||
|
||||
# Step 2: add the reference, preferring the managed overload.
|
||||
# Step 2: add the reference. Default is add_placeholder() to match the
|
||||
# modern '<Name>, * (System)' convention (placeholder resolves at
|
||||
# compile time so transitive deps stay flexible). USE_DIRECT=1 opts
|
||||
# into the legacy direct add_library() path (specific-version pin).
|
||||
# Per docs: ScriptLibManObject exposes BOTH add_library(...) and
|
||||
# add_placeholder(...) -- see helpme-codesys.com/ScriptLibManObject.
|
||||
added = False
|
||||
add_attempt_errors = []
|
||||
|
||||
if resolved_lib is not None and hasattr(lib_manager, 'add_library'):
|
||||
if not USE_DIRECT and hasattr(lib_manager, 'add_placeholder'):
|
||||
# Default branch: placeholder add. Try (name, default_resolution)
|
||||
# then (name) -- the default_resolution arg lets the IDE record
|
||||
# which managed lib the placeholder should resolve to.
|
||||
try:
|
||||
lib_manager.add_library(resolved_lib)
|
||||
added = True
|
||||
print("DEBUG: add_library(ManagedLib) succeeded.")
|
||||
if resolved_lib is not None:
|
||||
try:
|
||||
lib_manager.add_placeholder(LIBRARY_NAME, resolved_lib)
|
||||
added = True
|
||||
print("DEBUG: add_placeholder(name, ManagedLib) succeeded.")
|
||||
except Exception as e_pm:
|
||||
add_attempt_errors.append("add_placeholder(name, ManagedLib): %s" % e_pm)
|
||||
print("DEBUG: add_placeholder(name, ManagedLib) failed: %s" % e_pm)
|
||||
if not added:
|
||||
lib_manager.add_placeholder(LIBRARY_NAME)
|
||||
added = True
|
||||
print("DEBUG: add_placeholder(name) succeeded.")
|
||||
except Exception as e:
|
||||
add_attempt_errors.append("add_library(ManagedLib): %s" % e)
|
||||
print("DEBUG: add_library(ManagedLib) failed: %s" % e)
|
||||
add_attempt_errors.append("add_placeholder(name): %s" % e)
|
||||
print("DEBUG: add_placeholder(name) failed: %s" % e)
|
||||
|
||||
# Direct add_library path -- explicit opt-in OR fallback if the IDE
|
||||
# doesn't expose add_placeholder.
|
||||
if not added and hasattr(lib_manager, 'add_library'):
|
||||
try:
|
||||
lib_manager.add_library(LIBRARY_NAME)
|
||||
added = True
|
||||
print("DEBUG: add_library(name) succeeded (placeholder overload).")
|
||||
except Exception as e:
|
||||
add_attempt_errors.append("add_library(name): %s" % e)
|
||||
print("DEBUG: add_library(name) failed: %s" % e)
|
||||
if resolved_lib is not None:
|
||||
try:
|
||||
lib_manager.add_library(resolved_lib)
|
||||
added = True
|
||||
print("DEBUG: add_library(ManagedLib) succeeded.")
|
||||
except Exception as e:
|
||||
add_attempt_errors.append("add_library(ManagedLib): %s" % e)
|
||||
print("DEBUG: add_library(ManagedLib) failed: %s" % e)
|
||||
if not added:
|
||||
try:
|
||||
lib_manager.add_library(LIBRARY_NAME)
|
||||
added = True
|
||||
print("DEBUG: add_library(name) succeeded (placeholder overload).")
|
||||
except Exception as e:
|
||||
add_attempt_errors.append("add_library(name): %s" % e)
|
||||
print("DEBUG: add_library(name) failed: %s" % e)
|
||||
|
||||
if not added:
|
||||
# Per Bug 4 step 3: detect partial-success state. Neither
|
||||
# add_library nor add_placeholder is exposed -- emit a clear
|
||||
# error rather than silent-failing.
|
||||
api_attrs = []
|
||||
try:
|
||||
api_attrs = sorted([a for a in dir(lib_manager) if not a.startswith('_')])
|
||||
except Exception:
|
||||
pass
|
||||
raise RuntimeError(
|
||||
"Could not add library '%s'. add_library overloads failed: %s"
|
||||
% (LIBRARY_NAME, "; ".join(add_attempt_errors) or "no add_library on libman"))
|
||||
"Could not add library '%s'. Add overloads failed: %s. lm api: %s"
|
||||
% (LIBRARY_NAME,
|
||||
"; ".join(add_attempt_errors) or "neither add_library nor add_placeholder on libman",
|
||||
', '.join(api_attrs) if api_attrs else '<dir() empty>'))
|
||||
|
||||
# Step 3: verify the just-added reference actually resolved. If it did
|
||||
# not, REMOVE it and refuse to save -- saving an unresolvable
|
||||
|
|
|
|||
|
|
@ -1854,18 +1854,22 @@ export async function startMcpServer(config: ServerConfig): Promise<void> {
|
|||
|
||||
s.tool(
|
||||
'add_library',
|
||||
'Adds a library reference to the CODESYS project. The library must be installed in the CODESYS library repository.',
|
||||
"Adds a library reference to the CODESYS project. By default uses add_placeholder() to match the modern '<Name>, * (System)' convention so transitive deps resolve at compile time -- pass direct=true to opt into the legacy direct add_library() (specific version pin). Pre-checks lm.references for an existing reference with the same name and no-ops with a confirmation message unless force=true. The library must be installed in the CODESYS library repository.",
|
||||
{
|
||||
projectFilePath: z.string().describe("Path to the project file."),
|
||||
libraryName: z.string().describe("Name of the library to add (e.g., 'Standard', 'Util', 'CAA Memory')."),
|
||||
direct: z.boolean().optional().describe("If true, use direct add_library() (specific-version pin) instead of the default add_placeholder() (resolves at compile)."),
|
||||
force: z.boolean().optional().describe("If true, add even if a reference with the same name already exists (creates a duplicate). Default: dedup -- silently no-op with a confirmation message."),
|
||||
},
|
||||
async (args: { projectFilePath: string; libraryName: string }) => {
|
||||
async (args: { projectFilePath: string; libraryName: string; direct?: boolean; force?: boolean }) => {
|
||||
const escaped = resolvePath(args.projectFilePath, workspaceDir);
|
||||
const script = scriptManager.prepareScriptWithHelpers(
|
||||
'add_library',
|
||||
{
|
||||
PROJECT_FILE_PATH: escaped,
|
||||
LIBRARY_NAME: args.libraryName.trim(),
|
||||
USE_DIRECT: args.direct ? '1' : '0',
|
||||
FORCE_DUP: args.force ? '1' : '0',
|
||||
},
|
||||
['ensure_project_open']
|
||||
);
|
||||
|
|
|
|||
|
|
@ -111,6 +111,8 @@ describe('E2E Script Preparation', () => {
|
|||
{
|
||||
PROJECT_FILE_PATH: 'C:\\test.project',
|
||||
LIBRARY_NAME: 'Util',
|
||||
USE_DIRECT: '0',
|
||||
FORCE_DUP: '0',
|
||||
},
|
||||
['ensure_project_open']
|
||||
);
|
||||
|
|
@ -118,8 +120,13 @@ describe('E2E Script Preparation', () => {
|
|||
// Pre-resolve via the IDE-level library_manager
|
||||
expect(script).toContain('library_manager');
|
||||
expect(script).toContain('find_library');
|
||||
// Managed-overload preference
|
||||
// Managed-overload preference (when USE_DIRECT=1 or add_placeholder unavailable)
|
||||
expect(script).toContain('add_library(resolved_lib)');
|
||||
// Default-to-placeholder branch added per Bug 4
|
||||
expect(script).toContain('add_placeholder');
|
||||
// Dedup pre-check added per Bug 4
|
||||
expect(script).toContain('FORCE_DUP');
|
||||
expect(script).toContain('Library Already Present');
|
||||
// Post-add resolution gate
|
||||
expect(script).toContain('effective_resolution');
|
||||
expect(script).toContain('is_placeholder');
|
||||
|
|
|
|||
Loading…
Reference in a new issue