fix: make agent and review failure handling resilient - #27
Conversation
The installers ran a single network install (curl|bash for Claude, npm install -g for Codex) with no retry, so one transient failure (DNS SERVFAIL, connection reset) on a CI runner aborted the whole run. The thrown error carried only the exit code, surfacing as the opaque "exited with code 1" with the real cause buried in logs. Extract a shared installer-support module providing retryWithBackoff, formatInstallerError (captures the tail of installer output), and runCapturing (streams and captures stdout/stderr). Both installers now retry with linear backoff and surface the underlying failure. The Claude install additionally runs under `set -o pipefail` so a curl-leg failure propagates as the pipeline exit code instead of being masked by bash exiting 0 on empty stdin.
When every review lens failed before returning a verdict (e.g. the Claude CLI never installed), the aggregator emitted REQUEST_CHANGES, blocking an unevaluated PR with a misleading "changes requested" verdict and consuming a review iteration. Add an `errored` aggregate outcome for the all-failed case. It posts a non-blocking COMMENT plus an `error` commit status and leaves verdict labels untouched so a re-trigger re-reviews rather than parking the PR. To stop a persistently broken CLI from spinning the pipeline silently, persist a consecutive-error counter in the PR footer (Shopfloor-Review-Error-Count) and escalate to the review-stuck label after MAX_CONSECUTIVE_REVIEW_ERRORS so a human is paged, mirroring the iteration-cap backstop. A completed review resets the counter. reviewOnly mode stays stateless: no counter, no labels, every push reviewed fresh.
08b620c to
92ac0dd
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Shopfloor agent review: changes requested (iteration 1/Infinity).
- Diff follows the repo's documented conventions (CLAUDE.md; no AGENTS.md/CONTRIBUTING.md exist): committed dist, tests under test/, pnpm-based scripts, and footer handling consistent with the existing Shopfloor-Review-Iteration pattern.
- The consecutive-error escalation backstop never fires in pipeline mode because resolveStage/parsePrMetadata never read the persisted Shopfloor-Review-Error-Count back into routed.reviewErrorCount.
- Reviewed the full diff (installer hardening, review error-aggregation, footer counter); shell commands use hardcoded version constants with no user input, spawn calls use arg arrays, and PR-body parsing is numeric regex only — no exploitable security issues found.
- Reviewed the full diff (installer hardening + errored-review aggregate/apply path); shared logic is properly extracted into installer-support.ts and duplicated formatting consolidated into renderLensFailure, with no dead code, misleading names, or substantial duplication worth flagging.
| // stateless reviewer: no iteration counter is persisted, every push | ||
| // gets a fresh review, and the iteration cap never fires. | ||
| const iteration = ctx.reviewOnly ? 0 : (routed.reviewIteration ?? 0); | ||
| const errorCount = ctx.reviewOnly ? 0 : (routed.reviewErrorCount ?? 0); |
There was a problem hiding this comment.
[bug / confidence 88]
The consecutive-error escalation backstop is dead in the normal pipeline (non-reviewOnly) path.
errorCount reads routed.reviewErrorCount, but that field is only ever set by resolveReviewOnly (machine.ts:608). The pipeline review stage is routed by resolvePullRequestEvent (machine.ts:444-449 and 471-476), which sets reviewIteration but never reviewErrorCount, and parsePrMetadata (metadata.ts:29-35) parses only the iteration line — not Shopfloor-Review-Error-Count.
Execution trace (pipeline mode): errored run #1 → routed.reviewErrorCount is undefined → errorCount = 0 → aggregateFindings computes errorCount = 0 + 1 = 1, escalate = false → apply.ts persists Shopfloor-Review-Error-Count: 1 to the PR footer. Next push fires synchronize → resolveStage returns a review decision with no reviewErrorCount → run #2 again reads 0 → computes 1 → never reaches MAX_CONSECUTIVE_REVIEW_ERRORS. review-stuck is therefore never applied, so a persistent infrastructure failure spins silently forever — exactly the failure mode this PR claims to fix. (reviewOnly mode also never escalates, but that is intended.)
Fix needs parsePrMetadata to parse the error-count line and the pipeline review decisions to pass reviewErrorCount through, mirroring how reviewIteration is wired.
The consecutive-error backstop never fired because the live pipeline review routing reads PR metadata via parsePrMetadata, which did not parse Shopfloor-Review-Error-Count, so routed.reviewErrorCount was always undefined (treated as 0) and escalation could never trigger. The count had only been wired into resolveReviewOnly, where the backstop is intentionally disabled, so it was dead code. Parse Shopfloor-Review-Error-Count in parsePrMetadata and forward meta.reviewErrorCount from the two review-stage router decisions; drop the unused resolveReviewOnly parse.
There was a problem hiding this comment.
Shopfloor agent review: changes requested (iteration 1/Infinity).
- Reviewed the full source diff against CLAUDE.md (the only convention file present); file placement, ESM import style, and the committed dist rebuild all comply, and no forbidden commands or pinned formatting/commit rules are violated.
- The previous dead-backstop bug is now correctly fixed (metadata/machine/runner all thread reviewErrorCount), but the approve path fails to reset the consecutive-error counter, contradicting the stated design and allowing non-consecutive errors to escalate to review-stuck.
- No exploitable security issues: installer commands use constant version strings (no injection), no hardcoded secrets, and no new auth/SSRF/path-traversal surface; the changes are CLI-install retry hardening and review-error state plumbing.
- The diff is well-factored: shared installer-support module, deduplicated lens-failure rendering, and no dead imports; the only minor duplication (retry config across the two installer files) falls below the smell confidence threshold.
| // A completed review proves the CLI works, so reset any consecutive-error | ||
| // counter while writing the iteration line. | ||
| const newBody = writeIterationToBody( | ||
| stripErrorCountLine(ctx.pr.body), |
There was a problem hiding this comment.
[bug / confidence 84]
[bug / confidence 84] The consecutive-error counter is reset on request_changes (here, via stripErrorCountLine) but NOT on approve. The stripErrorCountLine helper comment (apply.ts:25-27) explicitly states the reset should happen on any review that completes — "clean or with findings" — yet the approve branch (apply.ts:56-84) posts the APPROVE review and updates labels without ever calling updatePrBody, so Shopfloor-Review-Error-Count survives a clean approval.
Expected vs actual: expected a clean approve to clear the counter (since the CLI demonstrably worked); actual is the counter persists.
Execution trace (pipeline mode): push 1 errors → footer gets Shopfloor-Review-Error-Count: 1. Push 2 is clean → approve path runs, body untouched, count stays 1. Push 3 (synchronize on the impl PR, whose footer still says Shopfloor-Stage: implement) re-routes to review via machine.ts:459-478 with reviewErrorCount = 1. If push 3 errors → errorCount = 2; push 4 errors → 3 → escalate fires and review-stuck is applied. So errors separated by a successful clean review are wrongly treated as consecutive and prematurely page a human, defeating the "consecutive" semantics the counter is meant to enforce.
Fix: mirror the request_changes branch and strip the error-count line on the approve path (and ideally update the PR body there).
request_changes reset the consecutive-error counter but approve did not, even though the counter is meant to reset on any completed review (the CLI demonstrably worked). An error followed by a clean approval followed by more errors was therefore counted as consecutive and could page a human prematurely. Strip Shopfloor-Review-Error-Count from the PR body on approve when present.
There was a problem hiding this comment.
Shopfloor agent review: clean across 4/4 reviewers.
- No coding-standard or convention violations found: CLAUDE.md is the only convention file, and the diff (dist rebuild, pnpm toolchain, new footer field) breaks none of its written rules.
- Inspected the full diff: both previously-flagged bugs (dead pipeline-mode escalation and missing approve-path counter reset) are now fixed and tested, and I found no new logic defects in the error-count flow or installer hardening.
- No exploitable security issues: shell interpolation uses only hardcoded version constants, spawn calls avoid shell injection, URLs are non-user-controlled, and no secrets, SQLi, path traversal, or auth gaps are introduced.
- Reviewed the diff for maintainability smells; the PR extracts shared installer logic and a lens-failure renderer to reduce duplication, has no dead code, and its remaining minor repetition matches existing codebase conventions — no smells meet the reporting threshold.
1 participant
Summary
Hardens how Shopfloor handles agent and review failures, motivated by a dogfood review (konfirmity/app#799) where all four review lenses failed with
Claude CLI installer exited with code 1(a transientgetaddrinfo ESERVFAIL downloads.claude.aion the runner) and the result was surfaced as CHANGES_REQUESTED on an already-approved PR.Changes
fix(agents): classify timeout-triggered Claude aborts asagent_timeoutPre-existing commit on this branch: aborts caused by the stage timeout are now reported as
agent_timeoutrather than a generic execution error.fix(setup): harden Claude and Codex CLI installersinstaller-supportmodule:retryWithBackoff,formatInstallerError(captures the tail of installer output), andrunCapturing(streams + captures stdout/stderr).exited with code 1.set -o pipefailso a curl-leg failure propagates instead of being masked bybashexiting 0 on empty stdin.fix(review): classify all-reviewer infrastructure failures aserrorederroredaggregate outcome for the case where every lens fails before returning a verdict. It posts a non-blocking COMMENT plus anerrorcommit status and leaves verdict labels untouched, so a re-trigger re-reviews rather than blocking an unevaluated PR with a misleadingCHANGES_REQUESTED.Shopfloor-Review-Error-Countin the PR footer and escalates toreview-stuckafter a threshold so a persistent infrastructure failure pages a human instead of spinning silently. A completed review resets the counter.reviewOnlymode stays stateless: no counter, no labels, every push reviewed fresh.Testing
pnpm test— 303 passedpnpm typecheck,pnpm lint— cleanpnpm build—distrebuilt and committed