add factory tools to mcp server

[j4ys0n]

No description provided.

[j4ys0n]

Automated review 🤖

Summary of Changes

Adds a full factory management surface (21 new tools) to the MCP server, covering factory config CRUD, run lifecycle (start/status/result/pause/resume/cancel), step inspection, and schedule management. The change follows the same structural patterns established by the existing workflow tools and includes a comprehensive test suite (test/factory-tools.test.ts, 754 lines) plus tool-coverage registration checks.

Architecture & Organization

• src/schemas.ts defines input schemas (exported) while src/tools.ts defines response-parsing schemas (module-private). This split is consistent with the existing workflow pattern and keeps the public API surface clean.
• waitForFactoryRunRecord in tools.ts mirrors waitForWorkflowRunRecord but introduces a FactoryRunPausedSignal error class to break out of the SSE consumer loop on run_paused — a clean, self-documenting control-flow mechanism.
• parseSseJsonData is a new shared helper extracted into tools.ts rather than being inlined; it is currently only used by the factory SSE path. If the workflow SSE path ever needs the same logic, a small refactor would be needed to avoid duplication.
• Response-record schemas in tools.ts duplicate structural definitions already present in schemas.ts (e.g., FactoryTransitionSchema, FactoryAgentRefSchema). The duplication is intentional (input vs. output shapes with .passthrough()) but worth a comment to prevent future confusion.

Key Changes & Positives

• 🟢 FactoryRunPausedSignal extends Error provides a typed, non-generic signal for the pause case, avoiding silent swallowing of unrelated errors in the catch block (tools.ts, waitForFactoryRunRecord).
• 🟢 mapFactoryRunResult throws descriptive, actionable errors for every non-completed status before returning data, preventing callers from silently receiving an empty or intermediate carryPayload.
• 🟢 FactoryScheduleCreateSchema and FactoryScheduleUpdateSchema use superRefine to enforce daysOfWeek/dayOfMonth co-constraints at parse time, surfacing errors before any network call.
• 🟢 Test suite covers SSE reconnect behavior (stream closes before completion), pause signal propagation, all error-state branches of msq_get_factory_result, and schedule validation — these are the highest-risk paths.

Potential Issues & Recommendations

1. Issue / Risk: waitForFactoryRunRecord can loop indefinitely if the SSE stream repeatedly closes without emitting a terminal or pause hint and the REST poll always returns queued/running (tools.ts, waitForFactoryRunRecord while-loop).

   • Impact: An MCP tool invocation could hang forever, blocking the calling agent with no timeout or retry cap.
   • Recommendation: Add a maximum iteration count or wall-clock deadline (matching the pattern used or considered for waitForWorkflowRunRecord) and surface a timeout error to the caller.
   • Status: 🔴 Problem

2. Issue / Risk: FactoryScheduleUpdateSchema validates repeatInterval === 'weekly' requires daysOfWeek, but only when repeatInterval is explicitly provided in the partial update. If a schedule already has repeatInterval: 'weekly' on the backend and the caller sends { id, daysOfWeek: [] } to clear days, the schema passes (no repeatInterval key present) and the backend receives an invalid state.

   • Impact: Partial updates that clear daysOfWeek on an existing weekly schedule bypass client-side validation silently.
   • Recommendation: Document this known limitation in the tool description, or add a note in README that clearing daysOfWeek on a weekly schedule is unsupported via this tool.
   • Status: 🟡 Needs review

3. Issue / Risk: msq_list_factory_runs passes args.limit ?? 20 and args.offset ?? 0 to the query, but both fields have .default(20) / .default(0) in FactoryRunsListSchema, so the ?? fallbacks are unreachable dead code (tools.ts, msq_list_factory_runs and msq_list_factory_run_steps).

   • Impact: No functional bug, but misleading — the schema default and the inline fallback can diverge if one is changed without updating the other.
   • Recommendation: Remove the ?? 20 / ?? 0 / ?? 50 fallbacks and rely solely on the Zod schema defaults, as done consistently elsewhere.
   • Status: 🟡 Needs review

4. Issue / Risk: FactoryUpdateSchema is defined as FactoryIdSchema.merge(FactoryCreateSchema.omit({ id: true }).partial()). Because steps becomes optional in the partial, a caller can send { id, steps: [] } — an empty array — which passes the FactoryUpdateSchema (no min(1) constraint on the partial field) but would likely be rejected or produce a broken factory on the backend.

   • Impact: A zero-step factory update passes client-side validation and reaches the API, potentially corrupting the factory config.
   • Recommendation: Add a .superRefine or explicit check: if steps is present in the update body, enforce steps.length >= 1.
   • Status: 🟡 Needs review

Tests

• The reconnect test (reconnects when the factory stream closes before completion) verifies the loop iterates twice but does not assert a bound on iterations — consistent with the infinite-loop risk noted above. Consider adding a test that asserts the tool eventually times out or errors after N reconnects.
• msq_delete_factory_schedule test asserts result === { success: true } but the mock returns jsonResponse({ success: true }) without a data field; the tool passes the raw response through client.requestJson directly (no parse step), so this is correct — but worth a comment since it differs from every other schedule tool.

Approval Recommendation

Approve with caveats

• Address the unbounded waitForFactoryRunRecord loop (no timeout/max-retry guard) before shipping to production — this is the only blocking concern.
• Decide on and document the FactoryUpdateSchema empty-steps edge case.
• Remove the unreachable ?? default fallbacks in msq_list_factory_runs and msq_list_factory_run_steps.

[j4ys0n]

Automated review 🤖

Summary of Changes

Adds 20 new MCP tools covering the full factory lifecycle (CRUD, run management, step inspection, schedule management) across src/schemas.ts, src/tools.ts, test/factory-tools.test.ts, and test/tool-coverage.test.ts. The version is bumped from 0.4.2 to 0.5.0. This is a large, additive feature with no breaking changes to existing tools.

Architecture & Organization

• src/schemas.ts defines input/validation schemas (Zod, with superRefine cross-field validation for schedules) while src/tools.ts defines separate response-parsing schemas and mapper functions — this mirrors the existing workflow pattern and is consistent throughout.
• Response schemas in tools.ts are all passthrough(), correctly tolerating unknown backend fields without breaking on API additions.
• FactoryRunPausedSignal extends Error (tools.ts) is a clean sentinel for SSE control flow, avoiding silent swallowing of unrelated errors.
• assertValidEffectiveFactoryScheduleUpdate fetches all schedules to find the existing one by id rather than fetching a single schedule by id — this is a minor inefficiency but acceptable if no single-schedule GET endpoint exists.
• parseSseJsonData is a new shared utility that could benefit the existing workflow SSE path too, but its introduction here is scoped correctly.

Key Changes & Positives

• 🟢 FactoryScheduleCreateSchema uses superRefine for cross-field validation (weekly requires daysOfWeek, monthly requires dayOfMonth), and assertValidEffectiveFactoryScheduleUpdate re-applies the same logic for updates against the merged effective state — prevents invalid schedules from reaching the API.
• 🟢 waitForFactoryRunRecord correctly distinguishes FactoryRunPausedSignal from other errors, re-throwing non-pause errors rather than silently swallowing them.
• 🟢 SSE reconnect loop (waitForFactoryRunRecord) handles stream closure without a terminal hint by continuing to poll, matching the behavior described in the README.
• 🟢 mapFactoryRunResult throws descriptive, actionable errors for non-completed states (queued, running, paused, error, cancelled) rather than returning ambiguous data.
• 🟢 Test suite (test/factory-tools.test.ts) covers all 20 tools including SSE reconnect, pause signal, schedule validation, and error-state result fetching — 774 lines of meaningful coverage.

Potential Issues & Recommendations

1. Issue / Risk: waitForFactoryRunRecord in tools.ts has a logic gap: after the SSE stream closes and the re-fetched record is still in a waiting status, the while loop continues only if sawPauseHint || sawTerminalHint is true — but if neither flag is set (e.g., a heartbeat-only stream), the loop body re-enters and opens a new SSE connection, which is the intended reconnect behavior. However, the continue at the end of the if (sawPauseHint || sawTerminalHint) block is unreachable dead code: the while condition re-evaluates regardless, so the continue adds no effect and may confuse future readers.

   • Impact: No functional bug, but the continue is misleading and could cause confusion during maintenance.
   • Recommendation: Remove the if (sawPauseHint || sawTerminalHint) { continue } block entirely; the while loop already handles re-entry.
   • Status: 🟡 Needs review

2. Issue / Risk: msq_update_factory_schedule in tools.ts fetches all schedules (fetchFactorySchedules) and finds the target by id only when repeatInterval, daysOfWeek, or dayOfMonth are present in the update. If the existing schedule is not found in the list (e.g., pagination, deleted concurrently), existing is null and assertValidEffectiveFactoryScheduleUpdate uses only the update fields — meaning a partial update that clears daysOfWeek on a weekly schedule could pass validation if the schedule isn't in the list response.

   • Impact: A weekly schedule could be updated to have no daysOfWeek if the schedule list is paginated or the schedule is missing, bypassing the guard.
   • Recommendation: If a single-schedule GET endpoint exists, prefer fetching by id. Otherwise, document the null fallback behavior and consider adding a warning log or explicit error when existing === null and the effective interval is ambiguous.
   • Status: 🟡 Needs review

3. Issue / Risk: FactoryUpdateSchema is defined as FactoryIdSchema.merge(FactoryCreateSchema.omit({ id: true }).partial()) in schemas.ts. This means steps in an update is z.array(...).min(1).max(50).optional() — the min(1) constraint from FactoryCreateSchema is preserved even in the partial update context, so sending steps: [] is correctly rejected. However, the tool description says "send the full desired step array" but the schema allows omitting steps entirely, which is correct for partial updates. No issue, but worth confirming the backend treats a missing steps field as a no-op rather than clearing steps.

   • Impact: If the backend interprets a missing steps as "clear all steps," callers could accidentally delete steps by omitting the field.
   • Recommendation: Confirm backend PUT semantics for steps and add a note to the tool description if omitting steps is a no-op vs. a clear.
   • Status: 🟡 Needs review

Tests

• test/factory-tools.test.ts covers the SSE reconnect path (heartbeat-only stream → second stream with terminal event) but does not test the scenario where fetchFactoryRunRecord throws during the SSE wait loop (e.g., network error mid-stream). Given the SSE error re-throw logic, a test confirming non-FactoryRunPausedSignal errors propagate correctly would strengthen confidence.

Approval Recommendation

Approve with caveats

• Remove or clarify the unreachable if (sawPauseHint || sawTerminalHint) { continue } block in waitForFactoryRunRecord
• Confirm backend PUT semantics for steps on msq_update_factory (no-op vs. clear when omitted) and update the tool description accordingly
• Assess whether the existing === null fallback in assertValidEffectiveFactoryScheduleUpdate is acceptable given potential list pagination, and document or harden as needed