Uh oh!
There was an error while loading. Please reload this page.
Normalize dispatch-repository nested keys to kebab-case while preserving snake_case aliases#44942
dispatch-repository nested keys to kebab-case while preserving snake_case aliases#44942Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
dispatch-repository nested keys to kebab-case while preserving snake_case aliases🤖 PR Triage
Rationale: Normalizes
|
pelikhan
commented
Jul 11, 2026
@copilot make dispatch-positive NON experimental |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Implemented in |
Thanks for the PR — the change looks on-topic and reasonably focused, with test coverage and a clear description. One process issue blocks alignment with this repository's contribution guidelines: this PR appears to have been opened directly by a non-core author association ( Please convert this work into an issue or have a core team member recreate/sponsor the PR path.
|
There was a problem hiding this comment.
Pull request overview
Normalizes dispatch-repository nested keys to kebab-case while retaining snake_case aliases.
Changes:
- Adds canonical-key parsing, precedence, schema validation, and tests.
- Updates validation messages and references.
- Also removes the feature’s experimental warning/status.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/dispatch_repository.go | Parses canonical keys with alias fallback. |
pkg/workflow/dispatch_repository_validation.go | Updates validation guidance. |
pkg/workflow/dispatch_repository_test.go | Adds precedence and fallback tests. |
pkg/workflow/dispatch_repository_experimental_warning_test.go | Expects no experimental warning. |
pkg/workflow/compiler_validators.go | Removes the experimental warning. |
pkg/parser/schemas/main_workflow_schema.json | Defines canonical keys and aliases. |
docs/src/content/docs/reference/safe-outputs.md | Updates configuration reference. |
docs/src/content/docs/reference/glossary.md | Updates terminology and feature status. |
docs/src/content/docs/reference/frontmatter-full.md | Updates generated schema reference. |
.github/aw/safe-outputs-automation.md | Removes experimental labeling. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 10/10 changed files
- Comments generated: 3
- Review effort level: Medium
| // TestDispatchRepositoryNoExperimentalWarning tests that dispatch-repository no longer emits an experimental warning. | ||
| func TestDispatchRepositoryNoExperimentalWarning(t *testing.T) { |
| Triggers other agentic workflows using workflow_dispatch. Agent output includes `workflow_name` (without .md extension) and optional `inputs` (key-value pairs). Cross-repo dispatch is supported via `target-repo` plus an `allowed-repos` allowlist; cross-repo targets require a token with `actions: write` on the target repository. | ||
| - `dispatch-repository:` - Dispatch `repository_dispatch` events to external repositories (experimental) | ||
| - `dispatch-repository:` - Dispatch `repository_dispatch` events to external repositories |
| }, | ||
| } | ||
| config := compiler.parseDispatchRepositoryConfig(outputMap) |
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
✅ Test Quality Sentinel completed test quality analysis. |
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Review: Normalize dispatch-repository nested keys to kebab-case
The implementation is correct and well-tested. The canonical-first fallback pattern in parseDispatchRepositoryConfig is clean, the schema anyOf change properly enforces that at least one form of event-type/event_type is present, and the new tests cover canonical, alias, and precedence cases.
One minor observation (non-blocking): The validation error messages still prefix errors with dispatch_repository: (snake_case) while the YAML examples now correctly use dispatch-repository:. This is internally consistent (it's an error prefix, not a YAML key), but it might briefly confuse users comparing the error message prefix to the frontmatter key. Consider aligning to dispatch-repository: in a follow-up if consistency is a goal.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 16.4 AIC · ⌖ 4.33 AIC · ⊞ 4.8K
…ch-repository nested keys Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Design Decision Gate - ADR RequiredThis PR makes significant changes to core business logic (161 new lines in Draft ADR committed: This PR cannot merge until an ADR is linked in the PR body. 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
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 92/100 — Excellent
📊 Metrics (1 test modified)
i️ ContextModified:
Not modified in current commit:
Verdict
Test modifications cleanly reflect the feature graduation from experimental to stable. The single modified test is well-structured, design-focused, and includes proper edge-case coverage. No concerns with test quality or infrastructure. 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 /codebase-design and /tdd — requesting changes on two issues.
📋 Key Themes & Highlights
Issues
Error prefix inconsistency (
dispatch_repository_validation.go): All six error messages usedispatch_repository:as prefix while the body of those same messages uses canonicaldispatch-repository:. This leaks the old snake_case form precisely where users are most likely to see it.Test fixtures still use legacy keys (
dispatch_repository_experimental_warning_test.go, lines 34 & 64–65): The no-experimental-warning test suite exercisesevent_type/allowed_repositoriesalias paths, not the new canonicalevent-type/allowed-repositoriespaths. This misses testing that canonical keys compile clean.
Positive Highlights
- ✅ Canonical-first fallback logic in
parseDispatchRepositoryConfigis clean and explicit. - ✅ Comprehensive unit tests added for all four alias/canonical combinations.
- ✅
anyOfschema approach for backward compatibility is correct and expressive. - ✅ Removing the experimental caution banner is a good signal of feature maturity.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 48 AIC · ⌖ 4.67 AIC · ⊞ 6.6K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/workflow/dispatch_repository_validation.go:29
[/codebase-design] Error message prefixes throughout this file still say dispatch_repository: (snake_case) while all examples and field references in the same messages now use canonical dispatch-repository: — this inconsistency will confuse users who copy-paste error output.
<details>
<summary>💡 Suggested fix</summary>
Change the six dispatch_repository: error prefixes in this file to dispatch-repository:, e.g.:
// beforereturn errors.New("dispatch_repository: mustspecif…
</details><details><summary>pkg/workflow/dispatch_repository_experimental_warning_test.go:34</summary>**[/tdd]**Testfixturesinthisfilestillusesnake_casekeys (`event_type`, `allowed_repositories`) — theyareexercisingthelegacyaliaspath, notthecanonicalkebab-case path. Thisleavesthecanonicalpathuntestedintheexperimentalwarningsuite.
<details>
<summary>💡 Suggestedfix</summary>
UpdatetestYAMLcontenttouse `event-type` and `allowed-repositories` to prove canonical keys compile without warnings:```yamlsafe-outputs: dispatch-repository: notify-service: …</details><details><summary>pkg/workflow/dispatch_repository_validation.go:48</summary>**[/tdd]** Validation error for missing `event-type` does not mention that either canonical OR alias form is accepted. A user who writes `event_type` and makes a typo would see an error that only suggests `event-type`, missing the hint that `event_type` is also valid.<details><summary>💡 Suggested fix</summary>The message already says `(alias: 'event_type')` which is good — this comment is confirming it's correct as-is. However there's no test that verifies a config with *no event-type fie…
</details>There was a problem hiding this comment.
Non-blocking observations — safe to merge
The core logic is correct: canonical-first lookup with alias fallback, schema updated to accept either form, tests covering all four cases (canonical-only, alias-only, canonical-over-alias, alias-over-canonical). The removal of the experimental warning is appropriately reflected everywhere.
Findings (all medium/low, non-blocking)
Inconsistent error prefix (
dispatch_repository:vs the canonicaldispatch-repository:) — error messages indispatch_repository_validation.goupdated their example YAML bodies to kebab-case but left the leading label in snake_case. Confusing for users who read the error and then look at the docs.anyOfrequired schema — theanyOfwith two single-field branches is fragile. It works now, but the semantics are not obvious and future schema changes could silently weaken the constraint. Worth a clarifying comment or a refactor tooneOf.GetWarningCount()called twice in the test assertion — minor, but worth a local variable for clarity and safety.
None of these are correctness bugs or security issues.
🔎 Code quality review by PR Code Quality Reviewer · 41.2 AIC · ⌖ 4.7 AIC · ⊞ 5.4K
Comment /review to run again
Comments that could not be inline-anchored
pkg/workflow/dispatch_repository_experimental_warning_test.go:325
GetWarningCount() called twice in assertion — second call may observe stale state if the method has side effects or if compiler state is mutated between calls.
<details>
<summary>💡 Suggested fix</summary>
Extract to a local variable so both the condition and the error message reflect the same value:
warnCount:=compiler.GetWarningCount()
ifwarnCount!=0 {
t.Errorf("Expectedwarningcounttobe0, got%d", warnCount)
}While GetWarningCount() is likely a pure read t…
pkg/workflow/dispatch_repository_validation.go:489
Error prefix dispatch_repository: (snake_case) is inconsistent with the now-canonical kebab-case key dispatch-repository — users reading this error will be confused about the correct spelling.
<details>
<summary>💡 Suggested fix</summary>
Change the error prefix to match the canonical key in all error messages in this file:
// before"dispatch_repository: mustspecifyatleastonedispatchtool..."
// after"dispatch-repository: mustspecifyatleastonedispatchtool..."…
pkg/parser/schemas/main_workflow_schema.json:188
anyOf required-field validation allows a config with neitherevent-type nor event_type to pass schema validation if other anyOf branches are added in the future — the current structure is also semantically misleading.
<details>
<summary>💡 Suggested fix</summary>
The anyOf enforces that at least one ofevent-type or event_type is present. This is correct for backward compatibility. However, consider using oneOf or documenting that both-present is valid (canonical wins)…
safe-outputs.dispatch-repositorywas the only area mixing key styles: dashed parent key with snake_case nested fields. This change makes nested keys canonical in kebab-case (event-type,allowed-repositories) while retaining snake_case aliases for backward compatibility.Schema: canonical keys + compatibility aliases
main_workflow_schema.jsonto defineevent-typeandallowed-repositoriesas canonical nested fields.event_typeandallowed_repositoriesas deprecated-compatible aliases.event-typeorevent_type, and documented canonical-key precedence when both are present.Parser/runtime: canonical-first resolution
parseDispatchRepositoryConfigto read kebab-case first, then fallback to snake_case aliases.Validation and docs: aligned terminology
Focused behavior coverage