Skip to content

fix(skills): resolve the ten contract gaps (ADR 0011) - #51

Merged
glennfaison merged 7 commits into
mainfrom
fix/contract-gaps
Nov 2, 2026
Merged

glennfaison merged 7 commits into
mainfrom
fix/contract-gaps

Conversation

@glennfaison

Copy link
Copy Markdown
Owner

Stacked on #50 (base is feat/skill-contracts). Resolves the ten gaps listed in ADR 0010. One commit per change, so each can be reviewed or reverted on its own. The decisions are in ADR 0011.

Behaviour changes

# Commit What changes
1 write_log and dispatch forwarding New write_log input (default true) on the four item skills. A group skill dispatches with the URL, write_log=false and every group input the item also lists in params. Also fixes something I had not reported: a sub-agent used to get only the URL, so it lost runner, unattended and trusted_authors
2, 9 noop and shepherd pre-claim skipped always means nothing was claimed. New noop for claimed-then-released and not ready for review step 4. Shepherd reads the lane and merge state before it claims. A DIRTY branch is eligible whatever its checks say
3, 4 limit and explore_batch file-github-issues gets a real limit. explore_batch is always 5
5, 10 Pure eligibility and lease_held Eligibility no longer calls clear_stale_lease. New lease_held makes claim and release do nothing, for a nested call
6, 7 Shared rule and sweeper New skill reconcile-github-issues owns what triage-github-issues did to unclaimed issues, and also clears stale leases. New references/issue-after-pr.md replaces three copies of the close-after-merge rule
8 Triage order A prior trusted outcome is reconciled at step 4, before the format check and the unattended-triage call

Checks

  • scripts/contracts.mjs learned: a sweep kind, that a dispatched item which logs must accept write_log, and that a requires line may not name a writing helper (claim, release, clear_stale_lease, ...). 20 unit tests.
  • tests/lib.test.sh covers the new defaults and that claim/release do nothing under lease_held=true. 154 pass (with jq).
  • validate, lint, sync:check, check-plugin-version pass.

What this does not prove

Nothing here ran against real GitHub. The shell tests use the repo's gh mock, and the skill changes are text that a model follows. In particular:

  • Gap 9 is inferred, not observed. I believe a conflicted PR with no checks was skipped, and that a PR waiting on the lane got a lease comment per run, from reading the steps. A scenario fixture should confirm both.
  • reconcile-github-issues is new. It moves existing steps and adds stale-lease clearing, but it has not been run.
  • lease_held is only exercised at the helper level. No skill passes it yet.

For review

  • Is reconcile-github-issues right as a pipeline skill with kind: sweep, or should it sit elsewhere?
  • Are noop and not ready the right names?
  • In triage-github-issue, is "a standing prior outcome skips the format check as well as the decision" what you want? Skipping only the decision is the more conservative reading.

@glennfaison
glennfaison changed the base branch from feat/skill-contracts to main November 2, 2026 11:34
…spatched items

An item skill 'run on its own' wrote a run-log entry, but a dispatched sub-agent
was given only the item URL, so it could not tell it was dispatched and the item
and the group could both log. It also lost runner, unattended, trusted_authors
and every other input of the group run.

Add write_log (default true) to the four item skills with their own log entry.
Dispatching now passes the URL, write_log=false and every group input the item
also lists in params. The validator checks a dispatched item can take write_log.
…ore claiming

skipped meant 'nothing happened' before a claim but was also returned after one
(merge lane held), after the claim had posted a lease comment, so a PR waiting
on the lane collected a comment per run. Add #N noop: <reason> for a claimed
item that is released unchanged, and make eligibility read the lane and merge
state first so a PR that can only wait is skipped without a claim.

A conflicted PR often runs no checks, so shepherd-github-pr skipped it as
no-checks-yet and never reached its conflict handling. A DIRTY branch is now
eligible whatever the checks say.

review-github-pr step 4 sent a PR back to needs-work with no result line. It
now returns #N not ready: <reason>.
…atch deterministic

file-github-issues referred to a limit the caller could give, but had no such
input, so explore could not enforce explore_batch. Add limit: once that many are
filed the rest return deferred: over limit unchecked. explore-github-repo passes
limit=explore_batch in one call.

explore_batch defaulted to 5, or 3 when the backlog was 'large' (undefined). It
is now always 5. Dedupe already prefers a comment to a near-duplicate filing.
… lease

Eligibility called clear_stale_lease, which removes a label, so a precondition
had a side effect. Checking a lease now only reads: a stale label does not count
as a lease (lease_live decides) and is replaced by the next claim.

lease_live ignores who holds the lease, so a skill that called an item skill on
an item it already held would see its own lease and skip, and a nested release
would drop the caller's lease. Add lease_held (default false): claim and release
do nothing, and the lease must be live. The validator rejects a precondition
that names a writing helper.
…skill for housekeeping

'Close the issue after a merge unless it is keep-open' was written out in three
places (triage housekeeping, implement step 2, review step 7.4), and the
abandoned-PR rule in two. They now share references/issue-after-pr.md, and each
caller states only what it adds.

triage-github-issues did housekeeping itself, which made a group skill change
issues it had not claimed. Move it to reconcile-github-issues (kind: sweep),
which lists exactly what it may change, takes dry_run, and is called by
triage-github-issues or run on its own. It also takes over clearing stale lease
labels, now that eligibility only reads. The label repair priority is read from
labels.md instead of being copied.
…ub-issue

The check for a prior trusted outcome (old step 7) ran after the costly
unattended-triage call (old step 5), so a pass that had already decided once
paid for the decision again. Move it to step 4, ahead of the format check and
the decision: a standing outcome skips both and goes to a reconciliation note.
Steps 8 to 10 keep their numbers.
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