diff --git a/apps/backend/internal/agent/handlers/git_handlers.go b/apps/backend/internal/agent/handlers/git_handlers.go index 512ea8e359..0030e4daa5 100644 --- a/apps/backend/internal/agent/handlers/git_handlers.go +++ b/apps/backend/internal/agent/handlers/git_handlers.go @@ -184,6 +184,10 @@ type GitPushRequest struct { Force bool `json:"force"` SetUpstream bool `json:"set_upstream"` Repo string `json:"repo,omitempty"` + // Remote and ExpectedBranch are optional and forwarded uninterpreted; the + // workspace git operator owns every destination decision. + Remote string `json:"remote,omitempty"` + ExpectedBranch string `json:"expected_branch,omitempty"` } // GitContributionRequest carries the provider-head lease for a managed @@ -331,13 +335,18 @@ func (h *GitHandlers) wsPush(ctx context.Context, msg *ws.Message) (*ws.Message, return nil, fmt.Errorf("session_id is required") } - client, releaseClient, err := h.getAgentCtlClient(ctx, req.SessionID) + agentClient, releaseClient, err := h.getAgentCtlClient(ctx, req.SessionID) defer releaseClient() if err != nil { return nil, err } - result, err := client.GitPush(ctx, req.Force, req.SetUpstream, req.Repo) + result, err := agentClient.GitPush(ctx, req.Repo, client.PushOptions{ + Force: req.Force, + SetUpstream: req.SetUpstream, + Remote: req.Remote, + ExpectedBranch: req.ExpectedBranch, + }) if err != nil { return nil, fmt.Errorf("push failed: %w", err) } diff --git a/apps/backend/internal/agent/handlers/git_handlers_test.go b/apps/backend/internal/agent/handlers/git_handlers_test.go index 1fce949b58..d3249935bb 100644 --- a/apps/backend/internal/agent/handlers/git_handlers_test.go +++ b/apps/backend/internal/agent/handlers/git_handlers_test.go @@ -121,6 +121,7 @@ func TestGitMutationHandlersForwardRequestsAndReturnResults(t *testing.T) { }{ {name: "pull", path: "/api/v1/git/pull", request: GitPullRequest{SessionID: "s", Rebase: true, Repo: "repo"}, invoke: (*GitHandlers).wsPull, assertBody: expectGitBody("rebase", true)}, {name: "push", path: "/api/v1/git/push", request: GitPushRequest{SessionID: "s", Force: true, SetUpstream: true, Repo: "repo"}, invoke: (*GitHandlers).wsPush, assertBody: expectGitBody("force", true)}, + {name: "push with explicit target", path: "/api/v1/git/push", request: GitPushRequest{SessionID: "s", Repo: "repo", Remote: "backup", ExpectedBranch: "feature/work"}, invoke: (*GitHandlers).wsPush, assertBody: expectGitBody("remote", "backup")}, {name: "replace contribution", path: "/api/v1/git/contribution/replace", request: GitContributionRequest{SessionID: "s", ExpectedRemoteHead: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", Repo: "repo"}, invoke: (*GitHandlers).wsReplaceContribution, assertBody: expectGitBody("expected_remote_head", "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa")}, {name: "use contribution", path: "/api/v1/git/contribution/use", request: GitContributionRequest{SessionID: "s", ExpectedRemoteHead: "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", Repo: "repo"}, invoke: (*GitHandlers).wsUseContribution, assertBody: expectGitBody("expected_remote_head", "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb")}, {name: "commit", path: "/api/v1/git/commit", request: GitCommitRequest{SessionID: "s", Message: "message", StageAll: true, Repo: "repo"}, invoke: (*GitHandlers).wsCommit, assertBody: expectGitBody("message", "message")}, diff --git a/apps/backend/internal/agent/handlers/git_push_options_test.go b/apps/backend/internal/agent/handlers/git_push_options_test.go new file mode 100644 index 0000000000..dd67bb4c83 --- /dev/null +++ b/apps/backend/internal/agent/handlers/git_push_options_test.go @@ -0,0 +1,52 @@ +package handlers + +import ( + "context" + "encoding/json" + "net/http" + "strings" + "testing" + + ws "github.com/kandev/kandev/pkg/websocket" +) + +// TestWsPushForwardsPushOptions proves the ws payload's two optional inputs +// reach the agentctl request body together, rather than one being dropped. +func TestWsPushForwardsPushOptions(t *testing.T) { + var body map[string]any + h, server := gitHandlerServer(t, func(w http.ResponseWriter, r *http.Request) { + if err := json.NewDecoder(r.Body).Decode(&body); err != nil { + t.Errorf("decode request: %v", err) + } + _, _ = w.Write([]byte(`{"success":true,"operation":"push","output":"done","pushed_remote":"backup","pushed_branch":"feature/work"}`)) + }) + defer server.Close() + + msg, err := ws.NewRequest("id", "action", GitPushRequest{ + SessionID: "s", + Repo: "repo", + Remote: "backup", + ExpectedBranch: "feature/work", + }) + if err != nil { + t.Fatal(err) + } + response, err := h.wsPush(context.Background(), msg) + if err != nil { + t.Fatalf("wsPush: %v", err) + } + if body["remote"] != "backup" { + t.Errorf("body[remote] = %#v, want backup", body["remote"]) + } + if body["expected_branch"] != "feature/work" { + t.Errorf("body[expected_branch] = %#v, want feature/work", body["expected_branch"]) + } + // The destination fields must survive the round trip back to the caller. + payload := string(response.Payload) + if !strings.Contains(payload, `"pushed_remote":"backup"`) { + t.Errorf("response payload lost pushed_remote: %s", payload) + } + if !strings.Contains(payload, `"pushed_branch":"feature/work"`) { + t.Errorf("response payload lost pushed_branch: %s", payload) + } +} diff --git a/apps/backend/internal/agent/runtime/agentctl/git.go b/apps/backend/internal/agent/runtime/agentctl/git.go index f583665303..9835c76531 100644 --- a/apps/backend/internal/agent/runtime/agentctl/git.go +++ b/apps/backend/internal/agent/runtime/agentctl/git.go @@ -22,6 +22,26 @@ type GitOperationResult struct { PreflightReason string `json:"preflight_reason,omitempty"` ConflictFiles []string `json:"conflict_files,omitempty"` RecoveryBranch string `json:"recovery_branch,omitempty"` + // Destination a push or preflight validated. A push populates these only + // when the request carried an explicit push target. + PushedRemote string `json:"pushed_remote,omitempty"` + PushedBranch string `json:"pushed_branch,omitempty"` + // Branches accompanying a mismatch refusal. CurrentBranch is empty for a + // detached HEAD. + ExpectedBranch string `json:"expected_branch,omitempty"` + CurrentBranch string `json:"current_branch,omitempty"` + // BaselinePublished marks a mismatch refusal after empty-remote first + // publication already published the baseline in this request. + BaselinePublished bool `json:"baseline_published,omitempty"` +} + +// PushOptions carries the optional inputs of a push or push-preflight call. +// The zero value reproduces the behavior of a request that names neither. +type PushOptions struct { + Force bool + SetUpstream bool + Remote string + ExpectedBranch string } // PRCreateResult represents the result of a PR creation operation. @@ -51,28 +71,40 @@ func (c *Client) GitPull(ctx context.Context, rebase bool, repo string) (*GitOpe } // GitPush performs a git push operation on the worktree. -// If force is true, uses --force-with-lease. -// If setUpstream is true, uses --set-upstream. +// If opts.Force is true, uses --force-with-lease. +// If opts.SetUpstream is true, uses --set-upstream. +// opts.Remote and opts.ExpectedBranch are optional and passed through. // repo is the multi-repo subpath (e.g. "kandev"); empty for single-repo workspaces. -func (c *Client) GitPush(ctx context.Context, force, setUpstream bool, repo string) (*GitOperationResult, error) { +func (c *Client) GitPush(ctx context.Context, repo string, opts PushOptions) (*GitOperationResult, error) { payload := struct { - Force bool `json:"force"` - SetUpstream bool `json:"set_upstream"` - Repo string `json:"repo,omitempty"` + Force bool `json:"force"` + SetUpstream bool `json:"set_upstream"` + Repo string `json:"repo,omitempty"` + Remote string `json:"remote,omitempty"` + ExpectedBranch string `json:"expected_branch,omitempty"` }{ - Force: force, - SetUpstream: setUpstream, - Repo: repo, + Force: opts.Force, + SetUpstream: opts.SetUpstream, + Repo: repo, + Remote: opts.Remote, + ExpectedBranch: opts.ExpectedBranch, } return c.gitOperation(ctx, "/api/v1/git/push", payload) } -// GitPushPreflight validates the configured contribution push target without -// mutating the remote. repo is the multi-repo workspace subpath. -func (c *Client) GitPushPreflight(ctx context.Context, repo string) (*GitOperationResult, error) { +// GitPushPreflight validates the push destination without mutating the remote. +// repo is the multi-repo workspace subpath; opts carries the optional explicit +// push target and expected branch. +func (c *Client) GitPushPreflight(ctx context.Context, repo string, opts PushOptions) (*GitOperationResult, error) { payload := struct { - Repo string `json:"repo,omitempty"` - }{Repo: repo} + Repo string `json:"repo,omitempty"` + Remote string `json:"remote,omitempty"` + ExpectedBranch string `json:"expected_branch,omitempty"` + }{ + Repo: repo, + Remote: opts.Remote, + ExpectedBranch: opts.ExpectedBranch, + } return c.gitOperation(ctx, "/api/v1/git/push-preflight", payload) } diff --git a/apps/backend/internal/agent/runtime/agentctl/git_test.go b/apps/backend/internal/agent/runtime/agentctl/git_test.go index c2c954a833..bba4febe2e 100644 --- a/apps/backend/internal/agent/runtime/agentctl/git_test.go +++ b/apps/backend/internal/agent/runtime/agentctl/git_test.go @@ -11,6 +11,27 @@ import ( // gitOperationOK is the canonical success body every /api/v1/git/* POST returns. const gitOperationOK = `{"success":true,"operation":"pull","output":"Already up to date."}` +func TestGitPushSendsPushOptions(t *testing.T) { + srv, got := captureServer(t, jsonResponder(http.StatusOK, gitOperationOK)) + _, err := newHTTPOnlyClient(srv.URL).GitPush(context.Background(), "svc", PushOptions{ + Remote: "backup", ExpectedBranch: "feature/work", + }) + if err != nil { + t.Fatalf("GitPush() error = %v", err) + } + var body map[string]any + if err := json.Unmarshal(got.Body, &body); err != nil { + t.Fatalf("decode request body: %v", err) + } + for key, want := range map[string]any{ + "repo": "svc", "remote": "backup", "expected_branch": "feature/work", + } { + if gotValue := body[key]; gotValue != want { + t.Errorf("body[%q] = %#v, want %#v", key, gotValue, want) + } + } +} + // TestGitOperations_PostExpectedPathAndPayload pins the endpoint and JSON body // of every thin wrapper over gitOperation. A wrapper that posts to the wrong // path or drops a field is the whole failure mode for this layer. @@ -36,7 +57,7 @@ func TestGitOperations_PostExpectedPathAndPayload(t *testing.T) { { name: "push force with upstream", call: func(c *Client) (*GitOperationResult, error) { - return c.GitPush(context.Background(), true, true, "svc") + return c.GitPush(context.Background(), "svc", PushOptions{Force: true, SetUpstream: true}) }, wantPath: "/api/v1/git/push", wantBody: map[string]any{"force": true, "set_upstream": true, "repo": "svc"}, @@ -44,17 +65,44 @@ func TestGitOperations_PostExpectedPathAndPayload(t *testing.T) { { name: "push plain", call: func(c *Client) (*GitOperationResult, error) { - return c.GitPush(context.Background(), false, false, "") + return c.GitPush(context.Background(), "", PushOptions{}) }, wantPath: "/api/v1/git/push", wantBody: map[string]any{"force": false, "set_upstream": false}, }, { - name: "push preflight", - call: func(c *Client) (*GitOperationResult, error) { return c.GitPushPreflight(context.Background(), "svc") }, + name: "push preflight", + call: func(c *Client) (*GitOperationResult, error) { + return c.GitPushPreflight(context.Background(), "svc", PushOptions{}) + }, wantPath: "/api/v1/git/push-preflight", wantBody: map[string]any{"repo": "svc"}, }, + { + name: "push with explicit target and expected branch", + call: func(c *Client) (*GitOperationResult, error) { + return c.GitPush(context.Background(), "svc", PushOptions{ + Remote: "backup", ExpectedBranch: "feature/work", + }) + }, + wantPath: "/api/v1/git/push", + wantBody: map[string]any{ + "force": false, "set_upstream": false, "repo": "svc", + "remote": "backup", "expected_branch": "feature/work", + }, + }, + { + name: "push preflight with explicit target and expected branch", + call: func(c *Client) (*GitOperationResult, error) { + return c.GitPushPreflight(context.Background(), "svc", PushOptions{ + Remote: "backup", ExpectedBranch: "feature/work", + }) + }, + wantPath: "/api/v1/git/push-preflight", + wantBody: map[string]any{ + "repo": "svc", "remote": "backup", "expected_branch": "feature/work", + }, + }, { name: "replace remote contribution", call: func(c *Client) (*GitOperationResult, error) { @@ -331,7 +379,7 @@ func TestGitOperation_HonoursContextCancellation(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) cancel() - _, err := newHTTPOnlyClient(srv.URL).GitPush(ctx, false, false, "") + _, err := newHTTPOnlyClient(srv.URL).GitPush(ctx, "", PushOptions{}) if err == nil || !strings.Contains(err.Error(), "context canceled") { t.Fatalf("error = %v, want context canceled", err) } diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_startup.go b/apps/backend/internal/agent/runtime/lifecycle/manager_startup.go index 31da4efc06..046cdaa15e 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_startup.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_startup.go @@ -242,7 +242,7 @@ func (m *Manager) preflightRemoteContributionPushes(ctx context.Context, executi } sort.Strings(keys) for _, key := range keys { - result, err := client.GitPushPreflight(ctx, key) + result, err := client.GitPushPreflight(ctx, key, agentctl.PushOptions{}) if err != nil { return &BootstrapFailure{ Operation: bootstrapOperation(execution), diff --git a/apps/backend/internal/agentctl/server/api/git.go b/apps/backend/internal/agentctl/server/api/git.go index 6c24b032a0..3a499274bf 100644 --- a/apps/backend/internal/agentctl/server/api/git.go +++ b/apps/backend/internal/agentctl/server/api/git.go @@ -53,15 +53,24 @@ type GitPullRequest struct { } // GitPushRequest for POST /api/v1/git/push +// +// Remote and ExpectedBranch are optional. Remote is a configured remote name or +// a remote URL that must resolve to one; ExpectedBranch is the branch the +// caller believes it is publishing. Both are passed through untouched: this +// surface performs no destination logic of its own. type GitPushRequest struct { - Force bool `json:"force"` - SetUpstream bool `json:"set_upstream"` - Repo string `json:"repo,omitempty"` + Force bool `json:"force"` + SetUpstream bool `json:"set_upstream"` + Repo string `json:"repo,omitempty"` + Remote string `json:"remote,omitempty"` + ExpectedBranch string `json:"expected_branch,omitempty"` } // GitPushPreflightRequest for POST /api/v1/git/push-preflight. type GitPushPreflightRequest struct { - Repo string `json:"repo,omitempty"` + Repo string `json:"repo,omitempty"` + Remote string `json:"remote,omitempty"` + ExpectedBranch string `json:"expected_branch,omitempty"` } // GitContributionRequest is shared by the managed contribution replacement @@ -220,7 +229,12 @@ func (s *Server) handleGitPush(c *gin.Context) { if gitOp == nil { return } - result, err := gitOp.Push(c.Request.Context(), req.Force, req.SetUpstream) + result, err := gitOp.Push(c.Request.Context(), process.PushOptions{ + Force: req.Force, + SetUpstream: req.SetUpstream, + Remote: req.Remote, + ExpectedBranch: req.ExpectedBranch, + }) if err != nil { s.handleGitError(c, "push", err) return @@ -241,7 +255,10 @@ func (s *Server) handleGitPushPreflight(c *gin.Context) { if gitOp == nil { return } - result, err := gitOp.PushPreflight(c.Request.Context()) + result, err := gitOp.PushPreflight(c.Request.Context(), process.PushOptions{ + Remote: req.Remote, + ExpectedBranch: req.ExpectedBranch, + }) if err != nil { s.handleGitError(c, "push preflight", err) return diff --git a/apps/backend/internal/agentctl/server/api/git_push_target_test.go b/apps/backend/internal/agentctl/server/api/git_push_target_test.go new file mode 100644 index 0000000000..71da91381c --- /dev/null +++ b/apps/backend/internal/agentctl/server/api/git_push_target_test.go @@ -0,0 +1,139 @@ +package api + +import ( + "net/http" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/kandev/kandev/internal/agentctl/server/config" + "github.com/kandev/kandev/internal/agentctl/server/process" + "github.com/kandev/kandev/internal/common/logger" +) + +// TestHandleGitPushBindsRemoteAndExpectedBranch proves the HTTP surface carries +// both optional inputs into the operator rather than dropping them. +func TestHandleGitPushBindsRemoteAndExpectedBranch(t *testing.T) { + fixture := newGitAPIFixture(t) + backup := filepath.Join(t.TempDir(), "backup.git") + runGitAPI(t, fixture.repo, "init", "--bare", "--initial-branch=main", backup) + runGitAPI(t, fixture.repo, "remote", "add", "backup", backup) + + rec := postGitAPI(t, fixture.server, "/api/v1/git/push", GitPushRequest{ + Remote: "backup", + ExpectedBranch: "feature/work", + }) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d (body %s)", rec.Code, rec.Body.String()) + } + result := decodeGitOperationResult(t, rec) + if !result.Success { + t.Fatalf("push failed: %+v", result) + } + if result.PushedRemote != "backup" || result.PushedBranch != "feature/work" { + t.Errorf("destination = (%q, %q), want (backup, feature/work)", result.PushedRemote, result.PushedBranch) + } + head := fixture.head(t) + if got := strings.TrimSpace(runGitAPI(t, fixture.repo, "--git-dir="+backup, "rev-parse", "refs/heads/feature/work")); got != head { + t.Errorf("backup feature/work = %q, want %q", got, head) + } + // origin must be untouched by a push that named another destination. + runGitAPIExpectFailure(t, fixture.repo, "--git-dir="+fixture.bare, "rev-parse", "--verify", "refs/heads/feature/work") +} + +// TestHandleGitPushRejectsMismatchedExpectedBranch proves the expected branch is +// enforced through the HTTP surface, not merely accepted. +func TestHandleGitPushRejectsMismatchedExpectedBranch(t *testing.T) { + fixture := newGitAPIFixture(t) + + rec := postGitAPI(t, fixture.server, "/api/v1/git/push", GitPushRequest{ExpectedBranch: "feature/other"}) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d (body %s)", rec.Code, rec.Body.String()) + } + result := decodeGitOperationResult(t, rec) + if result.Success { + t.Fatalf("push succeeded on a mismatch: %+v", result) + } + if result.ErrorCode != "push_branch_mismatch" { + t.Errorf("error_code = %q, want push_branch_mismatch", result.ErrorCode) + } + if result.ExpectedBranch != "feature/other" || result.CurrentBranch != "feature/work" { + t.Errorf("branches = (%q, %q), want (feature/other, feature/work)", result.ExpectedBranch, result.CurrentBranch) + } +} + +// TestHandleGitPushPreflightBindsRemoteAndExpectedBranch covers the preflight +// endpoint's binding and its reporting of the destination it validated. +func TestHandleGitPushPreflightBindsRemoteAndExpectedBranch(t *testing.T) { + fixture := newGitAPIFixture(t) + backup := filepath.Join(t.TempDir(), "backup.git") + runGitAPI(t, fixture.repo, "init", "--bare", "--initial-branch=main", backup) + runGitAPI(t, fixture.repo, "remote", "add", "backup", backup) + + rec := postGitAPI(t, fixture.server, "/api/v1/git/push-preflight", GitPushPreflightRequest{ + Remote: "backup", + ExpectedBranch: "feature/work", + }) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d (body %s)", rec.Code, rec.Body.String()) + } + result := decodeGitOperationResult(t, rec) + if !result.Success { + t.Fatalf("preflight failed: %+v", result) + } + if result.PushedRemote != "backup" || result.PushedBranch != "feature/work" { + t.Errorf("validated (%q, %q), want (backup, feature/work)", result.PushedRemote, result.PushedBranch) + } + runGitAPIExpectFailure(t, fixture.repo, "--git-dir="+backup, "rev-parse", "--verify", "refs/heads/feature/work") +} + +// TestGitPushRequestScopesTargetToSelectedRepo proves both optional inputs act +// on the repository the request selects and leave every sibling untouched. +func TestGitPushRequestScopesTargetToSelectedRepo(t *testing.T) { + root := t.TempDir() + backups := map[string]string{} + for _, name := range []string{"alpha", "beta"} { + repo := filepath.Join(root, name) + if err := os.Mkdir(repo, 0o755); err != nil { + t.Fatalf("mkdir %s: %v", name, err) + } + backup := filepath.Join(root, name+"-backup.git") + runGitAPI(t, root, "init", "--bare", "--initial-branch=main", backup) + backups[name] = backup + + runGitAPI(t, repo, "init", "--initial-branch=main") + runGitAPI(t, repo, "config", "user.email", "test@test.com") + runGitAPI(t, repo, "config", "user.name", "Test User") + writeFileAPI(t, repo, "README.md", name+"\n") + runGitAPI(t, repo, "add", ".") + runGitAPI(t, repo, "commit", "-m", "initial") + runGitAPI(t, repo, "remote", "add", "backup", backup) + runGitAPI(t, repo, "checkout", "-b", "feature/work") + writeFileAPI(t, repo, "one.txt", "one\n") + runGitAPI(t, repo, "add", ".") + runGitAPI(t, repo, "commit", "-m", "work") + } + + log, _ := logger.NewLogger(logger.LoggingConfig{Level: "error"}) + cfg := &config.InstanceConfig{WorkDir: root} + server := NewServer(cfg, process.NewManager(cfg, log), nil, nil, log) + + rec := postGitAPI(t, server, "/api/v1/git/push", GitPushRequest{ + Repo: "alpha", + Remote: "backup", + ExpectedBranch: "feature/work", + }) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d (body %s)", rec.Code, rec.Body.String()) + } + if result := decodeGitOperationResult(t, rec); !result.Success { + t.Fatalf("push failed: %+v", result) + } + + alphaHead := strings.TrimSpace(runGitAPI(t, filepath.Join(root, "alpha"), "rev-parse", "HEAD")) + if got := strings.TrimSpace(runGitAPI(t, root, "--git-dir="+backups["alpha"], "rev-parse", "refs/heads/feature/work")); got != alphaHead { + t.Errorf("alpha backup = %q, want %q", got, alphaHead) + } + runGitAPIExpectFailure(t, root, "--git-dir="+backups["beta"], "rev-parse", "--verify", "refs/heads/feature/work") +} diff --git a/apps/backend/internal/agentctl/server/process/git.go b/apps/backend/internal/agentctl/server/process/git.go index 040d23d576..232f3b0fca 100644 --- a/apps/backend/internal/agentctl/server/process/git.go +++ b/apps/backend/internal/agentctl/server/process/git.go @@ -28,6 +28,12 @@ var ErrInvalidBranchName = errors.New("invalid branch name") const preflightReasonHistoryUpdateRequired = "history_update_required" +var contributionPreflightEnvironment = map[string]string{ + "GIT_TERMINAL_PROMPT": "0", + "LANG": "C", + "LC_ALL": "C", +} + // GitOperationResult represents the result of a git operation. type GitOperationResult struct { Success bool `json:"success"` @@ -38,6 +44,18 @@ type GitOperationResult struct { PreflightReason string `json:"preflight_reason,omitempty"` ConflictFiles []string `json:"conflict_files,omitempty"` RecoveryBranch string `json:"recovery_branch,omitempty"` + // PushedRemote and PushedBranch name the destination a push or preflight + // validated. A push reports them only when the request carried an explicit + // push target, so a request that named none keeps its existing shape. + PushedRemote string `json:"pushed_remote,omitempty"` + PushedBranch string `json:"pushed_branch,omitempty"` + // ExpectedBranch and CurrentBranch accompany a branch-mismatch refusal. + // CurrentBranch is empty for a detached HEAD. + ExpectedBranch string `json:"expected_branch,omitempty"` + CurrentBranch string `json:"current_branch,omitempty"` + // BaselinePublished marks a mismatch refused after empty-remote first + // publication already published the baseline in this request. + BaselinePublished bool `json:"baseline_published,omitempty"` } // GitOperator executes git operations in a workspace directory. @@ -458,7 +476,8 @@ func (g *GitOperator) Pull(ctx context.Context, rebase bool) (*GitOperationResul } // Push performs a git push operation. -func (g *GitOperator) Push(ctx context.Context, force bool, setUpstream bool) (*GitOperationResult, error) { +func (g *GitOperator) Push(ctx context.Context, opts PushOptions) (*GitOperationResult, error) { + opts = opts.normalized() if !g.tryLock("push") { return nil, ErrOperationInProgress } @@ -467,36 +486,21 @@ func (g *GitOperator) Push(ctx context.Context, force bool, setUpstream bool) (* result := &GitOperationResult{ Operation: "push", } - if g.remoteContributionErr != nil { - result.Error = g.remoteContributionErr.Error() - return result, nil - } - if g.contributionDestinationErr != nil { - result.Error = g.contributionDestinationErr.Error() - return result, nil - } - if g.contributionDestination != nil { - if err := g.validateContributionDestinationRemote(ctx); err != nil { - result.Error = err.Error() - return result, nil - } - } - if (g.remoteContribution != nil || g.contributionDestination != nil) && force { - result.Error = "force push is not allowed for a remote contribution" - return result, nil - } - if err := g.validateContributionRemote(ctx); err != nil { - result.Error = err.Error() + if refusal := g.validateContributionState(ctx, opts.Force); refusal != nil { + refusal.apply(result) return result, nil } - branch, err := g.getCurrentBranch(ctx) - if err != nil { - result.Error = err.Error() + // Every refusal below is raised before any remote is contacted, so a + // refused request is a no-op. + plan, refusal := g.resolvePushPlan(ctx, opts, false) + if refusal != nil { + refusal.apply(result) return result, nil } + basePublication := emptyRemotePublication{} - if g.remoteContribution == nil && g.contributionDestination == nil { + if plan.baselineEligible { basePublication = g.prepareEmptyRemotePublication(ctx, "") if basePublication.err != nil { result.Error = basePublication.err.Error() @@ -506,29 +510,24 @@ func (g *GitOperator) Push(ctx context.Context, force bool, setUpstream bool) (* } } - args := []string{"push"} - shouldSetUpstream := setUpstream || g.getUpstreamRef(ctx) == "" - remote := "origin" - refspec := branch - if g.contributionDestination != nil { - remote = g.contributionDestination.ContributionRemoteName() - refspec = branch - shouldSetUpstream = setUpstream - } else if g.remoteContribution != nil { - remote = g.remoteContribution.ContributionRemoteName() - refspec = "HEAD:refs/heads/" + g.remoteContribution.HeadBranch - shouldSetUpstream = setUpstream + // Every remaining read happens before the second verification, so that the + // branch read is the last git command before the push. + shouldSetUpstream := g.resolveSetUpstream(ctx, opts, plan) + if refusal := g.verifyExpectedBranch(ctx, opts.ExpectedBranch, basePublication.published); refusal != nil { + refusal.apply(result) + result.Output = basePublication.output + return result, nil } + + args := []string{"push"} if shouldSetUpstream { args = append(args, "--set-upstream") } - - if force { + if opts.Force { // Use --force-with-lease for safer force push args = append(args, "--force-with-lease") } - - args = append(args, remote, refspec) + args = append(args, plan.remote, plan.refspec) output, err := g.runGitCommand(ctx, args...) result.Output = output @@ -544,107 +543,148 @@ func (g *GitOperator) Push(ctx context.Context, force bool, setUpstream bool) (* result.Output = combineGitOutputs(basePublication.output, output) result.Success = true + plan.reportDestination(result) g.logger.Info("push completed", - zap.String("branch", branch), - zap.String("remote", remote), - zap.Bool("force", force), - zap.Bool("set_upstream", shouldSetUpstream)) + zap.String("branch", plan.branch), + zap.String("remote", plan.remote), + zap.Bool("force", opts.Force), + zap.Bool("set_upstream", shouldSetUpstream), + zap.Bool("expected_branch_supplied", opts.ExpectedBranch != ""), + zap.Bool("explicit_target", plan.explicit)) return result, nil } -// PushPreflight verifies that the configured contribution remote and final -// head-branch refspec are writable without mutating the remote or local refs. -func (g *GitOperator) PushPreflight(ctx context.Context) (*GitOperationResult, error) { - if !g.tryLock("push-preflight") { - return nil, ErrOperationInProgress - } - defer g.unlock() - result := &GitOperationResult{Operation: "push_preflight"} +// validateContributionState runs the existing contribution binding checks, +// which precede every refusal this capability adds. +func (g *GitOperator) validateContributionState(ctx context.Context, force bool) *pushRefusal { if g.remoteContributionErr != nil { - setPushPreflightError(result, g.remoteContributionErr) - return result, nil + return pushRefusalFromError(g.remoteContributionErr) } if g.contributionDestinationErr != nil { - setPushPreflightError(result, g.contributionDestinationErr) - return result, nil + return pushRefusalFromError(g.contributionDestinationErr) } if g.contributionDestination != nil { if err := g.validateContributionDestinationRemote(ctx); err != nil { - setPushPreflightError(result, err) - return result, nil + return pushRefusalFromError(err) } - branch, err := g.getCurrentBranch(ctx) - if err != nil { - setPushPreflightError(result, err) - return result, nil - } - output, err := g.runGitCommand(ctx, "push", "--dry-run", g.contributionDestination.ContributionRemoteName(), branch) - result.Output = output - if err != nil { - setPushPreflightError(result, err) - return result, nil - } - result.Success = true - g.logger.Info("contribution destination push preflight completed", zap.String("branch", branch)) - return result, nil } - if g.remoteContribution == nil { - result.Success = true - return result, nil + if g.contributionRouted() && force { + return &pushRefusal{message: "force push is not allowed for a remote contribution"} } if err := g.validateContributionRemote(ctx); err != nil { - setPushPreflightError(result, err) - return result, nil + return pushRefusalFromError(err) } - branch, err := g.getCurrentBranch(ctx) - if err != nil { - setPushPreflightError(result, err) - return result, nil + return nil +} + +func pushRefusalFromError(err error) *pushRefusal { + return &pushRefusal{ + code: classifyPushPreflightError(err), + message: err.Error(), } - environment := map[string]string{ - "GIT_TERMINAL_PROMPT": "0", - "LANG": "C", - "LC_ALL": "C", +} + +func (g *GitOperator) validateContributionSource( + ctx context.Context, + environmentOverrides map[string]string, +) *pushRefusal { + if g.remoteContribution == nil { + return nil } remote := g.remoteContribution.ContributionRemoteName() destinationRef := "refs/heads/" + g.remoteContribution.HeadBranch - sourceOutput, sourceErr := g.runGitCommandWithEnvironment( + output, err := g.runGitCommandWithEnvironment( ctx, - environment, + environmentOverrides, "ls-remote", "--refs", remote, destinationRef, ) - if sourceErr != nil { - setPushPreflightError(result, sourceErr) + if err != nil { + return pushRefusalFromError(err) + } + if strings.TrimSpace(output) == "" { + return &pushRefusal{ + code: models.AgentErrorCauseCodeSourceBranchMissing, + message: "contribution source branch is missing", + } + } + return nil +} + +// resolveSetUpstream reads the upstream tracking ref only on the path that can +// use it. The explicit-target path never sets upstream and never reads it, +// because that flag combination is refused before the push. +func (g *GitOperator) resolveSetUpstream(ctx context.Context, opts PushOptions, plan *pushPlan) bool { + if plan.explicit { + return false + } + if plan.routed { + return opts.SetUpstream + } + return opts.SetUpstream || g.getUpstreamRef(ctx) == "" +} + +// PushPreflight verifies that the configured contribution remote and final +// head-branch refspec are writable without mutating the remote or local refs. +func (g *GitOperator) PushPreflight(ctx context.Context, opts PushOptions) (*GitOperationResult, error) { + opts = opts.normalized() + if !g.tryLock("push-preflight") { + return nil, ErrOperationInProgress + } + defer g.unlock() + result := &GitOperationResult{Operation: "push_preflight"} + if refusal := g.validateContributionState(ctx, false); refusal != nil { + refusal.apply(result) return result, nil } - if strings.TrimSpace(sourceOutput) == "" { - result.Error = "contribution source branch is missing" - result.ErrorCode = models.AgentErrorCauseCodeSourceBranchMissing + // Preflight verifies the expected branch once. It publishes no baseline, so + // there is no window for a second read to close. + plan, refusal := g.resolvePushPlan(ctx, opts, true) + if refusal != nil { + refusal.apply(result) return result, nil } - refspec := "HEAD:refs/heads/" + g.remoteContribution.HeadBranch + if refusal := g.validateContributionSource(ctx, contributionPreflightEnvironment); refusal != nil { + refusal.apply(result) + return result, nil + } + + // --no-verify: a dry-run push still invokes the local pre-push hook, which + // can mutate the worktree or perform arbitrary side effects. Preflight must + // not mutate anything, so the hook must not run. output, err := g.runGitCommandWithEnvironment( ctx, - environment, + contributionPreflightEnvironment, "push", "--dry-run", + "--no-verify", "--porcelain", - remote, - refspec, + plan.remote, + plan.refspec, ) result.Output = output if err != nil { - if classifyPushPreflightHistoryUpdate(output, destinationRef) { + if destinationRef, ok := strings.CutPrefix(plan.refspec, "HEAD:"); ok && + classifyPushPreflightHistoryUpdate(output, destinationRef) { result.PreflightReason = preflightReasonHistoryUpdateRequired } setPushPreflightError(result, err) return result, nil } result.Success = true - g.logger.Info("contribution push preflight completed", zap.String("branch", branch)) + // A preflight under contribution routing keeps the result shape it has + // today; every other preflight reports what it validated. + if !plan.routed { + result.PushedRemote = plan.remote + result.PushedBranch = plan.branch + } + g.logger.Info("push preflight completed", + zap.String("branch", plan.branch), + zap.String("remote", plan.remote), + zap.Bool("contribution_routed", plan.routed), + zap.Bool("explicit_target", plan.explicit)) return result, nil } diff --git a/apps/backend/internal/agentctl/server/process/git_empty_remote.go b/apps/backend/internal/agentctl/server/process/git_empty_remote.go index ce50c38225..5a67d29718 100644 --- a/apps/backend/internal/agentctl/server/process/git_empty_remote.go +++ b/apps/backend/internal/agentctl/server/process/git_empty_remote.go @@ -19,6 +19,7 @@ const ( type emptyRemotePublication struct { active bool + published bool output string errorCode string err error @@ -135,7 +136,7 @@ func (g *GitOperator) publishEmptyRemoteBase( err: fmt.Errorf("empty-remote base was published but its local marker could not be retired: %s", g.sanitizePRFailure(retireErr.Error())), } } - return emptyRemotePublication{active: true, output: g.sanitizeGitPushOutput(output)} + return emptyRemotePublication{active: true, published: true, output: g.sanitizeGitPushOutput(output)} } func (g *GitOperator) validatePublishedEmptyRemoteBase( diff --git a/apps/backend/internal/agentctl/server/process/git_empty_remote_test.go b/apps/backend/internal/agentctl/server/process/git_empty_remote_test.go index 43ea68ea6d..0d03a4d9d5 100644 --- a/apps/backend/internal/agentctl/server/process/git_empty_remote_test.go +++ b/apps/backend/internal/agentctl/server/process/git_empty_remote_test.go @@ -16,7 +16,7 @@ import ( func TestGitOperatorPushPublishesEmptyRemoteBaseBeforeTaskBranch(t *testing.T) { repoDir, originDir, operator := setupEmptyRemoteTaskRepo(t) - result, err := operator.Push(context.Background(), false, false) + result, err := operator.Push(context.Background(), PushOptions{}) if err != nil { t.Fatalf("Push() error = %v", err) } @@ -43,7 +43,7 @@ func TestGitOperatorPushPublishesEmptyRemoteBaseBeforeTaskBranch(t *testing.T) { func TestGitOperatorPushPublishesAnotherTaskBranchAfterBaseInitialization(t *testing.T) { repoDir, originDir, operator := setupEmptyRemoteTaskRepo(t) - first, err := operator.Push(context.Background(), false, false) + first, err := operator.Push(context.Background(), PushOptions{}) if err != nil { t.Fatalf("first Push() error = %v", err) } @@ -58,7 +58,7 @@ func TestGitOperatorPushPublishesAnotherTaskBranchAfterBaseInitialization(t *tes runGit(t, repoDir, "add", "second.txt") runGit(t, repoDir, "commit", "-m", "second task change") - second, err := operator.Push(context.Background(), false, false) + second, err := operator.Push(context.Background(), PushOptions{}) if err != nil { t.Fatalf("second Push() error = %v", err) } @@ -83,7 +83,7 @@ func TestGitOperatorPushAcceptsMarkedBaseWithUnrelatedRemoteRefs(t *testing.T) { runGit(t, repoDir, "push", "origin", "external") runGit(t, repoDir, "checkout", "feature/empty") - result, err := operator.Push(context.Background(), false, false) + result, err := operator.Push(context.Background(), PushOptions{}) if err != nil { t.Fatalf("Push() error = %v", err) } @@ -116,7 +116,7 @@ func TestGitOperatorPushStopsWhenEmptyRemoteGainsHistory(t *testing.T) { runGit(t, seedDir, "commit", "-m", "external history") runGit(t, seedDir, "push", "origin", "external") - result, err := operator.Push(context.Background(), false, false) + result, err := operator.Push(context.Background(), PushOptions{}) if err != nil { t.Fatalf("Push() error = %v", err) } @@ -135,7 +135,7 @@ func TestGitOperatorPushReportsEmptyRemoteBasePublicationFailure(t *testing.T) { _, originDir, operator := setupEmptyRemoteTaskRepo(t) writeExecutable(t, filepath.Join(originDir, "hooks", "pre-receive"), "#!/bin/sh\necho reject >&2\nexit 1\n") - result, err := operator.Push(context.Background(), false, false) + result, err := operator.Push(context.Background(), PushOptions{}) if err != nil { t.Fatalf("Push() error = %v", err) } @@ -152,7 +152,7 @@ func TestGitOperatorPushReportsTaskBranchFailureAfterBasePublication(t *testing. hook := "#!/bin/sh\nwhile read old new ref; do\n case \"$ref\" in\n refs/heads/main) exit 0 ;;\n refs/heads/feature/empty) echo reject >&2; exit 1 ;;\n esac\ndone\nexit 0\n" writeExecutable(t, filepath.Join(originDir, "hooks", "pre-receive"), hook) - result, err := operator.Push(context.Background(), false, false) + result, err := operator.Push(context.Background(), PushOptions{}) if err != nil { t.Fatalf("Push() error = %v", err) } @@ -184,7 +184,7 @@ func TestGitOperatorPushRetainsMarkerWhenUnrelatedRefAppearsDuringBasePublicatio "exit 0\n" writeExecutable(t, filepath.Join(originDir, "hooks", "pre-receive"), hook) - result, err := operator.Push(context.Background(), false, false) + result, err := operator.Push(context.Background(), PushOptions{}) if err != nil { t.Fatalf("Push() error = %v", err) } @@ -229,7 +229,7 @@ func TestGitOperatorPushPublishesValidatedBaselineCommit(t *testing.T) { writeExecutable(t, filepath.Join(scriptDir, "git"), shim) t.Setenv("PATH", scriptDir+string(os.PathListSeparator)+os.Getenv("PATH")) - result, err := operator.Push(context.Background(), false, false) + result, err := operator.Push(context.Background(), PushOptions{}) if err != nil { t.Fatalf("Push() error = %v", err) } @@ -321,7 +321,7 @@ func TestGitOperatorPushRedactsEmptyRemoteProbeFailure(t *testing.T) { writeExecutable(t, filepath.Join(scriptDir, "git"), fmt.Sprintf("#!/bin/sh\nif [ \"$1\" = \"ls-remote\" ]; then\n printf 'fatal: unable to access https://user:%s@example.com/empty.git: denied\\n' >&2\n exit 1\nfi\nexec %q \"$@\"\n", secret, realGit)) t.Setenv("PATH", scriptDir+string(os.PathListSeparator)+os.Getenv("PATH")) - result, err := operator.Push(context.Background(), false, false) + result, err := operator.Push(context.Background(), PushOptions{}) if err != nil { t.Fatalf("Push() error = %v", err) } diff --git a/apps/backend/internal/agentctl/server/process/git_push_expected_branch_test.go b/apps/backend/internal/agentctl/server/process/git_push_expected_branch_test.go new file mode 100644 index 0000000000..a8c8044a89 --- /dev/null +++ b/apps/backend/internal/agentctl/server/process/git_push_expected_branch_test.go @@ -0,0 +1,838 @@ +package process + +import ( + "context" + "fmt" + "os" + osExec "os/exec" + "path/filepath" + "runtime" + "strings" + "testing" + + taskmodels "github.com/kandev/kandev/internal/task/models" +) + +// setupPushRemotesRepo builds a checkout on "feature/work" with two bare +// remotes, "origin" and "backup". origin already carries main, so empty-remote +// first publication does not apply unless a test arranges for it. +func setupPushRemotesRepo(t *testing.T) (repoDir, originDir, backupDir string, operator *GitOperator) { + t.Helper() + isolateTestGitEnv(t) + root := t.TempDir() + originDir = filepath.Join(root, "origin.git") + backupDir = filepath.Join(root, "backup.git") + repoDir = filepath.Join(root, "repo") + runGit(t, root, "init", "--bare", "--initial-branch=main", originDir) + runGit(t, root, "init", "--bare", "--initial-branch=main", backupDir) + runGit(t, root, "init", "-b", "main", repoDir) + runGit(t, repoDir, "config", "user.name", "Task User") + runGit(t, repoDir, "config", "user.email", "task@example.com") + runGit(t, repoDir, "remote", "add", "origin", originDir) + runGit(t, repoDir, "remote", "add", "backup", backupDir) + writeFile(t, repoDir, "seed.txt", "seed\n") + runGit(t, repoDir, "add", ".") + runGit(t, repoDir, "commit", "-m", "seed") + runGit(t, repoDir, "push", "origin", "main") + runGit(t, repoDir, "checkout", "-b", "feature/work") + writeFile(t, repoDir, "change.txt", "change\n") + runGit(t, repoDir, "add", ".") + runGit(t, repoDir, "commit", "-m", "task change") + + tracker := NewWorkspaceTracker(repoDir, newTestLogger(t)) + tracker.SetBaseBranch("main") + return repoDir, originDir, backupDir, NewGitOperator(repoDir, newTestLogger(t), tracker) +} + +func remoteBranchSHA(t *testing.T, remoteDir, branch string) string { + t.Helper() + cmd := osExec.Command("git", "-C", remoteDir, "rev-parse", "--verify", "--quiet", "refs/heads/"+branch) + out, err := cmd.Output() + if err != nil { + return "" + } + return strings.TrimSpace(string(out)) +} + +// recordGitCommands installs a PATH shim that logs every git invocation before +// delegating to the real binary, so a test can assert command ordering. +func recordGitCommands(t *testing.T) func() []string { + t.Helper() + if runtime.GOOS == "windows" { + t.Skip("git PATH shim is a POSIX shell script") + } + realGit, err := osExec.LookPath("git") + if err != nil { + t.Fatalf("LookPath(git) = %v", err) + } + dir := t.TempDir() + logPath := filepath.Join(dir, "git.log") + script := "#!/bin/sh\nprintf '%s\\n' \"$*\" >> " + logPath + "\nexec " + realGit + " \"$@\"\n" + if err := os.WriteFile(filepath.Join(dir, "git"), []byte(script), 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) + return func() []string { + data, err := os.ReadFile(logPath) + if err != nil { + return nil + } + var lines []string + for _, line := range strings.Split(string(data), "\n") { + if trimmed := strings.TrimSpace(line); trimmed != "" { + lines = append(lines, trimmed) + } + } + return lines + } +} + +func TestGitOperatorPushWithoutOptionsIsUnchanged(t *testing.T) { + repoDir, originDir, backupDir, operator := setupPushRemotesRepo(t) + + result, err := operator.Push(context.Background(), PushOptions{}) + if err != nil { + t.Fatalf("Push() error = %v", err) + } + if !result.Success { + t.Fatalf("Push() result = %+v, want success", result) + } + local := strings.TrimSpace(runGit(t, repoDir, "rev-parse", "HEAD")) + if got := remoteBranchSHA(t, originDir, "feature/work"); got != local { + t.Errorf("origin feature/work = %q, want %q", got, local) + } + if got := remoteBranchSHA(t, backupDir, "feature/work"); got != "" { + t.Errorf("backup gained %q, want untouched", got) + } + // A request naming no target keeps the result shape it has today. + if result.PushedRemote != "" || result.PushedBranch != "" { + t.Errorf("destination fields = (%q, %q), want both empty", result.PushedRemote, result.PushedBranch) + } + // The default path still sets upstream on first publication. + if upstream := operator.getUpstreamRef(context.Background()); upstream != "origin/feature/work" { + t.Errorf("upstream = %q, want origin/feature/work", upstream) + } +} + +func TestGitOperatorPushOmitsTargetFieldsWithoutExplicitTarget(t *testing.T) { + _, _, _, operator := setupPushRemotesRepo(t) + + result, err := operator.Push(context.Background(), PushOptions{ExpectedBranch: "feature/work"}) + if err != nil { + t.Fatalf("Push() error = %v", err) + } + if !result.Success { + t.Fatalf("Push() result = %+v, want success", result) + } + if result.PushedRemote != "" || result.PushedBranch != "" { + t.Errorf("destination fields = (%q, %q), want both empty", result.PushedRemote, result.PushedBranch) + } +} + +func TestGitOperatorPushPublishesHeadToExpectedBranch(t *testing.T) { + repoDir, originDir, backupDir, operator := setupPushRemotesRepo(t) + + result, err := operator.Push(context.Background(), PushOptions{ + Remote: "backup", + ExpectedBranch: "feature/work", + }) + if err != nil { + t.Fatalf("Push() error = %v", err) + } + if !result.Success { + t.Fatalf("Push() result = %+v, want success", result) + } + local := strings.TrimSpace(runGit(t, repoDir, "rev-parse", "HEAD")) + if got := remoteBranchSHA(t, backupDir, "feature/work"); got != local { + t.Errorf("backup feature/work = %q, want %q", got, local) + } + if got := remoteBranchSHA(t, originDir, "feature/work"); got != "" { + t.Errorf("origin gained %q, want untouched", got) + } + if result.PushedRemote != "backup" || result.PushedBranch != "feature/work" { + t.Errorf("destination fields = (%q, %q), want (backup, feature/work)", result.PushedRemote, result.PushedBranch) + } +} + +func TestGitOperatorPushCreatesMissingDestinationBranch(t *testing.T) { + repoDir, _, backupDir, operator := setupPushRemotesRepo(t) + + result, err := operator.Push(context.Background(), PushOptions{Remote: "backup"}) + if err != nil { + t.Fatalf("Push() error = %v", err) + } + if !result.Success { + t.Fatalf("Push() result = %+v, want success", result) + } + local := strings.TrimSpace(runGit(t, repoDir, "rev-parse", "HEAD")) + if got := remoteBranchSHA(t, backupDir, "feature/work"); got != local { + t.Errorf("backup feature/work = %q, want the branch created at %q", got, local) + } +} + +func TestGitOperatorPushToNamedRemoteLeavesUpstreamUnset(t *testing.T) { + _, _, _, operator := setupPushRemotesRepo(t) + + before := operator.getUpstreamRef(context.Background()) + result, err := operator.Push(context.Background(), PushOptions{Remote: "backup"}) + if err != nil { + t.Fatalf("Push() error = %v", err) + } + if !result.Success { + t.Fatalf("Push() result = %+v, want success", result) + } + if after := operator.getUpstreamRef(context.Background()); after != before { + t.Errorf("upstream changed from %q to %q", before, after) + } +} + +func TestGitOperatorPushDoesNotEscalateRejectedNonForcePush(t *testing.T) { + repoDir, _, backupDir, operator := setupPushRemotesRepo(t) + // Give backup a history the local branch does not contain. + runGit(t, repoDir, "push", "backup", "HEAD:refs/heads/feature/work") + runGit(t, repoDir, "checkout", "-b", "diverged") + writeFile(t, repoDir, "diverged.txt", "diverged\n") + runGit(t, repoDir, "add", ".") + runGit(t, repoDir, "commit", "-m", "diverged") + runGit(t, repoDir, "push", "--force", "backup", "HEAD:refs/heads/feature/work") + runGit(t, repoDir, "checkout", "feature/work") + remoteBefore := remoteBranchSHA(t, backupDir, "feature/work") + + result, err := operator.Push(context.Background(), PushOptions{Remote: "backup"}) + if err != nil { + t.Fatalf("Push() error = %v", err) + } + if result.Success { + t.Fatalf("non-force push to a diverged branch succeeded: %+v", result) + } + if got := remoteBranchSHA(t, backupDir, "feature/work"); got != remoteBefore { + t.Errorf("backup moved to %q, want %q", got, remoteBefore) + } +} + +func TestGitOperatorPushNamedFanoutRemotePublishesToEveryPushURL(t *testing.T) { + repoDir, originDir, backupDir, operator := setupPushRemotesRepo(t) + runGit(t, repoDir, "remote", "add", "mirror", originDir) + runGit(t, repoDir, "remote", "set-url", "--push", "mirror", originDir) + runGit(t, repoDir, "remote", "set-url", "--add", "--push", "mirror", backupDir) + + result, err := operator.Push(context.Background(), PushOptions{ + Remote: "mirror", ExpectedBranch: "feature/work", + }) + if err != nil { + t.Fatalf("Push() error = %v", err) + } + if !result.Success { + t.Fatalf("Push() result = %+v, want success", result) + } + want := strings.TrimSpace(runGit(t, repoDir, "rev-parse", "HEAD")) + for name, remoteDir := range map[string]string{"origin": originDir, "backup": backupDir} { + if got := remoteBranchSHA(t, remoteDir, "feature/work"); got != want { + t.Errorf("%s feature/work = %q, want %q", name, got, want) + } + } +} + +func TestGitOperatorPushToNamedRemoteSkipsBaselinePublication(t *testing.T) { + root := t.TempDir() + isolateTestGitEnv(t) + originDir := filepath.Join(root, "origin.git") + backupDir := filepath.Join(root, "backup.git") + repoDir := filepath.Join(root, "repo") + runGit(t, root, "init", "--bare", "--initial-branch=main", originDir) + runGit(t, root, "init", "--bare", "--initial-branch=main", backupDir) + runGit(t, root, "init", "-b", "main", repoDir) + runGit(t, repoDir, "config", "user.name", "Task User") + runGit(t, repoDir, "config", "user.email", "task@example.com") + runGit(t, repoDir, "remote", "add", "origin", originDir) + runGit(t, repoDir, "remote", "add", "backup", backupDir) + writeFile(t, repoDir, "seed.txt", "seed\n") + runGit(t, repoDir, "add", ".") + runGit(t, repoDir, "commit", "-m", "seed") + runGit(t, repoDir, "checkout", "-b", "feature/work") + tracker := NewWorkspaceTracker(repoDir, newTestLogger(t)) + tracker.SetBaseBranch("main") + operator := NewGitOperator(repoDir, newTestLogger(t), tracker) + + // origin is empty; a push to backup must not publish a baseline anywhere. + result, err := operator.Push(context.Background(), PushOptions{Remote: "backup"}) + if err != nil { + t.Fatalf("Push() error = %v", err) + } + if !result.Success { + t.Fatalf("Push() result = %+v, want success", result) + } + if got := remoteBranchSHA(t, originDir, "main"); got != "" { + t.Errorf("origin gained a baseline %q, want empty", got) + } + if result.BaselinePublished { + t.Error("BaselinePublished = true, want false") + } +} + +func TestGitOperatorPushRefusesTargetWithSetUpstream(t *testing.T) { + _, _, backupDir, operator := setupPushRemotesRepo(t) + + result, err := operator.Push(context.Background(), PushOptions{Remote: "backup", SetUpstream: true}) + if err != nil { + t.Fatalf("Push() error = %v", err) + } + if result.ErrorCode != pushRemoteUpstreamUnsupportedErrorCode { + t.Fatalf("ErrorCode = %q, want %q", result.ErrorCode, pushRemoteUpstreamUnsupportedErrorCode) + } + if got := remoteBranchSHA(t, backupDir, "feature/work"); got != "" { + t.Errorf("backup gained %q, want untouched", got) + } +} + +func TestGitOperatorPushRefusesExpectedBranchMismatch(t *testing.T) { + _, originDir, _, operator := setupPushRemotesRepo(t) + + result, err := operator.Push(context.Background(), PushOptions{ExpectedBranch: "feature/other"}) + if err != nil { + t.Fatalf("Push() error = %v", err) + } + if result.ErrorCode != pushBranchMismatchErrorCode { + t.Fatalf("ErrorCode = %q, want %q", result.ErrorCode, pushBranchMismatchErrorCode) + } + if result.ExpectedBranch != "feature/other" || result.CurrentBranch != "feature/work" { + t.Errorf("branches = (%q, %q), want (feature/other, feature/work)", result.ExpectedBranch, result.CurrentBranch) + } + if result.BaselinePublished { + t.Error("BaselinePublished = true, want false") + } + if got := remoteBranchSHA(t, originDir, "feature/work"); got != "" { + t.Errorf("origin gained %q, want no remote contact", got) + } +} + +func TestGitOperatorPushRefusesDetachedHeadAsMismatch(t *testing.T) { + repoDir, originDir, _, operator := setupPushRemotesRepo(t) + runGit(t, repoDir, "checkout", "--detach", "HEAD") + + result, err := operator.Push(context.Background(), PushOptions{ExpectedBranch: "feature/work"}) + if err != nil { + t.Fatalf("Push() error = %v", err) + } + if result.ErrorCode != pushBranchMismatchErrorCode { + t.Fatalf("ErrorCode = %q, want %q", result.ErrorCode, pushBranchMismatchErrorCode) + } + // A detached HEAD is an empty current branch, never the literal "HEAD". + if result.CurrentBranch != "" { + t.Errorf("CurrentBranch = %q, want empty", result.CurrentBranch) + } + if got := remoteBranchSHA(t, originDir, "feature/work"); got != "" { + t.Errorf("origin gained %q, want no remote contact", got) + } +} + +func TestGitOperatorPushRefusesDetachedHeadWithTargetOnly(t *testing.T) { + repoDir, _, backupDir, operator := setupPushRemotesRepo(t) + runGit(t, repoDir, "checkout", "--detach", "HEAD") + + result, err := operator.Push(context.Background(), PushOptions{Remote: "backup"}) + if err != nil { + t.Fatalf("Push() error = %v", err) + } + if result.ErrorCode != pushBranchDetachedErrorCode { + t.Fatalf("ErrorCode = %q, want %q", result.ErrorCode, pushBranchDetachedErrorCode) + } + if got := remoteBranchSHA(t, backupDir, "feature/work"); got != "" { + t.Errorf("backup gained %q, want untouched", got) + } +} + +func TestGitOperatorPushExpectedBranchAloneOnlyGates(t *testing.T) { + repoDir, originDir, _, operator := setupPushRemotesRepo(t) + + result, err := operator.Push(context.Background(), PushOptions{ExpectedBranch: "feature/work"}) + if err != nil { + t.Fatalf("Push() error = %v", err) + } + if !result.Success { + t.Fatalf("Push() result = %+v, want success", result) + } + // The expected branch is a precondition only: remote, refspec and upstream + // behavior are the ones the request would have had without it. + local := strings.TrimSpace(runGit(t, repoDir, "rev-parse", "HEAD")) + if got := remoteBranchSHA(t, originDir, "feature/work"); got != local { + t.Errorf("origin feature/work = %q, want %q", got, local) + } + if upstream := operator.getUpstreamRef(context.Background()); upstream != "origin/feature/work" { + t.Errorf("upstream = %q, want origin/feature/work", upstream) + } +} + +func TestGitOperatorPushRefusalOrdering(t *testing.T) { + _, _, _, operator := setupPushRemotesRepo(t) + + for _, tc := range []struct { + name string + opts PushOptions + want string + }{ + { + name: "upstream conflict outranks an invalid branch", + opts: PushOptions{Remote: "backup", SetUpstream: true, ExpectedBranch: "bad..branch"}, + want: pushRemoteUpstreamUnsupportedErrorCode, + }, + { + name: "invalid branch outranks target resolution", + opts: PushOptions{Remote: "missing", ExpectedBranch: "bad..branch"}, + want: pushBranchInvalidErrorCode, + }, + { + name: "target resolution outranks a branch mismatch", + opts: PushOptions{Remote: "missing", ExpectedBranch: "feature/other"}, + want: pushRemoteNotFoundErrorCode, + }, + { + name: "detached outranks nothing but is reported for a target alone", + opts: PushOptions{Remote: "backup", ExpectedBranch: "HEAD"}, + want: pushBranchInvalidErrorCode, + }, + } { + t.Run(tc.name, func(t *testing.T) { + result, err := operator.Push(context.Background(), tc.opts) + if err != nil { + t.Fatalf("Push() error = %v", err) + } + if result.ErrorCode != tc.want { + t.Errorf("ErrorCode = %q, want %q", result.ErrorCode, tc.want) + } + }) + } +} + +func TestGitOperatorPushRefusalsAreNoOps(t *testing.T) { + repoDir, originDir, backupDir, operator := setupPushRemotesRepo(t) + upstreamBefore := operator.getUpstreamRef(context.Background()) + headBefore := strings.TrimSpace(runGit(t, repoDir, "rev-parse", "HEAD")) + + for _, opts := range []PushOptions{ + {Remote: "missing"}, + {Remote: "https://example.test/nope.git"}, + {Remote: "backup", SetUpstream: true}, + {ExpectedBranch: "bad..branch"}, + {ExpectedBranch: "feature/other"}, + } { + result, err := operator.Push(context.Background(), opts) + if err != nil { + t.Fatalf("Push(%+v) error = %v", opts, err) + } + if result.Success { + t.Fatalf("Push(%+v) succeeded, want refusal", opts) + } + } + + if got := remoteBranchSHA(t, originDir, "feature/work"); got != "" { + t.Errorf("origin gained %q", got) + } + if got := remoteBranchSHA(t, backupDir, "feature/work"); got != "" { + t.Errorf("backup gained %q", got) + } + if got := operator.getUpstreamRef(context.Background()); got != upstreamBefore { + t.Errorf("upstream changed from %q to %q", upstreamBefore, got) + } + if got := strings.TrimSpace(runGit(t, repoDir, "rev-parse", "HEAD")); got != headBefore { + t.Errorf("HEAD moved from %q to %q", headBefore, got) + } +} + +func TestGitOperatorPushReportsOperationInProgress(t *testing.T) { + _, _, _, operator := setupPushRemotesRepo(t) + if !operator.tryLock("test") { + t.Fatal("tryLock() = false, want the lock") + } + defer operator.unlock() + + _, err := operator.Push(context.Background(), PushOptions{Remote: "backup"}) + if err != ErrOperationInProgress { + t.Fatalf("Push() error = %v, want ErrOperationInProgress", err) + } +} + +func TestGitOperatorPushRepeatedIdenticalRequestSucceeds(t *testing.T) { + repoDir, _, backupDir, operator := setupPushRemotesRepo(t) + opts := PushOptions{Remote: "backup", ExpectedBranch: "feature/work"} + + first, err := operator.Push(context.Background(), opts) + if err != nil || !first.Success { + t.Fatalf("first Push() = %+v, err = %v", first, err) + } + after := remoteBranchSHA(t, backupDir, "feature/work") + + second, err := operator.Push(context.Background(), opts) + if err != nil { + t.Fatalf("second Push() error = %v", err) + } + if !second.Success { + t.Fatalf("second Push() = %+v, want success", second) + } + if got := remoteBranchSHA(t, backupDir, "feature/work"); got != after { + t.Errorf("destination moved from %q to %q", after, got) + } + local := strings.TrimSpace(runGit(t, repoDir, "rev-parse", "HEAD")) + if after != local { + t.Errorf("destination = %q, want %q", after, local) + } +} + +func TestGitOperatorPushNeverCreatesRemote(t *testing.T) { + repoDir, _, _, operator := setupPushRemotesRepo(t) + before := strings.TrimSpace(runGit(t, repoDir, "remote")) + + if _, err := operator.Push(context.Background(), PushOptions{Remote: "https://example.test/new.git"}); err != nil { + t.Fatalf("Push() error = %v", err) + } + if _, err := operator.Push(context.Background(), PushOptions{Remote: "brand-new"}); err != nil { + t.Fatalf("Push() error = %v", err) + } + if after := strings.TrimSpace(runGit(t, repoDir, "remote")); after != before { + t.Errorf("remotes changed from %q to %q", before, after) + } +} + +func TestGitOperatorPushResultAndLogsCarryNoRemoteURL(t *testing.T) { + repoDir, _, backupDir, _ := setupPushRemotesRepo(t) + log, observed := newObservedTestLogger(t) + operator := NewGitOperator(repoDir, log, nil) + + result, err := operator.Push(context.Background(), PushOptions{Remote: backupDir}) + if err != nil { + t.Fatalf("Push() error = %v", err) + } + if !result.Success { + t.Fatalf("Push() result = %+v, want success", result) + } + // The URL form resolves to a name, and only the name is reported back. + if result.PushedRemote != "backup" { + t.Errorf("PushedRemote = %q, want backup", result.PushedRemote) + } + if strings.Contains(result.Error, backupDir) { + t.Errorf("error carries the remote URL: %q", result.Error) + } + for _, entry := range observed.All() { + if strings.Contains(entry.Message, backupDir) || strings.Contains(fmt.Sprint(entry.ContextMap()), backupDir) { + t.Errorf("log entry carries the remote URL: message=%q context=%v", entry.Message, entry.ContextMap()) + } + } +} + +func TestGitOperatorPushRedactsCredentialsOnExplicitTargetPath(t *testing.T) { + repoDir, _, _, operator := setupPushRemotesRepo(t) + const secret = "s3cr3t-token" + runGit(t, repoDir, "remote", "add", "secured", "https://user:"+secret+"@example.invalid/repo.git") + + result, err := operator.Push(context.Background(), PushOptions{Remote: "secured"}) + if err != nil { + t.Fatalf("Push() error = %v", err) + } + if result.Success { + t.Fatal("push to an unreachable host succeeded") + } + if strings.Contains(result.Output, secret) || strings.Contains(result.Error, secret) { + t.Errorf("credential leaked: output=%q error=%q", result.Output, result.Error) + } +} + +func TestGitOperatorPushVerifiesBranchAsLastReadBeforePush(t *testing.T) { + for _, tc := range []struct { + name string + opts PushOptions + }{ + {name: "explicit target", opts: PushOptions{Remote: "backup", ExpectedBranch: "feature/work"}}, + {name: "default path", opts: PushOptions{ExpectedBranch: "feature/work"}}, + } { + t.Run(tc.name, func(t *testing.T) { + _, _, _, operator := setupPushRemotesRepo(t) + readLog := recordGitCommands(t) + + result, err := operator.Push(context.Background(), tc.opts) + if err != nil { + t.Fatalf("Push() error = %v", err) + } + if !result.Success { + t.Fatalf("Push() result = %+v, want success", result) + } + + lines := readLog() + pushIndex := -1 + for i, line := range lines { + if strings.HasPrefix(line, "push ") { + pushIndex = i + } + } + if pushIndex <= 0 { + t.Fatalf("no push command recorded in %v", lines) + } + if prev := lines[pushIndex-1]; prev != "symbolic-ref HEAD" { + t.Errorf("command before push = %q, want the expected-branch read %q (full log: %v)", + prev, "symbolic-ref HEAD", lines) + } + }) + } +} + +func TestGitOperatorPushRefusesTargetUnderContributionRouting(t *testing.T) { + repoDir, _, backupDir, operator := setupPushRemotesRepo(t) + head := strings.TrimSpace(runGit(t, repoDir, "rev-parse", "HEAD")) + const sourceURL = "https://github.com/contributor/widget.git" + binding := &taskmodels.RemoteContribution{ + Version: taskmodels.RemoteContributionVersion, + Provider: taskmodels.RemoteContributionProviderGitHub, + Kind: taskmodels.RemoteContributionKindPullRequest, + CanonicalURL: "https://github.com/acme/widget/pull/7", + Number: 7, + State: taskmodels.RemoteContributionStateOpen, + BaseBranch: "main", + HeadBranch: "feature/work", + HeadSHA: head, + SourceRepository: taskmodels.RemoteContributionRepository{ + Host: "github.com", Path: "contributor/widget", RemoteURL: sourceURL, + }, + CollaborationAllowed: true, + } + if err := binding.Validate(); err != nil { + t.Fatalf("binding.Validate() = %v", err) + } + runGit(t, repoDir, "remote", "add", binding.ContributionRemoteName(), sourceURL) + operator.setRemoteContribution(binding) + + result, err := operator.Push(context.Background(), PushOptions{Remote: "backup"}) + if err != nil { + t.Fatalf("Push() error = %v", err) + } + if result.ErrorCode != pushRemoteContributionConflictErrorCode { + t.Fatalf("ErrorCode = %q, want %q", result.ErrorCode, pushRemoteContributionConflictErrorCode) + } + if got := remoteBranchSHA(t, backupDir, "feature/work"); got != "" { + t.Errorf("backup gained %q, want untouched", got) + } +} + +// TestGitOperatorPushReportsMismatchAfterBaselinePublication drives the only +// refusal in this capability that is not side-effect-free: HEAD moves after +// empty-remote first publication published the baseline, so the second +// verification refuses a push whose baseline is already on the remote. +func TestGitOperatorPushReportsMismatchAfterBaselinePublication(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("git PATH shim is a POSIX shell script") + } + repoDir, originDir, operator := setupEmptyRemoteTaskRepo(t) + realGit, err := osExec.LookPath("git") + if err != nil { + t.Fatalf("LookPath(git) = %v", err) + } + // Move HEAD immediately after the baseline push, which is the window the + // second verification exists to close. + shimDir := t.TempDir() + script := "#!/bin/sh\n" + + "case \"$1 $2\" in\n" + + " 'push --force-with-lease=refs/heads/main:')\n" + + " " + realGit + " \"$@\"; rc=$?\n" + + " " + realGit + " -C " + repoDir + " symbolic-ref HEAD refs/heads/switched\n" + + " exit $rc ;;\n" + + "esac\n" + + "exec " + realGit + " \"$@\"\n" + if err := os.WriteFile(filepath.Join(shimDir, "git"), []byte(script), 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", shimDir+string(os.PathListSeparator)+os.Getenv("PATH")) + + result, err := operator.Push(context.Background(), PushOptions{ExpectedBranch: "feature/empty"}) + if err != nil { + t.Fatalf("Push() error = %v", err) + } + if result.Success { + t.Fatalf("Push() succeeded, want a post-baseline mismatch refusal: %+v", result) + } + if result.ErrorCode != pushBranchMismatchAfterBaselineErrorCode { + t.Fatalf("ErrorCode = %q, want %q", result.ErrorCode, pushBranchMismatchAfterBaselineErrorCode) + } + if !result.BaselinePublished { + t.Error("BaselinePublished = false, want true") + } + if result.ExpectedBranch != "feature/empty" || result.CurrentBranch != "switched" { + t.Errorf("branches = (%q, %q), want (feature/empty, switched)", result.ExpectedBranch, result.CurrentBranch) + } + // The baseline landed; the task branch must not have. + if got := remoteBranchSHA(t, originDir, "main"); got == "" { + t.Error("baseline was not published, so this test did not exercise its window") + } + if got := remoteBranchSHA(t, originDir, "feature/empty"); got != "" { + t.Errorf("task branch published as %q, want unpublished", got) + } +} + +// TestGitOperatorPushReportsPlainMismatchAtSecondVerification drives the other +// outcome of the second verification: a mismatch that did not follow a +// baseline publication. An explicit push target to a remote other than +// "origin" is never baseline-eligible, so moving HEAD in the window between +// the first and second verification must still be caught, and must carry +// plain push_branch_mismatch rather than the after-baseline code. +func TestGitOperatorPushReportsPlainMismatchAtSecondVerification(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("git PATH shim is a POSIX shell script") + } + repoDir, _, backupDir, operator := setupPushRemotesRepo(t) + realGit, err := osExec.LookPath("git") + if err != nil { + t.Fatalf("LookPath(git) = %v", err) + } + // The explicit-target, no-baseline path issues exactly two "symbolic-ref + // HEAD" reads: the first verification inside resolvePushPlan, and the + // second verification immediately before the push. Move HEAD right after + // answering the first one. + shimDir := t.TempDir() + counterPath := filepath.Join(shimDir, "count") + script := "#!/bin/sh\n" + + "if [ \"$1 $2\" = 'symbolic-ref HEAD' ]; then\n" + + " n=$(cat " + counterPath + " 2>/dev/null || echo 0)\n" + + " n=$((n+1))\n" + + " echo $n > " + counterPath + "\n" + + " " + realGit + " \"$@\"; rc=$?\n" + + " if [ \"$n\" = \"1\" ]; then\n" + + " " + realGit + " -C " + repoDir + " symbolic-ref HEAD refs/heads/switched\n" + + " fi\n" + + " exit $rc\n" + + "fi\n" + + "exec " + realGit + " \"$@\"\n" + if err := os.WriteFile(filepath.Join(shimDir, "git"), []byte(script), 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", shimDir+string(os.PathListSeparator)+os.Getenv("PATH")) + + result, err := operator.Push(context.Background(), PushOptions{Remote: "backup", ExpectedBranch: "feature/work"}) + if err != nil { + t.Fatalf("Push() error = %v", err) + } + if result.Success { + t.Fatalf("Push() succeeded, want a second-verification mismatch refusal: %+v", result) + } + if result.ErrorCode != pushBranchMismatchErrorCode { + t.Fatalf("ErrorCode = %q, want %q", result.ErrorCode, pushBranchMismatchErrorCode) + } + if result.BaselinePublished { + t.Error("BaselinePublished = true, want false: backup is never baseline-eligible") + } + if result.ExpectedBranch != "feature/work" || result.CurrentBranch != "switched" { + t.Errorf("branches = (%q, %q), want (feature/work, switched)", result.ExpectedBranch, result.CurrentBranch) + } + if got := remoteBranchSHA(t, backupDir, "feature/work"); got != "" { + t.Errorf("backup gained %q, want untouched", got) + } +} + +// TestGitOperatorPushReportsPlainMismatchWhenBaselineWasAlreadyPublished +// covers the empty-remote path where the baseline was published by an earlier +// request: this request's prepareEmptyRemotePublication only retires the +// local marker and pushes nothing itself. A HEAD race at the second +// verification must still report plain push_branch_mismatch, not the +// after-baseline variant, because no baseline publication happened in this +// request. +func TestGitOperatorPushReportsPlainMismatchWhenBaselineWasAlreadyPublished(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("git PATH shim is a POSIX shell script") + } + repoDir, originDir, operator := setupEmptyRemoteTaskRepo(t) + // Publish the baseline directly, bypassing gitbootstrap, to simulate an + // earlier request having already published it. The local marker is left + // in place, so this request's empty-remote path takes the + // already-published, retire-only branch. + runGit(t, repoDir, "push", originDir, "main:refs/heads/main") + + realGit, err := osExec.LookPath("git") + if err != nil { + t.Fatalf("LookPath(git) = %v", err) + } + // Move HEAD right after the remote-ref probe that decides the baseline is + // already published, which is before this request issues any push. + shimDir := t.TempDir() + script := "#!/bin/sh\n" + + "if [ \"$1 $2\" = 'ls-remote --refs' ]; then\n" + + " " + realGit + " \"$@\"; rc=$?\n" + + " " + realGit + " -C " + repoDir + " symbolic-ref HEAD refs/heads/switched\n" + + " exit $rc\n" + + "fi\n" + + "exec " + realGit + " \"$@\"\n" + if err := os.WriteFile(filepath.Join(shimDir, "git"), []byte(script), 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", shimDir+string(os.PathListSeparator)+os.Getenv("PATH")) + + result, err := operator.Push(context.Background(), PushOptions{ExpectedBranch: "feature/empty"}) + if err != nil { + t.Fatalf("Push() error = %v", err) + } + if result.Success { + t.Fatalf("Push() succeeded, want a second-verification mismatch refusal: %+v", result) + } + if result.ErrorCode != pushBranchMismatchErrorCode { + t.Fatalf("ErrorCode = %q, want %q (not the after-baseline variant: no publication happened this request)", result.ErrorCode, pushBranchMismatchErrorCode) + } + if result.BaselinePublished { + t.Error("BaselinePublished = true, want false: the baseline was published by an earlier request, not this one") + } + if result.ExpectedBranch != "feature/empty" || result.CurrentBranch != "switched" { + t.Errorf("branches = (%q, %q), want (feature/empty, switched)", result.ExpectedBranch, result.CurrentBranch) + } + if got := remoteBranchSHA(t, originDir, "feature/empty"); got != "" { + t.Errorf("task branch published as %q, want unpublished", got) + } +} + +// TestGitOperatorPushUsesExpectedBranchNotARaceableRereadForNoTargetRefspec +// covers the no-explicit-target path (the default-remote and +// contribution-destination cases in buildPushPlan both build a bare-name +// refspec off a branch value). That value must come from the same identity +// verifyExpectedBranch confirms immediately before the push, not from a +// separate read: a separate read can observe a different branch if HEAD +// moves and moves back between the first verification and this one, letting +// the second verification pass while the push publishes unrelated content +// under an unverified name and leaves the intended branch unpublished. +func TestGitOperatorPushUsesExpectedBranchNotARaceableRereadForNoTargetRefspec(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("git PATH shim is a POSIX shell script") + } + repoDir, originDir, _, operator := setupPushRemotesRepo(t) + runGit(t, repoDir, "branch", "feature/other", "main") + + realGit, err := osExec.LookPath("git") + if err != nil { + t.Fatalf("LookPath(git) = %v", err) + } + // Move HEAD to feature/other around the rev-parse call that would decide + // the push's branch/refspec independently of the first verification, then + // move it back before the push runs, so only a re-read would observe the + // switch. + shimDir := t.TempDir() + script := "#!/bin/sh\n" + + "if [ \"$1 $2 $3\" = 'rev-parse --abbrev-ref HEAD' ]; then\n" + + " " + realGit + " -C " + repoDir + " checkout feature/other >/dev/null 2>&1\n" + + " " + realGit + " \"$@\"; rc=$?\n" + + " " + realGit + " -C " + repoDir + " checkout feature/work >/dev/null 2>&1\n" + + " exit $rc\n" + + "fi\n" + + "exec " + realGit + " \"$@\"\n" + if err := os.WriteFile(filepath.Join(shimDir, "git"), []byte(script), 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", shimDir+string(os.PathListSeparator)+os.Getenv("PATH")) + + wantSHA := strings.TrimSpace(runGit(t, repoDir, "rev-parse", "feature/work")) + + result, err := operator.Push(context.Background(), PushOptions{ExpectedBranch: "feature/work"}) + if err != nil { + t.Fatalf("Push() error = %v", err) + } + if !result.Success { + t.Fatalf("Push() = %+v, want success", result) + } + if got := remoteBranchSHA(t, originDir, "feature/work"); got != wantSHA { + t.Errorf("origin feature/work = %q, want %q: the verified branch was not what got pushed", got, wantSHA) + } + if got := remoteBranchSHA(t, originDir, "feature/other"); got != "" { + t.Errorf("origin gained feature/other (%q): the push targeted a branch never verified", got) + } +} diff --git a/apps/backend/internal/agentctl/server/process/git_push_preflight_test.go b/apps/backend/internal/agentctl/server/process/git_push_preflight_test.go index 0852adadf0..da3338905b 100644 --- a/apps/backend/internal/agentctl/server/process/git_push_preflight_test.go +++ b/apps/backend/internal/agentctl/server/process/git_push_preflight_test.go @@ -3,8 +3,15 @@ package process import ( "context" "encoding/json" + "fmt" + "os" + osExec "os/exec" + "path/filepath" + "runtime" "strings" "testing" + + taskmodels "github.com/kandev/kandev/internal/task/models" ) func TestPushPreflightHistoryClassification(t *testing.T) { @@ -12,7 +19,7 @@ func TestPushPreflightHistoryClassification(t *testing.T) { operator := NewGitOperator(repoDir, newTestLogger(t), nil) operator.setRemoteContribution(binding) - result, err := operator.PushPreflight(context.Background()) + result, err := operator.PushPreflight(context.Background(), PushOptions{}) if err != nil { t.Fatalf("PushPreflight returned error: %v", err) } @@ -40,6 +47,83 @@ func TestPushPreflightHistoryClassification(t *testing.T) { } } +func TestPushPreflightRejectsMissingContributionSource(t *testing.T) { + repoDir, originDir, _, binding, _, _, _ := setupDivergedContributionRepo(t) + runGit(t, originDir, "update-ref", "-d", "refs/heads/"+binding.HeadBranch) + + operator := NewGitOperator(repoDir, newTestLogger(t), nil) + operator.setRemoteContribution(binding) + result, err := operator.PushPreflight(context.Background(), PushOptions{}) + if err != nil { + t.Fatalf("PushPreflight returned error: %v", err) + } + if result.Success { + t.Fatalf("PushPreflight = %+v, want missing-source refusal", result) + } + if result.ErrorCode != taskmodels.AgentErrorCauseCodeSourceBranchMissing { + t.Fatalf("ErrorCode = %q, want %q", result.ErrorCode, taskmodels.AgentErrorCauseCodeSourceBranchMissing) + } + if got := remoteBranchSHA(t, originDir, binding.HeadBranch); got != "" { + t.Fatalf("missing contribution source was recreated at %q", got) + } +} + +func TestPushPreflightUsesStableGitEnvironment(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("git PATH shim is a POSIX shell script") + } + repoDir, _, _, binding, _, _, _ := setupDivergedContributionRepo(t) + operator := NewGitOperator(repoDir, newTestLogger(t), nil) + operator.setRemoteContribution(binding) + + realGit, err := osExec.LookPath("git") + if err != nil { + t.Fatalf("LookPath(git) = %v", err) + } + shimDir := t.TempDir() + envPath := filepath.Join(shimDir, "push-env") + script := fmt.Sprintf("#!/bin/sh\ncase \"$1\" in\nls-remote|push) printf 'LANG=%%s\\nLC_ALL=%%s\\nGIT_TERMINAL_PROMPT=%%s\\n' \"$LANG\" \"$LC_ALL\" \"$GIT_TERMINAL_PROMPT\" > \"$KANDEV_TEST_PUSH_ENV\";;\nesac\nexec %s \"$@\"\n", realGit) + if err := os.WriteFile(filepath.Join(shimDir, "git"), []byte(script), 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", shimDir+string(os.PathListSeparator)+os.Getenv("PATH")) + t.Setenv("KANDEV_TEST_PUSH_ENV", envPath) + t.Setenv("LANG", "pt_PT.UTF-8") + t.Setenv("LC_ALL", "pt_PT.UTF-8") + t.Setenv("GIT_TERMINAL_PROMPT", "1") + + _, err = operator.PushPreflight(context.Background(), PushOptions{}) + if err != nil { + t.Fatalf("PushPreflight returned error: %v", err) + } + env, err := os.ReadFile(envPath) + if err != nil { + t.Fatalf("read recorded Git environment: %v", err) + } + want := "LANG=C\nLC_ALL=C\nGIT_TERMINAL_PROMPT=0\n" + if got := string(env); got != want { + t.Fatalf("preflight Git environment = %q, want %q", got, want) + } +} + +func TestPushPreflightPreservesContributionValidationErrorCode(t *testing.T) { + repoDir, _, remoteName, binding, _, _, _ := setupDivergedContributionRepo(t) + runGit(t, repoDir, "remote", "set-url", remoteName, "https://example.test/wrong-source.git") + + operator := NewGitOperator(repoDir, newTestLogger(t), nil) + operator.setRemoteContribution(binding) + result, err := operator.PushPreflight(context.Background(), PushOptions{}) + if err != nil { + t.Fatalf("PushPreflight returned error: %v", err) + } + if result.Success { + t.Fatalf("PushPreflight = %+v, want validation refusal", result) + } + if result.ErrorCode != taskmodels.AgentErrorCauseCodeDestinationInvalid { + t.Fatalf("ErrorCode = %q, want %q", result.ErrorCode, taskmodels.AgentErrorCauseCodeDestinationInvalid) + } +} + func TestClassifyPushPreflightHistoryUpdate(t *testing.T) { const destination = "refs/heads/feature/remote" tests := []struct { @@ -86,3 +170,233 @@ func TestClassifyPushPreflightHistoryUpdate(t *testing.T) { }) } } + +func TestPushPreflightValidatesOriginWithoutTarget(t *testing.T) { + _, originDir, _, operator := setupPushRemotesRepo(t) + + result, err := operator.PushPreflight(context.Background(), PushOptions{}) + if err != nil { + t.Fatalf("PushPreflight() error = %v", err) + } + if !result.Success { + t.Fatalf("PushPreflight() = %+v, want success", result) + } + // It reports what it validated rather than a blanket success. + if result.PushedRemote != "origin" || result.PushedBranch != "feature/work" { + t.Errorf("validated (%q, %q), want (origin, feature/work)", result.PushedRemote, result.PushedBranch) + } + if got := remoteBranchSHA(t, originDir, "feature/work"); got != "" { + t.Errorf("preflight published %q, want a dry run", got) + } +} + +func TestPushPreflightValidatesNamedRemote(t *testing.T) { + _, originDir, backupDir, operator := setupPushRemotesRepo(t) + + result, err := operator.PushPreflight(context.Background(), PushOptions{ + Remote: "backup", + ExpectedBranch: "feature/work", + }) + if err != nil { + t.Fatalf("PushPreflight() error = %v", err) + } + if !result.Success { + t.Fatalf("PushPreflight() = %+v, want success", result) + } + if result.PushedRemote != "backup" || result.PushedBranch != "feature/work" { + t.Errorf("validated (%q, %q), want (backup, feature/work)", result.PushedRemote, result.PushedBranch) + } + if got := remoteBranchSHA(t, backupDir, "feature/work"); got != "" { + t.Errorf("preflight published %q to backup, want a dry run", got) + } + if got := remoteBranchSHA(t, originDir, "feature/work"); got != "" { + t.Errorf("preflight touched origin (%q)", got) + } +} + +func TestPushPreflightReportsValidatedRemoteAndBranch(t *testing.T) { + _, _, _, operator := setupPushRemotesRepo(t) + + // Both with and without an explicit target, a non-routed preflight reports + // the destination it checked. + for _, opts := range []PushOptions{{}, {Remote: "backup"}} { + result, err := operator.PushPreflight(context.Background(), opts) + if err != nil { + t.Fatalf("PushPreflight(%+v) error = %v", opts, err) + } + if !result.Success { + t.Fatalf("PushPreflight(%+v) = %+v, want success", opts, result) + } + if result.PushedRemote == "" || result.PushedBranch == "" { + t.Errorf("PushPreflight(%+v) reported (%q, %q), want both populated", + opts, result.PushedRemote, result.PushedBranch) + } + } +} + +func TestPushPreflightReportsNoRemoteConfigured(t *testing.T) { + root := t.TempDir() + isolateTestGitEnv(t) + repoDir := filepath.Join(root, "repo") + runGit(t, root, "init", "-b", "main", repoDir) + runGit(t, repoDir, "config", "user.name", "Task User") + runGit(t, repoDir, "config", "user.email", "task@example.com") + writeFile(t, repoDir, "seed.txt", "seed\n") + runGit(t, repoDir, "add", ".") + runGit(t, repoDir, "commit", "-m", "seed") + operator := NewGitOperator(repoDir, newTestLogger(t), nil) + + result, err := operator.PushPreflight(context.Background(), PushOptions{}) + if err != nil { + t.Fatalf("PushPreflight() error = %v", err) + } + if result.Success { + t.Fatalf("PushPreflight() = %+v, want a refusal", result) + } + if result.ErrorCode != pushNoRemoteConfiguredErrorCode { + t.Errorf("ErrorCode = %q, want %q", result.ErrorCode, pushNoRemoteConfiguredErrorCode) + } +} + +func TestPushPreflightMutatesNothing(t *testing.T) { + repoDir, originDir, backupDir, operator := setupPushRemotesRepo(t) + headBefore := strings.TrimSpace(runGit(t, repoDir, "rev-parse", "HEAD")) + branchesBefore := runGit(t, repoDir, "branch", "--all") + upstreamBefore := operator.getUpstreamRef(context.Background()) + originMain := remoteBranchSHA(t, originDir, "main") + + if _, err := operator.PushPreflight(context.Background(), PushOptions{Remote: "backup"}); err != nil { + t.Fatalf("PushPreflight() error = %v", err) + } + + if got := strings.TrimSpace(runGit(t, repoDir, "rev-parse", "HEAD")); got != headBefore { + t.Errorf("HEAD moved from %q to %q", headBefore, got) + } + if got := runGit(t, repoDir, "branch", "--all"); got != branchesBefore { + t.Errorf("local refs changed:\n%s\nwant:\n%s", got, branchesBefore) + } + if got := operator.getUpstreamRef(context.Background()); got != upstreamBefore { + t.Errorf("upstream changed from %q to %q", upstreamBefore, got) + } + if got := remoteBranchSHA(t, originDir, "main"); got != originMain { + t.Errorf("origin main moved from %q to %q", originMain, got) + } + if got := remoteBranchSHA(t, backupDir, "feature/work"); got != "" { + t.Errorf("backup gained %q", got) + } +} + +func TestPushPreflightReportsRemoteRefusalOutput(t *testing.T) { + repoDir, _, backupDir, operator := setupPushRemotesRepo(t) + // Give backup a history the local branch does not contain, so the write is + // a non-fast-forward the remote refuses. + runGit(t, repoDir, "checkout", "-b", "diverged") + writeFile(t, repoDir, "diverged.txt", "diverged\n") + runGit(t, repoDir, "add", ".") + runGit(t, repoDir, "commit", "-m", "diverged") + runGit(t, repoDir, "push", "backup", "HEAD:refs/heads/feature/work") + runGit(t, repoDir, "checkout", "feature/work") + before := remoteBranchSHA(t, backupDir, "feature/work") + + result, err := operator.PushPreflight(context.Background(), PushOptions{Remote: "backup"}) + if err != nil { + t.Fatalf("PushPreflight() returned a transport error rather than a result: %v", err) + } + if result.Success { + t.Fatalf("PushPreflight() = %+v, want the refusal reported", result) + } + if result.Error == "" && result.Output == "" { + t.Error("refusal reported neither an error nor the remote's output") + } + if got := remoteBranchSHA(t, backupDir, "feature/work"); got != before { + t.Errorf("backup moved from %q to %q", before, got) + } +} + +func TestPushPreflightRedactsCredentialsInRemoteOutput(t *testing.T) { + repoDir, _, _, operator := setupPushRemotesRepo(t) + const secret = "s3cr3t-token" + runGit(t, repoDir, "remote", "add", "secured", "https://user:"+secret+"@example.invalid/repo.git") + + result, err := operator.PushPreflight(context.Background(), PushOptions{Remote: "secured"}) + if err != nil { + t.Fatalf("PushPreflight() error = %v", err) + } + if result.Success { + t.Fatal("preflight against an unreachable host succeeded") + } + if strings.Contains(result.Output, secret) || strings.Contains(result.Error, secret) { + t.Errorf("credential leaked: output=%q error=%q", result.Output, result.Error) + } +} + +func TestPushPreflightDoesNotRunLocalPrePushHook(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("pre-push hook is a POSIX shell script") + } + repoDir, _, _, operator := setupPushRemotesRepo(t) + marker := filepath.Join(t.TempDir(), "hook-ran") + hookPath := filepath.Join(repoDir, ".git", "hooks", "pre-push") + hookScript := "#!/bin/sh\ntouch " + marker + "\nexit 0\n" + if err := os.WriteFile(hookPath, []byte(hookScript), 0o755); err != nil { + t.Fatal(err) + } + + result, err := operator.PushPreflight(context.Background(), PushOptions{Remote: "backup"}) + if err != nil { + t.Fatalf("PushPreflight() error = %v", err) + } + if !result.Success { + t.Fatalf("PushPreflight() = %+v, want success", result) + } + if _, statErr := os.Stat(marker); statErr == nil { + t.Error("PushPreflight() ran the local pre-push hook, want it bypassed") + } +} + +func TestPushPreflightReportsOperationInProgress(t *testing.T) { + _, _, _, operator := setupPushRemotesRepo(t) + if !operator.tryLock("test") { + t.Fatal("tryLock() = false, want the lock") + } + defer operator.unlock() + + _, err := operator.PushPreflight(context.Background(), PushOptions{Remote: "backup"}) + if err != ErrOperationInProgress { + t.Fatalf("PushPreflight() error = %v, want ErrOperationInProgress", err) + } +} + +func TestPushPreflightAppliesSameRefusalOrdering(t *testing.T) { + _, _, backupDir, operator := setupPushRemotesRepo(t) + + for _, tc := range []struct { + name string + opts PushOptions + want string + }{ + {name: "upstream unsupported", opts: PushOptions{Remote: "backup", SetUpstream: true}, want: pushRemoteUpstreamUnsupportedErrorCode}, + {name: "invalid expected branch", opts: PushOptions{ExpectedBranch: "bad..branch"}, want: pushBranchInvalidErrorCode}, + {name: "unknown remote name", opts: PushOptions{Remote: "missing"}, want: pushRemoteNotFoundErrorCode}, + {name: "unmatched remote url", opts: PushOptions{Remote: "https://example.test/nope.git"}, want: pushRemoteURLUnmatchedErrorCode}, + {name: "branch mismatch", opts: PushOptions{ExpectedBranch: "feature/other"}, want: pushBranchMismatchErrorCode}, + {name: "invalid branch outranks resolution", opts: PushOptions{Remote: "missing", ExpectedBranch: "bad..branch"}, want: pushBranchInvalidErrorCode}, + } { + t.Run(tc.name, func(t *testing.T) { + result, err := operator.PushPreflight(context.Background(), tc.opts) + if err != nil { + t.Fatalf("PushPreflight() error = %v", err) + } + if result.ErrorCode != tc.want { + t.Errorf("ErrorCode = %q, want %q", result.ErrorCode, tc.want) + } + // Preflight publishes no baseline, so this code is unreachable. + if result.ErrorCode == pushBranchMismatchAfterBaselineErrorCode { + t.Error("preflight reported a post-baseline mismatch") + } + }) + } + if got := remoteBranchSHA(t, backupDir, "feature/work"); got != "" { + t.Errorf("backup gained %q during refusals", got) + } +} diff --git a/apps/backend/internal/agentctl/server/process/git_push_target.go b/apps/backend/internal/agentctl/server/process/git_push_target.go new file mode 100644 index 0000000000..f926d1c643 --- /dev/null +++ b/apps/backend/internal/agentctl/server/process/git_push_target.go @@ -0,0 +1,441 @@ +package process + +import ( + "context" + "errors" + "fmt" + "os/exec" + "sort" + "strings" + + "github.com/kandev/kandev/internal/common/securityutil" +) + +// Stable error codes for the configured-push-target contract. Callers match on +// these rather than on message text. +const ( + pushRemoteNotFoundErrorCode = "push_remote_not_found" + pushRemoteURLUnmatchedErrorCode = "push_remote_url_unmatched" + pushRemoteFanoutErrorCode = "push_remote_fanout" + pushRemoteConfigUnreadableErrorCode = "push_remote_config_unreadable" + pushRemoteContributionConflictErrorCode = "push_remote_contribution_conflict" + pushRemoteUpstreamUnsupportedErrorCode = "push_remote_upstream_unsupported" + pushBranchInvalidErrorCode = "push_branch_invalid" + pushBranchMismatchErrorCode = "push_branch_mismatch" + pushBranchMismatchAfterBaselineErrorCode = "push_branch_mismatch_after_baseline" + pushBranchDetachedErrorCode = "push_branch_detached" + pushNoRemoteConfiguredErrorCode = "push_no_remote_configured" +) + +// defaultPushRemote is the remote the push contract targets when a request +// names none and no contribution routing applies. +const defaultPushRemote = "origin" + +// PushOptions carries the inputs of a push or push-preflight request. +// +// Remote is an explicit push target, given either as a configured remote name +// or as a remote URL that must resolve to an already-configured remote. +// ExpectedBranch is the branch the caller believes it is publishing. Both are +// optional; absent reproduces the behavior of a request that names neither. +type PushOptions struct { + Force bool + SetUpstream bool + Remote string + ExpectedBranch string +} + +// normalized trims both string inputs. A value that is empty after the trim is +// absent, so a whitespace-only input is an omitted input rather than a refusal. +func (o PushOptions) normalized() PushOptions { + o.Remote = strings.TrimSpace(o.Remote) + o.ExpectedBranch = strings.TrimSpace(o.ExpectedBranch) + return o +} + +// pushRefusal is a refusal carrying one of the stable error codes above. +type pushRefusal struct { + code string + message string + expectedBranch string + currentBranch string + baselinePublished bool +} + +func (r *pushRefusal) apply(result *GitOperationResult) { + result.ErrorCode = r.code + result.Error = r.message + result.ExpectedBranch = r.expectedBranch + result.CurrentBranch = r.currentBranch + result.BaselinePublished = r.baselinePublished +} + +// branchMismatchRefusal reports the expected and current branches so a caller +// can see what it asked for and what the checkout was actually on. A detached +// HEAD is reported as an empty current branch, not as the literal "HEAD". +func branchMismatchRefusal(expected, current string, baselinePublished bool) *pushRefusal { + code := pushBranchMismatchErrorCode + if baselinePublished { + code = pushBranchMismatchAfterBaselineErrorCode + } + shown := current + if shown == "" { + shown = "a detached HEAD" + } + return &pushRefusal{ + code: code, + message: fmt.Sprintf("expected to publish %q but the checkout is on %s", expected, shown), + expectedBranch: expected, + currentBranch: current, + baselinePublished: baselinePublished, + } +} + +// remoteConfig is a configured remote and its effective push URL set: the +// remote's push URLs when it has at least one, otherwise the single entry +// holding its fetch URL. A remote with neither has an empty set. +type remoteConfig struct { + name string + pushURLs []string +} + +// configuredRemotes reads every configured remote and its effective push URL +// set. A remote whose name fails the ref allowlist is skipped: it cannot be +// named by the name form either, so it is unreachable through this contract in +// both directions. +func (g *GitOperator) configuredRemotes(ctx context.Context) ([]remoteConfig, error) { + output, err := g.runGitCommand(ctx, "remote") + if err != nil { + return nil, fmt.Errorf("failed to list remotes: %w", err) + } + var remotes []remoteConfig + for _, line := range strings.Split(output, "\n") { + name := strings.TrimSpace(line) + if name == "" || !securityutil.IsValidBranchName(name) { + continue + } + urls, err := g.effectivePushURLs(ctx, name) + if err != nil { + return nil, err + } + remotes = append(remotes, remoteConfig{name: name, pushURLs: urls}) + } + return remotes, nil +} + +func (g *GitOperator) effectivePushURLs(ctx context.Context, name string) ([]string, error) { + pushURLs, err := g.remoteConfigValues(ctx, name, "pushurl") + if err != nil { + return nil, err + } + if len(pushURLs) > 0 { + return pushURLs, nil + } + return g.remoteConfigValues(ctx, name, "url") +} + +// remoteConfigValues reads one multi-valued remote config key. Git exits 1 when +// the key is simply unset, which is an empty set rather than a read failure. +func (g *GitOperator) remoteConfigValues(ctx context.Context, name, key string) ([]string, error) { + output, err := g.runGitCommand(ctx, "config", "--get-all", "remote."+name+"."+key) + if err != nil { + if isGitConfigKeyUnset(err) { + return nil, nil + } + return nil, fmt.Errorf("failed to read remote.%s.%s: %w", name, key, err) + } + var values []string + for _, line := range strings.Split(output, "\n") { + if trimmed := strings.TrimSpace(line); trimmed != "" { + values = append(values, trimmed) + } + } + return values, nil +} + +func isGitConfigKeyUnset(err error) bool { + var exitErr *exec.ExitError + return errors.As(err, &exitErr) && exitErr.ExitCode() == 1 +} + +// resolvePushTarget maps an explicit push target onto a configured remote name. +// Resolution never mutates configuration and never places a caller-supplied URL +// on a git command line: only the resolved name reaches a command. +func (g *GitOperator) resolvePushTarget(ctx context.Context, target string) (string, *pushRefusal) { + remotes, err := g.configuredRemotes(ctx) + if err != nil { + return "", &pushRefusal{ + code: pushRemoteConfigUnreadableErrorCode, + message: "failed to read the checkout's remote configuration", + } + } + // The permissive ref allowlist is the name/URL discriminator. A value read + // as a name still has to match an already-configured remote, so a + // permissive reading cannot reach a destination on its own. + if securityutil.IsValidBranchName(target) { + for _, remote := range remotes { + if remote.name == target { + return remote.name, nil + } + } + return "", &pushRefusal{ + code: pushRemoteNotFoundErrorCode, + message: fmt.Sprintf("no configured remote named %q", target), + } + } + return resolvePushTargetURL(remotes, target) +} + +// resolvePushTargetURL matches a URL against each remote's effective push URL +// set. Only a single-entry set matches: a remote that carries the value +// alongside other push URLs would publish to destinations the caller never +// named, so it is refused rather than matched. A caller that wants that +// fan-out names the remote instead. +func resolvePushTargetURL(remotes []remoteConfig, target string) (string, *pushRefusal) { + var matches []string + fanout := false + for _, remote := range remotes { + if len(remote.pushURLs) == 1 && remote.pushURLs[0] == target { + matches = append(matches, remote.name) + continue + } + for _, url := range remote.pushURLs { + if url == target { + fanout = true + break + } + } + } + if len(matches) > 0 { + // Several remotes can legitimately carry the same URL; they all address + // the same destination, so first by byte order is well defined. + sort.Strings(matches) + return matches[0], nil + } + if fanout { + return "", &pushRefusal{ + code: pushRemoteFanoutErrorCode, + message: "the requested push URL belongs to a remote that publishes to additional URLs; name the remote instead", + } + } + return "", &pushRefusal{ + code: pushRemoteURLUnmatchedErrorCode, + message: "no configured remote publishes to the requested URL", + } +} + +// currentBranch reads HEAD as a symbolic ref so that a detached HEAD, reported +// as an empty branch, is distinguishable from a branch literally named "HEAD". +// rev-parse --abbrev-ref returns that literal for a detached checkout and so +// cannot make this distinction. +func (g *GitOperator) currentBranch(ctx context.Context) (string, error) { + output, err := g.runGitCommand(ctx, "symbolic-ref", "HEAD") + if err == nil { + ref := strings.TrimSpace(output) + if !strings.HasPrefix(ref, "refs/heads/") { + return "", nil + } + return strings.TrimPrefix(ref, "refs/heads/"), nil + } + // symbolic-ref fails both for a detached HEAD and for an unreadable + // checkout. A resolvable HEAD commit distinguishes the two. + if _, headErr := g.runGitCommand(ctx, "rev-parse", "--verify", "HEAD"); headErr == nil { + return "", nil + } + return "", fmt.Errorf("failed to read HEAD: %w", err) +} + +// pushPlan is the destination a push or preflight resolved to, together with +// the refspec that reaches it. +type pushPlan struct { + remote string + branch string + refspec string + // explicit records that the request named a push target. A push reports + // the destination fields only in that case, so a request naming none keeps + // the result shape it has today. + explicit bool + // routed records that contribution routing selected the destination. + routed bool + // baselineEligible records that empty-remote first publication applies: + // the origin path with no contribution routing. + baselineEligible bool +} + +// contributionRouted reports whether a contribution binding selects the +// destination for this operator. +func (g *GitOperator) contributionRouted() bool { + return g.contributionDestination != nil || g.remoteContribution != nil +} + +// resolvePushPlan evaluates the refusal order from the contribution conflict +// through the first expected-branch verification, then yields the destination. +// Every refusal it returns is raised before any remote is contacted, so a +// refused request leaves the checkout, its refs, and every remote unchanged. +// requireConfiguredRemote is set by preflight, which answers whether a +// destination is writable and so must report a missing default remote. A push +// leaves that to the push command itself. +func (g *GitOperator) resolvePushPlan( + ctx context.Context, opts PushOptions, requireConfiguredRemote bool, +) (*pushPlan, *pushRefusal) { + if refusal := g.refuseIncompatibleOptions(opts); refusal != nil { + return nil, refusal + } + remote := "" + if opts.Remote != "" { + resolved, refusal := g.resolvePushTarget(ctx, opts.Remote) + if refusal != nil { + return nil, refusal + } + remote = resolved + } + if requireConfiguredRemote && remote == "" && !g.contributionRouted() { + if refusal := g.requireDefaultRemote(ctx); refusal != nil { + return nil, refusal + } + } + current, err := g.currentBranch(ctx) + if err != nil { + return nil, &pushRefusal{message: err.Error()} + } + if remote != "" && opts.ExpectedBranch == "" && current == "" { + return nil, &pushRefusal{ + code: pushBranchDetachedErrorCode, + message: "cannot publish a detached HEAD without an expected branch", + } + } + if opts.ExpectedBranch != "" && opts.ExpectedBranch != current { + return nil, branchMismatchRefusal(opts.ExpectedBranch, current, false) + } + return g.buildPushPlan(ctx, opts, remote, current) +} + +// refuseIncompatibleOptions covers the request-shape refusals, which are the +// cheapest and so are evaluated before any configuration or checkout read. +func (g *GitOperator) refuseIncompatibleOptions(opts PushOptions) *pushRefusal { + if opts.Remote != "" && g.contributionRouted() { + return &pushRefusal{ + code: pushRemoteContributionConflictErrorCode, + message: "an explicit push target cannot be combined with a configured contribution", + } + } + if opts.Remote != "" && opts.SetUpstream { + return &pushRefusal{ + code: pushRemoteUpstreamUnsupportedErrorCode, + message: "an explicit push target cannot set upstream tracking", + } + } + if opts.ExpectedBranch != "" && !securityutil.IsValidExpectedBranchName(opts.ExpectedBranch) { + return &pushRefusal{ + code: pushBranchInvalidErrorCode, + message: fmt.Sprintf("invalid expected branch %q", opts.ExpectedBranch), + } + } + return nil +} + +// buildPushPlan selects the destination and refspec. The contribution paths +// keep the refspec and branch read they use today; only the explicit-target +// path is new. +func (g *GitOperator) buildPushPlan(ctx context.Context, opts PushOptions, remote, current string) (*pushPlan, *pushRefusal) { + if remote != "" { + branch := opts.ExpectedBranch + if branch == "" { + branch = current + } + return &pushPlan{ + remote: remote, + branch: branch, + refspec: "HEAD:refs/heads/" + branch, + explicit: true, + baselineEligible: remote == defaultPushRemote, + }, nil + } + // The paths below name no explicit target. With no expected branch they + // must behave exactly as they do today, including reading the branch + // through getCurrentBranch. With an expected branch, that value is used + // directly instead of a separate read: verifyExpectedBranch confirms HEAD + // is still on it immediately before the push runs, and a refspec built + // from an earlier, independent read could name a branch that check never + // saw. + branch := opts.ExpectedBranch + if branch == "" { + var err error + branch, err = g.getCurrentBranch(ctx) + if err != nil { + return nil, &pushRefusal{message: err.Error()} + } + } + switch { + case g.contributionDestination != nil: + return &pushPlan{ + remote: g.contributionDestination.ContributionRemoteName(), + branch: branch, + refspec: branch, + routed: true, + }, nil + case g.remoteContribution != nil: + return &pushPlan{ + remote: g.remoteContribution.ContributionRemoteName(), + branch: g.remoteContribution.HeadBranch, + refspec: "HEAD:refs/heads/" + g.remoteContribution.HeadBranch, + routed: true, + }, nil + default: + return &pushPlan{ + remote: defaultPushRemote, + branch: branch, + refspec: branch, + baselineEligible: true, + }, nil + } +} + +// verifyExpectedBranch is the second verification. It is the last git read +// before the push command, so nothing Kandev issues can move HEAD between the +// check and the push. +func (g *GitOperator) verifyExpectedBranch(ctx context.Context, expected string, baselinePublished bool) *pushRefusal { + if expected == "" { + return nil + } + current, err := g.currentBranch(ctx) + if err != nil { + return &pushRefusal{message: err.Error(), baselinePublished: baselinePublished} + } + if current != expected { + return branchMismatchRefusal(expected, current, baselinePublished) + } + return nil +} + +// reportDestination populates the destination fields a successful push reports. +// A push that named no explicit target omits them so an existing consumer sees +// an unchanged result shape. +func (p *pushPlan) reportDestination(result *GitOperationResult) { + if !p.explicit { + return + } + result.PushedRemote = p.remote + result.PushedBranch = p.branch +} + +// requireDefaultRemote reports a checkout that has nowhere to publish, ahead of +// the branch guards so a caller learns that rather than a branch it cannot act +// on yet. +func (g *GitOperator) requireDefaultRemote(ctx context.Context) *pushRefusal { + remotes, err := g.configuredRemotes(ctx) + if err != nil { + return &pushRefusal{ + code: pushRemoteConfigUnreadableErrorCode, + message: "failed to read the checkout's remote configuration", + } + } + for _, remote := range remotes { + if remote.name == defaultPushRemote { + return nil + } + } + return &pushRefusal{ + code: pushNoRemoteConfiguredErrorCode, + message: "the checkout has no " + defaultPushRemote + " remote to publish to", + } +} diff --git a/apps/backend/internal/agentctl/server/process/git_push_target_test.go b/apps/backend/internal/agentctl/server/process/git_push_target_test.go new file mode 100644 index 0000000000..debd79a59e --- /dev/null +++ b/apps/backend/internal/agentctl/server/process/git_push_target_test.go @@ -0,0 +1,219 @@ +package process + +import ( + "context" + "os" + "path/filepath" + "testing" +) + +// setupPushTargetRepo builds a checkout with one commit on "feature/work" and +// no remotes. Tests add exactly the remotes they need. +func setupPushTargetRepo(t *testing.T) (string, *GitOperator) { + t.Helper() + root := t.TempDir() + repoDir := filepath.Join(root, "repo") + runGit(t, root, "init", "-b", "main", repoDir) + runGit(t, repoDir, "config", "user.name", "Task User") + runGit(t, repoDir, "config", "user.email", "task@example.com") + if err := os.WriteFile(filepath.Join(repoDir, "seed.txt"), []byte("seed\n"), 0o644); err != nil { + t.Fatal(err) + } + runGit(t, repoDir, "add", "seed.txt") + runGit(t, repoDir, "commit", "-m", "seed") + runGit(t, repoDir, "checkout", "-b", "feature/work") + + tracker := NewWorkspaceTracker(repoDir, newTestLogger(t)) + tracker.SetBaseBranch("main") + return repoDir, NewGitOperator(repoDir, newTestLogger(t), tracker) +} + +func TestResolvePushTargetTrimsAndTreatsEmptyAsAbsent(t *testing.T) { + for _, tc := range []struct { + name string + opts PushOptions + remote string + expected string + }{ + {name: "surrounding whitespace", opts: PushOptions{Remote: " backup ", ExpectedBranch: " main "}, remote: "backup", expected: "main"}, + {name: "whitespace only is absent", opts: PushOptions{Remote: " ", ExpectedBranch: "\t\n"}, remote: "", expected: ""}, + {name: "empty stays absent", opts: PushOptions{}, remote: "", expected: ""}, + } { + t.Run(tc.name, func(t *testing.T) { + got := tc.opts.normalized() + if got.Remote != tc.remote { + t.Errorf("Remote = %q, want %q", got.Remote, tc.remote) + } + if got.ExpectedBranch != tc.expected { + t.Errorf("ExpectedBranch = %q, want %q", got.ExpectedBranch, tc.expected) + } + }) + } +} + +func TestResolvePushTargetDiscriminatesNameFromURL(t *testing.T) { + repoDir, operator := setupPushTargetRepo(t) + runGit(t, repoDir, "remote", "add", "backup", "https://example.test/backup.git") + + name, refusal := operator.resolvePushTarget(context.Background(), "backup") + if refusal != nil { + t.Fatalf("name form refused: %+v", refusal) + } + if name != "backup" { + t.Errorf("name form resolved to %q, want %q", name, "backup") + } + + name, refusal = operator.resolvePushTarget(context.Background(), "https://example.test/backup.git") + if refusal != nil { + t.Fatalf("URL form refused: %+v", refusal) + } + if name != "backup" { + t.Errorf("URL form resolved to %q, want %q", name, "backup") + } +} + +func TestResolvePushTargetRejectsCaseAndPrefixVariants(t *testing.T) { + repoDir, operator := setupPushTargetRepo(t) + runGit(t, repoDir, "remote", "add", "backup", "https://example.test/backup.git") + + for _, target := range []string{"Backup", "BACKUP", "back", "backup2"} { + t.Run(target, func(t *testing.T) { + _, refusal := operator.resolvePushTarget(context.Background(), target) + if refusal == nil { + t.Fatalf("resolvePushTarget(%q) matched; want refusal", target) + } + if refusal.code != pushRemoteNotFoundErrorCode { + t.Errorf("code = %q, want %q", refusal.code, pushRemoteNotFoundErrorCode) + } + }) + } +} + +func TestResolvePushTargetReportsUnknownRemoteName(t *testing.T) { + _, operator := setupPushTargetRepo(t) + + _, refusal := operator.resolvePushTarget(context.Background(), "missing") + if refusal == nil || refusal.code != pushRemoteNotFoundErrorCode { + t.Fatalf("refusal = %+v, want %q", refusal, pushRemoteNotFoundErrorCode) + } +} + +func TestResolvePushTargetMatchesSingleEntryPushURL(t *testing.T) { + repoDir, operator := setupPushTargetRepo(t) + // pushurl shadows url, so the fetch URL must not match once a push URL exists. + runGit(t, repoDir, "remote", "add", "backup", "https://example.test/fetch.git") + runGit(t, repoDir, "config", "remote.backup.pushurl", "https://example.test/push.git") + + name, refusal := operator.resolvePushTarget(context.Background(), "https://example.test/push.git") + if refusal != nil { + t.Fatalf("push URL refused: %+v", refusal) + } + if name != "backup" { + t.Errorf("resolved %q, want %q", name, "backup") + } + + _, refusal = operator.resolvePushTarget(context.Background(), "https://example.test/fetch.git") + if refusal == nil || refusal.code != pushRemoteURLUnmatchedErrorCode { + t.Fatalf("shadowed fetch URL refusal = %+v, want %q", refusal, pushRemoteURLUnmatchedErrorCode) + } +} + +func TestResolvePushTargetPicksFirstByByteOrder(t *testing.T) { + repoDir, operator := setupPushTargetRepo(t) + shared := "https://example.test/shared.git" + runGit(t, repoDir, "remote", "add", "zulu", shared) + runGit(t, repoDir, "remote", "add", "alpha", shared) + runGit(t, repoDir, "remote", "add", "mike", shared) + + name, refusal := operator.resolvePushTarget(context.Background(), shared) + if refusal != nil { + t.Fatalf("shared URL refused: %+v", refusal) + } + if name != "alpha" { + t.Errorf("resolved %q, want %q", name, "alpha") + } +} + +func TestResolvePushTargetReportsUnmatchedURL(t *testing.T) { + repoDir, operator := setupPushTargetRepo(t) + runGit(t, repoDir, "remote", "add", "backup", "https://example.test/backup.git") + + _, refusal := operator.resolvePushTarget(context.Background(), "https://example.test/other.git") + if refusal == nil || refusal.code != pushRemoteURLUnmatchedErrorCode { + t.Fatalf("refusal = %+v, want %q", refusal, pushRemoteURLUnmatchedErrorCode) + } +} + +func TestResolvePushTargetReportsFanoutRemote(t *testing.T) { + repoDir, operator := setupPushTargetRepo(t) + runGit(t, repoDir, "remote", "add", "mirror", "https://example.test/a.git") + runGit(t, repoDir, "config", "--add", "remote.mirror.pushurl", "https://example.test/a.git") + runGit(t, repoDir, "config", "--add", "remote.mirror.pushurl", "https://example.test/b.git") + + _, refusal := operator.resolvePushTarget(context.Background(), "https://example.test/a.git") + if refusal == nil || refusal.code != pushRemoteFanoutErrorCode { + t.Fatalf("refusal = %+v, want %q", refusal, pushRemoteFanoutErrorCode) + } + + // A matching single-entry remote takes precedence over the fan-out refusal. + runGit(t, repoDir, "remote", "add", "direct", "https://example.test/a.git") + name, refusal := operator.resolvePushTarget(context.Background(), "https://example.test/a.git") + if refusal != nil { + t.Fatalf("single-entry remote refused: %+v", refusal) + } + if name != "direct" { + t.Errorf("resolved %q, want %q", name, "direct") + } +} + +func TestResolvePushTargetReportsUnreadableRemoteConfig(t *testing.T) { + repoDir, operator := setupPushTargetRepo(t) + runGit(t, repoDir, "remote", "add", "backup", "https://example.test/backup.git") + // A config file git cannot parse must not read as "no such remote". + if err := os.WriteFile(filepath.Join(repoDir, ".git", "config"), []byte("[remote \"backup\"\n"), 0o644); err != nil { + t.Fatal(err) + } + + _, refusal := operator.resolvePushTarget(context.Background(), "backup") + if refusal == nil || refusal.code != pushRemoteConfigUnreadableErrorCode { + t.Fatalf("refusal = %+v, want %q", refusal, pushRemoteConfigUnreadableErrorCode) + } +} + +func TestGitOperatorCurrentBranchReportsDetachedHeadAsEmpty(t *testing.T) { + repoDir, operator := setupPushTargetRepo(t) + + branch, err := operator.currentBranch(context.Background()) + if err != nil { + t.Fatalf("currentBranch() error = %v", err) + } + if branch != "feature/work" { + t.Errorf("branch = %q, want %q", branch, "feature/work") + } + + runGit(t, repoDir, "checkout", "--detach", "HEAD") + branch, err = operator.currentBranch(context.Background()) + if err != nil { + t.Fatalf("currentBranch() on detached HEAD error = %v", err) + } + if branch != "" { + t.Errorf("detached branch = %q, want empty", branch) + } +} + +// TestGitOperatorCurrentBranchReportsSymbolicRefOutsideHeadsAsEmpty covers a +// HEAD that resolves as a symbolic ref, but not under refs/heads/. Detached +// state is determined by whether HEAD resolves under refs/heads/, not merely +// by whether it fails to resolve at all. +func TestGitOperatorCurrentBranchReportsSymbolicRefOutsideHeadsAsEmpty(t *testing.T) { + repoDir, operator := setupPushTargetRepo(t) + + runGit(t, repoDir, "symbolic-ref", "HEAD", "refs/remotes/origin/main") + branch, err := operator.currentBranch(context.Background()) + if err != nil { + t.Fatalf("currentBranch() on a non-refs/heads symbolic ref error = %v", err) + } + if branch != "" { + t.Errorf("branch = %q, want empty: HEAD does not resolve under refs/heads/", branch) + } +} diff --git a/apps/backend/internal/agentctl/server/process/git_test.go b/apps/backend/internal/agentctl/server/process/git_test.go index 794c81fc07..c383175abd 100644 --- a/apps/backend/internal/agentctl/server/process/git_test.go +++ b/apps/backend/internal/agentctl/server/process/git_test.go @@ -83,7 +83,7 @@ func TestGitOperatorRemoteContributionRoutesPushesAndPreflightToSource(t *testin operator := NewGitOperator(repoDir, newTestLogger(t), nil) operator.setRemoteContribution(binding) - preflight, err := operator.PushPreflight(context.Background()) + preflight, err := operator.PushPreflight(context.Background(), PushOptions{}) if err != nil { t.Fatalf("PushPreflight returned error: %v", err) } @@ -93,8 +93,11 @@ func TestGitOperatorRemoteContributionRoutesPushesAndPreflightToSource(t *testin if got := strings.TrimSpace(runGit(t, originDir, "rev-parse", "refs/heads/feature/contribution")); got != headSHA { t.Fatalf("preflight changed source branch: %q != %q", got, headSHA) } + if preflight.PushedRemote != "" || preflight.PushedBranch != "" { + t.Fatalf("contribution-routed preflight = %+v, want PushedRemote and PushedBranch omitted", preflight) + } - forced, err := operator.Push(context.Background(), true, false) + forced, err := operator.Push(context.Background(), PushOptions{Force: true}) if err != nil { t.Fatalf("forced Push returned error: %v", err) } @@ -102,7 +105,7 @@ func TestGitOperatorRemoteContributionRoutesPushesAndPreflightToSource(t *testin t.Fatalf("forced Push = %+v, want a contribution force-push rejection", forced) } - pushed, err := operator.Push(context.Background(), false, false) + pushed, err := operator.Push(context.Background(), PushOptions{}) if err != nil { t.Fatalf("Push returned error: %v", err) } @@ -218,7 +221,7 @@ func TestGitOperatorContributionDestinationPushesWithoutChangingPullRemote(t *te operator := NewGitOperator(repoDir, newTestLogger(t), nil) operator.setContributionDestination(destination) - pushed, err := operator.Push(context.Background(), false, false) + pushed, err := operator.Push(context.Background(), PushOptions{}) if err != nil || !pushed.Success { t.Fatalf("Push = %+v, err = %v", pushed, err) } @@ -260,7 +263,7 @@ func TestGitOperatorPush_PreservesExistingUpstream(t *testing.T) { } gitOp := NewGitOperator(suffixedDir, log, nil) - result, err := gitOp.Push(context.Background(), false, false) + result, err := gitOp.Push(context.Background(), PushOptions{}) if err != nil { t.Fatalf("Push returned error: %v", err) } diff --git a/apps/backend/internal/common/securityutil/git.go b/apps/backend/internal/common/securityutil/git.go index ca8ec37cd0..20fa92280b 100644 --- a/apps/backend/internal/common/securityutil/git.go +++ b/apps/backend/internal/common/securityutil/git.go @@ -86,6 +86,21 @@ func IsValidDefaultBranchName(branch string) bool { return IsValidBaseBranchRef(branch) && !IsGitSymbolicRef(branch) } +// IsValidExpectedBranchName validates a caller-supplied expected branch: the +// IsValidBranchName allowlist plus git check-ref-format's rejection of a +// trailing "/" and consecutive "//", plus a reject on git's symbolic +// pseudo-refs. It deliberately does not route through IsValidBaseBranchRef, +// which strips an "origin/" prefix and so validates a different string than +// the caller supplied. An expected branch is used verbatim, both in the +// comparison against HEAD and in the push refspec, so the value that is +// checked must be the value that is used. +func IsValidExpectedBranchName(branch string) bool { + if strings.HasSuffix(branch, "/") || strings.Contains(branch, "//") { + return false + } + return IsValidBranchName(branch) && !IsGitSymbolicRef(branch) +} + // IsKnownSafeGitFlag returns true if the argument is a known safe git flag used by this codebase. // This prevents argument injection where user input could introduce malicious flags. // Only flags actually used by the Kandev codebase are whitelisted. @@ -111,7 +126,7 @@ func IsKnownSafeGitFlag(arg string) bool { "--format", "--format=", "--stat", "--shortstat", "--numstat", "-p", "-A", "--amend", "--allow-empty", "--soft", "--mixed", "--hard", "--cached", "--force", "--source=HEAD", "--staged", "--worktree", - "--dry-run", "--get-all", "--first-parent", "--is-ancestor", + "--dry-run", "--no-verify", "--get-all", "--first-parent", "--is-ancestor", "--refs", "--src-prefix=", "--dst-prefix=", } diff --git a/apps/backend/internal/common/securityutil/git_test.go b/apps/backend/internal/common/securityutil/git_test.go index 177cc8d35d..faa4d3f774 100644 --- a/apps/backend/internal/common/securityutil/git_test.go +++ b/apps/backend/internal/common/securityutil/git_test.go @@ -120,3 +120,37 @@ func TestIsKnownSafeGitFlagRejectsRebaseAndAbortVariants(t *testing.T) { } } } + +func TestIsValidExpectedBranchName(t *testing.T) { + for _, tc := range []struct { + branch string + want bool + }{ + {branch: "main", want: true}, + {branch: "feature/work", want: true}, + {branch: "release/v1.2.3", want: true}, + {branch: "", want: false}, + {branch: "HEAD", want: false}, + {branch: "ORIG_HEAD", want: false}, + {branch: "FETCH_HEAD", want: false}, + {branch: "MERGE_HEAD", want: false}, + {branch: "main/", want: false}, + {branch: "a//b", want: false}, + {branch: "a..b", want: false}, + {branch: "main.lock", want: false}, + {branch: "-flag", want: false}, + {branch: "has space", want: false}, + // A local branch may legally carry this name, and it is validated as + // itself rather than as the "main" IsValidBaseBranchRef would strip to. + {branch: "origin/main", want: true}, + // Distinguishes this validator from IsValidBaseBranchRef, which strips + // the prefix and then rejects the leading dash of the remainder. + {branch: "origin/-dash", want: true}, + } { + t.Run(tc.branch, func(t *testing.T) { + if got := IsValidExpectedBranchName(tc.branch); got != tc.want { + t.Errorf("IsValidExpectedBranchName(%q) = %v, want %v", tc.branch, got, tc.want) + } + }) + } +} diff --git a/docs/plans/configured-push-target/plan.md b/docs/plans/configured-push-target/plan.md new file mode 100644 index 0000000000..551e844e94 --- /dev/null +++ b/docs/plans/configured-push-target/plan.md @@ -0,0 +1,276 @@ +--- +created: 2026-09-08 +status: implemented +requirements: + - REQ-WORKSPACES-CONFIGURED-PUSH-TARGET-001 + - REQ-WORKSPACES-CONFIGURED-PUSH-TARGET-002 + - REQ-WORKSPACES-CONFIGURED-PUSH-TARGET-003 + - REQ-WORKSPACES-CONFIGURED-PUSH-TARGET-004 +system_design: + - ../../specs/workspaces/system-design/configured-push-target.md +legacy_specs: [] +--- + +# Implementation Plan: Configured Push Target + +## Overview + +The workspace push contract can publish only to `origin` or to a remote that a +contribution binding selects, and push preflight reports success without +contacting a remote at all on the ordinary path. This work package adds two +optional inputs to the push and push-preflight contracts, an explicit push +target and an expected branch, and makes preflight validate the destination it +would actually use. + +Implementation runs in four work orders. Task 01 lands the vocabulary the rest +of the package needs: the request options carried into the operator, the eleven +error codes, the five result fields, and the pure resolution and validation +helpers, with no behavior change. Task 02 rewires the push path onto those +helpers, which is where the refusal ordering and the two-point expected-branch +verification take effect. Task 04 carries the two inputs across the HTTP +surface, the runtime client, and the backend action handler; it touches files +disjoint from Task 02 and depends only on Task 01's signatures, so the two run +in the same wave. Task 03 lands last because preflight consumes the shared +resolution and guard call sites that Task 02 establishes. + +This order exists so that no work order changes a destination decision and its +transport in the same pass: the operator owns every new decision, and the +transport work carries values it does not interpret. + +## Scope + +### In scope + +- An optional explicit push target on the push and push-preflight contracts, + accepted as a configured remote name or as a remote URL resolved against + already-configured remotes. +- An optional expected branch on both contracts, verified twice on the push + path and once on preflight. +- The fixed refusal ordering and the eleven stable error codes. +- Push preflight validating the destination-branch refspec against the resolved + remote, including the `origin` path that reports blanket success today. +- The five optional result fields, with the push/preflight asymmetry the design + requires. +- Transport plumbing for both inputs across agentctl HTTP, the runtime client, + and the backend workspace Git action handler. + +### Out of scope + +- Exact-OID `--force-with-lease=:`. Force keeps emitting the bare + lease and stays refused under contribution routing. +- Auto-push policy. This package only makes such a caller expressible. +- Creating, renaming, retargeting, or deleting a remote. +- Any change to remote-contribution or contribution-destination routing, their + force refusal, or their existing preflight behavior. +- Any web, desktop, or mobile control. The existing push control keeps sending + neither input, so this package ships no user-visible surface. +- Preflighting the empty-remote baseline refspec. +- Pushing tags, multiple refspecs, or deleting a remote branch. + +## Technical approach + +### Operator primitives (`internal/agentctl/server/process`) + +A new `git_push_target.go` owns everything the resolution and guard need, so the +decision logic sits in one file rather than growing `Push`: + +- `PushOptions{Force, SetUpstream bool; Remote, ExpectedBranch string}` replaces + the two positional booleans on `GitOperator.Push` and is added to + `GitOperator.PushPreflight`, which takes no options today. +- Error-code constants for the eleven codes the design tables, alongside the + existing `emptyRemote*ErrorCode` constants. +- `configuredRemotes(ctx)` enumerates names with `git remote`, then reads each + name's `--get-all remote..pushurl` and `--get-all remote..url`. + The effective push URL set is the push URL list when non-empty, otherwise the + single-element fetch URL list, otherwise empty. A read failure yields + `push_remote_config_unreadable` rather than an absent or unmatched target. +- `resolvePushTarget` applies the trim, discriminates name from URL with + `securityutil.IsValidBranchName`, and returns a remote **name**. A + caller-supplied URL never reaches a Git command line, which is what keeps + `runGitCommand`'s existing argument allowlist unchanged. +- `currentBranch(ctx)` reads `git symbolic-ref HEAD` and trims `refs/heads/`. + A non-zero exit is the detached state, reported as an empty branch. This + replaces `getCurrentBranch`'s `rev-parse --abbrev-ref HEAD` on the new paths, + which returns the literal `HEAD` when detached and cannot satisfy + AC-WORKSPACES-CONFIGURED-PUSH-TARGET-002.11. `symbolic-ref HEAD` needs no + flag and `HEAD` is already a safe literal, so the allowlist is untouched. + +### Expected-branch validation (`internal/common/securityutil`) + +The strict allowlist the expected branch must pass is `IsValidBranchName` plus +a reject on a trailing `/`, on `//`, and on the four symbolic pseudo-refs. The +existing `IsValidDefaultBranchName` composes exactly those checks but reaches +them through `IsValidBaseBranchRef`, which first strips an `origin/` prefix, so +an expected branch of `origin/main` would validate as `main` and pass. A new +`IsValidExpectedBranchName` applies the same checks without that strip. + +### Push path (`Push`, `internal/agentctl/server/process/git.go`) + +`Push` gains a `resolvePushPlan` step that evaluates the whole refusal list +before any remote is contacted, then keeps the existing structure. Two ordering +changes are load-bearing: + +- Empty-remote first publication runs only when the resolved remote is `origin` + and no contribution routing applies. +- `getUpstreamRef` is read today after first publication and inside the argument + build. It moves ahead of the second expected-branch verification, so that the + branch read is the last Git command before the push + (AC-WORKSPACES-CONFIGURED-PUSH-TARGET-002.12). + +Refspec selection is `HEAD:refs/heads/` with an explicit target and an +expected branch, `HEAD:refs/heads/` with an explicit target alone, and +unchanged on every path that names no target. The `HEAD:refs/heads/` form is +already admitted by `runGitCommand`. + +### Preflight (`PushPreflight`) + +The contribution-destination and remote-contribution branches keep their current +bodies and result shape. The `g.remoteContribution == nil` early `result.Success += true` return is replaced by the resolved-remote dry-run, defaulting to +`origin`, reporting `push_no_remote_configured` when no explicit target, no +contribution routing, and no `origin` remote exist. A successful preflight to +which contribution routing did not apply reports `pushed_remote` and +`pushed_branch`. + +### Transport + +- `internal/agentctl/server/api/git.go`: `GitPushRequest` and + `GitPushPreflightRequest` gain `remote` and `expected_branch`, bound and + forwarded without interpretation. +- `internal/agent/runtime/agentctl/git.go`: both result structs gain the five + fields; `GitPush` and `GitPushPreflight` carry the two inputs. +- `internal/agent/handlers/git_handlers.go`: the `worktree.push` payload gains + both fields and forwards them. +- `internal/agent/runtime/lifecycle/manager_startup.go` keeps passing neither, + so the startup remote-contribution preflight is unchanged. + +### Output redaction + +`runGitCommand` already routes every `push` invocation, dry-run included, +through `sanitizeGitPushOutput`, so the redaction +AC-WORKSPACES-CONFIGURED-PUSH-TARGET-001.18 requires is satisfied by +construction on both new paths. No second sanitization is added; see +[Open questions](#open-questions). + +## Tests + +| Acceptance criteria | Evidence | +| --- | --- | +| 001.1, 004.7 | `TestResolvePushTargetTrimsAndTreatsEmptyAsAbsent` in `internal/agentctl/server/process/git_push_target_test.go` | +| 001.3, 001.15 | `TestResolvePushTargetDiscriminatesNameFromURL`, `TestResolvePushTargetRejectsCaseAndPrefixVariants` in `git_push_target_test.go` | +| 001.4 | `TestResolvePushTargetReportsUnknownRemoteName` in `git_push_target_test.go` | +| 001.5, 001.6 | `TestResolvePushTargetMatchesSingleEntryPushURL`, `TestResolvePushTargetPicksFirstByByteOrder` in `git_push_target_test.go` | +| 001.7 | `TestResolvePushTargetReportsUnmatchedURL` in `git_push_target_test.go` | +| 001.16 | `TestResolvePushTargetReportsUnreadableRemoteConfig` in `git_push_target_test.go` | +| 001.17 | `TestResolvePushTargetReportsFanoutRemote`, `TestGitOperatorPushNamedFanoutRemotePublishesToEveryPushURL` in `git_push_target_test.go` | +| 002.2 | `TestIsValidExpectedBranchName` in `internal/common/securityutil/git_test.go` | +| 002.11 | `TestGitOperatorCurrentBranchReportsDetachedHeadAsEmpty` in `git_push_target_test.go` | +| 001.2 | `TestGitOperatorPushWithoutOptionsIsUnchanged` in `internal/agentctl/server/process/git_push_expected_branch_test.go` | +| 001.8 | `TestGitOperatorPushNeverCreatesRemote` in `git_push_expected_branch_test.go` | +| 001.9, 001.10 | `TestGitOperatorPushRefusesTargetUnderContributionRouting`, `TestGitOperatorPushRefusesTargetWithSetUpstream` in `git_push_expected_branch_test.go` | +| 001.11, 001.12 | `TestGitOperatorPushToNamedRemoteLeavesUpstreamUnset`, `TestGitOperatorPushToNamedRemoteSkipsBaselinePublication` in `git_push_expected_branch_test.go` | +| 001.13 | `TestGitOperatorPushToNamedRemoteStaysNonForce` in `git_push_expected_branch_test.go` | +| 001.14 | `TestGitOperatorPushResultAndLogsCarryNoRemoteURL` in `git_push_expected_branch_test.go` | +| 001.18 | `TestGitOperatorPushRedactsCredentialsOnExplicitTargetPath` in `git_push_expected_branch_test.go`; `TestPushPreflightRedactsCredentialsInRemoteOutput` in `git_push_preflight_test.go` | +| 002.1, 002.9 | `TestGitOperatorPushExpectedBranchAloneOnlyGates` in `git_push_expected_branch_test.go` | +| 002.3, 002.4 | `TestGitOperatorPushRefusesExpectedBranchMismatch`, `TestGitOperatorPushRefusesDetachedHeadAsMismatch` in `git_push_expected_branch_test.go` | +| 002.5, 002.12 | `TestGitOperatorPushVerifiesBranchAsLastReadBeforePush` in `git_push_expected_branch_test.go` | +| 002.6 | `TestGitOperatorPushReportsMismatchAfterBaselinePublication` in `git_push_expected_branch_test.go` | +| 002.7, 002.8, 002.10 | `TestGitOperatorPushPublishesHeadToExpectedBranch`, `TestGitOperatorPushRefusesDetachedHeadWithTargetOnly`, `TestGitOperatorPushCreatesMissingDestinationBranch` in `git_push_expected_branch_test.go` | +| 003.1, 003.7 | `TestPushPreflightAppliesSameRefusalOrdering`, `TestPushPreflightReportsValidatedRemoteAndBranch` in `internal/agentctl/server/process/git_push_preflight_test.go` | +| 003.2, 003.3 | `TestPushPreflightValidatesNamedRemote`, `TestPushPreflightValidatesOriginWithoutTarget` in `git_push_preflight_test.go` | +| 003.4 | `TestPushPreflightReportsNoRemoteConfigured` in `git_push_preflight_test.go` | +| 003.5 | `TestPushPreflightMutatesNothing` in `git_push_preflight_test.go` | +| 003.6 | `TestPushPreflightReportsRemoteRefusalOutput` in `git_push_preflight_test.go` | +| 004.1, 004.2 | `TestGitOperatorPushRefusalOrdering`, `TestGitOperatorPushRefusalsAreNoOps` in `git_push_expected_branch_test.go` | +| 004.3 | `TestGitOperatorPushReportsOperationInProgress` in `git_push_expected_branch_test.go` | +| 004.4, 004.5 | `TestGitOperatorPushRepeatedIdenticalRequestSucceeds`, `TestGitOperatorPushDoesNotEscalateRejectedNonForcePush` in `git_push_expected_branch_test.go` | +| 004.6 | `TestGitOperatorPushOmitsTargetFieldsWithoutExplicitTarget` in `git_push_expected_branch_test.go` | +| 004.8 | `TestGitPushRequestScopesTargetToSelectedRepo` in `internal/agentctl/server/api/git_handlers_test.go` | +| 001.1, 002.1 transport | `TestHandleGitPushBindsRemoteAndExpectedBranch` in `internal/agentctl/server/api/git_handlers_test.go`; `TestGitPushSendsPushOptions` in `internal/agent/runtime/agentctl/git_test.go`; `TestWsPushForwardsPushOptions` in `internal/agent/handlers/git_handlers_test.go` | + +## E2E tests + +None. The design places every web, desktop, and mobile surface out of scope, +and the existing push control keeps sending neither input, so this package adds +no user-visible behavior for a browser test to exercise. Evidence is the Go +unit and integration coverage above, which drives real Git repositories through +the existing `setupEmptyRemoteTaskRepo` fixture pattern. + +## Work orders + +- [x] [Task 01: Push-target and expected-branch primitives](task-01-resolution-primitives.md) +- [x] [Task 02: Explicit push target and expected-branch verification on the push path](task-02-push-path.md) +- [x] [Task 03: Push preflight validates the resolved destination](task-03-preflight.md) +- [x] [Task 04: Carry both inputs across the push transport](task-04-transport.md) + +Dependency order: Task 01, then Tasks 02 and 04 together, then Task 03. + +## Verification results + +All four work orders are implemented and verified on the neo SSH runner. + +- `make -C apps/backend lint` — 0 issues. +- `go test -tags fts5 ./internal/agentctl/server/process/...` — all push, preflight, + resolution and empty-remote tests pass. +- `go test -tags fts5 ./internal/agentctl/server/api/... ./internal/agent/runtime/agentctl/ + ./internal/agent/handlers/... ./internal/agent/runtime/lifecycle/... + ./internal/common/securityutil/...` — all new and existing push tests pass. + +Four tests fail identically on this runner both with these changes and on a +clean worktree at `HEAD`, so they are pre-existing and environment-specific, not +regressions: `TestRepairManagedRuntimeCacheClearsPreviousStderr`, +`TestManagedRuntimeCacheRepairUsesAgentEnvironmentAndExactTree`, the Kubernetes +PVC and `WorktreePreparer` cases in `internal/agent/runtime/lifecycle`, and +`TestBuildAuthMethodsIdentityAgentOverridesEnvironment`. +`TestWorkspaceGitAdmissionWaitDoesNotConsumeTimeout` is load-sensitive and fails +under parallel suite runs on clean `HEAD` too. + +Two ordering guarantees are proved directly rather than by inspection. A PATH +shim records every git invocation, so +`TestGitOperatorPushVerifiesBranchAsLastReadBeforePush` asserts that +`symbolic-ref HEAD` is the command immediately preceding `push` on both the +explicit-target and default paths. A second shim moves `HEAD` during +empty-remote first publication, so +`TestGitOperatorPushReportsMismatchAfterBaselinePublication` exercises the +second-verification window that the first verification cannot cover. + +## Risks + +- `Push` is already long and carries contribution routing, force refusal, and + empty-remote publication. Adding resolution and two verification points + in-line would breach the backend function-length limit; the plan therefore + puts resolution and the guard in `git_push_target.go` and has `Push` call a + single `resolvePushPlan`. +- Moving `getUpstreamRef` ahead of the second verification changes the order of + existing Git reads on the path that names no target. The existing + `git_empty_remote_test.go` cases are the regression net for that path and must + stay green unmodified. +- Replacing preflight's blanket `result.Success = true` makes a previously + inert call contact `origin`. The startup remote-contribution preflight is not + affected because it runs only when a binding exists, but any future caller on + the ordinary path now performs network I/O. +- A configured remote whose name fails `IsValidBranchName` cannot be addressed + by the name form and is skipped during URL matching. This is consistent in + both directions, but it means such a remote is unreachable through this + contract. +- The five result fields are `omitempty`, so `current_branch` reported as empty + for a detached `HEAD` is omitted from the JSON rather than present and empty. + Consumers must read a missing key as the detached state. + +## Resolved questions + +- The system design's Security section claimed `Push` assigns Git's output to + the result without sanitizing it. `runGitCommand` in fact sanitizes every + invocation whose subcommand is `push`, dry-run included, so + AC-WORKSPACES-CONFIGURED-PUSH-TARGET-001.18 was already satisfied on both new + paths and no new redaction call was needed. The design's Security section has + been corrected to describe the existing behavior, and + `TestGitOperatorPushRedactsCredentialsOnExplicitTargetPath` plus + `TestPushPreflightRedactsCredentialsInRemoteOutput` guard it against + regression. +- The plan proposed `IsValidExpectedBranchName` because + `IsValidDefaultBranchName` strips an `origin/` prefix. Implementation + confirmed the two disagree on inputs such as `origin/-dash`, and the new + validator checks the value that is actually used in the comparison and the + refspec. `TestIsValidExpectedBranchName` pins the distinguishing case. diff --git a/docs/plans/configured-push-target/task-01-resolution-primitives.md b/docs/plans/configured-push-target/task-01-resolution-primitives.md new file mode 100644 index 0000000000..daa2a70eff --- /dev/null +++ b/docs/plans/configured-push-target/task-01-resolution-primitives.md @@ -0,0 +1,141 @@ +--- +id: "01-resolution-primitives" +title: "Push-target and expected-branch primitives" +status: completed +wave: 1 +depends_on: [] +plan: "plan.md" +requirements: + - REQ-WORKSPACES-CONFIGURED-PUSH-TARGET-001 + - REQ-WORKSPACES-CONFIGURED-PUSH-TARGET-002 + - REQ-WORKSPACES-CONFIGURED-PUSH-TARGET-004 +acceptance_criteria: + - AC-WORKSPACES-CONFIGURED-PUSH-TARGET-001.3 + - AC-WORKSPACES-CONFIGURED-PUSH-TARGET-001.4 + - AC-WORKSPACES-CONFIGURED-PUSH-TARGET-001.5 + - AC-WORKSPACES-CONFIGURED-PUSH-TARGET-001.6 + - AC-WORKSPACES-CONFIGURED-PUSH-TARGET-001.7 + - AC-WORKSPACES-CONFIGURED-PUSH-TARGET-001.15 + - AC-WORKSPACES-CONFIGURED-PUSH-TARGET-001.16 + - AC-WORKSPACES-CONFIGURED-PUSH-TARGET-001.17 + - AC-WORKSPACES-CONFIGURED-PUSH-TARGET-002.2 + - AC-WORKSPACES-CONFIGURED-PUSH-TARGET-002.11 + - AC-WORKSPACES-CONFIGURED-PUSH-TARGET-004.7 +system_design: + - ../../specs/workspaces/system-design/configured-push-target.md +--- + +# Task 01: Push-target and expected-branch primitives + +## Summary + +Add the vocabulary and pure helpers the rest of the package builds on: a +`PushOptions` request struct, the eleven error-code constants, the five result +fields, remote enumeration, push-target resolution, strict expected-branch +validation, and a symbolic-ref current-branch read. Push and preflight take the +new options and ignore them, so this work order changes no observable behavior. + +## In scope + +- `PushOptions{Force, SetUpstream bool; Remote, ExpectedBranch string}` in + `internal/agentctl/server/process`, threaded through `GitOperator.Push` and + `GitOperator.PushPreflight` as a pass-through with behavior unchanged. +- The eleven error-code constants from the system design's table. +- `PushedRemote`, `PushedBranch`, `ExpectedBranch`, `CurrentBranch`, and + `BaselinePublished` on `process.GitOperationResult`, all `omitempty`. +- `configuredRemotes`, which reads each remote's effective push URL set and + distinguishes a genuine read failure from an absent remote. +- `resolvePushTarget`, returning a remote name or one of the four resolution + refusals, and never placing a caller-supplied URL on a Git command line. +- `currentBranch`, reading `git symbolic-ref HEAD` so a detached `HEAD` is an + empty branch rather than the literal `HEAD`. +- `securityutil.IsValidExpectedBranchName`, the strict allowlist without + `IsValidBaseBranchRef`'s `origin/` prefix strip. + +## Out of scope + +- Any change to how `Push` or `PushPreflight` behave. The options are accepted + and ignored here; Tasks 02 and 03 own the behavior. +- The runtime client and backend handler result structs, which Task 04 owns. +- Widening `runGitCommand`'s argument allowlist. `symbolic-ref HEAD` and + `config --get-all` are already admitted; if an implementation needs a flag + that is not, change the approach rather than the allowlist. + +## Acceptance + +- `resolvePushTarget` returns the resolved remote name for the name form and + the single-entry URL form, `push_remote_fanout` for a multi-push-URL remote + carrying the value, `push_remote_url_unmatched` when nothing carries it, + `push_remote_not_found` for an unknown name, and + `push_remote_config_unreadable` when the remote configuration cannot be read. +- `currentBranch` returns the branch for an attached `HEAD` and an empty string + with no error for a detached one, distinguished from a real read failure. +- `go build ./...` and the existing process and securityutil suites pass with + `Push` and `PushPreflight` behavior unchanged. + +## Verification + +```bash +cd apps/backend && go test -tags fts5 ./internal/agentctl/server/process/... ./internal/common/securityutil/... +cd apps/backend && make lint +``` + +## Files likely touched + +- `apps/backend/internal/agentctl/server/process/git_push_target.go` (new) +- `apps/backend/internal/agentctl/server/process/git_push_target_test.go` (new) +- `apps/backend/internal/agentctl/server/process/git.go` +- `apps/backend/internal/common/securityutil/git.go` +- `apps/backend/internal/common/securityutil/git_test.go` +- `apps/backend/internal/agentctl/server/api/git.go` (call-site signature only) + +## Dependencies + +None. + +## Risks + +- `IsValidDefaultBranchName` looks like the right validator but strips an + `origin/` prefix, so reusing it would let `origin/main` validate as `main`. + The new validator must not route through `IsValidBaseBranchRef`. +- Changing `Push`'s signature touches its existing call site in + `internal/agentctl/server/api/git.go`. Keep that edit mechanical; Task 04 + owns the request-field work there. +- A remote name that fails `IsValidBranchName` cannot be safely interpolated + into `remote..url`. Skip such remotes during URL matching rather than + failing the whole enumeration. + +## Parallelism + +`sequential` + +## Inputs + +- Requirements REQ-WORKSPACES-CONFIGURED-PUSH-TARGET-001 and -002, and the + system design's [Push-target resolution](../../specs/workspaces/system-design/configured-push-target.md) + and [Data and contracts](../../specs/workspaces/system-design/configured-push-target.md) sections. +- `internal/agentctl/server/process/git.go` `runGitCommand` argument allowlist + and `getCurrentBranch`. +- `internal/common/securityutil/git.go` existing validators. +- `setupEmptyRemoteTaskRepo` in `git_empty_remote_test.go` as the fixture + pattern for real-repository tests. + +## Results + +Implemented. `PushOptions`, the eleven error-code constants, the five result +fields, `configuredRemotes`, `resolvePushTarget`, and `currentBranch` live in +`apps/backend/internal/agentctl/server/process/git_push_target.go`; +`securityutil.IsValidExpectedBranchName` lives alongside the existing +validators. `Push` and `PushPreflight` take `PushOptions` as a pass-through, so +this work order changed no observable behavior. + +`IsValidBaseBranchRef`'s `origin/` strip was confirmed to matter: it and the new +validator disagree on inputs such as `origin/-dash`, so the new validator checks +the value that is actually compared against `HEAD` and used in the refspec. + +`currentBranch` distinguishes a detached `HEAD` from an unreadable checkout by +falling back to `rev-parse --verify HEAD` only when `symbolic-ref` fails, so a +detached checkout reports an empty branch and a broken one still errors. + +Verified with `go test -tags fts5 ./internal/agentctl/server/process/... +./internal/common/securityutil/...` and `make -C apps/backend lint`. diff --git a/docs/plans/configured-push-target/task-02-push-path.md b/docs/plans/configured-push-target/task-02-push-path.md new file mode 100644 index 0000000000..f1003b573d --- /dev/null +++ b/docs/plans/configured-push-target/task-02-push-path.md @@ -0,0 +1,155 @@ +--- +id: "02-push-path" +title: "Explicit push target and expected-branch verification on the push path" +status: completed +wave: 2 +depends_on: ["01-resolution-primitives"] +plan: "plan.md" +requirements: + - REQ-WORKSPACES-CONFIGURED-PUSH-TARGET-001 + - REQ-WORKSPACES-CONFIGURED-PUSH-TARGET-002 + - REQ-WORKSPACES-CONFIGURED-PUSH-TARGET-004 +acceptance_criteria: + - AC-WORKSPACES-CONFIGURED-PUSH-TARGET-001.2 + - AC-WORKSPACES-CONFIGURED-PUSH-TARGET-001.8 + - AC-WORKSPACES-CONFIGURED-PUSH-TARGET-001.9 + - AC-WORKSPACES-CONFIGURED-PUSH-TARGET-001.10 + - AC-WORKSPACES-CONFIGURED-PUSH-TARGET-001.11 + - AC-WORKSPACES-CONFIGURED-PUSH-TARGET-001.12 + - AC-WORKSPACES-CONFIGURED-PUSH-TARGET-001.13 + - AC-WORKSPACES-CONFIGURED-PUSH-TARGET-001.14 + - AC-WORKSPACES-CONFIGURED-PUSH-TARGET-001.18 + - AC-WORKSPACES-CONFIGURED-PUSH-TARGET-002.3 + - AC-WORKSPACES-CONFIGURED-PUSH-TARGET-002.4 + - AC-WORKSPACES-CONFIGURED-PUSH-TARGET-002.5 + - AC-WORKSPACES-CONFIGURED-PUSH-TARGET-002.6 + - AC-WORKSPACES-CONFIGURED-PUSH-TARGET-002.7 + - AC-WORKSPACES-CONFIGURED-PUSH-TARGET-002.8 + - AC-WORKSPACES-CONFIGURED-PUSH-TARGET-002.9 + - AC-WORKSPACES-CONFIGURED-PUSH-TARGET-002.10 + - AC-WORKSPACES-CONFIGURED-PUSH-TARGET-002.12 + - AC-WORKSPACES-CONFIGURED-PUSH-TARGET-004.1 + - AC-WORKSPACES-CONFIGURED-PUSH-TARGET-004.2 + - AC-WORKSPACES-CONFIGURED-PUSH-TARGET-004.3 + - AC-WORKSPACES-CONFIGURED-PUSH-TARGET-004.4 + - AC-WORKSPACES-CONFIGURED-PUSH-TARGET-004.5 + - AC-WORKSPACES-CONFIGURED-PUSH-TARGET-004.6 +system_design: + - ../../specs/workspaces/system-design/configured-push-target.md +--- + +# Task 02: Explicit push target and expected-branch verification on the push path + +## Summary + +Wire `GitOperator.Push` onto Task 01's primitives: evaluate the fixed refusal +list before contacting any remote, skip empty-remote first publication for a +resolved remote other than `origin`, and verify the expected branch twice, the +second time as the last Git read before the push command. A request that names +neither input performs the same push it performs today. + +## In scope + +- A `resolvePushPlan` helper that evaluates the refusal list in the design's + fixed order and yields the resolved remote, the refspec, and the + set-upstream decision. +- Refusing `push_remote_contribution_conflict` and + `push_remote_upstream_unsupported` before any remote read. +- Restricting empty-remote first publication to the `origin` path. +- Moving `getUpstreamRef` ahead of the second verification so the branch read is + the last Git command before the push. +- The second verification, reporting `push_branch_mismatch_after_baseline` with + `baseline_published` set when first publication published a baseline in this + request, and `push_branch_mismatch` otherwise. +- Refspec selection: `HEAD:refs/heads/` with a target and an expected + branch, `HEAD:refs/heads/` with a target alone, unchanged otherwise. +- Populating `pushed_remote` and `pushed_branch` on a successful push only when + the request carried an explicit target. +- Extending the push completion log with the resolved remote, destination + branch, and whether an expected branch was supplied and matched. + +## Out of scope + +- `PushPreflight`, which Task 03 owns even though it reuses `resolvePushPlan`. +- Transport plumbing, which Task 04 owns. +- Contribution routing behavior, its force refusal, and the empty-remote + publication mechanics themselves, all of which stay as they are. +- Adding a second output sanitization. `runGitCommand` already redacts every + `push` invocation. + +## Acceptance + +- A push naming a configured remote other than `origin` publishes + `HEAD:refs/heads/` to it, sets no upstream, and publishes no baseline. +- An expected branch that does not match the current branch, including a + detached `HEAD`, refuses the push with the expected and current branches + reported and no remote contacted. +- A request naming neither input produces the same Git commands and the same + result shape as before this task, with `git_empty_remote_test.go` unmodified + and green. + +## Verification + +```bash +cd apps/backend && go test -tags fts5 ./internal/agentctl/server/process/... +cd apps/backend && make lint +``` + +## Files likely touched + +- `apps/backend/internal/agentctl/server/process/git.go` +- `apps/backend/internal/agentctl/server/process/git_push_target.go` +- `apps/backend/internal/agentctl/server/process/git_push_expected_branch_test.go` (new) + +## Dependencies + +Task 01. + +## Risks + +- `Push` is near the backend function-length limit already. Keep + `resolvePushPlan` and the second verification in `git_push_target.go` rather + than growing `Push` in place. +- The second verification can follow a completed baseline publication, so the + refusal is deliberately not side-effect-free. Do not add a rollback; report + `baseline_published` instead. +- Reordering the upstream read is observable to any test that asserts Git + command sequence on the `origin` path. Existing empty-remote tests must pass + unmodified; if one fails, the reorder is wrong rather than the test. + +## Parallelism + +`parallel-safe` + +## Inputs + +- The system design's [Control flow](../../specs/workspaces/system-design/configured-push-target.md) + and [Refusal ordering](../../specs/workspaces/system-design/configured-push-target.md) sections. +- `Push` at `internal/agentctl/server/process/git.go`, and + `prepareEmptyRemotePublication` in `git_empty_remote.go`. +- `git_empty_remote_test.go` as the regression net for the unchanged path. + +## Results + +Implemented. `Push` now calls `validateContributionState`, then +`resolvePushPlan`, which evaluates the whole refusal list before any remote is +contacted. Empty-remote first publication is gated on `plan.baselineEligible`, +so it runs only for `origin` with no contribution routing. `resolveSetUpstream` +moves the upstream read ahead of the second verification and skips it entirely +on the explicit-target path. + +The ordering guarantee is proved rather than asserted: a PATH shim records every +git invocation, and `TestGitOperatorPushVerifiesBranchAsLastReadBeforePush` +checks that `symbolic-ref HEAD` immediately precedes `push` on both the +explicit-target and default paths. A second shim moves `HEAD` during first +publication so +`TestGitOperatorPushReportsMismatchAfterBaselinePublication` exercises the +post-baseline refusal, confirming `push_branch_mismatch_after_baseline`, +`baseline_published`, and that the task branch stays unpublished while the +baseline remains. + +The existing `git_empty_remote_test.go` cases pass unmodified, which is the +regression evidence that the path naming no target is unchanged. + +Verified with `go test -tags fts5 ./internal/agentctl/server/process/...` and +`make -C apps/backend lint`. diff --git a/docs/plans/configured-push-target/task-03-preflight.md b/docs/plans/configured-push-target/task-03-preflight.md new file mode 100644 index 0000000000..15694109ec --- /dev/null +++ b/docs/plans/configured-push-target/task-03-preflight.md @@ -0,0 +1,121 @@ +--- +id: "03-preflight" +title: "Push preflight validates the resolved destination" +status: completed +wave: 3 +depends_on: ["02-push-path"] +plan: "plan.md" +requirements: + - REQ-WORKSPACES-CONFIGURED-PUSH-TARGET-003 +acceptance_criteria: + - AC-WORKSPACES-CONFIGURED-PUSH-TARGET-003.1 + - AC-WORKSPACES-CONFIGURED-PUSH-TARGET-003.2 + - AC-WORKSPACES-CONFIGURED-PUSH-TARGET-003.3 + - AC-WORKSPACES-CONFIGURED-PUSH-TARGET-003.4 + - AC-WORKSPACES-CONFIGURED-PUSH-TARGET-003.5 + - AC-WORKSPACES-CONFIGURED-PUSH-TARGET-003.6 + - AC-WORKSPACES-CONFIGURED-PUSH-TARGET-003.7 +system_design: + - ../../specs/workspaces/system-design/configured-push-target.md +--- + +# Task 03: Push preflight validates the resolved destination + +## Summary + +Replace push preflight's blanket success on the ordinary path with a real +dry-run against the destination the equivalent push would use. Preflight reuses +Task 02's resolution, guard, and refusal ordering, verifies the expected branch +once, and reports the remote and branch it validated whenever contribution +routing did not apply. + +## In scope + +- Replacing the `g.remoteContribution == nil` early `result.Success = true` + return with a resolved-remote dry-run, defaulting to `origin`. +- Reporting `push_no_remote_configured` when there is no explicit target, no + contribution routing, and no `origin` remote. +- Applying the same refusal ordering and codes as the push path, with a single + expected-branch verification and no reachable + `push_branch_mismatch_after_baseline`. +- Reporting `pushed_remote` and `pushed_branch` on every successful preflight to + which contribution routing did not apply, with or without an explicit target. +- Reporting a remote's write refusal as a failure carrying the remote's output + rather than an unhandled error. + +## Out of scope + +- The contribution-destination and remote-contribution preflight branches, + which keep their bodies, their result shape, and their output handling. +- Preflighting the empty-remote baseline refspec. Preflight exercises the + destination-branch refspec only. +- Any change to the startup remote-contribution preflight caller in + `internal/agent/runtime/lifecycle/manager_startup.go`. + +## Acceptance + +- Preflight with no explicit target and no contribution routing dry-runs against + `origin` for the current branch and reports the remote and branch it checked. +- Preflight against a remote that refuses the write reports failure with the + remote's output, and creates, moves, updates, or deletes no ref anywhere. +- A checkout with no `origin` and no contribution routing reports + `push_no_remote_configured` instead of success. + +## Verification + +```bash +cd apps/backend && go test -tags fts5 ./internal/agentctl/server/process/... ./internal/agent/runtime/lifecycle/... +cd apps/backend && make lint +``` + +## Files likely touched + +- `apps/backend/internal/agentctl/server/process/git.go` +- `apps/backend/internal/agentctl/server/process/git_push_target.go` +- `apps/backend/internal/agentctl/server/process/git_push_preflight_test.go` (new) + +## Dependencies + +Task 02, whose `resolvePushPlan` this reuses. + +## Risks + +- This turns a previously inert call into one that contacts a remote. The + startup caller is unaffected because it runs only when a contribution binding + exists, but the behavior change is real for any other caller on that path. +- A dry-run is still a `push` subcommand, so it must not be allowed to mutate. + Assert absence of ref movement directly in the test rather than trusting + `--dry-run`. + +## Parallelism + +`sequential` + +## Inputs + +- Requirement REQ-WORKSPACES-CONFIGURED-PUSH-TARGET-003 and the system design's + [Preflight](../../specs/workspaces/system-design/configured-push-target.md) section. +- `PushPreflight` at `internal/agentctl/server/process/git.go`. +- The startup caller at `internal/agent/runtime/lifecycle/manager_startup.go`. + +## Results + +Implemented. `PushPreflight` reuses `validateContributionState` and +`resolvePushPlan`, passing `requireConfiguredRemote: true` so a checkout with no +`origin` and no contribution routing reports `push_no_remote_configured` instead +of success. The blanket `result.Success = true` return is gone; every +non-routed preflight now dry-runs the destination-branch refspec and reports the +remote and branch it validated. A preflight under contribution routing keeps its +existing body and result shape. + +Preflight verifies the expected branch once, through `resolvePushPlan`, and +never calls `verifyExpectedBranch`, so `push_branch_mismatch_after_baseline` is +unreachable from preflight. `TestPushPreflightAppliesSameRefusalOrdering` +asserts that directly. + +`TestPushPreflightMutatesNothing` checks `HEAD`, the full local ref list, +upstream configuration, and both remotes across a preflight rather than trusting +`--dry-run`. + +Verified with `go test -tags fts5 ./internal/agentctl/server/process/... +./internal/agent/runtime/lifecycle/...` and `make -C apps/backend lint`. diff --git a/docs/plans/configured-push-target/task-04-transport.md b/docs/plans/configured-push-target/task-04-transport.md new file mode 100644 index 0000000000..39ebbdbc52 --- /dev/null +++ b/docs/plans/configured-push-target/task-04-transport.md @@ -0,0 +1,123 @@ +--- +id: "04-transport" +title: "Carry both inputs across the push transport" +status: completed +wave: 2 +depends_on: ["01-resolution-primitives"] +plan: "plan.md" +requirements: + - REQ-WORKSPACES-CONFIGURED-PUSH-TARGET-001 + - REQ-WORKSPACES-CONFIGURED-PUSH-TARGET-002 + - REQ-WORKSPACES-CONFIGURED-PUSH-TARGET-004 +acceptance_criteria: + - AC-WORKSPACES-CONFIGURED-PUSH-TARGET-001.1 + - AC-WORKSPACES-CONFIGURED-PUSH-TARGET-002.1 + - AC-WORKSPACES-CONFIGURED-PUSH-TARGET-004.8 +system_design: + - ../../specs/workspaces/system-design/configured-push-target.md +--- + +# Task 04: Carry both inputs across the push transport + +## Summary + +Carry the explicit push target and the expected branch from the backend +workspace Git action handler, through the runtime client, to the agentctl HTTP +surface, and add the five result fields to the client-side result struct. Every +layer passes both values through without interpreting them; all destination +logic stays in the operator. + +## In scope + +- `remote` and `expected_branch` on `GitPushRequest` and + `GitPushPreflightRequest` in `internal/agentctl/server/api/git.go`, bound and + forwarded into `PushOptions`. +- The five result fields on `client.GitOperationResult` in + `internal/agent/runtime/agentctl/git.go`, matching the server struct's JSON + keys. +- Both inputs on `Client.GitPush` and `Client.GitPushPreflight`, carried in the + request payloads. +- Both fields on the `worktree.push` payload in + `internal/agent/handlers/git_handlers.go`, forwarded to the client. +- Keeping the startup preflight caller in + `internal/agent/runtime/lifecycle/manager_startup.go` passing neither. + +## Out of scope + +- Any validation, resolution, or refusal decision at any transport layer. The + HTTP surface performs no destination logic of its own. +- Any web, desktop, or mobile control. The existing push control keeps sending + neither input. +- Operator behavior, which Tasks 02 and 03 own. + +## Acceptance + +- A push request carrying `remote` and `expected_branch` reaches + `GitOperator.Push` with both values intact, and a request carrying neither + produces a payload and behavior identical to today's. +- Both values are scoped to the repository the request already selects via + `repo` and affect no other repository in a multi-repository task. +- The client result struct deserializes all five new fields. + +## Verification + +```bash +cd apps/backend && go test -tags fts5 ./internal/agentctl/server/api/... ./internal/agent/runtime/agentctl/... ./internal/agent/handlers/... ./internal/agent/runtime/lifecycle/... +cd apps/backend && make lint +``` + +## Files likely touched + +- `apps/backend/internal/agentctl/server/api/git.go` +- `apps/backend/internal/agentctl/server/api/git_handlers_test.go` +- `apps/backend/internal/agent/runtime/agentctl/git.go` +- `apps/backend/internal/agent/runtime/agentctl/git_test.go` +- `apps/backend/internal/agent/handlers/git_handlers.go` +- `apps/backend/internal/agent/handlers/git_handlers_test.go` + +## Dependencies + +Task 01, for `PushOptions` and the server-side result fields. + +## Risks + +- `Client.GitPush` already takes four positional arguments. Adding two more + makes the call site unreadable; carry a small options struct instead, and + update the single existing caller in `internal/agent/handlers/git_handlers.go`. +- The client and server result structs are duplicated by design and drift + silently. The JSON keys of all five fields must match exactly. + +## Parallelism + +`parallel-safe` + +## Inputs + +- The system design's [Components and responsibilities](../../specs/workspaces/system-design/configured-push-target.md) + and [Data and contracts](../../specs/workspaces/system-design/configured-push-target.md) sections. +- The existing `repo` field convention across the git request structs. +- `GitContributionRequest` as the pattern for an optional string field carried + end to end. + +## Results + +Implemented. `GitPushRequest` and `GitPushPreflightRequest` carry `remote` and +`expected_branch` through the agentctl HTTP surface; the runtime client gained a +`PushOptions` struct plus the five result fields; and the backend `worktree.push` +payload forwards both. The startup remote-contribution preflight passes an empty +`PushOptions`, so it is unchanged. + +Two call sites shadowed the client package identifier with a local variable named +`client`, which the package's misleading `Package agentctl` doc comment hides. The +handler's local was renamed to `agentClient` to match its siblings, and the +lifecycle call site imports the package under an explicit `agentctlclient` alias. + +The new API-surface tests were placed in +`internal/agentctl/server/api/git_push_target_test.go` and +`internal/agent/handlers/git_push_options_test.go` rather than appended to the +existing `git_handlers_test.go` files, which were already close enough to +revive's 800-line file-length limit that appending crossed it. + +Verified with `go test -tags fts5 ./internal/agentctl/server/api/... +./internal/agent/runtime/agentctl/ ./internal/agent/handlers/... +./internal/agent/runtime/lifecycle/...` and `make -C apps/backend lint`. diff --git a/docs/public/git-operations.md b/docs/public/git-operations.md index 481e9a63f2..4a6c904f9a 100644 --- a/docs/public/git-operations.md +++ b/docs/public/git-operations.md @@ -302,7 +302,7 @@ These are the registered Kandev WebSocket actions. Every payload requires `sessi | Action | Additional payload | |--------|--------------------| | `worktree.pull` | `rebase` boolean | -| `worktree.push` | `force` and `set_upstream` booleans | +| `worktree.push` | `force` and `set_upstream` booleans; optional `remote` (configured remote name or exact configured push URL) and `expected_branch` (current-branch precondition) | | `worktree.rebase` | required `base_branch` | | `worktree.merge` | required `base_branch` | | `worktree.abort` | `operation`: exactly `merge` or `rebase` | @@ -315,6 +315,12 @@ These are the registered Kandev WebSocket actions. Every payload requires `sessi | `worktree.rename_branch` | required `new_name` | | `worktree.reset` | required `commit_sha`; `mode` is `soft`, `mixed`, or `hard` | +The optional push target fields apply to the WebSocket action and the backend +API. An explicit target uses the expected branch as the destination and does +not set upstream tracking. A branch mismatch returns the expected and current +branch values. Existing contribution routing takes precedence over an explicit +target. + Example request and normal operation result: ```json diff --git a/docs/specs/workspaces/requirements/configured-push-target.md b/docs/specs/workspaces/requirements/configured-push-target.md new file mode 100644 index 0000000000..0bb5b67fee --- /dev/null +++ b/docs/specs/workspaces/requirements/configured-push-target.md @@ -0,0 +1,344 @@ +--- +status: draft +system: workspaces +created: 2026-09-08 +owners: + - kandev +--- + +# Configured Push Target Requirements + +## Overview + +The workspace push API can today publish only to `origin` or to a remote a +contribution binding selects. A caller cannot name a third destination, and +cannot state which branch it believes it is publishing. + +This capability adds two independent, optional inputs to the workspace push and +push-preflight contracts: an explicit push target, and an expected branch. The +expected branch exists so a caller that decided to publish `feature/x` cannot +publish different content under that name when the agent moved `HEAD` between +the decision and the push. + +The workspace system owns this contract because it governs workspace Git state +and its remotes, not the runtime that executes the push. + +## Terminology + +- **Push target:** The destination a push publishes to, expressed either as a + configured Git remote name or as a remote URL. +- **Explicit push target:** A push target the caller names in the request, + rather than one Kandev derives from `origin` or a contribution binding. +- **Configured remote:** A remote already present in the checkout's Git + configuration. +- **Effective push URL set:** A configured remote's push URLs when it has at + least one, otherwise the single-element list holding its fetch URL. A remote + with neither has an empty set. +- **Contribution routing:** The existing remote-contribution and + contribution-destination behavior that selects a remote and refspec for the + caller. +- **Expected branch:** The branch name a caller states it intends to publish. +- **Current branch:** The branch the workspace checkout's `HEAD` points at. + A detached `HEAD` has no current branch. + +## Requirements + +### REQ-WORKSPACES-CONFIGURED-PUSH-TARGET-001: Explicit Push Target Selection + +**Intent:** Let a caller publish a task branch to a remote it names, without +changing what a caller that names nothing observes today. + +**User story:** As a Kandev service that publishes task work, I want to name the +remote a push goes to, so that a work branch can be preserved somewhere other +than `origin`. + +#### Acceptance criteria + +- **AC-WORKSPACES-CONFIGURED-PUSH-TARGET-001.1:** The workspace push and + push-preflight contracts shall accept an optional explicit push target. +- **AC-WORKSPACES-CONFIGURED-PUSH-TARGET-001.2:** When a request omits the + explicit push target and omits the expected branch, Kandev shall perform the + same push it performs today, including `origin` selection, contribution + routing, upstream selection, force behavior, and first publication. +- **AC-WORKSPACES-CONFIGURED-PUSH-TARGET-001.3:** When the explicit push target + satisfies the permissive ref-syntax allowlist Kandev already applies to branch + arguments, Kandev shall interpret it as a configured remote name; otherwise + Kandev shall interpret it as a remote URL. +- **AC-WORKSPACES-CONFIGURED-PUSH-TARGET-001.4:** When the explicit push target + is interpreted as a name and no configured remote carries that name, Kandev + shall refuse the request with error code `push_remote_not_found` and shall not + contact any remote. +- **AC-WORKSPACES-CONFIGURED-PUSH-TARGET-001.5:** When the explicit push target + is interpreted as a URL, Kandev shall resolve it to a configured remote whose + effective push URL set holds exactly one entry equal to the supplied value. + Each configured value shall be trimmed likewise, and the comparison shall + otherwise be exact: no case folding, no `.git` suffix added or + removed, no redirect resolved, no normalization. An empty set never matches. +- **AC-WORKSPACES-CONFIGURED-PUSH-TARGET-001.6:** When a URL push target matches + more than one configured remote, Kandev shall select the matching remote whose + name sorts first by byte order and shall publish to it. +- **AC-WORKSPACES-CONFIGURED-PUSH-TARGET-001.7:** When a URL push target matches + no configured remote, Kandev shall refuse the request with error code + `push_remote_url_unmatched` and shall not contact any remote. +- **AC-WORKSPACES-CONFIGURED-PUSH-TARGET-001.8:** Kandev shall never create, + rename, retarget, or delete a remote while serving a push or push-preflight + request. A push target that is not already configured shall be refused rather + than materialized. +- **AC-WORKSPACES-CONFIGURED-PUSH-TARGET-001.9:** When a request carries an + explicit push target and the workspace has contribution routing configured, + Kandev shall refuse with error code `push_remote_contribution_conflict` and + shall not push. +- **AC-WORKSPACES-CONFIGURED-PUSH-TARGET-001.10:** When a request carries an + explicit push target together with the set-upstream flag, Kandev shall refuse + the request with error code `push_remote_upstream_unsupported` and shall not + push. +- **AC-WORKSPACES-CONFIGURED-PUSH-TARGET-001.11:** A push that carries an + explicit push target shall not set, change, or remove the current branch's + upstream tracking configuration. +- **AC-WORKSPACES-CONFIGURED-PUSH-TARGET-001.12:** When a request carries an + explicit push target that resolves to a remote other than `origin`, Kandev + shall not perform empty-remote first publication. When it resolves to + `origin`, first publication shall behave exactly as it does for a request that + names no push target. +- **AC-WORKSPACES-CONFIGURED-PUSH-TARGET-001.13:** A push that carries an + explicit push target shall remain a non-force push unless the caller requests + force, and force shall stay refused whenever contribution routing applies. +- **AC-WORKSPACES-CONFIGURED-PUSH-TARGET-001.14:** No result field Kandev + populates, no error message Kandev composes, and no log record Kandev emits + shall contain a remote URL; each shall identify a push target by its resolved + remote name only. Git's passed-through output is excluded and keeps naming the + destination it contacted, as it does today. +- **AC-WORKSPACES-CONFIGURED-PUSH-TARGET-001.15:** Remote-name matching shall be + exact and case-sensitive. Kandev shall not accept a prefix, a suffix, or a + case-insensitive variant of a configured remote name. +- **AC-WORKSPACES-CONFIGURED-PUSH-TARGET-001.16:** When Kandev cannot read the + checkout's remote configuration while resolving an explicit push target, it + shall refuse the request with error code `push_remote_config_unreadable` and + shall not push. An unreadable configuration shall never be treated as an absent + push target, as an unmatched push target, or as a successful resolution. +- **AC-WORKSPACES-CONFIGURED-PUSH-TARGET-001.17:** When no configured remote + matches a URL push target under + AC-WORKSPACES-CONFIGURED-PUSH-TARGET-001.5 but at least one remote carries that + value among several push URLs, Kandev shall refuse with error code + `push_remote_fanout` and shall not push. A matching + single-entry remote takes precedence and resolves normally; only when none + exists does the fan-out refusal apply, and only when neither exists does + `push_remote_url_unmatched` apply. Naming a multi-push-URL remote stays + allowed and shall publish through it unchanged, reaching every configured push + URL without reduction or reordering. +- **AC-WORKSPACES-CONFIGURED-PUSH-TARGET-001.18:** On the explicit-push-target + push path, and on the preflight paths this capability introduces or changes, + Kandev shall apply to Git's passed-through output the same credential redaction + it already applies to empty-remote first-publication output. The push path + naming no explicit push target, and the existing contribution preflight paths, + shall keep the output handling they have today. + +### REQ-WORKSPACES-CONFIGURED-PUSH-TARGET-002: Expected-Branch Verification + +**Intent:** Let a caller state the branch it decided to publish, so an agent that +switches branches after that decision cannot cause different content to be +published under that name. + +**User story:** As a Kandev service that publishes task work, I want the push to +be refused when the workspace is no longer on the branch I decided to publish, +so that I never publish one branch's commits as another branch. + +#### Acceptance criteria + +- **AC-WORKSPACES-CONFIGURED-PUSH-TARGET-002.1:** The workspace push and + push-preflight contracts shall accept an optional expected branch, independent + of whether the request carries an explicit push target. +- **AC-WORKSPACES-CONFIGURED-PUSH-TARGET-002.2:** When the expected branch is + supplied and is not a valid branch name, Kandev shall refuse the request with + error code `push_branch_invalid` and shall not contact any remote. Valid means + the strictest branch allowlist Kandev already applies to a persisted branch + name: beyond the permissive rules on characters, length, `..` and `.lock`, an + expected branch ending in `/`, containing `//`, or equal to a reserved Git + symbolic pseudo-ref (`HEAD`, `ORIG_HEAD`, `FETCH_HEAD`, `MERGE_HEAD`) shall + also be rejected. +- **AC-WORKSPACES-CONFIGURED-PUSH-TARGET-002.3:** When the expected branch is + supplied and does not equal the current branch at the first verification point, + Kandev shall refuse with error code `push_branch_mismatch`, shall report the + expected and current branches in the result fields the system design declares, + and shall not contact any remote. A detached `HEAD` shall be reported as an + empty current branch, not as the literal `HEAD`. +- **AC-WORKSPACES-CONFIGURED-PUSH-TARGET-002.4:** When the expected branch is + supplied and `HEAD` is detached, Kandev shall treat the request as a mismatch + under AC-WORKSPACES-CONFIGURED-PUSH-TARGET-002.3. +- **AC-WORKSPACES-CONFIGURED-PUSH-TARGET-002.5:** On the push operation Kandev + shall verify the expected branch at two points inside the workspace Git + operation lock: once before it contacts any remote, so that a mismatch refuses + the request with no side effect; and again as the last Git read before the push + command, with no other Kandev-issued Git command between that verification and + the push. A mismatch at either point shall refuse the request. +- **AC-WORKSPACES-CONFIGURED-PUSH-TARGET-002.6:** When the second verification + fails, Kandev shall refuse the push, shall keep the task branch unpublished, + and shall report the expected and current branches as in + AC-WORKSPACES-CONFIGURED-PUSH-TARGET-002.3. When empty-remote first publication + published the baseline earlier in the same request, the refusal shall carry + error code `push_branch_mismatch_after_baseline` and shall set the result's + baseline-published field; otherwise it shall carry `push_branch_mismatch` and + shall omit that field. A second-point refusal is not required to be + side-effect-free: empty-remote preparation may already have read `origin`'s + advertised refs, and may already have retired the local first-publication + marker without publishing anything. +- **AC-WORKSPACES-CONFIGURED-PUSH-TARGET-002.7:** When a request carries an + explicit push target and an expected branch, Kandev shall publish the current + `HEAD` to the branch named by the expected branch on the resolved remote. +- **AC-WORKSPACES-CONFIGURED-PUSH-TARGET-002.8:** When a request carries an + explicit push target and no expected branch, Kandev shall publish the current + `HEAD` to the branch named by the current branch on the resolved remote, and + shall refuse a detached `HEAD` with error code `push_branch_detached` without + contacting any remote. +- **AC-WORKSPACES-CONFIGURED-PUSH-TARGET-002.9:** When a request carries an + expected branch and no explicit push target, the expected branch shall act only + as a precondition. Kandev shall not change the remote, the refspec, the + upstream behavior, or the first-publication behavior that the request would + otherwise have. + +- **AC-WORKSPACES-CONFIGURED-PUSH-TARGET-002.10:** When the destination branch + does not exist on the resolved remote, the push shall create it. Creating it + on a remote other than `origin` shall not publish a baseline commit. + +- **AC-WORKSPACES-CONFIGURED-PUSH-TARGET-002.11:** Kandev shall determine whether + `HEAD` is detached by reading `HEAD` as a symbolic ref and observing whether it + resolves under `refs/heads/`, and shall not infer the detached state by + comparing an abbreviated ref name against the literal string `HEAD`. + +- **AC-WORKSPACES-CONFIGURED-PUSH-TARGET-002.12:** Every Git read Kandev needs to + build the push command shall complete before the second expected-branch + verification required by AC-WORKSPACES-CONFIGURED-PUSH-TARGET-002.5, including + the upstream-tracking read on the path naming no explicit push target. Between that verification and the push, Kandev + shall issue no Git command other than the push itself. + +### REQ-WORKSPACES-CONFIGURED-PUSH-TARGET-003: Push Preflight Validation + +**Intent:** Make push preflight answer whether the destination-branch push that +would run is writable, instead of reporting success for a target it never +checked. + +**User story:** As a Kandev service preparing to publish, I want preflight to +check the destination my push would actually use, so that a failure surfaces +before the push runs. + +#### Acceptance criteria + +- **AC-WORKSPACES-CONFIGURED-PUSH-TARGET-003.1:** Push preflight shall apply the + same push-target resolution, expected-branch validation and comparison, refusal + codes, and refusal ordering that the push contract applies, and shall verify the + expected branch once, before its dry-run. Preflight publishes no baseline, so it + has no second verification and shall never report + `push_branch_mismatch_after_baseline`. +- **AC-WORKSPACES-CONFIGURED-PUSH-TARGET-003.2:** When a request carries an + explicit push target, preflight shall verify write access to the resolved + remote for the refspec the equivalent push would use. +- **AC-WORKSPACES-CONFIGURED-PUSH-TARGET-003.3:** When a request carries no + explicit push target and no contribution routing applies, preflight shall + verify write access to `origin` for the refspec the equivalent push would use, + instead of reporting success without checking a remote. +- **AC-WORKSPACES-CONFIGURED-PUSH-TARGET-003.4:** When no explicit push target is + supplied, no contribution routing applies, and the checkout has no `origin` + remote, preflight shall report failure with error code + `push_no_remote_configured`. +- **AC-WORKSPACES-CONFIGURED-PUSH-TARGET-003.5:** Push preflight shall not create, + move, update, or delete any ref on any remote, and shall not change local refs, + the working tree, or upstream tracking configuration. +- **AC-WORKSPACES-CONFIGURED-PUSH-TARGET-003.6:** Preflight against a remote that + refuses the write shall report failure with the remote's output rather than an + unhandled error, credential-redacted under + AC-WORKSPACES-CONFIGURED-PUSH-TARGET-001.18. +- **AC-WORKSPACES-CONFIGURED-PUSH-TARGET-003.7:** A successful preflight to which + contribution routing did not apply shall report the resolved remote name and + destination branch it validated, in the same result fields a successful push + uses, whether or not the request carried an explicit push target. A successful + preflight under contribution routing shall report neither field and shall keep + the result shape it has today. + +### REQ-WORKSPACES-CONFIGURED-PUSH-TARGET-004: Determinism, Concurrency, and Result Reporting + +**Intent:** Make the outcome of a push request predictable when inputs conflict, +when two callers arrive together, and when a caller retries. + +#### Acceptance criteria + +- **AC-WORKSPACES-CONFIGURED-PUSH-TARGET-004.1:** When a request would be refused + for more than one reason, Kandev shall report the first applicable refusal in + this fixed order: malformed request body; existing contribution binding + validation failure; `push_remote_contribution_conflict`; + `push_remote_upstream_unsupported`; `push_branch_invalid`; explicit push-target + resolution refusals (`push_remote_config_unreadable`, `push_remote_not_found`, + `push_remote_url_unmatched`, `push_remote_fanout`); + `push_no_remote_configured`; `push_branch_detached`; `push_branch_mismatch` + raised at the first verification point. Kandev shall evaluate every refusal in + this ordered list before it contacts any remote, including before empty-remote + first publication. +- **AC-WORKSPACES-CONFIGURED-PUSH-TARGET-004.2:** Every refusal listed in + AC-WORKSPACES-CONFIGURED-PUSH-TARGET-004.1 shall leave the workspace checkout, + its refs, its upstream configuration, and every remote unchanged. The + second-verification refusals governed by + AC-WORKSPACES-CONFIGURED-PUSH-TARGET-002.6 are the only refusals in this + capability that are not listed there, and the only ones permitted to leave a + published baseline or a retired first-publication marker behind. +- **AC-WORKSPACES-CONFIGURED-PUSH-TARGET-004.3:** A push or push-preflight + request shall hold the existing single workspace Git operation lock for the + whole request. When a second request for the same checkout arrives while one + holds the lock, Kandev shall report the existing operation-in-progress result + rather than queue, merge, or serialize it. +- **AC-WORKSPACES-CONFIGURED-PUSH-TARGET-004.4:** Repeating a successful push + request with identical inputs while the workspace `HEAD` and the destination + branch are unchanged shall report success and shall not change the destination + branch. +- **AC-WORKSPACES-CONFIGURED-PUSH-TARGET-004.5:** When the destination branch on + the resolved remote has commits the local `HEAD` does not contain, a non-force + push shall be refused by the remote and Kandev shall report that failure + without retrying and without escalating to a force push. +- **AC-WORKSPACES-CONFIGURED-PUSH-TARGET-004.6:** A successful push that carried + an explicit push target shall report the resolved remote name and the + destination branch name. A successful push that carried no explicit push target + shall omit both, so an existing consumer observes an unchanged result shape. + This omission rule governs the push operation only; preflight reports them in + every case under AC-WORKSPACES-CONFIGURED-PUSH-TARGET-003.7. +- **AC-WORKSPACES-CONFIGURED-PUSH-TARGET-004.7:** Kandev shall trim leading and + trailing whitespace from the explicit push target and from the expected branch + before any other check, including before the validation + AC-WORKSPACES-CONFIGURED-PUSH-TARGET-002.2 requires. Either input supplied + empty, or empty after that trim, shall be treated as absent and shall produce + the behavior of a request that omits it. +- **AC-WORKSPACES-CONFIGURED-PUSH-TARGET-004.8:** The explicit push target and + expected branch shall be scoped to the repository the request already selects + in a multi-repository task, and shall not affect any other repository in the + same workspace. + +## Out of scope + +- **Exact-OID lease.** No `--force-with-lease=:` form. A force push + keeps emitting the bare lease it emits today, and force stays refused for + contribution routing. +- **Eliminating the residual race window.** An agent shell can move `HEAD` + outside the operation lock, so the expected-branch check narrows the window but + cannot close it. Closing it requires the exact-OID lease above. +- **Preflighting the empty-remote baseline.** A push on the `origin` path may + first publish the baseline, a different refspec. Preflight validates only the + destination-branch refspec, so it can still report success where that baseline + publication would fail. +- **Auto-push policy.** When and under what condition a branch publishes + automatically is a separate capability; this contract only makes such a caller + expressible. +- **Creating or configuring remotes.** Adding a backup remote belongs to + workspace materialization, not the push contract. +- **Changing contribution routing.** Remote-contribution and + contribution-destination selection, their force-push refusal, and their + preflight behavior are unchanged. +- **A user-facing control.** No web, desktop, or mobile surface gains either + control; the existing push control keeps sending neither input. +- **Pushing tags, multiple refspecs, or deleting a remote branch.** +- **Credential selection.** Unchanged: the task runtime Git credential route. + +## Related requirements + +- [Empty Remote Repository Requirements](empty-remote-repositories.md) +- [Branch Policies](branch-policies.md) + +## System design + +- [Configured Push Target System Design](../system-design/configured-push-target.md) diff --git a/docs/specs/workspaces/system-design/configured-push-target.md b/docs/specs/workspaces/system-design/configured-push-target.md new file mode 100644 index 0000000000..0ab961fd27 --- /dev/null +++ b/docs/specs/workspaces/system-design/configured-push-target.md @@ -0,0 +1,368 @@ +--- +status: draft +system: workspaces +requirements: + - REQ-WORKSPACES-CONFIGURED-PUSH-TARGET-001 + - REQ-WORKSPACES-CONFIGURED-PUSH-TARGET-002 + - REQ-WORKSPACES-CONFIGURED-PUSH-TARGET-003 + - REQ-WORKSPACES-CONFIGURED-PUSH-TARGET-004 +--- + +# Configured Push Target System Design + +## Purpose and boundaries + +The workspace system owns workspace Git state and the contract that publishes a +task branch to a remote. This design extends the existing workspace push and +push-preflight contracts with an explicit push target and an expected branch. + +Adjacent contracts this design uses but does not own: + +- Contribution routing (remote contribution, contribution destination) and its + force-push refusal. This design refuses rather than overriding it. +- Empty-remote first publication, which stays attached to `origin`. +- The task runtime Git credential route. +- The agent runtime transport that carries a push request from the orchestrator + into the workspace. + +## Requirement mapping + +| Requirement | Design section | +| --- | --- | +| `REQ-WORKSPACES-CONFIGURED-PUSH-TARGET-001` | [Data and contracts](#data-and-contracts), [Push-target resolution](#push-target-resolution) | +| `REQ-WORKSPACES-CONFIGURED-PUSH-TARGET-002` | [Control flow](#control-flow) | +| `REQ-WORKSPACES-CONFIGURED-PUSH-TARGET-003` | [Preflight](#preflight) | +| `REQ-WORKSPACES-CONFIGURED-PUSH-TARGET-004` | [Refusal ordering](#refusal-ordering), [Failure and recovery](#failure-and-recovery) | + +## Components and responsibilities + +- **Workspace Git operator (agentctl, per checkout).** Owns the operation lock, + push-target resolution, the expected-branch guard, and Git command + construction. All new decisions live here so that no caller can reach the + destination selection by a different route. +- **Workspace Git HTTP surface (agentctl).** Binds the two new optional request + fields for both the push and push-preflight endpoints and passes them through + unchanged. It performs no destination logic of its own. +- **Agent runtime Git client (backend).** Carries the two new optional fields on + the push and push-preflight calls. Existing callers that pass neither keep + their current signatures' behavior. +- **Workspace Git action handler (backend).** Accepts the two new optional + fields on the existing push action and forwards them. The web client keeps + sending neither. +- **Git argument validator (agentctl).** The existing defense-in-depth allowlist + that inspects every argument before a Git process starts. This design keeps a + remote URL off the command line entirely so the allowlist does not have to be + widened. + +## Data and contracts + +Two optional inputs are added to the push and push-preflight request bodies, and +to the equivalent backend action payload and client calls. Both default to +absent, and absent reproduces today's behavior exactly. + +- `remote`: the explicit push target, as a configured remote name or a remote + URL. +- `expected_branch`: the branch the caller believes it is publishing. + +Both inputs are trimmed of leading and trailing whitespace before any other +check, and a value that is empty after that trim is absent. For the expected +branch this runs before the strict branch allowlist, so a whitespace-only value +is an omitted input rather than a `push_branch_invalid` refusal. The trim is +normalization, not a refusal: request-body parsing necessarily precedes it, and +it precedes every check in the refusal ordering below that reads either value. + +The result gains five optional output fields. Each is omitted when its rule +below does not apply, so a consumer that reads none of them sees the shape it +sees today. + +- `pushed_remote`: the resolved remote name. Populated on a successful push that + carried an explicit push target, and on a successful preflight to which + contribution routing did not apply. +- `pushed_branch`: the destination branch name. Same rule as `pushed_remote`. +- `expected_branch`: the expected branch the request supplied. Populated on a + `push_branch_mismatch` or `push_branch_mismatch_after_baseline` refusal. +- `current_branch`: the branch the checkout was actually on. Populated on the + same two refusals, and reported as empty when `HEAD` is detached rather than as + the literal `HEAD`. +- `baseline_published`: true when empty-remote first publication published the + baseline during this request. Populated only on + `push_branch_mismatch_after_baseline`, which is precisely the second-verification + refusal that followed such a publication. A successful push never sets it, even when first + publication ran, because doing so would change the result shape on the path + that names no push target. + +The asymmetry between push and preflight on the first two fields is deliberate. +The push path omits them without an explicit target so that today's only +frontend consumer observes an unchanged result; a preflight on that same path +has no consumer that reads them at all, so it reports what it validated, which +is the whole point of asking it. A preflight under contribution routing reports +neither, because `## Out of scope` promises that path's behavior is unchanged +and these fields are `omitempty`: populating them would add keys to a response +shape this capability undertook not to touch. + +Stable error codes, carried on the existing result error-code field: + +| Code | Meaning | +| --- | --- | +| `push_remote_not_found` | Name form; no configured remote carries that name. | +| `push_remote_url_unmatched` | URL form; no configured remote has a single-entry effective push URL set equal to the supplied value. | +| `push_remote_fanout` | URL form; a remote carries that URL but also others, so publishing through it would reach destinations the caller did not name. | +| `push_remote_config_unreadable` | The checkout's remote configuration could not be read while resolving an explicit push target. | +| `push_remote_contribution_conflict` | Explicit push target supplied while contribution routing is configured. | +| `push_remote_upstream_unsupported` | Explicit push target supplied together with set-upstream. | +| `push_branch_invalid` | Expected branch fails the ref allowlist. | +| `push_branch_mismatch` | Expected branch is not the current branch, including detached `HEAD`. Side-effect-free at the first verification; at the second it may follow empty-remote preparation that read the remote and retired the local marker without publishing. | +| `push_branch_mismatch_after_baseline` | Same mismatch at the second verification, after first publication published the baseline in this request. | +| `push_branch_detached` | Explicit push target with no expected branch, and `HEAD` is detached. | +| `push_no_remote_configured` | Preflight with no explicit target, no contribution routing, and no `origin`. | + +### Push-target resolution + +Resolution is a pure function of the request value and the checkout's remote +configuration, and never mutates configuration. + +1. Apply the shared trim above. An empty result means absent. +2. If the value satisfies the existing branch-name allowlist, treat it as a + remote name and require that the checkout's configured remote list contains + it. +3. Otherwise treat it as a URL. Read each configured remote's effective push URL + set: its push URLs when it has one or more, and otherwise the single entry + holding its fetch URL. Git allows a remote to carry several push URLs and + publishes to every one of them, so a remote matches only when its set holds + exactly one entry and that entry equals the supplied value under exact string + comparison, both sides trimmed. A remote that carries the supplied URL + alongside others is refused with `push_remote_fanout` rather than matched, + because resolving to it would publish to destinations the caller never named; + a caller that wants that fan-out names the remote instead, which is the + difference between asking for a destination and asking for a remote. A remote + with an empty set never matches. Several single-entry remotes can still match, + because several can legitimately carry the same URL; select the matching name + that sorts first by byte order, which is well defined because all such matches + address the same destination. +4. The name/URL discriminator is deliberately the permissive ref-syntax + allowlist rather than the strict branch allowlist an expected branch must + pass. A permissive reading cannot reach a destination on its own, because a + value read as a name must still match an already-configured remote before + anything is published; the expected branch has no such backstop, which is why + it is held to the stricter rule. The strict rule is also what keeps `HEAD` + out: the abbreviated-name read returns that literal for a detached checkout, + so an expected branch of `HEAD` would otherwise compare equal to it and pass a + guard whose whole purpose is to fail there. +5. Resolution yields a remote **name**. Only that name reaches a Git command + line. A caller-supplied URL is never passed to Git, which is what keeps the + existing argument allowlist unchanged and keeps credentials out of process + arguments. + +## Control flow + +A push request runs entirely inside the existing per-checkout operation lock. + +1. Acquire the lock. A busy checkout returns the existing + operation-in-progress signal. +2. Evaluate every refusal in the fixed order below, including the first + expected-branch verification. This whole step runs before any remote is + contacted and before any local ref changes, so a refused request is a no-op. +3. When the resolved remote is `origin` and no contribution routing applies, run + empty-remote first publication as it runs today. Skip it for any other + resolved remote. +4. Complete every remaining Git read needed to build the push command. On the + path that names no explicit push target this includes the upstream-tracking + read that decides set-upstream behavior; the explicit-target path needs no + such read, because that flag combination is already refused at step 2. Then + re-read the current branch, reading `HEAD` as a symbolic ref so that a + detached checkout is distinguishable from a branch literally named `HEAD`. + That branch read is the last Git read before the push. +5. When an expected branch was supplied, compare it again to the current branch + and refuse on mismatch. A detached `HEAD` is a mismatch because it has no + current branch. The refusal carries `push_branch_mismatch_after_baseline` + when step 3 published a baseline in this request, and `push_branch_mismatch` + otherwise. +6. Issue the push. No Kandev-issued Git command runs between the branch read in + step 4 and the push in step 6. + +Verifying twice is what lets both guarantees hold at once. The step 2 +verification keeps every refusal side-effect-free even though first publication +sits between the refusal set and the push. The step 5 verification gives the +ordering guarantee the caller actually needs: nothing Kandev issues can move +`HEAD` between the check and the push. The residual window is bounded by the +push itself, because an agent shell can still move `HEAD` outside the lock; +closing that needs an exact source-OID refspec or an equivalent lease, which +the requirement places out of scope. A destination lease alone cannot freeze +local `HEAD`. In the rare case where step 5 refuses after step 3 already published a +baseline, the result reports the completed baseline publication and leaves the +task branch unpublished, which is the existing partial first-publication +contract rather than a new failure mode. A step 5 refusal after a step 3 that +published nothing is a plain `push_branch_mismatch`. It is still not a strict +no-op: when a first-publication marker is present but the baseline is already on +the remote, step 3 reads `origin`'s advertised refs and retires that local +marker. Both are idempotent and neither publishes the task branch, which is why +this case needs no distinct code — but it is why the no-op guarantee is stated +for the step 2 refusal list only. + +Refspec selection: + +- No explicit push target: unchanged on every existing path. An expected branch + acts only as a precondition and does not rewrite the refspec. +- Explicit push target with an expected branch: `HEAD:refs/heads/`. +- Explicit push target without an expected branch: `HEAD:refs/heads/`, + with a detached `HEAD` refused first. + +The explicit-target path never appends the set-upstream flag and never reads the +upstream ref, because the flag combination is refused before the push. + +### Refusal ordering + +Fixed, so that a request with several problems always reports the same code: + +1. Malformed request body. +2. Existing contribution binding validation failure. +3. `push_remote_contribution_conflict`. +4. `push_remote_upstream_unsupported`. +5. `push_branch_invalid`. +6. `push_remote_config_unreadable` / `push_remote_not_found` / + `push_remote_url_unmatched` / `push_remote_fanout`. These four are mutually + exclusive by construction, so their relative order is never observable. +7. `push_no_remote_configured`. Preflight only: a push with no explicit target + and no `origin` fails at the push itself rather than at resolution. Ordered + ahead of the branch guards so a checkout with nowhere to publish reports that, + rather than reporting a stale branch the caller cannot act on yet. +8. `push_branch_detached`. +9. `push_branch_mismatch` raised at the first verification. + +The second-verification refusal is deliberately absent from this list. It is +produced at step 5, which runs after first publication, and it is the only +refusal in this capability that can follow a completed baseline publication or a +retired first-publication marker. It carries +`push_branch_mismatch_after_baseline` when a baseline was published in this +request and `push_branch_mismatch` otherwise. Everything in the list above is +evaluated at step 2 and is therefore a no-op. Once the second-verification +refusal is set aside, the list is exhaustive for this capability: every other +refusal it introduces appears in it. + +Cheap request-shape checks precede configuration reads, and configuration reads +precede the checkout read that resolves `HEAD`, so a rejected request performs +the least work. The whole list is evaluated at control-flow step 2, before +empty-remote first publication, which is why a refusal drawn from it never leaves +a partially-published remote behind. + +### Preflight + +Preflight reuses the same resolution, the same guard, and the same refusal +ordering, then issues a dry-run push for the refspec the equivalent push would +use. Two behaviors change relative to today: + +- The path that previously reported success without contacting a remote now + dry-runs against `origin` for the current branch. +- Every successful preflight to which contribution routing did not apply, with + or without an explicit push target, reports the remote name and branch it + validated. The push path's rule of omitting those fields without an explicit + target exists to protect an existing consumer; a preflight on that path has + none, so withholding what it checked would only make the answer less useful. A + preflight under contribution routing reports neither and keeps the result shape + it has today. +- A checkout with no `origin` and no contribution routing now reports + `push_no_remote_configured` instead of success. + +Preflight dry-runs the destination-branch refspec only. On the `origin` path an +actual push may first publish the empty-remote baseline, a different refspec that +preflight does not exercise, so preflight can still report success where that +baseline publication would fail. The requirement's `## Out of scope` names this +limit; REQ-003 removes preflight's blanket success for the destination branch and +does not extend it to the baseline. + +Preflight verifies the expected branch once rather than twice, and the +requirement scopes the two-point rule to the push operation for that reason. The +second verification exists on the push path only because first publication sits +between the refusal set and the push; preflight runs no first publication and +mutates nothing, so there is no window for the second read to close and +`push_branch_mismatch_after_baseline` is unreachable from preflight. A dry-run is +a push command, so this is a deliberate exception stated in both documents rather +than an oversight. + +The preflight paths this capability introduces or changes apply the same +credential redaction to Git's output that empty-remote first publication already +applies. The existing contribution preflight paths are left exactly as they are, +which is what `## Out of scope` promises. + +These changes are reachable only through the preflight endpoint and the startup +remote-contribution preflight, and the startup caller runs only when a +contribution binding exists, so none of them alters an existing live path. + +## Failure and recovery + +- Every refusal in the ordered list is a no-op: no remote contacted, no local + ref, upstream setting, or working tree changed. A caller can retry after + correcting its inputs. The single exception is + `push_branch_mismatch_after_baseline`, which by construction follows a + completed baseline publication on `origin`; its result sets + `baseline_published`, so a caller can see that a retry does not begin from the + state the first attempt began from. +- A remote that rejects a non-force push is reported as a failure with the + remote's output. Kandev does not retry it and does not escalate to a force + push. +- Repeating an identical successful push while `HEAD` and the destination branch + are unchanged succeeds and changes nothing, which is what makes the operation + safe for a retrying caller. +- Concurrency is handled by the existing single per-checkout lock. Requests are + rejected rather than queued, so a caller never waits on a lock it cannot see. +- Empty-remote failure codes keep their current meaning and keep applying only + to the `origin` path. + +## Persistence + +None. Both inputs are per-request and are not stored. No schema, migration, or +retention change. Restart behavior is unchanged because nothing new survives a +request. + +## Security + +- The remote URL form is resolved against already-configured remotes and is + never placed on a Git command line, so the existing command-injection + allowlist stays as narrow as it is today. +- The remote name form is validated against the existing ref allowlist and must + name an already-configured remote, so a request cannot introduce a new + destination or a flag-shaped argument. +- Kandev-composed output never carries a remote URL: the result fields above, + the error messages Kandev writes, and its log records identify a push target by + its resolved name only. +- Git's own output is a separate thing, and the honest description of today is + not that it is already URL-free. The workspace Git operator redacts the output + of every command whose subcommand is `push`, the preflight dry-run included, + through the same credential redaction empty-remote first publication uses. So + the credential redaction this capability requires is already in force on both + paths it adds, by construction rather than by a new call, and no path's output + handling changes. That redaction removes embedded credentials and known token + values, not URLs, so Git's `To ` line ships today and continues to. This + design does not change that, which is what keeps the path naming no explicit + push target byte-identical. +- Credentials continue to come from the task runtime Git credential route. Read + access alone does not authorize publication. + +## Observability + +The existing push completion log gains the resolved remote name, the destination +branch, and whether an expected branch was supplied and matched. Each refusal is +logged once at its error code. No new metric is introduced: the failure modes are +per-request and are already visible to the caller in the result. + +## Prior art + +- **Kandev knowledge vault (`wiki-query`).** Searched for: nothing; the leg did + not run. The skill is not installed on this runner (`~/.claude/skills` has no + `wiki-*` entry), there is no `~/.obsidian-wiki` directory or config symlink, + and `OBSIDIAN_VAULT_PATH` is unset, so neither a QMD collection nor a `grep` + fallback resolved. Treat this as missing input, not an empty result. +- **`saas-kb` MCP (`search_fsm_docs`, `category: "ai_sdlc"`).** Searched for: + nothing; the leg did not run. The server is not configured for this session, + in which `kandev` is the only MCP server. +- **In-repository prior reasoning.** Read directly: + [Empty Remote Repositories](empty-remote-repositories.md), which owns first + publication on the `origin` path, and + [ADR 2026-08-12 task-bound fork destinations](../../../decisions/2026-08-12-task-bound-fork-destinations.md), + which owns contribution-destination routing. This capability departs from + neither: it refuses rather than overrides when contribution routing applies, + and leaves first publication attached to `origin`. + +## Related decisions + +- [Task-bound fork destinations](../../../decisions/2026-08-12-task-bound-fork-destinations.md)