compiler: error on inputs declared under custom jobs - #48607
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #48607 does not have the 'implementation' label and has only 31 new lines of code in business logic directories (threshold: 100). |
There was a problem hiding this comment.
Pull request overview
Adds validation intended to reject unsupported jobs.*.inputs declarations.
Changes:
- Adds an actionable compilation error for custom-job inputs.
- Adds unit coverage for the error path.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/compiler_custom_jobs.go |
Rejects inputs on custom jobs. |
pkg/workflow/compiler_custom_jobs_test.go |
Tests direct custom-job validation. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
pkg/workflow/compiler_custom_jobs.go:179
- For a reusable-workflow caller (
uses), this remediation is invalid because caller jobs cannot useenv; reusable-workflow inputs must be passed withwith. Tailor the guidance forusesjobs so this error does not direct users to another invalid configuration.
return fmt.Errorf("jobs.%s.inputs: inputs are not supported on jobs; use 'env' to pass values to job steps", jobName)
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Medium
| } | ||
|
|
||
| func (c *Compiler) extractCustomJobCoreProperties(job *Job, jobName string, configMap map[string]any) error { | ||
| if _, hasInputs := configMap["inputs"]; hasInputs { |
There was a problem hiding this comment.
Implemented in a3561ac. I added a pre-schema validation in parser validation flow so jobs.<name>.inputs now fails before JSON schema, and added both parser-level and compile-string end-to-end tests to verify the actionable error is returned (not the generic unknown-property message). Kept the compiler-side check as defense-in-depth for direct builder paths that bypass schema.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The change is minimal and correct. The early guard in extractCustomJobCoreProperties is placed appropriately, the error message is actionable, and the test covers both the error path and the expected message. No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 8.68 AIC · ⌖ 5.16 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — changes look solid. Approving.
📋 Key Themes & Highlights
Key Themes
- Correct guard placement: the
inputscheck runs before any other property extraction, failing fast and clearly. - Clear error message: names the invalid field path (
jobs.<name>.inputs) and recommends the fix (use env).
Positive Highlights
- ✅ Regression test added alongside the fix — good
/tddpractice. - ✅ Test asserts both the error path and the specific message content.
- ✅ Minimal, surgical change with no unrelated side-effects.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 12.7 AIC · ⌖ 4.58 AIC · ⊞ 6.7K
Comment /matt to run again
🧪 Test Quality Sentinel Report✅ Test Quality Score: 85/100 — Excellent
📊 Metrics (1 test)
Analysis
Verdict
|
There was a problem hiding this comment.
Verdict: no blocking issues, but the added check is unreachable dead code in the normal compile path
The new inputs guard in extractCustomJobCoreProperties never fires during a real gh aw compile run — schema validation already rejects jobs.<name>.inputs before buildCustomJob is called.
💡 Details
I verified that ValidateMainWorkflowFrontmatterWithSchemaAndLocation (invoked before buildCustomJob in both compiler_orchestrator_frontmatter.go and compiler_string_api.go) already rejects jobs.<name>.inputs via JSON schema: main_workflow_schema.json's job properties list does not include inputs, and additionalProperties: false is set on jobs. I compiled a test workflow with inputs: under a custom job and it failed at the schema layer with Unknown property: inputs, before the code in this PR is ever reached.
The only way the new Go-level error path executes is the new unit test, which calls compiler.buildCustomJob(...) directly, bypassing schema validation entirely. This may be reasonable defense-in-depth if buildCustomJob/CompileWorkflowData can be invoked from another path that skips schema validation, but the PR description's claim that inputs "was silently ignored, masking misconfiguration" appears inaccurate for the standard CLI compile flow — it was already a hard compile error at the schema stage. Please confirm there is a genuinely reachable path where schema validation is skipped and this check adds value; otherwise this is redundant code justified by a misleading problem statement.
Not blocking merge since it introduces no incorrect behavior, but worth clarifying before merge.
🔎 Code quality review by PR Code Quality Reviewer · sonnet50 · 39.1 AIC · ⌖ 4.36 AIC · ⊞ 7.4K
Comment /review to run again
| if _, hasInputs := configMap["inputs"]; hasInputs { | ||
| return fmt.Errorf("jobs.%s.inputs: inputs are not supported on jobs; use 'env' to pass values to job steps", jobName) |
There was a problem hiding this comment.
This check is dead code in the real compile path — schema validation already rejects jobs.<name>.inputs before buildCustomJob is ever called.
There was a problem hiding this comment.
Addressed in a3561ac by adding pre-schema validation for jobs.<name>.inputs plus end-to-end coverage through ParseWorkflowString, so the normal compile path now emits the actionable jobs.inputs error instead of a generic schema unknown-property message.
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
jobs.*.inputsis not a valid GitHub Actions field for regular jobs (only reusable workflow callers support it). Declaring it on a custom job was silently ignored, masking misconfiguration.Changes
pkg/workflow/compiler_custom_jobs.go—extractCustomJobCorePropertiesnow returns a compilation error wheninputsis present in the job config:pkg/workflow/compiler_custom_jobs_test.go—TestBuildCustomJob_InputsNotSupportedErrorcovers the new error path.