Skip to content

Release: merge development into beta - #4580

Merged
WilcoLouwerse merged 525 commits into
betafrom
development
Oct 20, 2026
Merged

WilcoLouwerse merged 525 commits into
betafrom
development

Conversation

@github-actions

@github-actions github-actions Bot commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

Automated PR to sync development changes to beta for beta release.

Merging this PR will trigger the beta release workflow.

Reminder: Add a major, minor, or patch label to this PR to control the version bump. Default is patch.

Changelist

Snapshot of development at 8e7bbd8 (16 Oct 2026) against beta at b996608: 46 PRs, 13 more bundled inside them. The chore(sync) PR that carries beta back into development is left out. PRs merged to development after this snapshot are not listed yet.

Mostly the case, draft and forms engine work (decisions 179 to 229), example sets and RBAC refinements, plus the migration-pack editor (#4602) that stackiq's CMDB import uses to show and edit its mapping packs.

Features (21)

Fixes (14)

Trains (bundled PRs) (2)

Dependencies and security (7)

Maintenance (CI, gates, specs, docs) (2)

rubenvdlinde and others added 21 commits October 14, 2026 21:47
…olds it

A listed object that fails is retried once after the rest of the import,
failures are named in failed.objects, and a listener refusal withholds the
content hash so the next run imports again. An update carrying a higher
@self.version takes it. An array search result no longer has jsonSerialize()
called on it.
… organisation

CI ran the draft collection after openregister-crud, which switches
multitenancy on. The owner could read their draft and their activation
answered 404: the controller looked the register up through the
organisation filter, and the register was the administrator's. The object
API looks registers and schemas up past that filter
(ObjectService::setRegister()); the activation now does the same, and the
draft's own read under RBAC and the organisation filter stays the
authorisation. Reproduced on a throwaway instance by running the collections
in CI's order; green after the fix.
…ts may set user preferences (#4632)

* fix(example-sets): a kept object follows the objects it references after a partial remove

A reinstall after a partial remove wrote the deleted themes again with new
ids, while the kept publications still pointed at the old ones. A kept
object now resolves its {{object:}} and {{lookup:}} fields again and writes
only the fields that differ, recorded as the change <app>/<key>#references
so the proof checks it and remove stays exact (REQ-EXS-023).

* feat(example-sets): a fragment may set its own app's preferences for the set's own accounts

userPreferences entries [{user, key, value}] are written with
IConfig::setUserValue for the app that ships the fragment, only for an
account the set created or adopted, never for an administrator and never
for a key that holds a secret. The old value is recorded and put back on
remove while it is still the set's value (REQ-EXS-024). Asked by dossiq
#3896 so the demo staff do not get the getting-started tour.

* style(example-sets): no inline ifs in the user preference steps, proof split for complexity
…e-shipped-version

fix(import): a shipped seed object reaches an instance that already holds it
…and-lifetime

feat(objects): a draft's owner makes it definitive, and a schema sets a draft lifetime (decision 216)
…ffects

feat(objects): a draft holds its creation side effects until it leaves draft (decision 227)
* feat(rbac): a special right to manage the grants on a record (decision 234)

An organisation's special right grant_manage lets its holder list, add and
revoke the grants on a record it does not own, and name the owning group.
It opens neither the record's contents nor its scope, links or invitations,
a holder may not let itself in, and only an administrator or the
organisation's owner may assign it. An entry can be narrowed to a register
and schemas.

* feat(rbac): give a special right in every organisation, for an app's upgrade step

* fix(rbac): count the page once, outside the loop condition

* docs(rbac): cite the change on grant() and revoke()
…choices

An input may now carry label and options ({value, label}) beside field and
required. publishedInputs() carries both through in every mode, so a provider
that knows the record can publish the choices that hold for it. Presentation
only: the allowlist and the writer's validation are unchanged, and an input
declaring neither key keeps the bare {field, required} shape.

First consumer: dossiq's closing move publishes the case type's result types,
which nc-vue's CnTransitionInputDialog renders as a picker.
…rs-choices

feat(lifecycle): a transition input can name its field and offer its choices
…gration-pack-editor

# Conflicts:
#	l10n/nl.json
…-editor

feat(migration-packs): view and edit migration packs in OpenRegister
…arvest

Task 2.4 is done: the authenticated harvest against real GitHub ran on the
WOO-585 rig through a brokered github credential (24 of 24 shards, 698
components on 2026-10-01, 902 on 2026-10-13).

The allow-rule requirement joins the main spec credential-broker, the
change moves to the archive, and the test's @SPEC tag points at the main
spec.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…s its token

The GitHub code search refusal scenario said the broker refuses with
'no allow-rule matches'. That string is the logged reason; the caller gets
the static 403 'Request not permitted'. Fixed in the main spec and in the
archived delta. Task 2.4 no longer says no token exists next to its
verification, and names the token the harvest used.

Closes review findings: refusal scenario promises a response text the broker
never returns; ticked task 2.4 still says no token exists yet.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Lock-only update (composer update symfony/yaml --with symfony/yaml:6.4.48
--no-install), staying on the 6.4 line the lock already used.

composer audit --locked: no security vulnerability advisories found.

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

fix(deps): symfony/yaml 6.4.48 clears CVE-2026-109413 (Hydra gate 4)
…b-changes

chore(openspec): archive github-provider-code-search after the live harvest
Comment thread lib/Service/Object/DraftStatusPolicy.php
Comment thread lib/Db/MagicMapper/MagicRbacHandler.php
Comment thread lib/Service/ShippedBaseline/GuardedDescriptorMerge.php
Comment thread lib/Service/Object/DraftListLens.php
Comment thread lib/Service/Object/ObjectVersionHandler.php
Comment thread lib/Mcp/AttributeToolScanner.php
Comment thread lib/Controller/AuditTrailController.php
Comment thread lib/Controller/CredentialOauth2Controller.php
Comment thread lib/Controller/SchemasController.php
Comment thread lib/Service/Rbac/GrantManageGuard.php Outdated

@WilcoLouwerse WilcoLouwerse left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: REQUEST_CHANGES (Strict)

Not ready for beta: six blockers in code that reached development without any review, most in the new forms, drafts, frozen-file and deferral work. Under Strict every 🟡 below blocks as well.

🔧 Fix PR: #4655 — 23 of 33 findings, one commit each

Merge it into development, then re-request the review. The other findings stay on their threads.

🔴 Blockers (6)

🟡 Concerns (19)

  • RBAC draft predicate ignores unhealed _status — lib/Db/MagicMapper/MagicRbacHandler.php:359 (introduced in #4566, #4610). The self-healing change promises that a table whose metadata columns cannot be added "degrades quietly": MagicSearchHandler leaves the lifecycle predicates out for the columns MagicMetadataColumnHealer::missingFor() reports (REQ-MTH-002). (fix proposed in #4655)
  • Forced re-import deletes local authorization the file lacks — lib/Service/ShippedBaseline/GuardedDescriptorMerge.php:165 (introduced in #4618). With a decision in effect, line 165 adds every authorization root found in live OR incoming to the decisions, so AuthorizationBlocks::settle() turns the block's state into UPSTREAM; foldUpstream() with the block absent from incoming then drops it fro (fix proposed in #4655)
  • Aggregation fast path counts drafts the list hides — lib/Service/Object/DraftListLens.php:175 (introduced in #4566, #4600). AggregationRunner builds its own WHERE (_deleted, _archived predicates, with a comment saying it must reproduce the list path's defaults) and takes the access rule from MagicMapper::rbacRowPredicateSql(). (fix proposed in #4655)
  • Upload endpoint ignores the form's audience — lib/Controller/FormsController.php:274 (introduced in #4563). submit() answers 401 when the visitor is anonymous and the form's audience is not public, but upload() (also #[PublicPage]) never reads $form['audience']. (fix proposed in #4655)
  • Idempotency key is neither subject-bound nor race-safe — lib/Service/Form/FormSubmitService.php:268 (introduced in #4563). The key scope is the form id only, so on a public form anyone who presents the same Idempotency-Key within 24 hours gets the first submitter's stored answer (reference, id, and every confirmation property, which can be personal data). (fix proposed in #4655)
  • Draft cleanup deletes without re-checking the draft — lib/Service/Object/DraftExpiry.php:368 (introduced in #4600). expiredIn() reads every page of expired draft uuids first; remove() then deletes them one by one (up to 1000 per run) with _rbac: false and no re-read. (fix proposed in #4655)
  • Dependent form reported unpublished when nothing was unpublished — lib/Service/Form/FormDependentsCheck.php:158 (introduced in #4563). unpublish() calls the closure only when $form->unpublish !== null and otherwise falls through to notify the author and return unpublished. (fix proposed in #4655)
  • Task wake still overwrites another listener's signal — lib/Service/Flow/FlowTaskBridge.php:369 (introduced in #4541). The docblock says the signal now carries the outcome bag "so it cannot erase what another listener on the same event signalled". (fix proposed in #4655)
  • Decision ownership check is skipped in two cases — lib/Service/Decision/RelationDecisionService.php:106 (introduced in #4595). decide() reads $current and runs the ownership refusal only when $current !== null, outside the transaction. (fix proposed in #4655)
  • Failure to arm a statutory expiry timer is only logged — lib/Service/Case/CasePlanItemTimers.php:219 (introduced in #4544). arm() catches every Throwable from FlowTimerService::arm and returns false, and onEntered() just counts. (fix proposed in #4655)
  • Email recipient reference may point at any object in the organisation — lib/Service/Notification/ReferencedAddressNotifier.php:308 (introduced in #4559). findReferenced() reads the referenced object with _rbac: false and _multitenancy: false by id only; sameTenant() then requires equal organisation, and passes for any object with no organisation.
  • Aborted delete or rename reads as success to internal callers — lib/Listener/FrozenNodeWriteListener.php:123 (introduced in #4549). For delete and rename the listener calls abortOperation(). (fix proposed in #4655)
  • A later listener's allow() overrides an earlier refusal — lib/Event/DecisionChangeRequestedEvent.php:129 (introduced in #4595). DecisionChangeRequestedEvent is a vetoable question ("may this user change this decision?"), but allow() sets allowed = true and clears the reason regardless of an earlier refuse(). (fix proposed in #4655)
  • Object GET /logs bypasses the audit-history redaction — lib/Controller/AuditTrailController.php:569 (introduced in #4614). #4614 makes .../audit-trails readable by anyone who may read the object, and takes care to withhold session, request and ipAddress, to strip from changed the properties the caller may not see, and to run the doelbinding guard. (fix proposed in #4655)
  • Copies still send users to the removed personal settings form — lib/Controller/CredentialOauth2Controller.php:102 (introduced in #4587). #4587 deletes OpenRegisterPersonal, personalSettings.js and the <personal> entry in info.xml, so Nextcloud's personal settings page no longer shows anything of OpenRegister. (fix proposed in #4655)
  • Dependent-form re-check runs on one schema write path only — lib/Controller/SchemasController.php:1281 (introduced in #4563). The spec says 'When a schema's properties or required list are saved, OpenRegister SHALL dispatch FormDestinationDependentsEvent'. (fix proposed in #4655)
  • The "may not let itself in" check ignores remote principals — lib/Service/Rbac/GrantManageGuard.php:123 (introduced in #4601). letsItselfIn() compares only the user and group grantee against the caller, but ObjectSharingService::GRANTABLE_TYPES also accepts remote and remote_group (ObjectSharingService.php:94-95). (fix proposed in #4655)
  • Delegated grant changes leave no audit-trail row — lib/Service/Rbac/ObjectOwnershipService.php:366 (introduced in #4601), no changed line within reach, so no thread. A grant_manage holder can grant, revoke and set the owning group, and an admin can change the organisation's grant_manage list, but none of those writes an OpenRegister audit-trail entry: ObjectSharingService has no audit call at all, setOwnerGroup() writes the block and returns, and OrganisationController::update records nothing for the right. reassign() in the same class does write one (ownership_handover, via AuditTrailMapper::createAuditTrail), so who widened access to a record is the one thing the trail cannot answer. Fix: write an audit row (actor, object or organisation, principal before/after) for grant, revoke, owning-group change and grant_manage list change, the way ownership_handover is written. Done when: grep -n -i audit lib/Service/Rbac/ObjectSharingService.php finds the write, and a test asserts the row per action. Verification: grep -c -i -E "auditTrail|AuditTrailMapper" lib/Service/Rbac/ObjectSharingService.php → 0; ObjectOwnershipService.php:95 AUDIT_ACTION = 'ownership_handover' used only by reassign() Found by the persistence audit that ran alongside this review.
  • Guard failure imports shipped authorization unguarded — lib/Service/ShippedBaseline/ShippedConfigurationGuard.php:201 (introduced in #4618), no changed line within reach, so no thread. When the guard throws, the catch returns definition => $incoming with guarded: false: the app's shipped authorization block replaces whatever the administrator set on this instance, which is the one thing the guard exists to prevent. The comment calls that "a known state", but for an authorization block it is fail-open towards the shipped policy. Authorization changes taken because only the app changed them (applied) are also never written to the audit trail. Fix: on guard failure keep the instance's current authorization (or skip importing that key) and log; record applied authorization changes on the trail as recordReplaced does for explicit decisions. Done when: a test makes the guard throw and asserts the instance's authorization block is unchanged after the import. Verification: sed -n 201,216p lib/Service/ShippedBaseline/ShippedConfigurationGuard.php → catch returns 'definition' => $incoming, 'guarded' => false Found by the persistence audit that ran alongside this review. (fix proposed in #4655)

🟢 Minor (8)

✅ Verified clean

  • The five new migrations (Version1Date20261009150000 … 20261011140000): idempotent guards, additive and nullable columns, no two PRs on one table.
  • #4589: a roles block that resolves to nothing grants nothing (PermissionHandler.php:3622, fail-closed).
  • #4601: non-admins cannot change the organisation's grant_manage list; patch routes through update.
  • Draft privacy on the single-object read and write paths (MagicRbacHandler.php:336-363, ObjectService.php:2321); the bulk path is the exception (🔴 above).
  • #4571 graduated/bandRate: band boundaries, zero, negative quantity and the open last band match the spec and are asserted.
  • OAuth2 connect: safeReturnUrl() still blocks open redirects; the archived spec deltas are synced into openspec/specs; every info.xml job, command and repair class exists.
  • The beta release computes its version from tags, so the stale info.xml version on development is not a defect.

Scope decision: promotion of 296 commits, 0 direct. 37 merged PRs: 34 by rubenvdlinde with no review at all, read at full depth in four slices (data/RBAC/migrations, process engines, platform services, controllers/frontend/specs) plus an integration pass over the 59 files touched by two or more PRs; 3 by WilcoLouwerse (#4602, #4642, #4643), trusted on their earlier self-review after a spot-check (no open threads). Residual: the 53 files only those three touched were not re-read; per slice, most test files, l10n and parity JSON were skimmed, not read.
Gates: quality / Hydra Gates and quality / Quality Report green on b9e5d8df; what they cover (lint, licence headers, l10n parity, unit suite) was not re-checked.
Persistence audit: run on this checkout; its grant-related findings are posted above (remote grantees, missing audit rows, the shipped-authorization guard). Two audit items were refuted against the spec and dropped (a holder naming another owning group; a stored form without a mapping). LOW items not posted: occ openregister:example-set:install has no environment guard; the portal assertion uses one HS256 secret with no kid and no jti record; exports with a null retentionDays are kept forever.
Verification: every 🔴 was re-checked on the head by reading the code (nothing was run: no vendor tree, no rig). The Nextcloud 32/33 job-argument limit (plat-f3) was read from the server tags v32.0.0, v33.0.0 and v34.0.0.

…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>
WilcoLouwerse and others added 8 commits October 19, 2026 18:42
Thread: #4580 (comment)

line() capped the count at 50 but stored each message as given, so a node
writing one large string per line grew the run log by the string size times
50 per step. A message longer than FlowStepReport::MAX_MESSAGE_LENGTH (500
characters, counted in characters not bytes) is now cut, and the result,
marker included, ends in FlowStepReport::TRUNCATION_MARKER. A message at the
cap is untouched.

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

Thread: #4580 (comment)

Hidden/not-assignable was set only when the tag was created; findOrCreateTag
returned an existing tag untouched, so tags created before the default, or
first created by a user in the Files tag UI under the same name, stayed in
every user's tag list.

findOrCreateTag now repairs a found object: tag that is still visible or
assignable with updateTag(userVisible: false, userAssignable: false), keeping
its name and colour; this covers both lookup paths (the first search and the
TagAlreadyExists retry). A refused update is logged and the tag is still
used, so the file link is not lost. Tags that are never attached again are
not reached; hiding those needs an upgrade step, which is left to the author.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The fixes for rel-f1, rel-f2, rel-f4 and rel-f7 pushed four classes over a
PHPMD threshold (coupling between objects, class length, method length,
constructor parameter count, a boolean flag argument, a missing import).
No behaviour change:

- RelatedRowAccess takes over the property-read assertion and the organisation
  predicate from RelatedRowQueryApplier; the applier exposes mayReadProperty()
  so MagicSearchHandler no longer needs its own guard parameter.
- Engine detection moves from the applier to RelatedRowExistsClause::engineFor.
- ViewAlertCrossedListener re-checks recipients through
  ViewerReachResolver::recipientReads (now public), the rule the save already
  uses, instead of its own copy.
- ViewsController::refuseForbiddenViewFields takes the saved body instead of a
  boolean flag.
- phpmd.baseline.xml gains one CouplingBetweenObjects entry for
  RelatedRowQueryApplier, the established way this repo records a class that
  legitimately coordinates this many collaborators.

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

The applier sat at the coupling limit only because it called the RBAC handler
itself. RelatedRowAccess now owns the buildRbacPredicateForAlias call together
with the organisation term (one predicate per related alias), so the applier
no longer takes or references MagicRbacHandler and the CouplingBetweenObjects
baseline entry added earlier is removed. phpmd passes on a clean export with
the baseline as it is on the base commit.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ted/_referenced takes the sequential path

The refusal added in buildWhereConditionsSql is an uncaught
InvalidArgumentException on the controller routes (crossTableSearch), so a
cross-table search carrying _related or _referenced ended in HTTP 500.

MagicMapper::shouldUseUnionQuery now returns false when either key is
present. Both the search (dispatchCrossTableSearch) and the count
(countAcrossMultipleTables) decide through it, so the sequential path, which
builds the EXISTS subqueries, answers both and total and rows agree. The
refusal in buildWhereConditionsSql stays as a defensive guard for a caller
that bypasses the routing.

Not routed: the UNION facet branches (getSimpleFacetsUnion) have no sequential
equivalent and still refuse.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e same 64-character cap as the read path

RelatedRowExistsClause::columnName applied every step of the column-naming
rule except the 64-character cap MagicSearchHandler::sanitizeColumnName
applies (MagicMapper's own copy has none; no shared helper exists), so a
property name over 64 characters was filtered on a different column than the
read path uses. The cap is applied in the same order (trim underscores, then
cut) and pinned in ReferencedRowFilterTest.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…d beyond the default flags

Pins the scope the related-row subquery gets for _multitenancy=false (and its
string and unreadable spellings), _rbac=false with an RBAC bypass available,
an anonymous caller (the empty scope) and a public schema with and without an
explicit multitenancy request. Test-only.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…th _related/_referenced answers 400, not 500

Before this round those facets silently ignored the filter and returned wrong
counts; a clear 400 is the honest answer.

- New RelatedRowFilterNotSupportedException (an InvalidArgumentException) is
  thrown by MagicSearchHandler::refuseRelatedRowFilters, now public.
- MagicFacetHandler::getSimpleFacetsUnion calls it first. The per-facet UNION
  queries have their own catch blocks that return empty buckets, so the
  refusal had to come before them or it would have been swallowed.
- New RelatedRowFilterMiddleware (registered in Application) answers the
  exception with HTTP 400 and {"error": <message>}; every other exception is
  rethrown. A middleware covers every controller that reaches the facets:
  facets are only reached through getSimpleFacets (MagicMapper), which is
  called by FacetHandler::getFacetsForObjects (list route with _facets,
  ViewPresentationService, ObjectMetricSource); there is no facets-only
  controller route. The UNION search builder keeps raising the same
  exception as a defensive guard.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
rubenvdlinde and others added 6 commits October 19, 2026 22:31
…every save (#4699)

prepareObjectDataForTable() reads each metadata column's value from `@self`
under the column's own name, so `_schema_version` was looked up as
`schema_version`. The entity serialises it as `schemaVersion`, so the lookup
always missed and every create or update wrote NULL into the column. The
camelCase name is now read first, with the snake_case one kept as a fallback.

Assisted-by: ClaudeCode:claude-opus-5-5
)

* feat(search): an orNull modifier admits rows whose field is empty

A filter such as `isFinalStatus[ne]=true` or `isFinalStatus=false` leaves out
every object whose flag was never written, because SQL treats a comparison
with NULL as unknown. Operators in a bag AND together, so "false or not set"
could not be said. dossiq's dashboard widgets read their case lists through
these filters, and a case that had not been recomputed yet dropped off.

`orNull` is a modifier in the operator bag. When it reads as true, the
field's other operators still AND together and the result is ORed with
`column IS NULL`:

  ?isFinalStatus[ne]=true&isFinalStatus[orNull]=true
  ['isFinalStatus' => ['in' => [false], 'orNull' => true]]
  ['@self' => ['expires' => ['lt' => '2026-01-01', 'orNull' => true]]]

It works on schema properties and `@self` metadata, on the QueryBuilder and
the raw UNION path, and on paths inside object properties. NullFilter reads
it for every path. `ne` and the other operators keep their meaning when the
modifier is absent. To make one field's conditions groupable, the builders
now collect a field's predicates before ANDing them, which also brings the
two long builders under the phpmd length limit.

Assisted-by: ClaudeCode:claude-opus-5-5

* fix(search): keep a like condition beside the rest of its operator bag

Splitting a field's operator bag into its own method returned the bag's
conditions on their own, so `{like: 'a', ne: 'b'}` lost its `like` on both
the QueryBuilder and the UNION path. The field's earlier conditions are
merged back in.

Assisted-by: ClaudeCode:claude-opus-5-5
…e an object (#4698)

`updated` moves on every write. SaveObject sets it to now on every create
and update, the mapper defaults `_updated` to now, and the bulk upsert
writes `_updated = NOW()`. So a background job or a listener that
recomputes a derived field (dossiq's `currentStatusDwellDays`,
`statusDwellBreached`) makes every case look freshly touched, and a
"stalled cases" list built on `updated` is always empty.

`@self.lastUserActivity` (column `_last_user_activity`) moves only on a
person's write. UserActivityStamper decides that in SaveObject's create and
update: a web request (not the CLI, not `cron.php`), a signed-in user, not
inside SystemOperationContext, and a WriteCause of `person`. Every other
write carries the stored value forward; the mapper never defaults it to now.

It filters and sorts like `updated`:

  ?@self[lastUserActivity][lt]=2026-10-01T00:00:00
  &_order[@self.lastUserActivity]=asc

The column is retrofitted onto existing magic tables by addMissingColumns()
and indexed on new ones. Sorting on a camelCase `@self` field now resolves
its snake_case column on the single-table path, as the UNION path already
did. `updated` keeps its meaning and its writers.

Assisted-by: ClaudeCode:claude-opus-5-5
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… filtered properties

MagicSearchHandlerOrNullModifierTest (from #4697) built the handler with a
bare RelatedRowQueryApplier mock, whose mayReadProperty() answers false. Since
rel-f1 a nested-path filter whose root property the caller cannot read matches
nothing (1 = 0), so testNestedPathAdmitsNullOnBothPaths saw the refusal
instead of the orNull predicate. The mock now grants read access, as
MagicSearchHandlerNestedPathFilterTest does. Test-only.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
WilcoLouwerse
WilcoLouwerse previously approved these changes Oct 19, 2026

@WilcoLouwerse WilcoLouwerse left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Re-review: approved (round 2 fixes merged)

All 10 findings of my round-2 review (verdict) are fixed in #4696, merged into development; this PR's head (ceacdc84ac) is that merge, and my 10 threads are resolved. This approval covers my findings only. Still between this PR and a merge: the 55 open threads of rjzondervan (beta requires every thread resolved), and CI on ceacdc84ac, which was still running when I approved (34 passed, 0 failed so far).

Connected issues

  • #4691 and #4692: open author decisions from the review, tracked outside this release.
Round Findings Outcome
1 86 threads 78 fixed in #4669 (merged), 8 author decisions
2 (e07092e) 7 🟡, 3 🟢 in the 12 unreviewed development PRs fixed in #4696 (merged), threads resolved
3 (ceacdc84ac) — this review — approved
Left to the author (from #4696, not blocking)
  • rel-f1: the plain column filter path and the outer property of a _referenced join are not covered by the property read rule.
  • rel-f6: GET /api/organisations/{uuid} still scans every magic table per request.
  • rel-f10: object: tags that are never attached again keep their old flags; hiding them needs an upgrade step.

A terms facet named its UUID buckets with one batch lookup and then looked
every bucket up again on its own. A UUID the batch could not name (a
dangling reference, or an object the caller may not read) cost a scan of
every magic table per bucket. Buckets the batch already asked about now
keep the batch answer.

A property that declares a referenceType holds an integration's ids (a
Nextcloud group, a contact), so its values are no longer looked up as
object names at all.

On the dossiq case list (296 cases, 51 facets) `_facets=extend` drops
from about 64 s to about 4 s; the labels are unchanged except that a
group id in a referenceType facet is shown in full.

Assisted-by: ClaudeCode:claude-opus-5-5
Comment thread lib/Db/MagicMapper/MagicFacetHandler.php
Comment thread lib/Db/MagicMapper/MagicFacetHandler.php
Comment thread lib/Db/MagicMapper/MagicFacetHandler.php Outdated
Comment thread lib/Db/MagicMapper/MagicFacetHandler.php Outdated

@rjzondervan rjzondervan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: REQUEST_CHANGES (Strict, round 2)

1 new concern blocks under Strict: #4703's union-path skip has no test; my 5 round-1 blockers are fixed on the head and every other round-1 thread of mine is resolved or tracked. Before a merge: that concern, one approval, and 4 open threads (all mine, this round's: f2 plus three 🟢 that only need resolving).

🔧 Fix PR: #4669 — merged, 5 of 5 blockers verified

#4669 (with #4696 for @WilcoLouwerse's round 2) carried my round-1 findings; @WilcoLouwerse resolved 47 of my 55 threads at e07092e, and I re-checked my 5 blockers myself on b12ef29. The 8 left as author decisions are now tracked or recorded (below).

Round Found Status
1 (8e7bbd8) 🔴 5 and 50 other findings 47 fixed in #4669 (resolved by @WilcoLouwerse), 5 blockers re-verified here; 8 author decisions: 4 tracked in #4706–#4709, 4 recorded as decided
2 (b12ef29) — this review 🟡 1 · 🟢 3, all in #4703 (one pre-existing on beta) open
Finding Was Now Where
🔴 A form payload's id picks the uuid, and a scoped draft save writes over any draft 🔴 round 1 resolved in #4669: mapPayload() drops id and @self.id thread
🔴 Bulk delete ignores every draft rule 🔴 round 1 resolved in #4669: per-row draftKeptFromCaller() in deleteObjects() thread
🔴 /api/audit-trails/readable shows other users' draft contents 🔴 round 1 resolved in #4669: the lister hands the object to hasPermission(), which refuses another user's draft thread
🔴 Tree fields returns writeOnly, property-restricted and encrypted values raw 🔴 round 1 resolved in #4669: redacted() goes through redactWriteOnlyFromRows thread
🔴 Bulk writer lets a _-named schema property overwrite metadata columns 🔴 round 1 resolved in #4669: skippedOnWrite() thread
🟡 Remove deletes the set's organisation without checking whether it is in use 🟡 round 1 tracked in #4706 thread
🟡 An oversized deferred entry silently loses its listener side effect 🟡 round 1 tracked in #4707 thread
🟡 Personal data in a draft cannot be found by a data-subject request 🟡 round 1 tracked in #4708 thread
🟡 Another user's hidden draft blocks a RESTRICT delete 🟡 round 1 tracked in #4709 thread
🟢 Four minors left as author decisions (/forms/validate across organisations, the draft 403 oracle, decision tables across organisations, the object folder node) 🟢 round 1 decided as #4669 recorded; resolved see #4669 "Left open"
🟡 The union-path referenceType skip has no test new new — 🟡 comment
🟢 Batch facet labels come from a cache every caller shares (pre-existing on beta) new new — 🟢, tracked in #4705 comment
🟢 The union path decides the skip from the first table's schema only new new — 🟢 comment
🟢 Labels for UUIDs held by referenceType properties change from short to full new new — 🟢 comment
Details of this round
  • Scope: 213 commits since 8e7bbd8. @WilcoLouwerse reviewed the 12 development PRs that came in after my round 1 (his round 2, fixed in #4696) and approved at ceacdc84; I took those as reviewed. The one unreviewed change is #4703 (b12ef291, merged this morning, +298/−11 in MagicFacetHandler), analysed at Strict depth.
  • #4703 verified clean: the batch and the per-bucket path both end in getMultipleObjectNames(), which filters per caller; #4703 only removes per-bucket lookups the batch already overrode, so nothing is resolved more widely than before. The label is still getName() ?? uuid; the batch is capped at MAX_FACET_BUCKETS (10,000); bucket keys and counts are unchanged.
  • The pre-existing label cache (#4705): batchResolveUuidLabels() keys its 24 h distributed cache on register, schema and field only (:2241) and serves it without visibleNames() (:2277-2296); origin/beta has the same code (:2136, :2174, :2245), so it is not new in this release. Assigned to @rubenvdlinde.
  • Tracking issues: #4705 (the label cache), #4706–#4709 (the four 🟡 author decisions from round 1); #4691 and #4692 track two other decisions from @WilcoLouwerse's rounds.
  • CI: green on b12ef29, including quality / Hydra Gates, phpmd and the required Quality Report.

🤖 Generated with Claude Code

… across every schema

Threads:
#4580 (comment)
#4580 (comment)
#4580 (comment)

- getTermsFacetUnion() skipped the object-name lookup when the FIRST table's
  schema declared a referenceType for the column, so a union whose later
  schema holds real object references lost those names. The new
  anyHoldsObjectReferences() keeps the lookup unless every schema of the
  union declares a referenceType.
- testUnionIntegrationReferencesAreNotLookedUpAsObjects calls the union path
  with a referenceType property and expects no lookup; removing the skip
  turns it red. testUnionKeepsTheLookupWhenAnySchemaHoldsObjectReferences
  pins the mixed union; the first-schema-only check turns it red.
- The @SPEC exclude text on holdsObjectReferences() now says a UUID-shaped
  integration id is labelled in full instead of its shortened form.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
fix(review): #4580 union-path facet skip, round 3

@rjzondervan rjzondervan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: APPROVE (Quick, round 3)

Nothing left to address: #4710 fixes this release's last concern and both minors, and the label cache is tracked in #4705; no thread on this PR is open. Before a merge: nothing from my side.

🔧 Fix PR: #4710 — merged, 3 of 3 verified

Round Found Status
1 (8e7bbd8) 🔴 5 and 50 other findings fixed in #4669 or tracked (#4706–#4709)
2 (b12ef29) 🟡 1 · 🟢 3 (in #4703) 3 fixed in #4710; 1 tracked in #4705
3 (0943fb8) — this review nothing new —
Finding Was Now Where
🟡 The union-path referenceType skip has no test 🟡 round 2 resolved in 6c025c49 (#4710) thread
🟢 The union path decides the skip from the first table's schema only 🟢 round 2 resolved in 6c025c49 (#4710) thread
🟢 Labels for UUIDs held by referenceType properties change 🟢 round 2 resolved in 6c025c49 (#4710) thread
🟢 Batch facet labels come from a cache every caller shares (pre-existing on beta) 🟢 round 2 tracked in #4705 thread
Details of this round
  • Scope: 2 commits since b12ef29, both #4710 (6c025c49 and its merge); no other change, so no analysis agent ran.
  • Verified on 0943fb8: #4710's two files are identical to its tip; anyHoldsObjectReferences() clears the batch lookup only when no schema of the union holds object references (holdsObjectReferences() is false only for a declared referenceType, and a schema without the column keeps the lookup); testUnionIntegrationReferencesAreNotLookedUpAsObjects and testUnionKeepsTheLookupWhenAnySchemaHoldsObjectReferences are in MagicFacetHandlerBucketLabelTest.
  • CI: every check green on 0943fb8 (51 passed, 9 skipped), including quality / Hydra Gates and the required Quality Report.

🤖 Generated with Claude Code

@WilcoLouwerse
WilcoLouwerse merged commit fc72b70 into beta Oct 20, 2026
65 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants