Skip to content

fix(setup): one canonical review-workflow template — five of six copies could never fire (#161) - #185

Merged
mmcky merged 2 commits into
mainfrom
161-canonical-review-workflow
Jul 24, 2026
Merged

fix(setup): one canonical review-workflow template — five of six copies could never fire (#161)#185
mmcky merged 2 commits into
mainfrom
161-canonical-review-workflow

Conversation

@mmcky

@mmcky mmcky commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Closes #161.

Wave 1 PR D. The review workflow existed in six divergent copies — the translate setup scaffolder plus five doc pages — and five of them could never fire on Action sync PRs: the action-translation label is applied after the PR opens, so types: [opened, synchronize] never sees it and the label gate always fails. That is the same shape as the v0.21.0 incident where six PRs of real translated content merged unreviewed. Once scaffolded, a template is a copy that never updates, so every repo created from it carried the defect.

The structural fix (F7 + F136)

examples/review-translations.yml is now the single canonical template, promoted from the one correct copy (connect-existing.md): [opened, synchronize, labeled, reopened] triggers, the two-part label guard, permissions, the per-PR concurrency group, checkout@v7, @v0. translate setup renders it at runtime — the examples directory is threaded from the CLI entry point exactly like the glossary dir, for the same Jest-CJS reason — substituting source-repo/source-language/docs-folder, and rendering the template with its own example values is the identity (tested), so the file doubles as a golden output. A substitution key missing from the template throws at scaffold time. All five doc pages quote the same shape, and a new drift test in the branch-naming.test.ts mold (workflow-templates.test.ts) fails when any copy — canonical file or doc page — loses a structural line. The old setup.test.ts assertions that passed on the broken template (pull_request: and the bare string action-translation) are replaced with assertions on the lines that matter.

translate doctor now warns instead of passing when a review workflow's trigger list lacks labeled (the second half of F7), naming the file and the fix.

The scaffolder catch-up (F27 + F91 + F135 + F137)

The scaffolder emitted @v0.9.0 — 25 releases stale — from two bare string literals no release-checklist step knew to bump. It now emits the floating @v0, which closes F137 by construction: there is nothing left to forget. The generated source workflow gains the issue_comment trigger and composite guard for \translate-resync (it previously scaffolded a repo where the documented recovery command silently did nothing), checkout@v7 replaces @v4 in both templates plus README's front-page snippet, setup now also writes rebase-translations.yml verbatim from its canonical file, and the paths-filter normalizer gets its missing g flag — ./lectures/ used to emit a broken lectures//**/*.md filter (regression-tested over ['lectures', 'lectures/', './lectures/', '/lectures/', '.', '/', '']).

F127 — the docs remnant, deliberately not the code fix

Review mode has never read a target-language input (it detects the language from the repository-name suffix; since v0.23.0 it warns when it can't). This PR lands option (b) of the audit's recommendation: the dead knob is removed from the review-inputs table, quickstart, and every snippet, and the suffix convention is documented in its place — plus the drift test asserts no review block re-teaches it. Option (a) — honouring the input — would touch inputs.ts/src/index.ts, and this PR is declared no dist-action/ rebuild; that decision is recorded in the .dev log as deliberately not taken here. Also corrected while in the tables: mode admits rebase, cli-reference no longer names a scaffold file (translation-sync.yml) that setup never wrote, and its .translate/ samples show <tool version> instead of a hard-coded 0.8.0.

Verification

57/57 suites (1320 passed / 3 skipped — 14 new or rewritten assertions across setup, doctor, and the new drift suite), lint/format/check-dev-refs green, and zero dist-action/ drift as the issue requires (setup/doctor are CLI-only and not bundled).

Fleet note: repos scaffolded by older setup versions still carry the unfireable workflow — translate doctor now surfaces them, but the estate's existing editions were hand-fixed back in the #96/#98 round and are unaffected.

🤖 Generated with Claude Code

Tech-debt Wave 1 PR D (#161, audit F7/F136/F27/F91/F135/F137 + F127's
docs remnant). The review workflow existed in six divergent copies and
five could never fire on Action sync PRs — the action-translation label
lands after the PR opens, so types: [opened, synchronize] never sees it
(the v0.21.0 six-PRs-merged-unreviewed shape).

- examples/review-translations.yml is the single canonical template.
  setup renders it at runtime (examples dir threaded from the entry
  point, glossary-dir pattern) substituting source-repo/source-language/
  docs-folder; rendering with the template's own example values is the
  identity. Docs quote the same shape; workflow-templates.test.ts
  (branch-naming mold) fails when any copy loses a structural line.
- Scaffolder: @v0 replaces the v0.9.0 literals (nothing left for a
  release to forget — closes the F137 checklist gap), writes
  rebase-translations.yml verbatim from its canonical file, source
  workflow gains the issue_comment resync trigger + composite guard,
  checkout@v7 everywhere, and the paths-filter normalizer gets its
  missing g flag (./lectures/ emitted a broken lectures//**/*.md).
- doctor warns on a review workflow whose trigger list lacks `labeled`
  instead of passing the repo.
- F127 remnant (docs half only — this PR does not rebuild dist-action):
  review mode never read target-language; the input row, quickstart
  line and scaffolded key are removed and every review snippet
  documents the repo-name-suffix convention. Mode row admits rebase;
  cli-reference drops the never-written translation-sync.yml name and
  hard-coded 0.8.0 samples.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 24, 2026 03:28

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a long-standing “review workflow can’t fire” defect in translate setup and the documentation by promoting examples/review-translations.yml to the single canonical template, rendering it at scaffold-time, and adding drift/doctor guards so repos don’t silently ship broken review automation.

Changes:

  • Make examples/review-translations.yml the canonical, rendered-at-runtime review workflow template (with drift tests to keep docs and templates aligned).
  • Update translate setup scaffolding to use @v0, checkout@v7, add resync triggers for the source workflow, and scaffold the rebase workflow from canonical templates.
  • Strengthen translate doctor to warn when a review workflow is structurally unable to trigger on sync PRs.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
src/cli/index.ts Adds EXAMPLES_DIR resolution and threads it into setup options.
src/cli/commands/setup.ts Loads canonical workflow templates from examples/, renders the review workflow via substitutions, updates source workflow triggers and checkout/action pins.
src/cli/commands/doctor.ts Adds a warning when a review workflow appears unfireable due to missing labeled.
src/cli/tests/workflow-templates.test.ts Adds drift-guard tests ensuring docs + canonical workflow keep required structural lines and don’t re-teach target-language for review.
src/cli/tests/setup.test.ts Updates setup workflow assertions for new triggers/templates and adds regression coverage for docs-folder normalization.
src/cli/tests/doctor.test.ts Adds coverage for the new “unfireable review workflow” doctor warning.
README.md Updates the quickstart snippet to actions/checkout@v7.
examples/review-translations.yml Introduces the canonical review workflow template (triggers, label guard, permissions, concurrency, checkout@v7, @v0).
docs/user/quickstart.md Updates the review workflow snippet and removes the dead review target-language knob.
docs/user/action-reference.md Updates mode list, updates review workflow snippet, and documents suffix-based target language detection.
docs/user/cli-reference.md Fixes scaffolded file list and replaces hard-coded tool versions with placeholders.
docs/user/tutorials/fresh-setup.md Updates the review workflow snippet and documents suffix-based language detection.
docs/user/tutorials/add-language.md Updates the review workflow snippet and documents suffix-based language detection.
docs/user/tutorials/connect-existing.md Removes dead target-language knob and documents suffix-based language detection.
CHANGELOG.md Documents the canonical-template migration and related scaffolder/doctor changes under Unreleased.
.dev/STATE.md Updates Wave 1 status to reflect #160 merged and #161 open.
.dev/log/2026-07-24-pr-d-canonical-workflow.md Adds a maintainer log entry capturing the rationale and intended non-changes (no dist-action rebuild).
Comments suppressed due to low confidence (2)

src/cli/commands/setup.ts:160

  • The sync workflow runs on every issue_comment event where the body contains \\translate-resync, including comments on issues (not PRs) and comments that merely mention the command mid-text. The action itself no-ops in those cases, but the workflow still burns a runner and checks out the repo. Consider tightening the job if: to require github.event.issue.pull_request and to use startsWith(github.event.comment.body, '\\translate-resync') to match the action’s own parsing behavior.
    # Merged PRs sync; the issue_comment trigger lets \`\\translate-resync\`
    # on a merged PR retry a failed sync.
    if: >
      (github.event_name == 'pull_request' && github.event.pull_request.merged == true) ||
      (github.event_name == 'issue_comment' && contains(github.event.comment.body, '\\translate-resync'))

src/cli/tests/setup.test.ts:136

  • This assertion is line-ending sensitive (@v0\n). Consider checking for QuantEcon/action-translation@v0 without the newline so the test doesn’t depend on the file’s line endings or trailing newline.
    expect(yaml).toContain('QuantEcon/action-translation@v0\n');

Comment thread src/cli/commands/setup.ts Outdated
Comment thread src/cli/commands/setup.ts Outdated
Comment thread src/cli/commands/doctor.ts
Comment thread src/cli/__tests__/workflow-templates.test.ts Outdated
Comment thread src/cli/__tests__/setup.test.ts Outdated
…lesDir, trigger-scoped doctor check

- normalizePathsFilter strips all trailing slashes (lectures// was still
  broken), and the cleaned value is echoed into the docs-folder input
  line of both scaffolded workflows, not just the paths filter.
- SetupOptions.examplesDir is required in the type instead of optional
  with a runtime throw — every caller already passes it.
- doctor's unfireable-review check looks for labeled in trigger types:
  lists (inline or dash form), so an if: guard mentioning 'labeled'
  no longer suppresses the warning; tests cover the half-fixed shape.
- @v0 assertions drop the literal newline boundary for a no-pinned-tag
  negative match, keeping the @v0.9.0 regression guard without
  line-ending sensitivity.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@mmcky
mmcky merged commit b127712 into main Jul 24, 2026
1 check passed
@mmcky
mmcky deleted the 161-canonical-review-workflow branch July 24, 2026 03:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tech-debt D: Five of six review-workflow templates emit a workflow that can never fire

2 participants