Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions docs/adr/53659-add-safe-output-config-fields-to-json-schema.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
# ADR-53659: Add Missing Safe-Output Config Fields to JSON Schema

**Date**: 2026-08-18
**Status**: Draft
**Deciders**: Unknown

---

### Context

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.

### Decision

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.

### Alternatives Considered

#### Alternative 1: Auto-generate the JSON schema from Go struct `yaml` tags

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.

#### Alternative 2: Annotate compiler-populated fields with a skip marker on Go structs

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.

### Consequences

#### Positive
- Workflow authors gain full editor autocomplete, type checking, and schema-based documentation for the previously undocumented user-facing safe-output config fields.
- 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.

#### Negative
- 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.
- 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.

#### Neutral
- No change to the Go parsing or validation logic; existing workflow files that already use these fields continue to compile and run identically.
- The conformance checker script is now slightly more complex (a set lookup before the `missing` append), but the logic remains easy to follow.

---

*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*
54 changes: 54 additions & 0 deletions pkg/parser/schema_safe_outputs_target_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,60 @@ import (
"testing"
)

func TestMainWorkflowSchema_SafeOutputConfigCoverage(t *testing.T) {
t.Parallel()

frontmatter := map[string]any{
"on": "push",
"engine": "copilot",
"safe-outputs": map[string]any{
"add-comment": map[string]any{
"allows-comment-ids": []any{"IC_kwDOABCD123456"},
"hide-older-comments-match": []any{"workflow-id"},
},
"assign-milestone": map[string]any{
"auto_create": true,
},
"create-issue": map[string]any{
"require-temporary-id": true,
},
"create-pull-request": map[string]any{
"require-temporary-id": true,
},
"push-to-pull-request-branch": map[string]any{
"base-branch": "main",
},
"threat-detection": map[string]any{
"engine-config": "copilot",
"environment": "production",
"model": "gpt-5",
},
},
}

if err := ValidateMainWorkflowFrontmatterWithSchemaAndLocation(frontmatter, "/tmp/gh-aw/safe-output-config-coverage-test.md"); err != nil {
t.Fatalf("expected safe-output configuration fields to pass schema validation, got: %v", err)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The new test only validates the happy path. Per the pattern in this file, adding at least one negative case would give this schema regression real value — e.g. an unknown field in assign-milestone or a wrong type for auto_create.

💡 Suggested addition
func TestMainWorkflowSchema_SafeOutputConfigCoverage_InvalidField(t *testing.T) {
	t.Parallel()

	frontmatter := map[string]any{
		"on":     "push",
		"engine": "copilot",
		"safe-outputs": map[string]any{
			"assign-milestone": map[string]any{
				"not_a_real_field": true,
			},
		},
	}

	if err := ValidateMainWorkflowFrontmatterWithSchemaAndLocation(frontmatter, "/tmp/gh-aw/safe-output-config-negative-test.md"); err == nil {
		t.Fatal("expected schema validation to reject unknown field, but got no error")
	}
}

@copilot please address this.

}
}

func TestMainWorkflowSchema_SafeOutputsRejectsCommentMemory(t *testing.T) {
t.Parallel()

frontmatter := map[string]any{
"on": "push",
"engine": "copilot",
"safe-outputs": map[string]any{
"comment-memory": map[string]any{
"footer": true,
},
},
}

if err := ValidateMainWorkflowFrontmatterWithSchemaAndLocation(frontmatter, "/tmp/gh-aw/safe-output-comment-memory-test.md"); err == nil {
t.Fatal("expected safe-outputs.comment-memory to fail schema validation")
}
}

// TestMainWorkflowSchema_SafeOutputsTargetProperties validates that safe output
// types which support target/target-repo/allowed-repos in the Go code also accept
// those properties in the JSON schema. This is a regression test for cases where
Expand Down
42 changes: 42 additions & 0 deletions pkg/parser/schemas/main_workflow_schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -5474,6 +5474,10 @@
"type": "string",
"description": "Optional prefix to add to the beginning of the issue title (e.g., '[ai] ' or '[analysis] ')"
},
"require-temporary-id": {
"type": "boolean",
"description": "Require create_issue tool calls to include a temporary_id."
},
"labels": {
"type": "array",
"description": "Optional list of labels to automatically attach to created issues (e.g., ['automation', 'ai-generated'])",
Expand Down Expand Up @@ -7074,6 +7078,20 @@
}
]
},
"allows-comment-ids": {
"type": "array",
"description": "Trusted allowlist of issue or pull request comment IDs the agent may update when target is '*'.",
"items": {
"type": "string"
}
Comment on lines +7081 to +7086
},
"hide-older-comments-match": {
"type": "array",
"description": "Exact workflow IDs whose older comments are eligible to be hidden.",
"items": {
"type": "string"
}
},
Comment on lines +7088 to +7094
"allowed-reasons": {
"type": "array",
"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).",
Expand Down Expand Up @@ -7191,6 +7209,10 @@
"type": "string",
"description": "Optional prefix to prepend to the pull request branch name (e.g. \"signed/\"). Applied before the agent-specified or auto-generated branch name."
},
"require-temporary-id": {
"type": "boolean",
"description": "Require create_pull_request tool calls to include a temporary_id."
},
"pre-create": {
"type": "boolean",
"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.",
Expand Down Expand Up @@ -8684,6 +8706,10 @@
"minItems": 1,
"maxItems": 50
},
"auto_create": {
"type": "boolean",
"description": "Automatically create missing milestones from the allowed list."
},
"max": {
"description": "Optional maximum number of milestone assignments (default: 1) Supports integer or GitHub Actions expression (e.g. '${{ inputs.max }}').",
"oneOf": [
Expand Down Expand Up @@ -9551,6 +9577,10 @@
"type": "string",
"description": "The branch to push changes to (defaults to 'triggering')"
},
"base-branch": {
"type": "string",
"description": "Base branch of the target repository for incremental patch computation. Defaults to the local checkout branch or the repository default branch."
},
"target": {
"type": "string",
"description": "Target for push operations: 'triggering' (default), '*' (any pull request), or explicit pull request number"
Expand Down Expand Up @@ -10942,6 +10972,14 @@
}
]
},
"engine-config": {
"$ref": "#/$defs/engine_config",
"description": "Extended engine configuration for threat detection."
},
"model": {
"type": "string",
"description": "Model override for threat detection engine execution."
},
Comment on lines +10975 to +10982
"steps": {
"type": "array",
"description": "Array of extra job steps to run before engine execution",
Expand Down Expand Up @@ -10972,6 +11010,10 @@
}
]
},
"environment": {
"type": "string",
"description": "GitHub Actions environment override for the detection job."
},
Comment on lines +11013 to +11016
"continue-on-error": {
"$ref": "#/$defs/templatable_boolean",
"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."
Expand Down
20 changes: 19 additions & 1 deletion pkg/workflow/comment_memory.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import "github.com/github/gh-aw/pkg/logger"

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

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

const commentMemoryHandlerKey = "comment_memory"

func buildCommentMemoryHandlerConfig(config *CommentMemoryConfig, globalFooter *bool) map[string]any {
if config == nil {
return nil
}
return newHandlerConfigBuilder().
AddTemplatableInt("max", config.Max).
AddIfNotEmpty("target", config.Target).
AddIfNotEmpty("target-repo", config.TargetRepoSlug).
AddStringSlice("allowed_repos", config.AllowedRepos).
AddIfNotEmpty("memory_id", config.MemoryID).
AddTemplatableBool("footer", getEffectiveFooterForTemplatable(config.Footer, globalFooter)).
AddIfNotEmpty("github-token", resolveHandlerGitHubTokenWithStepID(config.GitHubApp, "comment-memory-app-token", config.GitHubToken)).
AddTemplatableBool("staged", templatableBoolPtrToStringPtr(config.Staged)).
Build()
}

// extractCommentMemoryConfig extracts comment-memory configuration from tools section.
func (c *Compiler) extractCommentMemoryConfig(toolsConfig *ToolsConfig) *CommentMemoryConfig {
if toolsConfig == nil || toolsConfig.CommentMemory == nil {
Expand Down
8 changes: 3 additions & 5 deletions pkg/workflow/comment_memory_config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,13 +91,11 @@ permissions:
compiler := NewCompiler(WithVersion("1.0.0"))
workflowData, err := compiler.ParseWorkflowFile(testFile)
require.NoError(t, err, "Failed to parse workflow")
require.NotNil(t, workflowData.SafeOutputs, "SafeOutputs should be present")

if tt.expectedCommentMemory != nil {
require.NotNil(t, workflowData.SafeOutputs.CommentMemory, "CommentMemory should be enabled")
assert.Equal(t, tt.expectedCommentMemory, workflowData.SafeOutputs.CommentMemory)
require.NotNil(t, workflowData.CommentMemoryConfig, "CommentMemory should be enabled")
assert.Equal(t, tt.expectedCommentMemory, workflowData.CommentMemoryConfig)
} else {
assert.Nil(t, workflowData.SafeOutputs.CommentMemory, "CommentMemory should be disabled")
assert.Nil(t, workflowData.CommentMemoryConfig, "CommentMemory should be disabled")
}
})
}
Expand Down
6 changes: 3 additions & 3 deletions pkg/workflow/compiler_custom_job_memory.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ func extractRestoreMemoryConfig(configMap map[string]any, jobName string, data *
cfg := &restoreMemoryConfig{
CacheMemory: data.CacheMemoryConfig != nil && len(data.CacheMemoryConfig.Caches) > 0,
RepoMemory: data.RepoMemoryConfig != nil && len(data.RepoMemoryConfig.Memories) > 0,
CommentMemory: data.SafeOutputs != nil && data.SafeOutputs.CommentMemory != nil,
CommentMemory: data.CommentMemoryConfig != nil,
}

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

Expand All @@ -184,7 +184,7 @@ func generateCommentMemoryRestoreLines(data *WorkflowData) []string {
lines = append(lines, " - name: Prepare comment memory files\n")
lines = append(lines, fmt.Sprintf(" uses: %s\n", getCachedActionPin("actions/github-script", data)))
lines = append(lines, " with:\n")
lines = append(lines, fmt.Sprintf(" github-token: %s\n", getEffectiveSafeOutputGitHubToken(data.SafeOutputs.CommentMemory.GitHubToken)))
lines = append(lines, fmt.Sprintf(" github-token: %s\n", getEffectiveSafeOutputGitHubToken(data.CommentMemoryConfig.GitHubToken)))
lines = append(lines, " script: |\n")
lines = append(lines, " const { setupGlobals } = require('${{ runner.temp }}/gh-aw/actions/setup_globals.cjs');\n")
lines = append(lines, " setupGlobals(core, github, context, exec, io, getOctokit);\n")
Expand Down
4 changes: 1 addition & 3 deletions pkg/workflow/compiler_custom_job_memory_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -524,9 +524,7 @@ func TestExtractRestoreMemoryConfig(t *testing.T) {
RepoMemoryConfig: &RepoMemoryConfig{
Memories: []RepoMemoryEntry{{ID: "default"}},
},
SafeOutputs: &SafeOutputsConfig{
CommentMemory: &CommentMemoryConfig{},
},
CommentMemoryConfig: &CommentMemoryConfig{},
}
emptyData := &WorkflowData{}

Expand Down
11 changes: 2 additions & 9 deletions pkg/workflow/compiler_orchestrator_workflow.go
Original file line number Diff line number Diff line change
Expand Up @@ -459,15 +459,8 @@ func (c *Compiler) extractAdditionalConfigurations(
// Use the already extracted output configuration
workflowData.SafeOutputs = safeOutputs

// Extract comment-memory from tools and attach to safe-outputs configuration.
// comment-memory now belongs under tools: next to cache-memory and repo-memory.
commentMemoryConfig := c.extractCommentMemoryConfig(toolsConfig)
if commentMemoryConfig != nil {
if workflowData.SafeOutputs == nil {
workflowData.SafeOutputs = &SafeOutputsConfig{}
}
workflowData.SafeOutputs.CommentMemory = commentMemoryConfig
}
// comment-memory belongs under tools: next to cache-memory and repo-memory.
workflowData.CommentMemoryConfig = c.extractCommentMemoryConfig(toolsConfig)

// Extract mcp-scripts configuration
workflowData.MCPScripts = c.extractMCPScriptsConfig(frontmatter)
Expand Down
4 changes: 2 additions & 2 deletions pkg/workflow/compiler_pre_activation_job.go
Original file line number Diff line number Diff line change
Expand Up @@ -266,14 +266,14 @@ func (c *Compiler) buildPreActivationMemoryRestoreSteps(data *WorkflowData, step
steps = append(steps, repoMemorySteps.String())
}

if data.SafeOutputs != nil && data.SafeOutputs.CommentMemory != nil {
if data.CommentMemoryConfig != nil {
if configLines, ok := c.generateCommentMemoryEarlyConfigLines(data); ok {
steps = append(steps, strings.Join(configLines, ""))
var commentMemorySteps strings.Builder
commentMemorySteps.WriteString(" - name: Prepare comment memory files\n")
fmt.Fprintf(&commentMemorySteps, " uses: %s\n", getCachedActionPin("actions/github-script", data))
commentMemorySteps.WriteString(" with:\n")
fmt.Fprintf(&commentMemorySteps, " github-token: %s\n", getEffectiveSafeOutputGitHubToken(data.SafeOutputs.CommentMemory.GitHubToken))
fmt.Fprintf(&commentMemorySteps, " github-token: %s\n", getEffectiveSafeOutputGitHubToken(data.CommentMemoryConfig.GitHubToken))
commentMemorySteps.WriteString(" script: |\n")
commentMemorySteps.WriteString(" const { setupGlobals } = require('${{ runner.temp }}/gh-aw/actions/setup_globals.cjs');\n")
commentMemorySteps.WriteString(" setupGlobals(core, github, context, exec, io, getOctokit);\n")
Expand Down
19 changes: 10 additions & 9 deletions pkg/workflow/compiler_safe_outputs_config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ func TestAddHandlerManagerConfigEnvVar(t *testing.T) {
tests := []struct {
name string
safeOutputs *SafeOutputsConfig
commentMemory *CommentMemoryConfig
checkContains []string
checkJSON bool
expectedKeys []string
Expand Down Expand Up @@ -855,14 +856,13 @@ func TestAddHandlerManagerConfigEnvVar(t *testing.T) {
expectedKeys: []string{"create_check_run"},
},
{
name: "comment_memory config",
safeOutputs: &SafeOutputsConfig{
CommentMemory: &CommentMemoryConfig{
BaseSafeOutputConfig: BaseSafeOutputConfig{
Max: strPtr("1"),
},
MemoryID: "test-memory",
name: "comment_memory config",
safeOutputs: &SafeOutputsConfig{},
commentMemory: &CommentMemoryConfig{
BaseSafeOutputConfig: BaseSafeOutputConfig{
Max: strPtr("1"),
},
MemoryID: "test-memory",
},
checkContains: []string{
"GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG",
Expand Down Expand Up @@ -925,8 +925,9 @@ func TestAddHandlerManagerConfigEnvVar(t *testing.T) {
compiler := NewCompiler()

workflowData := &WorkflowData{
Name: "Test Workflow",
SafeOutputs: tt.safeOutputs,
Name: "Test Workflow",
SafeOutputs: tt.safeOutputs,
CommentMemoryConfig: tt.commentMemory,
}

var steps []string
Expand Down
11 changes: 11 additions & 0 deletions pkg/workflow/compiler_safe_outputs_steps.go
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,17 @@ func (c *Compiler) addAppTokenMintingSteps(data *WorkflowData) []string {
stepID,
)...)
}
if commentMemory := data.CommentMemoryConfig; commentMemory != nil && commentMemory.GitHubApp != nil &&
!isHandlerStaged(templatableBoolIsTrue(data.SafeOutputs.Staged), commentMemory.Staged) {
steps = append(steps, c.buildGitHubAppTokenMintStepWithMeta(
commentMemory.GitHubApp,
NewPermissionsIssuesWrite(),
"",
"",
"Generate GitHub App token (comment-memory)",
"comment-memory-app-token",
)...)
}

// Dispatch-repository tool tokens: each non-staged tool with a github-app gets a token step.
if data.SafeOutputs.DispatchRepository != nil && len(data.SafeOutputs.DispatchRepository.Tools) > 0 {
Expand Down
Loading
Loading