From 251b5f32c9b5948db249f812f41b847fff103ab0 Mon Sep 17 00:00:00 2001 From: Dan Barr <6922515+danbarr@users.noreply.github.com> Date: Mon, 24 Aug 2026 13:35:31 -0400 Subject: [PATCH 1/3] Assign a release owner to autogen-docs PRs Review-then-merge-by-committee left these PRs with no clear owner: five contributors each had a review request, nobody owned merging, and PRs stalled for days while later releases queued behind them. Only 2 of 10 toolhive docs PRs since July 1 had every requested reviewer approve within the 2-business-day SLO. Two changes to fix the incentives: Name an owner. A new step resolves whoever cut the upstream release and sets them as the PR assignee as well as a reviewer. They collect the remaining approvals and merge. `release.author` is unreliable here (it returns the release App for toolhive and registry-server), so the resolution falls back to the merger of the upstream release PR, then the tagged commit's author. Stop requesting reviews nobody needs. The skill now classifies each release contributor as docs-facing or not from the diffs it already read in Phase 2, and writes REVIEWERS.json. Contributors whose work had no reader-visible surface get no review request; they are named on one cc line in the PR body and one line in the Slack message, asking them to speak up if the call was wrong. The owner is always requested regardless of classification. A missing or stub REVIEWERS.json falls back to requesting everyone, so losing the classification degrades to today's behavior rather than to silence. The Slack message is restructured around the same idea: owner and their duty first, then one short line per reviewer, then a single consolidated FYI line. The owner-duty and FYI wording is fixed in the poster script rather than composed per release so it stays consistent and short enough to actually get read. Co-Authored-By: Claude Opus 5 (1M context) --- .claude/skills/upstream-release-docs/SKILL.md | 49 ++- .github/scripts/post_autogen_docs_slack.py | 122 ++++++- .github/workflows/autogen-docs-notify.yml | 72 ++-- .github/workflows/upstream-release-docs.yml | 314 ++++++++++++++++-- .gitignore | 5 + 5 files changed, 510 insertions(+), 52 deletions(-) diff --git a/.claude/skills/upstream-release-docs/SKILL.md b/.claude/skills/upstream-release-docs/SKILL.md index 96d53554..c3db93de 100644 --- a/.claude/skills/upstream-release-docs/SKILL.md +++ b/.claude/skills/upstream-release-docs/SKILL.md @@ -39,7 +39,7 @@ Any output that may be rendered as a GitHub comment, PR body, or Markdown file i This skill runs in one of two modes. **The caller signals the mode; absent an explicit unattended signal, assume interactive.** Never infer unattended mode from surrounding context. -**Interactive (default):** a human is present. At decision points that need product context you cannot derive from source (Phase 2 step 4), ask the user. **Never write the `GAPS.md`, `SUMMARY.md`, or `NO_CHANGES.md` artifacts described below in interactive mode**; surface that information conversationally instead. Those files are machine-readable handoff artifacts for an automated caller, and writing them during a local run just litters the repo root. +**Interactive (default):** a human is present. At decision points that need product context you cannot derive from source (Phase 2 step 4), ask the user. **Never write the `GAPS.md`, `SUMMARY.md`, `NO_CHANGES.md`, or `REVIEWERS.json` artifacts described below in interactive mode**; surface that information conversationally instead. Those files are machine-readable handoff artifacts for an automated caller, and writing them during a local run just litters the repo root. **Unattended:** no interactive user, for example a CI workflow that invokes `/upstream-release-docs ... in unattended mode`. Never ask clarifying questions; proceed best-effort at every decision point, and route anything genuinely unresolvable into the artifacts below. @@ -76,6 +76,51 @@ These files are read by the automated caller and spliced into the PR body. The f > ``` +**`REVIEWERS.json`** - written whenever `.release-meta.json` exists at the repo root and lists contributors. This one is JSON, not markdown, because the workflow parses it with `jq` to decide who gets a review request. + +Read `.release-meta.json` first (the caller writes it before invoking you): + +```json +{ + "repo": "stacklok/toolhive", + "prev_tag": "v0.42.0", + "new_tag": "v0.43.0", + "owner": "jerm-dro", + "owner_source": "merged release PR stacklok/toolhive#6333", + "contributors": ["alice", "bob", "carol"] +} +``` + +Classify **every** login in `contributors` as docs-facing or not, and write `REVIEWERS.json` at the repo root: + +```json +{ + "contributors": [ + { + "login": "alice", + "docs_facing": true, + "note": "Confirm the ai-plugin timeout flag section matches what you shipped." + }, + { + "login": "bob", + "docs_facing": false, + "reason": "CI workflow and test-fixture changes only" + } + ] +} +``` + +- `docs_facing: true` means at least one of this person's commits in the release range changed something a reader of the docs can observe: a CLI flag or subcommand, a CRD or config field, an API route, a default, an error message, a user-visible behavior, or anything you documented or corrected in this run. When you are unsure, classify as `true`. A needless review request is a minor annoyance; a missing one means a wrong page ships. +- `docs_facing: false` is for changes with no reader-visible surface: CI and build plumbing, dependency bumps, tests and fixtures, internal refactors, lint fixes, comment-only edits. **Base this on the actual diff you read in Phase 2, not on the commit message.** A commit titled "refactor" that changes a default value is docs-facing. +- `note` (docs-facing only): one short sentence naming the specific thing that person should check, in their terms. Not a summary of the release, and not a restatement of their PR title. Skip the note rather than pad it. +- `reason` (non-docs-facing only): a short phrase naming what their changes actually were. This is shown to them as the justification for not requesting their review, so it has to be specific enough that they can tell whether you got it wrong. +- Include the owner in the list with an honest classification. The workflow requests a review from the owner either way, so classifying the owner `false` costs nothing and keeps the classification truthful. +- Bot logins are already filtered out of `contributors`; if one appears anyway, omit it. + +Write this file even on a `NO_CHANGES.md` run: "no doc-relevant changes for this release" means every contributor is non-docs-facing, which is exactly the case where suppressing review requests matters most. Classify them all `false` with a reason. + +The consequence of skipping this file is concrete: the workflow falls back to requesting a review from every contributor, which is the noisy committee behavior this artifact exists to prevent. + **`NO_CHANGES.md`** - if the Phase 3 impact map is empty (no doc-relevant changes for this release), write this at repo root with a one-line explanation and stop. Do not hand-edit any file. **`SUMMARY.md`** - before the final commit, write a concise list of the hand-written doc changes you made. The caller surfaces it as the PR's "Summary of changes" so reviewers see what shipped without reading the diff. Skip it only when you wrote `NO_CHANGES.md` or made zero hand-edits. Keep it to 3-8 bullets, each formatted as one of: @@ -172,6 +217,8 @@ For each PR identified in Phase 1 (skip internal/infra unless user requests): 10. **Inventory the new public surface.** As you read the source and the regenerated reference assets, list every new or changed user-facing symbol the release introduces: CRD/struct fields, enum values, CLI flags and subcommands, env vars, config keys, and API routes. Most are already enumerated in the auto-synced reference assets (CLI `.md`, CRD `*.schema.json`, Swagger YAML) and the diff, so this is mostly transcription, not discovery. This list is the checklist the completeness pass in Phase 5 verifies against. It is the difference between documentation that is _accurate_ and documentation that is _complete_: a release can ship five new config fields, and a section that explains one of them correctly passes every accuracy check while silently omitting the other four. +11. **Classify the release contributors** (unattended mode only, when `.release-meta.json` is present). You have just read every PR's diff, which makes this the only point in the run where the classification is cheap and well-informed. For each login in `.release-meta.json`, decide whether their commits in the release range changed anything a reader can observe, and write `REVIEWERS.json` per the contract in [Artifacts](#artifacts-unattended-mode-only-written-at-repo-root). Do this from the diffs you read, not from commit messages. + ## Phase 3: Audit Existing Docs 1. Search the documentation codebase for references to affected areas: diff --git a/.github/scripts/post_autogen_docs_slack.py b/.github/scripts/post_autogen_docs_slack.py index 4fc10a40..914b3c2f 100644 --- a/.github/scripts/post_autogen_docs_slack.py +++ b/.github/scripts/post_autogen_docs_slack.py @@ -7,11 +7,19 @@ curl/network access. This script is the ONLY place the Slack token lives, and it only ever contacts slack.com. -It reads the JSON content file, resolves each reviewer's GitHub login to -a Slack user id (falling back to the literal @handle when no confident -match exists), builds one Slack mrkdwn message, and posts it via +It reads the JSON content file, resolves each GitHub login to a Slack +user id (falling back to the literal @handle when no confident match +exists), builds one Slack mrkdwn message, and posts it via chat.postMessage. +The message names one OWNER (the person who cut the upstream release, +who is the PR's assignee and is responsible for merging it), then the +other requested reviewers, then a single line for contributors whose +changes had no docs impact and who were deliberately not sent a review +request. The owner-duty and no-impact wording is fixed in this file +rather than composed by the Claude step, so the process expectations +stay identical release to release. + Reviewer resolution reads the workspace's custom "GitHub handle" profile field reliably: it first discovers that field's id once via team.profile.get, then enumerates members via users.list (used only for @@ -323,8 +331,43 @@ def reviewer_tag(login, login_to_slack_id): return "@{}".format(login) +# Fixed wording for the owner and no-docs-impact lines: these two +# sentences carry the process expectations (who merges, who is not +# blocking), so they live here rather than being composed per release +# by the Claude step, which supplies names only. +# +# The owner duty has two variants so a release with no other reviewers +# doesn't tell the owner to collect approvals nobody was asked for. +OWNER_DUTY_WITH_REVIEWERS = ( + "cut this release and owns this PR: review, collect the approvals " + "below, and merge. Target: 2 business days." +) +OWNER_DUTY_SOLO = ( + "cut this release and owns this PR: review and merge. " + "Target: 2 business days." +) +NO_OWNER_LINE = ( + "*No owner resolved* - nobody is assigned to merge this. " + "A docs maintainer needs to adopt it." +) +NO_IMPACT_SUFFIX = ( + "no docs impact found in your changes, so no review is requested " + "and you're not blocking this. Shout if we misjudged it." +) + + def build_message(content, pr_url, login_to_slack_id): - """Build the single Slack mrkdwn message string.""" + """Build the single Slack mrkdwn message string. + + Layout, in reading order: linked headline, 2-3 scope bullets, the + owner and their duty, one line per reviewer with what to check, and + a single consolidated line for contributors who were deliberately + not sent a review request. + + The owner line comes before the reviewer list because "who merges + this" is the question the message exists to answer; a five-person + reviewer list with no named owner is what produced multi-day stalls. + """ headline = str(content.get("headline") or "Documentation update") lines = ["<{}|{}>".format(pr_url, headline)] @@ -334,22 +377,79 @@ def build_message(content, pr_url, login_to_slack_id): if isinstance(bullet, str) and bullet.strip(): lines.append("• {}".format(bullet.strip())) + # ----- owner ----- + owner_login = "" + owner = content.get("owner") + if isinstance(owner, dict): + owner_login = str(owner.get("login") or "").strip() + elif isinstance(owner, str): + # Tolerate a bare login string in place of the documented + # object form rather than dropping the owner line entirely. + owner_login = owner.strip() + + # ----- reviewers ----- + # Rendered before the owner line is emitted, because the owner's + # duty wording depends on whether anyone else was asked to review. + # The owner is filtered out here as well as in the prompt, so a + # composer that ignores the exclusion can't list them twice. reviewers = content.get("reviewers") - if isinstance(reviewers, list) and reviewers: - lines.append("") - lines.append("*Reviewers*") + rendered = [] + if isinstance(reviewers, list): for reviewer in reviewers: if not isinstance(reviewer, dict): continue - login = reviewer.get("login") + login = str(reviewer.get("login") or "").strip() if not login: continue - note = reviewer.get("note") or "" + if owner_login and normalize_handle(login) == normalize_handle( + owner_login + ): + continue + note = str(reviewer.get("note") or "").strip() tag = reviewer_tag(login, login_to_slack_id) if note: - lines.append("{} — {}".format(tag, note)) + rendered.append("• {} - {}".format(tag, note)) else: - lines.append("{}".format(tag)) + rendered.append("• {}".format(tag)) + + lines.append("") + if owner_login: + duty = ( + OWNER_DUTY_WITH_REVIEWERS if rendered else OWNER_DUTY_SOLO + ) + lines.append( + "*Owner:* {} {}".format( + reviewer_tag(owner_login, login_to_slack_id), duty + ) + ) + else: + lines.append(NO_OWNER_LINE) + + if rendered: + lines.append("*Also review:*") + lines.extend(rendered) + + # ----- no docs impact ----- + # One line, all handles. These contributors get no review request, + # so this is their only notice that the release they contributed + # to has a docs PR at all. + no_impact = content.get("no_docs_impact") + tags = [] + if isinstance(no_impact, list): + for entry in no_impact: + # Accepts a plain login or a {"login": ...} object, so a + # composer reusing the reviewer shape still renders. + if isinstance(entry, dict): + login = str(entry.get("login") or "").strip() + else: + login = str(entry or "").strip() + if login: + tags.append(reviewer_tag(login, login_to_slack_id)) + if tags: + lines.append("") + lines.append( + "*FYI* {} - {}".format(" ".join(tags), NO_IMPACT_SUFFIX) + ) return "\n".join(lines) diff --git a/.github/workflows/autogen-docs-notify.yml b/.github/workflows/autogen-docs-notify.yml index 44aa3f67..a15261a1 100644 --- a/.github/workflows/autogen-docs-notify.yml +++ b/.github/workflows/autogen-docs-notify.yml @@ -191,19 +191,32 @@ jobs: Run this exact command (the values below are already filled in -- do not substitute shell variables, and do not quote the numbers or the repo name): - gh pr view ${{ steps.pr.outputs.number }} --repo ${{ github.repository }} --json title,body,reviewRequests,files,url + gh pr view ${{ steps.pr.outputs.number }} --repo ${{ github.repository }} --json title,body,reviewRequests,assignees,files,url From the JSON, extract: - the PR title - the PR body. The body contains a marker-delimited section written by the Upstream Release Docs workflow, between `` and - ``. Inside it is an - "At a glance" table / summary. PREFER reusing that - summary's content for your bullets; do not pad beyond it. + ``. Inside it are an + "At a glance" table and a "Who does what" section. + PREFER reusing the summary content already there for + your bullets; do not pad beyond it. + - the OWNER: the login in the "Owner" row of the "At a + glance" table, which is also the PR's assignee + (`assignees[].login`). The owner cut the upstream + release and owns getting this PR merged. If the table + says the owner is unresolved and there is no assignee, + set "owner" to null. + - the REQUESTED REVIEWERS (`reviewRequests[].login`). + Exclude the owner from this list; the owner is + reported separately. + - the NO-DOCS-IMPACT contributors: the @-handles listed + under the body's "### No docs impact identified" + heading, if that section is present. These people were + deliberately NOT sent a review request. If the section + is absent, use an empty list. - the list of changed files (for a fallback sense of scope only — do not enumerate every file). - - the list of REQUESTED REVIEWERS (reviewRequests[].login - are GitHub logins). STEP 2 — Write the message content as JSON. Use the **Write** tool to write a file at exactly this path: @@ -212,27 +225,48 @@ jobs: { "headline": "", "summary_bullets": ["...", "..."], + "owner": {"login": ""}, "reviewers": [ {"login": "", "note": ""} ], + "no_docs_impact": ["", "..."], "pr_url": "${{ steps.pr.outputs.url }}" } + + This message is read on a phone, in a busy channel, by + people who have learned to skim it. Length is the enemy; + every word you add lowers the chance the owner sees that + they own it. Be ruthless. + Rules: - "headline" is plain text only (NO Slack/markdown link syntax, NO ids). The poster step turns it into a link. - - "summary_bullets" is a tight list of 2–4 strings, - reusing the PR's "At a glance" content. No padding, no - raw file lists, no internal ids. - - "reviewers" has one entry per REQUESTED reviewer. "login" - is the plain GitHub login (no "@"). "note" is the - specific thing that reviewer should check. When deciding - each note, keep this shared expectation in mind and let - it shape the notes: the goal is for everyone involved in - the release to review and approve within 2 business days; - a given reviewer's contribution to the release may not - have produced any user- or docs-facing changes, which is - expected and fine, and in that case their approval simply - confirms nothing was missed in the generated docs. + Name the project and version and nothing else, e.g. + "toolhive v0.43.0 docs". + - "summary_bullets": 2-3 strings, max ~12 words each, + reusing the PR's "At a glance" and "Summary of changes" + content. No padding, no raw file lists, no internal + ids, no restating the headline. If the PR is a + no-doc-changes or silent run, use ONE bullet saying so + and stop. + - "owner" is an object with the owner's plain GitHub + login (no "@"), or null if none was resolved. Do NOT + write a note for the owner; the poster step states the + owner's responsibilities in fixed wording, so anything + you add here is duplicated text. + - "reviewers": one entry per requested reviewer, EXCLUDING + the owner. "login" is the plain GitHub login (no "@"). + "note" is a fragment, not a sentence: name the specific + thing to check in at most 10 words, no leading verb + like "Please review" and no trailing period. Good: + "ai-plugin timeout flag and its subcommand list". Bad: + "Please review the changes related to the ai-plugin + timeout flag to make sure they are correct." If you + genuinely cannot name something specific for a + reviewer, use an empty string rather than filler. + - "no_docs_impact" is a flat array of plain GitHub logins + (no "@", no notes). The poster step renders them on one + line with fixed wording. - "pr_url" must be copied exactly as filled in above. Do NOT include any Slack ids, Slack mrkdwn, `<@...>` tags, or `` links — only the plain content above. Do NOT diff --git a/.github/workflows/upstream-release-docs.yml b/.github/workflows/upstream-release-docs.yml index 1125ec52..f506efbf 100644 --- a/.github/workflows/upstream-release-docs.yml +++ b/.github/workflows/upstream-release-docs.yml @@ -24,7 +24,10 @@ name: Upstream Release Docs # issues for product context. # 6. Auto-fixes prettier/eslint on the skill-touched files, # commits everything to the PR branch, augments the PR body, -# assigns reviewers from non-bot release contributors +# assigns the release owner, and requests reviews from the +# contributors whose changes the skill classified as docs-facing +# (see the "WHO GETS A REVIEW REQUEST" comment on the +# "Assign reviewers" step for why it isn't everyone) # # Renovate is configured with rebaseWhen: never + recreateWhen: never # so we can push commits without force-push races. @@ -431,6 +434,132 @@ jobs: echo "refreshed=true" >> "$GITHUB_OUTPUT" fi + # Resolves the release OWNER and the contributor candidate list, + # and writes both to .release-meta.json for the skill to read. + # Runs before the skill so the skill can classify each + # contributor as docs-facing or not; the "Assign reviewers" step + # later consumes that classification from REVIEWERS.json. + # + # OWNER RESOLUTION, in order. The owner is whoever cut the + # release and owns driving this PR to merge. + # 1. The release author, when it is a human. Applies to + # projects where a person publishes the release + # (toolhive-studio). + # 2. The merger of the upstream `Release ` PR that the + # tag points at. Applies to projects released by a GitHub + # App, where `release.author` is the App and carries no + # human (toolhive, toolhive-registry-server). + # 3. The tagged commit's author. + # Every candidate is bot-filtered, so a source that yields only + # a bot falls through to the next one. An empty owner is a + # supported outcome: the PR body and Slack message both say the + # owner is unresolved rather than naming someone arbitrary. + - name: Resolve release owner and contributors + id: release_meta + env: + REPO: ${{ steps.detect.outputs.repo }} + PREV: ${{ steps.detect.outputs.prev_tag }} + NEW: ${{ steps.detect.outputs.new_tag }} + run: | + # Shared bot filter. Applied to every login this step + # resolves, from any of the three owner sources and from the + # contributor compare. + is_bot() { + printf '%s\n' "$1" | grep -qE '(\[bot\]$|^app/|^github-actions|^stacklokbot$|^dependabot|^renovate|^copilot)' + } + + # ---------- contributor candidates ---------- + if COMPARE=$(gh api "repos/$REPO/compare/$PREV...$NEW" \ + --jq '[.commits[].author.login? // empty] | unique | .[]' 2>/dev/null); then + COMPARE_OK=true + else + COMPARE="" + COMPARE_OK=false + fi + CANDIDATES=$(echo "$COMPARE" | + grep -Ev '(\[bot\]$|^github-actions|^stacklokbot$|^dependabot|^renovate|^copilot)' || true) + + # ---------- owner ---------- + OWNER="" + OWNER_SOURCE="" + + # (1) Release author, if human. + RELEASE_AUTHOR=$(gh release view "$NEW" --repo "$REPO" \ + --json author --jq '.author.login // ""' 2>/dev/null || true) + if [ -n "$RELEASE_AUTHOR" ] && ! is_bot "$RELEASE_AUTHOR"; then + OWNER="$RELEASE_AUTHOR" + OWNER_SOURCE="release author" + fi + + # (2) Merger of the release PR the tag points at. Deref an + # annotated tag object to its commit first; a lightweight + # tag already points straight at the commit. + if [ -z "$OWNER" ]; then + REF_JSON=$(gh api "repos/$REPO/git/ref/tags/$NEW" 2>/dev/null || true) + OBJ_TYPE=$(printf '%s' "$REF_JSON" | jq -r '.object.type // ""' 2>/dev/null || true) + TAG_SHA=$(printf '%s' "$REF_JSON" | jq -r '.object.sha // ""' 2>/dev/null || true) + if [ "$OBJ_TYPE" = "tag" ] && [ -n "$TAG_SHA" ]; then + TAG_SHA=$(gh api "repos/$REPO/git/tags/$TAG_SHA" \ + --jq '.object.sha // ""' 2>/dev/null || true) + fi + if [ -n "$TAG_SHA" ]; then + # The commits//pulls list endpoint does not populate + # merged_by, so take the PR number from it and read + # mergedBy off the PR itself. + PR_NUM=$(gh api "repos/$REPO/commits/$TAG_SHA/pulls" \ + --jq 'map(select(.merged_at != null)) | .[0].number // ""' 2>/dev/null || true) + if [ -n "$PR_NUM" ]; then + MERGER=$(gh pr view "$PR_NUM" --repo "$REPO" \ + --json mergedBy --jq '.mergedBy.login // ""' 2>/dev/null || true) + if [ -n "$MERGER" ] && ! is_bot "$MERGER"; then + OWNER="$MERGER" + OWNER_SOURCE="merged release PR $REPO#$PR_NUM" + fi + fi + # (3) Tagged commit author, last resort. + if [ -z "$OWNER" ]; then + COMMIT_AUTHOR=$(gh api "repos/$REPO/commits/$TAG_SHA" \ + --jq '.author.login // ""' 2>/dev/null || true) + if [ -n "$COMMIT_AUTHOR" ] && ! is_bot "$COMMIT_AUTHOR"; then + OWNER="$COMMIT_AUTHOR" + OWNER_SOURCE="author of the tagged commit" + fi + fi + fi + fi + + # ---------- handoff file for the skill ---------- + # The skill reads .release-meta.json to learn who the owner + # is and which contributors it must classify. Gitignored. + jq -n \ + --arg owner "$OWNER" \ + --arg owner_source "$OWNER_SOURCE" \ + --arg repo "$REPO" \ + --arg prev "$PREV" \ + --arg new "$NEW" \ + --arg candidates "$CANDIDATES" \ + '{ + repo: $repo, + prev_tag: $prev, + new_tag: $new, + owner: (if $owner == "" then null else $owner end), + owner_source: (if $owner_source == "" then null else $owner_source end), + contributors: ($candidates | split("\n") | map(select(length > 0))) + }' > .release-meta.json + + { + echo "owner=$OWNER" + echo "owner_source=$OWNER_SOURCE" + echo "compare_ok=$COMPARE_OK" + echo "candidates<> "$GITHUB_OUTPUT" + + echo "Release owner: ${OWNER:-} (${OWNER_SOURCE:-no source})" + echo "Contributor candidates:" + echo "${CANDIDATES:-}" + # Anchor the "skill touched" set for the autofix step below. # HEAD at this point is either the refresh commit (if it was # non-empty) or the PR's pre-workflow tip. Either way, anything @@ -559,9 +688,18 @@ jobs: proceed best-effort at every decision point. The skill's "Execution modes" section is the single source of truth for unattended behavior and the GAPS.md / SUMMARY.md / - NO_CHANGES.md artifact contracts (their format, when to - write each, and what to exclude). Follow it; this prompt - deliberately no longer duplicates those contracts. + NO_CHANGES.md / REVIEWERS.json artifact contracts (their + format, when to write each, and what to exclude). Follow + it; this prompt deliberately no longer duplicates those + contracts. + + `.release-meta.json` at the repo root carries the release + owner and the contributor list this run must classify. + Writing REVIEWERS.json is REQUIRED whenever that file + lists contributors: a later workflow step uses it to + decide who gets a review request, and its absence falls + back to requesting a review from every contributor, which + is the noisy behavior we are trying to fix. Prefer reading source code from the local clone at ${{ steps.clone.outputs.scratch_dir }} instead of @@ -954,28 +1092,95 @@ jobs: # text. Running this right before the body augmentation and # flip-to-ready gives reviewers a single coherent notification # on a PR that's actually reviewable. + # + # WHO GETS A REVIEW REQUEST + # ------------------------- + # OWNER -> assignee AND reviewer, unconditionally. They own + # collecting the remaining approvals and merging, so the + # request stands even when the classification marks their + # own commits non-docs-facing. + # DOCS-FACING contributors -> reviewer. + # NON-DOCS-FACING contributors -> no request. Named on one cc + # line in the PR body and one line in the Slack message, + # asking them to speak up if the call was wrong. + # + # The classification comes from REVIEWERS.json, written by the + # skill (see its "Execution modes" section). An absent, + # unparseable, or empty file falls back to requesting every + # contributor: a noisy request costs less than a missing one, so + # the degraded path is the noisy one. - name: Assign reviewers and prepare contributor mentions id: reviewers env: - REPO: ${{ steps.detect.outputs.repo }} - PREV: ${{ steps.detect.outputs.prev_tag }} - NEW: ${{ steps.detect.outputs.new_tag }} REVIEW_REPO: ${{ github.repository }} PR_NUMBER: ${{ steps.eff.outputs.number }} + OWNER: ${{ steps.release_meta.outputs.owner }} + COMPARE_OK: ${{ steps.release_meta.outputs.compare_ok }} + CANDIDATES: ${{ steps.release_meta.outputs.candidates }} run: | - # Get non-bot commit authors in the release range. - if COMPARE=$(gh api "repos/$REPO/compare/$PREV...$NEW" \ - --jq '[.commits[].author.login? // empty] | unique | .[]' 2>/dev/null); then - echo "compare_ok=true" >> "$GITHUB_OUTPUT" + # compare_ok is re-exported unchanged so the PR body's + # tri-state contributor cell keeps working: "" means this + # step never ran, "false" means the upstream compare failed. + echo "compare_ok=$COMPARE_OK" >> "$GITHUB_OUTPUT" + + # ---------- read the skill's classification ---------- + # Both lists are newline-separated logins. + DOCS_FACING="" + NON_DOCS_FACING="" + CLASSIFIED=false + if [ -f REVIEWERS.json ] && jq -e . REVIEWERS.json >/dev/null 2>&1; then + DOCS_FACING=$(jq -r ' + (.contributors // []) + | map(select(.docs_facing == true)) + | .[].login' REVIEWERS.json 2>/dev/null || true) + NON_DOCS_FACING=$(jq -r ' + (.contributors // []) + | map(select(.docs_facing == false)) + | .[].login' REVIEWERS.json 2>/dev/null || true) + # A file that classified nobody is a stub; fall back + # rather than silently requesting no reviews at all. + if [ -n "$DOCS_FACING$NON_DOCS_FACING" ]; then + CLASSIFIED=true + else + echo "::warning::REVIEWERS.json classified no contributors; falling back to requesting all." + fi else - COMPARE="" - echo "compare_ok=false" >> "$GITHUB_OUTPUT" + echo "::warning::No usable REVIEWERS.json; falling back to requesting a review from all contributors." fi - # Filter out bot accounts. - CANDIDATES=$(echo "$COMPARE" | - grep -Ev '(\[bot\]$|^github-actions|^stacklokbot$|^dependabot|^renovate|^copilot)' || true) + if [ "$CLASSIFIED" != "true" ]; then + DOCS_FACING="$CANDIDATES" + NON_DOCS_FACING="" + fi + + # The owner always gets a review request, even when their own + # commits were classified non-docs-facing. Fold them into the + # request list and out of the cc list. + if [ -n "$OWNER" ]; then + DOCS_FACING=$(printf '%s\n%s\n' "$DOCS_FACING" "$OWNER" | grep -v '^$' | sort -u || true) + NON_DOCS_FACING=$(printf '%s\n' "$NON_DOCS_FACING" | grep -v '^$' | grep -Fxv "$OWNER" || true) + fi + # ---------- assign the owner ---------- + # Assignee, not just reviewer: it puts the PR in the owner's + # "Assigned to me" queue. Soft-failed, since an owner + # GitHub won't accept (not a repo collaborator) must not + # abort the run. + OWNER_ASSIGNED=false + if [ -n "$OWNER" ]; then + if gh pr edit "$PR_NUMBER" --add-assignee "$OWNER" 2>/dev/null; then + OWNER_ASSIGNED=true + echo "Assigned owner: $OWNER" + else + echo "::warning::Could not assign $OWNER as assignee (GitHub rejected it)." + fi + else + echo "::warning::No release owner resolved; PR has no assignee." + fi + echo "owner=$OWNER" >> "$GITHUB_OUTPUT" + echo "owner_assigned=$OWNER_ASSIGNED" >> "$GITHUB_OUTPUT" + + # ---------- request reviews ---------- # Attempt to assign each candidate as a reviewer individually, # rather than filtering upfront and batching. Rationale: # - `gh pr edit --add-reviewer "a,b,c"` is atomic. A single @@ -998,17 +1203,28 @@ jobs: [ -z "$login" ] && continue if gh pr edit "$PR_NUMBER" --add-reviewer "$login" 2>/dev/null; then ASSIGN_LIST="${ASSIGN_LIST:+$ASSIGN_LIST,}$login" - echo "Assigned: $login" + echo "Review requested: $login" else MENTION_LIST="${MENTION_LIST:+$MENTION_LIST }@$login" echo "Mention (assignment rejected by GitHub): $login" fi - done <<< "$CANDIDATES" + done <<< "$DOCS_FACING" + + # ---------- cc list for non-docs-facing contributors ---------- + # Rendered as one consolidated line downstream, not a + # per-person section. + FYI_LIST="" + while IFS= read -r login; do + [ -z "$login" ] && continue + FYI_LIST="${FYI_LIST:+$FYI_LIST }@$login" + done <<< "$NON_DOCS_FACING" # Exposed for diagnostic visibility in the PR body (e.g., # "Auto-assigned: @alice @bob") and for the next workflow_ # dispatch retry to know what was attempted. echo "list=$ASSIGN_LIST" >> "$GITHUB_OUTPUT" + echo "fyi_list=$FYI_LIST" >> "$GITHUB_OUTPUT" + echo "classified=$CLASSIFIED" >> "$GITHUB_OUTPUT" { echo "mention_block<> "$GITHUB_OUTPUT" - echo "Auto-assigned: ${ASSIGN_LIST:-}" + echo "Owner: ${OWNER:-}" + echo "Requested: ${ASSIGN_LIST:-}" echo "Mentioned: ${MENTION_LIST:-}" + echo "FYI (no req): ${FYI_LIST:-}" - name: Augment PR body (marker-delimited section) # Runs even if earlier steps soft-failed so the augmentation @@ -1040,6 +1258,10 @@ jobs: COMPARE_OK: ${{ steps.reviewers.outputs.compare_ok }} MENTION_BLOCK: ${{ steps.reviewers.outputs.mention_block }} ASSIGN_LIST: ${{ steps.reviewers.outputs.list }} + FYI_LIST: ${{ steps.reviewers.outputs.fyi_list }} + OWNER: ${{ steps.reviewers.outputs.owner }} + OWNER_ASSIGNED: ${{ steps.reviewers.outputs.owner_assigned }} + OWNER_SOURCE: ${{ steps.release_meta.outputs.owner_source }} SKILL_COMMIT_COUNT: ${{ steps.skill_commits.outputs.count }} GEN_CONCLUSION: ${{ steps.skill_gen.conclusion }} REVIEW_CONCLUSION: ${{ steps.skill_review.conclusion }} @@ -1149,15 +1371,38 @@ jobs: elif [ "$COMPARE_OK" != "true" ]; then CONTRIB_CELL="**Compare failed** — pinned \`$PREV_TAG\` missing upstream, no auto-assignment" elif [ "$ASSIGN_COUNT" -gt 0 ] && [ "$MENTION_COUNT" -gt 0 ]; then - CONTRIB_CELL="$ASSIGN_COUNT auto-assigned · $MENTION_COUNT mentioned below" + CONTRIB_CELL="$ASSIGN_COUNT review requested · $MENTION_COUNT mentioned below" elif [ "$ASSIGN_COUNT" -gt 0 ]; then - CONTRIB_CELL="$ASSIGN_COUNT auto-assigned (see sidebar)" + CONTRIB_CELL="$ASSIGN_COUNT review requested (see sidebar)" elif [ "$MENTION_COUNT" -gt 0 ]; then CONTRIB_CELL="$MENTION_COUNT mentioned below" else CONTRIB_CELL="none in release range" fi + # Folded into the same cell so the table keeps one + # contributor row; the names go on the cc line further down. + if [ -n "$FYI_LIST" ]; then + FYI_COUNT=$(printf '%s\n' "$FYI_LIST" | grep -oE '@[A-Za-z0-9_-]+' | wc -l | tr -d ' ') + CONTRIB_CELL="$CONTRIB_CELL · $FYI_COUNT not requested (no docs impact)" + else + FYI_COUNT=0 + fi + + # Owner cell. An unresolved owner means nobody is on the + # hook for merging, so it renders bold rather than blank. + if [ -n "$OWNER" ]; then + OWNER_CELL="@$OWNER" + if [ "$OWNER_ASSIGNED" != "true" ]; then + OWNER_CELL="$OWNER_CELL (couldn't set as assignee)" + fi + if [ -n "$OWNER_SOURCE" ]; then + OWNER_CELL="$OWNER_CELL - identified from $OWNER_SOURCE" + fi + else + OWNER_CELL="**Unresolved** - no human found for this release; a docs maintainer needs to adopt this PR" + fi + { echo "$START" echo "" @@ -1197,10 +1442,26 @@ jobs: echo "| **Hand-written changes** | $CHANGES_CELL |" echo "| **Reference assets** | $REFRESH_CELL |" echo "| **Gaps** | $GAPS_COUNT |" + echo "| **Owner** | $OWNER_CELL |" echo "| **Release contributors** | $CONTRIB_CELL |" echo "| **Action required** | $ACTION_REQUIRED |" echo "" + # ----- OWNERSHIP ----- + # Directly under the table and kept to three lines, so + # "who merges this" is answered before anything a reader + # might skim past. + echo "### Who does what" + echo "" + if [ -n "$OWNER" ]; then + echo "@$OWNER cut this release and owns this PR: review your own changes, chase the remaining approvals, and merge once they're in. You don't need to wait on a review from anyone listed as having no docs impact below." + else + echo "No release owner could be resolved automatically, so this PR has no assignee. A docs maintainer needs to adopt it, collect approvals, and merge." + fi + echo "" + echo "Everyone with a review request: the target is a review and approval within **2 business days**." + echo "" + # ----- SUMMARY OF CHANGES ----- # Renders either the skill's hand-written SUMMARY.md body # or the commit-title fallback from the signals step. @@ -1230,6 +1491,17 @@ jobs: echo "" fi + # ----- NO-DOCS-IMPACT CONTRIBUTORS (no review requested) ----- + # One line, not a per-person section. These contributors + # are absent from the reviewer sidebar by design, so this + # is the only place their involvement is recorded. + if [ "$FYI_COUNT" -gt 0 ]; then + echo "### No docs impact identified" + echo "" + echo "$FYI_LIST - your changes in this release didn't appear to affect the docs, so no review is requested and you're not blocking this PR. Please skim the diff anyway and comment if something of yours was missed or misjudged." + echo "" + fi + # ----- OVERFLOW CONTRIBUTORS (not auto-assigned) ----- # Skipped entirely when everyone who should review got # auto-assigned -- GitHub's sidebar covers that case. This diff --git a/.gitignore b/.gitignore index 9cfd3fb1..68175f85 100644 --- a/.gitignore +++ b/.gitignore @@ -28,6 +28,11 @@ yarn-error.log* /GAPS.md /NO_CHANGES.md /SUMMARY.md +/REVIEWERS.json + +# Release owner + contributor list written by upstream-release-docs.yml +# BEFORE the skill runs, and read by the skill to classify contributors. +/.release-meta.json .claude/scheduled_tasks.lock # Scratch workspace claude-code-action creates during skill runs. From 5d6e4c56d5b316a66f60fc69ba59bfc888fae4f2 Mon Sep 17 00:00:00 2001 From: Dan Barr <6922515+danbarr@users.noreply.github.com> Date: Mon, 24 Aug 2026 13:36:02 -0400 Subject: [PATCH 2/3] Harden the release-docs skill against known doc defects Auditing every autogen-docs PR since July 1: 38% needed a post-generation correction (60% for toolhive), via either a human commit or an @claude/@copilot fix request. The interventions cluster into four repeatable failure modes, three of which the skill can prevent. Each addition names the release that hit it, so the instruction reads as evidence rather than as generic caution. Premature documentation of experimental features, 3 interventions. The old guidance was a single bullet asking for `Hidden: true`. Replaced with an explicit readiness gate listing the signals to check in source (hidden commands, default-off flags, experimental naming, unserved CRD versions, upstream labels) and what to do when one fires: don't document, record the deferral in SUMMARY.md so the completeness pass reads it as a decision, and don't leave a "coming soon" note behind. Factual errors, 5 interventions. Three specific root causes now have their own verification rules: a PR body's claims about absence ("not wired up", "not yet supported") are the least reliable thing in it because a later commit in the same release silently falsifies them; a release removes user-facing surface as well as adding it, and removals never appear in a feature list; and command strings must trace to their actual registration rather than being inferred from sibling commands. Information architecture, 1 intervention. New pages now need at least one inbound prose link from a page readers reach first. A sidebars.ts entry satisfies navigation, not the journey. The fourth mode, editorial and dedup trims, is already what the Phase 5 docs-review pass is for. Co-Authored-By: Claude Opus 5 (1M context) --- .claude/skills/upstream-release-docs/SKILL.md | 51 ++++++++++++++++--- 1 file changed, 45 insertions(+), 6 deletions(-) diff --git a/.claude/skills/upstream-release-docs/SKILL.md b/.claude/skills/upstream-release-docs/SKILL.md index c3db93de..21adbd12 100644 --- a/.claude/skills/upstream-release-docs/SKILL.md +++ b/.claude/skills/upstream-release-docs/SKILL.md @@ -202,6 +202,8 @@ For each PR identified in Phase 1 (skip internal/infra unless user requests): 7. Note discrepancies between PR descriptions and actual code. Trust the code. + **A PR body describes the moment it was written, not the release.** This is a distinct failure from a PR body simply being wrong: the body was accurate when opened, then later commits, review changes, or a follow-up PR in the same release moved past it. `stacklok/docs-website#1037` documented a field as "not wired up" because an upstream PR note said so; by the time the release shipped it was wired up, and the note was months stale. A caveat, limitation, or "not yet supported" claim in a PR body is the single least trustworthy kind of statement you will read, because it is a claim about absence, and absence is exactly what a later commit silently fixes. Before writing any limitation into the docs, verify in source at the tag that the limitation still holds. If you cannot confirm it, leave it out rather than repeating it. + 8. **Deep-verify behavioral claims**: these are the most common source of documentation inaccuracy. For each feature, verify not just struct definitions but actual runtime behavior: - **API routes**: Check the actual route registration code (e.g., `r.Get`, `r.Post`, `r.Delete`), not just handler names. Docs often claim endpoints exist at paths where no handler is registered. - **Required fields**: Check validation code (e.g., `if field == ""` checks), not just struct definitions. A field present in a struct is not necessarily required; only fields checked in validation logic are enforced. @@ -209,15 +211,37 @@ For each PR identified in Phase 1 (skip internal/infra unless user requests): - **Precedence rules**: Read the actual `if/else` chain. For example, `if commit != "" { ... } else if branch != "" { ... } else if tag != "" { ... }` means commit > branch > tag, not commit > tag > branch. - **Delete/cleanup behavior**: Check whether the code reassigns pointers, cascades deletes, or leaves orphans. Delete behavior is frequently mis-documented. - **Query parameters**: Check whether parsed parameters are actually wired to the service layer and database queries. Parameters can be parsed from the URL but silently ignored if no service option or SQL filter exists for them. + - **Removed surface**: a release takes things away as well as adding them, and a removal is invisible in a release-notes list of features. Diff the previous tag against the new one for _deleted_ user-facing symbols (CRD fields, status subresources, flags, subcommands, routes, config keys) and grep the docs for every one you find. `stacklok/docs-website#1086` documented `status.referencingWorkloads` in the same release that removed it, because the PR that added the surrounding feature described only what it added. Treat every deletion in the diff as a docs defect until you have confirmed no page mentions it. + - **Command and subcommand strings**: never assemble a command from a pattern. Every command, subcommand, and flag you write into a page must be traced to its actual registration in source (`AddCommand`, flag registration, route table). `stacklok/docs-website#1095` documented `thv ai-plugin sync` and `thv ai-plugin upgrade` because sibling commands had them; neither existed. If you cannot point at the line that registers it, it does not go in the docs. - **Containment/authorization direction**: When documenting subset/superset checks, verify which argument is the caller and which is the resource. Getting the direction wrong produces examples that show the opposite of actual behavior. 9. Identify: - **Auto-generated content**: files generated from upstream (OpenAPI specs, CLI reference docs, JSON schemas). Do not manually edit these; flag them for automated update instead. However, auto-generated reference docs (e.g., API endpoints from a swagger spec) do **not** replace the need for conceptual explanations, guide content, or cross-references in existing pages. A new feature with auto-generated API docs still needs: (1) a conceptual explanation of what it is and why it exists, (2) mentions and cross-references in related existing pages (intro pages, feature lists, related guides), and (3) guide content if the feature has non-trivial workflows. Only skip creating a **duplicate API reference page**: never skip the surrounding documentation. - - **Hidden/experimental features**: look for indicators like `Hidden: true` in CLI command definitions, feature flags, or internal-only annotations. Do not document these unless the user explicitly asks. + - **Hidden, flag-gated, and experimental features**: see the readiness gate in step 11 below. This is the single most common over-documentation failure in this workflow, and the release notes will not warn you about it. 10. **Inventory the new public surface.** As you read the source and the regenerated reference assets, list every new or changed user-facing symbol the release introduces: CRD/struct fields, enum values, CLI flags and subcommands, env vars, config keys, and API routes. Most are already enumerated in the auto-synced reference assets (CLI `.md`, CRD `*.schema.json`, Swagger YAML) and the diff, so this is mostly transcription, not discovery. This list is the checklist the completeness pass in Phase 5 verifies against. It is the difference between documentation that is _accurate_ and documentation that is _complete_: a release can ship five new config fields, and a section that explains one of them correctly passes every accuracy check while silently omitting the other four. -11. **Classify the release contributors** (unattended mode only, when `.release-meta.json` is present). You have just read every PR's diff, which makes this the only point in the run where the classification is cheap and well-informed. For each login in `.release-meta.json`, decide whether their commits in the release range changed anything a reader can observe, and write `REVIEWERS.json` per the contract in [Artifacts](#artifacts-unattended-mode-only-written-at-repo-root). Do this from the diffs you read, not from commit messages. +11. **Apply the readiness gate before documenting anything.** A release ships code; it does not thereby declare that code ready for readers. Three separate releases leaked experimental features into published docs (skills `sync`/`upgrade` and the lockfile, Sigstore signing for skills, desktop theme customization), and each cost a reviewer round-trip to remove. The pattern is always the same: the PR is well-written and enthusiastic, the code is real and complete, and nothing in either says "not yet". Readiness lives in the code, not the narrative. + + For every feature you are about to document, check the source at the tag for these signals: + + - **CLI**: `Hidden: true` on the `cobra.Command`, or registration behind a build tag or an `if` on an env var / config flag. A hidden command is not user-facing no matter how complete it is. + - **Feature flags and gates**: a config key, env var, or CRD field that defaults to off/false and guards the code path. Search for the feature's entry point and read what has to be true for it to execute. + - **Naming**: `experimental`, `alpha`, `beta`, `preview`, `internal`, `unstable`, `wip`, or `x-` prefixes on packages, flags, config keys, API routes, or CRD fields. + - **API/CRD**: an unserved or unstored CRD version, a route registered only under an experimental prefix, or a field marked `// +optional` and absent from any example, defaulting, or validation path. + - **Upstream signals**: PR labels or titles carrying `experimental`/`do-not-document`, or a PR body that says the flag will be removed later. + + When any signal fires, **do not document the feature**. Instead: + + - Leave existing docs alone; don't add a section, a flag row, or a passing mention. + - Record it in `SUMMARY.md` as a one-line deferral ("Skipped `thv skill sync` - hidden command at `cmd/skill.go:NN`") so the reviewer can see you considered and rejected it rather than missed it. This is the same treatment as a conscious deferral in the Phase 5 completeness pass, and it is what keeps the completeness check from flagging it as a gap. + - Do not add a "coming soon" or "planned for a future release" note in its place. That is a change-log statement about an unreleased feature, and it goes stale on its own. + + The exception is a flag-gated change to something already documented, where the flag's existence is itself the reader-facing fact (an opt-in for new behavior a reader must deliberately enable). Document the flag and what enabling it does; don't document unflagged internals behind it. + + When a signal is ambiguous (a flag that defaults on, an `experimental` package name for a feature the release notes headline as shipped), document it and flag the call in `SUMMARY.md` so a reviewer can overrule you cheaply. Ambiguity resolves toward documenting; a fired signal does not. + +12. **Classify the release contributors** (unattended mode only, when `.release-meta.json` is present). You have just read every PR's diff, which makes this the only point in the run where the classification is cheap and well-informed. For each login in `.release-meta.json`, decide whether their commits in the release range changed anything a reader can observe, and write `REVIEWERS.json` per the contract in [Artifacts](#artifacts-unattended-mode-only-written-at-repo-root). Do this from the diffs you read, not from commit messages. ## Phase 3: Audit Existing Docs @@ -350,17 +374,27 @@ One exception: a breaking change or major behavioral change (a changed default, Any inventoried symbol that is neither documented nor deferred is a coverage gap: document it, or record why not. Do not let a symbol fall through silently. A section that documents a feature's happy path but omits its flags, enum values, or config knobs is incomplete even when every sentence in it is accurate. For large surfaces, spawn a coverage agent that takes the inventory and the changed/related doc files and returns, per symbol, "documented at `file:line`" or "not found." -3. **Build the site**: run the project's build command to check for broken links, missing references, or build errors. +3. **Check every new page for inbound links.** A page that exists but nothing links to is unreachable by navigation and breaks the journey the docs are organized around. `stacklok/docs-website#1086` shipped a new AI-plugins page with a sidebar entry and zero inbound links; a reviewer had to work out where it belonged in the CLI journey after the fact. + + For each page you created this run: + - Grep the docs set for its path. A sidebar entry in `sidebars.ts` is necessary but is **not** an inbound link; it satisfies navigation, not the journey. + - Require at least one inbound prose link from a page a reader plausibly reaches first: the section's introduction, the related how-to guide, or the concept page for the capability it extends. + - Add the outbound half too. The new page needs a "Next steps" section pointing 1-3 pages further along, so it isn't a dead end. + - Ask where in the journey phase order (install, use, secure, operate, optimize) the page sits, and link it from the phase before it. Placing a page in the sidebar without answering this is what produces an orphan with a correct-looking home. + + Treat a new page with no inbound prose link as a build failure you must fix, not a nit for the reviewer. + +4. **Build the site**: run the project's build command to check for broken links, missing references, or build errors. -4. **Run linting**: execute the project's lint/format commands. +5. **Run linting**: execute the project's lint/format commands. -5. **Run `/docs-review`**: invoke the docs-review skill on all changed and new files to catch style, structure, and clarity issues. When the review returns, **do not stop or present the findings to the user**. Instead, immediately apply every actionable fix yourself: +6. **Run `/docs-review`**: invoke the docs-review skill on all changed and new files to catch style, structure, and clarity issues. When the review returns, **do not stop or present the findings to the user**. Instead, immediately apply every actionable fix yourself: - For primary issues: edit the files to resolve them. - For secondary issues and inline suggestions: apply the fixes directly. - For items you disagree with (e.g., they conflict with verified source code): do not apply the suggestion, but briefly log each skipped item with a source-verified reason for auditability. - After applying fixes, re-run formatting/linting to ensure the fixes are clean. -6. Fix any remaining issues found in the build or lint steps. Re-run validation until clean. +7. Fix any remaining issues found in the build or lint steps. Re-run validation until clean. ## Phase 6: Handle Review Feedback @@ -389,6 +423,11 @@ When receiving review comments (from humans or automated reviewers): - **Flag gaps honestly**: if consumption tooling, client support, or integration isn't ready yet, say so explicitly rather than omitting the topic - **Use realistic examples**: guide pages need end-to-end examples with plausible data, exact commands, and expected output, not placeholder values - **Call out naming conventions**: when a feature introduces naming rules (casing, allowed characters, namespacing), document them explicitly with valid/invalid examples +- **Apply the readiness gate**: a release shipping code is not a declaration that readers should see it. Check for `Hidden: true`, default-off flags, and experimental naming in source before documenting anything; a fired signal means don't document, and record the deferral in `SUMMARY.md` so it reads as a decision rather than an omission (Phase 2 step 11) +- **Distrust claims about absence**: "not yet supported", "not wired up", and "planned" in a PR body are the least reliable statements you will read, because a later commit in the same release silently falsifies them. Verify every limitation in source at the tag before writing it into a page (Phase 2 step 7) +- **A release removes surface too**: deletions never appear in a feature list. Diff the tags for removed fields, flags, subcommands, and routes, and grep the docs for each one (Phase 2 step 8) +- **Never assemble a command from a pattern**: every command, subcommand, and flag in the docs must trace to its registration in source. Sibling commands having a subcommand is not evidence this one does (Phase 2 step 8) +- **No orphan pages**: a new page needs at least one inbound prose link from a page readers reach first. A `sidebars.ts` entry is navigation, not a journey (Phase 5 step 3) - **Don't document hidden features**: skip features marked as hidden, experimental, or internal unless explicitly asked - **Follow existing conventions**: match the project's style guide, writing voice, file structure, and naming patterns - **Be project-agnostic**: this workflow applies to any upstream project and any docs site. Do not assume specific frameworks, file paths, or tools. From b8b4112260245e3ee0f5bd3ce2ff79d70864097d Mon Sep 17 00:00:00 2001 From: Dan Barr <6922515+danbarr@users.noreply.github.com> Date: Mon, 24 Aug 2026 15:00:34 -0400 Subject: [PATCH 3/3] Address Copilot review on the ownership workflow Unify the bot filter. The contributor compare had its own copy of the bot regex, missing the `^app/` case that is_bot() carries. Both now go through is_bot(), which documents why two bot spellings are needed: the REST compare endpoint returns `renovate[bot]` while gh pr view and gh release view return `app/toolhive-release-app`. No behavior change. The REST compare endpoint only ever emits the `[bot]` suffix form, so the old filter already caught App identities there; verified the candidate list is byte-identical for toolhive v0.43.0 (13 contributors) and toolhive-studio v0.39.4. Fix an inaccurate claim in the Slack composer prompt. It described the owner as the PR assignee, which the workflow knowingly breaks: when GitHub rejects the assignment the owner is still named in the PR body's Owner row, which now reads as the authoritative source. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/autogen-docs-notify.yml | 13 ++++++++----- .github/workflows/upstream-release-docs.yml | 21 ++++++++++++++++----- 2 files changed, 24 insertions(+), 10 deletions(-) diff --git a/.github/workflows/autogen-docs-notify.yml b/.github/workflows/autogen-docs-notify.yml index a15261a1..33f5fbd3 100644 --- a/.github/workflows/autogen-docs-notify.yml +++ b/.github/workflows/autogen-docs-notify.yml @@ -202,11 +202,14 @@ jobs: PREFER reusing the summary content already there for your bullets; do not pad beyond it. - the OWNER: the login in the "Owner" row of the "At a - glance" table, which is also the PR's assignee - (`assignees[].login`). The owner cut the upstream - release and owns getting this PR merged. If the table - says the owner is unresolved and there is no assignee, - set "owner" to null. + glance" table. That row is the authoritative source. + The owner is usually also the PR's assignee, but not + always: when GitHub rejects the assignment the row + reads "(couldn't set as assignee)" and `assignees` is + empty, and the owner named in the row still stands. + The owner cut the upstream release and owns getting + this PR merged. Set "owner" to null only when the row + itself says the owner is unresolved. - the REQUESTED REVIEWERS (`reviewRequests[].login`). Exclude the owner from this list; the owner is reported separately. diff --git a/.github/workflows/upstream-release-docs.yml b/.github/workflows/upstream-release-docs.yml index f506efbf..612e3adb 100644 --- a/.github/workflows/upstream-release-docs.yml +++ b/.github/workflows/upstream-release-docs.yml @@ -461,9 +461,13 @@ jobs: PREV: ${{ steps.detect.outputs.prev_tag }} NEW: ${{ steps.detect.outputs.new_tag }} run: | - # Shared bot filter. Applied to every login this step - # resolves, from any of the three owner sources and from the - # contributor compare. + # Shared bot filter, the single definition used for every + # login this step resolves: all three owner sources and the + # contributor compare. Two bot spellings are in play and both + # must be caught: the REST compare endpoint returns + # `renovate[bot]`, while `gh pr view`/`gh release view` + # return `app/toolhive-release-app` for the same class of + # identity. is_bot() { printf '%s\n' "$1" | grep -qE '(\[bot\]$|^app/|^github-actions|^stacklokbot$|^dependabot|^renovate|^copilot)' } @@ -476,8 +480,15 @@ jobs: COMPARE="" COMPARE_OK=false fi - CANDIDATES=$(echo "$COMPARE" | - grep -Ev '(\[bot\]$|^github-actions|^stacklokbot$|^dependabot|^renovate|^copilot)' || true) + CANDIDATES="" + while IFS= read -r login; do + [ -z "$login" ] && continue + if is_bot "$login"; then + continue + fi + CANDIDATES="${CANDIDATES}${login}"$'\n' + done <<< "$COMPARE" + CANDIDATES="${CANDIDATES%$'\n'}" # ---------- owner ---------- OWNER=""