From f32762a0dcae6598d770100e77c5d4dc4c65b722 Mon Sep 17 00:00:00 2001 From: bluestreak Date: Fri, 4 Sep 2026 13:52:38 +0100 Subject: [PATCH 1/3] ci(build): replace Danger with a dependency-free PR title check Danger JS read the pull request title and reported on it through the questdb-butler account, using a personal access token held in the DANGER_GITHUB_TOKEN secret. Nothing about the job needed a separate identity, and the token's expiry would have quietly stopped the check: Danger is not a required status here, so it would have failed open rather than loudly. ci/validate-pr-title/check.js now does the same two things on the workflow's own GITHUB_TOKEN: it posts the commit status and leaves a comment explaining a rejection, updating that comment in place while the title stays wrong and deleting it once the title is fixed. The yarn global add danger step and dangerfile.js are gone, and the scripts use only the Node standard library. DANGER_GITHUB_TOKEN can be deleted from the repository secrets once this has settled. The status context is "PR title", not the "Danger" that Danger posted. The rename is safe here and was checked rather than assumed: branch protection on main requires only the four questdb.java-questdb-client Azure contexts, and every open pull request that carries a Danger status carries a green one, so nothing is left waiting on a check that stopped reporting. The copy in questdb/questdb cannot do this, because its master ruleset names "Danger" exactly. The subType list is unchanged. It already matches what this repository merges: across 69 merged and 8 open pull requests only qwp, build, ilp, core and conf appear, all of them already allowed. The rules gain one thing, the Conventional Commits breaking-change marker, so feat(qwp)!: passes. That is strictly more permissive and cannot redden a title that passes today. validate.test.js loses a long-standing hole: testInvalid passed a bare undefined onError, so the assertion was satisfied by a ReferenceError rather than by the title being rejected, and every negative case passed against a validator that accepts everything. Replacing the regex body with an unconditional accept now fails the suite; before this change it passed. check.test.js covers the reporting against a stubbed API, including the comment lifecycle, the merge-group path, and the fail-closed behaviour that publishes a verdict even when the run comes apart. Two details are load-bearing and pinned by tests: the status must be posted on pull_request.head.sha, since GITHUB_SHA on a pull_request event is the throwaway merge commit and a status there is invisible to the pull request; and synchronize has to stay in the trigger list, because the status is attached to a commit and a new head needs its own verdict. The tests run in their own job rather than as a first step of the job that posts the verdict, so a failing test cannot stop check.js from running and leave a corrected title still carrying its rejection comment. --- .github/workflows/danger.yml | 26 --- .github/workflows/pr_title.yml | 51 ++++++ ci/validate-pr-title/check.js | 241 ++++++++++++++++++++++++ ci/validate-pr-title/check.test.js | 254 ++++++++++++++++++++++++++ ci/validate-pr-title/dangerfile.js | 4 - ci/validate-pr-title/readme.md | 34 +++- ci/validate-pr-title/validate.js | 13 +- ci/validate-pr-title/validate.test.js | 59 ++++-- 8 files changed, 632 insertions(+), 50 deletions(-) delete mode 100644 .github/workflows/danger.yml create mode 100644 .github/workflows/pr_title.yml create mode 100644 ci/validate-pr-title/check.js create mode 100644 ci/validate-pr-title/check.test.js delete mode 100644 ci/validate-pr-title/dangerfile.js diff --git a/.github/workflows/danger.yml b/.github/workflows/danger.yml deleted file mode 100644 index 53a07c505..000000000 --- a/.github/workflows/danger.yml +++ /dev/null @@ -1,26 +0,0 @@ -name: Danger - -on: - pull_request: - types: [ synchronize, opened, reopened, edited ] - -jobs: - build: - if: ${{ github.event.pull_request.head.repo.full_name == github.repository }} # Only run on non-forked PRs - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@master - - name: Use Node.js 22.x - uses: actions/setup-node@master - with: - node-version: 22.x - - name: install danger - run: yarn global add danger - - name: Validate PR title validation rules - working-directory: ./ci/validate-pr-title - run: node validate.test.js - - name: Danger - run: danger ci - working-directory: ./ci/validate-pr-title - env: - DANGER_GITHUB_API_TOKEN: ${{ secrets.DANGER_GITHUB_TOKEN }} diff --git a/.github/workflows/pr_title.yml b/.github/workflows/pr_title.yml new file mode 100644 index 000000000..3b88dcd97 --- /dev/null +++ b/.github/workflows/pr_title.yml @@ -0,0 +1,51 @@ +name: PR title + +on: + pull_request: + # synchronize is here even though pushing cannot change a title: the status is + # attached to the head commit, so every new head needs its own verdict or the + # check reads as missing against the commit the pull request is showing. + types: [ synchronize, opened, reopened, edited ] + merge_group: + +permissions: {} + +jobs: + # Deliberately a separate job rather than a first step of `validate`. Run in one + # job, a broken or flaky test would stop check.js from running at all, which + # leaves a fixed title still carrying its rejection comment and no status posted. + # The rules and the reporting are worth failing on, but not at the cost of the + # verdict itself. + test: + runs-on: ubuntu-latest + permissions: + # The top-level `permissions: {}` above leaves a job that declares nothing + # with a token that has no scopes. Cloning works here regardless because the + # repository is public, but saying so keeps the job working if that changes. + contents: read + steps: + - uses: actions/checkout@v5 + # No setup-node step: the runner ships a current Node, and nothing under + # ci/validate-pr-title depends on anything outside the standard library. + - name: Test the title rules and the reporting + working-directory: ./ci/validate-pr-title + run: | + node validate.test.js + node check.test.js + + validate: + # A pull request from a fork gets a read-only token, so this job could post + # neither the status nor the comment. Same condition the Danger job carried. + if: ${{ github.event_name == 'merge_group' || github.event.pull_request.head.repo.full_name == github.repository }} + runs-on: ubuntu-latest + permissions: + contents: read + statuses: write # the "PR title" commit status + pull-requests: write # write, update and delete the explanation comment + steps: + - uses: actions/checkout@v5 + - name: Validate the pull request title + working-directory: ./ci/validate-pr-title + env: + GITHUB_TOKEN: ${{ github.token }} + run: node check.js diff --git a/ci/validate-pr-title/check.js b/ci/validate-pr-title/check.js new file mode 100644 index 000000000..2babeea98 --- /dev/null +++ b/ci/validate-pr-title/check.js @@ -0,0 +1,241 @@ +"use strict"; + +// Validates a pull request title and reports the verdict to GitHub: it posts a +// commit status and leaves a comment explaining a rejection, updating that comment +// in place while the title stays wrong and deleting it once the title is fixed. +// Everything here runs on the workflow's own GITHUB_TOKEN and uses only the Node +// standard library, so the job installs nothing at run time. + +const fs = require("node:fs"); +const { validate } = require("./validate"); + +// Hidden marker on the comment this job writes, so a later run can find that same +// comment and update or delete it rather than stacking a new one on every push. +const MARKER = ""; + +// The commit status context. Danger posted "Danger" here, but nothing required +// that string: the branch protection on main requires only the four Azure +// contexts, so the rename costs nothing and no open pull request is left waiting +// on a check that stopped reporting. The OSS copy is stuck with "Danger" because +// its master ruleset does name that exact string. Making this a required check is +// a branch-protection edit, and until then a rejection is visible but not +// blocking, which is what Danger already was here. +const CONTEXT = "PR title"; + +const apiUrl = process.env.GITHUB_API_URL || "https://api.github.com"; +const repo = process.env.GITHUB_REPOSITORY; + +function sleep(ms) { + return new Promise((resolve) => setTimeout(resolve, ms)); +} + +// GitHub occasionally answers a write with a 5xx, and a dropped verdict is worse +// than a slow one, so transient failures are retried. A 4xx is a permanent answer +// about this request, so it fails immediately instead of burning the retries. +async function request(method, path, body) { + let lastError; + for (let attempt = 1; attempt <= 3; attempt++) { + if (attempt > 1) { + await sleep(2000 * (attempt - 1)); + } + let response; + try { + response = await fetch(`${apiUrl}${path}`, { + method, + headers: { + accept: "application/vnd.github+json", + authorization: `Bearer ${process.env.GITHUB_TOKEN}`, + "x-github-api-version": "2022-11-28", + ...(body ? { "content-type": "application/json" } : {}), + }, + ...(body ? { body: JSON.stringify(body) } : {}), + }); + } catch (error) { + lastError = error; + continue; + } + if (response.status === 204) { + return null; + } + if (response.ok) { + return response.json(); + } + const detail = await response.text().catch(() => ""); + lastError = new Error( + `${method} ${path} answered ${response.status}: ${detail.slice(0, 300)}` + ); + if (response.status < 500 && response.status !== 429) { + break; + } + } + throw lastError; +} + +function readEvent() { + return JSON.parse(fs.readFileSync(process.env.GITHUB_EVENT_PATH, "utf8")); +} + +// A pull_request event carries the title and the head commit directly. A merge +// group carries neither: it names the queued pull request only in its ref, so the +// number is recovered from there and the title read back from the API. GitHub +// writes the pull request title into the squash commit, and a title edited after +// the entry joins the queue passes through no other check, so the merge group is +// validated rather than rubber-stamped. This repository has no merge queue today; +// the path is kept so enabling one does not silently lose the check. +async function resolveTarget() { + const event = readEvent(); + if (process.env.GITHUB_EVENT_NAME === "pull_request") { + return { + event: "pull_request", + number: event.pull_request.number, + // Not GITHUB_SHA: on a pull_request event that is the throwaway merge commit, + // and a status posted there is invisible to the pull request. + sha: event.pull_request.head.sha, + title: event.pull_request.title, + }; + } + // refs/heads/gh-readonly-queue//pr--. A group holding more + // than one entry names only the last one, so that is the title being checked. + const ref = (event.merge_group && event.merge_group.head_ref) || process.env.GITHUB_REF || ""; + const match = ref.match(/^refs\/heads\/gh-readonly-queue\/.*\/pr-(\d+)-/); + if (!match) { + throw new Error(`cannot read a pull request number from ${ref}`); + } + const number = Number(match[1]); + const pullRequest = await request("GET", `/repos/${repo}/pulls/${number}`); + return { + event: "merge_group", + number, + sha: (event.merge_group && event.merge_group.head_sha) || process.env.GITHUB_SHA, + title: pullRequest.title, + }; +} + +// Every match, not just the first. A pull request that ends up carrying two of +// these — a race between two runs, or a write that half succeeded — would +// otherwise shed one comment per run and keep the rest, which reads to the author +// as a complaint that no longer clears when the title is fixed. +async function findComments(number) { + const found = []; + for (let page = 1; page <= 10; page++) { + const comments = await request( + "GET", + `/repos/${repo}/issues/${number}/comments?per_page=100&page=${page}` + ); + for (const comment of comments) { + if (typeof comment.body === "string" && comment.body.includes(MARKER)) { + found.push(comment); + } + } + if (comments.length < 100) { + break; + } + } + return found; +} + +function commentBody(title, reason) { + return [ + MARKER, + "### This pull request title does not follow the required format", + "", + // Four backticks so a title containing a fence of its own cannot break out. + "````", + title, + "````", + "", + reason, + "", + "_Edit the title and this comment removes itself on the next run._", + ].join("\n"); +} + +// The comment is an explanation, not the gate: the status is. A comment that +// cannot be written is reported and stepped over, so an unrelated API problem +// cannot fail a pull request whose title is perfectly valid. +async function syncComment(number, title, reason) { + const existing = await findComments(number); + if (!reason) { + for (const comment of existing) { + await request("DELETE", `/repos/${repo}/issues/comments/${comment.id}`); + } + return; + } + const body = commentBody(title, reason); + if (existing.length === 0) { + await request("POST", `/repos/${repo}/issues/${number}/comments`, { body }); + return; + } + // Keep one and reword it only when the reason actually changed, so a rerun on an + // unchanged bad title does not bump the comment and re-notify everyone watching. + if (existing[0].body !== body) { + await request("PATCH", `/repos/${repo}/issues/comments/${existing[0].id}`, { body }); + } + for (const duplicate of existing.slice(1)) { + await request("DELETE", `/repos/${repo}/issues/comments/${duplicate.id}`); + } +} + +async function run() { + // Falls back to the event's own commit so that a failure while resolving the + // target still has somewhere to publish a verdict. + let sha = process.env.GITHUB_SHA; + try { + const target = await resolveTarget(); + sha = target.sha; + + let reason = ""; + validate({ + title: target.title, + onError: (message) => { + reason = message; + }, + }); + + // A merge group has no conversation of its own, and commenting would land on + // the pull request a second time, so that path reports by status alone. + if (target.event === "pull_request") { + try { + await syncComment(target.number, target.title, reason); + } catch (error) { + console.log(`::warning::could not update the explanation comment: ${error.message}`); + } + } + + if (reason) { + await postStatus(sha, "failure", `PR #${target.number} title must match type(subType): description`); + console.log(`::error::${reason}`); + process.exitCode = 1; + return; + } + await postStatus(sha, "success", `Title of PR #${target.number} validated`); + console.log(`Title of PR #${target.number} is valid: ${target.title}`); + } catch (error) { + // A required check that never reports leaves a pull request stuck behind a + // check that is merely missing, and leaves a merge group to wait out its + // status-check timeout before being ejected with nothing naming the cause. + // Publish a verdict even when the run itself came apart. + console.log(`::error::${error.message}`); + process.exitCode = 1; + try { + await postStatus(sha, "failure", "PR title check could not run; see the workflow run"); + } catch (statusError) { + console.log(`::error::could not post the ${CONTEXT} status: ${statusError.message}`); + } + } +} + +function postStatus(sha, state, description) { + return request("POST", `/repos/${repo}/statuses/${sha}`, { + state, + context: CONTEXT, + // GitHub truncates a description past 140 characters. + description: description.slice(0, 140), + }); +} + +if (require.main === module) { + run(); +} + +module.exports = { run, MARKER, CONTEXT, commentBody }; diff --git a/ci/validate-pr-title/check.test.js b/ci/validate-pr-title/check.test.js new file mode 100644 index 000000000..417ed3fd3 --- /dev/null +++ b/ci/validate-pr-title/check.test.js @@ -0,0 +1,254 @@ +"use strict"; + +// Exercises check.js against a stubbed GitHub API. The point is the reporting +// behaviour rather than the rules themselves, which validate.test.js covers: that a +// rejection is explained exactly once, that fixing the title takes the explanation +// away again, that the status lands on the commit the pull request is actually +// showing, and that a verdict is published even when the run comes apart. + +const assert = require("node:assert").strict; +const fs = require("node:fs"); +const os = require("node:os"); +const path = require("node:path"); + +process.env.GITHUB_REPOSITORY = "questdb/java-questdb-client"; +process.env.GITHUB_API_URL = "https://api.github.com"; +process.env.GITHUB_TOKEN = "stub-token"; + +const { run, MARKER, CONTEXT, commentBody } = require("./check"); + +const VALID = "fix(qwp): repair sender state after rollback"; +const INVALID = "just some words"; + +// Records every call and answers from a small routing table, so a test can assert +// on what the job asked GitHub to do rather than on how it phrased it. +function stubApi({ comments = [], title = VALID, fail = null }) { + const calls = []; + global.fetch = async (url, options) => { + const method = options.method; + const route = String(url).replace("https://api.github.com", ""); + calls.push({ method, route, body: options.body ? JSON.parse(options.body) : null }); + + const answer = (status, payload) => ({ + ok: status < 400, + status, + json: async () => payload, + text: async () => JSON.stringify(payload), + }); + + if (fail && fail(method, route)) { + return answer(500, { message: "stub failure" }); + } + if (method === "GET" && /\/issues\/\d+\/comments/.test(route)) { + return answer(200, route.includes("page=1") ? comments : []); + } + if (method === "GET" && /\/pulls\/\d+$/.test(route)) { + return answer(200, { title }); + } + if (method === "DELETE") { + return answer(204, null); + } + return answer(201, { id: 4242 }); + }; + return calls; +} + +function pullRequestEvent(title) { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "pr-title-")); + const file = path.join(dir, "event.json"); + fs.writeFileSync( + file, + JSON.stringify({ pull_request: { number: 93, title, head: { sha: "headsha" } } }) + ); + process.env.GITHUB_EVENT_NAME = "pull_request"; + process.env.GITHUB_EVENT_PATH = file; + // What GITHUB_SHA is on a pull_request event: the throwaway merge commit, which + // is not where the status belongs. + process.env.GITHUB_SHA = "mergesha"; +} + +function mergeGroupEvent() { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "pr-title-")); + const file = path.join(dir, "event.json"); + fs.writeFileSync( + file, + JSON.stringify({ + merge_group: { + head_ref: "refs/heads/gh-readonly-queue/main/pr-93-abc123", + head_sha: "queuesha", + }, + }) + ); + process.env.GITHUB_EVENT_NAME = "merge_group"; + process.env.GITHUB_EVENT_PATH = file; + process.env.GITHUB_SHA = "queuesha"; +} + +const statusOf = (calls) => calls.find((call) => call.route.includes("/statuses/")); +const commentCalls = (calls) => + calls.filter((call) => call.method !== "GET" && call.route.includes("comments")); + +async function test(name, body) { + process.exitCode = 0; + await body(); + console.log(`ok - ${name}`); +} + +async function main() { + await test("an invalid title fails the status and explains itself once", async () => { + pullRequestEvent(INVALID); + const calls = stubApi({}); + await run(); + + const status = statusOf(calls); + assert.equal(status.route, "/repos/questdb/java-questdb-client/statuses/headsha"); + assert.equal(status.body.state, "failure"); + assert.equal(status.body.context, CONTEXT); + + const posted = commentCalls(calls); + assert.equal(posted.length, 1); + assert.equal(posted[0].method, "POST"); + assert.ok(posted[0].body.body.includes(MARKER)); + assert.ok(posted[0].body.body.includes(INVALID)); + assert.equal(process.exitCode, 1); + }); + + await test("the status is posted on the head commit, not the merge commit", async () => { + pullRequestEvent(VALID); + const calls = stubApi({}); + await run(); + assert.equal(statusOf(calls).route, "/repos/questdb/java-questdb-client/statuses/headsha"); + assert.ok( + !calls.some((call) => call.route.includes("mergesha")), + "a status on the merge commit is invisible to the pull request" + ); + }); + + await test("a repeat run on the same bad title does not stack a second comment", async () => { + pullRequestEvent(INVALID); + const existing = { id: 11, body: commentBody(INVALID, rejectionReason(INVALID)) }; + const calls = stubApi({ comments: [existing] }); + await run(); + + assert.equal(commentCalls(calls).length, 0, "identical comment must be left alone"); + assert.equal(statusOf(calls).body.state, "failure"); + }); + + await test("fixing the title deletes the comment and turns the status green", async () => { + pullRequestEvent(VALID); + const calls = stubApi({ comments: [{ id: 11, body: `${MARKER}\nold complaint` }] }); + await run(); + + const removed = commentCalls(calls); + assert.equal(removed.length, 1); + assert.equal(removed[0].method, "DELETE"); + assert.equal(removed[0].route, "/repos/questdb/java-questdb-client/issues/comments/11"); + assert.equal(statusOf(calls).body.state, "success"); + assert.equal(process.exitCode, 0); + }); + + // The OSS copy looks up a single comment, so a pull request carrying two sheds + // one per run and keeps the other. Fixing the title has to clear all of them. + await test("fixing the title clears every duplicate comment, not just the first", async () => { + pullRequestEvent(VALID); + const calls = stubApi({ + comments: [ + { id: 11, body: `${MARKER}\nold complaint` }, + { id: 12, body: `${MARKER}\na second copy` }, + ], + }); + await run(); + + const removed = commentCalls(calls); + assert.deepEqual( + removed.map((call) => `${call.method} ${call.route.split("/").pop()}`), + ["DELETE 11", "DELETE 12"] + ); + assert.equal(statusOf(calls).body.state, "success"); + }); + + await test("a duplicate is cleared while the surviving comment is reworded", async () => { + pullRequestEvent(INVALID); + const calls = stubApi({ + comments: [ + { id: 11, body: `${MARKER}\nstale wording` }, + { id: 12, body: `${MARKER}\nanother copy` }, + ], + }); + await run(); + + const touched = commentCalls(calls); + assert.deepEqual( + touched.map((call) => call.method), + ["PATCH", "DELETE"], + "one comment carries the explanation, the rest go away" + ); + assert.ok(touched[0].body.body.includes(INVALID)); + assert.equal(statusOf(calls).body.state, "failure"); + }); + + await test("a clean title with nothing to clean up touches no comment", async () => { + pullRequestEvent(VALID); + const calls = stubApi({}); + await run(); + assert.equal(commentCalls(calls).length, 0); + assert.equal(statusOf(calls).body.state, "success"); + }); + + await test("a merge group reads the queued title and reports by status only", async () => { + mergeGroupEvent(); + const calls = stubApi({ title: INVALID }); + await run(); + + assert.ok(calls.some((call) => call.route === "/repos/questdb/java-questdb-client/pulls/93")); + assert.equal(commentCalls(calls).length, 0, "a merge group must not comment"); + assert.equal(statusOf(calls).route, "/repos/questdb/java-questdb-client/statuses/queuesha"); + assert.equal(statusOf(calls).body.state, "failure"); + assert.equal(process.exitCode, 1); + }); + + await test("a comment that cannot be written does not fail a valid title", async () => { + pullRequestEvent(VALID); + const calls = stubApi({ + comments: [{ id: 11, body: `${MARKER}\nold complaint` }], + fail: (method) => method === "DELETE", + }); + await run(); + + assert.equal(statusOf(calls).body.state, "success", "the status is the gate, not the comment"); + assert.equal(process.exitCode, 0); + }); + + await test("an unreadable merge group ref still publishes a failure", async () => { + mergeGroupEvent(); + process.env.GITHUB_EVENT_PATH = (() => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "pr-title-")); + const file = path.join(dir, "event.json"); + fs.writeFileSync(file, JSON.stringify({ merge_group: { head_ref: "refs/heads/nonsense" } })); + return file; + })(); + const calls = stubApi({}); + await run(); + + assert.equal(statusOf(calls).body.state, "failure"); + assert.equal(process.exitCode, 1); + }); + + // The scenarios deliberately leave process.exitCode at 1 behind them, since that + // is what the job under test sets on a rejection. + process.exitCode = 0; + console.log("\nall check.js scenarios passed"); +} + +// The exact rejection text, so the "no duplicate comment" case can build the body +// the job would have written on the previous run. +function rejectionReason(title) { + let reason = ""; + require("./validate").validate({ title, onError: (message) => (reason = message) }); + return reason; +} + +main().catch((error) => { + console.error(error); + process.exit(1); +}); diff --git a/ci/validate-pr-title/dangerfile.js b/ci/validate-pr-title/dangerfile.js deleted file mode 100644 index c60d62666..000000000 --- a/ci/validate-pr-title/dangerfile.js +++ /dev/null @@ -1,4 +0,0 @@ -const { danger, fail } = require("danger"); -const { validate } = require("./validate"); - -validate({ title: danger.github.pr.title, onError: fail }); diff --git a/ci/validate-pr-title/readme.md b/ci/validate-pr-title/readme.md index 18a284d3e..2d500ef11 100644 --- a/ci/validate-pr-title/readme.md +++ b/ci/validate-pr-title/readme.md @@ -1,5 +1,33 @@ -This folder contains configuration files which are used to run validation rules on Github pull requests titles. +This folder holds the validation rules applied to GitHub pull request titles, and +the job that reports on them. -It is done by running [Danger JS](https://danger.systems/js/) tool in [github action](../../.github/workflows/danger.yml). +- `validate.js` — the rules themselves: `type(subType): description`. +- `check.js` — reads the title from the workflow event, posts the `PR title` commit + status, and leaves a comment explaining a rejection. The comment is updated in + place while the title stays wrong and deleted once it is fixed. +- Tests run with node and need no dependencies: `node ./validate.test.js` and + `node ./check.test.js`. -In addition, the validation rules are tested. Tests can be executed with node, by running `node ./validate.test.js` +Run by [.github/workflows/pr_title.yml](../../.github/workflows/pr_title.yml) on +pull requests and on merge groups. It authenticates with the workflow's own +`GITHUB_TOKEN`, so it needs no bot account and no personal access token. + +## Replacing Danger + +This used to run [Danger JS](https://danger.systems/js/), which read the title and +reported through the questdb-butler account using a personal access token held in +the `DANGER_GITHUB_TOKEN` secret. Nothing about the job needed a separate identity, +and the token's expiry would have quietly stopped the check. `check.js` does the +same work on the workflow's own token, and `dangerfile.js` and the `yarn global add +danger` step are gone. Once this has settled, `DANGER_GITHUB_TOKEN` can be deleted +from the repository secrets. + +The status context is `PR title` rather than the `Danger` that Danger posted. That +rename is safe here because the branch protection on `main` requires only the four +`questdb.java-questdb-client` Azure contexts, so nothing waits on `Danger`. The +copy in questdb/questdb cannot do the same: its `master` ruleset names `Danger` +exactly, and renaming it there without editing the ruleset in the same change +blocks every pull request. + +The subType list is deliberately shorter than the server repositories': this is a +client, so `sql`, `wal`, `repl` and the rest are rejected on purpose. diff --git a/ci/validate-pr-title/validate.js b/ci/validate-pr-title/validate.js index f3e018fed..a704b0d66 100644 --- a/ci/validate-pr-title/validate.js +++ b/ci/validate-pr-title/validate.js @@ -39,16 +39,21 @@ perf(sql): improve pattern matching performance for SELECT sub-queries \`\`\` `.trim(); -/* The basic valid PR title formats are: - * 1. allowedType(allowedSubtype): optional description - * 2. allowedType: optional description +/* The valid PR title formats are: + * 1. allowedType(allowedSubType): description + * 2. build: description * + * Note that format 2 is available to `build` alone. Every other type has to name + * a subType, so `feat: thing` is rejected while `build: 6.6` is accepted. + * + * A `!` before the colon is the Conventional Commits marker for a breaking + * change, as in `feat(qwp)!: ...`, and is accepted on either format. * consult ./validate.test.js for a full list * */ const prTitleRegex = new RegExp( `^(((?:${allowedTypes.join("|")})\\((?:${allowedSubTypes.join( "|", - )})\\))|build): .*`, + )})\\))|build)!?: .*`, ); function validate({ title, onError }) { diff --git a/ci/validate-pr-title/validate.test.js b/ci/validate-pr-title/validate.test.js index dc90e8436..8667f7f7f 100644 --- a/ci/validate-pr-title/validate.test.js +++ b/ci/validate-pr-title/validate.test.js @@ -2,27 +2,36 @@ const assert = require("node:assert").strict; const { validate, allowedTypes, allowedSubTypes } = require("./validate"); const testValid = (title) => - assert.doesNotThrow(() => - validate({ - title, - onError: () => { - throw `should accept "${title}"`; - }, - }) + assert.doesNotThrow( + () => + validate({ + title, + onError: () => { + throw new Error(`should accept "${title}"`); + }, + }), + `should accept "${title}"`, ); +// onError has to be a real callback here. Passing a bare `onError` identifier makes +// this assertion pass on the ReferenceError that raises instead of on the title +// being rejected, which lets every negative case below succeed against a validator +// that accepts everything. const testInvalid = (title) => assert.throws( - () => validate({ title, onError }), - `should NOT accept "${title}"` + () => + validate({ + title, + onError: () => { + throw new Error(`rejected "${title}"`); + }, + }), + `should NOT accept "${title}"`, ); allowedTypes.forEach((type) => { allowedSubTypes.forEach((subType) => { - testValid( - `${type}(${subType}): foo`, - `should accept "${type}(${subType}): foo"` - ); + testValid(`${type}(${subType}): foo`); }); }); @@ -36,3 +45,27 @@ testInvalid(`build(house)`); testInvalid(`foo: bar`); testInvalid(`update(bar): baz`); testInvalid(`ui: updating stuff`); + +// Titles this repository actually merges. +testValid("feat(qwp): add table options API to name the designated timestamp column"); +testValid("fix(ilp): fix a leaked socket when an HTTP sender fails"); +testValid("chore(build): build client native library with Maven"); +testValid("build: 6.6"); + +// The Conventional Commits breaking-change marker, on both accepted formats. +testValid("feat(qwp)!: drop the legacy sender constructor"); +testValid("build!: require JDK 17"); +testInvalid("feat(qwp)!"); +testInvalid("feat(nonsense)!: still an unknown area"); + +// Subtypes that belong to the server repositories, not this client. +testInvalid("fix(sql): not an area of this repository"); +testInvalid("fix(wal): not an area of this repository"); +testInvalid("fix(repl): not an area of this repository"); + +// Only `build` may skip the subType. Every other type has to name one. This is +// what rejects the automated "Bump version to x.y.z-SNAPSHOT" release titles. +testInvalid("chore: bump a dependency"); +testInvalid("Bump version to 1.3.10-SNAPSHOT"); + +console.log("all validate.js rules passed"); From eb9c4c7fc5310301324dc99ae13b6597122f71da Mon Sep 17 00:00:00 2001 From: bluestreak Date: Fri, 4 Sep 2026 15:52:23 +0100 Subject: [PATCH 2/3] ci(build): correct the note about the status context in questdb/questdb readme.md and the comment above CONTEXT both said questdb/questdb could not rename its status context away from "Danger". It has: questdb/questdb#7599 moves it to "PR title", the same string this repository posts, paired with a hand edit of the master ruleset. What is actually different survives the rename and is the part worth keeping, because it is about what a rename costs rather than what the context is called. Here the string is free: branch protection on main requires only the four questdb.java-questdb-client Azure contexts, so nothing ever waited on "Danger" and no open pull request was left behind a check that stopped reporting. There the ruleset names the context, so the code and the ruleset are a contract, and the gap between changing one and the other is a gap in which nothing merges. --- ci/validate-pr-title/check.js | 10 +++++----- ci/validate-pr-title/readme.md | 16 ++++++++++------ 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/ci/validate-pr-title/check.js b/ci/validate-pr-title/check.js index 2babeea98..71648f3b1 100644 --- a/ci/validate-pr-title/check.js +++ b/ci/validate-pr-title/check.js @@ -15,11 +15,11 @@ const MARKER = ""; // The commit status context. Danger posted "Danger" here, but nothing required // that string: the branch protection on main requires only the four Azure -// contexts, so the rename costs nothing and no open pull request is left waiting -// on a check that stopped reporting. The OSS copy is stuck with "Danger" because -// its master ruleset does name that exact string. Making this a required check is -// a branch-protection edit, and until then a rejection is visible but not -// blocking, which is what Danger already was here. +// contexts, so the rename cost nothing and left no open pull request waiting on a +// check that stopped reporting. The OSS copy posts the same string, but there it +// is named by the master ruleset, so renaming it takes a matching ruleset edit. +// Making this a required check is a branch-protection edit, and until then a +// rejection is visible but not blocking, which is what Danger already was here. const CONTEXT = "PR title"; const apiUrl = process.env.GITHUB_API_URL || "https://api.github.com"; diff --git a/ci/validate-pr-title/readme.md b/ci/validate-pr-title/readme.md index 2d500ef11..be48bfaaf 100644 --- a/ci/validate-pr-title/readme.md +++ b/ci/validate-pr-title/readme.md @@ -22,12 +22,16 @@ same work on the workflow's own token, and `dangerfile.js` and the `yarn global danger` step are gone. Once this has settled, `DANGER_GITHUB_TOKEN` can be deleted from the repository secrets. -The status context is `PR title` rather than the `Danger` that Danger posted. That -rename is safe here because the branch protection on `main` requires only the four -`questdb.java-questdb-client` Azure contexts, so nothing waits on `Danger`. The -copy in questdb/questdb cannot do the same: its `master` ruleset names `Danger` -exactly, and renaming it there without editing the ruleset in the same change -blocks every pull request. +The status context is `PR title` rather than the `Danger` that Danger posted. The +rename was free here: the branch protection on `main` requires only the four +`questdb.java-questdb-client` Azure contexts, so nothing ever waited on `Danger`, +and every open pull request that carried one carried a green one. questdb/questdb +moved to the same name, but had to pay for it — its `master` ruleset names the +context, so the rename had to be paired with a ruleset edit, and the gap between +the two is a gap in which nothing can merge. + +That is the difference to keep in mind before renaming this one again: here the +string is free, there it is a contract with the ruleset. The subType list is deliberately shorter than the server repositories': this is a client, so `sql`, `wal`, `repl` and the rest are rejected on purpose. From 2697dbb82089ddeb2c4beaa21ebc5ce623034483 Mon Sep 17 00:00:00 2001 From: bluestreak Date: Fri, 4 Sep 2026 16:14:59 +0100 Subject: [PATCH 3/3] ci(build): stop the title check showing three near-identical rows The workflow name and the status context are both "PR title", so a pull request showed three rows that read the same way: the "PR title" status, plus the "PR title / test" and "PR title / validate" check runs GitHub names after the workflow and its jobs. The rules' tests move to pr_title_rules.yml behind a path filter, so they run when ci/validate-pr-title or either workflow changes and stay off every other pull request. The reason they are not a step of the job that posts the status is unchanged and now structural rather than conventional: run there, a failing or flaky test stops check.js from running at all, no status is posted, and the pull request sits behind a check that is merely missing. Separate workflows cannot do that to each other. A pull request now shows the status and one "PR title / validate" row, and a third only when the checker itself is under change, which is when the tests are worth looking at. --- .github/workflows/pr_title.yml | 27 +++++------------------ .github/workflows/pr_title_rules.yml | 32 ++++++++++++++++++++++++++++ ci/validate-pr-title/readme.md | 10 +++++++++ 3 files changed, 47 insertions(+), 22 deletions(-) create mode 100644 .github/workflows/pr_title_rules.yml diff --git a/.github/workflows/pr_title.yml b/.github/workflows/pr_title.yml index 3b88dcd97..b3ab87204 100644 --- a/.github/workflows/pr_title.yml +++ b/.github/workflows/pr_title.yml @@ -10,29 +10,12 @@ on: permissions: {} +# One job, so a pull request shows one "PR title / validate" row beside the +# "PR title" status this posts. The rules' own tests live in pr_title_rules.yml, +# which runs only when ci/validate-pr-title changes: keeping them out of this +# workflow is what stops a failing test from taking the verdict down with it, and +# keeps two near-identical rows off every unrelated pull request. jobs: - # Deliberately a separate job rather than a first step of `validate`. Run in one - # job, a broken or flaky test would stop check.js from running at all, which - # leaves a fixed title still carrying its rejection comment and no status posted. - # The rules and the reporting are worth failing on, but not at the cost of the - # verdict itself. - test: - runs-on: ubuntu-latest - permissions: - # The top-level `permissions: {}` above leaves a job that declares nothing - # with a token that has no scopes. Cloning works here regardless because the - # repository is public, but saying so keeps the job working if that changes. - contents: read - steps: - - uses: actions/checkout@v5 - # No setup-node step: the runner ships a current Node, and nothing under - # ci/validate-pr-title depends on anything outside the standard library. - - name: Test the title rules and the reporting - working-directory: ./ci/validate-pr-title - run: | - node validate.test.js - node check.test.js - validate: # A pull request from a fork gets a read-only token, so this job could post # neither the status nor the comment. Same condition the Danger job carried. diff --git a/.github/workflows/pr_title_rules.yml b/.github/workflows/pr_title_rules.yml new file mode 100644 index 000000000..fe11fc936 --- /dev/null +++ b/.github/workflows/pr_title_rules.yml @@ -0,0 +1,32 @@ +name: PR title rules + +# The tests for the title rules and for the reporting, kept out of pr_title.yml +# for two reasons. They must not gate the verdict: run as a step of the job that +# posts the status, a failing or flaky test stops check.js from running at all, +# no status is posted, and the pull request sits behind a check that is merely +# missing. And they are only interesting when the checker itself changes, so a +# path filter keeps a second near-identical row off every pull request that has +# nothing to do with them. +on: + pull_request: + paths: + - 'ci/validate-pr-title/**' + - '.github/workflows/pr_title.yml' + - '.github/workflows/pr_title_rules.yml' + +permissions: {} + +jobs: + test: + runs-on: ubuntu-latest + permissions: + contents: read + steps: + - uses: actions/checkout@v5 + # No setup-node step: the runner ships a current Node, and nothing under + # ci/validate-pr-title depends on anything outside the standard library. + - name: Test the title rules and the reporting + working-directory: ./ci/validate-pr-title + run: | + node validate.test.js + node check.test.js diff --git a/ci/validate-pr-title/readme.md b/ci/validate-pr-title/readme.md index be48bfaaf..968e8bf3a 100644 --- a/ci/validate-pr-title/readme.md +++ b/ci/validate-pr-title/readme.md @@ -12,6 +12,16 @@ Run by [.github/workflows/pr_title.yml](../../.github/workflows/pr_title.yml) on pull requests and on merge groups. It authenticates with the workflow's own `GITHUB_TOKEN`, so it needs no bot account and no personal access token. +The tests live in a second workflow, +[pr_title_rules.yml](../../.github/workflows/pr_title_rules.yml), rather than +alongside the job that posts the status. Run as a step of that job, a failing or +flaky test stops check.js from running at all, no status is posted, and the pull +request sits behind a check that is merely missing. A path filter runs them only +when this folder or either workflow changes, which also keeps a second +near-identical row off every unrelated pull request: the workflow name and the +status context are both "PR title", so every job here costs a row that reads like +the verdict. + ## Replacing Danger This used to run [Danger JS](https://danger.systems/js/), which read the title and