nowledge-mem-opencode-plugin/src/index.ts (1)

190-215: ⚡ Quick win

normalizeSessionMessages looks robust; consider logging in fetchSessionMessages's catch block.

The { path: { id }, query } shape in the first attempt matches the current OpenCode SDK convention (the official docs show client.session.get({ path: { id: "invalid-id" } }) as the canonical pattern), so the first attempt should succeed on current builds and the second acts as a correct legacy fallback.

However, the empty catch {} at line 210 silently swallows every exception — including real failures like a dead server, auth errors, or a future SDK breaking change. When both attempts fail, the caller receives [] and returns "No messages found in current session", with no diagnostic signal about why. A single console.warn (or client.app.log if the client is in scope) would let users distinguish a genuine empty session from a transport/API failure without changing any behavior.

🔍 Suggested change

-      try {
-        const messages = normalizeSessionMessages(await client.session.messages(options as any))
-        if (messages.length > 0) return messages
-      } catch {
-        // Keep compatibility across OpenCode SDK shapes.
-      }
+      try {
+        const messages = normalizeSessionMessages(await client.session.messages(options as any))
+        if (messages.length > 0) return messages
+      } catch (err: any) {
+        // Keep compatibility across OpenCode SDK shapes; log for diagnosability.
+        console.warn("[nowledge-mem] session.messages attempt failed:", err?.message ?? err)
+      }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@nowledge-mem-opencode-plugin/src/index.ts` around lines 190 - 215, The empty
catch in fetchSessionMessages silently swallows errors; update the catch in
fetchSessionMessages (the try around client.session.messages calls) to log the
caught error details (e.g., using console.warn or client.app.log if client is
available) along with context such as the options/attempt being used and the
sessionID so transient transport/auth/SDK errors are visible while preserving
the existing fallback behavior to the legacy attempt and the final return of [].

nowledge-mem-hermes/tests/test_space_resolution.py (1)

283-336: ⚡ Quick win

Add a negative test to assert space_id is absent when no space is configured.

The updated test now covers the positive path, but there is no companion assertion confirming that space_id is not included in the payload when NowledgeMemClient() is constructed without a space argument. Without it, silently removing the if self._has_explicit_space and self._space: guard in client.py would go undetected.

✅ Suggested companion test

def test_thread_append_omits_space_id_when_no_space_configured(self):
    captured: dict[str, object] = {}
    original_run = client_module.subprocess.run
    original_urlopen = client_module.urlrequest.urlopen
    previous_url = os.environ.get("NMEM_API_URL")
    previous_key = os.environ.get("NMEM_API_KEY")
    os.environ["NMEM_API_URL"] = "http://mem.test"
    os.environ["NMEM_API_KEY"] = ""

    def _bad_run(*_args, **_kwargs):
        raise AssertionError("thread append must not use subprocess")

    class _Response:
        def __enter__(self): return self
        def __exit__(self, *_): return False
        def read(self): return b'{"success": true, "messages_added": 1}'

    def _fake_urlopen(request, **_kwargs):
        captured["body"] = request.data.decode("utf-8")
        return _Response()

    try:
        client_module.subprocess.run = _bad_run
        client_module.urlrequest.urlopen = _fake_urlopen
        # No space argument — space_id must not appear in payload
        client = client_module.NowledgeMemClient()
        client.append_thread("s1", [{"role": "user", "content": "hello"}])
        payload = json.loads(captured["body"])
        self.assertNotIn("space_id", payload)
    finally:
        client_module.subprocess.run = original_run
        client_module.urlrequest.urlopen = original_urlopen
        if previous_url is None:
            os.environ.pop("NMEM_API_URL", None)
        else:
            os.environ["NMEM_API_URL"] = previous_url
        if previous_key is None:
            os.environ.pop("NMEM_API_KEY", None)
        else:
            os.environ["NMEM_API_KEY"] = previous_key

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@nowledge-mem-hermes/tests/test_space_resolution.py` around lines 283 - 336,
Add a negative test that constructs NowledgeMemClient() without the space
argument and asserts that the outgoing append_thread payload does not include
"space_id": copy the mocking pattern from
test_thread_append_posts_payload_without_subprocess_argv (override
client_module.subprocess.run to raise, monkeypatch
client_module.urlrequest.urlopen to capture request.data in captured["body"],
create a fake response that returns success), call client =
client_module.NowledgeMemClient() then client.append_thread(...), parse json
from captured["body"] and assert "space_id" is not in the payload; this will
validate the guard around NowledgeMemClient._has_explicit_space /
NowledgeMemClient._space in client.py remains effective.

nowledge-mem-opencode-plugin/package.json (1)

31-33: Consider tightening the @opencode-ai/plugin peer dependency lower bound to ^1.3.7.

The { path: { id } } call shape for client.session.messages() was introduced in version 1.3.7 or earlier. The current peer dependency ^1.3.0 permits versions 1.3.0–1.3.6, which would not support the primary call pattern that fetchSessionMessages targets on line 208. While the code includes a fallback attempt with { sessionID } syntax for broader compatibility, tightening the lower bound to ^1.3.7 would eliminate the risk of silent failures on outdated SDK versions and clarify the actual minimum SDK requirement for this plugin.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@nowledge-mem-opencode-plugin/package.json` around lines 31 - 33, Update the
package peer dependency for `@opencode-ai/plugin` from "^1.3.0" to "^1.3.7" to
reflect the minimum SDK that implements the { path: { id } } call shape used by
fetchSessionMessages; this ensures client.session.messages() supports the {
path: { id } } signature (and avoids relying solely on the fallback using {
sessionID }) and clearly documents the required minimum version for the plugin.

nowledge-mem-codex-plugin/scripts/install_hooks.py (1)

139-148: 💤 Low value

TOML validation doesn't prevent writing if input is invalid.

_validate_toml_if_possible parses the existing config but doesn't raise or warn if parsing fails. If the existing config.toml is malformed, the script will proceed to modify it with string manipulation, potentially making it worse or creating a hybrid invalid file.

Consider either warning the user or backing up invalid TOML before modification.

♻️ Proposed enhancement

 def _validate_toml_if_possible(text: str) -> None:
     if not text.strip():
         return
     try:
         import tomllib
 
         tomllib.loads(text)
-    except ModuleNotFoundError:
+    except ModuleNotFoundError:
+        return
+    except Exception as exc:
+        print(f"warning: existing config.toml may be invalid: {exc}", file=sys.stderr)
         return

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@nowledge-mem-codex-plugin/scripts/install_hooks.py` around lines 139 - 148,
The _validate_toml_if_possible function currently swallows TOML parse errors and
does nothing, allowing the script to continue modifying an invalid config;
update _validate_toml_if_possible to catch tomllib.TOMLDecodeError (or generic
Exception if tomllib lacks that name) and either raise a descriptive exception
or return a flag indicating invalid TOML, then update callers (e.g., the code
that writes/edits config.toml) to abort or create a backup copy of the existing
file before making changes and log a clear warning including the parse error;
reference the function name _validate_toml_if_possible and ensure callers check
its result or handle the raised exception to avoid in-place modifications of
malformed TOML.

nowledge-mem-hermes/__init__.py (1)

60-64: 💤 Low value

Consider logging shutdown failures for diagnostics.

The static analysis tool flagged the silent try-except-pass pattern. While swallowing shutdown errors is appropriate here (the provider is being replaced anyway), logging at debug level would help diagnose issues without affecting runtime.

♻️ Proposed fix

         if _fallback_provider is not None:
             try:
                 _fallback_provider.shutdown()
-            except Exception:
-                pass
+            except Exception as exc:
+                logger.debug("Fallback provider shutdown error (ignored): %s", exc)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@nowledge-mem-hermes/__init__.py` around lines 60 - 64, The silent except in
the shutdown of _fallback_provider should log the exception at debug level for
diagnostics: wrap the call to _fallback_provider.shutdown() in the existing
try/except, capture the Exception as e, and call a module logger (e.g. logger =
logging.getLogger(__name__)) with logger.debug("Fallback provider shutdown
failed", exc_info=e) or logger.debug(..., exc_info=True) so the stacktrace is
available; keep swallowing the exception after logging so behavior is unchanged
and reference the symbol _fallback_provider.shutdown to locate the change.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes