feat: compile-time validation of needs: ordering on custom safe-output jobs#23486
Merged
feat: compile-time validation of needs: ordering on custom safe-output jobs#23486
Conversation
Add compile-time validation of needs: declarations on custom safe-output
jobs (defined under safe-outputs.jobs):
- Add SafeOutputsJobName, UploadAssetsJobName, ConclusionJobName, and
UnlockJobName constants to pkg/constants/constants.go
- Add pkg/workflow/safe_jobs_needs_validation.go with:
- validateSafeJobNeeds: validates each custom safe-job's needs: entries
against jobs that will actually exist in the compiled workflow
- computeValidSafeJobNeeds: computes valid job IDs from workflow config
(threat detection, upload-asset, lock-for-agent, other safe-jobs)
- detectSafeJobCycles: DFS-based cycle detection between custom safe-jobs
- Hook validateSafeJobNeeds into compiler.go:validateWorkflowData
- Add pkg/workflow/safe_jobs_needs_validation_test.go with comprehensive tests
Valid needs: targets: agent, safe_outputs, detection (when enabled),
upload_assets (when configured), unlock (when lock-for-agent enabled),
and other custom safe-job names. All other targets fail compilation with
a clear error listing valid alternatives. Cycles are also detected.
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/5fc1500e-7e47-4c1e-a6ce-b6bb9d465276
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Fix ordering of custom safe-output jobs relative to generated jobs
feat: compile-time validation of needs: ordering on custom safe-output jobs
Mar 29, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Adds compile-time validation for safe-outputs.jobs[*].needs so custom safe-output jobs can declare ordering against generated jobs without edits being lost on recompile.
Changes:
- Introduces job name constants for generated jobs (e.g.,
safe_outputs,upload_assets,unlock). - Adds
validateSafeJobNeeds+ helpers to validateneedstargets and detect cycles among custom safe-jobs. - Adds unit tests covering valid/invalid
needstargets and cycle detection.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/constants/constants.go | Adds job-name constants intended to be used when reasoning about generated job IDs. |
| pkg/workflow/compiler.go | Hooks new safe-job needs validation into compile-time workflow validation. |
| pkg/workflow/safe_jobs_needs_validation.go | Implements validation of custom safe-job needs and DFS-based cycle detection. |
| pkg/workflow/safe_jobs_needs_validation_test.go | Adds unit tests for needs validation, valid target computation, and cycle detection. |
Comments suppressed due to low confidence (2)
pkg/workflow/safe_jobs_needs_validation.go:134
- detectSafeJobCycles builds a
normalizedmap keyed byNormalizeSafeOutputIdentifier(name), which will silently overwrite entries if two different custom job keys normalize to the same ID (e.g.my-jobandmy_job). That can hide real cycles and/or produce confusing cycle reports. Please add an explicit collision check when populatingnormalized/originalNamesand return a clear error indicating the conflicting job names after normalization.
// Build normalized name mapping
normalized := make(map[string]*SafeJobConfig, len(jobs))
originalNames := make(map[string]string, len(jobs))
for name, cfg := range jobs {
n := stringutil.NormalizeSafeOutputIdentifier(name)
normalized[n] = cfg
originalNames[n] = name
}
pkg/workflow/safe_jobs_needs_validation.go:86
- computeValidSafeJobNeeds unconditionally adds
safe_outputswheneverdata.SafeOutputs != nil, but the compiler can skip generating the consolidatedsafe_outputsjob when no safe output steps are added (seebuildConsolidatedSafeOutputsJob: it returns nil whensafeOutputStepNamesis empty). In workflows that only configuresafe-outputs.jobs(custom safe-jobs) and no other safe outputs, this will cause the validator to acceptneeds: safe_outputseven though the compiled workflow will not contain that job ID. Please makesafe_outputsa valid target only when the consolidated job will actually be emitted (or alternatively ensure the compiler always emits asafe_outputsjob, e.g. with a noop step, when safe-outputs is configured).
// safe_outputs job is always present when safe-outputs is configured
valid[string(constants.SafeOutputsJobName)] = true
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+34
to
+41
| name: "needs safe_outputs – always valid", | ||
| data: &WorkflowData{ | ||
| SafeOutputs: &SafeOutputsConfig{ | ||
| Jobs: map[string]*SafeJobConfig{ | ||
| "packaging": { | ||
| Needs: []string{"safe_outputs"}, | ||
| Steps: []any{map[string]any{"run": "echo hi"}}, | ||
| }, |
There was a problem hiding this comment.
This test assumes needs: safe_outputs is always valid whenever safe-outputs.jobs is present, but the compiler can omit the safe_outputs job entirely when there are no consolidated safe output steps (only custom safe-jobs configured). If safe_outputs is only conditionally generated, the validator + tests should reflect that (and/or the compiler should be updated to always emit safe_outputs when safe-outputs is configured).
Comment on lines
+42
to
+52
| normalizedJobName := stringutil.NormalizeSafeOutputIdentifier(originalName) | ||
| for _, need := range jobConfig.Needs { | ||
| normalizedNeed := stringutil.NormalizeSafeOutputIdentifier(need) | ||
| if !validIDs[normalizedNeed] { | ||
| return fmt.Errorf( | ||
| "safe-outputs.jobs.%s: unknown needs target %q\n\nValid dependency targets for custom safe-jobs are:\n%s\n\n"+ | ||
| "Custom safe-jobs cannot depend on workflow control jobs such as 'conclusion' or 'activation'", | ||
| originalName, | ||
| need, | ||
| formatValidNeedsTargets(validIDs), | ||
| ) |
There was a problem hiding this comment.
validateSafeJobNeeds normalizes each need (dash→underscore) for membership checks, but it does not normalize the stored value in jobConfig.Needs. As a result, a config like needs: safe-outputs or needs: other-job can pass validation (because it normalizes to safe_outputs / other_job) while the compiler will still render needs: safe-outputs / other-job into YAML, which will not match the actual job IDs (the compiler normalizes safe-job IDs to underscores). Consider either (1) rejecting non-canonical needs values with an error that tells the user to use the underscore form, or (2) rewriting jobConfig.Needs entries to the normalized form during validation so the rendered workflow is correct.
This issue also appears in the following locations of the same file:
- line 84
- line 127
Collaborator
|
@copilot review comments |
…exist; normalize needs values
- Add consolidatedSafeOutputsJobWillExist helper to correctly predict
whether the compiled workflow will include a safe_outputs job.
The consolidated job is only emitted when at least one builtin
safe-output handler, custom script, custom action, or user-provided
step is configured — custom safe-jobs (safe-outputs.jobs) compile to
separate jobs and do NOT trigger safe_outputs creation.
- Rewrite needs entries to their canonical underscore form during
validation so the compiled YAML references the correct job IDs
(e.g. "safe-outputs" → "safe_outputs").
- Update tests:
- "needs safe_outputs" test now includes a builtin type (create-issue)
so the consolidated job actually exists
- Add negative test: safe_outputs invalid when only custom jobs present
- Add TestValidateSafeJobNeeds_NeedsNormalization to verify in-place
rewrite of dash-form needs values
- Add TestConsolidatedSafeOutputsJobWillExist for the new helper
- Update TestComputeValidSafeJobNeeds to reflect conditional safe_outputs
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/ec1306c6-ecfc-4f24-8c77-57209e6ce8ef
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
Author
Both review comments addressed in f649a17:
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
github-actions bot
added a commit
that referenced
this pull request
Mar 30, 2026
Add needs: property to the Job Properties table and a new "Job Ordering" section in the Custom Safe Outputs reference, documenting the compile-time validation, valid targets, and dash-to-underscore normalization introduced in #23486. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Custom safe-output jobs (
safe-outputs.jobs) had no first-class way to declare ordering relative to generated jobs likesafe_outputs. Authors patchingneeds:directly into.lock.ymlhad those edits silently wiped on every recompile.Changes
New job name constants (
pkg/constants/constants.go)SafeOutputsJobName,UploadAssetsJobName,ConclusionJobName,UnlockJobNameCompile-time validation (
pkg/workflow/safe_jobs_needs_validation.go)validateSafeJobNeeds— validates each custom safe-job'sneeds:entries against the jobs that will actually exist in the compiled workflow; fails with a clear error listing valid alternatives. Also rewrites eachneeds:entry to its canonical underscore form in-place (e.g.safe-outputs→safe_outputs) so the compiled YAML always references the correct job ID.computeValidSafeJobNeeds— dynamically resolves valid targets based on workflow config (threat detection state, upload-asset, lock-for-agent, peer safe-jobs)consolidatedSafeOutputsJobWillExist— predicts whether thesafe_outputsconsolidated job will be emitted; it is only created when at least one builtin handler, custom script, custom action, or user-provided step is configured. Workflows with onlysafe-outputs.jobsentries do not produce asafe_outputsjob, sosafe_outputsis not a validneeds:target in that case.detectSafeJobCycles— DFS cycle detection among custom safe-jobsHooked into
validateWorkflowDataalongside existing safe-outputs validators.Valid
needs:targets for custom safe-jobsagentsafe_outputsdetectionupload_assetsupload-assetconfiguredunlocklock-for-agentenabled<other safe-job name>safe-outputs.jobsExample
Self-dependencies and inter-job cycles are also caught at compile time.
⚡ Quickly spin up Copilot coding agent tasks from anywhere on your macOS or Windows machine with Raycast.