Skip to content

feat(review): bundle review criteria and a verify step into the code-review skill #747

Closed
Closed
feat(review): bundle review criteria and a verify step into the code-review skill#747
Labels
area:metaOrganization-wide policy, templates, or meta-repo work.type:featureA request for new behavior or capability.

Description

@ss-o
ss-o
opened on Oct 7, 2026

Problem

The organization code-review skill (.github/skills/code-review/SKILL.md) is vendored into at least 15 consumer repositories at the approved revision 5593b7d. The checked copies match the approved content apart from YAML indentation. The skill is mostly a router, though, and the z-shell review criteria it routes to are not present where vendored reviews run.

  1. The criteria are absent from every consumer. The severity table (CRITICAL, IMPORTANT, SUGGESTION), dialect classification and deterministic checks live only in .github/instructions/quality/code-review.instructions.md in this repository. No consumer has a local copy. The skill says to follow that file by its github.com URL, or a local copy "when available". Copilot code review reads instructions and skills from the PR head branch; whether it follows the external URL is unverified. In a plugin, annex or module repository the review may therefore run without the z-shell priorities.
  2. The pin covers only part of the skill. The skill is pinned, but its links to the criteria, tool-integration guidance and the org-review and pull-request runbooks point at blob/main. The effective rules can change without a re-pin, while org-routing.py check still reports the consumer as current.
  3. There is no verification procedure. The criteria ask for verified failure scenarios, but the skill has no step that separates finding suspected defects from verifying them. It also does not ask the reviewer to pin base, head and merge-base, to read existing review threads (thread resolution is a merge gate under ADR-0013), to review only the delta on a re-review, or to recheck the head before reporting.
  4. Every PR review loads org process text. About a third of the body covers repository-health readiness and the ADR-0026 fallback election. Health readiness is already owned by runbooks/org-review.md and enforced by automation/agents/org-routing.py. The fallback election is meaningful to a local agent, not to the hosted reviewer.

No recorded review yet shows Copilot attributing a review to the skill, so its effect on review quality is unmeasured (runbooks/org-review.md already treats invocation as unverified).

Proposal

Scope is this repository. Re-pinning each consumer remains a separate authorized change per runbooks/org-review.md.

  1. Bundle the criteria in the skill. Extend automation/knowledge/knowledge-delivery.py so knowledge/domains/quality/code-review.md also renders .github/skills/code-review/references/criteria.md. The skill reference drops the Copilot frontmatter (applyTo, excludeAgent) and rewrites relative links that leave the skill directory to absolute https://github.com/z-shell/.github/blob/main/... URLs, so no bundled link breaks in a consumer. The core criteria are then pinned with the skill; the linked Zsh scripting standard and machine policy stay org-wide current by design. The knowledge source remains the only editable copy.
  2. Rewrite SKILL.md to about 600 words (currently 756): read references/criteria.md first and use its severity labels; add a verify step (record each suspected defect, check it against code, callers and tests, report confirmed and suspected separately, drop refuted ones); add the PR mechanics above; reduce health readiness to a pointer to runbooks/org-review.md; keep the fallback paragraph, scoped to agents that are not the hosted reviewer.
  3. Add references/criteria.md to the files list of code-review in knowledge/domains/agents/data/approved-skills.json. Advance the approved revision in a follow-up change once the skill commit exists.
  4. Update the install and update section of runbooks/org-review.md and the quality knowledge page for the multi-file skill and the second consumer.
  5. Measure before and after the re-pin, each step separately authorized: a draft PR in one plugin repository with three planted defects (Bash-only syntax in native Zsh, a feature above the declared floor, network activity in the load path), a requested Copilot review, and a record of which defects were found, which severity labels were used and whether the skill was attributed. Close the PR unmerged.
  6. Re-pin in the runbook pilot order: this repository, one standard plugin, one documentation repository, then the remaining consumers.

Acceptance criteria

  • references/criteria.md is generated from the knowledge source, carries the generated header, has no Copilot frontmatter and no relative link that leaves the skill directory.
  • knowledge-delivery.py --check fails when the reference drifts from its source; generator tests cover the second consumer and the link rewrite.
  • SKILL.md names the bundled criteria, contains the verify step and the PR mechanics, and points to runbooks/org-review.md for health readiness.
  • approved-skills.json lists both files and org-routing.py verify-approved passes at the advanced revision.
  • The affected runbook and knowledge pages describe the multi-file skill.
  • Baseline and post-change measurements are recorded on this issue (local Claude Code and Codex runs; Copilot measurement moved to Measure Copilot code review with the bundled code-review criteria #762).

Activity

  1. ss-o
    added
    type:featureA request for new behavior or capability.
    area:metaOrganization-wide policy, templates, or meta-repo work.
    on Oct 7, 2026
  2. github-project-automation
    added this to Z-shell Deliveryon Oct 7, 2026
  3. ss-o
    added 3 commits that reference this issue on Oct 7, 2026

    feat(review): bundle review criteria and a verify step into the code-…

    f90a75e

    fix(review): state the approval ancestry rule exactly and align the s…

    c7bba09

    fix(review): state the approval ancestry rule exactly

    653eaa9

  4. ss-o commented on Oct 7, 2026

    @ss-o
    ss-o
    on Oct 7, 2026
    MemberAuthor

    Progress: items 1 to 4 of the proposal are merged.

    Downstream impact: consumers call org-routing.yml at a full SHA, and the job reads the approved record at that SHA, so no consumer's check changes until it moves that pin. A consumer that moves its caller pin past b96ad52 must re-pin the skill to ede9ed9 in the same change, or its check fails.

    Next: the baseline measurement (item 5) in z-shell/zsh-eza while it still vendors 5593b7d, then its re-pin, then the post-change measurement. Results will be recorded here.

  5. ss-o commented on Oct 7, 2026

    @ss-o
    ss-o
    on Oct 7, 2026
    MemberAuthor

    Baseline measurement (item 5), first attempt: review request not registered; cause unconfirmed.

    • Repository: z-shell/zsh-eza, base main (ruleset has copilot_code_review with review_draft_pull_requests: false, review_on_push: false). Measurement PR feat(init): warn when the installed eza is older than the latest release zsh-eza#139, head c9fbb9e, vendored code-review skill at 5593b7d.
    • Requests: as a draft, POST /pulls/139/requested_reviewers with copilot-pull-request-reviewer[bot], then gh pr edit --add-reviewer @copilot; after ready_for_review at 2026-10-07T02:33:02Z, one more REST request at 02:33:23Z. Every response succeeded with an empty requested_reviewers, and the timeline shows no review_requested event for Copilot and no Copilot review.
    • Comparison: the most recent registered Copilot request found in the organization is add extendedglob option to toml parsing zsh-string-lib#5 (requested and reviewed 2025-08-23, not a draft).
    • Not checked: Copilot allowance and budget for the requester, which need the billing owner.

    The planted defects are not described here so a later run on the same PR is not primed. No further requests until the cause is known.

  6. ss-o commented on Oct 7, 2026

    @ss-o
    ss-o
    on Oct 7, 2026
    MemberAuthor

    Cause of the unregistered request above: Copilot usage limits were reached for the requester (confirmed by the maintainer). The baseline stays pending on z-shell/zsh-eza#139, which is back in draft. Its head vendors the skill at 5593b7d and will not be rebased, so the baseline can still run there once the limit resets, independently of zsh-eza's re-pin on main.

    Proceeding with the re-pin pilot in z-shell/zsh-eza meanwhile.

  7. ss-o commented on Oct 7, 2026

    @ss-o
    ss-o
    on Oct 7, 2026
    MemberAuthor

    Re-pin pilot: z-shell/zsh-eza#140 re-pins code-review to ede9ed9 and moves its Org Routing caller to a07ed57.

    • Confirmed: gh skill install ... --pin ede9ed9... --dir .github/skills copies references/criteria.md, and the installed file is the approved blob cb2520c5. One of the two open verification questions is settled.
    • Found: Prettier realigns the tables in the generated criteria.md. A consumer that formats Markdown (Trunk trunk-fmt-pre-commit, or a Trunk check on changed files) would commit different bytes, and org-routing.py check would then fail the resource blob comparison. SKILL.md is unaffected because its digest ignores frontmatter formatting.
    • Consumer fix, per maintainer decision: each re-pin adds this to the consumer's .trunk/trunk.yaml under lint, as zsh-eza#140 does:
    ignore:
  - linters: [prettier, markdownlint]
    paths:
      - .github/skills/**

    Every remaining consumer re-pin needs the same entry (merged into an existing ignore list where one exists) before references/criteria.md is committed.

  8. ss-o commented on Oct 7, 2026

    @ss-o
    ss-o
    on Oct 7, 2026
    MemberAuthor

    Re-pin pilot: z-shell/wiki#939 merged (0d2f24f), re-pinning code-review to ede9ed9 with the Org Routing caller at a07ed57. Post-merge runs on main, including Org Routing and Deploy Wiki, pass. The vendored SKILL.md and references/criteria.md are byte-identical to zsh-eza#140.

    Refinement of the formatter exclusion above: z-shell/wiki has ten locally authored skills under .github/skills/, so its exclusion is scoped to the vendored skill (.github/skills/code-review/**) to keep those formatted. Use .github/skills/** only where every skill under it is vendored, and otherwise scope it to the vendored skill directories.

    Review note: the wiki is class 1. Copilot is configured but its request did not register, so the review of record used the ADR-0035 sole-maintainer exception, elected by the maintainer, with a separate read-only review of the same head.

    Pilot order is complete (this repository, a standard plugin, a documentation repository). The remaining consumers and the Copilot measurement on z-shell/zsh-eza#139 are still open.

  9. ss-o
    added 2 commits that reference this issue on Oct 7, 2026

    ci: re-pin the code-review skill with bundled criteria (#16)

    2693669

    ci: re-pin the code-review skill with bundled criteria (#15)

    41b5f4c

  10. ss-o commented on Oct 7, 2026

    @ss-o
    ss-o
    on Oct 7, 2026
    MemberAuthor

    Re-pin wave: 11 more consumers re-pinned to code-review at ede9ed9, with the Org Routing caller at a07ed57 and the Trunk exclusion scoped to .github/skills/code-review/** in each.

    Remaining: z-shell/src and z-shell/zd, both class 1 with Copilot configured, which need the ADR-0035 route per PR; and the Copilot measurement on z-shell/zsh-eza#139 once the usage limit resets.

  11. ss-o commented on Oct 7, 2026

    @ss-o
    ss-o
    on Oct 7, 2026
    MemberAuthor

    Coordination note: the z-shell/src consumer re-pin of code-review (to ede9ed9, approved in #753) is being done together with the zi-install re-pin from #741 in one z-shell/src pull request, because bumping z-src's Org Routing caller past #753 requires both. Please skip z-shell/src in the #747 re-pins; the PR will link here.

  12. ss-o commented on Oct 7, 2026

    @ss-o
    ss-o
    on Oct 7, 2026
    MemberAuthor

    Correction to my previous note: z-shell/src#232 already re-pins code-review in z-shell/src, so keep it as planned. The #741 zi-install re-pin will follow on top of it once it merges, moving the caller from a07ed57 to 75e8552 (#759).

  13. ss-o commented on Oct 7, 2026

    @ss-o
    ss-o
    on Oct 7, 2026
    MemberAuthor

    Re-pin complete: z-shell/src#232 and z-shell/zd#134 are merged, so all 15 consumers vendor code-review at ede9ed9. Each has references/criteria.md at the approved blob cb2520c5, the Org Routing caller at a07ed57, and Org Routing passing on its default branch after the merge. z-shell/zi has it on next; zi's main keeps 5593b7d with its matching caller pin until the next promotion.

    Both class 1 PRs used the ADR-0035 sole-maintainer exception, elected by the maintainer per PR. In each, one Copilot request did not register (usage limit), and a separate read-only review of the same head found no actionable findings before the review of record was posted.

    Remaining on this issue: the baseline and post-change Copilot measurement (item 5, last acceptance criterion). The baseline PR z-shell/zsh-eza#139 is in draft and still vendors 5593b7d; it runs once the Copilot limit resets, followed by the same planted defects against the new skill.

  14. ss-o commented on Oct 7, 2026

    @ss-o
    ss-o
    on Oct 7, 2026
    MemberAuthor

    Measurement (item 5), run locally because Copilot code review could not register (usage limit). The Copilot-specific questions move to #762.

    Setup. The same planted commit was reviewed with the previous skill (5593b7d) and with the new one (ede9ed9), in otherwise identical z-shell/zsh-eza trees: the diff under review is byte-identical in both arms. Each arm ran Claude Code and Codex twice, headless and read-only, with one neutral prompt: review the last commit using the repository's code-review skill and report findings in a fixed JSON shape. The prompt does not mention planted defects. The three defects are Bash-only syntax in native Zsh, a feature above the declared 5.9.2 floor, and a network request in the load path.

    Isolated runs (decisive). Each arm is a standalone clone outside any workspace, with no network: Claude restricted to file tools, git and zsh -n, with no MCP servers, and Codex in its read-only sandbox with web search disabled. Transcripts show no web, MCP or URL access.

    Previous skill New skill
    Planted defects found 11 of 12 11 of 12
    Runs that read the z-shell criteria 0 of 4 4 of 4
    Runs using CRITICAL / IMPORTANT / SUGGESTION 0 of 4 (generic high/medium/low or P1/P2) 4 of 4
    Bash-only syntax labelled CRITICAL, per the criteria n/a Claude 2 of 2; Codex labelled it IMPORTANT
    Above-floor feature labelled IMPORTANT, per the criteria n/a 2 of the 3 runs that found it

    In both arms, Codex's second run missed the above-floor defect. Neither arm produced false positives. Both arms also reported real, unplanted issues: no tests, != used as a version ordering, unlocalized =~ match parameters, and an undocumented load-time side effect.

    In-workspace runs. Reviewers run inside the maintainer's meta-workspace reached the org criteria from a sibling checkout of this repository even with the previous skill, so both arms tied there: 4 of 4 used the z-shell labels, and all three defects were found in every run. The first isolated attempt was discarded after its logs showed Codex's built-in web search fetching the org instructions from raw.githubusercontent.com.

    Conclusion. With the criteria bundled, a reviewer that has only the consumer repository applies the z-shell severity scale, which it did not do before. Detection of these three defects did not change at this sample size: two runtimes, two runs each per arm. Whether Copilot reads the skill and its reference, and credits it, is tracked in #762.

  15. ss-o commented on Oct 7, 2026

    @ss-o
    ss-o
    on Oct 7, 2026
    MemberAuthor

    Closing: all acceptance criteria are met. Copilot code review measurement continues in #762.

  16. ss-o
    closed this as completedon Oct 7, 2026
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

    area:metaOrganization-wide policy, templates, or meta-repo work.type:featureA request for new behavior or capability.

    Type

    No type

    Projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions