fix: address second-round review findings
- Remove redundant `agent` re-declarations from trace subtypes - Change TraceEventBase.type from `string` to literal union type - Update onTrace callback type to `=> void | Promise<void>` - Add readonly to onProgress/onTrace on OrchestratorConfig - Remove Date.now() conditional guards (always capture timing) - Auto-generate runId fallback when onTrace is set without runId - Remove emitTrace from public API surface (keep generateRunId) - Add TODO comments for runParallel()/runAny() trace forwarding
This commit is contained in:
parent
8f7fc2019b
commit
e696d877e7
|
|
@ -32,7 +32,7 @@ import type {
|
||||||
TokenUsage,
|
TokenUsage,
|
||||||
ToolUseContext,
|
ToolUseContext,
|
||||||
} from '../types.js'
|
} from '../types.js'
|
||||||
import { emitTrace } from '../utils/trace.js'
|
import { emitTrace, generateRunId } from '../utils/trace.js'
|
||||||
import type { ToolDefinition as FrameworkToolDefinition, ToolRegistry } from '../tool/framework.js'
|
import type { ToolDefinition as FrameworkToolDefinition, ToolRegistry } from '../tool/framework.js'
|
||||||
import type { ToolExecutor } from '../tool/executor.js'
|
import type { ToolExecutor } from '../tool/executor.js'
|
||||||
import { createAdapter } from '../llm/adapter.js'
|
import { createAdapter } from '../llm/adapter.js'
|
||||||
|
|
@ -275,7 +275,7 @@ export class Agent {
|
||||||
): Promise<AgentRunResult> {
|
): Promise<AgentRunResult> {
|
||||||
this.transitionTo('running')
|
this.transitionTo('running')
|
||||||
|
|
||||||
const agentStartMs = callerOptions?.onTrace ? Date.now() : 0
|
const agentStartMs = Date.now()
|
||||||
|
|
||||||
try {
|
try {
|
||||||
const runner = await this.getRunner()
|
const runner = await this.getRunner()
|
||||||
|
|
@ -283,9 +283,12 @@ export class Agent {
|
||||||
this.state.messages.push(msg)
|
this.state.messages.push(msg)
|
||||||
callerOptions?.onMessage?.(msg)
|
callerOptions?.onMessage?.(msg)
|
||||||
}
|
}
|
||||||
|
// Auto-generate runId when onTrace is provided but runId is missing
|
||||||
|
const needsRunId = callerOptions?.onTrace && !callerOptions.runId
|
||||||
const runOptions: RunOptions = {
|
const runOptions: RunOptions = {
|
||||||
...callerOptions,
|
...callerOptions,
|
||||||
onMessage: internalOnMessage,
|
onMessage: internalOnMessage,
|
||||||
|
...(needsRunId ? { runId: generateRunId() } : undefined),
|
||||||
}
|
}
|
||||||
|
|
||||||
const result = await runner.run(messages, runOptions)
|
const result = await runner.run(messages, runOptions)
|
||||||
|
|
|
||||||
|
|
@ -149,6 +149,7 @@ export class AgentPool {
|
||||||
*
|
*
|
||||||
* @param tasks - Array of `{ agent, prompt }` descriptors.
|
* @param tasks - Array of `{ agent, prompt }` descriptors.
|
||||||
*/
|
*/
|
||||||
|
// TODO(#18): accept RunOptions per task to forward trace context
|
||||||
async runParallel(
|
async runParallel(
|
||||||
tasks: ReadonlyArray<{ readonly agent: string; readonly prompt: string }>,
|
tasks: ReadonlyArray<{ readonly agent: string; readonly prompt: string }>,
|
||||||
): Promise<Map<string, AgentRunResult>> {
|
): Promise<Map<string, AgentRunResult>> {
|
||||||
|
|
@ -187,6 +188,7 @@ export class AgentPool {
|
||||||
*
|
*
|
||||||
* @throws {Error} If the pool is empty.
|
* @throws {Error} If the pool is empty.
|
||||||
*/
|
*/
|
||||||
|
// TODO(#18): accept RunOptions to forward trace context
|
||||||
async runAny(prompt: string): Promise<AgentRunResult> {
|
async runAny(prompt: string): Promise<AgentRunResult> {
|
||||||
const allAgents = this.list()
|
const allAgents = this.list()
|
||||||
if (allAgents.length === 0) {
|
if (allAgents.length === 0) {
|
||||||
|
|
|
||||||
|
|
@ -78,8 +78,8 @@ export interface RunOptions {
|
||||||
readonly onToolResult?: (name: string, result: ToolResult) => void
|
readonly onToolResult?: (name: string, result: ToolResult) => void
|
||||||
/** Fired after each complete {@link LLMMessage} is appended. */
|
/** Fired after each complete {@link LLMMessage} is appended. */
|
||||||
readonly onMessage?: (message: LLMMessage) => void
|
readonly onMessage?: (message: LLMMessage) => void
|
||||||
/** Trace callback for observability spans. */
|
/** Trace callback for observability spans. Async callbacks are safe. */
|
||||||
readonly onTrace?: (event: TraceEvent) => void
|
readonly onTrace?: (event: TraceEvent) => void | Promise<void>
|
||||||
/** Run ID for trace correlation. */
|
/** Run ID for trace correlation. */
|
||||||
readonly runId?: string
|
readonly runId?: string
|
||||||
/** Task ID for trace correlation. */
|
/** Task ID for trace correlation. */
|
||||||
|
|
@ -264,16 +264,15 @@ export class AgentRunner {
|
||||||
// ------------------------------------------------------------------
|
// ------------------------------------------------------------------
|
||||||
// Step 1: Call the LLM and collect the full response for this turn.
|
// Step 1: Call the LLM and collect the full response for this turn.
|
||||||
// ------------------------------------------------------------------
|
// ------------------------------------------------------------------
|
||||||
const llmStartMs = options.onTrace ? Date.now() : 0
|
const llmStartMs = Date.now()
|
||||||
const response = await this.adapter.chat(conversationMessages, baseChatOptions)
|
const response = await this.adapter.chat(conversationMessages, baseChatOptions)
|
||||||
if (options.onTrace) {
|
if (options.onTrace) {
|
||||||
const llmEndMs = Date.now()
|
const llmEndMs = Date.now()
|
||||||
const agentName = options.traceAgent ?? this.options.agentName ?? 'unknown'
|
|
||||||
emitTrace(options.onTrace, {
|
emitTrace(options.onTrace, {
|
||||||
type: 'llm_call',
|
type: 'llm_call',
|
||||||
runId: options.runId ?? '',
|
runId: options.runId ?? '',
|
||||||
taskId: options.taskId,
|
taskId: options.taskId,
|
||||||
agent: agentName,
|
agent: options.traceAgent ?? this.options.agentName ?? 'unknown',
|
||||||
model: this.options.model,
|
model: this.options.model,
|
||||||
turn: turns,
|
turn: turns,
|
||||||
tokens: response.usage,
|
tokens: response.usage,
|
||||||
|
|
@ -352,12 +351,11 @@ export class AgentRunner {
|
||||||
options.onToolResult?.(block.name, result)
|
options.onToolResult?.(block.name, result)
|
||||||
|
|
||||||
if (options.onTrace) {
|
if (options.onTrace) {
|
||||||
const agentName = options.traceAgent ?? this.options.agentName ?? 'unknown'
|
|
||||||
emitTrace(options.onTrace, {
|
emitTrace(options.onTrace, {
|
||||||
type: 'tool_call',
|
type: 'tool_call',
|
||||||
runId: options.runId ?? '',
|
runId: options.runId ?? '',
|
||||||
taskId: options.taskId,
|
taskId: options.taskId,
|
||||||
agent: agentName,
|
agent: options.traceAgent ?? this.options.agentName ?? 'unknown',
|
||||||
tool: block.name,
|
tool: block.name,
|
||||||
isError: result.isError ?? false,
|
isError: result.isError ?? false,
|
||||||
startMs: startTime,
|
startMs: startTime,
|
||||||
|
|
|
||||||
|
|
@ -162,6 +162,7 @@ export type {
|
||||||
OrchestratorEvent,
|
OrchestratorEvent,
|
||||||
|
|
||||||
// Trace
|
// Trace
|
||||||
|
TraceEventType,
|
||||||
TraceEventBase,
|
TraceEventBase,
|
||||||
TraceEvent,
|
TraceEvent,
|
||||||
LLMCallTrace,
|
LLMCallTrace,
|
||||||
|
|
@ -174,4 +175,4 @@ export type {
|
||||||
MemoryStore,
|
MemoryStore,
|
||||||
} from './types.js'
|
} from './types.js'
|
||||||
|
|
||||||
export { emitTrace, generateRunId } from './utils/trace.js'
|
export { generateRunId } from './utils/trace.js'
|
||||||
|
|
|
||||||
13
src/types.ts
13
src/types.ts
|
|
@ -315,19 +315,22 @@ export interface OrchestratorConfig {
|
||||||
readonly defaultProvider?: 'anthropic' | 'copilot' | 'openai'
|
readonly defaultProvider?: 'anthropic' | 'copilot' | 'openai'
|
||||||
readonly defaultBaseURL?: string
|
readonly defaultBaseURL?: string
|
||||||
readonly defaultApiKey?: string
|
readonly defaultApiKey?: string
|
||||||
onProgress?: (event: OrchestratorEvent) => void
|
readonly onProgress?: (event: OrchestratorEvent) => void
|
||||||
onTrace?: (event: TraceEvent) => void
|
readonly onTrace?: (event: TraceEvent) => void | Promise<void>
|
||||||
}
|
}
|
||||||
|
|
||||||
// ---------------------------------------------------------------------------
|
// ---------------------------------------------------------------------------
|
||||||
// Trace events — lightweight observability spans
|
// Trace events — lightweight observability spans
|
||||||
// ---------------------------------------------------------------------------
|
// ---------------------------------------------------------------------------
|
||||||
|
|
||||||
|
/** Trace event type discriminants. */
|
||||||
|
export type TraceEventType = 'llm_call' | 'tool_call' | 'task' | 'agent'
|
||||||
|
|
||||||
/** Shared fields present on every trace event. */
|
/** Shared fields present on every trace event. */
|
||||||
export interface TraceEventBase {
|
export interface TraceEventBase {
|
||||||
/** Unique identifier for the entire run (runTeam / runTasks / runAgent call). */
|
/** Unique identifier for the entire run (runTeam / runTasks / runAgent call). */
|
||||||
readonly runId: string
|
readonly runId: string
|
||||||
readonly type: string
|
readonly type: TraceEventType
|
||||||
/** Unix epoch ms when the span started. */
|
/** Unix epoch ms when the span started. */
|
||||||
readonly startMs: number
|
readonly startMs: number
|
||||||
/** Unix epoch ms when the span ended. */
|
/** Unix epoch ms when the span ended. */
|
||||||
|
|
@ -343,7 +346,6 @@ export interface TraceEventBase {
|
||||||
/** Emitted for each LLM API call (one per agent turn). */
|
/** Emitted for each LLM API call (one per agent turn). */
|
||||||
export interface LLMCallTrace extends TraceEventBase {
|
export interface LLMCallTrace extends TraceEventBase {
|
||||||
readonly type: 'llm_call'
|
readonly type: 'llm_call'
|
||||||
readonly agent: string
|
|
||||||
readonly model: string
|
readonly model: string
|
||||||
readonly turn: number
|
readonly turn: number
|
||||||
readonly tokens: TokenUsage
|
readonly tokens: TokenUsage
|
||||||
|
|
@ -352,7 +354,6 @@ export interface LLMCallTrace extends TraceEventBase {
|
||||||
/** Emitted for each tool execution. */
|
/** Emitted for each tool execution. */
|
||||||
export interface ToolCallTrace extends TraceEventBase {
|
export interface ToolCallTrace extends TraceEventBase {
|
||||||
readonly type: 'tool_call'
|
readonly type: 'tool_call'
|
||||||
readonly agent: string
|
|
||||||
readonly tool: string
|
readonly tool: string
|
||||||
readonly isError: boolean
|
readonly isError: boolean
|
||||||
}
|
}
|
||||||
|
|
@ -362,7 +363,6 @@ export interface TaskTrace extends TraceEventBase {
|
||||||
readonly type: 'task'
|
readonly type: 'task'
|
||||||
readonly taskId: string
|
readonly taskId: string
|
||||||
readonly taskTitle: string
|
readonly taskTitle: string
|
||||||
readonly agent: string
|
|
||||||
readonly success: boolean
|
readonly success: boolean
|
||||||
readonly retries: number
|
readonly retries: number
|
||||||
}
|
}
|
||||||
|
|
@ -370,7 +370,6 @@ export interface TaskTrace extends TraceEventBase {
|
||||||
/** Emitted when an agent run completes (wraps the full conversation loop). */
|
/** Emitted when an agent run completes (wraps the full conversation loop). */
|
||||||
export interface AgentTrace extends TraceEventBase {
|
export interface AgentTrace extends TraceEventBase {
|
||||||
readonly type: 'agent'
|
readonly type: 'agent'
|
||||||
readonly agent: string
|
|
||||||
readonly turns: number
|
readonly turns: number
|
||||||
readonly tokens: TokenUsage
|
readonly tokens: TokenUsage
|
||||||
readonly toolCalls: number
|
readonly toolCalls: number
|
||||||
|
|
|
||||||
|
|
@ -10,7 +10,7 @@ import type { TraceEvent } from '../types.js'
|
||||||
* subscriber never crashes agent execution.
|
* subscriber never crashes agent execution.
|
||||||
*/
|
*/
|
||||||
export function emitTrace(
|
export function emitTrace(
|
||||||
fn: ((event: TraceEvent) => void) | undefined,
|
fn: ((event: TraceEvent) => void | Promise<void>) | undefined,
|
||||||
event: TraceEvent,
|
event: TraceEvent,
|
||||||
): void {
|
): void {
|
||||||
if (!fn) return
|
if (!fn) return
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue