Merge pull request #66 from jerfowler/issue-60-step-count-validation

feat: add stepCount parameter for plan validation (Issue #60)

Author: jerfowler

src/tools/report-progress.ts

@@ -2,17 +2,21 @@
  * Report progress tool - Progress updates without file exposure
  * Updates progress markers without exposing file operations
  * Enhanced with context status tracking (Issue #51)
+ * Enhanced with stepCount metadata usage (Issue #60)
  */
 
 import { ServerConfig } from '../types.js';
 import { TaskContextManager, ProgressUpdate, ProgressReportResult } from '../core/TaskContextManager.js';
 import { validateRequiredString, validateRequiredConfig } from '../utils/validation.js';
 import { ErrorLogEntry } from '../logging/ErrorLogger.js';
 import type { ContextStatus, CapabilityChanges } from '../types/context-types.js';
+import { parsePlanCheckboxes } from '../utils/plan-parser.js';
+import { PlanMetadata } from '../types/plan-metadata.js';
+import * as fs from '../utils/fs-extra-safe.js';
+import path from 'path';
 import debug from 'debug';
 
-
-const log = debug('agent-comm:tools:reportprogress');
+const log = debug('agent-comm:tools:report-progress');
 /**
  * Report progress updates without file exposure
  */
@@ -245,31 +249,8 @@ export async function reportProgress(
       // Continue processing - don't throw
     }
 
-    // Log extremely large step numbers but don't block (for resilience)
-    if (step > 100) {
-      log('Warning: Extremely large step number detected: %d at index %d', step, index);
-      // Log unusual condition for analysis but don't block
-      if (config.errorLogger) {
-        const errorEntry: ErrorLogEntry = {
-          timestamp: new Date(),
-          source: 'validation',
-          operation: 'report_progress',
-          agent,
-          ...(taskId && { taskId }),
-          error: {
-            message: `Extremely large step number detected: ${step} at index ${index}`,
-            name: 'ValidationWarning'
-          },
-          context: {
-            tool: 'report_progress',
-            parameters: { unusualStep: step, typicalMax: 100, updateIndex: index }
-          },
-          severity: 'medium' // Not blocking, just unusual
-        };
-        await config.errorLogger.logError(errorEntry);
-      }
-      // Continue processing - don't throw
-    }
+    // Note: Extremely large step number validation moved to after stepCount determination
+    // to avoid double logging when step is both large and out of range
 
     if (typeof status !== 'string' || !['COMPLETE', 'IN_PROGRESS', 'PENDING', 'BLOCKED'].includes(status)) {
       const error = new Error(`Update at index ${index}: status must be one of COMPLETE, IN_PROGRESS, PENDING, BLOCKED`);
@@ -330,7 +311,136 @@ export async function reportProgress(
       ...(typeof blocker === 'string' && blocker.trim() && { blocker: blocker.trim() })
     });
   }
-  
+
+  // Validate step numbers against stepCount if metadata exists (Issue #60)
+  try {
+    const startTime = Date.now();
+    const taskPath = path.join(config.commDir, agent, taskId ?? 'current-task');
+    const metadataPath = path.join(taskPath, 'PLAN.metadata.json');
+
+    let stepCount: number | undefined;
+
+    // Try to read metadata first for performance
+    if (await fs.pathExists(metadataPath)) {
+      try {
+        const metadata = await fs.readJSON(metadataPath) as PlanMetadata;
+        stepCount = metadata.stepCount;
+        log('Using cached stepCount from metadata: %d', stepCount);
+      } catch (error) {
+        log('Failed to read metadata, falling back to plan parsing: %s', (error as Error).message);
+      }
+    }
+
+    // Fall back to parsing PLAN.md if no metadata
+    if (stepCount === undefined) {
+      const planPath = path.join(taskPath, 'PLAN.md');
+      if (await fs.pathExists(planPath)) {
+        const planContent = await fs.readFile(planPath, 'utf8');
+        const checkboxes = parsePlanCheckboxes(planContent);
+        stepCount = checkboxes.length;
+        log('Parsed stepCount from PLAN.md: %d', stepCount);
+      }
+    }
+
+    const validationTime = Date.now() - startTime;
+    log('Step validation completed in %dms', validationTime);
+
+    if (validationTime > 10) {
+      log('PERFORMANCE WARNING: Step validation took %dms (>10ms threshold)', validationTime);
+    }
+
+    // Validate all step numbers are within range and check for extremely large steps
+    if (stepCount !== undefined) {
+      for (const update of updates) {
+        if (update.step > stepCount) {
+          // Step is out of range - this takes priority over "extremely large" warning
+          const errorMessage = `Step ${update.step} is out of range (max: ${stepCount})`;
+
+          if (config.errorLogger) {
+            const errorEntry: ErrorLogEntry = {
+              timestamp: new Date(),
+              source: 'validation',
+              operation: 'report_progress',
+              agent,
+              ...(taskId && { taskId }),
+              error: {
+                message: errorMessage,
+                name: 'StepOutOfRangeWarning',
+                code: 'STEP_OUT_OF_RANGE'
+              },
+              context: {
+                tool: 'report_progress',
+                parameters: {
+                  invalidStep: update.step,
+                  maxStep: stepCount
+                }
+              },
+              severity: 'medium'
+            };
+            await config.errorLogger.logError(errorEntry);
+          }
+
+          // Log warning but continue processing (permissive handling)
+          log('Warning: %s', errorMessage);
+        } else if (update.step > 100) {
+          // Step is within range but extremely large - log separately
+          log('Warning: Extremely large step number detected: %d', update.step);
+          if (config.errorLogger) {
+            const errorEntry: ErrorLogEntry = {
+              timestamp: new Date(),
+              source: 'validation',
+              operation: 'report_progress',
+              agent,
+              ...(taskId && { taskId }),
+              error: {
+                message: `Extremely large step number detected: ${update.step}`,
+                name: 'ValidationWarning'
+              },
+              context: {
+                tool: 'report_progress',
+                parameters: { unusualStep: update.step, typicalMax: 100, maxStep: stepCount }
+              },
+              severity: 'medium'
+            };
+            await config.errorLogger.logError(errorEntry);
+          }
+        }
+      }
+    } else {
+      // No stepCount available, fall back to extremely large step validation only
+      for (const update of updates) {
+        if (update.step > 100) {
+          log('Warning: Extremely large step number detected: %d', update.step);
+          if (config.errorLogger) {
+            const errorEntry: ErrorLogEntry = {
+              timestamp: new Date(),
+              source: 'validation',
+              operation: 'report_progress',
+              agent,
+              ...(taskId && { taskId }),
+              error: {
+                message: `Extremely large step number detected: ${update.step}`,
+                name: 'ValidationWarning'
+              },
+              context: {
+                tool: 'report_progress',
+                parameters: { unusualStep: update.step, typicalMax: 100 }
+              },
+              severity: 'medium'
+            };
+            await config.errorLogger.logError(errorEntry);
+          }
+        }
+      }
+    }
+  } catch (error) {
+    // Log error but only fail if it's a validation error
+    if ((error as Error).message.includes('out of range')) {
+      throw error;
+    }
+    log('Non-critical error during step validation: %s', (error as Error).message);
+  }
+
   // Create connection for the agent with optional taskId
   const connection = {
     id: `report-progress-${Date.now()}-${Math.random().toString(36).substring(2, 11)}`,

src/tools/submit-plan.ts

@@ -2,6 +2,7 @@
  * Submit plan tool - Plan submission without file exposure
  * Accepts plan content and handles file creation internally
  * Enhanced with optional context reporting (Issue #51)
+ * Enhanced with stepCount parameter for efficient validation (Issue #60)
  */
 
 import { ServerConfig } from '../types.js';
@@ -10,10 +11,13 @@ import { validateRequiredString, validateRequiredConfig } from '../utils/validat
 import { AgentCommError } from '../types.js';
 import { ErrorLogEntry } from '../logging/ErrorLogger.js';
 import type { AgentContextData, ContextEstimate } from '../types/context-types.js';
+import { parsePlanCheckboxes, validateStepCount } from '../utils/plan-parser.js';
+import { PlanMetadata } from '../types/plan-metadata.js';
+import * as fs from '../utils/fs-extra-safe.js';
+import path from 'path';
 import debug from 'debug';
 
-
-const log = debug('agent-comm:tools:submitplan');
+const log = debug('agent-comm:tools:submit-plan');
 interface PlanValidationResult {
   valid: boolean;
   checkboxCount: number;
@@ -81,14 +85,15 @@ export async function submitPlan(
   const content = validateRequiredString(args['content'], 'content');
   const agent = validateRequiredString(args['agent'], 'agent');
   const taskId = args['taskId'] as string | undefined; // Optional taskId parameter
+  const stepCount = args['stepCount'] as number | undefined; // Optional stepCount parameter (Issue #60)
 
   // Optional context parameters (Issue #51)
   const agentContext = args['agentContext'] as AgentContextData | undefined;
   const contextEstimate = args['contextEstimate'] as ContextEstimate | undefined;
 
   // Validate plan format before submission
   const validation = validatePlanFormat(content);
-  
+
   if (!validation.valid) {
     const errorMessage = [
       'Plan format validation failed:',
@@ -140,7 +145,51 @@ export async function submitPlan(
 
     throw new AgentCommError(errorMessage, 'PLAN_FORMAT_INVALID');
   }
-  
+
+  // Validate stepCount if provided (Issue #60)
+  if (stepCount !== undefined) {
+    const startTime = Date.now();
+    const checkboxes = parsePlanCheckboxes(content);
+    const actualCount = checkboxes.length;
+    const validationTime = Date.now() - startTime;
+
+    log('Step count validation: expected=%d, actual=%d, time=%dms', stepCount, actualCount, validationTime);
+
+    if (validationTime > 10) {
+      log('PERFORMANCE WARNING: Step validation took %dms (>10ms threshold)', validationTime);
+    }
+
+    if (!validateStepCount(stepCount, actualCount)) {
+      const errorMessage = `Step count mismatch: expected ${stepCount}, actual ${actualCount}`;
+
+      if (config.errorLogger) {
+        const errorEntry: ErrorLogEntry = {
+          timestamp: new Date(),
+          source: 'validation',
+          operation: 'submit_plan',
+          agent,
+          error: {
+            message: errorMessage,
+            name: 'StepCountMismatchError',
+            code: 'STEP_COUNT_MISMATCH'
+          },
+          context: {
+            tool: 'submit-plan',
+            parameters: {
+              expectedStepCount: stepCount,
+              actualStepCount: actualCount,
+              planContent: content.substring(0, 500)
+            }
+          },
+          severity: 'medium'
+        };
+        await config.errorLogger.logError(errorEntry);
+      }
+
+      throw new AgentCommError(errorMessage, 'STEP_COUNT_MISMATCH');
+    }
+  }
+
   // Create connection for the agent with optional taskId
   const connection = {
     id: `submit-plan-${Date.now()}-${Math.random().toString(36).substring(2, 11)}`,
@@ -178,5 +227,43 @@ export async function submitPlan(
     });
   }
 
-  return await contextManager.submitPlan(content, connection);
+  const result = await contextManager.submitPlan(content, connection);
+
+  // Create PLAN.metadata.json after successful submission (Issue #60)
+  try {
+    const startTime = Date.now();
+
+    // Calculate actual step count if not provided
+    const actualStepCount = stepCount ?? parsePlanCheckboxes(content).length;
+
+    // Get the task path from context manager or use default
+    const taskPath = path.join(config.commDir, agent, taskId ?? 'current-task');
+
+    // Create metadata object
+    const metadata: PlanMetadata = {
+      stepCount: actualStepCount,
+      agent,
+      ...(taskId && { taskId }),
+      createdAt: new Date().toISOString(),
+      checkboxPattern: 'markdown',
+      version: '2.0.0'
+    };
+
+    // Write metadata file
+    const metadataPath = path.join(taskPath, 'PLAN.metadata.json');
+    await fs.ensureDir(taskPath);
+    await fs.writeFile(metadataPath, JSON.stringify(metadata, null, 2));
+
+    const metadataTime = Date.now() - startTime;
+    log('Created PLAN.metadata.json with stepCount=%d in %dms', actualStepCount, metadataTime);
+
+    if (metadataTime > 10) {
+      log('PERFORMANCE WARNING: Metadata creation took %dms (>10ms threshold)', metadataTime);
+    }
+  } catch (error) {
+    // Log error but don't fail the submission - metadata is optimization
+    log('Failed to create PLAN.metadata.json: %s', (error as Error).message);
+  }
+
+  return result;
 }