Skip to content

Commit 568d558

Browse files
Copilotpelikhan
andauthored
fix: address reviewer comments on merge-base validation and tests
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/2b4ea663-bae9-4763-b9ed-30316e844c32 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
1 parent 392f35e commit 568d558

File tree

3 files changed

+32
-1
lines changed

3 files changed

+32
-1
lines changed

actions/setup/js/safe_output_type_validator.cjs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -445,7 +445,7 @@ function executeCustomValidation(item, customValidation, lineNum, itemType) {
445445
// Parse custom validation rule
446446
if (customValidation.startsWith("requiresOneOf:")) {
447447
const fields = customValidation.slice("requiresOneOf:".length).split(",");
448-
const hasValidField = fields.some(field => item[field] !== undefined);
448+
const hasValidField = fields.some(field => item[field] !== undefined && item[field] !== false);
449449
if (!hasValidField) {
450450
return {
451451
isValid: false,

actions/setup/js/safe_output_type_validator.test.cjs

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,16 @@ const SAMPLE_VALIDATION_CONFIG = {
4646
issue_number: { issueOrPRNumber: true },
4747
},
4848
},
49+
update_pull_request: {
50+
defaultMax: 1,
51+
customValidation: "requiresOneOf:title,body,merge_base",
52+
fields: {
53+
title: { type: "string", sanitize: true, maxLength: 256 },
54+
body: { type: "string", sanitize: true, maxLength: 65000 },
55+
merge_base: { type: "boolean" },
56+
pull_request_number: { issueOrPRNumber: true },
57+
},
58+
},
4959
assign_to_agent: {
5060
defaultMax: 1,
5161
customValidation: "requiresOneOf:issue_number,pull_number",
@@ -399,6 +409,23 @@ describe("safe_output_type_validator", () => {
399409
expect(result.error).toContain("issue_number");
400410
expect(result.error).toContain("pull_number");
401411
});
412+
413+
it("should fail for update_pull_request when merge_base is false and no title/body is provided", async () => {
414+
const { validateItem } = await import("./safe_output_type_validator.cjs");
415+
416+
const result = validateItem({ type: "update_pull_request", merge_base: false }, "update_pull_request", 1);
417+
418+
expect(result.isValid).toBe(false);
419+
expect(result.error).toContain("requires at least one of");
420+
});
421+
422+
it("should pass for update_pull_request when merge_base is true", async () => {
423+
const { validateItem } = await import("./safe_output_type_validator.cjs");
424+
425+
const result = validateItem({ type: "update_pull_request", merge_base: true }, "update_pull_request", 1);
426+
427+
expect(result.isValid).toBe(true);
428+
});
402429
});
403430

404431
describe("custom validation: startLineLessOrEqualLine", () => {

pkg/workflow/compiler_safe_outputs_config_test.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1236,9 +1236,11 @@ func TestUpdatePullRequestMergeBaseHandlerConfig(t *testing.T) {
12361236

12371237
var steps []string
12381238
compiler.addHandlerManagerConfigEnvVar(&steps, workflowData)
1239+
foundHandlerConfig := false
12391240

12401241
for _, step := range steps {
12411242
if strings.Contains(step, "GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG") {
1243+
foundHandlerConfig = true
12421244
parts := strings.Split(step, "GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG: ")
12431245
if len(parts) == 2 {
12441246
jsonStr := strings.TrimSpace(parts[1])
@@ -1258,6 +1260,8 @@ func TestUpdatePullRequestMergeBaseHandlerConfig(t *testing.T) {
12581260
}
12591261
}
12601262
}
1263+
1264+
require.True(t, foundHandlerConfig, "Expected GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG in generated steps")
12611265
})
12621266
}
12631267
}

0 commit comments

Comments
 (0)