Skip to content

source-control: D6's verify-commit-pushed gate reads only the tip — a positional index on a list, violating two of the three rules the same skill publishes as absolute #2244

Description

@kyle-sexton

Found by two independent post-merge verifiers on #2163 (merged as 1b338d74), then reproduced directly against live data. This is the third instance of the defect class #2163 exists to eliminate, and it sits inside the files #2163 edited.

The site

plugins/source-control/reference/review-discipline.md:233-234, checklist step D6 — verify commit pushed:

gh api "repos/{owner}/{repo}/commits?sha=<branch>&per_page=1" --jq '.[0].sha'

Why this is a defect and not a nit

plugins/source-control/skills/pull-request/reference/readiness.md:53 publishes "Three rules, all absolute." This one command breaks two of them:

  • Rule 1 — "Paginate every list read." commits is a paginated list endpoint. No --paginate, no per_page=100.
  • Rule 2 — "Never pair a positional index with a list." .[0] is a positional index on a list. Rule 2 is stated as absolute, and this is the only surviving positional index on a list read in a runnable command in the skill.

It sits six lines above D7, where the same file says "never on .[-1]" — the line #2163 rewrote for exactly this class.

The sharper problem: it answers a different question than it asks

D6's stated purpose is "confirm the fix commit SHA on the remote" — a presence question. .[0].sha reads only the branch tip, so it answers "what is the tip?" Those coincide only when the fix commit is the most recent commit on the branch.

Apply rule 2's own newly-added clause — "ask what else could satisfy this selector". The selector is purely positional, so any commit at the tip satisfies it.

Reproduced on PR #2171's own branch, where the fix commit 1c17be55 was later followed by af771ff0:

$ gh api "repos/{o}/{r}/commits?sha=fix/babysit-merge-ruleset-context-union&per_page=1" --jq '.[0].sha'
af771ff082a71e48b6e1ffe8ed683bb538c5b808        <- NOT the fix commit

$ gh api --paginate "repos/{o}/{r}/commits?sha=fix/babysit-merge-ruleset-context-union&per_page=100" \
    --jq '.[] | select(.sha | startswith("1c17be55")) | .sha'
1c17be55645a7d63473d638868582c64d649f7ae        <- present, correctly found

An agent following D6 verbatim after pushing a fix and then pushing anything else — a follow-up commit, a rebase, a CHANGELOG renumber, a sibling lane's push — concludes the fix is not on the remote when it is. That is a false negative on a control gate, the mirror of the false positive D7 was just fixed for.

Suggested fix

The single-resource form is index-free, identity-bound, and verified to discriminate:

$ gh api "repos/{o}/{r}/commits/be3d73ef..."  -> be3d73ef...            (present)
$ gh api "repos/{o}/{r}/commits/0000...0000" -> HTTP 422 "No commit found for SHA"  (absent)

Either that, or the paginated property-based select shown above. Both state their own intent and cannot be satisfied by the wrong record.

Scope

  • plugins/source-control/reference/review-discipline.md:233-234
  • plugins/source-control/skills/pull-request/SKILL.md — the verifiers reported the same form at ~line 181; verify before editing, as a grep of current main (c1b4c629) did not reproduce it there. It may have moved or already been removed.

Cross-reference: #2238 and #2239 cover the narrower per_page=100 omission in review/agents/ci-log-auditor.md only.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions