Skip to content

ci: pin the doc bot to the merge commit and always report the outcome - #25762

Merged
mshabarov merged 6 commits into
mainfrom
fix-deleted-branch-for-docs-bot
Sep 21, 2026
Merged

mshabarov merged 6 commits into
mainfrom
fix-deleted-branch-for-docs-bot

Conversation

@mshabarov

Copy link
Copy Markdown
Contributor

DX/docs · doc-bot workflow · everyone merging a pull request to main

Background — the gh-aw pull request checkout. A workflow compiled by
gh-aw from a pull request trigger always gets a "Checkout PR branch" step
that fetches the head branch by name. gh-aw suppresses that step for
pull_request_target, a change its ADR-46771 made for this situation.

The bot runs when a pull request is merged, the one moment its head branch
no longer exists, because GitHub deletes it on merge. Every run therefore
failed the branch checkout and wrote an expected-failure warning into its
summary. The bot also said nothing when it found no documentation was
needed, so the only way to learn how a run ended was to open the run log.

Risks:

  • ⚠️ Merged pull requests from forks are documented now. A pull_request
    run from a fork gets no secrets, so those merges went undocumented.
    The same-repository guard the compiler injected for pull_request is
    gone with the trigger, as is a stacked-pull-request guard.
  • ⚠️ Every run that reaches the agent comments on the source pull
    request now, the runs that write no documentation included.
  • 🔒 pull_request_target runs with secrets in reach. Nothing untrusted
    runs: the workspace is pinned to a commit that is already on main, and
    the workflow never builds or runs the project. The compiler warns about
    the combination anyway, because it matches on the shape, not on the ref.
  • ✅ No API, dependency, secret or action changes, and no build impact.

Context. The failing step broke nothing, because github.sha happens
to be the merge commit for a merged pull request. It did hide an
inconsistency: when a head branch survived the merge, the step succeeded
and moved the workspace onto the pull request head instead.

  • Switched the trigger to pull_request_target, which drops the "Checkout
    PR branch" step and its checkout_pr_success output
  • Pinned the workspace checkout to the merge commit
  • Made the Phase 6 comment mandatory, with one shape per outcome:
    documentation written, nothing to document, or documentation rejected by
    an earlier review
  • Removed the noop safe-output and told the agent not to reach for the
    built-in one, because the standing comment is the record of a run now
  • Described the new trigger and the mandatory comment in the workflow README

The bot runs when a pull request is merged, which is the one moment its
head branch no longer exists: GitHub deletes it on merge. gh-aw emits a
"Checkout PR branch" step for every pull request trigger, so every run
failed it and wrote an expected-failure warning into its summary.
`pull_request_target` is the trigger gh-aw suppresses that step for, and
pinning the checkout to the merge commit makes the tree the agent reads
right by construction rather than by coincidence.

The standing comment is mandatory now, including for the runs that write
no documentation, so whoever merged the pull request does not have to
open a run log to find out how the run ended.
@mshabarov

Copy link
Copy Markdown
Contributor Author

Type of change

  • Internal change

How to test

The trigger only fires on a merge into main, so this cannot be exercised
before it is merged. On the next merged pull request that passes the cheap
filters:

  1. Open the Documentation Bot run for it in the Actions tab
  2. The summary has no "⚠️ Closed Pull Request" block, and the job has no
    "Checkout PR branch" step at all
  3. The merged pull request carries exactly one Documentation Bot comment,
    either linking a draft pull request in vaadin/docs or saying no
    documentation was needed and why
What changed in detail

gh aw compile leaves one warning on this workflow now:

pull_request_target trigger with checkout enabled is extremely insecure.

It is a shape match, not an analysis of the ref. The checkout is pinned to
github.event.pull_request.merge_commit_sha, which for a merged pull
request is a commit already on main, reviewed by whoever pressed Merge.
The agent only reads files with cat and grep and writes into
docs-repo/; nothing in the workflow builds or runs the project, and the
permissions stay contents: read and pull-requests: read.

The compiled diff is only these removals and one addition:

  • gone: the Checkout PR branch step, the checkout_pr_success output and
    its GH_AW_CHECKOUT_PR_SUCCESS wiring into the failure handler
  • gone: Restore agent config folders from base branch, which was gated on
    that step. With no pull request head in the workspace there is no
    untrusted agent config to overwrite
  • gone: the same-repository guard and the stacked-pull-request guard the
    compiler injects for pull_request
  • added: ref: on the workspace checkout, in the agent job and in the
    safe-output job

actionlint passes. Spotless has nothing to do here — it covers Java,
TypeScript, CSS and POM files, and this change touches Markdown and the
generated YAML only.

Evidence from the runs

The run for #25710 (34942441389) shows both halves of the problem:

Fetching branch: fix/theme-css-plugin-breaking-sourcemaps from origin
fatal: couldn't find remote ref fix/theme-css-plugin-breaking-sourcemaps
⚠️ Closed PR Checkout Warning
Branch likely deleted: fix/theme-css-plugin-breaking-sourcemaps

and, a few seconds earlier, the workspace landing on the right commit
anyway:

git fetch --depth=1 origin +dd98e9a032936120044e19ef8945cff4e49b1284:refs/remotes/origin/main

dd98e9a032936120044e19ef8945cff4e49b1284 is the merge commit of #25710,
which is why the failing step never broke a run.

For the fork half: #25606 was merged from a fork on 10 September. Its
Documentation Bot run, 34490804140, has zero jobs and no logs, and no
documentation pull request came out of it.

@mshabarov
mshabarov marked this pull request as ready for review September 16, 2026 10:45
@github-actions

Copy link
Copy Markdown
Contributor

The trigger swap drops a step, and the checkout ref is pinned instead of assumed

flowchart LR
    subgraph Before
        direction TB
        B1["pull_request (closed, merged)"] -->|"checks out github.sha (default)"| B2["actions/checkout"]
        B1 -->|"fetches head branch by name"| B3["Checkout PR branch (checkout-pr): fails, branch deleted on merge"]
        B1 -->|"runs Phase 0-5"| B4["Phase 6: comment, or noop if nothing to report"]
    end
    subgraph After
        direction TB
        A1["pull_request_target (closed, merged)"] -->|"checks out merge_commit_sha (new)"| A2["actions/checkout"]:::changed
        A1 -->|"runs Phase 0-5"| A3["Phase 6: always comments, one of three shapes (new)"]:::changed
    end
    Before ~~~ After
    classDef changed stroke:#c9a227,stroke-width:3px
Loading

Switching the trigger from pull_request to pull_request_target in doc-bot.md removes gh-aw's generated Checkout PR branch step, which always failed here because the head branch is gone by the time a merge fires the bot. The workspace checkout now pins ref to github.event.pull_request.merge_commit_sha instead of relying on github.sha, and Phase 6 in the prompt no longer has a noop escape hatch — every run ends in exactly one of three comment shapes, per the commit message and PR description.

Diagram Bot draws the mechanism this pull request touches; it does not review the change. Verify it against the diff.

Generated by Diagram Bot for #25762 · agent · 40.1 AIC · ⌖ 5.18 AIC · ⊞ 8.4K · ◷

@github-actions

github-actions Bot commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

Test Results

 1 455 files  ±0   1 539 suites  ±0   1h 32m 21s ⏱️ + 2m 9s
11 942 tests ±0  11 874 ✅ ±0  68 💤 ±0  0 ❌ ±0 
12 257 runs  ±0  12 189 ✅ ±0  68 💤 ±0  0 ❌ ±0 

Results for commit d545433. ± Comparison against base commit 1500da8.

♻️ This comment has been updated with latest results.

platosha
platosha previously approved these changes Sep 16, 2026
@platosha
platosha added this pull request to the merge queue Sep 16, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to a conflict with the base branch Sep 16, 2026
…r-docs-bot

Conflicts in the doc bot's prompt and its compiled lock file, both from
#25760 landing on main in the meantime. Phase 6 keeps that pull request's
`<REFERENCE>` paragraph and loses its "do not comment when you recorded a
`noop`" line, which this branch's mandatory comment replaces. The lock
file was recompiled with `gh aw compile` rather than merged by hand.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
platosha and others added 4 commits September 18, 2026 10:03
#25797 raised the doc bot's bar on main and, in doing so, added `noop`
instructions to Phase 0, Phase 2, Phase 3 and a new self-check — the
outcome this branch replaces with a mandatory Phase 6 comment. Its
substance is kept whole; each `noop` it records is routed to that comment
instead, and Phase 6 gains a fourth shape for the open pull request under
the old `[docs] ` prefix that #25797 introduced. The lock file was
recompiled with `gh aw compile` rather than merged by hand.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sonarqubecloud

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
C Security Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@mshabarov
mshabarov merged commit 8c119de into main Sep 21, 2026
40 of 41 checks passed
@mshabarov
mshabarov deleted the fix-deleted-branch-for-docs-bot branch September 21, 2026 12:26
totally-not-ai Bot added a commit that referenced this pull request Sep 21, 2026
The same fix landed on main in #25762: `create-pull-request` now carries
`assignees`, the `assign-to-user` output is gone, and Phase 5a tells the
agent the assignment is done for it. Both conflicting files are resolved
to main's version, which says all of that and keeps the wording of the
surrounding rewrite that came with it. Nothing is left on this branch
that main does not already have.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants