Conversation
🟡 Waiting for changesLast updated: 2026-09-16 14:37 UTC |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Solid generator/registry design and correctly-scoped permission narrowing, but the branch was forked before recent changes landed on main that this registry doesn't account for.
CI passing. No UI files in scope, Phase 3 skipped.
Blocking:
automation-registry.yml:79— stale/too-broadmanage-issue-headerdispatch condition, and a whole new automation (good-first-issue-comment) missing from the registry entirely (inline comment).
Suggestion:
- PR body —
Closes #86will auto-close the tracking issue despite the acknowledged-incomplete migration checklist; considerPart of #86instead, or open a follow-up tracking issue for the remaining per-repo migrations.
Nitpick:
package.json:10—js-yamlis only used by the build-time codegen script; considerdevDependenciesinstead.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran a phased review pipeline over the pull request diff:
- Classified the diff to select review passes (core, frontend, backend) and whether manual QA was required
- Core review pass checked correctness, design, architecture, testing, completeness, and DRY/SRP/Rule-of-Three principles
- Specialized frontend/backend review passes applied framework-specific lenses where those files changed
- For UI changes: manual QA and an accessibility audit against a live dev server, when available
- Checked CI status and linked issue acceptance criteria
- Synthesized one review from those passes and chose the verdict from the findings, CI status, and QA evidence
| on: | ||
| issues: | ||
| types: [opened, reopened, labeled, unlabeled] | ||
| if: "github.event_name == 'issues' && contains(fromJSON('[\"opened\",\"reopened\",\"labeled\",\"unlabeled\"]'), github.event.action)" |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: This branch's merge-base with upstream/main (5512260) is stale — main has since merged #62/#90, which refactored issue-header dispatch:
manage-issue-header.yml's own leaf-levelif:filter (opened/reopened/labeled+help wanted/unlabeled+help wanted) was removed and pushed up into two new callers,issue-open.yml(unconditional for opened/reopened) andissue-label.yml(labeled/unlabeled gated tolabel.name == 'help wanted'only).- This registry entry's
if:fires on any labeled/unlabeled action regardless of label name, and since the leaf no longer self-filters, merging as-is meansmanage-issue-header.jsruns on every label add/remove on every issue — a real behavior regression, not just missed parity. issue-label.ymlalso adds a second job,good-first-issue-comment(gated onlabel.name in ['good first issue', 'help wanted']), calling a brand-new leafgood-first-issue-comment.yml. This automation has no entry anywhere in the registry, so it's silently dropped from the generatedautomation.yml.
Please rebase onto current upstream/main, add a good-first-issue-comment registry entry, and split this entry's if: to match issue-label.yml's help wanted-only gating.
There was a problem hiding this comment.
Done — rebased onto current main, split manage-issue-header into issue-open (opened/reopened, unconditional) and issue-label-header (labeled/unlabeled, help wanted only), and added a good-first-issue-comment entry calling the new leaf directly from automation.yml.
There was a problem hiding this comment.
Confirmed — all three are resolved. issue-open (opened/reopened, unconditional) and issue-label-header (labeled/unlabeled gated to help wanted) now match the split in issue-open.yml/issue-label.yml, and the new good-first-issue-comment entry dispatches good-first-issue-comment.yml directly with the good first issue/help wanted gate. Generated automation.yml reflects all three. Thanks for the quick turnaround.
| "axios": "^1.13.6", | ||
| "dotenv": "^17.3.1", | ||
| "googleapis": "^142.0.0", | ||
| "js-yaml": "^4.1.0", |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: js-yaml is only consumed by scripts/generate-automation.js (a build-time tool), not by any runtime workflow script — consider moving it to devDependencies.
There was a problem hiding this comment.
Good catch — moved to devDependencies.
…ntry point Replace the per-feature call-*.yml callers and the pull-request-target.yml dispatcher with one reusable automation.yml plus a single generated caller template, driven by automation-registry.yml as the source of truth. A pre-commit hook regenerates and fails on drift between the registry and the generated files. Closes #86 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…chedule handling, permissions model - Mark all Slack/spreadsheet/GCP secrets as required in the registry to match leaf workflow declarations (restoring pre-PR behavior where callers forwarded all secrets unconditionally and leaves failed loudly if any were absent) - Add automation-caller.yml to the generate-automation pre-commit hook's files: pattern so direct edits to that generated file are caught by the drift check - Fix unionOn() to handle both object and list form for schedule entries, guarding against undefined cron if a second schedule is added in GHA-native format - Give automation-caller.yml its own generated header distinct from the consumer-facing one in automation-template.yml - Replace needs_write_permissions boolean with a generic permissions: map in the registry; generator now emits it directly rather than special-casing a flag - Compute collectSecrets() once in main() and pass to both build functions Regenerated automation.yml, automation-template.yml, and automation-caller.yml. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
automation.yml now dispatches good-first-issue-comment directly rather than going through the issue-label intermediate workflow. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
dab693f to
01729bf
Compare
Only used by scripts/generate-automation.js at build time. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The caller template's permissions were hardcoded while the registry declared them per automation. A future entry needing a new scope would have made automation.yml request more than the caller grants, and the drift check could not catch it. The template now takes the union of every enabled automation's permissions. The regenerated template and caller came out unchanged, which confirms the union reproduces the old map. unionOn threw on undefined.includes or silently narrowed the trigger when one automation declared an event with types and another declared the same event without. A missing types list now means all types, whichever order the entries appear in. The five non-bot secrets are now optional, which matches the criteria in #86. Note that the leaf workflows still declare them required. Nothing breaks, because automation.yml always forwards the key. issue-open.yml and issue-label.yml lost their only callers when call-issue-open.yml and call-issue-label.yml were removed. No consumer repo references them, because consumers use call-manage-issue-header.yml, which points straight at the leaf. Also show PR instead of Pr in three job names, run the drift-check hook when only the generator changes, and fix the secrets paragraph in the docs, which promised a loud failure that does not happen.
akolson
changed the title
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #88 — 3 of 4 prior findings resolved; 1 still open, plus 2 new suggestions in the newly added generator/doc code.
CI passing. No UI files in scope, so Phase 3 was skipped.
PR body — suggestion: Closes #86 still auto-closes the tracking issue on merge, but #86's Migration & cutover section has ten unchecked per-repo boxes that this PR explicitly defers ("Migrating the consumer repos is out of scope here and lands in separate PRs"). Drop the keyword or move the migration checklist to a follow-up issue.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Compared the current PR state against findings from a prior review:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Ran the same phased review passes as a first review (core, frontend/backend lenses, manual QA when required)
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
Prior-finding status
RESOLVED — automation-registry.yml:79 — stale/too-broad manage-issue-header dispatch and missing good-first-issue-comment entry
RESOLVED — package.json:10 — js-yaml belongs in devDependencies
RESOLVED — automation-registry.yml — branch based on stale merge-base (now 688cf6b, includes #62/#90)
UNADDRESSED — PR body — Closes #86 auto-closes the tracking issue despite the deferred migration checklist
| | `CONTRIBUTIONS_SHEET_NAME` | no | Sheet name within the spreadsheet | | ||
| | `GH_UPLOADER_GCP_SA_CREDENTIALS` | no | GCP service account credentials for Sheets access | | ||
|
|
||
| Every automation authenticates as the bot, so the two required secrets must be set. A repo that |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: Both sentences an onboarding maintainer will act on are inaccurate.
- "Every automation authenticates as the bot" —
resolve-bot-pr-threadshassecrets: {}(automation-registry.yml:147) and runs on the defaultGITHUB_TOKEN. This claim is the stated justification for the two required secrets, so it's load-bearing. - L55 "the one automation that needs it fails at run time" —
SLACK_COMMUNITY_NOTIFICATIONS_WEBHOOK_URLis forwarded to four automations (.github/workflows/automation.yml:26). Skipping it means a failing job on every PR open, every issue comment, and every weekly run.
Suggest "every automation that needs it fails at run time", dropping the bot-auth universal, and stating plainly that "optional" means opting out of a working automation rather than graceful degradation.
There was a problem hiding this comment.
Both correct, fixed in c1311d0. The bot-auth sentence now excludes resolve-bot-pr-threads, the failure sentence reads "every automation that needs it", and the paragraph states that optional means losing the automation rather than graceful degradation.
| if (!on[event].types.includes(t)) on[event].types.push(t); | ||
| } | ||
| } else { | ||
| on[event] = {}; |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: The L133 guard plus this unconditional on[event] = {} make a typeless registry entry win over typed ones in either order. Correct for issues/issue_comment/pull_request_review_comment (no types: means all activity types), wrong for pull_request_target, where omitting types: means only opened, synchronize, reopened.
If a future entry declares pull_request_target: {} alongside review-requested (types: [review_requested]) and update-pr-spreadsheet, the template emits bare pull_request_target: {} and every consumer silently stops firing on labeled, review_requested, assigned — the drift #86 exists to eliminate, now generator-emitted. Latent today since every non-schedule entry declares types:.
Refusing the ambiguity avoids needing per-event knowledge of GitHub's defaults:
} else if (on[event]) {
throw new Error(`Event "${event}" is declared both with and without types (automation "${a.name}") - declare types: explicitly on every entry.`);
}There was a problem hiding this comment.
Right, and the pull_request_target default is what makes "typeless means all types" unsafe. Fixed in c1311d0.
I did not use the snippet as written. } else if (on[event]) also throws when two automations declare the same event without types:, which workflow_dispatch hits as soon as a second scheduled automation is added. The guard is symmetric instead, so it throws only when one entry declares types: and another omits it, in either order.
| name: Check automation.yml / automation-template.yml are up to date | ||
| entry: node scripts/generate-automation.js --check | ||
| language: system | ||
| files: '^(automation-registry\.yml|scripts/generate-automation\.js|\.github/workflows/automation(-caller)?\.yml|automation-template\.yml)$' |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
praise: Adding the generator itself to files: closes a real drift hole — editing the output format without touching the registry previously left stale generated files checked in without the hook even running.
Treating a missing types: as all types was wrong for pull_request_target, where omitting types: means only opened, synchronize and reopened. A registry entry declaring pull_request_target without types: would have silently narrowed every consumer's triggers, which is the drift #86 exists to remove. The generator now refuses the mix in either order. Two entries that both declare the same event without types:, such as workflow_dispatch, stay valid. The docs claimed that every automation authenticates as the bot, but resolve-bot-pr-threads has no secrets and uses the default GITHUB_TOKEN. They also said that one automation fails when an optional secret is absent, where SLACK_COMMUNITY_NOTIFICATIONS_WEBHOOK_URL reaches four of them.
|
Changed to The three other repos in #86's list are archived (and are now part of the kolibri repo): |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #88 — 7 of 7 prior findings resolved; no new findings. CI passing.
Prior-finding status
RESOLVED — docs/automation.md:50 — "every automation authenticates as the bot" is false for resolve-bot-pr-threads
RESOLVED — docs/automation.md:55 — "the one automation that needs it" understates the Slack webhook's reach
RESOLVED — scripts/generate-automation.js:149 — typeless entry silently wins over typed ones in either order
RESOLVED — automation-registry.yml:79 — stale/too-broad manage-issue-header dispatch and missing good-first-issue-comment entry
RESOLVED — automation-registry.yml — branch based on stale merge-base (now includes #62/#90)
RESOLVED — package.json:10 — js-yaml belongs in devDependencies
RESOLVED — PR body — Closes #86 auto-closes the tracking issue despite the deferred migration checklist
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Compared the current PR state against findings from a prior review:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Ran the same phased review passes as a first review (core, frontend/backend lenses, manual QA when required)
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
rtibbles
left a comment
rtibbles
left a comment
There was a problem hiding this comment.
This looks ready to go - my only concern is propagating updates when we have to update the calling workflow, but I think it's best to try out the process a few times to see where the friction points are before trying to optimize!
|
Hit in learningequality/le-utils#238 — --- automation-template.yml (upstream, blob aa524e2)
+++ .github/workflows/automation.yml (after yamlfmt v0.16.0)
@@ -4,7 +4,6 @@
#
# Caller template: copy this file to .github/workflows/automation.yml in a consumer repo.
# No edits are needed - the on: block is the exhaustive union of every enabled automation.
-
name: Automation
on:
pull_request_target:Ask: have @rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly |
|
zizmor's rules:
dangerous-triggers:
ignore:
- automation.ymlThat form is worse than the inline one in two ways: zizmor matches these keys by basename rather than path, so it also exempts any future file named Ask: have Hit in learningequality/kolibri#15284 — zizmor v1.29.0 via prek, and Separately, the yamlfmt blank-line strip reported above also reproduces on v0.21.0. @rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly |
|
learningequality/studio deleted No consumer can fix it locally — the template is generated and copied unedited. Fix is Registry entries have no per-repo scoping key ( |
yamlfmt v0.16.0 strips the blank line between the header comment and the first key. le-utils pins that version and runs it over all files, so a consumer could not hold both an unedited copy of the template and a green lint job. This repo pins v0.14.0, which leaves the line alone, and excludes the generated files from yamlfmt anyway, so nothing here caught it. The generator now emits the header without the trailing blank line. All three generated files are byte-stable under both v0.14.0 and v0.16.0.
|
Confirmed and fixed in b3bbf69. I reproduced it with both yamlfmt versions: v0.16.0 strips the blank line, v0.14.0 does not. The generator now emits the header without it, and all three generated files are byte-stable under both. Worth noting for later: this repo pins v0.14.0 and also excludes the generated files from yamlfmt, so neither condition here would have caught this. Bumping the pin to v0.16.0 would, but it also reformats |
zizmor reports pull_request_target as a dangerous trigger. The hand-written callers this template replaces each carry an inline ignore on their `on:` key, so consumers running zizmor pass today. The generated template carried none, which left those consumers a config-level ignore as the only option. That form matches by basename, so it would also exempt any other file named automation.yml, and it would silently cover workflow_run if one were ever added. The generator now emits the same inline ignore on the `on:` key of the template and this repo's caller. automation.yml is untouched, because workflow_call is not a dangerous trigger.
|
Fixed in 5c97f47. The generator now emits Verified with zizmor v1.29.0 at On yamlfmt: b3bbf69 removed the blank line entirely rather than relocating it, so v0.21.0 is covered too. All three generated files are byte-stable under both v0.14.0 and v0.16.0 with the new comment in place. |
holiday-message was enabled in the registry, but the leaf has no date guard and the message text names fixed dates in December and January. None of the eight consumer repos runs it today, so migrating with the flag on would have started posting a holiday notice year-round on every contributor pull request and every issue comment, in all eight. The flag is now false, to be flipped on before the holidays and off after them. Flipping it also reordered the template's on: keys, because the union was built in registry iteration order. A consumer holds a copied template, so a reshuffle with no functional change would still force all eight repos to re-copy it, twice a year. The union now sorts event keys and type lists, which makes the template byte-identical whether holiday-message is on or off. The event and type sets are unchanged. Only their order is.
|
Fixed in fc9c4eb, and the scope is wider than reported. I checked all eight consumers, not just
Flipping the flag also surfaced a second problem. It reordered the template's |
3 participants
Summary
Every consumer repo copies about seven
call-*.ymlworkflows, one per automation. Each one carries its ownon:block and its own exhaustivetypes:list. Adding, removing or toggling an automation therefore means a change in every repo. Thetypes:lists also drift, because a defaultpull_request_targettrigger silently misseslabeled,review_requestedandassigned.This replaces all of them with one file to copy,
automation-template.yml. The template calls a central reusableautomation.yml, which dispatches to each leaf workflow based on the triggering event. Routing and on/off control now live in this repo.automation.ymlcalls the leaves directly instead of going through the oldpull-request-target.ymldispatcher. That keeps the chain atcaller -> automation.yml -> leaf -> is-contributor.yml, which is GitHub's limit of four levels for nested reusable workflows.The change
automation-registry.ymlis the source of truth. One entry per automation declares its leaf, trigger events, dispatchif:, secrets and permissions.scripts/generate-automation.jsgeneratesautomation.yml,automation-template.yml, andautomation-caller.yml, which is this repo's own copy of the template.LE_BOT_APP_IDandLE_BOT_PRIVATE_KEYare required. The Slack, spreadsheet and GCP secrets are optional. See the note in reviewer guidance for what that does and does not mean.resolve-bot-pr-threadsis the only automation that uses the defaultGITHUB_TOKENwith write access, so its job getscontents: writeandpull-requests: write. Every other job narrows tocontents: read.call-*.ymlcallers and thepull-request-target.yml,issue-open.ymlandissue-label.ymldispatchers. The leaf workflows stay as they are.Automations wired up:
review-requestedpull-request-labeldependabot-reviewercontributor-pr-replyholiday-messageissue-openissue-label-headerhelp wantedgood-first-issue-commentgood first issueorhelp wantedupdate-pr-spreadsheetcommunity-contribution-labelcontributor-issue-commentresolve-bot-pr-threadsunassign-inactive-issuesNo leaf workflow's internal logic changed. This is a routing and packaging consolidation only.
Migration
Migrating the consumer repos is out of scope here and lands in separate PRs. Merge this one only after those are open, because deleting
pull-request-target.ymlbreaks any repo that still calls it.#86 asks for the org-wide search to be re-run at execution time, to confirm the consumer set. I did that. Eight repos still call
pull-request-target.yml:kolibri,studio,ricecooker,kolibri-design-system,le-utils,kolibri-data-portal,kolibri-installer-debianandmorango.morangois the one addition to the list in #86. Each ofthe eight has a migration issue, linked under References.
Three further repos still hold a caller file:
kolibri-app,kolibri-installer-androidandkolibri-image-pi. All three are archived, so Actions do not run there and no migration is needed.References
Refs #86. Merging this does not close the tracking issue, because the consumer migrations below are
still outstanding. Please close it by hand once they land.
One migration issue per live consumer:
Reviewer guidance
The table in #86 records what each consumer caller triggers on. I re-checked it against the
call-*.ymlfiles in the consumer repos, and the two agree, so every automation here fires on the events it fires on today. To check that yourself:on:block inautomation-template.ymlagainst the callers in a consumer repo, for examplekolibri. Every event and type must appear, and the cron must stay1 0 * * 1.if:on each job in.github/workflows/automation.ymlagainst the table above. Each one must match the trigger that its old caller used.node scripts/generate-automation.js --check. It must exit 0. Then editautomation-registry.ymlwithout regenerating, and run it again. It must name the drifted file and exit 1.prek run --all-files. All eight hooks must pass, including actionlint overautomation.ymlandautomation-caller.yml.Two behaviour changes are deliberate and worth a second opinion. Consumer repos gain
good-first-issue-comment, because none of them copied that caller. Andmanage-issue-headernow runs only forhelp wantedlabel events, which is whatscripts/manage-issue-header.jsalready did internally.On the optional secrets: the registry marks five secrets optional, which describes the
workflow_callboundary ofautomation.ymlonly. The leaf workflows still declare them required, and the steps that use them are not guarded on the secret being set. A repo that omits a Slack webhook will still see that Slack step fail. Making these secrets optional in practice means guarding those steps, which #86 puts out of scope.AI usage
I used Claude Code (Sonnet 4.6) to implement this from the architecture in #86, which I directed: a single entry point, a declarative registry with a generator, explicit secrets instead of
secrets: inherit, a hard cutover, and pre-commit plus CI enforcement. I reviewed the generatedautomation.ymlandautomation-template.ymloutput. During that review I caught a permissions gap, becauseresolve-bot-pr-threadsneeds write access that the other automations must not inherit, and I directed the job-level narrowing before it became a security problem.I then ran a second Claude Code pass as a self-review, before asking for review here. It traced trigger parity against the real consumer callers and found five problems, which I fixed in 0a2d045. The template permissions were hardcoded rather than derived from the registry.
unionOncrashed or silently narrowed a trigger when an event appeared both with and withouttypes:. The docs promised a secret failure that does not happen. The drift-check hook skipped commits that touched only the generator. Two dispatchers were left unreferenced. I reviewed each fix, confirmed that the regenerated template and caller were unchanged, and confirmed thatprekand CI pass.