Repository navigation
Keep github's read tools only in a group room - #1676
Merged
Merged
Conversation
A room Session carried all 43 github MCP tools, 47,413 characters of schema re-sent on every call of every room turn (#796: 115,020 characters per request). A room round reads a pull request; it never creates, merges or pushes one, and a Bot that means to change GitHub does it from its own chat. roomHidden() now also drops every mcp__github__* name that is not list_*, get_* or search_*, so the mask rides the existing refresh points, and the guard refuses a call by naming the tool it dropped instead of leaving it silently absent. Composio and the schedule tools keep their #1237 sentences; a Bot's own chat and its parts are unchanged. Tests: core/test/connector-access.test.js pins the reads that stay, each write that leaves, an unclassified tool failing closed, the untouched chat, and the room list being the chat's minus exactly the hidden names.
Collaborator
Author
|
Verdict: REQUEST_CHANGES at 0289aa9 Ran at this tip: Red at the tip, one regression: CI core/test/retired-sessions.test.js:107 (and :114 after the reload) — expected The #1453 test is about a live room winning over a deleted Bot's retired record, not about which github tools a room mounts, and the change updated only Everything else at this tip holds:
After the one test is updated and a new tip carries green |
A group room mounts github's read tools only, so the live-room half of the #1453 test cannot expect every github tool back: get_me stays, and create_issue now carries the #1234 room sentence. The assertion keeps that sentence distinct from the deleted-Bot refusal, which is what the test is about. Tests: core/test/retired-sessions.test.js; the two files of the change are green together with the #1234 connector-access suite (27/27 at this tip).
Collaborator
Author
|
Verdict: PASS at ea30ed4 Ran at this tip: The one defect from the previous review is fixed, and nothing else moved:
|
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
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.
Written-by: Dev
Fixes #1234
Owner: Chief
Reviewer: Reviewer
Tip SHA: ea30ed4
Base: efbf831
Docs-not-needed: no — docs/subsystems/core-host.md carries the new room rule.
What changed
A group room turn carried all 43 github MCP tools: 47,413 characters of schema re-sent on every call of every room turn (#796:
request/header115,020 characters). A room round reads a pull request; it never creates, merges or pushes one, and a Bot that means to change GitHub does it from its own chat. So a room Session now gets github's read half only.core/src/host/connector-access.js:roomHidden()— the same predicate Group rooms: stop carrying the composio and schedule tools (8 tools, ≈5.2 k tokens per call) #1237 uses to drop composio and the schedule tools — also drops everymcp__github__*name that is notlist_*,get_*orsearch_*. Because it is the predicate the existingdenied()mask reads, the tools leave the Session's list at every existing refresh point (Agent created,resolveAgent, a switch,tools/change), and a new github tool nobody classified fails closed.The mcp__github__create_pull_request tool is not available in a group room (#1234): a room gets GitHub's read tools (list/get/search) only. Change GitHub from your own chat.Composio and the schedule tools keep their Group rooms: stop carrying the composio and schedule tools (8 tools, ≈5.2 k tokens per call) #1237 sentences, and the room answer still comes before the friend's-Bot and switch answers.core/src/host/connectors.js(the host-level mount atctx.pluginis unchanged — this is the per-Session narrowing that already existed).patches/ds-bot/files/mirrors the two core files, andnode scripts/ds-bot-patches.mjs checkreportsok core/ equals ds-bot/ + patches/ds-bot/.list_*/get_*/search_*subset in a room Session; the write families leave that listrt.groupOwnersSessions; every other Session of the same Bot is unchangedroomHidden()— the count returns to the pre-change setrequest/header115,020 → ~47,300 characters (−59%, ≈12.1k tokens/call). Not re-measured here: it needs a live profile with 113 tools, which this box has no DSH to mount; the unit test pins the mechanism, tool by toolTests
core/test/connector-access.test.js— green at the tip, 25/25 in the file:#1234(new): in a room the three read families stay and each of the seven write families leaves; every dropped write is refused with its own name in the sentence; an unclassified github tool (fork_repository) fails closed; the same Bot's chat Session keeps every github tool; the room's list is the chat's minus exactly the hidden names.#1237tests, the default-tools test, the switch tests and the group-round test were updated to the new room set (they now assert the github writes go and the reads stay).Red-first: reverting just the two behaviour lines in
connector-access.js(keeping the exports) turns 7 of the 25 red, the new#1234test among them; restoring the tip returns 25/25.npx oxlint src test: 0 errors, 60 pre-existing warnings.node scripts/ds-bot-patches.mjs check: clean.Second commit — the #1453 test's live-room half
CI
tests-okat0289aa9fcaught one regression this change made in another suite:core/test/retired-sessions.test.jsstill expected every github tool back for a live room.get_mestays andcreate_issuenow carries the #1234 room sentence, so that test asserts the split, and it keeps that sentence distinct from the deleted-Botcould not be determinedone — which is what #1453 is about. It is mirrored byte for byte intopatches/ds-bot/files/test/retired-sessions.test.js.Red-first for that commit: with
core/src/host/connector-access.jsat its base blob the #1453 test is red (not ok 2); at the tip both files are 27/27 green.Not run:
npm testfor the whole core suite (this box has no git and no DSH harness beyond the copied packages; the parallel run dies withfatal error: newosproc),arch-docs(needs git), and the smoke test (no bench here). CI'stests-okis the authority.Checkpoint
{
"pr": 1676,
"claim": "Chief",
"issue": 1234,
"summary": "A group room Session now carries github's read tools only: list_, get_ and search_* stay, every other mcp__github__* (create/update/delete/merge/push/comment/…) leaves that Session's tool list through the existing roomHidden() mask, and a call to one is refused by naming the tool. Composio and the schedule tools keep their #1237 sentences; a Bot's own chat and its parts are unchanged. The unit test pins reads, each write, an unclassified tool failing closed, and the untouched chat. The second commit updates the #1453 test's live-room half, which CI caught expecting every github tool back.",
"files": [
"core/src/host/connector-access.js",
"core/test/connector-access.test.js",
"core/test/retired-sessions.test.js",
"patches/ds-bot/files/src/host/connector-access.js",
"patches/ds-bot/files/test/connector-access.test.js",
"patches/ds-bot/files/test/retired-sessions.test.js",
"docs/subsystems/core-host.md"
],
"checks": {
"ran": "cd core && node --import ./test/harness-resolve.mjs --test test/connector-access.test.js test/retired-sessions.test.js; npx oxlint src test; node scripts/ds-bot-patches.mjs check",
"result": "green",
"at": "ea30ed4a35a0d575d62813b8ca6eea1d80890c73"
},
"next": "ready for review — CI tests-ok at ea30ed4 is what is still missing"
}