fix(spawn): map invalid branch name to 400 instead of opaque 500 (#213)

validateBranch returned an untyped error for a name rejected by
git check-ref-format, so toAPIError fell through to INTERNAL_ERROR 500.

Add a ports.ErrWorkspaceBranchInvalid sentinel (mirroring the not-fetched /
checked-out-elsewhere ones), wrap it in validateBranch, and map it to
INVALID_BRANCH (400). Completes the residual of #152 Bug 3, which typed the
not-fetched and checked-out-elsewhere cases but left the invalid-format case
collapsing to 500.

Closes #212
This commit is contained in:
Khushi Diwan 2026-06-15 14:50:58 +05:30 committed by GitHub
parent cbf3f0a2db
commit 9e84a3b5ae
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 36 additions and 1 deletions

View File

@ -34,6 +34,7 @@ var (
var ( var (
ErrBranchCheckedOutElsewhere = ports.ErrWorkspaceBranchCheckedOutElsewhere ErrBranchCheckedOutElsewhere = ports.ErrWorkspaceBranchCheckedOutElsewhere
ErrBranchNotFetched = ports.ErrWorkspaceBranchNotFetched ErrBranchNotFetched = ports.ErrWorkspaceBranchNotFetched
ErrBranchInvalid = ports.ErrWorkspaceBranchInvalid
) )
// RepoResolver maps a project to the absolute path of its source git repo. // RepoResolver maps a project to the absolute path of its source git repo.
@ -282,7 +283,7 @@ func (w *Workspace) addWorktree(ctx context.Context, repo, path, branch, baseBra
func (w *Workspace) validateBranch(ctx context.Context, repo, branch string) error { func (w *Workspace) validateBranch(ctx context.Context, repo, branch string) error {
if _, err := w.run(ctx, w.binary, checkRefFormatBranchArgs(repo, branch)...); err != nil { if _, err := w.run(ctx, w.binary, checkRefFormatBranchArgs(repo, branch)...); err != nil {
return fmt.Errorf("gitworktree: invalid branch %q: %w", branch, err) return fmt.Errorf("%w: %q (%w)", ErrBranchInvalid, branch, err)
} }
return nil return nil
} }

View File

@ -406,6 +406,34 @@ func TestAddWorktreeRefusesBranchCheckedOutElsewhere(t *testing.T) {
} }
} }
// TestCreateRejectsInvalidBranchName covers the residual of #152 Bug 3: a branch
// name rejected by `git check-ref-format` must surface
// ports.ErrWorkspaceBranchInvalid so the HTTP layer renders a typed 400 instead
// of leaking raw git stderr through a 500.
func TestCreateRejectsInvalidBranchName(t *testing.T) {
root := t.TempDir()
repo := t.TempDir()
ws, err := New(Options{ManagedRoot: root, RepoResolver: StaticRepoResolver{"proj": repo}})
if err != nil {
t.Fatalf("new: %v", err)
}
ws.run = func(_ context.Context, _ string, args ...string) ([]byte, error) {
joined := strings.Join(args, " ")
if strings.Contains(joined, "check-ref-format") {
return nil, errors.New("fatal: 'bad branch!!' is not a valid branch name")
}
t.Fatalf("no git beyond check-ref-format should run for an invalid branch: %v", args)
return nil, nil
}
_, err = ws.Create(context.Background(), ports.WorkspaceConfig{ProjectID: "proj", SessionID: "sess", Branch: "bad branch!!"})
if !errors.Is(err, ports.ErrWorkspaceBranchInvalid) {
t.Fatalf("err = %v, want ports.ErrWorkspaceBranchInvalid", err)
}
if !strings.Contains(err.Error(), "bad branch!!") {
t.Fatalf("err = %v, want message to include the rejected branch name", err)
}
}
// TestAddWorktreeReportsBranchNotFetched covers Bug 3 (b): if no local head, // TestAddWorktreeReportsBranchNotFetched covers Bug 3 (b): if no local head,
// no origin remote-tracking branch, no default branch ref, and no tag of the // no origin remote-tracking branch, no default branch ref, and no tag of the
// same name is reachable, Create must surface ports.ErrWorkspaceBranchNotFetched // same name is reachable, Create must surface ports.ErrWorkspaceBranchNotFetched

View File

@ -118,6 +118,9 @@ var (
// ErrWorkspaceBranchNotFetched reports the requested branch exists nowhere // ErrWorkspaceBranchNotFetched reports the requested branch exists nowhere
// reachable (no local head, no remote-tracking branch, no tag). // reachable (no local head, no remote-tracking branch, no tag).
ErrWorkspaceBranchNotFetched = errors.New("workspace: branch is not fetched") ErrWorkspaceBranchNotFetched = errors.New("workspace: branch is not fetched")
// ErrWorkspaceBranchInvalid reports the requested branch name is not a valid
// git ref (rejected by `git check-ref-format`).
ErrWorkspaceBranchInvalid = errors.New("workspace: invalid branch name")
// ErrWorkspaceDirty reports Destroy refused to remove a workspace because // ErrWorkspaceDirty reports Destroy refused to remove a workspace because
// it holds uncommitted changes or untracked files. Teardown is never // it holds uncommitted changes or untracked files. Teardown is never
// forced; callers treat the workspace as intentionally preserved. // forced; callers treat the workspace as intentionally preserved.

View File

@ -344,6 +344,8 @@ func toAPIError(err error) error {
return apierr.Conflict("BRANCH_CHECKED_OUT_ELSEWHERE", err.Error(), nil) return apierr.Conflict("BRANCH_CHECKED_OUT_ELSEWHERE", err.Error(), nil)
case errors.Is(err, ports.ErrWorkspaceBranchNotFetched): case errors.Is(err, ports.ErrWorkspaceBranchNotFetched):
return apierr.Invalid("BRANCH_NOT_FETCHED", err.Error(), nil) return apierr.Invalid("BRANCH_NOT_FETCHED", err.Error(), nil)
case errors.Is(err, ports.ErrWorkspaceBranchInvalid):
return apierr.Invalid("INVALID_BRANCH", err.Error(), nil)
case errors.Is(err, ports.ErrAgentBinaryNotFound): case errors.Is(err, ports.ErrAgentBinaryNotFound):
return apierr.Invalid("AGENT_BINARY_NOT_FOUND", err.Error(), nil) return apierr.Invalid("AGENT_BINARY_NOT_FOUND", err.Error(), nil)
default: default:

View File

@ -289,6 +289,7 @@ func TestToAPIErrorMapsWorkspaceBranchSentinels(t *testing.T) {
}{ }{
{"checked out elsewhere", fmt.Errorf("spawn mer-1: workspace: %w: \"x\" is checked out at \"/tmp\"", ports.ErrWorkspaceBranchCheckedOutElsewhere), apierr.KindConflict, "BRANCH_CHECKED_OUT_ELSEWHERE"}, {"checked out elsewhere", fmt.Errorf("spawn mer-1: workspace: %w: \"x\" is checked out at \"/tmp\"", ports.ErrWorkspaceBranchCheckedOutElsewhere), apierr.KindConflict, "BRANCH_CHECKED_OUT_ELSEWHERE"},
{"not fetched", fmt.Errorf("spawn mer-1: workspace: %w: \"x\" has no local head", ports.ErrWorkspaceBranchNotFetched), apierr.KindInvalid, "BRANCH_NOT_FETCHED"}, {"not fetched", fmt.Errorf("spawn mer-1: workspace: %w: \"x\" has no local head", ports.ErrWorkspaceBranchNotFetched), apierr.KindInvalid, "BRANCH_NOT_FETCHED"},
{"invalid branch", fmt.Errorf("spawn mer-1: workspace: %w: \"bad!!\" (exit 1)", ports.ErrWorkspaceBranchInvalid), apierr.KindInvalid, "INVALID_BRANCH"},
{"agent binary not found", fmt.Errorf("spawn mer-1: %w", ports.ErrAgentBinaryNotFound), apierr.KindInvalid, "AGENT_BINARY_NOT_FOUND"}, {"agent binary not found", fmt.Errorf("spawn mer-1: %w", ports.ErrAgentBinaryNotFound), apierr.KindInvalid, "AGENT_BINARY_NOT_FOUND"},
{"unknown harness", fmt.Errorf("spawn: %w: %q", sessionmanager.ErrUnknownHarness, "bogus"), apierr.KindInvalid, "UNKNOWN_HARNESS"}, {"unknown harness", fmt.Errorf("spawn: %w: %q", sessionmanager.ErrUnknownHarness, "bogus"), apierr.KindInvalid, "UNKNOWN_HARNESS"},
} }