From 3626ad2a3a753d57ce71a8af9f20d7dbf6f94cb6 Mon Sep 17 00:00:00 2001 From: Kristiyan Stefanov Nikolov Date: Mon, 15 Jun 2026 13:25:00 +0300 Subject: [PATCH 1/2] feat(factory): add GitHub issue trigger provider --- README.md | 7 +- cmd/AGENTS.md | 1 + cmd/bach-github-issue-trigger/main.go | 23 ++ docs/reference.md | 20 +- docs/reference/36-factory-work-items.md | 20 +- internal/AGENTS.md | 2 + internal/githubissuetrigger/AGENTS.md | 24 ++ internal/githubissuetrigger/provider.go | 395 +++++++++++++++++++ internal/githubissuetrigger/provider_test.go | 155 ++++++++ 9 files changed, 642 insertions(+), 5 deletions(-) create mode 100644 cmd/bach-github-issue-trigger/main.go create mode 100644 internal/githubissuetrigger/AGENTS.md create mode 100644 internal/githubissuetrigger/provider.go create mode 100644 internal/githubissuetrigger/provider_test.go diff --git a/README.md b/README.md index 831ea17..5ba184c 100644 --- a/README.md +++ b/README.md @@ -173,8 +173,13 @@ factory "delivery" { manual {} provider "github_issues" { - command = ["bach-trigger-fixture"] + command = ["bach-github-issue-trigger"] poll_interval = "5m" + config = { + repo = "ApplauseLab/bachkator" + token_env = "GITHUB_TOKEN" + labels = "factory:ship" + } route { label = "factory:ship" diff --git a/cmd/AGENTS.md b/cmd/AGENTS.md index 75dbbcd..9018314 100644 --- a/cmd/AGENTS.md +++ b/cmd/AGENTS.md @@ -9,6 +9,7 @@ - `cmd/bach`: main Bach CLI executable. - `cmd/bach-docs-gen`: reference documentation generator invoked by the `shell/docs-generate` Bach target. - `cmd/bach-file-lines`: internal Go file length checker for architecture hygiene. +- `cmd/bach-github-issue-trigger`: GitHub Issues Trigger Provider executable. - `cmd/bach-lint-cap`: internal lint-report capping helper invoked by the `shell/lint` Bach target. - Command wiring and production dependency assembly belong in `internal/app` and `internal/cli`, not in executable packages. diff --git a/cmd/bach-github-issue-trigger/main.go b/cmd/bach-github-issue-trigger/main.go new file mode 100644 index 0000000..543d80c --- /dev/null +++ b/cmd/bach-github-issue-trigger/main.go @@ -0,0 +1,23 @@ +package main + +import ( + "context" + "fmt" + "os" + + "github.com/applauselab/bachkator/internal/githubissuetrigger" + "github.com/applauselab/bachkator/pkg/triggerprotocol" +) + +func main() { + provider := githubissuetrigger.New(nil) + if err := triggerprotocol.Serve( + context.Background(), + os.Stdin, + os.Stdout, + provider, + ); err != nil { + _, _ = fmt.Fprintln(os.Stderr, err) + os.Exit(1) + } +} diff --git a/docs/reference.md b/docs/reference.md index 51f5ba1..4e949cb 100644 --- a/docs/reference.md +++ b/docs/reference.md @@ -1866,10 +1866,12 @@ factory "delivery" { manual {} provider "github_issues" { - command = ["bach-trigger-fixture"] + command = ["bach-github-issue-trigger"] poll_interval = "5m" config = { - items_path = ".bach/fixtures/trigger-items.json" + repo = "example/repo" + token_env = "GITHUB_TOKEN" + labels = "factory:ship" } route { @@ -1899,6 +1901,20 @@ Provider trigger fields: - `config`: optional map of string keys to string values passed to the provider during handshake and poll. - `route { label = "...", workflow = "..." }`: optional routing rule. Items with the matching label are routed to the named workflow. When a Factory has multiple workflows, at least one route is required; with a single workflow, omitted routes default to that workflow. +The `bach-github-issue-trigger` provider reads GitHub Issues through the trigger provider protocol. It accepts these `config` keys: + +- `repo`: required GitHub repository in `owner/name` form. +- `token_env`: optional environment variable name containing a GitHub token; defaults to `GITHUB_TOKEN`. The token value must stay out of the Bachfile. +- `api_url`: optional GitHub API base URL; defaults to `https://api.github.com`. +- `labels`: optional comma-separated GitHub label filter passed to the Issues API. +- `state`: optional issue state, one of `open`, `closed`, or `all`; defaults to `open`. +- `since`: optional RFC3339 timestamp used as the initial cursor when no Bach cursor exists. +- `per_page`: optional GitHub page size from `1` to `100`; defaults to `100`. +- `max_pages`: optional positive page limit per poll; defaults to `5`. +- `priority_label_prefix`: optional label prefix for Work Item priority extraction; defaults to `priority:`. + +GitHub Issue labels are preserved as Work Item labels for route matching. Pull requests returned by the GitHub Issues API are ignored. The provider advances its cursor from GitHub `updated_at` timestamps and treats `priority:critical`, `priority:urgent`, `priority:high`, `priority:normal`, and `priority:low` labels as Bach priorities. + Validation rules: - Factory and workflow names must start with an ASCII letter, digit, or `_`, and may then contain ASCII letters, digits, `_`, `.`, or `-`. diff --git a/docs/reference/36-factory-work-items.md b/docs/reference/36-factory-work-items.md index bb3f19b..3c6cda6 100644 --- a/docs/reference/36-factory-work-items.md +++ b/docs/reference/36-factory-work-items.md @@ -41,10 +41,12 @@ factory "delivery" { manual {} provider "github_issues" { - command = ["bach-trigger-fixture"] + command = ["bach-github-issue-trigger"] poll_interval = "5m" config = { - items_path = ".bach/fixtures/trigger-items.json" + repo = "example/repo" + token_env = "GITHUB_TOKEN" + labels = "factory:ship" } route { @@ -74,6 +76,20 @@ Provider trigger fields: - `config`: optional map of string keys to string values passed to the provider during handshake and poll. - `route { label = "...", workflow = "..." }`: optional routing rule. Items with the matching label are routed to the named workflow. When a Factory has multiple workflows, at least one route is required; with a single workflow, omitted routes default to that workflow. +The `bach-github-issue-trigger` provider reads GitHub Issues through the trigger provider protocol. It accepts these `config` keys: + +- `repo`: required GitHub repository in `owner/name` form. +- `token_env`: optional environment variable name containing a GitHub token; defaults to `GITHUB_TOKEN`. The token value must stay out of the Bachfile. +- `api_url`: optional GitHub API base URL; defaults to `https://api.github.com`. +- `labels`: optional comma-separated GitHub label filter passed to the Issues API. +- `state`: optional issue state, one of `open`, `closed`, or `all`; defaults to `open`. +- `since`: optional RFC3339 timestamp used as the initial cursor when no Bach cursor exists. +- `per_page`: optional GitHub page size from `1` to `100`; defaults to `100`. +- `max_pages`: optional positive page limit per poll; defaults to `5`. +- `priority_label_prefix`: optional label prefix for Work Item priority extraction; defaults to `priority:`. + +GitHub Issue labels are preserved as Work Item labels for route matching. Pull requests returned by the GitHub Issues API are ignored. The provider advances its cursor from GitHub `updated_at` timestamps and treats `priority:critical`, `priority:urgent`, `priority:high`, `priority:normal`, and `priority:low` labels as Bach priorities. + Validation rules: - Factory and workflow names must start with an ASCII letter, digit, or `_`, and may then contain ASCII letters, digits, `_`, `.`, or `-`. diff --git a/internal/AGENTS.md b/internal/AGENTS.md index e13b624..72d4333 100644 --- a/internal/AGENTS.md +++ b/internal/AGENTS.md @@ -14,6 +14,7 @@ - `cli`: Cobra command adapters and public CLI contract presentation. - `config`: Bachfile loading, decoding, validation, and config-time references. - `dag`, `graph`, `model`, and `target`: target model, dependency graph, graph plugins, and target-kind registration behavior. +- `githubissuetrigger`: GitHub Issues Trigger Provider implementation. - `evidence`: trusted local evidence path handling, private artifact writes, and Agent Target workspace path resolution. - `factory`: Factory Work Item service logic, manual queue validation, intake evidence creation, and queue-facing DTOs. - `factorydaemon`: Factory daemon leases, queue polling, Work Item phase orchestration, and workflow execution. @@ -67,6 +68,7 @@ - `internal/factorydaemon/AGENTS.md`: Factory daemon leases, polling, claims, and workflow phase orchestration. - `internal/git/AGENTS.md`: git evidence helpers. - `internal/graph/AGENTS.md`: affected-target, explain, provenance, and risk graph analysis. +- `internal/githubissuetrigger/AGENTS.md`: GitHub Issues Trigger Provider implementation. - `internal/model/AGENTS.md`: shared domain model and target address semantics. - `internal/plan/AGENTS.md`: Markdown Plan parsing, validation, hashing, graph construction, and pure status derivation. - `internal/planbatch/AGENTS.md`: multi-Plan batch execution and review-queue orchestration. diff --git a/internal/githubissuetrigger/AGENTS.md b/internal/githubissuetrigger/AGENTS.md new file mode 100644 index 0000000..bb06c0e --- /dev/null +++ b/internal/githubissuetrigger/AGENTS.md @@ -0,0 +1,24 @@ +# Agent Instructions + +## Purpose + +`internal/githubissuetrigger/` owns the GitHub Issues Trigger Provider implementation used by the +`bach-github-issue-trigger` executable. + +## Local Contracts + +- Speak only the public `bach.trigger.v1` protocol through `pkg/triggerprotocol`. +- Keep GitHub API access read-only; do not mutate issues, labels, comments, or repository state. +- Read GitHub tokens from environment variables named by provider config. Never accept token values in + Bachfile config and never log token values. +- Keep Work Item mapping deterministic: stable source IDs, issue labels preserved for Factory routing, and + cursor advancement based on GitHub `updated_at` values. + +## Verification + +- Use `go run ./cmd/bach run shell/test` after provider changes. +- Use `go run ./cmd/bach run shell/fmt` after Go edits. + +## Child DOX Index + +- No child `AGENTS.md` files currently exist under `internal/githubissuetrigger/`. diff --git a/internal/githubissuetrigger/provider.go b/internal/githubissuetrigger/provider.go new file mode 100644 index 0000000..b975483 --- /dev/null +++ b/internal/githubissuetrigger/provider.go @@ -0,0 +1,395 @@ +package githubissuetrigger + +import ( + "context" + "encoding/json" + "fmt" + "io" + "net/http" + "net/url" + "os" + "strconv" + "strings" + "time" + + "github.com/applauselab/bachkator/pkg/triggerprotocol" +) + +const ( + ProviderName = "bach-github-issue-trigger" + providerVersion = "v1" + defaultAPIURL = "https://api.github.com" + defaultTokenEnv = "GITHUB_TOKEN" + defaultState = "open" + defaultPerPage = 100 + defaultMaxPages = 5 + defaultPriorityLabelPrefx = "priority:" + maxErrorBodyBytes = 4096 +) + +type Provider struct { + client *http.Client + config providerConfig +} + +type providerConfig struct { + Repo string + APIURL string + TokenEnv string + State string + Labels string + PerPage int + MaxPages int + Since string + PriorityLabelPrefix string +} + +type githubIssue struct { + Number int `json:"number"` + HTMLURL string `json:"html_url"` + Title string `json:"title"` + Body *string `json:"body"` + State string `json:"state"` + Labels []githubLabel `json:"labels"` + User githubUser `json:"user"` + CreatedAt time.Time `json:"created_at"` + UpdatedAt time.Time `json:"updated_at"` + PullRequest *struct{} `json:"pull_request"` +} + +type githubLabel struct { + Name string `json:"name"` +} + +type githubUser struct { + Login string `json:"login"` +} + +func New(client *http.Client) *Provider { + if client == nil { + client = http.DefaultClient + } + return &Provider{client: client} +} + +func (p *Provider) Handshake( + _ context.Context, + params triggerprotocol.HandshakeParams, +) (triggerprotocol.HandshakeResult, error) { + if params.Protocol != triggerprotocol.ProtocolVersion { + return triggerprotocol.HandshakeResult{}, triggerprotocol.NewError( + triggerprotocol.ErrorUnsupportedProtocol, + "unsupported protocol "+params.Protocol, + ) + } + cfg, err := parseConfig(params.Config) + if err != nil { + return triggerprotocol.HandshakeResult{}, triggerError(err) + } + p.config = cfg + return triggerprotocol.HandshakeResult{ + Protocol: triggerprotocol.ProtocolVersion, + Provider: ProviderName, + Version: providerVersion, + Capabilities: []triggerprotocol.Capability{ + triggerprotocol.CapabilityPoll, + triggerprotocol.CapabilityAck, + triggerprotocol.CapabilityNack, + }, + }, nil +} + +func (p *Provider) Poll( + ctx context.Context, + params triggerprotocol.PollParams, +) (triggerprotocol.PollResult, error) { + cfg := p.config + if len(params.Config) > 0 { + parsed, err := parseConfig(params.Config) + if err != nil { + return triggerprotocol.PollResult{}, triggerError(err) + } + cfg = parsed + p.config = parsed + } + if cfg.Repo == "" { + return triggerprotocol.PollResult{}, triggerError(fmt.Errorf("repo is required")) + } + cursor := strings.TrimSpace(params.Cursor) + if cursor == "" { + cursor = cfg.Since + } + if cursor != "" { + if _, err := parseTime(cursor); err != nil { + return triggerprotocol.PollResult{}, triggerError( + fmt.Errorf("cursor is not RFC3339: %w", err), + ) + } + } + issues, nextCursor, err := p.fetchIssues(ctx, cfg, cursor) + if err != nil { + return triggerprotocol.PollResult{}, triggerprotocol.NewError( + triggerprotocol.ErrorInternal, + err.Error(), + ) + } + items := make([]triggerprotocol.PollItem, 0, len(issues)) + for _, issue := range issues { + if issue.PullRequest != nil { + continue + } + items = append(items, mapIssue(cfg, issue)) + } + return triggerprotocol.PollResult{Cursor: nextCursor, Items: items}, nil +} + +func (p *Provider) Ack(_ context.Context, _ triggerprotocol.AckParams) error { + return nil +} + +func (p *Provider) Nack(_ context.Context, _ triggerprotocol.NackParams) error { + return nil +} + +func (p *Provider) fetchIssues( + ctx context.Context, + cfg providerConfig, + cursor string, +) ([]githubIssue, string, error) { + issues := []githubIssue{} + nextCursor := cursor + for page := 1; page <= cfg.MaxPages; page++ { + batch, err := p.fetchIssuePage(ctx, cfg, cursor, page) + if err != nil { + return nil, "", err + } + for _, issue := range batch { + issues = append(issues, issue) + if issue.UpdatedAt.IsZero() { + continue + } + updated := issue.UpdatedAt.UTC().Format(time.RFC3339Nano) + if nextCursor == "" || compareTimes(updated, nextCursor) > 0 { + nextCursor = updated + } + } + if len(batch) < cfg.PerPage { + break + } + } + return issues, nextCursor, nil +} + +func (p *Provider) fetchIssuePage( + ctx context.Context, + cfg providerConfig, + cursor string, + page int, +) ([]githubIssue, error) { + requestURL, err := issueListURL(cfg, cursor, page) + if err != nil { + return nil, err + } + req, err := http.NewRequestWithContext(ctx, http.MethodGet, requestURL, nil) + if err != nil { + return nil, err + } + req.Header.Set("Accept", "application/vnd.github+json") + req.Header.Set("User-Agent", ProviderName) + req.Header.Set("X-GitHub-Api-Version", "2022-11-28") + if token := strings.TrimSpace(os.Getenv(cfg.TokenEnv)); token != "" { + req.Header.Set("Authorization", "Bearer "+token) + } + resp, err := p.client.Do(req) + if err != nil { + return nil, fmt.Errorf("github issues request: %w", err) + } + defer func() { + _ = resp.Body.Close() + }() + if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices { + body, _ := io.ReadAll(io.LimitReader(resp.Body, maxErrorBodyBytes)) + return nil, fmt.Errorf( + "github issues request failed with %s: %s", + resp.Status, + strings.TrimSpace(string(body)), + ) + } + var issues []githubIssue + if err := json.NewDecoder(resp.Body).Decode(&issues); err != nil { + return nil, fmt.Errorf("decode github issues: %w", err) + } + return issues, nil +} + +func parseConfig(values map[string]string) (providerConfig, error) { + cfg := providerConfig{ + APIURL: defaultAPIURL, + TokenEnv: defaultTokenEnv, + State: defaultState, + PerPage: defaultPerPage, + MaxPages: defaultMaxPages, + PriorityLabelPrefix: defaultPriorityLabelPrefx, + } + if values == nil { + values = map[string]string{} + } + cfg.Repo = strings.TrimSpace(values["repo"]) + if cfg.Repo == "" { + return providerConfig{}, fmt.Errorf("repo is required") + } + if strings.Count(cfg.Repo, "/") != 1 { + return providerConfig{}, fmt.Errorf("repo must be owner/name") + } + if value := strings.TrimSpace(values["api_url"]); value != "" { + cfg.APIURL = strings.TrimRight(value, "/") + } + if _, err := url.ParseRequestURI(cfg.APIURL); err != nil { + return providerConfig{}, fmt.Errorf("api_url is invalid: %w", err) + } + if value := strings.TrimSpace(values["token_env"]); value != "" { + cfg.TokenEnv = value + } + if value := strings.TrimSpace(values["state"]); value != "" { + cfg.State = value + } + if cfg.State != "open" && cfg.State != "closed" && cfg.State != "all" { + return providerConfig{}, fmt.Errorf("state must be open, closed, or all") + } + cfg.Labels = strings.TrimSpace(values["labels"]) + if value := strings.TrimSpace(values["per_page"]); value != "" { + perPage, err := strconv.Atoi(value) + if err != nil || perPage < 1 || perPage > 100 { + return providerConfig{}, fmt.Errorf("per_page must be an integer from 1 to 100") + } + cfg.PerPage = perPage + } + if value := strings.TrimSpace(values["max_pages"]); value != "" { + maxPages, err := strconv.Atoi(value) + if err != nil || maxPages < 1 { + return providerConfig{}, fmt.Errorf("max_pages must be a positive integer") + } + cfg.MaxPages = maxPages + } + if value := strings.TrimSpace(values["since"]); value != "" { + if _, err := parseTime(value); err != nil { + return providerConfig{}, fmt.Errorf("since is not RFC3339: %w", err) + } + cfg.Since = value + } + if value := values["priority_label_prefix"]; value != "" { + cfg.PriorityLabelPrefix = value + } + return cfg, nil +} + +func issueListURL(cfg providerConfig, cursor string, page int) (string, error) { + parts := strings.Split(cfg.Repo, "/") + base, err := url.Parse(strings.TrimRight(cfg.APIURL, "/") + "/") + if err != nil { + return "", err + } + base.Path = strings.TrimRight(base.Path, "/") + "/repos/" + url.PathEscape(parts[0]) + "/" + + url.PathEscape(parts[1]) + "/issues" + query := base.Query() + query.Set("state", cfg.State) + query.Set("sort", "updated") + query.Set("direction", "asc") + query.Set("per_page", strconv.Itoa(cfg.PerPage)) + query.Set("page", strconv.Itoa(page)) + if cursor != "" { + query.Set("since", cursor) + } + if cfg.Labels != "" { + query.Set("labels", cfg.Labels) + } + base.RawQuery = query.Encode() + return base.String(), nil +} + +func mapIssue(cfg providerConfig, issue githubIssue) triggerprotocol.PollItem { + labels := issueLabels(issue.Labels) + body := "" + if issue.Body != nil { + body = *issue.Body + } + metadata := map[string]string{ + "github_repo": cfg.Repo, + "github_number": strconv.Itoa(issue.Number), + "github_state": issue.State, + "github_author": issue.User.Login, + "github_created_at": issue.CreatedAt.UTC().Format(time.RFC3339Nano), + "github_updated_at": issue.UpdatedAt.UTC().Format(time.RFC3339Nano), + } + return triggerprotocol.PollItem{ + Source: triggerprotocol.ItemSource{ + Type: "github_issue", + ID: cfg.Repo + "#" + strconv.Itoa(issue.Number), + URL: issue.HTMLURL, + Revision: issue.UpdatedAt.UTC().Format(time.RFC3339Nano), + }, + Title: issue.Title, + Body: body, + Labels: labels, + Priority: priorityFromLabels(labels, cfg.PriorityLabelPrefix), + Metadata: metadata, + } +} + +func issueLabels(labels []githubLabel) []string { + result := make([]string, 0, len(labels)) + for _, label := range labels { + name := strings.TrimSpace(label.Name) + if name != "" { + result = append(result, name) + } + } + return result +} + +func priorityFromLabels(labels []string, prefix string) string { + valid := map[string]struct{}{ + "critical": {}, + "urgent": {}, + "high": {}, + "normal": {}, + "low": {}, + } + for _, label := range labels { + candidate := strings.ToLower(strings.TrimSpace(label)) + if prefix != "" && strings.HasPrefix(candidate, strings.ToLower(prefix)) { + candidate = strings.TrimSpace(strings.TrimPrefix(candidate, strings.ToLower(prefix))) + } + if _, ok := valid[candidate]; ok { + return candidate + } + } + return "" +} + +func triggerError(err error) error { + return triggerprotocol.NewError(triggerprotocol.ErrorValidationFailed, err.Error()) +} + +func parseTime(value string) (time.Time, error) { + t, err := time.Parse(time.RFC3339Nano, value) + if err != nil { + return time.Time{}, err + } + return t.UTC(), nil +} + +func compareTimes(left, right string) int { + leftTime, leftErr := parseTime(left) + rightTime, rightErr := parseTime(right) + if leftErr != nil || rightErr != nil { + return strings.Compare(left, right) + } + if leftTime.After(rightTime) { + return 1 + } + if leftTime.Before(rightTime) { + return -1 + } + return 0 +} diff --git a/internal/githubissuetrigger/provider_test.go b/internal/githubissuetrigger/provider_test.go new file mode 100644 index 0000000..eaeadbf --- /dev/null +++ b/internal/githubissuetrigger/provider_test.go @@ -0,0 +1,155 @@ +package githubissuetrigger + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/applauselab/bachkator/pkg/triggerprotocol" +) + +func TestProviderPollMapsGitHubIssues(t *testing.T) { + t.Setenv("BACH_TEST_GITHUB_TOKEN", "secret-token") + + var firstRequest *http.Request + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if firstRequest == nil { + firstRequest = r.Clone(r.Context()) + } + if r.URL.Query().Get("page") == "2" { + _ = json.NewEncoder(w).Encode([]any{}) + return + } + _ = json.NewEncoder(w).Encode([]map[string]any{ + { + "number": 42, + "html_url": "https://github.com/acme/widgets/issues/42", + "title": "Ship the widget", + "body": "Build it with tests.", + "state": "open", + "labels": []map[string]string{ + {"name": "factory:ship"}, + {"name": "priority:high"}, + }, + "user": map[string]string{"login": "octo"}, + "created_at": "2026-06-01T10:00:00Z", + "updated_at": "2026-06-02T10:00:00Z", + }, + { + "number": 43, + "html_url": "https://github.com/acme/widgets/pull/43", + "title": "Ignore pull requests", + "state": "open", + "labels": []map[string]string{{"name": "factory:ship"}}, + "user": map[string]string{"login": "octo"}, + "created_at": "2026-06-01T10:00:00Z", + "updated_at": "2026-06-03T10:00:00Z", + "pull_request": map[string]string{ + "url": "https://api.github.com/repos/acme/widgets/pulls/43", + }, + }, + }) + })) + defer server.Close() + + provider := New(server.Client()) + _, err := provider.Handshake(context.Background(), triggerprotocol.HandshakeParams{ + Protocol: triggerprotocol.ProtocolVersion, + Factory: "delivery", + Trigger: "github_issues", + Config: map[string]string{ + "repo": "acme/widgets", + "api_url": server.URL, + "token_env": "BACH_TEST_GITHUB_TOKEN", + "labels": "factory:ship", + "per_page": "2", + }, + }) + if err != nil { + t.Fatalf("handshake error = %v", err) + } + + result, err := provider.Poll(context.Background(), triggerprotocol.PollParams{ + Cursor: "2026-06-01T00:00:00Z", + }) + if err != nil { + t.Fatalf("poll error = %v", err) + } + if firstRequest == nil { + t.Fatal("server did not receive request") + } + if got := firstRequest.URL.Path; got != "/repos/acme/widgets/issues" { + t.Fatalf("request path = %q", got) + } + query := firstRequest.URL.Query() + if query.Get("state") != "open" || query.Get("sort") != "updated" || + query.Get("direction") != "asc" || query.Get("since") != "2026-06-01T00:00:00Z" || + query.Get("labels") != "factory:ship" { + t.Fatalf("unexpected query = %s", firstRequest.URL.RawQuery) + } + if got := firstRequest.Header.Get("Authorization"); got != "Bearer secret-token" { + t.Fatalf("authorization header = %q", got) + } + if result.Cursor != "2026-06-03T10:00:00Z" { + t.Fatalf("cursor = %q", result.Cursor) + } + if len(result.Items) != 1 { + t.Fatalf("items len = %d", len(result.Items)) + } + item := result.Items[0] + if item.Source.Type != "github_issue" || item.Source.ID != "acme/widgets#42" || + item.Source.URL != "https://github.com/acme/widgets/issues/42" || + item.Source.Revision != "2026-06-02T10:00:00Z" { + t.Fatalf("source = %#v", item.Source) + } + if item.Title != "Ship the widget" || item.Body != "Build it with tests." { + t.Fatalf("item text = %#v", item) + } + if item.Priority != "high" { + t.Fatalf("priority = %q", item.Priority) + } + if item.Metadata["github_author"] != "octo" || item.Metadata["github_number"] != "42" { + t.Fatalf("metadata = %#v", item.Metadata) + } +} + +func TestProviderHandshakeValidatesConfig(t *testing.T) { + provider := New(nil) + _, err := provider.Handshake(context.Background(), triggerprotocol.HandshakeParams{ + Protocol: triggerprotocol.ProtocolVersion, + }) + if err == nil { + t.Fatal("handshake error is nil") + } + if got := err.Error(); got != "validation_failed: repo is required" { + t.Fatalf("handshake error = %q", got) + } +} + +func TestProviderPollReportsGitHubStatus(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + http.Error(w, "bad credentials", http.StatusUnauthorized) + })) + defer server.Close() + + provider := New(server.Client()) + _, err := provider.Handshake(context.Background(), triggerprotocol.HandshakeParams{ + Protocol: triggerprotocol.ProtocolVersion, + Config: map[string]string{ + "repo": "acme/widgets", + "api_url": server.URL, + }, + }) + if err != nil { + t.Fatalf("handshake error = %v", err) + } + _, err = provider.Poll(context.Background(), triggerprotocol.PollParams{}) + if err == nil { + t.Fatal("poll error is nil") + } + if got := err.Error(); got != "internal: github issues request failed with 401 Unauthorized: bad credentials" { + t.Fatalf("poll error = %q", got) + } +} From 6e8df9a0b60fc3c932d8c8016170c103d1085e69 Mon Sep 17 00:00:00 2001 From: Kristiyan Stefanov Nikolov Date: Tue, 16 Jun 2026 00:34:05 +0300 Subject: [PATCH 2/2] fix(factory): harden GitHub issue trigger polling --- docs/reference.md | 6 +- docs/reference/36-factory-work-items.md | 6 +- internal/factorydaemon/AGENTS.md | 1 + internal/factorydaemon/triggers.go | 15 +++- internal/factorydaemon/triggers_test.go | 34 +++++++++ internal/githubissuetrigger/AGENTS.md | 5 +- internal/githubissuetrigger/provider.go | 15 +++- internal/githubissuetrigger/provider_test.go | 72 ++++++++++++++++++++ 8 files changed, 142 insertions(+), 12 deletions(-) diff --git a/docs/reference.md b/docs/reference.md index 4e949cb..3e44822 100644 --- a/docs/reference.md +++ b/docs/reference.md @@ -1904,7 +1904,7 @@ Provider trigger fields: The `bach-github-issue-trigger` provider reads GitHub Issues through the trigger provider protocol. It accepts these `config` keys: - `repo`: required GitHub repository in `owner/name` form. -- `token_env`: optional environment variable name containing a GitHub token; defaults to `GITHUB_TOKEN`. The token value must stay out of the Bachfile. +- `token_env`: optional environment variable name containing a GitHub token; defaults to `GITHUB_TOKEN` when the provider process receives that variable. `bach factory start` only forwards the variable named by `token_env`, so set `token_env = "GITHUB_TOKEN"` when the daemon should use GitHub token auth. The token value must stay out of the Bachfile. - `api_url`: optional GitHub API base URL; defaults to `https://api.github.com`. - `labels`: optional comma-separated GitHub label filter passed to the Issues API. - `state`: optional issue state, one of `open`, `closed`, or `all`; defaults to `open`. @@ -1913,7 +1913,7 @@ The `bach-github-issue-trigger` provider reads GitHub Issues through the trigger - `max_pages`: optional positive page limit per poll; defaults to `5`. - `priority_label_prefix`: optional label prefix for Work Item priority extraction; defaults to `priority:`. -GitHub Issue labels are preserved as Work Item labels for route matching. Pull requests returned by the GitHub Issues API are ignored. The provider advances its cursor from GitHub `updated_at` timestamps and treats `priority:critical`, `priority:urgent`, `priority:high`, `priority:normal`, and `priority:low` labels as Bach priorities. +GitHub Issue labels are preserved as Work Item labels for route matching. Pull requests returned by the GitHub Issues API are ignored. The provider advances its cursor from GitHub `updated_at` timestamps, suppresses issues at or before the stored cursor to avoid duplicate delivery, and treats `priority:critical`, `priority:urgent`, `priority:high`, `priority:normal`, and `priority:low` labels as Bach priorities. Validation rules: @@ -1989,6 +1989,8 @@ resumes, the Work Item fails with a stale-approval message instead of silently i When a Factory declares provider triggers, `bach factory start` also starts a long-running JSON-RPC session with each provider process. The daemon polls each provider on its configured interval, routes returned items to workflows using labels, and enqueues or updates pending Work Items. If any item in a polled batch fails intake validation, the entire batch is nacked so the provider can redeliver; successfully processed batches are acked and the trigger cursor is advanced. Provider trigger protocol messages conform to `docs/schemas/trigger-provider-v1.schema.json`. Provider intake failures do not fail Work Items that are already queued or active. +Provider subprocesses receive only `PATH`, temp directory variables, and the environment variable named by `config.token_env` when present. Configure token env names explicitly instead of relying on the daemon to pass through the full shell environment. + Use `--json` with any Factory command for machine-readable output. `factory submit` returns `{"item": , "created": true|false}`. `factory list` returns `{"items": [, ...]}`. `factory inspect` and `factory cancel` return a single Work Item object. `factory inspect` includes an diff --git a/docs/reference/36-factory-work-items.md b/docs/reference/36-factory-work-items.md index 3c6cda6..f22a3ca 100644 --- a/docs/reference/36-factory-work-items.md +++ b/docs/reference/36-factory-work-items.md @@ -79,7 +79,7 @@ Provider trigger fields: The `bach-github-issue-trigger` provider reads GitHub Issues through the trigger provider protocol. It accepts these `config` keys: - `repo`: required GitHub repository in `owner/name` form. -- `token_env`: optional environment variable name containing a GitHub token; defaults to `GITHUB_TOKEN`. The token value must stay out of the Bachfile. +- `token_env`: optional environment variable name containing a GitHub token; defaults to `GITHUB_TOKEN` when the provider process receives that variable. `bach factory start` only forwards the variable named by `token_env`, so set `token_env = "GITHUB_TOKEN"` when the daemon should use GitHub token auth. The token value must stay out of the Bachfile. - `api_url`: optional GitHub API base URL; defaults to `https://api.github.com`. - `labels`: optional comma-separated GitHub label filter passed to the Issues API. - `state`: optional issue state, one of `open`, `closed`, or `all`; defaults to `open`. @@ -88,7 +88,7 @@ The `bach-github-issue-trigger` provider reads GitHub Issues through the trigger - `max_pages`: optional positive page limit per poll; defaults to `5`. - `priority_label_prefix`: optional label prefix for Work Item priority extraction; defaults to `priority:`. -GitHub Issue labels are preserved as Work Item labels for route matching. Pull requests returned by the GitHub Issues API are ignored. The provider advances its cursor from GitHub `updated_at` timestamps and treats `priority:critical`, `priority:urgent`, `priority:high`, `priority:normal`, and `priority:low` labels as Bach priorities. +GitHub Issue labels are preserved as Work Item labels for route matching. Pull requests returned by the GitHub Issues API are ignored. The provider advances its cursor from GitHub `updated_at` timestamps, suppresses issues at or before the stored cursor to avoid duplicate delivery, and treats `priority:critical`, `priority:urgent`, `priority:high`, `priority:normal`, and `priority:low` labels as Bach priorities. Validation rules: @@ -164,6 +164,8 @@ resumes, the Work Item fails with a stale-approval message instead of silently i When a Factory declares provider triggers, `bach factory start` also starts a long-running JSON-RPC session with each provider process. The daemon polls each provider on its configured interval, routes returned items to workflows using labels, and enqueues or updates pending Work Items. If any item in a polled batch fails intake validation, the entire batch is nacked so the provider can redeliver; successfully processed batches are acked and the trigger cursor is advanced. Provider trigger protocol messages conform to `docs/schemas/trigger-provider-v1.schema.json`. Provider intake failures do not fail Work Items that are already queued or active. +Provider subprocesses receive only `PATH`, temp directory variables, and the environment variable named by `config.token_env` when present. Configure token env names explicitly instead of relying on the daemon to pass through the full shell environment. + Use `--json` with any Factory command for machine-readable output. `factory submit` returns `{"item": , "created": true|false}`. `factory list` returns `{"items": [, ...]}`. `factory inspect` and `factory cancel` return a single Work Item object. `factory inspect` includes an diff --git a/internal/factorydaemon/AGENTS.md b/internal/factorydaemon/AGENTS.md index 114e11c..86e19e3 100644 --- a/internal/factorydaemon/AGENTS.md +++ b/internal/factorydaemon/AGENTS.md @@ -11,6 +11,7 @@ - Use Backend client methods for Factory state; do not query private State Store tables directly. - Run executable work through existing Agent Target, Plan execution, and runner paths instead of creating a parallel execution universe. - Trigger provider polling runs inside `bach factory start`; there is no standalone trigger poll command. +- Trigger provider subprocesses receive only `PATH`, temp directory variables, and the env variable named by `config.token_env` when present. - Provider trigger failures are logged and nacked; they do not fail queued or active Work Items. - Release the daemon lease on shutdown using a fresh timeout context so SIGINT/SIGTERM teardown is not blocked by the canceled signal context. - Expose tunable queue poll, lease renewal, and lease TTL intervals through the CLI adapter; defaults are 5s poll, 10s renew, 30s TTL. diff --git a/internal/factorydaemon/triggers.go b/internal/factorydaemon/triggers.go index aeb6735..4bd216c 100644 --- a/internal/factorydaemon/triggers.go +++ b/internal/factorydaemon/triggers.go @@ -235,7 +235,7 @@ func (p *triggerPoller) ensureSession(ctx context.Context) error { cmdCtx, cancel := context.WithCancel(ctx) cmd := exec.CommandContext(cmdCtx, command[0], command[1:]...) cmd.Dir = p.service.ConfigProject.Root - cmd.Env = triggerEnvironment() + cmd.Env = triggerEnvironment(p.trigger.Config) stdin, err := cmd.StdinPipe() if err != nil { cancel() @@ -334,9 +334,18 @@ func resolveTriggerCommand(command []string) ([]string, error) { return resolved, nil } -func triggerEnvironment() []string { +func triggerEnvironment(config map[string]string) []string { env := []string{} - for _, key := range []string{"PATH", "TMPDIR", "TEMP", "TMP"} { + seen := map[string]struct{}{} + keys := []string{"PATH", "TMPDIR", "TEMP", "TMP"} + if config != nil && config["token_env"] != "" { + keys = append(keys, config["token_env"]) + } + for _, key := range keys { + if _, ok := seen[key]; ok { + continue + } + seen[key] = struct{}{} if value, ok := os.LookupEnv(key); ok { env = append(env, key+"="+value) } diff --git a/internal/factorydaemon/triggers_test.go b/internal/factorydaemon/triggers_test.go index 4ec0f83..be6e39a 100644 --- a/internal/factorydaemon/triggers_test.go +++ b/internal/factorydaemon/triggers_test.go @@ -163,6 +163,31 @@ func TestTriggerPollerNackOnIntakeError(t *testing.T) { } } +func TestTriggerEnvironmentDoesNotIncludeDefaultGitHubToken(t *testing.T) { + t.Setenv("PATH", "/bin") + t.Setenv("GITHUB_TOKEN", "secret-token") + + env := triggerEnvironment(nil) + + if !hasEnvValue(env, "PATH=/bin") { + t.Fatalf("env missing PATH: %v", env) + } + if hasEnvValue(env, "GITHUB_TOKEN=secret-token") { + t.Fatalf("env included implicit GITHUB_TOKEN: %v", env) + } +} + +func TestTriggerEnvironmentIncludesConfiguredTokenEnv(t *testing.T) { + t.Setenv("PATH", "/bin") + t.Setenv("BACH_TEST_GITHUB_TOKEN", "secret-token") + + env := triggerEnvironment(map[string]string{"token_env": "BACH_TEST_GITHUB_TOKEN"}) + + if !hasEnvValue(env, "BACH_TEST_GITHUB_TOKEN=secret-token") { + t.Fatalf("env missing configured token: %v", env) + } +} + type fakeTriggerHandler struct { mu sync.Mutex result triggerprotocol.PollResult @@ -263,3 +288,12 @@ func newTestPoller( } return poller, handler, cleanup } + +func hasEnvValue(env []string, want string) bool { + for _, value := range env { + if value == want { + return true + } + } + return false +} diff --git a/internal/githubissuetrigger/AGENTS.md b/internal/githubissuetrigger/AGENTS.md index bb06c0e..40b26cb 100644 --- a/internal/githubissuetrigger/AGENTS.md +++ b/internal/githubissuetrigger/AGENTS.md @@ -11,8 +11,9 @@ - Keep GitHub API access read-only; do not mutate issues, labels, comments, or repository state. - Read GitHub tokens from environment variables named by provider config. Never accept token values in Bachfile config and never log token values. -- Keep Work Item mapping deterministic: stable source IDs, issue labels preserved for Factory routing, and - cursor advancement based on GitHub `updated_at` values. +- Keep Work Item mapping deterministic: stable source IDs, issue labels preserved for Factory routing, cursor + advancement based on GitHub `updated_at` values, and duplicate suppression for issues at or before the + stored cursor. ## Verification diff --git a/internal/githubissuetrigger/provider.go b/internal/githubissuetrigger/provider.go index b975483..b060853 100644 --- a/internal/githubissuetrigger/provider.go +++ b/internal/githubissuetrigger/provider.go @@ -164,11 +164,15 @@ func (p *Provider) fetchIssues( return nil, "", err } for _, issue := range batch { - issues = append(issues, issue) if issue.UpdatedAt.IsZero() { + issues = append(issues, issue) continue } updated := issue.UpdatedAt.UTC().Format(time.RFC3339Nano) + if cursor != "" && compareTimes(updated, cursor) <= 0 { + continue + } + issues = append(issues, issue) if nextCursor == "" || compareTimes(updated, nextCursor) > 0 { nextCursor = updated } @@ -197,7 +201,8 @@ func (p *Provider) fetchIssuePage( req.Header.Set("Accept", "application/vnd.github+json") req.Header.Set("User-Agent", ProviderName) req.Header.Set("X-GitHub-Api-Version", "2022-11-28") - if token := strings.TrimSpace(os.Getenv(cfg.TokenEnv)); token != "" { + token := strings.TrimSpace(os.Getenv(cfg.TokenEnv)) + if token != "" { req.Header.Set("Authorization", "Bearer "+token) } resp, err := p.client.Do(req) @@ -209,10 +214,14 @@ func (p *Provider) fetchIssuePage( }() if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices { body, _ := io.ReadAll(io.LimitReader(resp.Body, maxErrorBodyBytes)) + message := strings.TrimSpace(string(body)) + if token != "" { + message = strings.ReplaceAll(message, token, "[REDACTED]") + } return nil, fmt.Errorf( "github issues request failed with %s: %s", resp.Status, - strings.TrimSpace(string(body)), + message, ) } var issues []githubIssue diff --git a/internal/githubissuetrigger/provider_test.go b/internal/githubissuetrigger/provider_test.go index eaeadbf..420fb78 100644 --- a/internal/githubissuetrigger/provider_test.go +++ b/internal/githubissuetrigger/provider_test.go @@ -128,6 +128,49 @@ func TestProviderHandshakeValidatesConfig(t *testing.T) { } } +func TestProviderPollSkipsIssuesAtCursor(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _ = json.NewEncoder(w).Encode([]map[string]any{ + { + "number": 42, + "html_url": "https://github.com/acme/widgets/issues/42", + "title": "Already delivered", + "state": "open", + "labels": []map[string]string{}, + "user": map[string]string{"login": "octo"}, + "created_at": "2026-06-01T10:00:00Z", + "updated_at": "2026-06-02T10:00:00Z", + }, + }) + })) + defer server.Close() + + provider := New(server.Client()) + _, err := provider.Handshake(context.Background(), triggerprotocol.HandshakeParams{ + Protocol: triggerprotocol.ProtocolVersion, + Config: map[string]string{ + "repo": "acme/widgets", + "api_url": server.URL, + }, + }) + if err != nil { + t.Fatalf("handshake error = %v", err) + } + + result, err := provider.Poll(context.Background(), triggerprotocol.PollParams{ + Cursor: "2026-06-02T10:00:00Z", + }) + if err != nil { + t.Fatalf("poll error = %v", err) + } + if result.Cursor != "2026-06-02T10:00:00Z" { + t.Fatalf("cursor = %q", result.Cursor) + } + if len(result.Items) != 0 { + t.Fatalf("items len = %d", len(result.Items)) + } +} + func TestProviderPollReportsGitHubStatus(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { http.Error(w, "bad credentials", http.StatusUnauthorized) @@ -153,3 +196,32 @@ func TestProviderPollReportsGitHubStatus(t *testing.T) { t.Fatalf("poll error = %q", got) } } + +func TestProviderPollRedactsTokenFromGitHubStatus(t *testing.T) { + t.Setenv("BACH_TEST_GITHUB_TOKEN", "secret-token") + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + http.Error(w, "reflected secret-token", http.StatusUnauthorized) + })) + defer server.Close() + + provider := New(server.Client()) + _, err := provider.Handshake(context.Background(), triggerprotocol.HandshakeParams{ + Protocol: triggerprotocol.ProtocolVersion, + Config: map[string]string{ + "repo": "acme/widgets", + "api_url": server.URL, + "token_env": "BACH_TEST_GITHUB_TOKEN", + }, + }) + if err != nil { + t.Fatalf("handshake error = %v", err) + } + _, err = provider.Poll(context.Background(), triggerprotocol.PollParams{}) + if err == nil { + t.Fatal("poll error is nil") + } + if got := err.Error(); got != "internal: github issues request failed with 401 Unauthorized: reflected [REDACTED]" { + t.Fatalf("poll error = %q", got) + } +}