From b6e778d5abc9620dc917eb793828cf609eda18b6 Mon Sep 17 00:00:00 2001 From: jordan Date: Tue, 10 Feb 2026 20:57:27 -0700 Subject: [PATCH] fix(git): harden git flow for concurrent SDLC stress test failures 5 fixes from stress test analysis: 1. CRITICAL: Add pull-before-push to claudebox GitOperations.CommitAndPush, matching the fix already in PodGitOperations (prevents push rejections when concurrent builds advance the remote). 2. HIGH: Extract ResetToMain into PodGitOperations as a shared public method. Wire into BuildExecutor after CloneRepo and update SDLCTaskExecutor to use the shared method. Prevents builds from running on wrong branch when worker pods are reused across tasks. 3. HIGH: Make branch create push failure fatal with retry+rollback in cmd/sdlc/cmd_branch.go. Prevents orphaned .sdlc/ state that causes merge failures after completing all 10 SDLC phases. 4. MEDIUM: Shell-escape token in credential helpers (both PodGitOperations and claudebox GitOperations) to prevent shell injection via tokens containing special characters. 5. MEDIUM: Add GitResetToMain to claudebox sidecar (git.go implementation, server.go endpoint, client.go HTTP method) and wire into HTTPSDLCTaskExecutor for the HTTP sidecar path. Co-Authored-By: Claude Opus 4.6 --- cmd/sdlc/cmd_branch.go | 19 ++++++---- internal/adapter/claudebox/client.go | 50 +++++++++++++++++++++++++++ internal/claudebox/git.go | 26 ++++++++++++-- internal/claudebox/server.go | 38 ++++++++++++++++++++ internal/worker/build_executor.go | 9 +++++ internal/worker/http_sdlc_executor.go | 15 ++++++++ internal/worker/pod_git_operations.go | 34 +++++++++++++++++- internal/worker/sdlc_executor.go | 28 +-------------- 8 files changed, 183 insertions(+), 36 deletions(-) diff --git a/cmd/sdlc/cmd_branch.go b/cmd/sdlc/cmd_branch.go index b55de9c..e5f7e8c 100644 --- a/cmd/sdlc/cmd_branch.go +++ b/cmd/sdlc/cmd_branch.go @@ -2,7 +2,6 @@ package main import ( "fmt" - "os" "os/exec" "strings" "time" @@ -55,11 +54,19 @@ var branchCreateCmd = &cobra.Command{ pushCmd := exec.Command("git", "push", "origin", "HEAD") pushCmd.Dir = root - if out, err := pushCmd.CombinedOutput(); err != nil { - // Push failure is non-fatal. In executor contexts, origin exists - // (from git clone) and this succeeds. In local/test contexts without - // a remote, the commit is still persisted locally on main. - fmt.Fprintf(os.Stderr, "warning: failed to push .sdlc/ state: %s\n", strings.TrimSpace(string(out))) + if _, err := pushCmd.CombinedOutput(); err != nil { + // Retry once — transient Gitea failures are common + time.Sleep(2 * time.Second) + retryCmd := exec.Command("git", "push", "origin", "HEAD") + retryCmd.Dir = root + if retryOut, retryErr := retryCmd.CombinedOutput(); retryErr != nil { + // Roll back the local commit so state doesn't diverge + rollbackCmd := exec.Command("git", "reset", "--soft", "HEAD~1") + rollbackCmd.Dir = root + _ = rollbackCmd.Run() + return fmt.Errorf("failed to push branch state to remote (branch not created): %s: %w", + strings.TrimSpace(string(retryOut)), retryErr) + } } // Create the git branch diff --git a/internal/adapter/claudebox/client.go b/internal/adapter/claudebox/client.go index b2bd005..92c0c8f 100644 --- a/internal/adapter/claudebox/client.go +++ b/internal/adapter/claudebox/client.go @@ -312,6 +312,56 @@ func (c *Client) GitCommitAndPush(ctx context.Context, message string, push bool return &result, nil } +// GitResetToMainRequest is the request to reset the workspace to main. +type GitResetToMainRequest struct { + WorkDir string `json:"work_dir,omitempty"` +} + +// GitResetToMainResponse is the response from resetting to main. +type GitResetToMainResponse struct { + Success bool `json:"success"` + Error string `json:"error,omitempty"` +} + +// GitResetToMain resets the workspace to the main branch with a clean state. +func (c *Client) GitResetToMain(ctx context.Context, workDir string) (*GitResetToMainResponse, error) { + req := GitResetToMainRequest{ + WorkDir: workDir, + } + + body, err := json.Marshal(req) + if err != nil { + return nil, fmt.Errorf("marshal request: %w", err) + } + + httpReq, err := http.NewRequestWithContext(ctx, http.MethodPost, c.baseURL+"/git/reset-to-main", bytes.NewReader(body)) + if err != nil { + return nil, fmt.Errorf("create request: %w", err) + } + httpReq.Header.Set("Content-Type", "application/json") + + resp, err := c.httpClient.Do(httpReq) + if err != nil { + return nil, fmt.Errorf("git reset to main: %w", err) + } + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusOK { + bodyBytes, readErr := io.ReadAll(resp.Body) + if readErr != nil { + return nil, fmt.Errorf("git reset returned status %d (failed to read body: %w)", resp.StatusCode, readErr) + } + return nil, fmt.Errorf("git reset returned status %d: %s", resp.StatusCode, string(bodyBytes)) + } + + var result GitResetToMainResponse + if err := json.NewDecoder(resp.Body).Decode(&result); err != nil { + return nil, fmt.Errorf("decode response: %w", err) + } + + return &result, nil +} + // GitStatusResponse is the response from git status. type GitStatusResponse struct { IsRepo bool `json:"is_repo"` diff --git a/internal/claudebox/git.go b/internal/claudebox/git.go index f101561..f91303f 100644 --- a/internal/claudebox/git.go +++ b/internal/claudebox/git.go @@ -148,7 +148,7 @@ func (g *GitOperations) CloneRepo(ctx context.Context, workDir, cloneURL string) if err := cmd.Run(); err != nil { errMsg := g.redactToken(stderr.String()) stdoutMsg := g.redactToken(stdout.String()) - result.Error = fmt.Errorf("git clone exit %v: %s", err, errMsg) + result.Error = fmt.Errorf("git clone exit %w: %s", err, errMsg) g.logger.Error("git clone failed", "error", err, "stderr", errMsg, @@ -232,12 +232,20 @@ func (g *GitOperations) CommitAndPush(ctx context.Context, workDir, message stri if push { // Configure credential helper if g.giteaToken != "" { - credHelper := fmt.Sprintf("!f() { echo username=token; echo password=%s; }; f", g.giteaToken) + // Use single quotes around password to prevent shell interpretation + credHelper := fmt.Sprintf("!f() { echo username=token; echo 'password=%s'; }; f", + strings.ReplaceAll(g.giteaToken, "'", "'\\''")) if err := g.runGit(ctx, workDir, "config", "credential.helper", credHelper); err != nil { g.logger.Debug("credential helper config failed, continuing with push", "error", err) } } + // Pull before push to handle concurrent builds that may have advanced the remote. + if err := g.runGit(ctx, workDir, "pull", "--rebase", "origin", "HEAD"); err != nil { + g.logger.Warn("git pull --rebase before push failed, attempting push anyway", + "error", err, "work_dir", workDir) + } + if err := g.runGit(ctx, workDir, "push", "origin", "HEAD"); err != nil { result.Error = fmt.Errorf("git push: %w", err) return result @@ -248,6 +256,20 @@ func (g *GitOperations) CommitAndPush(ctx context.Context, workDir, message stri return result } +// ResetToMain resets the workspace to the main branch with a clean state. +func (g *GitOperations) ResetToMain(ctx context.Context, workDir string) error { + if err := g.runGit(ctx, workDir, "fetch", "origin"); err != nil { + return fmt.Errorf("git fetch: %w", err) + } + if err := g.runGit(ctx, workDir, "checkout", "main"); err != nil { + return fmt.Errorf("git checkout main: %w", err) + } + if err := g.runGit(ctx, workDir, "reset", "--hard", "origin/main"); err != nil { + return fmt.Errorf("git reset --hard: %w", err) + } + return nil +} + // GitStatusResult contains git status information. type GitStatusResult struct { IsRepo bool `json:"is_repo"` diff --git a/internal/claudebox/server.go b/internal/claudebox/server.go index fda8440..05daffb 100644 --- a/internal/claudebox/server.go +++ b/internal/claudebox/server.go @@ -55,6 +55,7 @@ func (s *Server) Mount(r chi.Router) { r.Post("/execute/stream", s.handleExecuteStream) r.Post("/git/clone", s.handleGitClone) r.Post("/git/commit-and-push", s.handleGitCommitAndPush) + r.Post("/git/reset-to-main", s.handleGitResetToMain) r.Get("/git/status", s.handleGitStatus) r.Post("/sdlc", s.handleSDLC) } @@ -297,6 +298,43 @@ func (s *Server) handleGitCommitAndPush(w http.ResponseWriter, r *http.Request) writeRawJSON(w, http.StatusOK, resp) } +// GitResetToMainRequest is the request to reset the workspace to main. +type GitResetToMainRequest struct { + WorkDir string `json:"work_dir,omitempty"` // defaults to /workspace +} + +// GitResetToMainResponse is the response from resetting to main. +type GitResetToMainResponse struct { + Success bool `json:"success"` + Error string `json:"error,omitempty"` +} + +// handleGitResetToMain resets the workspace to the main branch. +func (s *Server) handleGitResetToMain(w http.ResponseWriter, r *http.Request) { + ctx := r.Context() + + var req GitResetToMainRequest + if err := api.DecodeJSON(r, &req); err != nil { + api.WriteBadRequest(w, r, "invalid request body") + return + } + + workDir := req.WorkDir + if workDir == "" { + workDir = s.gitOps.workDir + } + + err := s.gitOps.ResetToMain(ctx, workDir) + resp := GitResetToMainResponse{ + Success: err == nil, + } + if err != nil { + resp.Error = err.Error() + } + + writeRawJSON(w, http.StatusOK, resp) +} + // GitStatusResponse is the response from git status. type GitStatusResponse struct { IsRepo bool `json:"is_repo"` diff --git a/internal/worker/build_executor.go b/internal/worker/build_executor.go index 8d9f0a0..6c32647 100644 --- a/internal/worker/build_executor.go +++ b/internal/worker/build_executor.go @@ -145,6 +145,15 @@ func (b *BuildExecutor) Execute(ctx context.Context, task *domain.WorkTask) *dom "content": fmt.Sprintf("Cloned repository to %s", workDir), }) } + + // Reset to main to ensure clean workspace state. + // Worker pods may be left on a feature branch from a previous task. + if err := b.podGitOps.ResetToMain(ctx, podName, workDir); err != nil { + log.Warn("failed to reset workspace to main, continuing", + "task_id", task.ID, + logging.FieldError, err, + ) + } } // Build the agent request with pod metadata for Claude Code adapter diff --git a/internal/worker/http_sdlc_executor.go b/internal/worker/http_sdlc_executor.go index d11462f..f048334 100644 --- a/internal/worker/http_sdlc_executor.go +++ b/internal/worker/http_sdlc_executor.go @@ -75,6 +75,21 @@ func (e *HTTPSDLCTaskExecutor) Execute(ctx context.Context, task *domain.WorkTas } } + // Reset workspace to main for clean state. + // Worker pods may be left on a feature branch from a previous task. + resetResp, resetErr := e.client.GitResetToMain(ctx, e.workDir) + if resetErr != nil { + log.Warn("failed to reset workspace to main, continuing", + "task_id", task.ID, + logging.FieldError, resetErr, + ) + } else if !resetResp.Success { + log.Warn("reset to main returned failure, continuing", + "task_id", task.ID, + "error", resetResp.Error, + ) + } + // Run SDLC command sdlcResp, err := e.client.RunSDLC(ctx, spec.Command, spec.Args, e.workDir) if err != nil { diff --git a/internal/worker/pod_git_operations.go b/internal/worker/pod_git_operations.go index ef50a29..51e2824 100644 --- a/internal/worker/pod_git_operations.go +++ b/internal/worker/pod_git_operations.go @@ -198,6 +198,27 @@ func (g *PodGitOperations) dirExists(ctx context.Context, podName, path string) return cmd.Run() == nil } +// ResetToMain resets the workspace to the main branch with a clean state. +// This ensures each task starts from a known-good state regardless of +// what previous tasks may have done (e.g., switched to a feature branch). +func (g *PodGitOperations) ResetToMain(ctx context.Context, podName, workDir string) error { + resetScript := fmt.Sprintf( + "cd %s && git fetch origin && git checkout main && git reset --hard origin/main", + workDir, + ) + args := []string{ + "exec", "-n", g.namespace, podName, "--", + "sh", "-c", resetScript, + } + cmd := exec.CommandContext(ctx, "kubectl", args...) + var stderr bytes.Buffer + cmd.Stderr = &stderr + if err := cmd.Run(); err != nil { + return fmt.Errorf("reset to main: %s: %w", stderr.String(), err) + } + return nil +} + // CommitAndPush performs post-build git operations inside the pod: // 1. Configures git user/email // 2. Checks for changes (git status) @@ -276,13 +297,24 @@ func (g *PodGitOperations) CommitAndPush(ctx context.Context, podName, workDir, if g.giteaToken != "" { // Use git credential helper to inject token // This avoids putting the token in the URL which would be visible in logs - credHelper := fmt.Sprintf("!f() { echo username=token; echo password=%s; }; f", g.giteaToken) + // Use single quotes around password to prevent shell interpretation + credHelper := fmt.Sprintf("!f() { echo username=token; echo 'password=%s'; }; f", + strings.ReplaceAll(g.giteaToken, "'", "'\\''")) if err := g.runGitInPod(ctx, podName, workDir, "config", "credential.helper", credHelper); err != nil { log.Warn("failed to configure credential helper", logging.FieldError, err) // Continue anyway - push might still work if pod has other auth configured } } + // Pull before push to handle concurrent builds that may have advanced the remote. + // Use --rebase to replay our commit on top of any new remote commits. + if err := g.runGitInPod(ctx, podName, workDir, "pull", "--rebase", "origin", "HEAD"); err != nil { + log.Warn("git pull --rebase before push failed, attempting push anyway", + logging.FieldPodName, podName, + logging.FieldError, err, + ) + } + if err := g.runGitInPod(ctx, podName, workDir, "push", "origin", "HEAD"); err != nil { result.Error = fmt.Errorf("git push: %w", err) return result diff --git a/internal/worker/sdlc_executor.go b/internal/worker/sdlc_executor.go index eb0ff2e..8afa44d 100644 --- a/internal/worker/sdlc_executor.go +++ b/internal/worker/sdlc_executor.go @@ -87,7 +87,7 @@ func (e *SDLCTaskExecutor) Execute(ctx context.Context, task *domain.WorkTask) * // 1b. Reset workspace to main to ensure a known-good starting state. // Worker pods are reused across tasks and may be left on a feature branch // from a previous command (e.g., `sdlc branch create` switches branches). - if err := e.resetToMain(ctx, podName, workDir); err != nil { + if err := e.podGitOps.ResetToMain(ctx, podName, workDir); err != nil { log.Warn("failed to reset workspace to main, continuing", "task_id", task.ID, logging.FieldError, err, @@ -149,32 +149,6 @@ func (e *SDLCTaskExecutor) Execute(ctx context.Context, task *domain.WorkTask) * return result } -// resetToMain resets the workspace to the main branch with a clean state. -// This ensures each SDLC task starts from a known-good state regardless of -// what previous tasks may have done (e.g., switched to a feature branch). -func (e *SDLCTaskExecutor) resetToMain(ctx context.Context, podName, workDir string) error { - resetScript := fmt.Sprintf( - "cd %s && git fetch origin && git checkout main && git reset --hard origin/main", - workDir, - ) - - args := []string{ - "exec", "-n", e.namespace, podName, "--", - "sh", "-c", resetScript, - } - - cmd := exec.CommandContext(ctx, "kubectl", args...) - var stdout, stderr bytes.Buffer - cmd.Stdout = &stdout - cmd.Stderr = &stderr - - if err := cmd.Run(); err != nil { - return fmt.Errorf("reset to main: %s: %w", stderr.String(), err) - } - - return nil -} - // ensureSDLCInit checks if .sdlc/ exists and runs `sdlc init` if it doesn't. // This enables SDLC operations on skeleton projects that don't have .sdlc/ pre-initialized. func (e *SDLCTaskExecutor) ensureSDLCInit(ctx context.Context, podName, workDir string) error {