Skip to content

feat(eval): check what a skill writes, not only where (#67, step 1 of 3) - #97

Merged
glennfaison merged 1 commit into
mainfrom
feat/structured-effects-allow
Nov 13, 2026
Merged

glennfaison merged 1 commit into
mainfrom
feat/structured-effects-allow

Conversation

@glennfaison

Copy link
Copy Markdown
Owner

Step 1 of 3 for #67: the schema, the checker, and one skill as a worked example.

What changes

The scenario checker could only compare an effect to its scope (claimed, repository, other, shared). A skill that added wontfix to the PR it had claimed passed. An entry of effects.writes.<scope> can now be {text, allow}: the sentence stays as the explanation, and allow (one mapping or a list) says what it permits:

claimed:
  - text: "Attempt, branch-update and ready-handoff comments, and the swap of agent:needs-work for agent:ready-for-review."
    allow:
      - { op: comment, marker: agent:attempt }
      - { op: label_remove, label: agent:needs-work }
      - { op: label_add, label: agent:ready-for-review }
  • op is required. label (name or * glob) is required for label_add/label_remove. marker (name, glob, or null for no marker) is required for comment/comment_edit.
  • A comment_edit carries only an index, so its marker is that of the comment it edits in the world the run started from. A comment that can't be found matches nothing, not even null.
  • scopeOf is unchanged. The three places that checked a scope (pure skills, item skills, reruns) now share one function, forbiddenWrite.
  • A scope is all sentences or all allow. A scope without allow is checked by scope alone, as before, so nothing changes until a contract is migrated. The validator rejects a mix (a bare sentence beside an allow would look like permission it doesn't give) and an allow whose op can never land in its scope.
  • shepherd-github-pr is migrated in full (all four scopes). Its 9 golden scenarios pass unchanged.

Design and trade-offs: docs/adr/0017-structured-write-allowances.md. The protocol's Contracts section documents the format.

Behaviour guard

517 existing tests pass unchanged after the refactor alone (before migrating shepherd). 19 new tests: schema validation, glob/marker matching, comment_edit lookup, and the worked example against the real shepherd contract (wontfix label, unlisted marker, unmarked comment, a label on the linked issue, the rerun path, and prose scopes keeping their old behaviour). Disabling the allow check makes 5 of them fail.

npm test: 536/536. npm run validate and check-plugin-version pass.

Not in this PR

  • create_pr, create_issue, close and review_request are matched by op alone. The schema rejects label/marker on them so they can't be mistaken for checked.
  • Steps 2 (triage-github-issue, implement-github-issue, review-github-pr) and 3 (groups, sweep, file skills).

Refs #67, #72.

An entry of effects.writes.<scope> can carry an allow (op, and where it
applies label and marker). The scenario checker places an effect in its
scope as before, then, if the scope has allow entries, requires one to
match. A scope is all sentences or all allow, and the validator rejects
the mix. shepherd-github-pr is the worked example (ADR 0017); the other
skills are migrated in later steps.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants