feat: Add semble install#176
Conversation
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
|
@greptileai review |
Confidence Score: 4/5Safe to merge; the installer reads and modifies only well-scoped agent config files, the JSON/JSONC path uses a verified reparse guard before writing, and the interactive confirmation step protects against accidental changes. The core file-manipulation logic is well-tested and the reparse guard prevents writing malformed JSON. Two small rough edges: the apply output treats already-configured integrations identically to unsupported ones (dim dash), which could confuse users re-running the installer, and src/semble/installer.py — specifically the Reviews (1): Last reviewed commit: "Group types/vars" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| result = integ.apply(agent, mode) | ||
| if result is None: | ||
| print(f" {_DIM}– {integ.id}: not supported{_RESET}") | ||
| continue | ||
| ok = result.action in ("created", "updated", "removed") | ||
| detail = _ACTION_DETAIL.get(result.action, "") | ||
| suffix = f" — {detail}" if detail else "" | ||
| print(f" {_tick(ok)} {integ.id} ({result.action}){suffix} → {result.path}") |
There was a problem hiding this comment.
When an integration is already correctly configured, result.action is "unchanged" and ok evaluates to False, so _tick(ok) returns the same dim – that is used for "not supported" integrations. A user running semble install on an already-configured machine would see a screen of dim dashes and might reasonably conclude the installation failed when every integration was already in place. Including "unchanged" in the ok set — or using a distinct tick symbol — would make the output less ambiguous.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| existed = dest.exists() | ||
| dest.parent.mkdir(parents=True, exist_ok=True) | ||
| dest.write_text(files("semble").joinpath(f"agents/{agent.id}.md").read_text(encoding="utf-8"), encoding="utf-8") | ||
| return WriteResult(dest, "updated" if existed else "created") |
There was a problem hiding this comment.
_apply_subagent propagates exceptions instead of returning an error action
Every other integration helper (_apply_mcp, _apply_instructions) surfaces failures through the Action return value (e.g. "error", "skipped"), keeping _apply in control and printing a friendly message. _apply_subagent, however, has no try/except around the package-data file read. If the template file is absent from the installed package (e.g. a new agent ID was added to AGENTS without a matching .md resource), a raw FileNotFoundError propagates all the way through _apply and crashes semble install with a traceback. Wrapping the write in a try/except and returning WriteResult(dest, "error") keeps the error-reporting consistent with the rest of the function.
| existed = dest.exists() | |
| dest.parent.mkdir(parents=True, exist_ok=True) | |
| dest.write_text(files("semble").joinpath(f"agents/{agent.id}.md").read_text(encoding="utf-8"), encoding="utf-8") | |
| return WriteResult(dest, "updated" if existed else "created") | |
| existed = dest.exists() | |
| dest.parent.mkdir(parents=True, exist_ok=True) | |
| try: | |
| dest.write_text(files("semble").joinpath(f"agents/{agent.id}.md").read_text(encoding="utf-8"), encoding="utf-8") | |
| except Exception: | |
| return WriteResult(dest, "error") | |
| return WriteResult(dest, "updated" if existed else "created") |
Confidence Score: 3/5The install path works correctly for fresh configs; the uninstall path can corrupt pre-existing configs where semble was not the first entry. The src/semble/installer.py — specifically the Reviews (2): Last reviewed commit: "Group types/vars" | Re-trigger Greptile |
| def _delete_member(src: bytes, member: Node) -> bytes: | ||
| """Remove `member` plus one adjacent comma and its leading line indentation.""" | ||
| start, end = member.start_byte, member.end_byte | ||
| after = end | ||
| while after < len(src) and src[after : after + 1] in (b" ", b"\t"): | ||
| after += 1 | ||
| if after < len(src) and src[after : after + 1] == b",": # prefer a trailing comma | ||
| end = after + 1 | ||
| else: | ||
| before = start | ||
| while before > 0 and src[before - 1 : before] in (b" ", b"\t"): | ||
| before -= 1 | ||
| if before > 0 and src[before - 1 : before] == b",": | ||
| start = before - 1 | ||
| while start > 0 and src[start - 1 : start] in (b" ", b"\t"): | ||
| start -= 1 | ||
| if start > 0 and src[start - 1 : start] == b"\n": | ||
| start -= 1 # drop the now-empty line | ||
| return src[:start] + src[end:] |
There was a problem hiding this comment.
_delete_member has two strategies for consuming the comma adjacent to a removed member: (1) look for a trailing comma directly after the member, or (2) walk backwards through leading whitespace and check the one byte that precedes it. Strategy (2) stops at the \n that ends the previous line, so when the layout is "other": {...},\n "semble": {...}, the check at src[before - 1 : before] sees \n rather than ,. The comma on the previous line is never consumed.
Concretely: if a user previously added semble via claude mcp add semble -s user -- … (which appends at the end of mcpServers) and then runs semble uninstall, the resulting .claude.json will contain a trailing comma after the predecessor entry—making the file invalid strict JSON. The _reparse_ok guard does not catch this because the JSON5 parser accepts trailing commas.
The _insert_first_member insertion strategy means the install/uninstall round-trip is safe only when semble is the first entry. Any pre-existing semble entry added by another tool stays in its original position (last) after merge_json_member updates just the value, and the uninstall then hits this path.
| def _strip_toml_section(text: str, header: str) -> str: | ||
| """Drop the TOML table beginning at `header` (a [table] line) up to the next table or EOF.""" | ||
| result, skipping = [], False | ||
| for line in text.splitlines(keepends=True): | ||
| stripped = line.strip() | ||
| if stripped == header: | ||
| skipping = True | ||
| continue | ||
| if skipping and not stripped.startswith("["): | ||
| continue | ||
| skipping = False | ||
| result.append(line) | ||
| return "".join(result) |
There was a problem hiding this comment.
When _strip_toml_section encounters a line that does not start with [ while skipping = True, it drops it unconditionally. Any blank lines or comments sitting between the semble block and the next [table] header are removed along with the semble content. For most users Codex's config won't have such comments, but silently removing user-written content on uninstall is unexpected.
| def _json5_parser() -> Parser | None: | ||
| """Return a tree-sitter JSON5 parser, downloading the grammar if needed. | ||
|
|
||
| "json5" ships in tree-sitter-language-pack but isn't in its typed language list, hence the cast. | ||
| """ | ||
| try: | ||
| download(["json5"]) | ||
| return get_parser(cast(SupportedLanguage, "json5")) | ||
| except Exception: | ||
| return None |
There was a problem hiding this comment.
_json5_parser() calls download(["json5"]) on every invocation
_json5_parser is called twice per JSON write operation—once from _json5_object and once from _reparse_ok. Each call invokes download(["json5"]), which performs a filesystem or network check on every call. Caching the Parser instance at module level would avoid redundant work, especially for agents like Claude Code or Gemini that each trigger multiple parse-and-reparse cycles during a single semble install run.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
This PR adds a
semble installcommand.