# Regression Review Report

**Branch**: feat/orchestrator-mode -> main
**Date**: 2026-03-27
**PR**: #123
**Commits**: 22 (7bbc8e6..3d07ac5)

## Issues in Your Changes (BLOCKING)

### MEDIUM

**Prompt limit removal in loop-manager creates inconsistency with MCP adapter schemas** - `src/services/loop-manager.ts:44-52`, `src/adapters/mcp-adapter.ts:46,90,144,171,235,288`
**Confidence**: 85%
- Problem: The service-level 4000-character prompt validation was removed entirely from `LoopManagerService.validateCreateRequest()` (commit `7cdc92c`: "remove arbitrary 16000-char service-level prompt limit"). However, the MCP adapter Zod schemas still enforce `.max(4000)` on prompt fields (`CreateLoopSchema`, `ScheduleLoopSchema`, `DelegateTaskSchema`, `ScheduleTaskSchema`, pipeline step schemas). This creates an inconsistency: prompts submitted via CLI (`beat loop create`) or internal service calls (e.g., `OrchestrationManagerService`) now have no upper bound, while MCP-submitted prompts remain capped at 4000 characters. The orchestrator prompt generated by `buildOrchestratorPrompt()` is approximately 2000-3000 chars, so this does not break the immediate use case. But the divergence means the same operation behaves differently depending on the entry point.
- Fix: Either (a) remove `.max(4000)` from the MCP Zod schemas to match the service layer change, or (b) add a documented comment in the MCP schemas explaining the intentional MCP-specific limit, or (c) reintroduce a higher limit (e.g., 16000) in the service layer as a safety net against unbounded input.

**`TEST_STANDARDS.md` contains stale positional constructor example** - `tests/TEST_STANDARDS.md:91`
**Confidence**: 82%
- Problem: The test standards document shows the old positional constructor pattern: `new TaskManagerService(eventBus, repository, logger, config)`. All actual constructors were migrated to deps-object pattern in this PR (commit `ffaad91`). Developers following the documented pattern will write code that fails to compile.
- Fix: Update the example in `tests/TEST_STANDARDS.md` line 91 to use the new deps-object pattern:
  ```typescript
  const taskManager = new TaskManagerService({ eventBus, logger, config, taskRepo: repository, outputCapture, outputRepository });
  ```

## Issues in Code You Touched (Should Fix)

### MEDIUM

**`withServices()` makes orchestrationService required -- could fail existing CLI commands** - `src/cli/services.ts:89-94`
**Confidence**: 83%
- Problem: The `withServices()` helper now resolves `orchestrationService` via `exitOnError()` (line 89-94). If the orchestration service fails to resolve (e.g., database migration v14 not yet applied, or a container registration error), ALL CLI commands that use `withServices()` will exit with "Failed to get orchestration service" -- not just orchestration commands. This affects `beat cancel`, `beat retry`, `beat resume`, `beat schedule create`, `beat loop create/cancel/pause/resume`, and `beat pipeline create`. The migration should auto-apply during bootstrap, but if bootstrap runs in `cli` mode with reduced initialization, this adds a new failure path to every mutation command.
- Fix: Make orchestrationService resolution optional:
  ```typescript
  const orchestrationServiceResult = container.get<OrchestrationService>('orchestrationService');
  const orchestrationService = orchestrationServiceResult.ok ? orchestrationServiceResult.value : undefined;
  ```
  And update the return type to `orchestrationService?: OrchestrationService`. Callers that need it (like `orchestrate cancel`) can check for undefined.

## Pre-existing Issues (Not Blocking)

No critical pre-existing issues found in the reviewed files.

## Suggestions (Lower Confidence)

- **`OrchestrationHandler.create()` still uses positional params** - `src/services/handlers/orchestration-handler.ts:37-42` (Confidence: 72%) -- All other handler `create()` methods were migrated to deps-object pattern in commit `ffaad91`, but `OrchestrationHandler.create()` still accepts 5 positional parameters: `(orchestrationRepo, loopRepo, database, eventBus, logger)`. This is a new file so there are no existing callers to break, but it is inconsistent with the convention established in the same PR.

- **State file orphaned on failed loop creation** - `src/services/orchestration-manager.ts:128-166` (Confidence: 65%) -- If `loopService.createLoop()` fails after the state file was written (line 108-117), the state file is orphaned on disk. The 7-day cleanup only targets completed/failed/cancelled orchestrations -- this one stays in PLANNING status forever (no completedAt timestamp). The file is small (~200 bytes) so this is not a resource concern, but it is a loose end.

- **Event count comment may drift** - `src/core/events/events.ts:5` (Confidence: 60%) -- Comment says "34 event types" after adding orchestration events. Manual event counts in comments tend to drift. Consider removing the count or adding a test that validates it.

## Summary

| Category | CRITICAL | HIGH | MEDIUM | LOW |
|----------|----------|------|--------|-----|
| Blocking | 0 | 0 | 2 | 0 |
| Should Fix | 0 | 0 | 1 | 0 |
| Pre-existing | 0 | 0 | 0 | 0 |

**Regression Score**: 8/10
**Recommendation**: APPROVED_WITH_CONDITIONS

### Previous Review Issues -- Resolution Status

The first regression review (prior to fix commits) identified 4 issues. Here is their resolution status:

1. **RESOLVED -- Orchestration PLANNING cancel gap** (was HIGH): Fixed in commit `c06fb38`. The `cancelOrchestration()` method now directly updates the DB for PLANNING-state orchestrations (lines 268-276) with an explanatory comment. The code matches the suggested fix exactly.

2. **RESOLVED -- `test:orchestration` not in `test:all`** (was MEDIUM): Fixed in commit `527cd4b`. The `test:all` script now includes `npm run test:orchestration` in the chain.

3. **PARTIALLY RESOLVED -- Loop prompt limit change** (was HIGH, now MEDIUM): Commit `7cdc92c` removed the service-level limit entirely rather than raising it to 16000. This eliminates the previous concern about a service-layer behavioral change (the limit was removed rather than raised). However, an inconsistency remains between the MCP schemas (still `.max(4000)`) and the now-unlimited service layer. Downgraded from HIGH to MEDIUM because the runtime behavior is consistent for MCP callers (limit still enforced at Zod layer) and the only gap is the CLI/internal path which was previously limited but rarely hit.

4. **NOT RESOLVED -- `withServices()` making orchestrationService required**: Still present at `src/cli/services.ts:89-94`. The `exitOnError` call will terminate the process for any CLI mutation command if the orchestration service cannot be resolved.

### Regression Checklist

- [x] No exports removed without deprecation -- `handleDetachMode` widened from sync `void` to async `Promise<void>`; export preserved; caller updated
- [x] Return types backward compatible -- Only additive changes to return types
- [x] Default values unchanged (or documented) -- New orchestrator defaults are additive; no existing defaults changed
- [x] Side effects preserved (events, logging) -- All existing event subscriptions and emissions preserved; 3 new orchestration event types added
- [x] All consumers of changed code updated -- All 9 constructor migrations (positional -> deps object) have updated call sites in source and tests
- [x] Migration complete across codebase -- All constructors/factory methods migrated; no stale positional call sites remain in source or test files
- [x] CLI options preserved or deprecated -- All existing CLI commands preserved; `orchestrate` subcommand added additively
- [x] API endpoints preserved or versioned -- All existing MCP tools preserved; 4 new orchestrator tools added additively
- [x] Commit messages match implementation -- All 22 commits match their implementations
- [ ] Breaking changes documented -- `tests/TEST_STANDARDS.md` not updated to reflect deps-object pattern

### Known Pitfall Verification

All 6 pitfalls from `.memory/knowledge/pitfalls.md` are in `src/services/handlers/loop-handler.ts` and `src/utils/git-state.ts`. The loop-handler was refactored (positional -> deps object) but the git-state-related fixes from PF-001 through PF-006 are untouched by this PR. Resolved pitfalls remain resolved.

### Conditions for Approval

1. Make `orchestrationService` optional in `withServices()` to avoid breaking existing CLI commands if the service fails to resolve
2. Update `tests/TEST_STANDARDS.md:91` to reflect the new deps-object constructor pattern
3. Decide on the MCP/service prompt limit inconsistency (remove MCP limit, or document the divergence)
