Fix: stop the pre-commit hook rewriting generator-owned files, and check the hook's glob in CI - #636
Open
AmaadMartin wants to merge 2 commits into
Open
Fix: stop the pre-commit hook rewriting generator-owned files, and check the hook's glob in CI#636AmaadMartin wants to merge 2 commits into
AmaadMartin wants to merge 2 commits into
Conversation
added 2 commits
August 4, 2026 06:47
…glob in CI
The pre-commit hook maps `**/*.{json,md}` to `prettier --write`, but
`.prettierignore` listed only build output, so the release-please-generated
`CHANGELOG.md` files were in scope. Staging a two-line note on
`integrations/CHANGELOG.md` produced a +11/-14 staged diff; `core/CHANGELOG.md`
loses 264 of its 341 lines to the same rewrite. Ignore `CHANGELOG.md` (bare, so
it matches every workspace package now and in future) so the hook reproduces the
author's edit exactly.
Widen `format`/`format:check` to the same `**/*.{ts,json,md}` glob the hook
formats, so the formatting the hook writes is the formatting CI enforces and the
tree cannot silently drift again.
`package-lock.json` is ignored as a forward-looking guard for that widened
check: Prettier does not reformat it today (it routes `package.json` and
`package-lock.json` to the `json-stringify` parser, whose output is
byte-identical to npm's own serialization), but npm owns the file and it should
not become a CI formatting failure on three operating systems.
Produced by `npx prettier --write` on exactly these four paths, with no hand edits. They are the entire backlog the widened `format:check` surfaces: JSON and Markdown were never checked in CI even though the pre-commit hook formats them, so these had drifted out of the standard the hook applies. `.vscode/settings.json` also carried a trailing comma, which is not valid JSON.
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
Closes: #issue_number
Related: #issue_number
Problem:
Two related defects in the repository's formatting setup, both reproduced against
this checkout with the Prettier version that resolves from the committed lockfile
(
prettier@3.8.4).1. The pre-commit hook rewrites release-please-generated changelogs.
package.jsonmaps**/*.{json,md}toprettier --writein itslint-stagedblock, and
.prettierignorelisted onlydist/,node_modules/anddev/src/browser. The three changelogs release-please owns(
release-please-config.jsonsetschangelog-path: CHANGELOG.mdforcore,devand
integrations) were therefore in scope, and none of them is Prettier-clean —Prettier rewrites release-please's
*bullets to-and strips its blank lines:prettier --writecore/CHANGELOG.mddev/CHANGELOG.mdintegrations/CHANGELOG.mdSo a contributor who appends two lines to a changelog and commits gets a staged
diff of +11/-14 on
integrations/CHANGELOG.md— the real change buried underformatting noise nobody asked for.
2. Nothing enforces the formatting the hook writes.
format:checkwasprettier "**/*.ts" --check, and.github/workflows/validation.yamlruns it onubuntu/windows/macos. JSON and Markdown were never checked in CI even though the
hook formats them, so the tree had drifted: four hand-authored files were not
Prettier-clean, and any contributor who happened to stage one of them picked up an
unrelated reformat in their PR.
Solution:
.prettierignore— ignore the files a generator owns:CHANGELOG.md, bare..prettierignoreuses gitignore pattern syntax, so thismatches every package changelog at any depth and keeps matching when a new
workspace package is added. This is the fix for defect 1. Prettier applies
.prettierignoreeven to paths passed explicitly on the command line, which isexactly how
lint-stagedinvokes it, so the hook now reproduces the author'sedit byte for byte.
package-lock.json. To be explicit about why, since it would otherwise look likea bug fix it is not: Prettier does not currently reformat the lockfile.
Prettier maps the filenames
package.jsonandpackage-lock.jsonto itsjson-stringifyparser, which emits exactlyJSON.stringify(value, null, 2)—byte-identical to what npm itself writes. Verified here:
npx prettier package-lock.jsonis byte-identical to the file on disk, and the file isbyte-identical to
JSON.stringify(JSON.parse(src), null, 2) + "\n". The entry isa forward-looking guard: once
format:checkmatches**/*.json, CI wouldstart checking a 566 KB npm-generated artifact on three operating systems, and
any future change in npm's serializer would surface as a bogus formatting
failure. npm owns the file.
package.json— widenformatandformat:checkfrom**/*.tsto**/*.{ts,json,md}, i.e. exactly the file set the pre-commit hook formats, so theformatting the hook writes is the formatting CI enforces and the tree cannot
silently drift again. Only those two string values changed; the
lint-stagedblock,
.prettierrc.js,.husky/pre-commitandvalidation.yamlare untouched(
validation.yamlalready runsnpm run format:check, so it picks the wider globup for free).
Four hand-authored files — reformatted by running
npx prettier --writeonexactly those paths, with no hand edits. This is the whole backlog the widened
check surfaces:
.github/ISSUE_TEMPLATE/bug_report.md-bullets, blank line before an ordered list, trailing newline.github/ISSUE_TEMPLATE/feature_request.md.vscode/settings.jsondev/README.mdThe generated changelogs are deliberately left byte-for-byte as release-please
wrote them — cleaning them up here would be the exact noise this change exists to
prevent.
Cost. The widened check is slower, measured on this machine as the mean of
three runs with a warm
node_modules:**/*.ts15.6 s →**/*.{ts,json,md}16.9 s, i.e. +1.3 s per CI leg.
Collision check. All 535 open PRs on the fork were listed and the plausibly
adjacent ones diffed. Two touch the same files but neither lands this change:
fix/node-protocol-build-banner-and-js-lint-scope, itself stacked onChore: lint JavaScript files, not just TypeScript #426) widens the same two script lines to
**/*.{ts,js,cjs,mjs}and appendsapi-reference/to.prettierignore. That is the JavaScript side of the samegap (
lint-stagedformats**/*.{js,ts}whileformat:checkcovered only.ts) — a different defect, a disjoint set of reformatted files, andcomplementary to this PR.
fix/remove-dead-dev-src-browser-ignores) removes thedev/src/browserline from
.prettierignore.This PR was not stacked on either, deliberately: the two of them modify
.prettierignorein mutually exclusive ways, so there is no single base thatcovers both; and #552's base chain reaches into
core/srcTypeScript, which woulddrag unrelated changes into a diff that is meant to contain no TypeScript at all.
Both overlaps are single-line textual conflicts that resolve trivially for whichever
lands second.
Testing Plan
Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.
No test file is added, and that is not an oversight. This change adds zero
lines of executable code — it is two glob strings, two
.prettierignorepatterns,and Prettier's own output on four documents. A
*_test.tsthat shells out toPrettier to assert its own config would be testing Prettier, not ADK. The
verification below is run against the real
husky+lint-stagedpipelineinstead, and each check was also run in a state where it fails, so none of it is
a green light with no signal.
Unit Tests:
[ ] I have added or updated unit tests for my change. — no executable code is
added; see above.
[x] All unit tests pass locally. — unchanged by this PR (no TypeScript is
touched).
npm run lintexits 0; the widened check is the gate that mattersand is covered below.
1. The widened check passes on the resulting tree.
2. Generated files are genuinely out of scope (this is how
lint-stagedinvokes Prettier — explicit paths):
3. Package manifests are still in scope and still clean — guards against the
bare
package-lock.jsonpattern over-matching:Also confirmed through the API, which needs the ignore file passed explicitly:
getFileInfo(f, {ignorePath: '.prettierignore'})reportsignored: truefor thethree changelogs and
package-lock.json, andignored: falsefor all fourpackage.jsonfiles and.release-please-manifest.json.4. Nothing else regressed.
npm run lintexits 0.Proof the checks can fail
Regression proof for defect 1 — real
git committhrough.husky/pre-commit:The committed content still carries release-please's
*bullets, unmodified. Samesequence before this change:
11 14.Attribution mutation — revert only the two
.prettierignoreentries, changenothing else, re-run the real hook:
So the
.prettierignoreentries, and not the script change, are what fix defect 1.Mutation proof for defect 2 — drift a Markdown file, then run both globs:
The old glob would have shipped that drift; the widened one catches it.
Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.
From a clean checkout of this branch:
CI result.
run-testspassed on all three legs (ubuntu-latest,windows-latest, macos-latest). The Windows leg was the one worth watching, since
.prettierrc.jssetsendOfLine: "auto"and the brace glob has to survivePowerShell; its
Run format checkstep reportsAll matched files use Prettier code style!, so the widened glob is fine there.The first Windows attempt failed in the earlier
Run tests and check code coveragestep —tests/integration/a2a/input_required/input_required_test.ts,Error: CLI exited prematurely with code 1attests/integration/test_case_utils.ts:341— with 0 failing tests (2677passed, 1 failed suite). That is the known Windows spawned-server flake, not
this change: this diff contains no TypeScript and no test file, so there is no
path by which two npm script strings, two
.prettierignorelines and fourreformatted documents reach a spawned CLI subprocess. It passed on re-run without
any code change.
api-reference/was checked and is not a hazard for the widened glob:typedocemits zero
.json/.md/.tsfiles (verified by generating it), and CI'sdocs:checkrunstypedoc --emit noneafterformat:checkanyway.Checklist
[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas. — the one
comment added is the
# Generated files: owned by their generators, not Prettier.header in.prettierignore.[x] I have added tests that prove my fix is effective or that my feature works. —
as executable verification against the real hook rather than a test file; see
"Proof the checks can fail" above for the mutations and their failure output.
[x] New and existing unit tests pass locally with my changes.