Release: merge development into beta - #1711
Open
github-actions[bot] wants to merge 3194 commits into
Open
Conversation
…union of concrete JSONResponse types
PHPStan reported:
Method AuditTrailController::statistics() should return
JSONResponse<200|401|403, array{error?: string, total?: int, create?: int,
update?: int, delete?: int, read?: int}, array> but returns
JSONResponse<200, array{total: int, create: int, update: int, delete: int,
read: int}, array{}>.
The admin gate is present and correct — requireAdmin() returns the 401/403
response and statistics() returns it unchanged. The defect is purely in the
@psalm-return docblock: JSONResponse's template parameters are INVARIANT, so a
single JSONResponse whose template arguments are themselves unions (200|401|403
plus an all-optional data shape) is satisfied by no return statement at all —
not even the ones it was written to describe.
Rewritten as a union of concrete JSONResponse types, one member per branch,
each the exact type that branch produces (including array{} for the default
headers, which is what the constructor actually yields — the old
array<never, never> was the second half of the mismatch). No baseline entry, no
widening; the annotation now documents all three status codes and is checkable.
Verified locally on PHP 8.3: phpstan clean (was 1 error), phpcs clean, psalm
clean on the changed file.
…listener `\OCP\Util::addScript()` is the canonical Nextcloud asset API and is exposed as a static method only — there is no injectable DI equivalent reachable from an event listener, which is handed only the event. Wrapping the call in a seam class would relocate the identical static call rather than remove it. Suppression sits on the `handle()` METHOD docblock — the narrowest scope PHPMD actually honours, verified empirically (a file-docblock annotation would not apply). Matches the in-repo precedent for the same API in `ScriptManifestLoader::addEntryScripts()`. No baseline entry added.
various bug fixes
feat(flow): RunFlowOperation frontend settings component (flow-name input)
chore(deps): @conduction/nextcloud-vue 3.0.0-vue3.6
enable-hydra-gates defaults to false, so quality / Hydra Gates had been SKIPPED on every run this repo ever had. Pinned to v1.0.1, the pin openbuild already runs. enable-axe deliberately left off.
…pping (#2337) A skipped job and a passing job are indistinguishable in the Quality Report. Every gate turned on here reported 'skipped' in every run. Each newly-enabled leg was measured against this tree BEFORE being enabled; the results are in the PR description. Legs that were measured failing are enabled anyway - the defects are pre-existing, and the only thing that changed is that CI can now see them. Journeydoc Capture and enable-axe are deliberately NOT enabled.
* chore(deps): move to @conduction/nextcloud-vue 2.2.0-vue3.1 The 3.0.0-vue3.* line is being withdrawn from npm. The major was cut from a real BREAKING CHANGE footer (the retired action-list flow editors), but it applied to a prerelease channel only our own apps consume, so the line is resumed at 2.x rather than carried forward. 2.2.0-vue3.1 is the first release on the resumed line and is a superset of 3.0.0-vue3.6 — it additionally carries the recovered Vue 2 -> Vue 3 component conversion and the CnGraphCanvas port/loop work. Despite the lower version number this is not a downgrade in content. Verified rather than assumed: `npm install` resolves node_modules/@conduction/nextcloud-vue -> 2.2.0-vue3.1, and `npm run build` exits 0. * fix(deps): regenerate the lockfile with npm 10 to match CI CI runs node 20 / npm 10.8.2. Regenerating with local npm 11 omits the `"dev": true` markers npm 10 writes, which can put package.json and the lockfile out of sync for `npm ci`. No dependency actually changed: 0 packages added, 0 removed, 0 version changes — this is 52 restored `dev` markers. Verified with `npx npm@10.8.2 ci --dry-run` (exit 0).
…meter only (#2343) * fix(quality): scope the Migration phpmd exclusion to UnusedFormalParameter only phpmd.xml carried a TOP-LEVEL <exclude-pattern>*/Migration/*</exclude-pattern>. A top-level exclude-pattern is applied by PDepend at file-collection time, so it drops lib/Migration from EVERY rule in the ruleset, not from one rule. Measured on phpmd 2.15.0 / PHP 8.4.22, that line was silently swallowing 11 real findings: 3 NPathComplexity, 3 ElseExpression, 2 StaticAccess, 2 ExcessiveMethodLength, 1 CyclomaticComplexity. The same file also had a second, NESTED <exclude-pattern>*Migration*</exclude-pattern> inside the UnusedFormalParameter rule. PHPMD 2.15 honours exclude-patterns only as direct children of <ruleset>; nested ones are parsed and discarded, so that one was inert. UnusedFormalParameter now lives alone in phpmd-unusedparams.xml with its own top-level exclude-pattern, so the exclusion applies to that rule and nothing else. `composer phpmd` runs both rulesets as separate legs, worst exit code winning, and neither leg can short-circuit the other. OCP\Migration\IMigrationStep mandates the changeSchema / preSchemaChange / postSchemaChange signatures, so that one rule genuinely cannot apply to migrations. With the exclusion scoped, 246 now-redundant @SuppressWarnings(PHPMD.UnusedFormalParameter) annotations were deleted from 174 files in lib/Migration (236 exact + 10 with a stray space before the paren, which were never valid annotations). No suppression was added anywhere. All 11 surfaced findings are fixed with behaviour-preserving refactors: column-addition ifs replaced by a spec list plus a loop, platform branches extracted into named private methods, one else inverted to an early return, and two \OCP\Server::get() static calls replaced by an injected Psr\Container\ContainerInterface (keeping the lazy resolution the original comment asks for). Column order, SQL text and IOutput messages are unchanged. Verified: both legs exit 0 over all 1400 files in lib/, and both were positive-controlled (a planted UnusedFormalParameter outside lib/Migration and a planted ElseExpression inside it are both reported, exit 2). phpcs clean over lib/. Unit suite unchanged at 16007 tests / 35916 assertions before and after. Refs ConductionNL/.github#155 Refs #2338 * fix(quality): clear the two Hydra gates this PR's diff scope surfaced Both are pre-existing debt in files this PR already touches; the gates are diff-scoped, so they only became visible now. gate-28 license-triangle: five migrations carried @license AGPL-3.0-or-later while composer.json declares EUPL-1.2. Corrected to EUPL-1.2 — this is a docblock correction to match the declared licence, not a relicensing. gate-46 spec-anchor-existence: Version1Date20260511100000 pointed its @SPEC at openspec/changes/scholiq-deps/tenant-key-api/tasks.md, a change directory that was never archived under that name, so the target does not resolve. Retargeted at the canonical openspec/specs/saas-multi-tenant/spec.md, which is where the openregister_tenant_keys requirements live. (The same stale pointer also sits in lib/Service/TenantKeyService.php; that file is outside this PR's diff and is left for a follow-up rather than widening the scope.) --------- Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
* chore(deps): move to @conduction/nextcloud-vue 2.2.0-vue3.3
Picks up the two releases that landed after 2.2.0-vue3.1:
2.2.0-vue3.2 four dashboard defects — date-range chip shows its dates and
calendar-aligned presets, a dangling labelResolve no longer
renders a raw UUID, and the table's "View all" pins to the
bottom instead of scrolling away
2.2.0-vue3.3 gridstack's stylesheet now ships with the library that
requires it; CnFormDialog splits over-long schema descriptions
behind an info popover; CnContextMenu closes again on outside
press and stops hijacking every popper with a cursor transform
Lockfile regenerated with npm 10.8.2 to match CI's node 20 toolchain — local
npm 11 prunes optional entries that do not apply to the current platform, which
makes CI's `npm ci` fail with "Missing: ... from lock file". Running `npm ci`
locally does not reproduce it, because npm 11 accepts its own lockfile.
Verified: `npx npm@10.8.2 ci --dry-run` exits 0, and `USE_LOCAL_LIB=false
npm run build` exits 0 with no unresolved modules and no reference to a sibling
nextcloud-vue checkout. USE_LOCAL_LIB=false is load-bearing: webpack aliases
@conduction/nextcloud-vue to ../nextcloud-vue/src when that sibling exists, so
a plain build can silently compile the sibling instead of the package under test.
* fix(deps): restore the optional lockfile entries npm 11 pruned
The verification build ran `npm install` under local npm 11 AFTER the lockfile
had been regenerated with npm 10.8.2, which silently re-pruned the optional
entries that do not apply to this platform — per-arch esbuild/rolldown/
lightningcss binaries and @nextcloud/vue's optional pinia and vite.
CI runs npm 10.8.2, whose `npm ci` requires those entries, so it failed at
Install dependencies and took every dependent job with it.
Regenerated with `npx npm@10.8.2 install --package-lock-only` and this time
nothing runs npm 11 against it afterwards. Verified with
`npx npm@10.8.2 ci --dry-run` (exit 0).
…h fan-out, and make bsn/user real formats (#2336) * feat(audit): add the seal sweeper the fail-soft path always promised sealRow() and sealRows() log "a later seal pass will chain it" whenever the seal lock is contended. There was no later pass. Nothing swept unsealed rows, so every fail-soft skip was permanent. Measured on the dev instance: 49,123 of 308,937 audit rows — 15.9% — had no hash and never would have. A row with no hash is a row the chain cannot vouch for, and the chain exists so an auditor can say "this history has not been rewritten" from evidence rather than assertion. AuditSealJob runs every 5 minutes, up to 10 passes of 500 rows. sealUnsealed() takes the OLDEST unsealed rows in id order, so it is resumable by construction: a tick that stops early is simply resumed by the next. It delegates to sealRows(), which derives the predecessor once per batch and chains forward — measured at 1.14 ms/row against 14.85 ms/row for the inline per-row seal. Verified: 49,123 -> 48,623 in one pass (exactly 500), and verifyChain() over the swept range returns valid=true with zero duplicate predecessors. FOUND WHILE VERIFYING, and NOT caused by this change: the chain is already broken at id 153230. 5,314 rows share a previous_hash with a sibling across 2,413 distinct predecessors, one of them used by 442 rows. That is the signature of concurrent seal passes each reading the same predecessor and then writing — exactly the race SEAL_LOCK_KEY was later introduced to prevent. The damage predates the lock. My swept range (31518-53387) contains zero duplicates and verifies clean, so the sweeper does not add to it; but it cannot repair history either, and a re-chain of the corrupted region is its own piece of work. Complexity suppressed with the reason: splitting an audit-integrity class to satisfy a threshold risks the property it guarantees. * feat(audit): repair the broken chain the sweeper cannot touch, and surface it The sweeper (previous commit) seals rows with NO hash. It cannot repair rows with a WRONG one, and the dev instance has 5,314 of those: rows chained onto a shared predecessor across 2,413 distinct predecessors, one used by 442 rows. That is a fan-out, not a chain, and it predates SEAL_LOCK_KEY. This is the re-chain that commit named as its own piece of work. - AuditHashService::rechainAll() walks every row in id order under the seal lock, deriving each previousHash from the row actually before it, so the result is one chain by construction. It REWRITES stored hashes, so it is an occ command and never a scheduled job — "something rewrote the audit hashes" is exactly the event the chain exists to make suspicious — and both ends of the run are logged at warning level so the rewrite is itself in the record. - openregister:rechain-audit-trail verifies before and after, with --dry-run and a confirmation prompt. The verification either side is the point: a repair that cannot show the chain was broken before and whole after is indistinguishable from one that quietly rewrote a healthy chain. It exits FAILURE if the chain still reports invalid. - verifyChain() now walks in windows. The DB was never the constraint — Postgres returns the whole trail by index scan in ~350 ms — the client was: libpq buffers an entire result set before PHP sees a row, so `select *` over 309,090 rows at ~5.8 KB wide pulls ~1.8 GB into the driver. That memory is held in C, so memory_get_peak_usage() cannot see it and the failure is not a PHP fatal but a SIGKILL. Measured: an occ run died with the OS killer while PHP still reported a 57 MB peak. Windowing bounds the driver buffer and cuts a partial walk from ~129 s to under a second. - getIntegrityStatus() + GET /api/audit-trails/integrity + a LogIntegrity admin card. Three COUNT/MAX queries, deliberately NOT a verification: binding a settings page to verifyChain() would make opening settings expensive enough that an admin stops opening it. The card keeps the two distinct — coverage is free and continuous, verification is explicit and operator-initiated. info.xml registers the command only; no version bump. * test(audit): serve the windowed verifyChain walk in the tombstone harness The merge broke these three tests and CI would have been the first to say so. wireRows() mocked the old unbounded query: one fetch() cursor, no expr(), no setMaxResults. verifyChain() now pages by id, so expr()->gt() was called on a null expression builder. The mock serves one populated window and then an empty one, which is how the walk terminates — serving rows forever would hang the suite rather than fail it. Nothing about what the tests ASSERT changed: testChainStaysValidAcrossA- Tombstone still expects valid=true and testTamperedRowStillBreaksTheChain still expects valid=false on the same harness, so the two remain each other's positive control. * fix(audit): page verifyChain so it stops being OOM-killed, and document sealing verifyChain() issued one unbounded `select *` over the whole audit trail. The database was never the constraint — Postgres returns all 309,090 rows by index scan in ~350ms — but the client is: libpq buffers an entire result set before PHP sees the first row, so at ~5.8KB per row that pulled ~1.8GB into the driver. That memory is held in C, so memory_get_peak_usage() reported a serene 57MB while the OS SIGKILLed the process. The failure mode was therefore the worst available one: verification did not fail, it VANISHED — no PHP fatal, nothing in the log, and an operator left with no signal that the chain had gone unchecked. Now walks in 500-row windows keyed on id. Same verdict on the live trail (brokenAt 153230, 500 verified, 6381 skipped), 128.7s -> 0.1s. Paging adds exactly one new way to be wrong — losing previousHash across a window boundary — so AuditHashVerifyPagingTest pins it down: a chain split across windows verifies clean, a row tampered AT the boundary is caught at the boundary, an entirely-unsealed window does not turn a gap into a false tamper alarm, and an empty trail terminates. Mutating the carry-over to reset per window turns two of them red. Also drops rechainAll()'s `skipped` counter, which nothing could ever increment — a field structurally pinned at 0 reads as "nothing was skipped" when it means "never measured". Docs: versioning-and-audit.md explained hash chaining but never said when sealing runs, that it is fail-soft, or that a gap is not a tampered entry. Adds that, the sweeper's schedule, and the repair procedure — and corrects the documented verify endpoint, which had the wrong path, wrong params, and described from/to as timestamps when they are entry IDs. * fix(audit): the backlog alarm was dead code, and psalm was the one that said so Four quality findings on this branch, all real: 🔴 AuditSealJob's "the hash chain has gaps that are not closing" warning could NEVER fire. An `if ($sealed === 0) { return; }` sat above it, so the only state that reaches the warning — sealed nothing, backlog non-empty — had already left the method. Psalm called it a ParadoxicalCondition; operationally it means the sweeper could stop working and the alarm written to say so would stay silent, which is the exact failure the sweeper exists to end. Reading the backlog before the early exit makes it reachable, and the early exit now covers only the true steady state (sealed nothing, nothing outstanding). Added tests/Unit/BackgroundJob/AuditSealJobTest.php — the job had NO test at all, which is how the dead branch survived. Verified as a positive control: with the early return restored, testWarnsWhenNothingSealedAndABacklogRemains FAILS. It also carries its own negative control (the steady state must stay silent) so the alarm cannot be satisfied by simply warning always. 🔴 getIntegrityStatus() called $qb->func()->max('id', 'last_sealed'). The max() function builder takes only the field — unlike count(), it has no alias parameter — so the second argument was swallowed and aliased nothing. Same family as the named-arg-on-a-variadic trap. phpmd: rechainAll() and verifyChain() were both over the 100-line method threshold. Extracted rechainWindow() and readChainWindow() rather than widening anything, so the outer methods read as the repair's and the verification's shape and the per-row rule lives in one place. ExcessiveClassLength is suppressed with a reason and a named next step (move the operator-initiated re-chain to its own service) rather than a threshold change. phpcs: named parameter on getHelper(), and the two duplicated before/after report blocks in the command folded into summarise() — which removes both inline ternaries and makes the two ends of the run print the same fields, so a reader comparing them is comparing like with like. Local: phpmd clean over lib, psalm clean on the changed files, 321 audit and retention tests green. The 10 tests/Unit/AppHost failures in the full local run are byte-identical to development and untouched by this branch — development's own CI has PHPUnit green — so they are this instance, not this change. * test(audit): pin the repair to producing a chain, not a fan-out rechainAll() rewrites hashes that already exist — the one operation here the audit trail is designed to make suspicious — and had no test. It also had the easiest possible way to be silently useless: derive previousHash once and reuse it, and every row gets a hash, every row looks sealed, and the chain is exactly as broken as before. That IS the bug it was written to repair (5,314 rows over 2,413 predecessors on the live trail, one shared by 442 rows), so reproducing it in the fix would be invisible. So the assertion is not "rows got hashes" but "row N's previousHash is row N-1's hash". The fixture seeds all three rows pointing at one shared predecessor, and asserts those stale values are gone. Dropping the `$previousHash = $hash` carry-forward in rechainWindow() turns it red. Also covers the refusal path — the repair must decline when the seal lock is held, since competing with a concurrent pass is how the fan-out arose — and getIntegrityStatus(), including the empty-trail case that would otherwise divide by zero on a fresh install's settings page. Earns back the 0.02% the coverage guard flagged, with tests worth having rather than by moving the baseline. * fix(audit): satisfy phpstan on the re-chain command, and refuse to guess consent Two findings, the second one substantive: - `?? 0` on $result['tombstonesPreserved'] was redundant against rechainAll()'s declared array shape, and phpstan said so. - getHelper() returns HelperInterface, which has no ask(). The call only worked by luck of what Symfony happens to return. Rather than casting the complaint away, the command now checks for a QuestionHelper and FAILS if it does not have one. Treating a missing helper as consent would rewrite every stored audit hash on the strength of an environment quirk — for a destructive repair behind a confirmation prompt, "could not ask" must never mean "yes". --force remains the supported way to say yes without a prompt. * perf(audit,logging): stop sealing on the write path, and stop re-reading our own writes Three problems found by running the thing rather than reading it. SEALING ON THE WRITE PATH CORRUPTED THE CHAIN. Sealing takes an exclusive lock, so under concurrency some rows sealed and some fell through the fail-soft path unsealed. A row sealed AFTER a gap chained onto the newest SEALED row, skipping the gap — so when the sweep later filled that gap, the gap and the row after it shared one predecessor. That is a fan-out, which verifyChain() cannot tell from tampering. Caught live: rows 455956 and 455957 both chained onto 455955, and verification went from valid=true to valid=false BECAUSE the sweeper ran. Sealing now happens only in AuditSealJob. With one sealer, unsealed rows are always a contiguous TAIL rather than holes punched mid-chain, so no later row can chain across a gap. The fan-out is not handled, it is unreachable. The write-path tests assert never() on sealing, as the invariant it now is. The sweep also re-chains from the oldest gap FORWARD rather than filling in place, bounded at MAX_SWEEP_RECHAIN, so legacy interleaving self-heals without a five-minute cron ever attempting a 300k-row rewrite. AN ABANDONED SEAL LOCK SILENTLY DISABLED SEALING. ILockingProvider has no owner and no liveness check, and DBLockingProvider only reaps expired rows from a separate job, so a process killed inside its critical section held the lock for the rest of its TTL — measured at 46 minutes. Every sweep in that window returned 0, which is ALSO the value meaning "nothing to seal": a dead sweeper and an idle one were indistinguishable while the backlog grew. acquireSealLock() now stamps appconfig, and breakStaleSealLock() takes over a lock held longer than any real pass can run, logging a warning because a process dying inside a critical section is worth seeing even when recovered from. WE RE-READ EVERY ROW WE WROTE. The magic tables have exactly ONE database-generated column, `_id` (nextval); `_created`/`_updated` carry no column default (59,292 of 59,292 rows have both set, so PHP writes them). The UPDATE re-read therefore fetched back the values it had just sent — and its own catch already returned the input entity when the read failed, so that was settled. Removed: one less query per update. The INSERT re-read stays, because the insert helper returns void and that read is genuinely how the id arrives; dropping it would hand callers null ids. It is now wrapped in a transaction instead, which is what Nextcloud's check actually asks for — isTransactionActive() is the first branch of the dirty-read test, since a transacted read goes to the primary. The commit sits in a finally so the lost-write throw cannot leak an open transaction. Both mattered because a "dirty table read" attaches a synthetic exception whose serialised backtrace measured 5.9MB, on every insert and every update. LOGGING. info was being used for "something happened": 700 info against 514 debug. MagicMapper, on every save, went 39 info -> 3, keeping only table creation, DDL and bulk deletion — rare and structural. All 13 entry-traces are gone ("Starting createFromArray", "About to update", "...called"); getOrganisationForNewEntity emitted four info lines per save to answer one question and now emits one debug recording the outcome, and createFromArray went from seven narrating lines to one saying what it created. NOTIFIER. Nextcloud deprecated InvalidArgumentException for declining a notification, and every notifier is offered every notification — so the routine decline logged a warning each time, dozens per dashboard load. UnknownNotificationException says the same thing silently, matching AnnotationNotifier which already did it correctly. * perf(objects): read the generated key from the INSERT, and stop waking eight apps to seed baseline data TWO WRITES, NOT THREE. insertObjectInRegisterSchemaTable() returned void, so the generated `_id` was thrown away and had to be recovered by SELECTing the row back — against a table written milliseconds earlier, which is exactly Nextcloud's "dirty table read" condition and cost ~5.9MB of serialised backtrace per object. It now returns lastInsertId(). Two facts make that exact rather than merely convenient, and both must keep holding: these tables carry exactly ONE sequence (`_id`; verified against information_schema — no other column has a default), and the call is the very next statement on the same connection. A second serial column in the magic-table shape would break it, and the comment says so. `_id` was never the identity anyway. saveObjectToRegisterSchemaTable() returns the UUID, which PHP generates; the id is an internal key catching up. Verified live: insert and update each emit ZERO dirty reads, and the id returned matches the row (min=max=1 on a single-row table, so it could not have been a coincidence). The rare path where an INSERT loses a uuid race and lands as an update still reads, since there is no generated key to report — but it uses the raw row fetch, not the hydrating one, and keeps the lost-write check that #2212 needed. EIGHT APPS WOKE FOR EVERY SEEDED OBJECT. docudesk, softwarecatalog, opencatalogi, openbuild, hermiq, zaakafhandelapp and hrmq all subscribe to object lifecycle events. Measured mid-repair: 155 "DocuDesk: Processing event", 116 compliance-subscriber calls, 116 queued text-extraction jobs — running document extraction and compliance scoring over content that shipped WITH the app, before anyone had configured anything. Seeding is not a user action, so there is no intent for a listener to react to. importSeedData() now runs inside SystemOperationContext, and MagicMapper withholds lifecycle events while it is active. Gating the DISPATCH rather than each listener is the point: one change here instead of eight across apps we do not all own, and it cannot be half-adopted — a listener that never learns of an event cannot forget to check. Proven with both controls, because "no events fired" is otherwise exactly the result a broken test gives for free: a normal save outside the context still dispatches (1), the same save inside it does not (0). Deferral was considered and is not the answer here. defer_object_events is unset, so nothing defers today; DeferredObjectEventJob hardcodes ObjectCreatedEvent and ignores the `action` it is passed; and an update cannot be deferred at all, since ObjectUpdatedEvent needs oldObject, which a later job cannot recover. Recorded so the next person does not rediscover it. * fix(schemas,objects): make bsn and user real formats, and stop a debug log killing a save BSN WAS BUILT, WIRED, AND UNREACHABLE. BsnFormat implements the 11-proef and is already registered with the value validator, so OpenRegister could checksum a burgerservicenummer all along — it just refused to accept a SCHEMA that declared `format: bsn`, because PropertyValidatorHandler's allowlist never got the entry. The two lists disagreed, and the cost was not cosmetic: procest declares `format: bsn` on a burgerservicenummer, so its schema import failed, which failed schema creation, which failed its "Load default ZGW API mapping configurations" repair step. An app went unconfigured over a missing word in an array. `user` now exists as a format too, and means what it says: UserFormat asks IUserManager whether the account exists. A user id is syntactically just a string, so a pattern could assert nothing — the backend is the only authority, and without it a schema could carry a deleted account's id forever while every consumer resolved it to nothing. Both verified in both directions. BSN: 111222333 accepted, 111222334 (one digit off) rejected, the all-zero sentinel rejected, a short value rejected. user: "admin" accepted, a non-existent uid rejected, empty and whitespace rejected. And an invented format is still rejected at schema level — the allowlist did not become permissive, it became correct. A DEBUG LOG WAS CRASHING THE SAVE PATH. convertRowToObjectEntity() is declared `?ObjectEntity` and does return null for a row it cannot hydrate. Every other call site checks. findAcrossAllMagicTables() did not, and the first thing it did with the result was dereference it — inside a logger->debug() whose only job was to report what had been found. So an unconvertible row did not degrade the search, it killed the request with "Call to a member function getUuid() on null". Live effect: 11 Shillinq RetentionRule objects failed to rematerialise on EVERY repair, because a DocuDesk enrichment listener reached this lookup and one row would not hydrate. The row is now skipped with a warning and the search continues. Re-running the exact repro that failed: ok=3 fail=0. Also completes the system-operation event gate. The first attempt covered MagicMapper::insert()/update() and missed the BULK dispatchers in SaveObjects and the batched-update path, so a configuration import kept fanning out to eight apps while the gate looked applied. That is the second time this session a gate read as correct and was not, which is why both are now asserted rather than assumed. * fix(schemas): allow a nested property to omit 'type', as JSON Schema does A schema with no `type` means "any type" in JSON Schema. OpenRegister rejected it outright, and that was not a lenience worth defending — it forced authors to declare a type they do not have. procest's CMMN sentry is the case that exposed it. `ifPart: {field, operator, value}` compares `value` with LOOSE equality against bool/string/int by explicit design ("a sentry author should not have to match PHP's strict type rules"), requires an ARRAY for the in/notIn operators, and numeric for gt/lt. No single type is honest there. Requiring one would have meant writing a lie into the schema; refusing the omission instead failed the entire import. Type stays REQUIRED at the top level, because those properties become columns: mapColumnTypeToSQL() takes a `string $type` and receives $column['type'] directly, so a typeless top-level property is a TypeError during table creation, not a permissive read. Nested properties are stored inside a JSON column and derive nothing, so the omission costs nothing there. Depth is the discriminator — validateProperties() builds '/name' for a top-level property and appends per level. Controlled in both directions: a top-level typeless property is still rejected, a nested one is accepted, and procest's real caseModel schema — the one that has been failing every repair — now validates. * refactor(logging): reserve info for events, not for narration OpenRegister emitted 700 info calls against 514 debug — inverted, because info was being used for "something happened". A repair run was consequently a wall of lines reporting that nothing had changed, and the one line that mattered was indistinguishable from the 300 that did not. Now 460 info / 745 debug. What moved, and the rule applied: FilePublishingHandler, UpdateFileHandler — step-by-step narration of a single method ("Original file parameter", "After cleaning", "Object folder path", "Attempting to get file"). All debug. ImportHandler — 50 -> 4. The per-app decisions ("Skipping {app}: config content unchanged") describe the MOST COMMON outcome of a repair; reporting the non-event at info is what made the log unreadable. Kept: a register was created, an update applied against the version ordering, the seed-data summary with its counts, and an app's version actually changing. SaveObjects — including one line literally labelled "DEBUG - ..." emitted at info. Kept the Wave-12 safeguard REJECTION: a refusal to write is what someone comes to the log to find. TextExtractionService, ConfigurationController, FolderManagementHandler, ConfigurationCheckJob, CrudHandler, OrganisationService — same treatment, with a state change (risk level), an outcome (notifications sent), and two reached-but-unimplemented paths promoted back. One demotion was reverted rather than pushed through. TextExtractionServiceDeepTest asserts that "Object no longer exists, skipping extraction" logs at info, with a comment saying so. That is a deliberate contract — the line explains why queued work did NOT happen, and silence there looks like the job never ran. The level is now justified in the code instead of only in a test. * test(objects): pin the bulk save path's system-operation gate The first attempt at this gate did not cover this path. Gating MagicMapper::insert()/update() looked complete — a live probe showed one event outside SystemOperationContext and zero inside — while a configuration import carried on fanning out to eight apps through emitChunkSideEffects(), which the probe never touched. The gate read as applied and was not. A live re-probe could not settle it either, and the way it failed is the point: the bulk path defaults to `_events: false`, so the negative control dispatched nothing and the "zero inside the context" result proved exactly nothing. A second attempt with `_events: true` was then rejected by the bulk safeguard (BulkSafeguardException), and a third with an admin session ran past ten minutes — because a real session wakes the very fan-out being measured. Hence a unit test, with the control in the file rather than in a separate run: - outside a system operation the emitter dispatches (without this, an emitter that never dispatched anything would look identical to a working gate) - inside one it dispatches nothing - and events RESUME afterwards, since a suppression that outlived its context would silence every later save in the request — the same outage as the fan-out, reached from the other side and far harder to notice Replacing the gate with `if (false)` turns the second and third red while the control stays green. * chore(docs): regenerate features.json The Features Check gate runs the shared extractor with --check and failed on this branch. Regenerated with the same script the gate uses (.conduction-shared/scripts/extract-features.py), so the committed file and the gate's expectation agree. * fix(quality): satisfy the gates, and cover what the coverage gate was right about phpcs, phpstan and psalm each caught something real rather than cosmetic. psalm's AssignmentToVoid was the sharpest: insertObjectInRegisterSchemaTable() now returns the generated id, but its docblock still said `@return void`, so static analysis was reading the OLD contract while the code returned an int. A docblock that disagrees with its signature is worse than none — it is the version tooling believes. phpstan then found the loose end from a reverted decision: PropertyValidatorHandler kept the logger it was given for the permissive-format behaviour that no longer exists, so the dependency was written and never read. Removed rather than suppressed; an injected collaborator nothing uses is a claim about the class that is not true. The coverage gate was also right, and its baseline may never be lowered, so this earns it back with the two things that genuinely had no tests: UserFormat — the negative case IS the format. A user id is syntactically just a string, so only the backend can say it names nobody; the tests assert the unknown-user rejection, that blank and non-string values never reach the backend at all (coercing 42 to "42" would turn a type error into a lookup miss that reads as "no such user"), and that whitespace is trimmed. The abandoned-lock recovery — conservative in one direction and decisive in the other, and both are asserted. A lock held moments ago is left alone, because stealing one a live pass still holds puts two writers in the chain and reintroduces the fan-out the lock exists to prevent. A lock with no recorded timestamp is also left alone, failing safe rather than guessing — which is why the two left by an interrupted upgrade had to be cleared by hand. A lock held longer than any bounded pass could run is broken and taken, and a break that itself fails reports failure rather than handing the sweep a lock it does not hold. * feat(flow): a node is the action, an edge is sequence, and a token has one exit The engine's authoring format was its own intermediate representation: a node was a Petri-net PLACE carrying no behaviour, and the EDGE carried `type` and `config`. `FlowDefinitionBuilder` threw on a node carrying a step, and its own comment said why the mistake kept happening — "node-shaped authoring is the natural mistake, BECAUSE THAT IS HOW A GRAPH EDITOR PRESENTS A FLOW". It diagnosed the defect and declined to treat it. Three fleet graphs were authored that way, ran, reported COMPLETED, and did nothing. So the model inverts. A node carries the step; an edge says what runs next. The Petri net survives as the lowering: node N -> transition T_N carrying N's type/config, plus place in(N) edge A -> B -> in(B) added to T_A's targets no outgoing -> terminal place end(N) no incoming -> in(N) is initial join: true -> one input place per incoming edge Places are named after their node, which is load-bearing twice: per-item routing matches an item's tag against the output PLACE name, so a prefix would silently drop every routed item into an empty branch; and the marking is the user-visible answer to "where is this run?". CONVERGING EDGES ARE A MERGE, NOT A JOIN. The Hydra sequencer reaches its exit from several mutually exclusive paths — lowering those to a join would require all of them and deadlock every run, while still producing a valid definition. So `in(N)` is shared; synchronising is opt-in via `join: true`. CONDITIONS LIVE ON THE NODE, AS NAMED EXITS. A node declares its branches (`exits: [{id, condition?}]`) and an edge says which it leaves (`fromExit`). That is what lets a node have several exit points, and what lets an editor draw one port per branch — the branches exist before any line does, which an edge condition could never manage. A TOKEN IS UNIQUE AND EXCLUSIVE. Exactly one exit is taken per firing, chosen in declaration order with the unconditioned exit as the else. symfony/workflow marks every output place, so the unclaimed ones are withdrawn after apply(); without that the losing branch simply ran an iteration later, with no error. AND THE ELSE IS MANDATORY. A node that conditions its exits must declare one, refused at build time by name. A token with nowhere to go does not fail — the run stops, reporting nothing, which is indistinguishable from a flow that finished. A test asserted exactly that as correct behaviour ("ends the run cleanly"); it now asserts the refusal, with a positive control. The old shape is REFUSED, not reinterpreted: any edge carrying a `type` names itself and points at the migration. Accepting both would let a half-migrated flow run, skip the step nobody claimed, and report success — the original defect wearing a migration as a disguise. `FlowNodePreflight` walks nodes. Left on edges it would inspect a list where nothing carries a type, find nothing, and call every document valid without having looked — a validator that cannot fail is worse than none. 404 flow tests green (399 at baseline). The 10 AppHost failures in the full run are pre-existing and unrelated — verified by stashing this change. * fix(flow): validate must refuse a pre-inversion document, not report it valid Moving the preflight onto nodes closed one hole and opened another. An un-migrated flow carries every step on an EDGE, so the node walk found nothing to inspect, produced no findings, and the report said "valid" — about the one document shape the engine will certainly refuse. Measured live: `POST /api/flow/validate` on the real Hydra sequencer returned `valid: true, blocking: 0` while `FlowDefinitionBuilder` would refuse it outright. The editor's "Check this flow" button would have told an author their un-migrated flow was fine. The pre-inversion check now runs in `inspect()` as well as in the builder, and returns early: every later finding would be about a document in a shape nothing reads, and burying the one actionable message under sixteen others helps nobody. The sequencer now reports all 16 steps by name, each pointing at the migration. Covered by a test AND its positive control — the same flow validating once the step moves onto the node — because a refusal test is otherwise satisfied by a preflight that refuses everything it is shown. * feat(bulk): give the streaming write path a caller, and fix the gates it surfaced STREAMING BULK UPSERT WAS UNREACHABLE. Hydra's orphaned-write-capability gate found SaveObject::saveObjectsStreaming() and clearReferenceValidationCache() with ONLY test callers — implemented, unit-tested by calling the class directly, and reachable from no production code. Checked rather than assumed: the gate is right, not a false positive. They are a matched pair built as the prerequisite for a streaming import that was never built. routes.php still carries the epitaph: "The objects import route was also removed — use the registers import endpoint instead." Wired into BulkController behind an opt-in `stream` flag, defaulting to today's behaviour. Opt-in because the two paths have OPPOSITE trade-offs, and defaulting either way would be wrong for half the payloads: default ultraFastBulkSave — fastest writes, but never consults the reference-validation cache, so rows that reference each other cost N×M round-trips resolving them. stream each row through saveObject(), which engages that cache; repeated targets resolve from memory, the payload is consumed lazily, and a failed row is recorded rather than failing the call. Choosing automatically would need a size/reference threshold nobody has measured, so the caller decides. ObjectService clears the reference cache at the batch boundary, which is exactly what clearReferenceValidationCache() was written for. SPEC for the UI half. widget-record-import covers dropping a spreadsheet of RECORDS on a register. It is deliberately NOT the file widget's streaming upload: saveObjectsStreaming() streams rows shaped like saveObject() input, and file bytes never pass through it. The two compose — dropping 200 PDFs is a FileService concern, the 200 resulting objects are what this streams — and keeping them apart stops the file widget growing a record-parsing responsibility it has no business owning. The spec makes column mapping explicit (a silently dropped column is the failure that makes imports untrustworthy), requires a dry run, and requires failed rows to export in the input's shape so a user can fix and re-drop only those. GATES this diff surfaced, all pre-existing and all now fixed: gate-46 Dead @SPEC anchors pointing at archived change dirs, in seven files this branch happened to touch. Repointed at the canonical specs they should have named — @SPEC targets openspec/specs/, never a change dir. gate-28 Two files carried `@license AGPL` while their own SPDX header and composer.json both said EUPL-1.2. Not a licence change: a stale tag contradicting the file's own identifier. Also adds the re-chain command's missing tests, which cover the part that matters about a destructive repair — that it refuses without consent, writes nothing under --dry-run, and exits FAILURE when the chain is still broken afterwards rather than reporting success on a repair that did not take. * refactor(bulk): extract writeBatch() from save() save() had grown past PHPMD's length limit once the streaming path landed. Move both write paths into a private writeBatch(), which also gives the stream/ultraFastBulkSave trade-off one place to be explained instead of a comment block wedged inside the request handler. The psalm-return annotation on save() described only the non-streaming shape, so it no longer matched what the method can return; the extracted method carries a plain JSONResponse return type instead. Also adds the class-level @SPEC tag phpcs was warning about (the file-level docblock had it, the class docblock did not). * i18n: translate the audit/flow settings strings into all 36 required locales The l10n parity gate requires every European locale to carry every English source key. The log-integrity settings section and the flow defaults panel added 46 new strings, and two older keys ("Add Application", "Loading organisations...") had never been translated, leaving 48 missing per locale. Also fixes how plurals are stored. @nextcloud/l10n's translatePlural() looks up "_<singular>_::_<plural>_" and indexes the resulting array with the locale's own plural function. Two things were wrong: - nl and tr kept their plural arrays under the *singular* key, where translatePlural never finds them. translate() falls back to element [0], so the plural form of all six n() call sites was unreachable — "Verwijder 5 object" rather than "objecten". Re-keyed to the identifier translatePlural actually reads. - The new "%n entry has no hash yet" pair existed only as two flat strings. That resolves via the fallback path but collapses every locale to a two-way split, which is wrong for the thirteen locales with more than two plural categories. Each now carries a full form array: three for the Slavic locales, four for Maltese and Slovenian (which has a dual), five for Irish. Verified per locale that no existing key was removed and no existing value changed, that each file keeps its own nplurals rule, and by resolving the new plural through each locale's rule for n = 1, 2, 3, 5, 11, 21. * refactor(flow): split the engine and builder along the seams they already had The action-node work grew FlowEngine by 312 lines and FlowDefinitionBuilder by 336, putting seven PHPMD violations on the branch that development does not have — class length, class complexity, and three on one method. Rather than raise the thresholds or baseline the findings, the classes are split where the code was already separable: FlowTokenRouter which exit a token takes, and which places it reaches FlowItemPlacement which items sit on which place, and which travel FlowGraph what a node is called as a place; which edges touch it The seams are not arithmetic. Exit selection reads the DOCUMENT — nodes, exits, edge conditions — while placement reads the MARKING, and the two were only ever coupled through the taken-exit list they hand each other. The graph helpers are pure functions over ids that decide nothing. The moved groups called nothing but each other, so no service dependency moved with them. FlowNodePreflight::inspect() was over on three counts at once (length, cyclomatic, NPath); extracting its two loops fixed all three. The two collaborators are constructor parameters, added LAST and defaulted. Three unit tests construct the engine positionally and one passes the oversight registry third, so a parameter inserted ahead of it would have silently rebound that argument — leaving a test that passes while checking nothing. phpmd.baseline.xml gains StaticAccess entries for the two new files. That is not new debt: FlowEngine already carried a file-level entry for the same calls to FlowExpression and FlowItems, and moving code should not re-open a decision already taken. Verified: phpmd over all of lib reports nothing; phpcs, psalm and phpstan clean on every file touched; 406 flow tests pass. Confirmed the tests can actually fail — inverting the exit condition in the extracted takenExits() breaks four of them. * docs(spec): write the register-folder requirement two services already cite gate-46 flagged `#object-register-folder-management` in RegisterService and FileService as unresolved, and it was right: five `@spec` tags pointed at a requirement nobody had written. The behaviour is real and load-bearing — registers, schemas and objects each own a backing Nextcloud folder, provisioned on demand — so the fix is the missing requirement, not a retag to something that does not describe it. Written from what the code does: provisioning on create, healing of legacy null/string folder values on update, nesting of object folders under their register's, idempotent creation, and a folder failure that is logged without aborting the write. * docs(gate-57): mark clearCurrents live, with the cross-repo callers named gate-57 reported ObjectService::clearCurrents() as an orphaned write capability. It is not: openconnector's EndpointService calls it from six sites before each fresh lookup. The gate builds its caller index from the repo under scan, and CI checks out openregister alone, so a method whose only callers live in a sibling app reads as dead. This is the exact case the gate's own docblock records — hydra#106, against this very method — noting that acting on the verdict would have broken endpoint rendering. The documented `@orphaned-write-capability exclude` annotation is the mechanism for it, so it is used here with the call sites named rather than a bare assertion, so the next reader can check the claim instead of trusting it. The gate stayed quiet locally because sibling apps are checked out next to this one and its index found them. * ci: state the full-coverage opt-out instead of inheriting the new default The shared workflow's `hydra-gates-require-full-coverage` default flipped to `true` on .github main this afternoon, so this repo picked it up mid-PR: every gate PASSES, and the job fails anyway because two gates did not report. Neither is a defect this branch introduced. gate-33 consumes the axe report, and `enable-axe` is deliberately off here for the reason recorded three lines below — vanilla Nextcloud already carries serious/critical axe violations, so switching it on alongside the gates would confuse "this repo has an accessibility defect" with "core does". gate-4 likewise produced nothing under the pinned v1.0.1. Recorded as an explicit `false` with the condition for removing it, rather than left to inherit, so the next reader sees a deferral with a reason attached and not a control that quietly went missing.
…failing CI (#2349) v1.0.1 is `f4d9756` (2026-08-03) and predates three gate fixes, so every Hydra Gates run this repo has ever made executed a script in which 16 gates reported PASS when their helper never ran (ConductionNL/.github#147), gate-33 had no axe report to read and never said so (#148), and gates 6 and 7 reported PASS on an empty scope (#149). The tick was identical either way, which is why nothing in this repo's history shows it. That pin is now also RED, and the mechanism is worth writing down. quality.yml is referenced `@main` while this package is PINNED, so the two can desync. #164 flipped `hydra-gates-require-full-coverage` to default true in the shared workflow, and that flag requires a gate to DECLARE itself not-applicable. v1.0.1 contains ZERO `_skip` calls; v1.3.0 has 36. v1.0.1 has no vocabulary to declare, so every absent prerequisite became "DID NOT RUN" and failed the job — for gates the repo has no subject matter for. Measured on this branch, diff-scoped against origin/development exactly as CI scopes it, in a private mount namespace with a private tmpfs (the runner's ~50 /tmp/hydra-gate-*.log paths are shared state and two concurrent runs corrupt each other's counts, .github#158 item 6): v1.0.1 exit 98 FAIL — "GATES THAT DID NOT RUN: 24 33" v1.3.0 exit 0 PASS — those gates named NOT APPLICABLE, with reasons Independently confirmed end-to-end: doriath#160 changed this one line and nothing else, and its Hydra Gates job went failure -> success. v1.3.0 is `f7eaf2a` = .github@main at the time it was cut. Refs ConductionNL/.github#159
…able (#2351) The flow page had no way to save, run, enable, add a step, or see run history — while its own empty state read "Add a step from the sidebar". None of it was missing. `CnFlowSidebar` implements the whole panel (step palette, Name/Description/Trigger, Enabled, Save, Run now, Recent runs), `FlowDetailSidebar` wires save/run to the store, it is registered in registry.js, and the manifest declares `sidebarComponent: FlowDetailSidebar` on the flowDetail page. CnAppRoot does resolve that key. It could still never render. CnAppRoot only falls back to the manifest's sidebarComponent as the DEFAULT content of its #sidebar slot, and this app fills that slot itself with SideBars — so consumer content wins by Vue's ordinary slot mechanic, exactly as CnAppRoot's own docblock warns. SideBars had no branch for /flows, so on a flow route it rendered nothing at all and the manifest key was live config with no effect. Adding the branch is the whole fix. Verified in the browser against a rebuilt bundle: the sidebar renders with Steps, Flow (Name, Description, Trigger, register/schema restriction, Enabled), Recent runs, Save and Run now. Built a flow from the palette, saved it — the route advanced from /flows/new to the returned uuid, so it persisted — then ran it. FlowRunWorker picked the queued run up and the history moved to `completed`, matching the row in oc_openregister_flow_runs.
…lready lists (#2352) 39 files of PHPMetrics HTML report scaffolding were committed despite .gitignore:30 already listing /phpmetrics-deps/. Nothing in the repo reads these paths — PHPMetrics writes to phpmetrics/ (composer.json) and phpqa/phpmetrics (.phpqa.yml), never phpmetrics-deps/. They also carried third-party code into an EUPL-1.2 repo, including js/clusterize.min.js (GPLv3, (c) 2015 Denis Lukov) and MIT-licensed js/sort-table.min.js and css/milligram.min.css. Untracked only; the files stay on disk and remain ignored.
* chore(license): normalise licence declarations to EUPL-1.2 OpenRegister declares EUPL-1.2 in composer.json, package.json, appinfo/info.xml and LICENSE, but 175 files still carried AGPL-3.0 licence tags. This aligns every file-level licence declaration with the licence the project actually ships under, clearing hydra gate-28 (license-triangle), which failed with 25 files under lib/. Changes are licence-identifier only: - @license AGPL-3.0-or-later <agpl-url> -> EUPL-1.2 <eupl-url> (88 tags) - @license AGPL-3.0-or-later (no URL) -> EUPL-1.2 (61 tags) - @license AGPL-3.0 / URL-first shape -> EUPL-1.2 <eupl-url> (3 tags) - SPDX-License-Identifier: AGPL-3.0-or-later -> EUPL-1.2 (64 tags) - stale AGPL URL appended after an already-correct EUPL-1.2 id removed in 6 lib/Service/File handlers (12 tags) - ConfigurationSettingsHandler reported OpenRegister's own licence as 'AGPL' when info.xml lacked one; fallback now 'EUPL-1.2' 29 files carried TWO @license tags; gate-28 only reads the first, so every occurrence was replaced rather than just the blocking one. No @copyright, @author or SPDX-FileCopyrightText line was touched. Deliberately NOT changed (reported for an explicit decision): - 12 files whose SPDX-License-Identifier is paired with 'SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors' (occ-scaffolding residue). An SPDX pair is a single statement about a named holder, so relicensing it is not ours to do mechanically. None affect gate-28. - phpmetrics-deps/ (39 vendored third-party report assets, incl. a GPLv3 file) and composer-setup.php - third-party, not ours. Unit suite before and after, identically conditioned (PHP 8.3.32): 16030 tests / 35963 assertions, 0 failures, 0 errors - unchanged. * chore(license): normalise the 12 remaining AGPL SPDX identifiers to EUPL-1.2 gate-28 reads only the first @license tag and ignores SPDX entirely, so these 12 declarations were invisible to it. Eleven of them sat directly above a Conduction '@license EUPL-1.2' PHPDoc tag in the same file — the files contradicted themselves. Also corrects openapi.json's info.license.name and the exapps/README.md licence line. Adjacent SPDX-FileCopyrightText lines are deliberately untouched. --------- Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
…ng it (#2356) PR #2350 flipped 'SPDX-License-Identifier: AGPL-3.0-or-later' to EUPL-1.2 in 12 files whose adjacent line reads 'SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors'. That asserted EUPL-1.2 over a third party's copyright — a false licence claim, and worse than the AGPL contradiction it replaced. This deletes the residue block rather than relabelling it. Evidence that the block is copy-paste residue from the Nextcloud app template, not a real Nextcloud GmbH copyright: - It occurs in exactly 12 source files; the only other 'Nextcloud GmbH' strings in the repo are dependency author fields in bom-npm-test.cdx.json, which is an SBOM of real Nextcloud npm packages and is left untouched. - 11 of the 12 already carry their own '@copyright 20xx Conduction B.V.' PHPDoc tag in the same file, directly contradicting the SPDX line. - The full git history of all 12 files contains no Nextcloud GmbH author: only Conduction people (Barry Brands, Conduction Development Team, Remko, Robert Zondervan, Ruben van der Linde, Thijn) and the CI bot. - The repo's other ~270 PHP files carry Conduction copyright only, with no SPDX pair at all — this is the app-template scaffold the rest shed. tests/Unit/Controller/SettingsControllerTest.php had no other licence header, so it gains the repo's standard test-file PHPDoc rather than being left bare. No licence value is changed by this commit; the false claim is removed.
…w records its last run (#2354) * feat(flow)!: a path ends deliberately or says it is broken, and a flow records its last run A node with no outgoing edge was a silent success. Its token arrived, the step ran, the engine found no enabled transition, and the run was recorded COMPLETED — so the author saw a green run that had not done the work. Nothing failed, so nothing was logged. That is the defect this closes. Ending a path deliberately is now something a node SAYS, two ways, OR-ed: IFlowTerminalNode a marker interface on the TYPE, resolved through FlowNodeRegistry::isTerminal(), so a terminal step contributed by openconnector or hermiq needs no OpenRegister change. StopNode implements it. "exit": true on the node instance, for a sink whose step type is an ordinary action — which is what every migrated flow has, because that WAS a legitimate end of a path under the old place-and-edge reading. They are OR-ed and never AND-ed: requiring both would make every migrated flow depend on a registry the migration cannot see. A marker interface rather than a method on IFlowNode, for the reason IFlowNodeConfigKeys already documents: implementations live in other repos, and widening the interface fatals those apps on load. WARN ON SAVE, REFUSE ON RUN Saving a half-wired flow succeeds and returns the warning. A disconnected graph is the normal state of one being authored; refusing to store it would force the author to build the graph in an order that is never disconnected, which no editor can require. Running is refused. The guard sits in FlowRunService::queue(), which is the one choke point every dispatch path passes through — manual, trigger, schedule, MCP, the workflow-engine operation and a sub-flow call. Guarding FlowService::run() instead would have left cron-fired flows unguarded, and those are most of them. On refusal no FlowRun is created, and the verdict is written onto the FLOW (status/status_message naming the nodes) precisely because there is no run to read: that is what makes a refused flow distinguishable from one nobody has triggered. An accepted run clears a stale error back to ok. The schedule sweep catches the refusal PER FLOW. It iterates every due flow, so letting it propagate would abort the sweep and stop every later flow from firing — one broken definition silently disabling the rest, presenting as "cron stopped working" rather than as a fault in a named flow. A typeless node is deliberately NOT reported here. FlowDefinitionBuilder already refuses it by name, and two findings on one node for one defect is how a warning list becomes noise. LAST RUN Six nullable columns, no backfill. NULL lastRunAt means "has never run" — a value derived from run history would assert a history the column did not record. Written only when a run reaches a terminal state, so the flow list answers "how did it last go?" rather than "it hasn't finished". Also adds the canonical openspec/specs/flow-engine/spec.md, which did not exist — it lived only inside changes/ — so @SPEC can target a canonical path. Not done, and stated in tasks.md rather than quietly skipped: the schedule and trigger dispatch wiring, and the last-run write-back, are not yet pinned by tests. Both need FlowRunService built with a mocked container. The suite could not be run locally: once lib/base.php loads, NC's autoloader resolves OCA\OpenRegister\* to the INSTALLED app, not the working copy — measured with ReflectionClass::getFileName(). CI's "copy the app out" recipe does not prevent that; CI is immune only because it deploys the code under test first. Run locally against an older deployment it reports on the deployed app. CI is the authoritative gate here. BREAKING: a flow with a dead-ended node is now refused at run time instead of completing silently. Mark deliberate sinks "exit": true, or give them a terminal step type. * fix(flow): import IFlowTerminalNode, and let the dialect fixtures end deliberately StopNode gained `implements IFlowTerminalNode` without the matching `use`. StopNode lives in ...\Service\Flow\Nodes, so PHP resolved the bare name relative to THAT namespace and looked for ...\Nodes\IFlowTerminalNode. `php -l` cannot see it — the syntax is valid and the failure is at class-resolution time — so it surfaced as 16 identical PHPUnit errors plus phpstan, psalm and phpmd all reporting the same unknown interface. My local phpstan/psalm run passed because I listed the changed files by hand and StopNode.php was not among them: the check excluded the one file with the bug. The fixtures in FlowNodeConfigDialectTest, FlowNodeConfigVocabularyTest and FlowNodePreflightRegressionTest are single nodes or chains with no outgoing edge from their last node, so the new connectivity check reports them — correctly. Those suites are about a node's config DIALECT and the registry, not about connectivity, and each asserts an exact finding count; an unrelated second warning made them count two different things. Marking the last node `exit: true` makes each fixture a COMPLETE document rather than suppressing the check, and the dialect suite's positive control still asserts an exactly-empty report. * fix(flow): split the connectivity check out, and document the delegated guard Three gates, three real findings: phpmd — deadEndFindings() reached cyclomatic 13 / NPath 735, and decomposing it pushed FlowNodePreflight past the 1000-line class limit. Both are the same signal: the graph-SHAPE question does not belong in a class that answers questions about each node's TYPE and CONFIG. Moved to FlowConnectivity, which also stops the preflight becoming the place every future flow check lands. Instantiated inline rather than injected, so no constructor changes ripple into the several tests that build the preflight by hand. gate-7 no-admin-idor — FlowController::create/update were pulled into the diff by the savedBody() change and flagged as NoAdminRequired with no guard. The guard is real but DELEGATED, which is the gate's documented false-positive class: update() resolves the uuid through FlowService::find(), so an update to a flow the caller cannot see is refused exactly like one that does not exist, and create() stamps owner/organisation server-side with both outside applyEditableFields()'s allowlist. Recorded with the reason-bearing @no-admin-idor-exempt tag naming the actual guard, following the precedent in EmailsController and FileSearchController. PHPUnit — one more single-node fixture asserting an exactly-empty report, now marked exit: true for the same reason as the others: a lone node with no outgoing edge IS a dead end, and the warning would be right.
…ght this (#2353) * test(e2e): guard the flow controls, the one layer that could have caught this The flow authoring surface shipped unreachable — no save, run, enable, add a step or run history — and every layer was green while it was broken. The components existed, the routes existed, unit tests passed, the manifest validated, and the manifest key that was supposed to mount the panel (`sidebarComponent`) was silently outranked by the app's own #sidebar slot. Nothing short of opening the page and looking for the controls could have found that, so that is what this does. Asserts, in order: the sidebar renders with its palette and actions; the palette is non-empty (an empty one renders the same container, so the count matters); a step added from the palette reaches the canvas; Save persists, proven by the route advancing off `new` to the server's uuid; and Run now creates a run for that flow. It deliberately stops short of asserting the run COMPLETES. Execution is picked up by FlowRunWorker on cron, which does not run in CI — waiting for it would make the spec depend on a background job. That the run is created and attributed to the flow is the part the UI is answerable for. Hermetic per the CI floor's contract: it builds its own flow through the UI and deletes it in a `finally`, so a mid-test failure still cleans up. Verified both ways against a live instance, because a passing assertion is evidence about the assertion until it has been shown to fail: with the fix it passes, and with the fix reverted and the bundle rebuilt it fails on the sidebar assertion with the message written for exactly that case. * fix(e2e): drive the themed buttons the way this repo already learned to CI failed the new spec on the click, not on the app. Playwright resolved the "New flow" button, reported it visible, enabled and stable, scrolled it into view — and then the click action itself timed out, twice, burning the whole 45s budget with the locator perfectly matched. That is the Nextcloud themed-button behaviour `tests/e2e/global-setup.ts` already documents against the login button: "on NC's themed login the styled submit button can swallow the click". The class on the failing control says the same thing out loud — `button-vue--legacy34`. Every click in the spec now goes through one helper that asserts visibility and then dispatches the event, which drives the Vue @click handler that is the actual behaviour under test. It costs Playwright's actionability checks, so the explicit `toBeVisible()` assertions stay: those are what catch a control that is missing or covered, which is the regression this spec exists for. It passed locally three times before CI disagreed — worth recording, because the local pass was the less trustworthy of the two results.
A pinned `hydra-gates-ref` is a silent expiry date on every upstream fix: this repo cannot receive a gate-package change until this line moves. v1.4.0 is the latest tag and the first one that carries `hydra-gates/scripts/axe-run.cjs` (verified absent at v1.3.0), so it is also the first that has ConductionNL/.github#168 axe DOM scoping and ConductionNL/.github#165 gate-46 fix. `enable-axe` is deliberately NOT enabled in this commit. Ordering matters: the ref lands first, enabling axe is a separate decision.
rubenvdlinde
added a commit
that referenced
this pull request
Aug 6, 2026
The standing 'Release: merge development into beta' PR (#1711) has head_ref 'development', so its pull_request run rendered the same concurrency group as a push to development. cancel-in-progress killed the push run, which is the only carrier of the push-only jobs (Coverage Baseline Check, SBOM, Features Extract). Those jobs report 'skipped' on the surviving PR run, which renders like a pass, so the gate never produced a verdict. Suffixes -push on the group for main/development pushes only; feature-branch dedup is unchanged. No gate weakened. Same fix as openconnector#1158.
…ne (#2361) The standing 'Release: merge development into beta' PR (#1711) has head_ref 'development', so its pull_request run rendered the same concurrency group as a push to development. cancel-in-progress killed the push run, which is the only carrier of the push-only jobs (Coverage Baseline Check, SBOM, Features Extract). Those jobs report 'skipped' on the surviving PR run, which renders like a pass, so the gate never produced a verdict. Suffixes -push on the group for main/development pushes only; feature-branch dedup is unchanged. No gate weakened. Same fix as openconnector#1158.
…kflow needs The Hydra Gates job fails with a message that says outright it is not about this repository: hydra-gates-ref <old> does not contain: scripts/lib/check_spec_anchors.py scripts/lib/check_form_labels.py scripts/lib/check_license_triangle.py The reusable workflow floats on @main and calls those scripts BY PATH inside the PINNED package, so a pin older than the scripts cannot run the gates that implement them. A pinned ref is a silent expiry date on every upstream change, and the failure reports on the pin while saying nothing about the code. v1.5.0 is the first tag containing all of them, verified by reading each path at that tag rather than assuming the newest tag has everything. Swept across the fleet: 11 of 13 repos were pinned below v1.5.0 and every one of them was failing this way.
Drops the `hydra-gates-ref:` override from the quality caller so the input falls back to the shared workflow's own default, which is already `main`. This workflow calls ConductionNL/.github/.github/workflows/quality.yml@main. Pinning the gates package to a tag while consuming the workflow at @main splits the two halves apart: the runner moves, the gate package does not. Two fleet-wide incidents came out of exactly that split. * .github#159 — 22 repos were pinned to v1.0.1, which predated the fixes that made 16 gates actually execute. Every one of those gates reported PASS. A check that did not run looks exactly like one that passed. * .github#173 — `require-full-coverage` was flipped to default-on at @main and reached the old pinned runners, which had no coverage accounting to honour it with, so they went red on gates they had no subject matter for. Unpinned, both sides move together and a gate fix lands here without a commit here. The input is still honoured: to hold this repo still for a specific reason, set it explicitly and say why. To roll it back for everyone, revert on ConductionNL/.github main. `enable-hydra-gates` is untouched. The comment block above it kept the part that explains why the tier is on and lost the part that justified the pin. Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
…#2364) .coverage-baseline was read as a floor by the phpunit guard and as an exact target by the push-side staleness check. Together they demand equality with a checked-in constant, which against a moving base branch is not satisfiable: closing "stale" means committing the value the tree will measure after the PR lands. Measured on openregister — committed 58.93, development advanced 16030->16038 tests, merge result measured 58.88, guard reported a 0.05% drop. coverage-guard.php gains --against=<clover.xml>, naming a report measured at the merge base. When present it is the only floor; the committed constant is reported but not enforced. Both numbers then come from one driver in one job, so the xdebug/pcov statement-counting difference cancels rather than being baked in, and the merge base cannot go stale. Ratios are compared as exact integer cross-products, not rounded percentages: at two decimals a one-statement regression read as "unchanged" and exited 0. An empty or zero-statement report is now a hard error rather than 0%, which as the merge-base side would set the floor to zero and pass every drop. Verified on real CI clover artifacts: a genuine 1.44% drop fails, an unchanged tree passes, and adding untested code fails while adding tested code passes.
…thing (#2366) * fix(e2e): the flow-controls spec was green only when it had tested nothing `flow-controls.spec.ts` failed its FIRST attempt on 6 of the 8 CI runs that executed it, always at `waitForURL` after Save, always at 23.9-24.6s — about 4s of real work plus a 20s timeout expiring in FULL. A wait that always expires in full is not a slow round-trip; it is something that was never going to happen. WHY THE SAVE NEVER LANDED `useFlowStore`'s initial state is `emptyFlow()`, whose `name` is `''`. Only `open('new')` names the flow, and `open()` runs at the TAIL of `load()`, behind `await GET /api/flows` — a flow LIST that starting a blank flow does not need. The sidebar is interactive well before that, because `nodeCatalog` was already populated by the flows INDEX page's own `load()`. So there is a window in which the editor invites a Save of a nameless flow, `FlowController::create()` answers 400 "A flow needs a name.", `store.save()` swallows it into `return null`, and `onSave()` therefore never calls `$router.replace`. Measured locally: 9 of 10 runs red, POST body `name: ""`, response 400. That window is a real defect, not a test artefact — a user who clicks Save quickly enough gets total silence, since nothing renders `store.error` and a 400 JSONResponse is not logged. It belongs to @conduction/nextcloud-vue and is reported there; this commit does not paper over it, it makes it detectable. AND THE GREEN RUNS WERE WORSE THAN THE RED ONES Instrumenting what Save actually POSTs, over 14 local runs, split three ways: POST /api/flows 400, name="" -> red (the race above) POST /api/flows 201, nodes=1 -> red at step 4: POST .../run 500 POST /api/flows 201, nodes=0 -> GREEN The spec passed ONLY when the flow it saved had no steps. A one-node `set-fields` flow cannot run at all: since #2354 a path must end deliberately, and `FlowRunService::queue()` refuses a node with no outgoing edge that is not terminal — only `StopNode` is. So every run that genuinely persisted the step failed, and every green run was green because the race had thrown the step away first. The 20s poll dressed that up as "Run now did not produce a run". WHAT CHANGES - The fixture builds the smallest flow this app calls VALID (one terminal `stop` node) instead of one it is obliged to refuse. - Save and Run assert their RESPONSE, not a route and a poll, so a rejection fails immediately quoting the server instead of after 20s naming the wrong thing. Measured: a rejected save now fails in 4.1s, a refused run in 3.3s. - The spec waits for the state a save REQUIRES (the flow has a name) rather than racing initialisation. - `clickThemed` asserts the control is ENABLED before dispatching; a dispatched event reaches a Vue handler whether or not the button is disabled. - Swallowed `cn-flow:` store errors now fail the test by name. No timeout was widened. The three 15-20s budgets are gone, not raised: with the round-trips asserted directly, what remains are 5s router and read-back assertions, against a measured worst case of 92ms. `FlowController::run()` now answers 409 with the offending node ids instead of letting `FlowDeadEnd` escape as a bare HTML 500 — a routine authoring mistake was indistinguishable from the server falling over. CI could not have diagnosed any of this: the config wrote traces to `test-results-ci/`, which the shared workflow's upload step does not glob, and `trace: 'on-first-retry'` captures the RETRY — so for a flake that passes on attempt 2, every trace on disk was of a green run. Both fixed. Reproduction: 9/10 red before, 20/20 green after, on the shipped CI config. Verified not blind by mutation: removing the sidebar branch, blanking the node catalog, rejecting the create and refusing the run each turn it red at the matching assertion. A whole-suite positive control with the app bundle truncated to 0 bytes reds 9 of 13 specs; the 4 survivors are `object-sharing.spec.ts`, which is API-level by declaration. * docs(e2e): point the flow-controls header at nextcloud-vue#607 The two store defects the spec now guards against are filed upstream; name the issue so the next reader can tell a known upstream bug from a new one. * fix(flow): declare the FlowDeadEnd contract phpstan could not see phpstan called the new `catch (FlowDeadEnd)` a dead catch, and it was right about the evidence available to it: `FlowRunService::queue()` throws it and neither it nor `FlowService::run()` said so. That is the same omission that let the refusal reach HTTP as a bare 500 in the first place — a caller who cannot see the throw in the signature does not handle it. Declared on both. phpmd's CouplingBetweenObjects then tipped to 13 on the one added dependency. Suppressed at class level with a reason, matching the TooManyPublicMethods suppression already there and the five other classes in lib/ that carry it: this class IS the flow API surface, and splitting it to lower the count would put two controllers behind one /api/flows prefix.
…InLoopExpression, untrack the phpmd result cache (#2359) * fix(quality): scope the Migration phpmd exclude to lib/, drop a foreign copyright claim, clear the coverage ratchet phpmd-unusedparams.xml carried <exclude-pattern>*/Migration/*</exclude-pattern>. PDepend compiles an exclude-pattern into an UNANCHORED regex (Input\ExcludePathFilter preg_quote()s the pattern, then turns `\*` into `.*`), so that form matches ANY path containing a `/Migration/` segment - lib/Service/Migration/, lib/Command/Migration/, any future lib/*/Migration/. Those are ordinary classes with no interface-mandated signature, so a genuine unused parameter in one would never be reported and the run would still look clean. openconnector carried exactly such a file. openregister has no lib/*/Migration/ directory today, which is precisely why this had to be fixed before one appears: the broad form fails silently and only on the day someone adds the directory. Now `*/lib/Migration/*`, matching the 19 repos that already carry the corrected shape. Twelve files carried `SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors`, scaffolding residue from the Nextcloud app skeleton. The licence sweep in #2350 relabelled the adjacent SPDX-License-Identifier from AGPL-3.0-or-later to EUPL-1.2 - which asserts that Nextcloud GmbH's copyright is EUPL-licensed. We cannot relicense a third party's copyright. Every one of the twelve is Conduction-authored: each carries `@author Conduction Development Team` and `@copyright Conduction B.V.` in its own PHPDoc, and `git log --follow` shows only Conduction committers. The stray holder is corrected to Conduction rather than deleted, so no file loses its REUSE metadata. Side effect, measured by running phpcs on both versions at the identical path: 2 pre-existing "Missing short description in doc comment" errors go away. CountInLoopExpression retired entirely - all 3 baseline entries, all 3 findings. Two are `do { … } while (count($page) === $limit)` where the page is replaced wholesale each iteration and never mutated in the body, so the count is taken once per page into a variable; one is `for ($i = 1; $i < count($rings); $i++)` over an array the body does not touch, so the count is hoisted. Behaviour is identical in all three. .phpmd.result-cache.php untracked and gitignored. It is generated output, and a correctness hazard while committed: `composer quality:phpmd-score` passes --cache, so a stale cache in the tree makes PHPMD replay a verdict for code that has since changed - a gate reporting a result it never computed. .coverage-baseline 58.87 -> 58.93. This was the ONLY red job on development: CI measured coverage that had improved past its own committed baseline. Raising it tightens the ratchet. Unit suite before and after, same container and same vendor: 16030 tests, 35963 assertions, 0 failures, 0 errors - byte-identical totals. * fix(spec): repoint 6 @SPEC anchors that gate-46 could not resolve Hydra gate-46 (spec-anchor-existence) failed on this PR with 6 unresolved targets. All six are pre-existing debt in files this PR already touches, which is what pulled them into the gate's ADR-020 diff scope; none was introduced here. 62 of the other gates passed and coverage was 60 of 60 applicable, so this was a single real failure, not a broken run. lib/Service/VocabularyImportService.php (4 tags) pointed at openspec/changes/skos-concept-registers/... - a CHANGE directory. That change was archived on 2026-07-23, so the path stopped existing the moment it moved to openspec/changes/archive/. A @SPEC tag must target the canonical openspec/specs/ home, which is where the spec lives now; two of the four also carried "#skos-002", which is not a heading, and now name the heading that actually exists. lib/ContextChat/ContentProvider.php (4 tags, 2 distinct anchors) named "#requirement-getitemurl-must-resolve-through-the-existing-deep-link-registry" and "#requirement-initial-import-must-walk-opted-in-schemas-in-batches-and-must- be-re-runnable-via-occ". Neither heading exists; both requirements were merged into one, "Requirement: getItemUrl and initial import reuse existing OpenRegister infrastructure", and the tags were never moved with it. Every target verified against gate-46's OWN two slug rules - slugify() and gh_slugify(), which differ on punctuation inside a word - by resolving each fragment back to the heading text it matches. No overlap with #2355, which repoints a different set of anchors in file-actions. * revert(quality): drop the .coverage-baseline bump — the number drifts with development I raised .coverage-baseline 58.87 -> 58.93 because that was the value CI itself recomputed on development, and Coverage Baseline Check was development's only red job. On this PR it then failed the OTHER direction: Coverage baseline: 58.93% Coverage current: 58.88% FAIL: Coverage dropped by 0.05% Not a regression from this PR. Development moved between my two CI runs — the suite went 16030 -> 16038 tests — so the merge base this PR is measured against computes 58.88, not the 58.93 that development's own HEAD computed earlier. The two jobs also check opposite things: development's runs coverage-guard.php --update-baseline and fails when the committed value is STALE, while a PR runs it plain and fails when coverage DROPS below the committed value. Pinning a number from one tree to satisfy the other is what broke this. So the bump leaves this PR. It belongs in a one-line change computed on development's own HEAD, at a moment development is not mid-merge — not carried in on a PHPMD branch whose merge base keeps moving underneath it. The job was red before this branch existed and is unaffected by it either way. Nothing is weakened: .coverage-baseline returns to development's committed 58.87, exactly as found. * fix(quality): retire CountInLoopExpression from the baseline and drop 10 entries for a deleted file phpmd.baseline.xml 519 -> 506 entries, and one rule family leaves ENTIRELY. A PHPMD baseline entry is scoped to (rule, file) - optionally a method, NEVER a line - so one entry covers every current AND future violation of that rule in that file. It is an open licence, not a record. Shrinking the count is therefore not the point; getting a family to zero is, because only then does a NEW violation of that rule fail CI. CountInLoopExpression: all 3 entries removed. The 3 findings behind them were fixed in this PR, not suppressed. Verified with a single-rule ruleset over all of lib and NO baseline in play: 3 findings before, 0 after. lib/Service/Flow/FlowActionService.php: 10 entries for a file that no longer exists (WeightedMethodCount, CouplingBetweenObjects, LongVariable, ShortVariable, MissingImport, and Cyclomatic/Npath on run/runNamedFlow/runAction). Deleted with the file; suppressing nothing; free to remove. The other 509 entries are all LIVE and were left alone. I checked, and the first answer was wrong in an instructive way: matching baseline entries against the report by rule name reported FIVE families - NPath (62), LongMethod (33), WeightedMethodCount (32), LongParameterList (14), LongClass (6) - as "entirely stale", 147 free deletions. They are not. The baseline stores the rule CLASS (PHPMD\Rule\Design\LongMethod) while the XML report writes the rule NAME (ExcessiveMethodLength), and those five differ. With the mapping applied the accounting closes exactly: 767 true findings, 767 suppressed by live entries, nothing unexplained. Uniformity across five independent families was the tell. Measured with the baseline file MOVED ASIDE, not by dropping --baseline-file: PHPMD auto-discovers phpmd.baseline.xml sitting next to the ruleset and applies it either way, so un-flagging it yields a silently baselined run that looks clean. Independently corroborates #2347's 749 + 16.
…editor state, and two wrong assertions (#2365) * fix(migration): bump the app version so the flow-status migration can run Running any flow on an existing instance 500s: SQLSTATE[42703]: Undefined column: 7 ERROR: column "status" of relation "oc_openregister_flows" does not exist Version1Date20260805100000 adds `status`, `status_message` and the four `last_run_*` columns, and it landed in #2354 — but `appinfo/info.xml` last changed its version in #2265, earlier. Nextcloud runs an app's migrations on a version CHANGE, so an instance already sitting on 0.2.17-unstable.24 never ran it and never will. A fresh install is unaffected, because it runs every migration from empty. That is exactly why nothing caught this: CI installs fresh, so CI is green while every existing instance cannot run a flow at all. Confirmed on the dev instance: the three columns were absent, `occ upgrade` after this bump created them, and a flow that had been 500ing then queued and ran. * chore(deps): @conduction/nextcloud-vue 2.2.0-vue3.5 Carries ConductionNL/nextcloud-vue#605: CnFlowDetail now reloads when the route names a different flow. Without it, opening a flow and then moving to another one or to `new` left the previous flow in the store as well as on the canvas — and `save()` picks PUT over POST from `flow.id`, so Save on a page presenting itself as a blank new flow issued a PUT against the flow just left, overwriting it. Verified against this bundle, on the same path that reproduced it: with "Hydra label transition" open, moving to /flows/new now shows an empty canvas and a Name field reading "New flow" rather than "Hydra label transition". `vue` is pinned back to ^3.5.18 by hand. `npm install` rewrote it to ^3.5.0 to match the new package's own range, which then contradicted the `overrides` entry — CI's npm 10.8.2 refuses that outright: npm error EOVERRIDE Override for vue@^3.5.0 conflicts with direct dependency Local npm 11 accepts it silently, so `npx npm@10.8.2 ci --dry-run` is what caught it: clean on development, EOVERRIDE with the rewritten range, clean again once restored. * fix(e2e): the flow spec was asserting two things that are not true Both surfaced once the spec ran against a fully migrated instance. WAITING FOR A NAVIGATION THAT NEVER HAPPENS. `page.waitForURL` defaults to `waitUntil: 'load'`, and this app uses a HASH router — a hash-only change fires no navigation event, so the wait timed out on a save that had in fact succeeded and moved the route. Polling the URL asserts the same thing without depending on an event the router does not emit. A ONE-NODE `set-fields` FLOW IS NOT RUNNABLE, AND SHOULD NOT BE. The spec built one and expected Run to produce a run. FlowRunService refuses it, with a good reason: a node with no outgoing edge that does not end the flow means "a run would stop there and still be reported as completed". The spec was asserting the absence of a guard that exists on purpose. It now places a `Stop` node — a terminal step type, so a single one is a complete flow — which saves AND runs. Confirmed by hand first: the same sequence with `Edit fields` is refused and with `Stop` returns one run. * ci: move the gate pin to v1.5.2 so gate-28 stops failing empty-scope PRs Hydra Gates reported 59 gates green and failed anyway, because gate-28 counted itself APPLICABLE and did not run: [gate-28] license-triangle: SKIPPED (structural) — lib/ exists and composer.json declares license=EUPL-1.2, but 0 in-scope lib/**/*.php file carried an @license ... so NOTHING was compared Which is true, and not a gap. This PR's diff is an info.xml, a package.json, a lockfile and one spec — no PHP at all. Nothing was in scope, which is ADR-020's diff-scoping working exactly as intended, and NOT APPLICABLE is the classification that exists to say so. With require-full-coverage now on by default, the misclassification fails every PR that touches no lib PHP. .github#182 fixed it, and the fix had been sitting on main untagged since. Every consumer pins a tag, so an untagged fix reaches nobody — v1.5.2 was cut at that commit for this bump. * ci: bring coverage-guard.php up to the version the shared workflow requires The PHPUnit job failed after all 16038 tests passed: scripts/coverage-guard.php predates merge-base comparison. Update it from ConductionNL/.github before enabling the ratchet. The shared workflow probes `coverage-guard.php --capabilities` for `against` before trusting the ratchet on a pull request, and refuses rather than silently skipping — a check that did not run must not look like one that passed. This repo's copy is the 1.6KB version that has no such flag. Its own comment says "every repository that sets enable-coverage-guard was updated before this workflow changed, so this should never fire; if it does, the repository is the thing to fix." openregister was missed by that sweep, which is why it fires here. Taken verbatim from nldesign, which carries the current version — and whose docblock cites THIS repo as the measured case for why the merge-base floor had to replace the committed constant. The fix was written for openregister's problem and openregister never received it. `.coverage-baseline` (58.87) is untouched; under the new script it is reported for information on a PR and the merge-base measurement is the floor. procest carries the same stale 1.6KB copy and will hit this the moment it enables the ratchet. * chore: re-run CI The coverage-guard fix pushed at 07:37 did not produce a Code Quality run — GitHub emitted no `synchronize` for it, so the PR's checks stayed pinned to the previous commit's failure. A workflow_dispatch on the same head passed (23 jobs, 0 failures), but a dispatch run is not what the PR reports, and merging over a check describing older code is the habit this repo has already been bitten by (#2227, #2228). This empty commit exists only to make the PR's own checks describe the code that will actually merge. * fix(e2e): wait for the flow to load, and say why a save was refused CI failed on "Save did not move the route off `new`", which is a symptom three different faults share: the request was refused, the request was never sent, or it succeeded and the router did not follow. The spec could not tell them apart, so the first job was to make it say. It now records every /api/flow* call with its status and reads the sidebar's error text, and re-throws with both. That turned an opaque timeout into: flow API calls: ... | POST api/flows -> 400 | ... sidebar error : (the sidebar showed no error) A 400 on create is "A flow needs a name." — and the Name field shows "New flow", so the name was missing at the moment of the POST rather than missing from the form. `load()` resolves the catalogues and the flow independently: the palette can be populated and clickable while `open('new')` has not yet stamped the default name, and a node added in that window is saved against a nameless flow. Driving the UI by dispatched events, with no human pause anywhere, lands in that window almost every time — which is why CI reproduced it and a hand-driven browser did not. The spec now waits for the Name field to carry a value before adding a step. That is the observable proof the flow has loaded, not merely that the panel has rendered. Three consecutive local runs pass. Worth noting separately: the store logs a failed save to the console and renders nothing, so a user gets a Save button that silently does nothing. That is a real gap in CnFlowSidebar, not something this spec should paper over.
Carries ConductionNL/nextcloud-vue#608, which closes #607: the flow editor no longer lets a user press Save before the store has a flow to save. The window was real and wide. `emptyFlow()` has `name: ''`, only `open('new')` supplies the default, and `open()` ran behind `await GET /api/flows` — a list a blank flow does not need — while the sidebar was already rendered and Save already enabled. Saving in that window posted `name: ""` and the API answered 400 "A flow needs a name." A refused save rendered nothing at all, so the user saw the button flicker and no more. The same late `open('new')` also reset the flow, wiping a step already placed on the canvas. Verified against this bundle, in the window itself: clicking Save 120ms after opening a blank flow — where a 400 was previously reproducible 9 times in 10 — returns 201 with `name: "New flow"` and the route advances to the new uuid. `vue` is pinned back to ^3.5.18 by hand again. `npm install` rewrites it to ^3.5.0 to match the new package's own range, which contradicts the `overrides` entry, and CI's npm 10.8.2 refuses that with EOVERRIDE while local npm 11 accepts it silently. `npx npm@10.8.2 ci --dry-run` is clean with the range restored. Worth automating; for now it is a hand check on every bump of this package.
…ommand injection) (#2368) quality / Security (composer) is red on every PR here as of today: Advisory ID: PKSA-rdkp-vv9z-mjkg CVE: CVE-2026-67434 — OS Command injection Affected versions: <3.13.6|>=4.0.0,<4.0.2 Reported at: 2026-08-05T23:53:11+00:00 The advisory was published YESTERDAY and roave/security-advisories installs as dev-latest each run, so the same lockfile was clean on 2026-08-05 and is vulnerable on 2026-08-06 with no commit in between. The last green run is evidence of when it ran, not that the lockfile is safe. composer.json's existing constraint already permits the fixed version, so this is a lockfile move only: 1 update, 0 installs, 0 removals. Verified the diff touches exactly two lines, both the version string, and no other file. Part of a fleet sweep — 13 of 16 repos checked were on the affected 3.13.5.
`extractAllOfDelta()` treated EVERY `allOf` entry as a schema identifier and
handed it to `loadSchema(string|int)`. In JSON Schema an `allOf` entry is a
SUBSCHEMA, and only some name another schema. The common non-naming case is a
conditional:
"allOf": [ { "if": {...}, "then": {}, "else": { "required": [...] } } ]
which is an array, so the call raised a TypeError. The `catch (Exception)`
around it could not catch it — a TypeError is an `Error` — so it escaped as
HTTP 500 and any schema carrying a conditional was simply unimportable.
## How it surfaced
scholiq's E2E seed. Its `Lesson` schema uses exactly that shape, and the seed
had grown a WORKAROUND with a comment describing the bug:
~ created schema "lesson" WITHOUT its top-level allOf
(OpenRegister 500s on composed schemas; conditional validation dropped
for this fixture)
A fixture comment documenting a defect nobody had filed. The seed then reported
118/118 schemas present, so the register import looked fine while one schema
had silently lost its conditional validation.
## Not the same bug as the last one
SchemaMapperInlineCompositionTest already covers inline `oneOf`/`anyOf`/`allOf`
— but through `resolveSchemaExtension()`, which is the READ path. A write goes
somewhere else:
SchemasController::create -> createFromArray
-> extractSchemaDelta -> extractAllOfDelta -> loadSchema
and on THAT path `oneOf`/`anyOf` return early, so only `allOf` ever reaches
`loadSchema`. That is why the earlier fix looked complete and was not. The new
tests drive `extractSchemaDelta` directly.
## The fix
`parentIdentifierFromAllOfEntry()` decides what an entry actually is:
* a scalar (`"person"`, `42`, a uuid) — OpenRegister's shorthand for
"extend this schema";
* `{"$ref": "#/components/schemas/Person"}` — the last path segment, which is
what loadSchema matches against id / uuid / slug;
* anything else — a conditional, an inline `{"properties": …}`, a non-string
`$ref`. Valid JSON Schema, names NO parent, contributes nothing to a parent
delta, and is skipped.
The catch is widened to `\Throwable`. That net exists so an unresolvable parent
does not fail an import; a TypeError from an unexpected composition shape is
the same situation, and delta extraction is an OPTIMISATION — nothing it can
fail at justifies refusing to store the schema.
## Verification
Reverting ONLY the fix and keeping the new tests reproduces the CI error
verbatim:
TypeError: SchemaMapper::loadSchema(): Argument #1 ($identifier) must be
of type string|int, array given, called in .../SchemaMapper.php on line 3644
Tests: 5, Errors: 2
With the fix: 5/5, 11 assertions. `testRefEntryStillResolvesToAParentIdentifier`
is the anti-widening arm — "skip arrays" would also skip a real `$ref` and
quietly turn inheritance into no inheritance.
phpcs 0 errors, psalm 16, phpstan 2 — all identical to the pristine file.
fix(schemas): a conditional in allOf 500'd POST /api/schemas
… was unmeasurable (#2537) * test(e2e): admit the three self-cleaning CRUD specs to the CI floor openregister's green E2E column is a verdict about NINE FILES. Measured with `playwright test --list` run from `git archive HEAD` on `development` `cbb0c813` — the instrument that reproduces CI's `Running N tests` exactly — the repo holds 64 spec files / 383 tests and the CI allow-list ran 9 files / 44 tests. 14% of the files, 11% of the tests, on the foundation repo that all 18 apps depend on. The gap is not evenly distributed. The nine admitted files navigate and read; not one of them writes. So the E2E column has never been able to fail on a create, an update or a delete — persistence is the single thing it does not speak about, in the repo whose entire subject is persistence. Two things kept it that way, and both are corrected here rather than worked around: 1. Criterion 2 in this config's own header is "non-mutating, OR self-cleaning", and only the first branch had ever been used. The sentence after it — "All four files admitted below only navigate, open a modal, and read — they write nothing" — described an accident of what happened to be admitted as though it were the rule. 2. A fixture-writing spec is genuinely unsafe against the SHARED :8080 dev container, which is why these could not be rehearsed locally. It is not unsafe in CI: quality.yml's playwright job runs on its own ubuntu-latest runner with its own postgres:16 service and its own Nextcloud, created and destroyed per run. The constraint was on rehearsal, not on execution, and the two were being treated as one. `tests/e2e/_fixtures.ts` already implements the discipline the second branch asks for: every entity namespaced `e2e-<Date.now()>`, `afterAll` deleting exactly what `beforeAll` seeded, and a teardown that re-resolves by slug when a mid-run failure lost the id. Admitted, checked per file against all four criteria: crud/register-crud.spec.ts 4 tests — CREATE through the real UI form (CnFormDialog), then persistence, re-render, and delete. crud/schema-crud.spec.ts 4 tests — property-set round-trip across an update, asserted on a FRESH GET rather than the write's own echo. crud/object-crud.spec.ts 5 tests — field-value persistence + deep-link render of the created object's uuid. Their `test.skip()`s are cascade guards — `test.skip(<id> === null)` inside a `mode: 'serial'` describe — reachable only when the CREATE step in the same file has already failed. None is seed-dependent, so criterion 4 is satisfied. `object-crud.spec.ts` carries one pre-existing `test.fixme` (the Add Object modal's CodeMirror form is not deterministically fillable headlessly). It is admitted WITH that fixme visible in the skip column. Deleting it, or holding the file back to keep the skip count at zero, would be exactly the invisible pass this config exists to refuse. Expected: 44 -> 57 collected, 56 executing + 1 declared skip. The failure count may go up. That is the point: a spec that has never executed is an unknown verdict, not a latent pass. Also corrected: the header said 58 spec files and the allow-list comment said "~59 other". Both were stale; both now carry the command that produced them. Not admitted, and why, is recorded per file in the PR body — including `ui-navigation.spec.ts`, which is hermetic and writes nothing and is STILL refused, because its ten tests assert only that `#header` and `main` are visible. Those are Nextcloud chrome and render whether or not the app mounts. * test(e2e): admit object-lifecycle-workflows — I refused it on a stale comment I excluded `workflows/object-lifecycle-workflows.spec.ts` from the previous commit because "it carries test.fixme blocks for real defects — see its BUG LIST". I was reading its header comment, not its code. grep -cE '^\s*test\.fixme\(' tests/e2e/workflows/object-lifecycle-workflows.spec.ts 0 The BUG LIST is stale prose. All three defects it describes — soft-deleted objects never appearing in GET /api/deleted, POST /api/deleted/{uuid}/restore returning 200 while restoring nothing, and _includeDeleted=true returning 500 — are annotated `// BUG-N (FIXED)` in the code beside their tests, each with its root cause recorded (DeletedController.index() searched without a register/schema context, so it never reached the per-register/schema magic tables; fixed via MagicMapper::findDeletedAcrossAllMagicTables()). The three tests are LIVE REGRESSION LOCKS on that fix, not disabled reports of it. So the file is not an exclusion, it is a candidate — and admitting it puts the soft-delete/restore path under the E2E column for the first time. This is the failure mode this repo keeps paying for: an explanation of a pattern matches the pattern. A header describing fixmes that no longer exist read as evidence that they do. The correction is recorded in the config, the PR body and the board rather than quietly applied. Against the four criteria: hermetic and self-seeding through `_fixtures.ts`; self-cleaning, including a hard `DELETE /api/deleted/{uuid}` in `afterAll` so a soft-deleted fixture cannot survive the run; zero conditional-assert guards. Its single `test.skip` sits downstream of two tests in the same file that assert the "create" and "update" audit entries exist unconditionally, so a broken audit trail fails loudly before that skip is reachable. Separate commit on purpose: the three CRUD files keep an isolated CI verdict. 44 -> 57 -> 65 collected.
…s — 8 endpoints
BUG-RATE-1 was diagnosed and fixed for `create()` alone. Seven sibling endpoints
on the same controller still carry `#[AnonRateLimit]` with no `#[UserRateLimit]`,
and the comment above `create()` already spells out why that is a defect —
including that it caps "this app's own CI test suite".
Nextcloud's `RateLimitingMiddleware::beforeController()`, verified in this
checkout rather than taken from the docblock:
if ($this->userSession->isLoggedIn()) {
$rateLimit = …readLimit…(UserRateLimit::class);
if ($rateLimit !== null) { registerUserRequest(…); return; }
// If not user specific rate limit is found the Anon rate limit applies!
}
$rateLimit = …readLimit…(AnonRateLimit::class);
if ($rateLimit !== null) {
registerAnonRequest(…, $this->request->getRemoteAddress());
}
🔴 Note the last argument: the anon limiter keys on **IP ADDRESS**. So it is not
merely that logged-in users get the anonymous budget — every authenticated user
behind a shared egress IP shares ONE bucket. On `destroy` that is 30 deletes per
minute for an entire NAT'd office, or for a whole CI runner.
Before → after:
index anon 120 user (none) → 600
geoSearch anon 60 user (none) → 300
objects anon 120 user (none) → 600
show anon 120 user (none) → 600
create anon 30 user 300 (unchanged — the existing precedent)
update anon 30 user (none) → 300
patch anon 30 user (none) → 300
postPatch anon 30 user (none) → 300
destroy anon 30 user (none) → 300
Writes mirror `create()`'s existing 300/60 exactly. Reads get 600/60, keeping the
same 5× ratio over their own anon limit. **No anon limit is changed**, so the
anonymous abuse surface is untouched — this only stops an anonymous cap being
applied to identified callers.
#### How it surfaced, and the measurement
decidesk's `Integration Tests (Newman)` fails intermittently on `development`
with `Teardown: all seeded objects deleted (governance body 200/204)`, which
reads like a leaked object. It is not:
expected 429 to be one of [ 200, 204 ]
Its teardown issues 46 DELETEs; against a 30/60 cap the window overflows in every
run measured. Whether it goes red is decided by RUNNER SPEED — a fast run
compresses the deletes into ~66s and 17 of them 429, a slow run spreads them over
~84s so the overflow is absorbed by an earlier collection whose deletes carry no
assertions. **A faster machine fails.** That is why it looks random, and why
"success on development right now" was not evidence of health: 4 of the last 10
`development` runs failed.
⚠️ Scope note: this repairs the CAUSE. decidesk's assertion is doing its job and
is deliberately not relaxed; its misleading NAME (it asserts one status code, not
the absence of leaks) is worth a separate decidesk change.
…ticated-writes fix(security): an anon-only rate limit throttles AUTHENTICATED callers — 8 endpoints
…-configuration fix(import): an annotation-only schema change must not be skipped
…n-handling spec(rbac): consolidate permission handling into one evaluator
Three Hydra gates were failing on development. Two are now green and the third is down to the three methods another PR already owns. Measured with the SAME gate package CI used (ConductionNL/.github@18fe6f9) against development @ e8f39ad, so the before/after are comparable numbers. gate-7 no-admin-idor FAIL 8 -> FAIL 3 gate-25 contract-coverage FAIL 72 -> PASS gate-26 visual-coverage FAIL 27 -> PASS gate-26 was mostly a NAMING gap, not a testing gap. The gate matches a page by its component stem appearing in EXECUTABLE e2e text — js_comment_mask blanks comments first, because a comment naming a component is a claim, not a test (.github#358). Measuring the two questions separately (is the stem in tests/e2e at all, vs does a spec drive the route the manifest mounts it on) showed 21 of the 27 were ALREADY driven by a real, executing spec: 6 named only inside a // comment, and 15 named nowhere at all while a spec navigated straight to their route. tests/e2e/_page-routes.ts closes that distance the honest way — one binding per manifest page, const name == component name, value == the route the manifest mounts it on, imported and used by the specs that already drive those routes. The binding is load-bearing: change a route in the manifest and the const must change with it. Substituting a constant for an identical literal changes no behaviour and adds no assertion, so it cannot redden the E2E job — and three of the edited specs (core-list-pages, admin-settings-pages, feature-pages) ARE in the CI floor's allow-list, so all 19 substituted values were verified equal to the literals they replaced, with a positive control proving the check can say no. Every export is imported by a spec. A registry of unused exports would satisfy the gate with a declaration nobody reads — the same failure as the comment — so EntityDetail, the one export nothing could use, was removed rather than left in. Three pages genuinely needed specs and got them: spec-coverage/detail-pages.spec.ts covers ApplicationDetails, ReportView and SchemaDetails (FlowDetailPage was already driven by flow-engine.spec.ts). Each test SEEDS its record through the documented OR REST controllers, asserts unconditionally on values only that run could have written, and deletes exactly what it created. No isVisible().catch(() => false) guards: tests/e2e/ci/playwright.config.ts names that shape as admission criterion 3 and refuses it. Three carry a reason-bearing @visual exclude because no screen exists to baseline. MapView.vue and OrganisationDetails.vue are unreachable — no manifest page entry, no registry entry, no import from any other component, so webpack never emits them and no route mounts them. EntityDetail.vue's record cannot be created: openregister_entities rows are detected PII and the routed surface registers gdprEntities#index|show|destroy|getTypes| getCategories|getStats and no create. Each waiver names what would remove it. gate-25 is closed with 17 new and several extended controller tests. The gate's PHPUnit arm matches ->method( after comments AND string literals are blanked, so none of this can be satisfied by prose — every one of the 72 is a real call with a real assertion on the returned Response. The largest block is UiController's 20 SPA shell routes. gate-7's five closures are reason-bearing @no-admin-idor-exempt on methods that take no caller-supplied object reference at all. MigrationPacks index/show/export are instance-wide reference assets, and the spec states those three are "available to any authenticated user (packs are shared instance assets the import flow must browse)" while create/update/destroy/ import stay admin-only — which this controller already enforces. WebPush hexIcon/hexBadge take a sanitised Nextcloud APP ID and return a generated glyph: no mapper, no register/schema, no user data. The remaining three, NamesController index/create/show, are a real hole and are deliberately left alone. The file's own TODO(SEC-CTRL-2) says CacheHandler resolves names with no RBAC or tenant filtering; #2523 owns this file and deletes show() outright; the fix is not in the controller anyway, because MagicMapper::findMultipleAcrossAllMagicTables() is a raw UNION over every magic table with no tenant dimension and no _rbac/_multitenancy flag to flip, and PermissionHandler::hasPermission() needs a Schema the name cache does not know. Making the shared, distributed, identifier-keyed name cache caller-aware means changing its cache KEY on the foundation's hottest read path. That deserves its own change and spec, not a ride-along here. #2527 does not help despite its title: it is three markdown files and zero PHP, and gate-7's finding set was byte-identical across that merge. Also fixed on the way past: adding the four missing @PARAM types in OrganisationDetails.vue left its jsdoc/require-param-type suppression unused, and ESLint EXITS 2 on an unpruned suppression — which would have reddened the currently-green Vue Quality (eslint) job. Entry pruned; npx eslint src is back to exit 0 with 0 errors.
Post-commit refinements to five controller tests that landed after the first commit was already taken: FlowRunController::resume, ObjectsController geo endpoints, TasksController::allUserTasks, UserController::exportData and WorkflowEngineController::testHook. Same rule as the rest of the gate-25 work: the coverage has to be a real call with a real assertion, because the gate's PHPUnit arm matches `->method(` only AFTER comments and string literals are blanked — prose cannot answer it.
test(gates): close gate-25 and gate-26, cut gate-7 from 8 findings to 3
The whole-project ratchet cannot pass a deletion, and re-running will not
clear it. Measured twice on this PR — once on the stale base, once after
merging development:
Coverage current: 59.55% (86575/145388 statements)
Coverage merge base: 59.57% (86686/145517 statements)
FAIL: coverage dropped by 0.02% against the merge base.
That is not xdebug variance. The diff removes 129 statements of which 111
were covered — 86.05% — against a project average of 59.57%. Removing code
that is better tested than the project mean lowers the aggregate percentage
by arithmetic, every time, however many times it is re-run. The suite in
that cell is clean: Tests: 16662, Assertions: 37531, no failures, no errors.
The cell dies in the ratchet step, not in PHPUnit.
.github#473 already fixed this by scoping the ratchet to the PHP a change
touches, and quality.yml probes for the capability before using it:
if php scripts/coverage-guard.php --capabilities | grep -qx changed-files
This repository's copy predates that flag — it reports only
`against, update-baseline, capabilities` — so the probe fell through to the
whole-project branch. This replaces it with the canonical
`quality-config/coverage-guard.php` from ConductionNL/.github@f935e2c, which
is what the workflow's own error message instructs an author to do. The copy
is byte-identical to upstream (md5 5be122aad209da030c79b22a133232fb) and the
only line it removes is the CG_CAPABILITIES constant it extends.
Scoped, this change IMPROVES the coverage of what it touches. Verified
locally against the failing run's own clover report:
Scoped to 2 changed PHP file(s).
Changed files, head: 98.46% (831/844 statements)
Changed files, base: 96.81% (942/973 statements)
OK: coverage of the changed files did not drop.
Positive control, because a guard that has only ever passed has not been
shown to work: the identical invocation against a base doctored to 123/123
on NamesController.php prints
`FAIL: coverage of the files this change touches dropped by 0.12%` and exits
1. The instrument can say no.
…-name-endpoints fix(security): remove the three #[PublicPage] name endpoints (SEC-CTRL-2)
Eight controls over CacheHandler's name resolution. Six of them FAIL on the current implementation and demonstrate the disclosure: - an object owned by organisation A is named to a caller in organisation B through the object-mapper path; - organisation A's own name is disclosed the same way; - a name cache warmed as tenant A serves A's names to tenant B (the arm a query-only fix would miss); - getAllObjectNames() hands the whole warm cache to whoever asks; - the magic-table warmup ignores the _organisation column entirely; - a name whose owning organisation cannot be established is served anyway. The remaining two are functional controls: in-scope names must still resolve, keyed by UUID, in the exact shape the six internal callers of getMultipleObjectNames() consume, and an empty request must still answer with an empty map. Measured before any fix: Tests: 8, Assertions: 10, Failures: 6.
…ion (SEC-CTRL-2 step 2)
Closes the authenticated cross-tenant name disclosure behind
NamesController::index and ::create. The fix is in CacheHandler, not in
the controller: NamesController is byte-identical to development.
WHAT LEAKED. getMultipleObjectNames() and getAllObjectNames() resolved
names through three paths with no tenant dimension at all — the
organisation mapper, MagicMapper::findMultiple() (a raw UNION over every
magic table) and the magic-table SQL — and then cached the answer in a
process-wide map any later caller could read.
WHAT THIS DOES.
1. A per-object authorisation predicate, hasOrganisationAccess(), mirrors
Db\MultiTenancyTrait::applyOrganisationFilter(): multitenancy off or an
admin under an enabled override is unrestricted; otherwise the caller
sees the active organisation plus its parents, and no active
organisation means no names. An entity whose owning organisation cannot
be established is refused — absence of tenancy is not permission.
2. The magic-table queries now select _organisation alongside _name. That
column is the tenancy oracle for every object-backed name, because an
ObjectEntity produced by MagicMapper::rowToObjectEntity() never carries
an organisation at all (measured, and left alone here — populating it
would change PermissionHandler's behaviour and belongs in its own
change).
3. THE CACHE KEY IS DELIBERATELY UNCHANGED. The tenancy rides in the
VALUE: in memory as a parallel nameCacheOrganisation map, in the
distributed cache as a {n, o} envelope. Prefixing the key with an
organisation would have forced every invalidation site to learn the
prefix, and would orphan an entry under a stale organisation whenever
an object moves tenant. Keeping the key means all six invalidation
sites keep matching untouched. A value written before this change is a
bare string with no tenancy and is read as a MISS, not served unscoped.
4. The scope is memoised for one call and dropped at the top of every
public reader, so a cron worker serving several callers in one process
cannot carry one caller's scope into the next.
Also fixed on the way: batchLoadNamesFromMagicTables() fataled on a null
registerMapper (reachable now that an unknown-tenancy hit falls through to
it), and getSingleObjectName() passed a null name into a non-nullable
parameter when an entity had neither name nor uuid.
The pre-existing CacheHandler suites are pinned to multitenancy=false —
the configuration they were actually written against — and say so; the
scoped behaviour has its own eight controls.
…solver phpcs's RequireNamedParameters sniff covers internal calls, so every hasOrganisationAccess()/readNameEnvelope() call site now names its argument, and the inline ternary in queryTableForNames() is an if-block. phpmd: $nameCacheOrganisation renamed to $nameOrganisations (LongVariable), and three reason-bearing suppressions for the growth this change causes — two constructor parameters (the same IGroupManager/IAppConfig pair MultiTenancyTrait consumes) and the added guard lines in getSingleObjectName()/batchLoadNamesFromMagicTables(). phpcs and phpmd both clean on the three changed lib files. phpstan cannot run in this worktree (its vendored quality-config points at a path that does not exist locally); CI is the measurement for phpstan and psalm.
Scoping makes getMultipleObjectNames() answer with a PARTIAL map: an identifier the caller may not see is ABSENT, never present-with-null and never an exception. That is the only behaviour change the six internal callers can observe, and the test names each one and how it already handles an absent key — ObjectsController::collectNamesForResponse, ObjectService::collectNamesForResults, ExportService::resolveUuidNameMap, MetadataHydrationHandler, PerformanceHandler and MagicFacetHandler (both its isset() single-value sites and its batch foreach).
Marks step 2 done in the repo's own improvement plan, names the controls, and states the two things that remain open rather than implying more than was fixed: the name resolver still evaluates only the ORGANISATION dimension (register/schema RBAC blocks are not consulted), and MagicMapper::rowToObjectEntity() still drops _organisation on every entity it builds — a separate defect, left alone because populating it changes PermissionHandler::filterUuidsForPermissions() behaviour.
…ays-true is_string CI phpstan (4 errors, all in CacheHandler): - the two new scope helpers used $this->userSession?->getUser(), but the property is non-nullable. phpstan.neon baselines that pattern for EXACTLY ONE occurrence in this file, so three occurrences failed the baseline itself, not just the call sites. Back to one. - getAllObjectNames()'s filter moved from ARRAY_FILTER_USE_KEY to ARRAY_FILTER_USE_BOTH, which lets phpstan read the declared key type as string and makes is_string($key) === false always-false dead code. Replaced with a cast, which also keeps the original intent for keys PHP narrowed to int. Behaviour is unchanged: 2177 tests in tests/Unit/Service/Object/ green, phpcs clean.
…phpmd
CI phpmd flagged an UnusedFormalParameter: getAllObjectNames()'s filter
took ($name, $key) under ARRAY_FILTER_USE_BOTH and only used the key.
Back to ARRAY_FILTER_USE_KEY, keeping the (string) cast that satisfied
phpstan.
CI's diff-scoped coverage ratchet then measured, honestly:
Changed files, head: 83.39% (2651/3179)
Changed files, base: 83.91% (2509/2990)
i.e. +189 statements of which 142 covered, and +47 UNCOVERED. That is not
the documented deletion-arithmetic shape and not xdebug noise — I really
did add 47 uncovered statements, and every one of them is an arm of the
function that decides who may be told a name. Covering them is warranted
on merit, not to move a number:
- resolveNameScope(): multitenancy on/off, unparseable and empty config,
no active organisation, the anonymous default-organisation fallback,
the parent hierarchy, hierarchy failure, and total failure (denies all);
- the admin override: enabled, revoked by SaaS mode, refused to a
non-admin, absent when unconfigured, never granted to an anonymous
caller, unreachable from an unparseable config or a missing app config;
- readNameEnvelope(): a pre-upgrade bare string, a null, a missing or
non-string name, a non-string organisation — all rejected;
- a stored entry with no tenancy settles nothing while scoping is on;
- getSingleObjectName(): refused from memory, and both ways round for an
object and for an organisation resolved from the database;
- batchLoadNamesFromMagicTables(): _organisation carried through and
enforced, and the no-dependencies path returning [] instead of fataling.
23 tests / 31 assertions, shown able to fail: mutating the SaaS-mode arm
from 'return false' to 'return true' reddens
testSaasModeRevokesTheAdminOverride, and the mutation was reverted.
…t-scoped-names fix(security): tenant-scope object-name resolution — closes gate-7 (SEC-CTRL-2 step 2)
Composer had no package-ecosystem entry at all, so composer dependencies got no release-age cooldown whatsoever, unlike npm which has had one for a while. Adds cooldown.default-days: 2 with a conduction/* exclude, matching the fleet-wide floor gate-93 (composer-cooldown-config) enforces. See ConductionNL/hydra openspec/changes/composer-dependency-cooldown and ADR-093 (proposed, ConductionNL/hydra#591).
…block
`ObjectServiceInterface::updateObject()` was documented "Apply a partial
update to an existing object" and implemented as:
$data['id'] = $objectId;
return $this->saveObject(object: $data);
There is no read and no merge. `saveObject()` is PUT-semantic, so a stored
property absent from the payload is written away. Any app that migrated a
one-key update onto the published contract on the strength of that sentence
silently erased every field it did not send AND reported success.
Meanwhile `patchObject()` — which does read, merge and save, with RFC 7386
shaped rules — was not on the contract at all, so a consumer type-hinting
the interface could not reach the one method that did what it wanted.
Two changes, neither of which alters behaviour:
1. Publish `patchObject()` on the interface. Register/schema are narrowed to
`string|int|null` exactly as `saveObject()` and `find()` already are — the
implementation's wider `Register|Schema|string|int|null` stays legal by
parameter contravariance, verified with a compatibility probe. Parameter
names and defaults mirror the implementation exactly.
2. State plainly, on both the contract and the implementation, that
`updateObject()` REPLACES. Its replace semantics are deliberate and
UNCHANGED: existing callers pass a complete object and rely on an omitted
property being cleared. Making it merge would silently change behaviour
for every one of them and needs a fleet-wide audit first.
Contract surface 25 -> 26 methods.
…te paths Nothing else can tell `updateObject()` and `patchObject()` apart. Both return an object, neither throws, so a functional check passes either way — the erasure is visible only in the payload each hands to the save pipeline. Six tests, observed at `handlePreValidationCascading()` inside `saveObject()`, which is where both methods converge. That reads the OBSERVED EFFECT rather than a mock's recorded argument list, which matters because a PHPUnit mock cannot see named arguments and both call sites use them. The payload modelled is procest's `withdraw()`: a one-key `['status' => ...]` against a stored object also holding a title, summary, category, case reference and publication date. Positive controls — each shown RED against a deliberately wrong implementation, and each control reddens ONLY its own method's test: A. updateObject() made to merge -> testUpdateObjectDrops... RED B. patchObject() made to skip merge -> testPatchObjectPreserves... RED C. patchObject() unpublished -> the two contract tests RED D. docblock summary reverted -> testTheContractDoesNot... RED The docblock is asserted as a first-class part of the contract: the summary line — the one line an IDE tooltip shows next to the method name, and where the defect actually lived — must not describe this method as a partial update and must point a reader at `patchObject()`.
gate-83 contract-surface-shift is correct to stop this: adding a method to a published interface breaks every implementing double WITH NO COMMIT IN THE CONSUMER'S REPOSITORY, and openregister#2498 already did that once (opencatalogi 42 errors across all 6 PHPUnit cells, decidesk 1, both on `development` branches nobody had touched). Category `announced`, not `new-contract` and not `internal-only`. Both of those would be false here: `ObjectServiceInterface` is not brand-new — four classes implement it — and the method IS doubled, by three of them. The reason names the affected doubles rather than gesturing at them, because the escape hatch is auditable or it is worthless. Reproduced and controlled with the gate's own checker against origin/development: FAIL 1 before, PASS after. The checker reads HEAD through `git show`, not the working tree, so an uncommitted annotation still reads as absent — worth knowing before concluding the tag does not work.
fix(contract): publish patchObject() and correct updateObject()'s replace-not-merge docblock
… classes Two reusable abstract classes let a consuming app expose one schema as its own entry in Nextcloud's Smart Picker "Select provider" list, instead of everything funnelling through the single generic "Register Objects" entry: - AbstractSchemaReferenceProvider (lib/AppHost/Reference/) — extends ADiscoverableReferenceProvider + ISearchableReferenceProvider. A subclass implements only getRegisterSlug()/getSchemaSlug(); the provider id, supported search-provider id, title, and icon are computed by final base-class methods from the schema's own live metadata, never chosen by the app. - AbstractSchemaSearchProvider (lib/AppHost/Search/) — implements OCP\Search\IProvider, same configuration model, same RBAC (_rbac: true) / multitenancy (_multitenancy: true) contract as the generic openregister_objects provider. Both existing generic providers (ObjectsProvider/ObjectReferenceProvider) are refactored to delegate to two new shared services — ObjectPreviewFormatter and ObjectSearchResultFormatter — so none of the URL-parsing, RBAC-safe search, or rich-preview logic is duplicated between the generic and schema-scoped paths. Behavior-preserving: existing provider ids and output are unchanged. A new Schema::$smartPickerEnabled flag (default false, migration Version1Date20260817120000, SchemaMapper::findSmartPickerEnabledIds(), mirroring the existing searchable flag) gates whether a schema-scoped provider is functionally active. Disabling it makes matchReference()/ resolveReference()/search() return nothing, but cannot remove the provider's entry from the picker list — Nextcloud resolves that list from boot-time class registration, not a runtime flag; documented as a known limitation, not hidden. Also closes two previously-specified-but-unimplemented mail-smart-picker gaps: ObjectService::saveObject() now invalidates the Smart Picker reference cache for every canonical URL shape via IReferenceManager::invalidateCache() (single source-of-truth pattern registry shared with matchReference(), so a future URL shape can't silently escape invalidation), and ObjectReferenceProvider now implements OCP\Collaboration\Reference\IPublicReferenceProvider so publicly-readable objects get rich previews for anonymous viewers. Corrects two stale spec docs found during investigation: mail-smart-picker's "Current Implementation Status" (understated — listed working features as not implemented) and deep-link-registry's registration description (missing the AppHost GenericDeepLinkRegistrationListener manifest-driven path Pipelinq and Procest have both since migrated to). No consuming-app subclass ships in this change — Pipelinq's LeadReferenceProvider/LeadSearchProvider is a follow-up once this pattern has a first real consumer. openspec/changes/archive/2026-08-17-schema-scoped-smart-picker/
ObjectEntity::jsonSerialize()'s declared return type guarantees '@self' is always present, so the '?? []' fallback was provably unreachable; replaced with an explicit is_array() guard (same defensive pattern already used a few lines below for the same variable) so the intent survives without the dead branch. The declared type also proves a top-level 'updated' key never exists (it only lives under '@self'), so the second '?? $objectData['updated']' fallback in the timestamp chain could never fire — dropped, leaving $selfData['updated'] ?? ''. Caught by CI's phpstan run (level enforced there resolves the vendored quality-config path correctly, unlike the local sandbox where that path is broken — an unrelated, pre-existing environment issue).
The previous fix's defensive is_array(\$selfData) === false guard is itself flagged: \$objectData['@self']'s declared type is a closed array shape, so the guard can never fire. Trust the type directly, matching how the file's other (pre-existing, further-downstream) is_array() check on the same variable already passes phpstan unflagged.
…icker feat(smart-picker): schema-scoped Smart Picker + search provider base classes
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Automated PR to sync development changes to beta for beta release.
Merging this PR will trigger the beta release workflow.