Make safe-job conclusion dependencies deterministic#40363
Conversation
|
Hey A couple of things to land before this is ready:
If you'd like a nudge, here's a ready-to-use prompt:
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #40363 does not have the 'implementation' label (has_implementation_label=false) and has only 57 new lines in business logic directories (requires_adr_by_default_volume=false, threshold=100). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
This pull request makes compilation of custom safe-outputs.jobs deterministic by ensuring safe-job construction is based on a stable ordering, and adds a regression test to prevent .lock.yml output drift across runs.
Changes:
- Sort
safe-outputs.jobskeys before building custom safe-jobs to avoid Go map iteration nondeterminism. - Ensure the list of returned safe-job names (used for downstream dependency wiring) is deterministic.
- Add a workflow-compilation regression test asserting the
conclusionjob’sneedsordering for multiple safe-jobs.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/safe_jobs.go |
Sorts safe-job map keys prior to building jobs to make safe-job ordering stable. |
pkg/workflow/safe_jobs_test.go |
Adds a compiled-workflow regression test validating deterministic conclusion.needs ordering for multiple safe-jobs. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
![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 /diagnose and /tdd — no blocking issues; two suggestions to improve robustness.
📋 Key Themes & Highlights
Key Themes
-
Sort target mismatch (edge case): The fix sorts original job names, but
NormalizeSafeOutputIdentifierreplaces both-and.with_. For names mixing.and_, the original-name sort order can differ from the normalized-name sort order, sosafeJobNames(and the conclusion job'sneeds:) may still be non-deterministic in that narrow case. Sorting on normalized names would close this gap cleanly. -
Test helper consistency: The regression test uses
strings.Indexfor order comparisons;indexInNonCommentLinesalready exists in the test helpers for exactly this purpose.
Positive Highlights
- ✅ Root cause correctly identified: Go map iteration non-determinism in
buildSafeJobs - ✅ Fix is minimal and surgical — collect → sort → iterate is idiomatic Go
- ✅ Regression test is end-to-end: declares jobs in reverse-alphabetical order, compiles, reads the lock file, and asserts the conclusion job's
needs:is alphabetically sorted - ✅
+57/-1lines; no API surface or schema changes
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 78.6 AIC · ⌖ 6.85 AIC · ⊞ 6.9K
| for jobName := range data.SafeOutputs.Jobs { | ||
| jobNames = append(jobNames, jobName) | ||
| } | ||
| sort.Strings(jobNames) |
There was a problem hiding this comment.
[/diagnose] Sorting by original name may not preserve sort order after normalization for .-containing job names.
NormalizeSafeOutputIdentifier maps both - (0x2D) and . (0x2E) to _ (0x5F). Because - < . < _ in ASCII, the sorted order of original names can differ from the sorted order of their normalized counterparts — the conclusion job's needs: could still be non-deterministic for job names that include ..
💡 Concrete example and suggested fix
Given original keys a.z and a_a:
- originals sorted:
[a.z, a_a](because.0x2E <_0x5F) - normalized in that order:
[a_z, a_a]— not sorted (becausea_z>a_a)
The safer approach is to sort on normalized names:
// collect normalized names and a lookup map
normalized := make([]string, 0, len(data.SafeOutputs.Jobs))
configByNorm := make(map[string]*SafeJobConfig, len(data.SafeOutputs.Jobs))
for jobName, jobConfig := range data.SafeOutputs.Jobs {
n := stringutil.NormalizeSafeOutputIdentifier(jobName)
normalized = append(normalized, n)
configByNorm[n] = jobConfig
}
sort.Strings(normalized)
for _, normalizedJobName := range normalized {
jobConfig := configByNorm[normalizedJobName]
// ... rest of loop unchanged, drop the NormalizeSafeOutputIdentifier call
}This is an edge case today—job names with . are unusual—but NormalizeSafeOutputIdentifier explicitly documents the executor-workflow.agent pattern, so it is a plausible future input.
| zebraIdx := strings.Index(conclusionSection, "- zebra_job") | ||
| require.NotEqual(t, -1, alphaIdx, "conclusion job should depend on alpha_job") | ||
| require.NotEqual(t, -1, zebraIdx, "conclusion job should depend on zebra_job") | ||
| require.Less(t, alphaIdx, zebraIdx, "conclusion job should list safe-jobs in deterministic sorted order") |
There was a problem hiding this comment.
[/tdd] Order comparison uses strings.Index directly; the codebase has indexInNonCommentLines for exactly this purpose.
compiler_test_helpers_test.go defines indexInNonCommentLines to avoid false matches when a compiled YAML contains job names embedded in frontmatter comment lines. Using the helper here keeps the test consistent with the established pattern and guards against false-positive ordering assertions.
💡 Suggested change
- alphaIdx := strings.Index(conclusionSection, "- alpha_job")
- zebraIdx := strings.Index(conclusionSection, "- zebra_job")
+ alphaIdx := indexInNonCommentLines(conclusionSection, "- alpha_job")
+ zebraIdx := indexInNonCommentLines(conclusionSection, "- zebra_job")The risk is low because the conclusion section is unlikely to have comment lines with job names, but it makes the intent explicit.
🧪 Test Quality Sentinel — PR #40363Score: 90/100 ✅ Excellent One new test ( 📊 Metrics
Score components:
📋 Per-Test Classification
Key assertions:
i️ Inflation Note
Verdict: ✅ PASS — 0% implementation tests (threshold: 30%), no coding-guideline violations detected. Build tag References:
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 90/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). The single new test directly verifies the deterministic ordering behavioral contract introduced by this PR, using adversarial input and appropriate assertion messages.
There was a problem hiding this comment.
✅ Approved — fix is correct, two non-blocking observations
The root cause (Go map iteration order flowing into the conclusion job's needs: list) is correctly fixed by collecting keys, sorting, then re-iterating. The returned safeJobNames slice is deterministic. The regression test covers the primary scenario.
Findings summary
Non-blocking (2):
sort.Stringssorts source key names, not normalized output names — in edge cases involving names that mix hyphens and digits the emittedneeds:order may differ from sorted-normalized-name order. Common all-hyphen names are unaffected. (See inline comment onsafe_jobs.go:169.)- Ordering assertion uses
strings.Indexrather than the package's existingindexInNonCommentLineshelper, which is the established pattern for order checks on compiled YAML. (See inline comment onsafe_jobs_test.go:286.)
Dropped:
sort.Stringsvsslices.Sort—sort.Stringsis the existing pattern throughout the workflow package.- Normalization collision detection — pre-existing concern, out of scope for this fix.
- Test exercising only two jobs — adequate for an ordering regression test.
🔎 Code quality review by PR Code Quality Reviewer · 81.7 AIC · ⌖ 7.3 AIC · ⊞ 5.1K
| for jobName := range data.SafeOutputs.Jobs { | ||
| jobNames = append(jobNames, jobName) | ||
| } | ||
| sort.Strings(jobNames) |
There was a problem hiding this comment.
Sort is on source key names, not normalized output names — in edge cases these orders can diverge, making the emitted needs: list stable but not sorted-by-normalized-name.
💡 Details
The sort compares the original frontmatter key strings (e.g. a-job, a0job). After normalization hyphens and dots become underscores, but digits stay. ASCII ordering: '-' (0x2D) < '0' (0x30) < '_' (0x5F), so:
- source order:
a-job<a0job(hyphen sorts before digit) - normalized order:
a0job<a_job(digit sorts before underscore)
...the conclusion job's needs: list is stable but its order won't match alphabetical-by-normalized-name. For all-hyphen-separated names (the common case) there is no divergence. To guarantee normalized-order stability, sort after normalizing:
type safeJobEntry struct {
normalizedName string
config *SafeJobConfig
}
entries := make([]safeJobEntry, 0, len(data.SafeOutputs.Jobs))
for rawName, cfg := range data.SafeOutputs.Jobs {
entries = append(entries, safeJobEntry{stringutil.NormalizeSafeOutputIdentifier(rawName), cfg})
}
sort.Slice(entries, func(i, j int) bool { return entries[i].normalizedName < entries[j].normalizedName })Non-blocking: the current approach is deterministic and correct for typical naming conventions.
| conclusionSection := extractJobSection(string(compiled), "conclusion") | ||
| require.NotEmpty(t, conclusionSection) | ||
|
|
||
| alphaIdx := strings.Index(conclusionSection, "- alpha_job") |
There was a problem hiding this comment.
Use indexInNonCommentLines instead of strings.Index for ordering assertions — the codebase provides this helper specifically for compiled-YAML ordering checks, and it guards against future changes that embed extra content as YAML comments.
💡 Suggested fix
// Instead of:
alphaIdx := strings.Index(conclusionSection, "- alpha_job")
zebraIdx := strings.Index(conclusionSection, "- zebra_job")
// Use the existing non-comment-aware helper:
alphaIdx := indexInNonCommentLines(conclusionSection, "- alpha_job")
zebraIdx := indexInNonCommentLines(conclusionSection, "- zebra_job")indexInNonCommentLines (defined in compiler_test_helpers_test.go) is already used in the same package for exactly this pattern. While comment lines in the compiled lock YAML would typically use the original hyphenated job names (not the underscore-normalized forms searched here), using the consistent helper makes the assertion robust against future emitter changes and mirrors established test idioms in this file.
Non-blocking.
|
@copilot run pr-finisher skill |
… test Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done — both issues from the github-actions[bot] review are addressed in commit
|
buildSafeJobswas deriving conclusion-jobneeds:entries from Go map iteration order, so workflows with multiple custom safe-jobs could compile to different.lock.ymloutput across runs. This change makes safe-job ordering stable and adds a regression test at the compiled workflow level.Compiler behavior
safe-outputs.jobskeys before building custom safe-jobsRegression coverage
conclusionjob lists normalized safe-job dependencies in sorted orderIllustrative change