chore(ci): CI orchestration hygiene + dependency-review (#420, #416) - #429
Conversation
… review (#420, #416) #420 — concurrency (cancel-in-progress) on PR workflows (test, claude-review, and sonarcloud PR-only); .turbo actions/cache on turbo jobs; nightly.yml hosting #417's full-scope mutation report; deploy/release workflows left uncancelled. #416 (ui slice) — dependency-review.yml (fail-on-severity high) for orbit-ui-mobile. Must merge LAST (references the Lint/Dependency-Review/mutation checks the other PRs add). Closes #420 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Editing claude-review.yml makes the claude-code-action self-skip the PR (reports pass but posts no review), which blocks the required approval. The concurrency block stays on test.yml + sonarcloud.yml where it matters. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: PR #429
Scope: PR #429 in thomasluizon/orbit-ui-mobile (chore(ci): CI orchestration hygiene + dependency-review (#420, #416))
Recommendation: APPROVE
Summary
Pure CI-configuration diff: adds .github/workflows/dependency-review.yml and .github/workflows/nightly.yml, and adds concurrency + turbo actions/cache steps to sonarcloud.yml and test.yml. No apps/*, packages/shared source, or orbit-api files change, so the UI/parity/i18n/contract/security/backend-hard-rule dimensions are all N/A. The workflow logic itself (cancel-in-progress scoping, cache key scoping) is correct and consistent with the repo's existing conventions. One Medium cross-PR coordination gap was found and verified.
Findings
Critical
None
High
None
Medium
[MEDIUM] nightly.yml duplicates the nightly full-scope mutation job already shipped in PR #427's mutation.yml
· dimension: 1. Correctness (cross-boundary / cross-PR)
· location: orbit-ui-mobile/.github/workflows/nightly.yml:11-35
· issue: This PR adds a new `mutation` job on `schedule: cron: '0 3 * * *'` that runs `npm run mutation` (full scope) in packages/shared and uploads an artifact named `mutation-report-nightly`. PR #427 (open, branch issue-417, same base commit 8ba9c750) independently adds `.github/workflows/mutation.yml`, which already contains a `nightly:` job (`if: github.event_name == 'schedule'`, its own `cron: '0 6 * * *'`) doing the exact same full-scope `npm run mutation` run and uploading an artifact with the identical name/path. Neither PR's diff removes or coordinates with the other's nightly job. Additionally, `packages/shared/package.json` has no `mutation` script and there is no `packages/shared/stryker.config.json` on current main (both only exist on #427) — confirmed via `git show origin/main:packages/shared/package.json` and a repo-wide search for `stryker.config*`. nightly.yml is already live on this branch's merge ref (HEAD 61c4cba0) and would fail nightly with "Missing script: mutation" until #427 lands.
· risk: If #429 merges before #427, the new nightly job fails every night (harmless — non-blocking, report-only) until #427 lands. Once both PRs are on main, main runs the identical full-scope mutation test twice nightly (03:00 and 06:00 UTC), doubling CI minutes for zero benefit and producing two same-named `mutation-report-nightly` artifacts from different workflow runs — the exact kind of redundant CI this PR's own title ("dedupe PR runs") is meant to eliminate.
· fix: Coordinate the two PRs before both land: either drop the `nightly:` job (and the `schedule:` trigger) from #427's mutation.yml now that #429's nightly.yml is meant to own it (per this PR's own body: "nightly.yml … hosting #417's full-scope mutation report run"), or drop nightly.yml's `mutation` job from this PR and let #427's mutation.yml keep owning it. Whichever PR merges second should carry the removal.
· reference: PR #429 body ("nightly.yml … hosting #417's full-scope mutation report run"); CLAUDE.md rule 10 (DRY at the right level — one nightly mutation job, not two)
Low / Info
None
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A — no apps/web/** or apps/mobile/** file changed |
| i18n-syncer | N/A — no user-facing strings or i18n JSON changed |
| contract-aligner | N/A — no packages/shared/src/types/* / endpoints.ts, no orbit-api changes |
| security-reviewer | N/A — no orbit-api code changed |
| design-reviewer | N/A — no apps/* or landing-page UI files changed |
Validation
| Check | Result |
|---|---|
| Lint | N/A — diff touches only .github/workflows/*.yml, no lintable source |
| Type check | N/A — no TS/C# source changed |
| Tests | N/A — no source changed; nothing to exercise |
| Build (api) | N/A — orbit-api untouched |
| YAML well-formedness | Manually verified — all 4 files parse as valid, consistently-indented GitHub Actions YAML matching the repo's existing workflow style (automated js-yaml/python -c parse was blocked by sandbox tool approval, so this was a careful manual read rather than a tool-run check) |
Deferred — N/A dimensions & files not verdicted
- Dimensions 2 (dead/stale code), 3 (SOLID/clean-arch), 4 (comment policy), 5 (no-workaround), 6 (type safety), 7 (no console.log), 8 (DESIGN.md/AI-slop), 9 (parity), 10 (i18n), 11 (contract drift), 12 (security — frontend categories), 13 (backend hard rules), 14 (FEATURES.md parity): all N/A — this diff's only surface is
.github/workflows/*.yml; none of these dimensions' trigger surfaces are touched. - All 4 changed files (
dependency-review.yml,nightly.yml,sonarcloud.yml,test.yml) were read in full and given a verdict — nothing deferred among changed files. - Automated YAML parsing (
python3/node -e) was blocked by sandbox permission prompts in this session; substituted with a manual structural read of all 4 files instead. - Writing the report to the skill's default
.claude/reviews/output path was also blocked by a sandbox sensitive-file permission gate; the report was written to/tmp/pr-429-review.mdinstead (the same fallback location the CI reviewer wrapper itself uses). - The
claude-review.ymlconcurrency change mentioned in the PR body was investigated: it was added then reverted within this same PR's commit history (5dd3984bthen82303d1b, see commit message "Editing claude-review.yml makes the claude-code-action self-skip the PR") and nets to no diff against main — confirmed viagh pr diff 429, which correctly shows noclaude-review.ymlhunk. Not a gap, just resolved before the diff was cut.
What's good
concurrencyscoping is correctly differentiated:test.yml(PR-only trigger) cancels unconditionally;sonarcloud.yml(push-to-main + PR trigger) conditionscancel-in-progressongithub.event_name == 'pull_request'so a main-push SonarCloud analysis is never cancelled mid-run — exactly matches the PR body's stated intent.- Turbo cache steps are consistently placed (after
npm ci, before the turbo command) and correctly job-scoped via${{ github.job }}in the cache key across all four jobs they touch, avoiding cross-job cache pollution. dependency-review.ymlandnightly.ymlboth declare minimalpermissions: contents: read.- No script-injection surface introduced (no untrusted
github.event.*context interpolated into arun:block). - The commit history shows the author already caught and reverted one real problem mid-PR (claude-review.yml self-skip) — good self-correction.
Recommendation
Approve as-is; the one Medium finding is a cross-PR coordination item between #429 and the still-open #427, not a defect in this PR's own code, and is safe to resolve as a fast follow-up (drop the duplicate nightly: job from whichever of #429/#427 merges second) rather than blocking this PR.
Avoids running the full-scope mutation report twice nightly (mutation.yml 06:00 + nightly.yml 03:00). nightly.yml is the dedicated home for slow gates. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review: PR #429
Scope: PR #429 in thomasluizon/orbit-ui-mobile (already merged as 85d95a22)
Recommendation: APPROVE
Summary
CI-orchestration hygiene PR: adds dependency-review.yml (fail-on-severity high), adds
concurrency/turbo-cache to test.yml and sonarcloud.yml, and splits the nightly
full-scope mutation run out of mutation.yml into a new nightly.yml. The diff touches
only .github/workflows/*.yml — no application source, no packages/shared contract
surface, no apps/* UI. All five files are internally consistent with the PR's stated
intent and with each other (action versions, cache-key shape, trigger scoping).
Findings
Critical
None.
High
None.
Medium
[MEDIUM] nightly.yml lost the concurrency guard the old nightly job had
· dimension: 3 (SOLID/clean-arch — pattern consistency) / defense-in-depth gap
· location: .github/workflows/nightly.yml:1-12
· issue: In the old mutation.yml, the nightly job was covered by the file-level
concurrency: { group: mutation-${{ github.workflow }}-${{ github.ref }}, cancel-in-progress: true }
block (schedule events all share ref = refs/heads/main, so a slow run would be
protected). The new standalone nightly.yml carries no concurrency: block at all.
· risk: A workflow_dispatch fired manually while the 03:00 cron run is still in
flight (or a cron run that overruns 24h) launches two concurrent full-scope Stryker
runs writing to the same mutation-report-nightly artifact name and consuming double
the runner minutes. Low probability, no correctness break, but the safeguard the old
file had was silently dropped in the split.
· fix: add
concurrency:
group: nightly-mutation
cancel-in-progress: falseto nightly.yml (queue rather than cancel, since this is a report job you don't want
to kill mid-run).
· reference: CLAUDE.md rule 10 (pattern consistency when splitting a file) / severity
ladder "defense-in-depth gap"
Low / Info
[INFO] Turbo cache keys are ${{ runner.os }}-turbo-${{ github.job }}-${{ github.sha }}
with a prefix restore-keys fallback — correct pattern for turbo's own incremental
cache (exact-sha key will always miss on a fresh run; the restore-keys prefix match is
what actually pays off). No action needed, called out because it looks at first glance
like a cache that can never hit.
[INFO] dependency-review.yml, actions/cache@v4, and actions/setup-node@v6 are
pinned by tag, not commit SHA, while sonarcloud.yml's third-party
SonarSource/sonarqube-scan-action is pinned by full SHA. This matches the repo's
existing convention (first-party actions/* by tag, third-party orgs by SHA) — not a
new deviation introduced by this PR.
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A — no apps/web/** or apps/mobile/** file changed |
| i18n-syncer | N/A — no user-facing strings or i18n JSON changed |
| contract-aligner | N/A — no packages/shared/src/types/*/endpoints.ts, no orbit-api change |
| security-reviewer | N/A — no orbit-api code changed |
| design-reviewer | N/A — no apps/*/orbit-landing-page UI file changed |
Validation
| Check | Result |
|---|---|
| Lint | N/A — diff is CI-workflow YAML only, not covered by repo lint |
| Type check | N/A — no TS/C# source in the diff |
| Tests | N/A — no application test surface touched |
| Build (api) | N/A — orbit-api not touched |
| YAML syntax (manual read of all 5 files) | PASS — well-formed, consistent indentation, matches GitHub Actions schema |
actionlint / automated YAML validator |
Not available in this sandbox — deferred, see below |
Deferred — N/A dimensions & files not verdicted
- Dimensions 8 (DESIGN.md/AI-slop), 9 (Parity), 10 (i18n), 11 (Contract drift +
backward-compat), 13 (Backend hard rules), 14 (FEATURES.md parity): all gated N/A —
their triggering surfaces (apps/*UI,packages/sharedtypes,orbit-api,
user-facing feature surface) are untouched by this diff. - Dimensions 6 (type safety) and 7 (
console.log): N/A — no TS/C#/application code in
the diff, only YAML. - All five subagents: N/A per the gate table above — none of their trigger conditions
were met. - Automated workflow-syntax validation (
actionlint): not installed in this sandbox,
and shell invocations that would have run a YAML parser (python3 -c) were blocked by
the environment's approval gate — relied on manual read-through instead. Every changed
file was still read and given a verdict. - Branch-protection follow-up the PR body calls for (registering
Lint/
Dependency Reviewas required checks) is an operational step outside the diff itself
— not verified here; flagged as Info, not a code-review finding. - This report could not be written to the repo's own
.claude/reviews/directory: the
sandbox's file-permission layer flags anything under.claude/as a protected path
and blocked bothmkdirandWritethere without an interactive approval this
headless run has no way to grant. Written to/tmp/pr-429-review.mdinstead; full
content is also relayed in this session's final message.
Every one of the 5 changed files (dependency-review.yml, mutation.yml, nightly.yml,
sonarcloud.yml, test.yml) received an explicit verdict above — nothing changed was
skipped.
What's good
- Correctly extracts the nightly full-scope mutation job out of the per-PR workflow
instead of leaving aschedule:trigger bolted onto a PR-triggered file — cleaner
separation of concerns, matches the PR's own stated intent. sonarcloud.yml's conditionalcancel-in-progress: ${{ github.event_name == 'pull_request' }}
is the right call — a push-to-main SonarCloud analysis is exactly the kind of run you
never want silently cancelled by a subsequent push.- Deploy/release workflows (smoke-prod, promote-prod, android-release) were deliberately
left out of the cancel-in-progress rollout, per the PR body — correct, cancelling a
live deploy mid-flight would be far worse than a duplicate CI run. dependency-review.ymluses minimalpermissions: contents: read— no
over-provisioned token scope.
Recommendation
Approve as-is. The one Medium (missing concurrency guard on nightly.yml) is a
low-blast-radius, easily-fixed follow-up — worth a quick one-line addition in a later
commit but not worth blocking this merge over, and the PR is already merged
(85d95a22). No Critical or High findings survive review.
|
… lcov) (#463) The Sonar job's persisted turbo cache (added #429) replayed the `test` task instead of running vitest, so no lcov was generated and the merged coverage/lcov.info was empty -> Sonar reported 0.0% (regressed from ~56%). Declare coverage/** as the test task's turbo outputs and move each workspace's vitest reportsDirectory inside its package so a cache hit restores real coverage; repoint the sonarcloud.yml merge step at the in-package lcov paths. Also exclude the generated Zod snapshot from coverage. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Bundles #420 + the orbit-ui-mobile slice of #416.
#420 — orchestration hygiene
concurrency: {cancel-in-progress: true}on PR workflows (test.yml, claude-review.yml); sonarcloud.yml cancels only on PR events (never a main-push analysis). Deploy/release (smoke-prod, promote-prod, android-release) deliberately left uncancelled..turboactions/cacheon the turbo jobs.nightly.yml(0 3 * * *) hosting Testing: scoped mutation testing (StrykerJS) on packages/shared critical logic #417's full-scope mutation report run.#416 — GitHub-native scanning (ui slice)
dependency-review.yml(fail-on-severity: high). CodeQL + secret-scanning already enabled repo-side; landing-page's Dependabot was turned on too.All 5 workflow files YAML-validated.
🤖 Generated with Claude Code