[jsweep] Clean write_large_content_to_file.cjs#47466
Conversation
- Remove misleading triple-slash reference to @actions/github-script types (file is a pure Node.js CJS module, not a github-script context) - Use err.message instead of String(err) in error messages for cleaner output - Add 2 new tests covering error paths (mkdir failure, writeFile failure) bringing total test count from 20 to 22 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #47466 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
There was a problem hiding this comment.
Pull request overview
Simplifies error formatting in the large-content file writer and updates its tests.
Changes:
- Removes an unused type reference.
- Uses
err.messagein filesystem errors. - Adds directory and file-write error tests.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/write_large_content_to_file.cjs |
Cleans type metadata and error messages. |
actions/setup/js/write_large_content_to_file.test.cjs |
Adds error-path tests. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
actions/setup/js/write_large_content_to_file.test.cjs:256
- These substring checks also pass with the old
String(err)output because...: Error: disk fullincludes both strings. Assert the full error-message shape so this regression test fails against the previous implementation.
expect(() => writeLargeContentToFile("{}")).toThrow("Failed to write file");
expect(() => writeLargeContentToFile("{}")).toThrow("disk full");
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Medium
| expect(() => writeLargeContentToFile("{}")).toThrow("Failed to create directory"); | ||
| expect(() => writeLargeContentToFile("{}")).toThrow("permission denied"); |
There was a problem hiding this comment.
The changes look correct and well-tested.
- Removing the unused
/// <reference types="@actions/github-script" />is correct — this is a plain Node.js CJS module. - Using
err.messageinstead ofString(err)avoids the redundantError:prefix in the wrappingnew Error(...)message;{ cause: err }still preserves the original for full diagnostics. - The two new error-path tests directly validate the behavioral change.
Minor note: err.message would be undefined if a non-Error value is thrown, whereas String(err) handles that. Since Node.js fs methods always throw Error instances, this is not a practical concern here.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 13.5 AIC · ⌖ 4.5 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — clean, well-scoped change with solid test coverage.
📋 Summary
Positive Highlights
- ✅ Removed unused
/// <reference types>triple-slash directive — reduces confusion for readers - ✅
err.messageoverString(err)is the right call: avoids duplicating theError:prefix since the wrappingnew Error(...)already adds contextual labeling - ✅ Two new regression tests cover both error paths and verify the exact message shape (prefix + original message), not just that an error is thrown
- ✅
finallyblocks ensure mock cleanup — no test pollution
Minor Observation (non-blocking)
Each new test calls writeLargeContentToFile twice inside the try block (once per expect). This is harmless here, but capturing const fn = () => writeLargeContentToFile("{}") and reusing it would make the intent clearer.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 13.6 AIC · ⌖ 4.55 AIC · ⊞ 6.7K
Comment /matt to run again
There was a problem hiding this comment.
✅ Approve — clean sweep with one non-blocking advisory
The triple-slash reference removal is correct; the file is pure Node.js CJS. Error message cleanup and the new tests are well-structured.
One non-blocking advisory posted inline: err.message is undefined for non-Error thrown values, whereas String(err) was fully defensive. Low-risk in practice (Node fs always throws Error), but worth a ?? String(err) fallback.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 13.3 AIC · ⌖ 4.58 AIC · ⊞ 5.7K
Comment /review to run again
Comments that could not be inline-anchored
actions/setup/js/write_large_content_to_file.cjs:16
err.message may be undefined for non-Error throws: if fs.mkdirSync ever throws a non-Error value, err.message is undefined, producing "Failed to create directory /path: undefined".
<details>
<summary>💡 Suggested fix</summary>
String(err) was strictly more defensive. Consider a fallback to retain cleanliness while handling any thrown value:
throw new Error(`Failed to create directory ${logsDir}: ${err?.message ?? String(err)}`, { cause: err });Same applies to …
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics (2 tests)
i️ Inflation noteBoth tests add ~30 lines vs 2 production lines changed (ratio ~15:1). This is expected here: the tests require saving/restoring mocked Verdict
|
|
@copilot run pr-finisher skill |
…ile tests Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Implemented. I ran the PR-finisher flow and pushed commit |
|
@copilot please run the Unresolved review feedback:
|
|
🎉 This pull request is included in a new release. Release: |
Summary
Cleaned
write_large_content_to_file.cjsas part of the daily jsweep.Context
Execution context: Node.js (pure CJS module — not github-script)
Changes Made
Removed misleading
/// <reference types="@actions/github-script" />triple-slash reference — this file is a plain Node.js CJS module that usesfs,path, andcryptodirectly. The reference was unused and potentially confusing.Use
err.messageinstead ofString(err)in both error messages —String(err)would produceError: permission deniedwhileerr.messagegives cleanerpermission denied, avoiding redundancy since the wrappingnew Error(...)already adds contextual labeling.Test Improvements
Added 2 new error-path tests, growing the suite from 20 → 22 tests:
should include err.message (not String(err)) in directory creation errorshould include err.message (not String(err)) in file write errorValidation ✅
npm run format:cjsnpm run lint:cjsnpm run typechecknpm run test:js(22/22 tests)Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
traces.example.comSee Network Configuration for more information.
Run URL: https://github.com/github/gh-aw/actions/runs/29984131428