Skip to content

fix(open-pr): time out hung git push and gh pr create - #189

Merged
steipete merged 2 commits into
openclaw:mainfrom
SebTardif:fix/open-pr-command-timeout
Aug 16, 2026
Merged

fix(open-pr): time out hung git push and gh pr create#189
steipete merged 2 commits into
openclaw:mainfrom
SebTardif:fix/open-pr-command-timeout

Conversation

@SebTardif

Copy link
Copy Markdown
Contributor

What Problem This Solves

Fixes an issue where users running clawpatch open-pr would hang indefinitely when git push or gh pr create stalled on the remote. Providers and validation already pass timeoutMs into runCommandArgs. These two network calls did not, so a wedged origin or GitHub CLI pinned the process until an outer job killed it.

Why This Change Was Made

git push now uses a 120s deadline and gh pr create uses a 60s deadline, the same runCommandArgs({ timeoutMs }) path already used by providers and validation. Operators can raise or lower those values with CLAWPATCH_GIT_PUSH_TIMEOUT_MS and CLAWPATCH_GH_PR_CREATE_TIMEOUT_MS. Local git switch / add / commit calls are unchanged.

User Impact

A stalled push or GitHub CLI no longer leaves clawpatch open-pr running forever. The command fails closed with git-failure or github-failure and the timeout text from runCommandArgs. Successful opens behave the same.

Evidence

Before this change, src/open-pr.ts called runCommandArgs for git push and gh pr create with no timeoutMs. After the change, a live node import of the compiled helpers prints 120000 / 60000. A hung child with timeoutMs: 80 returns exit 124 and command timed out after 80ms.

Compiled open-pr call sites:

$ rg -n "timeoutMs" dist/open-pr.js
98:        timeoutMs: gitPushTimeoutMs(),
102:        timeoutMs: ghPrCreateTimeoutMs(),

Live compiled helpers:

$ node /tmp/proof-open-pr-timeout.mjs
open-pr network command timeouts:
{
  "gitPushMs": 120000,
  "ghPrCreateMs": 60000
}
hung node child with timeoutMs=80:
{
  "exitCode": 124,
  "stderr": "command timed out after 80ms",
  "elapsedMs": 583
}

Real behavior proof

  • Behavior or issue addressed: clawpatch open-pr hung forever when git push or gh pr create stalled, because those runCommandArgs calls had no timeoutMs.

  • Real environment tested: macOS 26.6.1, Node v26.7.0, clawpatch built from this branch at /tmp/oc-impl-clawpatch-timeout.

  • Exact steps or command run after this patch:

    node /tmp/proof-open-pr-timeout.mjs
    rg -n "timeoutMs" dist/open-pr.js
  • Evidence after fix: terminal output from the patched build:

    $ node /tmp/proof-open-pr-timeout.mjs
    open-pr network command timeouts:
    {
      "gitPushMs": 120000,
      "ghPrCreateMs": 60000
    }
    hung node child with timeoutMs=80:
    {
      "exitCode": 124,
      "stderr": "command timed out after 80ms",
      "elapsedMs": 583
    }
  • Observed result after fix: The compiled helpers report 120s for git push and 60s for gh pr create. A hung child is cut off with exit 124 instead of blocking the process.

  • What was not tested: A live GitHub.com push that actually stalls on the public remote. Local hang wrappers and the compiled runCommandArgs timeout path were used instead.

Summary

Same hang class as openclaw/clawsweeper#1175 (hung git I/O) and openclaw/clawpatch#115 (wedged Codex exec). The missing deadlines date to the open-pr extract in 157ed753 (2026-06-18).

A stalled origin or GitHub CLI used to block clawpatch open-pr
indefinitely because those two runCommandArgs calls had no timeoutMs.
Providers and validation already time out. Pass 120s for git push and
60s for gh pr create, with env overrides for operators.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
@SebTardif
SebTardif requested a review from a team as a code owner August 15, 2026 23:48
@clawsweeper

clawsweeper Bot commented Aug 15, 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: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P1 Urgent regression or broken agent/channel workflow affecting real users now. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Aug 15, 2026
@clawsweeper

clawsweeper Bot commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed August 15, 2026, 7:51 PM ET / 23:51 UTC.

ClawSweeper review

What this changes

The PR adds configurable timeouts to git push and GitHub CLI PR creation in clawpatch open-pr, adds simulated-hang tests, and adds an unreleased changelog entry.

Merge readiness

Blocked until stronger real behavior proof is added - 8 items remain

Keep open: the timeout wiring is focused, but the new 120-second push and 60-second PR-create defaults can fail existing slow workflows. Resolve that upgrade contract, remove the release-owned changelog edit, and provide an actual open-pr timeout transcript before merge.

Priority: P1
Reviewed head: 26f91d5d20c5f41b484cdc2c8067ee39f1ac9e39
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) The implementation and simulated-hang tests are focused, but upgrade safety and end-to-end proof remain unresolved.
Proof confidence 🦐 gold shrimp (3/6) Needs stronger real behavior proof before merge: The terminal output verifies compiled helpers and the shared runner, but not the patched open-pr workflow reaching either stalled boundary; add a redacted actual workflow transcript. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🦐 gold shrimp (3/6) 2 actionable review findings remain.

Verification

Check Result Evidence
Real behavior Needs proof Needs stronger real behavior proof before merge: The terminal output verifies compiled helpers and the shared runner, but not the patched open-pr workflow reaching either stalled boundary; add a redacted actual workflow transcript. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 5 items Current and proposed behavior: Current main runs the two remote commands without timeouts; the branch adds fixed 120,000 ms and 60,000 ms limits at those calls.
Timeout result: The shared runner kills a timed-out command and returns exit 124 with timeout text, so a slow remote beyond the new limit becomes an open-pr failure.
Configuration documentation: Current docs list environment overrides but do not document the two proposed open-pr timeout variables used as the recovery path.
Findings 2 actionable findings [P1] Preserve slow remote workflows before imposing short defaults
[P3] Remove the release-owned changelog edit
Security None None.

How this fits together

clawpatch open-pr pushes a prepared patch branch, creates a GitHub pull request, and saves the resulting remote state. This change bounds its two network-facing subprocesses so stalled commands return an error.

flowchart LR
  A[Recorded patch] --> B[Open PR workflow]
  B --> C[Git push]
  C --> D{Before timeout?}
  D -->|yes| E[GitHub CLI PR creation]
  D -->|no| F[Git failure]
  E --> G{Before timeout?}
  G -->|yes| H[Persist PR state]
  G -->|no| I[GitHub failure]
Loading

Decision needed

Question Recommendation
Should open-pr impose 120-second push and 60-second PR-creation deadlines for all existing users, or use a different documented compatibility policy? Use a compatibility-oriented policy: Choose a longer default or opt-in strategy that bounds confirmed hangs without surprising slow existing repositories.

Why: Current behavior has no deadline, while the branch turns slow operations past fixed limits into failures; the default and recovery contract need maintainer intent.

Before merge

  • Add real behavior proof - Needs stronger real behavior proof before merge: The terminal output verifies compiled helpers and the shared runner, but not the patched open-pr workflow reaching either stalled boundary; add a redacted actual workflow transcript. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Preserve slow remote workflows before imposing short defaults (P1) - These calls previously waited for the remote, but the 120s/60s defaults turn any slow-yet-successful operation into git-failure or github-failure. The new override variables are also undocumented. Settle a documented compatibility policy—such as a longer default or opt-in limit—and cover the selected upgrade behavior before making this the default.
  • Remove the release-owned changelog edit (P3) - CHANGELOG.md is release-owned under the applicable review policy, so this feature PR should leave release-note selection and attribution to the release process. Keep the user-visible summary in the PR body instead.
  • Resolve merge risk (P1) - Existing slow-but-successful pushes or PR-creation requests will now fail at 120 or 60 seconds; the override variables are not documented.
  • Resolve merge risk (P1) - The provided terminal proof checks helpers and the runner, not an actual open-pr workflow reaching either stalled boundary.
  • Resolve merge risk (P1) - The PR modifies release-owned CHANGELOG.md.
  • Complete next step (P2) - A maintainer must choose the upgrade contract for remote deadlines, and contributor real-workflow proof is required before merge.

Findings

  • [P1] Preserve slow remote workflows before imposing short defaults — src/open-pr.ts:150-158
  • [P3] Remove the release-owned changelog edit — CHANGELOG.md:5
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Patch surface 22 production lines changed, 196 test lines added, 1 changelog line added Most of the branch is a new hang-fixture test around a small workflow change.

Merge-risk options

Maintainer options:

  1. Settle and document the timeout contract (recommended)
    Choose an upgrade-safe default or opt-in policy, document recovery variables, and test the selected compatibility behavior.
  2. Accept short hard deadlines
    Merge the proposed limits as an intentional behavior change, accepting that slow existing operations can fail unless users set overrides.
  3. Pause the PR
    Pause this branch if maintainers do not want a new timeout configuration contract for open-pr.

Technical review

Best possible solution:

Adopt a documented, upgrade-safe timeout policy for remote publish steps, retain focused hang coverage, remove the changelog edit, and attach a redacted actual open-pr run against controlled stalled commands.

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

Yes for the source path: current open-pr lacks timeouts and the shared runner deterministically returns exit 124 when configured. The contributor has not yet shown that behavior through an actual open-pr invocation.

Is this the best way to solve the issue?

Unclear: the existing runner is the right narrow mechanism, but the proposed short defaults create an undocumented upgrade contract that needs approval.

Full review comments:

  • [P1] Preserve slow remote workflows before imposing short defaults — src/open-pr.ts:150-158
    These calls previously waited for the remote, but the 120s/60s defaults turn any slow-yet-successful operation into git-failure or github-failure. The new override variables are also undocumented. Settle a documented compatibility policy—such as a longer default or opt-in limit—and cover the selected upgrade behavior before making this the default.
    Confidence: 0.91
  • [P3] Remove the release-owned changelog edit — CHANGELOG.md:5
    CHANGELOG.md is release-owned under the applicable review policy, so this feature PR should leave release-note selection and attribution to the release process. Keep the user-visible summary in the PR body instead.
    Confidence: 0.98

Overall correctness: patch is incorrect
Overall confidence: 0.9

AGENTS.md: found and applied where relevant.

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

Labels

Label changes:

  • add P1: The proposed defaults can break currently successful remote publishing workflows after upgrade.
  • add merge-risk: 🚨 compatibility: Fixed default deadlines replace previously unbounded behavior for existing users.
  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🦐 gold shrimp.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The terminal output verifies compiled helpers and the shared runner, but not the patched open-pr workflow reaching either stalled boundary; add a redacted actual workflow transcript. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Label justifications:

  • P1: The proposed defaults can break currently successful remote publishing workflows after upgrade.
  • merge-risk: 🚨 compatibility: Fixed default deadlines replace previously unbounded behavior for existing users.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The terminal output verifies compiled helpers and the shared runner, but not the patched open-pr workflow reaching either stalled boundary; add a redacted actual workflow transcript. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

  • Current and proposed behavior: Current main runs the two remote commands without timeouts; the branch adds fixed 120,000 ms and 60,000 ms limits at those calls. (src/open-pr.ts:148, 26f91d5d20c5)
  • Timeout result: The shared runner kills a timed-out command and returns exit 124 with timeout text, so a slow remote beyond the new limit becomes an open-pr failure. (src/exec.ts:106, c19979fa0225)
  • Configuration documentation: Current docs list environment overrides but do not document the two proposed open-pr timeout variables used as the recovery path. (docs/configuration.md:69, c19979fa0225)
  • Feature history: Blame attributes the current push/PR-creation path to the v0.7.2 release commit; history identifies Peter Steinberger and Vincent Koc as recent contributors to this workflow. (src/open-pr.ts:148, 3c2a1a4e2d04)
  • Main and release status: The PR head is not contained by checked-out main or any local release tag, so current main and v0.7.2 do not already contain this change. (src/open-pr.ts:148, 26f91d5d20c5)

Likely related people:

  • Peter Steinberger: Current blame covers both the open-pr boundary and shared timeout runner; history also shows the original workflow extraction. (role: feature owner and recent area contributor; confidence: high; commits: 3c2a1a4e2d04, 157ed753251d, 0f0b645025b8; files: src/open-pr.ts, src/exec.ts)
  • Vincent Koc: Recent history includes focused fixes to the open-pr branch workflow. (role: recent adjacent contributor; confidence: medium; commits: 24763d26cdff, 4eb9a55d8f70; files: src/open-pr.ts)

Rank-up moves

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

  • Provide a redacted terminal transcript of patched open-pr against controlled stalled git-push and GitHub-CLI commands.
  • Resolve and document the default-timeout compatibility contract.
  • Remove the release-owned changelog line.

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.

@clawsweeper

clawsweeper Bot commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper status: review started.

I am starting a fresh review of this pull request: fix(open-pr): time out hung git push and gh pr create This is item 1/1 in the current shard. Shard 0/1.

This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking.

Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted.

@steipete
steipete merged commit 36b5c73 into openclaw:main Aug 16, 2026
4 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P1 Urgent regression or broken agent/channel workflow affecting real users now. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants