fix: second round of review fixes for the development → beta promotion #2643 by WilcoLouwerse · Pull Request #2692 · ConductionNL/integriq

WilcoLouwerse · 2026-10-16T13:13:36Z

Second round of fixes for the reviews of #2643 (Wilco's and Robert's), next to #2686. It is cut from development and carries only what can land without #2686. It merges with #2686 in either order without a conflict (git merge-tree against review/pr-2643-fixes), although both touch StufZdsOutbound, StufProviderReceiver, StufZdsConfirmationReceiver and BodyCapturePolicy.

Fixed here

Thread on #2643	Commit	What changed
🟡 A 5xx with a non-XML or DOCTYPE body is a permanent failure, never retried	`7fcd04fce`	`PARSE_ERROR` is not a StUF fault, so the HTTP status decides: an HTML 503 page or a plain-text 502 is retried.
🟡 A message that was sent can be reported as refused, or stay in `retrying` with no conclusion	`7fcd04fce`, `5bb7bb500`, `6ca75e4d7`	After the case system answered, a failed row save is logged and the outcome stands (`concludeRow()`), also for a question. Every circuit-breaker call goes through `breakerCall()`, so its app-config reads and writes cannot change the outcome; `allows()` fails open. A row that cannot be marked `retrying`, or a retry job that cannot be scheduled (also after a cooldown), concludes the message as failed (`RETRY_NOT_SCHEDULED`, the last attempt's code in `details`) and announces it. A failed conclusion save leaves the envelope on the `retrying` row until #2686's expiry sweep removes it.
🟡 A zakLk01 W cannot clear a field, yet it is answered with a Bv03	`3c83bcf43`, `4a9e42d87`, `55a227d1f`, `c72c979ea`	An element of the new object sent empty (`xsi:nil` / `StUF:noValue`) counts as a clear when the old object holds a value at the same path, relations and nested values included. Such a W is refused with a Fo03 (StUF058) before the owning app is asked. Empty elements the old object did not hold pass, because senders, integriq's own translators among them, write every empty optional element that way. A W with a single object has nothing to compare with, so it is refused when it carries an empty element. Both objects are walked once, so 5000 empty elements cost milliseconds.
🟢 The owning app's refusal text goes to the external caller unchanged	`3c83bcf43`, `2041a16a9`, `79c3aff46`, `912271de5`	The reason leaves as one line of valid UTF-8, at most 200 columns wide, with control characters replaced, and the log carries the same line. A fixed text per kind would drop reasons the case system needs ("Zaaktype is not mapped"), which `refuse()` documents as safe to return.
🟢 A second confirmation with a different code flips the row and concludes the message again	`e1eaaa421`, `9bebaf442`, `d604b2e47`	The first asynchronous confirmation concludes the message; a later one with another code is acknowledged and logged. The requirement says so, with a scenario.
🟡 The new BSN → target-schema map in app config only ever grows (part)	`abe445c3b`	Both roster maps are stored lazy and sensitive, and read as lazy values.
🟢 Product analytics read call_log with RBAC on and silently return zero (in the review body, no thread)	`1b3687161`	`fetchProductCallLogRows()` reads with `_rbac: false, _multitenancy: false`; only aggregates leave the controller.
🟢 A `bodyCaptureUntil` written any other way than `BodyCaptureController` is honoured however far away it is	`1fc33d25b`, `80d820869`, `4bde41249`	`isOpen()` also requires the end to lie within `maxHours()` of the call; a window ending exactly at the maximum is open. Three tests that opened a window ending in 2099 or 2999 now open one an hour ahead.

ae6acc445, 6b269d840 and 131a57740 wrap lines phpcs flagged and keep StufZdsOutbound under the phpmd class-complexity and coupling limits, on this branch and in the merged result with #2686.

Not in this PR

🟡 The creeerZaak/actualiseerZaak integriq sends is not the shape integriq's own receiver reads. Aligning the builder changes the messages sent to external case systems. Whether ZDS 1.2 updates a case with a zakLk01 old/new pair rather than a zakLk02 has to be checked against the ZDS 1.2 XSD first, and fix(review): #2643 review findings, round 1 #2686 rewrites StufZdsMessageBuilder (the envelope without a Security header). This follows once fix(review): #2643 review findings, round 1 #2686 is merged, with a builder → mapper round-trip test.
🟡 The release tree runs on core's Symfony 6.4, which no CI job tests. This needs a choice: lower the root constraints to what core ships (^6.4 || ^7.4), or add a CI job that installs --no-dev and smoke-tests on NC32.
The rest of the BSN roster thread: no code calls a provider's unsubscribe(), so "call remove() on unsubscribe" needs an unsubscribe flow first. The read-modify-write race and moving the map into an OpenRegister object are design calls.
🟡 iq-f5: BRP relation read has no caller authorization: changes the by-name contract dossiq calls, so it lands together with a dossiq change.
🟢 sourceApp on a ZDS case command is a free-text label: needs a new source field (configuration.owningApp).
From fix(review): #2643 review findings, round 1 #2686's own list: the remaining aud-f3 items (separate inbound secrets, throttle, persisted audit row), the iq-f3 Microsoft 365 binding, the iq-f6 residue, and bounding the legacy HMAC bridge's body (StufZknRequestBody exists only on fix(review): #2643 review findings, round 1 #2686).

Checks run on this branch

bash hydra/scripts/run-hydra-gates.sh --scope-to-diff --base origin/development → exit 0, COVERAGE 46 of 46 applicable gates ran (51 not applicable).
vendor/bin/phpcs, phpstan analyse and phpmd (phpmd.xml and the hydra-gates unused-params rules) on every changed lib/ file → clean.
vendor/bin/phpunit -c phpunit-unit.xml (full suite): 5531 tests, 11 errors and 4 failures. The 11 errors are the tests/Unit/AppInfo/Application*Test container errors fix(review): #2643 review findings, round 1 #2686 records. Two are integration tests (EndoflifeDateLiveSyncTest, which needs the live endpoint, and ExecutionTraceIntegrationTest); they fail the same way with development's lib/. The other two were caused by the window maximum and are fixed in 80d820869.
Every new test was confirmed to fail with its fix reverted.
phpmd on StufZdsOutbound, StufProviderReceiver, StufZdsConfirmationReceiver and StufProviderOperationMapper as they are in the merged tree with fix(review): #2643 review findings, round 1 #2686 → clean.
A review of this PR (Strict) and three pre-push adversarial checks of the fixes; their remaining 🟢 notes are inline.
Not run: psalm, which crashes in this environment while loading the autoloader files; CI runs it.
Not run: the test suite on the merged result of this branch and fix(review): #2643 review findings, round 1 #2686 (only git merge-tree was checked); eslint and vitest (no JS change).

🤖 Generated with Claude Code

…ite never undoes a send isTransient() counted any classified fout as a StUF fault, and a body that does not parse is classified as PARSE_ERROR, so a load balancer's HTML 503 page or a plain-text 502 ended the message as permanently failed on the first attempt. PARSE_ERROR is no StUF fault: the HTTP status decides. After the case system answered, a row save that threw reached the listener's catch, which refused a zaak that now existed; a retry by the caller would create a second one. The conclusion is now written through concludeRow(), which logs a failed save and keeps the outcome. A row that could not be marked retrying, or a retry job that could not be scheduled, left the message 'retrying' with no job behind it and the owning app waiting forever. Both now conclude the message as failed and announce it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…reason out as one bounded line A zakLk01 W clears a value with an empty element carrying xsi:nil or StUF:noValue. The mapper dropped every empty value, so the owning app never saw the clear, kept the old value, and integriq answered Bv03. The mapper now names the cleared elements in the operation's 'clears' and the receiver refuses such a W with a Fo03 (StUF058) before the owning app is asked. Carrying clears to the owning app is a change of the operation contract and is left to the author. refuse() asks for a reason safe to return, but nothing enforced it; an app passing an exception message sent internal errors to the remote party. The reason now leaves as one line of at most 200 characters, and the full text is logged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…cting later one is logged Only an identical repeat was skipped, so a Fo03 arriving after a Bv03 for the same crossRefnummer (or the reverse) flipped the row and dispatched a second StufZdsCaseConcludedEvent with the opposite outcome. The first confirmation now stands; a later one with another code is acknowledged and logged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

… sensitive Both roster maps in app config are keyed by a followed BSN or KvK number and were stored non-lazy and non-sensitive: Nextcloud loaded them on every request and occ config:list printed them. They are now written lazy and sensitive and read as lazy values, which also finds a value an older release stored eagerly. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…analytics Since call_log became admin-only, the product analytics read ran with RBAC on, so a delegated integriq administrator, who passes #[AuthorizedAdminSetting], was shown zero traffic for every product. The rows are read with RBAC and multitenancy off, like the product lookup before it; only aggregates leave the controller. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…ts as closed REQ-OCD-008 limits an investigation window to the instance maximum, but only BodyCaptureController::open() checked it. A bodyCaptureUntil written through the generic source API, a source update or an import opened a window of any length without a reason. isOpen() now also requires the end to lie within maxHours() of the call. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…instance maximum Three tests opened a window ending in 2099 or 2999. With a window beyond the 72-hour maximum now counting as closed, two of them failed and the third no longer proved its point (no bodies even inside an open window). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

github-actions · 2026-10-16T13:20:21Z

Quality Report — ConductionNL/integriq @ `1a22403`

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					✅ 5510/5510 · 1 skipped
Newman					✅ 130/130
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-16 13:20 UTC

Download the full PDF report from the workflow artifacts.

The clear check flagged every element of the new object that carried noValue or xsi:nil, nested relations included. Senders that write each empty optional element that way, integriq's own translators among them, had every W refused. An element now counts as cleared only when it is a direct child of the new object and the old object holds a value for it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…outcome The breaker's reset and recordFailure write app config and could still throw after the case system answered: the listener then refused a zaak that existed, or a failed attempt left the row retrying with no job. Both calls now go through breakerRecords(), which logs a failed write. A message concluded as RETRY_NOT_SCHEDULED keeps the last attempt's fout code in its details. Tests cover the breaker failures, an answered question whose row cannot be saved, and a cooldown that cannot be rescheduled. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…; scrub invalid UTF-8 The full refusal text went into a warning log line, where free text from the owning app can carry identifiers. The log now gets the same bounded line the case system receives. Invalid UTF-8 made preg_replace() return null, so the reason was lost; it is scrubbed first. A receive() test proves the Fo03 carries the bounded line. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

The requirement said each new confirmation dispatches the conclusion event; the receiver now announces only the first and logs a later one with another code. The requirement says so and names why. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…ute beyond is closed Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…; phpcs line length The breaker guard takes the breaker call as a callable instead of branching on a flag, and RETRY_NOT_SCHEDULED keeps the last attempt's fout code in details as is, so the class stays at the phpmd complexity threshold. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

One branch fewer keeps StufProviderReceiver under the phpmd class complexity limit once #2686's query checks land beside it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…bject; refuse clears in a W without one The previous fix compared only direct children by their text, so a cleared relation (heeftAlsInitiator) or a cleared value inside one (inp.bsn) did not count and was answered Bv03. Every nil element of the new object is now compared with the old object at the same path of local names. A W that carries a single object has nothing to compare with, so its empty elements count as clears and it is refused. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…not stranded retry() called the breaker's allows() unguarded. Once a cooldown has passed it resets the breaker, an app-config write; when that threw, the retry job had already been consumed and the row stayed retrying with no job and no conclusion. All breaker calls now go through breakerCall(), which logs the failure and returns a fallback; allows() fails open. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…hronous confirmation The requirement claimed the owning app never receives two contradicting conclusions, but the synchronous answer at send concludes the row too and the first asynchronous confirmation still overrides it. The text now says so, and a scenario covers a contradicting later confirmation. The test points at this requirement instead of the retry one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…wide characters Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…s as a coupling Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

The clear check ran an ancestor lookup and a path lookup in the old object for every empty element, so a W with thousands of them took seconds per request (5000 took about 6 s), from any source holding the token. Both objects are now walked once, collecting the paths that hold a value and those sent empty, and the clears are their intersection. Tests cover an old relation that held nothing and a W with 5000 empty elements. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

WilcoLouwerse · 2026-10-16T14:08:44Z

+	 *
+	 * @spec openspec/changes/stuf-provider-writes/specs/stuf-provider-receiver/spec.md#requirement-a-refused-write-is-a-stuf-fault-with-a-refusing-status
+	 */
+	public function testARefusedWriteSendsTheRefusalOutAsOneLine(): void {


🟢 The log line of a refusal is not asserted

The receiver now logs the same bounded line it sends ('reason' => self::outwardReason(...)). This test asserts only the Fo03 body, so reverting the log context to the raw $refusal would stay green.

Fix: assert the logger's warning context. #2686 adds a $this->logger property to this test on the lines that build the receiver, so this lands after #2686 to avoid a conflict.

Verification: grep -n "createMock(LoggerInterface::class)" tests/Unit/Service/StufZkn/StufProviderReceiverTest.php → the receiver gets an anonymous mock.

WilcoLouwerse · 2026-10-16T14:08:44Z

+				'StUF-ZDS message {id} was concluded (sent: {sent}) but its row could not be saved: {error}',
+				['id' => (string)($row['id'] ?? ''), 'sent' => json_encode($sent), 'error' => $exception->getMessage()]
+			);
+			return $row;


🟢 A failed conclusion save leaves the envelope on a retrying row until #2686 sweeps it

When log->conclude() throws, the stored row stays retrying with its envelope, which holds personal data such as a BSN; conclude() is what erases it. On this branch alone nothing removes it later. #2686 adds a 24-hour expires to rows that keep an envelope, and LogCleanUpTask sweeps expired retrying rows, so the two PRs together close this.

Fix: none needed once #2686 is merged; until then this is a dependency, now named in the description.

Verification: git show origin/review/pr-2643-fixes:lib/Service/StufZkn/StufZdsMessageLog.php | grep -n expires → #2686 sets and clears it.

WilcoLouwerse · 2026-10-16T14:08:45Z

+		$found  = $message->xpath('./*[local-name()="object"]');
+		$this->walk(element: $object, path: '', filled: $filled, empty: $empty);
+		if (is_array($found) === false || count($found) < 2) {
+			return array_keys($empty);


🟢 A W with a single object now refuses every empty element

With no old object to compare with, every noValue/nil element counts as a clear and the W gets a Fo03. That fails closed, but a sender that sends a W as one object and writes every empty optional element as nil will see every such update refused.

Fix: none in this PR. If a real counterparty sends that shape, the author decides whether to accept it (and drop the nils) or keep refusing.

Verification: testClearingARelationANestedValueOrWithoutAnOldObjectIsRefused, case no old object → 400, StUF058.

WilcoLouwerse

Verdict: APPROVE (Strict) — posted as a comment, because GitHub refuses a review verdict on one's own PR

No blockers left. The review (Strict) found one 🔴 and five 🟡; three pre-push checks then found three 🔴 and three 🟡 in the fixes. All of them are fixed on this branch:

The W-clear check refused ordinary updates, and then missed relation and nested clears. Then it was quadratic in the number of empty elements.
Circuit-breaker calls, allows() included, could still change a message's outcome.
The spec contradicted first-confirmation-wins.
The refusal text was logged unbounded.
Several claims had no test.

Three notes remain, inline.

Connected issues

Issue	State	What this PR does
#2684	open	Unaffected: the stored WS-Security password is fixed in #2686.
#2685	open	Unaffected: the retention sweep is fixed in #2686.

🟢 Minor (3)

The log line of a refusal is not asserted: this follows #2686, which adds the logger property to that test.
A failed conclusion save leaves the envelope on a retrying row until #2686 sweeps it: a dependency on #2686, now named in the description.
A W with a single object now refuses every empty element: fails closed; the author decides if a real sender needs that shape accepted.

Heads-up, already there before this PR and outside its diff:

StUFXMLBuilder::buildFo03 sets faultstring->nodeValue, so an & in a refusal text leaves an empty faultstring.
A SOAP 1.1 fault without a StUF <code> is retried on its HTTP status.

✅ Verified clean

analytics() reads call_log with RBAC and multitenancy off behind #[AuthorizedAdminSetting], and only aggregates leave the controller.
Nextcloud 32 and 35 accept turning an existing key lazy and sensitive, and read an eager value with lazy: true; checked in AppConfig::setTypedValue / getTypedValue.
The body-capture ceiling never closes a window BodyCaptureController::open() created.
phpmd on the four StufZkn classes, on this branch and in the merged tree with #2686, stays under the class-complexity and coupling limits.
The merge with #2686 is clean (git merge-tree); 155 StufZkn and body-capture tests pass on the merged tree.

Persistence audit: not run (declined; the diff adds no grants, accounts, secrets or trust). CI: green on 80d820869; the fix commits' run was still in progress when this was posted. Not run: psalm (crashes locally), the full suite on the merged tree.

🤖 Generated with Claude Code

github-actions · 2026-10-16T14:14:02Z

Quality Report — ConductionNL/integriq @ `018ae5e`

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					✅ 5521/5521 · 1 skipped
Newman					✅ 130/130
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-16 14:13 UTC

Download the full PDF report from the workflow artifacts.

WilcoLouwerse and others added 8 commits October 16, 2026 14:24

style(stuf): wrap long lines and drop an inline if flagged by phpcs

ae6acc4

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

WilcoLouwerse requested review from bbrands02 and rjzondervan as code owners October 16, 2026 13:13

WilcoLouwerse mentioned this pull request Oct 16, 2026

fix(review): #2643 review findings, round 1 #2686

Merged

WilcoLouwerse and others added 13 commits October 16, 2026 15:39

test(call-log): a window ending exactly at the maximum is open, a min…

4bde412

…ute beyond is closed Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

refactor(stuf-provider): truncate the outward refusal with mb_strimwidth

79c3aff

One branch fewer keeps StufProviderReceiver under the phpmd class complexity limit once #2686's query checks land beside it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

test(stuf-provider): pin the column width of the outward refusal for …

912271d

…wide characters Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

refactor(stuf-zds): no mixed type on breakerCall, which pdepend count…

131a577

…s as a coupling Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

WilcoLouwerse requested review from Rem-Dam and rubenvdlinde as code owners October 16, 2026 14:07

WilcoLouwerse commented Oct 16, 2026

View reviewed changes

WilcoLouwerse merged commit dafbc43 into development Oct 16, 2026
47 checks passed

WilcoLouwerse deleted the review/pr-2643-fixes-round2 branch October 16, 2026 14:15

WilcoLouwerse mentioned this pull request Oct 16, 2026

fix: third round of review fixes for the beta promotion (#2643) #2694

Merged

rjzondervan mentioned this pull request Oct 16, 2026

Release: merge development into beta #2643

Merged

Navigation Menu

Repository navigation

fix: second round of review fixes for the development → beta promotion #2643 - #2692

fix: second round of review fixes for the development → beta promotion #2643#2692
WilcoLouwerse merged 21 commits into
developmentfrom
review/pr-2643-fixes-round2

WilcoLouwerse commented Oct 16, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Oct 16, 2026

Uh oh!

WilcoLouwerse Oct 16, 2026

Uh oh!

WilcoLouwerse Oct 16, 2026

Uh oh!

WilcoLouwerse Oct 16, 2026

Uh oh!

WilcoLouwerse left a comment

Uh oh!

github-actions Bot commented Oct 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

WilcoLouwerse commented Oct 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fixed here

Not in this PR

Checks run on this branch

Uh oh!

github-actions Bot commented Oct 16, 2026

Quality Report — ConductionNL/integriq @ 1a22403

Uh oh!

WilcoLouwerse Oct 16, 2026

Choose a reason for hiding this comment

Uh oh!

WilcoLouwerse Oct 16, 2026

Choose a reason for hiding this comment

Uh oh!

WilcoLouwerse Oct 16, 2026

Choose a reason for hiding this comment

Uh oh!

WilcoLouwerse left a comment

Choose a reason for hiding this comment

Verdict: APPROVE (Strict) — posted as a comment, because GitHub refuses a review verdict on one's own PR

Connected issues

🟢 Minor (3)

✅ Verified clean

Uh oh!

github-actions Bot commented Oct 16, 2026

Quality Report — ConductionNL/integriq @ 018ae5e

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

WilcoLouwerse commented Oct 16, 2026 •

edited

Loading

Quality Report — ConductionNL/integriq @ `1a22403`

Quality Report — ConductionNL/integriq @ `018ae5e`