Skip to content

feat(flow)!: a path ends deliberately or says it is broken, and a flow records its last run - #2354

Merged
rubenvdlinde merged 3 commits into
developmentfrom
feat/or-flow-connectivity-and-last-run
Aug 5, 2026
Merged

feat(flow)!: a path ends deliberately or says it is broken, and a flow records its last run#2354
rubenvdlinde merged 3 commits into
developmentfrom
feat/or-flow-connectivity-and-last-run

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

Closes the silent-success hole in the flow engine, and gives a flow a memory of its own last run.

The defect

A node with no outgoing edge was a silent success. Its token arrived, the step ran, the engine found no enabled transition, the run stopped — and was recorded COMPLETED. Nothing failed, so nothing was logged, and the author saw a green run that had not done the work.

Ending a path is now something a node says

Two ways, OR-ed and never AND-ed:

IFlowTerminalNode a marker interface on the TYPE, resolved via FlowNodeRegistry::isTerminal(), so a terminal step contributed by openconnector or hermiq needs no OpenRegister change. StopNode implements it.
"exit": true on the node instance — for a sink whose step type is an ordinary action, which is what every migrated flow has, because that was a legitimate end of a path under the old place-and-edge reading.

Requiring both would make every migrated flow depend on a registry the migration cannot see.

A marker interface rather than a method on IFlowNode, for the reason IFlowNodeConfigKeys already documents: implementations live in other repositories, and widening the interface fatals those apps on load.

Warn on save, refuse on run

Saving succeeds and warns. A disconnected graph is the normal state of one being authored; refusing to store it would force the author to build the graph in an order that is never disconnected, which no editor can require.

Running is refused, and the guard sits in FlowRunService::queue() — the one choke point every dispatch path passes through: manual, trigger, schedule, MCP, the workflow-engine operation, and a sub-flow call. Guarding FlowService::run() alone would have left cron-fired flows unguarded, and those are most of them.

On refusal no FlowRun is created, so the verdict is written onto the flow (status / statusMessage, naming the nodes). That is exactly what makes a refused flow distinguishable from one nobody has triggered — there is no run to read. An accepted run clears a stale error back to ok.

The schedule sweep catches the refusal per flow: it iterates every due flow, so letting it propagate would abort the sweep and stop every later flow from firing — one broken definition silently disabling the rest, presenting as "cron stopped working" rather than as a fault in a named flow.

A typeless node is deliberately not reported here — FlowDefinitionBuilder already refuses it by name, and two findings on one node for one defect is how a warning list becomes noise.

Last run

Six nullable columns, no backfill. A null lastRunAt means "has never run"; a value derived from run history would assert a history the column did not record. Written only when a run reaches a terminal state, so the list answers "how did it last go?" rather than "it hasn't finished".

Also

Adds the canonical openspec/specs/flow-engine/spec.md. It did not exist — the flow-engine spec lived only inside changes/ — so @spec had nowhere canonical to point.

What is NOT done

Stated in tasks.md rather than quietly skipped: the schedule/trigger dispatch wiring and the last-run write-back are not yet pinned by tests. Both need FlowRunService built with a mocked container, mapper and preflight. The connectivity verdict they consume is covered, each assertion with a positive control proving the same graph goes quiet once wired.

Verification honesty

The unit suite could not be run locally. Once lib/base.php loads, Nextcloud's autoloader resolves OCA\OpenRegister\* to the installed app, not the working copy — measured with ReflectionClass::getFileName(). CI's "copy the app out of /var/www/html" step does not prevent this; CI is immune only because it deploys the code under test first, making both copies identical. Run that recipe locally against an older deployment and the suite reports on the deployed app — a green that says nothing. CI is the authoritative gate for this change.

Breaking

A flow with a dead-ended node is now refused at run time instead of completing silently. Mark deliberate sinks "exit": true, or give them a terminal step type.

…w records its last run

A node with no outgoing edge was a silent success. Its token arrived, the step
ran, the engine found no enabled transition, and the run was recorded COMPLETED
— so the author saw a green run that had not done the work. Nothing failed, so
nothing was logged. That is the defect this closes.

Ending a path deliberately is now something a node SAYS, two ways, OR-ed:

  IFlowTerminalNode   a marker interface on the TYPE, resolved through
                      FlowNodeRegistry::isTerminal(), so a terminal step
                      contributed by openconnector or hermiq needs no
                      OpenRegister change. StopNode implements it.
  "exit": true        on the node instance, for a sink whose step type is an
                      ordinary action — which is what every migrated flow has,
                      because that WAS a legitimate end of a path under the old
                      place-and-edge reading.

They are OR-ed and never AND-ed: requiring both would make every migrated flow
depend on a registry the migration cannot see.

A marker interface rather than a method on IFlowNode, for the reason
IFlowNodeConfigKeys already documents: implementations live in other repos, and
widening the interface fatals those apps on load.

WARN ON SAVE, REFUSE ON RUN

Saving a half-wired flow succeeds and returns the warning. A disconnected graph
is the normal state of one being authored; refusing to store it would force the
author to build the graph in an order that is never disconnected, which no
editor can require.

Running is refused. The guard sits in FlowRunService::queue(), which is the one
choke point every dispatch path passes through — manual, trigger, schedule,
MCP, the workflow-engine operation and a sub-flow call. Guarding
FlowService::run() instead would have left cron-fired flows unguarded, and those
are most of them. On refusal no FlowRun is created, and the verdict is written
onto the FLOW (status/status_message naming the nodes) precisely because there
is no run to read: that is what makes a refused flow distinguishable from one
nobody has triggered. An accepted run clears a stale error back to ok.

The schedule sweep catches the refusal PER FLOW. It iterates every due flow, so
letting it propagate would abort the sweep and stop every later flow from
firing — one broken definition silently disabling the rest, presenting as "cron
stopped working" rather than as a fault in a named flow.

A typeless node is deliberately NOT reported here. FlowDefinitionBuilder already
refuses it by name, and two findings on one node for one defect is how a warning
list becomes noise.

LAST RUN

Six nullable columns, no backfill. NULL lastRunAt means "has never run" — a
value derived from run history would assert a history the column did not record.
Written only when a run reaches a terminal state, so the flow list answers "how
did it last go?" rather than "it hasn't finished".

Also adds the canonical openspec/specs/flow-engine/spec.md, which did not exist
— it lived only inside changes/ — so @SPEC can target a canonical path.

Not done, and stated in tasks.md rather than quietly skipped: the schedule and
trigger dispatch wiring, and the last-run write-back, are not yet pinned by
tests. Both need FlowRunService built with a mocked container.

The suite could not be run locally: once lib/base.php loads, NC's autoloader
resolves OCA\OpenRegister\* to the INSTALLED app, not the working copy —
measured with ReflectionClass::getFileName(). CI's "copy the app out" recipe
does not prevent that; CI is immune only because it deploys the code under test
first. Run locally against an older deployment it reports on the deployed app.
CI is the authoritative gate here.

BREAKING: a flow with a dead-ended node is now refused at run time instead of
completing silently. Mark deliberate sinks "exit": true, or give them a terminal
step type.
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ 1345887

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-specs
test-l10n
composer ✅ 173/173
npm ✅ 713/713
PHPUnit
Newman
Playwright
Hydra gates

Quality workflow — 2026-08-05 20:18 UTC

Download the full PDF report from the workflow artifacts.

… deliberately

StopNode gained `implements IFlowTerminalNode` without the matching `use`.
StopNode lives in ...\Service\Flow\Nodes, so PHP resolved the bare name
relative to THAT namespace and looked for ...\Nodes\IFlowTerminalNode. `php -l`
cannot see it — the syntax is valid and the failure is at class-resolution
time — so it surfaced as 16 identical PHPUnit errors plus phpstan, psalm and
phpmd all reporting the same unknown interface.

My local phpstan/psalm run passed because I listed the changed files by hand and
StopNode.php was not among them: the check excluded the one file with the bug.

The fixtures in FlowNodeConfigDialectTest, FlowNodeConfigVocabularyTest and
FlowNodePreflightRegressionTest are single nodes or chains with no outgoing edge
from their last node, so the new connectivity check reports them — correctly.
Those suites are about a node's config DIALECT and the registry, not about
connectivity, and each asserts an exact finding count; an unrelated second
warning made them count two different things. Marking the last node
`exit: true` makes each fixture a COMPLETE document rather than suppressing the
check, and the dialect suite's positive control still asserts an exactly-empty
report.
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ 68baa6f

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-specs
test-l10n
composer ✅ 173/173
npm ✅ 713/713
PHPUnit
Newman
Playwright
Hydra gates

Quality workflow — 2026-08-05 21:06 UTC

Download the full PDF report from the workflow artifacts.

…ed guard

Three gates, three real findings:

phpmd — deadEndFindings() reached cyclomatic 13 / NPath 735, and decomposing it
pushed FlowNodePreflight past the 1000-line class limit. Both are the same
signal: the graph-SHAPE question does not belong in a class that answers
questions about each node's TYPE and CONFIG. Moved to FlowConnectivity, which
also stops the preflight becoming the place every future flow check lands.
Instantiated inline rather than injected, so no constructor changes ripple into
the several tests that build the preflight by hand.

gate-7 no-admin-idor — FlowController::create/update were pulled into the diff
by the savedBody() change and flagged as NoAdminRequired with no guard. The
guard is real but DELEGATED, which is the gate's documented false-positive
class: update() resolves the uuid through FlowService::find(), so an update to a
flow the caller cannot see is refused exactly like one that does not exist, and
create() stamps owner/organisation server-side with both outside
applyEditableFields()'s allowlist. Recorded with the reason-bearing
@no-admin-idor-exempt tag naming the actual guard, following the precedent in
EmailsController and FileSearchController.

PHPUnit — one more single-node fixture asserting an exactly-empty report, now
marked exit: true for the same reason as the others: a lone node with no
outgoing edge IS a dead end, and the warning would be right.
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ 95bfb10

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-specs
test-l10n
composer ✅ 173/173
npm ✅ 713/713
PHPUnit
Newman
Playwright
Hydra gates

Quality workflow — 2026-08-05 21:31 UTC

Download the full PDF report from the workflow artifacts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant