Conversation
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/cda45cd0-2cd5-4281-8447-72e88f5168af Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/cda45cd0-2cd5-4281-8447-72e88f5168af Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/cda45cd0-2cd5-4281-8447-72e88f5168af Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Fix needs: duplication/self-cycle when built-in job names are used
Prevent built-in Apr 22, 2026
jobs.* customization entries from creating agent needs duplicates/cycles
pelikhan
reviewed
Apr 22, 2026
|
|
||
| func isBuiltinJobName(jobName string) bool { | ||
| switch jobName { | ||
| case string(constants.PreActivationJobName), "pre-activation", |
Contributor
Author
There was a problem hiding this comment.
Done in f51e560. I centralized built-in job names in pkg/constants (including aliases) via KnownBuiltInJobNames and updated isBuiltinJobName to use that shared constants map; tests were expanded accordingly.
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/297b8ad8-80f4-4721-b825-fda378b948dc Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR prevents built-in jobs.* customization entries (used for pre-steps) from being treated as custom jobs when computing the main agent job dependencies, avoiding duplicate needs entries and dependency cycles.
Changes:
- Added
isBuiltinJobName(jobName string) booland used it to skip built-in job IDs when injectingagent.needs. - Updated both dependency-injection paths (iterating
data.Jobsand scanning referenced jobs in workflow/custom-step content) to use the new guard. - Added regression tests to ensure built-in customization keys do not introduce invalid
agent.needsdependencies, plus direct tests forisBuiltinJobName.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/compiler_main_job.go | Adds a built-in job-name guard and applies it to agent.needs injection logic. |
| pkg/workflow/compiler_jobs_test.go | Adds regression tests covering built-in job customization keys and helper behavior. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 1
Comment on lines
+16
to
22
| func isBuiltinJobName(jobName string) bool { | ||
| _, isBuiltIn := constants.KnownBuiltInJobNames[jobName] | ||
| return isBuiltIn | ||
| } | ||
|
|
||
| // buildMainJob creates the main agent job that runs the AI agent with the configured engine and tools. | ||
| // This job depends on the activation job if it exists, and handles the main workflow logic. |
There was a problem hiding this comment.
isBuiltinJobName only includes a subset of built-in job IDs. There are additional built-in jobs defined in pkg/constants/job_constants.go (e.g. detection, upload_assets, upload_code_scanning_sarif, unlock) that can also appear under jobs: as customization-only entries. If one of those appears in data.Jobs, buildMainJob will currently treat it as a custom job and may inject it into agent.needs, which can create self-dependency cycles (e.g. detection depends on agent, so adding agent -> detection creates a cycle) or reference a job that may not exist. Expand isBuiltinJobName (and its tests) to include all built-in job IDs (and any supported dash/underscore aliases), or centralize this logic by reusing an existing reserved-job predicate.
Collaborator
|
@copilot Review all comments |
Contributor
|
Warning The 🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (122 new lines in AI has analyzed the PR diff and generated a draft ADR to help you get started: 📄 Draft ADR: 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
References: §24759421035
|
Contributor
🧪 Test Quality Sentinel ReportTest Quality Score: 90/100✅ Excellent test quality
Test Classification DetailsView test classification table
Test AnalysisBoth new tests are behavioral contract tests that directly exercise observable outputs of the production code fix.
This is precisely the behavioral contract that the fix in
Flagged Items
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/297b8ad8-80f4-4721-b825-fda378b948dc Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
Author
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. 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:
|
Contributor
Author
pelikhan
approved these changes
Apr 22, 2026
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.
When built-in job IDs are used under
jobs:forpre-stepscustomization, the main job dependency loop was treating them as custom jobs. This could inject invalidagent.needsentries (duplicateactivation, self-dependency onagent, and cycles involvingsafe_outputs/conclusion).What changed
pkg/constants/job_constants.go:PreActivationHyphenJobName(pre-activation)SafeOutputsHyphenJobName(safe-outputs)KnownBuiltInJobNames map[string]struct{}agent,activation,pre_activation/pre-activation,detection,safe_outputs/safe-outputs,upload_assets,upload_code_scanning_sarif,conclusion,unlock).pkg/workflow/compiler_main_job.go:isBuiltinJobName(jobName string) boolnow checksconstants.KnownBuiltInJobNamesinstead of hardcoded literals.isBuiltinJobName(...)in both dependency-addition paths:data.JobsRegression coverage
pkg/workflow/compiler_jobs_test.goto verify built-in customization keys are excluded from agent dependency injection:jobs.activation.pre-stepsdoes not produce duplicateactivationinagent.needsjobs.agent.pre-stepsdoes not addagenttoagent.needsjobs.safe_outputs.pre-stepsdoes not addsafe_outputstoagent.needsjobs.conclusion.pre-stepsdoes not addconclusiontoagent.needsdetection,upload_assets,upload_code_scanning_sarif,unlock)pkg/constants/constants_test.go:TestKnownBuiltInJobNamesContainsAllKnownJobsTestConstantValuesfor newly introduced job-name constants