diff --git a/README.md b/README.md index 6f2661e..704ac50 100644 --- a/README.md +++ b/README.md @@ -226,7 +226,11 @@ flight at a time), controlled by `run.max_concurrent_repos`. in the PR body so a human sees it immediately. 7. **Deliver** — the remote is re-checked, then pushed; a draft PR is opened with `Closes #`, verification result, model used, and cost; the issue is commented with the PR link; - `agent-working` and `agent-planned` are swapped for `agent-done` or `agent-failed`. + `agent-working` and `agent-planned` are swapped for `agent-done` or `agent-failed`. Every commit on + the branch — whether made by the harness or by Claude itself during step 5 — carries the + `git.author_name`/`git.author_email` identity, not your own; the PR itself is still opened under + the `gh` token's account, since that is a GitHub-level attribution the identity config does not + touch. 8. **Cleanup** — the worktree is removed (kept on disk if `workspace.keep_failed` and the run failed, for post-mortem). The claim is always released, on every exit path. @@ -346,6 +350,10 @@ This repository's own `config.json` is also **compiled into the binary** at buil "enabled": false, "webhook_url": "" }, + "git": { + "author_name": "coding-agent-loop[bot]", + "author_email": "coding-agent-loop@users.noreply.github.com" + }, "models_path": "models.json" } ``` @@ -387,6 +395,8 @@ This repository's own `config.json` is also **compiled into the binary** at buil | `store.path` | SQLite database path | | `discord.enabled` | turn on Discord status notifications (see [Discord notifications](#discord-notifications)) | | `discord.webhook_url` | the channel's incoming webhook URL; **required** if `discord.enabled` is true | +| `git.author_name` | commit author name for work the loop produces; distinct from your own so agent commits are easy to spot; empty falls back to `coding-agent-loop[bot]` | +| `git.author_email` | commit author email to go with `git.author_name`; empty falls back to `coding-agent-loop@users.noreply.github.com` | | `models_path` | where to look for `models.json`; see [Embedded defaults](#embedded-defaults) for how the default value is resolved | ### models.json diff --git a/cmd/agent.go b/cmd/agent.go index 2dedbf2..37ec655 100644 --- a/cmd/agent.go +++ b/cmd/agent.go @@ -173,11 +173,13 @@ func run(f flags) error { ghBinary = abs } gitMgr := &gitpkg.Manager{ - ReposRoot: cfg.Workspace.ReposRoot, - WorkRoot: cfg.Workspace.Root, - GHBinary: ghBinary, - DryRun: suppressMutations, - Log: func(format string, args ...any) { log.Info(fmt.Sprintf(format, args...)) }, + ReposRoot: cfg.Workspace.ReposRoot, + WorkRoot: cfg.Workspace.Root, + AuthorName: cfg.Git.AuthorName, + AuthorEmail: cfg.Git.AuthorEmail, + GHBinary: ghBinary, + DryRun: suppressMutations, + Log: func(format string, args ...any) { log.Info(fmt.Sprintf(format, args...)) }, } runner := &claude.Runner{Log: func(format string, args ...any) { log.Debug(fmt.Sprintf(format, args...)) }} gateway := gate.New(st, cfg.Claude, func(format string, args ...any) { log.Info(fmt.Sprintf(format, args...)) }) diff --git a/config.example.json b/config.example.json index f27a234..8c0d87f 100644 --- a/config.example.json +++ b/config.example.json @@ -62,5 +62,9 @@ "enabled": false, "webhook_url": "" }, + "git": { + "author_name": "coding-agent-loop[bot]", + "author_email": "coding-agent-loop@users.noreply.github.com" + }, "models_path": "models.json" } diff --git a/internal/claude/runner.go b/internal/claude/runner.go index 9448af9..4b2922e 100644 --- a/internal/claude/runner.go +++ b/internal/claude/runner.go @@ -139,6 +139,8 @@ type Options struct { PermissionMode string // WorkDir is the process cwd, i.e. the worktree. WorkDir string + // Env is appended to the subprocess's inherited environment. + Env []string // ExtraArgs are appended verbatim. ExtraArgs []string // LogPath receives the raw JSONL transcript. Required. @@ -224,6 +226,9 @@ func (r *Runner) Run(ctx context.Context, opts Options) (*Result, error) { cmd := exec.CommandContext(ctx, opts.Binary, args...) cmd.Dir = opts.WorkDir + if len(opts.Env) > 0 { + cmd.Env = append(os.Environ(), opts.Env...) + } cmd.Stdin = strings.NewReader(opts.Prompt) // Claude Code spawns children (the bash tool, language servers); kill the // whole group on cancellation or a timed-out run leaves the worker stuck. diff --git a/internal/claude/runner_test.go b/internal/claude/runner_test.go index 7d95d51..03f9bc0 100644 --- a/internal/claude/runner_test.go +++ b/internal/claude/runner_test.go @@ -175,6 +175,25 @@ printf '{"type":"result","subtype":"success","is_error":false,"result":"%s"}\n' } } +// Env must reach the child process, since this is how the harness's commit +// identity is delivered to Claude when it commits on its own. +func TestEnvReachesTheChildProcess(t *testing.T) { + bin := stubCLI(t, `cat > /dev/null +printf '{"type":"result","subtype":"success","is_error":false,"result":"%s"}\n' "$GIT_AUTHOR_NAME" +`) + res, err := (&Runner{}).Run(context.Background(), Options{ + Binary: bin, + LogPath: filepath.Join(t.TempDir(), "run.jsonl"), + Env: []string{"GIT_AUTHOR_NAME=coding-agent-loop[bot]"}, + }) + if err != nil { + t.Fatal(err) + } + if res.Result != "coding-agent-loop[bot]" { + t.Fatalf("Env did not reach the child process, got result %q", res.Result) + } +} + func TestLogPathRequired(t *testing.T) { if _, err := (&Runner{}).Run(context.Background(), Options{Binary: "true"}); err == nil { t.Fatal("LogPath must be required so every run leaves a transcript") diff --git a/internal/config/config.go b/internal/config/config.go index a33bdbd..6909698 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -47,6 +47,7 @@ type Config struct { Server ServerConfig `json:"server"` Store StoreConfig `json:"store"` Discord DiscordConfig `json:"discord"` + Git GitConfig `json:"git"` // ModelsPath points at models.json, resolved by the caller (cmd/agent.go), // not expanded here: "models.json" (the default) is looked up next to the @@ -180,6 +181,14 @@ type DiscordConfig struct { WebhookURL string `json:"webhook_url"` } +// GitConfig identifies the author of commits the loop produces, so they are +// visibly distinct from the human repo owner's own commits. Empty means "use +// the built-in default" — see git.Manager's author()/email(). +type GitConfig struct { + AuthorName string `json:"author_name"` + AuthorEmail string `json:"author_email"` +} + // Default returns a Config with every field populated to a sane value. func Default() Config { return Config{ @@ -231,6 +240,10 @@ func Default() Config { Server: ServerConfig{Addr: "127.0.0.1:8787"}, Store: StoreConfig{Path: "~/.agent-loop/state.db"}, Discord: DiscordConfig{Enabled: false}, + Git: GitConfig{ + AuthorName: "coding-agent-loop[bot]", + AuthorEmail: "coding-agent-loop@users.noreply.github.com", + }, } } @@ -326,6 +339,12 @@ func (c *Config) Validate() error { if c.Discord.Enabled && c.Discord.WebhookURL == "" { return fmt.Errorf("discord.webhook_url must be set when discord.enabled is true") } + if containsCommitHeaderBreakingChars(c.Git.AuthorName) { + return fmt.Errorf("git.author_name must not contain '<', '>', or a newline") + } + if containsCommitHeaderBreakingChars(c.Git.AuthorEmail) { + return fmt.Errorf("git.author_email must not contain '<', '>', or a newline") + } if c.GitHub.PRComments.Enabled { pc := c.GitHub.PRComments if !strings.HasPrefix(pc.Mention, "@") { @@ -344,6 +363,12 @@ func (c *Config) Validate() error { return nil } +// containsCommitHeaderBreakingChars reports whether s has a character that +// would corrupt the "Name " line git writes into a commit header. +func containsCommitHeaderBreakingChars(s string) bool { + return strings.ContainsAny(s, "<>\n") +} + // validReactions are the only content values GitHub's reactions API accepts. var validReactions = map[string]bool{ "+1": true, "-1": true, "laugh": true, "confused": true, diff --git a/internal/config/config_test.go b/internal/config/config_test.go index f079bbb..7d404f3 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -175,6 +175,47 @@ func TestRetryBackoffMaxMustNotBeBelowTheBase(t *testing.T) { } } +// Agent-authored commits should carry their own identity by default, distinct +// from whatever the host's global gitconfig would otherwise resolve to. +func TestGitIdentityDefaults(t *testing.T) { + cfg := Default() + if cfg.Git.AuthorName != "coding-agent-loop[bot]" { + t.Fatalf("unexpected default git author name %q", cfg.Git.AuthorName) + } + if cfg.Git.AuthorEmail != "coding-agent-loop@users.noreply.github.com" { + t.Fatalf("unexpected default git author email %q", cfg.Git.AuthorEmail) + } +} + +func TestGitIdentityOverlaysFromConfig(t *testing.T) { + cfg, err := Load(writeConfig(t, `{"github":{"owners":["acme"]},"git":{"author_name":"Acme Bot","author_email":"bot@acme.example"}}`), false) + if err != nil { + t.Fatal(err) + } + if cfg.Git.AuthorName != "Acme Bot" { + t.Fatalf("author_name = %q", cfg.Git.AuthorName) + } + if cfg.Git.AuthorEmail != "bot@acme.example" { + t.Fatalf("author_email = %q", cfg.Git.AuthorEmail) + } +} + +// A name/email containing '<', '>', or a newline would corrupt the commit +// header line git builds ("Name "). +func TestGitIdentityWithAngleBracketIsRejected(t *testing.T) { + _, err := Load(writeConfig(t, `{"github":{"owners":["acme"]},"git":{"author_name":"evil>name"}}`), false) + if err == nil || !strings.Contains(err.Error(), "author_name") { + t.Fatalf("want a git.author_name validation error, got %v", err) + } +} + +func TestGitIdentityEmailWithAngleBracketIsRejected(t *testing.T) { + _, err := Load(writeConfig(t, `{"github":{"owners":["acme"]},"git":{"author_email":""); strings.TrimSpace(got) != "agent " { + t.Fatalf("commit author = %q, want the configured identity", got) + } // Committed work still counts as work, via the commit-ahead check. if work, err := m.HasWork(ctx, wt, "main"); err != nil || !work { t.Fatalf("committed work should still register: work=%v err=%v", work, err) @@ -191,6 +194,67 @@ func TestWorktreeLifecycle(t *testing.T) { } } +// This is the case the issue actually cares about: Claude, or any other +// subprocess, commits directly with a bare `git commit` (no -c flags, unlike +// CommitAll). EnsureRepo must have already written the harness identity into +// the shared clone's own config, so that commit picks it up instead of +// falling through to the host's global gitconfig — which is what caused +// agent commits to be attributed to the human repo owner. +func TestEnsureRepoAppliesIdentityOverridingGlobalConfig(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not available") + } + ctx := context.Background() + origin := originRepo(t) + root := t.TempDir() + + globalConfig := filepath.Join(root, "global-gitconfig") + if err := os.WriteFile(globalConfig, []byte("[user]\n\tname = Human Author\n\temail = human@example.com\n"), 0o644); err != nil { + t.Fatal(err) + } + t.Setenv("GIT_CONFIG_GLOBAL", globalConfig) + + m := &Manager{ + ReposRoot: filepath.Join(root, "repos"), + WorkRoot: filepath.Join(root, "work"), + AuthorName: "agent", + AuthorEmail: "agent@example.com", + } + + repoPath, err := m.EnsureRepo(ctx, "acme/widgets", origin) + if err != nil { + if strings.Contains(err.Error(), "unknown option") || strings.Contains(err.Error(), "unknown environment variable") { + t.Skip("git too old to support GIT_CONFIG_GLOBAL") + } + t.Fatalf("EnsureRepo: %v", err) + } + + wt := m.WorktreePath("acme/widgets", 7) + if err := m.AddWorktree(ctx, repoPath, wt, "agent/issue-7", "main"); err != nil { + t.Fatalf("AddWorktree: %v", err) + } + if err := os.WriteFile(filepath.Join(wt, "feature.txt"), []byte("implemented\n"), 0o644); err != nil { + t.Fatal(err) + } + + // Simulate Claude: a bare commit, no -c user.name/user.email. + cmd := exec.Command("git", "add", "-A") + cmd.Dir = wt + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git add: %v\n%s", err, out) + } + cmd = exec.Command("git", "commit", "-m", "add feature") + cmd.Dir = wt + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git commit: %v\n%s", err, out) + } + + got := git(t, wt, "log", "-1", "--format=%an <%ae>") + if strings.TrimSpace(got) != "agent " { + t.Fatalf("bare commit author = %q, want the harness identity, not the host's global gitconfig", got) + } +} + // AssertRemote is the last gate before anything leaves the machine. func TestAssertRemote(t *testing.T) { if _, err := exec.LookPath("git"); err != nil { diff --git a/internal/orchestrator/loop.go b/internal/orchestrator/loop.go index 377bf87..654ee55 100644 --- a/internal/orchestrator/loop.go +++ b/internal/orchestrator/loop.go @@ -685,6 +685,7 @@ func (o *Orchestrator) execute(ctx context.Context, log *slog.Logger, cand candi Fallbacks: fallbacks, PermissionMode: permissionMode, WorkDir: worktree, + Env: o.opts.Git.IdentityEnv(), ExtraArgs: cfg.Claude.ExtraArgs, LogPath: logPath, Timeout: cfg.Run.Timeout.D(), diff --git a/internal/orchestrator/prompt.go b/internal/orchestrator/prompt.go index b0af202..7550376 100644 --- a/internal/orchestrator/prompt.go +++ b/internal/orchestrator/prompt.go @@ -38,6 +38,8 @@ The harness, not you, owns version control and GitHub. Specifically: - Do NOT run git push, git rebase, git reset --hard, or any force operation. - Do NOT create branches, tags, pull requests, or issue comments. - Do NOT amend or rewrite any commit that already exists. +- Do NOT change git's user.name/user.email or pass --author/--reset-author; the harness has + already set the commit identity for this worktree. - You MAY commit your work locally. If you do not, the harness commits it for you. Scope rules: diff --git a/internal/orchestrator/prompt_test.go b/internal/orchestrator/prompt_test.go index 652359d..6d98a8e 100644 --- a/internal/orchestrator/prompt_test.go +++ b/internal/orchestrator/prompt_test.go @@ -52,6 +52,17 @@ func TestPlanTaskPromptIncludesThePreviousPlanOnReplan(t *testing.T) { } } +// The harness sets the commit identity for the worktree before Claude runs; +// this stops a helpful model from "fixing" an unfamiliar author. +func TestSystemPromptForbidsChangingGitIdentity(t *testing.T) { + p := systemPrompt("acme/widgets", "agent/issue-9", "/work/widgets/issue-9") + for _, want := range []string{"user.name", "user.email", "--author", "already set the commit identity"} { + if !strings.Contains(p, want) { + t.Errorf("system prompt missing %q:\n%s", want, p) + } + } +} + func TestPlanSystemPromptForbidsEditing(t *testing.T) { p := planSystemPrompt("acme/widgets", "/work/widgets/issue-9") for _, want := range []string{"Do NOT edit", "Do NOT", "plan"} {