Skip to content

Commit 64d2c05

Browse files
outof-placeclaude
andcommitted
fix(worker): stop silently skipping pre-PR checks when dependencies are missing
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015kfeohXE66xx7RJPxZ2pvH
1 parent 20f42df commit 64d2c05

3 files changed

Lines changed: 145 additions & 3 deletions

File tree

apps/worker/src/pre-pr-checks/runner.test.ts

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,40 @@ describe("runPrePrChecksWithFixes", () => {
177177
expect(result.failures).toHaveLength(1);
178178
});
179179

180+
it("fails a check that exits 0 while reporting its dependencies are not installed", async () => {
181+
mockRunCommand.mockImplementation((cmd, args) => {
182+
if (cmd === "cat" && args[0] === WORKSPACE_MANIFEST_PATH) {
183+
return commandResult(0, JSON.stringify(manifest));
184+
}
185+
if (cmd === "git" && args[0] === "-C" && args[2] === "rev-parse") {
186+
return commandResult(0, "web-head");
187+
}
188+
// The check tool self-skips on missing deps and exits 0.
189+
return commandResult(
190+
0,
191+
"Yarn checks were blocked because dependencies are not installed",
192+
);
193+
});
194+
195+
const result = await runPrePrChecksWithFixes(
196+
"sbx-test-123",
197+
{ repositories: [config.repositories[0]!] },
198+
"codex",
199+
"gpt-5",
200+
0,
201+
);
202+
203+
expect(result.outcome).toBe("failed");
204+
expect(result.passed).toBe(false);
205+
expect(result.failures).toHaveLength(1);
206+
expect(result.failures[0]).toMatchObject({
207+
provider: "github",
208+
repoPath: "acme/web",
209+
command: "pnpm typecheck",
210+
});
211+
expect(result.summary.toLowerCase()).toContain("did not actually run");
212+
});
213+
180214
it("treats inability to inspect a repository as an execution failure", async () => {
181215
mockRunCommand.mockImplementation((cmd, args) => {
182216
if (cmd === "cat" && args[0] === WORKSPACE_MANIFEST_PATH) {

apps/worker/src/pre-pr-checks/runner.ts

Lines changed: 37 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -179,14 +179,26 @@ async function runConfiguredPrePrChecks(
179179
command,
180180
exitCode: result.exitCode,
181181
});
182-
if (result.exitCode !== 0) {
182+
const stdout = await commandStdout(result);
183+
const stderr = await commandStderr(result);
184+
// A configured check that exits 0 while reporting that it never ran (its
185+
// dependencies are not installed) must fail loudly instead of being trusted
186+
// as a pass. The Run Workspace is never dependency-installed, so a check tool
187+
// that self-skips on missing deps with a success exit code once let a blocked
188+
// check clear the pre-PR gate: the branch was pushed and the PR's own CI then
189+
// caught the lint failure the gate exists to prevent.
190+
const blockedByMissingDependencies =
191+
result.exitCode === 0 && checkBlockedByMissingDependencies(stdout, stderr);
192+
if (result.exitCode !== 0 || blockedByMissingDependencies) {
183193
failures.push({
184194
provider: repo.provider,
185195
repoPath: repo.repoPath,
186196
command,
187197
exitCode: result.exitCode,
188-
stdout: await commandStdout(result),
189-
stderr: await commandStderr(result),
198+
stdout,
199+
stderr: blockedByMissingDependencies
200+
? [stderr, MISSING_DEPENDENCY_FAILURE_REASON].filter(Boolean).join("\n")
201+
: stderr,
190202
});
191203
}
192204
}
@@ -469,6 +481,28 @@ function repositoryKey(repo: Pick<WorkspaceRepo, "provider" | "repoPath">): stri
469481
return `${repo.provider}:${repo.repoPath}`;
470482
}
471483

484+
const MISSING_DEPENDENCY_FAILURE_REASON =
485+
"Pre-PR check exited 0 but its dependencies are not installed, so the check did not actually run.";
486+
487+
/**
488+
* True when a check tool reported it could not run because the project's
489+
* dependencies are not installed (yarn/npm/pnpm all print a variant of this,
490+
* and some exit 0 while doing so). Matched against the tool's own output so an
491+
* exit-0 "success" that never executed the check is surfaced as a failure
492+
* instead of silently certifying the workspace.
493+
*/
494+
function checkBlockedByMissingDependencies(stdout: string, stderr: string): boolean {
495+
const haystack = `${stderr}\n${stdout}`.toLowerCase();
496+
return (
497+
haystack.includes("dependencies are not installed") ||
498+
haystack.includes("dependencies must be installed") ||
499+
haystack.includes("run `yarn install`") ||
500+
haystack.includes("run 'yarn install'") ||
501+
haystack.includes("run `npm install`") ||
502+
haystack.includes("run `pnpm install`")
503+
);
504+
}
505+
472506
async function commandStdout(result: SandboxCommandResult): Promise<string> {
473507
return (await result.stdout()).trim();
474508
}
Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
# Design note: watch the external PR/MR CI pipeline to green
2+
3+
Status: proposal, not implemented. Recommendation: file as its own ticket.
4+
5+
## Problem
6+
7+
Today the workflow bot runs the dashboard-configured pre-PR checks inside the Run
8+
Workspace, then pushes the branch and opens the PR/MR. Once the PR is open, the
9+
provider (GitHub Actions / GitLab CI) runs its own pipeline. The bot never looks
10+
at that pipeline. When the provider pipeline fails (for reasons the in-sandbox
11+
pre-PR checks did not or could not catch), nobody feeds that failure back into a
12+
fix loop. A client hit exactly this: an MR opened, GitLab's `lint` /
13+
`Lint-Docker-Frontend` jobs failed, and the run had already reported success.
14+
15+
The related fix in this PR stops one silent-skip (a pre-PR check that exits 0
16+
while its dependencies are not installed now fails loudly). Watching the external
17+
pipeline is the broader, separate capability the client also asked for and is
18+
intentionally out of scope here.
19+
20+
## Proposed behavior
21+
22+
After `open_pr` publishes a PR/MR, a new optional block (working name
23+
`watch_ci_pipeline`) polls the provider's pipeline for the pushed head SHA until
24+
it reaches a terminal state, then branches on the result:
25+
26+
1. Resolve the pipeline for the PR's head SHA via the VCS adapter
27+
(`apps/worker/src/adapters/vcs/*`): GitHub check runs / commit statuses for a
28+
ref; GitLab pipelines + jobs for a ref.
29+
2. Poll on an interval with a bounded deadline, driven by the run budget's
30+
remaining duration (reuse `ctx.observeBudget()` so a stuck pipeline cannot
31+
outlive the run). Poll cadence and max wait are block params.
32+
3. Ignore non-gating checks. Meticulous is explicitly excluded (it posts its own
33+
visual-review status that must not gate the bot). Maintain an ignore-list of
34+
check/job names (and/or contexts), defaulting to a Meticulous matcher, so the
35+
watcher only waits on and reacts to gating jobs.
36+
4. Terminal outcomes:
37+
- all gating jobs succeeded -> `ok: true` (branch to done / Slack / ticket move);
38+
- one or more gating jobs failed -> collect each failed job's name + log tail
39+
and feed them into the existing fix loop (same shape the pre-PR runner uses
40+
for `runFixAgent`: a prompt with the failing job output, then re-push and
41+
re-watch), capped by a max-attempts param and the run budget;
42+
- pipeline never reaches terminal state before the deadline -> surface a
43+
bounded, transparent failure (budget/deadline), never a silent pass.
44+
45+
## Where it plugs in
46+
47+
- New block type registered in `apps/worker/src/workflow-definition/block-registry.ts`
48+
and a matching case in the interpreter/agent block switch (`agent.ts`).
49+
- New VCS adapter methods: `getPipelineForRef(headSha)` / `listCheckRunsForRef`
50+
returning a normalized `{ name, status, conclusion, logsUrl }[]` across GitHub
51+
and GitLab, plus a way to fetch a failed job's log tail.
52+
- Reuse the fix-agent machinery already in `pre-pr-checks/runner.ts`
53+
(`runFixAgent`, `buildFixPrompt`) so external-CI failures and pre-PR failures
54+
share one repair path and one budget accounting.
55+
- Ignore-list config (Meticulous by default) alongside the pre-PR checks config
56+
so it is dashboard-managed and versioned like the rest of the gate config.
57+
58+
## Risks / open questions (for the ticket)
59+
60+
- Polling long pipelines against the per-invocation limit: this must be
61+
budget-bounded and heartbeat-safe; a pipeline that runs for an hour cannot pin
62+
the run. Confirm the WDK invocation model tolerates the poll loop.
63+
- Re-push after a fix re-triggers the provider pipeline, which re-triggers the
64+
watcher: needs a clear max-attempts and loop-boundary so it cannot ping-pong.
65+
- Which statuses gate (required checks only vs all): should read the provider's
66+
branch-protection / required-checks set where available rather than guessing.
67+
- Meticulous-style checks that are `pending` forever must be treated as ignored,
68+
not as "still running", or the watcher waits on them until the deadline.
69+
70+
## Recommendation
71+
72+
File as its own ticket (its own block type, adapter surface, and budget/loop
73+
design). Do not fold it into the missing-dependency pre-PR fix, which is a
74+
narrow, self-contained correctness fix.

0 commit comments

Comments
 (0)