Code review — findings and fixes (commit 2b3900c)

All findings below have been fixed and pushed to the PR branch.

🔴 HIGH — _updateRole allows empty name to reach the API

File: lib/features/developer/services/mcp_tools.dart · _updateRole

The original code:

if (args.containsKey('name')) data['name'] = (args['name'] ?? '').toString();

If an agent passes name: "" explicitly, the empty string is forwarded to client.roles.update(...). The server would either accept a blank role name or return an opaque error. Fixed with an explicit guard:

if (args.containsKey('name')) {
  final name = (args['name'] ?? '').toString();
  if (name.isEmpty) return {'error': 'name cannot be empty'};
  data['name'] = name;
}

🟡 MEDIUM — _listPermissions unnecessarily marked async

File: lib/features/developer/services/mcp_tools.dart · _listPermissions

The handler never awaits anything but was declared async, causing an unnecessary microtask allocation on every call. Replaced with an expression body using Future.value(...), consistent with other pure handlers in the class (e.g. _openDm).

🟡 MEDIUM — No tests for new tools

File: test/features/developer/mcp_tools_test.dart (new)

No tests existed for the developer MCP layer. Added a test suite covering:

• Tool registration: all 10 new tools exist and are placed in the correct group (read vs manage)
• Schema validation: required-field arrays are correct (e.g. set_channel_permission requires channel_id, target_id, target_type)
• list_permissions handler: returns ok: true, includes all AccordPermission.all() entries with non-empty descriptions
• update_role validation: rejects name: "" and the no-fields-to-update path
• set_channel_permission: rejects invalid and empty target_type
• All network-bound manage tools: return Not connected when no AccordAuthLoggedIn state is present

ℹ️ LOW — Observation: _overwriteBrief skips AccordPermissionOverwrite deserialization (intentional)

channels.listOverwrites returns raw JSON without calling .deserializeArray(AccordPermissionOverwrite.fromJson). _overwriteBrief therefore reads the raw map directly. This is actually correct for round-trip consistency: AccordPermissionOverwrite.fromJson normalises the type field from "member" → "user" internally, but the MCP set_channel_permission tool accepts "member". By using the raw server response, list_channel_permissions output can be fed directly back into set_channel_permission. No change made, but worth documenting.

ℹ️ File size

mcp_tools.dart is now ~998 lines — just under the 1 000-line threshold. If future tool groups are added here, consider splitting the handlers into a separate _mcp_manage_handlers.dart mixin or part file.

Generated by Claude Code