diff --git a/.claude/settings.json b/.claude/settings.json index 4b9fabd..b72b40b 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -40,7 +40,8 @@ "mcp__hermit__run_requirements_sweep", "mcp__hermit__now", "mcp__hermit__get_loop_state", - "mcp__hermit__update_loop_state" + "mcp__hermit__update_loop_state", + "mcp__hermit__run_health_checks" ] } } diff --git a/CLAUDE.md b/CLAUDE.md index 913063d..8c34fdf 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -43,15 +43,20 @@ Because this cycle now runs with the full tool access of the invoking context (n 7. Run the requirements reconcile sweep roughly once an hour using `run_requirements_sweep`, tracking a separate "last requirements-sweep time" across passes the same way step 5 tracks its own PR-comment-check "since" timestamp (store the current time before calling): - Only call `run_requirements_sweep` when at least 3600 seconds have elapsed since the last recorded sweep time; otherwise skip this step for the current pass (do not call the tool early — it runs the configured `test_command` for every requirement and shouldn't be wasted on sub-hourly passes) - Update the stored last-sweep timestamp to now after calling -8. If there are no Issues, end this pass — the recurring cron trigger starts the next pass -9. For each Issue (up to 4 at a time): - a. Mark as in-progress with `assign_issue` (assignee: your own username) - b. Create a worktree with `create_worktree` (base_branch: default branch) -10. **Spawn all Engineers for the Issues prepared in step 9 in parallel at once using the Agent tool** (`run_in_background: true`, so this inline pass can wait for them without blocking on each one sequentially) +8. Run the production health-check sweep roughly every 5 minutes using `run_health_checks` (Issue #190), tracking a separate "last health-check time" across passes (`health_checks_since`) the same way step 7 tracks its own requirements-sweep timestamp: + - Only call `run_health_checks` when at least 300 seconds have elapsed since the last recorded health-check time; otherwise skip this step for the current pass + - `run_health_checks` itself runs each configured `[[health_checks]]` command, opens a deduped `production-incident`-labeled Issue for any newly-failing check, and posts a one-time "recovered at ..." comment on the corresponding Issue for any check that has returned to passing (never auto-closing it) — no further action is needed here beyond calling the tool and updating the timestamp + - Update the stored last-health-check timestamp to now after calling + - If no `[[health_checks]]` are configured for this project, the tool is a no-op; keep calling it on cadence anyway rather than special-casing it away, so a project that later adds health checks starts getting swept without a CLAUDE.md change +9. If there are no Issues, end this pass — the recurring cron trigger starts the next pass +10. For each Issue (up to 4 at a time): + a. Mark as in-progress with `assign_issue` (assignee: your own username) + b. Create a worktree with `create_worktree` (base_branch: default branch) +11. **Spawn all Engineers for the Issues prepared in step 10 in parallel at once using the Agent tool** (`run_in_background: true`, so this inline pass can wait for them without blocking on each one sequentially) - Information to pass to each Engineer: Issue number, title, body, `worktree_path` and `branch` returned by `create_worktree` - If the parallel count exceeds 4, process the first 4 and defer the rest to the next pass -11. Wait for all Engineers to complete -12. Run `check_ci_status` on the PR for each Issue (including PRs from Engineers spawned on an earlier pass that are still awaiting evaluation — use `list_prs` to find open HERMIT PRs) +12. Wait for all Engineers to complete +13. Run `check_ci_status` on the PR for each Issue (including PRs from Engineers spawned on an earlier pass that are still awaiting evaluation — use `list_prs` to find open HERMIT PRs) - If CI is failing: the tool automatically posts an investigation comment listing the failing checks; skip merging and wait for fixes - If CI is passing: run `evaluate_risk` - LOW / MEDIUM: run `merge_pr` with `worktree_path` and `branch` so the worktree is cleaned up automatically after a successful merge @@ -61,7 +66,7 @@ Because this cycle now runs with the full tool access of the invoking context (n - Check whether the branch is stale relative to the base branch in a way that could hide semantic conflicts, not just textual `mergeable` conflicts - Post your findings as a separate PR comment via `add_issue_comment`: a short summary of what changed, anything concerning, and an explicit recommendation (e.g. "looks safe to merge pending approval" vs. "found X, should be fixed first") - Skip merging and wait for a human decision -13. End the pass with a short report of what was done, and return control to the prompt — do **not** loop back to step 1 yourself; the recurring cron job fires the next pass +14. End the pass with a short report of what was done, and return control to the prompt — do **not** loop back to step 1 yourself; the recurring cron job fires the next pass --- diff --git a/cmd/hermit/main.go b/cmd/hermit/main.go index e84d3af..9154ee3 100644 --- a/cmd/hermit/main.go +++ b/cmd/hermit/main.go @@ -20,6 +20,7 @@ import ( "github.com/ytnobody/hermit/internal/git" gh "github.com/ytnobody/hermit/internal/github" + "github.com/ytnobody/hermit/internal/healthcheck" "github.com/ytnobody/hermit/internal/mcp" "github.com/ytnobody/hermit/internal/notification" "github.com/ytnobody/hermit/internal/permissions" @@ -79,6 +80,17 @@ func (r RiskConfig) toRiskConfig() risk.Config { } } +// HealthCheckEntry is a single [[health_checks]] entry: a named production +// health check HERMIT runs periodically via the run_health_checks MCP tool +// (Issue #190). Only Type "command" (the default when empty) is supported +// today; entries with any other Type are skipped by healthcheck.RunChecks +// as a forward-compatible no-op. +type HealthCheckEntry struct { + Name string `toml:"name"` + Command string `toml:"command"` + Type string `toml:"type"` +} + type Config struct { GitHub struct { Owner string `toml:"owner"` @@ -164,7 +176,14 @@ type Config struct { // needing to opt in. Paths []string `toml:"paths"` } `toml:"requirements"` - Security struct { + // HealthChecks is the [[health_checks]] array of tables (Issue #190): + // production health-check commands run periodically by the + // run_health_checks MCP tool. An empty/omitted list means the project + // has not opted into production health checks, in which case + // run_health_checks is a no-op — existing projects see no change in + // behavior. + HealthChecks []HealthCheckEntry `toml:"health_checks"` + Security struct { // TrustedAuthorAssociations is the allowlist of GitHub // "author_association" values (e.g. "OWNER", "MEMBER", // "COLLABORATOR", "CONTRIBUTOR", "FIRST_TIME_CONTRIBUTOR", "NONE") @@ -530,6 +549,8 @@ func cmdServe() { TestCommand: cfg.Requirements.TestCommand, } + healthChecks := resolveHealthChecks(cfg) + model := mcp.ModelConfig{ Superintendent: cfg.Model.Superintendent, Engineer: cfg.Model.Engineer, @@ -539,7 +560,7 @@ func cmdServe() { AnalystEffort: resolveAnalystEffort(cfg), } - if err := mcp.Serve(client, cfg.GitHub.RateLimitThreshold, rootDir, prefix, cfg.Agent.LoopInterval, cfg.Notification.WebhookURL, cfg.Notification.Type, repos, cfg.Agent.TriggerComment, readinessCfg, defaultRiskCfg, repoRiskCfgs, model, requirementsCfg, cfg.Agent.MaxEngineers); err != nil { + if err := mcp.Serve(client, cfg.GitHub.RateLimitThreshold, rootDir, prefix, cfg.Agent.LoopInterval, cfg.Notification.WebhookURL, cfg.Notification.Type, repos, cfg.Agent.TriggerComment, readinessCfg, defaultRiskCfg, repoRiskCfgs, model, requirementsCfg, cfg.Agent.MaxEngineers, healthChecks); err != nil { fatal(err.Error()) } } @@ -705,6 +726,23 @@ func resolveRequirementsDoc(cfg Config) string { return defaultRequirementsDoc } +// resolveHealthChecks converts cfg's [[health_checks]] entries (TOML shape) +// into []healthcheck.Check for the run_health_checks MCP tool (Issue #190). +// Entries missing a name or command are skipped rather than passed through +// as a broken check, since an empty Command would otherwise run `sh -c ""` +// (a vacuous success) on every sweep. +func resolveHealthChecks(cfg Config) []healthcheck.Check { + var checks []healthcheck.Check + for _, e := range cfg.HealthChecks { + if e.Name == "" || e.Command == "" { + log.Printf("health_checks: skipping entry with missing name or command: %+v", e) + continue + } + checks = append(checks, healthcheck.Check{Name: e.Name, Command: e.Command, Type: e.Type}) + } + return checks +} + // runRequirementsSweep runs the requirements reconcile sweep (Issue #106) at // `hermit serve` startup: it parses the requirements document for "## REQ-xxx:" // blocks, runs each requirement's test via the configured test_command, and diff --git a/cmd/hermit/serve_test.go b/cmd/hermit/serve_test.go index 6e67c26..c9df513 100644 --- a/cmd/hermit/serve_test.go +++ b/cmd/hermit/serve_test.go @@ -103,8 +103,8 @@ language = "ja" continue } tools, _ := msg.Result["tools"].([]any) - if len(tools) != 20 { - t.Errorf("expected 20 tools, got %d", len(tools)) + if len(tools) != 21 { + t.Errorf("expected 21 tools, got %d", len(tools)) } return case <-deadline: @@ -207,8 +207,8 @@ language = "ja" continue } tools, _ := msg.Result["tools"].([]any) - if len(tools) != 20 { - t.Errorf("expected 20 tools, got %d", len(tools)) + if len(tools) != 21 { + t.Errorf("expected 21 tools, got %d", len(tools)) } return case <-deadline: @@ -314,8 +314,8 @@ language = "ja" continue } tools, _ := msg.Result["tools"].([]any) - if len(tools) != 20 { - t.Errorf("expected 20 tools, got %d", len(tools)) + if len(tools) != 21 { + t.Errorf("expected 21 tools, got %d", len(tools)) } return case <-deadline: diff --git a/cmd/hermit/templates/CLAUDE.md.tmpl b/cmd/hermit/templates/CLAUDE.md.tmpl index 2431c77..ec60fb5 100644 --- a/cmd/hermit/templates/CLAUDE.md.tmpl +++ b/cmd/hermit/templates/CLAUDE.md.tmpl @@ -24,19 +24,24 @@ - Only call `run_requirements_sweep` when at least 3600 seconds have elapsed since the last recorded sweep time; otherwise skip this step for the current pass - Update the stored last-sweep timestamp to now after calling - Note this is distinct from step 5: step 5 is about a human dictating new/changed requirements via conversation; this step is about detecting when already-declared requirements have silently regressed or gone unimplemented in the code, by re-running the reconcile sweep against the current requirements document -7. If there are no non-requirements Issues, end this pass — the recurring cron trigger starts the next pass -8. For each remaining Issue (up to {{ .MaxEngineers }} at a time), perform a granularity check: +7. Run the production health-check sweep roughly every 5 minutes using `run_health_checks` (Issue #190), tracking a "last health-check time" across passes (`health_checks_since`) the same way step 6 tracks its own requirements-sweep timestamp: + - Only call `run_health_checks` when at least 300 seconds have elapsed since the last recorded health-check time; otherwise skip this step for the current pass + - `run_health_checks` itself runs each configured `[[health_checks]]` command, opens a deduped `production-incident`-labeled Issue for any newly-failing check, and posts a one-time "recovered at ..." comment on the corresponding Issue for any check that has returned to passing (never auto-closing it) — no further action is needed here beyond calling the tool and updating the timestamp + - Update the stored last-health-check timestamp to now after calling + - If no `[[health_checks]]` are configured for this project, the tool is a no-op; keep calling it on cadence anyway rather than special-casing it away +8. If there are no non-requirements Issues, end this pass — the recurring cron trigger starts the next pass +9. For each remaining Issue (up to {{ .MaxEngineers }} at a time), perform a granularity check: - If the Issue body is empty, extremely short, or covers multiple independent features, judge it as **ambiguous or too large** - Ambiguous/oversized Issues: post a `[Clarification Needed]` comment via `add_issue_comment` and skip - For Issues of appropriate scope only: a. Mark as in-progress with `assign_issue` (assignee: your own username) b. Create a worktree with `create_worktree` (base_branch: default branch) -9. **Spawn all Engineers for the Issues prepared in step 8 in parallel at once using the Agent tool** (`run_in_background: true`, so this inline pass can wait for them without blocking on each one sequentially) - - Information to pass to each Engineer: Issue number, title, body, `worktree_path` and `branch` returned by `create_worktree` - - Call the Agent tool with `model: "{{ .EngineerModel }}"`{{ if .EngineerEffort }} and `effort: "{{ .EngineerEffort }}"`{{ end }} so each Engineer runs with the model{{ if .EngineerEffort }}/effort{{ end }} configured in `harness.toml`'s `[model]` section - - If the parallel count exceeds {{ .MaxEngineers }}, process the first {{ .MaxEngineers }} and defer the rest to the next pass -10. Wait for all Engineers to complete -11. Run `evaluate_risk` on the PR for each Issue (including PRs opened by Engineers spawned on an earlier pass that are still awaiting evaluation — use `list_prs` to find open HERMIT PRs) +10. **Spawn all Engineers for the Issues prepared in step 9 in parallel at once using the Agent tool** (`run_in_background: true`, so this inline pass can wait for them without blocking on each one sequentially) + - Information to pass to each Engineer: Issue number, title, body, `worktree_path` and `branch` returned by `create_worktree` + - Call the Agent tool with `model: "{{ .EngineerModel }}"`{{ if .EngineerEffort }} and `effort: "{{ .EngineerEffort }}"`{{ end }} so each Engineer runs with the model{{ if .EngineerEffort }}/effort{{ end }} configured in `harness.toml`'s `[model]` section + - If the parallel count exceeds {{ .MaxEngineers }}, process the first {{ .MaxEngineers }} and defer the rest to the next pass +11. Wait for all Engineers to complete +12. Run `evaluate_risk` on the PR for each Issue (including PRs opened by Engineers spawned on an earlier pass that are still awaiting evaluation — use `list_prs` to find open HERMIT PRs) - LOW / MEDIUM: run `merge_pr` with `worktree_path`/`branch` so the worktree is cleaned up automatically - HIGH: `evaluate_risk` auto-posts a generic risk comment (`⚠️ HERMIT: HIGH risk detected.\nReasons: [...]`) restating the `risk_reasons`. That comment is not a review — before skipping, perform a substantive review of the PR yourself: - Read the actual diff (not just the file paths / line counts in `risk_reasons`) @@ -44,7 +49,7 @@ - Check whether the branch is stale relative to the base branch in a way that could hide semantic conflicts, not just textual `mergeable` conflicts - Post your findings as a separate PR comment via `add_issue_comment`: a short summary of what changed, anything concerning, and an explicit recommendation (e.g. "looks safe to merge pending approval" vs. "found X, should be fixed first") - Skip merging and wait for a human decision -12. End the pass with a short report of what was done, and return control to the prompt — do **not** loop back to step 1 yourself; the recurring cron job fires the next pass +13. End the pass with a short report of what was done, and return control to the prompt — do **not** loop back to step 1 yourself; the recurring cron job fires the next pass --- diff --git a/cmd/hermit/templates/harness.toml.tmpl b/cmd/hermit/templates/harness.toml.tmpl index 9b3b5e3..3dc59de 100644 --- a/cmd/hermit/templates/harness.toml.tmpl +++ b/cmd/hermit/templates/harness.toml.tmpl @@ -79,3 +79,18 @@ analyst_effort = "{{ .AnalystEffort }}" # reasoning effort: low/medium/ # [run] # failure_notify_threshold = 3 # `hermit run`: consecutive failed passes before a [notification] webhook fires (default: 3) + +# [[health_checks]] +# # Production health checks run periodically by the run_health_checks MCP +# # tool (Issue #190). Each entry's command is run with a 30s timeout; exit +# # code 0 means healthy. A failing check with no existing open issue opens +# # one labeled "production-incident" (deduped by an "[health-check: ]" +# # title prefix); a check that recovers gets a one-time "recovered at ..." +# # comment on that issue (it is never auto-closed). Omitting this section +# # entirely (the default) means run_health_checks is a no-op. +# name = "api-health" +# command = "curl -sf https://example.com/healthz" +# # type: only "command" is supported today (default when omitted); other +# # values are reserved for future check mechanisms (e.g. "http") and are +# # skipped if set to anything else. +# type = "command" diff --git a/cmd/hermit/unit_test.go b/cmd/hermit/unit_test.go index 54e3ef0..555b729 100644 --- a/cmd/hermit/unit_test.go +++ b/cmd/hermit/unit_test.go @@ -1165,3 +1165,84 @@ func TestResolveAnalystEffort_FallsBackToSuperintendentEffort(t *testing.T) { t.Errorf("resolveAnalystEffort() = %q, want configured %q", got, "medium") } } + +// --- health_checks (Issue #190) --- + +// TestLoadConfig_HealthChecks verifies that [[health_checks]] entries in +// harness.toml are parsed into Config.HealthChecks. +func TestLoadConfig_HealthChecks(t *testing.T) { + dir := t.TempDir() + content := `[github] +owner = "owner" +repo = "repo" + +[[health_checks]] +name = "api-health" +command = "curl -sf https://example.com/healthz" + +[[health_checks]] +name = "db-health" +command = "pg_isready" +type = "command" +` + if err := os.WriteFile(filepath.Join(dir, "harness.toml"), []byte(content), 0o644); err != nil { + t.Fatal(err) + } + prev, _ := os.Getwd() + os.Chdir(dir) + defer os.Chdir(prev) + + cfg := loadConfig() + if len(cfg.HealthChecks) != 2 { + t.Fatalf("expected 2 health_checks entries, got %d: %+v", len(cfg.HealthChecks), cfg.HealthChecks) + } + if cfg.HealthChecks[0].Name != "api-health" || cfg.HealthChecks[0].Command != "curl -sf https://example.com/healthz" { + t.Errorf("unexpected first health check: %+v", cfg.HealthChecks[0]) + } + if cfg.HealthChecks[1].Name != "db-health" || cfg.HealthChecks[1].Type != "command" { + t.Errorf("unexpected second health check: %+v", cfg.HealthChecks[1]) + } +} + +// TestLoadConfig_HealthChecks_Omitted verifies that a project with no +// [[health_checks]] section at all parses to a nil/empty slice — the +// "unconfigured is a no-op" contract from Issue #190. +func TestLoadConfig_HealthChecks_Omitted(t *testing.T) { + dir := t.TempDir() + content := `[github] +owner = "owner" +repo = "repo" +` + if err := os.WriteFile(filepath.Join(dir, "harness.toml"), []byte(content), 0o644); err != nil { + t.Fatal(err) + } + prev, _ := os.Getwd() + os.Chdir(dir) + defer os.Chdir(prev) + + cfg := loadConfig() + if len(cfg.HealthChecks) != 0 { + t.Errorf("expected no health_checks entries, got %+v", cfg.HealthChecks) + } + if got := resolveHealthChecks(cfg); len(got) != 0 { + t.Errorf("resolveHealthChecks() = %v, want empty", got) + } +} + +// TestResolveHealthChecks_SkipsIncompleteEntries verifies that entries +// missing a name or command are skipped rather than passed through as a +// broken check (which would otherwise silently run `sh -c ""`, a vacuous +// success, on every sweep). +func TestResolveHealthChecks_SkipsIncompleteEntries(t *testing.T) { + cfg := Config{ + HealthChecks: []HealthCheckEntry{ + {Name: "good-check", Command: "exit 0"}, + {Name: "", Command: "exit 0"}, + {Name: "no-command", Command: ""}, + }, + } + got := resolveHealthChecks(cfg) + if len(got) != 1 || got[0].Name != "good-check" { + t.Errorf("resolveHealthChecks() = %+v, want only good-check", got) + } +} diff --git a/internal/healthcheck/github_adapter.go b/internal/healthcheck/github_adapter.go new file mode 100644 index 0000000..2458522 --- /dev/null +++ b/internal/healthcheck/github_adapter.go @@ -0,0 +1,75 @@ +package healthcheck + +import ( + "fmt" + "strings" + "time" + + gh "github.com/ytnobody/hermit/internal/github" +) + +// ghClient is the subset of *github.Client the GitHub-backed IssueClient +// adapter needs. It is defined locally (rather than depending on the +// concrete *github.Client) so it can be satisfied by a lightweight fake in +// tests, mirroring internal/requirements/github_adapter.go's ghClient. +type ghClient interface { + ListOpenIssues(label string) ([]gh.Issue, error) + CreateIssue(title, body string) (int, error) + AddLabel(number int, label string) error + HasCommentMatching(number int, trigger string) (bool, error) + PostComment(number int, body string) error +} + +// GitHubIssueClient adapts a GitHub client to the IssueClient interface used +// by Reconcile, deduping against open issues by matching the +// "[health-check: ]" title prefix (see TitlePrefix). +type GitHubIssueClient struct { + client ghClient +} + +// NewGitHubIssueClient wraps client for use as a Reconcile IssueClient. +func NewGitHubIssueClient(client ghClient) *GitHubIssueClient { + return &GitHubIssueClient{client: client} +} + +func (g *GitHubIssueClient) FindOpenIssue(name string) (int, bool, error) { + issues, err := g.client.ListOpenIssues("") + if err != nil { + return 0, false, err + } + prefix := TitlePrefix(name) + for _, issue := range issues { + if strings.HasPrefix(issue.Title, prefix) { + return issue.Number, true, nil + } + } + return 0, false, nil +} + +func (g *GitHubIssueClient) CreateIncidentIssue(name, command, output string, detectedAt time.Time) (int, error) { + title := fmt.Sprintf("%s health check failing", TitlePrefix(name)) + body := fmt.Sprintf( + "## Production health check failure\n\n"+ + "- **Check name**: %s\n"+ + "- **Command**: `%s`\n"+ + "- **Detected at**: %s\n\n"+ + "### Output\n\n```\n%s\n```\n", + name, command, detectedAt.UTC().Format(time.RFC3339), output, + ) + num, err := g.client.CreateIssue(title, body) + if err != nil { + return 0, err + } + if err := g.client.AddLabel(num, IncidentLabel); err != nil { + return num, err + } + return num, nil +} + +func (g *GitHubIssueClient) HasRecoveredComment(number int) (bool, error) { + return g.client.HasCommentMatching(number, RecoveredTrigger) +} + +func (g *GitHubIssueClient) PostRecoveredComment(number int, recoveredAt time.Time) error { + return g.client.PostComment(number, fmt.Sprintf("%s %s", RecoveredTrigger, recoveredAt.UTC().Format(time.RFC3339))) +} diff --git a/internal/healthcheck/github_adapter_test.go b/internal/healthcheck/github_adapter_test.go new file mode 100644 index 0000000..c7d558f --- /dev/null +++ b/internal/healthcheck/github_adapter_test.go @@ -0,0 +1,178 @@ +package healthcheck + +import ( + "strings" + "testing" + "time" + + gh "github.com/ytnobody/hermit/internal/github" +) + +type fakeGHClient struct { + openIssues []gh.Issue + comments map[int][]string + createdN int + + listErr error + createErr error + addLabelErr error + hasCommentErr error + postCommentErr error + + addedLabels []string + posted []string +} + +func newFakeGHClient() *fakeGHClient { + return &fakeGHClient{comments: map[int][]string{}} +} + +func (f *fakeGHClient) ListOpenIssues(label string) ([]gh.Issue, error) { + if f.listErr != nil { + return nil, f.listErr + } + return f.openIssues, nil +} + +func (f *fakeGHClient) CreateIssue(title, body string) (int, error) { + if f.createErr != nil { + return 0, f.createErr + } + f.createdN++ + f.openIssues = append(f.openIssues, gh.Issue{Number: f.createdN, Title: title, Body: body}) + return f.createdN, nil +} + +func (f *fakeGHClient) AddLabel(number int, label string) error { + if f.addLabelErr != nil { + return f.addLabelErr + } + f.addedLabels = append(f.addedLabels, label) + return nil +} + +func (f *fakeGHClient) HasCommentMatching(number int, trigger string) (bool, error) { + if f.hasCommentErr != nil { + return false, f.hasCommentErr + } + for _, c := range f.comments[number] { + if strings.Contains(strings.ToLower(c), strings.ToLower(trigger)) { + return true, nil + } + } + return false, nil +} + +func (f *fakeGHClient) PostComment(number int, body string) error { + if f.postCommentErr != nil { + return f.postCommentErr + } + f.comments[number] = append(f.comments[number], body) + f.posted = append(f.posted, body) + return nil +} + +func TestGitHubIssueClient_FindOpenIssue_MatchesTitlePrefix(t *testing.T) { + fake := newFakeGHClient() + fake.openIssues = []gh.Issue{ + {Number: 1, Title: "unrelated issue"}, + {Number: 2, Title: TitlePrefix("api-health") + " health check failing"}, + } + client := NewGitHubIssueClient(fake) + + num, found, err := client.FindOpenIssue("api-health") + if err != nil { + t.Fatalf("FindOpenIssue() error = %v", err) + } + if !found || num != 2 { + t.Errorf("FindOpenIssue() = (%d, %v), want (2, true)", num, found) + } + + // A different check name must not match. + _, found, err = client.FindOpenIssue("other-check") + if err != nil { + t.Fatalf("FindOpenIssue() error = %v", err) + } + if found { + t.Errorf("should not match an issue for a different check name") + } +} + +func TestGitHubIssueClient_CreateIncidentIssue_EmbedsDetailsAndLabels(t *testing.T) { + fake := newFakeGHClient() + client := NewGitHubIssueClient(fake) + now := time.Date(2026, 7, 28, 12, 0, 0, 0, time.UTC) + + num, err := client.CreateIncidentIssue("api-health", "curl -sf https://example.com/healthz", "connection refused", now) + if err != nil { + t.Fatalf("CreateIncidentIssue() error = %v", err) + } + if len(fake.openIssues) != 1 { + t.Fatalf("expected 1 issue to be created, got %d", len(fake.openIssues)) + } + issue := fake.openIssues[0] + if issue.Number != num { + t.Errorf("issue.Number = %d, want %d", issue.Number, num) + } + if !strings.HasPrefix(issue.Title, TitlePrefix("api-health")) { + t.Errorf("issue.Title = %q, want it to start with %q", issue.Title, TitlePrefix("api-health")) + } + for _, want := range []string{"api-health", "curl -sf https://example.com/healthz", "connection refused", "2026-07-28T12:00:00Z"} { + if !strings.Contains(issue.Body, want) { + t.Errorf("issue.Body = %q, want it to contain %q", issue.Body, want) + } + } + if len(fake.addedLabels) != 1 || fake.addedLabels[0] != IncidentLabel { + t.Errorf("addedLabels = %v, want [%q]", fake.addedLabels, IncidentLabel) + } + + // The just-created issue must now be discoverable by FindOpenIssue too. + found, ok, err := client.FindOpenIssue("api-health") + if err != nil || !ok || found != num { + t.Errorf("FindOpenIssue() = (%d, %v, %v), want (%d, true, nil)", found, ok, err, num) + } +} + +func TestGitHubIssueClient_RecoveredComment_OnceOnly(t *testing.T) { + fake := newFakeGHClient() + client := NewGitHubIssueClient(fake) + now := time.Date(2026, 7, 28, 13, 30, 0, 0, time.UTC) + + has, err := client.HasRecoveredComment(1) + if err != nil { + t.Fatalf("HasRecoveredComment() error = %v", err) + } + if has { + t.Errorf("expected no recovered comment yet") + } + + if err := client.PostRecoveredComment(1, now); err != nil { + t.Fatalf("PostRecoveredComment() error = %v", err) + } + if len(fake.posted) != 1 || !strings.Contains(fake.posted[0], "2026-07-28T13:30:00Z") { + t.Errorf("posted = %v, want a comment mentioning the timestamp", fake.posted) + } + + has, err = client.HasRecoveredComment(1) + if err != nil { + t.Fatalf("HasRecoveredComment() error = %v", err) + } + if !has { + t.Errorf("expected HasRecoveredComment to report true after posting") + } +} + +func TestGitHubIssueClient_FindOpenIssue_PropagatesListError(t *testing.T) { + fake := newFakeGHClient() + fake.listErr = errFake + client := NewGitHubIssueClient(fake) + if _, _, err := client.FindOpenIssue("api-health"); err == nil { + t.Fatalf("expected error to propagate from ListOpenIssues") + } +} + +var errFake = &fakeErr{"boom"} + +type fakeErr struct{ msg string } + +func (e *fakeErr) Error() string { return e.msg } diff --git a/internal/healthcheck/healthcheck.go b/internal/healthcheck/healthcheck.go new file mode 100644 index 0000000..c26755c --- /dev/null +++ b/internal/healthcheck/healthcheck.go @@ -0,0 +1,108 @@ +// Package healthcheck implements the production health-check sweep +// described in HERMIT Issue #190: a project can declare a list of +// [[health_checks]] commands in harness.toml, and the Superintendent loop +// periodically runs them, opening (deduped) GitHub issues for newly-failing +// checks and posting a one-time "recovered" comment when a previously +// failing check starts passing again. +// +// This intentionally mirrors internal/requirements' config-loading, +// command-execution/timeout, and MCP-tool-registration patterns (see +// internal/requirements/runner.go and internal/requirements/reconcile.go) so +// the two sweeps stay consistent for anyone reading both. +package healthcheck + +import ( + "context" + "fmt" + "os/exec" + "time" +) + +// Check is a single configured health check. +type Check struct { + // Name identifies the check (used in the issue title-prefix dedup + // convention "[health-check: ]" — see TitlePrefix). + Name string + // Command is the shell command executed to determine health. Exit code + // 0 is treated as healthy (ok:true); any other exit code, or a command + // that fails to start, is treated as unhealthy (ok:false). + Command string + // Type selects the check mechanism. Only "command" (the zero value + // also defaults to "command") is supported today; any other value is + // skipped by RunChecks as a forward-compatible no-op (e.g. a future + // "http" type declared in harness.toml by a newer HERMIT version). + Type string +} + +// Result is the outcome of running a single Check. +type Result struct { + Name string `json:"name"` + Ok bool `json:"ok"` + Output string `json:"output"` +} + +// Timeout is the maximum duration a single health-check command may run +// before being treated as a failure. It is a var (not a const) so tests can +// shrink it to keep the timeout path fast to exercise; production code +// should treat it as a constant and leave it at its default value. +var Timeout = 30 * time.Second + +// WaitDelay bounds how long a timed-out check's CombinedOutput call may +// additionally block waiting for output pipes to close (see runOne). Also a +// var so tests can shrink it. +var WaitDelay = 2 * time.Second + +// TypeCommand is the only currently-supported Check.Type value. +const TypeCommand = "command" + +// RunChecks executes each configured check's command and returns one Result +// per supported check, in the same order as checks. Checks whose Type is +// set to anything other than "" or "command" are silently skipped (not +// included in the returned slice) — see Check.Type. +func RunChecks(checks []Check) []Result { + results := make([]Result, 0, len(checks)) + for _, c := range checks { + if c.Type != "" && c.Type != TypeCommand { + continue + } + results = append(results, runOne(c)) + } + return results +} + +// runOne runs a single check's command with a Timeout deadline, via +// `sh -c` (matching requirements.CommandRunner's convention). +func runOne(c Check) Result { + ctx, cancel := context.WithTimeout(context.Background(), Timeout) + defer cancel() + + cmd := exec.CommandContext(ctx, "sh", "-c", c.Command) + // WaitDelay bounds how long CombinedOutput waits for the output pipes + // to close after the process is killed. Without it, a command whose + // shell forks a grandchild that inherits stdout/stderr (rather than + // exec-replacing itself, which depends on the shell) can leave + // CombinedOutput blocked for the grandchild's full runtime even though + // the immediate child was killed on the Timeout deadline — silently + // defeating the timeout. See https://pkg.go.dev/os/exec#Cmd.WaitDelay. + cmd.WaitDelay = WaitDelay + out, err := cmd.CombinedOutput() + output := string(out) + + if ctx.Err() == context.DeadlineExceeded { + return Result{ + Name: c.Name, + Ok: false, + Output: appendNote(output, fmt.Sprintf("(timed out after %s)", Timeout)), + } + } + return Result{Name: c.Name, Ok: err == nil, Output: output} +} + +// appendNote appends note to output on its own trailing line, avoiding a +// leading blank line when output is empty. +func appendNote(output, note string) string { + if output == "" { + return note + } + return output + "\n" + note +} diff --git a/internal/healthcheck/healthcheck_test.go b/internal/healthcheck/healthcheck_test.go new file mode 100644 index 0000000..2d098ba --- /dev/null +++ b/internal/healthcheck/healthcheck_test.go @@ -0,0 +1,95 @@ +package healthcheck + +import ( + "strings" + "testing" + "time" +) + +func TestRunChecks_Passed(t *testing.T) { + results := RunChecks([]Check{{Name: "ok-check", Command: "echo all-good; exit 0"}}) + if len(results) != 1 { + t.Fatalf("len(results) = %d, want 1", len(results)) + } + r := results[0] + if r.Name != "ok-check" { + t.Errorf("Name = %q, want %q", r.Name, "ok-check") + } + if !r.Ok { + t.Errorf("Ok = false, want true (output=%q)", r.Output) + } + if !strings.Contains(r.Output, "all-good") { + t.Errorf("Output = %q, want it to contain %q", r.Output, "all-good") + } +} + +func TestRunChecks_Failed(t *testing.T) { + results := RunChecks([]Check{{Name: "bad-check", Command: "echo something-broke; exit 1"}}) + if len(results) != 1 { + t.Fatalf("len(results) = %d, want 1", len(results)) + } + r := results[0] + if r.Ok { + t.Errorf("Ok = true, want false") + } + if !strings.Contains(r.Output, "something-broke") { + t.Errorf("Output = %q, want it to contain %q", r.Output, "something-broke") + } +} + +func TestRunChecks_Timeout(t *testing.T) { + origTimeout, origWaitDelay := Timeout, WaitDelay + // Shrink the timeout (and its WaitDelay grace period) for the test so + // it runs fast; restore both after. + Timeout = 50 * time.Millisecond + WaitDelay = 50 * time.Millisecond + defer func() { Timeout, WaitDelay = origTimeout, origWaitDelay }() + + results := RunChecks([]Check{{Name: "slow-check", Command: "sleep 5"}}) + if len(results) != 1 { + t.Fatalf("len(results) = %d, want 1", len(results)) + } + r := results[0] + if r.Ok { + t.Errorf("Ok = true, want false (command should have timed out)") + } + if !strings.Contains(r.Output, "timed out") { + t.Errorf("Output = %q, want it to mention the timeout", r.Output) + } +} + +func TestRunChecks_MultipleInOrder(t *testing.T) { + results := RunChecks([]Check{ + {Name: "first", Command: "exit 0"}, + {Name: "second", Command: "exit 1"}, + }) + if len(results) != 2 { + t.Fatalf("len(results) = %d, want 2", len(results)) + } + if results[0].Name != "first" || !results[0].Ok { + t.Errorf("results[0] = %+v, want ok first", results[0]) + } + if results[1].Name != "second" || results[1].Ok { + t.Errorf("results[1] = %+v, want failing second", results[1]) + } +} + +func TestRunChecks_SkipsUnsupportedType(t *testing.T) { + results := RunChecks([]Check{ + {Name: "command-check", Command: "exit 0", Type: "command"}, + {Name: "http-check", Command: "irrelevant", Type: "http"}, + {Name: "default-type-check", Command: "exit 0"}, + }) + var names []string + for _, r := range results { + names = append(names, r.Name) + } + if len(results) != 2 { + t.Fatalf("results = %v, want only the two supported (command-type) checks", names) + } + for _, n := range names { + if n == "http-check" { + t.Errorf("unsupported type=http check %q should have been skipped, got results = %v", n, names) + } + } +} diff --git a/internal/healthcheck/issues.go b/internal/healthcheck/issues.go new file mode 100644 index 0000000..1467e19 --- /dev/null +++ b/internal/healthcheck/issues.go @@ -0,0 +1,46 @@ +package healthcheck + +import ( + "fmt" + "time" +) + +// IncidentLabel is the GitHub label applied to every issue this package +// opens for a failing health check. +const IncidentLabel = "production-incident" + +// TitlePrefix returns the "[health-check: ]" title-prefix convention +// used to identify/dedupe the GitHub issue for a given check name. Per +// Issue #190, dedup is judged by scanning open issue titles for this exact +// prefix (not a hidden marker comment, unlike internal/requirements' sweep) +// so a human skimming the issue list can immediately tell which check an +// incident issue is about. +func TitlePrefix(name string) string { + return fmt.Sprintf("[health-check: %s]", name) +} + +// RecoveredTrigger is the substring HasRecoveredComment implementations +// look for in an issue's comments to decide whether the one-time recovery +// comment has already been posted (see PostRecoveredComment). +const RecoveredTrigger = "recovered at" + +// IssueClient is the subset of GitHub issue operations the health-check +// reconcile needs. It is deliberately narrow so tests can supply an +// in-memory fake, mirroring internal/requirements.IssueClient. +type IssueClient interface { + // FindOpenIssue reports whether an open issue already exists for the + // given check name (matched via TitlePrefix), and its issue number if + // so. + FindOpenIssue(name string) (number int, found bool, err error) + // CreateIncidentIssue opens a new production-incident-labeled issue for + // a failing check, embedding the check name, command, failure output, + // and detection time into the issue body. + CreateIncidentIssue(name, command, output string, detectedAt time.Time) (number int, err error) + // HasRecoveredComment reports whether the given issue already has a + // one-time "recovered at ..." comment posted on it. + HasRecoveredComment(number int) (bool, error) + // PostRecoveredComment posts the one-time "recovered at