fix(flow): ContractSweepNode is a legitimate end node, and report per-node outcomes by bbrands02 · Pull Request #2718 · ConductionNL/integriq

bbrands02 · 2026-10-22T10:07:57Z

Summary

A flow ending in a ContractSweepNode had its generator-appended trailing openregister.end node, which the engine read as a deliberate FlowStop and reported the run as "stopped" instead of "completed". ContractSweepNode now implements IFlowEndNode to declare itself a legitimate dead end for connectivity purposes, and SynchronizationFlowGenerator no longer appends the now-redundant end node.
Adds FlowStepReport instrumentation to SourcePaginateNode (objects/pages fetched), ContractMatchNode (create/update/skip/invalid tally) and ContractSweepNode (swept count), so a run's per-node summary in the Logboek/Logs tab shows real counts instead of nothing.

Test plan

ContractMatchNodeTest, ContractSweepNodeTest, SourcePaginateNodeTest, SynchronizationFlowGeneratorTest all green (100 tests)

🤖 Generated with Claude Code

…-node outcomes A flow ending in a ContractSweepNode previously had its generator-appended trailing openregister.end node, which the engine read as a deliberate FlowStop and reported the run as "stopped" rather than "completed". ContractSweepNode now implements IFlowEndNode to declare itself a legitimate dead end for connectivity purposes, and SynchronizationFlowGenerator no longer appends the now-redundant end node. Also adds FlowStepReport instrumentation to SourcePaginateNode (objects/pages fetched), ContractMatchNode (create/update/skip/invalid tally) and ContractSweepNode (swept count), so a run's per-node summary in the Logboek/ Logs tab shows real counts instead of nothing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…ort rendering The frontend half of the previous commit's report instrumentation: this app's library version predated CnRunDetailSidebar's per-node report rendering entirely, so the new report blocks reached the API response but never rendered in the Logboek/Logs tab. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

github-actions · 2026-10-22T10:38:52Z

Quality Report — ConductionNL/integriq @ `2afaa67`

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
build		✅
check-specs		✅
test-l10n		✅
format		❌
check-schema-l10n		✅
check-l10n-js		✅
composer			✅	✅ 143/143
npm			✅	✅ 646/646
app:check-code	⏭️
info.xml	✅
REUSE	❌
lockfile sync		✅
PHPUnit					❌
Newman					❌
Playwright					⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets `development`, so the suite is asked once per promotion into `beta` and `main` rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into `development`.
Hydra gates					❌
Hydra gates (axe)					✅

Quality workflow — 2026-10-22 10:38 UTC

Download the full PDF report from the workflow artifacts.

github-actions · 2026-10-23T11:39:48Z

Quality Report — ConductionNL/integriq @ `2afaa67`

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
build		✅
check-specs		✅
test-l10n		✅
format		❌
check-schema-l10n		✅
check-l10n-js		✅
composer			✅	✅ 143/143
npm			✅	✅ 646/646
app:check-code	⏭️
info.xml	✅
REUSE	❌
lockfile sync		✅
PHPUnit					❌
Newman					❌
Playwright					⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets `development`, so the suite is asked once per promotion into `beta` and `main` rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into `development`.
Hydra gates					❌
Hydra gates (axe)					✅

Quality workflow — 2026-10-23 11:39 UTC

Download the full PDF report from the workflow artifacts.

… stubs Review feedback on #2718: [Investigated, no exploitable bug, fixed for consistency] ContractSweepNode wrote its report before constructing the final FlowItems::item() output. Confirmed neither summary() (pure array construction from already-resolved scalars) nor FlowItems::item() can throw, so there was no observable inconsistency today. Reordered anyway to match the convention established in openregister's ObjectWriteNode (report after output is fully built), so a future reader does not find two different conventions in sibling files. [Confirmed root cause of a real CI gate failure] hydra-gates' gate-46 (spec-anchor-existence) failed with "4 unresolved finding(s) ... from 2 distinct targets" -- an exact match for the 4 @SPEC tag occurrences (1 bare + 3 anchored) this PR's two new test stubs carried over verbatim from their real openregister counterparts, pointing at openspec/specs/flow-engine/spec.md, which openconnector does not ship. Removed the @SPEC tags from the stub copies with a comment explaining why (a stub is not the authoritative source whose spec compliance is being tracked); left everything else in the stubs untouched. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

bbrands02 · 2026-10-23T11:42:51Z

All five points investigated against the actual code; two led to real fixes, three were confirmed already correct. Pushed 8a177ff → 820e389.

1. [Medium] ContractSweepNode reports before output construction — investigated, no exploitable bug found, fixed anyway for consistency. Checked both summary() (pure array construction from already-resolved scalars — no exceptions possible) and FlowItems::item() (pure value constructor, same as verified for ObjectWriteNode in a sibling review). No throw path exists between the old writeReport() call and the return, so there was no observable inconsistency today. Reordered anyway to match the ObjectWriteNode convention (report after output is fully built) so sibling files don't disagree on the pattern for a future change to lean on.

2. [Medium] IFlowEndNode semantics — verified directly against openregister's actual code, not assumed. IFlowEndNode (lib/Service/Flow/IFlowEndNode.php:86-87) is a pure marker interface — zero methods, its own docblock says so explicitly. The only consumer is FlowNodeTaxonomyResolver::isEnd() → FlowConnectivity::isDeadEnd()/rolesPresent(), both preflight-only, non-blocking warnings. Confirmed zero references to IFlowEndNode anywhere in FlowEngine.php or FlowDefinitionBuilder.php — the runtime engine's edge-following logic is completely unaware the interface exists. Answering your five sub-questions directly: (1) yes, exempts from the dead-end warning only; (2) yes, a manually-wired outgoing edge on an IFlowEndNode node works exactly like any other edge; (3) yes, the engine follows it normally, with zero special-casing; (4) it's an "exempt from the warning" signal, not an "unconditional terminal" one; (5) no dedicated test exists for a manually-wired ContractSweepNode with an outgoing edge, but given the interface has zero runtime coupling (confirmed by the grep above), every other FlowEngine test that exercises edge-following already covers this implicitly. The implementation is correct as-is; no change made.

3. [Medium] SourcePaginateNode across suspend/resume — investigated in depth. Confirmed: FlowSuspension is deliberately uncaught in paginateForEachItem(), so on suspension the function aborts before writeReport() is ever reached — nothing is reported for that invocation, partial or otherwise. On resume, the engine re-delivers the entire original item batch to execute() (confirmed in FlowEngine.php, $itemsIn round-trips unchanged; the step's marking isn't advanced) — this is the same architecture SynchronizationRunNode already relies on for the identical rate-limit-to-suspension mechanism, and its correctness there rests on one-item-per-shard, not on accurate cross-suspension totals. Practically: when the node eventually completes without throwing, it has made exactly one full pass over all its input items in that final call, and the report it writes reflects that complete pass accurately. What it does NOT reflect is the total number of fetch attempts across multiple suspend/resume cycles if earlier items get re-fetched on resume — but that's an existing, accepted, engine-wide suspend/resume property (whole-step re-invocation, not partial resume), not something this reporting change introduces or could fix without redesigning suspension semantics broadly. No change made; flagging the re-fetch-on-resume behavior as a pre-existing architectural property worth knowing, not a defect of this PR.

4. [Low] ContractMatchNode outcome counters — verified exhaustive. Read decisionFor() fully: every code path returns exactly create, update, or skip (no contract → create; has contract → update, downgraded to skip if unchanged — no other return value exists). Combined with the caller's own invalid default for a missing origin id, that's a complete, 4-outcome space; the isset() guard is defensive but never actually needed. No early continue in the loop, so the sum of all counts always equals the processed-item count. And yes, confirmed: this node performs zero database writes anywhere in the file — it only decides; persistence happens downstream. No change needed.

5. [CI] Investigated all 5 listed failures:

Formatting: pre-existing on development, unrelated files (src/integration/leafReport.js, SyncedFromTab.vue, tests/vitest/leafReport.spec.js — none touched by this PR; reproduces on a clean checkout with npx prettier --check).
Hydra Gates: real, confirmed cause — gate-46 spec-anchor-existence failed on exactly 4 @spec tag occurrences across 2 distinct targets, matching this PR's two new test stubs verbatim-copying a dangling @spec openspec/specs/flow-engine/spec.md reference from openregister (a path openconnector doesn't ship). Fixed — see commit above.
PHPUnit / Newman: same root cause for both, and it's not pre-existing CI flakiness — confirmed by re-running the PHPUnit job a full day later and getting the identical failure: a PHP fatal Maximum call stack size ... Infinite recursion? inside openregister's own lib/AppInfo/Application.php (dependency-injection container), which aborts CI's "enable the openregister sibling app" bootstrap step entirely. This is a real, currently-live bug on openregister's development branch, unrelated to any file in this PR's diff, and it's blocking cross-repo CI for every app that depends on openregister as a sibling — worth raising with the openregister team directly, outside this PR's scope.
REUSE: currently shows passing on the latest check run — contradicts the review's listing; may have been a stale read.

Full relevant suites green: tests/Unit/Flow/ + SynchronizationFlowGeneratorTest (259 tests), phpcs clean, phpmd clean on the changed files.

WilcoLouwerse · 2026-10-23T12:01:29Z

+				// IFlowEndNode`, which satisfies the connectivity check on its
+				// own, so the run instead ends through the engine's normal "no
+				// transitions left" path and is reported `completed`.
 				'id' => 'sweep',


🟡 E2E spec still expects the trailing openregister.end node
tests/e2e/spec-coverage/flow-native-sync.spec.ts asserts the generated node list ends in 'openregister.end' (lines 862 and 1285), counts one openregister-end canvas node (line 991) and documents the chain with it (line 25); with this change those three assertions fail on the next Playwright run (the E2E job is skipped on this PR, so CI does not show it).
Drop the 'openregister.end' entries and the ['openregister-end', 1] row (the edges.length === types.length - 1 check keeps holding on its own).
Verification: grep -n "openregister.end\|openregister-end" tests/e2e/spec-coverage/flow-native-sync.spec.ts → lines 25, 862, 991, 1285; gh pr checks 2718 → E2E Tests (Playwright) skipping

🔧 Proposed fix in #2729, commits d977248 and 5759237: the spec no longer expects openregister.end or the trailing end transition, and the canvas comment counts ten nodes. The thread stays open until it is merged.

WilcoLouwerse · 2026-10-23T12:01:29Z

+			context: $context,
+			report: [
+				'objects' => ['count' => $totalObjects],
+				'pages' => ['count' => $totalPages],


🟢 Reported pages is the node's own chunking, not source pages fetched
fetch.pages.count is count(array_chunk($objects, $pageSize)), while the PR body calls it "pages fetched"; your own test shows the gap (fetchInfo.pagesFetched 2, reported pages 3), so a reader of the Logboek will take it for the source's page count.
Either report fetchInfo.pagesFetched as well (e.g. sourcePages), or name the bucket so it reads as emitted pages.
Verification: sed -n 340,362p tests/Unit/Flow/SourcePaginateNodeTest.php → pagesFetched => 2 in, pages => ['count' => 3] asserted

WilcoLouwerse · 2026-10-23T12:01:29Z

 					reference: $reference
 				);

+				$totalObjects += count($objects);


🟢 Failed fetches leave no trace in the hop report
Under onError: continue a FlowNodeException emits an error item and skips the counters, so a run whose every fetch failed reports objects 0 / pages 0, indistinguishable from an empty source.
Add an errors bucket incremented in the catch branch.
Verification: sed -n 400,452p lib/Flow/SourcePaginateNode.php → counters only in the try branch, catch emits errorItem() and continues

🔧 Proposed fix in #2729, commit f98297d: SourcePaginateNode reports an errors count, incremented once per failed fetch under onError: continue, with a unit test. The thread stays open until it is merged.

WilcoLouwerse · 2026-10-23T12:01:29Z

+ *
+ * @link https://OpenRegister.app
+ *
+ * Test stub: a verbatim copy of openregister's real class, kept here only so


🟢 Stubs called verbatim copies, but FlowStepReport is not
The real class on openregister development also has line(), MAX_LINES, MAX_MESSAGE_LENGTH, TRUNCATION_MARKER, LEVELS and a $dropped counter, and both stub headers name openconnector as the repository that does not ship the spec path; put(), take() and CONTEXT_KEY do match, so the tests are sound.
Say "subset of" instead of "verbatim copy", and change openconnector to integriq.
Verification: gh api 'repos/ConductionNL/openregister/contents/lib/Service/Flow/FlowStepReport.php?ref=development' --jq .content | base64 -d | grep -n 'public' → line() and four extra constants absent from the stub

🔧 Proposed fix in #2729, commit cb23c87: the FlowStepReport stub header calls it a subset, and both stub headers name integriq. The thread stays open until it is merged.

WilcoLouwerse · 2026-10-23T12:01:29Z

 				],
 			],
 			[
+				// The pipeline's own end — no trailing `openregister.end`. That


🟢 Already generated flows keep the trailing end node
The fix only applies to flows generated from now on; a flow already generated and saved still has openregister.end after the sweep and keeps reporting stopped.
Mention in the PR body that existing generated flows need the end node removed (or regenerated), or add a repair step.
Verification: grep -rn "openregister.end" lib/Migration lib/Repair 2>/dev/null → no repair step touches persisted flows

WilcoLouwerse · 2026-10-23T12:01:29Z

  "dependencies": {
    "@codemirror/lang-json": "^6.0.1",
-    "@conduction/nextcloud-vue": "^2.65.0",
+    "@conduction/nextcloud-vue": "^2.72.0",


🟢 Dependency bump not in the PR body; lock resolves 2.82.0
The @conduction/nextcloud-vue bump to ^2.72.0 is explained only in commit 8a177ff, and the lockfile resolves 2.82.0, ten minors beyond the stated floor, in a PR that touches no src/.
Add one line to the PR body on why the floor is 2.72.0 and that 2.82.0 is what ships.
Verification: grep -A1 '"node_modules/@conduction/nextcloud-vue"' package-lock.json → "version": "2.82.0" (lock deps match the registry, so npm ci is consistent)

github-actions · 2026-10-23T12:11:05Z

Quality Report — ConductionNL/integriq @ `ae0db18`

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
build		✅
check-specs		✅
test-l10n		✅
format		❌
check-schema-l10n		✅
check-l10n-js		✅
composer			✅	✅ 143/143
npm			❌	✅ 647/647
app:check-code	⏭️
info.xml	✅
REUSE	❌
lockfile sync		✅
PHPUnit					❌
Newman					❌
Playwright					⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets `development`, so the suite is asked once per promotion into `beta` and `main` rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into `development`.
Hydra gates					❌
Hydra gates (axe)					✅

Quality workflow — 2026-10-23 12:11 UTC

Download the full PDF report from the workflow artifacts.

WilcoLouwerse

Verdict: APPROVE (Standard · full)

The fix holds: the generated sync pipeline always ends in the sweep node, OpenRegister's connectivity check accepts it through IFlowEndNode (present on openregister development, beta and main), and each node writes its step report once, after its output, guarded when the report handle is absent. One concern before the next Playwright run: the e2e spec still asserts the removed end node.

🔧 Fix PR: #2729 — 3 of 6 findings, one commit each

Merge it into fix/flow-step-report-and-contractsweep-end-node, then re-request the review. The other findings stay on their threads.

🟡 Concerns (1)

E2E spec still expects the trailing openregister.end node — lib/Service/SynchronizationFlowGenerator.php:732. tests/e2e/spec-coverage/flow-native-sync.spec.ts lines 862, 991 and 1285 assert the node this PR stops generating; the E2E job is skipped on PRs, so the break shows only on the next Playwright run. (fix proposed in #2729)

🟢 Minor (5)

Reported pages is the node's own chunking, not source pages fetched — lib/Flow/SourcePaginateNode.php:459. The PR's own test feeds pagesFetched 2 and asserts pages 3.
Failed fetches leave no trace in the hop report — lib/Flow/SourcePaginateNode.php:412. Under onError: continue a run whose every fetch failed reports 0 / 0, like an empty source. (fix proposed in #2729)
Stubs called verbatim copies, but FlowStepReport is not — tests/stubs/OCA/OpenRegister/Service/Flow/FlowStepReport.php:28. The real class also has line() and four more constants, and both headers name openconnector for this repository. (fix proposed in #2729)
Already generated flows keep the trailing end node — lib/Service/SynchronizationFlowGenerator.php:724. Saved flows keep reporting stopped until regenerated; no repair step touches them.
Dependency bump not in the PR body; lock resolves 2.82.0 — package.json:50. The @conduction/nextcloud-vue floor ^2.72.0 is explained only in commit 8a177ff.

Gates: CI quality / Hydra Gates on 820e389: 85 of 85 applicable gates ran; the one failure, gate-105 duplicate-page-refresh, is identical on development and this PR touches no src/ — pre-existing, not this PR's finding.
CI: Frontend Check (format), PHPUnit (the PHP 8.3 jobs), Integration Tests (Newman) and Security (npm) are red on development as well (npm audit counts identical on both); none is introduced here. The E2E job did not run on this PR.
Scope decision: full first review, Standard, size band M (13 files, 475 lines); no connected issues.

SUCCESSFUL_STATUSES accepts `stopped`, so dropping the trailing `openregister.end` node (which threw FlowStop and turned every successful sync into a `stopped` run) was not pinned by the spec. The first run of the generated flow now asserts `completed` at its own call site; the shared driveRunToTerminal helper keeps accepting `stopped` for flows that end in a deliberate stop. Raised in #2729 (comment) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

bbrands02 requested review from WilcoLouwerse and rjzondervan as code owners October 22, 2026 10:07

bbrands02 requested review from SudoThijn and remko48 as code owners October 22, 2026 10:09

WilcoLouwerse reviewed Oct 23, 2026

View reviewed changes

WilcoLouwerse approved these changes Oct 23, 2026

View reviewed changes

WilcoLouwerse mentioned this pull request Oct 23, 2026

fix(review): #2718 review findings, round 1 #2729

Open

Conversation

bbrands02 commented Oct 22, 2026

Summary

Test plan

Uh oh!

github-actions Bot commented Oct 22, 2026

Quality Report — ConductionNL/integriq @ 2afaa67

Uh oh!

github-actions Bot commented Oct 23, 2026

Quality Report — ConductionNL/integriq @ 2afaa67

Uh oh!

bbrands02 commented Oct 23, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Oct 23, 2026

Quality Report — ConductionNL/integriq @ ae0db18

Uh oh!

WilcoLouwerse left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Verdict: APPROVE (Standard · full)

🔧 Fix PR: #2729 — 3 of 6 findings, one commit each

🟡 Concerns (1)

🟢 Minor (5)

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Quality Report — ConductionNL/integriq @ `2afaa67`

Quality Report — ConductionNL/integriq @ `2afaa67`

Quality Report — ConductionNL/integriq @ `ae0db18`

WilcoLouwerse left a comment •

edited

Loading