dispatch_workflow: support per-call ref override for runtime branch targeting - #49408
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
ref override for runtime branch targeting
|
@copilot message.ref should be controlled by a set of ref globs in a field called allowed-refs: list of string. Otherwise, message.ref is not allowed |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Implemented in 27f6cb6. |
PR Triage
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ Test Quality Sentinel completed test quality analysis. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (36 additions detected). |
There was a problem hiding this comment.
Pull request overview
Adds safe, per-call ref overrides for dispatch_workflow, enabling agents to dispatch CI against newly created branches.
Changes:
- Adds
refandallowed-refsschemas/configuration. - Resolves and validates per-message refs before dispatch.
- Adds runtime, parser, configuration, and generated-workflow coverage.
Show a summary per file
| File | Description |
|---|---|
schemas/agent-output.json |
Adds the optional output ref. |
pkg/workflow/safe_outputs_validation_config.go |
Adds runtime field validation. |
pkg/workflow/safe_outputs_handler_registry.go |
Emits allowed-ref configuration. |
pkg/workflow/safe_outputs_cross_repo_config_test.go |
Tests configuration parsing and emission. |
pkg/workflow/dispatch_workflow.go |
Parses allowed-refs. |
pkg/parser/schemas/main_workflow_schema.json |
Defines the frontmatter field. |
pkg/parser/schema_safe_outputs_target_test.go |
Tests schema acceptance. |
actions/setup/js/dispatch_workflow.test.cjs |
Tests override priority, normalization, and rejection. |
actions/setup/js/dispatch_workflow.cjs |
Implements ref normalization and allowlisting. |
.github/workflows/smoke-copilot.lock.yml |
Regenerates validation configuration. |
.github/workflows/smoke-copilot-arm.lock.yml |
Regenerates ARM validation configuration. |
.github/workflows/smoke-copilot-aoai-entra.lock.yml |
Regenerates Entra validation configuration. |
.github/workflows/smoke-copilot-aoai-apikey.lock.yml |
Regenerates API-key validation configuration. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 13/13 changed files
- Comments generated: 2
- Review effort level: Balanced
| "allowed-refs": { | ||
| "description": "List of allowed ref glob patterns for per-call dispatch_workflow message.ref overrides. Supports arrays and GitHub Actions expressions resolving to a comma-separated list (e.g. '${{ inputs['allowed-refs'] }}').", |
| Fields: map[string]FieldValidation{ | ||
| "workflow_name": {Required: true, Type: "string", Sanitize: true, MinLength: 1, MaxLength: 256, Pattern: ".*\\S.*", PatternError: "must not be empty"}, | ||
| "inputs": {Type: "object"}, | ||
| "ref": {Type: "string", Sanitize: true, MinLength: 1, MaxLength: 256, Pattern: ".*\\S.*", PatternError: "must not be empty"}, |
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. 🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (4 tests)
✅ Test SummaryAll 4 new JavaScript tests in
Go test extensions ( Verdict
|
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Review: dispatch_workflow per-call ref override
The overall design is solid — using an allowed-refs allowlist gated at config time mirrors the existing allowed-repos pattern, and the priority chain (message.ref → config target-ref → env → default branch) is clearly reasoned.
One correctness issue found: normalizeRef / normalizeRefPattern only check for the refs/ prefix, so bare short forms like "tags/v*" are incorrectly expanded to "refs/heads/tags/v*" instead of "refs/tags/v*". The one test with a full tag ref (refs/tags/v1.2.3) passes because it already starts with refs/, masking this gap. See inline comment for a fix suggestion.
Everything else looks good: validation config entries are correct, the schema additions are consistent, and the test coverage for the happy path and rejection cases is thorough.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 48.2 AIC · ⊞ 5.3K
| core.warning(error); | ||
| return { | ||
| success: false, | ||
| error, |
There was a problem hiding this comment.
normalizeRefPattern silently misroutes short tag patterns
normalizeRefPattern("tags/*") returns "refs/heads/tags/*" instead of "refs/tags/*", so an allowed-refs entry like ["tags/*"] will never match a real tag ref after normalization.
Note that normalizeRef has the same gap: normalizeRef("v1.2.3") → "refs/heads/v1.2.3", which also would not match a tag ref. This inconsistency means users writing short-form tag patterns in allowed-refs get silently wrong matching.
Consider checking the tags/ prefix:
function normalizeRef(refOrBranch) {
if (refOrBranch.startsWith("refs/")) return refOrBranch;
if (refOrBranch.startsWith("tags/")) return `refs/${refOrBranch}`;
return `refs/heads/${refOrBranch}`;
}At minimum, add a test that covers bare tags/v* patterns in allowed-refs so the current behavior is explicit and not accidentally "fixed" later.
@copilot please address this.
There was a problem hiding this comment.
Fixed in the latest commit. Both normalizeRef and normalizeRefPattern now check for the tags/ prefix before defaulting to refs/heads/:
function normalizeRef(refOrBranch) {
if (refOrBranch.startsWith("refs/")) return refOrBranch;
if (refOrBranch.startsWith("tags/")) return `refs/${refOrBranch}`;
return `refs/heads/${refOrBranch}`;
}A test case covering tags/v* in allowed-refs and a tags/v1.2.3 message ref was added to dispatch_workflow.test.cjs.
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — no blocking issues, but three targeted improvements to edge-case coverage and clarity.
📋 Key Themes & Highlights
Positive Highlights
- ✅ Excellent overall test coverage — 130 new test cases covering the happy path, ref priority, normalization, and rejection scenarios
- ✅ Clean security gate:
allowed-refsis required before per-callrefis honoured — fail-safe by default - ✅ Normalization logic (
normalizeRef/normalizeRefPattern) is well-factored and independently testable - ✅ Cross-layer consistency: Go config parsing, JSON schema, validation config, and JS handler all add the new field in lock-step
- ✅ Priority ordering is clearly documented and tested
Issues Found (3 inline comments)
- Empty
allowed-refs: []misleading error —allowedRefRegexes.length === 0conflates "not configured" with "configured but empty"; the wrong error message is emitted in the latter case. - Bare-branch rejection path not tested — all rejection tests use full
refs/heads/...inputs; no test exercises a bare branch name being normalized then rejected. - Silent non-string
message.ref— a non-string truthyreffield is silently ignored; worth a warning and a documenting test.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 59.6 AIC · ⊞ 7K
Comment /matt to run again
Comments that could not be inline-anchored
actions/setup/js/dispatch_workflow.cjs:135
[/tdd] Empty allowed-refs: [] gives a misleading error: the current check allowedRefRegexes.length === 0 cannot distinguish "not configured" from "configured but empty array", so the error message is wrong in the latter case. No test covers this edge.
<details>
<summary>💡 Suggested fix</summary>
Track whether allowed-refs was supplied at all:
const allowedRefsConfigured = config.allowed_refs !== undefined;
const allowedRefPatterns = parseAllowedRefPatterns(config.allowed_ref…
</details>
<details><summary>actions/setup/js/dispatch_workflow.cjs:143</summary>
**[/tdd]** The `normalizeRef` call on line 134 happens *before* the `allowed-refs` check, but the error message on line 144 reconstructs the matched ref from `allowedRefPatterns` joined string, not the normalized ref. If a user provides `refs/heads/feature/new-ui` and the pattern is `refs/heads/release/*`, the error is clear — but if they provide a bare branch name like `feature/new-ui` the normalization silently changes it before validation, which is fine, but none of the test cases cover a ba…
</details>
<details><summary>actions/setup/js/dispatch_workflow.cjs:90</summary>
**[/codebase-design]** `allowedRefPatterns` and `allowedRefRegexes` are computed once at the top of `main()` from `config.allowed_refs`, but they're used inside the per-message loop. This is correct and efficient, but the naming is slightly asymmetric: `allowedRefPatterns` holds normalized pattern strings while `allowedRefRegexes` holds compiled regexes — consider naming them `normalizedAllowedRefPatterns` / `allowedRefRegexes` or grouping them into a single `allowedRefs` object so the relation…
</details>
<details><summary>actions/setup/js/dispatch_workflow.cjs:131</summary>
**[/tdd]** When `message.ref` is a non-string truthy value (e.g., `message.ref = 42` or `true`), the `typeof` guard correctly produces `outputRef = ""` and falls through to `defaultRef`. This is the right defensive behaviour, but it is silent — the message payload had a `ref` field that was ignored without a warning. Consider adding `core.warning` for non-string non-null `message.ref` values, and add a test to document the intended behaviour.
@copilot please address this.
</details>|
@copilot run pr-finisher skill |
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. @copilot please run the
|
…malization, update docs - Remove Sanitize: true from dispatch_workflow ref field validation (prevents @-chars in refs like refs/heads/release/@candidate from being mangled by markdown sanitizer); use a git-ref-safe pattern instead - Fix normalizeRef/normalizeRefPattern to expand tags/ prefix to refs/tags/ instead of refs/heads/ (silent misrouting bug for short-form tag patterns in allowed-refs) - Add test coverage for tags/ prefix normalization via allowed-refs - Add allowed-refs and per-call ref documentation to safe-outputs.md (Configuration section + new Per-call Ref Override section with ref resolution priority table) - Add allowed-refs to frontmatter-full.md dispatch-workflow config block - Regenerate affected .lock.yml files to reflect ref validation schema change Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Completed the pr-finisher pass. Here's the summary:
Actions taken: removed markdown sanitizer from ref field, fixed tags/ normalization, added test, updated docs, regenerated lock files |
…y allowed-refs, bare-branch rejection Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in the latest commit:
Pre-existing CI failure: |
|
@pelikhan Wow! Thank you for the quick turnaround time! GitHub Agentic Workflows are pretty convenient to deploy so far. Very cool project here. |
|
Keep bringing the feedback , we'll try to eat it right away |
|
🎉 This pull request is included in a new release. Release: |
…on its own PR branches (tenstorrent#51834) ## Summary Two related changes: bump the gh-aw toolchain from **v0.84.0 to [v0.84.2](https://github.com/github/gh-aw/releases/tag/v0.84.2)**, and use the new capability that release ships to wire up Silencer's CI validation — scoped to exactly one PR per run, by design. This closes the loop on github/gh-aw#49400, the feature request filed from this repo, fixed by **github/gh-aw#49408** and released in v0.84.2. It also surfaced a doc/behavior mismatch upstream, filed as github/gh-aw#49600 (not blocking, unrelated to what shipped here). ## Part 1 — gh-aw v0.84.2 `github/gh-aw-actions` is repinned from `f3ca209` (v0.84.0) to `fd783ac87efde5e0c0e05d593f1906ea25b5d92e` (v0.84.2) everywhere it appears: | File | How | | --- | --- | | `.github/workflows/copilot-setup-steps.yml` | hand-edited (`uses:` SHA + comment, and `with: version:`) — this one is hand-authored | | `.github/aw/actions-lock.json` | regenerated (`setup`, `setup-cli`) | | `.github/workflows/agentics-maintenance.yml` | regenerated | | the four `*.lock.yml` files | regenerated | Done with `gh aw upgrade` + `gh aw compile` on v0.84.2 — no generated file was hand-edited. `compiler_version` now reads `v0.84.2` in all four lock files, and no `v0.84.0` pin remains under `.github/`. The recompile also rolls the compiler-bundled container images forward, all still digest-pinned: `gh-aw-firewall` 0.27.42 → 0.27.43, `gh-aw-mcpg` v0.4.6 → v0.4.7, `github-mcp-server` v1.7.0 → v1.8.0. `gh aw upgrade` bumped only the two `gh-aw-actions` entries — no unrelated third-party actions were touched. `.github/skills/agentic-workflows/SKILL.md` picks up three new entries in the dispatcher skill's doc list. ## Part 2 — Silencer can now validate its own fix, one PR at a time Silencer opens noise-fix PRs but had no way to get CI to run on them. `dispatch-workflow` had no per-call `ref`, so it could not target the branch Silencer had just created. `## Validating changes via CI` was a placeholder admitting as much: > We should attempt a fresh run of the targeted workflow with our PR branch. However, that is not working yet. Thus, nothing to do at the moment. github/gh-aw#49408 added an `allowed-refs` glob allowlist that gates a per-call `message.ref`. New frontmatter: ```yaml safe-outputs: create-pull-request: ... max: 1 push-to-pull-request-branch: ... max: 1 create-issue: ... max: 1 dispatch-workflow: workflows: - sanity-tests - blackhole-sanity-tests - blackhole-e2e-tests - blackhole-demo-tests - galaxy-profiler-tests - galaxy-multi-user-isolation-tests - galaxy-deepseek-tests - galaxy-perf-tests - galaxy-demo-tests - galaxy-unit-tests - galaxy-integration-tests - galaxy-stress-tests - galaxy-e2e-tests - galaxy-sanity - galaxy-health - t3000-perf-tests - t3000-e2e-tests - t3000-integration-tests - t3000-profiler-tests - single-card-profiler-tests - pipeline-select-profiler - t3000-demo-tests - t3000-unit-tests - models-t1-e2e-tests - models-t1-unit-tests - models-t2-e2e-tests - models-t2-unit-tests - models-t3-e2e-tests - models-t3-unit-tests - perf-device-models - perf-models - single-card-ttnn-models-frequent-tests - single-card-demo-tests - tt-metal-l2-nightly - ttnn-run-sweeps - vllm-nightly-tests - metal-run-microbenchmarks - sanity-tests-debug - merge-gate - pr-gate - runtime-sanity-tests - runtime-unit-tests - runtime-integration-tests - runtime-perf-tests allowed-refs: ["silencer/*"] max: 1 ``` **`workflows:`** is all 44 workflows currently tracked in `aggregate-workflow-data.yaml`'s `workflow_ids`, as bare stems, listed in the same order (including the blank-line grouping) so the two lists can be diffed by eye. Note the asymmetry, called out in a comment in the file: Silencer's *runtime* scan-target list is read out of `aggregate-workflow-data.yaml` on every run precisely so there is "no parallel list to drift", but this one is a **compile-time** allowlist and cannot be derived dynamically — **it has to be kept in sync by hand.** If a workflow is added there and not here, Silencer can still fix noise in it but cannot validate the fix (and is instructed to say so in the PR rather than dispatch something else). **`allowed-refs: ["silencer/*"]`** is the safety boundary. It normalizes to `refs/heads/silencer/*`, which matches exactly the `silencer/<category>-<short-desc>` branches Silencer names itself — so it can dispatch CI onto branches it created and nothing else: not `main`, not a release branch, not a maintainer's branch. A non-matching ref **fails at runtime** rather than silently falling back to the default branch, and without this field any per-call `ref` is refused outright. `target-ref` is deliberately left unset: it is a single static string, and Silencer's branch differs every run. **`max: 1` everywhere, on purpose.** Silencer was originally set up to allow up to 3 PRs/targets per run (matching an earlier "occasionally two if trivially related" allowance in the scan procedure). That's been walked back: Silencer now handles **exactly one PR/target per run** — simpler to reason about, and it removes any chance of a turn opening several PRs but only being able to validate the first one. `create-pull-request`, `push-to-pull-request-branch`, `create-issue`, and `dispatch-workflow` are all explicitly `max: 1`. This had to be explicit rather than omitted on all four: while `create-pull-request`/`create-issue`/`dispatch-workflow` do default to `1` when the key is left out, `push-to-pull-request-branch` renders with **no `max` key at all** when omitted (confirmed by compiling probe workflows and diffing the generated config) — so one of the four would have been silently unbounded if we'd relied on defaults instead of setting all four explicitly. `update-issue` was already `max: 1`; `add-comment` stays `max: 5` since it covers incidental comments on unrelated existing issues/PRs found during triage, not "how many targets this run touches." ### Prompt body - Scan procedure: "select ONE high-value target" — the prior "(occasionally two if trivially related)" allowance is gone. - `## Validating changes via CI` rewritten with concrete instructions: in the same turn as the PR output, dispatch the same tracked workflow whose logs motivated the fix, with `ref` set to that PR's exact branch string, exactly once. - It is explicit about the limits of that evidence — the dispatched run confirms **compilation** (categories 1–2 and 4), but a runtime or log-spam pattern (categories 3/5/6) is only confirmed gone by re-grepping the dispatched run's own logs, which a later run does. Silencer's turn ends before the run completes, so it has no run ID or outcome to cite, exactly like the PR itself. A maintainer should check the dispatched run's outcome. - Scan procedure step 7, the **Test Status** PR-body requirement, and the "Validate via CI" guideline updated to match. All "not working yet" / "needs a maintainer to trigger" / "up to 3" language is gone. - The `strict: false` rationale was re-verified rather than assumed: test-compiling with `strict: true` reports `tools.github.private-to-public-flows` as the *only* violation, so this change costs no additional strict property. Comment updated to record that, and to note the agent job stays read-only while the `actions: write` a dispatch needs is auto-granted to the generated `safe_outputs` job. ## Verification - `gh aw compile` and `gh aw validate` both succeed on v0.84.2 for all four agentic workflows (`silencer`, `ci-failure-triage`, `daily-repo-status`, `repo-assist`). - The compiled `silencer.lock.yml` carries `"allowed_refs":["silencer/*"]`, `"max":1` on all four affected safe-outputs, and all **44** workflow stems resolved to their real `.yaml` files. - All 44 tracked workflows were confirmed to declare a `workflow_dispatch` trigger, which this safe-output requires. - The agent job's compiled permissions remain read-only (`actions: read`); `actions: write` appears only on the `safe_outputs` job. Adding `actions: write` to the agent job is still hard-rejected by the compiler in v0.84.2 ("The agent job must not have write permissions"), so it was not added. - One pre-existing compile warning on `silencer.md` ("Prompt references /tmp/ directly") is unchanged by this PR — identical count on `main`. - Diff for the simplification pass was confined to exactly `silencer.md` and `silencer.lock.yml` — no drift picked up from `main`. ## Notes for reviewers - `agentics-maintenance.yml` is regenerated by `gh aw compile`, but **not** by `gh aw upgrade` — its post-processing skips the maintenance generator, so the file silently stayed at v0.84.0 until a plain `compile` was run. Worth knowing for future bumps. - `gh aw lint` (actionlint) could not run in this environment because it requires Docker. `compile` and `validate` both passed. - Unrelated and left alone: a prose comment in `repo-assist.md` still says "re-verified on v0.84.0" about the compiler rejecting a bare `awmgmcpg` token. Recompiling on v0.84.2 does not re-test that specific claim, so the note was not edited. - Filed upstream but not blocking: github/gh-aw#49600, a docs/implementation mismatch on `dispatch-workflow`'s default `max` (spec doc says 3, code says 1) — moot for this PR since every relevant safe-output here is explicit, but worth a heads-up for anyone else configuring `dispatch-workflow` from the docs alone. ### CI Status _Auto-generated on every push. Badges update live. Click a badge to filter runs by this branch._ - [](https://github.com/tenstorrent/tt-metal/actions/workflows/sanity-tests.yaml?query=branch:chore/gh-aw-v0.84.2-silencer-dispatch) - [](https://github.com/tenstorrent/tt-metal/actions/workflows/runtime-sanity-tests.yaml?query=branch:chore/gh-aw-v0.84.2-silencer-dispatch) - [](https://github.com/tenstorrent/tt-metal/actions/workflows/blackhole-sanity-tests.yaml?query=branch:chore/gh-aw-v0.84.2-silencer-dispatch) - [](https://github.com/tenstorrent/tt-metal/actions/workflows/tt-metal-l2-nightly.yaml?query=branch:chore/gh-aw-v0.84.2-silencer-dispatch) - [](https://github.com/tenstorrent/tt-metal/actions/workflows/all-model-tests.yaml?query=branch:chore/gh-aw-v0.84.2-silencer-dispatch) - [](https://github.com/tenstorrent/tt-metal/actions/workflows/pipeline-select.yaml?query=branch:chore/gh-aw-v0.84.2-silencer-dispatch) - [](https://github.com/tenstorrent/tt-metal/actions/workflows/pipeline-select-t3k.yaml?query=branch:chore/gh-aw-v0.84.2-silencer-dispatch) - [](https://github.com/tenstorrent/tt-metal/actions/workflows/pipeline-select-galaxy.yaml?query=branch:chore/gh-aw-v0.84.2-silencer-dispatch) --------- Co-authored-by: tt-metal automation <noreply@tenstorrent.com> Co-authored-by: BrAIn <brain@tenstorrent.com>
dispatch_workflowcould only resolvereffrom static/frontmatter or runner context, which blocks agent flows that create a branch and immediately dispatch CI against that branch in the same turn. This change adds a per-callrefon the safe output payload and makes it the highest-priority ref source.Safe output contract
reftoDispatchWorkflowOutputinschemas/agent-output.json.Validation config parity
dispatch_workflow.reffield rules inpkg/workflow/safe_outputs_validation_config.go(non-empty string, sanitized/pattern-checked) so runtime accepts and validates the new payload key.Dispatch ref resolution behavior
actions/setup/js/dispatch_workflow.cjsto resolverefper message:message.ref(highest priority)config["target-ref"]GITHUB_HEAD_REFGITHUB_REF || context.refrefs/heads/<name>; full refs (e.g.refs/tags/...) are used as-is.Behavioral coverage
actions/setup/js/dispatch_workflow.test.cjswith cases for:refoverriding configured/env refsWarning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Run details: https://github.com/github/gh-aw/actions/runs/30678610470