Conversation
- replace bun lockfile with package-lock.json - add npm build, test, and pack smoke checks - update docs and ci for node 24/npm
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 2 minutes and 3 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe pull request migrates the Hydra project from Bun to Node.js 24 as the runtime, switching the build toolchain to npm/tsx/vitest, replacing Bun's built-in SQLite with better-sqlite3, updating all ES module imports to use explicit Changes
Sequence Diagram(s)sequenceDiagram
participant Client as HTTP Client
participant Server as Node HTTP Server
participant Handler as Request Handler
participant Pipeline as HydraPipeline
participant DB as better-sqlite3
Client->>Server: POST /api/run (with query)
Server->>Server: Convert IncomingMessage → Request
Server->>Handler: handleWebRequest(req, sessionToken)
Handler->>Handler: Check authorization
Handler->>Handler: Parse route & method
alt POST /api/run
Handler->>Pipeline: Create HydraPipeline
Handler->>Pipeline: pipeline.run(query)
Pipeline->>DB: Query initialization
DB-->>Pipeline: Run record created
Pipeline->>Handler: Emit run-created event
Handler->>Client: Send SSE stream opened
Pipeline->>Pipeline: Execute phases
Pipeline->>DB: Update run status
Pipeline->>Handler: Emit pipeline events
Handler->>Client: Write SSE events
Pipeline-->>Handler: Complete or fail
Handler->>Client: Close SSE stream
else GET /api/runs
Handler->>DB: listRuns()
DB-->>Handler: Return run records
Handler->>Client: JSON response
end
Server->>Server: Convert Response → ServerResponse
Client->>Client: Receive result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Incremental Review (commit
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
src/opentui-core.d.ts (1)
1-45: Excessive use ofunknownundermines type safety.This declaration file uses
unknownthroughout, which provides minimal compile-time validation. Based on actual usage insrc/ui/tui.ts, more specific types would catch errors earlier:
StyledText = unknowncould be an opaque branded typecreateCliRendererconfig uses known keys (exitOnCtrlC,useAlternateScreen)add(child: unknown)should constrain to renderables with anidproperty- Styling helpers should accept
string | number | StyledTextrather thanunknown♻️ Suggested type improvements
declare module "@opentui/core" { - export type StyledText = unknown; + // Opaque type for styled text - prevents accidental misuse + export type StyledText = { readonly __brand: unique symbol }; + + export interface CliRendererConfig { + exitOnCtrlC?: boolean; + useAlternateScreen?: boolean; + } + + export interface Renderable { + id: string; + } export class CliRenderer { width: number; root: { - add(child: unknown): void; + add(child: Renderable): void; }; requestLive(): void; dropLive(): void; destroy(): void; } export function createCliRenderer( - config?: Record<string, unknown>, + config?: CliRendererConfig, ): Promise<CliRenderer>; - export class BoxRenderable { + export class BoxRenderable implements Renderable { id: string; - constructor(renderer: CliRenderer, options?: Record<string, unknown>); - add(child: unknown): void; + constructor(renderer: CliRenderer, options?: BoxRenderableOptions); + add(child: Renderable): void; getChildren(): Array<{ id: string }>; remove(id: string): void; } - export class TextRenderable { + export class TextRenderable implements Renderable { id: string; text: StyledText; content: StyledText; - constructor( - renderer: CliRenderer, - options?: Record<string, unknown> & { text?: StyledText }, - ); + constructor(renderer: CliRenderer, options?: TextRenderableOptions); } - export function bold(input: unknown): StyledText; - export function brightBlack(input: unknown): StyledText; - export function green(input: unknown): StyledText; - export function magenta(input: unknown): StyledText; - export function yellow(input: unknown): StyledText; + type Styleable = string | number | StyledText; + export function bold(input: Styleable): StyledText; + export function brightBlack(input: Styleable): StyledText; + export function green(input: Styleable): StyledText; + export function magenta(input: Styleable): StyledText; + export function yellow(input: Styleable): StyledText; export function t( strings: TemplateStringsArray, - ...values: unknown[] + ...values: Styleable[] ): StyledText; }Define
BoxRenderableOptionswith:border?,borderStyle?,flexDirection?,gap?,padding?,width?,height?,flexGrow?properties.
DefineTextRenderableOptionswith:content?property.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/opentui-core.d.ts` around lines 1 - 45, The declaration overuses unknown; replace it with concrete types: define an opaque branded type StyledText (e.g., type StyledText = string & { __styled?: true }) and update all functions to return/accept StyledText; tighten createCliRenderer config to an interface with known keys (exitOnCtrlC?: boolean, useAlternateScreen?: boolean, any other known flags); change add(child: unknown) and BoxRenderable.getChildren()/remove signatures to use a Renderable or HasId interface (e.g., { id: string }) and add a BoxRenderableOptions type (border?, borderStyle?, flexDirection?, gap?, padding?, width?, height?, flexGrow?); add TextRenderableOptions with content?: StyledText and update TextRenderable constructor to use it; and change styling helpers (bold, brightBlack, green, magenta, yellow, t) to accept string | number | StyledText instead of unknown so callers get proper type checking.src/engine/pipeline.ts (1)
863-869: Avoid dropping all assignments when only one item is malformed.Line 863 currently returns an empty set if any parsed assignment is invalid. That makes decomposition brittle to partial LLM noise and discards otherwise valid assignments.
Suggested resilience tweak
- if (assignments.some((item) => item === null)) { - return []; - } - - return assignments.filter( + return assignments.filter( (assignment): assignment is DecomposedAssignment => assignment !== null, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/engine/pipeline.ts` around lines 863 - 869, The current code bails out and returns [] if any entry in assignments is null, which drops all valid decomposed results; remove the early if (assignments.some((item) => item === null)) { return []; } check and instead simply return the filtered list (assignments.filter((assignment): assignment is DecomposedAssignment => assignment !== null)), optionally emitting a warning/log for any discarded nulls so callers still get all valid DecomposedAssignment entries.package.json (1)
52-54: Verify Node 24 availability and consider documenting the version requirement rationale.The
enginesfield requires Node>=24. While Node 24 should be available by this timeframe, it's a relatively new major version. Consider documenting in the README why Node 24 is required (e.g., specific ESM features, native fetch, etc.) to help users understand the constraint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 52 - 54, The package.json currently enforces "engines": { "node": ">=24" } which may be surprising to users; update project docs to justify this requirement by adding a short note in README (or a new docs/ note) explaining why Node 24 is required (e.g., usage of ESM-only features, native fetch, new timers/promises APIs, or specific language/runtime behavior) and, if appropriate, include a fallback or compatibility notes for older Node versions; reference the "engines" field and the "node" constraint when adding the rationale so readers can connect the requirement to concrete features in the codebase.README.md (1)
44-50: Minor: Consider using ESM syntax in the example for consistency.The inline
node -ecommand usesrequire('node:fs')(CommonJS), while the project is ESM ("type": "module"). While this works because-edefaults to CommonJS evaluation, using ESM would be more consistent.📖 Alternative ESM-based example
-node -e "console.log(JSON.parse(require('node:fs').readFileSync('./examples/ai-transition-2036.json', 'utf8')).brief)" +node --input-type=module -e "import {readFileSync} from 'node:fs'; console.log(JSON.parse(readFileSync('./examples/ai-transition-2036.json', 'utf8')).brief)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 44 - 50, The README example currently uses a CommonJS inline command (the `node -e` invocation that calls require('node:fs')), so update that example to use ESM semantics instead: change the inline command to run Node in module mode and use an ESM import of 'fs' with top-level await or fs.promises to read and parse ./examples/ai-transition-2036.json and then print the .brief field. Locate the example block in README.md (the `node -e` / require('node:fs') snippet) and replace it with an ESM-based one-liner that imports fs and logs the parsed brief.src/db/queries.ts (1)
502-508: Consider capturingDate.now()once for consistency.The
completedAtandelapsedMscalculations use separateDate.now()calls, introducing a minor drift. While negligible in practice, a single timestamp would be more precise.♻️ Optional fix for timestamp consistency
export function markRunComplete(runId: string, brief: string): RunRecord { const run = getRun(runId); if (!run) { throw new Error(`Run ${runId} not found`); } + const now = Date.now(); return updateRunStatus(runId, { status: "complete", brief, - completedAt: Date.now(), - elapsedMs: Date.now() - run.createdAt, + completedAt: now, + elapsedMs: now - run.createdAt, error: null, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/db/queries.ts` around lines 502 - 508, The code calls Date.now() twice when setting completedAt and computing elapsedMs in the updateRunStatus call; capture a single timestamp into a local variable (e.g., const completedAt = Date.now()) before calling updateRunStatus and then pass completedAt and elapsedMs: completedAt - run.createdAt so both fields are consistent; modify the block around updateRunStatus(runId, { status: "complete", brief, ... }) to use that single timestamp variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Around line 1-59: The repo's package-lock.json is out of sync with
package.json; run npm install locally to regenerate/update package-lock.json to
match the declared dependencies (e.g., those under "dependencies" and
"devDependencies" in package.json), verify the change, and commit the updated
package-lock.json so CI can perform a clean npm ci. Ensure you do not modify
package.json when updating the lockfile and include only the updated
package-lock.json in the commit.
In `@src/cli.smoke.test.ts`:
- Around line 11-21: The two spawnSync invocations that create build and result
(calling npmCommand() for "npm run build" and process.execPath for
"dist/index.js --help") need a timeout option to avoid hung CI; update both
spawnSync calls to include a timeout (e.g., timeout: 120000) in their options
object so the child process is killed after the specified milliseconds and the
test fails fast if the command stalls.
In `@src/config.ts`:
- Around line 267-285: In writeConfig, validation happens after the file is
already written; call normalizeConfig (merging DEFAULTS, readConfigFile(), and
updates) first to validate and obtain the normalized config, throw/propagate any
validation errors, and only then serialize and write that normalized result to
disk (using writeFileSync and chmodSync on CONFIG_FILE); ensure the returned
value is the normalized config (not a re-normalization of fileData) and keep
ensureConfigDir/readConfigFile usage but validate before persisting.
In `@src/web/server.ts`:
- Around line 986-992: Remove the deprecated "aborted" event listener: delete
the req.once("aborted", () => abortController.abort()); line and rely on the
existing req.once("close", () => { if (!req.complete) { abortController.abort();
} }); logic; keep the AbortController instantiation (const abortController = new
AbortController();) and the "close" handler as-is so cancellation behavior
remains unchanged.
---
Nitpick comments:
In `@package.json`:
- Around line 52-54: The package.json currently enforces "engines": { "node":
">=24" } which may be surprising to users; update project docs to justify this
requirement by adding a short note in README (or a new docs/ note) explaining
why Node 24 is required (e.g., usage of ESM-only features, native fetch, new
timers/promises APIs, or specific language/runtime behavior) and, if
appropriate, include a fallback or compatibility notes for older Node versions;
reference the "engines" field and the "node" constraint when adding the
rationale so readers can connect the requirement to concrete features in the
codebase.
In `@README.md`:
- Around line 44-50: The README example currently uses a CommonJS inline command
(the `node -e` invocation that calls require('node:fs')), so update that example
to use ESM semantics instead: change the inline command to run Node in module
mode and use an ESM import of 'fs' with top-level await or fs.promises to read
and parse ./examples/ai-transition-2036.json and then print the .brief field.
Locate the example block in README.md (the `node -e` / require('node:fs')
snippet) and replace it with an ESM-based one-liner that imports fs and logs the
parsed brief.
In `@src/db/queries.ts`:
- Around line 502-508: The code calls Date.now() twice when setting completedAt
and computing elapsedMs in the updateRunStatus call; capture a single timestamp
into a local variable (e.g., const completedAt = Date.now()) before calling
updateRunStatus and then pass completedAt and elapsedMs: completedAt -
run.createdAt so both fields are consistent; modify the block around
updateRunStatus(runId, { status: "complete", brief, ... }) to use that single
timestamp variable.
In `@src/engine/pipeline.ts`:
- Around line 863-869: The current code bails out and returns [] if any entry in
assignments is null, which drops all valid decomposed results; remove the early
if (assignments.some((item) => item === null)) { return []; } check and instead
simply return the filtered list (assignments.filter((assignment): assignment is
DecomposedAssignment => assignment !== null)), optionally emitting a warning/log
for any discarded nulls so callers still get all valid DecomposedAssignment
entries.
In `@src/opentui-core.d.ts`:
- Around line 1-45: The declaration overuses unknown; replace it with concrete
types: define an opaque branded type StyledText (e.g., type StyledText = string
& { __styled?: true }) and update all functions to return/accept StyledText;
tighten createCliRenderer config to an interface with known keys (exitOnCtrlC?:
boolean, useAlternateScreen?: boolean, any other known flags); change add(child:
unknown) and BoxRenderable.getChildren()/remove signatures to use a Renderable
or HasId interface (e.g., { id: string }) and add a BoxRenderableOptions type
(border?, borderStyle?, flexDirection?, gap?, padding?, width?, height?,
flexGrow?); add TextRenderableOptions with content?: StyledText and update
TextRenderable constructor to use it; and change styling helpers (bold,
brightBlack, green, magenta, yellow, t) to accept string | number | StyledText
instead of unknown so callers get proper type checking.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d2ce7df1-b7e5-47f5-8b5e-e166ecd9b11a
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (36)
.github/workflows/ci.ymlREADME.mdpackage.jsonscripts/copy-assets.mjssrc/cli.smoke.test.tssrc/config.test.tssrc/config.tssrc/db/client.tssrc/db/queries.test.tssrc/db/queries.tssrc/engine/concurrency.test.tssrc/engine/concurrency.tssrc/engine/eta.tssrc/engine/model.test.tssrc/engine/model.tssrc/engine/personas.error.test.tssrc/engine/personas.test.tssrc/engine/personas.tssrc/engine/pipeline.test.tssrc/engine/pipeline.tssrc/engine/prompts.tssrc/engine/search.tssrc/index.test.tssrc/index.tssrc/opentui-core.d.tssrc/security.test.tssrc/security.tssrc/types.tssrc/ui/agent-mode.tssrc/ui/animations.test.tssrc/ui/tui.tssrc/web/index.tssrc/web/server.tstsconfig.build.jsontsconfig.jsonvitest.config.ts
Comment on lines
1
to
59
| { | ||
| "name": "hydra-swarm", | ||
| "version": "0.1.0", | ||
| "description": "Multi-model swarm intelligence engine for your terminal", | ||
| "repository": { | ||
| "type": "git", | ||
| "url": "https://github.com/baanish/hydra-cli.git" | ||
| }, | ||
| "type": "module", | ||
| "bin": { | ||
| "hydra": "./src/index.ts" | ||
| }, | ||
| "scripts": { | ||
| "dev": "bun run src/index.ts", | ||
| "start": "bun run src/index.ts", | ||
| "test": "bun test", | ||
| "build": "bun build src/index.ts --compile --outfile dist/hydra", | ||
| "typecheck": "tsc --noEmit", | ||
| "lint": "biome check src/", | ||
| "format": "biome format --write src/" | ||
| }, | ||
| "dependencies": { | ||
| "@opentui/core": "^0.1.81", | ||
| "commander": "^13.1.0", | ||
| "nanoid": "^5.1.5", | ||
| "openai": "^4.83.0", | ||
| "unicode-animations": "^1.0.3" | ||
| }, | ||
| "devDependencies": { | ||
| "@biomejs/biome": "^1.9.4", | ||
| "@types/bun": "latest", | ||
| "typescript": "^5.7.3" | ||
| }, | ||
| "keywords": [ | ||
| "cli", | ||
| "ai", | ||
| "swarm", | ||
| "multi-agent", | ||
| "research", | ||
| "intelligence", | ||
| "llm", | ||
| "tui", | ||
| "opentui" | ||
| ], | ||
| "author": "Aanish Bhirud", | ||
| "license": "MIT", | ||
| "engines": { | ||
| "bun": ">=1.1.0" | ||
| } | ||
| "name": "@baanish/hydra-cli", | ||
| "version": "0.1.0", | ||
| "description": "Multi-model swarm intelligence engine for your terminal", | ||
| "repository": { | ||
| "type": "git", | ||
| "url": "https://github.com/baanish/hydra-cli.git" | ||
| }, | ||
| "type": "module", | ||
| "bin": { | ||
| "hydra": "./dist/index.js" | ||
| }, | ||
| "scripts": { | ||
| "dev": "tsx src/index.ts", | ||
| "start": "tsx src/index.ts", | ||
| "test": "vitest run", | ||
| "build": "tsc -p tsconfig.build.json && node scripts/copy-assets.mjs", | ||
| "typecheck": "tsc --noEmit", | ||
| "lint": "biome check src scripts README.md package.json tsconfig.json tsconfig.build.json vitest.config.ts .github", | ||
| "format": "biome format --write src scripts README.md package.json tsconfig.json tsconfig.build.json vitest.config.ts .github", | ||
| "prepack": "npm run build" | ||
| }, | ||
| "dependencies": { | ||
| "@opentui/core": "^0.1.81", | ||
| "better-sqlite3": "^12.8.0", | ||
| "commander": "^13.1.0", | ||
| "nanoid": "^5.1.5", | ||
| "openai": "^4.83.0", | ||
| "unicode-animations": "^1.0.3" | ||
| }, | ||
| "devDependencies": { | ||
| "@biomejs/biome": "^1.9.4", | ||
| "@types/better-sqlite3": "^7.6.13", | ||
| "@types/node": "^24.5.2", | ||
| "tsx": "^4.20.5", | ||
| "typescript": "^5.7.3", | ||
| "vitest": "^3.2.4" | ||
| }, | ||
| "keywords": [ | ||
| "cli", | ||
| "ai", | ||
| "swarm", | ||
| "multi-agent", | ||
| "research", | ||
| "intelligence", | ||
| "llm", | ||
| "tui", | ||
| "opentui" | ||
| ], | ||
| "author": "Aanish Bhirud", | ||
| "license": "MIT", | ||
| "engines": { | ||
| "node": ">=24" | ||
| }, | ||
| "files": ["dist", "README.md", "LICENSE"], | ||
| "publishConfig": { | ||
| "access": "public" | ||
| } | ||
| } |
There was a problem hiding this comment.
Pipeline failure: package-lock.json is out of sync with package.json.
The CI pipeline reports:
npm ci cannot perform a clean install when package.json and lock file are not in sync. Please update package-lock.json by running 'npm install' before continuing.
Run npm install locally and commit the updated package-lock.json.
🧰 Tools
🪛 GitHub Actions: ci
[error] 1-1: npm ci cannot perform a clean install when package.json and lock file are not in sync. Please update package-lock.json by running 'npm install' before continuing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` around lines 1 - 59, The repo's package-lock.json is out of
sync with package.json; run npm install locally to regenerate/update
package-lock.json to match the declared dependencies (e.g., those under
"dependencies" and "devDependencies" in package.json), verify the change, and
commit the updated package-lock.json so CI can perform a clean npm ci. Ensure
you do not modify package.json when updating the lockfile and include only the
updated package-lock.json in the commit.
- document node-24-first support in the readme - update cli and test code for esm and web platform primitives - refresh lockfile after dependency changes
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
1 participant
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.
Summary
package-lock.json, npm scripts, and a node 24+ baseline for local dev and CI.dist/output and omits source-only artifacts.Testing
npm cinpm run lintnpm run typechecknpm testnpm run buildnpm packsmoke check forpackage/dist/index.jsandpackage/dist/web/app.htmlSummary by CodeRabbit
Release Notes
New Features
Documentation
~/.config/hydra-cli)Chores
@baanish/hydra-cli