Skip to content

Add skill CLI install mode - #1215

Closed
alecuba16 wants to merge 5 commits into
DeusData:mainfrom
alecuba16:skill-cli-mode
Closed

Add skill CLI install mode#1215
alecuba16 wants to merge 5 commits into
DeusData:mainfrom
alecuba16:skill-cli-mode

Conversation

@alecuba16

Copy link
Copy Markdown

Summary

This PR adds an install mode for the Codebase Memory skill that can use the CLI instead of installing the MCP config.

The idea is simple: sometimes we do not want the LLM session to always load the MCP tool definitions. Those tool definitions use context space even when the user does not need code graph tools. With this skill mode, the LLM keeps only a small skill instruction. When it needs the graph, it runs codebase-memory-mcp cli <tool> '<json>' from the shell.

So the tool is still available when needed, but it does not need to stay as MCP tools in the active context all the time.

What changed

  • Added --skill-mode=cli|mcp for install.
  • --skill-mode=mcp keeps current default behavior.
  • --skill-mode=cli installs the Claude Code skill and skips MCP configs/hooks.
  • Added optional MCP idle timeout support for wrappers.
  • Updated docs and help text with the skill install mode and skill paths.
  • Synced the generated skill guidance with current graph features like check_index_coverage.

Notes

This does not include local memory / personal memory functionality. That belongs to another branch.

Validation

  • scripts/build.sh
  • scripts/test.sh tests/test_cli.c tests/test_mcp.c
  • Isolated install smoke for --skill-mode=cli

@alecuba16
alecuba16 requested a review from DeusData as a code owner July 22, 2026 17:34
@DeusData DeusData added enhancement New feature or request editor/integration Editor compatibility and CLI integration ux/behavior Display bugs, docs, adoption UX security Security vulnerabilities, hardening priority/backlog Valuable contribution, lower scheduling urgency; review when maintainer capacity opens. labels Jul 24, 2026
@DeusData DeusData added this to the 0.9.2-rc milestone Jul 24, 2026
@DeusData

Copy link
Copy Markdown
Owner

Thanks for the submission. This head is blocked as submitted for concrete security and scope reasons. The installed skill includes a mutable main/install.sh network-to-shell command, which is why security-static fails; that instruction must be removed. All five commits also lack valid DCO sign-offs, lint fails, and the CLI skill mode is bundled with the unrelated CBM_IDLE_TIMEOUT_S / --idle-timeout server API. Please split idle timeout into a separate proposal, rebase the installer work onto current main, replace raw-JSON CLI guidance with the current flag/stdin interface, and add behavioral tests proving MCP config and hooks are skipped only in the selected mode. The product decision on adding CLI skill mode remains pending; no need to rework the branch until that direction is confirmed.

@DeusData

Copy link
Copy Markdown
Owner

Thanks for this, and sorry for the slow first response. Queued for review.

Two mechanical blockers before I read it: the branch is CONFLICTING against main, and CI currently shows 4 failing checks. A rebase may clear some of the failures if they stem from drift rather than the change itself — worth doing first and seeing what remains.

Flagging one thing early so it does not surprise you at review time: an install-mode addition touches the CLI public surface, so beyond correctness there is a scope question about whether it belongs. That is a maintainer call rather than a reflection on the code.

@DeusData

Copy link
Copy Markdown
Owner

Reviewed alongside the other agent-install PRs in the queue. This one is not superseded — but it needs a maintainer decision and a rework, and there is one thing in the shipped skill content I would ask you to change regardless.

First, the thing I would change whatever else happens. The new CLI-mode skill text embeds this:

curl -fsSL https://raw.githubusercontent.com/DeusData/codebase-memory-mcp/main/install.sh | bash -s -- --skip-config

It is our own repository and you gated it behind "ask the user before installing", which shows the right instinct. But it is still a remote-code-execution recipe sitting inside an instruction file that an agent reads and follows, and it is pinned to main — so whatever main serves at the moment of execution is what runs, unversioned. Our own scripts/security-install.sh URL audit would flag it, since raw.githubusercontent.com/DeusData is not on its allow pattern. Pointing at the documented install command, or dropping the recipe and letting the user run it themselves, removes the whole category.

The rest of the instruction content is fine — assertive but product-consistent, and I noted the protective rules you included ("ask before delete_project, uninstall, global config mutation, or ADR writes"). That is the right reflex.

Why it needs a decision rather than a merge:

  1. It creates a second, permanently divergent truth for the one shipped skill. g_cli_skill_mode mutates skills[0].content as a global, so which text lands depends on the last mode selected. That collides directly with the other two PRs in this queue, which install that same skill per-client — merged together, they would ship whichever variant happened to be selected. Dual skill content is a maintenance commitment, not just a flag.
  2. The premise sits in tension with our stated priority. CLI mode exists to avoid always-loaded MCP tools, i.e. to save context. Our doctrine is findability over token thrift — so "do not use MCP tools unless the user explicitly asks" is a product-direction statement, not an implementation detail. It might well be the right call; it is just not mine to make.

Also architectural: cli mode short-circuits cbm_install_agent_configs with an early return, bypassing the per-client framework rather than extending it — and a3903ca has since rebuilt that framework underneath you.

On the idle-timeout half: it is a clean additive API and the parse is properly bounds-checked, but 0baddf7 (the daemon rework) rewrote the run loop it patches — cbm_mcp_server_run moved, and one-shot cli tool calls now run in an isolated local server. So that leg needs re-targeting, and I could not assess its runtime behaviour against today's loop without a rebase. Switching modes also leaves the previous mode's artifacts in place, since MCP configs are skipped rather than removed — worth deciding whether that is intended.

I have put both questions to the maintainer. Please hold off reworking until there is an answer — between the framework rebuild and the daemon rework, this is a substantial rebase and it would be unfair to have you do it against a design that might change.

One incidental thank-you: your drive-by adding check_index_coverage to the help list was a real bug, and it has since been fixed independently on main.

@DeusData DeusData mentioned this pull request Aug 28, 2026
4 tasks
@DeusData

Copy link
Copy Markdown
Owner

The product answer I have owed you since 24 July: yes to the concept — a CLI-invocation skill mode is wanted, as an install flag — but it will be realised inside the client-surface architecture being built in #1188, not as this branch's implementation. You were asked to hold off reworking until there was an answer; you did exactly that, including not pushing a single commit after the block. That discipline is noted and appreciated — and it is why this close comes with the concept accepted rather than the PR churned.

What "inside #1188's architecture" means concretely: the marketplace/client-surface direction was decided today (see the comment there). The skill-content selection in its slice 1 is being designed with a per-client variant seam, and the CLI-instruction variant becomes an install flag choosing that variant per client — rather than this branch's process-wide skills[0].content swap, which creates a second permanently divergent truth for the one shipped skill and collides with per-client installs.

The mechanical blockers from 24/31 July stand for any future incarnation, so they are on the record in one place: the curl | bash recipe goes (unpinned network-to-shell inside an agent-followed instruction file — our own security-install.sh audit flags it); DCO sign-offs on every commit; and the idle-timeout feature splits out entirely. On that last one, a finding you deserve to know before anyone salvages the code: the Unix implementation exits ~1 second after idle regardless of the configured value — poll_for_input_unix ticks at a fixed 1000 ms and the loop breaks on the first zero-tick, so --idle-timeout=3600 behaves as =1, and the test cannot tell the difference (it passes for the wrong reason at every N). The daemon rework has since rebuilt that loop anyway, so a fresh implementation against the current architecture is the only viable path; if you want to carry that as its own PR, it is a reasonable standalone concept.

I am parking this PR closed in favour of the #1188 track. When the CLI variant ships you will be credited — the concept, the ask-first protective gates in your skill text, and the install-flag shape all trace to this contribution. Thank you for it, and for how you handled the wait.

@DeusData DeusData closed this Aug 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

editor/integration Editor compatibility and CLI integration enhancement New feature or request priority/backlog Valuable contribution, lower scheduling urgency; review when maintainer capacity opens. security Security vulnerabilities, hardening ux/behavior Display bugs, docs, adoption UX

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants