Skip to content

Commit fa8a56a

Browse files
committed
fix(pi): budget Babysit against the run's real deadline
Three fixes that each needed to land a level below where the symptom showed. Execution deadline. Babysit planned its wait loop against `getMaxExecutionTimeout()`, which unconditionally returns the enterprise async ceiling (90 min) with no regard for the run's plan or sync/async mode — the sync ceiling is 300s on free, 3000s on pro. A synchronously triggered run was therefore killed mid-loop with the PR already opened, its review comments already posted, and none of the rounds/stopReason outputs produced. Rather than thread a numeric deadline through ExecutionContext — five entry points that would each have to remember it, and nothing to catch the one that forgot — `createTimeoutAbortController` now records the deadline against the signal it creates, and `getRemainingExecutionMs(signal)` reads it back. The number cannot disagree with the timer that enforces it, because both are established in the one place the timeout exists, and every entry point already hands the executor that signal. `undefined` means unknown, not unlimited: Babysit keeps the old ceiling as its fallback for an untimed run. Job logs. The executor caps a response at 10 MB and throws rather than truncating, so a verbose CI job produced no diagnostic at all — and GitHub Actions reports null title/summary on every check run, leaving the agent a bare URL it has no tool to follow. The tool now sends `Range: bytes=-N` so the storage host returns only the tail. Since a suffix range is a request and not a guarantee, a 200 still takes the local slice. That made the old output dishonest, so the contract changed while the tool is still new and has one consumer: `totalCharacters` (which a ranged read cannot know) is replaced by `totalBytes`, sourced from the `Content-Range` total and null when unreported. A ranged body is trimmed at its first line break, since the byte window cuts mid-line and can split a multi-byte character. Generated docs. `github.mdx` was missing the `repo_full_name` the PR reader now returns, and regenerating deleted the head/base rows instead of adding it: `scripts/generate-docs.ts` only expands a spread at depth 0 of a const, and `PR_BRANCH_REF_OUTPUT` spread inline under `properties`. Restructured into a named properties const in types.ts, next to the shapes it belongs with, which the generator resolves the same way it already resolves BRANCH_REF_OUTPUT. Only the github.mdx hunk is committed. The generator is lossy elsewhere — it drops 126 lines of trigger configuration from jira.mdx — which is pre-existing drift for whoever owns that surface, not this branch.
1 parent 8c7d9d2 commit fa8a56a

9 files changed

Lines changed: 237 additions & 41 deletions

File tree

apps/docs/content/docs/en/integrations/github.mdx

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,10 +59,12 @@ Fetch PR details including diff and files changed
5959
|`label` | string | Branch label \(owner:branch\) |
6060
|`ref` | string | Branch name |
6161
|`sha` | string | Commit SHA |
62+
|`repo_full_name` | string | Full name \(owner/repo\) of the branch's repository |
6263
| `base` | object | Branch reference info |
6364
|`label` | string | Branch label \(owner:branch\) |
6465
|`ref` | string | Branch name |
6566
|`sha` | string | Commit SHA |
67+
|`repo_full_name` | string | Full name \(owner/repo\) of the branch's repository |
6668
| `id` | number | Pull request ID |
6769
| `number` | number | Pull request number |
6870
| `title` | string | PR title |

apps/sim/executor/handlers/pi/cloud-backend.test.ts

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ vi.mock('@/executor/handlers/pi/keys', () => ({
3737
}))
3838
vi.mock('@/executor/handlers/pi/context', () => ({ buildPiPrompt: () => 'PROMPT' }))
3939

40+
import { createTimeoutAbortController } from '@/lib/core/execution-limits'
4041
import type { PiCloudRunParams } from '@/executor/handlers/pi/backend'
4142
import { runCloudPi } from '@/executor/handlers/pi/cloud-backend'
4243

@@ -288,6 +289,25 @@ describe('runCloudPi', () => {
288289
})
289290
})
290291

292+
// Babysit spends this budget sitting in waits, so it has to reflect the deadline the
293+
// platform will actually enforce. Planning against the enterprise-async ceiling meant a
294+
// sync run was killed mid-loop with the PR opened and its review comments posted.
295+
it('budgets Babysit against the run signal deadline, not the platform maximum', async () => {
296+
const controller = createTimeoutAbortController(4 * 60 * 1000)
297+
298+
await runCloudPi(
299+
baseParams({
300+
babysit: { maxRounds: 4, reviewMentions: ['@greptile'] },
301+
}),
302+
{ onEvent: vi.fn(), signal: controller.signal }
303+
)
304+
305+
const budget = mockRunBabysit.mock.calls[0][0].executionBudgetMs
306+
expect(budget).toBeGreaterThan(0)
307+
expect(budget).toBeLessThanOrEqual(4 * 60 * 1000)
308+
controller.cleanup()
309+
})
310+
291311
it('skips the PR when nothing was pushed', async () => {
292312
mockRun.mockImplementation((command: string) => {
293313
if (command.includes('git clone')) {

apps/sim/executor/handlers/pi/cloud-backend.ts

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@
2121
import { createLogger } from '@sim/logger'
2222
import { generateShortId } from '@sim/utils/id'
2323
import { truncate } from '@sim/utils/string'
24-
import { getMaxExecutionTimeout } from '@/lib/core/execution-limits'
24+
import { getMaxExecutionTimeout, getRemainingExecutionMs } from '@/lib/core/execution-limits'
2525
import { withPiSandbox } from '@/lib/execution/remote-sandbox'
2626
import { resolvePiSandboxLifetimeMs } from '@/lib/execution/remote-sandbox/pi-lifetime'
2727
import { runBabysitPi } from '@/executor/handlers/pi/babysit-backend'
@@ -439,7 +439,15 @@ export const runCloudPi: PiBackendRun<PiCloudRunParams> = async (params, context
439439
return combineCreateAndBabysit(created, skippedBabysitResult('startup_failure'))
440440
}
441441

442-
const remainingExecutionMs = Math.max(0, getMaxExecutionTimeout() - (Date.now() - startedAt))
442+
// The run's own deadline when the platform set one, because Babysit spends this
443+
// budget sitting in waits: planning against `getMaxExecutionTimeout()` — the
444+
// longest-lived plan's async ceiling — meant a sync run was killed mid-loop with
445+
// the PR already opened, its review comments already posted, and none of the
446+
// `rounds`/`stopReason` outputs produced. The ceiling stays as the fallback for an
447+
// untimed execution, where it is the only bound available.
448+
const remainingExecutionMs =
449+
getRemainingExecutionMs(context.signal) ??
450+
Math.max(0, getMaxExecutionTimeout() - (Date.now() - startedAt))
443451
const sandboxBudgetMs = resolvePiSandboxLifetimeMs() ?? getMaxExecutionTimeout()
444452
const babysit = await runBabysitPi(
445453
{

apps/sim/lib/core/execution-limits/types.test.ts

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ declare module '@/lib/core/execution-limits/types?execution-limits-test' {
3535
import {
3636
createTimeoutAbortController,
3737
getExecutionTimeout,
38+
getRemainingExecutionMs,
3839
isTimeoutAbortReason,
3940
} from '@/lib/core/execution-limits/types?execution-limits-test'
4041

@@ -113,3 +114,42 @@ describe('getExecutionTimeout', () => {
113114
controller.cleanup()
114115
})
115116
})
117+
118+
describe('getRemainingExecutionMs', () => {
119+
it('reports the time left on the signal that enforces the timeout', () => {
120+
const controller = createTimeoutAbortController(60_000)
121+
122+
const remaining = getRemainingExecutionMs(controller.signal)
123+
124+
expect(remaining).toBeGreaterThan(55_000)
125+
expect(remaining).toBeLessThanOrEqual(60_000)
126+
controller.cleanup()
127+
})
128+
129+
it('never reports a negative remainder once the deadline has passed', () => {
130+
vi.useFakeTimers()
131+
try {
132+
const controller = createTimeoutAbortController(1_000)
133+
vi.advanceTimersByTime(5_000)
134+
135+
expect(getRemainingExecutionMs(controller.signal)).toBe(0)
136+
controller.cleanup()
137+
} finally {
138+
vi.useRealTimers()
139+
}
140+
})
141+
142+
/**
143+
* `undefined` is "unknown", not "unlimited" — an untimed run, a signal from
144+
* somewhere else, or a derived signal all land here, and a caller that needs a
145+
* bound has to keep its own fallback rather than assume the run is untimed.
146+
*/
147+
it('returns undefined for an untimed, foreign, or absent signal', () => {
148+
const untimed = createTimeoutAbortController()
149+
150+
expect(getRemainingExecutionMs(untimed.signal)).toBeUndefined()
151+
expect(getRemainingExecutionMs(new AbortController().signal)).toBeUndefined()
152+
expect(getRemainingExecutionMs(undefined)).toBeUndefined()
153+
untimed.cleanup()
154+
})
155+
})

apps/sim/lib/core/execution-limits/types.ts

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -157,12 +157,45 @@ export function isTimeoutAbortReason(reason: unknown): boolean {
157157
)
158158
}
159159

160+
/**
161+
* Absolute deadline of each timeout signal, keyed by the signal that enforces it.
162+
*
163+
* Recorded here rather than threaded through {@link ExecutionContext} so the
164+
* number can never disagree with the timer that acts on it: both are established
165+
* in the one place the timeout exists. Every entry point already hands the
166+
* executor `timeoutController.signal`, so a block that holds the signal can ask
167+
* how long it really has without any call site remembering to pass a second
168+
* field. Weakly keyed, so an finished execution's entry is collectable.
169+
*/
170+
const signalDeadlines = new WeakMap<AbortSignal, number>()
171+
172+
/**
173+
* Wall-clock milliseconds left before `signal`'s own timeout fires.
174+
*
175+
* `undefined` means the deadline is unknown — an untimed execution, or a derived
176+
* signal rather than the one {@link createTimeoutAbortController} produced. Treat
177+
* that as "no information", not as "no limit": callers that need a bound should
178+
* keep their own conservative fallback rather than assume the run is untimed.
179+
*
180+
* Use this instead of {@link getMaxExecutionTimeout} when the question is "how
181+
* much time does *this* run have left" — that function answers the different
182+
* question of how long the longest-lived plan may ever run, and is a large
183+
* overestimate for a sync run or a lower-tier plan.
184+
*/
185+
export function getRemainingExecutionMs(signal?: AbortSignal): number | undefined {
186+
if (!signal) return undefined
187+
const deadline = signalDeadlines.get(signal)
188+
if (deadline === undefined) return undefined
189+
return Math.max(0, deadline - Date.now())
190+
}
191+
160192
export function createTimeoutAbortController(timeoutMs?: number): TimeoutAbortController {
161193
const abortController = new AbortController()
162194
let isTimedOut = false
163195
let timeoutId: NodeJS.Timeout | undefined
164196

165197
if (timeoutMs) {
198+
signalDeadlines.set(abortController.signal, Date.now() + timeoutMs)
166199
timeoutId = setTimeout(() => {
167200
isTimedOut = true
168201
// AbortError with a typed message — see isTimeoutAbortReason.

apps/sim/tools/github/job_logs.test.ts

Lines changed: 49 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,17 @@ function logResponse(body: string): Response {
1616
return new Response(body, { headers: { 'Content-Type': 'text/plain' } })
1717
}
1818

19+
/** A storage host that honoured the suffix range: 206 plus the full size. */
20+
function partialLogResponse(body: string, totalBytes: number): Response {
21+
return new Response(body, {
22+
status: 206,
23+
headers: {
24+
'Content-Type': 'text/plain',
25+
'Content-Range': `bytes ${totalBytes - Buffer.byteLength(body)}-${totalBytes - 1}/${totalBytes}`,
26+
},
27+
})
28+
}
29+
1930
describe('github_job_logs', () => {
2031
it('reads the per-job log endpoint, not the run-level archive', () => {
2132
const url = (jobLogsTool.request.url as (params: JobLogsParams) => string)(BASE_PARAMS)
@@ -50,7 +61,7 @@ describe('github_job_logs', () => {
5061

5162
expect(result).toEqual({
5263
success: true,
53-
output: { logs: 'boom\n', totalCharacters: 5, truncated: false },
64+
output: { logs: 'boom\n', truncated: false, totalBytes: 5 },
5465
})
5566
})
5667

@@ -64,7 +75,43 @@ describe('github_job_logs', () => {
6475

6576
expect(result.output.logs).toHaveLength(40)
6677
expect(result.output.logs.endsWith('FAILED: expected 1 to be 2')).toBe(true)
67-
expect(result.output).toMatchObject({ totalCharacters: log.length, truncated: true })
78+
expect(result.output).toMatchObject({ totalBytes: log.length, truncated: true })
79+
})
80+
81+
// A verbose CI job exceeds the executor's 10 MB response cap, which throws rather
82+
// than truncating — so asking for only the tail is what keeps a diagnostic available
83+
// on exactly the runs that most need one.
84+
it('asks the storage host for only the tail it intends to keep', () => {
85+
const headers = jobLogsTool.request.headers({ ...BASE_PARAMS, maxCharacters: 4_096 })
86+
87+
expect(headers.Range).toBe('bytes=-4096')
88+
})
89+
90+
it('trims the partial first line of a ranged response and reports the full size', async () => {
91+
const result = await jobLogsTool.transformResponse!(
92+
partialLogResponse('ise\nFAILED: expected 1 to be 2', 10_000),
93+
{ ...BASE_PARAMS, maxCharacters: 4_096 }
94+
)
95+
96+
expect(result.output).toEqual({
97+
logs: 'FAILED: expected 1 to be 2',
98+
truncated: true,
99+
totalBytes: 10_000,
100+
})
101+
})
102+
103+
// A host that ignores the range answers 200 with the whole body, so the local
104+
// slice has to remain the fallback rather than an assumption about partiality.
105+
it('falls back to the local slice when the range is ignored', async () => {
106+
const log = `${'noise\n'.repeat(100)}tail`
107+
108+
const result = await jobLogsTool.transformResponse!(logResponse(log), {
109+
...BASE_PARAMS,
110+
maxCharacters: 10,
111+
})
112+
113+
expect(result.output.logs).toBe(log.slice(-10))
114+
expect(result.output).toMatchObject({ truncated: true, totalBytes: log.length })
68115
})
69116

70117
it('rejects a cap outside the supported range', async () => {

apps/sim/tools/github/job_logs.ts

Lines changed: 49 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -28,23 +28,44 @@ function jobLogsPath(owner: string, repo: string, jobId: number): string {
2828
return `${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/actions/jobs/${jobId}/logs`
2929
}
3030

31+
/**
32+
* Total size from a `Content-Range: bytes <start>-<end>/<total>` header.
33+
*
34+
* `null` for an unsatisfied-range form, an unknown total, an unparsable value, or
35+
* an absent header — all of which mean the full size is simply unknown here.
36+
*/
37+
function parseContentRangeTotal(header: string | null): number | null {
38+
const total = header?.match(/^bytes\s+\d+-\d+\/(\d+)$/)?.[1]
39+
if (!total) return null
40+
const parsed = Number(total)
41+
return Number.isSafeInteger(parsed) && parsed >= 0 ? parsed : null
42+
}
43+
3144
/**
3245
* The tail is what matters: a failing job reports its error at the end.
3346
*
34-
* Reading the whole body first is safe because the tool executor already caps a
35-
* response at 10 MB and hands `transformResponse` a buffer, so a log larger than
36-
* that fails with the executor's size-limit error before reaching here — which
37-
* is the honest outcome, since a truncated head would read as a passing job.
47+
* A ranged response already *is* the tail, so it is only trimmed at the first
48+
* line break — the byte window almost always cuts mid-line, and it can also split
49+
* a multi-byte character into a replacement char. A full response is sliced
50+
* locally instead, which is the path taken whenever the storage host ignores the
51+
* range and answers 200.
3852
*/
3953
function logTail(
4054
text: string,
41-
maxCharacters: number
42-
): { logs: string; totalCharacters: number; truncated: boolean } {
43-
return {
44-
logs: text.slice(-maxCharacters),
45-
totalCharacters: text.length,
46-
truncated: text.length > maxCharacters,
55+
maxCharacters: number,
56+
partial: boolean,
57+
totalBytes: number | null
58+
): { logs: string; truncated: boolean; totalBytes: number | null } {
59+
if (!partial) {
60+
return {
61+
logs: text.slice(-maxCharacters),
62+
truncated: text.length > maxCharacters,
63+
totalBytes: totalBytes ?? Buffer.byteLength(text),
64+
}
4765
}
66+
const firstBreak = text.indexOf('\n')
67+
const trimmed = firstBreak === -1 ? text : text.slice(firstBreak + 1)
68+
return { logs: trimmed.slice(-maxCharacters), truncated: true, totalBytes }
4869
}
4970

5071
export const jobLogsTool: ToolConfig<JobLogsParams, JobLogsResponse> = {
@@ -98,6 +119,13 @@ export const jobLogsTool: ToolConfig<JobLogsParams, JobLogsResponse> = {
98119
Accept: 'application/vnd.github+json',
99120
Authorization: `Bearer ${params.apiKey}`,
100121
'X-GitHub-Api-Version': '2022-11-28',
122+
// Ask the storage host for only the tail we intend to keep. A CI job with a
123+
// verbose build routinely exceeds the executor's 10 MB response cap, and that
124+
// cap throws rather than truncating — so without this a large log yielded no
125+
// diagnostic at all, on exactly the runs that most need one. A suffix range is
126+
// a request, not a guarantee: a host that ignores it answers 200 with the full
127+
// body and the local slice below still applies.
128+
Range: `bytes=-${resolveMaxCharacters(params.maxCharacters)}`,
101129
}),
102130
// The redirect target is third-party blob storage. Sim's tool fetch follows
103131
// redirects itself rather than through the fetch spec, so without this the
@@ -107,15 +135,24 @@ export const jobLogsTool: ToolConfig<JobLogsParams, JobLogsResponse> = {
107135

108136
transformResponse: async (response, params) => {
109137
const maxCharacters = resolveMaxCharacters(params?.maxCharacters)
110-
return { success: true, output: logTail(await response.text(), maxCharacters) }
138+
const partial = response.status === 206
139+
const totalBytes = parseContentRangeTotal(response.headers.get('content-range'))
140+
return {
141+
success: true,
142+
output: logTail(await response.text(), maxCharacters, partial, totalBytes),
143+
}
111144
},
112145

113146
outputs: {
114147
logs: { type: 'string', description: 'Trailing portion of the job log' },
115-
totalCharacters: { type: 'number', description: 'Full length of the log before truncation' },
116148
truncated: {
117149
type: 'boolean',
118150
description: 'Whether earlier output was dropped to fit maxCharacters',
119151
},
152+
totalBytes: {
153+
type: 'number',
154+
description: 'Full size of the log in bytes, null when the server did not report it',
155+
nullable: true,
156+
},
120157
},
121158
}

apps/sim/tools/github/pr.ts

Lines changed: 2 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -18,30 +18,8 @@ import type {
1818
PullRequestResponse,
1919
PullRequestV2Response,
2020
} from '@/tools/github/types'
21-
import {
22-
BRANCH_REF_OUTPUT_PROPERTIES,
23-
PR_FILE_OUTPUT_PROPERTIES,
24-
USER_OUTPUT,
25-
} from '@/tools/github/types'
26-
import type { OutputProperty, ToolConfig } from '@/tools/types'
27-
28-
/**
29-
* The PR reader's own branch-reference output. `repo_full_name` is declared here
30-
* rather than on the shared `BRANCH_REF_OUTPUT_PROPERTIES` because `list_prs`
31-
* reuses those with a pass-through transform that never derives the field.
32-
*/
33-
const PR_BRANCH_REF_OUTPUT = {
34-
type: 'object',
35-
description: 'Branch reference info',
36-
properties: {
37-
...BRANCH_REF_OUTPUT_PROPERTIES,
38-
repo_full_name: {
39-
type: 'string',
40-
description: "Full name (owner/repo) of the branch's repository",
41-
nullable: true,
42-
},
43-
},
44-
} as const satisfies OutputProperty
21+
import { PR_BRANCH_REF_OUTPUT, PR_FILE_OUTPUT_PROPERTIES, USER_OUTPUT } from '@/tools/github/types'
22+
import type { ToolConfig } from '@/tools/types'
4523

4624
type GitHubPullRequest = Omit<GitHubPullRequestV2Output, 'files'>
4725

0 commit comments

Comments
 (0)