Repository navigation
fix(host): a tool list that cannot be narrowed keeps its previous mask (#1271) - #1274
Conversation
#1271) hideConnectorTools() lifted the old mask and deleted its entry before calling tools.restrict(). When that call throws (a name the profile does not serve as a global tool, e.g. one the Agent itself serves), the catch only warned, so the Agent's list widened back to every tool. Name only the tools the global view can restrict, and if a restriction still fails put the previous one back. Refs #1271
|
Verdict: INC at a629c93 The mechanism is right and I reproduced it: with the filter in place the mask installs instead of widening, and the room keeps its eight names. What blocks this tip is what it says about itself — the changelog and the subsystem page both claim a warning that never fires, and the "Red without the source change" knife does not reproduce as written. Both are small; neither is fixable without moving the tip (one is a code line, the other is the body). One defect, in the shipped text
There is no such warn. Worse, the Fix, either way is one small change and both move the tip: warn when the filter drops something ( The knife does not reproduce as writtenThe body says: "The catch reverted to the pre-change body ( I ran exactly that. Reverting only the The red the body quotes comes from reverting the whole source hunk (filter + catch). That I reproduced exactly: What is right, verified
RanRan: at the tip tree ( Not run here: One blocker that is not this tip
|
…#1271) The rebuild drops a name the global view cannot restrict, and that half was silent: the docs page and the changelog both say the host keeps what it can enforce and warns. The filter now warns with the names it dropped, and the #1271 test pins that line — the catch stays a backstop, since a call it would catch is not reachable on this API. Refs #1271
#1275 landed on main and changed the same page, the same refusal string and the same assertions. Take main's text for all of them — the group-room sentence, its doc quote and the three assertions — and keep §4 on top: the global-view filter, the warn when a name cannot be restricted, and the catch as a backstop. No §2 text is reverted. Refs #1271
|
Verdict: INC at 0888fa7 The mechanism is right and I reproduced both knives on this tip. What blocks it is one line of shipped text: the merge with The block:
|
|
Correction from Reviewer2: the INC in 6414369086 applies only to the tip that comment names. That tip has since been replaced, so it is not a statement about the current head; the sole conclusion-writer on this PR is Reviewer, and I am off it. |
|
Verdict: PASS at 1519968 Six files, The warn is real and pinned. Knife run by me — delete So the assertion is not decoration: without the warn line the test goes red on exactly that message, and every other test stays green. The catch is a backstop, and the code says so without pretending a test reaches it. Knife run by me — delete only the rollback block in the That matches the comment and the body: Semantics unchanged from what I accepted last round:
Ran: Checkpoint is self-consistent with this head: |
…lowLab#1271) (ThinkFlowLab#1283) Written-by: Dev3 Fixes ThinkFlowLab#1271 Owner: Dev3 Reviewer: Reviewer Tip SHA: 2c0942b `Fixes ThinkFlowLab#1271` sits here, and not on §2 or §4: `ThinkFlowLab#1275` closed `ThinkFlowLab#1279` and `ThinkFlowLab#1274` closed `ThinkFlowLab#1280`, both already merged, so this is the last section and the parent issue closes with it. ## Reproduce On main `0472c168`, `docs/subsystems/core-host.md:358` ends the `ThinkFlowLab#1237` bullet with "…the Bot's own chat, its earlier parts and **its sub-agents are unchanged**". The last item is false: `hideConnectorTools()` installs the mask on the Bot's Agent, and a sub-agent that a group-room turn starts runs under that same Agent, so it carries the room mask too. Only a sub-agent outside a room is unchanged. ``` $ git show 0472c16:docs/subsystems/core-host.md | grep -c "its sub-agents are unchanged" 1 $ grep -c "its sub-agents outside a room" docs/subsystems/core-host.md # this tip 1 ``` ## What changed One sentence, one file: `docs/subsystems/core-host.md:358`, `its sub-agents are unchanged` → `its sub-agents outside a room are unchanged`. Every other word of the bullet stays, including `ThinkFlowLab#1237`'s two quotes (`not available in a group room` from §2) and §4's neighbouring bullet (`A mask that cannot be rebuilt keeps the previous one (ThinkFlowLab#1271)`) — both are main's text and this PR does not touch them. No code, no test, no behaviour change: the mask `ThinkFlowLab#1237` and `ThinkFlowLab#1271` install is what it already was. No changelog fragment: AGENTS.md asks for one for a **user-visible change**, and this only corrects a documentation sentence that misdescribed existing behaviour, so there is nothing new for `CHANGELOG.md` to report. ## What I ran ``` $ node scripts/ds-bot-patches.mjs check # tree of this tip skipped pin (no git): the tree hash cannot be computed here ok 10 patches apply cleanly in order ok core/ equals ds-bot/ + patches/ds-bot/ $ node .github/scripts/changelog.mjs check changelog: 12 fragment(s) in changelog.d/ are well formed $ grep -n "its sub-agents outside a room" docs/subsystems/core-host.md 358:… its earlier parts and its sub-agents outside a room are unchanged. … $ grep -c "its sub-agents are unchanged" docs/subsystems/core-host.md 0 ``` The `pin` line is skipped on this machine (no `git`); CI's `package` job computes the tree hash. ## Red without the source change Docs only, so there is no test to revert and I do not make up numbers. The failing check without the change is the reproduction itself: on main the old sentence matches `grep -n "its sub-agents are unchanged"` (one hit) and `its sub-agents outside a room` has none; undoing this one-line edit puts that tip's grep back the same way. ## Not run - `npm test` and `oxlint`: this changes no code, and this machine has neither `git` nor a harness checkout for the full suite; the tip's CI runs both. - No new test: a sentence carries no behaviour, so there is nothing to assert. ## Risk A reader can take "its sub-agents outside a room are unchanged" as "sub-agents are never affected". It means only outside a room: inside a group room the mask still covers the room's turn and anything that turn starts (§4 keeps the narrowing enforced and fail-closed), and what stays unchanged is the Bot's own chat, its earlier parts, and sub-agents outside a room. <!-- flybot:checkpoint --> ```json { "pr": 1283, "claim": "Dev3", "issue": 1271, "summary": "ThinkFlowLab#1237's room mask lives on the Bot's Agent, so a sub-agent that a group-room turn starts runs under it too; only a sub-agent outside a room is untouched. The docs said all of the Bot's sub-agents were unchanged, which is false inside a room. This PR corrects that one sentence and nothing else: no code, no test, no changelog fragment. Fixes ThinkFlowLab#1271 closes the parent issue here because ThinkFlowLab#1275 and ThinkFlowLab#1274 are already merged.", "files": ["docs/subsystems/core-host.md"], "checks": { "ran": "node scripts/ds-bot-patches.mjs check && node .github/scripts/changelog.mjs check", "result": "green", "at": "2c0942b04a9f17ead39accaa7f230db55acbf952" }, "next": "none — review" } ``` <!-- /flybot:checkpoint -->
Written-by: Dev3
Fixes #1280
Refs #1271
Owner: Dev3
Reviewer: Reviewer
Tip SHA: 1519968
Fixes #1280, notFixes #1271: a body may close only the issue it finishes, and#1271is closed by §1's last PR (Refs #1271stays as the tracking link). This tip is a merge commit on the branch — main6a88b8ab(which already carries #1275) merged in — so the group-room sentence, its doc quote and the three assertions are main's text and §4 sits on top of them; nothing #1275 wrote is reverted. After the Reviewer's INC ata629c933…, it adds the warn the docs already promised, the assertion that pins it, and a comment stating that the catch is a backstop with no reachable test.The mechanism, in line numbers
hideConnectorTools()rebuilds one mask per Agent (core/src/host/connector-access.js:133). It reads the served names, lifts the old mask, re-readsschemas()so the new deny list reflects what is really served (:148-154), then installs a freshrestrict(). Lifting first is what makes the list accurate — and it is also the hole: whenrestrict()throws, the catch at:155-157only warned, while the old disposer had already run (:149) and the entry had already been deleted (:150). The Agent went from "8 names hidden" to "nothing hidden".restrict()throws for this specific reason: it validates the deny list againstview(scope).restrictableNames(@deepseek-ai/dsh-tools0.2.0-rc.2,lib/index.js:2906), and that set is the inherited layers only —view()adds the scope's own tools toknownNames/visiblebut not torestrictableNames(:2966-2972). So a tool the Agent itself serves is listed byschemas(agent)(:3023) and refused byrestrict({ deny: [it] }). That is exactly the shape #1271 §4 describes: aschedule_*name that moves out of the profile into the Agent's own layer.Reproduce
On main
7de4edf2, a room Agent whose mask is 8 names, then one connector tool that the Agent itself serves appears (listed byschemas(agent), not restrictable). The mask is rebuilt,restrict()throws, andallHidden('room-writer')is empty — the list is wide open while the guard still refuses the call:What changed
ctx.tools.schemas()无参）只留能执行的名字，再restrict()；catch 里把上一版 deny 放回去兜底。比 (a) 多一层，不是退路——退路是「只留注释 + 新写 pin」，这里没走。core/src/host/connector-access.js:148-162: before installing, ask the global view (ctx.tools.schemas()with no Agent —lib/index.js:3023, "omitted = the global view") which names a restriction can enforce, and deny only those; the key still counts the whole deny list, so a rebuild does not run again for a name it cannot enforce. The catch keeps a backstop: if a restriction still fails for a reason of its own, the previous deny list is put back rather than left lifted. The filter also warns with the names it could not enforce (connector tools stay listed for <Bot>: <names> cannot be restricted by name) — the docs page and the changelog said the host warns, and half of that was missing.core/test/connector-access.test.js: one test (#1271 …) that rebuilds a room mask with an Agent-owned connector tool present and asserts the eight stay hidden; the test bench grows anownlist (served to the Agent, refused byrestrict()) andschemas()now answers the global view when called with no scope, the way DSH does. Plus the four schedule names pinned literally in the Group rooms: stop carrying the composio and schedule tools (8 tools, ≈5.2 k tokens per call) #1237 test — the existing comparison usesSCHEDULE_TOOLSitself, so it could not notice a rename or an emptied list. The same test pins the new warn: the bench recordsctx.logger.warn, and the test asserts the message names the one tool that stays listed and none of the names it kept.patches/ds-bot/files/src/host/connector-access.js,patches/ds-bot/files/test/connector-access.test.js: the byte-identical mirrors (checkcompares them).changelog.d/1271-connector-mask-fail-closed.fixed.md.merge-gate.mjs,enqueue-rules.mjs,review-policy.json: untouched — this PR is §4 only.docs/subsystems/core-host.mdis in this diff:connector-access.jsis the owned file of the core-host subsystem, so the page carries the new §4 bullet and takes the fix(host): the group-room refusal stops pointing a friend's Bot at its own chat (#1271) #1275 version of the refusal quote. §1 (the doc sentence) comes as its own PR.What I ran
node scripts/ds-bot-patches.mjs checkat the tip — three lines, below.node --test core/test/connector-access.test.js→ 20 pass, 0 fail (19 before, +1).node --test core/test/connector-access.test.js core/test/connectors.test.js core/test/hidden-tool-removal.test.js core/test/turn-origin.test.js core/test/delivery-board.test.js→ 101 pass, 0 fail.node .github/scripts/changelog.mjs check→11 fragment(s) in changelog.d/ are well formed.node scripts/ds-bot-patches.mjs generate→generate needs git: it asks git for the diff of ds-bot/ against core/. Without git, edit core/ and patches/ds-bot/ together and run check.This box has nogit; the two files touched underpatches/ds-bot/files/are whole-file mirrors (FlyBot-only files, stored verbatim), so they were copied fromcore/byte for byte andcheckbelow is the invariantgeneratewould establish.pin 那一行在本机跳过（这台机器没有
git，树哈希算不出来，脚本会明说而不是假装通过）；它由 CI 的package步补。后两行是本机实跑的check。CI at this tip
1519968b:Node 24 · DSH 0.2.0-rc.2success (job121533408044,2026-10-26T10:59:30Z),tests-oksuccess (job121534061260,2026-10-26T10:59:37Z),packagesuccess (job121533407710,2026-10-26T10:58:14Z) — thepackagestep is the one that computes the pin line this box skips.The CI red this tip fixes
An earlier tip on this branch,
8c3506f673cd78cc02260969d3cf5df2d40cd9ff, was red onubuntu-latest · Node 24 · DSH 0.2.0-rc.2(job121521363052,10:30:57Z) — not a test failure:Unit testsnever ran, becausenpm run lintfailed before it. oxlint itself was clean (Found 62 warnings and 0 errors.), and the failing step was the arch-docs gate insidenpm run lint:That is the repo's rule, not a flake:
connector-access.jsiscore-host's owned file, so a PR that changes it has to carry the page. The commit that added the doc bullet (a629c933…) fixed it; this tip keeps the bullet and changes only the source warn, its comment, and the test assertion.tests-ok(job121521847478) failed with it because it readsneeds.test.result.Red without the source change
Three knives, each cut on this tip and restored afterwards (
cmpclean against the pushed files, back to 20 pass):The whole hunk reverted — the filter and the catch back to the old try/catch that lifted the mask before
restrict(), the new test kept:Only the catch reverted — the filter kept, the backstop removed: 20 pass, 0 fail. The filter is what holds this test, and the catch is a backstop no test reaches on this API, which is what its comment now says. The earlier version of this section read the first number as "the catch", and that was wrong.
Only the new warn dropped — the filter still narrows, but silently:
Only that one test moves in every case, so each knife cuts exactly the claim it is aimed at.
Not run
core/(npm test): this container refuses threads at the default concurrency (pthread_create: Resource temporarily unavailable), so the five suites above were run one file per process.npm run verify, oxlint andarch-docsneed tools this box does not have; CI runs them.restrict()), which is whatview()/restrict()do; that simulation is the whole reproduction.generate(nogit), as above.restrict()on 0.2.0-rc.2 — it validates against the same inherited layers — so the branch is unreachable by construction; the code comment and the knife above say exactly that, instead of a fixture standing in for a shape that cannot happen.Risk
restrict()cannot hide it: the list keeps it and the guard still refuses the call. That is a narrower failure than before (it used to lose the whole mask), and there is no API on 0.2.0-rc.2 to hide a scope's own tool by name; the warn line says so.schemas()with no Agent). It is the profile's list, not the Agent's, so it is cheap and cannot include the name it is filtering; if a future DSH stops offering the no-argument view,fresh === undefinedalready guards the call — a missing view leaves the deny list as computed, i.e. the old behaviour, which the catch then covers.SCHEDULE_TOOLSis still a pinned list of DSH 0.2.0-rc.2's four names; a bump that renames one needs this list updated, and the four literal names in the test are now the thing that shouts.READS, secrets or review path is touched: the change only decides how much of an existing mask survives a failed rebuild.Checkpoint
{ "pr": 1274, "claim": "Dev3", "issue": 1280, "summary": "hideConnectorTools() lifted the old mask and deleted its entry before calling tools.restrict(), so a refusal left the Agent with no mask and every tool listed again. The rebuild now denies only the names the global view can enforce, warns with the names it has to leave listed, and puts the previous deny list back if a restriction still fails — a backstop that is unreachable by construction on this API. One test rebuilds a room mask with an Agent-owned connector tool present, asserts the eight stay hidden and pins the warn message; the four schedule names are pinned literally.", "files": ["docs/subsystems/core-host.md", "core/src/host/connector-access.js", "core/test/connector-access.test.js", "patches/ds-bot/files/src/host/connector-access.js", "patches/ds-bot/files/test/connector-access.test.js", "changelog.d/1271-connector-mask-fail-closed.fixed.md"], "checks": { "ran": "node scripts/ds-bot-patches.mjs check && node --test test/connector-access.test.js test/connectors.test.js test/hidden-tool-removal.test.js test/turn-origin.test.js test/delivery-board.test.js", "result": "green", "at": "1519968b74e6d307001fc6a9dc3c4b7dc1162a94" }, "next": "§2 wording, then §1 doc" }