Skip to content

Commit 40cb5ec

Browse files
committed
fix(pi): correct the push-hardening claim, reserve finalize time, tighten isRequired
Three review findings, each verified rather than taken on faith. PUSH_SCRIPT's comment claimed GIT_CONFIG_NOSYSTEM and GIT_CONFIG_GLOBAL close config-driven URL rewriting. They do not: reproduced locally on git 2.43, a repository-local url.*.insteadOf still rewrites the push URL and sends the token's userinfo to another host. That is the scope a root agent in the checkout can actually write, and it stays open until a mode verifies the config digest — which is Babysit, per the plan. The comment now says that instead of the opposite. PI_TIMEOUT_MS capped the Pi command at the whole sandbox lifetime, so the sandbox always died first and the stated benefit — a clean timeout instead of an opaque SDK error — could never happen. It now reserves the clone and finalize budgets it shares the sandbox with, leaving the host time to commit and push whatever the agent produced. isRequired is Boolean! on both CheckRun and StatusContext (confirmed by schema introspection), so the nullable parse modelled a value GitHub cannot send and left stage 2 a tri-state to handle. It is required now, and an absent value fails loudly rather than reading as "not required", which would let a failing required check stop blocking the green verdict. Also: a github-pr.test.ts pinning that the raw fetchPrSnapshot does not throw on a closed PR (the entire reason for the wrapper split, previously untested), a cloud-shared.test.ts for the timeout reserve and the digest line, the E2B lifetime ceiling documented next to E2B_PI_TEMPLATE_ID as section 7 asks, and the new registry tests no longer leaving their fake tools registered.
1 parent 509ce37 commit 40cb5ec

9 files changed

Lines changed: 201 additions & 21 deletions

File tree

apps/docs/content/docs/en/workflows/blocks/pi.mdx

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,8 @@ The one case neither layer can rescue is a *first* prompt that already exceeds t
155155
Create PR runs in a sandbox image with the Pi CLI and git baked in.
156156

157157
1. **Enable sandbox execution.** On self-hosted Sim, set `E2B_ENABLED=true`, `E2B_API_KEY`, `E2B_PI_TEMPLATE_ID` (the Pi template id), and `NEXT_PUBLIC_E2B_ENABLED=true` (this reveals Create PR and Review Code in the UI). Build the template with `bun run apps/sim/scripts/build-pi-e2b-template.ts`. Both modes stay hidden until `NEXT_PUBLIC_E2B_ENABLED` is set.
158+
159+
Sim asks E2B to keep a Pi sandbox alive for just under an hour, because E2B caps a sandbox at 1 hour on Hobby accounts (24 hours on Pro) and rejects a longer one outright. Set `PI_SANDBOX_LIFETIME_MS` to lower that; it cannot raise it. A run that outlives the sandbox loses its work before the push, and an orphaned sandbox — one whose Sim process died mid-run — bills until the lifetime expires.
158160
2. **Bring your own model key.** Set the provider API key in the block's API Key field, or store it in **Settings → BYOK** when the provider supports workspace BYOK.
159161
3. **Create a GitHub token** with permission to clone, push, and open a PR:
160162
- *Fine-grained:* select the repo, then **Contents: Read and write** + **Pull requests: Read and write**.
Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { describe, expect, it } from 'vitest'
5+
import { PI_SANDBOX_MAX_LIFETIME_MS } from '@/lib/execution/remote-sandbox/pi-lifetime'
6+
import {
7+
CLONE_TIMEOUT_MS,
8+
FINALIZE_TIMEOUT_MS,
9+
GIT_CONFIG_DIGEST_LINE,
10+
GIT_CONFIG_DIGEST_MARKER,
11+
PI_TIMEOUT_MS,
12+
} from '@/executor/handlers/pi/cloud-shared'
13+
14+
describe('PI_TIMEOUT_MS', () => {
15+
it('leaves the host room to commit and push after the agent turn ends', () => {
16+
// Capping at the bare sandbox lifetime would mean the sandbox always died
17+
// first, taking the agent's finished work with it unpushed.
18+
expect(PI_TIMEOUT_MS).toBeLessThanOrEqual(
19+
PI_SANDBOX_MAX_LIFETIME_MS - CLONE_TIMEOUT_MS - FINALIZE_TIMEOUT_MS
20+
)
21+
expect(PI_TIMEOUT_MS).toBeGreaterThan(0)
22+
})
23+
})
24+
25+
describe('GIT_CONFIG_DIGEST_LINE', () => {
26+
it('emits the marker a host parses, over the one config scope a root agent can write', () => {
27+
expect(GIT_CONFIG_DIGEST_LINE).toContain(GIT_CONFIG_DIGEST_MARKER)
28+
expect(GIT_CONFIG_DIGEST_LINE).toContain('.git/config')
29+
// A worktree config is not always present, and its absence must not fail the clone.
30+
expect(GIT_CONFIG_DIGEST_LINE).toContain('.git/config.worktree')
31+
expect(GIT_CONFIG_DIGEST_LINE).toContain('2>/dev/null')
32+
})
33+
})

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

Lines changed: 25 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -19,14 +19,24 @@ export const FINALIZE_TIMEOUT_MS = 10 * 60 * 1000
1919
export const MAX_DIFF_BYTES = 200_000
2020
export const PUSH_ERROR_MAX = 1000
2121

22+
/** Floor for {@link PI_TIMEOUT_MS}, so a very short configured lifetime still leaves a usable turn. */
23+
const MIN_PI_TIMEOUT_MS = 60 * 1000
24+
2225
/**
23-
* How long one Pi CLI invocation may run. Bounded by the sandbox lifetime: the
24-
* platform's max execution timeout is longer, so an uncapped hung CLI would sit
25-
* there until the sandbox was reaped and surface as an opaque SDK error rather
26-
* than as a timeout. The sandbox clock starts at create and the clone runs
27-
* first, so this bounds the command rather than guaranteeing it times out.
26+
* How long one Pi CLI invocation may run. The platform's max execution timeout
27+
* is longer than the sandbox lives, so without this a hung CLI would sit there
28+
* until E2B reaped the sandbox and surface as an opaque SDK error.
29+
*
30+
* The reserve matters as much as the cap: the sandbox clock starts at create,
31+
* and the clone runs before the agent while the commit and push run after it.
32+
* Capping at the bare lifetime would mean the sandbox always died first, taking
33+
* the agent's finished work with it unpushed. Reserving both surrounding command
34+
* budgets leaves the host time to finalize whatever the agent produced.
2835
*/
29-
export const PI_TIMEOUT_MS = Math.min(getMaxExecutionTimeout(), resolvePiSandboxLifetimeMs())
36+
export const PI_TIMEOUT_MS = Math.min(
37+
getMaxExecutionTimeout(),
38+
Math.max(resolvePiSandboxLifetimeMs() - CLONE_TIMEOUT_MS - FINALIZE_TIMEOUT_MS, MIN_PI_TIMEOUT_MS)
39+
)
3040

3141
/**
3242
* Marker carrying a digest of the cloned repository's git config. A clone script
@@ -67,8 +77,15 @@ if git diff --quiet "$BASE_SHA" HEAD; then echo "__NO_CHANGES__=1"; else echo "_
6777
* The only token-bearing command. It neutralizes repository-configured hooks,
6878
* credential helpers, and fsmonitor before pushing agent-authored changes, and
6979
* must be run with `GIT_CONFIG_NOSYSTEM=1` and `GIT_CONFIG_GLOBAL=/dev/null` in
70-
* its env — those cover config-driven URL rewriting, which would send the
71-
* token's userinfo to another host and which the `-c` flags do not reach.
80+
* its env, which the `-c` flags cannot substitute for.
81+
*
82+
* Be precise about what that pair buys. It closes system- and global-scope
83+
* config, so it removes two of the three places a `url.*.insteadOf` rewrite
84+
* could send the token's userinfo to another host. Repository-local config —
85+
* the scope a root agent inside the checkout can actually write — still
86+
* rewrites the push URL, and stays open until a mode compares the
87+
* {@link GIT_CONFIG_DIGEST_MARKER} digest before pushing. Babysit does; Create
88+
* PR does not, and inherits the pre-existing exposure it always had.
7289
*
7390
* Git is invoked by absolute path so a shim planted earlier on `$PATH` is not
7491
* what runs. Both sandbox images apt-install git on Debian (see
Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { beforeEach, describe, expect, it, vi } from 'vitest'
5+
6+
const { mockExecuteTool } = vi.hoisted(() => ({ mockExecuteTool: vi.fn() }))
7+
8+
vi.mock('@/tools', () => ({ executeTool: mockExecuteTool }))
9+
10+
import {
11+
fetchOpenPrSnapshot,
12+
fetchPrSnapshot,
13+
validateRepositoryCoordinates,
14+
} from '@/executor/handlers/pi/github-pr'
15+
16+
const HEAD_SHA = 'a'.repeat(40)
17+
const BASE_SHA = 'b'.repeat(40)
18+
19+
const COORDINATES = {
20+
owner: 'octo',
21+
repo: 'demo',
22+
pullNumber: 7,
23+
githubToken: 'ghp_secret',
24+
}
25+
26+
function snapshot(overrides: Record<string, unknown> = {}) {
27+
return {
28+
title: 'Add feature',
29+
body: 'Does the thing',
30+
html_url: 'https://github.com/octo/demo/pull/7',
31+
state: 'open',
32+
head: { sha: HEAD_SHA },
33+
base: { sha: BASE_SHA, ref: 'staging' },
34+
...overrides,
35+
}
36+
}
37+
38+
describe('fetchPrSnapshot', () => {
39+
beforeEach(() => {
40+
vi.clearAllMocks()
41+
mockExecuteTool.mockResolvedValue({ success: true, output: snapshot() })
42+
})
43+
44+
it('reads the pull request without its files, using the caller-supplied token', async () => {
45+
const result = await fetchPrSnapshot(COORDINATES)
46+
47+
expect(mockExecuteTool).toHaveBeenCalledWith(
48+
'github_pr_v2',
49+
{
50+
owner: 'octo',
51+
repo: 'demo',
52+
pullNumber: 7,
53+
includeFiles: false,
54+
apiKey: 'ghp_secret',
55+
},
56+
{ signal: undefined }
57+
)
58+
expect(result).toMatchObject({ headSha: HEAD_SHA, baseSha: BASE_SHA, state: 'open' })
59+
})
60+
61+
it('returns a closed or merged pull request instead of throwing', async () => {
62+
// This is the whole reason the state guard lives in the wrapper: a mode that
63+
// must report "the PR closed mid-run" as a result rather than as a failure
64+
// builds on this form, so folding the guard back in here would break it.
65+
mockExecuteTool.mockResolvedValue({ success: true, output: snapshot({ state: 'closed' }) })
66+
67+
await expect(fetchPrSnapshot(COORDINATES)).resolves.toMatchObject({ state: 'closed' })
68+
})
69+
70+
it('surfaces a failed read rather than returning an empty snapshot', async () => {
71+
mockExecuteTool.mockResolvedValue({ success: false, error: 'Not Found' })
72+
73+
await expect(fetchPrSnapshot(COORDINATES)).rejects.toThrow('Failed to fetch PR #7: Not Found')
74+
})
75+
76+
it('rejects a head SHA that is not a full commit id', async () => {
77+
mockExecuteTool.mockResolvedValue({ success: true, output: snapshot({ head: { sha: 'abc' } }) })
78+
79+
await expect(fetchPrSnapshot(COORDINATES)).rejects.toThrow(
80+
/head\.sha must be a full commit SHA/
81+
)
82+
})
83+
})
84+
85+
describe('fetchOpenPrSnapshot', () => {
86+
beforeEach(() => vi.clearAllMocks())
87+
88+
it('passes an open pull request through', async () => {
89+
mockExecuteTool.mockResolvedValue({ success: true, output: snapshot() })
90+
91+
await expect(fetchOpenPrSnapshot(COORDINATES)).resolves.toMatchObject({ state: 'open' })
92+
})
93+
94+
it('refuses anything that is not open', async () => {
95+
mockExecuteTool.mockResolvedValue({ success: true, output: snapshot({ state: 'closed' }) })
96+
97+
await expect(fetchOpenPrSnapshot(COORDINATES)).rejects.toThrow(
98+
'PR #7 is closed; only open PRs can be reviewed'
99+
)
100+
})
101+
})
102+
103+
describe('validateRepositoryCoordinates', () => {
104+
it('accepts ordinary GitHub coordinates', () => {
105+
expect(() => validateRepositoryCoordinates(COORDINATES)).not.toThrow()
106+
})
107+
108+
it.each([
109+
['a traversal in the owner', { owner: '../octo' }],
110+
['a traversal in the repo', { repo: '..' }],
111+
['a slash in the repo', { repo: 'demo/evil' }],
112+
['a non-positive pull number', { pullNumber: 0 }],
113+
['a fractional pull number', { pullNumber: 1.5 }],
114+
])('rejects %s before any credential is used', (_label, overrides) => {
115+
expect(() => validateRepositoryCoordinates({ ...COORDINATES, ...overrides })).toThrow(
116+
/Invalid GitHub repository coordinates/
117+
)
118+
})
119+
})

apps/sim/tools/github/graphql.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,8 @@
1+
/**
2+
* Shared plumbing for the GitHub GraphQL tools: the endpoint, its headers, and
3+
* the two response shapes every query has to handle the same way.
4+
*/
5+
16
import { isRecord, readGitHubErrorMessage } from '@/tools/github/response-parsers'
27

38
export const GITHUB_GRAPHQL_URL = 'https://api.github.com/graphql'

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

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -123,7 +123,6 @@ describe('github_status_check_rollup', () => {
123123
conclusion: null,
124124
detailsUrl: null,
125125
databaseId: null,
126-
isRequired: null,
127126
}),
128127
statusContext({ description: null, targetUrl: null }),
129128
])
@@ -133,11 +132,19 @@ describe('github_status_check_rollup', () => {
133132
conclusion: null,
134133
detailsUrl: null,
135134
databaseId: null,
136-
isRequired: null,
137135
})
138136
expect(result.output.contexts[1]).toMatchObject({ description: null, targetUrl: null })
139137
})
140138

139+
it('fails on a missing requiredness signal rather than reading it as optional', async () => {
140+
// GraphQL declares isRequired non-null on both variants, so an absent value
141+
// means something changed — and defaulting it would quietly let a failing
142+
// required check stop blocking the green verdict.
143+
await expect(parse(rollupPayload([actionsCheckRun({ isRequired: null })]))).rejects.toThrow(
144+
/isRequired must be a boolean/
145+
)
146+
})
147+
141148
it('keeps a third-party app output that is actually populated', async () => {
142149
const result = await parse(
143150
rollupPayload([

apps/sim/tools/github/status_check_rollup.ts

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,9 @@ import {
77
} from '@/tools/github/graphql'
88
import {
99
isRecord,
10-
nullableBoolean,
1110
nullableNumber,
1211
nullableString,
12+
requiredBoolean,
1313
requiredNumber,
1414
requiredString,
1515
} from '@/tools/github/response-parsers'
@@ -85,7 +85,7 @@ function parseRollupContext(value: unknown, index: number): StatusCheckRollupCon
8585
conclusion: nullableString(value, 'conclusion', context),
8686
detailsUrl: nullableString(value, 'detailsUrl', context),
8787
databaseId: nullableNumber(value, 'databaseId', context),
88-
isRequired: nullableBoolean(value, 'isRequired', context),
88+
isRequired: requiredBoolean(value, 'isRequired', context),
8989
title: nullableString(value, 'title', context),
9090
summary: nullableString(value, 'summary', context),
9191
}
@@ -97,7 +97,7 @@ function parseRollupContext(value: unknown, index: number): StatusCheckRollupCon
9797
state: requiredString(value, 'state', context),
9898
description: nullableString(value, 'description', context),
9999
targetUrl: nullableString(value, 'targetUrl', context),
100-
isRequired: nullableBoolean(value, 'isRequired', context),
100+
isRequired: requiredBoolean(value, 'isRequired', context),
101101
}
102102
}
103103
// Stopping beats guessing: a caller buckets unknown states as blocking, and it
@@ -131,7 +131,6 @@ const ROLLUP_CONTEXT_PROPERTIES = {
131131
isRequired: {
132132
type: 'boolean',
133133
description: 'Whether the check is required to merge this pull request',
134-
nullable: true,
135134
},
136135
title: {
137136
type: 'string',

apps/sim/tools/github/types.ts

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2156,11 +2156,8 @@ export type StatusCheckRollupContext =
21562156
detailsUrl: string | null
21572157
/** REST id of the check run; the Actions job id for an Actions check run. */
21582158
databaseId: number | null
2159-
/**
2160-
* Whether the check gates the merge. GraphQL declares this non-null, so a
2161-
* null only means GitHub stopped reporting it — treat it as blocking.
2162-
*/
2163-
isRequired: boolean | null
2159+
/** Whether the check gates the merge for the pull request that was asked about. */
2160+
isRequired: boolean
21642161
/**
21652162
* The check run's reported output. GraphQL exposes these as flat fields on
21662163
* `CheckRun`, unlike REST's nested `output` object, and GitHub Actions
@@ -2177,8 +2174,7 @@ export type StatusCheckRollupContext =
21772174
state: string
21782175
description: string | null
21792176
targetUrl: string | null
2180-
/** See the `CheckRun` variant: null means unknown, not "not required". */
2181-
isRequired: boolean | null
2177+
isRequired: boolean
21822178
}
21832179

21842180
export interface StatusCheckRollupParams extends BaseGitHubParams {

apps/sim/tools/index.test.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1019,6 +1019,7 @@ describe('Automatic Internal Route Detection', () => {
10191019
expect.objectContaining({ stripAuthOnRedirect: true })
10201020
)
10211021

1022+
Reflect.deleteProperty(tools, 'test_redirecting_download')
10221023
Object.assign(tools, originalTools)
10231024
})
10241025

@@ -1048,6 +1049,7 @@ describe('Automatic Internal Route Detection', () => {
10481049
expect.objectContaining({ stripAuthOnRedirect: undefined })
10491050
)
10501051

1052+
Reflect.deleteProperty(tools, 'test_plain_external')
10511053
Object.assign(tools, originalTools)
10521054
})
10531055

0 commit comments

Comments
 (0)