b51cbf96cfbfad2fa4acac3d3c6d29c04aaf72d6
4 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Tom Boucher
|
cafb874c4a |
fix(#1224): accept --pr 0 placeholder at changeset creation (#1231)
* fix(#1224): accept --pr 0 placeholder at changeset creation The required-field guard `!opts.pr` treated the integer 0 as falsy, rejecting the documented `pr: 0` two-push placeholder with a usage error (exit 2). Non-numeric `--pr abc` (NaN) was also silently accepted before (passes `!NaN === true`... actually `!NaN` is true, so NaN would trigger the guard already). The new explicit checks use `opts.pr === null` for missing flag and `Number.isNaN` for non-numeric input, accepting all finite integer values including 0. The merge-time safety net in parse.cjs (`pr <= 0` → INVALID_PR) is unchanged — a pr:0 fragment is still rejected at lint/render time. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(#1224): backfill changeset PR number (#1231) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Tom Boucher
|
a28dcec981 |
chore(#597): replace count-based ratchet guards with AST lint + named-set allowlists (#603)
The windows-test-parity ratchet greps test source for fs.rmSync-without-
maxRetries (and six other Windows-portability anti-patterns), failing when an
integer offender COUNT exceeds a frozen baseline (rmSync: 95). A count ratchet
is a Goodhart metric: fixing one offender and adding another keeps the count
constant, so a new defect slips through green. Replace it — and every other
count ratchet in the repo — with a layered, masking-proof design.
Behavioral seam test
- tests/helpers-cleanup.test.cjs proves helpers.cleanup() carries the Windows
EBUSY retry budget. cleanup() delegates retries to Node's fs.rmSync via
maxRetries (it owns no loop), so the test asserts the option contract
(recursive/force/maxRetries>0/retryDelay>0) + real-FS removal + the cwd-guard,
rather than a loop that does not exist. The EBUSY risk is now tested ONCE at
the helper, not approximated textually at every call site.
Write-time ESLint rule (AST-accurate, replaces the grep)
- eslint-rules/no-raw-rmsync-in-tests.cjs (error in tests/**/*.test.cjs) bans
raw fs.rmSync, steering to cleanup(). Catches member, computed (fs['rmSync']),
destructured and aliased forms; escape hatch is inline
`// eslint-disable-next-line local/no-raw-rmsync-in-tests -- <reason>` only.
- Migrated 336 raw fs.rmSync teardown calls across ~116 test files to cleanup().
~18 genuinely load-bearing sites (mid-test SUT/fault-injection removals,
error-swallowing or name-colliding local teardown helpers) keep the raw call
with an inline eslint-disable + reason.
Shared anti-ratchet primitive
- scripts/lib/allowlist-ratchet.cjs:
- assertWithinAllowlist: fails on NOVEL ids (new offender introduced) AND on
STALE ids (a known offender was fixed but not pruned) — identity, not count,
and a ratchet DOWN toward zero.
- assertTightCeiling: a size/length budget whose ceiling must stay within a
grace band of the high-water mark, so budgets may only tighten, never creep.
Ratchets converted onto the primitive
- windows-test-parity-guard.test.cjs: rmSync rule deleted (now ESLint-enforced);
the remaining six patterns moved from integer baselines to named-set
allowlists with ratchet-down.
- scripts/lint-test-file-count.{cjs,allowlist.json}: per-module integer counts →
named filename sets (closes the swap-a-file-keep-the-count blind spot); a
module dropping under cap now FAILS to force pruning its allowlist entry.
- enh-2790 skill-count `<= 63` → named skill allowlist (ratchets toward ~58).
Size budgets hardened (tighten-only)
- agent-size / workflow-size / feat-3039 help-tiered: ceilings lowered to the
current high-water mark and an assertTightCeiling anti-creep check added per
tier. Fixed external-contract limits (description ≤100 chars, agent ≤100 KB)
are intentionally left as-is — they are not grandfathered creeping budgets.
No user-facing behavior change (tests + tooling only); no USER_FACING_PREFIXES
touched, so no changeset fragment is required.
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
|
||
|
Cristian Uibar
|
7f8b5701bf |
Enforce documentation updates via lint:docs + PR templates (#3651)
* Enforce documentation updates via lint:docs + PR templates (#3213) New scripts/lint-docs-required.cjs + Docs Required CI workflow fail any PR whose changeset fragment is typed Added / Changed / Deprecated / Removed without modifying at least one file under docs/. Mirrors scripts/changeset/lint.cjs: pure evaluateLint({ changedFiles, fragments, labels }) returning { ok, reason, triggering } over a frozen LINT_REASON enum; CLI wrapper reads the PR diff and parses each touched changeset fragment via the existing parseFragment helper. Escape hatches: - no-docs PR label (global) - per-fragment <!-- docs-exempt: <reason> --> marker, all triggering fragments must carry it for the PR to pass Fixed and Security fragments do not trigger the lint — bug fixes restore documented behavior, they do not introduce new behavior to document. PR templates (enhancement.md, feature.md) gain a Documentation checklist section pointing at the which-doc-to-update matrix. CONTRIBUTING.md adds a Documentation Updates section codifying that matrix, the English-canonical language policy for docs/ and the root README, and the two opt-out routes. Closes #3213 * Address Codex review: fail-closed on malformed fragments and strip docs-exempt marker from rendered release notes (#3213) Two P2 issues caught by `codex review --base main`: 1) Malformed fragments could silently bypass docs enforcement. parseFragment would return ok:false on a triggering Added fragment with bad frontmatter and readFragmentsFromDisk dropped it, so evaluateLint saw no triggering fragments and passed. The changeset-required lint only checks fragment _presence_ not _validity_, so the assumed fallback did not catch it. Fix: readFragmentsFromDisk now returns { fragments, malformed }; evaluateLint accepts a malformed param and emits a new FAIL_MALFORMED_FRAGMENT verdict that outranks every OK path (including the no-docs label) — a parse failure must be fixed before docs lint can decide anything else. 2) The per-fragment <!-- docs-exempt: reason --> marker lived in the fragment body, so the existing changelog (serializeChangelog) and GitHub release-notes (formatBullet) serializers published it verbatim. Worse, both serializers append `(#NNNN)` to the body's last line — with the marker as the trailing line, the PR suffix attached to the hidden comment instead of the visible bullet. Fix: parseFragment now extracts the marker into a typed `docsExempt` field and strips it from `body`, so all downstream renderers produce clean output without remembering to strip. The regex is anchored to its own line (^...$ with m flag) so inline mentions of the marker syntax in documentation (e.g. inside backticks) cannot accidentally exempt a fragment. Bounded character class [^\n>] keeps the regex linear-time. Test additions: - tests/lint-docs-required.test.cjs: FAIL_MALFORMED_FRAGMENT coverage, end-to-end "Added fragment with bad pr → malformed → fail-closed" regression test, updated readFragmentsFromDisk return-shape assertions, isExemptFragment now checks the typed docsExempt field rather than body content. - tests/changeset-parse.test.cjs: extractDocsExempt extraction cases (with/ without reason, case-insensitive, EMPTY_BODY when body is only a marker), inline-mention false-positive guard, real-marker-wins-when-also-inline test. - tests/changeset-new.test.cjs: fragment shape now includes docsExempt: null. CONTRIBUTING.md updated to clarify the "on its own line" requirement and the parse-time stripping behavior. The bootstrap fragment cleaned up so its body no longer contains a literal marker example that would have triggered the false-positive case. Full suite: 9696/9696 pass. * CRLF-safe docs-exempt marker stripping (Codex review pass 2, #3213) Second `codex review --commit` pass caught a CRLF regression in the docs-exempt extraction added in the previous commit. Repro: a Windows-authored fragment ---\r\ntype: Added\r\npr: 1\r\n---\r\nFeature.\r\n\r\n<!-- docs-exempt: x -->\r\n would parse to body `Feature.\r\n\r\n\r` because: - The previous trailing-newline slice trimmed only `\n`, leaving `\r`. - DOCS_EXEMPT_RE was anchored with `$` only — in multiline mode `$` matches before `\n` but does not consume `\r`, so the marker line's trailing `\r` was left behind after replace. - The cleanup regex stripped trailing `\n` but not `\r`. Net effect: serializeChangelog emitted - Feature.\r \r \r (#1) — the `(#1)` PR suffix landed on a blank line instead of attached to the visible bullet. Same bug surfaces in github-release-notes formatBullet. Fix: - DOCS_EXEMPT_RE: add `\r?` before `$` so the regex consumes the CR of a CRLF terminator. Switch reason character class from `[^\n>]` to `[^\r\n>]` so CRLF-authored reasons don't carry a trailing `\r`. - extractDocsExempt cleanup: `[ \t\r]+$/gm` strips trailing `\r` on each line; `(?:\r?\n){3,}` collapses CRLF triple-blank-lines; `[\r\n]+$` strips every trailing line terminator (LF or CR). - parseFragment trailing-newline slice: CRLF-aware — strips `\r\n` (2 chars) before falling through to single `\n`. Tests: two CRLF regression cases in tests/changeset-parse.test.cjs — Codex's exact repro (end-to-end through serializeChangelog) plus the no-marker CRLF passthrough case. Full suite: 9698/9698 pass. * CRLF regression test asserts on parseChangelog IR not rendered text (Codex review pass 3, #3213) Third `codex review` pass caught that the CRLF regression test added in the previous commit asserted on serializeChangelog's rendered Markdown via `out.split('\n')` + `assert.match`. That violates CONTRIBUTING.md's "Prohibited: Raw Text Matching on Test Outputs" rule and the documented serializer contract in `serialize.cjs`: > tests assert via round-trip (parse(serialize(ir))) > rather than by inspecting serialized text Replace the regex check with the established `parseChangelog(out)` round-trip and assert on the structured `{ body: 'Feature.', pr: 1 }` bullet. This is also a stronger regression check than the substring match: Codex's own probe in the review session confirmed the pre-fix buggy body shape (`Feature.\r\n\r\n\r`) breaks parseChangelog's bullet regex entirely (returns `bullets: []`), so the round-trip catches the exact failure mode end-to-end. Full suite: 9698/9698 pass. * Address CodeRabbit findings: anchor link + require non-empty docs-exempt reason (#3213) CodeRabbit's review on the PR caught two actionable issues, both quick wins. Anchor link in PR templates pointed to a heading that does not exist. The CONTRIBUTING.md heading "Documentation Updates — Update the Relevant Docs" contains an em-dash, which GitHub strips entirely when generating anchor slugs (it does NOT collapse to a hyphen). The actual anchor is #documentation-updates-update-the-relevant-docs (single hyphen between every word), not #documentation-updates--update-the-relevant-docs (double hyphen where the em-dash was). Both feature.md and enhancement.md fixed. The docs-exempt marker matched a bare `<!-- docs-exempt -->` with no reason, which defeats the entire purpose of the escape hatch — the marker exists to leave an audit trail explaining WHY a PR is exempt. Without a reason it is a silent bypass. Fix: DOCS_EXEMPT_RE now requires both the colon AND a non-whitespace first reason character. Bare `<!-- docs-exempt -->`, empty `<!-- docs-exempt: -->`, and whitespace-only `<!-- docs-exempt: -->` are all rejected as if the marker were not present (`docsExempt: null`). The lint then falls through to its normal docs-required / no-docs-label checks. `isExemptFragment` in the lint module tightened too — defense-in-depth: even if a caller constructs a fragment with `docsExempt: ''` directly, it does not count as exempt. The predicate now requires `typeof === 'string'` and non-empty after trim. Tests: - changeset-parse.test.cjs: three new explicit-rejection cases (bare marker, empty reason, whitespace-only reason). Existing DOCS_EXEMPT_RE shape test extended with negative assertions for the same three forms. - lint-docs-required.test.cjs: prior "empty reason still exempt" test inverted — empty/whitespace docsExempt now produces FAIL_DOCS_MISSING. isExemptFragment helper test extended with the same negative cases. - CONTRIBUTING.md: clarified that the reason is required and non-empty. Skipped CodeRabbit's third finding ("use `npm run lint:docs` in CI workflow instead of `node scripts/lint-docs-required.cjs`") — the existing changeset-required.yml uses the direct-node form for the equivalent changeset lint, so the new docs-required.yml is convention-consistent. Switching one without the other would create drift, and switching both is out of scope for #3213. Bootstrap fragment continues to extract cleanly under the stricter regex (verified — `docsExempt` field still contains the full bootstrap reason). Full suite: 9701/9701 pass. |
||
|
Tom Boucher
|
9d5db87249 |
feat(#2975): adopt changeset-fragment workflow to eliminate CHANGELOG conflicts (#2978)
* feat(#2975): adopt changeset-fragment workflow to eliminate CHANGELOG conflicts Two PRs that both edit `### Fixed` in CHANGELOG.md always conflict on merge. Recently bit on #2960/#2972 in the same session — fix-the-conflict-and-rebase tax. Replace the shared-file model with per-PR fragment files that never share lines. Implementation built TDD per #2975, vertical slices with structured-IR assertions throughout: scripts/changeset/parse.cjs - fragment text → typed record + frozen FRAGMENT_ERROR enum (8 tests) scripts/changeset/render.cjs - fragments → structured IR with Keep-a-Changelog section ordering (2 tests) scripts/changeset/serialize.cjs - IR ↔ markdown round-trip pair (parse(serialize(ir)) === ir, 3 tests) scripts/changeset/cli.cjs - file-I/O wrapper with --json mode; reads .changeset/, folds into CHANGELOG.md, deletes consumed fragments. Idempotent. (1 test) scripts/changeset/lint.cjs - pure verdict (changedFiles, labels) → { ok, reason } via LINT_REASON enum. Honors `no-changelog` label. (5 tests) scripts/changeset/new.cjs - fragment scaffolder with random adjective-noun-noun filename. Tests assert via parseFragment round-trip. (3 tests) Total: 22 tests, all assertions on typed structured fields. No regex on text, no String#includes on file content. Lint clean across 356 test files. Supporting: .changeset/README.md - format spec + workflow docs .changeset/eager-hawks-rally.md - dogfood fragment for THIS PR (will be the first thing the new release tool consumes) .github/workflows/changeset-required.yml - CI: every PR runs lint.cjs package.json - npm run changeset, changelog:render, lint:changeset CONTRIBUTING.md - new "CHANGELOG Entries — Drop a Fragment" section between PR Guidelines and Testing Standards Closes #2975 * fix(#2975): address CodeRabbit findings on changeset workflow 7 valid findings (4 Major, 3 Minor); all addressed: scripts/changeset/parse.cjs - Preserve fragment body verbatim. Previously body.trim() ate intentional leading whitespace (code blocks, etc.); now trim() is used only for the emptiness check, and a single trailing newline is stripped (the editor-added one) so well-formed fragments round-trip byte-for-byte. Added a regression test asserting a code-block-leading body is preserved. scripts/changeset/cli.cjs - Validate flag values during argument parsing. parseArgs now returns { ok, opts | error }; rejects `--repo` etc. with no following value or with another flag as the value. main() surfaces the error message before exiting 2. - Handle post-write fragment-deletion failures. After CHANGELOG.md is written, any unlink failure is captured into a structured deleteFailures list with reason 'fail_fragment_delete'; cmdRender returns exitCode=1 with the partial-failure detail instead of leaving the changelog updated and fragments behind (which would cause double-consumption on rerun). scripts/changeset/lint.cjs - Treat CHANGELOG.md as a linted user-facing path. Direct edits to CHANGELOG.md (the bypass route around the new workflow) now fail the lint with FAIL_MISSING_FRAGMENT. Added a regression test for that case. - Use cp.execFileSync instead of cp.execSync for the git diff call. Eliminates the shell-interpolation surface on GITHUB_BASE_REF; git's own arg parser remains the validator. scripts/changeset/new.cjs - Atomic fragment creation. existsSync() + writeFileSync was racy under concurrent invocations. Now writeFileSync uses { flag: 'wx' } which fails EEXIST on collision; the random-name retry loop catches EEXIST and re-rolls. Throws explicitly after 16 attempts rather than silently overwriting. .changeset/README.md - Add language tag `md` to the format example fence (markdownlint MD040). All 25 changeset tests pass; lint clean (356 test files, 0 violations). * fix(#2975): sanitize --type and validate flag values in new.cjs (CR fixes) Two CR findings on scripts/changeset/new.cjs: 1. (Minor) `type` was embedded in frontmatter without sanitization. A newline in the value (e.g. `--type 'Fixed\ntype: Added'`) would corrupt the fragment. scaffoldFragment now validates `type` against the Keep-a-Changelog ALLOWED_TYPES set BEFORE writing — same set parse.cjs uses on consume. Throws with a typed error referencing the allowed values; tests cover the newline case + 4 other non-allowed values. 2. (Minor) `--repo` (and other value-taking flags) without a value silently set opts.repo to undefined, which produced a cryptic ERR_INVALID_ARG_TYPE deep inside path.join. parseArgs now mirrors the cli.cjs convention: returns { ok, opts | error }, validates that the next token exists and is not itself another flag, and surfaces a precise "missing value for --repo" message before exit. Added 3 tests: missing-trailing-value, flag-as-value, well-formed. 29 tests pass across the changeset suite (4 new regression tests). |