diff --git a/docs/adr/53659-add-safe-output-config-fields-to-json-schema.md b/docs/adr/53659-add-safe-output-config-fields-to-json-schema.md new file mode 100644 index 00000000000..90a368b52f9 --- /dev/null +++ b/docs/adr/53659-add-safe-output-config-fields-to-json-schema.md @@ -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.* diff --git a/pkg/parser/schema_safe_outputs_target_test.go b/pkg/parser/schema_safe_outputs_target_test.go index 53d782cf3c4..428ca63162e 100644 --- a/pkg/parser/schema_safe_outputs_target_test.go +++ b/pkg/parser/schema_safe_outputs_target_test.go @@ -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) + } +} + +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 diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index fc633c71566..0ebab39bedf 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -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'])", @@ -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" + } + }, + "hide-older-comments-match": { + "type": "array", + "description": "Exact workflow IDs whose older comments are eligible to be hidden.", + "items": { + "type": "string" + } + }, "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).", @@ -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.", @@ -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": [ @@ -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" @@ -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." + }, "steps": { "type": "array", "description": "Array of extra job steps to run before engine execution", @@ -10972,6 +11010,10 @@ } ] }, + "environment": { + "type": "string", + "description": "GitHub Actions environment override for the detection job." + }, "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." diff --git a/pkg/workflow/comment_memory.go b/pkg/workflow/comment_memory.go index 0ef5fe2584d..528b4b1df8a 100644 --- a/pkg/workflow/comment_memory.go +++ b/pkg/workflow/comment_memory.go @@ -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 @@ -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 { diff --git a/pkg/workflow/comment_memory_config_test.go b/pkg/workflow/comment_memory_config_test.go index 5060f83df29..4b3ef8a09bb 100644 --- a/pkg/workflow/comment_memory_config_test.go +++ b/pkg/workflow/comment_memory_config_test.go @@ -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") } }) } diff --git a/pkg/workflow/compiler_custom_job_memory.go b/pkg/workflow/compiler_custom_job_memory.go index bcd04a288df..59300231ccd 100644 --- a/pkg/workflow/compiler_custom_job_memory.go +++ b/pkg/workflow/compiler_custom_job_memory.go @@ -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 { @@ -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 } @@ -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") diff --git a/pkg/workflow/compiler_custom_job_memory_test.go b/pkg/workflow/compiler_custom_job_memory_test.go index de2cdcf9388..e67bf3fdcec 100644 --- a/pkg/workflow/compiler_custom_job_memory_test.go +++ b/pkg/workflow/compiler_custom_job_memory_test.go @@ -524,9 +524,7 @@ func TestExtractRestoreMemoryConfig(t *testing.T) { RepoMemoryConfig: &RepoMemoryConfig{ Memories: []RepoMemoryEntry{{ID: "default"}}, }, - SafeOutputs: &SafeOutputsConfig{ - CommentMemory: &CommentMemoryConfig{}, - }, + CommentMemoryConfig: &CommentMemoryConfig{}, } emptyData := &WorkflowData{} diff --git a/pkg/workflow/compiler_orchestrator_workflow.go b/pkg/workflow/compiler_orchestrator_workflow.go index e5b5b13671b..2046966ea2e 100644 --- a/pkg/workflow/compiler_orchestrator_workflow.go +++ b/pkg/workflow/compiler_orchestrator_workflow.go @@ -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) diff --git a/pkg/workflow/compiler_pre_activation_job.go b/pkg/workflow/compiler_pre_activation_job.go index 2d202ea90c6..83ab864a65d 100644 --- a/pkg/workflow/compiler_pre_activation_job.go +++ b/pkg/workflow/compiler_pre_activation_job.go @@ -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") diff --git a/pkg/workflow/compiler_safe_outputs_config_test.go b/pkg/workflow/compiler_safe_outputs_config_test.go index 2fd7ab47cf2..ba64221dbdd 100644 --- a/pkg/workflow/compiler_safe_outputs_config_test.go +++ b/pkg/workflow/compiler_safe_outputs_config_test.go @@ -21,6 +21,7 @@ func TestAddHandlerManagerConfigEnvVar(t *testing.T) { tests := []struct { name string safeOutputs *SafeOutputsConfig + commentMemory *CommentMemoryConfig checkContains []string checkJSON bool expectedKeys []string @@ -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", @@ -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 diff --git a/pkg/workflow/compiler_safe_outputs_steps.go b/pkg/workflow/compiler_safe_outputs_steps.go index 734a7d5c212..345253a2270 100644 --- a/pkg/workflow/compiler_safe_outputs_steps.go +++ b/pkg/workflow/compiler_safe_outputs_steps.go @@ -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 { diff --git a/pkg/workflow/compiler_yaml_post_agent.go b/pkg/workflow/compiler_yaml_post_agent.go index 80af483bc0f..442257e01ee 100644 --- a/pkg/workflow/compiler_yaml_post_agent.go +++ b/pkg/workflow/compiler_yaml_post_agent.go @@ -67,7 +67,7 @@ func (c *Compiler) collectArtifactPaths(data *WorkflowData, engine CodingAgentEn paths = append(paths, constants.TmpGhAwDirSlash+constants.SafeOutputsFilename) // Processed agent output JSON produced by collect_ndjson_output.cjs paths = append(paths, constants.TmpGhAwDirSlash+constants.AgentOutputFilename) - if data.SafeOutputs.CommentMemory != nil { + if data.CommentMemoryConfig != nil { paths = append(paths, constants.TmpCommentMemoryDir) } } diff --git a/pkg/workflow/compiler_yaml_runtime_setup.go b/pkg/workflow/compiler_yaml_runtime_setup.go index 3140c0ddc03..f33830aa24b 100644 --- a/pkg/workflow/compiler_yaml_runtime_setup.go +++ b/pkg/workflow/compiler_yaml_runtime_setup.go @@ -241,11 +241,11 @@ func (c *Compiler) generateActivationArtifactAndCommentMemorySteps(yaml *strings yaml.WriteString(" path: /tmp/gh-aw\n") generateRestoreAmbientFoldersStep(yaml, data) - // Materialize comment-memory safe outputs as editable markdown files BEFORE user steps. + // Materialize tools.comment-memory state as editable markdown files BEFORE user steps. // This prepares /tmp/gh-aw/comment-memory/*.md from prior comment history and injects // prompt guidance so the agent can update files directly and persist them via the // comment_memory safe output. - if data.SafeOutputs == nil || data.SafeOutputs.CommentMemory == nil { + if data.CommentMemoryConfig == nil { return } @@ -259,7 +259,7 @@ func (c *Compiler) generateActivationArtifactAndCommentMemorySteps(yaml *strings yaml.WriteString(" - name: Prepare comment memory files\n") fmt.Fprintf(yaml, " uses: %s\n", getCachedActionPin("actions/github-script", data)) yaml.WriteString(" with:\n") - fmt.Fprintf(yaml, " github-token: %s\n", getEffectiveSafeOutputGitHubToken(data.SafeOutputs.CommentMemory.GitHubToken)) + fmt.Fprintf(yaml, " github-token: %s\n", getEffectiveSafeOutputGitHubToken(data.CommentMemoryConfig.GitHubToken)) yaml.WriteString(" script: |\n") yaml.WriteString(" const { setupGlobals } = require('${{ runner.temp }}/gh-aw/actions/setup_globals.cjs');\n") yaml.WriteString(" setupGlobals(core, github, context, exec, io, getOctokit);\n") @@ -289,12 +289,11 @@ func (c *Compiler) generateCommentMemoryEarlyConfigStep(yaml *strings.Builder, d // jobs, and pre-activation memory restore so all deterministic paths can prepare // comment-memory files before the full safe-outputs config exists. func (c *Compiler) generateCommentMemoryEarlyConfigLines(data *WorkflowData) ([]string, bool) { - builder := handlerRegistry[commentMemoryHandlerKey] - if builder == nil { - compilerYamlLog.Printf("Warning: %s handler not found in registry; skipping early config write", commentMemoryHandlerKey) - return nil, false + var globalFooter *bool + if data.SafeOutputs != nil { + globalFooter = data.SafeOutputs.Footer } - cfg := builder(data.SafeOutputs) + cfg := buildCommentMemoryHandlerConfig(data.CommentMemoryConfig, globalFooter) if cfg == nil { return nil, false } diff --git a/pkg/workflow/safe_output_handlers.go b/pkg/workflow/safe_output_handlers.go index 3e0b44983f5..79cf3cc97e7 100644 --- a/pkg/workflow/safe_output_handlers.go +++ b/pkg/workflow/safe_output_handlers.go @@ -157,17 +157,6 @@ var safeOutputHandlers = []safeOutputHandlerDescriptor{ return buildAddCommentPermissions(safeOutputs.AddComments) }, }, - { - Key: "comment-memory", - StructField: "CommentMemory", - NewConfig: func() any { return &CommentMemoryConfig{} }, - PermissionBuilder: func(safeOutputs *SafeOutputsConfig) *Permissions { - if !isSafeOutputHandlerEnabledAndUnstaged(safeOutputs, "CommentMemory") { - return nil - } - return NewPermissionsIssuesWrite() - }, - }, { Key: "create-pull-request", StructField: "CreatePullRequests", diff --git a/pkg/workflow/safe_outputs_config_generation.go b/pkg/workflow/safe_outputs_config_generation.go index 263536eec63..2e13c515173 100644 --- a/pkg/workflow/safe_outputs_config_generation.go +++ b/pkg/workflow/safe_outputs_config_generation.go @@ -29,8 +29,11 @@ import ( // they stay in sync with GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG. func generateSafeOutputsConfig(data *WorkflowData) (string, error) { if data.SafeOutputs == nil { - safeOutputsConfigLog.Print("No safe outputs configuration found, returning empty config") - return "", nil + if data.CommentMemoryConfig == nil { + safeOutputsConfigLog.Print("No safe outputs configuration found, returning empty config") + return "", nil + } + data.SafeOutputs = &SafeOutputsConfig{} } safeOutputsConfigLog.Print("Generating safe outputs configuration for workflow") @@ -73,6 +76,9 @@ func generateSafeOutputsConfig(data *WorkflowData) (string, error) { safeOutputsConfig[handlerName] = handlerCfg } } + if handlerConfig := buildCommentMemoryHandlerConfig(data.CommentMemoryConfig, data.SafeOutputs.Footer); handlerConfig != nil { + safeOutputsConfig[commentMemoryHandlerKey] = handlerConfig + } // Safe-jobs configuration: custom output types that run as separate GitHub Actions jobs. // These are not standard handlers but must be in config.json so the ingestion step can diff --git a/pkg/workflow/safe_outputs_config_generation_test.go b/pkg/workflow/safe_outputs_config_generation_test.go index 40803912435..3b05dd51532 100644 --- a/pkg/workflow/safe_outputs_config_generation_test.go +++ b/pkg/workflow/safe_outputs_config_generation_test.go @@ -60,6 +60,24 @@ jobs: assert.Equal(t, ".lock.yml", workflowFiles["ci"], "ci should map to .lock.yml") } +func TestGenerateSafeOutputsConfigCommentMemoryToolsOnly(t *testing.T) { + data := &WorkflowData{ + CommentMemoryConfig: &CommentMemoryConfig{ + BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("1")}, + MemoryID: "default", + }, + } + + result, err := generateSafeOutputsConfig(data) + require.NoError(t, err) + require.NotEmpty(t, result) + require.NotNil(t, data.SafeOutputs) + + var parsed map[string]any + require.NoError(t, json.Unmarshal([]byte(result), &parsed)) + assert.Contains(t, parsed, commentMemoryHandlerKey) +} + // TestGenerateSafeOutputsConfigActions tests that generateSafeOutputsConfig includes custom // action tool names as enabled keys so both MCP server implementations register them. func TestGenerateSafeOutputsConfigActions(t *testing.T) { diff --git a/pkg/workflow/safe_outputs_config_runtime.go b/pkg/workflow/safe_outputs_config_runtime.go index 406ef2f71e4..809eecc175c 100644 --- a/pkg/workflow/safe_outputs_config_runtime.go +++ b/pkg/workflow/safe_outputs_config_runtime.go @@ -92,6 +92,9 @@ func (c *Compiler) addHandlerManagerConfigEnvVar(steps *[]string, data *Workflow config[handlerName] = handlerConfig } } + if handlerConfig := buildCommentMemoryHandlerConfig(data.CommentMemoryConfig, safeOutputs.Footer); handlerConfig != nil { + config[commentMemoryHandlerKey] = handlerConfig + } // Include top-level mentions configuration so the handler manager can pass it to // markdown-producing handlers that call sanitizeContent with allowed aliases. diff --git a/pkg/workflow/safe_outputs_config_types.go b/pkg/workflow/safe_outputs_config_types.go index 36a4ffcbab7..657e4e2cbd5 100644 --- a/pkg/workflow/safe_outputs_config_types.go +++ b/pkg/workflow/safe_outputs_config_types.go @@ -47,7 +47,6 @@ type SafeOutputsConfig struct { ApproveWorkflowRun *ApproveWorkflowRunConfig `yaml:"approve-workflow-run,omitempty"` // Approve a pending workflow run awaiting required approval DismissPullRequestReview *DismissPullRequestReviewConfig `yaml:"dismiss-pull-request-review,omitempty"` // Dismiss a pull request review authored by the workflow actor AddComments *AddCommentsConfig `yaml:"add-comment,omitempty"` - CommentMemory *CommentMemoryConfig `yaml:"comment-memory,omitempty"` // Persist and update managed memory comments on issues/PRs CreatePullRequests *CreatePullRequestsConfig `yaml:"create-pull-request,omitempty"` CreatePullRequestReviewComments *CreatePullRequestReviewCommentsConfig `yaml:"create-pull-request-review-comment,omitempty"` SubmitPullRequestReview *SubmitPullRequestReviewConfig `yaml:"submit-pull-request-review,omitempty"` // Submit a PR review with status (APPROVE, REQUEST_CHANGES, COMMENT) diff --git a/pkg/workflow/safe_outputs_handler_registry.go b/pkg/workflow/safe_outputs_handler_registry.go index e5e8823d8c2..eb39b48ca88 100644 --- a/pkg/workflow/safe_outputs_handler_registry.go +++ b/pkg/workflow/safe_outputs_handler_registry.go @@ -4,10 +4,6 @@ import "github.com/github/gh-aw/pkg/logger" var handlerRegistryLog = logger.New("workflow:safe_outputs_handler_registry") -// commentMemoryHandlerKey is the registry key for the comment_memory safe output handler. -// Using a constant here prevents silent regressions if the key is ever renamed. -const commentMemoryHandlerKey = "comment_memory" - // resolveHandlerGitHubToken returns the effective GitHub token expression for a handler. // When app is non-nil (a per-handler github-app is configured), the compiler has already // minted a dedicated token step whose ID is "{handlerKey}-app-token"; this function returns @@ -108,22 +104,6 @@ var handlerRegistry = map[string]handlerBuilder{ AddTemplatableBool("staged", templatableBoolPtrToStringPtr(c.Staged)). Build() }, - commentMemoryHandlerKey: func(cfg *SafeOutputsConfig) map[string]any { - if cfg.CommentMemory == nil { - return nil - } - c := cfg.CommentMemory - return newHandlerConfigBuilder(). - AddTemplatableInt("max", c.Max). - AddIfNotEmpty("target", c.Target). - AddIfNotEmpty("target-repo", c.TargetRepoSlug). - AddStringSlice("allowed_repos", c.AllowedRepos). - AddIfNotEmpty("memory_id", c.MemoryID). - AddTemplatableBool("footer", getEffectiveFooterForTemplatable(c.Footer, cfg.Footer)). - AddIfNotEmpty("github-token", resolveHandlerGitHubToken(c.GitHubApp, "comment-memory", c.GitHubToken)). - AddTemplatableBool("staged", templatableBoolPtrToStringPtr(c.Staged)). - Build() - }, "create_discussion": func(cfg *SafeOutputsConfig) map[string]any { if cfg.CreateDiscussions == nil { return nil diff --git a/pkg/workflow/safe_outputs_prompt_tools_test.go b/pkg/workflow/safe_outputs_prompt_tools_test.go index 6af06554117..2dd77f1438d 100644 --- a/pkg/workflow/safe_outputs_prompt_tools_test.go +++ b/pkg/workflow/safe_outputs_prompt_tools_test.go @@ -128,7 +128,7 @@ func TestBuildSafeOutputsSectionsCustomTools(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - sections := buildSafeOutputsSections(tt.safeOutputs) + sections := buildSafeOutputsSections(tt.safeOutputs, nil) if tt.expectNil { assert.Nil(t, sections, "Expected nil sections for empty/nil config") @@ -163,7 +163,7 @@ func TestBuildSafeOutputsSectionsCustomToolsConsistency(t *testing.T) { }, } - sections := buildSafeOutputsSections(config) + sections := buildSafeOutputsSections(config, nil) require.NotNil(t, sections, "Expected non-nil sections") actualToolNames := extractToolNamesFromSections(t, sections) @@ -208,7 +208,7 @@ func TestBuildSafeOutputsSectionsMaxExpressionExtraction(t *testing.T) { }, }, NoOp: &NoOpConfig{}, - }) + }, nil) require.NotNil(t, sections, "Expected non-nil sections") @@ -245,10 +245,7 @@ func TestBuildSafeOutputsSectionsMaxExpressionExtraction(t *testing.T) { } func TestBuildSafeOutputsSections_IncludesCommentMemoryPromptFile(t *testing.T) { - sections := buildSafeOutputsSections(&SafeOutputsConfig{ - CommentMemory: &CommentMemoryConfig{}, - NoOp: &NoOpConfig{}, - }) + sections := buildSafeOutputsSections(nil, &CommentMemoryConfig{}) require.NotNil(t, sections, "Expected non-nil sections") @@ -262,8 +259,11 @@ func TestBuildSafeOutputsSections_IncludesCommentMemoryPromptFile(t *testing.T) assert.True(t, found, "Expected comment-memory guidance file to be included when comment_memory is enabled") - actualToolNames := extractToolNamesFromSections(t, sections) - assert.NotContains(t, actualToolNames, "comment_memory", "comment_memory should not be exposed as an agent tool when file-based sync is enabled") + for _, section := range sections { + if !section.IsFile { + assert.NotContains(t, section.Content, "comment_memory", "comment_memory should not be exposed as an agent tool when file-based sync is enabled") + } + } } // the list of tool names in the order they appear, stripping any max-budget annotations @@ -287,7 +287,6 @@ func extractToolNamesFromSections(t *testing.T, sections []PromptSection) []stri toolsListLine := lines[1] require.True(t, strings.HasPrefix(toolsListLine, "Tools: "), "Second line should start with 'Tools: ', got: %q", toolsListLine) - toolsList := strings.TrimPrefix(toolsListLine, "Tools: ") toolEntries := strings.Split(toolsList, ", ") diff --git a/pkg/workflow/safe_outputs_state.go b/pkg/workflow/safe_outputs_state.go index 99ff6b30ae2..7f7f90f7fc4 100644 --- a/pkg/workflow/safe_outputs_state.go +++ b/pkg/workflow/safe_outputs_state.go @@ -39,8 +39,7 @@ func hasAnySafeOutputEnabled(safeOutputs *SafeOutputsConfig) bool { return true } - // Direct nil checks — no reflection, no heap allocation (43 fields matching safeOutputFieldMapping - // plus CommentMemory which is attached via tools.comment-memory and not in safeOutputFieldMapping). + // Direct nil checks — no reflection, no heap allocation. return safeOutputs.CreateIssues != nil || safeOutputs.CreateAgentSessions != nil || safeOutputs.CreateDiscussions != nil || @@ -52,7 +51,6 @@ func hasAnySafeOutputEnabled(safeOutputs *SafeOutputsConfig) bool { safeOutputs.ApproveWorkflowRun != nil || safeOutputs.DismissPullRequestReview != nil || safeOutputs.AddComments != nil || - safeOutputs.CommentMemory != nil || safeOutputs.CreatePullRequests != nil || safeOutputs.CreatePullRequestReviewComments != nil || safeOutputs.SubmitPullRequestReview != nil || @@ -106,9 +104,7 @@ func hasNonBuiltinSafeOutputsEnabled(safeOutputs *SafeOutputsConfig) bool { return true } - // Direct nil checks for non-builtin pointer fields (40 fields = 43 total minus 3 builtins: - // NoOp, MissingData, MissingTool). Includes CommentMemory which is attached via - // tools.comment-memory and is not in safeOutputFieldMapping. + // Direct nil checks for non-builtin pointer fields. return safeOutputs.CreateIssues != nil || safeOutputs.CreateAgentSessions != nil || safeOutputs.CreateDiscussions != nil || @@ -120,7 +116,6 @@ func hasNonBuiltinSafeOutputsEnabled(safeOutputs *SafeOutputsConfig) bool { safeOutputs.ApproveWorkflowRun != nil || safeOutputs.DismissPullRequestReview != nil || safeOutputs.AddComments != nil || - safeOutputs.CommentMemory != nil || safeOutputs.CreatePullRequests != nil || safeOutputs.CreatePullRequestReviewComments != nil || safeOutputs.SubmitPullRequestReview != nil || @@ -174,7 +169,7 @@ func HasSafeOutputsEnabled(safeOutputs *SafeOutputsConfig) bool { // 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 hasNonBuiltinSafeOutputsEnabled(workflowData.SafeOutputs) { + if workflowData.CommentMemoryConfig != nil || hasNonBuiltinSafeOutputsEnabled(workflowData.SafeOutputs) { return } if workflowData.SafeOutputs == nil { diff --git a/pkg/workflow/safe_outputs_state_test.go b/pkg/workflow/safe_outputs_state_test.go index d7d620267b6..5048c88e03e 100644 --- a/pkg/workflow/safe_outputs_state_test.go +++ b/pkg/workflow/safe_outputs_state_test.go @@ -47,17 +47,3 @@ func TestSafeOutputStateFieldCoverage(t *testing.T) { }) } } - -// TestSafeOutputStateCommentMemoryCoverage explicitly tests CommentMemory, which is -// attached to SafeOutputs via tools.comment-memory (not listed in safeOutputFieldMapping) -// and must be checked by both state inspection functions. -func TestSafeOutputStateCommentMemoryCoverage(t *testing.T) { - cfg := &SafeOutputsConfig{ - CommentMemory: &CommentMemoryConfig{}, - } - - assert.True(t, hasAnySafeOutputEnabled(cfg), - "hasAnySafeOutputEnabled should return true when CommentMemory is set") - assert.True(t, hasNonBuiltinSafeOutputsEnabled(cfg), - "hasNonBuiltinSafeOutputsEnabled should return true when CommentMemory is set") -} diff --git a/pkg/workflow/unified_prompt_step.go b/pkg/workflow/unified_prompt_step.go index 6271510b3ed..822d4b78a26 100644 --- a/pkg/workflow/unified_prompt_step.go +++ b/pkg/workflow/unified_prompt_step.go @@ -122,7 +122,7 @@ func (c *Compiler) collectPromptSections(data *WorkflowData) []PromptSection { } // 8. Safe outputs instructions (if enabled) - if HasSafeOutputsEnabled(data.SafeOutputs) { + if HasSafeOutputsEnabled(data.SafeOutputs) || data.CommentMemoryConfig != nil { unifiedPromptLog.Print("Adding safe outputs section") // Static intro from file (gh CLI warning, temporary ID rules, noop note) sections = append(sections, PromptSection{ @@ -130,7 +130,7 @@ func (c *Compiler) collectPromptSections(data *WorkflowData) []PromptSection { IsFile: true, }) // Per-tool sections: opening tag + tools list (inline), tool instruction files, closing tag - sections = append(sections, buildSafeOutputsSections(data.SafeOutputs)...) + sections = append(sections, buildSafeOutputsSections(data.SafeOutputs, data.CommentMemoryConfig)...) } // 8a. MCP CLI tools instructions (if any MCP servers are mounted as CLIs) @@ -462,9 +462,9 @@ func toolWithMaxBudget(name string, max *string) string { // // The static intro (gh CLI warning, temporary ID rules, noop note) lives in // actions/setup/md/safe_outputs_prompt.md and is included by the caller before these sections. -func buildSafeOutputsSections(safeOutputs *SafeOutputsConfig) []PromptSection { +func buildSafeOutputsSections(safeOutputs *SafeOutputsConfig, commentMemory *CommentMemoryConfig) []PromptSection { if safeOutputs == nil { - return nil + safeOutputs = &SafeOutputsConfig{} } safeOutputsPromptLog.Print("Building safe outputs sections") @@ -634,7 +634,7 @@ func buildSafeOutputsSections(safeOutputs *SafeOutputsConfig) []PromptSection { } } - if len(tools) == 0 { + if len(tools) == 0 && commentMemory == nil { return nil } @@ -645,7 +645,10 @@ func buildSafeOutputsSections(safeOutputs *SafeOutputsConfig) []PromptSection { // run: heredoc (which is subject to GitHub Actions' 21KB expression-size limit). // Expressions are replaced with __GH_AW_...__ placeholders and added to EnvVars // so the placeholder substitution step can resolve them at runtime. - toolsContent := "\nTools: " + strings.Join(tools, ", ") + toolsContent := "\n" + if len(tools) > 0 { + toolsContent += "Tools: " + strings.Join(tools, ", ") + } envVars := make(map[string]string) extractor := NewExpressionExtractor() exprMappings, err := extractor.ExtractExpressions(toolsContent) @@ -671,7 +674,7 @@ func buildSafeOutputsSections(safeOutputs *SafeOutputsConfig) []PromptSection { if safeOutputs.PushToPullRequestBranch != nil { sections = append(sections, PromptSection{Content: safeOutputsPushToBranchFile, IsFile: true}) } - if safeOutputs.CommentMemory != nil { + if commentMemory != nil { sections = append(sections, PromptSection{Content: safeOutputsCommentMemoryFile, IsFile: true}) } if safeOutputs.UploadAssets != nil { diff --git a/pkg/workflow/workflow_data.go b/pkg/workflow/workflow_data.go index d52980d1c25..b294f09d103 100644 --- a/pkg/workflow/workflow_data.go +++ b/pkg/workflow/workflow_data.go @@ -141,6 +141,7 @@ type WorkflowData struct { Bots []string // allow list of bot identifiers that can trigger workflow RateLimit *RateLimitConfig // rate limiting configuration for workflow triggers CacheMemoryConfig *CacheMemoryConfig // parsed cache-memory configuration + CommentMemoryConfig *CommentMemoryConfig // parsed tools.comment-memory configuration RepoMemoryConfig *RepoMemoryConfig // parsed repo-memory configuration Runtimes map[string]any // runtime version overrides from frontmatter ToolsTimeout string // timeout for tool/MCP operations: numeric string (seconds) or GitHub Actions expression (empty = use engine default) diff --git a/scripts/check-safe-outputs-conformance.sh b/scripts/check-safe-outputs-conformance.sh index 31c33b432d1..9c516c2d298 100755 --- a/scripts/check-safe-outputs-conformance.sh +++ b/scripts/check-safe-outputs-conformance.sh @@ -454,6 +454,14 @@ for line in handlers.splitlines(): if field_match and handler_key: handler_fields[field_match.group(1)] = handler_key +compiler_populated_fields = { + "safe-outputs.call-workflow.workflow_files", + "safe-outputs.dispatch-workflow.workflow_files", + "safe-outputs.dispatch-workflow.aw_context_workflows", +} + +tool_configured_outputs = {"comment-memory"} + def yaml_fields(struct_name): for line in structs.get(struct_name, "").splitlines(): @@ -489,6 +497,8 @@ for line in structs["SafeOutputsConfig"].splitlines(): if struct_field not in handler_fields: continue output_name = output_name.split(",", 1)[0] + if output_name in tool_configured_outputs: + continue output_schema = safe_outputs.get(output_name) if output_schema is None: missing.append(f"safe-outputs.{output_name}") @@ -496,8 +506,9 @@ for line in structs["SafeOutputsConfig"].splitlines(): output_properties = properties(output_schema) for tag, inline in yaml_fields(config_type): - if not inline and tag not in output_properties: - missing.append(f"safe-outputs.{output_name}.{tag}") + property_path = f"safe-outputs.{output_name}.{tag}" + if not inline and property_path not in compiler_populated_fields and tag not in output_properties: + missing.append(property_path) print("\n".join(sorted(set(missing)))) PY