Skip to content

fix(cli): survive unavailable chat log directory - #1186

Open
c8dhjp4tyv-bit wants to merge 2 commits into
CodebuffAI:mainfrom
c8dhjp4tyv-bit:recreate/1099-cli-logger-startup-resilience
Open

fix(cli): survive unavailable chat log directory#1186
c8dhjp4tyv-bit wants to merge 2 commits into
CodebuffAI:mainfrom
c8dhjp4tyv-bit:recreate/1099-cli-logger-startup-resilience

Conversation

@c8dhjp4tyv-bit

Copy link
Copy Markdown

Recreated on the rewritten main after #1099 was auto-closed during repository maintenance. This carries the same reviewed change set on the new history.

Summary

  • prevent logger initialization from crashing the CLI when the current chat directory cannot be created
  • keep file logging best-effort while preserving the existing development and production destinations
  • add regression coverage for both destinations and the unavailable-chat-directory path

Fixes #783

Validation

Prior validation before the history rewrite:

  • logger.test.ts: 3 passed
  • common typecheck: passed
  • SDK typecheck: passed
  • git diff --check: passed
  • full CLI test command: 2,111 passed; existing checkout/environment failures remain in unrelated Infisical, read-only config, and missing release-dependency tests

@codebuff-team

Copy link
Copy Markdown
Contributor

Good instinct and clean execution. Extracting resolveLogTarget as a pure, injectable function (cli/src/utils/logger.ts) makes the dev/production branching testable without touching the filesystem, and wrapping both directory resolution and setLogPath in try/catch in trySetLogPath correctly turns a hard crash into best-effort logging, which matches the intent of #783.

The three logger.test.ts cases cover the two normal destinations plus the failure path, and the assertions (currentChatDirCalls stays 0 in dev, target is undefined when getCurrentChatDir throws) are meaningful rather than tautological.

A couple of things worth double-checking before porting:

  • Confirm CHAT_LOG_FILENAME is already exported from logger.ts (not shown as changed in this diff) since the test imports it directly.
  • Swallowing the error silently in trySetLogPath means there's no signal at all (not even a stderr warning) that file logging failed — worth considering whether a single best-effort console.error on the caught error would help debugging without reintroducing the crash risk. Not blocking, but a reasonable follow-up.

Small, in-scope, focused change with regression coverage — good candidate for porting as-is.

@codebuff-team codebuff-team added bot:triaged Classified by the community triage bot pr:port-candidate Worth porting into the private source tree labels Sep 1, 2026

Copy link
Copy Markdown
Author

Confirmed CHAT_LOG_FILENAME is already exported (export const CHAT_LOG_FILENAME = 'log.jsonl'). I kept the startup fallback silent intentionally rather than adding console.error: stderr output during startup can corrupt the interactive terminal UI, while file logging is explicitly best-effort. Added that rationale to the catch-path comment. Logger regression tests: 3 passed, 0 failed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:triaged Classified by the community triage bot pr:port-candidate Worth porting into the private source tree

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Can't launch (ENOENT)

2 participants