Skip to content

fix: timeout GitHub CLI calls in repair github-cli - #1302

Open
SebTardif wants to merge 1 commit into
openclaw:mainfrom
SebTardif:fix/f005-gh-cli-timeout
Open

fix: timeout GitHub CLI calls in repair github-cli#1302
SebTardif wants to merge 1 commit into
openclaw:mainfrom
SebTardif:fix/f005-gh-cli-timeout

Conversation

@SebTardif

Copy link
Copy Markdown

What Problem This Solves

Fixes an issue where ClawSweeper repair and review mutation workers would hang forever when a gh child stopped making progress. ghText, ghTextAsync, and ghSpawn launched the GitHub CLI through execFileSync, execFile, and spawnSync with no deadline, so one stalled API call blocked comment routing, PR intake, cluster planning, and other mutation paths until the process was killed.

Why This Change Was Made

GhRunOptions now accepts timeoutMs and every GitHub CLI spawn passes a resolved timeout through to Node. Callers can set a per-call budget. When they omit it, the helper uses CLAWSWEEPER_GH_COMMAND_TIMEOUT_MS or CLAWSWEEPER_NETWORK_COMMAND_TIMEOUT_MS, then a 2-minute default that matches the existing repair execute-fix-github budget. This is the same class of hang fix as media-proof and review-blob timeouts.

User Impact

Operators and hosted workers no longer lose an entire repair or comment-router process to one stuck gh invocation. A hung GitHub CLI call fails with a timeout instead of sitting on the event loop until the runner is cancelled.

OpenClaw Bay Impact

Unaffected. This change only bounds worker-side GitHub CLI child processes. Bay remains an observer-only surface and does not call gh.

Documentation Impact

No documentation change. docs/repair/README.md already describes CLAWSWEEPER_GH_COMMAND_TIMEOUT_MS and CLAWSWEEPER_NETWORK_COMMAND_TIMEOUT_MS as the GitHub CLI budget knobs. The repair helper now honors those same variables. The existing contract stays accurate.

Evidence

Live node run against compiled dist/repair/github-cli.js on this branch. GH_BIN pointed at a 30-second sleeper. Each helper was called with timeoutMs: 250.

Before this patch, ghText ignored timeoutMs and waited for the child to finish (late after 2000ms, no exception).

After this patch, all three helpers returned in 759ms instead of 90 seconds:

$ node /tmp/clawsweeper-F005-proof.mjs
{
  "elapsedMs": 759,
  "timeoutMs": 250,
  "ghText": {
    "message": "spawnSync /opt/homebrew/Cellar/node/26.7.0/bin/node ETIMEDOUT",
    "code": "ETIMEDOUT",
    "signal": "SIGTERM"
  },
  "ghTextAsync": {
    "message": "Command failed: /opt/homebrew/Cellar/node/26.7.0/bin/node --eval setTimeout(() => { process.stdout.write('late\\n'); }, 30000); -- api user",
    "code": null,
    "killed": true,
    "signal": "SIGTERM"
  },
  "ghSpawn": {
    "message": "spawnSync /opt/homebrew/Cellar/node/26.7.0/bin/node ETIMEDOUT",
    "code": "ETIMEDOUT"
  },
  "ghSpawnStatus": null,
  "ghSpawnSignal": "SIGTERM"
}

A fast command still succeeds under the same timeout field:

$ node /tmp/clawsweeper-F005-proof-success.mjs
{
  "elapsedMs": 25,
  "text": "ok"
}

Real behavior proof

  • Behavior or issue addressed: Repair GitHub CLI helpers (ghText, ghTextAsync, ghSpawn) no longer wait forever when the gh child hangs.

  • Real environment tested: macOS Darwin 25.6.0 arm64, Node v26.7.0, branch fix/f005-gh-cli-timeout at /tmp/oc-pr-clawsweeper-F005, compiled dist/repair/github-cli.js.

  • Exact steps or command run after this patch:

    node /tmp/clawsweeper-F005-proof.mjs
    node /tmp/clawsweeper-F005-proof-success.mjs
  • Evidence after fix: terminal output from the patched helper is in the Evidence section above. The 30-second sleeper was killed at 250ms per call (ETIMEDOUT / SIGTERM). The healthy command returned ok in 25ms.

  • Observed result after fix: Hung GitHub CLI children fail closed on a deadline. Successful commands still return their stdout. Default and env budgets remain available when timeoutMs is omitted.

  • What was not tested: Live gh api against api.github.com, Windows job-object kill behavior, and hosted comment-router workers under a real GitHub outage.

Summary

Related same-repo hang bounds: #1173 (ffmpeg/ffprobe), #1175 (git fetch and state-blob fetch). Repair execution already times out gh in src/repair/execute-fix-github.ts. This patch closes the shared helper used by comment-router and other mutation paths.

The unbounded execFileSync path has been present since 62dd779eb (2026-04-29, repair-lane unification), about 122 days.

Add timeoutMs to GhRunOptions and pass it through execFileSync,
execFile, and spawnSync. Default to the existing 2-minute repair
GitHub CLI budget so a stalled gh child fails instead of blocking
comment-router and other mutation paths.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
@clawsweeper

clawsweeper Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

@clawsweeper clawsweeper Bot added merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Aug 30, 2026
@clawsweeper

clawsweeper Bot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs changes before merge. Reviewed August 29, 2026, 8:01 PM ET / August 30, 2026, 00:01 UTC.

ClawSweeper review

What this changes

The PR adds configurable deadlines to repair-lane GitHub CLI helper calls so stalled child processes time out instead of blocking worker operations.

Merge readiness

⚠️ Needs maintainer review before merge - 3 items remain

Keep open: the timeout change is useful and its central timeout behavior has real runtime proof, but the new resolver ignores timeout settings supplied through a call’s existing env option, so a focused repair is needed before merge.

Priority: P2
Reviewed head: fbde9401b4148d95636ad895472f23b81677d49b

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) The central behavior has credible real-process proof, but a localized configuration-precedence defect blocks merge readiness.
Proof confidence 🐚 platinum hermit (4/6) Sufficient (terminal): The supplied PR-body terminal trace runs the compiled production GitHub CLI helper against a deliberately stalled real Node child process and observes ETIMEDOUT or SIGTERM for all three changed launch paths, while a healthy command returns output; it does not cover the separate per-call environment-resolution defect.
Patch quality 🦐 gold shrimp (3/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The supplied PR-body terminal trace runs the compiled production GitHub CLI helper against a deliberately stalled real Node child process and observes ETIMEDOUT or SIGTERM for all three changed launch paths, while a healthy command returns output; it does not cover the separate per-call environment-resolution defect.
Evidence reviewed 5 items Introduced resolver misses per-call environment: The new timeout resolver reads only the parent process environment, even though the helper accepts an env override and supplies that merged environment to the child process. A caller using env: { CLAWSWEEPER_GH_COMMAND_TIMEOUT_MS: ... } therefore gets the ambient/default deadline instead.
Existing option precedence establishes the intended pattern: The adjacent retry resolver already gives options.env precedence over process.env, so the new timeout resolver is inconsistent with the helper’s established per-call environment behavior.
Focused test omits the configuration path: The added test proves explicit timeoutMs for the three process-launch paths, but does not cover either documented environment timeout variable supplied through options.env.
Findings 1 actionable finding [P2] Honor timeout variables supplied through options.env
Security None None.

How this fits together

The repair lane invokes the GitHub CLI to read and mutate repository state for review and repair workflows. These helpers turn command arguments and environment settings into child-process results that feed routing, intake, and repair operations.

flowchart LR
A[Repair and review workers] --> B[GitHub CLI helper]
B --> C[Timeout selection]
C --> D[GitHub CLI child process]
D --> E[Command result or timeout]
E --> F[Routing and repair operations]
Loading

Before merge

  • Honor timeout variables supplied through `options.env` (P2) - ghCommandEnv passes the caller’s env override to the child, but this resolver only reads process.env. Thus an isolated worker that supplies CLAWSWEEPER_GH_COMMAND_TIMEOUT_MS or CLAWSWEEPER_NETWORK_COMMAND_TIMEOUT_MS through GhRunOptions.env silently uses the ambient/default deadline instead. Read the override first, as resolveRetryOptions already does, and add coverage.
  • Resolve merge risk (P2) - A worker that supplies its GitHub CLI timeout through GhRunOptions.env will silently use the parent-process value or two-minute default, weakening its intended deadline and potentially delaying recovery from a stalled command.
  • Complete next step (P2) - The introduced configuration-precedence defect is a narrow mechanical repair with clear source and test ownership.

Findings

  • [P2] Honor timeout variables supplied through options.envsrc/repair/github-cli.ts:393-395
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Patch scope production +27, tests +40, changelog +1 The change is narrowly scoped to one shared process helper and its direct test file.
Timeout coverage 3 helper paths exercised The supplied runtime trace covers synchronous, asynchronous, and spawn-result launch behavior, but not environment-derived timeout resolution.

Merge-risk options

Maintainer options:

  1. Honor per-call timeout environment (recommended)
    Update the timeout resolver to give options.env the same precedence as other per-call settings and add a focused regression test.
Copy recommended automerge instruction
@clawsweeper automerge

Special instructions:
Update ghRunTimeoutMs to give `options.env` timeout variables the same precedence as `resolveRetryOptions`, then add focused regression coverage.

Technical review

Best possible solution:

Resolve timeout variables from options.env before the ambient process environment, matching retry-option precedence, and cover that isolated-call configuration path with a regression test.

Do we have a high-confidence way to reproduce the issue?

Yes: the supplied post-fix terminal trace demonstrates the central child-process timeout path, and source inspection directly shows that an options.env timeout setting is not consulted.

Is this the best way to solve the issue?

No: applying deadlines is the right narrow approach, but the resolver should honor the per-call environment already accepted by the helper before this implementation is merged.

Full review comments:

  • [P2] Honor timeout variables supplied through options.envsrc/repair/github-cli.ts:393-395
    ghCommandEnv passes the caller’s env override to the child, but this resolver only reads process.env. Thus an isolated worker that supplies CLAWSWEEPER_GH_COMMAND_TIMEOUT_MS or CLAWSWEEPER_NETWORK_COMMAND_TIMEOUT_MS through GhRunOptions.env silently uses the ambient/default deadline instead. Read the override first, as resolveRetryOptions already does, and add coverage.
    Confidence: 0.96

Overall correctness: patch is incorrect
Overall confidence: 0.96

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 9f4da9302aea.

Labels

Label changes:

  • add P2: This is a bounded repair to shared worker command execution with a concrete configuration defect.
  • add merge-risk: 🚨 availability: The introduced deadline resolver can ignore a caller’s supplied worker timeout and delay recovery from a hung GitHub CLI process.
  • add proof: sufficient: Contributor real behavior proof is sufficient. The supplied PR-body terminal trace runs the compiled production GitHub CLI helper against a deliberately stalled real Node child process and observes ETIMEDOUT or SIGTERM for all three changed launch paths, while a healthy command returns output; it does not cover the separate per-call environment-resolution defect.
  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🐚 platinum hermit and patch quality is 🦐 gold shrimp.
  • add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (terminal): The supplied PR-body terminal trace runs the compiled production GitHub CLI helper against a deliberately stalled real Node child process and observes ETIMEDOUT or SIGTERM for all three changed launch paths, while a healthy command returns output; it does not cover the separate per-call environment-resolution defect.

Label justifications:

  • P2: This is a bounded repair to shared worker command execution with a concrete configuration defect.
  • merge-risk: 🚨 availability: The introduced deadline resolver can ignore a caller’s supplied worker timeout and delay recovery from a hung GitHub CLI process.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🐚 platinum hermit and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (terminal): The supplied PR-body terminal trace runs the compiled production GitHub CLI helper against a deliberately stalled real Node child process and observes ETIMEDOUT or SIGTERM for all three changed launch paths, while a healthy command returns output; it does not cover the separate per-call environment-resolution defect.
  • proof: sufficient: Contributor real behavior proof is sufficient. The supplied PR-body terminal trace runs the compiled production GitHub CLI helper against a deliberately stalled real Node child process and observes ETIMEDOUT or SIGTERM for all three changed launch paths, while a healthy command returns output; it does not cover the separate per-call environment-resolution defect.

Evidence

Acceptance criteria:

  • [P1] pnpm run build:repair.
  • [P1] pnpm run test:repair.
  • [P1] pnpm run check.

What I checked:

  • Introduced resolver misses per-call environment: The new timeout resolver reads only the parent process environment, even though the helper accepts an env override and supplies that merged environment to the child process. A caller using env: { CLAWSWEEPER_GH_COMMAND_TIMEOUT_MS: ... } therefore gets the ambient/default deadline instead. (src/repair/github-cli.ts:394, fbde9401b414)
  • Existing option precedence establishes the intended pattern: The adjacent retry resolver already gives options.env precedence over process.env, so the new timeout resolver is inconsistent with the helper’s established per-call environment behavior. (src/repair/github-cli.ts:409, fbde9401b414)
  • Focused test omits the configuration path: The added test proves explicit timeoutMs for the three process-launch paths, but does not cover either documented environment timeout variable supplied through options.env. (test/repair/github-cli.test.ts:215, fbde9401b414)
  • Runtime proof covers the central timeout behavior: The reviewed PR body records a post-fix compiled-helper run where a real Node child process acting as the configured GitHub CLI is terminated at the requested deadline for synchronous, asynchronous, and spawn-result helper paths; a fast command also succeeds. (fbde9401b414)
  • Current-main area history: Recent shared-helper maintenance includes public-read fallback work by Gio Della-Libera and review/publication safeguards by Peter Steinberger; the contributor branch is a verified one-commit delta from the pinned base. (src/repair/github-cli.ts:163, f6f6bfca7d65)

Likely related people:

  • Peter Steinberger: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • Gio Della-Libera: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Honor timeout variables supplied through options.env and add focused regression coverage for that precedence.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant