diff --git a/.claude/skills/fix-release-pr/SKILL.md b/.claude/skills/fix-release-pr/SKILL.md new file mode 100644 index 000000000..1a154b338 --- /dev/null +++ b/.claude/skills/fix-release-pr/SKILL.md @@ -0,0 +1,174 @@ +--- +name: fix-release-pr +description: > + Fix CI failures and address code-review comments on a pull request that + targets the protected `release` branch of `ClickHouse/clickhouse-js`. Release + PRs are snapshots of `main` (their head branch is usually `main` itself) and + cannot be edited directly — branch protection blocks pushing fixes onto them. + Use this skill whenever the work is "fix the CI / address the review comments + on PR #N" and that PR's base branch is `release`: it routes the fix through a + separate PR to `main`, then closes the loop on the original release PR by + replying to and resolving the review threads. Triggers on phrasing like "fix + this release PR", "the release PR is failing CI", "address the review comments + but it's a release branch", or after checking out a PR whose base is `release`. + Do NOT use this for ordinary feature/fix PRs that target `main` — for those, + just push to the PR's own branch. +--- + +# Fixing PRs to the `release` branch + +## Why this skill exists + +In `clickhouse-js`, the `release` branch receives **release PRs** — snapshots of +`main` opened to cut a version (e.g. titled "1.23 beta2"). Two properties make +them special: + +- The PR's **head branch is usually `main` itself** (base `release`, head `main`). + `gh pr checkout ` is therefore a no-op that leaves you on `main` — do **not** + commit fixes there. +- `release` is **protected**: you cannot push commits onto the release PR to fix + CI or review feedback. + +So fixes never go onto the release PR. They go to **`main` via a separate PR**; +once that merges, the release branch is re-synced from `main` and the release PR +picks the fix up. This skill is that workflow. + +## Step 1 — Confirm it's a release PR + +```bash +gh pr view --json title,baseRefName,headRefName,headRefOid +``` + +If `baseRefName` is `release`, proceed. (If it's `main`, this skill does not +apply — push to the PR's branch normally.) + +## Step 2 — Gather what needs fixing + +**CI failures.** List checks and find the `fail` rows: + +```bash +gh pr checks +``` + +- The job named **`success`** is an _aggregate gate_ ("Fail if any needed job + failed") — it only fails _because_ a real job failed. Ignore it as a root cause + and find the actual failing job. +- Open the real failure log: + +```bash +gh run view --job --log-failed | tail -50 +``` + +**Review comments.** Inline review comments with their REST IDs and the GraphQL +thread node IDs (you need both: REST `id` to reply, thread node `id` to resolve): + +```bash +# Inline comments: REST id + location + author + body +gh api repos/ClickHouse/clickhouse-js/pulls//comments \ + -q '.[] | "\(.id)\t\(.path):\(.line)\t\(.user.login)\n\(.body)\n---"' + +# Review threads: node id (PRRT_…), resolved state, and first comment's databaseId +gh api graphql -f query=' +{ repository(owner:"ClickHouse", name:"clickhouse-js") { + pullRequest(number: ) { + reviewThreads(first: 50) { nodes { + id isResolved + comments(first: 1) { nodes { databaseId path body } } + } } + } } }' \ + -q '.data.repository.pullRequest.reviewThreads.nodes[] + | "\(.id)\tresolved=\(.isResolved)\tdbId=\(.comments.nodes[0].databaseId)\t\(.comments.nodes[0].path)"' +``` + +Match each thread (`PRRT_…` node id) to its first comment's `databaseId` — that +`databaseId` is the REST comment id you reply to in Step 5. + +## Step 3 — Branch off the latest `main` + +Never branch off the release PR head. Start from up-to-date `main`: + +```bash +git fetch origin +git checkout -b fix/ origin/main +``` + +## Step 4 — Make the fix and verify + +Apply the fixes. Then verify with the repo's own tooling (run the `setup` skill +first if `node_modules` isn't populated): + +- **Prettier** is the most common release-PR CI failure. House style is the + Prettier defaults (`.prettierrc` is `{}` → double quotes + semicolons). + Fix and check: + ```bash + node_modules/.bin/prettier --write + npm run -s prettier:check + ``` + `prettier:check` runs on the whole repo and may flag **untracked local scratch + dirs** (e.g. a `type-parser/` working directory). Those aren't part of the PR + and CI never sees them — only tracked files matter. Confirm no _tracked_ file + is flagged: + ```bash + npm run -s prettier:check 2>&1 \ + | grep -oE '\[(warn|error)\] [^ ]+\.(mjs|ts|js|json|ya?ml|md)' \ + | grep -v '/' | sort -u # empty = clean + ``` +- For standalone scripts: `node --check `. +- For library code: `npm run typecheck`, `npm run lint`, and the relevant + `npm run test:*` target (see the `setup` skill for what each needs). +- When the fix is logic (not just formatting), exercise it directly — e.g. drive + the script against synthetic inputs in the scratchpad dir and assert the + exit code / output, rather than trusting it by inspection. + +## Step 5 — Commit, push, open the PR to `main` + +```bash +git commit -m "(): + +Addresses CI failure / review feedback on # (release PR). The fix lands +on \`main\` separately because # targets the protected \`release\` branch. + +Co-Authored-By: Claude Opus 4.8 (1M context) " + +git push -u origin fix/ +gh pr create --base main --head fix/ --title "…" --body "…" +``` + +In the new PR body, explicitly state it addresses #N and why it's a separate PR +(release branch is protected). Map each fix back to the specific CI failure or +review comment it resolves. + +## Step 6 — Close the loop on the release PR + +For **each** original review comment, reply pointing to the new PR, then resolve +the thread. + +```bash +# Reply (use the REST comment id from Step 2) +gh api repos/ClickHouse/clickhouse-js/pulls//comments//replies \ + -f body='Fixed in #. . (Lands on `main` separately since this PR targets the protected `release` branch.)' \ + -q '.html_url' + +# Resolve the thread (use the PRRT_… node id from Step 2) +gh api graphql \ + -f query='mutation($id:ID!){resolveReviewThread(input:{threadId:$id}){thread{id isResolved}}}' \ + -f id='' \ + -q '.data.resolveReviewThread.thread | "\(.id) resolved=\(.isResolved)"' +``` + +## Step 7 — Flag the re-sync + +The fix is on `main`, not on `release`. Remind the maintainer that once the new +PR merges, the `release` branch / the release PR must be re-synced from `main` +to pick the fix up. Do not attempt to push to `release` yourself. + +## Quick reference — the whole flow + +1. `gh pr view --json baseRefName` → confirm base is `release`. +2. `gh pr checks ` + `gh run view --job --log-failed` → real CI failure (ignore the `success` gate). +3. `gh api …/pulls//comments` + GraphQL `reviewThreads` → review comments + thread ids. +4. `git checkout -b fix/… origin/main` → branch off latest `main`. +5. Fix → verify (prettier / typecheck / lint / tests / `node --check`). +6. Commit → push → `gh pr create --base main`. +7. Reply to each review comment (REST `…/comments//replies`) → resolve each thread (GraphQL `resolveReviewThread`). +8. Remind: re-sync `release` from `main` after merge. diff --git a/scripts/ci/lockfile-age-audit.mjs b/scripts/ci/lockfile-age-audit.mjs index 7b73c74ae..67fa125f0 100644 --- a/scripts/ci/lockfile-age-audit.mjs +++ b/scripts/ci/lockfile-age-audit.mjs @@ -3,38 +3,65 @@ // True new entries only — compares base vs head lockfile resolutions, not just diff `+` lines, // so lockfile reorders don't trigger false positives. // Skips first-party @clickhouse/* packages (no upstream-compromise risk). -// Fails closed on registry errors (skip via the 'lockfile-age-skip' PR label). -import { execSync } from 'node:child_process' -import { readFileSync } from 'node:fs' +// Fails closed on registry errors and on new deps resolved from a non-registry source +// (skip via the 'lockfile-age-skip' PR label). +import { execSync } from "node:child_process"; +import { readFileSync } from "node:fs"; -const MIN_AGE_DAYS = Number.parseInt(process.env.MIN_AGE_DAYS ?? '7', 10) +const MIN_AGE_DAYS = Number.parseInt(process.env.MIN_AGE_DAYS ?? "7", 10); if (!Number.isFinite(MIN_AGE_DAYS) || MIN_AGE_DAYS <= 0) { console.error( - `MIN_AGE_DAYS must be a positive integer (got: ${process.env.MIN_AGE_DAYS ?? '7'})`, - ) - process.exit(2) + `MIN_AGE_DAYS must be a positive integer (got: ${process.env.MIN_AGE_DAYS ?? "7"})`, + ); + process.exit(2); } -const BASE_REF = process.env.GITHUB_BASE_REF || 'main' -const cutoffMs = Date.now() - MIN_AGE_DAYS * 86400_000 +const BASE_REF = process.env.GITHUB_BASE_REF || "main"; +const cutoffMs = Date.now() - MIN_AGE_DAYS * 86400_000; // First-party scopes — npm has no native equivalent to yarn's npmPreapprovedPackages, // so hardcoded here. Mirrors the Dependabot cooldown.exclude list. -const PREAPPROVED_SCOPES = ['@clickhouse/'] +const PREAPPROVED_SCOPES = ["@clickhouse/"]; -const REGISTRY_HOSTS = ['registry.npmjs.org', 'registry.npmmirror.com'] +const REGISTRY_HOSTS = ["registry.npmjs.org", "registry.npmmirror.com"]; -function isRegistryEntry(resolved) { - if (!resolved || typeof resolved !== 'string') return false +// Classify an entry's `resolved` field: +// 'registry' — an allowed HTTPS registry tarball; audited against the age gate. +// 'foreign' — present but not an allowed HTTPS registry tarball (alternative +// registry, plain http, git+https, or a non-URL string from a +// hand-edited lockfile). A bypass vector for the age gate, so we fail +// closed on newly-added foreign entries (escape hatch: the +// 'lockfile-age-skip' PR label). +// 'local' — no resolved URL at all (workspace source dirs) or a file: dependency; +// not a registry download, so there is nothing to age-check. +function classifyResolved(resolved) { + if (!resolved || typeof resolved !== "string") return "local"; + let u; try { - const u = new URL(resolved) - return REGISTRY_HOSTS.includes(u.host) + u = new URL(resolved); } catch { - return false + // A present-but-unparseable resolved string (e.g. a hand-edited lockfile) + // must not slip through the gate — fail closed. + return "foreign"; + } + if (u.protocol === "file:") return "local"; + if (u.protocol === "https:" && REGISTRY_HOSTS.includes(u.host)) + return "registry"; + return "foreign"; +} + +// Render a `resolved` value for CI logs without leaking any embedded credentials +// (e.g. https://user:token@host/...). `URL.host` excludes userinfo, query, and path. +function redactResolved(resolved) { + try { + const u = new URL(resolved); + return `${u.protocol}//${u.host}`; + } catch { + return ""; } } function isPreapproved(name) { - return PREAPPROVED_SCOPES.some((scope) => name.startsWith(scope)) + return PREAPPROVED_SCOPES.some((scope) => name.startsWith(scope)); } // node_modules/foo -> foo @@ -42,167 +69,203 @@ function isPreapproved(name) { // node_modules/foo/node_modules/baz -> baz // node_modules/foo/node_modules/@scope/baz -> @scope/baz function packageNameFromPath(path) { - const idx = path.lastIndexOf('node_modules/') - if (idx === -1) return null - return path.slice(idx + 'node_modules/'.length) + const idx = path.lastIndexOf("node_modules/"); + if (idx === -1) return null; + return path.slice(idx + "node_modules/".length); } function extractNpmResolutions(text) { - const out = new Map() - let parsed + const out = new Map(); + let parsed; try { - parsed = JSON.parse(text) + parsed = JSON.parse(text); } catch (err) { - throw new Error(`Invalid JSON in lockfile: ${err.message}`) + throw new Error(`Invalid JSON in lockfile: ${err.message}`); } - const version = parsed.lockfileVersion + const version = parsed.lockfileVersion; if (version !== 2 && version !== 3) { throw new Error( `Unsupported lockfileVersion ${version}. This audit handles package-lock.json v2/v3 (npm 7+).`, - ) + ); } - const packages = parsed.packages - if (!packages || typeof packages !== 'object') return out + const packages = parsed.packages; + if (!packages || typeof packages !== "object") return out; for (const [path, entry] of Object.entries(packages)) { - if (path === '') continue // root project - if (!entry || typeof entry !== 'object') continue - if (entry.link === true) continue // workspace symlinks - if (!entry.version) continue - if (!isRegistryEntry(entry.resolved)) continue - const name = packageNameFromPath(path) - if (!name) continue - // Key by name@version so multiple paths resolving to the same registry entry collapse. - const key = `${name}@${entry.version}` - out.set(key, { name, version: entry.version }) + if (path === "") continue; // root project + if (!entry || typeof entry !== "object") continue; + if (entry.link === true) continue; // workspace symlinks + if (!entry.version) continue; + const kind = classifyResolved(entry.resolved); + if (kind === "local") continue; + const name = packageNameFromPath(path); + if (!name) continue; + // Key by name@version so multiple paths resolving to the same entry collapse. + const key = `${name}@${entry.version}`; + out.set(key, { + name, + version: entry.version, + kind, + resolved: entry.resolved, + }); } - return out + return out; } // Use the merge-base, not the branch tip — what the PR *actually introduced* // is what's in head but not in the common ancestor with main. Comparing // against the branch tip would flag stale pins as "new" when they pre-date // the PR. -let mergeBase +let mergeBase; try { mergeBase = execSync(`git merge-base "origin/${BASE_REF}" HEAD`, { - encoding: 'utf8', - }).trim() + encoding: "utf8", + }).trim(); } catch (err) { - console.error(`Could not find merge-base of HEAD and origin/${BASE_REF}: ${err.message}`) - process.exit(2) + console.error( + `Could not find merge-base of HEAD and origin/${BASE_REF}: ${err.message}`, + ); + process.exit(2); } -let baseLockfile +let baseLockfile; try { baseLockfile = execSync(`git show "${mergeBase}:package-lock.json"`, { - encoding: 'utf8', + encoding: "utf8", maxBuffer: 256 * 1024 * 1024, - }) + }); } catch (err) { console.error( `Could not read package-lock.json at merge-base ${mergeBase.slice(0, 10)}: ${err.message}`, - ) + ); console.error( `If package-lock.json is being introduced for the first time, use the 'lockfile-age-skip' label.`, - ) - process.exit(2) + ); + process.exit(2); } -const headLockfile = readFileSync('package-lock.json', 'utf8') +const headLockfile = readFileSync("package-lock.json", "utf8"); -let baseResolutions, headResolutions +let baseResolutions, headResolutions; try { - baseResolutions = extractNpmResolutions(baseLockfile) - headResolutions = extractNpmResolutions(headLockfile) + baseResolutions = extractNpmResolutions(baseLockfile); + headResolutions = extractNpmResolutions(headLockfile); } catch (err) { - console.error(err.message) - process.exit(2) + console.error(err.message); + process.exit(2); } -const added = new Map() +const added = new Map(); for (const [key, value] of headResolutions) { - if (baseResolutions.has(key)) continue - if (isPreapproved(value.name)) continue - added.set(key, value) + if (baseResolutions.has(key)) continue; + if (isPreapproved(value.name)) continue; + added.set(key, value); } if (added.size === 0) { - console.log('No new npm resolutions to audit.') - process.exit(0) + console.log("No new npm resolutions to audit."); + process.exit(0); } -console.log(`Auditing ${added.size} new lockfile entries against ${MIN_AGE_DAYS}-day age gate.`) +console.log( + `Auditing ${added.size} new lockfile entries against ${MIN_AGE_DAYS}-day age gate.`, +); -const violations = [] -const errors = [] +const violations = []; +const errors = []; + +// Newly-added entries resolved from a non-registry source bypass the age gate entirely, +// so fail closed on them rather than silently ignoring (escape hatch: 'lockfile-age-skip'). +const registryEntries = []; +for (const value of added.values()) { + if (value.kind === "foreign") { + errors.push( + `${value.name}@${value.version}: resolved from a non-registry source (${redactResolved(value.resolved)}) — not covered by the age gate`, + ); + } else { + registryEntries.push(value); + } +} async function checkOne({ name, version }) { // Scoped names contain '/' which must be percent-encoded for the registry URL. // encodeURIComponent handles all unsafe chars (replace('/', ...) only hits the first). - const url = `https://registry.npmjs.org/${encodeURIComponent(name)}` - let res + const url = `https://registry.npmjs.org/${encodeURIComponent(name)}`; + let res; try { - res = await fetch(url, { headers: { Accept: 'application/vnd.npm.install-v1+json' } }) + res = await fetch(url, { + headers: { Accept: "application/vnd.npm.install-v1+json" }, + }); } catch (err) { - errors.push(`${name}: fetch failed (${err.message})`) - return + errors.push(`${name}: fetch failed (${err.message})`); + return; } if (!res.ok) { - errors.push(`${name}: registry returned ${res.status}`) - return + errors.push(`${name}: registry returned ${res.status}`); + return; } - let data + let data; try { - data = await res.json() + data = await res.json(); } catch { - errors.push(`${name}: invalid JSON from registry`) - return + errors.push(`${name}: invalid JSON from registry`); + return; } - const publishedAt = data.time?.[version] + const publishedAt = data.time?.[version]; if (!publishedAt) { - errors.push(`${name}@${version}: missing publish time in registry response`) - return + errors.push( + `${name}@${version}: missing publish time in registry response`, + ); + return; } - const publishedMs = new Date(publishedAt).getTime() + const publishedMs = new Date(publishedAt).getTime(); if (publishedMs > cutoffMs) { - const ageDays = Math.floor((Date.now() - publishedMs) / 86400_000) - const mergeAfter = new Date(publishedMs + MIN_AGE_DAYS * 86400_000).toISOString() - violations.push({ name, version, publishedAt, ageDays, mergeAfter }) + const ageDays = Math.floor((Date.now() - publishedMs) / 86400_000); + const mergeAfter = new Date( + publishedMs + MIN_AGE_DAYS * 86400_000, + ).toISOString(); + violations.push({ name, version, publishedAt, ageDays, mergeAfter }); } } -const queue = [...added.values()] -const CONCURRENCY = 8 +const queue = [...registryEntries]; +const CONCURRENCY = 8; async function worker() { while (queue.length) { - const next = queue.shift() - if (next) await checkOne(next) + const next = queue.shift(); + if (next) await checkOne(next); } } -await Promise.all(Array.from({ length: CONCURRENCY }, worker)) +await Promise.all(Array.from({ length: CONCURRENCY }, worker)); -let failed = false +let failed = false; if (errors.length > 0) { - console.error(`\n✗ ${errors.length} registry lookup error(s) — failing closed:`) - for (const e of errors) console.error(` - ${e}`) - failed = true + console.error(`\n✗ ${errors.length} issue(s) — failing closed:`); + for (const e of errors) console.error(` - ${e}`); + failed = true; } if (violations.length > 0) { - console.error(`\n✗ ${violations.length} entries younger than ${MIN_AGE_DAYS} days:`) + console.error( + `\n✗ ${violations.length} entries younger than ${MIN_AGE_DAYS} days:`, + ); for (const v of violations) { - console.error(` ${v.name}@${v.version}`) - console.error(` published: ${v.publishedAt} (${v.ageDays} days ago)`) - console.error(` mergeable after: ${v.mergeAfter}`) + console.error(` ${v.name}@${v.version}`); + console.error(` published: ${v.publishedAt} (${v.ageDays} days ago)`); + console.error(` mergeable after: ${v.mergeAfter}`); } - const latestMergeAfter = violations.map((v) => v.mergeAfter).sort().at(-1) - console.error(`\nEarliest this PR can merge: ${latestMergeAfter}`) - failed = true + const latestMergeAfter = violations + .map((v) => v.mergeAfter) + .sort() + .at(-1); + console.error(`\nEarliest this PR can merge: ${latestMergeAfter}`); + failed = true; } if (failed) { - console.error(`\nTo bypass for an urgent security fix, add the 'lockfile-age-skip' label to the PR.`) - process.exit(1) + console.error( + `\nTo bypass for an urgent security fix, add the 'lockfile-age-skip' label to the PR.`, + ); + process.exit(1); } -console.log(`✓ All ${added.size} new entries ≥ ${MIN_AGE_DAYS} days old.`) +console.log(`✓ All ${added.size} new entries ≥ ${MIN_AGE_DAYS} days old.`);