Repository navigation
Support create-issue: false to disable automatic issue output - #69369
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
create-issue: false to disable automatic issue output
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Ponytail Reviewer completed successfully! Ponytail ReviewerOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Commit pushed:
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (146 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 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
|
Comment MemoryPeek at saved memory (pr-code-quality-reviewer)Note This comment is managed by comment memory.Expand the saved memory block to view or edit the persistent context for this thread.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
No actionable blocking issues found in the diff.
Scope
Reviewed the create-issue opt-out flow across schema validation, config extraction, import merge precedence, default injection, tests, and docs. The behavior looks consistent and the added coverage hits the key cases.🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 85.6 AIC · ⌖ 5.91 AIC · ⊞ 12.5K
Comment /review to run again
| // instruction for the agent. This aligns create-issue with the other builtin safe outputs | ||
| // (noop, missing-tool, missing-data) that are always available. | ||
| func applyDefaultCreateIssue(workflowData *WorkflowData) { | ||
| if workflowData.SafeOutputs != nil && workflowData.SafeOutputs.CreateIssuesDisabled { |
There was a problem hiding this comment.
create-issue: false is not a complete opt-out here: it skips the auto-injected create_issue tool, but the implicit report-incomplete.create-issue default is still true, so these workflows can still publish GitHub issues on incomplete runs.
💡 Why this blocks the feature contract
The PR promises a way for workflows to disable issue publication entirely, especially for workflows that must not open issues. Right now `extractSafeOutputsConfig` still auto-defaults `report-incomplete.create-issue` from `defaultReportIncompleteCreateIssue(config)`, and nothing in this new early return overrides that when `CreateIssuesDisabled` is set. That leaves a second issue-creation path enabled and can still require `issues: write` unexpectedly.A minimal fix is to force the implicit report-incomplete default to false whenever create-issue: false is set, unless the workflow explicitly sets report-incomplete.create-issue itself. For example:
if _, exists := outputMap["report-incomplete"]; !exists {
value := defaultReportIncompleteCreateIssue(config)
if config.CreateIssuesDisabled {
value = "false"
}
config.ReportIncomplete = &ReportIncompleteConfig{
CreateIssue: &value,
TitlePrefix: "",
Labels: nil,
Implicit: true,
}
}That keeps the explicit report-incomplete: configuration as the override while making the new opt-out actually close the implicit issue path.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — found one correctness regression that needs a fix before merge; everything else is well covered.
📋 Key Themes & Highlights
Key Themes
- Early-return ordering bug in
applyDefaultCreateIssue(pkg/workflow/safe_outputs_state.go:198): the newCreateIssuesDisabledcheck returns before the repo-memory ledger block runs, socreate-issue: falsecombined with a configured repo-memory ledger silently dropsThreatDetection/NoOpsetup that previously always ran. Verified with a standalone reproduction test. See inline comment for a suggested fix and the missing regression test case.
Positive Highlights
- ✅ Thorough test coverage for the main opt-out path: schema validation (
true/string/zero-maxcorrectly rejected), auto-injection suppression, import precedence (main overrides import, import-only disables, explicit re-enable via{}ornull), and compiled lock-file assertions thatcreate_issuetooling/prompt text are actually absent. - ✅ Clean, minimal schema addition (
const: false) consistent with existingoneOfpattern for other nullable/boolean safe-output fields. - ✅ Docs update correctly describes the new opt-out and clarifies the auto-injection caveat.
Note: investigation was bounded to the pre-fetched diff plus targeted reads of applyDefaultCreateIssue, mergeSafeOutputConfig, and the repo-memory ledger helpers — no open-ended tracing was needed given the diff size.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 116 AIC · ⌖ 15 AIC · ⊞ 10.1K
Comment /matt to run again
| // instruction for the agent. This aligns create-issue with the other builtin safe outputs | ||
| // (noop, missing-tool, missing-data) that are always available. | ||
| func applyDefaultCreateIssue(workflowData *WorkflowData) { | ||
| if workflowData.SafeOutputs != nil && workflowData.SafeOutputs.CreateIssuesDisabled { |
There was a problem hiding this comment.
[/diagnosing-bugs] Returning early here also skips the repo-memory ledger setup (ThreatDetection + NoOp) a few lines below, which is unrelated to create-issue and previously always ran. A workflow with create-issue: false and a repo-memory ledger configured will silently lose its ledger audit wiring.
💡 Repro and suggested fix
Confirmed with a standalone test: calling applyDefaultCreateIssue on a WorkflowData with SafeOutputs.CreateIssuesDisabled: true and a RepoMemoryConfig containing a Ledger entry leaves ThreatDetection and NoOp both nil after the call, whereas before this PR they were always populated when a ledger was configured.
Fix: move the CreateIssuesDisabled check so it only short-circuits the create-issue injection block, not the repo-memory ledger block, e.g.:
func applyDefaultCreateIssue(workflowData *WorkflowData) {
if workflowData.CommentMemoryConfig != nil || hasNonBuiltinSafeOutputsEnabled(workflowData.SafeOutputs) {
return
}
if repoMemoryLedgerEnabled(workflowData.RepoMemoryConfig) {
// ... existing ledger setup, unaffected by CreateIssuesDisabled
return
}
if workflowData.SafeOutputs != nil && workflowData.SafeOutputs.CreateIssuesDisabled {
return
}
// ... existing create-issue injection
}Please add a regression test combining create-issue: false with a configured repo-memory ledger to lock in the expected ThreatDetection/NoOp setup.
@copilot please address this.
There was a problem hiding this comment.
🟡 Changes recommended
The opt-out can bypass required repo-memory ledger audit setup when all builtin outputs are disabled.
1 open finding
What changed in this PR
Adds create-issue: false to gh-aw workflows so authors can disable automatic issue output.
Changes:
- Supports explicit disablement of the issue tool and reporting instruction.
- Preserves import precedence and existing defaults.
- Adds regression coverage and documents the opt-out.
| File | Description |
|---|---|
pkg/workflow/safe_outputs_state.go |
Skips automatic issue injection when disabled. |
pkg/workflow/safe_outputs_default_create_issue_test.go |
Tests suppression, builtins, and import precedence. |
pkg/workflow/safe_outputs_config_types.go |
Tracks explicit disablement. |
pkg/workflow/safe_outputs_config_extraction.go |
Extracts the disablement flag. |
pkg/workflow/imports.go |
Preserves imported disablement. |
pkg/workflow/create_issue.go |
Handles false during parsing. |
pkg/parser/schemas/main_workflow_schema.json |
Accepts explicit false. |
pkg/parser/schema_safe_outputs_target_test.go |
Tests accepted and rejected configurations. |
docs/src/content/docs/reference/safe-outputs.md |
Documents the supported opt-out. |
🧠 Review effort: Max
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| if workflowData.SafeOutputs != nil && workflowData.SafeOutputs.CreateIssuesDisabled { | ||
| return | ||
| } |
Workflows with only builtin safe outputs automatically receive a
create_issuetool and an instruction to use it, with no explicit opt-out. This can contradict workflows that must not publish issues.false; suppress both the tool and automatic reporting instruction.