Accept milestone_title in assign_milestone safe-output validation#37529
Conversation
Co-authored-by: dsyme <7204669+dsyme@users.noreply.github.com>
milestone_title in assign_milestone safe-output validation
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #37529 does not have the implementation label (has_implementation_label=false) and has 71 new lines of code in business logic directories, which is at or below the 100-line threshold (requires_adr_by_default_volume=false). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
Updates the safe-output validation configuration so assign_milestone accepts either milestone_number or milestone_title, preventing valid items from being dropped before handlers can resolve milestone titles. The PR also includes several unrelated linter-suppression tweaks (tolowerequalfold) and a small JS typing/prettier adjustment.
Changes:
- Relax
assign_milestonevalidation to require one ofmilestone_numberormilestone_title, and addmilestone_titlefield validation/sanitization. - Add Go and JS regression tests to ensure the new
requiresOneOfrule is present and enforced. - Apply multiple
//nolint:tolowerequalfoldsuppressions (and oneEqualFoldrefactor) across various files.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/safe_outputs_validation_config.go | Adds milestone_title support + requires-one-of validation for assign_milestone. |
| pkg/workflow/safe_output_validation_config_test.go | Adds regression tests for assign_milestone validation config and JSON serialization. |
| pkg/workflow/role_checks.go | Adds a tolowerequalfold suppression in association normalization. |
| pkg/workflow/observability_otlp.go | Adds a tolowerequalfold suppression in Sentry endpoint detection. |
| pkg/workflow/domains.go | Adds a tolowerequalfold suppression in provider parsing. |
| pkg/cli/workflows.go | Replaces ToLower(...) == with strings.EqualFold for README detection. |
| pkg/cli/outcome_eval_review.go | Adds multiple tolowerequalfold suppressions in review outcome evaluation. |
| pkg/cli/model_costs.go | Adds tolowerequalfold suppressions around normalization checks. |
| pkg/cli/effective_tokens.go | Adds a tolowerequalfold suppression for empty normalized model key check. |
| pkg/cli/codemod_steps_run_secrets_env.go | Adds a tolowerequalfold suppression in shell detection. |
| pkg/cli/audit_diff.go | Adds a tolowerequalfold suppression in bash tool detection. |
| pkg/cli/add_package_manifest.go | Adds a tolowerequalfold suppression around normalized filename key checks. |
| pkg/cli/add_interactive_schedule.go | Adds tolowerequalfold suppressions in fuzzy cron placeholder matching. |
| actions/setup/js/safe_output_type_validator.test.cjs | Adds assign_milestone validation config + acceptance/rejection tests for milestone title/number presence. |
| actions/setup/js/daily_effective_workflow_helpers.cjs | Adjusts JSDoc casting to be prettier-stable and clearer. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 15/15 changed files
- Comments generated: 4
| for _, association := range associations { | ||
| normalized := strings.ToUpper(strings.TrimSpace(association)) | ||
| if normalized != "" { | ||
| if normalized != "" { //nolint:tolowerequalfold | ||
| normalizedAssociations = append(normalizedAssociations, normalized) | ||
| } |
| 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") | ||
| } | ||
| } |
| 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 |
| } | ||
| state := strings.ToUpper(outcomeString(review["state"])) | ||
| submittedAt := outcomeString(review["submitted_at"]) | ||
| if state == "" || state == "PENDING" || submittedAt == "" { | ||
| if state == "" || state == "PENDING" || submittedAt == "" { //nolint:tolowerequalfold | ||
| continue | ||
| } |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /diagnose — no blocking issues, but four test-coverage gaps are worth addressing.
📋 Key Themes & Highlights
Key Themes
- Test coverage gaps: Three JS test cases missing (
milestone_number-only, both-fields, original regression); Go tests verify config shape but not field-validator semantics - Handler pipeline unverified: The fix prevents the silent drop at validation, but there is no integration test confirming
milestone_titleactually resolves end-to-end through the handler
Positive Highlights
- ✅ Core fix is correct and minimal —
requiresOneOf+OptionalPositiveInteger+ newmilestone_titlefield aligns the validation config with the documented MCP schema - ✅ Two-layer test strategy (Go struct assertions + JS behavioral tests) is a good pattern
- ✅ JSON round-trip test (
TestAssignMilestoneValidationConfigJSON) is a valuable addition that would have caught a serialization regression - ✅ The 10
//nolint:tolowerequalfoldadditions are appropriate — those comparisons operate on already-normalized values; the one genuine simplification opportunity (workflows.go) was correctly refactored usingstrings.EqualFold
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 838.2K · 251.5 AIC · ⌖ 38.6 AIC
| expect(result.isValid).toBe(true); | ||
| expect(result.normalizedItem).toBeDefined(); | ||
| expect(result.normalizedItem.milestone_title).toBe("v1.0"); | ||
| }); |
There was a problem hiding this comment.
[/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.
| expect(result.error).toContain("requires at least one of"); | ||
| expect(result.error).toContain("milestone_number"); | ||
| expect(result.error).toContain("milestone_title"); | ||
| }); |
There was a problem hiding this comment.
[/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).
| "assign_milestone": { | ||
| DefaultMax: 1, | ||
| DefaultMax: 1, | ||
| CustomValidation: "requiresOneOf:milestone_number,milestone_title", |
There was a problem hiding this comment.
[/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:
- Constructs an
assign_milestoneitem with onlymilestone_title: "v1.0" - Exercises the safe-output processor/handler
- 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.
| 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") |
There was a problem hiding this comment.
[/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.
There was a problem hiding this comment.
REQUEST_CHANGES — one validation gap in the new milestone_title field
The assign_milestone requiresOneOf migration is correct in intent, but the milestone_title field is missing a MinLength: 1 constraint. An empty-string value passes every validation layer yet fails at runtime with a misleading error.
Full analysis
Blocking issue
milestone_title: "" traces through all three defenses without rejection:
requiresOneOfcheck (safe_output_type_validator.cjsline 494):"" !== undefined && "" !== false→ both true → considered present.- Field validation:
type: "string", norequired, nominLength→ passes. - Handler (
assign_milestone.cjsline 177):item.milestone_title || null→null. - Handler guard (line 181):
!hasMilestoneNumber && !milestoneTitle→ fires, emitting "Either milestone_number or milestone_title must be provided."
An AI agent that follows the validated schema and passes milestone_title: "" will see a validator-approved call fail with an error telling it to provide a field it already provided. This contradiction is hard to recover from.
Fix: add MinLength: 1 to the milestone_title entry in both the Go config (line 126) and the JS test config mirror (line 74). No new validation logic is needed — the validateField path already enforces minLength (line 401 of safe_output_type_validator.cjs).
Non-blocking observations
- The
nolint:tolowerequalfoldsuppressions are appropriate wherestrings.ToLower(x) == ""is an empty-check or where both sides are already the same case. The one real candidate (isWorkflowFile) was correctly converted toEqualFold. daily_effective_workflow_helpers.cjsJSDoc cast fix is correct; the intermediaterecordvariable is harmless but could be collapsed to a single// prettier-ignore+ return line.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 513K · ⌖ 13 AIC
| "issue_number": {IssueNumberOrTemporaryID: true}, | ||
| "milestone_number": {Required: true, PositiveInteger: true}, | ||
| "milestone_number": {OptionalPositiveInteger: true}, | ||
| "milestone_title": {Type: "string", Sanitize: true, MaxLength: 128}, |
There was a problem hiding this comment.
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.
🧪 Test Quality Sentinel Report
📊 Metrics & Test Classification (4 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
|
assign_milestoneitems were being dropped during post-processing when they providedmilestone_titlewithoutmilestone_number, despite the MCP schema and tool docs allowing either field. This caused valid sampled outputs to disappear before the safe-output handler could resolve titles.Validation config alignment (
pkg/workflow)assign_milestonevalidation to require one ofmilestone_numberormilestone_title(instead of hard-requiringmilestone_number).milestone_numberto optional-positive-integer validation.milestone_titleas a validated/sanitized string field.Regression coverage (Go config tests)
assign_milestoneincludes:customValidation: requiresOneOf:milestone_number,milestone_titlemilestone_numberandmilestone_titlefieldsRegression coverage (JS validator tests)
assign_milestonewith onlymilestone_titleis accepted.