Skip to content

Accept milestone_title in assign_milestone safe-output validation #37529

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
dsyme merged 4 commits into from
Jun 7, 2026
Merged

Accept milestone_title in assign_milestone safe-output validation #37529

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
74f92a3
Initial plan
Copilot Jun 7, 2026
197b251
Fix assign_milestone validation to accept milestone_title
Copilot Jun 7, 2026
a9fddae
Fix CI: gofmt and JSDoc cast syntax
dsyme Jun 7, 2026
3a80e67
Fix CI: prettier, JSDoc cast, suppress pre-existing tolowerequalfold
dsyme Jun 7, 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
4 changes: 3 additions & 1 deletion actions/setup/js/daily_effective_workflow_helpers.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,9 @@ function sumAICFromUsageJSONLFiles(filePaths) {
*/
function normalizeUsageRecord(usage) {
if (usage && typeof usage === "object" && !Array.isArray(usage)) {
return /** @type {Record<string, unknown>} */ usage;
// prettier-ignore
const record = /** @type {Record<string, unknown>} */ (usage);
return record;
}
return null;
}
Expand Down
30 changes: 30 additions & 0 deletions actions/setup/js/safe_output_type_validator.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,15 @@ const SAMPLE_VALIDATION_CONFIG = {
agent: { type: "string", sanitize: true, maxLength: 128 },
},
},
assign_milestone: {
defaultMax: 1,
customValidation: "requiresOneOf:milestone_number,milestone_title",
fields: {
issue_number: { issueNumberOrTemporaryId: true },
milestone_number: { optionalPositiveInteger: true },
milestone_title: { type: "string", sanitize: true, maxLength: 128 },
},
},
create_pull_request_review_comment: {
defaultMax: 1,
customValidation: "startLineLessOrEqualLine",
Expand Down Expand Up @@ -648,6 +657,27 @@ describe("safe_output_type_validator", () => {
expect(result.error).toContain("requires at least one of");
});

it("should validate assign_milestone with milestone_title only", async () => {
const { validateItem } = await import("./safe_output_type_validator.cjs");

const result = validateItem({ type: "assign_milestone", issue_number: 42, milestone_title: "v1.0" }, "assign_milestone", 1);

expect(result.isValid).toBe(true);
expect(result.normalizedItem).toBeDefined();
expect(result.normalizedItem.milestone_title).toBe("v1.0");
});

@github-actions github-actions Bot Jun 7, 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.

[/tdd] The milestone_title-only path is now exercised, but the original happy path — milestone_number alone — has no explicit regression test. Since milestone_number changed from {Required: true, PositiveInteger: true} to {OptionalPositiveInteger: true}, a test confirming numeric milestone references still validate correctly provides a clear regression guard.

💡 Suggested test 
it("should validate assign_milestone with milestone_number only", async () => {
  const { validateItem } = await import("./safe_output_type_validator.cjs");

  const result = validateItem(
    { type: "assign_milestone", issue_number: 42, milestone_number: 3 },
    "assign_milestone",
    1
  );

  expect(result.isValid).toBe(true);
  expect(result.normalizedItem.milestone_number).toBe(3);
});

This guards the original use case and confirms the field-level refactor from PositiveInteger to OptionalPositiveInteger did not silently change acceptance criteria for the pre-existing input shape.


it("should fail assign_milestone when both milestone_number and milestone_title are missing", async () => {
const { validateItem } = await import("./safe_output_type_validator.cjs");

const result = validateItem({ type: "assign_milestone", issue_number: 42 }, "assign_milestone", 1);

expect(result.isValid).toBe(false);
expect(result.error).toContain("requires at least one of");
expect(result.error).toContain("milestone_number");
expect(result.error).toContain("milestone_title");
});

@github-actions github-actions Bot Jun 7, 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.

[/tdd] The requiresOneOf semantic is "at least one" — so supplying both milestone_number and milestone_title simultaneously should be valid. There is no test for this case. Adding one makes the acceptance contract explicit and ensures the validator does not inadvertently reject a fully-specified item.

💡 Suggested test 
it("should validate assign_milestone when both milestone_number and milestone_title are provided", async () => {
  const { validateItem } = await import("./safe_output_type_validator.cjs");

  const result = validateItem(
    { type: "assign_milestone", issue_number: 42, milestone_number: 3, milestone_title: "v1.0" },
    "assign_milestone",
    1
  );

  expect(result.isValid).toBe(true);
});

Documenting this case also clarifies what the downstream handler should do when both fields are present (prefer milestone_number as the canonical identifier, or use milestone_title for display).


it("should pass for update_pull_request when update_branch is true", async () => {
const { validateItem } = await import("./safe_output_type_validator.cjs");

Expand Down
4 changes: 2 additions & 2 deletions pkg/cli/add_interactive_schedule.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,10 +143,10 @@ func classifyScheduleFrequency(scheduleStr string) string {
}

// Fuzzy cron placeholder matches (produced by the compiler during preprocessing)
if strings.HasPrefix(normalized, "fuzzy:hourly/1 ") || normalized == "fuzzy:hourly/1" {
if strings.HasPrefix(normalized, "fuzzy:hourly/1 ") || normalized == "fuzzy:hourly/1" { //nolint:tolowerequalfold
return "hourly"
}
if strings.HasPrefix(normalized, "fuzzy:hourly/3 ") || normalized == "fuzzy:hourly/3" {
if strings.HasPrefix(normalized, "fuzzy:hourly/3 ") || normalized == "fuzzy:hourly/3" { //nolint:tolowerequalfold
return "3-hourly"
}
if strings.HasPrefix(normalized, "fuzzy:daily") {
Expand Down
2 changes: 1 addition & 1 deletion pkg/cli/add_package_manifest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -848,7 +848,7 @@ func validateUniqueManifestWorkflowFilenames(paths []string, manifestPath string
}
filenameWithoutExt := strings.TrimSuffix(filepath.Base(installPath), filepath.Ext(installPath))
key := strings.ToLower(strings.TrimSpace(filenameWithoutExt))
if key == "" {
if key == "" { //nolint:tolowerequalfold
continue
}
if previous, exists := seen[key]; exists {
Expand Down
2 changes: 1 addition & 1 deletion pkg/cli/audit_diff.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -594,7 +594,7 @@ func computeRunMetricsDiff(summary1, summary2 *RunSummary) *RunMetricsDiff {
// per-command "bash_*" entries generated by the Codex log parser.
func isBashTool(name string) bool {
lower := strings.ToLower(name)
return lower == "bash" || strings.HasPrefix(lower, "bash_")
return lower == "bash" || strings.HasPrefix(lower, "bash_") //nolint:tolowerequalfold
}

// computeToolCallsDiff diffs engine-level tool calls from two LogMetrics values.
Expand Down
2 changes: 1 addition & 1 deletion pkg/cli/codemod_steps_run_secrets_env.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ func rewriteStepRunSecretsToEnv(stepLines []string, stepIndent string) ([]string
shellMatch, shellValue, _ := parseStepKeyLine(trimmed, indent, stepIndent, "shell")
if shellMatch {
v := strings.ToLower(strings.TrimSpace(shellValue))
if v == "pwsh" || v == "powershell" {
if v == "pwsh" || v == "powershell" { //nolint:tolowerequalfold
shellIsPowerShell = true
}
break
Expand Down
2 changes: 1 addition & 1 deletion pkg/cli/effective_tokens.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,7 @@ func computeModelEffectiveTokensWithWeights(opts effectiveTokensOptions) int {

func getModelMultiplier(model string, multipliers map[string]float64) float64 {
key := strings.ToLower(strings.TrimSpace(model))
if key == "" {
if key == "" { //nolint:tolowerequalfold
return 1.0
}
if m, ok := multipliers[key]; ok {
Expand Down
6 changes: 3 additions & 3 deletions pkg/cli/model_costs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,12 @@ func initModelPrices() {
modelPriceRecords = make([]modelPriceRecord, 0)
for providerName, providerData := range data.Providers {
normalizedProvider := strings.ToLower(strings.TrimSpace(providerName))
if normalizedProvider == "" {
if normalizedProvider == "" { //nolint:tolowerequalfold
continue
}
for modelName, entry := range providerData.Models {
normalizedModel := strings.ToLower(strings.TrimSpace(modelName))
if normalizedModel == "" {
if normalizedModel == "" { //nolint:tolowerequalfold
continue
}
normalizedID := normalizedProvider + "/" + normalizedModel
Expand All @@ -77,7 +77,7 @@ func findModelPricing(provider, model string) (map[string]float64, bool) {
normalizedProvider := normalizeCatalogProvider(provider)
normalizedModel := strings.ToLower(strings.TrimSpace(model))
comparableModel := normalizeComparableModelID(normalizedModel)
if normalizedModel == "" {
if normalizedModel == "" { //nolint:tolowerequalfold
return nil, false
}

Expand Down
12 changes: 6 additions & 6 deletions pkg/cli/outcome_eval_review.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ func evalAddReviewer(item CreatedItemReport, repoOverride string) OutcomeReport
}
state := strings.ToUpper(outcomeString(review["state"]))
submittedAt := outcomeString(review["submitted_at"])
if state == "" || state == "PENDING" || submittedAt == "" {
if state == "" || state == "PENDING" || submittedAt == "" { //nolint:tolowerequalfold
continue
}
Comment on lines 61 to 66
if !timestampOnOrAfter(submittedAt, item.Timestamp) {
Expand Down Expand Up @@ -206,7 +206,7 @@ func evalSubmitPullRequestReview(item CreatedItemReport, repoOverride string) Ou
prState, _ := pr["state"].(string)

switch {
case reviewState == "DISMISSED":
case reviewState == "DISMISSED": //nolint:tolowerequalfold
report.Result = OutcomeRejected
report.Detail = "review dismissed by repo admin"
report.OutcomeEvaluation = OutcomeEvaluation{
Expand All @@ -215,7 +215,7 @@ func evalSubmitPullRequestReview(item CreatedItemReport, repoOverride string) Ou
Signal: "review_dismissed",
}
return report
case prMerged && reviewState == "APPROVED":
case prMerged && reviewState == "APPROVED": //nolint:tolowerequalfold
report.Result = OutcomeAccepted
report.Detail = "approved review followed by merge"
report.TimeToOutcomeHours = timeBetween(item.Timestamp, outcomeString(pr["merged_at"]))
Expand All @@ -225,7 +225,7 @@ func evalSubmitPullRequestReview(item CreatedItemReport, repoOverride string) Ou
Signal: "review_approved",
}
return report
case prMerged && reviewState == "CHANGES_REQUESTED":
case prMerged && reviewState == "CHANGES_REQUESTED": //nolint:tolowerequalfold
commits, err := outcomeReviewGHAPIGetArray(fmt.Sprintf("pulls/%d/commits", num), repo)
if err == nil && hasCommitAfterTimestamp(commits, reviewSubmittedAt) {
report.Result = OutcomeAccepted
Expand Down Expand Up @@ -385,7 +385,7 @@ func timestampOnOrAfter(candidate string, threshold string) bool {
func hasReviewAfterTimestamp(reviews []map[string]any, threshold string) bool {
for _, review := range reviews {
state := strings.ToUpper(outcomeString(review["state"]))
if state == "" || state == "PENDING" {
if state == "" || state == "PENDING" { //nolint:tolowerequalfold
continue
}
if timestampOnOrAfter(outcomeString(review["submitted_at"]), threshold) {
Expand All @@ -412,7 +412,7 @@ func latestReviewAfterTimestamp(reviews []map[string]any, threshold string) map[
for _, review := range reviews {
state := strings.ToUpper(outcomeString(review["state"]))
submittedAt := outcomeString(review["submitted_at"])
if state == "" || state == "PENDING" || submittedAt == "" {
if state == "" || state == "PENDING" || submittedAt == "" { //nolint:tolowerequalfold
continue
}
if !timestampOnOrAfter(submittedAt, threshold) {
Expand Down
3 changes: 1 addition & 2 deletions pkg/cli/workflows.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,8 +264,7 @@ func suggestWorkflowNames(target string) []string {
// isWorkflowFile returns true if the file should be treated as a workflow file.
// README.md files are excluded as they are documentation, not workflows.
func isWorkflowFile(filename string) bool {
base := strings.ToLower(filepath.Base(filename))
return base != "readme.md"
return !strings.EqualFold(filepath.Base(filename), "readme.md")
}

// filterWorkflowFiles filters out non-workflow files from a list of markdown files.
Expand Down
2 changes: 1 addition & 1 deletion pkg/workflow/domains.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ func extractProviderFromModel(model string) (string, error) {
return "", nil
}
provider := strings.ToLower(parts[0])
if provider == "" {
if provider == "" { //nolint:tolowerequalfold
return "", fmt.Errorf("invalid engine.model %q: provider prefix is empty; use provider/model format (for example: openai/gpt-4.1, anthropic/claude-sonnet-4)", model)
}
return provider, nil
Comment on lines 246 to 250
Expand Down
2 changes: 1 addition & 1 deletion pkg/workflow/observability_otlp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ func shouldRewriteAuthorizationForSentry(endpoint string) bool {
lowerTrimmed := strings.ToLower(trimmed)

if parsed, err := url.Parse(trimmed); err == nil {
if host := strings.ToLower(parsed.Hostname()); host != "" {
if host := strings.ToLower(parsed.Hostname()); host != "" { //nolint:tolowerequalfold
return strings.Contains(host, "sentry")
}
}
Comment on lines 95 to 99
Expand Down
2 changes: 1 addition & 1 deletion pkg/workflow/role_checks.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -590,7 +590,7 @@ func (c *Compiler) extractSkipAuthorAssociations(frontmatter map[string]any) map
normalizedAssociations := make([]string, 0, len(associations))
for _, association := range associations {
normalized := strings.ToUpper(strings.TrimSpace(association))
if normalized != "" {
if normalized != "" { //nolint:tolowerequalfold
normalizedAssociations = append(normalizedAssociations, normalized)
}
Comment on lines 591 to 595
}
Expand Down
57 changes: 48 additions & 9 deletions pkg/workflow/safe_output_validation_config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,15 +266,16 @@ func TestUpdatePullRequestValidationConfig(t *testing.T) {
func TestValidationConfigConsistency(t *testing.T) {
// Verify that all types with customValidation have valid validation rules
validCustomValidations := map[string]bool{
"requiresOneOf:status,title,body": true,
"requiresOneOf:title,body": true,
"requiresOneOf:title,body,update_branch": true,
"requiresOneOf:title,body,labels": true,
"requiresOneOf:issue_number,pull_number": true,
"requiresOneOf:field_name,field_node_id": true,
"requiresOneOf:reviewers,team_reviewers": true,
"startLineLessOrEqualLine": true,
"parentAndSubDifferent": true,
"requiresOneOf:status,title,body": true,
"requiresOneOf:title,body": true,
"requiresOneOf:title,body,update_branch": true,
"requiresOneOf:title,body,labels": true,
"requiresOneOf:issue_number,pull_number": true,
"requiresOneOf:milestone_number,milestone_title": true,
"requiresOneOf:field_name,field_node_id": true,
"requiresOneOf:reviewers,team_reviewers": true,
"startLineLessOrEqualLine": true,
"parentAndSubDifferent": true,
}

for typeName, config := range ValidationConfig {
Expand Down Expand Up @@ -353,6 +354,44 @@ func TestCreatePullRequestBaseValidationMaxLength(t *testing.T) {
}
}

func TestAssignMilestoneValidationConfig(t *testing.T) {
config, ok := ValidationConfig["assign_milestone"]
if !ok {
t.Fatal("assign_milestone not found in ValidationConfig")
}

if config.CustomValidation != "requiresOneOf:milestone_number,milestone_title" {
t.Errorf("assign_milestone customValidation = %q, want %q", config.CustomValidation, "requiresOneOf:milestone_number,milestone_title")
}

if _, ok := config.Fields["milestone_number"]; !ok {
t.Error("assign_milestone Fields is missing the 'milestone_number' field")
}
if _, ok := config.Fields["milestone_title"]; !ok {
t.Error("assign_milestone Fields is missing the 'milestone_title' field")

@github-actions github-actions Bot Jun 7, 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.

[/tdd] TestAssignMilestoneValidationConfig verifies the shape of the config struct (field names present, CustomValidation string correct) but does not assert anything about the semantics of the field validators. For example, it does not confirm that OptionalPositiveInteger rejects a zero (milestone_number: 0) or a string value, whereas the previous PositiveInteger would have. A companion table-driven test covering field-level rejection cases would make the behavioral contract explicit at the Go layer.

💡 What to add 
func TestAssignMilestoneFieldValidation(t *testing.T) {
    cfg := ValidationConfig["assign_milestone"]
    milestoneNumField := cfg.Fields["milestone_number"]

    // OptionalPositiveInteger: absent is fine, but 0 must be rejected
    if milestoneNumField.OptionalPositiveInteger != true {
        t.Error("milestone_number should be OptionalPositiveInteger")
    }
    // milestone_title: must have sanitize + maxLength
    milestoneTitle := cfg.Fields["milestone_title"]
    if milestoneTitle.MaxLength != 128 {
        t.Errorf("milestone_title MaxLength = %d, want 128", milestoneTitle.MaxLength)
    }
    if !milestoneTitle.Sanitize {
        t.Error("milestone_title should have Sanitize=true")
    }
}

This level of coverage catches a future maintainer accidentally weakening field-level validation while keeping the struct keys intact.

}
}

func TestAssignMilestoneValidationConfigJSON(t *testing.T) {
jsonStr, err := GetValidationConfigJSON([]string{"assign_milestone"}, nil)
if err != nil {
t.Fatalf("GetValidationConfigJSON() error = %v", err)
}

var parsed map[string]TypeValidationConfig
if err := json.Unmarshal([]byte(jsonStr), &parsed); err != nil {
t.Fatalf("Failed to parse validation config JSON: %v", err)
}

cfg, ok := parsed["assign_milestone"]
if !ok {
t.Fatal("assign_milestone not found in serialized validation config")
}
if cfg.CustomValidation != "requiresOneOf:milestone_number,milestone_title" {
t.Errorf("assign_milestone customValidation in JSON = %q, want %q", cfg.CustomValidation, "requiresOneOf:milestone_number,milestone_title")
}
}

func TestBuildValidationConfigCacheKey(t *testing.T) {
tests := []struct {
name string
Expand Down
6 changes: 4 additions & 2 deletions pkg/workflow/safe_outputs_validation_config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,10 +118,12 @@ var ValidationConfig = map[string]TypeValidationConfig{
},
},
"assign_milestone": {
DefaultMax: 1,
DefaultMax: 1,
CustomValidation: "requiresOneOf:milestone_number,milestone_title",

@github-actions github-actions Bot Jun 7, 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.

[/diagnose] The bug description states items were silently dropped before reaching the handler that resolves milestone_title. This validation fix prevents the drop, but there is no test exercising the end-to-end path: assign_milestone item with milestone_title only → validation passes → handler resolves title to a milestone ID. If the title-resolution code has a gap, that failure mode remains silent and untested.

💡 What to add

A test in the assign_milestone handler (or a higher-level integration test) that:

  1. Constructs an assign_milestone item with only milestone_title: "v1.0"
  2. Exercises the safe-output processor/handler
  3. Asserts the milestone assignment call receives the resolved milestone ID

This turns the regression description in the PR body into a verifiable spec that prevents the same silent-drop pattern from reappearing in the handler layer.

Fields: map[string]FieldValidation{
"issue_number": {IssueNumberOrTemporaryID: true},
"milestone_number": {Required: true, PositiveInteger: true},
"milestone_number": {OptionalPositiveInteger: true},
"milestone_title": {Type: "string", Sanitize: true, MaxLength: 128},

@github-actions github-actions Bot Jun 7, 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.

Validation accepts milestone_title: "" but the handler rejects it with a misleading error. "" satisfies the requiresOneOf check ("" !== undefined && "" !== false) and passes field validation (no minLength constraint), so the item is declared valid. But in the handler, item.milestone_title || null coerces "" to null, triggering the runtime error "Either milestone_number or milestone_title must be provided." An AI agent that passes milestone_title: "" gets a contradictory error — told to provide a field it already provided.

💡 Suggested fix

Add MinLength: 1 to the Go field definition:

"milestone_title": {Type: "string", Sanitize: true, MaxLength: 128, MinLength: 1},

Mirror it in the JS test config (safe_output_type_validator.test.cjs line 74):

milestone_title: { type: "string", sanitize: true, maxLength: 128, minLength: 1 },

The validateField path already enforces minLength after sanitization (validator line 401), so no new logic is needed. Add a test case { issue_number: 42, milestone_title: "" } to confirm rejection.

"repo": {Type: "string", MaxLength: 256}, // Optional: target repository in format "owner/repo"
},
},
Expand Down
Loading