Skip to content

[jsweep] Clean validate_secrets.cjs #38031

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 5 commits into from
Jun 9, 2026
Merged

[jsweep] Clean validate_secrets.cjs #38031

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b90f614
jsweep: clean validate_secrets.cjs
github-actions[bot] Jun 9, 2026
919b23c
Merge branch 'main' into signed/jsweep/validate-secrets-88f92a12ade07aa2
pelikhan Jun 9, 2026
6b8c167
address review: export Status+makePostRequest, add 3 makePostRequest …
Copilot Jun 9, 2026
ede6442
review nits: rename _timeoutCb → timeoutCallback, use optional chaini…
Copilot Jun 9, 2026
a9329fa
fix: run prettier on validate_secrets.test.cjs to fix lint-cjs CI fai…
Copilot Jun 9, 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
222 changes: 104 additions & 118 deletions actions/setup/js/validate_secrets.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
* - Integration tokens (Brave Search, Notion)
*/

const fs = require("fs");
const https = require("https");
const { promisify } = require("util");
const { exec } = require("child_process");
Expand All @@ -32,6 +33,7 @@ const Status = {

/**
* Secret documentation URLs
* @type {Record<string, string>}
*/
const SECRET_DOCS = {
GH_AW_GITHUB_TOKEN: "https://docs.github.com/en/authentication/keeping-your-account-and-data-secure/managing-your-personal-access-tokens",
Expand Down Expand Up @@ -84,6 +86,43 @@ function makeRequest(hostname, path, headers, method = "GET") {
});
}

/**
* Make an HTTPS POST request
* @param {string} hostname
* @param {string} path
* @param {Object} headers
* @param {string} body
* @returns {Promise<{statusCode: number, data: string}>}
*/
function makePostRequest(hostname, path, headers, body) {

@github-actions github-actions Bot Jun 9, 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] makePostRequest is now the shared critical path for both testGitHubGraphQLAPI and testAnthropicAPI, but it has no direct unit tests — only indirect coverage through higher-level mock tests.

💡 Suggested tests to add

Add a describe("makePostRequest", ...) block in validate_secrets.test.cjs covering the three key paths:

describe("makePostRequest", () => {
  it("resolves with statusCode and data on success", async () => {
    // stub https.request to emit data + end
  });

  it("rejects on request error", async () => {
    // simulate req.emit("error", new Error("oops"))
  });

  it("rejects with timeout error after 10 s", async () => {
    // simulate the setTimeout callback firing
  });
});

Without these, a regression in timeout wiring, Content-Length calculation, or response assembly would only surface at the higher-level integration layer.

[/improve-codebase-architecture] Also worth noting: makeRequest (GET) and makePostRequest (POST) share nearly identical structure — response streaming, error handling, timeout wiring. A single makeRequest(hostname, path, headers, { method, body } = {}) would reduce surface area and keep the patterns consistent.

Copilot AI Jun 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Done in 6b8c167. Added a describe("makePostRequest", ...) block in validate_secrets.test.cjs covering all three paths — success (resolves with statusCode + data), request error (rejects on req.emit("error", ...)), and timeout (fires the setTimeout callback, expects Request timeout and req.destroy() called). Used vi.spyOn(https, "request") with fake EventEmitter-backed request/response objects, restored via afterEach(() => vi.restoreAllMocks()).

return new Promise((resolve, reject) => {
const options = {
hostname,
path,
method: "POST",
headers: { ...headers, "Content-Length": Buffer.byteLength(body) },
};

const req = https.request(options, res => {
let data = "";
res.on("data", chunk => {
data += chunk;
});
res.on("end", () => {
resolve({ statusCode: res.statusCode || 0, data });
});
});

req.on("error", reject);
req.setTimeout(10000, () => {
req.destroy();
reject(new Error("Request timeout"));
});
req.write(body);
req.end();
});
}

/**
* Test GitHub token with REST API
* @param {string | undefined} token
Expand Down Expand Up @@ -158,39 +197,17 @@ async function testGitHubGraphQLAPI(token, owner, repo) {
`;

try {
const result = await new Promise((resolve, reject) => {
const postData = JSON.stringify({ query });
const graphqlUrl = new URL(process.env.GITHUB_GRAPHQL_URL || "https://api.github.com/graphql");
const options = {
hostname: graphqlUrl.hostname,
path: graphqlUrl.pathname,
method: "POST",
headers: {
"User-Agent": "gh-aw-secret-validation",
Authorization: `Bearer ${token}`,
"Content-Type": "application/json",
"Content-Length": Buffer.byteLength(postData),
},
};

const req = https.request(options, res => {
let data = "";
res.on("data", chunk => {
data += chunk;
});
res.on("end", () => {
resolve({ statusCode: res.statusCode || 0, data });
});
});

req.on("error", reject);
req.setTimeout(10000, () => {
req.destroy();
reject(new Error("Request timeout"));
});
req.write(postData);
req.end();
});
const graphqlUrl = new URL(process.env.GITHUB_GRAPHQL_URL || "https://api.github.com/graphql");
const result = await makePostRequest(
graphqlUrl.hostname,
graphqlUrl.pathname,
{
"User-Agent": "gh-aw-secret-validation",
Authorization: `Bearer ${token}`,
"Content-Type": "application/json",
},
JSON.stringify({ query })
);

if (result.statusCode === 200) {
const data = JSON.parse(result.data);
Expand Down Expand Up @@ -274,43 +291,20 @@ async function testAnthropicAPI(apiKey) {

try {
// Test with a minimal API call to check authentication
const result = await new Promise((resolve, reject) => {
const postData = JSON.stringify({
const result = await makePostRequest(
"api.anthropic.com",
"/v1/messages",
{
"Content-Type": "application/json",
"x-api-key": apiKey,
"anthropic-version": "2023-06-01",
},
JSON.stringify({
model: "claude-3-haiku-20240307",
max_tokens: 1,
messages: [{ role: "user", content: "test" }],
});

const options = {
hostname: "api.anthropic.com",
path: "/v1/messages",
method: "POST",
headers: {
"Content-Type": "application/json",
"x-api-key": apiKey,
"anthropic-version": "2023-06-01",
"Content-Length": Buffer.byteLength(postData),
},
};

const req = https.request(options, res => {
let data = "";
res.on("data", chunk => {
data += chunk;
});
res.on("end", () => {
resolve({ statusCode: res.statusCode || 0, data });
});
});

req.on("error", reject);
req.setTimeout(10000, () => {
req.destroy();
reject(new Error("Request timeout"));
});
req.write(postData);
req.end();
});
})
);

if (result.statusCode === 200) {
return {
Expand Down Expand Up @@ -506,9 +500,14 @@ function statusEmoji(status) {
}
}

/**
* @typedef {{ status: string, message: string, details?: any }} ApiTestResult
* @typedef {{ secret: string, test: string, status: string, message: string, details?: any }} TestResult
*/

/**
* Generate markdown diagnostic report
* @param {Array} results
* @param {TestResult[]} results
* @returns {string}
*/
function generateMarkdownReport(results) {
Expand Down Expand Up @@ -549,10 +548,7 @@ function generateMarkdownReport(results) {
report += `> [!WARNING]\n`;
report += `> **Failed Tests:** ${failedSecrets.length} secret(s) failed validation\n`;
report += `>\n`;
failedSecrets.forEach(secret => {
const docsLink = SECRET_DOCS[secret] || "https://docs.github.com";
report += `> - [\`${secret}\`](${docsLink})\n`;
});
report += failedSecrets.map(secret => `> - [\`${secret}\`](${SECRET_DOCS[secret] || "https://docs.github.com"})\n`).join("");
report += `>\n`;
report += `> Review the secret values and ensure they have proper permissions.\n\n`;
}
Expand All @@ -561,10 +557,7 @@ function generateMarkdownReport(results) {
report += `> [!NOTE]\n`;
report += `> **Not Configured:** ${notSetSecrets.length} secret(s) not set\n`;
report += `>\n`;
notSetSecrets.forEach(secret => {
const docsLink = SECRET_DOCS[secret] || "https://docs.github.com";
report += `> - [\`${secret}\`](${docsLink})\n`;
});
report += notSetSecrets.map(secret => `> - [\`${secret}\`](${SECRET_DOCS[secret] || "https://docs.github.com"})\n`).join("");
report += `>\n`;
report += `> Configure these secrets in [repository settings](${process.env.GITHUB_SERVER_URL || "https://github.com"}/${repository}/settings/secrets/actions) if needed.\n\n`;
}
Expand All @@ -574,47 +567,40 @@ function generateMarkdownReport(results) {
report += `## 🔍 Detailed Results\n\n`;

// Group by secret
const bySecret = {};
results.forEach(result => {
if (!bySecret[result.secret]) {
bySecret[result.secret] = [];
}
bySecret[result.secret].push(result);
});

Object.entries(bySecret).forEach(([secret, tests]) => {
// Determine overall status for the secret
const hasFailure = tests.some(t => t.status === Status.FAILURE);
const hasNotSet = tests.some(t => t.status === Status.NOT_SET);

let statusIcon = "✅";
if (hasFailure) statusIcon = "❌";
else if (hasNotSet) statusIcon = "⚪";
else if (tests.some(t => t.status === Status.SKIPPED)) statusIcon = "⏭️";

// Add documentation link for the secret
const docsLink = SECRET_DOCS[secret] || "https://docs.github.com";

// Use collapsible sections for each secret
report += `<details>\n`;
report += `<summary><strong>${statusIcon} <a href="${docsLink}">${secret}</a></strong> (${tests.length} test${tests.length > 1 ? "s" : ""})</summary>\n\n`;

tests.forEach(test => {
report += `### ${statusEmoji(test.status)} ${test.test}\n\n`;
report += `**Status:** ${test.status} | **Message:** ${test.message}\n\n`;

if (test.details) {
report += `<details>\n`;
report += `<summary>View details</summary>\n\n`;
report += `\`\`\`json\n`;
report += `${JSON.stringify(test.details, null, 2)}\n`;
report += `\`\`\`\n\n`;
report += `</details>\n\n`;
}
});

report += `</details>\n\n`;
});
const bySecret = results.reduce(

@github-actions github-actions Bot Jun 9, 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] generateMarkdownReport was substantially rewritten from imperative to functional style. The existing tests verify presence of key sections and emojis, which is a solid foundation — consider adding one snapshot test to lock in the exact rendered Markdown output.

💡 Snapshot test idea 
it("generateMarkdownReport matches snapshot", () => {
  const results = [
    { secret: "GH_AW_GITHUB_TOKEN", test: "REST API",   status: "failure", message: "401" },
    { secret: "ANTHROPIC_API_KEY",  test: "Anthropic",  status: "not_set", message: "not set" },
    { secret: "BRAVE_SEARCH_API_KEY", test: "Brave",    status: "success", message: "OK" },
  ];
  expect(generateMarkdownReport(results)).toMatchSnapshot();
});

This would catch any subtle whitespace, ordering, or structural differences between the old and new implementations, especially in the reduce grouping and map().join("") chains.

Copilot AI Jun 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Done in 6b8c167. Added generateMarkdownReport matches snapshot test using vi.useFakeTimers() / vi.setSystemTime(new Date("2024-01-15T12:00:00.000Z")) to freeze the generated timestamp, so the snapshot is stable across runs. Snapshot written to __snapshots__/validate_secrets.test.cjs.snap.

/** @param {Record<string, TestResult[]>} acc */
(acc, result) => {
(acc[result.secret] ??= []).push(result);
return acc;
},
{}
);

report += Object.entries(bySecret)
.map(([secret, tests]) => {
const hasFailure = tests.some(t => t.status === Status.FAILURE);
const hasNotSet = tests.some(t => t.status === Status.NOT_SET);

let statusIcon = "✅";
if (hasFailure) statusIcon = "❌";
else if (hasNotSet) statusIcon = "⚪";
else if (tests.some(t => t.status === Status.SKIPPED)) statusIcon = "⏭️";

const docsLink = SECRET_DOCS[secret] || "https://docs.github.com";
const testSections = tests
.map(test => {
let section = `### ${statusEmoji(test.status)} ${test.test}\n\n`;
section += `**Status:** ${test.status} | **Message:** ${test.message}\n\n`;
if (test.details) {
section += `<details>\n<summary>View details</summary>\n\n\`\`\`json\n${JSON.stringify(test.details, null, 2)}\n\`\`\`\n\n</details>\n\n`;
}
return section;
})
.join("");

return `<details>\n<summary><strong>${statusIcon} <a href="${docsLink}">${secret}</a></strong> (${tests.length} test${tests.length > 1 ? "s" : ""})</summary>\n\n${testSections}</details>\n\n`;
})
.join("");

return report;
}
Expand Down Expand Up @@ -736,7 +722,6 @@ async function main() {
const report = generateMarkdownReport(results);

// Write to file for artifact upload
const fs = require("fs");
fs.writeFileSync("secret-validation-report.md", report);
core.info("✅ Report written to secret-validation-report.md");

Expand Down Expand Up @@ -765,6 +750,7 @@ async function main() {
module.exports = {
main,
isForkRepository,
statusEmoji,
testGitHubRESTAPI,
testGitHubGraphQLAPI,
testCopilotCLI,
Expand Down
28 changes: 27 additions & 1 deletion actions/setup/js/validate_secrets.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// @ts-check

import { describe, it, expect } from "vitest";
import { testGitHubRESTAPI, testGitHubGraphQLAPI, testCopilotCLI, testAnthropicAPI, testOpenAIAPI, testBraveSearchAPI, testNotionAPI, generateMarkdownReport, isForkRepository } from "./validate_secrets.cjs";
import { testGitHubRESTAPI, testGitHubGraphQLAPI, testCopilotCLI, testAnthropicAPI, testOpenAIAPI, testBraveSearchAPI, testNotionAPI, generateMarkdownReport, isForkRepository, statusEmoji } from "./validate_secrets.cjs";

describe("validate_secrets", () => {
describe("testGitHubRESTAPI", () => {
Expand Down Expand Up @@ -241,4 +241,30 @@ describe("validate_secrets", () => {
expect(isForkRepository(undefined)).toBe(false);
});
});

describe("statusEmoji", () => {
it("should return ✅ for success", () => {

@github-actions github-actions Bot Jun 9, 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] The statusEmoji tests hardcode raw string literals ("success", "failure", etc.) instead of the Status enum constants. If a Status value is ever renamed, these tests would keep passing while statusEmoji silently breaks.

💡 Suggested fix: export Status and use it in tests

In validate_secrets.cjs, add Status to the exports:

module.exports = {
  ...,
  Status,
};

Then update the tests:

import { ..., statusEmoji, Status } from "./validate_secrets.cjs";

it("should return ✅ for success", () => {
  expect(statusEmoji(Status.SUCCESS)).toBe("✅");
});

This ensures tests stay in sync if the enum changes.

Copilot AI Jun 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Done in 6b8c167. Status is now exported from module.exports. The statusEmoji tests updated to use Status.SUCCESS, Status.FAILURE, Status.NOT_SET, and Status.SKIPPED — the unknown/empty-string cases remain raw strings as there are no corresponding enum values.

expect(statusEmoji("success")).toBe("✅");
});

it("should return ❌ for failure", () => {
expect(statusEmoji("failure")).toBe("❌");
});

it("should return ⚪ for not_set", () => {
expect(statusEmoji("not_set")).toBe("⚪");
});

it("should return ⏭️ for skipped", () => {
expect(statusEmoji("skipped")).toBe("⏭️");
});

it("should return ❓ for unknown status", () => {
expect(statusEmoji("unknown")).toBe("❓");
});

it("should return ❓ for empty string", () => {
expect(statusEmoji("")).toBe("❓");
});
});
});
Loading