Skip to content

Commit 26d151f

Browse files
Copilotpelikhangithub-actions[bot]claudegh-aw-bot
authored
Add missing safe-output configuration schema coverage (#53659)
* Initial plan * Add safe output schema coverage Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> * docs(adr): add draft ADR-53659 for safe-output config schema coverage Draft ADR documenting the decision to add missing safe-output configuration fields to the workflow JSON schema and allowlist compiler-populated fields in the IMP-004 conformance checker. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Correct comment-memory schema placement Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> * Restrict comment-memory to tools configuration Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
1 parent fede203 commit 26d151f

25 files changed

Lines changed: 263 additions & 116 deletions
Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
# ADR-53659: Add Missing Safe-Output Config Fields to JSON Schema
2+
3+
**Date**: 2026-08-18
4+
**Status**: Draft
5+
**Deciders**: Unknown
6+
7+
---
8+
9+
### Context
10+
11+
The workflow JSON schema at `pkg/parser/schemas/main_workflow_schema.json` defines `safe-outputs.*` sub-schemas that control what configuration fields workflow authors may set. The Go implementation in `pkg/workflow/` had already grown YAML-tagged config fields across six safe-output types (`add-comment`, `assign-milestone`, `create-issue`, `create-pull-request`, `push-to-pull-request-branch`, `threat-detection`) that were absent from the schema. The daily conformance checker (IMP-004 / `check_safe_output_config_schema_coverage`) surfaces this as a spec violation: because the schema does not set `additionalProperties: false` for these sections, there is no hard validation failure at parse time, but editor autocomplete, type checking, and docs-generation tooling are blind to the missing fields. Three flagged fields (`call-workflow.workflow_files`, `dispatch-workflow.workflow_files`, `dispatch-workflow.aw_context_workflows`) are compiler-populated internals and must be excluded from the user-facing schema rather than documented in it. `comment-memory` is configured under `tools`, not `safe-outputs`, and is excluded from the conformance check.
12+
13+
### Decision
14+
15+
We will add all user-facing safe-output configuration fields to `main_workflow_schema.json` under their respective `safe-outputs.*` sub-schemas, matching the Go types and semantics. `comment-memory` remains available only under `tools`, where it is parsed by the compiler. For the 3 compiler-populated fields, we will add an explicit `compiler_populated_fields` allowlist to `scripts/check-safe-outputs-conformance.sh`; a separate `tool_configured_outputs` set excludes `comment-memory`. The primary driver is IMP-004 conformance and closing the DX gap for workflow authors.
16+
17+
### Alternatives Considered
18+
19+
#### Alternative 1: Auto-generate the JSON schema from Go struct `yaml` tags
20+
21+
Generate `main_workflow_schema.json` entries automatically from Go struct reflection or `go generate` tooling, eliminating manual drift. This would make schema and Go implementation structurally impossible to diverge. It was not chosen because it requires building a code-generation pipeline and the schema also contains human-authored descriptions, constraints, and `anyOf`/`oneOf` wrappers that go beyond what struct tags can express automatically; the investment was not justified for this incremental fix.
22+
23+
#### Alternative 2: Annotate compiler-populated fields with a skip marker on Go structs
24+
25+
Add a `schema:"-"` tag (or equivalent) to the three compiler-populated Go struct fields and update the conformance checker to honour the annotation, rather than maintaining a hardcoded allowlist in the shell script. This would be more self-documenting at the field level. It was not chosen because it requires changing the Go struct definitions and establishing a new annotation convention, while the hardcoded allowlist in the script is simpler for the immediate fix; the three fields are stable and unlikely to change.
26+
27+
### Consequences
28+
29+
#### Positive
30+
- Workflow authors gain full editor autocomplete, type checking, and schema-based documentation for the previously undocumented user-facing safe-output config fields.
31+
- IMP-004 conformance check now passes cleanly; the explicit `compiler_populated_fields` allowlist distinguishes internal-only fields from user-authored ones, preventing false positives on future runs.
32+
33+
#### Negative
34+
- The JSON schema must continue to be manually maintained in sync with Go struct changes; any future new YAML-tagged field in a Go safe-output config struct requires a coordinated schema update to avoid regressing IMP-004.
35+
- The `compiler_populated_fields` set in `check-safe-outputs-conformance.sh` is a separate maintenance artifact: if compiler-populated fields are renamed or added in Go, the allowlist must be updated in lockstep.
36+
37+
#### Neutral
38+
- No change to the Go parsing or validation logic; existing workflow files that already use these fields continue to compile and run identically.
39+
- The conformance checker script is now slightly more complex (a set lookup before the `missing` append), but the logic remains easy to follow.
40+
41+
---
42+
43+
*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*

pkg/parser/schema_safe_outputs_target_test.go

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,60 @@ import (
66
"testing"
77
)
88

9+
func TestMainWorkflowSchema_SafeOutputConfigCoverage(t *testing.T) {
10+
t.Parallel()
11+
12+
frontmatter := map[string]any{
13+
"on": "push",
14+
"engine": "copilot",
15+
"safe-outputs": map[string]any{
16+
"add-comment": map[string]any{
17+
"allows-comment-ids": []any{"IC_kwDOABCD123456"},
18+
"hide-older-comments-match": []any{"workflow-id"},
19+
},
20+
"assign-milestone": map[string]any{
21+
"auto_create": true,
22+
},
23+
"create-issue": map[string]any{
24+
"require-temporary-id": true,
25+
},
26+
"create-pull-request": map[string]any{
27+
"require-temporary-id": true,
28+
},
29+
"push-to-pull-request-branch": map[string]any{
30+
"base-branch": "main",
31+
},
32+
"threat-detection": map[string]any{
33+
"engine-config": "copilot",
34+
"environment": "production",
35+
"model": "gpt-5",
36+
},
37+
},
38+
}
39+
40+
if err := ValidateMainWorkflowFrontmatterWithSchemaAndLocation(frontmatter, "/tmp/gh-aw/safe-output-config-coverage-test.md"); err != nil {
41+
t.Fatalf("expected safe-output configuration fields to pass schema validation, got: %v", err)
42+
}
43+
}
44+
45+
func TestMainWorkflowSchema_SafeOutputsRejectsCommentMemory(t *testing.T) {
46+
t.Parallel()
47+
48+
frontmatter := map[string]any{
49+
"on": "push",
50+
"engine": "copilot",
51+
"safe-outputs": map[string]any{
52+
"comment-memory": map[string]any{
53+
"footer": true,
54+
},
55+
},
56+
}
57+
58+
if err := ValidateMainWorkflowFrontmatterWithSchemaAndLocation(frontmatter, "/tmp/gh-aw/safe-output-comment-memory-test.md"); err == nil {
59+
t.Fatal("expected safe-outputs.comment-memory to fail schema validation")
60+
}
61+
}
62+
963
// TestMainWorkflowSchema_SafeOutputsTargetProperties validates that safe output
1064
// types which support target/target-repo/allowed-repos in the Go code also accept
1165
// those properties in the JSON schema. This is a regression test for cases where

pkg/parser/schemas/main_workflow_schema.json

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5474,6 +5474,10 @@
54745474
"type": "string",
54755475
"description": "Optional prefix to add to the beginning of the issue title (e.g., '[ai] ' or '[analysis] ')"
54765476
},
5477+
"require-temporary-id": {
5478+
"type": "boolean",
5479+
"description": "Require create_issue tool calls to include a temporary_id."
5480+
},
54775481
"labels": {
54785482
"type": "array",
54795483
"description": "Optional list of labels to automatically attach to created issues (e.g., ['automation', 'ai-generated'])",
@@ -7074,6 +7078,20 @@
70747078
}
70757079
]
70767080
},
7081+
"allows-comment-ids": {
7082+
"type": "array",
7083+
"description": "Trusted allowlist of issue or pull request comment IDs the agent may update when target is '*'.",
7084+
"items": {
7085+
"type": "string"
7086+
}
7087+
},
7088+
"hide-older-comments-match": {
7089+
"type": "array",
7090+
"description": "Exact workflow IDs whose older comments are eligible to be hidden.",
7091+
"items": {
7092+
"type": "string"
7093+
}
7094+
},
70777095
"allowed-reasons": {
70787096
"type": "array",
70797097
"description": "List of allowed reasons for hiding older comments when hide-older-comments is enabled. Default: all reasons allowed (spam, abuse, off_topic, outdated, resolved, low_quality).",
@@ -7191,6 +7209,10 @@
71917209
"type": "string",
71927210
"description": "Optional prefix to prepend to the pull request branch name (e.g. \"signed/\"). Applied before the agent-specified or auto-generated branch name."
71937211
},
7212+
"require-temporary-id": {
7213+
"type": "boolean",
7214+
"description": "Require create_pull_request tool calls to include a temporary_id."
7215+
},
71947216
"pre-create": {
71957217
"type": "boolean",
71967218
"description": "\u26a0\ufe0f Experimental. Pre-create a draft pull request during activation, check out its branch in the agent job, and reuse it when processing create_pull_request output. This value is compile-time only and cannot be templated. Using this field emits a compile-time warning.",
@@ -8684,6 +8706,10 @@
86848706
"minItems": 1,
86858707
"maxItems": 50
86868708
},
8709+
"auto_create": {
8710+
"type": "boolean",
8711+
"description": "Automatically create missing milestones from the allowed list."
8712+
},
86878713
"max": {
86888714
"description": "Optional maximum number of milestone assignments (default: 1) Supports integer or GitHub Actions expression (e.g. '${{ inputs.max }}').",
86898715
"oneOf": [
@@ -9551,6 +9577,10 @@
95519577
"type": "string",
95529578
"description": "The branch to push changes to (defaults to 'triggering')"
95539579
},
9580+
"base-branch": {
9581+
"type": "string",
9582+
"description": "Base branch of the target repository for incremental patch computation. Defaults to the local checkout branch or the repository default branch."
9583+
},
95549584
"target": {
95559585
"type": "string",
95569586
"description": "Target for push operations: 'triggering' (default), '*' (any pull request), or explicit pull request number"
@@ -10942,6 +10972,14 @@
1094210972
}
1094310973
]
1094410974
},
10975+
"engine-config": {
10976+
"$ref": "#/$defs/engine_config",
10977+
"description": "Extended engine configuration for threat detection."
10978+
},
10979+
"model": {
10980+
"type": "string",
10981+
"description": "Model override for threat detection engine execution."
10982+
},
1094510983
"steps": {
1094610984
"type": "array",
1094710985
"description": "Array of extra job steps to run before engine execution",
@@ -10972,6 +11010,10 @@
1097211010
}
1097311011
]
1097411012
},
11013+
"environment": {
11014+
"type": "string",
11015+
"description": "GitHub Actions environment override for the detection job."
11016+
},
1097511017
"continue-on-error": {
1097611018
"$ref": "#/$defs/templatable_boolean",
1097711019
"description": "When true (default), detection failures produce warnings and allow safe outputs to proceed with a caution notice and 'needs-review' label. When false, detection failures block safe outputs entirely. Accepts a boolean literal or a GitHub Actions expression."

pkg/workflow/comment_memory.go

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import "github.com/github/gh-aw/pkg/logger"
44

55
var commentMemoryLog = logger.New("workflow:comment_memory")
66

7-
// CommentMemoryConfig holds configuration for the comment_memory safe output type.
7+
// CommentMemoryConfig holds parsed tools.comment-memory configuration.
88
type CommentMemoryConfig struct {
99
BaseSafeOutputConfig `yaml:",inline"`
1010
Target string `yaml:"target,omitempty"` // Target: "triggering" (default), "*" or explicit issue/PR number
@@ -14,6 +14,24 @@ type CommentMemoryConfig struct {
1414
Footer *string `yaml:"footer,omitempty"` // Footer visibility control ("true"/"false" templatable string); nil defaults to visible footer
1515
}
1616

17+
const commentMemoryHandlerKey = "comment_memory"
18+
19+
func buildCommentMemoryHandlerConfig(config *CommentMemoryConfig, globalFooter *bool) map[string]any {
20+
if config == nil {
21+
return nil
22+
}
23+
return newHandlerConfigBuilder().
24+
AddTemplatableInt("max", config.Max).
25+
AddIfNotEmpty("target", config.Target).
26+
AddIfNotEmpty("target-repo", config.TargetRepoSlug).
27+
AddStringSlice("allowed_repos", config.AllowedRepos).
28+
AddIfNotEmpty("memory_id", config.MemoryID).
29+
AddTemplatableBool("footer", getEffectiveFooterForTemplatable(config.Footer, globalFooter)).
30+
AddIfNotEmpty("github-token", resolveHandlerGitHubTokenWithStepID(config.GitHubApp, "comment-memory-app-token", config.GitHubToken)).
31+
AddTemplatableBool("staged", templatableBoolPtrToStringPtr(config.Staged)).
32+
Build()
33+
}
34+
1735
// extractCommentMemoryConfig extracts comment-memory configuration from tools section.
1836
func (c *Compiler) extractCommentMemoryConfig(toolsConfig *ToolsConfig) *CommentMemoryConfig {
1937
if toolsConfig == nil || toolsConfig.CommentMemory == nil {

pkg/workflow/comment_memory_config_test.go

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -91,13 +91,11 @@ permissions:
9191
compiler := NewCompiler(WithVersion("1.0.0"))
9292
workflowData, err := compiler.ParseWorkflowFile(testFile)
9393
require.NoError(t, err, "Failed to parse workflow")
94-
require.NotNil(t, workflowData.SafeOutputs, "SafeOutputs should be present")
95-
9694
if tt.expectedCommentMemory != nil {
97-
require.NotNil(t, workflowData.SafeOutputs.CommentMemory, "CommentMemory should be enabled")
98-
assert.Equal(t, tt.expectedCommentMemory, workflowData.SafeOutputs.CommentMemory)
95+
require.NotNil(t, workflowData.CommentMemoryConfig, "CommentMemory should be enabled")
96+
assert.Equal(t, tt.expectedCommentMemory, workflowData.CommentMemoryConfig)
9997
} else {
100-
assert.Nil(t, workflowData.SafeOutputs.CommentMemory, "CommentMemory should be disabled")
98+
assert.Nil(t, workflowData.CommentMemoryConfig, "CommentMemory should be disabled")
10199
}
102100
})
103101
}

pkg/workflow/compiler_custom_job_memory.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ func extractRestoreMemoryConfig(configMap map[string]any, jobName string, data *
4040
cfg := &restoreMemoryConfig{
4141
CacheMemory: data.CacheMemoryConfig != nil && len(data.CacheMemoryConfig.Caches) > 0,
4242
RepoMemory: data.RepoMemoryConfig != nil && len(data.RepoMemoryConfig.Memories) > 0,
43-
CommentMemory: data.SafeOutputs != nil && data.SafeOutputs.CommentMemory != nil,
43+
CommentMemory: data.CommentMemoryConfig != nil,
4444
}
4545

4646
if !cfg.CacheMemory && !cfg.RepoMemory && !cfg.CommentMemory {
@@ -175,7 +175,7 @@ func generateRepoMemoryRestoreLines(data *WorkflowData) []string {
175175
// for a custom job. The step fetches the comment-memory content from GitHub and
176176
// materialises it as local files — the same operation performed in the agent job.
177177
func generateCommentMemoryRestoreLines(data *WorkflowData) []string {
178-
if data.SafeOutputs == nil || data.SafeOutputs.CommentMemory == nil {
178+
if data.CommentMemoryConfig == nil {
179179
return nil
180180
}
181181

@@ -184,7 +184,7 @@ func generateCommentMemoryRestoreLines(data *WorkflowData) []string {
184184
lines = append(lines, " - name: Prepare comment memory files\n")
185185
lines = append(lines, fmt.Sprintf(" uses: %s\n", getCachedActionPin("actions/github-script", data)))
186186
lines = append(lines, " with:\n")
187-
lines = append(lines, fmt.Sprintf(" github-token: %s\n", getEffectiveSafeOutputGitHubToken(data.SafeOutputs.CommentMemory.GitHubToken)))
187+
lines = append(lines, fmt.Sprintf(" github-token: %s\n", getEffectiveSafeOutputGitHubToken(data.CommentMemoryConfig.GitHubToken)))
188188
lines = append(lines, " script: |\n")
189189
lines = append(lines, " const { setupGlobals } = require('${{ runner.temp }}/gh-aw/actions/setup_globals.cjs');\n")
190190
lines = append(lines, " setupGlobals(core, github, context, exec, io, getOctokit);\n")

pkg/workflow/compiler_custom_job_memory_test.go

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -524,9 +524,7 @@ func TestExtractRestoreMemoryConfig(t *testing.T) {
524524
RepoMemoryConfig: &RepoMemoryConfig{
525525
Memories: []RepoMemoryEntry{{ID: "default"}},
526526
},
527-
SafeOutputs: &SafeOutputsConfig{
528-
CommentMemory: &CommentMemoryConfig{},
529-
},
527+
CommentMemoryConfig: &CommentMemoryConfig{},
530528
}
531529
emptyData := &WorkflowData{}
532530

pkg/workflow/compiler_orchestrator_workflow.go

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -459,15 +459,8 @@ func (c *Compiler) extractAdditionalConfigurations(
459459
// Use the already extracted output configuration
460460
workflowData.SafeOutputs = safeOutputs
461461

462-
// Extract comment-memory from tools and attach to safe-outputs configuration.
463-
// comment-memory now belongs under tools: next to cache-memory and repo-memory.
464-
commentMemoryConfig := c.extractCommentMemoryConfig(toolsConfig)
465-
if commentMemoryConfig != nil {
466-
if workflowData.SafeOutputs == nil {
467-
workflowData.SafeOutputs = &SafeOutputsConfig{}
468-
}
469-
workflowData.SafeOutputs.CommentMemory = commentMemoryConfig
470-
}
462+
// comment-memory belongs under tools: next to cache-memory and repo-memory.
463+
workflowData.CommentMemoryConfig = c.extractCommentMemoryConfig(toolsConfig)
471464

472465
// Extract mcp-scripts configuration
473466
workflowData.MCPScripts = c.extractMCPScriptsConfig(frontmatter)

pkg/workflow/compiler_pre_activation_job.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -266,14 +266,14 @@ func (c *Compiler) buildPreActivationMemoryRestoreSteps(data *WorkflowData, step
266266
steps = append(steps, repoMemorySteps.String())
267267
}
268268

269-
if data.SafeOutputs != nil && data.SafeOutputs.CommentMemory != nil {
269+
if data.CommentMemoryConfig != nil {
270270
if configLines, ok := c.generateCommentMemoryEarlyConfigLines(data); ok {
271271
steps = append(steps, strings.Join(configLines, ""))
272272
var commentMemorySteps strings.Builder
273273
commentMemorySteps.WriteString(" - name: Prepare comment memory files\n")
274274
fmt.Fprintf(&commentMemorySteps, " uses: %s\n", getCachedActionPin("actions/github-script", data))
275275
commentMemorySteps.WriteString(" with:\n")
276-
fmt.Fprintf(&commentMemorySteps, " github-token: %s\n", getEffectiveSafeOutputGitHubToken(data.SafeOutputs.CommentMemory.GitHubToken))
276+
fmt.Fprintf(&commentMemorySteps, " github-token: %s\n", getEffectiveSafeOutputGitHubToken(data.CommentMemoryConfig.GitHubToken))
277277
commentMemorySteps.WriteString(" script: |\n")
278278
commentMemorySteps.WriteString(" const { setupGlobals } = require('${{ runner.temp }}/gh-aw/actions/setup_globals.cjs');\n")
279279
commentMemorySteps.WriteString(" setupGlobals(core, github, context, exec, io, getOctokit);\n")

pkg/workflow/compiler_safe_outputs_config_test.go

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ func TestAddHandlerManagerConfigEnvVar(t *testing.T) {
2121
tests := []struct {
2222
name string
2323
safeOutputs *SafeOutputsConfig
24+
commentMemory *CommentMemoryConfig
2425
checkContains []string
2526
checkJSON bool
2627
expectedKeys []string
@@ -855,14 +856,13 @@ func TestAddHandlerManagerConfigEnvVar(t *testing.T) {
855856
expectedKeys: []string{"create_check_run"},
856857
},
857858
{
858-
name: "comment_memory config",
859-
safeOutputs: &SafeOutputsConfig{
860-
CommentMemory: &CommentMemoryConfig{
861-
BaseSafeOutputConfig: BaseSafeOutputConfig{
862-
Max: strPtr("1"),
863-
},
864-
MemoryID: "test-memory",
859+
name: "comment_memory config",
860+
safeOutputs: &SafeOutputsConfig{},
861+
commentMemory: &CommentMemoryConfig{
862+
BaseSafeOutputConfig: BaseSafeOutputConfig{
863+
Max: strPtr("1"),
865864
},
865+
MemoryID: "test-memory",
866866
},
867867
checkContains: []string{
868868
"GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG",
@@ -925,8 +925,9 @@ func TestAddHandlerManagerConfigEnvVar(t *testing.T) {
925925
compiler := NewCompiler()
926926

927927
workflowData := &WorkflowData{
928-
Name: "Test Workflow",
929-
SafeOutputs: tt.safeOutputs,
928+
Name: "Test Workflow",
929+
SafeOutputs: tt.safeOutputs,
930+
CommentMemoryConfig: tt.commentMemory,
930931
}
931932

932933
var steps []string

0 commit comments

Comments
 (0)