From b3a8bdf155e98331c516f51cc853135faef42074 Mon Sep 17 00:00:00 2001 From: claude Date: Fri, 13 Nov 2026 23:08:07 +0000 Subject: [PATCH] feat(eval): structured write allowances for the item skills (#67) triage-github-issue, implement-github-issue and review-github-pr list what they may write as allow entries (ADR 0017, step 2). 12 tests add an effect each skill's text forbids to a golden scenario. --- ...structured-write-allowances-item-skills.md | 5 ++ docs/adr/0017-structured-write-allowances.md | 13 ++++-- .../pipeline/implement-github-issue/SKILL.md | 45 ++++++++++++++---- skills/pipeline/review-github-pr/SKILL.md | 46 +++++++++++++++---- skills/pipeline/triage-github-issue/SKILL.md | 36 ++++++++++++--- tests/scenarios.test.mjs | 25 ++++++++++ 6 files changed, 142 insertions(+), 28 deletions(-) create mode 100644 .changeset/structured-write-allowances-item-skills.md diff --git a/.changeset/structured-write-allowances-item-skills.md b/.changeset/structured-write-allowances-item-skills.md new file mode 100644 index 0000000..23856e9 --- /dev/null +++ b/.changeset/structured-write-allowances-item-skills.md @@ -0,0 +1,5 @@ +--- +"github-agent-workflow": patch +--- + +`triage-github-issue`, `implement-github-issue` and `review-github-pr` now list what they may write as `allow` entries (step 2 of ADR 0017), so the scenario checker refuses a label, a comment marker or an op that the skill's text does not permit, even where its scope allows a write. For example triage applying `wontfix`, implementing merging a PR, or review sending a linked issue back to `ready-for-agent`. The wording of the skills' contracts is unchanged. diff --git a/docs/adr/0017-structured-write-allowances.md b/docs/adr/0017-structured-write-allowances.md index b999a2f..941c23d 100644 --- a/docs/adr/0017-structured-write-allowances.md +++ b/docs/adr/0017-structured-write-allowances.md @@ -2,7 +2,7 @@ ## Status -Accepted. Step 1 of 3 is built (#67): the schema, the checker and `shepherd-github-pr` as the worked example. The other skills move a scope at a time, so each step can be reviewed. +Accepted. Steps 1 and 2 of 3 are built (#67): the schema, the checker and `shepherd-github-pr` as the worked example, then the other item skills. The rest move a scope at a time, so each step can be reviewed. ## Context @@ -41,8 +41,15 @@ claimed: - **The golden scenarios still come from the same reading of the skill.** An `allow` made from the prose and a scenario made from the prose can share a mistake. Running a model (ADR 0016) is what tests that. - **It checks effects, not their order or count.** One extra comment with an allowed marker passes. +## Notes from migrating the item skills + +- **The goldens did not cover everything the skill text permits.** Each `allow` list was written from the skill's own text and its references, then run against the goldens, so a label that no scenario applies (`bug`, `enhancement`) is still listed. A golden that applies something the text does not permit would now fail, and none did. +- **Some writes cannot be expressed as effects.** `triage-github-issue` updates a description and minimizes comments. Those are `unmodelled`, which no contract lists, so a run that does either is still rejected, as before. The sentence says so, and only the comments are checked. +- **The outcome comment has no fixed marker.** The skill text does not name one and the goldens use both `agent:outcome` and none, so triage allows both. Pinning it is a skill change and not part of this one. +- **Removing a state label is listed only where the text does it.** Triage may remove `needs-triage` and its own lease, not `needs-info` (its `never` list says only a human does). An issue that already carries a state label is ineligible, so triage never swaps one. + ## Migration -1. The schema, the checker and `shepherd-github-pr` (this step). -2. The item skills: `triage-github-issue`, `implement-github-issue`, `review-github-pr`. +1. The schema, the checker and `shepherd-github-pr`. Done. +2. The other item skills: `triage-github-issue`, `implement-github-issue`, `review-github-pr`. Done. 3. The group, sweep and file skills: the `-issues` and `-prs` groups, `reconcile-github-issues`, `file-github-issues` and `explore-github-repo`. diff --git a/skills/pipeline/implement-github-issue/SKILL.md b/skills/pipeline/implement-github-issue/SKILL.md index 24d43af..bd9f7ba 100644 --- a/skills/pipeline/implement-github-issue/SKILL.md +++ b/skills/pipeline/implement-github-issue/SKILL.md @@ -52,19 +52,44 @@ effects: reads: [the Agent Brief or description, comments, every image, linked ADRs, relevant code, "prs_for_issue", remote branches, the repository's own guidance, item labels] writes: claimed: - - "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." + - text: "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." + allow: + - { op: label_add, label: agent:in-progress } + - { op: label_remove, label: agent:in-progress } + - { op: comment, marker: agent:lease } + - { op: comment_edit, marker: agent:lease } + - text: "The issue's state label (ready-for-agent to agent:pr-open, needs-info or ready-for-human)." + allow: + - { op: label_remove, label: ready-for-agent } + - { op: label_remove, label: agent:pr-open } + - { op: label_add, label: agent:pr-open } + - { op: label_add, label: needs-info } + - { op: label_add, label: ready-for-human } + - text: "A Triage Notes, closed-PR or `Implemented in` comment, and closing the issue when its PR already merged." + allow: + - { op: comment, marker: null } + - { op: close } + - text: "One comment ending in the `agent:split` marker when the issue is split." + allow: { op: comment, marker: agent:split } + - text: "An agent:failure marker on failure. On escalation, the escalate comment and agent:stuck." + allow: + - { op: comment, marker: agent:failure } + - { op: comment, marker: agent:escalate } + - { op: label_add, label: agent:stuck } repository: - - "A feature or bugfix branch pushed to origin, with commits." - - "One PR against pr_base_branch, labelled agent:needs-work." + - text: "A feature or bugfix branch pushed to origin, with commits." + allow: { op: push } + - text: "One PR against pr_base_branch, labelled agent:needs-work." + allow: { op: create_pr } 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." + - text: "agent:needs-work on an existing OPEN agent PR for the issue." + allow: { op: label_add, label: agent:needs-work } + - text: "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." + allow: { op: create_issue } shared: - - "One run-log entry (when write_log is true)." + - text: "One run-log entry (when write_log is true)." + allow: { op: comment, marker: null } + external: ["build_cmd, when set, and the repository's test commands."] never: - "Wait for checks, fix CI, or post the reviewer handoff. shepherd-github-pr does that." diff --git a/skills/pipeline/review-github-pr/SKILL.md b/skills/pipeline/review-github-pr/SKILL.md index 84c044e..cc501c5 100644 --- a/skills/pipeline/review-github-pr/SKILL.md +++ b/skills/pipeline/review-github-pr/SKILL.md @@ -48,17 +48,47 @@ effects: reads: ["the PR title, body and comments, and its diff at the pinned SHA", linked issues and their labels, check state, "protected_files", "codeowners_for", markers, the repository's own guidance] writes: claimed: - - "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." - - "Deferred, review, merged and human-merge comments, and the label swaps they come with." - - "An agent:failure marker on failure. On escalation, the escalate comment and agent:stuck." + - text: "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." + allow: + - { op: label_add, label: agent:in-progress } + - { op: label_remove, label: agent:in-progress } + - { op: comment, marker: agent:lease } + - { op: comment_edit, marker: agent:lease } + - text: "Deferred, review, merged and human-merge comments, and the label swaps they come with." + allow: + - { op: comment, marker: agent:deferred } + - { op: comment, marker: agent:handoff } + - { op: comment, marker: agent:human-merge } + - { op: label_remove, label: agent:ready-for-review } + - { op: label_remove, label: agent:needs-work } + - { op: label_add, label: agent:needs-work } + - { op: label_add, label: agent:human-merge } + - text: "An agent:failure marker on failure. On escalation, the escalate comment and agent:stuck." + allow: + - { op: comment, marker: agent:failure } + - { op: comment, marker: agent:escalate } + - { op: label_add, label: agent:stuck } repository: - - "The merge of the PR into its base (merge_method) and deletion of its head branch." - - "A small fix pushed to the PR branch when findings are sent back." + - text: "The merge of the PR into its base (merge_method) and deletion of its head branch." + allow: + - { op: merge } + - { op: delete_branch } + - text: "A small fix pushed to the PR branch when findings are sent back." + allow: { op: push } other: - - "Linked issues: removal of agent:pr-open, closing with a comment, or ready-for-human for a keep-open issue." - - "A review request to human_reviewer or the CODEOWNERS when a PR is escalated or handed to a person." + - text: "Linked issues: removal of agent:pr-open, closing with a comment, or ready-for-human for a keep-open issue." + allow: + - { op: label_remove, label: agent:pr-open } + - { op: label_remove, label: ready-for-agent } + - { op: label_add, label: ready-for-human } + - { op: comment, marker: null } + - { op: close } + - text: "A review request to human_reviewer or the CODEOWNERS when a PR is escalated or handed to a person." + allow: { op: review_request } shared: - - "One run-log entry (when write_log is true)." + - text: "One run-log entry (when write_log is true)." + allow: { op: comment, marker: null } + external: ["The repository's test commands, when a fix is pushed."] never: - "Merge in the same run in which you pushed a commit. A fresh review run must review the new head." diff --git a/skills/pipeline/triage-github-issue/SKILL.md b/skills/pipeline/triage-github-issue/SKILL.md index a9fff6a..5537cb3 100644 --- a/skills/pipeline/triage-github-issue/SKILL.md +++ b/skills/pipeline/triage-github-issue/SKILL.md @@ -44,15 +44,37 @@ effects: reads: [the issue description and every comment, images, linked ADRs, repository files, other issues (dedupe), the repository's own guidance, item labels] writes: claimed: - - "The agent:triage 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." - - "State and category labels, and removal of needs-triage." - - "The outcome comment, a link comment, description updates, and minimized resolved comments." - - "Closing the issue as a duplicate, when references/dedupe.md says so." - - "An agent:failure marker on failure. On escalation, the escalate comment and agent:stuck." + - text: "The agent:triage 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." + allow: + - { op: label_add, label: agent:triage } + - { op: label_remove, label: agent:triage } + - { op: comment, marker: agent:lease } + - { op: comment_edit, marker: agent:lease } + - text: "State and category labels, and removal of needs-triage." + allow: + - { op: label_add, label: needs-info } + - { op: label_add, label: ready-for-agent } + - { op: label_add, label: ready-for-human } + - { op: label_add, label: bug } + - { op: label_add, label: enhancement } + - { op: label_remove, label: needs-triage } + - text: "The outcome comment, a link comment, description updates, and minimized resolved comments. The description updates and minimizing are not modelled, so only the comments are checked." + allow: + - { op: comment, marker: agent:outcome } + - { op: comment, marker: null } + - text: "Closing the issue as a duplicate, when references/dedupe.md says so." + allow: { op: close } + - text: "An agent:failure marker on failure. On escalation, the escalate comment and agent:stuck." + allow: + - { op: comment, marker: agent:failure } + - { op: comment, marker: agent:escalate } + - { op: label_add, label: agent:stuck } other: - - "The comments references/dedupe.md allows on other issues." + - text: "The comments references/dedupe.md allows on other issues." + allow: { op: comment, marker: null } shared: - - "One run-log entry (when write_log is true)." + - text: "One run-log entry (when write_log is true)." + allow: { op: comment, marker: null } never: - "Write or change code, or touch PRs." - "Commit, or edit GLOSSARY.md, ADRs or .out-of-scope/." diff --git a/tests/scenarios.test.mjs b/tests/scenarios.test.mjs index 65b27ce..6243441 100644 --- a/tests/scenarios.test.mjs +++ b/tests/scenarios.test.mjs @@ -876,3 +876,28 @@ test("a comment edited in place has the marker of the comment it edits", () => { assert.equal(allowedBy(edit(1), [{ op: "comment_edit", marker: "agent:lease" }], ctx), null, "the plain comment is not the lease"); assert.equal(allowedBy(edit(5), [{ op: "comment_edit", marker: null }], ctx), null, "a comment that is not there matches nothing, not even `null`"); }); + +// ---- the item skills are migrated to `allow`: what each skill's text forbids is now caught --------------------------------- +// Each row is a golden scenario plus one effect the skill's own text forbids. A scope-only check accepted every one. +const FORBIDDEN = [ + ["triage-github-issue/open-questions-mean-needs-info.yaml", { op: "label_add", target: "issue#12", label: "wontfix" }, "triage never applies wontfix"], + ["triage-github-issue/open-questions-mean-needs-info.yaml", { op: "label_add", target: "issue#12", label: "agent:pr-open" }, "triage does not open a PR state"], + ["triage-github-issue/open-questions-mean-needs-info.yaml", { op: "comment", target: "issue#12", marker: "agent:handoff" }, "a PR handoff marker on an issue"], + ["triage-github-issue/duplicate-is-closed.yaml", { op: "close", target: "issue#7" }, "only the claimed issue is ever closed"], + ["triage-github-issue/duplicate-is-closed.yaml", { op: "label_add", target: "issue#7", label: "ready-for-agent" }, "no state label on another issue"], + ["implement-github-issue/existing-open-pr-is-not-duplicated.yaml", { op: "label_add", target: "pr#40", label: "agent:ready-for-review" }, "only agent:needs-work goes on an existing PR"], + ["implement-github-issue/existing-open-pr-is-not-duplicated.yaml", { op: "merge", target: "pr#40" }, "implementing never merges"], + ["implement-github-issue/existing-open-pr-is-not-duplicated.yaml", { op: "label_add", target: "issue#12", label: "wontfix" }, "a label the skill never sets"], + ["review-github-pr/findings-are-sent-back.yaml", { op: "label_add", target: "pr#50", label: "agent:ready-for-review" }, "a sent-back PR does not return to the lane"], + ["review-github-pr/findings-are-sent-back.yaml", { op: "comment", target: "pr#50" }, "an unmarked comment on the claimed PR"], + ["review-github-pr/merge-closes-the-linked-issue.yaml", { op: "label_add", target: "issue#12", label: "ready-for-agent" }, "a linked issue is not sent back to the agents"], + ["review-github-pr/merge-closes-the-linked-issue.yaml", { op: "comment", target: "issue#12", marker: "agent:handoff" }, "a marker on a linked issue"], +]; +for (const [file, extra, why] of FORBIDDEN) { + test(`${file.split("/")[0]}: ${why}`, () => { + const sc = find(file); + assert.deepEqual(problems(contractOf(sc.skill), sc), [], "the golden scenario is clean first"); + sc.expect.effects.push(extra); + assert.ok(problems(contractOf(sc.skill), sc).some((p) => notAllowed.test(p)), JSON.stringify(extra)); + }); +}