fix(forge-core): drop -o pipefail from the push script — dash aborts on it (closes #62) - #63
Merged
Merged
Conversation
…s on it Closes #62 `pushBranch`'s fresh-mint script opened with `set -euo pipefail`, but `sandbox.exec` runs every command through `sh -c`, and `/bin/sh` in the sandbox image is dash. dash has no `-o pipefail`, so it aborted on line 1 with exit 2 and `git push` never ran. With APP_ID/APP_PRIVATE_KEY present — i.e. every CI run — no agent PR could be opened at all. Forge's factory has been fully down since #61 merged. Reproduced and fixed under the exact shell: OLD $ dash -c 'set -euo pipefail; echo reached' dash: 1: set: Illegal option -o pipefail exit 2 NEW $ dash -c 'set -eu; ...' LINE 7 REACHED — push would run; mode=600 bytes=11 exit 0 `-o pipefail` was never load-bearing here: the script contains no pipeline. `set -eu` keeps the fail-fast behaviour that matters, and the mode-600 token file, the EXIT trap and the empty `credential.helper=` reset are all untouched — verified still present in the dash run above. Fixed in both copies that go through `sandbox.exec`: - packages/forge-core/src/sandcastle-runners.ts - .sandcastle/agent-implement-issue.ts Deliberately NOT changed: packages/forge-cli/src/stamp.ts's generated verify stub, which carries a `#!/usr/bin/env bash` shebang and is invoked as `bash verify/<id>.sh` — pipefail is valid and wanted there. Not propagating: connector's runner has no wrapper script, so no other factory repo carries this shape. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #62. Regression from #61, which I merged — owning the fix.
pushBranch's mint script opened withset -euo pipefail, butsandbox.execruns throughsh -cand the sandbox's/bin/shis dash. dash has no-o pipefail: it aborts on line 1 with exit 2, sogit pushnever ran. Every CI run has APP_ID/APP_PRIVATE_KEY set, so no agent PR could be opened — the factory has been down since #61 landed at 19:15Z.Reproduced under the exact shell, both directions
-o pipefailwas never load-bearing: the script contains no pipeline.set -eukeeps the fail-fast behaviour, and the dash run above confirms the mode-600 token file and the token round-trip still work. The emptycredential.helper=reset — the load-bearing part of #61 — is untouched.Scope
Fixed both copies reached via
sandbox.exec:packages/forge-core/src/sandcastle-runners.tsand.sandcastle/agent-implement-issue.ts.Not changed:
packages/forge-cli/src/stamp.ts's generated verify stub — it carries#!/usr/bin/env bashand is invoked asbash verify/<id>.sh, so pipefail is correct there.Not propagating: connector's runner has no wrapper script; no other factory repo carries this shape. Checked before assuming.
Test note
pnpm testshows 5 failed files / 6 failed tests — identical onmain(runPrGateLadder is not a function, indoctor.test.ts), so pre-existing and unrelated. Flagging separately that Forge'sgatepasses while those fail, which means the gate is not running the full suite.How this got in
I reviewed #61's diff and checked the credential handling, which was correct — but I did not check the shell the script runs under. The gate cannot catch it: the push path only executes in-sandbox at runtime.