Release: merge development into beta - #79
Open
github-actions[bot] wants to merge 549 commits into
Open
Conversation
…rage gaps - Fixed the "Logo defaults..." requirement anchor (apostrophe slugifies to "-s-", not "s") across all three files that reference it. - Added missing @SPEC tags to every changed method/computed flagged by hydra gate-16 (spec-coverage): AppBrandedHeader.vue, AppCustomThemeSection.vue (including the headerStyleOption get/set pair), useAppCustomTheme.js, checkThemeContrast.js helper functions. Result: 38/39 hydra gates green (--scope-to-diff); the sole remaining failure (gate-46 spec-anchor-existence) is pre-existing dangling @SPEC references to archived change directories, verified identical on origin/development before this change.
feat(app-theming): logo + 3-color + header-style theming with WCAG contrast guardrail
- Move openspec/changes/app-theming/ to openspec/changes/archive/2026-07-24-app-theming/ - Sync openspec/specs/app-theming/spec.md from the archived delta (Status: done), corrected against the actual implementation (var()-fallback precedence mechanism, PageDesignerHost.save() as the persist-boundary hard block, AppCustomThemeSection.vue as the editor location) rather than design.md's implementation-time guesses - CHANGELOG.md entry under the existing [0.7.7] - 2026-07-24 release
docs(openspec): archive app-theming
…run log Adds the Agent Workspace feature on top of the existing ai-copilot plan/ execute engine (ADR-022 consume-not-rebuild, design.md Decision 1): - Agent + AgentRun schemas (register.d fragment 70-agent-workspace.json). enabledTools is validated against the eight OpenBuildToolProvider tool ids via a schema-level enum (declarative, ADR-031). - CopilotService::plan()/execute() gain an optional agentId that resolves the Agent server-side (never trusted from the client beyond the id), narrows the effective tool allow-list to the intersection with the agent's enabledTools, and prefixes its instructions onto the system prompt. maxActionsPerRun is enforced at plan-acceptance time (422 naming max_actions_per_run). A new discard() endpoint logs a discarded proposal. - AgentRunLogger persists one AgentRun per plan+execute/discard turn (prompt, plan, every tool call's arguments+result, outcome), as a thin wrapper CopilotService calls only on the agent-scoped branches — the bare copilot path is unchanged and unaffected. - AgentsController::runs() serves the owners/editors-only run-history read with an explicit per-object RBAC guard (closes the IDOR shape a bare #[NoAdminRequired] + generic OR REST read would have). - Frontend: AgentsPage.vue (CRUD + chat + history), AgentEditDialog.vue, AgentRunHistory.vue; CopilotPanel.vue gains optional agentId/name/ instructions/enabledTools props, fully backwards-compatible. PHPUnit: 729 passing (baseline 699 + 30 new). Vitest: 1366 passing (baseline 1338 + 28 new). Playwright e2e written, CI-run only per policy.
…ge, no-admin-idor exempt, e2e visual reference
…gelog - Move openspec/changes/agent-workspace/ to openspec/changes/archive/2026-07-24-agent-workspace/ - Sync openspec/specs/agent-workspace/spec.md from the archived delta (Status: done) - Apply the MODIFIED REQ-OBAIC-002/004 requirements + new agent-scoped scenarios into openspec/specs/ai-copilot/spec.md; mark agent-workspace archived in its OpenSpec changes list, Status: done - CHANGELOG.md entry under the existing [0.7.7] - 2026-07-24 release
…fs to the archived change path
feat(agent-workspace): named, tool-scoped AI agents with transparent run log
… new JS methods
feat(runtime-group-scoped-access): server-side group-scoped menu/page filtering
Deploy marker for the 2026-07-23/24 market-deepdive wave (7 merged features: public-forms-runtime, automation-approval-steps, automation-document-action, component-blocks, app-theming, agent-workspace, runtime-group-scoped-access). Version bump triggers InitializeSettings repair (register.d fragment import) and busts the frontend ?v= bundle cache.
chore(release): bump 0.7.7 → 0.8.0 (deploy marker for deepdive wave)
…liq/OR and theming to nldesign public-forms-runtime (PR #8) built a public/citizen form surface + ShareToken model + anonymous-write service inside OpenBuild. Citizen-facing rendering is Portaliq's domain, and external-user object creation already exists in OpenRegister (CaseTokenController + FormLinkService). Removed entirely; will be rebuilt correctly later as a thin leaf. app-theming (PR #15 + archive PR #16) built per-app theming + a WCAG contrast checker + scoped-CSS applier inside OpenBuild. App-specific theming belongs to nldesign (NlDesignTheme). Removed entirely. Deleted all new files from both features (controllers, services, Vue components/composables, dialogs, templates, tests, openspec change/spec dirs) and surgically reverted their hunks in files also touched by later, preserved PRs (routes.php, DashboardController, ManifestResolverService, PageDesigner.vue, ApplicationDetailActions.vue, FormPageEditor.vue, useManifestValidator.js, BuilderHost.vue, PageDesignerHost.vue, webpack.config.js, CHANGELOG.md, openbuild-runtime spec.md) — none of automation-approval-steps (#10), automation-document-action (#12), component-blocks (#13/#14), agent-workspace (#17), runtime-group-scoped-access (#18), or the 0.8.0 version bump (#19) were altered. PHPUnit: 742 -> 715 tests (delta = 27, exactly ShareTokenServiceTest + PublicSubmissionServiceTest), 0 failures before and after. Vitest: 132 files / 1290 tests, all passing after the revert. ESLint (src): 0 errors.
…app-theming revert(public-forms-runtime,app-theming): move citizen forms to Portaliq/OR and theming to nldesign
…ous intake + Portaliq portalPage Rebuilds the reverted public-forms-runtime (PR#8/#20) as a configuration leaf: OpenBuild PATCHes an OpenRegister schema's authorization (read-merge- write, never a partial fragment) and creates/updates a Portaliq portalPage object, then surfaces the resulting public URLs. No PublicPage controller, no anonymous route, no ShareToken model, no rendering — every anonymous surface stays owned by OpenRegister/Portaliq (ADR-022). - externalFormProvisioningService.js: enablePublicCreate/revokePublicCreate (schema authorization) + provisionPortalPage/draftPortalPage (Portaliq), all GET-merge-PUT/PATCH. - ExternalFormAccessDialog.vue: builder toggle wired from FormPageEditor's new "External access" section (gated on an OR-shaped submitEndpoint). - useTrackLinkAction.js + TrackLinkAction.vue: owner-context "mint a track-link" action, registered in runtimeRegistry.js for BUILT apps' Detail pages, gated on runtime.externalForms[].trackLinkAction.enabled. - manifestValidation/externalForms.js: strict shape validation for the new runtime.externalForms[] manifest block, wired into useManifestValidator.
…ed annotations Satisfies hydra gate-16 (spec-coverage) and gate-19 (e2e-coverage): tags the remaining changed computed/methods in TrackLinkAction.vue and ExternalFormAccessDialog.vue, and marks the 5 spec scenarios that need a live running instance as @e2e exclude (deferred — no shared-dev deploy per workflow), pointing each at its Vitest coverage.
…tus=done All 7 requirements implemented and verified (1.1-4.2, 5.3); live Playwright e2e (5.1) and Newman (5.2) deferred — no shared-dev deploy available this session, marked DEFERRED in tasks.md with reasons, unit/vitest coverage substitutes at the request-shape level per the fleet's established DEFERRED convention (see 2026-06-14-procest-workflow-attachments precedent).
feat(external-form-provisioning): thin-leaf provisioning of OR anonymous intake + Portaliq portalPage
Bumps @conduction/nextcloud-vue to ^1.0.0-beta.221 (ships useScopedTheme, CnAppRoot self-application of runtime.theme, manifest schema 2.21.0) and deletes OpenBuild's local scoped-CSS applier (useAppTheme.js) and local theme manifest validation (manifestValidation/theme.js), both now owned by the library. ThemePickerDialog collapses its three-tier admin/probe/free-text fallback to a single useScopedTheme().listTokenSets() call against nldesign's real non-admin GET /api/token-sets endpoint, adds warn-only contrast display via evaluateContrast(), and retargets live preview at the page-designer sandboxed CnAppRoot instance instead of a separate applier. BuilderHost and PageDesignerHost drop their data-openbuild-theme-scope wiring entirely. Closes the loop nldesign-theme-selection's own spec flagged as a stopgap.
Hydra gate-16 flagged the changed setup() method (adds the useLivePreview composable) as missing traceability.
…al spec Syncs REQ-NTS-002/003/006 (modified) and REQ-NTS-007/008 (added) into the canonical nldesign-theme-selection spec, and moves the change to openspec/changes/archive/2026-07-25-theme-picker-consumes-nldesign/.
…design Theme picker consumes nldesign's published useScopedTheme
…ave) Deploy marker for the leaf-architecture correction: reverted misplaced public-forms + app-theming (#20), added thin external-form-provisioning leaf (#21) and theme-picker-consumes-nldesign leaf (#22, consumes @conduction/ nextcloud-vue beta.221 useScopedTheme + nldesign token/contrast endpoints). Version bump busts the frontend ?v= bundle cache and re-runs the settings repair.
…ppHost (gate-64 / ADR-040) (#138) * fix(apphost): register OpenRegister's autoloader before referencing AppHost `openbuild` sorts before `openregister`, and Nextcloud registers apps in sorted order: OC_App::getEnabledApps() does sort($apps) and Coordinator::registerApps() walks that list calling OC_App::registerAutoloading($appId, $path) and then $app->register() for one app at a time. So OCA\OpenRegister\ was NOT autoloadable inside OpenBuild's own register() — on a perfectly healthy instance, with OpenRegister enabled. Measured, not theorised: `OpenBuild: OpenRegister AppHost\Bootstrap is not autoloadable` was logged 3x in the E2E job (every occ call in ci-seed.sh) while lib/AppHost/Bootstrap.php existed on OpenRegister the whole time. Because the reference sits behind a class_exists() guard, this degraded SILENTLY: Bootstrap::register() apparently never ran, so the generic dashboard/settings/preferences controllers, the observability controllers, the install repair steps and the manifest-driven deep-link listener were absent, with nothing in the UI to say so. Fix: put OpenRegister's PSR-4 prefix on the autoloader ourselves before the guard. registerAutoloading() touches only the autoloader and is idempotent. IAppManager::loadApp() is deliberately NOT used: it marks OpenRegister loaded and calls Coordinator::bootApp(), booting it before its own register() has run. The prelude lives in its own class so its 'never throws' contract is reachable from a unit test — Application cannot be constructed without a NC DI container. Caught by hydra gate-64 (apphost-autoload-prelude), ADR-040. * fix(apphost): make the prelude branch-free and declare OC_App to psalm Two CI findings on the prelude, both real: 1. psalm UndefinedClass on \OC_App. It is Nextcloud's server-private legacy bootstrap class, absent from nextcloud/ocp, and there is no OCP interface for registering another app's autoloader. Declared as a suppressed referencedClass in psalm.xml, the same way doriath declares it. 2. The coverage ratchet. `return true` after the call plus `return false` in the catch gave the method a branch that NO environment can exercise both sides of — whichever runs, the other is dead in that run — so the class could never reach full line coverage. No caller ever consumed the return value either: what callers depend on is the class_exists() guard that follows the call. The method is now void with a single statement in the try and a comment-only catch, so every executable line runs in every environment. The tests now assert the two things that are actually observable: that control returns to the caller at all (a Throwable escaping would fail the test, and in production would abort the whole register()), and that a second call does not stack another autoloader. phpmd StaticAccess on the new composition-root call is documented on the calling method rather than baselined. * test(e2e): assert the AppHost-bound routes actually dispatch, and drop the exclusions The spec scenarios for the ADR-040 prelude carried '@e2e exclude composition-root load order'. That was wrong twice over: an exclusion is not evidence, and the load order HAS a user-visible symptom that Playwright can reach. OpenBuild ships no concrete HealthController or MetricsController. Their route targets health#index and metrics#index — declared by \OCA\OpenRegister\AppHost\Routes::standard() in appinfo/routes.php — exist ONLY as DI aliases created by Bootstrap::register(). Without the prelude, class_exists(Bootstrap::class) answered false, Bootstrap::register() never ran, those aliases were never created, and the routes resolved to a class with no binding: HTTP 500, not 404. tests/e2e/apphost-observability.spec.ts asserts /api/health returns 200 with the engine's {status, app, version, checks} shape and app = 'openbuild', and that /api/metrics does not 5xx. It FAILS on the code before the prelude and passes after it — which is exactly what an exclusion cannot do. Verified locally: gate-19 PASSES with the @e2e annotation on the scenario, and FAILS with the annotation removed, so the traceability is real and not a tag that would be counted whatever happened. The absent-OpenRegister scenario is removed rather than excluded: it is not reachable from an HTTP client at all, so it is stated in the requirement prose and asserted in tests/Unit/AppInfo/OpenRegisterAutoloaderTest.php. * test: cover the prelude's degraded path, which no instance could reach The coverage ratchet was right and the code was wrong. Clover for scholiq shows it exactly: line 100 (the registerAutoloading call) count=2, line 101 (the catch) count=0. The catch was never entered — because every instance this suite runs on HAS OpenRegister installed, so getAppPath() never throws. The never-rethrow branch, which is the entire reason this class exists, had never once been executed by a test. register() now takes an optional app id. Production callers pass nothing and get 'openregister'; the new test passes an id that cannot resolve, so getAppPath() throws and the catch runs. The literal stays AT the registerAutoloading call site rather than becoming a signature default, so it remains visible to a reader and to hydra gate-64, which reads that call's arguments. The new test asserts something real rather than merely not throwing: a prelude whose app cannot be resolved must leave spl_autoload_functions() untouched. * fix(apphost): drop the error_log else-branch, point the @SPEC anchors at specs, and correct the blast-radius claim Three things, all forced by measurement rather than taste. 1. gate-2 forbidden-patterns FAILS on this branch and PASSED on the baseline run (31081906401). Nothing new was added: `error_log()` has been in Application.php all along, and gate-2 is diff-scoped, so touching the surrounding lines is what put it in scope. The else-branch is removed rather than suppressed. With the prelude above it, a false `class_exists()` no longer means "not autoloadable YET" — it means OpenRegister genuinely is absent, which is a whole-instance condition an app cannot usefully narrate from its composition root with no PSR logger resolvable. /api/health, bound by `observability => true`, is where that state is readable. Same choice doriath made. 2. gate-46 spec-anchor-existence FAILS on two `@spec` tags pointing at `openspec/changes/archive/2026-05-12-openbuild-rbac/tasks.md`. An archived change dir is never a valid @SPEC target; the canonical home is `openspec/specs/openbuild-rbac/spec.md`, which exists. 3. The prose overclaimed, and the overclaim is the interesting part. Comparing the pre-prelude run 31081906401 with this branch's run 31085597692: - `OpenBuild: OpenRegister AppHost\Bootstrap is not autoloadable` appeared 3x before and 0x after — once per `occ` call in ci-seed.sh. So under the CLI SAPI the guard really was answering false on a healthy instance, and every occ command, background job and repair step lost the whole generic AppHost plumbing. - But in that SAME pre-prelude run, `/api/health` returned 200 with `status: ok` and `/api/metrics` rendered the manifest gauges. OpenBuild ships no concrete HealthController/MetricsController and `health#index` exists ONLY as a `Bootstrap::register()` DI alias — so under the web SAPI the guard was answering TRUE and Bootstrap::register() did run. The CLI/web divergence is not explained. So the claims "on every single request" and "had apparently never run" are withdrawn from Application.php, OpenRegisterAutoloader.php, REQ-OBS-006 and the e2e docblock, and replaced with what was actually measured. In particular tests/e2e/apphost-observability.spec.ts is relabelled: it did NOT fail before the prelude (both routes already answered 200), so it is a regression guard on the aliases, not the before/after evidence. The before/after evidence for the prelude's own contract is the unit test. Verified locally against this tree: - tests/Unit/AppInfo/OpenRegisterAutoloaderTest.php: OK (3 tests). - Negative control: deleting the try/catch from the prelude turns all 3 into errors ("Class \"OC\" not found"), so the never-throws contract is a check that can fail, not a check that always passes. * style(phpcs): no blank line between the removed-else note and the comment that follows Removing the else branch left an inline comment block followed by a blank line, which Squiz.Commenting.InlineComment rejects: phpcs went 0 ERRORS -> 1 ERROR at Application.php:191 while every other job held. Verified locally against this tree — full-tree phpcs now exits 0 with 0 errors. Also drops 'on every single request' from that same note. The error_log line was measured on occ calls (3 per E2E run); whether web requests reached it was never established, and the comment should not claim more than the measurement does. * fix(deps): squizlabs/php_codesniffer 3.13.5 -> 3.13.6 (CVE-2026-67434) quality / Security (composer) went success -> failure between run 31095490811 (11:11Z) and run 31097941416 (11:42Z) on this branch. Nothing in the branch changed in between: the advisory was published 2026-08-05T23:53Z and reached the Packagist advisory database in that window. Package: squizlabs/php_codesniffer CVE-2026-67434 — OS Command injection Affected: <3.13.6 | >=4.0.0,<4.0.2 The composer.json constraint (^3.9) already permitted the patched release, so this is a lock-only change: one package, 5 lines. Verified afterwards with 'composer audit' -> 'No security vulnerability advisories found', full-tree phpcs exit 0 / 0 errors, and the prelude unit suite still OK (3 tests). This is a dev dependency and unrelated to the ADR-040 fix, but it blocks the merge and the fix is a version bump rather than a suppression, so it lands here instead of behind an ignore entry. --------- Co-authored-by: Ruben van der Linde <juan.claude@conduction.nl>
) min-version is enforced at install time, so a 32 floor makes occ app:enable refuse on stable31, which this repo's own CI runs. The e2e seed then fails with "is not installed or enabled". The reason the floor was raised no longer holds: openregister#2372 removed every eager reference to its ContextChat provider, so the class is only loaded behind interface_exists() guards and never read on an older server. openregister#2380 restored its own 28 floor on that evidence.
… not added one (#127) * fix(security): the setup wizard's #[NoAdminRequired] removed a layer, not added one Hydra gate-9 (semantic-auth), run against the FULL tree rather than the enablement PR's own 3-file diff, flags all three SetupController methods: `#[NoAdminRequired]` on a body that calls `requireAdmin()`. The class docblock defended the shape as defence in depth — "the body enforces an explicit IGroupManager::isAdmin gate (ADR-005 — do not rely on the SecurityMiddleware default alone)". That is inverted. `#[NoAdminRequired]` does not ADD a check, it REMOVES one: it tells NC's SecurityMiddleware to stop requiring admin. The body gate was therefore not a second layer, it was the ONLY layer, in front of endpoints that write app config (`registry_url`/`registry_token`) and seed OpenRegister objects. Swapped for `#[AuthorizedAdminSetting(AdminSettings::class)]`, so the middleware enforces admin before dispatch AND `requireAdmin()` stays in each body — the two layers the original comment was reaching for. Effective policy is unchanged for admins and strictly stricter for everyone else; the body gate still decides the response shape, so the 403 payload non-admins already saw is unchanged for anyone who reaches it. gate-5 (route-auth) stays green: `#[AuthorizedAdminSetting]` is an accepted auth posture, so the methods remain explicitly annotated. Hydra gates, full tree: 16 -> 15 failing; gate-9 3 -> PASS. * fix(quality): correct the AppHost settings stub, give the spec anchors a canonical home, cover the posture Three CI failures from the previous commit, all fixed at the cause. 1. phpstan: "#[AuthorizedAdminSetting] expects class-string<IDelegatedSettings>, string given" x3. Not a problem with the attribute — a WRONG STUB. The real OCA\OpenRegister\AppHost\Settings\GenericAdminSettings implements IDelegatedSettings; tests/stubs/openregister-apphost.stub.php declared it `implements ISettings`. AdminSettings extends it, so static analysis could only see a plain string for code that is correct at runtime. Corrected the stub to IDelegatedSettings and added the two members that interface adds (getName, getAuthorizedAppConfig), with a note to keep it in step with the real class. A stub that understates the real contract is a trap for the next person to reach for this attribute. 2. gate-46 spec-anchor-existence: 6 unresolved @SPEC targets. All six anchors in this file pointed at openspec/changes/openbuild-first-time-setup/tasks.md#task-21|31|41 — headings that do not exist (tasks.md has "## 2. Setup action endpoint" and friends). Anchors into openspec/changes/** dangle the moment the change is archived, so re-pointing at another change dir would only reset the timer. Promoted the change's spec delta to openspec/specs/first-time-setup/spec.md and pointed all six at real requirement headings there. Diff-scoped gate, so touching the file is what surfaced the debt; leaving it red was not an option. 3. gate-47 security-change-has-tests: a security-touching change with no test. Fair. Added SetupControllerAuthPostureTest: the attribute invariant (no #[NoAdminRequired], exactly one #[AuthorizedAdminSetting] on each of the three routed methods) plus the behavioural gate — a non-admin gets 403 and a session-less caller 401, with `seed()` and `updateSettings()` asserted NEVER to run on either path. It fails if either layer is removed. Suite: 749 tests green. phpstan OK. phpcs lib: 0 errors. phpmd: only pre-existing findings under lib/Resources/template (the verbatim snapshot). * revert(openspec): back the first-time-setup spec promotion out of this PR Promoting the change delta to openspec/specs/first-time-setup/ is the right fix for the six dangling @SPEC anchors in SetupController — but it brings SEVEN new Scenarios under gate-19 (e2e-coverage), and openbuild has no setup-wizard Playwright spec. The only ways to close that are to write and VERIFY seven e2e scenarios, or to add `@e2e exclude`. Shipping seven unverified e2e specs would be the green-but-dead pattern this whole exercise exists to find, and an exclude is a suppression. So it comes out of this PR and is filed as its own piece of work. gate-46 therefore still reports the six pre-existing anchors on the diff-scoped CI run — stated, not silenced. --------- Co-authored-by: Ruben van der Linde <juan.claude@conduction.nl>
…ack on (#142) * fix(e2e): install Docudesk in CI, repair 2 lying skips, turn traces back on The E2E job measured 168 passed / 5 failed / 76 skipped (run 31083894467). This addresses the instrument first, then the failures whose cause was a missing dependency or a guard that lied. TRACES WERE NEVER WRITTEN. tests/e2e/playwright.config.ts pairs `trace: 'on-first-retry'` with `retries: 0`. Playwright only records under that mode ON a retry, and a retry can never happen, so no trace file has ever been produced by this job — while the workflow's upload step ran, found nothing, and said so quietly under `if-no-files-found: ignore`. Every red run had a screenshot and a video but not the one artifact carrying the network log. `retain-on-failure` has no dependence on retries. The output directory is already globbed by the shared workflow (it uploads both `server/apps/<app>/test-results/` and `.../tests/e2e/test-results/`), so the traces now actually leave the runner. The root config gets the same change plus the `globalTimeout` the CI config already carries — it is the documented fallback if the CI config is ever absent, and a job cancelled at `timeout-minutes: 45` produces no tally and uploads nothing. DOCUDESK WAS NOT INSTALLED. Three REQ-DDT-002 specs drive Docudesk's REST surface (`api/templates`, `api/templates/{id}`, `.../preview`). With the app absent every call is a router 404 and the run said so: `[globalSetup] docudesk not installed`, then `Expected: 200 / Received: 404`. No code change here could have fixed that. global-setup.ts already configures Docudesk's template register and seeds the fixtures once it IS present, so adding it to `additional-apps` is the whole fix. `ref: main` — Docudesk publishes `main` and `beta` only, no `development`. TWO WARNINGS RENDERED AT ONCE. `.ob-document-attach__warn` resolved to two elements — "Docudesk is not installed" and "the attached template no longer exists in Docudesk" — which are mutually exclusive claims; the second is not knowable when the first is true, because a 404 from an absent app is the router reporting a missing ROUTE, not Docudesk reporting a missing TEMPLATE. They were two independent `v-if`s and both fired whenever the capability flag arrived late, which is the normal case (PageDesignerHost initialises it `true` and resolves it asynchronously). Now `v-else-if`, with a unit test that fails on the old markup: `expected [...] to have a length of 1 but got 2`. A SKIP THAT COULD HAVE SEEDED ITSELF. builder-undo-redo REQ-BUR-004 probed for an ApplicationVersion named "staging", found none, and skipped with "seed one to exercise this scenario". Nothing was ever going to. The suite's own `ensureVersionChain()` provisions development -> staging -> production on demand and is proven in this same job by versionRouting.spec.ts. Given a DEDICATED slug, not the shared one: the neighbouring tests open `/pages` with no `?_version=`, so growing a chain on `pw-undo-redo` would change what they test on every run after the first. A FIXTURE THAT ONLY EXISTED ON ONE MACHINE. page-editor REQ-PEC-006 bound `openbuild-hello-world-production` / `hello-world-production-hello-message`. Those are wizard-minted names; the CI fixture is built by `occ openbuild:seed-hello-world-fixture`, which deliberately does not mint a per-version register and puts everything in the shared `openbuild` register — ci-seed.sh prints the register list and it is not there. Bound to the pair the fixture actually provides, so CI and a dev box now agree. FOUR STALE QUARANTINES LIFTED. bootstrap-openbuild's index-page test, page-designer-ui REQ-OBPDUI-001/002 and application-detail-overview REQ-OBADO-006 all carry the openbuild#41 reason that builder-host.spec.ts's own note says "no longer holds" — and the routes they claim are broken are driven successfully by passing specs in the same run. Two were strengthened while being lifted: REQ-OBPDUI-002 asserted "the page body does not contain the word 404", which would fail on the legitimate version-not-found copy the next test requires and pass on a blank designer; REQ-OBADO-006 used a page-wide `locator('a, button').filter(...).first()` that would keep passing with the Register widget deleted, and read an `href` the widget does not have (it navigates via `window.location.href`), guarded by an `if (href)` that asserted nothing. * fix(e2e): the automations-RBAC guard was reporting a failed lookup as a defect Both tests in automations-rbac.spec.ts skipped with "the openbuild `automation` schema slug collides with a pre-existing schema of the same slug on this shared instance — automation CREATE/SAVE 400s regardless of app/version". Run 31083894467 disproves that within a single run. Seven tests in automations.spec.ts sit behind a copy of the SAME helper, against the SAME instance, and passed — composing and saving real automations end to end (REQ-AUTD-002 x3, -003, -005, -006, -007). Only this file's two skipped. The discriminator is not the instance, it is the auth context. This describe declares `test.use({ storageState: { cookies: [], origins: [] } })` so each test can log in as a non-admin. That also makes the `request` fixture anonymous, so the probe's read of `api/schemas/automation` was refused — and `if (resp.ok() === false) return false` turned "I could not look" into "the feature is broken", with a confident and wrong explanation attached. The probe now authenticates with the admin credentials the config already uses for `httpCredentials`, independently of the test's storageState. And a non-OK response THROWS instead of returning false: a probe that cannot probe now fails the run loudly rather than silently becoming a verdict. The skip reasons are rewritten to describe what the check actually establishes. These two tests may now fail for real reasons. That is the point — a real failure is information and a false skip is not. --------- Co-authored-by: Ruben van der Linde <juan.claude@conduction.nl>
Contributor
|
Triage 2026-08-08 (openbuild dedupe sweep) — left open with a precise blocker. State: Failing checks, by name (count is not the measure — the names are):
Everything else on this PR is SUCCESS, including all four
Blockers before this can move:
|
… layer, not added one (#147) `ApplicationCreationController::wizard()` is admin-only — it provisions an OpenRegister Register, mirroring OR's admin-only RegistersController gate (issue #157, OR #1949) — and enforces that with an `IGroupManager::isAdmin()` check in the body. It also carried `#[NoAdminRequired]`, which does not ADD a layer: it tells NC's SecurityMiddleware that any logged-in user may reach the method, leaving the body check as the only thing between a regular user and register provisioning. Hydra gate-9 (semantic-auth) reports exactly that contradiction. It is the same defect SetupController carried until #127, and it gets the same correction: `#[AuthorizedAdminSetting(AdminSettings::class)]`, so the middleware refuses before dispatch and the in-body gate becomes defence in depth rather than the whole defence. Measured, full-tree (hydra-gates @main over the entire tree, not diff-scoped): `development` fails 15 gates; with this change, 14. gate-5 (route-auth) stays PASS — `#[AuthorizedAdminSetting]` is one of the four attributes it accepts, so this does not trade a gate-9 finding for a gate-5 one. The unit test that asserted the OLD posture (`wizard() must carry #[NoAdminRequired]`) asserted the defect, so it is replaced rather than kept. Can-fail proof: re-adding `#[NoAdminRequired]` to the method turns `wizardDeclaresTheAdminPostureAtTheMiddlewareLayer` RED with "wizard() must not carry #[NoAdminRequired]... Failed asserting that actual size 1 matches expected size 0" — the assertion fails for the reason it exists. `openspec/changes/openbuild-first-time-setup/tasks.md` claims `AuthorizedAdminSetting` "fails PHPStan L5 in this repo's stub set". Measured on this change: phpcs 0 errors, phpstan `[OK] No errors`. That note is stale. Co-authored-by: Ruben van der Linde <juan.claude@conduction.nl>
…sured two different numbers because of it (#149) `.gitignore` carried a scaffold block headed "Files with unusual extensions or no extensions that could be mistakes" — patterns aimed at stray notes-to-self like `PR notes` or `endpoints list`. They are substring globs, so `**/*references*` also matches: lib/Controller/PreferencesController.php tests/Unit/Controller/PreferencesControllerTest.php Both are tracked, so git keeps versioning them and nothing complains day to day. `git check-ignore` does not even report them without `--no-index`, because it skips tracked paths by default — which is why this survived since the initial scaffold commit (9bfa57e). It is not harmless. Anything that re-adds from scratch drops them, and every tool that enumerates the repository through `git ls-files` cannot see them. The hydra-gates runner enumerates controllers exactly that way (`_enum_tracked` → `git ls-files`). Measured 2026-08-08 on this commit: working tree gate-14: FAIL — 7 unrouted method(s) `git archive` + `git add -A` gate-14: FAIL — 5 unrouted method(s) Same commit, same file bytes (`diff` reports IDENTICAL), two verdicts — the second copy simply had no PreferencesController.php in it (20 controllers tracked instead of 21). Two numbers for one measurement means one came through a lossy channel. The fix is a negation for the source directories, not an edit to each pattern: the block is inherited scaffold and the next `PreferencesService.php` would be swallowed again. Verified the negation adds nothing unwanted — the count of ignored-but-untracked files under lib/src/appinfo/tests was 0 before and is 0 after, and `git status --porcelain` reports no newly visible untracked file, so nothing junk starts being tracked. Scope is source only. The other 304 tracked-and-ignored paths are all under `docs/build/` and `docs/.docusaurus/` — committed build output, a separate decision, deliberately left alone. `scripts/check-gitignore.js` makes this a standing check rather than a one-time repair, wired into the CI `frontend-checks` matrix as `check:gitignore` so it runs as its own named job. Can-fail proofs, both run: - restore `origin/development`'s .gitignore → exit 1, naming both files. - point the script at a non-existent directory (positive control on the INPUT) → exit 2, "Enumerated ZERO tracked files ... That is a broken enumeration, not an empty repository." Without that control, a scanner that reads nothing is indistinguishable from one that finds nothing. - restored → exit 0, "743 tracked file(s) ... none matched by .gitignore." Archive round-trip after the fix: 21 controllers, PreferencesController.php present. Co-authored-by: Ruben van der Linde <juan.claude@conduction.nl>
…ects
Two tools in the same pipeline gave OPPOSITE instructions about where an
`@spec` tag should point, and following the one that runs FIRST manufactured
findings for the one that runs SECOND.
`SpecTagSniff` runs as a blocking `PHP Quality (phpcs)` job and told every
developer, in its file docblock and in its own warning text:
@SPEC openspec/changes/{change-name}/tasks.md#task-N
A change directory is temporary by definition — completing a change moves it
to `openspec/changes/archive/<date>-<name>/`, and renaming or dropping one
removes the target outright. Every tag written to that instruction dangles
from that moment on, and gate-46 (spec-anchor-existence) reports it. The
developer who wrote the tag had followed this sniff's own advice.
Measured on portaliq: 100 unresolved gate-46 targets, and 260 of its 385 live
tags pointing into a change directory. The sniff ships identically in 20
ConductionNL repos, so grinding the tags without fixing the sniff regenerates
them at the rate changes are archived.
This changes the docblock example and BOTH warning messages to the canonical
form gate-46 and the project rule agree on:
@SPEC openspec/specs/{capability}/spec.md#requirement-{slug}
The method-level message previously carried no guidance at all, so a developer
reading it had only the class message to copy from; it now names the same
canonical shape.
Behaviour is unchanged: severity stays WARNING (verified via phpcs — an
untagged class and public method still report 0 errors / 2 warnings, and a
tagged file still reports nothing), and an `openspec/changes/...` target is
still accepted, since this sniff only checks that a tag is PRESENT.
No `@spec` tags are repointed here — this repo's existing tags are untouched.
Refs ConductionNL/.github#228
…l-target fix(phpcs): stop the SpecTagSniff instructing the pattern gate-46 rejects
…UT /api/settings (#155) `\OCA\OpenRegister\AppHost\Routes::standard()` ships the canonical route `settings#update` (PUT /api/settings), and `appinfo/routes.php` calls it, so the route is live. `AppHost\Bootstrap::aliasControllerUnlessLeafDefinesIt()` only substitutes OpenRegister's `GenericSettingsController` when the leaf does NOT ship a class of that name — OpenBuild DOES, so the alias is skipped and OpenBuild owes every method the canonical table routes to `settings#`. It had `index/create/load` but no `update()`. Measured live on the dev instance 2026-08-08: GET /apps/openbuild/api/settings -> 200 (positive control) PUT /apps/openbuild/api/settings -> 500 ReflectionException: Method OCA\OpenBuild\Controller\SettingsController::update() does not exist at lib/private/AppFramework/Utility/ControllerMethodReflector.php:40 The router matches the URL and the dispatcher reflects the method, so a missing canonical method is a 500, not a 404. `create()`'s body — the H6 admin guard included — moves into `update()`, and `create()` becomes `return $this->update();` while keeping its own attributes. Both verbs now share one enforcement path and cannot drift apart. Auth posture is unchanged and deliberately NOT converted to `#[AuthorizedAdminSetting]`: `update()` keeps `#[NoAdminRequired]` plus the in-body 401/403 guard, matching `index()`/`load()`. Nextcloud's SecurityMiddleware only evaluates attributes on the DISPATCHED method, so `#[NoAdminRequired]` alone would leave this instance-wide write open to any authenticated user — the in-body check is the guard. Tests (tests/Unit/AppInfo/CanonicalRouteMethodContractTest.php, tests/Unit/Controller/SettingsControllerWriteTest.php): - each canonical method is asserted individually as existing, public and non-static (the ITEM, not the container), with an `$inspected > 0` positive control so a green cannot be produced vacuously; - a second control asserts routes.php still delegates to Routes::standard() and cross-checks the transcribed canonical table against OpenRegister's own source when resolvable; - update() writes through SettingsService and returns the stored config; create() delegates and still writes; - the guard is asserted on update() itself: non-admin -> 403, unauthenticated -> 401, and updateSettings() is never called in either case. Also closes the one pre-existing uncovered statement in this controller (the unauthenticated branch of `load()`), taking SettingsController to 100% line and method coverage. Co-authored-by: Ruben van der Linde <juan.claude@conduction.nl>
…s 32, 43 and 45 to green (#150) * fix(a11y): keyboard parity, table semantics and reduced-motion — gates 32, 43 and 45 to green Three mechanical accessibility gates, 13 findings, all read and confirmed real before touching anything. gate-32 semantic-controls (2) — WCAG 2.2 AA 2.1.1 Keyboard PageListEditor's page row was selectable by CLICK ONLY. Every field inside it carries `@click.stop`, so a keyboard user tabbing into a row's inputs was editing a page that had never been selected. `@focusin` is the substantive repair: focus reaching any descendant now selects the row, the same outcome the mouse always had. The row gets `role="group"` + `aria-label`, NOT `role="button"`. A button's children are presentational, so `role="button"` on a container full of inputs would hide those inputs from assistive technology — it satisfies the gate and makes the page less accessible. `tabindex="-1"` makes the row programmatically focusable without inserting a second tab stop ahead of its own fields. WalkthroughDesigner's tour <li> has no nested interactive content, so there `role="button"` + Enter/Space handlers IS right, plus `aria-current` so the open tour is announced rather than only coloured. gate-43 table-headers (8) — WCAG 2.2 AA 1.3.1 Info and Relationships Seven tables carried `<th>` with no `scope`. The eighth, ImportDataWizard's CSV sample, had no `<th>` at all: `sampleRows` is populated only by readCsvSample(), and a CSV's first line IS its header — it was rendering as ordinary `<td>` data. Promoting row 0 to a `<thead>` of `<th scope="col">` is both the a11y fix and a truer rendering of the file. Three action columns had a bare `<th />`. They are still column headers, so they keep `scope="col"` and gain an sr-only name rather than being downgraded to `<td>` — the downgrade would have satisfied the gate by removing the header. gate-45 prefers-reduced-motion (3) — WCAG 2.2 AA 2.3.3 Three `transition:` declarations with no reduced-motion fallback. Each fix is scoped to the component's own selector, never a blanket `* { transition: none }`, so it cannot reach into NcButton/NcSelect internals. Measured, full-tree (hydra-gates @main over the entire tree, not diff-scoped, with ajv provisioned into the gates package exactly as quality.yml does): `development` fails 15 gates; with this change, 12. Gates 32, 43 and 45 are green; no other gate's verdict changed. Frontend suite: 140 files / 1366 tests before, 140 / 1369 after — all passing. eslint: 0 errors, 0 warnings (the added `scope="col"` made several `<th>` multi-attribute, which the `vue/singleline-html-element-content-newline` rule reformats; run through `lint-fix` so the branch introduces no new warnings — `development` has zero). Can-fail proofs, all four run: - remove `@focusin` from the row -> "focusing a field inside a row emits select(index)" FAILS (1 failed | 18 passed). - `role="group"` -> `role="button"` -> "expected 'button' to be 'group'" (1 failed | 18 passed). NB the first attempt at this mutation edited the word inside the explanatory COMMENT and the suite stayed green — a reminder that a string match is not a code match, in both directions. - drop `scope="col"` from the sample header -> "expected undefined to be 'col'" (1 failed | 6 passed). - `sampleBody` slice(1) -> slice(0), i.e. the header duplicated as data -> "expected ... to have a length of 2 but got 3" (1 failed | 6 passed). The new tests assert on the CELLS and the EMITTED EVENT, never on the `<thead>` wrapper or the presence of an attribute: an empty `<thead>` would satisfy a container assertion while announcing nothing, and a `tabindex` assertion would pass over a handler that does nothing. NOT fixed here, deliberately: gate-31 img-alt's 3 findings are false positives. All three are the literal string `<img>` inside JSDoc `@param` comments in `<script>` blocks; every real `<img>` in those files already carries `:alt`, which the gate's own regex accepts. Reproduced with the gate's own matcher. Reported upstream rather than "fixed" by rewording documentation. * fix(l10n): register the new "Page {position}" source string `Frontend Check (test:l10n)` went RED on this branch and nowhere else: l10n-check: FAIL — 1 translation key(s) used in source but MISSING from l10n/en.json: • "Page {position}" src/components/page-editor/PageListEditor.vue:66 The row's `aria-label` introduced a new translatable literal. `test:l10n` is exactly the check that is supposed to notice, and it did — this is the gate working, not noise. Registered in en.json (source === English) and translated in nl.json rather than left English-only, since the aria-label is what a Dutch screen-reader user hears. --------- Co-authored-by: Ruben van der Linde <juan.claude@conduction.nl>
…es (#157) gate-46 spec-anchor-existence measured 102 unresolved findings across 41 targets. They are not 102 bad tags; they are three defects. 1. TAGS POINTING AT AN ARCHIVED CHANGE DIR (52 findings). Seven targets named `openspec/changes/<change>/specs/<cap>/spec.md` for changes that have since been archived. The gate's resolver finds the ARCHIVED copy under `openspec/changes/archive/<date>-<change>/` and judges the anchor against that frozen snapshot — never against the canonical spec. Retargeted to `openspec/specs/<capability>/spec.md`, which is where the project rule (and openbuild#152's SpecTagSniff) says a @SPEC tag points. 2. REQUIREMENT IDS LIVED IN THE BODY, NOT THE HEADING (50 findings). The specs carried `**ID:** REQ-XXX-NNN` as a body line. ADR-037 rule 2 is explicit that the ID belongs in the heading as a parenthesised suffix, and explicit about why the body line is wrong. An anchor resolves against HEADINGS, so `#req-autd-001` named nothing. Moved the IDs into the headings for every requirement in the six specs involved — not only the ones a tag happens to reference, so the files are uniformly ADR-037 Form A. 3. `2.1` WRITTEN AS `21` (15 findings). `#task-21` is read by the gate as "the 21st checkbox". The tasks files have 17. The tags meant task **2.1** — and each one lines up exactly with its task (SetupController against 2.1 "Create SetupController.php"). The anchor spelling that names it is `#task-2.1`. Measured with the helper directly, both directions: `#task-21` FAILS and `#task-2.1` RESOLVES against the same file, so the fix is not the check being loosened. Full suite at gate package 48c88ba: gate-46 102 -> 0. Note: gate-61 (listener-work-placement) reports 6 findings on this branch that development does not show. They are PRE-EXISTING debt in AutomationApprovalTriggerListener and DocumentGenerationListener — gate-61 is diff-scoped against origin/development inside its own helper, and these @SPEC retags are what pulled the two files into its scope. No behaviour in either listener changed here. Co-authored-by: Ruben van der Linde <juan.claude@conduction.nl>
…rsionUuid (#158) gate-51 schema-property-titles: 11 -> 0. gate-54 relation-dialect: 2 -> 1 (the remaining one is not honestly fixable here — see below). gate-51. Eleven properties in the Automation schema shipped with no title and, in nine cases, no description at all: the notification recipient `kind`/`permission` pair, the `nl`/`en` subject templates, and every field of the `provenance` bookkeeping arrays. These are the labels OpenRegister renders in its own object editor, so an unnamed property shows as a raw JSON key to whoever has to fill it in. The two `type` enums (trigger, condition) had a title but no description, which is the same problem one step smaller: the enum values are the whole contract and nothing said what they mean. gate-54. `Automation.versionUuid` is `format: uuid` with a description that says it points at an ApplicationVersion, but carried no `$ref` — so OpenRegister had no way to resolve or render the relation. Added `"$ref": "ApplicationVersion"`, the canonical ADR-062 rule-7 dialect, which resolves against the schema key in `lib/Settings/openbuild_register.json`. NOT FIXED, deliberately: `Automation.actions.items.templateId`. It is `format: uuid` and its description does say "UUID of the Docudesk template", so gate-54's relation-shape heuristic matches. But the object it names is a DOCUDESK template, fetched over Docudesk's own REST API — it is not an object in OpenBuild's register, and ADR-062 rule 7 requires a `$ref` that resolves to a schema key IN THE SAME REGISTER FILE SET (check f). Any `$ref` I could write here would be a dangling one, trading a real finding for a worse one. The alternative — rewording the description until the regex stops matching — would leave the schema unchanged and only move the words, which is exactly the comment-satisfaction this programme forbids. Rule 7 has no vocabulary for a cross-app foreign key; that is the gap, and it belongs upstream rather than in this file. Co-authored-by: Ruben van der Linde <juan.claude@conduction.nl>
…y installs on (#148) * chore: raise the Nextcloud floor to 32, and pin it to what CI actually installs on Product decision: the fleet standardises on Nextcloud 32 so it can require PHP 8.3, which `<php min-version="8.3"/>` already declares. opencatalogi, openconnector and docudesk are already at 32; this brings OpenBuild in line. max-version stays 34 — the fleet value everywhere except openconnector, which declares 35 (measured 2026-08-08 across 8 repos' `development`). This is NOT the openconnector#1172 dependency-floor rule firing, and the comment that stood here said it was. That rule — min-version >= the max of every `<app>` dependency's floor — is satisfied either way: measured on 2026-08-08, `ConductionNL/openregister` declares `min-version="28"` on BOTH `development` and `main`, having reverted its own 32 bump in #2380 after #2372 removed every eager reference to its ContextChat provider. 32 >= 28 today, and still >= 32 if openregister moves back up. The previous comment also contradicted the file it was in: it asserted "min-version is 32, NOT 28" directly above `min-version="28"`. #146 lowered the value and left the prose. Prose is what a reader trusts, so this replaces it rather than editing around it. Safety of raising the floor: min-version is enforced at INSTALL time, so a floor above what CI installs on makes `occ app:enable openbuild` refuse, and the failure then surfaces ~70s later as missing tables — which reads like a migration fault and sends you to the wrong file. `code-quality.yml` already runs `nextcloud-test-refs: ["stable32", "stable33"]` (stable31 was removed), and it is the only place in this repo that names a Nextcloud version, so no leg is left behind. `NextcloudFloorTest` makes that a standing invariant rather than a one-time check: every `nextcloud-test-refs` entry must be >= the declared floor, and the floor itself is pinned to 32 so a silent lowering has to argue with a test. Can-fail proofs, all three run: - floor 32 -> 34: "CI installs OpenBuild on stable32 (Nextcloud 32) but appinfo/info.xml declares min-version=34" (2 failures). - CI refs + stable31: "CI installs OpenBuild on stable31 (Nextcloud 31) but appinfo/info.xml declares min-version=32" (1 failure). - CI refs -> '[]' (parser positive control): "Parsed ZERO nextcloud-test-refs ... That is a broken parser, not a clean result" (1 failure). Without this the whole assertion is vacuously true when the parser breaks. * fix(ci): move the floor invariant out of PHPUnit — it runs against a DEPLOYED app, not the repo The previous commit added `tests/Unit/AppInfo/NextcloudFloorTest.php`. It went RED on all four PHPUnit legs: RuntimeException: Could not parse /home/runner/work/openbuild/openbuild/server/apps/openbuild/appinfo/info.xml That path is the tell. This fleet's PHPUnit job copies the app to `server/apps/openbuild` and runs the suite from THERE — against a deployed copy, not the repository. `.github/workflows/code-quality.yml` is not part of a deployed app, so half the invariant had no input, and the deployed `appinfo/` did not parse the way it does in a checkout either. A test asserting on REPOSITORY LAYOUT is in the wrong harness, and its red said nothing about the floor. `scripts/check-nc-floor.js` is the same invariant in the harness that can answer it: the `frontend-checks` matrix runs in the checkout. It asserts the same two things and one more: 1. every `nextcloud-test-refs` entry is >= `<nextcloud min-version>`; 2. the floor is pinned at 32 and PHP at 8.3, so a silent LOWERING also fails (the comparison alone would happily accept 28 — which is how #146's regression would have slipped through again); 3. a positive control on the input — a parser that reads zero refs exits 2 rather than passing vacuously. It also reads the assignment LINE, never the surrounding comment: the comment above that input names `stable31` while explaining it was REMOVED, so a comment-blind grep reads a leg that does not exist. Can-fail proofs, all four run: - floor 32 -> 34: two "CI installs OpenBuild on stableN ... declares min-version=34" lines plus the pin, exit 1. - floor 32 -> 28: "Nextcloud floor is 28, expected 32", exit 1. - CI refs + stable31: "CI installs OpenBuild on stable31 (Nextcloud 31) ...", exit 1. - CI refs -> '[]': "Parsed ZERO nextcloud-test-refs ... That is a broken parser, not a clean result", exit 2. - restored: exit 0, "floor NC 32 / PHP 8.3; CI legs stable32, stable33 all satisfy it." NOTE for whoever merges second: this touches the same `frontend-checks` line as the `check:gitignore` branch (#149). The two lists have to be unioned by hand; they are otherwise independent. * docs(info.xml): re-measure the openregister floor — it is 32 again (#2384) The comment shipped in this PR asserted openregister declares min-version=28, citing openregister#2380. That was true when written and expired 25 minutes before this PR's head commit: openregister#2384 merged 2026-08-08T10:45Z and put the floor back to 32 fleet-wide. The consequence is not cosmetic. Under #2380's value the 32 here was a free product choice (32 >= 28). Under #2384 it is FORCED: openregister is a hard <app> dependency, so by the openconnector#1172/#1173 rule this app's floor may not sit below it. Also avoids writing any XML element syntax inside the comment: check:nc-floor asserts the COUNT of nextcloud elements over raw file text, so a quoted example in a comment reads as a second, contradictory declaration. --------- Co-authored-by: Ruben van der Linde <juan.claude@conduction.nl>
…rew first (#159) * fix(controllers): seven 404 branches were unreachable — the lookup threw first gate-49 controller-exception-translation: 7 -> 0. Measured full-repo at gate package 48c88ba. This is not a docblock change. Every one of the seven methods declares "null when missing" and every caller has an `if (... === null)` branch that returns a 404 — and none of those branches could ever be taken, because the lookup throws instead of returning null: * `ObjectService::find()` documents `@throws Exception If the object is not found`. ApplicationPublishController::loadApplication, VersionPromotionController::loadApplication and ::loadVersion all treated it as null-returning. A missing Application or ApplicationVersion escaped as an exception and came back as a 500 `internal_error` from the outer `catch (Throwable)` — the 404 was dead code. * `RegisterMapper::find()` / `SchemaMapper::find()` throw DoesNotExistException when the register or schema is absent. ApplicationVersionsController::findVersionRowBySlug, ::loadApplication and ::findVersionForApplication, and ApplicationsController:: resolveApplicationBySlug, all called them unguarded. Three of those call sites (ApplicationVersionsController::show, ::release, ApplicationsController's manifest path) sit OUTSIDE any try/catch, so an unprovisioned OpenRegister answered with a framework 500 and a stack trace — on #[NoAdminRequired] endpoints. Each lookup is now wrapped and translated. The object-level misses return null, which is what the signature always claimed and what makes the callers' 404 branches reachable. The register/schema misses are logged at warning (they mean OpenRegister is not provisioned, which is not a 404 about one object) and resolveApplicationBySlug returns a translated 500 envelope instead of an unhandled throw. Causes are logged in every arm, so nothing is flattened into a silent null. * refactor(versions): one register/schema resolver instead of four copies Follow-up to the gate-49 fix in this branch, addressing two findings that change CAUSED and that development does not have: * phpmd ExcessiveClassLength — ApplicationVersionsController reached 1006 lines against a 1000 threshold. The cause was my own fix: the same try/catch around RegisterMapper::find() + SchemaMapper::find() written out three times. * the coverage ratchet — those three copies were three untested arms. Both are answered by the same change rather than by shaving the metric: `resolveRegisterAndSchema()` is now the single place the mapper's DoesNotExistException is translated, and the four call sites (index(), findVersionRowBySlug(), loadApplication(), findVersionForApplication()) go through it. index() was already inside a try/catch so gate-49 never flagged it; it is included because leaving one copy behind is how these drift. Measured: class length 1006 -> 994 (threshold 1000, development 970), longest line 144 (limit 150), gate-49 still PASS, php -l clean. * fix(versions): restore named arguments, and cover the two translated arms Two follow-ups to the previous commit, both caused by it. 1. phpcs: "All arguments in calls to internal code must use named arguments" — 4 errors. I had collapsed the resolveRegisterAndSchema() call sites onto one line to win back class-length budget and dropped `schemaSlug:` doing it. That was shaving a metric at the cost of the repo's own convention. The named form fits on one line anyway (longest line 144, limit 150), so both constraints hold: class length 995 (threshold 1000, development 970) and named arguments at all four sites. 2. The coverage ratchet: the new catch arm was untested, so the ratchet correctly reported a drop. Two regression tests added, one per lookup direction: * release() with a RegisterMapper that throws DoesNotExistException * show() with a SchemaMapper that throws DoesNotExistException Both call their lookup helper OUTSIDE any try/catch, so against the pre-fix code the exception escapes the controller and the test RAISES. They fail before and pass after — a regression test, not a restatement of current behaviour. --------- Co-authored-by: Ruben van der Linde <juan.claude@conduction.nl>
… not a label (#160) gate-40 form-label-association: 54 -> 0, measured full-repo at gate package 48c88ba. ADJUDICATED PER ITEM FIRST. gate-40 carries a known fleet-wide false-positive rate (~58%, .github#273), so every finding was checked against the file rather than assumed: does a real `<label for>` already point at this control, or does it already carry an aria-label? For openbuild the answer was NO in all 54 — zero false positives here. The FP class elsewhere is inputs that ARE labelled and get flagged anyway; openbuild's are bare `<input>`s whose only labelling is a `placeholder`, which is not an accessible name (WCAG 2.2 AA SC 1.3.1 / 3.3.2 — a placeholder disappears on first keystroke and is not reliably announced). Two shapes of fix: * Where a visible label already exists for sibling fields, add a real one. Step1Basics had `<label for>` for name and description but the slug input under "Advanced" had none — it now has the same visible label its neighbours have, not an ARIA patch. * Where the control is a narrow cell in a repeated builder row and a visible label would break the layout, `aria-label` carrying THE SAME string the placeholder shows. Screen-reader users get exactly the name sighted users already get. Reusing the existing `t('openbuild', …)` string means this introduces no translation churn: 52 of the 54 needed no new source string at all. The two exceptions: DecisionTableEditor's condition cells take the column name (`col.naam`) so each cell announces its own column; its decision cell reuses the `<th>`'s own "Decision" string. One genuinely new source string, "Raw page configuration (JSON)", for StubPageEditor's textarea — it had no placeholder, no legend and no heading of its own to borrow. Registered in l10n/en.json AND l10n/nl.json. Verified: `node tests/l10n/check-l10n.js` OK; eslint clean on all 24 changed files; full gate suite shows gate-40 gone and NO other gate gained a finding. Co-authored-by: Ruben van der Linde <juan.claude@conduction.nl>
…eady ignores (#161) gate-29 gitignore-then-commit: 7 -> 2 (the 2 left are an upstream gate bug, see below). Measured at gate package 651e5c5, --scope-to-diff --base <root>. `.gitignore:19` ignores `docs/build/`, and 263 files under it were tracked anyway. That combination is the defect the gate is named for: the directory is `docusaurus build` output, so the committed copy is a snapshot that no longer matches the sources beside it and that nothing updates. Nothing consumes it. `.github/workflows/documentation.yml` only fires on the `documentation` branch and delegates to the shared reusable workflow, whose build step begins `rm -rf node_modules/.cache .docusaurus build` and then publishes `<source-folder>/build` — it DELETES the committed output and regenerates it. So these 263 files have never been the published site; they are a stale copy that the ignore rule was already trying to stop. `git rm -r --cached` only: the files stay on disk and stay ignored. NOT FIXED — 2 remaining findings are a gate false positive: ignore_pattern=!tests/vitest/setup.js tracked_file=tests/vitest/setup.js ignore_pattern=!tests/e2e/global-setup.ts tracked_file=tests/e2e/global-setup.ts Both patterns begin with `!`. A `!` line UN-ignores; these two exist precisely to rescue real source from the broader `**/setup*` rule three lines above, and they are the reason those files are still tracked. The gate reads them as "newly-ignored path(s)", which inverts their meaning. Verified by reading OUTPUT, not an exit code (`git check-ignore -q` returns 0 when a NEGATION matches, which is the trap that makes this look real): $ git check-ignore -v tests/vitest/setup.js -> no output (NOT ignored) $ git check-ignore -v tests/e2e/global-setup.ts -> no output (NOT ignored) $ git status --ignored --porcelain <both> -> neither is in the ignored set Deleting the negations to satisfy the gate would ignore two real test files — the exact outcome the gate exists to prevent. Left red and filed upstream. Co-authored-by: Ruben van der Linde <juan.claude@conduction.nl>
…165) gate-54 relation-dialect: 1 -> 0. Measured at gate package 365fa31. `Automation.actions.items.templateId` is `format: uuid` with a description that says it points at a Docudesk template, so gate-54's relation-shape rule matched it. It was the last gate-54 finding in this repo and, until now, it had no honest fix: the object it names lives in DOCUDESK's register, and check (f) requires a `$ref` that resolves to a schema key in the SAME register file set. Both arms failed — WITH "$ref": <anything> -> "does not resolve to a schema key in the register set (case-exact)" WITHOUT $ref -> "lacks canonical $ref (ADR-062 rule 7)" — so the only route to green was rewording the description until the regex stopped matching, which degrades documentation to dodge a check. I refused that and reported it upstream instead (ConductionNL/.github#307). .github#286 added the missing vocabulary: `x-external-register: <app>`, for a relation whose target lives in another app's register. That is exactly this property's shape, so it is now declared rather than hidden — and the description says so in words as well. Verified against package 365fa31, all four arms: A with x-external-register -> 0 findings B with it REMOVED (negative control) -> 1 "lacks canonical $ref" C with it AND a $ref (abuse control) -> 1 "carries x-external-register 'docudesk' AND $ref … drop the $ref" D restored -> 0 findings Arm B proves the gate still fires here, so this is not a blind pass. Arm C proves the exemption is narrow: it suppresses only the two $ref rules and cannot be used as a blanket waiver. Full root-scoped suite unchanged otherwise: gate-54 gone, no other gate moved. Co-authored-by: Ruben van der Linde <juan.claude@conduction.nl>
…logs (#163) * fix(a11y): replace 7 native window.confirm/prompt calls with real dialogs gate-34 window-confirm: 7 -> 0. Measured full-repo at gate package 651e5c5. Native window.confirm/prompt blocks the JS thread, cannot be translated or themed, and inside Nextcloud's modal stack renders outside the dialog the user is already in. window.prompt is worse still: its input has no programmatic label at all. All seven call sites had the same shape — a synchronous boolean guard in front of a destructive continuation — so they share two new components rather than each growing an inline NcDialog, which ADR-004 / gate-13 forbids: src/dialogs/ConfirmActionDialog.vue (6 sites) src/dialogs/PromptTextDialog.vue (1 site, the window.prompt) Sites: AutomationsPage (delete automation), ThemeSection (remove theme), SchedulesSection (remove task), WorkflowAttachmentsSection (detach case type), DocumentAttachmentsSection (detach template), DataSourceOriginToggle (discard connector mapping), ConnectorFieldMapper (name a display field). THE FAIL-SAFE DIRECTION IS PRESERVED, AND NOW TESTED. The dialogs never run the action; they only emit. The parent holds the target in `pending*` and acts only in an `onConfirm*` handler, so a closed, cancelled or dismissed dialog leaves state untouched. Every one of the six sites gained a test asserting the destructive step has NOT run after the ask — the property window.confirm gave us for free and the one worth pinning. Two behaviours improved rather than merely ported: * AutomationsPage keeps the dialog open with disabled buttons while the DELETE is in flight (`busy`) — a synchronous confirm could not express it. * PromptTextDialog disables submit while the value is blank, so the empty -name guard is enforced at the source instead of by `if (!name) return`. Verification, all re-run on this branch: * gate-34 PASS. Proven not blind: planting one window.confirm back into ThemeSection flips it PASS -> FAIL(1) -> PASS on removal. * gate-12 nc-input-labels PASS, gate-13 modal-isolation PASS, gate-16 spec-coverage PASS — the new dialogs did not push a finding elsewhere. * vitest 140 files / 1375 tests pass (was 1366; +9 net). * eslint clean on all changed and new files. * check-l10n OK; 6 new source strings registered in en.json AND nl.json. * fix(spec): tag PromptTextDialog's two methods, which gate-16 caught in CI gate-16 spec-coverage: 2 -> 0 on this branch's diff. CI caught what my local run could not: gate-16 is DIFF-SCOPED, so a full-repo run reports nothing for it and my "no other gate moved" check was blind to it by construction. The two findings are both in the new file this PR adds: src/dialogs/PromptTextDialog.vue::onSubmit — missing @SPEC src/dialogs/PromptTextDialog.vue::open — missing @SPEC (`open` is the WATCHER on the `open` prop, which is a method in the options object — worth knowing, because it reads like a prop in the finding.) Both now carry `@spec openspec/specs/openconnector-api-sources/spec.md #req-ocas-003` — "Schema mapping of the external payload to display fields", which is exactly what this dialog serves: it collects the display-field name for a mapped leaf. Deliberately the CANONICAL spec, not the tasks file the calling component uses. `openspec/changes/openconnector-api-sources/tasks.md#task-3.1` does resolve, but only through the ARCHIVE index — it is an archived change dir, which is precisely the shape #157 cleaned 52 tags out of. Adding a new tag of that shape would re-introduce it. Verified the canonical anchor resolves directly: has_anchor('openspec/specs/openconnector-api-sources/spec.md', 'req-ocas-003') -> True. Re-measured --scope-to-diff --base origin/development at gate package 365fa31: NO gate fails on this diff (gate-16 PASS, gate-34 PASS, gate-46 PASS). eslint clean; vitest 140 files / 1375 tests pass. --------- Co-authored-by: Ruben van der Linde <juan.claude@conduction.nl>
…167) gate-25 contract-coverage: 10 -> 0 (PASS). Measured at gate package 365fa31. Ten `#[NoAdminRequired]` / `#[PublicPage]` endpoints were registered in appinfo/routes.php with no Newman collection and no PHPUnit test touching them. gate-25 matches on an actual `->method(` call in a *Test.php, so these could not be satisfied by annotation — only by really calling them. AppOverrideController saveUser, getUser, clearUser (8 tests) AutomationsController disable (2 tests) RulesController testAll (4 tests) ShopController githubSearch, githubInstall (5 tests, new file) GitHubSyncController link, pull (7 tests, new file) ApplicationsController saveManifest (2 tests, new file) They pin authorisation and input validation rather than restating the happy path, because that is where a network-facing endpoint actually fails: * anonymous -> 401 AND the service is never called (expects(never())), so a guard that returns the right status while still doing the work would fail; * non-owner -> 403 with a REAL PermissionResolver, not a stubbed verdict, so the role logic is genuinely exercised — including a VIEWER being refused a write, the case a coarse "has any role" check would wrongly allow; * malformed owner/repo/ref -> 400 BEFORE any outbound GitHub call, which is the guard that keeps an attacker-supplied string out of the request URL. Proven not blind: replacing the `->githubSearch()` calls in ShopControllerTest with a dummy name flips gate-25 PASS -> FAIL(1), and restoring them returns it to PASS. Verified by running PHPUnit locally in a DISPOSABLE container against this worktree (nextcloud:34.0.0-apache, PHP 8.4) rather than the shared instance, whose bind mount points at the main checkout and would have tested that lib/ instead of this branch's: 797 tests, 2480 assertions, OK (was 769). One incidental finding while writing these: ShopController::githubSearch reads `$result['brokerUsed']` unguarded. Every documented return path of GitHubCatalogService::search() supplies it, so the fixture was at fault and was corrected — but note search() has a `return $cached` path, so a cache entry written before that key existed would surface as an undefined-key warning. Not changed here; recorded for whoever touches that cache. Co-authored-by: Ruben van der Linde <juan.claude@conduction.nl>
* chore(deps): pin @conduction/nextcloud-vue to 2.2.0-vue3.9 Hard pin to the vue3 dist-tag head. Not a range: ^2.2.0 does not match 2.2.0-vue3.9, and latest/beta are the retired Vue 2 lineage. Lockfile control ran first: regenerating with the pin UNCHANGED already produces +115/-1351 (npm 11 dedupe). Bump-attributable portion is +5/-5. installed off disk after a real npm ci: 2.2.0-vue3.9, one copy, peer vue ^3.5.0 unit 1375 passed / 140 files -> 1375 passed / 140 files test:l10n exit 0; check:manifest exit 0 build 3 warnings -> 3 warnings bundle 31,715,872 -> 31,733,865 bytes * fix(deps): regenerate the lockfile with npm 10, the version CI actually runs The earlier lockfile was generated with npm 11.13.0, which prunes optional+peer entries npm 10 still requires. CI runs node 20.20.2 / npm 10.8.2, matching this repo's declared engines (node ^20, npm ^10). Regenerated with npm 10.8.2 under node 20. The +115/-1351 previously attributed to pre-existing npm dedupe was an npm 11 artifact, not real churn. Total diff vs development is now exactly +5/-5. npm ci under npm 10.8.2 exits 0; installed off disk 2.2.0-vue3.9, one copy, peer vue ^3.5.0. unit 1375/140 pass; test:l10n exit 0; check:manifest exit 0; build 3 warnings, 0 errors.
…ng (#172) E2E Tests (Playwright) has failed on the `development` push run since #163. Two of its four failures share one cause. #163 replaced seven native window.confirm/prompt guards with real in-page dialogs (gate-34 window-confirm: 7 -> 0). It did not update the two e2e specs that drove two of those call sites, and both still carried page.once('dialog', (dialog) => dialog.accept()) immediately before the destructive click. That handler does not fail when the page opens no native dialog — it simply never fires. So the click opened ConfirmActionDialog, the spec never answered it, the DELETE/manifest write was never issued, and the follow-up assertion failed as though the endpoint were broken: automations.spec.ts:293 Expected: 0 Received: 1 docudesk-document-templates.spec.ts:322 Expected: 0 Received: 1 Neither list was ever asked to change. The product is correct; the tests were addressing a browser primitive it no longer uses. Both now drive the dialog the product actually renders, through one shared helper. The helper ASSERTS the dialog appeared before answering it: a helper that merely clicked "confirm if present" would keep passing if a future change dropped the confirmation step altogether, which is the one property worth pinning here. The dialog is located by its ACCESSIBLE NAME (NcDialog `name` -> aria-labelledby) rather than a CSS class, so an unnamed confirmation dialog fails to match here instead of passing unnoticed. Refs #163 Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
…ate-58) (#174) * test(e2e): replace 14 networkidle waits with deterministic signals (gate-58) * test(e2e): accept the Dashboard create-app entry point as the chat-companion readiness signal * test(e2e): poll the Version history tab to a selected end state instead of racing the sidebar transition * test(e2e): require the Version history panel body, not just tab selection
…e anchors removed (#179) * test(e2e): 25 gate-19 anchors onto tests that already prove them, and 2 that did not gate-19 (@e2e traceability), FULL scope, measured with hydra-gates @ main (b8c7ead) against openbuild@development: before {scenarios: 767, covered: 72, excluded: 509, uncovered: 186} after {scenarios: 767, covered: 97, excluded: 509, uncovered: 161} 186 -> 161. No test was skipped, no timeout widened, no exclusion added. WHAT THE 25 ARE. Not new tests: `builder-undo-redo.spec.ts` and `form-editor-logic.spec.ts` already drive these scenarios and already run green; they carried no `@e2e` reference, so the gate could not see them. Every mapping was re-derived by reading the test BODY against the scenario text, not by matching titles. builder-undo-redo 11 anchors across 6 tests form-editor-logic 14 anchors across 5 tests One of the 14 needed the test EXTENDED before it could be anchored honestly: `deleting-a-step-returns-its-fields-to-the-unassigned-pool` asserted the removal and the pool re-appearance but not the spec's final AND (save auto-assigns an unassigned field to the last step). The surface exists (`assignUnassignedFieldsToFinalStep()`, called from PageDesignerHost's save()), so the test now re-adds a step, asserts the pool note, saves, and polls the PERSISTED manifest for the assignment. ONE SCENARIO WAS REFUSED, deliberately, with no exclusion written: `form-editor-logic::raw-json-authored-logic-survives-unrelated-editor-edits` asserts a Design <-> Raw JSON tab round-trip. That tab does not exist on /builder/:slug/pages — the raw editor is a sidebar tab on a different page, writing a different object. Anchoring it would have been false and excluding it would have been a false statement about observability, so it stays uncovered and an issue is the right next step. AND TWO ANCHORS WERE REMOVED, which RAISES the covered count's honesty at the cost of the number. `save-as-template.spec.ts` credited `viewer-cannot-save-a-template` and `seeded-cards-remain-read-only` to a test whose own comment, three lines below the tags, states that it proves neither: the assertion is toHaveCount(0) on a button, under an ADMIN session, on a page where the button is not scoped to appear. Filed as #178. Removing them did NOT raise `uncovered` (161 either way) — because both scenarios are ALSO swallowed by a requirement-level `@e2e exclude` whose stated reason is about something else entirely. Measuring that led to the wider finding: 471 of openbuild's 509 exclusions (92.5%) share a reason with a sibling, 42 reasons covering them, one covering 32 scenarios. Filed upstream as ConductionNL/.github#356. NEGATIVE CONTROL (scope-independent): 161 with tests/e2e/builder-undo-redo.spec.ts present 172 with it removed (+11 = exactly its 11 anchors) 161 restored Positive control that the gate validates slugs rather than the tag's presence: corrupting one slug to #adding-a-step-groups-fields-by-BOGUS dropped covered 99 -> 98; restoring returned it to 99. Refs #178 * test(e2e): a dangling @e2e anchor hid a scenario a running test already proves 161 -> 160. `component-blocks.spec.ts` anchored `component-blocks::blocks-filter-shows-blocks-without-the-clone-action`. That scenario lives in `openbuild-template-catalogue`, not `component-blocks`, so the ref resolved to NOTHING: gate-19 credited no scenario and reported no error, while the real scenario sat in the uncovered list and a green test that proves it was sitting right there. A dangling anchor is silent in both directions — it looks like coverage in the file and like an absence in the gate. Found by walking every anchor in the suite against the gate's own parser rather than by eye. openbuild has 15 of them. This is the only one that was a simple mis-spelled target. The other 14 are in `spec-coverage/page-designer-ui.spec.ts`, name slugs that exist in no spec, sit on seven tests CI never runs (`test.skip(!LIVE)`, `OPENBUILD_E2E_LIVE` is never set), and belong to ten scenarios that are ALL `@e2e exclude`d on the grounds that another spec's Playwright tests cover them. Repointing those while the tests stay skipped would move them from silently uncounted to counted against tests that never execute, so they are filed rather than patched — #181. What this anchor does NOT claim is recorded next to it: the scenario's THEN lists "name, description, category and a preview"; the test asserts the name and the AND (no clone action). The distinguishing behaviour — browse-only, no clone affordance — is covered. Refs #181 * test(e2e): an expect timeout equal to the test budget can never elapse The new REQ-OBFEL-001 poll carried `timeout: 30_000`, which is exactly the per-test budget in tests/e2e/playwright.config.ts. The test would always die first, so the poll's own message — "saving must append the still-unassigned field keys to the final step" — could never be printed, and the failure would surface as a bare test timeout instead. That is the precise failure mode the config's shorter `expect` timeout exists to prevent; its own comment says so. Use the repo default, 15_000. Not a widening: this LOWERS a number, and it makes the assertion able to report itself.
…actured coverage the moment anyone un-skipped them (#182) Seven `@e2e` anchors sat on `test.skip`ed tests whose bodies do not assert the scenario they name. gate-19 does not credit a skipped test, so none of them moves a number today. That is precisely why they are dangerous: un-skipping is the obvious way to burn gate-19 down, and doing it would have scored seven scenarios as covered without one new assertion being written. MEASURED, expectation computed BEFORE the run (hydra-gates @ 81c8c97, 1735-line checker), openbuild@development ff25c1b: as shipped {scenarios: 767, covered: 98, excluded: 509, uncovered: 160} the 7 tests un-skipped, nothing else changed {scenarios: 767, covered: 105, excluded: 509, uncovered: 153} restored {scenarios: 767, covered: 98, excluded: 509, uncovered: 160} +7 exactly, which is the count of anchors, naming those seven scenarios. WHAT IS WRONG WITH EACH, read from the body, not the title: openbuild-rbac two anchors about the VIEWER and the EDITOR sit on tests titled for the OWNER and the ADMIN, both driving the shared admin session. The second one's only assertion is `expect(count).toBeGreaterThanOrEqual(0)` on a locator count — a count is never negative, so the assertion CANNOT FAIL. application-detail-overview a requirement naming six rows IN DOM ORDER plus four sidebar tabs, anchored to a test that asserts `main` is visible and the title matches /openbuild/i. Its sibling computes `iconCount` and never asserts on it. app-icon-management "uploads a light icon" uploads nothing; "removes the dark icon" removes nothing and both of its branches pass without the feature; "non-SVG is rejected" puts its rejection assertion under `if (fileInputCount > 0)` and then `if (errorCount > 0)`, so the product failing to reject is exactly the case where nothing is asserted. That body already says so: "the test passes vacuously because the UI is not built". THE NUMBER DOES NOT MOVE, and that is the point: after removal {scenarios: 767, covered: 98, excluded: 509, uncovered: 160} No test was skipped, un-skipped, deleted or loosened; no exclusion was written; no timeout was widened. What changed is the count of uncovered scenarios carrying a dead anchor: 35 -> 28 (the gate's own `collect_ref_status`). The anchors are DELETED rather than commented out. Per ConductionNL/.github#358 both gate-19 and gate-26 parse prose, so an anchor quoted in an explanatory comment is re-read as a live anchor — the replacement notes name no slug. SCOPE, stated because it is weak: this change touches three TEST files and no spec file, so `changed_spec_files()` returns the empty set and gate-19's own cell on this PR is EMPTY SCOPE (exit 3, NOT APPLICABLE) — it corroborates nothing. The full-scope figures above and the +7/-7 control are the only evidence, and the control is the load-bearing half. Refs #178, ConductionNL/.github#343, ConductionNL/.github#358
Lock-only bump; `composer audit` clean after it and CI's phpcs job exercises it. Merged over a red E2E cell that is pre-existing and STRICTLY NOT WORSENED, verified against the same-tree baseline rather than an eyeball: | | development 31459739568 | this PR | |---|---|---| | failed | 2 | **2 — identical set** | | passed | 181 | **181** | | skipped | 68 | **68** | Both remaining failures are `automations-rbac.spec.ts:124` and `:152`, and both are test-harness gaps (a first-open CnSupportDialog modal mask, and a fixture app CI never creates) rather than the RBAC 500 — remedied by the harness half of the review-only PR #177. `version-rollback.spec.ts:263`, which was red on the current development tip, PASSED here — consistent with the same-SHA-opposite-verdict pair already recorded (`2b1a8900` failed it in run 31438032584 and passed it in run 31459739568), so it is not attributable to any diff. Mechanism candidate filed as nextcloud-vue#632, deliberately not masked.
…sors — the slug resolver was dead (#185) ObjectSchemaSlugResolver guarded all three of its accessor calls with method_exists(). OpenRegister's ObjectEntity, Schema and Register extend Nextcloud's Entity, which serves every column accessor through __call() and declares it as an @method docblock only. method_exists() is therefore false for getSchema(), getRegister() and getSlug() on the real classes, so every guard took its false branch, schemaSlug() returned '' for every real entity, and isOpenBuildSchema() was false even for openbuild's own objects. Measured on this instance: method_exists false / is_callable true for those three, against the concrete control getObject() which is true for both. That disabled the two listeners the resolver exists to serve — ProductionVersionGuardListener and AutomationCleanupListener. Both sit behind ListenerSlugContract, which defaults off, so default behaviour does not change here: what changes is that enabling the flag now actually wakes them instead of silently keeping them off. Waking them remains a rollout decision, unchanged. The file's own header already documented the neighbouring trap (getSchemaSlug() does not exist and the ids are not slugs) while all three of its guards carried this one, so the fix is applied at the single private seam they funnel into rather than at the reported line. is_callable() is not a membership test on a __call class — it is true for any name — so readAccessor() is exception-safe; Entity throws BadFunctionCallException for a column it does not have. ProductionVersionGuardListener::extractUuid() carried the same defect on getUuid() and is fixed the same way. Why the suite never caught it: tests/stubs/openregister-stubs.php declares these accessors concretely, for a real reason (PHPUnit 10 removed addMethods(), so createMock()->method('getUuid') cannot configure a magic method). That stub inverts the exact predicate under test — method_exists is true in the suite and false in production. The new test therefore extends the real OCP\AppFramework\Db\Entity so its accessors are reached the way production reaches them, and carries a concrete-method control so a false cannot come from a broken fixture. Negative control, predicted before running: reverting the guard at readAccessor() turns exactly 3 of the 7 new tests red. Observed exactly those 3 (testSchemaSlugResolvesThroughMagicAccessor, testRegisterSlugResolvesThroughMagicAccessor, testIsOpenBuildSchemaMatchesOpenBuildsOwnObject); the other 4 stayed green. Suite 797 -> 804 tests, all green. lint/phpcs/phpmd/psalm/phpstan clean. Co-authored-by: Ruben van der Linde <juan.claude@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
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Automated PR to sync development changes to beta for beta release.
Merging this PR will trigger the beta release workflow.