Repository navigation
Conversation
Contributors open pull requests that leave required sections empty, and reviewers ask for the missing information by hand. The check resolves the template the way GitHub does, the repo's own copy first and the shared one second, then reads which sections that template marks as required. A section is required when its heading carries a REQUIRED comment, and a template that marks none requires every section, so a repo is strict rather than silently unchecked. It reports a required section that is missing, empty, or still holding the placeholder, and it names every one of them, because the job log is the only channel it has. The template is read from the base branch, so a pull request cannot relax the rules it is judged by. Bot authors are exempt, as the kolibri-design-system changelog check already does.
GitHub applies an org default template in the web form only. A pull request opened with gh pr create, the API, or a coding agent reads the working copy instead, and the org file is not in it, so a contributor in a kolibri checkout has no template to follow. The sync now carries a list of files rather than one. A consumer is in sync when every file it takes matches, one pull request carries every file that drifted, and the declined and toolchain-conflict guards hold only when every drifted file agrees. A consumer keeping a template of its own keeps it. That is checked in all six places GitHub reads a template from, and a path that cannot be read counts as a template of its own, so a failed read never overwrites a file the run could not see. morango keeps its own template at the path the sync writes to, so existence alone cannot tell a copy of ours from a template of theirs. The shared template opens with a marker comment naming this repo, and at that one path a file without the marker is left alone.
🟡 Waiting for changesOutcome: reviewed Last updated: 2026-10-22 23:22 UTC |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #116 has no blocking findings. The leaf workflow, registry and multi-file sync match #102. CI passes.
- Suggestions: ellipsis false positive, bot exemption, read-error fallback, per-file verdicts, sync PR text and dry-run output, three test gaps, and the unreported
test-actionsruns (all inline). - Nitpick: several added comments restate their code (e.g.
scripts/pull-request-template.js:29-31,scripts/sync-automation-template.js:269-271). Drop them.
Next step: Needs a full code review by a human engineer.
- Workflow steps changed:
.github/workflows/automation-caller.yml,.github/workflows/automation-group-pull-request-target.yml,.github/workflows/automation.yml,.github/workflows/pull-request-template.yml,.github/workflows/sync-automation-template.yml. - Decide whether the org's coding-agent account should be exempt from the template check.
- Confirm the per-file sync verdict behaviour for consumers that declined or deleted the template.
- Confirm the
test-actionsend-to-end cases ran, or are planned after merge.
Written by rtibblesbot, an LLM-based coding agent.
An ellipsis anywhere in a section failed it, so a summary reading "Adds a Loading… spinner" was rejected. Only a line that is the placeholder on its own counts now. A 403 or 5xx reading a repo's own template fell back to the shared one, which would fail a body that follows the template its author was given. The check is skipped instead, matching what the sync does with a path it cannot read. One verdict for every pending file misjudged two cases: a consumer who declined the template was offered it again whenever automation.yml drifted, and a consumer who merged it and then deleted it became a permanent toolchain-conflict. Each file carries its own verdict now, only drifted files are proposed, and the most severe of the rest is reported. A consumer whose only drifted file was the template got a pull request titled "Refresh automation.yml from the shared template", describing a file it did not contain. The title and body are built from the files in the pull request, and the result carries them so the dry run says which ones it would write. The path list lived in both scripts and could drift, so it moved to constants. Three tests were weak or missing. The uppercase-path test passed with that path removed. Nothing checked that the real template carries the sync marker, whose loss would silently stop the sync refreshing copies it placed. Nothing covered verdicts across several pending files.
|
On the nitpick in the review body: dropped in 784d984. The comments that restated their code are gone, including the two you named. The two left carry facts from outside the file: the contents API being case sensitive, and the morango template sitting at the path the sync writes to. |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #116: 10 of 11 prior findings settled; 1 still open.
CI passing.
- suggestion (open):
BOT_USERNAMESstill makes the org's coding agent skip the template check (scripts/pull-request-template.js:99). The thread has no reply. Fix it, or say why the agent is exempt.
Prior-finding status
ACKNOWLEDGED — .github/workflows/pull-request-template.yml:12 — two acceptance criteria have no test
RESOLVED — scripts/pull-request-template.js — any … in a section fails it
UNADDRESSED — scripts/pull-request-template.js:99 — coding agent skips the check via BOT_USERNAMES
RESOLVED — scripts/pull-request-template.js:86 — non-404 read falls back to the org template
RESOLVED — scripts/pull-request-template.js — duplicated template path list
RESOLVED — scripts/pull-request-template.test.js — uppercase-path test passes without the path
RESOLVED — scripts/sync-automation-template.js:365 — sync PR title and body describe only automation.yml
RESOLVED — scripts/sync-automation-template.js:303 — dry run omits pending files
RESOLVED — scripts/sync-automation-template.js — one verdict for all files
RESOLVED — scripts/sync-automation-template.test.js:425 — no test merges verdicts across files
RESOLVED — scripts/sync-automation-template.test.js:50 — no test that the real template carries the marker
Next step: Needs a full code review by a human engineer.
- Workflow steps changed:
.github/workflows/automation-caller.yml,.github/workflows/automation-group-pull-request-target.yml,.github/workflows/automation.yml,.github/workflows/pull-request-template.yml,.github/workflows/sync-automation-template.yml. - Decide whether the org's coding agent should be exempt from the template check.
- Confirm the sync now writing the PR template to consumer repos is how the org wants it rolled out.
Written by rtibblesbot, an LLM-based coding agent.
The check knew one placeholder, the lone `…` line that the shared template uses. morango fills its Summary with `*Short description*`, so a body that kept that line passed. kolibri-design-system has the same hole in its Changelog, where its template writes a bullet skeleton. A required section now fails when its text matches the same section in the template. That needs no list of placeholder strings per repo. A part filled section still passes, because the bullet labels a contributor is meant to keep would otherwise fail.
79c20f7 to
b1b7bac
Compare
The instruction to keep the marker sits next to a paragraph about morango's own file at the same path, so it now names this repo's template and links to it.
The template sync paragraph landed between the list of shared automations and the note that the spreadsheet is not one of them, so the note read as if it were about the synced files.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #116: 10 prior findings resolved, 1 acknowledged; 1 new suggestion inline.
CI passing.
Prior-finding status
RESOLVED — .github/workflows/pull-request-template.yml:12 — two acceptance criteria have no test
RESOLVED — scripts/pull-request-template.js — any … in a filled section fails it
ACKNOWLEDGED — scripts/pull-request-template.js:101 — coding agent skips the template check
RESOLVED — scripts/pull-request-template.js:88 — non-404 read falls back to org template
RESOLVED — scripts/pull-request-template.js — template path list duplicated
RESOLVED — scripts/pull-request-template.test.js — test passes without the uppercase path
RESOLVED — scripts/sync-automation-template.js:365 — sync PR title and body describe only automation.yml
RESOLVED — scripts/sync-automation-template.js:303 — dry-run output omits pending files
RESOLVED — scripts/sync-automation-template.js — one verdict for all files
RESOLVED — scripts/sync-automation-template.test.js:425 — no test for merged verdicts
RESOLVED — scripts/sync-automation-template.test.js:50 — no test that the real template carries the marker
Next step: Needs a full code review by a human engineer.
- Workflow steps changed:
.github/workflows/automation-caller.yml,.github/workflows/automation-group-pull-request-target.yml,.github/workflows/automation.yml,.github/workflows/pull-request-template.yml,.github/workflows/sync-automation-template.yml.
Written by rtibblesbot, an LLM-based coding agent.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #116: 11 of 12 prior findings resolved; 1 still open (see below).
CI passing. The delta is wording only and raises nothing new.
- suggestion (still open):
scripts/pull-request-template.js:61: an untouched multi-line section still passes when the body uses CRLF line endings. Line 21 splits the body on\nonly. Each line keeps its\r, so it never equals the template text. Split on/\r?\n/and add a CRLF test. That thread has no reply yet.
Prior-finding status
RESOLVED — .github/workflows/pull-request-template.yml:12 — two acceptance criteria have no test
RESOLVED — scripts/pull-request-template.js — … anywhere in a section fails it
ACKNOWLEDGED — scripts/pull-request-template.js:101 — coding agent skips the template check
RESOLVED — scripts/pull-request-template.js:88 — non-404 read falls back to the org template
RESOLVED — scripts/pull-request-template.js — path list duplicates the sync script
RESOLVED — scripts/pull-request-template.test.js — uppercase-path test passes without the path
RESOLVED — scripts/sync-automation-template.js:365 — sync PR title/body describe only automation.yml
RESOLVED — scripts/sync-automation-template.js:303 — dry-run output omits pending files
RESOLVED — scripts/sync-automation-template.js — one verdict for all files
RESOLVED — scripts/sync-automation-template.test.js:425 — no test for merged verdicts
RESOLVED — scripts/sync-automation-template.test.js:50 — real template marker not pinned
UNADDRESSED — scripts/pull-request-template.js:61 — untouched multi-line section passes with CRLF bodies
Next step: Needs a full code review by a human engineer.
- Workflow steps changed:
.github/workflows/automation-caller.yml,.github/workflows/automation-group-pull-request-target.yml,.github/workflows/automation.yml,.github/workflows/pull-request-template.yml,.github/workflows/sync-automation-template.yml. - Confirm that the shared PR template check, which runs in every consumer repo, should fail PRs whose required sections are empty or still hold the template text.
- Confirm that the sync now proposing
.github/PULL_REQUEST_TEMPLATE.mdto consumer repos, with a verdict per file, is the rollout you want.
Written by rtibblesbot, an LLM-based coding agent.
The marker line said "Edit it there", which left the reader to carry the repo name across a full stop. It now names the repo and the path. The constant the sync matches on follows the new wording. It names the repo too, so another repo's own template cannot be mistaken for a copy of ours.
The GitHub web form returns a pull request body with CRLF line endings, and the contents API returns the template with LF. Splitting on LF alone left a carriage return on every body line but the last, so a multi line section left as the template wrote it compared as different and passed. Single line sections were unaffected, because the trim removed the lone carriage return.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #116: all 12 prior findings resolved. No new findings. CI passing.
Prior-finding status
RESOLVED — .github/workflows/pull-request-template.yml:12 — Two acceptance criteria have no test
RESOLVED — scripts/pull-request-template.js — Any … in a filled section fails it
RESOLVED — scripts/pull-request-template.js:101 — Coding agent skips the template check
RESOLVED — scripts/pull-request-template.js:88 — Non-404 read falls back to the org template
RESOLVED — scripts/pull-request-template.js — Path list duplicates sync script
RESOLVED — scripts/pull-request-template.test.js — Test passes without the uppercase path
RESOLVED — scripts/sync-automation-template.js:365 — Sync PR title/body describe only automation.yml
RESOLVED — scripts/sync-automation-template.js:303 — Dry-run output omits pending files
RESOLVED — scripts/sync-automation-template.js — One verdict for all files
RESOLVED — scripts/sync-automation-template.test.js:425 — No test for merged verdicts
RESOLVED — scripts/sync-automation-template.test.js:50 — No test that the real template carries the marker
RESOLVED — scripts/pull-request-template.js:61 — Untouched multi-line section passes with CRLF body
Next step: Needs a full code review by a human engineer.
- Workflow steps changed:
.github/workflows/automation-caller.yml,.github/workflows/automation-group-pull-request-target.yml,.github/workflows/automation.yml,.github/workflows/pull-request-template.yml,.github/workflows/sync-automation-template.yml. - Confirm a required-section check that fails PRs across every org repo is the intended rollout, including the bot exemption for the coding agent.
- Confirm the sync script's per-file verdicts, which decide when consumer repos get a template sync PR.
Written by rtibblesbot, an LLM-based coding agent.
The test that used it now passes an empty body, which is what makes the case, so the fixture was left unreferenced and eslint flagged it.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #116: 11 of 12 prior findings resolved. 1 acknowledged. No new findings. CI passing.
Prior-finding status
RESOLVED — .github/workflows/pull-request-template.yml:12 — untested acceptance criteria
RESOLVED — scripts/pull-request-template.js — any … fails a filled section
ACKNOWLEDGED — scripts/pull-request-template.js:101 — coding agent skips the template check
RESOLVED — scripts/pull-request-template.js:88 — non-404 read falls back to org template
RESOLVED — scripts/pull-request-template.js — template path list duplicated
RESOLVED — scripts/pull-request-template.test.js — uppercase path test passes without it
RESOLVED — scripts/sync-automation-template.js:365 — sync PR title/body describe only automation.yml
RESOLVED — scripts/sync-automation-template.js:303 — dry-run omits pending files
RESOLVED — scripts/sync-automation-template.js — one verdict for all files
RESOLVED — scripts/sync-automation-template.test.js:425 — no multi-file verdict tests
RESOLVED — scripts/sync-automation-template.test.js:50 — real template marker untested
RESOLVED — scripts/pull-request-template.js:61 — CRLF multi-line section passes
Next step: Needs a full code review by a human engineer.
- Workflow steps changed:
.github/workflows/automation-caller.yml,.github/workflows/automation-group-pull-request-target.yml,.github/workflows/automation.yml,.github/workflows/pull-request-template.yml,.github/workflows/sync-automation-template.yml. - Confirm the org-wide template check should fail PRs in every consumer repo, given its required-section rules are a new gate others will hit.
- Confirm the template sync PRs it opens in consumer repos are wanted alongside automation.yml.
Written by rtibblesbot, an LLM-based coding agent.
rtibbles
left a comment
rtibbles
left a comment
There was a problem hiding this comment.
Let's undo the unnecessary PULL_REQUEST_TEMPLATE propagation logic.
| /** | ||
| * Opens one pull request per consumer repo whose copy of automation.yml has | ||
| * drifted from automation-template.yml. | ||
| * Opens one pull request per consumer repo whose copy of a shared file has |
There was a problem hiding this comment.
I am a bit confused by the edits to this file. I assumed at first it was trying to do the automatic version of what you had done previously, of conforming custom PR templates to the header markers this script needs - but instead it seems to be copying the PR template from this repository into each repository that doesn't have a custom template?
This is unnecessary, as having a PR template in this repository acts as the PR template in every other repo on this org with out one. I think probably best to revert the PR template copying changes, as they're doing work that Github already handles natively.
There was a problem hiding this comment.
Hrm, I see the rationale is that gh pr create doesn't respect this... but I kind of want people to be paying more attention than that, if that makes sense?
Either they should be manually opening the PR via the web UI and hence getting the org template, or they should know enough (or internally, be using le-skills) to do this properly.
I think reverting this might help us to more directly notice sloppy PRs - and then the remediation message handles it after the fact instead of trying to pre-empt.
There was a problem hiding this comment.
Ohhh, okay. Reverted the changes, should be good i think?
GitHub already applies this repo's template to any repo in the org holding none of its own, so copying the file into each consumer repeated work GitHub does natively. The check stays. It reads a repo's own template when there is one and falls back to this repo's, so it needs no copy in the consumer.
akolson
changed the title
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #116: 11 of 12 prior findings resolved. 1 acknowledged. No new findings. CI passing.
Prior-finding status
RESOLVED — .github/workflows/pull-request-template.yml:12 — two acceptance criteria have no test
RESOLVED — scripts/pull-request-template.js — any … in a filled section fails it
ACKNOWLEDGED — scripts/pull-request-template.js:101 — coding agent skips the template check
RESOLVED — scripts/pull-request-template.js:88 — non-404 read falls back to the org template
RESOLVED — scripts/pull-request-template.js — template path list duplicated across scripts
RESOLVED — scripts/pull-request-template.test.js — uppercase-path test passes without the path
RESOLVED — scripts/sync-automation-template.js — sync PR title/body describe only automation.yml (code removed)
RESOLVED — scripts/sync-automation-template.js — dry-run output omits pending files (code removed)
RESOLVED — scripts/sync-automation-template.js — one verdict for all files (code removed)
RESOLVED — scripts/sync-automation-template.test.js — no test merging verdicts across files (code removed)
RESOLVED — scripts/sync-automation-template.test.js — no test that the real template carries the marker (marker removed)
RESOLVED — scripts/pull-request-template.js:61 — CRLF body passes an untouched multi-line section
Next step: Needs a full code review by a human engineer.
- Workflow steps changed:
.github/workflows/automation-caller.yml,.github/workflows/automation-group-pull-request-target.yml,.github/workflows/automation.yml,.github/workflows/pull-request-template.yml. - Confirm a check that fails contributor PRs on unfilled template sections is wanted across every org repo that calls this workflow.
- Confirm dropping template propagation through the sync (acceptance criterion 1) is settled rather than deferred to a follow-up.
Written by rtibblesbot, an LLM-based coding agent.
3 participants
Summary
Contributors open pull requests that leave required sections of the template empty, and
reviewers ask for the missing information by hand. A body can carry every heading and still
say nothing under them.
The change
pull-request-templateautomation fails the run when the body does not carry thesections its template marks as required. It resolves the template the way GitHub does, the
repo's own copy first and this repo's second, and reads it from the base branch, so a pull
request cannot relax the rules it is judged by.
<!-- REQUIRED -->comment. A templatemarking none requires every level-two heading, so a repo is strict rather than silently
unchecked.
it still holds
…, or it still reads word for word as the template wrote it. Every failure isnamed, because the job log is the only channel. Bot authors are exempt.
contents API returns the template with LF.
editedandreopenedjoin theon:union, so a corrected body clears the check. Consumerstake the widened template on the next sync run.
References
Closes #102.
#115 marked this repo's own template, and the three repos that keep their own took the same
markers in learningequality/kolibri-design-system#1381, learningequality/morango#397 and
learningequality/test-actions#102. All four are merged.
An earlier version of this branch also copied the shared template into every consumer. That is
reverted in c494075. GitHub already applies this repo's template to any repo holding none of its
own, and the check falls back to this repo's template, so it needs no copy in the consumer.
Reviewer guidance
npm test. Expect 122 passing, 19 of them new.scripts/pull-request-template.js. Check that the template is read frompull_request.base.refand never from the head.test-actionsleaving a required section empty. Expectthe check to fail and the log to name that section. Then fill the section in the description.
Expect the next run to pass. Those are cases 1 to 4 of the issue's testing section, and they
can only run after this merges, because the leaf is called at
@main.AI usage
I used Claude Code to write the check and its tests, to confirm that the web form returns CRLF
bodies while the contents API returns LF, and to run the suite and
prek. I decided the markerconvention, which sections are required, and that the check reads the base branch. I reviewed
the diff.