Pr 4333#4856
Conversation
- New openhuman chat subcommand (interactive REPL) - openhuman with no args starts chat (Claude Code-like) - npm package README with install/usage docs - Default dispatch to chat when no subcommand given
Default changed to start chat directly, update all docs.
- Type / or press Enter on empty line to open command menu - /model <name> to switch AI model mid-session - /login to authenticate with token - /session to show auth status - Better UI with styled prompts, thinking indicator, error display - First-run friendly error messages with guidance
- /threads: list threads, select to view messages - /memory: browse namespaces, list docs, semantic query, recall - /config: show config snapshots (full, model, agent, paths) - /files: browse memory files - /status: auth status with user info & balance - /logout: de-authenticate - /model: switch AI model - Subcommand routing: /memory list, /memory query, /config model, etc. - / and Enter triggers command menu picker - Commands organized as structured enum with completions - npm README updated with full command reference
Swap dialoguer::Input for raw-mode read_line_raw using console::Term::read_char() so typing / as first char shows the command menu instantly, matching Claude Code UX.
Full terminal UI matching opencode-style interaction: - OpenHuman ASCII logo and styling preserved - Chat panel with scrollable message list - Input bar with inline cursor editing - Command popup: type / instantly shows filtered command list - Tab toggles menu, Enter selects, Esc dismisses - Ctrl+C / /exit to quit - Agent integration via channels
- TUI sends messages to agent thread via mpsc channel - Agent processes messages on tokio runtime, sends responses back - No more placeholder echo responses - Cleaned up old REPL code replaced by TUI
…, trim palette, init cleanup
…mands - /model shows model selection popup at top of TUI - Arrow keys navigate, Enter selects, Esc cancels - AgentCmd enum + command channel for backend actions (SwitchModel) - All other commands show local system messages - Proper separation: chat messages go to agent, commands stay local
- Expand CMDS to include login, logout, status, threads, memory, files, config, usage, tools (12 total) - Add AgentCmd variants for all backend-routed commands - Wire Enter handler: /model -> popup, /help -> local list, /exit -> quit, backend cmds -> AgentCmd channel, rest -> agent - Add handle_cmd dispatcher in chat_cli.rs with per-command handler fns - Handle SwitchModel (recreate agent), Login/Logout (session mgmt), Status (config + auth), Tools (agent.tools()), Config (format fields) - Remove old /t command alias (superceded by /threads) - Fix Config field access (action_dir is PathBuf, not Option)
- /threads: calls memory_conversations::list_threads(workspace_dir)
to show actual thread titles and message counts
- /memory: calls memory_list_namespaces(EmptyRequest{}) for
real namespace listing via memory store client
- /files: calls ai_list_memory_files to list stored memory files
- /usage: uses cost::try_global() + get_daily_history(7) to
show daily token counts and costs
- /status: uses build_session_state for richer auth info
- /config: shows all Config fields (workspace, action dir, model,
API/inference URLs, temperature, language, schema version)
- /tools: unchanged (uses agent.tools())
- /login: shows helpful message about CLI login or env var
- /usage now creates a CostTracker from config.cost + workspace_dir when the global tracker is uninitialized (was always None since cost::init_global is never called in the CLI path) - Remove unused get_session_token, memory_list_documents, DailyCostEntry, ListDocumentsRequest imports
- PageUp/PageDown scroll through message history; ↑↓ scroll when input is empty (so you don't need Page keys on compact keyboards) - Status bar at bottom shows model name + controls hint - /new clears all chat messages and resets scroll - New messages auto-scroll to bottom (reset scroll_offset = 0) - Model name parsed from 'Switched to model: ...' response
Override action_dir to current working directory after loading config, so the agent operates in the directory where the user launched openhuman instead of the default ~/OpenHuman/projects.
Require initial model name from config so status bar shows the actual model (e.g. chat-v1) immediately instead of 'unknown'.
# Conflicts: # Cargo.lock
This reverts commit 8b3e045.
# Conflicts: # app/src-tauri/Cargo.lock
# Conflicts: # src/openhuman/channels/providers/web/mod.rs # src/openhuman/memory/chat.rs # src/openhuman/memory/tree_source/file.rs # src/openhuman/sandbox/docker.rs
# Conflicts: # app/src-tauri/Cargo.lock
- Add 11 SKILL.md skill definitions (review, plan, memory, debug, code, workflow, search, voice, browser, agent, config) in both .claude/skills/ and .openhuman/skills/ for cross-tool compatibility - Configure MCP servers (git, filesystem, github, openhuman-docs) - Set up permissions, approval gates, and voice/browser/settings config - Add .claude/skills/ to openhuman skill discovery paths - Create project trust marker for project-scope skill loading - Fix duplicate import in dictation_listener.rs All features verified working with local qwen3:0.6b via Ollama.
- 14 agent profiles: architectobot, build-agent, codecrusher, deploy-agent, designguru, dev-agent, memory-keeper, mobile-agent, pr-manager, pr-manager-lite, pr-reviewer, qualityqueen, taskmaster, test-agent - 2 command workflows: ws-reset (branch reset), ship-and-babysit (PR pipeline) - MCP servers: added playwright and context7 - Replaced generic 'agent' skill with specific role-based profiles - All skills sync to both project and user scope for discovery
- P1: Separate approval replies from chat input via dedicated channel - P1: Add quit-signal poll during active turns (cancel_check interval) - P2: Guard /new against active turn (don't clear screen while thinking) - P2: Drain stale approval-channel input before entering approval wait - Add is_approval_reply helper to route yes/no/approve/deny through tx_approval
…+ update submodules)
📝 WalkthroughWalkthroughThe pull request adds an interactive Rust TUI chat command, expands Claude/OpenHuman configuration and skills, introduces Linux build automation and operational workflows, and updates supporting test imports, module exports, and the vendored tinycortex revision. ChangesAssistant configuration and skill discovery
Interactive chat
Automation and compatibility
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
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 Warning |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 72271114a5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if is_approval_reply(&text) { | ||
| let _ = tx_approval.send(text.clone()); | ||
| } |
There was a problem hiding this comment.
When an approval prompt is active, typing yes/no sends the text to tx_approval, but this branch then falls through to Action::Send(text) for non-slash input. The main loop is still blocked in the current agent turn, so after the approval is applied it will process that queued yes/no as a brand-new user message, causing an unintended extra agent turn and transcript pollution. Return Action::Continue after sending an approval reply.
Useful? React with 👍 / 👎.
| if prompted_request_id.as_deref() != Some(request_id.as_str()) { | ||
| // Drain any stale input queued before this approval request | ||
| while rx_approval.try_recv().is_ok() {} | ||
| let _ = tx_resp.send(format_cli_approval_prompt(&gate, &request_id)); |
There was a problem hiding this comment.
During an active tool approval, this drain can discard valid replies: wait_for_cli_approval_reply is recreated whenever the sibling cancel_check.tick() branch wins in the surrounding tokio::select!, so prompted_request_id resets and a yes/no typed after the prompt but before the next recreated future polls is treated as stale and removed. In that scenario the tool remains parked and the user sees repeated approval prompts despite having answered.
Useful? React with 👍 / 👎.
| "filesystem": { | ||
| "type": "stdio", | ||
| "command": "npx", | ||
| "args": ["@modelcontextprotocol/server-filesystem", "/home/harry"] |
There was a problem hiding this comment.
Committing a filesystem MCP server rooted at /home/harry makes this project config non-portable and, on any machine where that path exists, exposes an entire personal home directory rather than the repo/workspace to the assistant. This should be scoped to a project-relative path or left for user-local, gitignored configuration so other contributors do not inherit a broken or over-broad filesystem server.
Useful? React with 👍 / 👎.
coderabbitai
Bot
added
feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (3)
src/core/tui.rs (2)
88-100: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoffNo panic-safety net for terminal restoration.
Cleanup (leave alternate screen, disable raw mode, restore cursor) only runs on normal return from the render loop. A panic anywhere in
render/handle_keywould unwind past this and leave the user's terminal stuck in raw/alternate-screen mode. Consider installing a panic hook (or a Drop guard) that restores the terminal before the default panic message prints.Also applies to: 163-166
🤖 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/core/tui.rs` around lines 88 - 100, Update the terminal setup and render loop around Terminal::new, render, and handle_key to install panic-safe cleanup that leaves the alternate screen, disables raw mode, and restores the cursor before panic reporting. Ensure the cleanup runs both during normal shutdown and when a panic unwinds, without suppressing the original panic.
175-451: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftNew file has zero test coverage and exceeds the ~500-line guideline.
tui.rsis a brand-new ~701-line file with no#[cfg(test)]module at all.handle_key,filtered_cmds, andis_approval_replyare pure functions overApp/KeyEventand are cheap to unit-test without a real terminal. As per coding guidelines, "Keep files preferably at or below approximately 500 lines," and based on learnings, "PRs require at least 80% coverage on changed lines" — this file currently has neither.🤖 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/core/tui.rs` around lines 175 - 451, Add unit tests for the pure input-handling helpers handle_key, filtered_cmds, and is_approval_reply, covering command filtering, approval replies, navigation, editing, and command submission behavior without requiring a real terminal. Include a #[cfg(test)] module in tui.rs and ensure changed-line coverage meets the project’s 80% requirement. Reduce tui.rs below the approximately 500-line guideline by extracting cohesive TUI logic into an appropriate module while preserving existing behavior.Sources: Coding guidelines, Learnings
src/openhuman/chat_cli/mod.rs (1)
510-545: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftCoverage gap on the core session/approval logic.
Only two small helpers are covered; the async session loop, approval-reply handling, and all
handle_*command dispatchers (the exact area with the critical bug flagged above) have no tests. Based on learnings, "PRs require at least 80% coverage on changed lines" — this file is far below that on its riskiest logic.🤖 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/openhuman/chat_cli/mod.rs` around lines 510 - 545, Expand the tests module beyond rewrite_chat_provider_model and format_cli_approval_decision to cover the async session loop, approval-reply handling, and each handle_* command dispatcher in the surrounding CLI implementation. Exercise successful, rejected, and edge-case paths—especially the critical approval flow—and ensure the added tests bring changed-line coverage to at least 80% without altering production behavior.Source: Learnings
🤖 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 @.claude/mcp.json:
- Around line 12-16: Replace the hardcoded /home/harry path in the filesystem
MCP configuration with a portable project-appropriate path or configurable
variable that resolves for each developer’s environment. Keep the filesystem
server entry and its intended access scope unchanged without referencing a
personal home directory.
- Around line 7-31: Add the non-interactive auto-confirm flag to the npx
argument arrays for the git, filesystem, github, and playwright MCP entries in
the MCP configuration, matching the existing context7 entry while preserving
each package name and command.
In @.claude/settings.json:
- Around line 15-39: Update the permissions entries in the permissions object:
replace the noncanonical FileRead allow rule with Read, and replace Delete in
the ask list with Edit so file access and write/delete operations match Claude
Code’s canonical tool names. Preserve the existing Bash, Read-related, and other
permission patterns.
In @.github/workflows/build-core.yml:
- Around line 22-26: Update the Checkout step using actions/checkout@v7 to set
persist-credentials to false in its with configuration, while preserving the
existing fetch-depth and recursive submodule settings.
In @.openhuman/skills/mobile-agent/SKILL.md:
- Line 4: Move when_to_use from the top-level frontmatter into the structured
metadata consumed by WorkflowFrontmatter, preserving its value and ensuring it
no longer lands in extra. Only add a dedicated typed field instead if the
existing consumers require top-level access, wiring that field through all
relevant parsing and consumption paths.
In @.openhuman/skills/pr-manager/SKILL.md:
- Around line 13-26: Make PR branch and remote safety explicit in both
workflows: in .openhuman/skills/pr-manager/SKILL.md lines 13-26, add preflight
validation requiring upstream/main as the base and verifying the target branch
and remote before checkout, push, or force-with-lease; in
.openhuman/skills/pr-manager-lite/SKILL.md lines 17-24, require pushes to origin
only and explicitly reject main as the target branch.
- Line 4: Remove the unsupported when_to_use key from the SKILL.md frontmatter
for the pr-manager skill, leaving the remaining frontmatter unchanged; do not
add loader support unless explicitly required.
In `@AGENTS.md`:
- Around line 17-18: The AGENTS.md skill documentation understates the contents
of .openhuman/skills/. Update AGENTS.md lines 17-18 to reflect the actual total
or explicitly identify the 11 listed bundles as a subset, and update lines
354-386 to include the newly added skills such as deploy-agent, designguru,
dev-agent, and memory-keeper or clearly scope the list to that subset; keep both
sections consistent.
In `@src/core/tui.rs`:
- Around line 319-354: Return Action::Continue immediately after sending an
approval reply through tx_approval in the Enter-key handling path, preventing it
from falling through to Action::Send(text) and being forwarded as a new agent
turn. Preserve the existing approval dispatch behavior for is_approval_reply
while leaving normal input handling unchanged.
- Around line 594-598: Update the cursor positioning logic in the rendering
block to derive the horizontal offset from the display width of the text prefix
before app.cursor, rather than using app.cursor as a byte-based column. Preserve
the existing menu/model_popup guard and bounds clamping while using the rendered
substring’s width for cx.
- Around line 530-562: The msgs_to_items and render_status functions incorrectly
embed ANSI escape sequences in raw text, so colors are rendered literally;
replace ansi_str-based text construction with ratatui styled Span/Line elements,
preserving the existing sender colors, labels, message layout, and status dot
appearance.
In `@src/openhuman/chat_cli/mod.rs`:
- Around line 27-33: The chat flow lacks entry and exit logging with a stable
correlation field. In src/openhuman/chat_cli/mod.rs:27-33, update
run_chat_command/run_interactive_session to log session start and a matching
exit before returning, including cli_thread_id; src/core/cli.rs:64-67 requires
no direct change once the root log exposes that field, while
src/core/cli.rs:88-88 may optionally add a debug dispatch log matching the agent
branch.
- Around line 1-33: Move the chat session loop, approval handling, command
handlers, and formatting helpers out of chat_cli’s mod.rs into a dedicated
session module, such as session.rs, preserving run_chat_command’s behavior and
public API. Update module declarations and imports so mod.rs contains only the
session module declaration and re-export of run_chat_command; split additional
approval or command logic into submodules only if needed.
- Around line 118-162: In the active-turn handling around
`wait_for_cli_approval_reply`, create and pin the approval-wait future before
entering the `tokio::select!` loop so timer ticks do not recreate it or reset
its state. After receiving `CliTurnSignal::ApprovalHandled`, replace and repin
the waiter for the next approval; preserve the existing `Quit` and
turn-completion behavior.
---
Nitpick comments:
In `@src/core/tui.rs`:
- Around line 88-100: Update the terminal setup and render loop around
Terminal::new, render, and handle_key to install panic-safe cleanup that leaves
the alternate screen, disables raw mode, and restores the cursor before panic
reporting. Ensure the cleanup runs both during normal shutdown and when a panic
unwinds, without suppressing the original panic.
- Around line 175-451: Add unit tests for the pure input-handling helpers
handle_key, filtered_cmds, and is_approval_reply, covering command filtering,
approval replies, navigation, editing, and command submission behavior without
requiring a real terminal. Include a #[cfg(test)] module in tui.rs and ensure
changed-line coverage meets the project’s 80% requirement. Reduce tui.rs below
the approximately 500-line guideline by extracting cohesive TUI logic into an
appropriate module while preserving existing behavior.
In `@src/openhuman/chat_cli/mod.rs`:
- Around line 510-545: Expand the tests module beyond
rewrite_chat_provider_model and format_cli_approval_decision to cover the async
session loop, approval-reply handling, and each handle_* command dispatcher in
the surrounding CLI implementation. Exercise successful, rejected, and edge-case
paths—especially the critical approval flow—and ensure the added tests bring
changed-line coverage to at least 80% without altering production behavior.
🪄 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
Run ID: 66acabac-d100-4310-b24e-f4e084f9c088
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockapp/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (48)
.claude/mcp.json.claude/settings.json.github/workflows/build-core.yml.openhuman/skills/architectobot/SKILL.md.openhuman/skills/browser/SKILL.md.openhuman/skills/build-agent/SKILL.md.openhuman/skills/code/SKILL.md.openhuman/skills/codecrusher/SKILL.md.openhuman/skills/config/SKILL.md.openhuman/skills/debug/SKILL.md.openhuman/skills/deploy-agent/SKILL.md.openhuman/skills/designguru/SKILL.md.openhuman/skills/dev-agent/SKILL.md.openhuman/skills/memory-keeper/SKILL.md.openhuman/skills/memory/SKILL.md.openhuman/skills/mobile-agent/SKILL.md.openhuman/skills/plan/SKILL.md.openhuman/skills/pr-manager-lite/SKILL.md.openhuman/skills/pr-manager/SKILL.md.openhuman/skills/pr-reviewer/SKILL.md.openhuman/skills/qualityqueen/SKILL.md.openhuman/skills/review/SKILL.md.openhuman/skills/search/SKILL.md.openhuman/skills/taskmaster/SKILL.md.openhuman/skills/test-agent/SKILL.md.openhuman/skills/voice/SKILL.md.openhuman/trust.openhuman/workflows/ship-and-babysit/WORKFLOW.md.openhuman/workflows/ws-reset/WORKFLOW.mdAGENTS.mdCargo.tomlsrc/core/chat_cli.rssrc/core/cli.rssrc/core/mod.rssrc/core/tui.rssrc/openhuman/channels/providers/web/mod.rssrc/openhuman/chat_cli/mod.rssrc/openhuman/config/schema/load/mod.rssrc/openhuman/inference/local/service/ollama_admin/mod.rssrc/openhuman/inference/provider/reliable.rssrc/openhuman/mcp_server/tools/mod.rssrc/openhuman/memory/chat.rssrc/openhuman/memory/schema/mod.rssrc/openhuman/memory/tree_source/file.rssrc/openhuman/mod.rssrc/openhuman/sandbox/docker.rssrc/openhuman/skills/ops_discover.rsvendor/tinycortex
| "git": { | ||
| "type": "stdio", | ||
| "command": "npx", | ||
| "args": ["@modelcontextprotocol/server-git"] | ||
| }, | ||
| "filesystem": { | ||
| "type": "stdio", | ||
| "command": "npx", | ||
| "args": ["@modelcontextprotocol/server-filesystem", "/home/harry"] | ||
| }, | ||
| "github": { | ||
| "type": "stdio", | ||
| "command": "npx", | ||
| "args": ["@modelcontextprotocol/server-github"] | ||
| }, | ||
| "playwright": { | ||
| "type": "stdio", | ||
| "command": "npx", | ||
| "args": ["@playwright/mcp@latest"] | ||
| }, | ||
| "context7": { | ||
| "type": "stdio", | ||
| "command": "npx", | ||
| "args": ["-y", "@upstash/context7-mcp"] | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
echo "=== .claude/mcp.json ==="
cat -n .claude/mcp.json
echo
echo "=== npx usage from local environment (if available) ==="
if command -v npx >/dev/null 2>&1; then
npx --help 2>/dev/null | sed -n '1,120p'
else
echo "npx not available in sandbox"
fiRepository: tinyhumansai/openhuman
Length of output: 2152
🌐 Web query:
npx -y --yes default install confirmation non-interactive prompts documentation
💡 Result:
To bypass the interactive installation confirmation prompt when using npx, you can use the --yes (or -y) flag [1][2][3]. By default, modern versions of npm (starting from version 7) prompt the user before installing packages that are not present in the local project dependencies [3][4]. Adding --yes to your npx command explicitly instructs it to proceed with the installation without requesting user confirmation, making it suitable for non-interactive environments such as CI/CD pipelines or automated scripts [1][4]. For example: npx --yes Alternatively, if you are working in an environment where you cannot easily modify the command arguments, you can set the environment variable npm_config_yes to true to achieve the same effect: npm_config_yes=true npx [4]
Citations:
- 1: [BUG] New --yes behavior in npx is a breaking change that can't easily be handled npm/cli#2226
- 2: https://stackoverflow.com/questions/68531768/make-npx-silently-install-latest-package-version-without-prompting-user-for-inp
- 3: https://docs.npmjs.com/cli/v8/commands/npx
- 4: https://kiko.io/notes/2023/avoid-npx-install-prompt/
Add -y to the remaining npx MCP entries. git, filesystem, github, and playwright can prompt for package install confirmation and block non-interactive startup; context7 already avoids that.
🤖 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 @.claude/mcp.json around lines 7 - 31, Add the non-interactive auto-confirm
flag to the npx argument arrays for the git, filesystem, github, and playwright
MCP entries in the MCP configuration, matching the existing context7 entry while
preserving each package name and command.
| "filesystem": { | ||
| "type": "stdio", | ||
| "command": "npx", | ||
| "args": ["@modelcontextprotocol/server-filesystem", "/home/harry"] | ||
| }, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Hardcoded user-specific path /home/harry.
This grants filesystem MCP access to a specific developer's home directory. It won't resolve for other contributors/machines and, if this is meant to be a shared project config, may unintentionally scope broad filesystem access to a personal directory.
🔧 Suggested fix
- "args": ["`@modelcontextprotocol/server-filesystem`", "/home/harry"]
+ "args": ["`@modelcontextprotocol/server-filesystem`", "${PROJECT_ROOT}"]🤖 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 @.claude/mcp.json around lines 12 - 16, Replace the hardcoded /home/harry
path in the filesystem MCP configuration with a portable project-appropriate
path or configurable variable that resolves for each developer’s environment.
Keep the filesystem server entry and its intended access scope unchanged without
referencing a personal home directory.
| "permissions": { | ||
| "allow": [ | ||
| { "tool": "Bash", "command": "git *" }, | ||
| { "tool": "Bash", "command": "npm *" }, | ||
| { "tool": "Bash", "command": "pnpm *" }, | ||
| { "tool": "Bash", "command": "cargo *" }, | ||
| { "tool": "Read" }, | ||
| { "tool": "Glob" }, | ||
| { "tool": "Grep" }, | ||
| { "tool": "FileRead" } | ||
| ], | ||
| "deny": [ | ||
| { "tool": "Bash", "command": "rm -rf /" }, | ||
| { "tool": "Bash", "command": "sudo *" }, | ||
| { "tool": "Bash", "command": "chmod 777 *" } | ||
| ], | ||
| "ask": [ | ||
| { "tool": "Bash", "command": "curl *" }, | ||
| { "tool": "Bash", "command": "wget *" }, | ||
| { "tool": "Write" }, | ||
| { "tool": "Edit" }, | ||
| { "tool": "Delete" } | ||
| ], | ||
| "defaultMode": "prompt" | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Claude Code have a "Delete" tool or "FileRead" tool in its permissions system?
💡 Result:
Yes, Claude Code includes a FileReadTool in its toolset, and it maintains a comprehensive permissions system that governs all tool usage, including file operations [1][2][3]. The FileReadTool is a core component of Claude Code's file operations, designed for reading file contents (including support for images, PDFs, and Jupyter notebooks) [3][4][5]. Regarding a "Delete" tool, while there is no specific standalone "FileDeleteTool" listed in the standard core file operations, Claude Code includes a TeamDeleteTool for managing agent teams [3][4]. For general file deletion, the agent typically utilizes the BashTool, which allows for shell command execution (e.g., executing rm) [3][5]. Because the BashTool is a high-capability tool, it is subject to rigorous security checks within the unified permission system, which handles all tool requests through a multi-stage validation pipeline [1][2]. The permissions system is designed with "fail-closed" defaults and uses a granular engine to evaluate tool usage based on configured rules, workspace trust, and mode-based defaults (such as plan, auto, or interactive modes) [6][7][5]. Every tool, including those for file operations or team management, must pass through this permission check, which can deny, approve, or require user confirmation depending on the safety evaluation of the requested operation [6][1][2].
Citations:
- 1: https://github.com/Windy3f3f3f3f/how-claude-code-works/blob/main/en/docs/01-overview.md
- 2: https://notes.tsukino.dev/99-%E5%B7%A5%E5%85%B7%E4%B8%8E%E5%8F%82%E8%80%83/repos/how-claude-code-works/en/docs/04-tool-system
- 3: https://github.com/Windy3f3f3f3f/how-claude-code-works/blob/main/en/docs/04-tool-system.md
- 4: https://news.lavx.hu/article/claude-code-unpacked-inside-the-40-tools-and-agent-architecture
- 5: https://github.com/Windy3f3f3f3f/how-claude-code-works/blob/main/en/docs/quick-start.md
- 6: https://codex.cadences.app/en/blog/claude-code-tool-permission-system/
- 7: https://zhu1090093659.github.io/claude-code-cookbook/books/Chapter-07-Permission-and-Security-Model.html
🌐 Web query:
Official Claude Code permission tool names FileRead Delete permissions settings.json
💡 Result:
Claude Code uses specific tool names and syntax within the settings.json file to manage permissions, including Read and Edit (which covers file deletions and other write operations) [1][2][3]. Permission Rules To configure permissions, you define arrays under the permissions object in your settings.json (at ~/.claude/settings.json for global settings or .claude/settings.json for project-specific settings) [2][4][3]. 1. Tool Names: You reference tools by their specific names, such as Read, Edit, Bash, or WebFetch [1][3]. 2. Rule Format: Rules follow the format ToolName(specifier) [1]. - Read permissions: Use Read(path-pattern) to control file access [1][3]. - Write/Delete permissions: Use Edit(path-pattern) to control file modifications, including writes, deletions, and moves [1]. 3. Deny/Allow Lists: - deny: Explicitly blocks tools or specific paths, even if they would otherwise be permitted [2][3]. Example: "deny": ["Read(./.env)", "Read(./secrets/)"] [2][3]. - allow: Pre-approves tools or specific paths to avoid prompts [2][4]. Management Interface Beyond editing settings.json directly, you can manage these rules interactively by running the /permissions command in the Claude Code CLI [5][6]. This interface allows you to view existing rules and manage permissions for the current session or project [5][6]. Important Notes on Enforcement - Evaluation Order: Rules are processed in the order deny -> ask -> allow. If a rule matches, the first applicable outcome (typically deny) takes precedence [3]. - Security Limitations: While the documentation outlines this permission system, users have reported issues where certain deny patterns for Read and Write/Edit tools are not consistently enforced in some versions [7][8][9]. For critical security, consider that bypassPermissions or defaultMode: "bypassPermissions" will ignore standard permission checks regardless of deny rules (though explicit ask rules may still trigger as a circuit breaker) [10][11][12][2]. Always verify your configuration using the /permissions UI [5][6].
Citations:
- 1: https://code.claude.com/docs/en/tools-reference
- 2: https://code.claude.com/docs/en/settings.md
- 3: https://devbriefs.com/claude-code-settings-json-options-guide/
- 4: https://claudeguide.io/claude-code-settings-json-reference
- 5: https://code.claude.com/docs/en/permissions
- 6: https://code.claude.com/docs/en/commands.md
- 7: https://stackoverflow.com/questions/79747164/claude-code-doesnt-respect-settings-json-deny-block
- 8: [BUG] Deny permissions in .claude/settings.json ignored anthropics/claude-code#27040
- 9: [BUG] Permission deny patterns not working for Read/Write tools in settings.local.json anthropics/claude-code#4467
- 10: https://code.claude.com/docs/en/permissions.md
- 11: https://code.claude.com/docs/en/permission-modes?_rsc=4Vrbm42DZc3Y7r9j
- 12: https://code.claude.com/docs/en/agent-sdk/permissions
Replace FileRead and Delete with canonical Claude Code tools
FileRead and Delete won't match the permission rules here. Use Read(...) for file access and Edit(...) for write/delete gating.
🤖 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 @.claude/settings.json around lines 15 - 39, Update the permissions entries
in the permissions object: replace the noncanonical FileRead allow rule with
Read, and replace Delete in the ask list with Edit so file access and
write/delete operations match Claude Code’s canonical tool names. Preserve the
existing Bash, Read-related, and other permission patterns.
| - name: Checkout | ||
| uses: actions/checkout@v7 | ||
| with: | ||
| fetch-depth: 1 | ||
| submodules: recursive |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Fetch the latest release version of actions/checkout
gh release list --repo actions/checkout --limit 1Repository: tinyhumansai/openhuman
Length of output: 202
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the workflow snippet and related checkout usages.
git ls-files .github/workflows/build-core.yml .github/workflows/*.yml | sed -n '1,20p'
echo
cat -n .github/workflows/build-core.yml | sed -n '1,120p'
echo
rg -n "actions/checkout@v[0-9]+" .github/workflows -SRepository: tinyhumansai/openhuman
Length of output: 6820
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# See whether checkout hardening is already used elsewhere.
rg -n -C 2 "persist-credentials:\s*false|uses: actions/checkout@v7|uses: actions/checkout@v4" .github/workflows -SRepository: tinyhumansai/openhuman
Length of output: 25330
Set persist-credentials: false on the checkout step
actions/checkout@v7 is fine here, but this job should disable credential persistence so the token isn’t left in git config.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 22-26: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 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 @.github/workflows/build-core.yml around lines 22 - 26, Update the Checkout
step using actions/checkout@v7 to set persist-credentials to false in its with
configuration, while preserving the existing fetch-depth and recursive submodule
settings.
Source: Linters/SAST tools
| --- | ||
| name: mobile-agent | ||
| description: Android and iOS development specialist | ||
| when_to_use: Working on mobile-specific features or configuration |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== files ==\n'
git ls-files '.openhuman/skills/mobile-agent/SKILL.md' || true
printf '\n== search WorkflowFrontmatter and when_to_use ==\n'
rg -n "WorkflowFrontmatter|when_to_use|metadata" .openhuman -g '!**/node_modules/**' || true
printf '\n== find likely schema files ==\n'
fd -a 'WorkflowFrontmatter|frontmatter|skill' .openhuman . 2>/dev/null | head -n 200Repository: tinyhumansai/openhuman
Length of output: 5543
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the skill file and nearby frontmatter schema definitions.
printf '\n== skill file ==\n'
cat -n .openhuman/skills/mobile-agent/SKILL.md | sed -n '1,80p'
printf '\n== candidate schema files ==\n'
fd -a '.*(frontmatter|workflow|skill).*' .openhuman 2>/dev/null | head -n 200
printf '\n== search for typed frontmatter definition ==\n'
rg -n "type WorkflowFrontmatter|interface WorkflowFrontmatter|WorkflowFrontmatter" .openhuman -g '!**/node_modules/**' || trueRepository: tinyhumansai/openhuman
Length of output: 2535
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== search frontmatter parsers / skill loaders ==\n'
rg -n "frontmatter|gray-matter|yaml|allowed-tools|when_to_use|SKILL\.md|WORKFLOW\.md|skills_data|skill registry|workflow" src app tests .openhuman -g '!**/node_modules/**' || true
printf '\n== inspect likely skill registry files ==\n'
sed -n '1,240p' src/openhuman/skill_registry/mod.rs 2>/dev/null || true
sed -n '1,240p' src/openhuman/skill_registry/lib.rs 2>/dev/null || true
sed -n '1,240p' src/openhuman/skill_registry/agent/skill_setup/mod.rs 2>/dev/null || true
sed -n '1,240p' src/openhuman/skill_runtime/mod.rs 2>/dev/null || true
sed -n '1,240p' src/openhuman/skill_runtime/agent/skill_executor/mod.rs 2>/dev/null || trueRepository: tinyhumansai/openhuman
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== exact when_to_use definitions ==\n'
rg -n "when_to_use" src app .openhuman -g '!**/node_modules/**' | head -n 200
printf '\n== exact frontmatter struct/type definitions ==\n'
rg -n "struct .*Frontmatter|type .*Frontmatter|interface .*Frontmatter|when_to_use:|allowed-tools:|allowed_tools:" src app .openhuman -g '!**/node_modules/**' | head -n 200
printf '\n== inspect dispatch and registry slices ==\n'
sed -n '220,320p' src/openhuman/mcp_server/tools/dispatch.rs
sed -n '1,220p' src/openhuman/skill_registry/mod.rs 2>/dev/null || true
sed -n '1,220p' src/openhuman/skill_registry/agent/skill_setup/mod.rs 2>/dev/null || true
sed -n '1,240p' src/openhuman/skill_runtime/agent/skill_executor/mod.rs 2>/dev/null || trueRepository: tinyhumansai/openhuman
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== WorkflowFrontmatter and related types ==\n'
sed -n '1,180p' src/openhuman/skills/ops_types.rs
printf '\n== skill create/render paths around when_to_use ==\n'
sed -n '300,620p' src/openhuman/skills/ops_create.rs
printf '\n== registry parsing/normalization around when_to_use ==\n'
sed -n '180,280p' src/openhuman/skills/registry.rsRepository: tinyhumansai/openhuman
Length of output: 23364
Keep when_to_use inside the structured metadata
WorkflowFrontmatter only exposes typed fields for name, description, license, compatibility, platforms, metadata, allowed-tools, and triggers; a top-level when_to_use falls into extra. Move it under metadata, or add a dedicated typed field and wire consumers to it.
🤖 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 @.openhuman/skills/mobile-agent/SKILL.md at line 4, Move when_to_use from the
top-level frontmatter into the structured metadata consumed by
WorkflowFrontmatter, preserving its value and ensuring it no longer lands in
extra. Only add a dedicated typed field instead if the existing consumers
require top-level access, wiring that field through all relevant parsing and
consumption paths.
| fn msgs_to_items(app: &App) -> Vec<ListItem<'static>> { | ||
| let mut items = Vec::new(); | ||
| for m in &app.msgs { | ||
| let tag = match m.sender.as_str() { | ||
| "you" => ansi_str("you", CYAN), | ||
| "ai" | "assistant" => ansi_str("ai", Color::Green), | ||
| "system" => ansi_str("sys", Color::Gray), | ||
| _ => ansi_str(&m.sender, Color::Gray), | ||
| }; | ||
| let mut first = true; | ||
| for line in m.content.split('\n') { | ||
| let text = if first { | ||
| format!(" {} {}", tag, line) | ||
| } else { | ||
| format!(" {}", line) | ||
| }; | ||
| items.push(ListItem::new(Text::raw(text)).style(Style::default().bg(DARK_BG))); | ||
| first = false; | ||
| } | ||
| } | ||
| items | ||
| } | ||
|
|
||
| fn render_status(f: &mut Frame, app: &App, area: Rect) { | ||
| let msg = format!( | ||
| " {} Model: {} | Ctrl+C to quit | / for menu ", | ||
| ansi_str("●", Color::Green), | ||
| app.model_name | ||
| ); | ||
| let para = | ||
| Paragraph::new(Text::raw(msg)).style(Style::default().bg(SURFACE).fg(Color::DarkGray)); | ||
| f.render_widget(para, area); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant file and show the surrounding lines.
git ls-files src/core/tui.rs
wc -l src/core/tui.rs
sed -n '500,730p' src/core/tui.rs | cat -n
# Find all uses/definition of ansi_str.
rg -n "ansi_str|Text::raw|ListItem::new|Paragraph::new" src/core/tui.rsRepository: tinyhumansai/openhuman
Length of output: 8555
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Identify the ratatui dependency version and inspect the second ansi_str area.
rg -n 'ratatui|ansi_str' Cargo.toml Cargo.lock src/core/tui.rs
sed -n '680,705p' src/core/tui.rs | cat -nRepository: tinyhumansai/openhuman
Length of output: 1549
🌐 Web query:
ratatui 0.29 Text::raw ANSI escape sequences rendered as text ansi-to-tui crate documentation
💡 Result:
In Ratatui, the Text::raw method is used to create text without applying any Ratatui-specific styling [1][2]. It does not parse or render ANSI escape sequences [3][4][5]. If you pass a string containing ANSI escape codes to Text::raw, the terminal may attempt to interpret them directly as control characters when the buffer is rendered, which often results in incorrect character placement, broken formatting, or visual artifacts, as Ratatui manages style information separately within its Buffer and Cell structures [6][7][5]. To properly render ANSI-styled text in Ratatui, you should use the ansi-to-tui crate [8][4]. This crate is designed to parse bytes containing ANSI SGR (Select Graphic Rendition) escape sequences and convert them into a Ratatui Text object, correctly mapping colors and modifiers to Ratatui's Style system [8][9]. Key details regarding this approach: - Functionality: ansi-to-tui parses ANSI codes (such as those for bold, italic, and various color modes including 3/4-bit, 256-color, and 24-bit RGB) and produces a valid Ratatui Text or Line struct [8][9]. - Implementation: You can use the IntoText trait provided by the crate to convert your input directly [8][9]. - Usage Example: use ansi_to_tui::IntoText as _; let bytes = b"\x1b[31mRed Text\x1b[0m"; // Example ANSI encoded bytes let text = bytes.into_text.unwrap; // Converts to Ratatui Text // Now pass 'text' to a widget like Paragraph::new(text) Using this crate is the recommended way to handle ANSI-encoded strings in Ratatui applications [6][3][4].
Citations:
- 1: https://docs.rs/ratatui/latest/ratatui/text/struct.Text.html
- 2: https://docs.rs/crate/ratatui/0.29.0
- 3: https://forum.ratatui.rs/t/write-raw-ansi-piped-from-other-cli-command/252
- 4: https://github.com/ratatui/ratatui-website/blob/main/src/content/docs/concepts/rendering/under-the-hood.md
- 5: Ansi sequences stripped? ratatui/ratatui#563
- 6: Don't swallow ANSI ratatui/ratatui#902
- 7: Don't swallow ANSI ratatui/ratatui#902
- 8: https://docs.rs/ansi-to-tui/latest/ansi_to_tui/
- 9: https://docs.rs/crate/ansi-to-tui/latest
Use ratatui styles instead of raw ANSI escapes.
ansi_str injects literal \x1b[..m sequences into Text::raw, but ratatui treats that as plain text, so the sender tags and status dot won’t actually render with color. Build these as styled Spans/Lines instead.
🤖 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/core/tui.rs` around lines 530 - 562, The msgs_to_items and render_status
functions incorrectly embed ANSI escape sequences in raw text, so colors are
rendered literally; replace ansi_str-based text construction with ratatui styled
Span/Line elements, preserving the existing sender colors, labels, message
layout, and status dot appearance.
| f.render_widget(Paragraph::new(text).style(style), inner); | ||
| if !app.menu && !app.model_popup { | ||
| let cx = inner.x + 1 + app.cursor as u16; | ||
| f.set_cursor_position((cx.min(area.right().saturating_sub(1)), inner.y)); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- src/core/tui.rs outline ---\n'
ast-grep outline src/core/tui.rs --view expanded || true
printf '\n--- relevant cursor/input symbols ---\n'
rg -n "cursor|input|unicode-width|UnicodeWidth" src/core/tui.rs src -g '*.rs' || true
printf '\n--- src/core/tui.rs excerpt around the cursor code ---\n'
sed -n '560,610p' src/core/tui.rs
printf '\n--- app definition / cursor type ---\n'
rg -n "struct App|cursor:|input:" src -g '*.rs' || trueRepository: tinyhumansai/openhuman
Length of output: 251
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- src/core/tui.rs outline ---'
ast-grep outline src/core/tui.rs --view expanded || true
echo
echo '--- relevant cursor/input symbols ---'
rg -n "cursor|input|unicode-width|UnicodeWidth" src/core/tui.rs src -g '*.rs' || true
echo
echo '--- src/core/tui.rs excerpt around the cursor code ---'
sed -n '560,610p' src/core/tui.rs
echo
echo '--- app definition / cursor type ---'
rg -n "struct App|cursor:|input:" src -g '*.rs' || trueRepository: tinyhumansai/openhuman
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- src/core/tui.rs lines 560-610 ---'
sed -n '560,610p' src/core/tui.rs
echo
echo '--- src/core/tui.rs AST outline (trimmed) ---'
ast-grep outline src/core/tui.rs --view expanded | sed -n '1,220p'
echo
echo '--- cursor/input definitions across src/core and likely app state ---'
rg -n "struct App|pub struct App|cursor: (usize|u16|usize)|input: String|cursor as u16|UnicodeWidthStr|unicode-width" src/core src -g '*.rs' -g '!target' || trueRepository: tinyhumansai/openhuman
Length of output: 6752
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- handle_key / cursor mutation logic ---'
sed -n '175,360p' src/core/tui.rs
echo
echo '--- app initialization and any cursor reset/update paths ---'
sed -n '80,175p' src/core/tui.rs
echo
echo '--- any direct cursor arithmetic elsewhere ---'
rg -n "app\.cursor|cursor\s*=" src/core/tui.rs -nRepository: tinyhumansai/openhuman
Length of output: 10848
Cursor position should use display width, not byte offset. app.cursor advances in UTF-8 bytes, so the caret drifts for non-ASCII input; compute the column from the rendered substring width instead.
🤖 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/core/tui.rs` around lines 594 - 598, Update the cursor positioning logic
in the rendering block to derive the horizontal offset from the display width of
the text prefix before app.cursor, rather than using app.cursor as a byte-based
column. Preserve the existing menu/model_popup guard and bounds clamping while
using the rendered substring’s width for cx.
| //! `openhuman chat` — TUI-based interactive chat session. | ||
|
|
||
| use std::sync::mpsc; | ||
|
|
||
| use anyhow::{anyhow, Result}; | ||
| use console::style; | ||
|
|
||
| use crate::core::tui::AgentCmd; | ||
| use crate::core::types::{approval_gate_boot_decision, HostKind}; | ||
| use crate::openhuman::agent::turn_origin::{with_origin, AgentTurnOrigin}; | ||
| use crate::openhuman::agent::Agent; | ||
| use crate::openhuman::approval::gate::{record_boot_state, ApprovalGateBootState}; | ||
| use crate::openhuman::approval::{ | ||
| parse_approval_reply, ApprovalChatContext, ApprovalDecision, ApprovalGate, | ||
| APPROVAL_CHAT_CONTEXT, | ||
| }; | ||
| use crate::openhuman::config::ops::ModelSettingsPatch; | ||
| use crate::openhuman::config::rpc::load_and_apply_model_settings; | ||
| use crate::openhuman::config::Config; | ||
| use crate::openhuman::cost::{try_global, CostTracker}; | ||
| use crate::openhuman::credentials::ops::clear_session; | ||
| use crate::openhuman::credentials::session_support::build_session_state; | ||
| use crate::openhuman::memory::ops::{ai_list_memory_files, memory_list_namespaces}; | ||
| use crate::openhuman::memory::rpc_models::{EmptyRequest, ListMemoryFilesRequest}; | ||
| use crate::openhuman::memory_conversations::list_threads; | ||
|
|
||
| pub fn run_chat_command(args: &[String]) -> Result<()> { | ||
| if args.iter().any(|a| a == "-h" || a == "--help") { | ||
| print_help(); | ||
| return Ok(()); | ||
| } | ||
| run_interactive_session() | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Business logic belongs in a submodule, not mod.rs.
This mod.rs (~690 lines) holds the full session loop, approval handling, command handlers, and formatting helpers. As per coding guidelines, "src/openhuman/**/mod.rs: Keep mod.rs export-focused: only module declarations, re-exports, and the controller schema pair; do not put business logic there," and separately "Keep files preferably at or below approximately 500 lines." Move the orchestration/handlers into e.g. src/openhuman/chat_cli/session.rs (and maybe approval.rs/commands.rs), leaving mod.rs with just pub mod session; pub use session::run_chat_command;.
🤖 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/openhuman/chat_cli/mod.rs` around lines 1 - 33, Move the chat session
loop, approval handling, command handlers, and formatting helpers out of
chat_cli’s mod.rs into a dedicated session module, such as session.rs,
preserving run_chat_command’s behavior and public API. Update module
declarations and imports so mod.rs contains only the session module declaration
and re-export of run_chat_command; split additional approval or command logic
into submodules only if needed.
Source: Coding guidelines
| pub fn run_chat_command(args: &[String]) -> Result<()> { | ||
| if args.iter().any(|a| a == "-h" || a == "--help") { | ||
| print_help(); | ||
| return Ok(()); | ||
| } | ||
| run_interactive_session() | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
New interactive-chat flow lacks the guideline-mandated entry/branch logging across its call chain. The root cause is that run_chat_command/run_interactive_session never log session start, so every caller that dispatches into it is also silent, unlike sibling flows (e.g. the "agent" arm) that log before dispatching. As per coding guidelines, "New or changed flows must log entry and exit, branches, external calls, ... using stable grep-friendly prefixes and correlation fields."
src/openhuman/chat_cli/mod.rs#L27-L33: add an entry log (e.g.log::info!("[chat_cli] session started thread_id={cli_thread_id}")) inrun_chat_command/run_interactive_session, and a matching exit log before returning.src/core/cli.rs#L64-L67: once the root entry log exists, this empty-args default-to-chat branch is adequately covered by it; no separate change needed here beyond confirming the correlation field surfaces.src/core/cli.rs#L88-L88: same — covered once the root log lands; optionally add alog::debug!before dispatch to match the"agent"arm's style for grep-ability at the dispatch site itself.
📍 Affects 2 files
src/openhuman/chat_cli/mod.rs#L27-L33(this comment)src/core/cli.rs#L64-L67src/core/cli.rs#L88-L88
🤖 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/openhuman/chat_cli/mod.rs` around lines 27 - 33, The chat flow lacks
entry and exit logging with a stable correlation field. In
src/openhuman/chat_cli/mod.rs:27-33, update
run_chat_command/run_interactive_session to log session start and a matching
exit before returning, including cli_thread_id; src/core/cli.rs:64-67 requires
no direct change once the root log exposes that field, while
src/core/cli.rs:88-88 may optionally add a debug dispatch log matching the agent
branch.
Source: Coding guidelines
| let turn = with_origin( | ||
| origin, | ||
| APPROVAL_CHAT_CONTEXT | ||
| .scope(approval_ctx, active_agent.run_single(&msg)), | ||
| ); | ||
| tokio::pin!(turn); | ||
| let mut cancel_check = tokio::time::interval( | ||
| std::time::Duration::from_millis(200), | ||
| ); | ||
| loop { | ||
| tokio::select! { | ||
| _ = cancel_check.tick() => { | ||
| if tui_requested_quit(&rx_quit) { | ||
| log::debug!("[chat_cli] TUI quit during active turn; aborting"); | ||
| break; | ||
| } | ||
| } | ||
| result = &mut turn => { | ||
| match result { | ||
| Ok(response) => { | ||
| let _ = tx_resp.send(response); | ||
| } | ||
| Err(e) => { | ||
| log::debug!("[chat_cli] agent turn failed: {e}"); | ||
| let _ = tx_resp.send(format!("Error: {e}")); | ||
| } | ||
| } | ||
| break; | ||
| } | ||
| signal = wait_for_cli_approval_reply( | ||
| &rx_approval, | ||
| &rx_quit, | ||
| &tx_resp, | ||
| &cli_thread_id, | ||
| ) => { | ||
| match signal { | ||
| CliTurnSignal::ApprovalHandled => continue, | ||
| CliTurnSignal::Quit => { | ||
| log::debug!("[chat_cli] TUI quit signal received during agent turn; cancelling session"); | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,340p' src/openhuman/chat_cli/mod.rsRepository: tinyhumansai/openhuman
Length of output: 13948
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path('src/openhuman/chat_cli/mod.rs')
for start,end in [(96,188),(213,287)]:
print(f"\n--- lines {start}-{end} ---")
for i,line in enumerate(p.read_text().splitlines(),1):
if start <= i <= end:
print(f"{i:4}: {line}")
PYRepository: tinyhumansai/openhuman
Length of output: 9067
🏁 Script executed:
rg -n "wait_for_cli_approval_reply|tokio::pin!\(.*approval|ApprovalHandled" src/openhuman -g '*.rs'Repository: tinyhumansai/openhuman
Length of output: 606
Pin the approval wait across select! iterations. src/openhuman/chat_cli/mod.rs:147-154, 213-239 Calling wait_for_cli_approval_reply(...) inline recreates that future whenever cancel_check.tick() wins, which resets prompted_request_id and can re-send the approval prompt while also draining a queued reply. Keep the approval waiter pinned outside the loop and replace it only after ApprovalHandled.
🤖 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/openhuman/chat_cli/mod.rs` around lines 118 - 162, In the active-turn
handling around `wait_for_cli_approval_reply`, create and pin the approval-wait
future before entering the `tokio::select!` loop so timer ticks do not recreate
it or reset its state. After receiving `CliTurnSignal::ApprovalHandled`, replace
and repin the waiter for the next approval; preserve the existing `Quit` and
turn-completion behavior.
|
Triage check-in: this PR has changes requested and the branch now conflicts with main, and the title/description don't make the intended scope clear. Are you still pursuing it? If so, please rebase, address the review feedback, and give it a descriptive title. No response in ~2 weeks and we'll close it to keep the queue tidy — it can always be reopened. |
3 participants
Summary
Problem
Solution
Submission Checklist
diff-cover) meet the gate enforced by.github/workflows/ci-lite.yml. Runpnpm test:coverageandpnpm test:rustlocally; PRs below 80% on changed lines will not merge.docs/TEST-COVERAGE-MATRIX.mdreflect this change (orN/A: behaviour-only change)## Relateddocs/RELEASE-MANUAL-SMOKE.md)Closes #NNNin the## RelatedsectionImpact
Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
Validation Run
pnpm --filter openhuman-app format:checkpnpm typecheckValidation Blocked
command:error:impact:Behavior Changes
Parity Contract
Duplicate / Superseded PR Handling
Summary by CodeRabbit
New Features
Documentation
Chores