[julien-nc]

Log any admin setting change with this message:
The assistant admin setting "%s" was changed from "%s" to "%s" by "%s"

Support the new operation name that appeared in 36.
On Nextcloud 35, the extra constructor argument is silently ignored by PHP.

🤖 AI (if applicable)

• The content of this PR was partly or fully generated using AI (N/A)

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 45 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: f52912e6-b04f-4e83-bb6c-e15a876617d8

Commits

Reviewing files that changed from the base of the PR and between 32a9033 and b04de04.

Files selected for processing: 1

• lib/Controller/ConfigController.php

Walkthrough

ConfigController now receives an event dispatcher. It compares existing and requested values for admin settings and the global skills folder. When a value changes, it dispatches a critical-action audit event with change details. Unchanged values do not trigger an audit event. The tests cover configuration persistence, audit parameters, and unchanged-value behavior.

Merge Risk: Low · previous review 32a90

The change is mergeable with owner awareness of two bounded risks: concurrent admin updates can produce inaccurate audit history, and an audit-listener failure can leave a settings update partially applied. The suggested tests would strengthen regression protection.

Pre-merge checks: 5

Check name	Status	Explanation
Title check		The title clearly and concisely describes the main change: adding audit logs for admin setting changes.
Description check		The description directly explains the audit-log message and compatibility-related constructor change. It is related to the changeset.
Docstring Coverage		Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 2 functions across 2 files.

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Tip

🎯 Customize your risk assessments

Guide how CodeRabbit assesses merge risk and security architecture risk. Highlight sensitive changes and explain your security boundaries using Merge Risk Instructions and Security Risk Instructions in your review settings. Security architecture risk assessments require the Advanced plan or above.

✅ Automatic approvals, with your rules

Let CodeRabbit approve pull requests once required threads are resolved, pre-merge checks pass, and your risk limits are met. Enable Automatic approvals in review settings, with overrides for teams or file paths.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[coderabbitai]

Actionable comments posted: 2

Nitpick comments: 3

tests/unit/Controller/ConfigControllerTest.php: 3

63-66: Trivial · Maintainability & Code Quality · Quick win

Assert the lazy argument for the chat_last_n_messages reads.

mockAppConfig() ignores the $lazy argument of hasKey() and getValueString(). The getValueString() stub returns the seeded value for any lazy flag. The audit test for chat_last_n_messages checks only that oldValue is '10'. If the controller read this key with the wrong lazy flag, that test would still pass.

Add an expectation that getValueString receives lazy: true for chat_last_n_messages. Alternatively, make the stub return the default when the lazy flag does not match the key's stored flag.

---

101-109: Trivial · Maintainability & Code Quality · Quick win

Add assertions for the audit message and operation in the ConfigController tests.

The audit tests check only getParameters(). A change to the message text or to either operation string would still pass the suite. Assert getMessage() in testSetAdminConfigAuditLogsChangedValues and testSetGlobalSkillsFolderAuditLogsChange. Assert the operation only after confirming that getOperation() exists on the CriticalActionPerformedEvent version the test suite loads. The controller docblock says the operation argument exists only since Nextcloud 36, so the pinned stubs may not provide that accessor.

---

185-192: Trivial · Maintainability & Code Quality · Quick win

Cover the same path under a different admin.

testSetGlobalSkillsFolderDoesNotAuditUnchangedValue stores the current admin, so it cannot detect an early return that compares only path. Such a regression would skip the service update and the audit event when the admin changes but the path stays the same.

Add a test that stores other as the admin and skills as the path. The test then calls setGlobalSkillsFolder('skills') as admin. It asserts one setGlobalSkillsFolder('admin', 'skills') call and one audit event with oldAdminUid other and newAdminUid admin.

Suggested test

 		self::assertCount(0, $this->auditEvents);
 	}
+
+	public function testSetGlobalSkillsFolderUpdatesSamePathOfOtherAdmin(): void {
+		$this->agentSkillsService->method('getGlobalSkillsConfig')
+			->willReturn(['admin_uid' => 'other', 'path' => 'skills']);
+		$this->agentSkillsService->expects($this->once())
+			->method('setGlobalSkillsFolder')
+			->with('admin', 'skills');
+
+		$this->controller->setGlobalSkillsFolder('skills');
+
+		$this->assertCount(1, $this->auditEvents, 'Should audit the owner change');
+		self::assertSame(
+			[
+				'user' => 'admin',
+				'oldPath' => 'skills',
+				'oldAdminUid' => 'other',
+				'newPath' => 'skills',
+				'newAdminUid' => 'admin',
+			],
+			$this->auditEvents[0]->getParameters(),
+		);
+	}

Review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 54d0e46b-ec8f-487d-88b6-b6e0285bb5d2

Commits

Reviewing files that changed from the base of the PR and between 0af038c and 32a9033.

Files selected for processing: 2

• lib/Controller/ConfigController.php
• tests/unit/Controller/ConfigControllerTest.php

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

[coderabbitai]

Minor · Functional Correctness · Poor tradeoff

Supported by static analysis

Script executed:

rg -n 'function setValueString|function getValueString|setAdminConfig|transaction|lock' lib/Controller/ConfigController.php vendor/nextcloud/ocp tests/unit/Controller/ConfigControllerTest.php 2>/dev/null | head -n 100

Repository: nextcloud/assistant

Length of output: 12653

---

Script executed:

nl -ba lib/Controller/ConfigController.php | sed -n '1,160p'
rg -n -F -e 'function audit' -e 'AuditTrait' -e 'IAuditLogger' -- lib/ || test "$?" -eq 1
nl -ba vendor/nextcloud/ocp/OCP/IAppConfig.php | sed -n '150,320p'

Repository: nextcloud/assistant

Length of output: 13568

---

Serialize the old-value read with the write.

setAdminConfig() reads oldValue at lines 74-76 and writes the setting at line 77. No lock or transaction covers both calls. If two requests for the same key overlap, both can read the same predecessor. The audit entry then records that stale value. For example, if A and B both read 10, B writes 7, and A writes 5, the log records 10 to 5 instead of 7 to 5. Make the read and the write one atomic operation, for example with a database transaction and a row lock, and log the value that operation replaced.

[marcelklehr]

Just making sure we have no credential-like settings that would get logged here...

[julien-nc]

Yeah for now we don't.