Skip to content

Commit 234693e

Browse files
committed
fix(pi): correct switch coercion for draft and tidy Babysit reporting
- `draft` had the same string-coercion bug that `babysitMode` was fixed for one line above it. A switch arrives as `'true'`/`'false'` when its value came through a variable reference, an API trigger payload, or a legacy serialized workflow, and `inputs.draft !== false` read `'false'` as truthy — opening a draft PR against the user's explicit setting. Both now go through one `isSwitchEnabled` helper that handles either polarity and takes the field's default, because the bug is opposite on each. - `mergePhaseDiffs` joined two separately-capped diffs without re-capping, so the combined output could reach twice MAX_DIFF_BYTES. - The cancellation poller's `logger.warn` was the one message in these files emitted unscrubbed. A Redis poll error is unlikely to carry a run credential, but a uniform invariant is easier to keep than a per-call-site argument. - Renamed `waitWithSandboxKeepalive` to `waitWithSandboxProbe`. E2B's `timeoutMs` counts down from create and is reset only by `Sandbox.setTimeout`, never by running a command, so `true` every four minutes proves liveness and buys no time. The old name invited raising the round wait on the assumption that waits extend the sandbox, which would let E2B reap it mid-wait. - Dropped a `{@link}` to a symbol in another module that was never imported.
1 parent 130c76b commit 234693e

5 files changed

Lines changed: 93 additions & 17 deletions

File tree

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

Lines changed: 27 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,7 @@ const MIN_BABYSIT_BUDGET_MS = MIN_PI_TIMEOUT_MS
110110
*/
111111
const MIN_ROUND_BUDGET_MS = 5 * 60 * 1000
112112
const ROUND_FINALIZATION_RESERVE_MS = 2 * FINALIZE_TIMEOUT_MS
113-
const SANDBOX_KEEPALIVE_INTERVAL_MS = 4 * 60 * 1000
113+
const SANDBOX_PROBE_INTERVAL_MS = 4 * 60 * 1000
114114

115115
const BABYSIT_GUIDANCE =
116116
'You are fixing an existing pull request in a long-lived automated sandbox. Make only minimal ' +
@@ -431,7 +431,8 @@ function buildRoundPrompt(
431431
function createCancellationSignal(
432432
parent: AbortSignal | undefined,
433433
executionId: string | undefined,
434-
pollMs: number
434+
pollMs: number,
435+
secrets: readonly string[]
435436
): { signal: AbortSignal; cleanup: () => void } {
436437
const controller = new AbortController()
437438
const onAbort = () => controller.abort(parent?.reason ?? 'workflow_abort')
@@ -451,9 +452,12 @@ function createCancellationSignal(
451452
}
452453
})
453454
.catch((error) => {
455+
// Scrubbed like every other message this file emits. A Redis poll
456+
// error is unlikely to carry a run credential, but the invariant is
457+
// easier to keep than to reason about per call site.
454458
logger.warn('Failed to poll Babysit execution cancellation', {
455459
executionId,
456-
error: getErrorMessage(error),
460+
error: scrubPiSecrets(getErrorMessage(error), secrets),
457461
})
458462
})
459463
.finally(() => {
@@ -655,21 +659,30 @@ async function waitForHeadConvergence(
655659
return 'lagging'
656660
}
657661

658-
async function waitWithSandboxKeepalive(
662+
/**
663+
* Sleeps in slices, checking the sandbox is still answering between them.
664+
*
665+
* Named a probe rather than a keepalive because it cannot extend anything: E2B's
666+
* `timeoutMs` counts down from create and is reset only by `Sandbox.setTimeout`,
667+
* never by running a command, so `true` proves liveness and buys no time. The wait
668+
* is safe today only because {@link runBabysitPiWithOptions} starts its clock
669+
* before the sandbox is created, so the budget always expires first — a reordering,
670+
* or a `roundWaitMs` raised past the remaining lifetime, would let E2B reap the
671+
* sandbox mid-wait. Extending the lifetime would need `setTimeout` plumbed through
672+
* {@link PiSandboxRunner}.
673+
*/
674+
async function waitWithSandboxProbe(
659675
runner: PiSandboxRunner,
660676
durationMs: number,
661677
signal: AbortSignal
662678
): Promise<void> {
663679
let remainingMs = durationMs
664680
while (remainingMs > 0) {
665-
const intervalMs = Math.min(remainingMs, SANDBOX_KEEPALIVE_INTERVAL_MS)
681+
const intervalMs = Math.min(remainingMs, SANDBOX_PROBE_INTERVAL_MS)
666682
await sleepUntilAborted(intervalMs, signal)
667683
if (signal.aborted) throw new Error('Pi run aborted')
668-
const keepalive = await raceAbort(
669-
runner.run('true', { timeoutMs: FINALIZE_TIMEOUT_MS }),
670-
signal
671-
)
672-
if (keepalive.exitCode !== 0) throw new Error('Babysit sandbox keepalive failed')
684+
const probe = await raceAbort(runner.run('true', { timeoutMs: FINALIZE_TIMEOUT_MS }), signal)
685+
if (probe.exitCode !== 0) throw new Error('Babysit sandbox stopped responding')
673686
remainingMs -= intervalMs
674687
}
675688
}
@@ -721,7 +734,8 @@ export async function runBabysitPiWithOptions(
721734
const cancellation = createCancellationSignal(
722735
context.signal,
723736
params.executionId,
724-
options.cancellationPollMs
737+
options.cancellationPollMs,
738+
secrets
725739
)
726740
const { signal } = cancellation
727741
const startedAt = Date.now()
@@ -878,7 +892,7 @@ export async function runBabysitPiWithOptions(
878892
)
879893
return resultFor(totals, reason, progress, threadsClean, latestChecks!.checksGreen)
880894
}
881-
await waitWithSandboxKeepalive(runner, options.roundWaitMs, signal)
895+
await waitWithSandboxProbe(runner, options.roundWaitMs, signal)
882896
snapshot = await fetchBabysitSnapshot(params, signal)
883897
assertBabysitPinned(
884898
{ headSha: pinnedHeadSha, headRef: pinnedHeadRef, baseRef: pinnedBaseRef },
@@ -1221,7 +1235,7 @@ export async function runBabysitPiWithOptions(
12211235
remainingBeforeWait >
12221236
options.roundWaitMs + MIN_ROUND_BUDGET_MS + ROUND_FINALIZATION_RESERVE_MS
12231237
) {
1224-
await waitWithSandboxKeepalive(runner, options.roundWaitMs, signal)
1238+
await waitWithSandboxProbe(runner, options.roundWaitMs, signal)
12251239
}
12261240

12271241
snapshot = await fetchBabysitSnapshot(params, signal)

apps/sim/executor/handlers/pi/babysit-github.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -545,7 +545,7 @@ async function fetchCheckDiagnostic(
545545
*
546546
* Fanned out in small batches rather than one at a time: each Actions log is a separate
547547
* HTTP read of a body that can approach the executor's response cap, and a round may
548-
* carry up to {@link MAX_FAILING_CHECKS_IN_PROMPT} of them. Serialized, that put minutes
548+
* carry up to the caller's per-round check bound of them. Serialized, that put minutes
549549
* of avoidable wall clock inside a budget the round loop is carefully rationing. The
550550
* batch size stays small so a wide matrix cannot burst GitHub's rate limiter.
551551
*/

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

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -170,8 +170,15 @@ function mergeChangedFiles(
170170
return [...new Set([...(createFiles ?? []), ...(babysitFiles ?? [])])]
171171
}
172172

173+
/**
174+
* Joins both phases' diffs under the same ceiling each already respects
175+
* individually — without the re-cap, two 200 KB diffs produced a 400 KB output.
176+
*/
173177
function mergePhaseDiffs(createDiff: string | undefined, babysitDiff: string | undefined): string {
174-
return [createDiff, babysitDiff].filter((diff): diff is string => !!diff).join('\n')
178+
const merged = [createDiff, babysitDiff].filter((diff): diff is string => !!diff).join('\n')
179+
return merged.length > MAX_DIFF_BYTES
180+
? `${merged.slice(0, MAX_DIFF_BYTES)}\n[diff truncated]`
181+
: merged
175182
}
176183

177184
function combineCreateAndBabysit(created: CreatePrPhaseResult, babysit: PiRunResult): PiRunResult {

apps/sim/executor/handlers/pi/pi-handler.test.ts

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -353,6 +353,43 @@ describe('PiBlockHandler', () => {
353353
expect(mockRunCloud.mock.calls[0][0]).not.toHaveProperty('babysit')
354354
})
355355

356+
// A `switch` arrives as a string when its value came through a variable reference,
357+
// an API trigger payload, or a legacy serialized workflow.
358+
it('enables Babysit when the toggle arrives as the string "true"', async () => {
359+
await handler.execute(ctx(), block, {
360+
mode: 'cloud',
361+
task: 'build it',
362+
model: 'claude',
363+
owner: 'o',
364+
repo: 'r',
365+
githubToken: 'ghp',
366+
babysitMode: 'true',
367+
reviewMentions: '@greptile',
368+
})
369+
370+
expect(mockRunCloud.mock.calls[0][0].babysit).toMatchObject({
371+
reviewMentions: ['@greptile'],
372+
})
373+
})
374+
375+
// The negative polarity is the one `draft` needs: it defaults on, so a strict
376+
// `!== false` read the string 'false' as truthy and opened a draft PR against
377+
// the user's explicit setting.
378+
it('honours a draft toggle supplied as the string "false"', async () => {
379+
await handler.execute(ctx(), block, {
380+
mode: 'cloud',
381+
task: 'build it',
382+
model: 'claude',
383+
owner: 'o',
384+
repo: 'r',
385+
githubToken: 'ghp',
386+
babysitMode: false,
387+
draft: 'false',
388+
})
389+
390+
expect(mockRunCloud.mock.calls[0][0].draft).toBe(false)
391+
})
392+
356393
it('parses review mentions as a bounded, trimmed list', () => {
357394
expect(parsePiReviewMentions(' @one, , @two ')).toEqual(['@one', '@two'])
358395
expect(parsePiReviewMentions('')).toEqual([])

apps/sim/executor/handlers/pi/pi-handler.ts

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,21 @@ function isReviewEvent(value: string): value is PiCloudReviewRunParams['reviewEv
7373
return REVIEW_EVENTS.some((event) => event === value)
7474
}
7575

76+
/**
77+
* Reads a `switch` subblock, tolerating the string form.
78+
*
79+
* A switch reaches a handler as `'true'`/`'false'` when its value arrived through
80+
* a variable reference, an API trigger payload, or a legacy serialized workflow —
81+
* `wait-handler` coerces the same way. Both polarities need it: a strict `=== true`
82+
* silently disables an enabled toggle, and a strict `!== false` silently enables a
83+
* disabled one.
84+
*/
85+
function isSwitchEnabled(value: unknown, defaultValue = false): boolean {
86+
if (value === true || value === 'true') return true
87+
if (value === false || value === 'false') return false
88+
return defaultValue
89+
}
90+
7691
function parsePiMode(value: unknown): PiRunParams['mode'] {
7792
if (value === 'cloud' || value === 'cloud_review' || value === 'local') {
7893
return value
@@ -246,7 +261,7 @@ export class PiBlockHandler implements BlockHandler {
246261
// workflow (see the same coercion in `wait-handler`). A strict boolean compare
247262
// silently opened a draft PR and skipped Babysit entirely while the editor showed
248263
// the toggle on and Reviewer Mentions as required.
249-
const babysitMode = inputs.babysitMode === true || inputs.babysitMode === 'true'
264+
const babysitMode = isSwitchEnabled(inputs.babysitMode)
250265
const reviewMentions = babysitMode ? parsePiReviewMentions(inputs.reviewMentions) : []
251266
if (babysitMode && reviewMentions.length === 0) {
252267
throw new Error('Create PR Babysit Mode requires at least one reviewer mention')
@@ -266,7 +281,10 @@ export class PiBlockHandler implements BlockHandler {
266281
githubToken,
267282
baseBranch: asOptString(inputs.baseBranch),
268283
branchName: asOptString(inputs.branchName),
269-
draft: babysitMode ? false : inputs.draft !== false,
284+
// `draft` defaults on, so the negative form is the one that must tolerate the
285+
// string: `'false'` from a variable reference would otherwise read as truthy
286+
// and open a draft PR against the user's explicit setting.
287+
draft: babysitMode ? false : isSwitchEnabled(inputs.draft, true),
270288
prTitle: asOptString(inputs.prTitle),
271289
prBody: asOptString(inputs.prBody),
272290
...(babysitMode

0 commit comments

Comments
 (0)