[jsweep] Clean validate_lockdown_requirements.cjs#39498
[jsweep] Clean validate_lockdown_requirements.cjs#39498github-actions[bot] wants to merge 4 commits into
Conversation
- Remove unused ERR_VALIDATION import - Extract failWithError helper to eliminate repeated setOutput+setFailed+throw pattern (DRY) - Use template literals with \n for multi-line error messages instead of string concatenation - Add 6 new tests: setOutput side-effects, multiple tokens, error message newline formatting Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #39498 does not have the 'implementation' label (has_implementation_label=false) and has 0 new lines of code in business logic directories (requires_adr_by_default_volume=false, default_business_additions=0, threshold=100). |
There was a problem hiding this comment.
Pull request overview
This PR performs a small cleanup/refactor of the actions/setup/js/validate_lockdown_requirements.cjs runtime validator (used from github-script), and strengthens its Vitest coverage to ensure behavior and error formatting remain correct.
Changes:
- Removed an unused
ERR_VALIDATIONimport fromvalidate_lockdown_requirements.cjs. - Extracted a
failWithError(message)helper to de-duplicate the repeated failure pattern (setOutput+setFailed+throw). - Added additional tests to validate
setOutputside effects, multi-token configurations, and newline formatting in error messages.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/validate_lockdown_requirements.cjs | Removes unused import, factors repeated failure logic into a helper, and switches error message construction to newline-containing strings. |
| actions/setup/js/validate_lockdown_requirements.test.cjs | Expands coverage to verify no success-path setOutput, multi-token success behavior, and newline presence in error messages. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /improve-codebase-architecture and /tdd — approving with two non-blocking suggestions.
📋 Key Themes & Highlights
Key Themes
- Template literal chaining: The
\\n→\nsemantic fix is real and correct, but the strings are still built by concatenating individual template literals. A single template literal (or a true multiline one) would complete the job. .toThrow()consistency: The three new formatting tests use bare.toThrow()while the existing suite already passes a message substring to.toThrow(...). Small gap worth closing.
Positive Highlights
- ✅
@returns {never}JSDoc onfailWithErroris precise — TypeScript will use this for control-flow narrowing. - ✅
failWithErrorDRY extraction eliminates three identical three-liner blocks cleanly. - ✅ The
\\n→\nmigration is a genuine semantic bug fix (old code emitted literal backslash-n text), and the newerror message formattingdescribe block provides direct proof. - ✅ Success-path
setOutputside-effect tests (expect(mockCore.setOutput).not.toHaveBeenCalled()) are excellent regression guards. - ✅ Dead
ERR_VALIDATIONimport removed, eliminating an unnecessary coupling toerror_codes.cjs.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 217.6 AIC · ⌖ 14 AIC · ⊞ 29.4K
|
|
||
| expect(() => { | ||
| validateLockdownRequirements(mockCore); | ||
| }).toThrow(); |
There was a problem hiding this comment.
[/tdd] .toThrow() without an argument does not verify which error is thrown — inconsistent with the existing suite (line 301 uses .toThrow("not compiled with strict mode")).
💡 Match the specificity of existing tests
The pre-existing test block uses:
}).toThrow("not compiled with strict mode");Applying the same pattern here makes all three new formatting tests consistent and catches regressions in the thrown message, not just the fact that something was thrown:
// lockdown test
}).toThrow("no custom GitHub token is configured");
// strict mode test
}).toThrow("not compiled with strict mode");
// pull_request_target test
}).toThrow("pwn request");
There was a problem hiding this comment.
Review
Clean, correct refactor. No blocking issues.
What was examined
failWithError extraction — Correct DRY helper. core.setFailed does not throw in the Actions runtime, so the subsequent throw new Error(message) always executes. The @returns {never} JSDoc annotation is valid and aids TypeScript control-flow analysis under // @ts-check``.
\\n → \n behavioral fix — Old code used "text\\\\n" in regular string literals, producing literal backslash-n in the string value. New code uses `text\n` in template literals, producing real newline characters. @actions/core properly URL-encodes \n → %0A in ::error:: annotations. This is a genuine bug fix that improves log readability.
New tests — The three added describe blocks (setOutput side effects, multiple tokens, error message formatting) validate concrete behaviors: no false-positive output on success paths, correct || token logic, and the \n fix round-trip. Scope is appropriate for unit tests.
🔎 Code quality review by PR Code Quality Reviewer · 340.5 AIC · ⌖ 13.4 AIC · ⊞ 17.3K
| core.setFailed(errorMessage); | ||
| throw new Error(errorMessage); | ||
| failWithError( | ||
| `Lockdown mode is enabled (lockdown: true) but no custom GitHub token is configured.\n` + |
There was a problem hiding this comment.
@copilot refactor to template file and renderTemplate
There was a problem hiding this comment.
Addressed in commit 6c9ce2b. I moved the lockdown/strict/pull_request_target error text into a dedicated template module and render it via renderTemplate.
| core.setFailed(errorMessage); | ||
| throw new Error(errorMessage); | ||
| failWithError( | ||
| `This workflow is running on a public repository but was not compiled with strict mode.\n` + |
There was a problem hiding this comment.
Addressed in commit 6c9ce2b. The strict-mode public-repo failure message now comes from the new template module and is rendered through renderTemplate.
| core.setFailed(errorMessage); | ||
| throw new Error(errorMessage); | ||
| failWithError( | ||
| `This workflow is triggered by the pull_request_target event on a public repository.\n` + |
There was a problem hiding this comment.
Addressed in commit 6c9ce2b. The pull_request_target public-repo failure message was moved to the template module and rendered through renderTemplate.
🧪 Test Quality Sentinel Report
📊 Metrics & Test Classification (6 tests analyzed)
Go: 0; JavaScript: 6 (
|
There was a problem hiding this comment.
❌ Test Quality Sentinel: 60/100. 50% of new tests are classified as low-value implementation tests, exceeding the 30% threshold. The three error message formatting tests assert on \n presence and exact message wording (internal formatting) rather than behavioral contracts. Please review the flagged tests in the comment above and remove the newline assertions — the content assertions are valuable and should be kept.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Summary
Cleans
validate_lockdown_requirements.cjsas part of the daily jsweep unbloat run.Context type: github-script (uses
core.info,core.setFailed,core.setOutput)Changes
🗑️ Remove unused import
const { ERR_VALIDATION } = require("./error_codes.cjs")—ERR_VALIDATIONwas imported but never referenced in the file.♻️ Extract
failWithErrorhelper (DRY)The pattern
core.setOutput("lockdown_check_failed", "true"); core.setFailed(msg); throw new Error(msg)was repeated verbatim three times. Extracted into a localfailWithError(message)helper with a@returns {never}JSDoc annotation so TypeScript understands it never returns normally.🧩 Refactor error text to template module +
renderTemplatevalidate_lockdown_requirements.cjsinto a dedicated template module:actions/setup/js/validate_lockdown_requirements_templates.cjsrenderTemplatefrommessages_core.cjs.Test improvements
Added 6 new tests (24 → 30 total) across three new describe blocks:
setOutput side effectslockdown_check_failedoutput is NOT set on successful runsmultiple tokenserror message formatting\nnewline characters✅ Validation
npm run format:cjs✓npm run lint:cjs✓npm run typecheck✓npm run test:js) ✓> [!WARNING]
>
> Generated by 🧹 jsweep - JavaScript Unbloater · 710.2 AIC · ⌖ 41.8 AIC · ⊞ 18.7K · ◷
> - [x] expires on Jun 17, 2026, 9:46 PM UTC-08:00