Skip to content

Commit d79c1c4

Browse files
Copilotmnkiefer
andcommitted
Add environment variable validation to safe output jobs
- Create shared validateEnvironment utility function - Add validation to create_pull_request, create_issue, add_comment, create_discussion - Update all test files to set required GH_AW_WORKFLOW_NAME - All tests passing successfully Co-authored-by: mnkiefer <8320933+mnkiefer@users.noreply.github.com>
1 parent afbdbca commit d79c1c4

10 files changed

+226
-13
lines changed

pkg/workflow/js/add_comment.cjs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ const { generateFooterWithMessages } = require("./messages_footer.cjs");
66
const { getRepositoryUrl } = require("./get_repository_url.cjs");
77
const { replaceTemporaryIdReferences, loadTemporaryIdMap } = require("./temporary_id.cjs");
88
const { getTrackerID } = require("./get_tracker_id.cjs");
9+
const { validateEnvironment } = require("./validate_environment.cjs");
910

1011
/**
1112
* Hide/minimize a comment using the GraphQL API
@@ -262,6 +263,9 @@ async function commentOnDiscussion(github, owner, repo, discussionNumber, messag
262263
}
263264

264265
async function main() {
266+
// Environment validation - fail early if required variables are missing
267+
validateEnvironment(["GH_AW_WORKFLOW_NAME"]);
268+
265269
// Check if we're in staged mode
266270
const isStaged = process.env.GH_AW_SAFE_OUTPUTS_STAGED === "true";
267271
const isDiscussionExplicit = process.env.GITHUB_AW_COMMENT_DISCUSSION === "true";

pkg/workflow/js/add_comment.test.cjs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ const mockCore = {
4141
(fs.writeFileSync(tempFilePath, content), (process.env.GH_AW_AGENT_OUTPUT = tempFilePath));
4242
};
4343
(beforeEach(() => {
44-
(vi.clearAllMocks(), delete process.env.GH_AW_AGENT_OUTPUT, delete process.env.GITHUB_WORKFLOW, (global.context.eventName = "issues"), (global.context.payload.issue = { number: 123 }));
44+
(vi.clearAllMocks(), delete process.env.GH_AW_AGENT_OUTPUT, delete process.env.GITHUB_WORKFLOW, (global.context.eventName = "issues"), (global.context.payload.issue = { number: 123 }), (process.env.GH_AW_WORKFLOW_NAME = "Test Workflow"));
4545
const scriptPath = path.join(process.cwd(), "add_comment.cjs");
4646
createCommentScript = fs.readFileSync(scriptPath, "utf8");
4747
}),

pkg/workflow/js/create_discussion.cjs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ const { replaceTemporaryIdReferences, loadTemporaryIdMap } = require("./temporar
88
const { parseAllowedRepos, getDefaultTargetRepo, validateRepo, parseRepoSlug } = require("./repo_helpers.cjs");
99
const { addExpirationComment } = require("./expiration_helpers.cjs");
1010
const { removeDuplicateTitleFromDescription } = require("./remove_duplicate_title.cjs");
11+
const { validateEnvironment } = require("./validate_environment.cjs");
1112

1213
/**
1314
* Fetch repository ID and discussion categories for a repository
@@ -91,6 +92,9 @@ async function main() {
9192
core.setOutput("discussion_number", "");
9293
core.setOutput("discussion_url", "");
9394

95+
// Environment validation - fail early if required variables are missing
96+
validateEnvironment(["GH_AW_WORKFLOW_NAME"]);
97+
9498
// Load the temporary ID map from create_issue job
9599
const temporaryIdMap = loadTemporaryIdMap();
96100
if (temporaryIdMap.size > 0) {

pkg/workflow/js/create_discussion.test.cjs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,8 @@ const mockCore = {
4646
delete process.env.GH_AW_DISCUSSION_TITLE_PREFIX,
4747
delete process.env.GH_AW_DISCUSSION_CATEGORY,
4848
delete process.env.GH_AW_TARGET_REPO_SLUG,
49-
delete process.env.GH_AW_ALLOWED_REPOS);
49+
delete process.env.GH_AW_ALLOWED_REPOS,
50+
(process.env.GH_AW_WORKFLOW_NAME = "Test Workflow"));
5051
const scriptPath = path.join(process.cwd(), "create_discussion.cjs");
5152
((createDiscussionScript = fs.readFileSync(scriptPath, "utf8")), (createDiscussionScript = createDiscussionScript.replace("export {};", "")));
5253
}),

pkg/workflow/js/create_issue.cjs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ const { generateTemporaryId, isTemporaryId, normalizeTemporaryId, replaceTempora
1010
const { parseAllowedRepos, getDefaultTargetRepo, validateRepo, parseRepoSlug } = require("./repo_helpers.cjs");
1111
const { addExpirationComment } = require("./expiration_helpers.cjs");
1212
const { removeDuplicateTitleFromDescription } = require("./remove_duplicate_title.cjs");
13+
const { validateEnvironment } = require("./validate_environment.cjs");
1314

1415
async function main() {
1516
// Initialize outputs to empty strings to ensure they're always set
@@ -18,6 +19,9 @@ async function main() {
1819
core.setOutput("temporary_id_map", "{}");
1920
core.setOutput("issues_to_assign_copilot", "");
2021

22+
// Environment validation - fail early if required variables are missing
23+
validateEnvironment(["GH_AW_WORKFLOW_NAME"]);
24+
2125
const isStaged = process.env.GH_AW_SAFE_OUTPUTS_STAGED === "true";
2226

2327
const result = loadAgentOutput();

pkg/workflow/js/create_issue.test.cjs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,8 @@ const mockCore = {
4747
delete process.env.GH_AW_ISSUE_TITLE_PREFIX,
4848
delete process.env.GH_AW_TARGET_REPO_SLUG,
4949
delete process.env.GH_AW_ALLOWED_REPOS,
50-
delete global.context.payload.issue);
50+
delete global.context.payload.issue,
51+
(process.env.GH_AW_WORKFLOW_NAME = "Test Workflow"));
5152
const scriptPath = path.join(process.cwd(), "create_issue.cjs");
5253
((createIssueScript = fs.readFileSync(scriptPath, "utf8")),
5354
(createIssueScript = createIssueScript.replace("export {};", "")),

pkg/workflow/js/create_pull_request.cjs

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ const { updateActivationComment } = require("./update_activation_comment.cjs");
99
const { getTrackerID } = require("./get_tracker_id.cjs");
1010
const { addExpirationComment } = require("./expiration_helpers.cjs");
1111
const { removeDuplicateTitleFromDescription } = require("./remove_duplicate_title.cjs");
12+
const { validateEnvironment } = require("./validate_environment.cjs");
1213

1314
/**
1415
* Generate a patch preview with max 500 lines and 2000 chars for issue body
@@ -53,15 +54,10 @@ async function main() {
5354
const isStaged = process.env.GH_AW_SAFE_OUTPUTS_STAGED === "true";
5455

5556
// Environment validation - fail early if required variables are missing
56-
const workflowId = process.env.GH_AW_WORKFLOW_ID;
57-
if (!workflowId) {
58-
throw new Error("GH_AW_WORKFLOW_ID environment variable is required");
59-
}
57+
validateEnvironment(["GH_AW_WORKFLOW_ID", "GH_AW_BASE_BRANCH"]);
6058

59+
const workflowId = process.env.GH_AW_WORKFLOW_ID;
6160
const baseBranch = process.env.GH_AW_BASE_BRANCH;
62-
if (!baseBranch) {
63-
throw new Error("GH_AW_BASE_BRANCH environment variable is required");
64-
}
6561

6662
const agentOutputFile = process.env.GH_AW_AGENT_OUTPUT || "";
6763

pkg/workflow/js/create_pull_request.test.cjs

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,9 @@ const createTestableFunction = scriptContent => {
1212
(scriptBody = scriptBody.replace(/const \{ getTrackerID \} = require\("\.\/get_tracker_id\.cjs"\);?\s*/g, "")),
1313
(scriptBody = scriptBody.replace(/const \{ addExpirationComment \} = require\("\.\/expiration_helpers\.cjs"\);?\s*/g, "")),
1414
(scriptBody = scriptBody.replace(/const \{ removeDuplicateTitleFromDescription \} = require\("\.\/remove_duplicate_title\.cjs"\);?\s*/g, "")),
15+
(scriptBody = scriptBody.replace(/const \{ validateEnvironment \} = require\("\.\/validate_environment\.cjs"\);?\s*/g, "")),
1516
new Function(
16-
`\n const { fs, crypto, github, core, context, process, console, updateActivationComment, getTrackerID, addExpirationComment, removeDuplicateTitleFromDescription } = arguments[0];\n \n ${scriptBody}\n \n return main;\n `
17+
`\n const { fs, crypto, github, core, context, process, console, updateActivationComment, getTrackerID, addExpirationComment, removeDuplicateTitleFromDescription, validateEnvironment } = arguments[0];\n \n ${scriptBody}\n \n return main;\n `
1718
)
1819
);
1920
};
@@ -70,6 +71,18 @@ describe("create_pull_request.cjs", () => {
7071
getTrackerID: vi.fn(format => ""),
7172
addExpirationComment: vi.fn(),
7273
removeDuplicateTitleFromDescription: vi.fn((title, description) => description),
74+
validateEnvironment: vi.fn((required) => {
75+
// Implement actual validation logic for tests
76+
const missing = required.filter(varName => {
77+
const value = mockDependencies.process.env[varName];
78+
return value === undefined || value === null || value.trim() === "";
79+
});
80+
if (missing.length > 0) {
81+
throw new Error(
82+
`Missing required environment variable${missing.length > 1 ? "s" : ""}: ${missing.join(", ")}\n\nPlease ensure these are set in the safe_outputs job configuration.`
83+
);
84+
}
85+
}),
7386
}));
7487
}),
7588
afterEach(() => {
@@ -79,12 +92,12 @@ describe("create_pull_request.cjs", () => {
7992
}),
8093
it("should throw error when GH_AW_WORKFLOW_ID is missing", async () => {
8194
const mainFunction = createMainFunction(mockDependencies);
82-
await expect(mainFunction()).rejects.toThrow("GH_AW_WORKFLOW_ID environment variable is required");
95+
await expect(mainFunction()).rejects.toThrow("Missing required environment variables: GH_AW_WORKFLOW_ID, GH_AW_BASE_BRANCH");
8396
}),
8497
it("should throw error when GH_AW_BASE_BRANCH is missing", async () => {
8598
mockDependencies.process.env.GH_AW_WORKFLOW_ID = "test-workflow";
8699
const mainFunction = createMainFunction(mockDependencies);
87-
await expect(mainFunction()).rejects.toThrow("GH_AW_BASE_BRANCH environment variable is required");
100+
await expect(mainFunction()).rejects.toThrow("Missing required environment variable: GH_AW_BASE_BRANCH");
88101
}),
89102
it("should handle missing patch file with default warn behavior", async () => {
90103
((mockDependencies.process.env.GH_AW_WORKFLOW_ID = "test-workflow"), (mockDependencies.process.env.GH_AW_BASE_BRANCH = "main"), mockDependencies.fs.existsSync.mockReturnValue(!1));
Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
// @ts-check
2+
/// <reference types="@actions/github-script" />
3+
4+
/**
5+
* Validate Environment Variables Module
6+
*
7+
* This module provides utilities for validating required environment variables
8+
* in safe output job scripts. It ensures that necessary variables are present
9+
* before execution, providing clear error messages when variables are missing.
10+
*
11+
* Usage:
12+
* const { validateEnvironment } = require("./validate_environment.cjs");
13+
* validateEnvironment(['GH_AW_WORKFLOW_ID', 'GITHUB_TOKEN']);
14+
*/
15+
16+
/**
17+
* Validate that required environment variables are present
18+
*
19+
* Checks for the presence of required environment variables and throws a
20+
* descriptive error if any are missing. This provides clear feedback to users
21+
* about what configuration is needed.
22+
*
23+
* @param {string[]} required - Array of required environment variable names
24+
* @throws {Error} If any required environment variables are missing
25+
*
26+
* @example
27+
* // Check for a single required variable
28+
* validateEnvironment(['GH_AW_WORKFLOW_ID']);
29+
*
30+
* @example
31+
* // Check for multiple required variables
32+
* validateEnvironment(['GH_AW_WORKFLOW_ID', 'GH_AW_BASE_BRANCH']);
33+
*/
34+
function validateEnvironment(required) {
35+
if (!Array.isArray(required) || required.length === 0) {
36+
return;
37+
}
38+
39+
const missing = required.filter(varName => {
40+
const value = process.env[varName];
41+
return value === undefined || value === null || value.trim() === "";
42+
});
43+
44+
if (missing.length > 0) {
45+
const errorMessage = [
46+
`Missing required environment variable${missing.length > 1 ? "s" : ""}: ${missing.join(", ")}`,
47+
"",
48+
"Please ensure these are set in the safe_outputs job configuration.",
49+
].join("\n");
50+
51+
throw new Error(errorMessage);
52+
}
53+
}
54+
55+
module.exports = {
56+
validateEnvironment,
57+
};
Lines changed: 133 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,133 @@
1+
import { describe, it, expect, beforeEach, afterEach } from "vitest";
2+
3+
describe("validate_environment.cjs", () => {
4+
let validateEnvironment;
5+
let originalEnv;
6+
7+
beforeEach(async () => {
8+
// Save original environment
9+
originalEnv = { ...process.env };
10+
11+
// Import the module
12+
const module = await import("./validate_environment.cjs");
13+
validateEnvironment = module.validateEnvironment;
14+
});
15+
16+
afterEach(() => {
17+
// Restore original environment
18+
process.env = originalEnv;
19+
});
20+
21+
it("should not throw when all required variables are present", () => {
22+
process.env.GH_AW_WORKFLOW_ID = "test-workflow";
23+
process.env.GITHUB_TOKEN = "test-token";
24+
25+
expect(() => {
26+
validateEnvironment(["GH_AW_WORKFLOW_ID", "GITHUB_TOKEN"]);
27+
}).not.toThrow();
28+
});
29+
30+
it("should throw when a single required variable is missing", () => {
31+
delete process.env.GH_AW_WORKFLOW_ID;
32+
33+
expect(() => {
34+
validateEnvironment(["GH_AW_WORKFLOW_ID"]);
35+
}).toThrow(/Missing required environment variable: GH_AW_WORKFLOW_ID/);
36+
});
37+
38+
it("should throw when multiple required variables are missing", () => {
39+
delete process.env.GH_AW_WORKFLOW_ID;
40+
delete process.env.GITHUB_TOKEN;
41+
42+
expect(() => {
43+
validateEnvironment(["GH_AW_WORKFLOW_ID", "GITHUB_TOKEN"]);
44+
}).toThrow(/Missing required environment variables: GH_AW_WORKFLOW_ID, GITHUB_TOKEN/);
45+
});
46+
47+
it("should include helpful message about safe_outputs configuration", () => {
48+
delete process.env.GH_AW_WORKFLOW_ID;
49+
50+
expect(() => {
51+
validateEnvironment(["GH_AW_WORKFLOW_ID"]);
52+
}).toThrow(/Please ensure these are set in the safe_outputs job configuration/);
53+
});
54+
55+
it("should treat empty string as missing", () => {
56+
process.env.GH_AW_WORKFLOW_ID = "";
57+
58+
expect(() => {
59+
validateEnvironment(["GH_AW_WORKFLOW_ID"]);
60+
}).toThrow(/Missing required environment variable: GH_AW_WORKFLOW_ID/);
61+
});
62+
63+
it("should treat whitespace-only string as missing", () => {
64+
process.env.GH_AW_WORKFLOW_ID = " ";
65+
66+
expect(() => {
67+
validateEnvironment(["GH_AW_WORKFLOW_ID"]);
68+
}).toThrow(/Missing required environment variable: GH_AW_WORKFLOW_ID/);
69+
});
70+
71+
it("should not throw when required array is empty", () => {
72+
expect(() => {
73+
validateEnvironment([]);
74+
}).not.toThrow();
75+
});
76+
77+
it("should handle mix of present and missing variables", () => {
78+
process.env.GH_AW_WORKFLOW_ID = "test-workflow";
79+
delete process.env.GH_AW_BASE_BRANCH;
80+
81+
expect(() => {
82+
validateEnvironment(["GH_AW_WORKFLOW_ID", "GH_AW_BASE_BRANCH"]);
83+
}).toThrow(/Missing required environment variable: GH_AW_BASE_BRANCH/);
84+
});
85+
86+
it("should not include present variables in error message", () => {
87+
process.env.GH_AW_WORKFLOW_ID = "test-workflow";
88+
delete process.env.GH_AW_BASE_BRANCH;
89+
90+
try {
91+
validateEnvironment(["GH_AW_WORKFLOW_ID", "GH_AW_BASE_BRANCH"]);
92+
expect.fail("Should have thrown an error");
93+
} catch (error) {
94+
expect(error.message).not.toContain("GH_AW_WORKFLOW_ID");
95+
expect(error.message).toContain("GH_AW_BASE_BRANCH");
96+
}
97+
});
98+
99+
it("should handle validation with a single variable", () => {
100+
process.env.GITHUB_TOKEN = "test-token";
101+
102+
expect(() => {
103+
validateEnvironment(["GITHUB_TOKEN"]);
104+
}).not.toThrow();
105+
});
106+
107+
it("should handle validation with many variables", () => {
108+
process.env.VAR1 = "value1";
109+
process.env.VAR2 = "value2";
110+
process.env.VAR3 = "value3";
111+
process.env.VAR4 = "value4";
112+
process.env.VAR5 = "value5";
113+
114+
expect(() => {
115+
validateEnvironment(["VAR1", "VAR2", "VAR3", "VAR4", "VAR5"]);
116+
}).not.toThrow();
117+
});
118+
119+
it("should report all missing variables when multiple are absent", () => {
120+
delete process.env.VAR1;
121+
delete process.env.VAR2;
122+
delete process.env.VAR3;
123+
124+
try {
125+
validateEnvironment(["VAR1", "VAR2", "VAR3"]);
126+
expect.fail("Should have thrown an error");
127+
} catch (error) {
128+
expect(error.message).toContain("VAR1");
129+
expect(error.message).toContain("VAR2");
130+
expect(error.message).toContain("VAR3");
131+
}
132+
});
133+
});

0 commit comments

Comments
 (0)