From 7e5f4d3ef5814645f38b7f96b4e90aad91196206 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Wed, 16 Sep 2026 06:47:33 +0000 Subject: [PATCH 1/7] docs(refresh-reviewers): add its own caller guide so the workflows_ref cross-check covers it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit refresh-reviewers.yml declares workflows_ref required-with-no-default but had no docs/callers page, so the BE-6508 docs cross-check derived a page name from its basename, found none, and silently skipped it. Give it its own guide and let the cross-check bind. - docs/callers/refresh-reviewers.md: new caller guide mirroring pr-risk.md's structure. Inputs table populated from the workflow's own workflow_call.inputs (window_months / top_k / floor / etc. — the real names), with the compliant no-default workflows_ref row. - docs/callers/README.md + README.md: index the new page (README's Setup cell was a bare em dash precisely because no guide existed). - check_workflow_pins.py + tests: reword the two comments that asserted refresh-reviewers is documented under assign-reviewers.md (a different reusable that does not declare workflows_ref) to a generic internal-workflow example, and add two DocsCrossCheckTests proving a reusable IS cross-checked via its own-name page. --- .github/workflow-pins/check_workflow_pins.py | 7 +- .../tests/test_check_workflow_pins.py | 27 ++++- README.md | 2 +- docs/callers/README.md | 1 + docs/callers/refresh-reviewers.md | 105 ++++++++++++++++++ 5 files changed, 137 insertions(+), 5 deletions(-) create mode 100644 docs/callers/refresh-reviewers.md diff --git a/.github/workflow-pins/check_workflow_pins.py b/.github/workflow-pins/check_workflow_pins.py index feb9b17e..ac608c53 100644 --- a/.github/workflow-pins/check_workflow_pins.py +++ b/.github/workflow-pins/check_workflow_pins.py @@ -3607,9 +3607,10 @@ def check_dir(workflows_dir, exempt=KNOWN_EXEMPT, docs_dir=None): base = name.rsplit(".", 1)[0] docs_path = os.path.join(docs_dir, base + ".md") # No page under this name = not documented as its own caller guide - # (e.g. refresh-reviewers.yml is covered by assign-reviewers.md), so - # there is nothing to cross-check. A page that EXISTS but lacks the - # row is the case that must not pass silently, below. + # (e.g. an internal bump-*/ci-*/test-* workflow, which legitimately + # have no docs/callers guide), so there is nothing to cross-check. A + # page that EXISTS but lacks the row is the case that must not pass + # silently, below. if os.path.isfile(docs_path): ann_docs_path = _ann_prop(docs_path) with open(docs_path, "r", encoding="utf-8", errors="replace") as f: diff --git a/.github/workflow-pins/tests/test_check_workflow_pins.py b/.github/workflow-pins/tests/test_check_workflow_pins.py index 00cbbbb5..b9ad1a52 100644 --- a/.github/workflow-pins/tests/test_check_workflow_pins.py +++ b/.github/workflow-pins/tests/test_check_workflow_pins.py @@ -5171,6 +5171,31 @@ def test_a_required_marker_row_is_clean(self): self.assertEqual(checked, ["pr-foo.yml"]) self.assertEqual(errors, [], errors) + def test_a_reusable_is_cross_checked_via_its_own_name_page(self): + # The derivation is the workflow's OWN basename, so a reusable whose + # name shares no prefix with any other guide is still cross-checked + # against its same-named page — a page claiming a `main` default fails. + # (This is the coverage refresh-reviewers.yml gained once it got its own + # docs/callers/refresh-reviewers.md page instead of being skipped.) + self._write_wf("refresh-foo.yml", _reusable(PINNED)) + self._write_doc("refresh-foo.md", _docs_page("main")) + errors, checked, _, _ = self._check() + self.assertEqual(checked, ["refresh-foo.yml"]) + self.assertEqual(len(errors), 1, errors) + self.assertIn("refresh-foo.md", errors[0]) + self.assertIn("line=%d" % _DOCS_ROW_LINE, errors[0]) + self.assertIn("refresh-foo.yml", errors[0]) # names the declaration + self.assertIn("BE-6508", errors[0]) + + def test_a_required_marker_row_on_its_own_name_page_is_clean(self): + # Companion to the above: the compliant `— (**required**)` marker on a + # reusable's own-name page passes. + self._write_wf("refresh-foo.yml", _reusable(PINNED)) + self._write_doc("refresh-foo.md", _docs_page("— (**required**)")) + errors, checked, _, _ = self._check() + self.assertEqual(checked, ["refresh-foo.yml"]) + self.assertEqual(errors, [], errors) + def test_a_page_missing_the_row_is_a_hard_error(self): self._write_wf("pr-foo.yml", _reusable(PINNED)) # Present page, real inputs table, but no `workflows_ref` row. @@ -5200,7 +5225,7 @@ def test_a_page_with_only_prose_and_a_fenced_example_is_a_hard_error(self): self.assertIn("no `workflows_ref` row", errors[0]) def test_a_missing_page_is_skipped_not_an_error(self): - # refresh-reviewers.yml is documented under assign-reviewers.md, so a + # An internal bump-*/ci-*/test-* workflow has no docs/callers guide, so a # page absent under the workflow's own name is "not applicable". self._write_wf("pr-foo.yml", _reusable(PINNED)) errors, checked, _, _ = self._check() diff --git a/README.md b/README.md index 0010f6cb..4bc0ee5f 100644 --- a/README.md +++ b/README.md @@ -30,7 +30,7 @@ complete, copy-pasteable caller. | [`public-repo-hygiene.yml`](.github/workflows/public-repo-hygiene.yml) | Fails a **public** repo's CI when its tracked files carry internal-only references, in three categories: ticket-style identifiers (`TEAM-1234`-shaped, a generic shape rather than a list of real team keys, with common tech acronyms like `SHA-256`/`AES-192` allowlisted and `ticket_allowlist:` to extend that list); internal collaboration-tool links (Notion, Slack archives/client, Google Docs/Drive, Datadog, PostHog projects, Linear, `incident-NNN`); and `Comfy-Org/` references outside a **default-deny** known-public allowlist, plus the `@Comfy-Org/` CODEOWNERS-handle case against a separate team allowlist. A lightweight regression guard, not a secrets scanner. Only tracked files are scanned (`git ls-files`), and for each one the scan covers its CONTENTS, its tracked PATH and (for a symlink) its target string — the same matcher and allowlists on all three, so a private repo name in a directory component is a finding just as it is in a line of prose; binaries are skipped for their contents but still have their path scanned; a tree with no git metadata is a hard config error (exit 2), never a silent pass. `exclude_paths` (newline-/comma-separated, `dir/` for a subtree or an exact file path, default empty) is the per-repo scanning scope — every entry is logged with its skipped-file count *including one that skipped nothing*, and a value naming the repo root is rejected (exit 2). Every other skip is loud too: an unreadable tracked file warns rather than vanishing, and a run that scanned zero files says so instead of reporting clean. **The known-public allowlist is deliberately NOT an input**: it lives with the checker in this repo, loaded from the `workflows_ref` SHA, so a PR in the caller repo cannot reach the checker or the allowlist *through this workflow's inputs* — the failure the per-repo copies had. What no reusable workflow can enforce from the inside is *which* `uses:` line runs (a PR that rewrites `uses:` **and** `workflows_ref` together runs a different workflow entirely), so protect the caller's `.github/workflows/` with a rule requiring a non-author approving review — see the setup guide. Hosting it here leaks nothing — by design it names public repos only. Fails with a non-zero exit + GitHub annotations so it wires in as a required status check. The checker lives in [`.github/public-repo-hygiene/`](.github/public-repo-hygiene) (pin `workflows_ref` to the same ref as `uses:`); no secrets required. | [public-repo-hygiene.md](docs/callers/public-repo-hygiene.md) | | [`coderabbit-config-validate.yml`](.github/workflows/coderabbit-config-validate.yml) | Validates the caller repo's `.coderabbit.yaml` against CodeRabbit's config schema, **on the PR that breaks it**. CodeRabbit rejects an invalid config *whole* — it discards the file and reviews with org-wide UI defaults, so every reviewer instruction, path filter and path instruction in it goes silently inert — and it validates the **base** branch, not the PR head, so the breakage first surfaces on the *next* PR, blamed on a change that did not cause it. Severity mirrors CodeRabbit's own behaviour: a YAML parse error, a `maxLength` violation or a type/enum error **fails**; an unknown/additional property **warns** (CodeRabbit strips those rather than rejecting the file, so the config loads while everything under the key silently does nothing) unless `strict_unknown_keys: true`. A repo with no `.coderabbit.yaml` passes, and says so. The schema is **vendored** at [`.github/coderabbit-config/`](.github/coderabbit-config) — no network fetch on the validation path, so an upstream schema change cannot redden the fleet without a reviewed PR here; `refresh-coderabbit-schema.yml` opens that PR weekly when upstream drifts. Setup: [docs/callers/coderabbit-config-validate.md](docs/callers/coderabbit-config-validate.md). Pin `workflows_ref` to the same ref as `uses:`; no secrets required. | | [`cursor-review-catalog-drift.yml`](.github/workflows/cursor-review-catalog-drift.yml) | **Internal to this repo — not `workflow_call`able.** Weekly (Mon 06:17 UTC) + `workflow_dispatch` drift check between the `cursor-review.yml` model pins and Cursor's live `cursor-agent models` catalog. Reads the pins *out of* `cursor-review.yml` (panel heredoc + `judge_model` default) rather than duplicating them, then reports: a **delisted pin** (urgent — consumer PRs are about to fail the review preflight; this also fails the run), a **pin whose catalog line now says NO-ZDR** (also urgent, and the quieter failure — nothing breaks, private review diffs just keep flowing to a model that may retain them), **unpinned same-lab catalog ids** (a review-me list, never an auto-recommendation — "newest highest-reasoning ZDR-eligible" is a human call, and NO-ZDR markers are surfaced verbatim; rendered one row per model *family*, newest first, since ids that differ only by a reasoning/speed tier are not separate candidates), **catalog ids from families the panel pins nothing from** (a quieter collapsed catch-all, never urgent — a lab is an id's first `-`/`.`-separated token, so a lab the panel *does* pin can only surface there once it rebrands under a new prefix, e.g. OpenAI's `o` series alongside `gpt-*`), and a **`last checked` audit date** older than 30 days (or future-dated). Findings land in one sticky issue (`[cursor-review catalog drift]`, label `cursor-review-catalog-drift`, updated in place, and closed only by a run that finds *no* drift at all — rare in practice, since a live catalog always offers unpinned tiers, so the issue's own footer states whether that report needs action now or is advisory) with the raw catalog folded in. Least privilege by construction: the job that pipes Cursor's installer into bash holds only `contents: read` and hands its rendered report to a separate `issues: write` job via an artifact. Comparison logic + tests live in [`.github/cursor-review/catalog-drift.py`](.github/cursor-review/catalog-drift.py). Requires `CURSOR_API_KEY`. | — | -| [`refresh-reviewers.yml`](.github/workflows/refresh-reviewers.yml) | Companion to `assign-reviewers.yml` — a scheduled drift-detector that recomputes the caller's `.github/reviewers.yml` from git history (recency-decayed commit touches per rule bucket, same glob semantics as the assigner, collaborators only, bots and generated/churn paths excluded) and opens ONE idempotent single-file PR when the committed map drifts. The rewrite is surgical (only the `reviewers:`/`default_pool:` lists change — comments preserved), the PR body carries per-rule before/after scores plus a report-only taxonomy-gap section, and a rule with too few qualifiers is left unchanged. Never a live mutator. Engine + knob rationale in [`.github/refresh-reviewers/`](.github/refresh-reviewers). Requires `vars.APP_ID` + `CLOUD_CODE_BOT_PRIVATE_KEY`, and `workflows_ref` (required) must pin the generator to the same SHA as `uses:`. | — | +| [`refresh-reviewers.yml`](.github/workflows/refresh-reviewers.yml) | Companion to `assign-reviewers.yml` — a scheduled drift-detector that recomputes the caller's `.github/reviewers.yml` from git history (recency-decayed commit touches per rule bucket, same glob semantics as the assigner, collaborators only, bots and generated/churn paths excluded) and opens ONE idempotent single-file PR when the committed map drifts. The rewrite is surgical (only the `reviewers:`/`default_pool:` lists change — comments preserved), the PR body carries per-rule before/after scores plus a report-only taxonomy-gap section, and a rule with too few qualifiers is left unchanged. Never a live mutator. Engine + knob rationale in [`.github/refresh-reviewers/`](.github/refresh-reviewers). Requires `vars.APP_ID` + `CLOUD_CODE_BOT_PRIVATE_KEY`, and `workflows_ref` (required) must pin the generator to the same SHA as `uses:`. | [refresh-reviewers.md](docs/callers/refresh-reviewers.md) | Per-workflow inputs and secrets are also documented in each workflow file's header comment; the setup guides above are the maintained, copy-pasteable version. diff --git a/docs/callers/README.md b/docs/callers/README.md index 60225d45..ca7f188d 100644 --- a/docs/callers/README.md +++ b/docs/callers/README.md @@ -17,6 +17,7 @@ per-workflow pages give you a complete, copy-pasteable caller: | `public-repo-hygiene.yml` | [public-repo-hygiene.md](public-repo-hygiene.md) | nothing | | `coderabbit-config-validate.yml` | [coderabbit-config-validate.md](coderabbit-config-validate.md) | nothing | | `assign-reviewers.yml` | [assign-reviewers.md](assign-reviewers.md) | `vars.APP_ID` + App key + `.github/reviewers.yml` | +| `refresh-reviewers.yml` | [refresh-reviewers.md](refresh-reviewers.md) | `vars.APP_ID` + App key + `.github/reviewers.yml` | | `assign-prs-to-author.yml` | [assign-prs-to-author.md](assign-prs-to-author.md) | nothing | | `stale.yml` | [stale.md](stale.md) | `SLACK_BOT_TOKEN` (optional) | | `detect-unreviewed-merge.yml` | [detect-unreviewed-merge.md](detect-unreviewed-merge.md) | `UNREVIEWED_MERGES_TOKEN` | diff --git a/docs/callers/refresh-reviewers.md b/docs/callers/refresh-reviewers.md new file mode 100644 index 00000000..ef3c3098 --- /dev/null +++ b/docs/callers/refresh-reviewers.md @@ -0,0 +1,105 @@ +# `refresh-reviewers.yml` — reviewer-map drift detector + +Read [the shared caller contract](README.md) first. + +## What it does + +Recomputes the caller repo's reviewer expertise map +(`.github/reviewers.yml`, the config [`assign-reviewers.yml`](assign-reviewers.md) +consumes) from git history and opens **one** idempotent single-file PR when the +committed map has drifted from reality. It is a drift **detector**, never a live +mutator: nothing is assigned and nothing merges — the whole deliverable is a +reviewable PR a human accepts or edits. + +Scoring is recency-decayed commit touches per rule bucket: per commit, per rule +glob matched by at least one surviving changed file, `score += 0.5^(age_days / +half_life_days)` and `touches += 1`. Line counts are intentionally unused. Bot +authors, generated/churn paths, and non-collaborators are excluded (collaborators +rather than org members, because `addAssignees` silently drops non-collaborators). +The rewrite is surgical — only the `reviewers: [...]` / `default_pool: [...]` +lists change, every comment is preserved — and a rule that cannot reach its floor +of qualifiers is left unchanged. + +Runs are idempotent: each re-run force-resets the same `pr_branch` from the +default branch and edits the one open drift PR in place, so duplicate PRs never +stack. A drift branch a human has pushed to is never force-reset, and a no-drift +run closes a stale still-bot-authored drift PR so an obsolete proposal cannot +linger mergeable. + +## Prerequisites + +| | | +|---|---| +| `vars.APP_ID` | **Required.** CLOUD_CODE_BOT app id (same app as `assign-reviewers.yml`). | +| `secrets.CLOUD_CODE_BOT_PRIVATE_KEY` | **Required.** The drift-PR branch push and PR open/edit are made by the app (`contents: write` + `pull-requests: write`), so the workflow's own token stays read-only. | +| `.github/reviewers.yml` in **your** repo | **Required.** The expertise map the generator rewrites. | + +## Caller + +`.github/workflows/refresh-reviewers.yml`, e.g. on a weekly schedule: + +```yaml +name: CI - Refresh Reviewers + +on: + schedule: + - cron: '0 6 * * 1' # Mondays 06:00 UTC + workflow_dispatch: {} + +jobs: + refresh: + permissions: + contents: read + uses: Comfy-Org/github-workflows/.github/workflows/refresh-reviewers.yml@ + with: + workflows_ref: + map_exclude: some-operator-login + secrets: + CLOUD_CODE_BOT_PRIVATE_KEY: ${{ secrets.CLOUD_CODE_BOT_PRIVATE_KEY }} +``` + +## Required permissions + +```yaml +contents: read +``` + +Every mutation (branch push, PR open/edit) goes through the App token, so the +calling job needs only `contents: read`. + +## Inputs + +| Input | Default | Notes | +|---|---|---| +| `reviewer_config_path` | `.github/reviewers.yml` | Path in the caller repo to the expertise/path-glob reviewer config (same meaning as in `assign-reviewers.yml`). | +| `window_months` | `12` | How many months of git history to score. | +| `half_life_days` | `90` | Decay half-life in days for commit recency weighting. | +| `top_k` | `4` | Max experts per rule. | +| `floor` | `2` | Min experts per rule; below-threshold candidates backfill up to this, and a rule that still can't reach it is left unchanged. | +| `min_touches` | `5` | Raw commit-touches needed to qualify for a rule. | +| `min_score` | `1.5` | Decayed score needed to qualify for a rule. | +| `floor_min_touches` | `2` | Relaxed touch threshold used only for floor backfill. | +| `map_exclude` | `''` | Whitespace-separated logins never to place in the map (distinct from the runtime `vars.REVIEWER_EXCLUDE` — e.g. an operator login whose commits are largely agent-authored). | +| `extra_exclude_paths` | `''` | Newline-separated regexes appended to the built-in generated/churn path exclusion list. | +| `pr_branch` | `bot/refresh-reviewers` | Head branch for the drift PR (reset + reused across runs). Must never equal the default branch (hard error). | +| `workflows_ref` | — (**required**) | Pin to the SAME full commit SHA as `uses:`. No default on purpose: a floating default would let a caller SHA-pin `uses:` and still load the generator from a moving ref. | + +## Gotchas + +**`map_exclude` is not `vars.REVIEWER_EXCLUDE`.** `map_exclude` controls who may +appear in the **committed** map; `vars.REVIEWER_EXCLUDE` (read by +`assign-reviewers.yml`) controls who the assigner skips at PR time. Seed +`map_exclude` with operator logins whose commits are largely agent-authored — +their commit volume is not personal expertise. + +**`workflows_ref` accepts any non-empty ref but warns on a non-SHA.** Like +`cursor-review`, `pr-size`, `agents-md-integrity` and +`coderabbit-config-validate`, this workflow `::warning::`s a `workflows_ref` that +is not a full 40-hex commit SHA (branches and tags are mutable and can skew +between jobs mid-run) and fails only on an empty or omitted value. Pin it to the +same SHA as `uses:` — the generator is loaded from that ref at run time, never +from your checkout, so a caller-side change can't rewrite the logic scoring it. + +**No fork concern — it is not PR-triggered.** The workflow runs on a schedule or +manual dispatch, not on `pull_request`, so the fork/Dependabot empty-secret +pitfall that affects `assign-reviewers.yml` does not apply here. From fc9f7ac3dd01fbcc857add10c8a4c3ef9e59edbf Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Wed, 16 Sep 2026 07:16:16 +0000 Subject: [PATCH 2/7] docs(refresh-reviewers): tighten guide accuracy from cursor-review panel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address the cursor-review threads on the new caller guide and the adjacent catalog/comment edits (all doc/comment accuracy, no behavior): - Scope the 'never force-reset' guarantee to a branch with an OPEN bot drift PR — the guard keys on that; a closed/merged/retargeted PR or a pre-PR branch is reset from the default branch. - pr_branch: warn it is force-reset+force-pushed every run, so it must name a throwaway branch (default-branch equality is the only rejected value; every other pre-existing branch is overwritten). - Qualify 'every comment is preserved': an inline trailing comment on a block-form list item being rewritten is not carried over. - top_k is a target cap only when floor <= top_k (inputs unvalidated). - reviewers.yml absent = silent green no-op, not a hard failure. - Fork claim scoped to the recommended schedule/dispatch caller. - Note there is no auto-bump fleet yet (pin bumped by hand) + record refresh-reviewers.yml in the deliberate-no-fleet table. - docs/callers/README.md: refresh-reviewers now in the concurrency and scheduled/dispatchable-callers lists. - check_workflow_pins.py + its test: correct the silent-skip comment — this branch is only reachable for a declared-no-default reusable with no own-name guide, not for bump-*/ci-*/test-* files. Co-Authored-By: Claude Opus 4.8 --- .github/bump-callers/README.md | 1 + .github/workflow-pins/check_workflow_pins.py | 14 ++++--- .../tests/test_check_workflow_pins.py | 5 ++- docs/callers/README.md | 9 ++-- docs/callers/refresh-reviewers.md | 42 +++++++++++++------ 5 files changed, 48 insertions(+), 23 deletions(-) diff --git a/.github/bump-callers/README.md b/.github/bump-callers/README.md index e64a87e1..4733636c 100644 --- a/.github/bump-callers/README.md +++ b/.github/bump-callers/README.md @@ -88,6 +88,7 @@ the filenames literally) and `cursor-review` does not. |---|---|---| | `stale.yml` | 0 | Nothing to bump. Add a fleet when the first caller lands. | | `assign-prs-to-author.yml` | 0 | Same. | +| `refresh-reviewers.yml` | 0 | Ships a consumer caller guide but no external caller has enrolled yet. Add a fleet + `REFRESH_REVIEWERS_CALLERS` roster once the first one lands; until then a consumer's `uses:`/`workflows_ref` pin is bumped by hand. | A reusable that has callers but no fleet is the trap this whole directory exists to prevent: the pins simply never move, so consumers drift behind indefinitely diff --git a/.github/workflow-pins/check_workflow_pins.py b/.github/workflow-pins/check_workflow_pins.py index ac608c53..219987c8 100644 --- a/.github/workflow-pins/check_workflow_pins.py +++ b/.github/workflow-pins/check_workflow_pins.py @@ -3606,11 +3606,15 @@ def check_dir(workflows_dir, exempt=KNOWN_EXEMPT, docs_dir=None): if not defaults: base = name.rsplit(".", 1)[0] docs_path = os.path.join(docs_dir, base + ".md") - # No page under this name = not documented as its own caller guide - # (e.g. an internal bump-*/ci-*/test-* workflow, which legitimately - # have no docs/callers guide), so there is nothing to cross-check. A - # page that EXISTS but lacks the row is the case that must not pass - # silently, below. + # We only reach here for a reusable that DECLARES workflows_ref with + # no default (`not defaults`); bump-*/ci-*/test-* files don't declare + # the input at all and were skipped far above. So no page under this + # name means this reusable ships no own-name caller guide, and there + # is no row to cross-check — the lint's one blind spot (a declared- + # no-default reusable with no guide is not covered). That gap is + # exactly why refresh-reviewers.yml needed its own docs/callers page + # instead of being skipped here. A page that EXISTS but lacks the row + # is the case that must not pass silently, below. if os.path.isfile(docs_path): ann_docs_path = _ann_prop(docs_path) with open(docs_path, "r", encoding="utf-8", errors="replace") as f: diff --git a/.github/workflow-pins/tests/test_check_workflow_pins.py b/.github/workflow-pins/tests/test_check_workflow_pins.py index b9ad1a52..acec4f33 100644 --- a/.github/workflow-pins/tests/test_check_workflow_pins.py +++ b/.github/workflow-pins/tests/test_check_workflow_pins.py @@ -5225,8 +5225,9 @@ def test_a_page_with_only_prose_and_a_fenced_example_is_a_hard_error(self): self.assertIn("no `workflows_ref` row", errors[0]) def test_a_missing_page_is_skipped_not_an_error(self): - # An internal bump-*/ci-*/test-* workflow has no docs/callers guide, so a - # page absent under the workflow's own name is "not applicable". + # A reusable that declares workflows_ref-no-default but ships no own-name + # docs/callers guide: a page absent under the workflow's own name is "not + # applicable" and skipped (the lint's blind spot — nothing to cross-check). self._write_wf("pr-foo.yml", _reusable(PINNED)) errors, checked, _, _ = self._check() self.assertEqual(checked, ["pr-foo.yml"]) diff --git a/docs/callers/README.md b/docs/callers/README.md index ca7f188d..315d5cbc 100644 --- a/docs/callers/README.md +++ b/docs/callers/README.md @@ -199,8 +199,9 @@ with: ## Concurrency -Check whether the reusable already declares a `concurrency` group. Today only -`groom.yml` does (`groom-${{ github.repository }}`). +Check whether the reusable already declares a `concurrency` group. Today +`groom.yml` (`groom-${{ github.repository }}`) and `refresh-reviewers.yml` +(`refresh-reviewers-${{ github.repository }}`, `cancel-in-progress: false`) do. **If it does, do not declare the same group in your caller.** The caller holds the group while waiting for its `uses:` job, which is waiting to acquire the same @@ -266,8 +267,8 @@ gh workflow list --repo / gh run list --repo / --workflow .yml --limit 1 ``` -**Scheduled / dispatchable callers** — `groom`, `stale`, `assign-prs-to-author`. -These declare `workflow_dispatch`, so you can fire them by hand: +**Scheduled / dispatchable callers** — `groom`, `stale`, `assign-prs-to-author`, +`refresh-reviewers`. These declare `workflow_dispatch`, so you can fire them by hand: ```bash gh workflow run .yml --repo / diff --git a/docs/callers/refresh-reviewers.md b/docs/callers/refresh-reviewers.md index ef3c3098..957c85ba 100644 --- a/docs/callers/refresh-reviewers.md +++ b/docs/callers/refresh-reviewers.md @@ -17,14 +17,20 @@ half_life_days)` and `touches += 1`. Line counts are intentionally unused. Bot authors, generated/churn paths, and non-collaborators are excluded (collaborators rather than org members, because `addAssignees` silently drops non-collaborators). The rewrite is surgical — only the `reviewers: [...]` / `default_pool: [...]` -lists change, every comment is preserved — and a rule that cannot reach its floor -of qualifiers is left unchanged. +lists change and everything else is kept byte-for-byte — and a rule that cannot +reach its floor of qualifiers is left unchanged. (One caveat: an inline trailing +comment on a *block-form list item* being rewritten — `- alice # rationale` — +is not carried over when that list's membership changes; keep per-reviewer +rationales as their own comment lines, not inline.) Runs are idempotent: each re-run force-resets the same `pr_branch` from the default branch and edits the one open drift PR in place, so duplicate PRs never -stack. A drift branch a human has pushed to is never force-reset, and a no-drift -run closes a stale still-bot-authored drift PR so an obsolete proposal cannot -linger mergeable. +stack. While an **open** drift PR's tip is human-authored, that branch is left +untouched (never force-reset) — but the guard keys on an open *bot* PR: if that +PR is closed, merged or retargeted, or the branch pre-dates any PR, the next run +resets it from the default branch, so only push to it while its bot PR is open. +A no-drift run closes a stale still-bot-authored drift PR so an obsolete proposal +cannot linger mergeable. ## Prerequisites @@ -32,7 +38,7 @@ linger mergeable. |---|---| | `vars.APP_ID` | **Required.** CLOUD_CODE_BOT app id (same app as `assign-reviewers.yml`). | | `secrets.CLOUD_CODE_BOT_PRIVATE_KEY` | **Required.** The drift-PR branch push and PR open/edit are made by the app (`contents: write` + `pull-requests: write`), so the workflow's own token stays read-only. | -| `.github/reviewers.yml` in **your** repo | **Required.** The expertise map the generator rewrites. | +| `.github/reviewers.yml` in **your** repo | **Required** for any effect. The expertise map the generator rewrites. Absent, the run is a silent green no-op (nothing recomputed, no PR opened), so a scheduled caller with no config passes weekly while doing nothing — create the file before trusting the schedule. | ## Caller @@ -74,14 +80,14 @@ calling job needs only `contents: read`. | `reviewer_config_path` | `.github/reviewers.yml` | Path in the caller repo to the expertise/path-glob reviewer config (same meaning as in `assign-reviewers.yml`). | | `window_months` | `12` | How many months of git history to score. | | `half_life_days` | `90` | Decay half-life in days for commit recency weighting. | -| `top_k` | `4` | Max experts per rule. | -| `floor` | `2` | Min experts per rule; below-threshold candidates backfill up to this, and a rule that still can't reach it is left unchanged. | +| `top_k` | `4` | Target cap on experts per rule. This holds only when `floor <= top_k`; the two inputs are not validated against each other, so a `floor` set higher (or `top_k: 0`) backfills past `top_k`. | +| `floor` | `2` | Min experts per rule; below-threshold candidates backfill up to this, and a rule that still can't reach it is left unchanged. Keep `floor <= top_k`. | | `min_touches` | `5` | Raw commit-touches needed to qualify for a rule. | | `min_score` | `1.5` | Decayed score needed to qualify for a rule. | | `floor_min_touches` | `2` | Relaxed touch threshold used only for floor backfill. | | `map_exclude` | `''` | Whitespace-separated logins never to place in the map (distinct from the runtime `vars.REVIEWER_EXCLUDE` — e.g. an operator login whose commits are largely agent-authored). | | `extra_exclude_paths` | `''` | Newline-separated regexes appended to the built-in generated/churn path exclusion list. | -| `pr_branch` | `bot/refresh-reviewers` | Head branch for the drift PR (reset + reused across runs). Must never equal the default branch (hard error). | +| `pr_branch` | `bot/refresh-reviewers` | Head branch for the drift PR: **force-reset from the default branch and force-pushed on every run**, so it must name a throwaway branch dedicated to this workflow — never a real branch (`dev`, `release/*`, …) whose history you keep. Equality with the default branch is a hard error, but that is the *only* value the workflow itself rejects; every other pre-existing branch named here is silently overwritten. | | `workflows_ref` | — (**required**) | Pin to the SAME full commit SHA as `uses:`. No default on purpose: a floating default would let a caller SHA-pin `uses:` and still load the generator from a moving ref. | ## Gotchas @@ -100,6 +106,18 @@ between jobs mid-run) and fails only on an empty or omitted value. Pin it to the same SHA as `uses:` — the generator is loaded from that ref at run time, never from your checkout, so a caller-side change can't rewrite the logic scoring it. -**No fork concern — it is not PR-triggered.** The workflow runs on a schedule or -manual dispatch, not on `pull_request`, so the fork/Dependabot empty-secret -pitfall that affects `assign-reviewers.yml` does not apply here. +**No fork concern with the recommended caller.** The caller shown above runs on +a schedule / manual dispatch, not on `pull_request`, so the fork/Dependabot +empty-secret pitfall that affects `assign-reviewers.yml` does not apply. The +trigger is the caller's to choose, though — this reusable is `on: workflow_call` +only, so wiring it to a `pull_request`/`pull_request_target` caller reintroduces +that pitfall *and* runs the App token's `contents: write` + `pull-requests: +write` in a fork-influenced context. Keep it on schedule / dispatch. + +**No auto-bump fleet yet — bump the pin by hand.** There is no +`bump-refresh-reviewers-callers.yml` fleet or `REFRESH_REVIEWERS_CALLERS` roster +(it is recorded in the [deliberate-no-fleet table](../../.github/bump-callers/README.md#reusables-with-no-fleet--deliberate-not-an-oversight)), +so a consumer's `uses:` + `workflows_ref` pins do **not** move automatically — +bump both to a newer SHA by hand, or ask a maintainer to stand up a fleet once +several repos enrol. Until then an un-bumped pin drifts indefinitely, the exact +trap the [shared caller contract](README.md#staying-current) warns about. From 1bbe0e7b164eb58110b360aba8efb45d74280d96 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Wed, 16 Sep 2026 13:51:19 +0000 Subject: [PATCH 3/7] test(refresh-reviewers): pin workflow_call inputs against the docs/callers Inputs table Two-way, stdlib-only drift check between on.workflow_call.inputs in .github/workflows/refresh-reviewers.yml and the ## Inputs table in docs/callers/refresh-reviewers.md, modeled on the cursor-review panel's test_workflow_inputs_docs.py. Guards the BE-4691 failure mode (a phantom documented input is a zero-job startup_failure for a copying caller; a deleted-but-documented or added-but-undocumented input drifts silently) for the new refresh-reviewers caller guide, which had no such pin. Two sets only: the .github/refresh-reviewers/README.md knob table is intentionally partial and uses combined-cell rows, so it is deliberately not a third set. Both scans are derived, not hardcoded, and each is sanity-guarded (non-empty and contains workflows_ref) so a silenced parser fails loudly instead of comparing two empty sets. Auto-discovered by test-refresh-reviewers.yml; no CI wiring change. --- .../tests/test_inputs_docs.py | 135 ++++++++++++++++++ 1 file changed, 135 insertions(+) create mode 100644 .github/refresh-reviewers/tests/test_inputs_docs.py diff --git a/.github/refresh-reviewers/tests/test_inputs_docs.py b/.github/refresh-reviewers/tests/test_inputs_docs.py new file mode 100644 index 00000000..e6d37c02 --- /dev/null +++ b/.github/refresh-reviewers/tests/test_inputs_docs.py @@ -0,0 +1,135 @@ +#!/usr/bin/env python3 +"""Two-way drift check: declared workflow inputs vs the documented ones. + +The same loud alarm BE-4691 asked for, applied to the refresh-reviewers caller +guide. `cursor-review.yml`'s `blocking:` input (and its whole gate job) was +deleted by accident in #31 while its documentation lived on in three places for +weeks — a phantom input in the docs is a broken caller for whoever copies it, +because GitHub rejects an unknown `workflow_call` input at startup with a +zero-job `startup_failure` and no logs. The reverse drift is quieter but real +too: an input added to the workflow and documented nowhere is a knob nobody can +discover. The refresh-reviewers guide had no such pin until now. + +So this test pins set equality between: + +* `on.workflow_call.inputs` in `.github/workflows/refresh-reviewers.yml`, and +* the "Inputs" table in `docs/callers/refresh-reviewers.md`. + +This is deliberately a TWO-set check. `.github/refresh-reviewers/README.md`'s +"Knob defaults (and why)" table is intentionally partial (it omits +`reviewer_config_path`, `map_exclude`, `extra_exclude_paths` and `workflows_ref`) +and uses combined-cell rows (`| `top_k` / `floor` |`), so the bare-name table +regex neither captures nor should capture it — including it would make this test +permanently red or vacuous. + +Deliberately parsed WITHOUT PyYAML, like the cursor-review model +(test_workflow_inputs_docs.py): this repo is stdlib-only and CI installs no +requirements. The workflow is uniformly 2-space indented and every input key +sits alone on its 6-space line, which is all the scanners below need — and each +scanner's result is sanity-checked (non-empty, contains `workflows_ref`) so a +parser gone quiet fails instead of passing vacuously. + +Run: python3 .github/refresh-reviewers/tests/test_inputs_docs.py +""" + +import os +import re +import unittest + +HERE = os.path.dirname(os.path.abspath(__file__)) +WORKFLOW = os.path.normpath( + os.path.join(HERE, "..", "..", "workflows", "refresh-reviewers.yml") +) +SETUP_GUIDE = os.path.normpath( + os.path.join(HERE, "..", "..", "..", "docs", "callers", "refresh-reviewers.md") +) + +# An input declaration: the key alone on its 6-space line, directly under +# ` inputs:`. Sub-keys of an input (description/type/default) are 8-space, +# and folded description text deeper still, so none of them match. +INPUT_KEY = re.compile(r"^ ([A-Za-z0-9_-]+):\s*$") +# A table row's first cell: `| `name` | ...`. +TABLE_KEY = re.compile(r"^\|\s*`([A-Za-z0-9_-]+)`\s*\|") +HEADING = re.compile(r"^#{2,3}\s") + + +def read_lines(path): + with open(path, encoding="utf-8") as f: + return f.read().split("\n") + + +def workflow_inputs(): + """Input names declared under on.workflow_call.inputs.""" + lines = read_lines(WORKFLOW) + # Constrain to the pre-`jobs:` header so a 6-space key inside some job's + # step mapping can never register as an input. + head = lines[: lines.index("jobs:")] + names, in_inputs = set(), False + for line in head: + if line == " inputs:": + in_inputs = True + continue + if in_inputs and re.match(r"^ \S", line): # dedent: secrets:, etc. + break + if in_inputs: + match = INPUT_KEY.match(line) + if match: + names.add(match.group(1)) + return names + + +def documented_inputs(path, heading): + """First-cell backticked names of the table under `heading` in `path`.""" + names, in_section = set(), False + for line in read_lines(path): + if line.strip() == heading: + in_section = True + continue + if in_section and HEADING.match(line): + break + if in_section: + match = TABLE_KEY.match(line) + if match: + names.add(match.group(1)) + return names + + +class RefreshReviewersInputsDocsTest(unittest.TestCase): + def setUp(self): + self.declared = workflow_inputs() + self.documented = documented_inputs(SETUP_GUIDE, "## Inputs") + # Guard the parsers: if either scanner silently stopped matching, the + # equality assertions below would compare empty sets and pass. + for label, found in ( + ("workflow declaration", self.declared), + ("setup guide inputs table", self.documented), + ): + self.assertIn( + "workflows_ref", + found, + f"the {label} scanner lost `workflows_ref` — parser or file " + "structure changed, every assertion here is now vacuous", + ) + + def test_every_documented_input_is_declared(self): + # The #31 failure mode: docs outliving a deleted input. A caller who + # copies a phantom input gets a zero-job startup_failure with no logs. + phantom = self.documented - self.declared + self.assertFalse( + phantom, + "`docs/callers/refresh-reviewers.md` inputs table documents inputs " + f"refresh-reviewers.yml does not declare: {sorted(phantom)} — " + "deleting an input is a docs change too", + ) + + def test_every_declared_input_is_documented(self): + missing = self.declared - self.documented + self.assertFalse( + missing, + "refresh-reviewers.yml declares inputs missing from " + f"`docs/callers/refresh-reviewers.md` inputs table: {sorted(missing)}", + ) + + +if __name__ == "__main__": + unittest.main(verbosity=2) From 2321bdc284f1022f5e173c5a6bbac47f9a168075 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Wed, 16 Sep 2026 14:07:28 +0000 Subject: [PATCH 4/7] test(refresh-reviewers): run the drift test on the files it parses; harden its scanners Address cursor-review panel on #298. - High: test-refresh-reviewers.yml path-filtered only on .github/refresh-reviewers/** + itself, so a PR editing refresh-reviewers.yml's inputs or the guide's Inputs table ran zero jobs and the drift test never tripped. Add both parsed files to the pull_request and push paths filters, mirroring the cursor-review analogue. - Medium: skip blank/comment lines in the workflow input scan so a 4-space comment inside inputs: cannot trip the dedent break and silently truncate the scan. - Nit: HEADING now matches h4+ (^#{2,}) so a #### subsection under ## Inputs terminates the row scan. - Nit: read_lines tolerates CRLF; workflow_inputs aborts with an explanatory message when top-level jobs: is absent. Co-Authored-By: Claude Opus 4.8 --- .../refresh-reviewers/tests/test_inputs_docs.py | 17 +++++++++++++++-- .github/workflows/test-refresh-reviewers.yml | 4 ++++ 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/.github/refresh-reviewers/tests/test_inputs_docs.py b/.github/refresh-reviewers/tests/test_inputs_docs.py index e6d37c02..1659c471 100644 --- a/.github/refresh-reviewers/tests/test_inputs_docs.py +++ b/.github/refresh-reviewers/tests/test_inputs_docs.py @@ -50,12 +50,15 @@ INPUT_KEY = re.compile(r"^ ([A-Za-z0-9_-]+):\s*$") # A table row's first cell: `| `name` | ...`. TABLE_KEY = re.compile(r"^\|\s*`([A-Za-z0-9_-]+)`\s*\|") -HEADING = re.compile(r"^#{2,3}\s") +HEADING = re.compile(r"^#{2,}\s") def read_lines(path): + # Tolerate CRLF: split on \n and drop a trailing \r so indent-anchored + # matches (dedent break, INPUT_KEY) and `lines.index("jobs:")` are not + # thrown off by a stray carriage return. with open(path, encoding="utf-8") as f: - return f.read().split("\n") + return [line.rstrip("\r") for line in f.read().split("\n")] def workflow_inputs(): @@ -63,12 +66,22 @@ def workflow_inputs(): lines = read_lines(WORKFLOW) # Constrain to the pre-`jobs:` header so a 6-space key inside some job's # step mapping can never register as an input. + if "jobs:" not in lines: + raise AssertionError( + f"no top-level `jobs:` line in {WORKFLOW} — file moved or its " + "structure changed; the input scan cannot be bounded" + ) head = lines[: lines.index("jobs:")] names, in_inputs = set(), False for line in head: if line == " inputs:": in_inputs = True continue + if in_inputs and (not line.strip() or line.lstrip().startswith("#")): + # Blank or comment lines carry no indentation signal: a 4-space + # comment is legal YAML inside `inputs:` and must not trip the + # dedent break and silently truncate the scan. + continue if in_inputs and re.match(r"^ \S", line): # dedent: secrets:, etc. break if in_inputs: diff --git a/.github/workflows/test-refresh-reviewers.yml b/.github/workflows/test-refresh-reviewers.yml index 42fafff1..6eaafba6 100644 --- a/.github/workflows/test-refresh-reviewers.yml +++ b/.github/workflows/test-refresh-reviewers.yml @@ -10,12 +10,16 @@ on: pull_request: paths: - '.github/refresh-reviewers/**' + - '.github/workflows/refresh-reviewers.yml' - '.github/workflows/test-refresh-reviewers.yml' + - 'docs/callers/refresh-reviewers.md' push: branches: [main] paths: - '.github/refresh-reviewers/**' + - '.github/workflows/refresh-reviewers.yml' - '.github/workflows/test-refresh-reviewers.yml' + - 'docs/callers/refresh-reviewers.md' permissions: contents: read From f6d5156605f746c13d8da4c0d45f9b6fbdea43df Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Wed, 16 Sep 2026 14:26:19 +0000 Subject: [PATCH 5/7] test(refresh-reviewers): extend drift check to caller with: blocks, README knobs, and the Default column MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follows the two name-only input/docs pins in this file with three extensions covering the surfaces a consumer actually copies: 1. Fenced/comment `with:` blocks (guide ## Caller, workflow header comment) must be a SUBSET of the declared inputs — a phantom key there is a zero-job startup_failure for whoever copies the example, the #31 failure one level in from the Inputs table. 2. README "Knob defaults (and why)" phantom-only check: assert documented - declared == set() (the table is partial by design and uses combined-cell rows the TABLE_KEY regex correctly skips, so no set-equality). 3. Pin the guide Inputs table's Default column to the workflow's real `default:`, with the `workflows_ref` required-no-default special case. Each new scanner is guarded non-empty / still finding workflows_ref, matching the existing setUp, so a parser gone quiet fails instead of passing vacuously. Stdlib only. --- .../tests/test_inputs_docs.py | 260 ++++++++++++++++++ 1 file changed, 260 insertions(+) diff --git a/.github/refresh-reviewers/tests/test_inputs_docs.py b/.github/refresh-reviewers/tests/test_inputs_docs.py index 1659c471..06254fae 100644 --- a/.github/refresh-reviewers/tests/test_inputs_docs.py +++ b/.github/refresh-reviewers/tests/test_inputs_docs.py @@ -43,6 +43,7 @@ SETUP_GUIDE = os.path.normpath( os.path.join(HERE, "..", "..", "..", "docs", "callers", "refresh-reviewers.md") ) +README = os.path.normpath(os.path.join(HERE, "..", "README.md")) # An input declaration: the key alone on its 6-space line, directly under # ` inputs:`. Sub-keys of an input (description/type/default) are 8-space, @@ -51,6 +52,15 @@ # A table row's first cell: `| `name` | ...`. TABLE_KEY = re.compile(r"^\|\s*`([A-Za-z0-9_-]+)`\s*\|") HEADING = re.compile(r"^#{2,}\s") +# An input's `default:` / `required:` sub-keys — 8-space, one level under the +# 6-space input key — used to pin the guide's Default column to the real value. +INPUT_DEFAULT = re.compile(r"^ default:\s*(.*?)\s*$") +INPUT_REQUIRED = re.compile(r"^ required:\s*(\S+)\s*$") +# A `with:` mapping in an example caller (at any indent), and a mapping key +# directly under it. A placeholder scalar VALUE (``, a login) never +# starts a line, so MAPPING_KEY never mistakes one for an input. +WITH_LINE = re.compile(r"^(\s*)with:\s*$") +MAPPING_KEY = re.compile(r"^\s*([A-Za-z0-9_-]+):") def read_lines(path): @@ -107,6 +117,165 @@ def documented_inputs(path, heading): return names +def strip_comment_prefix(line): + """Drop the leading `# ` from a full-line YAML comment while preserving the + example's own indentation, so relative-indent parsing still works. A bare + `#` becomes empty.""" + if line.startswith("# "): + return line[2:] + if line == "#": + return "" + return line + + +def fenced_blocks(lines): + """Content lines of every ``` fenced code block, one list per block.""" + blocks, current, inside = [], [], False + for line in lines: + if line.lstrip().startswith("```"): + if inside: + blocks.append(current) + current = [] + inside = not inside + continue + if inside: + current.append(line) + return blocks + + +def with_keys(lines): + """Mapping keys directly under each `with:` in `lines`. + + Relative-indentation only, no YAML parser: the first non-blank line after a + `with:` fixes the child indent, and keys at exactly that indent are + collected until the block dedents back to (or past) the `with:` line. So + `uses:` / `secrets:`, which sit at the `with:` indent, end the block rather + than count as inputs. + """ + keys = set() + i, n = 0, len(lines) + while i < n: + opener = WITH_LINE.match(lines[i]) + if not opener: + i += 1 + continue + base = len(opener.group(1)) + child = None + j = i + 1 + while j < n: + line = lines[j] + if not line.strip() or line.lstrip().startswith("#"): + j += 1 + continue + indent = len(line) - len(line.lstrip()) + if indent <= base: # dedent to a sibling (secrets:, next job key) + break + if child is None: + child = indent + if indent == child: + key = MAPPING_KEY.match(line) + if key: + keys.add(key.group(1)) + j += 1 + i = j + return keys + + +def example_with_keys(): + """`with:` keys from the two example callers this repo ships: the fenced + YAML under the guide's ## Caller heading, and the `# `-prefixed example in + the workflow header comment. Returns {source_label: set_of_keys}.""" + guide_lines = [] + for block in fenced_blocks(read_lines(SETUP_GUIDE)): + guide_lines.extend(block) + header_comment = [ + strip_comment_prefix(line) + for line in read_lines(WORKFLOW) + if line.startswith("#") + ] + return { + "docs/callers/refresh-reviewers.md ## Caller": with_keys(guide_lines), + "refresh-reviewers.yml header comment": with_keys(header_comment), + } + + +def workflow_input_defaults(): + """(`{name: raw default:}`, `{names marked required: true}`) from the + workflow. Required inputs carry no default (`workflows_ref`).""" + lines = read_lines(WORKFLOW) + if "jobs:" not in lines: + raise AssertionError( + f"no top-level `jobs:` line in {WORKFLOW} — file moved or its " + "structure changed; the input scan cannot be bounded" + ) + head = lines[: lines.index("jobs:")] + defaults, required, current, in_inputs = {}, set(), None, False + for line in head: + if line == " inputs:": + in_inputs = True + continue + if not in_inputs: + continue + if not line.strip() or line.lstrip().startswith("#"): + continue + if re.match(r"^ \S", line): # dedent out of inputs: (secrets:, etc.) + break + key = INPUT_KEY.match(line) + if key: + current = key.group(1) + continue + if current is None: + continue + default = INPUT_DEFAULT.match(line) + if default: + defaults[current] = default.group(1) + continue + req = INPUT_REQUIRED.match(line) + if req and req.group(1) == "true": + required.add(current) + return defaults, required + + +def documented_input_defaults(path, heading): + """`{input key: raw Default-column cell}` from the table under `heading`.""" + result, in_section = {}, False + for line in read_lines(path): + if line.strip() == heading: + in_section = True + continue + if in_section and HEADING.match(line): + break + if in_section and line.lstrip().startswith("|"): + cells = [cell.strip() for cell in line.strip().strip("|").split("|")] + if len(cells) < 2: + continue + key = re.fullmatch(r"`([A-Za-z0-9_-]+)`", cells[0]) + if key: + result[key.group(1)] = cells[1] + return result + + +# Canonical form for the required-with-no-default input, shared by both sides +# of the Default-column comparison so `workflows_ref`'s `— (**required**)` cell +# and the workflow's `required: true` compare equal. +_REQUIRED = "\x00required-no-default" + + +def canonical_guide_default(cell): + text = cell.strip() + if "**required**" in text: + return _REQUIRED + return text.strip("`").strip() + + +def canonical_workflow_default(name, defaults, required): + if name in defaults: + return defaults[name].strip() + if name in required: + return _REQUIRED + return None + + class RefreshReviewersInputsDocsTest(unittest.TestCase): def setUp(self): self.declared = workflow_inputs() @@ -143,6 +312,97 @@ def test_every_declared_input_is_documented(self): f"`docs/callers/refresh-reviewers.md` inputs table: {sorted(missing)}", ) + def test_example_with_blocks_are_declared_inputs(self): + # The two copy-paste callers (guide ## Caller fence, workflow header + # comment) pass inputs via `with:`. A phantom key there is a zero-job + # startup_failure for whoever copies it — the #31 failure, one level in + # from the Inputs table the two methods above already police. Subset, + # not equality: an example legitimately shows only a couple of inputs. + for label, keys in example_with_keys().items(): + # Guard: each example genuinely names inputs incl. workflows_ref, so + # a scanner gone quiet fails loudly instead of asserting {} ⊆ {}. + self.assertIn( + "workflows_ref", + keys, + f"the {label} `with:` scanner found no workflows_ref — the " + "example block moved or the parser broke; this subset check is " + "now vacuous", + ) + undeclared = keys - self.declared + self.assertFalse( + undeclared, + f"the {label} example passes `with:` inputs refresh-reviewers." + f"yml does not declare: {sorted(undeclared)} — a phantom input " + "there is a zero-job startup_failure for whoever copies it", + ) + + def test_readme_knob_table_has_no_phantom_knob(self): + # `.github/refresh-reviewers/README.md`'s "Knob defaults (and why)" + # table is intentionally partial and uses combined-cell rows + # (`| `top_k` / `floor` |`) the TABLE_KEY regex correctly does NOT + # capture, so assert ONLY the phantom direction — a knob named there but + # not declared. Set-equality would be permanently red (the table omits + # many inputs by design). + documented = documented_inputs(README, "## Knob defaults (and why)") + # Guard: a heading rename would empty this and make the check vacuous. + self.assertTrue( + documented, + "the README `## Knob defaults (and why)` scanner found no bare-name " + "knob rows — the heading was renamed or the table reshaped", + ) + phantom = documented - self.declared + self.assertFalse( + phantom, + "`.github/refresh-reviewers/README.md` Knob defaults table names " + f"knobs refresh-reviewers.yml does not declare: {sorted(phantom)}", + ) + + def test_guide_default_column_matches_workflow(self): + # Both assertions above are name-only, so the guide's Default column can + # silently drift from the workflow's real `default:`. Pin it. + defaults, required = workflow_input_defaults() + guide = documented_input_defaults(SETUP_GUIDE, "## Inputs") + # Guard both scanners: the workflow side must have parsed real defaults + # and the required-only workflows_ref; the guide side must show its cell. + self.assertTrue( + defaults, + "workflow_input_defaults found no `default:` lines — parser or file " + "structure changed; the Default-column check is now vacuous", + ) + self.assertIn( + "workflows_ref", + required, + "workflow_input_defaults lost the required `workflows_ref` — parser " + "or file structure changed; the Default-column check is now vacuous", + ) + self.assertIn( + "workflows_ref", + guide, + "the guide Inputs table Default scanner lost `workflows_ref` — " + "parser or table shape changed; this check is now vacuous", + ) + for name in sorted(self.declared): + with self.subTest(input=name): + self.assertIn( + name, + guide, + f"`{name}` is declared but absent from the guide Inputs " + "table Default column", + ) + want = canonical_workflow_default(name, defaults, required) + got = canonical_guide_default(guide[name]) + self.assertEqual( + got, + want, + f"guide Default column for `{name}` is {guide[name]!r} but " + "refresh-reviewers.yml has " + + ( + "required: true (no default)" + if name in required + else f"default: {defaults.get(name)!r}" + ), + ) + if __name__ == "__main__": unittest.main(verbosity=2) From f4f70387856c4a6daeb9d6e59ea5cb4e9329442c Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Wed, 16 Sep 2026 14:48:51 +0000 Subject: [PATCH 6/7] test(refresh-reviewers): scope drift scanners and harden Default parsing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address cursor-review panel findings on the input/docs drift test: - Scope the guide with: scan to the ## Caller fences and the workflow with: scan to the pre-jobs header comment, and scan each fenced block on its own, so an unrelated block can't satisfy the workflows_ref anti-vacuity guard while the real example loses its with:, a stray step-level with: can't register as a phantom input, and one block's with: can't absorb the next block's lines. - Parse the README's combined-cell knob rows (top_k / floor, min_touches / min_score) so those four knobs are still policed for phantom drift. - Anchor the required-cell check on the '— (**required**)' cell shape instead of a loose **required** substring, so a cell advertising a default the workflow lacks no longer canonicalizes to required. - Canonicalize an optional-no-default input to '' (its runtime value) instead of None, which the guide side can never produce. - Strip an inline comment and cosmetic surrounding quotes from a raw default: (preserving the '' spelling) so ordinary YAML edits don't false-red the suite. - Split table rows on unescaped pipes only, honoring GFM's \| so a Default cell containing a pipe doesn't shift every later column. - Raise on a duplicate input row so two contradictory Default cells can't pass as long as the last one is right. Co-Authored-By: Claude Opus 4.8 --- .../tests/test_inputs_docs.py | 180 ++++++++++++++---- 1 file changed, 147 insertions(+), 33 deletions(-) diff --git a/.github/refresh-reviewers/tests/test_inputs_docs.py b/.github/refresh-reviewers/tests/test_inputs_docs.py index 06254fae..af0e56fe 100644 --- a/.github/refresh-reviewers/tests/test_inputs_docs.py +++ b/.github/refresh-reviewers/tests/test_inputs_docs.py @@ -15,12 +15,13 @@ * `on.workflow_call.inputs` in `.github/workflows/refresh-reviewers.yml`, and * the "Inputs" table in `docs/callers/refresh-reviewers.md`. -This is deliberately a TWO-set check. `.github/refresh-reviewers/README.md`'s -"Knob defaults (and why)" table is intentionally partial (it omits -`reviewer_config_path`, `map_exclude`, `extra_exclude_paths` and `workflows_ref`) -and uses combined-cell rows (`| `top_k` / `floor` |`), so the bare-name table -regex neither captures nor should capture it — including it would make this test -permanently red or vacuous. +The guide's Inputs table is checked for full set-equality in BOTH directions. +`.github/refresh-reviewers/README.md`'s "Knob defaults (and why)" table is +intentionally partial (it omits `reviewer_config_path`, `map_exclude`, +`extra_exclude_paths` and `workflows_ref`), so it is checked ONE way only — no +phantom knob — but its combined-cell rows (`| `top_k` / `floor` |`) ARE parsed, +so every knob it names is still policed. Set-equality there would be permanently +red, since the table omits many inputs by design. Deliberately parsed WITHOUT PyYAML, like the cursor-review model (test_workflow_inputs_docs.py): this repo is stdlib-only and CI installs no @@ -71,6 +72,32 @@ def read_lines(path): return [line.rstrip("\r") for line in f.read().split("\n")] +def section_lines(lines, heading): + """Lines strictly under `heading` (a `## …` line), up to the next heading at + the same or a higher level. Lets a scan be scoped to one section instead of + the whole file, so an unrelated block elsewhere can't stand in for it.""" + out, in_section = [], False + for line in lines: + if line.strip() == heading: + in_section = True + continue + if in_section and HEADING.match(line): + break + if in_section: + out.append(line) + return out + + +def split_cells(line): + """Split a markdown table row into trimmed cells, honoring GFM's escaped + `\\|` (a literal pipe inside a cell) so a Default value that contains a pipe + — plausible for a regex-valued input — doesn't shift every later column and + make the Default comparison run against the wrong cell.""" + body = line.strip().strip("|") + cells = re.split(r"(? 2 and text[0] in "\"'" and text[-1] == text[0]: + return text[1:-1] + # An unquoted inline comment (` # …`) is not part of the value. + hash_at = text.find(" #") + if hash_at != -1: + text = text[:hash_at].rstrip() + return text + + def canonical_guide_default(cell): text = cell.strip() - if "**required**" in text: + # Anchor on the required-cell SHAPE (`— (**required**)`), not a loose + # `**required**` substring: a cell like `` `main` (**required**) `` advertises + # a default the workflow deliberately lacks and must NOT canonicalize to + # _REQUIRED (that drift is the whole point of this check), while a real + # default annotated in prose must not false-match either. + if text.startswith("—") and "**required**" in text: return _REQUIRED return text.strip("`").strip() def canonical_workflow_default(name, defaults, required): if name in defaults: - return defaults[name].strip() + return _clean_workflow_default(defaults[name]) if name in required: return _REQUIRED - return None + # Optional with no `default:` — a legal `workflow_call` shape that arrives at + # runtime as `''`. Canonicalize to the empty string it actually delivers + # rather than to None (which the guide side can never produce, permanently + # reddening the subTest). + return "" class RefreshReviewersInputsDocsTest(unittest.TestCase): @@ -338,17 +450,18 @@ def test_example_with_blocks_are_declared_inputs(self): def test_readme_knob_table_has_no_phantom_knob(self): # `.github/refresh-reviewers/README.md`'s "Knob defaults (and why)" - # table is intentionally partial and uses combined-cell rows - # (`| `top_k` / `floor` |`) the TABLE_KEY regex correctly does NOT - # capture, so assert ONLY the phantom direction — a knob named there but - # not declared. Set-equality would be permanently red (the table omits - # many inputs by design). - documented = documented_inputs(README, "## Knob defaults (and why)") + # table is intentionally partial, so assert ONLY the phantom direction — + # a knob named there but not declared. Set-equality would be permanently + # red (the table omits many inputs by design). Combined-cell rows + # (`| `top_k` / `floor` |`) ARE parsed here: skipping them would leave + # top_k/floor/min_touches/min_score free to drift to a stale name with + # this test still green. + documented = documented_knob_names(README, "## Knob defaults (and why)") # Guard: a heading rename would empty this and make the check vacuous. self.assertTrue( documented, - "the README `## Knob defaults (and why)` scanner found no bare-name " - "knob rows — the heading was renamed or the table reshaped", + "the README `## Knob defaults (and why)` scanner found no knob rows " + "— the heading was renamed or the table reshaped", ) phantom = documented - self.declared self.assertFalse( @@ -391,16 +504,17 @@ def test_guide_default_column_matches_workflow(self): ) want = canonical_workflow_default(name, defaults, required) got = canonical_guide_default(guide[name]) + if name in required: + have = "required: true (no default)" + elif name in defaults: + have = f"default: {defaults[name]!r}" + else: + have = "no default and not required (arrives as '')" self.assertEqual( got, want, f"guide Default column for `{name}` is {guide[name]!r} but " - "refresh-reviewers.yml has " - + ( - "required: true (no default)" - if name in required - else f"default: {defaults.get(name)!r}" - ), + f"refresh-reviewers.yml has {have}", ) From ceefaa125f46fcb1927541de3db1d1f31044322f Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Thu, 17 Sep 2026 07:52:41 +0000 Subject: [PATCH 7/7] fix(refresh-reviewers): stop comment-stripping from leaving stray quotes on a quoted default _clean_workflow_default checked for surrounding quotes before stripping a trailing inline comment, so a quoted default followed by a comment (e.g. `"bot/refresh-reviewers" # pinned`) never matched the quote check and kept its quotes, diverging from the guide's unquoted cell. Scan for the closing quote first, treating anything after it as a comment. Addresses a CodeRabbit finding on PR #299. --- .../tests/test_inputs_docs.py | 22 +++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/.github/refresh-reviewers/tests/test_inputs_docs.py b/.github/refresh-reviewers/tests/test_inputs_docs.py index af0e56fe..65e359f3 100644 --- a/.github/refresh-reviewers/tests/test_inputs_docs.py +++ b/.github/refresh-reviewers/tests/test_inputs_docs.py @@ -353,10 +353,24 @@ def _clean_workflow_default(raw): docs. Backticks are stripped from the guide side, so the two sides would otherwise compare a value against its raw YAML source.""" text = raw.strip() - # Surrounding matching quotes are cosmetic — EXCEPT the empty string, which - # both the guide and this file spell `''`, so leave that spelling intact. - if len(text) > 2 and text[0] in "\"'" and text[-1] == text[0]: - return text[1:-1] + # A quoted scalar ends at its closing quote; anything after that (including + # a ` #` that would otherwise look like a comment marker) is a trailing + # inline comment, not part of the value. Keep the `''` spelling both the + # guide and this file use for the empty string. + if text[:1] in "\"'": + quote = text[0] + end = 1 + while end < len(text): + if quote == '"' and text[end] == "\\": + end += 2 + continue + if quote == "'" and text[end : end + 2] == "''": + end += 2 + continue + if text[end] == quote: + inner = text[1:end] + return inner if inner else quote * 2 + end += 1 # An unquoted inline comment (` # …`) is not part of the value. hash_at = text.find(" #") if hash_at != -1: