feat(github-checks): rewire the worker onto the backend-owned plan loop (A3b-2) - #3667
Conversation
…op (A3b-2) The worker stops ordering candidates, deciding continue/stop and computing the final outcome. It now begins a plan before any sandbox work, reports every phase as an attempt, obeys the backend's typed action, binds the eval run at launch, and completes with no `outcome` field. It keeps — deliberately, in the open — deterministic mcpjam.yaml parsing and detection, clone/build/start/probe, E2B lifecycle, egress lockdown, cleanup, and the /proc listener-identity check. The evidence-digest encoder moves here: the backend retired its own and treats the digest as opaque, so this repo is the single encoder. The golden vector comes across as a regression test and still reproduces. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
dosubot
Bot
added
the
size:XXL
GitHub Actions
Inactive
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📄 Knowledge reviewDosu skipped reviewing this PR because your organization has used its
|
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_690b513c-d9d5-4b8e-b346-cddc6c763a40) |
Internal previewPreview URL: https://mcp-inspector-pr-3667.up.railway.app |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe change adds backend-owned GitHub check plans, typed attempt actions, retry handling, and completion requests without client-derived outcomes. Resolver execution follows planned candidates, submits versioned evidence digests, records phase-specific attempts, and returns candidate identifiers. The worker opens plans before sandbox work, binds evaluation runs at launch, reports planless infrastructure failures separately, and delegates verdict derivation to completion. Tests cover transport behavior, digest encoding, candidate execution, lifecycle handling, cleanup, lease loss, heartbeat behavior, and degraded modes. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
mcpjam-inspector/server/services/github-checks/__tests__/service-route.test.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: ESLint configuration in --config is invalid:
mcpjam-inspector/server/services/github-checks/evidence-digest.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: ESLint configuration in --config is invalid:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
mcpjam-inspector/server/services/github-checks/resolve-and-start.ts (1)
394-425: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoff
stopPolicy.wallClockMsis received and never enforced.
issued.stopPolicyreaches Line 405 as log context only.maxCandidatesneeds no local enforcement — the backend bounds the search by issuing fewer candidates and by returningcompleteonce the cap is spent, as Line 521 records.wallClockMshas no such second enforcement point.The consequence is narrow but real: while the backend keeps answering
try_next_candidate, each candidate spends a full build-and-probe cycle, and nothing in this loop consults the deadline the backend just sent. The lease heartbeat keeps succeeding throughout, so it is not a backstop.Either check the elapsed time against
wallClockMsat the top of each iteration and stop with a named reason, or record in the docblock that the field is display-only and the backend owns the deadline.♻️ Sketch of a per-iteration deadline check
+ const deadlineAt = + issued.stopPolicy.wallClockMs > 0 + ? deps.now() + issued.stopPolicy.wallClockMs + : null; + for (let index = 0; index < issued.candidates.length; index += 1) { const planned = issued.candidates[index]; const recipe = recipeOf(planned); + if (deadlineAt !== null && deps.now() >= deadlineAt) { + throw new CheckStoppedByPlan("plan_wall_clock_exhausted"); + } if (index > 0) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpjam-inspector/server/services/github-checks/resolve-and-start.ts` around lines 394 - 425, Enforce issued.stopPolicy.wallClockMs in the candidate execution loop around recipeOf and freshSandbox by tracking the plan start time and checking elapsed time at the top of each iteration. When the deadline is reached, stop with a named CheckStoppedByPlan reason, while preserving the existing maxCandidates handling and normal candidate execution behavior.mcpjam-inspector/server/services/github-checks/check-plan.ts (1)
530-584: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the shared retry-and-classify loop.
beginCheckPlanrepeats the exact control flow ofHttpCheckPlanSession.callat Lines 474-523: one transport replay,200 && oksuccess, 404 as unreachable, other 4xx as protocol, anything else as unreachable. The two copies agree today. They are also two places a future timeout or status-policy edit must land — the same hazardservice-route.tswas extracted to close, reappearing one layer up.A free function taking
post,step,path,bodyand an optionalatwould serve both:beginCheckPlancalls it before a session exists, andcalldelegates to it afterwards.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpjam-inspector/server/services/github-checks/check-plan.ts` around lines 530 - 584, Extract the shared retry-and-response-classification logic from beginCheckPlan and HttpCheckPlanSession.call into a reusable free function accepting post, step, path, body, and optional at parameters. Preserve one transport retry sequence, 200/ok success handling, 404 PlanUnreachableError, other 4xx PlanProtocolError, and all other responses as PlanUnreachableError, then have beginCheckPlan and call delegate to this helper.mcpjam-inspector/server/services/github-checks/evidence-digest.ts (1)
324-343: 🗄️ Data Integrity & Integration | 🔵 TrivialClarify
repoFiles’ place in the cache-key rules.
computeResolverEvidenceDigesthashes the fixed file bytes plus the resolver version, and the detector explicitly documentsrepoFiles.kindas not joining the R3 cache key because it comes from the same commit. Add one short exception in the digest rules thatDetectionInputs.repoFilesis intentionally out of scope, soTHE DIGEST MUST COVER EVERY BYTE DETECTION IS CAPABLE OF READINGdoes not invite future inclusion ofrepoFiles.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpjam-inspector/server/services/github-checks/evidence-digest.ts` around lines 324 - 343, The digest rules documentation should explicitly state that DetectionInputs.repoFiles is intentionally excluded from the R3 cache key because it comes from the same commit, while the digest continues hashing the fixed file contents and resolverVersion. Update the documentation nearest computeResolverEvidenceDigest or the digest rules, without changing the hashing implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mcpjam-inspector/server/services/github-checks-worker.ts`:
- Around line 849-858: Handle failures from onRunStarted in the run-start flow
by invoking the same best-effort cleanup used by the unowned-run branch: mark
setup-pending iterations failed and finalize the recorder before propagating the
original error. Ensure this cleanup covers errors such as conflicts or
PlanUnreachableError and prevents the prepared run from remaining running with
pending iterations.
---
Nitpick comments:
In `@mcpjam-inspector/server/services/github-checks/check-plan.ts`:
- Around line 530-584: Extract the shared retry-and-response-classification
logic from beginCheckPlan and HttpCheckPlanSession.call into a reusable free
function accepting post, step, path, body, and optional at parameters. Preserve
one transport retry sequence, 200/ok success handling, 404 PlanUnreachableError,
other 4xx PlanProtocolError, and all other responses as PlanUnreachableError,
then have beginCheckPlan and call delegate to this helper.
In `@mcpjam-inspector/server/services/github-checks/evidence-digest.ts`:
- Around line 324-343: The digest rules documentation should explicitly state
that DetectionInputs.repoFiles is intentionally excluded from the R3 cache key
because it comes from the same commit, while the digest continues hashing the
fixed file contents and resolverVersion. Update the documentation nearest
computeResolverEvidenceDigest or the digest rules, without changing the hashing
implementation.
In `@mcpjam-inspector/server/services/github-checks/resolve-and-start.ts`:
- Around line 394-425: Enforce issued.stopPolicy.wallClockMs in the candidate
execution loop around recipeOf and freshSandbox by tracking the plan start time
and checking elapsed time at the top of each iteration. When the deadline is
reached, stop with a named CheckStoppedByPlan reason, while preserving the
existing maxCandidates handling and normal candidate execution behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ca66e4c8-2152-4abf-8e77-7b3b6c0565df
📒 Files selected for processing (10)
mcpjam-inspector/server/services/__tests__/github-checks-worker.test.tsmcpjam-inspector/server/services/github-checks-worker.tsmcpjam-inspector/server/services/github-checks/__tests__/check-plan.test.tsmcpjam-inspector/server/services/github-checks/__tests__/evidence-digest.test.tsmcpjam-inspector/server/services/github-checks/__tests__/resolve-and-start.test.tsmcpjam-inspector/server/services/github-checks/check-plan.tsmcpjam-inspector/server/services/github-checks/evidence-digest.tsmcpjam-inspector/server/services/github-checks/resolve-and-start.tsmcpjam-inspector/server/services/github-checks/resolver/index.tsmcpjam-inspector/server/services/github-checks/service-route.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
7 issues found across 10 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="mcpjam-inspector/server/services/github-checks/check-plan.ts">
<violation number="1" location="mcpjam-inspector/server/services/github-checks/check-plan.ts:436">
P2: Degraded completions send a differently shaped idempotency key (`plan:candidate:phase:degraded:seq`) than the `/attempt` contract. If the backend validates the documented four-part key, it rejects the inline marker and this method intentionally completes without `backend_unreachable`, losing the neutral degraded attribution; retain the normal sequence-only shape.</violation>
<violation number="2" location="mcpjam-inspector/server/services/github-checks/check-plan.ts:491">
P2: A 200 response with an unparseable or empty body is classified as `backend_unreachable` instead of failing closed. `service-route.ts` intentionally tolerates a malformed body by returning `{ status: 200, body: null }`, but `call()` only accepts a 200 when `body?.ok` is truthy, so that tolerated 200 falls through to `PlanUnreachableError`. That makes the degraded marker say "backend unreachable" (attributed `infra_error`) for a backend that actually responded 200, which is a different branch than the `unknown_action` fail-closed path the module's Rule 3 describes. Surface 200 responses (even with a missing/`ok:false` body) so `attempt()` can fail closed to `complete` + `unknownAction` here, and reserve `PlanUnreachableError` for genuinely unreachable/5xx/404 cases.</violation>
</file>
<file name="mcpjam-inspector/server/services/github-checks/service-route.ts">
<violation number="1" location="mcpjam-inspector/server/services/github-checks/service-route.ts:35">
P3: Timeout and malformed-response handling has no direct coverage despite being the new transport boundary. Add fetch-mocked tests for a normal response, malformed JSON, fetch abort, and abort while `response.json()` is pending.</violation>
<violation number="2" location="mcpjam-inspector/server/services/github-checks/service-route.ts:35">
P3: Service-route behavior can drift between workers because this repeats Scheduled Evals' backend transport. Extract a configurable shared helper so authentication, timeout, and body-read semantics have one implementation.</violation>
</file>
<file name="mcpjam-inspector/server/services/github-checks-worker.ts">
<violation number="1" location="mcpjam-inspector/server/services/github-checks-worker.ts:1260">
P2: Failures between candidate acceptance and `runEvalSuite` completion are completed without an eval-phase failure attempt. Record `eval: ok:false, failureKind:'sandbox_error'` for unbound setup failures such as bearer minting or ephemeral-server creation before completing.</violation>
<violation number="2" location="mcpjam-inspector/server/services/github-checks-worker.ts:1289">
P1: An unknown or missing backend action completes without the required degraded marker. Preserve the unknown-action signal through `CheckStoppedByPlan` and include a `backend_unreachable` terminal attempt so a malformed response cannot be attributed from the preceding candidate attempt.</violation>
</file>
<file name="mcpjam-inspector/server/services/github-checks/evidence-digest.ts">
<violation number="1" location="mcpjam-inspector/server/services/github-checks/evidence-digest.ts:330">
P1: Candidate selection can be replayed for a checkout whose ownership listing changed, because `repoFiles` affects resolver output but is absent from `evidenceDigest`. Include a deterministic, bounded encoding of the listing (path and kind), and bump the evidence-hash version so a new tracked file or symlink cannot reuse a plan issued for another checkout.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
| ): string { | ||
| return computeRecipeEvidenceHash({ | ||
| resolverVersion, | ||
| files: { |
There was a problem hiding this comment.
P1: Candidate selection can be replayed for a checkout whose ownership listing changed, because repoFiles affects resolver output but is absent from evidenceDigest. Include a deterministic, bounded encoding of the listing (path and kind), and bump the evidence-hash version so a new tracked file or symlink cannot reuse a plan issued for another checkout.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcpjam-inspector/server/services/github-checks/evidence-digest.ts, line 330:
<comment>Candidate selection can be replayed for a checkout whose ownership listing changed, because `repoFiles` affects resolver output but is absent from `evidenceDigest`. Include a deterministic, bounded encoding of the listing (path and kind), and bump the evidence-hash version so a new tracked file or symlink cannot reuse a plan issued for another checkout.</comment>
<file context>
@@ -0,0 +1,343 @@
+): string {
+ return computeRecipeEvidenceHash({
+ resolverVersion,
+ files: {
+ "mcpjam.yaml":
+ inputs.mcpjamYaml.kind === "present" ? inputs.mcpjamYaml.text : null,
</file context>
| // things travelling here are display text and — when the backend went away | ||
| // mid-flight — the degraded marker. | ||
| const details = described.detailsMarkdown ?? evalFailureDetails; | ||
| const marker = degradedMarker(error); |
There was a problem hiding this comment.
P1: An unknown or missing backend action completes without the required degraded marker. Preserve the unknown-action signal through CheckStoppedByPlan and include a backend_unreachable terminal attempt so a malformed response cannot be attributed from the preceding candidate attempt.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcpjam-inspector/server/services/github-checks-worker.ts, line 1289:
<comment>An unknown or missing backend action completes without the required degraded marker. Preserve the unknown-action signal through `CheckStoppedByPlan` and include a `backend_unreachable` terminal attempt so a malformed response cannot be attributed from the preceding candidate attempt.</comment>
<file context>
@@ -1179,57 +1221,81 @@ export async function executeClaimedCheck(
+ // things travelling here are display text and — when the backend went away
+ // mid-flight — the degraded marker.
+ const details = described.detailsMarkdown ?? evalFailureDetails;
+ const marker = degradedMarker(error);
+ await safeComplete({
+ ...(details
</file context>
| : {}), | ||
| idempotencyKey: `${this.planId}:${attempt.candidateId ?? "ladder"}:${ | ||
| attempt.phase | ||
| }:degraded:${this.attemptSeq}`, |
There was a problem hiding this comment.
P2: Degraded completions send a differently shaped idempotency key (plan:candidate:phase:degraded:seq) than the /attempt contract. If the backend validates the documented four-part key, it rejects the inline marker and this method intentionally completes without backend_unreachable, losing the neutral degraded attribution; retain the normal sequence-only shape.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcpjam-inspector/server/services/github-checks/check-plan.ts, line 436:
<comment>Degraded completions send a differently shaped idempotency key (`plan:candidate:phase:degraded:seq`) than the `/attempt` contract. If the backend validates the documented four-part key, it rejects the inline marker and this method intentionally completes without `backend_unreachable`, losing the neutral degraded attribution; retain the normal sequence-only shape.</comment>
<file context>
@@ -0,0 +1,584 @@
+ : {}),
+ idempotencyKey: `${this.planId}:${attempt.candidateId ?? "ladder"}:${
+ attempt.phase
+ }:degraded:${this.attemptSeq}`,
+ },
+ };
</file context>
| await safeReport({ | ||
| triggerId: claimed.triggerId, | ||
| outcome, | ||
| await safeComplete({ |
There was a problem hiding this comment.
P2: Failures between candidate acceptance and runEvalSuite completion are completed without an eval-phase failure attempt. Record eval: ok:false, failureKind:'sandbox_error' for unbound setup failures such as bearer minting or ephemeral-server creation before completing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcpjam-inspector/server/services/github-checks-worker.ts, line 1260:
<comment>Failures between candidate acceptance and `runEvalSuite` completion are completed without an eval-phase failure attempt. Record `eval: ok:false, failureKind:'sandbox_error'` for unbound setup failures such as bearer minting or ephemeral-server creation before completing.</comment>
<file context>
@@ -1179,57 +1221,81 @@ export async function executeClaimedCheck(
- await safeReport({
- triggerId: claimed.triggerId,
- outcome,
+ await safeComplete({
+ // VERIFIED, never adopted: this must equal the run the `eval` attempt
+ // bound, and the backend 409s if it does not.
</file context>
| lastTransportError = error; | ||
| continue; | ||
| } | ||
| if (response.status === 200 && response.body?.ok) return response.body; |
There was a problem hiding this comment.
P2: A 200 response with an unparseable or empty body is classified as backend_unreachable instead of failing closed. service-route.ts intentionally tolerates a malformed body by returning { status: 200, body: null }, but call() only accepts a 200 when body?.ok is truthy, so that tolerated 200 falls through to PlanUnreachableError. That makes the degraded marker say "backend unreachable" (attributed infra_error) for a backend that actually responded 200, which is a different branch than the unknown_action fail-closed path the module's Rule 3 describes. Surface 200 responses (even with a missing/ok:false body) so attempt() can fail closed to complete + unknownAction here, and reserve PlanUnreachableError for genuinely unreachable/5xx/404 cases.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcpjam-inspector/server/services/github-checks/check-plan.ts, line 491:
<comment>A 200 response with an unparseable or empty body is classified as `backend_unreachable` instead of failing closed. `service-route.ts` intentionally tolerates a malformed body by returning `{ status: 200, body: null }`, but `call()` only accepts a 200 when `body?.ok` is truthy, so that tolerated 200 falls through to `PlanUnreachableError`. That makes the degraded marker say "backend unreachable" (attributed `infra_error`) for a backend that actually responded 200, which is a different branch than the `unknown_action` fail-closed path the module's Rule 3 describes. Surface 200 responses (even with a missing/`ok:false` body) so `attempt()` can fail closed to `complete` + `unknownAction` here, and reserve `PlanUnreachableError` for genuinely unreachable/5xx/404 cases.</comment>
<file context>
@@ -0,0 +1,584 @@
+ lastTransportError = error;
+ continue;
+ }
+ if (response.status === 200 && response.body?.ok) return response.body;
+ if (response.status >= 400 && response.status < 500) {
+ if (response.status === 404) {
</file context>
| return { convexUrl, serviceToken }; | ||
| } | ||
|
|
||
| export async function postServiceRoute( |
There was a problem hiding this comment.
P3: Timeout and malformed-response handling has no direct coverage despite being the new transport boundary. Add fetch-mocked tests for a normal response, malformed JSON, fetch abort, and abort while response.json() is pending.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcpjam-inspector/server/services/github-checks/service-route.ts, line 35:
<comment>Timeout and malformed-response handling has no direct coverage despite being the new transport boundary. Add fetch-mocked tests for a normal response, malformed JSON, fetch abort, and abort while `response.json()` is pending.</comment>
<file context>
@@ -0,0 +1,84 @@
+ return { convexUrl, serviceToken };
+}
+
+export async function postServiceRoute(
+ path: string,
+ body: Record<string, unknown>
</file context>
| return { convexUrl, serviceToken }; | ||
| } | ||
|
|
||
| export async function postServiceRoute( |
There was a problem hiding this comment.
P3: Service-route behavior can drift between workers because this repeats Scheduled Evals' backend transport. Extract a configurable shared helper so authentication, timeout, and body-read semantics have one implementation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcpjam-inspector/server/services/github-checks/service-route.ts, line 35:
<comment>Service-route behavior can drift between workers because this repeats Scheduled Evals' backend transport. Extract a configurable shared helper so authentication, timeout, and body-read semantics have one implementation.</comment>
<file context>
@@ -0,0 +1,84 @@
+ return { convexUrl, serviceToken };
+}
+
+export async function postServiceRoute(
+ path: string,
+ body: Record<string, unknown>
</file context>
Review triage on #3667. `prepareEvalRun` creates the run row AND one pending iteration row per attempt before the `eval` attempt is posted, so a throw from the binding call (a 409 from the state machine, an unreachable backend) bypassed the catch around `execute()` where the cleanup lives — leaving the run `running` with every iteration `pending`, permanently. Aborting is still right: a run nothing may read is a run not worth paying for. It just has to reach a terminal state on the way out. The cleanup the unowned-run branch already performed is now one helper, `abandonPreparedRun`, used by both aborts so they cannot drift. Also from the same review: - `stopPolicy.wallClockMs` was received and never enforced. `maxCandidates` needs no local enforcement (the backend issues fewer candidates and returns `complete` once the cap is spent), but nothing stopped a full build-and-probe cycle per candidate against the deadline the backend itself sent — the lease heartbeat is not a backstop. Checked at each candidate boundary now; `0` still means "no deadline issued". - `beginCheckPlan` and `HttpCheckPlanSession.call` had two copies of the same retry-and-classify loop, which a future timeout or status-policy edit could half-apply. One `callPlanRoute`, both callers delegate. - Documented `DetectionInputs.repoFiles` as the deliberate exception to "the digest must cover every byte detection is capable of reading", so the rule does not invite churning the digest with a listing that cannot vary independently of the commit. Tests: the binding abort is pinned in `github-checks-worker-source.test.ts` (verified failing without the fix), the deadline and the `0` case in `resolve-and-start.test.ts`. 454 tests pass, exit 0.
GitHub Actions
Inactive
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_f4585b33-6296-4a6b-a81d-98c9ef5f6140) |
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Follow-up to the wall-clock enforcement in the previous commit, from cubic's review of it (P2, valid). The deadline was checked once per candidate, at the top of the iteration — before that candidate's own sandbox is provisioned and the repo cloned. Those are minutes of budget themselves, so a candidate that was inside the deadline when its iteration began could be outside it by the time there was a box to build in, and start the expensive half anyway. Checked again after `freshSandbox`, immediately before `buildAndStart`. Both checks go through one `assertWithinDeadline()` so they cannot drift. Pinned by a test that advances the clock during the SECOND provision: the box is created, and then nothing is built in it. Verified failing without the post-provision check. 455 tests pass, exit 0.
GitHub Actions
Inactive
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_c974d901-b1d1-4d25-9e9c-ede10f4d5ce9) |
…oFiles note
Second cubic review run (it posted while its check still read
`in_progress`, so the first triage pass missed it). Two changes; the
rest of that run is answered in the PR thread rather than in code,
because the backend source settles them.
`service-route.ts` is the new transport boundary and had no direct
coverage, which matters more than usual here: `check-plan.ts` classifies
on `{status, body}` alone, so anything this function gets wrong arrives
one layer up wearing the shape of a legitimate answer. Six tests pin the
parts that carry that weight — the token header and URL composition, a
malformed body being TOLERATED, a body read that hit the deadline
surfacing as a TIMEOUT rather than as `body: null` (the two are
indistinguishable to the caller otherwise), an aborted fetch propagating
instead of becoming a synthetic status, and the deadline being disarmed
on the way out. Each was checked against a mutated source first.
The `repoFiles` note added in the previous commit asserted the exclusion
was safe. That was too strong, and it came from taking a review comment
at face value. Corrected to state what is actually true: cross-repository
replay cannot happen because the backend's cache is keyed by
`by_repo_evidence` — `(repoFullName, evidenceHash)` — but WITHIN one
repository a commit can leave every hashed byte identical and still
change the listing, most sharply by replacing a tracked entry point with
a symlink into `node_modules/`. Fresh detection suppresses that; a cache
hit issued when the path was a regular file does not. Closing it needs a
`RECIPE_EVIDENCE_HASH_VERSION` bump that orphans every existing entry, so
it is documented as tracked-not-settled rather than decided here.
461 tests pass, exit 0.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_eab35827-3148-47ac-b91e-8fcc363747d8) |
Review triageFixed in
Not changing, with reasons:
|
1 participant
What this does
Rewires the GitHub-checks worker onto the backend-owned plan/verdict loop that landed in backend #843 + #847. Receiver-first: the backend already speaks this contract; this is the caller.
The new sequence
The moat boundary — what the inspector stopped owning
resolveAndStart's attribution mappingfailureKind;FAILURE_KIND_POLICYdecides what it meansKept, deliberately and in the open: deterministic
mcpjam.yamlparsing and detection, clone/build/start/probe, E2B lifecycle, egress lockdown, cleanup,/proclistener identity, structured telemetry.Accepted cost, named: a check is now backend-dependent mid-flight, so a self-hosted inspector cannot run checks standalone. That is the moat working as intended, stated rather than emergent.
The encoder move
server/services/github-checks/evidence-digest.tsis new and is the single encoder — the backend retired its own and now shape-checks the digest only (opaque hex sha-256). Ported verbatim in behaviour: fixed ordered path list, present/absent markers, per-path byte budgets, length-prefixed framing, sha-256, coveringresolverVersion. Two improvements over the mirrored version: the budgets are now imported from the detector's real constants instead of hand-copied (assertEvidenceBudgetsCoverDetector()still runs at module load), andcomputeResolverEvidenceDigest()takes the reader's own output, so what is hashed is exactly what detection saw.The golden vector came across as an inspector regression test and reproduces the pinned digest
139b76e3…cb06, so cache entries written under the old backend encoder stay addressable.Typed action + fail closed
continue_phase/try_next_candidate/run_eval/complete, obeyed verbatim. An action this build does not recognize — or a 200 with no action at all — is normalized tocompletewithunknownAction, never "carry on".run_evalis the only action allowed to follow an accepted candidate; anything else stops.Degraded mode and 409s
backend_unreachableon the first call that gets through (aterminalAttempton/complete). Never a green, never a PR-blamed outcome, and never a silent local fallback —localCandidatesare never executed in our own order./plan/beginfailing means no plan exists, so nothing is provisioned at all and the check is completed neutral through the legacy planless shape (the only place the worker still names an outcome; it retires with that shape)./attemptcarries an explicit caller-generated key (plan:candidate:phase:seq), fresh per distinct attempt and identical on a replay, so a retry replays rather than duplicating a corpus row.evals_failedis never sent: it is not in the worker-facing union. A failed eval launch is{phase:'eval', ok:false, failureKind:'sandbox_error'}; once the run is bound, no further attempt is posted at all.The in-box eval-failure diagnostic survives as display text only — it no longer reclassifies an outcome, because after the eval attempt the verdict comes from the bound run.
#3644 hardening — verified intact after the refactor
/tmpPID file);head -c, nevercat);setsidwrapping, process group trusted only whenpgrp === pid;listener_mismatch(evidence about the candidate) andlistener_unknown(evidence about us) reported as different kinds;mcpjam.yamlstill attributedrecipe_invalidrather than collapsing to "absent" — pinned by a test that runs the REAL ladder.Re-run against a live E2B sandbox after the refactor (
scripts/verify-listener-identity.ts, four scenarios, four fresh boxes, all killed):Tests
New/rewritten, all over the existing injected-fake dep seam:
check-plan.test.ts(15) — idempotency keys, the same key on a replay, 409 = one call and a hard stop, 404/5xx = unreachable carrying the phase it was at, unknown action fails closed,/completesends nooutcome, and the degraded marker being dropped (not the completion) when the backend refuses it.resolve-and-start.test.ts(31) — the plan's order beats the detector's, each typed action drives the right next step, unknown action stops, 409 = no retry and no local fallback,/plan/candidatesunreachable executes nothing, pre-candidate provision/clone failures reported against the begun plan,recipe_invalid(incl. over-cap) andno_candidatesas ladder-scopedresolvefailures, fresh box per candidate with kill-before-provision, listener mismatch/unknown, plus the unchangedjudgeListenersmatrix.evidence-digest.test.ts(12) — the golden vector, budgets covering the detector, absent ≠ empty, unforgeable framing, byte- not code-unit truncation.github-checks-worker.test.ts(56) — plan begun before any sandbox work, eval attempt posted AT LAUNCH carrying the runId, no second attempt once bound, noevals_failedanywhere, degraded marker on the completion, planlessinfra_errorwith nothing executed.(
github-checks-worker.test.tsneedsmcpjam-inspector/node_modulesand the generated harness bundle present; in a bare worktree it fails to load on@ai-sdk/harness/agentat unmodified HEAD too. Both were supplied before the runs above, and the suite loads and passes.)Not done here
e2e awaits the backend deploy. The routes are merged on backend
mainbutplan/beginstill 404s in prod, so no live end-to-end was attempted. Once deployed: fixture withmcpjam.yaml→ declared; yaml deleted → detected; broken yaml →recipe_invalid; nothing resolvable →recipe_unresolvable.The follow-up that retires the legacy explicit-
outcome/completeshape and the worker-facing recipe PUT stays tracked; the planless report path in this PR is the last consumer of the former.🤖 Generated with Claude Code
Note
High Risk
Large refactor of GitHub check execution and verdict attribution; incorrect binding, completion, or degraded-mode handling could mis-report PR checks or strand runs until backend deploy aligns with the new routes.
Overview
Moves GitHub PR checks onto a backend-owned plan loop:
begin→ ladder attempts →candidates→ per-phase/attemptwith idempotency keys →/completewithout anoutcome. The control plane derives pass/fail from the attempt log and the bound eval run; the worker no longer sendsevals_failedor other PR verdicts.Worker flow:
/plan/beginruns before any sandbox work.resolveAndStartexecutes candidates in the plan’s order and obeys typed actions (continue_phase,try_next_candidate,run_eval,complete). On eval launch,onRunStartedposts{ phase: 'eval', ok: true, runId }immediately so the run binds to the check. Failures usedescribeCheckFailurefor display only; planlessinfra_errorremains only when begin never returns a plan.New modules:
check-plan.ts(HTTP session, 409 hard-stop, degradedbackend_unreachable, unknown action → fail closed),evidence-digest.ts(sole recipe evidence encoder + golden vector),service-route.ts(shared Convex transport).classifyCheckFailure/outcomeForRunResultand worker-side outcome reporting are removed.Reliability:
abandonPreparedRunfinalizes runs when eval binding (onRunStarted) throws so rows are not leftrunningwith pending iterations.Reviewed by Cursor Bugbot for commit ada5921. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Rewired the GitHub-checks worker onto the backend-owned plan/verdict loop so the backend orders candidates, returns typed actions, and derives the conclusion. The worker reports attempts with idempotency keys, binds the eval run at launch, completes with no outcome, and now uses a single evidence-digest encoder and a shared service-route transport with pinned behavior.
New Features
continue_phase,try_next_candidate,run_eval,complete); unknown actions fail closed.RESOLVER_VERSION; shared service-route transport.Bug Fixes
0means no deadline.repoFilesnote in the evidence digest: no cross-repo replay (cache keyed by repo+evidence), but within-repo listing changes may require a future hash-version bump.Written for commit ada5921. Summary will update on new commits.