Skip to content

feat(review-agent)!: skip review dispatch that cannot produce a review #6587

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 10 commits into
base: main
Choose a base branch
from
Open

feat(review-agent)!: skip review dispatch that cannot produce a review #6587

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c479546
feat(review-agent): skip review dispatch that cannot produce a review
guyoron1 Aug 25, 2026
38d3cc5
fix(dispatch): protect contract markdown from the docs-only skip
guyoron1 Sep 5, 2026
cd8a6dd
test(scaffold): run the docs skip instead of matching its text
guyoron1 Sep 5, 2026
1052342
docs(review): record what the review skips actually do
guyoron1 Sep 5, 2026
17986ed
fix(dispatch): docs-only skip is an allowlist that reads the page
guyoron1 Sep 10, 2026
2951d86
fix(dispatch): clear ready-for-merge when a push skips review
guyoron1 Sep 10, 2026
f517a01
docs(adr): state the rule that keeps the docs skip out of the scaffold
guyoron1 Sep 10, 2026
6965cd1
fix(dispatch): close markup-scanner bypasses that skip live pages
guyoron1 Sep 11, 2026
932f1eb
fix(dispatch)!: label clearing needs its token and the kill switch
guyoron1 Sep 11, 2026
b9fdcc6
docs(review): the ready-for-review label is subject to the skips
guyoron1 Sep 11, 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
181 changes: 170 additions & 11 deletions .github/workflows/reusable-dispatch.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,9 +108,10 @@ jobs:
issues: read
pull-requests: read
outputs:
stage: ${{ steps.role-check.outputs.skipped != 'true' && steps.agent-check.outputs.skipped != 'true' && steps.pr-check.outputs.skipped != 'true' && steps.route.outputs.stage || '' }}
stage: ${{ steps.role-check.outputs.skipped != 'true' && steps.agent-check.outputs.skipped != 'true' && steps.pr-check.outputs.skipped != 'true' && steps.docs-lockfile-check.outputs.skipped != 'true' && steps.route.outputs.stage || '' }}

@waynesun09 waynesun09 Sep 8, 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 — A skipped review dispatch never clears a stale ready-for-merge label

docs/architecture.md's Coordinator merge algorithm section states the invariant: 'each review run start (including push-triggered re-review) clears ready-for-merge together with ready-for-review so merge approval is never stale after new commits.' Verified on head that this clearing is not implemented anywhere in the workflow YAML — it happens only as part of the 'Run review agent' step itself (line ~1001), which is gated by steps.route.outputs.stage (line 111), and that output is forced empty whenever role-check, agent-check, pr-check, or the new docs-lockfile-check report skipped=true. So on a docs-only push, or a push to a PR just labeled fullsend-no-review, to a PR that already carries ready-for-merge from an earlier round, the job never starts and the label-clearing logic embedded in the review agent run never fires — the stale ready-for-merge label survives unreviewed new commits. The PR's own doc edit to docs/guides/user/bugfix-workflow.md (line 101) implicitly concedes this: it changes 'ready-for-merge is never stale — it always reflects the current PR head' to 'unless the push lands on a draft, on a fullsend-no-review-labeled PR, or is documentation prose only. Short of those, ready-for-merge is never stale' — narrowing the guarantee without saying what happens in the carved-out cases. GitHub's own draft-merge block partially covers the draft case, but not the docs-only or fullsend-no-review cases.

Suggestion: On the docs-lockfile-check and fullsend-no-review skip paths (and ideally the draft path too, for defense in depth), explicitly strip ready-for-merge and ready-for-review via a cheap gh api ... DELETE /labels/{name} step before short-circuiting, so the label can never point at a superseded head. Update docs/guides/user/bugfix-workflow.md and docs/architecture.md to describe the actual (fixed) behavior once this lands.

trigger_source: ${{ steps.route.outputs.trigger_source }}
event_payload: ${{ steps.payload.outputs.event_payload }}
kill_switch: ${{ steps.kill-switch.outputs.kill_switch }}
steps:
- name: Validate event_action
if: ${{ inputs.event_action == '' }}
Expand Down Expand Up @@ -151,6 +152,7 @@ jobs:
PR_HEAD_REPO: ${{ github.event.pull_request.head.repo.full_name }}
PR_BASE_REPO: ${{ github.event.pull_request.base.repo.full_name }}
PR_USER_LOGIN: ${{ github.event.pull_request.user.login }}
PR_IS_DRAFT: ${{ github.event.pull_request.draft && 'true' || 'false' }}
ORG_NAME: ${{ github.repository_owner }}
GH_TOKEN: ${{ github.token }}
run: |
Expand Down Expand Up @@ -273,7 +275,7 @@ jobs:
STAGE="code"
fi
elif [[ "${TRIGGERING_LABEL}" == "ready-for-review" ]]; then
if [[ "${ISSUE_IS_PR}" == "true" ]]; then
if [[ "${ISSUE_IS_PR}" == "true" ]] && ! has_label "fullsend-no-review"; then
STAGE="review"
fi
fi
Expand All @@ -283,12 +285,19 @@ jobs:
pull_request_target)
case "${EVENT_ACTION}" in
opened|synchronize|ready_for_review)
if [[ "${PR_USER_LOGIN}" =~ \[bot\]$ ]] || is_event_actor_authorized "${PR_USER_LOGIN}" triage; then
STAGE="review"
# Drafts don't get an automatic review on open/sync — the
# review fires once the PR is marked ready (ADR 0096).
# fullsend-no-review blocks automatic dispatch only, mirroring
# the fullsend-no-fix label (ADR 0034); an explicit /fs-review
# comment still works on drafts and labeled PRs alike.
if [[ "${EVENT_ACTION}" == "ready_for_review" || "${PR_IS_DRAFT}" != "true" ]] && ! has_label "fullsend-no-review" "${PR_LABELS}"; then
if [[ "${PR_USER_LOGIN}" =~ \[bot\]$ ]] || is_event_actor_authorized "${PR_USER_LOGIN}" triage; then
STAGE="review"

@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 — Behaviour-changing PR missing the ! breaking-change marker required by COMMITS.md

Verified against COMMITS.md on head (Breaking changes section, ~lines 64-117): "Default values change in ways that alter existing behavior" is listed as breaking, both the ! suffix and a BREAKING CHANGE: trailer are required, and the section closes by instructing reviewers to "Flag a missing ! as an important-severity finding" because GoReleaser builds user-facing release notes from merged PR titles.

This PR changes default dispatch behaviour for every enrolled repo: drafts, fullsend-no-review-labeled PRs and docs-prose-only PRs no longer receive an automatic review, and a synchronize that skips now deletes ready-for-merge/ready-for-review.

Checked directly: the PR title is feat(review-agent): skip review dispatch that cannot produce a review (no !), none of the seven commit subjects on the branch carry !, and the PR body contains no BREAKING CHANGE note.

Suggestion: Retitle to feat(review-agent)!: skip review dispatch that cannot produce a review and add a BREAKING CHANGE: trailer to the relevant commit body naming the three new skip conditions, the new label-clearing on skipped pushes, and /fs-review as the escape hatch.

fi
fi
;;
labeled)
if [[ "${TRIGGERING_LABEL}" == "ready-for-review" ]]; then
if [[ "${TRIGGERING_LABEL}" == "ready-for-review" ]] && ! has_label "fullsend-no-review" "${PR_LABELS}"; then
STAGE="review"
fi
;;
Expand Down Expand Up @@ -405,16 +414,24 @@ jobs:
fi

- name: Check kill switch
if: steps.route.outputs.stage != ''
id: kill-switch
# Always runs so it can publish kill_switch downstream, even when no
# stage routed (draft, fullsend-no-review, below triage): the
# clear-stale-merge-labels job must honour the switch on those paths
# too. Dispatch is still halted (exit 1) only when a stage routed.
env:
STAGE: ${{ steps.route.outputs.stage }}
run: |
set -euo pipefail
KILL_SWITCH=false
if [[ -f .fullsend/config.yaml ]]; then
KILL_SWITCH=$(yq '.kill_switch // false' .fullsend/config.yaml)
if [[ "$KILL_SWITCH" == "true" ]]; then
echo "::error::Kill switch is active — all agent dispatch halted"
echo "::error::Set kill_switch: false in .fullsend/config.yaml to resume"
exit 1
fi
fi
echo "kill_switch=${KILL_SWITCH}" >> "${GITHUB_OUTPUT}"
if [[ "$KILL_SWITCH" == "true" && -n "$STAGE" ]]; then
echo "::error::Kill switch is active — all agent dispatch halted"
echo "::error::Set kill_switch: false in .fullsend/config.yaml to resume"
exit 1
fi

- name: Validate agents config
Expand Down Expand Up @@ -511,6 +528,111 @@ jobs:
exit 0
fi

- name: Check docs/lockfile-only diff
id: docs-lockfile-check
if: steps.route.outputs.stage == 'review' && steps.role-check.outputs.skipped != 'true' && steps.agent-check.outputs.skipped != 'true' && github.event_name != 'issue_comment'
env:
GH_TOKEN: ${{ github.token }}
SOURCE_REPO: ${{ github.repository }}
PR_NUMBER: ${{ github.event_name == 'pull_request_target' && github.event.pull_request.number || github.event.issue.number }}
run: |
set -euo pipefail
# Skip review when every changed file is documentation prose
# (ADR 0096). An explicit /fs-review comment bypasses this check
# entirely (the issue_comment guard above), matching the draft and
# label bypasses.
#
# Prose is an allowlist, not "docs/ minus contracts": `case` globs
# match `/`, so a bare docs/*.md arm reaches every nested markdown
# file under docs/, and the tree keeps growing load-bearing pages
# (architecture.md, cli/, normative/, contributing/, reference/,
# ADRs/, .vitepress/) that a denylist has to chase. A directory is
# skippable only once it is listed here. Everything else stays
# reviewed — including markdown outside docs/ (skills/*/SKILL.md,
# AGENTS.md, CLAUDE.md are executable agent instruction) and
# lockfiles (npm resolves from package-lock.json, so a lockfile-only
# diff can repoint a transitive dependency without touching
# package.json).
is_prose() {
case "$1" in
docs/guides/*.md|docs/problems/*.md|docs/agents/*.md|docs/glossary.md) return 0 ;;
esac
return 1
}
if [[ -z "${PR_NUMBER}" || "${PR_NUMBER}" == "null" ]]; then
exit 0
fi
# per_page belongs in the query string: `-F per_page=100` makes
# `gh api` issue a POST, which this endpoint answers with a 404.
# previous_filename is listed too, so a file renamed into docs/ is
# classified on the path it came from as well; it goes last because
# `read` with IFS=tab collapses an empty middle field.
FILES=$(gh api "repos/${SOURCE_REPO}/pulls/${PR_NUMBER}/files?per_page=100" --paginate \
--jq '.[] | [.status, .filename, .contents_url, (.previous_filename // "")] | @tsv') || {
echo "::warning::Failed to fetch changed files for PR #${PR_NUMBER} — not skipping review"
exit 0
}
[[ -z "${FILES}" ]] && exit 0
# GitHub caps this endpoint at 3000 files and stops paginating
# without erroring, so a larger PR would look docs-only from a
# truncated head. Never skip on a truncated listing.
if [[ "$(wc -l <<< "${FILES}")" -ge 3000 ]]; then
echo "::notice::PR #${PR_NUMBER} file list may be truncated — not skipping review"
exit 0
fi
skippable=true
while IFS=$'\t' read -r status path url prev; do
[[ -z "${path}" ]] && continue
if ! is_prose "${path}" || { [[ -n "${prev}" ]] && ! is_prose "${prev}"; }; then
skippable=false; break
fi
[[ "${status}" == "removed" ]] && continue
# A prose path is not enough. VitePress compiles every markdown
# page under docs/ into a Vue component (config.ts srcExclude
# only drops icons and testing/), so a page can carry a
# root-level <script setup> that runs at build time, a <style>
# block, a head: frontmatter key that injects tags, {{ }}
# expressions evaluated during SSG, a build-time <!-- @include -->,
# or bound attributes and directives (:prop, @event, v-*, on*) on
# raw HTML — docs/v/index.md already ships a <script setup>. Read
# the page at the PR head and keep the review unless it is free of
# all of these outside code, where VitePress renders text verbatim
# (v-pre).
#
# The scan is deliberately over-eager — a false positive only
# keeps the review (ADR 0096). Fenced code is stripped because it
# renders verbatim, EXCEPT a fence whose info string ends in -vue,
# which VitePress evaluates; then inline code spans are stripped.
# Matching is case-insensitive because config.ts KNOWN_TAGS is /i,
# so <SCRIPT> renders too; head: tolerates spaces before the colon;
# and the attribute/directive pattern runs over the page with
# newlines flattened, so a binding wrapped onto its own line still
# matches. An unreadable page never skips.
BODY=$(gh api -H 'Accept: application/vnd.github.raw' "${url}") || {
skippable=false; break
}
SCANNED=$(awk '
/^```/ {
if (fence) { fence = 0 }
else { fence = 1; vue = ($0 ~ /-vue[[:space:]]*$/) }
next
}
!fence || vue
' <<< "${BODY}" | sed 's/`[^`]*`//g')
if grep -Eiq '<script|<style|^head[[:space:]]*:|\{\{|@include[[:space:]]*:' <<< "${SCANNED}" \

@waynesun09 waynesun09 Sep 17, 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 — Docs-skip scanner misses quoted and flow-style head frontmatter keys

The scanner's frontmatter check is the regex branch ^head[[:space:]]*: in the grep at :622. It only matches an unquoted key literally at line start. Ran the exact awk/sed/grep pipeline from :614-624 on head b9fdcc6 against crafted pages: plain head: prints KEEP-REVIEW, but "head":, 'head': and a root flow mapping {head: [[script, {src: ...}]]} all print SKIPPED. YAML semantics make each of those the same head key once parsed (VitePress reads frontmatter through gray-matter/js-yaml — this half rests on YAML semantics, not a VitePress run), so a page can inject <script src>/meta tags into the rendered <head> while the step summary reports the PR 'only touches documentation prose ... with no executable markup' and no review is dispatched. This is a residual of the thread at :599 (whose five listed bypasses were closed in 6965cd166); the quoted/flow-style variants were not in that list and are not covered by the adversarial cases added to TestReviewRoutingDocsSkipRuntime.

Suggestion: Stop pattern-matching the frontmatter text. yq is already a dependency of this workflow (kill-switch step, :428): extract the leading --- block from BODY and gate on yq 'has("head")' (or keys | map(ascii_downcase) | index("head")), treating any parse failure as 'keep the review'. Add "head":, 'head': and {head: [...]} cases to TestReviewRoutingDocsSkipRuntime; false positives only keep the review, which ADR 0096 already accepts.

|| printf '%s' "${SCANNED}" | tr '\n' ' ' \
| grep -Eiq '<[A-Za-z][^>]*[[:space:]](:|@|v-|on)[A-Za-z-]+='; then
echo "::notice::${path} carries executable markup — not skipping review"
skippable=false; break
fi
done <<< "${FILES}"
if [[ "${skippable}" == "true" ]]; then
echo "::notice::Skipping review — PR #${PR_NUMBER} only touches documentation prose under docs/"
echo "### fullsend review skipped" >> "${GITHUB_STEP_SUMMARY}"
echo "PR #${PR_NUMBER} only touches documentation prose (markdown under \`docs/guides/\`, \`docs/problems/\`, \`docs/agents/\` or \`docs/glossary.md\`, with no executable markup) — no review dispatched." >> "${GITHUB_STEP_SUMMARY}"
echo "skipped=true" >> "${GITHUB_OUTPUT}"
fi

- name: Resolve PR head for issue_comment events
id: pr-head
if: steps.route.outputs.stage != '' && steps.role-check.outputs.skipped != 'true' && steps.agent-check.outputs.skipped != 'true' && steps.pr-check.outputs.skipped != 'true' && github.event_name == 'issue_comment' && github.event.issue.pull_request
Expand Down Expand Up @@ -582,6 +704,43 @@ jobs:
echo "${DELIM}"
} >> "${GITHUB_OUTPUT}"

clear-stale-merge-labels:

@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 — New label mutation runs even when the kill switch is active, in both dispatch workflows

Verified on head. In reusable-dispatch.yml the "Check kill switch" step is gated if: steps.route.outputs.stage != '' (:415-416). Draft, fullsend-no-review and below-triage skips all produce stage == '', so the kill switch is never evaluated for them and the route job succeeds. The new clear-stale-merge-labels job then runs on needs.route.outputs.stage != 'review' (:688), which is true for exactly those cases, and DELETEs ready-for-merge and ready-for-review.

So with kill_switch: true in .fullsend/config.yaml — documented as "all agent dispatch halted" — fullsend still mutates PR labels on every push to a draft or to a fullsend-no-review PR. Before this PR nothing happened on those paths, so this is new behaviour escaping the kill switch.

The scaffold is broader still: its kill-switch step is gated steps.route.outputs.stage != '' && steps.pr-check.outputs.skipped != 'true' (dispatch.yml:360-361), while the new last step at :600-603 fires on stage != 'review' || role-check skipped || agent-check skipped. Only the docs-prose skip happens to be covered, and only incidentally, because its stage was review when the kill switch ran.

Suggestion: Expose a kill_switch output from the route job (or run the kill-switch step whenever github.event.action == 'synchronize') and add it to the clear-stale-merge-labels job if: and to the scaffold's step if:. Add a test case pinning that no label DELETE is issued while the kill switch is active.

name: Clear stale merge labels
needs: route
# New commits that no review round will look at: the labels the round
# would have cleared at start are cleared here instead. Kept out of the
# route job so that job stays read-only. Skipped while the kill switch is
# active — a halted repo mutates no labels.
if: github.event_name == 'pull_request_target' && github.event.action == 'synchronize' && needs.route.outputs.stage != 'review' && needs.route.outputs.kill_switch != 'true'
runs-on: ${{ inputs.runner_image }}
permissions:
issues: write
pull-requests: write
steps:
- name: Remove ready-for-merge and ready-for-review
env:
GH_TOKEN: ${{ github.token }}
SOURCE_REPO: ${{ github.repository }}
PR_NUMBER: ${{ github.event.pull_request.number }}
run: |
set -euo pipefail
# A push that starts no review round still supersedes the head that
# ready-for-merge and ready-for-review described. A review run start
# clears both (docs/architecture.md, coordinator merge algorithm); when
# the round is skipped — draft, fullsend-no-review, documentation prose,
# or a pusher below triage — nothing else would, and ready-for-merge
# would keep describing a head nobody reviewed (ADR 0096). A 404 means
# the label was not there; anything else fails the job so the stale
# label is visible rather than silently kept.
for label in ready-for-merge ready-for-review; do
if out=$(gh api --method DELETE "repos/${SOURCE_REPO}/issues/${PR_NUMBER}/labels/${label}" 2>&1); then
echo "::notice::Removed stale ${label} from PR #${PR_NUMBER} — this push was not reviewed"
elif [[ "${out}" != *"HTTP 404"* ]]; then
echo "::error::Could not remove ${label} from PR #${PR_NUMBER}: ${out}"
exit 1
fi
done

triage:
name: Triage
needs: route
Expand Down
Loading
Loading