From 623d183b06c0bb697e12c89ba78301105a7703ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Sun, 13 Sep 2026 21:57:54 +0100 Subject: [PATCH] fix: bound Git subprocess execution (#3646) --- apps/backend/AGENTS.md | 1 + .../runtime/lifecycle/env_preparer_local.go | 27 +- .../lifecycle/env_preparer_local_test.go | 24 ++ .../internal/agentctl/server/api/git.go | 9 +- .../server/api/workspace_materialize.go | 28 +- .../server/api/workspace_materialize_test.go | 20 + .../manager_comparison_targets_test.go | 2 +- .../server/process/comparison_target.go | 10 +- .../server/process/comparison_target_test.go | 20 +- .../internal/agentctl/server/process/git.go | 49 ++- .../agentctl/server/process/git_auth_test.go | 139 ++++++ .../process/manager_comparison_targets.go | 37 ++ .../manager_comparison_targets_test.go | 40 +- .../server/process/workspace_git_cmd.go | 124 +----- .../server/process/workspace_git_diff.go | 31 +- .../workspace_git_diff_lifecycle_unix_test.go | 150 +++++++ .../server/process/workspace_git_env_test.go | 19 +- .../server/winproc/lifecycle_windows.go | 289 +------------ .../common/subproc/git_environment.go | 376 ++++++++++++++++ .../internal/common/subproc/git_lifecycle.go | 336 +++++++++++++++ .../common/subproc/git_lifecycle_other.go | 39 ++ .../common/subproc/git_lifecycle_unix.go | 77 ++++ .../common/subproc/git_lifecycle_unix_test.go | 324 ++++++++++++++ .../common/subproc/git_lifecycle_windows.go | 72 ++++ .../subproc/git_lifecycle_windows_test.go | 65 +++ .../common/subproc/git_noninteractive_test.go | 401 ++++++++++++++++++ .../subproc/git_noninteractive_unix_test.go | 76 ++++ .../git_noninteractive_windows_test.go | 41 ++ .../common/subproc/raw_git_audit_test.go | 115 ++++- .../backend/internal/common/subproc/shared.go | 29 +- .../common/winproc/lifecycle_other.go | 3 + .../common/winproc/lifecycle_windows.go | 303 +++++++++++++ .../internal/office/configloader/git.go | 17 +- .../internal/office/skills/materialization.go | 51 ++- .../office/skills/materialization_test.go | 17 +- .../executor/executor_host_gh_bridge.go | 3 + .../executor/executor_host_gh_bridge_test.go | 88 ++++ apps/backend/internal/repoclone/clone.go | 92 ++-- .../internal/repoclone/clone_auth_test.go | 24 ++ .../internal/repoclone/remote_state.go | 66 ++- .../internal/task/service/repository_fetch.go | 9 +- apps/backend/internal/worktree/manager_git.go | 146 +++---- .../backend/internal/worktree/manager_test.go | 7 +- .../worktree/submodule_hardening_test.go | 6 +- apps/web/components/task/dockview-shared.tsx | 1 + .../task/mobile/mobile-changes-panel.tsx | 11 + .../git/fork-pr-comparison-target-helpers.ts | 339 ++++++++++++++- .../git/fork-pr-comparison-target.spec.ts | 34 +- .../mobile-fork-pr-comparison-target.spec.ts | 39 +- docs/plans/fork-pr-comparison-targets/plan.md | 5 + docs/plans/git-subprocess-admission/plan.md | 5 + .../plan.md | 5 + .../noninteractive-git-execution/plan.md | 188 ++++++++ .../task-01-prompt-policy.md | 103 +++++ .../task-02-owned-cleanup.md | 100 +++++ .../task-03-network-budgets.md | 128 ++++++ .../task-04-comparison-recovery.md | 96 +++++ .../task-05-guidance.md | 103 +++++ docs/public/git-operations.md | 2 + .../requirements/git-subprocess-admission.md | 26 +- .../system-design/git-subprocess-execution.md | 121 ++++++ .../system-design/workspace-git-status.md | 18 +- 62 files changed, 4463 insertions(+), 663 deletions(-) create mode 100644 apps/backend/internal/agentctl/server/process/git_auth_test.go create mode 100644 apps/backend/internal/agentctl/server/process/workspace_git_diff_lifecycle_unix_test.go create mode 100644 apps/backend/internal/common/subproc/git_environment.go create mode 100644 apps/backend/internal/common/subproc/git_lifecycle.go create mode 100644 apps/backend/internal/common/subproc/git_lifecycle_other.go create mode 100644 apps/backend/internal/common/subproc/git_lifecycle_unix.go create mode 100644 apps/backend/internal/common/subproc/git_lifecycle_unix_test.go create mode 100644 apps/backend/internal/common/subproc/git_lifecycle_windows.go create mode 100644 apps/backend/internal/common/subproc/git_lifecycle_windows_test.go create mode 100644 apps/backend/internal/common/subproc/git_noninteractive_test.go create mode 100644 apps/backend/internal/common/subproc/git_noninteractive_unix_test.go create mode 100644 apps/backend/internal/common/subproc/git_noninteractive_windows_test.go create mode 100644 apps/backend/internal/common/winproc/lifecycle_other.go create mode 100644 apps/backend/internal/common/winproc/lifecycle_windows.go create mode 100644 docs/plans/noninteractive-git-execution/plan.md create mode 100644 docs/plans/noninteractive-git-execution/task-01-prompt-policy.md create mode 100644 docs/plans/noninteractive-git-execution/task-02-owned-cleanup.md create mode 100644 docs/plans/noninteractive-git-execution/task-03-network-budgets.md create mode 100644 docs/plans/noninteractive-git-execution/task-04-comparison-recovery.md create mode 100644 docs/plans/noninteractive-git-execution/task-05-guidance.md create mode 100644 docs/specs/platform/system-design/git-subprocess-execution.md diff --git a/apps/backend/AGENTS.md b/apps/backend/AGENTS.md index e53575e8c8..3c926a0ea3 100644 --- a/apps/backend/AGENTS.md +++ b/apps/backend/AGENTS.md @@ -226,6 +226,7 @@ Client (WS) ← Orchestrator ← Lifecycle Manager ←──── stream update - Pass context through chains; event bus for cross-component comm. - **Dependency direction:** Shared admission limits used by task models and orchestrator/messagequeue belong in a neutral internal package; task/models must not import the higher-level orchestrator/messagequeue package. - Production Git commands must use `subproc.NewGitCommand` with a classified `subproc.RunGit*` helper, or hold a classified admission slot across streaming `Start`/`Wait`. Do not construct raw Git commands outside `internal/common/subproc`; choose `interactive`, `lifecycle`, or `background`. +- Managed Git final preparation runs immediately before `Start` and owns the full `Start`-to-`Wait` lifecycle. Network callers choose finite post-admission budgets; use `AfterAcquire` builders so queue wait is outside the execution budget. Shared lifecycle code belongs in `internal/common`, and agentctl keeps only compatibility wrappers. - **Cross-tier shared code belongs in `internal/common/`.** agentctl ships as a standalone binary uploaded into containers, so neither it nor the backend may own code the other imports — put shared logic in a neutral `internal/common/*` package instead of importing across the boundary or forking a copy (`ptyexec` is the worked example; `subproc`, `gitref`, `securityutil` are older ones). Duplicating to "avoid the dependency" is how the two PTY copies drifted. - **Startup configuration ownership:** `internal/common/config/catalog.go` and `source.go` own the stable operator catalog, discovery, precedence, provenance, and typed values; managed agentctl children receive a private subset contract. Do not reparse stable env in executors or copy YAML into public child environments. - **Pure computation that only matters on Windows should not carry `//go:build windows`.** One CI job runs on Windows and it tests a package allowlist, so tagged code is easily unverified — loginpty's copy of the Win32 quoting was compiled by no job at all. Keep platform API calls behind the tag, leave string/path helpers untagged, and add packages needing native coverage to the `test-windows` job in `.github/workflows/backend-tests.yml`. diff --git a/apps/backend/internal/agent/runtime/lifecycle/env_preparer_local.go b/apps/backend/internal/agent/runtime/lifecycle/env_preparer_local.go index 10aad48ba9..67048d9866 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/env_preparer_local.go +++ b/apps/backend/internal/agent/runtime/lifecycle/env_preparer_local.go @@ -32,6 +32,8 @@ type LocalPreparer struct { logger *logger.Logger } +const localGitNetworkTimeout = 30 * time.Second + // NewLocalPreparer creates a new LocalPreparer. func NewLocalPreparer(log *logger.Logger) *LocalPreparer { return &LocalPreparer{ @@ -315,14 +317,10 @@ func checkoutBranch( var fetchOut []byte var fetchErr error if !remoteSyncHandled { - fetchCmd := subproc.NewGitCommand(ctx, "fetch", "origin", branch) - fetchCmd.Dir = workDir - fetchOut, fetchErr = subproc.RunGitCombinedOutputClass(ctx, subproc.GitLifecycle, fetchCmd) + fetchOut, fetchErr = runLocalGit(ctx, workDir, "fetch", "origin", branch) } - cmd := subproc.NewGitCommand(ctx, "checkout", branch) - cmd.Dir = workDir - out, err := subproc.RunGitCombinedOutputClass(ctx, subproc.GitLifecycle, cmd) + out, err := runLocalGit(ctx, workDir, "checkout", branch) outStr := redactCheckoutOutput(strings.TrimSpace(string(out)), sensitiveValues) if err != nil { if fetchErr != nil { @@ -334,6 +332,23 @@ func checkoutBranch( return outStr, nil } +func runLocalGit(ctx context.Context, workDir string, args ...string) ([]byte, error) { + output, runErr, execCtxErr := subproc.RunGitCombinedAfterAcquire( + ctx, + subproc.GitLifecycle, + localGitNetworkTimeout, + func(execCtx context.Context) *exec.Cmd { + cmd := subproc.NewGitCommand(execCtx, args...) + cmd.Dir = workDir + return cmd + }, + ) + if runErr == nil { + runErr = execCtxErr + } + return output, runErr +} + var credentialURLPattern = regexp.MustCompile(`(?i)(https?://)[^\s/@]+@`) // redactCheckoutOutput removes credentials from git diagnostics before they diff --git a/apps/backend/internal/agent/runtime/lifecycle/env_preparer_local_test.go b/apps/backend/internal/agent/runtime/lifecycle/env_preparer_local_test.go index 8fe850c481..b1b5242906 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/env_preparer_local_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/env_preparer_local_test.go @@ -10,6 +10,7 @@ import ( "path/filepath" "strings" "testing" + "time" "github.com/kandev/kandev/internal/common/logger" "github.com/kandev/kandev/internal/worktree" @@ -514,6 +515,29 @@ esac } } +func TestLocalCheckoutFetchDeadline(t *testing.T) { + binDir := t.TempDir() + fakeGit := filepath.Join(binDir, "git") + if err := os.WriteFile(fakeGit, []byte("#!/bin/sh\nsleep 10\n"), 0o755); err != nil { + t.Fatalf("write fake git: %v", err) + } + t.Setenv("PATH", binDir+string(os.PathListSeparator)+os.Getenv("PATH")) + + ctx, cancel := context.WithTimeout(context.Background(), 75*time.Millisecond) + defer cancel() + started := time.Now() + _, err := runLocalGit(ctx, t.TempDir(), "fetch", "origin", "main") + if err == nil { + t.Fatal("runLocalGit() error = nil, want deadline failure") + } + if ctx.Err() == nil { + t.Fatal("runLocalGit() returned before the caller context expired") + } + if elapsed := time.Since(started); elapsed > 2*time.Second { + t.Fatalf("runLocalGit() took %s after cancellation", elapsed) + } +} + func TestLocalPreparer_RunsSetupScriptWithoutCheckout(t *testing.T) { isolateGitEnv(t) log := newTestLocalLogger() diff --git a/apps/backend/internal/agentctl/server/api/git.go b/apps/backend/internal/agentctl/server/api/git.go index 3a499274bf..3e4c0aabe8 100644 --- a/apps/backend/internal/agentctl/server/api/git.go +++ b/apps/backend/internal/agentctl/server/api/git.go @@ -1477,6 +1477,9 @@ func (s *Server) handleGitStatusMulti(c *gin.Context) { subpaths = []string{""} } fresh := c.Query("fresh") == queryParamTrue + if fresh { + s.procMgr.RetryUnavailableComparisonTargets() + } // Parallel fan-out: fresh=true skips the cache, so serial scales linearly and would blow the 2s subscribe timeout for multi-repo workspaces. result := MultiRepoGitStatusResult{Success: true, Repos: make([]PerRepoGitStatus, len(subpaths))} ctx := c.Request.Context() @@ -1570,7 +1573,11 @@ func (s *Server) handleGitStatus(c *gin.Context) { return } - status, err := wt.GetGitStatus(c.Request.Context(), c.Query("fresh") == queryParamTrue) + fresh := c.Query("fresh") == queryParamTrue + if fresh { + s.procMgr.RetryUnavailableComparisonTargets() + } + status, err := wt.GetGitStatus(c.Request.Context(), fresh) if err != nil { s.logger.Error("git status failed", zap.Error(err)) c.JSON(http.StatusInternalServerError, GitStatusResult{ diff --git a/apps/backend/internal/agentctl/server/api/workspace_materialize.go b/apps/backend/internal/agentctl/server/api/workspace_materialize.go index e63d0b9486..12335e7a79 100644 --- a/apps/backend/internal/agentctl/server/api/workspace_materialize.go +++ b/apps/backend/internal/agentctl/server/api/workspace_materialize.go @@ -8,8 +8,10 @@ import ( "net/http" "net/url" "os" + "os/exec" "path/filepath" "strings" + "time" "unicode" "github.com/gin-gonic/gin" @@ -733,9 +735,15 @@ func clearMaterializedQuarantine(root *os.Root) error { } func materializeGitOutput(ctx context.Context, args ...string) (string, error) { - cmd := subproc.NewGitCommand(ctx, args...) - output, err := subproc.RunGitCombinedOutputClass(ctx, subproc.GitLifecycle, cmd) - if err != nil { + output, runErr, execCtxErr := subproc.RunGitCombinedAfterAcquire( + ctx, + subproc.GitLifecycle, + materializeGitTimeout(args), + func(execCtx context.Context) *exec.Cmd { + return subproc.NewGitCommand(execCtx, args...) + }, + ) + if runErr != nil || execCtxErr != nil { if ctx.Err() != nil { return "", ctx.Err() } @@ -743,3 +751,17 @@ func materializeGitOutput(ctx context.Context, args ...string) (string, error) { } return string(output), nil } + +func materializeGitTimeout(args []string) time.Duration { + for index := 0; index < len(args); index++ { + switch args[index] { + case "-C": + index++ + case "clone", "push", "submodule": + return 5 * time.Minute + case "fetch", "pull", "ls-remote": + return 30 * time.Second + } + } + return 30 * time.Second +} diff --git a/apps/backend/internal/agentctl/server/api/workspace_materialize_test.go b/apps/backend/internal/agentctl/server/api/workspace_materialize_test.go index 234153f4e1..bf96ac07dd 100644 --- a/apps/backend/internal/agentctl/server/api/workspace_materialize_test.go +++ b/apps/backend/internal/agentctl/server/api/workspace_materialize_test.go @@ -13,6 +13,7 @@ import ( "path/filepath" "strings" "testing" + "time" "github.com/kandev/kandev/internal/agentctl/server/config" "github.com/kandev/kandev/internal/agentctl/server/process" @@ -29,6 +30,25 @@ func TestContributionDestinationPushURLsMatchAcceptsRepeatedTarget(t *testing.T) } } +func TestMaterializeGitTimeoutClassifiesNetworkVerbs(t *testing.T) { + for _, test := range []struct { + name string + args []string + want time.Duration + }{ + {name: "clone", args: []string{"clone", "--no-checkout", "url", "path"}, want: 5 * time.Minute}, + {name: "fetch after directory", args: []string{"-C", "/workspace", "fetch", "origin"}, want: 30 * time.Second}, + {name: "push", args: []string{"push", "origin", "HEAD"}, want: 5 * time.Minute}, + {name: "local status", args: []string{"-C", "/workspace", "status"}, want: 30 * time.Second}, + } { + t.Run(test.name, func(t *testing.T) { + if got := materializeGitTimeout(test.args); got != test.want { + t.Fatalf("materializeGitTimeout(%v) = %s, want %s", test.args, got, test.want) + } + }) + } +} + func TestMaterializeRepository_ClonesIntoWorkspaceDestination(t *testing.T) { origin := createMaterializeOrigin(t) workDir := t.TempDir() diff --git a/apps/backend/internal/agentctl/server/instance/manager_comparison_targets_test.go b/apps/backend/internal/agentctl/server/instance/manager_comparison_targets_test.go index 294b44add8..6787beaef8 100644 --- a/apps/backend/internal/agentctl/server/instance/manager_comparison_targets_test.go +++ b/apps/backend/internal/agentctl/server/instance/manager_comparison_targets_test.go @@ -156,7 +156,7 @@ func runInstanceComparisonGitShim() { switch args[0] + " " + args[1] { case "rev-parse --git-dir": _, _ = os.Stdout.WriteString(".git\n") - case "remote get-url": + case "config --get": if marker := os.Getenv("KANDEV_TEST_COMPARISON_STARTED"); marker != "" { _ = os.WriteFile(marker, nil, 0o600) } diff --git a/apps/backend/internal/agentctl/server/process/comparison_target.go b/apps/backend/internal/agentctl/server/process/comparison_target.go index 632930ae95..3930fd5c8e 100644 --- a/apps/backend/internal/agentctl/server/process/comparison_target.go +++ b/apps/backend/internal/agentctl/server/process/comparison_target.go @@ -168,7 +168,10 @@ func materializeComparisonTarget( ) } remoteName := target.ComparisonRemoteName() - configuredURL, remoteErr := run(ctx, "remote", "get-url", remoteName) + // Read the stored URL directly. `remote get-url` applies Git's + // url..insteadOf transport rewrite, which can make a canonical + // comparison remote look like a collision when a local transport is in use. + configuredURL, remoteErr := run(ctx, "config", "--get", "remote."+remoteName+".url") if remoteErr == nil { if strings.TrimSpace(configuredURL) != target.TargetRepository.RemoteURL { return comparisonTargetMaterialization{}, comparisonTargetFailure( @@ -190,7 +193,10 @@ func materializeComparisonTarget( fmt.Errorf("comparison remote push protection failed: %w", err), ) } - refspec := "refs/heads/" + target.TargetBranch + ":" + target.ComparisonRef() + // The comparison ref is an internal cache of a moving provider branch. + // Force-update only this exact ref so recovery can replace a stale or + // partially materialized value without changing any user remote. + refspec := "+refs/heads/" + target.TargetBranch + ":" + target.ComparisonRef() if _, err := run(ctx, "fetch", "--no-tags", remoteName, refspec); err != nil { return comparisonTargetMaterialization{}, comparisonTargetFailure( comparisonTargetErrorFetch, diff --git a/apps/backend/internal/agentctl/server/process/comparison_target_test.go b/apps/backend/internal/agentctl/server/process/comparison_target_test.go index f721cb9b5c..b439be8fbc 100644 --- a/apps/backend/internal/agentctl/server/process/comparison_target_test.go +++ b/apps/backend/internal/agentctl/server/process/comparison_target_test.go @@ -25,7 +25,7 @@ func (f *comparisonTargetGitFake) run(_ context.Context, args ...string) (string if output, ok := f.outputs[key]; ok { return output, nil } - if len(args) >= 3 && args[0] == "remote" && args[1] == "get-url" { + if len(args) >= 3 && args[0] == "config" && args[1] == "--get" && strings.HasPrefix(args[2], "remote.") { if f.remoteURL == "" { return "", errors.New("remote not found") } @@ -74,7 +74,7 @@ func TestMaterializeComparisonTargetUsesExactNoPushRefspec(t *testing.T) { if !hasComparisonCommand(fake.commands, "config", "remote."+target.ComparisonRemoteName()+".pushurl", "DISABLED") { t.Fatalf("push disabling command missing: %v", fake.commands) } - if !hasComparisonCommand(fake.commands, "fetch", "--no-tags", target.ComparisonRemoteName(), "refs/heads/main:"+target.ComparisonRef()) { + if !hasComparisonCommand(fake.commands, "fetch", "--no-tags", target.ComparisonRemoteName(), "+refs/heads/main:"+target.ComparisonRef()) { t.Fatalf("exact fetch command missing: %v", fake.commands) } for _, command := range fake.commands { @@ -93,6 +93,22 @@ func TestMaterializeComparisonTargetRejectsRemoteCollision(t *testing.T) { } } +func TestMaterializeComparisonTargetAcceptsCanonicalConfiguredURL(t *testing.T) { + target := comparisonTargetProcessTestTarget() + fake := &comparisonTargetGitFake{remoteURL: target.TargetRepository.RemoteURL, outputs: map[string]string{ + "rev-parse --verify " + target.ComparisonRef() + "^{commit}": "0123456789abcdef0123456789abcdef01234567", + }} + if _, err := materializeComparisonTarget(context.Background(), fake.run, target); err != nil { + t.Fatalf("materializeComparisonTarget: %v", err) + } + if hasComparisonCommand(fake.commands, "remote", "add", "--no-tags", target.ComparisonRemoteName(), target.TargetRepository.RemoteURL) { + t.Fatalf("materialization re-added the canonical comparison remote: %v", fake.commands) + } + if !hasComparisonCommand(fake.commands, "config", "--get", "remote."+target.ComparisonRemoteName()+".url") { + t.Fatalf("raw comparison remote URL lookup missing: %v", fake.commands) + } +} + func TestWorkspaceTrackerComparisonResolutionNeverFallsBackWhenTargetUnavailable(t *testing.T) { tracker := NewWorkspaceTracker(t.TempDir(), newTestLogger(t)) target := comparisonTargetProcessTestTarget() diff --git a/apps/backend/internal/agentctl/server/process/git.go b/apps/backend/internal/agentctl/server/process/git.go index 232f3b0fca..2a489e41e6 100644 --- a/apps/backend/internal/agentctl/server/process/git.go +++ b/apps/backend/internal/agentctl/server/process/git.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "os" + "os/exec" "path/filepath" "strings" "sync" @@ -28,6 +29,11 @@ var ErrInvalidBranchName = errors.New("invalid branch name") const preflightReasonHistoryUpdateRequired = "history_update_required" +const ( + gitOperatorDefaultTimeout = 30 * time.Second + gitOperatorNetworkTimeout = 5 * time.Minute +) + var contributionPreflightEnvironment = map[string]string{ "GIT_TERMINAL_PROMPT": "0", "LANG": "C", @@ -216,20 +222,25 @@ func (g *GitOperator) runGitCommandWithEnvironment( return "", err } - // All args validated: flags in securityutil.IsKnownSafeGitFlag whitelist, branch names via securityutil.IsValidBranchName - // regex, commit SHAs via securityutil.LooksLikeCommitSHA pattern, args after "--" separator skipped. - // This defense-in-depth validation prevents injection of arbitrary commands. - cmd := subproc.NewGitCommand(ctx, args...) - cmd.Dir = g.workDir - cmd.Env = withEnvironmentOverrides(filterGitEnv(g.environmentValues()), environmentOverrides) - - var stdout, stderr bytes.Buffer - cmd.Stdout = &stdout - cmd.Stderr = &stderr - g.logger.Debug("executing git command", zap.Strings("args", args)) - - err := subproc.RunGitClass(ctx, subproc.GitInteractive, cmd) + environment := withEnvironmentOverrides(filterGitEnv(g.environmentValues()), environmentOverrides) + var stdout, stderr bytes.Buffer + err, execCtxErr := subproc.RunGitAfterAcquire( + ctx, + subproc.GitInteractive, + gitOperatorTimeout(args), + func(execCtx context.Context) *exec.Cmd { + cmd := subproc.NewGitCommand(execCtx, args...) + cmd.Dir = g.workDir + cmd.Env = append([]string(nil), environment...) + cmd.Stdout = &stdout + cmd.Stderr = &stderr + return cmd + }, + ) + if err == nil { + err = execCtxErr + } output := stdout.String() if stderr.Len() > 0 { if output != "" { @@ -250,6 +261,18 @@ func (g *GitOperator) runGitCommandWithEnvironment( return output, nil } +func gitOperatorTimeout(args []string) time.Duration { + if len(args) == 0 { + return gitOperatorDefaultTimeout + } + switch args[0] { + case "clone", "push", "submodule": + return gitOperatorNetworkTimeout + default: + return gitOperatorDefaultTimeout + } +} + func validateGitCommandArgs(args []string) error { // Validate that user-controlled arguments don't introduce command injection risks. // exec.CommandContext does not use a shell, but git still interprets unsafe flags. diff --git a/apps/backend/internal/agentctl/server/process/git_auth_test.go b/apps/backend/internal/agentctl/server/process/git_auth_test.go new file mode 100644 index 0000000000..b772d3bda9 --- /dev/null +++ b/apps/backend/internal/agentctl/server/process/git_auth_test.go @@ -0,0 +1,139 @@ +package process + +import ( + "context" + "fmt" + "net/http" + "net/http/httptest" + "os" + "os/exec" + "path/filepath" + "strings" + "sync/atomic" + "testing" +) + +func TestGitOperatorAuthenticationFailureAndRecovery(t *testing.T) { + repoDir, cleanup := setupTestRepo(t) + t.Cleanup(cleanup) + remoteDir := strings.TrimSpace(runGit(t, repoDir, "remote", "get-url", "origin")) + + var unauthorized atomic.Int32 + var authenticated atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(response http.ResponseWriter, request *http.Request) { + username, password, ok := request.BasicAuth() + if !ok || username != "x-access-token" || password != "good-token" { + unauthorized.Add(1) + response.Header().Set("WWW-Authenticate", `Basic realm="git-operator-test"`) + response.WriteHeader(http.StatusUnauthorized) + return + } + authenticated.Add(1) + serveProcessGitHTTPBackend(response, request, filepath.Dir(remoteDir)) + })) + t.Cleanup(server.Close) + + remoteURL := server.URL + "/" + filepath.Base(remoteDir) + runGit(t, repoDir, "remote", "set-url", "origin", remoteURL) + + credential := "bad-token" + baseEnv := filterTestGitEnv(os.Environ()) + baseEnv = append(baseEnv, + "HOME="+t.TempDir(), + "GIT_CONFIG_GLOBAL="+os.DevNull, + "GIT_CONFIG_NOSYSTEM=1", + "GIT_CONFIG_COUNT=2", + "GIT_CONFIG_KEY_0=credential.helper", + "GIT_CONFIG_VALUE_0=", + "GIT_CONFIG_KEY_1=credential."+server.URL+".helper", + ) + operator := NewGitOperator(repoDir, newTestLogger(t), nil) + operator.setEnvironmentProvider(func() []string { + env := append([]string(nil), baseEnv...) + env = append(env, "GIT_CONFIG_VALUE_1="+gitTestCredentialHelper(credential)) + return env + }) + + first, err := operator.Pull(context.Background(), false) + if err != nil { + t.Fatalf("first Pull() returned error: %v", err) + } + if first == nil || first.Success { + t.Fatalf("first Pull() = %+v, want required authentication failure", first) + } + if strings.Contains(first.Error+first.Output, "bad-token") { + t.Fatalf("first Pull() leaked rejected credential: %+v", first) + } + if unauthorized.Load() == 0 { + t.Fatal("HTTP fixture did not observe the rejected credential") + } + if !operator.tryLock("after-auth-failure") { + t.Fatal("GitOperator operation lock was not released after required failure") + } + operator.unlock() + + credential = "good-token" + second, err := operator.Pull(context.Background(), false) + if err != nil { + t.Fatalf("recovery Pull() returned error: %v", err) + } + if second == nil || !second.Success { + t.Fatalf("recovery Pull() = %+v, want success", second) + } + if authenticated.Load() == 0 { + t.Fatal("HTTP fixture did not observe the restored credential") + } +} + +func gitTestCredentialHelper(token string) string { + return fmt.Sprintf("!f() { printf 'username=x-access-token\\npassword=%s\\n'; }; f", token) +} + +func serveProcessGitHTTPBackend(response http.ResponseWriter, request *http.Request, root string) { + cmd := exec.Command("git", "http-backend") + cmd.Env = append(os.Environ(), + "GIT_PROJECT_ROOT="+root, + "GIT_HTTP_EXPORT_ALL=1", + "PATH_INFO="+request.URL.Path, + "QUERY_STRING="+request.URL.RawQuery, + "REQUEST_METHOD="+request.Method, + "REMOTE_ADDR=127.0.0.1", + "CONTENT_TYPE="+request.Header.Get("Content-Type"), + "CONTENT_LENGTH="+request.Header.Get("Content-Length"), + ) + cmd.Stdin = request.Body + output, err := cmd.Output() + if err != nil { + response.WriteHeader(http.StatusBadGateway) + _, _ = fmt.Fprintf(response, "git http-backend failed: %v", err) + return + } + outputString := string(output) + headerEnd := strings.Index(outputString, "\r\n\r\n") + delimiterLength := 4 + if headerEnd < 0 { + headerEnd = strings.Index(outputString, "\n\n") + delimiterLength = 2 + } + if headerEnd < 0 { + response.WriteHeader(http.StatusBadGateway) + return + } + for _, line := range strings.Split(outputString[:headerEnd], "\n") { + line = strings.TrimSuffix(line, "\r") + key, value, ok := strings.Cut(line, ": ") + if !ok { + continue + } + if key == "Status" { + var status int + _, _ = fmt.Sscanf(value, "%d", &status) + if status != 0 { + response.WriteHeader(status) + } + continue + } + response.Header().Set(key, value) + } + _, _ = response.Write(output[headerEnd+delimiterLength:]) +} diff --git a/apps/backend/internal/agentctl/server/process/manager_comparison_targets.go b/apps/backend/internal/agentctl/server/process/manager_comparison_targets.go index becb74aa5e..fced1a30fa 100644 --- a/apps/backend/internal/agentctl/server/process/manager_comparison_targets.go +++ b/apps/backend/internal/agentctl/server/process/manager_comparison_targets.go @@ -51,6 +51,43 @@ func (m *Manager) PrepareComparisonTargets(_ context.Context) { } } +// RetryUnavailableComparisonTargets schedules one new materialization attempt +// for each explicit target that is currently unavailable. This is called only +// by an explicit fresh Git-status request; it does not create a timer or a +// background retry loop. Invalid targets remain unavailable until their +// configuration changes. +func (m *Manager) RetryUnavailableComparisonTargets() { + root, trackers := m.snapshotTrackers() + + m.workspaceTrackersMu.Lock() + lazyTrackers := make([]*WorkspaceTracker, 0, len(m.workspaceTrackersBySubpath)) + for _, tracker := range m.workspaceTrackersBySubpath { + lazyTrackers = append(lazyTrackers, tracker) + } + m.workspaceTrackersMu.Unlock() + + all := make([]*WorkspaceTracker, 0, 1+len(trackers)+len(lazyTrackers)) + all = append(all, root) + all = append(all, trackers...) + all = append(all, lazyTrackers...) + seen := make(map[*WorkspaceTracker]struct{}, len(all)) + for _, tracker := range all { + if tracker == nil { + continue + } + if _, ok := seen[tracker]; ok { + continue + } + seen[tracker] = struct{}{} + resolution := tracker.ComparisonResolution() + if !resolution.Explicit || resolution.Status != comparisonTargetStatusUnavailable || + resolution.ErrorCode == comparisonTargetErrorInvalid { + continue + } + m.prepareTrackerComparisonTarget(tracker) + } +} + // UpdateComparisonTargets replaces the desired map and refreshes only trackers // whose target changed. Existing ready siblings keep their state and are not // refetched when another repository is updated. Target materialization runs in diff --git a/apps/backend/internal/agentctl/server/process/manager_comparison_targets_test.go b/apps/backend/internal/agentctl/server/process/manager_comparison_targets_test.go index ba45a8779e..9d285fb182 100644 --- a/apps/backend/internal/agentctl/server/process/manager_comparison_targets_test.go +++ b/apps/backend/internal/agentctl/server/process/manager_comparison_targets_test.go @@ -97,6 +97,41 @@ func TestComparisonTargetMaterializationPublishesBoundedFetchFailureNoTransportF } } +func TestRetryUnavailableComparisonTargetsRecoversOnFreshRequest(t *testing.T) { + repoDir, cleanup := setupTestRepo(t) + t.Cleanup(cleanup) + target := comparisonTargetProcessTestTarget() + mgr, markers := newComparisonTargetTestManager(t, repoDir, target, map[string]string{ + "KANDEV_TEST_FETCH_ERROR": "1", + }) + + mgr.PrepareComparisonTargets(context.Background()) + waitForComparisonResolution(t, mgr.GetWorkspaceTracker(), comparisonTargetStatusUnavailable, comparisonTargetErrorFetch) + + // The test shim reads this environment from the tracker command rather than + // the test process, so recovery exercises the same per-instance environment + // update used when a credential service becomes available again. + mgr.GetWorkspaceTracker().SetGitEnvironment([]string{ + comparisonTargetGitShimModeEnv + "=comparison", + "KANDEV_TEST_GIT_LOG=" + markers.commandLog, + "KANDEV_TEST_FETCH_STARTED=" + markers.fetchStarted, + "KANDEV_TEST_STATUS_STARTED=" + markers.statusStarted, + }) + mgr.RetryUnavailableComparisonTargets() + + resolution := waitForComparisonResolution(t, mgr.GetWorkspaceTracker(), comparisonTargetStatusReady, "") + if resolution.Ref != target.ComparisonRef() { + t.Fatalf("recovered comparison ref = %q, want %q", resolution.Ref, target.ComparisonRef()) + } + logBytes, err := os.ReadFile(markers.commandLog) + if err != nil { + t.Fatalf("read Git command log: %v", err) + } + if got := strings.Count(string(logBytes), "fetch --no-tags"); got != 2 { + t.Fatalf("comparison fetch count after recovery = %d, want two; commands:\n%s", got, logBytes) + } +} + func TestComparisonTargetPublicationRequiresActiveOperation(t *testing.T) { repoDir, cleanup := setupTestRepo(t) t.Cleanup(cleanup) @@ -326,12 +361,13 @@ func runComparisonTargetGitShim() { if len(args) < 2 { os.Exit(0) } + if args[0] == "config" && args[1] == "--get" && len(args) > 2 && strings.HasPrefix(args[2], "remote.") { + os.Exit(1) + } if args[0] == "config" && strings.HasPrefix(args[1], "remote.") { os.Exit(0) } switch args[0] + " " + args[1] { - case "remote get-url": - os.Exit(1) case "remote add": os.Exit(0) case "fetch --no-tags": diff --git a/apps/backend/internal/agentctl/server/process/workspace_git_cmd.go b/apps/backend/internal/agentctl/server/process/workspace_git_cmd.go index 8073a67480..8389cb0425 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_git_cmd.go +++ b/apps/backend/internal/agentctl/server/process/workspace_git_cmd.go @@ -33,8 +33,6 @@ func gitWorkClass(ctx context.Context) subproc.GitWorkClass { // See: https://git-scm.com/docs/git#Documentation/git.txt-codeGITOPTIONALLOCKSltbooleangtcode const gitOptionalLocksOff = "GIT_OPTIONAL_LOCKS=0" -const defaultGitSSHCommand = "ssh -oBatchMode=yes" - // pollingGitCommand builds an exec.Cmd with optional Git locks disabled. The // lock policy is independent from the admission class: fresh interactive // status also needs lockless reads while still using the interactive queue. @@ -48,128 +46,12 @@ func (wt *WorkspaceTracker) pollingGitCommand(ctx context.Context, args ...strin func (wt *WorkspaceTracker) gitCommandEnv(ctx context.Context, lockless bool) []string { env := wt.gitEnvironmentSnapshot() if lockless { - env = replaceGitEnvAssignment(env, gitOptionalLocksOff) + env = append(env, gitOptionalLocksOff) } if indexPath := gitIndexFile(ctx); indexPath != "" { - env = replaceGitEnvAssignment(env, "GIT_INDEX_FILE="+indexPath) + env = append(env, "GIT_INDEX_FILE="+indexPath) } - for _, assignment := range []string{ - "GIT_TERMINAL_PROMPT=0", - "GCM_INTERACTIVE=Never", - "GIT_ASKPASS=echo", - "SSH_ASKPASS=/bin/false", - } { - env = replaceGitEnvAssignment(env, assignment) - } - if command, ok := gitEnvValue(env, "GIT_SSH_COMMAND"); ok && strings.TrimSpace(command) != "" { - env = replaceGitEnvAssignment(env, "GIT_SSH_COMMAND="+forceGitSSHBatchMode(command)) - } else { - env = replaceGitEnvAssignment(env, "GIT_SSH_COMMAND="+defaultGitSSHCommand) - } - return env -} - -func gitEnvValue(env []string, key string) (string, bool) { - prefix := key + "=" - var value string - found := false - for _, entry := range env { - if strings.HasPrefix(entry, prefix) { - value = strings.TrimPrefix(entry, prefix) - found = true - } - } - return value, found -} - -// forceGitSSHBatchMode places BatchMode=yes immediately after a direct -// OpenSSH executable. OpenSSH uses the first command-line value for an option, -// so this prevents a later BatchMode=no in the inherited command from -// restoring terminal prompting while preserving the command's other options. -// Shell prefixes and custom wrappers are not parsed here. They use the safe -// default instead of receiving an option in the wrong position. -func forceGitSSHBatchMode(command string) string { - command = strings.TrimSpace(command) - if command == "" { - return defaultGitSSHCommand - } - commandEnd := shellWordEnd(command) - if commandEnd == 0 { - return defaultGitSSHCommand - } - executable, ok := shellWordValue(command[:commandEnd]) - if !ok || !isOpenSSHExecutable(executable) { - return defaultGitSSHCommand - } - return command[:commandEnd] + " -oBatchMode=yes" + command[commandEnd:] -} - -func shellWordValue(word string) (string, bool) { - if word == "" { - return "", false - } - if word[0] == '\'' || word[0] == '"' { - if len(word) < 2 || word[len(word)-1] != word[0] { - return "", false - } - return word[1 : len(word)-1], true - } - if strings.ContainsAny(word, "'\"") { - return "", false - } - return word, true -} - -func isOpenSSHExecutable(executable string) bool { - lastSeparator := strings.LastIndexAny(executable, `/\`) - base := executable[lastSeparator+1:] - return base == "ssh" || base == "ssh.exe" -} - -func shellWordEnd(command string) int { - var quote byte - escaped := false - for index := 0; index < len(command); index++ { - character := command[index] - if escaped { - escaped = false - continue - } - if quote != 0 { - if quote == '"' && character == '\\' { - escaped = true - continue - } - if character == quote { - quote = 0 - } - continue - } - switch character { - case '\\': - escaped = true - case '\'', '"': - quote = character - case ' ', '\t', '\n', '\r': - return index - } - } - return len(command) -} - -func replaceGitEnvAssignment(env []string, assignment string) []string { - key, _, ok := strings.Cut(assignment, "=") - if !ok { - return env - } - prefix := key + "=" - filtered := make([]string, 0, len(env)+1) - for _, entry := range env { - if !strings.HasPrefix(entry, prefix) { - filtered = append(filtered, entry) - } - } - return append(filtered, assignment) + return subproc.PrepareGitEnvironment(env) } // gitCommand builds a git command for the already-admitted execution context. diff --git a/apps/backend/internal/agentctl/server/process/workspace_git_diff.go b/apps/backend/internal/agentctl/server/process/workspace_git_diff.go index dc52dde01b..0c056245db 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_git_diff.go +++ b/apps/backend/internal/agentctl/server/process/workspace_git_diff.go @@ -5,6 +5,7 @@ import ( "context" "io" "os" + "os/exec" "path/filepath" "regexp" "strconv" @@ -343,24 +344,20 @@ func carryForwardFileDiff(fi types.FileInfo, filePath string, update *types.GitS // pipe-based reads. Slot is acquired before Start; if Start fails we // release immediately, else release runs after Wait. func capDiffOutput(ctx context.Context, workDir string, args ...string) (string, bool) { - release, err := subproc.AcquireGit(ctx, gitWorkClass(ctx)) - if err != nil { - return "", false - } - defer release() - execCtx, cancel := context.WithTimeout(ctx, gitCommandTimeout) - defer cancel() - cmd := subproc.NewGitCommand(execCtx, args...) - cmd.Dir = workDir - - stdout, err := cmd.StdoutPipe() - if err != nil { - return "", false - } - if err := cmd.Start(); err != nil { - _ = stdout.Close() + stream, runErr, execCtxErr := subproc.StartGitStreamAfterAcquire( + ctx, + gitWorkClass(ctx), + gitCommandTimeout, + func(execCtx context.Context) *exec.Cmd { + cmd := subproc.NewGitCommand(execCtx, args...) + cmd.Dir = workDir + return cmd + }, + ) + if runErr != nil || execCtxErr != nil { return "", false } + stdout := stream.Stdout() limited := io.LimitReader(stdout, maxDiffOutputSize+1) data, _ := io.ReadAll(limited) @@ -371,7 +368,7 @@ func capDiffOutput(ctx context.Context, workDir string, args ...string) (string, // Drain remaining stdout so the process doesn't hang on a full pipe. _, _ = io.Copy(io.Discard, stdout) - _ = cmd.Wait() + _, _ = stream.Wait() return string(data), truncated } diff --git a/apps/backend/internal/agentctl/server/process/workspace_git_diff_lifecycle_unix_test.go b/apps/backend/internal/agentctl/server/process/workspace_git_diff_lifecycle_unix_test.go new file mode 100644 index 0000000000..c2329fc740 --- /dev/null +++ b/apps/backend/internal/agentctl/server/process/workspace_git_diff_lifecycle_unix_test.go @@ -0,0 +1,150 @@ +//go:build unix + +package process + +import ( + "context" + "errors" + "os" + "os/exec" + "path/filepath" + "strconv" + "strings" + "syscall" + "testing" + "time" + + "github.com/kandev/kandev/internal/common/subproc" +) + +func TestCapDiffOutputCancellationClosesReader(t *testing.T) { + restore := subproc.Git().SetCapForTest(1) + defer restore() + + dir := t.TempDir() + gitPath := filepath.Join(dir, "git") + if err := os.WriteFile(gitPath, []byte(`#!/bin/sh +if [ "$1" = diff ]; then + (trap '' TERM INT; while :; do sleep 1; done) & + printf '%s\n' "$!" > "$GIT_TEST_CHILD_PID" + printf started > "$GIT_TEST_STARTED" + printf diff-output + exit 0 +fi +printf quick +`), 0o700); err != nil { + t.Fatalf("write Git fixture: %v", err) + } + t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) + childPath := filepath.Join(dir, "child_pid") + t.Setenv("GIT_TEST_CHILD_PID", childPath) + t.Setenv("GIT_TEST_STARTED", filepath.Join(dir, "started")) + + ctx, cancel := context.WithCancel(context.Background()) + result := make(chan struct { + output string + truncated bool + }, 1) + go func() { + output, truncated := capDiffOutput(ctx, dir, "diff") + result <- struct { + output string + truncated bool + }{output, truncated} + }() + waitForProcessGitFile(t, filepath.Join(dir, "started"), time.Second) + cancel() + + select { + case value := <-result: + if value.output == "" || value.truncated { + t.Fatalf("capDiffOutput result = (%q, %t), want partial output without truncation", value.output, value.truncated) + } + case <-time.After(2 * time.Second): + t.Fatal("capDiffOutput did not return after cancellation closed its reader") + } + + childPID := readProcessGitPID(t, childPath) + waitForProcessGitPIDExit(t, childPID, time.Second) + + quickCtx, quickCancel := context.WithTimeout(context.Background(), time.Second) + defer quickCancel() + quickCmd := subproc.NewGitCommand(quickCtx, "version") + quickCmd.Env = []string{"PATH=" + os.Getenv("PATH")} + output, runErr, execErr := subproc.RunGitOutputAfterAcquire(quickCtx, subproc.GitInteractive, time.Second, func(context.Context) *exec.Cmd { + return quickCmd + }) + if runErr != nil || execErr != nil { + t.Fatalf("Git slot was not released after capDiffOutput cancellation: run=%v exec=%v output=%q", runErr, execErr, output) + } +} + +func waitForProcessGitFile(t *testing.T, path string, timeout time.Duration) { + t.Helper() + deadline := time.NewTimer(timeout) + defer deadline.Stop() + ticker := time.NewTicker(10 * time.Millisecond) + defer ticker.Stop() + for { + if _, err := os.Stat(path); err == nil { + return + } + select { + case <-ticker.C: + case <-deadline.C: + t.Fatalf("timed out waiting for %s", path) + } + } +} + +func readProcessGitPID(t *testing.T, path string) int { + t.Helper() + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read Git child PID: %v", err) + } + pid, err := strconv.Atoi(strings.TrimSpace(string(data))) + if err != nil { + t.Fatalf("parse Git child PID: %v", err) + } + return pid +} + +func waitForProcessGitPIDExit(t *testing.T, pid int, timeout time.Duration) { + t.Helper() + deadline := time.NewTimer(timeout) + defer deadline.Stop() + ticker := time.NewTicker(10 * time.Millisecond) + defer ticker.Stop() + for { + if processGitPIDExited(pid) { + return + } + select { + case <-ticker.C: + case <-deadline.C: + _ = syscall.Kill(pid, syscall.SIGKILL) + t.Fatalf("Git child %d remained alive", pid) + } + } +} + +func processGitPIDExited(pid int) bool { + err := syscall.Kill(pid, 0) + if errors.Is(err, syscall.ESRCH) { + return true + } + if err != nil { + return false + } + + // Orphaned descendants can remain as zombies briefly after the process + // group is killed. They cannot execute, although signal-zero still finds + // their PID. + data, readErr := os.ReadFile("/proc/" + strconv.Itoa(pid) + "/stat") + if readErr != nil { + return false + } + closeParen := strings.LastIndex(string(data), ")") + return closeParen >= 0 && len(data) > closeParen+2 && (data[closeParen+2] == 'Z' || data[closeParen+2] == 'X') +} diff --git a/apps/backend/internal/agentctl/server/process/workspace_git_env_test.go b/apps/backend/internal/agentctl/server/process/workspace_git_env_test.go index 91458d613b..ae0316aecb 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_git_env_test.go +++ b/apps/backend/internal/agentctl/server/process/workspace_git_env_test.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/kandev/kandev/internal/agentctl/server/config" + "github.com/kandev/kandev/internal/common/subproc" ) func TestManagerTrackerGitEnvironmentUsesInstanceEnvironment(t *testing.T) { @@ -45,8 +46,10 @@ func TestManagerTrackerGitEnvironmentUsesInstanceEnvironment(t *testing.T) { "GIT_CONFIG_VALUE_0": "Authorization: Bearer instance-token", "GIT_TERMINAL_PROMPT": "0", "GCM_INTERACTIVE": "Never", - "GIT_ASKPASS": "echo", - "SSH_ASKPASS": "/bin/false", + "GCM_GUI_PROMPT": "0", + "GIT_ASKPASS": "exit 1", + "SSH_ASKPASS": "exit 1", + "SSH_ASKPASS_REQUIRE": "never", "GIT_SSH_COMMAND": "ssh -oBatchMode=yes", } for key, wantValue := range want { @@ -134,14 +137,14 @@ func TestForceGitSSHBatchModeSupportsDirectOpenSSHOnly(t *testing.T) { tests := map[string]string{ "ssh -i /instance/key -oBatchMode=no": "ssh -oBatchMode=yes -i /instance/key -oBatchMode=no", `'path with spaces/ssh' -i key`: `'path with spaces/ssh' -oBatchMode=yes -i key`, - `env FOO=bar ssh -i key`: defaultGitSSHCommand, - `FOO=bar ssh -i key`: defaultGitSSHCommand, - `exec ssh -i key`: defaultGitSSHCommand, - `plink -i key`: defaultGitSSHCommand, + `env FOO=bar ssh -i key`: "ssh -oBatchMode=yes", + `FOO=bar ssh -i key`: "ssh -oBatchMode=yes", + `exec ssh -i key`: "ssh -oBatchMode=yes", + `plink -i key`: "ssh -oBatchMode=yes", } for command, want := range tests { - if got := forceGitSSHBatchMode(command); got != want { - t.Errorf("forceGitSSHBatchMode(%q) = %q, want %q", command, got, want) + if got := subproc.ForceGitSSHBatchMode(command); got != want { + t.Errorf("ForceGitSSHBatchMode(%q) = %q, want %q", command, got, want) } } } diff --git a/apps/backend/internal/agentctl/server/winproc/lifecycle_windows.go b/apps/backend/internal/agentctl/server/winproc/lifecycle_windows.go index 3f2daab902..bd8291d35d 100644 --- a/apps/backend/internal/agentctl/server/winproc/lifecycle_windows.go +++ b/apps/backend/internal/agentctl/server/winproc/lifecycle_windows.go @@ -1,303 +1,38 @@ //go:build windows +// Package winproc keeps the historical agentctl import path for Windows +// process lifecycle helpers. The implementation is shared with platform +// subprocess callers in internal/common/winproc. package winproc import ( - "context" - "errors" - "fmt" "os/exec" - "strings" - "sync" - "syscall" - "time" - "unsafe" - "golang.org/x/sys/windows" + commonwinproc "github.com/kandev/kandev/internal/common/winproc" ) -// KillOnCloseJob owns a Windows Job Object configured to terminate all assigned -// processes when the handle is closed. -type KillOnCloseJob struct { - state *killOnCloseJobState -} +type KillOnCloseJob = commonwinproc.KillOnCloseJob -type killOnCloseJobState struct { - mu sync.Mutex - handle windows.Handle -} - -// InstallKillOnCloseJobForSuspendedCommand assigns a suspended child process to -// a kill-on-close Job Object before resuming it. If job setup fails, the -// suspended child is terminated before it can create unowned descendants. func InstallKillOnCloseJobForSuspendedCommand(cmd *exec.Cmd) (KillOnCloseJob, error) { - if cmd == nil || cmd.Process == nil { - return KillOnCloseJob{}, fmt.Errorf("process not started") - } - return InstallKillOnCloseJobForSuspendedProcess(cmd.Process.Pid) + return commonwinproc.InstallKillOnCloseJobForSuspendedCommand(cmd) } -// InstallKillOnCloseJobForCommand assigns an already-running child process to -// a kill-on-close Job Object. Use the suspended variant when the child must not -// execute before job assignment. func InstallKillOnCloseJobForCommand(cmd *exec.Cmd) (KillOnCloseJob, error) { - if cmd == nil || cmd.Process == nil { - return KillOnCloseJob{}, fmt.Errorf("process not started") - } - return InstallKillOnCloseJobForProcess(cmd.Process.Pid) + return commonwinproc.InstallKillOnCloseJobForCommand(cmd) } func InstallKillOnCloseJobForProcess(pid int) (KillOnCloseJob, error) { - job, err := createKillOnCloseJob() - if err != nil { - return KillOnCloseJob{}, err - } - if err := assignProcessToJob(job, pid); err != nil { - _ = windows.CloseHandle(job) - return KillOnCloseJob{}, err - } - return newKillOnCloseJob(job), nil + return commonwinproc.InstallKillOnCloseJobForProcess(pid) } func InstallKillOnCloseJobForSuspendedProcess(pid int) (KillOnCloseJob, error) { - job, err := createKillOnCloseJob() - if err != nil { - return KillOnCloseJob{}, errors.Join(err, terminateSuspendedProcess(pid)) - } - if err := assignProcessToJob(job, pid); err != nil { - _ = windows.CloseHandle(job) - return KillOnCloseJob{}, errors.Join( - err, - terminateSuspendedProcess(pid), - ) - } - if err := ResumeSuspendedProcess(pid); err != nil { - _ = windows.CloseHandle(job) - return KillOnCloseJob{}, err - } - return newKillOnCloseJob(job), nil -} - -func terminateSuspendedProcess(pid int) error { - process, err := windows.OpenProcess(windows.PROCESS_TERMINATE, false, uint32(pid)) - if err != nil { - return fmt.Errorf("OpenProcess(pid=%d) for suspended cleanup: %w", pid, err) - } - defer windows.CloseHandle(process) - if err := windows.TerminateProcess(process, 1); err != nil { - return fmt.Errorf("TerminateProcess(pid=%d): %w", pid, err) - } - return nil -} - -func (j KillOnCloseJob) Close() error { - if j.state == nil { - return nil - } - j.state.mu.Lock() - defer j.state.mu.Unlock() - if j.state.handle == 0 { - return nil - } - if err := windows.CloseHandle(j.state.handle); err != nil { - return err - } - j.state.handle = 0 - return nil -} - -func (j KillOnCloseJob) RawHandle() uintptr { - if j.state == nil { - return 0 - } - j.state.mu.Lock() - defer j.state.mu.Unlock() - return uintptr(j.state.handle) -} - -// Valid reports whether this value refers to a job lifecycle, including one -// already reaped through another copy of the handle. -func (j KillOnCloseJob) Valid() bool { - return j.state != nil -} - -// TerminateAndWait terminates every process assigned to the job and waits -// until Windows reports that the job is empty before releasing its handle. -// On failure the handle remains owned so teardown can retry safely. -func (j KillOnCloseJob) TerminateAndWait(ctx context.Context) error { - if j.state == nil { - return nil - } - j.state.mu.Lock() - defer j.state.mu.Unlock() - if j.state.handle == 0 { - return nil - } - - active, err := activeJobProcesses(j.state.handle) - if err != nil { - return err - } - if active > 0 { - if err := windows.TerminateJobObject(j.state.handle, 1); err != nil { - return fmt.Errorf("TerminateJobObject: %w", err) - } - } - - ticker := time.NewTicker(10 * time.Millisecond) - defer ticker.Stop() - for active > 0 { - select { - case <-ctx.Done(): - return fmt.Errorf("wait for job processes: %w", ctx.Err()) - case <-ticker.C: - } - active, err = activeJobProcesses(j.state.handle) - if err != nil { - return err - } - } - - if err := windows.CloseHandle(j.state.handle); err != nil { - return err - } - j.state.handle = 0 - return nil -} - -type basicJobAccountingInformation struct { - TotalUserTime int64 - TotalKernelTime int64 - ThisPeriodTotalUserTime int64 - ThisPeriodTotalKernelTime int64 - TotalPageFaultCount uint32 - TotalProcesses uint32 - ActiveProcesses uint32 - TotalTerminatedProcesses uint32 -} - -func newKillOnCloseJob(handle windows.Handle) KillOnCloseJob { - return KillOnCloseJob{state: &killOnCloseJobState{handle: handle}} -} - -func activeJobProcesses(job windows.Handle) (uint32, error) { - var info basicJobAccountingInformation - if err := windows.QueryInformationJobObject( - job, - windows.JobObjectBasicAccountingInformation, - uintptr(unsafe.Pointer(&info)), - uint32(unsafe.Sizeof(info)), - nil, - ); err != nil { - return 0, fmt.Errorf("QueryInformationJobObject: %w", err) - } - return info.ActiveProcesses, nil -} - -func createKillOnCloseJob() (windows.Handle, error) { - job, err := windows.CreateJobObject(nil, nil) - if err != nil { - return 0, fmt.Errorf("CreateJobObject: %w", err) - } - info := windows.JOBOBJECT_EXTENDED_LIMIT_INFORMATION{ - BasicLimitInformation: windows.JOBOBJECT_BASIC_LIMIT_INFORMATION{ - LimitFlags: windows.JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE, - }, - } - if _, err := windows.SetInformationJobObject( - job, - windows.JobObjectExtendedLimitInformation, - uintptr(unsafe.Pointer(&info)), - uint32(unsafe.Sizeof(info)), - ); err != nil { - _ = windows.CloseHandle(job) - return 0, fmt.Errorf("SetInformationJobObject: %w", err) - } - return job, nil -} - -func assignProcessToJob(job windows.Handle, pid int) error { - procHandle, err := windows.OpenProcess( - windows.PROCESS_SET_QUOTA|windows.PROCESS_TERMINATE, - false, - uint32(pid), - ) - if err != nil { - return fmt.Errorf("OpenProcess(pid=%d): %w", pid, err) - } - defer windows.CloseHandle(procHandle) - if err := windows.AssignProcessToJobObject(job, procHandle); err != nil { - return fmt.Errorf("AssignProcessToJobObject: %w", err) - } - return nil + return commonwinproc.InstallKillOnCloseJobForSuspendedProcess(pid) } func ResumeSuspendedProcess(pid int) error { - snapshot, err := windows.CreateToolhelp32Snapshot(windows.TH32CS_SNAPTHREAD, 0) - if err != nil { - return fmt.Errorf("CreateToolhelp32Snapshot: %w", err) - } - defer windows.CloseHandle(snapshot) - - entry := windows.ThreadEntry32{Size: uint32(unsafe.Sizeof(windows.ThreadEntry32{}))} - if err := windows.Thread32First(snapshot, &entry); err != nil { - return fmt.Errorf("Thread32First: %w", err) - } - - resumed := 0 - for { - if entry.OwnerProcessID == uint32(pid) { - if err := resumeThread(entry.ThreadID); err != nil { - return err - } - resumed++ - } - if err := windows.Thread32Next(snapshot, &entry); err != nil { - if errors.Is(err, windows.ERROR_NO_MORE_FILES) { - break - } - return fmt.Errorf("Thread32Next: %w", err) - } - } - if resumed == 0 { - return fmt.Errorf("no threads found for pid %d", pid) - } - return nil + return commonwinproc.ResumeSuspendedProcess(pid) } -func resumeThread(threadID uint32) error { - thread, err := windows.OpenThread(windows.THREAD_SUSPEND_RESUME, false, threadID) - if err != nil { - return fmt.Errorf("OpenThread(thread_id=%d): %w", threadID, err) - } - defer windows.CloseHandle(thread) - if _, err := windows.ResumeThread(thread); err != nil { - return fmt.Errorf("ResumeThread(thread_id=%d): %w", threadID, err) - } - return nil -} +func RunTaskkill(args ...string) error { return commonwinproc.RunTaskkill(args...) } -func RunTaskkill(args ...string) error { - output, err := exec.Command("taskkill", args...).CombinedOutput() - if err == nil { - return nil - } - msg := strings.TrimSpace(string(output)) - if IsTaskkillMissing(msg) { - return syscall.ESRCH - } - if msg == "" { - return err - } - return fmt.Errorf("%w: %s", err, msg) -} - -func IsTaskkillMissing(msg string) bool { - if msg == "" { - return false - } - lower := strings.ToLower(msg) - return strings.Contains(lower, "not found") || - strings.Contains(lower, "not be found") || - strings.Contains(lower, "no running instance") -} +func IsTaskkillMissing(msg string) bool { return commonwinproc.IsTaskkillMissing(msg) } diff --git a/apps/backend/internal/common/subproc/git_environment.go b/apps/backend/internal/common/subproc/git_environment.go new file mode 100644 index 0000000000..1c708a4171 --- /dev/null +++ b/apps/backend/internal/common/subproc/git_environment.go @@ -0,0 +1,376 @@ +package subproc + +import ( + "os/exec" + "runtime" + "strings" +) + +const ( + gitTerminalPromptAssignment = "GIT_TERMINAL_PROMPT=0" + gcmInteractiveAssignment = "GCM_INTERACTIVE=Never" + gcmGUIPromptAssignment = "GCM_GUI_PROMPT=0" + gitAskpassAssignment = "GIT_ASKPASS=exit 1" + sshAskpassAssignment = "SSH_ASKPASS=exit 1" + sshAskpassRequireAssignment = "SSH_ASKPASS_REQUIRE=never" + defaultGitSSHCommand = "ssh -oBatchMode=yes" +) + +// PrepareGitCommand applies the final managed Git policy after a caller has +// assembled its command environment. It preserves the caller's selected +// credential and configuration scope while overriding prompt controls. +func PrepareGitCommand(cmd *exec.Cmd) { + if cmd == nil { + return + } + cmd.Env = prepareGitEnvironment(cmd.Environ()) +} + +// PrepareGitEnvironment applies the final managed Git policy to an explicit +// environment. A nil environment is treated as empty; command callers that +// need inherited values should pass os.Environ or use PrepareGitCommand. +func PrepareGitEnvironment(env []string) []string { + return prepareGitEnvironment(env) +} + +func prepareGitEnvironment(env []string) []string { + prepared := collapseEnvironmentAssignments(env) + prepared = replaceEnvironmentAssignment(prepared, gitTerminalPromptAssignment) + prepared = replaceEnvironmentAssignment(prepared, gcmInteractiveAssignment) + prepared = replaceEnvironmentAssignment(prepared, gcmGUIPromptAssignment) + prepared = replaceEnvironmentAssignment(prepared, gitAskpassAssignment) + prepared = replaceEnvironmentAssignment(prepared, sshAskpassAssignment) + prepared = replaceEnvironmentAssignment(prepared, sshAskpassRequireAssignment) + prepared = replaceEnvironmentAssignment(prepared, "GIT_SSH_COMMAND="+gitSSHCommand(prepared)) + return prepared +} + +func collapseEnvironmentAssignments(env []string) []string { + last := make(map[string]int, len(env)) + for index, entry := range env { + key, ok := environmentAssignmentKey(entry) + if ok { + last[normalizeEnvironmentKey(key)] = index + } + } + result := make([]string, 0, len(env)) + for index, entry := range env { + key, ok := environmentAssignmentKey(entry) + if ok && last[normalizeEnvironmentKey(key)] != index { + continue + } + result = append(result, entry) + } + return result +} + +func replaceEnvironmentAssignment(env []string, assignment string) []string { + key, _, ok := strings.Cut(assignment, "=") + if !ok { + return env + } + result := make([]string, 0, len(env)+1) + for _, entry := range env { + entryKey, hasValue := environmentAssignmentKey(entry) + if hasValue && environmentKeysEqual(entryKey, key) { + continue + } + result = append(result, entry) + } + return append(result, assignment) +} + +func environmentAssignmentKey(entry string) (string, bool) { + key, _, ok := strings.Cut(entry, "=") + return key, ok && key != "" +} + +func normalizeEnvironmentKey(key string) string { + if runtime.GOOS == "windows" { + return strings.ToUpper(key) + } + return key +} + +func environmentKeysEqual(left, right string) bool { + if runtime.GOOS == "windows" { + return strings.EqualFold(left, right) + } + return left == right +} + +func environmentValue(env []string, key string) (string, bool) { + for index := len(env) - 1; index >= 0; index-- { + entryKey, ok := environmentAssignmentKey(env[index]) + if !ok || !environmentKeysEqual(entryKey, key) { + continue + } + return strings.TrimPrefix(env[index], entryKey+"="), true + } + return "", false +} + +func gitSSHCommand(env []string) string { + if command, ok := environmentValue(env, "GIT_SSH_COMMAND"); ok && strings.TrimSpace(command) != "" { + return ForceGitSSHBatchMode(command) + } + if executable, ok := environmentValue(env, "GIT_SSH"); ok && strings.TrimSpace(executable) != "" { + return ForceGitSSHBatchMode(quoteSSHExecutable(executable)) + } + return defaultGitSSHCommand +} + +func quoteSSHExecutable(executable string) string { + executable = strings.TrimSpace(executable) + if executable == "" || (executable[0] == '\'' && executable[len(executable)-1] == '\'') || + (executable[0] == '"' && executable[len(executable)-1] == '"') { + return executable + } + if strings.ContainsAny(executable, " \t\n\r") { + return "'" + strings.ReplaceAll(executable, "'", "'\\''") + "'" + } + return executable +} + +// ForceGitSSHBatchMode places BatchMode=yes immediately after a direct +// OpenSSH executable. Unsupported wrappers use the safe default rather than +// receiving an option in an unknown position. +func ForceGitSSHBatchMode(command string) string { + command = strings.TrimSpace(command) + if command == "" { + return defaultGitSSHCommand + } + commandEnd := shellWordEnd(command) + if commandEnd == 0 { + return defaultGitSSHCommand + } + executable, ok := shellWordValue(command[:commandEnd]) + if !ok || !isOpenSSHExecutable(executable) { + return defaultGitSSHCommand + } + command = removeBatchModeYes(command, commandEnd) + return command[:commandEnd] + " -oBatchMode=yes" + command[commandEnd:] +} + +func removeBatchModeYes(command string, commandEnd int) string { + words, ok := parseShellWords(command, commandEnd) + if !ok { + return command + } + + removals := make([]shellRange, 0, len(words)) + for index := 0; index < len(words); index++ { + word := words[index] + end := word.end + remove := false + switch word.value { + case "-oBatchMode=yes": + // Match the decoded value so shell quoting around the option is + // removed together with the option itself. + remove = true + case "-o": + if index+1 < len(words) && words[index+1].value == "BatchMode=yes" { + end = words[index+1].end + index++ + remove = true + } + } + if !remove { + continue + } + + start := word.start + for start > commandEnd && isShellSpace(command[start-1]) { + start-- + } + removals = append(removals, shellRange{start: start, end: end}) + } + + if len(removals) == 0 { + return command + } + var result strings.Builder + result.Grow(len(command)) + cursor := 0 + for _, removal := range removals { + if removal.start < cursor { + continue + } + result.WriteString(command[cursor:removal.start]) + cursor = removal.end + } + result.WriteString(command[cursor:]) + return result.String() +} + +type shellRange struct { + start int + end int +} + +type shellWord struct { + start int + end int + value string +} + +func parseShellWords(command string, offset int) ([]shellWord, bool) { + words := make([]shellWord, 0) + for offset < len(command) { + for offset < len(command) && isShellSpace(command[offset]) { + offset++ + } + if offset == len(command) { + break + } + + word, next, ok := parseShellWord(command, offset) + if !ok { + return nil, false + } + words = append(words, word) + offset = next + } + return words, true +} + +func parseShellWord(command string, start int) (shellWord, int, bool) { + offset := start + var value strings.Builder + for offset < len(command) && !isShellSpace(command[offset]) { + next, ok := parseShellWordPart(command, offset, &value) + if !ok { + return shellWord{}, 0, false + } + offset = next + } + return shellWord{start: start, end: offset, value: value.String()}, offset, true +} + +func parseShellWordPart(command string, offset int, value *strings.Builder) (int, bool) { + switch command[offset] { + case '\'': + return parseSingleQuotedShellWordPart(command, offset, value) + case '"': + return parseDoubleQuotedShellWordPart(command, offset, value) + case '\\': + return parseEscapedShellWordPart(command, offset, value) + default: + value.WriteByte(command[offset]) + return offset + 1, true + } +} + +func parseSingleQuotedShellWordPart(command string, offset int, value *strings.Builder) (int, bool) { + offset++ + for offset < len(command) && command[offset] != '\'' { + value.WriteByte(command[offset]) + offset++ + } + if offset == len(command) { + return 0, false + } + return offset + 1, true +} + +func parseDoubleQuotedShellWordPart(command string, offset int, value *strings.Builder) (int, bool) { + offset++ + for offset < len(command) { + if command[offset] == '"' { + return offset + 1, true + } + if command[offset] == '\\' { + next, ok := parseDoubleQuotedEscape(command, offset, value) + if !ok { + return 0, false + } + offset = next + continue + } + value.WriteByte(command[offset]) + offset++ + } + return 0, false +} + +func parseDoubleQuotedEscape(command string, offset int, value *strings.Builder) (int, bool) { + if offset+1 >= len(command) { + return 0, false + } + if command[offset+1] != '\n' { + value.WriteByte(command[offset+1]) + } + return offset + 2, true +} + +func parseEscapedShellWordPart(command string, offset int, value *strings.Builder) (int, bool) { + if offset+1 >= len(command) { + return 0, false + } + if command[offset+1] != '\n' { + value.WriteByte(command[offset+1]) + } + return offset + 2, true +} + +func isShellSpace(character byte) bool { + switch character { + case ' ', '\t', '\n', '\r': + return true + default: + return false + } +} + +func shellWordValue(word string) (string, bool) { + if word == "" { + return "", false + } + if word[0] == '\'' || word[0] == '"' { + if len(word) < 2 || word[len(word)-1] != word[0] { + return "", false + } + return word[1 : len(word)-1], true + } + if strings.ContainsAny(word, "'\"") { + return "", false + } + return word, true +} + +func isOpenSSHExecutable(executable string) bool { + lastSeparator := strings.LastIndexAny(executable, `/\\`) + base := executable[lastSeparator+1:] + return base == "ssh" || base == "ssh.exe" +} + +func shellWordEnd(command string) int { + var quote byte + escaped := false + for index := 0; index < len(command); index++ { + character := command[index] + if escaped { + escaped = false + continue + } + if quote != 0 { + if quote == '"' && character == '\\' { + escaped = true + continue + } + if character == quote { + quote = 0 + } + continue + } + switch character { + case '\\': + escaped = true + case '\'', '"': + quote = character + case ' ', '\t', '\n', '\r': + return index + } + } + return len(command) +} diff --git a/apps/backend/internal/common/subproc/git_lifecycle.go b/apps/backend/internal/common/subproc/git_lifecycle.go new file mode 100644 index 0000000000..48aec38e7b --- /dev/null +++ b/apps/backend/internal/common/subproc/git_lifecycle.go @@ -0,0 +1,336 @@ +package subproc + +import ( + "bytes" + "context" + "errors" + "fmt" + "io" + "os/exec" + "sync" + "time" +) + +const ( + gitPipeWaitDelay = 500 * time.Millisecond + gitCleanupLimit = 2 * time.Second +) + +// runManagedGit owns the complete Start-to-Wait lifetime of a Git command. +// The command must already have its final environment and output wiring. +func runManagedGit(ctx context.Context, cmd *exec.Cmd) error { + process, err := startManagedGit(ctx, cmd) + if err != nil { + return err + } + return process.wait() +} + +func runManagedGitOutput(ctx context.Context, cmd *exec.Cmd) ([]byte, error) { + if cmd == nil { + return nil, errors.New("nil Git command") + } + if cmd.Stdout != nil { + return nil, errors.New("exec: Stdout already set") + } + + var stdout synchronizedBuffer + cmd.Stdout = &stdout + var stderr *synchronizedBuffer + if cmd.Stderr == nil { + stderr = &synchronizedBuffer{} + cmd.Stderr = stderr + } + process, err := startManagedGit(ctx, cmd) + if err != nil { + return nil, err + } + runErr := process.wait() + if runErr != nil && stderr != nil { + attachGitStderr(runErr, stderr.Bytes()) + } + return stdout.Bytes(), runErr +} + +func runManagedGitCombinedOutput(ctx context.Context, cmd *exec.Cmd) ([]byte, error) { + if cmd == nil { + return nil, errors.New("nil Git command") + } + if cmd.Stdout != nil { + return nil, errors.New("exec: Stdout already set") + } + if cmd.Stderr != nil { + return nil, errors.New("exec: Stderr already set") + } + + var output synchronizedBuffer + cmd.Stdout = &output + cmd.Stderr = &output + process, err := startManagedGit(ctx, cmd) + if err != nil { + return nil, err + } + runErr := process.wait() + return output.Bytes(), runErr +} + +// GitStream owns a classified Git slot and a streaming stdout pipe. The +// caller must consume Stdout and call Wait exactly once. +type GitStream struct { + process *managedGitProcess + stdout io.ReadCloser + execErr func() error + cancel context.CancelFunc + release func() + waitOnce sync.Once + waitErr error + execDone error +} + +// Stdout returns the command-owned stdout pipe. It is valid until Wait or +// Close returns. +func (s *GitStream) Stdout() io.ReadCloser { + if s == nil { + return nil + } + return s.stdout +} + +// Close closes the streaming stdout pipe. Wait must still be called to reap +// the process and release the admission slot. +func (s *GitStream) Close() error { + if s == nil || s.process == nil { + return nil + } + s.process.closePipes() + s.process.mu.Lock() + defer s.process.mu.Unlock() + return s.process.closeErr +} + +// Wait reaps the command, closes its lifecycle resources, and releases its +// Git admission slot. The second result is the post-admission execution +// context error, if any. +func (s *GitStream) Wait() (error, error) { + if s == nil { + return errors.New("nil Git stream"), nil + } + s.waitOnce.Do(func() { + s.waitErr = s.process.wait() + s.execDone = s.execErr() + s.cancel() + s.release() + }) + return s.waitErr, s.execDone +} + +// StartGitStreamAfterAcquire builds and starts a streaming Git command after +// acquiring its class slot. The slot remains held until GitStream.Wait. +func StartGitStreamAfterAcquire( + ctx context.Context, + class GitWorkClass, + execTimeout time.Duration, + build func(execCtx context.Context) *exec.Cmd, +) (*GitStream, error, error) { + release, err := gitThrottle.AcquireClass(ctx, class) + if err != nil { + return nil, wrapAdmissionError(err), nil + } + execCtx, cancel := withGitExecTimeout(ctx, execTimeout) + cmd := build(execCtx) + if cmd == nil { + cancel() + release() + return nil, errors.New("Git command builder returned nil"), execCtx.Err() + } + PrepareGitCommand(cmd) + stdout, err := cmd.StdoutPipe() + if err != nil { + cancel() + release() + return nil, fmt.Errorf("attach Git stdout pipe: %w", err), execCtx.Err() + } + process, err := startManagedGit(execCtx, cmd, stdout) + if err != nil { + _ = stdout.Close() + cancel() + release() + return nil, err, execCtx.Err() + } + return &GitStream{ + process: process, + stdout: stdout, + execErr: execCtx.Err, + cancel: cancel, + release: release, + }, nil, nil +} + +type managedGitProcess struct { + cmd *exec.Cmd + lifecycle gitLifecycleHandle + pipes []io.Closer + + finished chan struct{} + cancelDone chan struct{} + closeOnce sync.Once + waitOnce sync.Once + cancelOnce sync.Once + releaseOnce sync.Once + + mu sync.Mutex + waitErr error + cancelErr error + closeErr error + releaseErr error +} + +func startManagedGit(ctx context.Context, cmd *exec.Cmd, pipes ...io.Closer) (*managedGitProcess, error) { + if cmd == nil { + return nil, errors.New("nil Git command") + } + if err := ctx.Err(); err != nil { + return nil, err + } + PrepareGitCommand(cmd) + if err := prepareGitLifecycleCommand(cmd); err != nil { + return nil, err + } + if err := cmd.Start(); err != nil { + _ = closeGitPipes(pipes...) + return nil, err + } + lifecycle, err := installGitLifecycle(cmd) + if err != nil { + closeErr := closeGitPipes(pipes...) + reapErr := abortStartedGit(cmd) + return nil, errors.Join(fmt.Errorf("install Git lifecycle: %w", err), closeErr, reapErr) + } + process := &managedGitProcess{ + cmd: cmd, + lifecycle: lifecycle, + pipes: append([]io.Closer(nil), pipes...), + finished: make(chan struct{}), + cancelDone: make(chan struct{}), + } + go process.watchCancellation(ctx) + return process, nil +} + +func (p *managedGitProcess) watchCancellation(ctx context.Context) { + defer close(p.cancelDone) + select { + case <-ctx.Done(): + p.cancelOwnedProcess() + case <-p.finished: + // Both channels can be ready when cancellation races with process exit. + // Check the context after observing process completion so a canceled + // command still cleans up descendants that outlive its parent. + if ctx.Err() != nil { + p.cancelOwnedProcess() + } + } +} + +func (p *managedGitProcess) cancelOwnedProcess() { + p.cancelOnce.Do(func() { + cancelErr := cancelGitLifecycle(p.lifecycle) + p.mu.Lock() + p.cancelErr = errors.Join(p.cancelErr, cancelErr) + p.mu.Unlock() + p.closePipes() + p.releaseLifecycle() + }) +} + +func (p *managedGitProcess) wait() error { + p.waitOnce.Do(func() { + waitErr := p.cmd.Wait() + p.mu.Lock() + p.waitErr = waitErr + p.mu.Unlock() + close(p.finished) + // The cancellation watcher may still be finishing a platform cleanup + // operation. Waiting here keeps the returned error and admission release + // ordered after owned cleanup, including when the process exits by signal. + cleanupComplete := false + select { + case <-p.cancelDone: + cleanupComplete = true + case <-time.After(gitCleanupLimit): + p.mu.Lock() + p.cancelErr = errors.Join(p.cancelErr, errors.New("Git cancellation cleanup timed out")) + p.mu.Unlock() + } + p.closePipes() + if cleanupComplete { + p.releaseLifecycle() + } + }) + p.mu.Lock() + defer p.mu.Unlock() + return errors.Join(p.waitErr, p.cancelErr, p.closeErr, p.releaseErr) +} + +func (p *managedGitProcess) releaseLifecycle() { + p.releaseOnce.Do(func() { + releaseErr := releaseGitLifecycle(p.lifecycle) + p.mu.Lock() + p.releaseErr = errors.Join(p.releaseErr, releaseErr) + p.mu.Unlock() + }) +} + +func (p *managedGitProcess) closePipes() { + p.closeOnce.Do(func() { + var err error + for _, pipe := range p.pipes { + if pipe != nil { + err = errors.Join(err, pipe.Close()) + } + } + p.mu.Lock() + p.closeErr = err + p.mu.Unlock() + }) +} + +type synchronizedBuffer struct { + mu sync.Mutex + buffer bytes.Buffer +} + +func (b *synchronizedBuffer) Write(data []byte) (int, error) { + b.mu.Lock() + defer b.mu.Unlock() + return b.buffer.Write(data) +} + +func (b *synchronizedBuffer) Bytes() []byte { + b.mu.Lock() + defer b.mu.Unlock() + return append([]byte(nil), b.buffer.Bytes()...) +} + +func attachGitStderr(err error, stderr []byte) { + var exitErr *exec.ExitError + if errors.As(err, &exitErr) { + exitErr.Stderr = append([]byte(nil), stderr...) + } +} + +func setGitWaitDelay(cmd *exec.Cmd) { + if cmd.WaitDelay <= 0 || cmd.WaitDelay > gitPipeWaitDelay { + cmd.WaitDelay = gitPipeWaitDelay + } +} + +func closeGitPipes(pipes ...io.Closer) error { + var err error + for _, pipe := range pipes { + if pipe != nil { + err = errors.Join(err, pipe.Close()) + } + } + return err +} diff --git a/apps/backend/internal/common/subproc/git_lifecycle_other.go b/apps/backend/internal/common/subproc/git_lifecycle_other.go new file mode 100644 index 0000000000..5df957197c --- /dev/null +++ b/apps/backend/internal/common/subproc/git_lifecycle_other.go @@ -0,0 +1,39 @@ +//go:build !unix && !windows + +package subproc + +import ( + "fmt" + "os/exec" + "time" +) + +type gitLifecycleHandle struct{} + +func prepareGitLifecycleCommand(cmd *exec.Cmd) error { + setGitWaitDelay(cmd) + return nil +} + +func installGitLifecycle(*exec.Cmd) (gitLifecycleHandle, error) { + return gitLifecycleHandle{}, nil +} + +func cancelGitLifecycle(gitLifecycleHandle) error { return nil } + +func releaseGitLifecycle(gitLifecycleHandle) error { return nil } + +func abortStartedGit(cmd *exec.Cmd) error { + if cmd == nil || cmd.Process == nil { + return nil + } + _ = cmd.Process.Kill() + done := make(chan error, 1) + go func() { done <- cmd.Wait() }() + select { + case err := <-done: + return err + case <-time.After(gitCleanupLimit): + return fmt.Errorf("Git process was not reaped after startup failure") + } +} diff --git a/apps/backend/internal/common/subproc/git_lifecycle_unix.go b/apps/backend/internal/common/subproc/git_lifecycle_unix.go new file mode 100644 index 0000000000..a2cda92571 --- /dev/null +++ b/apps/backend/internal/common/subproc/git_lifecycle_unix.go @@ -0,0 +1,77 @@ +//go:build unix + +package subproc + +import ( + "errors" + "fmt" + "os/exec" + "syscall" + "time" +) + +type gitLifecycleHandle struct { + pid int +} + +func prepareGitLifecycleCommand(cmd *exec.Cmd) error { + if cmd.SysProcAttr == nil { + cmd.SysProcAttr = &syscall.SysProcAttr{} + } + attr := *cmd.SysProcAttr + if attr.Setctty || attr.Foreground || attr.Ctty != 0 { + return errors.New("Git command requests a controlling terminal") + } + if attr.Setpgid || attr.Pgid != 0 { + return errors.New("Git command requests caller-owned process-group attributes") + } + // setsid creates a new session and process group whose ID is the child + // PID. Combining it with Setpgid is rejected by the kernel on several + // Unix platforms, so the session is the ownership boundary we signal. + attr.Setsid = true + attr.Setpgid = false + attr.Pgid = 0 + cmd.SysProcAttr = &attr + setGitWaitDelay(cmd) + return nil +} + +func installGitLifecycle(cmd *exec.Cmd) (gitLifecycleHandle, error) { + return gitLifecycleHandle{pid: cmd.Process.Pid}, nil +} + +func cancelGitLifecycle(lifecycle gitLifecycleHandle) error { + if lifecycle.pid <= 0 { + return nil + } + if err := syscall.Kill(-lifecycle.pid, syscall.SIGKILL); err != nil && !errors.Is(err, syscall.ESRCH) { + return fmt.Errorf("kill Git process group %d: %w", lifecycle.pid, err) + } + return nil +} + +func releaseGitLifecycle(lifecycle gitLifecycleHandle) error { + // A Git leader can exit while a helper or hook still owns an inherited + // output pipe. Terminate the owned process group on every terminal path so + // normal exit cannot leave descendants behind until the next cancellation. + return cancelGitLifecycle(lifecycle) +} + +func abortStartedGit(cmd *exec.Cmd) error { + if cmd == nil || cmd.Process == nil { + return nil + } + _ = syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) + return waitForStartedGit(cmd) +} + +func waitForStartedGit(cmd *exec.Cmd) error { + done := make(chan error, 1) + go func() { done <- cmd.Wait() }() + select { + case err := <-done: + return err + case <-time.After(gitCleanupLimit): + return fmt.Errorf("Git process %d was not reaped after startup failure", cmd.Process.Pid) + } +} diff --git a/apps/backend/internal/common/subproc/git_lifecycle_unix_test.go b/apps/backend/internal/common/subproc/git_lifecycle_unix_test.go new file mode 100644 index 0000000000..223587628d --- /dev/null +++ b/apps/backend/internal/common/subproc/git_lifecycle_unix_test.go @@ -0,0 +1,324 @@ +//go:build unix + +package subproc + +import ( + "context" + "errors" + "os" + "os/exec" + "path/filepath" + "strconv" + "strings" + "syscall" + "testing" + "time" +) + +func TestGitHelperCancellationReleasesSlot(t *testing.T) { + restore := Git().SetCapForTest(1) + defer restore() + + fakeDir, env := writeGitLifecycleFixture(t, `#!/bin/sh +if [ "$1" = hold ]; then + (trap '' TERM INT; while :; do sleep 1; done) & + printf '%s\n' "$!" > "$GIT_TEST_CHILD_PID" + printf started > "$GIT_TEST_STARTED" + wait + exit 0 +fi +printf quick +`) + t.Setenv("PATH", fakeDir+string(os.PathListSeparator)+os.Getenv("PATH")) + + ctx, cancel := context.WithCancel(context.Background()) + result := make(chan struct { + output []byte + runErr error + execErr error + }, 1) + go func() { + output, runErr, execErr := RunGitOutputAfterAcquire(ctx, GitLifecycle, time.Second, func(execCtx context.Context) *exec.Cmd { + cmd := NewGitCommand(execCtx, "hold") + cmd.Env = append([]string(nil), env...) + return cmd + }) + result <- struct { + output []byte + runErr error + execErr error + }{output, runErr, execErr} + }() + + waitForTestFile(t, filepath.Join(fakeDir, "started"), time.Second) + cancel() + resultValue := receiveGitLifecycleResult(t, result, time.Second) + if resultValue.runErr == nil && resultValue.execErr == nil { + t.Fatal("canceled Git helper unexpectedly succeeded") + } + + quickCtx, quickCancel := context.WithTimeout(context.Background(), time.Second) + defer quickCancel() + quick, runErr, execErr := RunGitOutputAfterAcquire(quickCtx, GitLifecycle, time.Second, func(execCtx context.Context) *exec.Cmd { + cmd := NewGitCommand(execCtx, "quick") + cmd.Env = append([]string(nil), env...) + return cmd + }) + if runErr != nil || execErr != nil { + t.Fatalf("Git slot was not released after cancellation: run=%v exec=%v output=%q", runErr, execErr, quick) + } + if string(quick) != "quick" { + t.Fatalf("quick Git output = %q, want quick", quick) + } + killTestPID(t, filepath.Join(fakeDir, "child_pid")) +} + +func TestGitDescendantPipeCleanup(t *testing.T) { + fakeDir, env := writeGitLifecycleFixture(t, `#!/bin/sh +if [ "$1" = descendant ]; then + (trap '' TERM INT; while :; do sleep 1; done) & + printf '%s\n' "$!" > "$GIT_TEST_CHILD_PID" + printf started > "$GIT_TEST_STARTED" + printf parent-exited + exit 0 +fi +printf quick +`) + t.Setenv("PATH", fakeDir+string(os.PathListSeparator)+os.Getenv("PATH")) + + control := exec.Command("/bin/sh", "-c", "sleep 10") + if err := control.Start(); err != nil { + t.Fatalf("start control process: %v", err) + } + defer func() { + _ = control.Process.Kill() + _ = control.Wait() + }() + + ctx, cancel := context.WithCancel(context.Background()) + result := make(chan struct { + output []byte + runErr error + execErr error + }, 1) + go func() { + output, runErr, execErr := RunGitOutputAfterAcquire(ctx, GitLifecycle, time.Second, func(execCtx context.Context) *exec.Cmd { + cmd := NewGitCommand(execCtx, "descendant") + cmd.Env = append([]string(nil), env...) + return cmd + }) + result <- struct { + output []byte + runErr error + execErr error + }{output, runErr, execErr} + }() + + waitForTestFile(t, filepath.Join(fakeDir, "started"), time.Second) + cancel() + resultValue := receiveGitLifecycleResult(t, result, 2*time.Second) + if resultValue.runErr == nil && resultValue.execErr == nil { + t.Fatal("descendant Git command unexpectedly succeeded after cancellation") + } + + childPID := readTestPID(t, filepath.Join(fakeDir, "child_pid")) + waitForPIDExit(t, childPID, time.Second) + if err := control.Process.Signal(syscall.Signal(0)); err != nil { + t.Fatalf("unrelated control process was terminated: %v", err) + } +} + +func TestGitDescendantPipeCleanupAfterParentExit(t *testing.T) { + fakeDir, env := writeGitLifecycleFixture(t, `#!/bin/sh +if [ "$1" = descendant ]; then + (trap '' TERM INT; while :; do sleep 1; done) & + printf '%s\n' "$!" > "$GIT_TEST_CHILD_PID" + printf started > "$GIT_TEST_STARTED" + printf parent-exited + exit 0 +fi +printf quick +`) + t.Setenv("PATH", fakeDir+string(os.PathListSeparator)+os.Getenv("PATH")) + + output, runErr, execErr := RunGitOutputAfterAcquire( + context.Background(), GitLifecycle, time.Second, + func(execCtx context.Context) *exec.Cmd { + cmd := NewGitCommand(execCtx, "descendant") + cmd.Env = append([]string(nil), env...) + return cmd + }, + ) + if execErr != nil { + t.Fatalf("normal-exit Git execution context = %v", execErr) + } + if !errors.Is(runErr, exec.ErrWaitDelay) { + t.Fatalf("normal-exit Git run error = %v, want exec.ErrWaitDelay from inherited pipe", runErr) + } + if !strings.Contains(string(output), "parent-exited") { + t.Fatalf("normal-exit Git output = %q, want parent output", output) + } + childPID := readTestPID(t, filepath.Join(fakeDir, "child_pid")) + waitForPIDExit(t, childPID, time.Second) +} + +func TestGitLifecycleStartFailureReleasesSlot(t *testing.T) { + restore := Git().SetCapForTest(1) + defer restore() + + missing := filepath.Join(t.TempDir(), "missing-git") + cmd := exec.CommandContext(context.Background(), missing, "version") + if err := RunGitClass(context.Background(), GitLifecycle, cmd); err == nil { + t.Fatal("missing Git executable unexpectedly succeeded") + } + + quickCtx, cancel := context.WithTimeout(context.Background(), time.Second) + defer cancel() + output, err := RunGitOutputClass(quickCtx, GitLifecycle, exec.CommandContext(quickCtx, "/bin/echo", "released")) + if err != nil { + t.Fatalf("Git slot remained held after start failure: %v", err) + } + if string(output) != "released\n" { + t.Fatalf("post-failure output = %q", output) + } +} + +func TestGitLifecycleRejectsControllingTerminalAttributes(t *testing.T) { + for name, attr := range map[string]syscall.SysProcAttr{ + "controlling terminal": {Setctty: true}, + "caller-owned process group": {Setpgid: true}, + } { + t.Run(name, func(t *testing.T) { + cmd := NewGitCommand(context.Background(), "version") + cmd.SysProcAttr = &attr + if err := RunGitClass(context.Background(), GitLifecycle, cmd); err == nil { + t.Fatal("Git command with incompatible process attributes unexpectedly started") + } else if !strings.Contains(err.Error(), "Git command requests") { + t.Fatalf("incompatible attribute error = %v", err) + } + }) + } +} + +func writeGitLifecycleFixture(t *testing.T, script string) (string, []string) { + t.Helper() + dir := t.TempDir() + gitPath := filepath.Join(dir, "git") + if err := os.WriteFile(gitPath, []byte(script), 0o700); err != nil { + t.Fatalf("write Git fixture: %v", err) + } + return dir, []string{ + "PATH=" + dir + string(os.PathListSeparator) + os.Getenv("PATH"), + "GIT_TEST_STARTED=" + filepath.Join(dir, "started"), + "GIT_TEST_CHILD_PID=" + filepath.Join(dir, "child_pid"), + "GIT_TERMINAL_PROMPT=1", + } +} + +func receiveGitLifecycleResult(t *testing.T, result <-chan struct { + output []byte + runErr error + execErr error +}, timeout time.Duration) struct { + output []byte + runErr error + execErr error +} { + t.Helper() + select { + case value := <-result: + return value + case <-time.After(timeout): + t.Fatal("Git lifecycle did not return within the cleanup bound") + return struct { + output []byte + runErr error + execErr error + }{} + } +} + +func waitForTestFile(t *testing.T, path string, timeout time.Duration) { + t.Helper() + deadline := time.NewTimer(timeout) + defer deadline.Stop() + ticker := time.NewTicker(10 * time.Millisecond) + defer ticker.Stop() + for { + if _, err := os.Stat(path); err == nil { + return + } + select { + case <-ticker.C: + case <-deadline.C: + t.Fatalf("timed out waiting for %s", path) + } + } +} + +func readTestPID(t *testing.T, path string) int { + t.Helper() + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read child PID: %v", err) + } + pid, err := strconv.Atoi(strings.TrimSpace(string(data))) + if err != nil { + t.Fatalf("parse child PID: %v", err) + } + return pid +} + +func killTestPID(t *testing.T, path string) { + t.Helper() + if _, err := os.Stat(path); errors.Is(err, os.ErrNotExist) { + return + } + killPID(t, readTestPID(t, path)) +} + +func killPID(t *testing.T, pid int) { + t.Helper() + if err := syscall.Kill(pid, syscall.SIGKILL); err != nil && !errors.Is(err, syscall.ESRCH) { + t.Fatalf("kill test child %d: %v", pid, err) + } +} + +func waitForPIDExit(t *testing.T, pid int, timeout time.Duration) { + t.Helper() + deadline := time.NewTimer(timeout) + defer deadline.Stop() + ticker := time.NewTicker(10 * time.Millisecond) + defer ticker.Stop() + for { + if testPIDExited(pid) { + return + } + select { + case <-ticker.C: + case <-deadline.C: + killPID(t, pid) + t.Fatalf("Git descendant %d remained alive", pid) + } + } +} + +func testPIDExited(pid int) bool { + err := syscall.Kill(pid, 0) + if errors.Is(err, syscall.ESRCH) { + return true + } + if err != nil { + return false + } + + // A descendant is reparented when its Git leader exits. Some Unix test + // runners retain the killed child as a zombie briefly, so signal-zero alone + // still reports it as present even though it can no longer run. + data, readErr := os.ReadFile("/proc/" + strconv.Itoa(pid) + "/stat") + if readErr != nil { + return false + } + closeParen := strings.LastIndex(string(data), ")") + return closeParen >= 0 && len(data) > closeParen+2 && (data[closeParen+2] == 'Z' || data[closeParen+2] == 'X') +} diff --git a/apps/backend/internal/common/subproc/git_lifecycle_windows.go b/apps/backend/internal/common/subproc/git_lifecycle_windows.go new file mode 100644 index 0000000000..c314582494 --- /dev/null +++ b/apps/backend/internal/common/subproc/git_lifecycle_windows.go @@ -0,0 +1,72 @@ +//go:build windows + +package subproc + +import ( + "context" + "errors" + "fmt" + "os/exec" + "syscall" + "time" + + commonwinproc "github.com/kandev/kandev/internal/common/winproc" + "golang.org/x/sys/windows" +) + +type gitLifecycleHandle struct { + job commonwinproc.KillOnCloseJob +} + +func prepareGitLifecycleCommand(cmd *exec.Cmd) error { + if cmd.SysProcAttr == nil { + cmd.SysProcAttr = &syscall.SysProcAttr{} + } + attr := *cmd.SysProcAttr + if attr.CreationFlags&windows.CREATE_NEW_CONSOLE != 0 { + return errors.New("Git command requests a new controlling console") + } + attr.CreationFlags |= syscall.CREATE_NEW_PROCESS_GROUP | windows.CREATE_SUSPENDED + cmd.SysProcAttr = &attr + setGitWaitDelay(cmd) + return nil +} + +func installGitLifecycle(cmd *exec.Cmd) (gitLifecycleHandle, error) { + job, err := commonwinproc.InstallKillOnCloseJobForSuspendedCommand(cmd) + if err != nil { + return gitLifecycleHandle{}, err + } + return gitLifecycleHandle{job: job}, nil +} + +func cancelGitLifecycle(lifecycle gitLifecycleHandle) error { + if !lifecycle.job.Valid() { + return nil + } + ctx, cancel := context.WithTimeout(context.Background(), gitCleanupLimit) + defer cancel() + return lifecycle.job.TerminateAndWait(ctx) +} + +func releaseGitLifecycle(lifecycle gitLifecycleHandle) error { + if !lifecycle.job.Valid() { + return nil + } + return lifecycle.job.Close() +} + +func abortStartedGit(cmd *exec.Cmd) error { + if cmd == nil || cmd.Process == nil { + return nil + } + _ = cmd.Process.Kill() + done := make(chan error, 1) + go func() { done <- cmd.Wait() }() + select { + case err := <-done: + return err + case <-time.After(gitCleanupLimit): + return fmt.Errorf("Git process was not reaped after startup failure") + } +} diff --git a/apps/backend/internal/common/subproc/git_lifecycle_windows_test.go b/apps/backend/internal/common/subproc/git_lifecycle_windows_test.go new file mode 100644 index 0000000000..df812829e6 --- /dev/null +++ b/apps/backend/internal/common/subproc/git_lifecycle_windows_test.go @@ -0,0 +1,65 @@ +//go:build windows + +package subproc + +import ( + "context" + "os" + "os/exec" + "path/filepath" + "testing" + "time" +) + +func TestWindowsManagedGitJobCleanup(t *testing.T) { + dir := t.TempDir() + started := filepath.Join(dir, "started") + gitCommand := filepath.Join(dir, "git.cmd") + if err := os.WriteFile(gitCommand, []byte("@echo off\r\nif \"%1\"==\"hold\" (\r\n echo started>\"%GIT_TEST_STARTED%\"\r\n cmd.exe /c \"ping.exe 127.0.0.1 -n 100 >nul\"\r\n) else (\r\n echo quick\r\n)\r\n"), 0o700); err != nil { + t.Fatalf("write Git fixture: %v", err) + } + t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) + t.Setenv("GIT_TEST_STARTED", started) + + ctx, cancel := context.WithCancel(context.Background()) + result := make(chan error, 1) + go func() { + _, runErr, execErr := RunGitOutputAfterAcquire(ctx, GitLifecycle, 5*time.Second, func(execCtx context.Context) *exec.Cmd { + return NewGitCommand(execCtx, "hold") + }) + if runErr != nil { + result <- runErr + return + } + result <- execErr + }() + waitForWindowsGitFile(t, started, time.Second) + cancel() + + select { + case err := <-result: + if err == nil { + t.Fatal("canceled managed Git command unexpectedly succeeded") + } + case <-time.After(2 * time.Second): + t.Fatal("managed Git Job Object did not terminate the owned process tree") + } +} + +func waitForWindowsGitFile(t *testing.T, path string, timeout time.Duration) { + t.Helper() + deadline := time.NewTimer(timeout) + defer deadline.Stop() + ticker := time.NewTicker(10 * time.Millisecond) + defer ticker.Stop() + for { + if _, err := os.Stat(path); err == nil { + return + } + select { + case <-ticker.C: + case <-deadline.C: + t.Fatalf("timed out waiting for %s", path) + } + } +} diff --git a/apps/backend/internal/common/subproc/git_noninteractive_test.go b/apps/backend/internal/common/subproc/git_noninteractive_test.go new file mode 100644 index 0000000000..c08fbed98f --- /dev/null +++ b/apps/backend/internal/common/subproc/git_noninteractive_test.go @@ -0,0 +1,401 @@ +package subproc + +import ( + "bytes" + "context" + "fmt" + "net/http" + "net/http/httptest" + "os" + "os/exec" + "path/filepath" + "runtime" + "strings" + "sync/atomic" + "testing" + "time" +) + +func TestGitFinalEnvironment(t *testing.T) { + fakeGitDir := t.TempDir() + fakeGit := filepath.Join(fakeGitDir, "git") + if err := os.WriteFile(fakeGit, []byte(`#!/bin/sh +printf 'GIT_TERMINAL_PROMPT=%s\n' "${GIT_TERMINAL_PROMPT-}" +printf 'GCM_INTERACTIVE=%s\n' "${GCM_INTERACTIVE-}" +printf 'GCM_GUI_PROMPT=%s\n' "${GCM_GUI_PROMPT-}" +printf 'GIT_ASKPASS=%s\n' "${GIT_ASKPASS-}" +printf 'SSH_ASKPASS=%s\n' "${SSH_ASKPASS-}" +printf 'SSH_ASKPASS_REQUIRE=%s\n' "${SSH_ASKPASS_REQUIRE-}" +printf 'GIT_SSH_COMMAND=%s\n' "${GIT_SSH_COMMAND-}" +printf 'GIT_CONFIG_COUNT=%s\n' "${GIT_CONFIG_COUNT-}" +for i in 0 1 2 3 4 5; do + eval "key=\${GIT_CONFIG_KEY_$i-}" + eval "value=\${GIT_CONFIG_VALUE_$i-}" + if [ -n "$key" ]; then + printf 'GIT_CONFIG_%s=%s\n' "$i" "$key=$value" + fi +done +printf 'SELECTED_CREDENTIAL_DIR=%s\n' "${SELECTED_CREDENTIAL_DIR-}" +printf 'AMBIENT_ONLY=%s\n' "${AMBIENT_ONLY-}" +`), 0o700); err != nil { + t.Fatalf("write fake git: %v", err) + } + t.Setenv("PATH", fakeGitDir+string(os.PathListSeparator)+os.Getenv("PATH")) + t.Setenv("AMBIENT_ONLY", "ambient-value") + + cmd := NewGitCommand(context.Background(), "credential", "fill") + cmd.Env = []string{ + "PATH=" + os.Getenv("PATH"), + "SELECTED_CREDENTIAL_DIR=/selected/credentials", + "GIT_TERMINAL_PROMPT=1", + "GCM_INTERACTIVE=Always", + "GCM_GUI_PROMPT=1", + "GIT_ASKPASS=ambient-askpass", + "SSH_ASKPASS=ambient-ssh-askpass", + "SSH_ASKPASS_REQUIRE=force", + "GIT_SSH_COMMAND=ssh -i /selected/key -oBatchMode=no", + "GIT_CONFIG_COUNT=6", + "GIT_CONFIG_KEY_0=credential.helper", + "GIT_CONFIG_VALUE_0=", + "GIT_CONFIG_KEY_1=credential.helper", + "GIT_CONFIG_VALUE_1=!user-helper", + "GIT_CONFIG_KEY_2=credential.helper", + "GIT_CONFIG_VALUE_2=cache", + "GIT_CONFIG_KEY_3=core.hooksPath", + "GIT_CONFIG_VALUE_3=/selected/hooks", + "GIT_CONFIG_KEY_4=notes.augment.mergeStrategy", + "GIT_CONFIG_VALUE_4=union", + "GIT_CONFIG_KEY_5=credential.https://github.com.helper", + "GIT_CONFIG_VALUE_5=!kandev-host-gh-bridge", + } + + out, err := RunGitCombinedOutputClass(context.Background(), GitInteractive, cmd) + if err != nil { + t.Fatalf("run fake git: %v\n%s", err, out) + } + got := gitEnvironmentLines(string(out)) + want := map[string]string{ + "GIT_TERMINAL_PROMPT": "0", + "GCM_INTERACTIVE": "Never", + "GCM_GUI_PROMPT": "0", + "GIT_ASKPASS": "exit 1", + "SSH_ASKPASS": "exit 1", + "SSH_ASKPASS_REQUIRE": "never", + "GIT_SSH_COMMAND": "ssh -oBatchMode=yes -i /selected/key -oBatchMode=no", + "GIT_CONFIG_COUNT": "6", + "GIT_CONFIG_0": "credential.helper=", + "GIT_CONFIG_1": "credential.helper=!user-helper", + "GIT_CONFIG_2": "credential.helper=cache", + "GIT_CONFIG_3": "core.hooksPath=/selected/hooks", + "GIT_CONFIG_4": "notes.augment.mergeStrategy=union", + "GIT_CONFIG_5": "credential.https://github.com.helper=!kandev-host-gh-bridge", + "SELECTED_CREDENTIAL_DIR": "/selected/credentials", + "AMBIENT_ONLY": "", + } + for key, wantValue := range want { + if gotValue := got[key]; gotValue != wantValue { + t.Errorf("%s = %q, want %q", key, gotValue, wantValue) + } + } + if strings.Contains(string(out), "ambient-askpass") || strings.Contains(string(out), "ambient-ssh-askpass") { + t.Fatalf("inherited interactive askpass reached Git: %s", out) + } +} + +func gitEnvironmentLines(output string) map[string]string { + result := make(map[string]string) + for _, line := range strings.Split(strings.TrimSpace(output), "\n") { + key, value, ok := strings.Cut(line, "=") + if ok { + result[key] = value + } + } + return result +} + +func TestPrepareGitCommandPreservesNilAndDuplicateEnvironment(t *testing.T) { + t.Setenv("AMBIENT_ONLY", "ambient-value") + nilEnvCommand := exec.Command("git", "version") + PrepareGitCommand(nilEnvCommand) + if got, ok := environmentValue(nilEnvCommand.Env, "AMBIENT_ONLY"); !ok || got != "ambient-value" { + t.Fatalf("nil command environment lost inherited value: %q, present=%t", got, ok) + } + + env := []string{ + "PATH=" + os.Getenv("PATH"), + "SELECTED=first", + "SELECTED=last", + "GIT_CONFIG_COUNT=2", + "GIT_CONFIG_KEY_0=credential.helper", + "GIT_CONFIG_VALUE_0=", + "GIT_CONFIG_KEY_1=notes.augment.mergeStrategy", + "GIT_CONFIG_VALUE_1=union", + } + prepared := PrepareGitEnvironment(env) + selectedCount := 0 + for _, entry := range prepared { + if strings.HasPrefix(entry, "SELECTED=") { + selectedCount++ + } + } + if selectedCount != 1 { + t.Fatalf("SELECTED appears %d times, want one: %v", selectedCount, prepared) + } + if got, ok := environmentValue(prepared, "SELECTED"); !ok || got != "last" { + t.Fatalf("SELECTED = %q, present=%t, want last duplicate", got, ok) + } + if runtime.GOOS == "windows" { + prepared = append(prepared, "selected=case-insensitive-last") + prepared = collapseEnvironmentAssignments(prepared) + if got, ok := environmentValue(prepared, "SELECTED"); !ok || got != "case-insensitive-last" { + t.Fatalf("case-insensitive duplicate = %q, present=%t", got, ok) + } + } +} + +func TestGitSSHCommandPreservation(t *testing.T) { + tests := map[string]string{ + "ssh -i /selected/key -oBatchMode=no": "ssh -oBatchMode=yes -i /selected/key -oBatchMode=no", + `'path with spaces/ssh' -i key`: `'path with spaces/ssh' -oBatchMode=yes -i key`, + "ssh -oBatchMode=yes -i key": "ssh -oBatchMode=yes -i key", + "ssh -o BatchMode=yes -i key": "ssh -oBatchMode=yes -i key", + "ssh '-oBatchMode=yes' -i key": "ssh -oBatchMode=yes -i key", + "ssh -o ProxyCommand='ssh -oBatchMode=yes bastion -W %h:%p' -i key": "ssh -oBatchMode=yes -o ProxyCommand='ssh -oBatchMode=yes bastion -W %h:%p' -i key", + "env FOO=bar ssh -i key": "ssh -oBatchMode=yes", + "FOO=bar ssh -i key": "ssh -oBatchMode=yes", + "exec ssh -i key": "ssh -oBatchMode=yes", + "plink -i key": "ssh -oBatchMode=yes", + } + for command, want := range tests { + if got := ForceGitSSHBatchMode(command); got != want { + t.Errorf("ForceGitSSHBatchMode(%q) = %q, want %q", command, got, want) + } + } + + cmd := exec.Command("git", "version") + cmd.Env = []string{"GIT_SSH=/selected/path/ssh"} + PrepareGitCommand(cmd) + got, ok := environmentValue(cmd.Env, "GIT_SSH_COMMAND") + if !ok || got != "/selected/path/ssh -oBatchMode=yes" { + t.Fatalf("GIT_SSH_COMMAND = %q, present=%t, want direct GIT_SSH path with BatchMode", got, ok) + } +} + +func TestGitRunnersApplyFinalEnvironment(t *testing.T) { + fakeGitDir := t.TempDir() + fakeGit := filepath.Join(fakeGitDir, "git") + if err := os.WriteFile(fakeGit, []byte(`#!/bin/sh +printf 'GIT_TERMINAL_PROMPT=%s\n' "${GIT_TERMINAL_PROMPT-}" +printf 'GCM_INTERACTIVE=%s\n' "${GCM_INTERACTIVE-}" +printf 'GCM_GUI_PROMPT=%s\n' "${GCM_GUI_PROMPT-}" +printf 'GIT_ASKPASS=%s\n' "${GIT_ASKPASS-}" +printf 'SSH_ASKPASS=%s\n' "${SSH_ASKPASS-}" +printf 'SSH_ASKPASS_REQUIRE=%s\n' "${SSH_ASKPASS_REQUIRE-}" +`), 0o700); err != nil { + t.Fatalf("write fake git: %v", err) + } + t.Setenv("PATH", fakeGitDir+string(os.PathListSeparator)+os.Getenv("PATH")) + env := []string{ + "PATH=" + os.Getenv("PATH"), + "GIT_TERMINAL_PROMPT=1", + "GCM_INTERACTIVE=Always", + "GCM_GUI_PROMPT=1", + "GIT_ASKPASS=ambient-askpass", + "SSH_ASKPASS=ambient-ssh-askpass", + "SSH_ASKPASS_REQUIRE=force", + "GIT_SSH_COMMAND=ssh -oBatchMode=no", + } + assertPolicy := func(t *testing.T, output []byte, err error) { + t.Helper() + if err != nil { + t.Fatalf("fake git failed: %v\n%s", err, output) + } + got := gitEnvironmentLines(string(output)) + for key, want := range map[string]string{ + "GIT_TERMINAL_PROMPT": "0", + "GCM_INTERACTIVE": "Never", + "GCM_GUI_PROMPT": "0", + "GIT_ASKPASS": "exit 1", + "SSH_ASKPASS": "exit 1", + "SSH_ASKPASS_REQUIRE": "never", + } { + if got[key] != want { + t.Errorf("%s = %q, want %q", key, got[key], want) + } + } + } + + t.Run("run", func(t *testing.T) { + cmd := NewGitCommand(context.Background(), "version") + cmd.Env = append([]string(nil), env...) + var stdout bytes.Buffer + cmd.Stdout = &stdout + err := RunGitClass(context.Background(), GitLifecycle, cmd) + assertPolicy(t, stdout.Bytes(), err) + }) + + t.Run("combined-output", func(t *testing.T) { + cmd := NewGitCommand(context.Background(), "version") + cmd.Env = append([]string(nil), env...) + output, err := RunGitCombinedOutputClass(context.Background(), GitLifecycle, cmd) + assertPolicy(t, output, err) + }) + + t.Run("output", func(t *testing.T) { + cmd := NewGitCommand(context.Background(), "version") + cmd.Env = append([]string(nil), env...) + output, err := RunGitOutputClass(context.Background(), GitLifecycle, cmd) + assertPolicy(t, output, err) + }) + + t.Run("combined-after-acquire", func(t *testing.T) { + output, runErr, execErr := RunGitCombinedAfterAcquire( + context.Background(), GitLifecycle, time.Second, + func(execCtx context.Context) *exec.Cmd { + cmd := NewGitCommand(execCtx, "version") + cmd.Env = append([]string(nil), env...) + return cmd + }, + ) + if execErr != nil { + t.Fatalf("execution context: %v", execErr) + } + assertPolicy(t, output, runErr) + }) + + t.Run("output-after-acquire", func(t *testing.T) { + output, runErr, execErr := RunGitOutputAfterAcquire( + context.Background(), GitLifecycle, time.Second, + func(execCtx context.Context) *exec.Cmd { + cmd := NewGitCommand(execCtx, "version") + cmd.Env = append([]string(nil), env...) + return cmd + }, + ) + if execErr != nil { + t.Fatalf("execution context: %v", execErr) + } + assertPolicy(t, output, runErr) + }) + + t.Run("after-acquire", func(t *testing.T) { + var stdout bytes.Buffer + runErr, execErr := RunGitAfterAcquire( + context.Background(), GitLifecycle, time.Second, + func(execCtx context.Context) *exec.Cmd { + cmd := NewGitCommand(execCtx, "version") + cmd.Env = append([]string(nil), env...) + cmd.Stdout = &stdout + return cmd + }, + ) + if execErr != nil { + t.Fatalf("execution context: %v", execErr) + } + assertPolicy(t, stdout.Bytes(), runErr) + }) +} + +func TestGitCredentialHTTPSelectedScope(t *testing.T) { + root := t.TempDir() + repo := filepath.Join(root, "repo.git") + runTestGit(t, root, "init", "--bare", repo) + + var authenticated int32 + server := httptest.NewServer(http.HandlerFunc(func(response http.ResponseWriter, request *http.Request) { + username, password, ok := request.BasicAuth() + if !ok || username != "selected-user" || password != "selected-token" { + response.Header().Set("WWW-Authenticate", `Basic realm="git-test"`) + response.WriteHeader(http.StatusUnauthorized) + return + } + atomic.AddInt32(&authenticated, 1) + serveGitHTTPBackend(response, request, root) + })) + t.Cleanup(server.Close) + + host := strings.TrimPrefix(server.URL, "http://") + environment := []string{ + "PATH=" + os.Getenv("PATH"), + "GIT_CONFIG_NOSYSTEM=1", + "GIT_TERMINAL_PROMPT=1", + "GIT_CONFIG_COUNT=2", + "GIT_CONFIG_KEY_0=credential.helper", + "GIT_CONFIG_VALUE_0=!false", + "GIT_CONFIG_KEY_1=credential.http://" + host + ".helper", + "GIT_CONFIG_VALUE_1=!f() { printf 'username=selected-user\\npassword=selected-token\\n'; }; f", + } + + out, runErr, execErr := RunGitOutputAfterAcquire( + context.Background(), GitInteractive, 5*time.Second, func(execCtx context.Context) *exec.Cmd { + cmd := NewGitCommand(execCtx, "ls-remote", server.URL+"/repo.git") + cmd.Env = append([]string(nil), environment...) + return cmd + }, + ) + if runErr != nil || execErr != nil { + t.Fatalf("authenticated HTTP ls-remote failed: run=%v exec=%v\n%s", runErr, execErr, out) + } + if atomic.LoadInt32(&authenticated) == 0 { + t.Fatal("HTTP Git fixture did not observe a request with selected credentials") + } + if strings.Contains(string(out), "selected-token") { + t.Fatalf("Git output leaked selected credential: %q", out) + } +} +func runTestGit(t *testing.T, dir string, args ...string) { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + if output, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v failed: %v\n%s", args, err, output) + } +} + +func serveGitHTTPBackend(response http.ResponseWriter, request *http.Request, root string) { + cmd := exec.Command("git", "http-backend") + cmd.Env = append(os.Environ(), + "GIT_PROJECT_ROOT="+root, + "GIT_HTTP_EXPORT_ALL=1", + "PATH_INFO="+request.URL.Path, + "QUERY_STRING="+request.URL.RawQuery, + "REQUEST_METHOD="+request.Method, + "REMOTE_ADDR=127.0.0.1", + "CONTENT_TYPE="+request.Header.Get("Content-Type"), + "CONTENT_LENGTH="+request.Header.Get("Content-Length"), + ) + cmd.Stdin = request.Body + output, err := cmd.Output() + if err != nil { + response.WriteHeader(http.StatusBadGateway) + _, _ = fmt.Fprintf(response, "git http-backend failed: %v", err) + return + } + headerEnd := strings.Index(string(output), "\r\n\r\n") + delimiterLength := 4 + if headerEnd < 0 { + headerEnd = strings.Index(string(output), "\n\n") + delimiterLength = 2 + } + if headerEnd < 0 { + response.WriteHeader(http.StatusBadGateway) + return + } + for _, line := range strings.Split(string(output[:headerEnd]), "\n") { + line = strings.TrimSuffix(line, "\r") + key, value, ok := strings.Cut(line, ": ") + if !ok { + continue + } + if key == "Status" { + var status int + _, _ = fmt.Sscanf(value, "%d", &status) + if status != 0 { + response.WriteHeader(status) + } + continue + } + response.Header().Set(key, value) + } + _, _ = response.Write(output[headerEnd+delimiterLength:]) +} diff --git a/apps/backend/internal/common/subproc/git_noninteractive_unix_test.go b/apps/backend/internal/common/subproc/git_noninteractive_unix_test.go new file mode 100644 index 0000000000..2d5f851fc9 --- /dev/null +++ b/apps/backend/internal/common/subproc/git_noninteractive_unix_test.go @@ -0,0 +1,76 @@ +//go:build !windows + +package subproc + +import ( + "context" + "io" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/creack/pty" +) + +func TestManagedGitCredentialFillPTY(t *testing.T) { + home := t.TempDir() + config := filepath.Join(home, ".gitconfig") + if err := os.WriteFile(config, []byte("[credential]\n\thelper = !false\n"), 0o600); err != nil { + t.Fatalf("write isolated git config: %v", err) + } + + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + cmd := NewGitCommand(ctx, "credential", "fill") + cmd.Env = []string{ + "HOME=" + home, + "PATH=" + os.Getenv("PATH"), + "GIT_CONFIG_NOSYSTEM=1", + "GIT_TERMINAL_PROMPT=1", + "GCM_INTERACTIVE=Always", + "GCM_GUI_PROMPT=1", + "GIT_ASKPASS=", + "SSH_ASKPASS=", + } + PrepareGitCommand(cmd) + + terminal, err := pty.Start(cmd) + if err != nil { + t.Fatalf("start git under controlling PTY: %v", err) + } + t.Cleanup(func() { + _ = terminal.Close() + if cmd.Process != nil { + _ = cmd.Process.Kill() + } + }) + if _, err := terminal.Write([]byte("protocol=https\nhost=example.invalid\n\n")); err != nil { + t.Fatalf("write credential request: %v", err) + } + + type result struct { + output []byte + err error + wait error + } + finished := make(chan result, 1) + go func() { + output, readErr := io.ReadAll(terminal) + finished <- result{output: output, err: readErr, wait: cmd.Wait()} + }() + + var got result + select { + case got = <-finished: + case <-time.After(2 * time.Second): + t.Fatal("managed Git credential fill remained attached to the controlling PTY") + } + if got.wait == nil { + t.Fatal("credential fill unexpectedly succeeded without credentials") + } + if got.err != nil && !strings.Contains(got.err.Error(), "input/output error") { + t.Fatalf("read PTY output: %v", got.err) + } +} diff --git a/apps/backend/internal/common/subproc/git_noninteractive_windows_test.go b/apps/backend/internal/common/subproc/git_noninteractive_windows_test.go new file mode 100644 index 0000000000..450218d795 --- /dev/null +++ b/apps/backend/internal/common/subproc/git_noninteractive_windows_test.go @@ -0,0 +1,41 @@ +//go:build windows + +package subproc + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + "time" +) + +func TestWindowsManagedGitAskpassIsExecutedAndDenied(t *testing.T) { + marker := filepath.Join(t.TempDir(), "ambient-askpass.cmd") + if err := os.WriteFile(marker, []byte("@echo off\r\necho marker>\""+filepath.Join(t.TempDir(), "invoked")+"\"\r\nexit /b 0\r\n"), 0o700); err != nil { + t.Fatalf("write ambient askpass: %v", err) + } + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + cmd := NewGitCommand(ctx, "credential", "fill") + cmd.Env = []string{ + "PATH=" + os.Getenv("PATH"), + "GIT_CONFIG_NOSYSTEM=1", + "GIT_TERMINAL_PROMPT=1", + "GIT_ASKPASS=" + marker, + "GIT_CONFIG_COUNT=1", + "GIT_CONFIG_KEY_0=credential.helper", + "GIT_CONFIG_VALUE_0=!false", + } + cmd.Stdin = strings.NewReader("protocol=https\nhost=example.invalid\n\n") + + output, err := RunGitCombinedOutputClass(ctx, GitInteractive, cmd) + if err == nil { + t.Fatalf("credential fill unexpectedly succeeded: %s", output) + } + if !strings.Contains(strings.ToLower(string(output)), "exit 1") { + t.Fatalf("Git did not execute the managed deny-only askpass command: %s", output) + } +} diff --git a/apps/backend/internal/common/subproc/raw_git_audit_test.go b/apps/backend/internal/common/subproc/raw_git_audit_test.go index 4aeaf82969..12d2b17876 100644 --- a/apps/backend/internal/common/subproc/raw_git_audit_test.go +++ b/apps/backend/internal/common/subproc/raw_git_audit_test.go @@ -61,16 +61,37 @@ func scanGoFile(t *testing.T, path string) []string { if err != nil { t.Fatalf("parse %s: %v", path, err) } + return scanParsedGoFile(fileSet, path, file) +} + +func scanGoSource(t *testing.T, filename, source string) []string { + t.Helper() + fileSet := token.NewFileSet() + file, err := parser.ParseFile(fileSet, filename, source, 0) + if err != nil { + t.Fatalf("parse %s: %v", filename, err) + } + return scanParsedGoFile(fileSet, filename, file) +} + +func scanParsedGoFile(fileSet *token.FileSet, path string, file *ast.File) []string { var violations []string + gitCommands := make(map[string]bool) ast.Inspect(file, func(node ast.Node) bool { + markGitCommandVariables(node, gitCommands) call, ok := node.(*ast.CallExpr) - if !ok || len(call.Args) == 0 { + if !ok { return true } selector, ok := call.Fun.(*ast.SelectorExpr) if !ok { return true } + if command, ok := execPkgForGitCommand(selector, gitCommands); ok { + position := fileSet.Position(call.Pos()) + violations = append(violations, fmt.Sprintf("%s:%d: direct Git lifecycle %s", filepath.ToSlash(path), position.Line, command)) + return true + } execPkg, ok := selector.X.(*ast.Ident) if !ok { return true @@ -113,6 +134,98 @@ func scanGoFile(t *testing.T, path string) []string { return violations } +func markGitCommandVariables(node ast.Node, gitCommands map[string]bool) { + switch statement := node.(type) { + case *ast.FuncDecl: + clear(gitCommands) + case *ast.FuncLit: + clear(gitCommands) + case *ast.AssignStmt: + for index, rhs := range statement.Rhs { + if index >= len(statement.Lhs) { + continue + } + identifier, ok := statement.Lhs[index].(*ast.Ident) + if !ok { + continue + } + gitCommands[identifier.Name] = isGitCommandConstructor(rhs) + } + case *ast.ValueSpec: + for index, value := range statement.Values { + if index >= len(statement.Names) { + continue + } + gitCommands[statement.Names[index].Name] = isGitCommandConstructor(value) + } + } +} + +func isGitCommandConstructor(expr ast.Expr) bool { + call, ok := expr.(*ast.CallExpr) + if !ok { + return false + } + switch function := call.Fun.(type) { + case *ast.Ident: + return function.Name == "newGitCommand" || function.Name == "newNonInteractiveGitCmd" + case *ast.SelectorExpr: + return function.Sel.Name == "NewGitCommand" || function.Sel.Name == "newNonInteractiveGitCmd" + default: + return false + } +} + +func execPkgForGitCommand(selector *ast.SelectorExpr, gitCommands map[string]bool) (string, bool) { + if selector.Sel.Name != "Start" && selector.Sel.Name != "Wait" && + selector.Sel.Name != "Run" && selector.Sel.Name != "Output" && + selector.Sel.Name != "CombinedOutput" && selector.Sel.Name != "StdoutPipe" && + selector.Sel.Name != "StderrPipe" { + return "", false + } + identifier, ok := selector.X.(*ast.Ident) + if !ok || !gitCommands[identifier.Name] { + return "", false + } + return selector.Sel.Name, true +} + +func TestScanGoSourceRejectsDirectGitLifecycle(t *testing.T) { + violations := scanGoSource(t, "fixture.go", `package fixture + +import ( + "context" + "github.com/kandev/kandev/internal/common/subproc" +) + +func run() { + cmd := subproc.NewGitCommand(context.Background(), "status") + _ = cmd.Start() +} +`) + if len(violations) != 1 || !strings.Contains(violations[0], "direct Git lifecycle Start") { + t.Fatalf("direct Git lifecycle violations = %v", violations) + } +} + +func TestScanGoSourceAllowsClassifiedGitLifecycle(t *testing.T) { + violations := scanGoSource(t, "fixture.go", `package fixture + +import ( + "context" + "github.com/kandev/kandev/internal/common/subproc" +) + +func run() error { + cmd := subproc.NewGitCommand(context.Background(), "status") + return subproc.RunGitClass(context.Background(), subproc.GitLifecycle, cmd) +} +`) + if len(violations) != 0 { + t.Fatalf("classified Git lifecycle violations = %v", violations) + } +} + func isGitString(expr ast.Expr) bool { // NOTE: this guard intentionally recognizes direct string literals only; // keep production Git construction on the shared subproc seam rather than diff --git a/apps/backend/internal/common/subproc/shared.go b/apps/backend/internal/common/subproc/shared.go index e9e923a243..0dca1bfffe 100644 --- a/apps/backend/internal/common/subproc/shared.go +++ b/apps/backend/internal/common/subproc/shared.go @@ -102,7 +102,7 @@ func RunGitClass(ctx context.Context, class GitWorkClass, cmd *exec.Cmd) error { return err } defer release() - return cmd.Run() + return runManagedGit(ctx, cmd) } // RunGitCombinedOutputClass is RunGitClass's CombinedOutput sibling. @@ -112,7 +112,7 @@ func RunGitCombinedOutputClass(ctx context.Context, class GitWorkClass, cmd *exe return nil, err } defer release() - return cmd.CombinedOutput() + return runManagedGitCombinedOutput(ctx, cmd) } // RunGitOutputClass is RunGitClass's Output sibling. Stderr is captured in @@ -124,7 +124,7 @@ func RunGitOutputClass(ctx context.Context, class GitWorkClass, cmd *exec.Cmd) ( return nil, err } defer release() - return cmd.Output() + return runManagedGitOutput(ctx, cmd) } // RunGitCombinedAfterAcquire starts the execution timeout only after the @@ -141,9 +141,10 @@ func RunGitCombinedAfterAcquire( return nil, wrapAdmissionError(err), nil } defer release() - execCtx, cancel := withExecTimeout(ctx, execTimeout) + execCtx, cancel := withGitExecTimeout(ctx, execTimeout) defer cancel() - out, runErr := build(execCtx).CombinedOutput() + cmd := build(execCtx) + out, runErr := runManagedGitCombinedOutput(execCtx, cmd) return out, runErr, execCtx.Err() } @@ -177,9 +178,10 @@ func RunGitOutputAfterAcquireWithExecutionContext( return nil, wrapAdmissionError(err), nil } defer release() - execCtx, cancel := withExecTimeout(execBaseCtx, execTimeout) + execCtx, cancel := withGitExecTimeout(execBaseCtx, execTimeout) defer cancel() - out, runErr := build(execCtx).Output() + cmd := build(execCtx) + out, runErr := runManagedGitOutput(execCtx, cmd) return out, runErr, execCtx.Err() } @@ -195,9 +197,10 @@ func RunGitAfterAcquire( return wrapAdmissionError(err), nil } defer release() - execCtx, cancel := withExecTimeout(ctx, execTimeout) + execCtx, cancel := withGitExecTimeout(ctx, execTimeout) defer cancel() - runErr := build(execCtx).Run() + cmd := build(execCtx) + runErr := runManagedGit(execCtx, cmd) return runErr, execCtx.Err() } @@ -292,3 +295,11 @@ func withExecTimeout(ctx context.Context, execTimeout time.Duration) (context.Co } return context.WithTimeout(ctx, execTimeout) } + +// withGitExecTimeout preserves a caller's explicit Git budget. A non-positive +// budget intentionally creates an already-expired context so required probes +// fail closed; network callers must choose a positive budget before invoking +// an AfterAcquire helper. +func withGitExecTimeout(ctx context.Context, execTimeout time.Duration) (context.Context, context.CancelFunc) { + return context.WithTimeout(ctx, execTimeout) +} diff --git a/apps/backend/internal/common/winproc/lifecycle_other.go b/apps/backend/internal/common/winproc/lifecycle_other.go new file mode 100644 index 0000000000..0521ca50e8 --- /dev/null +++ b/apps/backend/internal/common/winproc/lifecycle_other.go @@ -0,0 +1,3 @@ +//go:build !windows + +package winproc diff --git a/apps/backend/internal/common/winproc/lifecycle_windows.go b/apps/backend/internal/common/winproc/lifecycle_windows.go new file mode 100644 index 0000000000..06feaf71d5 --- /dev/null +++ b/apps/backend/internal/common/winproc/lifecycle_windows.go @@ -0,0 +1,303 @@ +//go:build windows + +package winproc + +import ( + "context" + "errors" + "fmt" + "os/exec" + "strings" + "sync" + "syscall" + "time" + "unsafe" + + "golang.org/x/sys/windows" +) + +// KillOnCloseJob owns a Windows Job Object configured to terminate all assigned +// processes when the handle is closed. +type KillOnCloseJob struct { + state *killOnCloseJobState +} + +type killOnCloseJobState struct { + mu sync.Mutex + handle windows.Handle +} + +// InstallKillOnCloseJobForSuspendedCommand assigns a suspended child process to +// a kill-on-close Job Object before resuming it. If job setup fails, the +// suspended child is terminated before it can create unowned descendants. +func InstallKillOnCloseJobForSuspendedCommand(cmd *exec.Cmd) (KillOnCloseJob, error) { + if cmd == nil || cmd.Process == nil { + return KillOnCloseJob{}, fmt.Errorf("process not started") + } + return InstallKillOnCloseJobForSuspendedProcess(cmd.Process.Pid) +} + +// InstallKillOnCloseJobForCommand assigns an already-running child process to +// a kill-on-close Job Object. Use the suspended variant when the child must not +// execute before job assignment. +func InstallKillOnCloseJobForCommand(cmd *exec.Cmd) (KillOnCloseJob, error) { + if cmd == nil || cmd.Process == nil { + return KillOnCloseJob{}, fmt.Errorf("process not started") + } + return InstallKillOnCloseJobForProcess(cmd.Process.Pid) +} + +func InstallKillOnCloseJobForProcess(pid int) (KillOnCloseJob, error) { + job, err := createKillOnCloseJob() + if err != nil { + return KillOnCloseJob{}, err + } + if err := assignProcessToJob(job, pid); err != nil { + _ = windows.CloseHandle(job) + return KillOnCloseJob{}, err + } + return newKillOnCloseJob(job), nil +} + +func InstallKillOnCloseJobForSuspendedProcess(pid int) (KillOnCloseJob, error) { + job, err := createKillOnCloseJob() + if err != nil { + return KillOnCloseJob{}, errors.Join(err, terminateSuspendedProcess(pid)) + } + if err := assignProcessToJob(job, pid); err != nil { + _ = windows.CloseHandle(job) + return KillOnCloseJob{}, errors.Join( + err, + terminateSuspendedProcess(pid), + ) + } + if err := ResumeSuspendedProcess(pid); err != nil { + _ = windows.CloseHandle(job) + return KillOnCloseJob{}, errors.Join(err, terminateSuspendedProcess(pid)) + } + return newKillOnCloseJob(job), nil +} + +func terminateSuspendedProcess(pid int) error { + process, err := windows.OpenProcess(windows.PROCESS_TERMINATE, false, uint32(pid)) + if err != nil { + return fmt.Errorf("OpenProcess(pid=%d) for suspended cleanup: %w", pid, err) + } + defer windows.CloseHandle(process) + if err := windows.TerminateProcess(process, 1); err != nil { + return fmt.Errorf("TerminateProcess(pid=%d): %w", pid, err) + } + return nil +} + +func (j KillOnCloseJob) Close() error { + if j.state == nil { + return nil + } + j.state.mu.Lock() + defer j.state.mu.Unlock() + if j.state.handle == 0 { + return nil + } + if err := windows.CloseHandle(j.state.handle); err != nil { + return err + } + j.state.handle = 0 + return nil +} + +func (j KillOnCloseJob) RawHandle() uintptr { + if j.state == nil { + return 0 + } + j.state.mu.Lock() + defer j.state.mu.Unlock() + return uintptr(j.state.handle) +} + +// Valid reports whether this value refers to a job lifecycle, including one +// already reaped through another copy of the handle. +func (j KillOnCloseJob) Valid() bool { + return j.state != nil +} + +// TerminateAndWait terminates every process assigned to the job and waits +// until Windows reports that the job is empty before releasing its handle. +// On failure the handle remains owned so teardown can retry safely. +func (j KillOnCloseJob) TerminateAndWait(ctx context.Context) error { + if j.state == nil { + return nil + } + j.state.mu.Lock() + defer j.state.mu.Unlock() + if j.state.handle == 0 { + return nil + } + + active, err := activeJobProcesses(j.state.handle) + if err != nil { + return err + } + if active > 0 { + if err := windows.TerminateJobObject(j.state.handle, 1); err != nil { + return fmt.Errorf("TerminateJobObject: %w", err) + } + } + + ticker := time.NewTicker(10 * time.Millisecond) + defer ticker.Stop() + for active > 0 { + select { + case <-ctx.Done(): + return fmt.Errorf("wait for job processes: %w", ctx.Err()) + case <-ticker.C: + } + active, err = activeJobProcesses(j.state.handle) + if err != nil { + return err + } + } + + if err := windows.CloseHandle(j.state.handle); err != nil { + return err + } + j.state.handle = 0 + return nil +} + +type basicJobAccountingInformation struct { + TotalUserTime int64 + TotalKernelTime int64 + ThisPeriodTotalUserTime int64 + ThisPeriodTotalKernelTime int64 + TotalPageFaultCount uint32 + TotalProcesses uint32 + ActiveProcesses uint32 + TotalTerminatedProcesses uint32 +} + +func newKillOnCloseJob(handle windows.Handle) KillOnCloseJob { + return KillOnCloseJob{state: &killOnCloseJobState{handle: handle}} +} + +func activeJobProcesses(job windows.Handle) (uint32, error) { + var info basicJobAccountingInformation + if err := windows.QueryInformationJobObject( + job, + windows.JobObjectBasicAccountingInformation, + uintptr(unsafe.Pointer(&info)), + uint32(unsafe.Sizeof(info)), + nil, + ); err != nil { + return 0, fmt.Errorf("QueryInformationJobObject: %w", err) + } + return info.ActiveProcesses, nil +} + +func createKillOnCloseJob() (windows.Handle, error) { + job, err := windows.CreateJobObject(nil, nil) + if err != nil { + return 0, fmt.Errorf("CreateJobObject: %w", err) + } + info := windows.JOBOBJECT_EXTENDED_LIMIT_INFORMATION{ + BasicLimitInformation: windows.JOBOBJECT_BASIC_LIMIT_INFORMATION{ + LimitFlags: windows.JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE, + }, + } + if _, err := windows.SetInformationJobObject( + job, + windows.JobObjectExtendedLimitInformation, + uintptr(unsafe.Pointer(&info)), + uint32(unsafe.Sizeof(info)), + ); err != nil { + _ = windows.CloseHandle(job) + return 0, fmt.Errorf("SetInformationJobObject: %w", err) + } + return job, nil +} + +func assignProcessToJob(job windows.Handle, pid int) error { + procHandle, err := windows.OpenProcess( + windows.PROCESS_SET_QUOTA|windows.PROCESS_TERMINATE, + false, + uint32(pid), + ) + if err != nil { + return fmt.Errorf("OpenProcess(pid=%d): %w", pid, err) + } + defer windows.CloseHandle(procHandle) + if err := windows.AssignProcessToJobObject(job, procHandle); err != nil { + return fmt.Errorf("AssignProcessToJobObject: %w", err) + } + return nil +} + +func ResumeSuspendedProcess(pid int) error { + snapshot, err := windows.CreateToolhelp32Snapshot(windows.TH32CS_SNAPTHREAD, 0) + if err != nil { + return fmt.Errorf("CreateToolhelp32Snapshot: %w", err) + } + defer windows.CloseHandle(snapshot) + + entry := windows.ThreadEntry32{Size: uint32(unsafe.Sizeof(windows.ThreadEntry32{}))} + if err := windows.Thread32First(snapshot, &entry); err != nil { + return fmt.Errorf("Thread32First: %w", err) + } + + resumed := 0 + for { + if entry.OwnerProcessID == uint32(pid) { + if err := resumeThread(entry.ThreadID); err != nil { + return err + } + resumed++ + } + if err := windows.Thread32Next(snapshot, &entry); err != nil { + if errors.Is(err, windows.ERROR_NO_MORE_FILES) { + break + } + return fmt.Errorf("Thread32Next: %w", err) + } + } + if resumed == 0 { + return fmt.Errorf("no threads found for pid %d", pid) + } + return nil +} + +func resumeThread(threadID uint32) error { + thread, err := windows.OpenThread(windows.THREAD_SUSPEND_RESUME, false, threadID) + if err != nil { + return fmt.Errorf("OpenThread(thread_id=%d): %w", threadID, err) + } + defer windows.CloseHandle(thread) + if _, err := windows.ResumeThread(thread); err != nil { + return fmt.Errorf("ResumeThread(thread_id=%d): %w", threadID, err) + } + return nil +} + +func RunTaskkill(args ...string) error { + output, err := exec.Command("taskkill", args...).CombinedOutput() + if err == nil { + return nil + } + msg := strings.TrimSpace(string(output)) + if IsTaskkillMissing(msg) { + return syscall.ESRCH + } + if msg == "" { + return err + } + return fmt.Errorf("%w: %s", err, msg) +} + +func IsTaskkillMissing(msg string) bool { + if msg == "" { + return false + } + lower := strings.ToLower(msg) + return strings.Contains(lower, "not found") || + strings.Contains(lower, "not be found") || + strings.Contains(lower, "no running instance") +} diff --git a/apps/backend/internal/office/configloader/git.go b/apps/backend/internal/office/configloader/git.go index a41f79ff7f..fd57624909 100644 --- a/apps/backend/internal/office/configloader/git.go +++ b/apps/backend/internal/office/configloader/git.go @@ -16,7 +16,10 @@ import ( "go.uber.org/zap" ) -const gitCmdTimeout = 30 * time.Second +const ( + gitCmdTimeout = 30 * time.Second + gitNetworkTimeout = 5 * time.Minute +) // GitStatus holds the git status for a workspace directory. type GitStatus struct { @@ -262,7 +265,7 @@ func isAllowedRepoURL(u string) bool { // runGit executes a git command and returns combined stdout. If dir is empty, // the command runs in the current directory. func (g *GitManager) runGit(ctx context.Context, dir string, args ...string) (string, error) { - output, runErr, execCtxErr := subproc.RunGitCombinedAfterAcquire(ctx, subproc.GitLifecycle, gitCmdTimeout, func(execCtx context.Context) *exec.Cmd { + output, runErr, execCtxErr := subproc.RunGitCombinedAfterAcquire(ctx, subproc.GitLifecycle, gitOperationTimeout(args), func(execCtx context.Context) *exec.Cmd { cmd := subproc.NewGitCommand(execCtx, args...) if dir != "" { cmd.Dir = dir @@ -277,3 +280,13 @@ func (g *GitManager) runGit(ctx context.Context, dir string, args ...string) (st } return string(output), nil } + +func gitOperationTimeout(args []string) time.Duration { + if len(args) > 0 { + switch args[0] { + case "clone", "push", "submodule": + return gitNetworkTimeout + } + } + return gitCmdTimeout +} diff --git a/apps/backend/internal/office/skills/materialization.go b/apps/backend/internal/office/skills/materialization.go index 097548fd48..7bef645b0f 100644 --- a/apps/backend/internal/office/skills/materialization.go +++ b/apps/backend/internal/office/skills/materialization.go @@ -7,15 +7,21 @@ import ( "fmt" "net/url" "os" + "os/exec" "path/filepath" "strings" + "time" "github.com/kandev/kandev/internal/common/subproc" "github.com/kandev/kandev/internal/office/models" ) // skillSourceTypeGit is the source type for skills fetched from a git repository. -const skillSourceTypeGit = "git" +const ( + skillSourceTypeGit = "git" + skillGitFetchTimeout = 30 * time.Second + skillGitCloneTimeout = 5 * time.Minute +) // SkillDir represents a materialized skill directory on disk. type SkillDir struct { @@ -39,7 +45,7 @@ func (s *SkillService) MaterializeSkills(ctx context.Context, skillIDs []string, s.logger.Warn("skipping skill: " + err.Error()) continue } - sd, err := materializeSkill(skill, cacheDir, allowedRoot) + sd, err := materializeSkill(ctx, skill, cacheDir, allowedRoot) if err != nil { s.logger.Warn("failed to materialize skill " + skill.Slug + ": " + err.Error()) continue @@ -49,14 +55,14 @@ func (s *SkillService) MaterializeSkills(ctx context.Context, skillIDs []string, return dirs, nil } -func materializeSkill(skill *models.Skill, cacheDir, allowedRoot string) (SkillDir, error) { +func materializeSkill(ctx context.Context, skill *models.Skill, cacheDir, allowedRoot string) (SkillDir, error) { switch skill.SourceType { case SkillSourceTypeInline, "filesystem": return materializeInline(skill, cacheDir) case "local_path": return materializeLocalPath(skill, allowedRoot) case skillSourceTypeGit: - return materializeGit(skill, cacheDir) + return materializeGit(ctx, skill, cacheDir) default: return SkillDir{}, fmt.Errorf("unknown source type: %s", skill.SourceType) } @@ -115,7 +121,7 @@ func validateLocalPathUnderRoot(locator, allowedRoot string) error { return nil } -func materializeGit(skill *models.Skill, cacheDir string) (SkillDir, error) { +func materializeGit(ctx context.Context, skill *models.Skill, cacheDir string) (SkillDir, error) { if err := validateGitLocator(skill.SourceLocator); err != nil { return SkillDir{}, err } @@ -124,10 +130,10 @@ func materializeGit(skill *models.Skill, cacheDir string) (SkillDir, error) { return SkillDir{}, fmt.Errorf("creating git cache: %w", err) } if _, err := os.Stat(filepath.Join(repoDir, ".git")); err == nil { - if err := runGit(repoDir, "pull", "--ff-only"); err != nil { + if err := runGit(ctx, repoDir, "pull", "--ff-only"); err != nil { return SkillDir{}, err } - } else if err := runGit("", gitCloneArgs(skill.SourceLocator, repoDir)...); err != nil { + } else if err := runGit(ctx, "", gitCloneArgs(skill.SourceLocator, repoDir)...); err != nil { return SkillDir{}, err } if _, err := os.Stat(filepath.Join(repoDir, "SKILL.md")); err != nil { @@ -175,18 +181,35 @@ func hashLocator(locator string) string { return hex.EncodeToString(sum[:]) } -func runGit(dir string, args ...string) error { - cmd := subproc.NewGitCommand(context.Background(), args...) - if dir != "" { - cmd.Dir = dir +func runGit(ctx context.Context, dir string, args ...string) error { + output, runErr, execCtxErr := subproc.RunGitCombinedAfterAcquire( + ctx, + subproc.GitLifecycle, + skillGitTimeout(args), + func(execCtx context.Context) *exec.Cmd { + cmd := subproc.NewGitCommand(execCtx, args...) + if dir != "" { + cmd.Dir = dir + } + return cmd + }, + ) + if runErr == nil { + runErr = execCtxErr } - out, err := subproc.RunGitCombinedOutputClass(context.Background(), subproc.GitLifecycle, cmd) - if err != nil { - return fmt.Errorf("git %s: %w: %s", strings.Join(args, " "), err, strings.TrimSpace(string(out))) + if runErr != nil { + return fmt.Errorf("git %s: %w: %s", strings.Join(args, " "), runErr, strings.TrimSpace(string(output))) } return nil } +func skillGitTimeout(args []string) time.Duration { + if len(args) > 0 && args[0] == "clone" { + return skillGitCloneTimeout + } + return skillGitFetchTimeout +} + // SymlinkSkills creates symlinks from the agent's skill directory to skill paths. func SymlinkSkills(agentHome string, skillDirs []SkillDir) error { skillsDir := filepath.Join(agentHome, ".claude", "skills") diff --git a/apps/backend/internal/office/skills/materialization_test.go b/apps/backend/internal/office/skills/materialization_test.go index a145c4f627..7a59bb94d0 100644 --- a/apps/backend/internal/office/skills/materialization_test.go +++ b/apps/backend/internal/office/skills/materialization_test.go @@ -7,6 +7,7 @@ import ( "path/filepath" "slices" "testing" + "time" "github.com/kandev/kandev/internal/office/models" ) @@ -28,7 +29,7 @@ func TestMaterializeSkills_GitSourceClonesRepo(t *testing.T) { t.Fatalf("mkdirall: %v", err) } // Clone directly, bypassing the URL-scheme validator (unit-tested separately). - if err := runGit("", "clone", "--depth=1", src, repoDir); err != nil { + if err := runGit(context.Background(), "", "clone", "--depth=1", src, repoDir); err != nil { t.Fatalf("git clone: %v", err) } if _, err := os.Stat(filepath.Join(repoDir, "SKILL.md")); err != nil { @@ -42,7 +43,7 @@ func TestMaterializeSkills_GitSourceRejectsTraversal(t *testing.T) { SourceType: "git", SourceLocator: "../repo", } - if _, err := materializeSkill(skill, t.TempDir(), ""); err == nil { + if _, err := materializeSkill(context.Background(), skill, t.TempDir(), ""); err == nil { t.Fatal("expected path traversal locator to be rejected") } } @@ -141,6 +142,18 @@ func TestGitCloneArgs_HasEndOfOptionsSeparator(t *testing.T) { } } +func TestSkillGitTimeoutUsesCloneBudget(t *testing.T) { + if got := skillGitTimeout([]string{"clone", "--depth=1"}); got != skillGitCloneTimeout { + t.Fatalf("clone timeout = %s, want %s", got, skillGitCloneTimeout) + } + if got := skillGitTimeout([]string{"pull", "--ff-only"}); got != skillGitFetchTimeout { + t.Fatalf("pull timeout = %s, want %s", got, skillGitFetchTimeout) + } + if skillGitCloneTimeout != 5*time.Minute || skillGitFetchTimeout != 30*time.Second { + t.Fatal("skill Git budgets changed unexpectedly") + } +} + func runGitCmd(t *testing.T, dir string, args ...string) { t.Helper() cmd := exec.CommandContext(context.Background(), "git", args...) diff --git a/apps/backend/internal/orchestrator/executor/executor_host_gh_bridge.go b/apps/backend/internal/orchestrator/executor/executor_host_gh_bridge.go index 4df7badb32..0ea3032d22 100644 --- a/apps/backend/internal/orchestrator/executor/executor_host_gh_bridge.go +++ b/apps/backend/internal/orchestrator/executor/executor_host_gh_bridge.go @@ -143,6 +143,9 @@ func (e *Executor) probeHostGitHubHosts( } cancel() workers.Wait() + if err := ctx.Err(); err != nil { + return nil, err + } available := make([]string, 0, len(candidates)) for _, index := range candidates { diff --git a/apps/backend/internal/orchestrator/executor/executor_host_gh_bridge_test.go b/apps/backend/internal/orchestrator/executor/executor_host_gh_bridge_test.go index 5f9ccd0773..5f453dccae 100644 --- a/apps/backend/internal/orchestrator/executor/executor_host_gh_bridge_test.go +++ b/apps/backend/internal/orchestrator/executor/executor_host_gh_bridge_test.go @@ -14,6 +14,7 @@ import ( "time" runtimeenv "github.com/kandev/kandev/internal/agent/runtime/environment" + "github.com/kandev/kandev/internal/common/subproc" "github.com/kandev/kandev/internal/gitconfigenv" "github.com/kandev/kandev/internal/gitcredentials" "github.com/kandev/kandev/internal/task/models" @@ -101,6 +102,93 @@ exit 2 } } +func TestExecutorHostGHBridgeNoninteractiveExecution(t *testing.T) { + ghPath := filepath.Join(t.TempDir(), "host tools", "gh") + if err := os.MkdirAll(filepath.Dir(ghPath), 0o700); err != nil { + t.Fatalf("create fake gh directory: %v", err) + } + const ghScript = `#!/bin/sh +if [ "$1" = "auth" ] && [ "$2" = "token" ]; then + exit 0 +fi +if [ "$1" = "auth" ] && [ "$2" = "git-credential" ]; then + cat >/dev/null + printf 'username=x-access-token\npassword=host-token\n' + exit 0 +fi +exit 2 +` + if err := os.WriteFile(ghPath, []byte(ghScript), 0o700); err != nil { + t.Fatalf("write fake gh: %v", err) + } + t.Setenv("PATH", filepath.Dir(ghPath)+string(os.PathListSeparator)+os.Getenv("PATH")) + + executor := newTestExecutor(t, &mockAgentManager{}, newMockRepository()) + executor.SetTaskGitCredentialPolicyResolver(fakeTaskGitCredentialPolicyResolver{ + policy: TaskGitCredentialPolicy{Mode: taskGitCredentialsModeExecutor}, + }) + request := executorHostGHBridgeRequest(nil) + if err := executor.configureGitCredentialBrokerForRepositories( + context.Background(), request, + []*repoInfo{executorHostGHBridgeRepository("repo-1", "github.com")}, + ); err != nil { + t.Fatalf("configureGitCredentialBrokerForRepositories() error = %v", err) + } + + env := isolatedGitEnvironment(t, request.Env) + input := "protocol=https\nhost=github.com\npath=acme/widgets\n\n" + output, runErr, execCtxErr := subproc.RunGitCombinedAfterAcquire( + context.Background(), + subproc.GitInteractive, + 5*time.Second, + func(execCtx context.Context) *osExec.Cmd { + cmd := osExec.CommandContext(execCtx, "git", "credential", "fill") + cmd.Env = env + cmd.Stdin = strings.NewReader(input) + return cmd + }, + ) + if runErr != nil || execCtxErr != nil { + t.Fatalf("final Git runner error = (%v, %v), output = %q", runErr, execCtxErr, output) + } + if got := string(output); !strings.Contains(got, "username=x-access-token") || + !strings.Contains(got, "password=host-token") { + t.Fatalf("final Git runner output = %q, want routed host credentials", got) + } +} + +func TestExecutorHostGHBridgeRecoveryAfterUnavailableProbe(t *testing.T) { + initialPath := t.TempDir() + t.Setenv("PATH", initialPath) + executor := newTestExecutor(t, &mockAgentManager{}, newMockRepository()) + executor.SetTaskGitCredentialPolicyResolver(fakeTaskGitCredentialPolicyResolver{ + policy: TaskGitCredentialPolicy{Mode: taskGitCredentialsModeExecutor}, + }) + request := executorHostGHBridgeRequest(nil) + repositories := []*repoInfo{executorHostGHBridgeRepository("repo-1", "github.com")} + if err := executor.configureGitCredentialBrokerForRepositories(context.Background(), request, repositories); err != nil { + t.Fatalf("unavailable probe configuration: %v", err) + } + if hasHostGitHubHelper(request.Env) { + t.Fatalf("unavailable probe installed a host helper: %#v", request.Env) + } + + ghPath := filepath.Join(t.TempDir(), "host tools", "gh") + if err := os.MkdirAll(filepath.Dir(ghPath), 0o700); err != nil { + t.Fatalf("create recovered gh directory: %v", err) + } + if err := os.WriteFile(ghPath, []byte("#!/bin/sh\nexit 0\n"), 0o700); err != nil { + t.Fatalf("write recovered gh: %v", err) + } + t.Setenv("PATH", filepath.Dir(ghPath)+string(os.PathListSeparator)+initialPath) + if err := executor.configureGitCredentialBrokerForRepositories(context.Background(), request, repositories); err != nil { + t.Fatalf("recovered probe configuration: %v", err) + } + if !hasHostGitHubHelper(request.Env) { + t.Fatalf("recovered probe did not install a host helper: %#v", request.Env) + } +} + func TestExecutorHostGHBridgePreflightComposesIndexedProfileBlock(t *testing.T) { executor := newTestExecutor(t, &mockAgentManager{}, newMockRepository()) req := executorHostGHBridgeRequest(indexedGitConfigTestEnvironment( diff --git a/apps/backend/internal/repoclone/clone.go b/apps/backend/internal/repoclone/clone.go index 6452cfe7cf..b5e880241b 100644 --- a/apps/backend/internal/repoclone/clone.go +++ b/apps/backend/internal/repoclone/clone.go @@ -14,6 +14,7 @@ import ( "regexp" "strings" "sync" + "time" "go.uber.org/zap" @@ -35,6 +36,8 @@ const ( managedWorkspacesDir = "workspaces" providerCloneDir = "_providers" maxGitDiagnosticBytes = 4096 + gitFetchTimeout = 30 * time.Second + gitCloneTimeout = 5 * time.Minute ) var ( @@ -508,13 +511,9 @@ func (c *Cloner) refreshWorkspaceRepository( if refspec != "" { args = append(args, refspec) } - cmd := subproc.NewGitCommand(ctx, args...) - cleanup, err := configureGitCommand(cmd, auth) - if err != nil { - return err - } - defer cleanup() - if out, runErr := subproc.RunGitCombinedOutputClass(ctx, subproc.GitLifecycle, cmd); runErr != nil { + if out, runErr := c.runConfiguredGitCombined(ctx, gitFetchTimeout, args, + func(cmd *exec.Cmd) (func(), error) { return configureGitCommand(cmd, auth) }, + ); runErr != nil { return fmt.Errorf("refresh scoped workspace repository: %s: %w", redactCloneOutput(string(out), authToken(auth)), runErr) } @@ -789,16 +788,10 @@ func gitCredentialOrigin(cloneURL string) (string, error) { func (c *Cloner) fetch(ctx context.Context, repoPath string, auth *cloneAuth) { c.logger.Debug("repository already cloned, fetching", zap.String("path", repoPath)) - cmd := subproc.NewGitCommand( - ctx, "-C", repoPath, "fetch", "--all", "--prune", "--force", gitNoTags, - ) - cleanup, err := configureGitCommand(cmd, auth) - if err != nil { - c.logger.Warn("configure Git fetch credentials failed", zap.String("path", repoPath), zap.Error(err)) - return - } - defer cleanup() - if out, err := subproc.RunGitCombinedOutputClass(ctx, subproc.GitLifecycle, cmd); err != nil { + args := []string{"-C", repoPath, "fetch", "--all", "--prune", "--force", gitNoTags} + if out, err := c.runConfiguredGitCombined(ctx, gitFetchTimeout, args, + func(cmd *exec.Cmd) (func(), error) { return configureGitCommand(cmd, auth) }, + ); err != nil { c.logger.Warn("git fetch failed (non-fatal)", zap.String("path", repoPath), zap.String("output", redactCloneOutput(string(out), authToken(auth))), zap.Error(err)) } @@ -814,13 +807,9 @@ func (c *Cloner) clone(ctx context.Context, cloneURL, targetPath string, auth *c args = append(args, "--filter=blob:none") } args = append(args, gitNoTags, "--", cloneURL, targetPath) - cmd := subproc.NewGitCommand(ctx, args...) - cleanup, err := configureGitCommand(cmd, auth) - if err != nil { - return err - } - defer cleanup() - if out, err := subproc.RunGitCombinedOutputClass(ctx, subproc.GitLifecycle, cmd); err != nil { + if out, err := c.runConfiguredGitCombined(ctx, gitCloneTimeout, args, + func(cmd *exec.Cmd) (func(), error) { return configureGitCommand(cmd, auth) }, + ); err != nil { return fmt.Errorf("git clone failed: %s: %w", redactCloneOutput(string(out), authToken(auth)), err) } return nil @@ -828,11 +817,13 @@ func (c *Cloner) clone(ctx context.Context, cloneURL, targetPath string, auth *c func (c *Cloner) fetchWithHTTPHeader(ctx context.Context, repoPath, authURL, header string) { c.logger.Debug("repository already cloned, fetching", zap.String("path", repoPath)) - cmd := subproc.NewGitCommand( - ctx, "-C", repoPath, "fetch", "--all", "--prune", "--force", gitNoTags, - ) - configureHTTPHeaderCommand(cmd, authURL, header) - if out, err := subproc.RunGitCombinedOutputClass(ctx, subproc.GitLifecycle, cmd); err != nil { + args := []string{"-C", repoPath, "fetch", "--all", "--prune", "--force", gitNoTags} + if out, err := c.runConfiguredGitCombined(ctx, gitFetchTimeout, args, + func(cmd *exec.Cmd) (func(), error) { + configureHTTPHeaderCommand(cmd, authURL, header) + return func() {}, nil + }, + ); err != nil { c.logger.Warn("authenticated git fetch failed (non-fatal)", zap.String("path", repoPath), zap.String("output", string(out)), zap.Error(err)) } @@ -843,16 +834,49 @@ func (c *Cloner) cloneWithHTTPHeader(ctx context.Context, cloneURL, targetPath, return fmt.Errorf("create parent directory: %w", err) } c.logger.Info("cloning authenticated repository", zap.String("url", redactCloneURL(cloneURL)), zap.String("target", targetPath)) - cmd := subproc.NewGitCommand( - ctx, "clone", "--filter=blob:none", gitNoTags, "--", cloneURL, targetPath, - ) - configureHTTPHeaderCommand(cmd, cloneURL, header) - if out, err := subproc.RunGitCombinedOutputClass(ctx, subproc.GitLifecycle, cmd); err != nil { + args := []string{"clone", "--filter=blob:none", gitNoTags, "--", cloneURL, targetPath} + if out, err := c.runConfiguredGitCombined(ctx, gitCloneTimeout, args, + func(cmd *exec.Cmd) (func(), error) { + configureHTTPHeaderCommand(cmd, cloneURL, header) + return func() {}, nil + }, + ); err != nil { return fmt.Errorf("git clone failed: %s: %w", string(out), err) } return nil } +func (c *Cloner) runConfiguredGitCombined( + ctx context.Context, + execTimeout time.Duration, + args []string, + configure func(*exec.Cmd) (func(), error), +) ([]byte, error) { + template := subproc.NewGitCommand(ctx, args...) + cleanup, err := configure(template) + if err != nil { + return nil, err + } + defer cleanup() + + output, runErr, execCtxErr := subproc.RunGitCombinedAfterAcquire( + ctx, + subproc.GitLifecycle, + execTimeout, + func(execCtx context.Context) *exec.Cmd { + cmd := subproc.NewGitCommand(execCtx, args...) + cmd.Dir = template.Dir + cmd.Env = append([]string(nil), template.Env...) + cmd.ExtraFiles = append([]*os.File(nil), template.ExtraFiles...) + return cmd + }, + ) + if runErr == nil { + runErr = execCtxErr + } + return output, runErr +} + func configureGitCommand(cmd *exec.Cmd, auth *cloneAuth) (func(), error) { env := cleanGitEnvironment() if auth == nil { diff --git a/apps/backend/internal/repoclone/clone_auth_test.go b/apps/backend/internal/repoclone/clone_auth_test.go index 09a5da9e09..c23ed27251 100644 --- a/apps/backend/internal/repoclone/clone_auth_test.go +++ b/apps/backend/internal/repoclone/clone_auth_test.go @@ -98,6 +98,30 @@ func TestEnsureWorkspaceClonedWithBasicAuthKeepsCredentialScopedToGitChild(t *te } } +func TestCloneDeadlineAfterAdmission(t *testing.T) { + binDir := t.TempDir() + fakeGit := filepath.Join(binDir, "git") + if err := os.WriteFile(fakeGit, []byte("#!/bin/sh\nsleep 10\n"), 0o755); err != nil { + t.Fatalf("write fake git: %v", err) + } + t.Setenv("PATH", binDir+string(os.PathListSeparator)+os.Getenv("PATH")) + + cloner := NewCloner(Config{BasePath: t.TempDir()}, ProtocolHTTPS, "", logger.Default()) + ctx, cancel := context.WithTimeout(context.Background(), 75*time.Millisecond) + defer cancel() + started := time.Now() + err := cloner.clone(ctx, "https://example.test/acme/widgets.git", filepath.Join(t.TempDir(), "widgets"), nil) + if err == nil { + t.Fatal("clone() error = nil, want deadline failure") + } + if ctx.Err() == nil { + t.Fatal("clone() returned before the caller context expired") + } + if elapsed := time.Since(started); elapsed > 2*time.Second { + t.Fatalf("clone() took %s after cancellation", elapsed) + } +} + func TestAuthenticatedCloneDoesNotLeavePromisorCheckout(t *testing.T) { binDir := t.TempDir() capturePath := filepath.Join(t.TempDir(), "git-args") diff --git a/apps/backend/internal/repoclone/remote_state.go b/apps/backend/internal/repoclone/remote_state.go index f5207c9b98..64e27cb3d4 100644 --- a/apps/backend/internal/repoclone/remote_state.go +++ b/apps/backend/internal/repoclone/remote_state.go @@ -4,7 +4,10 @@ import ( "bytes" "context" "fmt" + "os" + "os/exec" "strings" + "time" "github.com/kandev/kandev/internal/common/subproc" ) @@ -42,17 +45,12 @@ func (c *Cloner) InspectLocalRepositoryRemoteRefState( if strings.TrimSpace(repositoryPath) == "" { return RemoteRefStateUnknown, fmt.Errorf("repository path is required") } - cmd := subproc.NewGitCommand(ctx, "-C", repositoryPath, "ls-remote", "--refs", "origin") - cleanup, err := configureGitCommand(cmd, nil) + output, stderr, err := runConfiguredGitOutput(ctx, gitFetchTimeout, + []string{"-C", repositoryPath, "ls-remote", "--refs", "origin"}, + func(cmd *exec.Cmd) (func(), error) { return configureGitCommand(cmd, nil) }, + ) if err != nil { - return RemoteRefStateUnknown, err - } - defer cleanup() - var stderr bytes.Buffer - cmd.Stderr = &stderr - output, err := subproc.RunGitOutputClass(ctx, subproc.GitLifecycle, cmd) - if err != nil { - diagnostic := redactCloneOutput(stderr.String(), "") + diagnostic := redactCloneOutput(stderr, "") return RemoteRefStateUnknown, fmt.Errorf("inspect local repository refs: %s: %w", strings.TrimSpace(diagnostic), err) } @@ -60,23 +58,51 @@ func (c *Cloner) InspectLocalRepositoryRemoteRefState( } func (c *Cloner) remoteRefState(ctx context.Context, cloneURL string, auth *cloneAuth) (RemoteRefState, error) { - cmd := subproc.NewGitCommand(ctx, "ls-remote", "--refs", "--", cloneURL) - cleanup, err := configureGitCommand(cmd, auth) + output, stderr, err := runConfiguredGitOutput(ctx, gitFetchTimeout, + []string{"ls-remote", "--refs", "--", cloneURL}, + func(cmd *exec.Cmd) (func(), error) { return configureGitCommand(cmd, auth) }, + ) if err != nil { - return RemoteRefStateUnknown, err - } - defer cleanup() - var stderr bytes.Buffer - cmd.Stderr = &stderr - output, err := subproc.RunGitOutputClass(ctx, subproc.GitLifecycle, cmd) - if err != nil { - diagnostic := redactCloneOutput(stderr.String(), authToken(auth)) + diagnostic := redactCloneOutput(stderr, authToken(auth)) return RemoteRefStateUnknown, fmt.Errorf("inspect remote refs: %s: %w", strings.TrimSpace(diagnostic), err) } return parseRemoteRefState(string(output)) } +func runConfiguredGitOutput( + ctx context.Context, + execTimeout time.Duration, + args []string, + configure func(*exec.Cmd) (func(), error), +) ([]byte, string, error) { + template := subproc.NewGitCommand(ctx, args...) + cleanup, err := configure(template) + if err != nil { + return nil, "", err + } + defer cleanup() + + var stderr bytes.Buffer + output, runErr, execCtxErr := subproc.RunGitOutputAfterAcquire( + ctx, + subproc.GitLifecycle, + execTimeout, + func(execCtx context.Context) *exec.Cmd { + cmd := subproc.NewGitCommand(execCtx, args...) + cmd.Dir = template.Dir + cmd.Env = append([]string(nil), template.Env...) + cmd.ExtraFiles = append([]*os.File(nil), template.ExtraFiles...) + cmd.Stderr = &stderr + return cmd + }, + ) + if runErr == nil { + runErr = execCtxErr + } + return output, stderr.String(), runErr +} + func parseRemoteRefState(output string) (RemoteRefState, error) { hasRef := false for _, line := range strings.Split(strings.TrimSpace(output), "\n") { diff --git a/apps/backend/internal/task/service/repository_fetch.go b/apps/backend/internal/task/service/repository_fetch.go index 25d3c0e80c..19599bee08 100644 --- a/apps/backend/internal/task/service/repository_fetch.go +++ b/apps/backend/internal/task/service/repository_fetch.go @@ -126,14 +126,7 @@ func (f *branchFetcher) runFetch(ctx context.Context, repoPath string) BranchRef // failures rather than hanging the dropdown. func newNonInteractiveGitFetchCmd(ctx context.Context, repoPath string) *exec.Cmd { cmd := subproc.NewGitCommand(ctx, "-C", repoPath, "fetch", "--all", "--prune", "--no-tags") - cmd.Env = append(os.Environ(), - "GIT_TERMINAL_PROMPT=0", - "GCM_INTERACTIVE=Never", - "GIT_ASKPASS=echo", - "SSH_ASKPASS=/bin/false", - "GIT_SSH_COMMAND=ssh -oBatchMode=yes", - ) - cmd.WaitDelay = 500 * time.Millisecond + cmd.Env = os.Environ() return cmd } diff --git a/apps/backend/internal/worktree/manager_git.go b/apps/backend/internal/worktree/manager_git.go index 58242aecb7..51e5de2b46 100644 --- a/apps/backend/internal/worktree/manager_git.go +++ b/apps/backend/internal/worktree/manager_git.go @@ -45,27 +45,16 @@ func (m *Manager) isGitRepo(path string) bool { // "missing branch" from a "could not tell" and avoid surfacing a // misleading ErrInvalidBaseBranch. func (m *Manager) branchExists(ctx context.Context, repoPath, branch string) (bool, error) { - // Acquire the throttle slot FIRST, then start the inspectTimeout - // timer. Building inspectCtx before Acquire (as we did originally) - // let throttle queue time eat through the 10s budget under load, - // producing 70s-lock-held / signal:killed cascades under git-pool - // contention. With this ordering the 10s timer starts the moment - // git is about to run, so we get an accurate "could not tell" only - // when the inspect itself is the slow part. - release, err := subproc.AcquireGit(ctx, subproc.GitLifecycle) - if err != nil { - m.logger.Warn("branchExists bounded by context", - zap.String("repository_path", repoPath), - zap.String("branch", branch), - zap.Error(err)) - return false, fmt.Errorf("branch check timed out for %q before throttle acquire: %w", branch, err) - } - defer release() - inspectCtx, cancel := context.WithTimeout(ctx, m.inspectTimeout) - defer cancel() - cmd := m.newNonInteractiveGitCmd(inspectCtx, repoPath, "rev-parse", "--verify", branch) - if err := cmd.Run(); err != nil { - if ctxErr := inspectCtx.Err(); ctxErr != nil { + runErr, execCtxErr := subproc.RunGitAfterAcquire( + ctx, + subproc.GitLifecycle, + m.inspectTimeout, + func(execCtx context.Context) *exec.Cmd { + return m.newNonInteractiveGitCmd(execCtx, repoPath, "rev-parse", "--verify", branch) + }, + ) + if runErr != nil { + if ctxErr := firstContextError(execCtxErr, runErr); ctxErr != nil { m.logger.Warn("branchExists bounded by context", zap.String("repository_path", repoPath), zap.String("branch", branch), @@ -124,17 +113,15 @@ func normalizeOriginBranchName(branch string) string { // acquiring the lifecycle throttle. The timeout starts after admission so // queue wait does not consume the command's inspection budget. func (m *Manager) runBoundedGitInspect(ctx context.Context, repoPath string, args ...string) (string, error) { - release, err := subproc.AcquireGit(ctx, subproc.GitLifecycle) - if err != nil { - return "", err - } - defer release() - - inspectCtx, cancel := context.WithTimeout(ctx, m.inspectTimeout) - defer cancel() - cmd := m.newNonInteractiveGitCmd(inspectCtx, repoPath, args...) - output, runErr := cmd.CombinedOutput() - if ctxErr := inspectCtx.Err(); ctxErr != nil { + output, runErr, execCtxErr := subproc.RunGitCombinedAfterAcquire( + ctx, + subproc.GitLifecycle, + m.inspectTimeout, + func(execCtx context.Context) *exec.Cmd { + return m.newNonInteractiveGitCmd(execCtx, repoPath, args...) + }, + ) + if ctxErr := firstContextError(execCtxErr, runErr); ctxErr != nil { return string(output), fmt.Errorf("git inspection timed out: %w", ctxErr) } return string(output), runErr @@ -251,13 +238,18 @@ func (m *Manager) prepareBranchFromRefreshedOrigin( return "", fmt.Errorf("required fetched remote ref %q is missing", remoteRef) } if !localExists { - release, err := subproc.AcquireGit(ctx, subproc.GitLifecycle) - if err != nil { - return "", err - } - defer release() - cmd := m.newNonInteractiveGitCmd(ctx, repoPath, "branch", "--track", localBranch, remoteRef) - if output, runErr := cmd.CombinedOutput(); runErr != nil { + output, runErr, execCtxErr := subproc.RunGitCombinedAfterAcquire( + ctx, + subproc.GitLifecycle, + m.inspectTimeout, + func(execCtx context.Context) *exec.Cmd { + return m.newNonInteractiveGitCmd(execCtx, repoPath, "branch", "--track", localBranch, remoteRef) + }, + ) + if runErr != nil { + if ctxErr := firstContextError(execCtxErr, runErr); ctxErr != nil { + return "", ctxErr + } return "", fmt.Errorf("create local branch %q from refreshed origin: %s: %w", localBranch, strings.TrimSpace(string(output)), runErr) } @@ -299,22 +291,15 @@ func (m *Manager) BranchRecoveryStatus(ctx context.Context, repoPath, branch str // A non-zero ancestry result is distinct from a failed probe: the former is a // proven negative, while the latter must stop required refresh preparation. func (m *Manager) refContains(ctx context.Context, repoPath, container, contained string) (bool, error) { - // Same Acquire-then-build-execCtx ordering as branchExists. - release, err := subproc.AcquireGit(ctx, subproc.GitLifecycle) - if err != nil { - m.logger.Warn("refContains bounded by context before throttle acquire", - zap.String("repository_path", repoPath), - zap.String("container", container), - zap.String("contained", contained), - zap.Error(err)) - return false, fmt.Errorf("acquire Git ancestry check: %w", err) - } - defer release() - inspectCtx, cancel := context.WithTimeout(ctx, m.inspectTimeout) - defer cancel() - cmd := m.newNonInteractiveGitCmd(inspectCtx, repoPath, "merge-base", "--is-ancestor", contained, container) - runErr := cmd.Run() - if ctxErr := inspectCtx.Err(); ctxErr != nil { + runErr, execCtxErr := subproc.RunGitAfterAcquire( + ctx, + subproc.GitLifecycle, + m.inspectTimeout, + func(execCtx context.Context) *exec.Cmd { + return m.newNonInteractiveGitCmd(execCtx, repoPath, "merge-base", "--is-ancestor", contained, container) + }, + ) + if ctxErr := firstContextError(execCtxErr, runErr); ctxErr != nil { return false, fmt.Errorf("git ancestry check timed out: %w", ctxErr) } if runErr == nil { @@ -328,18 +313,16 @@ func (m *Manager) refContains(ctx context.Context, repoPath, container, containe } func (m *Manager) currentBranch(ctx context.Context, repoPath string) string { - // Same Acquire-then-build-execCtx ordering as branchExists. - release, err := subproc.AcquireGit(ctx, subproc.GitLifecycle) - if err != nil { - return "" - } - defer release() - inspectCtx, cancel := context.WithTimeout(ctx, m.inspectTimeout) - defer cancel() - cmd := m.newNonInteractiveGitCmd(inspectCtx, repoPath, "rev-parse", "--abbrev-ref", "HEAD") - output, runErr := cmd.Output() + output, runErr, execCtxErr := subproc.RunGitOutputAfterAcquire( + ctx, + subproc.GitLifecycle, + m.inspectTimeout, + func(execCtx context.Context) *exec.Cmd { + return m.newNonInteractiveGitCmd(execCtx, repoPath, "rev-parse", "--abbrev-ref", "HEAD") + }, + ) if runErr != nil { - if ctxErr := inspectCtx.Err(); ctxErr != nil { + if ctxErr := firstContextError(execCtxErr, runErr); ctxErr != nil { m.logger.Warn("currentBranch bounded by context", zap.String("repository_path", repoPath), zap.Error(ctxErr)) @@ -352,13 +335,7 @@ func (m *Manager) currentBranch(ctx context.Context, repoPath string) string { func (m *Manager) newNonInteractiveGitCmd(ctx context.Context, repoPath string, args ...string) *exec.Cmd { cmd := newGitCommand(ctx, args...) cmd.Dir = repoPath - cmd.Env = append(os.Environ(), - "GIT_TERMINAL_PROMPT=0", - "GCM_INTERACTIVE=Never", - "GIT_ASKPASS=echo", - "SSH_ASKPASS=/bin/false", - "GIT_SSH_COMMAND=ssh -oBatchMode=yes", - ) + cmd.Env = subproc.PrepareGitEnvironment(os.Environ()) // After the context cancels and the process is killed, child processes // (e.g. credential helpers) may still hold stdout/stderr pipes open. // WaitDelay bounds how long CombinedOutput waits for those pipes to close. @@ -740,14 +717,17 @@ func syncFailureCause(reason string, _ error, contextErr error) error { func (m *Manager) runGitCombinedAfterAcquire( ctx context.Context, execTimeout time.Duration, repoPath string, args ...string, ) ([]byte, error, error) { - release, err := subproc.AcquireGit(ctx, subproc.GitLifecycle) - if err != nil { - return nil, err, ctx.Err() - } - defer release() - execCtx, cancel := context.WithTimeout(ctx, execTimeout) - defer cancel() - cmd := m.newNonInteractiveGitCmd(execCtx, repoPath, args...) - out, runErr := cmd.CombinedOutput() - return out, runErr, execCtx.Err() + return subproc.RunGitCombinedAfterAcquire(ctx, subproc.GitLifecycle, execTimeout, func(execCtx context.Context) *exec.Cmd { + return m.newNonInteractiveGitCmd(execCtx, repoPath, args...) + }) +} + +func firstContextError(execCtxErr, runErr error) error { + if execCtxErr != nil { + return execCtxErr + } + if errors.Is(runErr, context.Canceled) || errors.Is(runErr, context.DeadlineExceeded) { + return runErr + } + return nil } diff --git a/apps/backend/internal/worktree/manager_test.go b/apps/backend/internal/worktree/manager_test.go index de1cc6b720..f0b8d0ff36 100644 --- a/apps/backend/internal/worktree/manager_test.go +++ b/apps/backend/internal/worktree/manager_test.go @@ -11,6 +11,7 @@ import ( "time" "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/common/subproc" ) func newTestLogger() *logger.Logger { @@ -748,7 +749,11 @@ esac } got := string(envBytes) - want := "0|Never|echo|/bin/false|ssh -oBatchMode=yes" + wantSSH := "ssh -oBatchMode=yes" + if ambientSSH := os.Getenv("GIT_SSH_COMMAND"); ambientSSH != "" { + wantSSH = subproc.ForceGitSSHBatchMode(ambientSSH) + } + want := "0|Never|exit 1|exit 1|" + wantSSH if got != want { t.Fatalf("fake git env = %q, want %q", got, want) } diff --git a/apps/backend/internal/worktree/submodule_hardening_test.go b/apps/backend/internal/worktree/submodule_hardening_test.go index 4a5a332b6d..493924e509 100644 --- a/apps/backend/internal/worktree/submodule_hardening_test.go +++ b/apps/backend/internal/worktree/submodule_hardening_test.go @@ -166,11 +166,15 @@ func TestNewSubmoduleUpdateCmd_HasHardeningFlags(t *testing.T) { wantEnv := []string{ "GIT_TERMINAL_PROMPT=0", "GIT_SSH_COMMAND=ssh -oBatchMode=yes", - "SSH_ASKPASS=/bin/false", + "SSH_ASKPASS=exit 1", "GCM_INTERACTIVE=Never", } haveEnv := make(map[string]bool, len(cmd.Env)) for _, e := range cmd.Env { + if strings.HasPrefix(e, "GIT_SSH_COMMAND=") { + haveEnv["GIT_SSH_COMMAND=ssh -oBatchMode=yes"] = strings.Contains(e, "-oBatchMode=yes") + continue + } haveEnv[e] = true } for _, want := range wantEnv { diff --git a/apps/web/components/task/dockview-shared.tsx b/apps/web/components/task/dockview-shared.tsx index c323425b85..e999d74ee9 100644 --- a/apps/web/components/task/dockview-shared.tsx +++ b/apps/web/components/task/dockview-shared.tsx @@ -334,6 +334,7 @@ function ChangesContent({ panelId }: { panelId: string }) { // Dynamic title with file count — use environment-stable sessionId so the // tab title doesn't re-fetch on same-environment session tab switches. const activeSessionId = useEnvironmentSessionId(); + useResyncGitStatusOnTabActivate(panelId, activeSessionId); const gitStatus = useSessionGitStatus(activeSessionId); const { commits } = useSessionCommits(activeSessionId); const fileCount = gitStatus?.files ? Object.keys(gitStatus.files).length : 0; diff --git a/apps/web/components/task/mobile/mobile-changes-panel.tsx b/apps/web/components/task/mobile/mobile-changes-panel.tsx index 67ba65352f..38b3aa491c 100644 --- a/apps/web/components/task/mobile/mobile-changes-panel.tsx +++ b/apps/web/components/task/mobile/mobile-changes-panel.tsx @@ -11,6 +11,7 @@ import { ChangesPanelHeader } from "../changes-panel-header"; import { MobileDiffSheet } from "./mobile-diff-sheet"; import { useReviewSources } from "@/hooks/domains/session/use-review-sources"; import { useAppStore } from "@/components/state-provider"; +import { getWebSocketClient } from "@/lib/ws/connection"; import { useRequestChangesWalkthrough } from "@/hooks/domains/session/use-request-changes-walkthrough"; import type { SelectedDiff } from "../task-layout"; import type { OpenDiffOptions, DiffSheetMode } from "../changes-diff-target"; @@ -31,6 +32,13 @@ function buildContributionHeaderProps(data: ReturnType { + if (!sessionId) return; + getWebSocketClient()?.refreshSessionData(sessionId); + }, [sessionId]); +} + /** * Mobile Changes panel — renders the same timeline summary surface as desktop. * Reuses ChangesPanelBody + ChangesPanelHeader from desktop. @@ -45,6 +53,9 @@ export const MobileChangesPanel = memo(function MobileChangesPanel({ const activeSessionId = useAppStore((s) => s.tasks.activeSessionId); const { sourceCounts } = useReviewSources(activeSessionId); const [diffSheet, setDiffSheet] = useState(null); + + useRefreshMobileSessionData(data.activeSessionId); + const requestWalkthrough = useRequestChangesWalkthrough({ taskId: data.activeTaskId, sessionId: data.activeSessionId, diff --git a/apps/web/e2e/tests/git/fork-pr-comparison-target-helpers.ts b/apps/web/e2e/tests/git/fork-pr-comparison-target-helpers.ts index 446a73f125..70f58879a4 100644 --- a/apps/web/e2e/tests/git/fork-pr-comparison-target-helpers.ts +++ b/apps/web/e2e/tests/git/fork-pr-comparison-target-helpers.ts @@ -1,4 +1,6 @@ -import { execFileSync } from "node:child_process"; +import { execFileSync, spawn, type ChildProcess } from "node:child_process"; +import { createServer, type IncomingMessage, type Server, type ServerResponse } from "node:http"; +import fs from "node:fs"; import path from "node:path"; import type { BackendContext } from "../../fixtures/backend"; import { resetSeedRepositoryCheckout, type SeedData } from "../../fixtures/test-base"; @@ -9,8 +11,24 @@ const targetURL = "https://github.com/upstream/widget.git"; type ForkPRComparisonOptions = { comparisonTargetAvailable?: boolean; + localUncommittedFile?: string; }; +export type ComparisonTargetAuthFixture = { + url: string; + unauthorizedRequestCount: () => number; + setAvailable: (available: boolean) => void; + close: () => Promise; +}; + +type ActiveComparisonTargetFixture = { + taskID?: string; + fixture?: ComparisonTargetAuthFixture; + releaseBackendEnv?: () => Promise; +}; + +const activeComparisonTargetFixtures = new Map(); + export async function seedForkPRComparisonTask( apiClient: ApiClient, seedData: SeedData, @@ -27,6 +45,10 @@ export async function seedForkPRComparisonTask( // leave it on a feature branch or with uncommitted fixture files, which // makes the fork checkout below fail before the scenario starts. resetSeedRepositoryCheckout(seedData, backend.tmpDir); + // A previous scenario can finish while its comparison fetch is still + // unwinding after the backend restart. Remove its deterministic remote ref + // again before this scenario creates its target. + clearComparisonTargetRefs(seedData, backend.tmpDir); execFileSync("git", ["clone", "--bare", seedData.repositoryRemoteURL, targetRemoteDir], { env: gitEnv, @@ -53,19 +75,85 @@ export async function seedForkPRComparisonTask( localGit.stageAll(); const headSHA = localGit.commit("Add three fork contribution files"); + if (options.localUncommittedFile) { + fs.writeFileSync( + path.join(seedData.repositoryPath, options.localUncommittedFile), + "local change remains available\n", + ); + } + // agentctl fetches the credential-free provider URL. The test maps that URL // to its disposable upstream bare repository without changing origin. The - // unavailable mode maps to a missing local path so it exercises the same - // fail-closed path without depending on external network state. - const comparisonTargetPath = - options.comparisonTargetAvailable === false - ? path.join(backend.tmpDir, "repos", `missing-upstream-${suffix}.git`) - : targetRemoteDir; - execFileSync( - "git", - ["config", "--global", `url.file://${comparisonTargetPath}.insteadOf`, targetURL], - { cwd: backend.tmpDir, env: gitEnv }, - ); + // unavailable mode uses a local HTTP server which returns 401 until the + // test restores it, so the scenario proves authentication failure and + // recovery rather than only a missing path. + let comparisonTargetURL: string; + let comparisonTargetFixture: ComparisonTargetAuthFixture | undefined; + let comparisonExecutorProfileID: string | undefined; + if (options.comparisonTargetAvailable === false) { + comparisonTargetFixture = await startComparisonTargetAuthFixture(targetRemoteDir); + comparisonTargetURL = comparisonTargetFixture.url; + const comparisonGitConfigPath = path.join( + backend.tmpDir, + `comparison-target-${suffix}.gitconfig`, + ); + execFileSync( + "git", + [ + "config", + "--file", + comparisonGitConfigPath, + `url.${comparisonTargetURL}.insteadOf`, + targetURL, + ], + { env: gitEnv }, + ); + + let releaseBackendEnv: (() => Promise) | undefined; + try { + // The root comparison tracker is created when agentctl starts. Install + // the rewrite in the backend's inherited environment before the task is + // launched so that first materialization and later task Git commands use + // the same disposable target. + releaseBackendEnv = await backend.useEnv({ + GIT_CONFIG_GLOBAL: comparisonGitConfigPath, + }); + const { executors } = await apiClient.listExecutors(); + const localExecutor = executors.find( + (candidate) => candidate.type === "local" || candidate.type === "local_pc", + ); + if (!localExecutor) { + throw new Error("comparison target fixture requires a direct local executor"); + } + const profile = await apiClient.createExecutorProfile(localExecutor.id, { + name: `E2E comparison target ${suffix}`, + env_vars: [ + { key: "GIT_CONFIG_COUNT", value: "1" }, + ...gitConfigEnvVarsForURLRewrite(comparisonTargetURL), + ], + }); + comparisonExecutorProfileID = profile.id; + activeComparisonTargetFixtures.set(backend.tmpDir, { + fixture: comparisonTargetFixture, + releaseBackendEnv, + }); + } catch (setupError) { + try { + await releaseBackendEnv?.(); + } finally { + await comparisonTargetFixture.close(); + } + throw setupError; + } + } else { + comparisonTargetURL = `file://${targetRemoteDir}`; + } + if (options.comparisonTargetAvailable !== false) { + execFileSync("git", ["config", "--global", `url.${comparisonTargetURL}.insteadOf`, targetURL], { + cwd: backend.tmpDir, + env: gitEnv, + }); + } await apiClient.updateRepository(seedData.repositoryId, { provider: "github", @@ -127,6 +215,7 @@ export async function seedForkPRComparisonTask( checkout_branch: "feature/fork", }, ], + executor_profile_id: comparisonExecutorProfileID, }); await apiClient.associateGitHubTaskPR({ @@ -138,13 +227,233 @@ export async function seedForkPRComparisonTask( await apiClient.launchSession({ task_id: task.id, agent_profile_id: seedData.agentProfileId, + executor_profile_id: comparisonExecutorProfileID, workflow_step_id: seedData.startStepId, prompt: "/e2e:simple-message", }); - return { task, headSHA }; + const active = activeComparisonTargetFixtures.get(backend.tmpDir); + activeComparisonTargetFixtures.set(backend.tmpDir, { + ...active, + taskID: task.id, + }); + + return { task, headSHA, comparisonTargetFixture }; } -export function resetForkPRComparisonRepository(seedData: SeedData, backend: BackendContext) { - resetSeedRepositoryCheckout(seedData, backend.tmpDir); +function gitConfigEnvVarsForURLRewrite(comparisonTargetURL: string) { + return [ + { key: "GIT_CONFIG_KEY_0", value: `url.${comparisonTargetURL}.insteadOf` }, + { key: "GIT_CONFIG_VALUE_0", value: targetURL }, + ]; +} + +export async function resetForkPRComparisonRepository( + seedData: SeedData, + backend: BackendContext, + apiClient: ApiClient, +) { + const active = activeComparisonTargetFixtures.get(backend.tmpDir); + activeComparisonTargetFixtures.delete(backend.tmpDir); + await active?.releaseBackendEnv?.(); + await active?.fixture?.close(); + await clearTaskComparisonTargetRefs(active?.taskID, apiClient, backend.tmpDir); + resetSeedRepositoryCheckout(seedData, backend.tmpDir); + clearComparisonTargetRefs(seedData, backend.tmpDir); +} + +async function clearTaskComparisonTargetRefs( + taskID: string | undefined, + apiClient: ApiClient, + tmpDir: string, +) { + if (!taskID) return; + const environment = await apiClient.getTaskEnvironment(taskID); + const repositoryPaths = new Set(); + if (environment?.workspace_path) repositoryPaths.add(environment.workspace_path); + if (environment?.worktree_path) repositoryPaths.add(environment.worktree_path); + for (const repository of environment?.repos ?? []) { + if (repository.worktree_path) repositoryPaths.add(repository.worktree_path); + } + for (const repositoryPath of repositoryPaths) { + clearComparisonTargetRefsAtPath(repositoryPath, tmpDir); + } +} + +function clearComparisonTargetRefs(seedData: SeedData, tmpDir: string) { + clearComparisonTargetRefsAtPath(seedData.repositoryPath, tmpDir); +} + +function clearComparisonTargetRefsAtPath(repositoryPath: string, tmpDir: string) { + if (!fs.existsSync(repositoryPath)) return; + const env = makeGitEnv(tmpDir); + let output: string; + try { + output = execFileSync( + "git", + ["-C", repositoryPath, "for-each-ref", "--format=%(refname)", "refs/remotes/compare-*"], + { env }, + ) + .toString() + .trim(); + } catch { + return; + } + for (const ref of output ? output.split(/\r?\n/) : []) { + execFileSync("git", ["-C", repositoryPath, "update-ref", "-d", ref], { env }); + } +} + +async function startComparisonTargetAuthFixture( + repoDir: string, +): Promise { + const repoName = path.basename(repoDir); + const repoPath = `/${repoName}`; + let available = false; + let unauthorizedRequests = 0; + const children = new Set(); + const requestContext: ComparisonTargetRequestContext = { + repoDir, + repoPath, + isAvailable: () => available, + recordUnauthorized: () => { + unauthorizedRequests += 1; + }, + children, + }; + + const server = createServer((request, response) => { + void serveComparisonTargetRequest(request, response, requestContext); + }); + const port = await listenComparisonTargetServer(server); + + return { + url: `http://127.0.0.1:${port}${repoPath}`, + unauthorizedRequestCount: () => unauthorizedRequests, + setAvailable: (next) => { + available = next; + }, + close: async () => { + for (const child of children) child.kill(); + await closeComparisonTargetServer(server); + }, + }; +} + +type ComparisonTargetRequestContext = { + repoDir: string; + repoPath: string; + isAvailable: () => boolean; + recordUnauthorized: () => void; + children: Set; +}; + +async function serveComparisonTargetRequest( + request: IncomingMessage, + response: ServerResponse, + context: ComparisonTargetRequestContext, +): Promise { + const { repoDir, repoPath, isAvailable, recordUnauthorized, children } = context; + const parsed = new URL(request.url ?? "/", "http://comparison-target"); + const pathname = decodeURIComponent(parsed.pathname); + const isAdvertiseRequest = pathname === `${repoPath}/info/refs`; + const isUploadRequest = pathname === `${repoPath}/git-upload-pack`; + if ( + (!isAdvertiseRequest && !isUploadRequest) || + (isAdvertiseRequest && request.method !== "GET") || + (isUploadRequest && request.method !== "POST") + ) { + request.resume(); + response.writeHead(404).end(); + return; + } + + const available = isAvailable(); + if (!available) { + recordUnauthorized(); + request.resume(); + response.writeHead(401, { "www-authenticate": 'Basic realm="comparison-target"' }).end(); + return; + } + + const child = spawn("git", ["http-backend"], { + env: { + ...process.env, + GIT_HTTP_EXPORT_ALL: "1", + GIT_PROJECT_ROOT: path.dirname(repoDir), + PATH_INFO: pathname, + QUERY_STRING: parsed.search.slice(1), + REQUEST_METHOD: request.method ?? "GET", + CONTENT_TYPE: String(request.headers["content-type"] ?? ""), + CONTENT_LENGTH: String(request.headers["content-length"] ?? ""), + ...(request.headers["git-protocol"] + ? { HTTP_GIT_PROTOCOL: String(request.headers["git-protocol"]) } + : {}), + }, + }); + children.add(child); + const output: Buffer[] = []; + child.stdout.on("data", (chunk: Buffer) => output.push(chunk)); + child.stderr.resume(); + child.stdin.on("error", () => undefined); + child.stdout.on("error", () => undefined); + child.once("error", () => { + children.delete(child); + response.writeHead(500).end(); + }); + child.once("close", (code) => { + children.delete(child); + if (code !== 0) { + response.writeHead(500).end(); + return; + } + const cgiResponse = Buffer.concat(output); + const separator = cgiResponse.indexOf(Buffer.from("\r\n\r\n")); + const separatorLength = separator >= 0 ? 4 : 2; + const headerEnd = separator >= 0 ? separator : cgiResponse.indexOf(Buffer.from("\n\n")); + if (headerEnd < 0) { + response.writeHead(500).end(); + return; + } + const headers = cgiResponse.subarray(0, headerEnd).toString("utf8").split(/\r?\n/); + let status = 200; + const responseHeaders: Record = {}; + for (const header of headers) { + const separatorIndex = header.indexOf(":"); + if (separatorIndex < 0) continue; + const name = header.slice(0, separatorIndex); + const value = header.slice(separatorIndex + 1).trim(); + if (name.toLowerCase() === "status") { + status = Number.parseInt(value, 10) || 500; + } else { + responseHeaders[name] = value; + } + } + response.writeHead(status, responseHeaders); + response.end(cgiResponse.subarray(headerEnd + separatorLength)); + }); + if (isAdvertiseRequest) child.stdin.end(); + else request.pipe(child.stdin); +} + +function listenComparisonTargetServer(server: Server): Promise { + return new Promise((resolve, reject) => { + server.once("error", reject); + server.listen(0, "127.0.0.1", () => { + server.off("error", reject); + const address = server.address(); + if (!address || typeof address === "string") { + reject(new Error("comparison target fixture did not receive a TCP port")); + return; + } + resolve(address.port); + }); + }); +} + +function closeComparisonTargetServer(server: Server): Promise { + return new Promise((resolve, reject) => { + server.close((error) => (error ? reject(error) : resolve())); + server.closeAllConnections(); + }); } diff --git a/apps/web/e2e/tests/git/fork-pr-comparison-target.spec.ts b/apps/web/e2e/tests/git/fork-pr-comparison-target.spec.ts index ae7b1a2e99..a5c8b9fc1c 100644 --- a/apps/web/e2e/tests/git/fork-pr-comparison-target.spec.ts +++ b/apps/web/e2e/tests/git/fork-pr-comparison-target.spec.ts @@ -9,8 +9,8 @@ import { test.describe("Fork pull-request comparison target", () => { test.describe.configure({ timeout: 120_000 }); - test.afterEach(async ({ seedData, backend }) => { - resetForkPRComparisonRepository(seedData, backend); + test.afterEach(async ({ seedData, backend, apiClient }) => { + await resetForkPRComparisonRepository(seedData, backend, apiClient); }); test("uses the upstream target for one fork commit and three files", async ({ @@ -59,9 +59,15 @@ test.describe("Fork pull-request comparison target", () => { seedData, backend, }) => { - const { task } = await seedForkPRComparisonTask(apiClient, seedData, backend, { - comparisonTargetAvailable: false, - }); + const { task, comparisonTargetFixture } = await seedForkPRComparisonTask( + apiClient, + seedData, + backend, + { + comparisonTargetAvailable: false, + localUncommittedFile: "local-auth-recovery.txt", + }, + ); await testPage.goto(`/t/${task.id}`); const session = new SessionPage(testPage); @@ -72,5 +78,23 @@ test.describe("Fork pull-request comparison target", () => { await expect(testPage.getByTestId("comparison-target-notice")).toContainText( "upstream/widget:main", ); + await expect + .poll(() => comparisonTargetFixture?.unauthorizedRequestCount() ?? 0, { + timeout: 15_000, + message: "the comparison fixture should observe the failed HTTP authentication request", + }) + .toBeGreaterThan(0); + await expect( + session.changes.locator('[data-changes-file="local-auth-recovery.txt"]'), + ).toBeVisible(); + + comparisonTargetFixture?.setAvailable(true); + await session.clickSessionChatTab(); + await session.clickTab("Changes"); + await expect(testPage.getByTestId("comparison-target-notice")).toHaveCount(0, { + timeout: 30_000, + }); + await session.expandCommitsSection(); + await expect(session.commitsSection().locator('[data-testid^="commit-row-"]')).toHaveCount(1); }); }); diff --git a/apps/web/e2e/tests/git/mobile-fork-pr-comparison-target.spec.ts b/apps/web/e2e/tests/git/mobile-fork-pr-comparison-target.spec.ts index a7ca352c12..b694b7e843 100644 --- a/apps/web/e2e/tests/git/mobile-fork-pr-comparison-target.spec.ts +++ b/apps/web/e2e/tests/git/mobile-fork-pr-comparison-target.spec.ts @@ -1,7 +1,10 @@ import { expect, type Locator } from "@playwright/test"; import { test } from "../../fixtures/test-base"; import { SessionPage } from "../../pages/session-page"; -import { seedForkPRComparisonTask } from "./fork-pr-comparison-target-helpers"; +import { + resetForkPRComparisonRepository, + seedForkPRComparisonTask, +} from "./fork-pr-comparison-target-helpers"; async function expandSection(toggle: Locator) { await expect @@ -19,6 +22,10 @@ async function expandSection(toggle: Locator) { test.describe("Mobile fork pull-request comparison target", () => { test.describe.configure({ timeout: 120_000 }); + test.afterEach(async ({ seedData, backend, apiClient }) => { + await resetForkPRComparisonRepository(seedData, backend, apiClient); + }); + test("shows the same upstream target in the touch branch drawer", async ({ testPage, apiClient, @@ -66,9 +73,15 @@ test.describe("Mobile fork pull-request comparison target", () => { seedData, backend, }) => { - const { task } = await seedForkPRComparisonTask(apiClient, seedData, backend, { - comparisonTargetAvailable: false, - }); + const { task, comparisonTargetFixture } = await seedForkPRComparisonTask( + apiClient, + seedData, + backend, + { + comparisonTargetAvailable: false, + localUncommittedFile: "mobile-local-auth-recovery.txt", + }, + ); await testPage.goto(`/t/${task.id}`); const session = new SessionPage(testPage); @@ -84,5 +97,23 @@ test.describe("Mobile fork pull-request comparison target", () => { await expect(changes.getByTestId("comparison-target-notice")).toContainText( "upstream/widget:main", ); + await expect + .poll(() => comparisonTargetFixture?.unauthorizedRequestCount() ?? 0, { + timeout: 15_000, + message: "the comparison fixture should observe the failed HTTP authentication request", + }) + .toBeGreaterThan(0); + await expect( + changes.locator('[data-changes-file="mobile-local-auth-recovery.txt"]'), + ).toBeVisible(); + + comparisonTargetFixture?.setAvailable(true); + await testPage.getByRole("button", { name: "Chat" }).tap(); + await testPage.getByRole("button", { name: "Changes" }).tap(); + await expect(changes.getByTestId("comparison-target-notice")).toHaveCount(0, { + timeout: 30_000, + }); + await expandSection(changes.getByTestId("commits-section-collapse-toggle")); + await expect(changes.locator('[data-testid^="commit-row-"]')).toHaveCount(1); }); }); diff --git a/docs/plans/fork-pr-comparison-targets/plan.md b/docs/plans/fork-pr-comparison-targets/plan.md index 7a0ba2fd02..dc1ff3e8ae 100644 --- a/docs/plans/fork-pr-comparison-targets/plan.md +++ b/docs/plans/fork-pr-comparison-targets/plan.md @@ -204,3 +204,8 @@ git diff --check - Backfilling historical PR associations without complete head/base repository identity. They reconcile on the next authoritative provider read when an exact attachment match is available. - Plugin-provider implementation. The provider-neutral input remains available for a later adapter. + +## Shared execution follow-up (2026-09-13) + +The [noninteractive Git execution package](../noninteractive-git-execution/plan.md) owns final runner enforcement, helper cleanup, and authentication-failure recovery coverage. +That package is complete. Existing results above remain historical evidence for this package's original implementation; its follow-up validation is recorded in the linked package. diff --git a/docs/plans/git-subprocess-admission/plan.md b/docs/plans/git-subprocess-admission/plan.md index bac2c1c2b1..926ca61c19 100644 --- a/docs/plans/git-subprocess-admission/plan.md +++ b/docs/plans/git-subprocess-admission/plan.md @@ -233,3 +233,8 @@ and diagnostics files; waves do not authorize subagents. - Documentation links and indexes resolve. - The Windows before/after evidence required by the spec is attached before closing issue #2150. + +## Shared execution follow-up (2026-09-13) + +The [noninteractive Git execution package](../noninteractive-git-execution/plan.md) owns final runner enforcement, helper cleanup, and authentication-failure recovery coverage. +That package is complete. Existing results above remain historical evidence for this package's original implementation; its follow-up validation is recorded in the linked package. diff --git a/docs/plans/noninteractive-comparison-target-git/plan.md b/docs/plans/noninteractive-comparison-target-git/plan.md index 5221104b6c..c71d0ca7aa 100644 --- a/docs/plans/noninteractive-comparison-target-git/plan.md +++ b/docs/plans/noninteractive-comparison-target-git/plan.md @@ -99,3 +99,8 @@ git diff --check - Changes to the Git-status API or WebSocket payload. - Changes to `origin`, upstream tracking, or push routing. - New UI controls for Git credentials or transport selection. + +## Shared execution follow-up (2026-09-13) + +The [noninteractive Git execution package](../noninteractive-git-execution/plan.md) owns final runner enforcement, helper cleanup, and authentication-failure recovery coverage. +That package is complete. Existing results above remain historical evidence for this package's original implementation; its follow-up validation is recorded in the linked package. diff --git a/docs/plans/noninteractive-git-execution/plan.md b/docs/plans/noninteractive-git-execution/plan.md new file mode 100644 index 0000000000..ea5fe1d385 --- /dev/null +++ b/docs/plans/noninteractive-git-execution/plan.md @@ -0,0 +1,188 @@ +--- +created: 2026-09-13 +status: completed +requirements: + - REQ-PLATFORM-GIT-SUBPROCESS-ADMISSION-002 + - REQ-PLATFORM-WORKSPACE-GIT-STATUS-001 +system_design: + - ../../specs/platform/system-design/git-subprocess-execution.md + - ../../specs/platform/system-design/workspace-git-status.md +legacy_specs: [] +--- + +# Implementation Plan: Noninteractive Git execution + +## Overview + +Enforce application Git prompt suppression after callers assemble their environments, then bound helper cleanup and uncovered network operations. +Prove selected credential compatibility and task recovery before updating public guidance. +The five work orders were implemented sequentially on the verified PR #3635 descendant and validated with focused backend, web, build, documentation, and desktop/mobile E2E checks. + +## Evidence and root cause + +Source inspected at `bd7974dc8`, matching the supplied report's build. +`NewGitCommand` constructs Git without prompt controls. All shared runner variants execute caller commands without final preparation. +`GitOperator.runGitCommandWithEnvironment` replaces the environment after construction, making constructor-only enforcement insufficient. +Local worktree, repository-fetch, and tracker helpers apply inconsistent controls; the tracker uses `GIT_ASKPASS=echo`. + +A temporary real-Git experiment on 2026-09-13 used a controlling PTY, isolated HOME/global/system config, and `credential.helper=!false`. +`git credential fill` for `example.invalid` printed a username prompt and was still waiting after 1.039 seconds. +With terminal prompting disabled, deny-only askpass, and GCM noninteractive mode, Git failed in 0.047 seconds. +The parent killed only its owned waiting process group. No network, credentials, or live instance were used. +The failure diagnostic also quotes the username prompt, so substring absence alone is not a correct regression assertion. + +This confirms the terminal-fallback mechanism and missing enforcement boundary. +The original task-navigation subprocess, regression commit, whole-backend outage, and provider-trigger hypothesis remain unconfirmed. +The supplied GitHub incident and invalid-token observations are reported context, not independently verified facts in this package. + +## Merged dependency: PR #3635 + +[PR #3635](https://github.com/kdlbs/kandev/pull/3635) merged on 2026-09-13 at 11:19:46 UTC as `9cc146ee21c296d553ae684531219a5c711a4213`. +The merge commit was the inspected `origin/main` head when this package began. The working checkout includes this commit and the implementation changes. +Verify that `git merge-base --is-ancestor 9cc146ee21c296d553ae684531219a5c711a4213 HEAD` succeeds before compatibility tests. +Recheck upstream changes if main advances again. + +The merged [host bridge package](https://github.com/kdlbs/kandev/blob/9cc146ee21c296d553ae684531219a5c711a4213/docs/plans/executor-host-gh-bridge/plan.md) is implemented, not another pending work order. +It owns `REQ-INTEGRATIONS-GITHUB-AUTHENTICATION-002`, criteria `.1` through `.7`. +The [merged credential design](https://github.com/kdlbs/kandev/blob/9cc146ee21c296d553ae684531219a5c711a4213/docs/specs/integrations/system-design/github-authentication-02.md#executor-host-cli-bridge) is the compatibility authority. +No change to our observable requirements or five-task dependency order is needed. + +The merge adds credential routing and environment composition. It does not modify `common/subproc/shared.go` or `git_command.go`. +The shared execution defect is the target of this package. The implementation consumes the composed environment without rebuilding host eligibility or helper registration. + +Preserve existing user-helper order, optional marker-owned host helpers, token precedence, and managed-mode exclusion. +Environment duplicate removal applies to repeated assignments of the same environment variable, not repeated Git configuration entries at different indexes. +The existing optional helper chain is allowed; the prohibition on new fallback means no new routing after authentication failure. +Do not remove the host bridge because its shell function is not a direct OpenSSH command; SSH normalization applies only to SSH settings. + +An installed bridge can recover on a later command when its credential store or service recovers. +If bridge registration was skipped, existing launch/resume/prepared-start evaluation can install it after recovery, without restarting the backend. +Do not promise that every existing instance acquires a previously absent bridge on its next arbitrary Git command. + +## Requirement conformance + +Platform owns shared execution safety across callers. +Reuse `AC-PLATFORM-WORKSPACE-GIT-STATUS-001.16` through `.18` for comparison behavior. +Add `REQ-PLATFORM-GIT-SUBPROCESS-ADMISSION-002` to the existing shared Git requirement because universal prompt and helper-cleanup criteria were missing. +The existing accepted transport ADR remains authoritative. Its credential scope and no-fallback policy do not change. +The assumption check found no material unanswered product choice: the prompt explicitly settles scope, recovery, exclusions, and planning handoff. + +## Scope + +### In scope + +- Final prompt enforcement and selected-scope credential compatibility. +- Finite network budgets after admission, owned helper cancellation, pipe cleanup, and slot release. +- Required error completion and optional comparison recovery on desktop and phone. +- Targeted TDD, source audit, scoped engineering guidance, and public recovery guidance. + +### Out of scope + +- Credential changes, automatic retries, transport changes, and new authentication UI. +- Launcher-wide environment changes or deliberately interactive task terminals. +- Publication, live-instance mutation, credential changes, and provider operations. +- Attribution of the original incident to an unproven subprocess or provider event. + +## Technical approach + +The [execution design](../../specs/platform/system-design/git-subprocess-execution.md) defines the shared preparation and lifecycle boundary. +Preserve the [GitHub credential authority](../../specs/integrations/system-design/github-authentication-02.md) and existing clone isolation. +Move only reusable process primitives below agentctl; keep the unrelated agent lifecycle stable through compatibility wrappers. +Do not replace stdin or pass sensitive credentials through logs. + +### Initial caller inventory + +Task 03 completed the repository-wide inventory and applied the shared execution boundary to each discovered application-owned network path. + +| Caller / path under apps/backend/internal | Finding and treatment | +| --- | --- | +| orchestrator/executor/executor_host_gh_bridge.go (merged dependency) | Keep optional host probing and helper registration unchanged; add compatibility evidence through final Git execution | +| agent/runtime/environment/environment.go and agentctl/server/process/manager.go (merged dependency) | Consume canonical indexed configuration; preserve overlay/complete replacement and stale generated-helper removal | +| common/subproc/shared.go | All runner variants now use final preparation and bounded lifecycle handling | +| agentctl/server/process/git.go | Explicit instance environment and operation-specific AfterAcquire budgets are preserved | +| agentctl/server/process/workspace_git_cmd.go | Tracker environment, index, lock behavior, prompt/SSH controls, and the 10-second budget are preserved | +| agentctl/server/process/workspace_git_diff.go | Manual admission uses the shared stream lifecycle and closes the reader on cancellation before Wait | +| worktree/manager_git.go | Manual admitted paths use the shared lifecycle and 500 ms WaitDelay while retaining manager budgets | +| task/service/repository_fetch.go | The 30-second refresh, singleflight, cooldown, and shared prompt controls are preserved | +| task/service/fresh_branch.go | The 30-second AfterAcquire budget, best-effort fetch, and required checkout errors are preserved | +| agent/runtime/lifecycle/env_preparer_local.go | Fetch and checkout use finite post-admission budgets with the caller context | +| repoclone/clone.go | Clone, fetch, and submodule operations retain scoped helper authority with finite budgets | +| gitbootstrap/gitbootstrap.go | Network reachability is covered without changing local initialization behavior | +| office/configloader/git.go | The 30-second AfterAcquire path inherits final policy and cleanup | +| backendapp and submodule callers | Generic command construction and network verbs are covered by the completed inventory | +| task/gitinit/command_descriptor.go | Local descriptor-based ExecGit exception; preserve security and process ownership | + +The completed budget inventory is summarized below. A shorter caller lifetime always wins over these post-admission budgets. + +| Caller group | Network operations | Post-admission budget | Error owner | +| --- | --- | --- | --- | +| `agentctl/server/process` Git operator | fetch, push, remote inspection | 30 seconds for ordinary operations; five minutes for clone, push, and submodule operations | Required API result with sanitized error and released operation lock | +| Workspace tracker and comparison materialization | fetch and target refresh | Existing 10-second tracker command budget | Optional unavailable comparison state; local Changes remains usable | +| Worktree manager | fetch and pull | Existing configured fetch/pull budgets, 60 seconds by default; 30-second inspect commands | Existing lifecycle and fallback policy | +| Runtime, task, repository, and Office setup | fetch, checkout, clone, submodule | 30 seconds for ordinary fetch/checkout; five minutes for clone/submodule | Existing required setup errors or documented best-effort fetch handling | +| Backend materialization and repository state | fetch, pull, clone, push, submodule, `ls-remote` | 30 seconds for ordinary operations; five minutes for clone/push/submodule | Existing caller-specific required or optional result | + +Uncovered ordinary fetch, pull, checkout, and `ls-remote` paths use the 30-second default. Uncovered clone, submodule, and push paths use the five-minute default. +Existing explicit operation budgets remain intact, and earlier parent cancellation wins. + +## Tests + +The following table records the behavioral evidence implemented by the work orders. + +| Acceptance criteria | Behavioral evidence | +| --- | --- | +| ADMISSION-002.1, .2 | Task 01: `TestManagedGitCredentialFillPTY`, `TestGitFinalEnvironment`, `TestGitCredentialHTTPSelectedScope`, `TestGitSSHCommandPreservation` | +| ADMISSION-002.3, .4 | Task 02: `TestGitHelperCancellationReleasesSlot`, `TestGitDescendantPipeCleanup`, `TestWindowsManagedGitJobCleanup`, `TestCapDiffOutputCancellationClosesReader` | +| ADMISSION-002.2 through .6 | Task 03: `TestGitOperatorAuthenticationFailureAndRecovery`, `TestLocalCheckoutFetchDeadline`, `TestCloneDeadlineAfterAdmission`, existing queue-cancellation regressions | +| WORKSPACE-GIT-STATUS-001.16 through .18 | Task 04: authentication fixture with available local status, unavailable comparison, and later recovery | + +Prefixes above abbreviate `AC-PLATFORM-`; work orders contain full IDs and exact paths/commands. +Protect PTY/helper tests with outer timeouts and owned-process cleanup. Use local HTTP and synthetic credentials only. +Task 03 also runs the merged executor, runtime-environment, configuration, and lifecycle regressions. +Native Windows must execute environment, helper, and job-lifecycle tests. Linux cross-compilation cannot replace that evidence. + +## E2E tests + +The implementation extends `apps/web/e2e/tests/git/fork-pr-comparison-target.spec.ts` in `chromium` and `mobile-fork-pr-comparison-target.spec.ts` in `mobile-chrome`. +Reuse `fork-pr-comparison-target-helpers.ts`, adding real authentication-failure fixture behavior. +Desktop opens Changes through its session tab. Phone taps Changes and uses the existing mobile panel. +Both retain the existing unavailable-target notice and can open local file changes before authentication recovers. +No rendered UI change is proposed, so no new composition or ASCII preview is required. +Task 04 owns exact installation, build, and guarded runner commands. Run the two projects sequentially. + +## Companion packages + +The merged host bridge package above adds compatibility inputs for Tasks 01, 03, and 05. Its completed statuses and historical results remain unchanged. + +[Git admission](../git-subprocess-admission/plan.md), [Noninteractive comparison Git](../noninteractive-comparison-target-git/plan.md), and [Fork comparison targets](../fork-pr-comparison-targets/plan.md) are prior implemented packages. +Their recorded checks remain historical evidence for their original scope. +This package owns the broader runner enforcement and additional authentication-failure scenarios; it does not relabel their old results as new validation. +Workspace scalability, dependency exclusion, and mixed-change packages have no changed task scope. + +## Work orders + +- [x] [Task 01: Enforce final Git prompt policy](task-01-prompt-policy.md) +- [x] [Task 02: Bound Git helpers and streaming cleanup](task-02-owned-cleanup.md) +- [x] [Task 03: Cover network callers and failure recovery](task-03-network-budgets.md) +- [x] [Task 04: Prove desktop and phone task access](task-04-comparison-recovery.md) +- [x] [Task 05: Document Git failure and recovery](task-05-guidance.md) + +## Verification results + +Package authoring: temporary PTY mechanism reproduced as recorded above. +Implementation: completed on the verified PR #3635 descendant. Focused backend packages, the shared subprocess race test, web lint, Vite build, TypeScript typecheck, and Linux build passed. +The full backend `make lint` command passes after updating pre-existing Office test fixtures to discard the unused `QueueRun` outcome. +Cross-platform evidence: Windows subprocess, agentctl, and common lifecycle test binaries cross-compiled successfully. Native Windows execution was unavailable in this workspace, so the native Windows requirement remains a platform CI responsibility. +Browser evidence: the desktop Chromium and mobile Chrome comparison-recovery specs passed against the disposable local HTTP authentication fixture. +Documentation: public-doc validation, specification catalog validation, specification lint, and `git diff --check` passed. + +## Risks + +- Windows suspended-start job handling and Git shell askpass need native execution evidence. +- Caller process attributes may conflict with session isolation; fail before start rather than weakening ownership. +- Arbitrary custom helpers may ignore prompt controls or escape process ownership; never kill shared daemons to compensate. +- Constructor audit alone cannot prove direct Start coverage. Task 02 covers the lifecycle bypasses. +- Previously implicit clone/push lifetimes become finite; preserve established budgets and document uncovered defaults. +- The original incident call site remains unknown; universal execution-boundary coverage addresses the confirmed defect without assigning blame. + +Package reconciliation with merged PR #3635: source and contract comparison completed; the implementation consumes the merged host bridge environment without adding a credential or transport fallback. diff --git a/docs/plans/noninteractive-git-execution/task-01-prompt-policy.md b/docs/plans/noninteractive-git-execution/task-01-prompt-policy.md new file mode 100644 index 0000000000..0f62e09456 --- /dev/null +++ b/docs/plans/noninteractive-git-execution/task-01-prompt-policy.md @@ -0,0 +1,103 @@ +--- +id: "01-prompt-policy" +title: "Enforce final Git prompt policy" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +requirements: + - REQ-PLATFORM-GIT-SUBPROCESS-ADMISSION-002 +acceptance_criteria: + - AC-PLATFORM-GIT-SUBPROCESS-ADMISSION-002.1 + - AC-PLATFORM-GIT-SUBPROCESS-ADMISSION-002.2 +system_design: + - ../../specs/platform/system-design/git-subprocess-execution.md +--- + +# Task 01: Enforce final Git prompt policy + +## Summary + +All runner variants deny interactive authentication after caller overrides; valid selected credentials and input protocols still work. + +## In scope + +Apply shared final preparation to every runner variant after environment assembly. Move the existing SSH normalization into the shared package. +Preserve explicit credential scope, indexed configuration, stdin, and working-directory semantics. +Integrate the merged dependency recorded in plan.md before this work begins. +Do not deduplicate repeated Git helper/configuration keys at different indexes; normalize only repeated assignments to the same environment variable. +Add an ordered block case with empty helper resets, repeated user helpers, hooks/notes entries, and a marker-owned host helper. +Assert that preparation preserves the entire indexed block and selected credential-directory variables. +Keep the host bridge's shell function unchanged; it is not an SSH wrapper. + +Write `TestManagedGitCredentialFillPTY` first in `git_noninteractive_unix_test.go`. +Start a test subprocess under a controlling PTY, then invoke the production runner with real Git and a failing helper. +On the current code, assert that the test detects an actual waiting prompt and fails under an outer timeout. +Do not mistake Git's fatal message quoting a username prompt for an interactive read. + +In `git_noninteractive_test.go`, add `TestGitFinalEnvironment`, `TestGitCredentialHTTPSelectedScope`, and `TestGitSSHCommandPreservation`. +Cover all runner variants, nil and explicit empty environments, duplicate keys, inherited interactive settings, and replacement of `cmd.Env` after construction. +Use a local HTTP Basic-auth Git fixture with distinct synthetic host and instance credentials. +Prove valid helper success, selected-scope denial without fallback, stdin preservation, and no secrets in captured errors or logs. +Cover quoted/direct SSH paths, `GIT_SSH`, inherited BatchMode=no, unsupported wrappers, and repeated preparation. +Native Windows coverage must execute deny-only askpass, not merely inspect its environment value. + +## Out of scope + +Credential-authority changes, transport fallback, live-instance changes, and unrelated refactors. + +## Acceptance + +- All runner variants deny interactive authentication after caller overrides; valid selected credentials and input protocols still work. +- Run new behavioral tests before implementation and record the expected failure, then the passing result. +- Preserve the contracts and exclusions in the linked design. + +## Verification + +```bash +git merge-base --is-ancestor 9cc146ee21c296d553ae684531219a5c711a4213 HEAD +(cd apps/backend && go test ./internal/common/subproc ./internal/gitconfigenv ./internal/agentctl/server/process -count=1 -timeout=5m) +``` + +Run the same applicable package tests on native Windows. Skip only Unix PTY assertions; record platform evidence separately. + +## Files likely touched + +- `apps/backend/internal/common/subproc/git_command.go` +- `apps/backend/internal/common/subproc/shared.go` +- `apps/backend/internal/common/subproc/git_noninteractive*.go (new)` +- `apps/backend/internal/agentctl/server/process/workspace_git_cmd.go` +- `apps/backend/internal/agentctl/server/process/workspace_git_cmd_test.go` +- `apps/backend/internal/gitconfigenv/environment.go` + +## Dependencies + +Merged PR #3635 must be in HEAD; see the prerequisite in plan.md. + +## Risks + +Credential helper and process-lifecycle compatibility require real subprocess evidence. Do not infer success from environment assertions alone. + +## Parallelism + +`sequential` + +## Inputs + +- [Execution design](../../specs/platform/system-design/git-subprocess-execution.md). +- [Plan evidence and caller inventory](plan.md). +- Scoped AGENTS.md and relevant fix, TDD, and E2E skills. + +## Results + +Implemented on the verified PR #3635 descendant `9cc146ee21c296d553ae684531219a5c711a4213`. + +Results: shared final preparation now runs at every classified Git runner boundary, preserving explicit environments, indexed Git configuration, selected credential scope, and direct OpenSSH options. Added PTY, local HTTP Basic-auth, runner-variant, duplicate-environment, SSH-normalization, and native Windows compile coverage. The Unix lifecycle tests were added ahead of Task 02 and pass against the current implementation. + +Checks passed: + +- `git merge-base --is-ancestor 9cc146ee21c296d553ae684531219a5c711a4213 HEAD` +- `(cd apps/backend && go test ./internal/common/subproc ./internal/gitconfigenv ./internal/agentctl/server/process -count=1 -timeout=5m)` +- `GOOS=windows GOARCH=amd64 CGO_ENABLED=0 go test -c -o /tmp/kandev-subproc-windows.test ./internal/common/subproc` +- `GOOS=windows GOARCH=amd64 CGO_ENABLED=0 go test -c -o /tmp/kandev-process-windows.test ./internal/agentctl/server/process` +- `GOOS=windows GOARCH=amd64 CGO_ENABLED=0 go test -c -o /tmp/kandev-winproc-windows.test ./internal/agentctl/server/winproc` diff --git a/docs/plans/noninteractive-git-execution/task-02-owned-cleanup.md b/docs/plans/noninteractive-git-execution/task-02-owned-cleanup.md new file mode 100644 index 0000000000..5061cfcd2a --- /dev/null +++ b/docs/plans/noninteractive-git-execution/task-02-owned-cleanup.md @@ -0,0 +1,100 @@ +--- +id: "02-owned-cleanup" +title: "Bound Git helpers and streaming cleanup" +status: done +wave: 2 +depends_on: ["01-prompt-policy"] +plan: "plan.md" +requirements: + - REQ-PLATFORM-GIT-SUBPROCESS-ADMISSION-002 +acceptance_criteria: + - AC-PLATFORM-GIT-SUBPROCESS-ADMISSION-002.3 + - AC-PLATFORM-GIT-SUBPROCESS-ADMISSION-002.4 +system_design: + - ../../specs/platform/system-design/git-subprocess-execution.md +--- + +# Task 02: Bound Git helpers and streaming cleanup + +## Summary + +Helper hangs and inherited pipes return within execution plus cleanup bounds; slots and owned resources are released on every terminal path. + +## In scope + +Add command-owned lifecycle support below agentctl. Preserve existing agents through compatibility wrappers if generic Windows job code moves. +The execution design defines Unix session isolation, Windows suspended-start job attachment, 500 ms pipe waiting, and two-second cleanup limits. +Reject incompatible process attributes before start and preserve stronger compatible cleanup. + +Write `TestGitHelperCancellationReleasesSlot` and `TestGitDescendantPipeCleanup` before changing lifecycle execution. +Use helpers which hang, spawn pipe-holding descendants, and exit while descendants retain pipes. +Add a marker proving owned descendants stop and an unrelated control process remains alive. +Cover start failure, cancellation races, normal exit, cleanup failure, and repeated preparation. +Use `TestWindowsManagedGitJobCleanup` on Windows to prove attachment precedes descendant execution. + +Migrate all manual AcquireGit/Run paths in `manager_git.go` and streaming `capDiffOutput`. +Write `TestCapDiffOutputCancellationClosesReader` with a pipe-holding descendant before fixing the pre-Wait read/drain. +Keep the full Start-to-Wait lifetime inside one slot. Extend `raw_git_audit_test.go` to detect bypasses and test its rejection fixtures. +Retain the narrow descriptor-based gitinit exception; test its existing ownership contract unchanged. + +## Out of scope + +Credential-authority changes, transport fallback, live-instance changes, and unrelated refactors. + +## Acceptance + +- Helper hangs and inherited pipes return within execution plus cleanup bounds; slots and owned resources are released on every terminal path. +- Run new behavioral tests before implementation and record the expected failure, then the passing result. +- Preserve the contracts and exclusions in the linked design. + +## Verification + +```bash +(cd apps/backend && go test ./internal/common/... ./internal/agentctl/server/winproc ./internal/agentctl/server/process ./internal/worktree ./internal/task/gitinit -count=1 -timeout=5m) +(cd apps/backend && go test -race ./internal/common/subproc -count=1 -timeout=5m) +``` + +Run the same applicable package tests on native Windows. Skip only Unix PTY assertions; record platform evidence separately. + +## Files likely touched + +- `apps/backend/internal/common/subproc/shared.go` +- `apps/backend/internal/common/subproc/git_lifecycle*.go (new)` +- `apps/backend/internal/common/subproc/raw_git_audit_test.go` +- `apps/backend/internal/common/winproc/ (generic lifecycle code, if extracted)` +- `apps/backend/internal/agentctl/server/winproc/lifecycle_windows.go` +- `apps/backend/internal/agentctl/server/winproc/lifecycle_other.go` +- `apps/backend/internal/agentctl/server/process/workspace_git_diff.go` +- `apps/backend/internal/agentctl/server/process/workspace_git_diff_test.go` +- `apps/backend/internal/worktree/manager_git.go` +- `apps/backend/internal/task/gitinit/command_descriptor.go` + +## Dependencies + +01-prompt-policy. + +## Risks + +Credential helper and process-lifecycle compatibility require real subprocess evidence. Do not infer success from environment assertions alone. + +## Parallelism + +`sequential` + +## Inputs + +- [Execution design](../../specs/platform/system-design/git-subprocess-execution.md). +- [Plan evidence and caller inventory](plan.md). +- Scoped AGENTS.md and relevant fix, TDD, and E2E skills. + +## Results + +Implemented after Task 01 on the verified PR #3635 descendant `9cc146ee21c296d553ae684531219a5c711a4213`. + +Results: shared lifecycle code now owns Git Start-to-Wait, Unix session/process-group cleanup, Windows suspended Job Object attachment, pipe closure, and admission release. Manual worktree and streaming diff paths use the shared lifecycle; incompatible terminal and process-group attributes fail before start. The narrow `task/gitinit` descriptor exception remains unchanged. + +Checks passed: + +- `(cd apps/backend && go test ./internal/common/... ./internal/agentctl/server/winproc ./internal/agentctl/server/process ./internal/worktree ./internal/task/gitinit -count=1 -timeout=10m)` +- `(cd apps/backend && go test -race ./internal/common/subproc -count=1 -timeout=10m)` +- Windows cross-compilation of the shared subprocess, agentctl process, and common Windows lifecycle test binaries. diff --git a/docs/plans/noninteractive-git-execution/task-03-network-budgets.md b/docs/plans/noninteractive-git-execution/task-03-network-budgets.md new file mode 100644 index 0000000000..74fc7994f4 --- /dev/null +++ b/docs/plans/noninteractive-git-execution/task-03-network-budgets.md @@ -0,0 +1,128 @@ +--- +id: "03-network-budgets" +title: "Cover network callers and failure recovery" +status: done +wave: 3 +depends_on: ["02-owned-cleanup"] +plan: "plan.md" +requirements: + - REQ-PLATFORM-GIT-SUBPROCESS-ADMISSION-002 +acceptance_criteria: + - AC-PLATFORM-GIT-SUBPROCESS-ADMISSION-002.2 + - AC-PLATFORM-GIT-SUBPROCESS-ADMISSION-002.3 + - AC-PLATFORM-GIT-SUBPROCESS-ADMISSION-002.4 + - AC-PLATFORM-GIT-SUBPROCESS-ADMISSION-002.5 + - AC-PLATFORM-GIT-SUBPROCESS-ADMISSION-002.6 +system_design: + - ../../specs/platform/system-design/git-subprocess-execution.md +--- + +# Task 03: Cover network callers and failure recovery + +## Summary + +Every network caller has a recorded finite post-admission budget; selected credentials remain authoritative and subsequent recovery succeeds. + +## In scope + +Complete the caller inventory in plan.md using repository-wide construction, runner, and manual admission searches. +Record each network operation's execution budget, selected environment, admission class, and required/optional error owner. +An inherited context alone is not proof of a finite post-admission execution budget. + +Migrate GitOperator and local checkout preparation to AfterAcquire builders. +Keep existing operation-specific budgets. For uncovered ordinary fetch/pull use 30 seconds; use five minutes for uncovered clone, submodule update, and push. +A shorter parent lifetime still wins. These defaults are internal operation constants, not new public settings. +Preserve worktree manager fetch/pull budgets, tracker ten-second commands, Office 30-second commands, and fresh-branch/refresh 30-second commands. + +Keep repoclone's managed-helper isolation and existing transport selection. Preserve argument validation and push error sanitization. +Replace duplicate prompt implementations only after shared enforcement covers their direct starts. +Audit bootstrap, clone fetch/checkout, submodules, backendapp, and all network verbs including ls-remote. +Add focused tests for every modified caller package; record the exact added test names in Results. + +Write `TestGitOperatorAuthenticationFailureAndRecovery` with a local HTTP fixture. +Assert a required failure returns a usable sanitized error and releases the operation lock, then restore fixture credentials and retry on the same operator. +Add `TestLocalCheckoutFetchDeadline` and `TestCloneDeadlineAfterAdmission` for the uncovered lifecycle routes. +Use existing scheduler test patterns for canceled queues and queue waits exceeding the execution budget before a successful admitted command. +Preserve fresh-branch's documented best-effort fetch behavior and repository refresh cooldown/singleflight cleanup. + +### Merged host bridge compatibility + +Reuse the real Git/fake CLI fixtures in `executor_host_gh_bridge_test.go` and `executor_host_gh_bridge_profile_test.go`. +Extend them to exercise the production final Git runner with the effective routed environment, not only raw `git credential fill`. +Add `TestExecutorHostGHBridgeNoninteractiveExecution` and `TestExecutorHostGHBridgeRecoveryAfterUnavailableProbe`. +Cover Local/Worktree host-helper success without global setup, selected profile credential directories, and an absolute CLI path containing spaces. +Preserve earlier user helpers, explicit public/enterprise token precedence, unrelated-host/remote exclusion, and managed-helper failure without host invocation. +Exercise launch/resume/prepared-workspace propagation and reconfiguration that removes an obsolete marker-owned helper while retaining user entries. + +Separate two recovery cases. An installed helper which fails can succeed on a later operation in the same instance. +An absent bridge is re-evaluated by the existing launch/resume/prepared-start path; assert recovery on the same backend without adding per-command probes. +Use fake CLI responses and isolated configuration for these tests. Do not use a developer's stored login. +The local HTTP fixture remains necessary to prove successful authenticated network Git, while credential-fill tests prove host-specific bridge routing. + +## Out of scope + +Credential-authority changes, transport fallback, live-instance changes, and unrelated refactors. + +## Acceptance + +- Every network caller has a recorded finite post-admission budget; selected credentials remain authoritative and subsequent recovery succeeds. +- Run new behavioral tests before implementation and record the expected failure, then the passing result. +- Preserve the contracts and exclusions in the linked design. + +## Verification + +```bash +(cd apps/backend && go test ./internal/agentctl/server/process ./internal/agent/runtime/lifecycle ./internal/repoclone ./internal/gitbootstrap ./internal/task/service ./internal/office/configloader ./internal/backendapp ./internal/worktree -count=1 -timeout=10m) +(cd apps/backend && go test ./internal/common/subproc ./internal/gitconfigenv -count=1 -timeout=5m) +(cd apps/backend && go test ./internal/orchestrator/executor ./internal/agent/runtime/environment ./internal/agent/runtime/agentctl ./internal/agentctl/server/config ./internal/agentctl/server/api -count=1 -timeout=10m) +rg -n 'NewGitCommand|AcquireGit|RunGit|ExecGit' apps/backend --glob '*.go' --glob '!**/*_test.go' +``` + +## Files likely touched + +- `apps/backend/internal/orchestrator/executor/executor_host_gh_bridge_test.go` (available after dependency integration) +- `apps/backend/internal/orchestrator/executor/executor_host_gh_bridge_profile_test.go` (available after dependency integration) +- `apps/backend/internal/agentctl/server/process/manager_temp_test.go` (existing reconfiguration coverage) + +- `apps/backend/internal/agentctl/server/process/git.go` +- `apps/backend/internal/agentctl/server/process/git_auth_test.go (new)` +- `apps/backend/internal/agent/runtime/lifecycle/env_preparer_local.go` +- `apps/backend/internal/agent/runtime/lifecycle/env_preparer_local_test.go` +- `apps/backend/internal/repoclone/clone.go` +- `apps/backend/internal/repoclone/clone_auth_test.go` +- `apps/backend/internal/gitbootstrap/gitbootstrap.go` +- `apps/backend/internal/task/service/fresh_branch.go` +- `apps/backend/internal/task/service/repository_fetch.go` +- `apps/backend/internal/office/configloader/git.go` +- `apps/backend/internal/backendapp/ (network Git callers found by inventory)` + +## Dependencies + +02-owned-cleanup. + +## Risks + +Credential helper and process-lifecycle compatibility require real subprocess evidence. Do not infer success from environment assertions alone. + +## Parallelism + +`sequential` + +## Inputs + +- [Execution design](../../specs/platform/system-design/git-subprocess-execution.md). +- [Plan evidence and caller inventory](plan.md). +- Scoped AGENTS.md and relevant fix, TDD, and E2E skills. + +## Results + +Implemented after Task 02 on the verified PR #3635 descendant `9cc146ee21c296d553ae684531219a5c711a4213`. + +Results: network-capable Git callers now choose finite post-admission budgets while preserving established operation-specific deadlines, selected credential environments, refresh cooldowns, and required/optional error ownership. GitOperator authentication failures release the operation lock and a later operation can recover. Comparison-target recovery re-evaluates unavailable targets only from an explicit fresh status request, and moving internal comparison refs are force-updated only at their exact cache ref. + +Checks passed: + +- `(cd apps/backend && go test ./internal/agent/runtime/lifecycle ./internal/repoclone ./internal/gitbootstrap ./internal/task/service ./internal/office/configloader ./internal/backendapp ./internal/worktree -count=1 -timeout=10m)` +- `(cd apps/backend && go test ./internal/common/subproc ./internal/gitconfigenv -count=1 -timeout=5m)` +- `(cd apps/backend && go test ./internal/orchestrator/executor ./internal/agent/runtime/environment ./internal/agent/runtime/agentctl ./internal/agentctl/server/config ./internal/agentctl/server/api -count=1 -timeout=10m)` +- Production Git source audit in `internal/common/subproc`. diff --git a/docs/plans/noninteractive-git-execution/task-04-comparison-recovery.md b/docs/plans/noninteractive-git-execution/task-04-comparison-recovery.md new file mode 100644 index 0000000000..d39e122999 --- /dev/null +++ b/docs/plans/noninteractive-git-execution/task-04-comparison-recovery.md @@ -0,0 +1,96 @@ +--- +id: "04-comparison-recovery" +title: "Prove desktop and phone task access" +status: done +wave: 4 +depends_on: ["03-network-budgets"] +plan: "plan.md" +requirements: + - REQ-PLATFORM-GIT-SUBPROCESS-ADMISSION-002 + - REQ-PLATFORM-WORKSPACE-GIT-STATUS-001 +acceptance_criteria: + - AC-PLATFORM-GIT-SUBPROCESS-ADMISSION-002.5 + - AC-PLATFORM-GIT-SUBPROCESS-ADMISSION-002.6 + - AC-PLATFORM-WORKSPACE-GIT-STATUS-001.16 + - AC-PLATFORM-WORKSPACE-GIT-STATUS-001.17 + - AC-PLATFORM-WORKSPACE-GIT-STATUS-001.18 +system_design: + - ../../specs/platform/system-design/git-subprocess-execution.md + - ../../specs/platform/system-design/workspace-git-status.md +--- + +# Task 04: Prove desktop and phone task access + +## Summary + +Desktop and phone retain task access and local file changes during real fixture authentication failure, then recover comparison data. + +## In scope + +Reuse the existing fork comparison desktop and mobile tests and their shared seeding helper. +Add a local HTTP authentication-failure target to the isolated fixture; an unavailable boolean or missing local path alone does not prove authentication behavior. +Create a local uncommitted file, open the task, and prove session readiness, local Changes access, and the existing unavailable target notice. +On mobile, enter Changes through its visible touch button and use the mobile panel. +Assert comparison-derived numbers are not fabricated from origin/main. +Restore the fixture response and use the existing refresh trigger to prove comparison recovery without restarting the backend. +Backend fixture assertions must prove the HTTP auth request occurred and local file status survives. + +Keep production UI composition, copy, and navigation unchanged. These are rendered regression tests, so no new UI preview is needed. +If an existing error surface cannot expose completion, update this package before adding localized UI. +PTY and HTTP Go tests remain the terminal suppression evidence; browser tests cannot substitute for them. + +## Out of scope + +Credential-authority changes, transport fallback, live-instance changes, and unrelated refactors. + +## Acceptance + +- Desktop and phone retain task access and local file changes during real fixture authentication failure, then recover comparison data. +- Run new behavioral tests before implementation and record the expected failure, then the passing result. +- Preserve the contracts and exclusions in the linked design. + +## Verification + +```bash +(cd apps && pnpm install --frozen-lockfile) +make build-backend build-web-e2e +(cd apps/backend && go test ./internal/agentctl/server/process -count=1 -timeout=5m) +(cd apps/web && pnpm e2e:run --project=chromium e2e/tests/git/fork-pr-comparison-target.spec.ts) +(cd apps/web && pnpm e2e:run --project=mobile-chrome e2e/tests/git/mobile-fork-pr-comparison-target.spec.ts) +``` + +## Files likely touched + +- `apps/web/e2e/tests/git/fork-pr-comparison-target.spec.ts` +- `apps/web/e2e/tests/git/mobile-fork-pr-comparison-target.spec.ts` +- `apps/web/e2e/tests/git/fork-pr-comparison-target-helpers.ts` +- `apps/backend/internal/agentctl/server/process/workspace_comparison_target_test.go (new)` + +## Dependencies + +03-network-budgets. + +## Risks + +Credential helper and process-lifecycle compatibility require real subprocess evidence. Do not infer success from environment assertions alone. + +## Parallelism + +`sequential` + +## Inputs + +- [Execution design](../../specs/platform/system-design/git-subprocess-execution.md). +- [Plan evidence and caller inventory](plan.md). +- Scoped AGENTS.md and relevant fix, TDD, and E2E skills. + +## Results + +Implemented after Task 03 on the verified PR #3635 descendant `9cc146ee21c296d553ae684531219a5c711a4213`. + +Results: the desktop and mobile fork comparison specs use a disposable local smart HTTP Git fixture that returns authentication failure before recovery. Both flows preserve session access and a local uncommitted file, show the existing unavailable comparison state, observe the failed request, then recover comparison data after the fixture becomes available and Changes is refreshed. Reset cleanup removes comparison refs from the seed repository and task environments. + +Checks passed: + +- `(cd apps/web && pnpm e2e:run --host --no-build --project=chromium e2e/tests/git/fork-pr-comparison-target.spec.ts)` +- `(cd apps/web && pnpm e2e:run --host --no-build --project=mobile-chrome e2e/tests/git/mobile-fork-pr-comparison-target.spec.ts)` diff --git a/docs/plans/noninteractive-git-execution/task-05-guidance.md b/docs/plans/noninteractive-git-execution/task-05-guidance.md new file mode 100644 index 0000000000..f5af6dd207 --- /dev/null +++ b/docs/plans/noninteractive-git-execution/task-05-guidance.md @@ -0,0 +1,103 @@ +--- +id: "05-guidance" +title: "Document Git failure and recovery" +status: done +wave: 5 +depends_on: ["04-comparison-recovery"] +plan: "plan.md" +requirements: + - REQ-PLATFORM-GIT-SUBPROCESS-ADMISSION-002 +acceptance_criteria: + - AC-PLATFORM-GIT-SUBPROCESS-ADMISSION-002.1 + - AC-PLATFORM-GIT-SUBPROCESS-ADMISSION-002.2 + - AC-PLATFORM-GIT-SUBPROCESS-ADMISSION-002.3 + - AC-PLATFORM-GIT-SUBPROCESS-ADMISSION-002.4 + - AC-PLATFORM-GIT-SUBPROCESS-ADMISSION-002.5 + - AC-PLATFORM-GIT-SUBPROCESS-ADMISSION-002.6 +system_design: + - ../../specs/platform/system-design/git-subprocess-execution.md +--- + +# Task 05: Document Git failure and recovery + +## Summary + +Public recovery guidance matches shipped behavior; all task checks and platform limitations have accurate recorded results. + +## In scope + +Update the existing Git operations how-to with concise failure/recovery guidance. +Link to the merged host-bridge guidance in `docs/public/integrations.md` instead of repeating its setup instructions. +Working SSH and eligible host HTTPS setups need no new user configuration for this fix. +Explain existing-helper retry versus launch/resume re-evaluation when optional bridge registration was skipped. +Preserve backend-account/profile credential-directory scope and remote-executor exclusions. +Do not state that `gh auth token` success proves repository access or token validity. +Explain noninteractive failure and repair in the selected credential environment without asserting token revocation from a provider error. +Do not instruct automatic logout, credential replacement, or transport switching. +Update scoped backend guidance with the required final preparation and direct execution lifecycle helper. +Synchronize design, caller inventory, work-order results, and linked companion-plan follow-up notes with the actual implementation. +Do not overwrite historical test results or mark unrun Windows checks complete. +No push, issue publication, PR, or live-instance operation is authorized by this package. + +## Out of scope + +Credential-authority changes, transport fallback, live-instance changes, and unrelated refactors. + +## Acceptance + +- Public recovery guidance matches shipped behavior; all task checks and platform limitations have accurate recorded results. +- Record the implementation work orders' actual checks; run the documentation validators below. +- Preserve the contracts and exclusions in the linked design. + +## Verification + +```bash +node --test scripts/validate-public-docs.test.mjs +node scripts/validate-public-docs.mjs +python3 scripts/list-docs.py validate +python3 scripts/lint-spec-files.py --all +git diff --check +``` + +## Files likely touched + +- `docs/public/git-operations.md` +- `apps/backend/AGENTS.md` +- `docs/specs/platform/system-design/git-subprocess-execution.md` +- `docs/specs/platform/system-design/workspace-git-status.md` +- `docs/plans/noninteractive-git-execution/` +- `docs/plans/noninteractive-comparison-target-git/plan.md` +- `docs/plans/fork-pr-comparison-targets/plan.md` +- `docs/plans/git-subprocess-admission/plan.md` + +## Dependencies + +04-comparison-recovery. + +## Risks + +Credential helper and process-lifecycle compatibility require real subprocess evidence. Do not infer success from environment assertions alone. + +## Parallelism + +`sequential` + +## Inputs + +- [Execution design](../../specs/platform/system-design/git-subprocess-execution.md). +- [Plan evidence and caller inventory](plan.md). +- Scoped AGENTS.md and relevant fix, TDD, and E2E skills. + +## Results + +Implemented after Task 04 on the verified PR #3635 descendant `9cc146ee21c296d553ae684531219a5c711a4213`. + +Results: public Git Operations guidance now describes bounded noninteractive authentication failures, selected executor credential repair, existing-helper retry, and launch/resume re-evaluation for a skipped optional host bridge. The scoped backend guide and platform designs describe final preparation, owned lifecycle cleanup, finite post-admission budgets, and exact comparison-ref recovery. Companion plans link to this package without rewriting their historical results. + +Checks passed: + +- `node --test scripts/validate-public-docs.test.mjs` +- `node scripts/validate-public-docs.mjs` +- `python3 scripts/list-docs.py validate` +- `python3 scripts/lint-spec-files.py --all` +- `git diff --check` diff --git a/docs/public/git-operations.md b/docs/public/git-operations.md index 4a6c904f9a..85365b85c8 100644 --- a/docs/public/git-operations.md +++ b/docs/public/git-operations.md @@ -370,6 +370,8 @@ Before deleting a task or performing a hard reset, commit and push anything you - **No agent/client available:** launch or prepare the session and confirm its executor is healthy. Workspace Git actions can reconstruct runtime control after a backend restart, but still need a valid task environment. - **Remote/authentication error during worktree preparation:** test `git fetch origin` inside the same executor workspace. Verify the remote URL, SSH agent or key, known-hosts entry, token or credential-helper availability, DNS, and firewall access there. Do not paste command output containing tokens or authenticated URLs into a task or issue. +- **Noninteractive Git authentication failure:** Kandev-owned Git commands finish with a bounded error when the selected executor credential source cannot authenticate. Repair the SSH, credential-helper, or eligible host GitHub access in that same executor and retry the operation. Kandev does not replace credentials or switch transports after the failure. Existing host bridge registration can recover on a later operation; when registration was skipped, launch, resume, or prepared-start re-evaluates it. See [Choose task Git credentials](integrations.md#choose-task-git-credentials) for the credential scope. +- **Comparison target unavailable:** local Changes and local file status remain usable while a fork comparison target is unavailable. The target's commits, diff, ahead/behind values, and numeric totals remain unavailable until authentication recovers. Refresh Changes or request fresh status to trigger the one explicit re-evaluation; Kandev does not substitute a same-named `origin` branch. - **Host GitHub SSH setup:** for host-based Git credentials, run `gh config set git_protocol ssh --host github.com`, restart Kandev, and retry the task. For Docker, SSH, or Sprites, configure the same Git and SSH access in that executor instead of relying on the host. - **Merge or Rebase without `origin`:** the selected base branch must exist locally. If it does not, Kandev returns `base branch "BASE" does not exist locally` before changing history. If `origin` exists, Kandev does not fall back to a local branch after a fetch or authentication error. - **Pull fetched the wrong branch:** Kandev always uses `origin` and, once any upstream exists, the current local branch name. Align local and remote branch names or use an explicit terminal command. diff --git a/docs/specs/platform/requirements/git-subprocess-admission.md b/docs/specs/platform/requirements/git-subprocess-admission.md index c67591e9e2..65a877ff0d 100644 --- a/docs/specs/platform/requirements/git-subprocess-admission.md +++ b/docs/specs/platform/requirements/git-subprocess-admission.md @@ -28,6 +28,27 @@ Kandev limits concurrent Git subprocesses, but every caller currently competes i - **AC-PLATFORM-GIT-SUBPROCESS-ADMISSION-001.7:** Empty classes are skipped, so available capacity is never reserved for work that does not exist. - **AC-PLATFORM-GIT-SUBPROCESS-ADMISSION-001.8:** When all three classes remain queued and admitted commands complete, each class receives a slot within at most three successful slot releases. +### REQ-PLATFORM-GIT-SUBPROCESS-ADMISSION-002: Noninteractive managed Git execution + +**Intent:** Application-owned Git completes or fails without taking control of a user's terminal or authentication interface. + +#### Acceptance criteria + +- **AC-PLATFORM-GIT-SUBPROCESS-ADMISSION-002.1:** Every application-owned Git command shall suppress credential, host-trust, and interactive-login prompts through terminals, askpass, and supported credential managers. +- **AC-PLATFORM-GIT-SUBPROCESS-ADMISSION-002.2:** Commands shall preserve the selected host, executor, or managed credential scope. Valid helpers and noninteractive SSH credentials shall continue to work. +- **AC-PLATFORM-GIT-SUBPROCESS-ADMISSION-002.3:** Every managed Git network operation shall have a finite execution budget that starts after admission. Earlier lifetime cancellation shall still apply. +- **AC-PLATFORM-GIT-SUBPROCESS-ADMISSION-002.4:** Cancellation, helper failure, and inherited output pipes shall have bounded cleanup. Each terminal path shall release its admission slot and operation-owned locks. +- **AC-PLATFORM-GIT-SUBPROCESS-ADMISSION-002.5:** Required operations shall return credential-free failures through existing error surfaces and clear pending state. Optional comparison work shall preserve the outcomes in `AC-PLATFORM-WORKSPACE-GIT-STATUS-001.17`. +- **AC-PLATFORM-GIT-SUBPROCESS-ADMISSION-002.6:** A later operation shall work after authentication or service recovery without restarting Kandev. Failure shall not cause credential replacement, account fallback, transport fallback, remote rewriting, or a new retry loop. + +These criteria apply the accepted noninteractive transport decision across application-owned Git. +Deliberately interactive task terminals and commands entered by users remain outside this policy. +Arbitrary third-party helpers are not trusted to honor prompt controls; process isolation and deadlines bound their execution. + +## System design + +- [Managed Git execution](../system-design/git-subprocess-execution.md) + ## Migrated source detail Related issue: [#2150](https://github.com/kdlbs/kandev/issues/2150) @@ -204,8 +225,7 @@ resolution at process startup. - Serializing all instance creation independently of Git admission. - Changing public status, log, or diff payloads. - Aggregating admission snapshots across Docker or remote agentctl processes. -- Guaranteeing wall-clock completion when Git itself hangs; execution timeouts - continue to bound commands after admission. +- Guaranteeing successful completion during provider outages. Execution and cleanup remain bounded under requirement `REQ-PLATFORM-GIT-SUBPROCESS-ADMISSION-002`. ## Success criteria @@ -231,3 +251,5 @@ resolution at process startup. ## Implementation plan - [Git subprocess admission plan](../../../plans/git-subprocess-admission/plan.md) + +- [Noninteractive Git execution repair](../../../plans/noninteractive-git-execution/plan.md) diff --git a/docs/specs/platform/system-design/git-subprocess-execution.md b/docs/specs/platform/system-design/git-subprocess-execution.md new file mode 100644 index 0000000000..868cbfddf0 --- /dev/null +++ b/docs/specs/platform/system-design/git-subprocess-execution.md @@ -0,0 +1,121 @@ +--- +status: current +system: platform +created: 2026-09-13 +requirements: + - REQ-PLATFORM-GIT-SUBPROCESS-ADMISSION-002 +owners: + - kandev +--- + +# Managed Git execution + +## Purpose and boundaries + +Platform owns this contract because admission, subprocess lifetime, and prompt suppression serve all application Git callers. +Credential selection remains with the [GitHub authentication design](../../integrations/system-design/github-authentication-02.md). +Comparison state remains with [Workspace Git Status](workspace-git-status.md). +This design applies the accepted transport ADR; it does not select a new credential authority. + +## Requirement mapping + +| Criteria under REQ-PLATFORM-GIT-SUBPROCESS-ADMISSION-002 | Design section | +| --- | --- | +| .1, .2 | Final preparation | +| .3, .4 | Execution and cleanup | +| .5, .6 | Failure and recovery | + +## Final preparation + +`internal/common/subproc` owns final preparation immediately before Git starts. +All classified Run, Output, CombinedOutput, and AfterAcquire variants apply the same policy. +`NewGitCommand` remains the construction seam. Constructor defaults cannot enforce this contract because callers replace `cmd.Env`. +Manually admitted commands use the same preparation and lifecycle helper before Start or Run. +`GitInteractive` selects admission priority; it does not allow credential interaction. + +Preparation copies `cmd.Environ()`. An explicit empty environment stays explicit; only a nil environment inherits the parent. +Collapse duplicate assignments of the same environment key deterministically, using the last value and platform environment-key comparison rules. +Different indexed Git entries can intentionally repeat the same configuration key. Preserve their values and order, including empty helper resets. +Override prompt keys after that normalization. Preserve unrelated entries, working-directory behavior, stdin, and indexed `GIT_CONFIG_*` entries. +Never add ambient credentials to an explicit instance environment or reset a credential-helper chain. + +PR #3635's host bridge is part of the selected executor environment, not a new fallback introduced by prompt preparation. +Preserve its `kandev-host-gh-bridge` marker, quoted absolute CLI executable, and inherited helpers before it. +Explicit public/enterprise tokens retain precedence. Managed helper failure must never invoke this optional host route. +Remote executors and unrelated hosts remain outside host-bridge eligibility. +The integration layer owns HOME/GH_CONFIG_DIR/XDG_CONFIG_HOME selection and source-aware indexed composition. +Consume the resulting environment without re-probing credentials or merging it with the ambient host again. +Preserve agentctl overlay versus complete-block replacement, including intentional removal of a previous generated bridge. +SSH command normalization must not inspect or rewrite credential-helper shell functions. + +Enforce `GIT_TERMINAL_PROMPT=0`, `GCM_INTERACTIVE=Never`, and `GCM_GUI_PROMPT=0`. +Set Git and SSH askpass to a deny-only command which emits no credential and exits unsuccessfully. +Use a shell-builtin denial (`exit 1`) through Git's shell command handling, including Git for Windows. +Native Windows tests must prove invocation and denial; a Unix `/bin/false` path is not the Windows contract. +Set `SSH_ASKPASS_REQUIRE=never` as defense in depth. Do not use `echo` as an artificial credential. +Prompt preparation must be idempotent, including SSH normalization. + +Move the existing direct-OpenSSH normalization from `workspace_git_cmd.go` into the shared layer. +Preserve quoted executable paths, identities, proxy options, and host configuration. +Place `BatchMode=yes` before inherited options. Repeated preparation must not append repeated batch options. +Resolve direct `GIT_SSH` executables without silently discarding their path when `GIT_SSH_COMMAND` is absent. +Unsupported shell wrappers retain the ADR's safe-default behavior. Do not build another general shell parser. +Do not disable host-key verification or accept unknown hosts automatically. + +## Execution and cleanup + +Operation owners select finite budgets. Existing AfterAcquire budgets remain authoritative. +Migrate uncovered network callers to builders so queue time does not consume the command budget. +Keep earlier parent deadlines and distinguish admission cancellation from execution failure. +Do not impose the existing 30-second generic helper default on every clone or push. + +Shared Git lifecycle support must own Start, cancellation, and Wait, including output capture variants. +On Unix, detach ordinary managed Git into its own session without a controlling terminal. +Its process group is owned by the command and can be terminated on cancellation. +Preserve compatible attributes, including Linux parent-death behavior. Reject incompatible terminal/group attributes before starting; never target the launcher's group. +Compose caller cancellation cleanup rather than silently discarding it. + +Windows uses a kill-on-close Job Object installed while Git is suspended, before descendants can start. +Reuse the implementation in `internal/agentctl/server/winproc` by moving the required generic lifecycle code to a common package. +Keep compatibility wrappers for existing agentctl consumers. The shared layer must not import the agentctl server. +Job installation failure kills the owned suspended process and returns an error; it cannot leave a suspended orphan. + +Use a maximum 500 ms pipe WaitDelay, preserving a shorter caller value. +Bound owned-tree termination/reaping to two seconds, then return any cleanup failure. +Normal completion also closes owned lifecycle resources. Never terminate shared credential daemons or unrelated shells. +A helper which deliberately escapes ownership is outside process-tree containment; deadline and pipe closure still bound the caller. + +`capDiffOutput` must close its reader when execution is canceled, including while draining with `io.Copy` before Wait. +Keep output caps, partial-output semantics, and exactly-once admission release. +The descriptor-based `task/gitinit` ExecGit path remains a local-only exception with its existing process ownership and descriptor security. +The source audit records that narrow exception and rejects new unprepared direct execution paths. + +## Failure and recovery + +Keep existing API/WS shapes, error wrapping, redaction, locks, singleflight cleanup, and cooldown behavior. +Required operations finish with their existing error result. Optional materialization publishes unavailable comparison data while local Changes remains usable. +Desktop and phone retain their current navigation and unavailable notice. No new rendered UI is planned. +Never substitute a same-named local comparison ref after remote failure. + +Each later operation resolves its effective credentials through the existing authority. +No global failure latch, token mutation, new retries, or transport changes are added. +An already-installed host helper reads its selected credential store on later commands and can recover within the same instance. +If optional bridge registration was skipped, launch, resume, or prepared-start re-evaluates eligibility through the existing integration path. +This requires no backend restart, but does not imply automatic helper installation on every Git command. +Provider request failure alone does not establish token revocation. +An explicit fresh Git-status request may schedule one new attempt for an unavailable comparison target, so a recovered credential service can restore comparison data without a backend restart. This is a user-triggered re-evaluation, not a timer or retry loop. +When that attempt refreshes a moving comparison target, Git may force-update only the deterministic internal comparison ref for that exact target branch. It never force-updates `origin`, the checkout upstream, or the push route. + +## Persistence and observability + +No schema, runtime flag, startup setting, or persisted failure state changes. +Existing admission metrics continue to distinguish queueing from execution. +Logs and returned errors must exclude fixture credentials and real tokens. + +## Related decisions and delivery + +- [Deterministic noninteractive transport](../../../decisions/2026-08-31-deterministic-noninteractive-git-transport.md) +- [Class-aware admission](../../../decisions/2026-08-02-class-aware-git-subprocess-admission.md) +- [Implementation plan](../../../plans/noninteractive-git-execution/plan.md) + +- [Merged host bridge contract at PR #3635](https://github.com/kdlbs/kandev/blob/9cc146ee21c296d553ae684531219a5c711a4213/docs/specs/integrations/system-design/github-authentication-02.md#executor-host-cli-bridge) diff --git a/docs/specs/platform/system-design/workspace-git-status.md b/docs/specs/platform/system-design/workspace-git-status.md index 4674bb9470..5d99d5e6d5 100644 --- a/docs/specs/platform/system-design/workspace-git-status.md +++ b/docs/specs/platform/system-design/workspace-git-status.md @@ -124,20 +124,18 @@ A branch name alone is not a complete comparison identity. A task may be attache The instance's effective Git environment is authoritative for Kandev-owned workspace Git commands. This environment contains executor credentials and managed credential-helper entries. The process manager gives a detached environment copy to each workspace tracker. This rule applies to root, repository, submodule, rescan, and lazy trackers. -Each tracker starts Git with its instance environment, not the ambient environment of agentctl. The command environment preserves managed `GIT_CONFIG_*` entries and the command and options from a direct OpenSSH command, while placing `BatchMode=yes` before inherited batch-mode options. Unsupported shell prefixes or wrappers use the safe non-interactive default. It also enforces the established non-interactive controls: - -- `GIT_TERMINAL_PROMPT=0` -- `GCM_INTERACTIVE=Never` -- `GIT_ASKPASS=echo` -- `SSH_ASKPASS=/bin/false` -- `GIT_SSH_COMMAND=ssh -oBatchMode=yes` when the instance does not supply an SSH command - -These values prevent Git, Git Credential Manager, and SSH from reading the launcher terminal. Each command keeps the existing Git-command deadline and throttle behavior. +Each tracker starts Git with its instance environment. Final execution preparation follows +[Managed Git execution](git-subprocess-execution.md), including deny-only askpass, +SSH option preservation, finite execution budgets, and bounded helper cleanup. +The shared boundary applies controls after caller environment assembly. Tracker-specific +index selection and optional-lock suppression remain owned by the tracker. The process manager sets an explicit target to `pending` before it starts network work. It then materializes the target with the manager's lifetime context. Initial materialization, live updates, rescan, and lazy tracker creation do not wait for the fetch. A successful fetch publishes the exact comparison ref and starts a detached status refresh. An error publishes a bounded unavailable state. Manager shutdown cancels unfinished materialization. Kandev selects the transport before it starts a comparison-target command. Canonical comparison targets remain HTTPS. An HTTPS authentication or transport error does not start an SSH retry. This rule prevents a command from changing identity, host trust, or credential scope after the first error. Kandev does not rewrite `origin`, the checkout upstream, or push routing. +An explicit fresh status request may re-evaluate an unavailable comparison target once. A moving target refresh may force-update only its deterministic internal comparison ref, so a stale cache does not block recovery; user remotes and push routing are unchanged. + This contract follows [ADR-2026-08-31-deterministic-noninteractive-git-transport](../../../decisions/2026-08-31-deterministic-noninteractive-git-transport.md). ## API surface @@ -234,3 +232,5 @@ Existing Git-status routes remain in place. Their result and stream payloads add ## Implementation plan See [Workspace Git Status Scalability plan](../../../plans/workspace-git-status-scalability/plan.md), [Workspace Git Status Dependency-Tree Exclusion plan](../../../plans/workspace-git-status-dependency-tree-exclusion/plan.md), [Fork PR Comparison Targets plan](../../../plans/fork-pr-comparison-targets/plan.md), [Non-interactive Comparison-target Git plan](../../../plans/noninteractive-comparison-target-git/plan.md), and [Mixed Staged and Unstaged Changes plan](../../../plans/mixed-staged-unstaged-changes/plan.md). + +The [shared Git execution repair](../../../plans/noninteractive-git-execution/plan.md) extends enforcement beyond comparison commands.