Skip to content

Validate the structured Copilot review marker on live reviews #798

Open
Open
Validate the structured Copilot review marker on live reviews#798
Labels
agentsAgents instructionsscriptA defect in hub toolingskillsAgent skill

Description

@ptr727
ptr727
opened on Aug 17, 2026

Summary

Validate the structured Copilot review marker introduced by #793 against live GitHub reviews, then make pr_review.py enforce every field that proves useful.

Context

#793 instructs Copilot code review to end each review body with:

<!-- fleet-review: reviewed=N changed=N findings=N -->

scripts/pr_review.py reads reviewed and changed as a stable alternative to Copilot's changing coverage prose. It retains the legacy prose readers for reviews created before the marker ships. An absent coverage statement blocks with exit 45.

The implementation deliberately does not treat one local parser test as evidence that GitHub-hosted Copilot follows the instruction. It also parses the findings field as part of the marker shape but does not yet compare that value with inline or body-only findings.

Measurement

After #793 is available on a pull request head:

  1. Request Copilot review on at least one clean change and one change expected to produce a finding.
  2. Record whether the review body carries exactly one marker and whether GitHub preserves the HTML comment.
  3. Record whether reviewed and changed match the pull request file set.
  4. Record how findings relates to inline threads, review-body findings, and any suppressed block.
  5. Repeat after one fix push to verify the marker describes the current round rather than an earlier round.
  6. Capture any alternate output shape verbatim in fixtures before changing the parser.

Implementation Scope

  • Add corpus-derived fixtures for every observed marker shape.
  • Fail closed on duplicate, malformed, impossible, or conflicting markers.
  • Decide whether findings=N can be reconciled reliably with GitHub's review objects.
  • If it can, report the declared and observed counts in status and block on disagreement.
  • If it cannot, remove findings from the contract rather than retaining a field that appears enforced but is ignored.
  • Confirm that the no-suppression instruction prevents hidden findings. Keep the existing suppressed-block reader as backward-compatible defense.
  • Update the code-review skill, Copilot bootstrap, parser help, and tests together when the measured format differs.

Acceptance Criteria

  • Evidence links identify the live review rounds used for validation.
  • Tests reproduce the clean, finding, and post-fix rounds from sanitized review bodies.
  • The documented marker exactly matches the parser's accepted shape.
  • Every retained marker field affects a reported or blocking decision.
  • Missing or malformed structured coverage cannot exit successfully.
  • Legacy pre-marker coverage bodies remain readable until an explicit retirement decision is recorded.

Relationship to Existing Issues

#788 identified the unstated-coverage ambiguity. #793 introduces the structured marker and exit 45. This issue validates the external reviewer behavior that neither local unit tests nor CI can exercise.

Activity

  1. ptr727
    added
    skillsAgent skill
    agentsAgents instructions
    on Aug 17, 2026

  2. ptr727 commented on Aug 17, 2026

    @ptr727
    ptr727
    on Aug 17, 2026
    OwnerAuthor

    Live evidence from PR #799 review 4955381803:

    • Copilot reported full coverage using legacy prose: Copilot reviewed 12 out of 12 changed files ....
    • It did not emit the required <!-- fleet-review: reviewed=N changed=N findings=N --> marker.
    • It emitted one inline finding and one Suppressed comments (1) block despite the explicit instruction to publish every supported finding and never suppress one.
    • The review footer did not attribute the repository's .agents/skills/code-review skill and instead suggested adding .github/skills/code-review/SKILL.md.

    This is the clean baseline showing that the bootstrap text alone does not enforce the schema or no-suppression contract. Re-run the same PR shape after #797 supplies a Copilot-discoverable skill.

  3. ptr727 commented on Aug 17, 2026

    @ptr727
    ptr727
    on Aug 17, 2026
    OwnerAuthor

    Second-round evidence from PR #799 review 4955414705, after fixing both first-round findings:

    • The review covered the current f1c24d7 head and reported 12 of 12 files using legacy prose.
    • It generated no new comments and no suppressed finding on this head.
    • It again omitted the required fleet-review marker.
    • It carried no skill attribution or footer indicating that .agents/skills/code-review loaded.

    This removes findings as a possible reason the first marker was absent. The structured contract is not being followed on either a finding-bearing round or a clean round.

  4. ptr727 commented on Aug 18, 2026

    @ptr727
    ptr727
    on Aug 18, 2026
    OwnerAuthor

    Live evidence from PR #799 after .github/skills/ was generated on the head branch:

    • da7249f review: legacy full coverage, no structured marker, two suppressed findings.
    • c3738e5 review: legacy full coverage, no structured marker, one suppressed finding.
    • 04f4bf7 review: legacy full coverage, no structured marker, one suppressed finding.
    • ced266c review: legacy full coverage, no structured marker, no findings.

    All four rounds reviewed 53 of 53 changed files. None emitted the required fleet-review marker or visible skill attribution. Three rounds continued to use hidden suppressed-finding blocks despite the code-review skill and always-on instructions explicitly forbidding suppression.

    The findings demonstrate that Copilot inspected the generated skill files as changed content, but they do not prove that Copilot invoked the code-review skill. The missing attribution and marker leave skill activation unverified.

    Keep this issue open as a behavior and observability tracker. The feature is generally available, but the live implementation still does not reliably honor or attest to the requested schema and no-suppression contract.

  5. ptr727
    added a commit that references this issue on Aug 18, 2026

    Refactor Copilot Review Instructions and Skills (#799)

    05c101e

  6. ptr727 commented on Aug 21, 2026

    @ptr727
    ptr727
    on Aug 21, 2026
    OwnerAuthor

    Current audit against develop at 1832702: partial. Live rounds recorded here establish that Copilot omits the structured marker and can still suppress findings. The current code-review skill continues to require the marker, while pr_review.py retains legacy coverage parsing. No retained findings reconciliation or replacement contract resolves the observed external behavior.

  7. ptr727
    added
    scriptA defect in hub tooling
    on Sep 5, 2026

  8. ptr727 commented on Nov 1, 2026

    @ptr727
    ptr727
    on Nov 1, 2026
    OwnerAuthor

    Decision needed (decision triage, 2026-11-01)

    Question: Copilot has never emitted the fleet-review: reviewed=N changed=N findings=N HTML-comment marker. Should the marker contract in .github/copilot-instructions.md item 5 and fleet-code-review SKILL.md be retired, or kept and enforced, and should findings=N be dropped?

    Options, recommended first:

    1. Retire the ask and findings=, keep the reviewed/changed read (recommended): Remove item 5 and the fleet-code-review marker paragraph, drop findings from the regex, and keep the existing reviewed/changed reader as a harmless tested form. Coverage now comes from prose and the file table (coverage=table). Carried content, so downstream follows on resync. No work in flight.
    2. Retire everything, including the parser read: Also delete FLEET_REVIEW from pr_review_copilot.py and rewrite the many test fixtures that use the marker as shorthand for full coverage. Most cleanup, no live reading changes. No work in flight.
    3. Retire the Copilot ask, keep the marker for local reports: fleet-code-review keeps the marker so a local strict-review report carries findings=N, which Bind a Local Review Record to the Reviewer's Actual Report #3699 option 1 could derive its count from. Couples this to Bind a Local Review Record to the Reviewer's Actual Report #3699. No work in flight.
    4. Keep and enforce (fail closed when absent): Decline: Copilot has emitted the marker on no review on record, so this blocks every PR. No work in flight.

    Evidence: Six live Copilot rounds on #799 with no marker, and 11 Copilot reviews across the last six promotions into main (#3736, #3715, #3703, #3693, #3681, #3678) carry 0 markers. pr_review_copilot.py:68-70 parses three groups but lines 399-402 use only reviewed/changed, so findings affects no decision. The marker is still required at .github/copilot-instructions.md:27 and fleet-code-review SKILL.md:108-118. The issue's acceptance criteria: "Every retained marker field affects a reported or blocking decision".

    Blocks: none

  9. ptr727
    added
    decisionA question waiting on the maintainer, alone or beside whatever else the issue carries
    on Nov 1, 2026

  10. ptr727 commented on Nov 1, 2026

    @ptr727
    ptr727
    on Nov 1, 2026
    OwnerAuthor

    Maintainer answer (decision triage, 2026-11-01): Option 1: retire the Copilot marker ask (copilot-instructions.md item 5 and the fleet-code-review marker paragraph) and drop findings= from the regex, keeping the existing reviewed/changed reader. This is now work for the next burndown round.

  11. ptr727
    removed
    decisionA question waiting on the maintainer, alone or beside whatever else the issue carries
    on Nov 1, 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

    agentsAgents instructionsscriptA defect in hub toolingskillsAgent skill

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions