Skip to content

fix(ci): serialize mobile typechecks to avoid install races - #24001

Open
bankit711 wants to merge 1 commit into
stablyai:mainfrom
bankit711:ci-mobile-typecheck-order
Open

bankit711 wants to merge 1 commit into
stablyai:mainfrom
bankit711:ci-mobile-typecheck-order

Conversation

@bankit711

Copy link
Copy Markdown

ELI5

Some mobile PR checks failed before checking the code because two commands tried to change the same dependency folder at once. Run them one after the other so neither interferes with the other.

What Changed

The production typecheck is now a normal foreground step before the test-typecheck ratchet. Removed the unused background-step bookkeeping and updated the existing workflow test to reject the old overlap. Both commands and all later checks remain unchanged.

Why

pnpm can install dependencies before running either command; the failing run showed both installs racing on the patched React Native directory. Native sequential steps remove that overlap without changing dependency options, increasing timeouts, weakening a check, or keeping a background task that is immediately waited on.

Linked Issue

Observed on #23552 — https://github.com/stablyai/orca/actions/runs/36656193200/job/109700961021. Shared workflow correction rather than an unrelated change to that PR's feature.

Visual Proof

N/A — workflow ordering and its regression test only; no app UI or interaction change.

Testing

  • Manually checked the local workflow and completed the focused automated proof on macOS.
  • Updated the existing regression test: actual Vitest run passes 8/8; changed case fails on the original workflow shape and passes the fixed candidate, with source restored afterward.
  • Direct command: ORCA_BACKGROUND_LAUNCH=1 node node_modules/vitest/vitest.mjs run config/scripts/ci-background-step-barriers.test.mjs --config config/vitest.config.ts (Node 24).
  • Targeted oxlint: 0 warnings/errors. oxfmt checks both changed files, exit 0. git diff --check passes.
  • No pnpm/compiler-race reproduction, full app/native checks or remote CI rerun claimed. Existing fix(native-chat): main's draft-queue tests follow #23524's /clear (fixes main) #23940 CI SUCCESS/local proof reused, not rerun.

Review

Fresh Astra same-session final correctness/ponytail verdict PASS at 0e536bfd035b818be5c2fa6c1fe1d0f86c43af74. Both initial P3 findings resolved in one builder round; reviewer independently ran 8/8 focused tests and checked actual parent/tree, two-file scope and clean tracked state. No remaining findings.

Agent skill upstream boundary

  • Not applicable; no upstream skill source copied or translated.

Notes

No dependency manifest, lockfile, compiler options, ratchet baseline, runtime behavior, wire format, platform command, or remote/folder-workspace execution change. Only the two intended paths are committed; task notes/install proof remain untracked or ignored.

Checklist

  • Small and focused; before/after, mechanism and choice explained.
  • Visual proof N/A with reason.
  • Changed scope, unchanged gate commands and restoration checked.
  • Cross-platform/SSH/path impact considered: no runtime changes.
  • Full lint/typecheck/test/build not rerun; focused executed proof above, published CI still pending.

@greptile-apps

greptile-apps Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

[High risk] Changes CI workflow serialization for mobile type checks.

The PR appears safe to merge; the mobile checks remain ordered and gated.

Summary

The PR runs the mobile production typecheck and test-typecheck ratchet sequentially to avoid competing dependency operations. It removes the obsolete background-step barrier and updates the workflow regression test.

Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart LR
  Install[Install dependencies] --> Production[Production typecheck]
  Production --> Ratchet[Test-typecheck ratchet]
  Ratchet --> Test[Test]
Loading

Reviews (1) · Last reviewed commit: "fix(ci): serialize mobile typechecks to ..."

@pullfrog pullfrog Bot 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.

✅ No new issues found.

Reviewed changes

  • Serializes the two mobile typechecks — .github/workflows/mobile.yml drops id: production-types / background: true from Typecheck and the later - wait: production-types, so pnpm typecheck now runs as a foreground step immediately before the Typecheck tests (ratchet) step.
  • Updates the barrier regression test — config/scripts/ci-background-step-barriers.test.mjs replaces the assertJoinedBefore case with a run-string lookup that asserts production < ratchet < Test, that neither typecheck is background, and that no step still waits on production-types.

The stated root cause checks out: pnpm 12's verifyDepsBeforeRun default is install, and neither pnpm-workspace.yaml, mobile/pnpm-workspace.yaml, nor any .npmrc overrides it, so both pnpm script invocations could trigger a pre-run install concurrently. The mobile workspace patches react-native@0.83.10, matching the reported race on the patched React Native tree. The updated test genuinely fails on the old shape (the background assertion), and no other file references production-types. I ran the focused barrier test (8/8) and the mobile dispatch-scope contract test (43/43).

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@coderabbitai

coderabbitai Bot commented Sep 30, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 4bd4ee73-2921-4489-b7d1-d8fc6845f7e2

📥 Commits

Reviewing files that changed from the base of the PR and between 9803967 and 0e536bf.

📒 Files selected for processing (2)
  • .github/workflows/mobile.yml
  • config/scripts/ci-background-step-barriers.test.mjs

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

The mobile workflow runs production and ratchet typechecks sequentially before the Test step. Both typechecks run in the foreground, and the workflow no longer includes a wait for production-types. The verification test checks the step order and confirms these workflow conditions.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 0e536

The typechecks now run sequentially before Test, preventing the overlap this change addresses. No merge-blocking risk remains; normal checks can proceed.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 0e536

The change removes overlapping mobile typechecks without changing their commands, job identity, or credential configuration. No introduced security concern was identified. Confidence remains bounded because external required-check enforcement and complete security coverage were not verified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The demonstrated change affects ordering and dependency-directory mutation within the mobile CI verification job. Source comparison does not establish a change to application data stores, cloud SQL rollout authority, or production service exposure.

Trust Boundaries and Controls

  • observed — The verify job identity, pull-request execution condition, checkout configuration, and runner selection are unchanged. The workflow diff only removes background-task bookkeeping and changes its explanatory comment; it adds no credentials, permissions, secret access, or new execution inputs.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: serializing mobile typechecks to prevent dependency installation races.
Description check ✅ Passed The description is complete and focused. It explains the change, cause, rationale, testing, scope, visual proof, and checklist status. It also clearly states which full checks were not rerun.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant