From 4fc5b086e58eef58bea8a11102f3539a4ec42189 Mon Sep 17 00:00:00 2001 From: bradygaster Date: Sun, 22 Mar 2026 03:02:26 -0700 Subject: [PATCH] fix: surface rate limit errors with recovery options (#464) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add rateLimitGuidance() and extractRetryAfter() to error-messages.ts - Detect rate limit errors (instanceof RateLimitError or message pattern) in handleDispatch catch block instead of falling through to generic message - Show actionable recovery: retry time, squad economy on, model override - Write .squad/rate-limit-status.json on detection for squad doctor to read - Add checkRateLimitStatus() to squad doctor — warns on recent rate limits, passes when stale (>4h), silent when no rate limit has been hit - 36 new tests covering rateLimitGuidance, extractRetryAfter, and doctor check Closes #464 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .squad/agents/eecom/history.md | 17 ++++++ packages/squad-cli/src/cli/commands/doctor.ts | 52 ++++++++++++++++ .../squad-cli/src/cli/shell/error-messages.ts | 39 ++++++++++++ packages/squad-cli/src/cli/shell/index.ts | 42 +++++++++++-- test/cli/doctor.test.ts | 42 +++++++++++++ test/error-messages.test.ts | 60 +++++++++++++++++++ 6 files changed, 246 insertions(+), 6 deletions(-) diff --git a/.squad/agents/eecom/history.md b/.squad/agents/eecom/history.md index 0cd497032..9278cc75c 100644 --- a/.squad/agents/eecom/history.md +++ b/.squad/agents/eecom/history.md @@ -5,6 +5,23 @@ ## Learnings 📌 **Team update (2026-03-22T09-35Z — Wave 1):** Economy mode fully implemented: ECONOMY_MODEL_MAP + resolveModel() integration in SDK, `squad economy on|off` CLI command, `--economy` flag, 34 tests passing. PR #504 open for review. Soft dependency: #464 rate limit UX should offer economy mode as recovery. Next: Phase 1 of ambient personal squad (T1–T5, T19) — ready to start immediately after merging current work. Procedures wrote governance proposals for squad.agent.md — awaiting Flight review. +### Rate Limit UX (#464) (2026-03-20) + +**Context:** Users hitting Copilot rate limits saw generic "Something went wrong processing your message." Squad hid the actual error. `squad doctor` reported nothing — useless to diagnose. + +**Root cause:** The catch block in `shell/index.ts` line ~1119 always emitted `genericGuidance()` unless `SQUAD_DEBUG=1`. Rate limit errors never got special treatment despite `RateLimitError` existing in `adapter/errors.ts`. + +**Fix:** +1. `error-messages.ts` — Added `rateLimitGuidance({ retryAfter?, model? })` and `extractRetryAfter(message)` utilities. Rate limit guidance shows clear message + recovery options (retry time, `squad economy on`, config.json model override). +2. `shell/index.ts` — Catch block now detects rate limits via `instanceof RateLimitError` OR regex on the raw message. Writes `.squad/rate-limit-status.json` on detection. +3. `doctor.ts` — Added `checkRateLimitStatus()` check. Reads status file and warns if rate limit was recent. +4. `test/error-messages.test.ts` — Added 11 new tests covering `rateLimitGuidance` and `extractRetryAfter`. + +**Pattern:** Rate limit status written to `.squad/rate-limit-status.json` as `{ timestamp, retryAfter, model, message }`. Doctor reads it on next run. File is never deleted automatically — doctor marks it `pass` when > 4h stale. + +**Import path for `RateLimitError`:** `@bradygaster/squad-sdk/adapter/errors` (subpath export, not in main barrel). + +**PR:** #464 fix — squad/464-rate-limit-ux ### CLI Entry Point Architecture cli-entry.ts is the central router for ~30+ CLI commands using dynamic imports (lazy-loading). Commands are routed via if-else blocks. Has a recurring "unwired command" bug class — implementations exist in cli/commands/ but aren't routed in cli-entry.ts. The cli-command-wiring.test.ts regression test catches this by verifying every .ts file in cli/commands/ is imported. diff --git a/packages/squad-cli/src/cli/commands/doctor.ts b/packages/squad-cli/src/cli/commands/doctor.ts index 1e19023aa..717156be6 100644 --- a/packages/squad-cli/src/cli/commands/doctor.ts +++ b/packages/squad-cli/src/cli/commands/doctor.ts @@ -213,6 +213,56 @@ function checkDecisionsMd(squadDir: string): DoctorCheck { }; } +/** + * Report the last detected rate limit, if any, by reading the status file + * written by the shell when a rate limit error is caught. + */ +function checkRateLimitStatus(squadDir: string): DoctorCheck | undefined { + const statusPath = path.join(squadDir, 'rate-limit-status.json'); + if (!fileExists(statusPath)) return undefined; + + const data = tryReadJson(statusPath) as Record | undefined; + if (!data) { + return { + name: 'rate limit status', + status: 'warn', + message: 'rate-limit-status.json exists but could not be parsed', + }; + } + + const ts = typeof data['timestamp'] === 'string' ? new Date(data['timestamp']) : null; + const retryAfter = typeof data['retryAfter'] === 'number' ? data['retryAfter'] : null; + const model = typeof data['model'] === 'string' ? data['model'] : null; + + const age = ts ? Math.floor((Date.now() - ts.getTime()) / 1000) : null; + const ageStr = age !== null ? ` (${formatAge(age)} ago)` : ''; + const modelStr = model ? ` on model: ${model}` : ''; + const retryStr = retryAfter ? ` — retry after ${retryAfter}s` : ''; + + // If last rate limit was more than 4 hours ago, treat as stale info (pass) + const stale = age !== null && age > 4 * 3600; + + return { + name: 'rate limit status', + status: stale ? 'pass' : 'warn', + message: stale + ? `Last rate limit${ageStr}${modelStr} — appears resolved. Run \`squad economy on\` to reduce future risk.` + : `Rate limit detected${ageStr}${modelStr}${retryStr}. Run \`squad economy on\` to switch to cheaper models.`, + }; +} + +function formatAge(seconds: number): string { + if (seconds >= 3600) { + const h = Math.floor(seconds / 3600); + return `${h}h`; + } + if (seconds >= 60) { + const m = Math.floor(seconds / 60); + return `${m}m`; + } + return `${seconds}s`; +} + // ── ESM compatibility checks ──────────────────────────────────────── // ── environment checks ───────────────────────────────────────────── @@ -363,6 +413,8 @@ export async function runDoctor(cwd?: string): Promise { checks.push(checkAgentsDir(squadDir)); checks.push(checkCastingRegistry(squadDir)); checks.push(checkDecisionsMd(squadDir)); + const rateLimitCheck = checkRateLimitStatus(squadDir); + if (rateLimitCheck) checks.push(rateLimitCheck); } // 10. Node.js version (node:sqlite availability) diff --git a/packages/squad-cli/src/cli/shell/error-messages.ts b/packages/squad-cli/src/cli/shell/error-messages.ts index e4dc836fc..bee06d31d 100644 --- a/packages/squad-cli/src/cli/shell/error-messages.ts +++ b/packages/squad-cli/src/cli/shell/error-messages.ts @@ -46,6 +46,45 @@ export function agentSessionGuidance(agentName: string, detail?: string): ErrorG }; } +/** Format seconds into a human-readable duration string */ +function formatDuration(seconds: number): string { + const hours = Math.ceil(seconds / 3600); + const minutes = Math.ceil(seconds / 60); + if (seconds >= 3600) return `${hours} hour${hours === 1 ? '' : 's'}`; + if (seconds >= 60) return `${minutes} minute${minutes === 1 ? '' : 's'}`; + return `${seconds} second${seconds === 1 ? '' : 's'}`; +} + +/** + * Extract retry-after duration (in seconds) from an error message string. + * Handles patterns like "retry after 120 seconds", "try again in 2 hours", etc. + */ +export function extractRetryAfter(message: string): number | undefined { + const secMatch = message.match(/retry.{0,15}after\s+(\d+)\s*second/i); + if (secMatch) return parseInt(secMatch[1]!, 10); + const hrMatch = message.match(/(?:try again|retry).{0,20}in\s+(\d+)\s*hour/i); + if (hrMatch) return parseInt(hrMatch[1]!, 10) * 3600; + const minMatch = message.match(/(?:try again|retry).{0,20}in\s+(\d+)\s*minute/i); + if (minMatch) return parseInt(minMatch[1]!, 10) * 60; + return undefined; +} + +/** Rate limit hit — model or endpoint temporarily throttled */ +export function rateLimitGuidance(opts?: { retryAfter?: number; model?: string }): ErrorGuidance { + const modelStr = opts?.model ? ` for ${opts.model}` : ''; + const retryStr = opts?.retryAfter + ? `Try again in ${formatDuration(opts.retryAfter)}` + : 'Try again later when the limit resets'; + return { + message: `Rate limit reached${modelStr}. Copilot has temporarily throttled your requests.`, + recovery: [ + retryStr, + 'Enable economy mode to switch to cheaper models: `squad economy on`', + 'Or set a different model: add `"defaultModel": "gpt-4.1"` to .squad/config.json', + ], + }; +} + /** Generic error with context */ export function genericGuidance(detail: string): ErrorGuidance { return { diff --git a/packages/squad-cli/src/cli/shell/index.ts b/packages/squad-cli/src/cli/shell/index.ts index 28eab2dc5..c5265509b 100644 --- a/packages/squad-cli/src/cli/shell/index.ts +++ b/packages/squad-cli/src/cli/shell/index.ts @@ -6,7 +6,7 @@ */ import { createRequire } from 'node:module'; -import { existsSync, readFileSync, unlinkSync } from 'node:fs'; +import { existsSync, readFileSync, unlinkSync, writeFileSync } from 'node:fs'; import { join, resolve as pathResolve } from 'node:path'; import React from 'react'; import { render } from 'ink'; @@ -20,6 +20,7 @@ import { ShellLifecycle, loadWelcomeData } from './lifecycle.js'; import { SquadClient } from '@bradygaster/squad-sdk/client'; import type { SquadSession } from '@bradygaster/squad-sdk/client'; import type { SquadPermissionHandler } from '@bradygaster/squad-sdk/client'; +import { RateLimitError } from '@bradygaster/squad-sdk/adapter/errors'; import type { ShellMessage } from './types.js'; import { initSquadTelemetry, TIMEOUTS, StreamingPipeline, recordAgentSpawn, recordAgentDuration, recordAgentError, recordAgentDestroy, RuntimeEventBus, resolveSquad, resolveGlobalSquadPath } from '@bradygaster/squad-sdk'; import type { UsageEvent } from '@bradygaster/squad-sdk'; @@ -28,7 +29,7 @@ import { buildCoordinatorPrompt, buildInitModePrompt, parseCoordinatorResponse, import { loadAgentCharter, buildAgentPrompt } from './spawn.js'; import { createSession, saveSession, loadLatestSession, type SessionData } from './session-store.js'; import { parseDispatchTargets, type ParsedInput } from './router.js'; -import { agentSessionGuidance, genericGuidance, formatGuidance } from './error-messages.js'; +import { agentSessionGuidance, genericGuidance, rateLimitGuidance, extractRetryAfter, formatGuidance } from './error-messages.js'; import { parseCastResponse, createTeam, formatCastSummary, augmentWithCastingEngine, type CastProposal } from '../core/cast.js'; export { SessionRegistry } from './sessions.js'; @@ -61,6 +62,8 @@ export { teamConfigGuidance, agentSessionGuidance, genericGuidance, + rateLimitGuidance, + extractRetryAfter, timeoutGuidance, unknownCommandGuidance, formatGuidance, @@ -1120,11 +1123,38 @@ export async function runShell(): Promise { debugLog('handleDispatch error:', err); recordShellError('dispatch', err instanceof Error ? err.constructor.name : 'unknown'); const errorMsg = err instanceof Error ? err.message : String(err); - const friendly = errorMsg.replace(/^Error:\s*/i, ''); - // Only show raw error detail when SQUAD_DEBUG=1; otherwise keep it generic - const detail = process.env['SQUAD_DEBUG'] === '1' ? friendly : 'Something went wrong processing your message.'; if (shellApi) { - const guidance = genericGuidance(detail); + const isRateLimit = + err instanceof RateLimitError || + /rate.?limit|quota.*exceed|429/i.test(errorMsg); + let guidance; + if (isRateLimit) { + const retryAfter = + err instanceof RateLimitError + ? err.retryAfter + : extractRetryAfter(errorMsg); + const model = + err instanceof RateLimitError ? err.context.model : undefined; + guidance = rateLimitGuidance({ retryAfter, model }); + // Persist rate limit status so `squad doctor` can surface it. + try { + const squadDir = join(teamRoot, '.squad'); + writeFileSync( + join(squadDir, 'rate-limit-status.json'), + JSON.stringify({ + timestamp: new Date().toISOString(), + retryAfter, + model, + message: errorMsg, + }), + ); + } catch { /* non-fatal */ } + } else if (process.env['SQUAD_DEBUG'] === '1') { + const friendly = errorMsg.replace(/^Error:\s*/i, ''); + guidance = genericGuidance(friendly); + } else { + guidance = genericGuidance('Something went wrong processing your message.'); + } shellApi.addMessage({ role: 'system', content: formatGuidance(guidance), diff --git a/test/cli/doctor.test.ts b/test/cli/doctor.test.ts index 0935f3671..f2d630499 100644 --- a/test/cli/doctor.test.ts +++ b/test/cli/doctor.test.ts @@ -130,6 +130,48 @@ describe('squad doctor', () => { expect(result.status).toBe('pass'); }); + it('warns when a recent rate limit status file exists', async () => { + await scaffold(TEST_ROOT); + const status = { + timestamp: new Date().toISOString(), + retryAfter: 7200, + model: 'claude-sonnet-4.5', + message: 'Rate limit exceeded', + }; + await writeFile( + join(TEST_ROOT, '.squad', 'rate-limit-status.json'), + JSON.stringify(status), + ); + + const checks = await runDoctor(TEST_ROOT); + const rlCheck = checks.find((c: DoctorCheck) => c.name === 'rate limit status'); + expect(rlCheck).toBeDefined(); + expect(rlCheck?.status).toBe('warn'); + expect(rlCheck?.message).toContain('claude-sonnet-4.5'); + expect(rlCheck?.message).toContain('squad economy on'); + }); + + it('passes rate limit status as stale when timestamp is old', async () => { + await scaffold(TEST_ROOT); + const oldTs = new Date(Date.now() - 5 * 3600 * 1000).toISOString(); // 5h ago + await writeFile( + join(TEST_ROOT, '.squad', 'rate-limit-status.json'), + JSON.stringify({ timestamp: oldTs, retryAfter: 7200, model: null, message: 'old' }), + ); + + const checks = await runDoctor(TEST_ROOT); + const rlCheck = checks.find((c: DoctorCheck) => c.name === 'rate limit status'); + expect(rlCheck?.status).toBe('pass'); + expect(rlCheck?.message).toContain('appears resolved'); + }); + + it('does not include rate limit status check when file is absent', async () => { + await scaffold(TEST_ROOT); + const checks = await runDoctor(TEST_ROOT); + const rlCheck = checks.find((c: DoctorCheck) => c.name === 'rate limit status'); + expect(rlCheck).toBeUndefined(); + }); + it('warns on absolute teamRoot', async () => { await scaffold(TEST_ROOT); const abs = process.platform === 'win32' ? 'C:\\some\\absolute\\path' : '/some/absolute/path'; diff --git a/test/error-messages.test.ts b/test/error-messages.test.ts index 0df65c665..af71bfe00 100644 --- a/test/error-messages.test.ts +++ b/test/error-messages.test.ts @@ -10,6 +10,8 @@ import { teamConfigGuidance, agentSessionGuidance, genericGuidance, + rateLimitGuidance, + extractRetryAfter, formatGuidance, } from '@bradygaster/squad-cli/shell/error-messages'; @@ -107,4 +109,62 @@ describe('error-messages', () => { expect(lines.length).toBeGreaterThanOrEqual(4); // message + Try: + at least 2 bullets }); }); + + // ---------- rateLimitGuidance ---------- + describe('rateLimitGuidance', () => { + it('returns a rate limit message with no options', () => { + const g = rateLimitGuidance(); + expect(g.message).toContain('Rate limit'); + expect(g.recovery.length).toBeGreaterThanOrEqual(2); + }); + + it('includes model name when provided', () => { + const g = rateLimitGuidance({ model: 'claude-sonnet-4.5' }); + expect(g.message).toContain('claude-sonnet-4.5'); + }); + + it('shows retry time when retryAfter is provided in seconds', () => { + const g = rateLimitGuidance({ retryAfter: 120 }); + expect(g.recovery[0]).toContain('2 minutes'); + }); + + it('shows retry time in hours for large values', () => { + const g = rateLimitGuidance({ retryAfter: 7200 }); + expect(g.recovery[0]).toContain('2 hours'); + }); + + it('shows fallback when no retryAfter', () => { + const g = rateLimitGuidance({}); + expect(g.recovery[0]).toContain('later'); + }); + + it('suggests economy mode as recovery', () => { + const g = rateLimitGuidance(); + expect(g.recovery.some(r => r.includes('squad economy on'))).toBe(true); + }); + }); + + // ---------- extractRetryAfter ---------- + describe('extractRetryAfter', () => { + it('extracts seconds from "retry after N seconds"', () => { + expect(extractRetryAfter('Please retry after 120 seconds')).toBe(120); + }); + + it('extracts hours from "try again in N hours"', () => { + expect(extractRetryAfter('Sorry, try again in 2 hours')).toBe(7200); + }); + + it('extracts minutes from "try again in N minutes"', () => { + expect(extractRetryAfter('Please try again in 30 minutes')).toBe(1800); + }); + + it('returns undefined when no pattern matches', () => { + expect(extractRetryAfter('Something went wrong')).toBeUndefined(); + }); + + it('handles the Copilot rate limit message format', () => { + const msg = "Sorry, you've hit a rate limit. Please try again in 2 hours."; + expect(extractRetryAfter(msg)).toBe(7200); + }); + }); });