Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR adds the Effect library and rewrites registry control flow and error handling to use Result/Effect patterns, replacing many Promise/throw-based paths with typed error unions, Result utilities, and effectful I/O across registry resolution, parsing, planning, and installation. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes -重点审查:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Preview Deployment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
packages/core/src/registry/config-root.ts (1)
6-8:isDirrefactor preserves semantics; consider dropping redundantasync.
Since you’re returning the promise chain directly,asyncisn’t adding value here (but keeping it is harmless).-async function isDir(p: string): Promise<boolean> { +function isDir(p: string): Promise<boolean> { return stat(p).then((s) => s.isDirectory()).catch(() => false); }package.json (1)
15-15: Consider pinning theeffectversion for reproducibility.Using
"latest"can lead to unexpected breaking changes when the dependency is updated. Sinceeffectis a core library for error handling across the registry module, consider pinning to a specific version (e.g.,"3.x.x") to ensure consistent behavior across environments and CI runs.packages/core/src/registry/embedded.ts (1)
14-18: Consider addingas constto the error type for stricter discrimination.For better type narrowing with discriminated unions, you could use a const assertion or readonly modifier on
_tag:export type EmbeddedReadFailed = { - _tag: "EmbeddedReadFailed"; + readonly _tag: "EmbeddedReadFailed"; name: string; cause: unknown; };This prevents accidental mutation and improves TypeScript's ability to narrow the type in switch/if statements.
packages/cli/index.ts (1)
89-100: Consider extracting the inline type forprintHooks.The inline type
Array<{ item: { kind: string; name: string }; postinstall: null | { commands: string[] } }>is functional but verbose. IfInstallPlanis exported from core, you could usePick<InstallPlan, 'item' | 'postinstall'>[]for clarity and to stay in sync with the core types.packages/core/src/registry/install.ts (2)
106-109: Consider explicit handling for unexpected mode values.The
toModefunction defaults to0o755for any value other than"0644". If an invalid mode string is passed, this silent fallback could lead to unexpected permissions.function toMode(mode: "0644" | "0755"): number { if (mode === "0644") return 0o644; + if (mode === "0755") return 0o755; + // TypeScript ensures exhaustiveness, but this guards against runtime issues return 0o755; }
315-381: Clean Result-based validation flow.The function properly validates each entry and file path, returning early on validation errors. The Result type makes error propagation explicit and type-safe.
I noticed the TODO on line 375 about
bun adddefaults. Would you like me to open an issue to track implementing dependency installation support?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
package.json(1 hunks)packages/cli/index.ts(5 hunks)packages/core/package.json(1 hunks)packages/core/src/registry/config-root.ts(1 hunks)packages/core/src/registry/embedded.ts(3 hunks)packages/core/src/registry/errors.ts(1 hunks)packages/core/src/registry/index.ts(1 hunks)packages/core/src/registry/install.ts(5 hunks)packages/core/src/registry/jsonc.ts(9 hunks)packages/core/src/registry/registry.ts(2 hunks)packages/core/src/registry/resolve.ts(1 hunks)packages/core/src/registry/result.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
package.json
📄 CodeRabbit inference engine (CLAUDE.md)
package.json: Usebun installinstead ofnpm install,yarn install, orpnpm installfor dependency management
Usebun run <script>instead ofnpm run <script>,yarn run <script>, orpnpm run <script>for running scripts defined in package.json
Files:
package.json
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Usebun <file>instead ofnode <file>orts-node <file>for running scripts
Bun automatically loads .env files, so don't use the dotenv package
Usebun:sqlitefor SQLite database operations instead ofbetter-sqlite3
UseBun.redisfor Redis operations instead ofioredis
UseBun.sqlfor Postgres database operations instead ofpgorpostgres.js
Use built-inWebSocketinstead of thewspackage
PreferBun.fileovernode:fsreadFile/writeFile methods for file operations
UseBun.$for shell commands instead ofexeca
Files:
packages/core/src/registry/resolve.tspackages/core/src/registry/config-root.tspackages/core/src/registry/errors.tspackages/core/src/registry/jsonc.tspackages/core/src/registry/result.tspackages/cli/index.tspackages/core/src/registry/registry.tspackages/core/src/registry/install.tspackages/core/src/registry/embedded.tspackages/core/src/registry/index.ts
**/*.{html,ts,tsx,css}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
bun build <file.html|file.ts|file.css>instead ofwebpackoresbuildfor bundling
Files:
packages/core/src/registry/resolve.tspackages/core/src/registry/config-root.tspackages/core/src/registry/errors.tspackages/core/src/registry/jsonc.tspackages/core/src/registry/result.tspackages/cli/index.tspackages/core/src/registry/registry.tspackages/core/src/registry/install.tspackages/core/src/registry/embedded.tspackages/core/src/registry/index.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: UseBun.serve()with built-in WebSocket, HTTPS, and routing support instead of express
Usebun --hotto run TypeScript files with hot module reloading for development
Files:
packages/core/src/registry/resolve.tspackages/core/src/registry/config-root.tspackages/core/src/registry/errors.tspackages/core/src/registry/jsonc.tspackages/core/src/registry/result.tspackages/cli/index.tspackages/core/src/registry/registry.tspackages/core/src/registry/install.tspackages/core/src/registry/embedded.tspackages/core/src/registry/index.ts
**/*.{html,tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
Use HTML imports with
Bun.serve()for frontend development instead of vite
Files:
packages/core/src/registry/resolve.tspackages/core/src/registry/config-root.tspackages/core/src/registry/errors.tspackages/core/src/registry/jsonc.tspackages/core/src/registry/result.tspackages/cli/index.tspackages/core/src/registry/registry.tspackages/core/src/registry/install.tspackages/core/src/registry/embedded.tspackages/core/src/registry/index.ts
🧠 Learnings (7)
📚 Learning: 2025-12-04T21:23:25.944Z
Learnt from: CR
Repo: better-slop/ocx PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-04T21:23:25.944Z
Learning: Applies to **/*.{ts,tsx} : Use `bun --hot` to run TypeScript files with hot module reloading for development
Applied to files:
package.json
📚 Learning: 2025-12-04T21:23:25.944Z
Learnt from: CR
Repo: better-slop/ocx PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-04T21:23:25.944Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use `bun <file>` instead of `node <file>` or `ts-node <file>` for running scripts
Applied to files:
package.json
📚 Learning: 2025-12-04T21:23:25.944Z
Learnt from: CR
Repo: better-slop/ocx PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-04T21:23:25.944Z
Learning: Applies to **/*.{html,ts,tsx,css} : Use `bun build <file.html|file.ts|file.css>` instead of `webpack` or `esbuild` for bundling
Applied to files:
package.json
📚 Learning: 2025-12-04T21:23:25.944Z
Learnt from: CR
Repo: better-slop/ocx PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-04T21:23:25.944Z
Learning: Applies to package.json : Use `bun run <script>` instead of `npm run <script>`, `yarn run <script>`, or `pnpm run <script>` for running scripts defined in package.json
Applied to files:
package.json
📚 Learning: 2025-12-04T21:23:25.944Z
Learnt from: CR
Repo: better-slop/ocx PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-04T21:23:25.944Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use `Bun.$` for shell commands instead of `execa`
Applied to files:
package.json
📚 Learning: 2025-12-04T21:23:25.944Z
Learnt from: CR
Repo: better-slop/ocx PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-04T21:23:25.944Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Bun automatically loads .env files, so don't use the dotenv package
Applied to files:
package.json
📚 Learning: 2025-12-04T21:23:25.944Z
Learnt from: CR
Repo: better-slop/ocx PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-04T21:23:25.944Z
Learning: Applies to package.json : Use `bun install` instead of `npm install`, `yarn install`, or `pnpm install` for dependency management
Applied to files:
package.json
🧬 Code graph analysis (3)
packages/core/src/registry/jsonc.ts (1)
packages/core/src/registry/result.ts (3)
Result(13-13)err(19-21)ok(15-17)
packages/cli/index.ts (4)
packages/core/src/registry/index.ts (6)
ResolveRegistryTreeError(13-13)InstallError(10-10)resolveRegistryTree(5-5)isErr(6-6)planInstalls(1-1)applyInstallPlans(1-1)packages/core/src/registry/resolve.ts (1)
resolveRegistryTree(59-64)packages/core/src/registry/result.ts (2)
isErr(27-29)err(19-21)packages/core/src/registry/install.ts (2)
planInstalls(315-381)applyInstallPlans(426-434)
packages/core/src/registry/registry.ts (4)
packages/core/src/registry/index.ts (5)
RegistryItemParseError(12-12)FetchRegistryItemError(9-9)Result(15-15)RegistryItemV1(23-23)RegistryItem(22-22)packages/core/src/registry/result.ts (5)
Result(13-13)ok(15-17)err(19-21)toEffect(59-62)runEffect(64-73)packages/core/src/registry/types.ts (2)
RegistryItemV1(14-30)RegistryItem(32-32)packages/core/src/registry/embedded.ts (1)
getEmbeddedRegistryItemEffect(20-49)
⏰ 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). (2)
- GitHub Check: Agent
- GitHub Check: CodeQL analysis (javascript-typescript)
🔇 Additional comments (29)
packages/core/package.json (1)
16-17: LGTM!Correctly referencing the
effectdependency from the workspace catalog ensures consistent versioning across packages.packages/core/src/registry/index.ts (1)
1-24: LGTM!The barrel file cleanly organizes the public API surface with proper separation of runtime exports and type-only exports. The new Result utilities (
isErr,isOk) and error types are appropriately exposed for consumers.packages/core/src/registry/embedded.ts (1)
31-48: LGTM!Good use of
Effect.tryPromiseto wrap the async file read operation with proper error mapping. The functional pipeline cleanly transforms the file content into the registry item structure.packages/core/src/registry/resolve.ts (2)
16-57: LGTM!The Effect-based DFS implementation correctly handles:
- Cycle detection: Using a
visitingset with proper cleanup viaEffect.ensuring- Deduplication: Using a
mapto skip already-resolved items- Sequential traversal:
concurrency: 1ensures proper DFS ordering- Post-order output: Items are pushed to
outafter their dependencies resolveThe closure over mutable state (
map,visiting,out) is safe here due to sequential execution.
12-14: The suggestion to addas constis unnecessary. ThedependencyCyclefunction is already consistent with all other error types in the codebase. None of the error type definitions inpackages/core/src/registry/errors.tsusereadonlyon_tag—this includesJSONCError,RegistryItemParseError,FetchRegistryItemError,ResolveRegistryTreeError, andInstallError. The current implementation matches the established pattern across the entire error type system.packages/core/src/registry/result.ts (1)
1-73: Well-structured Result type implementation.The discriminated union approach with
_tagis idiomatic and the combinators (map,flatMap,mapError) correctly propagate errors.One consideration:
runEffectusesEffect.runPromisewhich will reject on defects (unexpected errors). If a defect occurs inside the Effect, the outer Promise will reject rather than returning anErr. This is likely intentional since defects represent programmer errors, but worth documenting for consumers.packages/core/src/registry/jsonc.ts (4)
14-22: Good internal Result helpers.The
fail<T>helper cleanly wraps error creation. The generic parameter allows proper type inference at call sites.
212-228: Consistent Result propagation infindValueEnd.The early-return pattern on
._tag === "Err"correctly short-circuits and preserves the error type.
421-448: Public API correctly returns Result.The refactored
getTopLevelJsoncPropertyValueTextproperly chains Result checks and returnsok(null)for missing properties vsok(value)for found ones.
450-496:upsertTopLevelJsoncPropertyhandles all Result paths.Error propagation is complete through
rootCloseRes,spanRes, andformattedValueRes. Both update and insert paths correctly returnok(...).packages/core/src/registry/registry.ts (5)
8-34: Clean error factory functions.These constructors align with the error types in
errors.tsand provide a single place to create each variant.
62-154: ThoroughparseV1validation with Result-based error handling.All required fields are validated, and the structured error messages will help users diagnose malformed registry items. The
filesloop correctly accumulates valid entries while short-circuiting on the first error.
164-194: Effect helpers correctly wrap I/O and parsing.
fetchJSONEffectproperly distinguishes network failures from HTTP status errorsreadTextEffectusesBun.fileas per coding guidelinesparseJsonEffectusesEffect.tryfor synchronous parsing
201-241: Well-structured Effect pipeline for registry item fetching.The fallback chain (embedded → URL → file → unknown) is clear. The
mapErrorat Line 212 correctly widensEmbeddedReadFailedto the fullFetchRegistryItemErrorunion for uniform error handling downstream.
243-248: Clean Promise-based public API wrapper.
fetchRegistryItembridges the Effect-based implementation to a Promise-returning API viarunEffect, maintaining backward compatibility for consumers that prefer Promises.packages/core/src/registry/errors.ts (1)
1-32: Comprehensive error taxonomy with discriminated unions.The
_tagdiscriminant enables exhaustive pattern matching in consumers like the CLI'sformatError. The compositional approach (e.g.,ResolveRegistryTreeErrorincludingFetchRegistryItemError) avoids duplication while preserving type precision.Minor observation: The
cause: unknownfields on several variants allow capturing the underlying error, which is good for debugging. Consider whether you want to expose a utility to safely extract stack traces from these causes for logging.packages/cli/index.ts (2)
30-82: Exhaustive error formatting via switch on_tag.The switch covers all variants of
ResolveRegistryTreeErrorandInstallError. TypeScript will flag missing cases if new error variants are added to the unions, ensuring this stays in sync.
218-222: Catch block ensures CLI exits gracefully on unexpected errors.The
.catchonparseAsynchandles any unhandled rejections and sets the exit code appropriately.packages/core/src/registry/install.ts (11)
1-9: Imports look good.The imports are well-organized, bringing in the necessary Node.js modules for filesystem operations, the Effect library for the new error-handling pattern, and local modules for types and utilities.
11-23: Type definitions are well-structured.The
InstalledOCXItemandOCXManagedConfigtypes clearly define the shape of installed items and the managed configuration.
25-55: Error constructors follow a clean discriminated union pattern.The tagged error objects with
_tagdiscriminators enable exhaustive pattern matching and provide good context for each error type.
57-80: Utility functions are correctly implemented.The helper functions handle edge cases properly—
isRecordexcludes arrays and null, andstringifyCausegracefully handles both Error instances and other thrown values.
82-98: Solid path validation with security considerations.The function correctly prevents path traversal attacks by checking for absolute paths, parent directory escapes, and empty paths. Using
path.posixensures consistent behavior across platforms.
111-155: Effect wrappers are well-structured with proper error mapping.The filesystem operation wrappers consistently convert native errors to typed
InstallErrorvalues. UsingBun.writealigns with the coding guidelines for file operations.
157-220: Well-designed atomic write and staging patterns.The
withTmpDirEffectcorrectly implements resource acquisition/release semantics. ThestageDirEffectfunction provides important security validation by checking that writes don't escape the target directory. The atomic rename pattern ensures crash-safe installations.
222-253: Robust JSONC parsing with graceful fallbacks.The function handles missing or malformed
ocxproperties gracefully, usingensureRecordMapto safely coerce each item kind. This defensive approach prevents crashes from corrupted config files.
283-313: Postinstall execution with appropriate opt-in control.The function correctly uses sequential execution (
concurrency: 1) for postinstall commands and requires explicit opt-in viaallowPostinstall. Usingbash -lcensures the user's shell profile is loaded, which is typical for tools that need proper PATH resolution.
383-424: Effectful installation pipeline is well-structured.The function properly sequences the installation steps: creating directories, staging files atomically, updating configuration, and optionally running postinstall scripts. The assumption that all plans share the same
configRoot(line 390) is reasonable for batch installation operations.
426-438: Clean async wrapper maintains backward compatibility.The
applyInstallPlanswrapper provides a familiar Promise-based API for consumers not using Effect directly, whilerunEffecthandles the Effect-to-Result conversion internally.
There was a problem hiding this comment.
Pull request overview
This PR incrementally adopts the effect library to refactor the registry module's error handling, replacing traditional throws/try-catch patterns with typed, explicit failure paths using Result types. This change improves type safety and makes error paths explicit throughout the registry APIs.
Key changes:
- Introduced Result type and Effect-based error handling infrastructure
- Updated registry APIs (fetch, resolve, install) to return
Result<T, E>types - Modified CLI to handle and format structured errors from the registry
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
packages/core/src/registry/result.ts |
New Result type implementation with utilities for Effect integration |
packages/core/src/registry/errors.ts |
Defines typed error unions for registry operations |
packages/core/src/registry/registry.ts |
Refactored fetch and parse logic using Effect and Result |
packages/core/src/registry/resolve.ts |
Converted dependency resolution to Effect-based flow |
packages/core/src/registry/install.ts |
Migrated install operations to Effect with structured errors |
packages/core/src/registry/jsonc.ts |
Updated JSONC parsing to return Result types |
packages/core/src/registry/embedded.ts |
Converted embedded item fetching to Effect |
packages/core/src/registry/config-root.ts |
Minor refactoring of error handling |
packages/core/src/registry/index.ts |
Updated exports to include Result types and error utilities |
packages/cli/index.ts |
Added error formatting and handling for structured Result types |
packages/core/package.json |
Added effect dependency from workspace catalog |
package.json |
Added effect to workspace catalog |
bun.lock |
Locked effect@3.19.11 and dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Addressed review comments:
I didn’t open a separate issue for the |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
packages/core/src/registry/config-root.ts (1)
6-7: Consider surfacing errors instead of silently catching them.The change from
asyncto promise chaining is correct and maintains the same behavior. However, the.catch(() => false)silently swallows all errors (permission denied, I/O errors, etc.), which seems inconsistent with this PR's goal of adopting Effect for explicit, typed failure paths.While returning
falsemay be acceptable for a simple "does directory exist?" check, it prevents distinguishing between "not a directory" and "error accessing path," potentially masking legitimate infrastructure issues.Would you like me to suggest a refactor using
ResultorEffectpatterns that surfaces errors explicitly while maintaining backward compatibility?packages/core/src/registry/embedded.ts (1)
14-48: MakeEmbeddedReadFailedfully immutable (minor typing polish).
_tagisreadonlybutname/causeare not; consider marking all fields readonly for consistency with other error variants.export type EmbeddedReadFailed = { readonly _tag: "EmbeddedReadFailed"; - name: string; - cause: unknown; + readonly name: string; + readonly cause: unknown; };packages/core/src/registry/install.ts (1)
147-149: Consider not swallowingstat()errors other than ENOENT.
pathExistsEffectcurrently treats any error as “false”; permission issues can later surface as confusing write/rename failures.-function pathExistsEffect(p: string): Effect.Effect<boolean, never> { - return Effect.promise(() => stat(p).then(() => true).catch(() => false)); +function pathExistsEffect(p: string): Effect.Effect<boolean, InstallError> { + return Effect.tryPromise({ + try: async () => { + await stat(p); + return true; + }, + catch: (cause) => ioError("stat", cause, p), + }).pipe( + // If you want “missing => false”, map only ENOENT-like cases here (platform-specific) + ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
package.json(1 hunks)packages/cli/index.ts(5 hunks)packages/core/src/registry/config-root.ts(1 hunks)packages/core/src/registry/embedded.ts(3 hunks)packages/core/src/registry/install.ts(5 hunks)packages/core/src/registry/jsonc.ts(9 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
package.json
📄 CodeRabbit inference engine (CLAUDE.md)
package.json: Usebun installinstead ofnpm install,yarn install, orpnpm installfor dependency management
Usebun run <script>instead ofnpm run <script>,yarn run <script>, orpnpm run <script>for running scripts defined in package.json
Files:
package.json
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Usebun <file>instead ofnode <file>orts-node <file>for running scripts
Bun automatically loads .env files, so don't use the dotenv package
Usebun:sqlitefor SQLite database operations instead ofbetter-sqlite3
UseBun.redisfor Redis operations instead ofioredis
UseBun.sqlfor Postgres database operations instead ofpgorpostgres.js
Use built-inWebSocketinstead of thewspackage
PreferBun.fileovernode:fsreadFile/writeFile methods for file operations
UseBun.$for shell commands instead ofexeca
Files:
packages/core/src/registry/install.tspackages/core/src/registry/config-root.tspackages/core/src/registry/jsonc.tspackages/cli/index.tspackages/core/src/registry/embedded.ts
**/*.{html,ts,tsx,css}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
bun build <file.html|file.ts|file.css>instead ofwebpackoresbuildfor bundling
Files:
packages/core/src/registry/install.tspackages/core/src/registry/config-root.tspackages/core/src/registry/jsonc.tspackages/cli/index.tspackages/core/src/registry/embedded.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: UseBun.serve()with built-in WebSocket, HTTPS, and routing support instead of express
Usebun --hotto run TypeScript files with hot module reloading for development
Files:
packages/core/src/registry/install.tspackages/core/src/registry/config-root.tspackages/core/src/registry/jsonc.tspackages/cli/index.tspackages/core/src/registry/embedded.ts
**/*.{html,tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
Use HTML imports with
Bun.serve()for frontend development instead of vite
Files:
packages/core/src/registry/install.tspackages/core/src/registry/config-root.tspackages/core/src/registry/jsonc.tspackages/cli/index.tspackages/core/src/registry/embedded.ts
🧠 Learnings (8)
📚 Learning: 2025-12-04T21:23:25.944Z
Learnt from: CR
Repo: better-slop/ocx PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-04T21:23:25.944Z
Learning: Applies to **/*.{ts,tsx} : Use `bun --hot` to run TypeScript files with hot module reloading for development
Applied to files:
package.json
📚 Learning: 2025-12-04T21:23:25.944Z
Learnt from: CR
Repo: better-slop/ocx PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-04T21:23:25.944Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use `bun <file>` instead of `node <file>` or `ts-node <file>` for running scripts
Applied to files:
package.json
📚 Learning: 2025-12-04T21:23:25.944Z
Learnt from: CR
Repo: better-slop/ocx PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-04T21:23:25.944Z
Learning: Applies to **/*.{html,ts,tsx,css} : Use `bun build <file.html|file.ts|file.css>` instead of `webpack` or `esbuild` for bundling
Applied to files:
package.json
📚 Learning: 2025-12-04T21:23:25.944Z
Learnt from: CR
Repo: better-slop/ocx PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-04T21:23:25.944Z
Learning: Applies to package.json : Use `bun run <script>` instead of `npm run <script>`, `yarn run <script>`, or `pnpm run <script>` for running scripts defined in package.json
Applied to files:
package.json
📚 Learning: 2025-12-04T21:23:25.944Z
Learnt from: CR
Repo: better-slop/ocx PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-04T21:23:25.944Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use `Bun.$` for shell commands instead of `execa`
Applied to files:
package.json
📚 Learning: 2025-12-04T21:23:25.944Z
Learnt from: CR
Repo: better-slop/ocx PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-04T21:23:25.944Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Bun automatically loads .env files, so don't use the dotenv package
Applied to files:
package.json
📚 Learning: 2025-12-04T21:23:25.944Z
Learnt from: CR
Repo: better-slop/ocx PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-04T21:23:25.944Z
Learning: Applies to package.json : Use `bun install` instead of `npm install`, `yarn install`, or `pnpm install` for dependency management
Applied to files:
package.json
📚 Learning: 2025-12-04T21:23:25.944Z
Learnt from: CR
Repo: better-slop/ocx PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-04T21:23:25.944Z
Learning: Applies to **/*.{test,spec}.{ts,tsx} : Use `bun:test` with `test` and `expect` functions for unit testing in TypeScript
Applied to files:
package.json
🧬 Code graph analysis (3)
packages/core/src/registry/jsonc.ts (2)
packages/core/src/registry/result.ts (3)
Result(13-13)err(19-21)ok(15-17)packages/core/src/registry/index.ts (2)
Result(15-15)JSONCError(11-11)
packages/cli/index.ts (3)
packages/core/src/registry/types.ts (1)
InstallPlan(34-61)packages/core/src/registry/resolve.ts (1)
resolveRegistryTree(59-64)packages/core/src/registry/result.ts (2)
isErr(27-29)err(19-21)
packages/core/src/registry/embedded.ts (1)
packages/core/src/registry/types.ts (1)
RegistryItemV1(14-30)
🔇 Additional comments (5)
packages/core/src/registry/embedded.ts (1)
20-48: Effect pipeline looks solid; verifyEffect.tryPromisemapping semantics on your Effect version.
This is a clean conversion from throw/await to typed failure. Just ensureEffect.tryPromise({ try, catch })behaves as expected ineffect@3.19.11(esp. what counts as “caught” rejections vs sync throws).package.json (1)
10-16: Dependency catalog additions: verify workspace resolution + runtime compatibility.
Please confirm the workspace “catalog:” resolution produces the intendedeffect@3.19.11(and that it’s compatible with your Bun/TS setup).packages/cli/index.ts (1)
85-145: Result-driven CLI flow is clear and consistent.
TheisErrearly-returns + centralizedexitWithErrorkeeps control flow straightforward and avoids try/catch sprawl.packages/core/src/registry/jsonc.ts (1)
14-23: Nice Result-based refactor; verify downstream callers handleErreverywhere.
The internal helpers + explicit propagation look consistent; just make sure every consumer ofgetTopLevelJsoncPropertyValueText/upsertTopLevelJsoncPropertynow checks_tag(no lingering throw/assume-success paths).Also applies to: 80-297, 414-496
packages/core/src/registry/install.ts (1)
256-289: Good fix: don’t swallow config read errors anymore.
This addresses the earlier “.catch(() => "{}")” pitfall by only treating a missing file as{}and mapping other failures toIOError.
| function formatError(error: ResolveRegistryTreeError | InstallError): string { | ||
| switch (error._tag) { | ||
| case "DependencyCycle": | ||
| return `Registry dependency cycle detected at ${error.at}`; | ||
|
|
||
| case "MissingSpec": | ||
| return "Missing registry item spec"; | ||
|
|
||
| case "UnknownSpec": | ||
| return `Unknown registry spec: ${error.spec} (try embedded item, URL, or path to .json)`; | ||
|
|
||
| case "EmbeddedReadFailed": | ||
| return `Failed to read embedded item ${error.name}: ${stringifyCause(error.cause)}`; | ||
|
|
||
| case "FetchFailed": | ||
| return error.status === undefined | ||
| ? `Failed to fetch registry item: ${error.url}${error.cause ? ` (${stringifyCause(error.cause)})` : ""}` | ||
| : `Failed to fetch registry item: ${error.url} (${error.status})`; | ||
|
|
||
| case "FileReadFailed": | ||
| return `Failed to read file: ${error.path} (${stringifyCause(error.cause)})`; | ||
|
|
||
| case "JSONParseFailed": | ||
| return `Failed to parse JSON from ${error.source}: ${stringifyCause(error.cause)}`; | ||
|
|
||
| case "RegistryItemParseError": | ||
| return error.message; | ||
|
|
||
| case "NoPlans": | ||
| return "No install plans to apply"; | ||
|
|
||
| case "MissingConfigRoot": | ||
| return "Missing config root"; | ||
|
|
||
| case "InvalidRegistryFilePath": | ||
| return `Invalid registry file path ${error.path}: ${error.message}`; | ||
|
|
||
| case "WriteOutsideTargetDir": | ||
| return `Refusing to write outside target dir: ${error.path}`; | ||
|
|
||
| case "TargetAlreadyExists": | ||
| return `Target already exists: ${error.path} (use --overwrite)`; | ||
|
|
||
| case "IOError": | ||
| return `I/O error (${error.operation})${error.path ? `: ${error.path}` : ""} (${stringifyCause(error.cause)})`; | ||
|
|
||
| case "PostinstallFailed": | ||
| return `Postinstall command failed (${error.code}): ${error.cmd}`; | ||
|
|
||
| case "JSONCError": | ||
| return error.message; | ||
| } | ||
| } |
There was a problem hiding this comment.
Add an exhaustiveness guard (prevents silent “undefined” on new error variants).
If a new _tag is added upstream, formatError can fall through and return undefined, producing confusing CLI output.
function formatError(error: ResolveRegistryTreeError | InstallError): string {
switch (error._tag) {
...
case "JSONCError":
return error.message;
+ default: {
+ const _exhaustive: never = error;
+ return `Unexpected error: ${String((_exhaustive as any)?._tag ?? "unknown")}`;
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function formatError(error: ResolveRegistryTreeError | InstallError): string { | |
| switch (error._tag) { | |
| case "DependencyCycle": | |
| return `Registry dependency cycle detected at ${error.at}`; | |
| case "MissingSpec": | |
| return "Missing registry item spec"; | |
| case "UnknownSpec": | |
| return `Unknown registry spec: ${error.spec} (try embedded item, URL, or path to .json)`; | |
| case "EmbeddedReadFailed": | |
| return `Failed to read embedded item ${error.name}: ${stringifyCause(error.cause)}`; | |
| case "FetchFailed": | |
| return error.status === undefined | |
| ? `Failed to fetch registry item: ${error.url}${error.cause ? ` (${stringifyCause(error.cause)})` : ""}` | |
| : `Failed to fetch registry item: ${error.url} (${error.status})`; | |
| case "FileReadFailed": | |
| return `Failed to read file: ${error.path} (${stringifyCause(error.cause)})`; | |
| case "JSONParseFailed": | |
| return `Failed to parse JSON from ${error.source}: ${stringifyCause(error.cause)}`; | |
| case "RegistryItemParseError": | |
| return error.message; | |
| case "NoPlans": | |
| return "No install plans to apply"; | |
| case "MissingConfigRoot": | |
| return "Missing config root"; | |
| case "InvalidRegistryFilePath": | |
| return `Invalid registry file path ${error.path}: ${error.message}`; | |
| case "WriteOutsideTargetDir": | |
| return `Refusing to write outside target dir: ${error.path}`; | |
| case "TargetAlreadyExists": | |
| return `Target already exists: ${error.path} (use --overwrite)`; | |
| case "IOError": | |
| return `I/O error (${error.operation})${error.path ? `: ${error.path}` : ""} (${stringifyCause(error.cause)})`; | |
| case "PostinstallFailed": | |
| return `Postinstall command failed (${error.code}): ${error.cmd}`; | |
| case "JSONCError": | |
| return error.message; | |
| } | |
| } | |
| function formatError(error: ResolveRegistryTreeError | InstallError): string { | |
| switch (error._tag) { | |
| case "DependencyCycle": | |
| return `Registry dependency cycle detected at ${error.at}`; | |
| case "MissingSpec": | |
| return "Missing registry item spec"; | |
| case "UnknownSpec": | |
| return `Unknown registry spec: ${error.spec} (try embedded item, URL, or path to .json)`; | |
| case "EmbeddedReadFailed": | |
| return `Failed to read embedded item ${error.name}: ${stringifyCause(error.cause)}`; | |
| case "FetchFailed": | |
| return error.status === undefined | |
| ? `Failed to fetch registry item: ${error.url}${error.cause ? ` (${stringifyCause(error.cause)})` : ""}` | |
| : `Failed to fetch registry item: ${error.url} (${error.status})`; | |
| case "FileReadFailed": | |
| return `Failed to read file: ${error.path} (${stringifyCause(error.cause)})`; | |
| case "JSONParseFailed": | |
| return `Failed to parse JSON from ${error.source}: ${stringifyCause(error.cause)}`; | |
| case "RegistryItemParseError": | |
| return error.message; | |
| case "NoPlans": | |
| return "No install plans to apply"; | |
| case "MissingConfigRoot": | |
| return "Missing config root"; | |
| case "InvalidRegistryFilePath": | |
| return `Invalid registry file path ${error.path}: ${error.message}`; | |
| case "WriteOutsideTargetDir": | |
| return `Refusing to write outside target dir: ${error.path}`; | |
| case "TargetAlreadyExists": | |
| return `Target already exists: ${error.path} (use --overwrite)`; | |
| case "IOError": | |
| return `I/O error (${error.operation})${error.path ? `: ${error.path}` : ""} (${stringifyCause(error.cause)})`; | |
| case "PostinstallFailed": | |
| return `Postinstall command failed (${error.code}): ${error.cmd}`; | |
| case "JSONCError": | |
| return error.message; | |
| default: { | |
| const _exhaustive: never = error; | |
| return `Unexpected error: ${String((_exhaustive as any)?._tag ?? "unknown")}`; | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/cli/index.ts around lines 31 to 83, the switch over error._tag is
not exhaustive so adding new error variants upstream can cause formatError to
return undefined; add an exhaustiveness guard as the final branch (either a
default case or an assertNever-style function) that throws or logs a clear error
and/or returns a generic fallback string including the unexpected tag and
serialized error so the CLI never returns undefined and new error variants are
obvious during runtime.
| function runPostinstallEffect(plan: InstallPlan): Effect.Effect<void, InstallError> { | ||
| if (!plan.postinstall) return Effect.void; | ||
|
|
||
| return Effect.forEach( | ||
| plan.postinstall.commands, | ||
| (cmd) => | ||
| Effect.tryPromise({ | ||
| try: async () => { | ||
| const proc = Bun.spawn({ | ||
| cmd: ["bash", "-lc", cmd], | ||
| cwd: plan.postinstall?.cwd, | ||
| stdout: "inherit", | ||
| stderr: "inherit", | ||
| stdin: "inherit", | ||
| env: { | ||
| ...process.env, | ||
| }, | ||
| }); | ||
|
|
||
| return await proc.exited; | ||
| }, | ||
| catch: (cause): InstallError => ioError("postinstall", cause), | ||
| }).pipe( | ||
| Effect.flatMap((code) => { | ||
| if (code === 0) return Effect.void; | ||
| return Effect.fail(postinstallFailed(cmd, code)); | ||
| }), | ||
| ), | ||
| { concurrency: 1 }, | ||
| ).pipe(Effect.asVoid); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l packages/core/src/registry/install.tsRepository: better-slop/ocx
Length of output: 100
🏁 Script executed:
head -330 packages/core/src/registry/install.ts | tail -40Repository: better-slop/ocx
Length of output: 1151
🏁 Script executed:
sed -n '285,330p' packages/core/src/registry/install.tsRepository: better-slop/ocx
Length of output: 1341
🏁 Script executed:
sed -n '346,365p' packages/core/src/registry/install.tsRepository: better-slop/ocx
Length of output: 614
🏁 Script executed:
rg -n "type InstallError|class InstallError" packages/core/src/registry/install.ts -A 20Repository: better-slop/ocx
Length of output: 41
🏁 Script executed:
rg -n "postinstall" packages/core/src/registry/install.tsRepository: better-slop/ocx
Length of output: 806
🏁 Script executed:
rg -n "InstallError" packages/core/src/registry/install.ts | head -20Repository: better-slop/ocx
Length of output: 1488
🏁 Script executed:
rg -B5 -A10 "type.*InstallError\s*=" packages/core/src/registry/install.tsRepository: better-slop/ocx
Length of output: 41
🏁 Script executed:
fd -t f "*.ts" packages/core/src | xargs rg -l "InstallError" | head -5Repository: better-slop/ocx
Length of output: 492
🏁 Script executed:
rg -B2 -A15 "^export type InstallError|^type InstallError" packages/core/src/Repository: better-slop/ocx
Length of output: 903
🏁 Script executed:
python3 << 'EOF'
import os
import pathlib
# Test path.relative() behavior equivalent
def test_path_escape(root, user_input):
# Simulate what the code does
resolved = os.path.normpath(os.path.join(root, user_input))
relative = os.path.relpath(resolved, root)
escapes = relative.startswith("..")
print(f"Input: {user_input!r}")
print(f" Root: {root}")
print(f" Resolved: {resolved}")
print(f" Relative: {relative}")
print(f" Escapes: {escapes}")
print()
root = "/home/user/project"
# Test cases
test_path_escape(root, ".")
test_path_escape(root, "subdir")
test_path_escape(root, "../..")
test_path_escape(root, "../../etc/passwd")
test_path_escape(root, "/etc/passwd")
test_path_escape(root, "/home/user/project/safe")
EOFRepository: better-slop/ocx
Length of output: 145
🏁 Script executed:
rg -n "postinstall" packages/core/src/registry/install.ts -B2 -A2 | head -40Repository: better-slop/ocx
Length of output: 1263
🏁 Script executed:
find packages -name "*.test.ts" -o -name "*.spec.ts" | xargs rg -l "postinstall" 2>/dev/null | head -5Repository: better-slop/ocx
Length of output: 216
🏁 Script executed:
rg -B5 -A10 "interface InstallPlan|type InstallPlan" packages/core/src/registry/install.tsRepository: better-slop/ocx
Length of output: 41
Restrict postinstall.cwd to prevent path traversal outside project root.
The current code at line 356 allows postinstall.cwd values like ../../.. or absolute paths to escape configRoot.rootDir, enabling postinstall hooks to execute outside the project when using remote manifests. Add validation using path.relative() to ensure the resolved path stays within the project boundary:
const postinstall = item.postinstall
? {
commands: item.postinstall.commands,
- cwd: path.resolve(configRoot.rootDir, item.postinstall.cwd ?? "."),
+ cwd: (() => {
+ const cwdAbs = path.resolve(configRoot.rootDir, item.postinstall.cwd ?? ".");
+ const rel = path.relative(configRoot.rootDir, cwdAbs);
+ if (rel.startsWith("..") || path.isAbsolute(rel)) {
+ throw new Error(`postinstall.cwd must stay within project root: ${item.postinstall.cwd ?? "."}`);
+ }
+ return cwdAbs;
+ })(),
}
: null;Alternatively, add a dedicated InstallError variant (e.g., PostinstallCwdEscape) and return it from planInstalls instead of throwing.
Also applies to: line 356
There was a problem hiding this comment.
@coderabbitai we have different plans for this implementation, likely one that includes a verified registry list that is allowed postinstall. otherwise, we deny
There was a problem hiding this comment.
@cau1k, thanks for clarifying! A verified registry list with postinstall allowlisting is a solid security model — it centralizes trust at the registry level rather than validating individual paths. That approach will make the security boundary clearer and easier to audit.
Looking forward to seeing how the registry verification is implemented when you get to it. Let me know if you'd like me to review that part when it's ready! 🐰
🐰💭
(thinking about
verified
registries...)
✏️ Learnings added
Learnt from: cau1k
Repo: better-slop/ocx PR: 6
File: packages/core/src/registry/install.ts:292-322
Timestamp: 2025-12-13T05:48:32.141Z
Learning: In packages/core/src/registry/install.ts, the project plans to implement a verified registry list approach for postinstall security. Registries on the verified list will be allowed to execute postinstall hooks, while unverified registries will be denied. This is the preferred security model over individual path validation for postinstall.cwd.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| return Effect.gen(function* () { | ||
| yield* mkdirEffect(root.opencodeDir); | ||
|
|
||
| const exists = await pathExists(plan.item.targetDir); | ||
| if (exists) { | ||
| if (!overwrite) { | ||
| throw new Error( | ||
| `Target already exists: ${plan.item.targetDir} (use --overwrite)`, | ||
| ); | ||
| } | ||
| await rm(plan.item.targetDir, { recursive: true, force: true }); | ||
| for (const plan of plans) { | ||
| const dir = yield* stageDirEffect(plan, opts.overwrite); | ||
| wrote.push(dir); | ||
| } | ||
|
|
||
| await rename(tmp, plan.item.targetDir); | ||
| return plan.item.targetDir; | ||
| } catch (err) { | ||
| await rm(tmp, { recursive: true, force: true }); | ||
| throw err; | ||
| } | ||
| } | ||
|
|
||
| async function updateConfig( | ||
| path: string, | ||
| plans: InstallPlan[], | ||
| ): Promise<void> { | ||
| const text = await Bun.file(path).text().catch(() => "{}"); | ||
| yield* mkdirEffect(path.dirname(root.configPath)); | ||
| yield* updateConfigEffect(root.configPath, plans); | ||
|
|
||
| const cfg = parseExistingOCXManagedConfig(text); | ||
| let ran = false; | ||
|
|
||
| for (const plan of plans) { | ||
| const { kind, name, source, entryRel } = plan.item; | ||
| const dir = entryRel.split("/").slice(0, -1).join("/"); | ||
| if (opts.allowPostinstall) { | ||
| for (const plan of plans) { | ||
| if (!plan.postinstall) continue; | ||
| ran = true; | ||
| yield* runPostinstallEffect(plan); | ||
| } | ||
| } | ||
|
|
||
| cfg.items[kind][name] = { | ||
| source, | ||
| dir, | ||
| entry: entryRel, | ||
| ...(plan.postinstall ? { postinstall: plan.postinstall } : {}), | ||
| return { | ||
| wroteFiles: wrote, | ||
| editedConfigPath: root.configPath, | ||
| ranPostinstall: ran, | ||
| }; | ||
| } | ||
|
|
||
| const updated = upsertTopLevelJsoncProperty(text, "ocx", cfg); | ||
| await writeAtomic(path, updated); | ||
| } | ||
|
|
||
| async function runPostinstall(plan: InstallPlan): Promise<void> { | ||
| if (!plan.postinstall) return; | ||
|
|
||
| for (const cmd of plan.postinstall.commands) { | ||
| const proc = Bun.spawn({ | ||
| cmd: ["bash", "-lc", cmd], | ||
| cwd: plan.postinstall.cwd, | ||
| stdout: "inherit", | ||
| stderr: "inherit", | ||
| stdin: "inherit", | ||
| env: { | ||
| ...process.env, | ||
| }, | ||
| }); | ||
|
|
||
| const code = await proc.exited; | ||
| if (code !== 0) { | ||
| throw new Error(`Postinstall command failed (${code}): ${cmd}`); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
ranPostinstall should reflect “executed at least one command”.
Right now ran = true for any plan.postinstall, even if commands is empty.
- if (opts.allowPostinstall) {
+ if (opts.allowPostinstall) {
for (const plan of plans) {
- if (!plan.postinstall) continue;
- ran = true;
+ if (!plan.postinstall || plan.postinstall.commands.length === 0) continue;
+ ran = true;
yield* runPostinstallEffect(plan);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return Effect.gen(function* () { | |
| yield* mkdirEffect(root.opencodeDir); | |
| const exists = await pathExists(plan.item.targetDir); | |
| if (exists) { | |
| if (!overwrite) { | |
| throw new Error( | |
| `Target already exists: ${plan.item.targetDir} (use --overwrite)`, | |
| ); | |
| } | |
| await rm(plan.item.targetDir, { recursive: true, force: true }); | |
| for (const plan of plans) { | |
| const dir = yield* stageDirEffect(plan, opts.overwrite); | |
| wrote.push(dir); | |
| } | |
| await rename(tmp, plan.item.targetDir); | |
| return plan.item.targetDir; | |
| } catch (err) { | |
| await rm(tmp, { recursive: true, force: true }); | |
| throw err; | |
| } | |
| } | |
| async function updateConfig( | |
| path: string, | |
| plans: InstallPlan[], | |
| ): Promise<void> { | |
| const text = await Bun.file(path).text().catch(() => "{}"); | |
| yield* mkdirEffect(path.dirname(root.configPath)); | |
| yield* updateConfigEffect(root.configPath, plans); | |
| const cfg = parseExistingOCXManagedConfig(text); | |
| let ran = false; | |
| for (const plan of plans) { | |
| const { kind, name, source, entryRel } = plan.item; | |
| const dir = entryRel.split("/").slice(0, -1).join("/"); | |
| if (opts.allowPostinstall) { | |
| for (const plan of plans) { | |
| if (!plan.postinstall) continue; | |
| ran = true; | |
| yield* runPostinstallEffect(plan); | |
| } | |
| } | |
| cfg.items[kind][name] = { | |
| source, | |
| dir, | |
| entry: entryRel, | |
| ...(plan.postinstall ? { postinstall: plan.postinstall } : {}), | |
| return { | |
| wroteFiles: wrote, | |
| editedConfigPath: root.configPath, | |
| ranPostinstall: ran, | |
| }; | |
| } | |
| const updated = upsertTopLevelJsoncProperty(text, "ocx", cfg); | |
| await writeAtomic(path, updated); | |
| } | |
| async function runPostinstall(plan: InstallPlan): Promise<void> { | |
| if (!plan.postinstall) return; | |
| for (const cmd of plan.postinstall.commands) { | |
| const proc = Bun.spawn({ | |
| cmd: ["bash", "-lc", cmd], | |
| cwd: plan.postinstall.cwd, | |
| stdout: "inherit", | |
| stderr: "inherit", | |
| stdin: "inherit", | |
| env: { | |
| ...process.env, | |
| }, | |
| }); | |
| const code = await proc.exited; | |
| if (code !== 0) { | |
| throw new Error(`Postinstall command failed (${code}): ${cmd}`); | |
| } | |
| } | |
| }); | |
| return Effect.gen(function* () { | |
| yield* mkdirEffect(root.opencodeDir); | |
| for (const plan of plans) { | |
| const dir = yield* stageDirEffect(plan, opts.overwrite); | |
| wrote.push(dir); | |
| } | |
| yield* mkdirEffect(path.dirname(root.configPath)); | |
| yield* updateConfigEffect(root.configPath, plans); | |
| let ran = false; | |
| if (opts.allowPostinstall) { | |
| for (const plan of plans) { | |
| if (!plan.postinstall || plan.postinstall.commands.length === 0) continue; | |
| ran = true; | |
| yield* runPostinstallEffect(plan); | |
| } | |
| } | |
| return { | |
| wroteFiles: wrote, | |
| editedConfigPath: root.configPath, | |
| ranPostinstall: ran, | |
| }; | |
| }); |
🤖 Prompt for AI Agents
In packages/core/src/registry/install.ts around lines 406 to 432, the
ranPostinstall flag is set to true for any plan that has a postinstall object
even if that postinstall has no commands; change the loop to only treat
postinstall as executed when it actually contains commands (e.g., skip plans
where !plan.postinstall or plan.postinstall.commands is empty), set ran = true
only when you will run the commands, and then yield* runPostinstallEffect(plan)
so ranPostinstall accurately reflects "executed at least one command."
Summary
effectinternally in the registry module to replace throws/try-catch with typed, explicit failure paths.Resultfor fetch/resolve/install flows; CLI updated to handle and format structured errors.Notes
effectdependency to@better-slop/corevia workspace catalog.Summary by CodeRabbit
New Features
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.