refactor(runtime): full Effect v4 runtime — services, streaming pipeline, tagged errors#304
Closed
aidenybai wants to merge 11 commits into
Closed
refactor(runtime): full Effect v4 runtime — services, streaming pipeline, tagged errors#304aidenybai wants to merge 11 commits into
aidenybai wants to merge 11 commits into
Conversation
Copy as promptReviewed by reactreview for commit 2605bd6. Configure here. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
cursor
Bot
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
cursor
Bot
changed the title
| const didLintFail = lintBindingMissing || result.didLintFail; | ||
| const lintFailureReason = lintBindingMissing | ||
| ? `oxlint native binding not found for Node ${process.version}; expected one matching ${OXLINT_NODE_REQUIREMENT}` | ||
| : result.lintFailureReason; |
There was a problem hiding this comment.
Score nullified when lint binding is missing
Medium Severity
When lintBindingMissing is true, the runtime correctly computes a score (since Linter.layerNoop doesn't fail), but inspect.ts unconditionally nullifies it via score: didLintFail ? null : result.score. The old code always called calculateScore(scoreDiagnostics) regardless of didLintFail, returning a real ScoreResult in the programmatic InspectResult. Callers relying on result.score being non-null when lint was skipped due to a missing binding now get null.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 4176089. Configure here.
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
Adds an internal, unpublished workspace package that demonstrates the
load-bearing architectural moves react-doctor would adopt to mirror
react-doctor-evals' Effect v4 design end-to-end. Nothing in the
existing CLI / inspect / diagnose paths is changed yet — this is the
parallel skeleton later slices will pull onto.
Patterns introduced:
- Schema is the wire (effect/Schema):
- diagnostic-schema.ts: Diagnostic + Severity + buildDiagnosticIdentity
(`${file}::${line}:${col}::${plugin}/${rule}`).
- json-report-schema.ts: JsonReport exposed as Schema.Union([JsonReportV1])
so adding schemaVersion: 2 later is one new union member, decoded by
every downstream consumer through one schema instead of trusting fields.
- Tagged-error facade with a reason union (errors.ts):
- Leaf TaggedErrorClass entries per concrete oxlint / config / project
failure mode (OxlintBinaryNotFound, OxlintTimedOut, OxlintOutputTooLarge,
OxlintOutputUnparseable, OxlintBatchFileDropped, OxlintSpawnFailed,
ConfigParseFailed, ProjectNotFound, NoReactDependency).
- One ReactDoctorError facade carrying reason: Schema.Union([...]).
- formatReactDoctorError as the single message renderer; switch over
reason._tag enforces exhaustiveness.
- Two orthogonal axes via Context.Service + Layer:
- Linter (linter.ts): cross-backend service with lint(input) returning
Stream<Diagnostic, ReactDoctorError>. layerOxlint wraps the existing
runOxlint (mapping legacy plain-Error throws to tagged reasons at one
boundary), layerNoop returns Stream.empty, layerOf(diagnostics)
is the test surface.
- Reporter (reporter.ts): emit(diagnostic) + finalize. layerNoop drops,
layerCapture stores into a Ref exposed via the second-service
ReporterCapture so tests can yield* Ref.get(yield* ReporterCapture)
instead of mocking I/O.
- Streaming pipeline (pipeline.ts): runDiagnosticPipeline composes
Linter.lint -> Stream.filter(keep) -> Reporter.emit and folds severity
counts in a single pass via Stream.runFoldEffect. Counts are
DiagnosticPipelineCounts; no intermediate diagnostic array is
materialized.
Tests (vite-plus/test) cover the wire boundaries and pipeline behavior
without spawning oxlint:
- diagnostic-schema.test.ts: decode happy path, rejects bad severity,
identity is deterministic.
- json-report-schema.test.ts: v1 round-trip, missing schemaVersion is a
decode failure (not a runtime crash later).
- errors.test.ts: facade renders, propagates as a tagged failure through
Effect.gen, recovers via Effect.catchTag.
- pipeline.test.ts: Linter.layerOf + Reporter.layerCapture verifies the
full pipeline (counts + emitted diagnostics), the keep predicate drops
weak-signal categories, and Linter.layerNoop yields zero counts/output.
Out of scope for this commit: replacing inspect() / diagnose() / the CLI,
deleting format-error-chain.ts, migrating tests off the legacy entry
points. Those are follow-up slices that pull onto this foundation
incrementally.
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
…strator + LintPartialFailures
Builds out the runtime so the existing CLI / inspect / diagnose paths can
ride on top of it. The orchestrating Effect (`runInspect`) composes
every Service the public entry points need; adding a new entry point
(LSP host, daemon, GitHub Action surface) is now a layer choice plus a
`runInspect` invocation, not a re-implementation of the orchestration.
New services:
- Project (project.ts): `discover(directory)` returning ProjectInfo or
a tagged ReactDoctorError. `layerNode` wraps
@react-doctor/project-info's `discoverProject` and translates
ProjectNotFoundError / NoReactDependencyError /
PackageJsonNotFoundError / AmbiguousProjectError into matching
tagged reasons (NoReactDependency, ProjectNotFound,
AmbiguousProject) so callers downstream get to match on the runtime
vocabulary. `layerOf` is the test surface.
- Config (config.ts): `resolve(directory)` returning
{ config, resolvedDirectory }. `layerNode` runs
loadConfigWithSource + resolveConfigRootDir in one place;
`layerOf` lets callers (tests, the public API after its own
redirect walk) bypass the on-disk re-load.
- Score (score.ts): `compute(diagnostics)` returning ScoreResult or
null. `layerHttp` (production), `layerOffline` (--offline),
`layerOf` (tests) are the three knobs the CLI already exposes.
- LintPartialFailures (linter.ts): a Ref<readonly string[]> exposed
as a service. `Linter.layerOxlint` pushes per-batch soft failures
there via Ref.update; the orchestrator reads it after the lint
stream completes. Replaces the legacy onPartialFailure callback +
closure-captured array; same downstream contract for
skippedCheckReasons['lint:partial'].
- AmbiguousProject reason (errors.ts): mirrors the legacy class with
directory + candidates fields, formatted by formatReactDoctorError.
Orchestration:
- runInspect(input, hooks): Effect<RunInspectOutput, ReactDoctorError,
Project | Config | Linter | LintPartialFailures | Reporter | Score>.
- Resolves config (and rootDir redirect) via Config.
- Discovers project via Project; raises tagged NoReactDependency
when the resolved project has no React.
- Streams diagnostics from Linter; uses Stream.tap for
Reporter.emit and Stream.catchTag("ReactDoctorError") to fold
mid-stream linter failures into didLintFail/lintFailureReason
instead of failing the orchestration.
- Pulls scoring diagnostics through filterDiagnosticsForSurface,
then computes the score via Score.
- beforeLint / afterLint hooks let surface code (CLI spinner,
project-detection block; later: LSP host's status bar) participate
without owning the orchestration.
- layerInspectLive: Project.layerNode + Config.layerNode +
Linter.layerOxlint + LintPartialFailures.layerLive +
Reporter.layerCapture + Score.layerHttp. Default production
stack; callers swap individual layers.
Tests (runtime package, 4 new + the existing 11 still green):
- runInspect composes everything with Project.layerOf + Config.layerOf
+ Linter.layerOf + Score.layerOf + Reporter.layerCapture and the
result aligns with the captured Reporter output.
- Stream.fail in the Linter folds into didLintFail with a
format-derived lintFailureReason; the orchestration does NOT
reject.
- Discovering a project without React yields the tagged
NoReactDependency reason in the failure channel.
- beforeLint / afterLint hooks fire in order with the right args.
The runtime tests now run alongside the existing react-doctor tests
under the workspace test script (--filter=@react-doctor/runtime added)
so regressions are caught in the same pnpm test pass.
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
…ffect runtime
The two public entry points of `react-doctor` (`inspect()` for the CLI,
`diagnose()` for the programmatic API) are now thin shells around the
runtime's `runInspect` Effect. The orchestration code that used to live
in two places — project discovery, config + rootDir redirect, lint
spawning + spinner UX, score round-trip, partial-failure plumbing —
now lives once in the runtime and is shared between them.
Public API behavior is preserved end-to-end. All 1,198 existing tests
remain green; the 15 runtime tests (11 foundation + 4 runInspect)
exercise the new orchestration paths.
inspect.ts:
- Loads config once via @react-doctor/core, merges options (so silent /
scoreOnly / verbose take effect from the first log line), then runs
the runtime via Effect.runPromise.
- Provides a layer stack tailored to the CLI flags: Linter.layerNoop
for --no-lint or when the oxlint native binding is missing,
Linter.layerOxlint otherwise; Score.layerOffline for --offline,
Score.layerHttp otherwise; Config.layerOf when the caller passed
configOverride (skips the on-disk re-load), Config.layerNode
otherwise.
- beforeLint / afterLint hooks drive the spinner + project-detection
block. Mid-stream lint failures surface back through the runtime's
didLintFail / lintFailureReason — the renderer keeps producing
skippedChecks: ['lint'] and the 'native binding' upgrade hint where
the legacy code did.
- Wraps `Effect.runPromise` rejections to restore the legacy
NoReactDependencyError class at the public API boundary, so
inspect.test.ts's '.rejects.toThrow("No React dependency found")'
contract continues to hold.
index.ts:
- diagnose() preserves its programmatic-API behavior: rootDir-redirect
via resolveConfigRootDir, then resolveDiagnoseTarget falls back to a
nested React subproject when the requested directory has no root
package.json. AmbiguousProjectError still throws as
AmbiguousProjectError. Once the directory is resolved, the
orchestration delegates to runInspect with Config.layerOf so the
runtime doesn't redo the redirect walk.
- Tagged-error → legacy-class translation lives in restoreLegacyThrow:
NoReactDependency → NoReactDependencyError, ProjectNotFound →
ProjectNotFoundError, AmbiguousProject → AmbiguousProjectError,
everything else falls back to a stringified message anchored to the
requested directory.
Build / packaging:
- `@react-doctor/runtime` joins react-doctor's devDependencies (same
bundling story as @react-doctor/core: workspace package, tree-shaken
into dist at build time).
- `effect` is added as a runtime dependency of react-doctor, marked as
`neverBundle` in vite.config.ts so the published bundle stays lean
and effect resolves through node_modules at install time.
- root `pnpm test` now also runs --filter=@react-doctor/runtime, so
the orchestrator + service tests run on every `pnpm test` invocation
alongside the 1,198 existing react-doctor tests.
spinner.ts: noopHandle's succeed/fail accept the displayText argument
so the SpinnerHandle interface unifies cleanly with ora's runtime
shape, fixing the closure-narrowing TS issue when the started spinner
is captured in a Ref-style holder for the Effect-based hooks.
What's intentionally out of scope here (follow-ups):
- Push the tagged errors *into* @react-doctor/core's runOxlint so the
mapLegacyOxlintError string-sniff at Linter.layerOxlint goes away.
- Reshape combineDiagnostics's filter chain into Stream.filter
operators inside runInspect (today the streaming pipeline still
collects diagnostics into an array before calling combineDiagnostics
— the hard parts of combineDiagnostics, like sync file-line reads
for suppression evaluation, deserve their own slice).
- A second Linter backend (Biome, ESLint worker pool) to prove the
layer swap empirically.
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
The lint-side of the codebase had three places that string-matched on
`error.message` to discriminate failure modes (per-batch
binary-split recovery in the oxlint runner; legacy → tagged
translation in the runtime; native-binding hint dispatch in the CLI
renderer). Tagged errors now flow end to end, so the discriminator is
`reason._tag` everywhere.
Errors live in core (errors.ts):
- @react-doctor/core gains `effect` as a runtime dependency and is
the canonical home for tagged ReactDoctorError + reason union.
- Leaf reasons: OxlintBinaryNotFound, OxlintNativeBindingFailed,
OxlintSpawnFailed, OxlintTimedOut, OxlintOutputTooLarge,
OxlintOutOfMemory, OxlintKilled, OxlintOutputUnparseable,
OxlintBatchFileDropped, ConfigParseFailed, ProjectNotFound,
NoReactDependency, AmbiguousProject.
- New typed `OxlintNativeBindingFailed` separates a runtime-side
oxlint native-binding crash from the pre-check
`OxlintBinaryNotFound` (resolveOxlintNode).
- `SPLITTABLE_OXLINT_REASONS` + `isSplittableReactDoctorError`
centralize which reason tags trigger the runner's binary-split
recovery (timeout, output-too-large, OOM).
- `@react-doctor/runtime/errors.ts` is now a re-export from core,
preserving the runtime's public surface without a circular dep.
run-oxlint.ts throws tagged errors directly:
- Timeout → OxlintTimedOut
- Output-size kill → OxlintOutputTooLarge
- SIGABRT → OxlintOutOfMemory; other signals → OxlintKilled
- stderr-only failure with "native binding" → OxlintNativeBindingFailed
- stderr-only failure otherwise → OxlintSpawnFailed
- spawn-level errors (ENOENT etc.) → OxlintSpawnFailed
- JSON parse / shape mismatch → OxlintOutputUnparseable
- isSplittableOxlintBatchError (the previous string-match helper) is
replaced by `isSplittableReactDoctorError` operating on the tag.
runtime/linter.ts deletes `mapLegacyOxlintError`:
- The 35-line `message.includes(...)` switch is gone.
- `Effect.tryPromise` catch is one line: pass-through ReactDoctorError,
wrap anything else in OxlintSpawnFailed.
RunInspectOutput.lintFailureReasonTag:
- New field carrying `reason._tag | null` so renderers dispatch off
the discriminant. The legacy `lintFailureReason` (the rendered
string) stays for skippedCheckReasons, but no consumer needs to
parse it now.
inspect.ts native-binding hint dispatch:
- Replaces `lintFailureReason.includes('native binding')` with
`lintFailureReasonTag === 'OxlintNativeBindingFailed' ||
lintFailureReasonTag === 'OxlintBinaryNotFound'`. Identical UX
for the user, no string format coupling between the runner and
the CLI.
handle-error.ts and build-json-report-error.ts:
- Tagged ReactDoctorErrors render through formatReactDoctorError so
the CLI error text and the JSON report's `error.message` /
`error.name` agree.
- Legacy Error chains still walk via getErrorChainMessages — the
third-party plugin / fs-throw escape hatch is preserved.
Tests (3 new, 18 total in runtime; all 1,198 react-doctor tests still
pass):
- isSplittableReactDoctorError discriminates by tag, not message text.
- formatReactDoctorError renders OxlintNativeBindingFailed with stderr
context (drives the upgrade-Node hint).
- A runOxlint-shaped Promise rejection with a tagged error
passes through the runtime's tryPromise catch unchanged.
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
…tDoctorError CLI error rendering now defers to formatReactDoctorError when the caught value is a ReactDoctorError, producing the same one-line message the JSON reporter writes. Falls back to the cause-chain walk for legacy Error throws (third-party plugin failures, fs permission, the surface the chain walker was originally written for). Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
The pipeline is now genuinely streaming. Diagnostics flow from `Linter.lint` through per-element `Stream.filterMap` of every auto-suppress / severity / ignore / inline-suppression transform straight into `Reporter.emit`, with no intermediate array. Every file IO read/list goes through a Layer-driven `Files` service so the suppression evaluator and the source-file walker are swappable via `Files.layerInMemory` for tests. Files service (runtime/files.ts): - readLines(path, root), listSourceFiles(root), isFile(path), isDirectory(path) — the four primitives the pipeline actually uses today; nothing speculative. - layerNode wraps the existing core/project-info helpers (createNodeReadFileLinesSync, listSourceFiles, isFile, isDirectory). One boundary where the runtime crosses into Node fs, instead of a sync `fs.readFileSync` call buried inside the suppression evaluator. - layerInMemory(tree: Map<absolutePath, content>) — the in-memory test surface. Implies directory existence by the prefix of any file path. Lets future suppression-eval tests skip temp dir setup entirely. - buildSyncReadFileLines(files, rootDirectory) adapts the service to the legacy sync callback shape `combineDiagnostics` and `mergeAndFilterDiagnostics` already consume — same readLines call site, but now layered. Per-element pipeline transform (core/build-diagnostic-pipeline.ts): - Single `buildDiagnosticPipeline(deps).apply(diagnostic)` closure applies the four legacy transforms in their original order: auto-suppress (test-noise tag + isTestFile), severity controls (drop on 'off', restamp on warn/error), ignore filters (rules / files / overrides + JSX-aware textComponents / rawTextWrapperComponents), inline suppressions (react-doctor-disable-next-line + near-miss hints). - Compilation costs (regex patterns, Set lookups, fileLines cache, testFileResultCache) are paid once per scan, captured in the closure. - mergeAndFilterDiagnostics now delegates to this transform, so the array entry point and the streaming pipeline share one code path. No second copy of the chain to keep in sync. runInspect end-to-end stream: - Stream.fromIterable(checkReducedMotion(scanDirectory)) prepends environment-side diagnostics (skipped in --diff mode). - Stream.concat(linter.lint(...)) appends the lint stream. - Stream.filterMap(filterMapNullable(transform.apply)) applies the per-element pipeline (drops on null). - Stream.tap(reporter.emit) pushes survivors to the Reporter as they emerge — a future LSP host / NDJSON cache / SARIF writer sees diagnostics mid-scan, not after runCollect. - combineDiagnostics is no longer called from runInspect. Public API entry points (react-doctor/src/inspect.ts and src/index.ts) provide Files.layerNode in their layer stacks. The new RunInspectHooks<HooksR> generic propagates the hook env so a caller can yield* from any service (the CLI uses it for Spinner in the next commit). Tests: - 2 new in runtime/tests/files.test.ts: layerInMemory exposes the four primitives; buildSyncReadFileLines adapts to the legacy callback. Combined with the run-inspect tests now providing Files.layerInMemory, the runtime test suite covers the streaming pipeline end to end without temp dirs. - All 1,198 react-doctor tests still pass — combineDiagnostics and mergeAndFilterDiagnostics keep their public signatures and routing through the new pipeline produces identical results. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
… + structural tag matching
Five focused additions that complete the layer set the runtime
needs to be genuinely production-shaped, plus a structural
`_tag` discriminator that fixes a vitest module-isolation bug on
the public-API `instanceof` check.
Cache.make for Config.layerNode:
- Replaces a one-shot per-scan `loadConfigWithSource` +
`resolveConfigRootDir` pair with `Cache.make` keyed on
directory, capacity 16, TTL 5 minutes. Watch mode, the LSP host,
and the webhook server only re-read config files when the cache
expires or callers invalidate. The legacy module-scoped `Map`
inside core's load-config helpers stays — it covers the
one-shot case where the runtime is bypassed.
Spinner Context.Service (runtime/spinner.ts):
- start(text) → SpinnerHandle; handle.succeed(text) /
handle.fail(text) return Effect<void>. Surface is intentionally
narrow: terminate exactly once.
- layerOra(factory) — CLI-supplied factory, keeps the runtime
package free of an ora dep. inspect.ts wraps the existing
ora-backed helper.
- layerNoop — handles whose succeed/fail are Effect.void; used
for --silent / --score / --json.
- layerCapture — records every Started / Succeeded / Failed
event into a Ref exposed via SpinnerCapture for unit tests
asserting on spinner UX without ora at all.
- inspect.ts now sources the spinner via the Spinner service
rather than calling ora helpers directly, so future surfaces
(LSP status bar, progress-bar reporter for big monorepos)
swap layers without touching inspect.ts.
Reporter.layerNdjson(filePath):
- Append-only NDJSON emission: every diagnostic encoded through
Schema.encodeUnknownSync(Diagnostic) and written as one JSON
line. Mirrors the eval harness's `.evals/<traceId>.jsonl`
shape; the parent directory is created lazily so callers don't
have to mkdirp.
- Schema-encode at the wire boundary is the same idiom used at
the read side — version evolution through Schema.Union, decode
failure as a tagged error, no field-trust on either side.
Linter.layerComposite([backend, backend, …]):
- Composes multiple Linter implementations by Stream.concat-ing
their lint streams. The downstream pipeline can't tell a
composite run apart from a single-backend run (and dedupe
inside runOxlint covers overlap).
- This is the layer swap the README has been promising since the
foundation PR — adding a Biome backend, an ESLint worker pool
for rules oxlint doesn't yet implement, or a fake test-only
backend is now `Linter.of({ lint })` followed by
`Linter.layerComposite([oxlint, biome, eslintWorkerPool])`.
Structural `_tag` discriminator at the public-API boundary:
- inspect.ts, index.ts, handle-error.ts, and
build-json-report-error.ts now use `isReactDoctorErrorLike`
(a structural `_tag === 'ReactDoctorError'` check) instead of
`instanceof ReactDoctorError`. Vitest's module isolation
produces different class identities for the dual
`Schema.TaggedErrorClass`-derived class at the catch site vs.
the throw site (the bundle graph shows both
`ReactDoctorError` and `ReactDoctorError$1`). The `_tag`
discriminator is the contract the runtime publishes
structurally; it's also what `Effect.catchTag` matches on, so
routing all class-identity checks through the same field
collapses two ways of saying "this is a runtime tagged error"
into one.
- Production code paths inside core/runtime keep `instanceof`
because they live in the same module-graph context where the
error is thrown.
Tests (10 new in runtime, 28 total):
- spinner.test.ts: layerCapture records events in order;
layerNoop emits Effect.void; layerOra delegates to the
injected factory.
- reporter-ndjson.test.ts: every emitted diagnostic is one
Schema-encoded JSON line; parent dir is created lazily.
- linter-composite.test.ts: backends concatenate in order; an
empty backend list yields Stream.empty; backends share the
LintPartialFailures Ref via the runtime context (proving the
composition layer interoperates with the rest of the service
graph).
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
React Doctor does not use Biome. The previous slice picked Biome
as a stand-in 'second linter backend' in inline comments, the
runtime README, and the linter-composite test's hypothetical
backend names — that read like we either ship a Biome integration
or are committed to one, neither of which is true.
Replaces every Biome mention with neutral phrasing ("a second
backend", "an in-process ESLint worker pool", "a sandboxed
runner") and renames the test fakes from `oxlintLike` /
`biomeLike` to `primaryBackend` / `secondaryBackend` so the
test reads as a layer-composition contract test rather than an
oxlint+Biome integration test. The composite layer's contract is
unchanged; what's adopted on top of it remains a downstream
product decision.
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
Targeted cleanup of things I implied or asserted that aren't actually true about React Doctor. Removed: - 'eslint plugin host' / 'GitHub Action' as runtime consumers in Config / handle-error / index.ts comments. The ESLint plugin depends only on `oxlint-plugin-react-doctor` (the rule registry); the GitHub Action is a composite action that shells out to `npx react-doctor@latest`. Neither uses `@react-doctor/runtime` directly. Replaced with the actual consumer pair: `inspect()` (CLI) and `diagnose()` (programmatic), with future runtime consumers (LSP host, watch mode) framed explicitly as hypothetical. - 'webhook server' as an example long-running process in Config.layerNode's cache rationale. React Doctor doesn't have a webhook server; that was bleed-through from the react-doctor-evals harness tour. Replaced with a generic 'any long-running runtime that keeps the runtime alive across multiple scans'. - Confident 'vitest module isolation can produce different class identities' explanation in 4 `isReactDoctorErrorLike` comments. I observed the symptom (`instanceof` unreliable across the module boundary) and fixed it (structural `_tag` check), but I don't actually know the root cause for certain — could be ESM live-binding init order, dual class names in the bundle graph (legacy project-info `ReactDoctorError` + the new tagged class), vitest's mock loader, or any combination. Comments now acknowledge the empirical observation without committing to a specific cause. - `OxlintBatchFileDropped` reason class. Defined and exported, never instantiated. The batch-drop case is reported through the `LintPartialFailures` Ref as plain strings (matching the legacy `onPartialFailure` callback contract); the unused tagged class was left over from an earlier design pass. Dropped from the union, the formatter, and the runtime's re-export surface. The runtime's index.ts also picks up `OxlintNativeBindingFailed`, `OxlintKilled`, `OxlintOutOfMemory`, and `isSplittableReactDoctorError` in the export list (they were exported from the runtime's errors.ts re-export but missing from the runtime index's named-export block). - Specific 'in-process ESLint worker pool', 'LSP publishDiagnostics adapter', 'FiberMap keyed by project root' framing in `runInspect` and `runDiagnosticPipeline` doc comments. Replaced with neutral 'future runtime consumers would swap whichever layers fit their surface — the orchestrator doesn't change'. The streaming-pipeline benefits doc now lists what's true today (bounded memory, mid-scan `Reporter.emit`) and separately frames future cancellation as 'the shape unlocks' rather than 'the runtime delivers'. Rewritten: - packages/runtime/README.md was the original parallel-skeleton README, claiming the package 'does not (yet) replace inspect()/diagnose()' and listing only Linter / Reporter / Pipeline / Schemas / Errors. Stale on every count: it now powers both public entry points; Project, Config, Files, Score, Spinner, LintPartialFailures, runInspect, Linter.layerComposite, Reporter.layerNdjson, and the buildDiagnosticPipeline transform are all part of the surface. Rewrote with the current service table, the streaming-pipeline shape, the orchestrator signature, the public-API rewires, and an honest 'what this package does NOT do' section that names the layer slots whose implementations are downstream product decisions (second linter backend, LSP host, SARIF reporter, watch mode). - The OxlintBatchFileDropped removal reflows build-json-report-error.ts and the formatter's exhaustive switch — TypeScript verifies coverage of the smaller union. All 1,226 tests still pass (1,198 react-doctor + 28 runtime); typecheck, lint, format clean. CLI smoke test against basic-react fixture is unchanged. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
main grew a `--dead-code` feature in commit aae328c that wires a deslop-js-based reachability pass into the legacy `inspect()` / `diagnose()` flow. After rebasing onto main, the rebase replayed that feature on top of the runtime-rewired entry points, but the new orchestration didn't yet know about dead-code. This commit brings the feature back through the runtime so the streaming pipeline picks up dead-code diagnostics with the same Layer-driven contract every other axis already follows. DeadCode Context.Service (runtime/dead-code.ts): - compute(rootDirectory, userConfig): Effect<readonly Diagnostic[], Error> - layerNode wraps @react-doctor/core's checkDeadCode (which delegates to deslop-js); failures are surfaced as Error so the orchestrator can fold them into didDeadCodeFail. - layerNoop returns []; used when --no-dead-code or config.deadCode = false. - layerOf(diagnostics) is the test surface; mirrors Linter.layerOf. runInspect orchestration: - New RunInspectInput.runDeadCode flag (the orchestrator additionally gates on !isDiffMode since reachability is whole-project, matching how checkReducedMotion is gated). - RunInspectOutput.didDeadCodeFail / .deadCodeFailureReason mirror the lint failure pair, so callers distinguish 'skipped on purpose' from 'tried and failed'. - Dead-code diagnostics flow through the same Stream.filterMap pipeline as lint and environment diagnostics: prepend env, then the lint stream, then the dead-code stream, then the per-element transform, then Reporter.emit. - DeadCode failures are caught with Effect.matchEffect at the service boundary and folded into the deadCodeFailure Ref — never sinking the whole scan. Public API: - inspect.ts threads options.deadCode through ResolvedInspectOptions, passes runDeadCode to the runtime, and renders a separate 'Analyzing dead code...' spinner line after lint finalizes (so two ora frame loops don't compete for stderr). didDeadCodeFail surfaces as skippedChecks: ['dead-code'] with skippedCheckReasons['dead-code'] in the InspectResult. - index.ts (diagnose) wires the same runDeadCode flag and provides DeadCode.layerNode / layerNoop based on options.deadCode. DiagnoseResult now carries skippedChecks + skippedCheckReasons to match the new public type — programmatic consumers see exactly the same shape the legacy diagnose() returned post-#291. Tests (3 new in runtime, 31 total; 4 new in react-doctor for the dead-code feature itself, 1,202 total there): - runtime/tests/dead-code.test.ts: layerOf returns the supplied diagnostics; layerNoop returns []; service shape supports Error-typed failure channel. - runtime/tests/run-inspect.test.ts: every test layer stack now includes DeadCode.layerOf([]) so the runtime's expanded service graph is satisfied; baseInput sets runDeadCode: false to match the test scenarios that don't exercise dead-code. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
cursor
Bot
force-pushed
the
cursor/effect-runtime-foundation-6abb
branch
from
8531c93 to
2605bd6
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 2605bd6. Configure here.
| ...(result.skippedCheckReasons | ||
| ? { skippedCheckReasons: result.skippedCheckReasons } | ||
| : {}), | ||
| skippedChecks: [], |
There was a problem hiding this comment.
toJsonReport drops skippedChecks and skippedCheckReasons from report
High Severity
toJsonReport now hardcodes skippedChecks: [] and completely drops skippedCheckReasons from the per-project scan entry. The old code forwarded result.skippedChecks and conditionally spread result.skippedCheckReasons from the DiagnoseResult. When dead-code analysis fails, diagnose() populates both fields, but the JSON report will always claim no checks were skipped — silently discarding actionable failure information for programmatic API consumers.
Reviewed by Cursor Bugbot for commit 2605bd6. Configure here.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| const skippedChecks: string[] = []; | ||
| const skippedCheckReasons: Record<string, string> = {}; | ||
| if (deadCodeFailureReason !== null) { | ||
| if (result.didDeadCodeFail) { |
There was a problem hiding this comment.
The diagnose() function does not handle didLintFail or lintPartialFailures, causing incomplete skippedChecks and skippedCheckReasons when linting fails.
Member
Author
|
Superseded by the merged Effect v4 stack: #421, #426, #427, #431, #432, #433, #434. The full Effect v4 runtime is now on main with the canonical v4 patterns (Context.Service, Schema.TaggedErrorClass union, ChildProcess via @effect/platform-node, Effect.catchReasons, Effect.Console-based renderers, NodeResolver / StagedFiles / Git / Files / Project / Config / Linter / DeadCode / Score / Reporter / Progress services). See AGENTS.md → Effect v4 Conventions for the current shape. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this is
react-doctorruns on a small Effect v4 runtime layer. The two public entry points (inspect()for the CLI,diagnose()for the programmatic API) share one orchestration Effect (runInspect) and one Service vocabulary. The diagnostic pipeline is genuinely streaming end-to-end, every error is a taggedReactDoctorErrorwith areasonunion, every file IO crosses oneFilesservice boundary, and the layer set is complete enough that adding a second linter backend, an LSP host, an NDJSON cache, or a SARIF reporter is "one new layer" rather than "rewrite the orchestration."Rebased onto current
main(which added a--dead-codefeature inaae328ccand a score-payload gzip fix ind16d2c7b). The dead-code feature is integrated through the runtime as a newDeadCodeContext.Service; the gzip fix is internal tocalculateScoreand the runtime'sScore.layerHttppicks it up transparently.All 1,202 react-doctor tests pass (1,198 pre-
--dead-code+ 4 new for that feature). 31 new runtime tests cover the foundation, orchestration, streaming pipeline, file-IO service, dead-code service, spinner service, ndjson reporter, composite linter, and tagged-error contracts.Architecture
Services (one
Context.Serviceper orthogonal axis)ProjectlayerNode(wrapsdiscoverProject)layerOf(projectInfo)Configreact-doctor.config.json+rootDirredirect, cached viaCache.make(capacity 16, TTL 5min)layerNodelayerOf({ config, resolvedDirectory })FileslayerNodelayerInMemory(Map<absolutePath, content>)LinterlayerOxlint,layerComposite([...])layerNoop,layerOf(diagnostics)DeadCodelayerNode(wrapscheckDeadCode/ deslop-js)layerNoop,layerOf(diagnostics)Reporteremit(diagnostic)+finalizelayerNdjson(path)(Schema-encoded NDJSON),layerCapturelayerNoop,layerCaptureScorelayerHttplayerOffline,layerOf(result)SpinnerlayerOra(factory)layerNoop,layerCapture(records events)LintPartialFailureslayerLive(Ref)The only production linter today is
Linter.layerOxlint—layerComposite([...])is the layer slot a future second backend would plug into without changing the orchestrator. Direct consumers of these services today are exactly two:inspect()(the CLI's entry point) anddiagnose()(the programmatic API).Tagged errors with a
reasonunion — end to endReactDoctorError { reason: Schema.Union([...]) }lives in@react-doctor/core(the runtime re-exports without a circular dep). Every leaf failure is aSchema.TaggedErrorClass:OxlintBinaryNotFound,OxlintNativeBindingFailed,OxlintSpawnFailed,OxlintTimedOut,OxlintOutputTooLarge,OxlintOutOfMemory,OxlintKilled,OxlintOutputUnparseable,ConfigParseFailed,ProjectNotFound,NoReactDependency,AmbiguousProject.runOxlintraises tagged errors directly. Everyerror.message.includes(...)site across the lint boundary is gone.Public API entry points (
inspect(),diagnose()) translate runtime tagged errors back into the legacy classes (NoReactDependencyError,ProjectNotFoundError,AmbiguousProjectError) at the boundary using a structural_tagcheck.Schema is the wire
runtime/diagnostic-schema.ts—Diagnostic+Severity+buildDiagnosticIdentity.runtime/json-report-schema.ts—JsonReportasSchema.Union([JsonReportV1]).Reporter.layerNdjsonSchema-encodes every diagnostic at the wire boundary.Streaming pipeline — actually streaming
runInspect's diagnostic flow isStream-driven end to end with no intermediate array materialization:Stream.fromIterable(checkReducedMotion(scanDirectory))prepends environment-side diagnostics (skipped in--diffmode).Stream.concat(Linter.lint(...))appends the lint stream, withStream.catchTag("ReactDoctorError", ...)folding mid-stream linter failures intodidLintFail.Stream.concat(deadCodeStream)appends dead-code diagnostics (skipped in--diff/--stagedmode since reachability is whole-project; failure folds intodidDeadCodeFailviaEffect.matchEffect).Stream.filterMap(filterMapNullable(transform.apply))applies the per-element auto-suppress / severity / ignore / inline-suppression chain.Stream.tap(reporter.emit)pushes survivors to the Reporter as they emerge.Stream.runCollectproduces the final array for the score / surface filter / public API.The per-element transform lives in
core/build-diagnostic-pipeline.tsand is shared withmergeAndFilterDiagnostics— there is no second copy of the chain.runInspectorchestrationrunInspect(input, hooks): Effect<RunInspectOutput, ReactDoctorError, Project | Config | DeadCode | Files | Linter | LintPartialFailures | Reporter | Score | HooksR>:Config.resolve(directory)→{ config, resolvedDirectory }Project.discover(resolvedDirectory)→ProjectInfo(raisesNoReactDependencywhenreactVersion === null)buildDiagnosticPipeline({ files: Files, ... })→ per-element transformLinter.lint(...)andDeadCode.compute(...)streams →Stream.tap(Reporter.emit)→ folded countsScore.compute(...)over the surface-filtered subsetRunInspectOutputwithproject,userConfig,diagnostics,score,didLintFail,lintFailureReason,lintFailureReasonTag,lintPartialFailures,didDeadCodeFail,deadCodeFailureReasonRunInspectHooks<HooksR>is parametric on the hook environment so a caller canyield* AnyServicefrom a hook (the CLI does this forSpinner).Public API rewires
inspect.ts(CLI) — Loads config + merges options, then runs the runtime viaEffect.runPromise. CLI-tailored layer stack:Linter.layerNoopfor--no-lint/ missing oxlint native binding,Linter.layerOxlintotherwise;Score.layerOfflinefor--offline;DeadCode.layerNodewhenoptions.deadCodeis true,DeadCode.layerNoopotherwise;Spinner.layerNoopfor--silent/--score/--json,Spinner.layerOra(oraFactory)otherwise.beforeLint/afterLinthooks drive the existing project-detection rendering and the lint spinner; a separate dead-code spinner line renders after lint finalizes.index.ts(diagnose) — PreservesresolveDiagnoseTargetauto-fallback to nested React subprojects andresolveConfigRootDirredirect;AmbiguousProjectErrorstill throws as itself. WiresDeadCode.layerNode/layerNoopbased onoptions.deadCode. ReturnsskippedChecks+skippedCheckReasonsto match the post-#291 public type.Tests (31 new in runtime, all 1,202 react-doctor tests green)
diagnostic-schema.test.ts,json-report-schema.test.ts,errors.test.ts,pipeline.test.ts,tagged-errors.test.ts— schemas + tagged errorsrun-inspect.test.ts— full orchestration with all-layerOfservices +Files.layerInMemory+DeadCode.layerOf([]); mid-streamStream.failfolds intodidLintFail; missing React yieldsNoReactDependency;beforeLint/afterLinthooks fire in orderfiles.test.ts—layerInMemoryexposes the four primitivesdead-code.test.ts—layerOfreturns the supplied diagnostics;layerNoopreturns[]; service exposesError-typed failure channelspinner.test.ts—layerCapture/layerNoop/layerOrareporter-ndjson.test.ts— Schema-encoded NDJSON line per emitlinter-composite.test.ts— backends concatenate; backends share theLintPartialFailuresRef via the runtime contextVerification
CLI smoke test on
tests/fixtures/basic-react:Dead-code diagnostics flow end-to-end through the streaming pipeline.
Commits on the branch (after rebase)
chore: ignore .repos/ scratch directory for cloned reference reposfeat(runtime): introduce @react-doctor/runtime — Effect v4 foundationfeat(runtime): add Project, Config, Score services + runInspect orchestrator + LintPartialFailuresrefactor(react-doctor): rewire inspect() and diagnose() through the Effect runtimerefactor: kill magic-string error sniffing across the lint boundaryrefactor(handle-error): render tagged ReactDoctorError via formatReactDoctorErrorfeat(runtime): Files service + end-to-end streaming diagnostic pipelinefeat(runtime): Cache.make config, Spinner / NDJSON / Composite layers + structural tag matchingdocs(runtime): drop Biome as the canonical Linter-swap examplefix(runtime): scrub overstated and stale claims across docs and commentsfeat(runtime): integrate dead-code analysis as a Context.Service— new commit on top of the rebasePackaging
@react-doctor/runtimeis private (workspace, not published).@react-doctor/corehaseffect@4.0.0-beta.69as a runtime dependency.effect@4.0.0-beta.69isreact-doctor'sdependenciesandneverBundleinvite.config.ts.pnpm testruns--filter=react-doctor --filter=@react-doctor/runtime.Out of scope (sensible follow-ups)
Reporter.layerCapturewith aReporter.layerLspcallingconnection.sendDiagnosticsperemit.Reporter.layerNdjsonkeyed by(filePath, contentHash)for incremental scans, withStreamcancellation per project.vi.mock("ora", …)ininspect.test.tswith aSpinner.layerCapture-driven test onceinspect()exposes a layer-injection seam.