fix(core): harden skill loading and session persistence - #23
Conversation
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe agent core centralizes skill resolution, validates skill names, detects duplicate tools, converts assistant messages, persists events through one helper, and returns structured errors for invalid tool input, unsupported results, and storage failures. ChangesAgent core changes
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
microagents-core/src/agent.rs (1)
45-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename or drop
agent::GLOBAL_SKILLS_PATH
agent::GLOBAL_SKILLS_PATHis a static"~/.agents/skills"string whileskills::GLOBAL_SKILLS_PATHis the runtime-resolvedOnceLock<PathBuf>. Keeping both symbols with the same name and different types/semantics is confusing; useglobal_skills_path()where a real path is needed or rename the display literal.🤖 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 `@microagents-core/src/agent.rs` around lines 45 - 48, Remove or rename the static string constant agent::GLOBAL_SKILLS_PATH to avoid conflicting with skills::GLOBAL_SKILLS_PATH; update any references to use global_skills_path() when a resolved filesystem path is required, and retain a differently named display alias only if the literal is still needed.
🤖 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.
Inline comments:
In `@microagents-core/src/agent.rs`:
- Around line 322-366: Document the required builder call order in the method
comments for add_skill and find_skills: skills_path must be configured before
registering or discovering skills, because changing it afterward can make
existing registrations unresolvable. Keep the current implementation behavior
unchanged and clearly describe this ordering requirement near the affected
method documentation.
---
Nitpick comments:
In `@microagents-core/src/agent.rs`:
- Around line 45-48: Remove or rename the static string constant
agent::GLOBAL_SKILLS_PATH to avoid conflicting with skills::GLOBAL_SKILLS_PATH;
update any references to use global_skills_path() when a resolved filesystem
path is required, and retain a differently named display alias only if the
literal is still needed.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6f84a87b-4674-4f5b-b678-c83b1b1cfdf8
📒 Files selected for processing (2)
microagents-core/src/agent.rsmicroagents-core/src/skills.rs
AstraBert
left a comment
AstraBert
left a comment
There was a problem hiding this comment.
Hey @srimon12 , i like the DRY for event persistence and assistant message parts, I don't really like the complexity introduced by allowing users to specify a custom skills path.
In general, I would advise you to open an issue before creating a PR, so that I can give you an idea of what I would include and what I would not, before you spend too much time on it 🙏
| struct ConfiguredSkillsTool { | ||
| skills_path: PathBuf, | ||
| } | ||
|
|
There was a problem hiding this comment.
I think this is introducing more complexity than I would like: I don't see too much value in specifying a different path for the skill apart from the conventional .agents/skills (local and global), and, if anyone wants to expose skills that are under .claude/ or other paths I would simply suggest them to symlink
There was a problem hiding this comment.
i keep that deliberately because there was a API exposed but not connected internally.. removing it would be the wiser choice..
| async fn persist_event( | ||
| storage: &dyn AgentStorage, | ||
| event: &AgentEventAny, | ||
| ) -> Result<(), AgentError> { | ||
| storage | ||
| .update_session(event.clone()) | ||
| .await | ||
| .map_err(|error| { | ||
| AgentError::RunError(format!( | ||
| "An error occurred while updating the session in the storage: {error}" | ||
| )) | ||
| }) | ||
| } | ||
|
|
||
| fn assistant_message_parts(message: &Message) -> Result<Vec<AssistantMessagePart>, AgentError> { | ||
| message | ||
| .content | ||
| .iter() | ||
| .map(|part| match part { | ||
| MessagePart::Text(text) => Ok(AssistantMessagePart::Text(AssistantTextPart { | ||
| text: text.text.clone(), | ||
| })), | ||
| MessagePart::Thinking(thinking) => { | ||
| Ok(AssistantMessagePart::Thinking(AssistantThinkingPart { | ||
| thinking: thinking.thinking.clone(), | ||
| signature: thinking.signature.clone(), | ||
| })) | ||
| } | ||
| MessagePart::ToolCall(tool_call) => { | ||
| Ok(AssistantMessagePart::ToolCall(AssistantToolCallPart { | ||
| id: tool_call.id.clone(), | ||
| name: tool_call.name.clone(), | ||
| arguments: tool_call.arguments.clone(), | ||
| })) | ||
| } | ||
| _ => Err(AgentError::RunError( | ||
| "Assistant response contains an unsupported message part".to_string(), | ||
| )), | ||
| }) | ||
| .collect() | ||
| } | ||
|
|
|
Also, I will push a release with my previous changes before merging this, jfyi 👍 |
yea thats on me.. will push the changes tommorow.. |
|
No problem! I really appreciate the contributions by the way, thank you so much!!! 🙏 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
microagents-core/src/agent.rs (1)
873-878: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPersist a failed
SessionStopEventbefore returning these run errors.These branches run after session initialization and can run after turn events are persisted. They yield an error and return without a terminal event. A resumed session then has no recorded failure state for the turn.
microagents-core/src/agent.rs#L873-L878: PersistSessionStopEvent { success: false, ... }before returning an assistant-message conversion error.microagents-core/src/agent.rs#L924-L933: Persist the failed terminal event before returning an invalid tool-arguments error.microagents-core/src/agent.rs#L1048-L1053: Persist the failed terminal event before returning an unsupported tool-result error.🤖 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 `@microagents-core/src/agent.rs` around lines 873 - 878, Before each early return for a run error, persist a failed SessionStopEvent with success: false and the relevant error details: update the assistant-message conversion branch around microagents-core/src/agent.rs lines 873-878, the invalid tool-arguments branch at lines 924-933, and the unsupported tool-result branch at lines 1048-1053, then yield the error and return as before.
🧹 Nitpick comments (1)
microagents-core/src/common.rs (1)
274-291: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the remaining conversion branches.
This test covers only
MessagePart::Text. Add tests forMessagePart::ThinkingandMessagePart::ToolCall, includingsignature,id,name, andarguments. This protects the persistedAssistantMessagePartcontract.🤖 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 `@microagents-core/src/common.rs` around lines 274 - 291, Add tests alongside test_convert_message_to_assistant_parts_converts_text for the MessagePart::Thinking and MessagePart::ToolCall conversion branches. Assert the resulting AssistantMessagePart preserves Thinking.signature and ToolCall.id, name, and arguments exactly, covering the persisted conversion contract.
🤖 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.
Outside diff comments:
In `@microagents-core/src/agent.rs`:
- Around line 873-878: Before each early return for a run error, persist a
failed SessionStopEvent with success: false and the relevant error details:
update the assistant-message conversion branch around
microagents-core/src/agent.rs lines 873-878, the invalid tool-arguments branch
at lines 924-933, and the unsupported tool-result branch at lines 1048-1053,
then yield the error and return as before.
---
Nitpick comments:
In `@microagents-core/src/common.rs`:
- Around line 274-291: Add tests alongside
test_convert_message_to_assistant_parts_converts_text for the
MessagePart::Thinking and MessagePart::ToolCall conversion branches. Assert the
resulting AssistantMessagePart preserves Thinking.signature and ToolCall.id,
name, and arguments exactly, covering the persisted conversion contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0e87b68d-eede-4405-89da-893cd5533af3
📒 Files selected for processing (3)
microagents-core/src/agent.rsmicroagents-core/src/common.rsmicroagents-core/src/skills.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- microagents-core/src/skills.rs
|
@AstraBert i know you are busy with the transition.. still once you get time check the changes |
|
Hey, I decided to move this project to lightagent as I feel like microagents is pretty bloated and lost the original focus, and I really want to build an agent that I can use and scale with my codebase and not be blocked at the vector store stage forever. Thanks for your contributions, really appreciated 🙏 |
2 participants
Summary
This PR makes skill resolution configurable per agent, fixes a broken explicit
skill-loading path, and removes several failure-prone patterns from the core
agent runtime.
Skill resolution
Per-agent project skill directory
Adds:
The configured directory replaces the default project-local
.agents/skillslookup path for that agent only. It remains higher prioritythan the global
~/.agents/skillsdirectory.The configured path is now used consistently by:
MicroAgentBuilder::add_skillMicroAgentBuilder::find_skills/skill-name)skillstoolExisting public path constants remain available.
agent::SKILLS_PATHnowuses the canonical definition from
skills.Traversal protection
Skill names are now validated before being joined to a skills directory.
The resolver rejects:
.../\This prevents model-provided tool input such as
../outside-skillfromescaping a configured project skills directory.
Invalid names are handled according to their caller:
add_skill("../outside")returnsMicroAgentBuilderError::InvalidSkillNameskillstool returns an errorToolResultNoneExplicit skill loading bug fix
MicroAgentBuilder::add_skillpreviously passed the skill directory toparse_skill, even though the parser expects theSKILL.mdfile. It nowparses:
Agent runtime cleanup
storage.update_sessionerror mapping intopersist_event.expecton model-provided tool-call JSON with a streamedAgentError::RunError.unreachable!()branches for non-exhaustive message/result enumswith explicit run errors.
Option::take()for completed assistant messages, avoiding stalemessage reuse between tool-call iterations.
ToolAlreadyDefinedbuilder error instead ofsilently overwriting a tool with the same name.
Test reliability
Skill tests no longer change the process current working directory or depend
on the developer's global skills directory. Tests now inject explicit local
and global paths, making them safe to run in parallel and independent of
local machine state.
Validation
Passed:
Test results:
Summary by CodeRabbit
New Features
Bug Fixes
Reliability