Add maintenance.disabled_jobs to selectively omit agentics-maintenance jobs and permissions#43300
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
maintenance.disabled_jobs to selectively omit agentics-maintenance jobs and permissions
|
@copilot each job should have a minimal set of permissions write based on the required task |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Updated in |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
|
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Adds a repo-level maintenance.disabled_jobs config surface to allow selectively omitting specific jobs (and related triggers/outputs) from the generated agentics-maintenance.yml, so the workflow’s permissions align with actual repo usage.
Changes:
- Extend
aw.jsonmaintenance config + JSON schema to acceptmaintenance.disabled_jobswith job-name normalization (case-insensitive;_and-equivalent). - Update maintenance workflow generation to omit disabled jobs, suppress the
issues:labeledtrigger when no label-triggered jobs remain, and avoid output references to removed jobs. - Add/adjust tests for config parsing and workflow omission behavior.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/repo_config.go | Adds MaintenanceConfig.DisabledJobs plus normalization + IsJobDisabled helper for config-driven job omission. |
| pkg/workflow/repo_config_test.go | Adds parsing/normalization test coverage for maintenance.disabled_jobs. |
| pkg/workflow/maintenance_workflow.go | Threads normalized disabled-job set into workflow YAML generation. |
| pkg/workflow/maintenance_workflow_yaml.go | Implements job omission, conditional label trigger emission, and workflow_call output fallback when apply_safe_outputs is disabled. |
| pkg/workflow/maintenance_workflow_test.go | Adds/updates tests verifying omitted jobs, permissions shaping, trigger suppression, and output fallback. |
| pkg/parser/schemas/repo_config_schema.json | Updates schema to include maintenance.disabled_jobs. |
| docs/src/content/docs/reference/ephemerals.md | Documents maintenance.disabled_jobs and provides example configuration. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Low
Documents the decision to split close-expired-entities into three scoped jobs and add maintenance.disabled_jobs config for selective job omission.
Design Decision Gate - ADR RequiredThis PR makes significant changes to core business logic (234 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 MatterADRs 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
|
🧪 Test Quality Sentinel Report
📊 Metrics (5 tests)
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design, /tdd, /grill-with-docs, and /domain-modeling — requesting changes on a few design and test-coverage issues.
📋 Key Themes & Highlights
Key Themes
- Dual implementation of the same logic:
IsJobDisabled(exported, onMaintenanceConfig) andisMaintenanceJobDisabled(package-private, in the YAML builder) both callnormalizeMaintenanceJobNameand do the same lookup. One of them is unused in production paths and should either become the canonical API or be removed. - Virtual alias leaking into the public API:
close-expired-entitiesis a grouping shorthand — it's not an actual YAML job ID. Users copying a job name from the generated workflow will not find it, leading to silent misconfiguration. The docs and schema need to enumerate recognized values. - Missing edge-case tests: partial label-job disablement (disable one, keep the other) and normalized-duplicate handling in
disabled_jobsare not covered by the new tests.
Positive Highlights
- ✅ Excellent refactor: the three close-expired jobs moved from one monolithic block to a
writeCloseExpiredJobhelper, each with its own minimal permission scope — a significant security improvement. - ✅ The
issues:labeledtrigger is now gated per surviving job rather than the coarser!disableLabelTriggerflag — correct and precise. - ✅ The
applied_run_urlfallback toinputs.run_urlwhenapply_safe_outputsis omitted is a nice defensive touch that avoids a workflow-call failure. - ✅ Schema, struct, test, and docs are all updated together — well-rounded change.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 90.7 AIC · ⌖ 5.87 AIC · ⊞ 6.6K
Comment /matt to run again
|
|
||
| // IsJobDisabled reports whether the provided maintenance job ID is explicitly | ||
| // disabled in aw.json. | ||
| func (m *MaintenanceConfig) IsJobDisabled(jobName string) bool { |
There was a problem hiding this comment.
[/codebase-design] IsJobDisabled is exported but not called from production code — the actual disable-check path uses the map[string]struct{} built in maintenance_workflow.go + isMaintenanceJobDisabled in maintenance_workflow_yaml.go. This leaves two parallel implementations of the same lookup logic.
💡 Consolidation options
Option A — make IsJobDisabled the single source of truth and remove the map-building loop + package-level isMaintenanceJobDisabled. The linear scan is fine for an expected list of ≤10 entries.
Option B — if the map is preferred for clarity or future performance, remove the unused IsJobDisabled method to avoid misleading future maintainers about the real code path.
Either way, normalizeMaintenanceJobName should live in exactly one place and both callers should use it.
@copilot please address this.
| return *m.LabelTriggers | ||
| } | ||
|
|
||
| func normalizeMaintenanceJobName(name string) string { |
There was a problem hiding this comment.
[/codebase-design] normalizeMaintenanceJobName lives in repo_config.go but is also called from maintenance_workflow.go and maintenance_workflow_yaml.go, making it an implicit cross-package coupling. It's an unexported helper that belongs to the module it serves.
💡 Suggested refactor
Move normalizeMaintenanceJobName next to isMaintenanceJobDisabled in maintenance_workflow_yaml.go (or a shared internal helper file), so that all normalization logic is co-located with its only real callsite. Alternatively, if IsJobDisabled is the canonical public API, keep normalizeMaintenanceJobName private inside repo_config.go and remove the inlined call in maintenance_workflow.go by delegating to IsJobDisabled.
@copilot please address this.
| "type": "string", | ||
| "minLength": 1 | ||
| }, | ||
| "uniqueItems": true |
There was a problem hiding this comment.
[/tdd] The schema has uniqueItems: true but there is no test asserting that a duplicate in disabled_jobs is rejected at parse time. If the JSON schema validator silently de-duplicates or if validation is not enforced at runtime, a user could write ["close-expired-entities", "close_expired_entities"] (one hyphen, one underscore) and get two semantically-identical but syntactically-unique entries that both pass schema validation while the normalisation in Go collapses them to one.
💡 Suggested test
Add a case to TestLoadRepoConfig_DisabledJobs (or a sibling) that checks both:
- A literal duplicate (same string twice) is rejected by the schema validator.
- A normalisation-equivalent pair like
["close-expired-entities", "close_expired_entities"]produces the expected set (one entry) without error — or alternatively is also rejected.
This makes the contract explicit regardless of which validator is in use.
@copilot please address this.
| const { main } = require('${{ runner.temp }}/gh-aw/actions/close_expired_pull_requests.cjs'); | ||
| await main(); | ||
| `) | ||
| if !isMaintenanceJobDisabled(disabledJobs, "close-expired-entities") { |
There was a problem hiding this comment.
[/grill-with-docs] The alias close-expired-entities is a virtual name — it does not correspond to any actual generated job. The generated jobs are close-expired-discussions, close-expired-issues, and close-expired-pull-requests. Using an alias that doesn't appear in the generated YAML is a leaky abstraction: users who copy a job name from the workflow output will get it wrong.
💡 Alternatives to consider
- Keep the alias, but document it clearly — add a comment block in the generated YAML (or in the docs) listing the valid job IDs, including the alias, so users know
close-expired-entitiesis a shorthand. - Remove the alias — accept only the concrete job names (
close-expired-discussions, etc.) individually. Users then have fine-grained control and no confusion from an invisible name. - Validate unknown job names — if a user passes a
disabled_jobsvalue that doesn't match any recognized job, log a warning or return an error so misconfigurations surface immediately.
The current approach silently accepts unknown strings with no feedback to the user.
@copilot please address this.
| t.Error("label_apply_safe_outputs job should be omitted when disabled in aw.json") | ||
| } | ||
| if strings.Contains(yaml, " issues:\n types: [labeled]") { | ||
| t.Error("issues:labeled trigger should be omitted when all label-triggered jobs are disabled") |
There was a problem hiding this comment.
[/tdd] TestGenerateMaintenanceWorkflow_DisabledJobs only tests the all-or-nothing case (all label-triggered jobs disabled). There is no test for the partial case: disabling just one of label_disable_agentic_workflow or label_apply_safe_outputs while keeping the other. In that scenario the issues:labeled trigger should still be emitted for the surviving job.
💡 Missing test case sketch
Disable only label_disable_agentic_workflow while LabelTriggers: true is set, then assert:
issues:labeledtrigger is still present (surviving job needs it)label_disable_agentic_workflow:job block is absentlabel_apply_safe_outputs:job block is present
This covers the important edge: the trigger suppression logic in labelDisableJobEnabled || labelApplySafeOutputsJobEnabled needs a test to confirm it doesn't over-suppress.
@copilot please address this.
| } | ||
|
|
||
| appliedRunURLValue := "${{ jobs.apply_safe_outputs.outputs.run_url }}" | ||
| if isMaintenanceJobDisabled(disabledJobs, "apply_safe_outputs") { |
There was a problem hiding this comment.
[/tdd] When apply_safe_outputs is disabled, applied_run_url falls back to ${{ inputs.run_url }}. The test at line 1194 checks this via a raw string comparison, but there is no test ensuring the generated YAML is actually valid and parsable as a GitHub Actions workflow. A malformed value: line would pass the strings.Contains check but break real workflow runs.
💡 Suggested hardening
After generating the YAML, try unmarshaling the workflow_call.outputs block with gopkg.in/yaml.v3 (or any YAML parser already used in the test suite) and assert that applied_run_url.value equals the expected expression string. This prevents a subtle formatting bug (e.g., missing newline before value:) from going undetected.
@copilot please address this.
|
|
||
| // DisabledJobs lists maintenance job IDs that should be omitted from generated | ||
| // agentics-maintenance workflows. | ||
| DisabledJobs []string `json:"disabled_jobs,omitempty"` |
There was a problem hiding this comment.
[/domain-modeling] The field is named DisabledJobs / disabled_jobs but the PR description uses both disabled_jobs and maintenance.disabled_jobs interchangeably, and the referenced values include both real YAML job IDs (apply_safe_outputs) and a virtual alias (close-expired-entities). The domain model mixes two concepts:
- Concrete job identifiers — IDs that actually appear in the generated
agentics-maintenance.yml - Virtual group aliases — shorthand names (
close-expired-entities) that map to multiple concrete jobs
Without a clear distinction in the docs or schema description, users cannot know which names are valid without reading Go source.
💡 Suggestion
Add a JSON schema enum (or reference a $defs block) listing the recognized job IDs — both concrete and aliases — with a note in the description. This makes the contract self-documenting and enables IDE validation/autocompletion for aw.json.
@copilot please address this.
| @@ -138,6 +143,8 @@ The `runs_on` field accepts a single string or an array of strings for multi-lab | |||
|
|
|||
| The `action_failure_issue_expires` field controls expiration (in hours) for failure issues opened by the conclusion job (including grouped parent issues when `group-reports: true`). The default is `168` (7 days). | |||
There was a problem hiding this comment.
[/grill-with-docs] The docs say job IDs are case-insensitive and _/- equivalent, but they don't list the valid job names a user can put in disabled_jobs. Without an exhaustive list (even a short one), users must read source code to know what strings are accepted — or silently misconfigure without any error feedback.
💡 Suggested addition
Add a table or list of recognized job IDs to the docs block, e.g.:
| Job ID | Effect when disabled |
|---|---|
close-expired-entities |
Omits all three close-expired jobs |
apply_safe_outputs |
Omits the apply-safe-outputs dispatch job; applied_run_url falls back to inputs.run_url |
label_disable_agentic_workflow |
Omits the label-triggered disable job |
label_apply_safe_outputs |
Omits the label-triggered apply-safe-outputs job |
Also note what happens when an unrecognised job ID is provided (currently: silently ignored).
@copilot please address this.
There was a problem hiding this comment.
Review: maintenance.disabled_jobs selective job omission
Overall: The feature is well-designed and the refactoring from a monolithic close-expired-entities job to per-entity scoped-permission jobs is a clear win. Tests are comprehensive. Three non-trivial issues need attention before merging.
Blocking issues
-
Duplicate normalization logic (
repo_config.goL134):IsJobDisabledand theisMaintenanceJobDisabledhelper inmaintenance_workflow_yaml.goboth implement the same normalization independently. A future change to normalization rules would need to be applied in both places. -
applied_run_urlfallback scope (maintenance_workflow_yaml.goL113): The fallback to${{ inputs.run_url }}is valid only duringworkflow_call; forscheduleandworkflow_dispatchit produces an empty string. Consumers of this output should be aware of this or it should be documented.
Non-blocking suggestions
- Schema and docs don't enumerate valid job IDs: The
disabled_jobsarray accepts any string with no validation. A typo silently does nothing. Addingenumvalues to the schema items and a table of valid IDs in the docs would significantly improve usability and discoverability.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 113 AIC · ⌖ 6.29 AIC · ⊞ 4.9K
| normalizedJobName := normalizeMaintenanceJobName(jobName) | ||
| for _, disabledJob := range m.DisabledJobs { | ||
| if normalizeMaintenanceJobName(disabledJob) == normalizedJobName { | ||
| return true |
There was a problem hiding this comment.
Duplicate normalization logic — IsJobDisabled (added here) walks DisabledJobs with a linear scan, while maintenance_workflow.go builds a pre-normalised map[string]struct{} at generation time and calls the package-private isMaintenanceJobDisabled helper. The exported IsJobDisabled method is only used by tests, so there are now two parallel implementations of the same logic.
If IsJobDisabled is intended for external callers, the normalization rules are defined in two places. A change to the normalization logic must be applied in both normalizeMaintenanceJobName call-sites. Consider either removing IsJobDisabled if only tests use it (and add a helper that takes the map), or have IsJobDisabled convert to a map and reuse isMaintenanceJobDisabled.
@copilot please address this.
| "type": "array", | ||
| "items": { | ||
| "type": "string", | ||
| "minLength": 1 |
There was a problem hiding this comment.
No enumeration of valid job IDs in schema — The disabled_jobs array accepts any string, but only a fixed set of job IDs are meaningful (close-expired-entities, apply_safe_outputs, label_disable_agentic_workflow, label_apply_safe_outputs). A typo like "apply-safe-outputs" (hyphen instead of underscore) silently has no effect even though normalization maps them equivalently — but "appl_safe_outputs" would be silently ignored with no warning.
Consider adding an enum or examples array to the item schema to help users discover valid values and improve IDE autocompletion.
@copilot please address this.
| @@ -138,6 +143,8 @@ The `runs_on` field accepts a single string or an array of strings for multi-lab | |||
|
|
|||
| The `action_failure_issue_expires` field controls expiration (in hours) for failure issues opened by the conclusion job (including grouped parent issues when `group-reports: true`). The default is `168` (7 days). | |||
|
|
|||
There was a problem hiding this comment.
Docs omit the full list of valid job IDs — The description says the field accepts job IDs and explains normalization, but never lists which IDs are actually recognised. Users need to know the valid values. The PR description lists them (close-expired-entities, apply_safe_outputs, label_disable_agentic_workflow, label_apply_safe_outputs), but that information isn't surfaced in the docs.
Please add a table or inline list of all supported job IDs with a brief description of what each one does so users can make an informed choice.
@copilot please address this.
| appliedRunURLValue := "${{ jobs.apply_safe_outputs.outputs.run_url }}" | ||
| if isMaintenanceJobDisabled(disabledJobs, "apply_safe_outputs") { | ||
| appliedRunURLValue = "${{ inputs.run_url }}" | ||
| } |
There was a problem hiding this comment.
Output fallback bypasses workflow_call consumer expectations — When apply_safe_outputs is disabled, applied_run_url is wired to ${{ inputs.run_url }}. But inputs.run_url is only available when the workflow is called via workflow_call — it is not populated for workflow_dispatch or schedule triggers, where it would resolve to an empty string.
Callers that consume the applied_run_url output expecting a valid URL may get an empty string silently. Consider either: (a) documenting this clearly, or (b) having the output emit the input only in the workflow_call path and omit/skip it otherwise.
@copilot please address this.
🤖 PR Triage
Score breakdown: Impact 38 (new config surface reduces unnecessary write scopes in generated maintenance workflows) + Urgency 18 + Quality 12 (github-actions CHANGES_REQUESTED then DISMISSED, needs author follow-up) Adds
|
|
@copilot please run the
|
Addressed on the current head in |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in |
|
@copilot merge main and fix conflicts |
…maintenance-workflow # Conflicts: # pkg/workflow/github_cli.go # pkg/workflow/github_cli_test.go Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in |
|
@copilot please run the
|
Verified on |
|
🎉 This pull request is included in a new release. Release: |
agentics-maintenance.ymlwas always generated with generic maintenance jobs, which could force unnecessary write scopes (notably PR/discussion permissions) even for issue-triage-only repos. This change adds repo-level control to omit specific maintenance jobs so generated permissions align with actual usage.Config surface: selective maintenance job disablement
maintenance.disabled_jobsto.github/workflows/aw.jsonparsing and schema._and-treated equivalently).Maintenance workflow generation
agentics-maintenance.yml:close-expired-entitiesapply_safe_outputslabel_disable_agentic_workflowlabel_apply_safe_outputsissues:labeledtrigger when all label-triggered jobs are disabled.apply_safe_outputsis disabled,workflow_call.outputs.applied_run_urlnow falls back toinputs.run_url(avoids output references to a removed job).Docs and coverage
maintenance.disabled_jobs.{ "maintenance": { "runs_on": "ubuntu-latest", "disabled_jobs": [ "close-expired-entities", "apply_safe_outputs", "label_apply_safe_outputs" ] } }