From 7962abcc146d60a6cc9f4b6b0811d05c2d64577a Mon Sep 17 00:00:00 2001 From: Tomas Hykel Date: Wed, 30 Sep 2026 18:00:34 +0200 Subject: [PATCH 1/9] feat: WP health check --- CLAUDE.md | 57 ++++- README.md | 2 +- TODO.md | 1 - bin/opilot | 2 + lib/opilot/agent.rb | 14 + lib/opilot/cli.rb | 1 + lib/opilot/clients/openproject.rb | 7 + lib/opilot/health_check.rb | 411 ++++++++++++++++++++++++++++++ lib/opilot/health_runner.rb | 35 +++ lib/opilot/helpers.rb | 51 ++++ lib/opilot/item_pictures.rb | 1 + lib/opilot/prompts.rb | 94 ++++++- lib/opilot/pull.rb | 40 ++- lib/opilot/ui.rb | 9 +- opilot | 2 +- test/opilot/agent_test.rb | 10 + test/opilot/cli_test.rb | 7 + test/opilot/health_check_test.rb | 341 +++++++++++++++++++++++++ test/opilot/item_pictures_test.rb | 6 + test/opilot/pull_test.rb | 30 +++ test/test_helper.rb | 2 + 21 files changed, 1109 insertions(+), 14 deletions(-) create mode 100644 lib/opilot/health_check.rb create mode 100644 lib/opilot/health_runner.rb create mode 100644 test/opilot/health_check_test.rb diff --git a/CLAUDE.md b/CLAUDE.md index cb03a3e..93fc6ab 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -44,9 +44,10 @@ every documented path goes through the script. ### op-agent -Polls work packages, driven by `@opilot` comments. There are two command words — -**`@opilot build`** (alias `fix`) and **`@opilot create wp`** (long form -`create work package`); every other word is chat. The words `build` replaced — +Polls work packages, driven by `@opilot` comments. There are three command words — +**`@opilot build`** (alias `fix`), **`@opilot create wp`** (long form +`create work package`) and **`@opilot health`** (see "Health check" below); every +other word is chat. The words `build` replaced — `ship`, `plan`, `approve`, `prototype`, `pr`, `implement` — are chat too, so an old habit gets an answer that names the real command rather than silence. `create` without the noun is chat as well: alone it could mean a branch, a PR or a comment, @@ -205,6 +206,9 @@ nothing" and "not scanning" look identical in the log. - **`dev commit ...`** — stop after the local commit. A later `ship` finds the branch (`branch_has_commits?`) and goes straight to publish. - **`dev plan ...`** — stop at the approved plan. +- **`dev health ...`** — the `@opilot health` check (`HealthRunner` → + `HealthCheck`), printed to the terminal; it posts nothing, so a prompt change can + be tuned on a real work package first. - **`dev refresh ...`** — refresh shipped PRs (`PrRunner`). A URL is matched against local state, else *adopted* via the OpenProject ticket link in the description's top 15 lines; a WP id with no state is *discovered* by searching each @@ -323,6 +327,7 @@ before touching anything under `lib/opilot/pd/`. ./opilot dev build ... ./opilot dev commit ... # stop after the local commit — no push, no PR ./opilot dev plan ... # stop at the approved plan +./opilot dev health ... # check the WP for drift; prints, posts nothing # Refresh shipped PRs: merge base, fix CI, address new comments, push (confirmed) ./opilot dev refresh ... @@ -490,6 +495,8 @@ bare `docker compose run …` works from the repo root. | `gh_pr_cache.rb` | PR-content cache (`pr.json`, keyed by `updated_at`), mention matching, fresh-comment filtering, CI cache (`ci.json`, keyed by head SHA) | | `gh_agent.rb` | `gh-agent` loop — own PRs: reply + code + push; upstream: read-only. `#sources` keeps the banner honest | | `fix_runner.rb` | Terminal `dev build`/`commit`/`plan` — one pipeline named by where it stops | +| `health_check.rb` | `@opilot health` and `dev health`: the fact rules, the one LLM call, the composed report | +| `health_runner.rb` | Terminal `dev health` — prints `HealthCheck`'s report, posts nothing | | `pr_runner.rb` | Terminal `dev refresh`, and gh-agent's `@opilot refresh` via `#refresh_one` | | `op_runner.rb` | Terminal `op` — one command per `Clients::OpenProject` method it exposes. Three rules hold: **stdout is data** (JSON only, diagnostics to stderr, never `log_script`), every action **reads except `wp create`**, and **`--type` is required of every payload**. `wp form --required` is how you learn what else a project demands. The file header argues all three — read it there rather than re-deriving them | | `harness.rb` | HTTP client to the harness container; per-WP session IDs | @@ -658,7 +665,7 @@ up front rather than after a full plan and implement run (`build`/`plan` need no `Helpers#require_clone!` is called from **`Helpers#worktree`**, the funnel every git operation goes through, because a per-command check gets forgotten — `./opilot` only *warns* when a clone fails, and `Git.open`'s error names neither the repo nor -the fix. `#ensure_claude!` fails with "start the container" at every entry point that +the fix. `#ensure_harness!` fails with "start the container" at every entry point that will call the LLM, not mid-run with a connection error. `:ship` (`@opilot build`, alias `fix`) is the fix intent: it plans and @@ -672,6 +679,40 @@ difference is who is watching: the terminal verbs have an operator at the consol Chat lenses (`grill`, `summarize`) are preset instructions over the ordinary `:chat` intent (`Prompts::LENSES`), with trailing text as a focus hint. +**Health check (`@opilot health [focus]`, intent `:health`)** reports where a work +package is inconsistent with itself. It is **not a lens**: it needs a fact pass, its +own prompt, a parser and a Ruby-composed reply (`HealthCheck`). Two layers: + +- **Facts** (`HealthCheck#facts_for`, no LLM) — only rules on exact, + language-independent data, because the prompt tells the model not to dispute them: + status meaning from `GET /statuses` (`isClosed`, `isDefault`, looked up by name — + names are unique), relation labels, timestamps, linked-PR flags + (`/work_packages/:id/github_pull_requests`, which needs `show_github_content`), + and commits naming the WP on each registry base (`git log --grep` as a prefilter, + then a word-boundary match, so `#5994` never matches `#59942`). PR and commit rules + fire only on the **default** status: "Developed" is open and has merged PRs. + Anything heuristic — a reopen report, which status-name matching would confuse + with a subject edit — goes to the model as `history[]` instead. +- **Descendants** (`HealthCheck#descendants`) — the whole subtree at any depth from + ONE paginated `ancestor` filter query, capped at `MAX_DESCENDANTS`, written to + `descendants.json`. A list element embeds nothing, so status and type come from + `_links..title`. The tree rules (closed over open at any depth, a subtree all + closed, stale open descendants) **replace** the direct-child rules; a failed read + falls back to them and says so. The model gets the tree to check the description's + scope against the descendants' subjects — no per-descendant LLM call. +- **Judgement** — one read-only call **without the WP session**, so a check is + independent of earlier chat turns. The answer is `Prompts::HEALTH_CONTRACT` + (`BEGIN HEALTH` … `END HEALTH`, `FINDING:`/`GAP:` lines), read by + `Helpers.parse_health`: the END marker detects truncation (one retry, then a + failure note), and a finding without evidence is dropped. + +The reply is composed in Ruby (`HealthCheck#report`), for `#post_options`' reason, +and always lists **Not checked** — skipped attachments, an unreadable PR list or +status list, the model's own gaps — because silence about an input reads as "it is +fine". A **public** reply drops any finding whose evidence names an internal +comment's timestamp: the evidence is printed verbatim, so the prompt rule alone +is not enough. Like `create wp`, the handler answers its own failure. + **A chat answer can carry an ARTIFACT — a diagram or a long report — published as a secret gist and linked from the comment.** Three surfaces offer one, and they are deliberately **asymmetric**: a gh-agent PR reply (`Prompts::MERMAID_NOTE` in @@ -852,12 +893,16 @@ globally unique, so `pr_reviews/` is flat. │ ├── item.json # WP metadata + poll cache + acted_at + item_version │ │ # + refusal_noted_at (the one allowlist note per WP) │ │ # + create_wp_refusal_noted_at (the one `create wp` off note) -│ │ # + pictures[] / pictures_skipped[] (see pictures/ below) +│ │ # + pictures[] (with created_at) / pictures_skipped[] +│ │ # + history[] (field changes, no comment text) and +│ │ # description_changed_at, for `health` │ │ # + pictures_pending (an attachment read failed — │ │ # suppresses the cache until a run finishes) │ ├── pictures/ # every picture the WP shows, mirrored so the LLM can `read` │ │ # one; -., pruned to match the WP │ ├── related.json # related WPs pulled in at plan time +│ ├── health.json # the last health check's facts (HealthCheck#facts_for) +│ ├── descendants.json # the subtree the last health check read (HealthCheck#descendants) │ ├── plan.md # implementation plan (shared across target repos) │ ├── artifacts// # markdown a chat answer produced, keyed by the trigger's │ │ # comment_at — the local copy of what was gisted @@ -946,7 +991,7 @@ of it, and a runner that gives up first turns a named timeout into a bare | Variable | Purpose | |----------|---------| | `OPENPROJECT_URL` | OpenProject instance URL | -| `OPENPROJECT_TOKEN` | API token. Read access suffices for `op`/`chat` (except `op wp create`); agent mode needs write (to comment), plus `:add_work_packages` once `@opilot create wp` is enabled, `:manage_work_package_relations` for its backlink and `:manage_subtasks` to make several of them children of the source (without either link permission the work packages are still created, only unlinked — or related instead of parented); `pd` needs `:add_work_packages` | +| `OPENPROJECT_TOKEN` | API token. Read access suffices for `op`/`chat` (except `op wp create`); agent mode needs write (to comment), plus `:add_work_packages` once `@opilot create wp` is enabled, `:manage_work_package_relations` for its backlink and `:manage_subtasks` to make several of them children of the source (without either link permission the work packages are still created, only unlinked — or related instead of parented); `pd` needs `:add_work_packages`. `health` reads linked PRs only with `:show_github_content` (without it, that input is listed as not checked) | | `HARNESS_URL` | Optional; where the runner reaches the harness container (default `http://harness:47291`) | | `OP_REPO_PATH` | Optional; local openproject checkout to seed that clone from. openproject-only — other repos are configured in `repos.json` | | `GITHUB_CONTRIBUTOR_TOKEN` | The **contributor identity** — a bot account that is **not a collaborator on the canonical repos** (that lack of access is what enforces isolation). Classic token with `public_repo`, `workflow` (the lagging fork re-introduces upstream's `.github/workflows/*`, rejected without it) and `gist` (the plan gist and chat artifacts; both skipped if absent). Fine-grained tokens can't open fork→upstream PRs | diff --git a/README.md b/README.md index b6aa638..b91f63a 100644 --- a/README.md +++ b/README.md @@ -35,7 +35,7 @@ Use OPilot to remove that friction from your development workflow. It can automa ### General audience -* **Refine work packages**: Discuss work packages in the chat via free-form chatting or preset commands like `@OPilot grill`. +* **Refine work packages**: Discuss work packages in the chat via free-form chatting or preset commands like `@OPilot grill`. `@OPilot health` checks the description against the comments, pictures, related work packages, status, linked PRs and commits, and lists what it could not check. * **Draw diagrams and write reports**: Ask for a diagram or a long report in the chat. OPilot publishes it as a secret gist and links it from the comment — a diagram is a mermaid fence, which the gist renders as a picture. Needs `OPILOT_ALLOWED_OP_USER_IDS`. * **Create work packages**: Turn a suggestion made in a comment into its own work package(s) with `@OPilot create wp `. OPilot builds the package content, creates them in the same project, and relates them back. * **[Enterprise] Run project-wide discovery**: Ask `@OPilot` anything about the reachable projects' data -- it will leverage the instance's MCP server to give you a fresh answer. diff --git a/TODO.md b/TODO.md index f79bc65..6563982 100644 --- a/TODO.md +++ b/TODO.md @@ -25,7 +25,6 @@ OPilot's roadmap. See [README.md](README.md) for what the project already does. * Inspiration: https://andrewpatterson.dev/posts/token-savings-rtk-headroom/ ## Feature ideas -* WP health check command * Replace OpenSpec with a simple list of acceptance criteria * Matrix/Element integration for a better interface & activity tracking * Nextcloud integration, so that we can load relevant data during designs diff --git a/bin/opilot b/bin/opilot index 2790819..78f7beb 100755 --- a/bin/opilot +++ b/bin/opilot @@ -21,6 +21,8 @@ require "opilot/gh_pull" require "opilot/gh_agent" require "opilot/combined_agent" require "opilot/fix_runner" +require "opilot/health_check" +require "opilot/health_runner" require "opilot/pr_runner" require "opilot/chat_runner" require "opilot/usage_runner" diff --git a/lib/opilot/agent.rb b/lib/opilot/agent.rb index 6ef47d6..dca3d30 100644 --- a/lib/opilot/agent.rb +++ b/lib/opilot/agent.rb @@ -96,6 +96,7 @@ def handle(intent) when :chat then handle_chat(intent) when :ship then handle_ship(intent) when :create_wp then handle_create_wp(intent) + when :health then handle_health(intent) end end @@ -137,6 +138,19 @@ def handle_chat(intent) post_note(st.item_id, addressed(reply.strip)) unless reply.strip.empty? end + # Answers its own failure, like create wp: the reader waits for a report. + def handle_health(intent) + check = HealthCheck.new(@ctx, pull: @pull, harness: @harness, api: @api) + report = begin + check.run(intent.item_id, focus: intent.text.to_s, internal: intent.internal != false) || + "The health check could not read this work package." + rescue Harness::Error => e + log_script "Health check failed on #{wp_label(intent.item_id)}: #{e.message}" + "The health check did not finish: the model run failed. Ask again with `@opilot health`." + end + post_note(intent.item_id, addressed(report)) + end + # Take the artifacts out of a chat answer, mirror them, publish them as one # gist, and return the comment to post — the answer without the blocks, plus a # line naming what was published. diff --git a/lib/opilot/cli.rb b/lib/opilot/cli.rb index 0005b54..0317efb 100644 --- a/lib/opilot/cli.rb +++ b/lib/opilot/cli.rb @@ -89,6 +89,7 @@ def dev(args) when "build", "fix" then with_ids("dev build", rest) { |ids| FixRunner.new(@ctx).ship_ids(*ids) } when "commit" then with_ids("dev commit", rest) { |ids| FixRunner.new(@ctx).commit_ids(*ids) } when "plan" then with_ids("dev plan", rest) { |ids| FixRunner.new(@ctx).plan_ids(*ids) } + when "health" then with_ids("dev health", rest) { |ids| HealthRunner.new(@ctx).run_ids(*ids) } when "refresh" then refresh(rest) # Reads .opilot/ only — no config, no network, no log header. when "status" then @ui.status diff --git a/lib/opilot/clients/openproject.rb b/lib/opilot/clients/openproject.rb index 4b494d4..9ca4512 100644 --- a/lib/opilot/clients/openproject.rb +++ b/lib/opilot/clients/openproject.rb @@ -94,6 +94,13 @@ def work_package_emoji_reactions(wp_id) HTTP.get_json("#{@base}/api/v3/work_packages/#{wp_id}/activities_emoji_reactions", token: @token) end + # PRs the GitHub integration linked to a work package. Needs + # :show_github_content and the project's `github` module, so a 403 or 404 + # is a normal answer. `merged` is a boolean; `state` is open/closed/deployed. + def work_package_github_pull_requests(wp_id) + HTTP.get_json("#{@base}/api/v3/work_packages/#{wp_id}/github_pull_requests", token: @token) + end + def me HTTP.get_json("#{@base}/api/v3/users/me", token: @token) end diff --git a/lib/opilot/health_check.rb b/lib/opilot/health_check.rb new file mode 100644 index 0000000..eedb396 --- /dev/null +++ b/lib/opilot/health_check.rb @@ -0,0 +1,411 @@ +require "json" +require "time" + +module OPilot + # The work package health check, shared by `@opilot health` (Agent) and + # `./opilot dev health` (HealthRunner). Two layers: + # + # - Facts: rules on exact, language-independent data (status flags from + # /statuses, relation labels, timestamps, PR flags, commit ids). Written to + # health.json, and the prompt tells the model not to dispute them — so a + # heuristic never belongs here. + # - Judgement: one read-only LLM call, answered in Prompts::HEALTH_CONTRACT. + # + # The reply is composed here, not by the model, so it states what was checked. + class HealthCheck + include Helpers + + STALE_DAYS = 60 + MAX_COMMITS = 10 + MAX_DESCENDANTS = 200 + MAX_TREE_FINDINGS = 5 # per rule; the rest are counted in the text + SEVERITY_ORDER = Prompts::HEALTH_SEVERITIES + + # Labels from this work package's own side (Pull#relation_pairs): the other + # one must finish first. + PREREQUISITES = %w[blocked follows requires].freeze + + def initialize(ctx, pull:, harness:, api: nil) + @ctx = ctx + @pull = pull + @harness = harness + @api = api || Clients::OpenProject.new(ctx.op_url, ctx.token) + end + + # The report text, or nil when the work package cannot be fetched. + # `internal:` is the visibility the report is posted with. + def run(item_id, focus: "", internal: true) + item = @pull.fetch_single_item(item_id) + return nil unless item + + st = state_for(item["id"], item["subject"], item["type"]) + related_path = related_ref(st) + related = related_path ? (Helpers.safe_json_read(st.related_file) || []) : [] + + tree = descendants(item["id"]) + tree_file = st.item_dir / "descendants.json" + tree_file.write(JSON.pretty_generate(tree["nodes"])) if tree["nodes"]&.any? + tree_ref = if tree["nodes"].nil? then nil + elsif tree["nodes"].empty? then :none + else container_path(tree_file) + end + + facts = facts_for(item, related, status_map, linked_prs(item["id"]), commits(item["id"]), tree: tree) + facts["inputs"]["opilot_user_href"] = opilot_user_href + facts["inputs"]["opilot_prs"] = st.repos.filter_map { |r| st.pr_url_file(r).read.strip if st.pr_url_file(r).exist? } + facts_file = st.item_dir / "health.json" + facts_file.write(JSON.pretty_generate(facts)) + + prompt = Prompts.health(item_id: st.item_id, subject: st.subject, + item: container_path(st.item_file), facts: container_path(facts_file), + related: related_path, descendants: tree_ref, focus: focus, internal: internal, + op_mcp: @ctx.op_mcp?) + answer = ask(prompt) + return failed_note unless answer + + report(facts, answer, item, internal: internal) + end + + # ── facts ──────────────────────────────────────────────────────────────── + + # name => { "closed", "default" }, or nil when /statuses cannot be read. + # Status names are unique on an instance, so the lookup by name is exact. + def status_map + _code, body = @api.statuses + (body&.dig("_embedded", "elements") || []).to_h do |s| + [s["name"].to_s, { "closed" => s["isClosed"] == true, "default" => s["isDefault"] == true }] + end + rescue StandardError => e + log_script "Health: could not read statuses (#{e.message})" + nil + end + + # Every descendant at any depth, from one paginated `ancestor` query: + # { "nodes" => [...] | nil (not read), "truncated", "code" }. Each node is + # { id, parent, depth, subject, type, status, updated_at }, ids as displayed. + def descendants(item_id) + code, wp = @api.work_package(item_id) + return { "nodes" => nil, "truncated" => false, "code" => code } unless code == 200 && wp + root = wp["id"].to_s + filter = Clients::OpenProject.filter("ancestor", "=", root) + raw = [] + total = 0 + (1..).each do |page| + code, resp = @api.work_packages(filters_json: filter, page: page, page_size: 100, sort_by: '[["id","asc"]]') + return { "nodes" => nil, "truncated" => false, "code" => code } unless code == 200 && resp + total = resp["total"].to_i + elements = resp.dig("_embedded", "elements") || [] + raw.concat(elements) + break if elements.empty? || raw.length >= total || raw.length >= MAX_DESCENDANTS + end + { "nodes" => tree_nodes(raw.first(MAX_DESCENDANTS), root, Helpers.display_id(wp)), + "truncated" => total > MAX_DESCENDANTS, "code" => 200 } + end + + private def tree_nodes(raw, root_numeric, root_display) + shown = raw.to_h { |w| [w["id"].to_s, Helpers.display_id(w)] }.merge(root_numeric => root_display) + parent_of = raw.to_h { |w| [w["id"].to_s, w.dig("_links", "parent", "href").to_s.split("/").last.to_s] } + depth = lambda do |id, seen = 0| + up = parent_of[id] + up.nil? || up == root_numeric || seen > MAX_DESCENDANTS ? 1 : 1 + depth.(up, seen + 1) + end + raw.map do |w| + id = w["id"].to_s + { "id" => shown[id], "parent" => shown[parent_of[id]] || parent_of[id], "depth" => depth.(id), + "subject" => w["subject"], "type" => link_title(w, "type"), + "status" => link_title(w, "status"), "updated_at" => w["updatedAt"] } + end + end + + # A list element embeds nothing; the name is the link's title. + private def link_title(wp, key) + wp.dig("_links", key, "title") || wp.dig("_embedded", key, "name") + end + + # So the model can tell opilot's own comments from the thread. + private def opilot_user_href + id = @pull.own_user_id if @pull.respond_to?(:own_user_id) + id.to_s.empty? ? nil : "/api/v3/users/#{id}" + rescue StandardError + nil + end + + # [prs, code]. The GitHub integration answers 403/404 when it is off. + def linked_prs(item_id) + code, body = @api.work_package_github_pull_requests(item_id) + return [nil, code] unless code == 200 + prs = (body&.dig("_embedded", "elements") || []).map do |pr| + { "url" => pr["htmlUrl"], "repository" => pr["repository"], "number" => pr["number"], + "title" => pr["title"], "state" => pr["state"], "merged" => pr["merged"] == true, + "merged_at" => pr["mergedAt"], "draft" => pr["draft"] == true } + end + [prs, code] + end + + # PR numbers share the `#N` form with work package ids ("Merge pull request + # #25183", a squash's "(#25183)"), so they are removed before matching. + PR_NUMBER = /Merge pull request #\d+|\(#\d+\)/ + + # How a commit names a work package: the id in a branch name + # (`bug/op-123-slug`, `bug/59942-slug`), a work package URL, `[#N]`, `OP#N`. + # Never a prefix of a longer id. + def self.commit_pattern(item_id) + id = Regexp.escape(item_id.to_s) + return /(? [{ "sha", "subject" }] } for commits on each registry base + # that name the work package, or { repo_name => nil } when a clone cannot + # be read. + def commits(item_id) + exact = HealthCheck.commit_pattern(item_id) + @ctx.repos.all.to_h do |repo| + sync_base!(repo) + found = worktree(repo).log(500).object("origin/#{repo.base}").grep(HealthCheck.commit_prefilter(item_id)).execute + hits = found.select { |c| c.message.to_s.gsub(PR_NUMBER, "").match?(exact) }.first(MAX_COMMITS) + [repo.name, hits.map { |c| { "sha" => c.sha[0, 12], "subject" => c.message.to_s.lines.first.to_s.strip } }] + rescue StandardError => e + log_script "Health: could not read commits in #{repo.name} (#{e.message})" + [repo.name, nil] + end + end + + # Pure: every input already fetched, so the rules test without HTTP or git. + # `tree` is #descendants' answer; nil (not asked) keeps the direct-child + # rules on related.json, which is what a failed subtree read falls back to. + def facts_for(item, related, statuses, prs_and_code, commits, tree: nil, now: Time.now) + prs, pr_code = prs_and_code + findings = [] + not_checked = [] + nodes = tree&.dig("nodes") + if tree && nodes.nil? + not_checked << "The descendants could not be read (HTTP #{tree["code"]}); only direct children were checked." + elsif tree&.dig("truncated") + not_checked << "The subtree has more than #{MAX_DESCENDANTS} work packages; only the first #{MAX_DESCENDANTS} were checked." + end + # Children are part of the tree, so the tree rules replace the child rules. + related = related.reject { |r| r["relation"] == "child" } if nodes + + own = statuses && statuses[item["status"].to_s] + if own.nil? + not_checked << (statuses ? "The status \"#{item["status"]}\" is not in the status list, so no status rule ran." : + "The status list could not be read, so no status rule ran.") + else + findings.concat(relation_findings(item, related, statuses, own)) + findings.concat(tree_findings(nodes, statuses, own, now)) if nodes&.any? + findings.concat(pr_findings(item, prs, own)) if prs + findings.concat(commit_findings(item, commits, own)) + stale = item["history"].nil? ? nil : stale_finding(item, own, now) + findings << stale if stale + end + findings.concat(design_findings(item)) + if item["history"].nil? + not_checked << "The activities could not be read: comments and field changes are missing, " \ + "and the staleness and design rules did not run." + end + + Array(item["pictures_skipped"]).each do |p| + not_checked << "Attachment \"#{p["name"]}\" (#{p["where"] || "attached"}): #{p["reason"]}." + end + not_checked << "Linked pull requests could not be read (HTTP #{pr_code})." unless prs + commits.each { |repo, list| not_checked << "Commits in #{repo} could not be read." if list.nil? } + + { "findings" => findings, "not_checked" => not_checked, + "inputs" => { "status" => item["status"], "status_closed" => own&.dig("closed"), + "status_default" => own&.dig("default"), "pull_requests" => prs, + "commits" => commits, "descendant_count" => nodes&.length } } + end + + # Rules across the whole subtree. A node whose status is not in the list + # is neither open nor closed, so no rule uses it. + private def tree_findings(nodes, statuses, own, now) + closed = ->(n) { statuses.dig(n["status"].to_s, "closed") } + open_nodes = nodes.select { |n| closed.(n) == false } + ids = ->(list) { list.first(MAX_TREE_FINDINGS).map { |n| wp_label(n["id"]) }.join(", ") + (list.length > MAX_TREE_FINDINGS ? ", …" : "") } + out = [] + + if own["closed"] && open_nodes.any? + out << fact("high", "relations", "This work package is closed, but #{open_nodes.length} descendant(s) are still open.", ids.(open_nodes)) + elsif !own["closed"] && nodes.all? { |n| closed.(n) == true } + out << fact("low", "status", "All #{nodes.length} descendants are closed, but this work package is still open.", ids.(nodes)) + end + + # A closed node inside the tree with an open node under it. + by_id = nodes.to_h { |n| [n["id"], n] } + ancestors = lambda do |n| + chain = [] + while (up = by_id[n["parent"]]) && chain.length <= nodes.length + chain << up + n = up + end + chain + end + closed_over_open = open_nodes.flat_map { |n| ancestors.(n).select { |a| closed.(a) == true } }.uniq + closed_over_open.first(MAX_TREE_FINDINGS).each do |a| + out << fact("medium", "relations", "Descendant #{wp_label(a["id"])} is closed, but a work package under it is still open.", wp_label(a["id"])) + end + + stale = open_nodes.select { |n| (t = parse_time(n["updated_at"])) && (now - t) > STALE_DAYS * 86_400 } + if stale.any? + out << fact("low", "status", "#{stale.length} open descendant(s) did not change for #{STALE_DAYS} days.", ids.(stale)) + end + out + end + + private def relation_findings(item, related, statuses, own) + id = ->(r) { wp_label(r["id"]) } + closed = ->(r) { statuses.dig(r["status"].to_s, "closed") } + children = related.select { |r| r["relation"] == "child" } + out = [] + + if own["closed"] + related.each do |r| + next unless closed.(r) == false + if PREREQUISITES.include?(r["relation"]) + out << fact("medium", "relations", "This work package is closed, but #{id.(r)} (#{r["relation"]}) is still open.", id.(r)) + elsif r["relation"] == "child" + out << fact("high", "relations", "This work package is closed, but its child #{id.(r)} is still open.", id.(r)) + end + end + else + if children.any? && children.all? { |r| closed.(r) == true } + out << fact("low", "status", "All children are closed, but this work package is still open.", + children.map(&id).join(", ")) + end + related.select { |r| r["relation"] == "duplicates" && closed.(r) == true }.each do |r| + out << fact("medium", "relations", "This work package duplicates #{id.(r)}, which is closed, but it is still open.", id.(r)) + end + end + out + end + + # Only the DEFAULT status (the one a new work package starts in) is a clear + # contradiction: "Developed" or "In testing" is open and has merged PRs. + private def pr_findings(_item, prs, own) + out = [] + prs.each do |pr| + if pr["merged"] && own["default"] + out << fact("high", "prs", "A linked pull request is merged, but the status is still the initial one.", pr["url"]) + elsif pr["state"] == "open" && !pr["draft"] && own["closed"] + out << fact("medium", "prs", "This work package is closed, but a linked pull request is still open.", pr["url"]) + end + end + out + end + + private def commit_findings(_item, commits, own) + return [] unless own["default"] + hits = commits.flat_map { |repo, list| Array(list).map { |c| "#{repo}@#{c["sha"]}" } } + return [] if hits.empty? + [fact("low", "status", "Commits name this work package, but the status is still the initial one.", hits.first(3).join(", "))] + end + + private def stale_finding(item, own, now) + return nil if own["closed"] + times = [item["created_at"], *Array(item["comments"]).map { |c| c["created_at"] }, + *Array(item["history"]).map { |h| h["created_at"] }] + last = times.filter_map { |t| parse_time(t) }.max + return nil unless last && (now - last) > STALE_DAYS * 86_400 + fact("low", "status", "This work package is open, but nothing changed for #{((now - last) / 86_400).floor} days.", + "last activity #{last.utc.iso8601}") + end + + # A picture in the description arrives with that same edit, so only one in a + # comment or attached on its own can be newer than the text. + private def design_findings(item) + edited = parse_time(item["description_changed_at"]) + return [] unless edited + Array(item["pictures"]).filter_map do |p| + next if p["where"] == "description" + added = parse_time(p["created_at"]) + next unless added && added > edited + fact("medium", "designs", "The picture \"#{p["name"]}\" was added after the last description edit.", + "#{p["name"]}, #{added.utc.iso8601} (#{p["where"]})") + end + end + + private def fact(severity, area, text, evidence) + { "severity" => severity, "area" => area, "text" => text, "evidence" => evidence.to_s } + end + + private def parse_time(value) + value.to_s.empty? ? nil : Time.parse(value.to_s) + rescue ArgumentError + nil + end + + # ── judgement ──────────────────────────────────────────────────────────── + + RETRY_NOTE = "\n\nYour last answer had no complete BEGIN HEALTH … END HEALTH block. " \ + "Answer again, and end with that block exactly as described." + + private def ask(prompt) + answer = Helpers.parse_health(@harness.run(prompt, tools: read_tools)) + answer || Helpers.parse_health(@harness.run(prompt + RETRY_NOTE, tools: read_tools)) + end + + private def failed_note + "The health check did not finish: the answer had no report that I could read. " \ + "Ask again with `@opilot health`." + end + + # ── report ─────────────────────────────────────────────────────────────── + + # The whole reply. A public reply drops a finding whose evidence cites an + # internal comment: the evidence is printed verbatim. + def report(facts, answer, item, internal: true) + findings = facts["findings"] + answer["findings"] + hidden = 0 + unless internal + stamps = Array(item["comments"]).select { |c| c["internal"] }.filter_map { |c| c["created_at"].to_s[0, 16] } + stamps.reject!(&:empty?) + kept = findings.reject { |f| stamps.any? { |s| f["evidence"].include?(s) } } + hidden = findings.length - kept.length + findings = kept + end + findings = findings.sort_by.with_index { |f, i| [SEVERITY_ORDER.index(f["severity"]), i] } + + gaps = answer["gaps"].reject { |g| already_not_checked?(g, facts["not_checked"]) } + not_checked = facts["not_checked"] + gaps.map { |g| g["why"].empty? ? "#{g["what"]}." : "#{g["what"]}: #{g["why"]}" } + not_checked << "#{hidden} finding(s) cite an internal comment and are not shown in this public reply." if hidden.positive? + + lines = [summary_line(findings)] + SEVERITY_ORDER.each do |sev| + group = findings.select { |f| f["severity"] == sev } + next if group.empty? + lines << "" << "**#{sev.capitalize}**" + group.each { |f| lines << "- #{f["text"]} (#{f["evidence"]})" } + end + unless not_checked.empty? + lines << "" << "**Not checked**" + not_checked.each { |n| lines << "- #{n}" } + end + lines.join("\n") + end + + # The prompt forbids a GAP that repeats not_checked, and a real run wrote one + # anyway. A gap is a repeat when a fact line holds every word of its subject. + private def already_not_checked?(gap, lines) + words = gap["what"].to_s.downcase.scan(/[[:alnum:]]{4,}/) + return false if words.empty? + lines.any? { |line| words.all? { |w| line.downcase.include?(w) } } + end + + private def summary_line(findings) + return "**Health check: no findings.**" if findings.empty? + counts = SEVERITY_ORDER.filter_map do |sev| + n = findings.count { |f| f["severity"] == sev } + "#{n} #{sev}" if n.positive? + end + noun = findings.length == 1 ? "finding" : "findings" + "**Health check: #{findings.length} #{noun}** (#{counts.join(", ")})" + end + end +end diff --git a/lib/opilot/health_runner.rb b/lib/opilot/health_runner.rb new file mode 100644 index 0000000..43d05ab --- /dev/null +++ b/lib/opilot/health_runner.rb @@ -0,0 +1,35 @@ +module OPilot + # The terminal `dev health`: run HealthCheck on each id and print the report. + # It posts nothing, so a prompt change can be tuned on a real work package. + class HealthRunner + include Helpers + + def initialize(ctx, pull: Pull.new(ctx), harness: Harness.new(ctx), api: nil) + @ctx = ctx + @pull = pull + @harness = harness + @check = HealthCheck.new(ctx, pull: pull, harness: harness, api: api) + end + + # One failure does not stop the rest; with a single id it is fatal. + def run_ids(*wp_ids) + ensure_harness! + report_mcp_status + wp_ids.each do |wp_id| + log_script "Checking work package #{wp_label(wp_id)}…" + report = @check.run(wp_id) + unless report + msg = "could not fetch work package #{wp_label(wp_id)} — check the id and OPENPROJECT_TOKEN" + raise OPilot::FatalError, msg if wp_ids.length == 1 + log_script "#{wp_label(wp_id)} — #{msg}" + next + end + puts "" + puts render_markdown(report) + rescue Harness::Error => e + raise OPilot::FatalError, "The LLM run failed: #{e.message}" if wp_ids.length == 1 + log_script "#{wp_label(wp_id)} — The LLM run failed: #{e.message}" + end + end + end +end diff --git a/lib/opilot/helpers.rb b/lib/opilot/helpers.rb index acbd93c..325bdde 100644 --- a/lib/opilot/helpers.rb +++ b/lib/opilot/helpers.rb @@ -277,6 +277,57 @@ def self.work_package_fields(lines) end private_class_method :work_package_fields + HEALTH_BEGIN = /\A[ \t]*BEGIN HEALTH[ \t]*\z/ + HEALTH_END = /\A[ \t]*END HEALTH[ \t]*\z/ + HEALTH_FINDING_LINE = /\A[ \t]*FINDING:[ \t]*(.+)\z/i + HEALTH_GAP_LINE = /\A[ \t]*GAP:[ \t]*(.+)\z/i + HEALTH_CLEAN_LINE = /\A[ \t]*NO FINDINGS[ \t.]*\z/i + + # Read a health answer (Prompts::HEALTH_CONTRACT) into + # { "findings" => [...], "gaps" => [...] }, or nil when there is no complete + # block — the answer was cut off, or ignored the format. The LAST complete + # block wins, so a format the writer rehearsed first does not count. A + # malformed line drops only itself; so does a finding with no evidence. + def self.parse_health(body) + block = nil + open = nil + fence = nil + body.to_s.lines.each do |raw| + line = raw.chomp + fence = fence_state(fence, line) + next unless fence.nil? + + if line.match?(HEALTH_BEGIN) then open = [] + elsif line.match?(HEALTH_END) + block = open if open + open = nil + else open&.<<(line) + end + end + return nil unless block + + findings = block.filter_map { |l| health_finding(l[HEALTH_FINDING_LINE, 1]) } + gaps = block.filter_map do |l| + what, why = l[HEALTH_GAP_LINE, 1]&.split("|", 2)&.map(&:strip) + { "what" => what, "why" => why.to_s } unless what.to_s.empty? + end + readable = findings.any? || gaps.any? || + block.any? { |l| l.match?(HEALTH_CLEAN_LINE) || l.match?(HEALTH_FINDING_LINE) } + return nil unless readable + { "findings" => findings.first(Prompts::HEALTH_MAX_FINDINGS), "gaps" => gaps } + end + + def self.health_finding(text) + return nil unless text + severity, area, sentence, evidence = text.split("|", 4).map { |f| f.to_s.strip } + severity = severity.downcase + area = area.to_s.downcase + return nil unless Prompts::HEALTH_SEVERITIES.include?(severity) && Prompts::HEALTH_AREAS.include?(area) + return nil if sentence.to_s.empty? || evidence.to_s.empty? + { "severity" => severity, "area" => area, "text" => sentence, "evidence" => evidence } + end + private_class_method :health_finding + ARTIFACT_BEGIN = /\A[ \t]*BEGIN ARTIFACT[ \t]*\z/ ARTIFACT_END = /\A[ \t]*END ARTIFACT[ \t]*\z/ ARTIFACT_FILENAME_LINE = /\AFILENAME:[ \t]*(.+)\z/i diff --git a/lib/opilot/item_pictures.rb b/lib/opilot/item_pictures.rb index d3c0dbc..078cae1 100644 --- a/lib/opilot/item_pictures.rb +++ b/lib/opilot/item_pictures.rb @@ -185,6 +185,7 @@ def write_one(want, dir, api:, ctx:, budget:) [{ "id" => want["id"], "where" => want["where"], "name" => name, "content_type" => meta["contentType"], "bytes" => dest.size, + "created_at" => meta["createdAt"], "file" => Helpers.state_container_path(ctx, dest) }, false] end diff --git a/lib/opilot/prompts.rb b/lib/opilot/prompts.rb index fd8510e..3264f1d 100644 --- a/lib/opilot/prompts.rb +++ b/lib/opilot/prompts.rb @@ -531,7 +531,8 @@ def self.chat(item_id:, subject:, item:, plan:, message:, related: nil, can_crea Once the pull request exists, changes to the code are asked for **on the pull request**, not here — say so instead of promising a change on this work package. - @opilot grill [focus] — stress-test the ticket/plan: gaps, edge cases, risks, open questions - - @opilot summarize [focus] — recap the thread: state, decisions, open questions#{create_wp_line(can_create_wp)} + - @opilot summarize [focus] — recap the thread: state, decisions, open questions + - @opilot health [focus] — check the ticket for drift: description vs comments, designs, related WPs, status, PRs#{create_wp_line(can_create_wp)} USER: #{message} @@ -545,6 +546,97 @@ def self.chat(item_id:, subject:, item:, plan:, message:, related: nil, can_crea PROMPT end + # The health check's answer shape (Helpers.parse_health). The END marker + # detects a cut-off answer; the evidence field is what keeps a finding from + # being an opinion, so the parser drops a finding without it. + HEALTH_SEVERITIES = %w[high medium low].freeze + HEALTH_AREAS = %w[comments designs relations status prs].freeze + HEALTH_MAX_FINDINGS = 15 + HEALTH_CONTRACT = <<~TEXT.strip + ANSWER FORMAT — think first if you need to, then end your response with exactly + this block. I parse it, so keep one item on one line: + + BEGIN HEALTH + FINDING: <#{HEALTH_SEVERITIES.join("|")}> | <#{HEALTH_AREAS.join("|")}> | | + GAP: | + END HEALTH + + - Evidence is REQUIRED. Name the comment (author and created_at), the work + package (#id), the picture file name, the pull request URL, or the commit sha. + A finding without evidence is dropped. + - Write NO FINDINGS alone on a line inside the block when nothing is wrong. + - Write at most #{HEALTH_MAX_FINDINGS} findings, the most severe first. + TEXT + + # One-shot health check of a work package (read-only tools, no session). + # `facts` is the container path to health.json: the rules the runner already + # checked from exact data, so the model neither repeats nor disputes them. + # `descendants` is the container path to descendants.json, :none for a work + # package without children, or nil when the subtree could not be read. + def self.health(item_id:, subject:, item:, facts:, related: nil, descendants: nil, focus: "", + internal: true, op_mcp: false) + focus_line = focus.to_s.strip.empty? ? "" : "\nFOCUS: look especially at: #{focus.strip}\n" + audience = internal ? "an internal comment" : "a PUBLIC comment — do not cite or quote an internal comment" + # related_line omits the field when empty, which reads as "not loaded". + related_text = related.to_s.empty? ? "\nRELATED: none — this work package has no relations, parent, or children." : related_line(related) + tree_text, tree_check = + case descendants + when nil then ["", ""] + when :none then ["\nDESCENDANTS: none — this work package has no children.", ""] + else + ["\nDESCENDANTS: #{descendants} (JSON array — every work package under this one, at any " \ + "depth: id, parent, depth, subject, type, status. The runner already checked the " \ + "statuses across the tree.)", + "\n5. The description against the descendants. A requirement in the description that\n" \ + " no descendant covers, a descendant outside the scope of the description, or two\n" \ + " descendants that do the same work. Use the subjects; open a descendant with\n" \ + " op_query only when its subject is not enough to decide."] + end + <<~PROMPT + You are opilot. Check the health of OpenProject work package #{Helpers.wp_label(item_id)}: #{subject} + #{READ_ONLY} + + ISSUE: #{item} #{item_fields("type", "status", "history[]", "description_changed_at")}#{related_text}#{tree_text}#{op_query_line(op_mcp)} + history[] holds the field changes (status, assignee, description, …) as the + instance renders them. description_changed_at is the time of the last + description edit. + FACTS: #{facts} (JSON — `findings` the runner already established from exact + data, `not_checked`, and `inputs`: linked pull requests and commits. Do NOT + repeat a fact finding and do NOT dispute it. Use `inputs` as evidence.) + #{focus_line} + Find where this work package is not consistent with itself. Check: + 1. The description against the comments. A decision, a scope change, or a new + acceptance criterion in a comment that the description does not show. A + comment that contradicts the description. A question nobody answered. + Reactions on a comment (a 👍 from the assignee) are a sign of agreement. + 2. The description against the pictures. `read` every entry in pictures[]. + A mockup that shows a field, a label, or a flow that the text does not + mention, or the opposite. When you cannot see a picture, write a GAP. + 3. The description against the related work packages. Overlapping scope, or + a related work package that already did part of this work. + 4. The status against history[] and the comments. For example, a comment + after the work package closed that reports the problem again.#{tree_check} + + opilot is the tool that runs this check. Comments by `inputs.opilot_user_href`, + and comments that give opilot a command (`@opilot build`, …), are tool traffic, + not requirements: do not report on them. A pull request in `inputs.opilot_prs` + is a draft prototype. It does not change the status, so a status that ignores + it is correct. + + Report only what the evidence shows. Do not report style, wording, or a + missing detail that no comment asks for. Do not propose a fix. Do not write + a GAP for an item that is already in `not_checked`. Report one problem ONE + time, even when two checks show it: use the area that shows its cause, and + put all the evidence in that one finding. + + This report is posted as #{audience}. + + #{HEALTH_CONTRACT} + + #{PLAIN_ENGLISH} + PROMPT + end + # How a chat answer hands over a diagram or a long report. Present only when # artifacts are available, for create_wp_line's reason — and so the off path # pays none of these tokens. diff --git a/lib/opilot/pull.rb b/lib/opilot/pull.rb index e1490da..a29502c 100644 --- a/lib/opilot/pull.rb +++ b/lib/opilot/pull.rb @@ -331,6 +331,10 @@ def parse_command(raw) # the whitespace, so "create wp" arrives normalised. when /\A@opilot\s+create\s+(?:wp|work\s+package)\b\s*(.*)/im [:create_wp, $1.strip] + # Not a lens: it needs a fact pass, its own prompt and a composed reply + # (HealthCheck). Trailing text is a focus hint, as for a lens. + when /\A@opilot\s+health\b\s*(.*)/im + [:health, $1.strip] # Chat lenses: a preset instruction over the ordinary chat path, with any # trailing text folded in as a focus hint (see Prompts::LENSES). when /\A@opilot\s+(grill|summarize)\b\s*(.*)/im then [:chat, Prompts.lens($1, $2)] @@ -378,8 +382,9 @@ def wp_display_id(wp) # item.json's shape. The updated_at cache below would otherwise keep a work # package opilot has already seen on the old shape forever — which is how a - # mirror gains a field (this is 2 because "pictures" was added). - ITEM_VERSION = 2 + # mirror gains a field (3: "history", "description_changed_at", and each + # picture's "created_at", for the health check). + ITEM_VERSION = 3 # A work package is served from cache only when the mirror is COMPLETE. # `pictures_pending` says an attachment read failed, and updated_at cannot @@ -413,6 +418,10 @@ def fetch_work_package_item(wp) ) full = build_full_item(wp, comments) + # nil, not empty, when the read failed: "no changes" would be a false fact. + activities = acts.dig("_embedded", "elements") || [] + full["history"] = acts_code == 200 ? build_history(activities) : nil + full["description_changed_at"] = acts_code == 200 ? description_changed_at(activities, wp) : nil if item_path.exist? prev = Helpers.safe_json_read(item_path) || {} (CARRIED_KEYS + PICTURE_KEYS).each { |key| full[key] = prev[key] if prev.key?(key) } @@ -448,6 +457,31 @@ def build_comments(activities, reactions) end end + # Field changes (status, assignee, description, …), which build_comments + # drops. `changes` are the instance's own rendered sentences, so they are + # language-dependent: input for the LLM, never for a Ruby rule. + def build_history(activities) + activities.filter_map do |a| + changes = Array(a["details"]).map { |d| d["raw"].to_s.strip }.reject(&:empty?) + next if changes.empty? + { "id" => a["id"].to_s, + "user" => a.dig("_embedded", "user", "name") || a.dig("_links", "user", "title"), + "created_at" => a["createdAt"], "changes" => changes } + end + end + + # When the description last changed, or the creation time if it never did. + # The detail links to the journals diff route + # (`/journals//diff/description`), which is language-independent. + DESCRIPTION_DIFF = %r{/diff/description\b} + + def description_changed_at(activities, wp) + edits = activities.select do |a| + Array(a["details"]).any? { |d| "#{d["raw"]} #{d["html"]}".match?(DESCRIPTION_DIFF) } + end + edits.map { |a| a["createdAt"].to_s }.max || wp["createdAt"] + end + def build_full_item(wp, comments) { "id" => wp_display_id(wp), @@ -565,6 +599,8 @@ def own_user def own_user_id; own_user["id"]; end def bot_display_name; own_user["name"]; end + # The health check tells the model which comments are opilot's own. + public :own_user_id def mark_opilot_acted(wp_id, created_at) item_path = Helpers.item_dir(@ctx, wp_id) / "item.json" diff --git a/lib/opilot/ui.rb b/lib/opilot/ui.rb index 91e4f77..2a6fd61 100644 --- a/lib/opilot/ui.rb +++ b/lib/opilot/ui.rb @@ -109,7 +109,7 @@ def agent_usage def triggers <<~TRIGGERS.strip Triggers — on a work package: @opilot build | create wp | grill | - summarize, or anything else to just talk. + summarize | health, or just talk. build offers numbered options when a fix has more than one shape; reply `build ` to build one (one alias: fix). create wp @@ -137,6 +137,11 @@ def dev_commands Same, then open a draft PR from the bot's fork; picks up a branch an earlier commit left behind. (`dev fix` is an alias.) + ./opilot dev health ... + Check a work package for drift: description against comments, + pictures, related work packages, status, linked PRs and commits. + Prints the report and posts nothing. Same as `@opilot health`. + ./opilot dev refresh ... Refresh a shipped PR: merge the base branch in, fix failing CI, address new review comments, push (with confirmation). Same thing @@ -327,7 +332,7 @@ def usage #{indent(triggers, 2)} Terminal: - ./opilot dev software development: plan, commit, build, refresh, status + ./opilot dev software development: plan, commit, build, health, refresh, status ./opilot pd product development: the spec-driven pipeline ./opilot op read the OpenProject API directly (JSON out) ./opilot appsignal turn a production error into a work package and a PR diff --git a/opilot b/opilot index 6be7c12..2b096ff 100755 --- a/opilot +++ b/opilot @@ -257,7 +257,7 @@ export GIT_COMMITTER_NAME="$GIT_AUTHOR_NAME" GIT_COMMITTER_EMAIL="$GIT_AUTHOR_EM _needs_harness() { _is agent op-agent gh-agent chat && return 0 case "$CMD" in - dev) case "$SUB" in build|fix|commit|plan|refresh) return 0 ;; esac ;; + dev) case "$SUB" in build|fix|commit|plan|health|refresh) return 0 ;; esac ;; pd) case "$SUB" in propose|implement) return 0 ;; esac ;; # `fix` is appsignal's only verb, so a bare incident number reads as one # (`./opilot appsignal 2025`). Matched by EXCLUSION for that reason: a list diff --git a/test/opilot/agent_test.rb b/test/opilot/agent_test.rb index 00cd36a..bc62002 100644 --- a/test/opilot/agent_test.rb +++ b/test/opilot/agent_test.rb @@ -1270,6 +1270,16 @@ def artifact_dir(slug = "2024-02-01t00-00-00z") @ctx.state_dir / "work_packages" / "op.example.com" / "42" / "artifacts" / slug end + # The report itself is HealthCheckTest's; this is the routing, and the + # answer to a work package the check cannot read — never silence. + def test_health_answers_even_when_the_work_package_cannot_be_read + @pull.define_singleton_method(:fetch_single_item) { |_id| nil } + @agent.handle(intent(:health, text: "", user: "Ana", user_href: "/api/v3/users/5", internal: false)) + assert_equal 1, @notes.length + assert_includes @notes.first, "could not read this work package" + assert_equal [false], @note_visibility, "a public trigger gets a public answer" + end + # The guard on the surface that already worked: an ordinary answer must be # posted exactly as it is, artifacts on or off. def test_chat_without_an_artifact_posts_the_answer_unchanged diff --git a/test/opilot/cli_test.rb b/test/opilot/cli_test.rb index f478009..609b370 100644 --- a/test/opilot/cli_test.rb +++ b/test/opilot/cli_test.rb @@ -115,6 +115,13 @@ def test_build_and_fix_both_reach_the_publishing_path end end + def test_dev_health_reaches_the_runner + capture_io do + error = assert_raises(OPilot::FatalError) { CLI.new(ctx_double).run(%w[dev health 42]) } + assert_match(/Config not found/, error.message) + end + end + def test_a_bare_op_is_a_help_request_and_needs_no_config out, = capture_io { CLI.new(ctx_double).run(["op"]) } assert_includes out, "Usage: ./opilot op " diff --git a/test/opilot/health_check_test.rb b/test/opilot/health_check_test.rb new file mode 100644 index 0000000..221046d --- /dev/null +++ b/test/opilot/health_check_test.rb @@ -0,0 +1,341 @@ +require_relative "../test_helper" + +module OPilot + class HealthParseTest < Minitest::Test + def block(*lines) + "Thinking first.\nBEGIN HEALTH\n#{lines.join("\n")}\nEND HEALTH\n" + end + + def test_reads_findings_and_gaps + answer = Helpers.parse_health(block( + "FINDING: high | comments | The scope grew. | Ana, 2026-09-01T10:00:00Z", + "GAP: mockup.png | the model cannot see pictures" + )) + assert_equal [{ "severity" => "high", "area" => "comments", "text" => "The scope grew.", + "evidence" => "Ana, 2026-09-01T10:00:00Z" }], answer["findings"] + assert_equal [{ "what" => "mockup.png", "why" => "the model cannot see pictures" }], answer["gaps"] + end + + def test_no_findings_is_a_readable_clean_answer + assert_equal({ "findings" => [], "gaps" => [] }, Helpers.parse_health(block("NO FINDINGS"))) + end + + def test_a_cut_off_answer_is_nil + assert_nil Helpers.parse_health("BEGIN HEALTH\nFINDING: high | comments | x | y\n") + assert_nil Helpers.parse_health("No block at all.") + end + + def test_a_finding_without_evidence_or_with_an_unknown_area_is_dropped + answer = Helpers.parse_health(block( + "FINDING: high | comments | No evidence here. |", + "FINDING: high | vibes | Unknown area. | #42", + "FINDING: LOW | Status | Kept. | #42" + )) + assert_equal ["Kept."], answer["findings"].map { |f| f["text"] } + assert_equal "low", answer["findings"].first["severity"] + end + + def test_markers_inside_a_fence_are_text + body = "```\nBEGIN HEALTH\nFINDING: high | comments | quoted | #1\nEND HEALTH\n```\n" + assert_nil Helpers.parse_health(body) + end + + def test_the_last_complete_block_wins + body = block("FINDING: low | status | rehearsal | #1") + block("NO FINDINGS") + assert_equal [], Helpers.parse_health(body)["findings"] + end + + def test_findings_are_capped + lines = Array.new(20) { |i| "FINDING: low | status | Finding #{i}. | ##{i}" } + assert_equal Prompts::HEALTH_MAX_FINDINGS, Helpers.parse_health(block(*lines))["findings"].length + end + end + + class HealthCheckTest < Minitest::Test + include TestFixtures + + STATUSES = { + "New" => { "closed" => false, "default" => true }, + "Developed" => { "closed" => false, "default" => false }, + "Closed" => { "closed" => true, "default" => false } + }.freeze + NOW = Time.utc(2026, 9, 30) + + def setup + @tmpdir = Dir.mktmpdir + @ctx = build_ctx(@tmpdir) + @check = HealthCheck.new(@ctx, pull: nil, harness: nil, api: Object.new) + end + + def teardown + FileUtils.rm_rf(@tmpdir) + super + end + + def item(**over) + { "id" => "42", "status" => "New", "created_at" => "2026-09-01T00:00:00Z", + "description_changed_at" => "2026-09-01T00:00:00Z", "comments" => [], "history" => [] } + .merge(over.transform_keys(&:to_s)) + end + + def facts(it, related: [], statuses: STATUSES, prs: [[], 200], commits: {}, tree: nil) + @check.facts_for(it, related, statuses, prs, commits, tree: tree, now: NOW) + end + + def texts(f) = f["findings"].map { |x| x["text"] } + + def test_a_closed_wp_with_an_open_child_or_blocker_is_a_finding + related = [{ "id" => "7", "relation" => "child", "status" => "New" }, + { "id" => "8", "relation" => "blocked", "status" => "Developed" }, + { "id" => "9", "relation" => "relates", "status" => "New" }] + f = facts(item(status: "Closed"), related: related) + assert_equal ["This work package is closed, but its child #7 is still open.", + "This work package is closed, but #8 (blocked) is still open."], texts(f) + end + + def test_an_open_wp_whose_children_all_closed_is_a_finding + related = [{ "id" => "7", "relation" => "child", "status" => "Closed" }] + assert_equal ["All children are closed, but this work package is still open."], texts(facts(item, related: related)) + end + + def test_a_merged_pr_counts_only_on_the_default_status + prs = [[{ "url" => "https://github.com/o/r/pull/1", "merged" => true, "state" => "closed" }], 200] + assert_equal 1, facts(item(status: "New"), prs: prs)["findings"].length + assert_empty facts(item(status: "Developed"), prs: prs)["findings"], "Developed is open and has merged PRs" + end + + def test_an_unreadable_pr_list_is_not_checked + f = facts(item, prs: [nil, 403]) + assert_includes f["not_checked"], "Linked pull requests could not be read (HTTP 403)." + end + + def test_commits_count_only_on_the_default_status + commits = { "openproject" => [{ "sha" => "abc", "subject" => "[#42] Fix" }] } + assert_equal ["Commits name this work package, but the status is still the initial one."], + texts(facts(item, commits: commits)) + assert_empty facts(item(status: "Developed"), commits: commits)["findings"] + end + + def test_an_unreadable_status_list_skips_the_status_rules + f = facts(item(status: "Closed"), related: [{ "id" => "7", "relation" => "child", "status" => "New" }], statuses: nil) + assert_empty f["findings"] + assert_includes f["not_checked"], "The status list could not be read, so no status rule ran." + end + + def test_an_open_quiet_wp_is_stale + f = facts(item(created_at: "2026-06-01T00:00:00Z")) + assert_match(/nothing changed for 121 days/, texts(f).first) + end + + def test_a_picture_added_after_the_description_is_a_finding_unless_it_is_in_the_description + pictures = [{ "name" => "new.png", "where" => "comment 5", "created_at" => "2026-09-10T00:00:00Z" }, + { "name" => "inline.png", "where" => "description", "created_at" => "2026-09-10T00:00:00Z" }, + { "name" => "old.png", "where" => "attached", "created_at" => "2026-08-01T00:00:00Z" }] + assert_equal ["The picture \"new.png\" was added after the last description edit."], + texts(facts(item(pictures: pictures))) + end + + def test_unread_activities_run_no_timestamp_rule + pictures = [{ "name" => "new.png", "where" => "attached", "created_at" => "2026-09-10T00:00:00Z" }] + f = facts(item(history: nil, description_changed_at: nil, created_at: "2026-01-01T00:00:00Z", pictures: pictures)) + assert_empty f["findings"], "neither stale nor a design finding from missing data" + assert(f["not_checked"].any? { |n| n.start_with?("The activities could not be read") }) + end + + # ── descendants ───────────────────────────────────────────────────────── + + def node(id, parent, status, updated_at: "2026-09-20T00:00:00Z") + { "id" => id, "parent" => parent, "status" => status, "updated_at" => updated_at } + end + + def tree(*nodes) = { "nodes" => nodes, "truncated" => false, "code" => 200 } + + def test_a_closed_wp_with_an_open_grandchild_is_a_finding + t = tree(node("7", "42", "Closed"), node("8", "7", "New")) + f = facts(item(status: "Closed"), tree: t) + assert_equal ["This work package is closed, but 1 descendant(s) are still open.", + "Descendant #7 is closed, but a work package under it is still open."], texts(f) + end + + def test_the_tree_replaces_the_direct_child_rule + related = [{ "id" => "7", "relation" => "child", "status" => "New" }] + f = facts(item(status: "Closed"), related: related, tree: tree(node("7", "42", "New"))) + assert_equal 1, f["findings"].length, "one finding for #7, not one from each rule" + end + + def test_an_open_wp_whose_whole_subtree_is_closed_is_a_finding + t = tree(node("7", "42", "Closed"), node("8", "7", "Closed")) + assert_equal ["All 2 descendants are closed, but this work package is still open."], texts(facts(item, tree: t)) + end + + def test_open_descendants_that_did_not_change_are_counted + t = tree(node("7", "42", "Developed", updated_at: "2026-01-01T00:00:00Z"), node("8", "42", "New")) + assert_equal ["1 open descendant(s) did not change for 60 days."], texts(facts(item, tree: t)) + end + + def test_an_unreadable_or_cut_subtree_is_not_checked + f = facts(item, tree: { "nodes" => nil, "truncated" => false, "code" => 403 }) + assert(f["not_checked"].any? { |n| n.start_with?("The descendants could not be read (HTTP 403)") }) + f = facts(item, tree: tree(node("7", "42", "New")).merge("truncated" => true)) + assert(f["not_checked"].any? { |n| n.include?("more than #{HealthCheck::MAX_DESCENDANTS}") }) + end + + def test_descendants_pages_through_the_ancestor_filter + json = { "Content-Type" => "application/json" } + stub_request(:get, %r{/api/v3/work_packages/42\z}).to_return( + status: 200, headers: json, body: { "id" => 42, "displayId" => "TT-42" }.to_json + ) + el = ->(id, parent) { { "id" => id, "displayId" => "TT-#{id}", "subject" => "S#{id}", + "_links" => { "parent" => { "href" => "/api/v3/work_packages/#{parent}" }, + "status" => { "href" => "/api/v3/statuses/1", "title" => "New" } } } } + stub_request(:get, %r{/api/v3/work_packages\?.*ancestor.*offset=1}).to_return( + status: 200, headers: json, body: { "total" => 2, "_embedded" => { "elements" => [el.(7, 42)] } }.to_json + ) + stub_request(:get, %r{/api/v3/work_packages\?.*ancestor.*offset=2}).to_return( + status: 200, headers: json, body: { "total" => 2, "_embedded" => { "elements" => [el.(8, 7)] } }.to_json + ) + check = HealthCheck.new(@ctx, pull: nil, harness: nil) + nodes = check.descendants("42")["nodes"] + assert_equal [["TT-7", "TT-42", 1, "New"], ["TT-8", "TT-7", 2, "New"]], + nodes.map { |n| n.values_at("id", "parent", "depth", "status") } + end + + def test_skipped_attachments_are_not_checked + f = facts(item(pictures_skipped: [{ "name" => "flow.svg", "where" => "attached", "reason" => "not a picture (image/svg+xml)" }])) + assert_includes f["not_checked"], "Attachment \"flow.svg\" (attached): not a picture (image/svg+xml)." + end + + def test_commit_filter_does_not_match_a_longer_id + wt = Class.new do + def log(*) = self + def object(*) = self + def grep(*) = self + def execute + [TestFixtures::FakeCommit.new(sha: "a" * 40, message: "[#59942] Other"), + TestFixtures::FakeCommit.new(sha: "b" * 40, message: "[#5994] This one")] + end + end.new + @check.instance_variable_set(:@worktrees, Hash.new { |h, k| h[k] = wt }) + @check.define_singleton_method(:sync_base!) { |_repo| true } + found = @check.commits("5994") + assert_equal [["b" * 12]], found.values.map { |list| list.map { |c| c["sha"] } } + end + + def test_commit_pattern_reads_the_forms_a_work_package_is_named_in + numeric = HealthCheck.commit_pattern("5994") + ["[#5994] Fix", "Refs OP#5994", "Merge pull request #1 from opf/bug/5994-login", + "See https://community.openproject.org/wp/5994"].each { |m| assert_match numeric, m } + ["[#59942] Other", "bug/59942-x", "ᝪ"].each { |m| refute_match numeric, m } + assert_empty "Merge pull request #5994 from opf/x (#5994)".gsub(HealthCheck::PR_NUMBER, "").scan(numeric) + + semantic = HealthCheck.commit_pattern("COMMS-123") + assert_match semantic, "Merge pull request #9 from opf/bug/comms-123-toast" + refute_match semantic, "bug/comms-1234-x" + refute_match semantic, "xcomms-123" + assert_equal "[cC][oO][mM][mM][sS]-123", HealthCheck.commit_prefilter("COMMS-123") + end + + def test_report_orders_by_severity_and_lists_what_was_not_checked + f = { "findings" => [{ "severity" => "low", "area" => "status", "text" => "Stale.", "evidence" => "x" }], + "not_checked" => ["Attachment \"a.svg\": not a picture."] } + answer = { "findings" => [{ "severity" => "high", "area" => "comments", "text" => "Scope grew.", "evidence" => "Ana" }], + "gaps" => [{ "what" => "b.png", "why" => "unreadable" }] } + text = @check.report(f, answer, item) + assert_equal <<~TEXT.strip, text + **Health check: 2 findings** (1 high, 1 low) + + **High** + - Scope grew. (Ana) + + **Low** + - Stale. (x) + + **Not checked** + - Attachment "a.svg": not a picture. + - b.png: unreadable + TEXT + end + + # From a real run: the model repeated a fact line as a GAP despite the prompt. + def test_a_gap_that_repeats_a_not_checked_line_is_dropped + f = { "findings" => [], "not_checked" => ["Linked pull requests could not be read (HTTP 403)."] } + answer = { "findings" => [], "gaps" => [ + { "what" => "Linked pull requests", "why" => "The runner already lists this in not_checked (HTTP 403)." }, + { "what" => "mockup.png", "why" => "unreadable" } + ] } + text = @check.report(f, answer, item) + assert_equal 1, text.scan("Linked pull requests").length + assert_includes text, "- mockup.png: unreadable" + end + + def test_a_public_report_hides_a_finding_that_cites_an_internal_comment + it = item(comments: [{ "created_at" => "2026-09-02T08:15:00Z", "internal" => true }]) + answer = { "findings" => [{ "severity" => "high", "area" => "comments", "text" => "Secret.", + "evidence" => "Bo, 2026-09-02T08:15" }], "gaps" => [] } + text = @check.report({ "findings" => [], "not_checked" => [] }, answer, it, internal: false) + refute_includes text, "Secret." + assert_includes text, "1 finding(s) cite an internal comment" + assert_includes @check.report({ "findings" => [], "not_checked" => [] }, answer, it, internal: true), "Secret." + end + + # ── the whole run, with fakes at every edge ───────────────────────────── + + class FakePull + def initialize(ctx, item); @ctx = ctx; @item = item; end + def fetch_single_item(_id) + dir = Helpers.item_dir(@ctx, @item["id"]) + dir.mkpath + (dir / "item.json").write(JSON.generate(@item)) + @item + end + def related_work_packages(_id); []; end + end + + class ScriptedHarness + attr_reader :prompts + def initialize(*answers); @answers = answers; @prompts = []; end + def run(prompt, tools: nil, **) + @prompts << [prompt, tools] + @answers[@prompts.length - 1] || @answers.last + end + end + + def run_check(harness) + stub_request(:get, %r{/api/v3/statuses}).to_return( + status: 200, headers: { "Content-Type" => "application/json" }, + body: { "_embedded" => { "elements" => [{ "name" => "New", "isClosed" => false, "isDefault" => true }] } }.to_json + ) + stub_request(:get, %r{/work_packages/42/github_pull_requests}).to_return(status: 403, body: "{}") + stub_request(:get, %r{/api/v3/work_packages/42\z}).to_return( + status: 200, headers: { "Content-Type" => "application/json" }, body: { "id" => 42 }.to_json + ) + stub_request(:get, %r{/api/v3/work_packages\?.*ancestor}).to_return( + status: 200, headers: { "Content-Type" => "application/json" }, + body: { "total" => 0, "_embedded" => { "elements" => [] } }.to_json + ) + check = HealthCheck.new(@ctx, pull: FakePull.new(@ctx, item(subject: "Login")), harness: harness) + check.define_singleton_method(:commits) { |_id| {} } + check.run("42", focus: "the toast") + end + + def test_run_writes_the_facts_and_posts_the_composed_report + harness = ScriptedHarness.new("BEGIN HEALTH\nNO FINDINGS\nEND HEALTH") + text = run_check(harness) + assert_match(/\A\*\*Health check: no findings\.\*\*/, text) + assert_includes text, "Linked pull requests could not be read (HTTP 403)." + prompt, tools = harness.prompts.first + assert_includes prompt, "/health.json" + assert_includes prompt, "look especially at: the toast" + assert_includes prompt, "RELATED: none", "no relations must not read as 'not loaded'" + assert_includes prompt, "DESCENDANTS: none" + assert_equal Harness::TOOLS_READ, tools.split(",").first(5).join(",") + assert (Helpers.item_dir(@ctx, "42") / "health.json").exist? + end + + def test_run_retries_once_then_reports_the_failure + harness = ScriptedHarness.new("garbage", "still garbage") + assert_match(/did not finish/, run_check(harness)) + assert_equal 2, harness.prompts.length + end + end +end diff --git a/test/opilot/item_pictures_test.rb b/test/opilot/item_pictures_test.rb index 87624d0..f24a991 100644 --- a/test/opilot/item_pictures_test.rb +++ b/test/opilot/item_pictures_test.rb @@ -85,6 +85,12 @@ def test_a_picture_in_the_description_is_written_next_to_the_item assert_equal "\x89PNG-data".b, (@dir / "pictures" / "7-shot.png").binread end + def test_a_picture_records_when_it_was_attached + api = FakeOP.new(attached: [attachment("7").merge("createdAt" => "2026-09-10T00:00:00Z")]) + out = mirror(item(description: "![](#{ref(7)})"), api) + assert_equal "2026-09-10T00:00:00Z", out["pictures"].first["created_at"] + end + def test_the_inline_reference_is_rewritten_to_the_local_file # The half that makes a picture readable rather than merely present: the # harness has no egress, so the URL is dead either way. diff --git a/test/opilot/pull_test.rb b/test/opilot/pull_test.rb index ed642fd..b6d8228 100644 --- a/test/opilot/pull_test.rb +++ b/test/opilot/pull_test.rb @@ -180,6 +180,36 @@ def test_parse_command_create_wp_is_case_insensitive_with_mention_markup assert_equal [:create_wp, "for Rosanna"], @pull.send(:parse_command, "#{MENTION} Create WP for Rosanna") end + def test_parse_command_health_is_its_own_intent_with_a_focus + assert_equal [:health, "the toast"], @pull.send(:parse_command, "#{MENTION} Health the toast") + assert_equal [:health, ""], @pull.send(:parse_command, "@opilot health") + end + + # ── field-change history ────────────────────────────────────────────────── + + ACTIVITIES = [ + { "id" => 1, "createdAt" => "2024-01-03T00:00:00Z", "comment" => { "raw" => "hi" }, "details" => [] }, + { "id" => 2, "createdAt" => "2024-01-04T00:00:00Z", "comment" => { "raw" => "" }, + "_embedded" => { "user" => { "name" => "Ana" } }, + "details" => [{ "raw" => "Beschreibung geändert (/journals/9/diff/description)", "html" => "" }, + { "raw" => "Status changed from New to Developed", "html" => "" }] }, + { "id" => 3, "createdAt" => "2024-01-05T00:00:00Z", "comment" => { "raw" => "" }, + "details" => [{ "raw" => "Assignee set to Bo", "html" => "" }] } + ].freeze + + def test_history_keeps_field_changes_that_carry_no_comment + history = @pull.send(:build_history, ACTIVITIES) + assert_equal %w[2 3], history.map { |h| h["id"] } + assert_equal "Ana", history.first["user"] + assert_equal 2, history.first["changes"].length + end + + def test_description_changed_at_follows_the_diff_link_in_any_language + assert_equal "2024-01-04T00:00:00Z", @pull.send(:description_changed_at, ACTIVITIES, WP) + assert_equal WP["createdAt"], @pull.send(:description_changed_at, ACTIVITIES.last(1), WP), + "never edited = the creation time" + end + # ── chat lenses ─────────────────────────────────────────────────────────── def test_parse_command_grill_is_a_chat_with_the_lens_instruction diff --git a/test/test_helper.rb b/test/test_helper.rb index 5327462..7767847 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -21,6 +21,8 @@ require "opilot/gh_agent" require "opilot/combined_agent" require "opilot/fix_runner" +require "opilot/health_check" +require "opilot/health_runner" require "opilot/pr_runner" require "opilot/chat_runner" require "opilot/usage_runner" From 90839c02db44aa1f192c6bbe0f1e0b6a9b3e8ab6 Mon Sep 17 00:00:00 2001 From: Tomas Hykel Date: Wed, 30 Sep 2026 18:11:48 +0200 Subject: [PATCH 2/9] refactor: give every LLM call a named role Every harness call now goes through Helpers#llm with a named role (lib/opilot/roles.rb): base grant, whether the MCP tools join it, model, and whether it is stateless. No behaviour change: each role reproduces the exact (grant, model, session) tuple its call sites sent. A stateless role (auditor, triager, scribe) raises when given a session file, so health's independence from chat is now structural. The model plumbing through FixRunner, #implement_plan and #generate_pr_description is gone; the role carries the model. Divergences kept as-is, now visible in one table: - pr_author (gh-agent own PR) gets the MCP tools; pr_refresher (dev refresh) does not, for a similar job. - wp_writer (create wp) gets no MCP tools, unlike advisor and planner. - pr_advisor (upstream PRs) gets no MCP tools. Guard tests: every role x flag combination resolves to a grant in server.js's ALLOWED_TOOL_GRANTS, and no lib/ file outside helpers, harness and roles calls @harness.run/capture or names Harness::TOOLS_*. Co-Authored-By: Claude Opus 5.5 --- CLAUDE.md | 14 ++++-- lib/opilot/agent.rb | 10 ++--- lib/opilot/appsignal_runner.rb | 2 +- lib/opilot/chat_runner.rb | 2 +- lib/opilot/fix_runner.rb | 41 ++++++++--------- lib/opilot/gh_agent.rb | 4 +- lib/opilot/harness.rb | 8 ++-- lib/opilot/health_check.rb | 4 +- lib/opilot/helpers.rb | 38 +++++++--------- lib/opilot/pd/runner.rb | 20 +++++---- lib/opilot/pr_runner.rb | 2 +- lib/opilot/roles.rb | 30 +++++++++++++ test/opilot/roles_test.rb | 81 ++++++++++++++++++++++++++++++++++ 13 files changed, 183 insertions(+), 73 deletions(-) create mode 100644 lib/opilot/roles.rb create mode 100644 test/opilot/roles_test.rb diff --git a/CLAUDE.md b/CLAUDE.md index 93fc6ab..4e0ae81 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -500,6 +500,7 @@ bare `docker compose run …` works from the repo root. | `pr_runner.rb` | Terminal `dev refresh`, and gh-agent's `@opilot refresh` via `#refresh_one` | | `op_runner.rb` | Terminal `op` — one command per `Clients::OpenProject` method it exposes. Three rules hold: **stdout is data** (JSON only, diagnostics to stderr, never `log_script`), every action **reads except `wp create`**, and **`--type` is required of every payload**. `wp form --required` is how you learn what else a project demands. The file header argues all three — read it there rather than re-deriving them | | `harness.rb` | HTTP client to the harness container; per-WP session IDs | +| `roles.rb` | The roles the model plays (grant, model, stateless) — every LLM call names one via `Helpers#llm` | | `appsignal_runner.rb` | Terminal `appsignal` — incident → work package, then hands off to `FixRunner#ship_ids`. Owns the local-model guard, and every preflight runs before the create | | `clients/appsignal.rb` | AppSignal's GraphQL + V2 tracing APIs, assembled into one incident: metadata, the request payload, and the backtrace. The runner's client, never a tool for the model | | `clients/inference_gw.rb` | inference-gw's `GET /upstream` — the pinned inference address, which is what `Context#inference_privacy` judges | @@ -949,11 +950,18 @@ Runner POSTs to `http://harness:47291` with headers: `test/js/models_json_test.js` makes adding one deliberate. The base is `"read,grep,find,ls,bash"` (planning/chat) or `"read,grep,find,ls,bash,write,edit"` (implementation), each with a `,op_query` - variant sent by the specific call sites `Helpers#read_tools`/`#impl_tools` - cover when `Context#op_mcp?` is on (see `MCP.md`) — most `TOOLS_READ`/`TOOLS_IMPL` - call sites keep the plain grant regardless. `server.js` rejects any other + variant sent only by the roles marked `mcp` when `Context#op_mcp?` is on + (see `MCP.md`) — the other roles keep the plain grant regardless. `server.js` rejects any other grant, so its allowlist (`ALLOWED_TOOL_GRANTS`, four strings) must stay in sync with `TOOLS_READ`/`TOOLS_IMPL`/`TOOLS_READ_OP`/`TOOLS_IMPL_OP`. +- **Every call names a role** (`Helpers#llm`, table in `lib/opilot/roles.rb`): + a role's base grant, whether the MCP tools join it, its model, and whether it is + **stateless** (a stateless role given a `session_file` raises, so health's + independence from chat is structural). Nothing outside `helpers.rb`/`harness.rb` + calls `@harness.run` or names `Harness::TOOLS_*` — `test/opilot/personas_test.rb` + checks that, and checks every role against `ALLOWED_TOOL_GRANTS`. Roles that + look alike but differ in grant (`pr_author` vs `pr_refresher`) stay two + roles until someone decides to merge them. - `X-Harness-Model` — one model per WP for every session-bound phase (`MODEL_HEAVY`), plus `MODEL_LIGHT` for stateless one-shots (a commit subject, a PR description) — always `/` (`openrouter/anthropic/claude-sonnet-5.5`, diff --git a/lib/opilot/agent.rb b/lib/opilot/agent.rb index dca3d30..a7b9fea 100644 --- a/lib/opilot/agent.rb +++ b/lib/opilot/agent.rb @@ -130,7 +130,7 @@ def handle_chat(intent) related: related_ref(st), can_create_wp: create_wp_enabled?, can_make_artifact: artifacts_enabled?, max_artifacts: MAX_ARTIFACTS, op_mcp: @ctx.op_mcp?) - reply = @harness.run(prompt, tools: read_tools, session_file: st.session_file) + reply = llm(:advisor, prompt, session_file: st.session_file) # Only when artifacts are on: with the instructions never given, a BEGIN # ARTIFACT line is text the writer invented or quoted, and stripping it # would delete content from someone's reply. @@ -391,7 +391,7 @@ def write_work_packages(st, request, project_name, types, related, retry_bad: tr item: container_path(st.item_file), request: request, project: project_name, types: Helpers.types_for_prompt(types), max: MAX_CREATE_WP, related: related, format_note: format_note) - reply = @harness.run(prompt, tools: Harness::TOOLS_READ, session_file: st.session_file).to_s + reply = llm(:wp_writer, prompt, session_file: st.session_file).to_s # Only what follows the last `ANSWER:` marker; the writer's own deliberation # is scratch (Prompts.create_wp). Text with no marker is read whole, so an # answer that skips it still works. @@ -885,8 +885,7 @@ def produce_plan(st, feedback, allow_options: false, retry_bad_options: true) prompt = Prompts.replan(repos_summary: @ctx.repos.summary, repos: menu, item: item_c, plan: plan_c, feedback: feedback, item_id: st.item_id, title: st.subject, resumed: session_resumable?(st), related: related, op_mcp: @ctx.op_mcp?) - @harness.capture(prompt, tools: read_tools, outfile: st.plan_file, - session_file: st.session_file) + llm(:planner, prompt, outfile: st.plan_file, session_file: st.session_file) record_chosen_repos(st) return :ok end @@ -895,8 +894,7 @@ def produce_plan(st, feedback, allow_options: false, retry_bad_options: true) prompt = Prompts.plan(repos_summary: @ctx.repos.summary, repos: menu, item: item_c, item_id: st.item_id, title: st.subject, hint: feedback.to_s, related: related, allow_options: allow_options, op_mcp: @ctx.op_mcp?) - @harness.capture(prompt, tools: read_tools, outfile: st.plan_file, - session_file: st.session_file) + llm(:planner, prompt, outfile: st.plan_file, session_file: st.session_file) if st.plan_file.read.lstrip.start_with?("NEEDS_INFO") questions = st.plan_file.read.sub(/\A\s*NEEDS_INFO\s*\n?/, "").strip diff --git a/lib/opilot/appsignal_runner.rb b/lib/opilot/appsignal_runner.rb index ce3a1c0..d458a52 100644 --- a/lib/opilot/appsignal_runner.rb +++ b/lib/opilot/appsignal_runner.rb @@ -201,7 +201,7 @@ def write_work_package(number, incident_file, retry_bad: true, format_note: nil) incident: container_path(incident_file), number: number, app: @app, repos: repos_for_prompt(@ctx.repos.all), types: Helpers.types_for_prompt(project_types), format_note: format_note ) - reply = @harness.run(prompt, tools: read_tools, model: Harness::MODEL_HEAVY).to_s + reply = llm(:triager, prompt).to_s answer = Helpers.after_marker(reply, "ANSWER") if answer.lstrip.start_with?("NEEDS_INFO") diff --git a/lib/opilot/chat_runner.rb b/lib/opilot/chat_runner.rb index e0a2128..0203487 100644 --- a/lib/opilot/chat_runner.rb +++ b/lib/opilot/chat_runner.rb @@ -58,7 +58,7 @@ def run(initial_message = nil) Prompts.free_chat(state: @ctx.state_container, wp_root: wp_root, repos: repos, message: pending, op_mcp: @ctx.op_mcp?, gh_mcp: @ctx.gh_mcp?) end - @harness.run(prompt, tools: read_tools, session_file: session_file) + llm(:advisor, prompt, session_file: session_file) # Set only after the run returns: a failed turn never reached the model, # so the next one still has to orient it. oriented = true diff --git a/lib/opilot/fix_runner.rb b/lib/opilot/fix_runner.rb index c5b6bad..1e3f3f4 100644 --- a/lib/opilot/fix_runner.rb +++ b/lib/opilot/fix_runner.rb @@ -95,11 +95,6 @@ def process_item(item_data, mode:) # One guard for all three verbs — `plan` stops here too, which the old # per-verb check inside #commit/#ship could not do. return if report_already_shipped(st) - # One model for every session-bound phase of this WP (plan, chat, replan, - # implement) — switching mid-session would drop the context. The PR - # description is a separate, stateless call and picks its own model. - model = Harness::MODEL_HEAVY - replan_feedback = nil # Set once the operator picks an option (or types their own direction) at # the options prompt below. Present means the approach is settled, so the @@ -122,13 +117,14 @@ def process_item(item_data, mode:) begin # Read-only across all repos; the LLM re-declares the target repo(s) in # the revised plan. Branch checkout waits until #ship. - @harness.capture( + llm( + :planner, Prompts.replan(repos_summary: @ctx.repos.summary, repos: repos_for_prompt(@ctx.repos.all), item: container_path(st.item_file), plan: container_path(st.plan_file), feedback: replan_feedback, item_id: id, title: subject, resumed: session_resumable?(st), related: related_ref(st), op_mcp: @ctx.op_mcp?), - tools: read_tools, model: model, outfile: st.plan_file, session_file: st.session_file + outfile: st.plan_file, session_file: st.session_file ) record_chosen_repos(st) rescue Harness::Error @@ -141,12 +137,13 @@ def process_item(item_data, mode:) log_script "Planning #{wp_label(id)} — #{subject}" begin # Pass session_file so a prior chat's context carries into the (re-)plan. - @harness.capture( + llm( + :planner, Prompts.plan(repos_summary: @ctx.repos.summary, repos: repos_for_prompt(@ctx.repos.all), item: container_path(st.item_file), item_id: id, title: subject, hint: option_focus.to_s, related: related_ref(st), allow_options: option_focus.nil?, op_mcp: @ctx.op_mcp?), - tools: read_tools, model: model, outfile: st.plan_file, session_file: st.session_file + outfile: st.plan_file, session_file: st.session_file ) record_chosen_repos(st) if plan_present?(st) rescue Harness::Error @@ -194,7 +191,7 @@ def process_item(item_data, mode:) puts " ⚠ Plan generation failed — no plan came back." case prompt_plan_failed(id) when :retry then next # plan.md is gone, so the loop regenerates it - when :chat then run_chat(st, model); next + when :chat then run_chat(st); next when :skip then log_script "#{wp_label(id)} skipped."; break when :drop then log_script "#{wp_label(id)} dropped."; break end @@ -211,7 +208,7 @@ def process_item(item_data, mode:) puts "" case prompt_needs_info(id) when :chat - run_chat(st, model) + run_chat(st) next # retry planning — chat session carries context forward when :skip log_script "#{wp_label(id)} skipped (needs info)." @@ -238,11 +235,11 @@ def process_item(item_data, mode:) # `build`/`ship` picks it up. log_script "#{wp_label(id)} — plan approved; build or ship it later with " \ "`dev commit #{id}` / `dev build #{id}`." - when :commit then commit(st, model) - when :ship then ship(st, model) + when :commit then commit(st) + when :ship then ship(st) end break - when :chat then run_chat(st, model) + when :chat then run_chat(st) when :replan then replan_feedback = prompt_replan_feedback when :skip then log_script "#{wp_label(id)} skipped."; break when :drop then safe_rm(st.plan_file); log_script "#{wp_label(id)} dropped."; break @@ -317,7 +314,7 @@ def prompt_replan_feedback msg.empty? ? "Revise the plan to incorporate the changes requested in the preceding conversation." : msg end - def run_chat(st, model = Harness::MODEL_HEAVY) + def run_chat(st) # The session already holds the plan (just generated/revised), so pass its # path as a fallback rather than re-embedding the full text on every turn. plan_ref = st.plan_file.exist? ? container_path(st.plan_file) : "(no plan yet)" @@ -340,7 +337,7 @@ def run_chat(st, model = Harness::MODEL_HEAVY) item: container_path(st.item_file), plan: plan_ref, message: msg ) end - @harness.run(prompt, tools: read_tools, model: model, session_file: st.session_file) + llm(:advisor, prompt, session_file: st.session_file) oriented = true # Ring after the reply, not before the first message: the user just # chose [c]hat and is present; it's the LLM's answers they wander off on. @@ -351,8 +348,8 @@ def run_chat(st, model = Harness::MODEL_HEAVY) # Helpers#implement_plan plus the console's own report of a no-op plan (the # agent answers that on the work package instead). - def implement(st, model = Harness::MODEL_HEAVY) - changed = implement_plan(st, model: model) + def implement(st) + changed = implement_plan(st) if changed.empty? log_script "#{wp_label(st.item_id)} — no changes produced." puts " ⚠ No changes produced — plan may be a no-op or already applied." @@ -363,15 +360,15 @@ def implement(st, model = Harness::MODEL_HEAVY) # `commit`: implement and commit, then stop — nothing is pushed and no PR is # opened. The committed branch sits in the local clone for review; a later # `dev build ` finds it via branch_has_commits? and goes straight to publish. - def commit(st, model = Harness::MODEL_HEAVY) - implement(st, model).each do |repo| + def commit(st) + implement(st).each do |repo| record_progress(st.item_id, st.branch, "built:#{repo.name}") puts " ✓ Committed #{st.branch} (#{repo.name}) — review it in the clone, then ship it with `./opilot dev build #{st.item_id}`" end end - def ship(st, model = Harness::MODEL_HEAVY) - implement(st, model).each do |repo| + def ship(st) + implement(st).each do |repo| generate_pr_description(st, repo) url = @publish.open_pr(st.item_id, st.subject, st.branch, repo) if url diff --git a/lib/opilot/gh_agent.rb b/lib/opilot/gh_agent.rb index 564a9af..6e72e64 100644 --- a/lib/opilot/gh_agent.rb +++ b/lib/opilot/gh_agent.rb @@ -150,7 +150,7 @@ def handle_review(intent) comment_id: intent.comment_id, in_reply_to: intent.in_reply_to, ci: review_ci_ref(ci_file, intent.head_sha) ) - reply = @harness.run(prompt, tools: Harness::TOOLS_READ, session_file: session_file) + reply = llm(:pr_advisor, prompt, session_file: session_file) post_suggestions(intent, reply) post_reply(intent, reply) end @@ -244,7 +244,7 @@ def run_on_pr_head(intent) worktree_path: paths.repo.worktree_host) checkout_pr_branch(paths.repo, intent.branch) - reply = @harness.run(yield(paths), tools: impl_tools, session_file: paths.session_file) + reply = llm(:pr_author, yield(paths), session_file: paths.session_file) post_reply(intent, reply) push_followup(intent, paths.repo) if commit_followup(intent, paths.repo) diff --git a/lib/opilot/harness.rb b/lib/opilot/harness.rb index 6febb8c..267f3f6 100644 --- a/lib/opilot/harness.rb +++ b/lib/opilot/harness.rb @@ -31,10 +31,8 @@ class Harness TOOLS_READ = "read,grep,find,ls,bash" TOOLS_IMPL = "read,grep,find,ls,bash,write,edit" - # The op_query variants (see MCP.md), granted only at the call sites named - # in Context#op_mcp?'s call table via Helpers#read_tools/#impl_tools — most - # TOOLS_READ/TOOLS_IMPL call sites keep the plain constant even when the - # flag is on. Must stay in sync with ALLOWED_TOOL_GRANTS in server.js. + # The op_query variants (see MCP.md), granted only to the roles marked + # `mcp` (roles.rb). Must stay in sync with ALLOWED_TOOL_GRANTS in server.js. TOOLS_READ_OP = "#{TOOLS_READ},op_query" TOOLS_IMPL_OP = "#{TOOLS_IMPL},op_query" @@ -85,6 +83,8 @@ def self.env_minutes(name, fallback) env_minutes("OPILOT_PI_IDLE_TIMEOUT_MIN", 5) + 2) * 60 ).round + require_relative "roles" + def initialize(ctx) @ctx = ctx @uri = URI(@ctx.harness_url) diff --git a/lib/opilot/health_check.rb b/lib/opilot/health_check.rb index eedb396..7ec471b 100644 --- a/lib/opilot/health_check.rb +++ b/lib/opilot/health_check.rb @@ -347,8 +347,8 @@ def facts_for(item, related, statuses, prs_and_code, commits, tree: nil, now: Ti "Answer again, and end with that block exactly as described." private def ask(prompt) - answer = Helpers.parse_health(@harness.run(prompt, tools: read_tools)) - answer || Helpers.parse_health(@harness.run(prompt + RETRY_NOTE, tools: read_tools)) + answer = Helpers.parse_health(llm(:auditor, prompt)) + answer || Helpers.parse_health(llm(:auditor, prompt + RETRY_NOTE)) end private def failed_note diff --git a/lib/opilot/helpers.rb b/lib/opilot/helpers.rb index 325bdde..a08dc0b 100644 --- a/lib/opilot/helpers.rb +++ b/lib/opilot/helpers.rb @@ -925,20 +925,13 @@ def ensure_harness! @harness.ensure_available! if @harness.respond_to?(:ensure_available!) end - # The tool grant for a read-only LLM phase — includes op_query when - # OPILOT_OP_MCP is on (see MCP.md). Use ONLY at the specific call sites - # named in that plan's Step 3 table; every other TOOLS_READ call site - # (upstream PR review, the `create wp` draft, light one-shot passes) keeps - # the plain constant even when the flag is on. - def read_tools - Harness.tools_for(Harness::TOOLS_READ, op_mcp: @ctx.op_mcp?, gh_mcp: @ctx.gh_mcp?) - end - - # As #read_tools, for the one write-enabled call site the plan grants the - # tool to: gh-agent's own-PR reply and CI fix. The fix implement run keeps - # no MCP tool at all — see MCP.md's Step 3 table for why. - def impl_tools - Harness.tools_for(Harness::TOOLS_IMPL, op_mcp: @ctx.op_mcp?, gh_mcp: @ctx.gh_mcp?) + # The one way to call the model: the role decides tools and model. + # A stateless role refuses a session, so its independence is structural. + def llm(role, prompt, session_file: nil, outfile: nil) + r = Harness.role(role) + raise ArgumentError, "role #{r.name} is stateless" if r.stateless && session_file + opts = { tools: r.tools(@ctx), model: r.model, session_file: session_file } + outfile ? @harness.capture(prompt, outfile: outfile, **opts) : @harness.run(prompt, **opts) end # One-time, best-effort report of what the instance's MCP server actually @@ -1320,16 +1313,15 @@ def commit_and_log(wt, message, where = nil) # # Reporting the result is deliberately left to the caller — a work-package # comment and a console line are not the same message. - def implement_plan(st, model: Harness::MODEL_HEAVY) + def implement_plan(st) st.repos.each { |r| checkout_branch(st, r) } unless st.repos.all? { |r| branch_has_commits?(st, r) } log_script "Implementing #{wp_label(st.item_id)} in #{st.repos.map(&:name).join(", ")}" - @harness.run( - Prompts.implement(repos: repos_for_prompt(st.repos), plan: container_path(st.plan_file), - resumed: session_resumable?(st)), - tools: Harness::TOOLS_IMPL, model: model, session_file: st.session_file - ) + llm(:implementer, + Prompts.implement(repos: repos_for_prompt(st.repos), plan: container_path(st.plan_file), + resumed: session_resumable?(st)), + session_file: st.session_file) st.repos.each { |r| commit(st, r) } end @@ -1353,7 +1345,7 @@ def commit(st, repo) # gh-agent's follow-up commits and the terminal `pr` refresh. def generate_commit_subject(diff) prompt = Prompts.commit_subject(diff: diff.patch.to_s[0, 6000]) - reply = @harness.run(prompt, tools: Harness::TOOLS_READ, model: Harness::MODEL_LIGHT) + reply = llm(:scribe, prompt) strip_ansi(reply.to_s).lines.map(&:strip).find { |l| !l.empty? }.to_s .gsub(/\A["'`]+|["'`]+\z/, "") # strip wrapping quotes/backticks .sub(/\A\[[^\]]*\]\s*/, "") # drop any "[label]" the LLM prepended anyway @@ -1367,7 +1359,7 @@ def generate_commit_subject(diff) # Stateless — a fresh, cheap-model call rather than a resumed session, since # the item/plan/diff are all passed as file paths or plain text the model can # read itself, with nothing depending on the implement session's history. - def generate_pr_description(st, repo, model: Harness::MODEL_LIGHT) + def generate_pr_description(st, repo) pr_desc_file = st.pr_desc_file(repo) return if Helpers.file_has_content?(pr_desc_file) wt = worktree(repo) @@ -1380,7 +1372,7 @@ def generate_pr_description(st, repo, model: Harness::MODEL_LIGHT) item: container_path(st.item_file), plan: container_path(st.plan_file), diff_stat: diff_stat, template_section: template_section ) - pr_text = @harness.run(prompt, tools: Harness::TOOLS_READ, model: model) + pr_text = llm(:scribe, prompt) pr_body = pr_text[/^#.*/m] || pr_text pr_desc_file.write(strip_ansi(pr_body)) end diff --git a/lib/opilot/pd/runner.rb b/lib/opilot/pd/runner.rb index 4d8a98e..8651c57 100644 --- a/lib/opilot/pd/runner.rb +++ b/lib/opilot/pd/runner.rb @@ -172,10 +172,11 @@ def revise_proposal(change_id, comment_section:, pr_thread:, session_file:, repo state = change_state_for(change_id, store) store.materialise! - reply = @harness.run( + reply = llm( + :spec_writer, Prompts.propose_feedback(change_id: change_id, change_dir: state.working_change_container, pr_thread: pr_thread, comment_section: comment_section), - tools: Harness::TOOLS_IMPL, model: Harness::MODEL_HEAVY, session_file: session_file + session_file: session_file ) # A question rather than a change request leaves the tree untouched: reply @@ -365,7 +366,8 @@ def spec_pr_url(state) def write_proposal(state, repo) change_dir = state.working_change_dir log_script "Proposing #{state.change_id} in #{repo.name}…" - text = @harness.run( + text = llm( + :spec_writer, Prompts.propose( change_id: state.change_id, change_dir: state.working_change_container, @@ -375,7 +377,7 @@ def write_proposal(state, repo) repo_path: repo.worktree_container, instructions: artifact_instructions(state, repo) ), - tools: Harness::TOOLS_IMPL, model: Harness::MODEL_HEAVY, session_file: state.session_file + session_file: state.session_file ) # What the LLM DID beats what it said about what it did. It routinely @@ -449,11 +451,12 @@ def validate_proposal!(state, repo) "#{MAX_VALIDATE_ATTEMPTS} revisions:\n#{failures}" end log_script "openspec validate failed (attempt #{attempt + 1}/#{MAX_VALIDATE_ATTEMPTS}) — re-prompting" - @harness.run( + llm( + :spec_writer, Prompts.propose_revise(change_id: state.change_id, change_dir: state.working_change_container, failures: failures, attempt: attempt + 1, max_attempts: MAX_VALIDATE_ATTEMPTS), - tools: Harness::TOOLS_IMPL, model: Harness::MODEL_HEAVY, session_file: state.session_file + session_file: state.session_file ) end end @@ -736,14 +739,15 @@ def implement_one(wp_id, repo_name: nil) # Inside this branch, not above it: a re-run that only publishes an # already-built branch must not rewind the status it set last time. transition!(wp_id, item, @ctx.pd_implementing_status) - @harness.run( + llm( + :implementer, Prompts.implement_task( repo: repo.name, repo_path: repo.worktree_container, change_id: found[:change_id], change_dir: state.working_change_container, wp_label: wp_label(wp_id), section: found[:section].title, tasks: render_tasks(found[:section]), item: container_path(st.item_file) ), - tools: Harness::TOOLS_IMPL, model: Harness::MODEL_HEAVY, session_file: st.session_file + session_file: st.session_file ) restore_spec_tree!(found[:store], found[:change_id]) publish # memoize the identity commit() authors as diff --git a/lib/opilot/pr_runner.rb b/lib/opilot/pr_runner.rb index 54c0240..50ca0a5 100644 --- a/lib/opilot/pr_runner.rb +++ b/lib/opilot/pr_runner.rb @@ -404,7 +404,7 @@ def refresh_with_harness(wp_id, dir, repo, base_repo, number, content, base_ref, ci: ci, conflicts: conflicts, feedback_count: feedback.length ) # Shares gh-agent's per-PR session so prior PR conversations carry over. - @harness.run(prompt, tools: Harness::TOOLS_IMPL, session_file: dir / "gh_session_id") + llm(:pr_refresher, prompt, session_file: dir / "gh_session_id") end # Commit what the refresh produced. A conflicted merge is concluded here (the diff --git a/lib/opilot/roles.rb b/lib/opilot/roles.rb new file mode 100644 index 0000000..e2ecc8d --- /dev/null +++ b/lib/opilot/roles.rb @@ -0,0 +1,30 @@ +module OPilot + class Harness + # One role the model plays: its tool grant, its model, and whether it may + # resume a session. Every LLM call names one (Helpers#llm). The MCP tools + # resolve per call, because they follow the Context flags. + Role = Data.define(:name, :base, :mcp, :model, :stateless) do + def tools(ctx) + mcp ? Harness.tools_for(base, op_mcp: ctx.op_mcp?, gh_mcp: ctx.gh_mcp?) : base + end + end + + # One entry per distinct (grant, model, memory) tuple in use. Two roles that + # look alike but differ in grant stay separate until someone decides. + ROLES = [ + Role.new(:planner, TOOLS_READ, true, MODEL_HEAVY, false), # plan, re-plan + Role.new(:advisor, TOOLS_READ, true, MODEL_HEAVY, false), # WP chat, `chat` + Role.new(:wp_writer, TOOLS_READ, false, MODEL_HEAVY, false), # `create wp` + Role.new(:triager, TOOLS_READ, true, MODEL_HEAVY, true), # `appsignal fix` + Role.new(:auditor, TOOLS_READ, true, MODEL_HEAVY, true), # health + Role.new(:implementer, TOOLS_IMPL, false, MODEL_HEAVY, false), # fix, `pd implement` + Role.new(:spec_writer, TOOLS_IMPL, false, MODEL_HEAVY, false), # `pd propose` + Role.new(:pr_author, TOOLS_IMPL, true, MODEL_HEAVY, false), # own-PR reply, CI fix + Role.new(:pr_refresher, TOOLS_IMPL, false, MODEL_HEAVY, false), # `dev refresh` + Role.new(:pr_advisor, TOOLS_READ, false, MODEL_HEAVY, false), # upstream PR, reply-only + Role.new(:scribe, TOOLS_READ, false, MODEL_LIGHT, true), # commit subject, PR body + ].to_h { |r| [r.name, r] }.freeze + + def self.role(name) = ROLES.fetch(name) + end +end diff --git a/test/opilot/roles_test.rb b/test/opilot/roles_test.rb new file mode 100644 index 0000000..8c08778 --- /dev/null +++ b/test/opilot/roles_test.rb @@ -0,0 +1,81 @@ +require_relative "../test_helper" + +module OPilot + class RolesTest < Minitest::Test + ROOT = Pathname(__dir__) / "../.." + + class Caller + include Helpers + attr_reader :calls + + def initialize(ctx) + @ctx = ctx + @calls = [] + calls = @calls + @harness = Object.new + @harness.define_singleton_method(:run) { |prompt, **opts| calls << opts.merge(prompt: prompt); "ok" } + @harness.define_singleton_method(:capture) { |prompt, **opts| calls << opts.merge(prompt: prompt); "ok" } + end + end + + def ctx(op: false, gh: false) + Struct.new(:op_mcp?, :gh_mcp?).new(op, gh) + end + + def server_grants + (ROOT / "server.js").read[/ALLOWED_TOOL_GRANTS = new Set\(\[(.*?)\]\)/m, 1].scan(/'([^']+)'/).flatten + end + + def test_every_persona_resolves_to_a_grant_server_js_allows + grants = server_grants + refute_empty grants + Harness::ROLES.each_value do |p| + [[false, false], [true, false], [false, true], [true, true]].each do |op, gh| + assert_includes grants, p.tools(ctx(op: op, gh: gh)), "#{p.name} op=#{op} gh=#{gh}" + end + end + end + + def test_mcp_tools_follow_the_flags_only_for_mcp_personas + assert_equal "#{Harness::TOOLS_READ},op_query", Harness.role(:planner).tools(ctx(op: true)) + assert_equal Harness::TOOLS_READ, Harness.role(:wp_writer).tools(ctx(op: true, gh: true)) + end + + def test_llm_passes_the_personas_tools_and_model + c = Caller.new(ctx) + c.send(:llm, :scribe, "hi") + assert_equal({ tools: Harness::TOOLS_READ, model: Harness::MODEL_LIGHT, session_file: nil, prompt: "hi" }, + c.calls.last) + end + + def test_llm_with_outfile_captures + c = Caller.new(ctx) + c.send(:llm, :planner, "plan", outfile: "/tmp/x", session_file: "s") + assert_equal "/tmp/x", c.calls.last[:outfile] + end + + def test_a_stateless_persona_refuses_a_session + err = assert_raises(ArgumentError) { Caller.new(ctx).send(:llm, :auditor, "x", session_file: "s") } + assert_match(/stateless/, err.message) + end + + def test_unknown_persona_fails + assert_raises(KeyError) { Harness.role(:nobody) } + end + + # Keeps the role table the one place a grant or model is chosen. + def test_no_call_site_bypasses_llm + files = Dir[ROOT / "lib/**/*.rb"] + assert_operator files.size, :>, 20 + direct = File.readlines(ROOT / "lib/opilot/helpers.rb").grep(/@harness\.(run|capture)\b/) + assert_equal 1, direct.size, "helpers.rb calls the harness only from #llm" + offenders = files.flat_map do |f| + next [] if f.end_with?("/helpers.rb", "/harness.rb", "/roles.rb") + File.readlines(f).each_with_index.filter_map do |line, i| + "#{f}:#{i + 1}" if line.match?(/@harness\.(run|capture)\b|Harness::TOOLS_/) + end + end + assert_empty offenders + end + end +end From 113c5c367f76cb540dc2115e2a7286a453a0f66f Mon Sep 17 00:00:00 2001 From: Tomas Hykel Date: Wed, 30 Sep 2026 18:24:04 +0200 Subject: [PATCH 3/9] refactor: load roles from roles/*.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each role is now one markdown file: frontmatter for tools, mcp, model and memory, a body saying what the role does and who calls it. The table in lib/opilot/roles.rb is gone; the loader reads the files at boot and rejects an unknown key or value. The body is not sent to the model yet — it becomes the role's charter in a later step. No behaviour change: roles_test pins every role to the tuple the old table held. It also checks that every llm(:name) call site names a role that exists, and that a malformed role file fails to load. Co-Authored-By: Claude Opus 5.5 --- CLAUDE.md | 22 ++++++++------- lib/opilot/roles.rb | 55 ++++++++++++++++++++++++------------- roles/advisor.md | 9 +++++++ roles/auditor.md | 9 +++++++ roles/implementer.md | 9 +++++++ roles/planner.md | 9 +++++++ roles/pr_advisor.md | 9 +++++++ roles/pr_author.md | 9 +++++++ roles/pr_refresher.md | 9 +++++++ roles/scribe.md | 9 +++++++ roles/spec_writer.md | 9 +++++++ roles/triager.md | 9 +++++++ roles/wp_writer.md | 9 +++++++ test/opilot/roles_test.rb | 57 +++++++++++++++++++++++++++++++++++---- 14 files changed, 200 insertions(+), 33 deletions(-) create mode 100644 roles/advisor.md create mode 100644 roles/auditor.md create mode 100644 roles/implementer.md create mode 100644 roles/planner.md create mode 100644 roles/pr_advisor.md create mode 100644 roles/pr_author.md create mode 100644 roles/pr_refresher.md create mode 100644 roles/scribe.md create mode 100644 roles/spec_writer.md create mode 100644 roles/triager.md create mode 100644 roles/wp_writer.md diff --git a/CLAUDE.md b/CLAUDE.md index 4e0ae81..5388676 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -500,7 +500,7 @@ bare `docker compose run …` works from the repo root. | `pr_runner.rb` | Terminal `dev refresh`, and gh-agent's `@opilot refresh` via `#refresh_one` | | `op_runner.rb` | Terminal `op` — one command per `Clients::OpenProject` method it exposes. Three rules hold: **stdout is data** (JSON only, diagnostics to stderr, never `log_script`), every action **reads except `wp create`**, and **`--type` is required of every payload**. `wp form --required` is how you learn what else a project demands. The file header argues all three — read it there rather than re-deriving them | | `harness.rb` | HTTP client to the harness container; per-WP session IDs | -| `roles.rb` | The roles the model plays (grant, model, stateless) — every LLM call names one via `Helpers#llm` | +| `roles.rb` | Loads `roles/*.md`, the roles the model plays (grant, model, memory) — every LLM call names one via `Helpers#llm` | | `appsignal_runner.rb` | Terminal `appsignal` — incident → work package, then hands off to `FixRunner#ship_ids`. Owns the local-model guard, and every preflight runs before the create | | `clients/appsignal.rb` | AppSignal's GraphQL + V2 tracing APIs, assembled into one incident: metadata, the request payload, and the backtrace. The runner's client, never a tool for the model | | `clients/inference_gw.rb` | inference-gw's `GET /upstream` — the pinned inference address, which is what `Context#inference_privacy` judges | @@ -954,14 +954,18 @@ Runner POSTs to `http://harness:47291` with headers: (see `MCP.md`) — the other roles keep the plain grant regardless. `server.js` rejects any other grant, so its allowlist (`ALLOWED_TOOL_GRANTS`, four strings) must stay in sync with `TOOLS_READ`/`TOOLS_IMPL`/`TOOLS_READ_OP`/`TOOLS_IMPL_OP`. -- **Every call names a role** (`Helpers#llm`, table in `lib/opilot/roles.rb`): - a role's base grant, whether the MCP tools join it, its model, and whether it is - **stateless** (a stateless role given a `session_file` raises, so health's - independence from chat is structural). Nothing outside `helpers.rb`/`harness.rb` - calls `@harness.run` or names `Harness::TOOLS_*` — `test/opilot/personas_test.rb` - checks that, and checks every role against `ALLOWED_TOOL_GRANTS`. Roles that - look alike but differ in grant (`pr_author` vs `pr_refresher`) stay two - roles until someone decides to merge them. +- **Every call names a role** (`Helpers#llm`). A role is one file, + `roles/.md`: frontmatter for `tools` (`read`/`write`), `mcp` (whether the + MCP tools join the grant), `model` (`heavy`/`light`) and `memory` + (`session`/`none`), and a body saying what the role does and who calls it. The + body is not sent to the model yet. `lib/opilot/roles.rb` loads them strictly — + an unknown key or value fails at boot — and a `memory: none` role given a + `session_file` raises, so health's independence from chat is structural. + `test/opilot/roles_test.rb` pins every role's tuple, checks each against + `ALLOWED_TOOL_GRANTS`, and fails if anything outside `helpers.rb`/`harness.rb` + calls `@harness.run` or names `Harness::TOOLS_*`. Roles that look alike but + differ in grant (`pr_author` vs `pr_refresher`) stay two roles until someone + decides to merge them. - `X-Harness-Model` — one model per WP for every session-bound phase (`MODEL_HEAVY`), plus `MODEL_LIGHT` for stateless one-shots (a commit subject, a PR description) — always `/` (`openrouter/anthropic/claude-sonnet-5.5`, diff --git a/lib/opilot/roles.rb b/lib/opilot/roles.rb index e2ecc8d..0cc4a0a 100644 --- a/lib/opilot/roles.rb +++ b/lib/opilot/roles.rb @@ -1,29 +1,46 @@ +require "yaml" + module OPilot class Harness - # One role the model plays: its tool grant, its model, and whether it may - # resume a session. Every LLM call names one (Helpers#llm). The MCP tools - # resolve per call, because they follow the Context flags. - Role = Data.define(:name, :base, :mcp, :model, :stateless) do + # One role the model plays, loaded from roles/.md: frontmatter for the + # grant, model and memory, body for what the role does. Every LLM call names + # one (Helpers#llm). The MCP tools resolve per call, from the Context flags. + Role = Data.define(:name, :base, :mcp, :model, :memory, :description) do + def stateless = memory == :none + def tools(ctx) mcp ? Harness.tools_for(base, op_mcp: ctx.op_mcp?, gh_mcp: ctx.gh_mcp?) : base end end - # One entry per distinct (grant, model, memory) tuple in use. Two roles that - # look alike but differ in grant stay separate until someone decides. - ROLES = [ - Role.new(:planner, TOOLS_READ, true, MODEL_HEAVY, false), # plan, re-plan - Role.new(:advisor, TOOLS_READ, true, MODEL_HEAVY, false), # WP chat, `chat` - Role.new(:wp_writer, TOOLS_READ, false, MODEL_HEAVY, false), # `create wp` - Role.new(:triager, TOOLS_READ, true, MODEL_HEAVY, true), # `appsignal fix` - Role.new(:auditor, TOOLS_READ, true, MODEL_HEAVY, true), # health - Role.new(:implementer, TOOLS_IMPL, false, MODEL_HEAVY, false), # fix, `pd implement` - Role.new(:spec_writer, TOOLS_IMPL, false, MODEL_HEAVY, false), # `pd propose` - Role.new(:pr_author, TOOLS_IMPL, true, MODEL_HEAVY, false), # own-PR reply, CI fix - Role.new(:pr_refresher, TOOLS_IMPL, false, MODEL_HEAVY, false), # `dev refresh` - Role.new(:pr_advisor, TOOLS_READ, false, MODEL_HEAVY, false), # upstream PR, reply-only - Role.new(:scribe, TOOLS_READ, false, MODEL_LIGHT, true), # commit subject, PR body - ].to_h { |r| [r.name, r] }.freeze + ROLES_DIR = Pathname(__dir__).join("../../roles").expand_path + + ROLE_VALUES = { + "tools" => { "read" => TOOLS_READ, "write" => TOOLS_IMPL }, + "mcp" => { true => true, false => false }, + "model" => { "heavy" => MODEL_HEAVY, "light" => MODEL_LIGHT }, + "memory" => { "session" => :session, "none" => :none }, + }.freeze + + # Strict on purpose: a typo in a role file must fail at boot, not grant + # something unexpected at the first call. + def self.load_role(path) + name = path.basename(".md").to_s + _, front, body = path.read.split(/^---\s*$/, 3) + raise ArgumentError, "#{path}: no frontmatter" unless body + meta = YAML.safe_load(front) || {} + unless meta.keys.sort == ROLE_VALUES.keys.sort + raise ArgumentError, "#{path}: keys must be #{ROLE_VALUES.keys.join(", ")}" + end + values = ROLE_VALUES.to_h do |key, allowed| + raise ArgumentError, "#{path}: #{key}: #{meta[key].inspect} is not one of #{allowed.keys.join(", ")}" unless allowed.key?(meta[key]) + [key.to_sym, allowed[meta[key]]] + end + Role.new(name: name.to_sym, base: values[:tools], mcp: values[:mcp], model: values[:model], + memory: values[:memory], description: body.strip) + end + + ROLES = ROLES_DIR.glob("*.md").sort.map { |f| load_role(f) }.to_h { |r| [r.name, r] }.freeze def self.role(name) = ROLES.fetch(name) end diff --git a/roles/advisor.md b/roles/advisor.md new file mode 100644 index 0000000..3e97324 --- /dev/null +++ b/roles/advisor.md @@ -0,0 +1,9 @@ +--- +tools: read +mcp: true +model: heavy +memory: session +--- +Answers questions about a work package, its plan or the local mirrors. Changes no file. + +Used by `Agent` (`:chat` intent), `FixRunner#run_chat`, `ChatRunner` (`./opilot chat`). diff --git a/roles/auditor.md b/roles/auditor.md new file mode 100644 index 0000000..29aa020 --- /dev/null +++ b/roles/auditor.md @@ -0,0 +1,9 @@ +--- +tools: read +mcp: true +model: heavy +memory: none +--- +Checks a work package for inconsistencies with itself. Stateless, so the check does not depend on earlier chat turns. + +Used by `HealthCheck` (`@opilot health`, `dev health`). diff --git a/roles/implementer.md b/roles/implementer.md new file mode 100644 index 0000000..fd52f13 --- /dev/null +++ b/roles/implementer.md @@ -0,0 +1,9 @@ +--- +tools: write +mcp: false +model: heavy +memory: session +--- +Applies an approved plan, or one `pd` task section, in the target worktrees. Does not commit or run commands. + +Used by `Helpers#implement_plan`, `PD::Runner` (`pd implement`). diff --git a/roles/planner.md b/roles/planner.md new file mode 100644 index 0000000..d0539e4 --- /dev/null +++ b/roles/planner.md @@ -0,0 +1,9 @@ +--- +tools: read +mcp: true +model: heavy +memory: session +--- +Reads a work package and the code in every registry repo, and writes `plan.md`, or `NEEDS_INFO`, or a list of `OPTIONS`. + +Used by `Agent#produce_plan`, `FixRunner` (plan and re-plan). diff --git a/roles/pr_advisor.md b/roles/pr_advisor.md new file mode 100644 index 0000000..d45c295 --- /dev/null +++ b/roles/pr_advisor.md @@ -0,0 +1,9 @@ +--- +tools: read +mcp: false +model: heavy +memory: session +--- +Answers a mention on an upstream PR that opilot did not open. Text and inline suggestions only; never pushes. + +Used by `GhAgent` (tracked upstream PRs). diff --git a/roles/pr_author.md b/roles/pr_author.md new file mode 100644 index 0000000..87a0c11 --- /dev/null +++ b/roles/pr_author.md @@ -0,0 +1,9 @@ +--- +tools: write +mcp: true +model: heavy +memory: session +--- +Works on a PR that opilot opened: replies to comments, changes code, and fixes failed CI. The runner pushes to the fork. + +Used by `GhAgent#run_on_pr_head`. diff --git a/roles/pr_refresher.md b/roles/pr_refresher.md new file mode 100644 index 0000000..034690b --- /dev/null +++ b/roles/pr_refresher.md @@ -0,0 +1,9 @@ +--- +tools: write +mcp: false +model: heavy +memory: session +--- +Refreshes a shipped PR: resolves the base merge, fixes CI, and addresses new comments. + +Used by `PrRunner` (`dev refresh`, `@opilot refresh`). diff --git a/roles/scribe.md b/roles/scribe.md new file mode 100644 index 0000000..f2be58d --- /dev/null +++ b/roles/scribe.md @@ -0,0 +1,9 @@ +--- +tools: read +mcp: false +model: light +memory: none +--- +Writes short text from given input: a commit subject or a PR description. + +Used by `Helpers#generate_commit_subject`, `Helpers#generate_pr_description`. diff --git a/roles/spec_writer.md b/roles/spec_writer.md new file mode 100644 index 0000000..2d30e31 --- /dev/null +++ b/roles/spec_writer.md @@ -0,0 +1,9 @@ +--- +tools: write +mcp: false +model: heavy +memory: session +--- +Writes and revises an OpenSpec change proposal. + +Used by `PD::Runner` (`pd propose`, the validate re-prompts, and feedback on the spec PR). diff --git a/roles/triager.md b/roles/triager.md new file mode 100644 index 0000000..4e28841 --- /dev/null +++ b/roles/triager.md @@ -0,0 +1,9 @@ +--- +tools: read +mcp: true +model: heavy +memory: none +--- +Reads one AppSignal incident and drafts the work package that `appsignal fix` creates. + +Used by `AppSignalRunner`. diff --git a/roles/wp_writer.md b/roles/wp_writer.md new file mode 100644 index 0000000..f21b172 --- /dev/null +++ b/roles/wp_writer.md @@ -0,0 +1,9 @@ +--- +tools: read +mcp: false +model: heavy +memory: session +--- +Drafts the work packages for `@opilot create wp`. The runner checks each draft through the create form before anything is saved. + +Used by `Agent#write_work_packages`. diff --git a/test/opilot/roles_test.rb b/test/opilot/roles_test.rb index 8c08778..3d195f2 100644 --- a/test/opilot/roles_test.rb +++ b/test/opilot/roles_test.rb @@ -1,4 +1,5 @@ require_relative "../test_helper" +require "tmpdir" module OPilot class RolesTest < Minitest::Test @@ -26,7 +27,7 @@ def server_grants (ROOT / "server.js").read[/ALLOWED_TOOL_GRANTS = new Set\(\[(.*?)\]\)/m, 1].scan(/'([^']+)'/).flatten end - def test_every_persona_resolves_to_a_grant_server_js_allows + def test_every_role_resolves_to_a_grant_server_js_allows grants = server_grants refute_empty grants Harness::ROLES.each_value do |p| @@ -36,12 +37,12 @@ def test_every_persona_resolves_to_a_grant_server_js_allows end end - def test_mcp_tools_follow_the_flags_only_for_mcp_personas + def test_mcp_tools_follow_the_flags_only_for_mcp_roles assert_equal "#{Harness::TOOLS_READ},op_query", Harness.role(:planner).tools(ctx(op: true)) assert_equal Harness::TOOLS_READ, Harness.role(:wp_writer).tools(ctx(op: true, gh: true)) end - def test_llm_passes_the_personas_tools_and_model + def test_llm_passes_the_role_tools_and_model c = Caller.new(ctx) c.send(:llm, :scribe, "hi") assert_equal({ tools: Harness::TOOLS_READ, model: Harness::MODEL_LIGHT, session_file: nil, prompt: "hi" }, @@ -54,12 +55,58 @@ def test_llm_with_outfile_captures assert_equal "/tmp/x", c.calls.last[:outfile] end - def test_a_stateless_persona_refuses_a_session + def test_a_stateless_role_refuses_a_session err = assert_raises(ArgumentError) { Caller.new(ctx).send(:llm, :auditor, "x", session_file: "s") } assert_match(/stateless/, err.message) end - def test_unknown_persona_fails + # The table roles/*.md replaced, pinned so a role file edit is a deliberate test edit. + EXPECTED = { + planner: [Harness::TOOLS_READ, true, Harness::MODEL_HEAVY, :session], + advisor: [Harness::TOOLS_READ, true, Harness::MODEL_HEAVY, :session], + wp_writer: [Harness::TOOLS_READ, false, Harness::MODEL_HEAVY, :session], + triager: [Harness::TOOLS_READ, true, Harness::MODEL_HEAVY, :none], + auditor: [Harness::TOOLS_READ, true, Harness::MODEL_HEAVY, :none], + implementer: [Harness::TOOLS_IMPL, false, Harness::MODEL_HEAVY, :session], + spec_writer: [Harness::TOOLS_IMPL, false, Harness::MODEL_HEAVY, :session], + pr_author: [Harness::TOOLS_IMPL, true, Harness::MODEL_HEAVY, :session], + pr_refresher: [Harness::TOOLS_IMPL, false, Harness::MODEL_HEAVY, :session], + pr_advisor: [Harness::TOOLS_READ, false, Harness::MODEL_HEAVY, :session], + scribe: [Harness::TOOLS_READ, false, Harness::MODEL_LIGHT, :none], + }.freeze + + def test_role_files_hold_the_expected_tuples + actual = Harness::ROLES.transform_values { |r| [r.base, r.mcp, r.model, r.memory] } + assert_equal EXPECTED, actual + Harness::ROLES.each_value { |r| refute_empty r.description, r.name } + end + + def test_every_call_site_names_a_known_role + used = Dir[ROOT / "lib/**/*.rb"].flat_map { |f| File.read(f).scan(/\bllm\(\s*:(\w+)/).flatten } + refute_empty used + assert_empty used.map(&:to_sym).uniq - Harness::ROLES.keys + end + + def write_role(dir, text) + path = Pathname(dir) / "x.md" + path.write(text) + path + end + + def test_a_bad_role_file_fails_to_load + Dir.mktmpdir do |dir| + good = "---\ntools: read\nmcp: false\nmodel: heavy\nmemory: none\n---\nDoes x.\n" + assert_equal :x, Harness.load_role(write_role(dir, good)).name + [good.sub("read", "admin"), # unknown grant + good.sub("mcp: false\n", ""), # missing key + good.sub("memory: none", "memory: none\nextra: 1"), # unknown key + "no frontmatter\n"].each do |bad| + assert_raises(ArgumentError, bad) { Harness.load_role(write_role(dir, bad)) } + end + end + end + + def test_unknown_role_fails assert_raises(KeyError) { Harness.role(:nobody) } end From ad80d0a7a2dfea9d08b5b6c3ee78dd387d4e9ae2 Mon Sep 17 00:00:00 2001 From: Tomas Hykel Date: Wed, 30 Sep 2026 18:31:18 +0200 Subject: [PATCH 4/9] refactor: split prompts.rb into one module per role Each role now has lib/opilot/prompts/.rb holding the builders it sends (Prompts::Planner.plan, Prompts::PrAuthor.fix_ci, ...). What more than one role uses stays in prompts.rb: the constants at top level of Prompts, the helper methods in Prompts::Sections, which every role module and Prompts itself extend. A builder now returns a Prompts::Prompt, a String tagged with its role. Helpers#llm raises when a tagged prompt is sent under another role. A bare follow-up message is a plain String and is not checked. No prompt text changed: a snapshot of all 19 builders over 35 argument combinations renders byte-identical before and after. Co-Authored-By: Claude Opus 5.5 --- CLAUDE.md | 12 +- lib/opilot/agent.rb | 16 +- lib/opilot/appsignal_runner.rb | 2 +- lib/opilot/chat_runner.rb | 2 +- lib/opilot/fix_runner.rb | 6 +- lib/opilot/gh_agent.rb | 6 +- lib/opilot/health_check.rb | 2 +- lib/opilot/helpers.rb | 11 +- lib/opilot/pd/CLAUDE.md | 6 +- lib/opilot/pd/runner.rb | 8 +- lib/opilot/pr_runner.rb | 2 +- lib/opilot/prompts.rb | 1234 ++++------------------------ lib/opilot/prompts/advisor.rb | 129 +++ lib/opilot/prompts/auditor.rb | 77 ++ lib/opilot/prompts/implementer.rb | 92 +++ lib/opilot/prompts/planner.rb | 104 +++ lib/opilot/prompts/pr_advisor.rb | 55 ++ lib/opilot/prompts/pr_author.rb | 65 ++ lib/opilot/prompts/pr_refresher.rb | 63 ++ lib/opilot/prompts/scribe.rb | 52 ++ lib/opilot/prompts/spec_writer.rb | 115 +++ lib/opilot/prompts/triager.rb | 106 +++ lib/opilot/prompts/wp_writer.rb | 132 +++ lib/opilot/repo.rb | 2 +- test/opilot/agent_test.rb | 2 +- test/opilot/helpers_test.rb | 2 +- test/opilot/roles_test.rb | 19 + 27 files changed, 1227 insertions(+), 1095 deletions(-) create mode 100644 lib/opilot/prompts/advisor.rb create mode 100644 lib/opilot/prompts/auditor.rb create mode 100644 lib/opilot/prompts/implementer.rb create mode 100644 lib/opilot/prompts/planner.rb create mode 100644 lib/opilot/prompts/pr_advisor.rb create mode 100644 lib/opilot/prompts/pr_author.rb create mode 100644 lib/opilot/prompts/pr_refresher.rb create mode 100644 lib/opilot/prompts/scribe.rb create mode 100644 lib/opilot/prompts/spec_writer.rb create mode 100644 lib/opilot/prompts/triager.rb create mode 100644 lib/opilot/prompts/wp_writer.rb diff --git a/CLAUDE.md b/CLAUDE.md index 5388676..02290db 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -162,7 +162,7 @@ id names no WP dir. It also **auto-fixes failed CI** (always on). Once checks complete with ≥1 failure, the detail (annotations, output summaries, failed-job log tails) is cached to -`ci.json`, fixed with `Prompts.fix_ci`, committed and pushed. The trigger is the +`ci.json`, fixed with `Prompts::PrAuthor.fix_ci`, committed and pushed. The trigger is the **head SHA**: `gh_pr.json` tracks `ci_acted_sha` (once per commit) and `ci_attempts` (`OPILOT_CI_MAX_ATTEMPTS`, default 5; past the cap it posts a one-time "needs a human" note and sets `ci_gave_up`). It acts on the *first* failure rather than @@ -178,7 +178,7 @@ which would truncate a big CI matrix). an LLM call is spent only on real mentions. The trigger is a prompt addressed to opilot, not a review pass over other people's work; what differs is write access, so these intents are `reply_only` — read-only fetch, answered in text -(`Prompts.pr_review`), never pushed. Applicable code still lands: for lines already +(`Prompts::PrAdvisor.pr_review`), never pushed. Applicable code still lands: for lines already in the diff the review emits a `SUGGESTIONS:` block (`Prompts::SUGGESTION_CONTRACT`) that `GhAgent#post_suggestions` posts as a review of inline `suggestion` comments (anchored to the head SHA, `event: COMMENT`) — the author @@ -225,7 +225,7 @@ nothing" and "not scanning" look identical in the log. is posted as a 🤖 comment and the cutoff advances so gh-agent doesn't re-handle it. - **`chat [message]`** — free read-only conversation over the local mirrors, never fetching, planning, or shipping. `.opilot/` is mounted read-only at `/state`, so - `Prompts.free_chat` orients the LLM at the layout and it Greps/Reads from there. + `Prompts::Advisor.free_chat` orients the LLM at the layout and it Greps/Reads from there. Fresh per-run session; needs no tokens or allowlist. **It reads only what another run already mirrored** — a `dev` verb or an agent tick. Nothing seeds the cache on its own: `op wp get` prints a work package but caches nothing, so a WP opilot has @@ -504,7 +504,7 @@ bare `docker compose run …` works from the repo root. | `appsignal_runner.rb` | Terminal `appsignal` — incident → work package, then hands off to `FixRunner#ship_ids`. Owns the local-model guard, and every preflight runs before the create | | `clients/appsignal.rb` | AppSignal's GraphQL + V2 tracing APIs, assembled into one incident: metadata, the request payload, and the backtrace. The runner's client, never a tool for the model | | `clients/inference_gw.rb` | inference-gw's `GET /upstream` — the pinned inference address, which is what `Context#inference_privacy` judges | -| `prompts.rb` | All LLM prompts in one place. Everything opilot publishes (WP comments, PR replies and descriptions, plans, spec proposals) is written in ASD-STE100 Simplified Technical English — stated once in `Prompts::PLAIN_ENGLISH` and pulled into the shared blocks (`OP_COMMENT_FORMAT`, `REPLY_CONTRACT`, `TERMINAL_REPLY`, `#plan_skeleton`), never re-worded per prompt. Code and commit messages are out of scope | +| `prompts.rb`, `prompts/.rb` | All LLM prompts: one module per role (`Prompts::Planner.plan`, `Prompts::PrAuthor.fix_ci`, …) holding that role's builders, and the shared blocks in `prompts.rb`. A builder returns a `Prompts::Prompt` tagged with its role, and `Helpers#llm` refuses one sent under another role. Everything opilot publishes (WP comments, PR replies and descriptions, plans, spec proposals) is written in ASD-STE100 Simplified Technical English — stated once in `Prompts::PLAIN_ENGLISH` and pulled into the shared blocks (`OP_COMMENT_FORMAT`, `REPLY_CONTRACT`, `TERMINAL_REPLY`, `Planner.plan_skeleton`), never re-worded per prompt. Code and commit messages are out of scope | | `publish.rb` | Pushes branches to the fork; opens cross-repo draft PRs via Octokit | | `clients/openproject.rb` | OpenProject REST API. `#add_comment` is the funnel every WP comment passes through, so it demotes markdown headings to bold — the activity tab is a narrow column | | `clients/github.rb` | GitHub API (Octokit) | @@ -717,7 +717,7 @@ is not enough. Like `create wp`, the handler answers its own failure. **A chat answer can carry an ARTIFACT — a diagram or a long report — published as a secret gist and linked from the comment.** Three surfaces offer one, and they are deliberately **asymmetric**: a gh-agent PR reply (`Prompts::MERMAID_NOTE` in -`gh_reply`/`pr_review`) and a plan's Approach section (`Prompts#plan_skeleton`) are +`gh_reply`/`pr_review`) and a plan's Approach section (`Prompts::Planner.plan_skeleton`) are **prompt-only**, because GitHub and gists render a ```mermaid fence themselves. Only op-agent chat has machinery behind it. @@ -778,7 +778,7 @@ duplicate create. `Pull#intent_from_comments` drops a non-allowlisted trigger whenever a list exists, so every create that reaches the handler is from a listed user. - **Every work package it will create comes out of ONE LLM call, gated by - `NEEDS_INFO`** (`Prompts.create_wp`, `Agent#write_work_packages`). When the request + `NEEDS_INFO`** (`Prompts::WpWriter.create_wp`, `Agent#write_work_packages`). When the request points at nothing in the thread, questions are the only acceptable answer. N answers cost the same one call as one, and a call per work package would not see the others — two of them could write the same suggestion, and the duplicate could not be deleted. diff --git a/lib/opilot/agent.rb b/lib/opilot/agent.rb index a7b9fea..ae17b97 100644 --- a/lib/opilot/agent.rb +++ b/lib/opilot/agent.rb @@ -124,7 +124,7 @@ def handle_chat(intent) # Pass the plan's path, not its text: a resumed session already holds the # plan, so re-embedding it every turn just burns tokens. plan_ref = st.plan_file.exist? ? container_path(st.plan_file) : "(no plan yet)" - prompt = Prompts.chat(item_id: st.item_id, subject: st.subject, + prompt = Prompts::Advisor.chat(item_id: st.item_id, subject: st.subject, item: container_path(st.item_file), plan: plan_ref, message: intent.text.to_s, related: related_ref(st), can_create_wp: create_wp_enabled?, @@ -298,7 +298,7 @@ def artifacts_enabled? # enforced HERE, because a prompt limit drifts and a work package can never # be deleted: "create one for every suggestion in this thread" must not be # able to mint twenty rows nobody can remove. Five also sits well inside one - # output budget — see Prompts.create_wp on why a cut-off answer is the + # output budget — see Prompts::WpWriter.create_wp on why a cut-off answer is the # failure mode to fear. MAX_CREATE_WP = 5 @@ -387,13 +387,13 @@ def project_type_names(project_id) # is a lost request, not a duplicate work package. def write_work_packages(st, request, project_name, types, related, retry_bad: true, format_note: nil) log_script "Writer: drafting work packages from #{wp_label(st.item_id)} — #{request}" - prompt = Prompts.create_wp(item_id: st.item_id, subject: st.subject, + prompt = Prompts::WpWriter.create_wp(item_id: st.item_id, subject: st.subject, item: container_path(st.item_file), request: request, project: project_name, types: Helpers.types_for_prompt(types), max: MAX_CREATE_WP, related: related, format_note: format_note) reply = llm(:wp_writer, prompt, session_file: st.session_file).to_s # Only what follows the last `ANSWER:` marker; the writer's own deliberation - # is scratch (Prompts.create_wp). Text with no marker is read whole, so an + # is scratch (Prompts::WpWriter.create_wp). Text with no marker is read whole, so an # answer that skips it still works. answer = Helpers.after_marker(reply, "ANSWER") @@ -504,7 +504,7 @@ def payloads_accepted?(st, payloads, types) # customer?"), a work package can never be deleted, and a guess would be # permanent. So the fields are named back to the reader, who can create it in # OpenProject or NAME A DIFFERENT TYPE — required-ness is per type, and their - # answer lands in this thread, which the next draft reads (Prompts.create_wp's + # answer lands in this thread, which the next draft reads (Prompts::WpWriter.create_wp's # TYPE line). Choosing another type here instead would be opilot re-classifying # somebody's work to get past a validation, on a work package nobody can delete. # @@ -754,7 +754,7 @@ def already_created_note(records) # # The shape is always STATED, never implied: whether an offshoot is a child of # this work package or a peer beside it is the writer's per-block decision - # (Prompts.create_wp's LINK line), so the reader cannot work it out from the + # (Prompts::WpWriter.create_wp's LINK line), so the reader cannot work it out from the # count and must be told. def single_notes(record) notes = +"" @@ -882,7 +882,7 @@ def produce_plan(st, feedback, allow_options: false, retry_bad_options: true) if feedback && !feedback.empty? && st.plan_file.exist? log_script "Writer: revising plan for #{wp_label(st.item_id)} from feedback" - prompt = Prompts.replan(repos_summary: @ctx.repos.summary, repos: menu, item: item_c, plan: plan_c, + prompt = Prompts::Planner.replan(repos_summary: @ctx.repos.summary, repos: menu, item: item_c, plan: plan_c, feedback: feedback, item_id: st.item_id, title: st.subject, resumed: session_resumable?(st), related: related, op_mcp: @ctx.op_mcp?) llm(:planner, prompt, outfile: st.plan_file, session_file: st.session_file) @@ -891,7 +891,7 @@ def produce_plan(st, feedback, allow_options: false, retry_bad_options: true) end log_script "Writer: generating plan for #{wp_label(st.item_id)} — #{st.subject}" - prompt = Prompts.plan(repos_summary: @ctx.repos.summary, repos: menu, item: item_c, + prompt = Prompts::Planner.plan(repos_summary: @ctx.repos.summary, repos: menu, item: item_c, item_id: st.item_id, title: st.subject, hint: feedback.to_s, related: related, allow_options: allow_options, op_mcp: @ctx.op_mcp?) llm(:planner, prompt, outfile: st.plan_file, session_file: st.session_file) diff --git a/lib/opilot/appsignal_runner.rb b/lib/opilot/appsignal_runner.rb index d458a52..255758b 100644 --- a/lib/opilot/appsignal_runner.rb +++ b/lib/opilot/appsignal_runner.rb @@ -197,7 +197,7 @@ def drafted_work_package(dir, number) # request rather than a duplicate work package. def write_work_package(number, incident_file, retry_bad: true, format_note: nil) log_script "Drafting a work package from AppSignal incident ##{number}…" - prompt = Prompts.appsignal_wp( + prompt = Prompts::Triager.appsignal_wp( incident: container_path(incident_file), number: number, app: @app, repos: repos_for_prompt(@ctx.repos.all), types: Helpers.types_for_prompt(project_types), format_note: format_note ) diff --git a/lib/opilot/chat_runner.rb b/lib/opilot/chat_runner.rb index 0203487..065577e 100644 --- a/lib/opilot/chat_runner.rb +++ b/lib/opilot/chat_runner.rb @@ -55,7 +55,7 @@ def run(initial_message = nil) prompt = if oriented pending else - Prompts.free_chat(state: @ctx.state_container, wp_root: wp_root, repos: repos, + Prompts::Advisor.free_chat(state: @ctx.state_container, wp_root: wp_root, repos: repos, message: pending, op_mcp: @ctx.op_mcp?, gh_mcp: @ctx.gh_mcp?) end llm(:advisor, prompt, session_file: session_file) diff --git a/lib/opilot/fix_runner.rb b/lib/opilot/fix_runner.rb index 1e3f3f4..95dcdd3 100644 --- a/lib/opilot/fix_runner.rb +++ b/lib/opilot/fix_runner.rb @@ -119,7 +119,7 @@ def process_item(item_data, mode:) # the revised plan. Branch checkout waits until #ship. llm( :planner, - Prompts.replan(repos_summary: @ctx.repos.summary, repos: repos_for_prompt(@ctx.repos.all), + Prompts::Planner.replan(repos_summary: @ctx.repos.summary, repos: repos_for_prompt(@ctx.repos.all), item: container_path(st.item_file), plan: container_path(st.plan_file), feedback: replan_feedback, item_id: id, title: subject, resumed: session_resumable?(st), @@ -139,7 +139,7 @@ def process_item(item_data, mode:) # Pass session_file so a prior chat's context carries into the (re-)plan. llm( :planner, - Prompts.plan(repos_summary: @ctx.repos.summary, repos: repos_for_prompt(@ctx.repos.all), + Prompts::Planner.plan(repos_summary: @ctx.repos.summary, repos: repos_for_prompt(@ctx.repos.all), item: container_path(st.item_file), item_id: id, title: subject, hint: option_focus.to_s, related: related_ref(st), allow_options: option_focus.nil?, op_mcp: @ctx.op_mcp?), @@ -332,7 +332,7 @@ def run_chat(st) prompt = if oriented msg else - Prompts.plan_chat( + Prompts::Advisor.plan_chat( item_id: st.item_id, subject: st.subject, item: container_path(st.item_file), plan: plan_ref, message: msg ) diff --git a/lib/opilot/gh_agent.rb b/lib/opilot/gh_agent.rb index 6e72e64..b28cef9 100644 --- a/lib/opilot/gh_agent.rb +++ b/lib/opilot/gh_agent.rb @@ -143,7 +143,7 @@ def handle_review(intent) @github.fetch_branch(head_repo(intent), branch: intent.branch, worktree_path: repo.worktree_host) checkout_pr_branch(repo, intent.branch) - prompt = Prompts.pr_review( + prompt = Prompts::PrAdvisor.pr_review( repo: intent.repo, pr_number: intent.pr_number, title: intent.subject, worktree: repo.worktree_container, base: repo.base, pr_thread: container_path(pr_file), comment: intent.text.to_s, author: intent.user_login.to_s, @@ -252,7 +252,7 @@ def run_on_pr_head(intent) def handle_own(intent) run_on_pr_head(intent) do |p| - Prompts.gh_reply( + Prompts::PrAuthor.gh_reply( worktree: p.repo.worktree_container, repo: intent.repo, pr_number: intent.pr_number, title: intent.subject, item: p.item_ref, plan: p.plan_ref, pr_thread: container_path(p.pr_file), comment: intent.text.to_s, @@ -267,7 +267,7 @@ def handle_own(intent) # it in the worktree, then commit and push to update the draft PR. def handle_ci(intent) run_on_pr_head(intent) do |p| - Prompts.fix_ci( + Prompts::PrAuthor.fix_ci( op_mcp: @ctx.op_mcp?, worktree: p.repo.worktree_container, repo: intent.repo, pr_number: intent.pr_number, title: intent.subject, item: p.item_ref, plan: p.plan_ref, diff --git a/lib/opilot/health_check.rb b/lib/opilot/health_check.rb index 7ec471b..2987328 100644 --- a/lib/opilot/health_check.rb +++ b/lib/opilot/health_check.rb @@ -56,7 +56,7 @@ def run(item_id, focus: "", internal: true) facts_file = st.item_dir / "health.json" facts_file.write(JSON.pretty_generate(facts)) - prompt = Prompts.health(item_id: st.item_id, subject: st.subject, + prompt = Prompts::Auditor.health(item_id: st.item_id, subject: st.subject, item: container_path(st.item_file), facts: container_path(facts_file), related: related_path, descendants: tree_ref, focus: focus, internal: internal, op_mcp: @ctx.op_mcp?) diff --git a/lib/opilot/helpers.rb b/lib/opilot/helpers.rb index a08dc0b..806dc8b 100644 --- a/lib/opilot/helpers.rb +++ b/lib/opilot/helpers.rb @@ -183,7 +183,7 @@ def self.parse_leading_options(body) WP_LINKS = %w[child related].freeze DEFAULT_WP_LINK = "related".freeze - # Read the work packages a `create wp` answer asks for (Prompts.create_wp). + # Read the work packages a `create wp` answer asks for (Prompts::WpWriter.create_wp). # Each one is a block — BEGIN WORK PACKAGE, a SUBJECT: line, an optional # TYPE: and LINK: line, the description, END WORK PACKAGE — and a request # that names several pieces of work answers with several blocks, in order. @@ -930,6 +930,9 @@ def ensure_harness! def llm(role, prompt, session_file: nil, outfile: nil) r = Harness.role(role) raise ArgumentError, "role #{r.name} is stateless" if r.stateless && session_file + if prompt.is_a?(Prompts::Prompt) && prompt.role != r.name + raise ArgumentError, "a #{prompt.role} prompt sent as #{r.name}" + end opts = { tools: r.tools(@ctx), model: r.model, session_file: session_file } outfile ? @harness.capture(prompt, outfile: outfile, **opts) : @harness.run(prompt, **opts) end @@ -1319,7 +1322,7 @@ def implement_plan(st) unless st.repos.all? { |r| branch_has_commits?(st, r) } log_script "Implementing #{wp_label(st.item_id)} in #{st.repos.map(&:name).join(", ")}" llm(:implementer, - Prompts.implement(repos: repos_for_prompt(st.repos), plan: container_path(st.plan_file), + Prompts::Implementer.implement(repos: repos_for_prompt(st.repos), plan: container_path(st.plan_file), resumed: session_resumable?(st)), session_file: st.session_file) st.repos.each { |r| commit(st, r) } @@ -1344,7 +1347,7 @@ def commit(st, repo) # on any failure so the caller can fall back to a generic subject. Shared by # gh-agent's follow-up commits and the terminal `pr` refresh. def generate_commit_subject(diff) - prompt = Prompts.commit_subject(diff: diff.patch.to_s[0, 6000]) + prompt = Prompts::Scribe.commit_subject(diff: diff.patch.to_s[0, 6000]) reply = llm(:scribe, prompt) strip_ansi(reply.to_s).lines.map(&:strip).find { |l| !l.empty? }.to_s .gsub(/\A["'`]+|["'`]+\z/, "") # strip wrapping quotes/backticks @@ -1368,7 +1371,7 @@ def generate_pr_description(st, repo) diff_stat = wt.diff("HEAD~1", "HEAD").stats[:files] .map { |f, s| " #{f} | +#{s[:insertions]} -#{s[:deletions]}" } .join("\n") - prompt = Prompts.pr_description( + prompt = Prompts::Scribe.pr_description( item: container_path(st.item_file), plan: container_path(st.plan_file), diff_stat: diff_stat, template_section: template_section ) diff --git a/lib/opilot/pd/CLAUDE.md b/lib/opilot/pd/CLAUDE.md index a427e50..3e7b41b 100644 --- a/lib/opilot/pd/CLAUDE.md +++ b/lib/opilot/pd/CLAUDE.md @@ -71,7 +71,7 @@ PR needs the store's layout on every agent tick. `MAX_VALIDATE_ATTEMPTS` (2) revisions in the same session — the CLI isn't in the harness container, so "the agent iterates on its own output" is a runner-driven re-prompt loop. Intake spanning more than one atomic feature returns `TOO_BROAD` - with a suggested split (mirroring `Prompts.plan`'s `NEEDS_INFO`), since one change + with a suggested split (mirroring `Prompts::Planner.plan`'s `NEEDS_INFO`), since one change becomes exactly one FEATURE. **The write scope is enforced** (`#enforce_write_scope!`) — a planning stage must @@ -118,7 +118,7 @@ PR needs the store's layout on every agent tick. change in one pass produces the unreviewable PR the decomposition exists to avoid. Change and repo are resolved *from* the id via `reverse_index` (`--repo` narrows it); an unbound id lists what *is* bound instead of just failing. - `Prompts.implement_task` carries the spec paths, the WP's `item.json` (a human + `Prompts::Implementer.implement_task` carries the spec paths, the WP's `item.json` (a human comment added after the proposal qualifies or overrides it) and **only this section's checklist** — a sibling section is another work package's PR. The write scope is `propose`'s mirror image: there the spec was output and source off limits; @@ -210,7 +210,7 @@ produced a proposal with none of the template's four required headings and still passed validation. So `PD::Runner#artifact_instructions` calls `openspec instructions --change ` for each of `proposal`/`specs`/`design`/ `tasks` (dependency order — proposal `` the rest) and drops the result -straight into `Prompts.propose`. Each block carries the CLI's own ``, +straight into `Prompts::SpecWriter.propose`. Each block carries the CLI's own ``, ``, `