refactor(ai): extract AiAuditService from AiService — clears the last phpmd finding by rubenvdlinde · Pull Request #722 · ConductionNL/procest

rubenvdlinde · 2026-08-04T08:58:25Z

What

ConductionNL/procest had exactly one phpmd finding left on development:

lib/Service/AiService.php:62  TooManyPublicMethods
The class AiService has 12 public methods. Consider refactoring AiService to keep number of public methods under 10.

This clears it with a real refactor — no suppression, no threshold change, no baseline.

How

AiService was 845 lines doing two jobs. The three oversight methods move into a new OCA\Procest\Service\Ai\AiAuditService:

recordUserAction()
listAuditEntries()
recordAssistantAuditEntry()

That leaves AiService doing model orchestration only (is the feature on, build the prompt, scrub PII, make the one outbound guarded call), while "what did a human do with this suggestion" and "read the Algoritmeregister trail back" live in AiAuditService. Storage stays where it already was, in AiAuditLog — this is a surface split, not a second sink. 12 → 9 phpmd-counted public methods.

The model identifier (<type>/<name>) is needed by both halves, so instead of duplicating the config lookup it moves into a small AiModelIdentity, injected into each. Behaviour unchanged.

Note: TooManyPublicMethods ignores get*/set*/is*/has*/with* by default, so isEnabled, isFeatureEnabled and getAiSettings were never counted — making them private would have moved the number by zero.

Call sites updated

File	Change
`lib/Controller/AiController.php`	takes both services (`recordUserAction` + `listAuditEntries` → audit; 6 AI ops stay)
`lib/Controller/AiAuditExportController.php`	audit-only, swapped outright
`lib/Service/Assistant/CaseAssistantService.php`	audit-only, swapped outright
`lib/Service/WOOAnonymisationAssistService.php`	needs both — also uses `detectDeterministicPiiSpans`

No DI registration change: these services are autowired; nothing in lib/AppInfo/ references any Ai* class.

Also fixed

Dropped a dead @SuppressWarnings(PHPMD.ExcessiveParameterList) that sat on recordUserAction. The method has 7 parameters against a threshold of 10 — it suppressed nothing. Verified by removing it and re-running phpmd. Net: this PR removes one suppression annotation and adds none.

Verification

Measured in a worktree pinned to origin/development @ 6d650bec9, phpmd over 595 files in lib/, PHP 8.4 container (host PHP is 8.2 and dies in platform_check.php with exit 255, which greps as a clean run).

	before	after
phpmd	exit 2, 1 finding	exit 0, 0 findings
positive control (injected probe file)	exit 2, 3 probe findings	exit 2, 3 probe findings

The positive control is the point: a clean phpmd run and a phpmd that never started look identical, so both arms were checked with a deliberately-dirty probe file.

composer check:strict, real exit codes (not tail's):

step	before	after
lint	0	0
phpcs	0 (501 warnings)	0 (501 warnings)
phpmd	2	0
psalm	0	0
phpstan	0	0
phpunit	0 — 1686 tests, 5632 assertions, 10 deprecations, 5 skipped	0 — 1691 tests, 5649 assertions, 10 deprecations, 5 skipped

Intermediate runs went through 38 errors → 23 errors → 0, so the suite demonstrably responds to this tree rather than passing by inertia.

New tests

The extracted model-identifier logic had no test anywhere in the repo, so it would have broken silently. Added:

AiModelIdentityTest — configured <type>/<name>, and the local/unknown fallback an unconfigured instance relies on
AiAuditServiceRecordTest — recordUserAction() stamps the model + normalises null actualValue/reason; recordAssistantAuditEntry() forwards verbatim

Mutation-checked: flipping the / separator in AiModelIdentity turns 3 of the new tests red, so they assert something.

Not done

No @SuppressWarnings, @psalm-suppress, phpcs:ignore, phpstan-ignore, markTestSkipped or .skip( added
No baseline file created (there is none in this repo)
phpmd.xml, phpcs.xml, psalm.xml, phpstan.neon, phpunit.xml, composer.json untouched
No behaviour change — methods moved, not redesigned

@SuppressWarnings

…licMethods) AiService carried 12 phpmd-counted public methods against a threshold of 10 — the one remaining phpmd finding on development. Move the three oversight methods (recordUserAction, listAuditEntries, recordAssistantAuditEntry) into a new OCA\Procest\Service\Ai\AiAuditService. That is a coherent responsibility split rather than a shuffle: AiService now does model orchestration only (is the feature on, build the prompt, scrub PII, make the one outbound call), while recording what a human did with a suggestion and reading the Algoritmeregister trail back live in AiAuditService. Storage stays where it already was, in AiAuditLog. 12 -> 9 counted methods. The model identifier (`<type>/<name>`) was needed by both halves, so rather than duplicate the config lookup it moves to a small AiModelIdentity, injected into each. Behaviour is unchanged. Call sites updated: AiController (now takes both services), AiAuditExportController and CaseAssistantService (audit-only, swapped outright), WOOAnonymisationAssistService (needs both — it also uses detectDeterministicPiiSpans). No DI registration change: these services are autowired, not bound in ServiceRegistrar. Also drops a dead @SuppressWarnings(PHPMD.ExcessiveParameterList) that was sitting on recordUserAction — the method has 7 parameters against a threshold of 10, so the annotation suppressed nothing. Verified by removing it and re-running phpmd. The extracted model-identifier logic had no test anywhere, so add coverage for AiModelIdentity and for AiAuditService's two write paths. Mutation-checked: flipping the `/` separator turns 3 of the new tests red. No suppression, threshold change or baseline was introduced; phpmd.xml, phpcs.xml, psalm.xml, phpstan.neon and phpunit.xml are untouched. phpmd on lib/ (595 files): exit 2 / 1 finding -> exit 0 / 0 findings. Positive-controlled both sides (injected probe yields 3 findings, exit 2). check:strict all green: lint 0, phpcs 0, phpmd 0, psalm 0, phpstan 0, phpunit 0 (1686 -> 1691 tests, 5632 -> 5649 assertions, 5 skipped unchanged).

CI ran red on all 4 PHPUnit legs with 7 RISKY tests (failOnRisky=true): This test executed code that is not listed as code to be covered or used: - OCA\Procest\Service\Ai\AiModelIdentity phpunit.xml sets beStrictAboutCoverageMetadata="true", so a suite carrying @Covers must declare every class it actually executes. AiService now composes AiModelIdentity, so both suites that build a real AiService execute it. This was invisible locally because the check only fires when a coverage driver is present, and the local container has none — the gate's ABSENCE looked exactly like its success. Reproduced by building a container with Xdebug (CI uses Xdebug, not PCOV — PCOV does not drive this check at all) and running the same command CI runs. Positive-controlled: with the @uses line removed the run is exit 1 / 4 risky; with it restored, exit 0 / 0 risky.

github-actions · 2026-08-04T09:17:03Z

Quality Report — ConductionNL/procest @ `172636c`

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
composer			✅	✅ 100/100
npm			✅	✅ 550/550
PHPUnit					❌
Newman					⏭️
Playwright					❌

Quality workflow — 2026-08-04 09:17 UTC

Download the full PDF report from the workflow artifacts.

github-actions · 2026-08-04T09:33:22Z

Quality Report — ConductionNL/procest @ `f527af4`

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
composer			✅	✅ 100/100
npm			✅	✅ 550/550
PHPUnit					✅
Newman					⏭️
Playwright					✅

Quality workflow — 2026-08-04 09:33 UTC

Download the full PDF report from the workflow artifacts.

…figuration() (#725) phpmd on development flagged the last remaining finding: lib/Service/SettingsService.php:531 ExcessiveMethodLength The method loadConfiguration() has 102 lines of code. Current threshold is set to 100. It crossed the line in #721, which replaced the `+frag.<md5>` version suffix with a long explanatory comment. loadConfiguration() was doing two jobs: resolving the effective register configuration (read procest_register.json, parse it, deep-merge the ADR-037 register.d fragments) and importing it. Extract the first into a private readEffectiveConfiguration() returning ['data' => …] or ['error' => …], so loadConfiguration() stays a single import flow. Behaviour is unchanged — the same two failure shapes are returned to the caller in the same order. The extracted path had NO test: the only loadConfiguration test covers the OpenRegister-unavailable early return, which returns before any of this runs, so read/parse/merge could have broken silently. Add a test driving loadConfiguration() through to importFromApp, asserting it receives the on-disk version and parsed content, and that the version carries no `+` build suffix (the #721 invariant). Mutation-checked: returning an empty config array turns it red (0.13.2 -> 0.0.0). phpmd lib/ (597 files): exit 2 / 1 finding -> exit 0 / 0 findings. Positive-controlled both arms (injected probe yields 3 findings, exit 2). check:strict green: lint 0, phpcs 0, phpmd 0, psalm 0, phpstan 0, phpunit 0 (1691 -> 1692 tests, 5649 -> 5659 assertions). PHPUnit was run under Xdebug, because phpunit.xml sets beStrictAboutCoverageMetadata + failOnRisky and that check is INERT without a coverage driver — it silently caught nothing locally on #722 and only fired in CI. It flagged the missing @uses RegisterFragmentMerger here immediately.

rubenvdlinde requested review from WilcoLouwerse, bbrands02 and rjzondervan as code owners August 4, 2026 08:58

rubenvdlinde merged commit 773a8bf into development Aug 4, 2026
33 checks passed

rubenvdlinde deleted the chore/split-ai-audit-service branch August 4, 2026 09:33

rubenvdlinde mentioned this pull request Aug 4, 2026

refactor(settings): extract readEffectiveConfiguration() — clears the last phpmd finding #725

Merged

Navigation Menu

Uh oh!

refactor(ai): extract AiAuditService from AiService — clears the last phpmd finding - #722

refactor(ai): extract AiAuditService from AiService — clears the last phpmd finding#722
rubenvdlinde merged 2 commits into
developmentfrom
chore/split-ai-audit-service

rubenvdlinde commented Aug 4, 2026

Uh oh!

github-actions Bot commented Aug 4, 2026

Uh oh!

github-actions Bot commented Aug 4, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

rubenvdlinde commented Aug 4, 2026

What

How

Call sites updated

Also fixed

Verification

New tests

Not done

Uh oh!

github-actions Bot commented Aug 4, 2026

Quality Report — ConductionNL/procest @ 172636c

Uh oh!

github-actions Bot commented Aug 4, 2026

Quality Report — ConductionNL/procest @ f527af4

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Quality Report — ConductionNL/procest @ `172636c`

Quality Report — ConductionNL/procest @ `f527af4`