Openheim lib#22
Merged
Merged
Conversation
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis PR transforms Openheim from an internal CLI tool into an embeddable Rust library by refactoring ACP session operations into reusable methods and introducing a client facade with flexible configuration, session management, and RAG access patterns. Library documentation guides users through setup and common usage scenarios. ChangesLibrary Client API & Session Lifecycle Refactoring
Sequence DiagramssequenceDiagram
participant User
participant OpenheimBuilder
participant AgentState
participant RagContext
participant HistoryManager
User->>OpenheimBuilder: provider/api_key/model/mcp_server
OpenheimBuilder->>OpenheimBuilder: resolve defaults, load/merge config
OpenheimBuilder->>RagContext: initialize
OpenheimBuilder->>AgentState: construct with config
OpenheimBuilder->>User: return OpenheimClient
User->>OpenheimClient: new_session().model(m).skills(s).start()
OpenheimClient->>AgentState: acp_new_session
AgentState->>HistoryManager: create session
AgentState->>User: return SessionHandle
User->>SessionHandle: prompt(text, |update| callback)
SessionHandle->>AgentState: acp_prompt
AgentState->>HistoryManager: prepare conversation
AgentState->>AgentState: run_agent_streaming
AgentState->>User: emit SessionUpdate via callback
AgentState->>HistoryManager: save history
sequenceDiagram
participant User
participant OpenheimClient
participant HistoryManager
User->>OpenheimClient: load_session(session_id, |update| callback)
OpenheimClient->>HistoryManager: load conversation history
HistoryManager->>User: replay user/assistant messages as SessionUpdate
User->>OpenheimClient: return SessionHandle
User->>OpenheimClient: delete_session(session_id)
OpenheimClient->>HistoryManager: delete conversation file
HistoryManager->>User: return success
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The PR introduces substantial new public API surface (OpenheimClient, builder, session types) and refactors the ACP module with session lifecycle methods, but changes are cohesive and follow clear patterns. Review focuses on ACP refactoring correctness, client API ergonomics, configuration merging logic, and documentation completeness. Mixed complexity: new client facade is straightforward builder/delegation patterns, ACP refactoring involves streaming logic and session state management, and config/re-exports are lower density. Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/client.rs (3)
75-79: 💤 Low valueUUID parsing error loses specificity.
The UUID parse error is converted to a generic
Error::Otherwith message "invalid session id". For a library API, preserving the original parse error would help callers distinguish between malformed UUIDs and other failure modes.Consider wrapping the
uuid::Errorin a dedicatedError::InvalidSessionId(uuid::Error)variant.🤖 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/client.rs` around lines 75 - 79, The UUID parsing in get_session currently maps all parse failures to a generic crate::error::Error::Other which loses the original uuid::Error; change the error handling so Uuid::parse_str(session_id).map_err(|e| crate::error::Error::InvalidSessionId(e))? (or add a new Error::InvalidSessionId(uuid::Error) variant if it doesn't exist) so callers can distinguish malformed UUIDs from other failures, leaving the subsequent call to self.state.rag.history.load_conversation(&uuid) unchanged.
45-52: ⚡ Quick winCurrent directory fallback may be surprising.
Line 50 falls back to
"/"ifcurrent_dir()fails, which could happen in containerized environments or when the original directory is deleted. This default may be unexpected for library users and could cause sessions to be incorrectly grouped.Consider either:
- Documenting this fallback behavior clearly
- Requiring
cwdto be set explicitly (no default)- Using a sentinel value that's more obviously a fallback (e.g.,
PathBuf::from(""))🤖 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/client.rs` around lines 45 - 52, The new_session method currently sets SessionBuilder.cwd to std::env::current_dir().unwrap_or_else(|_| PathBuf::from("/")), which silently falls back to "/" on error; change this to a safer behavior: either (a) remove the implicit fallback and return a Result from new_session or require callers to set cwd (i.e., propagate the current_dir() error instead of unwrap_or_else), or (b) use a clear sentinel like PathBuf::from("") and document that SessionBuilder.cwd may be empty when current_dir() fails; update new_session (and the SessionBuilder type docs) and any call sites that assume a valid cwd accordingly (refer to new_session, SessionBuilder, and the cwd field).
304-352: ⚡ Quick winEmpty API key default may cause confusing errors.
Line 316 uses
api_key.unwrap_or_default(), which creates an empty string when no key is provided. This will cause authentication failures with cryptic error messages from the LLM provider rather than a clear "API key required" error at build time.Consider validating that
api_keyis present and non-empty when building programmatic config, or document that an empty key is allowed for providers that don't require authentication.🤖 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/client.rs` around lines 304 - 352, The build_programmatic function currently uses api_key.unwrap_or_default() which silently produces an empty string and leads to cryptic auth errors; change build_programmatic to validate the incoming api_key Option (ensure it's Some and not an empty string) and surface a clear error instead of defaulting: update the function signature to return Result<(AgentConfig, AppConfig), Error> (or an appropriate custom error), if api_key is required for the chosen provider return Err with a descriptive message like "API key required for provider <provider>" otherwise proceed to populate ProviderConfig and AgentConfig with the validated api_key; reference build_programmatic, ProviderConfig, and AgentConfig when making the change.src/acp/mod.rs (1)
163-168: ⚖️ Poor tradeoffConsider surfacing conversation save failures.
The conversation save failure is logged but execution continues successfully. In a library context, callers may want to know if history persistence failed so they can retry or alert the user.
Consider either:
- Returning the save error (breaking change to error semantics)
- Exposing a "partial success" indicator in the response
- Documenting this behavior clearly in the API docs
🤖 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/acp/mod.rs` around lines 163 - 168, The current code swallows errors from self.rag.history.save_conversation(&conversation) (logged only) so callers can't detect persistence failures; change the API to surface partial-save state by extending the function's return payload to include a "history_saved: bool" (or a richer enum like HistorySaveStatus) and set it to false when save_conversation returns Err(e) while still returning the existing run_result (use the existing run_result.map(|_| response_with_history_saved_flag)). Update the function's response type and any callers of this function accordingly so consumers can retry or alert when history_saved is false; keep the tracing::warn but also set history_saved=false when the save errors.
🤖 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/acp/mod.rs`:
- Around line 163-168: The current code swallows errors from
self.rag.history.save_conversation(&conversation) (logged only) so callers can't
detect persistence failures; change the API to surface partial-save state by
extending the function's return payload to include a "history_saved: bool" (or a
richer enum like HistorySaveStatus) and set it to false when save_conversation
returns Err(e) while still returning the existing run_result (use the existing
run_result.map(|_| response_with_history_saved_flag)). Update the function's
response type and any callers of this function accordingly so consumers can
retry or alert when history_saved is false; keep the tracing::warn but also set
history_saved=false when the save errors.
In `@src/client.rs`:
- Around line 75-79: The UUID parsing in get_session currently maps all parse
failures to a generic crate::error::Error::Other which loses the original
uuid::Error; change the error handling so
Uuid::parse_str(session_id).map_err(|e|
crate::error::Error::InvalidSessionId(e))? (or add a new
Error::InvalidSessionId(uuid::Error) variant if it doesn't exist) so callers can
distinguish malformed UUIDs from other failures, leaving the subsequent call to
self.state.rag.history.load_conversation(&uuid) unchanged.
- Around line 45-52: The new_session method currently sets SessionBuilder.cwd to
std::env::current_dir().unwrap_or_else(|_| PathBuf::from("/")), which silently
falls back to "/" on error; change this to a safer behavior: either (a) remove
the implicit fallback and return a Result from new_session or require callers to
set cwd (i.e., propagate the current_dir() error instead of unwrap_or_else), or
(b) use a clear sentinel like PathBuf::from("") and document that
SessionBuilder.cwd may be empty when current_dir() fails; update new_session
(and the SessionBuilder type docs) and any call sites that assume a valid cwd
accordingly (refer to new_session, SessionBuilder, and the cwd field).
- Around line 304-352: The build_programmatic function currently uses
api_key.unwrap_or_default() which silently produces an empty string and leads to
cryptic auth errors; change build_programmatic to validate the incoming api_key
Option (ensure it's Some and not an empty string) and surface a clear error
instead of defaulting: update the function signature to return
Result<(AgentConfig, AppConfig), Error> (or an appropriate custom error), if
api_key is required for the chosen provider return Err with a descriptive
message like "API key required for provider <provider>" otherwise proceed to
populate ProviderConfig and AgentConfig with the validated api_key; reference
build_programmatic, ProviderConfig, and AgentConfig when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 61f57dc4-5944-4749-9665-30fd71c0c0e2
📒 Files selected for processing (7)
Cargo.tomldocs/library.mdsrc/acp/mod.rssrc/client.rssrc/config/mod.rssrc/lib.rssrc/rag/history.rs
This was referenced May 23, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
New Features
Documentation