feat: add managed preset agent marketplace#52
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a managed agent marketplace, allowing users to install curated agent presets with predefined configurations, workspaces, and persona files. Key features include per-agent skill scope management with a 6-skill limit, persona write protection for managed agents, and an unmanage flow to convert presets into custom agents. Feedback focuses on correcting absolute local file paths in the design documentation, refactoring duplicated skill scope normalization logic, and aligning the implementation with the design spec regarding empty skill scopes. Additionally, it is recommended to investigate the root cause of listener warnings in tests rather than simply increasing the limit.
| - [electron/utils/agent-config.ts](/Users/lsave/workspace/AI/ClawX/electron/utils/agent-config.ts) owns agent config reads, writes, and filesystem provisioning. | ||
| - [electron/services/agents/agent-runtime-sync.ts](/Users/lsave/workspace/AI/ClawX/electron/services/agents/agent-runtime-sync.ts) syncs GeeClaw-managed agent state back into `openclaw.json`. | ||
| - [src/pages/Agents/index.tsx](/Users/lsave/workspace/AI/ClawX/src/pages/Agents/index.tsx) already provides the main agent management UI. | ||
| - [src/pages/Chat/PersonaDrawer.tsx](/Users/lsave/workspace/AI/ClawX/src/pages/Chat/PersonaDrawer.tsx) is a second editing surface for persona files and must therefore respect managed restrictions too. |
There was a problem hiding this comment.
This design document contains absolute local file paths (e.g., /Users/lsave/...). These paths are specific to your local machine and will not work for other contributors. It also leaks information about your local environment setup. Please replace them with relative paths from the repository root to make the links useful for everyone.
There was a problem hiding this comment.
Fixed in 8252a45. The design spec now uses repo-relative markdown links and the example workspace paths no longer include absolute local machine paths.
| function normalizeSkillScope(scope: unknown): AgentSkillScope { | ||
| if (!scope || typeof scope !== 'object') { | ||
| return { mode: 'default' }; | ||
| } | ||
|
|
||
| const mode = (scope as { mode?: unknown }).mode; | ||
| if (mode !== 'specified') { | ||
| return { mode: 'default' }; | ||
| } | ||
|
|
||
| const rawSkills = Array.isArray((scope as { skills?: unknown[] }).skills) | ||
| ? (scope as { skills?: unknown[] }).skills ?? [] | ||
| : []; | ||
| const normalized = rawSkills | ||
| .filter((value): value is string => typeof value === 'string') | ||
| .map((value) => value.trim()) | ||
| .filter(Boolean); | ||
|
|
||
| if (new Set(normalized).size !== normalized.length) { | ||
| throw new Error('Specified skill scope must not contain duplicate skills'); | ||
| } | ||
| if (normalized.length === 0) { | ||
| throw new Error('Specified skill scope must contain at least 1 skill'); | ||
| } | ||
| if (normalized.length > 6) { | ||
| throw new Error('Specified skill scope must not contain more than 6 skills'); | ||
| } | ||
|
|
||
| return { mode: 'specified', skills: normalized }; | ||
| } |
There was a problem hiding this comment.
The normalizeSkillScope function here is nearly identical to the logic in electron/utils/agent-presets.ts. This code duplication can lead to maintenance challenges, as changes or bug fixes might not be applied consistently across both files.
Consider refactoring this logic into a single, shared utility function that can be imported and used in both agent-config.ts and agent-presets.ts to improve maintainability.
There was a problem hiding this comment.
Fixed in 8252a45. I extracted the shared specified-skill normalization into electron/utils/agent-skill-scope.ts and reused it from both agent-config.ts and agent-presets.ts.
electron/utils/agent-config.ts
Outdated
| if (normalized.length === 0) { | ||
| throw new Error('Specified skill scope must contain at least 1 skill'); | ||
| } |
There was a problem hiding this comment.
This function throws an error for a specified skill scope with an empty skills array. However, the design specification states that an empty Specified scope is invalid and should become Default.
To align with the specification and make the API more robust, please consider modifying this logic to return { mode: 'default' } instead of throwing an error when normalized.length is 0.
| if (normalized.length === 0) { | |
| throw new Error('Specified skill scope must contain at least 1 skill'); | |
| } | |
| if (normalized.length === 0) { | |
| return { mode: 'default' }; | |
| } |
There was a problem hiding this comment.
Not adopting this change. The current product rule keeps default and specified as explicit modes, so an empty specified list is invalid rather than being coerced back to default. I updated the design spec in 8252a45 to match the shipped behavior so the docs and validation stay aligned.
tests/unit/agent-presets.test.ts
Outdated
| }); | ||
|
|
||
| beforeAll(() => { | ||
| process.setMaxListeners(Math.max(originalMaxListeners, 30)); |
There was a problem hiding this comment.
Increasing the max event listeners limit can hide potential memory leaks or inefficient resource management in the code or test setup. While sometimes necessary for complex tests, it's generally better to investigate the root cause of the MaxListenersExceededWarning.
I recommend investigating why so many listeners are being created and refactoring if possible, rather than just increasing the limit.
There was a problem hiding this comment.
Fixed in 8252a45. I removed the test-side max-listener workaround and fixed the root cause by making the logger exit-handler registration idempotent, which eliminates the warning on this path.
lsave
changed the title
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a managed agent marketplace, allowing users to install curated agent presets with predefined workspaces, persona files, and skill scopes. It includes the backend logic for preset loading, installation, and enforcement of management rules (such as locked fields and skill floors), along with a new marketplace UI and updates to the agent settings and persona drawer. Feedback is provided regarding a redundant fallback for the workspace path during preset installation, as the workspace is already a validated requirement for presets.
| expandPath(newEntry.workspace || getDefaultWorkspacePathForAgent(nextId)), | ||
| preset.files, | ||
| ); | ||
| await provisionAgentFilesystem(config, newEntry); |
There was a problem hiding this comment.
The fallback getDefaultWorkspacePathForAgent(nextId) seems unnecessary here. The newEntry.workspace is derived from preset.meta.agent.workspace, which is validated as a required non-empty string in agent-presets.ts. Relying on the validation as the single source of truth and removing this fallback would make the code cleaner and more robust against future changes in validation logic.
await seedPresetFilesIntoWorkspace(
expandPath(newEntry.workspace),
preset.files,
);
1 participant
Summary:
Validation: