fix: clarify invalid config JSON errors#118
Open
nightcityblade wants to merge 6 commits into
Open
Conversation
404-Page-Found
approved these changes
Jun 6, 2026
Contributor
404-Page-Found
left a comment
404-Page-Found
left a comment
There was a problem hiding this comment.
PR #118 Review: fix: clarify invalid config JSON errors
Overview
| Author | @nightcityblade |
| Base → Head | main ← fix/issue-102 |
| Size | +59/−1 across 2 files (1 commit) |
| Linked issue | #102 — "Improve invalid config JSON error message with file path" |
Scope & purpose: Wraps JSON.parse in loadConfig() with a try/catch that rethrows SyntaxError as a user-friendly error including the config file path and a fix hint (commit-echo init). Adds one regression test. Clean, focused, single-concern PR.
✅ What's good
-
Meets every acceptance criterion from #102:
- Catches
JSON.parsefailures and throws a user-friendly message - Includes the config file path in the error message
- Non-JSON errors (e.g., permission errors) are rethrown unchanged
- Regression test validates the full error message content
- Catches
-
Preserves the original error via
{ cause: error }— theSyntaxErroris still accessible in the error chain for debugging. -
Correct error discrimination —
instanceof SyntaxErrorguard ensures only JSON parse errors get wrapped; anything else rethrows cleanly. -
Test is thorough — validates three distinct parts of the error message (prefix, config path, hint), uses
escapeRegExpfor the dynamic path, and cleans up environment variables in afinallyblock. -
Prettier clean, builds successfully, all 83 tests pass (1 skipped — same as
mainminus streaming tests added after the branch point). No regressions. -
Commit message follows conventional commits (
fix: clarify invalid config JSON errors).
⚠️ Issues
Important — Test not hermetic on Windows
The test manipulates HOME and XDG_CONFIG_HOME to redirect getConfigDir() to a temp directory. This works on Linux/macOS, but on Windows the getConfigDir() function takes the win32 branch first and uses APPDATA, which the test never overrides:
// src/config/store.ts — getConfigDir()
if (os === 'win32') {
const appData = process.env['APPDATA'];
if (appData) return join(appData, 'commit-echo'); // ← always taken on Windows
}Impact: On Windows, the test writes "{ invalid json" to the real user's %APPDATA%/commit-echo/config.json instead of a temp directory. The finally block restores env vars but never restores the original file contents, so running npm test on Windows would silently corrupt the developer's config.
Suggestion: Override APPDATA in the test (or mock getConfigPath directly). Something like:
const originalAppData = process.env.APPDATA;
process.env.APPDATA = home;
// ... run test ...
process.env.APPDATA = originalAppData;Or, to avoid env var manipulation entirely, mock getConfigPath / getConfigDir to return the temp path directly. This would make the test platform-agnostic.
Note: CI runs on Linux, so this won't break CI — but it will break local npm test for any Windows contributor.
Nitpick — Minor observations
- Double-quoted strings in the test — The test uses double quotes (
"node:assert/strict") while the project convention (visible in.prettierrcwithsingleQuote: trueand in all other test files) is single quotes. This doesn't fail Prettier because Prettier is not configured for the.mjstest files, but it's a minor style inconsistency.
Verdict: Approve ✅
The PR is well-scoped, correct for its target platform (Linux/macOS CI), and cleanly implemented. The Windows test hermeticity issue is real but does not block merge — it's a pre-existing platform gap that affects any test relying on HOME/XDG_CONFIG_HOME on Windows. A follow-up issue to harden tests for Windows would be the appropriate place to address it.
The core change in src/config/store.ts is solid and ready to ship.
404-Page-Found
requested changes
Jun 6, 2026
Contributor
404-Page-Found
left a comment
404-Page-Found
left a comment
There was a problem hiding this comment.
PR #118 Review: fix: clarify invalid config JSON errors
Overview
| Author | @nightcityblade |
| Base → Head | main ← fix/issue-102 |
| Size | +59/−1 across 2 files (1 commit) |
| Linked issue | #102 — "Improve invalid config JSON error message with file path" |
Scope & purpose: Wraps JSON.parse in loadConfig() with a try/catch that rethrows SyntaxError as a user-friendly error including the config file path and a fix hint (commit-echo init). Adds one regression test. Clean, focused, single-concern PR.
✅ What's good
-
Meets every acceptance criterion from #102:
- Catches
JSON.parsefailures and throws a user-friendly message - Includes the config file path in the error message
- Non-JSON errors (e.g., permission errors) are rethrown unchanged
- Regression test validates the full error message content
- Catches
-
Preserves the original error via
{ cause: error }— theSyntaxErroris still accessible in the error chain for debugging. -
Correct error discrimination —
instanceof SyntaxErrorguard ensures only JSON parse errors get wrapped; anything else rethrows cleanly. -
Test is thorough — validates three distinct parts of the error message (prefix, config path, hint), uses
escapeRegExpfor the dynamic path, and cleans up environment variables in afinallyblock. -
Prettier clean, builds successfully, all 83 tests pass (1 skipped — same as
mainminus streaming tests added after the branch point). No regressions. -
Commit message follows conventional commits (
fix: clarify invalid config JSON errors).
🚫 Blocking — Test corrupts real config on Windows
The test manipulates HOME and XDG_CONFIG_HOME to redirect getConfigDir() to a temp directory. This works on Linux/macOS, but on Windows the getConfigDir() function takes the win32 branch first and uses APPDATA, which the test never overrides:
// src/config/store.ts — getConfigDir()
if (os === 'win32') {
const appData = process.env['APPDATA'];
if (appData) return join(appData, 'commit-echo'); // ← always taken on Windows
}Impact: On Windows, the test writes "{ invalid json" to the real user's %APPDATA%/commit-echo/config.json instead of a temp directory. The finally block restores env vars but never restores the original file contents, so running npm test on Windows would silently corrupt the developer's config.
Fix: Add APPDATA override to the env-var backup/restore block so the test is hermetic on all platforms:
const originalHome = process.env.HOME;
const originalXdgConfigHome = process.env.XDG_CONFIG_HOME;
const originalAppData = process.env.APPDATA;
const home = await mkdtemp(`${tmpdir()}/commit-echo-config-`);
process.env.HOME = home;
process.env.APPDATA = home; // ← needed on Windows
delete process.env.XDG_CONFIG_HOME;
// ... test body ...
} finally {
if (originalHome === undefined) {
delete process.env.HOME;
} else {
process.env.HOME = originalHome;
}
if (originalAppData === undefined) {
delete process.env.APPDATA;
} else {
process.env.APPDATA = originalAppData;
}
if (originalXdgConfigHome === undefined) {
delete process.env.XDG_CONFIG_HOME;
} else {
process.env.XDG_CONFIG_HOME = originalXdgConfigHome;
}
}CI runs on Linux so this won't break the pipeline, but it will silently destroy the local config of any Windows contributor who runs npm test.
Verdict: Request changes 🚫
The core loadConfig() change is solid and ready to ship. The test must be updated to also override APPDATA on Windows (or mock getConfigPath directly) to prevent corrupting the developer's real config file. Once that's addressed, this is good to merge.
* feat: add streaming support for commit suggestions * fix: add error handling to streaming path and clean up code quality issues - Wrap streaming for-await in try/catch with user-friendly error message - Remove redundant truncateDiff call in streaming branch (already done inside generateSuggestionsStream) - Consolidate duplicate parseSuggestions/truncateDiff imports into single line - Use const instead of let for never-reassigned model variable * feat: enhance streaming support for commit suggestions with incremental output - Introduced `--stream` option in README for real-time suggestion output. - Updated `generateSuggestionsStream` to yield meta events and text chunks. - Improved error handling and parsing for both OpenAI and Anthropic providers. - Added new utility functions for parsing SSE lines and handling state. - Included tests for streaming functionality and parsing logic. * feat: implement streaming support validation and enhance error handling - Added `assertStreamingSupported` function to check if the provider supports streaming. - Integrated streaming support validation in the `suggestCommand` function to provide user feedback for unsupported providers. - Updated tests to cover streaming functionality and validation for various providers. - Ensured truncation warnings are displayed correctly in both streaming and non-streaming modes. * feat: enhance streaming support with model event handling - Introduced `ProviderStreamChunk` type to encapsulate streaming events for text and model updates. - Updated `completeStream` methods in providers to yield `ProviderStreamChunk` instead of raw strings. - Enhanced `generateSuggestionsStream` to handle model events and update the model dynamically during streaming. - Refactored request body construction in providers to support streaming options. - Added tests to verify model event extraction from streaming responses and ensure correct handling in SSE parsing. * feat: enhance OpenAI streaming handling with [DONE] support and refactor tests - Added handling for the [DONE] signal in the OpenAI-compatible provider's streaming logic to gracefully terminate the stream. - Refactored tests to utilize a new `streamFromChunks` helper for improved readability and maintainability. - Introduced new tests to validate the handling of final buffer states and ensure correct parsing of streamed output. * feat: enhance streaming suggestions with auto-commit support - Updated README to include `--yes` option for non-interactive streaming that auto-commits the first suggestion. - Refactored `suggestCommand` to support global options for auto-commit. - Enhanced streaming logic to handle provider-specific streaming capabilities. - Added tests for streaming output with Anthropic provider and auto-commit functionality. * refactor: unify stream event naming and consolidate truncation warning Rename SuggestionStreamEvent kind 'chunk' to 'text' to align with ProviderStreamChunk at the provider layer. Consolidate the truncation warning into a single code path shared by streaming and non-streaming modes. * refactor: extract shared SSE reader and fix releaseLock safety - Add streamSseResponse() helper in sse.ts to eliminate duplicated reader-loop boilerplate across OpenAI and Anthropic providers - Guard reader.releaseLock() with a cancelled flag to prevent errors when reader.cancel() was already called - Replace parseAnthropicSseLines generator (boolean return type) with parseAnthropicSseLine line-at-a-time function and SSE_STREAM_END sentinel for cleaner control flow - Rename assertStreamingSupported to getStreamingProvider to better reflect that it creates and returns a provider * refactor: remove dead duplicate SSE_STREAM_END check in streamSseResponse When done is true, buffer is always empty (set on the line before the for-loop), so the trailing buffer-flush block was unreachable dead code. Simplify to break immediately after the for-loop when done, since the final line is already captured in lines when lines.pop() is skipped.
Co-authored-by: Peter7896 <265430982+Peter7896@users.noreply.github.com>
# Conflicts: # src/index.ts Co-authored-by: Peter7896 <265430982+Peter7896@users.noreply.github.com>
* feat: add interactive multi-repo batch mode (404-PF#6) Add a command that scans a directory for git repositories, identifies those with staged changes, and processes each one in sequence — generating suggestions, committing, and reporting results. - finds all git repos in ./projects - Processes repos with staged changes in sequence - Supports --yes for unattended batch operation (auto-commit first suggestion) - Supports --recursive to search subdirectories for nested repos - Prints clear summary of results per repo (succeeded/skipped/failed) - Skips repos without staged changes with a notice - 13 unit tests covering repo discovery, change detection, and diff retrieval Closes 404-PF#6 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * perf: hoist buildProfile call outside per-repo loop in batch command buildProfile(config.historySize) was called inside the per-repository loop, causing redundant file I/O (reading + parsing the full commit history file) once per repo. Move it before the loop so it executes once, and the cached profile is reused across all repos. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix: address CodeRabbit review findings on batch command - check if rootDir itself is a git repo before scanning children - bump getGitDiff maxBuffer to 100 MB for large monorepo diffs - wrap readdirSync with try/catch to skip unreadable directories - relax gitCommit regex to match detached HEAD output - add optional body prompt in interactive batch mode (consistent UX) - export gitCommit and add unit tests (message, body, empty commit) - add tests for rootDir-as-repo edge case Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix: relax overly strict git hash length assertion in batch test assert.equal(result.hash.length, 7) was brittle — Git abbreviated hashes can exceed 7 characters depending on repository size. Replace with a hex regex check and a minimum-length assertion (≥7), which matches Git's shortest-prefix behavior. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
Thanks — I addressed the Windows hermeticity issue on this branch. What changed:
Validation:
|
📝 WalkthroughWalkthroughThis PR adds streaming support for real-time commit suggestion generation and introduces a new batch command for processing multiple Git repositories with automated LLM-based commits. Streaming works with OpenAI-compatible and Anthropic providers via server-sent events, while batch discovers repos, detects changes, generates suggestions, and auto-commits or interactively confirms per repository. ChangesStreaming Support for Suggestions
Batch Repository Processing Command
Sequence Diagram(s)sequenceDiagram
participant App as suggest command
participant Provider as Streaming Provider
participant LLM as LLM API
App->>Provider: getStreamingProvider(config)
Provider-->>App: validated provider
App->>App: startSpinner
App->>Provider: provider.completeStream(params)
Provider->>LLM: POST /messages or /chat/completions with stream:true
loop SSE Event Stream
LLM-->>Provider: data: SSE line
Provider->>Provider: parseLine (OpenAI/Anthropic)
Provider-->>App: { kind: 'model', model: name }
Provider-->>App: { kind: 'text', text: chunk }
end
App->>App: accumulate text + parse suggestions
App->>App: displaySuggestions or skip for --stream
App->>User: interactive select or auto-commit
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 5
🤖 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 `@README.md`:
- Around line 190-195: The README incorrectly states that "commit-echo suggest
--stream --yes" will auto-commit; update the docs/examples to require the actual
commit flag by changing the example and wording to include "--commit" (i.e., use
"commit-echo suggest --stream --yes --commit") and clarify in the sentence that
suggest only performs a commit when the --commit flag is present; reference the
CLI command "suggest" and flags "--stream", "--yes", and "--commit" when making
the edit.
In `@src/commands/batch.ts`:
- Around line 295-324: The current try/catch around gitCommit(...) and
appendEntry(...) hides appendEntry failures as "Commit failed"; split the
operations so gitCommit(repoPath, first.message, first.body) is executed and
handled first (log success and push a success result using commitResult,
repoPath, repoName), then call appendEntry(...) in its own try/catch; if
appendEntry fails, log and record that the history append failed (not the
commit), and do not mark the commit itself as failed. Apply the same split logic
in the interactive commit block where gitCommit and appendEntry are currently
wrapped together.
- Around line 163-166: The current directory check uses existsSync(dir) then
statSync(dir).isDirectory(), but statSync can throw (e.g., permission denied)
even when existsSync returns true; wrap the statSync call in a try/catch (around
the block that calls statSync and checks isDirectory) so any thrown error is
caught, call outro(pc.red(`Directory not found: ${dir}`)) or a clearer message
including err.message, and return; specifically update the validation around
existsSync and statSync in the batch command to handle exceptions from statSync
and avoid crashing.
- Around line 64-74: The current try/catch around execSync('git diff --cached
--quiet', ...) and execSync('git diff --quiet', ...) treats any git error as
“changes exist”, which hides real failures; update the catch blocks in
src/commands/batch.ts to inspect the thrown error from execSync (the Error
object with a .status/exit code) and only set staged = true / unstaged = true
when the exit code indicates diffs were found (exit code 1); for any other error
(e.g., non-zero status other than 1 or missing repo/permission errors) rethrow
or propagate the original error so failures aren’t masked. Ensure you reference
the specific execSync calls and the staged/unstaged variables when making the
change.
In `@src/providers/index.ts`:
- Around line 62-70: In completeStream, validate that the resolved provider
actually supports streaming before invoking resolvedProvider.completeStream;
check whether resolvedProvider.completeStream is a function and if not, either
replace the injected provider with getStreamingProvider(configProvider) or
throw/return a clear CLI-friendly error. Update the logic in completeStream to
test the optional provider (and the result of getStreamingProvider) for a
completeStream method and only yield* from resolvedProvider.completeStream when
present, otherwise surface a descriptive error referring to completeStream and
getStreamingProvider so callers know a non-streaming Provider was supplied.
🪄 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: ba939be5-4f56-4e34-a244-8988e9142666
📒 Files selected for processing (20)
README.mdsrc/commands/batch.tssrc/commands/suggest.tssrc/config/store.tssrc/index.tssrc/llm/client.tssrc/providers/anthropic.tssrc/providers/index.tssrc/providers/openai-compatible.tssrc/providers/sse.tssrc/types.tstests/batch.test.mjstests/client-stream.test.mjstests/config-store.test.mjstests/e2e/suggest-smoke.test.mjstests/git-diff.test.mjstests/helpers/stream-from-chunks.mjstests/history-profile.test.mjstests/providers-index.test.mjstests/stream-sse.test.mjs
Comment on lines
+190
to
+195
| Use `--stream` to print LLM output incrementally instead of waiting behind a spinner. Supported for OpenAI-compatible and Anthropic providers; use non-streaming mode for Cohere. Pair with `--yes` for a non-interactive workflow that streams output and auto-commits the first suggestion. | ||
|
|
||
| ```bash | ||
| commit-echo suggest --stream | ||
| commit-echo suggest --stream --yes | ||
| ``` |
There was a problem hiding this comment.
commit-echo suggest --stream --yes does not auto-commit without --commit.
The new wording/examples say this combo auto-commits, but in the current CLI flow suggest only commits when --commit is set. Please update the text/example to avoid misleading users.
Suggested doc fix
-Use `--stream` to print LLM output incrementally instead of waiting behind a spinner. Supported for OpenAI-compatible and Anthropic providers; use non-streaming mode for Cohere. Pair with `--yes` for a non-interactive workflow that streams output and auto-commits the first suggestion.
+Use `--stream` to print LLM output incrementally instead of waiting behind a spinner. Supported for OpenAI-compatible and Anthropic providers; use non-streaming mode for Cohere. Pair with `--yes` for non-interactive selection, and add `--commit` if you also want auto-commit.
```bash
commit-echo suggest --stream
-commit-echo suggest --stream --yes
+commit-echo suggest --stream --yes --commit</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
Use `--stream` to print LLM output incrementally instead of waiting behind a spinner. Supported for OpenAI-compatible and Anthropic providers; use non-streaming mode for Cohere. Pair with `--yes` for non-interactive selection, and add `--commit` if you also want auto-commit.
🤖 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 `@README.md` around lines 190 - 195, The README incorrectly states that
"commit-echo suggest --stream --yes" will auto-commit; update the docs/examples
to require the actual commit flag by changing the example and wording to include
"--commit" (i.e., use "commit-echo suggest --stream --yes --commit") and clarify
in the sentence that suggest only performs a commit when the --commit flag is
present; reference the CLI command "suggest" and flags "--stream", "--yes", and
"--commit" when making the edit.
Comment on lines
+64
to
+74
| try { | ||
| execSync('git diff --cached --quiet', { cwd, stdio: 'pipe' }); | ||
| } catch { | ||
| staged = true; | ||
| } | ||
|
|
||
| try { | ||
| execSync('git diff --quiet', { cwd, stdio: 'pipe' }); | ||
| } catch { | ||
| unstaged = true; | ||
| } |
There was a problem hiding this comment.
Differentiate “has changes” from git command failures.
Line 66 and Line 72 treat any git error as “changes exist”. That masks real failures (e.g., invalid repo/permission errors) and can send the flow down the wrong path.
Suggested fix
export function gitHasChanges(cwd: string): { staged: boolean; unstaged: boolean } {
- let staged = false;
- let unstaged = false;
-
- try {
- execSync('git diff --cached --quiet', { cwd, stdio: 'pipe' });
- } catch {
- staged = true;
- }
-
- try {
- execSync('git diff --quiet', { cwd, stdio: 'pipe' });
- } catch {
- unstaged = true;
- }
+ const stagedRes = spawnSync('git', ['diff', '--cached', '--quiet'], {
+ cwd,
+ stdio: 'pipe',
+ shell: false,
+ });
+ if (stagedRes.status === null || stagedRes.status > 1) {
+ throw new Error((stagedRes.stderr || stagedRes.stdout || 'git diff --cached failed').toString().trim());
+ }
+
+ const unstagedRes = spawnSync('git', ['diff', '--quiet'], {
+ cwd,
+ stdio: 'pipe',
+ shell: false,
+ });
+ if (unstagedRes.status === null || unstagedRes.status > 1) {
+ throw new Error((unstagedRes.stderr || unstagedRes.stdout || 'git diff failed').toString().trim());
+ }
- return { staged, unstaged };
+ return {
+ staged: stagedRes.status === 1,
+ unstaged: unstagedRes.status === 1,
+ };
}🤖 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 `@src/commands/batch.ts` around lines 64 - 74, The current try/catch around
execSync('git diff --cached --quiet', ...) and execSync('git diff --quiet', ...)
treats any git error as “changes exist”, which hides real failures; update the
catch blocks in src/commands/batch.ts to inspect the thrown error from execSync
(the Error object with a .status/exit code) and only set staged = true /
unstaged = true when the exit code indicates diffs were found (exit code 1); for
any other error (e.g., non-zero status other than 1 or missing repo/permission
errors) rethrow or propagate the original error so failures aren’t masked.
Ensure you reference the specific execSync calls and the staged/unstaged
variables when making the change.
Comment on lines
+163
to
+166
| if (!existsSync(dir) || !statSync(dir).isDirectory()) { | ||
| outro(pc.red(`Directory not found: ${dir}`)); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Handle statSync errors when validating the target directory.
Line 163 can throw (e.g., permission-denied) even when existsSync(dir) is true, which will crash the command before your user-facing outro path runs.
Suggested fix
- if (!existsSync(dir) || !statSync(dir).isDirectory()) {
+ let isDirectory = false;
+ try {
+ isDirectory = existsSync(dir) && statSync(dir).isDirectory();
+ } catch {
+ isDirectory = false;
+ }
+ if (!isDirectory) {
outro(pc.red(`Directory not found: ${dir}`));
return;
}📝 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.
Suggested change
| if (!existsSync(dir) || !statSync(dir).isDirectory()) { | |
| outro(pc.red(`Directory not found: ${dir}`)); | |
| return; | |
| } | |
| let isDirectory = false; | |
| try { | |
| isDirectory = existsSync(dir) && statSync(dir).isDirectory(); | |
| } catch { | |
| isDirectory = false; | |
| } | |
| if (!isDirectory) { | |
| outro(pc.red(`Directory not found: ${dir}`)); | |
| return; | |
| } |
🤖 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 `@src/commands/batch.ts` around lines 163 - 166, The current directory check
uses existsSync(dir) then statSync(dir).isDirectory(), but statSync can throw
(e.g., permission denied) even when existsSync returns true; wrap the statSync
call in a try/catch (around the block that calls statSync and checks
isDirectory) so any thrown error is caught, call outro(pc.red(`Directory not
found: ${dir}`)) or a clearer message including err.message, and return;
specifically update the validation around existsSync and statSync in the batch
command to handle exceptions from statSync and avoid crashing.
Comment on lines
+295
to
+324
| try { | ||
| const commitResult = gitCommit(repoPath, first.message, first.body); | ||
| await appendEntry({ | ||
| timestamp: new Date().toISOString(), | ||
| message: first.body | ||
| ? `${first.message}\n\n${first.body}` | ||
| : first.message, | ||
| diff, | ||
| model: config.model, | ||
| provider: config.provider, | ||
| }); | ||
| console.log( | ||
| ` ${pc.green(`✓ ${pc.bold(commitResult.hash)} ${commitResult.summary}`)}`, | ||
| ); | ||
| results.push({ | ||
| repo: repoPath, | ||
| repoName, | ||
| status: 'success', | ||
| message: first.message, | ||
| }); | ||
| } catch (err) { | ||
| const msg = err instanceof Error ? err.message : String(err); | ||
| console.log(` ${pc.red(`✖ Commit failed: ${msg}`)}`); | ||
| results.push({ | ||
| repo: repoPath, | ||
| repoName, | ||
| status: 'failed', | ||
| message: msg, | ||
| }); | ||
| } |
There was a problem hiding this comment.
A history write failure is misreported as a commit failure.
In both commit paths, gitCommit(...) and appendEntry(...) share one try/catch. If commit succeeds but history append fails, the code logs “Commit failed” and records failed status even though the commit already exists.
Suggested fix pattern
- try {
- const commitResult = gitCommit(repoPath, first.message, first.body);
- await appendEntry({
+ try {
+ const commitResult = gitCommit(repoPath, first.message, first.body);
+ let historyError: string | undefined;
+ try {
+ await appendEntry({
timestamp: new Date().toISOString(),
message: first.body
? `${first.message}\n\n${first.body}`
: first.message,
diff,
model: config.model,
provider: config.provider,
- });
+ });
+ } catch (err) {
+ historyError = err instanceof Error ? err.message : String(err);
+ }
console.log(
` ${pc.green(`✓ ${pc.bold(commitResult.hash)} ${commitResult.summary}`)}`,
);
+ if (historyError) {
+ console.log(` ${pc.yellow(`⚠ Committed, but failed to write history: ${historyError}`)}`);
+ }
results.push({
repo: repoPath,
repoName,
status: 'success',
message: first.message,
});
} catch (err) {
const msg = err instanceof Error ? err.message : String(err);
console.log(` ${pc.red(`✖ Commit failed: ${msg}`)}`);
results.push({Apply the same split in the interactive commit block (Line 407-440).
Also applies to: 407-440
🤖 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 `@src/commands/batch.ts` around lines 295 - 324, The current try/catch around
gitCommit(...) and appendEntry(...) hides appendEntry failures as "Commit
failed"; split the operations so gitCommit(repoPath, first.message, first.body)
is executed and handled first (log success and push a success result using
commitResult, repoPath, repoName), then call appendEntry(...) in its own
try/catch; if appendEntry fails, log and record that the history append failed
(not the commit), and do not mark the commit itself as failed. Apply the same
split logic in the interactive commit block where gitCommit and appendEntry are
currently wrapped together.
Comment on lines
+62
to
+70
| export async function* completeStream( | ||
| configProvider: string, | ||
| baseUrlOverride: string | undefined, | ||
| params: Omit<ChatParams, 'baseUrl'>, | ||
| provider?: Provider, | ||
| ): AsyncIterable<ProviderStreamChunk> { | ||
| const resolvedProvider = provider ?? getStreamingProvider(configProvider); | ||
| const baseUrl = getBaseUrl(configProvider, baseUrlOverride); | ||
| yield* resolvedProvider.completeStream!({ ...params, baseUrl }); |
There was a problem hiding this comment.
Validate injected providers before calling completeStream.
At Line 68-70, the optional provider parameter can bypass getStreamingProvider(), but resolvedProvider.completeStream! is still invoked unconditionally. If a non-streaming provider is passed, this throws a raw runtime error instead of the intended CLI message.
Suggested fix
export async function* completeStream(
configProvider: string,
baseUrlOverride: string | undefined,
params: Omit<ChatParams, 'baseUrl'>,
provider?: Provider,
): AsyncIterable<ProviderStreamChunk> {
const resolvedProvider = provider ?? getStreamingProvider(configProvider);
+ if (!resolvedProvider.completeStream) {
+ throw new Error(
+ `Streaming is not supported for the '${configProvider}' provider. Use non-streaming mode.`,
+ );
+ }
const baseUrl = getBaseUrl(configProvider, baseUrlOverride);
- yield* resolvedProvider.completeStream!({ ...params, baseUrl });
+ yield* resolvedProvider.completeStream({ ...params, baseUrl });
}🤖 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 `@src/providers/index.ts` around lines 62 - 70, In completeStream, validate
that the resolved provider actually supports streaming before invoking
resolvedProvider.completeStream; check whether resolvedProvider.completeStream
is a function and if not, either replace the injected provider with
getStreamingProvider(configProvider) or throw/return a clear CLI-friendly error.
Update the logic in completeStream to test the optional provider (and the result
of getStreamingProvider) for a completeStream method and only yield* from
resolvedProvider.completeStream when present, otherwise surface a descriptive
error referring to completeStream and getStreamingProvider so callers know a
non-streaming Provider was supplied.
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
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #102
Summary
Tests
Note: npm run format:check currently reports pre-existing formatting warnings in unrelated files (src/commands/init.ts, src/commands/suggest.ts, src/git/hook.ts, src/llm/client.ts); changed TypeScript file passes Prettier.
Summary by CodeRabbit
New Features
--streamflag for real-time suggestion streaming with OpenAI and Anthropic providers--yes/--autoflags for automatic commits using the first suggestionBug Fixes
Documentation