cli refactoring and merge into app codebase#68
Merged
Conversation
Closed
|
To use Codex here, create an environment for this repo. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughConsolidates Bun and Node CLIs into apps/cli with platform adapters, yargs-based options parsing, a runCliCommand executor using a new core streaming API (csv/jsonl), refactors legacy entrypoints to delegate, and adds tests, docs, and CI/workflow updates. ChangesUnified CLI Architecture with Streaming Support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
apps/cli/src/tests/integration.cli-params.test.js (1)
16-18: 💤 Low valueConsider cleaning up temp files after tests.
The
tempFilehelper creates unique temporary files but doesn't clean them up. While the OS eventually clears/tmp, adding cleanup in anafterAllor per-testfinallyblock would be cleaner.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/tests/integration.cli-params.test.js` around lines 16 - 18, The tempFile helper (function tempFile) creates unique files but never removes them; modify tests to track created temp paths (e.g., push each path returned by tempFile into an array) and add cleanup that deletes those files after tests — either an afterAll hook that iterates the array and fs.unlink or fs.promises.unlink each file (catch ENOENT), or ensure per-test finally blocks remove the file(s); update imports to include fs if needed and reference tempFile and the test suite hooks when adding the cleanup logic.apps/cli/src/tests/integration.cli-output.test.js (1)
28-41: ⚡ Quick winClean up temp output files after the integration test.
The test creates a temp file but never removes it. Adding
finallycleanup keeps repeated runs/CI workers tidy.Suggested fix
test('integration: writes generated content to file and keeps stdout progress-only in test mode', async () => { const inputPath = path.join(repoRoot, 'cli', 'examples', 'company-literal.txt'); const outputPath = path.join(os.tmpdir(), `anywaydata-cli-test-${Date.now()}.csv`); - const result = runCli(['-i', inputPath, '-n', '2', '-f', 'csv', '-o', outputPath, '--testMode']); - - expect(result.status).toBe(0); - expect(result.stderr).toBe(''); - expect(result.stdout).toContain('> Processing Input File'); - expect(result.stdout).toContain('> Writing output to'); - expect(result.stdout).not.toContain('"Company","AnyWayData"'); - - const written = await fs.readFile(outputPath, 'utf8'); - expect(written).toContain('"Company"'); - expect(written).toContain('AnyWayData'); + try { + const result = runCli(['-i', inputPath, '-n', '2', '-f', 'csv', '-o', outputPath, '--testMode']); + + expect(result.status).toBe(0); + expect(result.stderr).toBe(''); + expect(result.stdout).toContain('> Processing Input File'); + expect(result.stdout).toContain('> Writing output to'); + expect(result.stdout).not.toContain('"Company"'); + expect(result.stdout).not.toContain('AnyWayData'); + + const written = await fs.readFile(outputPath, 'utf8'); + expect(written).toContain('"Company"'); + expect(written).toContain('AnyWayData'); + } finally { + await fs.unlink(outputPath).catch(() => {}); + } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/tests/integration.cli-output.test.js` around lines 28 - 41, The test creates a temporary output file (outputPath) but never removes it; update the test around the runCli call to ensure cleanup by adding a try...finally (or wrapping assertions) where after reading the written file you unlink the temp file (use await fs.unlink(outputPath) or fs.promises.unlink) in the finally block so the temp CSV is removed even if assertions fail; locate the integration test in apps/cli/src/tests/integration.cli-output.test.js around the outputPath, runCli and fs.readFile usage and add the finally cleanup referencing outputPath.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/cli/package.json`:
- Around line 1-24: The CI is failing because package-lock.json is missing the
new package entry for "@anywaydata/cli-next" declared in package.json; fix it by
running npm install (or npm install --package-lock-only) at the repository root
to regenerate package-lock.json so it includes the new package, verify the lock
now contains an entry for "@anywaydata/cli-next" and then commit and push the
updated package-lock.json so npm ci in CI will succeed.
In `@apps/cli/src/platform/bun-platform.js`:
- Around line 12-22: The current createLineWriter implementation buffers the
entire file in memory (buffer in createLineWriter, writeLine and close) instead
of streaming; replace it to use Bun.file(path).writer() (or
bunRef.file(path).writer()) to obtain a streaming writer in createLineWriter,
call writer.write for each writeLine (appending "\n"), and call await
writer.close() in close(); also add error handling on write and close to surface
and log errors (mirror Node's createWriteStream error handling), ensure
writeLine returns/awaits the writer.write promise so backpressure/errors
propagate.
In `@apps/cli/src/tests/integration.cli-output.test.js`:
- Line 36: The current assertion only forbids one exact CSV string; update the
assertion that references result.stdout (the expect(result.stdout) call) to
reject any CSV-like output instead of a single combined string—replace the exact
not-toContain check with a broader check (e.g., use a negative regex match via
not.toMatch or test for absence of the CSV header token like "Company" or any
quoted comma-separated cells) so leaked CSV fragments (headers or quoted fields)
will fail the test.
- Around line 6-7: Replace the process.cwd() relative resolution with a
deterministic __dirname-based path: compute repoRoot from the test file's
directory (use path.resolve(__dirname, '../../../../') from the tests folder) so
repoRoot is independent of the current working directory, then keep building
cliEntry with path.join(repoRoot, 'apps','cli','src','node-entry.js'); update
the repoRoot declaration (and any dependent references) accordingly to ensure
tests resolve the CLI entry reliably.
In `@packages/core/src/index.js`:
- Around line 335-345: The code is collecting every generated row into the rows
array (rows, generator.generateRow(), safeRowCount) which defeats streaming;
modify the logic so that rows are only accumulated when explicitly requested
(e.g., add options.collectRows boolean or options.sampleRows number) — when
outputFormat === 'csv' or other streaming modes, avoid pushing each row into
rows unless options.collectRows is true (or instead collect only a limited
sample like first/last N when options.sampleRows is provided); update the
generator loop to conditionally push into rows based on that flag/sample and
ensure the returned object omits or returns an empty/undefined rows field when
not collecting.
- Around line 343-351: Wrap the entire streaming block that calls
streamFromTextSpec(...) and uses writer.writeLine / writer.close in a try/catch
(and a finally) so runtime I/O errors are caught and reported instead of
crashing: surround the await streamFromTextSpec(...) and subsequent streaming
loop (including calls to writer.writeLine via onChunk) with try { ... } catch
(err) { console.error("Failed to stream output:", err); process.exit(1); } and
place writer.close() in a finally (or catch its own errors) to ensure cleanup
and report any close errors; reference the streamFromTextSpec call, the
writer.writeLine usages indirectly invoked by onChunk/rowToCsv/rowToJsonLine,
and writer.close so you locate and protect the whole streaming sequence.
---
Nitpick comments:
In `@apps/cli/src/tests/integration.cli-output.test.js`:
- Around line 28-41: The test creates a temporary output file (outputPath) but
never removes it; update the test around the runCli call to ensure cleanup by
adding a try...finally (or wrapping assertions) where after reading the written
file you unlink the temp file (use await fs.unlink(outputPath) or
fs.promises.unlink) in the finally block so the temp CSV is removed even if
assertions fail; locate the integration test in
apps/cli/src/tests/integration.cli-output.test.js around the outputPath, runCli
and fs.readFile usage and add the finally cleanup referencing outputPath.
In `@apps/cli/src/tests/integration.cli-params.test.js`:
- Around line 16-18: The tempFile helper (function tempFile) creates unique
files but never removes them; modify tests to track created temp paths (e.g.,
push each path returned by tempFile into an array) and add cleanup that deletes
those files after tests — either an afterAll hook that iterates the array and
fs.unlink or fs.promises.unlink each file (catch ENOENT), or ensure per-test
finally blocks remove the file(s); update imports to include fs if needed and
reference tempFile and the test suite hooks when adding the cleanup logic.
🪄 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 Plus
Run ID: 622731c5-0cfb-4750-b9cd-a666d822c51a
📒 Files selected for processing (18)
.gitignoreapps/cli/jest.config.cjsapps/cli/package.jsonapps/cli/src/bun-entry.jsapps/cli/src/cli-options.jsapps/cli/src/index.jsapps/cli/src/node-entry.jsapps/cli/src/platform/bun-platform.jsapps/cli/src/platform/node-platform.jsapps/cli/src/run-cli.jsapps/cli/src/tests/cli-options.test.jsapps/cli/src/tests/integration.cli-output.test.jsapps/cli/src/tests/integration.cli-params.test.jsapps/cli/src/tests/run-cli.test.jscli/src/command.tspackages/cli/src/index.jspackages/core/src/index.jspackages/core/src/tests/core-api/streamFromTextSpec.test.js
Owner
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/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: 38b76ddfa9
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/codex-pr-review.yml:
- Around line 23-27: The workflow uses mutable action tags — specifically the
`uses: actions/checkout@v4` and `uses: openai/codex-action@v1` entries — which
must be pinned to immutable commit SHAs; update each `uses:` entry to replace
the tag (e.g., `@v4`, `@v1`) with the corresponding 40-character commit SHA for
that action, verifying the correct repository and commit on GitHub, and ensure
no other `uses:` lines remain non-SHA pinned (run the provided grep/python check
if needed).
- Around line 19-29: The workflow step "Run Codex PR review" (uses:
openai/codex-action@v1, input openai-api-key) lacks a fork guard so the step
fails when secrets.OPENAI_API_KEY is unavailable for forked PRs; update the
job/step condition to skip runs for forked repositories (e.g., require
github.event.pull_request.head.repo.fork == false) or otherwise check that the
secret is present before invoking the action so the Codex step only runs on
non-fork PRs.
🪄 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 Plus
Run ID: b47ac22e-8095-46e7-b969-00f5cacb234b
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
.github/workflows/codex-pr-review.ymlREADME.mdapps/cli/package.jsonapps/cli/src/platform/bun-platform.jsapps/cli/src/run-cli.jsapps/cli/src/tests/integration.cli-output.test.jsapps/cli/src/tests/integration.cli-params.test.jsapps/cli/src/tests/run-cli.test.jspackages/cli/CHANGELOG.mdpackages/cli/src/index.jspackages/core/src/index.js
💤 Files with no reviewable changes (2)
- packages/cli/CHANGELOG.md
- packages/cli/src/index.js
✅ Files skipped from review due to trivial changes (2)
- README.md
- apps/cli/src/run-cli.js
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/cli/src/platform/bun-platform.js
- apps/cli/src/tests/run-cli.test.js
- apps/cli/src/tests/integration.cli-output.test.js
- apps/cli/src/tests/integration.cli-params.test.js
- packages/core/src/index.js
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs-src/docs/070-interfaces-and-deployment/050-cli-node-and-bun.md`:
- Line 45: Update the docs text for the `--show-progress` CLI option to clarify
the exact command-line syntax users should use; in the `--show-progress`
description replace the current "(true or false)" with a brief example showing
the accepted form(s) (e.g., `--show-progress true` and/or
`--show-progress=true`) or explicitly state which one is supported by the CLI,
and add the example into the examples section if present so users see usage in
context.
🪄 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 Plus
Run ID: e231143c-c67a-4d05-a890-c883f766fd49
⛔ Files ignored due to path filters (1)
cli/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
README.mdapps/cli/examples/company-literal.txtapps/cli/examples/company.txtapps/cli/examples/error-extra-line.txtapps/cli/examples/faker-and-regex.txtapps/cli/examples/regex-field.txtapps/cli/examples/test.txtcli/.gitignorecli/index.tscli/package.jsoncli/readme.mdcli/src/command.tscli/src/outputData.jscli/tsconfig.jsondocs-src/docs/070-interfaces-and-deployment/050-cli-node-and-bun.md
💤 Files with no reviewable changes (7)
- cli/tsconfig.json
- cli/readme.md
- cli/.gitignore
- cli/index.ts
- cli/src/outputData.js
- cli/package.json
- cli/src/command.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/codex-pr-review.yml:
- Around line 11-17: The concurrency group currently references
github.event.pull_request.number and the codex-review job-level if uses
github.event.pull_request and secrets (which are invalid for workflow_dispatch
and job ifs); update the concurrency group to use a run-scoped value like
github.run_id (replace group: codex-pr-review-${{
github.event.pull_request.number }} with group: codex-pr-review-${{
github.run_id }}) and remove the job-level if on the codex-review job (delete
the if: github.event.pull_request.draft == false && secrets.OPENAI_API_KEY != ''
line) so the workflow can run for workflow_dispatch and rely on the existing
step-level secrets.OPENAI_API_KEY check instead.
🪄 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 Plus
Run ID: 6f81cd93-5bc6-4885-bbea-4ed25dde9536
📒 Files selected for processing (1)
.github/workflows/codex-pr-review.yml
This was referenced May 9, 2026
Merged
Merged
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
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.
closes #60
@codex
Summary by CodeRabbit
New Features
Tests
Documentation
Chores