Fix trial forwarding issue_number to workflows that don't declare it#50638
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 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. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix is correct and well-tested. workflowDeclaresDispatchInput correctly fails-safe to false on read/parse errors, preventing unwanted --field args from being forwarded to workflows that did not declare them.
One minor nit: the // saveTrialResult saves a trial result to a JSON file doc comment was removed and no blank line was added between the closing } of workflowDeclaresDispatchInput and func saveTrialResult. Small cosmetic issue, not blocking.> 🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 19.3 AIC · ⌖ 7.44 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Pull request overview
Fixes trial dispatch failures by forwarding issue_number only to workflows that declare it.
Changes:
- Inspects compiled lock-file dispatch inputs before forwarding
issue_number. - Passes each workflow’s lock-file path into dispatch logic.
- Adds coverage for declared, undeclared, absent, and unreadable inputs.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/trial_helpers.go |
Conditionally forwards trigger-derived inputs. |
pkg/cli/trial_issue_mode_test.go |
Tests dispatch-input detection cases. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — approving with one minor nit.
📋 Key Themes & Highlights
Key Themes
- Root cause addressed: the fix correctly guards
--field issue_numberbehind a YAML parse of the lock file'son.workflow_dispatch.inputs, rather than patching the caller. - Fail-safe default:
workflowDeclaresDispatchInputreturnsfalseon read/parse error, which is exactly the safe direction. - One cosmetic nit: the
// saveTrialResult saves...doc comment and its preceding blank line were accidentally dropped in the same hunk — see the inline comment.
Positive Highlights
- ✅ Comprehensive test matrix: declared input, undeclared input, dispatch without inputs, schedule-only trigger, missing file — all covered.
- ✅ Verbose-mode logging retained for both the forwarded and skipped paths.
- ✅
triggerWorkflowRun's signature change is minimal and backward-compatible within the package. - ✅ YAML parsing uses the already-imported
go-yamllibrary with an anonymous struct — no over-engineering.
| _, ok := parsed.On.WorkflowDispatch.Inputs[inputName] | ||
| return ok | ||
| } | ||
| func saveTrialResult(filename string, result any, verbose bool) error { |
There was a problem hiding this comment.
[/diagnosing-bugs] The doc comment // saveTrialResult saves a trial result to a JSON file was dropped in this diff, and there is no blank line between workflowDeclaresDispatchInput's closing brace and saveTrialResult.
💡 Suggested fix
Restore the blank line and the comment:
return ok
}
// saveTrialResult saves a trial result to a JSON file
func saveTrialResult(filename string, result any, verbose bool) error {@copilot please address this.
🧪 Test Quality Sentinel ReportExecutive Summary✅ Test Quality Score: 83/100 (Excellent) 4 new test functions added with comprehensive edge case coverage and no code violations. All tests are design-focused behavioral tests verifying public API contracts. Test Coverage Overview
Test-by-Test Analysis1. TestExtractIssueNumberFromURL (10 subtests)Design: URL parsing invariant — extract issue numbers from GitHub URLs only, reject invalid/foreign hosts 2. TestExtractIssueNumberFromURL_GHES (6 subtests)Design: Multi-host invariant — correctly handle GitHub Enterprise Server configuration 3. TestTrialWorkflowSpecParsing (3 subtests)Design: Workflow spec validation — parse specs and validate format 4. TestWorkflowDeclaresDispatchInput (5 test cases)Design: Workflow schema validation — verify lock file declares dispatch inputs Design Invariants Verified
Code Quality ObservationsPositive Signals:
Test Inflation:
Scoring CalculationThreshold: Conclusion✅ APPROVED — No violations, comprehensive design-focused test coverage with excellent edge case handling. Test inflation slightly above threshold is justified by breadth of URL parsing and YAML validation scenarios.
|
…lock file schema check Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (114 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
Verdict: Comment — logic is sound but error handling and duplication risks need attention
Themes
- Fail-safe error swallowing:
workflowDeclaresDispatchInputreturnsfalsefor read errors, parse errors, and genuine absence alike — conflates a real bug (bad path, broken lock file) with legitimate 'input not declared', silently reintroducing a variant of the original 422 bug at a different layer. - Duplicated path/name construction:
lockFileName(short name, used forgh workflow run) andlockFilePath(full path, used for input inspection) are built independently; if one convention changes without the other, the input-forwarding decision silently diverges from what actually runs. - Minor: dropped doc comment before
saveTrialResult. - Test coverage for the new helper itself is solid (declared/undeclared/no-inputs/missing-file cases all covered).> 🔎 Code quality review by PR Code Quality Reviewer · auto · 49.1 AIC · ⌖ 4.38 AIC · ⊞ 7.9K
Comment /review to run again
| // workflowDeclaresDispatchInput reports whether the compiled lock file at lockFilePath | ||
| // declares the given workflow_dispatch input. It returns false if the file cannot be | ||
| // read or parsed, so that trigger-derived inputs are not forwarded to workflows whose | ||
| // workflow_dispatch schema does not declare them (which would cause an HTTP 422). |
There was a problem hiding this comment.
Swallowing read/parse errors as false conflates "input genuinely not declared" with "lock file missing/corrupt/wrong path" — this silently hides real bugs (e.g. a stale or mistyped lockFilePath) rather than surfacing them.
💡 Details
workflowDeclaresDispatchInput returns false uniformly whether the file genuinely lacks the input, the file doesn't exist yet (e.g. compile step failed or path mismatch), or the YAML fails to parse for an unrelated reason. All three cases currently just log via trialLog.Printf and silently disable issue_number forwarding — which is exactly the class of bug this PR set out to fix (workflow silently never receiving the intended input), just moved one level deeper.
Consider distinguishing "file not found" (arguably fine to fail-safe) from parse errors on an existing file (should probably surface a warning to the user even outside verbose mode, since it indicates a compiler/format problem).
if verbose || err != nil {
fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Could not parse lock file %s: %v", lockFilePath, err)))
}| args := []string{"workflow", "run", lockFileName, "--repo", repoSlug} | ||
|
|
||
| // If trigger context is provided, extract issue number and add it as input | ||
| // If trigger context is provided, extract issue number and add it as input. |
There was a problem hiding this comment.
lockFileName (used to invoke gh workflow run) and the caller's lockFilePath (used to inspect declared inputs) are built via two separate, unlinked expressions — a future edit to one path convention without the other will silently break input-forwarding decisions.
| } | ||
|
|
||
| _, ok := parsed.On.WorkflowDispatch.Inputs[inputName] | ||
| return ok |
There was a problem hiding this comment.
The saveTrialResult doc comment was dropped/merged into the new function's closing brace, losing the godoc entry for that function.
PR Triage: #50638Category: bug | Risk: medium | Priority Score: 58/100 (impact 25, urgency 18, quality 15)
|
|
@copilot please follow up on the latest review feedback on this PR:
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
gh aw trialappendedissue_numberas aworkflow_dispatchfield to every workflow in a trial. Workflows whose compiled schema doesn't declare that input (e.g. scheduled/global companions) rejected the dispatch withHTTP 422: Unexpected inputs provided: ["issue_number"]and were silently never run.The trigger-derived input is now forwarded only to workflows that declare it, using the lock file already available locally at dispatch time.
Changes
workflowDeclaresDispatchInput(lockFilePath, inputName)(new): parseson.workflow_dispatch.inputsfrom the compiled.lock.yml; fails safe (false) on read/parse errors so inputs are never forwarded to workflows that may reject them.triggerWorkflowRun: takes the lock file path and appends--field issue_number=…only when the input is declared; otherwise logs an informational note in verbose mode instead of failing.executeTrialRun: passes the per-workflow lock file path.workflow_dispatchwithout inputs, schedule-only triggers, and missing files.Behavior
In a mixed-trigger trial (entity-triggered workflow + scheduled companion), the issue-triggered workflow still receives
issue_numberwhile the scheduled one dispatches cleanly without it.issue_numberto workflows that don't declare it, causing HTTP 422 and a silently skipped dispatch #50584