fix(athena): address 6 council review findings — launcher, schema, filtering, presentation
- Forward temperature and permission through council-launcher to background manager - Add LaunchInput.temperature and LaunchInput.permission to background-agent types - Extract session guard with 5-minute timeout to prevent stale council locks - Make council optional in AthenaOverrideConfigSchema for partial user overrides - Support member lookup by both name and model ID in filterCouncilMembers - Add provider/model-id format validation to CouncilMemberSchema - Fix findings-presenter group header to show finding count instead of first finding's reporter count
This commit is contained in:
ismeth
@@ -0,0 +1,77 @@
|
||||
import { describe, expect, test } from "bun:test"
|
||||
import type { BackgroundManager } from "../../features/background-agent"
|
||||
import type { BackgroundTask, LaunchInput } from "../../features/background-agent/types"
|
||||
import { createCouncilLauncher } from "./council-launcher"
|
||||
|
||||
function createMockTask(id: string): BackgroundTask {
|
||||
return {
|
||||
id,
|
||||
parentSessionID: "session-1",
|
||||
parentMessageID: "message-1",
|
||||
description: "test",
|
||||
prompt: "test",
|
||||
agent: "athena",
|
||||
status: "running",
|
||||
}
|
||||
}
|
||||
|
||||
describe("createCouncilLauncher", () => {
|
||||
//#given a council launch input with temperature and permission
|
||||
//#when launch is called
|
||||
//#then temperature and permission are forwarded to the background manager
|
||||
test("forwards temperature and permission to background manager", async () => {
|
||||
const capturedInputs: LaunchInput[] = []
|
||||
const mockManager = {
|
||||
launch: async (input: LaunchInput) => {
|
||||
capturedInputs.push(input)
|
||||
return createMockTask("bg-1")
|
||||
},
|
||||
getTask: () => undefined,
|
||||
} as unknown as BackgroundManager
|
||||
|
||||
const launcher = createCouncilLauncher(mockManager)
|
||||
|
||||
await launcher.launch({
|
||||
description: "Council member: test",
|
||||
prompt: "Analyze this",
|
||||
agent: "athena",
|
||||
parentSessionID: "session-1",
|
||||
parentMessageID: "message-1",
|
||||
model: { providerID: "openai", modelID: "gpt-5.3-codex" },
|
||||
temperature: 0.3,
|
||||
permission: { write: "deny", edit: "deny", task: "deny" },
|
||||
})
|
||||
|
||||
expect(capturedInputs).toHaveLength(1)
|
||||
expect(capturedInputs[0]?.temperature).toBe(0.3)
|
||||
expect(capturedInputs[0]?.permission).toEqual({ write: "deny", edit: "deny", task: "deny" })
|
||||
})
|
||||
|
||||
//#given a council launch input without temperature and permission
|
||||
//#when launch is called
|
||||
//#then undefined temperature and permission are forwarded (not dropped)
|
||||
test("forwards undefined temperature and permission without error", async () => {
|
||||
const capturedInputs: LaunchInput[] = []
|
||||
const mockManager = {
|
||||
launch: async (input: LaunchInput) => {
|
||||
capturedInputs.push(input)
|
||||
return createMockTask("bg-2")
|
||||
},
|
||||
getTask: () => undefined,
|
||||
} as unknown as BackgroundManager
|
||||
|
||||
const launcher = createCouncilLauncher(mockManager)
|
||||
|
||||
await launcher.launch({
|
||||
description: "Council member: test",
|
||||
prompt: "Analyze this",
|
||||
agent: "athena",
|
||||
parentSessionID: "session-1",
|
||||
parentMessageID: "message-1",
|
||||
})
|
||||
|
||||
expect(capturedInputs).toHaveLength(1)
|
||||
expect(capturedInputs[0]?.temperature).toBeUndefined()
|
||||
expect(capturedInputs[0]?.permission).toBeUndefined()
|
||||
})
|
||||
})
|
||||
@@ -12,6 +12,8 @@ export function createCouncilLauncher(manager: BackgroundManager): CouncilLaunch
|
||||
parentMessageID: input.parentMessageID,
|
||||
parentAgent: input.parentAgent,
|
||||
model: input.model,
|
||||
temperature: input.temperature,
|
||||
permission: input.permission,
|
||||
})
|
||||
},
|
||||
}
|
||||
|
||||
@@ -0,0 +1,72 @@
|
||||
import { afterEach, describe, expect, test } from "bun:test"
|
||||
import {
|
||||
isCouncilRunning,
|
||||
markCouncilDone,
|
||||
markCouncilRunning,
|
||||
_resetForTesting,
|
||||
_setTimestampForTesting,
|
||||
} from "./session-guard"
|
||||
|
||||
afterEach(() => {
|
||||
_resetForTesting()
|
||||
})
|
||||
|
||||
describe("session-guard", () => {
|
||||
//#given no active sessions
|
||||
//#when isCouncilRunning is checked
|
||||
//#then returns false
|
||||
test("returns false for unknown session", () => {
|
||||
expect(isCouncilRunning("session-1")).toBe(false)
|
||||
})
|
||||
|
||||
//#given a session is marked as running
|
||||
//#when isCouncilRunning is checked
|
||||
//#then returns true
|
||||
test("returns true after markCouncilRunning", () => {
|
||||
markCouncilRunning("session-1")
|
||||
|
||||
expect(isCouncilRunning("session-1")).toBe(true)
|
||||
})
|
||||
|
||||
//#given a session was marked running then done
|
||||
//#when isCouncilRunning is checked
|
||||
//#then returns false
|
||||
test("returns false after markCouncilDone", () => {
|
||||
markCouncilRunning("session-1")
|
||||
markCouncilDone("session-1")
|
||||
|
||||
expect(isCouncilRunning("session-1")).toBe(false)
|
||||
})
|
||||
|
||||
//#given a session was marked running 6 minutes ago (past 5-minute timeout)
|
||||
//#when isCouncilRunning is checked
|
||||
//#then stale entry is purged and returns false
|
||||
test("purges stale entries older than 5 minutes", () => {
|
||||
const sixMinutesAgo = Date.now() - 6 * 60 * 1000
|
||||
_setTimestampForTesting("stale-session", sixMinutesAgo)
|
||||
|
||||
expect(isCouncilRunning("stale-session")).toBe(false)
|
||||
})
|
||||
|
||||
//#given a session was marked running 4 minutes ago (within 5-minute timeout)
|
||||
//#when isCouncilRunning is checked
|
||||
//#then entry is kept and returns true
|
||||
test("keeps entries within timeout window", () => {
|
||||
const fourMinutesAgo = Date.now() - 4 * 60 * 1000
|
||||
_setTimestampForTesting("recent-session", fourMinutesAgo)
|
||||
|
||||
expect(isCouncilRunning("recent-session")).toBe(true)
|
||||
})
|
||||
|
||||
//#given multiple sessions where one is stale and one is fresh
|
||||
//#when isCouncilRunning is checked for the fresh one
|
||||
//#then stale entry is purged but fresh entry remains
|
||||
test("purges only stale entries while keeping fresh ones", () => {
|
||||
const sixMinutesAgo = Date.now() - 6 * 60 * 1000
|
||||
_setTimestampForTesting("stale-session", sixMinutesAgo)
|
||||
markCouncilRunning("fresh-session")
|
||||
|
||||
expect(isCouncilRunning("stale-session")).toBe(false)
|
||||
expect(isCouncilRunning("fresh-session")).toBe(true)
|
||||
})
|
||||
})
|
||||
@@ -0,0 +1,37 @@
|
||||
/** Timeout in ms after which a stale council session lock is automatically released. */
|
||||
const COUNCIL_SESSION_TIMEOUT_MS = 5 * 60 * 1000
|
||||
|
||||
/** Tracks active council executions per session with timestamps for stale entry cleanup. */
|
||||
const activeCouncilSessions = new Map<string, number>()
|
||||
|
||||
function purgeStaleEntries(): void {
|
||||
const now = Date.now()
|
||||
for (const [sessionId, startedAt] of activeCouncilSessions) {
|
||||
if (now - startedAt > COUNCIL_SESSION_TIMEOUT_MS) {
|
||||
activeCouncilSessions.delete(sessionId)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
export function isCouncilRunning(sessionId: string): boolean {
|
||||
purgeStaleEntries()
|
||||
return activeCouncilSessions.has(sessionId)
|
||||
}
|
||||
|
||||
export function markCouncilRunning(sessionId: string): void {
|
||||
activeCouncilSessions.set(sessionId, Date.now())
|
||||
}
|
||||
|
||||
export function markCouncilDone(sessionId: string): void {
|
||||
activeCouncilSessions.delete(sessionId)
|
||||
}
|
||||
|
||||
/** Visible for testing only. */
|
||||
export function _resetForTesting(): void {
|
||||
activeCouncilSessions.clear()
|
||||
}
|
||||
|
||||
/** Visible for testing only. */
|
||||
export function _setTimestampForTesting(sessionId: string, timestamp: number): void {
|
||||
activeCouncilSessions.set(sessionId, timestamp)
|
||||
}
|
||||
@@ -97,6 +97,30 @@ describe("filterCouncilMembers", () => {
|
||||
)
|
||||
})
|
||||
|
||||
test("selects named member by model ID when name differs from model", () => {
|
||||
// #given - "Claude" has name "Claude" but model "anthropic/claude-sonnet-4-5"
|
||||
const selectedMembers = ["anthropic/claude-sonnet-4-5"]
|
||||
|
||||
// #when
|
||||
const result = filterCouncilMembers(configuredMembers, selectedMembers)
|
||||
|
||||
// #then - should find the member by model ID even though it has a custom name
|
||||
expect(result.members).toEqual([configuredMembers[0]])
|
||||
expect(result.error).toBeUndefined()
|
||||
})
|
||||
|
||||
test("deduplicates when same member is selected by both name and model", () => {
|
||||
// #given
|
||||
const selectedMembers = ["Claude", "anthropic/claude-sonnet-4-5"]
|
||||
|
||||
// #when
|
||||
const result = filterCouncilMembers(configuredMembers, selectedMembers)
|
||||
|
||||
// #then - should return only one copy
|
||||
expect(result.members).toEqual([configuredMembers[0]])
|
||||
expect(result.error).toBeUndefined()
|
||||
})
|
||||
|
||||
test("returns error listing only unmatched names when partially matched", () => {
|
||||
// #given
|
||||
const selectedMembers = ["claude", "non-existent"]
|
||||
|
||||
@@ -4,11 +4,9 @@ import type { CouncilConfig, CouncilMemberConfig } from "../../agents/athena/typ
|
||||
import type { BackgroundManager } from "../../features/background-agent"
|
||||
import { ATHENA_COUNCIL_TOOL_DESCRIPTION_TEMPLATE } from "./constants"
|
||||
import { createCouncilLauncher } from "./council-launcher"
|
||||
import { isCouncilRunning, markCouncilDone, markCouncilRunning } from "./session-guard"
|
||||
import type { AthenaCouncilLaunchResult, AthenaCouncilToolArgs } from "./types"
|
||||
|
||||
/** Tracks active council executions per session to prevent duplicate launches. */
|
||||
const activeCouncilSessions = new Set<string>()
|
||||
|
||||
function isCouncilConfigured(councilConfig: CouncilConfig | undefined): councilConfig is CouncilConfig {
|
||||
return Boolean(councilConfig && councilConfig.members.length > 0)
|
||||
}
|
||||
@@ -28,13 +26,15 @@ export function filterCouncilMembers(
|
||||
|
||||
const memberLookup = new Map<string, CouncilMemberConfig>()
|
||||
members.forEach((member) => {
|
||||
const key = (member.name ?? member.model).toLowerCase()
|
||||
memberLookup.set(key, member)
|
||||
memberLookup.set(member.model.toLowerCase(), member)
|
||||
if (member.name) {
|
||||
memberLookup.set(member.name.toLowerCase(), member)
|
||||
}
|
||||
})
|
||||
|
||||
const unresolved: string[] = []
|
||||
const filteredMembers: CouncilMemberConfig[] = []
|
||||
const includedMemberKeys = new Set<string>()
|
||||
const includedMembers = new Set<CouncilMemberConfig>()
|
||||
|
||||
selectedNames.forEach((selectedName) => {
|
||||
const selectedKey = selectedName.toLowerCase()
|
||||
@@ -44,12 +44,11 @@ export function filterCouncilMembers(
|
||||
return
|
||||
}
|
||||
|
||||
const memberKey = matchedMember.model
|
||||
if (includedMemberKeys.has(memberKey)) {
|
||||
if (includedMembers.has(matchedMember)) {
|
||||
return
|
||||
}
|
||||
|
||||
includedMemberKeys.add(memberKey)
|
||||
includedMembers.add(matchedMember)
|
||||
filteredMembers.push(matchedMember)
|
||||
})
|
||||
|
||||
@@ -98,11 +97,11 @@ export function createAthenaCouncilTool(args: {
|
||||
return filteredMembers.error
|
||||
}
|
||||
|
||||
if (activeCouncilSessions.has(toolContext.sessionID)) {
|
||||
if (isCouncilRunning(toolContext.sessionID)) {
|
||||
return "Council is already running for this session. Wait for the current council execution to complete."
|
||||
}
|
||||
|
||||
activeCouncilSessions.add(toolContext.sessionID)
|
||||
markCouncilRunning(toolContext.sessionID)
|
||||
try {
|
||||
const execution = await executeCouncil({
|
||||
question: toolArgs.question,
|
||||
@@ -132,10 +131,10 @@ export function createAthenaCouncilTool(args: {
|
||||
})),
|
||||
}
|
||||
|
||||
activeCouncilSessions.delete(toolContext.sessionID)
|
||||
markCouncilDone(toolContext.sessionID)
|
||||
return JSON.stringify(launchResult)
|
||||
} catch (error) {
|
||||
activeCouncilSessions.delete(toolContext.sessionID)
|
||||
markCouncilDone(toolContext.sessionID)
|
||||
throw error
|
||||
}
|
||||
},
|
||||
|
||||
Reference in New Issue
Block a user