msd-core/.github/PULL_REQUEST_TEMPLATE/feature.md at 080bacdb4b7155b65858de8ad24b2ee25cb25d9e

Files

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.

2026-05-16 13:09:54 -04:00

4.4 KiB

Raw Blame History

Feature PR

Using the wrong template? — Bug fix: use fix.md — Enhancement to existing behavior: use enhancement.md

Linked Issue

Required. This PR will be auto-closed if no valid issue link is found. The linked issue must have the approved-feature label. If it does not, this PR will be closed without review — no exceptions.

Closes #

⛔ No approved-feature label on the issue = immediate close. Do not open this PR if a maintainer has not yet approved the feature spec. Do not open this PR if you wrote code before the issue was approved.

Feature summary

What changed

New files

File	Purpose

Modified files

File	What changed

Implementation notes

Spec compliance

Testing

Test coverage

Platforms tested

macOS
Windows (including backslash path handling)
Linux

Runtimes tested

Claude Code
Gemini CLI
OpenCode
Codex
Copilot
Other: ___
N/A — specify which runtimes are supported and why others are excluded

Scope confirmation

The implementation matches the scope approved in the linked issue exactly
No additional features, commands, or behaviors were added beyond what was approved
If scope changed during implementation, I updated the issue spec and received re-approval

Documentation

CI enforces this — lint:docs fails any PR with an Added / Changed / Deprecated / Removed changeset fragment that does not also touch at least one file under docs/. Features almost always trigger Added. See CONTRIBUTING.md → Documentation Updates.

Updated the relevant file(s) under docs/ to reflect this feature
- New command or flag → docs/COMMANDS.md and docs/FEATURES.md
- New workflow or behavior → docs/USER-GUIDE.md
- Configuration / schema change → docs/CONFIGURATION.md
- Architectural change → docs/ARCHITECTURE.md and/or docs/adr/
- Agent or skill change → docs/AGENTS.md
All docs/ content added in this PR is written in English (translated READMEs README.pt-BR.md / README.zh-CN.md / README.ja-JP.md / README.ko-KR.md are community-maintained and do not need to be updated in this PR)
If genuinely no user-facing docs impact (rare for features — explain in PR), apply the no-docs label or add  inside each triggering changeset fragment.

Checklist

Issue linked above with Closes #NNN — PR will be auto-closed if missing
Linked issue has the approved-feature label — PR will be closed if missing
All acceptance criteria from the issue are met (listed above)
Implementation scope matches the approved spec exactly
All existing tests pass (npm test)
New tests cover the happy path, error cases, and edge cases
.changeset/ fragment added with a user-facing description of the feature (npm run changeset -- --type Added --pr <NNN> --body "...")
No unnecessary external dependencies added
Works on Windows (backslash paths handled)

Breaking changes

None

4.4 KiB Raw Blame History

Feature PR

Linked Issue

Feature summary

What changed

New files

Modified files

Implementation notes

Spec compliance

Testing

Test coverage

Platforms tested

Runtimes tested

Scope confirmation

Documentation

Checklist

Breaking changes

Screenshots / recordings

4.4 KiB

Raw Blame History