fix(git): harden git flow for concurrent SDLC stress test failures
Some checks failed
ci/woodpecker/push/woodpecker Pipeline failed
Some checks failed
ci/woodpecker/push/woodpecker Pipeline failed
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 <noreply@anthropic.com>
This commit is contained in:
parent
8715411727
commit
b6e778d5ab
@ -2,7 +2,6 @@ package main
|
|||||||
|
|
||||||
import (
|
import (
|
||||||
"fmt"
|
"fmt"
|
||||||
"os"
|
|
||||||
"os/exec"
|
"os/exec"
|
||||||
"strings"
|
"strings"
|
||||||
"time"
|
"time"
|
||||||
@ -55,11 +54,19 @@ var branchCreateCmd = &cobra.Command{
|
|||||||
|
|
||||||
pushCmd := exec.Command("git", "push", "origin", "HEAD")
|
pushCmd := exec.Command("git", "push", "origin", "HEAD")
|
||||||
pushCmd.Dir = root
|
pushCmd.Dir = root
|
||||||
if out, err := pushCmd.CombinedOutput(); err != nil {
|
if _, err := pushCmd.CombinedOutput(); err != nil {
|
||||||
// Push failure is non-fatal. In executor contexts, origin exists
|
// Retry once — transient Gitea failures are common
|
||||||
// (from git clone) and this succeeds. In local/test contexts without
|
time.Sleep(2 * time.Second)
|
||||||
// a remote, the commit is still persisted locally on main.
|
retryCmd := exec.Command("git", "push", "origin", "HEAD")
|
||||||
fmt.Fprintf(os.Stderr, "warning: failed to push .sdlc/ state: %s\n", strings.TrimSpace(string(out)))
|
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
|
// Create the git branch
|
||||||
|
|||||||
@ -312,6 +312,56 @@ func (c *Client) GitCommitAndPush(ctx context.Context, message string, push bool
|
|||||||
return &result, nil
|
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.
|
// GitStatusResponse is the response from git status.
|
||||||
type GitStatusResponse struct {
|
type GitStatusResponse struct {
|
||||||
IsRepo bool `json:"is_repo"`
|
IsRepo bool `json:"is_repo"`
|
||||||
|
|||||||
@ -148,7 +148,7 @@ func (g *GitOperations) CloneRepo(ctx context.Context, workDir, cloneURL string)
|
|||||||
if err := cmd.Run(); err != nil {
|
if err := cmd.Run(); err != nil {
|
||||||
errMsg := g.redactToken(stderr.String())
|
errMsg := g.redactToken(stderr.String())
|
||||||
stdoutMsg := g.redactToken(stdout.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",
|
g.logger.Error("git clone failed",
|
||||||
"error", err,
|
"error", err,
|
||||||
"stderr", errMsg,
|
"stderr", errMsg,
|
||||||
@ -232,12 +232,20 @@ func (g *GitOperations) CommitAndPush(ctx context.Context, workDir, message stri
|
|||||||
if push {
|
if push {
|
||||||
// Configure credential helper
|
// Configure credential helper
|
||||||
if g.giteaToken != "" {
|
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 {
|
if err := g.runGit(ctx, workDir, "config", "credential.helper", credHelper); err != nil {
|
||||||
g.logger.Debug("credential helper config failed, continuing with push", "error", err)
|
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 {
|
if err := g.runGit(ctx, workDir, "push", "origin", "HEAD"); err != nil {
|
||||||
result.Error = fmt.Errorf("git push: %w", err)
|
result.Error = fmt.Errorf("git push: %w", err)
|
||||||
return result
|
return result
|
||||||
@ -248,6 +256,20 @@ func (g *GitOperations) CommitAndPush(ctx context.Context, workDir, message stri
|
|||||||
return result
|
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.
|
// GitStatusResult contains git status information.
|
||||||
type GitStatusResult struct {
|
type GitStatusResult struct {
|
||||||
IsRepo bool `json:"is_repo"`
|
IsRepo bool `json:"is_repo"`
|
||||||
|
|||||||
@ -55,6 +55,7 @@ func (s *Server) Mount(r chi.Router) {
|
|||||||
r.Post("/execute/stream", s.handleExecuteStream)
|
r.Post("/execute/stream", s.handleExecuteStream)
|
||||||
r.Post("/git/clone", s.handleGitClone)
|
r.Post("/git/clone", s.handleGitClone)
|
||||||
r.Post("/git/commit-and-push", s.handleGitCommitAndPush)
|
r.Post("/git/commit-and-push", s.handleGitCommitAndPush)
|
||||||
|
r.Post("/git/reset-to-main", s.handleGitResetToMain)
|
||||||
r.Get("/git/status", s.handleGitStatus)
|
r.Get("/git/status", s.handleGitStatus)
|
||||||
r.Post("/sdlc", s.handleSDLC)
|
r.Post("/sdlc", s.handleSDLC)
|
||||||
}
|
}
|
||||||
@ -297,6 +298,43 @@ func (s *Server) handleGitCommitAndPush(w http.ResponseWriter, r *http.Request)
|
|||||||
writeRawJSON(w, http.StatusOK, resp)
|
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.
|
// GitStatusResponse is the response from git status.
|
||||||
type GitStatusResponse struct {
|
type GitStatusResponse struct {
|
||||||
IsRepo bool `json:"is_repo"`
|
IsRepo bool `json:"is_repo"`
|
||||||
|
|||||||
@ -145,6 +145,15 @@ func (b *BuildExecutor) Execute(ctx context.Context, task *domain.WorkTask) *dom
|
|||||||
"content": fmt.Sprintf("Cloned repository to %s", workDir),
|
"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
|
// Build the agent request with pod metadata for Claude Code adapter
|
||||||
|
|||||||
@ -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
|
// Run SDLC command
|
||||||
sdlcResp, err := e.client.RunSDLC(ctx, spec.Command, spec.Args, e.workDir)
|
sdlcResp, err := e.client.RunSDLC(ctx, spec.Command, spec.Args, e.workDir)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
|
|||||||
@ -198,6 +198,27 @@ func (g *PodGitOperations) dirExists(ctx context.Context, podName, path string)
|
|||||||
return cmd.Run() == nil
|
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:
|
// CommitAndPush performs post-build git operations inside the pod:
|
||||||
// 1. Configures git user/email
|
// 1. Configures git user/email
|
||||||
// 2. Checks for changes (git status)
|
// 2. Checks for changes (git status)
|
||||||
@ -276,13 +297,24 @@ func (g *PodGitOperations) CommitAndPush(ctx context.Context, podName, workDir,
|
|||||||
if g.giteaToken != "" {
|
if g.giteaToken != "" {
|
||||||
// Use git credential helper to inject token
|
// Use git credential helper to inject token
|
||||||
// This avoids putting the token in the URL which would be visible in logs
|
// 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 {
|
if err := g.runGitInPod(ctx, podName, workDir, "config", "credential.helper", credHelper); err != nil {
|
||||||
log.Warn("failed to configure credential helper", logging.FieldError, err)
|
log.Warn("failed to configure credential helper", logging.FieldError, err)
|
||||||
// Continue anyway - push might still work if pod has other auth configured
|
// 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 {
|
if err := g.runGitInPod(ctx, podName, workDir, "push", "origin", "HEAD"); err != nil {
|
||||||
result.Error = fmt.Errorf("git push: %w", err)
|
result.Error = fmt.Errorf("git push: %w", err)
|
||||||
return result
|
return result
|
||||||
|
|||||||
@ -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.
|
// 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
|
// 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).
|
// 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",
|
log.Warn("failed to reset workspace to main, continuing",
|
||||||
"task_id", task.ID,
|
"task_id", task.ID,
|
||||||
logging.FieldError, err,
|
logging.FieldError, err,
|
||||||
@ -149,32 +149,6 @@ func (e *SDLCTaskExecutor) Execute(ctx context.Context, task *domain.WorkTask) *
|
|||||||
return result
|
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.
|
// 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.
|
// 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 {
|
func (e *SDLCTaskExecutor) ensureSDLCInit(ctx context.Context, podName, workDir string) error {
|
||||||
|
|||||||
Loading…
Reference in New Issue
Block a user