Release: merge development into beta - #2906
Open
github-actions[bot] wants to merge 164 commits into
Open
Conversation
…260820201450 chore(release): 1.1.5-unstable.20260820201450
…260820204506 chore(release): 1.1.5-unstable.20260820204506
…260820210405 chore(release): 1.1.5-unstable.20260820210405
…260820212658 chore(release): 1.1.5-unstable.20260820212658
…260820214342 chore(release): 1.1.5-unstable.20260820214342
…260820221600 chore(release): 1.1.5-unstable.20260820221600
…260820223544 chore(release): 1.1.5-unstable.20260820223544
…260820224828 chore(release): 1.1.5-unstable.20260820224828
…260820231029 chore(release): 1.1.5-unstable.20260820231029
…260820233856 chore(release): 1.1.5-unstable.20260820233856
…260820235724 chore(release): 1.1.5-unstable.20260820235724
…260821001315 chore(release): 1.1.5-unstable.20260821001315
…260821002700 chore(release): 1.1.5-unstable.20260821002700
…260821004947 chore(release): 1.1.5-unstable.20260821004947
…260821010031 chore(release): 1.1.5-unstable.20260821010031
…ndJob (#2870) * refactor(lib): move background jobs out of lib/Cron into lib/BackgroundJob This app carried BOTH directories — 43 jobs already in lib/BackgroundJob/ and 11 in lib/Cron/, with 11 of the 12 <job> entries registering from the retired one. Nextcloud's convention is BackgroundJob/; Cron/ is a directory the app framework has no notion of. Part of the 2026-08-25 fleet structure audit (ADR-100 Decision 3), which found five apps using lib/Cron/ and two carrying both. This is the last of them. All 11 classes move with their namespaces and @Package tags, plus 8 unit tests (whose own namespaces were BOTH Unit\Cron and OCA\OpenRegister\Tests\Unit\Cron — two conventions in one directory), FlowRunMapper, HandoffQueueDrainListener and one more test that referenced them. THE LINE THAT WOULD HAVE BROKEN SILENTLY is appinfo/info.xml's <job>, which registers by fully-qualified class name. A move without it leaves the registration pointing at a class that no longer exists, and Nextcloud does not fail the install for that: the job simply never runs, indistinguishable from one that ran and found nothing to do. All 34 registered classes were verified to resolve to real files after the move. CONFIG THAT KEYS ON PATH, NOT CLASS — five entries, in two files, invisible in the diff of a move: phpmd.baseline.xml FlowScheduleWorker, SyncDataJob phpstan-baseline.neon ArchivalRetentionTask, TransferCheckJob, WebhookRetryJob The TransferCheckJob phpstan entry also embeds the namespace in its `message` regex, not only in `path`, so both halves had to move. A stale baseline entry does not narrow a finding — it stops suppressing one, and the violation resurfaces as if it were new. Three further baseline paths are dangling and are LEFT ALONE, because they are pre-existing and unrelated to this move: lib/Service/Flow/FlowActionService.php, lib/Service/Object/ExportHandler.php and lib/Service/Object/RelationshipOptimizationHandler.php name files that do not exist on development either. RemoveRetiredCronJobs removes the oc_jobs rows the rename orphans. Measured on a live instance after the equivalent opencatalogi move: oc_jobs still held OCA\OpenCatalogi\Cron\DirectorySync and ...\Cron\RetentionEvaluation beside their replacements. info.xml's <job> entries ADD registrations on upgrade and never remove one whose class disappeared. It is the counterpart to the existing ReconcileDeclaredBackgroundJobs, which solves the INVERSE problem — a declared job Nextcloud never added. Neither subsumes the other: reconciliation works forwards from the declaration list, so a row naming an undeclared class is invisible to it. The retired classes are an explicit list rather than "any row whose class is missing", because the generic form would delete on the strength of a negative. 27 docs and the generated coverage report updated; archived openspec changes left as the record of what was true when written. * style(repair): put the file docblock above declare(), as phpcs requires phpcs failed with one blocking error: 4 | ERROR | Inline doc block comments are not allowed; | use "/* Comment */" or "// Comment" instead A docblock that FOLLOWS declare(strict_types=1) is an inline docblock, not a file docblock. Every existing lib/Repair/*.php in this repo puts it first; the header now matches, with the same @category/@Package tags. Worth recording how this slipped through: I checked the convention by reading line 2 of a sibling file, which is BLANK — the docblock starts at line 3. The detector reported "declare-first" for a repo that is docblock-first. And I verified the fix with `phpcs | grep -c "| ERROR"` against COLOURED output, where the ANSI escapes sit between the pipe and the word, so the pattern never matched and a failing file read as clean. Both readings are now taken with `sed "s/\x1b\[[0-9;]*m//g"` first. Also adds reasoned @SPEC exclusions on the public methods: exclusions rather than links because no capability spec covers this move — ADR-100 Decision 3 is an architecture record, and the jobs behaviour is unchanged, only where their classes live. * fix(phpstan): annotate the one call that must pass a plain string phpstan: Parameter #1 $job of method OCP\BackgroundJob\IJobList::remove() expects class-string<OCP\BackgroundJob\IJob>|OCP\BackgroundJob\IJob, string given. The narrow type is correct for the callers it was written for — code REGISTERING a job has the class. This step RETIRES one, and the class is gone by construction: that is the entire reason the oc_jobs row has to be removed. A class-string is unobtainable here, and remove() only uses the value as the `class` column to delete on. Annotated at the call site rather than added to a baseline, so the reason travels with the code instead of living in a file nobody reads. Verified with phpstan on the file: [OK] No errors. phpcs still reports 0 blocking errors. --------- Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
* feat(walkthrough): give OpenRegister a getting-started tour
OpenRegister is the foundation every other app stores through, and it had no
walkthrough at all. A new admin landed on the dashboard with 36 pages and no
route through them.
Seven first-visit steps over the three ideas the rest of the product depends
on: a register is the container, a schema is the shape, and search/views is
where the resulting objects surface.
Two deliberate constraints on how this is built:
NO ELEMENT TARGETS. The house convention for "click the add button" is
`{"kind":"element","ref":"index-add"}`, which works in apps whose pages are
`type: index`. Every OpenRegister page is `type: custom`, and a grep for
`index-add` (and for any `data-testid` containing "add") across src/ returns
nothing. Pointing a step at an element id that does not exist highlights
nothing and reports no error, so the creation steps use `allowManualNext` with
a page target instead of pretending to anchor on a button.
NAV IDS COME FROM CHILDREN, NOT TOP-LEVEL ENTRIES. This menu is grouped:
DataGroup, IntegrationGroup and friends carry the real items as `children`.
`Registers`, `Schemas` and `Tables` are child ids, and a validator that only
walked top-level `menu[].id` would have called all three broken.
Verified against this manifest, not assumed:
page targets ..... dashboard, registers, schemas all in pages[]
nav targets ...... Registers, Schemas, Tables, Documentation all in the
flattened menu + children id list
route-match ...... registers, schemas, tables are real route names
final CTA ........ has a task and points at the Documentation nav entry
check:manifest PASSES. Copy carries no em-dashes. The diff is 119 insertions
and zero deletions, so nothing else in the manifest moved.
* test(e2e): suppress the new walkthrough so nav clicks are not intercepted
Pre-empting the failure the sibling petstore PR already hit. Adding a
`first-visit` walkthrough mounts a modal spotlight whose full dim layer sits
over the page and swallows pointer events, so sidebar links are visible,
enabled, and unclickable. In petstore that surfaced as two runs of
`locator.click: Test timeout of 30000ms exceeded` on the left-navigation spec.
Same fix dossiq and shillinq already carry. The tour's "seen" marker is
browser-local, so a fresh Playwright context re-triggers it every run; seeding
it with a high sentinel version makes every step's `sinceVersion` sort below
it, and the tour composes to an empty step set.
Verified rather than pattern-matched:
app id ..... `<id>openregister</id>` in appinfo/info.xml
key ........ WALKTHROUGH_SEEN_STORAGE_PREFIX + appId in useWalkthrough.js,
so `cn-walkthrough-seen:openregister`
URL ........ `/apps/openregister/` is the path this suite already navigates to
elsewhere, not a guess from the app name
Both the key and the URL fail silently if wrong: a bad key reads back as
"never seen" and a bad path is swallowed by the catch, either way leaving the
suppression present in the diff and absent in effect.
* test(e2e): seed the walkthrough via the library helper, and format
Two fixes to my own previous commit.
CI's `format` leg failed on this file. The cause was one line over the width
limit inside the block I added, not the manifest - I initially misread the
failure as being about manifest.json, which the `format` script does not even
match (`prettier --check "**/*.{js,ts,vue,css,scss}"`, no json).
While fixing that: replaced the hand-rolled localStorage write with
`seedFirstVisitOverlaysSeen(page, 'openregister')` from
`@conduction/nextcloud-vue/testing/playwright`. Integriq's suite already used
that helper, which is how I noticed mine was reinventing it. The helper calls
`seedWalkthroughSeen`, which writes exactly the key and sentinel version
`useWalkthrough` reads.
That matters beyond tidiness: the key name and the `999.0.0` sentinel are the
library's to define, and both halves fail SILENTLY when a local copy drifts.
A mistyped key reads back as "never seen", the tour mounts, and the
suppression sits in the diff looking correct while doing nothing.
The `/index.php/` prefix is deliberate and load-bearing: CI serves Nextcloud
with `php -S` and no router script, where `/apps/openregister/` is a directory
with no index.php and 404s. A 404 page still shares the origin, so a seed
written against it would appear to succeed.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
* fix(integration): resolve fleet app ids across the rename
A cross-app id reference is a DUCK-TYPED RUNTIME LOOKUP.
isInstalled('openconnector') against an instance running 'integriq' does
not error - it returns false, and the integration silently stops working.
A URL segment behaves the same way: /apps/<id>/ is a routing key, so the
wrong name is a 404, not a diagnosable failure.
The fleet is mid-rename and the two halves disagree. Measured 2026-08-26
from appinfo/info.xml, the only authority:
branch integriq filinq stackiq dossiq buildiq
development integriq filinq stackiq dossiq buildiq
beta/main openconn. docudesk software. procest openbuild
So neither id alone is correct - hardcoding the new one breaks against
beta/main, and keeping the old one breaks against development.
Adds FleetAppId, which takes a LIST of candidate ids, newest first, and
returns whichever the instance actually registered. Callers ask for the
canonical name and get back the real one.
Rewires the production call sites that were pinned to a legacy id:
ExternalIntegrationRouter isInstalled + isEnabledForUser
PdokGeocoder isInstalled
IntegrationsAdminSettings isInstalled, ROUTE NAME, and URL path -
all three are keyed on the app id
OpenConnectorTransport talks to a REMOTE instance, where the local
app manager cannot answer. testConnection()
probes candidates newest-first on 404;
send() does not probe (it POSTs, and a probe
risks a double synchronisation) and instead
honours an explicit config key.
Remaining matches under lib/ are docblock examples, not executable.
Also fixes 7 pre-existing test errors. MarkerLookupTrait's catch block -
which exists to degrade gracefully per AD-23 - called
\OCP\Server::get(LoggerInterface::class)->debug(). That returns null
with no server container up, so the ERROR HANDLER fatalled with 'Call to
a member function debug() on null' and seven provider tests died in the
handler rather than in the code under test. Confirmed pre-existing by
running the suite with these changes stashed: 7 errors either way.
Tests: 12 new, mutation-checked - removing the legacy fallback from the
candidate map fails 5 of them. Touched suites: 401 tests, 0 errors.
* fix(quality): satisfy phpcs named-params and phpmd StaticAccess
phpcs: internal calls must use named parameters - the three self::resolve()
calls inside FleetAppId did not.
phpmd: FleetAppId is static by design, so it trips StaticAccess. Declared
as an exception via the rule's OWN exceptions property, following the
convention learniq and shillinq already use - not a baseline entry and not
an @SuppressWarnings, so the rule stays active everywhere else. Verified
the exception is narrow: a probe on a DIFFERENT static call in
MarkerLookupTrait is still reported.
Also reverts a StaticAccess violation I introduced. MarkerLookupTrait's
logging originally read
\OCP\Server::get(LoggerInterface::class)->debug(...)
which phpmd does not flag. My null-guard assigned the result to a variable
first, which it does. Measured against origin/development: 0 violations
there, 1 with my version. Restored the chained form and used ?-> instead,
which keeps the null-safety that fixed the 7 test errors without the
new violation.
* fix(docs): pin patched transitive deps via npm overrides
The docs site is the largest single source of dependabot alerts here -
128 of this repo's 217 open alerts sit in docs/package-lock.json.
None are fixable by upgrading: they are transitive dependencies of
Docusaurus's own build toolchain, and npm audit reports
fixAvailable:false across the whole @docusaurus/* cascade. A lockfile
refresh changes nothing either - it is already at the newest versions the
declared ranges permit. So the remedy is overrides.
Added 27, each the newest release within the major the tree already
resolves:
69 advisories -> 38, all three criticals cleared
Seven packages were deliberately excluded because they resolve to TWO
majors in this tree - ajv, brace-expansion, js-yaml, minimatch,
path-to-regexp, uuid, ws. Forcing one version collapses the older
consumer.
`webpack` is excluded too, and that one was learned the hard way: bumping
it 5.98.0 -> 5.109.2, within the same major, broke webpackbar with
ValidationError: Progress Plugin has been initialized using an
options object that does not match the API schema
Worth recording that removing the override did NOT fix it - npm install
will not downgrade a version already in the lock, so the failure repeated
identically until the lockfile was restored and re-resolved.
npm audit was perfectly happy with both broken trees. It counts
advisories; it does not run the build.
Verified against a control: baseline (no overrides) builds 553 pages, and
this change builds 553 pages. Byte-for-byte the same page count, exit 0.
Note: this repo's Documentation workflow only triggers on a
`documentation` branch, so the docs build is never exercised for
development - last run 2026-05-25. This was verified locally.
---------
Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
…licated' (#2871) * fix(logging): say whether the register was absent, not 'absent or duplicated' The probe caps itself to one row (setMaxResults(1), ORDER BY id ASC) so that duplicate-slug rows cannot raise MultipleObjectsReturnedException — its own comment says exactly that. findEntity() therefore never sees a second row, and the only reachable cause is absence. The catch nevertheless listed both exception types and logged 'does not exist (or is duplicated)', hedging between one cause that is real and one the code had already excluded. It cost a diagnosis. learniq#620 traced a runtime 'DossierNote fetch failed: 404' back to this line and could not tell from the log whether the register was missing or duplicated — absence and duplication have entirely different fixes, and the message answered neither. DoesNotExistException now reports absence plainly. MultipleObjectsReturnedException keeps its own branch at error level, worded as the contradiction it would be: reaching it means the single-row cap did not hold, which is a fact about the query builder rather than the data, and should be loud rather than folded back into 'not found'. * fix(phpmd): keep the single catch, distinguish the cause in the message The two-catch version added ~9 lines of code and took RegisterMapper from 996 to 1005, over phpmd's ExcessiveClassLength threshold of 1000. Comments do not count toward it, so trimming prose would not have helped — the second catch block itself was the cost. One catch, both exception types as before, and the message chosen by instanceof. Same information, +2 lines instead of +9. * fix(phpcs): record the cause in context, not via an inline if The ternary tripped 'Inline IF statements are not allowed'. Putting the exception class in the log context is better anyway: structured data belongs in context rather than baked into prose, a consumer can filter on it, and it costs one line instead of the nine the two-catch version needed. +1 code line against development, no inline if, php -l clean.
* feat(delegation): enforce grants at save and at fire (refs #2850) Closes or-delegation-grants tasks 3.1-3.3. The grant record and its resolver landed in #2851 and nothing consulted them; this is the half that refuses. `DelegationService::runAsDelegated()` is the guarded form of "act as this user". It is deliberately NOT on ObjectService, despite ADR-099 naming it there: `runAs()` is the primitive (hand it an IUser, it narrows) and the grant check is the authorization layer above it. Folding the check into the primitive would make it unusable by callers that legitimately have no delegation to check, and the usual answer to that is a $skipCheck flag — a security check with an off switch. There is still exactly one identity-switch primitive; this calls it. SAVE TIME. FlowTriggerValidator refuses a schedule trigger naming a user the saver holds no grant for. Naming yourself stays free. A permitted delegation is STAMPED with `runAsDeclaredBy: <saver>`, server-written on every save and never read from the request body — and stripped whenever the trigger names its own saver, so a forged value cannot stand in for a grant. FIRE TIME. Without that stamp task 3.3 is not implementable: a schedule fires unattended, so at 03:00 there is no principal to check a grant against and the only candidate left would be `flow.owner` — the fallback ADR-099 removed. FlowDelegationCheck re-resolves at queue time, because save-time passing is not standing authorization: revoking is supposed to stop the next firing, and treating the stored trigger as proof would make revocation cosmetic for exactly the runs nobody is watching. The schedule is left ENABLED on a delegation refusal, unlike the unattributed case. A flow naming nobody cannot fix itself without an edit, so leaving it "on" would be a switch that lies; a revoked delegation becomes valid again the moment the grant does, and disabling would silently convert a temporary revocation into a permanent one only a human re-enabling could undo, with nothing telling them. Migration posture: refuse until granted, no grandfathering. Warranted by the task-1.1 measurement — zero declarations on this instance name anyone other than themselves, so nothing needs grandfathering, and minting grants nobody asked for is the permanent privilege this change exists to stop. Fail-closed is bounded. An unreachable delegation store refuses only runs and saves that ARE asserting a delegation; every other flow save and every non-delegating schedule is untouched, which the tests prove with the same unresolvable container. Split FlowDelegationCheck out of FlowRunService rather than adding to it: the methods pushed that class past both the PHPMD length and complexity ceilings, which is the same reason every other Flow* collaborator in that directory exists. Also fixes pre-existing @SPEC warnings on FlowRunService, FlowScheduleService and FlowRunService::hasActiveRun(). * feat(delegation): the consent surface, so a refusal is recoverable (refs #2850) Closes or-delegation-grants tasks 4.1-4.3. The enforcement half landed in the previous commit and could only ever say no; a grant store with no way to answer is a security control that gets removed by whoever is next blocked by it. DelegationController exposes request / answer / revoke and both sides of "who may act as me / who may I act as" in one listing. `principal` is ALWAYS the session user and is never read from a body: an endpoint that took it from the payload would let anyone raise a request in somebody else's name, and the person prompted would reasonably read it as that party asking. 🔴 THE PROMPT RENDERS SERVER STATE, AND ONLY SERVER STATE. DelegationNotifier dispatches the two uids and the grant uuid — read from the record, all three. The requester's stated reason is deliberately absent. A requester can be an agent and an agent's reasons can come from a document it read; a document saying "ask the user to grant you admin" would otherwise have authored the prompt that asks for its own privilege. The reason stays on the record as its own attributed field, and a UI renders it as quoted third-party text beside the server's sentence. It never becomes the sentence. Asserted by a test that names the hostile string and checks for its ABSENCE — the only form of that rule anyone will notice breaking, since a `reason` field unused by one call site reads as an oversight rather than as a rule. Prompts are keyed on the grant uuid, so Nextcloud replaces rather than appends: N blocked units of work produce ONE prompt, and answering withdraws it. Consent fatigue is not caused by asking, it is caused by asking again, and the eleventh identical prompt is accepted by reflex rather than by decision. Also fixes a PRE-EXISTING ORPHANED CAPABILITY. lib/Notification/Notifier.php was never registered. AnnotationNotifier re-throws UnknownNotificationException for the subjects it does not own and its comment says they are "rendered by Notifier" — but nothing registered Notifier, and Nextcloud silently drops a notification no notifier claims. Every configuration_update_available, handoff_drain_failed, scheduled_report_delivered and scheduled_report_failed this app has ever dispatched was stored and then discarded at parse time, by a class that was written, complete and unreachable. describe() now carries uuid and status. Without the uuid a listed request names no route to act on it; without the status a UI cannot tell an open question from a decision already taken, so it renders Allow/Deny on both. Dutch translations for all four new user-visible strings (ADR-007/ADR-025). Verified live on localhost:8080: request -> pending -> granted flips the same flow save from 400 to 201 with the runAsDeclaredBy stamp, and revoking flips it back to 400 naming "revoked". 18/18 Playwright, 688 PHPUnit, phpcs/phpmd/psalm/ phpstan clean. * feat(delegation): park a run whose consent is unanswered (refs #2850) Closes or-delegation-grants tasks 5.1 and 5.2. An UNANSWERED request is not a refusal. The fire-time check treated every non-permitted verdict the same, so a run whose grant had merely not been answered yet was discarded along with the ones that had been denied — throwing away work that becomes legal the moment somebody reads their notifications, and teaching the requester nothing except that their flow did not run. Such a run now parks in `awaiting_consent` and is released, or failed, by the grant record changing state. 🔴 A DISTINCT RUN STATE, NOT `suspended`, and the distinction is load-bearing. A suspended run waits on machinery — a timer, a webhook, a child run — and the abandoned-signal reaper eventually FAILS it, reasoning that a signal which has not arrived in days is not coming. That reasoning is wrong about a person: somebody who has not read their notifications in two hours has not declined, they are at lunch. Parking in `suspended` would have handed these runs to that reaper and failed them while the prompt sat unread — reporting "nobody answered" about a question nobody had yet seen. The parked run carries no `resume_at`, deliberately: with one, the timed-resume sweep would start it before anybody had answered. What releases it is the grant, re-resolved — so one answer frees every run it unblocks, which is the other half of the request dedup. 🔴 THE TIMEOUT FAILS, IT DOES NOT PROCEED. Running the work after a timeout would convert an unread prompt into an approval at whatever hour the timer elapsed, which is the exact substitution this subsystem exists to prevent. The recorded error says "an unanswered request is not consent" rather than merely noting that time passed. Default 72h, `flow_consent_wait_hours`. An UNREADABLE store leaves the run parked rather than failing it — the trade-off inverts from the fire-time check, where refusing costs one run and permitting costs an unauthorized execution. Here nothing runs either way, so waiting is free and failing destroys work over a blip. `awaiting_consent` is ACTIVE and not TERMINAL: a parked run is still going to happen, and omitting it would hide it from every "currently running" surface — which is exactly where somebody goes to find out why their work has not run. The worker's delegation service is nullable so the two positional unit suites keep working, but its absence is REPORTED when runs are actually parked. A sweep that skips produces the same output as one that found nothing to do. Verified live on localhost:8080, both jobs driven by occ background-job:execute: grant -> save (stamped) -> revoke -> re-request (pending) -> schedule fires -> run parks reading `Waiting for "ddauth-alice" to allow "admin" to act as them.` -> answer allow -> worker sweep releases -> queued -> executed -> stopped. 769 PHPUnit green; phpcs/phpmd/psalm/phpstan clean. FlowRunService went one line over the PHPMD class-length ceiling, so the park branch is its own method. * docs(delegation): tasks 6 and 7 — the gate, and what is verified vs stated open Gate 96 (system-elevation-reachability) is ConductionNL/.github#579. Recorded as ADDED ALONGSIDE SystemOperationContextBoundaryTest rather than replacing it: the test runs in milliseconds on every local phpunit and names the four permitted files exactly, which the gate deliberately does not — it permits by directory. Deleting the faster, more specific instrument because a broader one exists trades a signal for nothing. Three things are recorded as OPEN rather than ticked, because each would otherwise read as verified: * the awaiting_consent path is verified live by hand (occ background-job: execute) but has no Playwright spec — the api-direct harness cannot drive a schedule fire or a cron sweep, and a spec that asserted the setup rather than the behaviour would be worse than none; * composer test:all was not run to completion locally — the full tests/Unit tree is 17,351 tests and exhausts 2GB on this box. The affected subtrees run green at 769; CI runs the whole suite; * the consent prompt has had no accessibility pass. It renders on Nextcloud's own notification surface, but the two action labels are ours. * test(delegation): cover the controller, the fire-time check and the prompt The coverage ratchet on PR #2864's changed files failed: 73.19% head against 75.68% base, a 2.49% drop. The suite was green — the drop is new code arriving with e2e coverage and no unit coverage, and the ratchet only reads clover. Three gaps, all real rather than cosmetic: * DelegationController — 328 lines with no unit test at all. It carries the authorization routing, and the case that most needed pinning is not reachable cheaply from e2e: a caller putting `principal` in the body. The test asserts on the value handed to the LIFECYCLE, because that is where the substitution would take effect. Also pinned: 403 rather than 400 for "you may not answer this" (being refused is an authorization outcome; reporting it as a malformed request sends the reader to their payload), and that all four routes refuse without a session — asserted across all four, because the check is per-method and a route that forgot it would be invisible to a test that only exercised its neighbour. * FlowDelegationCheck — driven indirectly by FlowRunAttributionTest, but its recording branch was unreached. The two tests that matter assert the asymmetry: a revocation IS written onto the flow, and the schedule is LEFT ENABLED. A flow naming nobody cannot fix itself without an edit; a revoked delegation becomes valid again the moment the grant does. * Notifier::prepareDelegationConsentRequested — ~40 statements rendering the prompt, untested. It must NAME the requester (a prompt saying "somebody" is one a person cannot answer responsibly) and survive a parameterless notification row, since a row can outlive the shape that wrote it and throwing would take the notifications endpoint down for every other subject. Also fixes PRE-EXISTING DEBT found while chasing this: the local unit suite could not complete. BlobMigrationJobTest resolves real services from \OC::$server in setUp(), and on a box where the bootstrap found an NC root it could not initialise that resolution does not fail — it RUNS AWAY. Measured: identical death at test 259 of 17,359 with a 2GB limit and with a 5GB one, so it was never a size problem, and the fatal took the other 17,100 tests with it. The bootstrap already sets OPENREGISTER_TEST_SKIP_NC=1 in exactly that case and its comment promises container-bound tests "will fail clearly"; this is the class where that promise was not kept. It now skips with a message naming what went unverified — the batching, the completion flag and the orphan grouping — because "skipped" alone cannot tell "no NC" from "the job is broken". * fix(delegation): a request may only name someone the caller can already see Gate-7 (no-admin-idor) flagged `request()`: `#[NoAdminRequired]` with no per-object guard. It was right, and the finding is not the one the gate's name suggests. 🔴 THE ENDPOINT WAS A USER-EXISTENCE ORACLE. Any authenticated user could POST a uid and read the status code as an answer — 201 for a real account, 404 for an invented one. And it would have been a NEW oracle: Nextcloud governs enumeration through its own sharing settings, so an endpoint that answers around them has removed a control rather than added a feature. I introduced that while adding an existence check for a good reason (a pending request naming nobody can never be answered, so it would sit until it expired while its requester waited for a prompt no account could receive). Both facts now return ONE response. "Not someone you may ask" is all a requester needs and all they are entitled to; the difference between "no such person" and "not in your organisation" is exactly what an oracle is built out of. A test asserts the two are indistinguishable down to the uid the caller already supplied. ORGANISATION is the boundary because it is the fleet's tenancy unit — the same one scoping every register, schema and run. A delegation that crossed it would let one tenant's user request rights inside another's. An administrator is exempt: answering across tenants is what they are for, and that exemption is its own test, so a controller that refused every cross-organisation request could not pass this file. An unreadable organisation list REFUSES. Treating it as "no restriction" would re-open the enumeration surface wholesale, and it is the fail-open shape this subsystem has already been bitten by twice. One test note worth keeping: `Organisation::getUsers()` is a MAGIC method served by Entity::__call, so createMock() cannot stub it — it answers "Method name is not configured". The fixture uses a real entity with setUsers(). 16 tests / 29 assertions green; gate-7 clean on the file; phpcs, phpmd, psalm and phpstan clean; 18/18 Playwright still green live. * feat(delegation): an accessible consent prompt, and the parking path in e2e Two of the three things this change had recorded as open, closed. Both were recorded honestly and both turned out to be smaller than the note implied. 🔴 THE PROMPT NOW RENDERS AS A PERSON, NOT A TOKEN. `setRichSubject` / `setRichMessage` carry the requester as a `user` parameter, so a client renders a real user reference — display name, avatar, semantics a screen reader can announce as a person — and `setParsedSubject` / `setParsedMessage` keep the plain-text sentence every other surface reads. Both, and neither optional: NC's own isValidParsed() refuses a notification with only rich text, and a client without rich rendering shows nothing without the parsed half. A test asserts the two say the SAME sentence, because a rich subject reading differently from its fallback would mean two people looking at one security decision on different clients were answering different questions. A uid is an identifier, not a name. Asking somebody to grant rights to `j.devries3` when the person they know is "Jan de Vries" is harder for every reader and materially harder for one who cannot glance at a face beside it, so the display name is resolved and asserted — with the uid kept as the rich parameter's `id`, which is how a client resolves the avatar. The action labels stay "Allow" and "Deny" rather than becoming OK/Cancel. Out of context — which is how a screen reader reaches a button list — "OK" is meaningless and "Allow" is not. The message states the consequence BEFORE the controls, because a reader meets them in reading order.⚠️ I WAS WRONG THAT THE HARNESS COULD NOT DRIVE THIS. I recorded `awaiting_consent` as untestable in e2e because "the api-direct harness cannot drive a schedule fire or a cron sweep". `flow-schedule.spec.ts` has been doing exactly that since it was written — `occ background-job:execute` through the dev container, skipping when unreachable. delegation-parking.spec.ts follows that pattern: park on an unanswered request, assert the run says WHO it waits on, then answer and assert the sweep releases it into work that actually runs. Also fixes PRE-EXISTING DEBT the same file revealed: flow-schedule.spec.ts matched `Cron\FlowScheduleWorker`, and #2870 moved that job to `BackgroundJob\`. A matcher pinned to a namespace stops finding its job after a move — which does not FAIL the spec, it SKIPS it, so the whole scheduled-trigger path would have gone unverified with nothing red to say so. Both specs now match the class basename, which survives a namespace move.⚠️ NOT RUN LIVE. Docker is down on this box, and resolveContainer() refuses the shared `nextcloud` container by design, so this spec skips here regardless. The path it covers was verified by hand earlier today against a live instance — grant, save, revoke, re-request, schedule tick, park, answer, sweep, executed — and the spec encodes that sequence. Said plainly rather than reported as green. Dutch translations for all three new strings. 20 tests / 42 assertions in the Notifier suite; phpcs, phpmd, phpstan and the l10n parity check clean. * docs(delegation): close task 2.3 as not applicable, and say why Ticked as DECIDED rather than left open. An open box invites somebody to 'finish' it by making the grant an object — and that is exactly the circularity task 2.2 exists to prevent: x-openregister-lifecycle and x-openregister-notifications are properties of a register schema evaluated by the object layer, so a grant declaring either would be read through object RBAC, and resolving a delegation would require the delegation. The two tasks cannot both be satisfied. 2.2 is the one holding a security property, so it wins, permanently — this is not a deferral and the box should not read like one. The notification half IS delivered, by 4.1, and the same argument explains why it is imperative rather than declarative: the dialect fires on OBJECT lifecycle events and a grant has none. or-delegation-grants now has no unticked task.
…9 §5) (#2868) * feat(capability): relocate the tool-grant grammar from hermiq (ADR-099 §5) The OpenRegister half of the pair. hermiq's companion PR repoints its consumers and deletes the originals; it cannot merge until a build carrying this ships. ADR-099 §5 keeps TWO grant axes and forbids merging them: * DELEGATION — may principal P act as user B. New, built in lib/Service/Delegation. * CAPABILITY — may agent X use tool T. Mature, and the subject of this commit. The capability system is not agent-specific and does not belong to hermiq. It resolves against `ToolRegistryFacade`, which already lives here, so it moves to sit beside what it resolves against. 🔴 A RELOCATION WITH ITS TESTS, NOT A REWRITE. Nothing below the docblock changed except the namespace. That codec carries a measured scar — 35 of 87 tools parsed wrong — and ADR-095's persistence constraint, and a rewrite-while-moving reopens both. The five classes arrived by `cp` and were edited only where the namespace, the @Package tag, the @Covers tag and the class TITLE named hermiq. 🔴 NAMED `Capability`, NEVER `Grant`. Once both axes live in one codebase the conflation risk rises rather than falls, and a user approving a tool must not be able to widen whose identity the agent wears. The directory name is the control.⚠️ FOUR OF FIVE TESTS MOVED. `ToolGrantResolverTest` did NOT, and that is deliberate rather than an omission: it builds its catalog from hermiq's real tool providers — HermiqToolProvider, MemoryService, MailReadService, NcNativeWriteService, WebFetchService and four more — so it is an integration test against a catalog, not a unit test of the grammar. It stays where the catalog is, repointed at this namespace in the companion PR. Moving it would have meant rewriting its fixtures, which is the one thing this relocation must not do. Verified: 58 tests, 153 assertions green here — the same assertions that were green in hermiq. phpcs, phpmd (both rulesets), psalm and phpstan clean. * fix(capability): bring the spec targets with the code (gate-46) The relocation moved five classes and left all 51 of their `@spec` tags pointing at hermiq's openspec tree. Gate-46 (spec-anchor-existence) reported 94 unresolved findings — and every one of them was mine: the count matched exactly when the checker was run over the relocated files alone. A tag whose target does not exist is worse than no tag. It reads as traceability and dereferences to nothing, which is the shape this fleet already carries ~300 of from archived changes. WHAT MOVED WITH THE CODE. Five spec files, copied VERBATIM into openspec/specs/: agent-capability-reach, agent-tool-governance, governed-cli-mcp-transport, structured-tool-grants, agent-object-leaf. Verbatim matters mechanically as well as editorially — the headings ARE the anchors, so rewording one would break the tags the copy exists to serve. Three of them had only ever existed as change deltas in hermiq. Two of the targets were ALREADY DEAD THERE: `agent-capability-reach` and `agent-tool-governance-and-disclosure` have been archived, so those tags dereferenced to nothing in hermiq too, and the relocation is what surfaced it. They now point at promoted specs that exist. THREE TAGS POINTED AT tasks.md, which is a change artefact that dies on archive by construction. Those are repointed at the requirement each one implements — a task number is not a behavioural contract and cannot be one. Verified with the gate's own checker rather than by inspection: 94 findings before, 0 after, and 0 across every tracked lib/**/*.php in the repository, so nothing else was disturbed. 58 tests / 153 assertions still green; phpcs clean; `npm run check:specs` passes. * docs(capability): trim the relocated specs to what the moved code implements The previous commit brought five hermiq specs across whole so every `@spec` anchor would resolve. That was correct and too broad: ~1,534 lines duplicated across two repos, most of it describing behaviour hermiq still owns. MEASURED WHICH REQUIREMENTS BELONG HERE rather than judging by title. For each `### Requirement:` block, whether any anchor the relocated code cites falls inside it — computed with BOTH slug rules gate-46 accepts, since they part company on punctuation inside a word. 14 blocks kept, 18 dropped; 1,534 lines down to 920.⚠️ A FIRST MEASUREMENT SAID SOMETHING ELSE, and was wrong. Counting hermiq citations by requirement SLUG reported zero for every requirement in all five specs, which would have licensed moving them wholly. Counting by spec PATH instead reported 11, 14, 10, 15 and 1. The slug pass was silently failing to match — `writedestructive` against `write-destructive` — so its zeros meant "my normalisation differs", not "nobody cites this". Two instruments disagreed and the one that could not be fooled by normalisation won. So hermiq stays canonical for all five. Its specs are untouched and its ~50 references keep resolving. What is here is a bounded subset, and each file says so in a banner naming how many requirements were left behind and where. WHY DUPLICATE AT ALL. A `@spec` tag is dereferenced by gate-46 against the repository it sits in, so a cross-repo reference is not expressible — an openregister class citing a hermiq spec resolves to nothing, which is the ~300-dead-tag shape this fleet already carries from archived changes. The duplication is structural, not an oversight, and it is now bounded to exactly the requirements the moved code implements. Verified with the gate's own checker: 0 unresolved anchors after the trim, same as before it. The trim removed text, not traceability. --------- Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…e read paths finally agree (#2885) * feat(api): `_limit` accepts an explicit unlimited value, and the three read paths finally agree `_limit` is normalised in one place — `Support\QueryLimit` — to either a positive row count or `null` meaning no limit at all. ## The three paths disagreed, and two of them lied The same `_limit` answered differently depending on which read path served the request: | `_limit` | single-schema | cross-schema UNION | external database | |---|---|---|---| | absent | unlimited | 100 | 200 | | `0` | **0 rows** (`LIMIT 0`) | **1 row** (`max(1, …)`) | 200 | | `5000` | 5000 | **1000**, silently | **1000**, silently | | `abc` | **0 rows** | 1 row | 200 | Every cell in bold is a caller who asked for something and received something else with HTTP 200 and no indication. The `abc` row is the sharp one: `(int)"abc"` is `0` in PHP, so a typo in a query string produced a confidently empty list. `TmloController::summary()` passes `'_limit' => 0` meaning "no limit" — the intent this change now honours. ## What it does now - `false`, `null`, `0`, `all`, `unlimited`, `none` (any case), the empty string, a negative number, or a non-numeric string → **no limit**, on every path. - A positive number → that many rows. - An oversized number → still clamped. See below. Prefer `_limit=false`; `0` is accepted because callers already wrote it meaning exactly this. ## What this deliberately does NOT do It does not remove the hard page-size cap. `openspec/specs/objects-crud` requires it ("List page size is bounded by a hard maximum"), added by the archived `clamp-list-limit-and-optional-count` change specifically so a client "SHALL NOT cause the server to load an arbitrarily large result set". Reversing that silently would be trading one invisible behaviour for another. So the bound is now escapable only by asking: `_limit=1000000` is still clamped (a caller who has not thought about the result-set size), while `_limit=false` is honoured (a caller who has). The spec is amended to say so, with a scenario for each, plus a new requirement for the unlimited value itself. The UNION path's magic `1000` became a named `MAX_PAGE_SIZE` constant — it was a literal inside `min()`, invisible to anything wanting to state or test it. Unlimited there OMITS the clause rather than interpolating a sentinel, because that SQL is built by string concatenation with nothing to bind to. ## A pre-existing divergence this surfaced, not fixed The requirement says EVERY list endpoint clamps. The canonical single-schema path — the one the fleet's apps actually use — never did: it passed the raw value to `setMaxResults()`. So the protection the spec promises has never existed where it matters most. Left alone here: adding a clamp would REDUCE what existing callers receive, which is a behaviour change in the dangerous direction and deserves its own decision. Filed separately. ## Verification - 7 new tests / 40 assertions. Mutation-checked: reverting `normalise()` to the old `max(1, min(1000, (int)$x))` fails 6 of the 7. - phpcs clean, phpstan `[OK] No errors`, psalm 0 errors. - Full suite: 17,418 tests / 39,144 assertions, no failures and no errors. - Also fixed 2 pre-existing phpcs errors in `Notification/Notifier.php` (an inline `if`, and an internal call without named arguments) — `development` is currently red on phpcs because of them. - `docs/api/objects.md` documents the values, the cost of an unlimited read, and the changed `_limit=0` behaviour. * chore(phpmd): allow static access to QueryLimit, on the ruleset's own terms phpmd flagged `QueryLimit::normalise()` as StaticAccess. This repo already has a deliberate convention for that: the rule is excluded from the shared ruleset and re-added with an `exceptions` property naming the one class that qualifies, with the reasoning attached in config rather than hidden in an @SuppressWarnings annotation or a baseline entry. QueryLimit meets the same bar — it answers one question about one argument, holds no state, has no collaborators, and takes the only thing it needs as a parameter, so there is nothing to inject. It is deliberately not a service: three read paths must reach the SAME answer, and a service would let one of them be constructed with a different implementation, which is precisely the drift it was written to end. Verified the exception is narrow rather than a disable: removing QueryLimit from the list brings the violation straight back (1), restoring it clears it (0). The rule stays fully active for every other class. --------- Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
Without target-branch, dependabot opens against the repository's DEFAULT branch (main). Branch protection refuses any PR to main that does not come from beta or hotfix/*, so every one of those PRs is unmergeable BY CONSTRUCTION and simply accumulates. Measured across the fleet: 15 repos had no target-branch at all, and their dependabot PRs all sit against main failing branch-protection/check-branch. The repos that already set it (humaniq, versioniq, thematiq, dossiq, shillinq, decidiq, buildiq) open against development and merge normally. Cooldown windows and open-pull-requests-limit are untouched.
* fix(flow): read an edge endpoint as the list the document stores
A flow saved by the canvas stores `{"from": ["a"], "to": ["b"]}`, and four
places read `from` with `(string)$edge['from']`. Casting an array to string
in PHP yields the literal "Array" rather than failing, so:
- FlowConnectivity counted no node as having an exit, and reported EVERY
non-terminal node as a dead end. A correct flow saved fine and then refused
to run, blaming the author for a graph that was properly connected.
- FlowTokenRouter matched no edge to its source, so a token that fired had
nowhere to go. Fixing only the check would have made such a flow look
runnable and then route nowhere.
- exitCondition() resolved the source to an empty step, which made every
guarded exit read as unconditional — a token took a branch whose condition
was false and the run still reported success.
All four now go through FlowGraph::normaliseEndpoints(), the document's one
definition of an endpoint, and FlowTokenRouter's two hand-rolled copies of the
same unwrapping are gone: duplicated grammar is what let the two ends of an
edge drift apart in the first place.
The suite could not catch this because every fixture wrote the scalar form
while the editor writes the list form — one fixture used a list for `to`,
which is exactly why `to` had array handling and `from` never did. The three
new tests use the shape the editor actually saves, and each fails against the
previous code.
* test(flow): cover the router's endpoint reading
The coverage ratchet caught a real gap: the fix changed FlowTokenRouter and
FlowConnectivity, and only FlowConnectivity had tests. The router had no direct
unit test at all, so its half of the fix — the half that decides where a fired
token actually goes — was shipping unverified.
Eleven cases across both public entry points, each with a negative control so
a router that matched everything could not pass them. Five fail against the
pre-fix router, including the quiet one: a guarded exit resolved through a list
source. That path returned null before, which reads as "this branch is the
unconditional default" rather than "the lookup failed" — so a token took a
branch whose condition was false and the run reported success.
The scalar-form and null-case tests pass on both sides deliberately: they pin
that this is a widening, not a swap.
---------
Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
…ight specs (#2888) 🔴 THREE GREEN PLAYWRIGHT SPECS AND ZERO CI COVERAGE LOOK IDENTICAL FROM THE OUTSIDE. `delegated-identity`, `delegation-consent` and `delegation-parking` all live in tests/e2e/api-direct/, and playwright.config.ts excludes `**/api-direct/**` from every project — twice, once at the top level and again inside the chromium project, both with a comment explaining the gate-19 convention: API/contract assertions belong to Newman. Measured on this PR's own E2E job: `grep -c api-direct` over the run log returns 0. So those specs run only when a developer invokes the ad-hoc flow config by hand. I ran them and reported them passing, which was true and is not the same as the behaviour being guarded. This is the half CI can see, in the convention the repo already has. Nine requests, each paired with its control: * the listing answers with BOTH directions and the inbox * asking to act as yourself is refused as meaningless * an unknown account is refused in words that do not confirm or deny existence — the user-existence oracle check, asserted on the message and not just the status * a request is created pending, and the PRINCIPAL IS THE SESSION USER: the body deliberately sends `principal: newman-forged-principal` and the assertion is that it was ignored * the stated reason is quoted, never adopted into the system's own sentence * asking twice REUSES the outstanding request and returns the ORIGINAL reason * granting records who granted it; an unknown grant 404s (the control) * revoking reports revoked and when⚠️ THE ROUND-TRIP SKIPS ON A SINGLE-ACCOUNT INSTANCE, and says so. A delegation needs somebody to delegate TO. Rather than mint a fixture user — which would leave an account behind on every CI run — the collection reads the user list and stops, logging that request/grant/revoke went UNVERIFIED. A bare "skipped" cannot tell "one account here" from "the loop is broken". Self-cleaning: the one grant it creates is revoked in the final request, so a re-run starts from the same state and no later collection inherits a live delegation nobody declared.
* fix(l10n): translate the 28 untranslated manifest strings The manifest is data the renderer walks, not source the l10n extractor scans, so CnAppNav's `menu[].label` and CnWalkthrough's step copy looked up keys that were never in the catalogue. A missing key falls back to the English source and nothing reports it, so a Dutch user reads English. Twenty-eight strings: the nav group labels and the whole getting-started tour added in #2866. The tour copy carries the load here, and it is the part worth reading in translation rather than machine-mapping. "Renaming a register slug breaks every app that addresses it by name" is a warning, not a label, and it has to still warn in Dutch: "Het hernoemen van een registerslug breekt elke app die het register op naam benadert, dus denk er nu even goed over na." nl.json ONLY. Adding the same keys to en.json is the obvious move and it is wrong in this fleet: check-l10n-parity.js is a ratchet over every required locale (the official language of every European country, plus Russian and Turkish), so one new English source key demands a real translation in about thirty languages. keepiq#449 shows the failure mode — eleven new en.json keys produced "+568 more missing" across the other locales. The runtime does not need it: lookup is by source string, so `translate('Data quality')` reads nl.json directly and returns "Datakwaliteit". Verified: 0 manifest strings missing Dutch. * fix(l10n): carry 357 Dutch strings into nl.js, which is what the browser loads My previous commit added 28 translations to l10n/nl.json and would have shipped NOTHING. I checked instead of assuming, and all 21 sampled keys were absent from l10n/nl.js: of my keys present in nl.js: 0 / 21 The .json is the server-side catalogue. The frontend reads OC.L10N, which is populated by l10n/nl.js. `translate()` in a Vue component therefore never sees a key that exists only in JSON. A fix that lands only in nl.json is inert, and every check in this repo would still call it green: CI runs `test:l10n` and `check:schema-l10n`, neither of which compares the two files. keepiq and buildiq have `check:l10n-js` and it caught them within one CI run; this repo has no such check, which is why the drift here got to 329 entries. So this carries all 357 missing entries across, not only my 28. The other 329 are pre-existing: real Dutch, written by someone, sitting in nl.json where no browser reads it. Leaving them would mean knowingly shipping a half-fix alongside a commit message claiming the app is translated. This repo has no l10n:build script, no .tx config, and nothing in CI that regenerates nl.js, so the entries were inserted directly in the OC.L10N.register map rather than by a generator that does not exist. Verified by executing the artifact, not by reading it: stubbed OC.L10N.register and required the file. It parses, registers under "openregister", carries 2,765 keys with no duplicates, keeps its plural form, and resolves "Data quality" -> "Datakwaliteit". * fix(l10n): generate every browser catalogue, and add the check that keeps them The earlier commit on this branch carried 357 Dutch strings into nl.js by hand. That fixed Dutch and left 36 other languages exactly as broken, so this ports keepiq's generator rather than repeating the manual insert per locale. Running `--check` before building reported 37 stale catalogues. Not a Dutch problem: every language this app ships was unreachable in the browser. `l10n/<locale>.json` is read server-side by PHP `$l->t()`, while the browser only ever sees `OC.L10N.register(...)` from `l10n/<locale>.js`, and a raw .json is not served from an app directory at all. German and French each gain 2,704 keys that previously reached nobody. The generator is keepiq's, unmodified. It reads the app id from appinfo/info.xml rather than hard-coding it, which matters in a fleet that renames apps: a catalogue registered under a stale id is silently ignored by `t()` and every string falls back to English with no error anywhere. Adds `l10n:build` and `check:l10n-js`. The check is the point. keepiq, buildiq, portaliq and humaniq own it and each caught this drift within one CI run; this app, dossiq, filinq and pipelinq did not, and had accumulated 329, 142, 257 and 1,090 unreachable entries. Wiring it into code-quality.yml is a separate change so this one stays reviewable. Verified rather than assumed: - `--check` exits 1 before the build and 0 after - executed nl.js, de.js and fr.js against a stubbed OC.L10N.register: each registers under "openregister" with 2,756 / 2,704 / 2,704 keys - no locale's .js ends up with fewer keys than its .json - test:l10n and check:schema-l10n PASS * ci: run check:l10n-js, so the browser catalogues cannot drift again The generator added in the previous commit fixes the drift once. This is what stops it returning. Without a check here, adding a key to l10n/<locale>.json and forgetting the .js is invisible: the server renders Dutch, the browser renders English, and every existing check passes. That is exactly how this app accumulated its backlog, and how a translation PR merged green earlier today having changed nothing a browser loads. The fleet already proved which side of this line matters. keepiq, buildiq, portaliq and humaniq run this check and each caught the same drift inside a single CI run. dossiq, openregister, filinq and pipelinq did not, and had 142, 329, 257 and 1,090 unreachable entries between them. Same code, same generator, different outcome — the check is the whole difference. Appended to the existing frontend-checks list rather than replacing it, so every check this repo already runs still runs. Verified before pushing: the workflow YAML still parses, and `node scripts/build-l10n-js.js --check` exits 0 on this tree, so the new leg is green on arrival rather than red for someone else to clean up.
* style(notifier): fix two phpcs errors that landed on development #2864 merged while its `PHP Quality (phpcs)` cell was red, so `development` now carries both: * an inline IF — the display-name fallback ternary * a positional call to `displayName()` Neither is a behaviour change; both block every subsequent PR's phpcs cell until they are gone, which is why this is its own small PR rather than a follow-up. 🔑 I had reported this file clean. The run was real and described an earlier version of it: I ran phpcs, then made two more edits to localise the rich strings, and never re-ran. A check that ran before the last edit is not a check on what shipped. Reproduced, fixed, re-verified: phpcs, phpmd and phpstan clean on lib/Notification; 20 tests / 42 assertions green. * test(notifier): cover the two displayName branches the ratchet found The coverage ratchet failed this PR on a 0.09% drop, and it was right. Turning the fallback ternary into an explicit early return took the file from 182 statements to 184, and one of the two new ones — `return \$uid` when the display name is blank — had nothing exercising it. Two branches now do: * a BLANK display name falls back to the uid. Nextcloud permits an empty display name, and an empty one here reads as " asks to act on your behalf" — a security decision with no subject in it. The uid is poorer copy and it is always a name for somebody. * NO user manager at all. The collaborator is nullable so the existing hand-built construction keeps binding, and that is only safe if the null path is exercised — an untested optional dependency is one whose absence is discovered in production. A 0.09% drop is small enough to wave through and was a real gap. 12 tests / 28 assertions. --------- Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
…2876) * fix(services): give three services the constructors they never had NotificationService, EndpointService and UploadService each declared typed `readonly` properties and had NO CONSTRUCTOR AT ALL: NotificationService $notificationManager, $groupManager, $logger EndpointService $endpointLogMapper, $logger, $userSession, $groupManager UploadService $client PHP does not object at parse time, at autoload time, or in any static analyser this fleet runs. It objects the first time the property is READ, with a fatal: Typed property NotificationService::$notificationManager must not be accessed before initialization So these were not degraded paths - they were dead code that killed the request. The configuration-update notification route could not run at all, and neither could ConfigurationController's import nor ConfigurationCheckJob's cron tick, both of which inject NotificationService. EndpointService's permission checks (lines 402, 410, 457) and its audit-log write (487) were in the same state. Found via learniq's e2e server log, on POST /apps/openregister/api/configurations/8/import - a defect in THIS repository that only a downstream consumer's browser test could see, because nothing here ever constructed the objects. Adds a mechanical guard so it cannot recur: a contract test that scans lib/ for typed properties which are declared with no default, never promoted, and never assigned. It is source-based rather than reflection-based because loading these classes needs the Nextcloud server, which unit tests do not have. The guard carries its own control - a second test writes a synthetic class with the exact defect and asserts the detector reports it, so a future refactor of the regexes cannot quietly turn the suite into a no-op. Verified against the real bug too: reverting NotificationService to its previous state fails the guard, restoring the fix passes it. * test: inject NotificationService instead of reflecting into it The suite constructed the service with no arguments and then used ReflectionProperty::setValue() to populate three readonly properties, with a comment stating the reason: // NotificationService has no constructor, so we use reflection to // set readonly props That comment was accurate, and that is exactly the problem. The class genuinely had no constructor, so nothing assigned those properties in production and every method died with 'must not be accessed before initialization'. The reflection workaround kept this suite green over a class that could not run anywhere else - the test documented the defect and then routed around it. Now that the class has a constructor, injecting normally is both the simpler setup and the one that would have failed loudly the moment the constructor went missing. Full unit suite after the change: 17342 tests, 0 failures, 0 errors. * test(services): construct the three services, so the new constructors are covered The coverage ratchet failed this PR: "base 250/284 -> head 252/290 statements. This change adds 6 statements to those files." The six are the constructor assignments themselves, and nothing executed them. `Contract\\TypedPropertyInitialisationTest` parses constructor BODIES, so it already catches a dropped assignment — I removed one and watched it fail. What it cannot do is RUN a constructor, so those statements stayed uncovered. It also only checks that each property is assigned something, not that the right collaborator lands in the right property: duplicating one assignment leaves it green. Both measured, not assumed. This constructs each service and reads back what was stored, via reflection because the properties are private. `assertNotNull($service)` would pass against an empty constructor body, which is the original bug exactly. Includes the `UploadService(null)` default branch — the production container constructs it with no argument, so an unbuilt default would leave the property as uninitialised as before. 4 tests, 9 assertions. phpcs clean. --------- Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…tors (#2898) Follows the constructor fix (#2876). Those three services had been unreachable, so their tests had grown around that: each one built the object in a way production cannot. EndpointServiceTest's TestableEndpointService said so in its own docblock: Test-only subclass to inject dependencies since EndpointService has no constructor. and used Closure::bind to write four private properties from outside the class. All 78 tests passed against a wiring that existed only in that file. The subclass now calls parent::__construct(), so those 78 exercise the same construction path production uses and would fail loudly if it went missing again. (NotificationServiceTest's reflection workaround was replaced the same way in #2876.) Adds coverage for what was never reachable: UploadServiceSourceRoutingTest (6 tests) covers getUploadedJson's routing - the four private helpers that had no test at all, driven through the PUBLIC entry point rather than by reflection, because the routing to them is exactly what was broken. Includes the ordering that matters: internal `_`-prefixed params are stripped BEFORE the source check, so a body of nothing but control params is a 400 rather than falling through to the json branch. NotificationPayloadTest (3 tests) pins the notification contract. The eight existing tests stub the notification with willReturnSelf(), which asserts the call chain does not break and says nothing about the subject key or parameter names - so renaming `configuration_update_available` or dropping `currentVersion` would leave all eight green while every consumer stopped recognising the message. lib/Notification/Notifier.php reads those parameters back BY NAME. Mutation-checked: renaming the subject key and dropping the 'unknown' version fallback fails 2 of the 3 new tests. Restoring passes. Full unit suite: 17507 tests, 0 failures. phpcs clean. Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
…overage (#2899) * test(aggregation): remove the coverage metadata that was discarding coverage Closes #2847. Under `beStrictAboutCoverageMetadata="true"`, PHPUnit does not merely restrict recording to the units a test names — it marks any test that executes anything else RISKY and throws that test's coverage away entirely. The message is explicit once you look: This test executed code that is not listed as code to be covered or used: - OCA\OpenRegister\Db\Register - OCA\OpenRegister\Db\Schema - OCA\OpenRegister\Service\Aggregation\AggregationQuery Almost every test in this directory legitimately runs a collaborator — AggregationQuery, PlaceholderResolver, the Db entities — so naming the class under test did not focus the measurement, it deleted it. Measured locally with pcov, identical scope both runs (237 tests, 461 assertions): with @Covers without AggregationRunner 44.39% (613/1381) 80.30% (1109/1381) ...methods 9.80% (5/51) 33.33% (17/51) scope statements 1618 2137 Risky tests 33 0 Same tests, same assertions, same executed lines — only the attribution differs.⚠️ Those are pcov numbers. coverage-guard.php's own docblock records that CI measures with xdebug and the two do not count statements identically, so treat the DIRECTION as the result and let CI's `--against` measurement be the authority. `.coverage-baseline` is untouched: the guard treats a measurement above the floor as good news, and this only moves it up. `TimeseriesRequestValidatorTest` had `@coversDefaultClass` with not one `@covers ::method` to pair with it — naming a default nothing used, while still restricting recording. The reasoning already existed, measured, in AggregationJoinAndCompositeGroupByTest's docblock; the other ten files simply never had it applied. Each now carries a short note pointing there, so the annotation is not helpfully restored later. Full suite: 17336 tests, 0 failures. * fix(test): name the annotation without its at-sign in the docblocks CI failed all six PHPUnit cells with "@Covers ::method`" is invalid My own explanatory comment was the cause. PHPUnit parses a CLASS docblock, so the sentence describing what had been removed re-declared it — and the method-scoped spelling is malformed, so it errored rather than being tolerated. A comment about the bug became the bug. Every docblock added by this branch now names the annotation without a leading at-sign, and TimeseriesRequestValidatorTest says why so the next person does not helpfully "fix" the prose. (I wrote the replacement comment containing the same literal string once more before catching it. It is a genuinely easy trap: the natural way to document an annotation is to write it.) The pre-existing mentions in AggregationJoinAndCompositeGroupByTest are left alone — those parse harmlessly; only the method-scoped form is malformed. Full suite: 17498 tests, 0 failures, no invalid annotation.
…rators (#2904) Two changes to the same machinery, both about a filter that silently answers the wrong question rather than failing. CONDITIONAL METRICS `metrics[].condition` scopes ONE figure to a subset of the grouped rows. That is what a debit/credit split needs: `totalDebit` and `totalCredit` are the same SUM over the same field, separated only by `side`. Declaring two aggregations instead is not equivalent — it groups and scans the table twice, and the two results can disagree if a row is written between the calls, which is exactly what a trial balance must never do. It is a FILTER OBJECT, deliberately, not a SQL string: the same shape as the aggregation's own `filter`, through the same applyFilter(). One grammar, one implementation. A second string-shaped grammar is how a consuming app ended up with 265 declarations this engine could not read. `metrics[].as` names the response key, and conditional metrics REQUIRE it: two conditional sums over one field both derive `sum_amount`, so the second would overwrite the first and return one figure where the caller asked for two. The native SQL path REFUSES a conditional spec rather than running it. tryNativeMultiMetric() aggregates every entry over the same filtered rows and keys from metric+field, so it would drop the condition and collide the aliases — answering wrongly and fast. 🔑 Writing the test found a defect in the implementation: AggregationQuery::getMetrics() rebuilt each entry as {metric, field}, STRIPPING condition and as before the runner ever saw them. Must-fail control: revert that and `openTotal` comes back 35.0 — the unconditioned total — instead of 30.0. The validator refuses a string `condition`, an empty `as`, and a condition naming a property the schema does not declare. That last one matters most: at run time such a filter is not an error, it matches nothing and returns an empty result, which a page renders as "no data" over live rows. UNKNOWN FILTER OPERATORS NOW THROW checkOn()'s `default => true` let an unrecognised operator match EVERY row, so the filter widened instead of narrowing. Measured in shillinq: `{"not-in": [...]}` — the implemented spelling is `notIn` — meant an AR-ageing report silently included settled invoices. Blast radius measured before changing it: exactly ONE metric-bearing aggregation in shillinq uses an unknown operator (APInvoice.apAging), and it is wrong today. The rest (`equals`, `not`, `between`, `gteOrNull`, `notStartsWith`, `not_in`) sit on aggregations that compute nothing yet, so they will now fail loudly when someone gives them a metric rather than returning a quietly widened set. Verified: 17506 tests, 0 failures; phpcs and phpmd clean on the changed files; both must-fail controls confirmed.
…#2900) * fix(flow): enforce the assignee that AwaitSignalNode already recorded ADR-098 names this gap exactly: "no task authz — anyone reaching the resume endpoint can decide". Concretely, AwaitSignalNode has ALWAYS written an `assignee` onto the suspension, and nothing ever read it back. The only check on POST /api/flow-runs/{uuid}/resume asked "may you run this flow?", which is a different question from "is this decision yours to make" — so everyone who could run the flow could approve a step assigned to someone else, while the recorded assignee made it look otherwise. A field that looks like authorization and is not is worse than no field. The resume endpoint now refuses when the awaiting step names an assignee and the caller is neither that uid nor a member of that group. Anonymous is refused outright: an assigned decision is never anonymous. SCOPE, STATED HONESTLY. This closes the WHO of an already-recorded assignment. It is NOT the task entity, inbox or definition versioning ADR-098 describes — those remain unbuilt, and Wave 4's flow consolidation still depends on them. A step with NO assignee is deliberately unchanged: silence still means anyone, because most await-signal suspensions are webhooks and child-run completions rather than human decisions, and tightening that would break every one of them. Mutation-checked, because a security guard that cannot fail is the worst kind: replacing the assignee lookup with an unconditional allow makes the suite fail. * test(flow): cover the assignee guard's branches, not just its happy paths The coverage ratchet failed this branch: it adds 33 statements to FlowRunController and only 25 of them were reached, so coverage of the code this change keeps or adds fell from 83.93% to 82.59%. The three original tests covered the shape of the guard - assigned to someone else, assigned to you, not assigned. What they did not cover were the branches where getting it wrong is quiet: - An assigned step answered with NO session. This is the fail-CLOSED half, and it is the half that is easy to invert, because the unassigned case deliberately lets anyone through: an implementation that treated "no uid" the same way would pass every other test in this file while leaving an assigned decision open to an unauthenticated caller. - A GROUP assignee. AwaitSignalNode records one `assignee` string without saying whether it names a person or a group, so the guard tries both. Without a test, a broken group lookup refuses the step's own intended audience - and reads as "the guard works", because refusing is what a guard does. Both the member and the non-member case are asserted, so the lookup is a check rather than a rubber stamp. - The three shape guards in recordedAssignee(). The context is stored JSON written by older runs under earlier shapes. A slot that has not asked yet must not gate the step that is asking, or a future step refuses the right person now; a malformed slot must read as unassigned rather than 500 an in-flight run. Each is mutation-checked: disabling the anonymous refusal, the group branch, and the askedAt guard each make the suite fail. 18 tests -> 24. --------- Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
…ns (#2901) * test(e2e): make the delegation suites establish their own preconditions Running the three delegation suites live against merged `development` gave a different answer three times over identical code — 18 passed, then 8 failed, then 2 failed. Neither the code nor the assertions were wrong; the suites were inheriting state instead of establishing it. Two separate causes, both of which let a run report something it had not measured: 1. A HARDCODED FIXTURE UID. `NEXTCLOUD_OTHER_USER || 'ddauth-alice'` named an account that existed when the specs were written and did not exist after the dev instance was rebuilt. The failure read `"ddauth-alice" resolves to no account you may ask` — which is the delegation guard's own refusal message, i.e. a dead fixture wearing the words of a working control. The uid is now DISCOVERED from the instance, and a single-account instance SKIPS with a sentence naming what went unverified rather than failing for a reason that is not about the code. 2. A LEAKED GRANT. Every suite opens by asserting the save is REFUSED — the baseline the later "now it saves" assertion is measured against. Grants outlive a run, and a suite killed mid-way (a `head` closing the reporter's pipe is enough) leaves a `granted` row behind, so the next run's baseline got a cheerful 201. The failing direction was the lucky one: a leaked REVOKED grant would have made the same baseline pass for the wrong reason and the suite would have proved nothing. Each suite now revokes any live grant over its target before asserting. The account probe deliberately does NOT swallow errors. An earlier draft wrapped it in `catch { return null }`, and when the probe came back as Nextcloud's login page — HTTP 200, HTML, `.json()` throws — the catch reported "no second account", eleven specs skipped citing a single-account instance, and the run said `0 failed`. It now throws with the status and body, because a probe that cannot answer must say so. Also lets `NC_CONTAINER=nextcloud` through under an explicit `NC_ALLOW_SHARED_CONTAINER=1`. The guard exists to stop an accidental default, not to make the parking path — which only exists once a real TimedJob ticks — permanently unverifiable. Verified: 18 passed / 0 failed, twice back to back, against merged `development` on a live instance. * test(e2e): prefer the shared dev container for occ, gate only restarts The container guard refused the shared `nextcloud` container for every purpose, and that was too blunt in both directions. 🔑 THE TWO ACTIONS ARE NOT THE SAME RISK, so they no longer share a rule. `resolveContainer('exec')` — the default — now DEFAULTS TO the shared container instead of returning null. Running one named `occ` command there is how the dev box is meant to be exercised, and refusing to do so bought nothing: it made every spec that needs a real TimedJob tick skip everywhere. The delegation parking suite is the clearest casualty — a run parked on `awaiting_consent` and released by a cron sweep only exists once a job actually runs, so the headline behaviour of that subsystem was verified by nothing but a unit test, while the summary said "3 skipped" in a tone indistinguishable from "3 passed". `resolveContainer('restart')` still refuses the shared container without an explicit `NC_ALLOW_SHARED_RESTART=1`. That is the action the original guard was really about: `docker restart nextcloud` bounces an environment that bind-mounts several developers' working trees, mid-session, with no warning to them. One `occ` command is recoverable; restarting somebody else's instance is not. `federated-config-store.spec.ts` is the only caller that restarts, and it asks for that purpose explicitly. Verified with NO container env set at all — the new default path: 26 passed, 5 skipped, 1 failed All three delegation-parking tests now RUN and pass, including the park and the release through real FlowScheduleWorker and FlowRunWorker ticks. The remaining failure is pre-existing and unrelated: `federated-config.spec.ts` (like `flow-schedule.spec.ts`) needs a `flows` REGISTER, which this rebuilt dev instance never seeded — its beforeAll fails on the register lookup regardless of any container setting. * style(e2e): prettier-format the delegation fixtures module
Both classes are fully written, implement IRepairStep, and were named ZERO times in appinfo/info.xml. Nextcloud only runs what the manifest declares, so neither has ever executed. gate-98 (repair-step-registration) catches it. A class that exists is not a class that runs, and neither failure is visible: - ImportFlowRegister creates the `flows` register and flow schema. Without it that register was never created, so every flow step listed BELOW it — MigrateRenamedFlowNodeTypes, BackfillFlowTriggerIndex, InitializeFlowActions — silently operated on nothing. It is therefore registered ABOVE them, in both <post-migration> and <install>. - RenameDutchColumns moves stored data from the Dutch columns to the English ones the register now declares. MagicMapper ADDS a column when a snake_cased property is absent and never renames — there is not one RENAME COLUMN in the app — so a renamed property leaves the data in the old column while every read looks at the new one and finds null. No error, no data loss, and invisible to a suite that asserts against fixtures rather than migrated rows. For shillinq those columns carry invoice, subsidy, payroll and tax amounts. Its own docblock documents the step as non-destructive and idempotent: it renames only when the old column exists and the new one does not, copies across and LEAVES the old column when the new one already exists, refuses two sources targeting one destination, and deletes nothing. A re-run is a no-op. Registered under <post-migration> ONLY — a fresh install has no Dutch columns to move, so listing it under <install> would be a guaranteed no-op. Pre-existing on development: neither name appears in its info.xml. This is not introduced by any open PR; gate-98 is full-tree, so it reddens every branch until this lands. Note for whoever cuts the next release: repair steps run on upgrade, so these two first execute on the next version bump.
Contributor
Author
Quality Report — ConductionNL/openregister @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ❌ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-specs | ✅ | ||||
| test-l10n | ✅ | ||||
| format | ✅ | ||||
| check-schema-l10n | ✅ | ||||
| check-l10n-js | ✅ | ||||
| composer | ✅ | ✅ 175/175 | |||
| npm | ✅ | ✅ 545/545 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ❌ | ||||
| Newman | ❌ | ||||
| Playwright | ❌ | ||||
| Hydra gates | ❌ |
Quality workflow — 2026-08-27 10:02 UTC
Download the full PDF report from the workflow artifacts.
…to nobody (#2915) Refs #2905. `FlowShareableConfigType::deserialise()` stored an imported flow with `owner = null` and `organisation = null`, on the reasoning — correct as far as it went — that the SENDER's identity means nothing on this instance. But null is not "the absence of the sender's identity". It is the absence of ANY, and `FlowMapper` scopes every read with an EQUALITY predicate: $qb->andWhere($qb->expr()->eq('organisation', $qb->createNamedParameter($organisation))); `NULL = 'anything'` is never true in SQL. So the row inserted, `install` returned its uuid with HTTP 200, and the flow was excluded from `flow#index`, `flow#show` and the run path BY CONSTRUCTION — with no adopt route to rescue it and no way for `flow#update` to reach it either. Install was a one-way door. Five permanent orphans on one dev instance. 🔴 THE RULE WAS ALREADY FIXED — ON THE OTHER WRITER. `FlowService::flowToSave()` refuses this exact write, and its comment describes this exact outcome: > REFUSE rather than stamp nulls. […] a flow with no organisation belongs to > nobody: it does not appear in index(), find() refuses it, and it can never > be run or edited again. Accepting the write produced a permanent orphan and > reported success — the caller had no way to tell that from a flow that saved. Two writers each deriving ownership their own way is how the rule came to hold on one and not the other. Both now read `FlowService::callerOwnership()`, so there is ONE place that decides it, and `deserialise()` refuses rather than stamping nulls — the same refusal, on the path that was missing it. THE SECURITY PROPERTY IS UNCHANGED, and it never depended on the null. `Flow::canDispatch()` requires `enabled === true` AND a non-empty owner, so `enabled = false` alone already refuses dispatch. A bundle still cannot arrive and start executing against the receiving tenant's data, and the sender's claimed `owner`/`organisation` are still discarded. What the null added was not safety — it was unreachability. A TEST REQUIRED THE DEFECT. `testAnImportedFlowLandsDisabledAndOwnerless` asserted `getOwner() === null`, so the orphan was pinned as a requirement. It is now `testAnImportedFlowLandsDisabledAndOwnedByTheInstaller` and asserts both halves: the sender's claim is rejected (`not 'victim'`, `not 'their-org'`) AND the installer owns it. A second test pins the refusal when no caller resolves. Verified: 10 tests, 33 assertions, OK. phpcs clean on both changed lib files.
…2916) Plan item 4, and the largest single capability #1261 is waiting on — 11 declarations need it. `groupBy: ["AnalyticalDimension.parentCode"]` asks to roll parent rows up to a column the parent does not have. applyJoin() cannot produce it: it runs AFTER grouping and attaches figures to groups that already exist, and by then the rows are gone. So the value is projected onto each parent row FIRST, through the join key, and grouping proceeds normally. The result is a roll-up to the joined dimension — a cost-centre hierarchy summing its children, which is the shape this exists for. THREE THINGS THE TESTS FOUND, each a wrong answer rather than an error: 1. The `on` SHORTHAND cannot be used here, and now says so. "Schema.column" infers the parent-side field FROM THE GROUP FIELDS — same-named one if present, otherwise the first. When the group field is the joined one, that inference picks the JOINED field as the parent key, reads it off rows that do not have it, and puts everything in one bucket. Measured: a fixture summing 350 + 7 came back as a single bucket of 357 keyed ''. The explicit {parentField: joinedField} map names both sides. 2. The post-grouping merge had to be SKIPPED. After projection the group key IS the joined dimension, so the original join key is no longer in the row; mergeJoinedValues() would look up a tuple that cannot match and hang a map of nulls on every group — reading as "the joined schema had nothing" rather than "the grouping already answered this". The envelope now reports `join.consumedForGrouping: true`. Attaching figures there would mean re-aggregating the joined schema BY the projected dimension, a second and different join, which is not invented silently. 3. An unmatched parent row keeps a NULL group instead of being dropped. Dropping it would quietly shrink every total with nothing saying so. The test pins 350 + 7 + 11 = 368. The native SQL path refuses a join-qualified group field: its SQL is emitted against the parent table alone, so the column exists on no row and every row would land in one null bucket, reported as a working total. Also: the map is read directly rather than through resolveJoinKey(), which enforces "every parent-side field must be one of the groupBy fields" — right for the post-grouping merge, false here by construction. phpmd flagged the first cut at complexity 20 / NPath 25600; split into joinedProjectionKeyMap(), loadJoinedRowsForProjection() and stampProjectedColumn(). Both tools clean. Verified: 17526 tests, 0 failures; phpcs 0 errors; phpmd clean; must-fail control confirmed (disabling the projection reddens both roll-up tests).
Contributor
Author
Quality Report — ConductionNL/openregister @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-specs | ✅ | ||||
| test-l10n | ✅ | ||||
| format | ✅ | ||||
| check-schema-l10n | ✅ | ||||
| check-l10n-js | ✅ | ||||
| composer | ✅ | ✅ 175/175 | |||
| npm | ✅ | ✅ 545/545 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ✅ | ||||
| Hydra gates | ❌ |
Quality workflow — 2026-08-27 11:11 UTC
Download the full PDF report from the workflow artifacts.
…2917) * feat(graphql): filtered groups, and declared aggregations by name Two changes to the GraphQL aggregation surface. 1. A FILTERED LIST RETURNED UNFILTERED GROUP TOTALS resolveList() maps the query's `filter` into the row list. resolveGroupBy() built its aggregation input with `'filter' => []` HARDCODED, so the edges honoured the filter and the groups silently did not — group totals were computed over the whole schema. gLLines(filter: {eliminationFlag: false}, groupBy: {field: "accountNumber", metric: SUM, metricField: "amount"}) { edges { node { amount } } # filtered groups { key value } # NOT filtered — every GLLine } Nothing errored. The caller was shown a bigger number than the rows it was given, which is the hardest kind of wrong answer to notice on a dashboard, and launchpad widgets read these over runtime GraphQL. Only the PROPERTY filter forwards. Paging must not — a total over "the first 20 rows" is not a total and would change as the user paged. `search` must not — it is a relevance query the aggregation engine does not implement, so forwarding it would filter on a property named `_search`, match nothing, and return an empty result rather than an error. `selfFilter` addresses @self metadata, a different namespace. Each omission is deliberate and documented, because anything left out means the groups describe a wider population than the rows. 2. DECLARED AGGREGATIONS ARE NOW REACHABLE FROM GRAPHQL `groupBy` is ad-hoc: the caller describes the aggregation. A schema's `x-openregister-aggregations` were REST-only, so a page wanting a declared figure had to hand-build a URL alongside its GraphQL query. gLLines(filter: {...}, aggregation: "consolidatedTrialBalance") { edges { node { amount } } aggregation } The name is the whole input — the declaration already carries the metric, grouping, filter and join, and was validated at save time. The query's filter is passed as a NARROWING constraint, which is safe by construction: the engine refuses any request key the declaration already pins, so a caller can add a constraint and can never relax a declared scoping one. The envelope is JSON, deliberately. A declared aggregation's shape varies with what it declares — a scalar `value`, a `values` map for `metrics`, `groups[].keys` for a composite groupBy, `joined` when it joins. Typing it now would repeat the mistake GroupBucket already makes, where `value: Float!` cannot carry a values map and a null group key coerces to "". A typed AggregationResult is the right next step once a consumer needs introspection over it. Verified: 17520 tests, 0 failures; phpcs 0 errors; phpmd clean. The filter fix has a must-fail control — reverting it reddens the two filtered tests while the unfiltered control correctly stays green. * feat(graphql): let a bucket carry what the engine actually returned Plan item 3. GroupBucket flattened the engine's result into two scalars, and each coercion lost something real. 'key' => (string)($bucket['key'] ?? '') 'value' => (float)($bucket['value'] ?? 0) A NULL group key became '' — rows whose grouped field is null were indistinguishable from rows whose value is genuinely the empty string. And a multi-metric bucket carries `values` with no scalar `value` at all, so the float cast reported 0.0 FOR EVERY BUCKET rather than admitting the figures live under another key. The second was unreachable only because GraphQL could not ask for `metrics[]`. Widening the input without widening the bucket would have made it reachable, which is why both halves land together. key String (was String!) — null stays null value Float (was Float!) — null for a multi-metric grouping keys JSON — composite group key as {field: value} values JSON — figure per response key, incl. `as` aliases joined JSON — figures from a joined schema The input widened to match: `fields` for composite grouping, and `metrics` with per-entry `condition` + `as` for the conditional split. VALIDATION STAYS IN ONE PLACE. TimeseriesRequestValidator handles both new keys and delegates the metric entries to AggregationMetricsAnnotationValidator — the SAME class the annotation path uses. An ad-hoc request and a declared aggregation cannot drift in what they accept. A validator and an executor each owning a copy of the grammar is exactly how this engine acquired specs it could not run; adding a third copy for GraphQL would have repeated it. `condition` is JSON because it is a filter OBJECT, the same shape as the aggregation's own filter — deliberately not a string expression. Must-fail control: restoring the two coercions reddens both new tests with "Failed asserting that '' is null" and "Failed asserting that 0.0 is null". Verified: 17522 tests, 0 failures; phpcs 0 errors; phpmd clean. * fix(graphql): cache AggregationMetricInput in the shared map, not its own field phpmd failed the PR on two counts, both from the same addition: a dedicated `$aggregationMetricInputType` property took TypeMapperHandler to 16 fields (TooManyFields, threshold 15) and its name was past the LongVariable limit. The $inputTypes map already exists for exactly this — a shared input type cached by purpose — so the type moves there under a 'shared:' key. No behaviour change: same instance, same single construction, same reuse.
… an unanswerable @self (#2922) Two ways a cross-schema aggregation returned a plausible wrong number. 1. `metrics` was never read when `from` was set. runCrossSchema() resolved only the SINGULAR `metric`/`select`, so a spec asking for several conditioned figures fell through to the default `count`. A debit/credit segment P&L came back as a ROW COUNT under HTTP 200, with nothing in the envelope naming what had been dropped. The intra-schema path has honoured `metrics` since #2917 — two paths reading different halves of the same declaration is drift that answers wrongly rather than erroring. The cross-schema path now extracts `metrics`, includes it in the cache key, skips the native path when it is present (tryNativeAggregation() takes one metric/field pair and has nowhere to put per-entry `condition`/`as`), passes it to the computeGrouped(metrics:) branch that already existed, and yields `values` rather than a scalar when ungrouped. 2. An `@self.<field>` the parent row cannot answer resolved to null. This was described as failing closed. It does not: the null is applied as a real filter VALUE, so the aggregation returns the target rows whose own field is null. For a segment P&L keyed on `@self.code` that is the unassigned-cost- centre total — returned confidently, identically, for every parent record. No production caller supplies a parent row at all: AggregationController, ReportRenderService and ThresholdEvaluationService each call run() without one. So every such declaration answered wrongly rather than visibly. An absent key now raises and names the reference; a key PRESENT but null still correlates on null, which is a legitimate query. The test that pinned the old behaviour asserted 0 against a fixture whose one row happened to have a non-null field. A row with a null there would have returned 1 under the same code — it was pinning the fixture, not the behaviour. It now asserts the refusal, with that fixture inverted. Refs #1261
Contributor
Author
Quality Report — ConductionNL/openregister @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ⏭️ | ||||
| phpcs | ⏭️ | ||||
| phpmd | ⏭️ | ||||
| psalm | ⏭️ | ||||
| phpstan | ⏭️ | ||||
| phpmetrics | ⏭️ | ||||
| eslint | ⏭️ | ||||
| stylelint | ⏭️ | ||||
| build | ⏭️ | ||||
| composer | ⏭️ | ⏭️ | |||
| npm | ⏭️ | ⏭️ | |||
| app:check-code | ⏭️ | ||||
| info.xml | ⏭️ | ||||
| REUSE | ⏭️ | ||||
| PHPUnit | ❌ | ||||
| Newman | ❌ | ||||
| Playwright | ❌ | ||||
| Hydra gates | ❌ |
Quality workflow — 2026-08-27 11:25 UTC
Download the full PDF report from the workflow artifacts.
`setSchema()` stores a pending ref so that a LATER `setRegister()` can
re-resolve the slug inside the register the caller names. When a register is
already set, `setSchema()` resolves immediately and returns early — and left
the ref set anyway, with nothing remaining to resolve.
ObjectService is shared for the whole request, so that ref outlives the chain
that created it. The next caller's `setRegister()` then re-resolves a finished
operation's slug inside a register that has never heard of it, and refuses
that caller.
Measured 2026-08-27 on a fresh instance. buildiq registers its navigation
entries from `Application::boot()`, which runs on EVERY request and calls
findAll() -> prepareFindAllConfig(), which calls setRegister() and then
setSchema('application'). Every request therefore ended with `application`
pending. Portaliq's PortalResolver — typically the first caller afterwards to
name its own register — was told:
Schema slug "application" is not carried by register "portaliq" (id 35)
PortalResolver fails closed by design, so it returned an empty portal list and
every public portal page 404'd, with nothing in the error attributable to
portaliq. Confirmed by instrumenting setSchema() to record its caller: the
producer was buildiq's boot, several frames below OpenRegister's own findAll().
The same leak reached buildiq (`automation`) and hermiq (`job_log`) on cron.
#2803 cleared the ref on the save path. This is the read path, which it did
not cover — verified against origin/development before writing the fix.
Verified live: with the fix applied, the previously-404ing portal renders in a
browser (heading, search hero, menus, footer) with no other change.
Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
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
2 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.