90 feature add pengine cli#94
Open
maximedogawa wants to merge 23 commits into
Open
Conversation
…ling - Added a new CLI plan and implementation for Pengine, allowing users to execute native commands directly from the terminal. - Updated documentation to reflect the new CLI capabilities, including command usage and integration with the Telegram bot. - Enhanced error handling in various API calls and added a new CLI commands panel in the dashboard. - Refactored existing modules to support the CLI functionality, ensuring a seamless user experience across terminal and GUI environments. - Updated package metadata to reflect the new versioning and naming conventions.
- Fixed a syntax error in the dispatch_native function by removing an unnecessary comma, ensuring proper command handling in the CLI.
- Updated the TerminalPreview component to improve log display. - Simplified messages in the CliCommandsPanel for clarity. - Introduced a new syntax highlighting module for better code block rendering in CLI. - Enhanced the output rendering logic to support highlighted code blocks. - Updated Cargo dependencies to include new libraries for syntax highlighting.
- Simplified the formatting logic in the model catalog list function for improved readability. - Enhanced the output handling in the model loading response to maintain consistent formatting. - Cleaned up unnecessary line breaks and improved code structure for better maintainability.
9 tasks
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Terminal CLI subsystem (bootstrap, dispatch, REPL, native commands, output sinks, syntax highlighting, Telegram Changes
Sequence Diagram(s)sequenceDiagram
participant Shell as User/Shell
participant Bootstrap as CLI Bootstrap
participant TauriApp as Tauri App
participant State as AppState
participant Dispatch as CLI Dispatcher
participant Handler as CLI Handler
participant Sink as OutputSink
Shell->>Bootstrap: run `pengine` [args]
Bootstrap->>Bootstrap: parse args / tauri-plugin-cli
alt GUI launch (no CLI intent)
Bootstrap->>TauriApp: continue setup
TauriApp->>TauriApp: open_main_window()
else CLI intent or TTY
Bootstrap->>State: build minimal AppState
Bootstrap->>Dispatch: run_subcommand or enter REPL
loop REPL / one-shot
Shell->>Dispatch: input line
Dispatch->>Handler: classify & route (Native / Agent / Unknown)
Handler->>Handler: execute (ask, logs, help, etc.)
Handler->>Sink: produce CliReply
Sink->>Shell: render or emit JSON envelope
end
end
sequenceDiagram
participant Telegram as Telegram User
participant BotSvc as Bot Service
participant Bridge as Telegram CLI Bridge
participant Dispatch as CLI Dispatcher
participant Handler as CLI Handler
participant State as AppState
Telegram->>BotSvc: message "$some cli line"
BotSvc->>Bridge: strip_dollar_cli_payload()
Bridge->>State: warm MCP registry (best-effort)
Bridge->>Dispatch: dispatch_line(payload, ctx=telegram)
Dispatch->>Handler: native/agent routing (telegram constraints applied)
Handler->>Bridge: CliReply
Bridge->>BotSvc: format + chunk for Telegram
BotSvc->>Telegram: send chunked replies
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/modules/bot/api/index.ts (1)
28-35:⚠️ Potential issue | 🟡 MinorSilent delete failures lose the only diagnostic signal.
For
auditListFilesandauditReadFile, the UI surfaces an error state (retry banner / empty message) whennullis returned, so droppingconsole.erroris fine.auditDeleteFile, however, is consumed atAuditLogPanel.tsxLines 171–181 as a silent no-op onfalse— with theconsole.erroralso removed, a failed delete is now invisible to both the user and a developer inspecting devtools. Consider keeping aconsole.warn/console.errorhere, or surfacing a toast/error state in the panel.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/bot/api/index.ts` around lines 28 - 35, The delete path in auditDeleteFile currently swallows errors and returns false silently; restore diagnostic logging or surface an error so failures are visible. In the catch block of auditDeleteFile, log the caught error (e.g., via console.error or console.warn with context "auditDeleteFile failed") and/or rethrow or return a richer error indicator so AuditLogPanel (which consumes auditDeleteFile) can show a toast or error state; update auditDeleteFile's catch to capture the exception (e) and emit a clear log/message before returning false so developers and users can detect failed deletes.src-tauri/Cargo.toml (1)
3-3:⚠️ Potential issue | 🟡 MinorSync Rust crate version with frontend and Tauri configuration.
src-tauri/Cargo.tomldeclaresversion = "1.0.2"while bothpackage.jsonandsrc-tauri/tauri.conf.jsondeclareversion = "1.0.3". UpdateCargo.tomlto1.0.3to maintain consistency across all three manifest files and avoid version mismatch in--versionoutput.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/Cargo.toml` at line 3, The Cargo.toml currently declares version = "1.0.2" which is out of sync with the other manifests; update the version field in src-tauri Cargo.toml by changing the version = "1.0.2" entry to version = "1.0.3" so it matches package.json and tauri.conf.json and prevents --version mismatches.
🧹 Nitpick comments (18)
cli_plan.md (2)
273-279: Also update the doc sync checklist to reflect what this PR already did.This PR updates
AGENTS.mdto referencemodules/agent/but the checklist still listsdoc/README.mdanddoc/architecture/README.mdas needing the same fix. Worth either (a) including those doc updates in this PR so the checklist can be trimmed, or (b) tightening the bullets to track only what remains outstanding so readers know which items are still open.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli_plan.md` around lines 273 - 279, Update the documentation sync checklist to reflect the changes already made by this PR: either remove or mark as completed the items that were fixed (e.g., the AGENTS.md change referencing modules/agent/), and tighten the remaining bullets to only list outstanding work such as updates still needed in doc/README.md and doc/architecture/README.md (or include those updates in this PR and then clear them); reference AGENTS.md and the modules/agent/ path in your edits to ensure readers can verify which items are done versus still open.
1-5: Consider placing this plan underdoc/rather than repo root.The repo's documentation hub is
doc/README.md(per the architecture/documentation conventions). Droppingcli_plan.mdat the repo root bypasses that index and makes the plan harder to discover/maintain alongsidedoc/architecture/README.mdand thedoc/guides/cli.mdmentioned in Section 15. Consider moving it to something likedoc/architecture/cli-plan.md(ordoc/guides/cli-plan.md) and linking it fromdoc/README.md.Based on learnings: "Use doc/README.md as the main documentation index (topics, feature map, API overview)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli_plan.md` around lines 1 - 5, Move cli_plan.md into the repository documentation tree and update references: relocate the file into doc/architecture/ (e.g., doc/architecture/cli-plan.md) or doc/guides/ (e.g., doc/guides/cli-plan.md), add an entry/link to it from doc/README.md so it appears in the documentation index, and update any mentions in doc/architecture/README.md or doc/guides/cli.md to point to the new path to keep discoverability and maintainability consistent with the repo’s documentation conventions.src-tauri/src/modules/ollama/service.rs (1)
204-239: Consider reusingpost_chat/ChatOptionsto avoid a parallel payload builder.
touch_activate_modelhand-rolls a payload that overlaps withbuild_payload(model,messages,stream,keep_alive,options.num_predict,options.num_ctx). A thin wrapper that constructs a minimalChatOptions { num_ctx: 2048, num_predict: Some(1), keep_alive: "30m", .. }plus a one-message history and callspost_chatwould keep payload shape in one place if Ollama's/api/chatschema changes — the only unique bit here is the longer timeout.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/modules/ollama/service.rs` around lines 204 - 239, touch_activate_model duplicates the chat payload construction instead of reusing existing chat helpers; refactor it to build a minimal ChatOptions (set num_ctx=2048, num_predict=Some(1), keep_alive="30m") and a single-message history, then call post_chat (or the existing build_payload/post_chat pathway) with a longer timeout rather than hand-crafting the JSON; update touch_activate_model to delegate payload creation to build_payload/ChatOptions and only override the timeout before sending so payload shape stays centralized.src-tauri/src/modules/cli/banner.rs (1)
4-28: Optional: Unicode glyphs may render as boxes on legacy Windows consoles.Characters like
◘,▓,♣,○,•are outside ASCII and can render as replacement boxes incmd.exewithout a Unicode-capable font/codepage (UTF-8 chcp 65001). Modern Windows Terminal is fine. If you want maximal portability for the welcome banner, consider a fallback ASCII banner when!io::stdout().is_terminal()or on Windows with a legacy code page — otherwise this is cosmetic and can be deferred.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/modules/cli/banner.rs` around lines 4 - 28, The banner constant CLI_WELCOME uses non-ASCII glyphs that may render as boxes on legacy Windows consoles; add a portable fallback path: detect when stdout is not a true terminal or when running on Windows with a legacy code page (e.g., check cfg!(windows) plus current code page/UTF-8 support) or when io::stdout().is_terminal() is false, and return a simple ASCII-only banner instead of CLI_WELCOME; implement this detection where the banner is printed (caller code that references CLI_WELCOME) or by replacing CLI_WELCOME with a function (e.g., get_cli_banner()) that performs the runtime checks and returns either the Unicode banner or an ASCII fallback.src-tauri/Cargo.toml (1)
37-38: Verify dependency versions are current / compatible.
tauri-plugin-cli = "2"should track the rest of the Tauri 2 plugin surface — worth confirming the current published minor for reproducibility.rustyline = "14"is a recent major with a few breaking changes vs. 13; confirm it still fits your REPL usage inmodules/cli/repl.rs.syntectwithdefault-features = false+regex-fancyis the right call to avoid theonig(C-based) dependency, and keeps builds portable; nice.What is the latest stable version of the tauri-plugin-cli crate on crates.io?What is the latest stable version of the rustyline crate on crates.io?Also applies to: 43-43
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/Cargo.toml` around lines 37 - 38, The Cargo.toml pins for tauri-plugin-cli and rustyline need verification and possibly updating: check the latest stable crates.io versions for tauri-plugin-cli and rustyline and update the Cargo.toml entries (tauri-plugin-cli = "2" and rustyline = "14") to the appropriate published minors to ensure compatibility; if you upgrade rustyline, review and adapt modules/cli/repl.rs for any API/breaking changes introduced in rustyline 14 (adjust imports, Editor/Completer/validate APIs as needed), then run cargo update and cargo build/test to confirm; leave the syntect configuration (default-features = false + regex-fancy) as-is if it avoids onig and builds are portable.package.json (1)
13-14: Consider keepingcliusage aligned with the docs.
bun run cliinvokescargo run --manifest-path src-tauri/Cargo.toml --, which builds in debug and prints Cargo noise on every invocation. If the guide showcases this as the canonical developer entry point (per doc/guides/cli.md), consider adding-qto keep output clean, e.g.:♻️ Optional tweak
- "cli": "cargo run --manifest-path src-tauri/Cargo.toml --", + "cli": "cargo run -q --manifest-path src-tauri/Cargo.toml --",No functional concern with
cli:test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 13 - 14, The "cli" npm script currently runs "cargo run --manifest-path src-tauri/Cargo.toml --" which prints Cargo build noise each time; update the "cli" script to pass the quiet flag to Cargo (e.g., add "-q" to the cargo run invocation) so developer entrypoint output matches the guide while leaving "cli:test" unchanged; modify the "cli" script string only (reference: the "cli" script entry in package.json, keep "cli:test" as-is).src-tauri/src/infrastructure/http_server.rs (2)
410-414: Handler is fine; optionally drop the unusedStateextractor.
_stateis unused, and sinceCOMMANDSis a static slice ofCopyitems,to_vec()is a cheap memcpy suitable for a per-request clone. Minor ergonomics tweak:♻️ Optional simplification
-async fn handle_cli_commands_list(State(_state): State<AppState>) -> Json<CliCommandsResponse> { +async fn handle_cli_commands_list() -> Json<CliCommandsResponse> { Json(CliCommandsResponse { commands: cli_commands::COMMANDS.to_vec(), }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/infrastructure/http_server.rs` around lines 410 - 414, The handler handle_cli_commands_list currently accepts an unused State extractor (State(_state): State<AppState>); remove that parameter from the function signature so the handler becomes async fn handle_cli_commands_list() -> Json<CliCommandsResponse>, leaving the body that returns Json(CliCommandsResponse { commands: cli_commands::COMMANDS.to_vec() }) unchanged; also update any route registration references if they expect a matching handler signature.
4-4: Rename thebot_agentalias to reflect the new module location.Post-reorg,
agentno longer lives underbot/, so aliasing it asbot_agentis misleading to future readers. A plainuse crate::modules::agent;(no alias) and updating the single call site on line 2196 would be clearer.♻️ Proposed refactor
-use crate::modules::agent as bot_agent; +use crate::modules::agent;And at line 2196:
- bot_agent::run_system_turn(&state, &prompt, skills_filter), + agent::run_system_turn(&state, &prompt, skills_filter),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/infrastructure/http_server.rs` at line 4, Update the import alias so it matches the module's new location: replace "use crate::modules::agent as bot_agent;" with "use crate::modules::agent;" and then update the single call site that currently references "bot_agent::" to use "agent::" instead (keep the same function/identifier name after the module prefix). Ensure no other references to "bot_agent" remain.src/modules/cli/api/index.ts (1)
4-10: Optional: preserve status error detail instead of collapsing tonull.
cliShimStatuscurrently swallows every invoke error intonull, so the UI can only display a generic "invoke failed" message. If you ever surface a structured error fromcli_shim::status()(e.g., permission/home-dir resolution failure), this wrapper will hide it. Cheap to keep symmetric withcliShimInstall/cliShimRemove:♻️ Return a discriminated union for status
-export async function cliShimStatus(): Promise<CliShimStatus | null> { - try { - return await invoke<CliShimStatus>("cli_shim_status"); - } catch { - return null; - } -} +export async function cliShimStatus(): Promise< + { ok: true; status: CliShimStatus } | { ok: false; error: string } +> { + try { + const status = await invoke<CliShimStatus>("cli_shim_status"); + return { ok: true, status }; + } catch (e) { + const msg = e instanceof Error ? e.message : String(e); + return { ok: false, error: msg }; + } +}Note this requires updating
CliCommandsPanel.refreshShimto destructure the new shape.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/cli/api/index.ts` around lines 4 - 10, The cliShimStatus function currently swallows invoke errors and returns null; change it to return a discriminated union that preserves error details (e.g., type CliShimStatusResult = { ok: CliShimStatus } | { error: unknown }) instead of Promise<CliShimStatus | null>, then modify cliShimStatus to catch the error and return { error: err } while successful invokes return { ok: result }; update callers such as CliCommandsPanel.refreshShim to destructure the new shape (check for .ok vs .error) and handle/display structured error information.src-tauri/tests/cli_oneshot.rs (1)
9-22: Fallback lookup only coverstarget/debug.The primary path via
CARGO_BIN_EXE_pengineis what Cargo normally provides for integration tests, so this rarely matters in practice. The fallback, however, silently ignoresCARGO_TARGET_DIRand thereleaseprofile — anyone runningcargo test --release --test cli_oneshotafter acargo buildwill hit the "binary missing" assertion even though a binary exists.♻️ Honor `CARGO_TARGET_DIR` and try `release` as a secondary fallback
fn pengine_exe() -> PathBuf { if let Some(p) = std::env::var_os("CARGO_BIN_EXE_pengine") { return PathBuf::from(p); } - let dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")); - #[cfg(target_os = "windows")] - { - dir.join("target").join("debug").join("pengine.exe") - } - #[cfg(not(target_os = "windows"))] - { - dir.join("target").join("debug").join("pengine") - } + let target_dir = std::env::var_os("CARGO_TARGET_DIR") + .map(PathBuf::from) + .unwrap_or_else(|| PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("target")); + let exe_name = if cfg!(target_os = "windows") { "pengine.exe" } else { "pengine" }; + for profile in ["debug", "release"] { + let p = target_dir.join(profile).join(exe_name); + if p.exists() { + return p; + } + } + target_dir.join("debug").join(exe_name) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/tests/cli_oneshot.rs` around lines 9 - 22, The fallback in pengine_exe currently only looks in manifest_dir/target/debug; update pengine_exe to honor CARGO_TARGET_DIR (fall back to manifest_dir/target if unset) and attempt both debug and release binaries (prefer debug, then release) before failing; keep the initial CARGO_BIN_EXE_pengine check, and ensure Windows filenames use "pengine.exe" while other OSes use "pengine", trying both profiles in that order.scripts/pengine (1)
6-7: Optional: resolve symlinks when computingROOT.If a user drops a symlink to this script into a PATH directory (
ln -s /repo/scripts/pengine ~/.local/bin/pengine),dirname "$0"returns the symlink's parent, so$ROOTpoints outside the repo and$EXEwon't be found — the error message then prints a nonsensicalcargo build --manifest-pathhint. The README suggests putting the scripts dir on PATH directly, so this is defensive only.♻️ Symlink-safe root resolution (portable, no `readlink -f`)
-ROOT="$(cd "$(dirname "$0")/.." && pwd)" +SELF="$0" +# Follow symlinks to locate the real script +while [ -L "$SELF" ]; do + TARGET="$(readlink "$SELF")" + case "$TARGET" in + /*) SELF="$TARGET" ;; + *) SELF="$(dirname "$SELF")/$TARGET" ;; + esac +done +ROOT="$(cd "$(dirname "$SELF")/.." && pwd)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/pengine` around lines 6 - 7, The ROOT computation uses dirname "$0" which breaks when the script is invoked via a symlink; update the startup logic to resolve symlinks for the script path before computing ROOT (e.g. use a SOURCE variable, loop while [ -L "$SOURCE" ] to follow links and set DIR=$(cd "$(dirname "$SOURCE")/.." && pwd) ), then set EXE based on that resolved ROOT; modify the lines that currently set ROOT and EXE to use the resolved-source approach so EXE="$ROOT/src-tauri/target/debug/pengine" will point into the repo even when the script is symlinked.src-tauri/src/modules/bot/commands.rs (1)
117-132: LGTM — one cohesion nit.The three wrappers delegate cleanly to
cli_shim::*and the error-to-Stringmapping matches what the TS layer expects. Purely organizational: these commands live inmodules/bot/commands.rsbut operate only onmodules::cli::shim— ifbot/commands.rsis meant to remain bot-focused, a follow-up move into amodules/cli/commands.rs(ormodules/cli/shim.rs::*_commandwrappers) and updatinggenerate_handler!would improve cohesion. Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/modules/bot/commands.rs` around lines 117 - 132, These three Tauri command wrappers (cli_shim_status, cli_shim_install, cli_shim_remove) only delegate to cli_shim::status, cli_shim::install_shim, and cli_shim::remove_shim and should be moved out of modules::bot::commands into a CLI-focused module (e.g., modules::cli::commands or modules::cli::shim.rs) for cohesion; create the new file with those three #[tauri::command] functions, remove them from bot::commands, and update the handler registration (generate_handler! or wherever handlers are collected) to export the new CLI commands so the rest of the app continues to call the same functions.doc/guides/cli.md (1)
143-149: Add a language to the two unannotated fences (MD040).Line 143 (REPL session example) and line 167 (
pengine logs --tailexample) trip markdownlint MD040. Addingtext(orconsole) keeps the rendering identical while silencing the linter.📝 Proposed fix
-``` +```text ❯ what changed in the fetch tool?-``` +```text pengine logs --tail 100The remaining MD038 warnings on lines 151-155/184 look intentional (the literal
⎿prefix is the content being documented) and aren't worth escaping.Also applies to: 167-169
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/guides/cli.md` around lines 143 - 149, Update the two unannotated fenced code blocks in doc/guides/cli.md (the REPL session example starting with "❯ what changed in the fetch tool?" and the "pengine logs --tail" example) to add a language specifier such as "text" (or "console") after the opening triple backticks so the fences become annotated and markdownlint MD040 is silenced.src-tauri/src/modules/cli/repl.rs (2)
94-99:history_pathsilently falls back to CWD whenstore_path.parent()isNone.If
state.store_pathever lacks a parent (edge cases: relative bare filename, root-level path), history gets written to./cli_historyin whatever directory the user launchedpenginefrom — different per invocation and easily missed. In practicestore_pathis always a full path toconnection.json, so this is defensive, but consider failing closed (skip history) instead of writing to CWD:fn history_path(store_path: &std::path::Path) -> PathBuf { store_path .parent() .map(|p| p.join("cli_history")) .unwrap_or_else(|| PathBuf::from("cli_history")) }Alternatively, log a warning when the fallback path is used so the behavior is at least visible.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/modules/cli/repl.rs` around lines 94 - 99, history_path currently falls back to "./cli_history" when store_path.parent() is None, causing writes to the current working directory; change history_path to return Option<PathBuf> (e.g., fn history_path(store_path: &Path) -> Option<PathBuf>) and return None when parent() is None so callers can skip creating/writing history instead of using CWD, and update the caller that uses state.store_path to handle None by skipping history (and optionally emit a warning via the existing logger) rather than creating a cli_history file in the process CWD.
37-39: MCP warmup "skipped" rendered as red error may overstate severity.
CliReply::errorrenders in red and is prefixed with error styling byrender_reply, but the message itself says "skipped" and is explicitly non-fatal. For first-time users who haven't configured MCP yet, a red banner on every REPL start is louder than warranted. Consider downgrading to plain text (or introducing a warning kind if one is planned) so the welcome screen stays calm when MCP is simply absent.- if let Err(e) = mcp_service::rebuild_registry_into_state(state).await { - sink.render(&CliReply::error(format!("mcp warmup skipped: {e}"))); - } + if let Err(e) = mcp_service::rebuild_registry_into_state(state).await { + sink.render(&CliReply::text(format!("mcp warmup skipped: {e}"))); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/modules/cli/repl.rs` around lines 37 - 39, The current code calls mcp_service::rebuild_registry_into_state(...) and renders failures via sink.render(&CliReply::error(...)), which shows a red error banner for a non-fatal "skipped" condition; change the rendered reply to a non-error variant (e.g., CliReply::info or another plain/text variant provided by CliReply) so the message remains visible but not styled as a fatal error, keeping the original message text (mcp warmup skipped: {e}) and the same sink.render call site.src-tauri/src/modules/cli/telegram_bridge.rs (1)
37-60: Fence markers can end up split across Telegram chunks for large replies.
telegram_cli_reply_chunksformats the reply (adding`lang` …) and only then splits by char budget. For any fenced reply larger thanTELEGRAM_CLI_CHUNK_BUDGET(2000 chars), the opening fence lives in chunk 1 and the closing fence in the last chunk, with intermediate chunks containing neither. Today this is purely cosmetic becausesend_messageinbot/service.rs` doesn't set a parse mode, so users see literal backticks — but if MarkdownV2/HTML is ever enabled, Telegram will render the first chunk as an unterminated pre-block and the last chunk as a stray```. Worth considering re-fencing each chunk (or splitting the body first, then wrapping each piece) before adding parse mode.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/modules/cli/telegram_bridge.rs` around lines 37 - 60, The current flow in telegram_cli_reply_chunks formats the whole reply (via format_reply_body_telegram and fenced) then calls split_by_chars, which can split fenced code blocks across chunks; change telegram_cli_reply_chunks to handle fenced replies specially: when reply.kind is ReplyKind::CodeBlock/Log/Diff, first split the raw body using split_by_chars (or a new splitter) into pieces sized to TELEGRAM_CLI_CHUNK_BUDGET minus the overhead of the opening/closing fences, then wrap each piece with fenced(lang, piece) so every chunk has its own complete fence; for ReplyKind::Text and ReplyKind::Error keep the existing behavior. Update references to split_by_chars, TELEGRAM_CLI_CHUNK_BUDGET, format_reply_body_telegram, fenced, and ReplyKind to implement this.src-tauri/src/modules/cli/syntax_highlight.rs (1)
40-48: Remove redundantfind_syntax_by_extensioncall beforefind_syntax_by_token.
SyntaxSet::find_syntax_by_tokeninternally tries extension lookup first viafind_syntax_by_extension, then falls back to name matching, so the explicit call on line 45 is redundant and never catches anything thatfind_syntax_by_tokenwouldn't. Simplify to:Diff
- ss.find_syntax_by_extension(l) - .or_else(|| ss.find_syntax_by_token(l)) - .unwrap_or_else(|| ss.find_syntax_plain_text()) + ss.find_syntax_by_token(l) + .unwrap_or_else(|| ss.find_syntax_plain_text())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/modules/cli/syntax_highlight.rs` around lines 40 - 48, The function resolve_syntax currently calls ss.find_syntax_by_extension(l) before ss.find_syntax_by_token(l) which is redundant because SyntaxSet::find_syntax_by_token already attempts extension lookup internally; update resolve_syntax to remove the explicit find_syntax_by_extension call and instead just call ss.find_syntax_by_token(l).or_else(|| ss.find_syntax_plain_text()), keeping the initial empty-string check and returning ss.find_syntax_plain_text() when l.is_empty(); reference resolve_syntax, SyntaxSet::find_syntax_by_token, find_syntax_by_extension, and find_syntax_plain_text when making the change.src-tauri/src/modules/cli/output.rs (1)
181-195: Replace rawisattyFFI withstd::io::IsTerminal— also fixes Windows behavior.This function has two issues:
- On non-Unix (
#[cfg(not(unix))]), it unconditionally returnstrue, so Windows always outputs ANSI colors even when stdout is redirected to a pipe, file, or CI log. This corrupts downstream consumers.std::io::IsTerminalis already imported in this file and is stable since Rust 1.70 — wrapping a platform-specificextern "C" fn isattywithunsafeis unnecessary and doesn't handle Windows correctly.♻️ Suggested refactor
-fn is_terminal_stdout() -> bool { - // Avoids pulling a new crate; the file-descriptor check is enough for color gating. - #[cfg(unix)] - unsafe { - // SAFETY: `isatty` takes a raw fd and has no memory effects. - extern "C" { - fn isatty(fd: i32) -> i32; - } - isatty(1) == 1 - } - #[cfg(not(unix))] - { - true - } -} +fn is_terminal_stdout() -> bool { + std::io::stdout().is_terminal() +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/modules/cli/output.rs` around lines 181 - 195, The is_terminal_stdout function currently uses an unsafe Unix-only isatty FFI and returns true on non-Unix, causing incorrect ANSI output on Windows; replace the whole cfg-guarded FFI with the stable IsTerminal API by returning std::io::stdout().is_terminal() (the IsTerminal trait is already imported), remove the unsafe extern block and #[cfg] branches, and ensure the function simply queries stdout via IsTerminal so it behaves correctly cross-platform and when stdout is redirected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli_plan.md`:
- Line 3: Update the inconsistent dates so the document reflects a single
authoritative snapshot: change either the header "Last updated: 2026-04-23" or
the acceptance-criteria table title "Status on 2026-04-24" so they match (pick
the correct canonical date), e.g., replace "Last updated: 2026-04-23" with "Last
updated: 2026-04-24" or vice versa; ensure the two strings ("Last updated:"
header and the "Status on ..." table title) are identical throughout the file.
- Line 18: The bullet "Same feature that claude code -
https://github.com/anthropics/claude-code" is a dangling, unclear sentence;
replace it with a clear, actionable phrasing or remove it. Suggested
replacement: "Parity target: feature surface comparable to Anthropic's Claude
Code CLI (https://github.com/anthropics/claude-code)". Update the CLI plan entry
containing that exact string to use the replacement text (or delete the line) so
the bullet reads as a complete, scoped target.
In `@package.json`:
- Line 2: The package.json "name" field uses uppercase "Pengine" which violates
npm's lowercase requirement; update the package.json by changing the "name"
property value from "Pengine" to a lowercase, URL-safe string like "pengine" so
it conforms to npm spec and avoids downstream tooling issues (edit the "name"
key in package.json).
In `@src-tauri/src/modules/cli/handlers.rs`:
- Around line 108-138: The current loop applies each kv pair immediately, so a
later invalid pair can fail after earlier successful writes; instead first
validate and parse all kvs without writing (iterate kvs, split_once, trim, match
keys and parse values using user_settings::save_skills_hint_max_bytes or the
same validation logic but without persisting) collecting either parsed validated
entries or errors; if any error occurred return a CliReply::error listing the
invalid entries, otherwise perform the actual writes—call
user_settings::save_skills_hint_max_bytes and then update
state.skills_hint_max_bytes under the write lock for each validated entry, and
finally return CliReply::code with a clear applied list (the applied
Vec<String>).
- Around line 468-483: follow_logs_from_broadcast currently prints events with
println! and bypasses the configured sink (breaking --json); change the function
to accept the sink (or a flag indicating JSON mode) passed from
dispatch_stateful/run_subcommand, and for each received event emit via the sink
pipeline (e.g., construct a CliReply::log(...) and call sink.render or format
with the JsonSink envelope) instead of println!, preserving the existing final
CliReply::code("bash", "log stream closed"); ensure subscription errors
(Lagged/Closed) are still handled but routed through the sink when in JSON mode.
In `@src-tauri/src/modules/cli/shim.rs`:
- Around line 24-43: The PATH comparison in path_env_contains_dir is currently
case-sensitive, causing mismatches on Windows; update path normalization and
comparison (functions normalize_path_for_compare and path_env_contains_dir) to
perform a case-insensitive comparison on Windows by normalizing entries to a
stable representation (e.g., convert the path to a String via to_string_lossy()
and .to_lowercase() or otherwise lowercasing components) before equality checks;
ensure you still trim entries and keep the existing trailing-slash/empty-segment
handling so normalize_path_for_compare returns the same canonical form used for
both the input dir and each PATH entry, but apply lowercasing only on
cfg!(windows).
In `@src-tauri/src/modules/ollama/service.rs`:
- Around line 223-232: The code calls resp.json().await before checking HTTP
status, which can hide non-JSON error responses; change the flow to first read
the response as text (e.g., let text = resp.text().await.map_err(|e|
e.to_string())?), then check status: if !status.is_success() attempt to parse
that text into serde_json::Value to extract body.get("error").and_then(|v|
v.as_str()) and include that or the raw text in the returned error
(format!("ollama chat HTTP {status}: {err_or_text}")), otherwise parse the text
into JSON for the success path; apply the same pattern where resp.json().await,
body, and err are used (and consider mirroring the change in the post_chat
helper).
---
Outside diff comments:
In `@src-tauri/Cargo.toml`:
- Line 3: The Cargo.toml currently declares version = "1.0.2" which is out of
sync with the other manifests; update the version field in src-tauri Cargo.toml
by changing the version = "1.0.2" entry to version = "1.0.3" so it matches
package.json and tauri.conf.json and prevents --version mismatches.
In `@src/modules/bot/api/index.ts`:
- Around line 28-35: The delete path in auditDeleteFile currently swallows
errors and returns false silently; restore diagnostic logging or surface an
error so failures are visible. In the catch block of auditDeleteFile, log the
caught error (e.g., via console.error or console.warn with context
"auditDeleteFile failed") and/or rethrow or return a richer error indicator so
AuditLogPanel (which consumes auditDeleteFile) can show a toast or error state;
update auditDeleteFile's catch to capture the exception (e) and emit a clear
log/message before returning false so developers and users can detect failed
deletes.
---
Nitpick comments:
In `@cli_plan.md`:
- Around line 273-279: Update the documentation sync checklist to reflect the
changes already made by this PR: either remove or mark as completed the items
that were fixed (e.g., the AGENTS.md change referencing modules/agent/), and
tighten the remaining bullets to only list outstanding work such as updates
still needed in doc/README.md and doc/architecture/README.md (or include those
updates in this PR and then clear them); reference AGENTS.md and the
modules/agent/ path in your edits to ensure readers can verify which items are
done versus still open.
- Around line 1-5: Move cli_plan.md into the repository documentation tree and
update references: relocate the file into doc/architecture/ (e.g.,
doc/architecture/cli-plan.md) or doc/guides/ (e.g., doc/guides/cli-plan.md), add
an entry/link to it from doc/README.md so it appears in the documentation index,
and update any mentions in doc/architecture/README.md or doc/guides/cli.md to
point to the new path to keep discoverability and maintainability consistent
with the repo’s documentation conventions.
In `@doc/guides/cli.md`:
- Around line 143-149: Update the two unannotated fenced code blocks in
doc/guides/cli.md (the REPL session example starting with "❯ what changed in the
fetch tool?" and the "pengine logs --tail" example) to add a language specifier
such as "text" (or "console") after the opening triple backticks so the fences
become annotated and markdownlint MD040 is silenced.
In `@package.json`:
- Around line 13-14: The "cli" npm script currently runs "cargo run
--manifest-path src-tauri/Cargo.toml --" which prints Cargo build noise each
time; update the "cli" script to pass the quiet flag to Cargo (e.g., add "-q" to
the cargo run invocation) so developer entrypoint output matches the guide while
leaving "cli:test" unchanged; modify the "cli" script string only (reference:
the "cli" script entry in package.json, keep "cli:test" as-is).
In `@scripts/pengine`:
- Around line 6-7: The ROOT computation uses dirname "$0" which breaks when the
script is invoked via a symlink; update the startup logic to resolve symlinks
for the script path before computing ROOT (e.g. use a SOURCE variable, loop
while [ -L "$SOURCE" ] to follow links and set DIR=$(cd "$(dirname
"$SOURCE")/.." && pwd) ), then set EXE based on that resolved ROOT; modify the
lines that currently set ROOT and EXE to use the resolved-source approach so
EXE="$ROOT/src-tauri/target/debug/pengine" will point into the repo even when
the script is symlinked.
In `@src-tauri/Cargo.toml`:
- Around line 37-38: The Cargo.toml pins for tauri-plugin-cli and rustyline need
verification and possibly updating: check the latest stable crates.io versions
for tauri-plugin-cli and rustyline and update the Cargo.toml entries
(tauri-plugin-cli = "2" and rustyline = "14") to the appropriate published
minors to ensure compatibility; if you upgrade rustyline, review and adapt
modules/cli/repl.rs for any API/breaking changes introduced in rustyline 14
(adjust imports, Editor/Completer/validate APIs as needed), then run cargo
update and cargo build/test to confirm; leave the syntect configuration
(default-features = false + regex-fancy) as-is if it avoids onig and builds are
portable.
In `@src-tauri/src/infrastructure/http_server.rs`:
- Around line 410-414: The handler handle_cli_commands_list currently accepts an
unused State extractor (State(_state): State<AppState>); remove that parameter
from the function signature so the handler becomes async fn
handle_cli_commands_list() -> Json<CliCommandsResponse>, leaving the body that
returns Json(CliCommandsResponse { commands: cli_commands::COMMANDS.to_vec() })
unchanged; also update any route registration references if they expect a
matching handler signature.
- Line 4: Update the import alias so it matches the module's new location:
replace "use crate::modules::agent as bot_agent;" with "use
crate::modules::agent;" and then update the single call site that currently
references "bot_agent::" to use "agent::" instead (keep the same
function/identifier name after the module prefix). Ensure no other references to
"bot_agent" remain.
In `@src-tauri/src/modules/bot/commands.rs`:
- Around line 117-132: These three Tauri command wrappers (cli_shim_status,
cli_shim_install, cli_shim_remove) only delegate to cli_shim::status,
cli_shim::install_shim, and cli_shim::remove_shim and should be moved out of
modules::bot::commands into a CLI-focused module (e.g., modules::cli::commands
or modules::cli::shim.rs) for cohesion; create the new file with those three
#[tauri::command] functions, remove them from bot::commands, and update the
handler registration (generate_handler! or wherever handlers are collected) to
export the new CLI commands so the rest of the app continues to call the same
functions.
In `@src-tauri/src/modules/cli/banner.rs`:
- Around line 4-28: The banner constant CLI_WELCOME uses non-ASCII glyphs that
may render as boxes on legacy Windows consoles; add a portable fallback path:
detect when stdout is not a true terminal or when running on Windows with a
legacy code page (e.g., check cfg!(windows) plus current code page/UTF-8
support) or when io::stdout().is_terminal() is false, and return a simple
ASCII-only banner instead of CLI_WELCOME; implement this detection where the
banner is printed (caller code that references CLI_WELCOME) or by replacing
CLI_WELCOME with a function (e.g., get_cli_banner()) that performs the runtime
checks and returns either the Unicode banner or an ASCII fallback.
In `@src-tauri/src/modules/cli/output.rs`:
- Around line 181-195: The is_terminal_stdout function currently uses an unsafe
Unix-only isatty FFI and returns true on non-Unix, causing incorrect ANSI output
on Windows; replace the whole cfg-guarded FFI with the stable IsTerminal API by
returning std::io::stdout().is_terminal() (the IsTerminal trait is already
imported), remove the unsafe extern block and #[cfg] branches, and ensure the
function simply queries stdout via IsTerminal so it behaves correctly
cross-platform and when stdout is redirected.
In `@src-tauri/src/modules/cli/repl.rs`:
- Around line 94-99: history_path currently falls back to "./cli_history" when
store_path.parent() is None, causing writes to the current working directory;
change history_path to return Option<PathBuf> (e.g., fn history_path(store_path:
&Path) -> Option<PathBuf>) and return None when parent() is None so callers can
skip creating/writing history instead of using CWD, and update the caller that
uses state.store_path to handle None by skipping history (and optionally emit a
warning via the existing logger) rather than creating a cli_history file in the
process CWD.
- Around line 37-39: The current code calls
mcp_service::rebuild_registry_into_state(...) and renders failures via
sink.render(&CliReply::error(...)), which shows a red error banner for a
non-fatal "skipped" condition; change the rendered reply to a non-error variant
(e.g., CliReply::info or another plain/text variant provided by CliReply) so the
message remains visible but not styled as a fatal error, keeping the original
message text (mcp warmup skipped: {e}) and the same sink.render call site.
In `@src-tauri/src/modules/cli/syntax_highlight.rs`:
- Around line 40-48: The function resolve_syntax currently calls
ss.find_syntax_by_extension(l) before ss.find_syntax_by_token(l) which is
redundant because SyntaxSet::find_syntax_by_token already attempts extension
lookup internally; update resolve_syntax to remove the explicit
find_syntax_by_extension call and instead just call
ss.find_syntax_by_token(l).or_else(|| ss.find_syntax_plain_text()), keeping the
initial empty-string check and returning ss.find_syntax_plain_text() when
l.is_empty(); reference resolve_syntax, SyntaxSet::find_syntax_by_token,
find_syntax_by_extension, and find_syntax_plain_text when making the change.
In `@src-tauri/src/modules/cli/telegram_bridge.rs`:
- Around line 37-60: The current flow in telegram_cli_reply_chunks formats the
whole reply (via format_reply_body_telegram and fenced) then calls
split_by_chars, which can split fenced code blocks across chunks; change
telegram_cli_reply_chunks to handle fenced replies specially: when reply.kind is
ReplyKind::CodeBlock/Log/Diff, first split the raw body using split_by_chars (or
a new splitter) into pieces sized to TELEGRAM_CLI_CHUNK_BUDGET minus the
overhead of the opening/closing fences, then wrap each piece with fenced(lang,
piece) so every chunk has its own complete fence; for ReplyKind::Text and
ReplyKind::Error keep the existing behavior. Update references to
split_by_chars, TELEGRAM_CLI_CHUNK_BUDGET, format_reply_body_telegram, fenced,
and ReplyKind to implement this.
In `@src-tauri/src/modules/ollama/service.rs`:
- Around line 204-239: touch_activate_model duplicates the chat payload
construction instead of reusing existing chat helpers; refactor it to build a
minimal ChatOptions (set num_ctx=2048, num_predict=Some(1), keep_alive="30m")
and a single-message history, then call post_chat (or the existing
build_payload/post_chat pathway) with a longer timeout rather than hand-crafting
the JSON; update touch_activate_model to delegate payload creation to
build_payload/ChatOptions and only override the timeout before sending so
payload shape stays centralized.
In `@src-tauri/tests/cli_oneshot.rs`:
- Around line 9-22: The fallback in pengine_exe currently only looks in
manifest_dir/target/debug; update pengine_exe to honor CARGO_TARGET_DIR (fall
back to manifest_dir/target if unset) and attempt both debug and release
binaries (prefer debug, then release) before failing; keep the initial
CARGO_BIN_EXE_pengine check, and ensure Windows filenames use "pengine.exe"
while other OSes use "pengine", trying both profiles in that order.
In `@src/modules/cli/api/index.ts`:
- Around line 4-10: The cliShimStatus function currently swallows invoke errors
and returns null; change it to return a discriminated union that preserves error
details (e.g., type CliShimStatusResult = { ok: CliShimStatus } | { error:
unknown }) instead of Promise<CliShimStatus | null>, then modify cliShimStatus
to catch the error and return { error: err } while successful invokes return {
ok: result }; update callers such as CliCommandsPanel.refreshShim to destructure
the new shape (check for .ok vs .error) and handle/display structured error
information.
🪄 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: fb8e4c7b-765d-4187-b9d1-e97a72c7b20b
⛔ Files ignored due to path filters (3)
bun.lockis excluded by!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.jsonsrc-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (47)
AGENTS.mdREADME.mdcli_plan.mddoc/README.mddoc/guides/cli.mddoc/reference/http-api.mdpackage.jsonscripts/penginesrc-tauri/Cargo.tomlsrc-tauri/capabilities/default.jsonsrc-tauri/permissions/cli-shim.tomlsrc-tauri/src/app.rssrc-tauri/src/build_info.rssrc-tauri/src/infrastructure/http_server.rssrc-tauri/src/modules/agent/mod.rssrc-tauri/src/modules/agent/search_followup.rssrc-tauri/src/modules/bot/commands.rssrc-tauri/src/modules/bot/mod.rssrc-tauri/src/modules/bot/service.rssrc-tauri/src/modules/bot/token_verify.rssrc-tauri/src/modules/cli/banner.rssrc-tauri/src/modules/cli/bootstrap.rssrc-tauri/src/modules/cli/commands.rssrc-tauri/src/modules/cli/dispatch.rssrc-tauri/src/modules/cli/handlers.rssrc-tauri/src/modules/cli/mod.rssrc-tauri/src/modules/cli/output.rssrc-tauri/src/modules/cli/repl.rssrc-tauri/src/modules/cli/router.rssrc-tauri/src/modules/cli/shim.rssrc-tauri/src/modules/cli/syntax_highlight.rssrc-tauri/src/modules/cli/telegram_bridge.rssrc-tauri/src/modules/cron/scheduler.rssrc-tauri/src/modules/cron/types.rssrc-tauri/src/modules/mod.rssrc-tauri/src/modules/ollama/service.rssrc-tauri/src/modules/skills/service.rssrc-tauri/src/shared/state.rssrc-tauri/tauri.conf.jsonsrc-tauri/tests/cli_oneshot.rssrc/modules/bot/api/index.tssrc/modules/bot/components/TerminalPreview.tsxsrc/modules/cli/api/index.tssrc/modules/cli/components/CliCommandsPanel.tsxsrc/modules/cli/index.tssrc/modules/cli/types.tssrc/pages/DashboardPage.tsx
💤 Files with no reviewable changes (2)
- src-tauri/src/build_info.rs
- src-tauri/src/modules/cron/types.rs
| @@ -0,0 +1,286 @@ | |||
| # Pengine CLI Plan (v3 - Tauri-only, implementation-corrected) | |||
|
|
|||
| > Last updated: 2026-04-23. | |||
There was a problem hiding this comment.
Date inconsistency between header and status table.
Header says "Last updated: 2026-04-23" (Line 3) while the acceptance-criteria table is titled "Status on 2026-04-24" (Line 242). Align to a single date to avoid confusion about which snapshot is authoritative.
Also applies to: 242-242
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli_plan.md` at line 3, Update the inconsistent dates so the document
reflects a single authoritative snapshot: change either the header "Last
updated: 2026-04-23" or the acceptance-criteria table title "Status on
2026-04-24" so they match (pick the correct canonical date), e.g., replace "Last
updated: 2026-04-23" with "Last updated: 2026-04-24" or vice versa; ensure the
two strings ("Last updated:" header and the "Status on ..." table title) are
identical throughout the file.
| - Native subcommands for status/config/model/bot/tools/skills/fs/logs (and `app`, etc.). | ||
| - Agent path via `ask` and REPL free text. | ||
| - Telegram `$` bridge reusing the same router + handlers. | ||
| - Same feature that claude code - https://github.com/anthropics/claude-code |
There was a problem hiding this comment.
Fix dangling bullet — grammar and missing context.
"Same feature that claude code - https://…" reads as an incomplete sentence next to concrete in-scope bullets and provides no actionable scope. Either rephrase (e.g. "Parity target: feature surface comparable to Anthropic's Claude Code CLI (link)") or drop it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli_plan.md` at line 18, The bullet "Same feature that claude code -
https://github.com/anthropics/claude-code" is a dangling, unclear sentence;
replace it with a clear, actionable phrasing or remove it. Suggested
replacement: "Parity target: feature surface comparable to Anthropic's Claude
Code CLI (https://github.com/anthropics/claude-code)". Update the CLI plan entry
containing that exact string to use the replacement text (or delete the line) so
the bullet reads as a complete, scoped target.
| @@ -1,7 +1,7 @@ | |||
| { | |||
| "name": "pengine", | |||
| "name": "Pengine", | |||
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
npm package.json name field lowercase requirement spec
💡 Result:
The npm package.json "name" field must contain only lowercase letters for new packages. Uppercase letters are not allowed. This requirement is explicitly stated in the official npm documentation: "New packages must not have uppercase letters in the name." The name also cannot contain non-URL-safe characters, must be <= 214 characters, and for unscoped packages, cannot start with . or _. This rule exists primarily due to case-sensitive filesystems (e.g., Unix), URL path sensitivity, and to avoid confusion, as package names become folder names, command-line arguments, and parts of URLs. Legacy packages published before this rule may have uppercase letters, but npm's validate-npm-package-name tool distinguishes between validForNewPackages (false if uppercase) and validForOldPackages (may be true). Publishing new packages with uppercase will fail validation. The rule applies to unscoped package names; scoped packages (e.g., @scope/name) follow the same lowercase requirement for the name part.
Citations:
- 1: https://docs.npmjs.com/cli/v11/configuring-npm/package-json/
- 2: https://docs.npmjs.com/cli/v9/configuring-npm/package-json/?v=true
- 3: https://github.com/npm/validate-npm-package-name
- 4: https://docs.npmjs.com/package-name-guidelines/
npm package name must be lowercase.
Per the npm package.json spec, name must be lowercase (and URL-safe). Even though "private": true prevents publish-time rejection, uppercase letters violate the specification and can break tools that derive artifact names, Docker image tags, or lockfile keys from the package name. Change to "pengine".
Proposed fix
- "name": "Pengine",
+ "name": "pengine",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "name": "Pengine", | |
| "name": "pengine", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` at line 2, The package.json "name" field uses uppercase
"Pengine" which violates npm's lowercase requirement; update the package.json by
changing the "name" property value from "Pengine" to a lowercase, URL-safe
string like "pengine" so it conforms to npm spec and avoids downstream tooling
issues (edit the "name" key in package.json).
Comment on lines
+108
to
+138
| let mut applied: Vec<String> = Vec::new(); | ||
| for kv in kvs { | ||
| let Some((key, value)) = kv.split_once('=') else { | ||
| return CliReply::error(format!("invalid form `{kv}`; expected `key=value`")); | ||
| }; | ||
| let key = key.trim(); | ||
| let value = value.trim(); | ||
| match key { | ||
| "skills_hint_max_bytes" => match value.parse::<u32>() { | ||
| Ok(n) => match user_settings::save_skills_hint_max_bytes(&state.store_path, n) { | ||
| Ok(clamped) => { | ||
| let mut w = state.skills_hint_max_bytes.write().await; | ||
| *w = clamped; | ||
| applied.push(format!("{key}={clamped}")); | ||
| } | ||
| Err(e) => { | ||
| return CliReply::error(format!("save failed: {e}")); | ||
| } | ||
| }, | ||
| Err(_) => { | ||
| return CliReply::error(format!("{key}: expected u32, got `{value}`")); | ||
| } | ||
| }, | ||
| other => { | ||
| return CliReply::error(format!( | ||
| "unknown setting `{other}`. Known: skills_hint_max_bytes", | ||
| )); | ||
| } | ||
| } | ||
| } | ||
| CliReply::code("bash", format!("updated: {}", applied.join(", "))) |
There was a problem hiding this comment.
config applies settings non-atomically.
If kvs contains multiple pairs (e.g. a=1 b=bogus), the first valid pair is written to disk and to state.skills_hint_max_bytes, then the second pair returns an Error without ever telling the user which ones actually landed. Consider pre-validating every pair before any write, or aggregating applied/failed into the returned message.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src-tauri/src/modules/cli/handlers.rs` around lines 108 - 138, The current
loop applies each kv pair immediately, so a later invalid pair can fail after
earlier successful writes; instead first validate and parse all kvs without
writing (iterate kvs, split_once, trim, match keys and parse values using
user_settings::save_skills_hint_max_bytes or the same validation logic but
without persisting) collecting either parsed validated entries or errors; if any
error occurred return a CliReply::error listing the invalid entries, otherwise
perform the actual writes—call user_settings::save_skills_hint_max_bytes and
then update state.skills_hint_max_bytes under the write lock for each validated
entry, and finally return CliReply::code with a clear applied list (the applied
Vec<String>).
Comment on lines
+468
to
+483
| async fn follow_logs_from_broadcast(state: &AppState) -> CliReply { | ||
| let mut rx = match state.log_tx.lock().await.as_ref() { | ||
| Some(tx) => tx.subscribe(), | ||
| None => return CliReply::error("logs: broadcast channel is closed"), | ||
| }; | ||
| loop { | ||
| match rx.recv().await { | ||
| Ok(ev) => println!("{}", format_log_line(&ev)), | ||
| Err(tokio::sync::broadcast::error::RecvError::Lagged(skipped)) => { | ||
| eprintln!("[logs lagged: {skipped} event(s) dropped]"); | ||
| } | ||
| Err(tokio::sync::broadcast::error::RecvError::Closed) => break, | ||
| } | ||
| } | ||
| CliReply::code("bash", "log stream closed") | ||
| } |
There was a problem hiding this comment.
logs --follow bypasses the chosen sink — breaks --json contract.
Each event is emitted with a bare println!, so when the user runs pengine --json logs --follow the stream is plain text timestamp [kind] message lines instead of the versioned {"v":1,"kind":"...","body":"..."} envelopes that JsonSink produces. Only the final "log stream closed" reply is funneled through the sink. Scripted consumers pinning against v will break.
Either thread the sink through to follow_logs_from_broadcast (so each event renders as a CliReply::log(...) via sink.render), or detect JSON mode from the caller and encode the envelope here.
🛠️ Suggested reshape
-async fn follow_logs_from_broadcast(state: &AppState) -> CliReply {
+async fn follow_logs_from_broadcast(state: &AppState, sink: &dyn OutputSink) -> CliReply {
let mut rx = match state.log_tx.lock().await.as_ref() {
Some(tx) => tx.subscribe(),
None => return CliReply::error("logs: broadcast channel is closed"),
};
loop {
match rx.recv().await {
- Ok(ev) => println!("{}", format_log_line(&ev)),
+ Ok(ev) => sink.render(&CliReply::log(format_log_line(&ev))),
Err(tokio::sync::broadcast::error::RecvError::Lagged(skipped)) => {
eprintln!("[logs lagged: {skipped} event(s) dropped]");
}
Err(tokio::sync::broadcast::error::RecvError::Closed) => break,
}
}
CliReply::code("bash", "log stream closed")
}(and plumb sink through from dispatch_stateful / run_subcommand.)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async fn follow_logs_from_broadcast(state: &AppState) -> CliReply { | |
| let mut rx = match state.log_tx.lock().await.as_ref() { | |
| Some(tx) => tx.subscribe(), | |
| None => return CliReply::error("logs: broadcast channel is closed"), | |
| }; | |
| loop { | |
| match rx.recv().await { | |
| Ok(ev) => println!("{}", format_log_line(&ev)), | |
| Err(tokio::sync::broadcast::error::RecvError::Lagged(skipped)) => { | |
| eprintln!("[logs lagged: {skipped} event(s) dropped]"); | |
| } | |
| Err(tokio::sync::broadcast::error::RecvError::Closed) => break, | |
| } | |
| } | |
| CliReply::code("bash", "log stream closed") | |
| } | |
| async fn follow_logs_from_broadcast(state: &AppState, sink: &dyn OutputSink) -> CliReply { | |
| let mut rx = match state.log_tx.lock().await.as_ref() { | |
| Some(tx) => tx.subscribe(), | |
| None => return CliReply::error("logs: broadcast channel is closed"), | |
| }; | |
| loop { | |
| match rx.recv().await { | |
| Ok(ev) => sink.render(&CliReply::log(format_log_line(&ev))), | |
| Err(tokio::sync::broadcast::error::RecvError::Lagged(skipped)) => { | |
| eprintln!("[logs lagged: {skipped} event(s) dropped]"); | |
| } | |
| Err(tokio::sync::broadcast::error::RecvError::Closed) => break, | |
| } | |
| } | |
| CliReply::code("bash", "log stream closed") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src-tauri/src/modules/cli/handlers.rs` around lines 468 - 483,
follow_logs_from_broadcast currently prints events with println! and bypasses
the configured sink (breaking --json); change the function to accept the sink
(or a flag indicating JSON mode) passed from dispatch_stateful/run_subcommand,
and for each received event emit via the sink pipeline (e.g., construct a
CliReply::log(...) and call sink.render or format with the JsonSink envelope)
instead of println!, preserving the existing final CliReply::code("bash", "log
stream closed"); ensure subscription errors (Lagged/Closed) are still handled
but routed through the sink when in JSON mode.
Comment on lines
+24
to
+43
| fn path_env_contains_dir(dir: &Path) -> bool { | ||
| let Ok(path_var) = std::env::var("PATH") else { | ||
| return false; | ||
| }; | ||
| let dir_norm = normalize_path_for_compare(dir); | ||
| path_var | ||
| .split(if cfg!(windows) { ';' } else { ':' }) | ||
| .any(|entry| { | ||
| let p = Path::new(entry.trim()); | ||
| !p.as_os_str().is_empty() && normalize_path_for_compare(p) == dir_norm | ||
| }) | ||
| } | ||
|
|
||
| fn normalize_path_for_compare(p: &Path) -> PathBuf { | ||
| let mut b = p.to_path_buf(); | ||
| while b.as_os_str().len() > 1 && b.file_name().is_none() { | ||
| b.pop(); | ||
| } | ||
| b | ||
| } |
There was a problem hiding this comment.
Windows PATH comparison should be case-insensitive.
On Windows, filesystem paths are case-insensitive, so comparing PATH entries verbatim as OsStr will miss matches like C:\Users\Foo\AppData\Local\Pengine\bin vs c:\users\foo\appdata\local\pengine\bin. That will cause local_bin_on_path to be false even when the shim directory is actually on PATH, triggering a misleading path_export_hint in the UI.
🛠️ Suggested fix
fn path_env_contains_dir(dir: &Path) -> bool {
let Ok(path_var) = std::env::var("PATH") else {
return false;
};
let dir_norm = normalize_path_for_compare(dir);
path_var
.split(if cfg!(windows) { ';' } else { ':' })
.any(|entry| {
let p = Path::new(entry.trim());
- !p.as_os_str().is_empty() && normalize_path_for_compare(p) == dir_norm
+ if p.as_os_str().is_empty() {
+ return false;
+ }
+ let p_norm = normalize_path_for_compare(p);
+ #[cfg(windows)]
+ {
+ p_norm.to_string_lossy().eq_ignore_ascii_case(&*dir_norm.to_string_lossy())
+ }
+ #[cfg(not(windows))]
+ {
+ p_norm == dir_norm
+ }
})
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src-tauri/src/modules/cli/shim.rs` around lines 24 - 43, The PATH comparison
in path_env_contains_dir is currently case-sensitive, causing mismatches on
Windows; update path normalization and comparison (functions
normalize_path_for_compare and path_env_contains_dir) to perform a
case-insensitive comparison on Windows by normalizing entries to a stable
representation (e.g., convert the path to a String via to_string_lossy() and
.to_lowercase() or otherwise lowercasing components) before equality checks;
ensure you still trim entries and keep the existing trailing-slash/empty-segment
handling so normalize_path_for_compare returns the same canonical form used for
both the input dir and each PATH entry, but apply lowercasing only on
cfg!(windows).
Comment on lines
+223
to
+232
| let status = resp.status(); | ||
| let body: serde_json::Value = resp.json().await.map_err(|e| e.to_string())?; | ||
| if !status.is_success() { | ||
| let err = body.get("error").and_then(|v| v.as_str()).unwrap_or(""); | ||
| return Err(if err.is_empty() { | ||
| format!("ollama chat HTTP {status}") | ||
| } else { | ||
| format!("ollama chat HTTP {status}: {err}") | ||
| }); | ||
| } |
There was a problem hiding this comment.
Parsing JSON before status check can mask HTTP errors.
If the daemon or an intermediate proxy returns a non‑JSON body on a 5xx (HTML error page, plain text, empty), resp.json().await fails first and the caller sees a generic serde error instead of ollama chat HTTP 5xx…. Consider reading as bytes/text first, then attempting JSON decode, so the status-based error path at Lines 225–232 can still report the HTTP code.
🛠️ Proposed fix
- let status = resp.status();
- let body: serde_json::Value = resp.json().await.map_err(|e| e.to_string())?;
- if !status.is_success() {
- let err = body.get("error").and_then(|v| v.as_str()).unwrap_or("");
- return Err(if err.is_empty() {
- format!("ollama chat HTTP {status}")
- } else {
- format!("ollama chat HTTP {status}: {err}")
- });
- }
- if let Some(err) = body.get("error").and_then(|v| v.as_str()) {
+ let status = resp.status();
+ let text = resp.text().await.map_err(|e| e.to_string())?;
+ let body: serde_json::Value = serde_json::from_str(&text).unwrap_or(serde_json::Value::Null);
+ if !status.is_success() {
+ let err = body.get("error").and_then(|v| v.as_str()).unwrap_or("");
+ return Err(if err.is_empty() {
+ format!("ollama chat HTTP {status}")
+ } else {
+ format!("ollama chat HTTP {status}: {err}")
+ });
+ }
+ if let Some(err) = body.get("error").and_then(|v| v.as_str()) {
if !err.is_empty() {
return Err(err.to_string());
}
}Note: the existing post_chat helper has the same ordering, so this is consistent with prior art — feel free to defer if you'd rather address both together.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let status = resp.status(); | |
| let body: serde_json::Value = resp.json().await.map_err(|e| e.to_string())?; | |
| if !status.is_success() { | |
| let err = body.get("error").and_then(|v| v.as_str()).unwrap_or(""); | |
| return Err(if err.is_empty() { | |
| format!("ollama chat HTTP {status}") | |
| } else { | |
| format!("ollama chat HTTP {status}: {err}") | |
| }); | |
| } | |
| let status = resp.status(); | |
| let text = resp.text().await.map_err(|e| e.to_string())?; | |
| let body: serde_json::Value = serde_json::from_str(&text).unwrap_or(serde_json::Value::Null); | |
| if !status.is_success() { | |
| let err = body.get("error").and_then(|v| v.as_str()).unwrap_or(""); | |
| return Err(if err.is_empty() { | |
| format!("ollama chat HTTP {status}") | |
| } else { | |
| format!("ollama chat HTTP {status}: {err}") | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src-tauri/src/modules/ollama/service.rs` around lines 223 - 232, The code
calls resp.json().await before checking HTTP status, which can hide non-JSON
error responses; change the flow to first read the response as text (e.g., let
text = resp.text().await.map_err(|e| e.to_string())?), then check status: if
!status.is_success() attempt to parse that text into serde_json::Value to
extract body.get("error").and_then(|v| v.as_str()) and include that or the raw
text in the returned error (format!("ollama chat HTTP {status}:
{err_or_text}")), otherwise parse the text into JSON for the success path; apply
the same pattern where resp.json().await, body, and err are used (and consider
mirroring the change in the post_chat helper).
- Introduced new commands: `doctor`, `plan`, `cost`, `resume`, and `compact` for improved session management and diagnostics. - Added support for non-interactive mode with the `--print` flag to run agent prompts directly. - Implemented session persistence and restoration features, allowing users to resume previous sessions seamlessly. - Enhanced the folder trust prompt for first-run scenarios, improving user experience when launching the REPL in untrusted directories. - Updated command documentation to reflect new features and usage examples.
- Streamlined formatting logic in various functions for enhanced clarity. - Removed unnecessary line breaks and improved structure in session handling and command dispatching. - Consolidated formatting for better maintainability across the CLI codebase.
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (2)
doc/guides/cli.md (2)
180-187: Add language identifiers to fenced code blocks (MD040).markdownlint-cli2 flags multiple fenced blocks missing an explicit language. This can commonly be enforced in CI.
Suggested patch
@@ -``` +180~ +```text ❯ what changed in the fetch tool? ⎿ · called fetch (step 0) ⎿ · fetch: 4012 bytes ⎿ Baked for 4.8s ⎿ The fetch tool now deduplicates URLs per user message … -``` +186~ +``` @@ -``` +204~ +```bash pengine logs --tail 100 -``` +207~ +``` @@ -``` +242~ +```text ⎿ /Users/you/Projects/myapp Add this folder to Pengine's MCP filesystem roots? [y/n] -``` +246~ +``` @@ -``` +297~ +```text ❯ /plan on ⎿ plan mode: ON · agent will produce a markdown plan · write tools (memory writes, fs writes, edits) are stripped from the catalog ❯ migrate the user table to add a `created_at` column ⎿ ## Plan 1. Add `created_at TIMESTAMPTZ DEFAULT now()` to `users` … 2. Backfill existing rows in batches … -``` +306~ +``` @@ -``` +403~ +```bash pengine doctor -``` +406~ +```Also applies to: 204-213, 242-245, 297-306, 403-406
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/guides/cli.md` around lines 180 - 187, Multiple fenced code blocks in the CLI guide lack language identifiers (MD040); update each affected block by adding an appropriate language tag (e.g., ```text or ```bash) to the opening fence. Specifically, locate the fenced blocks that start with "❯ what changed in the fetch tool?", "pengine logs --tail 100", "⎿ /Users/you/Projects/myapp\n Add this folder to Pengine's MCP filesystem roots? [y/n]", the block beginning with "❯ /plan on" and the "pengine doctor" example, and change their opening ``` to include the correct language (`text` for CLI output examples, `bash` for command examples). Ensure all matching closing fences remain unchanged.
188-193: Fix inline-code spacing to satisfy MD038.markdownlint-cli2 reports “spaces inside code span elements” for prompts/reply prefixes like
`❯ `and` ⎿ `. Adjust them so the code spans don’t include leading/trailing spaces (e.g., put the space outside the backticks or describe spacing in prose).Suggested patch
@@ -- **Prompt**: bold-cyan `❯ ` on a TTY, plain `> ` when stdout is piped. -- **Reply prefix**: ` ⎿ ` on the first line, five-space continuation for the rest. Replies are coloured by [`ReplyKind`](../../src-tauri/src/modules/cli/output.rs): diff blocks get green `+` / red `-`, code blocks print raw. -- **Inline tool-event blocks**: while `ask` / a free-text REPL line is running, each `"tool"` log event (call start, `name: N bytes` result, errors, host auto-fetch) is printed as its own persistent ` ⎿ · …` line above the spinner (`handlers::inline_tool_block`). This is Claude-Code-like per-step visibility without touching the agent loop — events come from the existing `AppState.log_tx` broadcast. +- **Prompt**: bold-cyan `❯` (with a trailing space) on a TTY, plain `>` (with a trailing space) when stdout is piped. +- **Reply prefix**: a `⎿` marker with two leading and two trailing spaces on the first line, five-space continuation for the rest. Replies are coloured by [`ReplyKind`](../../src-tauri/src/modules/cli/output.rs): diff blocks get green `+` / red `-`, code blocks print raw. +- **Inline tool-event blocks**: while `ask` / a free-text REPL line is running, each `"tool"` log event (call start, `name: N bytes` result, errors, host auto-fetch) is printed as its own persistent `⎿ · …` line above the spinner (`handlers::inline_tool_block`). This is Claude-Code-like per-step visibility without touching the agent loop — events come from the existing `AppState.log_tx` broadcast. @@ -- **Streaming tool-call result bodies inside a reply**: each tool call now shows as its own ` ⎿ · …` line, but the **full** result body is still collapsed. Claude Code shows expandable tool outputs; Pengine only shows the one-line summary (`name: N bytes`, `name error: …`). Surfacing full bodies would need `agent::run_turn` to forward content, not just `emit_log` notices. +- **Streaming tool-call result bodies inside a reply**: each tool call now shows as its own `⎿ · …` line, but the **full** result body is still collapsed. Claude Code shows expandable tool outputs; Pengine only shows the one-line summary (`name: N bytes`, `name error: …`). Surfacing full bodies would need `agent::run_turn` to forward content, not just `emit_log` notices.Also applies to: 415-415
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/guides/cli.md` around lines 188 - 193, The markdown has code spans with leading/trailing spaces (MD038) such as `❯ `, `` ⎿ ``, and ` ⎿ · …` in the Prompt/Reply/inline-tool/Thinking spinner/Elapsed summary text; remove the spaces from inside the backticks and either place the space outside the code span or describe the spacing in prose (e.g., use `❯` + a following literal space, or write “a space follows the prompt”). Update every instance (including the other occurrence noted for the reply prefix) so no code span contains leading/trailing spaces and rerun markdownlint to confirm MD038 is resolved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doc/guides/cli.md`:
- Around line 346-347: Re-run LanguageTool on the sentence "Headers can be
repeated. `Key: value` and `Key=value` are both accepted." to confirm whether
it's a false positive; inspect and remove any invisible/control characters or
stray whitespace around the backticks, and if nothing is wrong, either slightly
rephrase to "Headers can be repeated; `Key: value` and `Key=value` are both
accepted." or add a comment/disable rule for that line so the linter won't flag
it. Ensure you edit the exact sentence text (the "Headers can be repeated..."
line) and keep the inline code formatting intact.
In `@src-tauri/src/modules/cli/dispatch.rs`:
- Around line 52-60: Replace the hardcoded use of !ctx.telegram_surface when
calling handlers::ask_in_session from dispatch_line (and the similar call at the
other path around dispatch.rs:148) with the explicit continuation intent carried
on DispatchContext (the flag set by the CLI --continue / resume command); i.e.,
pass the DispatchContext field that represents the user's explicit
"continue/resume" intent (rather than !ctx.telegram_surface) as the
persist_session third argument to handlers::ask_in_session so only explicit
continue/resume flows persist the session.
In `@src-tauri/src/modules/cli/handlers.rs`:
- Around line 625-669: The fs handler updates mcp.json but doesn't refresh the
in-memory cache, so after adding/removing roots you must update
state.cached_filesystem_paths to reflect the new paths; inside the
"add"|"remove" branch (after mcp_service::set_filesystem_allowed_paths and
successful mcp_service::save_config) set or replace
state.cached_filesystem_paths with the new paths (or call the existing refresh
helper if one exists) so ask_in_session() will see the change; update this in
the fs function and ensure the update occurs while holding the same
mcp_config_mutex/_guard to avoid races.
- Around line 136-155: The cost estimation is using state.preferred_ollama_model
but should use the models actually used in the session (each SessionTurn.model)
so mid-session /model changes don't flip pricing; change the logic that builds
cost_line to iterate session turns (use the session struct that has
turns/SessionTurn.model), call ollama::classify_model(&turn.model) for each
turn, sum prompt and eval tokens separately for Cloud vs Local turns, then
compute cost as 0 for Local and (1e-6 * prompt_tokens + 3e-6 * eval_tokens) for
Cloud and format that total into cost_line; reference ollama::classify_model,
SessionTurn.model, and the existing prompt_tokens_total/eval_tokens aggregation
to locate where to replace the single preferred_ollama_model check.
In `@src-tauri/src/modules/cli/mcp_cmd.rs`:
- Around line 124-127: The add/remove/import handlers currently only call
mcp_service::save_config(&state.mcp_config_path, &cfg) and return success, but
they never refresh the in-memory registry (state.mcp), so the REPL sees stale
data; after saving, reload or update state.mcp by re-reading the saved
configuration (e.g., call the existing loader function such as
mcp_service::load_config or a registry-read helper) and assign the result into
state.mcp (or patch it in-place) before returning the CliReply::code success
message so the live registry reflects the mutation.
In `@src-tauri/src/modules/cli/mentions.rs`:
- Around line 63-92: The expand_mentions function currently treats an empty
allowed_roots as "no restriction"; change it to fail-closed by treating an empty
allowed_roots as meaning only the cwd is allowed (or require an explicit
unrestricted flag), i.e. before resolving each mention replace an empty
allowed_roots with a single root equal to cwd.to_path_buf() (so
is_under_any_root is always checked against at least cwd), and keep the existing
canonicalize + is_under_any_root checks and error messages intact (symbols:
expand_mentions, allowed_roots, cwd, is_under_any_root, MAX_FILES_PER_MESSAGE).
In `@src-tauri/src/modules/cli/session.rs`:
- Around line 44-48: The current fresh() constructor creates an id formatted
only to whole seconds which can collide when two sessions start in the same
second; update fresh() (the id field generation in
src-tauri/src/modules/cli/session.rs) to produce a collision-resistant id by
appending high-resolution time or randomness (e.g., include fractional
seconds/milliseconds from Utc::now() or append a UUID via Uuid::new_v4()) so
cli_sessions/<id>.json filenames are unique across rapid startups; ensure
started_at remains Utc::now() and only the id generation is changed.
- Around line 87-100: The loop currently iterates &self.turns[take_from..]
oldest→newest so it breaks when bytes exceed HISTORY_BYTES_BUDGET and drops the
newest turns; change to iterate newest→oldest (e.g., for t in
self.turns[take_from..].iter().rev()) and collect pieces in a temporary
Vec<String> then reverse before returning/using so the most recent turns are
kept when the byte budget is exceeded; preserve the existing turn limit logic
using HISTORY_TURN_BUDGET and HISTORY_BYTES_BUDGET and ensure bytes_used and
pieces handling (the variables bytes_used, pieces, and the format code that
references t.user/t.assistant) remain correct when iterating in reverse.
In `@src-tauri/src/modules/mcp/http_transport.rs`:
- Around line 104-121: notify() currently ignores HTTP error statuses because it
doesn't inspect the Response; capture the Response from request.send().await,
check response.status().is_success(), and if not return Err with a formatted
message like "http notify: status {status}: {body_or_status}" similar to how
call_with_timeout() handles non-success statuses (see call_with_timeout, lines
around its status/error handling) so server 4xx/5xx responses are surfaced;
ensure you still map network send errors with map_err("http notify: {e}") before
status checking.
In `@src-tauri/src/modules/mcp/service.rs`:
- Around line 693-715: The project `.mcp.json` overlay is applied after
cached_filesystem_paths is populated, so new project-added Claude-style
filesystem roots never get cached; after merging proj_servers into cfg.servers
(the loop that inserts into cfg.servers and the call to state.emit_log),
recompute and store the filesystem_allowed_paths(&cfg) into
state.cached_filesystem_paths (i.e., call
state.cached_filesystem_paths.write().await = filesystem_allowed_paths(&cfg)
again) so the cache reflects project-local roots loaded by
load_project_mcp_servers.
- Around line 451-461: load_project_mcp_servers currently swallows
read/parse/validation errors and returns None so broken .mcp.json files produce
no diagnostics; change the function to return a Result<Option<(PathBuf,
BTreeMap<String, ServerEntry>)>, anyhow::Error> (or a suitable error type) and
propagate detailed errors from std::fs::read_to_string, serde_json::from_str,
and parse_claude_mcp_servers using context (e.g., map_err or ? with
anyhow::Context) so callers can log or surface the exact failure; update callers
to handle Result (log error with path and underlying error) instead of only
inspecting the success path.
In `@src-tauri/src/modules/mcp/types.rs`:
- Around line 49-60: The Http variant currently persists plaintext headers via
the headers: HashMap<String, String> field (mcp.json), which leaks credentials;
update the MCP config handling so that either secret headers are stored in a
secure keychain-backed path or, until that exists, reject sensitive header names
on load/deserialize: validate the Http.headers map in the Http variant (or in
the serde Deserialize/path that loads mcp.json) and return an error if keys like
"authorization", "api-key", "x-api-key", "cookie", "set-cookie", or other common
secret header names are present; add a clear error message directing users to
the keychain-backed approach (see doc/architecture/mcp.md and
doc/guides/custom-mcp-tools.md) and ensure tests cover rejection behavior for
Http.headers in mcp.json.
In `@src-tauri/src/shared/state.rs`:
- Around line 113-118: AppState currently exposes process-global fields
plan_mode and cli_session (types Arc<RwLock<bool>> and
Arc<RwLock<Option<crate::modules::cli::session::CliSession>>>) which will leak
state across concurrent surfaces; change these to be stored
per-conversation/surface instead of on AppState — e.g. add them to the
conversation/surface context struct (or a ConversationState/SurfaceState map
keyed by conversation ID) and update callers that read/write plan_mode and
cli_session to access the per-conversation entry; ensure construction/cleanup of
those per-surface entries happens where conversations are created/terminated,
and consult doc/platform/data-and-startup.md before touching app startup, disk
paths, or secrets.
In `@src-tauri/tauri.conf.json`:
- Around line 108-116: The "connect" subcommand currently defines a positional
"token" arg which leaks secrets via argv; remove the positional arg entry (the
"token" arg with "index": 1) and instead accept the bot token via a
non-positional, non-logged channel—either a named option (e.g. a "--token" flag
that is read from env/stdin and not written to logs) or by forcing
interactive/stdin input before handing it to secure_store; update the "connect"
command handler to no longer read argv positional token but to read from
process.stdin/env or prompt securely and pass that value to secure_store.
---
Nitpick comments:
In `@doc/guides/cli.md`:
- Around line 180-187: Multiple fenced code blocks in the CLI guide lack
language identifiers (MD040); update each affected block by adding an
appropriate language tag (e.g., ```text or ```bash) to the opening fence.
Specifically, locate the fenced blocks that start with "❯ what changed in the
fetch tool?", "pengine logs --tail 100", "⎿ /Users/you/Projects/myapp\n Add
this folder to Pengine's MCP filesystem roots? [y/n]", the block beginning with
"❯ /plan on" and the "pengine doctor" example, and change their opening ``` to
include the correct language (`text` for CLI output examples, `bash` for command
examples). Ensure all matching closing fences remain unchanged.
- Around line 188-193: The markdown has code spans with leading/trailing spaces
(MD038) such as `❯ `, `` ⎿ ``, and ` ⎿ · …` in the
Prompt/Reply/inline-tool/Thinking spinner/Elapsed summary text; remove the
spaces from inside the backticks and either place the space outside the code
span or describe the spacing in prose (e.g., use `❯` + a following literal
space, or write “a space follows the prompt”). Update every instance (including
the other occurrence noted for the reply prefix) so no code span contains
leading/trailing spaces and rerun markdownlint to confirm MD038 is resolved.
🪄 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: 0e1e7455-e6af-45d3-a533-d078a601ee05
📒 Files selected for processing (22)
doc/guides/cli.mdsrc-tauri/src/modules/agent/mod.rssrc-tauri/src/modules/bot/service.rssrc-tauri/src/modules/cli/bootstrap.rssrc-tauri/src/modules/cli/commands.rssrc-tauri/src/modules/cli/dispatch.rssrc-tauri/src/modules/cli/doctor.rssrc-tauri/src/modules/cli/folder_trust.rssrc-tauri/src/modules/cli/handlers.rssrc-tauri/src/modules/cli/mcp_cmd.rssrc-tauri/src/modules/cli/mentions.rssrc-tauri/src/modules/cli/mod.rssrc-tauri/src/modules/cli/repl.rssrc-tauri/src/modules/cli/session.rssrc-tauri/src/modules/mcp/client.rssrc-tauri/src/modules/mcp/http_transport.rssrc-tauri/src/modules/mcp/mod.rssrc-tauri/src/modules/mcp/service.rssrc-tauri/src/modules/mcp/types.rssrc-tauri/src/shared/state.rssrc-tauri/tauri.conf.jsonvite/pengine-logger.ts
✅ Files skipped from review due to trivial changes (3)
- src-tauri/src/modules/mcp/mod.rs
- vite/pengine-logger.ts
- src-tauri/src/modules/cli/bootstrap.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- src-tauri/src/modules/cli/mod.rs
- src-tauri/src/modules/bot/service.rs
- src-tauri/src/modules/cli/repl.rs
- src-tauri/src/modules/agent/mod.rs
Comment on lines
+346
to
+347
| Headers can be repeated. `Key: value` and `Key=value` are both accepted. | ||
| Pengine accepts `application/json` and `text/event-stream` responses. |
There was a problem hiding this comment.
Re-check the LanguageTool “spelling” warning (Line 346).
LanguageTool flags Line 346 around the “Headers can be repeated…” sentence. The text shown there looks correct (“Key: value and Key=value”), so this is likely a false positive—but please confirm there’s no subtle typo or formatting artifact in that exact line.
🧰 Tools
🪛 LanguageTool
[grammar] ~346-~346: Ensure spelling is correct
Context: ...lueandKey=valueare both accepted. Pengine acceptsapplication/jsonandtext/ev...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@doc/guides/cli.md` around lines 346 - 347, Re-run LanguageTool on the
sentence "Headers can be repeated. `Key: value` and `Key=value` are both
accepted." to confirm whether it's a false positive; inspect and remove any
invisible/control characters or stray whitespace around the backticks, and if
nothing is wrong, either slightly rephrase to "Headers can be repeated; `Key:
value` and `Key=value` are both accepted." or add a comment/disable rule for
that line so the linter won't flag it. Ensure you edit the exact sentence text
(the "Headers can be repeated..." line) and keep the inline code formatting
intact.
Comment on lines
+52
to
+60
| pub async fn dispatch_line(state: &AppState, line: &str, ctx: DispatchContext) -> CliReply { | ||
| match router::classify_line(line) { | ||
| RouterOutcome::Unknown(name) => { | ||
| CliReply::error(format!("unknown command: /{name} (try /help)",)) | ||
| } | ||
| RouterOutcome::Agent(text) => { | ||
| handlers::ask_in_session(state, text, !ctx.telegram_surface).await | ||
| } | ||
| RouterOutcome::Native { name, rest } => dispatch_native(state, name, rest, ctx).await, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C4 '\bask_in_session\s*\(' src-tauri/src/modules/cli
fd -p 'handlers.rs' src-tauri | while read -r f; do
echo "== $f =="
rg -n -C8 'async fn ask_in_session|fn ask_in_session' "$f"
doneRepository: pengine-ai/pengine
Length of output: 4998
Pass the explicit resume/continue intent to ask_in_session instead of hardcoding based on transport.
The third parameter to ask_in_session is persist_session (see handlers.rs:769), which controls whether the persistent session is extended. Per the function's documentation, this should reflect the caller's explicit intent to continue. However, dispatch.rs:58 and dispatch.rs:148 hardcode !ctx.telegram_surface (always true for CLI), which auto-persists the session for every non-Telegram path. This contradicts bootstrap.rs:113, which only persists when the user explicitly passes --continue. As a result, /ask commands pull stale history without explicit --continue or /resume.
Only paths that represent explicit continuation intent (like --continue flag or /resume command) should pass true.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src-tauri/src/modules/cli/dispatch.rs` around lines 52 - 60, Replace the
hardcoded use of !ctx.telegram_surface when calling handlers::ask_in_session
from dispatch_line (and the similar call at the other path around
dispatch.rs:148) with the explicit continuation intent carried on
DispatchContext (the flag set by the CLI --continue / resume command); i.e.,
pass the DispatchContext field that represents the user's explicit
"continue/resume" intent (rather than !ctx.telegram_surface) as the
persist_session third argument to handlers::ask_in_session so only explicit
continue/resume flows persist the session.
Comment on lines
+625
to
+669
| pub async fn fs(state: &AppState, action: Option<&str>, path: Option<&str>) -> CliReply { | ||
| let action = action.map(str::trim).unwrap_or("list"); | ||
| let _guard = state.mcp_config_mutex.lock().await; | ||
| match action { | ||
| "list" | "" => { | ||
| let cfg = match mcp_service::load_or_init_config(&state.mcp_config_path) { | ||
| Ok(c) => c, | ||
| Err(e) => return CliReply::error(format!("fs: {e}")), | ||
| }; | ||
| let paths = mcp_service::filesystem_allowed_paths(&cfg); | ||
| if paths.is_empty() { | ||
| CliReply::code("bash", "(no roots)") | ||
| } else { | ||
| CliReply::code("bash", paths.join("\n")) | ||
| } | ||
| } | ||
| "add" | "remove" => { | ||
| let Some(path) = path.map(str::trim).filter(|p| !p.is_empty()) else { | ||
| return CliReply::error(format!("fs {action}: path required")); | ||
| }; | ||
| let mut cfg = match mcp_service::load_or_init_config(&state.mcp_config_path) { | ||
| Ok(c) => c, | ||
| Err(e) => return CliReply::error(format!("fs: {e}")), | ||
| }; | ||
| let mut paths = mcp_service::filesystem_allowed_paths(&cfg); | ||
| let before = paths.len(); | ||
| if action == "add" { | ||
| if !paths.iter().any(|p| p == path) { | ||
| paths.push(path.to_string()); | ||
| } | ||
| } else { | ||
| paths.retain(|p| p != path); | ||
| } | ||
| if paths.len() == before { | ||
| return CliReply::code( | ||
| "bash", | ||
| format!("no change ({action} `{path}` had no effect)"), | ||
| ); | ||
| } | ||
| mcp_service::set_filesystem_allowed_paths(&mut cfg, &paths); | ||
| if let Err(e) = mcp_service::save_config(&state.mcp_config_path, &cfg) { | ||
| return CliReply::error(format!("save: {e}")); | ||
| } | ||
| CliReply::code("bash", format!("{action}: {path}")) | ||
| } |
There was a problem hiding this comment.
fs add/remove leaves the current process on stale filesystem roots.
This updates mcp.json only. ask_in_session() later reads state.cached_filesystem_paths for mention expansion, so newly added roots are not usable until some separate rebuild refreshes that cache.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src-tauri/src/modules/cli/handlers.rs` around lines 625 - 669, The fs handler
updates mcp.json but doesn't refresh the in-memory cache, so after
adding/removing roots you must update state.cached_filesystem_paths to reflect
the new paths; inside the "add"|"remove" branch (after
mcp_service::set_filesystem_allowed_paths and successful
mcp_service::save_config) set or replace state.cached_filesystem_paths with the
new paths (or call the existing refresh helper if one exists) so
ask_in_session() will see the change; update this in the fs function and ensure
the update occurs while holding the same mcp_config_mutex/_guard to avoid races.
Comment on lines
+451
to
+461
| pub fn load_project_mcp_servers( | ||
| cwd: &Path, | ||
| ) -> Option<(PathBuf, std::collections::BTreeMap<String, ServerEntry>)> { | ||
| let path = cwd.join(".mcp.json"); | ||
| if !path.is_file() { | ||
| return None; | ||
| } | ||
| let raw = std::fs::read_to_string(&path).ok()?; | ||
| let value: serde_json::Value = serde_json::from_str(&raw).ok()?; | ||
| let servers = parse_claude_mcp_servers(&value).ok()?; | ||
| Some((path, servers)) |
There was a problem hiding this comment.
Don't silently ignore malformed project .mcp.json.
load_project_mcp_servers() turns read, parse, and validation failures into None, and the caller only logs the success path. A broken project config is therefore skipped with no diagnostics at all.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src-tauri/src/modules/mcp/service.rs` around lines 451 - 461,
load_project_mcp_servers currently swallows read/parse/validation errors and
returns None so broken .mcp.json files produce no diagnostics; change the
function to return a Result<Option<(PathBuf, BTreeMap<String, ServerEntry>)>,
anyhow::Error> (or a suitable error type) and propagate detailed errors from
std::fs::read_to_string, serde_json::from_str, and parse_claude_mcp_servers
using context (e.g., map_err or ? with anyhow::Context) so callers can log or
surface the exact failure; update callers to handle Result (log error with path
and underlying error) instead of only inspecting the success path.
Comment on lines
693
to
+715
| *state.cached_filesystem_paths.write().await = filesystem_allowed_paths(&cfg); | ||
|
|
||
| // Project-local `.mcp.json` (Claude Code shape) overlays the global config | ||
| // for this rebuild only — it is never persisted. Project keys win on | ||
| // collision so a workspace can override a globally-configured server. | ||
| if let Ok(cwd) = std::env::current_dir() { | ||
| if let Some((proj_path, proj_servers)) = load_project_mcp_servers(&cwd) { | ||
| let n = proj_servers.len(); | ||
| for (k, v) in proj_servers { | ||
| cfg.servers.insert(k, v); | ||
| } | ||
| state | ||
| .emit_log( | ||
| "mcp", | ||
| &format!( | ||
| "loaded project `.mcp.json` ({} server(s)) from {}", | ||
| n, | ||
| proj_path.display() | ||
| ), | ||
| ) | ||
| .await; | ||
| } | ||
| } |
There was a problem hiding this comment.
Project .mcp.json roots never make it into cached_filesystem_paths.
The cache is populated from the global config before the project overlay runs. If the project file adds a Claude-style filesystem server, MCP may connect it, but CLI mention expansion still sees only the old global roots.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src-tauri/src/modules/mcp/service.rs` around lines 693 - 715, The project
`.mcp.json` overlay is applied after cached_filesystem_paths is populated, so
new project-added Claude-style filesystem roots never get cached; after merging
proj_servers into cfg.servers (the loop that inserts into cfg.servers and the
call to state.emit_log), recompute and store the filesystem_allowed_paths(&cfg)
into state.cached_filesystem_paths (i.e., call
state.cached_filesystem_paths.write().await = filesystem_allowed_paths(&cfg)
again) so the cache reflects project-local roots loaded by
load_project_mcp_servers.
Comment on lines
+49
to
+60
| /// Remote MCP server reached over HTTP. JSON-RPC requests are POSTed to | ||
| /// `url`; responses can be either `application/json` or an SSE stream | ||
| /// (`text/event-stream`). Headers (e.g. `Authorization: Bearer …`) are | ||
| /// stored in plain text — for secrets, use a wrapper script today and a | ||
| /// keychain-backed mechanism later. Compatible with Claude Code's | ||
| /// `{ "type": "http", "url": …, "headers": {…} }` shape. | ||
| Http { | ||
| url: String, | ||
| #[serde(default)] | ||
| headers: HashMap<String, String>, | ||
| #[serde(default)] | ||
| direct_return: bool, |
There was a problem hiding this comment.
Plaintext HTTP headers in mcp.json are a credential leak.
This shape persists arbitrary headers directly to disk, so the common Authorization/API-key case now bypasses the repo’s existing keychain pattern. Please move secret headers behind secure storage, or at minimum reject sensitive header names until a keychain-backed path exists.
Based on learnings, reference doc/architecture/mcp.md and doc/guides/custom-mcp-tools.md when working with MCP client, registry, native tools, and mcp.json.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src-tauri/src/modules/mcp/types.rs` around lines 49 - 60, The Http variant
currently persists plaintext headers via the headers: HashMap<String, String>
field (mcp.json), which leaks credentials; update the MCP config handling so
that either secret headers are stored in a secure keychain-backed path or, until
that exists, reject sensitive header names on load/deserialize: validate the
Http.headers map in the Http variant (or in the serde Deserialize/path that
loads mcp.json) and return an error if keys like "authorization", "api-key",
"x-api-key", "cookie", "set-cookie", or other common secret header names are
present; add a clear error message directing users to the keychain-backed
approach (see doc/architecture/mcp.md and doc/guides/custom-mcp-tools.md) and
ensure tests cover rejection behavior for Http.headers in mcp.json.
Comment on lines
+113
to
+118
| /// Plan mode: when true the agent receives a planning system prompt and | ||
| /// write-style tools are stripped from the catalog. Toggled via `/plan`. | ||
| pub plan_mode: Arc<RwLock<bool>>, | ||
| /// Active CLI/REPL session (turn history, summary, token totals). | ||
| /// `None` outside the REPL or before the first ask. | ||
| pub cli_session: Arc<RwLock<Option<crate::modules::cli::session::CliSession>>>, |
There was a problem hiding this comment.
These fields shouldn’t be process-global on AppState.
AppState is shared across the whole process, so a single plan_mode and cli_session will bleed between concurrent surfaces. A Telegram $plan/$ask can flip another REPL into plan mode or mix turn history/cost totals into the wrong session. Keep these scoped per conversation/surface instead of on global app state.
Based on learnings, consult doc/platform/data-and-startup.md before modifying startup logic, AppState, disk paths, or secrets in src-tauri/src/app.rs, shared/state.rs, or modules/secure_store/.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src-tauri/src/shared/state.rs` around lines 113 - 118, AppState currently
exposes process-global fields plan_mode and cli_session (types Arc<RwLock<bool>>
and Arc<RwLock<Option<crate::modules::cli::session::CliSession>>>) which will
leak state across concurrent surfaces; change these to be stored
per-conversation/surface instead of on AppState — e.g. add them to the
conversation/surface context struct (or a ConversationState/SurfaceState map
keyed by conversation ID) and update callers that read/write plan_mode and
cli_session to access the per-conversation entry; ensure construction/cleanup of
those per-surface entries happens where conversations are created/terminated,
and consult doc/platform/data-and-startup.md before touching app startup, disk
paths, or secrets.
Comment on lines
+108
to
+116
| "connect": { | ||
| "description": "Connect with a bot token", | ||
| "args": [ | ||
| { | ||
| "name": "token", | ||
| "description": "Telegram bot token", | ||
| "takesValue": true, | ||
| "index": 1 | ||
| } |
There was a problem hiding this comment.
Don’t take the Telegram token as a positional CLI argument.
pengine bot connect <token> leaks the bot token through shell history and process inspection before secure_store ever sees it. This flow should read the token interactively or from stdin/env instead of argv.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src-tauri/tauri.conf.json` around lines 108 - 116, The "connect" subcommand
currently defines a positional "token" arg which leaks secrets via argv; remove
the positional arg entry (the "token" arg with "index": 1) and instead accept
the bot token via a non-positional, non-logged channel—either a named option
(e.g. a "--token" flag that is read from env/stdin and not written to logs) or
by forcing interactive/stdin input before handing it to secure_store; update the
"connect" command handler to no longer read argv positional token but to read
from process.stdin/env or prompt securely and pass that value to secure_store.
- Introduced a new task spawning mechanism to allow recursive task execution with depth control. - Added support for managing scheduled tasks, including creation, deletion, and enabling/disabling jobs. - Enhanced MCP tool management by implementing a recovery mechanism for tool calls during transport failures. - Updated the CLI to support new commands for managing tasks and improved error handling in command execution. - Refactored existing code for better maintainability and clarity, including updates to JSON schemas for tools.
- Streamlined function signatures and formatting for better clarity. - Consolidated error handling checks in the mcp_stdio_recoverable function. - Enhanced the structure of the run_task_spawn_inline function for improved maintainability.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (5)
src-tauri/src/shared/state.rs (1)
113-118:⚠️ Potential issue | 🟠 MajorThese session flags still shouldn't live on process-global
AppState.
plan_modeandcli_sessionare shared by every surface. A Telegram/CLI action can flip plan mode or overwrite session history for another concurrent conversation. Keep them keyed by conversation/surface instead of global state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/shared/state.rs` around lines 113 - 118, The Plan mode flag and CLI session are stored as process-global fields on AppState (plan_mode and cli_session) which allows one surface (e.g., Telegram or CLI) to affect another; refactor these into per-conversation/per-surface storage instead of globals: remove plan_mode: Arc<RwLock<bool>> and cli_session: Arc<RwLock<Option<crate::modules::cli::session::CliSession>>> from AppState and introduce a keyed map (e.g., HashMap<ConversationId, ConversationState> or SurfaceId -> {plan_mode, cli_session}) that stores plan_mode and cli_session per conversation/surface, update codepaths that access AppState::plan_mode and AppState::cli_session to look up the appropriate conversation key, and ensure concurrent access uses the same Arc/RwLock semantics on the per-key entries to preserve thread-safety.src-tauri/src/modules/mcp/service.rs (2)
697-718:⚠️ Potential issue | 🟠 MajorProject-local filesystem roots still never reach
cached_filesystem_paths.The cache is written from the global config before the project
.mcp.jsonoverlay is applied, so project-local filesystem servers are invisible to code that relies onstate.cached_filesystem_paths—for example the gitrepo_pathinference added insrc-tauri/src/modules/agent/mod.rs. Refresh the cache from the effective merged config after applying the project overlay.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/modules/mcp/service.rs` around lines 697 - 718, The cached filesystem roots are written from the global cfg before applying project `.mcp.json`, so project-local servers never reach state.cached_filesystem_paths; after merging project servers into cfg (the loop that inserts into cfg.servers after load_project_mcp_servers returns), recompute and write the effective paths by calling filesystem_allowed_paths(&cfg) and assigning it to state.cached_filesystem_paths.write().await so the cache reflects the merged configuration.
447-461:⚠️ Potential issue | 🟠 MajorDon't silently drop malformed project
.mcp.jsonfiles.A read/parse/validation failure still becomes
Nonehere, so a broken workspace config is treated as “no project config” with no diagnostics. Please propagate the error and let the rebuild path log the file path plus the underlying failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/modules/mcp/service.rs` around lines 447 - 461, The function load_project_mcp_servers currently swallows I/O/parse/validation errors and returns None; change its signature to return a Result<(PathBuf, BTreeMap<String, ServerEntry>), anyhow::Error> (or a suitable error type) and propagate failures from std::fs::read_to_string, serde_json::from_str and parse_claude_mcp_servers instead of .ok()?:, so callers (e.g., the rebuild path) can log the file path and the underlying error; ensure errors include context (e.g., using anyhow::Context or map_err) mentioning the .mcp.json path and whether the failure was read/parse/validation.src-tauri/src/modules/mcp/http_transport.rs (1)
105-122:⚠️ Potential issue | 🟠 MajorCheck HTTP status in
notify().
send().awaitonly tells you the request reached a response; it does not make 4xx/5xx fail here. Right now an HTTP MCP server can rejectnotifications/initializedand the transport still reports success. Please mirror the non-2xx handling already used incall_with_timeout().In reqwest, does `RequestBuilder::send().await` return `Ok(Response)` for HTTP 4xx/5xx unless `error_for_status()` is called or `response.status()` is checked explicitly?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/modules/mcp/http_transport.rs` around lines 105 - 122, The notify() method currently treats any HTTP response as success because it only awaits request.send().await; modify notify() to inspect the Response and fail on non-2xx like call_with_timeout() does — after request.send().await obtain the Response and call response.error_for_status() (or check response.status().is_success()) and map non-success into an Err with a descriptive message, preserving the existing error formatting (e.g., "http notify: ..."); ensure you still return Ok(()) only when the status is 2xx.src-tauri/src/modules/cli/mcp_cmd.rs (1)
121-124:⚠️ Potential issue | 🟠 MajorThese mutations still leave the live registry stale.
Each path returns after saving
mcp.json, but nothing rebuilds or patchesstate.mcp. In the current REPL session,/toolsand free-text routing will keep using the old registry until some unrelated rebuild happens, so the success text is misleading. Please refresh the in-memory registry after a successful save.Also applies to: 169-181, 197-203, 222-225, 278-283
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/modules/cli/mcp_cmd.rs` around lines 121 - 124, After saving the updated mcp.json with mcp_service::save_config(&state.mcp_config_path, &cfg), you must refresh the in-memory registry (state.mcp) so the REPL uses the new registry; change each early return path that currently returns after save to instead call the service that loads or rebuilds the registry (e.g. mcp_service::load_config or a builder like mcp_service::build_registry_from_file) using state.mcp_config_path and assign the result to state.mcp (handle errors similarly), then return the CliReply; apply this fix to the blocks around the references to save_config, cfg, and state.mcp so the live registry is updated immediately after saving.
🧹 Nitpick comments (2)
tools/notebook/Dockerfile (1)
1-4: The PyPI package arg and runtime executable can drift apart.The build is parameterized by package name, but Line 13 always launches
cursor-notebook-mcp. If the catalog ever swapsUPSTREAM_MCP_PYPI_PACKAGE, this image can install cleanly and still fail on startup.Also applies to: 13-13
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/notebook/Dockerfile` around lines 1 - 4, The Dockerfile hardcodes the runtime executable "cursor-notebook-mcp" while installation uses ARG UPSTREAM_MCP_PYPI_PACKAGE/UPSTREAM_MCP_PYPI_VERSION, which can drift; update the Dockerfile to derive the runtime command from the build ARG (e.g., export ARG UPSTREAM_MCP_PYPI_PACKAGE as an ENV or substitute it into the CMD/ENTRYPOINT) so the startup executable matches the installed package (reference UPSTREAM_MCP_PYPI_PACKAGE, UPSTREAM_MCP_PYPI_VERSION and the hardcoded "cursor-notebook-mcp" string).tools/shell/Dockerfile (1)
1-4: The image is only partially parameterized.
UPSTREAM_MCP_NPM_PACKAGEcan change at build time, but Line 24 still hardcodes the default package path. The next catalog package swap will build and then fail at runtime unless this entrypoint is updated in lockstep.📦 Make the executable configurable too
ARG UPSTREAM_MCP_NPM_PACKAGE=@mako10k/mcp-shell-server ARG UPSTREAM_MCP_NPM_VERSION=2.7.1 +ARG UPSTREAM_MCP_NPM_BIN=mcp-shell-server @@ RUN apk add --no-cache --virtual .build-deps python3 make g++ \ && npm install --omit=dev --prefix /mcp "${UPSTREAM_MCP_NPM_PACKAGE}@${UPSTREAM_MCP_NPM_VERSION}" \ && npm cache clean --force \ && rm -rf /root/.npm \ + && printf '#!/bin/sh\nexec /mcp/node_modules/.bin/%s "$@"\n' "${UPSTREAM_MCP_NPM_BIN}" > /usr/local/bin/mcp-entrypoint \ + && chmod +x /usr/local/bin/mcp-entrypoint \ && chown -R mcp:mcp /mcp \ && apk del .build-deps @@ -ENTRYPOINT ["node", "/mcp/node_modules/@mako10k/mcp-shell-server/dist/index.js"] +ENTRYPOINT ["/usr/local/bin/mcp-entrypoint"]Also applies to: 24-24
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/shell/Dockerfile` around lines 1 - 4, The Dockerfile currently parameterizes UPSTREAM_MCP_NPM_PACKAGE and UPSTREAM_MCP_NPM_VERSION but still hardcodes the executable path in the image entrypoint/CMD; update the ENTRYPOINT/CMD to use the package argument so the executable is configurable. Replace the hardcoded package path with the ARG/ENV (UPSTREAM_MCP_NPM_PACKAGE or an exported env like MCP_PACKAGE) and reference it when building the command (e.g., construct the node require/import using the package variable) ensuring proper quoting/escaping so runtime requires use the chosen package; keep UPSTREAM_MCP_NPM_VERSION as-is for install/versioning and ensure the final ENTRYPOINT or CMD references the variable instead of a literal package name.
🤖 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-tauri/src/modules/agent/mod.rs`:
- Around line 970-972: The plan-mode read-only filter is only applied once but
must be reapplied whenever the tool context is rebuilt after routing/escalation;
update the code so that after any place that recreates or replaces tool_ctx
(e.g., where tool_ctx is reassigned or full_tool_context is used during routing
escalation) you call plan_mode_filter_writes(&mut tool_ctx.tools_json) when
plan_mode is true. In practice, ensure every code path that constructs or swaps
in a new ToolContext (the variable/tool_ctx) runs the same conditional: if
plan_mode { plan_mode_filter_writes(&mut tool_ctx.tools_json); } so the filtered
catalog is enforced after escalation.
In `@src-tauri/src/modules/mcp/native.rs`:
- Around line 481-503: The parse_schedule_from_args function currently casts
daily_at_hour and daily_at_minute to u8 directly (via as u8), which silently
wraps out-of-range values; update parse_schedule_from_args to validate that hour
and minute are within valid ranges (0..=23 for hour and 0..=59 for minute)
before narrowing, converting them with try_from or explicit bounds checks and
returning a descriptive Err (e.g., "daily_at_hour out of range" /
"daily_at_minute out of range") if validation fails so Schedule::DailyAt only
receives valid u8 values.
In `@src-tauri/src/modules/mcp/transport.rs`:
- Around line 19-21: The rpc_mutex should only protect the NDJSON write/flush,
not the entire request/response lifetime: change the code paths that currently
lock rpc_mutex (the transport methods that perform outgoing JSON-RPC
calls/notifications—e.g., the code around rpc_mutex usage in the
request/send/call logic referenced between lines ~99-165) so that you acquire
the Mutex<()>, perform the ndjson write and flush, then immediately drop/release
the lock before awaiting the response; ensure you still register the pending
response handler (e.g., insert the response oneshot or pending entry keyed by id
into whatever pending map you have) before releasing the lock so responses can
be matched, and do not hold rpc_mutex while awaiting the oneshot or handling the
incoming response.
In `@src-tauri/src/shared/state.rs`:
- Around line 119-122: The task_spawn_depth Arc<AtomicU8> in shared state is
global and causes unrelated conversations to interfere; move the depth counter
to be per-conversation (e.g., add a task_spawn_depth AtomicU8 field to the
conversation-specific struct or maintain a HashMap<conversation_id, AtomicU8>
guarded by the existing state) instead of the global pub task_spawn_depth:
Arc<AtomicU8> in state.rs, ensure creations/clones of child agents read and
increment the conversation-local counter, and enforce the existing cap via
TASK_SPAWN_MAX_DEPTH from crate::modules::mcp::native when
incrementing/decrementing so each conversation has its own recursion guard.
In `@tools/lsp/Dockerfile`:
- Around line 11-13: The Dockerfile currently installs tools without pinned
versions (ARG GOPLS_VERSION=latest and unversioned typescript/rust-analyzer) and
downloads rust-analyzer without checksum verification; update the Dockerfile to
accept explicit version ARGs (e.g., GOPLS_VERSION, RUST_ANALYZER_VERSION,
TYPESCRIPT_VERSION) and replace occurrences such as the go install
golang.org/x/tools/gopls@${GOPLS_VERSION} and the rust-analyzer download URL
(currently using releases/latest) to use the pinned version variables, and add a
checksum check for the rust-analyzer artifact (download .sha256 or compute and
compare sha256sum) before installing to ensure reproducible, verifiable builds.
---
Duplicate comments:
In `@src-tauri/src/modules/cli/mcp_cmd.rs`:
- Around line 121-124: After saving the updated mcp.json with
mcp_service::save_config(&state.mcp_config_path, &cfg), you must refresh the
in-memory registry (state.mcp) so the REPL uses the new registry; change each
early return path that currently returns after save to instead call the service
that loads or rebuilds the registry (e.g. mcp_service::load_config or a builder
like mcp_service::build_registry_from_file) using state.mcp_config_path and
assign the result to state.mcp (handle errors similarly), then return the
CliReply; apply this fix to the blocks around the references to save_config,
cfg, and state.mcp so the live registry is updated immediately after saving.
In `@src-tauri/src/modules/mcp/http_transport.rs`:
- Around line 105-122: The notify() method currently treats any HTTP response as
success because it only awaits request.send().await; modify notify() to inspect
the Response and fail on non-2xx like call_with_timeout() does — after
request.send().await obtain the Response and call response.error_for_status()
(or check response.status().is_success()) and map non-success into an Err with a
descriptive message, preserving the existing error formatting (e.g., "http
notify: ..."); ensure you still return Ok(()) only when the status is 2xx.
In `@src-tauri/src/modules/mcp/service.rs`:
- Around line 697-718: The cached filesystem roots are written from the global
cfg before applying project `.mcp.json`, so project-local servers never reach
state.cached_filesystem_paths; after merging project servers into cfg (the loop
that inserts into cfg.servers after load_project_mcp_servers returns), recompute
and write the effective paths by calling filesystem_allowed_paths(&cfg) and
assigning it to state.cached_filesystem_paths.write().await so the cache
reflects the merged configuration.
- Around line 447-461: The function load_project_mcp_servers currently swallows
I/O/parse/validation errors and returns None; change its signature to return a
Result<(PathBuf, BTreeMap<String, ServerEntry>), anyhow::Error> (or a suitable
error type) and propagate failures from std::fs::read_to_string,
serde_json::from_str and parse_claude_mcp_servers instead of .ok()?:, so callers
(e.g., the rebuild path) can log the file path and the underlying error; ensure
errors include context (e.g., using anyhow::Context or map_err) mentioning the
.mcp.json path and whether the failure was read/parse/validation.
In `@src-tauri/src/shared/state.rs`:
- Around line 113-118: The Plan mode flag and CLI session are stored as
process-global fields on AppState (plan_mode and cli_session) which allows one
surface (e.g., Telegram or CLI) to affect another; refactor these into
per-conversation/per-surface storage instead of globals: remove plan_mode:
Arc<RwLock<bool>> and cli_session:
Arc<RwLock<Option<crate::modules::cli::session::CliSession>>> from AppState and
introduce a keyed map (e.g., HashMap<ConversationId, ConversationState> or
SurfaceId -> {plan_mode, cli_session}) that stores plan_mode and cli_session per
conversation/surface, update codepaths that access AppState::plan_mode and
AppState::cli_session to look up the appropriate conversation key, and ensure
concurrent access uses the same Arc/RwLock semantics on the per-key entries to
preserve thread-safety.
---
Nitpick comments:
In `@tools/notebook/Dockerfile`:
- Around line 1-4: The Dockerfile hardcodes the runtime executable
"cursor-notebook-mcp" while installation uses ARG
UPSTREAM_MCP_PYPI_PACKAGE/UPSTREAM_MCP_PYPI_VERSION, which can drift; update the
Dockerfile to derive the runtime command from the build ARG (e.g., export ARG
UPSTREAM_MCP_PYPI_PACKAGE as an ENV or substitute it into the CMD/ENTRYPOINT) so
the startup executable matches the installed package (reference
UPSTREAM_MCP_PYPI_PACKAGE, UPSTREAM_MCP_PYPI_VERSION and the hardcoded
"cursor-notebook-mcp" string).
In `@tools/shell/Dockerfile`:
- Around line 1-4: The Dockerfile currently parameterizes
UPSTREAM_MCP_NPM_PACKAGE and UPSTREAM_MCP_NPM_VERSION but still hardcodes the
executable path in the image entrypoint/CMD; update the ENTRYPOINT/CMD to use
the package argument so the executable is configurable. Replace the hardcoded
package path with the ARG/ENV (UPSTREAM_MCP_NPM_PACKAGE or an exported env like
MCP_PACKAGE) and reference it when building the command (e.g., construct the
node require/import using the package variable) ensuring proper quoting/escaping
so runtime requires use the chosen package; keep UPSTREAM_MCP_NPM_VERSION as-is
for install/versioning and ensure the final ENTRYPOINT or CMD references the
variable instead of a literal package name.
🪄 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: f74cd5cc-415a-44b1-a1b4-11068c255e45
⛔ Files ignored due to path filters (1)
.claude/scheduled_tasks.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
src-tauri/src/modules/agent/mod.rssrc-tauri/src/modules/cli/folder_trust.rssrc-tauri/src/modules/cli/mcp_cmd.rssrc-tauri/src/modules/cli/repl.rssrc-tauri/src/modules/mcp/client.rssrc-tauri/src/modules/mcp/http_transport.rssrc-tauri/src/modules/mcp/native.rssrc-tauri/src/modules/mcp/service.rssrc-tauri/src/modules/mcp/tool_metadata.rssrc-tauri/src/modules/mcp/transport.rssrc-tauri/src/shared/state.rstools/lsp/Dockerfiletools/lsp/cclsp.jsontools/mcp-tools.jsontools/notebook/Dockerfiletools/ripgrep/Dockerfiletools/shell/Dockerfile
✅ Files skipped from review due to trivial changes (2)
- tools/lsp/cclsp.json
- tools/ripgrep/Dockerfile
Comment on lines
+481
to
+503
| fn parse_schedule_from_args(args: &Value) -> Result<crate::modules::cron::types::Schedule, String> { | ||
| use crate::modules::cron::types::Schedule; | ||
| let every = args.get("every_minutes").and_then(|v| v.as_u64()); | ||
| let hour = args.get("daily_at_hour").and_then(|v| v.as_u64()); | ||
| let minute = args.get("daily_at_minute").and_then(|v| v.as_u64()); | ||
|
|
||
| match (every, hour, minute) { | ||
| (Some(_), Some(_), _) | (Some(_), _, Some(_)) => { | ||
| Err("provide either every_minutes OR daily_at_*; not both".into()) | ||
| } | ||
| (Some(m), None, None) => { | ||
| let m = u32::try_from(m).map_err(|_| "every_minutes out of range".to_string())?; | ||
| Ok(Schedule::EveryMinutes { minutes: m }) | ||
| } | ||
| (None, Some(h), Some(m)) => Ok(Schedule::DailyAt { | ||
| hour: h as u8, | ||
| minute: m as u8, | ||
| }), | ||
| (None, Some(_), None) | (None, None, Some(_)) => { | ||
| Err("daily_at requires both daily_at_hour and daily_at_minute".into()) | ||
| } | ||
| (None, None, None) => Err("missing schedule: provide every_minutes or daily_at_*".into()), | ||
| } |
There was a problem hiding this comment.
Validate daily_at_* before narrowing to u8.
prepare_tool_invocation() / call_tool() pass raw args straight through, so this path cannot rely on JSON Schema bounds. Values like daily_at_minute = 300 wrap to 44 on as u8 and may schedule the job at the wrong time instead of rejecting it.
🕒 Reject out-of-range daily schedules explicitly
- (None, Some(h), Some(m)) => Ok(Schedule::DailyAt {
- hour: h as u8,
- minute: m as u8,
- }),
+ (None, Some(h), Some(m)) => {
+ let hour = u8::try_from(h).map_err(|_| "daily_at_hour out of range".to_string())?;
+ let minute =
+ u8::try_from(m).map_err(|_| "daily_at_minute out of range".to_string())?;
+ if hour > 23 {
+ return Err("daily_at_hour must be in 0..=23".into());
+ }
+ if minute > 59 {
+ return Err("daily_at_minute must be in 0..=59".into());
+ }
+ Ok(Schedule::DailyAt { hour, minute })
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fn parse_schedule_from_args(args: &Value) -> Result<crate::modules::cron::types::Schedule, String> { | |
| use crate::modules::cron::types::Schedule; | |
| let every = args.get("every_minutes").and_then(|v| v.as_u64()); | |
| let hour = args.get("daily_at_hour").and_then(|v| v.as_u64()); | |
| let minute = args.get("daily_at_minute").and_then(|v| v.as_u64()); | |
| match (every, hour, minute) { | |
| (Some(_), Some(_), _) | (Some(_), _, Some(_)) => { | |
| Err("provide either every_minutes OR daily_at_*; not both".into()) | |
| } | |
| (Some(m), None, None) => { | |
| let m = u32::try_from(m).map_err(|_| "every_minutes out of range".to_string())?; | |
| Ok(Schedule::EveryMinutes { minutes: m }) | |
| } | |
| (None, Some(h), Some(m)) => Ok(Schedule::DailyAt { | |
| hour: h as u8, | |
| minute: m as u8, | |
| }), | |
| (None, Some(_), None) | (None, None, Some(_)) => { | |
| Err("daily_at requires both daily_at_hour and daily_at_minute".into()) | |
| } | |
| (None, None, None) => Err("missing schedule: provide every_minutes or daily_at_*".into()), | |
| } | |
| fn parse_schedule_from_args(args: &Value) -> Result<crate::modules::cron::types::Schedule, String> { | |
| use crate::modules::cron::types::Schedule; | |
| let every = args.get("every_minutes").and_then(|v| v.as_u64()); | |
| let hour = args.get("daily_at_hour").and_then(|v| v.as_u64()); | |
| let minute = args.get("daily_at_minute").and_then(|v| v.as_u64()); | |
| match (every, hour, minute) { | |
| (Some(_), Some(_), _) | (Some(_), _, Some(_)) => { | |
| Err("provide either every_minutes OR daily_at_*; not both".into()) | |
| } | |
| (Some(m), None, None) => { | |
| let m = u32::try_from(m).map_err(|_| "every_minutes out of range".to_string())?; | |
| Ok(Schedule::EveryMinutes { minutes: m }) | |
| } | |
| (None, Some(h), Some(m)) => { | |
| let hour = u8::try_from(h).map_err(|_| "daily_at_hour out of range".to_string())?; | |
| let minute = | |
| u8::try_from(m).map_err(|_| "daily_at_minute out of range".to_string())?; | |
| if hour > 23 { | |
| return Err("daily_at_hour must be in 0..=23".into()); | |
| } | |
| if minute > 59 { | |
| return Err("daily_at_minute must be in 0..=59".into()); | |
| } | |
| Ok(Schedule::DailyAt { hour, minute }) | |
| } | |
| (None, Some(_), None) | (None, None, Some(_)) => { | |
| Err("daily_at requires both daily_at_hour and daily_at_minute".into()) | |
| } | |
| (None, None, None) => Err("missing schedule: provide every_minutes or daily_at_*".into()), | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src-tauri/src/modules/mcp/native.rs` around lines 481 - 503, The
parse_schedule_from_args function currently casts daily_at_hour and
daily_at_minute to u8 directly (via as u8), which silently wraps out-of-range
values; update parse_schedule_from_args to validate that hour and minute are
within valid ranges (0..=23 for hour and 0..=59 for minute) before narrowing,
converting them with try_from or explicit bounds checks and returning a
descriptive Err (e.g., "daily_at_hour out of range" / "daily_at_minute out of
range") if validation fails so Schedule::DailyAt only receives valid u8 values.
Comment on lines
+19
to
+21
| /// JSON-RPC over stdio must be strictly serialized (one request in flight); parallel | ||
| /// `tools/call` / `notify` would interleave NDJSON lines and corrupt the MCP session. | ||
| rpc_mutex: Mutex<()>, |
There was a problem hiding this comment.
Don't hold the RPC lock for the full request/response lifetime.
stdin is already mutex-protected, so NDJSON writes cannot interleave. Holding rpc_mutex from Line 121 until the response arrives turns the transport into global single-flight: one long tools/call now blocks every other call and notify for up to the timeout window.
⚙️ Narrow the lock to write/flush only
- let _rpc_guard = self.rpc_mutex.lock().await;
let id = self.next_id.fetch_add(1, Ordering::Relaxed);
let req = JsonRpcRequest::new(id, method, params);
let mut payload = serde_json::to_vec(&req).map_err(|e| format!("encode request: {e}"))?;
payload.push(b'\n');
@@
{
+ let _rpc_guard = self.rpc_mutex.lock().await;
let mut stdin = self.stdin.lock().await;
if let Err(e) = stdin.write_all(&payload).await {
self.pending.lock().await.remove(&id);
return Err(format!("write stdin: {e}"));
}
@@
- let _rpc_guard = self.rpc_mutex.lock().await;
let mut msg = serde_json::json!({
"jsonrpc": "2.0",
"method": method,
});
@@
- let mut stdin = self.stdin.lock().await;
+ let _rpc_guard = self.rpc_mutex.lock().await;
+ let mut stdin = self.stdin.lock().await;Also applies to: 99-100, 121-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src-tauri/src/modules/mcp/transport.rs` around lines 19 - 21, The rpc_mutex
should only protect the NDJSON write/flush, not the entire request/response
lifetime: change the code paths that currently lock rpc_mutex (the transport
methods that perform outgoing JSON-RPC calls/notifications—e.g., the code around
rpc_mutex usage in the request/send/call logic referenced between lines ~99-165)
so that you acquire the Mutex<()>, perform the ndjson write and flush, then
immediately drop/release the lock before awaiting the response; ensure you still
register the pending response handler (e.g., insert the response oneshot or
pending entry keyed by id into whatever pending map you have) before releasing
the lock so responses can be matched, and do not hold rpc_mutex while awaiting
the oneshot or handling the incoming response.
Comment on lines
+119
to
+122
| /// Recursion guard for `task_spawn`. Incremented before a child agent turn | ||
| /// runs, decremented after; child agents see a non-zero value and refuse to | ||
| /// spawn further sub-tasks. Capped via [`crate::modules::mcp::native::TASK_SPAWN_MAX_DEPTH`]. | ||
| pub task_spawn_depth: Arc<AtomicU8>, |
There was a problem hiding this comment.
task_spawn_depth also bleeds across concurrent conversations.
This counter guards a task tree, but Line 122 stores it once for the whole process. If one conversation is inside a child task, an unrelated top-level conversation can be refused as already at depth 1 even though its own tree is empty.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src-tauri/src/shared/state.rs` around lines 119 - 122, The task_spawn_depth
Arc<AtomicU8> in shared state is global and causes unrelated conversations to
interfere; move the depth counter to be per-conversation (e.g., add a
task_spawn_depth AtomicU8 field to the conversation-specific struct or maintain
a HashMap<conversation_id, AtomicU8> guarded by the existing state) instead of
the global pub task_spawn_depth: Arc<AtomicU8> in state.rs, ensure
creations/clones of child agents read and increment the conversation-local
counter, and enforce the existing cap via TASK_SPAWN_MAX_DEPTH from
crate::modules::mcp::native when incrementing/decrementing so each conversation
has its own recursion guard.
Comment on lines
+11
to
+13
| ARG GOPLS_VERSION=latest | ||
| ENV GOTOOLCHAIN=auto | ||
| RUN GOBIN=/out go install golang.org/x/tools/gopls@${GOPLS_VERSION} |
There was a problem hiding this comment.
❓ Verification inconclusive
Script executed:
find . -name "Dockerfile" -o -name "mcp-tools.json" | head -20Repository: pengine-ai/pengine
Repository: pengine-ai/pengine
Exit code: 0
stdout:
./deployment/Dockerfile
./tools/brave-search/Dockerfile
./tools/fetch/Dockerfile
./tools/file-manager/Dockerfile
./tools/git/Dockerfile
./tools/lsp/Dockerfile
./tools/mcp-tools.json
./tools/memory/Dockerfile
./tools/notebook/Dockerfile
./tools/ripgrep/Dockerfile
./tools/sequential-thinking/Dockerfile
./tools/shell/Dockerfile
./tools/time/Dockerfile
Script executed:
cat tools/lsp/DockerfileRepository: pengine-ai/pengine
Repository: pengine-ai/pengine
Exit code: 0
stdout:
# cclsp bridges MCP to one or more LSP servers. Image bakes in TypeScript,
# Python, Go, and Rust LSPs and a default config. Users can derive from this
# image and edit `/mcp/cclsp.json` for additional languages.
#
# Upstream MCP npm package + version: CI passes --build-arg from tools/mcp-tools.json (`upstream_mcp_npm`).
# Stage 1 — build `gopls` from source (no upstream binary release exists; `go install` is the
# only supported install path). Kept in its own stage so the Go toolchain is not in the final image.
ARG GOLANG_VERSION=1.22-bookworm
FROM golang:${GOLANG_VERSION} AS gopls-builder
ARG GOPLS_VERSION=latest
ENV GOTOOLCHAIN=auto
RUN GOBIN=/out go install golang.org/x/tools/gopls@${GOPLS_VERSION}
# `node:22-bookworm` (non-slim) ships `useradd` / `groupadd` out of the box; the
# `-slim` variant keeps the `passwd` dpkg record but strips its binaries.
FROM node:22-bookworm
ARG TARGETARCH
ARG UPSTREAM_MCP_NPM_PACKAGE=cclsp
ARG UPSTREAM_MCP_NPM_VERSION=0.7.0
ARG TS_LSP_VERSION=5.1.3
ARG PYLSP_VERSION=1.14.0
ENV PIP_NO_CACHE_DIR=1 \
PIP_DISABLE_PIP_VERSION_CHECK=1 \
PIP_BREAK_SYSTEM_PACKAGES=1 \
NODE_ENV=production \
CCLSP_CONFIG_PATH=/mcp/cclsp.json \
PATH=/opt/lsp:/usr/local/bin:/usr/local/sbin:/usr/sbin:/usr/bin:/sbin:/bin
RUN apt-get update \
&& apt-get install -y --no-install-recommends \
python3 python3-pip ca-certificates curl xz-utils passwd \
&& rm -rf /var/lib/apt/lists/*
RUN useradd -r -u 10001 -m -d /home/mcp mcp \
&& mkdir -p /mcp /opt/lsp \
&& chown mcp:mcp /mcp /opt/lsp
# rust-analyzer official binary release. The `latest/download/` URL always
# resolves to the most recent tag; rebuild the image to pick up upstream updates.
RUN ARCH="${TARGETARCH:-$(dpkg --print-architecture)}" \
&& case "${ARCH}" in \
amd64) RA_TARGET=x86_64-unknown-linux-gnu ;; \
arm64) RA_TARGET=aarch64-unknown-linux-gnu ;; \
*) echo "unsupported arch: ${ARCH}" >&2; exit 1 ;; \
esac \
&& curl -fsSL "https://github.com/rust-lang/rust-analyzer/releases/latest/download/rust-analyzer-${RA_TARGET}.gz" \
| gunzip > /opt/lsp/rust-analyzer \
&& chmod +x /opt/lsp/rust-analyzer
COPY --from=gopls-builder /out/gopls /opt/lsp/gopls
RUN pip install --no-cache-dir "python-lsp-server==${PYLSP_VERSION}"
WORKDIR /mcp
RUN npm install --omit=dev --prefix /mcp \
"${UPSTREAM_MCP_NPM_PACKAGE}@${UPSTREAM_MCP_NPM_VERSION}" \
"typescript-language-server@${TS_LSP_VERSION}" \
"typescript" \
&& npm cache clean --force \
&& rm -rf /root/.npm \
&& chown -R mcp:mcp /mcp
COPY cclsp.json /mcp/cclsp.json
USER mcp
ENTRYPOINT ["node", "/mcp/node_modules/cclsp/dist/index.js"]
Script executed:
cat tools/mcp-tools.json 2>/dev/null || echo "File not found"Repository: pengine-ai/pengine
Repository: pengine-ai/pengine
Exit code: 0
stdout:
{
"schema_version": 1,
"generated_at": "2026-04-29T00:00:00Z",
"catalog_revision": 8,
"valid_until": "2026-05-29T00:00:00Z",
"minimum_pengine_version": "0.5.0",
"tools": [
{
"id": "pengine/file-manager",
"name": "File Manager",
"description": "Filesystem MCP in a container. Add folders in MCP Tools; each mounts at /app/<folder-name>. Install works before any folder is set.",
"image": "ghcr.io/pengine-ai/pengine-file-manager",
"current": "0.1.0",
"versions": [
{
"version": "0.1.0",
"digest": "sha256:placeholder",
"released_at": "2026-04-12T00:00:00Z",
"yanked": false,
"revoked": false,
"security": false
}
],
"container_read_only_rootfs": false,
"mount_read_only": false,
"mount_workspace": true,
"append_workspace_roots": true,
"direct_return": true,
"upstream_mcp_npm": {
"package": "@modelcontextprotocol/server-filesystem",
"version": "2026.1.14"
},
"mcp_server_cmd": [],
"commands": [
{ "name": "read_text_file", "description": "Read a file as UTF-8 text; optional head/tail line limits" },
{ "name": "read_media_file", "description": "Read image or audio as base64 with MIME type" },
{ "name": "read_multiple_files", "description": "Read several files in one call" },
{ "name": "write_file", "description": "Create or overwrite a file" },
{ "name": "edit_file", "description": "Pattern-based selective edits with optional dry run" },
{ "name": "create_directory", "description": "Create a directory (and parents)" },
{ "name": "list_directory", "description": "List entries with [FILE]/[DIR] prefixes" },
{ "name": "list_directory_with_sizes", "description": "List directory with sizes and optional sort" },
{ "name": "move_file", "description": "Move or rename a file or directory" },
{ "name": "search_files", "description": "Recursive glob search under a path" },
{ "name": "directory_tree", "description": "Recursive JSON tree; requires absolute `path` under a mounted root (e.g. /app/<folder>)" },
{ "name": "get_file_info", "description": "Metadata: size, times, type, permissions" },
{ "name": "list_allowed_directories", "description": "List MCP roots currently allowed" }
],
"limits": {
"cpus": "0.5",
"memory": "256m",
"timeout_secs": 30
}
},
{
"id": "pengine/fetch",
"name": "Fetch",
"description": "Official MCP fetch server: fetches URLs and returns markdown-friendly content. By default the server respects robots.txt. To opt in for this install, set catalog field ignore_robots_txt to true (appends --ignore-robots-txt) or add that flag under mcp_server_cmd in advanced config — only if you accept bypassing robots for all fetched hosts. The robots_ignore_allowlist field is reserved for future per-host policy and is currently informational.",
"image": "ghcr.io/pengine-ai/pengine-fetch",
"current": "0.1.0",
"versions": [
{
"version": "0.1.0",
"digest": "sha256:placeholder",
"released_at": "2026-04-14T00:00:00Z",
"yanked": false,
"revoked": false,
"security": false
}
],
"container_read_only_rootfs": true,
"mount_read_only": true,
"mount_workspace": false,
"append_workspace_roots": false,
"direct_return": false,
"network_isolated": false,
"upstream_mcp_pypi": {
"package": "mcp-server-fetch",
"version": "2025.4.7"
},
"mcp_server_cmd": [],
"ignore_robots_txt": false,
"robots_ignore_allowlist": [],
"commands": [
{ "name": "fetch", "description": "Fetch a URL and return extracted content (markdown by default)" }
],
"limits": {
"cpus": "0.5",
"memory": "512m",
"timeout_secs": 60
}
},
{
"id": "pengine/git",
"name": "Git",
"description": "Official MCP git server: status, diff, log, branches, and commits against repos under your mounted folders (use paths like /app/<folder>).",
"image": "ghcr.io/pengine-ai/pengine-git",
"current": "0.1.0",
"versions": [
{
"version": "0.1.0",
"digest": "sha256:placeholder",
"released_at": "2026-04-14T00:00:00Z",
"yanked": false,
"revoked": false,
"security": false
}
],
"container_read_only_rootfs": false,
"mount_read_only": false,
"mount_workspace": true,
"append_workspace_roots": false,
"direct_return": true,
"upstream_mcp_pypi": {
"package": "mcp-server-git",
"version": "2026.1.14"
},
"mcp_server_cmd": [],
"commands": [
{ "name": "git_status", "description": "Working tree status for a repository" },
{ "name": "git_diff_unstaged", "description": "Diff unstaged changes" },
{ "name": "git_diff_staged", "description": "Diff staged changes" },
{ "name": "git_diff", "description": "Diff against a branch or commit" },
{ "name": "git_commit", "description": "Create a commit" },
{ "name": "git_add", "description": "Stage files" },
{ "name": "git_reset", "description": "Unstage all staged changes" },
{ "name": "git_log", "description": "Commit history with optional date filters" },
{ "name": "git_branch", "description": "List branches" },
{ "name": "git_checkout", "description": "Switch branches" }
],
"limits": {
"cpus": "0.5",
"memory": "512m",
"timeout_secs": 60
}
},
{
"id": "pengine/sequential-thinking",
"name": "Sequential Thinking",
"description": "Official MCP sequential-thinking server for structured step-by-step reasoning.",
"image": "ghcr.io/pengine-ai/pengine-sequential-thinking",
"current": "0.1.0",
"versions": [
{
"version": "0.1.0",
"digest": "sha256:placeholder",
"released_at": "2026-04-14T00:00:00Z",
"yanked": false,
"revoked": false,
"security": false
}
],
"container_read_only_rootfs": true,
"mount_read_only": true,
"mount_workspace": false,
"append_workspace_roots": false,
"direct_return": false,
"upstream_mcp_npm": {
"package": "@modelcontextprotocol/server-sequential-thinking",
"version": "2025.12.18"
},
"mcp_server_cmd": [],
"commands": [
{ "name": "sequentialthinking", "description": "Record and refine a sequence of reasoning steps" }
],
"limits": {
"cpus": "0.25",
"memory": "256m",
"timeout_secs": 30
}
},
{
"id": "pengine/time",
"name": "Time",
"description": "Official MCP time server: current time and timezone conversions.",
"image": "ghcr.io/pengine-ai/pengine-time",
"current": "0.1.0",
"versions": [
{
"version": "0.1.0",
"digest": "sha256:placeholder",
"released_at": "2026-04-14T00:00:00Z",
"yanked": false,
"revoked": false,
"security": false
}
],
"container_read_only_rootfs": true,
"mount_read_only": true,
"mount_workspace": false,
"append_workspace_roots": false,
"direct_return": true,
"upstream_mcp_pypi": {
"package": "mcp-server-time",
"version": "2026.1.26"
},
"mcp_server_cmd": [],
"commands": [
{ "name": "get_current_time", "description": "Current time in a timezone" },
{ "name": "convert_time", "description": "Convert a time between timezones" }
],
"limits": {
"cpus": "0.25",
"memory": "256m",
"timeout_secs": 30
}
},
{
"id": "pengine/brave-search",
"name": "Brave Search",
"description": "Brave Search MCP: web, local, video, image, news, and summarizer tools via the Brave Search API. After install, add BRAVE_API_KEY under Tool Engine → Container secrets (saved in mcp.json). Alternatively, export BRAVE_API_KEY in the shell before you start Pengine. Requires outbound network.",
"image": "ghcr.io/pengine-ai/pengine-brave-search",
"current": "0.1.0",
"versions": [
{
"version": "0.1.0",
"digest": "sha256:placeholder",
"released_at": "2026-04-17T00:00:00Z",
"yanked": false,
"revoked": false,
"security": false
}
],
"container_read_only_rootfs": true,
"mount_read_only": true,
"mount_workspace": false,
"append_workspace_roots": false,
"direct_return": false,
"network_isolated": false,
"upstream_mcp_npm": {
"package": "@brave/brave-search-mcp-server",
"version": "2.0.76"
},
"mcp_server_cmd": [],
"passthrough_env": ["BRAVE_API_KEY"],
"commands": [
{ "name": "brave_web_search", "description": "Web search with filtering, pagination, and optional AI summarization" },
{ "name": "brave_local_search", "description": "Local business and place discovery with ratings, hours, and AI descriptions" },
{ "name": "brave_video_search", "description": "Video search with metadata and thumbnails" },
{ "name": "brave_image_search", "description": "Image search with automatic fetching" },
{ "name": "brave_news_search", "description": "News search with freshness controls and breaking-news indicators" },
{ "name": "brave_summarizer", "description": "AI-powered summaries from web search results" }
],
"limits": {
"cpus": "0.5",
"memory": "512m",
"timeout_secs": 60
}
},
{
"id": "pengine/shell",
"name": "Shell",
"description": "Sandboxed shell-exec MCP. Runs commands inside the container; only paths you bind via MCP folders are visible to it. Pair with `pengine/file-manager` so the model can build, test, and run code it just wrote (Claude Code `Bash` parity).",
"image": "ghcr.io/pengine-ai/pengine-shell",
"current": "0.1.0",
"versions": [
{
"version": "0.1.0",
"digest": "sha256:placeholder",
"released_at": "2026-04-29T00:00:00Z",
"yanked": false,
"revoked": false,
"security": false
}
],
"container_read_only_rootfs": false,
"mount_read_only": false,
"mount_workspace": true,
"append_workspace_roots": false,
"direct_return": false,
"upstream_mcp_npm": {
"package": "@mako10k/mcp-shell-server",
"version": "2.7.1"
},
"mcp_server_cmd": [],
"commands": [
{ "name": "shell_execute", "description": "Run a shell command (foreground / background / detached / adaptive)" },
{ "name": "process_get_execution", "description": "Detailed execution info for a previous run" },
{ "name": "shell_set_default_workdir", "description": "Set the default working directory for subsequent shell calls" },
{ "name": "process_list", "description": "List running processes with filtering" },
{ "name": "process_terminate", "description": "Terminate a process with a specific signal" },
{ "name": "process_monitor", "description": "Start real-time monitoring for a running process" },
{ "name": "terminal_create", "description": "Create an interactive PTY-backed terminal session" },
{ "name": "terminal_send_input", "description": "Send input (with control codes) to a terminal" },
{ "name": "terminal_get_output", "description": "Read terminal output (ANSI-aware)" },
{ "name": "terminal_get_info", "description": "Get terminal session details" },
{ "name": "terminal_resize", "description": "Resize a terminal session" },
{ "name": "terminal_close", "description": "Close a terminal session" },
{ "name": "list_execution_outputs", "description": "List captured output files" },
{ "name": "read_execution_output", "description": "Read a captured output file" },
{ "name": "delete_execution_outputs", "description": "Delete captured output files" },
{ "name": "get_cleanup_suggestions", "description": "Recommend output files to clean up" },
{ "name": "perform_auto_cleanup", "description": "Apply retention policies to captured outputs" },
{ "name": "security_set_restrictions", "description": "Set runtime security restrictions" },
{ "name": "monitoring_get_stats", "description": "System-wide stats from the shell MCP" }
],
"limits": {
"cpus": "1.0",
"memory": "1024m",
"timeout_secs": 300
}
},
{
"id": "pengine/ripgrep",
"name": "Ripgrep",
"description": "High-throughput content search across mounted folders via the `rg` binary (Claude Code `Grep` parity). Use with `pengine/file-manager` for navigation; this server is read-only.",
"image": "ghcr.io/pengine-ai/pengine-ripgrep",
"current": "0.1.0",
"versions": [
{
"version": "0.1.0",
"digest": "sha256:placeholder",
"released_at": "2026-04-29T00:00:00Z",
"yanked": false,
"revoked": false,
"security": false
}
],
"container_read_only_rootfs": true,
"mount_read_only": true,
"mount_workspace": true,
"append_workspace_roots": false,
"direct_return": false,
"network_isolated": true,
"upstream_mcp_npm": {
"package": "mcp-ripgrep",
"version": "0.4.0"
},
"mcp_server_cmd": [],
"commands": [
{ "name": "search", "description": "ripgrep search with regex, file-type filters, and context lines" },
{ "name": "advanced-search", "description": "Search with extra ripgrep flags (multiline, fixed-strings, glob, …)" },
{ "name": "list-files", "description": "List files ripgrep would search under a path (respects .gitignore)" },
{ "name": "list-file-types", "description": "List the file-type aliases ripgrep understands" }
],
"limits": {
"cpus": "1.0",
"memory": "512m",
"timeout_secs": 60
}
},
{
"id": "pengine/lsp",
"name": "Language Server (LSP)",
"description": "Code intelligence via cclsp: go-to-definition, find-references, rename, diagnostics. Image bakes in TypeScript, Python, Go, and Rust LSPs (typescript-language-server, pylsp, gopls, rust-analyzer). Extend by deriving from this image and editing `/mcp/cclsp.json`.",
"image": "ghcr.io/pengine-ai/pengine-lsp",
"current": "0.1.0",
"versions": [
{
"version": "0.1.0",
"digest": "sha256:placeholder",
"released_at": "2026-04-29T00:00:00Z",
"yanked": false,
"revoked": false,
"security": false
}
],
"container_read_only_rootfs": false,
"mount_read_only": false,
"mount_workspace": true,
"append_workspace_roots": false,
"direct_return": false,
"upstream_mcp_npm": {
"package": "cclsp",
"version": "0.7.0"
},
"mcp_server_cmd": [],
"commands": [
{ "name": "find_definition", "description": "Find the definition of a symbol by name and kind in a file" },
{ "name": "find_references", "description": "Find all references to a symbol across the workspace" },
{ "name": "rename_symbol", "description": "Rename a symbol across the workspace (supports dry-run)" },
{ "name": "rename_symbol_strict", "description": "Rename a symbol at a specific position in a file" },
{ "name": "get_diagnostics", "description": "Errors / warnings / hints for a file from the language server" },
{ "name": "restart_server", "description": "Restart one or all backing LSP servers" }
],
"limits": {
"cpus": "1.0",
"memory": "1024m",
"timeout_secs": 120
}
},
{
"id": "pengine/notebook",
"name": "Notebook",
"description": "Jupyter-notebook (`.ipynb`) editor MCP — create, edit, move, search cells without a running kernel (Claude Code `NotebookEdit` parity). Operates on files under your mounted folders; `--allow-root /app` is baked in.",
"image": "ghcr.io/pengine-ai/pengine-notebook",
"current": "0.1.0",
"versions": [
{
"version": "0.1.0",
"digest": "sha256:placeholder",
"released_at": "2026-04-29T00:00:00Z",
"yanked": false,
"revoked": false,
"security": false
}
],
"container_read_only_rootfs": false,
"mount_read_only": false,
"mount_workspace": true,
"append_workspace_roots": false,
"direct_return": false,
"upstream_mcp_pypi": {
"package": "cursor-notebook-mcp",
"version": "0.3.1"
},
"mcp_server_cmd": [],
"commands": [
{ "name": "notebook_create", "description": "Create a new empty notebook file" },
{ "name": "notebook_delete", "description": "Delete a notebook file" },
{ "name": "notebook_rename", "description": "Move or rename a notebook" },
{ "name": "notebook_read", "description": "Read full notebook structure" },
{ "name": "notebook_read_cell", "description": "Read a single cell's source" },
{ "name": "notebook_add_cell", "description": "Insert a code/markdown cell after a given index" },
{ "name": "notebook_edit_cell", "description": "Replace a cell's source content" },
{ "name": "notebook_delete_cell", "description": "Delete a cell at a given index" },
{ "name": "notebook_change_cell_type", "description": "Convert between code / markdown / raw" },
{ "name": "notebook_duplicate_cell", "description": "Duplicate a cell N times" },
{ "name": "notebook_get_cell_count", "description": "Total cell count" },
{ "name": "notebook_read_metadata", "description": "Read notebook-level metadata" },
{ "name": "notebook_edit_metadata", "description": "Update notebook-level metadata" },
{ "name": "notebook_search", "description": "Search across cells with context" },
{ "name": "notebook_export", "description": "Export to Python / HTML / PDF" },
{ "name": "notebook_validate", "description": "Validate against the nbformat schema" }
],
"limits": {
"cpus": "0.5",
"memory": "512m",
"timeout_secs": 60
}
},
{
"id": "pengine/memory",
"name": "Memory",
"description": "Official MCP memory server: knowledge-graph style entities, relations, and observations; state file is stored in a dedicated folder (default under app data, or pick your own).",
"image": "ghcr.io/pengine-ai/pengine-memory",
"current": "0.1.0",
"versions": [
{
"version": "0.1.0",
"digest": "sha256:placeholder",
"released_at": "2026-04-14T00:00:00Z",
"yanked": false,
"revoked": false,
"security": false
}
],
"container_read_only_rootfs": false,
"mount_read_only": true,
"mount_workspace": false,
"append_workspace_roots": false,
"direct_return": false,
"upstream_mcp_npm": {
"package": "@modelcontextprotocol/server-memory",
"version": "2026.1.26"
},
"private_folder": {
"container_path": "/mcp/data",
"file_env_var": "MEMORY_FILE_PATH",
"file_extension": "json"
},
"mcp_server_cmd": [],
"commands": [
{ "name": "create_entities", "description": "Create entities in the knowledge graph" },
{ "name": "create_relations", "description": "Create relations between entities" },
{ "name": "add_observations", "description": "Add observations to entities" },
{ "name": "read_graph", "description": "Read the knowledge graph" },
{ "name": "search_nodes", "description": "Search nodes in the graph" },
{ "name": "open_nodes", "description": "Open nodes by name" },
{ "name": "delete_entities", "description": "Delete entities" },
{ "name": "delete_observations", "description": "Delete observations" },
{ "name": "delete_relations", "description": "Delete relations" }
],
"limits": {
"cpus": "0.25",
"memory": "256m",
"timeout_secs": 30
}
}
]
}
Pin every bundled language-server toolchain to ensure reproducible builds.
The Dockerfile currently uses GOPLS_VERSION=latest, rust-analyzer/releases/latest, and unversioned typescript. This makes pengine/lsp:0.1.0 non-reproducible—rebuilding the same image version later will pull newer versions of these tools, potentially breaking cclsp compatibility silently. Additionally, the rust-analyzer download lacks checksum verification, creating a supply-chain risk.
Pin versions and verify release assets
-ARG GOPLS_VERSION=latest
+ARG GOPLS_VERSION=vX.Y.Z
+ARG RUST_ANALYZER_VERSION=YYYY-MM-DD
+ARG RUST_ANALYZER_SHA256_AMD64=...
+ARG RUST_ANALYZER_SHA256_ARM64=...
+ARG TYPESCRIPT_VERSION=X.Y.Z
@@
-RUN ARCH="${TARGETARCH:-$(dpkg --print-architecture)}" \
+RUN ARCH="${TARGETARCH:-$(dpkg --print-architecture)}" \
&& case "${ARCH}" in \
- amd64) RA_TARGET=x86_64-unknown-linux-gnu ;; \
- arm64) RA_TARGET=aarch64-unknown-linux-gnu ;; \
+ amd64) RA_TARGET=x86_64-unknown-linux-gnu; RA_SHA256="${RUST_ANALYZER_SHA256_AMD64}" ;; \
+ arm64) RA_TARGET=aarch64-unknown-linux-gnu; RA_SHA256="${RUST_ANALYZER_SHA256_ARM64}" ;; \
*) echo "unsupported arch: ${ARCH}" >&2; exit 1 ;; \
esac \
- && curl -fsSL "https://github.com/rust-lang/rust-analyzer/releases/latest/download/rust-analyzer-${RA_TARGET}.gz" \
- | gunzip > /opt/lsp/rust-analyzer \
+ && curl -fsSL "https://github.com/rust-lang/rust-analyzer/releases/download/${RUST_ANALYZER_VERSION}/rust-analyzer-${RA_TARGET}.gz" \
+ -o /tmp/rust-analyzer.gz \
+ && echo "${RA_SHA256} /tmp/rust-analyzer.gz" | sha256sum -c - \
+ && gunzip -c /tmp/rust-analyzer.gz > /opt/lsp/rust-analyzer \
&& chmod +x /opt/lsp/rust-analyzer
@@
"${UPSTREAM_MCP_NPM_PACKAGE}@${UPSTREAM_MCP_NPM_VERSION}" \
"typescript-language-server@${TS_LSP_VERSION}" \
- "typescript" \
+ "typescript@${TYPESCRIPT_VERSION}" \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/lsp/Dockerfile` around lines 11 - 13, The Dockerfile currently installs
tools without pinned versions (ARG GOPLS_VERSION=latest and unversioned
typescript/rust-analyzer) and downloads rust-analyzer without checksum
verification; update the Dockerfile to accept explicit version ARGs (e.g.,
GOPLS_VERSION, RUST_ANALYZER_VERSION, TYPESCRIPT_VERSION) and replace
occurrences such as the go install golang.org/x/tools/gopls@${GOPLS_VERSION} and
the rust-analyzer download URL (currently using releases/latest) to use the
pinned version variables, and add a checksum check for the rust-analyzer
artifact (download .sha256 or compute and compare sha256sum) before installing
to ensure reproducible, verifiable builds.
- Refactored the default container path function to derive paths from environment variables for better flexibility. - Introduced new functions for merging directory tree exclude patterns and handling filesystem MCP path arguments. - Updated timeout settings for various MCP tool calls to optimize performance and prevent long stalls. - Added friendly tool action labels for improved user experience in CLI output. - Implemented human-readable status lines for tool actions in the CLI, enhancing clarity during operations. - Added new probe files and scripts for testing the MCP file manager functionality.
…dules - Simplified conditional checks and function calls for better clarity in the agent module. - Enhanced formatting of assertions and function signatures in the CLI handlers for improved readability. - Consolidated line breaks and streamlined code structure across multiple functions.
There was a problem hiding this comment.
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-tauri/src/modules/mcp/client.rs (1)
105-123:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate
notifications/initializedfailures.This handshake step is currently best-effort. If the final notification write/post fails,
connect*()can still continue and cache tools from a session that never fully initialized.Suggested fix
- let _ = transport.notify("notifications/initialized", None).await; + transport.notify("notifications/initialized", None).await?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/modules/mcp/client.rs` around lines 105 - 123, The notify call for "notifications/initialized" in async fn initialize is currently ignored (let _ = transport.notify(...).await); change it to propagate failures instead of swallowing them by awaiting and returning the error (e.g. use transport.notify("notifications/initialized", None).await? or map_err to a descriptive String and return Err), so that initialize returns an error when the final notification fails and prevents continuing with a partially initialized session; update the initialize function to handle the notify result (transport.notify) rather than discarding it.
♻️ Duplicate comments (7)
src-tauri/src/modules/mcp/http_transport.rs (1)
106-123:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake
notify()enforce the same HTTP failure semantics ascall_with_timeout().Right now
notify()treats HTTP 4xx/5xx as success and bypasses the per-call deadline path. That meansnotifications/initializedcan be rejected silently, and a stalled server can hold the connect flow much longer than the bounded request/response calls.Suggested fix
- request + let resp = request .send() .await .map_err(|e| format!("http notify: {e}"))?; + let status = resp.status(); + if !status.is_success() { + let body = resp.text().await.unwrap_or_default(); + return Err(format!( + "http notify {}: HTTP {status} {}", + self.url, + truncate_for_error(&body, 256) + )); + } Ok(())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/modules/mcp/http_transport.rs` around lines 106 - 123, notify currently sends the HTTP request and ignores non-2xx responses and the per-call deadline behavior used elsewhere; update notify (the method on the same struct that has client, url, headers) to follow the same semantics as call_with_timeout by sending the request through the same timeout/deadline path or by reusing call_with_timeout, then check the HTTP response status and return Err on non-success (e.g., use resp.status().is_success() and map non-2xx to an Err with the status/body), rather than treating any HTTP response as success.src-tauri/src/modules/mcp/transport.rs (1)
19-21:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNarrow
rpc_mutexto write/flush only.
stdinalready serializes NDJSON writes. Holdingrpc_mutexuntil the response arrives turns stdio MCP into global single-flight, so one slowtools/callblocks every other request/notification for up to the timeout window.Suggested fix
- let _rpc_guard = self.rpc_mutex.lock().await; let id = self.next_id.fetch_add(1, Ordering::Relaxed); let req = JsonRpcRequest::new(id, method, params); let mut payload = serde_json::to_vec(&req).map_err(|e| format!("encode request: {e}"))?; payload.push(b'\n'); let (tx, rx) = oneshot::channel(); self.pending.lock().await.insert(id, tx); { + let _rpc_guard = self.rpc_mutex.lock().await; let mut stdin = self.stdin.lock().await; if let Err(e) = stdin.write_all(&payload).await { self.pending.lock().await.remove(&id); return Err(format!("write stdin: {e}")); }Also applies to: 121-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/modules/mcp/transport.rs` around lines 19 - 21, The rpc_mutex is currently held across the entire request/response roundtrip, turning stdio MCP into a global single-flight; change usage so rpc_mutex (the Mutex<()> named rpc_mutex) only guards writing/flushing NDJSON to stdout/stderr: acquire rpc_mutex, perform serialization + flush, then drop the lock immediately before awaiting the response. For request paths that currently hold rpc_mutex while waiting (e.g., the functions that send RPC requests and await replies between the write and read phases — search for uses of rpc_mutex around send_request/call), move the await/response handling outside the lock and use per-request correlation (existing request-id map or a oneshot/future) to receive the reply; notifications should still acquire rpc_mutex only for the write/flush and then release it. Ensure any read-loop or response-dispatching code (lines ~121-156 area) matches this: writes under rpc_mutex, response delivery via the dispatcher without holding rpc_mutex.src-tauri/src/modules/cli/handlers.rs (4)
641-668:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRefresh
state.cached_filesystem_pathsafterfs add/remove.This updates
mcp.jsonon disk, but the in-memory filesystem roots stay stale.ask_in_session()later readsstate.cached_filesystem_pathsfor mention expansion and default workspace-path inference, so newly added or removed roots do not take effect until some unrelated refresh happens.Suggested fix
mcp_service::set_filesystem_allowed_paths(&mut cfg, &paths); if let Err(e) = mcp_service::save_config(&state.mcp_config_path, &cfg) { return CliReply::error(format!("save: {e}")); } + *state.cached_filesystem_paths.write().await = paths.clone(); CliReply::code("bash", format!("{action}: {path}"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/modules/cli/handlers.rs` around lines 641 - 668, After successfully updating cfg and saving it, refresh the in-memory cache by assigning the new paths into state.cached_filesystem_paths so subsequent calls (like ask_in_session) see the change; specifically, in the "add" | "remove" branch in handlers.rs (after mcp_service::set_filesystem_allowed_paths and successful mcp_service::save_config) update state.cached_filesystem_paths = paths (or paths.clone() as needed) so the in-memory list matches the persisted mcp.json.
332-361:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate the full
configbatch before writing any setting.The loop persists each pair immediately. If an early entry is valid and a later one is not, the command returns an error after partially applying the batch, which is hard to reason about from the CLI. Parse/validate all pairs first, then commit them.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/modules/cli/handlers.rs` around lines 332 - 361, The loop currently applies each key=value immediately and returns on first validation error; instead, first parse and validate the entire kvs batch into a temporary map/struct (e.g. collect parsed entries and call user_settings::save_skills_hint_max_bytes for each value but do not write state yet), accumulating any parse/validation errors and returning a single CliReply::error if any fail; only after all entries are validated, acquire the state locks (e.g. state.skills_hint_max_bytes.write().await) and commit the validated/clamped values and then build the applied vector to return. Ensure you still handle unknown keys (the match arm for "skills_hint_max_bytes") during validation and reference the same symbols (kvs, applied, user_settings::save_skills_hint_max_bytes, state.skills_hint_max_bytes, CliReply::error) so changes are localized.
136-155:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEstimate
/costfrom the models recorded on the session turns.This classifies only
state.preferred_ollama_model, so changing/modelmid-session can retroactively flip the whole session between$0.00and cloud pricing.CliSessionalready keeps the per-turn model; cost should be derived froms.turns[*].model, not the current preference.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/modules/cli/handlers.rs` around lines 136 - 155, The cost estimate currently uses state.preferred_ollama_model (variable model) and ollama::classify_model once, which lets switching /model mid-session retroactively change costs; instead iterate the session turns in CliSession (s.turns) and classify each turn's model (turn.model) via ollama::classify_model to accumulate token counts separately for Local vs Cloud turns, then compute in_cost/out_cost per-turn or aggregated by kind and build cost_line from those totals (replace the single-model logic around cost_line with this per-turn aggregation while keeping the same formatting and ModelKind handling).
681-707:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRoute
logs --followthrough the selected sink.
follow_logs_from_broadcast()writes raw lines withprintln!/eprintln!, sopengine --json logs --followbypasses the CLI envelope contract and emits plain text instead of structuredCliReplyoutput. This needs to render through the sink path the same way the non-follow commands do.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/modules/cli/handlers.rs` around lines 681 - 707, The follow path currently prints directly to stdout/stderr from follow_logs_from_broadcast (using println!/eprintln!), bypassing the CLI sink contract; change follow_logs_from_broadcast (called by logs and referencing AppState.log_tx and format_log_line) to emit each formatted line through the same sink/renderer used by non-follow commands (the CLI sink helper used by tail_logs_from_audit or the AppState sink), e.g. replace println!/eprintln! with the sink-emission call so output is wrapped as CliReply-compatible messages (and propagate any errors to produce a proper CliReply rather than raw text), and still return the final CliReply::code("bash","log stream closed") when the broadcast closes.src-tauri/src/modules/agent/mod.rs (1)
1193-1210:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReapply the plan-mode filter after routing escalation.
tool_ctxgets rebuilt fromfull_tool_context()here, but the read-only filter only ran on the initial subset. In plan mode, step 0 can still escalate into an unfiltered full catalog and expose write tools on the next iteration.Suggested fix
if step == 0 && !routing_escalated && tool_ctx.used_subset && tool_calls.is_empty() && content.is_empty() { routing_escalated = true; tool_ctx = { let reg = state.mcp.read().await; reg.full_tool_context(allow_brave_web_search) }; + if plan_mode { + plan_mode_filter_writes(&mut tool_ctx.tools_json); + } state .emit_log( "tool_ctx", &format!("escalate full catalog ({} tools)", tool_ctx.active_count), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/modules/agent/mod.rs` around lines 1193 - 1210, When routing_escalated triggers and you rebuild tool_ctx via reg.full_tool_context(allow_brave_web_search), reapply the plan-mode/read-only filter so the new tool_ctx does not expose write tools; specifically, after the assignment to tool_ctx in the branch that sets routing_escalated = true (the block that calls full_tool_context), run the same plan-mode filter logic used for the initial subset (the code that inspects tool_ctx.used_subset/read-only flags) to filter out write-capable tools before emitting the log and continuing, ensuring behavior is identical to the initial tool_ctx creation flow.
🤖 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-tauri/src/modules/agent/mod.rs`:
- Around line 231-250: The task_spawn logic currently does a non-atomic load +
fetch_add which allows races; replace that with an atomic compare_exchange loop
on state.task_spawn_depth that attempts to increment only if the observed value
is strictly less than crate::modules::mcp::native::TASK_SPAWN_MAX_DEPTH and
returns the Err if the cap is reached, then proceed to call run_system_turn;
also ensure the decrement (fetch_sub) always runs (use a drop guard or
finally-style RAII guard) so the counter is released even if run_system_turn
panics or returns early; reference symbols: task_spawn_depth,
TASK_SPAWN_MAX_DEPTH, run_system_turn.
- Around line 481-489: The current loop returns the first container whose host
path is a prefix of cwd_canon, which can choose a shallower root; change the
logic to track and return the deepest (longest) matching host path instead.
Iterate over pairs (host, container), compute hp and hcanon as now, and if
cwd_canon.starts_with(&hcanon) compare hcanon.len() (or
hcanon.as_os_str().len()) with the best match seen so far; update the best
container when you find a longer match and after the loop return that best
container (falling back to pairs.first().map(|(_, c)| c.clone()) if no match).
Use the same variables (pairs, host, container, hp, hcanon, cwd_canon) so the
change is minimal.
- Around line 1506-1574: The plan_mode_filter_writes behavior misses tools like
"git_commit" because WRITE_TOKENS doesn't include "git" or "commit"; update the
WRITE_TOKENS constant (used by plan_mode_filter_writes and checked via
contains_write_token) to include tokens that cover git operations (e.g., "git"
and/or "commit") or add a special-case entry like "git_commit" so the name match
logic removes that tool; after updating WRITE_TOKENS, run/update the test
plan_mode_filter_drops_write_tools to assert "git_commit" is filtered out.
In `@tools/mcp-probe-filemanager/probe.mjs`:
- Around line 27-40: The probe currently uses the workspace root (base
calculated from rawRoot/hostRoot/containerRoot) as the target, which pollutes
the repo and causes flakiness; create a unique per-run temp subdirectory (e.g.,
build a probeRunDir from base plus a deterministic prefix using timestamp/UUID
such as `_probe_${Date.now()}_${random}`) and use that probeRunDir for all probe
artifacts instead of base; ensure probeRunDir is created (fs.mkdirSync(..., {
recursive: true })) and that the mapping for container vs host uses the
corresponding containerRoot/hostRoot variant so calls that reference container
paths still point to `/app/<label>/...`; update all places that reference fixed
`_probe_*` paths (including the code that creates/moves/edits files later in the
file) to use this new probeRunDir and optionally remove/cleanup the dir on exit.
---
Outside diff comments:
In `@src-tauri/src/modules/mcp/client.rs`:
- Around line 105-123: The notify call for "notifications/initialized" in async
fn initialize is currently ignored (let _ = transport.notify(...).await); change
it to propagate failures instead of swallowing them by awaiting and returning
the error (e.g. use transport.notify("notifications/initialized", None).await?
or map_err to a descriptive String and return Err), so that initialize returns
an error when the final notification fails and prevents continuing with a
partially initialized session; update the initialize function to handle the
notify result (transport.notify) rather than discarding it.
---
Duplicate comments:
In `@src-tauri/src/modules/agent/mod.rs`:
- Around line 1193-1210: When routing_escalated triggers and you rebuild
tool_ctx via reg.full_tool_context(allow_brave_web_search), reapply the
plan-mode/read-only filter so the new tool_ctx does not expose write tools;
specifically, after the assignment to tool_ctx in the branch that sets
routing_escalated = true (the block that calls full_tool_context), run the same
plan-mode filter logic used for the initial subset (the code that inspects
tool_ctx.used_subset/read-only flags) to filter out write-capable tools before
emitting the log and continuing, ensuring behavior is identical to the initial
tool_ctx creation flow.
In `@src-tauri/src/modules/cli/handlers.rs`:
- Around line 641-668: After successfully updating cfg and saving it, refresh
the in-memory cache by assigning the new paths into
state.cached_filesystem_paths so subsequent calls (like ask_in_session) see the
change; specifically, in the "add" | "remove" branch in handlers.rs (after
mcp_service::set_filesystem_allowed_paths and successful
mcp_service::save_config) update state.cached_filesystem_paths = paths (or
paths.clone() as needed) so the in-memory list matches the persisted mcp.json.
- Around line 332-361: The loop currently applies each key=value immediately and
returns on first validation error; instead, first parse and validate the entire
kvs batch into a temporary map/struct (e.g. collect parsed entries and call
user_settings::save_skills_hint_max_bytes for each value but do not write state
yet), accumulating any parse/validation errors and returning a single
CliReply::error if any fail; only after all entries are validated, acquire the
state locks (e.g. state.skills_hint_max_bytes.write().await) and commit the
validated/clamped values and then build the applied vector to return. Ensure you
still handle unknown keys (the match arm for "skills_hint_max_bytes") during
validation and reference the same symbols (kvs, applied,
user_settings::save_skills_hint_max_bytes, state.skills_hint_max_bytes,
CliReply::error) so changes are localized.
- Around line 136-155: The cost estimate currently uses
state.preferred_ollama_model (variable model) and ollama::classify_model once,
which lets switching /model mid-session retroactively change costs; instead
iterate the session turns in CliSession (s.turns) and classify each turn's model
(turn.model) via ollama::classify_model to accumulate token counts separately
for Local vs Cloud turns, then compute in_cost/out_cost per-turn or aggregated
by kind and build cost_line from those totals (replace the single-model logic
around cost_line with this per-turn aggregation while keeping the same
formatting and ModelKind handling).
- Around line 681-707: The follow path currently prints directly to
stdout/stderr from follow_logs_from_broadcast (using println!/eprintln!),
bypassing the CLI sink contract; change follow_logs_from_broadcast (called by
logs and referencing AppState.log_tx and format_log_line) to emit each formatted
line through the same sink/renderer used by non-follow commands (the CLI sink
helper used by tail_logs_from_audit or the AppState sink), e.g. replace
println!/eprintln! with the sink-emission call so output is wrapped as
CliReply-compatible messages (and propagate any errors to produce a proper
CliReply rather than raw text), and still return the final
CliReply::code("bash","log stream closed") when the broadcast closes.
In `@src-tauri/src/modules/mcp/http_transport.rs`:
- Around line 106-123: notify currently sends the HTTP request and ignores
non-2xx responses and the per-call deadline behavior used elsewhere; update
notify (the method on the same struct that has client, url, headers) to follow
the same semantics as call_with_timeout by sending the request through the same
timeout/deadline path or by reusing call_with_timeout, then check the HTTP
response status and return Err on non-success (e.g., use
resp.status().is_success() and map non-2xx to an Err with the status/body),
rather than treating any HTTP response as success.
In `@src-tauri/src/modules/mcp/transport.rs`:
- Around line 19-21: The rpc_mutex is currently held across the entire
request/response roundtrip, turning stdio MCP into a global single-flight;
change usage so rpc_mutex (the Mutex<()> named rpc_mutex) only guards
writing/flushing NDJSON to stdout/stderr: acquire rpc_mutex, perform
serialization + flush, then drop the lock immediately before awaiting the
response. For request paths that currently hold rpc_mutex while waiting (e.g.,
the functions that send RPC requests and await replies between the write and
read phases — search for uses of rpc_mutex around send_request/call), move the
await/response handling outside the lock and use per-request correlation
(existing request-id map or a oneshot/future) to receive the reply;
notifications should still acquire rpc_mutex only for the write/flush and then
release it. Ensure any read-loop or response-dispatching code (lines ~121-156
area) matches this: writes under rpc_mutex, response delivery via the dispatcher
without holding rpc_mutex.
🪄 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: 91fd6bd3-72fb-4c27-9897-905cd1d6752e
⛔ Files ignored due to path filters (1)
tools/mcp-probe-filemanager/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
src-tauri/src/modules/agent/mod.rssrc-tauri/src/modules/cli/handlers.rssrc-tauri/src/modules/mcp/client.rssrc-tauri/src/modules/mcp/http_transport.rssrc-tauri/src/modules/mcp/transport.rstools/_mcp_probe_fixture.txttools/_probe_edit.txttools/_probe_moved.txttools/mcp-probe-filemanager/package.jsontools/mcp-probe-filemanager/probe-in-image.shtools/mcp-probe-filemanager/probe.mjs
✅ Files skipped from review due to trivial changes (5)
- tools/_probe_moved.txt
- tools/_probe_edit.txt
- tools/_mcp_probe_fixture.txt
- tools/mcp-probe-filemanager/probe-in-image.sh
- tools/mcp-probe-filemanager/package.json
Comment on lines
+231
to
+250
| let depth_before = state.task_spawn_depth.load(Ordering::Acquire); | ||
| if depth_before >= crate::modules::mcp::native::TASK_SPAWN_MAX_DEPTH { | ||
| return Err(format!( | ||
| "task_spawn refused: recursion depth {depth_before} >= cap {}", | ||
| crate::modules::mcp::native::TASK_SPAWN_MAX_DEPTH | ||
| )); | ||
| } | ||
| state.task_spawn_depth.fetch_add(1, Ordering::AcqRel); | ||
| state | ||
| .emit_log( | ||
| "task", | ||
| &format!("spawn[{}]: {description}", depth_before + 1), | ||
| ) | ||
| .await; | ||
|
|
||
| let started = std::time::Instant::now(); | ||
| // `Box::pin` breaks the cycle [run_model_turn → run_task_spawn_inline → run_system_turn → run_model_turn] | ||
| // that would otherwise produce an infinitely-sized future. | ||
| let result = Box::pin(run_system_turn(state, &prompt, None)).await; | ||
| state.task_spawn_depth.fetch_sub(1, Ordering::AcqRel); |
There was a problem hiding this comment.
task_spawn_depth is acting like a global concurrency counter, not recursion depth.
The load + fetch_add sequence is not an atomic reservation, and the counter lives on shared AppState. Two concurrent top-level turns can both observe the same depth and both pass the cap check, or one user's nested spawn can make another user's first spawn fail. This should be tracked per call stack, or guarded with an atomic compare/exchange loop at minimum.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src-tauri/src/modules/agent/mod.rs` around lines 231 - 250, The task_spawn
logic currently does a non-atomic load + fetch_add which allows races; replace
that with an atomic compare_exchange loop on state.task_spawn_depth that
attempts to increment only if the observed value is strictly less than
crate::modules::mcp::native::TASK_SPAWN_MAX_DEPTH and returns the Err if the cap
is reached, then proceed to call run_system_turn; also ensure the decrement
(fetch_sub) always runs (use a drop guard or finally-style RAII guard) so the
counter is released even if run_system_turn panics or returns early; reference
symbols: task_spawn_depth, TASK_SPAWN_MAX_DEPTH, run_system_turn.
Comment on lines
+481
to
+489
| for (host, container) in &pairs { | ||
| let hp = Path::new(host.trim()); | ||
| let hcanon = std::fs::canonicalize(hp).unwrap_or_else(|_| hp.to_path_buf()); | ||
| if cwd_canon.starts_with(&hcanon) { | ||
| return Some(container.clone()); | ||
| } | ||
| } | ||
|
|
||
| pairs.first().map(|(_, c)| c.clone()) |
There was a problem hiding this comment.
Pick the deepest matching workspace root, not the first one.
When allowed roots overlap, returning the first starts_with match can choose a broader mount than the current repo actually lives under. For example, /work will win over /work/app if it appears first, and the auto-filled path/repo_path will then point at the wrong container root.
Suggested fix
- for (host, container) in &pairs {
- let hp = Path::new(host.trim());
- let hcanon = std::fs::canonicalize(hp).unwrap_or_else(|_| hp.to_path_buf());
- if cwd_canon.starts_with(&hcanon) {
- return Some(container.clone());
- }
- }
-
- pairs.first().map(|(_, c)| c.clone())
+ let mut best: Option<(usize, String)> = None;
+ for (host, container) in &pairs {
+ let hp = Path::new(host.trim());
+ let hcanon = std::fs::canonicalize(hp).unwrap_or_else(|_| hp.to_path_buf());
+ if cwd_canon.starts_with(&hcanon) {
+ let depth = hcanon.components().count();
+ if best.as_ref().is_none_or(|(best_depth, _)| depth > *best_depth) {
+ best = Some((depth, container.clone()));
+ }
+ }
+ }
+
+ best.map(|(_, c)| c)
+ .or_else(|| pairs.first().map(|(_, c)| c.clone()))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (host, container) in &pairs { | |
| let hp = Path::new(host.trim()); | |
| let hcanon = std::fs::canonicalize(hp).unwrap_or_else(|_| hp.to_path_buf()); | |
| if cwd_canon.starts_with(&hcanon) { | |
| return Some(container.clone()); | |
| } | |
| } | |
| pairs.first().map(|(_, c)| c.clone()) | |
| let mut best: Option<(usize, String)> = None; | |
| for (host, container) in &pairs { | |
| let hp = Path::new(host.trim()); | |
| let hcanon = std::fs::canonicalize(hp).unwrap_or_else(|_| hp.to_path_buf()); | |
| if cwd_canon.starts_with(&hcanon) { | |
| let depth = hcanon.components().count(); | |
| if best.as_ref().is_none_or(|(best_depth, _)| depth > *best_depth) { | |
| best = Some((depth, container.clone())); | |
| } | |
| } | |
| } | |
| best.map(|(_, c)| c) | |
| .or_else(|| pairs.first().map(|(_, c)| c.clone())) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src-tauri/src/modules/agent/mod.rs` around lines 481 - 489, The current loop
returns the first container whose host path is a prefix of cwd_canon, which can
choose a shallower root; change the logic to track and return the deepest
(longest) matching host path instead. Iterate over pairs (host, container),
compute hp and hcanon as now, and if cwd_canon.starts_with(&hcanon) compare
hcanon.len() (or hcanon.as_os_str().len()) with the best match seen so far;
update the best container when you find a longer match and after the loop return
that best container (falling back to pairs.first().map(|(_, c)| c.clone()) if no
match). Use the same variables (pairs, host, container, hp, hcanon, cwd_canon)
so the change is minimal.
Comment on lines
+1506
to
+1574
| const WRITE_TOKENS: &[&str] = &[ | ||
| "write", "edit", "append", "create", "delete", "remove", "patch", "update", "save", | ||
| "rename", "move", "upsert", "insert", "destroy", "mutate", "replace", | ||
| ]; | ||
| let Some(arr) = tools_json.as_array_mut() else { | ||
| return; | ||
| }; | ||
| arr.retain(|t| { | ||
| let name = t | ||
| .get("function") | ||
| .and_then(|f| f.get("name")) | ||
| .and_then(|v| v.as_str()) | ||
| .unwrap_or("") | ||
| .to_ascii_lowercase(); | ||
| if name.is_empty() { | ||
| return true; | ||
| } | ||
| !contains_write_token(&name, WRITE_TOKENS) | ||
| }); | ||
| } | ||
|
|
||
| /// True when any of `tokens` appears in `name` at a name-component boundary | ||
| /// (start, end, or surrounded by `_` / `.` / `-`). Avoids false positives like | ||
| /// `output` containing `put`. | ||
| fn contains_write_token(name: &str, tokens: &[&str]) -> bool { | ||
| let bytes = name.as_bytes(); | ||
| for tok in tokens { | ||
| let needle = tok.as_bytes(); | ||
| if needle.is_empty() || bytes.len() < needle.len() { | ||
| continue; | ||
| } | ||
| let mut i = 0; | ||
| while i + needle.len() <= bytes.len() { | ||
| if &bytes[i..i + needle.len()] == needle { | ||
| let left_ok = i == 0 || matches!(bytes[i - 1], b'_' | b'.' | b'-'); | ||
| let right = i + needle.len(); | ||
| let right_ok = right == bytes.len() || matches!(bytes[right], b'_' | b'.' | b'-'); | ||
| if left_ok && right_ok { | ||
| return true; | ||
| } | ||
| } | ||
| i += 1; | ||
| } | ||
| } | ||
| false | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| #[test] | ||
| fn plan_mode_filter_drops_write_tools() { | ||
| let mut v = json!([ | ||
| { "type": "function", "function": { "name": "fetch" } }, | ||
| { "type": "function", "function": { "name": "memory_create_entity" } }, | ||
| { "type": "function", "function": { "name": "fs_write" } }, | ||
| { "type": "function", "function": { "name": "brave_web_search" } }, | ||
| { "type": "function", "function": { "name": "te_provider.edit_file" } }, | ||
| ]); | ||
| plan_mode_filter_writes(&mut v); | ||
| let names: Vec<&str> = v | ||
| .as_array() | ||
| .unwrap() | ||
| .iter() | ||
| .map(|t| t["function"]["name"].as_str().unwrap()) | ||
| .collect(); | ||
| assert_eq!(names, vec!["fetch", "brave_web_search"]); | ||
| } |
There was a problem hiding this comment.
Plan mode still leaves git_commit in the catalog.
WRITE_TOKENS never matches git_commit, but both the prompt and the CLI text promise that write tools are stripped in plan mode. That makes the read-only guarantee incomplete for a concrete mutating tool already exposed elsewhere in the app.
Suggested fix
const WRITE_TOKENS: &[&str] = &[
"write", "edit", "append", "create", "delete", "remove", "patch", "update", "save",
- "rename", "move", "upsert", "insert", "destroy", "mutate", "replace",
+ "rename", "move", "upsert", "insert", "destroy", "mutate", "replace", "commit",
]; let mut v = json!([
{ "type": "function", "function": { "name": "fetch" } },
{ "type": "function", "function": { "name": "memory_create_entity" } },
{ "type": "function", "function": { "name": "fs_write" } },
+ { "type": "function", "function": { "name": "git_commit" } },
{ "type": "function", "function": { "name": "brave_web_search" } },
{ "type": "function", "function": { "name": "te_provider.edit_file" } },
]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src-tauri/src/modules/agent/mod.rs` around lines 1506 - 1574, The
plan_mode_filter_writes behavior misses tools like "git_commit" because
WRITE_TOKENS doesn't include "git" or "commit"; update the WRITE_TOKENS constant
(used by plan_mode_filter_writes and checked via contains_write_token) to
include tokens that cover git operations (e.g., "git" and/or "commit") or add a
special-case entry like "git_commit" so the name match logic removes that tool;
after updating WRITE_TOKENS, run/update the test
plan_mode_filter_drops_write_tools to assert "git_commit" is filtered out.
Comment on lines
+27
to
+40
| const rawRoot = path.resolve(process.argv[2] ?? path.join(__dirname, "..", "..", ".")); | ||
| let hostRoot; | ||
| try { | ||
| hostRoot = fs.realpathSync(rawRoot); | ||
| } catch { | ||
| console.error("Directory does not exist:", rawRoot); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| const inContainer = process.env.MCP_PROBE_IN_CONTAINER === "1"; | ||
| const label = path.basename(hostRoot.replace(/\/$/, "")); | ||
| const containerRoot = `/app/${label}`; | ||
| /** Paths sent in MCP `tools/call` — `/app/…` inside Docker, host path when local node server. */ | ||
| const base = inContainer ? containerRoot : hostRoot; |
There was a problem hiding this comment.
Keep probe mutations inside a per-run temp directory.
The current default target is the workspace root, and the smoke calls create/move/edit fixed _probe_* paths directly there. That both pollutes the checked-out repo and makes reruns flaky because create_directory/move_file can fail on leftovers from the previous run. Please isolate all probe paths under a unique temp subdirectory per execution, or clean them up before the calls start.
Also applies to: 95-127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/mcp-probe-filemanager/probe.mjs` around lines 27 - 40, The probe
currently uses the workspace root (base calculated from
rawRoot/hostRoot/containerRoot) as the target, which pollutes the repo and
causes flakiness; create a unique per-run temp subdirectory (e.g., build a
probeRunDir from base plus a deterministic prefix using timestamp/UUID such as
`_probe_${Date.now()}_${random}`) and use that probeRunDir for all probe
artifacts instead of base; ensure probeRunDir is created (fs.mkdirSync(..., {
recursive: true })) and that the mapping for container vs host uses the
corresponding containerRoot/hostRoot variant so calls that reference container
paths still point to `/app/<label>/...`; update all places that reference fixed
`_probe_*` paths (including the code that creates/moves/edits files later in the
file) to use this new probeRunDir and optionally remove/cleanup the dir on exit.
- Added a new module for interactive CLI flavor, providing dynamic and engaging spinner labels and messages. - Implemented functions for generating randomized phrases to enhance user experience during long sessions. - Updated CLI handlers to utilize the new flavor functions for improved status messages and tool action labels. - Enhanced error output formatting for better clarity and user feedback in the terminal.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src-tauri/src/modules/ollama/service.rs (1)
215-222: ⚡ Quick winReuse the transport-error mapper in
touch_activate_model.This path still returns raw reqwest strings, so
/model <#>can surface opaque transport failures while normal chat now shows daemon/timeout guidance. Reusingexplain_ollama_chat_transport_errorhere keeps the CLI error surface consistent.♻️ Minimal reshape
let resp = http_client() .post(OLLAMA_CHAT_URL) .json(&payload) .timeout(timeout) .send() .await - .map_err(|e| e.to_string())?; + .map_err(|e| explain_ollama_chat_transport_error(&e.to_string()))?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/modules/ollama/service.rs` around lines 215 - 222, The transport error for the POST to OLLAMA_CHAT_URL in touch_activate_model is currently mapped to a raw string via map_err(|e| e.to_string()); update that mapping to reuse the existing transport-error helper by passing the reqwest error into explain_ollama_chat_transport_error so the function returns the consistent, user-friendly error string. Locate the POST using http_client().post(OLLAMA_CHAT_URL).json(&payload).timeout(timeout).send().await and replace the map_err closure with a call to explain_ollama_chat_transport_error (keeping the rest of the call chain unchanged).
🤖 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-tauri/src/modules/cli/handlers.rs`:
- Around line 514-529: The current bot_disconnect clears in-memory state before
durable cleanup, causing a partial disconnect on failures; change bot_disconnect
so it first call bot_lifecycle::stop_and_wait_for_bot(state).await, then read
and hold the bot_id without setting state.connection to None, perform durable
cleanup (call bot_repo::clear(&state.store_path) and then
secure_store::delete_token(&id) if Some), and only after those succeed enter the
critical section to clear state.connection; if any durable step fails return
CliReply::error and leave state.connection intact (or explicitly restore the
previous connection) so the in-memory state matches persisted state. Ensure you
reference and update the same symbols: bot_lifecycle::stop_and_wait_for_bot,
state.connection lock usage, bot_repo::clear, secure_store::delete_token, and
CliReply::error.
In `@src-tauri/src/modules/cli/output.rs`:
- Around line 275-297: The REPL path in render_reply_indented is treating
ReplyKind::Diff like generic text and letting render_with_prefix prepend
continuation spaces, which strips diff markers before
TerminalSink::render()/print_diff_with_ansi sees them; update
render_reply_indented (and the similar block around the 341–363 code) to match
ReplyKind::Diff explicitly and render it with the diff renderer (call the sink's
diff-rendering routine or invoke print_diff_with_ansi) without applying the
continuation prefix (i.e., ensure you call the diff render path with
FirstPrefix::None or bypass render_with_prefix) so that leading +/-, @@, etc.
are preserved for proper highlighting.
- Around line 187-200: The non-Unix branch of is_terminal_stdout currently
returns true (cfg(not(unix))) which falsely treats redirected output as a TTY;
change is_terminal_stdout so the non-Unix path performs a real TTY check instead
of returning true: either use the atty crate (call
atty::is(atty::Stream::Stdout)) or implement a Windows-specific check
(GetConsoleMode/GetStdHandle) for stdout, replacing the cfg(not(unix)) { true }
branch; keep the existing Unix isatty code and ensure the function still returns
a bool.
---
Nitpick comments:
In `@src-tauri/src/modules/ollama/service.rs`:
- Around line 215-222: The transport error for the POST to OLLAMA_CHAT_URL in
touch_activate_model is currently mapped to a raw string via map_err(|e|
e.to_string()); update that mapping to reuse the existing transport-error helper
by passing the reqwest error into explain_ollama_chat_transport_error so the
function returns the consistent, user-friendly error string. Locate the POST
using
http_client().post(OLLAMA_CHAT_URL).json(&payload).timeout(timeout).send().await
and replace the map_err closure with a call to
explain_ollama_chat_transport_error (keeping the rest of the call chain
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: 1cfe2268-f4f3-4993-94fc-b98ae16e830e
⛔ Files ignored due to path filters (1)
tools/mcp-probe-filemanager/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
eslint.config.tssrc-tauri/src/modules/cli/flavor.rssrc-tauri/src/modules/cli/handlers.rssrc-tauri/src/modules/cli/mod.rssrc-tauri/src/modules/cli/output.rssrc-tauri/src/modules/cli/repl.rssrc-tauri/src/modules/ollama/service.rstools/mcp-probe-filemanager/jsconfig.jsontools/mcp-probe-filemanager/package.jsontools/mcp-probe-filemanager/probe.mjs
✅ Files skipped from review due to trivial changes (2)
- tools/mcp-probe-filemanager/jsconfig.json
- tools/mcp-probe-filemanager/package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- src-tauri/src/modules/cli/mod.rs
- tools/mcp-probe-filemanager/probe.mjs
Comment on lines
+514
to
+529
| pub async fn bot_disconnect(state: &AppState) -> CliReply { | ||
| bot_lifecycle::stop_and_wait_for_bot(state).await; | ||
| let bot_id = { | ||
| let mut lock = state.connection.lock().await; | ||
| let id = lock.as_ref().map(|c| c.bot_id.clone()); | ||
| *lock = None; | ||
| id | ||
| }; | ||
| if let Err(e) = bot_repo::clear(&state.store_path) { | ||
| return CliReply::error(format!("clear store: {e}")); | ||
| } | ||
| if let Some(id) = bot_id { | ||
| if let Err(e) = secure_store::delete_token(&id) { | ||
| return CliReply::error(format!("keychain delete: {e}")); | ||
| } | ||
| } |
There was a problem hiding this comment.
bot_disconnect can leave a partial disconnect state on failure.
state.connection is cleared before either durable cleanup step runs. If bot_repo::clear() fails, the live process now thinks the bot is disconnected while the persisted metadata/token are still around; if keychain deletion fails, cleanup is only half applied. Make the in-memory state transition happen after durable cleanup succeeds, or restore/report a deliberate partial state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src-tauri/src/modules/cli/handlers.rs` around lines 514 - 529, The current
bot_disconnect clears in-memory state before durable cleanup, causing a partial
disconnect on failures; change bot_disconnect so it first call
bot_lifecycle::stop_and_wait_for_bot(state).await, then read and hold the bot_id
without setting state.connection to None, perform durable cleanup (call
bot_repo::clear(&state.store_path) and then secure_store::delete_token(&id) if
Some), and only after those succeed enter the critical section to clear
state.connection; if any durable step fails return CliReply::error and leave
state.connection intact (or explicitly restore the previous connection) so the
in-memory state matches persisted state. Ensure you reference and update the
same symbols: bot_lifecycle::stop_and_wait_for_bot, state.connection lock usage,
bot_repo::clear, secure_store::delete_token, and CliReply::error.
Comment on lines
+187
to
+200
| fn is_terminal_stdout() -> bool { | ||
| // Avoids pulling a new crate; the file-descriptor check is enough for color gating. | ||
| #[cfg(unix)] | ||
| unsafe { | ||
| // SAFETY: `isatty` takes a raw fd and has no memory effects. | ||
| extern "C" { | ||
| fn isatty(fd: i32) -> i32; | ||
| } | ||
| isatty(1) == 1 | ||
| } | ||
| #[cfg(not(unix))] | ||
| { | ||
| true | ||
| } |
There was a problem hiding this comment.
Don't force TTY mode on every non-Unix platform.
The #[cfg(not(unix))] { true } branch makes redirected output look like a TTY, so Windows/plain-file output will still get ANSI escapes and REPL chrome instead of plain text.
🛠️ Simpler cross-platform check
fn is_terminal_stdout() -> bool {
- // Avoids pulling a new crate; the file-descriptor check is enough for color gating.
- #[cfg(unix)]
- unsafe {
- // SAFETY: `isatty` takes a raw fd and has no memory effects.
- extern "C" {
- fn isatty(fd: i32) -> i32;
- }
- isatty(1) == 1
- }
- #[cfg(not(unix))]
- {
- true
- }
+ std::io::stdout().is_terminal()
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src-tauri/src/modules/cli/output.rs` around lines 187 - 200, The non-Unix
branch of is_terminal_stdout currently returns true (cfg(not(unix))) which
falsely treats redirected output as a TTY; change is_terminal_stdout so the
non-Unix path performs a real TTY check instead of returning true: either use
the atty crate (call atty::is(atty::Stream::Stdout)) or implement a
Windows-specific check (GetConsoleMode/GetStdHandle) for stdout, replacing the
cfg(not(unix)) { true } branch; keep the existing Unix isatty code and ensure
the function still returns a bool.
…andling - Removed deprecated CLI commands: `doctor`, `plan`, `cost`, `resume`, and `compact` to streamline command handling. - Introduced a new prelaunch module to manage macOS application activation policy, preventing Dock icon display during CLI invocations. - Updated the CLI bootstrap process to utilize the new activation policy logic for improved user experience. - Enhanced the Cargo configuration by adding new dependencies for macOS-specific functionality.
…st modules - Streamlined function signatures and assertions for better clarity in the agent module. - Enhanced formatting of test assertions in the CLI output module for improved readability. - Consolidated line breaks and structured code for better maintainability across test cases.
- Introduced a new .pengine file to store session metadata, including session ID and project directory. - Updated the agent module to handle project context more effectively by reading from the .pengine file. - Enhanced CLI session management to incorporate project context from the .pengine file, improving user experience during interactions. - Implemented functions to read and manage .pengine file data, ensuring proper handling of project metadata.
- Introduced a new .pengine file to store session metadata, including session ID and project directory. - Updated the agent module to handle project context more effectively by reading from the .pengine file. - Enhanced CLI session management to incorporate project context from the .pengine file, improving user experience during interactions. - Implemented functions to read and manage .pengine file data, ensuring proper handling of project metadata.
…gration - Added new CLI commands: `/compact` to summarize old session turns and `/new` to start a fresh session, improving user interaction in the REPL. - Updated macOS activation policy to ensure CLI processes are completely hidden from UI surfaces, enhancing user experience during CLI invocations. - Refactored session handling to automatically compact sessions when exceeding a defined turn threshold, optimizing memory usage. - Improved project context handling by auto-resuming the most recent session for the current project, streamlining user workflow.
- Simplified the return statement in the `compact_session_background` function for improved readability. - Enhanced logging formatting for session auto-compaction errors, ensuring better clarity in error messages.
…ve output tracking - Introduced a new `/session` command for managing named sessions, allowing users to list, create, switch, rename, and delete sessions. - Implemented session auto-compaction when exceeding a defined turn threshold, optimizing memory usage. - Added support for live output token tracking during chat interactions, improving user feedback on token generation. - Enhanced session metadata handling by saving session summaries and project context, streamlining user experience during CLI interactions.
- Eliminated the `/new` command as a shortcut for starting a fresh session, streamlining session management. - Updated session help text to clarify the usage of `/session new` for creating named sessions. - Enhanced the session list output format to include token count, improving user visibility on session details.
- Expanded the .pengine file to include detailed project context, emphasizing the importance of using specific file paths and avoiding directory scans. - Added a comprehensive layout section outlining the structure of frontend and backend components. - Updated the agent module's prompt to include a new sources section for web content, enhancing clarity on how to reference fetched data. - Cleaned up the session handling code by removing unnecessary whitespace and improving formatting for better readability.
…larity and functionality - Revised the agent's system prompt to clarify instructions for code changes, emphasizing concise summaries instead of markdown diffs. - Enhanced the `ask_in_session` function to automatically attach git diffs when changes are made, improving user feedback. - Introduced a new function to list project files while skipping common build directories, optimizing project context handling. - Updated the `dot_pengine_prompt_block` function to include an optional MCP prefix for better file path management in project contexts.
…in CLI - Updated the agent's system prompt to specify that only web URLs should be listed, clarifying instructions for data referencing. - Enhanced the `ask_in_session` function to capture and display only the new changes introduced during a session, improving user feedback. - Introduced utility functions to manage and display git diffs more effectively, ensuring only relevant changes are shown to users. - Cleaned up code formatting in the CLI session file for better readability and maintainability.
- Updated the spinner output formatting to include color coding for better visibility and user experience. - Improved the project file listing functionality to group files by top-level directory, enhancing organization and clarity. - Cleaned up the code in the session module for better readability and maintainability.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
$CLI bridge, CLI shim installer UI and dashboard panel, new HTTP endpoint listing CLI commands, MCP HTTP transport and new MCP tools.Documentation
Tests
Chores