[WilcoLouwerse]

Fixes for the round-1 review of #2718 (#2718 (review)). Merge into fix/flow-step-report-and-contractsweep-end-node, then re-request the review; nothing here is merged by the reviewer.

Findings → commits

Finding	Commit	What changed
f1 🟡 E2E spec still expects the trailing openregister.end node	d977248	Removed the four openregister.end / openregister-end expectations and the trailing end transition in the run-log list from flow-native-sync.spec.ts.
f1 (pre-push)	5759237	The canvas comment's node count reads ten, matching the ten-node generated pipeline.
f1 (review of this PR)	7c02124	The first run of the generated flow asserts status completed; the shared driveRunToTerminal helper still accepts stopped.
f3 🟢 Failed fetches leave no trace in the hop report	f98297d	SourcePaginateNode reports an errors count, incremented once per failed fetch under onError: continue; new unit test, existing report assertions updated.
f4 🟢 Stubs called verbatim copies, but FlowStepReport is not	cb23c87	The FlowStepReport stub header says it is a subset (CONTEXT_KEY, put(), take()); both stub headers name integriq instead of openconnector. IFlowEndNode stays "verbatim" (identical apart from the omitted @spec tag).

Left to the author (threads stay open)

• 🟢 Reported pages is the node's own chunking, not source pages fetched — which count the Logboek should show is a product choice; the PR's own test shows the two differ (pagesFetched 2, pages 3).
• 🟢 Already generated flows keep the trailing end node — a repair step or a note on regenerating existing flows.
• 🟢 Dependency bump not in the PR body; lock resolves 2.82.0 — the PR description is yours to edit.

Checks run on this branch

• php -l on the four changed PHP files → no syntax errors
• vendor/bin/phpunit -c phpunit-unit.xml on SourcePaginateNodeTest (27), ContractMatchNodeTest (16), ContractSweepNodeTest (13), SynchronizationFlowGeneratorTest (45) → OK; with the f3 change reverted the new and the updated report test fail
• vendor/bin/phpcs --standard=phpcs.xml → clean on SourcePaginateNode.php and both stubs; SourcePaginateNodeTest.php carries the same 110 pre-existing errors before and after, none added
• npx eslint tests/e2e/spec-coverage/flow-native-sync.spec.ts (Node 22) → clean (also after 7c02124)
• run-hydra-gates.sh --scope-to-diff --base 820e389 → exit 0, 36 of 36 applicable gates ran
• Independent pre-push review of the whole diff → no findings after the 5759237 round
• not run: the Playwright e2e spec (no local E2E environment; the E2E job is skipped on PRs)
• not run: psalm (crashes on PHP 8.4 locally), phpstan per file (no files found in single-file mode); CI runs both

🤖 Generated with Claude Code

[bbrands02]

Code Review — Integriq PR #2729

The implementation looks correct overall. The previous findings concerning outdated E2E expectations, missing pagination error counts, and inaccurate test-stub documentation have been addressed.

I found no confirmed blocking issues in the changed code.

Two points are worth checking before merging.

1. [Low] E2E test still accepts stopped as successful

File: tests/e2e/spec-coverage/flow-native-sync.spec.ts

The original purpose of PR #2718 was to make generated synchronization flows finish as completed rather than stopped.

This PR correctly removes the trailing openregister.end node from the expected pipeline.

However, the E2E test still accepts stopped as a successful terminal status.

Please investigate whether the generated synchronization flow should now always finish with status completed.

If so, update the relevant assertion to require completed rather than accepting stopped.

This would ensure the test actually protects the original behavioral fix.

Do not change unrelated tests that intentionally support stopped executions.

2. [Low — Needs verification] Pagination errors under dead_letter

File: lib/Flow/SourcePaginateNode.php

The new errors counter correctly records failed fetches when onError is continue.

Please verify that the same reporting behavior applies when onError is dead_letter.

Specifically:

1. Confirm whether both policies execute the same catch branch.
2. Verify that every failed fetch contributes exactly one error.
3. Confirm that throw/rethrow behavior remains unchanged.
4. Ensure the report does not incorrectly claim a completed execution when an exception propagates.

If both policies already share the same implementation, no production changes are required.

Add regression coverage only if there is a realistic untested difference.

Verified improvements

The following changes look correct:

• Removed obsolete openregister.end expectations.
• Updated the generated pipeline node count to ten.
• Removed the obsolete end transition from run-log expectations.
• Added errors reporting to SourcePaginateNode.
• Added regression coverage for multiple failed fetches.
• Updated successful-fetch reporting assertions to include errors: 0.
• Corrected FlowStepReport stub documentation.
• Corrected repository references in the test-stub documentation.

Please preserve these changes.

Requested actions

1. Verify whether the E2E flow status should explicitly be completed.
2. Verify error counting under dead_letter.
3. Make changes only if the investigation identifies a real gap.
4. Run the relevant tests.
5. Report which findings required changes and which were already correct.

Avoid unnecessary refactoring.

Overall recommendation: Approve once these minor points are clarified. No confirmed blocking defects identified.

[WilcoLouwerse]

Thanks, both checked. (1) Changed in 7c02124: the first run of the generated flow now asserts completed; the shared driveRunToTerminal helper still accepts stopped for flows that end in a deliberate stop. (2) No change needed: dead_letter and continue share the one catch branch, every failed fetch increments errors exactly once, and stop rethrows before the counter and before writeReport, so no report claims a completed run. The existing dead_letter test covers that branch. The Playwright spec could not run locally; the new assertion runs on the next E2E run.