Skip to content

chore: simplify gha workflow - #2979

Merged
ssbarnea merged 1 commit into
nextfrom
2/gha
Jul 2, 2026
Merged

chore: simplify gha workflow#2979
ssbarnea merged 1 commit into
nextfrom
2/gha

Conversation

@ssbarnea

@ssbarnea ssbarnea commented Jun 30, 2026

Copy link
Copy Markdown
Member
  • use a single runner for all test via parallel run
  • collect logs and coverage reports in format required by sonar
  • avoid running with node 20

Related: AAP-81317

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors the CI workflow to reduce the number of jobs/runners by consolidating linting, docs build, unit tests (with coverage), and integration tests into a single test job, while also adjusting artifacts/coverage outputs to support Sonar ingestion and dropping Node 20 from the matrix.

Changes:

  • Renames the primary job to test and restricts the Node matrix to [22].
  • Attempts to run lint/docs/tests/integration “in parallel” within the same job runner.
  • Adds an artifact upload step intended to provide coverage + transpiled JS outputs for Sonar.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread .github/workflows/ci.yml
Comment thread .github/workflows/ci.yml Outdated
Comment thread .github/workflows/ci.yml Outdated
Comment on lines 109 to 110
package:
runs-on: ubuntu-latest
@coderabbitai

coderabbitai Bot commented Jun 30, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Enterprise

Run ID: 88edd736-1256-446c-a497-5b466a5ac500

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch 2/gha

Comment @coderabbitai help to get the list of available commands.

@ssbarnea
ssbarnea force-pushed the 2/gha branch 2 times, most recently from c4eadcd to 3211cff Compare June 30, 2026 18:36
cidrblock

This comment was marked as resolved.

@cidrblock

This comment was marked as resolved.

cidrblock added a commit that referenced this pull request Jun 30, 2026
## Summary

Combines the SonarCloud-relevant improvements from #2977 and #2979 into
a
focused, merge-ready PR, plus fixes Codecov carry-forward inflation
discovered
during review. Credits @ssbarnea for the original SonarCloud work.

## Changes

### SonarCloud configuration (from #2977 / #2979)

- **LCOV report paths**: configure explicit comma-separated paths
(`coverage/lcov.info,coverage/wdio/lcov.info`) instead of glob patterns
—
  SonarScanner's wildcard handling for this property is inconsistent
([Copilot review on
#2977](#2977 (review)))
- **Test scope**: add `packages/common/test/` to `sonar.tests` (was
missing)
- **Branch-analysis docs**: document long-lived branch pattern for
SonarCloud
without hardcoding `sonar.branch.name`/`sonar.branch.target`, which
would
  break PR scans
- **Log verbosity**: reduce from `DEBUG`/`verbose=true` to
`INFO`/`false` —
  `DEBUG` is only useful when diagnosing scanner issues
- **Coverage artifact upload**: upload LCOV files from `build-and-test`
and
  `ui` jobs via `ansible/actions/upload-artifact@b2b0657c # v1.1.2`
  (gitleaks-secured wrapper around `actions/upload-artifact`)
- **`.gitignore`**: ignore `**/*.tgz` packaging artifacts

### Codecov carry-forward fix

After merging #2983 (honest coverage), we observed coverage jumping from
an
honest ~39% to an inflated 87.67% after late-arriving WSL uploads
triggered
re-computation. Root cause: Codecov's default `carryforward: true` was
merging
stale (pre-#2983) inflated coverage data for flags that hadn't yet
uploaded
fresh results.

- **Disable carry-forward** for all 5 flags (`unit-node22/24/26`,
`wdio`,
  `wsl-fedora`) — each flag uploads fresh data on every CI run, so
  carry-forward is unnecessary and was actively harmful
- **Increase `after_n_builds`** from 3 to 5 to match actual upload count
  (3 build-and-test matrix + 1 wdio + 1 wsl)
- **Enable `wait_for_ci`** so Codecov waits for all uploads before
computing
  the final merged report

## What was NOT included from #2979

- Job consolidation (we intentionally have separate
lint/test/integration/ui/package jobs)
- WSL workflow deletion (we recently added WSL CI with retry logic in
#2981)
- `package.json` script changes
- `prek.toml` changes (disabling skillmark/actionlint)

## Test plan

- [ ] CI passes (no source code changes — config only)
- [ ] Codecov reports honest coverage (~39%) after all 5 uploads
complete
- [ ] SonarCloud scan on `next` picks up LCOV reports after merge
- [ ] Coverage artifacts appear in Actions run summary

Supersedes #2977.

Co-authored-by: Sorin Sbarnea <ssbarnea@users.noreply.github.com>

---------

Co-authored-by: Sorin Sbarnea <ssbarnea@users.noreply.github.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@ssbarnea
ssbarnea force-pushed the 2/gha branch 2 times, most recently from 71e81ef to f52b6d9 Compare July 1, 2026 15:02
@ssbarnea
ssbarnea marked this pull request as ready for review July 2, 2026 12:54
- use a single runner for all test via parallel run
- collect logs and coverage reports in format required by sonar
- avoid running with node 20, use only 24 like current main
- temporary disable skillmark hook due to being unreliable
- temporary disable actionlint hook due to not support latest gha syntax
@ssbarnea
ssbarnea requested a review from cidrblock July 2, 2026 13:10
@ssbarnea
ssbarnea merged commit 65f2533 into next Jul 2, 2026
8 checks passed
@ssbarnea
ssbarnea deleted the 2/gha branch July 2, 2026 13:11
@cidrblock

Copy link
Copy Markdown
Contributor

Post-merge review: regressions and ADR compliance

After reviewing the merged diff, I wanted to document several regressions and ADR violations so we can track resolution.

Quality gate removals

  1. actionlint disabled (prek.toml) — Commented out citing actionlint#693, but .github/actionlint.yaml already has working exceptions for the parallel keyword. The hook should work with those exceptions — it just needs the path updated from ci.yml to next.yml since the workflow was renamed in this PR.

  2. skillmark disabled (prek.toml) — Commented out as "not mature enough, this repo has one github star." Skillmark was validating SKILL.md files against the agentskills.io spec and was passing. Popularity isn't a proxy for correctness — the tool enforces ADR-018's compliance requirements.

  3. prek removed from AGENTS.md prerequisitesprek was deleted from the prerequisites section, but pnpm run lint:prek still uses it. Contributors following AGENTS.md won't know to install it.

CI coverage reductions

  1. Node version matrix dropped from 3 to 1 — Was testing Node 22, 24, 26. Now only Node 24. Regressions on Node 22 (current LTS) and Node 26 will go undetected.

  2. No partial CI feedback — Single monolithic job means a lint failure blocks all visibility. Previously, separate jobs let you see "lint failed but tests passed." Now it's a single red X.

Supply-chain safety

  1. ansible/actions/upload-artifact@main (mutable ref) — New "Upload logs and coverage reports" step pins to @main. This is a supply-chain risk — the action can change without review. The existing SonarCloud upload in the same file correctly pins to @b2b0657c # v1.1.2.

  2. actions/upload-artifact@v7 (tag ref, not SHA) — Four new upload steps use unpinned tag refs. The project's pr-review skill and prior practice is to pin actions to commit SHAs with a tag comment (e.g., actions/checkout@SHA # v7).

Build artifacts

  1. npm tarballs no longer produced — The old pipeline packed and uploaded @ansible/common, @ansible/developer-services, @ansible/mcp-server, and @ansible/language-server as npm tarballs. This artifact output is gone.

  2. package script changedpnpm run package was changed from vsce package --no-dependencies to npm run build && vsce package --no-dependencies && ./scripts/build-plugin.sh. Note this uses npm run build inside a pnpm project.

ADR violations

Item ADR Detail
skillmark disabled ADR-018 + ADR-005 invariant 12 Invariant 12 requires skills to "conform to the agentskills.io frontmatter spec." Skillmark was the enforcement gate. Removing it without an ADR violates the process in ADR-005: "If an invariant needs to change, write a new ADR first."
actionlint disabled ADR-008 (spirit) ADR-008 established strict linting as policy. Disabling a linter that has a working exception config weakens that posture without justification.
No ADR written ADR-005 (process) This PR restructures CI architecture, removes quality gates, and drops distribution formats. ADR-005 requires an ADR before changing invariants or their enforcement.
npm tarballs removed ADR-020 (weakened) ADR-020 specifies five distribution formats from one repo. Removing tarball production reduces that to four.

Not a regression

  • macOS runner added — genuine improvement.
  • @j178/prek added as npm dependency — good, avoids pipx install prek for contributors.
  • .coderabbit.yaml added — useful review automation config.

Suggested follow-ups

  • Re-enable actionlint in prek.toml (update .github/actionlint.yaml path from ci.yml to next.yml)
  • Re-enable skillmark in prek.toml
  • Restore prek to AGENTS.md prerequisites (or document that @j178/prek npm package replaces it)
  • Pin ansible/actions/upload-artifact to SHA instead of @main
  • Consider restoring Node 22 to the matrix (it's current LTS)
  • Write an ADR if the CI consolidation and quality gate changes are intentional long-term decisions

cidrblock added a commit that referenced this pull request Jul 17, 2026
## Summary

- Restore CI Node matrix to **22 / 24 / 26** (`engines` still allows
`>=22.18.0`; #2979 collapsed to 24-only without a compatibility reason)
- Re-enable **actionlint** and **skillmark** in `prek.toml` (actionlint
path updated to `next.yml`; skillmark scoped to `.agents/skills` +
`skills` with `--exclude .wdio-vscode/**`)
- Pin artifact actions to commit SHAs; stop using
`ansible/actions/upload-artifact@main`
- Restore stable **`vsix`** artifact name + **npm tarball** uploads
(once on linux/node 24)
- Fix `package` script to use `pnpm run build` instead of `npm run
build`
- Drop the bogus `macos`×`ubuntu-latest` duplicate matrix cells —
tracked in #3034
- Skip WDIO UI on Node 26 only (`UND_ERR_INVALID_ARG`) — tracked in
#3035; lint/unit/integration/package still run on 26

Keeps the monolithic `test` job from #2979 (job count intentionally
unchanged).

Addresses post-merge review on #2979 / AAP-81317 fallout.

## Test plan

- [x] `pnpm run ci` locally (pre-push)
- [x] `pnpm exec prek run skillmark actionlint -a`
- [x] CI shows `test-linux-node22`, `test-linux-node24`,
`test-linux-node26`
- [ ] `test-linux-node26` green with UI skipped
- [ ] PR artifacts include `vsix`, `plugin`, `mcp-server`,
`language-server`, `npm-tarballs`

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
- Adds an aggregator job to emit the required `test-linux` check while
retaining version-specific matrix job names.
- Restores Node 22/24/26 Linux CI gates/artifacts: re-enables actionlint
+ skillmark, pins actions, restores VSIX and npm tarball uploads.
- Pins artifacts, updates the `package` script to `pnpm run build`, and
skips WDIO UI tests on Node 26.

Related: `#2979`
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants