docs(review): correct the trigger permission level (skip docs deferred to fullsend#6587)

[guyoron1]

Corrects the review Triggers documentation in docs/review.md to match
what is on fullsend main today:

• The trigger now reads triage-level repository permission or higher
  (triage, write, maintain, or admin), matching fullsend's own
  docs/agents/review.md and the triage gate in reusable-dispatch.yml.
• Removed the synchronize "(drafts excluded)" qualifier so the raw-event
  Triggers list is internally consistent.

Scope note (per @waynesun09's review): the "Automatic skips" section —
draft skip, fullsend-no-review, prose-only skip, label-clearing, and the
ADR 0096 link — describes behaviour that lives in
fullsend-ai/fullsend#6587 and is not on fullsend
main yet (ADR 0096 404s there). That content is removed from this PR and
will land in a follow-up pinned to #6587's merged head, so this PR documents
only live behaviour.

[github-actions]

Functional tests did not run

Functional tests run automatically for org/repo members and collaborators on pull requests.

For other contributors, a maintainer must add the ok-to-test label after the latest push.

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Tip of the day

💡 Did you know, you can turn these tips off under Display preferences

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[qodo-code-review]

PR Summary by Qodo

Document automatic review skips and no-review control label

📝 Documentation 🕐 10-20 Minutes

AI Description

• Documents draft, no-review label, and prose-only documentation skips for automatic reviews.
• Clarifies skipped pushes clear readiness labels to prevent stale review state.
• Adds fullsend-no-review while preserving explicit manual review commands.

Diagram

graph TD
  A["PR event"] --> B{"Draft?"} -- No --> C{"No-review label?"} -- No --> D{"Prose-only docs?"} -- No --> E["Automatic review"]
  B -- Yes --> F{"Push event?"}
  C -- Yes --> F
  D -- Yes --> F
  F -- Yes --> G["Clear state labels"] --> H["Skip review"]
  F -- No --> H

Loading

High-Level Assessment

Documenting skip behavior directly beside automatic triggers is the clearest approach because users encounter these exceptions where dispatch behavior is described. Relying only on ADR 0096 would hide operational guidance, while the retained ADR link provides deeper rationale without overloading the control-label table.

Files changed (1) +15 / -2

Documentation (1) +15 / -2

review.mdDocument automatic review skips and suppression label +15/-2

Document automatic review skips and suppression label

• Adds an Automatic skips section covering drafts, 'fullsend-no-review', and eligible prose-only documentation changes, including per-repository exceptions and fail-safe behavior. Clarifies readiness-label clearing for skipped pushes and adds 'fullsend-no-review' to the control-label reference.

docs/review.md

[waynesun09]

Review findings on the new Automatic skips section: 1 high, 6 medium. All are inline. The high one is a merge-order blocker — the behaviour this page documents is not on fullsend main yet and the ADR link 404s today.

[waynesun09]

HIGH — Documents behaviour that is not on fullsend main yet; the ADR link 404s today

Every behaviour this section documents — the draft skip, the fullsend-no-review label, the prose-only docs skip, and the label clearing on a skipped push — exists only in fullsend-ai/fullsend#6587, which is still OPEN (verified: state OPEN, mergedAt null, head b998b8d9). Verified against fullsend origin/main (ad1037e1): .github/workflows/reusable-dispatch.yml has no PR_IS_DRAFT check, no has_label "fullsend-no-review", no docs-lockfile-check step and no clear-stale-merge-labels job — the pull_request_target arm at line 286 dispatches review on every opened|synchronize|ready_for_review. The ADR link on line 40 returns HTTP 404 (verified by curl against https://github.com/fullsend-ai/fullsend/blob/main/docs/ADRs/0096-skip-provably-unnecessary-review-dispatch.md); the file exists only on the #6587 branch. Agents CI runs no markdown link checker, so this merges silently and public agents main will tell users that drafts and labeled PRs are not reviewed while every opened/synchronize still dispatches. Per-repo shims resolve reusable-dispatch.yml@main, so installs pick the behaviour up when #6587 merges, not when this PR merges.

Suggestion: Hold this PR until fullsend#6587 merges — mark it draft or add an explicit "blocked by fullsend-ai/fullsend#6587" line to the body — and re-check that the ADR 0096 URL resolves on main before merging.

[waynesun09]

MEDIUM — Triggers section still claims write-level permission; fullsend main gates review at triage

Independent of #6587. On fullsend origin/main (ad1037e1) .github/workflows/reusable-dispatch.yml gates automatic review dispatch with is_event_actor_authorized "${PR_USER_LOGIN}" triage (line 286) and /fs-review with is_authorized triage (line 229), per ADR 0054/#5223. fullsend's own docs/agents/review.md line 34 already says "Requires triage-level repository permission or higher (triage, write, maintain, or admin)". This page's unchanged line 26 ("All automatic triggers require the actor to have write-level repository permission (admin, maintain, or write)") and line 48 ("Requires write-level repository permission (admin, maintain, or write).") are both wrong today. A PR whose stated purpose is making the Triggers section accurate leaves the section stale.

Suggestion: Update line 26 and line 48 to "triage-level repository permission or higher (triage, write, maintain, or admin)", matching fullsend docs/agents/review.md. Mutation stages such as /fs-fix still require write, so keep that distinction if you mention it.

[waynesun09]

MEDIUM — "the label is an explicit request" overstates what ready-for-review bypasses

The parenthetical "(In per-repo installs, applying ready-for-review to a draft still dispatches — the label is an explicit request.)" holds only for the draft skip. Verified at #6587 head b998b8d9: the labeled arm reads if [[ "${TRIGGERING_LABEL}" == "ready-for-review" ]] && ! has_label "fullsend-no-review" "${PR_LABELS}" — so labeling a fullsend-no-review PR does not dispatch — and the docs-lockfile-check step's if: excludes only github.event_name != 'issue_comment', so it also runs on pull_request_target: labeled and still suppresses a prose-only PR. Only /fs-review bypasses all three skips. The stated rationale invites readers to generalise the label override to the other two skips, and the doc gives no precedence rule for PRs that hit more than one skip.

Suggestion: Reword to: "(In per-repo installs, applying ready-for-review to a draft still dispatches — the label bypasses the draft check only; it does not override fullsend-no-review or the prose-only skip. /fs-review is the only path that bypasses all three.)"

[waynesun09]

MEDIUM — Draft bullet says "PR/MR" but none of the three skips exist on GitLab

Line 20 says "A PR/MR is opened (drafts excluded — see below)" and line 34 says "The PR/MR is a draft.", but the skips are GitHub-only. Verified: #6587 leaves internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-dispatch.yml untouched (git diff origin/main pr6587-head -- internal/scaffold/fullsend-repo-gitlab/ is empty), and grepping that file at the #6587 head for draft|no-review returns nothing — every merge_request_event still routes to STAGE="review" (line 183). A GitLab user reading this page would expect draft MRs and fullsend-no-review to suppress review; neither does. The following two bullets say "The PR"/"A PR" without "/MR", so the section is also internally inconsistent about its own scope.

Suggestion: Scope the section to GitHub — e.g. open with "On GitHub installs, the review agent does not run automatically … when:" — and drop "/MR" from the draft bullet and from the line-20 parenthetical, or add an explicit note that GitLab MR pipelines do not implement these skips.

[waynesun09]

MEDIUM — Synchronize trigger bullet left unqualified while draft pushes are skipped

Line 20 gained "(drafts excluded — see below)" but line 21, "New commits are pushed to a PR/MR (synchronized)", was left with no caveat. Verified at #6587 head: the guard if [[ "${EVENT_ACTION}" == "ready_for_review" || "${PR_IS_DRAFT}" != "true" ]] covers opened|synchronize|ready_for_review, so pushes to a draft are skipped exactly as opens are. A reader scanning only the Triggers list concludes that pushes to a draft still trigger a review — the precise misconception this PR exists to remove.

Suggestion: Mirror the "(drafts excluded — see below)" qualifier on the synchronize bullet, or restructure so Triggers lists raw events and defers every exception to Automatic skips.

[waynesun09]

MEDIUM — Exact allowlist and scan rules transcribed from an unmerged PR that has already changed mid-review

The prose-skip bullet states as settled fact the four-path allowlist (docs/guides/, docs/problems/, docs/agents/, docs/glossary.md), the four executable-markup classes, and the truncation rule — all copied from fullsend#6587 at head b998b8d9, which is still under review. That rule has already changed once during review: commit d79011a67 ("fix(dispatch): docs-only skip is an allowlist that reads the page") replaced the earlier docs/*.md-minus-docs/ADRs/** formulation after a review finding, and commit 6ca0ee3e8 ("fix(dispatch): protect contract markdown from the docs-only skip") reshaped it again. Nothing in this PR or its body pins which revision of #6587 it mirrors, so if the allowlist moves again this page drifts with no signal. Separately, the bullet's "never skips" list omits the rename rule that is in the same implementation: if ! is_prose "${path}" || { [[ -n "${prev}" ]] && ! is_prose "${prev}"; } classifies previous_filename too, so a Go file renamed into docs/guides/ is still reviewed.

Suggestion: Reduce the bullet to the stable contract ("a small allowlist of prose directories under docs/; everything else — markdown elsewhere, contract markdown, lockfiles — is still reviewed") and defer the exact list to ADR 0096, or note which fullsend revision the list mirrors. Either way add the rename rule ("a file renamed into an allowlisted path is judged on its previous path as well") and re-diff this bullet against #6587's merged head before merging.

[waynesun09]

MEDIUM — "Mirrors fullsend-no-fix" overclaims — the label is not protected from the review agent's label actions

The new control-label row says fullsend-no-review "Mirrors the fix agent's fullsend-no-fix label", but the mirror is incomplete in this same repo. Verified at PR head ab6e2fa, scripts/post-review.src.sh lines 305-308: REVIEW_CONTROL_LABELS=("ready-for-merge" "requires-manual-review" "rejected" "ready-for-review" "fullsend-no-fix" "fullsend-fix") — fullsend-no-fix is protected, fullsend-no-review is not. is_control_label (line 310) is the only guard on the label_actions loop (line 370), so a review round whose agent emits {action: remove, label: fullsend-no-review} passes validation and strips the label, silently re-enabling automatic review on a PR a human opted out of. (Stale-label cleanup itself only touches risk/*, so this is agent-emitted actions, not automatic removal.) ADR 0096 defers /fs-review-stop but says nothing about the control-label denylist, so this gap is not a stated deferral.

Suggestion: Add fullsend-no-review to REVIEW_CONTROL_LABELS in scripts/post-review.src.sh (plus the compiled post-review.sh and coverage in post-review-test.sh) in this PR or a linked follow-up, or drop the "Mirrors … fullsend-no-fix" claim until the protection actually matches.

[guyoron1]

@waynesun09 — all seven confirmed and addressed. New head 2ec5ada.

• HIGH (line 40 / whole Automatic skips section): Agreed — the draft
  skip, fullsend-no-review, the prose-only skip, the label-clearing on a
  skipped push and ADR 0096 all live only in fullsend#6587 (verified OPEN,
  head b998b8d9; ADR 0096 404s on main). Rather than block on the
  merge-order, I removed the unshipped section, the fullsend-no-review
  control-label row and the 404 link so the page describes only what is on
  fullsend main today. The skip docs land in a follow-up alongside #6587,
  once ADR 0096 resolves on main.
• line 26 / 48 (write- vs triage-level): Fixed. Both now read
  "triage-level repository permission or higher (triage, write, maintain, or
  admin)", matching fullsend docs/agents/review.md and the triage gate in
  reusable-dispatch (lines 229/286, ADR 0054). Independent of #6587.
• line 34 (label-override wording): Moot — the draft-skip bullet was
  removed with the section. Will carry your precise "bypasses the draft check
  only" wording when it is re-added with #6587.
• line 34 (PR/MR vs GitLab scope): Moot — section removed; the follow-up
  will scope the skips to GitHub explicitly.
• line 21 (synchronize left unqualified): Fixed — the "(drafts excluded)"
  qualifier on line 20 was reverted, so the Triggers list is internally
  consistent again (raw events, no partial exceptions).
• line 36 (allowlist transcribed from an unmerged PR): Moot — the bullet
  was removed. The follow-up will state the stable contract and defer the
  exact allowlist plus the rename rule to ADR 0096, pinned to #6587's merged
  head.
• line 62 (fullsend-no-review overclaim + code gap): Doc row removed, so
  the "Mirrors fullsend-no-fix" overclaim is gone. Declining the
  REVIEW_CONTROL_LABELS code change here — the label is not dispatched on
  main yet, so protecting it belongs with #6587 that introduces it, not this
  docs branch. Worth a linked follow-up on #6587.

Net: this PR now only corrects the live trigger-permission wording; the skip
documentation is deferred to land with fullsend#6587.