feat(cmdb-import): import a TOPdesk CMDB export (xlsx) into the catalogue

[WilcoLouwerse]

Jira: WOO-586 · Plan: readonly-mirror-wilco-claude-plans › issues/jira/WOO-586

What

Gemeente Rotterdam wants its CMDB (a TOPdesk export, xlsx) as a source for OpenCatalogi. This adds a CMDB import to Stackiq's admin settings:

• An admin picks the consuming municipality (existing or new) and uploads the TOPdesk export.
• Stackiq reads the two CMDB sheets the municipality uses as its CMDB: "Onbeh Applicaties CMDB" (applications without arranged maintenance, derived from AIA) and "Beheerde Applicaties CMDB" (with arranged maintenance, derived from APP). The raw "Invoer" sheets are not read. Per row it creates or updates a module, the vendor and the municipality as organization, a usage linking the municipality to the module, and the owner as contactPerson, all through OpenRegister's object service.
• The column mapping is declarative (lib/Settings/cmdb-import/, five packs plus a profile) and runs through OpenRegister's migration-pack MappingEngine. OpenRegister's own import endpoint cannot apply packs to xlsx and maps one sheet to one schema, while one TOPdesk row becomes several objects.
• Modules are matched on topdesk:<municipality uuid>:<APPID> (the APPID is TOPdesk's ICT Applicatienummer; the Applicatie Code / Middel-ID can change in TOPdesk and is kept as externalId for reference). A re-import updates instead of duplicating. New modules get a publicationDate, so OpenCatalogi lists them. Records missing from a newer export are left alone.
• Mapped: name, Roepnaam/Nickname (short description), functional description, Applicatiesoort (hosting model), BNN Classificatie (BBN level), source dates, Vendor (supplier), Applicatie Status, Classificatie (TIME), End-of-Life Functioneel (phase-out date), and an internal note on the usage with Beheer geregeld: ja|nee, the cluster and the owner's department. The column table is in design.md and docs/features/cmdb-import.md.
• The CMDB sheets are formulas: the reader uses the value Excel cached and never evaluates a formula. A formula without a cached value is an empty cell with a row warning, never a failure. The CMDB placeholders NB (BNN) and 2036-01-01 (end of life) are read as empty.
• Each row is processed in isolation and reported (created / updated / unchanged / skipped / failed with a reason). Progress and cancel use the existing ProgressTracker.

OpenSpec change: openspec/changes/cmdb-export-import/ (proposal, specs REQ-CMDB-001..014, design, contract, migration, test-plan, tasks).

Security and privacy

• Both routes are admin-only and require CSRF (unlike the existing SBOM/ArchiMate uploads, which have NoCSRFRequired).
• xlsx only, checked by extension, ZIP signature and xl/workbook.xml before the reader runs; 10 MB per upload, 10,000 rows per sheet.
• Values are read with setReadDataOnly(true); formulas are never evaluated and external connections are never followed.
• Columns are resolved by header name per sheet, against an allowlist. The only person columns read are "Applicatie Eigenaar (Persoon)" and "(Functie)"; the functional administrator is not imported. Owner names are never logged or returned in the report. Imported contacts never become Nextcloud users.
• Owners are never publicly readable: contactPerson and usage have no public read rule, and a published module refers to them by id only. CmdbPersonDataVisibilityTest pins that on the merged register; the e2e suite checks it anonymously against the running stack (OpenRegister objects API and OpenCatalogi search).
• Test fixtures are anonymised; a PHPUnit hygiene test fails on document metadata or non-placeholder personal data.

Schema

module goes to 0.3.5 through the register fragment lib/Settings/register.d/topdesk-cmdb-import.json: five optional properties externalId, externalNumber, externalKey, externalCreatedAt, externalModifiedAt, plus three seed modules (no publicationDate).

Tests

Check	Result
PHPUnit, CMDB tests (--filter 'Cmdb|TopdeskCmdb', --no-coverage, OpenRegister on the path)	90 tests, 960 assertions, exit 0
Playwright tests/e2e/spec-coverage/cmdb-import.spec.ts on a local NC 32 rig	5 of 5 passed, including the anonymous owner-visibility check
phpcs / phpmd on the changed lib/ files, phpstan on lib/Service/CmdbExportImportService.php + lib/Service/Cmdb/	exit 0
npm run build, lint, stylelint, test:l10n, check:l10n-js	exit 0 (lint: warnings only, none in changed files)
Newman, folder "12 - CMDB import"	not re-run for this revision
Hydra gates (scripts/run-hydra-gates.sh)	not run yet

Manual check on the rig (data of the earlier import removed first): importing the anonymised export created 2 modules, 2 suppliers, 1 municipality, 2 usages and 2 contact persons, with cloudDienstverleningsmodel, bbnLevel, timeClassification, startDateOutPhased and the maintenance note filled where the fixture has values; a second import reported 2 unchanged. An anonymous OpenCatalogi search finds both applications with contactPerson and usages empty and no owner name in the hit; anonymous OpenRegister requests for contact persons and usages return 0 rows.

Not in this PR

Hosting party ("Hostingpartij") and "Leverancier", the functional administrator as technical owner, connections and suites, the "Gearchiveerde Applicaties" sheet and marking records missing from an export, an occ command or background job, and a live TOPdesk/ServiceNow connection. These are listed as follow-ups in the plan.

🤖 Generated with Claude Code

[github-actions]

Quality Report — ConductionNL/stackiq @ 69bec95

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
check-manifest		✅
check-vue-demi		✅
test-l10n		✅
format		✅
check-schema-l10n		❌
check-l10n-js		✅
composer			✅	✅ 130/130
npm			✅	✅ 807/807
app:check-code	⏭️
info.xml	✅
REUSE	❌
lockfile sync		✅
PHPUnit					❌
Newman					⏭️
Playwright					⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets development, so the suite is asked once per promotion into beta and main rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into development.
Hydra gates					❌

---

Quality workflow — 2026-10-01 12:40 UTC

Download the full PDF report from the workflow artifacts.

[WilcoLouwerse]

🔴 Blocker — Gate 5 route-auth fails: import and cancel declare no auth attribute

CI's required check quality / Hydra Gates fails on gate 5 (route-auth, PROTECTED): CmdbImportController.php:88 method=import and :167 method=cancel both report rule=missing-auth-attribute. The gate wants one of #[PublicPage], #[NoAdminRequired], #[NoCSRFRequired] or #[AuthorizedAdminSetting(...)] (docblock tags count too) in the 20 lines above each method. Neither method, nor the class, carries any; the class docblock only describes the posture in prose, which the gate does not read. Both gate misses reproduce locally (0 hits in the 20-line window above lines 88 and 167).

Actual exposure today, stated honestly: with no attribute Nextcloud's default applies (SecurityMiddleware: non-admin gets 403 NotAdminException, missing CSRF token gets 412), so this is not a live auth bypass. The block is on the declaration. The review checklist says absence is a valid admin-only mechanism unless the app already uses AuthorizedAdminSetting; this app does (ModerationController and FederationController use #[AuthorizedAdminSetting(settings: StackiqAdmin::class)] plus the docblock tag), so follow that precedent. Do NOT satisfy the gate with @NoCSRFRequired (contract.md and REQ-CMDB-001 deliberately require CSRF) and do not add a docblock line that merely mentions @NoAdminRequired: Nextcloud's regex parses it as the real annotation and the endpoint would become reachable by every signed-in user (the failure AdminAuthPostureTest documents).

Behaviour change to decide deliberately: StackiqAdmin implements IDelegatedSettings, and AuthorizedAdminSetting lets a group that an admin has delegated that settings section to reach the endpoint, not only full Nextcloud admins. An import writes catalogue-wide data for a municipality. If delegated admins are acceptable, say so in REQ-CMDB-001, contract.md (both Auth lines) and the openapi descriptions, which all currently say 'Nextcloud admin' and 'SHALL NOT carry NoAdminRequired/NoCSRFRequired' (the latter stays true). If not, the alternative that satisfies the gate and stays strictly admin is #[NoAdminRequired] plus an explicit IGroupManager::isAdmin() check in the body, which contradicts the spec sentence and fails open if the check is dropped, so the first option is preferred.

Skill: /home/wilco/hydra/.claude/skills/hydra-gate-route-auth/SKILL.md.

Persistence audit (A2): if the fix copies ModerationController's #[AuthorizedAdminSetting(settings: StackiqAdmin::class)], any group an admin delegated the Stackiq settings to can then bulk-write the catalogue with _rbac:false. Keep it full-admin unless delegation is a conscious decision, and then say so in REQ-CMDB-001 and contract.md. Do not add #[NoAdminRequired].

Done when:

• quality / Hydra Gates is green on the PR head, gate 5 reports nothing for CmdbImportController.php
• import() and cancel() each carry #[AuthorizedAdminSetting(settings: StackiqAdmin::class)] (plus the docblock tag as in ModerationController), or the author documents a different gate-accepted choice
• The delegated-admin consequence is decided and written into REQ-CMDB-001, contract.md and the openapi descriptions
• testBothRoutesAreAdminOnlyWithCsrf is updated to the new declaration (see A2)

_{Verification: cd /tmp/pr-1209-clone && for m in import cancel; do l=$(grep -nE "^\s*public\s+function\s+$m\s*\(" lib/Controller/CmdbImportController.php | cut -d: -f1); s=$((l-20)); sed -n "${s},${l}p" lib/Controller/CmdbImportController.php | grep -cE '#\[(PublicPage|NoAdminRequired|NoCSRFRequired|AuthorizedAdminSetting)\b|@(PublicPage|NoAdminRequired|NoCSRFRequired)\b' | sed "s/^/$m hits: /"; done # both must be >=1; today 0 and 0}

Choosing the attribute. Gate-5 accepts #[AuthorizedAdminSetting(...)] whatever its argument (hydra-gate-route-auth, "Heuristics the check accepts"). In Nextcloud that attribute admits full admins and every group delegated to the named settings class, so AuthorizedAdminSetting(settings: StackiqAdmin::class), the form ModerationController uses, widens who may bulk-write with _rbac:false. Nextcloud has no attribute that means full-admin only. So decide:

• (a) accept delegation and write that down in REQ-CMDB-001, contract.md and openapi.json; or
• (b) keep full-admin: add an explicit IGroupManager::isAdmin() check with a test. Note that the gate skill says such a check also passes, but its grep (SKILL.md line 56) only looks for attributes, so confirm gate-5 goes green before relying on it.

Do not add #[NoAdminRequired].

[WilcoLouwerse]

🔴 Blocker — gate-46: unresolved @SPEC anchors in vendored OR copies

🔴 Blocker — gate-46 spec-anchor-existence fails on the vendored OpenRegister copies (required check)

tests/Unit/Support/OpenRegister/MappingEngine.php (lines 35, 57, 69, 143) and PackDefinitionValidator.php (lines 29, 50, 84, 110) carry eight @spec openspec/specs/migration-mapping-packs/spec.md[#anchor] tags. That spec lives in openregister, not in stackiq, so Hydra Gates is red. See the hydra-gate-spec-anchor-existence skill for the accepted fixes (drop the tag from the copied header, or a reason-bearing @spec exclude {reason}).

Second question you asked, whether a test-side copy is a valid oracle: it is a verbatim copy (I diffed it against openregister/lib/Service/MigrationPack/*.php at 2.1.34: only the added TEST COPY header differs), so today it cannot diverge. But it is a snapshot with no drift check. Where OpenRegister is checked out the tests load the real class; where it is not (the fallback), every pack-validation and mapping assertion runs against a frozen 2.1.34 copy, so a later change to mapRow() (new error shape, a changed lookup rule) passes CI and fails in production. The copies also sit in the OCA\OpenRegister\... namespace inside stackiq's tests, which is how the foreign @spec tags got here.

Impact: the required gate stays red; the copy gives false assurance once OpenRegister moves on.

Suggested fix: remove the eight @spec lines from the copies (the header already says they are copies) and add a drift test that, when CmdbTestSupport::openRegisterDir() finds OpenRegister, asserts sha1 of the copy body equals the real file (skip with a named reason otherwise). Prefer running the CMDB tests against the real classes in CI instead of the copy.

Done when:

• git grep -n '@spec openspec/specs/migration-mapping-packs' -- tests/ returns nothing, and Hydra Gates is green
• a test fails when tests/Unit/Support/OpenRegister/MappingEngine.php is edited by one character while OpenRegister is on the path

_{Verification: gh pr checks 1209 -R ConductionNL/stackiq | grep 'Hydra Gates'; cd /tmp/pr-1209-clone && git grep -n '@spec openspec/specs/migration-mapping-packs' -- tests/ | wc -l -> 8}

[WilcoLouwerse]

🔴 Blocker — 10 MB / 10,000-row caps do not bound memory (OOM fatal)

🔴 Blocker — the size and row caps do not bound what the reader loads; a 9.7 MB upload kills the PHP worker

read() calls $reader->load($path) (line 166) on the whole sheet, and only afterwards counts rows (readRows(), line 333). The 10 MB cap is on the compressed upload, and assertXlsx() never looks at the uncompressed size of the package. I built a 9.77 MB xlsx (2,000,000 <row> elements, 99.8 MB of sheet XML, one unread column) and loaded it exactly as the reader does (setReadDataOnly(true), setReadEmptyCells(false), setLoadSheetsOnly): PHP died with Allowed memory size of 536870912 bytes exhausted inside Cell.php. That is an uncatchable fatal, so the try/catch at lines 165-173 never runs, the admin gets an HTML 500, and TOO_MANY_ROWS is never reached. A better-compressing file (repeated identical rows) needs fewer bytes still. PR body: "10 MB per upload, 10,000 rows per sheet" and the spec's security NFR ("bounded size and row count") are therefore not true for memory or time.

XXE is fine: PhpSpreadsheet 5.10 refuses a DOCTYPE/ENTITY (Detected use of ENTITY in XML ...), which surfaces as NOT_XLSX. I checked that too.

Impact: denial of service of the PHP worker (and the Nextcloud instance's memory) by anyone who can use the admin route, with a CSRF-passing 10 MB file; no row cap is reached.

Suggested fix: in assertXlsx() open the ZipArchive and sum statIndex()['size'] of xl/worksheets/*.xml, xl/sharedStrings.xml and the workbook; refuse above a profile limit (e.g. maxUncompressedBytes, default 50 MB) before PhpSpreadsheet runs. In read() pass a IReadFilter that admits only row 1 and rows 2..maxRowsPerSheet+1 and only the referenced columns, so the library never materialises more.

Done when:

• php -d memory_limit=512M /tmp/claude-1000/-home-wilco-hydra/4c54cb9e-dc0c-4004-8bbb-60471f7b46ca/scratchpad/b/rd.php /tmp/claude-1000/-home-wilco-hydra/4c54cb9e-dc0c-4004-8bbb-60471f7b46ca/scratchpad/b/bomb.xlsx (the 9.7 MB file) no longer dies: the import answers 422 before PhpSpreadsheet loads
• a unit test builds a package whose xl/worksheets/sheet1.xml inflates past the limit and expects CmdbImportException (TOO_MANY_ROWS or a new code), without calling load()
• a unit test with maxRowsPerSheet = 1 on a sheet with 3 data rows proves the read filter, not the post-load loop, stops it

_{Verification: cd /tmp/claude-1000/-home-wilco-hydra/4c54cb9e-dc0c-4004-8bbb-60471f7b46ca/scratchpad/b && php mk.php 2000000 bomb.xlsx && php -d memory_limit=512M rd.php bomb.xlsx -> 'Allowed memory size of 536870912 bytes exhausted' (needs /home/wilco/woo586-rig/apps/openregister/vendor)}

[WilcoLouwerse]

🔴 Blocker — Ten new schema strings have no l10n catalogue key

CI quality / Frontend Check (check:schema-l10n) fails: 691 schema strings, 489 uncovered, baseline 479. The fragment adds exactly ten strings (title + description of the five new module properties externalId, externalNumber, externalKey, externalCreatedAt, externalModifiedAt, lines 9/10, 18/19, 27/28, 40/41, 49/50). node scripts/check-schema-l10n.js --list on the head minus the same list on origin/development gives exactly these ten and nothing else.

Impact: the new fields render in English (label and helper text) inside an otherwise Dutch module form, and the ratchet is red so the PR cannot merge. Raising the baseline (--update) is not the fix: it hides the debt the ratchet exists to measure.

Suggested fix: add the ten strings to l10n/en.json (identity) and l10n/nl.json, then npm run l10n:build. Proposed Dutch: Source id = Bron-id; The identifier of the application in the source system it was imported from, such as the TOPdesk Middel-ID. = De identificatie van de applicatie in het bronsysteem waaruit is geimporteerd, zoals het TOPdesk Middel-ID.; Source number = Bronnummer; The application number in the source system, such as TOPdesk's ICT Applicatienummer. Shown for reference; not used to match records. = Het applicatienummer in het bronsysteem, zoals het ICT Applicatienummer van TOPdesk. Ter informatie; niet gebruikt om records te koppelen.; Import key = Importsleutel; The key a repeated import matches this application on: topdesk::. Set by the CMDB import; do not edit. = De sleutel waarop een herhaalde import deze applicatie herkent: topdesk::. Door de CMDB-import ingesteld; niet wijzigen.; Created in source = Aangemaakt in bron; The date the application was registered in the source system. = De datum waarop de applicatie in het bronsysteem is geregistreerd.; Changed in source = Gewijzigd in bron; The date the application was last changed in the source system. = De datum waarop de applicatie in het bronsysteem voor het laatst is gewijzigd. (use the proper i-diaeresis in geimporteerd.)

Done when:

• node scripts/check-schema-l10n.js exits 0 without a changed l10n/.schema-l10n-baseline.json
• node scripts/check-schema-l10n.js --list | grep -c topdesk-cmdb-import prints 0
• npm run check:l10n-js and npm run test:l10n still exit 0

_{Verification: cd /tmp/pr-1209-clone && node scripts/check-schema-l10n.js; echo exit=$? # red today: '691 schema string(s); 489 uncovered, baseline 479'; and: diff <(cd <origin/development checkout> && node scripts/check-schema-l10n.js --list) <(node scripts/check-schema-l10n.js --list) lists the 10 strings}

[WilcoLouwerse]

🔴 Blocker — Fixtures keep printer/device metadata and source timestamps from the original workbook

The four derived fixtures and topdesk-export-anonymised.xlsx are made by hand-anonymising the real export and then stripping only core.xml creator/lastModifiedBy, custom.xml, customXml, absPath and connections.xml (DROP_PARTS / DROP_PREFIXES, lines 36-37; sanitise(), lines 63-93). The tests/fixtures/cmdb/README.md says 'Document metadata ... removed'. That is not true for what is left:

• xl/printerSettings/printerSettings1..10.bin: 1 and 10 name 'HP055CC5 (HP Deskjet F4500 series)' and '... (vdi)', 2-9 name 'EPSON4E1D67 (XP-2200 Series)'. The suffixes are the device-specific part of the printer name (derived from its network address), and '(vdi)' shows it was a printer redirected into a virtual-desktop session, i.e. the machine the export was handled on. Referenced from the sheet rels, which sanitise() keeps.
• docProps/core.xml keeps dcterms:created 2022-05-04T12:35:44Z (creation time of the real export) in all four derived fixtures.
• xl/workbook.xml keeps the xr:revisionPtr documentId GUID and the _xlnm._FilterDatabase ranges, which expose the size of the real CMDB ('Beheerde Applicaties CMDB'!$A$1:$AP$1066, 'Gearchiveerde Applicaties'!$A$1:$I$434); sheet3.xml dimension A1:CD1136.
• Real (non-personal) organisation data remain as cell text: unit codes 'B10 Maatschappelijke Ontwikkeling', 'H10 IIFO (T)', 'H10 Bestuurs- en Concernondersteuning', 'B10 MOW Leiding & Ondersteuning 1', behandelgroep 'AC Liquide middelen', applications 'G4net' / 'Pronexus G4net' and 'Aangetekend B.V.'. These identify the source municipality; not person data, so a judgement call, but 'same structure, anonymised' (Jira WOO-586) suggests replacing them.

I enumerated all 224 shared strings and every cell value of all 10 sheets of topdesk-export-anonymised.xlsx: no person name, personnel number, phone number or non-placeholder e-mail address remains (see the surface table). The hard constraint on personal data holds for cell content; the residue above is device/provenance metadata, in a PUBLIC repo where it cannot be taken back.

Impact: a printer/device identifier and the creation time of the Rotterdam export are published permanently; the README's claim is inaccurate; the hygiene test (C4) cannot catch any of it.

Suggested fix: in sanitise(): drop xl/printerSettings/* plus their Relationship entries in xl/worksheets/_rels/*.rels, the r:id on <pageSetup> in every sheet, and the bin Default content type if unused; set dcterms:created/modified to a fixed fictional date; drop xr:revisionPtr; rewrite or drop the _xlnm._FilterDatabase defined names and the dimension of the data sheets; rebuild all five fixtures; optionally replace the real unit/vendor/application names with neutral ones.

Done when:

• unzip -l tests/fixtures/cmdb/*.xlsx | grep -c printerSettings prints 0 for every fixture
• unzip -p tests/fixtures/cmdb/topdesk-export-anonymised.xlsx docProps/core.xml shows no real creation timestamp
• a copy of a fixture with xl/printerSettings/printerSettings1.bin re-added makes CmdbFixtureHygieneTest fail (see C4)
• tests/fixtures/cmdb/README.md matches what the files hold

_{Verification: cd /tmp/pr-1209-clone/tests/fixtures/cmdb && for f in *.xlsx; do echo "$f printerSettings=$(unzip -l $f | grep -c printerSettings)"; done; unzip -p topdesk-export-anonymised.xlsx xl/printerSettings/printerSettings1.bin | strings -e l | head -2; unzip -p topdesk-export-anonymised.xlsx docProps/core.xml | grep -o 'created[^<]*<'}

[WilcoLouwerse]

🟢 Minor — Requirement headings use the REQ-ID prefix form, not ADR-038 Form A

ADR-038 puts the ID as a parenthesised suffix: ### Requirement: <Title> (REQ-XX-NNN). All 14 headings here (REQ-CMDB-001..014 in the delta) are ### Requirement: REQ-CMDB-001 <Title> (Form D), while REQ-CMDB-000 in openspec/specs/cmdb-export-import/spec.md uses the suffix form, so the merged capability will mix both at archive. The repo as a whole is mixed (116 suffix, 61 prefix headings under openspec/specs), so this is consistency, not a break. Renaming changes the heading slugs that @spec ...#requirement-req-cmdb-0NN-... tags in src/ and lib/ point at (gate 46), so it needs a sweep of those anchors.

[WilcoLouwerse]

🟢 Minor — Repo text points readers at a private plan folder

'the anonymised test export from the WOO-586 plan folder' (tasks.md:11), 'route A in the WOO-586 plan' (design.md:63) and 'USER MANUAL TEST, WOO-586 Stap 6b' (test-plan.md:96) refer to a private workspace that contributors of this public repo cannot open. tasks.md:11 also reads as if the source file is kept somewhere next to the plan; the README rightly says it must never be committed. State the rule, not the location.

[WilcoLouwerse]

🟢 Minor — t() with variables HTML-escapes values that Vue escapes again

@nextcloud/l10n translate() escapes variable values (escape: true) and runs DOMPurify (sanitize: true) by default. The component renders the result through {{ }}, which escapes once more. A municipality typed as Berkel & Rodenrijs renders in 'Import for ... finished' (lines 490-503) and 'A new municipality "{name}" is created' (line 40) as Berkel & Rodenrijs; same for sheet/column names from details in errorText() (cmdbImport.js:331, 343, 359). I reproduced it with @nextcloud/l10n 3.4.1 + jsdom: translate('stackiq','Import for {name} finished.',{name:'Berkel & Rodenrijs <b>'}) gives Berkel & Rodenrijs <b>. The same pattern exists at 37 other single-line sites in src/**/*.vue, so this is a fleet quirk, not new to this PR; ampersands in municipality names are rare. XSS is not possible (no v-html).

[WilcoLouwerse]

🟢 Minor — Report headings skip a level; file label looks like a drop zone

The report uses <h4> (lines 204, 231) directly under the settings section's heading (NcSettingsSection renders an h2), skipping h3 (WCAG 1.3.1 best practice, not a failure). The file control is a dashed, large 'drop zone' look but has no drop handler; the spec (REQ-CMDB-014 NFR) correctly says dragging does not apply, so the visual affordance promises something the control does not do. Runtime behaviour not checked (no browser run in this review).

[WilcoLouwerse]

🟢 Minor — E2E leans on vue-select internals and skips instead of failing

.vs__selected (line 190) and td nth(1) (line 336) are library class/position selectors instead of data-testid; requireFixture() (line 76) turns a missing fixture into a skip, which the gates policy reads as 'no evidence', not a pass; the second test skips when the first wrote nothing (line 372), hiding a failed first run if serial mode ever changes; a first-run-wizard workaround calls DELETE /apps/firstrunwizard/wizard (line 148). The tests do upload the real fixtures and assert real outcomes (created/unchanged counts, module links, 422 body, 400 NOT_XLSX), which is the right shape.

[WilcoLouwerse]

Verdict: REQUEST_CHANGES (Strict) — self-review posted as COMMENT (GitHub blocks REQUEST_CHANGES on your own PR)

The import works as designed for a well-formed, sequential import, but it cannot merge yet. The required quality / Hydra Gates check is red (gate-5 route-auth, gate-46 spec anchors), the workbook reader has no memory bound, ten new schema strings have no translation key, and the fixtures still carry device metadata from the original export. Under Strict the concerns block too; several of them (B2, B6/B14, B10, B13, P1) are design decisions rather than code fixes.

🔴 Blockers (5)

• Gate 5 route-auth fails: import and cancel declare no auth attribute — lib/Controller/CmdbImportController.php:88. import() and cancel() declare no auth attribute, so the required Hydra Gates check is red. Nextcloud's default keeps them admin-only today; choosing AuthorizedAdminSetting(StackiqAdmin) would also admit delegated groups.
• gate-46: unresolved @spec anchors in vendored OR copies — tests/Unit/Support/OpenRegister/MappingEngine.php:35. 8 @spec tags in the test-side copies of OpenRegister's MappingEngine/PackDefinitionValidator point at openspec/specs/migration-mapping-packs/spec.md, which does not exist in stackiq.
• 10 MB / 10,000-row caps do not bound memory (OOM fatal) — lib/Service/Cmdb/CmdbWorkbookReader.php:166. The reader load()s the whole workbook before the row cap at line 245 applies, with no read filter or decompressed-size check; a 9.77 MB crafted file exhausted a 512 MB memory limit.
• Ten new schema strings have no l10n catalogue key — lib/Settings/register.d/topdesk-cmdb-import.json:9. check:schema-l10n is red (489 uncovered against a baseline of 479): the new fragment adds 10 schema strings with no en.json/nl.json key.
• Fixtures keep printer/device metadata and source timestamps from the original workbook — tests/fixtures/cmdb/build-fixtures.py:36. Four of the five fixtures still contain 10 xl/printerSettings/*.bin parts with device names (HP055CC5 (HP Deskjet F4500…), EPSON4E1D67), the original 2022 creation timestamp and a revision GUID; build-fixtures.py does not strip them.

🟡 Concerns (29)

• Admin-only/CSRF requirement has no test that can fail for it — tests/Unit/Controller/CmdbImportControllerTest.php:135. The admin/CSRF requirement is pinned only by getAttributes() === [], which A1's fix must break, and the Newman 403/412 cases never run in CI (enable-newman: false).
• Catch of \Exception lets \Error escape the IMPORT_FAILED envelope — lib/Controller/CmdbImportController.php:102. catch (\Exception) lets an \Error/TypeError from the service escape the IMPORT_FAILED envelope as a bare 500.
• updateExisting parser fails open to the overwriting mode — lib/Controller/CmdbImportController.php:257. Any updateExisting value outside false/0/no/off (a typo like flase) means overwrite, so the parser fails open to the destructive mode.
• Cancel accepts finished operations and leaves a stale cancel flag — lib/Controller/CmdbImportController.php:168. cancel returns success for a finished operation and leaves a cancel flag behind that a reused operationId inherits.
• Import makes every imported application anonymously readable — lib/Service/CmdbExportImportService.php:690. Every created module gets publicationDate = now, which the module schema's public read rule turns into anonymous visibility. The PR intends this, but there is no way to import unpublished.
• Idempotency fails open: no check that module has the new properties — lib/Service/CmdbExportImportService.php:1129. If the module schema has not been migrated to 0.3.5, the externalKey filter becomes 1 = 0, so every re-import creates duplicates instead of failing.
• Concurrent or repeated submits duplicate every object — lib/Service/CmdbExportImportService.php:687. No lock or in-flight check: a double submit or two admins importing the same file at once duplicates every object.
• externalKey is a client-writable identity; import overwrites matches across tenants with RBAC off — lib/Service/CmdbExportImportService.php:1131. externalKey has no write authorization, and the import matches it across tenants with _rbac:false, so any module writer can plant a key the next import overwrites.
• Tombstoned and inactive organisations are matched as municipality or supplier — lib/Service/CmdbExportImportService.php:1107. Organisations with status merged (tombstones) or inactive are still matched as municipality or supplier.
• Municipality by name picks the first of several same-name matches — lib/Service/CmdbExportImportService.php:1000. A municipality given by name takes the first of several same-name matches, and an unknown name silently creates one.
• Synchronous 10k-row import: no time limit, operation left running on abort — lib/Service/CmdbExportImportService.php:260. The 10,000-row import runs synchronously, with no set_time_limit/ignore_user_abort, and an aborted request leaves the operation running.
• Owner personal data lands in the importing admin's address book — lib/Service/CmdbExportImportService.php:908. Owners with an e-mail are written as vCards into the importing admin's first writable address book, and nothing removes them later.
• Untrusted cell text flows unsanitised into the existing CSV export — lib/Settings/cmdb-import/topdesk-module.json:8. Cell text reaches the existing CSV export (PortfolioReportService fputcsv) unsanitised, so a =HYPERLINK(...) cell becomes formula injection.
• Row-failure detail echoes raw exception text into report and log — lib/Service/CmdbExportImportService.php:1323. Row failures put raw exception text into the report and the log; the 'no personal data' test injects a benign message, so it cannot catch this.
• Re-import overwrites stackiq-side edits; create and update paths differ — lib/Service/CmdbExportImportService.php:773. A re-import overwrites stackiq-side edits, including the lifecycle-managed usage.status, and create-only fields differ per pack without that being documented.
• Created objects belong to the admin's organisation, not the municipality — lib/Service/CmdbExportImportService.php:1172. OpenRegister stamps created objects with the importing admin's active organisation, not the municipality the rows belong to (needs a rig run to confirm).
• Service tests run against a fake that always filters correctly — tests/Unit/Service/CmdbExportImportServiceTest.php:219. The service tests use a search double that always filters correctly and never sees _rbac/_multitenancy, so they cannot catch B4 or a dropped filter.
• CI gives no evidence for these tests; several assertions are source greps — tests/Unit/Service/Cmdb/CmdbWorkbookReaderTest.php:132. CI's PHPUnit dies at bootstrap (OCP\EventDispatcher\Event not found), and several reader tests are source greps, so there is no CI evidence for the '81 tests'.
• Hygiene test only scans .xml/.rels parts and two core.xml fields — tests/Unit/Fixtures/CmdbFixtureHygieneTest.php:185. The hygiene test scans only .xml/.rels parts and two core.xml fields, so it misses C3's .bin parts, app.xml and the dates.
• Name-column check is skipped for 'Invoer APP data' in the missing-middel-id fixture — tests/Unit/Fixtures/CmdbFixtureHygieneTest.php:244. In the missing-id fixture the APP sheet's header is Middelnummer, so the name-column check break 2s and that sheet is never checked for names.
• UI hard-codes limits and sheet names the server reports (and the docs say admins can change) — src/utils/cmdbImport.js:19. The UI hard-codes the 10 MB / 10,000-row limits and the sheet names instead of reading the server's details, which the docs say an admin can change.
• A request that dies mid-import shows 'failed' and drops the report the server keeps — src/utils/cmdbImport.js:186. A 502/503/504 during a long import shows IMPORT_FAILED and discards the report the server keeps for the operation.
• Cancel failure is swallowed and Cancel gives no feedback — src/views/settings/sections/CmdbImport.vue:800. The cancel call's failure is swallowed in an empty catch, and the user gets no feedback either way.
• No frontend unit tests for cmdbImport.js (sibling helper has them) — src/utils/cmdbImport.js:1. cmdbImport.js (494 lines of request and error mapping) has no unit tests, while its sibling helper does.
• 29 new scenarios rely on @e2e exclude, which the e2e gate does not accept for added scenarios — openspec/changes/cmdb-export-import/specs/cmdb-export-import/spec.md:111. 29 added scenarios carry @e2e exclude; the typed-municipality and cancel flows have no browser test.
• Task 10 and docs screenshots are unfinished; task checkboxes disagree with the PR body — docs/features/cmdb-import.md:51. Task 10 and the three doc screenshots are unfinished (no docs/images/cmdb-import-*), and the task checkboxes disagree with the PR body.
• Report table renders every row with no pagination or cap — src/views/settings/sections/CmdbImport.vue:246. The report table renders every row, up to 10,000 per sheet, with no pagination or cap.
• Imported Active orgs can re-key a tenant by slug — lib/Settings/cmdb-import/topdesk-manufacturer.json:10. Suppliers and municipalities are imported as Active, and the existing OrganizationSyncService re-keys an organisation found by name slug, so a Fabrikant cell equal to a tenant's name can re-key that tenant (latent: the cron selects only actief).
• No actor-level audit record of an import — lib/Service/CmdbExportImportService.php:306. The only import-level log line names no user, file, municipality or updateExisting, so a bulk _rbac:false write leaves no record of who ran it.

🟢 Minor (17)

• Array-valued form fields become the string 'Array' — lib/Controller/CmdbImportController.php:141. An array-valued form field becomes the string Array through (string) casts.
• Upload error codes collapse into NO_FILE_UPLOADED / FILE_TOO_LARGE — lib/Controller/CmdbImportController.php:277. Every PHP upload error except size collapses into NO_FILE_UPLOADED, so a server-side tmp-dir failure reads as user error.
• openapi.json: operationId rules and security scheme missing — openapi.json:106. openapi.json lacks the operationId replace rule, a security scheme and cancel's 401.
• Controller unit tests miss param parsing, errors and logging — tests/Unit/Controller/CmdbImportControllerTest.php:281. The controller tests miss parameter parsing, the \Error path and the logging.
• Client-chosen operationId can overwrite a live operation — lib/Controller/CmdbImportController.php:153. A client-chosen operationId that is already live overwrites that operation's progress record and owner.
• Seed modules ship to every production register — lib/Settings/register.d/topdesk-cmdb-import.json:58. Three Voorbeeld seed modules in the register fragment ship to every production register.
• Cancel state: stale flag, cancel accepted after completion, early window — lib/Service/CmdbExportImportService.php:224. The cancel state is stale after completion, and the id regex uses $, so cmdb-12345678\n matches (use \z or D).
• Key normalisation: case, NBSP, key claimed before the Soort check — lib/Service/CmdbExportImportService.php:575. Middel-ID keys are case- and NBSP-sensitive, a Soort-skipped row consumes its id, and soft-deleted modules are re-created.
• municipalityByUuid turns any failure into MUNICIPALITY_INVALID — lib/Service/CmdbExportImportService.php:1035. municipalityByUuid maps every failure, infrastructure errors included, to MUNICIPALITY_INVALID.
• Report is stored whole in the distributed cache — lib/Service/CmdbExportImportService.php:510. The whole report, which can be large, is stored in the distributed cache.
• Profile allowlist and pack nits — lib/Settings/cmdb-import/topdesk-usage.json:36. Pack nits: cluster lost, Einddatum mapped but unused, registeredBy not set.
• Requirement headings use the REQ-ID prefix form, not ADR-038 Form A — openspec/changes/cmdb-export-import/specs/cmdb-export-import/spec.md:16. The 14 requirement headings use the ### Requirement: REQ-… prefix form, not ADR-038's canonical form.
• Repo text points readers at a private plan folder — openspec/changes/cmdb-export-import/tasks.md:11. tasks.md and other repo text point readers at a private plan folder that contributors cannot open.
• t() with variables HTML-escapes values that Vue escapes again — src/views/settings/sections/CmdbImport.vue:503. t() with variables HTML-escapes the values and Vue escapes them again, so & shows as & (the same pattern exists at 37 other places in src/).
• Report headings skip a level; file label looks like a drop zone — src/views/settings/sections/CmdbImport.vue:204. The report headings skip from h2 to h4, and the file label is styled like a drop zone that does not accept drops.
• E2E leans on vue-select internals and skips instead of failing — tests/e2e/spec-coverage/cmdb-import.spec.ts:190. The e2e spec relies on vue-select internals (vs__selected, nth(1)) and skips instead of failing when its precondition is missing.
• Newman folder gaps and a dead cmdb_operation_id variable — postman/stackiq-tests.json (the folder sits at line ~22,547; no diff anchor close enough, so it is listed here). Folder 12 - CMDB import misses 401, 400 NO_FILE_UPLOADED/NOT_XLSX, 422 MUNICIPALITY_REQUIRED and cancel's 403/412/200. The 200 request stores cmdb_operation_id, but the cancel test uses a hard-coded id, and the 413 test is skipped silently when cmdb_oversized_file is unset.

✅ Verified clean

• XXE and entity expansion are blocked: a package with a DOCTYPE entity was refused. Formula cells yield only the cached value (setReadDataOnly), and nothing is evaluated.
• The profile loader is safe against path traversal (basename guard), and a municipality uuid must resolve to an object of type Municipality.
• A key row cannot match an object in another register or schema, and @self, owner, organisation or id cannot be set from a cell (idStrategy: generate, id unset).
• Error envelopes carry only the code, a translated message and server-known details: no exception message, path or trace. The operationId is regex-checked before it touches the cache. The upload's tmp file is never copied.
• The UI handles every error code the server emits; requests go through @nextcloud/axios (CSRF token sent); no v-html. l10n: 103 keys added to each of en.json/nl.json, with no one-sided key, and the Dutch is real.
• Fixture cell content is anonymised: all 224 shared strings and every cell of all 10 sheets hold only placeholders (see C3 for the metadata that is not).
• openapi.json field names, codes and shapes match CmdbImportReport::toArray() and the controller.

Scope decision: XL (54 files, 12,106 lines), analysed in three slices at 6ddc2943: (A) HTTP surface, (B) import engine, (C) frontend, l10n, docs, specs and fixtures, plus the persistence audit. Residual: PHPUnit, Newman and Playwright were not run (no vendor tree or Nextcloud here); B14 and the lifecycle part of B13 need a rig run; TopdeskCmdbFragmentTest, CmdbImportProfileTest and CmdbRowNormaliserTest were read by name only; design.md, proposal.md and test-plan.md were grepped, not read; accessibility (C16) is from code, not a browser.
Persistence audit: 6 COVERED, 7 PARTIAL, 0 MISSING. P1 and P2 are posted above. The other PARTIALs are folded into A1 (delegation), A11 (operationId), B6/B14 (_rbac:false scope), B10 (contacts) and B12 (free text in logs).
Jira context: WOO-586 — CMDB export (TOPdesk, xlsx) via Stackiq into OpenRegister (status: Actief)
CI: required quality / Hydra Gates is red (A1, B1), and Frontend Check (check:schema-l10n) is red (C1); both are green on development. Features Check is red with the same docs/features.json drift that development has (Features Extract red there), so it is not caused by this PR. PHPUnit is red on development too (bootstrap: OCP\EventDispatcher\Event not found).

[github-actions]

Quality Report — ConductionNL/stackiq @ a13bd3b

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
check-manifest		✅
check-vue-demi		✅
test-l10n		✅
format		❌
check-schema-l10n		❌
check-l10n-js		✅
composer			✅	✅ 130/130
npm			✅	✅ 807/807
app:check-code	⏭️
info.xml	✅
REUSE	❌
lockfile sync		✅
PHPUnit					❌
Newman					⏭️
Playwright					⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets development, so the suite is asked once per promotion into beta and main rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into development.
Hydra gates					❌

---

Quality workflow — 2026-10-01 16:43 UTC

Download the full PDF report from the workflow artifacts.

[github-actions]

Quality Report — ConductionNL/stackiq @ ea784a6

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
check-manifest		✅
check-vue-demi		✅
test-l10n		✅
format		✅
check-schema-l10n		✅
check-l10n-js		✅
composer			✅	✅ 130/130
npm			✅	✅ 807/807
app:check-code	⏭️
info.xml	✅
REUSE	❌
lockfile sync		✅
PHPUnit					❌
Newman					⏭️
Playwright					⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets development, so the suite is asked once per promotion into beta and main rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into development.
Hydra gates					✅

---

Quality workflow — 2026-10-01 20:36 UTC

Download the full PDF report from the workflow artifacts.

[github-actions]

Quality Report — ConductionNL/stackiq @ d16e271

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
check-manifest		✅
check-vue-demi		✅
test-l10n		✅
format		✅
check-schema-l10n		✅
check-l10n-js		✅
composer			✅	✅ 130/130
npm			✅	✅ 807/807
app:check-code	⏭️
info.xml	✅
REUSE	❌
lockfile sync		✅
PHPUnit					❌
Newman					⏭️
Playwright					⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets development, so the suite is asked once per promotion into beta and main rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into development.
Hydra gates					✅

---

Quality workflow — 2026-10-01 20:39 UTC

Download the full PDF report from the workflow artifacts.