chore(dx): add local dev doctor and setup commands#37
Conversation
Add dev doctor and setup commands with fail-fast checks, document the contributor workflow, and wire a CI sanity check for doctor:dev. Co-authored-by: Codex <noreply@openai.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 Walkthroughwalkthroughintroduces two npm scripts, changes
sequence diagramsequenceDiagram
participant dev as developer
participant setup as setup:dev (lib/scripts/setup-dev.js)
participant doctor as doctor:dev (lib/scripts/doctor-dev.js)
participant npm as npm
participant verify as verify
dev->>setup: npm run setup:dev
setup->>doctor: runDevDoctor()
doctor->>doctor: check node >=18, find npm/git, check .git, package.json scripts
doctor-->>setup: status (ok|fail)
alt doctor fails
setup-->>dev: abort (non-zero)
else doctor passes
setup->>npm: npm ci (with retry/backoff)
npm-->>setup: exit (ok|fail)
alt npm ci fails
setup-->>dev: abort (non-zero)
else npm ci succeeds
setup->>verify: npm run verify
verify-->>setup: exit (ok|fail)
alt verify fails
setup-->>dev: abort (non-zero)
else verify passes
setup->>verify: npm test -- test/documentation.test.ts
verify-->>setup: exit (ok|fail)
setup-->>dev: success (0)
end
end
end
estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes missing regression tests: add targeted unit/integration tests for missing node_modules and malformed package.json cases referenced by 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 96-108: Add a short upgrade-note entry to docs/upgrade.md
documenting the new contributor bootstrap workflow introduced in README.md:
mention the two new npm scripts doctor:dev and setup:dev, describe what each
does (prerequisite checks and install+quality-gate), and note any behavior
changes or required actions for contributors (CLI flags or env vars if
applicable); ensure the note references the README section and follows the
project's docs style/format for upgrade notes.
In `@scripts/setup-dev.js`:
- Around line 9-45: The resolveNpmInvocation and runCommand functions introduce
platform-specific behavior and non-deterministic child-process event ordering
that need unit tests: refactor resolveNpmInvocation to expose/accept an override
for process.platform and process.env.npm_execpath (or extract logic into a pure
function) so you can assert the npm.cmd fallback on win32 and the npm_execpath
branch, and refactor runCommand to accept an injected spawnFactory (or wrapper
around spawn) so tests can stub child events; add tests that simulate
child.once("error") before/after "exit" to verify fail-fast exit codes (1, 130)
and that exit codes are propagated correctly, and ensure you preserve existing
semantics in resolveNpmInvocation and runCommand while making them injectable
for deterministic testing.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
Run ID: cee28f3a-346f-4801-8028-f0fd542596cc
📒 Files selected for processing (8)
.github/pull_request_template.md.github/workflows/ci.ymlCONTRIBUTING.mdREADME.mddocs/development/TESTING.mdpackage.jsonscripts/doctor-dev.jsscripts/setup-dev.js
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (1)
docs/**
⚙️ CodeRabbit configuration file
keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.
Files:
docs/development/TESTING.md
🧬 Code graph analysis (2)
scripts/setup-dev.js (1)
scripts/doctor-dev.js (2)
cwd(110-110)runDevDoctor(109-192)
scripts/doctor-dev.js (1)
scripts/setup-dev.js (1)
cwd(49-49)
🔇 Additional comments (8)
package.json (1)
42-43: script entries are wired correctly.lines [42-43] expose the new developer commands cleanly and match the added script entrypoints. ref:
lib/package.json:42-43..github/pull_request_template.md (1)
11-13: validation checklist update is consistent with the new workflow.this keeps pr evidence aligned with the new local gate commands. ref:
lib/.github/pull_request_template.md:11-13.CONTRIBUTING.md (2)
25-27: local setup and pr gate instructions are aligned.the new sequence is clear and matches the introduced doctor/setup workflow. ref:
lib/CONTRIBUTING.md:25-27andlib/CONTRIBUTING.md:56-67.Also applies to: 56-67
120-120: license wording change looks good.the updated sentence is concise and unambiguous. ref:
lib/CONTRIBUTING.md:120.scripts/setup-dev.js (1)
61-85: fail-fast gate sequencing is solid.the install -> lint -> typecheck -> build -> test order is deterministic and aborts on first failure, which is exactly what this command should do. ref:
lib/scripts/setup-dev.js:61-85.scripts/doctor-dev.js (1)
109-192: dev doctor checks are comprehensive and practical.the prerequisite checks and structured exit behavior look production-ready for local bootstrap sanity gates. ref:
lib/scripts/doctor-dev.js:109-192..github/workflows/ci.yml (1)
71-73: ci sanity gate placement is correct.running
doctor:devright after dependency install is a good early-stop point for lint job failures. ref:lib/.github/workflows/ci.yml:71-73.docs/development/TESTING.md (1)
22-25: testing guide updates are consistent with the new local workflow.the command sequence is aligned with the bootstrap scripts and matches expected contributor gating. ref:
lib/docs/development/TESTING.md:22-25andlib/docs/development/TESTING.md:49-53.Also applies to: 49-53
Add a canonical verify workflow shared by local and CI, integrate scoped Biome formatting checks, and document contributor/release runbooks to reduce setup and release friction. Co-authored-by: Codex <noreply@openai.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/development/TESTING.md (1)
22-35:⚠️ Potential issue | 🟡 Minoradd
doctor:devto testing commands for workflow parity.this page lists new core workflow commands but omits
doctor:dev, which is now part of setup/triage flow.ref:
lib/docs/development/TESTING.md:22-35.proposed docs patch
## Core Commands ```bash npm run setup:dev +npm run doctor:dev npm run verify npm run verify:ci</details> As per coding guidelines `docs/**: keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.` <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/development/TESTING.mdaround lines 22 - 35, Update the testing
commands list to include the new npm script "doctor:dev" so docs match the
actual CLI/workflow; locate the commands block that currently shows "npm run
setup:dev", "npm run verify", "npm run verify:ci" and insert "npm run
doctor:dev" immediately after "npm run setup:dev" (also add it to the Component
commands list if applicable) and ensure the README/docs mention the script name
consistently with the project's CLI/workflow.</details> </blockquote></details> </blockquote></details>♻️ Duplicate comments (2)
README.md (1)
96-116:⚠️ Potential issue | 🟡 Minoradd the upgrade-note entry for this workflow change.
line 96 introduces a contributor workflow change (
doctor:dev/setup:dev), but this changed set still does not show a paireddocs/upgrade.mdupdate documenting migration impact for contributors.as per coding guidelines "docs/**: keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 96 - 116, README.md was updated to introduce new contributor scripts (setup:dev, doctor:dev, verify) but docs/upgrade.md was not updated; add an upgrade-note entry describing the workflow change and migration steps. Edit docs/upgrade.md to add a short section titled something like "Contributor workflow changed: setup:dev and doctor:dev" that lists the new npm scripts (setup:dev, doctor:dev, verify), their purpose, expected preconditions, and any manual migration steps for contributors (e.g., run npm run setup:dev once, update local CI hooks if applicable), and cross-reference README.md and SECURITY/other docs so documentation is consistent with the new CLI flags/workflows.scripts/setup-dev.js (1)
9-45:⚠️ Potential issue | 🟠 Majoradd regression tests for windows invocation and child-process event ordering.
this path still has no direct coverage for win32
npm.cmdfallback /npm_execpathbranch, and no deterministic tests forerrorvsexitordering inrunCommand(line 24 onward). that leaves platform and concurrency-risk behavior unguarded.run this read-only check to confirm coverage gaps and exact call sites:
#!/bin/bash set -euo pipefail echo "== setup-dev symbols and event handlers ==" rg -n -C2 'function resolveNpmInvocation|function runCommand|child\.once\("error"|child\.once\("exit"|npm_execpath|npm\.cmd' scripts echo echo "== tests referencing setup-dev edge cases ==" rg -n --type=ts --type=js -C2 'runSetupDev|resolveNpmInvocation|runCommand|npm_execpath|npm\.cmd|SIGINT|child\.once\("error"|child\.once\("exit"' test testsexpected result: dedicated tests should explicitly assert win32/npm_execpath behavior and error/exit ordering semantics.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/setup-dev.js` around lines 9 - 45, Add focused regression tests for resolveNpmInvocation and runCommand: verify resolveNpmInvocation returns the execPath/prefixArgs when process.env.npm_execpath is set and returns "npm.cmd" on simulated win32 platform (override process.platform or stub the module) otherwise; for runCommand, stub/mock child_process.spawn to synchronously emit "error" then "exit", "exit" then "error", and a "exit" with signal "SIGINT" to deterministically assert the returned exit codes (1 for error, 1 or specific behavior when exit without code, and 130 for SIGINT) and that the child.once("error")/child.once("exit") handlers in runCommand handle ordering correctly; use spies/mocks to control env (npm_execpath) and spawn behavior and assert processLogger/console outputs as needed.🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed. Inline comments: In `@docs/development/LOCAL_DEV.md`: - Around line 63-69: Add a failure-mode entry describing transient Windows file-lock errors during setup where running npm ci can fail with EBUSY/EPERM (caused by antivirus or other process file contention); instruct users to retry the same command (npm ci) first, and if it persists advise stopping/pausing antivirus or excluding the repository, re-running in an elevated shell (PowerShell/CMD as Administrator), cleaning the working tree (git clean -fdx) and then re-running npm ci; mention that this applies to the doctor:dev/setup flow and include the error tokens EBUSY/EPERM and the remediation commands (npm ci, git clean -fdx) so the docs entry is discoverable. - Around line 1-35: docs/upgrade.md is missing mentions of the new npm scripts; update docs/upgrade.md to document the new workflow by adding brief entries for the doctor:dev and setup:dev scripts (what they run and when to use them) and include a short upgrade note advising users to adopt setup:dev after pulling changes. Also update the failure-modes section of LOCAL_DEV.md to mention the Windows npm ci lock contention risk and remediation: advise Windows users to try npm ci --no-audit or run the setup from WSL2 if npm ci stalls, and add this guidance near the existing failure modes paragraph. In `@package.json`: - Around line 42-43: Add regression tests for the npm scripts "doctor:dev" and "setup:dev" and implement Windows file-lock retry handling in scripts/setup-dev.js (and mirror behavior in scripts/doctor-dev.js as needed): modify the spawn() call in scripts/setup-dev.js that runs "npm ci" to catch transient errors EBUSY and EPERM and retry with exponential backoff (configurable max attempts and delay), ensuring the retry logic is only enabled on win32; add unit/integration tests that verify the success path, missing-tool detection, and that the retry logic is exercised by simulating EBUSY/EPERM failures; update any helper where PATHEXT is handled in scripts/doctor-dev.js so it also uses the same lock-aware spawn wrapper to keep behavior consistent. In `@scripts/setup-dev.js`: - Around line 61-66: The npm CI step currently does a single run of runNpm(["ci"]) and fails immediately on non-zero installExitCode; change this to perform a bounded retry loop around the runNpm(["ci"]) call (e.g., 3 attempts) with a short exponential or fixed backoff (e.g., 200–1000ms) between attempts and only return the final installExitCode if all attempts fail; specifically update the code around the installExitCode variable and the runNpm invocation to detect transient EPERM/file-lock failures on Windows (treat them as retriable) and retry accordingly before logging "setup:dev failed during npm ci." and returning the error. --- Outside diff comments: In `@docs/development/TESTING.md`: - Around line 22-35: Update the testing commands list to include the new npm script "doctor:dev" so docs match the actual CLI/workflow; locate the commands block that currently shows "npm run setup:dev", "npm run verify", "npm run verify:ci" and insert "npm run doctor:dev" immediately after "npm run setup:dev" (also add it to the Component commands list if applicable) and ensure the README/docs mention the script name consistently with the project's CLI/workflow. --- Duplicate comments: In `@README.md`: - Around line 96-116: README.md was updated to introduce new contributor scripts (setup:dev, doctor:dev, verify) but docs/upgrade.md was not updated; add an upgrade-note entry describing the workflow change and migration steps. Edit docs/upgrade.md to add a short section titled something like "Contributor workflow changed: setup:dev and doctor:dev" that lists the new npm scripts (setup:dev, doctor:dev, verify), their purpose, expected preconditions, and any manual migration steps for contributors (e.g., run npm run setup:dev once, update local CI hooks if applicable), and cross-reference README.md and SECURITY/other docs so documentation is consistent with the new CLI flags/workflows. In `@scripts/setup-dev.js`: - Around line 9-45: Add focused regression tests for resolveNpmInvocation and runCommand: verify resolveNpmInvocation returns the execPath/prefixArgs when process.env.npm_execpath is set and returns "npm.cmd" on simulated win32 platform (override process.platform or stub the module) otherwise; for runCommand, stub/mock child_process.spawn to synchronously emit "error" then "exit", "exit" then "error", and a "exit" with signal "SIGINT" to deterministically assert the returned exit codes (1 for error, 1 or specific behavior when exit without code, and 130 for SIGINT) and that the child.once("error")/child.once("exit") handlers in runCommand handle ordering correctly; use spies/mocks to control env (npm_execpath) and spawn behavior and assert processLogger/console outputs as needed.
ℹ️ Review info
Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID:
6746764a-e126-4e33-952f-4db179bec556⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json📒 Files selected for processing (12)
.github/pull_request_template.md.github/workflows/ci.ymlCONTRIBUTING.mdREADME.mdbiome.jsoncdocs/DOCUMENTATION.mddocs/README.mddocs/development/LOCAL_DEV.mddocs/development/RELEASE_RUNBOOK.mddocs/development/TESTING.mdpackage.jsonscripts/setup-dev.js📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (1)
docs/**
⚙️ CodeRabbit configuration file
keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.
Files:
docs/development/TESTING.mddocs/development/RELEASE_RUNBOOK.mddocs/README.mddocs/DOCUMENTATION.mddocs/development/LOCAL_DEV.md🧬 Code graph analysis (1)
scripts/setup-dev.js (1)
scripts/doctor-dev.js (3)
cwd(110-110)runDevDoctor(109-192)isDirectRun(194-200)🔇 Additional comments (8)
biome.jsonc (1)
9-18: looks good: biome scope is explicit and practical.this narrows formatting/lint discovery to intended config surfaces and avoids scanning heavyweight directories.
ref:
lib/biome.jsonc:9-18..github/pull_request_template.md (1)
11-19: good checklist refresh for the new gate flow.the additions make local and ci validation expectations explicit for contributors.
ref:
lib/.github/pull_request_template.md:11-19.CONTRIBUTING.md (1)
24-27: solid contributor flow update.this is clear, actionable, and aligned with the new bootstrap + gate commands.
ref:
lib/CONTRIBUTING.md:24-27,lib/CONTRIBUTING.md:31-35,lib/CONTRIBUTING.md:60-72.Also applies to: 31-35, 60-72
.github/workflows/ci.yml (1)
9-12: good ci hardening and gate consolidation.the concurrency guard and unified verify step reduce flaky overlap and simplify pipeline intent.
ref:
lib/.github/workflows/ci.yml:9-12,lib/.github/workflows/ci.yml:36-37,lib/.github/workflows/ci.yml:62-66.Also applies to: 36-37, 62-66
docs/development/RELEASE_RUNBOOK.md (1)
15-28: good release discipline in this runbook.the gate sequence and rollback steps are clear and operationally useful.
ref:
lib/docs/development/RELEASE_RUNBOOK.md:15-28,lib/docs/development/RELEASE_RUNBOOK.md:41-57.Also applies to: 41-57
docs/DOCUMENTATION.md (1)
42-43: looks good.the source-of-truth map update is consistent and keeps maintainer runbooks discoverable.
docs/README.md (1)
59-60: good docs portal alignment.these links keep maintainer workflow docs in sync with the new local-dev and release runbooks.
scripts/setup-dev.js (1)
48-84: fail-fast gate sequencing is clear and correct.the doctor → install → gate order is straightforward, and early-return on non-zero exit codes keeps failure mode deterministic.
Add regression tests for doctor/setup scripts, make setup command execution deterministic under event-order races, add bounded Windows npm ci retries, and document contributor upgrade + Windows lock remediation guidance. Co-authored-by: Codex <noreply@openai.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/development/TESTING.md`:
- Around line 51-54: The docs list runs verification and docs tests before
instructing a fresh-clone user to run the initial setup, which can break
first-time workflow; move the `npm run setup:dev` step so it appears before `npm
run verify` and `npm run test -- test/documentation.test.ts` in TESTING.md,
update any cross-references in LOCAL_DEV.md to match the reordered flow, and add
a short note saying the CLI/npm scripts (e.g., `setup:dev`, `verify`, `test`)
are authoritative for first-clone setup along with an upgrade note if the
ordering changed.
In `@scripts/doctor-dev.js`:
- Around line 241-243: The doctor currently checks only
["typecheck","lint","test","build"] via readPackageScripts in doctor-dev.js
(const requiredScripts) but misses the "verify" script that setup-dev.js later
runs; update the requiredScripts array to include "verify" so the doctor fails
fast, and add a regression test in test/scripts/doctor-dev.test.ts that asserts
doctor fails when "verify" is missing (mirror existing test structure for
missing scripts). Ensure you reference the readPackageScripts usage and the
setup execution of "verify" (setup-dev.js) when adding the test so it explicitly
covers the previously missed case.
- Around line 200-220: The checks currently treat npm/git as present based only
on path or a failed execution, so update the logic to verify executability:
after resolving npmPath (from npmExecPathFromEnv or findCommandInPath) call
runCommand(npmPath, ["--version"], spawnFn) and mark a failure via
failures.push(...) if the command exits non-zero or produces no valid output;
likewise for git, change the gitCheck handling so that a non-zero status or
empty stdout causes failures.push("git is required but was not found or is not
functional.") instead of merely adding a non-fatal detail; keep successful
outputs added to details (use gitCheck.stdout.trim()) and reference
npmExecPathFromEnv, npmPath, gitPath, runCommand and gitCheck when making the
edits.
ℹ️ Review info
Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 201fa13f-4891-4857-98aa-cb580f9cabe4
📒 Files selected for processing (7)
docs/development/LOCAL_DEV.mddocs/development/TESTING.mddocs/upgrade.mdscripts/doctor-dev.jsscripts/setup-dev.jstest/scripts/doctor-dev.test.tstest/scripts/setup-dev.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/scripts/doctor-dev.test.tstest/scripts/setup-dev.test.ts
docs/**
⚙️ CodeRabbit configuration file
keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.
Files:
docs/development/TESTING.mddocs/development/LOCAL_DEV.mddocs/upgrade.md
🧬 Code graph analysis (1)
test/scripts/doctor-dev.test.ts (1)
scripts/doctor-dev.js (5)
scripts(127-127)scripts(241-241)warnings(188-188)runDevDoctor(176-276)readPackageScripts(135-174)
🔇 Additional comments (5)
docs/upgrade.md (1)
52-61: looks good and aligned with the new bootstrap flow.the new upgrade section clearly documents
doctor:dev,setup:dev, andverify, plus the one-time post-pull action. this is consistent and actionable. ref: lib/docs/upgrade.md:52.docs/development/LOCAL_DEV.md (1)
21-35: solid runbook coverage for setup and windows failure modes.this is clear, deterministic, and matches the scripted flow (
doctor:dev→npm ci→verify→ docs smoke), including windows lock-contention handling. ref: lib/docs/development/LOCAL_DEV.md:21 and lib/docs/development/LOCAL_DEV.md:67.Also applies to: 63-72, 79-85
test/scripts/doctor-dev.test.ts (1)
37-130: good deterministic regression coverage for doctor paths.the win32 path/pathext case, missing-script failure, unsupported node failure, and
EBUSYretry case give solid safety coverage for this bootstrap check. ref: test/scripts/doctor-dev.test.ts:37.test/scripts/setup-dev.test.ts (1)
35-74: nice coverage for event-order races and windows retry behavior.these tests directly lock down the child-process ordering race and bounded win32 retry flow, which is exactly the risky surface here. ref: test/scripts/setup-dev.test.ts:35 and test/scripts/setup-dev.test.ts:113.
Also applies to: 113-163
scripts/setup-dev.js (1)
76-144: bootstrap orchestration is clean and fail-fast.the stage order and bounded install retry loop are well structured, and the gate short-circuits on first failure as expected. ref: lib/scripts/setup-dev.js:76.
- verify npm/git executability in doctor:dev - require verify script in doctor preflight - add regression coverage for missing verify script - reorder first-clone testing gate flow - keep setup/doctor scripts importable in vitest by removing shebangs Co-authored-by: Codex <noreply@openai.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/doctor-dev.js`:
- Around line 223-227: The git check uses gitCheck.stdout directly but
runCommand guarantees stdout is never null; to make it consistent with the npm
check, read stdout into a local string using the same nullish-guard pattern
(e.g., derive gitOutput from gitCheck.stdout ?? "" and trim it) and then use
gitOutput for the emptiness check and the details.push string, referencing
runCommand, gitCheck and gitPath to locate the logic.
In `@scripts/setup-dev.js`:
- Around line 98-101: The call to runDoctor in runSetupDev is not awaited,
causing an early return of a Promise when an async runDevDoctorFn is injected;
change the code to await runDoctor (e.g., const doctorExitCode = await
runDoctor({ cwd })) before comparing to 0, and ensure runCommandFn usage remains
awaited; also add a regression test in test/scripts/setup-dev.test.ts that
injects an async runDevDoctorFn mock to verify npm ci and validation gates run
and that the function returns a numeric exit code rather than a Promise.
In `@test/scripts/doctor-dev.test.ts`:
- Around line 27-30: The test fixture currently creates extensionless
executables (writeFileSync(join(binDir, "npm"), ""), writeFileSync(join(binDir,
"git"), "")) which lets resolution succeed before PATHEXT is exercised; change
the setup in test/scripts/doctor-dev.test.ts so the binDir contains only the
Windows-specific executables (create only "npm.cmd" and "git.cmd" via
writeFileSync(join(binDir, "npm.cmd"), "") and writeFileSync(join(binDir,
"git.cmd"), "")) and remove or do not create the extensionless "npm" and "git"
files (references: writeFileSync and binDir) so the code path that relies on
PATHEXT fallback is actually executed during the test. Ensure the same change is
applied to the other fixture block around lines 43-75.
ℹ️ Review info
Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ee609c3c-521d-4d7f-b291-47b7b5cc65c4
📒 Files selected for processing (4)
docs/development/TESTING.mdscripts/doctor-dev.jsscripts/setup-dev.jstest/scripts/doctor-dev.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/scripts/doctor-dev.test.ts
docs/**
⚙️ CodeRabbit configuration file
keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.
Files:
docs/development/TESTING.md
🧬 Code graph analysis (1)
test/scripts/doctor-dev.test.ts (1)
scripts/doctor-dev.js (5)
scripts(125-125)scripts(249-249)warnings(187-187)runDevDoctor(174-284)readPackageScripts(133-172)
🔇 Additional comments (1)
docs/development/TESTING.md (1)
22-27: looks aligned with the updated dev and release workflow.the command set and first-clone ordering are now consistent with the bootstrap flow and release gate expectations in
lib/docs/development/TESTING.md:22-27andlib/docs/development/TESTING.md:51-63.Also applies to: 51-63
- await injected async doctor hook in setup flow - tighten PATHEXT fixture to exercise .cmd fallback path - normalize git version output handling pattern in doctor - add regression test for async doctor injection path Co-authored-by: Codex <noreply@openai.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/scripts/doctor-dev.test.ts`:
- Around line 75-98: The test for missing required scripts (using
createDoctorFixture and runDevDoctor) fails to stub spawnSync for npm/git
version checks; update the test to stub/override the spawnSync used by
runDevDoctor (as done in the win32 test) to return successful stdout for "npm
--version" and "git --version" so the version checks don't invoke the real
binaries in CI, then restore/cleanup the stub after the test; ensure the stub
covers calls where spawnSync is invoked with npm_execpath and git so the test
reliably asserts the missing "build" script.
- Around line 100-120: The test "fails fast on unsupported Node major version"
in test/scripts/doctor-dev.test.ts fails to stub spawnSync so external npm/git
checks can produce nondeterministic failures; update the test to stub
child_process.spawnSync (or the helper used by runDevDoctor) to return a benign
success result before calling runDevDoctor (similar to the other tests that stub
spawnSync), ensuring createDoctorFixture(), tempDirs setup, and the runDevDoctor
invocation remain unchanged so the test deterministically fails on nodeVersion
"v16.20.0" only.
- Around line 142-166: The test "fails when verify script is missing from
package.json" is missing a spawnSync stub so real npm/git version checks run;
update the test to pass a stubbed spawnSync into runDevDoctor (or mock
child_process.spawnSync) that returns expected stdout/exitCode for npm/git
version checks (e.g., version strings and exitCode 0) to prevent external
execution, keep existing error collection via error: (message) =>
errors.push(String(message)), and then run the same assertions; locate this in
the test around createDoctorFixture / runDevDoctor calls and add the spawnSync
stub implementation and pass it through the runDevDoctor options.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 82ccd324-c8c7-468e-b4d5-da9b9af059d7
📒 Files selected for processing (4)
scripts/doctor-dev.jsscripts/setup-dev.jstest/scripts/doctor-dev.test.tstest/scripts/setup-dev.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (1)
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/scripts/doctor-dev.test.tstest/scripts/setup-dev.test.ts
🧬 Code graph analysis (4)
test/scripts/doctor-dev.test.ts (1)
scripts/doctor-dev.js (5)
scripts(125-125)scripts(250-250)warnings(187-187)runDevDoctor(174-285)readPackageScripts(133-172)
scripts/setup-dev.js (1)
scripts/doctor-dev.js (10)
platform(75-75)platform(134-134)platform(176-176)execPath(178-178)cwd(175-175)env(74-74)env(177-177)error(34-34)error(185-185)runDevDoctor(174-285)
test/scripts/setup-dev.test.ts (1)
scripts/setup-dev.js (5)
resolveNpmInvocation(20-36)spawnFactory(39-39)runCommand(38-72)runCommandFn(78-78)runSetupDev(74-142)
scripts/doctor-dev.js (2)
scripts/setup-dev.js (5)
runCommand(38-72)platform(21-21)platform(76-76)env(40-40)cwd(75-75)scripts/audit-dev-allowlist.js (2)
process(139-147)entry(86-92)
🔇 Additional comments (19)
scripts/setup-dev.js (4)
38-72: runCommand settle pattern looks solid for event ordering.the settle guard at lines 44-50 correctly handles the race between
errorandexitevents, ensuring the promise resolves only once. SIGINT mapping to 130 and fallback to exit code 1 for signals are appropriate unix conventions.
74-101: doctor hook is now properly awaited.line 98 correctly awaits
runDoctor({ cwd })before checking the exit code, addressing the prior review concern about async injection. the injectablerunDevDoctorFnpattern enables deterministic testing.
106-124: npm ci retry with exponential backoff implemented correctly.lines 108-120 implement bounded retry with
installRetryAttemptsand exponential delay (baseDelayMs * 2 ** (attempt - 1)). windows defaults to 3 attempts, non-windows defaults to 1. injectablewaitFnenables fast test execution.
20-36: resolveNpmInvocation handles npm_execpath and win32 fallback correctly.lines 25-30 prioritize
npm_execpathwhen set (running via npm script context), and line 33 falls back tonpm.cmdon win32. this aligns with the test coverage attest/scripts/setup-dev.test.ts:10-33.test/scripts/setup-dev.test.ts (4)
35-74: good coverage for child-process event ordering races.lines 35-74 test the three critical event-ordering scenarios: error-before-exit (→1), exit-before-error (→0, first wins), and SIGINT (→130). these reproduce concurrency edge cases per coding guidelines.
99-120: regression test for async doctor hook injection present.lines 99-120 verify that an async
runDevDoctorFnis properly awaited before install and validation gates run. this addresses the prior review concern about Promise-vs-number return value corruption.
136-163: windows retry behavior test validates exponential backoff.lines 136-163 exercise the win32 retry path with 3 attempts, asserting
waitFnis called with delays 10ms and 20ms (exponential). mockedrunCommandFnfails twice then succeeds, verifying retry-then-success flow.
165-186: retry exhaustion test ensures proper failure propagation.lines 165-186 verify that when all 3 attempts fail, the setup returns exit code 1 and
waitFnis called exactly twice (no wait after final attempt). this confirms bounded retry semantics.test/scripts/doctor-dev.test.ts (4)
27-28: fixture now creates only .cmd files to exercise PATHEXT fallback.lines 27-28 create
npm.cmdandgit.cmdwithout extensionless counterparts, ensuringfindCommandInPathmust use PATHEXT expansion to locate commands. this addresses the prior review concern about false-positive resolution.
41-73: win32 PATHEXT test validates .cmd fallback path.lines 41-73 set
PATHEXT: ".EXE;.CMD"and verify doctor passes when only.cmdfiles exist. the spawnSync stub returns valid version output for git and npm. assertions confirm exit code 0, no errors, and expected log messages.
122-140: windows EBUSY retry regression test present.lines 122-140 simulate a transient
EBUSYerror on first read attempt, then succeed on second.baseDelayMs: 0andmaxAttempts: 2keep the test fast. this reproduces windows filesystem lock behavior per coding guidelines.
142-166: missing verify script regression test added.lines 142-166 verify that doctor fails with exit code 1 when the
verifyscript is absent from package.json. this addresses the prior review concern that doctor should validate verify presence before setup runs it.scripts/doctor-dev.js (7)
12-27: sleepSync implementation is sound for short retry delays.lines 17-20 use
SharedArrayBuffer+Atomics.waitfor precise synchronous sleep. lines 22-25 provide a busy-wait fallback if atomics aren't available (e.g., some restricted environments). the early return at line 14 handles zero/negative delays.
73-111: findCommandInPath correctly handles win32 PATHEXT expansion.lines 78-82 parse
PATHEXTinto lowercase extensions. lines 93-107 try the extensionless name first, then each extension variant. this ensuresnpmresolves tonpm.cmdwhen only.cmdexists.
199-217: npm executability now validated, not just path presence.lines 207-216 run
npm --versionvia the appropriate invocation and fail if status is non-zero or output is empty. this addresses the prior review concern about false-positive environments with unusable npm.
219-230: git executability validation consistent with npm check.lines 223-229 run
git --versionand push to failures if status is non-zero or output is empty. line 224 uses the nullish guard pattern (?? "") for consistency with the npm check.
250-262: requiredScripts now includes verify.line 251 lists
["typecheck", "lint", "test", "build", "verify"], ensuring doctor fails fast if verify is missing. this addresses the prior review concern about deferring failure to post-install.
133-172: readPackageScripts retry logic handles windows lock errors correctly.lines 153-168 implement bounded retry with exponential backoff for
EBUSY/EPERM/EACCESerrors on win32.sleepSyncprovides synchronous delay between attempts. returns null after all retries exhausted.
93-101: win32 extensionless candidate checked before PATHEXT variants.line 97 adds the extensionless
commandNameto candidates before iteratingwindowsExtensions. this means if bothnpmandnpm.cmdexist, the extensionless file wins. on typical windows setups this is fine, but if a directory namednpmexists, it could false-match.this is likely acceptable for dev environments, but worth noting.
- add shared spawnSync stub for npm/git version probes - use stub in missing-script, old-node, and missing-verify tests - keep doctor tests deterministic across CI host environments Co-authored-by: Codex <noreply@openai.com>
1 participant
Summary
verify/verify:cigate shared by local and CIWhat changed
scripts/doctor-dev.jsscripts/setup-dev.js(now runsverify+ docs integrity smoke)doctor:dev,setup:devformat,format:checkverify:repo,verify:quality,verify,verify:cirelease:checkbiome.jsonc@biomejs/biometo dev dependencieslint-stagedformatting for JSON/JSONC/YAMLnpm run verify:ciREADME.md,CONTRIBUTING.md,docs/development/TESTING.mddocs/development/LOCAL_DEV.mddocs/development/RELEASE_RUNBOOK.mdValidation evidence
npm run setup:devnpm run verify:cinpm run release:checkRisk and rollback
b120301and5d0da22note: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
established one-command local bootstrap (
setup:dev) with fail-fast prerequisite checks (doctor:dev). consolidated CI and local validation into canonicalverify/verify:cigates shared across workflows. both doctor and setup scripts explicitly handle windows filesystem concurrency (EBUSY/EPERM/EACCES retries with exponential backoff) and include comprehensive vitest coverage addressing previous review gaps.key changes
doctor-dev.jsvalidates node >=18, npm, git, repo structure, and required package scripts with windows lock retry logic.setup-dev.jsorchestrates doctor + npm ci + verify gates with windows install retries.test/scripts/doctor-dev.test.tsandtest/scripts/setup-dev.test.tscover windows PATHEXT handling, lock error retries, node version checks, spawn event races, and validation flows.verify:repo(hygiene + audit + lockfile check) +verify:quality(lint + typecheck + coverage) =verify=verify:ci. addeddoctor:dev,setup:dev,format,format:check,release:check.verify:ciinvocation. added concurrency cancellation (cancel-in-progress) and explicit job timeouts (30min/20min). full dependency audit remains non-blocking.@biomejs/biomefor json/jsonc/yaml formatting. scoped biome config excludes dist/coverage/node_modules/vendor. lint-staged now formats config files.LOCAL_DEV.md(contributor workflow + windows troubleshooting) andRELEASE_RUNBOOK.md(maintainer gate). updatedREADME.md,CONTRIBUTING.md,TESTING.md, upgrade guide, pr template, and docs indexes.windows concurrency hardening
readPackageScripts()in doctor-dev.js: detects EBUSY/EPERM/EACCES, retries 4x on windows with exponential backoff (40ms base delay), explicit error messaging about transient locks.runSetupDev()in setup-dev.js: retries npm ci 3x on windows with 300ms base delay.addressed previous comments
no token handling changes. no product runtime behavior changes.
Confidence Score: 5/5
Important Files Changed
verify:ciinvocation. adds concurrency cancellation (group by workflow+ref) and explicit 30min/20min timeouts. full audit kept non-blocking.Last reviewed commit: 8d61b31
Context used:
dashboard- What: Every code change must explain how it defends against Windows filesystem concurrency bugs and ... (source)