Merge the two workflow analyzers into one in NodePilot.Core - #222
Merged
Conversation
analyze_workflow existed twice: WorkflowReviewAnalyzer (Core, behind the AI
chat) and WorkflowAnalyzer (Mcp/Analysis, behind the MCP tool). Both mirrored
the canvas linter independently, with no parity test between them, and they had
already drifted:
- cycle was an error on one side and a warning on the other, though the engine
has no inDegree fallback and a cyclic graph Fails outright;
- the chat copy flagged missing-target-machine on runScript/waitForCondition,
which the Localhost bypass makes perfectly valid — it told authors to "fix" a
correct workflow;
- the chat copy did not know unresolvable {{...}} references at all, so a log
message reading {{gibtsnicht.output}} analysed as ok with no findings and then
failed on the first run.
The MCP implementation was the superset and moves to
NodePilot.Core.WorkflowDefinitions, keeping the structural pre-check and the
empty-workflow case from the Core copy. Both surfaces now call it, so their
findings can no longer disagree. Mcp/Analysis/VariableResolver moves along as
WorkflowDataBusAnalyzer; the rename avoids a collision with the engine's
runtime VariableResolver, which Core is visible to.
Two behaviour notes: the analyzer no longer reports unknown-activity-type of its
own, because the structural pre-check already rejects it and names the node
index and the offending type; and ok now means "no error-severity finding" on
both surfaces, with warnings still counted in count/findings.
WorkflowAnalyzerFrontendParityTests moves to Engine.Tests, where Core-level
guards live, and becomes the single mirror guard against workflowLint.ts.
A roadmap entry under E records the direction that was NOT taken — routing the
chat through nodepilot-mcp — and why: one DPAPI session versus per-request
ClaimsPrincipal, loopback HTTP per call, and ~90 write tools the chat
deliberately lacks.
TemplateGrammarParityTests locates the databus analyzer by PATH and still read src/NodePilot.Mcp/Analysis/VariableResolver.cs, which the consolidation moved to NodePilot.Core/WorkflowDefinitions/WorkflowDataBusAnalyzer.cs. The test threw FileNotFoundException instead of comparing anything -- loudly here, but the same guard shape fails open if the read is ever made tolerant, so the missing file is now asserted with a message naming the path to update. Wording follows the move as well: the analyzer is no longer MCP-specific, it backs the MCP tool and the AI chat.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
analyze_workflowexisted twice —WorkflowReviewAnalyzer(Core, behind the AI chat) andWorkflowAnalyzer(Mcp/Analysis, behind the MCP tool). Both mirrored the canvas linter independently, with no parity test between them, and they had already drifted:cyclewas an error on one side and a warning on the other, though the engine has noinDegreefallback and a cyclic graph Fails outright;missing-target-machineonrunScript/waitForCondition, which the Localhost bypass makes perfectly valid — it told authors to "fix" a correct workflow;{{...}}references at all, so a log message reading{{gibtsnicht.output}}analysed as ok with no findings and then failed on the first run.Change
The MCP implementation was the superset and moves to
NodePilot.Core.WorkflowDefinitions, keeping the structural pre-check and the empty-workflow case from the Core copy. Both surfaces now call it, so their findings can no longer disagree.Mcp/Analysis/VariableResolvermoves along asWorkflowDataBusAnalyzer— the rename avoids a collision with the engine's runtimeVariableResolver, which Core is visible to.Two behaviour notes:
unknown-activity-typeof its own, because the structural pre-check already rejects it and names the node index and the offending type;oknow means "no error-severity finding" on both surfaces, with warnings still counted incount/findings.WorkflowAnalyzerFrontendParityTestsmoves to Engine.Tests, where Core-level guards live, and becomes the single mirror guard againstworkflowLint.ts. A roadmap entry under E records the direction that was not taken — routing the chat throughnodepilot-mcp— and why: one DPAPI session versus per-requestClaimsPrincipal, loopback HTTP per call, and ~90 write tools the chat deliberately lacks.Second commit: a guard that the move broke
TemplateGrammarParityTestslocates the databus analyzer by path and still readsrc/NodePilot.Mcp/Analysis/VariableResolver.cs. It threwFileNotFoundExceptioninstead of comparing anything. Fixed to the new path, with the file's existence now asserted explicitly so the failure names the path to update rather than surfacing as an IO stack trace.Tests
Full solution builds. Ran:
NodePilot.Engine.TestsNodePilot.Mcp.TestsNodePilot.Ai.Tests