chore(contributing): guard the duplicated CI-gate paragraph and ignore .devcontainer - #663
Conversation
…e .devcontainer .devcontainer/ arrived in libredb#658 and was named in none of the .dockerignore groups, so the production build context carried it while every other non-input dot-directory was listed. Added under "# Development files" beside .vscode and .idea; the payload is tiny, the point is that the file stays readable. The CI-gate paragraph lives in both CONTRIBUTING.md step 4 and .github/curated-issue-footer.md on purpose, and nothing held the copies together. tests/unit/curated-issue-footer.test.ts reads the paragraph out of the footer at runtime - not from a literal, which would just be a third copy that drifts while staying green - and asserts CONTRIBUTING.md still repeats it. Leading whitespace is stripped on both sides because the CONTRIBUTING.md copy is indented three spaces inside numbered list item 4. CONTRIBUTING.md now also points at the footer file, which nothing in the tree referenced, so a maintainer who wants it has a path other than retyping it. Closes libredb#661 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
cevheri
left a comment
There was a problem hiding this comment.
Reviewed by mutation rather than by reading, and the important half is right: the expected wording
is read out of the footer at runtime, and I can prove your test does that rather than hold a copy,
because when I changed a word in the footer the failure message carried the new word. That was the
trap #661 was most worried about and you avoided it.
Five mutations behave exactly as they should. Paragraph deleted from CONTRIBUTING.md: red. One word
changed in the footer: red. One word changed in CONTRIBUTING.md (the direction you did not report,
so I ran it): red. Marker removed from the footer: your guard throws a named error instead of
passing on nothing. Pointer link removed: red. Your reasoning about the coverage gate is also
correct, the file is under tests/ and merge-lcov.mjs filters those records, so the throw line that
never runs cannot break the 100% gate.
Two mutations go red when they should not, both because the comparison is line-exact.
- Re-wrapping the paragraph in CONTRIBUTING.md with every word unchanged, 4 lines to 5: red.
The two copies have different natural margins, the footer flush left and the CONTRIBUTING copy
three spaces deep inside list item 4, and this repo's style asks for one sentence per line, so
somebody will re-flow one of them without touching a word. - A single trailing space at the end of one line: red, and the failure prints two strings that
look identical, which is a bad half hour for whoever hits it.trimStart()leaves it.
Both disappear if the comparison is on wording rather than layout, which is also what "verbatim"
in the issue meant: line breaks are layout. I ran all seven mutations against this and the five
real ones stay red while the two false positives go green.
- .map((line) => line.trimStart())
+ .map((line) => line.trim())
+
+const words = (text: string): string => text.replace(/\s+/g, " ").trim();
test("CONTRIBUTING.md repeats it word for word", () => {
- expect(dedent(read(CONTRIBUTING))).toContain(sharedParagraph());
+ expect(words(read(CONTRIBUTING))).toContain(words(sharedParagraph()));
});Keep your comment explaining the indentation, it is still the reason the naive comparison fails;
it just now covers wrapping too. Optional, take it or leave it: with the comparison independent of
layout, the first test asserting the paragraph spans more than one line is measuring a layout
property. Asserting it is longer than the marker itself would say the same thing without depending
on how it wraps.
Please re-run your two red proofs after the change and put them in the PR body as you did the
first time, so the guard is shown to still fail on real drift.
Verified separately: .devcontainer really is out of the image build context now. I built the
context and listed it, with .github absent as a control and src present as a positive one.
Static gates all green, and a full bun run test on your branch fails exactly the same set my
machine fails on main, so nothing new.
One correction that is mine, not yours. #661 said .dockerignore names every non-input
dot-directory. It does not quite: .cursorrules, .devin, .editorconfig, .nvmrc,
.oxlintrc.json and a few more are still in the context. My framing was stronger than the file
supports. You implemented what was asked, so nothing to change here.
Review on libredb#663: the line-exact comparison went red on two mutations that change no word. Re-wrapping the CONTRIBUTING.md copy failed, and this repository writes one sentence per line while the two copies have different natural margins, so one of them gets re-flowed eventually. A single trailing space also failed, printing two error strings that look identical. Collapsing every run of whitespace covers indentation, wrapping and trailing space together, so dedent() folds into the one normalizer instead of both doing half the job. "Verbatim" in libredb#661 was about the words; line breaks are layout. The first test asserted the paragraph spans more than one line, which was the same layout property. It now asserts the paragraph is longer than the marker it is found by, which is what that test was really for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Taken, both of them. You are right that line breaks are layout, and mutation 6 is the one that convinces me: the two One deviation from your diff, and it is only about how many helpers do the job. Once Optional half taken as well: the first test now asserts the paragraph is longer than the marker it Both red proofs are re-run and back in the PR body, plus the full seven-mutation table so the two One thing I did not touch, because it is your call rather than mine. On a genuine drift the failure And noted on #661's framing — no change needed here, but I would not have caught that myself. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Verified, and folding the two helpers into one is the better call. I re-ran your seven against On the call you left me: do it. Two measurements decided it rather than taste. The failure output goes from 20,510 characters to 1,510, and the two paragraphs land one above the other, so the changed word is visible instead of buried in the whole file printed on one line. It is also more than cosmetic, which is the part neither of us had measured. Append a sentence to And the half you held back is an improvement, not a cost. Here is the diff I measured, with -/** The trimmed, blank-line-delimited paragraph of the footer that opens with `MARKER`. */
-const sharedParagraph = (): string => {
- const found = read(FOOTER)
+/** The trimmed, blank-line-delimited paragraph of `relative` that opens with `MARKER`. */
+const paragraphIn = (relative: string): string => {
+ const found = read(relative)
.split(/\n\s*\n/)
.map((paragraph) => paragraph.trim())
.find((paragraph) => paragraph.startsWith(MARKER));
if (found === undefined) {
// Rewording the footer's opening sentence is allowed; silently leaving this guard with nothing
// to compare is not, because every assertion below would then pass on any CONTRIBUTING.md at all.
- throw new Error(`${FOOTER} no longer contains a paragraph starting with ${MARKER}`);
+ throw new Error(`${relative} no longer contains a paragraph starting with ${MARKER}`);
}
return found;
};
+const sharedParagraph = (): string => paragraphIn(FOOTER);
+
test("CONTRIBUTING.md repeats it word for word", () => {
- expect(words(read(CONTRIBUTING))).toContain(words(sharedParagraph()));
+ expect(words(paragraphIn(CONTRIBUTING))).toBe(words(sharedParagraph()));
});Push that and I will merge. The |
Reading CONTRIBUTING.md's copy with the same finder as the footer's makes the assertion paragraph against paragraph. Containment against the whole file stayed green when a sentence was appended to one copy, since the other's wording is still present somewhere in the file, and on a real drift it printed the entire file as the received string.
|
Pushed it myself as
I re-ran twelve mutations against it. Your seven behave as they did, my four extras behave (indent-only change green; dropped full stop, reordered sentences and a footer cut back to the marker all red), and the appended-sentence case is now red where it was green. Eight static gates green, and mutation 2's message still carries the new word, so the wording is still derived at runtime. None of this is a correction of your work. Your commit is what made the comparison layout-independent; this only narrows what it compares to. If you would rather it read differently, push over it, it is your branch. Not merging until you have had a look. |
Description
Both follow-ups from #658, as spec'd in #661.
Part 1 —
.dockerignore..devcontainer/landed in #658 and was named in none of the groups, so the production build context carried it while every other non-input dot-directory was listed. Added under# Development files, beside.vscodeand.idea. No test is owed here:.dockerignorehas no executable lines forscripts/check-coverage.mjsto measure, and the repository has no guard over the file today.Part 2 — the duplicated CI-gate paragraph.
CONTRIBUTING.mdstep 4 and.github/curated-issue-footer.mdcarry the same**CI is the merge gate.**paragraph on purpose, and today they say the same words. Nothing kept them that way.tests/unit/curated-issue-footer.test.tsnow reads the paragraph out of the footer at runtime and assertsCONTRIBUTING.mdstill repeats it. It deliberately holds no copy of its own — a literal in the test would be the third copy, drifting with the other two while staying green.The comparison is on wording, not layout (this is the change from the first review round). The
CONTRIBUTING.mdcopy is indented three spaces inside numbered list item 4 and the footer copy is flush left; the two also have different natural margins while this repository writes one sentence per line, so one of them will be re-flowed eventually without a word changing. Collapsing every run of whitespace on both sides covers indentation, re-wrapping and a stray trailing space in one step, and the reason is in a comment on the helper. A vacuity guard fails loudly if the footer stops containing a paragraph that opens with the marker, so the comparison can never pass against nothing.CONTRIBUTING.mdalso gains a one-line pointer to.github/curated-issue-footer.mdunderContributor programs— nothing in the tree referenced that file, so a maintainer wanting the footer had to retype it, which is what #618 was trying to stop. The third test pins that pointer.Type of Change
Related Issue
Closes #661
Changes Made
.dockerignore:.devcontainerunder# Development files.tests/unit/curated-issue-footer.test.ts: new drift guard, expected text derived from the footer, compared on wording rather than line layout.CONTRIBUTING.md: pointer to.github/curated-issue-footer.mdin theContributor programslist.Testing
The two red runs the issue asks for, both against
tests/unit/curated-issue-footer.test.ts, re-run onb5faeadafter the review change:CONTRIBUTING.md—2 pass, 1 fail:approve->authorise) —2 pass, 1 fail, and note the expected string now carries the new word, which is the proof it is read from the footer rather than from a literal:Both files restored,
3 pass, 0 fail.All seven mutations from the review, same commit — the five real ones stay red, the two false positives are gone:
CONTRIBUTING.md2 pass, 1 fail)2 pass, 1 fail), expected string carries the new wordCONTRIBUTING.md2 pass, 1 fail)1 pass, 2 fail:.github/curated-issue-footer.md no longer contains a paragraph starting with **CI is the merge gate.**CONTRIBUTING.md2 pass, 1 fail), third testCONTRIBUTING.mdcopy re-wrapped 4 lines to 5, no word changedCONTRIBUTING.mdlineGate, run locally (
bun run test, never barebun test):bun run format— cleanbun run lint— 0 errors (135 pre-existing warnings, unchanged)bun run typecheck— cleanbun run knip— clean (2 pre-existing config hints)bun run readme:check,bun run chart:check,bun run channels:showcase:check,bun run security:check— all OKbun run test—10827 pass / 171 failacross 336 files. Baseline onupstream/mainwith these changes stashed:10824 pass / 171 failacross 335 files. Exactly+3 pass,+1 file, identical failure set.Commands I could not run locally, for CI to verify: the 171 failures above are entirely the
helm-chart-*and packaging suites —helmand7zare not installable in this sandbox, andCONTRIBUTING.mdalready documents that gap. For the same reasonbun run test:coverage && bun run coverage:checkcannot produce a merged lcov here; the change adds no executable lines undersrc/, andscripts/merge-lcov.mjsfilterstests/records out of coverage, so the 100% line gate is untouched either way.bun run buildwas also not run — nothing undersrc/changed andnext builddoes not read.dockerignore.Test Environment
main@efcc0c2)Checklist