Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion defaults/assistant.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -762,7 +762,6 @@ steps:
- "outputs['build-config'] != null"
guarantee: "(output?.text ?? '').length > 0"
ai:
timeout_mode: probe
max_iterations: "{{ inputs.max_iterations }}"
enableDelegate: "{{ inputs.enableDelegate }}"
enableTasks: "{{ inputs.enableTasks }}"
Expand Down
2 changes: 0 additions & 2 deletions defaults/code-talk.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,6 @@ steps:
guarantee: "(output?.projects?.length ?? 0) > 0 || (output?.notes?.length ?? 0) > 0"
fail_if: "(output?.projects?.length ?? 0) === 0 && !(output?.notes?.length > 0)"
ai:
timeout_mode: probe
skip_code_context: true
prompt_type: general
system_prompt: |
Expand Down Expand Up @@ -471,7 +470,6 @@ steps:
assume:
- "outputs['route-projects']?.projects?.length > 0"
ai:
timeout_mode: probe
skip_code_context: true
enableDelegate: true
enableExecutePlan: false
Expand Down
65 changes: 40 additions & 25 deletions src/ai-review-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,21 @@ import { processDiffWithOutline } from './utils/diff-processor';
import { shouldFilterVisorReviewComment } from './utils/comment-metadata';
import { extractFileSections, replaceFileSections } from './slack/markdown';

/**
* Grace period (ms) subtracted from Visor's hard timeout to derive Probe's
* maxOperationTimeout. This gives Probe enough headroom to inject its
* "TIME LIMIT REACHED" message and run 4 bonus wind-down steps before
* Visor's external Promise.race fires.
*/
const PROBE_GRACEFUL_MARGIN_MS = 90_000;

/**
* Minimum Visor timeout (ms) required before we subtract the grace margin.
* If the timeout is shorter than this, the full value is forwarded to Probe
* as-is because there isn't enough room for a meaningful margin.
*/
const MIN_TIMEOUT_FOR_MARGIN_MS = PROBE_GRACEFUL_MARGIN_MS + 30_000; // 120 000

/**
* Helper function to log debug messages using the centralized logger
*/
Expand Down Expand Up @@ -288,10 +303,10 @@ export interface AIReviewConfig {
completionPrompt?: string;
/** Shared concurrency limiter for global AI call gating */
concurrencyLimiter?: any;
// Timeout mode: 'probe' lets Probe handle timeout with graceful wind-down,
// 'visor' uses Visor's external Promise.race hard kill (legacy behavior).
// Default: 'probe'
timeoutMode?: 'probe' | 'visor';
// Probe-level timeout (maxOperationTimeout) for graceful wind-down.
// When set, Probe will inject "TIME LIMIT REACHED" and give bonus steps before hard abort.
// Defaults to timeout - 90s when not explicitly set.
aiTimeout?: number;
}

export interface AIDebugInfo {
Expand Down Expand Up @@ -500,14 +515,11 @@ export class AIReviewService {
try {
const call = this.callProbeAgent(prompt, schema, debugInfo, checkName, sessionId);
const timeoutMs = Math.max(0, this.config.timeout || 0);
const useProbeTimeout = (this.config.timeoutMode || 'probe') === 'probe';
const {
response,
effectiveSchema,
sessionId: usedSessionId,
} = timeoutMs > 0 && !useProbeTimeout
? await this.withTimeout(call, timeoutMs, 'AI review')
: await call;
} = timeoutMs > 0 ? await this.withTimeout(call, timeoutMs, 'AI review') : await call;
const processingTime = Date.now() - startTime;

if (debugInfo) {
Expand Down Expand Up @@ -669,11 +681,8 @@ export class AIReviewService {
checkName
);
const timeoutMs = Math.max(0, this.config.timeout || 0);
const useProbeTimeout = (this.config.timeoutMode || 'probe') === 'probe';
const { response, effectiveSchema } =
timeoutMs > 0 && !useProbeTimeout
? await this.withTimeout(call, timeoutMs, 'AI review (session)')
: await call;
timeoutMs > 0 ? await this.withTimeout(call, timeoutMs, 'AI review (session)') : await call;
const processingTime = Date.now() - startTime;

if (debugInfo) {
Expand Down Expand Up @@ -1453,11 +1462,15 @@ ${this.escapeXml(processedFallbackDiff)}
log(`⚙️ Model: ${this.config.model || 'default'}, Provider: ${this.config.provider || 'auto'}`);

// Update maxOperationTimeout for this reuse call (timeout may differ from original)
// Use explicit aiTimeout if set, otherwise default to timeout - 90s
const reuseTimeoutMs = this.config.timeout || 0;
if (reuseTimeoutMs > 120000) {
(agent as any).maxOperationTimeout = reuseTimeoutMs - 90000;
} else if (reuseTimeoutMs > 0) {
(agent as any).maxOperationTimeout = reuseTimeoutMs;
const reuseAiTimeout =
this.config.aiTimeout ||
(reuseTimeoutMs > MIN_TIMEOUT_FOR_MARGIN_MS
? reuseTimeoutMs - PROBE_GRACEFUL_MARGIN_MS
: reuseTimeoutMs);
if (reuseAiTimeout > 0) {
(agent as any).maxOperationTimeout = reuseAiTimeout;
}

try {
Expand Down Expand Up @@ -1975,15 +1988,17 @@ ${'='.repeat(60)}
(options as any).allowEdit = this.config.allowEdit;
}

// Wire Visor's check timeout into Probe's graceful timeout mechanism.
// Set maxOperationTimeout 90s before Visor's hard kill so Probe can wind
// down (4 bonus steps + 60s safety net) before the external Promise.race fires.
const timeoutMs = this.config.timeout || 0;
if (timeoutMs > 120000) {
(options as any).maxOperationTimeout = timeoutMs - 90000;
} else if (timeoutMs > 0) {
// Too short for graceful wind-down; let Probe use the full timeout
(options as any).maxOperationTimeout = timeoutMs;
// Wire Probe's graceful timeout (maxOperationTimeout).
// Use explicit aiTimeout if set, otherwise default to timeout - 90s so Probe's
// wind-down (4 bonus steps + 60s safety net) fires before Visor's hard kill.
const visorTimeout = this.config.timeout || 0;
const aiTimeout =
this.config.aiTimeout ||
(visorTimeout > MIN_TIMEOUT_FOR_MARGIN_MS
? visorTimeout - PROBE_GRACEFUL_MARGIN_MS
: visorTimeout);
if (aiTimeout > 0) {
(options as any).maxOperationTimeout = aiTimeout;
}

// Pass tool filtering options to ProbeAgent (native in rc168+)
Expand Down
19 changes: 9 additions & 10 deletions src/generated/config-schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1071,7 +1071,7 @@ export const configSchema = {
description: 'Arguments/inputs for the workflow',
},
overrides: {
$ref: '#/definitions/Record%3Cstring%2CPartial%3Cinterface-src_types_config.ts-14812-29498-src_types_config.ts-0-58065%3E%3E',
$ref: '#/definitions/Record%3Cstring%2CPartial%3Cinterface-src_types_config.ts-14886-29572-src_types_config.ts-0-58139%3E%3E',
description: 'Override specific step configurations in the workflow',
},
output_mapping: {
Expand All @@ -1088,7 +1088,7 @@ export const configSchema = {
'Config file path - alternative to workflow ID (loads a Visor config file as workflow)',
},
workflow_overrides: {
$ref: '#/definitions/Record%3Cstring%2CPartial%3Cinterface-src_types_config.ts-14812-29498-src_types_config.ts-0-58065%3E%3E',
$ref: '#/definitions/Record%3Cstring%2CPartial%3Cinterface-src_types_config.ts-14886-29572-src_types_config.ts-0-58139%3E%3E',
description: 'Alias for overrides - workflow step overrides (backward compatibility)',
},
ref: {
Expand Down Expand Up @@ -1407,11 +1407,10 @@ export const configSchema = {
description:
'Enable the execute_plan DSL orchestration tool (replaces analyze_all when enabled)',
},
timeout_mode: {
type: 'string',
enum: ['probe', 'visor'],
ai_timeout: {
type: 'number',
description:
"Timeout mode: 'probe' lets Probe handle timeout with graceful wind-down (injects \"TIME LIMIT REACHED\" message and bonus steps), 'visor' uses Visor's external Promise.race hard kill (legacy behavior). Default: 'probe'",
'Probe-level timeout in milliseconds for graceful wind-down (maxOperationTimeout). When set, Probe injects "TIME LIMIT REACHED" and gives bonus steps before hard abort. Defaults to (timeout - 90s) when not explicitly set. The main `timeout` field controls Visor\'s external hard kill (always active).',
},
},
additionalProperties: false,
Expand Down Expand Up @@ -1810,7 +1809,7 @@ export const configSchema = {
description: 'Custom output name (defaults to workflow name)',
},
overrides: {
$ref: '#/definitions/Record%3Cstring%2CPartial%3Cinterface-src_types_config.ts-14812-29498-src_types_config.ts-0-58065%3E%3E',
$ref: '#/definitions/Record%3Cstring%2CPartial%3Cinterface-src_types_config.ts-14886-29572-src_types_config.ts-0-58139%3E%3E',
description: 'Step overrides',
},
output_mapping: {
Expand All @@ -1825,14 +1824,14 @@ export const configSchema = {
'^x-': {},
},
},
'Record<string,Partial<interface-src_types_config.ts-14812-29498-src_types_config.ts-0-58065>>':
'Record<string,Partial<interface-src_types_config.ts-14886-29572-src_types_config.ts-0-58139>>':
{
type: 'object',
additionalProperties: {
$ref: '#/definitions/Partial%3Cinterface-src_types_config.ts-14812-29498-src_types_config.ts-0-58065%3E',
$ref: '#/definitions/Partial%3Cinterface-src_types_config.ts-14886-29572-src_types_config.ts-0-58139%3E',
},
},
'Partial<interface-src_types_config.ts-14812-29498-src_types_config.ts-0-58065>': {
'Partial<interface-src_types_config.ts-14886-29572-src_types_config.ts-0-58139>': {
type: 'object',
additionalProperties: false,
},
Expand Down
5 changes: 3 additions & 2 deletions src/providers/ai-check-provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -843,8 +843,9 @@ export class AICheckProvider extends CheckProvider {
const resolvedTimeout = (await resolveLiquid(aiAny.timeout)) ?? aiAny.timeout;
aiConfig.timeout = Number(resolvedTimeout);
}
if (aiAny.timeout_mode !== undefined) {
aiConfig.timeoutMode = aiAny.timeout_mode as 'probe' | 'visor';
if (aiAny.ai_timeout !== undefined) {
const resolvedAiTimeout = (await resolveLiquid(aiAny.ai_timeout)) ?? aiAny.ai_timeout;
aiConfig.aiTimeout = Number(resolvedAiTimeout);
}
if (aiAny.max_iterations !== undefined || aiAny.maxIterations !== undefined) {
const raw = aiAny.max_iterations ?? aiAny.maxIterations;
Expand Down
6 changes: 6 additions & 0 deletions src/providers/check-provider.interface.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,12 @@ export interface ExecutionContext {
/** reset per-run guard state before grouped execution */
resetPerRunState?: boolean;
};
/**
* Absolute timestamp (ms since epoch) by which this execution must complete.
* Set by the engine from `Date.now() + timeout` and inherited by sub-workflows
* so nested steps know how much time the parent has left.
*/
deadline?: number;
/** Optional event bus for emitting integration events (e.g., HumanInputRequested) */
eventBus?: import('../event-bus/event-bus').EventBus;
/** Optional webhook context (e.g., Slack Events API payload) */
Expand Down
19 changes: 17 additions & 2 deletions src/state-machine/dispatch/execution-invoker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -614,6 +614,19 @@ export async function executeSingleCheck(
require('../../providers/check-provider-registry').CheckProviderRegistry.getInstance();
const provider = providerRegistry.getProviderOrThrow(providerType);

// Compute effective timeout, capped by any inherited parent deadline.
const configTimeout = checkConfig.timeout || checkConfig.ai?.timeout || 1800000;
const parentDeadline = context.executionContext?.deadline;
let effectiveTimeout = configTimeout;
if (parentDeadline) {
const remaining = parentDeadline - Date.now();
if (remaining <= 0) {
throw new Error(`Parent deadline exceeded: no time remaining for check '${checkId}'`);
}
effectiveTimeout = Math.min(effectiveTimeout, remaining);
}
const deadline = Date.now() + effectiveTimeout;

const outputHistory = buildOutputHistoryFromJournal(context);
// Resolve workflow inputs from config or context (centralized logic)
const workflowInputs = resolveWorkflowInputs(checkConfig, context);
Expand All @@ -638,7 +651,7 @@ export async function executeSingleCheck(
workflowInputs,
ai: {
...(checkConfig.ai || {}),
timeout: checkConfig.timeout || checkConfig.ai?.timeout || 1800000,
timeout: effectiveTimeout,
debug: !!context.debug,
},
};
Expand Down Expand Up @@ -695,6 +708,8 @@ export async function executeSingleCheck(
_parentState: state,
// Explicitly propagate workspace reference for nested workflows
workspace: context.workspace,
// Propagate deadline so sub-workflows can cap their own timeouts
deadline,
};

// Attach session reuse hints for providers that support them (AI, Claude Code, etc).
Expand Down Expand Up @@ -759,7 +774,7 @@ export async function executeSingleCheck(
context,
prInfo,
dependencyResults,
checkConfig.timeout || checkConfig.ai?.timeout || 1800000,
effectiveTimeout,
() => provider.execute(prInfo, providerConfig, dependencyResults, executionContext)
);
// Capture output in span for trace visualization
Expand Down
9 changes: 5 additions & 4 deletions src/types/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -398,11 +398,12 @@ export interface AIProviderConfig {
/** Enable the execute_plan DSL orchestration tool (replaces analyze_all when enabled) */
enableExecutePlan?: boolean;
/**
* Timeout mode: 'probe' lets Probe handle timeout with graceful wind-down
* (injects "TIME LIMIT REACHED" message and bonus steps), 'visor' uses
* Visor's external Promise.race hard kill (legacy behavior). Default: 'probe'
* Probe-level timeout in milliseconds for graceful wind-down (maxOperationTimeout).
* When set, Probe injects "TIME LIMIT REACHED" and gives bonus steps before hard abort.
* Defaults to (timeout - 90s) when not explicitly set.
* The main `timeout` field controls Visor's external hard kill (always active).
*/
timeout_mode?: 'probe' | 'visor';
ai_timeout?: number;
}

/**
Expand Down
107 changes: 107 additions & 0 deletions tests/unit/ai-timeout-and-graceful-margin.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
/**
* Tests for ai_timeout config field and PROBE_GRACEFUL_MARGIN_MS / MIN_TIMEOUT_FOR_MARGIN_MS
* constants used when computing Probe's maxOperationTimeout from Visor's hard timeout.
*/

// These constants mirror the values in src/ai-review-service.ts
const PROBE_GRACEFUL_MARGIN_MS = 90_000;
const MIN_TIMEOUT_FOR_MARGIN_MS = PROBE_GRACEFUL_MARGIN_MS + 30_000; // 120_000

/**
* Mirrors the maxOperationTimeout derivation logic from ai-review-service.ts
*/
function deriveProbeTimeout(visorTimeout: number, aiTimeout?: number): number {
return (
aiTimeout ||
(visorTimeout > MIN_TIMEOUT_FOR_MARGIN_MS
? visorTimeout - PROBE_GRACEFUL_MARGIN_MS
: visorTimeout)
);
}

describe('ai_timeout and graceful margin', () => {
describe('deriveProbeTimeout (mirrors ai-review-service logic)', () => {
it('should use explicit aiTimeout when provided', () => {
expect(deriveProbeTimeout(1800000, 600000)).toBe(600000);
});

it('should use explicit aiTimeout even when shorter than visor timeout', () => {
expect(deriveProbeTimeout(1800000, 30000)).toBe(30000);
});

it('should use explicit aiTimeout even when longer than visor timeout', () => {
// User might want Probe to run longer than Visor's hard kill
// (Visor's Promise.race will still terminate, but Probe starts winding down later)
expect(deriveProbeTimeout(60000, 120000)).toBe(120000);
});

it('should subtract margin when visor timeout > MIN_TIMEOUT_FOR_MARGIN_MS', () => {
// 30 minutes → 30min - 90s = 1710000
expect(deriveProbeTimeout(1800000)).toBe(1800000 - PROBE_GRACEFUL_MARGIN_MS);
expect(deriveProbeTimeout(1800000)).toBe(1710000);
});

it('should use full visor timeout when at exactly MIN_TIMEOUT_FOR_MARGIN_MS', () => {
// At exactly 120s, condition is >, so it does NOT subtract
expect(deriveProbeTimeout(MIN_TIMEOUT_FOR_MARGIN_MS)).toBe(MIN_TIMEOUT_FOR_MARGIN_MS);
expect(deriveProbeTimeout(120000)).toBe(120000);
});

it('should use full visor timeout when below MIN_TIMEOUT_FOR_MARGIN_MS', () => {
expect(deriveProbeTimeout(60000)).toBe(60000);
expect(deriveProbeTimeout(30000)).toBe(30000);
expect(deriveProbeTimeout(1000)).toBe(1000);
});

it('should use full visor timeout when just above MIN_TIMEOUT_FOR_MARGIN_MS', () => {
// 120001ms → subtracts margin
expect(deriveProbeTimeout(MIN_TIMEOUT_FOR_MARGIN_MS + 1)).toBe(
MIN_TIMEOUT_FOR_MARGIN_MS + 1 - PROBE_GRACEFUL_MARGIN_MS
);
// = 120001 - 90000 = 30001
expect(deriveProbeTimeout(120001)).toBe(30001);
});

it('should handle zero visor timeout', () => {
expect(deriveProbeTimeout(0)).toBe(0);
});

it('should prefer explicit aiTimeout=0 over default derivation', () => {
// aiTimeout=0 is falsy, so falls through to default derivation
// This is by design: 0 means "not set"
expect(deriveProbeTimeout(1800000, 0)).toBe(1710000);
});
});

describe('constant relationships', () => {
it('PROBE_GRACEFUL_MARGIN_MS should be 90 seconds', () => {
expect(PROBE_GRACEFUL_MARGIN_MS).toBe(90_000);
});

it('MIN_TIMEOUT_FOR_MARGIN_MS should be margin + 30s headroom', () => {
expect(MIN_TIMEOUT_FOR_MARGIN_MS).toBe(PROBE_GRACEFUL_MARGIN_MS + 30_000);
});

it('margin should leave at least 30s for Probe when subtracting', () => {
// The minimum visor timeout that triggers subtraction is MIN_TIMEOUT_FOR_MARGIN_MS + 1
const minSubtractedResult = MIN_TIMEOUT_FOR_MARGIN_MS + 1 - PROBE_GRACEFUL_MARGIN_MS;
expect(minSubtractedResult).toBeGreaterThanOrEqual(30_000);
});
});

describe('integration: aiTimeout overrides default derivation', () => {
it('should allow user to set precise Probe timeout independent of Visor', () => {
// User sets visor timeout=600s, ai_timeout=300s
// Probe winds down at 300s, Visor hard kills at 600s
const probeTimeout = deriveProbeTimeout(600000, 300000);
expect(probeTimeout).toBe(300000);
// Without ai_timeout, would be 600000 - 90000 = 510000
expect(deriveProbeTimeout(600000)).toBe(510000);
});

it('should allow user to disable margin subtraction via ai_timeout = visor timeout', () => {
const visor = 1800000;
expect(deriveProbeTimeout(visor, visor)).toBe(visor);
});
});
});
Loading
Loading