[lihongyuan99]

Summary

wren memory watch is the one memory command that cannot tell you why it stopped working. When a reindex fails, the user sees a fixed sentence and the exception that explains the problem is thrown away:

Reindex failed; change kept pending, will retry next poll.

Meanwhile the object it just discarded said, in full (the OnnxBackendError from the issue's repro):

The onnx embedding backend implements mean pooling, but 'BAAI/bge-small-en-v1.5'
pools with ['cls_token']. Set WREN_EMBEDDING_BACKEND=sentence-transformers to use
this model.

This adds the opt-in on_error(event, exc) callback that #2724 suggests as the smaller of its two fixes, and prints the reason from watch.

Closes #2724 — consequence 2 only. See Scope below for why consequence 1 is not in this PR.

What changes for a user

wren memory watch now reports why a reindex failed, on every attempt, instead of an unexplained retry loop:

Reindex failed: The onnx embedding backend implements mean pooling, but 'BAAI/bge-
small-en-v1.5' pools with ['cls_token']. Set WREN_EMBEDDING_BACKEND=sentence-
transformers to use this model.; change kept pending, will retry next poll.

The retry behaviour, the fingerprint semantics and WatchState.errors are untouched.

How it works

The exception is available at the point where it is dropped, so this is a plumbing change:

• poll_once (src/wren/memory/watch.py) binds the exception it already catches and calls on_error("reindex-error", exc) before re-raising. Re-raising is unchanged, so watch_loop still keeps the change pending and retries.
• watch_loop accepts and forwards on_error. It deliberately does not call it again in its own except clause: the same exception arrives there as a re-raise from poll_once, so reporting at both sites would print two messages per attempt. The docstring states this.
• memory/cli.py moves the "Reindex failed" line out of _on_event into a new _on_error(event, exc) that interpolates str(exc) or type(exc).__name__ (an exception with an empty message still names its class).

on_event keeps its Callable[[str], None] contract, which is why this is additive rather than a widening: the existing tests pass on_event=events.append, a one-argument callable, and #2724 records that both of its proposed shapes "alter watch_loop's contract". A new keyword-only parameter with a None default is the one form that does not.

Tests

Three tests in tests/unit/test_memory_watch.py, all calling poll_once / watch_loop and asserting on what the callbacks actually receive — not on source text:

Test	Asserts
test_poll_once_reports_reindex_reason_to_on_error	the exact (event, exception) pair reaches on_error, and poll_once still re-raises
test_watch_loop_reports_reindex_reason_once_per_attempt	over two polls with a failing reindex: state.errors == 2 and the rendered messages are ["reindex-error: attempt 1: …", "reindex-error: attempt 2: …"] — i.e. one reason per attempt, no duplicate
test_watch_loop_on_event_still_works_without_on_error	on_event=events.append (one positional arg) still drives a failing loop: the new parameter is opt-in

Red before the fix, with the source changes reverted and only the tests present:

E   TypeError: watch_loop() got an unexpected keyword argument 'on_error'
2 failed, 14 passed in 0.44s

Green after: 16 passed in 0.25s. The third test is expected to pass both before and after — it pins the compatibility property, so it can only fail if on_event's contract is broken.

These run in CI. test_memory_watch.py carries no pytestmark, so a -m unit selection silently skips it; the unit tests job in .github/workflows/wren-ci.yml runs the whole tree instead and says so in a comment ("the marker is opt-in, so any unmarked file … was silently skipped"). Measured with that job's own command:

Command (core/wren, Python 3.11 venv)	Result
pytest tests/unit/ --ignore=tests/unit/test_memory.py --ignore=tests/unit/test_mcp_server.py on d26ab6a	1 failed, 1498 passed, 2 skipped
same command with this branch	1 failed, 1501 passed, 2 skipped

The one failure is test_cloud.py::test_create_refuses_without_a_git_identity_before_creating_anything in both runs — pre-existing on main, unrelated to this change (no git identity in a detached checkout). Delta is exactly +3, the three tests added here.

Lint, with the lint job's own commands (uvx ruff format --check src/, uvx ruff check src/): 71 files already formatted, All checks passed!.

Not run, and I am not claiming it: the end-to-end repro from #2724 (WREN_EMBEDDING_BACKEND=onnx + BAAI/bge-small-en-v1.5). It needs the memory extra and an ONNX model download, neither available here. The CLI branch is three lines of typer.echo fed by the callback the unit tests cover; I did not add a CLI-level test because reaching _reindex requires stubbing the backend guard and project discovery, which would assert on my own mocks rather than on the behaviour.

Scope

#2724 lists two consequences. This PR fixes consequence 2 (the dropped reason), which the issue itself calls "the smaller change" and "the one that costs a user their afternoon".

Consequence 1 — retrying a permanent failure forever — is not here on purpose. It requires choosing a terminal exception set (OnnxBackendError specifically, or something broader), and the issue says that is a deliberate decision because anything wider "risks killing a watcher over a genuinely transient error, which is the failure mode the current code was written to avoid". That is a maintainer call, not a doc-level fix, and the two compose independently.

I also did not touch #2624's recorded decision that this is not an injection surface, and no claim in this PR depends on one.

Duplicate check

Retrieved the changed-file list for all 73 open PRs (GET /pulls?state=open, pages 1-2, cross-checked against the Link: rel="last" header showing 73 is the complete count, so this is not a gh pr list --limit truncation). No open PR touches core/wren/src/wren/memory/watch.py, core/wren/src/wren/memory/cli.py or core/wren/tests/unit/test_memory_watch.py — in fact none touches any file under core/wren/src/wren/memory/.

#2707 is referenced by #2724 as where this was split out from; that PR is merged and covers routing the error for index/store/recall/fetch, which is the behaviour watch is now being brought in line with.

Summary by CodeRabbit

• Bug Fixes
  • Watch mode now shows the error message when reindexing fails, or the error type if no message is available.
  • Failed changes remain pending and are retried on the next poll; the watch does not mark them as processed.
  • Error details are reported without an extra generic failure message, making issues such as malformed JSON clearer.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

🧰 Additional context used

📚 Code guidelines (1)

.claude/CLAUDE.md — configured

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 783f33dd-a533-4571-8584-654badc9183e

📥 Commits

Reviewing files that changed from the base of the PR and between 5ae576f and c6870ec.

📒 Files selected for processing (2)

• core/wren/src/wren/memory/cli.py
• core/wren/tests/unit/test_memory_watch.py

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

---

Walkthrough

The watcher now accepts an optional on_error callback and forwards reindex exceptions to it. The CLI reports the exception message, or its type when the message is empty, while retaining the pending-change retry notice. The watch loop continues its existing retry behavior.

Changes

Watcher Reindex Error Reporting

Layer / File(s)	Summary
Forward reindex failures to the error callback core/wren/src/wren/memory/watch.py, core/wren/tests/unit/test_memory_watch.py	poll_once calls the optional on_error callback with the failure status and exception, then re-raises. watch_loop forwards the callback to each poll. Tests cover callback invocation and compatibility with the existing event callback.
Display reindex failure details core/wren/src/wren/memory/cli.py, core/wren/tests/unit/test_memory_watch.py	The CLI supplies the error callback to watch_loop. It displays exception text, or the exception type when the text is empty, and retains the pending-change retry notice. A CLI test checks the output for truncated manifest JSON.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: bartok9

Merge Risk: ⚪ Minimal · up to c6870

The watcher now reports reindex failure details while continuing to retry pending changes. No actionable merge-blocking risk is established.

Security Architecture Review

Security architecture risk: 🔵 Low · up to c6870

The change remains within the local memory command and preserves pending-change retries. No security defect was established, but the suitability of all backend exception messages for captured error output remains unconfirmed.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The added reporting path exposes reindex exception text to recipients of the invoking CLI’s stderr. It introduces no network transport or additional execution authority in the inspected change; externally configured log collection was not assessed.

Trust Boundaries and Controls

• observed — The new sink directly formats exception text into stderr. Detailed local error reporting already exists for OnnxBackendError and manifest parsing, but those precedents do not establish the contents of every exception now surfaced by watch.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 53.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 3 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: exposing the reindex failure reason from the memory watch loop.
Description check	✅ Passed	The description includes all required template sections. It provides the failure reproduction and actual output, explains the implementation and scope, documents tests and CI results, and records the …
Linked Issues check	✅ Passed	The PR meets the relevant coding objective in [#2724]. poll_once forwards the original reindex exception through on_error before it re-raises. watch_loop preserves the retry behavior and forward…
Out of Scope Changes check	✅ Passed	The changes remain within [#2724]. They add the error callback, connect it to wren memory watch, preserve existing retry behavior, and add focused tests. No unrelated change is demonstrated.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{A rabbit hears the watcher sigh,
An error’s words now reach the sky.
The pending change waits one more round,
Its cause is clear, its clues are found.
Soft paws applaud the callback’s sound.}

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

[goldmedal]

Thanks — the split from #2724 is well-reasoned, and keeping on_event's one-argument contract while adding a keyword-only on_error is the right shape. The three tests would genuinely fail on a broken implementation, and the once-per-attempt test pins the no-duplicate property nicely.

One issue in the CLI branch, reproduced against 5ae576f:

_reindex can fail by echoing its own error and raising typer.Exit(1) — e.g. _load_manifest on a half-written mdl.json, which is the canonical transient case the watcher's retry exists for. typer.Exit subclasses RuntimeError, so poll_once catches it, and str(typer.Exit(1)) == "1". With a truncated target/mdl.json the output is:

Error: invalid JSON in .../target/mdl.json: Unterminated string starting at: line 1 column 2 (char 1)
Reindex failed: 1; change kept pending, will retry next poll.

Reindex failed: 1 reads like an error code and adds nothing, since the real reason was already printed on the line above. Suggest skipping the reason for typer.Exit:

def _on_error(event: str, exc: BaseException) -> None:
    if event != "reindex-error":
        return
    # typer.Exit means _reindex already echoed its own error.
    reason = "" if isinstance(exc, typer.Exit) else f": {str(exc) or type(exc).__name__}"
    typer.echo(
        f"Reindex failed{reason}; change kept pending, will retry next poll.",
        err=True,
    )

This path is also cheap to cover at the CLI level: _load_manifest fails before _get_store, so a CliRunner test only needs resolve_backend monkeypatched to a non-grep value plus an invalid mdl.json. It doesn't need the memory extra or any backend mocks, and it would pin both the typer.Exit case and the message format.

[lihongyuan99]

Fixed in c6870ec — _on_error now skips the reason for typer.Exit (as you noted, the real error was already echoed by _reindex on the line above, so str(exc) only repeated the exit code). Also added test_cli_watch_truncated_mdl_reports_reason_without_exit_code following your sketch: resolve_backend monkeypatched to a non-grep value plus a truncated target/mdl.json — no memory extra or backend mocks needed, and it pins both the typer.Exit case and the message format. The full test_memory_watch.py passes locally (17 passed).

[goldmedal]

Thanks @lihongyuan99 👍