Accept $/ self-repository references in action manifests - #4669
Open
nodeselector wants to merge 2 commits into
Open
Accept $/ self-repository references in action manifests#4669nodeselector wants to merge 2 commits into
nodeselector wants to merge 2 commits into
Conversation
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The functional change is narrowly scoped, aligns behavior with legacy parsing, and is covered by targeted regression tests (only minor test/comment nits remain).
Review tier: Lite
Findings: 2
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
 |
src/Test/L0/Worker/ActionManifestManagerL0.cs — Using (… as ActionStep).Uses can throw a NullReferenceException if the step isn’t an ActionStep,… |
|
src/Test/L0/Worker/ActionManifestParserComparisonL0.cs — The comment has a duplicated “otherwise”, which reads like a typo and makes the intent harder to… |
What changed in this PR
This PR updates the new action-manifest parsing path to accept $/... self-repository action references (matching legacy behavior) so valid nested self-references don’t produce “Expected format …@ref” template errors, and adds regression coverage to keep both parser paths consistent.
Changes:
- Allow
$/self-repositoryuses:references to bypass external-repo format validation in the new workflow template converter path. - Teach
ActionManifestManagerWrapperto parse$/...intoselfRepositoryreferences (folding any@refinto the path to match legacy behavior). - Add L0 tests + testdata manifest covering
$/references (including a$/foo@v1case).
| File | Description |
|---|---|
| src/Test/TestData/self_repository_composite_action.yml | Adds a composite action manifest fixture with $/ self-repository references. |
| src/Test/L0/Worker/ActionManifestParserComparisonL0.cs | Adds a regression test asserting legacy/new parser agreement for $/ references. |
| src/Test/L0/Worker/ActionManifestManagerL0.cs | Adds an L0 test ensuring the new manager loads $/ composite actions without format errors. |
| src/Sdk/WorkflowParser/Conversion/WorkflowTemplateConverter.cs | Skips {org}/{repo}[/path]@ref validation for $/... self-repo references. |
| src/Runner.Worker/ActionManifestManagerWrapper.cs | Parses $/... into selfRepository RepositoryPathReference objects. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
kyle-sexton
added a commit
to melodic-software/ci-workflows
that referenced
this pull request
Sep 5, 2026
checks.yml references its composites by full path at a pinned SHA, because a relative action path inside a called workflow resolves against the caller's checkout. The dogfood call in ci.yml therefore ran nine composite bodies at 449157a rather than at the commit under test, so a pull request that broke one of their run.sh bodies could pass this repository's own CI. That is deviation 9 of the Phase 6a report. The new composites-head job runs typos, gitleaks, editorconfig, markdown, exec-bit, machine-specific-paths, eol-renormalize, comment-hygiene and lychee-offline through ./.github/actions/<x>, with the same exclusions the checks caller passes. actionlint, shellcheck and check-jsonschema are absent because each already runs at HEAD in its own job. The job copies its sibling lanes: ubuntu-24.04, needs changes, a fifteen minute budget, the contract-only gate, and the pinned checkout with persist-credentials false. It gates on the actionlint change-detection group, the narrowest group naming composite actions; every group in that job includes .github/** so any composite edit selects the lane. ci-status gains the lane in both its needs list and its aggregated results. GitHub's $/ self-repository syntax would remove the pin lag and this job together, but actionlint 1.7.12, which this repository's own actionlint composite pins, rejects it as an invalid format with a missing ref. Phase 6b promotes it once actionlint ships the support of rhysd/actionlint#732 in a version this repository pins, actions/runner#4669 merges, and one cross-repository $/ run is measured. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019APkoJh8NVN773d82fBBSG
kyle-sexton
added a commit
to melodic-software/ci-workflows
that referenced
this pull request
Sep 5, 2026
No related issue: melodic-software/github-iac#378 tracks the ci-perf program (Phase 6a) ## Summary `checks.yml` is the consolidated hygiene lane the ci-perf program's Phase 6a specifies, and the one the private repositories adopt in Phase 4: **one job, one runner spin-up**. It runs `change-detection` once against caller-supplied `filters`, then each content-agnostic composite as a step, and publishes `results` (the detection JSON, verbatim) and `outcome`. - Twelve boolean toggles (`typos`, `gitleaks`, `editorconfig`, `markdown`, `shellcheck`, `actionlint`, `exec-bit`, `machine-specific-paths`, `eol-renormalize`, `comment-hygiene`, `lychee-offline`, `check-jsonschema`), all default `true` except `check-jsonschema`. - Each step is skipped when the caller declares a filter group **named after its toggle** and that group evaluated `false`. An undeclared group leaves the composite ungated: fail-open is the detection contract. - Every composite runs under `continue-on-error: true`; one join step reports each outcome, names the first failure, and fails the job. Run-everything, fail-at-end — one failing tool never hides the rest. - `permissions` are `contents: read` plus `pull-requests: read` (the detection pass reads the pull request's file listing). A called workflow cannot elevate, so the caller's job block grants both. - `runner` is required with no default: a hosted default would silently bill a private caller's pool, which is the placement accident this consolidation exists to end. `timeout-minutes` defaults to `15`, the per-tool reusable value. Composites stay the unit of reuse — the reusable never calls a per-tool reusable workflow, which is another job and another spin-up. Two composites of the plan's thirteen are not toggles: - **`zizmor`** has no composite, only `.github/workflows/zizmor.yml`, which needs `security-events: write` and an optional SARIF upload. Callers keep a separate job (this repository does). - **`check-jsonschema`** defaults off: its `files` input is required with no universal default, and one call validates one schema family. **A `composites-head` job keeps the moved composites covered at HEAD.** Because `checks.yml` can only reach its composites at a pinned SHA (below), this repository's dogfood ran nine composite bodies at `449157aaa8e30f7b1457305d8048ebe6168e174a`, not at the commit under test, so a pull request breaking one of their `run.sh` bodies could have gone green here. The new job (display name "Composites at HEAD") runs `typos`, `gitleaks`, `editorconfig`, `markdown`, `exec-bit`, `machine-specific-paths`, `eol-renormalize`, `comment-hygiene` and `lychee-offline` through `./.github/actions/<x>` with the same exclusions the `checks` caller passes. `actionlint`, `shellcheck` and `check-jsonschema` are absent because each already runs at HEAD in its own job. `ci-status` aggregates the lane. **GitHub's `$/` self-repository syntax is deferred to Phase 6b.** It would resolve against the workflow's own repository and remove both the pin lag and the extra job, but actionlint 1.7.12, the version this repository's own `actionlint` composite pins, rejects it: `specifying action "$/..." in invalid format because ref is missing`. Promotion needs three things: actionlint shipping the `$/` support of rhysd/actionlint#732 in a version this repository pins, actions/runner#4669 (composite manifests) merging, and one measured cross-repository `$/` run. ## Fix The composites are referenced by full path at a pinned SHA, never `./.github/actions/...`. Inside a **called** workflow a relative action path resolves against the **caller's** checkout ([actions/runner#1348][runner-1348]), so a relative reference would fail in every consumer. The pins therefore lag one release behind the workflow carrying them; Dependabot's existing `github-actions` group bumps them like any other `uses:`. The `composites-head` job is the counterweight to that lag inside this repository, and the `checks.yml` header records both the reason it exists and the Phase 6b promotion trigger. Dogfood in this repository's own `ci.yml`: the `checks` job replaces the `typos`, `gitleaks`, `markdown`, `links` and `hygiene` jobs and the `Validate dependabot.yml` step, and `ci-status` aggregates `needs.checks.result` in their place. `actionlint` and `shellcheck` keep their own jobs with those toggles off — each carries extra dogfood steps that run against the pinned binary the composite installs, which no `uses:` in a shared reusable can hand back. The `comment-hygiene` prefilter superset test moves to `selector-contract`, since a shared reusable cannot run a repo-local script. `changes` stays for the language and toolchain lanes this repository fans out and Phase 6b converges; the `checks` lane runs its own detection pass over the groups its composites are gated on. `composites-head` copies its sibling lanes (`ubuntu-24.04`, `needs: changes`, `timeout-minutes: 15`, the contract-only gate, the pinned checkout with `persist-credentials: false`) and gates on the `actionlint` change-detection group, the narrowest group naming composite actions; every group in `changes` includes `.github/**`, so any composite edit selects the lane. `.github/scripts/checks-outcome.test.cjs` extracts the join's `run:` block with `workflow-yaml.cjs` and executes it under fixture outcomes: all green, all skipped, one failure, two failures (names the first, counts the rest), and a step that never ran. It also proves structurally that every `continue-on-error` step reaches the join and references its composite by pinned full path, so a composite added without a join entry fails the build instead of riding to silent green. `ci-fanout-consolidation.test.cjs` is updated where the job set legitimately changed: it now locks the `checks` caller's shape, its two permission grants, and the absence of the five replaced jobs from `needs` and from the aggregation. A new case locks `composites-head`: its display name, its `needs: changes`, all nine `uses: ./.github/actions/<x>` references, the absence of any pinned `melodic-software/ci-workflows/...` reference inside it, and its presence in both the `ci-status` `needs` list and the aggregated results. The contract-only predicate gate is unchanged and still asserted on every job including the new one. `.github/dependabot.yml` already carries a `github-actions` group; no change was needed. ## Verification - `ls .github/workflows/checks.yml` — succeeds. - `yq '.on.workflow_call.outputs | keys' .github/workflows/checks.yml` — prints `results` and `outcome`. - `yq '.jobs[] | select(.uses != null) | .uses' .github/workflows/ci.yml` — includes `./.github/workflows/checks.yml`. - `actionlint .github/workflows/checks.yml .github/workflows/ci.yml` — clean (actionlint 1.7.12). - `zizmor --persona regular .github/workflows/checks.yml` — no findings. On `ci.yml` the pre-existing low `self-repository` findings went 59 to 46 with the consolidation and back to 55 with `composites-head`, since that audit is the one recommending `$/`, the syntax actionlint 1.7.12 rejects. The lane is advisory (`fail-on-severity` defaults to `never`). - `git diff 449157a..HEAD --stat -- .github/actions/` — empty. No composite body changed since the pin, so `checks.yml` at the next tag runs bodies byte-identical to HEAD and no follow-up pin bump is owed after the tag. - `node --test .github/scripts/*.test.cjs` — 782 tests, 777 pass, 5 fail, all five in `probe-billing-usage.test.cjs` and failing identically on an untouched tree (environment, not this change). - `npx @biomejs/biome@2.5.11 ci --config-path=fixtures/typescript/good/biome.json --error-on-warnings fixtures/typescript/good .github/scripts` — clean. - `typos`, `editorconfig-checker` and `markdownlint-cli2` — clean. - This pull request's own run is the live proof of both lanes: the `checks` job for the reusable's contract, and `Composites at HEAD` for the nine composite bodies at the commit under test. ## Related Refs melodic-software/github-iac#378 [runner-1348]: actions/runner#1348 --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Accepts
$/references in composite action manifests in the new parser path. This prevents valid nested self-repository actions from being reported as malformed and keeps both parser results consistent.Carries forward #4666.
Validation