feat: add built-in tools for creating and updating internal loops#1046
feat: add built-in tools for creating and updating internal loops#1046aj47 wants to merge 4 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
🤖 Augment PR SummarySummary: Adds built-in SpeakMCP settings tools to manage persisted “internal loops” (recurring task definitions) in the desktop app. Changes:
Why: Enables agents/users to discover, create, and update recurring task definitions via MCP tool calls instead of manually editing configuration. 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| } | ||
|
|
||
| const config = configStore.get() | ||
| const loops = config.internalLoops || [] |
There was a problem hiding this comment.
config.json is user-editable and is spread over defaults, so config.internalLoops could be non-array (e.g., null/object), which would crash on .find/spread. Consider guarding with Array.isArray(config.internalLoops) (defaulting to []) in list_loops, create_loop, and update_loop.
Severity: medium
Other Locations
apps/desktop/src/main/builtin-tools.ts:959apps/desktop/src/main/builtin-tools.ts:1108
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
|
augment review |
| description: "Task prompt to run for this loop", | ||
| }, | ||
| intervalMinutes: { | ||
| type: "number", |
There was a problem hiding this comment.
intervalMinutes is declared as type: "number" in the tool schema, but the handlers reject non-integers via Number.isInteger. Consider making the schema type: "integer" (and setting minimum: 1) so callers don’t send floats and immediately hit validation errors.
Severity: low
Other Locations
apps/desktop/src/main/builtin-tool-definitions.ts:301
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
|
|
||
| const now = Date.now() | ||
| const loop: InternalLoop = { | ||
| id: `loop_${now}_${Math.random().toString(36).substr(2, 9)}`, |
There was a problem hiding this comment.
The loop id generation uses Date.now() + Math.random() and substr, which can (rarely) collide or even produce an empty random suffix, breaking update_loop’s ability to uniquely identify loops. Consider using a UUID (or similar) to make IDs reliably unique and avoid deprecated string APIs.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
|
augment review |
| const name = args.name.trim() | ||
| const prompt = args.prompt.trim() | ||
|
|
||
| const existingLoop = loops.find((loop) => loop.name.toLowerCase() === name.toLowerCase()) |
There was a problem hiding this comment.
Since config.json is user-editable, internalLoops elements could be null/non-objects; accessing loop.name.toLowerCase() (and loop.id) would throw and crash the tool handler. Consider sanitizing/filtering entries in getInternalLoops so these handlers are resilient to malformed stored loop entries.
Severity: medium
Other Locations
apps/desktop/src/main/builtin-tools.ts:1114apps/desktop/src/main/builtin-tools.ts:1132
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
|
augment review |
1 participant
Summary
InternalLoopmodel andconfig.internalLoopsspeakmcp-settings:list_loopsspeakmcp-settings:create_loopspeakmcp-settings:update_loopTesting
pnpm --filter @speakmcp/desktop typecheck