fix(skills): resolve the ten contract gaps (ADR 0011)

.changeset/resolve-contract-gaps.md

@@ -0,0 +1,13 @@
+---
+"github-agent-workflow": minor
+---
+
+Fixes the ten gaps found while writing the skill contracts (ADR 0011).
+
+New inputs: `write_log` (default `true`) on the four item skills, so a dispatched item no longer writes a second run-log entry; `lease_held` (default `false`), so a nested call can borrow its caller's lease; and `limit` on `file-github-issues`. Group skills now pass the item its URL, `write_log=false` and every group input the item also accepts, where a sub-agent used to get only the URL and lose `runner`, `unattended` and `trusted_authors`.
+
+New results: `#N noop: <reason>` (claimed, nothing to do, released), `#N not ready: <reason>` from `review-github-pr`, and `deferred: over limit` from `file-github-issues`. `skipped` now always means nothing was claimed.
+
+`shepherd-github-pr` now treats a conflicting branch as work whatever its checks say, and reads the merge lane and merge state before claiming, so a PR that can only wait no longer collects a lease comment on every run. Eligibility checks only read: `clear_stale_lease` is no longer part of them.
+
+New skill `reconcile-github-issues`, which takes over the housekeeping that `triage-github-issues` did (closing issues whose PR merged, abandoned PRs, label repair) and clears stale lease labels. The rule for what an issue becomes after its PRs settle is now one reference, `references/issue-after-pr.md`. `triage-github-issue` reconciles a prior trusted outcome before it decides, not after. `explore_batch` is always `5`; it no longer drops to `3` on a "large" backlog.

.claude-plugin/plugin.json

@@ -31,6 +31,7 @@
     "./skills/pipeline/file-github-issues",
     "./skills/pipeline/implement-github-issue",
     "./skills/pipeline/implement-github-issues",
+    "./skills/pipeline/reconcile-github-issues",
     "./skills/pipeline/review-github-pr",
     "./skills/pipeline/review-github-prs",
     "./skills/pipeline/shepherd-github-pr",

README.md

@@ -41,7 +41,7 @@ Cross-cutting skills that support the pipeline but are not steps themselves.
 ### GitHub workflow ([skills/pipeline](./skills/pipeline/README.md))
 
 - **[explore-github-repo](./skills/pipeline/explore-github-repo/SKILL.md)** / **[file-github-issues](./skills/pipeline/file-github-issues/SKILL.md)** — explore the live app and code, rank findings, file the best as issues (batch + dry-run supported).
-- **[triage-github-issues](./skills/pipeline/triage-github-issues/SKILL.md)** / **[triage-github-issue](./skills/pipeline/triage-github-issue/SKILL.md)** — scan open issues and triage each to `ready-for-agent`, `ready-for-human` or `needs-info`.
+- **[triage-github-issues](./skills/pipeline/triage-github-issues/SKILL.md)** / **[triage-github-issue](./skills/pipeline/triage-github-issue/SKILL.md)** — scan open issues and triage each to `ready-for-agent`, `ready-for-human` or `needs-info`. **[reconcile-github-issues](./skills/pipeline/reconcile-github-issues/SKILL.md)** tidies issues whose PRs finished and repairs state labels.
 - **[implement-github-issues](./skills/pipeline/implement-github-issues/SKILL.md)** / **[implement-github-issue](./skills/pipeline/implement-github-issue/SKILL.md)** — pick ready-for-agent issues, implement, open a PR.
 - **[shepherd-github-prs](./skills/pipeline/shepherd-github-prs/SKILL.md)** / **[shepherd-github-pr](./skills/pipeline/shepherd-github-pr/SKILL.md)** — get a PR through checks and post the review handoff.
 - **[review-github-prs](./skills/pipeline/review-github-prs/SKILL.md)** / **[review-github-pr](./skills/pipeline/review-github-pr/SKILL.md)** — review a ready PR and merge when safe.

docs/adr/0010-skills-as-functions-with-contracts.md

@@ -28,7 +28,7 @@ This change keeps behaviour as it is. The contracts describe what the skills do
 - Each skill grows by 20 to 30 lines net, after the prose sections the contract replaces are removed. That is the cost of having one source of truth.
 - Only pipeline skills have contracts so far. Utility skills follow.
 
-## Known gaps, not changed here
+## Known gaps, not changed here (resolved in ADR 0011)
 
 Writing the contracts down made these visible. Each is a behaviour change, so each is its own change.
 

docs/adr/0011-resolve-the-contract-gaps.md

@@ -0,0 +1,30 @@
+# 0011. Resolve the gaps the contracts exposed
+
+## Status
+
+Accepted
+
+## Context
+
+Writing a contract for each pipeline skill (ADR 0010) showed where a skill's behaviour was hard to state as a function. ADR 0010 listed ten gaps and left them alone so the contracts could describe today's behaviour first. This ADR records how each was resolved.
+
+## Decisions
+
+| # | Gap | Resolution |
+|---|---|---|
+| 1 | An item skill's own run-log entry depended on whether it was dispatched, which a sub-agent could not tell. It also lost every other input of the group run | New input `write_log` (default `true`). Dispatching passes the URL, `write_log=false`, and every group input the item also lists in `params`. The validator checks a dispatched item that logs accepts `write_log` |
+| 2 | `skipped` meant both "nothing happened" and "claimed, then gave up". `review-github-pr` step 4 had no result line | `skipped`: nothing claimed, nothing written. New `noop`: claimed, found nothing to do, released. New `#N not ready: <reason>` for review step 4 |
+| 3 | `file-github-issues` mentioned a limit with no input | New input `limit`. Past it, candidates return `deferred: over limit` unchecked. `explore-github-repo` passes `limit=explore_batch` |
+| 4 | `explore_batch` defaulted to 3 or 5 depending on an undefined "large" backlog | Always `5` |
+| 5 | Eligibility called `clear_stale_lease`, which writes | Eligibility only reads. A stale label does not count as a lease and the next `claim` replaces it. The validator rejects a `requires` that names a writing helper |
+| 6 | `triage-github-issues` changed issues it had not claimed | New skill `reconcile-github-issues` (`kind: sweep`). It lists what it may change, takes `dry_run`, and also clears stale lease labels. `triage-github-issues` calls it |
+| 7 | "Close the issue after a merge unless keep-open" was written in three places, "abandoned PR" in two | `references/issue-after-pr.md` holds the rule. Callers state only what they add. The label repair priority is read from `labels.md` instead of copied |
+| 8 | `triage-github-issue` reconciled a prior outcome after the costly decision | Moved to step 4, before the format check and the decision |
+| 9 | Two suspected bugs: a conflicted PR often runs no checks and was skipped as `no-checks-yet`, and a PR waiting on the merge lane collected a lease comment on every run | A `DIRTY` branch is eligible whatever its checks say. Eligibility reads the lane and merge state before claiming, so a PR that can only wait is skipped without a claim. A lane taken after the claim returns `noop` |
+| 10 | `lease_live` ignores who holds the lease, so a nested call would skip its own caller's item | New input `lease_held` (default `false`). When `true`, `claim` and `release` do nothing and the lease must be live |
+
+## Consequences
+
+- Behaviour changes for anyone running these skills: three new inputs (`write_log`, `lease_held`, `limit`), a new skill, new result lines (`noop`, `not ready`, `deferred: over limit`), and a different `explore_batch` default when the backlog was large.
+- Gap 9 is a fix for a problem inferred from reading the steps, not seen in a run. A scenario test with a conflicted PR and no checks should confirm it, and the same fixture can guard against a regression.
+- `lease_held` is the first piece of what a skill that calls itself needs. A skill that actually recurses also needs a depth bound, and children that cannot split again.

scripts/contracts.mjs

@@ -10,11 +10,11 @@
 import { parse } from "yaml";
 
 export const STEPS = ["Exploring", "Triaging", "Implementing", "Shepherding", "Reviewing"];
-export const KINDS = ["item", "group"];
+export const KINDS = ["item", "group", "sweep"];
 export const WRITE_SCOPES = ["claimed", "repository", "other", "shared"];
 
 const COMMON = ["kind", "step", "params", "requires", "returns", "ensures", "effects", "never", "calls", "idempotent", "terminates"];
-const REQUIRED = { item: ["target", ...COMMON], group: ["dispatches", ...COMMON] };
+const REQUIRED = { item: ["target", ...COMMON], group: ["dispatches", ...COMMON], sweep: [...COMMON] };
 const ALLOWED = new Set([...COMMON, "target", "types", "dispatches"]);
 
 // Prose sections that the contract replaces. A skill that still has one has two sources of truth.
@@ -76,7 +76,7 @@ export function checkContracts(skills, byName, external = new Set()) {
     if (!KINDS.includes(c.kind)) { err(`kind must be one of ${KINDS.join(", ")}`); continue; }
     if (!STEPS.includes(c.step)) err(`step must be one of ${STEPS.join(", ")}`);
     for (const k of REQUIRED[c.kind]) if (c[k] === undefined) err(`missing \`${k}\``);
-    if (c.kind === "item" && c.dispatches !== undefined) err("an item skill does not dispatch");
+    if (c.kind !== "group" && c.dispatches !== undefined) err("only a group skill dispatches");
 
     // params are the Inputs table
     if (!isStrings(c.params) && !(Array.isArray(c.params) && c.params.length === 0)) err("`params` must be a list of names");
@@ -95,6 +95,11 @@ export function checkContracts(skills, byName, external = new Set()) {
     }
     const argTarget = c.target?.param ? c.target.param : null;
 
+    // eligibility only reads: a precondition that calls a writing helper is a hidden side effect
+    for (const r of Array.isArray(c.requires) ? c.requires : []) {
+      const w = typeof r === "string" && r.match(/\b(clear_stale_lease|claim|renew|release|fail_item|log_run)\b/);
+      if (w) err(`\`requires\` mentions \`${w[1]}\`, which writes. Checking eligibility must only read`);
+    }
     for (const k of ["requires", "returns", "ensures", "never"]) {
       if (c[k] !== undefined && !isStrings(c[k])) err(`\`${k}\` must be a non-empty list of strings`);
     }
@@ -114,7 +119,7 @@ export function checkContracts(skills, byName, external = new Set()) {
             if (!WRITE_SCOPES.includes(scope)) err(`effects.writes scope \`${scope}\` is not one of ${WRITE_SCOPES.join(", ")}`);
             else if (!isStrings(list)) err(`effects.writes.${scope} must be a non-empty list of strings`);
           }
-          if (c.kind === "group" && e.writes.claimed) err("a group skill never claims, so it has no `effects.writes.claimed`");
+          if (c.kind !== "item" && e.writes.claimed) err("only an item skill claims, so a group or sweep skill has no `effects.writes.claimed`");
           if (c.kind === "item" && argTarget && !e.writes.claimed) err("an item skill that is given an issue or PR claims it, so it needs `effects.writes.claimed`");
         }
         if (e.external !== undefined && !isStrings(e.external)) err("`effects.external` must be a non-empty list of strings");
@@ -171,6 +176,9 @@ export function checkContracts(skills, byName, external = new Set()) {
       if (tc.kind !== "item") errors.push(`${s.rel}: contract: dispatches \`${c.dispatches}\`, which is not an item skill`);
       if (tc.step !== c.step) errors.push(`${s.rel}: contract: dispatches \`${c.dispatches}\`, which belongs to ${tc.step} and not ${c.step}`);
       if (t.userInvoked) errors.push(`${s.rel}: contract: dispatches \`${c.dispatches}\`, which is user-invoked`);
+      if (tc.effects?.writes?.shared && !(tc.params ?? []).includes("write_log")) {
+        errors.push(`${s.rel}: contract: dispatches \`${c.dispatches}\`, which writes the run log but has no \`write_log\` param, so the group cannot stop it logging twice`);
+      }
     }
   }
   return { errors, contracts: parsed };

scripts/sync.mjs

@@ -31,7 +31,7 @@ for (const bucket of dirs(join(root, "skills")).sort()) {
 // ---- the pipeline, in order: each step has a group skill and an item skill ---------------------
 const STEPS = [
   { step: "Exploring", group: "explore-github-repo", item: "file-github-issues", itemTarget: null },
-  { step: "Triaging", group: "triage-github-issues", item: "triage-github-issue", itemTarget: "123" },
+  { step: "Triaging", group: "triage-github-issues", item: "triage-github-issue", itemTarget: "123", sweeper: "reconcile-github-issues" },
   { step: "Implementing", group: "implement-github-issues", item: "implement-github-issue", itemTarget: "123" },
   { step: "Shepherding", group: "shepherd-github-prs", item: "shepherd-github-pr", itemTarget: "124" },
   { step: "Reviewing", group: "review-github-prs", item: "review-github-pr", itemTarget: "124" },
@@ -63,7 +63,7 @@ function inputs(skill) {
 }
 
 function promptsMd() {
-  const covered = new Set(STEPS.flatMap((s) => [s.group, s.item]));
+  const covered = new Set(STEPS.flatMap((s) => [s.group, s.item, s.sweeper].filter(Boolean)));
   for (const s of promoted.filter((x) => x.bucket === "pipeline")) {
     if (!covered.has(s.name)) throw new Error(`scripts/sync.mjs: pipeline skill ${s.name} is not in STEPS`);
   }
@@ -79,9 +79,11 @@ function promptsMd() {
   ];
   for (const s of STEPS) {
     out.push(`## ${s.step}`, "");
-    for (const [kind, name, target] of [["Group skill, run on a schedule", s.group, null], ["Item skill, run by hand", s.item, s.itemTarget]]) {
+    const skills = [["Group skill, run on a schedule", s.group, null], ["Item skill, run by hand", s.item, s.itemTarget]];
+    if (s.sweeper) skills.push(["Sweeper skill, run on a schedule or by hand", s.sweeper, null]);
+    for (const [kind, name, target] of skills) {
       const rows = inputs(name);
-      const head = kind.startsWith("Group") ? `Run /${name} on github.com/OWNER/REPO` : target ? `Run /${name} ${target}` : `Run /${name}`;
+      const head = kind.startsWith("Group") || kind.startsWith("Sweeper") ? `Run /${name} on github.com/OWNER/REPO` : target ? `Run /${name} ${target}` : `Run /${name}`;
       out.push(`### ${kind}: \`${name}\``, "");
       if (NOTES[name]) out.push(NOTES[name], "");
       out.push("```", head, ...rows.map((r) => `${r.key}=${r.def}`), "```", "");