Skip to content

Commit 5c23136

Browse files
Bill LeoutsakosBill Leoutsakos
Bill Leoutsakos
authored and
Bill Leoutsakos
committed
fix(pi): report babysit round state accurately
1 parent 770ca81 commit 5c23136

2 files changed

Lines changed: 128 additions & 13 deletions

File tree

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

Lines changed: 106 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@ import { DIFF_PATH } from '@/executor/handlers/pi/cloud-shared'
7070

7171
const OLD_SHA = 'a'.repeat(40)
7272
const NEW_SHA = 'c'.repeat(40)
73+
const SECOND_SHA = 'd'.repeat(40)
7374
const snapshot = {
7475
headSha: OLD_SHA,
7576
headRef: 'feature',
@@ -160,13 +161,15 @@ function makeRunner(options: {
160161
prepareStdout?: string | string[]
161162
pushResult?: ReturnType<typeof commandResult>
162163
roundFile?: string
164+
diff?: string | string[]
163165
}) {
164166
const runCalls: Array<{
165167
command: string
166168
envs?: Record<string, string>
167169
timeoutMs?: number
168170
}> = []
169171
let prepareCall = 0
172+
let diffRead = 0
170173
const runner = {
171174
run: vi.fn(
172175
async (
@@ -191,7 +194,7 @@ function makeRunner(options: {
191194
: options.prepareStdout
192195
return commandResult(
193196
configuredPrepare ??
194-
`__CHANGED__=src/a.ts\n__DIFF_BYTES__=20\n__NEW_SHA__=${NEW_SHA}\n__NEEDS_PUSH__=1\n`
197+
`__CUMULATIVE_CHANGED__=src/a.ts\n__CUMULATIVE_DIFF_BYTES__=20\n__CHANGED__=src/a.ts\n__NEW_SHA__=${NEW_SHA}\n__NEEDS_PUSH__=1\n`
195198
)
196199
}
197200
if (command.includes('CURRENT_DIGEST=')) {
@@ -202,7 +205,12 @@ function makeRunner(options: {
202205
),
203206
writeFile: vi.fn(),
204207
readFile: vi.fn(async (path: string) => {
205-
if (path === DIFF_PATH) return 'diff --git a/src/a.ts b/src/a.ts'
208+
if (path === DIFF_PATH) {
209+
if (Array.isArray(options.diff)) {
210+
return options.diff[diffRead++] ?? options.diff.at(-1) ?? ''
211+
}
212+
return options.diff ?? 'diff --git a/src/a.ts b/src/a.ts'
213+
}
206214
if (path === BABYSIT_ROUND_PATH) {
207215
return (
208216
options.roundFile ??
@@ -347,7 +355,7 @@ describe('runBabysitPiWithOptions', () => {
347355
}
348356
if (command.includes('git -c core.hooksPath=/dev/null add -A')) {
349357
return commandResult(
350-
`__CHANGED__=src/a.ts\n__DIFF_BYTES__=20\n__NEW_SHA__=${NEW_SHA}\n__NEEDS_PUSH__=1\n`
358+
`__CUMULATIVE_CHANGED__=src/a.ts\n__CUMULATIVE_DIFF_BYTES__=20\n__CHANGED__=src/a.ts\n__NEW_SHA__=${NEW_SHA}\n__NEEDS_PUSH__=1\n`
351359
)
352360
}
353361
if (command.includes('CURRENT_DIGEST=')) return commandResult('__PUSHED__=1\n')
@@ -402,6 +410,63 @@ describe('runBabysitPiWithOptions', () => {
402410
})
403411
})
404412

413+
it('reports only the last pushed round while enforcing cumulative markers', async () => {
414+
mockFetchSnapshot
415+
.mockResolvedValueOnce(snapshot)
416+
.mockResolvedValueOnce(snapshot)
417+
.mockResolvedValueOnce({ ...snapshot, headSha: NEW_SHA })
418+
.mockResolvedValueOnce({ ...snapshot, headSha: NEW_SHA })
419+
.mockResolvedValueOnce({ ...snapshot, headSha: NEW_SHA })
420+
.mockResolvedValueOnce({ ...snapshot, headSha: SECOND_SHA })
421+
.mockResolvedValueOnce({ ...snapshot, headSha: SECOND_SHA })
422+
mockFetchThreads.mockResolvedValue({
423+
actionable: [],
424+
skipped: [],
425+
totalUnresolved: 0,
426+
latestReview: null,
427+
})
428+
mockFetchChecks
429+
.mockResolvedValueOnce(failingChecks)
430+
.mockResolvedValueOnce(failingChecks)
431+
.mockResolvedValueOnce(greenChecks)
432+
mockReplyAndResolve.mockResolvedValue({
433+
repliesPosted: 0,
434+
threadsResolved: 0,
435+
replyFailures: [],
436+
resolveFailures: [],
437+
headMoved: false,
438+
awaitingConfirmation: false,
439+
})
440+
const { runner, runCalls } = makeRunner({
441+
prepareStdout: [
442+
`__CUMULATIVE_CHANGED__=src/a.ts\n__CUMULATIVE_DIFF_BYTES__=20\n__CHANGED__=src/a.ts\n__NEW_SHA__=${NEW_SHA}\n__NEEDS_PUSH__=1\n`,
443+
`__CUMULATIVE_CHANGED__=src/a.ts\n__CUMULATIVE_CHANGED__=src/b.ts\n__CUMULATIVE_DIFF_BYTES__=40\n__CHANGED__=src/b.ts\n__NEW_SHA__=${SECOND_SHA}\n__NEEDS_PUSH__=1\n`,
444+
],
445+
roundFile: JSON.stringify({ threads: [] }),
446+
diff: ['round-one-diff', 'round-two-diff'],
447+
})
448+
mockWithPiSandbox.mockImplementation(async (callback) => callback(runner))
449+
450+
const result = await runBabysitPiWithOptions(
451+
params(),
452+
{ onEvent: vi.fn() },
453+
{ convergenceWaitMs: 0, roundWaitMs: 0 }
454+
)
455+
456+
expect(result).toMatchObject({
457+
stopReason: 'clean',
458+
rounds: 2,
459+
commitsPushed: 2,
460+
changedFiles: ['src/b.ts'],
461+
diff: 'round-two-diff',
462+
})
463+
expect(
464+
runCalls
465+
.filter(({ command }) => command.includes('CURRENT_DIGEST='))
466+
.map(({ envs }) => envs?.PINNED_SHA)
467+
).toEqual([OLD_SHA, NEW_SHA])
468+
})
469+
405470
it('refuses .github changes before the credentialed push', async () => {
406471
mockFetchSnapshot.mockResolvedValue(snapshot)
407472
mockFetchThreads.mockResolvedValue({
@@ -412,7 +477,7 @@ describe('runBabysitPiWithOptions', () => {
412477
})
413478
mockFetchChecks.mockResolvedValue(greenChecks)
414479
const { runner, runCalls } = makeRunner({
415-
prepareStdout: `__CHANGED__=.github/workflows/ci.yml\n__DIFF_BYTES__=20\n__NEW_SHA__=${NEW_SHA}\n__NEEDS_PUSH__=1\n`,
480+
prepareStdout: `__CUMULATIVE_CHANGED__=.github/workflows/ci.yml\n__CUMULATIVE_DIFF_BYTES__=20\n__CHANGED__=.github/workflows/ci.yml\n__NEW_SHA__=${NEW_SHA}\n__NEEDS_PUSH__=1\n`,
416481
})
417482
mockWithPiSandbox.mockImplementation(async (callback) => callback(runner))
418483

@@ -672,10 +737,46 @@ describe('runBabysitPiWithOptions', () => {
672737
rounds: 2,
673738
commitsPushed: 0,
674739
threadsResolved: 0,
740+
threadsClean: false,
741+
checksGreen: true,
675742
})
676743
expect(mockFetchThreads).toHaveBeenCalledTimes(3)
677744
})
678745

746+
it('preserves clean threads when checks are stuck', async () => {
747+
mockFetchSnapshot.mockResolvedValue(snapshot)
748+
mockFetchThreads.mockResolvedValue({
749+
actionable: [],
750+
skipped: [],
751+
totalUnresolved: 0,
752+
latestReview: null,
753+
})
754+
mockFetchChecks.mockResolvedValue(failingChecks)
755+
mockReplyAndResolve.mockResolvedValue({
756+
repliesPosted: 0,
757+
threadsResolved: 0,
758+
replyFailures: [],
759+
resolveFailures: [],
760+
headMoved: false,
761+
awaitingConfirmation: false,
762+
})
763+
const { runner } = makeRunner({
764+
prepareStdout: '__NO_CHANGES__=1\n',
765+
roundFile: JSON.stringify({ threads: [] }),
766+
})
767+
mockWithPiSandbox.mockImplementation(async (callback) => callback(runner))
768+
769+
const result = await runBabysitPiWithOptions(params(), { onEvent: vi.fn() }, { roundWaitMs: 0 })
770+
771+
expect(result).toMatchObject({
772+
stopReason: 'stuck_checks',
773+
rounds: 2,
774+
commitsPushed: 0,
775+
threadsClean: true,
776+
checksGreen: false,
777+
})
778+
})
779+
679780
it('does not count a push round toward unchanged-pin stuck detection', async () => {
680781
mockFetchSnapshot
681782
.mockResolvedValueOnce(snapshot)
@@ -698,7 +799,7 @@ describe('runBabysitPiWithOptions', () => {
698799
})
699800
const { runner } = makeRunner({
700801
prepareStdout: [
701-
`__CHANGED__=src/a.ts\n__DIFF_BYTES__=20\n__NEW_SHA__=${NEW_SHA}\n__NEEDS_PUSH__=1\n`,
802+
`__CUMULATIVE_CHANGED__=src/a.ts\n__CUMULATIVE_DIFF_BYTES__=20\n__CHANGED__=src/a.ts\n__NEW_SHA__=${NEW_SHA}\n__NEEDS_PUSH__=1\n`,
702803
'__NO_CHANGES__=1\n',
703804
'__NO_CHANGES__=1\n',
704805
],

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

Lines changed: 22 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -125,9 +125,10 @@ else
125125
test "$(git rev-list --count "$ROUND_BASE_SHA"..HEAD)" = "1"
126126
test "$(git symbolic-ref --short HEAD)" = "$HEAD_REF"
127127
test "$(git rev-parse "refs/heads/$HEAD_REF")" = "$(git rev-parse HEAD)"
128-
git diff --name-only "$INITIAL_HEAD_SHA" HEAD | sed "s/^/__CHANGED__=/"
129-
git diff "$INITIAL_HEAD_SHA" HEAD > ${DIFF_PATH}
130-
wc -c < ${DIFF_PATH} | tr -d ' ' | sed "s/^/__DIFF_BYTES__=/"
128+
git diff --name-only "$INITIAL_HEAD_SHA" HEAD | sed "s/^/__CUMULATIVE_CHANGED__=/"
129+
git diff "$INITIAL_HEAD_SHA" HEAD | wc -c | tr -d ' ' | sed "s/^/__CUMULATIVE_DIFF_BYTES__=/"
130+
git diff --name-only "$ROUND_BASE_SHA" HEAD | sed "s/^/__CHANGED__=/"
131+
git diff "$ROUND_BASE_SHA" HEAD > ${DIFF_PATH}
131132
git rev-parse HEAD | sed "s/^/__NEW_SHA__=/"
132133
test -z "$(git status --porcelain)"
133134
echo "__NEEDS_PUSH__=1"
@@ -427,16 +428,19 @@ async function finalizeRound(
427428
return { commitPushed: false, newSha: roundBaseSha, changedFiles: [], diff: '' }
428429
}
429430

431+
const cumulativeChangedFiles = extractMarkerValues(prepare.stdout, '__CUMULATIVE_CHANGED__=')
432+
const cumulativeDiffBytes = Number(
433+
extractMarkerValues(prepare.stdout, '__CUMULATIVE_DIFF_BYTES__=')[0]
434+
)
430435
const changedFiles = extractMarkerValues(prepare.stdout, '__CHANGED__=')
431-
const diffBytes = Number(extractMarkerValues(prepare.stdout, '__DIFF_BYTES__=')[0])
432436
const newSha = extractMarkerValues(prepare.stdout, '__NEW_SHA__=')[0]
433-
if (!newSha || !Number.isSafeInteger(diffBytes)) {
437+
if (!newSha || !Number.isSafeInteger(cumulativeDiffBytes)) {
434438
throw new Error('Babysit finalize omitted its commit or diff bounds')
435439
}
436-
if (changedFiles.length > MAX_CHANGED_FILES || diffBytes > MAX_DIFF_BYTES) {
440+
if (cumulativeChangedFiles.length > MAX_CHANGED_FILES || cumulativeDiffBytes > MAX_DIFF_BYTES) {
437441
throw new Error('Babysit cumulative change bounds were exceeded')
438442
}
439-
if (changedFiles.some((file) => file === '.github' || file.startsWith('.github/'))) {
443+
if (cumulativeChangedFiles.some((file) => file === '.github' || file.startsWith('.github/'))) {
440444
throw new Error('Babysit refuses to push changes under .github/')
441445
}
442446

@@ -1026,7 +1030,17 @@ export async function runBabysitPiWithOptions(
10261030
: observedCheckSignature && lastAttempt.repeats >= 1
10271031
? 'stuck_checks'
10281032
: undefined
1029-
if (reason) return resultFor(totals, reason, progress, false, false)
1033+
if (reason) {
1034+
const observedThreadsClean =
1035+
latestThreads.actionable.length === 0 && latestThreads.skipped.length === 0
1036+
return resultFor(
1037+
totals,
1038+
reason,
1039+
progress,
1040+
observedThreadsClean,
1041+
latestChecks.checksGreen
1042+
)
1043+
}
10301044
lastAttempt = { ...observedAttempt, repeats: lastAttempt.repeats + 1 }
10311045
} else {
10321046
lastAttempt = { ...observedAttempt, repeats: 1 }

0 commit comments

Comments
 (0)