ci(release): reject unconfigured commit types and require ports to name upstream - #471
Open
IgorShevchik wants to merge 4 commits into
Open
ci(release): reject unconfigured commit types and require ports to name upstream#471IgorShevchik wants to merge 4 commits into
IgorShevchik wants to merge 4 commits into
Conversation
`changelog-sections` replaces the preset's type list wholesale, so a type
absent from ours has no entry. For an ordinary commit that means it is dropped.
For a breaking one it means something worse: the breaking note keeps the commit
alive, but the section rewrite in `conventional-changelog-conventionalcommits`
is guarded by `if (entry)`, so `commit.type` stays the raw string and becomes
the group title — and `commitGroupOrder.indexOf(title)` returns -1 for an
unknown one, which sorts it above index 0.
Reproduced before writing anything, by running the real writer at the version
release-please uses over this repository's actual config. A `style(Theme)!`
commit renders:
### ⚠ BREAKING CHANGES
* **Theme:** the legacy palette is gone.
### style
* **Theme:** drop the legacy palette
### Features
...
which is #437 exactly: a raw lowercase heading above everything real.
`assert-commit-parses.mjs` now checks the type as well as the parse. The list
comes from `release-please-config.json` rather than being restated — a guard
keeping its own copy of the list is one that will eventually disagree with the
file it guards, silently and in the permissive direction, and a test asserts
the script reads the config so a later simplification has to fail.
Rejecting the type is the second of the two options the issue offered, taken
over adding hidden entries per type. Twelve types appear in this history with
no entry — `playground` 31 times, `doc` 23, `style` 22 — and the list would
never be finished, because it cannot cover `feal(init)`, `ix` or `hore`, which
are the same failure arriving as a typo. Wrong case is rejected too:
release-please would not match `Fix` either, so accepting it hands back a raw
`### Fix` heading.
Swept over every commit reachable on `main`: one rejection, `dcb3bacf`, which
is the commit this guard was originally written about. No new false positives.
`pr-title.yml` runs the same check on the PR title, including on `edited` so a
corrected title turns green without an empty push. It is early feedback and not
the guarantee, and says so: the squash subject can be rewritten in the merge
dialog — #440 was — so the check on `main` stays the thing that is certain.
The title reaches the script through the environment rather than the command
line, since it is arbitrary text from whoever opened the PR.
Refs #437
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012MsMuj8Fic9tjWVjyEyrxc
Only the subject reaches `CHANGELOG.md` — the body is not rendered, breaking
notes aside — so it is the one place a reader of the release notes can be
handed a way back to what was actually ported. Without it a port is
indistinguishable from local work in the only artefact most consumers read.
The subject stays ours. Copying upstream's own first line was the request as
originally put, and it is not what this does: their `Slider` is this fork's
`Range`, and §1 of PORTING.md makes that class of rename mandatory, so their
wording would put a component this library does not ship into our changelog.
The reference points at the commit; the sentence describes what changed here.
fix(Range): forward aria attributes to the thumb (nuxt/ui@d6c3802)
The trigger is a **new key in `processed`**, not the ledger being edited. That
distinction is the whole design: a reconciliation commit — §6 step 4 requires
one whenever a run's last entry has no follower — touches the same file and
ports nothing, and demanding a reference there would be wrong. Checked against
real history rather than fixtures: #467 (bookkeeping) passes, #470 (local work)
passes, #466 and #464 (real ports) are flagged.
Two mistakes worth recording, both of the shape this repository keeps hitting.
The first draft returned an empty array and tested it for truthiness, so every
commit was reported as an unnamed port; running it once was enough. The second
was quieter: without `HEAD^` the check returns nothing and passed silently, and
`actions/checkout` defaults to depth 1 — the same fail-open as the PyYAML
fallback removed in #468. It now warns when it cannot see the previous
revision, and ci.yml fetches depth 2.
`--stdin` skips it: a bare title cannot say what a commit touches, so the
PR-title job checks the parse and the type only. Mutation testing caught that
the case asserting this was vacuous — run from the repository root it passed
whether the guard respected `--stdin` or not, because HEAD there ports nothing.
It now stands on a port revision where the HEAD path does fire.
Refs #437
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012MsMuj8Fic9tjWVjyEyrxc
Review found six defects in the two checks this branch adds, three of them undermining the point of it. Every one was confirmed by running rather than reading. **The type check could be walked past.** `/^([a-z]+)/i` takes a letters-only prefix; the parser's type token runs to the first `(`, `!`, `:` or space. So `fix2(x):` read as `fix` and was accepted, `fix-perf(x):` as `fix`, and `2fix(x):` captured nothing at all — skipping the check and printing "type `undefined` has a section", next to a comment of mine calling that a case that could not happen. All three are types release-please would not match, which is the exact bypass this guard exists to close: a typo in letters was caught, a typo with a digit was not. The type now comes from the AST the parse already produced. The grammar has one implementation and it is not this file. Mutation then showed the fix had no test behind it — the original regex still passed all 75 cases, because every unconfigured type in the spec was letters. Four cases added. **The port check flagged commits that port nothing.** It keyed on any new entry in `processed`, but 70 of the ledger's entries are `no-op`, `noop`, `skip` or `n/a`. #442, #439 and #361 were all flagged — correct messages, on `main`, where they can no longer be fixed. The trigger is now a new entry whose `decision` is `port`. **The new tests would have failed every CI run.** They worktree'd onto real SHAs while this same branch sets `fetch-depth: 2`, so the objects are absent on CI; locally they passed only because a dev clone has the history. Rewritten against a synthetic two-commit repository, which also removes a race that bit this branch for real — a concurrent process restoring the same file twice discarded edits mid-review. Three smaller ones. A batch port was required to name every SHA it added, while §6 4b and this file's own docstring say naming one is enough. The ledger key was interpolated into `new RegExp`, so an entry keyed `(a+)+$` hangs the check — it is a substring test against a key checked to be a SHA now. And an unreadable ledger warned at HEAD^ but was silent at HEAD. Two coverage holes closed on review's evidence: disabling `isMergeCommit()` killed no test, and the "reads the type list from the config" test was a string-containment check that a hardcoded copy passes with the comment intact. It now stands the script beside a config naming a type this repository does not configure and requires it to be accepted. Docs corrected where they promised more than the code does. §7 said CI checks the upstream reference; it does, but only on `push` to `main` — the title job works from a title, which cannot say what a commit touched — so the reviewer owns both halves. `pr-title.yml` now says green there does not mean every rule is satisfied. `AGENTS.md` states both requirements, which until now were discoverable only by reddening CI. Nine mutations that previously killed nothing now each kill at least one case. Follow-up #472 covers the install cost of the title job. Refs #437 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012MsMuj8Fic9tjWVjyEyrxc
`edited` fires for a body edit as well, and GitHub offers no title-only trigger. Observed on this PR's own run: updating the description queued a second check for a title nothing had touched. `github.event.changes` names what actually moved and is absent on every other action, so the condition leaves `opened`, `reopened` and `synchronize` alone and only filters the edit case. Measured while it was still unfiltered: the job takes 28 seconds end to end, rather less than the review feared. #472 still stands — it is about not resolving the whole workspace for one parser — but this removes the reruns that had nothing to check. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012MsMuj8Fic9tjWVjyEyrxc
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Linked issue
Resolves #437
Follow-up filed: #472 (the install cost of the title job).
Type of change
revert(Scope): ...)Description
Two ways a commit reaches the changelog wrong, both silent, and one convention that makes ported work traceable once it gets there.
assert-commit-parses.mjsalready caught a third — a message the parser throws on — and this extends it rather than adding scripts beside it: the family is "this commit will not appear correctly", and it should have one place and one error voice.1. A type with no section (#437)
changelog-sectionsreplaces the preset's type list wholesale, so a type absent from ours has no entry. An ordinary commit of that type is dropped. A breaking one is worse — the note keeps it alive, but the section rewrite inconventional-changelog-conventionalcommitsis guarded byif (entry), socommit.typestays the raw string and becomes the group title, andcommitGroupOrder.indexOf(title)returns-1, which sorts it above index 0.Reproduced before writing anything, by running the real writer at the version release-please uses, over this repository's actual config:
Option 2 from the issue, taken over hidden entries per type: twelve types appear in this history with no entry —
playground31 times,doc23,style22 — and the list would never be finished, because it cannot coverfeal(init),ixorhore.The type comes from the AST the parse already produced, not from a second regex — see the review notes; that distinction turned out to be the difference between a working check and a decorative one. The type list is read from
release-please-config.json, not restated.Swept over every commit reachable on
main: one rejection,dcb3bacf, the commit this guard was originally written about (#436).2. The check moves earlier, without pretending to be the guarantee
pr-title.ymlruns the parse and type checks on the PR title, witheditedin the trigger list so a corrected title turns green without an empty push. Its header says what it is not: the squash subject can be rewritten in the merge dialog — #440 was — and the upstream-reference check cannot run from a title at all, so green there does not mean every rule is satisfied.The title reaches the script through the environment, not the command line, since it is arbitrary text from whoever opened the PR.
3. Ports name the upstream commit
Requested by the maintainer, delivered as a reference rather than upstream's own first line — a departure agreed before implementing.
Only the subject reaches
CHANGELOG.md, so it is the one place a reader can be handed a way back. Upstream's wording would not survive the trip: theirSlideris this fork'sRange, andPORTING.md§1 makes that rename mandatory, so their first line would name a component this library does not ship.The trigger is a new entry whose
decisionisport. Narrower than it looks, and it has to be — 70 of the ledger's entries areno-op,noop,skiporn/a:A batch (§6 4b) names any one of the SHAs it added.
PORTING.md§6 carries the rule; §7 puts both halves on the reviewer, because CI only sees the reference after the squash.Review notes
Six defects, three of them undermining the point of the branch. All confirmed by running.
/^([a-z]+)/ireads a letters-only prefix; the parser's token runs to the first(,!,:or space.fix2(x):read asfixand was accepted;2fix(x):captured nothing, skipped the check and printed "typeundefinedhas a section" — next to my own comment calling that impossiblemain, unfixabledecision === 'port'fetch-depth: 2, so the objects are absent on CInew RegExp— an entry keyed(a+)+$hangs the checkHEAD^and was silent atHEADTwo coverage holes closed on review's evidence: disabling
isMergeCommit()killed no test, and "reads the type list from the config" was a string-containment check that a hardcoded copy passes with the comment intact — it now stands the script beside a config naming a type this repository does not configure and requires it to be accepted.Docs corrected where they promised more than the code does: §7 said CI checks the reference (it does, but only post-squash),
pr-title.ymlnow states its own limits, andAGENTS.mdstates both requirements, which until now were discoverable only by reddening CI.Nine mutations that previously killed nothing now each kill at least one case.
Checklist
Gate run locally with
CI=true:lint·typecheck·test(310 files, 7128 passed, 6 skipped).