Fix path-resolution bug in changenotes.mts validate by using getChangenotes - #4156
mbg merged 1 commit into
Conversation
Sorry, something went wrong.
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.
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
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.
Sorry, something went wrong.
mbg
left a comment
There was a problem hiding this comment.
The changes look good, but one question about the approach you took.
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
There was a problem hiding this comment.
I deleted isValidAllChangenoteFiles because the filter became redundant, which just left it to perform reduce. At that point, I think the function became a thin wrapper, which doesn't add much value (IMO). The deleted tests themselves were also redundant because they tested the same conditions but at a different level of abstraction. It seemed "cleaner" to inline it and shorten the test suite, but I'm indifferent about it. If you have a strong preference for keeping it, I won't mind.
Sorry, something went wrong.
There was a problem hiding this comment.
I am happy to merge this as-is for now. It's an easy enough change to make down the line if needed.
Sorry, something went wrong.
isValidChangenoteFile receives a file name (no leading path) and then attempts to read that file, regardless of the CWD. If the user's CWD is not in unreleased-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 the assemble and validate commands. We can take advantage of that here in validate, 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?
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