Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 19 additions & 7 deletions Makefile
Original file line numberDiff line numberDiff line change
Expand Up@@ -13,7 +13,7 @@ MODELS ?= models.json
EMBED_CONFIG := config.json
EMBED_MODELS := models.json

.PHONY: all help build build-check build-summary config check run once dry-run no-mutate \
.PHONY: all help build build-check build-summary config embed-ready check run once dry-run no-mutate \
install uninstall print-service \
test coverage vet fmt fmt-check lint staticcheck vulcheck ci tidy clean

Expand DownExpand Up@@ -49,13 +49,25 @@ build-summary:
cat "$(EMBED_MODELS)"; \
fi

build: build-check build-summary
build: embed-ready build-check build-summary
go build -ldflags="-w -s" -o $(BINARY) ./cmd

## config: create config.json from config.example.json if it doesn't exist yet
config:
@test -f $(CONFIG) && echo "$(CONFIG) already exists" || cp config.example.json $(CONFIG)

# embed-ready: make sure the files go:embed compiles in actually exist.
#
# config.json is gitignored, so a fresh clone or a git worktree — which is
# exactly what the agent builds every run in — has no config.json for
# embedded.go to embed, and the whole module fails to compile with
# "pattern config.json: no matching files found". Every target below that
# compiles anything depends on this, so testing a clean checkout works.
.PHONY: embed-ready
embed-ready:
@test -f $(EMBED_CONFIG) || cp config.example.json $(EMBED_CONFIG)
@test -f $(EMBED_MODELS) || { echo "missing $(EMBED_MODELS), which has no example to copy from"; exit 1; }

## check: run start-up checks (binaries, auth, config) and exit
check: build
$(BINARY) --config $(CONFIG) --check
Expand DownExpand Up@@ -99,15 +111,15 @@ print-service: build
$(BINARY) --print-service

## test: run the full test suite with the race detector
test:
test: embed-ready
go test -race $(PKG)

## coverage: run tests and print per-function coverage
coverage:
coverage: embed-ready
go test -race -coverprofile=coverage.out $(PKG)
go tool cover -func=coverage.out

vet:
vet: embed-ready
go vet $(PKG)

fmt:
Expand All@@ -124,11 +136,11 @@ fmt-check:
lint: vet fmt-check

## staticcheck: run staticcheck (must be installed: go install honnef.co/go/tools/cmd/staticcheck@latest)
staticcheck:
staticcheck: embed-ready
staticcheck $(PKG)

## vulcheck: run govulncheck (must be installed: go install golang.org/x/vuln/cmd/govulncheck@latest)
vulcheck:
vulcheck: embed-ready
govulncheck $(PKG)

## ci: everything CI should run — lint, then the full test suite
Expand Down
39 changes: 37 additions & 2 deletions README.md
Original file line numberDiff line numberDiff line change
Expand Up@@ -25,6 +25,7 @@ gh search issues --label agent-ready gh search prs --author <bot>
- [Quick start](#quick-start)
- [CLI flags](#cli-flags)
- [How work is selected](#how-work-is-selected)
- [Verification](#verification)
- [Lifecycle of one issue](#lifecycle-of-one-issue)
- [Responding to PR comments](#responding-to-pr-comments)
- [Configuration reference](#configuration-reference)
Expand DownExpand Up@@ -193,6 +194,37 @@ a `labels_failed` run event, and reported to Discord.
Discovery is parallel across repositories; within one repository, work is serial (one issue in
flight at a time), controlled by `run.max_concurrent_repos`.

## Verification

The test command is the repository's own, never a built-in assumption about it. `verify.commands`
takes an explicit per-repo command; otherwise `verify.auto_detect` reads the worktree: a `Makefile`
with a `test:` target wins (it is the repo's own opinion about how it is tested), then `go.mod`,
then a `package.json` with a `test` script, then `Cargo.toml`, then `pyproject.toml`/`pytest.ini`/
`tox.ini`. Nothing recognisable means nothing is run.

Three outcomes are distinguished, because they mean different things to a reviewer:

| Outcome | Meaning |
| ------------- | ---------------------------------------------------------------------------------- |
| `passed` | the command ran and exited zero |
| `failed` | the command ran and exited non-zero — the change is suspect |
| `unavailable` | the command could not be run at all — **the environment is wrong, not the change** |
| `skipped` | no command was configured or detected |

`unavailable` exists because the two failure modes are easy to confuse and expensive to confuse.
A daemon started by systemd inherits `/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin`
— which contains no language toolchain — so a repository that tests itself perfectly well from a
login shell fails with `make: go: No such file or directory`. Reporting that as "tests failed"
blames the agent's code for the operator's `PATH`. The installed systemd unit sets a `PATH` covering
the usual install locations, and `GOCACHE`/`GOMODCACHE` under `~/.agent-loop/cache` (the unit makes
`$HOME` read-only, and Go's caches default to `$HOME`). Anything beyond that goes in `verify.env`.

**Your repository must build from a clean checkout.** The agent works in a fresh `git worktree`, so
anything gitignored is absent there. This repository learned that the hard way: `embedded.go` has
`//go:embed config.json` while `config.json` is gitignored, so a clean checkout failed to compile
with `pattern config.json: no matching files found` — every compiling `make` target now depends on
`embed-ready`, which creates it from `config.example.json`.

## Lifecycle of one issue

1. **Discover** — `gh search issues --label <label> --state open` scoped to `github.owners`, minus
Expand DownExpand Up@@ -223,7 +255,8 @@ flight at a time), controlled by `run.max_concurrent_repos`.
queryable later via `GET /sessions`.
6. **Verify** — the repository's own test command runs (auto-detected, or from `verify.commands`).
A failing suite does **not** block the PR — it's a draft either way, and the failure is reported
in the PR body so a human sees it immediately.
in the PR body so a human sees it immediately. A command that could not be run at all is
reported separately from one that ran and failed (see [Verification](#verification)).
7. **Deliver** — the remote is re-checked, then pushed; a draft PR is opened with `Closes #<n>`,
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`. Every commit on
Expand DownExpand Up@@ -338,7 +371,8 @@ This repository's own `config.json` is also **compiled into the binary** at buil
},
"verify": {
"auto_detect": true,
"commands": {}
"commands": {},
"env": { "PATH": "/usr/local/go/bin:/usr/local/bin:/usr/bin:/bin" }
},
"server": {
"addr": "127.0.0.1:8787"
Expand DownExpand Up@@ -391,6 +425,7 @@ This repository's own `config.json` is also **compiled into the binary** at buil
| `claude.credentials_path` | where the CLI's OAuth token lives, read for the advisory usage snapshot |
| `verify.auto_detect` | try `Makefile` → `go.mod` → `package.json` → `Cargo.toml` → `pyproject.toml`, in that order |
| `verify.commands` | per-repo override, keyed `"owner/name": "shell command"` |
| `verify.env` | extra environment for the test command — mainly `PATH`, so the daemon can find language toolchains (see [Verification](#verification)) |
| `server.addr` | control API bind address; keep loopback-only |
| `store.path` | SQLite database path |
| `discord.enabled` | turn on Discord status notifications (see [Discord notifications](#discord-notifications)) |
Expand Down
11 changes: 9 additions & 2 deletions config.example.json
Original file line numberDiff line numberDiff line change
Expand Up@@ -20,7 +20,11 @@
"ack_reaction": "eyes",
"done_reaction": "+1",
"allowed_authors": [],
"allowed_associations": ["OWNER", "MEMBER", "COLLABORATOR"]
"allowed_associations": [
"OWNER",
"MEMBER",
"COLLABORATOR"
]
}
},
"workspace": {
Expand DownExpand Up@@ -50,7 +54,10 @@
},
"verify": {
"auto_detect": true,
"commands": {}
"commands": {},
"env": {
"PATH": "/usr/local/go/bin:/usr/local/bin:/usr/bin:/bin"
}
},
"server": {
"addr": "127.0.0.1:8787"
Expand Down
6 changes: 6 additions & 0 deletions internal/config/config.go
Original file line numberDiff line numberDiff line change
Expand Up@@ -163,6 +163,12 @@ type VerifyConfig struct {
AutoDetect bool `json:"auto_detect"`
// Commands maps "owner/name" to an explicit shell command.
Commands map[string]string `json:"commands"`
// Env is added to the environment of every verification command. It exists
// mainly for PATH: a daemon started by systemd does not inherit a login
// shell's PATH, so language toolchains installed outside /usr/bin (Go under
// /usr/local/go/bin, anything under ~/go/bin or a version manager) are
// invisible to it and the repository's own test command cannot run.
Env map[string]string `json:"env"`
}

type ServerConfig struct {
Expand Down
6 changes: 5 additions & 1 deletion internal/discord/notifier.go
Original file line numberDiff line numberDiff line change
Expand Up@@ -283,12 +283,16 @@ func (n *Notifier) VerifyResult(r RunRef, v verify.Result) {
what, color = "Verify failed", colorOrange
case store.VerifySkipped:
what, color = "Verify skipped", colorGray
case store.VerifyUnavailable:
// Yellow, not orange: this needs the operator's attention, but it is
// the daemon's environment that is wrong, not the agent's change.
what, color = "Verify could not run", colorYellow
}
fields := append(r.fields(), embedField{Name: "Status", Value: v.Status, Inline: true})
if v.Command != "" {
fields = append(fields, embedField{Name: "Command", Value: truncate(v.Command, 1000), Inline: false})
}
if v.Status == store.VerifyFailed {
if v.Status == store.VerifyFailed || v.Status == store.VerifyUnavailable {
if out := strings.TrimSpace(v.Output); out != "" {
fields = append(fields, embedField{Name: "Output (tail)", Value: codeBlock(tail(out, 900)), Inline: false})
}
Expand Down
14 changes: 14 additions & 0 deletions internal/install/coding-agent-loop.service
Original file line numberDiff line numberDiff line change
Expand Up@@ -11,6 +11,20 @@ ExecStart=/opt/coding-agent-loop/bin/coding-agent-loop --config /opt/coding-agen
Restart=on-failure
RestartSec=30s

# systemd's default PATH is /usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin,
# which contains no language toolchain. A repository that tests itself fine from
# a login shell then fails verification with "go: No such file or directory",
# and the daemon has no way to know that is an environment problem rather than a
# broken change. The usual install locations are added here; anything else
# belongs in config.json under verify.env.
Environment=PATH=/usr/local/go/bin:{{HOME}}/go/bin:{{HOME}}/.local/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin

# Go writes its build and module caches under $HOME, which ProtectHome makes
# read-only below. Point them somewhere the unit can actually write, or `go
# test` fails on a permission error even once go itself is on PATH.
Environment=GOCACHE={{HOME}}/.agent-loop/cache/go-build
Environment=GOMODCACHE={{HOME}}/.agent-loop/cache/go-mod

# SIGTERM makes the loop stop claiming and drain whatever is in flight, so give
# it longer than one run timeout before killing it.
KillSignal=SIGTERM
Expand Down
14 changes: 14 additions & 0 deletions internal/orchestrator/report.go
Original file line numberDiff line numberDiff line change
Expand Up@@ -52,6 +52,15 @@ func prBody(r prReport) string {
b.WriteString(out)
b.WriteString("\n```\n\n</details>\n\n")
}
case store.VerifyUnavailable:
fmt.Fprintf(&b, "**The tests could not be run** (`%s`) — this says nothing about the change "+
"below, only that the daemon's environment is missing something the command needs. "+
"Run it yourself before judging this PR.\n\n", r.Verify.Command)
if out := strings.TrimSpace(r.Verify.Output); out != "" {
b.WriteString("<details><summary>Output (tail)</summary>\n\n```\n")
b.WriteString(out)
b.WriteString("\n```\n\n</details>\n\n")
}
default:
b.WriteString("No test command was configured or detected for this repository, so nothing was run.\n\n")
}
Expand DownExpand Up@@ -85,6 +94,9 @@ func issueComment(prURL, runID string, v verify.Result) string {
fmt.Fprintf(&b, "Tests passed (`%s`).\n", v.Command)
case store.VerifyFailed:
fmt.Fprintf(&b, "Tests failed (`%s`) — see the PR for output.\n", v.Command)
case store.VerifyUnavailable:
fmt.Fprintf(&b, "The tests could not be run (`%s`) — the daemon's environment is missing "+
"something the command needs, so the change is unverified rather than broken.\n", v.Command)
default:
b.WriteString("No test command was detected for this repository.\n")
}
Expand DownExpand Up@@ -165,6 +177,8 @@ func prCommentComment(handled []gh.PRComment, summary, runID string, v verify.Re
fmt.Fprintf(&b, "Tests passed (`%s`).\n", v.Command)
case store.VerifyFailed:
fmt.Fprintf(&b, "Tests failed (`%s`).\n", v.Command)
case store.VerifyUnavailable:
fmt.Fprintf(&b, "Tests could not be run (`%s`) — missing toolchain, not a broken change.\n", v.Command)
default:
b.WriteString("No test command was detected for this repository.\n")
}
Expand Down
6 changes: 6 additions & 0 deletions internal/store/store.go
Original file line numberDiff line numberDiff line change
Expand Up@@ -54,6 +54,12 @@ const (
VerifySkipped = "skipped"
VerifyPassed = "passed"
VerifyFailed = "failed"
// VerifyUnavailable means the command could not be run at all — the
// toolchain it needs is not on the daemon's PATH, or the environment
// forbade it. It is deliberately not VerifyFailed: the tests did not fail,
// they never ran, and telling a reviewer otherwise blames the agent's code
// for the operator's environment.
VerifyUnavailable = "unavailable"
)

// Gate kinds. Model cooldowns use the prefix GateModelPrefix + model ID.
Expand Down
66 changes: 65 additions & 1 deletion internal/verify/verify.go
Original file line numberDiff line numberDiff line change
Expand Up@@ -11,6 +11,7 @@ import (
"bytes"
"context"
"encoding/json"
"fmt"
"os"
"os/exec"
"path/filepath"
Expand DownExpand Up@@ -107,7 +108,7 @@ func (r *Runner) Run(ctx context.Context, repo, worktree string) Result {

cmd := exec.CommandContext(ctx, "sh", "-c", cmdline)
cmd.Dir = worktree
cmd.Env = append(os.Environ(), "CI=true")
cmd.Env = r.env()
// Test suites fork freely (servers, containers, watchers); without this a
// timed-out suite leaves survivors holding the output pipe open.
proc.Isolate(cmd)
Expand All@@ -123,11 +124,74 @@ func (r *Runner) Run(ctx context.Context, repo, worktree string) Result {
case ctx.Err() != nil:
return Result{Status: store.VerifyFailed, Command: cmdline, Output: out, Err: ctx.Err()}
case err != nil:
// "The tests failed" and "the tests could not be run" are different
// facts, and only one of them is about the code under review.
if tool, missing := missingTool(cmd, out); missing {
return Result{
Status: store.VerifyUnavailable,
Command: cmdline,
Output: out,
Err: fmt.Errorf("%q is not available to the daemon (PATH=%s): %w", tool, r.pathOf(), err),
}
}
return Result{Status: store.VerifyFailed, Command: cmdline, Output: out, Err: err}
}
return Result{Status: store.VerifyPassed, Command: cmdline, Output: out}
}

// notFound matches the way a shell, make, and the usual build tools all say
// that something is not on PATH. Exit status 127 is the POSIX convention for
// it, but the wrapper (make, npm, cargo) often swallows that and exits 1 or 2
// with the message instead, so both are checked.
var notFound = regexp.MustCompile(`(?i)([\w./+-]+): (?:command not found|No such file or directory|not found)`)

// missingTool reports whether a command failed because something it needs is
// not installed or not on the daemon's PATH, and names it when it can.
func missingTool(cmd *exec.Cmd, output string) (string, bool) {
m := notFound.FindStringSubmatch(output)
name := ""
if len(m) > 1 {
name = filepath.Base(strings.TrimSpace(m[1]))
}
if name != "" {
return name, true
}
// No recognisable message, but 127 says it plainly enough on its own.
if cmd.ProcessState != nil && cmd.ProcessState.ExitCode() == 127 {
return "a required command", true
}
return "", false
}

// env is the environment the test command runs in: the daemon's own, plus
// CI=true, plus whatever the operator declared in verify.env.
//
// This matters more than it looks. A daemon started by systemd inherits a
// minimal PATH that excludes every language toolchain installed outside
// /usr/bin — Go under /usr/local/go/bin, anything under ~/go/bin or a version
// manager — so a repository that tests itself perfectly well from a login
// shell cannot be verified at all without saying where its tools live.
func (r *Runner) env() []string {
env := append(os.Environ(), "CI=true")
for k, v := range r.Cfg.Env {
if k = strings.TrimSpace(k); k != "" {
env = append(env, k+"="+v)
}
}
return env
}

// pathOf reports the PATH the command was given, for error messages.
func (r *Runner) pathOf() string {
path := os.Getenv("PATH")
for k, v := range r.Cfg.Env {
if strings.EqualFold(strings.TrimSpace(k), "PATH") {
path = v
}
}
return path
}

func tail(s string, n int) string {
s = strings.TrimSpace(s)
if len(s) <= n {
Expand Down
Loading
Loading