fix(review): #4580 review findings, round 2 by WilcoLouwerse · Pull Request #4669 · ConductionNL/openregister

WilcoLouwerse · 2026-10-16T15:14:27Z

Summary

Second fix round for the review findings on the release PR #4580 (development → beta). This branch contains every commit of #4655 (round 1, which it replaces) plus the fixes for rjzondervan's review of 2026-10-16 and the items round 1 left for the author. development is merged in, because the tree endpoint (#4656), the BRP count (#4657) and the bulk writer (#4661) that several findings are about landed after round 1 was cut.

Of the 86 unresolved threads on #4580, 20 were already fixed by round 1 (verified in code), 60 get a fix here, and 6 need a decision; for a few fixed threads a remaining part needs one too (all listed below). Every fix commit names its thread in the body.

Drafts, forms and permissions

The draft rule is in PermissionHandler::hasPermission() when an object is passed: somebody else's draft is refused for every action, as the read predicate already did. Admins, _rbac: false, SystemOperationContext and the CLI pass; the owner passes; a schema with draftsVisible passes. This closes bulk delete (Bulk delete ignores every draft rule: others' drafts deleted, soft-deleted, no draft.removed #4665), the readable audit trail (/api/audit-trails/readable shows other users' draft contents #4666) and bulk save in one place; the explicit guards in bulk delete and SaveObjects stay.
A form payload can no longer choose the uuid it writes (A form payload's id picks the uuid, and a scoped draft save then writes over any draft with RBAC off #4664, form path; see "Left open").
Concurrent leave-draft announces once; drafts beyond the 5,000-read cap are counted as deferred; a draft save claims its upload tokens; a required x-openregister-generated id no longer blocks submit or activation; draft sweep counts log at warning level; activation answers the rendered object; orphan upload .bin files are purged; @self.version is ignored when RBAC applies; RESTRICT by key also blocks a scalar reference on MySQL/MariaDB.

Object tree, bulk writer, files, tasks

Tree fields go through the same redaction as every read (writeOnly, property rules, encryption); tree(), graph() and exportGraph() enforce the doelbinding (PurposeGuard); an access-check failure answers truncatedBy: unreadable; the node cap bounds the queries, not only the answer; the topmost node names no unreadable parent.
A schema property named like a metadata column (_owner, _organisation, …) is not written by the bulk or the single-object writer.
Object-folder endpoints refuse writes under a frozen or archived object (409); an aborted file delete is reported by moveFile, merge and batch delete; the frozen-node listener resolves MagicMapper only when it needs it.
A blocked portal task is not listed, counted, read or completed until its blocker closes; blockedBy accepts only a readable task.
Grant changes and owning-group changes leave audit rows (grant_added, grant_revoked, owner_group_changed).
A schema written by the configuration import, the import endpoint or the AI and MCP tools re-checks its published forms, not only the schema API.
History entries of a reader no longer name another object through cascadeContext; OAS reads an unresolved roles block like enforcement does; the BRP resident count reports connector-cannot-withhold-bodies against an older integriq; referenceKey: uuid reads the same in the validator and the service.

Case plans, flows, decisions, example sets, configuration

A statutory expiry timer that could not be armed is marked and re-armed by CasePlanTimerRearmJob (every 5 minutes); items ensured as active arm their timers.
Advance-a-case moves the live realisation; a case step about another object is not anchored to the wrong register/schema; system-created plans queue their run without a user; direct-invoke caller config is limited to the node's keys.
An email recipient reference must name an object of the schema its property's $ref points at (upgrade note in the CHANGELOG).
The duplicate decision-table key check fails closed; a hidden decision no longer shows through anonymized / anonymizedValue; bandRate validates the schedule at any unit; an empty property no longer fills a deep-link placeholder; #[McpTool] annotations may not carry MCP's reserved hint keys.
The admin panel's re-import asks first (a dialog in src/dialogs/settings/) and lists the authorization blocks it took from the file; OpenRegister's own descriptors re-import under their install-time configuration id; a pack import that replaced a pack answers 200 and the page says "Replaced"; pack editor messages go through the translations; shipped seed updates compare with the version last shipped.
Example sets: lookups tell "absent" from "failed"; remove keeps an organisation somebody joined, a changed path a user edited, and an app-config key an administrator replaced; a change or lookup that matches more than one object is refused; a reinstall keeps a user's edit of a reference; a register whose import was removed is imported again on the next install; the install record is lazy app config; the set claims exactly the import job the import reports (also for mock registers).

CHANGELOG

The upgrade-visible changes of this release are listed under the unreleased section.

Left open (needs a decision)

/api/forms/validate reads schemas across organisations (FormsController:126): recommended to keep, since the submit path must resolve with multitenancy off anyway; a one-line switch if other organisations' schema definitions must stay private.
The 403 on somebody else's draft is a draft-id oracle (ObjectService:2345): the spec prescribes the 403 and a 404 would not close the oracle; recommended to record the exposure as accepted.
Personal data in drafts and data-subject requests (ObjectEventGate:86): needs a second, draft-aware discovery pass in DataSubjectRequestService and a decision on redacting a removed draft's audit values; partly legal.
A hidden draft blocks a RESTRICT delete (ReferentialIntegrityService:1121): recommended that drafts take no part in RESTRICT, CASCADE or SET_NULL (one WHERE clause), provided leaving draft validates reference existence.
The object folder node itself (FrozenNodeWriteListener:223): checking it as suggested would orphan the files of archived objects being destroyed; recommended to refuse rename/move of the folder and exempt delete through a destruction context.
Metadata-named schema properties: now skipped on write (the value reads back empty); refusing them at schema save would break existing schemas, so that needs a scan first.
An oversized deferred entry or webhook payload is still dropped and logged once oldData is removed; storing the payload separately is a design change.
Decision tables resolve across organisations (DecisionTableStore:158): recommended to treat the register as one instance-wide catalogue and say so in REQ-SDT-01; per-organisation tables are a data-model change.
Also not done: the objects half of "remove deletes an organisation in use" (OpenRegister has no cross-schema count by organisation), and the generic object API's create branch for a uuid the caller cannot read (A form payload's id picks the uuid, and a scoped draft save then writes over any draft with RBAC off #4664's second paragraph; only the form path is closed here).

Test plan

composer phpcs, phpmd (both rule sets with the baseline, on a clean git archive export), composer phpstan: clean
scripts/run-hydra-gates.sh --scope-to-diff --base origin/development: 67 of 67 applicable gates green
node tests/l10n/check-l10n-parity.js: every locale at parity
phpunit -c phpunit-unit-local.xml tests/Unit: no new failures against the same run on the base (the fix branch with development merged in); the failures both runs share need pdo_sqlite or a Nextcloud server
jest on the changed settings and migration-pack views; ESLint (Node 22) on every changed frontend file
Database and Nextcloud tests, Newman and the MySQL/MariaDB paths of referential integrity and draft expiry: CI

🤖 Generated with Claude Code

…isfied by an upload token validateAll() now excuses a property that carries an upload in the plan's files ('uploaded'), because mapPayload() moves the file out of the validated object. A required file property with a valid token is accepted; without a token it still answers 422 required. Thread: #4580 (comment) Sweep: saveDraft/promoteDraft/submit all go through validateAll, so one place. DraftActivation does not see form uploads. Check: phpunit tests/Unit/Service/Form/FormSubmitServiceTest.php -> OK 26 tests (new test errors without the fix); phpcs on the lib file clean. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…hen it cannot be read An unreadable schema now yields a decider-owned policy that also hides others, and a throwing file-to-object lookup in subjectOf() yields the same closed policy instead of the open instance default (also covers the surface slice's duplicate of this finding). Thread: #4580 (comment) Sweep: both failure branches of ReviewPolicyResolver now share closedPolicy(); no other resolver builds a fail-closed review policy. Check: phpunit tests/Unit/Service/Decision -> OK 17 tests; phpcs on the lib file clean. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

applyBulkSafeguards() now asks DraftStatusPolicy::keptFrom() for an existing row when RBAC is enforced and rejects the row with the same 'Permission denied' reason a refused update gives, so a draft stays unannounced. The owner's bulk save still updates it. Tests in Wave12BulkSafeguardsTest (the test fails without the fix). Thread: #4580 (comment) Sweep: keptFrom had one caller (ObjectService::refuseSomebodyElsesDraft); the bulk safeguard is the only other path that resolves an existing row past RBAC. Check: phpunit Wave12BulkSafeguardsTest + BulkSafeguardSchemaResolutionTest OK; phpcs on the lib file clean. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…rough a share A node outside the openregister home (a sharee's view: /alice/files/<share>/x.pdf) is now resolved by file id: only a node owned by the app account is looked up, in the app's home via IRootFolder::getById, and the existing UUID-ancestor + folder proof runs on that twin. A node that does not exist yet stands for its nearest existing ancestor. A home that cannot be searched fails closed with a log line. New constructor argument: IRootFolder (autowired). Thread: #4580 (comment) Sweep: the listener is the only path-prefix owner test (grep for '/files/' prefix checks in lib/ finds no other frozen-file gate). Check: phpunit tests/Unit/Listener/FrozenNodeWriteListenerTest.php OK 21 tests; phpcs on the lib file clean. The Newman PUT-as-sharee case from Done-when was not run (no live Nextcloud). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…he instance review default updateFileSettingsOnly() writes anonymisationReview only when the request carries the key; without it the stored declaration is kept, and an explicit null still clears it. The test that asserted null after an empty save now asserts the kept value, plus the explicit-null clear. The Vue form is left alone: the server keeps the stored value, so the form need not send it. Thread: #4580 (comment) Sweep: ReviewPolicy::SETTINGS_KEY is written nowhere else (ReviewPolicyResolver only reads it). Check: phpunit tests/Unit/Service/Settings/FileSettingsHandlerTest.php OK 20 tests; phpcs on the lib file clean. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

… and 33 accept 4000 The chunk budget now follows the running Nextcloud: ListenerDeferralService::getArgumentLimit() reads OC\BackgroundJob\JobList::MAX_ARGUMENT_JSON_LENGTH when the server defines it (34 and later: 32000) and uses 4000 otherwise, which is what NC 32 and 33 enforce. Picking by constant rather than a fixed 4000 keeps 35 at full chunks; the 4000 fallback is the safe side everywhere. The limit can be injected through the constructor (optional, autowired as null). An entry that alone exceeds the limit is enqueued without its oldData and the reduction logged at warning level (the 'updated' notification is still sent, only calculatedChange is skipped); an entry still too large is dropped and named as before. MAX_ARGUMENT_LENGTH is replaced by ARGUMENT_LENGTH_NC32/NC34 (no other user). DeferredChunksFitTheJobLimitTest runs at 4000 and at 32000 and covers the reduction; the 'too large for any job' test now uses an entry whose size is not oldData. Thread: #4580 (comment) Sweep: the openspec change deferred-listener-batches-stay-under-the-job-limit (proposal + spec requirement) restated 32,000 and is updated to the runtime limit and the reduction. Check: phpunit tests/Unit/Service/Deferral + ObjectCleanupJobTest OK 34 tests; phpcs on the lib file clean. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

FormsController::upload() applies submit()'s audience rule (shared audienceRefuses()/signInRequired() helpers): an anonymous visitor of a form that is not public gets 401 before any rule lookup or write. FormUploadStore also gets a 20 MiB default cap for a file property that declares no maxSize (surface slice's duplicate of this finding); a declared limit, larger or smaller, is kept. Thread: #4580 (comment) Sweep: FormUploadStore::issue has one caller (FormsController::upload). Check: phpunit FormsControllerTest + FormUploadStoreTest OK 27 tests; phpcs on the lib files clean. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…nor race-safe The stored answer is now bound to the submitter (user id, scope value or anonymous marker) and a canonical hash of the payload: another subject, or the same key with another payload, gets a 409 that carries nothing of the first answer. A key is reserved with an exclusive ILockingProvider lock for the length of its submit (acquire, find, write, remember, release in a finally), so a parallel retry answers 409 and creates no second object; the lock is released after a refusal too. FormIdempotencyStore gets ILockingProvider and IL10N (autowired), find()/remember() take the binding, and an entry without a binding (older than this change, at most 24 h) matches nobody. Tests use a small in-memory MemoryLockingProvider. docs/features/form-destination.md gets the 409 row and the binding sentence (same sentence as the finding's). Thread: #4580 (comment) Sweep: FormIdempotencyStore has one caller (FormSubmitService::submitAll) plus the purge job, which is unchanged; the idempotency header is read only in FormsController::submit. Check: phpunit tests/Unit/Service/Form + FormUploadPurgeJobTest + FormsControllerTest OK 106 tests; phpcs on the two lib files clean. The two-concurrent-requests Done-when is covered through a simulated parallel request inside the save, not a real parallel run. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

… the draft remove() now reads the object again (ObjectService::find, RBAC off, no render, no audit read) and deletes only while it is still a draft last changed before the cutoff the list was made with; a draft sent or touched since is counted as refused and logged at info level, an object that cannot be read again is left alone. The cutoff is passed through walk() and removeWithin(). DraftExpiryTest gets a find() fake and a test with a draft sent after the list. Thread: #4580 (comment) Sweep: DraftExpiry::remove has one caller; DraftExpiryJob and DraftsExpiredCommand only call sweep()/preview(). DraftExpiryJobTest gets the same find() fake (it builds DraftExpiry itself). Check: phpunit tests/Unit/BackgroundJob + DraftsExpiredCommandTest + DraftExpiryTest OK 338 tests; phpcs on the lib file clean. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…nothing was unpublished unpublish() answers 'unpublish-unavailable' (and logs a warning) for a FormDependent registered without a closure, instead of falling through to notify the author and answer 'unpublished'. The outcome is already listed per form in the schema save response ('affectedForms'), so the save surfaces it. FormDependentsCheckTest covers the null closure. Thread: #4580 (comment) Sweep: FormDependent::$unpublish is read only in FormDependentsCheck::unpublish. Check: phpunit tests/Unit/Service/Form/FormDependentsCheckTest.php OK 5 tests; phpcs on the lib file clean. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…ma write path only (logging part) formDependentsCheck() now logs at warning level when the container cannot build the check (it used to return null in silence, which skipped an owning app's refuse setting without a trace), and the lookup happens only for an update that carries properties or required. Test: testAFailingDependentFormCheckIsLogged. Not done here: running the check on the other schema write paths (configuration importer, SchemaImportController, SchemaTool, SchemasToolProvider, SchemaService), because what 'refuse' means for a write that has no response to refuse is the author's call. See the hand-back note. Thread: #4580 (comment) Check: phpunit tests/Unit/Controller/SchemasControllerTest.php OK 83 tests; phpcs on the lib file clean. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…tion the file lacks Under an administrator's decision only the authorization roots the incoming definition carries are decided; a block only the instance holds keeps its normal state (local is preserved and reported) instead of settling as UPSTREAM and being dropped, which left the schema default-open. New GuardedDescriptorMergeTest case under a ShippedAuthorizationDecision; the replace-when-shipped-has-it cases stay green (the test fails with the old roots($live, $incoming)). Thread: #4580 (comment) Sweep: AuthorizationBlocks::roots() has this one caller. Check: phpunit tests/Unit/Service/ShippedBaseline + RegisterDescriptorServiceTest OK 63 tests; phpcs on the lib file clean. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…tatus MagicRbacHandler now takes the MagicMetadataColumnHealer (built first in MagicMapper::initializeHandlers) and an optional bare table name; reachableRowSqlFor() leaves the (_status IS NULL OR _status <> 'draft') term out when the table is known to lack _status, so a non-admin on a degraded table no longer fails with an unknown-column error. The table name is threaded through applyRbacFilters, buildRbacConditionsSql, buildRbacRowPredicateSql, buildRbacPredicateForAlias and buildRbacSqlTerms (all optional; a caller that names no table keeps the predicate, which is the narrower and safe answer) and passed by every caller that knows it: MagicSearchHandler (list, UNION conditions, single-object reads), the three MagicMapper single-object and scan sites, RelatedRowQueryApplier (the related table) and AggregationRunner (via MagicMapper::rbacRowPredicateSql(register:)). New test: testADegradedTableGetsNoDraftPredicate (missing: no _status, owner admit kept; healthy table and unnamed table: predicate kept). Thread: #4580 (comment) Check: phpunit tests/Unit/Db tests/Unit/Service/Aggregation tests/Unit/Service/Query: 2407 tests, 49 errors, all 'PDOException: could not find driver' or 'OC\DB\Schema\Table is not loadable' (no database driver / Nextcloud server classes in this environment), none from the touched code; MagicRbacDraftVisibilityTest OK; phpcs on the touched lib files: no errors. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…list hides The native aggregation path adds DraftListLens's clause for the mode a query-less list uses (EXCLUDE, or NONE for a schema with draftsListed: false) next to the archive predicate: others' drafts are out of the count, the signed-in caller's own stay in through a bound owner placeholder (kept in step with the organisation bindings that follow), and the clause is left out of a table known to lack _status. This also holds for an administrator, whose RBAC predicate is null. New tests assert the generated WHERE and bindings for nobody, a user, draftsListed false and a degraded table. Thread: #4580 (comment) Check: phpunit tests/Unit/Service/Aggregation: 276 tests, 21 errors ('PDOException: could not find driver', identical at HEAD) and no other failure; phpcs on the lib file clean. The 'count over a table with one draft and one active row returns 1' Done-when needs a database and was not run (SQL-level assertions stand in for it). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…earlier refusal DecisionChangeRequestedEvent keeps a refused flag: once refuse() was called, allow() does nothing, so the registration order of two apps' listeners cannot turn a veto into an allow. testAnAllowClearsTheReason is replaced by a sticky-refusal test covering both orders; the supervisor-may-be-refused and listener-may-allow cases are untouched and green. Thread: #4580 (comment) Sweep: DecisionVisibility's event is deny-by-default and allow-only (order independent); no other vetoable event in lib/Event has an allow() that clears a refusal. Check: phpunit tests/Unit/Service/Decision + tests/Unit/Event OK 245 tests; phpcs on the lib file clean. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

… remote principals GrantManageGuard::letsItselfIn() takes the remote principal being named and refuses it outright for a holder admitted by the special right alone (a federated cloud id or remote group cannot be proved not to be the caller); ObjectSharingService passes remote and remote_group grantees. The selfGrants data provider of GrantManageSpecialRightTest gains a remote and a remote_group case. The owner and the administrator are unaffected. Thread: #4580 (comment) Sweep: letsItselfIn has two callers; ObjectOwnershipService names only a group (remote is not applicable there). Check: phpunit tests/Unit/Service/Rbac/GrantManageSpecialRightTest.php OK 17 tests; phpcs on the two lib files clean. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…on unguarded When the guard throws, the import still finishes but now writes the incoming definition with the instance's own authorization blocks put back (the schema's, and each property the instance already holds; a block the instance lacks is not added, a property the instance does not hold brings its shipped block), and logs it. Authorization blocks the merge took from the app because only the app changed them are now recorded on the audit trail (configuration.conflict.decided with decision applied-shipped, once per block root, skipping roots already recorded for an explicit decision); recordDecision() gets a decision argument for it. New tests: a throwing baseline read keeps the instance authorization, and an app-only authorization change is audited. Thread: verdict #4580 (review) (this finding has no inline thread) Check: phpunit tests/Unit/Service/ShippedBaseline + RegisterDescriptorServiceTest OK; phpcs on the lib file clean. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…ersonal settings form The relink notification now links to the OpenRegister app page with #user-settings (as the OAuth callback does) and says 'user settings'; the admin hint in PushNotificationsConfiguration.vue, its en/nl l10n keys (nl: gebruikersinstellingen), the credential-oauth2-connect requirement heading and text, the notificatie-engine requirement sentence, and the stale header comments in BrowserNotificationsSection.vue and OAuth2Endpoints.php are reworded to the user settings dialog. Other languages' l10n entries keep the old key until Transifex syncs. New NotifierTest case asserts the link. Thread: #4580 (comment) Sweep: git grep -i 'personal settings' over lib, src and openspec/specs leaves only comments that say OpenRegister is NOT in Nextcloud's personal settings (CredentialOauth2Controller, App.vue, UserSettings.vue). Check: phpunit tests/Unit/Notification/NotifierTest.php OK 22 tests; l10n/en.json and nl.json parse. The anchored code is spec text and strings only, no eslint run was needed beyond the one-line template change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…ory redaction ObjectsController::logs() answers a non-admin through ObjectHistoryForReaders::page() (the same read check, doelbinding guard and redaction as the audit-trails route: 404 for an object the caller may not read, no session, request or ipAddress, no property the caller may not see); an administrator keeps the stored history as before. The page keeps the results/total/page/pages/limit/offset shape; the legacy next/prev links are not produced on the reader path. The requirement in audit-trail-immutable/spec.md now names both URLs. The 17 existing testLogs* cases of ObjectsControllerTest, which exercise the stored-history path, now run as an administrator. New ObjectsControllerLogsRedactionTest covers delegation, the 404 and the administrator path; the redaction itself is covered by AuditTrailObjectHistoryForReadersTest. Thread: #4580 (comment) Check: phpunit tests/Unit/Controller/ObjectsControllerTest.php + ObjectsControllerLogsRedactionTest.php OK 279 tests; tests/Unit/Controller overall: only 'could not find driver' errors (no database driver here); phpcs on the lib file clean. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…er's signal FlowRunService::signal() merges the new payload over the signal still waiting on the suspended run (array_merge: keys of both survive, a key both set takes the later value) instead of assigning it outright, so the FlowTaskBridge outcome bag and another listener's payload no longer depend on listener order. A consumed signal is still removed by persistResult(), so nothing from an earlier question reaches the merge. FlowRunServiceTest gets a two-signal case. Thread: #4580 (comment) Sweep: signal() is the only writer of the signal context key (RegistryStepDispatcher only reads and unsets it). Check: phpunit tests/Unit/Service/Flow OK 1525 tests; phpcs on the lib file clean. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…o cases decide() now treats a relation that names a decision whose record cannot be found as owned by nobody but a supervisor (a synthetic decision with no decider goes through DecisionOwnership::refusal, so a supervisor, an administrator or the owning app's listener still gets through and a reviewer is refused), and links the new decision with a compare-and-swap: EntityRelationMapper::linkDecision() gains expectedUuid and guarded arguments, and a guarded link writes only while the stored link is still the one read, otherwise it throws DecisionConflictException, which decide() rolls back and answers as decision-owned. Two reviewers taking a first decision at once can no longer both link theirs. New cases in DecisionOwnershipTest: dangling link refused for a reviewer and allowed for a supervisor, and a lost link race refused. Thread: #4580 (comment) Check: phpunit tests/Unit/Controller/DecisionOwnershipTest + EntityRelationsControllerTest + tests/Unit/Service/Decision + EntityRelationMapperUpdateDecisionMetadataTest OK 57 tests; phpcs on the three lib files clean. The guarded SQL update itself needs a database and was not run here. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…is only logged CasePlanItemTimers::arm() now logs at error level and rethrows when the timer of a statutory (wettelijk) deadline is refused, so onEntered() raises it instead of letting the item sit active past a legal term with nothing to end it; advisory and service-norm timers stay a warning and a 0. New test: a statutory arm failure is raised, a service-norm one is not. Note for the author: CaseItemTimerListener::handle() calls onEntered() from an event listener, and Nextcloud's event dispatcher logs and swallows a listener's exception, so whether the transition itself fails depends on the caller of the transition; a visible marker on the item would need a schema field and is not added here. Thread: #4580 (comment) Check: phpunit tests/Unit/Service/Case + tests/Unit/Listener OK 608 tests; phpcs on the lib file clean. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…s to internal callers (delete part) DeleteFileHandler::deleteFile() reads the node's id before delete() and asks the root folder for it afterwards; a node that is still there (Nextcloud ignores a listener's abortOperation() through the Node API, as the frozen-object guard uses) is logged and reported false instead of true, so FileService::deleteFile() callers such as the example-set remover and the merge no longer believe a file is gone that is not. New test: delete() that leaves the node behind answers false. The rename/move handlers are not changed here (left to the author, see the hand-back note), and the ExampleSetRemoveLiveTest case from Done-when was not added. Thread: #4580 (comment) Check: phpunit tests/Unit/Service/File OK 489 tests; phpcs on the lib file clean. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

The spec-coverage gate flagged the new release() method; it carries the same @SPEC as reserve() and the rest of the store. Thread: #4580 (comment) Check: bash scripts/run-hydra-gates.sh --scope-to-diff --base b9e5d8d -> re-run after this commit (see hand-back). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…er path CasePlanStateMachine::flushEvents() catches Throwable around every dispatchTyped after the transition is committed, so the rethrow changed only a log line. It is reverted: arm() returns false again, and the error-level log now says the item is active WITHOUT its statutory expiry timer, naming the plan item, the case object and the legal effect. No schema field is added; a visible marker on the item is for the author. Thread: #4580 (comment) Check: phpunit tests/Unit/Service/Case OK 108 tests; phpcs on the lib file clean. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…other user's draft PermissionHandler::filterUuidsForPermissions() now asks DraftStatusPolicy::keptFrom() before the delete permission, so another user's draft is left out of a bulk delete like an object that does not exist. ObjectService::deleteObject() asks it too (RBAC on) after the object is found and before the handler runs, answering DoesNotExistException, as the save path does; the read lookup already hid it through the RBAC predicate, this is the explicit rule. Tests: bulk filter (fails at HEAD) and single delete. Thread: #4580 (comment) Check: phpunit PermissionHandlerBulkDeleteLookupTest, ObjectServiceBulkDeleteTest, ObjectServiceDraftDeleteTest and tests/Unit/Service/Object OK; phpcs on both lib files clean. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…nly check The draft guard (Step 2b) now runs above the appendOnly gate in applyBulkSafeguards(), so a non-owner of a draft on an appendOnly schema gets the same 'Permission denied' as everywhere else instead of an appendOnly message that confirms the uuid names an existing row. New test. Thread: #4580 (comment) Check: phpunit Wave12BulkSafeguardsTest + BulkSafeguardSchemaResolutionTest OK; phpcs on the lib file clean. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Dropping oldData for an entry that alone exceeds the job limit keeps the entry but skips the conditioned updated rules and the calculatedChange rules for that object (they fail closed without the previous data). The warning now says so, the docblock and comments no longer claim the notification is still sent, and the proposal, spec requirement and scenario of deferred-listener-batches-stay-under-the-job-limit are corrected; the test was renamed accordingly and asserts the log text. Thread: #4580 (comment) Check: phpunit tests/Unit/Service/Deferral OK; phpcs on the lib file clean. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…argument WebhookService::dispatchEvent() enqueues WebhookDeliveryJob with the whole event payload as the job argument, the same 4000 (NC 32 and 33) or 32000 (34 and later) limit. A refusal from IJobList::add() was uncaught: it aborted the webhooks after it in the loop and surfaced as an exception in the event listener, where it is only a log line. Each add is now guarded: the refusal is logged at error level with the webhook id, event, payload length and message, and the loop goes on. That delivery is still lost (no redesign of webhook delivery; trimming or a payload reference is the author's call). Test: a refused first webhook does not stop the second. Thread: #4580 (comment) Check: phpunit WebhookServiceTest, WebhookAsyncDispatchTest and WebhookEventListenerTest OK; phpcs on the lib file clean. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…w key behaviour The idempotency scenario in form-destination-validator now says same subject and same payload; two new scenarios state the 409 for another subject or payload and for a parallel retry. docs/features/form-destination.md states that the parallel-retry protection relies on Nextcloud file locking (filelocking.enabled); without it only the sequential repeat is answered from the stored answer. Thread: #4580 (comment) Check: documentation only; no test run. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…nceKey: uuid the same way Thread r4293206152 (PropertyValidatorHandler:982). ReferentialIntegrityService treated an empty or 'uuid' referenceKey as a plain uuid reference while the validator refused any non-null referenceKey unless onDelete was RESTRICT, so 'referenceKey: uuid' with CASCADE was refused at save although the delete would have honoured it. One reader, Service\Object\ReferenceKey::of(), now serves both (an instance method, so no static-access exception is needed in phpmd.xml). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

… or completable in the portal Thread PRRT_kwDOMnn3ws6tJjz3 / r4293206162 (TaskMapper:592). applyBlockedExclusion() only ran for the inbox predicates, and an external task has no other inbox than the portal list. The open-blocker predicate is now one method (excludeBlocked) used by the inbox and by applyPartyPredicates(), so page and total of the portal list leave a blocked task out; PortalTaskService::show() and complete() answer a blocked task like one that does not exist (after the authorization, so only the matched party can tell, and before anything is stored); and the list tells TaskInboxService::row() that no row is blocked, so no row runs its own blocker query (design D-3). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…armed is marked and armed again; an ensured active item arms its timers Two ways an active plan item ended without the timer that enforces its statutory term. (1) CasePlanItemTimers::arm() ran from a listener whose exception Nextcloud swallows, so a refused arm of a `wettelijk` expiry could only be logged. It now also leaves a durable marker (UnarmedTimerMarkers: a map of item uuid to first-failure moment in a lazy app-config value, so no table of the case layer changes), and the new CasePlanTimerRearm pass, run every 300 s by CasePlanTimerRearmJob (registered in info.xml), arms the item again, terminates it as a fired statutory timer would when the term passed meanwhile, and drops the marker of an item that is gone, no longer active or already has a running expiry timer. (2) CasePlanEnsurer inserts an item in the state it carries without the state machine, so no CaseItemTransitionedEvent reached the timer listener and an item ensured as active never got its due or expiry timer. After the commit it now calls CasePlanItemTimers::onEntered() for each item it created as active (the verb stays idempotent: an item that already existed is not touched); a refused statutory arm is marked like any other. flow-cases spec, tests for the pass, the job, the marker store and the ensure path added. Threads: discussion r4284660540 (CasePlanItemTimers.php:219, left for the author in round 1) and r4293205644 (CasePlanEnsurer.php:347) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

… not queued as the `system:<app>` label createPlanAsSystem() evaluates the plan as `system:<app>`, and CaseRealisationService::realise() passed that actor as the user of the flow run it queues for a bound stage or process task. FlowRunAttribution returns a non-null caller as the run's user, so the run's runAs became `system:dossiq`, which names no account, and FlowRunAsScope refused every scoped step of the run. realise() now queues such a run with user null, so the flow's own attribution (or its declared runAs) decides; a real caller is passed through unchanged, and a timer-expiry enforcement (actor null) is unaffected. The prefix becomes CasePlanService::SYSTEM_ACTOR_PREFIX. Test: CaseRealisationServiceTest asserts the queue call carries a null user for `system:dossiq`; CasePlanServiceTest mocks the realiser, so the assertion sits where the call is made. Thread: discussion r4293205655 (lib/Service/Case/CasePlanService.php:191) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…it of zero or less too RateSchedule::rateAt() returned null for a unit <= 0 before normalise() ran, so a malformed schedule (empty, not a list, falling bounds, an open band first) was not refused at unit 0 or below, while graduated refuses it at any quantity because accrued() normalises first; REQ-GRC-03 says such a schedule must raise an evaluation error and never be read as an amount. The schedule is now checked before the unit is. GraduatedRateTest runs bandRate at unit 0 over every malformed schedule of the existing data provider (red against the old order). Thread: discussion r4293205670 (lib/Service/Calculation/RateSchedule.php:137) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…eep-link placeholder with nothing ObjectSearchResultFormatter fell back to OpenRegister's own page only when a literal `{name}` survived the registry's substitution, but ownLinkProperties() passed every scalar, so `case: ''` (and false, which casts to '') filled `{case}` with an empty string and `/apps/dossiq/cases/{case}` became the broken `/apps/dossiq/cases/`, with no brace left to notice. ownLinkProperties() now leaves out empty and blank strings and booleans, so the placeholder stays unfilled and the hit falls back. The formatter test runs the real DeepLinkRegistration substitution over '', blank, false and true (red against the old code). Spec updated. Thread: discussion r4293205681 (lib/Service/Search/ObjectSearchResultFormatter.php:196) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…the grant_manage list leave audit-trail rows Verdict item without a thread (aud-p3, persistence audit, #4601; 'left to the author' in #4655). A grant_manage holder could widen access to a record and an administrator could change who holds the right with nothing on the trail, while an ownership handover was recorded. GrantChangeAuditor writes one typed entry through AuditTrailMapper::createAuditTrailEntry for a grant (grant_added), a revoke (grant_revoked) and a change of the owning group (owner_group_changed), naming the principal, permissions, and whether the caller was admitted as owner or as holder; a change of the right's list is written as a settings.updated row through SettingsChangeAuditor. Fail-soft: a missing row is logged at error and does not undo the stored change. Called from ObjectSharingService, ObjectOwnershipService::setOwnerGroup and OrganisationController::update (optional trailing constructor argument, autowired). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…hecked refuses the save, and a lookup never picks one of two tables DecisionTableValidationListener::keyTakenBy() answered "no clash" when the search for an existing key threw, so the save went through with a duplicate key; and isDecisionTable()/inDecisionTableRegister() answered "not a decision table" when the schema or register lookup threw, skipping validation entirely. A key that cannot be checked now refuses the save (decision-table-invalid, "the key could not be checked"); a schema or register that cannot be READ refuses it too, while one that does not exist (DoesNotExistException) still leaves the object alone. DecisionTableStore::lookup() collected the first row with the key from an unordered result: it now raises when more than one row carries the key, so a flow never decides by a table nobody chose and the answer cannot change between requests. A save path that dispatches no ObjectCreatingEvent (bulk) still bypasses the guard; the lookup refusal is the backstop for it. Spec REQ-SDT-02 and REQ-SDT-03 updated. Thread: discussion r4293205687 (lib/Listener/DecisionTableValidationListener.php:205) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…ugh anonymized / anonymizedValue DecisionVisibility::MASKED_FIELDS blanked bases, skipAnonymization and decisionRecord, but once an anonymise pass has run, `anonymized` and `anonymizedValue` on the same relation read the verdict off again (a released occurrence stays anonymized:false, a redacted one is true with its placeholder). Both are masked now, so a blind second review after a first pass cannot read the first reviewer's verdict from them. REQ-ROD-002 and HiddenDecisionTest updated. Thread: discussion r4293205723 (lib/Service/Decision/DecisionVisibility.php:59) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…a API re-checks its dependent forms Thread PRRT_kwDOMnn3ws6s1TX6 (SchemasController:1281; the part #4655 left to the author, recommendation in its hand-back note). Every schema write ends in SchemaMapper::update(), which dispatches SchemaUpdatedEvent only when something changed. FormDestinationSchemaListener listens there: when properties or required changed it assesses the dependent forms and unpublishes the broken ones (an owning app's refuse setting has no response to refuse on these paths, so it is logged as not honourable). The schema API judges the forms itself before the write, so SchemasController claims its own save on FormDependentsCheck (request-scoped) and the listener leaves it, so nothing is unpublished or announced twice. The importer path (an app upgrade shipping a new required property), SchemaImportController, SchemaTool and the MCP tools are covered by this one seam. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…e version last shipped, not the edited object's own A stored seed's own version moves with every save: shipped at 1.4.7 and edited once it reads 1.4.8, so the next shipped release at 1.4.8 compared equal (`version_compare('1.4.8', '1.4.8', '>')` is false), counted as unchanged and never arrived, while 1.5.0 replaced the edit; whether a shipped change reached an instance depended on how many times somebody saved the object. The import now remembers, apart from the object, the version it last shipped for each seed (SeedVersionLedger: one lazy app-config map per app id, keyed register id / schema id / slug) and compares the shipped version with that. A seed stored before the ledger existed is compared with its stored version once and remembered at what the file ships. The object keeps carrying the shipped version as before. Test: seed stored at 1.4.7, one user save (1.4.8), file ships 1.4.8, the shipped values arrive. Thread: discussion r4293205845 (lib/Service/Configuration/ImportHandler.php:3612) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

… router test helper Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…ine length, constructor docs) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…quest reader findings go through the app's translations PackDefinitionValidator and MappingEngine had no IL10N, so the pack editor (the save error lists the validator's findings, the preview panel shows the engine's row errors) gave a Dutch administrator a Dutch page with English findings, and so did the raw-body refusal in MigrationPackRequestReader. Both classes now take IL10N and every message goes through t() with the property names and values as parameters (the row label is part of its message: "Mapping row %s: ..."); the reader's id refusal is two translated sentences. 27 keys added to l10n/nl.json, as for the pack editor's other backend messages, which exist in nl.json only: adding them to en.json would put 27 keys into the parity gate of 34 further locales. MigrationPacksControllerTest::testEveryTranslatedMessageHasADutchTranslation now also scans the validator and the engine; validator and engine tests with a prefixing IL10N stub assert every returned message is translated. The ten test constructions pass a pass-through IL10N (PassThroughL10nTrait). Thread: discussion r4293205863 (lib/Service/MigrationPack/PackDefinitionValidator.php:205) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

… pack answers 200 and the page says "Replaced" import() knew whether it created or replaced a pack but answered 201 Created either way, so MigrationPacksIndex.vue always said `Imported "{name}".` and an administrator importing a file whose id already exists was never told that the stored pack, possibly edited on this instance, was overwritten. The controller now answers 201 for a created pack and 200 for a replaced one; importPack() returns the pack and whether it replaced one, and the index page says `Replaced "{name}".` (the new frontend key is translated in all 36 catalogues, none identical to English). Spec and the controller test updated; a jest case for the replaced notice was added but could not be run here (no node_modules in this worktree). Thread: discussion r4293205932 (lib/Controller/MigrationPacksController.php:293) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

The activation answer renders through ObjectService::renderEntity() instead of a new RenderObject constructor parameter (ExcessiveParameterList). DraftOwner::resolve() takes over FormSubmitService::ownerOf() and the payload key test becomes isPropertyKey() (ExcessiveClassLength, CyclomaticComplexity). FormUploadStore::purge() asks isOrphanedBytes(), DraftExpiry::walk() asks lifetimeOf() and the read cutoff is formatted without a DateTimeZone (CyclomaticComplexity, CouplingBetweenObjects). DraftStatusPolicy carries the repo's TooManyPublicMethods suppression with its reason, ReferentialIntegrityService imports RuntimeException, two lines are cut to 150 characters. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@SuppressWarnings

Complexity, length and parameter-count findings the new code raised: the call-signature probe is its own class (CallSignature) so the router stays under its length limit, the guard keeps the held-back rule (isHeldBack / assertNotHeldBack) so PortalTaskService stays under its complexity limit, the bounded level read is its own method in HierarchyDescender, TaskMapper keeps one blocked-exclusion method (criteria optional for the portal seam), ReferenceKey::keyOf has a name of three characters, and the bulk writer's property loop is shorter. @SuppressWarnings with a reason only where the repo already uses that pattern for the same finding (class complexity, coupling, constructor parameters). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…hows the authorization it took from the file The panel's Re-import button runs the app's whole real configuration, forced, under the administrator's decision to take shipped authorization (#4618): every schema's authorization block of that app becomes the file's, also where it was tightened on this instance, and for an app with a base document every register is re-imported, not only the clicked row. The response carries counts.authorizationReplaced and replaced, but RegisterDescriptors.vue ignored the body and showed a plain success. The button now opens a confirmation that says so, and the outcome note shows the count and each replaced block (schema: path) as a warning. Three new frontend keys (the dialog title, its text, the warning sentence) are translated in all 36 catalogues; the sr value for `Replaced "{name}".` is moved to the Latin script the rest of sr uses. REQ-RDA-012 updated. The new jest spec could not be run here (no node_modules in this worktree). Thread: discussion r4293205733 (lib/Controller/RegisterDescriptorController.php:152; the panel is src/views/settings/sections/RegisterDescriptors.vue) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…release Thread r4293206242 (CHANGELOG.md:37). Adds entries under Unreleased for the roles block that resolves to nothing (#4589), frozen and archived object files through WebDAV and the object-folder endpoints (#4549), the personal settings moved into the user settings dialog (#4587), the audit trail readable by anyone who may read the object (#4614), the draft rule on the bulk paths and the form identity (in general terms, fixed on the other slices of the review), the tree and graph reads, the bulk writer and metadata columns, audit rows for grants, the dependent-form re-check on every schema writer, the integriq requirement of the BRP resident count and the deferred-chunk limit; and the new API surface of the release under Added. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@SuppressWarnings

printReport() hands its outstanding lines to printOutstanding(), ExampleSetChanges::revert()/refresh() lose a branch and a short variable, and the three classes whose size is by design (CasePlanEnsurer, ExampleSetStore, RegisterDescriptorService::importDescriptorFile) carry a reasoned @SuppressWarnings in the repo's existing pattern. No baseline entry added. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…the reference property Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

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

The first merge of slice b took only its first commit; the rest of its work was committed on a detached HEAD. Conflicts with development's mock import (#4618 follow-up): ExampleSetRegisters keeps importMock() and the shared recordedImport(), which now claims the job the import reports instead of comparing the jobs before and after; reimportMock() reports importJobId and importAppId like reimport(). REQ-EXS-009 carries both rules. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…@SPEC on the new helpers Gate 13 (modal isolation) refuses an inline NcDialog in a settings section: the re-import confirmation moves to src/dialogs/settings/ReimportDescriptorDialog.vue. DescriptorConfigIds::forFile and ExampleSetStore::rowMatches get their @SPEC, and the reimportMock() return docblock fits the line limit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…ations MappingEngine takes IL10N since the round-2 l10n fix of the pack editor messages; the other MappingEngine tests already pass PassThroughL10nTrait. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

github-actions · 2026-10-16T15:31:00Z

Quality Report — ConductionNL/openregister @ `9aa5585`

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
check-specs		✅
test-l10n		✅
test-l10n-parity		✅
format		❌
check-schema-l10n		✅
composer			✅	✅ 173/173
npm			✅	✅ 648/648
app:check-code	⏭️
info.xml	✅
REUSE	✅
lockfile sync		✅
PHPUnit					❌ 26455/26455 · 25 skipped (no test failed, the job failed for another reason — see its log)
Newman					❌ 695/710 · 1 request/script errors
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					✅

Quality workflow — 2026-10-16 15:30 UTC

Download the full PDF report from the workflow artifacts.

WilcoLouwerse and others added 30 commits October 15, 2026 17:39

WilcoLouwerse and others added 28 commits October 16, 2026 15:03

test(review): #4580 integration — document the logger argument of the…

7f73f0b

… router test helper Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

fix(quality): phpcs findings in the review fixes (loose comparison, l…

659d812

…ine length, constructor docs) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

merge: review round 2, slice a

d9a9d71

merge: review round 2, slice b

77c6a8e

merge: review round 2, slice c

0656dad

docs(changelog): the referenced-address notification needs a $ref on …

857ce29

…the reference property Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

docs(spec): @SPEC on CallSignature::accepts and ReferenceKey::keyOf

9bc354f

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

test(import): give ImportServiceTest's MappingEngine the app's transl…

130f283

…ations MappingEngine takes IL10N since the round-2 l10n fix of the pack editor messages; the other MappingEngine tests already pass PassThroughL10nTrait. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

WilcoLouwerse mentioned this pull request Oct 16, 2026

fix(review): #4580 review findings, round 1 #4655

Closed

Navigation Menu

Repository navigation

fix(review): #4580 review findings, round 2 - #4669

fix(review): #4580 review findings, round 2#4669
WilcoLouwerse wants to merge 104 commits into
developmentfrom
review/pr-4580-fixes-r2

WilcoLouwerse commented Oct 16, 2026

Uh oh!

github-actions Bot commented Oct 16, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

WilcoLouwerse commented Oct 16, 2026

Summary

Drafts, forms and permissions

Object tree, bulk writer, files, tasks

Case plans, flows, decisions, example sets, configuration

CHANGELOG

Left open (needs a decision)

Test plan

Uh oh!

github-actions Bot commented Oct 16, 2026

Quality Report — ConductionNL/openregister @ 9aa5585

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Quality Report — ConductionNL/openregister @ `9aa5585`