Fix branch validation and error handling in setupBranch#1131
Fix branch validation and error handling in setupBranch#1131MrGDCrazy wants to merge 5 commits intoanthropics:mainfrom
Conversation
…lidation, prompt log exposure BUG-001 (HIGH): setupBranch() called process.exit(1) on error, bypassing run.ts's finally block. Tracking comments were never updated, step summaries never written, and action outputs never set on branch-setup failures. Changed to throw error so the catch/finally chain in run.ts runs normally. BUG-002 (MEDIUM): newBranch was interpolated into Bun's shell template ($`git ls-remote`) before validateBranchName() was called on it. Moved validation to immediately after branch name generation so the check always precedes any shell use. Also restructured the existence-check try/catch to separate the "exists" boolean from the fallback-generation logic, making error handling unambiguous. BUG-003 (LOW): context.inputs.prompt was logged verbatim to stdout, exposing potentially sensitive content in GitHub Actions logs (readable by anyone with repo read access). Truncated the preview to 100 characters. https://claude.ai/code/session_01VgvFdoRGPoJgsqJWtmVBoG
Remove spurious blank lines and realign markdown table columns. No content changes. https://claude.ai/code/session_01VgvFdoRGPoJgsqJWtmVBoG
There was a problem hiding this comment.
Pull request overview
This PR updates the branch setup flow to validate generated branch names earlier and to propagate failures via thrown errors (instead of exiting the process), enabling caller cleanup logic to run reliably.
Changes:
- Validate generated branch names before invoking any git shell commands, and re-validate after fallback name generation.
- Refactor
setupBranch()error handling to throw instead of callingprocess.exit(1). - Reduce log verbosity by printing a shortened prompt preview; plus minor documentation whitespace cleanups.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/github/operations/branch.ts |
Adds earlier branch-name validation, restructures remote-branch existence check, and changes fatal exit to throw. |
test/validate-branch-name.test.ts |
Adds regression-oriented tests asserting invalid branch names throw with descriptive messages. |
src/entrypoints/run.ts |
Logs a truncated prompt preview instead of the full prompt. |
docs/setup.md |
Removes extra blank lines/whitespace in lists. |
docs/security.md |
Removes extra blank lines/whitespace in lists. |
docs/migration-guide.md |
Removes extra blank lines/whitespace in lists. |
docs/configuration.md |
Removes extra blank lines/whitespace in lists. |
base-action/README.md |
Normalizes markdown spacing/formatting around tables and lists. |
base-action/CONTRIBUTING.md |
Removes an extra blank line/whitespace in a list section. |
.claude/commands/label-issue.md |
Removes extra blank lines/whitespace in task instructions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // BUG-001 regression: setupBranch() used to call process.exit(1) on errors, | ||
| // bypassing the finally block in run.ts. validateBranchName() is the gate that | ||
| // throws (instead of exiting) when a branch name is invalid. | ||
| describe("BUG-001: validateBranchName throws instead of exiting process", () => { | ||
| it("throws an Error for invalid names so callers can catch and propagate", () => { | ||
| // Verify the function throws a catchable Error, not a process exit | ||
| expect(() => validateBranchName("")).toThrow(Error); | ||
| expect(() => validateBranchName("bad branch!")).toThrow(Error); | ||
| expect(() => validateBranchName("-leading-dash")).toThrow(Error); | ||
| }); | ||
|
|
||
| it("error message is descriptive", () => { | ||
| expect(() => validateBranchName("")).toThrow(/cannot be empty/); | ||
| expect(() => validateBranchName("-x")).toThrow(/cannot start with a dash/); | ||
| expect(() => validateBranchName("a..b")).toThrow(/\.\./); | ||
| }); |
There was a problem hiding this comment.
The added BUG-001 regression tests only assert that validateBranchName() throws, but the regression described is setupBranch() calling process.exit(1) (preventing callers' finally blocks from running). As written, these tests would still pass even if setupBranch() exited the process. Consider adding a test that exercises setupBranch() in an error path (e.g., by forcing an invalid generated branch name) and asserts it throws/rejects without invoking process.exit.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@claude review |
… regression test From PR review comments on anthropics#1131: 1. git ls-remote catch block was swallowing ALL errors, treating network/auth failures as "branch doesn't exist". Fix checks exitCode: only code 2 (no matching refs) is expected and silently ignored; any other failure is logged and re-thrown so the caller sees the real error. 2. BUG-001 regression test only exercised validateBranchName(), not the actual setupBranch() error-propagation fix. Replaced with a test that calls setupBranch() with a mock octokit whose repos.get() throws, and asserts the promise rejects (not process.exit) — directly verifying the throw-vs-exit fix. https://claude.ai/code/session_01VgvFdoRGPoJgsqJWtmVBoG
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
The code changes look correct and well-motivated, but this PR modifies security-sensitive branch validation and error propagation paths that warrant human review.
Extended reasoning...
Overview
This PR makes three meaningful code changes plus documentation whitespace cleanup across 10 files:
src/github/operations/branch.ts: Replacesprocess.exit(1)withthrow errorinsetupBranch(), adds earlyvalidateBranchName()calls before shell use, and improvesgit ls-remoteerror handling to distinguish exit code 2 (no matching refs) from real failures.src/entrypoints/run.ts: Truncates prompt logging to 100 characters to prevent full prompt exposure in CI logs.test/validate-branch-name.test.ts: Adds BUG-001 regression tests verifyingvalidateBranchName()throws catchable errors.- Documentation files: Purely whitespace normalization (removing blank lines after markdown list markers).
Security risks
The branch validation changes are security-relevant — they affect command injection prevention. The changes themselves are improvements (validating earlier, before shell use), but any modification to this code path should be reviewed by someone familiar with the threat model. The prompt truncation in run.ts is also a security improvement, preventing accidental log exposure.
Level of scrutiny
This PR modifies security-sensitive code paths (branch name validation used to prevent command injection, error handling flow changes from process.exit to throw). The process.exit(1) to throw error change alters how errors propagate through the entire run() function in run.ts, which relies on its finally block for cleanup. While the change is correct and intentional, it deserves human verification that all callers handle the thrown error appropriately.
Other factors
Copilot previously suggested that the BUG-001 regression tests should exercise setupBranch() directly rather than just validateBranchName(), since the actual bug was setupBranch() calling process.exit(1). This feedback appears unaddressed — the tests still only verify validateBranchName() behavior. The pre-existing bug about duplicate fallback branch names (identified by the bug hunter) is genuinely pre-existing and not introduced by this PR.
| error, | ||
| ); | ||
| throw error; | ||
| } | ||
| // exitCode === 2: branch doesn't exist, continue with generated name | ||
| } | ||
|
|
||
| if (branchAlreadyExists) { | ||
| // If we get here, branch exists (exit code 0) |
There was a problem hiding this comment.
🟣 Pre-existing bug: When branchNameTemplate is undefined (no custom template configured), the fallback branch name generation at line 261 calls generateBranchName(undefined, ...) which produces the exact same name as the initial call at line 224, since both use the default format ${branchPrefix}${entityType}-${entityNumber}-${timestamp} with minute-level timestamp precision. The fallback silently produces a duplicate name, causing the subsequent git checkout -b to fail.
Extended reasoning...
What the bug is
In setupBranch(), when the initially generated branch name already exists on the remote, the code falls back to generating a new name by calling generateBranchName(undefined, ...) at line 261. The comment says "Force default template" — the intent is to fall back from a custom template to the default format. However, when branchNameTemplate is already undefined (i.e., no custom template was configured), this fallback produces the exact same branch name.
Why both calls produce the same name
Looking at generateBranchName() in src/utils/branch-template.ts (line 64-99): when the template parameter is undefined, the function skips the template branch (line 85) and falls through to the default format at line 96: ${branchPrefix}${entityType}-${entityNumber}-${variables.timestamp}. The timestamp variable is constructed at line 79 with minute-level precision (YYYYMMDD-HHMM). Since both the initial call (line 224) and the fallback call (line 261) execute within the same function invocation — separated only by a git ls-remote — they always share the same minute, producing identical branch names.
Step-by-step proof
- User has no
branchNameTemplateconfigured (it isundefined). setupBranch()is called for issue GettingAPI Error: 404whenuse_bedrock: "true"#42 at 2025-01-15 10:30.- Line 224:
generateBranchName(undefined, "claude/", "issue", 42, sha, ...)→ producesclaude/issue-42-20250115-1030. - Line 234:
validateBranchName("claude/issue-42-20250115-1030")passes. - Line 237-249:
git ls-remotefinds this branch already exists (from a previous run in the same minute, or a stale branch). branchAlreadyExists = true.- Line 261:
generateBranchName(undefined, "claude/", "issue", 42, sha, ...)→ producesclaude/issue-42-20250115-1030again (same inputs, same minute). - Line 271:
validateBranchNamepasses (name is valid, just duplicate). - Later,
git checkout -b claude/issue-42-20250115-1030fails because this branch already exists.
Impact
When no custom branch template is configured and the generated branch already exists on the remote, the fallback silently generates the same name and git checkout -b will fail. The error is then thrown up to the caller. This scenario requires a branch from a previous run (in the same minute) to still exist, making it uncommon but possible — e.g., if a workflow re-triggers quickly or a stale branch was never cleaned up.
How to fix
Append a disambiguator in the fallback path, such as a short random suffix or incrementing counter. For example:
newBranch = `${newBranch}-${Math.random().toString(36).substring(2, 6)}`;Alternatively, the fallback could append seconds or milliseconds to the timestamp to differentiate it from the original.
Pre-existing status
This is a pre-existing bug — the same generateBranchName(undefined, ...) fallback logic existed before this PR. The PR restructures the control flow (introducing the branchAlreadyExists flag and improved error handling for git ls-remote) but does not change the fundamental fallback behavior.
…emplate
When a generated branch name already exists on the remote and no custom
branchNameTemplate is configured, the fallback call to generateBranchName()
produces the exact same name as the original call: both use the default format
`${branchPrefix}${entityType}-${entityNumber}-${YYYYMMDD-HHMM}`, and since
both calls happen within the same minute the timestamp is identical.
The subsequent `git checkout -b <name>` then fails because the branch already
exists locally after the git ls-remote check.
Fix: append a 4-character base36 random suffix in the fallback path so the
fallback name is guaranteed to differ from the existing branch name.
Pre-existing bug identified in anthropics#1131 review.
https://claude.ai/code/session_01VgvFdoRGPoJgsqJWtmVBoG
Summary
This PR fixes error handling in the branch setup process by ensuring that
validateBranchName()throws catchable errors instead of callingprocess.exit(), allowing proper error propagation through the finally block inrun.ts.Key Changes
setupBranch()to throw errors instead of callingprocess.exit(1), enabling proper cleanup in the caller's finally blockbranchAlreadyExists) for clearer control flow and proper error handlingvalidateBranchName(newBranch)call that was executed after the branch existence checkvalidateBranchName()throws catchable errors with descriptive messagesImplementation Details
The branch validation is now performed at two strategic points:
This ensures invalid branch names are caught early and prevents them from being passed to shell commands, improving both security and error reporting.
https://claude.ai/code/session_01VgvFdoRGPoJgsqJWtmVBoG