Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
guyoron1 wants to merge 4 commits into
base: main
Choose a base branch
from
+2 −2
Open

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

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ab6e2fa
docs(review): list the dispatch skips under Triggers and the fullsend…
guyoron1 Sep 10, 2026
2ec5ada
docs(review): correct trigger permission; defer skip docs to #6587
guyoron1 Sep 11, 2026
1946583
Merge upstream main into docs/review-triggers-skips
guyoron1 Nov 14, 2026
d4bfb17
docs(review): state the GitLab Developer role for automatic review tr…
guyoron1 Nov 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions docs/review.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ No additional setup is required beyond the standard fullsend configuration.

The review agent runs automatically when:

- A PR/MR is opened
- A PR/MR is opened (drafts excluded — see below)
- New commits are pushed to a PR/MR (synchronized)

@waynesun09 waynesun09 Sep 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

- A PR/MR is moved out of draft

Expand All @@ -27,6 +27,18 @@ All automatic triggers require the actor to have write-level repository permissi

It can also be triggered manually with the `/fs-review` command.

@waynesun09 waynesun09 Sep 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


### Automatic skips

The review agent does not run automatically — `/fs-review` still works — when:

- **The PR/MR is a draft.** It runs once the PR/MR is marked ready for review. (In per-repo installs, applying `ready-for-review` to a draft still dispatches — the label is an explicit request.)

@waynesun09 waynesun09 Sep 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 waynesun09 Sep 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

- **The PR carries the `fullsend-no-review` label.** See [Control labels](#control-labels) below.
- **The diff is documentation prose only** (per-repo installs). A PR whose changed files are all markdown under `docs/guides/`, `docs/problems/`, `docs/agents/` or `docs/glossary.md`, with no executable markup outside code (`<script>`/`<style>` blocks, `head:` frontmatter, `{{ }}` interpolation, bound attributes or directives on raw HTML), is skipped with a notice in the job summary. Markdown anywhere else — every other `docs/` directory, `skills/*/SKILL.md`, `AGENTS.md`, `CLAUDE.md` — and lockfiles are still reviewed. A page that cannot be read, or a truncated file listing, never skips.

@waynesun09 waynesun09 Sep 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


A push skipped for any of these reasons still clears `ready-for-merge` and `ready-for-review`, so the labels never describe commits nobody reviewed.

See [fullsend ADR 0096](https://github.com/fullsend-ai/fullsend/blob/main/docs/ADRs/0096-skip-provably-unnecessary-review-dispatch.md) for the rationale.

@waynesun09 waynesun09 Sep 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


## Commands

| Command | Where | Effect |
Expand All @@ -39,14 +51,15 @@ The `/fs-review` command does not accept arguments.

## Control labels

These labels reflect the review outcome and are updated after each review.
These labels reflect the review outcome and are updated after each review; `fullsend-no-review` is the exception — it is an input, applied by a human.

| Label | Meaning |
|-------|---------|
| `ready-for-review` | Workflow state marker on the PR. Applied by the [code agent](code.md) after pushing. In per-repo installs, triggers review when applied to a PR. |
| `ready-for-merge` | The review agent approved the PR. No blocking findings. |
| `requires-manual-review` | The review agent found issues that require human judgment — it could not confidently approve or reject. |
| `rejected` | The review agent rejected the PR and closed it. |
| `fullsend-no-review` | Prevents automatic review runs on this PR. Mirrors the [fix agent](fix.md)'s `fullsend-no-fix` label. Applied manually; explicit `/fs-review` commands are unaffected. |

@waynesun09 waynesun09 Sep 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


When the review agent requests changes (without rejecting), no outcome label is
applied — the `pull_request_review` event triggers the [fix agent](fix.md) directly.
Expand Down
Loading