fix(create-issue): emit deduplicate-by-title in compiled handler config#43345
Conversation
…platableBoolOrInt Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
… field name) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
There was a problem hiding this comment.
Pull request overview
This PR wires safe-outputs.create-issue.deduplicate-by-title through to the emitted handler config and expands the configuration type system/schema to allow booleans, integers (0–100), and GitHub Actions expressions for that field.
Changes:
- Added
TemplatableBoolOrIntwith custom YAML/JSON marshal/unmarshal to accept bool/int/expression forms and emit native JSON types for literals. - Added
AddTemplatableBoolOrIntand updatedcreate-issuehandler config emission to includededuplicate_by_title. - Updated the main workflow JSON schema and added/updated tests covering bool/int/expression/nil cases.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/templatables.go | Introduces TemplatableBoolOrInt and related marshal/unmarshal helpers. |
| pkg/workflow/compiler_safe_outputs_builder.go | Adds builder support for emitting TemplatableBoolOrInt into handler config JSON. |
| pkg/workflow/create_issue.go | Changes CreateIssuesConfig.DeduplicateByTitle to *TemplatableBoolOrInt. |
| pkg/workflow/safe_outputs_handler_registry.go | Wires deduplicate_by_title into the emitted handler config. |
| pkg/workflow/safe_outputs_config_generation_test.go | Adds unit tests validating generated handler config JSON output for the new field. |
| pkg/workflow/compile_outputs_issue_test.go | Adds compile-level regression tests to ensure the lockfile contains the emitted deduplicate_by_title. |
| pkg/parser/schemas/main_workflow_schema.json | Adds $defs.templatable_bool_or_int and references it from deduplicate-by-title. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Low
| if value == nil { | ||
| return b | ||
| } | ||
| b.config[key] = value.ToValue() |
Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (407 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 MatterADRs 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
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix(create-issue): emit deduplicate-by-title in compiled handler config
Overall: The fix is correct and well-structured. The root cause (field parsed but silently dropped from the emitted handler config) is clearly addressed, and the three-form support (bool / int / GH Actions expression) is properly implemented with good test coverage.
Correctness ✅
TemplatableBoolOrIntcorrectly handles YAML booleans (!!bool), integers (!!intwith 0–100 range check), and expression strings (!!strwithisExpressionguard).ToValue()andMarshalJSON()agree:"true"→true,"false"→false, numeric string →int, expression →string.AddTemplatableBoolOrIntnil-guards correctly and mirrors the existingAddBoolOrIntbuilder pattern.- The handler registry swap from
AddBoolOrInt→AddTemplatableBoolOrIntis the minimal correct fix. - Schema
$ref+ siblingdescriptionis an established pattern throughout this schema — not a concern.
Non-blocking suggestion: The test suite covers true, 2, expression, and nil, but there is no test for explicit deduplicate-by-title: false. Setting false is valid YAML and exercises a distinct code path (ToValue() returns Go false; JSON emits false). A small companion test would close this gap.
Why this matters
false differs semantically from an absent field. The runtime may treat an explicit {"deduplicate_by_title":false} differently from the key being absent entirely. Without a test, a future refactor of ToValue() could silently break the false case. The implementation already handles it correctly; the test would just lock it in.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 46 AIC · ⌖ 4.96 AIC · ⊞ 4.9K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (7 tests)
Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd, and /codebase-design — the fix is sound and well-structured, with a few small improvements worth addressing.
📋 Key Themes & Highlights
Key Themes
- Test coverage gap:
TestDeduplicateByTitleBooleanonly verifiestrue; thefalsecode path in emission is untested - Logic duplication:
MarshalJSONreimplements the same bool/int/string dispatch already present inToValue - Minor test style:
InDeltawith delta0is less clear thanassert.Equal(t, float64(2), ...) - Schema concern: sibling
descriptionalongside$refmay be silently ignored by draft-07 validators - No unit-level tests for invalid inputs: e.g. float JSON (
1.5) or out-of-range integers rejected byUnmarshalJSON
Positive Highlights
- ✅ Excellent root-cause fix: the bug was a genuine silent data-loss in the emission path
- ✅ Clean new type design that mirrors the existing
TemplatableBool/TemplatableInt32patterns - ✅ Great test coverage for the happy paths (bool/int/expr/nil across both integration and unit layers)
- ✅ Schema extension is minimal and well-placed in
$defs - ✅ Wiring change in
safe_outputs_handler_registry.gois a one-liner — appropriately surgical
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 96.5 AIC · ⌖ 11 AIC · ⊞ 6.6K
Comment /matt to run again
| t.Fatal(err) | ||
| } | ||
|
|
||
| if !strings.Contains(string(lockContent), `\"deduplicate_by_title\":true`) { |
There was a problem hiding this comment.
[/tdd] The test name says "true/false is emitted correctly" but only true is exercised — false is never verified.
This is a meaningful edge case: false disables deduplication explicitly (vs. omitting the field), and the emission path has a separate code path for it. A regression that emits the string "false" instead of JSON boolean false would go undetected.
💡 Suggested extension
Add a companion test or subtest for deduplicate-by-title: false:
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.
|
|
||
| // MarshalJSON emits a JSON boolean for "true"/"false", a JSON integer for numeric | ||
| // strings, and a JSON string for GitHub Actions expressions. | ||
| func (t *TemplatableBoolOrInt) MarshalJSON() ([]byte, error) { |
There was a problem hiding this comment.
[/codebase-design] MarshalJSON and ToValue implement the same conversion logic independently — a future maintainer changing one is unlikely to update the other.
Both decode "true"/"false" to booleans and numeric strings to ints. Since ToValue already covers this logic, MarshalJSON could delegate to it:
💡 Suggested simplification
func (t *TemplatableBoolOrInt) MarshalJSON() ([]byte, error) {
return json.Marshal(t.ToValue())
}This eliminates the duplication and makes future changes safer — only one switch needs updating.
@copilot please address this.
| require.True(t, ok, "Expected create_issue key in config") | ||
|
|
||
| // JSON numbers unmarshal as float64 in map[string]any | ||
| assert.InDelta(t, float64(2), ciConfig["deduplicate_by_title"], 0, "deduplicate_by_title should be 2 (JSON number)") |
There was a problem hiding this comment.
[/tdd] assert.InDelta(..., 0, ...) with a delta of 0 is equivalent to == but uses float comparison — it works only because JSON unmarshals integers as float64 in map[string]any, which can be confusing.
The comment explains the float64 situation, but assert.Equal(t, float64(2), ciConfig["deduplicate_by_title"]) would be simpler and clearer than InDelta with a 0 tolerance.
💡 Alternative
assert.Equal(t, float64(2), ciConfig["deduplicate_by_title"], "deduplicate_by_title should be 2 (JSON number)")This makes the intent unambiguous and matches how other numeric assertions in this file work (e.g. the existing assert.InDelta callers all pass a non-zero delta like 0.0001).
@copilot please address this.
|
|
||
| // UnmarshalJSON allows TemplatableBoolOrInt to accept JSON booleans, JSON numbers, | ||
| // and JSON strings that are GitHub Actions expressions. | ||
| func (t *TemplatableBoolOrInt) UnmarshalJSON(data []byte) error { |
There was a problem hiding this comment.
[/diagnosing-bugs] UnmarshalJSON has a subtle ordering issue: it tries bool first, then int. In Go's encoding/json, json.Unmarshal([]byte("1"), &boolVar) fails, so the sequence is correct — but the code silently falls through when the JSON contains a floating-point number like 1.5, which would fail the int branch and reject the input with a confusing error.
This is consistent with schema-declared constraints (integers only), but no test covers the 1.5 rejection case, so there's no regression guard.
💡 Suggestion
Add a test that verifies a float JSON input returns an error:
func TestTemplatableBoolOrIntUnmarshalJSONFloat(t *testing.T) {
var v TemplatableBoolOrInt
err := json.Unmarshal([]byte("1.5"), &v)
require.Error(t, err, "float input should be rejected")
}@copilot please address this.
| } | ||
| ] | ||
| "$ref": "#/$defs/templatable_bool_or_int", | ||
| "description": "Title-based deduplication for create-issue. Set to true for exact title matching, or provide a non-negative integer (0–100) to deduplicate by Levenshtein edit distance (e.g., 1 allows one-character differences). Accepts a GitHub Actions expression that resolves to a boolean or integer at runtime. Applies within-run and against open/recently-closed repository issues." |
There was a problem hiding this comment.
[/diagnosing-bugs] Using both $ref and description on the same schema node may not behave as expected in all JSON Schema validators — the JSON Schema spec says sibling keywords alongside $ref are 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 $defs entry. Consider whether the field-specific description should live in the $defs entry itself, or if you need to use allOf to merge.
💡 Option
Move the field description into the $defs entry, or use allOf:
"deduplicate-by-title": {
"allOf": [{ "": "#//templatable_bool_or_int" }],
"description": "Title-based deduplication..."
}Or keep the description only on the $defs definition and drop the override here.
@copilot please address this.
There was a problem hiding this comment.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · 233.4 AIC · ⌖ 5.29 AIC · ⊞ 5.4K
Comment /review to run again
| t.Fatal(err) | ||
| } | ||
|
|
||
| if !strings.Contains(string(lockContent), `\"deduplicate_by_title\":true`) { |
There was a problem hiding this comment.
Missing false coverage: the docstring says this test verifies true/false but only true is exercised; false (explicit disable) is never tested anywhere in the suite.
💡 Why this matters and what to add
false and nil have distinct semantics: nil omits the field entirely (handler uses its default), while false explicitly passes deduplicate_by_title: false to the handler, disabling deduplication even if the handler would otherwise default it on. The false path through both ToValue() and MarshalJSON() is trivially correct, but untested paths drift silently.
Add a compile-level check alongside the existing true test:
// 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 safe_outputs_config_generation_test.go (alongside TestGenerateSafeOutputsConfigDeduplicateByTitleBool) would also close the gap at the config-generation layer.
|
@copilot please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in a1f8e90: added explicit |
deduplicate-by-titlewas parsed intoCreateIssuesConfigbut silently dropped from the emitted handler config — the field was never wired into the emission path. Additionally, GitHub Actions expression strings (e.g.${{ inputs.dedup }}) were rejected by schema validation and unsupported by the existingAddBoolOrIntbuilder method.Changes
New type
TemplatableBoolOrInt(templatables.go): string-backed type with custom YAML/JSON marshaling that accepts booleans, integers 0–100, and GH Actions expression strings. Integers are range-validated at parse time; expressions are stored verbatim and emitted as JSON strings for runtime evaluation.New builder method
AddTemplatableBoolOrInt(compiler_safe_outputs_builder.go): parallel to existingAddTemplatableBool/AddTemplatableInt32.CreateIssuesConfig.DeduplicateByTitle: changed fromany→*TemplatableBoolOrInt; handler registry updated to callAddTemplatableBoolOrInt.Schema (
main_workflow_schema.json): addedtemplatable_bool_or_int$defsentry (bool | int 0–100 | expression string);deduplicate-by-titleproperty updated to$refit.Example — all three forms now compile and emit correctly