Skip to content

feat(agent): simplify harness + trim bundled prompts#500

Merged
senamakel merged 19 commits intotinyhumansai:mainfrom
senamakel:feat/simplify-agents
Apr 11, 2026
Merged

feat(agent): simplify harness + trim bundled prompts#500
senamakel merged 19 commits intotinyhumansai:mainfrom
senamakel:feat/simplify-agents

Conversation

@senamakel
Copy link
Copy Markdown
Member

@senamakel senamakel commented Apr 11, 2026

Summary

Multi-commit work-in-progress branch that restructures the agent harness and
trims the bundled prompt set. Opening as a draft for visibility — the tip
commit (34e4870a Implement agent delegation tools and browser automation features) is an in-progress refactor that moves most tool implementations
under a new src/openhuman/tools/impl/ layout; src/openhuman/tools/mod.rs
still declares the old module names, so cargo check currently fails on the
core crate. Do not merge until that's resolved.

Highlights across the 16 commits on this branch:

  • Agent harness refactor — new src/openhuman/agent/harness/ module with
    builder, runtime, turn management, and session memory; removes legacy REPL,
    loop_, classifier, traits, cost-tracking, and identity modules; introduces
    built-in agent definitions under src/openhuman/agent/agents/* (orchestrator,
    planner, code executor, skills agent, researcher, critic, archivist,
    tool maker) with per-agent prompt.md + agent.toml.
  • Pure orchestrator pattern — orchestrator prompt is loaded via
    ArchetypePromptSection; main agent uses visible_tool_names to filter the
    tool catalog and skip sections that only matter to sub-agents.
  • Prompt cleanup (9f2ab79 + 7b41d55) — deletes six bundled prompt
    files (AGENTS.md, TOOLS.md, MEMORY.md, BOOTSTRAP.md,
    CONSCIOUS_LOOP.md, README.md) that were either duplicates, stale
    hand-rolled tool catalogs, integration quirks (Gmail/Notion/Slack rate
    limits) that belong in skill adapter code, or channel-specific onboarding
    scripts. Trims SOUL.md from ~259 → ~35 lines by removing the duplicated
    Telegram reaction protocol (already injected dynamically via
    channels/runtime/dispatch.rs as dispatcher_instructions), duplicated
    safety rules (already in SafetySection), and brochure-style content
    (Games You Know, Knowledge Areas, Emergency Responses, etc.).
    Updates prompt.rs, workspace/ops.rs, and channels/prompt.rs +
    tests accordingly. Workspace MEMORY.md is kept as an optional
    archivist-written file (silent skip when absent instead of a
    `[File not found]` marker).
  • In-progress tool refactor (34e4870) — starts moving
    src/openhuman/tools/*.rs into src/openhuman/tools/impl/{...}/ subtrees.
    Incomplete: tools/mod.rs still references the old flat paths, so the
    core crate does not currently build.

Test plan

  • Resolve the tools/mod.rs module declarations to point at the new
    tools/impl/ layout
  • cargo check --manifest-path Cargo.toml passes (core crate)
  • cargo test -p openhuman — agent harness + channel prompt tests
  • cargo check --manifest-path app/src-tauri/Cargo.toml (Tauri shell)
  • yarn typecheck / yarn lint in app/
  • Smoke-test orchestrator flow: sub-agent spawn, tool dispatch, memory
    recall injection
  • Verify Telegram channel still emits reaction markers correctly (the
    prompt instructions now come only from the channel dispatcher, not
    SOUL.md)
  • Confirm fresh-install workspace seeding writes the trimmed file set
    (`SOUL.md`, `IDENTITY.md`, `USER.md`) and no stale
    `AGENTS.md`/`TOOLS.md`/`MEMORY.md`/`BOOTSTRAP.md`

Summary by CodeRabbit

  • Removed Features

    • Interactive REPL/shell removed
    • Multi-agent orchestration and DAG-based planning removed
    • AIEOS identity format and automated history compaction removed
    • Token cost tracking removed
  • New Features

    • Built-in agents now load from configuration files
    • Added agents: Archivist, Code Executor, Critic, Planner, Researcher, Skills Agent, Tool Maker, Orchestrator
  • Architecture Changes

    • Simplified agent execution model and consolidated tool modules

… code executor, skills agent, researcher, critic, archivist, and tool maker

- Introduced a new module for built-in agent definitions, allowing for easy addition of agents through a structured format.
- Created TOML configuration files for each agent, detailing their properties such as id, display name, usage context, and tool specifications.
- Developed corresponding prompt files that outline the responsibilities and rules for each agent, enhancing their functionality and usability.
- Implemented a loading function to parse these definitions into usable agent structures, ensuring all agents are correctly initialized and validated.
…ent harness

- Deleted the `archetypes.rs`, `dag.rs`, `executor.rs`, and `types.rs` files, which contained the definitions and implementations for agent archetypes, task DAGs, and execution logic.
- Updated the `builtin_definitions.rs` and `definition.rs` files to remove references to the now-removed archetypes, streamlining the agent definition process.
- Refactored the `mod.rs` file to eliminate unused modules and clarify the structure of the multi-agent harness.
- Adjusted comments and documentation to reflect the removal of the DAG orchestration flow, emphasizing the current sub-agent delegation model.
… module

- Introduced `ContextGuard` to monitor context utilization and trigger auto-compaction when usage exceeds defined thresholds.
- Added `scrub_credentials` function to sanitize sensitive information from tool outputs.
- Implemented history management functions to trim conversation history and build compaction transcripts.
- Created `build_tool_instructions` to generate tool usage guidelines for the system prompt.
- Developed `memory_context` functions to manage user memory and relevant context for conversations.
- Enhanced the `tool_loop` to integrate the new context management and tool invocation logic.

This update improves the agent's ability to manage context effectively, ensuring efficient memory usage and secure handling of sensitive data.
- Updated import paths in `dispatcher.rs`, `memory_loader.rs`, `mod.rs`, `dispatch.rs`, and `startup.rs` to replace references from the `loop_` module to the `harness` module.
- This change enhances module organization and aligns with the recent architectural updates in the agent structure.
…cture

- Deleted `cost.rs` and `context_assembly.rs` files, which handled token cost tracking and context assembly for the multi-agent harness, respectively.
- Updated `mod.rs` files to remove references to the deleted modules, streamlining the agent's organization and focusing on essential components.
- This cleanup enhances maintainability and aligns with the current architectural direction of the project.
…cture

- Deleted the `identity.rs` file, which contained the AIEOS identity handling logic and related structures.
- Updated `mod.rs` and other files to remove references to the deleted identity module, streamlining the agent's organization.
- This change simplifies the codebase and aligns with the decision to rely solely on OpenClaw markdown files for identity management.
- Added spacing for better readability in the prompts of the Archivist, Code Executor, Critic, Orchestrator, Researcher, Skills Agent, and Tool Maker agents.
- Updated the table formatting in the Orchestrator prompt for consistency and clarity.
- These changes improve the overall presentation and usability of agent documentation, making it easier for users to understand agent responsibilities and rules.
…ture

- Deleted the `repl.rs` file, which contained the implementation for the interactive REPL.
- Removed references to the REPL in `cli.rs`, `mod.rs`, and other related files, streamlining the agent's organization.
- Eliminated unused REPL session handling logic from the agent schemas and local AI operations, enhancing maintainability and focusing on essential components.
- This cleanup aligns with the current architectural direction of the project, simplifying the codebase.
- Updated import paths in `startup.rs` to include `host_runtime` from the correct module.
- Streamlined the export statements in `mod.rs` and `schema/mod.rs` for better organization and clarity.
- Removed unnecessary line breaks in the export lists to enhance readability and maintainability of the configuration schema.
…exports

- Deleted the `history.rs` and `session.rs` files, which contained functions for managing conversation history and session handling.
- Updated `mod.rs` to remove references to the deleted modules and streamline the agent's organization.
- Cleaned up import statements in `mod.rs` and other files to enhance clarity and maintainability of the codebase.
- Introduced a new `harness` module for the `Agent`, encapsulating the `AgentBuilder`, runtime accessors, and turn lifecycle management.
- Implemented the `AgentBuilder` fluent API for constructing `Agent` instances with customizable configurations.
- Developed the `turn` method to handle user interactions, tool execution, and context management within the agent.
- Created a `runtime` module for public accessors and utility functions related to agent operations.
- Added comprehensive unit and integration tests to ensure the functionality of the new agent structure and its components.
- Deleted the `classifier.rs` and `traits.rs` files, which contained the classification logic and core agent traits, respectively.
- Updated `mod.rs` to remove references to the deleted modules, streamlining the agent's organization.
- This cleanup enhances maintainability and focuses on essential components of the agent architecture.
- Updated the `prompt.rs` file to streamline the orchestrator's logic by removing unnecessary tool documentation references and simplifying the file list.
- Deleted several prompt files (`AGENTS.md`, `BOOTSTRAP.md`, `CONSCIOUS_LOOP.md`, `MEMORY.md`, `README.md`, `TOOLS.md`) to declutter the project and focus on essential components.
- Adjusted the `harness/mod.rs` file to change module visibility from public to crate-level for better encapsulation.
- These changes enhance maintainability and align with the current architectural direction of the project.
- Simplified the list of bundled prompt files in `prompt.rs` by removing references to `AGENTS.md` and `TOOLS.md`, focusing on essential identity files.
- Adjusted the logic for injecting `MEMORY.md` to be optional, ensuring it only appears if it exists.
- Updated test cases in `common.rs`, `identity.rs`, and `prompt.rs` to reflect the changes in the bootstrap file structure and ensure accurate validation of the new logic.
- These modifications enhance clarity and maintainability of the codebase while aligning with the current project architecture.
- Introduced new tools for agent delegation, including `ArchetypeDelegationTool`, `AskClarificationTool`, `DelegateTool`, and `SkillDelegationTool`, enhancing the agent's ability to manage tasks through specialized sub-agents.
- Added `SpawnSubagentTool` for delegating tasks to sub-agents, allowing for more complex workflows and improved task management.
- Implemented browser automation capabilities with `BrowserOpenTool`, enabling secure opening of approved URLs in the Brave Browser.
- Enhanced the `BrowserTool` with pluggable backends for improved automation and user interaction.
- Added `ImageInfoTool` for extracting metadata from images, supporting future multimodal capabilities.
- Organized tools into a new `impl` module structure for better maintainability and clarity in the codebase.
- Updated import statements across various tool implementations to ensure consistency and clarity, particularly in the `traits.rs` file.
- Removed unnecessary line breaks and adjusted module paths for better organization.
- Cleaned up test files by removing trailing whitespace, enhancing code readability and maintainability.
- These changes streamline the codebase and improve the overall structure of tool-related components.
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 11, 2026

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 304d40f4-cba1-47c5-a54b-d915634fc62b

📥 Commits

Reviewing files that changed from the base of the PR and between 856297b and d6c9aa8.

📒 Files selected for processing (3)
  • src/openhuman/agent/harness/builtin_definitions.rs
  • src/openhuman/agent/harness/definition_loader.rs
  • src/openhuman/channels/prompt.rs

📝 Walkthrough

Walkthrough

This PR removes the interactive REPL and AIEOS identity subsystem, deletes multi-agent orchestration (archetypes/DAG/executor and related types), and reorganizes agents into TOML+prompt built-ins with a new harness/session Agent API; it also centralizes tool implementations under an impl/ hierarchy and updates many import paths.

Changes

Cohort / File(s) Summary
REPL & CLI
Cargo.toml, docs/design-repl.md, src/core/cli.rs, src/core/repl.rs, src/core/mod.rs
Removed rustyline dependency, deleted REPL design doc and REPL implementation, removed REPL CLI subcommand and helper re-export.
Agent public surface & harness
src/openhuman/agent/mod.rs, src/openhuman/agent/agent/mod.rs, src/openhuman/agent/harness/...
Replaced old agent/loop_ exports with harness::session::{Agent,AgentBuilder}; moved/rewired harness internals, reduced public re-exports, added session module and crate-visible helper exports.
Built-in agent configs
src/openhuman/agent/agents/mod.rs, src/openhuman/agent/agents/*/agent.toml, src/openhuman/agent/agents/*/prompt.md
Added BUILTINS list and load_builtins(); introduced 8 TOML-based built-in agent definitions and associated prompts; loader injects prompt.md as inline PromptSource.
Removed orchestration & DAG
src/openhuman/agent/harness/{archetypes,dag,executor,types}.rs, src/openhuman/agent/harness/context_assembly.rs
Deleted AgentArchetype, DAG/task planning, executor orchestration, and related types and helpers.
Agent loop & session removal
src/openhuman/agent/loop_/{mod,session,history,...}
Removed the old agent loop, session runner, history compaction, and related modules; many functions (run/process_message/auto_compact_history) deleted.
Identity removal & prompt simplification
src/openhuman/agent/identity.rs, src/openhuman/agent/prompts/*, src/openhuman/channels/prompt.rs, src/openhuman/config/schema/*
Removed AIEOS identity subsystem and IdentityConfig; simplified workspace prompt seeding to SOUL.md, IDENTITY.md, USER.md (MEMORY.md now conditional); condensed SOUL.md and removed several prompt files.
Removed utility modules
src/openhuman/agent/classifier.rs, src/openhuman/agent/cost.rs, src/openhuman/agent/traits.rs, src/openhuman/agent/loop_/types.rs
Deleted classifier, cost tracker, and core agent trait module (Provider/Tool/Memory/RuntimeAdapter), plus many tests.
Tools reorganization
src/openhuman/tools/impl/mod.rs, src/openhuman/tools/mod.rs, src/openhuman/tools/impl/...
Consolidated tool implementations under impl/ with new module entrypoints and re-exports; updated ~50+ tool files to import traits from crate::openhuman::tools::traits. Added new agent-delegation tools and centralized dispatch_subagent.
Sub-agent delegation & dispatch
src/openhuman/tools/impl/agent/mod.rs, src/openhuman/tools/impl/agent/{archetype_delegation,skill_delegation}.rs
Added dispatch_subagent() and new tools (ArchetypeDelegationTool, SkillDelegationTool) that call run_subagent via harness APIs and publish DomainEvents on spawn/completion/failure.
Channels, runtime, cron, workspace
src/openhuman/channels/*, src/openhuman/cron/scheduler.rs, src/openhuman/workspace/ops.rs
Removed identity param from build_system_prompt; updated imports to harness helpers; cron now builds Agent via Agent::from_config and calls run_single; workspace bootstrap files reduced to three.
Misc docs & loader validation
src/openhuman/agent/harness/definition_loader.rs, src/core/jsonrpc.rs, src/openhuman/agent/harness/definition.rs
Loader now rejects definitions with empty inline system_prompt; doc comments updated and AgentDefinition.system_prompt has serde default placeholder.

Sequence Diagram(s)

sequenceDiagram
    participant Tool as Tool Caller
    participant Reg as AgentDefinitionRegistry
    participant Runner as harness::subagent_runner
    participant Events as DomainEventPublisher

    Tool->>Reg: lookup(agent_id)
    alt not found
        Reg-->>Tool: error (agent missing)
    else found
        Tool->>Events: publish(SubagentSpawned)
        Tool->>Runner: run_subagent(def, options)
        alt success
            Runner-->>Events: publish(SubagentCompleted)
            Runner-->>Tool: ToolResult::success(output)
        else failure
            Runner-->>Events: publish(SubagentFailed)
            Runner-->>Tool: ToolResult::error(err)
        end
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Possibly related issues

Possibly related PRs

Poem

🐰 I tidy the burrow, tuck old REPLs away,

TOML blooms of agents greet the light of day,
Tools march from one house, neat and well-defined,
Prompts trimmed to purpose, and imports aligned,
Hop—simpler paths, clearer minds at play.

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

@senamakel senamakel marked this pull request as ready for review April 11, 2026 03:52
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: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/openhuman/tools/orchestrator_tools.rs (1)

34-43: ⚠️ Potential issue | 🟠 Major

Skill ID extraction is incorrect for skill__tool keys.

The code says it extracts skill IDs from names like notion__search, but it currently uses the whole key unchanged. That can generate invalid skill_ids for delegation and break sub-agent routing.

💡 Proposed fix
-        let mut skill_ids: Vec<String> = all_skill_tools
-            .iter()
-            .filter_map(|(skill_id, _)| {
-                if seen_names.insert(skill_id.clone()) {
-                    Some(skill_id.clone())
-                } else {
-                    None
-                }
-            })
-            .collect();
+        let mut skill_ids: Vec<String> = all_skill_tools
+            .iter()
+            .filter_map(|(tool_name, _)| {
+                let skill_id = tool_name
+                    .split_once("__")
+                    .map(|(id, _)| id)
+                    .unwrap_or(tool_name)
+                    .to_string();
+                if seen_names.insert(skill_id.clone()) {
+                    Some(skill_id)
+                } else {
+                    None
+                }
+            })
+            .collect();
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/tools/orchestrator_tools.rs` around lines 34 - 43, The
extraction of skill IDs is wrong: when building skill_ids from all_skill_tools
you must strip tool suffixes like "__search" by splitting each key on the
double-underscore and using the left-hand part as the skill id; update the
closure that currently uses skill_id.clone() to instead compute let base =
skill_id.splitn(2, "__").next().unwrap_or(&skill_id).to_string(), use that base
for seen_names and to push into skill_ids, keeping the dedup logic with
seen_names unchanged (references: all_skill_tools, seen_names, skill_ids).
🧹 Nitpick comments (8)
src/openhuman/channels/prompt.rs (1)

174-178: Unreachable fallback — consider removing.

The condition prompt.is_empty() can never be true at this point because earlier sections (lines 14-16, 102-109, 139-143, 151-153, 155-162, 165-172) unconditionally push content to the prompt. This fallback is dead code.

♻️ Proposed simplification
-    if prompt.is_empty() {
-        "You are OpenHuman, a fast and efficient AI assistant built in Rust. Be helpful, concise, and direct.".to_string()
-    } else {
-        prompt
-    }
+    prompt
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/channels/prompt.rs` around lines 174 - 178, The fallback branch
that returns a hardcoded string when prompt.is_empty() is unreachable and should
be removed; replace the conditional return with just returning the existing
prompt variable (or ensure the default is injected earlier). Update the code
around the prompt construction/return (the block referencing prompt and the
conditional using prompt.is_empty()) to simply return prompt, removing the dead
code path and any related unreachable default logic.
src/openhuman/tools/impl/mod.rs (1)

10-17: Glob re-exports may cause name collisions.

Using pub use <submodule>::* for all eight submodules works if each exports uniquely-named items. If any two submodules export the same identifier (e.g., a hypothetical Tool type), compilation will fail with an ambiguity error. This is acceptable if the naming convention ensures uniqueness (which appears to be the case with *Tool suffix pattern).

Consider documenting the naming convention or switching to explicit re-exports if collisions become an issue in the future.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/tools/impl/mod.rs` around lines 10 - 17, The current glob
re-exports (pub use agent::*; pub use browser::*; pub use cron::*; pub use
filesystem::*; pub use hardware::*; pub use memory::*; pub use network::*; pub
use system::*;) risk name collisions if two submodules export the same
identifier; either replace these globs with explicit re-exports of the public
types/functions you want to expose (e.g., list each Tool type or function from
agent, browser, cron, filesystem, hardware, memory, network, system) or add a
clear module-level comment documenting the required naming convention (e.g., all
public tool types must use a unique *Tool suffix) to prevent future ambiguity
and compilation errors.
src/openhuman/agent/harness/definition.rs (1)

291-297: Avoid fail-open empty prompts for custom TOMLs.

Defaulting omitted system_prompt to an empty inline prompt is useful for built-ins, but it can silently create under-specified custom agents. Consider validating that non-builtin definitions cannot keep this empty placeholder.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/agent/harness/definition.rs` around lines 291 - 297, The
current helper empty_inline_prompt() returns PromptSource::Inline("") which lets
custom TOMLs silently omit system_prompt; add validation so non-builtin agent
definitions cannot keep this empty placeholder: keep empty_inline_prompt() but
update the loader/validation path (e.g., AgentDefinition::validate or the code
that constructs AgentDefinition after parsing) to detect PromptSource::Inline(s)
where s.is_empty() and the definition is not a built-in, and return an error (or
fail the parse) describing the missing system_prompt; reference
PromptSource::Inline, empty_inline_prompt(), and AgentDefinition::system_prompt
to find where to add this check.
src/openhuman/agent/agents/mod.rs (2)

1-93: Consider moving operational code to a sibling file per coding guidelines.

Per coding guidelines, mod.rs files should be "light and export-focused" with operational code in sibling files. This module contains ~30 lines of loader logic (load_builtins, parse_builtin). Consider moving these to loader.rs and re-exporting from mod.rs.

That said, this is a minor concern given the loader is fundamental to the module's purpose and the code is compact. The current structure is acceptable for a WIP branch.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/agent/agents/mod.rs` around lines 1 - 93, The reviewer suggests
moving operational loader code out of mod.rs into a sibling loader.rs; to fix,
create a new sibling module file (loader.rs) and move the implementations of
load_builtins and parse_builtin (and any helper logic that iterates BUILTINS or
constructs AgentDefinition) into it, keeping BUILTINS and the BuiltinAgent
struct in mod.rs; then in mod.rs add a pub mod loader; and re-export the
functions with pub use loader::{load_builtins, parse_builtin}; so callers can
continue to reference load_builtins/parse_builtin while mod.rs remains
export-focused.

170-176: Minor: find() helper reloads all builtins on each call.

The test helper calls load_builtins() on every invocation. For the current test count this is fine, but if more tests are added, consider using a LazyLock<Vec<AgentDefinition>> to cache the parsed definitions.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/agent/agents/mod.rs` around lines 170 - 176, The helper find()
currently calls load_builtins() on every invocation causing repeated parsing;
replace that with a cached static LazyLock<Vec<AgentDefinition>> (e.g. static
BUILTINS: LazyLock<Vec<AgentDefinition>>) initialized by calling
load_builtins().unwrap(), then have find(id: &str) look up the id in
BUILTINS.iter() (instead of load_builtins()), preserving the existing
unwrap_or_else panic behavior; update references to AgentDefinition,
load_builtins, and find accordingly and remove repeated unwraps where the static
initialization already handles errors.
src/openhuman/agent/harness/builtin_definitions.rs (1)

74-79: Consider extracting the expected count to reduce test coupling.

The hardcoded 9 in the assertion (and the comment "8 built-in agents + 1 synthetic fork") couples this test to the exact count in BUILTINS. If a new agent is added, both this file and agents/mod.rs need updating. Consider deriving the count:

assert_eq!(defs.len(), crate::openhuman::agent::agents::BUILTINS.len() + 1);

This is a minor maintainability improvement — the current approach is acceptable given the explicit ID check in expected_builtin_ids_are_present.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/agent/harness/builtin_definitions.rs` around lines 74 - 79,
Replace the hardcoded expected count in the test function
all_definitions_present by deriving it from the actual BUILTINS collection so
the test doesn't need manual updates; use defs.len() compared to
crate::openhuman::agent::agents::BUILTINS.len() + 1 (the +1 for the synthetic
"fork") and update the assert_eq! to use that expression, keeping the defs
variable and intent unchanged.
src/openhuman/tools/impl/agent/mod.rs (2)

127-137: Hardcoded skill descriptions may drift from actual integrations.

The skill_description function hardcodes descriptions for known integration IDs. If new integrations are added or existing ones change, this function needs manual updates. Consider documenting this coupling or deriving descriptions from a single source of truth.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/tools/impl/agent/mod.rs` around lines 127 - 137, The
skill_description function currently hardcodes descriptions and will drift as
integrations change; update skill_description to read descriptions from a single
source of truth (e.g., an Integration metadata field or a central
INTEGRATION_DESCRIPTIONS map/registry) instead of embedding strings inline: add
or use an existing Integration::description or IntegrationRegistry lookup,
return that description when present, and fall back to the current
format!("Interact with the {skill_id} integration.") only if no metadata exists;
ensure the symbol skill_description is updated to query the new registry/field
and remove the duplicated hardcoded literals so descriptions stay in sync.

36-125: Consider extracting dispatch_subagent to a sibling file.

Per coding guidelines, mod.rs should be "light and export-focused." This file contains ~90 lines of operational code in dispatch_subagent. Consider moving it to dispatch.rs and re-exporting, keeping mod.rs focused on module declarations and re-exports.

The implementation itself is solid — proper error handling, event publishing for observability, and clean separation of concerns.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/tools/impl/agent/mod.rs` around lines 36 - 125, Move the heavy
implementation of dispatch_subagent into a new sibling file (dispatch.rs):
create dispatch.rs, copy the pub(crate) async fn dispatch_subagent and all local
uses (AgentDefinitionRegistry::global, run_subagent, SubagentRunOptions,
publish_global, DomainEvent, ToolResult, current_parent, uuid, log) and
necessary use/imports into it, keep the function signature and behavior
unchanged, then in mod.rs replace the body with a lightweight re-export like
pub(crate) use self::dispatch::dispatch_subagent; (and ensure mod dispatch; is
declared). Preserve visibility, comments, and any tests or helper functions used
by dispatch_subagent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/openhuman/agent/agents/archivist/agent.toml`:
- Around line 6-11: The safety preamble is currently disabled for the
write-capable background agent (omit_safety_preamble = true); re-enable it by
setting omit_safety_preamble = false (or removing that key) in the agent TOML
configuration so the Archivist agent includes the safety preamble before write
operations; also apply the same change where omit_safety_preamble appears
elsewhere in this config (the second occurrence mentioned) to ensure consistent
hardening across the agent.

In `@src/openhuman/agent/harness/definition.rs`:
- Around line 58-65: Update the doc comment on the sub-agent prompt source (the
field documented above #[serde(default = "defaults::empty_inline_prompt")]) to
reflect that built-in agents now receive their prompt as loader-injected inline
content rather than by file-path; remove or rephrase the sentence claiming
built-ins use a path under `agent/prompts/` and instead state that the loader
injects rendered inline prompt bodies for in-tree built-ins (see
crate::openhuman::agent::agents and defaults::empty_inline_prompt for the loader
behavior).

In `@src/openhuman/channels/tests/identity.rs`:
- Around line 30-34: The test currently unconditionally asserts that
prompt.contains("User likes Rust"), which enforces MEMORY.md presence; change
the test to cover both cases: add a second test case that constructs the same
context without a MEMORY.md file and assert that prompt does NOT contain the
memory string, or make the existing assertion conditional by checking whether
MEMORY.md was provided (e.g., inspect the test setup flag or Option used to load
memory) before asserting prompt.contains("User likes Rust"). Update the test(s)
around the prompt assertion (the call site using prompt.contains("User likes
Rust")) to either add the companion case or guard the assertion so the behavior
matches the documented "optional" MEMORY.md.

In `@src/openhuman/cron/scheduler.rs`:
- Around line 154-171: The match arm for job.session_target currently treats
SessionTarget::Main and SessionTarget::Isolated identically (both create a fresh
Agent via Agent::from_config, call agent.set_event_context with
format!("cron:{}", job.id), and run_single), which either makes the Main variant
redundant or misses intended shared-session behavior; either remove the Main
variant from SessionTarget (and update schema/consumers) if they should be
identical, or implement a separate branch for SessionTarget::Main that reuses or
looks up a shared session (e.g., fetch existing agent/session by id or use a
shared Agent pool) instead of calling Agent::from_config, ensuring you still
set_event_context and call run_single for Isolated while documenting the
semantics change.

---

Outside diff comments:
In `@src/openhuman/tools/orchestrator_tools.rs`:
- Around line 34-43: The extraction of skill IDs is wrong: when building
skill_ids from all_skill_tools you must strip tool suffixes like "__search" by
splitting each key on the double-underscore and using the left-hand part as the
skill id; update the closure that currently uses skill_id.clone() to instead
compute let base = skill_id.splitn(2,
"__").next().unwrap_or(&skill_id).to_string(), use that base for seen_names and
to push into skill_ids, keeping the dedup logic with seen_names unchanged
(references: all_skill_tools, seen_names, skill_ids).

---

Nitpick comments:
In `@src/openhuman/agent/agents/mod.rs`:
- Around line 1-93: The reviewer suggests moving operational loader code out of
mod.rs into a sibling loader.rs; to fix, create a new sibling module file
(loader.rs) and move the implementations of load_builtins and parse_builtin (and
any helper logic that iterates BUILTINS or constructs AgentDefinition) into it,
keeping BUILTINS and the BuiltinAgent struct in mod.rs; then in mod.rs add a pub
mod loader; and re-export the functions with pub use loader::{load_builtins,
parse_builtin}; so callers can continue to reference load_builtins/parse_builtin
while mod.rs remains export-focused.
- Around line 170-176: The helper find() currently calls load_builtins() on
every invocation causing repeated parsing; replace that with a cached static
LazyLock<Vec<AgentDefinition>> (e.g. static BUILTINS:
LazyLock<Vec<AgentDefinition>>) initialized by calling load_builtins().unwrap(),
then have find(id: &str) look up the id in BUILTINS.iter() (instead of
load_builtins()), preserving the existing unwrap_or_else panic behavior; update
references to AgentDefinition, load_builtins, and find accordingly and remove
repeated unwraps where the static initialization already handles errors.

In `@src/openhuman/agent/harness/builtin_definitions.rs`:
- Around line 74-79: Replace the hardcoded expected count in the test function
all_definitions_present by deriving it from the actual BUILTINS collection so
the test doesn't need manual updates; use defs.len() compared to
crate::openhuman::agent::agents::BUILTINS.len() + 1 (the +1 for the synthetic
"fork") and update the assert_eq! to use that expression, keeping the defs
variable and intent unchanged.

In `@src/openhuman/agent/harness/definition.rs`:
- Around line 291-297: The current helper empty_inline_prompt() returns
PromptSource::Inline("") which lets custom TOMLs silently omit system_prompt;
add validation so non-builtin agent definitions cannot keep this empty
placeholder: keep empty_inline_prompt() but update the loader/validation path
(e.g., AgentDefinition::validate or the code that constructs AgentDefinition
after parsing) to detect PromptSource::Inline(s) where s.is_empty() and the
definition is not a built-in, and return an error (or fail the parse) describing
the missing system_prompt; reference PromptSource::Inline,
empty_inline_prompt(), and AgentDefinition::system_prompt to find where to add
this check.

In `@src/openhuman/channels/prompt.rs`:
- Around line 174-178: The fallback branch that returns a hardcoded string when
prompt.is_empty() is unreachable and should be removed; replace the conditional
return with just returning the existing prompt variable (or ensure the default
is injected earlier). Update the code around the prompt construction/return (the
block referencing prompt and the conditional using prompt.is_empty()) to simply
return prompt, removing the dead code path and any related unreachable default
logic.

In `@src/openhuman/tools/impl/agent/mod.rs`:
- Around line 127-137: The skill_description function currently hardcodes
descriptions and will drift as integrations change; update skill_description to
read descriptions from a single source of truth (e.g., an Integration metadata
field or a central INTEGRATION_DESCRIPTIONS map/registry) instead of embedding
strings inline: add or use an existing Integration::description or
IntegrationRegistry lookup, return that description when present, and fall back
to the current format!("Interact with the {skill_id} integration.") only if no
metadata exists; ensure the symbol skill_description is updated to query the new
registry/field and remove the duplicated hardcoded literals so descriptions stay
in sync.
- Around line 36-125: Move the heavy implementation of dispatch_subagent into a
new sibling file (dispatch.rs): create dispatch.rs, copy the pub(crate) async fn
dispatch_subagent and all local uses (AgentDefinitionRegistry::global,
run_subagent, SubagentRunOptions, publish_global, DomainEvent, ToolResult,
current_parent, uuid, log) and necessary use/imports into it, keep the function
signature and behavior unchanged, then in mod.rs replace the body with a
lightweight re-export like pub(crate) use self::dispatch::dispatch_subagent;
(and ensure mod dispatch; is declared). Preserve visibility, comments, and any
tests or helper functions used by dispatch_subagent.

In `@src/openhuman/tools/impl/mod.rs`:
- Around line 10-17: The current glob re-exports (pub use agent::*; pub use
browser::*; pub use cron::*; pub use filesystem::*; pub use hardware::*; pub use
memory::*; pub use network::*; pub use system::*;) risk name collisions if two
submodules export the same identifier; either replace these globs with explicit
re-exports of the public types/functions you want to expose (e.g., list each
Tool type or function from agent, browser, cron, filesystem, hardware, memory,
network, system) or add a clear module-level comment documenting the required
naming convention (e.g., all public tool types must use a unique *Tool suffix)
to prevent future ambiguity and compilation errors.
🪄 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: 267616e0-1d77-4054-8413-f85eff513c69

📥 Commits

Reviewing files that changed from the base of the PR and between 88ae66a and 900e8ec.

⛔ Files ignored due to path filters (2)
  • Cargo.lock is excluded by !**/*.lock
  • app/src-tauri/Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (138)
  • Cargo.toml
  • docs/design-repl.md
  • src/core/cli.rs
  • src/core/jsonrpc.rs
  • src/core/mod.rs
  • src/core/repl.rs
  • src/openhuman/agent/agent/mod.rs
  • src/openhuman/agent/agents/archivist/agent.toml
  • src/openhuman/agent/agents/archivist/prompt.md
  • src/openhuman/agent/agents/code_executor/agent.toml
  • src/openhuman/agent/agents/code_executor/prompt.md
  • src/openhuman/agent/agents/critic/agent.toml
  • src/openhuman/agent/agents/critic/prompt.md
  • src/openhuman/agent/agents/mod.rs
  • src/openhuman/agent/agents/orchestrator/agent.toml
  • src/openhuman/agent/agents/orchestrator/prompt.md
  • src/openhuman/agent/agents/planner/agent.toml
  • src/openhuman/agent/agents/planner/prompt.md
  • src/openhuman/agent/agents/researcher/agent.toml
  • src/openhuman/agent/agents/researcher/prompt.md
  • src/openhuman/agent/agents/skills_agent/agent.toml
  • src/openhuman/agent/agents/skills_agent/prompt.md
  • src/openhuman/agent/agents/tool_maker/agent.toml
  • src/openhuman/agent/agents/tool_maker/prompt.md
  • src/openhuman/agent/classifier.rs
  • src/openhuman/agent/context_pipeline/pipeline.rs
  • src/openhuman/agent/cost.rs
  • src/openhuman/agent/dispatcher.rs
  • src/openhuman/agent/harness/archetypes.rs
  • src/openhuman/agent/harness/builtin_definitions.rs
  • src/openhuman/agent/harness/context_assembly.rs
  • src/openhuman/agent/harness/context_guard.rs
  • src/openhuman/agent/harness/credentials.rs
  • src/openhuman/agent/harness/dag.rs
  • src/openhuman/agent/harness/definition.rs
  • src/openhuman/agent/harness/executor.rs
  • src/openhuman/agent/harness/fork_context.rs
  • src/openhuman/agent/harness/instructions.rs
  • src/openhuman/agent/harness/memory_context.rs
  • src/openhuman/agent/harness/mod.rs
  • src/openhuman/agent/harness/parse.rs
  • src/openhuman/agent/harness/session/builder.rs
  • src/openhuman/agent/harness/session/mod.rs
  • src/openhuman/agent/harness/session/runtime.rs
  • src/openhuman/agent/harness/session/tests.rs
  • src/openhuman/agent/harness/session/turn.rs
  • src/openhuman/agent/harness/session/types.rs
  • src/openhuman/agent/harness/subagent_runner.rs
  • src/openhuman/agent/harness/tests.rs
  • src/openhuman/agent/harness/tool_loop.rs
  • src/openhuman/agent/harness/types.rs
  • src/openhuman/agent/host_runtime.rs
  • src/openhuman/agent/identity.rs
  • src/openhuman/agent/loop_/history.rs
  • src/openhuman/agent/loop_/mod.rs
  • src/openhuman/agent/loop_/session.rs
  • src/openhuman/agent/memory_loader.rs
  • src/openhuman/agent/mod.rs
  • src/openhuman/agent/prompt.rs
  • src/openhuman/agent/prompts/AGENTS.md
  • src/openhuman/agent/prompts/BOOTSTRAP.md
  • src/openhuman/agent/prompts/CONSCIOUS_LOOP.md
  • src/openhuman/agent/prompts/IDENTITY.md
  • src/openhuman/agent/prompts/MEMORY.md
  • src/openhuman/agent/prompts/README.md
  • src/openhuman/agent/prompts/SOUL.md
  • src/openhuman/agent/prompts/TOOLS.md
  • src/openhuman/agent/schemas.rs
  • src/openhuman/agent/tests.rs
  • src/openhuman/agent/traits.rs
  • src/openhuman/channels/prompt.rs
  • src/openhuman/channels/runtime/dispatch.rs
  • src/openhuman/channels/runtime/startup.rs
  • src/openhuman/channels/tests/common.rs
  • src/openhuman/channels/tests/identity.rs
  • src/openhuman/channels/tests/prompt.rs
  • src/openhuman/config/mod.rs
  • src/openhuman/config/schema/identity_cost.rs
  • src/openhuman/config/schema/mod.rs
  • src/openhuman/config/schema/orchestrator.rs
  • src/openhuman/config/schema/types.rs
  • src/openhuman/cron/scheduler.rs
  • src/openhuman/local_ai/ops.rs
  • src/openhuman/local_ai/schemas.rs
  • src/openhuman/tools/impl/agent/archetype_delegation.rs
  • src/openhuman/tools/impl/agent/ask_clarification.rs
  • src/openhuman/tools/impl/agent/delegate.rs
  • src/openhuman/tools/impl/agent/mod.rs
  • src/openhuman/tools/impl/agent/skill_delegation.rs
  • src/openhuman/tools/impl/agent/spawn_subagent.rs
  • src/openhuman/tools/impl/browser/browser.rs
  • src/openhuman/tools/impl/browser/browser_open.rs
  • src/openhuman/tools/impl/browser/image_info.rs
  • src/openhuman/tools/impl/browser/image_output.rs
  • src/openhuman/tools/impl/browser/mod.rs
  • src/openhuman/tools/impl/browser/screenshot.rs
  • src/openhuman/tools/impl/cron/add.rs
  • src/openhuman/tools/impl/cron/list.rs
  • src/openhuman/tools/impl/cron/mod.rs
  • src/openhuman/tools/impl/cron/remove.rs
  • src/openhuman/tools/impl/cron/run.rs
  • src/openhuman/tools/impl/cron/runs.rs
  • src/openhuman/tools/impl/cron/update.rs
  • src/openhuman/tools/impl/filesystem/file_read.rs
  • src/openhuman/tools/impl/filesystem/file_write.rs
  • src/openhuman/tools/impl/filesystem/git_operations.rs
  • src/openhuman/tools/impl/filesystem/mod.rs
  • src/openhuman/tools/impl/filesystem/read_diff.rs
  • src/openhuman/tools/impl/filesystem/run_linter.rs
  • src/openhuman/tools/impl/filesystem/run_tests.rs
  • src/openhuman/tools/impl/filesystem/update_memory_md.rs
  • src/openhuman/tools/impl/hardware/board_info.rs
  • src/openhuman/tools/impl/hardware/memory_map.rs
  • src/openhuman/tools/impl/hardware/memory_read.rs
  • src/openhuman/tools/impl/hardware/mod.rs
  • src/openhuman/tools/impl/memory/forget.rs
  • src/openhuman/tools/impl/memory/mod.rs
  • src/openhuman/tools/impl/memory/recall.rs
  • src/openhuman/tools/impl/memory/store.rs
  • src/openhuman/tools/impl/mod.rs
  • src/openhuman/tools/impl/network/composio.rs
  • src/openhuman/tools/impl/network/http_request.rs
  • src/openhuman/tools/impl/network/mod.rs
  • src/openhuman/tools/impl/network/skill_bridge.rs
  • src/openhuman/tools/impl/network/web_search.rs
  • src/openhuman/tools/impl/system/insert_sql_record.rs
  • src/openhuman/tools/impl/system/mod.rs
  • src/openhuman/tools/impl/system/proxy_config.rs
  • src/openhuman/tools/impl/system/pushover.rs
  • src/openhuman/tools/impl/system/schedule.rs
  • src/openhuman/tools/impl/system/shell.rs
  • src/openhuman/tools/impl/system/tool_stats.rs
  • src/openhuman/tools/impl/system/workspace_state.rs
  • src/openhuman/tools/local_cli.rs
  • src/openhuman/tools/mod.rs
  • src/openhuman/tools/orchestrator_tools.rs
  • src/openhuman/tools/traits.rs
  • src/openhuman/workspace/ops.rs
💤 Files with no reviewable changes (31)
  • src/core/mod.rs
  • Cargo.toml
  • src/openhuman/agent/prompts/IDENTITY.md
  • src/openhuman/agent/harness/session/runtime.rs
  • src/openhuman/agent/prompts/CONSCIOUS_LOOP.md
  • src/openhuman/agent/prompts/BOOTSTRAP.md
  • src/openhuman/agent/prompts/AGENTS.md
  • src/openhuman/config/schema/types.rs
  • src/openhuman/agent/prompts/README.md
  • src/openhuman/agent/prompts/TOOLS.md
  • docs/design-repl.md
  • src/openhuman/agent/harness/context_assembly.rs
  • src/openhuman/local_ai/schemas.rs
  • src/openhuman/agent/classifier.rs
  • src/openhuman/agent/harness/tests.rs
  • src/openhuman/agent/prompts/MEMORY.md
  • src/openhuman/agent/schemas.rs
  • src/core/repl.rs
  • src/openhuman/agent/harness/session/types.rs
  • src/openhuman/local_ai/ops.rs
  • src/openhuman/agent/harness/dag.rs
  • src/openhuman/agent/harness/archetypes.rs
  • src/openhuman/agent/loop_/history.rs
  • src/openhuman/agent/loop_/mod.rs
  • src/openhuman/agent/cost.rs
  • src/openhuman/agent/harness/executor.rs
  • src/openhuman/agent/loop_/session.rs
  • src/openhuman/agent/identity.rs
  • src/openhuman/agent/traits.rs
  • src/openhuman/agent/harness/types.rs
  • src/openhuman/agent/agent/mod.rs

Comment on lines +6 to +11
sandbox_mode = "none"
background = true
omit_identity = true
omit_memory_context = true
omit_safety_preamble = true
omit_skills_catalog = true
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 | 🟠 Major

Keep safety preamble enabled for this write-capable background agent.

At Line 10, disabling safety preamble while allowing persistent writes increases risk for unintended memory/DB mutations.

Suggested minimal hardening
-omit_safety_preamble = true
+omit_safety_preamble = false

Also applies to: 17-17

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/agent/agents/archivist/agent.toml` around lines 6 - 11, The
safety preamble is currently disabled for the write-capable background agent
(omit_safety_preamble = true); re-enable it by setting omit_safety_preamble =
false (or removing that key) in the agent TOML configuration so the Archivist
agent includes the safety preamble before write operations; also apply the same
change where omit_safety_preamble appears elsewhere in this config (the second
occurrence mentioned) to ensure consistent hardening across the agent.

Comment thread src/openhuman/agent/harness/definition.rs Outdated
Comment on lines +30 to +34
// MEMORY.md is optional (archivist-written). When present it should inline.
assert!(
prompt.contains("User likes Rust"),
"MEMORY.md content should be inlined when present"
);
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

Optional MEMORY behavior is not actually tested.

The comment states MEMORY.md is optional, but this assertion always requires it. Please add a companion case without MEMORY.md, or make this assertion conditional to match the intended behavior.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/channels/tests/identity.rs` around lines 30 - 34, The test
currently unconditionally asserts that prompt.contains("User likes Rust"), which
enforces MEMORY.md presence; change the test to cover both cases: add a second
test case that constructs the same context without a MEMORY.md file and assert
that prompt does NOT contain the memory string, or make the existing assertion
conditional by checking whether MEMORY.md was provided (e.g., inspect the test
setup flag or Option used to load memory) before asserting prompt.contains("User
likes Rust"). Update the test(s) around the prompt assertion (the call site
using prompt.contains("User likes Rust")) to either add the companion case or
guard the assertion so the behavior matches the documented "optional" MEMORY.md.

Comment on lines 154 to 171
let run_result = match job.session_target {
SessionTarget::Main | SessionTarget::Isolated => {
crate::openhuman::agent::run(
config.clone(),
Some(prefixed_prompt),
model_override,
config.default_temperature,
vec![],
)
.await
tracing::debug!(
job_id = %job.id,
target = ?job.session_target,
"[cron] building isolated agent for scheduled job"
);
match Agent::from_config(&effective) {
Ok(mut agent) => {
// Tag events so downstream subscribers can correlate
// cron-triggered turns. `cron` is the channel so the
// event bus can filter from other flows (`cli`, `web`…).
agent.set_event_context(format!("cron:{}", job.id), "cron");
agent.run_single(&prefixed_prompt).await
}
Err(e) => Err(e),
}
}
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 | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# 1) Locate SessionTarget definition and docs/comments about meaning.
rg -n -C3 '\benum\s+SessionTarget\b|SessionTarget::Main|SessionTarget::Isolated' src/openhuman

# 2) Find where session_target is surfaced to users/tools/apis.
rg -n -C3 '\bsession_target\b|add_agent_job|CronJobPatch|cron.*session' src/openhuman

# 3) Re-check scheduler handling for both variants.
rg -n -C3 'run_agent_job|match\s+job\.session_target|set_event_context|Agent::from_config' src/openhuman/cron/scheduler.rs

Repository: tinyhumansai/openhuman

Length of output: 26657


🏁 Script executed:

# Check for "main session" or session reuse patterns in Agent
rg -n 'main.*session|reuse.*session|SessionTarget|session.*target' src/openhuman/agent --max-count=20

# Look for TODO, FIXME, or comments about Main vs Isolated
rg -n 'TODO|FIXME|Main.*Isolated|session.*semantic' src/openhuman/cron

# Check if Agent has session management or reuse capabilities
rg -n 'fn.*session|set_session|current_session|get_session' src/openhuman/agent

Repository: tinyhumansai/openhuman

Length of output: 1163


🏁 Script executed:

# Look for ALL match statements on session_target
rg -n 'match.*session_target|SessionTarget::Main' src/openhuman

# Check if Main variant is used elsewhere or has special handling
rg -n 'Main\s*=>' src/openhuman

# Look for git history or comments about this change
git log --oneline -n 20 -- src/openhuman/cron/scheduler.rs 2>/dev/null || echo "Git history unavailable"

Repository: tinyhumansai/openhuman

Length of output: 481


🏁 Script executed:

# Look for docs or comments about SessionTarget variants
rg -B5 'enum SessionTarget|SessionTarget::Main|SessionTarget::Isolated' src/openhuman/cron/types.rs

# Check the full SessionTarget enum definition
sed -n '29,36p' src/openhuman/cron/types.rs

# Look for any README or design docs mentioning session handling
find src/openhuman -name "*.md" -o -name "*.txt" | xargs grep -l "SessionTarget\|session.*target" 2>/dev/null || echo "No docs found"

Repository: tinyhumansai/openhuman

Length of output: 394


Both SessionTarget variants execute identical code; consider collapsing enum or implementing distinct behavior.

At Line 155, SessionTarget::Main | SessionTarget::Isolated share a single match arm that creates a fresh isolated agent and tags it with cron:{job.id}. The SessionTarget enum exposes both variants to users via the tool schema (allowing "isolated" or "main" to be specified), but there is no separate implementation path or documentation explaining the intended semantic difference between them. Either:

  • If both should behave identically, remove the Main variant and simplify the API.
  • If Main should reuse a shared session or follow different semantics, implement a separate branch with the intended behavior and document the distinction.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/cron/scheduler.rs` around lines 154 - 171, The match arm for
job.session_target currently treats SessionTarget::Main and
SessionTarget::Isolated identically (both create a fresh Agent via
Agent::from_config, call agent.set_event_context with format!("cron:{}",
job.id), and run_single), which either makes the Main variant redundant or
misses intended shared-session behavior; either remove the Main variant from
SessionTarget (and update schema/consumers) if they should be identical, or
implement a separate branch for SessionTarget::Main that reuses or looks up a
shared session (e.g., fetch existing agent/session by id or use a shared Agent
pool) instead of calling Agent::from_config, ensuring you still
set_event_context and call run_single for Isolated while documenting the
semantics change.

…stency

- Revised the test for built-in agent definitions to dynamically reference the length of `BUILTINS`, enhancing maintainability.
- Clarified documentation for the `system_prompt` field in `AgentDefinition`, emphasizing the default behavior and usage of inline prompts.
- Simplified the `build_system_prompt` function by removing unnecessary conditional logic, ensuring a more straightforward return of the prompt string.
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

🧹 Nitpick comments (2)
src/openhuman/channels/prompt.rs (1)

25-30: Prefer is_file() for optional MEMORY.md injection guard.

exists() can also match directories/special paths. is_file() better matches the intent for optional file injection.

Proposed tweak
-    if workspace_dir.join("MEMORY.md").exists() {
+    if workspace_dir.join("MEMORY.md").is_file() {
         inject_workspace_file(prompt, workspace_dir, "MEMORY.md", max_chars_per_file);
     }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/channels/prompt.rs` around lines 25 - 30, The guard that checks
for the optional MEMORY.md uses exists(), which may be true for directories or
special paths; change the check to use is_file() on
workspace_dir.join("MEMORY.md") so only a regular file triggers injection,
leaving the call to inject_workspace_file(prompt, workspace_dir, "MEMORY.md",
max_chars_per_file) unchanged.
src/openhuman/agent/harness/builtin_definitions.rs (1)

19-21: Doc wording nit: “compile-time invariant” is misleading.

This is enforced at test/CI time, not at compile time. Rewording avoids confusion for maintainers.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/agent/harness/builtin_definitions.rs` around lines 19 - 21,
Update the documentation comment that currently says "a compile-time invariant
enforced by the unit tests in [`crate::openhuman::agent::agents::tests`]" to
avoid implying compile-time enforcement; reword it to indicate the invariant is
verified by the unit tests/CI (for example: "an invariant validated by the unit
tests in [`crate::openhuman::agent::agents::tests`] and enforced during
testing/CI"), keeping the reference to the existing test module so readers can
locate the check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/openhuman/agent/harness/definition.rs`:
- Around line 292-298: The current empty_inline_prompt() makes missing
system_prompt silently valid; update the TOML file-based loader (the code path
that constructs AgentDefinition from file) to reject/skip custom definitions
where AgentDefinition::system_prompt is PromptSource::Inline with an empty
string; require either PromptSource::File or a non-empty Inline string and
return an error or skip the agent load if neither is present. Locate the
file-to-definition loader function (the custom-definition loader) and add a
validation step after parsing that checks PromptSource::Inline.is_empty() and
enforces the failure, referencing PromptSource::Inline, PromptSource::File,
empty_inline_prompt(), and AgentDefinition::system_prompt.

---

Nitpick comments:
In `@src/openhuman/agent/harness/builtin_definitions.rs`:
- Around line 19-21: Update the documentation comment that currently says "a
compile-time invariant enforced by the unit tests in
[`crate::openhuman::agent::agents::tests`]" to avoid implying compile-time
enforcement; reword it to indicate the invariant is verified by the unit
tests/CI (for example: "an invariant validated by the unit tests in
[`crate::openhuman::agent::agents::tests`] and enforced during testing/CI"),
keeping the reference to the existing test module so readers can locate the
check.

In `@src/openhuman/channels/prompt.rs`:
- Around line 25-30: The guard that checks for the optional MEMORY.md uses
exists(), which may be true for directories or special paths; change the check
to use is_file() on workspace_dir.join("MEMORY.md") so only a regular file
triggers injection, leaving the call to inject_workspace_file(prompt,
workspace_dir, "MEMORY.md", max_chars_per_file) unchanged.
🪄 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: e3f2bf2a-e546-42a1-9ed5-627e4d002e86

📥 Commits

Reviewing files that changed from the base of the PR and between 900e8ec and 856297b.

📒 Files selected for processing (3)
  • src/openhuman/agent/harness/builtin_definitions.rs
  • src/openhuman/agent/harness/definition.rs
  • src/openhuman/channels/prompt.rs

Comment on lines +292 to +298
/// Placeholder for [`super::AgentDefinition::system_prompt`] when the
/// TOML omits the field. The built-in loader overwrites this with
/// the rendered sibling `prompt.md`; custom TOMLs that omit the
/// field get a no-op empty prompt (and should not).
pub(crate) fn empty_inline_prompt() -> PromptSource {
PromptSource::Inline(String::new())
}
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 | 🟠 Major

Reject missing system_prompt for custom TOML definitions.

Line 296’s default now makes all TOML definitions valid with an empty prompt. Built-ins are overwritten by the built-in loader, but workspace/user custom definitions can silently run with no core prompt. Please enforce in the custom-definition loader that file-sourced agents must provide either a non-empty inline prompt or PromptSource::File, and skip/error otherwise.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/agent/harness/definition.rs` around lines 292 - 298, The
current empty_inline_prompt() makes missing system_prompt silently valid; update
the TOML file-based loader (the code path that constructs AgentDefinition from
file) to reject/skip custom definitions where AgentDefinition::system_prompt is
PromptSource::Inline with an empty string; require either PromptSource::File or
a non-empty Inline string and return an error or skip the agent load if neither
is present. Locate the file-to-definition loader function (the custom-definition
loader) and add a validation step after parsing that checks
PromptSource::Inline.is_empty() and enforces the failure, referencing
PromptSource::Inline, PromptSource::File, empty_inline_prompt(), and
AgentDefinition::system_prompt.

- Updated the `load_file` function to reject definitions with missing or empty `system_prompt`, ensuring custom definitions are properly validated.
- Added a new test to verify that definitions lacking a `system_prompt` are correctly rejected, improving robustness.
- Clarified documentation for built-in definitions and TOML parsing, emphasizing compile-time guarantees and runtime checks.
- Improved the logic for checking the existence of `MEMORY.md` to prevent errors from stray directories, enhancing file handling reliability.
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.

1 participant