Fix path-resolution bug in changenotes.mts validate by using getChangenotes - #4156
mario-campos wants to merge 1 commit into
Conversation
fc0dbb2 to
dedce4e
Compare
changenotes.mts validate commandchangenotes.mts validate by using getChangenotes
bf6da6f to
609b22f
Compare
609b22f to
e1b1575
Compare
This reduces duplicate code between `assemble` and `validate`. It also has the benefit of fixing a bug in the current implementation of `validate`, where `isValidChangenoteFile` receives a relative file name where it should receive an absolute one.
e1b1575 to
738bd62
Compare
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The focused change correctly fixes working-directory-dependent path resolution without leaving unresolved usages.
Review effort: Balanced
Findings: None
What changed in this PR
Fixes changenote validation from arbitrary working directories by validating repository-rooted paths.
Changes:
- Uses
getChangenotes()to resolve changenote paths. - Removes the obsolete aggregate validator and its tests.
| File | Description |
|---|---|
pr-checks/changenotes.mts |
Validates changenotes using resolved paths. |
pr-checks/changelog/validate.mts |
Removes the unused aggregate validator. |
pr-checks/changelog/validate.test.mts |
Removes tests for the deleted helper. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
mbg
left a comment
There was a problem hiding this comment.
The changes look good, but one question about the approach you took.
| const allChangenotesValid = getChangenotes().reduce( | ||
| (r, changenote) => r && isValidChangenoteFile(changenote.absolutePath), | ||
| true, | ||
| ); |
There was a problem hiding this comment.
Rather than replacing the call to isValidAllChangenoteFiles with this inline implementation that uses getChangenotes(), do you think it would be better to update the existing implementation of isValidAllChangenoteFiles to accept ChangenoteFile[] and pass it the result of getChangenotes() here? That way you could keep (most of) the unit tests. You would need to move the definition of ChangenoteFile somewhere that is accessible to both files.
isValidChangenoteFilereceives a file name (no leading path) and then attempts to read that file, regardless of the CWD. If the user's CWD is not inunreleased-change-notes, then the command will likely fail to open the file.Incidentally, for #4155, I've added
getChangenotes, which consolidates some duplicative code between theassembleandvalidatecommands. We can take advantage of that here invalidate, and in so doing, fix the bug.Risk assessment
For internal use only. Please select the risk level of this change:
Which use cases does this change impact?
Workflow types:
Products:
Environments:
How did/will you validate this change?
.test.tsfiles).If something goes wrong after this change is released, what are the mitigation and rollback strategies?
How will you know if something goes wrong after this change is released?
Are there any special considerations for merging or releasing this change?
Merge / deployment checklist