Skip to content

[DO NOT MERGE]Open sync pull requests when a consumer's automation.yml drifts - #97

Open
akolson wants to merge 7 commits into
mainfrom
sync-automation-template
Open

akolson wants to merge 7 commits into
mainfrom
sync-automation-template

Conversation

@akolson

@akolson akolson commented Sep 21, 2026

Copy link
Copy Markdown
Member

Summary

Consumer repos hold a copied automation-template.yml, and nothing checked that the copies still
matched. A stale copy fails silently. The repo keeps running its old on: block, so a new
automation never fires there, with no error and no log line.

This is not hypothetical. Six of the eight consumers drifted within days of migrating in #88,
because the template changed after their pull requests were opened. Refreshing them by hand took six
pull requests for a change of one blank line and one comment.

This adds a workflow that compares each consumer's copy against the template and opens one pull
request per repo that differs, in the same shape dependabot uses. There is no accompanying issue,
because the change is a byte copy of one file with nothing to author.

The change

  • automation-registry.yml gains a consumers list. The repos that receive the template now sit
    beside the automations it carries. Each entry names the branch to target, because only three of
    the eight use main.
  • scripts/sync-automation-template.js compares each copy against the template, then opens or
    updates a pull request where they differ. A repo in sync gets nothing. A repo with a sync pull
    request already open gets it updated, never duplicated.
  • .github/workflows/sync-automation-template.yml runs it weekly, on manual dispatch, and when
    automation-template.yml changes on main. Manual dispatch takes a dry_run input. A
    concurrency group stops two runs racing to open the same pull request.
  • A consumer can carry a body_template, and the generated text lands where {{explanation}}
    appears. kolibri-design-system needs one, because its check-description job fails unless the
    body holds a Changelog block. Its template follows that repo's own section order.
  • scripts/sync-automation-template.test.js covers the state machine with an injected client, and
    asserts the payloads that decide correctness: the write targets the sync branch, carries the
    template, and keeps the file sha on an update.
  • docs/automation.md describes what the workflow opens, who merges it, and the four states that
    need a core maintainer.

Drift is classified against the time the template last changed here. A change after the last sync
pull request closed is ordinary drift. Without one, a merged pull request means the consumer
reverted the file, reported as toolchain-conflict, and a closed one means a maintainer declined
it, reported as declined and left alone until the template moves again. An unknown template date
resolves to ordinary drift, because a needless pull request costs one review where a wrong conflict
stops the repo entirely.

The workflow only proposes. It opens pull requests on a branch, never commits to a default branch,
and never merges, approves, or enables auto-merge. Each consumer's own review rules gate the change.

There is no bot pre-review. The copy either matches the template or it does not, and the script
writes those bytes by construction, so there is no content question a reviewer could answer
differently. What the core maintainer decides is whether to accept the file landing in their repo.

References

Refs #95. One acceptance criterion there is still open: confirming that learning-equality-bot[bot]
holds contents: write and pull-requests: write on all eight consumers. That cannot be checked
without running as the app, so the first dispatch answers it. Please record the result on #95 and
close it then.

Follows #88, which introduced the template and the generator.

Reviewer guidance

I ran the read and compare path against the live org with --dry-run, and it matches an audit I did
by hand. All eight pre-commit hooks pass, actionlint included, the generator reports no drift after
the registry change, and the 60 tests pass. The write path has never run.

  1. Run GITHUB_TOKEN=$(gh auth token) node scripts/sync-automation-template.js --dry-run. Expect
    eight rows. Repos whose refresh pull request has not merged yet report would-open.
  2. Read the consumers list against the base branches of the eight migration pull requests from
    Consolidate shared workflows into a single generated automation.yml entry point #88. Expect develop for kolibri, kolibri-design-system and kolibri-data-portal, unstable for
    studio, release-v0.9.x for morango, and main for the rest.
  3. Check that the script never merges and never writes to a base branch. All writes target the
    automation-template-sync branch, and a test asserts it.
  4. Do not dispatch it for real until the remaining refresh pull requests merge, in studio,
    kolibri-data-portal and morango. They use a different branch name, so a run today would open
    duplicates. After they merge, a dispatch should report all eight in sync.

AI usage

I used Claude Code to audit the drift across the eight consumers, to write the script, workflow and
tests, and to draft this description. I directed the design decisions: pull requests rather than
issues, the workflow doing the copy itself rather than delegating it to an agent, the consumer list
living in the registry, no bot pre-review on a change with nothing to judge, and human review as a
hard constraint. I verified the dry run against the live org, confirmed the hooks and tests
pass, and checked that the new payload assertions fail when the write is pointed at a default
branch.

Consumers hold a copied automation-template.yml, and nothing checked that the
copies still matched. A stale copy fails silently: the repo keeps running its
old on: block, so a new automation never fires there.

Six of the eight drifted within days of migrating, because the template changed
after their pull requests were opened. That is the case this addresses.

The workflow compares each consumer's copy against the template and opens one
pull request per repo that differs. A repo already in sync gets nothing, and a
repo with a sync pull request open gets it updated rather than duplicated. It
only proposes: it opens pull requests on a branch, never commits to a default
branch, and never merges, approves, or enables auto-merge.

The consumer list lives in automation-registry.yml, next to the automations it
distributes. Each entry carries the branch to target, because most consumers do
not use main. kolibri-design-system also carries a body prefix, because its
check-description job fails unless the description holds a Changelog block.

A repo whose own tooling rewrites the copy would generate a pull request
forever, so a merged sync pull request followed by fresh drift is reported as a
toolchain conflict instead.
@akolson
akolson requested a review from rtibblesbot September 21, 2026 16:59
@rtibblesbot

rtibblesbot commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

🟡 Waiting for changes

Last updated: 2026-09-22 17:13 UTC

@rtibblesbot rtibblesbot 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.

PR #97 is registry-driven and propose-only, as #95 specified. Two script bugs surface only in production. The rest are resilience and test coverage.

  • blocking — every consumer that merges a sync PR stops receiving sync PRs (sync-automation-template.js:88)
  • blocking — a repo the app cannot read is reported not-migrated and the run exits 0 (sync-automation-template.js:61)
  • suggestion — one transient API failure ends the run with no report (:140)
  • suggestion — the fixed branch is reused after a maintainer closes a sync PR unmerged (:97)
  • suggestion — no tests for the state machine (:79)
  • nitpick — no concurrency group on a workflow that writes (sync-automation-template.yml:19)

CI pending. No UI surface in the diff. The frontend lens produced no findings, and UI verification and manual QA did not apply.


@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

Comment thread scripts/sync-automation-template.js Outdated
if (copy.content === template) return { repo, state: 'in-sync' };

const existing = await openSyncPr(repo);
if (!existing && (await revertedAfterMerge(repo))) {

@rtibblesbot rtibblesbot Sep 21, 2026

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.

Resolved — addressed in the current code.

blocking: Every consumer that merges a sync PR stops receiving sync PRs from then on. revertedAfterMerge returns true for any merged prior sync PR (line 76). It never compares the consumer's copy against what that PR installed. So the next upstream template change is classified toolchain-conflict rather than ordinary drift. No PR is opened and the run exits 1 (line 156). #95 also asks for that report only after a revert-and-reopen happens more than once.

Please tell the two cases apart using the template's history. One option: compare the merge time against the last commit touching automation-template.yml (GET /repos/learningequality/.github/commits?path=automation-template.yml&per_page=1), where a later template commit means ordinary drift. The other: record the synced template SHA in the PR body and read it back.

Comment thread scripts/sync-automation-template.js Outdated

async function currentCopy(repo, ref) {
const r = await api('GET', `/repos/${ORG}/${repo}/contents/${TARGET_PATH}?ref=${ref}`);
if (r.status === 404) return { missing: true };

@rtibblesbot rtibblesbot Sep 21, 2026

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.

Resolved — addressed in the current code.

blocking: A repo the app cannot access is reported as not-migrated and the run still exits 0. GitHub returns 404, not 403, for a repo the token cannot see. That 404 maps to not-migrated (line 84), which is excluded from problems (line 149). So there is no ::error annotation and the overall verdict reads "in sync" for a repo the workflow never reached. #95 names this case directly: "A repo the workflow cannot write to is reported, not skipped silently."

Please probe with GET /repos/{ORG}/{repo} to disambiguate. Every entry in consumers exists, so a 404 there means no access and should map to error. All eight repos are migrated per #88, so also annotate not-migrated and exit non-zero rather than printing a quiet row.

const template = fs.readFileSync(TEMPLATE_PATH, 'utf8');

const results = [];
for (const consumer of registry.consumers) {

@rtibblesbot rtibblesbot Sep 21, 2026

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.

Resolved — addressed in the current code.

suggestion: One transient API failure ends the run with no report at all. There is no per-repo error handling and no catch in main(). api() calls JSON.parse(text) unconditionally (line 42), so GitHub's HTML 502 page throws. The loop then unwinds as an unhandled rejection and the accumulated results never print.

Please wrap syncRepo in a try/catch that records { state: 'error', detail }. That keeps the other seven repos moving and preserves the table. Guarding the JSON.parse with a raw-text fallback makes the detail readable.

Separately, openSyncPr returns null when its list request fails (line 68). A 403 then reads as "no open PR", and the later POST /pulls fails with a 422 that hides the real cause.

Comment thread scripts/sync-automation-template.js Outdated
if (!baseRef.ok) return { repo, state: 'error', detail: `base ${base} not found` };

if (!existing) {
const made = await api('POST', `/repos/${ORG}/${repo}/git/refs`, {

@rtibblesbot rtibblesbot Sep 21, 2026

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.

Resolved — addressed in the current code.

suggestion: After a maintainer closes a sync PR without merging, the next run re-proposes the same change from the same branch. BRANCH is a constant and ref creation tolerates 422 (line 101), so the branch outlives the closed PR. revertedAfterMerge is false in that case. The new PR therefore carries the stale commits and re-asks weekly for a change a human declined.

Please reset the ref to the base SHA (PATCH /git/refs/heads/{BRANCH} with force: true) whenever no PR is open. For the re-proposal itself, skip repos whose most recent sync PR was closed unmerged.

Comment thread scripts/sync-automation-template.js Outdated
return Boolean(r.data[0].merged_at);
}

async function syncRepo(consumer, template) {

@rtibblesbot rtibblesbot Sep 21, 2026

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.

Resolved — addressed in the current code.

suggestion: The state classification in syncRepo has no test coverage. It is pure given a stubbed api. scripts/pr-statistics.test.js already establishes the injected-client pattern that node --test scripts/ runs in CI. Both blocking findings above are reachable only in production because #95's test plan is manual across eight live repos.

Please make api injectable and assert each state.

- automation-template.yml
permissions:
contents: read
jobs:

@rtibblesbot rtibblesbot Sep 21, 2026

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.

Resolved — addressed in the current code.

nitpick: Two overlapping runs can both try to open the sync PR, and the loser reports a 422. A push to main touching automation-template.yml can start while the weekly cron is still running. Both see no open PR and both POST /pulls.

Please add concurrency: { group: sync-automation-template, cancel-in-progress: false }.

Comment thread automation-registry.yml Outdated
base: develop
# check-description fails unless the body carries a Changelog block whose
# Description line is not the template placeholder.
body_prefix: |

@rtibblesbot rtibblesbot Sep 21, 2026

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.

Resolved — addressed in the current code.

praise: KDS's check-description exception lives in the registry as a data field, not as a repo-name branch in the script. The next consumer needing a description check is a data change.

A sync pull request now requests a review from rtibblesbot when it is opened,
so it gets a first pass before a core maintainer looks at it. Dependabot pull
requests already work this way.

A failed reviewer request does not fail the sync. The pull request is already
open at that point, so the run notes it and carries on.
A merged sync pull request made every later template change look like a
toolchain conflict, so a repo would have received one sync and then silently
stopped receiving them. Drift is now classified against the time the template
last changed here. A change after the pull request closed is ordinary drift.
Without one, a merged pull request means the consumer reverted the file, and a
closed one means a maintainer declined it. A declined repo is left alone until
the template moves again.

GitHub answers 404 both for a missing file and for a repo the token cannot see,
and the second case was reported as not-migrated and exited 0. A 404 now probes
the repo itself to tell the two apart, and not-migrated counts as a problem,
because every listed consumer is already migrated.

One failed request used to end the run with no output at all. Each repo is now
wrapped, a response that is not JSON no longer throws, and a failed pull request
listing reports an error instead of reading as no open pull request. The sync
branch is reset to base when no pull request is open, so a closed one cannot
leave commits behind for the next run to carry.

The state machine has tests with an injected client, covering each state. Both
of the bugs above were reachable only in production.
@akolson
akolson requested a review from rtibblesbot September 21, 2026 17:24

@rtibblesbot rtibblesbot 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.

PR #97: 4 new blocking findings and 1 suggestion, all in code added since the last round (see inline). All 7 prior findings are resolved or acknowledged.

The state machine is right now, and the suite covers every classification. What is missing sits around it. The one call the classification depends on has no error path. The suite never asserts a write payload or the exit-code function. The operator runbook no longer lists every state that stops a merge.

CI passing. No UI files, so no visual verification or manual QA applied.

Prior-finding status

RESOLVED - scripts/sync-automation-template.js:88 - every consumer that merges a sync PR stops receiving sync PRs
RESOLVED - scripts/sync-automation-template.js:61 - inaccessible repo reported as not-migrated with exit 0
RESOLVED - scripts/sync-automation-template.js:185 - one transient API failure ends the run with no report
RESOLVED - scripts/sync-automation-template.js:97 - closed-unmerged PR re-proposed from a stale branch
RESOLVED - scripts/sync-automation-template.js:79 - no test coverage for the state classification
RESOLVED - .github/workflows/sync-automation-template.yml:22 - overlapping runs both POST /pulls
ACKNOWLEDGED - automation-registry.yml:38 - KDS description exception lives in the registry


@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

Comment thread scripts/sync-automation-template.js Outdated

async function lastTemplateChange(api) {
const r = await api('GET', `/repos/${ORG}/.github/commits?path=automation-template.yml&per_page=1`);
if (!r.ok || !r.data.length) return null;

@rtibblesbot rtibblesbot Sep 21, 2026

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.

Resolved — addressed in the current code.

blocking: A transient API failure here silently stops syncing a repo. lastTemplateChange has no error path, and every consumer's classification depends on its result.

A non-ok response returns null. classifyDrift then skips the timestamp comparison, and every consumer with a merged sync pull request falls through to toolchain-conflict. No pull request opens and the run exits 1. The annotation tells maintainers the template is not stable under that toolchain — for a 502.

A throw (null body, socket hang-up, missing commit) is worse. The call sits outside the per-repo try at line 186, and main() has no .catch(). The process dies with no table and no annotations.

Fail toward drift, not toward a stop. An unknown template date costs a human one review. A wrong toolchain-conflict stops syncing that repo and points the maintainer at the wrong file. Distinguish a failed request from "no commits found" — only the latter justifies null — and wrap the call in the same try/catch shape used at line 186.

const test = require('node:test');
const assert = require('node:assert/strict');

const { run, classifyDrift, BRANCH } = require('./sync-automation-template');

@rtibblesbot rtibblesbot Sep 21, 2026

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.

Resolved — addressed in the current code.

blocking: The prior round's exit-code fix can regress with CI green. report and PROBLEM_STATES are exported (sync-automation-template.js:224) and never exercised by a test.

report is the entire failure signal: main() does process.exit(report(results) ? 1 : 0), and the workflow step is a bare node scripts/sync-automation-template.js. Mutating PROBLEM_STATES to ['error'] leaves all 14 tests passing. That mutation reinstates the prior blocking finding — an inaccessible repo exits 0.

Feed report a hand-built mixed result set (in-sync, declined, not-migrated, error) and assert the returned count. Name declined explicitly there: it is deliberately not a problem state, and that decision should be a test rather than an omission. Add a case that sets reviewerFailed, which no test currently does.

assert.equal((await only(baseRoutes(TEMPLATE))).state, 'in-sync');
});

test('a drifted copy opens a pull request and requests the reviewer', async () => {

@rtibblesbot rtibblesbot Sep 21, 2026

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.

Resolved — addressed in the current code.

blocking: No test asserts a write payload. A commit to the consumer's default branch would pass the suite.

Both mutations are green against the current suite (14 pass, 0 fail):

  • PUT body branch: BRANCH -> branch: 'main'
  • PUT body content: <template> -> content: 'WRONG'

The first breaks the hard requirement from #95 — never commit to a default branch. On eight consumers it commits straight to develop, unstable and release-v0.9.x. The second means the suite cannot tell a correct sync from one that writes garbage.

calls is already captured here, so assert both fields:

const put = calls.find((c) => c.method === 'PUT');
assert.equal(put.body.branch, BRANCH);
assert.equal(Buffer.from(put.body.content, 'base64').toString('utf8'), TEMPLATE);

Dropping sha: onBranch.sha from that body is also green, and fails for real with a 422 on every update. Assert it too, in the update test at line 60.

Comment thread docs/automation.md Outdated

Run it with `dry_run` to see which repos have drifted without opening anything.

Two results need a core maintainer rather than a merge:

@rtibblesbot rtibblesbot Sep 21, 2026

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.

Resolved — addressed in the current code.

blocking: This section promises "Two results", but the code now emits four states. Two of them are undocumented.

  • not-migrated joined PROBLEM_STATES (sync-automation-template.js:28). It emits ::error title=<repo>::not-migrated and exits 1. A maintainer who sees the weekly run go red on that word finds nothing here.
  • declined is new. It stops sync for that repo until the template next moves, and exits 0. A repo can sit drifted with a green workflow, and the docs never say that closing a sync pull request has that effect.

Document both and fix the count. For not-migrated, the fix is installing or onboarding the app. For declined, reopening sync means waiting for the next template change or opening the pull request by hand.


const TEMPLATE = 'name: Automation\non: {}\n';
const STALE = 'name: Automation\non: {old: true}\n';
const REGISTRY = { consumers: [{ repo: 'demo', base: 'main' }] };

@rtibblesbot rtibblesbot Sep 21, 2026

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.

Resolved — addressed in the current code.

suggestion: The fixture pins base: 'main', the one value that cannot catch a hardcoded base. Only three of the eight real consumers use main; the base field exists because five do not. Today both GET git/ref/heads/${base} -> heads/main and POST /pulls base -> 'wrong' stay green.

Switch the fixture and its route to base: 'develop'. That costs nothing and makes both fail. Assert the POST /pulls body at the same time: reviewers: ['nobody'] and ignoring body_prefix are both green today, and the second silently fails KDS's check-description on every run.

async function readCopy(api, repo, ref) {
const r = await api('GET', `/repos/${ORG}/${repo}/contents/${TARGET_PATH}?ref=${ref}`);
if (r.ok) return { sha: r.data.sha, content: Buffer.from(r.data.content, 'base64').toString('utf8') };
// A repo the token cannot see answers 404, exactly like a missing file, so ask

@rtibblesbot rtibblesbot Sep 21, 2026

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.

Resolved — addressed in the current code.

praise: The comment names the ambiguity — missing file and invisible repo both answer 404. That makes the extra request read as necessary rather than defensive. Same for the reset comment at line 146.

A failed commits request returned null, which made classifyDrift skip the
comparison and label every consumer with a merged sync pull request a toolchain
conflict. A transient error would have stopped syncing those repos and told
maintainers their tooling rewrites the file. The call also sat outside the
per-repo error handling, so a malformed body killed the run with no table.

An unknown template date now resolves to ordinary drift. A needless pull request
costs one review, where a wrong toolchain conflict stops the repo entirely.

The suite now asserts the things that decide correctness rather than only the
classification. The write must target the sync branch, carry the template, and
keep the file sha on an update. The pull request must target the consumer's own
base and name the reviewer, and a body prefix must reach the body, because
kolibri-design-system fails its description check without one. The fixture base
moved off main, which is the one value that cannot catch a hardcoded base.

report and PROBLEM_STATES are tested directly, since they decide whether the
workflow goes red. Narrowing PROBLEM_STATES now fails a test rather than
quietly reinstating an inaccessible repo exiting 0.

The docs listed two states needing attention where the code emits four.
@akolson
akolson requested a review from rtibblesbot September 21, 2026 17:47

@rtibblesbot rtibblesbot 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.

PR #97 — 12 of 13 prior findings are resolved. One stays open, a nitpick, recorded below. CI is passing. No UI files, so no visual verification.

The new tests hold under mutation. Adding 'declined' to PROBLEM_STATES fails a test. Reverting the null-date handling in classifyDrift fails another. One new suggestion inline: a test that only restates the constant it checks.

.github/workflows/sync-automation-template.yml:22nitpick (unchanged since the last round, recorded not re-argued): two overlapping runs can both open the sync PR, and the loser reports a 422. Add a concurrency group to the workflow, or leave it as-is if the duplicate run is rare enough that the 422 is just noise.

Prior-finding status

RESOLVED — scripts/sync-automation-template.js — revertedAfterMerge stopped every merging consumer from receiving further sync PRs
RESOLVED — scripts/sync-automation-template.js — an inaccessible repo was reported not-migrated with exit 0
RESOLVED — scripts/sync-automation-template.js:198 — one transient API failure ended the run with no report
RESOLVED — scripts/sync-automation-template.js — lastTemplateChange had no error path, silently stopping a repo
RESOLVED — scripts/sync-automation-template.js — a declined sync PR was re-proposed on the next run
RESOLVED — scripts/sync-automation-template.test.js — state classification in syncRepo had no test coverage
RESOLVED — scripts/sync-automation-template.test.js — report/PROBLEM_STATES untested, so the exit-code fix could regress green
RESOLVED — scripts/sync-automation-template.test.js:56 — no test asserted the write payload
RESOLVED — scripts/sync-automation-template.test.js — fixture pinned base: 'main', the one value that cannot catch a hardcoded base
RESOLVED — docs/automation.md — the section promised "Two results" while the code emits four
ACKNOWLEDGED — scripts/sync-automation-template.js:74 — praise: the comment names the missing-file/invisible-repo 404 ambiguity
ACKNOWLEDGED — automation-registry.yml:38 — praise: KDS check-description exception lives in the registry as data
UNADDRESSED — .github/workflows/sync-automation-template.yml:22 — overlapping runs both open the sync PR, loser 422s


@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

}
});

test('declined is deliberately not a problem state', () => {

@rtibblesbot rtibblesbot Sep 21, 2026

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.

Resolved — addressed in the current code.

suggestion: Delete this test — it restates the PROBLEM_STATES declaration and catches nothing the report test above already catches.

That test (line 212) feeds report() a declined row alongside all three problem states. Adding 'declined' to PROBLEM_STATES fails it on its own. Removing 'not-migrated' fails it too. I confirmed both by mutation.

The one case unique to this test is adding a genuinely new problem state, which fails the deepEqual even after the report test has been updated to cover it. That leaves a list to edit in two places and no defect caught in between.

If the point is that the set is closed and any addition should force a second look, keep it and say that in an assertion message. With no message it reads as a mirror of the declaration.

assert.equal(calls.filter((c) => c.method !== 'GET').length, 0);
});

test('an unreadable template history proposes rather than stopping the repo', async () => {

@rtibblesbot rtibblesbot Sep 21, 2026

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.

Resolved — addressed in the current code.

praise: This one pins the outcome rather than the mock. It stubs the history call to fail while keeping a merged closed PR in the routes. So state === 'opened' can only pass if the unknown date really resolves to drift, not toolchain-conflict. Reverting the null-date branch fails it.

The report test already fails when a state is added to or removed from
PROBLEM_STATES, so the membership assertion next to it caught nothing. What is
left is the closed-set check, which only fires when someone adds a state the
report test does not cover. The assertion message now says that, instead of the
test reading as a copy of the declaration.
@akolson

akolson commented Sep 21, 2026

Copy link
Copy Markdown
Member Author

The concurrency group is already in the workflow. It landed in 06b3ee1, two commits before this review:

concurrency:
  group: sync-automation-template
  cancel-in-progress: false

The previous review listed the same finding as resolved at the same line, so this one looks like a stale status rather than a new observation.

The test suggestion is done in 4cc6deb. The membership assertion was redundant, as you showed, so it is gone. The closed-set check stays, now with a message saying it guards against widening PROBLEM_STATES and turning the weekly run red for repos that were passing.

body_prefix could only put text above the generated explanation, so the
Changelog block led the body. kolibri-design-system#1327 shows that repo's real
shape: Description first, Changelog fifth, Steps to test after it. A prefix
cannot express that, because the explanation belongs inside the first section
and the Changelog in the middle.

body_template replaces it. The generated text goes where {{explanation}}
appears, so the body follows the repo's own section order. A consumer without a
template still gets the plain explanation.

kolibri-design-system now gets a body shaped like its template. The parser in
check-description still finds the Description line, because it only needs that
line between the Changelog heading and the next line holding two hashes.
@akolson akolson changed the title Open sync pull requests when a consumer's automation.yml drifts [DO NOT MERGE]Open sync pull requests when a consumer's automation.yml drifts Sep 21, 2026
A sync pull request replaces one generated file with a known upstream file. The
copy either matches automation-template.yml on main or it does not, and the
script writes those bytes by construction, so there is no content question a
reviewer could answer differently. A pre-review would post a generated comment
on eight pull requests every time the template changes, saying nothing.

The dependabot comparison that justified it does not hold either. A dependabot
pull request carries a version jump with changelog and breaking-change risk.
This one carries no judgment at all.

What the core maintainer decides is whether to accept the file landing in their
repo, which is consent rather than review. That step is unchanged.

@rtibblesbot rtibblesbot 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.

PR #97 is approved. All 15 prior findings are resolved or acknowledged. The new commits raise no new findings. The four ACKNOWLEDGED items stay as they are — no action needed before merge.

Prior-finding status

RESOLVED — scripts/sync-automation-template.js — every consumer that merges a sync PR stops receiving sync PRs
RESOLVED — scripts/sync-automation-template.js — an inaccessible repo reported as not-migrated with exit 0
RESOLVED — scripts/sync-automation-template.js:194 — one transient API failure ends the run with no report
RESOLVED — scripts/sync-automation-template.js — a closed-without-merge sync PR is re-proposed every run
RESOLVED — scripts/sync-automation-template.js — syncRepo state classification has no test coverage
RESOLVED — scripts/sync-automation-template.js — lastTemplateChange has no error path
RESOLVED — .github/workflows/sync-automation-template.yml:22 — two overlapping runs race to open the same PR
RESOLVED — scripts/sync-automation-template.test.js — the exit-code fix can regress with CI green
RESOLVED — scripts/sync-automation-template.test.js — no test asserts a write payload
RESOLVED — scripts/sync-automation-template.test.js — the fixture pins base: 'main'
RESOLVED — docs/automation.md — "Two results" promised, four states emitted
ACKNOWLEDGED — scripts/sync-automation-template.test.js — delete the test that restates PROBLEM_STATES
ACKNOWLEDGED — scripts/sync-automation-template.test.js:192 — the history-failure test pins the outcome, not the mock
ACKNOWLEDGED — automation-registry.yml — the KDS exception lives in the registry as a data field
ACKNOWLEDGED — scripts/sync-automation-template.js:74 — the comment names the 404 ambiguity


@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?

Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:

  • 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 core review pass only
  • Specialized frontend/backend lenses and manual QA run only when a review is explicitly requested
  • Synthesized one review from the passes
  • Chose the verdict from the findings, CI status, and QA evidence

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.

2 participants