Repository navigation
Conversation
check-main-sync-spawn-ratchet.mjs (stablyai#27756) imported diffCounts, formatBaseline, parseBaseline and parseBaselineNotes from check-focus-read-ratchet.mjs, but stablyai#27788 had already moved them to ratchet-baseline.mjs under their Count names. The script threw on import, so the gate, audit:code-quality:native and pnpm lint all failed on main. It now imports the helpers from where they live. The baseline entry for mobile-notification-dismissal-store.ts is pruned with the script's own --prune: stablyai#27731 removed that launch minutes before the ratchet merged.
pty-process-ceiling.ts and pty-process-ceiling-runtime.ts each gained one runProcessSync in stablyai#27769, merged twelve hours before the ratchet (stablyai#27756), so its baseline never listed them. Both run in the daemon process, like the other daemon entries here. With them listed the gate passes again; converting them to runProcess would also clear it.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The inspected ratchet changes align with the helper API and source counts; no merge-blocking issue is established. Pre-merge checks ·
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files. (1 skipped: 1 … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: repairing the main-process synchronous-launch ratchet check. Description check The description follows the required template and explains the failure, changes, rationale, linked issue, testing, alternatives, and scope. It also documents the lack of UI changes and the relevant ch… Linked Issues check Issue #28052requires repairing the import-time crash and resolving the three baseline mismatches.check-main-sync-spawn-ratchet.mjsnow importsdiffCounts,formatCountBaseline, `parseCountBasel…Out of Scope Changes check The changes are limited to the sync-launch ratchet script and its baseline. The script changes support the repair by using the shared helper API. The baseline changes address the mismatches named in i…
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files. (1 skipped: 1 unsupported.)
- Fix all pre-merge checks with AI
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
Comment @coderabbitai help to get the list of available commands.
1 participant
ELI5
The check that stops new code from freezing Orca with synchronous launches doesn't run on
mainright now. It crashes before it checks anything, which also takespnpm lintdown with it. This makes it run again.What Changed
Before:
pnpm run check:main-sync-spawn-ratchetthrew on import:so that gate,
audit:code-quality:native,pnpm lint, and the "Check main-process sync launches" step inpr.ymlall failed onmain.After: the gate runs and passes:
Main sync-spawn ratchet OK — 119 use(s) in 81 legacy file(s).Two commits, so you can take the first without the second:
check-main-sync-spawn-ratchet.mjsnow importsdiffCounts,formatCountBaseline,parseCountBaselineandparseCountNotesfromratchet-baseline.mjs, where chore(ratchet): one ratchet baseline library #27788 moved them. The stale row formobile-notification-dismissal-store.tsis dropped with the script's own--prune(perf(notifications): stop every Windows notification from freezing Orca #27731 removed that launch a few minutes before the ratchet merged).pty-process-ceiling.tsandpty-process-ceiling-runtime.tseach have onerunProcessSyncfrom feat(daemon): opt-in OS ceiling on terminal processes, reported per host #27769, which merged before the ratchet did, so the baseline never listed them. Both run in the daemon process, like the other daemon entries in that file.Why
#27756 was written against the old helper names and merged after #27788 renamed them, so nothing was wrong with either PR on its own; they just crossed.
For the two daemon launches the other option is converting them to
runProcess. I kept this PR mechanical and didn't touch daemon code. If you'd rather convert them, drop the second commit and the gate will keep pointing at those two files.Linked Issue
Fixes #28052
Visual Proof
N/A. This only touches a CI script and its baseline file; there is no UI or runtime behaviour change.
Testing
No new test: the failure was an import that no longer resolved, which running the gate catches. The existing tests for the script and the baseline library still pass.
What I ran on macOS:
node config/scripts/check-main-sync-spawn-ratchet.mjs: crashes onmain, passes on this branch.pnpm run audit:code-quality:native: fails onmainon the four unresolved named imports, passes on this branch.pnpm test config/scripts/check-main-sync-spawn-ratchet.test.mjs config/scripts/ratchet-baseline.test.mjs: 63 tests pass.The script is plain Node with no platform-specific paths, so Linux and Windows behave the same.
Review
cc @brennanb2025 (the ratchet, #27756) and @OrcaWin (the baseline library, #27788, and the ceiling code, #27769). The second commit is the part that's really your call.
Agent skill upstream boundary
Notes
Ensure no issues in: Security, Cross-platoform support (Linux, Windows, Mac), Remote SSH, Mobile, general backwards compatibility, performance
No product code changes, so none of these are affected.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)