fix(rovodev): nest per-tool permissions under toolPermissions.tools and translate MCP transport - #2442
Merged
dyoshikawa merged 7 commits intoJul 27, 2026
Conversation
…nd translate MCP transport Two bugs where rulesync wrote a key Rovo Dev does not read. Per-tool permission levels went directly under `toolPermissions`, one level above the documented `toolPermissions.tools`. Rovo Dev ignores them there, so generated permissions never applied and a real user config round-tripped to nothing. Verified against both live support docs, which agree on the nesting. Generate now writes the nested block, merging into an existing one so a tool the user set by hand that rulesync has no category for survives, and import reads the nested block first while still falling back to the flat shape an earlier rulesync produced. MCP servers were written with the canonical `type` key, but Rovo Dev documents `transport` (`stdio` | `http` | `sse`) and no `type` alias. Canonical values are translated on the way out (`local` → `stdio`, `streamable-http` → `http`) and back on import; `ws` has no equivalent, so the key is omitted with a warning. The kilo/roo/goose/amp/codexcli adapters already did this. The issue's other items are left open: `.rovodev/.review-agent.md` needs a new checks adapter, `mcp.disabledMcpServers` needs the MCP feature to become a second writer of the shared `config.yml`, and hooks stay blocked on Atlassian never having published the `eventHooks` schema. Not implemented from the issue's gap 1: `toolPermissions.default`. The live docs page does not show that key, so the claim could not be confirmed. Refs #2422 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…not express Review follow-ups on the same two adapters. Permissions: generate left the legacy flat per-tool keys in place next to the new nested block. Rovo Dev ignores them, but this adapter still imports them as a fallback, so a category the user had since deleted came back on the next import and then propagated to every other tool. The eight per-tool keys are now rulesync-owned: both the flat copies and the stale nested ones are dropped before the current set is written, so revoking a rule in .rulesync/permissions.* actually revokes it. Tool keys no canonical category maps to are still preserved. Import no longer lets an invalid nested value shadow a usable legacy one, and allowedExternalPaths no longer overwrites a level already read from the tool keys — Rovo Dev appends to that list itself whenever the user approves a path at runtime, so an entry of "*" could turn a read deny into an allow. MCP: a ws server was written without a transport key, leaving Rovo Dev to guess; it is skipped instead, matching the Kimi Code adapter over the same vocabulary. A server marked disabled was written as a live entry, since Rovo Dev disables servers through mcp.disabledMcpServers in config.yml and mcp.json has no flag for it — skipping the entry reaches the same end state fail-closed. Import skips a hand-written non-object entry rather than throwing on it. The e2e permissions case asserted the old flat depth; it now reads the nested block. Also moves the new MCP note into the MCP section of file-formats.md, where the sibling notes live, and fixes a 404 doc link. Refs #2422 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…vels fail-closed Round-two review follow-ups. The last commit made the eight per-tool keys rulesync-owned but left `bash` and `allowedExternalPaths` merged, so deleting a bash rule or withdrawing a path grant in .rulesync/permissions.* left the old value live in config.yml — the same revocation hole, and the docs already claimed otherwise. Both keys are now dropped before the current set is written, which also covers a path added with the in-session /directories command. Import collapsed a category's four tool keys with last-write-wins. Rovo Dev rewrites a single key when the user answers "always allow" to one prompt, so `grep: allow` next to three denies imported as a blanket `read: allow` and propagated to every other tool on the next generate. They now fold onto the strictest level, matching the augmentcode and opencode adapters. A key present in the nested block also settles the level even when its value is unusable — falling back to a legacy copy Rovo Dev does not read would record a level the tool is not applying. MCP: an unrecognized `transport` was carried into the canonical config, whose transport field is a strict enum, so one typo in mcp.json made .rulesync/mcp.json fail to parse for every target. It is dropped instead. `disabled: false` is stripped alongside `disabled: true`, since mcp.json is not where a Rovo Dev server is switched on and off. Refs #2422 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… owned-key removals Round-three review follow-ups. When `edit` and `write` set conflicting catch-alls, only one of the two can be written to Rovo Dev's shared mutation tools. Resolving to `edit` meant `edit: allow` next to `write: deny` wrote an allow — dropping the stricter level the author had asked for. It now keeps the stricter of the two, the same rule the import direction uses, so the resolution never grants more than was authored. The warning still fires and now names the level that won. A generate that produces nothing this adapter can express — a source with only categories Rovo Dev has no target for — no longer clears the owned keys. There would be no rule of ours to put in their place, so the user's levels would just relax to Rovo Dev's defaults. Removal of an owned key is also announced now, since `allowedExternalPaths` is written from inside a session by `/directories` and would otherwise vanish silently. A `tools` value of some other shape is left alone rather than deleted. Two per-tool key lookups walked the prototype chain (`in` on the nested block, a bare index into the transport tables), so a server or tool named `toString` resolved to something from `Object.prototype`. Both use own-property checks now. Refs #2422 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…and name every owned key removed Round-four review follow-ups. The guard added last round skipped the write whenever the generated block came out empty, which also covered a source that states no rule at all. That is a deliberate clean slate, so a revoked blanket allow survived it. The two cases are now distinguished: an empty source clears the owned keys as ownership implies, while a source whose rules simply have no Rovo Dev counterpart leaves the block alone and says so. The removal warning covered only bash and allowedExternalPaths, but a per-tool level is a session-written surface too — Rovo Dev rewrites one of those keys when the user answers a prompt with "always allow". It now names every owned key being removed, per-tool ones included. Also folds the duplicated constructor call into one return and corrects the doc sentence that claimed the warning covered all eight per-tool keys. Refs #2422 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…write A source whose rules Rovo Dev cannot express leaves the toolPermissions block in place — but that block still held whatever an earlier generate wrote, so revoking `edit: allow` from a source that kept only unmappable rules left the mutation tools on `allow`. Nothing can be written in their place, yet dropping an `allow` is safe on its own: it hands the decision back to Rovo Dev's stricter default. That branch now strips the grants — per-tool `allow` levels, `allowedExternalPaths`, `bash.default: allow` and allow entries in `bash.commands` — while every restriction stays, and the warning names what it took out. The merge is now a pair of module-level helpers; it had grown past the complexity ceiling inside the method. Refs #2422 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The helper extraction moved the assignment to an unconditional one, so a
config.yml with no toolPermissions block — or one of some other shape — came
back with an empty `toolPermissions: {}` grafted on whenever the run had
nothing to write. The resolver now returns undefined for that case and the
caller leaves the key alone.
Also stops naming `bash` itself among the stripped grants when removing its
last sub-key empties it, and softens a doc sentence that called every stray
`allow` a leftover of an earlier generate — a prompt answered with "always
allow" writes one too.
Refs #2422
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Owner
Author
|
@dyoshikawa Thank you! |
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.
Two adapter bugs where rulesync wrote a key Rovo Dev does not read, plus the ownership and fail-closed fixes that fell out of reviewing them.
1. Per-tool permissions were written one level too shallow
Rovo Dev documents per-tool levels under
toolPermissions.tools.<tool>. Rulesync wrote them directly undertoolPermissions.<tool>, where Rovo Dev ignores them — so generated permissions never applied, and importing a real user config round-tripped to nothing. Confirmed against the live use tools page, which shows:Generate now writes the nested block, and import reads it — falling back to the legacy flat shape only for a key the nested block says nothing about, so a
config.ymlan earlier rulesync produced still imports.Not implemented from the issue's gap 1:
toolPermissions.default. The issue's YAML block shows it, but the live docs page does not, so the claim could not be confirmed from a primary source.2. MCP servers carried the canonical
typeinstead oftransportRovo Dev documents the per-server transport key as
transport(stdio|http|sse), with notypealias.RovodevMcp.fromRulesyncMcpwrotegetMcpServers()verbatim, so a canonical"type": "http"— the spelling this repo's own.rulesync/mcp.jsonuses — reached the file under a key Rovo Dev does not read.Translated in both directions (
local→stdio,streamable-http→http,sse→sse), matching the kilo / roo / goose / amp / codexcli adapters. Two entries are skipped with a warning rather than written as a guess: awsserver (no Rovo Dev equivalent — same as the Kimi Code adapter) and adisabledone, since Rovo Dev turns servers off throughmcp.disabledMcpServersinconfig.ymlandmcp.jsonhas no flag for it. On import, atransportvalue outside Rovo Dev's vocabulary is dropped rather than carried into the canonical config, whose transport field is a strict enum — passing one through would make.rulesync/mcp.jsonfail to parse for every target.3. Ownership: a revoked rule now actually goes away
Fixing (1) surfaced a worse problem than the wrong depth.
bash,allowedExternalPaths, and the eight per-tool keys were merged into whatever was already inconfig.yml, so deleting a rule from.rulesync/permissions.*left the old value live — and the stale flat copies were then resurrected by the import fallback and propagated to every other tool. Those keys are rulesync-owned now: dropped before the current set is written, with the keys rulesync has no category for (createTechnicalPlan, the Jira/Confluence tools) left untouched.Two guards around that:
allowedExternalPathsis also written from inside a session by/directories.4. Collisions resolve fail-closed
Both directions had a path where the looser of two levels won:
grep: allownext to three denies imported as a blanketread: allow. They fold onto the strictest level now, matching the augmentcode and opencode adapters.editandwritedrive the same mutation tools, so a conflicting catch-all cannot be represented. Resolving toeditwrote an allow when the author had also saidwrite: deny; the stricter level is kept instead, and the warning names it.Left open on the issue
.rovodev/.review-agent.md(checks) — needs a newRovodevCheckadapter; a feature addition rather than a drift fix.mcp.disabledMcpServers— honouring canonicaldisabledmeans making the MCP feature a second writer of theconfig.ymlthe permissions feature already owns, with sub-key seeding somcpConfigPath/allowedMcpServerssurvive. Worth doing, but its own change./hooksandeventHooksare referenced but the schema has never been published.Follow-up worth filing
strictestActionhere is the fourth implementation ofdeny > ask > allowinsrc/features/permissions/(grokcli, opencode, amp). A shared helper would be the right home for it.Tests
52 unit assertions across the two adapters, including the legacy-flat import, the nested-wins direction where the nested value is the looser one (so a regression is visible), the strictest fold, ownership deletion vs. preservation of unmapped keys, both conflict directions, the five transport translations, and the skip cases. The e2e permissions case moved to the nested depth.
pnpm cicheckand the permissions/MCP e2e suites pass locally.Refs #2422