Skip to content

[jsweep] Clean validate_lockdown_requirements_templates.cjs #40730

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 4 commits into from
Jun 22, 2026
+159 −0
Merged

[jsweep] Clean validate_lockdown_requirements_templates.cjs #40730

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b827104
jsweep: add JSDoc and tests for validate_lockdown_requirements_templa…
github-actions[bot] Jun 22, 2026
6f6b008
fix: address review feedback on validate_lockdown_requirements_templa…
Copilot Jun 22, 2026
d37b6a9
fix: use arrayContaining + toHaveLength for module-surface export check
Copilot Jun 22, 2026
5e5ccdd
fix: add content-contract comment to address static-phrase brittlenes…
Copilot Jun 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions actions/setup/js/validate_lockdown_requirements_templates.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,17 @@ const TEMPLATE_CONTEXT = {
strict_compile_command: "gh aw compile --strict",
};

/** @returns {string} Rendered lockdown token error message with documentation URL */
function renderLockdownTokenErrorMessage() {
return renderTemplate(LOCKDOWN_TOKEN_ERROR_TEMPLATE, TEMPLATE_CONTEXT);
}

/** @returns {string} Rendered public strict mode error message with compile command and documentation URL */
function renderPublicStrictModeErrorMessage() {
return renderTemplate(PUBLIC_STRICT_MODE_ERROR_TEMPLATE, TEMPLATE_CONTEXT);
}

/** @returns {string} Rendered pull_request_target security error message with documentation URL */
function renderPullRequestTargetErrorMessage() {
return renderTemplate(PULL_REQUEST_TARGET_ERROR_TEMPLATE, TEMPLATE_CONTEXT);
}
Expand Down
165 changes: 165 additions & 0 deletions actions/setup/js/validate_lockdown_requirements_templates.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,165 @@
// @ts-check
import { describe, it, expect } from "vitest";
import { createRequire } from "module";

const req = createRequire(import.meta.url);

@github-actions github-actions Bot Jun 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const { renderLockdownTokenErrorMessage, renderPublicStrictModeErrorMessage, renderPullRequestTargetErrorMessage } = req("./validate_lockdown_requirements_templates.cjs");

@github-actions github-actions Bot Jun 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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");

@github-actions github-actions Bot Jun 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

expect(renderLockdownTokenErrorMessage().length).toBeGreaterThan(0);
});

it("includes the auth documentation URL", () => {
const message = renderLockdownTokenErrorMessage();
expect(message).toContain("https://github.com/github/gh-aw/blob/main/docs/src/content/docs/reference/auth.mdx");
});

it("includes GH_AW_GITHUB_TOKEN recommendation", () => {
const message = renderLockdownTokenErrorMessage();
expect(message).toContain("GH_AW_GITHUB_TOKEN (recommended)");
});

it("includes GH_AW_GITHUB_MCP_SERVER_TOKEN as alternative", () => {
const message = renderLockdownTokenErrorMessage();
expect(message).toContain("GH_AW_GITHUB_MCP_SERVER_TOKEN (alternative)");
});

it("includes the gh aw secrets set command", () => {
const message = renderLockdownTokenErrorMessage();
expect(message).toContain("gh aw secrets set GH_AW_GITHUB_TOKEN");
});

it("mentions lockdown mode is enabled", () => {
const message = renderLockdownTokenErrorMessage();
expect(message).toContain("Lockdown mode is enabled");

@github-actions github-actions Bot Jun 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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:

  1. Keep them as explicit content specs, but comment them as "content contracts" so future editors know a change here is intentional.
  2. Replace with a snapshot test (expect(message).toMatchInlineSnapshot(...)) so rewrites show a clear diff.
  3. Remove the static-phrase tests and rely on the placeholder tests + the regex sweep (/\{[a-z_]+\}/) as the primary contract.

});

it("does not contain unreplaced {auth_docs_url} placeholder", () => {
const message = renderLockdownTokenErrorMessage();
expect(message).not.toContain("{auth_docs_url}");
});

it("does not contain unreplaced {security_docs_url} placeholder", () => {
const message = renderLockdownTokenErrorMessage();
expect(message).not.toContain("{security_docs_url}");

@github-actions github-actions Bot Jun 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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("returns the same value on repeated calls", () => {
expect(renderLockdownTokenErrorMessage()).toBe(renderLockdownTokenErrorMessage());

@github-actions github-actions Bot Jun 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

});
});

describe("renderPublicStrictModeErrorMessage", () => {
it("returns a non-empty string", () => {
expect(typeof renderPublicStrictModeErrorMessage()).toBe("string");
expect(renderPublicStrictModeErrorMessage().length).toBeGreaterThan(0);
});

it("includes the security documentation URL", () => {
const message = renderPublicStrictModeErrorMessage();
expect(message).toContain("https://github.com/github/gh-aw/blob/main/docs/src/content/docs/reference/security.mdx");
});

it("includes the strict compile command", () => {
const message = renderPublicStrictModeErrorMessage();
expect(message).toContain("gh aw compile --strict");
});

it("mentions public repository context", () => {
const message = renderPublicStrictModeErrorMessage();
expect(message).toContain("public repository");
});

it("mentions strict mode", () => {
const message = renderPublicStrictModeErrorMessage();
expect(message).toContain("strict mode");
});

it("does not contain unreplaced {strict_compile_command} placeholder", () => {
const message = renderPublicStrictModeErrorMessage();
expect(message).not.toContain("{strict_compile_command}");
});

it("does not contain unreplaced {security_docs_url} placeholder", () => {
const message = renderPublicStrictModeErrorMessage();
expect(message).not.toContain("{security_docs_url}");
});

it("returns the same value on repeated calls", () => {
expect(renderPublicStrictModeErrorMessage()).toBe(renderPublicStrictModeErrorMessage());
});
});

describe("renderPullRequestTargetErrorMessage", () => {
it("returns a non-empty string", () => {
expect(typeof renderPullRequestTargetErrorMessage()).toBe("string");
expect(renderPullRequestTargetErrorMessage().length).toBeGreaterThan(0);
});

it("includes the security documentation URL", () => {
const message = renderPullRequestTargetErrorMessage();
expect(message).toContain("https://github.com/github/gh-aw/blob/main/docs/src/content/docs/reference/security.mdx");
});

it("mentions the pull_request_target event", () => {
const message = renderPullRequestTargetErrorMessage();
expect(message).toContain("pull_request_target");
});

it("mentions pwn request security risk", () => {
const message = renderPullRequestTargetErrorMessage();
expect(message).toContain("pwn request");
});

it("mentions public repositories", () => {
const message = renderPullRequestTargetErrorMessage();
expect(message).toContain("public repositories");
});

it("suggests using the pull_request event instead", () => {
const message = renderPullRequestTargetErrorMessage();
expect(message).toContain("pull_request event instead");
});

it("does not contain unreplaced {security_docs_url} placeholder", () => {
const message = renderPullRequestTargetErrorMessage();
expect(message).not.toContain("{security_docs_url}");
});

it("returns the same value on repeated calls", () => {
expect(renderPullRequestTargetErrorMessage()).toBe(renderPullRequestTargetErrorMessage());
});
});

describe("cross-function checks", () => {
it("each render function returns a distinct message", () => {
const lockdown = renderLockdownTokenErrorMessage();
const strictMode = renderPublicStrictModeErrorMessage();
const prTarget = renderPullRequestTargetErrorMessage();

expect(lockdown).not.toBe(strictMode);
expect(lockdown).not.toBe(prTarget);
expect(strictMode).not.toBe(prTarget);
});

it("lockdown message does not contain strict mode content", () => {
const message = renderLockdownTokenErrorMessage();
expect(message).not.toContain("gh aw compile --strict");
});

it("strict mode message does not contain lockdown token content", () => {
const message = renderPublicStrictModeErrorMessage();
expect(message).not.toContain("GH_AW_GITHUB_TOKEN");
});

it("no message contains unreplaced placeholders", () => {
const messages = [renderLockdownTokenErrorMessage(), renderPublicStrictModeErrorMessage(), renderPullRequestTargetErrorMessage()];
for (const message of messages) {
expect(message).not.toMatch(/\{[a-z_]+\}/);

@github-actions github-actions Bot Jun 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
});
});
});
Loading