feat(eval): structured write allowances for the item skills (#67, step 2 of 3)

.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.

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`.

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."

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."

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/."

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));
+  });
+}