feat(review-agent): skip review dispatch that cannot produce a review - #6587
feat(review-agent): skip review dispatch that cannot produce a review#6587guyoron1 wants to merge 7 commits into
Conversation
E2E tests did not runE2E tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the See E2E testing guide for details. |
1 similar comment
E2E tests did not runE2E tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the See E2E testing guide for details. |
Site previewPreview: https://9baa9ce7-site.fullsend-ai.workers.dev Commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
05d40b9 to
ef01fdd
Compare
PR Summary by QodoSkip unnecessary automatic review dispatches
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
Code Review by Qodo
1. Planned command lacks issue link
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| Three additive skips in `reusable-dispatch.yml`'s "Determine stage" step. | ||
|
|
||
| ### 1. Draft skip |
There was a problem hiding this comment.
2. Adr records three decisions 📜 Skill insight ⚙ Maintainability
ADR 0096 separately decides draft suppression, label-based suppression, and documentation-only suppression. These are three independently applicable routing decisions and must not be recorded as one ADR.
Agent Prompt
## Issue description
ADR 0096 records three distinct review-routing decisions in one Decision section.
## Issue Context
Each ADR must record exactly one decision; create separate ADRs with their own context and consequences.
## Fix Focus Areas
- docs/ADRs/0096-skip-provably-unnecessary-review-dispatch.md[52-112]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| Builds on [ADR 0034](0034-centralized-shim-routing-via-dispatch.md) | ||
| (centralized dispatch routing, and the `fullsend-no-fix` label pattern this | ||
| ADR mirrors) and [ADR 0054](0054-require-authorization-on-all-agent-dispatch-paths.md) | ||
| (authorization on all agent dispatch paths — unaffected by this change). |
There was a problem hiding this comment.
7. Adr links outside context 📜 Skill insight ⚙ Maintainability
The related ADR cross-references are placed between Status and Context rather than inside the Context section. This also introduces a free-standing block that does not follow the ADR template's section order.
Agent Prompt
## Issue description
Related ADR references appear outside the Context section and disrupt the required template structure.
## Issue Context
Preserve the references but incorporate them concisely into the Context section.
## Fix Focus Areas
- docs/ADRs/0096-skip-provably-unnecessary-review-dispatch.md[19-30]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| | `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 the post-script closed it. | | ||
| | `fullsend-no-review` | Prevents automatic (bot-triggered) review runs on this PR. Mirrors the [fix agent](fix.md)'s `fullsend-no-fix` label. Explicit `/fs-review` commands are unaffected. Currently applied manually (no `/fs-review-stop` command yet — see [ADR 0096](../ADRs/0096-skip-provably-unnecessary-review-dispatch.md)). | |
There was a problem hiding this comment.
8. Planned command lacks issue link 📜 Skill insight ≡ Correctness
The not-yet-implemented /fs-review-stop command is documented as follow-up work without a `> **Planned:**` callout or issue link. Readers cannot trace the planned feature and may mistake the prose for an informal commitment.
Agent Prompt
## Issue description
The guide mentions the unimplemented `/fs-review-stop` feature without the required planned-feature callout and tracking link.
## Issue Context
Create or reference the relevant issue and use the exact `> **Planned:**` blockquote format.
## Fix Focus Areas
- docs/agents/review.md[61-61]
- docs/ADRs/0096-skip-provably-unnecessary-review-dispatch.md[74-82]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review sweep — 5 MEDIUM findings, 4 posted inline below; the one below targets a file not in this diff.
MEDIUM — User-facing bugfix guide still says closing the PR is the way to stop review (docs/guides/user/bugfix-workflow.md:169)
Verified on the PR head: docs/guides/user/bugfix-workflow.md:169 reads 'review is triggered automatically by PR events (pull_request_target), so closing the PR is the way to stop review dispatch'. This PR adds the fullsend-no-review label specifically as the lever to stop automatic review, and adds draft/docs-only skips, but the guide is not in the diff. The trigger descriptions at :83 ('review agents take over automatically'), :130/:134 ('Triggered by: pull_request_target events (PR opened, push to PR branch, ...)') and :183 ('Push commits ... the review agents will re-review') likewise do not mention the new skip exceptions. docs/agents/review.md was updated; this user-facing guide was not.
Suggestion: Update the 'Stopping automation' bullet at :169 to name the fullsend-no-review label as the way to stop automatic review (closing the PR is no longer the only option), and add a short note to the Stage 3 trigger description (:134) that drafts, fullsend-no-review-labeled PRs and docs-prose-only diffs are skipped unless /fs-review is used.
| [[ -z "$f" ]] && continue | ||
| case "$f" in | ||
| docs/ADRs/*) skippable=false; break ;; | ||
| docs/*.md) ;; |
There was a problem hiding this comment.
MEDIUM — docs/*.md skip swallows normative contract and contributing markdown the ADR says it protects
bash case globs match /, so the docs/*.md) ;; arm treats every nested markdown file under docs/ as skippable prose. Verified with the step's exact case block on the PR head: docs/normative/normalized-event/v1/README.md (NormalizedEvent v1 routing-input spec, ADR 0061), docs/normative/normalized-event/v1/jira-poll-adapter.md, docs/normative/prescript-output/v1/README.md (pre-script output contract) and docs/contributing/workflow-contracts.md (the sync contract governing the file this PR edits) all resolve to SKIP. ADR 0096 line 95 argues docs/* is dangerous precisely because it would swallow docs/normative/**, but the chosen pattern still swallows the normative prose specs alongside the JSON schema. A PR changing the documented meaning of a NormalizedEvent field without touching the JSON would receive no automatic review. docs/agents/review.md:47 documents only the ADR exclusion.
Suggestion: Add explicit non-skippable arms before docs/*.md: docs/normative/*|docs/contributing/*) skippable=false; break ;; (or invert to an allow-list of low-stakes prose directories such as docs/guides/*). Update ADR 0096 §3 and docs/agents/review.md:47 to state exactly which directories are excluded, and extend TestReviewRoutingDocsSkip to pin the new arms.
|
|
||
| - Fewer wasted review runs: drafts, `fullsend-no-review`-labeled PRs, and | ||
| documentation-prose-only PRs no longer trigger automatic review dispatch, | ||
| reducing inference cost with no loss of coverage — nothing that would have |
There was a problem hiding this comment.
MEDIUM — ADR 0096 asserts 'no loss of coverage' without evidence or threat-model evaluation
The Consequences bullet states 'no loss of coverage — nothing that would have been usefully reviewed is skipped' as settled fact, and the PR body says the work started from 'tallying dispatches', but no tally, cost figure, or sample of skipped PRs appears in the PR or ADR. Verified counter-evidence on head: (a) the glob already skips docs/normative/*.md and docs/contributing/*.md contract prose (see the comment at reusable-dispatch.yml:572); (b) the review agent's charter in fullsend-ai/agents docs/review.md lists 'docs currency' as a review dimension, and docs/agents/*.md + docs/contributing/*.md are the user-facing product surface — a docs-only follow-up to docs/agents/review.md (the page this PR edits) would itself be skipped; (c) docs/contributing/design-decisions.md:5 ranks external prompt injection as the top threat, and docs-only PRs are a low-friction path to land instruction-like prose that later agents read, with the automated reviewer disabled for exactly that path. The draft and label skips are self-evidently zero-loss; the docs skip is a judgement call shipped as fact. This is distinct from the existing threads on ADR format/length.
Suggestion: Either attach the dispatch tally (count and share of docs-only review runs over a stated window) and record in Consequences that the trade-off was evaluated against the injection threat model, or narrow the skip to an allow-list of low-stakes prose paths. In both cases soften the 'no loss of coverage' wording to describe the actual trade-off.
| The `/fs-review` command does not accept arguments. The review agent also runs automatically when a PR is opened, | ||
| synchronized (new commits pushed), or moved out of draft by a user with triage-level repository permission or higher. | ||
|
|
||
| ### Automatic skips |
There was a problem hiding this comment.
MEDIUM — fullsend-ai/agents docs/review.md 'Triggers' section is now stale (needs a companion PR in the agents repo)
Verified against the live fullsend-ai/agents main: docs/review.md 'Triggers' states the review agent runs automatically when 'A PR/MR is opened' and 'New commits are pushed to a PR/MR (synchronized)', with no draft, fullsend-no-review, or docs-only caveat, and its Control labels table has no fullsend-no-review row. After this PR those statements are wrong for drafts, labeled PRs and docs-prose-only PRs. That document lives in fullsend-ai/agents (ADR 0058 split) and cannot be fixed from this repo; the PR and ADR reference no companion change.
Suggestion: Open a companion PR in fullsend-ai/agents updating docs/review.md Triggers (draft exclusion, fullsend-no-review label, docs-only skip with the exact exclusions) and its Control labels table, and link it from this PR and ADR 0096. Do not attempt to fix it via internal/scaffold/ here.
| assert.Contains(t, s, "id: docs-lockfile-check") | ||
| assert.Contains(t, s, "steps.docs-lockfile-check.outputs.skipped != 'true'", | ||
| "job-level stage output must account for the docs skip") | ||
| assert.Contains(t, s, `if: steps.route.outputs.stage == 'review' && steps.role-check.outputs.skipped != 'true' && steps.agent-check.outputs.skipped != 'true'`, |
There was a problem hiding this comment.
MEDIUM — TestReviewRoutingDocsSkip pins substrings, not behaviour: passed green against a 404ing step and does not lock the /fs-review bypass guard
Verified on head. (1) The if: assertion at :651 checks only if: steps.route.outputs.stage == 'review' && steps.role-check.outputs.skipped != 'true' && steps.agent-check.outputs.skipped != 'true' and omits the trailing && github.event_name != 'issue_comment'; if that guard were removed the test still passes and the docs skip would apply to explicit /fs-review runs, contradicting ADR 0096's escape-hatch guarantee. (2) The gh-api pin at :653 matches the current command, which (per the existing thread at reusable-dispatch.yml:555) issues a POST and 404s, so the test passed CI while the feature it guards can never fire. (3) The two NotContains assertions at :663/:665 check literal strings (*.md|docs/*, package-lock.json|yarn.lock) that no plausible regression would produce, so they are trivially true. The PR body's claim that the tests were 'verified meaningful by reverting each guard' holds for TestReviewRoutingSkips' regexes but not for this function. hack/lint-workflow-size's own error text says logic belongs in scripts/ where it can be tested.
Suggestion: Add an explicit assertion that the docs-lockfile-check if: includes github.event_name != 'issue_comment'. When fixing the POST bug (the pin at :653 must change anyway), extract the step body to .github/scripts/check-docs-only-diff.sh and add a shell test that runs it with a stubbed gh on PATH covering: docs-only listing, a docs/ADRs/ entry, a docs/normative/ entry, a 3000-line listing, and a gh failure (fail-open). Keep the YAML test as a wiring check only, and cite a real dispatch run where the skip fired.
|
@waynesun09 Thanks. Three commits: c0f5bf7, 4d1defb, 8696c93. The skip now matches
With no tally, ADR 0096 states what a skipped diff can contain and the residual risk instead of claiming none. The guide names |
8696c93 to
4b3ef74
Compare
|
Rebased onto main at New commit ids, in order:
These replace the ids in my previous comment. Nothing else changed. |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Automated review sweep — 4 findings inline (2 HIGH, 2 MEDIUM).
| while IFS= read -r f; do | ||
| [[ -z "$f" ]] && continue | ||
| case "$f" in | ||
| docs/ADRs/*|docs/normative/*|docs/contributing/*|docs/reference/*|docs/.vitepress/*) |
There was a problem hiding this comment.
HIGH — Docs-prose skip assumes VitePress markdown is inert outside docs/.vitepress/ — it isn't
The docs-only skip treats every docs/*.md file as inert prose except five protected prefixes (docs/ADRs/, docs/normative/, docs/contributing/, docs/reference/, docs/.vitepress/). ADR 0096 (line 102) frames docs/.vitepress/ as 'executable' as if that's the only executable surface, but VitePress compiles every markdown file under the configured srcDir (not just .vitepress/), and any of those files may contain a root-level <script setup> block that runs Vue/JS at build time. Verified on PR head: docs/.vitepress/config.ts's srcExclude (line 194) only excludes **/agents/icons/** and **/testing/** — the rest of docs/ (including docs/v/, docs/guides/, docs/agents/**) is compiled. docs/v/index.md is a live counterexample: it is not under any protected prefix, matches the docs/*.md skippable arm, and already contains <script setup> importing a third-party package (@lando/vitepress-theme-default-plus). A future PR editing such a page's script, or adding a head: frontmatter key that injects a <script src=...> tag, would be classified as 'documentation prose' and skip the review agent — even though docs/contributing/design-decisions.md ranks external prompt injection as the top threat the review agent exists to catch.
Suggestion: Either reject any docs/*.md diff that introduces a <script, <style>, or head: frontmatter key (detectable from the diff content, not just the path), or replace the denylist-of-five-prefixes approach with an allowlist of directories verified to be pure prose. At minimum, update ADR 0096's threat model to acknowledge VitePress markdown is not inert outside .vitepress/, and add docs/v/** (and any other page using <script setup> or custom head:) to the protected set.
| 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 || '' }} |
There was a problem hiding this comment.
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.
| while IFS= read -r f; do | ||
| [[ -z "$f" ]] && continue | ||
| case "$f" in | ||
| docs/ADRs/*|docs/normative/*|docs/contributing/*|docs/reference/*|docs/.vitepress/*) |
There was a problem hiding this comment.
MEDIUM — Protected-docs prefix list omits docs/architecture.md and docs/cli/**, both load-bearing
The five protected prefixes (docs/ADRs/, docs/normative/, docs/contributing/, docs/reference/, docs/.vitepress/) do not cover docs/architecture.md (the living architecture overview contributors are told to keep current — and which this very PR should have updated but didn't) or docs/cli/**.md (the manually maintained CLI contract; verified 7 files exist under docs/cli/ on PR head: README.md, agent.md, github.md, inference.md, mint.md, repos.md, run.md). Because bash case glob matching treats * as matching /, both docs/architecture.md and docs/cli/agent.md fall through to the bare docs/*.md) ;; skippable arm and would silently skip automatic review today. This is a distinct, narrower gap from the already-flagged normative/contributing coverage (a prior review comment on this PR at reusable-dispatch.yml:579 already got docs/normative/ and docs/contributing/ added as protected prefixes — architecture.md and docs/cli/ were not part of that fix and remain unprotected).
Suggestion: Add docs/architecture.md and docs/cli/ to the protected-prefix case arms (or move to an allowlist of low-risk directories such as docs/guides/, docs/problems/, docs/agents/ instead of a denylist over an open-ended docs/ tree).
| review on a draft, a labeled PR, or a prose-only PR at any time. | ||
| - `fullsend-no-review` must be created and applied by hand until an | ||
| `/fs-review-stop` command exists, unlike `/fs-fix-stop`. | ||
| - The prose skip is per-repo only because the per-org mode is deprecated |
There was a problem hiding this comment.
MEDIUM — ADR 0096's scaffold-mirroring exemption for docs-lockfile-check is self-contradicting
ADR 0096's Consequences (lines 144-148) justify not mirroring docs-lockfile-check into internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml by saying it is 'per-repo only because the per-org mode is deprecated (ADR 0044) and docs/contributing/workflow-contracts.md scopes cross-mode sync to payload construction, stage routing, and secret threading — the draft and label checks, being stage routing, are mirrored into the scaffold as usual.' Verified on head: docs-lockfile-check is structurally identical to role-check/agent-check/pr-check — all four gate the job's stage output via the same steps.X.outputs.skipped != 'true' chain (reusable-dispatch.yml:111) — and role-check/agent-check/pr-check are confirmed present and mirrored in the scaffold's dispatch.yml (verified: id: role-check at line 406, id: agent-check at 429/430, id: pr-check at 249, with no docs-lockfile-check step between agent-check (ending line 452) and the next step (line 454) in the scaffold file). The ADR's own reasoning is circular: per-org being deprecated (ADR 0044) applies equally to the draft/label checks, yet those ARE mirrored 'as usual' despite the same deprecation fact — so deprecation cannot be what's actually driving the asymmetry, and workflow-contracts.md's text does not itself distinguish 'stage routing' from 'post-route gate' the way the ADR now claims.
Suggestion: Either mirror docs-lockfile-check into the scaffold dispatch.yml for consistency with its three sibling gates, or replace the current circular justification in ADR 0096 and docs/contributing/workflow-contracts.md with a rule that actually distinguishes 'this specific gate' from role-check/agent-check/pr-check/draft/label routing — e.g., ground it in something docs-lockfile-check alone requires (a paginated cross-API call not needed by per-org's config-driven routing) rather than the deprecation argument, which does not hold up against the mirrored siblings.
Three cases where the review agent is dispatched, runs a full inference, and cannot say anything useful (ADR 0096). Drafts no longer trigger an automatic review on open or push — the review fires once the PR is marked ready. A fullsend-no-review label suppresses automatic dispatch, mirroring the fullsend-no-fix label the fix agent already honours. And a PR whose changed files are all documentation prose under docs/ is skipped with a notice in the job summary. That last pattern is deliberately narrow, because bash case globs match '/': docs/* would swallow the TypeScript under docs/.vitepress and the normative event schema, and a bare *.md would swallow skills/*/SKILL.md, AGENTS.md and CLAUDE.md — markdown that is executable agent instruction rather than prose. ADRs are excluded because a new ADR is among the things most worth reviewing, and lockfiles are not skippable at all: npm resolves from package-lock.json, so a lockfile-only diff can repoint a transitive dependency without touching package.json. A truncated file listing never skips either, since GitHub caps the files endpoint at 3000 entries and stops paginating without erroring. An explicit /fs-review bypasses all three skips. The draft and label checks are mirrored into the per-org scaffold, which docs/contributing/workflow-contracts.md requires for stage routing. The docs skip is not: that installation mode is deprecated (ADR 0044) and the requirement is scoped to routing, payload construction, and secret threading, none of which this step is. Rebased onto current main; renumbered the new ADR from 0091 to 0096 to avoid a collision with 0091-per-agent-runtime-model-effort.md, which landed upstream after this branch was cut. Signed-off-by: guy oron <goron@redhat.com>
bash case globs match '/', so the docs/*.md arm reached every nested markdown file under docs/ — including docs/normative/, whose prose specs are the contracts other repos build payloads against, and docs/contributing/workflow-contracts.md, which governs this workflow. ADR 0096 argued docs/* was dangerous for exactly that reason and then shipped a pattern with the same reach. The contract directories now match first: docs/ADRs/, docs/normative/, docs/contributing/, docs/reference/ and docs/.vitepress/. Two other ways a non-prose change could have slipped through the check. -F per_page=100 makes gh api issue a POST, which the files endpoint answers with a 404 — the step then took its fail-open path on every run, so no PR was ever actually skipped; per_page moves into the query string. And the listing named only each file's new path, so renaming a Go file to docs/notes.md read as prose; previous_filename is now classified too. Signed-off-by: guy oron <goron@redhat.com>
TestReviewRoutingDocsSkip pinned substrings, which is why it stayed green while the step it guards could never fire: the gh-api assertion matched the POST form of the call exactly. Its two NotContains assertions checked literals no plausible regression would produce, and the if: assertion omitted the issue_comment guard that keeps /fs-review a complete escape hatch, so removing that guard would not have failed anything. The wiring half now asserts the whole if: expression, guard included. The decision itself is not observable from the YAML text, so a runtime test parses the step's run: body out of the workflow and executes it against a stubbed gh — the same shape as TestShimStopFixAuthorizationRuntime. It covers prose-only, each protected prefix, markdown outside docs/, a lockfile, a rename into docs/, a 3000-entry listing and an api failure, and the stub refuses anything that is not a GET. Reverting any one of the four guards fails at least one case. Signed-off-by: guy oron <goron@redhat.com>
The ADR's Consequences claimed "no loss of coverage — nothing that would have been usefully reviewed is skipped". That was asserted, not measured, and after the prefix fix it is still not quite true: docs currency is one of the review agent's own dimensions, and prose is where instruction-like text a later agent reads would land, with external prompt injection at the top of the threat order in docs/contributing/design-decisions.md. The bullets now say which diffs stop being reviewed, what one can and cannot contain, and where the residual risk sits — bounded by what the check can see rather than by trust, with /fs-review always available. Decision §3 gains the protected prefixes, the rename handling and the GET. docs/agents/review.md lists the exclusions rather than only ADRs. The user guide said closing the PR was the way to stop review dispatch; the fullsend-no-review label is, and it leaves /fs-review working. Stage 3 gains a "Not triggered when" line, and the three other places that promised a review on every push now say otherwise. The companion Triggers section in fullsend-ai/agents docs/review.md is stale for the same reason and needs its own PR there — it cannot be edited from this repo. Signed-off-by: guy oron <goron@redhat.com>
The docs-prose skip classified by path alone and treated docs/ minus a
denylist of contract directories as inert. Neither held. VitePress
compiles every markdown page under docs/ into a Vue component — config.ts
srcExclude only drops icons and testing/ — so a page on a prose path can
carry a root-level <script setup> (docs/v/index.md already does), a
<style> block, a head: frontmatter key that injects tags, {{ }}
expressions evaluated during SSG, or bound attributes on raw HTML, and
still be classified as prose. And the denylist kept missing pages:
docs/architecture.md and docs/cli/**, both load-bearing, fell through to
the docs/*.md arm because case globs match '/'.
Prose is now an allowlist — docs/guides/, docs/problems/, docs/agents/,
docs/glossary.md — so a new directory is reviewed until someone lists it,
and each allowlisted page is read at the PR head and keeps its review if
it carries any of those constructs outside fenced or inline code, where
VitePress renders text verbatim. An unreadable page never skips, and a
removed page is not read. grep reads the whole page instead of -q: under
pipefail an early exit can SIGPIPE awk on a long page and turn a hit into
a miss. previous_filename moves to the last tsv column because read with
IFS=tab collapses an empty middle field.
The runtime test's gh stub now serves page bodies, and covers each
executable construct, markup inside code, an unreadable page, a removed
page, and the pages the allowlist deliberately leaves out.
Signed-off-by: guy oron <goron@redhat.com>
docs/architecture.md holds that every review run start clears ready-for-merge together with ready-for-review, so merge approval is never stale after new commits. That clearing lives in the review run itself, so each skip this branch added — draft, fullsend-no-review, documentation prose — left a ready-for-merge applied to an earlier head standing on commits nobody reviewed. The guide had been narrowed to admit as much instead of saying what happens. A clear-stale-merge-labels job now runs on every pull_request_target synchronize whose composite stage output is not review and removes both labels; a 404 means the label was not there, anything else fails the job so the stale label is visible. It is a job of its own so the route job, which parses untrusted event data, keeps its read-only token; the per-repo shim already grants the dispatch job issues and pull-requests write. The scaffold gets the same script as the last step of its single job, with its token widened the same way. As a side effect a push by an actor below triage, which never routed, also clears the labels. The alignment test pins the job's wiring and the route job's read-only permissions, requires both workflows to run the same script, and runs that script against a stub gh for the 200, 404 and 403 cases. Signed-off-by: guy oron <goron@redhat.com>
ADR 0096 justified not mirroring docs-lockfile-check into the per-org scaffold by per-org's deprecation, while mirroring the draft and label skips "as usual" despite the same deprecation. role-check, agent-check and pr-check are all mirrored too, and two of them post-date ADR 0044, so neither the deprecation date nor "stage routing" separates the cases. The rule that does is correctness parity versus optimisation: whatever decides whether an event may dispatch, which stage it reaches, or which labels it leaves behind is mirrored, because the two modes must agree on that; a step that declines a dispatch that would be valid, to save inference spend, is not, because a mode scheduled for removal gets no optimisations. Recorded in the ADR and in workflow-contracts.md, where the sync contract is defined. Signed-off-by: guy oron <goron@redhat.com>
4b3ef74 to
b998b8d
Compare
|
@waynesun09 thanks — every thread addressed; rebased onto current main and force-pushed. 09-08:
09-02:
Scaffold shim now grants |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review sweep on head b998b8d — 5 findings inline (2 HIGH, 3 MEDIUM). Review-only; no approval or change request implied.
| contents: read | ||
| id-token: write | ||
| pull-requests: read | ||
| issues: write |
There was a problem hiding this comment.
HIGH — Scaffold dispatch job requests permissions its per-org caller shim does not grant — every per-org dispatch run fails at validation
Verified on head b998b8d. This PR raises the scaffold's single dispatch job from pull-requests: read to issues: write + pull-requests: write (dispatch.yml:22-29, hunk @@ -26,7 +26,8 @@), but the per-org caller was not updated: internal/scaffold/fullsend-repo/templates/shim-workflow-call.yaml:53-57 still grants only actions: write, id-token: write, contents: read, pull-requests: read — no issues scope at all — before uses: __ORG__/.fullsend/.github/workflows/dispatch.yml@main.
A workflow_call callee may only downgrade the caller's grant; requesting more fails validation at run start ("The workflow is requesting 'issues: write', but is only allowed 'issues: none'"), so the whole per-org dispatch job dies on every event — not just the new label step.
Contrast templates/shim-per-repo.yaml:40-46, which does grant issues: write + pull-requests: write; that is why the per-repo clear-stale-merge-labels job is fine. ADR 0096 §4 papers over the gap: "the per-repo shim already grants the dispatch job issues: write and pull-requests: write. The scaffold mirrors it as a last step of its single job, whose token is widened the same way" — that conflates the per-repo shim with the per-org caller.
The new test compounds this: internal/scaffold/workflow_call_alignment_test.go:1057-1059 asserts the callee's write permissions but nothing about the caller, so it pins the broken state green.
Suggestion: Widen templates/shim-workflow-call.yaml:53-57 to grant issues: write and pull-requests: write (or drop the label-clearing step from the per-org scaffold, consistent with how docs-lockfile-check is already excluded there), and add an assertion to TestReviewSkipClearsStaleMergeLabels that the caller shim's permission set is a superset of the callee job's, so caller/callee drift cannot pass again. Also correct the ADR §4 sentence, which currently cites the per-repo shim as evidence for the per-org widening.
| } | ||
| if awk '/^```/ { in_code = !in_code; next } !in_code' <<< "${BODY}" \ | ||
| | sed 's/`[^`]*`//g' \ | ||
| | grep -E '<script|<style|^head:|\{\{|<[A-Za-z][^>]*[[:space:]](:|@|v-|on)[A-Za-z-]+=' >/dev/null; then |
There was a problem hiding this comment.
HIGH — Executable-markup scanner is bypassable: case-sensitive tag match, line-anchored attribute regex, and skipped -vue fences
Verified empirically by running the exact pipeline from :597-599 against crafted pages. All of the following are classified as inert prose and skip review, while rendering as live content in this repo's VitePress pipeline:
<SCRIPT setup>/<STYLE>— the grep is case-sensitive (<script|<style), butdocs/.vitepress/config.ts:85-86KNOWN_TAGScarries the/iflag, soescapeVueSyntaxpasses the uppercase<through untouched and the tag reaches the renderer. Bypass confirmed.- Multiline bound attributes/directives —
<[A-Za-z][^>]*[[:space:]](:|@|v-|on)[A-Za-z-]+=is matched line by line, so<span\n v-html="payload"\n/>and<div\n :title="Math.random()"\n>split the tag from the binding and match nothing.v-htmlis the headline case: an unreviewed prose page can inject raw HTML. Bypass confirmed. ```js-vuefences — theawk '/^```/ { in_code = !in_code; next } !in_code'toggle drops all fence content, andescapeVueSyntaxalso returns fence lines unchanged (config.ts:80) while VitePress omitsv-prewhen the info string ends in-vue, so{{ 40 + 2 }}inside a```js-vuefence evaluates at build. Bypass confirmed.head :with a space before the colon in frontmatter —^head:misses it; YAML accepts it. Bypass confirmed.<!--@include: path-->— VitePress build-time file inclusion is not scanned at all, letting a prose page splice arbitrary repo content into a published page. Bypass confirmed.
One thing that is not a problem and should not be changed: {{ }} inside inline code really is inert here — config.ts:523-525 sets v-pre on code_inline and escapeVueSyntax copies backtick spans through untouched — so the markup inside code is inert case at workflow_call_alignment_test.go:971 is correct as written.
This is distinct from the thread at :577, which predates the scanner and asked for it to exist; this is about the scanner's implementation being evadable.
Suggestion: Make the grep case-insensitive (grep -Ei) and tolerate whitespace before the frontmatter colon (^head[[:space:]]*:). Join lines between < and the matching > before running the attribute/directive pattern (or scan with grep -Pzo in DOTALL mode) so multiline bindings cannot split. Keep scanning inside fences whose info string ends in -vue, and add @include: to the pattern. Add adversarial cases to internal/scaffold/workflow_call_alignment_test.go for uppercase <SCRIPT>, a multiline v-html, and a js-vue fence — all false positives merely keep the review, which ADR 0096 already accepts.
| echo "${DELIM}" | ||
| } >> "${GITHUB_OUTPUT}" | ||
|
|
||
| clear-stale-merge-labels: |
There was a problem hiding this comment.
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.
|
|
||
| The review agent does not run automatically — though `/fs-review` always works — when: | ||
|
|
||
| - **The PR is a draft.** Opening or pushing to a draft does not trigger a review; it runs once the PR is marked ready for review. (Applying the `ready-for-review` label to a draft still dispatches — the label is an explicit request.) |
There was a problem hiding this comment.
MEDIUM — Doc says the ready-for-review label is an unconditional explicit request, but two of the three skips still apply on the labeled path
Verified on head. Line 46 states: "(Applying the ready-for-review label to a draft still dispatches — the label is an explicit request.)" That is only true of the draft skip.
(a) The fullsend-no-review label suppresses the labeled path too — reusable-dispatch.yml and the scaffold both gate it with && ! has_label "fullsend-no-review" (scaffold hunks @@ -175,7 +177,7 @@ and @@ -185,12 +187,19 @@).
(b) The docs-prose skip runs on the labeled path as well: docs-lockfile-check's if: at reusable-dispatch.yml:524 excludes only github.event_name != 'issue_comment', so a labeled event flows straight into it.
Concretely: a maintainer who labels a docs-only draft PR ready-for-review expecting an explicit review gets silence. Only /fs-review is a genuine unconditional bypass, which is what ADR 0096 §1/§2 actually promise.
This is separate substance from the thread at review.md:42, which is about the stale docs/review.md in the agents repo.
Suggestion: Pick one semantics and make docs and code agree: either treat the label as explicit — add github.event.action != 'labeled' to the docs-lockfile-check if: and drop the has_label "fullsend-no-review" guard on the labeled arms — or reword line 46 to say the label is an automatic trigger subject to all three skips and that /fs-review is the only unconditional override. Pin the chosen behaviour in TestReviewRoutingSkips.
| # 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" |
There was a problem hiding this comment.
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.
The Automatic skips section, the fullsend-no-review control-label row and the ADR 0096 link documented behaviour that lives only in fullsend-ai/fullsend#6587, which is still OPEN. On fullsend main the reusable-dispatch review arm has no draft/no-review/prose-only skip and the ADR 0096 URL 404s, so the page told users drafts and labeled PRs are not reviewed while every opened/synchronize still dispatches. Remove that unshipped content; it lands with #6587. Also fix a live, independent inaccuracy the section left stale: review dispatch is gated at triage-level, not write-level (reusable-dispatch uses is_authorized/is_event_actor_authorized triage per ADR 0054). Match fullsend docs/agents/review.md. Signed-off-by: guy oron <goron@redhat.com>
2 participants
Heyaa : )
This started from tallying dispatches where the review agent runs a full inference and cannot produce a useful review. Three cases, now skipped before dispatch:
fullsend-no-reviewlabel — mirroring thefullsend-no-fixlabel the fix agent already honours (ADR 0034 pattern).docs/*.mdminusdocs/ADRs/**, with a notice in the job log and summary.An explicit
/fs-reviewbypasses all three.The docs glob is deliberately narrow, because the obvious patterns are dangerous:
docs/*would swallow the TypeScript underdocs/.vitepress/and the publisheddocs/normative/**schema;*.mdwould swallowSKILL.md/AGENTS.md— markdown that is executable agent instruction, not prose; and a new ADR is among the things most worth reviewing.Two things are never skippable: lockfiles (a lockfile-only diff can repoint a transitive dependency's
resolved/integritywithout touchingpackage.json— supply-chain relevant, so Renovate PRs keep their review) and truncated file listings (GitHub caps/pulls/{n}/filesat 3000 entries without erroring).TestReviewRoutingSkips/TestReviewRoutingDocsSkippin the routing conditions and assert the dangerous broad globs are absent — verified meaningful by reverting each guard and watching them fail.go test ./internal/scaffold/andpre-commit run --all-filespass, including the ADR lint hooks.Note: the ADR is 0096 — 0090 and 0091 were both taken upstream while this branch was in flight. Known follow-up, disclosed in the ADR:
fullsend-no-reviewrequires manual label creation; there's no/fs-review-stopcommand yet, unlike/fs-fix-stop.