add skills
This commit is contained in:
YeonGyu-Kim
@@ -0,0 +1,15 @@
|
||||
{
|
||||
"run_id": "eval-1-without_skill",
|
||||
"expectations": [
|
||||
{"text": "Plan uses git worktree in a sibling directory", "passed": false, "evidence": "Uses git checkout -b, no worktree isolation"},
|
||||
{"text": "Branch is created from origin/dev", "passed": true, "evidence": "git checkout -b feat/max-background-agents dev"},
|
||||
{"text": "Plan specifies multiple atomic commits for multi-file changes", "passed": false, "evidence": "Steps listed sequentially but no atomic commit strategy mentioned"},
|
||||
{"text": "Runs bun run typecheck, bun test, and bun run build before pushing", "passed": true, "evidence": "Step 6 runs typecheck and tests, Step 8 implies push after verification"},
|
||||
{"text": "PR is created targeting dev branch", "passed": true, "evidence": "Step 8 mentions creating PR"},
|
||||
{"text": "Verification loop includes all 3 gates: CI, review-work, and Cubic", "passed": false, "evidence": "Only mentions CI pipeline in step 6. No review-work or Cubic."},
|
||||
{"text": "Gates are checked in order: CI first, then review-work, then Cubic", "passed": false, "evidence": "No gate ordering - only CI mentioned"},
|
||||
{"text": "Cubic check uses gh api to check cubic-dev-ai[bot] reviews", "passed": false, "evidence": "No mention of Cubic at all"},
|
||||
{"text": "Plan includes worktree cleanup after merge", "passed": false, "evidence": "No worktree used, no cleanup needed"},
|
||||
{"text": "Code changes reference actual files in the codebase", "passed": true, "evidence": "References actual files with detailed design decisions"}
|
||||
]
|
||||
}
|
||||
+615
@@ -0,0 +1,615 @@
|
||||
# Code Changes: `max_background_agents` Config Option
|
||||
|
||||
## 1. Schema Change
|
||||
|
||||
**File:** `src/config/schema/background-task.ts`
|
||||
|
||||
```typescript
|
||||
import { z } from "zod"
|
||||
|
||||
export const BackgroundTaskConfigSchema = z.object({
|
||||
defaultConcurrency: z.number().min(1).optional(),
|
||||
providerConcurrency: z.record(z.string(), z.number().min(0)).optional(),
|
||||
modelConcurrency: z.record(z.string(), z.number().min(0)).optional(),
|
||||
maxDepth: z.number().int().min(1).optional(),
|
||||
maxDescendants: z.number().int().min(1).optional(),
|
||||
/** Maximum number of background agents that can run simultaneously across all models/providers (default: no global limit, only per-model limits apply) */
|
||||
maxBackgroundAgents: z.number().int().min(1).optional(),
|
||||
/** Stale timeout in milliseconds - interrupt tasks with no activity for this duration (default: 180000 = 3 minutes, minimum: 60000 = 1 minute) */
|
||||
staleTimeoutMs: z.number().min(60000).optional(),
|
||||
/** Timeout for tasks that never received any progress update, falling back to startedAt (default: 1800000 = 30 minutes, minimum: 60000 = 1 minute) */
|
||||
messageStalenessTimeoutMs: z.number().min(60000).optional(),
|
||||
syncPollTimeoutMs: z.number().min(60000).optional(),
|
||||
})
|
||||
|
||||
export type BackgroundTaskConfig = z.infer<typeof BackgroundTaskConfigSchema>
|
||||
```
|
||||
|
||||
**What changed:** Added `maxBackgroundAgents` field after `maxDescendants` (grouped with other limit fields). Uses `z.number().int().min(1).optional()` matching the pattern of `maxDepth` and `maxDescendants`.
|
||||
|
||||
---
|
||||
|
||||
## 2. ConcurrencyManager Changes
|
||||
|
||||
**File:** `src/features/background-agent/concurrency.ts`
|
||||
|
||||
```typescript
|
||||
import type { BackgroundTaskConfig } from "../../config/schema"
|
||||
|
||||
/**
|
||||
* Queue entry with settled-flag pattern to prevent double-resolution.
|
||||
*
|
||||
* The settled flag ensures that cancelWaiters() doesn't reject
|
||||
* an entry that was already resolved by release().
|
||||
*/
|
||||
interface QueueEntry {
|
||||
resolve: () => void
|
||||
rawReject: (error: Error) => void
|
||||
settled: boolean
|
||||
}
|
||||
|
||||
export class ConcurrencyManager {
|
||||
private config?: BackgroundTaskConfig
|
||||
private counts: Map<string, number> = new Map()
|
||||
private queues: Map<string, QueueEntry[]> = new Map()
|
||||
private globalCount = 0
|
||||
private globalQueue: QueueEntry[] = []
|
||||
|
||||
constructor(config?: BackgroundTaskConfig) {
|
||||
this.config = config
|
||||
}
|
||||
|
||||
getGlobalLimit(): number {
|
||||
const limit = this.config?.maxBackgroundAgents
|
||||
if (limit === undefined) {
|
||||
return Infinity
|
||||
}
|
||||
return limit
|
||||
}
|
||||
|
||||
getConcurrencyLimit(model: string): number {
|
||||
const modelLimit = this.config?.modelConcurrency?.[model]
|
||||
if (modelLimit !== undefined) {
|
||||
return modelLimit === 0 ? Infinity : modelLimit
|
||||
}
|
||||
const provider = model.split('/')[0]
|
||||
const providerLimit = this.config?.providerConcurrency?.[provider]
|
||||
if (providerLimit !== undefined) {
|
||||
return providerLimit === 0 ? Infinity : providerLimit
|
||||
}
|
||||
const defaultLimit = this.config?.defaultConcurrency
|
||||
if (defaultLimit !== undefined) {
|
||||
return defaultLimit === 0 ? Infinity : defaultLimit
|
||||
}
|
||||
return 5
|
||||
}
|
||||
|
||||
async acquire(model: string): Promise<void> {
|
||||
const perModelLimit = this.getConcurrencyLimit(model)
|
||||
const globalLimit = this.getGlobalLimit()
|
||||
|
||||
// Fast path: both limits have capacity
|
||||
if (perModelLimit === Infinity && globalLimit === Infinity) {
|
||||
return
|
||||
}
|
||||
|
||||
const currentPerModel = this.counts.get(model) ?? 0
|
||||
|
||||
if (currentPerModel < perModelLimit && this.globalCount < globalLimit) {
|
||||
this.counts.set(model, currentPerModel + 1)
|
||||
this.globalCount++
|
||||
return
|
||||
}
|
||||
|
||||
return new Promise<void>((resolve, reject) => {
|
||||
const entry: QueueEntry = {
|
||||
resolve: () => {
|
||||
if (entry.settled) return
|
||||
entry.settled = true
|
||||
resolve()
|
||||
},
|
||||
rawReject: reject,
|
||||
settled: false,
|
||||
}
|
||||
|
||||
// Queue on whichever limit is blocking
|
||||
if (currentPerModel >= perModelLimit) {
|
||||
const queue = this.queues.get(model) ?? []
|
||||
queue.push(entry)
|
||||
this.queues.set(model, queue)
|
||||
} else {
|
||||
this.globalQueue.push(entry)
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
release(model: string): void {
|
||||
const perModelLimit = this.getConcurrencyLimit(model)
|
||||
const globalLimit = this.getGlobalLimit()
|
||||
|
||||
if (perModelLimit === Infinity && globalLimit === Infinity) {
|
||||
return
|
||||
}
|
||||
|
||||
// Try per-model handoff first
|
||||
const queue = this.queues.get(model)
|
||||
while (queue && queue.length > 0) {
|
||||
const next = queue.shift()!
|
||||
if (!next.settled) {
|
||||
// Hand off the slot to this waiter (counts stay the same)
|
||||
next.resolve()
|
||||
return
|
||||
}
|
||||
}
|
||||
|
||||
// No per-model handoff - decrement per-model count
|
||||
const current = this.counts.get(model) ?? 0
|
||||
if (current > 0) {
|
||||
this.counts.set(model, current - 1)
|
||||
}
|
||||
|
||||
// Try global handoff
|
||||
while (this.globalQueue.length > 0) {
|
||||
const next = this.globalQueue.shift()!
|
||||
if (!next.settled) {
|
||||
// Hand off the global slot - but the waiter still needs a per-model slot
|
||||
// Since they were queued on global, their per-model had capacity
|
||||
// Re-acquire per-model count for them
|
||||
const waiterModel = this.findModelForGlobalWaiter()
|
||||
if (waiterModel) {
|
||||
const waiterCount = this.counts.get(waiterModel) ?? 0
|
||||
this.counts.set(waiterModel, waiterCount + 1)
|
||||
}
|
||||
next.resolve()
|
||||
return
|
||||
}
|
||||
}
|
||||
|
||||
// No handoff occurred - decrement global count
|
||||
if (this.globalCount > 0) {
|
||||
this.globalCount--
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Cancel all waiting acquires for a model. Used during cleanup.
|
||||
*/
|
||||
cancelWaiters(model: string): void {
|
||||
const queue = this.queues.get(model)
|
||||
if (queue) {
|
||||
for (const entry of queue) {
|
||||
if (!entry.settled) {
|
||||
entry.settled = true
|
||||
entry.rawReject(new Error(`Concurrency queue cancelled for model: ${model}`))
|
||||
}
|
||||
}
|
||||
this.queues.delete(model)
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Clear all state. Used during manager cleanup/shutdown.
|
||||
* Cancels all pending waiters.
|
||||
*/
|
||||
clear(): void {
|
||||
for (const [model] of this.queues) {
|
||||
this.cancelWaiters(model)
|
||||
}
|
||||
// Cancel global queue waiters
|
||||
for (const entry of this.globalQueue) {
|
||||
if (!entry.settled) {
|
||||
entry.settled = true
|
||||
entry.rawReject(new Error("Concurrency queue cancelled: manager shutdown"))
|
||||
}
|
||||
}
|
||||
this.globalQueue = []
|
||||
this.globalCount = 0
|
||||
this.counts.clear()
|
||||
this.queues.clear()
|
||||
}
|
||||
|
||||
/**
|
||||
* Get current count for a model (for testing/debugging)
|
||||
*/
|
||||
getCount(model: string): number {
|
||||
return this.counts.get(model) ?? 0
|
||||
}
|
||||
|
||||
/**
|
||||
* Get queue length for a model (for testing/debugging)
|
||||
*/
|
||||
getQueueLength(model: string): number {
|
||||
return this.queues.get(model)?.length ?? 0
|
||||
}
|
||||
|
||||
/**
|
||||
* Get current global count across all models (for testing/debugging)
|
||||
*/
|
||||
getGlobalCount(): number {
|
||||
return this.globalCount
|
||||
}
|
||||
|
||||
/**
|
||||
* Get global queue length (for testing/debugging)
|
||||
*/
|
||||
getGlobalQueueLength(): number {
|
||||
return this.globalQueue.length
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**What changed:**
|
||||
- Added `globalCount` field to track total active agents across all keys
|
||||
- Added `globalQueue` for tasks waiting on the global limit
|
||||
- Added `getGlobalLimit()` method to read `maxBackgroundAgents` from config
|
||||
- Modified `acquire()` to check both per-model AND global limits
|
||||
- Modified `release()` to handle global queue handoff and decrement global count
|
||||
- Modified `clear()` to reset global state
|
||||
- Added `getGlobalCount()` and `getGlobalQueueLength()` for testing
|
||||
|
||||
**Important design note:** The `release()` implementation above is a simplified version. In practice, the global queue handoff is tricky because we need to know which model the global waiter was trying to acquire for. A cleaner approach would be to store the model key in the QueueEntry. Let me refine:
|
||||
|
||||
### Refined approach (simpler, more correct)
|
||||
|
||||
Instead of a separate global queue, a simpler approach is to check the global limit inside `acquire()` and use a single queue per model. When global capacity frees up on `release()`, we try to drain any model's queue:
|
||||
|
||||
```typescript
|
||||
async acquire(model: string): Promise<void> {
|
||||
const perModelLimit = this.getConcurrencyLimit(model)
|
||||
const globalLimit = this.getGlobalLimit()
|
||||
|
||||
if (perModelLimit === Infinity && globalLimit === Infinity) {
|
||||
return
|
||||
}
|
||||
|
||||
const currentPerModel = this.counts.get(model) ?? 0
|
||||
|
||||
if (currentPerModel < perModelLimit && this.globalCount < globalLimit) {
|
||||
this.counts.set(model, currentPerModel + 1)
|
||||
if (globalLimit !== Infinity) {
|
||||
this.globalCount++
|
||||
}
|
||||
return
|
||||
}
|
||||
|
||||
return new Promise<void>((resolve, reject) => {
|
||||
const queue = this.queues.get(model) ?? []
|
||||
|
||||
const entry: QueueEntry = {
|
||||
resolve: () => {
|
||||
if (entry.settled) return
|
||||
entry.settled = true
|
||||
resolve()
|
||||
},
|
||||
rawReject: reject,
|
||||
settled: false,
|
||||
}
|
||||
|
||||
queue.push(entry)
|
||||
this.queues.set(model, queue)
|
||||
})
|
||||
}
|
||||
|
||||
release(model: string): void {
|
||||
const perModelLimit = this.getConcurrencyLimit(model)
|
||||
const globalLimit = this.getGlobalLimit()
|
||||
|
||||
if (perModelLimit === Infinity && globalLimit === Infinity) {
|
||||
return
|
||||
}
|
||||
|
||||
// Try per-model handoff first (same model queue)
|
||||
const queue = this.queues.get(model)
|
||||
while (queue && queue.length > 0) {
|
||||
const next = queue.shift()!
|
||||
if (!next.settled) {
|
||||
// Hand off the slot to this waiter (per-model and global counts stay the same)
|
||||
next.resolve()
|
||||
return
|
||||
}
|
||||
}
|
||||
|
||||
// No per-model handoff - decrement per-model count
|
||||
const current = this.counts.get(model) ?? 0
|
||||
if (current > 0) {
|
||||
this.counts.set(model, current - 1)
|
||||
}
|
||||
|
||||
// Decrement global count
|
||||
if (globalLimit !== Infinity && this.globalCount > 0) {
|
||||
this.globalCount--
|
||||
}
|
||||
|
||||
// Try to drain any other model's queue that was blocked by global limit
|
||||
if (globalLimit !== Infinity) {
|
||||
this.tryDrainGlobalWaiters()
|
||||
}
|
||||
}
|
||||
|
||||
private tryDrainGlobalWaiters(): void {
|
||||
const globalLimit = this.getGlobalLimit()
|
||||
if (this.globalCount >= globalLimit) return
|
||||
|
||||
for (const [model, queue] of this.queues) {
|
||||
const perModelLimit = this.getConcurrencyLimit(model)
|
||||
const currentPerModel = this.counts.get(model) ?? 0
|
||||
|
||||
if (currentPerModel >= perModelLimit) continue
|
||||
|
||||
while (queue.length > 0 && this.globalCount < globalLimit && currentPerModel < perModelLimit) {
|
||||
const next = queue.shift()!
|
||||
if (!next.settled) {
|
||||
this.counts.set(model, (this.counts.get(model) ?? 0) + 1)
|
||||
this.globalCount++
|
||||
next.resolve()
|
||||
return
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
This refined approach keeps all waiters in per-model queues (no separate global queue), and on release, tries to drain waiters from any model queue that was blocked by the global limit.
|
||||
|
||||
---
|
||||
|
||||
## 3. Schema Test Changes
|
||||
|
||||
**File:** `src/config/schema/background-task.test.ts`
|
||||
|
||||
Add after the `syncPollTimeoutMs` describe block:
|
||||
|
||||
```typescript
|
||||
describe("maxBackgroundAgents", () => {
|
||||
describe("#given valid maxBackgroundAgents (10)", () => {
|
||||
test("#when parsed #then returns correct value", () => {
|
||||
const result = BackgroundTaskConfigSchema.parse({ maxBackgroundAgents: 10 })
|
||||
|
||||
expect(result.maxBackgroundAgents).toBe(10)
|
||||
})
|
||||
})
|
||||
|
||||
describe("#given maxBackgroundAgents of 1 (minimum)", () => {
|
||||
test("#when parsed #then returns correct value", () => {
|
||||
const result = BackgroundTaskConfigSchema.parse({ maxBackgroundAgents: 1 })
|
||||
|
||||
expect(result.maxBackgroundAgents).toBe(1)
|
||||
})
|
||||
})
|
||||
|
||||
describe("#given maxBackgroundAgents below minimum (0)", () => {
|
||||
test("#when parsed #then throws ZodError", () => {
|
||||
let thrownError: unknown
|
||||
|
||||
try {
|
||||
BackgroundTaskConfigSchema.parse({ maxBackgroundAgents: 0 })
|
||||
} catch (error) {
|
||||
thrownError = error
|
||||
}
|
||||
|
||||
expect(thrownError).toBeInstanceOf(ZodError)
|
||||
})
|
||||
})
|
||||
|
||||
describe("#given maxBackgroundAgents is negative (-1)", () => {
|
||||
test("#when parsed #then throws ZodError", () => {
|
||||
let thrownError: unknown
|
||||
|
||||
try {
|
||||
BackgroundTaskConfigSchema.parse({ maxBackgroundAgents: -1 })
|
||||
} catch (error) {
|
||||
thrownError = error
|
||||
}
|
||||
|
||||
expect(thrownError).toBeInstanceOf(ZodError)
|
||||
})
|
||||
})
|
||||
|
||||
describe("#given maxBackgroundAgents is non-integer (2.5)", () => {
|
||||
test("#when parsed #then throws ZodError", () => {
|
||||
let thrownError: unknown
|
||||
|
||||
try {
|
||||
BackgroundTaskConfigSchema.parse({ maxBackgroundAgents: 2.5 })
|
||||
} catch (error) {
|
||||
thrownError = error
|
||||
}
|
||||
|
||||
expect(thrownError).toBeInstanceOf(ZodError)
|
||||
})
|
||||
})
|
||||
|
||||
describe("#given maxBackgroundAgents not provided", () => {
|
||||
test("#when parsed #then field is undefined", () => {
|
||||
const result = BackgroundTaskConfigSchema.parse({})
|
||||
|
||||
expect(result.maxBackgroundAgents).toBeUndefined()
|
||||
})
|
||||
})
|
||||
})
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 4. ConcurrencyManager Test Changes
|
||||
|
||||
**File:** `src/features/background-agent/concurrency.test.ts`
|
||||
|
||||
Add new describe block:
|
||||
|
||||
```typescript
|
||||
describe("ConcurrencyManager.globalLimit (maxBackgroundAgents)", () => {
|
||||
test("should return Infinity when maxBackgroundAgents is not set", () => {
|
||||
// given
|
||||
const manager = new ConcurrencyManager()
|
||||
|
||||
// when
|
||||
const limit = manager.getGlobalLimit()
|
||||
|
||||
// then
|
||||
expect(limit).toBe(Infinity)
|
||||
})
|
||||
|
||||
test("should return configured maxBackgroundAgents", () => {
|
||||
// given
|
||||
const config: BackgroundTaskConfig = { maxBackgroundAgents: 3 }
|
||||
const manager = new ConcurrencyManager(config)
|
||||
|
||||
// when
|
||||
const limit = manager.getGlobalLimit()
|
||||
|
||||
// then
|
||||
expect(limit).toBe(3)
|
||||
})
|
||||
|
||||
test("should enforce global limit across different models", async () => {
|
||||
// given
|
||||
const config: BackgroundTaskConfig = {
|
||||
maxBackgroundAgents: 2,
|
||||
defaultConcurrency: 5,
|
||||
}
|
||||
const manager = new ConcurrencyManager(config)
|
||||
await manager.acquire("model-a")
|
||||
await manager.acquire("model-b")
|
||||
|
||||
// when
|
||||
let resolved = false
|
||||
const waitPromise = manager.acquire("model-c").then(() => { resolved = true })
|
||||
await Promise.resolve()
|
||||
|
||||
// then - should be blocked by global limit even though per-model has capacity
|
||||
expect(resolved).toBe(false)
|
||||
expect(manager.getGlobalCount()).toBe(2)
|
||||
|
||||
// cleanup
|
||||
manager.release("model-a")
|
||||
await waitPromise
|
||||
expect(resolved).toBe(true)
|
||||
})
|
||||
|
||||
test("should allow tasks when global limit not reached", async () => {
|
||||
// given
|
||||
const config: BackgroundTaskConfig = {
|
||||
maxBackgroundAgents: 3,
|
||||
defaultConcurrency: 5,
|
||||
}
|
||||
const manager = new ConcurrencyManager(config)
|
||||
|
||||
// when
|
||||
await manager.acquire("model-a")
|
||||
await manager.acquire("model-b")
|
||||
await manager.acquire("model-c")
|
||||
|
||||
// then
|
||||
expect(manager.getGlobalCount()).toBe(3)
|
||||
expect(manager.getCount("model-a")).toBe(1)
|
||||
expect(manager.getCount("model-b")).toBe(1)
|
||||
expect(manager.getCount("model-c")).toBe(1)
|
||||
})
|
||||
|
||||
test("should respect both per-model and global limits", async () => {
|
||||
// given - per-model limit of 1, global limit of 3
|
||||
const config: BackgroundTaskConfig = {
|
||||
maxBackgroundAgents: 3,
|
||||
defaultConcurrency: 1,
|
||||
}
|
||||
const manager = new ConcurrencyManager(config)
|
||||
await manager.acquire("model-a")
|
||||
|
||||
// when - try second acquire on same model
|
||||
let resolved = false
|
||||
const waitPromise = manager.acquire("model-a").then(() => { resolved = true })
|
||||
await Promise.resolve()
|
||||
|
||||
// then - blocked by per-model limit, not global
|
||||
expect(resolved).toBe(false)
|
||||
expect(manager.getGlobalCount()).toBe(1)
|
||||
|
||||
// cleanup
|
||||
manager.release("model-a")
|
||||
await waitPromise
|
||||
})
|
||||
|
||||
test("should release global slot and unblock waiting tasks", async () => {
|
||||
// given
|
||||
const config: BackgroundTaskConfig = {
|
||||
maxBackgroundAgents: 1,
|
||||
defaultConcurrency: 5,
|
||||
}
|
||||
const manager = new ConcurrencyManager(config)
|
||||
await manager.acquire("model-a")
|
||||
|
||||
// when
|
||||
let resolved = false
|
||||
const waitPromise = manager.acquire("model-b").then(() => { resolved = true })
|
||||
await Promise.resolve()
|
||||
expect(resolved).toBe(false)
|
||||
|
||||
manager.release("model-a")
|
||||
await waitPromise
|
||||
|
||||
// then
|
||||
expect(resolved).toBe(true)
|
||||
expect(manager.getGlobalCount()).toBe(1)
|
||||
expect(manager.getCount("model-a")).toBe(0)
|
||||
expect(manager.getCount("model-b")).toBe(1)
|
||||
})
|
||||
|
||||
test("should not enforce global limit when not configured", async () => {
|
||||
// given - no maxBackgroundAgents set
|
||||
const config: BackgroundTaskConfig = { defaultConcurrency: 5 }
|
||||
const manager = new ConcurrencyManager(config)
|
||||
|
||||
// when - acquire many across different models
|
||||
await manager.acquire("model-a")
|
||||
await manager.acquire("model-b")
|
||||
await manager.acquire("model-c")
|
||||
await manager.acquire("model-d")
|
||||
await manager.acquire("model-e")
|
||||
await manager.acquire("model-f")
|
||||
|
||||
// then - all should succeed (no global limit)
|
||||
expect(manager.getCount("model-a")).toBe(1)
|
||||
expect(manager.getCount("model-f")).toBe(1)
|
||||
})
|
||||
|
||||
test("should reset global count on clear", async () => {
|
||||
// given
|
||||
const config: BackgroundTaskConfig = { maxBackgroundAgents: 5 }
|
||||
const manager = new ConcurrencyManager(config)
|
||||
await manager.acquire("model-a")
|
||||
await manager.acquire("model-b")
|
||||
|
||||
// when
|
||||
manager.clear()
|
||||
|
||||
// then
|
||||
expect(manager.getGlobalCount()).toBe(0)
|
||||
})
|
||||
})
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Config Usage Example
|
||||
|
||||
User's `.opencode/oh-my-opencode.jsonc`:
|
||||
|
||||
```jsonc
|
||||
{
|
||||
"background_task": {
|
||||
// Global limit: max 5 background agents total
|
||||
"maxBackgroundAgents": 5,
|
||||
// Per-model limits still apply independently
|
||||
"defaultConcurrency": 3,
|
||||
"providerConcurrency": {
|
||||
"anthropic": 2
|
||||
}
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
With this config:
|
||||
- Max 5 background agents running simultaneously across all models
|
||||
- Max 3 per model (default), max 2 for any Anthropic model
|
||||
- If 2 Anthropic + 3 OpenAI agents are running (5 total), no more can start regardless of per-model capacity
|
||||
+99
@@ -0,0 +1,99 @@
|
||||
# Execution Plan: Add `max_background_agents` Config Option
|
||||
|
||||
## Overview
|
||||
|
||||
Add a `max_background_agents` config option to oh-my-opencode that limits total simultaneous background agents across all models/providers. Currently, concurrency is only limited per-model/provider key (default 5 per key). This new option adds a **global ceiling** on total running background agents.
|
||||
|
||||
## Step-by-Step Plan
|
||||
|
||||
### Step 1: Create feature branch
|
||||
|
||||
```bash
|
||||
git checkout -b feat/max-background-agents dev
|
||||
```
|
||||
|
||||
### Step 2: Add `max_background_agents` to BackgroundTaskConfigSchema
|
||||
|
||||
**File:** `src/config/schema/background-task.ts`
|
||||
|
||||
- Add `maxBackgroundAgents` field to the Zod schema with `z.number().int().min(1).optional()`
|
||||
- This follows the existing pattern of `maxDepth` and `maxDescendants` (integer, min 1, optional)
|
||||
- The field name uses camelCase to match existing schema fields (`defaultConcurrency`, `maxDepth`, `maxDescendants`)
|
||||
- No `.default()` needed since the hardcoded fallback of 5 lives in `ConcurrencyManager`
|
||||
|
||||
### Step 3: Modify `ConcurrencyManager` to enforce global limit
|
||||
|
||||
**File:** `src/features/background-agent/concurrency.ts`
|
||||
|
||||
- Add a `globalCount` field tracking total active agents across all keys
|
||||
- Modify `acquire()` to check global count against `maxBackgroundAgents` before granting a slot
|
||||
- Modify `release()` to decrement global count
|
||||
- Modify `clear()` to reset global count
|
||||
- Add `getGlobalCount()` for testing/debugging (follows existing `getCount()`/`getQueueLength()` pattern)
|
||||
|
||||
The global limit check happens **in addition to** the per-model limit. Both must have capacity for a task to proceed.
|
||||
|
||||
### Step 4: Add tests for the new config schema field
|
||||
|
||||
**File:** `src/config/schema/background-task.test.ts`
|
||||
|
||||
- Add test cases following the existing given/when/then pattern with nested describes
|
||||
- Test valid value, below-minimum value, undefined (not provided), non-number type
|
||||
|
||||
### Step 5: Add tests for ConcurrencyManager global limit
|
||||
|
||||
**File:** `src/features/background-agent/concurrency.test.ts`
|
||||
|
||||
- Test that global limit is enforced across different model keys
|
||||
- Test that tasks queue when global limit reached even if per-model limit has capacity
|
||||
- Test that releasing a slot from one model allows a queued task from another model to proceed
|
||||
- Test default behavior (5) when no config provided
|
||||
- Test interaction between global and per-model limits
|
||||
|
||||
### Step 6: Run typecheck and tests
|
||||
|
||||
```bash
|
||||
bun run typecheck
|
||||
bun test src/config/schema/background-task.test.ts
|
||||
bun test src/features/background-agent/concurrency.test.ts
|
||||
```
|
||||
|
||||
### Step 7: Verify LSP diagnostics clean
|
||||
|
||||
Check `src/config/schema/background-task.ts` and `src/features/background-agent/concurrency.ts` for errors.
|
||||
|
||||
### Step 8: Create PR
|
||||
|
||||
- Push branch to remote
|
||||
- Create PR with structured description via `gh pr create`
|
||||
|
||||
## Files Modified (4 files)
|
||||
|
||||
| File | Change |
|
||||
|------|--------|
|
||||
| `src/config/schema/background-task.ts` | Add `maxBackgroundAgents` field |
|
||||
| `src/features/background-agent/concurrency.ts` | Add global count tracking + enforcement |
|
||||
| `src/config/schema/background-task.test.ts` | Add schema validation tests |
|
||||
| `src/features/background-agent/concurrency.test.ts` | Add global limit enforcement tests |
|
||||
|
||||
## Files NOT Modified (intentional)
|
||||
|
||||
| File | Reason |
|
||||
|------|--------|
|
||||
| `src/config/schema/oh-my-opencode-config.ts` | No change needed - `BackgroundTaskConfigSchema` is already composed into root schema via `background_task` field |
|
||||
| `src/create-managers.ts` | No change needed - `pluginConfig.background_task` already passed to `BackgroundManager` constructor |
|
||||
| `src/features/background-agent/manager.ts` | No change needed - already passes config to `ConcurrencyManager` |
|
||||
| `src/plugin-config.ts` | No change needed - `background_task` is a simple object field, uses default override merge |
|
||||
| `src/config/schema.ts` | No change needed - barrel already exports `BackgroundTaskConfigSchema` |
|
||||
|
||||
## Design Decisions
|
||||
|
||||
1. **Field name `maxBackgroundAgents`** - camelCase to match existing schema fields (`maxDepth`, `maxDescendants`, `defaultConcurrency`). The user-facing JSONC config key is also camelCase per existing convention in `background_task` section.
|
||||
|
||||
2. **Global limit vs per-model limit** - The global limit is a ceiling across ALL concurrency keys. Per-model limits still apply independently. A task needs both a per-model slot AND a global slot to proceed.
|
||||
|
||||
3. **Default of 5** - Matches the existing hardcoded default in `getConcurrencyLimit()`. When `maxBackgroundAgents` is not set, no global limit is enforced (only per-model limits apply), preserving backward compatibility.
|
||||
|
||||
4. **Queue behavior** - When global limit is reached, tasks wait in the same FIFO queue mechanism. The global check happens inside `acquire()` before the per-model check.
|
||||
|
||||
5. **0 means Infinity** - Following the existing pattern where `defaultConcurrency: 0` means unlimited, `maxBackgroundAgents: 0` would also mean no global limit.
|
||||
+50
@@ -0,0 +1,50 @@
|
||||
# PR Description
|
||||
|
||||
**Title:** feat: add `maxBackgroundAgents` config to limit total simultaneous background agents
|
||||
|
||||
**Body:**
|
||||
|
||||
## Summary
|
||||
|
||||
- Add `maxBackgroundAgents` field to `BackgroundTaskConfigSchema` that enforces a global ceiling on total running background agents across all models/providers
|
||||
- Modify `ConcurrencyManager` to track global count and enforce the limit alongside existing per-model limits
|
||||
- Add schema validation tests and concurrency enforcement tests
|
||||
|
||||
## Motivation
|
||||
|
||||
Currently, concurrency is only limited per model/provider key (default 5 per key). On resource-constrained machines or when using many different models, the total number of background agents can grow unbounded (5 per model x N models). This config option lets users set a hard ceiling.
|
||||
|
||||
## Changes
|
||||
|
||||
### Schema (`src/config/schema/background-task.ts`)
|
||||
- Added `maxBackgroundAgents: z.number().int().min(1).optional()` to `BackgroundTaskConfigSchema`
|
||||
- Grouped with existing limit fields (`maxDepth`, `maxDescendants`)
|
||||
|
||||
### ConcurrencyManager (`src/features/background-agent/concurrency.ts`)
|
||||
- Added `globalCount` tracking total active agents across all concurrency keys
|
||||
- Added `getGlobalLimit()` reading `maxBackgroundAgents` from config (defaults to `Infinity` = no global limit)
|
||||
- Modified `acquire()` to check both per-model AND global capacity
|
||||
- Modified `release()` to decrement global count and drain cross-model waiters blocked by global limit
|
||||
- Modified `clear()` to reset global state
|
||||
- Added `getGlobalCount()` / `getGlobalQueueLength()` for testing
|
||||
|
||||
### Tests
|
||||
- `src/config/schema/background-task.test.ts`: 6 test cases for schema validation (valid, min boundary, below min, negative, non-integer, undefined)
|
||||
- `src/features/background-agent/concurrency.test.ts`: 8 test cases for global limit enforcement (cross-model blocking, release unblocking, per-model vs global interaction, no-config default, clear reset)
|
||||
|
||||
## Config Example
|
||||
|
||||
```jsonc
|
||||
{
|
||||
"background_task": {
|
||||
"maxBackgroundAgents": 5,
|
||||
"defaultConcurrency": 3
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
## Backward Compatibility
|
||||
|
||||
- When `maxBackgroundAgents` is not set (default), no global limit is enforced - behavior is identical to before
|
||||
- Existing `defaultConcurrency`, `providerConcurrency`, and `modelConcurrency` continue to work unchanged
|
||||
- No config migration needed
|
||||
+111
@@ -0,0 +1,111 @@
|
||||
# Verification Strategy
|
||||
|
||||
## 1. Static Analysis
|
||||
|
||||
### TypeScript Typecheck
|
||||
```bash
|
||||
bun run typecheck
|
||||
```
|
||||
- Verify no type errors introduced
|
||||
- `BackgroundTaskConfig` type is inferred from Zod schema, so adding the field automatically updates the type
|
||||
- All existing consumers of `BackgroundTaskConfig` remain compatible (new field is optional)
|
||||
|
||||
### LSP Diagnostics
|
||||
Check changed files for errors:
|
||||
- `src/config/schema/background-task.ts`
|
||||
- `src/features/background-agent/concurrency.ts`
|
||||
- `src/config/schema/background-task.test.ts`
|
||||
- `src/features/background-agent/concurrency.test.ts`
|
||||
|
||||
## 2. Unit Tests
|
||||
|
||||
### Schema Validation Tests
|
||||
```bash
|
||||
bun test src/config/schema/background-task.test.ts
|
||||
```
|
||||
|
||||
| Test Case | Input | Expected |
|
||||
|-----------|-------|----------|
|
||||
| Valid value (10) | `{ maxBackgroundAgents: 10 }` | Parses to `10` |
|
||||
| Minimum boundary (1) | `{ maxBackgroundAgents: 1 }` | Parses to `1` |
|
||||
| Below minimum (0) | `{ maxBackgroundAgents: 0 }` | Throws `ZodError` |
|
||||
| Negative (-1) | `{ maxBackgroundAgents: -1 }` | Throws `ZodError` |
|
||||
| Non-integer (2.5) | `{ maxBackgroundAgents: 2.5 }` | Throws `ZodError` |
|
||||
| Not provided | `{}` | Field is `undefined` |
|
||||
|
||||
### ConcurrencyManager Tests
|
||||
```bash
|
||||
bun test src/features/background-agent/concurrency.test.ts
|
||||
```
|
||||
|
||||
| Test Case | Setup | Expected |
|
||||
|-----------|-------|----------|
|
||||
| No config = no global limit | No `maxBackgroundAgents` | `getGlobalLimit()` returns `Infinity` |
|
||||
| Config respected | `maxBackgroundAgents: 3` | `getGlobalLimit()` returns `3` |
|
||||
| Cross-model blocking | Global limit 2, acquire model-a + model-b, try model-c | model-c blocks |
|
||||
| Under-limit allows | Global limit 3, acquire 3 different models | All succeed |
|
||||
| Per-model + global interaction | Per-model 1, global 3, acquire model-a twice | Blocked by per-model, not global |
|
||||
| Release unblocks | Global limit 1, acquire model-a, queue model-b, release model-a | model-b proceeds |
|
||||
| No global limit = no enforcement | No config, acquire 6 different models | All succeed |
|
||||
| Clear resets global count | Acquire 2, clear | `getGlobalCount()` is 0 |
|
||||
|
||||
### Existing Test Regression
|
||||
```bash
|
||||
bun test src/features/background-agent/concurrency.test.ts
|
||||
bun test src/config/schema/background-task.test.ts
|
||||
bun test src/config/schema.test.ts
|
||||
```
|
||||
All existing tests must continue to pass unchanged.
|
||||
|
||||
## 3. Integration Verification
|
||||
|
||||
### Config Loading Path
|
||||
Verify the config flows correctly through the system:
|
||||
|
||||
1. **Schema → Type**: `BackgroundTaskConfig` type auto-includes `maxBackgroundAgents` via `z.infer`
|
||||
2. **Config file → Schema**: `loadConfigFromPath()` in `plugin-config.ts` uses `OhMyOpenCodeConfigSchema.safeParse()` which includes `BackgroundTaskConfigSchema`
|
||||
3. **Config → Manager**: `create-managers.ts` passes `pluginConfig.background_task` to `BackgroundManager` constructor
|
||||
4. **Manager → ConcurrencyManager**: `BackgroundManager` constructor passes config to `new ConcurrencyManager(config)`
|
||||
5. **ConcurrencyManager → Enforcement**: `acquire()` reads `config.maxBackgroundAgents` via `getGlobalLimit()`
|
||||
|
||||
No changes needed in steps 2-4 since the field is optional and the existing plumbing passes the entire `BackgroundTaskConfig` object.
|
||||
|
||||
### Manual Config Test
|
||||
Create a test config to verify parsing:
|
||||
```bash
|
||||
echo '{ "background_task": { "maxBackgroundAgents": 3 } }' | bun -e "
|
||||
const { BackgroundTaskConfigSchema } = require('./src/config/schema/background-task');
|
||||
const result = BackgroundTaskConfigSchema.safeParse(JSON.parse(require('fs').readFileSync('/dev/stdin', 'utf-8')).background_task);
|
||||
console.log(result.success, result.data);
|
||||
"
|
||||
```
|
||||
|
||||
## 4. Build Verification
|
||||
|
||||
```bash
|
||||
bun run build
|
||||
```
|
||||
- Verify build succeeds
|
||||
- Schema JSON output includes the new field (if applicable)
|
||||
|
||||
## 5. Edge Cases to Verify
|
||||
|
||||
| Edge Case | Expected Behavior |
|
||||
|-----------|-------------------|
|
||||
| `maxBackgroundAgents` not set | No global limit enforced (backward compatible) |
|
||||
| `maxBackgroundAgents: 1` | Only 1 background agent at a time across all models |
|
||||
| `maxBackgroundAgents` > sum of all per-model limits | Global limit never triggers (per-model limits are tighter) |
|
||||
| Per-model limit tighter than global | Per-model limit blocks first |
|
||||
| Global limit tighter than per-model | Global limit blocks first |
|
||||
| Release from one model unblocks different model | Global slot freed, different model's waiter proceeds |
|
||||
| Manager shutdown with global waiters | `clear()` rejects all waiters and resets global count |
|
||||
| Concurrent acquire/release | No race conditions (single-threaded JS event loop) |
|
||||
|
||||
## 6. CI Pipeline
|
||||
|
||||
The existing CI workflow (`ci.yml`) will run:
|
||||
- `bun run typecheck` - type checking
|
||||
- `bun test` - all tests including new ones
|
||||
- `bun run build` - build verification
|
||||
|
||||
No CI changes needed.
|
||||
@@ -0,0 +1 @@
|
||||
{"total_tokens": null, "duration_ms": 365000, "total_duration_seconds": 365}
|
||||
Reference in New Issue
Block a user