-
Notifications
You must be signed in to change notification settings - Fork 0
feat: cockatiel resilience, security hardening, and error classification #3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| { | ||
| "permissions": { | ||
| "allow": [ | ||
| "Bash(grep -E \"\\\\.ts$|\\\\.js$|\\\\.json$\")", | ||
| "Bash(xargs wc:*)", | ||
| "Bash(bun test:*)", | ||
| "Bash(bun run:*)" | ||
| ] | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,8 @@ | ||
| name: CI | ||
|
|
||
| on: | ||
| push: | ||
| branches: [master] | ||
| pull_request: | ||
| branches: [develop, master] | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -12,6 +12,7 @@ on: | |||
|
|
||||
| permissions: | ||||
| contents: write | ||||
| id-token: write | ||||
|
|
||||
| jobs: | ||||
| test: | ||||
|
|
@@ -41,6 +42,8 @@ jobs: | |||
| runs-on: ubuntu-latest | ||||
| steps: | ||||
| - uses: actions/checkout@v4 | ||||
| with: | ||||
| fetch-depth: 0 | ||||
| - uses: oven-sh/setup-bun@v2 | ||||
| with: | ||||
| bun-version: latest | ||||
|
|
@@ -49,14 +52,25 @@ jobs: | |||
| node-version: "22" | ||||
| registry-url: "https://registry.npmjs.org" | ||||
| - run: bun install | ||||
| - name: Determine version | ||||
| id: version | ||||
| run: | | ||||
| if [ "${{ github.event_name }}" = "workflow_dispatch" ]; then | ||||
| TAG="${{ inputs.tag }}" | ||||
| else | ||||
| PKG_VERSION=$(node -p "require('./package.json').version") | ||||
| TAG="v${PKG_VERSION}" | ||||
| fi | ||||
| echo "tag=$TAG" >> "$GITHUB_OUTPUT" | ||||
| npm version "${TAG#v}" --no-git-tag-version --allow-same-version | ||||
|
||||
| npm version "${TAG#v}" --no-git-tag-version --allow-same-version |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,38 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Error Classification — determines retry strategy based on error type. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Categories: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * transient → retry with standard backoff | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * rate_limit → retry with longer delay | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * permanent → do not retry, fallback immediately | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * crash → do not retry, fallback immediately | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export type ErrorClass = "transient" | "rate_limit" | "permanent" | "crash" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export function classifyError(error: any): ErrorClass { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const msg = String(error?.message || error?.stderr || "") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Crash: process killed, binary not found | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (error?.exitCode === 137 || msg.includes("SIGKILL") || msg.includes("ENOENT")) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return "crash" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Rate limit: HTTP 429 or quota errors | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (msg.includes("429") || msg.includes("rate limit") || msg.includes("quota")) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return "rate_limit" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Permanent: auth failures, not found | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| msg.includes("auth") || | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| msg.includes("401") || | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| msg.includes("403") || | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| msg.includes("not found") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+15
to
+31
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Crash: process killed, binary not found | |
| if (error?.exitCode === 137 || msg.includes("SIGKILL") || msg.includes("ENOENT")) { | |
| return "crash" | |
| } | |
| // Rate limit: HTTP 429 or quota errors | |
| if (msg.includes("429") || msg.includes("rate limit") || msg.includes("quota")) { | |
| return "rate_limit" | |
| } | |
| // Permanent: auth failures, not found | |
| if ( | |
| msg.includes("auth") || | |
| msg.includes("401") || | |
| msg.includes("403") || | |
| msg.includes("not found") | |
| const normalizedMsg = msg.toLowerCase() | |
| // Crash: process killed, binary not found | |
| if (error?.exitCode === 137 || normalizedMsg.includes("sigkill") || normalizedMsg.includes("enoent")) { | |
| return "crash" | |
| } | |
| // Rate limit: HTTP 429 or quota errors | |
| if (normalizedMsg.includes("429") || normalizedMsg.includes("rate limit") || normalizedMsg.includes("quota")) { | |
| return "rate_limit" | |
| } | |
| // Permanent: auth failures, not found | |
| if ( | |
| normalizedMsg.includes("auth") || | |
| normalizedMsg.includes("401") || | |
| normalizedMsg.includes("403") || | |
| normalizedMsg.includes("not found") |
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -4,6 +4,8 @@ | |||||||
|
|
||||||||
| import { execa } from "execa" | ||||||||
| import type { CliDef } from "./cli-defs" | ||||||||
| import { getSafeEnv } from "./safe-env" | ||||||||
| import { redactSecrets } from "./redact" | ||||||||
|
|
||||||||
| /** Prompts longer than this (chars) are delivered via stdin to avoid OS arg-length limits. */ | ||||||||
| export const STDIN_THRESHOLD = 30_000 | ||||||||
|
|
@@ -30,6 +32,7 @@ export async function executeCliOnce( | |||||||
| maxBuffer: 10 * 1024 * 1024, | ||||||||
| reject: false, | ||||||||
| windowsHide: true, | ||||||||
| env: getSafeEnv(), | ||||||||
|
||||||||
| env: getSafeEnv(), | |
| env: getSafeEnv(), | |
| extendEnv: false, |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -24,6 +24,7 @@ import { DEFAULT_RETRY_CONFIG } from "./retry" | |||||
| import { detectAllClis, type CliAvailability } from "./detection" | ||||||
| import { truncate } from "./executor" | ||||||
| import { executeWithResilience, type ResilienceContext, type UsageStats } from "./resilience" | ||||||
| import { redactSecrets } from "./redact" | ||||||
|
|
||||||
| // Agents that should NOT receive CLI injection | ||||||
| const NO_CLI_AGENTS = new Set(["orchestrator", "task_decomposer"]) | ||||||
|
|
@@ -141,7 +142,8 @@ Rules: One concern per call. Split large requests. Include "CLI Consultations" i | |||||
| return { | ||||||
| ...response, | ||||||
| stdout: truncate(response.stdout, 50_000), | ||||||
| stderr: truncate(response.stderr, 5_000), | ||||||
| stderr: redactSecrets(truncate(response.stderr, 5_000)), | ||||||
|
||||||
| stderr: redactSecrets(truncate(response.stderr, 5_000)), | |
| stderr: truncate(redactSecrets(response.stderr), 5_000), |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,48 @@ | ||||||
| /** | ||||||
| * Cockatiel Resilience Policies — composable retry, circuit breaker, | ||||||
| * bulkhead, and timeout policies using cockatiel. | ||||||
| * | ||||||
| * Composition order (outermost → innermost): | ||||||
| * timeout → retry → circuit breaker → bulkhead | ||||||
| */ | ||||||
|
|
||||||
| import { | ||||||
| CircuitBreakerPolicy, | ||||||
| ConsecutiveBreaker, | ||||||
| retry, | ||||||
| handleAll, | ||||||
| wrap, | ||||||
| bulkhead, | ||||||
| timeout, | ||||||
| ExponentialBackoff, | ||||||
| type IPolicy, | ||||||
| } from "cockatiel" | ||||||
|
|
||||||
| /** Per-CLI bulkhead: max 2 concurrent, queue up to 3 */ | ||||||
|
||||||
| /** Per-CLI bulkhead: max 2 concurrent, queue up to 3 */ | |
| /** Shared CLI bulkhead: max 2 concurrent, queue up to 3 */ |
Copilot
AI
Mar 30, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
policies.ts exports cockatiel policies and adds a new dependency, but nothing in src/ imports or uses these policies right now. If the intent is to switch the resilience engine to cockatiel, wire resilientPolicy into the execution path; otherwise remove the unused module/dependency to avoid shipping dead code.
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,10 @@ | ||||||
| /** | ||||||
| * Secret Redaction — removes API keys and tokens from text before | ||||||
| * returning it to the user in error messages or logs. | ||||||
| */ | ||||||
|
|
||||||
| export function redactSecrets(text: string): string { | ||||||
| return text | ||||||
| .replace(/(?:sk-|key-|AIza|ant-api)[a-zA-Z0-9_-]{20,}/g, "[REDACTED]") | ||||||
| .replace(/Bearer\s+[a-zA-Z0-9._-]+/g, "Bearer [REDACTED]") | ||||||
|
||||||
| .replace(/Bearer\s+[a-zA-Z0-9._-]+/g, "Bearer [REDACTED]") | |
| .replace(/(^|\s)Bearer\s+[a-zA-Z0-9._-]+/g, "$1Bearer [REDACTED]") |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -13,11 +13,13 @@ import { | |||||
| recordFailure, | ||||||
| } from "./circuit-breaker" | ||||||
| import type { RetryConfig } from "./retry" | ||||||
| import { DEFAULT_RETRY_CONFIG, calculateDelay, sleep, isRetryableError } from "./retry" | ||||||
| import { DEFAULT_RETRY_CONFIG, calculateDelay, sleep } from "./retry" | ||||||
| import { executeCliOnce } from "./executor" | ||||||
| import type { CliAvailability } from "./detection" | ||||||
| import type { Platform } from "./platform" | ||||||
| import type { CircuitState } from "./circuit-breaker" | ||||||
| import { classifyError, type ErrorClass } from "./error-classifier" | ||||||
| import { redactSecrets } from "./redact" | ||||||
|
|
||||||
| // ─── Structured Response (MCP pattern) ───────────────────────────────────── | ||||||
|
|
||||||
|
|
@@ -32,6 +34,7 @@ export interface CliResponse { | |||||
| used_fallback: boolean | ||||||
| fallback_chain: string[] | ||||||
| error: string | null | ||||||
| error_class: ErrorClass | null | ||||||
| circuit_state: CircuitState | ||||||
| attempt: number | ||||||
| max_attempts: number | ||||||
|
|
@@ -128,21 +131,37 @@ export async function executeWithResilience( | |||||
| used_fallback: cliName !== targetCli, | ||||||
| fallback_chain: fallbackChain, | ||||||
| error: null, | ||||||
| error_class: null, | ||||||
| circuit_state: breaker.state, | ||||||
| attempt: attempt + 1, | ||||||
| max_attempts: ctx.retryConfig.maxRetries + 1, | ||||||
| } | ||||||
| } catch (err: unknown) { | ||||||
| stats.failures++ | ||||||
|
|
||||||
| const retryable = isRetryableError(err) | ||||||
| const isLastAttempt = attempt === ctx.retryConfig.maxRetries | ||||||
| const errorClass = classifyError(err) | ||||||
|
|
||||||
| // permanent and crash errors: skip retries, fallback immediately | ||||||
| if (errorClass === "permanent" || errorClass === "crash") { | ||||||
| recordFailure(breaker, ctx.breakerConfig) | ||||||
| break // try next CLI in fallback chain | ||||||
| } | ||||||
|
|
||||||
| // rate_limit: wait longer before retrying | ||||||
| if (errorClass === "rate_limit") { | ||||||
| const rateLimitDelay = calculateDelay(attempt + 1, { | ||||||
| ...ctx.retryConfig, | ||||||
| baseDelayMs: ctx.retryConfig.baseDelayMs * 3, | ||||||
| }) | ||||||
| await sleep(rateLimitDelay) | ||||||
| } | ||||||
|
Comment on lines
+150
to
+157
|
||||||
|
|
||||||
| if (!retryable || isLastAttempt) { | ||||||
| const isLastAttempt = attempt === ctx.retryConfig.maxRetries | ||||||
| if (isLastAttempt) { | ||||||
| recordFailure(breaker, ctx.breakerConfig) | ||||||
| break // try next CLI in fallback chain | ||||||
| } | ||||||
| // retryable — loop continues | ||||||
| // transient or rate_limit — loop continues with retry | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
@@ -158,7 +177,10 @@ export async function executeWithResilience( | |||||
| timed_out: false, | ||||||
| used_fallback: fallbackChain.length > 1, | ||||||
| fallback_chain: fallbackChain, | ||||||
| error: `All CLI providers exhausted. Tried: ${fallbackChain.join(" → ")}. Check cli_status for details.`, | ||||||
| error: redactSecrets( | ||||||
| `All CLI providers exhausted. Tried: ${fallbackChain.join(" → ")}. Check cli_status for details.`, | ||||||
| ), | ||||||
| error_class: "transient", | ||||||
|
||||||
| error_class: "transient", | |
| error_class: null, |
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,30 @@ | ||||||||||||
| /** | ||||||||||||
| * Environment Variable Filtering — only passes safe variables to | ||||||||||||
| * spawned CLI processes, preventing accidental secret leakage. | ||||||||||||
| */ | ||||||||||||
|
|
||||||||||||
| export const SAFE_ENV_VARS = [ | ||||||||||||
| "PATH", | ||||||||||||
| "HOME", | ||||||||||||
| "USER", | ||||||||||||
| "TERM", | ||||||||||||
| "SHELL", | ||||||||||||
| "LANG", | ||||||||||||
| "LC_ALL", | ||||||||||||
| "ANTHROPIC_API_KEY", | ||||||||||||
| "GOOGLE_API_KEY", | ||||||||||||
| "OPENAI_API_KEY", | ||||||||||||
| "GEMINI_API_KEY", | ||||||||||||
| "CODEX_API_KEY", | ||||||||||||
| "HTTP_PROXY", | ||||||||||||
| "HTTPS_PROXY", | ||||||||||||
| "NO_PROXY", | ||||||||||||
| ] | ||||||||||||
|
|
||||||||||||
| export function getSafeEnv(): Record<string, string> { | ||||||||||||
| const env: Record<string, string> = {} | ||||||||||||
| for (const key of SAFE_ENV_VARS) { | ||||||||||||
| if (process.env[key]) env[key] = process.env[key]! | ||||||||||||
|
||||||||||||
| if (process.env[key]) env[key] = process.env[key]! | |
| const value = process.env[key] | |
| if (value !== undefined && value !== null) { | |
| env[key] = value | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is named
settings.local.json, but it’s committed to the repo. “.local” configs are typically developer-machine specific; consider renaming to a non-local config (if it’s meant to be shared) or add it to.gitignore(if it’s meant to stay local) to avoid unintended permission settings being versioned.