feat(eval): structured write allowances for the group, sweep and file skills (#67, step 3 of 3)

.changeset/structured-write-allowances-groups.md

@@ -0,0 +1,5 @@
+---
+"github-agent-workflow": patch
+---
+
+The remaining contracts (the four group skills, `reconcile-github-issues`, `file-github-issues`, `explore-github-repo` and `setup-github-agent-workflow`) now list what they may write as `allow` entries, which completes ADR 0017: every contract that writes is checked by what it writes and not only by where. An `allow` for `create_issue` and `create_pr` now has a required `labels` list, so "new issues, with no labels" and "a PR labelled agent:needs-work" are checked, and `implement-github-issue` is tightened to match. The wording of the contracts is unchanged.

docs/adr/0017-structured-write-allowances.md

@@ -2,7 +2,7 @@
 
 ## Status
 
-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.
+Accepted and built (#67), in three steps so each could be reviewed: the schema, the checker and `shepherd-github-pr` as the worked example, then the other item skills, then the group, sweep, file and setup skills. Every contract that writes now lists its writes as `allow` entries.
 
 ## Context
 
@@ -22,6 +22,7 @@ claimed:
 ```
 
 - `op` is required and is one of the effect ops the checker models, except `unmodelled`.
+- `labels` is required for `create_issue` and `create_pr`: the labels the new item may carry, each a name or a glob. Every label it is made with must match one, and `[]` means none.
 - `label` is required for `label_add` and `label_remove`, and is a name or a glob. `*` is the only wildcard. Use `*` alone to allow any label, which is then a visible choice and not a gap.
 - `marker` is required for `comment` and `comment_edit`: a name, a glob, or `null` for a comment with no marker (a run-log entry, a one-line comment on a linked issue). A `comment_edit` carries only an index, so its marker is the marker of the comment it edits in the world the run started from. A comment that cannot be found matches nothing.
 
@@ -37,7 +38,8 @@ claimed:
 
 ## What it does not do
 
-- **`create_pr`, `create_issue`, `close` and `review_request` are matched by op alone.** What a new item is labelled with, or what a PR is closed for, is not constrained yet. The schema rejects `label` and `marker` on them, so they cannot be mistaken for checked.
+- **`close` and `review_request` are matched by op alone.** What an issue is closed for, or who a review is requested from, is not constrained. The schema rejects `label`, `marker` and `labels` on them, so they cannot be mistaken for checked.
+- **A created item's other fields are unconstrained.** Only its labels are matched. Its title, body, parent and the PR's base are not.
 - **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.
 
@@ -48,8 +50,14 @@ claimed:
 - **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.
 
+## Notes from migrating the group and file skills
+
+- **A group's `allow` is declarative for now.** Scenarios already forbid a group from doing item work, with an earlier and stricter rule, so its `shared` entry is exercised by the contract tests and by real runs through `eval`, not by the goldens (they have no effects).
+- **`reconcile-github-issues` may add any one of the four state labels and remove the others.** Its repair picks the target by a priority order that the checker does not model, so the list permits the whole mutex set and the repair sentence says so. It never lists `needs-triage`, a lease label to add, or a lease marker.
+- **`setup-github-agent-workflow` is migrated too**, though it was not in the plan: its two entries map onto `label_create`, `label_update` and `write_file`.
+
 ## Migration
 
 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`.
+3. The four group skills (one run-log entry each), `reconcile-github-issues`, `file-github-issues`, `explore-github-repo` and `setup-github-agent-workflow`. Done. This step added `labels`, so "new issues, with no labels" is checked, and `implement-github-issue`'s PR (`agent:needs-work` only) and sub-issues (none) are now pinned too.

scripts/contracts.mjs

@@ -21,6 +21,7 @@ const ITEM_OPS = ["label_add", "label_remove", "comment", "comment_edit", "close
 export const ALLOW_OPS = [...ITEM_OPS, "create_issue", ...REPOSITORY_OPS];
 const LABEL_OPS = ["label_add", "label_remove"];
 const MARKER_OPS = ["comment", "comment_edit"];
+const CREATE_OPS = ["create_issue", "create_pr"];
 // The ops that can land in each scope: scopeOf puts REPOSITORY_OPS in `repository`, a comment on the run log in `shared`,
 // and a `create_issue` always in `other`.
 export const SCOPE_OPS = {
@@ -51,7 +52,7 @@ export function allowsOf(list) {
 function checkAllow(a, scope) {
   if (!a || typeof a !== "object" || Array.isArray(a)) return ["an `allow` must be a mapping with an `op`"];
   const out = [];
-  for (const k of Object.keys(a)) if (!["op", "label", "marker"].includes(k)) out.push(`\`allow\` has unknown key \`${k}\``);
+  for (const k of Object.keys(a)) if (!["op", "label", "marker", "labels"].includes(k)) out.push(`\`allow\` has unknown key \`${k}\``);
   if (!ALLOW_OPS.includes(a.op)) return [...out, `\`allow.op\` must be one of ${ALLOW_OPS.join(", ")}`];
   if (SCOPE_OPS[scope] && !SCOPE_OPS[scope].includes(a.op)) out.push(`\`${a.op}\` can never land in \`${scope}\`, so this entry would allow nothing`);
   if (LABEL_OPS.includes(a.op)) {
@@ -60,6 +61,9 @@ function checkAllow(a, scope) {
   if (MARKER_OPS.includes(a.op)) {
     if (!("marker" in a) || (a.marker !== null && !isText(a.marker))) out.push(`\`${a.op}\` needs a \`marker\`: a name, a glob such as \`agent:*\`, or \`null\` for a comment with no marker`);
   } else if ("marker" in a) out.push(`\`marker\` applies only to ${MARKER_OPS.join(" and ")}, not \`${a.op}\``);
+  if (CREATE_OPS.includes(a.op)) {
+    if (!Array.isArray(a.labels) || !a.labels.every(isText)) out.push(`\`${a.op}\` needs \`labels\`: the labels the new item may carry, each a name or a glob. \`[]\` means none`);
+  } else if (a.labels !== undefined) out.push(`\`labels\` applies only to ${CREATE_OPS.join(" and ")}, not \`${a.op}\``);
   return out;
 }
 

scripts/scenario.mjs

@@ -232,6 +232,7 @@ export function allowedBy(effect, allows, ctx) {
   for (const a of allows) {
     if (a.op !== effect.op) continue;
     if (a.label !== undefined && !(typeof effect.label === "string" && globMatches(a.label, effect.label))) continue;
+    if (a.labels !== undefined && !(effect.labels ?? []).every((l) => a.labels.some((g) => globMatches(g, l)))) continue;
     if ("marker" in a) {
       const m = markerOf(effect, ctx);
       if (a.marker === null ? m !== null : !(typeof m === "string" && globMatches(a.marker, m))) continue;
@@ -241,7 +242,7 @@ export function allowedBy(effect, allows, ctx) {
   return null;
 }
 
-const describe = (x) => [x.op, x.label !== undefined && `\`${x.label}\``, "marker" in x && (x.marker === null ? "(no marker)" : `\`${x.marker}\``)].filter(Boolean).join(" ");
+const describe = (x) => [x.op, x.label !== undefined && `\`${x.label}\``, x.labels !== undefined && (x.labels.length ? `with only ${x.labels.map((l) => `\`${l}\``).join(", ")}` : "with no labels"), "marker" in x && (x.marker === null ? "(no marker)" : `\`${x.marker}\``)].filter(Boolean).join(" ");
 
 /**
  * Why the contract does not allow an effect, or null when it does. First the scope must be one `effects.writes` lists.

skills/meta/setup-github-agent-workflow/SKILL.md

@@ -32,8 +32,12 @@ effects:
   reads: ["`gh auth status`, `gh repo view` and `gh label list`", "the tools and the skills available to the runner", "the repository's agent instructions file"]
   writes:
     repository:
-      - "The labels in `references/labels.md`: created when missing, and the `agent:` ones reset to their description."
-      - "The `## GitHub workflow` section of the agent instructions file."
+      - text: "The labels in `references/labels.md`: created when missing, and the `agent:` ones reset to their description."
+        allow:
+          - { op: label_create }
+          - { op: label_update }
+      - text: "The `## GitHub workflow` section of the agent instructions file."
+        allow: { op: write_file }
   external: ["The probes for `gh`, `jq`, `node` and the external skills."]
 never:
   - "Write a settings file. A repository's values belong in the prompt or the agent instructions."

skills/pipeline/explore-github-repo/SKILL.md

@@ -32,9 +32,13 @@ effects:
   reads: [the live app (when app_url is set and the browser stack is present), the code, open issues, recent Exploring log entries]
   writes:
     other:
-      - "New unlabelled issues, and evidence comments on duplicates, through file-github-issues (unless dry_run)."
+      - text: "New unlabelled issues, and evidence comments on duplicates, through file-github-issues (unless dry_run)."
+        allow:
+          - { op: create_issue, labels: [] }
+          - { op: comment, marker: null }
     shared:
-      - "One run-log entry."
+      - text: "One run-log entry."
+        allow: { op: comment, marker: null }
   external: ["A browser session against app_url, when in scope."]
 never:
   - "Apply, remove or change any label."

skills/pipeline/file-github-issues/SKILL.md

@@ -50,8 +50,10 @@ effects:
   reads: [open issues, recently closed issues (dedupe_closed_days), ".out-of-scope/", the repository's own guidance and issue templates]
   writes:
     other:
-      - "New issues, with no labels (unless dry_run)."
-      - "One evidence comment on an open issue that reports the same problem (unless dry_run)."
+      - text: "New issues, with no labels (unless dry_run)."
+        allow: { op: create_issue, labels: [] }
+      - text: "One evidence comment on an open issue that reports the same problem (unless dry_run)."
+        allow: { op: comment, marker: null }
 never:
   - "Apply, remove or change any label."
   - "Comment on, close or edit an issue other than the evidence comment in the dedupe step."

skills/pipeline/implement-github-issue/SKILL.md

@@ -80,12 +80,12 @@ effects:
       - 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 }
+        allow: { op: create_pr, labels: [agent:needs-work] }
     other:
       - 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 }
+        allow: { op: create_issue, labels: [] }
     shared:
       - text: "One run-log entry (when write_log is true)."
         allow: { op: comment, marker: null }

skills/pipeline/implement-github-issues/SKILL.md

@@ -31,7 +31,8 @@ effects:
   reads: [open issues labelled ready-for-agent]
   writes:
     shared:
-      - "One run-log entry."
+      - text: "One run-log entry."
+        allow: { op: comment, marker: null }
 never:
   - "Implement an issue itself, or claim one. Only implement-github-issue claims."
   - "Wait for checks or fix CI. shepherd-github-prs does that."

skills/pipeline/reconcile-github-issues/SKILL.md

@@ -46,14 +46,47 @@ effects:
   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)."
+      - text: "Closes an issue whose PR merged, with a comment, and removes agent:pr-open and ready-for-agent from it (unless dry_run)."
+        allow:
+          - { op: close }
+          - { op: comment, marker: null }
+          - { op: label_remove, label: agent:pr-open }
+          - { op: label_remove, label: ready-for-agent }
+      - text: "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)."
+        allow:
+          - { op: close }
+          - { op: comment, marker: null }
+          - { op: label_remove, label: ready-for-agent }
+          - { op: label_add, label: ready-for-human }
+      - text: "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)."
+        allow:
+          - { op: comment, marker: null }
+          - { op: label_remove, label: ready-for-agent }
+          - { op: label_add, label: ready-for-human }
+      - text: "Swaps agent:pr-open or ready-for-agent for ready-for-human, with a comment, when every PR was closed unmerged (unless dry_run)."
+        allow:
+          - { op: comment, marker: null }
+          - { op: label_remove, label: agent:pr-open }
+          - { op: label_remove, label: ready-for-agent }
+          - { op: label_add, label: ready-for-human }
+      - text: "Repairs conflicting state labels in one edit per issue, with a comment when it falls back to ready-for-human (unless dry_run). The target is any one of the four state labels, and every other one is removed."
+        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: agent:pr-open }
+          - { op: label_remove, label: needs-info }
+          - { op: label_remove, label: ready-for-agent }
+          - { op: label_remove, label: ready-for-human }
+          - { op: label_remove, label: agent:pr-open }
+          - { op: comment, marker: null }
+      - text: "Removes a stale agent:triage or agent:in-progress label (unless dry_run)."
+        allow:
+          - { op: label_remove, label: agent:triage }
+          - { op: label_remove, label: agent:in-progress }
     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:
   - "Triage, implement, review or merge anything, or change code."
   - "Claim an issue, or repair the labels of one that has a live lease."

skills/pipeline/review-github-prs/SKILL.md

@@ -31,7 +31,8 @@ effects:
   reads: [open non-draft PRs labelled agent:ready-for-review]
   writes:
     shared:
-      - "One run-log entry."
+      - text: "One run-log entry."
+        allow: { op: comment, marker: null }
 never:
   - "Review or merge a PR itself, or claim one. Only review-github-pr claims."
 calls: []

skills/pipeline/shepherd-github-prs/SKILL.md

@@ -32,7 +32,8 @@ effects:
   reads: [open PRs labelled agent:needs-work, their check state]
   writes:
     shared:
-      - "One run-log entry."
+      - text: "One run-log entry."
+        allow: { op: comment, marker: null }
 never:
   - "Shepherd a PR itself, or claim one. Only shepherd-github-pr claims."
   - "Wait for CI to finish."

skills/pipeline/triage-github-issues/SKILL.md

@@ -32,7 +32,8 @@ effects:
   reads: [open issues with their labels]
   writes:
     shared:
-      - "One run-log entry."
+      - text: "One run-log entry."
+        allow: { op: comment, marker: null }
 never:
   - "Triage an issue itself, or change code or PRs."
   - "Claim an issue. Only triage-github-issue claims."

tests/contracts.test.mjs

@@ -362,3 +362,19 @@ test("entryParts and allowsOf read both forms", () => {
   assert.deepEqual(allowsOf(["prose", { text: "T", allow: [{ op: "push" }, { op: "merge" }] }]), [{ op: "push" }, { op: "merge" }]);
   assert.deepEqual(allowsOf(["only", "prose"]), []);
 });
+
+// ---- `labels` on create_issue and create_pr ----------------------------------------------------------------------------
+const otherWith = (block) => ITEM.replace('claimed: ["A label."]', `claimed: ["A label."]\n    other:\n${block}`);
+
+test("a create op needs `labels`: a list of names or globs, and `[]` means none", () => {
+  assert.deepEqual(run([item(otherWith(`      - text: "New issues with no labels."\n        allow: { op: create_issue, labels: [] }`))]), []);
+  assert.deepEqual(run([item(otherWith(`      - text: "A labelled issue."\n        allow: { op: create_issue, labels: [bug, "agent:*"] }`))]), []);
+  assert.ok(run([item(otherWith(`      - text: "x"\n        allow: { op: create_issue }`))]).some((e) => /`create_issue` needs `labels`/.test(e)));
+  assert.ok(run([item(otherWith(`      - text: "x"\n        allow: { op: create_issue, labels: bug }`))]).some((e) => /`create_issue` needs `labels`/.test(e)));
+  assert.ok(run([item(otherWith(`      - text: "x"\n        allow: { op: create_issue, labels: [""] }`))]).some((e) => /`create_issue` needs `labels`/.test(e)));
+});
+
+test("`labels` on an op that creates nothing is rejected", () => {
+  assert.ok(allowErrors(`      - text: "x"\n        allow: { op: label_add, label: a, labels: [a] }`).some((e) => /`labels` applies only to create_issue and create_pr/.test(e)));
+  assert.ok(allowErrors(`      - text: "x"\n        allow: { op: comment, marker: null, labels: [] }`).some((e) => /`labels` applies only to/.test(e)));
+});