diff --git a/docs/audit/FINDINGS.md b/docs/audit/FINDINGS.md index 8c6986d9..b0fe0dac 100644 --- a/docs/audit/FINDINGS.md +++ b/docs/audit/FINDINGS.md @@ -2,14 +2,65 @@ > **Purpose:** Real issues, gaps, and risks discovered during code audits and real-world testing. > **This is NOT a task list.** Tasks live in [TASKS.md](TASKS.md). Findings document _what's wrong_ and _why it matters_. -> **Open:** 0 | **Fixed:** 0 (230 prior findings archived) | **Last Audit:** 2026-03-23 +> **Open:** 4 | **Fixed:** 1 (230 prior findings archived) | **Last Audit:** 2026-03-24 > **History:** 230 findings fixed across v0.0.1–v0.1.0. All prior archived in [archive/](archive/). --- ## Open Findings -_No open findings._ +### OB-F231 — Keyword classifier misclassifies action requests as quick-answer + +- **Severity:** 🟠 High +- **Status:** Open +- **Key Files:** `src/master/classification-engine.ts` +- **Root Cause / Impact:** + Messages containing action verbs ("add", "create", "update", "modify", "delete") are classified as `quick-answer` by the keyword matcher, resulting in only 3 maxTurns. This causes turn exhaustion on tasks that require file reads/writes. Observed: "From the project docs can you add an item for a supplier?" → `quick-answer` → max turns hit. +- **Fix:** Add action-verb detection to the keyword matcher. If the message contains imperative action verbs, classify as `tool-use` minimum (not `quick-answer`), regardless of other keyword signals. + +--- + +### OB-F232 — Turn exhaustion error leaks to user instead of auto-retrying + +- **Severity:** šŸ”“ Critical +- **Status:** āœ… Fixed (2026-03-24) +- **Key Files:** `src/master/master-manager.ts` +- **Root Cause / Impact:** + When a task ends with `turnsExhausted: true`, the raw "Error: Reached max turns (3)" message is sent back to the user via the messaging channel. The user then manually resends the message with the error appended, wasting a round trip and degrading UX. Observed on telegram-2650 → telegram-2654. +- **Fix:** Added Master-level turn-escalation retry mirroring the worker pattern (OB-903). On first exhaustion, auto-retries with `ceil(maxTurns Ɨ 1.5)` (capped at 50), injecting partial output as continuation context. Only surfaces guidance to the user if the escalated retry also exhausts. + +--- + +### OB-F233 — Length heuristic over-classifies conversational questions as tool-use + +- **Severity:** 🟔 Medium +- **Status:** Open +- **Key Files:** `src/master/classification-engine.ts` +- **Root Cause / Impact:** + The length heuristic promotes long multi-sentence messages to `tool-use` (15 turns) even when the intent is conversational/planning ("I wanna provide you a shopfy store and you extract..."). This wastes model budget on unnecessary tool-use allocations. Length should be a tiebreaker, not a primary classifier signal. +- **Fix:** Check intent before applying length heuristic — interrogative phrasing ("can you", "would it be possible") with no imperative action should remain `quick-answer` or `conversation`. Only promote to `tool-use` when the message contains clear action intent. + +--- + +### OB-F234 — RAG confidence consistently low due to shallow indexing + +- **Severity:** 🟔 Medium +- **Status:** Open +- **Key Files:** `src/master/master-manager.ts`, `src/memory/chunk-store.ts`, `src/memory/retrieval.ts` +- **Root Cause / Impact:** + All RAG queries in the session returned confidence 0.32–0.48 with only 2–3 chunks from FTS5. The chunk store had only 4 indexed chunks at startup. The Master is operating mostly blind about the workspace, reducing answer quality and forcing unnecessary tool-use turns to re-read files. +- **Fix:** Trigger re-indexing when RAG confidence is consistently below a threshold (e.g., < 0.5 over N consecutive queries). Consider deeper initial indexing or incremental chunk ingestion when workers read files. + +--- + +### OB-F235 — Timeout clamping fires on every tool-use task + +- **Severity:** 🟢 Low +- **Status:** Open +- **Key Files:** `src/master/master-manager.ts` +- **Root Cause / Impact:** + Every `tool-use` classification triggers `WARN: Timeout clamped to message timeout boundary` (originalTimeout 390–480s → safeTimeout 300s). The calculated timeout consistently exceeds the safe boundary, making the warning noise rather than a useful signal. Either the timeout formula or the safe boundary needs adjustment. +- **Fix:** Either raise the message timeout boundary to accommodate typical tool-use durations, or adjust the timeout formula so tool-use tasks don't routinely exceed the boundary. Alternatively, lower the log level to DEBUG if clamping is expected behavior. --- diff --git a/src/core/agent-runner.ts b/src/core/agent-runner.ts index 091caa5d..9aa0ea0a 100644 --- a/src/core/agent-runner.ts +++ b/src/core/agent-runner.ts @@ -66,8 +66,8 @@ export function setAgentRunnerMetrics(collector: MetricsCollector | null): void * the process timeout kills it (OB-F14: exit code 143 / SIGTERM). */ -/** Max turns for exploration tasks (file listing, classification) — fast, bounded */ -export const DEFAULT_MAX_TURNS_EXPLORATION = 15; +/** Max turns for exploration tasks (file listing, classification) — needs headroom for monorepos */ +export const DEFAULT_MAX_TURNS_EXPLORATION = 25; /** Max turns for user-facing tasks (implementation, reasoning) — more room to work */ export const DEFAULT_MAX_TURNS_TASK = 25; diff --git a/src/master/exploration-coordinator.ts b/src/master/exploration-coordinator.ts index 5794747e..0204c451 100644 --- a/src/master/exploration-coordinator.ts +++ b/src/master/exploration-coordinator.ts @@ -85,7 +85,7 @@ const DirectoryDiveResultAISchema = DirectoryDiveResultSchema.extend({ durationMs: z.number().int().nonnegative().optional().default(0), }) as z.ZodType; -const PHASE_TIMEOUT = 300_000; // 5 minutes per phase (large workspaces need more time) +const PHASE_TIMEOUT = 600_000; // 10 minutes per phase (large workspaces with multiple sub-projects) const DIRECTORY_DIVE_TIMEOUT = 180_000; // 3 minutes per directory dive const MAX_DIRECTORY_DIVE_TIMEOUT = 600_000; // 10 minutes max per directory dive const MAX_RETRIES = 3; diff --git a/src/master/master-manager.ts b/src/master/master-manager.ts index 32da12b2..00a0a587 100644 --- a/src/master/master-manager.ts +++ b/src/master/master-manager.ts @@ -3773,16 +3773,56 @@ export class MasterManager { let response = result.stdout.trim() || 'No response from AI'; - // Guard: Master session itself hit max-turns — provide actionable feedback (OB-F230) + // Guard: Master session itself hit max-turns — auto-retry with escalated budget (OB-F232) + // Mirrors the worker turn-escalation pattern (OB-903) at the Master level. if (result.turnsExhausted) { - const partial = response.length > 20 ? response : ''; - const turnsUsedStr = result.turnsUsed ? ` (used ${result.turnsUsed} turns)` : ''; + const originalMaxTurns = maxTurnsToUse; + const escalatedMaxTurns = Math.min(Math.ceil(originalMaxTurns * 1.5), 50); + logger.warn( - { taskId, maxTurns: maxTurnsToUse, turnsUsed: result.turnsUsed }, - 'Master session hit max-turns limit — returning partial result with guidance', + { taskId, originalMaxTurns, escalatedMaxTurns, turnsUsed: result.turnsUsed }, + 'Master session hit max-turns limit — auto-retrying with escalated budget', ); + + const partialOutput = result.stdout.trim(); + const incompleteMatch = partialOutput.match(/\[INCOMPLETE:\s*([^\]]+)\]/i); + const continuationNote = incompleteMatch?.[1] + ? `Previous attempt was incomplete: ${incompleteMatch[1].trim()}. Continue from where it left off.` + : 'Previous attempt hit the turn limit before completing. Continue from where it left off.'; + + const escalationPrompt = [ + promptToSend, + '', + '---', + 'CONTEXT FROM PREVIOUS ATTEMPT (partial output):', + partialOutput.slice(-2000), + '---', + continuationNote, + ].join('\n'); + + const escalationOpts = this.buildMasterSpawnOptions( + escalationPrompt, + safeTimeout, + escalatedMaxTurns, + masterContext, + ); + result = await this.agentRunner.spawn(escalationOpts); + await this.updateMasterSession(); + response = result.stdout.trim() || 'No response from AI'; + + if (result.turnsExhausted) { + logger.warn( + { taskId, escalatedMaxTurns, turnsUsed: result.turnsUsed }, + 'Escalated retry also exhausted — returning partial result with guidance', + ); + } + } + + // If still exhausted after escalation, provide actionable feedback (OB-F230) + if (result.turnsExhausted) { + const partial = response.length > 20 ? response : ''; + const turnsUsedStr = result.turnsUsed ? ` (used ${result.turnsUsed} turns)` : ''; if (partial && !hasSpawnMarkers(partial)) { - // Master produced some useful output before exhaustion — append guidance response = partial + `\n\nāš ļø I ran out of processing capacity${turnsUsedStr}. ` + diff --git a/tests/core/agent-runner.test.ts b/tests/core/agent-runner.test.ts index f5144d7a..a1c146cf 100644 --- a/tests/core/agent-runner.test.ts +++ b/tests/core/agent-runner.test.ts @@ -324,16 +324,16 @@ describe('Tool group constants', () => { // ── Max-turns defaults ────────────────────────────────────────────── describe('Max-turns defaults', () => { - it('DEFAULT_MAX_TURNS_EXPLORATION is 15', () => { - expect(DEFAULT_MAX_TURNS_EXPLORATION).toBe(15); + it('DEFAULT_MAX_TURNS_EXPLORATION is 25 (monorepo headroom)', () => { + expect(DEFAULT_MAX_TURNS_EXPLORATION).toBe(25); }); it('DEFAULT_MAX_TURNS_TASK is 25', () => { expect(DEFAULT_MAX_TURNS_TASK).toBe(25); }); - it('exploration default is lower than task default', () => { - expect(DEFAULT_MAX_TURNS_EXPLORATION).toBeLessThan(DEFAULT_MAX_TURNS_TASK); + it('exploration default does not exceed task default', () => { + expect(DEFAULT_MAX_TURNS_EXPLORATION).toBeLessThanOrEqual(DEFAULT_MAX_TURNS_TASK); }); it('--max-turns is always present in args even without explicit maxTurns', () => { @@ -348,7 +348,7 @@ describe('Max-turns defaults', () => { maxTurns: DEFAULT_MAX_TURNS_EXPLORATION, }); const idx = args.indexOf('--max-turns'); - expect(args[idx + 1]).toBe('15'); + expect(args[idx + 1]).toBe('25'); }); }); diff --git a/tests/setup.ts b/tests/setup.ts index 88d4c74b..b03c44c9 100644 --- a/tests/setup.ts +++ b/tests/setup.ts @@ -10,6 +10,8 @@ import * as os from 'node:os'; * by interrupted test runs. Runs once before all tests. */ async function cleanStaleTestWorkspaces(): Promise { + const STALE_THRESHOLD_MS = 300_000; // 5 minutes — only clean dirs older than this + const now = Date.now(); const dirs = [process.cwd(), os.tmpdir()]; for (const dir of dirs) { try { @@ -17,7 +19,11 @@ async function cleanStaleTestWorkspaces(): Promise { const stale = entries.filter((e) => e.startsWith('test-workspace-')); for (const entry of stale) { try { - await fs.rm(path.join(dir, entry), { recursive: true, force: true }); + const fullPath = path.join(dir, entry); + const stat = await fs.stat(fullPath); + if (now - stat.mtimeMs > STALE_THRESHOLD_MS) { + await fs.rm(fullPath, { recursive: true, force: true }); + } } catch { // Ignore individual cleanup errors }