Add missing safe-output configuration schema coverage - #53659
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. All changes are additive JSON schema properties, a regression test, and a small compiler-populated-fields allowlist in the conformance script — no reinvented logic, unused flexibility, or speculative abstractions to cut.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The changes look correct and well-structured. Schema additions are consistent with existing patterns, the new test provides good coverage of the added fields, and the conformance script fix correctly excludes compiler-populated fields from the missing-field check. No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 16.2 AIC · ⌖ 8.75 AIC · ⊞ 5.7K
There was a problem hiding this comment.
Request changes
This patch adds useful schema coverage, but one of the new schema branches no longer matches the actual parser contract for safe-outputs.comment-memory, so it can now bless configurations that the compiler rejects.
The blocking theme
safe-outputs.comment-memory.footeris declared here as a plain boolean-only field, while the parser still preprocesses it as a templatable boolean string/expression path.- Reusing
#/properties/tools/properties/comment-memoryviaanyOfalso silently broadens the accepted shape to includemax,github-token,staged,issue-intent, andsamples, which were not mentioned in the new regression test. - That mismatch means schema validation can drift again instead of proving parity.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 8.96 AIC · ⌖ 6.81 AIC · ⊞ 7K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — requesting changes on two schema correctness issues.
📋 Key Themes & Highlights
Issues
comment-memorysafe-outputsanyOfarm 1 —$refleak (main_workflow_schema.jsonline 7186): The$refto the tools definition silently allowstrue/nullshorthand forms in the safe-outputs context, which is likely unintended.comment-memorysafe-outputsanyOfarm 2 — open object (main_workflow_schema.jsonline 7212): Missing"additionalProperties": falsemeans unknown fields pass validation, breaking the guard that all other safe-outputs config objects enforce.- Happy-path-only test (
schema_safe_outputs_target_test.goline 48): A negative test covering at least one invalid field would turn this into a proper regression guard.
Positive Highlights
- ✅ The conformance-script change (
compiler_populated_fieldsallowlist) is clean and well-scoped. - ✅ All other new schema properties (
require-temporary-id,base-branch,auto_create,engine-config,model,environment) are well-formed and use consistent style. - ✅ Good coverage of the PR body's stated IMP-004 scope.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 32.8 AIC · ⌖ 11.3 AIC · ⊞ 7.8K
Comment /matt to run again
| }, | ||
| "memory-id": { | ||
| "type": "string", | ||
| "description": "Default memory identifier when an item does not provide memory_id." |
There was a problem hiding this comment.
[/codebase-design] The new comment-memory safe-outputs branch (the second anyOf arm) lacks "additionalProperties": false, so unknown fields won't be caught by schema validation.
💡 Suggested fix
Add "additionalProperties": false to the inline object branch — every other safe-outputs config object in this schema closes with it; this branch is the only exception introduced by this PR.
{
"type": "object",
"properties": { ... },
"additionalProperties": false
}@copilot please address this.
| ], | ||
| "description": "Enable AI agents to add comments to GitHub issues, pull requests, or discussions. Supports templating, cross-repository commenting, and automatic mentions." | ||
| }, | ||
| "comment-memory": { |
There was a problem hiding this comment.
[/codebase-design] The comment-memory safe-outputs entry's first anyOf arm is $ref: #/properties/tools/properties/comment-memory. The referenced tools definition is itself a oneOf that includes true and null shorthand variants, so those are silently allowed here too. Unless true/null are valid in the safe-outputs context, consider using a plain inline object with additionalProperties: false (matching the create-pull-request and create-issue patterns) rather than a $ref to the tools definition.
@copilot please address this.
| } | ||
|
|
||
| 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) |
There was a problem hiding this comment.
[/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.
There was a problem hiding this comment.
Pull request overview
Adds missing safe-output fields to the workflow schema for validation and editor tooling.
Changes:
- Expands safe-output schema coverage.
- Excludes compiler-populated fields from conformance checks.
- Adds schema regression validation.
Show a summary per file
| File | Description |
|---|---|
scripts/check-safe-outputs-conformance.sh |
Excludes compiler-owned fields. |
pkg/parser/schemas/main_workflow_schema.json |
Declares missing safe-output properties. |
pkg/parser/schema_safe_outputs_target_test.go |
Tests schema acceptance. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Suppressed comments (1)
pkg/parser/schemas/main_workflow_schema.json:7216
CommentMemoryConfig.Footeris a templatable string field, andparseCommentMemoryConfigValueusespreprocessBoolFieldAsString, which accepts both booleans and GitHub Actions expressions. This boolean-only declaration rejects the expression form supported by the Go implementation.
"footer": {
"type": "boolean",
"description": "Controls whether the AI-generated footer is added to the managed comment."
- Files reviewed: 3/3 changed files
- Comments generated: 5
- Review effort level: Balanced
| "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" | ||
| } | ||
| }, |
| "engine-config": { | ||
| "$ref": "#/$defs/engine_config", | ||
| "description": "Extended engine configuration for threat detection." | ||
| }, | ||
| "model": { | ||
| "type": "string", | ||
| "description": "Model override for threat detection engine execution." | ||
| }, |
| "environment": { | ||
| "type": "string", | ||
| "description": "GitHub Actions environment override for the detection job." | ||
| }, |
| "comment-memory": { | ||
| "description": "Comment memory configuration for managed comment persistence.", | ||
| "anyOf": [ | ||
| { | ||
| "$ref": "#/properties/tools/properties/comment-memory" |
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>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (122 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
|
|
@copilot Please address the remaining blocking review feedback on this PR, then run the
|
…issing-json-schema-properties Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in 0fc0f52: |
|
@copilot remove safe-outputs.comment-memory from schema and parser, go types. Only tools.comment-memory is valid |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot resolve the merge conflicts on this branch. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Resolved and merged current |
|
@copilot run pr-finisher skill |
PR-finisher completed. Targeted parser tests and workflow recompilation pass. The branch has no local changes, but merge readiness is blocked by unresolved review threads and repository-wide validation failures from current |
|
🎉 This pull request is included in a new release. Release: |
IMP-004 found safe-output configuration fields implemented in Go but absent from the workflow JSON schema, reducing editor validation and autocomplete coverage.
Schema coverage
safe-outputs.comment-memoryconfiguration support.Compiler-owned fields
workflow_filesandaw_context_workflowsin the conformance check because the compiler populates them; they remain excluded from user-facing schema.Regression coverage