Skip to content

fix(memory): clean up unused JWT token parameter in memory init (#204)#300

Merged
graycyrus merged 2 commits into
tinyhumansai:mainfrom
sanil-23:feature/204-jwt-remote-memory-init
Apr 3, 2026
Merged

fix(memory): clean up unused JWT token parameter in memory init (#204)#300
graycyrus merged 2 commits into
tinyhumansai:mainfrom
sanil-23:feature/204-jwt-remote-memory-init

Conversation

@sanil-23
Copy link
Copy Markdown
Contributor

@sanil-23 sanil-23 commented Apr 3, 2026

Summary

  • Removed unused MemoryClient::from_token() that silently ignored the JWT parameter
  • Made jwt_token optional (Option<String>) in MemoryInitRequest for backward compatibility
  • Updated RPC schema, frontend syncMemoryClientToken(), and SDK documentation to reflect local-only memory
  • Updated integration test to verify memory.init accepts empty params

Problem

memory_init RPC accepted a JWT token but always created a local SQLite client, ignoring the token entirely. The frontend sent a JWT on every login via syncMemoryClientToken(), creating confusion about whether remote memory sync was supported. (#204)

Solution

Chose the "remove and document" approach: removed from_token(), made jwt_token optional with #[serde(default)], and documented at every touch point (Rust doc-comments, RPC schema description, SDK docs, TypeScript JSDoc) that memory is local-only. Frontend still sends the token for backward compat but it's explicitly discarded.

Submission Checklist

  • Unit tests — Updated invoke_memory_init_accepts_empty_params test in jsonrpc.rs
  • E2E / integration — N/A, no user-visible behavior change
  • Doc comments — Added to MemoryClient, MemoryInitRequest, syncMemoryClientToken, SDK docs
  • Inline commentslet _ = request.jwt_token; with explanation

Impact

  • Desktop only. No migration needed — the RPC is backward compatible (old clients sending jwt_token still work).

Related

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Refactor

    • Memory storage now operates as local-only (SQLite-based) instead of cloud-backed, with JWT authentication no longer required.
  • Documentation

    • Updated integration documentation to reflect local-only memory initialization and storage approach.
  • Chores

    • Updated Windows MSVC build configuration via forked dependency.

sanil-23 and others added 2 commits April 2, 2026 19:50
The upstream whisper-rs-sys builds whisper.cpp via CMake which
defaults to /MD (dynamic CRT), but Rust and all other C deps
use /MT (static CRT). This causes LNK2038/LNK1169 linker errors
on Windows.

Patch whisper-rs-sys from tinyhumansai/whisper-rs-sys fork which
adds config.static_crt(true) and overrides all per-config CMake
flags (Debug/Release/MinSizeRel/RelWithDebInfo) from /MD to /MT.

Closes tinyhumansai#273
Memory is local-only (SQLite). The from_token() method accepted a JWT
but ignored it, always falling back to new_local(). Remove the dead
method, make jwt_token optional in MemoryInitRequest for backward
compat, and document the local-only design.

Closes tinyhumansai#204

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 3, 2026

📝 Walkthrough

Walkthrough

This PR resolves issue #204 by removing the unused JWT token parameter from the memory initialization flow. The changes clarify that memory operations are local-only (SQLite-backed), making the jwt_token parameter optional and explicitly ignored. The token is retained for backward compatibility but no longer validated or processed. Changes span the RPC models, memory store implementation, frontend integration, tests, and documentation.

Changes

Cohort / File(s) Summary
Dependency Configuration
Cargo.toml
Added [patch.crates-io] override to replace whisper-rs-sys with forked git dependency (https://github.com/tinyhumansai/whisper-rs-sys.git branch main).
RPC Model Updates
src/openhuman/memory/rpc_models.rs, src/openhuman/memory/schemas.rs
Changed jwt_token from required String to optional Option<String> with #[serde(default)] in request model. Updated controller schema to reflect optional, ignored jwt_token parameter and local-only (SQLite) initialization semantics.
Memory Store Implementation
src/openhuman/memory/store/client.rs, src/openhuman/memory/ops.rs
Removed from_token() constructor from MemoryClient. Removed validation of jwt_token in memory_init operation; token now accepted but unused. Added documentation clarifying local-only, on-device SQLite storage with no remote sync.
Frontend Integration
app/src/utils/tauriCommands.ts
Updated syncMemoryClientToken() early return to depend only on isTauri() check. Clarified in comments that jwt_token is accepted for backward compatibility but ignored by core; operation marked as "local-only (SQLite)".
Documentation
docs/tinyhumansai-sdk.md
Replaced JWT-derived remote initialization description with local-only model via openhuman.memory_init RPC. Updated code snippets to show new_local() and from_workspace_dir() constructors instead of from_token(); documented jwt_token as backward-compatible but ignored.
Unit Tests
src/core/jsonrpc.rs
Updated test for openhuman.memory_init to accept empty JSON parameters {} and verify error (if any) does not indicate missing required jwt_token.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Poem

🐰 Once tokens flew through distant skies,
But local SQLite caught our eyes—
No remote sync needed here,
Just cozy storage, crystal clear!
The token's kept for old friends' sake,
But local-only's now the way we take! 🌱

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title clearly summarizes the primary change: removing the unused JWT token parameter from memory initialization and cleaning up related code.
Linked Issues check ✅ Passed All acceptance criteria from issue #204 are met: JWT token parameter is removed, memory is documented as local-only, and frontend syncMemoryClientToken() is updated accordingly.
Out of Scope Changes check ✅ Passed All changes are directly scoped to issue #204: Cargo.toml patch is dependency-related, code changes remove unused token parameters, and documentation updates reflect local-only memory design.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/tinyhumansai-sdk.md`:
- Line 390: Update the docs to reflect the current runtime holder: replace the
description that the client is stored in MemoryState with the actual global
holder MEMORY_CLIENT_STATE, and state its type/structure as
OnceLock<Mutex<Option<MemoryClientRef>>> (the active holder defined in
openhuman::memory::ops, referencing MEMORY_CLIENT_STATE and MemoryClientRef) so
the documentation matches the implementation.
🪄 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: a5e5e615-5007-4524-9ae1-a77406572274

📥 Commits

Reviewing files that changed from the base of the PR and between 898fe13 and 5c2daa3.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (8)
  • Cargo.toml
  • app/src/utils/tauriCommands.ts
  • docs/tinyhumansai-sdk.md
  • src/core/jsonrpc.rs
  • src/openhuman/memory/ops.rs
  • src/openhuman/memory/rpc_models.rs
  • src/openhuman/memory/schemas.rs
  • src/openhuman/memory/store/client.rs

Comment thread docs/tinyhumansai-sdk.md
```

The client is stored as `Arc<MemoryClient>` inside a `Mutex<Option<MemoryClientRef>>` (`MemoryState`), shared across Tauri commands.
The client is stored as `Arc<MemoryClient>` inside a `Mutex<Option<MemoryClientRef>>` (`MemoryState`), shared across RPC handlers.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Fix the state-holder reference to match current implementation.

Line 390 says the client is stored via MemoryState, but the active runtime holder is the global MEMORY_CLIENT_STATE in src/openhuman/memory/ops.rs (OnceLock<Mutex<Option<MemoryClientRef>>>). Updating this avoids implementation drift in docs.

📝 Proposed doc fix
-The client is stored as `Arc<MemoryClient>` inside a `Mutex<Option<MemoryClientRef>>` (`MemoryState`), shared across RPC handlers.
+The client is stored as `Arc<MemoryClient>` in a process-wide `OnceLock<Mutex<Option<MemoryClientRef>>>`
+(`MEMORY_CLIENT_STATE` in `src/openhuman/memory/ops.rs`), shared across RPC handlers.
📝 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
The client is stored as `Arc<MemoryClient>` inside a `Mutex<Option<MemoryClientRef>>` (`MemoryState`), shared across RPC handlers.
The client is stored as `Arc<MemoryClient>` in a process-wide `OnceLock<Mutex<Option<MemoryClientRef>>>`
(`MEMORY_CLIENT_STATE` in `src/openhuman/memory/ops.rs`), shared across RPC handlers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/tinyhumansai-sdk.md` at line 390, Update the docs to reflect the current
runtime holder: replace the description that the client is stored in MemoryState
with the actual global holder MEMORY_CLIENT_STATE, and state its type/structure
as OnceLock<Mutex<Option<MemoryClientRef>>> (the active holder defined in
openhuman::memory::ops, referencing MEMORY_CLIENT_STATE and MemoryClientRef) so
the documentation matches the implementation.

@graycyrus graycyrus merged commit bd8d3ed into tinyhumansai:main Apr 3, 2026
9 checks passed
AusAgentSmith pushed a commit to AusAgentSmith/openhuman that referenced this pull request May 23, 2026
…humansai#204) (tinyhumansai#300)

* fix: patch whisper-rs-sys for Windows MSVC static CRT (/MT)

The upstream whisper-rs-sys builds whisper.cpp via CMake which
defaults to /MD (dynamic CRT), but Rust and all other C deps
use /MT (static CRT). This causes LNK2038/LNK1169 linker errors
on Windows.

Patch whisper-rs-sys from tinyhumansai/whisper-rs-sys fork which
adds config.static_crt(true) and overrides all per-config CMake
flags (Debug/Release/MinSizeRel/RelWithDebInfo) from /MD to /MT.

Closes tinyhumansai#273

* fix: clean up unused JWT token parameter in memory init

Memory is local-only (SQLite). The from_token() method accepted a JWT
but ignored it, always falling back to new_local(). Remove the dead
method, make jwt_token optional in MemoryInitRequest for backward
compat, and document the local-only design.

Closes tinyhumansai#204

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: sanil jain <jainsanil18@gmail.com>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature] Document or implement JWT-based remote memory init

3 participants