Update PSCAD MCP: project_tools.py and simset_tools.py

[Xiaoyang-Wang-TAMU]

PSCAD MCP Tool modification:
find_components is fixed with correct parameters.
Passed definition and name incorrectly as keyword arguments to Project.find_all(); the installed API expects positional arguments

simset_tools.py is fixed by replacing all project.simset with workspace.simset

[qian-harvard]

Thanks for digging into these — I checked both fixes against the actual mhi-pscad 3.1.2 source (pulled the wheel from PyPI), and you're right on both counts. Project has no simulation_set/simulation_sets methods at all; they only exist on PSCAD (pscad.py:1497, :1549), so the old simset tools would have raised AttributeError against a real PSCAD. And find_all really does take positional criteria — the old definition=/name= kwargs were falling into **params and being matched as literal parameter names, plus c.id/c.name don't exist on components (they're iid/defn_name). So all three of these tools were broken on main and only looked healthy because the tests mock them.

A few things to sort out before this can merge.

Blocking

1. Two tests fail on this branch (main is 17/17, this is 15/17) — see below.
2. find_components can silently return the wrong result set, because of how find_all reinterprets a lone positional argument. (inline)
3. component.defn_name is a remote call made outside robust_executor. (inline)

Worth discussing

4. run_simulation_set holds the single executor worker for the whole batch with the watchdog off, which starves every other tool. (inline)
5. project_name is still a required schema argument on tools that immediately del it. (inline)

---

The two failing tests — PSCAD/tests/test_enhanced_tools.py

These are mock-based, so they run fine without PSCAD installed:

FAILED test_enhanced_tools.py::TestEnhancedPSCADTools::test_list_simulation_sets
FAILED test_enhanced_tools.py::TestEnhancedPSCADTools::test_run_simulation_set

• test_list_simulation_sets — line 83 mocks self.mock_project.simulation_sets, but the tool now calls pscad.simulation_sets, so the mock is never reached: AssertionError: 'Batch1' not found in <MagicMock name='pscad_manager.pscad.simulation_sets()'>. It also needs to return plain strings now, since PSCAD.simulation_sets() is typed List[str] (pscad.py:1497) rather than objects with .name.

• test_run_simulation_set — line 94 asserts "started" but the message now says "completed". Heads up that there's a second, independent break behind that one: setUp line 27 wires the fixture as self.mock_project.simulation_set.return_value = self.mock_sim_set, so with the tool now resolving via pscad.simulation_set, .run() lands on an auto-created child of mock_pscad and line 95's self.mock_sim_set.run.assert_called_once() fails with "Called 0 times". Fixing only the assertion string will leave it red.

Roughly:

# setUp
self.mock_pscad.simulation_set.return_value = self.mock_sim_set

# test_list_simulation_sets
self.mock_pscad.simulation_sets.return_value = ["Batch1"]

Also worth adding: no test anywhere mocks find_all returning actual components (test_tools.py:88 returns []), so the entire new iid/defn_name/parameters() path is uncovered.

Stale docs

• PSCAD/README.md:113 still says list_simulation_sets discovers batch runs "in a project", and PSCAD/EXAMPLES.md:27 says "List all simulation sets in the active project" — both now workspace-wide. Since EXAMPLES is a prompt guide, an LLM client reading it will present workspace-wide results as project-specific.
• PSCAD/COMPARISON_GUIDE.md:34 now overclaims: "Watchdog Protection: If project.run() takes too long to respond, the MCP Executor triggers a timeout instead of hanging the AI" — no longer true for run_simulation_set, the one tool that disables the watchdog.

PSCAD/pyproject.toml:23 — unpinned mhi-pscad

Not introduced by this PR, but this PR is the first code to depend on it: defn_name changed return type in 3.0.2 (component.py:236-249: "Return type changed from Union[str, Tuple[str, str]] to str"), so on an older install find_components emits "definition": ["master", "source3"] instead of "master:source3". Since the extra is just windows = ["mhi-pscad", ...] with no bound, a >=3.0.2 lower bound would make this safe. Everything else the PR uses is 2.0+.

---

Happy to pair on the sim-set piece if you'd rather split it into its own PR — that one has a real design question in it.

[qian-harvard]

Passing the two criteria positionally means the library's disambiguation heuristic decides what they mean, and it can silently flip them. From mhi/pscad/project.py:1404-1417:

if defn:
    if defn in _BUILTIN_COMPONENTS:       # 'Bus', 'TLine', 'Cable', 'GraphFrame',
        pass                              # 'Sticky', 'Wire', 'Divider', ...
    elif ':' in defn:
        namespace, defn = defn.split(':', 1)
    elif not named:                       # <-- lone bare positional
        named = defn                      #     becomes a NAME search
        defn = None

Two consequences:

• find_components(name="Bus") → the lone positional "Bus" is a builtin, so it becomes a definition search and returns every bus bar in the project instead of the component named Bus.
• find_components(definition="source3") → not a builtin, no colon, no named → it collapses into a name search and returns [] instead of every master:source3 instance.

Neither raises, so the caller just gets wrong data.

The name-only case has a clean fix — pass None in the definition slot so the if defn: block is skipped entirely:

if definition and name:
    components = project.find_all(definition, name)   # this ordering is correct
elif definition:
    components = project.find_all(definition)
else:
    components = project.find_all(None, name)         # pins `name` to the name slot

The definition-only case can't be fixed the same way — the API has no way to express "this bare string is a definition" (passing find_all(definition, None) collapses identically, since named is falsy). So definition genuinely has to be namespace-qualified (master:source3) or one of the builtins. Worth stating that in the docstring, since the docstring becomes the MCP tool description the client reads:

"""Find components matching criteria in a project.

``definition`` must be namespace-qualified (e.g. ``master:source3``) or a
built-in name ("Bus", "TLine", "Cable", "GraphFrame", "Sticky", ...). A bare
definition name is interpreted by the PSCAD API as a component name.
"""

If you want to be stricter, rejecting an unqualified non-builtin definition with a clear error beats silently degrading it to a name search.

[qian-harvard]

This one looks like a plain attribute read but it isn't. From mhi/pscad/component.py:231-236:

@rmi_property
def _defn_name(self): ...

@cached_property
def defn_name(self) -> str:

So the first access per component is a synchronous RMI round-trip — and it's happening on the asyncio event-loop thread, outside run_safe. That bypasses all three things the executor exists to provide: the single COM-initialized worker, the lock, and the watchdog. If PSCAD is frozen or showing a modal dialog here, the event loop itself blocks and no watchdog can fire, so the whole server hangs for every client rather than one call timing out.

There's also a transport hazard: mhi/common/remote.py:1007-1012 is self._write(msg) then self._response.get() on a single shared queue with no correlation IDs and no send lock, so a defn_name read racing an in-flight worker call can consume the wrong response.

(component.iid on line 156 is fine — it's a local self._identity['iid'] read, no RMI.)

Easiest fix is the batching one in my other comment, which puts both the parameters() and defn_name reads on the worker thread.

[qian-harvard]

Two things here.

Missing None guard. component.parameters() can return None — FormCodec.decode passes it straight through (mhi/pscad/form.py:435-436), and this file already defends against it in get_component_parameters:

return params if params else {}   # project_tools.py:171

Without that guard, one component returning None raises AttributeError: 'NoneType' object has no attribute 'get' and kills the whole listing, discarding every result already collected.

One dispatch per component. Each iteration is a separate run_safe — a thread hop, a lock acquire/release, and its own 30s watchdog — plus a defn_name RMI. A search matching a few hundred components (easy with definition="Bus" on a real case) becomes a few hundred sequential round-trips, and total latency is unbounded even though no single call trips its timeout.

Both go away if the loop runs inside one run_safe closure on the worker thread:

def collect():
    results = []
    for component in components:
        parameters = component.parameters() or {}
        results.append({
            "id": component.iid,
            "name": (parameters.get("Name")
                     or parameters.get("name")
                     or parameters.get("NAME")
                     or ""),
            "definition": component.defn_name,
        })
    return results

# Covers N round-trips, so give it the same generous budget as load_projects.
return await robust_executor.run_safe(collect, _timeout=120)

One aside: the Name/name/NAME chain is correct, not defensive guesswork — find_all's own docstring (project.py:1370-1371) defines a component's name as "a parameter called name, Name, or NAME". Maybe add a one-line comment saying so, since it reads as speculative otherwise.

[qian-harvard]

Could you drop the commented-out old implementation and the ####### Code is modified / ######## banners? Git history already has the old version, and the commented block references attributes that don't exist on the real API (c.id, c.name), so it's actively misleading to anyone who tries to restore it later.

The trailing ######## on line 163 also sits at a stray 5-space indent after return results — harmless as a comment, but a future edit aligned to it would be an IndentationError.

[qian-harvard]

Minor: this is equivalent and matches the single-line run_safe style used elsewhere in the file (e.g. line 169):

criteria = [c for c in (definition, name) if c]

Also worth knowing: the no-criteria ValueError you added mirrors the library's own behavior — find_all raises ValueError("No search criteria given") at project.py:1398-1399 — so you're not removing a "list all components" mode. Good call.

[qian-harvard]

run_project refuses to start when the license isn't available (project_tools.py:37-38):

if not await robust_executor.run_safe(pscad.licensed):
    return {"started": False, "error": "PSCAD is not licensed."}

Worth adding the same guard here. Without it, an unlicensed PSCAD can stall or pop a licensing dialog inside a call that has its watchdog explicitly disabled — so the single worker blocks indefinitely on a batch that was never going to run. One call turns that into an immediate, clear error.

[qian-harvard]

This is the one I'd most like to talk through. SimulationSet.run() blocks until every task finishes (simset.py:362 wraps a blocking _run RMI), and RobustExecutor has exactly one worker plus a lock (executor.py:37, 57). So for the entire duration of a batch — potentially hours — every other tool call queues behind this and then dies at its own 30-second watchdog with:

PSCAD timed out during X. It might be frozen or showing a dialog.

...which is a confusing thing to tell a user whose simulation is running perfectly fine. get_run_status, list_projects, even repair_connection are all affected, and there's no cancel path: cancelling the coroutine can't stop a thread already running in the executor.

This is the exact failure mode run_project was written to avoid — its docstring says it uses the non-blocking start() specifically to avoid "holding the single PSCAD worker for the entire multi-minute build+run", with get_run_status for polling.

I don't think there's a clean fix inside this PR, because SimulationSet has no non-blocking start (only build, build_modified, run). Two options:

• Minimum for this PR: keep it blocking, but document the starvation in the docstring so callers know what they're buying, and add the license pre-flight (other comment).
• Follow-up: run the batch on a dedicated thread outside the shared executor, return immediately, and add a sim-set status tool — mirroring the run_project + get_run_status pair.

Happy either way, but the current docstring reads as though this is routine, and it isn't.

[qian-harvard]

Two problems with this message:

• project_name is never resolved or validated, so this asserts a project association that was never checked. run_simulation_set("WrongProj", "Batch1") happily reports success "in project 'WrongProj'" for a set whose tasks belong to entirely different projects.
• "completed" isn't verified. SimulationSet.run(consumer=None) skips the build-events subscription entirely (pscad.py:262-264 only subscribes if handler), so every task's compile can fail and this still reports completion.

Simplest version that doesn't overclaim:

return f"Simulation set '{sim_set_name}' finished running."

If you want real status, SimulationSet.list_tasks() exists (simset.py:192), or you could pass a consumer to run() and collect the build events.

[qian-harvard]

I don't think the schema-compatibility rationale holds up. The MCP SDK generates inputSchema from the function signature, marking any parameter without a default as required. Running these two signatures through the SDK's schema generator (func_metadata) gives:

run_simulation_set -> required: ['project_name', 'sim_set_name']
find_components    -> required: ['project_name']

(I had mcp 1.29 to hand rather than the pinned mcp>=2,<3, but this is core, stable schema-generation behavior — worth a sanity check on your end if you want to be sure.)

So the parameter isn't being preserved for compatibility; it's a required argument every client must invent and the tool then discards. And MCP clients re-fetch tools/list each session, so there's no older caller to stay compatible with.

add_task_to_set (line 44, same idiom) is the riskier one — it now has two project-shaped parameters side by side where the first is ignored, so a client that swaps them adds the wrong project as a task and gets no error.

I'd just drop the parameter from all three signatures (updating the two positional calls in the tests). If you'd rather keep it for now, Optional[str] = None at least stops it being required.

Good instinct on the docstrings, though — noting that "Simulation sets belong to the PSCAD workspace, not to one Project" is exactly right, and mcp surfaces it as the tool description.