From 6ba99c9c8e4afdfc6077a88df6dbc19cc0f12772 Mon Sep 17 00:00:00 2001 From: ytnobody Date: Tue, 28 Jul 2026 17:11:52 +0900 Subject: [PATCH] feat(selfaudit): add idle-time self-audit sweep + on-demand run_self_audit tool (Closes #164) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a lightweight, LLM-driven review pass (bugs, missing test coverage, security holes) that runs when the Superintendent's Issue queue is empty, throttled to roughly hourly via a new SelfAuditSince cadence timestamp (mirroring RequirementsSweepSince), plus an on-demand run_self_audit MCP tool that shares the exact same filing logic. Design (Go-vs-LLM split, per Issue #164's open judgment call): - internal/selfaudit is the single implementation both call paths share. It cannot itself decide what "a bug" is (unlike run_requirements_sweep's test-command exit code, or run_health_checks' check command, there is no deterministic verdict to shell out to for "review this code"), so that judgment is delegated to the calling LLM via a shared Instructions text. - Everything mechanical is done in Go: cadence tracking (self_audit_since via get_loop_state/update_loop_state, following the existing pattern exactly), deduping a finding against BOTH open and closed Issues (a new ListIssuesAnyState client method, since a finding whose Issue was already filed and since closed must not be re-filed), and actually filing the Issue (self-audit label). The Superintendent never touches the GitHub API directly for this — it only produces structured findings. - run_self_audit is two-phase/stateless: called with no `findings` it returns instructions to follow; called again with `findings` it dedupes and files. This keeps the on-demand and cadence-driven paths identical. Also updates CLAUDE.md's step 9 (extended rather than renumbering the whole cycle) to describe the new cadence, the .claude/settings.json permissions allowlist (new mcp__hermit__run_self_audit entry), and the expected-tool-count assertions in cmd/hermit/serve_test.go (21 -> 22). Co-Authored-By: Claude Sonnet 5 --- .claude/settings.json | 3 +- CLAUDE.md | 8 +- cmd/hermit/serve_test.go | 12 +- internal/github/client.go | 30 +++- internal/mcp/tools.go | 88 ++++++++++- internal/mcp/tools_test.go | 100 +++++++++++++ internal/selfaudit/github_adapter.go | 62 ++++++++ internal/selfaudit/github_adapter_test.go | 84 +++++++++++ internal/selfaudit/selfaudit.go | 173 ++++++++++++++++++++++ internal/selfaudit/selfaudit_test.go | 148 ++++++++++++++++++ internal/state/state.go | 9 +- internal/state/state_test.go | 6 +- 12 files changed, 708 insertions(+), 15 deletions(-) create mode 100644 internal/selfaudit/github_adapter.go create mode 100644 internal/selfaudit/github_adapter_test.go create mode 100644 internal/selfaudit/selfaudit.go create mode 100644 internal/selfaudit/selfaudit_test.go diff --git a/.claude/settings.json b/.claude/settings.json index b72b40b..ee7187d 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -41,7 +41,8 @@ "mcp__hermit__now", "mcp__hermit__get_loop_state", "mcp__hermit__update_loop_state", - "mcp__hermit__run_health_checks" + "mcp__hermit__run_health_checks", + "mcp__hermit__run_self_audit" ] } } diff --git a/CLAUDE.md b/CLAUDE.md index be2d167..0f7c57e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -60,7 +60,13 @@ This check is nearly free (one read-only call) and guards against a known flaky - `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 +9. If there are no Issues, run the idle-time self-audit sweep roughly once an hour instead of ending the pass immediately, tracking a separate "last self-audit time" across passes (`self_audit_since`) the same way step 7 tracks its own requirements-sweep timestamp (store the current time before calling) — this only applies when the queue is genuinely empty; a non-empty queue always skips straight to step 10: + - Only run the self-audit when at least 3600 seconds have elapsed since the last recorded self-audit time; otherwise skip straight to ending the pass (do not audit early — the point is to fill idle passes roughly hourly, not every empty-queue pass) + - When due, call `run_self_audit` with no `findings` argument first: it returns an `instructions` field describing the lightweight review to perform (bugs, missing test coverage, security holes — see `internal/selfaudit.Instructions`, the single source of truth this prose mirrors). Follow those instructions against the current codebase. + - For each concrete problem found, call `run_self_audit` again passing `findings` (an array of `{title, body}` objects, one call covering all findings from this sweep). The tool dedupes each finding against existing open **and** closed Issues by a normalized-title match and files a new Issue (labeled `self-audit`) only for non-duplicates — this is Go-side plumbing, not something to do by hand. + - The same hard prohibition on implementing applies here: never fix a finding yourself, however small it looks — only let `run_self_audit` file the Issue and leave the fix to the normal Engineer pipeline (step 10 onward, on a later pass once the Issue exists). + - Update the stored last-self-audit timestamp to now after calling `run_self_audit` (even if the review turned up zero findings — the throttle governs how often the sweep runs, not whether it found anything) + - Once the self-audit step has run (or been skipped because it isn't due yet), 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) diff --git a/cmd/hermit/serve_test.go b/cmd/hermit/serve_test.go index c9df513..13c36ba 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) != 21 { - t.Errorf("expected 21 tools, got %d", len(tools)) + if len(tools) != 22 { + t.Errorf("expected 22 tools, got %d", len(tools)) } return case <-deadline: @@ -207,8 +207,8 @@ language = "ja" continue } tools, _ := msg.Result["tools"].([]any) - if len(tools) != 21 { - t.Errorf("expected 21 tools, got %d", len(tools)) + if len(tools) != 22 { + t.Errorf("expected 22 tools, got %d", len(tools)) } return case <-deadline: @@ -314,8 +314,8 @@ language = "ja" continue } tools, _ := msg.Result["tools"].([]any) - if len(tools) != 21 { - t.Errorf("expected 21 tools, got %d", len(tools)) + if len(tools) != 22 { + t.Errorf("expected 22 tools, got %d", len(tools)) } return case <-deadline: diff --git a/internal/github/client.go b/internal/github/client.go index 573c76e..f6f98bb 100644 --- a/internal/github/client.go +++ b/internal/github/client.go @@ -179,8 +179,17 @@ func (c *Client) isTrustedAuthor(association string) bool { // are not silently dropped: each one is logged so a human operator watching // HERMIT's logs can notice. func (c *Client) listOpenIssuesFromRepo(owner, repo, label string) ([]Issue, error) { + return c.listIssuesFromRepoWithState(owner, repo, label, "open") +} + +// listIssuesFromRepoWithState is the shared implementation behind +// listOpenIssuesFromRepo (state="open") and ListIssuesAnyState (state="all"): +// it fetches issues from a specific owner/repo pair in the given GitHub +// issue-list state, optionally filtering by label. Each returned Issue has +// its Owner and Repo fields set to the provided values. +func (c *Client) listIssuesFromRepoWithState(owner, repo, label, state string) ([]Issue, error) { opts := &gogithub.IssueListByRepoOptions{ - State: "open", + State: state, } if label != "" { opts.Labels = []string{label} @@ -233,6 +242,25 @@ func (c *Client) ListOpenIssues(label string) ([]Issue, error) { return issues, nil } +// ListIssuesAnyState returns issues in any state (open or closed) from the +// client's primary repository, optionally filtered by label. Owner/Repo +// fields are NOT set, mirroring ListOpenIssues' single-repo backward +// compatibility. This is used by callers that need to dedupe against the +// full Issue history rather than just the open queue — e.g. the self-audit +// sweep (Issue #164), which must not re-file an Issue for a finding whose +// Issue was already opened and since closed. +func (c *Client) ListIssuesAnyState(label string) ([]Issue, error) { + issues, err := c.listIssuesFromRepoWithState(c.owner, c.repo, label, "all") + if err != nil { + return nil, err + } + for i := range issues { + issues[i].Owner = "" + issues[i].Repo = "" + } + return issues, nil +} + // ListAllIssues fetches open issues from all provided repos. If repos is // empty, it falls back to the client's primary repo (same as ListOpenIssues // but with Owner/Repo fields populated). The label filter in each RepoConfig diff --git a/internal/mcp/tools.go b/internal/mcp/tools.go index b3f30a0..d91e6e7 100644 --- a/internal/mcp/tools.go +++ b/internal/mcp/tools.go @@ -18,6 +18,7 @@ import ( "github.com/ytnobody/hermit/internal/readiness" "github.com/ytnobody/hermit/internal/requirements" "github.com/ytnobody/hermit/internal/risk" + "github.com/ytnobody/hermit/internal/selfaudit" "github.com/ytnobody/hermit/internal/state" ) @@ -58,8 +59,17 @@ type githubClient interface { // AddLabel is used by the run_health_checks tool to label newly-opened // production-incident issues via healthcheck.NewGitHubIssueClient — see // healthcheck.ghClient, which this interface must remain a superset of - // (Issue #190). + // (Issue #190). It is also used by run_self_audit to label newly-opened + // self-audit-finding issues via selfaudit.NewGitHubIssueClient (Issue + // #164). AddLabel(number int, label string) error + // ListIssuesAnyState is used by the run_self_audit tool to dedupe + // findings against both open and closed issues via + // selfaudit.NewGitHubIssueClient — see selfaudit.ghClient, which this + // interface must remain a superset of (Issue #164). Unlike + // ListOpenIssues, a self-audit finding whose issue was already filed and + // since closed must not be re-filed. + ListIssuesAnyState(label string) ([]gh.Issue, error) } // resolveRiskConfig returns the risk.Config to apply for the given owner/repo @@ -560,7 +570,7 @@ func registerTools(s *server.MCPServer, client githubClient, rateLimitThreshold s.AddTool( mcp.NewTool("get_loop_state", - mcp.WithDescription("Returns the cadence-tracking timestamps persisted in .hermit/superintendent-state.json: pr_comments_since, issue_comments_since, requirements_sweep_since, and health_checks_since (RFC3339, omitted if never recorded) — the 'since' values the Superintendent cycle uses to decide when it last checked PR comments, checked Issue comments, ran the requirements sweep, and ran the health-check sweep. Also reports last_success_tick and consecutive_failures, written by `hermit run`'s own tick loop. This file is owned by HERMIT's Go side: read/write these cadence timestamps only via this tool and update_loop_state, never by hand-writing the JSON file."), + mcp.WithDescription("Returns the cadence-tracking timestamps persisted in .hermit/superintendent-state.json: pr_comments_since, issue_comments_since, requirements_sweep_since, health_checks_since, and self_audit_since (RFC3339, omitted if never recorded) — the 'since' values the Superintendent cycle uses to decide when it last checked PR comments, checked Issue comments, ran the requirements sweep, ran the health-check sweep, and ran the idle-time self-audit sweep. Also reports last_success_tick and consecutive_failures, written by `hermit run`'s own tick loop. This file is owned by HERMIT's Go side: read/write these cadence timestamps only via this tool and update_loop_state, never by hand-writing the JSON file."), ), func(ctx context.Context, req mcp.CallToolRequest) (*mcp.CallToolResult, error) { st, err := state.Load(state.Path(rootDir)) @@ -574,11 +584,12 @@ func registerTools(s *server.MCPServer, client githubClient, rateLimitThreshold s.AddTool( mcp.NewTool("update_loop_state", - mcp.WithDescription("Updates one or more of the cadence-tracking timestamps in .hermit/superintendent-state.json: pr_comments_since, issue_comments_since, requirements_sweep_since, health_checks_since, each an RFC3339 timestamp. Only the fields provided are changed; omitted fields are left as-is. Call the now tool first to get an authoritative current timestamp to pass in, then use this instead of writing the JSON file directly. Returns the full updated state."), + mcp.WithDescription("Updates one or more of the cadence-tracking timestamps in .hermit/superintendent-state.json: pr_comments_since, issue_comments_since, requirements_sweep_since, health_checks_since, self_audit_since, each an RFC3339 timestamp. Only the fields provided are changed; omitted fields are left as-is. Call the now tool first to get an authoritative current timestamp to pass in, then use this instead of writing the JSON file directly. Returns the full updated state."), mcp.WithString("pr_comments_since", mcp.Description("RFC3339 timestamp to record as the last PR-review-comment check time")), mcp.WithString("issue_comments_since", mcp.Description("RFC3339 timestamp to record as the last Issue-comment check time")), mcp.WithString("requirements_sweep_since", mcp.Description("RFC3339 timestamp to record as the last requirements-sweep time")), mcp.WithString("health_checks_since", mcp.Description("RFC3339 timestamp to record as the last health-check sweep time")), + mcp.WithString("self_audit_since", mcp.Description("RFC3339 timestamp to record as the last self-audit sweep time")), ), func(ctx context.Context, req mcp.CallToolRequest) (*mcp.CallToolResult, error) { statePath := state.Path(rootDir) @@ -614,6 +625,13 @@ func registerTools(s *server.MCPServer, client githubClient, rateLimitThreshold } st.HealthChecksSince = &t } + if v := req.GetString("self_audit_since", ""); v != "" { + t, err := time.Parse(time.RFC3339, v) + if err != nil { + return mcp.NewToolResultError(fmt.Sprintf("self_audit_since: %v", err)), nil + } + st.SelfAuditSince = &t + } if err := state.Save(statePath, st); err != nil { return mcp.NewToolResultError(err.Error()), nil } @@ -801,6 +819,69 @@ func registerTools(s *server.MCPServer, client githubClient, rateLimitThreshold return mcp.NewToolResultText(string(b)), nil }, ) + + s.AddTool( + mcp.NewTool("run_self_audit", + mcp.WithDescription("Runs the idle-time self-audit sweep (Issue #164): a lightweight code-review pass looking for bugs, missing test coverage, and security holes, used to keep HERMIT finding work when the Issue queue is empty. This tool is two-phase and stateless between calls — it does not run any analysis itself:\n\n1. Call with no 'findings' argument. The response's 'instructions' field describes the review to perform (see selfaudit.Instructions) — read it and actually perform that review against the current codebase.\n2. For each concrete finding, call this tool again passing 'findings': an array of {\"title\", \"body\"} objects, one per finding. Each finding is deduped against existing open AND closed GitHub Issues by a normalized-title match (so a finding whose Issue was already filed and since closed is not re-filed) and, if not a duplicate, filed as a new Issue labeled 'self-audit'. Returns a summary of how many issues were opened vs. skipped as duplicates.\n\nThe caller (Superintendent) must never fix a finding itself — only file the Issue and leave implementation to the normal Engineer pipeline (same 'coordinator, not implementer' rule as every other step). The Superintendent loop should call this roughly hourly when the Issue queue is empty (tracking its own 'last self-audit' timestamp via self_audit_since, the same way it tracks requirements_sweep_since), not on every cycle. The on-demand caller (e.g. a human in a chat session) may call it any time without waiting for that cadence — both paths share the exact same dedupe/filing logic (internal/selfaudit.File)."), + mcp.WithArray("findings", + mcp.Description("Findings to file as (deduped) GitHub Issues, one call after the review described in 'instructions' has actually been performed. Omit entirely (or pass an empty array) to just receive the instructions without filing anything."), + mcp.Items(map[string]any{ + "type": "object", + "properties": map[string]any{ + "title": map[string]any{ + "type": "string", + "description": "Short human-readable summary of the finding, e.g. \"nil pointer dereference in foo.Bar when cfg is empty\". Do not include a [self-audit] prefix — it is added automatically.", + }, + "body": map[string]any{ + "type": "string", + "description": "Full finding detail: what/where the problem is, why it matters, and file references. Becomes the Issue body verbatim.", + }, + }, + "required": []string{"title", "body"}, + }), + ), + ), + func(ctx context.Context, req mcp.CallToolRequest) (*mcp.CallToolResult, error) { + rawFindings, hasFindings := req.GetArguments()["findings"] + findingsList, _ := rawFindings.([]any) + if !hasFindings || len(findingsList) == 0 { + b, _ := json.Marshal(map[string]any{ + "instructions": selfaudit.Instructions, + "note": "No findings provided — no Issues were filed. Perform the review described in 'instructions', then call run_self_audit again with a non-empty 'findings' array for each concrete problem found.", + }) + return mcp.NewToolResultText(string(b)), nil + } + + findings := make([]selfaudit.Finding, 0, len(findingsList)) + for i, raw := range findingsList { + obj, ok := raw.(map[string]any) + if !ok { + return mcp.NewToolResultError(fmt.Sprintf("findings[%d]: expected an object with title/body", i)), nil + } + title, _ := obj["title"].(string) + body, _ := obj["body"].(string) + if title == "" { + return mcp.NewToolResultError(fmt.Sprintf("findings[%d]: title is required", i)), nil + } + findings = append(findings, selfaudit.Finding{Title: title, Body: body}) + } + + summary, err := selfaudit.File(findings, selfaudit.NewGitHubIssueClient(client)) + if err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + b, err := json.Marshal(map[string]any{ + "findings_received": summary.FindingsReceived, + "issues_opened": summary.IssuesOpened, + "duplicates_skipped": summary.Duplicates, + "results": summary.Results, + }) + if err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + return mcp.NewToolResultText(string(b)), nil + }, + ) } // loopStateResponse converts a state.LoopState into the JSON-friendly shape @@ -820,6 +901,7 @@ func loopStateResponse(st state.LoopState) map[string]any { setIfNotNil("issue_comments_since", st.IssueCommentsSince) setIfNotNil("requirements_sweep_since", st.RequirementsSweepSince) setIfNotNil("health_checks_since", st.HealthChecksSince) + setIfNotNil("self_audit_since", st.SelfAuditSince) setIfNotNil("last_success_tick", st.LastSuccessTick) return resp } diff --git a/internal/mcp/tools_test.go b/internal/mcp/tools_test.go index bf13238..38947a2 100644 --- a/internal/mcp/tools_test.go +++ b/internal/mcp/tools_test.go @@ -105,6 +105,10 @@ func (m *mockGithubClient) ListAllIssues(_ []gh.RepoConfig) ([]gh.Issue, error) return m.issues, m.issuesErr } +func (m *mockGithubClient) ListIssuesAnyState(_ string) ([]gh.Issue, error) { + return m.issues, m.issuesErr +} + func (m *mockGithubClient) AssignIssue(_ int, _ string) error { return m.assignErr } @@ -1951,3 +1955,99 @@ func TestRunHealthChecks_PassingCheck_NoExistingIssue_NoOp(t *testing.T) { t.Errorf("expected no side effects, got created=%v posted=%v", mock.createdIssueTitles, mock.postedComments) } } + +// --- run_self_audit (Issue #164) --- + +func TestRunSelfAudit_NoFindings_ReturnsInstructionsWithoutFiling(t *testing.T) { + mock := &mockGithubClient{} + s := newTestServer(t, mock) + + got := mustToolJSON(t, callTool(t, s, "run_self_audit", map[string]any{})) + instructions, _ := got["instructions"].(string) + if instructions == "" { + t.Fatal("expected non-empty instructions when called with no findings") + } + if !strings.Contains(instructions, "bug") && !strings.Contains(instructions, "Bug") { + t.Errorf("instructions = %q, want it to mention bugs", instructions) + } + if len(mock.createdIssueTitles) != 0 { + t.Errorf("expected no issues filed when no findings are provided, got %v", mock.createdIssueTitles) + } +} + +func TestRunSelfAudit_EmptyFindingsArray_ReturnsInstructionsWithoutFiling(t *testing.T) { + mock := &mockGithubClient{} + s := newTestServer(t, mock) + + got := mustToolJSON(t, callTool(t, s, "run_self_audit", map[string]any{"findings": []any{}})) + if _, ok := got["instructions"]; !ok { + t.Errorf("expected instructions field for an empty findings array, got %v", got) + } + if len(mock.createdIssueTitles) != 0 { + t.Errorf("expected no issues filed, got %v", mock.createdIssueTitles) + } +} + +func TestRunSelfAudit_NewFinding_FilesIssueWithSelfAuditLabel(t *testing.T) { + mock := &mockGithubClient{} + s := newTestServer(t, mock) + + got := mustToolJSON(t, callTool(t, s, "run_self_audit", map[string]any{ + "findings": []any{ + map[string]any{"title": "nil deref in foo.Bar", "body": "detail about the bug"}, + }, + })) + if opened, _ := got["issues_opened"].(float64); opened != 1 { + t.Fatalf("expected issues_opened=1, got %v (full: %v)", got["issues_opened"], got) + } + if dup, _ := got["duplicates_skipped"].(float64); dup != 0 { + t.Errorf("expected duplicates_skipped=0, got %v", dup) + } + if len(mock.createdIssueTitles) != 1 { + t.Fatalf("expected exactly one issue to be created, got %d: %v", len(mock.createdIssueTitles), mock.createdIssueTitles) + } + if !strings.Contains(mock.createdIssueTitles[0], "nil deref in foo.Bar") { + t.Errorf("created issue title = %q, want it to contain the finding title", mock.createdIssueTitles[0]) + } + if len(mock.addedLabels) != 1 || mock.addedLabels[0].label != "self-audit" { + t.Errorf("addedLabels = %v, want a single %q label", mock.addedLabels, "self-audit") + } +} + +func TestRunSelfAudit_DuplicateFinding_SkipsFilingAgainstClosedIssue(t *testing.T) { + mock := &mockGithubClient{ + issues: []gh.Issue{ + {Number: 5, Title: "[self-audit] nil deref in foo.Bar"}, // e.g. a closed dup + }, + } + s := newTestServer(t, mock) + + got := mustToolJSON(t, callTool(t, s, "run_self_audit", map[string]any{ + "findings": []any{ + map[string]any{"title": "nil deref in foo.Bar", "body": "detail"}, + }, + })) + if opened, _ := got["issues_opened"].(float64); opened != 0 { + t.Errorf("expected issues_opened=0 (should dedupe), got %v", opened) + } + if dup, _ := got["duplicates_skipped"].(float64); dup != 1 { + t.Errorf("expected duplicates_skipped=1, got %v", dup) + } + if len(mock.createdIssueTitles) != 0 { + t.Errorf("expected no duplicate issue to be created, got %v", mock.createdIssueTitles) + } +} + +func TestRunSelfAudit_MissingTitle_ReturnsError(t *testing.T) { + mock := &mockGithubClient{} + s := newTestServer(t, mock) + + result := callTool(t, s, "run_self_audit", map[string]any{ + "findings": []any{ + map[string]any{"body": "no title here"}, + }, + }) + if !result.IsError { + t.Fatal("expected an error result for a finding missing a title") + } +} diff --git a/internal/selfaudit/github_adapter.go b/internal/selfaudit/github_adapter.go new file mode 100644 index 0000000..dee900a --- /dev/null +++ b/internal/selfaudit/github_adapter.go @@ -0,0 +1,62 @@ +package selfaudit + +import ( + "strings" + + 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 and +// internal/healthcheck/github_adapter.go. +type ghClient interface { + // ListIssuesAnyState is used (rather than ListOpenIssues) because a + // self-audit finding whose Issue was already filed and since closed + // (fixed, or intentionally won't-fix'd) must not be re-filed just + // because the Issue is no longer open (Issue #164 acceptance criteria: + // dedupe against existing open/closed Issues). + ListIssuesAnyState(label string) ([]gh.Issue, error) + CreateIssue(title, body string) (int, error) + AddLabel(number int, label string) error +} + +// GitHubIssueClient adapts a GitHub client to the IssueClient interface used +// by File, deduping against open and closed Issues by matching the +// normalized TitlePrefix-stripped title (see NormalizeTitle). +type GitHubIssueClient struct { + client ghClient +} + +// NewGitHubIssueClient wraps client for use as a File IssueClient. +func NewGitHubIssueClient(client ghClient) *GitHubIssueClient { + return &GitHubIssueClient{client: client} +} + +func (g *GitHubIssueClient) FindDuplicate(normalizedTitle string) (int, bool, error) { + issues, err := g.client.ListIssuesAnyState("") + if err != nil { + return 0, false, err + } + for _, issue := range issues { + if !strings.HasPrefix(issue.Title, TitlePrefix) { + continue + } + if NormalizeTitle(issue.Title) == normalizedTitle { + return issue.Number, true, nil + } + } + return 0, false, nil +} + +func (g *GitHubIssueClient) CreateFindingIssue(title, body string) (int, error) { + num, err := g.client.CreateIssue(title, body) + if err != nil { + return 0, err + } + if err := g.client.AddLabel(num, Label); err != nil { + return num, err + } + return num, nil +} diff --git a/internal/selfaudit/github_adapter_test.go b/internal/selfaudit/github_adapter_test.go new file mode 100644 index 0000000..c7ed3c8 --- /dev/null +++ b/internal/selfaudit/github_adapter_test.go @@ -0,0 +1,84 @@ +package selfaudit + +import ( + "testing" + + gh "github.com/ytnobody/hermit/internal/github" +) + +type fakeGhClient struct { + issues []gh.Issue + issuesErr error + createErr error + nextNum int + createdTitles []string + addedLabels []string +} + +func (f *fakeGhClient) ListIssuesAnyState(_ string) ([]gh.Issue, error) { + return f.issues, f.issuesErr +} + +func (f *fakeGhClient) CreateIssue(title, _ string) (int, error) { + if f.createErr != nil { + return 0, f.createErr + } + f.nextNum++ + f.createdTitles = append(f.createdTitles, title) + return f.nextNum, nil +} + +func (f *fakeGhClient) AddLabel(_ int, label string) error { + f.addedLabels = append(f.addedLabels, label) + return nil +} + +func TestGitHubIssueClient_FindDuplicate_MatchesClosedIssue(t *testing.T) { + client := &fakeGhClient{ + issues: []gh.Issue{ + {Number: 9, Title: TitlePrefix + " nil deref in foo.Bar"}, + }, + } + adapter := NewGitHubIssueClient(client) + + num, found, err := adapter.FindDuplicate(NormalizeTitle("nil deref in foo.Bar")) + if err != nil { + t.Fatalf("FindDuplicate: %v", err) + } + if !found || num != 9 { + t.Errorf("FindDuplicate = (%d, %v), want (9, true)", num, found) + } +} + +func TestGitHubIssueClient_FindDuplicate_IgnoresNonSelfAuditIssues(t *testing.T) { + client := &fakeGhClient{ + issues: []gh.Issue{ + {Number: 3, Title: "nil deref in foo.Bar"}, // no self-audit prefix + }, + } + adapter := NewGitHubIssueClient(client) + + _, found, err := adapter.FindDuplicate(NormalizeTitle("nil deref in foo.Bar")) + if err != nil { + t.Fatalf("FindDuplicate: %v", err) + } + if found { + t.Error("expected no match for an issue without the self-audit title prefix") + } +} + +func TestGitHubIssueClient_CreateFindingIssue_AddsLabel(t *testing.T) { + client := &fakeGhClient{} + adapter := NewGitHubIssueClient(client) + + num, err := adapter.CreateFindingIssue(TitlePrefix+" x", "body") + if err != nil { + t.Fatalf("CreateFindingIssue: %v", err) + } + if num != 1 { + t.Errorf("num = %d, want 1", num) + } + if len(client.addedLabels) != 1 || client.addedLabels[0] != Label { + t.Errorf("addedLabels = %v, want [%q]", client.addedLabels, Label) + } +} diff --git a/internal/selfaudit/selfaudit.go b/internal/selfaudit/selfaudit.go new file mode 100644 index 0000000..728f054 --- /dev/null +++ b/internal/selfaudit/selfaudit.go @@ -0,0 +1,173 @@ +// Package selfaudit implements the idle-time self-audit sweep (Issue #164): +// a lightweight, LLM-driven review pass that looks for bugs, missing test +// coverage, and security holes when the Superintendent's Issue queue is +// empty, and files (deduped) GitHub Issues for whatever it finds instead of +// fixing anything itself. +// +// Design split (documented here because the originating Issue left the +// exact Go-vs-LLM boundary as an engineering judgment call): +// +// - The actual review — reading code, deciding whether something is a +// real bug/gap/vulnerability worth filing — cannot reasonably be done in +// Go. Unlike internal/requirements (which runs a concrete test command +// per requirement) or internal/healthcheck (which runs a concrete health +// check command), "review the codebase for bugs" has no deterministic +// command to shell out to. That judgment is delegated to the calling +// agent (the Superintendent), guided by the Instructions text below. +// - Everything mechanical around that judgment — cadence tracking (via +// internal/state, mirroring RequirementsSweepSince), deduping a finding +// against existing Issues, and actually filing the Issue — is +// implemented here in Go, so the LLM side only ever produces structured +// findings and never talks to the GitHub API directly. This keeps the +// "Superintendent never edits/fixes, only files Issues" boundary +// enforced in code rather than relying on the LLM to police itself. +// +// This mirrors internal/healthcheck's split (Go does the dedupe/filing +// plumbing; the check command's pass/fail judgment is external) more than +// internal/requirements' (which does the "is it satisfied" judgment in Go +// too, since a test command's exit code is a deterministic verdict) — a code +// review sweep has no equivalent deterministic verdict to compute. +package selfaudit + +import ( + "fmt" + "regexp" + "strings" +) + +// TitlePrefix is the fixed prefix applied to every Issue this package +// files, used both for human-skimmable identification (mirroring +// internal/healthcheck.TitlePrefix's convention) and as the anchor for +// duplicate detection against existing Issues. +const TitlePrefix = "[self-audit]" + +// Label is the GitHub label applied to every Issue this package files. +const Label = "self-audit" + +// Finding is a single concrete issue surfaced by the calling agent's review +// (bug, missing test coverage, or security hole), to be filed as a GitHub +// Issue if it is not a duplicate of an existing one. +type Finding struct { + // Title is a short human-readable summary, e.g. "nil pointer dereference + // in foo.Bar when cfg is empty". Must not already include TitlePrefix — + // File adds it. + Title string `json:"title"` + // Body is the full finding detail: what/where the problem is, why it + // matters, and (for missing-test-coverage findings) what behavior is + // untested. Should read like a well-specified Issue body, since it + // becomes one verbatim (aside from an added provenance footer). + Body string `json:"body"` +} + +// FindingResult records what happened when File processed a single Finding. +type FindingResult struct { + Title string `json:"title"` + IssueCreated bool `json:"issue_created"` + IssueNumber int `json:"issue_number,omitempty"` + DuplicateOf int `json:"duplicate_of,omitempty"` + IsDuplicate bool `json:"is_duplicate"` +} + +// Summary is the aggregated outcome of File, returned to both the +// run_self_audit MCP tool and (indirectly, via the same code path) any +// future caller. +type Summary struct { + FindingsReceived int `json:"findings_received"` + IssuesOpened int `json:"issues_opened"` + Duplicates int `json:"duplicates_skipped"` + Results []FindingResult `json:"results"` +} + +// IssueClient is the subset of GitHub issue operations File needs. It is +// deliberately narrow so tests can supply an in-memory fake, mirroring +// internal/requirements.IssueClient and internal/healthcheck.IssueClient. +type IssueClient interface { + // FindDuplicate reports whether an Issue (open or closed) already exists + // for a finding with this normalized title, and its number if so. + FindDuplicate(normalizedTitle string) (number int, found bool, err error) + // CreateFindingIssue opens a new self-audit-labeled Issue for a finding. + CreateFindingIssue(title, body string) (number int, err error) +} + +// nonAlnum matches runs of characters that are not letters or digits, used +// by NormalizeTitle to collapse punctuation/whitespace differences before +// comparing titles for duplicate detection. +var nonAlnum = regexp.MustCompile(`[^a-z0-9]+`) + +// NormalizeTitle reduces a finding (or existing Issue) title to a +// lowercase, punctuation-collapsed form for duplicate comparison, and trims +// TitlePrefix if present. This is intentionally a simple, exact-match-after- +// normalization heuristic rather than fuzzy/semantic matching — proportionate +// to a "lightweight" audit sweep (see package doc), not a from-scratch +// duplicate-detection engine. Two findings whose titles normalize to the +// same string are treated as duplicates; anything less similar than that is +// filed as a new Issue (a human can always close a near-duplicate by hand). +func NormalizeTitle(title string) string { + t := strings.ToLower(title) + t = strings.TrimPrefix(t, strings.ToLower(TitlePrefix)) + t = nonAlnum.ReplaceAllString(t, " ") + return strings.TrimSpace(t) +} + +// File dedupes each finding against existing Issues (via +// IssueClient.FindDuplicate) and creates a new self-audit Issue for every +// finding that is not a duplicate. Findings are processed independently — +// an error filing one finding's Issue aborts the remaining findings and +// returns the partial Summary alongside the error, so callers can see what +// was already filed. +// +// This is the single implementation shared by the run_self_audit MCP +// tool's on-demand path and (per CLAUDE.md's cadence step) the Superintendent +// cycle's throttled idle-time path — both converge on this function so the +// two invocation paths can never diverge in what "file a self-audit finding" +// means. +func File(findings []Finding, issues IssueClient) (Summary, error) { + summary := Summary{FindingsReceived: len(findings)} + for _, f := range findings { + normalized := NormalizeTitle(f.Title) + result := FindingResult{Title: f.Title} + + if normalized != "" { + num, found, err := issues.FindDuplicate(normalized) + if err != nil { + summary.Results = append(summary.Results, result) + return summary, err + } + if found { + result.IsDuplicate = true + result.DuplicateOf = num + summary.Duplicates++ + summary.Results = append(summary.Results, result) + continue + } + } + + title := fmt.Sprintf("%s %s", TitlePrefix, f.Title) + num, err := issues.CreateFindingIssue(title, f.Body) + if err != nil { + summary.Results = append(summary.Results, result) + return summary, err + } + result.IssueCreated = true + result.IssueNumber = num + summary.IssuesOpened++ + summary.Results = append(summary.Results, result) + } + return summary, nil +} + +// Instructions is the shared prose describing the audit sweep an agent +// (Superintendent, or a human running run_self_audit on demand) should +// perform. It is the single source of truth for "what does a self-audit +// sweep actually look at", surfaced via the run_self_audit MCP tool's +// description/response so CLAUDE.md's cadence step and any on-demand caller +// see identical guidance. +const Instructions = `Perform a lightweight, proportionate review sweep of the codebase looking for three kinds of concrete problems: + +1. Bugs: logic errors, unhandled error paths, race conditions, off-by-one/nil-dereference-shaped mistakes — anything you can point at a specific file/line and explain why it is wrong. +2. Missing test coverage: exported functions or behavior-critical branches (especially recently changed ones) with no corresponding test. +3. Security holes: unsanitized input reaching a shell/SQL/file-path sink, secrets committed to the repo, missing auth checks, or similar. + +Scope this like a focused code-review pass (similar in spirit to the security-review skill), not an exhaustive static-analysis engine — a handful of well-substantiated findings is the expected output, not hundreds of nitpicks. Only surface something you are reasonably confident is a real, actionable problem. + +Do NOT fix anything you find. For each real finding, call run_self_audit again with a "findings" array of {"title", "body"} objects — title a short summary, body the full detail (what/where/why it matters, with file references). This tool dedupes each finding against existing open and closed Issues by a normalized-title match and only files a new Issue when no duplicate exists; filing is otherwise automatic. Leave the fix itself to the normal Engineer pipeline.` diff --git a/internal/selfaudit/selfaudit_test.go b/internal/selfaudit/selfaudit_test.go new file mode 100644 index 0000000..bf47056 --- /dev/null +++ b/internal/selfaudit/selfaudit_test.go @@ -0,0 +1,148 @@ +package selfaudit + +import "testing" + +type fakeIssueClient struct { + duplicates map[string]int // normalized title -> existing issue number + findDuplicateErr error + createErr error + nextNum int + createdTitles []string + createdBodies []string +} + +func (f *fakeIssueClient) FindDuplicate(normalizedTitle string) (int, bool, error) { + if f.findDuplicateErr != nil { + return 0, false, f.findDuplicateErr + } + if num, ok := f.duplicates[normalizedTitle]; ok { + return num, true, nil + } + return 0, false, nil +} + +func (f *fakeIssueClient) CreateFindingIssue(title, body string) (int, error) { + if f.createErr != nil { + return 0, f.createErr + } + f.nextNum++ + f.createdTitles = append(f.createdTitles, title) + f.createdBodies = append(f.createdBodies, body) + return f.nextNum, nil +} + +func TestFile_NewFinding_CreatesIssue(t *testing.T) { + client := &fakeIssueClient{duplicates: map[string]int{}} + findings := []Finding{{Title: "nil deref in foo.Bar", Body: "detail"}} + + summary, err := File(findings, client) + if err != nil { + t.Fatalf("File: %v", err) + } + if summary.IssuesOpened != 1 { + t.Errorf("IssuesOpened = %d, want 1", summary.IssuesOpened) + } + if summary.Duplicates != 0 { + t.Errorf("Duplicates = %d, want 0", summary.Duplicates) + } + if len(client.createdTitles) != 1 { + t.Fatalf("expected 1 issue created, got %d", len(client.createdTitles)) + } + if got := client.createdTitles[0]; got != TitlePrefix+" nil deref in foo.Bar" { + t.Errorf("created title = %q, want prefixed title", got) + } + if len(summary.Results) != 1 || !summary.Results[0].IssueCreated || summary.Results[0].IssueNumber != 1 { + t.Errorf("Results = %+v, want a single created result", summary.Results) + } +} + +func TestFile_DuplicateFinding_SkipsCreate(t *testing.T) { + client := &fakeIssueClient{duplicates: map[string]int{"nil deref in foo bar": 42}} + findings := []Finding{{Title: "nil deref in foo.Bar", Body: "detail"}} + + summary, err := File(findings, client) + if err != nil { + t.Fatalf("File: %v", err) + } + if summary.IssuesOpened != 0 { + t.Errorf("IssuesOpened = %d, want 0", summary.IssuesOpened) + } + if summary.Duplicates != 1 { + t.Errorf("Duplicates = %d, want 1", summary.Duplicates) + } + if len(client.createdTitles) != 0 { + t.Errorf("expected no issue created for a duplicate, got %v", client.createdTitles) + } + if len(summary.Results) != 1 || !summary.Results[0].IsDuplicate || summary.Results[0].DuplicateOf != 42 { + t.Errorf("Results = %+v, want a single duplicate result pointing at #42", summary.Results) + } +} + +func TestFile_MultipleFindings_MixedDuplicates(t *testing.T) { + client := &fakeIssueClient{duplicates: map[string]int{"already filed": 7}} + findings := []Finding{ + {Title: "already filed", Body: "b1"}, + {Title: "brand new bug", Body: "b2"}, + } + + summary, err := File(findings, client) + if err != nil { + t.Fatalf("File: %v", err) + } + if summary.FindingsReceived != 2 { + t.Errorf("FindingsReceived = %d, want 2", summary.FindingsReceived) + } + if summary.IssuesOpened != 1 || summary.Duplicates != 1 { + t.Errorf("IssuesOpened=%d Duplicates=%d, want 1 and 1", summary.IssuesOpened, summary.Duplicates) + } +} + +func TestFile_EmptyFindings_NoOp(t *testing.T) { + client := &fakeIssueClient{duplicates: map[string]int{}} + summary, err := File(nil, client) + if err != nil { + t.Fatalf("File: %v", err) + } + if summary.IssuesOpened != 0 || summary.Duplicates != 0 || summary.FindingsReceived != 0 { + t.Errorf("Summary = %+v, want all-zero for no findings", summary) + } +} + +func TestFile_CreateError_ReturnsPartialSummaryAndError(t *testing.T) { + client := &fakeIssueClient{duplicates: map[string]int{}, createErr: errBoom} + findings := []Finding{{Title: "will fail", Body: "b"}} + + _, err := File(findings, client) + if err == nil { + t.Fatal("expected error from CreateFindingIssue to propagate") + } +} + +func TestFile_DuplicateCheckError_ReturnsError(t *testing.T) { + client := &fakeIssueClient{findDuplicateErr: errBoom} + findings := []Finding{{Title: "x", Body: "b"}} + + _, err := File(findings, client) + if err == nil { + t.Fatal("expected error from FindDuplicate to propagate") + } +} + +func TestNormalizeTitle_StripsPrefixAndPunctuation(t *testing.T) { + cases := map[string]string{ + "[self-audit] nil deref in foo.Bar!": "nil deref in foo bar", + "Nil Deref In Foo.Bar": "nil deref in foo bar", + " extra spaces ": "extra spaces", + } + for in, want := range cases { + if got := NormalizeTitle(in); got != want { + t.Errorf("NormalizeTitle(%q) = %q, want %q", in, got, want) + } + } +} + +var errBoom = &testError{"boom"} + +type testError struct{ msg string } + +func (e *testError) Error() string { return e.msg } diff --git a/internal/state/state.go b/internal/state/state.go index e532086..1c0bf81 100644 --- a/internal/state/state.go +++ b/internal/state/state.go @@ -9,8 +9,9 @@ // - The get_loop_state / update_loop_state MCP tools (internal/mcp), // called from inside a Superintendent pass, own // PRCommentsSince / IssueCommentsSince / RequirementsSweepSince / -// HealthChecksSince — the "since" timestamps the Superintendent cycle -// previously had to hand-write into the file itself. +// HealthChecksSince / SelfAuditSince — the "since" timestamps the +// Superintendent cycle previously had to hand-write into the file +// itself. // // Either writer only ever does a load-modify-save round trip, and the two // never run concurrently by construction: `hermit run` blocks on the @@ -59,6 +60,10 @@ type LoopState struct { // HealthChecksSince is the last time run_health_checks ran, set via // update_loop_state (Issue #190). HealthChecksSince *time.Time `json:"health_checks_since,omitempty"` + // SelfAuditSince is the last time run_self_audit ran (either via the + // idle-time cadence step or an on-demand call), set via + // update_loop_state (Issue #164). + SelfAuditSince *time.Time `json:"self_audit_since,omitempty"` // LastSuccessTick is the wall-clock time of the most recent // `hermit run` pass that completed without error. Written directly by // internal/runloop, not via an MCP tool. diff --git a/internal/state/state_test.go b/internal/state/state_test.go index f21d9d0..e6c3d55 100644 --- a/internal/state/state_test.go +++ b/internal/state/state_test.go @@ -13,7 +13,7 @@ func TestLoadMissingFileReturnsZeroValue(t *testing.T) { if err != nil { t.Fatalf("Load: unexpected error: %v", err) } - if st.PRCommentsSince != nil || st.IssueCommentsSince != nil || st.RequirementsSweepSince != nil || st.HealthChecksSince != nil { + if st.PRCommentsSince != nil || st.IssueCommentsSince != nil || st.RequirementsSweepSince != nil || st.HealthChecksSince != nil || st.SelfAuditSince != nil { t.Fatalf("Load on missing file: want zero-value LoopState, got %+v", st) } if st.ConsecutiveFailures != 0 { @@ -31,6 +31,7 @@ func TestSaveThenLoadRoundTrips(t *testing.T) { IssueCommentsSince: &now, RequirementsSweepSince: &now, HealthChecksSince: &now, + SelfAuditSince: &now, LastSuccessTick: &now, ConsecutiveFailures: 2, } @@ -54,6 +55,9 @@ func TestSaveThenLoadRoundTrips(t *testing.T) { if got.HealthChecksSince == nil || !got.HealthChecksSince.Equal(now) { t.Errorf("HealthChecksSince = %v, want %v", got.HealthChecksSince, now) } + if got.SelfAuditSince == nil || !got.SelfAuditSince.Equal(now) { + t.Errorf("SelfAuditSince = %v, want %v", got.SelfAuditSince, now) + } if got.LastSuccessTick == nil || !got.LastSuccessTick.Equal(now) { t.Errorf("LastSuccessTick = %v, want %v", got.LastSuccessTick, now) }