Skip to content

feat: Add semble install#176

Open
Pringled wants to merge 25 commits into
mainfrom
add-semble-install
Open

feat: Add semble install#176
Pringled wants to merge 25 commits into
mainfrom
add-semble-install

Conversation

@Pringled
Copy link
Copy Markdown
Member

@Pringled Pringled commented Jun 2, 2026

This PR adds a semble install command. It detects installed coding agents and configures semble globally across three integration types (MCP, instructions, sub-agent). We use tree-sitter for json/jsonc parsing so comments in MCP/settings jsons are preserved.

Here's what the flow looks like in the terminal on runningsemble install (semble uninstall gives the same flow but the reverse (uninstalling whatever is selected)):

Screenshot 2026-06-03 at 07 45 16 Screenshot 2026-06-03 at 07 45 20 Screenshot 2026-06-03 at 07 44 40 Screenshot 2026-06-03 at 07 45 03

@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 2, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines Coverage Δ
src/semble/cli.py 100.00% <100.00%> (ø)
src/semble/installer/__init__.py 100.00% <100.00%> (ø)
src/semble/installer/agents.py 100.00% <100.00%> (ø)
src/semble/installer/config.py 100.00% <100.00%> (ø)
src/semble/installer/installer.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Pringled
Copy link
Copy Markdown
Member Author

Pringled commented Jun 2, 2026

@greptileai review

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Jun 2, 2026

Confidence Score: 4/5

Safe to merge with one follow-up: the uninstall code path needs an end-to-end test.

The installer logic itself is sound and individual helpers are well-tested. The gap is that run("uninstall") is never called end-to-end — the full uninstall flow (remove operations, "not-found" returns through _apply, different UI text, different footer) can silently regress without the test suite catching it. The reviewer specifically asked for e2e coverage of all codepaths, so this missing test is the main concern.

tests/test_installer.py — needs a run("uninstall") end-to-end test to match the run("install") test that already exists.

Reviews (3): Last reviewed commit: "Fix toml edgecase" | Re-trigger Greptile

Comment thread src/semble/installer.py Outdated
Comment thread src/semble/installer.py Outdated
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Jun 2, 2026

Confidence Score: 3/5

The install path works correctly for fresh configs; the uninstall path can corrupt pre-existing configs where semble was not the first entry.

The _delete_member function only removes an adjacent comma correctly when semble was inserted as the first member. If semble was added last by another tool (e.g. claude mcp add) and the user then runs semble uninstall, the preceding entry's trailing comma is left in place, making strict-JSON files like .claude.json invalid. The JSON5-based validation guard does not catch it.

src/semble/installer.py — specifically the _delete_member function and the missing test case in tests/test_installer.py.

Reviews (2): Last reviewed commit: "Group types/vars" | Re-trigger Greptile

Comment thread src/semble/installer.py Outdated
Comment thread src/semble/installer.py Outdated
Comment thread src/semble/installer.py Outdated
@Pringled
Copy link
Copy Markdown
Member Author

Pringled commented Jun 3, 2026

@greptileai review this branch again main. Focus on two things:

  • e2e testing, ensuring that all the codepaths and combinations resolve correctly or have a clear error message when they fail
  • architecture, ensuring that the architecture for the installer is maintainable, easy to understand, and not over-engineered

@Pringled Pringled requested a review from stephantul June 3, 2026 05:58
Copy link
Copy Markdown
Contributor

@stephantul stephantul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small docs comments, but looks good!

Comment thread docs/installation.md Outdated
```bash
uv tool install semble
semble install # configure your agents
semble uninstall # undo
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit weird, because the instruction seems to imply that you need to uninstall it immediately. Maybe make it a separate block?

i.e., if I copy paste this block, I'll just install and uninstall.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah agree, fixed

Comment thread src/semble/cli.py Outdated
"""Return the project-relative path where the semble sub-agent file should be written."""
base_dir = ".github" if agent is Agent.COPILOT else f".{agent.value}"
return Path(base_dir) / "agents" / "semble-search.md"
# Copilot requires the .agent.md extension; the others use a plain .md file.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do the others not accept .agent.md? As in: copilot only reads .agent.md files, but maybe the others do as well. If so, could be useful to just keep it like this. But maybe a bit brittle

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yanked semble init as discussed so this is also resolved

Comment thread pyproject.toml Outdated
"tree-sitter-language-pack>=1.0,<1.8.0,!=1.6.3",
"orjson"
"orjson",
"questionary>=2.0",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe an upper bound on the pin as well? (i.e., <3.0)

Comment thread src/semble/installer.py Outdated
from tree_sitter import Node, Parser
from tree_sitter_language_pack import SupportedLanguage, download, get_parser

_HOME = Path.home()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just out of caution: if this function does not work for some reason, this script will fail on import. I'm not sure if it ever will, just something to keep in mind.

To avoid this, you can create a function and potentially cache the result.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup better safe than sorry, fixed

Comment thread src/semble/installer.py Outdated
"json5" ships in tree-sitter-language-pack but isn't in its typed language list, hence the cast.
"""
try:
download(["json5"])
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can potentially ship this grammar with the package. It's a useful exception. I don't know how large it is, but I suspect it's pretty small.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a followup, it's a bit of work to do well

Comment thread src/semble/installer.py Outdated
]


def _json5_parser() -> Parser | None:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could be cached: if the download fails or times out, it will fail or time out every time.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment thread src/semble/installer.py Outdated
return None


def _json5_object(text: str) -> JsonObjectResult:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a big fan of these kinds of returns: they put a lot of work in the caller. Instead, you could consider returning a tuple containing

(result, success)

if success is True, you can safely read result, if it is False, the result contains the reason for the error.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I tried it but could not really come up with a nice way to do it, everything I tried ended up with #type ignores since the success flag does not narrow the result type. I tried a small additional dataclass but tbh that adds more complexity then it removes. But if you have a good way of doing it lmk

Comment thread src/semble/installer.py Outdated
class McpConfig:
"""MCP integration config for one agent."""

path: Path | PathResolver
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell, there is no need for this disjunction. The paths can resolve when creating the McpConfig itself, e.g.

instead of doing:

McpConfig(_vscode_mcp_path)

you can do:

McpConfig(_vscode_mcp_path())

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is that this can't resolve at definition time since VS Code and OpenCode paths are dynamic (OS-dependent), so I think we do need the callable right?

Comment thread src/semble/installer.py Outdated
SEMBLE_START = "<!-- SEMBLE_START -->"
SEMBLE_END = "<!-- SEMBLE_END -->"

_STDIO_ENTRY: dict[str, object] = {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the use of _ENTRY is slightly ambiguous here. I first read it as "an entry" (i.e., a record), but I think you mean "entrypoint"? If so, maybe rename it to read entrypoint.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah agree, renamed it

Comment thread src/semble/installer.py Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing I think would really improve this code is moving the JSON5 and TOML parsing to a separate file or separate files. It would lead to a lot more readability. Likewise, the definitions can move to a separate const file? That way you separate the entry points for the TOML and JSON5 parsing from the rest of the code, and also separate the constants from operational code. I think just making a submodule called installer and adding those under this could be fine.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yuuuuup I like this! Refactored it, lmk what you think

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants