[jsweep] Clean validate_lockdown_requirements_templates.cjs#40730
[jsweep] Clean validate_lockdown_requirements_templates.cjs#40730github-actions[bot] wants to merge 1 commit into
Conversation
…tes.cjs - Add @returns JSDoc annotations to the three exported render functions - Create validate_lockdown_requirements_templates.test.cjs with 29 tests covering all three render functions (lockdown token, strict mode, pull_request_target) and cross-function invariants 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. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #40730 does not have the 'implementation' label and has 0 new lines of code in business logic directories (≤100 threshold). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
This pull request cleans up the lockdown validation template renderer by improving JSDoc return-type annotations and adding a dedicated Vitest suite to validate the rendered output of each exported message renderer.
Changes:
- Added
@returns {string}JSDoc annotations to the three exported render functions invalidate_lockdown_requirements_templates.cjs. - Added a new Vitest test suite (
validate_lockdown_requirements_templates.test.cjs) with coverage for placeholder substitution, expected key phrases/URLs, idempotency, and cross-function distinctness.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/validate_lockdown_requirements_templates.cjs | Adds JSDoc @returns {string} annotations to exported template render helpers (no logic changes). |
| actions/setup/js/validate_lockdown_requirements_templates.test.cjs | New Vitest suite asserting correct rendering and absence of unreplaced {placeholder} tokens across all three message renderers. |
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 /tdd — approving with minor suggestions on test style and brittleness.
📋 Key Themes & Highlights
Suggestions
- Import formatting: The destructuring import on line 6 should be split across multiple lines for readability (4 comments).
typeofanti-pattern: Use vitest'stoBeTypeOf()and capture the result once per test to follow clear Arrange-Act-Assert structure.- Static-phrase brittleness: Tests that assert exact static template text (e.g.
"Lockdown mode is enabled") will break on valid rewording. Consider snapshot tests or scoping assertions to substituted tokens only. - Low-value idempotency tests: Three tests verify that a pure function returns the same value twice — this is guaranteed by the implementation, not a meaningful regression guard.
Positive Highlights
- ✅ The cross-function isolation tests ("lockdown message does not contain strict mode content" etc.) are excellent and genuinely regression-worthy.
- ✅ The catch-all regex sweep
/\{[a-z_]+\}/is a great belt-and-suspenders guard for unreplaced placeholders. - ✅ Placeholder substitution tests (
not.toContain("{auth_docs_url}")) are the right behavioral contract. - ✅
@returns {string}JSDoc additions are clean and correct; no logic was changed. - ✅ 29/29 tests passing with comprehensive describe organisation.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 70 AIC · ⌖ 7.49 AIC · ⊞ 6.4K
| import { createRequire } from "module"; | ||
|
|
||
| const req = createRequire(import.meta.url); | ||
| const { renderLockdownTokenErrorMessage, renderPublicStrictModeErrorMessage, renderPullRequestTargetErrorMessage } = req("./validate_lockdown_requirements_templates.cjs"); |
There was a problem hiding this comment.
[/tdd] Long single-line destructuring — split across lines for readability and diff clarity.
💡 Suggested formatting
const {
renderLockdownTokenErrorMessage,
renderPublicStrictModeErrorMessage,
renderPullRequestTargetErrorMessage,
} = req("./validate_lockdown_requirements_templates.cjs");The current line exceeds typical column limits and is harder to scan in diffs.
| describe("validate_lockdown_requirements_templates", () => { | ||
| describe("renderLockdownTokenErrorMessage", () => { | ||
| it("returns a non-empty string", () => { | ||
| expect(typeof renderLockdownTokenErrorMessage()).toBe("string"); |
There was a problem hiding this comment.
[/tdd] Two issues in one assertion: the function is invoked twice and typeof is used instead of vitest's idiomatic toBeTypeOf. Capture the result once to follow Arrange-Act-Assert clearly.
💡 Suggested refactor
it("returns a non-empty string", () => {
const message = renderLockdownTokenErrorMessage(); // arrange + act once
expect(message).toBeTypeOf("string");
expect(message.length).toBeGreaterThan(0);
});Same pattern applies to the equivalent it("returns a non-empty string", ...) blocks in the other two describe sections.
|
|
||
| it("mentions lockdown mode is enabled", () => { | ||
| const message = renderLockdownTokenErrorMessage(); | ||
| expect(message).toContain("Lockdown mode is enabled"); |
There was a problem hiding this comment.
[/tdd] This and similar tests ("includes GH_AW_GITHUB_TOKEN recommendation", "mentions public repository context", etc.) assert exact static phrases from the templates rather than substituted tokens. Any intentional reword (e.g. "Lockdown mode is enabled" → "Lockdown mode is active") will break these tests without indicating a real regression.
💡 Recommendation
The placeholder-substitution tests — expect(message).not.toContain("{auth_docs_url}") and the final regex sweep — are the real behavioral contract here. The static-phrase tests above those add documentation value but also brittleness.
Consider one of:
- Keep them as explicit content specs, but comment them as "content contracts" so future editors know a change here is intentional.
- Replace with a snapshot test (
expect(message).toMatchInlineSnapshot(...)) so rewrites show a clear diff. - Remove the static-phrase tests and rely on the placeholder tests + the regex sweep (
/\{[a-z_]+\}/) as the primary contract.
| }); | ||
|
|
||
| it("returns the same value on repeated calls", () => { | ||
| expect(renderLockdownTokenErrorMessage()).toBe(renderLockdownTokenErrorMessage()); |
There was a problem hiding this comment.
[/tdd] Idempotency tests for a pure, stateless function add minimal regression value — the function has no side effects and no mutable state, so same-call equality is guaranteed by design rather than tested behaviour. The test slot would be better used for a boundary/edge case (e.g. verifying that the exported module surface exposes exactly the three expected function names).
💡 Context
Per /tdd guidance, tests act as executable specifications that catch regressions. Since these wrappers are trivially deterministic, the idempotency assertion will never be the test that catches a real bug. All three idempotency blocks could be consolidated into a single module-surface check or simply removed.
🧪 Test Quality Sentinel Report
📊 Metrics & Test Classification (29 tests analyzed)
Files:
Go: 0 ( Score breakdown: Behavioral 40/40 · Edge cases 12/30 · Duplication −5 · Inflation −10 = 67/100 Edge-case note: these functions have no error-throwing paths — edge coverage comes from 12 negative assertions ( Verdict
|
There was a problem hiding this comment.
The JSDoc additions are clean and correct. The test suite has two non-blocking issues worth fixing before this pattern gets replicated.
🔍 Findings
ESM syntax in a .cjs file (line 5)
import statements and import.meta.url in a .cjs file work only via Vitest's build-time transform. Node.js rejects .cjs files containing import with a hard SyntaxError. The createRequire(import.meta.url) pattern exists specifically to call require() from within a real ESM file—it is redundant and misleading here since require() is already globally available in CJS. Fix: use require() directly, or rename to .test.mjs.
Trivially-passing placeholder test (line 47)
renderLockdownTokenErrorMessage's template uses only {auth_docs_url}, never {security_docs_url}. The assertion expect(message).not.toContain("{security_docs_url}") always passes regardless of whether rendering works, padding the test count with a check that cannot catch any regression.
🔎 Code quality review by PR Code Quality Reviewer · 80.5 AIC · ⌖ 7.64 AIC · ⊞ 5.1K
| import { describe, it, expect } from "vitest"; | ||
| import { createRequire } from "module"; | ||
|
|
||
| const req = createRequire(import.meta.url); |
There was a problem hiding this comment.
Module system contradiction: createRequire(import.meta.url) is an ESM idiom used in .mjs files to access require(); using it here—alongside top-level import statements—in a .cjs file means the file fails with SyntaxError: Cannot use import statement in a module when executed directly by Node.js. Tests pass only because Vitest transforms the file before execution; the .cjs extension actively promises semantics the file does not deliver.
💡 Suggested fix
If the file must stay .cjs, replace ESM imports with CommonJS equivalents—no createRequire needed since require() is already in scope:
// `@ts-check`
const { describe, it, expect } = require("vitest");
const {
renderLockdownTokenErrorMessage,
renderPublicStrictModeErrorMessage,
renderPullRequestTargetErrorMessage,
} = require("./validate_lockdown_requirements_templates.cjs");Alternatively, rename to .test.mjs and keep the ESM imports as-is. Either way, the file extension and the module syntax must agree.
|
|
||
| it("does not contain unreplaced {security_docs_url} placeholder", () => { | ||
| const message = renderLockdownTokenErrorMessage(); | ||
| expect(message).not.toContain("{security_docs_url}"); |
There was a problem hiding this comment.
Trivially-passing assertion: LOCKDOWN_TOKEN_ERROR_TEMPLATE never uses {security_docs_url}—only {auth_docs_url}. This assertion passes even if template expansion is completely broken, giving false confidence about substitution correctness.
💡 Suggested fix
Remove this test case. The meaningful guard—expect(message).not.toContain("{auth_docs_url}") at line 42—already covers the only placeholder in the lockdown template. A second placeholder-absence assertion for a token that was never present adds line count without adding signal.
| it("no message contains unreplaced placeholders", () => { | ||
| const messages = [renderLockdownTokenErrorMessage(), renderPublicStrictModeErrorMessage(), renderPullRequestTargetErrorMessage()]; | ||
| for (const message of messages) { | ||
| expect(message).not.toMatch(/\{[a-z_]+\}/); |
There was a problem hiding this comment.
Narrow placeholder guard: /\{[a-z_]+\}/ only matches lowercase snake_case tokens, so a future template placeholder using {camelCase} or {UPPER_CASE} naming would silently bypass this check if substitution fails.
💡 Suggested fix
Broaden the character class to cover all typical identifier conventions:
expect(message).not.toMatch(/\{[A-Za-z][A-Za-z0-9_]*\}/);All current placeholder names (auth_docs_url, security_docs_url, strict_compile_command) are already covered—this just closes the gap for future additions.
|
``
|
Cleans
validate_lockdown_requirements_templates.cjs— a Node.js helper module that renders lockdown-validation error messages from templates.Context
Execution context: Node.js (pure CJS helper, no GitHub Actions globals required)
Changes
validate_lockdown_requirements_templates.cjs@returns {string}JSDoc annotations to all three exported render functions to improve type documentation and IntelliSense support.@ts-checkand clean code structure.validate_lockdown_requirements_templates.test.cjs(new)Created a comprehensive test suite with 29 tests covering:
renderLockdownTokenErrorMessagerenderPublicStrictModeErrorMessagerenderPullRequestTargetErrorMessageTest coverage includes:
{auth_docs_url},{security_docs_url}){strict_compile_command}){placeholder}tokens{snake_case}placeholders remain in any rendered messageValidation
All checks passed:
npm run format:cjsnpm run lint:cjsnpm run typechecknpm run test:js— 29/29 tests passed for this fileWarning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
traces.example.comSee Network Configuration for more information.