fix: graceful error handling for /undo (issue #17) by wjiuxing · Pull Request #19 · clopca/open-mem

wjiuxing · 2026-05-14T12:13:23Z

Summary

Add configurable Logger utility that respects config.logLevel (default: warn)
Replace all 18 raw console.error("[open-mem] ...") calls in hooks with level-gated logger methods
Classify hook errors as warn (operational) or debug (cosmetic) — at default settings, most noise after /undo disappears
Add PendingMessageRepository.deleteBySessionId() for future undo cleanup integration

Problem

After /undo in OpenCode, error logs appear from open-mem because hooks have no undo awareness and use raw console.error for all errors. The existing config.logLevel field was defined but never enforced.

Changes

File	Change
`src/utils/logger.ts`	New — Logger with debug/info/warn/error gated by level
`src/db/pending.ts`	Add `deleteBySessionId()` for orphan cleanup
`src/hooks/tool-capture.ts`	`console.error` → `logger.warn`
`src/hooks/chat-capture.ts`	`console.error` → `logger.warn`
`src/hooks/session-events.ts`	6× `console.error` → `logger.warn`/`logger.debug`
`src/hooks/context-inject.ts`	`console.error` → `logger.warn`
`src/hooks/compaction.ts`	`console.error` → `logger.warn`
`src/index.ts`	Create Logger singleton, wire to all hooks
`src/adapters/platform/runtime.ts`	Pass logger to PlatformIngestionRuntime
`src/platform-worker.ts`	Pass Logger to PlatformIngestionRuntime

Verification

✅ TypeScript: tsc --noEmit clean
✅ Tests: 964 pass, 0 fail across 80 files
✅ Build: bun run build clean
✅ Lint: 0 new errors

Closes #17

Summary by CodeRabbit

New Features
- Configurable logging (debug/info/warn/error) threaded into hooks, runtimes, and workers.
- Observations can include an optional message ID with soft-delete-by-message-id support.
- Cleanup operations to remove pending/failed messages by session or call ID.
Chores
- Replaced console.* calls with structured logger usage across lifecycle handlers and hooks.
- Added DB migration to support message IDs.
Tests
- Added logger tests, pending-message cleanup tests, message-removed tests, and updated hook tests to use the logger.

Replace raw console.error in all hooks with a configurable Logger that respects config.logLevel (default: warn). Hook errors are classified as warn (operational) or debug (cosmetic), so at default settings most noise disappears after /undo. - Add Logger utility (src/utils/logger.ts) with debug/info/warn/error - Add PendingMessageRepository.deleteBySessionId() for future undo cleanup - Wire logger through all 5 hooks and PlatformIngestionRuntime - Update all affected tests Closes clopca#17

coderabbitai · 2026-05-14T12:13:34Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 5e58ce74-a6f6-4dc2-88b9-09d283540d7d

📥 Commits

Reviewing files that changed from the base of the PR and between ab933c7 and 642bb96.

📒 Files selected for processing (7)

src/db/observations.ts
src/db/schema.ts
src/hooks/chat-capture.ts
src/hooks/session-events.ts
src/types.ts
tests/db/schema.test.ts
tests/hooks/message-removed.test.ts

✅ Files skipped from review due to trivial changes (1)

tests/db/schema.test.ts

🚧 Files skipped from review as they are similar to previous changes (2)

src/types.ts
src/db/schema.ts

📝 Walkthrough

Walkthrough

Adds a Logger utility and threads Logger instances through plugin, worker, runtime, hooks, and session lifecycle; replaces console.* with logger.warn/debug; introduces observations.message_id, ObservationRepository.softDeleteByMessageId, and pending message deletion APIs with accompanying tests and migration.

Changes

Logger Implementation and Hook Integration

Layer / File(s)	Summary
Logger utility and tests `src/utils/logger.ts`, `tests/utils/logger.test.ts`	Adds `LogLevel` and `Logger` with level-gated emission, `[open-mem]` prefixing, runtime `setLevel`, and tests validating behavior and forwarding.
Plugin, worker, and runtime wiring `src/index.ts`, `src/platform-worker.ts`, `src/adapters/platform/runtime.ts`	Instantiate `Logger(config.logLevel)` in plugin/worker and pass logger into hook factories and `PlatformIngestionRuntime`; runtime forwards logger in lifecycleDeps.
Hook signatures, session lifecycle, and tests `src/hooks/`, `tests/hooks/`, `tests/context/*`	Update hook factories (`chat-capture`, `tool-capture`, `compaction`, `context-inject`) and session lifecycle (`session-events`) to accept `logger: Logger`, replace `console.*` with `logger.warn`/`logger.debug`, and update tests to inject test `Logger` instances.

Message ID and /undo Cleanup

Layer / File(s)	Summary
Observations schema and soft-delete `src/db/schema.ts`, `src/db/observations.ts`, `src/types.ts`, `tests/hooks/message-removed.test.ts`	Add migration to add `observations.message_id` and index, persist `message_id` in create/import, map to `Observation.messageId`, add `ObservationRepository.softDeleteByMessageId`, and tests verifying soft-delete and `message.removed` handler.
Pending message deletion by session/call `src/db/pending.ts`, `tests/db/crud.test.ts`	Add `PendingMessageRepository.deleteBySessionId` and `deleteByCallId` to delete only `pending`/`failed` rows and return deleted count; tests validate semantics and edge cases.

🎯 3 (Moderate) | ⏱️ ~25 minutes

clopca/open-mem#3: Overlaps with capture/persist pathway changes (chat/tool capture) and may interact with message capture preprocessing.

"A rabbit pads through logs at night,
Whispering warn where errors used to bite,
Undo now sweeps the crumbs away,
Quiet fields where messages lay,
Hops of code, and all is right." 🐰

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	Title clearly identifies the main change: graceful error handling for /undo fix addressing issue `#17`. It's specific and directly relates to the primary objective.
Description check	✅ Passed	Description provides a solid summary with clear problem statement, comprehensive change table, and verification results. However, the PR description template sections are not explicitly filled out.
Linked Issues check	✅ Passed	PR successfully addresses issue `#17` by implementing logger-based error suppression, replacing 18 console.error calls with level-gated logger methods, and providing config.logLevel enforcement as required.
Out of Scope Changes check	✅ Passed	All changes are within scope: Logger utility creation, hook error handling improvements, and pending message deletion are all directly tied to addressing /undo error logging. Database schema migration for message_id is a necessary supporting change.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint skipped: no ESLint configuration detected in root package.json. To enable, add eslint to devDependencies.

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

greptile-apps · 2026-05-14T12:17:21Z

Greptile Summary

This PR introduces a configurable Logger utility and replaces 18 raw console.error calls across hook files with level-gated logger methods, reducing noise after /undo operations. It also adds PendingMessageRepository.deleteBySessionId() as groundwork for future undo cleanup.

Logger wiring: A Logger singleton is created in src/index.ts and src/platform-worker.ts from config.logLevel, then threaded into all hook factories — session-events, chat-capture, tool-capture all receive a required logger, while compaction and context-inject accept an optional one (silently dropping errors when omitted).
Log level classification: Hook errors are classified as warn (operational) or debug (cosmetic), so at the default warn level most /undo noise disappears without changing visible error reporting for real failures.
Dead-code addition: deleteBySessionId() is added to PendingMessageRepository but not yet called or tested anywhere in this PR.

Confidence Score: 3/5

Safe to merge for the noise-reduction goal, but the optional-logger pattern in compaction and context-inject creates a silent error-swallowing path that differs from every other hook in this PR.

The compaction and context-inject hooks now silently swallow all errors when no logger is provided, which is a regression from the original always-visible console.error behavior. Every other hook in this PR makes logger required, making this inconsistency appear unintentional.

src/hooks/compaction.ts and src/hooks/context-inject.ts — the optional logger parameter should be aligned with the required pattern used in every other hook in this PR.

Important Files Changed

Filename	Overview
src/utils/logger.ts	New Logger class with level-gated output; all levels use console.error (stderr) regardless of severity, which may mislead log aggregators
src/hooks/compaction.ts	logger parameter made optional; when omitted, errors in the compaction hook are silently swallowed — a behavioral regression from the original console.error
src/hooks/context-inject.ts	Same optional-logger issue as compaction.ts; errors are silently dropped when logger is not provided
src/db/pending.ts	Adds deleteBySessionId() for undo cleanup — correct SQL pattern matching existing methods, but no tests added and the method is not yet called anywhere
src/hooks/session-events.ts	console.error calls replaced with logger.warn/debug; logger is required (not optional), preserving error visibility
src/index.ts	Logger singleton created from config.logLevel and wired to all hook factories cleanly
tests/utils/logger.test.ts	Thorough test coverage for Logger: level gating, setLevel, shouldLog, prefix format, and argument passthrough

Comments Outside Diff (3)

src/hooks/compaction.ts, line 103-113 (link)

Silent error swallowing when logger is omitted

logger is typed as optional (logger?: Logger), so any call site that omits it silently suppresses all errors via logger?.warn(...). The pre-PR behavior always printed via console.error regardless of context. Existing tests for this hook (which weren't updated) now exercise error paths with no logger — errors vanish completely. The same issue exists in context-inject.ts. Making logger required (matching tool-capture.ts, chat-capture.ts, and session-events.ts) would restore the original guarantee that hook errors are never silently dropped.

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/hooks/compaction.ts
Line: 103-113

Comment:
**Silent error swallowing when logger is omitted**

`logger` is typed as optional (`logger?: Logger`), so any call site that omits it silently suppresses all errors via `logger?.warn(...)`. The pre-PR behavior always printed via `console.error` regardless of context. Existing tests for this hook (which weren't updated) now exercise error paths with no logger — errors vanish completely. The same issue exists in `context-inject.ts`. Making `logger` required (matching `tool-capture.ts`, `chat-capture.ts`, and `session-events.ts`) would restore the original guarantee that hook errors are never silently dropped.

How can I resolve this? If you propose a fix, please make it concise.

src/utils/logger.ts, line 40-63 (link)

All levels route to console.error (stderr)

debug, info, warn, and error all call console.error, so every log message — regardless of level — is written to stderr. The JSDoc acknowledges this as intentional, but it means a log aggregator or monitoring tool that distinguishes fd2 from fd1 will classify every logger.debug(...) call as an error-level event. Using console.warn for the warn level and console.debug/console.log for lower levels would be more semantically correct.

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/utils/logger.ts
Line: 40-63

Comment:
**All levels route to `console.error` (stderr)**

`debug`, `info`, `warn`, and `error` all call `console.error`, so every log message — regardless of level — is written to stderr. The JSDoc acknowledges this as intentional, but it means a log aggregator or monitoring tool that distinguishes fd2 from fd1 will classify every `logger.debug(...)` call as an error-level event. Using `console.warn` for the `warn` level and `console.debug`/`console.log` for lower levels would be more semantically correct.

How can I resolve this? If you propose a fix, please make it concise.

src/db/pending.ts, line 125-134 (link)

deleteBySessionId is unused and untested

The method is described as "for future undo cleanup integration" but is not called anywhere in this PR, and no tests were added for it. Adding a test that verifies the method deletes only pending/failed rows and leaves processing/completed rows untouched would prevent an undetected regression if the row status values change.

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/db/pending.ts
Line: 125-134

Comment:
**`deleteBySessionId` is unused and untested**

The method is described as "for future undo cleanup integration" but is not called anywhere in this PR, and no tests were added for it. Adding a test that verifies the method deletes only `pending`/`failed` rows and leaves `processing`/`completed` rows untouched would prevent an undetected regression if the row status values change.

How can I resolve this? If you propose a fix, please make it concise.

Prompt To Fix All With AI

Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 3
src/hooks/compaction.ts:103-113
**Silent error swallowing when logger is omitted**

`logger` is typed as optional (`logger?: Logger`), so any call site that omits it silently suppresses all errors via `logger?.warn(...)`. The pre-PR behavior always printed via `console.error` regardless of context. Existing tests for this hook (which weren't updated) now exercise error paths with no logger — errors vanish completely. The same issue exists in `context-inject.ts`. Making `logger` required (matching `tool-capture.ts`, `chat-capture.ts`, and `session-events.ts`) would restore the original guarantee that hook errors are never silently dropped.

### Issue 2 of 3
src/utils/logger.ts:40-63
**All levels route to `console.error` (stderr)**

`debug`, `info`, `warn`, and `error` all call `console.error`, so every log message — regardless of level — is written to stderr. The JSDoc acknowledges this as intentional, but it means a log aggregator or monitoring tool that distinguishes fd2 from fd1 will classify every `logger.debug(...)` call as an error-level event. Using `console.warn` for the `warn` level and `console.debug`/`console.log` for lower levels would be more semantically correct.

### Issue 3 of 3
src/db/pending.ts:125-134
**`deleteBySessionId` is unused and untested**

The method is described as "for future undo cleanup integration" but is not called anywhere in this PR, and no tests were added for it. Adding a test that verifies the method deletes only `pending`/`failed` rows and leaves `processing`/`completed` rows untouched would prevent an undetected regression if the row status values change.

_{Reviews (1): Last reviewed commit: "fix: graceful error handling for /undo (..." | Re-trigger Greptile}

greptile-apps · 2026-05-14T12:17:48Z

+	debug(message: string, ...args: unknown[]): void {
+		if (this.shouldLog("debug")) {
+			console.error(`[open-mem] ${message}`, ...args);
+		}
+	}
+
+	info(message: string, ...args: unknown[]): void {
+		if (this.shouldLog("info")) {
+			console.error(`[open-mem] ${message}`, ...args);
+		}
+	}
+
+	warn(message: string, ...args: unknown[]): void {
+		if (this.shouldLog("warn")) {
+			console.error(`[open-mem] ${message}`, ...args);
+		}
+	}
+
+	error(message: string, ...args: unknown[]): void {
+		if (this.shouldLog("error")) {
+			console.error(`[open-mem] ${message}`, ...args);
+		}
+	}
+}


All levels route to console.error (stderr)

debug, info, warn, and error all call console.error, so every log message — regardless of level — is written to stderr. The JSDoc acknowledges this as intentional, but it means a log aggregator or monitoring tool that distinguishes fd2 from fd1 will classify every logger.debug(...) call as an error-level event. Using console.warn for the warn level and console.debug/console.log for lower levels would be more semantically correct.

Prompt To Fix With AI

This is a comment left during a code review. Path: src/utils/logger.ts Line: 40-63 Comment: **All levels route to `console.error` (stderr)** `debug`, `info`, `warn`, and `error` all call `console.error`, so every log message — regardless of level — is written to stderr. The JSDoc acknowledges this as intentional, but it means a log aggregator or monitoring tool that distinguishes fd2 from fd1 will classify every `logger.debug(...)` call as an error-level event. Using `console.warn` for the `warn` level and `console.debug`/`console.log` for lower levels would be more semantically correct. How can I resolve this? If you propose a fix, please make it concise.

Fixed in 2bf1552 — Logger now routes debug/info to console.log (stdout), warn to console.warn, error to console.error. Tests updated to verify correct console method routing.

greptile-apps · 2026-05-14T12:17:50Z

+	deleteBySessionId(sessionId: string): number {
+		const result = this.db.all<{ id: string }>(
+			`DELETE FROM pending_messages
+			 WHERE session_id = ?
+			 AND status IN ('pending', 'failed')
+			 RETURNING id`,
+			[sessionId],
+		);
+		return result.length;
+	}


deleteBySessionId is unused and untested

The method is described as "for future undo cleanup integration" but is not called anywhere in this PR, and no tests were added for it. Adding a test that verifies the method deletes only pending/failed rows and leaves processing/completed rows untouched would prevent an undetected regression if the row status values change.

Prompt To Fix With AI

This is a comment left during a code review. Path: src/db/pending.ts Line: 125-134 Comment: **`deleteBySessionId` is unused and untested** The method is described as "for future undo cleanup integration" but is not called anywhere in this PR, and no tests were added for it. Adding a test that verifies the method deletes only `pending`/`failed` rows and leaves `processing`/`completed` rows untouched would prevent an undetected regression if the row status values change. How can I resolve this? If you propose a fix, please make it concise.

Fixed — deleteBySessionId is now wired into all session lifecycle handlers (session.created, session.idle, session.completed/session.ended). Tests (3 test cases) already added in previous commit covering: status filtering (pending+failed removed, processing+completed retained), empty session handling (returns 0), and cross-session isolation.

- Logger: debug/info → console.log (stdout), warn → console.warn, error → console.error - Tests: pass Logger to all hook call sites (was made required) - Tests: capture console.log/warn/error for Logger verification - Fix extra bracket in crud.test.ts

Address review feedback — deleteBySessionId is now called in: - session.created: cleanup stale pending/failed from previous session lifecycle - session.idle: cleanup orphaned messages (covers /undo scenario) - session.completed/ended: final cleanup after processing Also updated session-events test mock to include deleteBySessionId.

coderabbitai

🧹 Nitpick comments (1)

src/db/schema.ts (1)
35-40: ⚡ Quick win

Migration array ordering: version 2 appears before version 1.

While the database migration framework (line 448 of src/db/database.ts) explicitly sorts migrations by version before execution, the current array ordering in the MIGRATIONS constant harms readability. Consider reordering migrations sequentially by version to match execution order and improve maintainability.
🤖 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/db/schema.ts` around lines 35 - 40, The MIGRATIONS array currently lists
the entry with version: 2 before the one with version: 1 which hurts
readability; reorder the objects in the MIGRATIONS constant so migrations are
listed sequentially (place the version: 1 migration before the version: 2
migration) to match execution order, leaving the migration SQL strings and index
creation (message_id column / idx_observations_message_id) unchanged.

🤖 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.

Nitpick comments:
In `@src/db/schema.ts`:
- Around line 35-40: The MIGRATIONS array currently lists the entry with
version: 2 before the one with version: 1 which hurts readability; reorder the
objects in the MIGRATIONS constant so migrations are listed sequentially (place
the version: 1 migration before the version: 2 migration) to match execution
order, leaving the migration SQL strings and index creation (message_id column /
idx_observations_message_id) unchanged.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: d9a69fbb-12ff-4bf5-ad12-43b21fb02086

📥 Commits

Reviewing files that changed from the base of the PR and between 85d2b39 and ab933c7.

📒 Files selected for processing (6)

src/db/observations.ts
src/db/pending.ts
src/db/schema.ts
src/hooks/chat-capture.ts
src/types.ts
tests/db/schema.test.ts

🚧 Files skipped from review as they are similar to previous changes (1)

src/db/pending.ts

- Add message_id column to observations table (migration v2) - Store messageID from chat.message hook in observations - Add ObservationRepository.softDeleteByMessageId() for undo handling - Handle message.removed events in createEventHandler to soft-delete observations tied to the removed message - Remove deleteBySessionId from session.idle (message.removed is more precise, avoids aggressive pending message cleanup) - Update schema idempotency test for 2 migrations - Add 8 tests for message.removed handling (repository + event handler)

wjiuxing requested a review from clopca as a code owner May 14, 2026 12:13

greptile-apps Bot reviewed May 14, 2026

View reviewed changes

jiuxingwang added 2 commits May 14, 2026 20:54

coderabbitai Bot reviewed May 15, 2026

View reviewed changes

wjiuxing force-pushed the fix/undo-error-handling branch from ab933c7 to 85d2b39 Compare May 15, 2026 02:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: graceful error handling for /undo (issue #17)#19

fix: graceful error handling for /undo (issue #17)#19
wjiuxing wants to merge 4 commits into
clopca:mainfrom
wjiuxing:fix/undo-error-handling

wjiuxing commented May 14, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented May 14, 2026 •

edited

Loading

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

greptile-apps Bot commented May 14, 2026 •

edited

Loading

Comments Outside Diff (3)

Uh oh!

greptile-apps Bot May 14, 2026

Uh oh!

wjiuxing May 14, 2026

Uh oh!

greptile-apps Bot May 14, 2026

Uh oh!

wjiuxing May 14, 2026

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

wjiuxing commented May 14, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Problem

Changes

Verification

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

greptile-apps Bot commented May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 3/5

Important Files Changed

Comments Outside Diff (3)

Uh oh!

greptile-apps Bot May 14, 2026

Choose a reason for hiding this comment

Uh oh!

wjiuxing May 14, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot May 14, 2026

Choose a reason for hiding this comment

Uh oh!

wjiuxing May 14, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

wjiuxing commented May 14, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented May 14, 2026 •

edited

Loading

greptile-apps Bot commented May 14, 2026 •

edited

Loading