fix(host): a tool list that cannot be narrowed keeps its previous mask (#1271)

changelog.d/1271-connector-mask-fail-closed.fixed.md

@@ -0,0 +1 @@
+A Bot whose tool list has to be rebuilt for a group room no longer ends up with every tool listed when one name in the mask cannot be restricted: the host keeps the narrowing it can enforce and warns, instead of dropping the whole mask after lifting it ([#1271](https://github.com/ThinkFlowLab/FlyBot/issues/1271)).

core/src/host/connector-access.js

@@ -151,9 +151,32 @@ export function install(rt) {
       // After the old mask is lifted, schemas() lists what is really served now.
       const fresh = served(agent)
       const deny = fresh === undefined ? [] : denied(who, fresh)
-      masks.set(agent, { key: deny.join(','), deny, dispose: deny.length > 0 ? agent.ctx.tools.restrict({ deny }) : undefined })
+      // #1271: restrict() refuses a name the profile does not serve as a global tool, and a tool the Agent
+      // itself serves is listed by schemas() all the same. One such name made the whole call throw after
+      // the old mask was already lifted, so the Agent quietly got every tool back. Ask the global view
+      // what a restriction can enforce and name only those: the guard still refuses a call on the rest.
+      const global = fresh === undefined ? undefined : new Set((ctx.tools.schemas?.() ?? []).map(schema => schema.name))
+      const enforceable = global === undefined ? deny : deny.filter(name => global.has(name))
+      // #1271: what the global view cannot name stays in the Agent's list, and the guard is the only thing
+      // left refusing it. Say which names those are — a silent narrow list is what the docs promised was
+      // not happening.
+      const listed = global === undefined ? [] : deny.filter(name => !global.has(name))
+      if (listed.length > 0) rt.warn('ds-bot: connector tools stay listed for %s: %s cannot be restricted by name', bot?.name ?? agent?.id, listed.join(', '))
+      masks.set(agent, { key: deny.join(','), deny: enforceable, dispose: enforceable.length > 0 ? agent.ctx.tools.restrict({ deny: enforceable }) : undefined })
     } catch (error) {
-      rt.warn('ds-bot: connector tools stay listed for %s: %s', bot?.name ?? agent?.id, error)
+      // #1271: a backstop, and unreachable by construction on this API — restrict() validates against the
+      // inherited layers, and that is exactly what schemas() without an Agent returns, so a name the filter
+      // above just allowed cannot be refused as unknown. No test reaches this branch today. It stays for a
+      // DSH that refuses for a reason of its own: by then the old mask is lifted, and putting the previous
+      // narrowing back beats leaving the list wide, because a Bot reads its own list to decide what it can do.
+      if (current !== undefined && current.deny.length > 0) {
+        try {
+          masks.set(agent, { ...current, dispose: agent.ctx.tools.restrict({ deny: current.deny }) })
+        } catch {
+          masks.delete(agent)
+        }
+      }
+      rt.warn('ds-bot: connector tools stay as they were for %s: %s', bot?.name ?? agent?.id, error)
     }
   }
 

core/test/connector-access.test.js

@@ -39,10 +39,15 @@ async function setup({ state = team() } = {}) {
   const home = mkdtempSync(join(tmpdir(), 'ds-bot-connector-access-'))
   writeFileSync(join(home, 'state.json'), JSON.stringify(state))
   const ctx = fakeContext()
+  // #1271: rt.warn is the logger; keep its lines so a test can read what the host said it could not do.
+  const warns = []
+  ctx.logger.warn = (...args) => warns.push(args)
   const agents = new Map()
   const sent = []
   const masks = new Map()
-  const live = { served: [...OWN, ...COMPOSIO, ...GITHUB, ...SCHEDULE] }
+  // `own` is what the Agent itself serves (its own layer): schemas(agent) lists it, schemas() and
+  // restrict() do not know it (#1271).
+  const live = { served: [...OWN, ...COMPOSIO, ...GITHUB, ...SCHEDULE], own: [] }
   const created = agent => Promise.all((ctx.listeners.get('agent/created') ?? []).map(listener => listener({ agent })))
   const hiddenFrom = id => new Set([...(masks.get(id) ?? [])].flatMap(mask => [...mask]).filter(name => connectorOfTool(name) !== undefined))
   const agentFor = async (id, { parent } = {}) => {
@@ -73,7 +78,8 @@ async function setup({ state = team() } = {}) {
     return agent
   }
   const allHidden = id => new Set([...(masks.get(id) ?? [])].flatMap(mask => [...mask]))
-  ctx.tools.schemas = agent => live.served.filter(name => !allHidden(agent?.id).has(name)).map(name => ({ name }))
+  ctx.tools.schemas = agent => (agent === undefined ? [...live.served] : [...live.served, ...live.own]
+    .filter(name => !allHidden(agent?.id).has(name))).map(name => ({ name }))
   let sessions = 0
   Object.assign(ctx, {
     sessions: { get: id => agents.get(id)?.session, flush: async () => {} },
@@ -105,7 +111,7 @@ async function setup({ state = team() } = {}) {
     begin(id)
     return () => end(id)
   }
-  return { ctx, rt, home, live, agentFor, hiddenFrom, allHidden, guard, call, stop, sent, event, begin, take, end, turn }
+  return { ctx, rt, home, live, warns, agentFor, hiddenFrom, allHidden, guard, call, stop, sent, event, begin, take, end, turn }
 }
 const tick = () => new Promise(resolve => setImmediate(resolve))
 
@@ -200,6 +206,9 @@ test('#1237: a room turn drops the composio and schedule tools and keeps github
   assert.deepEqual([...hiddenFrom('room-writer')].sort(), [...COMPOSIO].sort(), 'every composio tool goes')
   assert.deepEqual([...allHidden('room-writer')].sort(), [...COMPOSIO, ...SCHEDULE].sort(), 'and the four schedule tools, nothing else')
   for (const name of [...GITHUB, ...OWN]) assert.equal(allHidden('room-writer').has(name), false, `${name} stays`)
+  // #1271: pinned literally. The comparison above uses the same constant the mask reads, so it cannot
+  // notice a renamed or emptied SCHEDULE_TOOLS; these are the DSH names (0.2.0-rc.2).
+  for (const name of ['schedule_create', 'schedule_list', 'schedule_update', 'schedule_delete']) assert.ok(allHidden('room-writer').has(name), `${name} is hidden in a room`)
   for (const name of [...GITHUB, ...OWN]) assert.equal(guard(name, 'room-writer'), undefined, `${name}`)
   for (const name of [...SCHEDULE]) assert.match(guard(name, 'room-writer'), /own chat, not in a group room/, name)
   assert.equal(guard('schedule_list', 'room-writer'), 'Schedules are managed in your own chat, not in a group room (#1237).')
@@ -233,6 +242,26 @@ test('#1237: a schedule tool that appears after the mask landed is denied on the
   assert.deepEqual([...allHidden('room-writer')].sort(), [...COMPOSIO, ...SCHEDULE].sort(), 'the late tools are denied too')
 })
 
+// #1271: hideConnectorTools lifts the old mask before it installs the new one, so a restrict() that
+// throws left the Agent with no mask at all — the list widened back to every tool. A name the profile
+// does not serve globally (one the Agent itself serves) is what makes restrict() throw.
+test('#1271: a mask that cannot be installed keeps the previous narrowing, it does not widen the list', async (t) => {
+  const { ctx, live, agentFor, allHidden, warns, stop } = await setup()
+  t.after(stop)
+  await agentFor('room-writer')
+  assert.deepEqual([...allHidden('room-writer')].sort(), [...COMPOSIO, ...SCHEDULE].sort(), 'the room mask lands first')
+  // A connector tool the Agent itself serves: schemas(agent) lists it, restrict() refuses it.
+  live.own = ['mcp__composio__SOMETHING_LOCAL']
+  ctx.emit('tools/change')
+  await tick()
+  for (const name of [...COMPOSIO, ...SCHEDULE]) assert.ok(allHidden('room-writer').has(name), `${name} stays hidden`)
+  // #1271: and the host says which name it had to leave listed — the docs and the changelog both say it
+  // warns, so a silent filter was the other half of the defect.
+  const said = warns.map(args => args.map(arg => String(arg)).join(' ')).join('\n')
+  assert.match(said, /connector tools stay listed for Writer: mcp__composio__SOMETHING_LOCAL cannot be restricted by name/, 'the warn names the tool that stays listed')
+  assert.doesNotMatch(said, /schedule_list/, 'and only the names it dropped, not the whole deny list')
+})
+
 test('#601: a group round of any origin keeps the github tools, also with a friend in the room; #1237 keeps composio and schedule out', async (t) => {
   const { rt, agentFor, allHidden, guard, begin, take, end, stop } = await setup()
   t.after(stop)

docs/subsystems/core-host.md

@@ -354,6 +354,7 @@ The connector tools are mounted once for the whole DSH profile, so which Bot may
 - **Per-Bot record** (only what differs from the default is stored): `bot.connectors = { github?: false, composio?: false }` is an explicit off for that Bot (absent = on; a legacy `true` from #346 reads as on). `bot-connectors { botId, connector, enabled }` writes it; a `scope` an older client still sends is ignored. The retired "also when others use it" offer (`bot.connectorsShared`) means nothing any more and is dropped on the Bot's next switch or on a disconnect.
 - **Boundaries that stay.** A friend's Bot (`bot.remote`) never gets the tools. An Agent whose Bot cannot be determined (a parent Session that is gone, or a parent chain deeper than eight) gets none either (fail closed), so an unresolved sub-agent never reads as a plain owner Session. A chain that really ends at a Session of no Bot (the DSH Agent, a plain Session) keeps them. A sub-agent Session is followed to its parent Bot (`whoseAgent`, also on `rt.whoseAgent` for `secrets.js`).
 - **Enforcement is two layers.** A `ctx.tools.guard` refuses every `mcp__github__*` / `mcp__composio__*` call that is not allowed, and `agent.ctx.tools.restrict({ deny })` takes the same tools out of that Agent's tool list. The mask is refreshed when the Agent is created, on every `resolveAgent` (delivery), when a switch changes, and on `tools/change` (a connector that mounts or goes away later). Turns and sharing no longer change it. DSH's hidden "Tool added/removed" notices for these tools are not drawn in the chat (`cells.js`).
+- **A mask that cannot be rebuilt keeps the previous one (#1271).** `hideConnectorTools` lifts the old mask before it reads `schemas()` again, so a `restrict()` that refuses a name would otherwise leave the Agent with no mask and every tool listed again. The rebuild now names only what the global view (`schemas()` with no Agent) can restrict, and if a restriction still fails the catch puts the previous deny list back: the guard refuses a call either way, and a list that widens by itself is a wrong answer. A denied name the Agent itself serves stays listed — `restrict()` cannot name a scope's own tool — and the warn line says so.
 - **A group room turn carries no composio tools and no schedule tools (#1237).** The eight tools — `mcp__composio__*` and `schedule_create` / `schedule_list` / `schedule_update` / `schedule_delete` — are 20,355 characters of schema (≈5.2k tokens at the measured 3.92 chars/token) on every call, and a room round uses neither: an outside app is reached from the Bot's own chat, and a schedule belongs to a Session (#628), so managing it from a member's room Session would write to the wrong place. The rule is one function in `connector-access.js`: the same `denied()` that answers the per-Bot switches also names these eight when `rt.groupOwners` — the store's Session → room index — has the Agent's Session, so the mask rides the existing refresh points (Agent created, `resolveAgent` on every delivery, a switch, `tools/change` for a plugin that mounts later) and the guard refuses a call with `… not available in a group room` — the room answer comes before the friend's-Bot one, and a friend's Bot has no composio tools in its own chat either, so the refusal does not send it there. The github tools stay (a room judges pull requests), and the Bot's own chat, its earlier parts and its sub-agents are unchanged. Taking the github tools out too is a separate decision, in #1237's 「不做」.
 - **Bots prefer the connector for GitHub.** The Bot rules (`prompt.js`) say to use the GitHub connector tools first and `$DSH_SECRET_GITHUB_TOKEN` with `gh` or `git` in the shell only for what they cannot do, such as `git push`.
 - **Defaults and disconnect.** New and duplicated Bots (and tree children) start on. The Connectors row reads "On for all N Bots", "On for K of N Bots" or "Off for every Bot"; **Choose Bots** and each Bot's **Connectors** card show one switch per Bot and connector. **Disconnect** clears every Bot's record for that connector. The switches change only through the `bot-connectors` channel endpoint, which Bots do not call; it is in `SHARED_BLOCKED`, so with more than one member it is refused like the other connector writes.

patches/ds-bot/files/src/host/connector-access.js

@@ -151,9 +151,32 @@ export function install(rt) {
       // After the old mask is lifted, schemas() lists what is really served now.
       const fresh = served(agent)
       const deny = fresh === undefined ? [] : denied(who, fresh)
-      masks.set(agent, { key: deny.join(','), deny, dispose: deny.length > 0 ? agent.ctx.tools.restrict({ deny }) : undefined })
+      // #1271: restrict() refuses a name the profile does not serve as a global tool, and a tool the Agent
+      // itself serves is listed by schemas() all the same. One such name made the whole call throw after
+      // the old mask was already lifted, so the Agent quietly got every tool back. Ask the global view
+      // what a restriction can enforce and name only those: the guard still refuses a call on the rest.
+      const global = fresh === undefined ? undefined : new Set((ctx.tools.schemas?.() ?? []).map(schema => schema.name))
+      const enforceable = global === undefined ? deny : deny.filter(name => global.has(name))
+      // #1271: what the global view cannot name stays in the Agent's list, and the guard is the only thing
+      // left refusing it. Say which names those are — a silent narrow list is what the docs promised was
+      // not happening.
+      const listed = global === undefined ? [] : deny.filter(name => !global.has(name))
+      if (listed.length > 0) rt.warn('ds-bot: connector tools stay listed for %s: %s cannot be restricted by name', bot?.name ?? agent?.id, listed.join(', '))
+      masks.set(agent, { key: deny.join(','), deny: enforceable, dispose: enforceable.length > 0 ? agent.ctx.tools.restrict({ deny: enforceable }) : undefined })
     } catch (error) {
-      rt.warn('ds-bot: connector tools stay listed for %s: %s', bot?.name ?? agent?.id, error)
+      // #1271: a backstop, and unreachable by construction on this API — restrict() validates against the
+      // inherited layers, and that is exactly what schemas() without an Agent returns, so a name the filter
+      // above just allowed cannot be refused as unknown. No test reaches this branch today. It stays for a
+      // DSH that refuses for a reason of its own: by then the old mask is lifted, and putting the previous
+      // narrowing back beats leaving the list wide, because a Bot reads its own list to decide what it can do.
+      if (current !== undefined && current.deny.length > 0) {
+        try {
+          masks.set(agent, { ...current, dispose: agent.ctx.tools.restrict({ deny: current.deny }) })
+        } catch {
+          masks.delete(agent)
+        }
+      }
+      rt.warn('ds-bot: connector tools stay as they were for %s: %s', bot?.name ?? agent?.id, error)
     }
   }