[jsweep] Clean validate_memory_files.cjs#48126
Conversation
…ry_files - Fix ESM/CJS mismatch: replace bare require() with createRequire(import.meta.url) to match the established test pattern used across the codebase - Add test asserting core.info is called on the 'allow all' branch (no allowedExtensions) - Add test asserting core.info is called on the 'allow all' branch (empty allowedExtensions) - Add test asserting core.info is called when memory directory does not exist Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #48126 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Updates the memory-file validator tests to use the established CJS loading pattern and verify informational logging paths.
Changes:
- Loads the CJS module through
createRequire. - Tests allow-all and missing-directory
core.infomessages.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/validate_memory_files.test.cjs |
Modernizes module loading and expands logging assertions. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Medium
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The changes look correct:
createRequire(import.meta.url)fix is consistent with the established codebase pattern.- All 3 new tests accurately cover the previously untested
core.infocall paths and match the source logic.
No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 12.3 AIC · ⌖ 5.59 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — this PR is a clean test-only improvement that is ready to merge.
📋 Key Themes & Highlights
Positive Highlights
- ✅ ESM/CJS interop fix —
createRequire(import.meta.url)is the correct pattern and matches the established codebase convention - ✅ Branch coverage — the three new tests target previously untested
core.infocall paths: allow-all (noallowedExtensions), allow-all (empty array), and non-existent directory - ✅ Test naming — names read as specifications and clearly communicate intent
- ✅ All validation checks pass (format, lint, typecheck, 33/33 tests)
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 15.4 AIC · ⌖ 4.51 AIC · ⊞ 6.7K
Comment /matt to run again
There was a problem hiding this comment.
Clean, correct PR. No blocking issues.
Review summary
ESM/CJS module fix
Replacing bare require() with createRequire(import.meta.url)(...) on line 10 is correct — require is not available as a global in ESM contexts, and createRequire is the established pattern across this codebase (e.g. action_input_utils.test.cjs, allowed_issue_fields.test.cjs). This is the substantive fix.
New tests
The three added tests cover previously untested core.info call paths:
- Allow-all branch when
allowedExtensionsisundefined(line 43–46) - Allow-all branch when
allowedExtensionsis[](line 48–51) - Missing-directory branch with a non-empty extension list (line 60–64)
All three correctly match the source behavior in validate_memory_files.cjs. The vi.resetAllMocks() in beforeEach ensures mock isolation between tests.
Minor observation (non-blocking)
The new tests at lines 43–46 and 48–51 only assert on core.info and do not assert return values. Return value coverage is already provided by adjacent tests, so this is acceptable.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 · 27.8 AIC · ⌖ 6.16 AIC · ⊞ 5.7K
Comment /review to run again
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (3 tests)
Verdict
|
|
@copilot please run the This PR looks close to done:
|
|
Great work on the test improvements! 🧪 This PR looks ready for review: ✅ Module pattern fix — Updated the ESM/CJS require pattern to match the established codebase convention ( ✅ Test coverage — Added 3 targeted tests covering previously untested
✅ Validation — All formatting, linting, type checking, and tests pass (33/33). This PR is focused, well-documented, and ready to move forward.
|
Summary
Cleaned
validate_memory_files.cjs— fixed the test file module pattern and improved test coverage.Context
Execution context:
github-script(usescoreas a global from the github-script runtime)Changes
Test file:
validate_memory_files.test.cjsFixed ESM/CJS module loading pattern — replaced bare
require("./validate_memory_files.cjs")withcreateRequire(import.meta.url)("./validate_memory_files.cjs")to match the established pattern used across the codebase (e.g.issue_title_dedup.test.cjs). The previous pattern mixed ESMimportwith a bare CJSrequire()call, which is inconsistent.Improved test coverage for
core.infocalls — added 3 new tests:core.infois called with the correct message whenallowedExtensionsis not provided (allow-all branch)core.infois called with the correct message whenallowedExtensionsis an empty array (allow-all branch)core.infois called when the memory directory does not existThese tests verify the previously untested
core.infocall paths in the allow-all and missing-directory branches.Test Improvements
core.infocall pathsValidation
✅ All checks passed:
npm run format:cjs✓npm run lint:cjs✓npm run typecheck✓npm run test:js✓ (33/33 tests pass for this file)