diff --git a/.changeset/split-an-issue-that-is-too-large.md b/.changeset/split-an-issue-that-is-too-large.md new file mode 100644 index 0000000..bfa23da --- /dev/null +++ b/.changeset/split-an-issue-that-is-too-large.md @@ -0,0 +1,5 @@ +--- +"github-agent-workflow": minor +--- + +`implement-github-issue` splits an issue that is too large for one PR into linked sub-issues (`#N split into #A, #B`), bounded by the new `max_depth` input (default `1`). `file-github-issues` takes a `parent`, `reconcile-github-issues` closes the parent when its sub-issues close, and the scenario checker rejects `lease_held=true` from a skill that does not hold the lease. See ADR 0015. diff --git a/docs/RUNNERS.md b/docs/RUNNERS.md index da0411d..c14fe9f 100644 --- a/docs/RUNNERS.md +++ b/docs/RUNNERS.md @@ -62,6 +62,8 @@ Three inputs are mostly for skills that call other skills, and a scheduled promp | `lease_held` | the **Defaults** table, and the item skills | A nested call that borrows its caller's lease | | `limit` | `file-github-issues` | Capping one call. `explore-github-repo` passes `explore_batch` as `limit`, so set `explore_batch` on a schedule instead | +`max_depth` (default `1`) is a setting, not plumbing: it bounds how deep `implement-github-issue` splits an issue that is too large into sub-issues, and `0` turns splitting off. Splitting uses GitHub's sub-issue links, so the token needs write access to issues. Without them the issue is never split. See `references/split.md` in the protocol. + ## Cadence (starting point) - Exploring: a few times a week diff --git a/docs/adr/0014-gh-simulator.md b/docs/adr/0014-gh-simulator.md index 268f189..a7efa36 100644 --- a/docs/adr/0014-gh-simulator.md +++ b/docs/adr/0014-gh-simulator.md @@ -39,7 +39,7 @@ ADR 0012 checks a scenario against a skill's contract, and `diffWorlds` turns th ### `api` (#88) -`gh api` answers the nine REST calls the package makes, and refuses everything else as `unsupported` (graphql, another method on a path, `-F @file`, `--input`): +`gh api` answers the REST calls the package makes, and refuses everything else as `unsupported` (graphql, another method on a path, `-F @file`, `--input`): | call | answered from | | --- | --- | @@ -50,10 +50,14 @@ ADR 0012 checks a scenario against a skill's contract, and `diffWorlds` turns th | `GET .../contents/PATH?ref=` | the world's `files` (raw with the raw `Accept` header, otherwise a base64 object) | | `GET .../git/trees/HEAD?recursive=1` | the paths of the world's `files` | | `GET .../pulls?state=&per_page=&page=` and `GET .../pulls/N` | the world's PRs as REST pull requests, newest first. `author_association` is the PR's `association` (default `NONE`) | +| `GET .../issues/N` | the issue as REST shows it, with an `id` (200000 + its number) | +| `GET .../issues/N/parent`, `GET .../issues/N/sub_issues` | the world's `parent` field of an issue, and the issues whose `parent` is N. An issue with no parent is a 404, which `parent_of` relies on | +| `POST .../issues/P/sub_issues -F sub_issue_id=ID` | sets the child's `parent`. GitHub's refusals are kept: one parent only, not itself, no loops, the id must be an issue's | | `PUT .../pulls/N/update-branch -f expected_head_sha=` | gives a `BEHIND` PR a new head and makes it `CLEAN`. A stale sha, a conflict and a PR that is not behind are 422, as on GitHub | - **A comment edited in place is an effect.** Since the lease comment of a step is edited rather than added, the checker's effect model gained `comment_edit` (the comment's index and its new text), scoped like `comment`. `diffWorlds` reads an edited comment as one, and `applyEffects` makes it. - **A comment the simulator derives from a world field (a `lease`, a `readySha`) cannot be edited.** There is no stored comment to change, so the call is `unsupported` and `claim` falls back to posting a comment, as it does when an edit fails. +- **Linking a sub-issue the run filed is not `unmodelled`.** A new issue's `parent` is creation data, so `diffWorlds` puts it in the `create_issue` effect's `data` (`references/split.md` splits an issue this way). Linking an issue that already existed has no effect that can express it, and is `unmodelled`. - **Pagination is not modelled** beyond `per_page` and `page` on the PR list. `--paginate` returns a list whole. ### Faults (#88) diff --git a/docs/adr/0015-split-an-issue-that-is-too-large.md b/docs/adr/0015-split-an-issue-that-is-too-large.md new file mode 100644 index 0000000..61fce68 --- /dev/null +++ b/docs/adr/0015-split-an-issue-that-is-too-large.md @@ -0,0 +1,49 @@ +# 0015. An issue that is too large is split into sub-issues + +## Status + +Accepted + +## Context + +A maintainer files an issue that is clearly bigger than one PR. `implement-github-issue` cannot make it smaller. The most it could do was bail out to `needs-info` or `ready-for-human`, and a person had to split the issue by hand before the pipeline could continue. + +ADR 0011 added `lease_held` so that a skill could call an item skill on an item it already holds, and noted what a skill that really recurses would also need: a depth bound, and children that cannot split again. Nothing used it. "A skill can call itself" was untested design. + +Splitting is a good first use of recursion because the shape is natural. The parent files children, each child is an ordinary issue that the pipeline picks up on its own, and the only question is when to stop. + +## Decision + +**Trigger.** `implement-github-issue` decides that an issue is too large in step 4, after it has read the issue and before it writes code. Both must hold: the issue divides into two or more parts that each merge alone, and one PR is too much (`Size: large`, or separate areas, or a first read of the code). A hard issue is not too large, an indivisible one is not too large, and an issue with a branch already on origin is not split. Only size splits an issue. The rule is in `references/split.md`. + +**Depth bound.** New input `max_depth`, default `1`. The depth of an issue is the number of its ancestors, read from GitHub's sub-issue links (`depth_of`). An issue is split only when its depth is less than `max_depth`. With the default a sub-issue is never split again, and `max_depth=0` turns splitting off. If the depth cannot be read, the issue is not split. An issue that is too large and at the limit goes to `ready-for-human` with its proposed slices, so a person can split it. + +**Linking.** Slices are filed through `file-github-issues`, which takes a new `parent` input: the parent is never reported as a duplicate of a slice, and each filed body says `Part of #N.` for the reader. The link is GitHub's native sub-issue relationship, made by `implement-github-issue` with `link_sub_issue` after each `filed #X`. Native links need write access, so a stranger cannot forge one, which the text `Part of #N.` does not guarantee. A slice that is a duplicate of an issue that is not already a sub-issue of the parent is not linked, and the run fails instead of adopting someone else's issue. + +**Result.** New result `#N split into #A, #B`. The parent stays open and keeps `ready-for-agent`. It does not get `agent:pr-open`, because no PR is open. + +**Split marker.** After every slice is filed and linked, the parent gets one comment ending in ``. A parent with a trusted marker is split, and `implement-github-issue` does not take it (a new `requires`), so a split parent does not use up the batch of `implement-github-issues`. A parent with sub-issues and no marker is a split that died partway, and the next run files only the slices that are missing before it writes the marker. The marker is the commit point. + +**Closing.** `reconcile-github-issues` settles a split parent: every sub-issue closed as completed closes the parent (`#N closed: all sub-issues closed`), unless it is keep-open (`#N kept open: all sub-issues closed`). Any sub-issue closed as not planned sends the parent to `ready-for-human` (`#N ready-for-human: a sub-issue was closed as not planned`) and never back to an agent, as with an abandoned PR. An open sub-issue leaves the parent alone. + +**Leases.** The parent holds `agent:in-progress` while it files. A call into an item skill on the same item passes `lease_held=true`. A call about any other item does not, because the caller does not hold that item's lease. The split makes no call of the first kind: the children are other items, and `file-github-issues` takes no lease. The rule is stated under **Leases** in the protocol (Nested calls) so that a later nested call is safe, and the scenario checker enforces it (see below). + +**Termination.** Stated in the contract's `terminates`: a split happens only below `max_depth`, and a child is one level deeper than its parent. The checker also rejects any run that creates an issue deeper than `max_depth`. + +**Checker.** A scenario may list the calls a run makes with their inputs (`calls: [{skill, target, inputs}]`). The checker rejects `lease_held=true` unless the caller claimed the lease on that item in this run (or was itself given `lease_held=true` for it), the call is about that same item, and the callee is an item skill that lists `lease_held`. It rejects a split below the depth limit, a split that is not recorded with the marker, an issue created under a parent that is not the split item, and a parent closed while a sub-issue is open. A group skill never passes `lease_held`, because it holds no lease. + +## Alternatives considered + +- **A new state label such as `agent:split`.** It would make the state visible on the board. It also adds a fifth label to the mutex set, to setup, to label repair and to the lifecycle diagram, and the marker plus the sub-issue links already hold the state. A person can see the sub-issues on the parent. Revisit if people look for split parents by label. +- **Linking by text in the body (`Part of #N`).** Anyone can write it, and a stranger could use it to hold a parent open. Rejected for linking and kept only as a human-readable line. +- **Recursion through `implement-github-issue` on each child.** The children are other items with their own leases and their own triage. Calling them directly would skip triage and make `lease_held` meaningless. They are picked up through the normal pipeline instead. +- **Closing the parent when all children are closed, whatever the reason.** A child that was rejected would then close a parent whose work was dropped, silently. The abandoned-PR rule (never send it back to an agent, let a person decide) is reused. + +## Consequences + +- New inputs `max_depth` (on `implement-github-issue` and `implement-github-issues`) and `parent` (on `file-github-issues`). New result lines on `implement-github-issue` and `reconcile-github-issues`. New helpers `parent_of`, `children_of`, `depth_of` and `link_sub_issue`, and a new marker `agent:split`. +- `implement-github-issue` gains a write scope, `other`: new issues, and a call to `file-github-issues`. +- Sub-issues need GitHub's sub-issue feature. A repository or runner without it cannot read depth, so it never splits, and behaves as it did before this change. +- A sub-issue that `triage-github-issue` rates `ready-for-human` or `needs-info` leaves its parent waiting. That is the intended place for a person to step in. +- A run that dies after filing a slice and before linking it leaves an unlinked issue. The next run finds it as a duplicate, and fails rather than adopting it. A person links it or closes it. This is rare, and the failure count escalates it. +- What this does not prove: the scenarios check that the contract, the checker and the expected outcomes agree. They do not show that a model judges "too large" well. The trigger is a judgement, written down so a reviewer can disagree with it. diff --git a/scripts/scenario.mjs b/scripts/scenario.mjs index 346ad62..0c0a407 100644 --- a/scripts/scenario.mjs +++ b/scripts/scenario.mjs @@ -14,6 +14,7 @@ import { isDeepStrictEqual } from "node:util"; import { readFileSync } from "node:fs"; import { contractOf } from "./skill-contracts.mjs"; +import { canSplit, depthOf } from "../skills/meta/agent-workflow-protocol/scripts/helpers.mjs"; export const MUTEX_ISSUE = ["needs-info", "ready-for-agent", "ready-for-human", "agent:pr-open"]; export const MUTEX_PR = ["agent:needs-work", "agent:ready-for-review", "agent:human-merge"]; @@ -113,10 +114,25 @@ export function leaseProblem(it, leaseLabels, inputs = {}) { return held ? "another run holds a live lease" : null; } +/** True when a trusted comment on the item carries the marker. A world comment is `{marker, trusted?, text?}`, trusted unless it says `trusted: false`: who wrote a comment is part of the world, and a run only adds its own. */ +export const hasTrustedMarker = (it, m) => (it.comments ?? []).some((c) => c.marker === m && c.trusted !== false); + +/** The issues that are sub-issues of `n` in the world: those whose `parent` is `n`. */ +export const childrenOf = (world, n) => (world.issues ?? []).filter((i) => i.parent === n); + +/** + * How many ancestors issue `n` has in the world, from each issue's `parent` (null for none). Null when a link + * leads to an issue the world does not hold or loops: that depth could not be read, and an unread depth never splits. + */ +export function worldDepth(world, n) { + const parents = Object.fromEntries((world.issues ?? []).map((i) => [i.number, i.parent ?? null])); + try { return depthOf(n, parents); } catch { return null; } +} + /** How many `requires` entries each oracle mirrors. A test fails when a skill's count differs. */ export const ORACLE_REQUIRES = { "triage-github-issue": 3, - "implement-github-issue": 2, + "implement-github-issue": 3, "review-github-pr": 2, "shepherd-github-pr": 4, }; @@ -142,6 +158,7 @@ export const ORACLES = { if (issue.state !== "open" || !L.includes("ready-for-agent") || L.includes("agent:stuck")) { return { eligible: false, reason: "not an open ready-for-agent issue" }; } + if (hasTrustedMarker(issue, "agent:split")) return { eligible: false, reason: "already split" }; const lease = leaseProblem(issue, ["agent:in-progress"], inputs); return lease ? { eligible: false, reason: lease } : { eligible: true }; }, @@ -201,9 +218,9 @@ export function createdItem(w, e) { /** * What a new item is made with, besides what a `create_*` effect names: its title, its body, when it was created, who - * opened it and (for a PR) its branch. A `create_*` effect carries these as `data`. Editing one later is still `unmodelled`. + * opened it, (for a PR) its branch and (for an issue) the parent it is a sub-issue of. A `create_*` effect carries these as `data`. Editing one later is still `unmodelled`. */ -export const CREATION_DATA = ["title", "body", "createdAt", "author", "branch"]; +export const CREATION_DATA = ["title", "body", "createdAt", "author", "branch", "parent"]; /** Apply effects to a copy of the world. Throws when an effect is impossible. */ export function applyEffects(world, effects) { @@ -235,7 +252,7 @@ export function applyEffects(world, effects) { case "label_remove": if (!t.labels.includes(e.label)) throw new Error(`removes \`${e.label}\` from ${e.target}, which does not carry it`); t.labels = t.labels.filter((l) => l !== e.label); break; - case "comment": t.comments.push({ marker: e.marker, text: e.text }); break; + case "comment": t.comments.push({ marker: e.marker, text: e.text, ...(e.trusted !== undefined ? { trusted: e.trusted } : {}) }); break; // A comment the run edited in place (a lease): `index` is its place among the item's comments, `text` its new text. case "comment_edit": { const c = t.comments[e.index]; @@ -689,7 +706,9 @@ export function checkGroupScenario(contract, sc) { if (got.length === want.length && got.every((t) => want.includes(t)) && got.some((t, i) => t !== want[i])) bad(`dispatches in the wrong order: oldest first is ${want.join(", ")}`); // forwarding and results + if ("lease_held" in inputs) bad("a group skill holds no lease, so its run cannot be given lease_held"); for (const d of dispatches) { + if ("lease_held" in (d.inputs ?? {})) bad(`the dispatch of ${d.target} passes lease_held: a group skill holds no lease and never passes it`); compare(`the dispatch of ${d.target}`, d.inputs ?? {}, fwd, itemName); const n = String(d.target).match(/#(\d+)$/)?.[1]; if (typeof d.result !== "string" || !d.result.startsWith(`#${n} `) || !matchesReturn(d.result, item_.returns)) bad(`${d.target}'s result \`${d.result}\` is not one of ${itemName}'s returns`); @@ -761,6 +780,8 @@ export function checkScenario(contract, sc) { const isItem = contract.kind === "item" && contract.target?.param; const claimed = new Set(isItem && sc.target ? [sc.target] : []); const leaseHeld = sc.inputs?.lease_held === true; + // a call is a skill name, or {skill, target, inputs} when the inputs matter (lease_held) + const calls = (sc.expect.calls ?? []).map((c) => (typeof c === "string" ? { skill: c } : c)); // 1. results const interrupted = sc.interrupted === true; // the pass died partway, so it returned nothing @@ -812,9 +833,58 @@ export function checkScenario(contract, sc) { if (e.op === "close") { const t = item(world, e.target); if (t?.keepOpen) bad(`closes ${e.target}, which is keep-open`); + if (t && ref(e.target).kind === "issue" && hasTrustedMarker(t, "agent:split")) { // references/split.md, When the sub-issues close + const kids = childrenOf(world, t.number); + if (kids.length === 0) bad(`closes ${e.target}, a split issue whose sub-issues cannot be read`); + const open = kids.filter((c) => c.state === "open"); + if (open.length) bad(`closes ${e.target}, a split issue, while sub-issue ${open.map((c) => `#${c.number}`).join(", ")} is open`); + const dropped = kids.filter((c) => c.closeReason === "not planned"); + if (dropped.length) bad(`closes ${e.target}, a split issue, although sub-issue ${dropped.map((c) => `#${c.number}`).join(", ")} was closed as not planned`); + } } } + // splitting an issue that is too large (references/split.md) + const maxDepth = sc.inputs?.max_depth ?? 1; + const made = work.filter((e) => e.op === "create_issue"); + const targetN = isItem && sc.target ? ref(sc.target).n : null; + for (const e of made) { + if (e.data?.parent === undefined) continue; + const d = worldDepth(after, e.number); + if (d === null) bad(`sub-issue #${e.number} has a depth that cannot be read`); + else if (!canSplit(d - 1, maxDepth)) bad(`sub-issue #${e.number} is ${d} deep, past max_depth=${maxDepth}`); + if (isItem && e.data.parent !== targetN) bad(`sub-issue #${e.number} is linked to #${e.data.parent}, not to ${sc.target}, the issue this run split`); + } + if (sc.skill === "implement-github-issue" && isItem) { + const depth = worldDepth(world, targetN); + const may = canSplit(depth, maxDepth); + if (outcome === "split") { + const listed = [...results[0].matchAll(/#(\d+)/g)].map((m) => Number(m[1])).slice(1); + const sorted = (a) => [...a].sort((x, y) => x - y); + if (listed.length < 2) bad("a split makes two or more sub-issues"); + if (!isDeepStrictEqual(sorted(made.map((e) => e.number)), sorted(listed))) bad(`the result names ${JSON.stringify(listed)}, but the run created ${JSON.stringify(made.map((e) => e.number))}`); + for (const e of made) { + if ((e.labels ?? []).length) bad(`sub-issue #${e.number} is filed with labels: ${e.labels.join(", ")}. Triaging picks up an issue with none`); + if (e.data?.parent !== targetN) bad(`sub-issue #${e.number} is not linked to ${sc.target} (\`data.parent\`)`); + } + const mark = work.find((e) => e.op === "comment" && e.target === sc.target && e.marker === "agent:split"); + if (!mark) bad("a split without an `agent:split` comment on the issue"); + else { + const kids = (mark.text ?? "").match(/children=([0-9,]+)/)?.[1]; + if (kids === undefined || !isDeepStrictEqual(sorted(kids.split(",").map(Number)), sorted(listed))) bad(`the \`agent:split\` comment names ${kids ?? "no children"}, not ${listed.join(",")}`); + } + if (work.some((e) => e.op === "label_add" && e.target === sc.target && e.label === "agent:pr-open")) bad("a split issue does not gain agent:pr-open: no PR is open"); + if (work.some((e) => e.op === "label_remove" && e.target === sc.target && e.label === "ready-for-agent")) bad("a split issue keeps ready-for-agent"); + if (work.some((e) => e.op === "create_pr" || e.op === "push")) bad("a split opens no PR and pushes no branch"); + if (!may) bad(`splits ${sc.target}, which is ${depth === null ? "of a depth that cannot be read" : `${depth} deep`} with max_depth=${maxDepth}: only an issue with fewer ancestors than max_depth splits`); + } else if (made.length) bad(`creates issues but the result is \`${results[0]}\`, not a split`); + } + if (sc.skill === "file-github-issues" && sc.inputs?.parent !== undefined) { + const P = Number(sc.inputs.parent); + if (results.some((r) => r === `duplicate of #${P} (commented)`)) bad(`reports a slice as a duplicate of #${P}, the parent it is a slice of`); + for (const e of made) if (!String(e.data?.body ?? "").startsWith(`Part of #${P}.`)) bad(`issue #${e.number} does not start with \`Part of #${P}.\``); + } + // merge rules (review-github-pr) for (const e of work.filter((x) => x.op === "merge")) { const pr = item(world, e.target); @@ -869,7 +939,21 @@ export function checkScenario(contract, sc) { } // skills called must be in the contract's `calls` - for (const c of sc.expect.calls ?? []) if (!(contract.calls ?? []).includes(c)) bad(`calls \`${c}\`, which is not in the contract's calls`); + for (const c of calls) if (!(contract.calls ?? []).includes(c.skill)) bad(`calls \`${c.skill}\`, which is not in the contract's calls`); + + // lease_held=true lends the caller's lease to a nested call (**Nested calls** in the protocol). Only a skill that holds + // the lease on an item may pass it, only for that item, and only to an item skill that lists `lease_held`. + for (const c of calls) { + if (String(c.inputs?.lease_held) !== "true") continue; + const callee = contractOf(c.skill); + if (!isItem) bad(`passes lease_held=true to ${c.skill}, but this skill holds no lease`); + else { + if (outcome === "skipped") bad(`passes lease_held=true to ${c.skill}, but a skipped run claimed nothing, so it holds no lease`); + if (c.target === undefined) bad(`passes lease_held=true to ${c.skill} without saying which item the call is about`); + else if (c.target !== sc.target) bad(`passes lease_held=true to ${c.skill} for ${c.target}, but holds the lease on ${sc.target} only`); + if (!callee || callee.kind !== "item" || !(callee.params ?? []).includes("lease_held")) bad(`passes lease_held=true to ${c.skill}, which is not an item skill that lists \`lease_held\``); + } + } // triage-github-issue: a prior trusted outcome that stands is reused, so unattended-triage is not called for it if (sc.skill === "triage-github-issue" && sc.target) { @@ -877,7 +961,7 @@ export function checkScenario(contract, sc) { const cs = t.comments ?? []; const i = cs.findLastIndex((c) => c.marker === "agent:outcome" && c.trusted); const stands = i !== -1 && !cs.slice(i + 1).some((c) => c.author === "human"); - if (stands && (sc.expect.calls ?? []).includes("unattended-triage")) bad("calls unattended-triage although a prior trusted outcome still stands"); + if (stands && calls.some((c) => c.skill === "unattended-triage")) bad("calls unattended-triage although a prior trusted outcome still stands"); } // 5. rerun: asked again, the skill must find nothing to redo (`idempotent` in the contract) diff --git a/skills/meta/agent-workflow-protocol/SKILL.md b/skills/meta/agent-workflow-protocol/SKILL.md index cce01aa..ad96ffd 100644 --- a/skills/meta/agent-workflow-protocol/SKILL.md +++ b/skills/meta/agent-workflow-protocol/SKILL.md @@ -104,6 +104,7 @@ A key-value pair and free text that name the same thing resolve to the pair. An | `max_shepherd_attempts` | `3` | `shepherd-github-pr` | | `max_review_rounds` | `3` | `review-github-pr` | | `max_item_failures` | `3` | `triage-github-issue`, `implement-github-issue`, `shepherd-github-pr`, `review-github-pr` | +| `max_depth` | `1` | `implement-github-issues`, `implement-github-issue` | | `runner` | `unknown` | `explore-github-repo`, `triage-github-issues`, `triage-github-issue`, `implement-github-issues`, `implement-github-issue`, `shepherd-github-prs`, `shepherd-github-pr`, `review-github-prs`, `review-github-pr`, `reconcile-github-issues` | | `unattended` | `true` | `explore-github-repo`, `file-github-issues`, `triage-github-issues`, `triage-github-issue`, `implement-github-issues`, `implement-github-issue`, `shepherd-github-prs`, `shepherd-github-pr`, `review-github-prs`, `review-github-pr`, `reconcile-github-issues` | | `trusted_authors` | *(none)* | `triage-github-issues`, `triage-github-issue`, `implement-github-issues`, `implement-github-issue`, `shepherd-github-prs`, `shepherd-github-pr`, `review-github-prs`, `review-github-pr`, `reconcile-github-issues`, `setup-github-agent-workflow` | @@ -176,6 +177,8 @@ A lease is **live** only if the item carries the lease label **and** the newest **Nested calls.** A skill that calls an item skill (itself, or another step) on an item it already holds passes `lease_held=true`. The callee then skips its own claim and release, and requires that the lease is live. Without it the callee would see the caller's live lease and skip the item, and a nested release would drop the caller's lease. Only the caller that took a lease releases it. +A skill passes `lease_held=true` only when **all** of these hold, and the scenario checker rejects a call that breaks one: the caller holds the lease on that item (it claimed it in this run, or it was itself given `lease_held=true` for it); the call is about that same item and no other, because a lease on one item says nothing about another; and the callee is an item skill that lists `lease_held` in its `params`. A group skill holds no lease and never passes it. Splitting an issue (`references/split.md`) makes no such call: each sub-issue is another item with its own lease, and `file-github-issues` takes no lease. + TTLs are `lease_ttl_triage`, `lease_ttl_implement` and `lease_ttl_pr`. Concurrency is best-effort: two runs can claim the same item in the same second. Every step is idempotent, so the worst case is duplicated work. ## Markers @@ -186,6 +189,7 @@ Hidden HTML comments make state readable without parsing prose. Append them as t |---|---|---| | `` | `claim`, `renew` (edited in place, one comment per step) | lease expiry | | `` | Implementing, in the **PR description** | links a PR to its issue, so a second PR is never opened for it (`prs_for_issue N`) | +| `` | Implementing, when it has split an issue that is too large and every sub-issue is filed and linked | the issue is split: it is not implemented, and `reconcile-github-issues` closes it when its sub-issues close (`references/split.md`) | | `` | Shepherding | the PR was reviewable at this head SHA | | `` | Reviewing, when it does not merge; `N` = findings still unfixed | counts review rounds; `open_findings N` tells Shepherding what is left | | `` | Reviewing, after it merges | the review record stays with the PR | @@ -211,6 +215,8 @@ Hidden HTML comments make state readable without parsing prose. Append them as t | `protected_files N` | the changed files that match `human_merge_paths`. Fails closed: a non-zero status means the list could not be read and the caller must not merge | | `codeowners_for N` | the CODEOWNERS owners of the changed files, read from the base branch. Nothing if there is no file or no owner | | `lane_holder N` | the other PR that holds the merge lane, if any (see `references/labels.md`). Prints its number and returns 0, or prints nothing and returns 1 | +| `parent_of N`, `children_of N`, `depth_of N` | the issue N is a sub-issue of (nothing if it has none), its sub-issues (one number per line), and how many ancestors it has (`0` with no parent). They read GitHub's sub-issue links and nothing else. A status other than success means the answer could not be read. See `references/split.md` | +| `link_sub_issue N X` | make X a sub-issue of N. Succeeds when X already is one of N's. Only for an issue this run filed | | `client_guidance_files`, `item_labels N issue\|pr` | the repository's agent-facing files on the default branch (one path per line, plus `repo_guidance`), and each label on the item as `name: description`. See `references/client-guidance.md` | | `log_issue`, `log_run FILE`, `log_status`, `read_logs DAYS` | see `references/log.md` | @@ -238,6 +244,7 @@ This skill is reference material: it is never run, so it takes **no inputs** of | `references/human-merge.md` | when and how to hand a reviewed PR to a person to merge | | `references/client-guidance.md` | the repository's own agent rules: what to read, who wins, and issues that must stay open | | `references/dedupe.md` | duplicate search and the canonical-issue rule | +| `references/split.md` | when an issue is too large, how deep a split may go, how sub-issues are linked, and when the parent closes | | `references/issue-format.md` | well-formed issue contents (by category) and Triaging's check | | `references/pr-format.md` | agent PR body checklist; client templates take precedence | | `references/log.md` | run-log issue, its window and last-entry line, and the entry format | diff --git a/skills/meta/agent-workflow-protocol/references/dedupe.md b/skills/meta/agent-workflow-protocol/references/dedupe.md index 59079c8..6102243 100644 --- a/skills/meta/agent-workflow-protocol/references/dedupe.md +++ b/skills/meta/agent-workflow-protocol/references/dedupe.md @@ -12,6 +12,8 @@ Search all of these using two or three distinctive keywords from the title and b Ignore issues labelled `agent:log` or `agent:difficulty`. +A sub-issue and its parent are never duplicates of each other, and neither are two sub-issues of one parent (`parent_of`, `children_of`; see `references/split.md`). Link them as **Related but different**. Otherwise Triaging would close a slice as a duplicate of the issue it was cut from. + ## Decide | Finding | Exploring does | Triaging does | diff --git a/skills/meta/agent-workflow-protocol/references/issue-format.md b/skills/meta/agent-workflow-protocol/references/issue-format.md index b8a1c27..b5a12d2 100644 --- a/skills/meta/agent-workflow-protocol/references/issue-format.md +++ b/skills/meta/agent-workflow-protocol/references/issue-format.md @@ -62,7 +62,7 @@ Title: `Refactor to ` | **Behaviour guard.** Required | How anyone can tell behaviour did not change: the existing tests that cover the code, or a note that a characterization test must be written first | | **Where.** Required | File paths and symbols | | Cost of leaving it | What it slows down, breaks or risks | -| Size | `small`, `medium` or `large`. If `large`, propose how to split it | +| Size | `small`, `medium` or `large`. If `large`, propose how to split it. `implement-github-issue` splits a `large` issue that divides into sub-issues (`references/split.md`) | | Suggested direction | | ### Debt (`Category: debt`) diff --git a/skills/meta/agent-workflow-protocol/references/labels.md b/skills/meta/agent-workflow-protocol/references/labels.md index cf4e705..1f22e5c 100644 --- a/skills/meta/agent-workflow-protocol/references/labels.md +++ b/skills/meta/agent-workflow-protocol/references/labels.md @@ -62,6 +62,8 @@ Each row happens inside a lease held by the step named (except `reconcile-github | Issue | `needs-info` | a human removes the label after answering | untouched, so Triaging picks it up again | | Issue | `ready-for-agent` | Implementing | `agent:pr-open`, and the new PR gets `agent:needs-work`. If a PR for the issue already exists, no new PR is made | | Issue | `ready-for-agent` | Implementing (bail-out) | `needs-info` or `ready-for-human` | +| Issue | `ready-for-agent`, too large for one PR | Implementing (split) | unchanged, with a trusted `agent:split` marker comment and linked sub-issues that Triaging picks up. No `agent:pr-open`, because no PR is open. Implementing does not take it again (`references/split.md`) | +| Issue | `ready-for-agent`, split, every sub-issue closed | `reconcile-github-issues` | closed, or `ready-for-human` when a sub-issue was closed as not planned or the issue is keep-open (`references/split.md`) | | Issue | `agent:pr-open`, every PR closed unmerged | `reconcile-github-issues` | `ready-for-human` | | Issue | two or more mutex state labels, or clear label/comment drift | `reconcile-github-issues` (label repair) | single consistent state per the priority order above | | Issue | `ready-for-agent` or `agent:pr-open`, a PR already merged | Implementing, Reviewing or `reconcile-github-issues` | closed, and it loses `agent:pr-open`. A **keep-open** issue (`client-guidance.md`) stays open: it loses `agent:pr-open` and becomes `ready-for-human` | diff --git a/skills/meta/agent-workflow-protocol/references/split.md b/skills/meta/agent-workflow-protocol/references/split.md new file mode 100644 index 0000000..9164d57 --- /dev/null +++ b/skills/meta/agent-workflow-protocol/references/split.md @@ -0,0 +1,48 @@ +# Splitting an issue that is too large + +One rule, used wherever an issue is split or a split issue is settled: `implement-github-issue` (step 4, when it decides an issue is too large), `reconcile-github-issues` (when the sub-issues close) and `file-github-issues` (when it is given a `parent`). Keep it here. Do not copy it into a skill. The reasons are in `docs/adr/0015-split-an-issue-that-is-too-large.md` in the package repository. + +## Too large + +Decide this in `implement-github-issue` after reading the issue (step 3) and before writing code. An issue is **too large** only when both hold: + +1. **It divides.** It asks for two or more parts that each merge alone, leave the repository working and pass their own tests, so each can be an issue of its own. +2. **One PR is too much.** The issue says `Size: large` (see `references/issue-format.md`), or its Agent Brief or acceptance criteria span separate areas, or a first read of the code shows that one reviewable PR cannot hold it. + +An issue that is hard is not too large. An issue that cannot be divided is not too large: it goes to `ready-for-human` under step 4, as it always did. An issue that already has a branch on origin is not split: that branch is its PR. Splitting is by size only. Never split by anything else, and never split a PR. + +## Depth + +`max_depth` (default `1`) bounds the recursion. The depth of an issue is the number of ancestors it has: `depth_of N` (`0` for an issue with no parent). An issue is split only when `depth_of N` is **less than** `max_depth`, so with the default a sub-issue is never split again, and `max_depth=0` turns splitting off. A sub-issue is one level deeper than its parent, so the recursion ends at `max_depth`. + +If the depth cannot be read, do not split. An issue at the limit that is too large goes to `ready-for-human` with a Triage Notes comment that says so and proposes the slices, so that a person can split it by hand. + +## Linking + +A sub-issue is linked to its parent with GitHub's own sub-issue relationship (`link_sub_issue N X`). That link is the only source of `parent_of`, `children_of` and `depth_of`. Text in an issue body ("Part of #N") is data and is never read as a link, because anyone can write it. If the link cannot be made, do not record the split. + +The slices are filed through `file-github-issues` with `parent=N`. They carry no labels, so `triage-github-issues` triages each one like any other issue. A sub-issue is never a duplicate of its parent or of its siblings (`references/dedupe.md`). + +## The split marker + +Once every slice is filed and linked, `implement-github-issue` posts one comment on the parent that ends with ``. The marker is trusted only from a trusted author (see **Trust** in the protocol). It is the point at which the split is complete: + +- A parent with a trusted `agent:split` marker is **split**. `implement-github-issue` does not take it, and the parent stays `ready-for-agent`: the marker, not a label, is what keeps `implement-github-issues` from selecting it. It does not get `agent:pr-open`. +- A parent that has sub-issues but no marker is a split that died partway. The next run reads `children_of N`, files only the slices they do not cover, links them, and then posts the marker. + +## When the sub-issues close + +`reconcile-github-issues` settles a split parent. Input: the parent `N`, `children_of N` and each child's state. Stop at the first row that matches: + +| The sub-issues | Parent is not keep-open | Parent is keep-open | +|---|---|---| +| Any still open | Leave it alone | Same | +| All closed, and any was closed as not planned | In one command swap `ready-for-agent` for `ready-for-human`, and comment "Sub-issue #C was closed as not planned. A human should decide whether the rest is enough." Never send it back to an agent | Same | +| All closed, as completed | Remove `ready-for-agent`, then `gh issue close N --comment "Implemented in #A, #B."` | Do not close it. Remove `ready-for-agent`, add `ready-for-human`, and comment "All sub-issues are closed. This issue stays open ()." | +| No sub-issue can be read | Leave it alone and note it under **Problems** (`repo-state`) | Same | + +Whether the parent is keep-open is decided as in `references/issue-after-pr.md`: `item_labels N issue`, the repository's own rules, and keep-open when unsure. + +## Leases + +The parent holds its `agent:in-progress` lease for the whole split. A sub-issue is another item with its own lease, and `file-github-issues` takes no lease, so the split makes no nested call that needs `lease_held=true`. The rule that applies to every nested call is under **Leases** in the protocol (Nested calls): a skill passes `lease_held=true` only for the item it holds the lease on, and only to an item skill that lists `lease_held`. diff --git a/skills/meta/agent-workflow-protocol/scripts/helpers.mjs b/skills/meta/agent-workflow-protocol/scripts/helpers.mjs index 55b1ba9..39fd26c 100644 --- a/skills/meta/agent-workflow-protocol/scripts/helpers.mjs +++ b/skills/meta/agent-workflow-protocol/scripts/helpers.mjs @@ -337,6 +337,64 @@ export function isTrusted(c, { selfLogin = '', trustedAuthors = [] } = {}) { return login !== '' && [selfLogin, ...trustedAuthors].filter(Boolean).includes(login); } +// ---- splitting an issue that is too large (references/split.md) --------------------------- + +/** How many ancestors an issue may have before `depthOf` gives up. GitHub itself stops well below this. */ +export const MAX_ANCESTORS = 20; + +/** + * The comment that records a split. `children` are the sub-issue numbers. The marker is the last line, + * and it is the point at which the split is complete. + */ +export function splitComment(children) { + const list = children.map((n) => `#${n}`).join(', '); + const marker = ``; + return { body: `Split into ${list} because this issue is too large for one PR. Each is triaged on its own, and this issue closes when they do.\n${marker}`, marker }; +} + +/** + * The sub-issue numbers the newest `agent:split` marker names, or null when the issue is not split. `markers` + * is what `markersOf` returns, so a marker from an untrusted account is never in it. + */ +export function splitChildren(markers) { + const m = markers.filter((x) => x.includes('agent:split ')).pop(); + if (m === undefined) return null; + return (m.match(/children=([0-9,]*)/)?.[1] ?? '').split(',').filter(Boolean).map(Number); +} + +/** + * How many ancestors issue `n` has, given what was read: `parents` maps an issue number to its parent's number, + * or to null when it has none. A number that was not read (no key) is an error, because an unread parent would + * look like no parent and let an issue split below the limit. So is a cycle, or more than MAX_ANCESTORS. + */ +export function depthOf(n, parents) { + const has = (k) => Object.prototype.hasOwnProperty.call(parents, k); + const seen = new Set([Number(n)]); + let cur = Number(n); + let depth = 0; + for (;;) { + if (!has(cur)) throw new Error(`the parent of #${cur} was not read`); + const p = parents[cur]; + if (p === null || p === undefined) return depth; + if (seen.has(Number(p))) throw new Error(`#${p} is its own ancestor`); + seen.add(Number(p)); + cur = Number(p); + depth += 1; + if (depth > MAX_ANCESTORS) throw new Error(`more than ${MAX_ANCESTORS} ancestors`); + } +} + +/** + * True when an issue at `depth` may be split under `maxDepth`: it must have fewer ancestors than the limit, so a + * sub-issue is one level deeper than its parent and the recursion ends at the limit. `maxDepth` 0 turns splitting off. + * A depth that could not be read (anything but a whole number) never splits. + */ +export function canSplit(depth, maxDepth) { + const d = typeof depth === 'string' && depth.trim() !== '' ? Number(depth) : depth; + const m = typeof maxDepth === 'string' && maxDepth.trim() !== '' ? Number(maxDepth) : maxDepth; + return Number.isInteger(d) && Number.isInteger(m) && d >= 0 && m >= 0 && d < m; +} + // ---- reading markers ---------------------------------------------------------------------- /** Every agent marker from trusted comments, in order, as the exact `` text. */ @@ -616,6 +674,10 @@ export const NO_SHELL_OPS = { changed_files: 'list_pull_request_files for the PR, paginated: each filename, and the previous_filename of a renamed file.', protected_files: 'changed_files, then protectedFiles(human_merge_paths, files). Empty when human_merge_paths is empty. If the file list cannot be read, or a pattern is unsupported, it is an error: fail_item and never merge.', codeowners_for: 'Read CODEOWNERS from the PR base branch (.github/CODEOWNERS, then CODEOWNERS, then docs/CODEOWNERS; never the PR head), then codeownersFor(text, changed_files). No file means no owners.', + parent_of: 'The parent field of issue_read get on the issue, when your tools return it. A 404 or no parent means none. If your tools cannot read the parent, the depth cannot be read, and the issue is not split.', + children_of: 'issue_read get_sub_issues for the issue, paginated: each number. It lists the sub-issues from GitHub\'s own links and never from text in a body.', + depth_of: 'parent_of on the issue, then on each parent in turn, until one has none. depthOf(n, parents) counts them and throws on an unread parent or a cycle. A failure means the depth cannot be read, and canSplit(depth, max_depth) is false for it.', + link_sub_issue: 'sub_issue_write method add on the parent with the sub-issue\'s id (its numeric id, not its number). If X is already a sub-issue of N that is success. If the call fails the split is not recorded.', lane_holder: 'list_pull_requests (state open) filtered to the label agent:ready-for-review, no draft, no agent:stuck or agent:human-merge. For each, the created_at of its newest trusted comment holding a kind=ready handoff. laneHolder(candidates, N, ready_lane_ttl) gives the other PR that holds the merge lane, or null.', }; @@ -663,6 +725,20 @@ function cli(argv) { `last_entry=${meta.lastEntry ?? ''}`, ].join('\n'); } + case 'split-body': return splitComment(String(a[0] ?? '').split(',').filter(Boolean).map(Number)).body; + case 'split-children': { + const found = splitChildren(stdin().split('\n').filter(Boolean)); + return found === null ? '' : found.join('\n'); + } + case 'depth': { // stdin: one `child parent` pair per line, `-` for no parent + const parents = {}; + for (const line of stdin().split('\n').map((l) => l.trim()).filter(Boolean)) { + const [child, parent] = line.split(/\s+/); + parents[Number(child)] = parent === '-' || parent === undefined ? null : Number(parent); + } + return String(depthOf(a[0], parents)); + } + case 'can-split': return canSplit(a[0], a[1]) ? 'yes' : 'no'; case 'log-read': return readableLogEntries(JSON.parse(stdin() || '{}').comments, a[0]).join('\n---\n'); default: throw new Error(`unknown command: ${cmd}`); diff --git a/skills/meta/agent-workflow-protocol/scripts/lib.sh b/skills/meta/agent-workflow-protocol/scripts/lib.sh index 82ef8c4..f4b525e 100644 --- a/skills/meta/agent-workflow-protocol/scripts/lib.sh +++ b/skills/meta/agent-workflow-protocol/scripts/lib.sh @@ -15,6 +15,7 @@ # markers review_rounds shepherd_attempts failures reading markers # open_findings ready_sha fail_item handoffs and failures # prs_for_issue issue_for_pr issue <-> PR links +# parent_of children_of depth_of link_sub_issue sub-issue links (splitting an issue) # client_guidance_files item_labels the repository's own agent rules # log_issue log_run log_status read_logs run log # Every marker is read through trusted_jq / _trusted_logins: a marker written by an account that is @@ -47,6 +48,7 @@ AGENT_ROOT="$(git rev-parse --show-toplevel)" : "${max_shepherd_attempts:=3}" : "${max_review_rounds:=3}" : "${max_item_failures:=3}" +: "${max_depth:=1}" : "${runner:=unknown}" : "${unattended:=true}" : "${trusted_authors:=}" @@ -81,6 +83,7 @@ review_batch:REVIEW_BATCH max_shepherd_attempts:MAX_SHEPHERD_ATTEMPTS max_review_rounds:MAX_REVIEW_ROUNDS max_item_failures:MAX_ITEM_FAILURES +max_depth:MAX_DEPTH runner:RUNNER unattended:UNATTENDED trusted_authors:TRUSTED_AUTHORS @@ -323,6 +326,49 @@ issue_for_pr() { } | sed '/^$/d' | sort -un } +# ---- Sub-issue links --------------------------------------------------------------------- +# A split (../references/split.md) uses GitHub's own sub-issue links and nothing else: text in an issue body +# is not a link, because anyone can write it. These read and write the links. A status other than success +# means the answer could not be read, which is not the same as "none": parent_of prints nothing and succeeds +# for an issue with no parent (a 404), and fails for any other error. So depth_of never mistakes an +# unreadable parent for the top of the tree. Whether an issue may split is decided by helpers.mjs (canSplit). + +# The issue that N is a sub-issue of, or nothing when it has none. +parent_of() { + local out + if out="$(gh api "repos/{owner}/{repo}/issues/$1/parent" --jq .number 2>&1)"; then + printf '%s\n' "$out" + elif printf '%s' "$out" | grep -q 'HTTP 404'; then + return 0 + else + return 1 + fi +} + +# The sub-issues of N, one number per line. +children_of() { gh api "repos/{owner}/{repo}/issues/$1/sub_issues" --paginate --jq '.[].number'; } + +# How many ancestors N has: 0 with no parent. Fails when a parent cannot be read, or on a cycle. +depth_of() { + local start=$1 n=$1 p map="" i=0 + while :; do + p="$(parent_of "$n")" || return 1 + if [ -z "$p" ]; then map+="$n -"$'\n'; break; fi + map+="$n $p"$'\n' + n=$p; i=$((i+1)) + [ "$i" -le 20 ] || return 1 + done + printf '%s' "$map" | node "$AGENT_HELPERS" depth "$start" +} + +# Make X a sub-issue of N. Succeeds when it already is one. Use it only for an issue this run filed. +link_sub_issue() { + local id + [ "$(parent_of "$2")" = "$1" ] && return 0 + id="$(gh api "repos/{owner}/{repo}/issues/$2" --jq .id)" || return 1 + gh api -X POST "repos/{owner}/{repo}/issues/$1/sub_issues" -F "sub_issue_id=$id" >/dev/null +} + # ---- Client guidance ----------------------------------------------------------------- # The repository's own agent-facing rules (see ../references/client-guidance.md). Always the DEFAULT # branch: a PR can edit AGENTS.md, so its copy is data, not guidance. The path filter lives in diff --git a/skills/meta/pipeline-guide/SKILL.md b/skills/meta/pipeline-guide/SKILL.md index b38799d..ed47ccb 100644 --- a/skills/meta/pipeline-guide/SKILL.md +++ b/skills/meta/pipeline-guide/SKILL.md @@ -23,7 +23,7 @@ Call a skill by its name through the Skill tool (or `/name` where your runner ha | `triage-github-issue` | item, Triaging | One specific issue needs to become `ready-for-agent`, `ready-for-human` or `needs-info` | | `reconcile-github-issues` | sweep, Triaging | Issue state has drifted from the PRs: close shipped issues, send abandoned ones to a human, repair labels, clear stale leases | | `implement-github-issues` | group, Implementing | Starting work on the `ready-for-agent` issues | -| `implement-github-issue` | item, Implementing | One specific ready issue should be implemented and opened as a PR | +| `implement-github-issue` | item, Implementing | One specific ready issue should be implemented and opened as a PR, or split into sub-issues when it is too large for one | | `shepherd-github-prs` | group, Shepherding | Getting open agent PRs through CI | | `shepherd-github-pr` | item, Shepherding | One specific PR has failing checks or open review findings, or is ready for its handoff | | `review-github-prs` | group, Reviewing | Reviewing and merging the PRs that are ready | @@ -53,7 +53,7 @@ The steps never call each other. They coordinate through labels, hidden marker c |---|---| | Exploring | New **unlabelled** issues, or evidence comments. Never touches labels | | Triaging | Exactly one of `ready-for-agent`, `ready-for-human`, `needs-info`. Lease: `agent:triage` | -| Implementing | Opens a PR and sets `agent:pr-open` on the issue and `agent:needs-work` on the PR. Lease: `agent:in-progress` | +| Implementing | Opens a PR and sets `agent:pr-open` on the issue and `agent:needs-work` on the PR. An issue that is too large is split instead: linked sub-issues and an `agent:split` marker, the issue left as it is. Lease: `agent:in-progress` | | Shepherding | Fixes failures, then the ready handoff and `agent:ready-for-review`. Lease: `agent:in-progress` | | Reviewing | Merges pinned to the reviewed commit, or the review handoff and back to `agent:needs-work`. Lease: `agent:in-progress` | diff --git a/skills/meta/runner-prompts/references/prompts.md b/skills/meta/runner-prompts/references/prompts.md index 1ab327e..ddd93a7 100644 --- a/skills/meta/runner-prompts/references/prompts.md +++ b/skills/meta/runner-prompts/references/prompts.md @@ -27,6 +27,7 @@ One candidate as key-value pairs: fill in each empty `title` to `evidence` line. ``` Run /file-github-issues dry_run=false +parent= limit= repo_guidance=AGENTS.md dedupe_closed_days=30 @@ -92,6 +93,7 @@ write_log=true ``` Run /implement-github-issues on github.com/OWNER/REPO implement_batch=1 +max_depth=1 runner=NAME log_window_days=14 unattended=true @@ -108,6 +110,7 @@ git_hooks_path= repo_guidance=AGENTS.md lease_ttl_implement=3 hours max_item_failures=3 +max_depth=1 runner=NAME log_window_days=14 unattended=true diff --git a/skills/pipeline/file-github-issues/SKILL.md b/skills/pipeline/file-github-issues/SKILL.md index 3cbdfd0..cd1e761 100644 --- a/skills/pipeline/file-github-issues/SKILL.md +++ b/skills/pipeline/file-github-issues/SKILL.md @@ -29,7 +29,7 @@ types: evidence: "what the category's Required sections need" related: "optional issue numbers" screenshots: "optional" -params: [candidates, dry_run, limit, repo_guidance, dedupe_closed_days, run_id, title, category, area, impact, confidence, evidence, related, screenshots, unattended] +params: [candidates, dry_run, limit, parent, repo_guidance, dedupe_closed_days, run_id, title, category, area, impact, confidence, evidence, related, screenshots, unattended] requires: - "None: always eligible. It works on candidates, not on an item that could be leased." returns: @@ -45,6 +45,7 @@ ensures: - "A filed issue has no labels and no assignee, follows its category's title pattern, and has every Required section of its category." - "A candidate that is a possible security vulnerability is never filed publicly." - "A candidate that matches a closed-as-not-planned issue or `.out-of-scope/` is dropped. One closed as completed is filed as a possible regression of it." + - "With `parent` set, the parent is never the issue a candidate is a duplicate of, and each filed body says `Part of #.`. The sibling slices already filed are searched like any open issue, so a repeat is a duplicate of its own earlier slice. No sub-issue link is made here: the caller links." effects: reads: [open issues, recently closed issues (dedupe_closed_days), ".out-of-scope/", the repository's own guidance and issue templates] writes: @@ -70,6 +71,7 @@ terminates: "One pass over the candidates." |---|---|---| | `candidates` | *(from free text)* | Optional structured list; free text is also accepted | | `dry_run` | `false` | When `true`, do not create issues or comments; only return the decisions | +| `parent` | *(none)* | The issue these candidates are slices of, as a number. It is never reported as a duplicate of a slice, and each filed body says `Part of #.`. Set by `implement-github-issue` when it splits an issue that is too large. The caller makes the sub-issue links | | `limit` | *(none)* | Most issues to file in this call. Once that many are filed, each remaining candidate returns `deferred: over limit` without being checked. Without it, every candidate is processed | | `repo_guidance` | `AGENTS.md` | Extra files holding the repository's own agent rules, read in addition to the standard locations (see `references/client-guidance.md`) | | `dedupe_closed_days` | `30` | The "recently closed" window the duplicate search runs over | @@ -95,14 +97,14 @@ terminates: "One pass over the candidates." For **each** candidate, in order. If `limit` is set and `limit` candidates have already been `filed` (or `would-file`) in this call, return `deferred: over limit` for this candidate and every later one, and do not check them. A duplicate or a drop does not count toward the limit. -1. **Dedupe.** Follow the Exploring column of the dedupe table (see `agent-workflow-protocol/references/dedupe.md` or the summary below). Search open issues, recently closed issues and `.out-of-scope/`. +1. **Dedupe.** Follow the Exploring column of the dedupe table (see `agent-workflow-protocol/references/dedupe.md` or the summary below). Search open issues, recently closed issues and `.out-of-scope/`. With `parent` set, leave the parent out of the results: a slice of an issue is not a duplicate of it. 2. **Decide**: - Same problem as an open issue → do not file. Add a comment with any **new** evidence (unless dry_run). Result: `duplicate of #N (commented)`. - Closed as not planned, or matches `.out-of-scope/` → do not file. Result: `dropped: rejected before in #N` (or the file). - Closed as completed → file it, and say "possible regression of #N" in the body. - Related but different → file it and mention `#N` in the body. 3. **Check required sections.** Each category marks sections **Required** (see issue-format). If you cannot fill one from the evidence, do not file. Result: `dropped: missing
` (or `dropped: no use case` for a feature). Never file a possible security vulnerability as a public issue. -4. **Write the body** following the category's title pattern and sections, plus common fields and a last line `Filed by Exploring run ` when applicable. Prefer attaching screenshots only when the runner can put them on GitHub; otherwise note under **Visuals**. +4. **Write the body** following the category's title pattern and sections, plus common fields and a last line `Filed by Exploring run ` when applicable. With `parent` set, start the body with `Part of #.`. Prefer attaching screenshots only when the runner can put them on GitHub; otherwise note under **Visuals**. 5. **File it** (skip when `dry_run=true`). Create the issue with **no labels** and no assignee. Result: `filed #N`. ### Dedupe summary (Exploring) diff --git a/skills/pipeline/implement-github-issue/SKILL.md b/skills/pipeline/implement-github-issue/SKILL.md index 81bee22..24d43af 100644 --- a/skills/pipeline/implement-github-issue/SKILL.md +++ b/skills/pipeline/implement-github-issue/SKILL.md @@ -1,6 +1,6 @@ --- name: implement-github-issue -description: Implement one specific ready-for-agent GitHub issue by number and open a PR against the configured base branch. Checks first that no PR already exists for the issue, reads the Agent Brief, builds on a feature or bugfix branch, and hands the PR to Shepherding. Use when given an issue number to implement, or when implement-github-issues dispatches one to you. +description: Implement one specific ready-for-agent GitHub issue by number and open a PR against the configured base branch. Checks first that no PR already exists for the issue, reads the Agent Brief, builds on a feature or bugfix branch, and hands the PR to Shepherding. An issue that is too large for one PR is split into linked sub-issues instead, within max_depth. Use when given an issue number to implement, or when implement-github-issues dispatches one to you. --- # Implement GitHub issue @@ -9,7 +9,7 @@ Implement one `ready-for-agent` issue and open a PR. Getting the PR through chec **Input:** one `ready-for-agent` GitHub issue, as a URL or a number. Required. -**References:** call the Skill tool with `agent-workflow-protocol`. In that skill's directory, open `references/labels.md` before you set a label, `references/issue-format.md` at step 3, `references/pr-format.md` at step 8, `references/escalate.md` if you escalate, and `references/log.md` when you write the log entry. Also open `references/client-guidance.md`: the repository's own rules (`AGENTS.md` and the other files it lists, plus `$repo_guidance`) outrank this skill's defaults. +**References:** call the Skill tool with `agent-workflow-protocol`. In that skill's directory, open `references/labels.md` before you set a label, `references/issue-format.md` at step 3, `references/split.md` at step 4, `references/pr-format.md` at step 8, `references/escalate.md` if you escalate, and `references/log.md` when you write the log entry. Also open `references/client-guidance.md`: the repository's own rules (`AGENTS.md` and the other files it lists, plus `$repo_guidance`) outrank this skill's defaults. ## Contract @@ -19,14 +19,16 @@ See **Contracts** in the protocol for what each key means. kind: item step: Implementing target: { param: issue, type: issue } -params: [issue, pr_base_branch, build_cmd, git_hooks_path, repo_guidance, lease_ttl_implement, max_item_failures, runner, log_window_days, unattended, trusted_authors, write_log, lease_held] +params: [issue, pr_base_branch, build_cmd, git_hooks_path, repo_guidance, lease_ttl_implement, max_item_failures, max_depth, runner, log_window_days, unattended, trusted_authors, write_log, lease_held] requires: - "The issue is open and labelled ready-for-agent, without agent:stuck." - "No other run holds it: it does not carry agent:in-progress while `lease_live N` succeeds. A lease label with no live lease is stale and does not count, and checking never removes it. With `lease_held=true` the caller holds the lease, so `lease_live N` must succeed." + - "It is not already split: no trusted `agent:split` marker is on it (see references/split.md)." returns: - "#N PR #P opened" - "#N existing PR #P" - "#N already merged in #P" + - "#N split into " - "#N ready-for-human" - "#N needs-info" - "#N escalated: " @@ -36,6 +38,9 @@ ensures: - "At most one PR exists for the issue: `prs_for_issue N` was checked before any code was written and again before the PR was opened." - "A new PR targets pr_base_branch, from a branch matching ^[a-z]+/N-$. Its body satisfies references/pr-format.md, ends with ``, contains `Closes #N.` (`Refs #N.` when the issue is keep-open), and the PR is labelled agent:needs-work." - "After `PR #P opened` the issue lost ready-for-agent and gained agent:pr-open." + - "A split happens only when the issue is too large as references/split.md defines it, `depth_of N` was read and is less than `max_depth`, and no branch for the issue exists on origin. At the limit, or with the depth unreadable, the issue is not split: it is ready-for-human with a Triage Notes comment that proposes the slices." + - "After `split into ` every slice was filed through `file-github-issues` with `parent=N`, so none was a duplicate of the issue, and each carries no labels. Each is linked to the issue as a sub-issue, so its `depth_of` is one more than the issue's and never more than `max_depth`. The issue got one comment ending in ``, stays open, keeps ready-for-agent and did not gain agent:pr-open. No branch was pushed and no PR was opened." + - "A split that could not be completed (a slice came back dropped or a duplicate of an issue that is not already a sub-issue, or a link could not be made) wrote no marker and returned `failed`. The next run files only the slices that are missing." - "An existing OPEN PR means no second PR: the issue is agent:pr-open, and a PR that carries the implements marker and neither agent:needs-work nor agent:ready-for-review gets agent:needs-work. A PR without the marker keeps its labels." - "A MERGED PR on an issue that is not keep-open means the issue was closed with `Implemented in #P.` and lost ready-for-agent. On a keep-open issue merged PRs are ignored and it stays open." - "Only CLOSED PRs means ready-for-human with a comment naming the PR, and no retry." @@ -50,12 +55,14 @@ effects: - "The agent:in-progress lease label and its marker. A stale label from a dead run is replaced by the new lease. With `lease_held=true` nothing is written here." - "The issue's state label (ready-for-agent to agent:pr-open, needs-info or ready-for-human)." - "A Triage Notes, closed-PR or `Implemented in` comment, and closing the issue when its PR already merged." + - "One comment ending in the `agent:split` marker when the issue is split." - "An agent:failure marker on failure. On escalation, the escalate comment and agent:stuck." repository: - "A feature or bugfix branch pushed to origin, with commits." - "One PR against pr_base_branch, labelled agent:needs-work." other: - "agent:needs-work on an existing OPEN agent PR for the issue." + - "New sub-issues through file-github-issues when the issue is split, each with no labels, each linked to the issue as a sub-issue." shared: - "One run-log entry (when write_log is true)." external: ["build_cmd, when set, and the repository's test commands."] @@ -65,10 +72,14 @@ never: - "Open a second PR for an issue that already has one." - "Push a branch whose name does not match ^[a-z]+/N-$. Rename it or fail first." - "Stack unrelated issues or drive-by changes into one PR." + - "Split an issue by anything but size, split a PR, or split an issue that is already at `max_depth`." + - "Split an issue while a branch for it exists on origin." + - "Pass `lease_held=true` for any item but this issue, or to a skill that does not list `lease_held`." + - "Link an issue it did not file, or read a `Part of #N` line as a link." - "Claim verification you did not perform." -calls: [tdd, code-review] -idempotent: "prs_for_issue is read before coding and before opening, and an existing branch for the issue is checked out and continued." -terminates: "failures N Implementing reaching max_item_failures escalates and stops. One PR per issue." +calls: [tdd, code-review, file-github-issues] +idempotent: "prs_for_issue is read before coding and before opening, and an existing branch for the issue is checked out and continued. A split issue carries its marker and is not taken again. A split that died partway is finished from the sub-issues that already exist, so no slice is filed twice." +terminates: "failures N Implementing reaching max_item_failures escalates and stops. One PR per issue. A split happens only when `depth_of N` is below `max_depth` and each sub-issue is one level deeper, so the recursion ends at `max_depth`." ``` ## Inputs @@ -86,6 +97,7 @@ terminates: "failures N Implementing reaching max_item_failures escalates and st | `repo_guidance` | `AGENTS.md` | Extra files holding the repository's own agent rules, read in addition to the standard locations (see `references/client-guidance.md`) | | `lease_ttl_implement` | `3 hours` | How long the Implementing lease stays valid without a renewal | | `max_item_failures` | `3` | Failed runs of one step on one item before it is escalated | +| `max_depth` | `1` | How deep a chain of splits may go. An issue is split into sub-issues only when it has fewer ancestors than this, so `1` means a sub-issue is never split again and `0` turns splitting off | | `runner` | `unknown` | Name recorded in the run id and the run log | | `log_window_days` | `14` | Days one run-log issue is appended to before a new one starts | | `unattended` | `true` | `true`: no human is present, so never ask (see **Unattended runs** in the protocol). `false`: a human may be asked | @@ -93,7 +105,7 @@ terminates: "failures N Implementing reaching max_item_failures escalates and st | `write_log` | `true` | Write this item's own run-log entry. The group skill sets it to `false` when it dispatches, because the group logs the item | | `lease_held` | `false` | `true` when the caller already holds the lease on this item, as in a nested call. The skill then neither claims nor releases it, and the lease must be live | -**Implicit dependencies.** The common ones (**Shell setup** and **Running without a shell** in the protocol), plus the `tdd` and `code-review` skills (through the Skill tool), a working build toolchain, a git identity, and the issue's Agent Brief. Without a shell, keep the change small and record what you could not run under **Problems** in the run log. +**Implicit dependencies.** The common ones (**Shell setup** and **Running without a shell** in the protocol), plus the `tdd`, `code-review` and `file-github-issues` skills (through the Skill tool), a working build toolchain, a git identity, the issue's Agent Brief, and GitHub's sub-issue links (`parent_of`, `children_of`, `depth_of` and `link_sub_issue` in the protocol): without them the issue is never split. Without a shell, keep the change small and record what you could not run under **Problems** in the run log. ## Process @@ -104,12 +116,17 @@ terminates: "failures N Implementing reaching max_item_failures escalates and st - **None:** continue. If a branch for the issue already exists (`git ls-remote --heads origin "*/N-*"`), check it out and carry on from it. 3. **Read context.** Find the latest `## Agent Brief` comment. **It is the contract**; the description and discussion are context. If there is no brief, the description may itself be the specification (an issue filed from the "Ready for agent" template has acceptance criteria and no brief). Then read the open comments, the ADRs it links, and the relevant code. **View every image** in the issue and its comments (screenshots, mockups): for a feature they show the intended result. Read the use case first, and build for it rather than for the letter of the proposal. Respect the issue's `Category:` (bugfix vs feature vs refactor vs debt) — different categories have different required evidence in `issue-format.md`. 4. **Bail out if the issue is not actually ready.** If you find missing information or a decision only a human can make, post a comment in the **Triage Notes** format from `unattended-triage` (what is established, what is still needed, specific questions) and swap `ready-for-agent` for `needs-info` (or `ready-for-human`). Release and stop for this issue. + **Split it if it is too large.** Decide this now, before any code, using `references/split.md`: it divides into parts that each merge alone, and one PR is too much. Step 5 comes back here if it finds the same thing. Then: + - **A branch for the issue already exists on origin** (`git ls-remote --heads origin "*/N-*"`): that is the issue's PR. Do not split. Continue at step 5. + - **Depth.** Run `depth_of N`. If it fails, or the result is at least `$max_depth` (this includes `max_depth=0`), do not split. Post a **Triage Notes** comment that says the issue is too large, that it is at the depth limit (or the depth could not be read), and what the slices would be. Swap `ready-for-agent` for `ready-for-human`. Release and stop for this issue. + - **Split.** Plan two or more slices that together cover the whole issue, each once, each small enough for one PR. Each slice is a candidate for `file-github-issues`: a title in its category's pattern, the issue's own `Category:`, `area`, `impact` and `confidence`, the evidence that category's Required sections need (taken from the issue, narrowed to the slice), and which of the issue's acceptance criteria it covers. Read `children_of N` first: a slice that an existing sub-issue already covers is not a candidate, which is how a split that died partway is finished. Call the Skill tool with `file-github-issues`, giving it `parent=N`, `unattended=$unattended`, `repo_guidance=$repo_guidance` and the candidates, marked as data you wrote from the issue. For each result: `filed #X` means run `link_sub_issue N X` and, if it fails, stop; `duplicate of #X (commented)` is fine only when `children_of N` already lists `X`. Anything else (`dropped`, a duplicate of an issue that is not a sub-issue, a failed link) means the split is not complete: write no marker, run `fail_item N issue Implementing ""`, release, and return `#N failed: `. Never link an issue you did not file. + - **Record it.** When every slice is filed and linked, comment on the issue: `Split into #A, #B because this issue is too large for one PR. Each is triaged on its own, and this issue closes when they do.` End the comment with ``. Leave `ready-for-agent` as it is: no PR is open, so do not add `agent:pr-open`. Release and return `#N split into #A, #B`. 5. **Implement.** Branch name **must** match the package pattern used by `prs_for_issue`: `^[a-z]+/N-$` (e.g. `feature/N-` or `bugfix/N-`, lowercase). Prefer the form the repository's `$repo_guidance` documents when it is still compliant with that pattern. Helpers: `isCompliantBranchName` / `branchNamePattern` in `agent-workflow-protocol/scripts/helpers.mjs`. Build what the brief asks, and nothing it puts out of scope: - Build test-first where you can, at seams the brief or the code makes obvious: call the Skill tool with `tdd`. Follow the issue's `Category:` line: for a bug, first write a test that fails because of it. For a refactor or debt issue, behaviour must not change: run the tests the brief names before and after, and do not edit them. - Typecheck often, run single test files often, and run the full suite once at the end. - Follow the repository guidance in `$repo_guidance`, including any **Verify** checklist it has. If it describes how to split large work across sub-agents, do that for anything beyond a focused fix. - Commit to the branch. Then call the Skill tool with `code-review` on the work, and fix what it finds. - - Escalate (`references/escalate.md`) if you hit a blocker you cannot resolve. If you cannot run commands, keep the change small. If it needs more than a focused change, or you cannot verify it by reading the code, bail out under step 4. + - Escalate (`references/escalate.md`) if you hit a blocker you cannot resolve. If you cannot run commands, keep the change small. If it needs more than a focused change, or you cannot verify it by reading the code, go back to step 4: split it if it is too large and nothing is pushed, otherwise bail out there. 6. **Build.** If you can run commands and `$build_cmd` is set, run it on this change and fix what fails. If it is empty, skip this step. If you cannot fix it, or cannot run commands, say so in the PR description and go on to step 7. CI is the build gate, and `shepherd-github-pr` handles a failing build. 7. **Branch name check (before any push).** Read the current branch name (`git branch --show-current` or your tools' equivalent). It must match `isCompliantBranchName(name, N)` — the same `^[a-z]+/N-…` pattern `prs_for_issue` uses. If it does not: - Prefer renaming or recreating the branch to a compliant name (`feature/N-` or `bugfix/N-` from the issue title, lowercase kebab-case) and continue, **or** diff --git a/skills/pipeline/implement-github-issues/SKILL.md b/skills/pipeline/implement-github-issues/SKILL.md index a130bff..ba533f5 100644 --- a/skills/pipeline/implement-github-issues/SKILL.md +++ b/skills/pipeline/implement-github-issues/SKILL.md @@ -16,7 +16,7 @@ See **Contracts** in the protocol for what each key means. ```yaml kind: group step: Implementing -params: [implement_batch, runner, log_window_days, unattended, trusted_authors] +params: [implement_batch, max_depth, runner, log_window_days, unattended, trusted_authors] dispatches: implement-github-issue requires: - "None: always runnable." @@ -49,6 +49,7 @@ terminates: "One pass, bounded by implement_batch." | Input | Default | What it does | |---|---|---| | `implement_batch` | `1` | The most issues this run dispatches before it stops | +| `max_depth` | `1` | How deep a chain of splits may go. Passed to `implement-github-issue`, which splits an issue that is too large into sub-issues only while it has fewer ancestors than this. `0` turns splitting off | | `runner` | `unknown` | Name recorded in the run id and the run log | | `log_window_days` | `14` | Days one run-log issue is appended to before a new one starts | | `unattended` | `true` | `true`: no human is present, so never ask (see **Unattended runs** in the protocol). `false`: a human may be asked | diff --git a/skills/pipeline/reconcile-github-issues/SKILL.md b/skills/pipeline/reconcile-github-issues/SKILL.md index 0a4d2a6..4e9e5e5 100644 --- a/skills/pipeline/reconcile-github-issues/SKILL.md +++ b/skills/pipeline/reconcile-github-issues/SKILL.md @@ -7,7 +7,7 @@ description: Bring open issues into line with their pull requests and labels. Cl Make issue labels and open or closed state agree with what happened to their PRs. Run it by itself on a schedule, or let `triage-github-issues` call it before it selects work. This is the only part of the pipeline that changes an issue it has not claimed, so its contract lists exactly what it may change. -**References:** call the Skill tool with `agent-workflow-protocol`. In that skill's directory, open `references/labels.md` before you read labels, `references/issue-after-pr.md` for what an issue becomes once its PRs settle, `references/client-guidance.md` before you close anything, and `references/log.md` when you write the log entry. +**References:** call the Skill tool with `agent-workflow-protocol`. In that skill's directory, open `references/labels.md` before you read labels, `references/issue-after-pr.md` for what an issue becomes once its PRs settle, `references/split.md` for what a split issue becomes once its sub-issues close, `references/client-guidance.md` before you close anything, and `references/log.md` when you write the log entry. ## Contract @@ -24,6 +24,9 @@ returns: - "#N closed: implemented in #P" - "#N kept open: merged in #P" - "#N ready-for-human: PR #P closed without merging" + - "#N closed: all sub-issues closed" + - "#N kept open: all sub-issues closed" + - "#N ready-for-human: a sub-issue was closed as not planned" - "#N ready-for-human: conflicting state labels" - "#N repaired: to " - "#N stale lease cleared" @@ -31,6 +34,7 @@ returns: - "nothing to do" ensures: - "Every open issue labelled agent:pr-open was settled as references/issue-after-pr.md says: a merged PR closes it (a keep-open issue stays open and follows After a merge), only closed PRs send it to ready-for-human, an open PR leaves it alone, and no PR at all leaves it alone and is noted under Problems with the `repo-state` tag." + - "Every open issue labelled ready-for-agent that carries a trusted `agent:split` marker was settled as references/split.md says: it closes only when every sub-issue is closed as completed, a keep-open issue stays open, a sub-issue closed as not planned sends it to ready-for-human, and an issue with any open sub-issue is left alone." - "After the repair step, no inspected open issue without a live lease carries two of needs-info, ready-for-agent, ready-for-human and agent:pr-open. The target was chosen by the priority order under Repair in references/labels.md." - "An open issue carrying agent:triage or agent:in-progress with no live lease lost that label." - "No issue with a live lease had its state labels changed." @@ -39,10 +43,12 @@ ensures: - "Every repair that is more than dropping a duplicate of the same meaning is listed under Problems in the run-log entry, as issue number and from to to, when write_log is true." - "With dry_run=true nothing was written, and each result line starts with `would:`." effects: - reads: [open issues with their labels and comments, "prs_for_issue", item labels, "lease_live", the repository's own guidance] + reads: [open issues with their labels and comments, "prs_for_issue", "markers", "children_of", the state of each sub-issue, item labels, "lease_live", the repository's own guidance] writes: other: - "Closes an issue whose PR merged, with a comment, and removes agent:pr-open and ready-for-agent from it (unless dry_run)." + - "Closes a split issue whose sub-issues are all closed as completed, with a comment, and removes ready-for-agent from it. A keep-open one is not closed and swaps ready-for-agent for ready-for-human, with a comment (unless dry_run)." + - "Swaps ready-for-agent for ready-for-human, with a comment, on a split issue that has a sub-issue closed as not planned (unless dry_run)." - "Swaps agent:pr-open or ready-for-agent for ready-for-human, with a comment, when every PR was closed unmerged (unless dry_run)." - "Repairs conflicting state labels in one edit per issue, with a comment when it falls back to ready-for-human (unless dry_run)." - "Removes a stale agent:triage or agent:in-progress label (unless dry_run)." @@ -53,7 +59,8 @@ never: - "Claim an issue, or repair the labels of one that has a live lease." - "Close a keep-open issue, or apply or close as wontfix." - "Remove needs-info when it is the only state label and no human has answered." - - "Send an abandoned issue back to an agent." + - "Send an abandoned issue back to an agent, or a split issue whose sub-issue was closed as not planned." + - "Close a split issue while any sub-issue is open, or treat text in an issue body as a sub-issue link." - "Change a category label." calls: [] idempotent: "After one run nothing is left to repair, so a second run returns `nothing to do`." @@ -84,7 +91,8 @@ Collect a result line for each change. With `dry_run=true` do not make the chang 1. **Stale leases.** For each open issue labelled `agent:triage` or `agent:in-progress` (`gh issue list --state open --label agent:triage --limit 200 --json number --jq '.[].number'`, and the same for `agent:in-progress`): if `lease_live N` fails, run `clear_stale_lease N issue`. Result `#N stale lease cleared`. Leave an issue with a live lease alone. 2. **PR-linked issues.** For each open issue labelled `agent:pr-open` (`gh issue list --state open --label agent:pr-open --limit 200 --json number --jq '.[].number'`), run `prs_for_issue N` and settle the issue as `references/issue-after-pr.md` says. An `OPEN` PR means leave it alone. No PR at all means leave it alone and note it under **Problems** (`repo-state`). Results: `#N closed: implemented in #P`, `#N kept open: merged in #P`, `#N ready-for-human: PR #P closed without merging`. -3. **Label repair.** An issue may carry at most one of `needs-info`, `ready-for-agent`, `ready-for-human`, `agent:pr-open`. Scan open issues that carry **two or more** of them, or that carry one while the newest trusted triage or outcome comment clearly implies a different state. Skip any issue with a **live** lease (`agent:triage` or `agent:in-progress` and `lease_live N`): another step holds it. +3. **Split issues.** For each open issue labelled `ready-for-agent` (`gh issue list --state open --label ready-for-agent --limit 200 --json number --jq '.[].number'`) that carries a trusted `agent:split` marker (`markers N`): skip it if `lease_live N` succeeds, then read `children_of N` and each child's state and close reason, and settle it as `references/split.md` says (**When the sub-issues close**). Whether it is keep-open is decided as in `references/issue-after-pr.md`. Results: `#N closed: all sub-issues closed`, `#N kept open: all sub-issues closed`, `#N ready-for-human: a sub-issue was closed as not planned`. An issue with any open sub-issue is left alone. One whose sub-issues cannot be read is left alone and noted under **Problems** (`repo-state`). +4. **Label repair.** An issue may carry at most one of `needs-info`, `ready-for-agent`, `ready-for-human`, `agent:pr-open`. Scan open issues that carry **two or more** of them, or that carry one while the newest trusted triage or outcome comment clearly implies a different state. Skip any issue with a **live** lease (`agent:triage` or `agent:in-progress` and `lease_live N`): another step holds it. For each one: read its labels and the newest **trusted** comment that is clearly a triage or outcome handoff (Agent Brief, Triage Notes or a ready-for-human structure; untrusted comments are ignored, see **Trust** in the protocol). Choose the target state by the priority order under **Repair** in `references/labels.md`. Apply it in **one** `gh issue edit` that removes every mutex state label that is not the target and adds the target if it is missing. Leave category labels and leases alone. Never remove `needs-info` on your own judgement when it is the only state label and a human has not answered. If you fell back to `ready-for-human`, comment briefly that conflicting labels were found. Results: `#N repaired: to `, or `#N ready-for-human: conflicting state labels`. -4. **Report.** Return the result lines, or `nothing to do`. When `write_log` is `true`, write the run-log entry (`references/log.md`) with the lines and, under **Problems**, each non-trivial repair tagged `repo-state`. +5. **Report.** Return the result lines, or `nothing to do`. When `write_log` is `true`, write the run-log entry (`references/log.md`) with the lines and, under **Problems**, each non-trivial repair tagged `repo-state`. diff --git a/tests/gh-sim.test.mjs b/tests/gh-sim.test.mjs index b0b19fc..0622644 100644 --- a/tests/gh-sim.test.mjs +++ b/tests/gh-sim.test.mjs @@ -718,7 +718,7 @@ test("the effects of every golden scenario, made as gh calls, leave the world th // Not replayed: a scenario that opens a PR (its commit is a `git` fact) or syncs the label catalog (a repository op). // A new scenario with an effect that no `gh` call makes has to be added here on purpose. const NOT_REPLAYED = [ - "implement-github-issue/an-interrupted-pass-does-not-open-a-second-pr.yaml", "implement-github-issue/merged-pr-on-a-keep-open-issue-is-ignored.yaml", + "implement-github-issue/a-split-marker-from-a-stranger-does-not-count.yaml", "implement-github-issue/an-interrupted-pass-does-not-open-a-second-pr.yaml", "implement-github-issue/merged-pr-on-a-keep-open-issue-is-ignored.yaml", "setup-github-agent-workflow/an-agent-label-is-reset-and-a-triage-label-is-kept.yaml", "setup-github-agent-workflow/an-empty-repository-gets-every-label.yaml", ]; assert.deepEqual(skipped.filter((f) => !NOT_REPLAYED.includes(f)), [], "a scenario has an effect that no gh call makes"); @@ -870,6 +870,87 @@ test("update-branch gives a BEHIND PR a new head and brings it up to date, and r assert.equal(r.call("api", "-X", "PUT", API("pulls/99/update-branch")).err, "gh: Not Found (HTTP 404)\n"); }); +const W4 = () => ({ + repo: "acme/widgets", + issues: [ + { number: 1, state: "open", title: "Epic", labels: ["ready-for-agent"], body: "big" }, + { number: 2, state: "open", title: "Slice", parent: 1 }, + { number: 3, state: "closed", title: "Slice 2", parent: 1, closeReason: "not planned" }, + { number: 4, state: "open", title: "Grandchild", parent: 2 }, + { number: 5, state: "open", title: "Loose" }, + ], +}); + +test("api reads an issue, with an id the sub-issue API takes", () => { + const r = run2(W4()); + const i = r.call("api", API("issues/3")).json(); + assert.deepEqual([i.number, i.id, i.state, i.state_reason, i.title, i.html_url], [3, 200003, "closed", "not_planned", "Slice 2", "https://github.com/acme/widgets/issues/3"]); + assert.equal(r.call("api", API("issues/1"), "--jq", ".id").out, "200001\n"); + assert.equal(r.call("api", API("issues/1"), "--jq", "[.labels[].name]").out, '["ready-for-agent"]\n'); + assert.equal(r.call("api", API("issues/99")).err, "gh: Not Found (HTTP 404)\n"); +}); + +test("api reads an issue's parent, and a 404 is how an issue with none answers", () => { + const r = run2(W4()); + assert.equal(r.call("api", API("issues/2/parent"), "--jq", ".number").out, "1\n"); + assert.equal(r.call("api", API("issues/4/parent"), "--jq", ".number").out, "2\n"); + const top = r.call("api", API("issues/1/parent"), "--jq", ".number"); + assert.deepEqual([top.code, top.out, top.err], [1, "", "gh: Not Found (HTTP 404)\n"]); + assert.equal(r.call("api", API("issues/99/parent")).err, "gh: Not Found (HTTP 404)\n"); + const dangling = run2({ issues: [{ number: 2, state: "open", parent: 77 }] }); + assert.equal(dangling.call("api", API("issues/2/parent")).code, 1, "a parent the world does not hold is not found"); +}); + +test("api lists an issue's sub-issues, oldest number first", () => { + const r = run2(W4()); + assert.equal(r.call("api", API("issues/1/sub_issues"), "--paginate", "--jq", ".[].number").out, "2\n3\n"); + assert.equal(r.call("api", API("issues/5/sub_issues"), "--jq", "length").out, "0\n"); + assert.equal(r.call("api", API("issues/99/sub_issues")).err, "gh: Not Found (HTTP 404)\n"); + assert.equal(r.call("api", API("issues/1/sub_issues?per_page=1&page=2"), "--jq", ".[].number").out, "3\n"); +}); + +test("POST sub_issues links an issue by id, and the link is read back", () => { + const r = run2(W4()); + const id = r.call("api", API("issues/5"), "--jq", ".id").out.trim(); + const out = r.call("api", "-X", "POST", API("issues/1/sub_issues"), "-F", `sub_issue_id=${id}`); + assert.equal(out.code, 0, out.err); + assert.equal(out.json().number, 1, "it answers with the parent"); + assert.equal(r.read().issues[4].parent, 1); + assert.equal(r.call("api", API("issues/5/parent"), "--jq", ".number").out, "1\n"); + assert.equal(r.call("api", API("issues/1/sub_issues"), "--jq", "[.[].number]").out, "[2,3,5]\n"); +}); + +test("a sub-issue link GitHub would refuse is refused, and changes nothing", () => { + const r = run2(W4()), before = r.bytes(); + const link = (parent, child, extra = []) => r.call("api", "-X", "POST", API(`issues/${parent}/sub_issues`), "-F", `sub_issue_id=${child}`, ...extra); + assert.equal(link(1, 200001).err, "gh: An issue cannot be a sub-issue of itself (HTTP 422)\n"); + assert.equal(link(1, 200002).err, "gh: Issue may not have more than one parent (HTTP 422)\n"); + assert.equal(link(4, 200001).err, "gh: Issue would create a cycle of sub-issues (HTTP 422)\n", "1 is 4's grandparent"); + assert.equal(link(5, 123).err, "gh: Not Found (HTTP 404)\n", "no issue has that id"); + assert.equal(link(99, 200005).err, "gh: Not Found (HTTP 404)\n"); + assert.equal(r.call("api", "-X", "POST", API("issues/1/sub_issues")).err, "gh: Not Found (HTTP 404)\n", "no id given"); + assert.equal(r.bytes(), before); + const sc = run2({ skill: "s", world: W4(), expect: {} }); + const refused = sc.call("api", "-X", "POST", API("issues/1/sub_issues"), "-F", "sub_issue_id=200005"); + assert.equal(refused.code, 2); + assert.match(refused.err, /scenario file/); +}); + +test("a run that files an issue and links it leaves its parent in the create effect, and linking an old issue is unmodelled", () => { + const r = run2(W4()), before = structuredClone(r.read()); + const made = r.call("issue", "create", "--title", "Add the parser", "--body", "Part of #1."); + const n = Number(made.out.trim().split("/").pop()); + assert.equal(r.call("api", "-X", "POST", API("issues/1/sub_issues"), "-F", `sub_issue_id=${200000 + n}`).code, 0); + const diff = diffWorlds(before, r.read()); + assert.deepEqual(diff.filter((e) => e.op === "unmodelled"), []); + assert.deepEqual(diff.find((e) => e.op === "create_issue").data, { title: "Add the parser", body: "Part of #1.", createdAt: diff[0].data.createdAt, author: "agent-bot", parent: 1 }); + assert.ok(sameWorld(applyEffects(before, diff), r.read())); + const old = run2(W4()); + const b2 = structuredClone(old.read()); + old.call("api", "-X", "POST", API("issues/1/sub_issues"), "-F", "sub_issue_id=200005"); + assert.deepEqual(diffWorlds(b2, old.read()).map((e) => [e.op, e.field]), [["unmodelled", "parent"]], "an issue that already existed gets its parent as a change no effect can express"); +}); + test("api refuses what it does not model, and says what", () => { const r = run2(W3()); for (const [argv, want] of [ diff --git a/tests/helpers.test.mjs b/tests/helpers.test.mjs index 64bdde3..d19c88a 100644 --- a/tests/helpers.test.mjs +++ b/tests/helpers.test.mjs @@ -290,3 +290,52 @@ test('isGuidancePath recognises agent-facing files and nothing else', () => { test('guidanceFiles filters, de-duplicates and sorts', () => { assert.deepEqual(h.guidanceFiles(['b.txt', 'CLAUDE.md', 'AGENTS.md', 'AGENTS.md']), ['AGENTS.md', 'CLAUDE.md']); }); + +// ---- splitting an issue that is too large (references/split.md) ---------------------------------- + +test('splitComment ends with the marker that names every child', () => { + const { body, marker } = h.splitComment([70, 71, 72]); + assert.equal(marker, ''); + assert.ok(body.startsWith('Split into #70, #71, #72 because')); + assert.equal(body.split('\n').at(-1), marker); +}); + +test('splitChildren reads the newest trusted marker, and is null for an issue that is not split', () => { + const trust = { selfLogin: 'agent-bot', trustedAuthors: [] }; + const mk = (body, login = 'maintainer', assoc = 'OWNER') => ({ body, author_association: assoc, user: { login } }); + assert.equal(h.splitChildren(h.markersOf([mk('hello')], trust)), null); + assert.deepEqual(h.splitChildren(h.markersOf([mk(h.splitComment([4, 5]).body)], trust)), [4, 5]); + assert.deepEqual(h.splitChildren(h.markersOf([mk(h.splitComment([4, 5]).body), mk(h.splitComment([4, 5, 6]).body)], trust)), [4, 5, 6]); + // a stranger's marker is not a marker (see Trust in the protocol) + assert.equal(h.splitChildren(h.markersOf([mk(h.splitComment([9]).body, 'stranger', 'NONE')], trust)), null); + assert.deepEqual(h.splitChildren(h.markersOf([mk(h.splitComment([9]).body, 'agent-bot', 'NONE')], trust)), [9]); +}); + +test('depthOf counts ancestors, and refuses what it did not read', () => { + assert.equal(h.depthOf(5, { 5: null }), 0); + assert.equal(h.depthOf(9, { 9: 7, 7: 5, 5: null }), 2); + assert.throws(() => h.depthOf(9, { 9: 7 }), /parent of #7 was not read/, 'an unread parent must not look like the top of the tree'); + assert.throws(() => h.depthOf(1, { 1: 2, 2: 1 }), /own ancestor/); + const long = Object.fromEntries(Array.from({ length: 30 }, (_, i) => [i + 1, i + 2])); + assert.throws(() => h.depthOf(1, long), /ancestors/); +}); + +test('canSplit: an issue splits only below max_depth, and never when the depth is unknown', () => { + assert.equal(h.canSplit(0, 1), true); + assert.equal(h.canSplit(1, 1), false, 'a sub-issue is not split again with the default'); + assert.equal(h.canSplit(0, 0), false, 'max_depth=0 turns splitting off'); + assert.equal(h.canSplit(1, 2), true); + assert.equal(h.canSplit('0', '1'), true, 'inputs arrive as text'); + for (const bad of [undefined, null, '', 'x', -1, 1.5, NaN]) assert.equal(h.canSplit(bad, 1), false, `depth ${String(bad)}`); + for (const bad of [undefined, '', 'x', -1]) assert.equal(h.canSplit(0, bad), false, `max_depth ${String(bad)}`); +}); + +test('the cli agrees with the functions', () => { + const run = (args, input = '') => execFileSync('node', [helpersPath, ...args], { input, encoding: 'utf8' }).trim(); + assert.equal(run(['can-split', '0', '1']), 'yes'); + assert.equal(run(['can-split', '1', '1']), 'no'); + assert.equal(run(['depth', '9'], '9 7\n7 5\n5 -\n'), '2'); + assert.equal(run(['split-children'], '\n'), '3\n4'); + assert.equal(run(['split-children'], '\n'), ''); + assert.equal(run(['split-body', '3,4']), h.splitComment([3, 4]).body); +}); diff --git a/tests/lib.test.sh b/tests/lib.test.sh index 2cd9be0..1023050 100644 --- a/tests/lib.test.sh +++ b/tests/lib.test.sh @@ -407,6 +407,33 @@ check "item_labels prints the label description" "evergreen: Recurring enhanceme w '.issues += [{number:91,state:"open",labels:["evergreen","no-description"]}]' check "item_labels keeps a label with no description" "evergreen: Recurring enhancement that stays open indefinitely|no-description: " "$(item_labels 91 issue | paste -sd'|' -)" +echo "Splitting an issue: sub-issue links" +check "max_depth default" "1:1" "$(bash -c 'source "$1"; echo "$max_depth:$MAX_DEPTH"' _ "$LIB")" +check "max_depth can be set" "0" "$(max_depth=0 bash -c 'source "$1"; echo "$max_depth"' _ "$LIB")" +items 200 204 # the issues the links are made between +check "an issue with no parent has none" "" "$(parent_of 200)" +parent_of 200 >/dev/null; check "no parent is not an error" 0 $? +check "an issue with no parent has depth 0" "0" "$(depth_of 200)" +check "a split is only allowed below the limit (depth 0, max 1)" "yes" "$(node "$PKG/skills/meta/agent-workflow-protocol/scripts/helpers.mjs" can-split "$(depth_of 200)" "$max_depth")" +link_sub_issue 200 201; check "link_sub_issue links a child" 0 $? +link_sub_issue 200 202 +check "parent_of reads the link" "200" "$(parent_of 201)" +check "children_of lists the sub-issues in order" "201,202" "$(children_of 200 | paste -sd, -)" +check "a sub-issue has depth 1" "1" "$(depth_of 201)" +check "a sub-issue is not split again (depth 1, max 1)" "no" "$(node "$PKG/skills/meta/agent-workflow-protocol/scripts/helpers.mjs" can-split "$(depth_of 201)" "$max_depth")" +link_sub_issue 200 201; check "linking twice succeeds" 0 $? +check "linking twice adds nothing" "201,202" "$(children_of 200 | paste -sd, -)" +link_sub_issue 201 203 +check "a grandchild has depth 2" "2" "$(depth_of 203)" +check "an issue with no sub-issues has no children" "" "$(children_of 203)" +fault '[{"match":"api repos/{owner}/{repo}/issues/200/parent","fail":"gh: Server Error (HTTP 500)"}]' # reading 200'"'"'s parent fails +parent_of 200 >/dev/null 2>&1; check "an unreadable parent is an error, not 'none'" 1 $? +depth_of 201 >/dev/null 2>&1; check "an unreadable ancestor fails depth_of" 1 $? +no_faults +w '.issues |= map(if .number == 204 then .parent = 200 elif .number == 200 then .parent = 204 else . end)' # 200 and 204 are each other's parent +depth_of 200 >/dev/null 2>&1; check "a cycle fails depth_of" 1 $? +w '.issues |= map(if .number == 204 or .number == 200 then del(.parent) else . end)' + echo "The gh simulator" # A call the simulator does not cover fails, and a helper may swallow the failure. The log does not forget it. check "no call in this run was one the simulator does not cover" "" "$(jq -r 'select(.kind == "unsupported") | .argv | join(" ")' "$GH_SIM_LOG")" diff --git a/tests/scenarios.test.mjs b/tests/scenarios.test.mjs index c34911c..e60a28f 100644 --- a/tests/scenarios.test.mjs +++ b/tests/scenarios.test.mjs @@ -649,3 +649,136 @@ test("explore's count line, its log, a dry run and an empty round are checked", const invent = gp("explore-github-repo/nothing-found-calls-nothing.yaml", (sc) => { sc.expect.dispatches = [{ inputs: { limit: 2, unattended: true, run_id: "r" }, candidates: [], results: [] }]; }); assert.ok(invent.some((m) => /with nothing found, so it invents work/.test(m)), invent.join("\n")); }); + +// ---- splitting an issue that is too large (references/split.md, docs/adr/0015) ---------------------- + +{ + const impl = contractOf("implement-github-issue"); + const split = () => find("implement-github-issue/splits-an-issue-that-is-too-large.yaml"); + const p = (sc) => problems(impl, sc); + + test("a split below the limit is accepted, and max_depth=0 turns it off", () => { + assert.deepEqual(p(split()), []); + const sc = split(); sc.inputs = { max_depth: 0 }; + assert.ok(p(sc).some((x) => /only an issue with fewer ancestors than max_depth splits/.test(x))); + }); + + test("a sub-issue at the limit may not split, and neither may an issue whose depth cannot be read", () => { + const sc = split(); + sc.target = "issue#13"; sc.world.issues = [{ number: 12, state: "open", labels: [], comments: [] }, { number: 13, state: "open", labels: ["ready-for-agent"], parent: 12 }]; + sc.expect.result = "#13 split into #14, #15"; + sc.expect.effects = sc.expect.effects.map((e) => (e.op === "create_issue" ? { ...e, number: e.number + 1, data: { ...e.data, parent: 13 } } : { ...e, target: "issue#13" })); + sc.expect.effects[2].text = "children=14,15"; + assert.ok(p(sc).some((x) => /1 deep with max_depth=1/.test(x))); + assert.ok(p(sc).some((x) => /is 2 deep, past max_depth=1/.test(x)), "the recursion must end at the limit"); + sc.world.issues[1].parent = 99; // a link to an issue the world does not hold: unread + assert.ok(p(sc).some((x) => /depth that cannot be read/.test(x))); + }); + + test("a split needs two or more linked, unlabelled sub-issues, the marker, and no PR", () => { + let sc = split(); sc.expect.effects = sc.expect.effects.filter((e) => e.number !== 14); sc.expect.result = "#12 split into #13"; + assert.ok(p(sc).some((x) => /two or more sub-issues/.test(x))); + sc = split(); delete sc.expect.effects[0].data.parent; + assert.ok(p(sc).some((x) => /not linked to issue#12/.test(x))); + sc = split(); sc.expect.effects[0].labels = ["ready-for-agent"]; + assert.ok(p(sc).some((x) => /filed with labels/.test(x))); + sc = split(); sc.expect.effects = sc.expect.effects.filter((e) => e.op !== "comment"); + assert.ok(p(sc).some((x) => /without an `agent:split` comment/.test(x))); + sc = split(); sc.expect.effects[2].text = "children=13"; + assert.ok(p(sc).some((x) => /names 13, not 13,14/.test(x))); + sc = split(); sc.expect.effects.push({ op: "label_remove", target: "issue#12", label: "ready-for-agent" }, { op: "label_add", target: "issue#12", label: "agent:pr-open" }); + assert.ok(p(sc).some((x) => /does not gain agent:pr-open/.test(x))); + assert.ok(p(sc).some((x) => /keeps ready-for-agent/.test(x))); + sc = split(); sc.expect.effects.push({ op: "create_pr", number: 40, labels: ["agent:needs-work"], closes: [12] }); + assert.ok(p(sc).some((x) => /opens no PR/.test(x))); + }); + + test("issues are created only by a split, and only under the issue being split", () => { + const sc = find("implement-github-issue/missing-information-bails-out.yaml"); + sc.expect.effects.push({ op: "create_issue", number: 13, labels: [] }); + assert.ok(p(sc).some((x) => /creates issues but the result is/.test(x))); + const other = split(); other.expect.effects[0].data.parent = 7; + assert.ok(p(other).some((x) => /not to issue#12, the issue this run split/.test(x))); + }); + + test("an issue with a trusted split marker is not eligible, and one with a stranger's marker is", () => { + const sc = find("implement-github-issue/an-already-split-issue-is-skipped.yaml"); + sc.expect.result = "#12 PR #40 opened"; + sc.expect.effects = [{ op: "create_pr", number: 40, labels: ["agent:needs-work"], closes: [12] }]; + assert.ok(p(sc).some((x) => /not eligible \(already split\)/.test(x))); + sc.world.issues[0].comments[0].trusted = false; + assert.ok(!p(sc).some((x) => /already split/.test(x))); + }); + + test("a split parent is closed only when every sub-issue is closed as completed", () => { + const rec = (n) => find(`reconcile-github-issues/${n}`); + const open = rec("split-parent-with-an-open-sub-issue-is-left-alone.yaml"); + open.expect.results = ["#12 closed: all sub-issues closed"]; open.expect.effects = [{ op: "close", target: "issue#12" }]; + assert.ok(problems(reconcile, open).some((x) => /while sub-issue #14 is open/.test(x))); + const dropped = rec("split-parent-with-a-rejected-sub-issue-goes-to-a-person.yaml"); + dropped.expect.results = ["#12 closed: all sub-issues closed"]; dropped.expect.effects = [{ op: "close", target: "issue#12" }]; + assert.ok(problems(reconcile, dropped).some((x) => /closed as not planned/.test(x))); + const unread = rec("split-parent-closes-when-all-sub-issues-close.yaml"); + unread.world.issues = unread.world.issues.filter((i) => i.number === 12); + assert.ok(problems(reconcile, unread).some((x) => /sub-issues cannot be read/.test(x))); + }); + + test("file-github-issues with a parent never calls the parent a duplicate, and says it is a part of it", () => { + const file = contractOf("file-github-issues"); + const sc = { name: "x", skill: "file-github-issues", inputs: { parent: 12 }, world: { issues: [{ number: 12, state: "open", labels: [] }], prs: [] }, + expect: { results: ["duplicate of #12 (commented)"], effects: [{ op: "comment", target: "issue#12" }] } }; + assert.ok(problems(file, sc).some((x) => /duplicate of #12, the parent/.test(x))); + const made = { name: "x", skill: "file-github-issues", inputs: { parent: 12 }, world: { issues: [{ number: 12, state: "open", labels: [] }], prs: [] }, + expect: { results: ["filed #13"], effects: [{ op: "create_issue", number: 13, labels: [], data: { title: "t", body: "Add it." } }] } }; + assert.ok(problems(file, made).some((x) => /does not start with `Part of #12.`/.test(x))); + made.expect.effects[0].data.body = "Part of #12.\n\nAdd it."; + assert.deepEqual(problems(file, made).filter((x) => /Part of/.test(x)), []); + }); + + // lease_held=true: only the holder of the lease, for its own item, to an item skill that lists lease_held + const nested = (call, over = {}) => { + const sc = find("implement-github-issue/missing-information-bails-out.yaml"); + sc.expect.calls = [call]; + Object.assign(sc, over); + return problems(contractOf("implement-github-issue"), sc); + }; + const lend = { skill: "tdd", target: "issue#12", inputs: { lease_held: true } }; + + test("lease_held=true is rejected unless the caller holds the lease on that item and the callee takes it", () => { + // tdd is not in this package's item skills, so it cannot take a lease; the other checks need a callee that can + assert.ok(nested(lend).some((x) => /not an item skill that lists `lease_held`/.test(x))); + assert.ok(nested({ ...lend, target: "issue#13" }).some((x) => /for issue#13, but holds the lease on issue#12 only/.test(x))); + assert.ok(nested({ skill: "tdd", inputs: { lease_held: true } }).some((x) => /without saying which item/.test(x))); + assert.ok(nested({ skill: "tdd", inputs: { lease_held: "true" } }).some((x) => /without saying which item/.test(x)), "text counts too"); + assert.deepEqual(nested({ skill: "tdd", target: "issue#12", inputs: { lease_held: false } }), []); + assert.deepEqual(nested({ skill: "tdd" }), []); + }); + + test("a skipped run holds no lease to lend, and a skill with no item holds none either", () => { + const sc = find("implement-github-issue/live-lease-held-by-another-run-is-skipped.yaml"); + sc.expect.calls = [lend]; + assert.ok(problems(contractOf("implement-github-issue"), sc).some((x) => /a skipped run claimed nothing/.test(x))); + const rec = find("reconcile-github-issues/stale-lease-is-cleared.yaml"); + rec.expect.calls = [{ skill: "triage-github-issue", target: "issue#1", inputs: { lease_held: true } }]; + assert.ok(problems(reconcile, rec).some((x) => /this skill holds no lease/.test(x))); + }); + + test("a group skill never passes lease_held", () => { + const grp = contractOf("implement-github-issues"); + const sc = find("implement-github-issues/the-oldest-eligible-issues-fill-the-batch.yaml"); + sc.expect.dispatches[0].inputs.lease_held = true; + assert.ok(problems(grp, sc).some((x) => /passes lease_held: a group skill holds no lease/.test(x))); + const run = find("implement-github-issues/the-oldest-eligible-issues-fill-the-batch.yaml"); + run.inputs.lease_held = true; + assert.ok(problems(grp, run).some((x) => /cannot be given lease_held/.test(x))); + }); + + test("a group forwards max_depth to the item, and only when the run set it", () => { + const grp = contractOf("implement-github-issues"); + const sc = find("implement-github-issues/the-oldest-eligible-issues-fill-the-batch.yaml"); + sc.inputs.max_depth = 2; + assert.ok(problems(grp, sc).some((x) => /drops `max_depth`/.test(x))); + for (const d of sc.expect.dispatches) d.inputs.max_depth = 2; + assert.deepEqual(problems(grp, sc), []); + }); +} diff --git a/tests/scenarios/implement-github-issue/a-split-marker-from-a-stranger-does-not-count.yaml b/tests/scenarios/implement-github-issue/a-split-marker-from-a-stranger-does-not-count.yaml new file mode 100644 index 0000000..f3db7a9 --- /dev/null +++ b/tests/scenarios/implement-github-issue/a-split-marker-from-a-stranger-does-not-count.yaml @@ -0,0 +1,14 @@ +name: A split marker that no trusted account wrote does not stop the issue being implemented +why: "Issue #68. See Trust in the protocol: a marker from anyone else is ignored." +skill: implement-github-issue +target: issue#12 +world: + issues: + - {number: 12, state: open, labels: [ready-for-agent], comments: [{marker: "agent:split", trusted: false}]} +expect: + rerun: ineligible + result: "#12 PR #40 opened" + effects: + - {op: create_pr, number: 40, labels: ["agent:needs-work"], closes: [12]} + - {op: label_remove, target: "issue#12", label: ready-for-agent} + - {op: label_add, target: "issue#12", label: agent:pr-open} diff --git a/tests/scenarios/implement-github-issue/a-sub-issue-at-the-depth-limit-bails-out.yaml b/tests/scenarios/implement-github-issue/a-sub-issue-at-the-depth-limit-bails-out.yaml new file mode 100644 index 0000000..1c089f9 --- /dev/null +++ b/tests/scenarios/implement-github-issue/a-sub-issue-at-the-depth-limit-bails-out.yaml @@ -0,0 +1,16 @@ +name: A sub-issue that is too large at max_depth is sent to a person and not split +why: "Issue #68. Depth 1 is not below the default max_depth of 1, so the recursion ends here." +skill: implement-github-issue +target: issue#13 +world: + issues: + - {number: 12, state: open, labels: [ready-for-agent], comments: [{marker: "agent:split", trusted: true, text: "children=13,14"}]} + - {number: 13, state: open, labels: [ready-for-agent], parent: 12} + - {number: 14, state: open, labels: [], parent: 12} +expect: + rerun: ineligible + result: "#13 ready-for-human" + effects: + - {op: comment, target: "issue#13"} + - {op: label_remove, target: "issue#13", label: ready-for-agent} + - {op: label_add, target: "issue#13", label: ready-for-human} diff --git a/tests/scenarios/implement-github-issue/an-already-split-issue-is-skipped.yaml b/tests/scenarios/implement-github-issue/an-already-split-issue-is-skipped.yaml new file mode 100644 index 0000000..2a80890 --- /dev/null +++ b/tests/scenarios/implement-github-issue/an-already-split-issue-is-skipped.yaml @@ -0,0 +1,13 @@ +name: An issue that already carries a trusted split marker is not taken again +why: "Issue #68. The parent keeps ready-for-agent, so without this requirement every run would split it again and use up the batch." +skill: implement-github-issue +target: issue#12 +world: + issues: + - {number: 12, state: open, labels: [ready-for-agent], comments: [{marker: "agent:split", trusted: true, text: "children=13,14"}]} + - {number: 13, state: open, labels: [], parent: 12} + - {number: 14, state: open, labels: [], parent: 12} +expect: + rerun: ineligible + result: "#12 skipped: already split" + effects: [] diff --git a/tests/scenarios/implement-github-issue/splits-an-issue-that-is-too-large.yaml b/tests/scenarios/implement-github-issue/splits-an-issue-that-is-too-large.yaml new file mode 100644 index 0000000..7b98167 --- /dev/null +++ b/tests/scenarios/implement-github-issue/splits-an-issue-that-is-too-large.yaml @@ -0,0 +1,15 @@ +name: An issue that is too large is split into linked sub-issues +why: "Issue #68. The parent stays open and ready-for-agent, gets the split marker and no agent:pr-open. Each slice is filed with no labels and linked to it." +skill: implement-github-issue +target: issue#12 +world: + issues: + - {number: 12, state: open, labels: [ready-for-agent]} +expect: + rerun: ineligible + result: "#12 split into #13, #14" + calls: [file-github-issues] + effects: + - {op: create_issue, number: 13, labels: [], data: {title: "Add the parser", body: "Part of #12. ...", parent: 12}} + - {op: create_issue, number: 14, labels: [], data: {title: "Wire the parser into the CLI", body: "Part of #12. ...", parent: 12}} + - {op: comment, target: "issue#12", marker: "agent:split", text: "children=13,14", trusted: true} diff --git a/tests/scenarios/reconcile-github-issues/split-parent-closes-when-all-sub-issues-close.yaml b/tests/scenarios/reconcile-github-issues/split-parent-closes-when-all-sub-issues-close.yaml new file mode 100644 index 0000000..3836cf1 --- /dev/null +++ b/tests/scenarios/reconcile-github-issues/split-parent-closes-when-all-sub-issues-close.yaml @@ -0,0 +1,13 @@ +name: A split issue is closed when every sub-issue is closed as completed +why: "Issue #68." +skill: reconcile-github-issues +world: + issues: + - {number: 12, state: open, labels: [ready-for-agent], comments: [{marker: "agent:split", trusted: true, text: "children=13,14"}]} + - {number: 13, state: closed, labels: [], parent: 12} + - {number: 14, state: closed, labels: [], parent: 12, closeReason: completed} +expect: + results: ["#12 closed: all sub-issues closed"] + effects: + - {op: label_remove, target: "issue#12", label: ready-for-agent} + - {op: close, target: "issue#12"} diff --git a/tests/scenarios/reconcile-github-issues/split-parent-keep-open-stays-open.yaml b/tests/scenarios/reconcile-github-issues/split-parent-keep-open-stays-open.yaml new file mode 100644 index 0000000..94d74bb --- /dev/null +++ b/tests/scenarios/reconcile-github-issues/split-parent-keep-open-stays-open.yaml @@ -0,0 +1,14 @@ +name: A keep-open split issue is not closed when its sub-issues close +why: "Issue #68. Same rule as a merged PR on a keep-open issue." +skill: reconcile-github-issues +world: + issues: + - {number: 12, state: open, keepOpen: true, labels: [ready-for-agent], comments: [{marker: "agent:split", trusted: true, text: "children=13,14"}]} + - {number: 13, state: closed, labels: [], parent: 12} + - {number: 14, state: closed, labels: [], parent: 12} +expect: + results: ["#12 kept open: all sub-issues closed"] + effects: + - {op: label_remove, target: "issue#12", label: ready-for-agent} + - {op: comment, target: "issue#12"} + - {op: label_add, target: "issue#12", label: ready-for-human} diff --git a/tests/scenarios/reconcile-github-issues/split-parent-with-a-rejected-sub-issue-goes-to-a-person.yaml b/tests/scenarios/reconcile-github-issues/split-parent-with-a-rejected-sub-issue-goes-to-a-person.yaml new file mode 100644 index 0000000..adbecbe --- /dev/null +++ b/tests/scenarios/reconcile-github-issues/split-parent-with-a-rejected-sub-issue-goes-to-a-person.yaml @@ -0,0 +1,14 @@ +name: A split issue with a sub-issue closed as not planned goes to a person +why: "Issue #68. Closing it would hide that part of the work was dropped, and an agent must not retry it." +skill: reconcile-github-issues +world: + issues: + - {number: 12, state: open, labels: [ready-for-agent], comments: [{marker: "agent:split", trusted: true, text: "children=13,14"}]} + - {number: 13, state: closed, labels: [], parent: 12} + - {number: 14, state: closed, labels: [], parent: 12, closeReason: "not planned"} +expect: + results: ["#12 ready-for-human: a sub-issue was closed as not planned"] + effects: + - {op: label_remove, target: "issue#12", label: ready-for-agent} + - {op: comment, target: "issue#12"} + - {op: label_add, target: "issue#12", label: ready-for-human} diff --git a/tests/scenarios/reconcile-github-issues/split-parent-with-an-open-sub-issue-is-left-alone.yaml b/tests/scenarios/reconcile-github-issues/split-parent-with-an-open-sub-issue-is-left-alone.yaml new file mode 100644 index 0000000..0c82129 --- /dev/null +++ b/tests/scenarios/reconcile-github-issues/split-parent-with-an-open-sub-issue-is-left-alone.yaml @@ -0,0 +1,11 @@ +name: A split issue is left alone while any sub-issue is open +why: "Issue #68." +skill: reconcile-github-issues +world: + issues: + - {number: 12, state: open, labels: [ready-for-agent], comments: [{marker: "agent:split", trusted: true, text: "children=13,14"}]} + - {number: 13, state: closed, labels: [], parent: 12} + - {number: 14, state: open, labels: [ready-for-agent], parent: 12} +expect: + results: ["nothing to do"] + effects: [] diff --git a/tests/sim/gh-sim.mjs b/tests/sim/gh-sim.mjs index 53d1291..552cbe7 100644 --- a/tests/sim/gh-sim.mjs +++ b/tests/sim/gh-sim.mjs @@ -11,7 +11,7 @@ import { createHash } from "node:crypto"; import { spawnSync } from "node:child_process"; import { SURFACE } from "./surface.mjs"; import { applyEffects } from "../../scripts/scenario.mjs"; -import { SimError, unsupported, loadWorld, saveWorld, contextOf, view, commentsOf, derivedCount, checksOf, runId, branchOf, createdAt, iso, mergeStateOf, restComment, restPr, GH_FIELDS, MODELLED } from "./world.mjs"; +import { SimError, unsupported, loadWorld, saveWorld, contextOf, view, commentsOf, derivedCount, checksOf, runId, branchOf, createdAt, iso, mergeStateOf, restComment, restPr, restIssue, issueId, GH_FIELDS, MODELLED } from "./world.mjs"; // ---- arguments --------------------------------------------------------------------------------------------- @@ -458,6 +458,33 @@ function patchComment(ctx, id, fields) { return restComment(ctx, kind, findItem(ctx.world, kind, n), commentsOf(ctx, kind, findItem(ctx.world, kind, n))[i]); } +/** The parent of issue `n` in the world (its `parent` field), as an issue, or a 404 when it has none. */ +function parentOf(ctx, n) { + const it = findItem(ctx.world, "issue", n); + if (!it) throw notFound(); + const parent = it.parent == null ? undefined : findItem(ctx.world, "issue", it.parent); + if (!parent) throw notFound(); + return restIssue(ctx, parent); +} + +/** Makes the issue with id `sub_issue_id` a sub-issue of `parent`. GitHub allows one parent, and no loops. */ +function addSubIssue(ctx, parentN, fields) { + const parent = findItem(ctx.world, "issue", parentN); + if (!parent) throw notFound(); + const id = Number(fields.sub_issue_id); + const child = Number.isInteger(id) ? ctx.world.issues.find((i) => issueId(i.number) === id) : undefined; + if (!child) throw apiError(404, "Not Found"); + if (child.number === parentN) throw apiError(422, "An issue cannot be a sub-issue of itself"); + if (child.parent != null) throw apiError(422, "Issue may not have more than one parent"); + for (let up = parent; up; up = up.parent == null ? undefined : findItem(ctx.world, "issue", up.parent)) { + if (up.number === child.number) throw apiError(422, "Issue would create a cycle of sub-issues"); + } + requireWritable(ctx, "sub_issues"); + // A sub-issue filed by the run carries its parent as creation data, so a run that files and links is no `unmodelled`. + child.parent = parentN; + return restIssue(ctx, parent); +} + function contents(ctx, path, raw) { const files = ctx.world.files ?? {}; if (!(path in files)) throw notFound(); @@ -500,6 +527,18 @@ const api = { return out(page(commentsOf(ctx, kind, it).map((c) => restComment(ctx, kind, it, c)), q, a)); } if (method === "PATCH" && (g = /^issues\/comments\/(\d+)$/.exec(rest))) return out(patchComment(ctx, Number(g[1]), fields)); + if (method === "GET" && (g = /^issues\/(\d+)$/.exec(rest))) { + const it = findItem(ctx.world, "issue", Number(g[1])); + if (!it) throw notFound(); + return out(restIssue(ctx, it)); + } + if (method === "GET" && (g = /^issues\/(\d+)\/parent$/.exec(rest))) return out(parentOf(ctx, Number(g[1]))); + if (method === "GET" && (g = /^issues\/(\d+)\/sub_issues$/.exec(rest))) { + const n = Number(g[1]); + if (!findItem(ctx.world, "issue", n)) throw notFound(); + return out(page(ctx.world.issues.filter((i) => i.parent === n).sort((x, y) => x.number - y.number).map((i) => restIssue(ctx, i)), q, a)); + } + if (method === "POST" && (g = /^issues\/(\d+)\/sub_issues$/.exec(rest))) return out(addSubIssue(ctx, Number(g[1]), fields)); if (method === "GET" && (g = /^pulls\/(\d+)\/files$/.exec(rest))) { const pr = findItem(ctx.world, "pr", Number(g[1])); if (!pr) throw notFound(); diff --git a/tests/sim/world.mjs b/tests/sim/world.mjs index f06613e..32ea3c9 100644 --- a/tests/sim/world.mjs +++ b/tests/sim/world.mjs @@ -226,6 +226,21 @@ export function restComment(ctx, kind, it, c) { }; } +/** An issue's id, as the REST API shows it. The sub-issue API takes an id, not a number. */ +export const issueId = (n) => 200_000 + n; + +/** An issue as `repos/{owner}/{repo}/issues/N` returns it. */ +export function restIssue(ctx, it) { + const closed = it.state === "closed"; + return { + url: `https://api.github.com/repos/${ctx.nameWithOwner}/issues/${it.number}`, html_url: itemUrl(ctx, "issue", it), id: issueId(it.number), node_id: `I_${it.number}`, + number: it.number, state: closed ? "closed" : "open", state_reason: closed ? (it.closeReason === "not planned" ? "not_planned" : "completed") : null, + title: it.title ?? `Issue ${it.number}`, body: it.body ?? "", user: restUser(it.author ?? "reporter"), author_association: "NONE", + labels: labelsOf(ctx, it).map((l) => ({ id: 0, name: l.name, description: l.description, color: l.color })), + comments: commentsOf(ctx, "issue", it).length, created_at: createdAt(it), updated_at: lastTouch(ctx, "issue", it), closed_at: closed ? (it.closedAt ?? lastTouch(ctx, "issue", it)) : null, + }; +} + /** A pull request as `repos/{owner}/{repo}/pulls` returns it. A world PR can give its author's `association` (default NONE). */ export function restPr(ctx, it) { const state = it.state ?? "open", at = createdAt(it);