Repository navigation
Conversation
|
Thanks for opening this — it has been seen, and it is queued. This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence. Current review status: working through a backlog. What that means for this PR, concretely:
Things that will genuinely speed it up whenever review does happen:
If this fixes a bug, a reproduction we can run is worth more than a description of the symptom. Thanks for contributing, and sorry in advance for the wait. |
DeusData
left a comment
There was a problem hiding this comment.
Thank you, @davidwed! This is a clean, well-tested addition:
- The paths match agentty's own docs (
~/.agentty/mcp.jsonat user scope,~/.agentty/skills/). - Uninstall removes only what we own.
- Every headline count and the contract test stay in lockstep.
We'd like agentty in, with two changes before merge:
-
Please register agentty in the table-driven client registry (
src/cli/agent_clients.c) instead of the legacy detector.- Every client added since Grok Build lives there: Qoder, Kimi Code, GitLab Duo, Rovo Dev, Amp, Devin, Tabnine, CodeBuddy, IBM Bob Shell, Pochi, Pi and Oh My Pi. Each entry carries its stability class, capability bits, detection command and install/remove callbacks.
- The legacy path is
cbm_detected_agents_t,CLI_CLIENTS, the plan-names table and the hand-written install/uninstall blocks. We are no longer extending it. - Moving agentty to the registry also gives you detection output,
--clients,--planand uninstall from that one entry. - On the current legacy path,
print_detected_agentsdoesn't list agentty. A machine with only agentty would print "Detected agents: (none)" and then install anyway.
-
Honor
$AGENTTY_MCP_CONFIG.- agentty's plugin docs say this variable points at an explicit file that overrides both the user and the project
mcp.json. - When it is set, agentty never reads the entry in
~/.agentty/mcp.json, yet install still reports success. - Writing to the override file when the variable is set, as we already do for
CLAUDE_CONFIG_DIR,GROK_HOMEandVIBE_HOME, closes that gap.
- agentty's plugin docs say this variable points at an explicit file that overrides both the user and the project
One small thing: the suite comment in tests/test_cli.c changes "6 tests" to "7 tests", but the group already has more than that, so it's fine to drop the count.
Thanks again for picking this up and for modelling it so carefully on the existing clients!
…TTY_MCP_CONFIG Address the two pre-merge review requests on PR DeusData#2483: 1. Move agentty from the legacy detector to the table-driven client registry (src/cli/agent_clients.c), the home of every client added since Grok Build. The entry carries its stability class, capability bits (MCP | SKILL), detection command, and install/remove callbacks, which gives detection output, --clients selection, --plan receipts, and uninstall from that one row. The legacy path — cbm_detected_agents_t.agentty, the CLI_CLIENTS token, the plan-names row, and the hand-written install/uninstall blocks — is removed, so print_detected_agents no longer shows a machine with only agentty as "(none)" while installing anyway. 2. Honor $AGENTTY_MCP_CONFIG in both directions. When the variable is set, install and uninstall target that explicit file (with leading-~ expansion against HOME, matching CLAUDE_CONFIG_DIR/GROK_HOME/ VIBE_HOME) and leave ~/.agentty/mcp.json untouched, closing the gap where install reported success while agentty read only its override. Also drop the stale "7 tests" count from the suite comment and cover the registry entry, the env override, and the override-aware install/uninstall roundtrip in the CLI and registry suites. Validated: build clean; focused cli + agent_clients + agent_profiles runs at 388 passed / 20 failed, the identical pre-change baseline (install/uninstall staging under the local ASan runner; green on CI). The seven touched agentty/registry tests pass in isolation.
|
Thanks again for the agentty integration work. Following up on the review: the move to the table-driven client registry and support for AGENTTY_MCP_CONFIG are still the requested next steps in the thread. Please let us know if either request needs clarification; no additional scope is being requested here. |
|
Thank you, @davidwed, and apologies: you addressed both review points on Oct 8, and our follow-up on the 19th wrongly listed them as still open. The move into the table-driven registry is exactly what we asked for, The branch had fallen behind main, so we used maintainer edits to push two small commits on top of yours instead of sending you another round:
All of your own commits are unchanged. Thanks again for modelling this so carefully on the existing clients. |
|
Thank you again for the agentty integration. The October 20 correction stands: you had already addressed the registry and override requests, and the subsequent maintainer adjustments are recorded above. No repeat work is requested. Sorry for the delay in our remaining follow-up—I’ll follow up soon. |
|
Thank you, @davidwed, for bringing the branch up to date with main on the 23rd. Keeping it current like that is a real help to us. Main had reworded several of the docs that carry the client counts, so the web merge kept main's text in those spots, and with it the old numbers. The contract test you extended caught it right away, which is exactly why that test is worth having. We used maintainer edits once more to put it right:
Your code and tests are unchanged, and nothing more is needed from you. If you push to the branch again, please pull first so these commits stay in. Thanks again for such a careful, well-tested integration. |
|
Thank you for your patience, @davidwed. CI went green on this branch, but this evening's merges on main touched files this pull request also changes, so that green no longer covered today's main. We merged current main into the branch once more (one merge commit, 5af18b9; your changes are untouched) and will merge once the fresh checks are green. Please pull before pushing again. |
|
Thank you again for the agentty install support, @davidwed. This pull request was closed a moment ago, together with a reset of your fork's main branch to our main. Resetting a fork's main (for example "Sync fork" with "Discard commits") does exactly that when a pull request comes from that branch, and our merges of main into your branch made it look out of date - sorry for the confusion that caused. If closing was intentional, that is completely fine, and thank you for the work. If it was not: nothing is lost. The change was approved and was only waiting for its last CI legs. Its last state is commit 5af18b9, which GitHub still serves from this pull request, so you can restore it on a separate branch and open a new pull request from there: Opening it from a branch other than main avoids the same thing happening again. We will merge it as soon as CI is green. Thanks a lot! |
What does this PR do?
Add automatic detection, skill installation, MCP registration, uninstall, and --clients selection for the agentty coding agent (https://github.com/1ay1/agentty), following the established detected-agent pattern (OpenHands template):
Tests: detection, --clients vocabulary + leak guard, install plan and schema checks, and an install/uninstall roundtrip asserting foreign-server and user-skill preservation in both directions.
Checklist
git commit -s) — required, CI rejectsunsigned commits (DCO, see CONTRIBUTING.md)
make -f Makefile.cbm test)make -f Makefile.cbm lint-ci)