fix(create-issue): emit deduplicate-by-title in compiled handler config #43345
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| # ADR-43345: Introduce TemplatableBoolOrInt for Config Fields Accepting Boolean, Integer, or GitHub Actions Expressions | ||
|
|
||
| **Date**: 2026-07-04 | ||
| **Status**: Draft | ||
| **Deciders**: Unknown | ||
|
|
||
| --- | ||
|
|
||
| ### Context | ||
|
|
||
| The `deduplicate-by-title` field in `CreateIssuesConfig` was typed as `any`, which caused the field's value to be silently dropped when building the emitted handler config for the `create_issue` safe output. Additionally, users need to pass GitHub Actions expression strings (e.g., `${{ inputs.dedup }}`) to this field so that deduplication mode can be configured dynamically at runtime; the previous `any` type and schema definition only accepted booleans and integers, rejecting expression strings at validation time. A type-safe, serialization-aware representation was needed to correctly parse all three input forms (boolean, integer 0–100, or expression string) and emit them as their appropriate JSON types (JSON boolean, JSON number, or JSON string respectively). | ||
|
|
||
| ### Decision | ||
|
|
||
| We decided to introduce a new string-backed type `TemplatableBoolOrInt` in `pkg/workflow/templatables.go` with custom YAML and JSON marshaling. The type stores all three forms internally as a Go string (`"true"`, `"false"`, a decimal integer, or a `${{ … }}` expression), then emits the correct native JSON value via `MarshalJSON`/`ToValue`. `CreateIssuesConfig.DeduplicateByTitle` was changed from `any` to `*TemplatableBoolOrInt`, and the handler registry was updated to call a new `AddTemplatableBoolOrInt` builder method, completing the emission path that was previously silently broken. | ||
|
|
||
| ### Alternatives Considered | ||
|
|
||
| #### Alternative 1: Keep `any` type and add post-parse type-switch conversion | ||
|
|
||
| The `AddBoolOrInt` builder method already performs a type-switch on `any` values to emit booleans or integers. The emission bug could have been fixed by simply wiring `DeduplicateByTitle` into `AddBoolOrInt` without changing the field type. However, `any` carries no type constraints at parse time, so schema validation alone blocks expression strings from entering the field. Expression strings would have required additional ad-hoc handling that bypasses the type system and is invisible in struct definitions. | ||
|
|
||
| #### Alternative 2: Explicit union struct (`struct { Bool *bool; Int *int; Expr *string }`) | ||
|
|
||
| A struct with three optional fields makes the valid value space explicit in the type system with no custom marshalers beyond implementing `yaml.Unmarshaler`. This is more verbose and requires callers to inspect which field is set. It also complicates the JSON schema `$defs` entry and adds branching in every emission site. The string-backed approach centralises all conversion logic inside the type and allows the schema to reference a single `$defs/templatable_bool_or_int` entry. | ||
|
|
||
| ### Consequences | ||
|
|
||
| #### Positive | ||
| - `deduplicate-by-title` is now correctly emitted in the handler config for all three input forms, fixing the silent-drop bug that caused deduplication to never activate when configured. | ||
| - Users can pass GitHub Actions expression strings (e.g., `${{ inputs.dedup }}`) to `deduplicate-by-title`; the value is stored and emitted as a JSON string for runtime evaluation, enabling dynamic configuration without a schema change. | ||
| - The JSON schema `$defs/templatable_bool_or_int` definition is reusable for future fields that need the same three-way acceptance. | ||
|
|
||
| #### Negative | ||
| - `TemplatableBoolOrInt` introduces a custom string type with non-trivial YAML and JSON unmarshal/marshal logic that must be maintained and kept in sync with the schema definition. | ||
| - Integer range validation (0–100) is baked into the type's unmarshal methods, coupling the generic type to a specific field's constraints; reusing the type for a field with a different integer range would require a new type or a parameterised constructor. | ||
|
|
||
| #### Neutral | ||
| - All existing callsites that previously used `typeutil.ParseIntValue` to read `DeduplicateByTitle` have been updated to access the `*TemplatableBoolOrInt` pointer directly. | ||
| - Four new unit tests cover the boolean, integer, expression, and nil cases end-to-end through the config-generation pipeline. | ||
|
|
||
| --- | ||
|
|
||
| *ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,7 +9,6 @@ import ( | |
| "testing" | ||
|
|
||
| "github.com/github/gh-aw/pkg/testutil" | ||
| ) | ||
|
|
||
| // assertTokenInProcessSafeOutputsEnv verifies that a given environment variable name | ||
|
|
@@ -200,9 +199,9 @@ This workflow tests the output configuration parsing. | |
| } | ||
| } | ||
|
|
||
| deduplicateByTitle := workflowData.SafeOutputs.CreateIssues.DeduplicateByTitle | ||
| if deduplicateByTitle == nil || string(*deduplicateByTitle) != "1" { | ||
| t.Errorf("Expected deduplicate-by-title to parse as '1', got %#v", deduplicateByTitle) | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -495,3 +494,163 @@ This workflow tests that copilot assignment is wired in consolidated safe output | |
| t.Error("Did not expect legacy assign_copilot_to_created_issues output wiring in safe_outputs job") | ||
| } | ||
| } | ||
|
|
||
| // TestDeduplicateByTitleBoolean verifies that deduplicate-by-title: true is emitted correctly. | ||
| func TestDeduplicateByTitleBoolean(t *testing.T) { | ||
| tmpDir := testutil.TempDir(t, "dedup-bool-test") | ||
|
|
||
| testContent := `--- | ||
| on: workflow_dispatch | ||
| permissions: | ||
| contents: read | ||
| engine: copilot | ||
| strict: false | ||
| safe-outputs: | ||
| create-issue: | ||
| max: 1 | ||
| deduplicate-by-title: true | ||
| --- | ||
|
|
||
| Create test issues. | ||
| ` | ||
| testFile := filepath.Join(tmpDir, "test-dedup-bool.md") | ||
| if err := os.WriteFile(testFile, []byte(testContent), 0644); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| compiler := NewCompiler() | ||
| if err := compiler.CompileWorkflow(testFile); err != nil { | ||
| t.Fatalf("Failed to compile: %v", err) | ||
| } | ||
|
|
||
| lockContent, err := os.ReadFile(filepath.Join(tmpDir, "test-dedup-bool.lock.yml")) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| if !strings.Contains(string(lockContent), `\"deduplicate_by_title\":true`) { | ||
|
Contributor
There was a problem hiding this comment. [/tdd] The test name says "true/false is emitted correctly" but only This is a meaningful edge case: 💡 Suggested extensionAdd a companion test or subtest for if !strings.Contains(string(lockContent), `\"deduplicate_by_title\":false`) {
t.Errorf("Expected deduplicate_by_title:false in handler config, got:\n%s", lockContent)
}@copilot please address this.
Contributor
There was a problem hiding this comment. Missing 💡 Why this matters and what to add
Add a compile-level check alongside the existing // TestDeduplicateByTitleFalse verifies that deduplicate-by-title: false emits the
// JSON boolean false (not omits the field).
func TestDeduplicateByTitleFalse(t *testing.T) {
tmpDir := testutil.TempDir(t, "dedup-false-test")
testContent := `---
on: workflow_dispatch
permissions:
contents: read
engine: copilot
strict: false
safe-outputs:
create-issue:
max: 1
deduplicate-by-title: false
---
Create test issues.
`
// ... compile, read lock, assert:
if !strings.Contains(string(lockContent), `\"deduplicate_by_title\":false`) {
t.Errorf("Expected deduplicate_by_title:false in handler config, got:\n%s", lockContent)
}
}A parallel unit test in |
||
| t.Errorf("Expected deduplicate_by_title:true in handler config, got:\n%s", lockContent) | ||
| } | ||
| } | ||
|
|
||
| // TestDeduplicateByTitleFalse verifies that deduplicate-by-title: false is emitted correctly. | ||
| func TestDeduplicateByTitleFalse(t *testing.T) { | ||
| tmpDir := testutil.TempDir(t, "dedup-false-test") | ||
|
|
||
| testContent := `--- | ||
| on: workflow_dispatch | ||
| permissions: | ||
| contents: read | ||
| engine: copilot | ||
| strict: false | ||
| safe-outputs: | ||
| create-issue: | ||
| max: 1 | ||
| deduplicate-by-title: false | ||
| --- | ||
|
|
||
| Create test issues. | ||
| ` | ||
| testFile := filepath.Join(tmpDir, "test-dedup-false.md") | ||
| if err := os.WriteFile(testFile, []byte(testContent), 0644); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| compiler := NewCompiler() | ||
| if err := compiler.CompileWorkflow(testFile); err != nil { | ||
| t.Fatalf("Failed to compile: %v", err) | ||
| } | ||
|
|
||
| lockContent, err := os.ReadFile(filepath.Join(tmpDir, "test-dedup-false.lock.yml")) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| if !strings.Contains(string(lockContent), `\"deduplicate_by_title\":false`) { | ||
| t.Errorf("Expected deduplicate_by_title:false in handler config, got:\n%s", lockContent) | ||
| } | ||
| } | ||
|
|
||
| // TestDeduplicateByTitleInteger verifies that deduplicate-by-title: <int> is emitted correctly. | ||
| func TestDeduplicateByTitleInteger(t *testing.T) { | ||
| tmpDir := testutil.TempDir(t, "dedup-int-test") | ||
|
|
||
| testContent := `--- | ||
| on: workflow_dispatch | ||
| permissions: | ||
| contents: read | ||
| engine: copilot | ||
| strict: false | ||
| safe-outputs: | ||
| create-issue: | ||
| max: 1 | ||
| deduplicate-by-title: 2 | ||
| --- | ||
|
|
||
| Create test issues. | ||
| ` | ||
| testFile := filepath.Join(tmpDir, "test-dedup-int.md") | ||
| if err := os.WriteFile(testFile, []byte(testContent), 0644); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| compiler := NewCompiler() | ||
| if err := compiler.CompileWorkflow(testFile); err != nil { | ||
| t.Fatalf("Failed to compile: %v", err) | ||
| } | ||
|
|
||
| lockContent, err := os.ReadFile(filepath.Join(tmpDir, "test-dedup-int.lock.yml")) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| if !strings.Contains(string(lockContent), `\"deduplicate_by_title\":2`) { | ||
| t.Errorf("Expected deduplicate_by_title:2 in handler config, got:\n%s", lockContent) | ||
| } | ||
| } | ||
|
|
||
| // TestDeduplicateByTitleExpression verifies that deduplicate-by-title accepts | ||
| // GitHub Actions expressions and emits them as a JSON string so the runtime | ||
| // can evaluate them. | ||
| func TestDeduplicateByTitleExpression(t *testing.T) { | ||
| tmpDir := testutil.TempDir(t, "dedup-expr-test") | ||
|
|
||
| testContent := `--- | ||
| on: | ||
| workflow_dispatch: | ||
| inputs: | ||
| dedup: | ||
| type: boolean | ||
| default: true | ||
| permissions: | ||
| contents: read | ||
| engine: copilot | ||
| strict: false | ||
| safe-outputs: | ||
| create-issue: | ||
| max: 1 | ||
| deduplicate-by-title: ${{ inputs.dedup }} | ||
| --- | ||
|
|
||
| Create test issues. | ||
| ` | ||
| testFile := filepath.Join(tmpDir, "test-dedup-expr.md") | ||
| if err := os.WriteFile(testFile, []byte(testContent), 0644); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| compiler := NewCompiler() | ||
| if err := compiler.CompileWorkflow(testFile); err != nil { | ||
| t.Fatalf("Failed to compile: %v", err) | ||
| } | ||
|
|
||
| lockContent, err := os.ReadFile(filepath.Join(tmpDir, "test-dedup-expr.lock.yml")) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| // GH Actions expressions are emitted as JSON strings so they can be evaluated at runtime. | ||
| if !strings.Contains(string(lockContent), `\"deduplicate_by_title\":\"${{ inputs.dedup }}\"`) { | ||
| t.Errorf("Expected deduplicate_by_title expression in handler config, got:\n%s", lockContent) | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/diagnosing-bugs] Using both
$refanddescriptionon the same schema node may not behave as expected in all JSON Schema validators — the JSON Schema spec says sibling keywords alongside$refare ignored in draft-07 (which is commonly used by editor tooling).The description you want will likely be shadowed by the one in the
templatable_bool_or_int$defsentry. Consider whether the field-specific description should live in the$defsentry itself, or if you need to useallOfto merge.💡 Option
Move the field description into the
$defsentry, or useallOf:Or keep the description only on the
$defsdefinition and drop the override here.@copilot please address this.