Move withTmpFile next to withTmpDir - #4140
mario-campos merged 5 commits into
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Temporary files leak on callback failure, and the committed JavaScript output is not regenerated.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 1 · 1
| Severity | Finding |
|---|---|
| src/util.ts — Regenerate the committed JavaScript output | |
| src/util.ts — Preserve cleanup when the callback fails |
Moves withTmpFile into the shared utility module and reuses it in changelog validation tests.
Changes:
- Adds withTmpFile using withTmpDir.
- Removes the duplicated test-local implementation.
| File | Description |
|---|---|
| src/util.ts | Adds the shared temporary-file helper. |
| pr-checks/changelog/validate.test.mts | Uses the shared 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.
Thanks for preparing this PR! I am happy for us to add withTmpFile to util.ts, even though withTmpFile is not currently used by the main codebase. It won't affect the bundled JS (as you noted already in response to Copilot) since it's not reachable from the main codebase's entry points.
I added a few small suggestions, but nothing major.
Sorry, something went wrong.
Co-authored-by: Michael B. Gale <mbg@github.com>
mbg
left a comment
There was a problem hiding this comment.
As discussed elsewhere, we are happy to keep the changes to sync.sh as they are, since they should resolve the issue permanently and there's no immediately obvious drawback to that approach right now. Thanks for providing the context on that and addressing my other feedback!
Sorry, something went wrong.
Now that changetool has been moved to pr-checks (see #4128), this PR moves withTmpFile to util.ts, alongside withTmpDir.
Additionally, it refactors withTmpFile to make use of withTmpDir internally, reducing a bit of the duplication of creating a temporary directory.
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