fix(mcp): read a scoped overlay teammate's own grant, and bound add_agent by its minter - #745
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAgent creation and editing now enforce tool scopes from minter grants and actor permissions. MCP reachability uses each overlay agent’s configured scope, with company-level inheritance for empty scopes. ChangesAgent scope propagation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
|
oxoxDev
left a comment
oxoxDev
left a comment
There was a problem hiding this comment.
The split from #717 is the right call — the field and the fingerprint landed in 1171afcc, and this carries the reader half. Both things I checked on the earlier version survive it intact:
roster_grants's overlay half is fixed. It wasagent_effective_grants(allow, &[])— hard-coding every overlay teammate to the full company grant, so a teammate scoped away frommcp:*still showed as reaching every enabled server. Narrowing each by its own line makes console reachability equal what the agent is actually granted.- The narrow-to-empty guard in
add_agentis intact, with its actionable error naming what the minter holds. That guard is the reason for everything below.
1 major. Requesting changes.
Major — PATCH …/team/{agent_id} lets any member widen a teammate's scope
edit_agent takes ScopedCompany and says so deliberately:
Open to any signed-in member, matching
POST …/team: defining a teammate was never admin-only, so correcting one it defined is not either.
That reasoning is sound for name, role and description. It does not transfer to tools, and the handler applies it with no field-level check:
if let Some(tools) = tools {
agent.tools = tools;
}An empty tools list means inherit the company's standard grant — that is the semantics this feature is built on, and it is exactly why add_agent treats a narrowing that lands empty as a hard error rather than storing it. So a member can PATCH {"tools": []} onto a teammate an admin deliberately scoped to ["file_read"] and restore the full company [tools].allow.
It is bounded — [tools].allow is the ceiling and this route does not touch it — so this is not escalation past what the company granted. But it defeats an admin's deliberate narrowing, and that narrowing is the capability #619/#661 shipped. A scope any member can remove is not a scope.
The asymmetry is the clearest way to see it: an agent minting a teammate is bounded by its own effective grant and refused if the result is empty; a human member editing the same field over HTTP is bounded by nothing and can set empty as the documented way to widen. The tighter rule is on the automated path.
Two fixes, either fine:
- Gate the field, not the route. Keep
edit_agentmember-open for the cosmetic fields and require admin whentoolsis present — the same shapePOST …/teamuses for budgets, which the doc already notes stays admin-only "on its own route". - Refuse
[]here. Make widening its own explicit verb rather than a value, so the route cannot express "restore everything" by accident, and narrowing stays member-safe.
I would take the first: it keeps one route and puts the check where the privilege actually is.
Worth noting this is the same extractor class #729 just fixed — a bare ScopedCompany on a handler whose payload turned out to be an authority question. That one was a read; this is a write.
Also worth confirming
OVERLAY_EDITABLE now lists tools beside name / role / description. Once the gate above exists, the editable array a client reads should reflect who may actually edit what, or the console will offer a control the host will refuse.
`roster_grants` computes every MCP server row's `reachableBy`. tinyhumansai#661 gave `OverlayAgent` a `tools` list and taught two of the three readers to honour it; this one still passed an empty grant, so a teammate scoped to one server reported as reaching every enabled server — the console asserting a connection the harness does not grant, which is the exact divergence tinyhumansai#264 exists to prevent. Issue tinyhumansai#740. The comment that stood here ("no manifest tools row → the company's standard grant") was true before tinyhumansai#661 and read as a deliberate decision rather than an assumption that had expired. Read-only surface, so nothing was over-granted: the harness builds the agent correctly from `overlay_agent_to_manifest`. The defect was that the operator-facing answer disagreed with it — and that an enabled, healthy server no teammate can reach could no longer be flagged as a misconfiguration for a company whose teammates are all overlay-defined.
tinyhumansai#661 gave `add_agent` a `tools` argument clamped to the company grant. That leaves tinyhumansai#619's original defect intact: omitting `tools` still yields the company's whole grant, so an agent scoped to a corner of the company can mint a teammate holding everything the company holds. `add_agent` is `Reach::Nothing` and sits in `INTRINSIC_TOOLS`, so it is always present and never asks — nothing else in the path would catch it. The ceiling is now the minter, not the company. Omitting `tools` copies the minter's own line, so an unscoped minter still mints an unscoped teammate that keeps tracking `[tools].allow` rather than freezing a copy of it. An explicit list is narrowed against the minter's effective grant, and a list that narrows to nothing is a clean tool error rather than a stored empty list — an empty list means inherit everything, so storing one would turn the most deliberate narrowing an agent can ask for into the widest grant in the company. Every mint is logged with the minter, the teammate and the resolved scope. That was the condition attached to sanctioning the narrowing: the tool never asks, so the log is the only place the decision is visible, and a narrowing nobody can observe is the defect repeated.
tinyhumansai#661 made the scope writable at creation — `POST …/team` and `add_agent`. Narrowing a teammate that already exists had no route, so correcting a grant meant deleting and recreating the teammate, which orphans its workspace folder, budget row, desk memberships and inbox. `PATCH …/team/{agent_id}` now accepts `tools`, and `editable` names it. An omitted key leaves the scope alone; `[]` is the deliberate way back to the company's standard grant, so `null` never has to mean a third thing. A blank glob is a 400 — `""` matches nothing an operator meant and would read as a scope that grants nothing while looking like a scope that was set. Globs are stored verbatim, exactly like a manifest line: the `[tools].allow` ceiling is applied at read time, so a glob the company does not cover surfaces as asked-for-but-not-granted rather than vanishing on save. A manifest teammate stays a 409 — its line lives in the version-controlled `company.toml`.
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.0748 · 68,341 in / 23,311 out · 51,910 cached (76%) · z-ai/glm-5.2
critique: $0.0317 · 19,367 in / 11,217 out · 15,358 cached (79%) · z-ai/glm-5.2
security: $0.0167 · 19,262 in / 4,887 out · 15,290 cached (79%) · z-ai/glm-5.2
tests: $0.0147 · 13,441 in / 4,455 out · 9,799 cached (73%) · z-ai/glm-5.2
description: $0.0117 · 16,271 in / 2,752 out · 11,463 cached (70%) · z-ai/glm-5.2
What this change touches5 files, +792 -43 across 2 components. It reaches 6 untouched components (60 graph nodes walked). 4 further components left out to keep the diagram readable. flowchart LR
n0["src/server/ops<br/>2 files +505 -19"]:::changed
n1["src/harness<br/>3 files +287 -24"]:::changed
n2["src/company<br/>3 files reached"]:::impacted
n3["src/harness<br/>3 files reached"]:::impacted
n4["src/ports<br/>3 files reached"]:::impacted
n5["src<br/>2 files reached"]:::impacted
n6["src/runtime<br/>2 files reached"]:::impacted
n7["frontend/src/views<br/>1 file reached"]:::impacted
n3 -->|26 refs| n4
n0 -->|19 refs| n2
n3 -->|15 refs| n2
n3 -->|10 refs| n5
n0 -->|9 refs| n5
n2 -->|8 refs| n4
n2 -->|7 refs| n5
n0 -->|5 refs| n3
n0 -->|5 refs| n7
n3 -->|5 refs| n6
n3 -->|4 refs| n7
n2 -->|2 refs| n7
n6 -->|2 refs| n2
n6 -->|2 refs| n3
n6 -->|2 refs| n4
n0 -->|1 ref| n4
n4 -->|1 ref| n2
n4 -->|1 ref| n5
n5 -->|1 ref| n2
n5 -->|1 ref| n3
n5 -->|1 ref| n4
n6 -->|1 ref| n5
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Green: changed. Grey: untouched, reached through an import or a call. Orange: has findings. Red: has a finding that blocks the merge.
Changed files
|
tinysweeper
Bot
added
the
priority: p3
oxoxDev
added
cluster:mcp
`edit_agent` is open to any signed-in member, and that is right for
`name`, `role` and `description`: defining a teammate was never
admin-only, so correcting one is not either. It does not transfer to
`tools`.
An empty `tools` list means "the company's standard grant". So
`{"tools": []}` is not a small edit — it is the widest possible
widening, and left member-open any signed-in member could hand a
deliberately-scoped teammate the company's whole grant back. That is the
same inversion `add_agent` already refuses, where a narrowing that lands
empty is a hard error precisely because an empty list inherits
everything. The invariant was enforced in one of the two places it
applies. Found in review of tinyhumansai#745.
The check is conditional on the field being present, in the same shape
and for the same reason as the cap on `add_member`: a member editing a
name or a role keeps working exactly as before, and adding this field
must not quietly take an existing capability away.
Narrow-only-for-members was considered and rejected — it makes the scope
a one-way ratchet, so a teammate scoped too tightly could never be
loosened by anyone, and the only way back would be delete-and-recreate,
which orphans the workspace folder, budget row, desk memberships and
inbox this route exists to preserve.
`editable` is now actor-dependent too. It is the host stating the rule
so the console does not re-derive it, so advertising `tools` to a member
whose save would be a 403 is exactly the drift it exists to remove. The
read resolves authority through the same `may_administer` predicate the
enforcement path uses, so the two cannot disagree.
c781297 to
18fd2b1
Compare
|
@oxoxDev — you're right, and thank you for catching it. Fixed in The finding, restated so the fix is judged against the real thing: What I changed, and why not the other optionsConditional admin on the field, in the same shape if body.tools.is_some() {
require_admin(&headers, &state, &company.runtime).await?;
}I weighed the two alternatives seriously rather than taking the easiest:
The check is conditional on purpose: a member editing a name or role keeps working exactly as before. That is the stated reason One more gap, which you did not raise
VerificationRebased onto All 10 mechanisms revert-checked — each fix removed, the named test re-run, confirmed failing. The three for this fix:
The third is there deliberately: it fails if I over-correct and take the name/role edit away from members, so the conditionality is pinned in both directions rather than just the refusal. The widening test drives the route with two accounts — the harness signs every request in as an admin, so a check verified only as an admin passes identically against no check at all. |
|
@coderabbitai review |
|
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.0929 · 102,045 in / 27,062 out · 78,207 cached (77%) · z-ai/glm-5.2
critique: $0.0513 · 39,113 in / 17,235 out · 30,976 cached (79%) · z-ai/glm-5.2
security: $0.0169 · 23,694 in / 4,325 out · 18,225 cached (77%) · z-ai/glm-5.2
tests: $0.0162 · 17,873 in / 4,701 out · 13,700 cached (77%) · z-ai/glm-5.2
description: $0.0086 · 21,365 in / 801 out · 15,306 cached (72%) · z-ai/glm-5.2
| ) -> Result<Json<AgentDetailDto>, Response> { | ||
| // Authority before the write lock: a refused edit must not hold the lock, | ||
| // and must not have looked at the record either. | ||
| if body.tools.is_some() { |
There was a problem hiding this comment.
The admin check runs before the agent exists
This check runs before the lookup that turns a non-existent id into a 404. A non-admin requesting PATCH /{jamie} with tools will get 403 Forbidden instead of 404 Not Found. That is the wrong order: a 404 is a pre-condition to any authority check. As written, an empty tools request skips the check and correctly returns 404, which is consistent — but a non-admin who does send tools is now blocked before the lookup. This also means non-admins can probe for which agent ids exist vs. which they cannot edit, by observing the status code. Reorder: resolve the record first, then run the admin check. Note that moving the lookup inside the write lock would change the lock semantics; the check can stay outside the lock, but it must come after the 404 is known. Alternatively, return 404 for missing ids before checking admin. Alternatively, this may be acceptable if your API deliberately treats id enumeration as sensitive and prefers a 403, but that would be a surprising rule. The repo-standard rule is that a resource check precedes an authority check.
[RULE] Reorder: resolve the record first, then run the admin check. ·
tinysweeper
Bot
added
priority: p2
|
@oxoxDev — re-requesting review. Your Your finding was right. The fix — if body.tools.is_some() {
require_admin(&headers, &state, &company.runtime).await?;
}Conditional on purpose: a member editing a name or role keeps working exactly as before, because adding a field must not quietly remove an existing capability. Why not the two obvious alternatives (both now recorded in the PR body so a later simplification doesn't reintroduce them):
One gap you didn't raise, also fixed: Verification — rebased onto green |
oxoxDev
left a comment
oxoxDev
left a comment
There was a problem hiding this comment.
Re-reviewed at 18fd2b13. The major is closed, and the fix goes further than what I asked for. Restoring my approval.
The enforcement is right, and placed thoughtfully:
// Authority before the write lock: a refused edit must not hold the lock,
// and must not have looked at the record either.
if body.tools.is_some() {
require_admin(&headers, &state, &company.runtime).await?;
}Gated on tools being present rather than on the route, so a member correcting a name or role is unaffected — which was the property worth preserving. Some([]) is covered, which is the case that mattered: an empty list means the company's standard grant, so the widening path is the one now behind admin. And doing the check before taking the write lock, so a refused edit neither holds the lock nor reads the record, is a detail I would not have asked for and is correct on both counts.
editable is now actor-dependent, which was my secondary point and you have implemented the whole of it rather than the letter:
a console renders a field read-only exactly when the host says it is, so offering
toolsto a member who would meet a403on save is precisely the drifteditableexists to remove
Resolving an unknown principal as not-admin — under-claiming what the caller may edit rather than over-claiming it — is the right direction for a field whose whole purpose is telling a client what it may offer.
Your framing of the axis is better than mine. I called tools a privilege boundary; you named the distinction the codebase already draws:
That reasoning covers what a teammate is. It does not cover what a teammate may do … a write that settles something on behalf of the company rather than one a member makes for themselves.
That is a rule the next person can apply to a field I have not thought of, which "this one is sensitive" is not.
0 major. Approving. Thirteen checks green, MERGEABLE / CLEAN.
The two things I verified on the earlier revision both still hold: roster_grants narrows each overlay teammate by its own line instead of hard-coding &[], and add_agent's narrow-to-empty guard is intact with its actionable error.
Before merging: the branch is 38 commits behind main. main has gone red twice today from semantic collisions between branches that were each green against their own base, and this one touches orchestrator.rs and harness/mod.rs, which several PRs have landed in since you branched. Worth the rebase and a fresh run rather than trusting the current thirteen.
Superseded — tools is now admin-only, editable is actor-dependent; approved above.
Placing the conditional admin check at the top of `edit_agent` made one
route give two answers about whether a teammate exists: `{"name": "x"}`
on an unknown id returned 404, while `{"tools": […]}` on the same id
returned 403. An unrelated field decided whether the thing was there.
Found in review of tinyhumansai#745; introduced by the admin check itself.
The ordering is forced rather than preferred. The non-`tools` path
cannot be moved to match — a name edit is member-open and has no
authority check to run first — so authorising after existence is the
only order in which the two agree.
The usual reason to authorise first, refusing to confirm a resource
exists, does not apply here: `GET {scope}/team/{agent_id}` is open to
any signed-in member and already 404s on an unknown id, so 403-before-
404 would hide nothing from the caller it inconveniences.
Deliberately unlike `set_budget`, which authorises first: that route is
admin-only in full, so admin-first is self-consistent there. This one is
admin-only per field, which is what makes the position load-bearing.
The test pins the invariant rather than the choice — an unknown id must
answer the same way whether or not `tools` is present — and is driven as
a member, the only actor for whom the two orderings differ.
|
@tinysweeper — good catch, fixed in The internal inconsistency was the decisive part, as you framed it: same route, same non-existent id, and The ordering turns out to be forced, not preferred. The non- I weighed the counter-argument (403-first avoids leaking existence to an unauthorised caller) and it does not apply here: Deliberately unlike The test pins the invariant, not the choice — an unknown id answers the same way whether or not Revert-checked, and worth noting because my first attempt was worthless: I initially re-inserted the check in a position that was still after the That reproduces your exact Lanes: |
|
@coderabbitai review |
|
tinysweeper
Bot
added
priority: p3
|
@tinysweeper — your finding on What happened: I pushed the fix ( Verified after the fact: So the #764 cherry-picks that commit onto current How the escape was caught: re-running this area's tests on Apologies for the misleading reply above — it was accurate about what I had pushed and wrong about what had merged, and those are not the same fact. |
Placing the conditional admin check at the top of `edit_agent` made one
route give two answers about whether a teammate exists: `{"name": "x"}`
on an unknown id returned 404, while `{"tools": […]}` on the same id
returned 403. An unrelated field decided whether the thing was there.
Found in review of #745; introduced by the admin check itself.
The ordering is forced rather than preferred. The non-`tools` path
cannot be moved to match — a name edit is member-open and has no
authority check to run first — so authorising after existence is the
only order in which the two agree.
The usual reason to authorise first, refusing to confirm a resource
exists, does not apply here: `GET {scope}/team/{agent_id}` is open to
any signed-in member and already 404s on an unknown id, so 403-before-
404 would hide nothing from the caller it inconveniences.
Deliberately unlike `set_budget`, which authorises first: that route is
admin-only in full, so admin-first is self-consistent there. This one is
admin-only per field, which is what makes the position load-bearing.
The test pins the invariant rather than the choice — an unknown id must
answer the same way whether or not `tools` is present — and is driven as
a member, the only actor for whom the two orderings differ.
3 participants
Summary
Closes #740. Completes the remaining half of #619 — referenced deliberately without a closing keyword; see Issue linkage below.
This builds on #661 (PR #713), which landed the foundation:
OverlayAgent.tools, theoverlay_fingerprinthash and its test, the harness carry throughoverlay_agent_to_manifest, the team-detail read, and scope-at-creation viaPOST …/teamandadd_agent. None of that is re-done here.Three things that foundation left open:
1. The third reader — a live defect on
main(#740)roster_grantsinsrc/server/ops/mcp.rscomputes every MCP server row'sreachableBy. #661 taught two of the three readers to honour the new field; this one still passed an empty grant:So a teammate scoped to one server reported as reaching every enabled server — the console asserting a connection the harness does not grant, which is exactly the divergence #264 exists to prevent and which #661 was careful about in the other two readers.
The comment that stood there ("no manifest tools row → the company's standard grant") was true before #661 and read as a deliberate decision rather than an assumption that had expired — which is the likeliest reason it was missed.
Read-only surface, so nothing was over-granted: the harness builds the agent correctly. The defect was that the operator-facing answer disagreed with it. It also silently degrades a second signal —
reachableByis what flags an enabled, healthy server no teammate can reach as a probable misconfiguration, and with every overlay teammate reporting universal reach that check can no longer fire for a company whose teammates are all overlay-defined.2.
add_agentwas bounded by the company, not by the minter#661 clamps an explicit
toolsargument to the company grant, and its own comment says so: "this can only narrow the new teammate below the company grant." That leaves #619's original defect intact — omittingtoolsstill yields the company's entire grant, so an agent scoped to a corner of the company can mint a teammate holding everything the company holds.add_agentisReach::Nothingand sits inINTRINSIC_TOOLS, so it is always present and never asks; nothing else in the path would catch it.The ceiling is now the minter:
toolscopies the minter's own line. Copying the line rather than its resolved grant is deliberate — an unscoped minter still mints an unscoped teammate that keeps tracking[tools].allow, instead of freezing today's allow-list into the record as an explicit scope a later company-wide narrowing would not reach.3. The mint is now observable
Every mint logs the minter, the teammate and the resolved scope, naming inheritance explicitly when the grant is inherited. This was the condition attached to sanctioning the narrowing at all: the tool never asks, so the log is the only place the decision is visible, and a narrowing nobody can observe is the defect repeated one layer down.
4. A teammate can be narrowed after it exists
#661 made the scope writable at creation. Correcting one afterwards had no route, so it meant deleting and recreating the teammate — which orphans its workspace folder, budget row, desk memberships and inbox.
PATCH …/team/{agent_id}now acceptstools, andeditablenames it. An omitted key leaves the scope alone;[]is the deliberate way back to the standard grant, sonullnever has to mean a third thing. A blank glob is a400—""matches nothing an operator meant and would read as a scope that grants nothing while looking like a scope that was set. Globs are stored verbatim, exactly like a manifest line, so theallowceiling applies at read time and an uncovered glob surfaces as asked-for-but-not-granted rather than vanishing on save. A manifest teammate stays a409.5. Why
toolsis the one admin-only field on that routeFound in review by @oxoxDev, and it was the same inversion this PR exists to close — reintroduced through the path added to fix it.
edit_agentis open to any signed-in member, deliberately: defining a teammate was never admin-only, so correcting one is not either. That reasoning covers what a teammate is. It does not cover what a teammate may do.The sharp edge is that an empty
toolslist means "the company's standard grant" — the widest grant the company has. So{"tools": []}is not a small edit, it is the widest possible widening, and left member-open any signed-in member could hand a deliberately-scoped teammate the whole company grant back with one call.The honest description of the defect: the invariant existed in my head and was enforced in only one of the two places I wrote it.
add_agentalready refuses its own version — a narrowing that lands empty is a hard error there, precisely because an empty list inherits everything — andedit_agent, one file away, appliedtoolswith no check at all.The check is therefore conditional on the field being present, in the same shape and for the same reason as the budget cap on
add_member: a member editing a name or a role keeps working exactly as before, because adding a field must not quietly take an existing capability away. Matching that existing pattern is deliberate, so asymmetric authority on one field reads as an established shape rather than a special case.Two other designs were considered and rejected. Both are recorded here so a later "simplification" does not reintroduce them:
add_agent's guard directly. The most attractive option, since two enforcement sites are exactly how this gap appeared. It does not transfer, for a reason sharper than the layering: the two empties mean different things. Inadd_agentan empty set is the accidental residue of an intersection, which is why it must be refused. Inedit_agent,[]is a deliberate, documented request for the standard grant. Collapsing them would either forbid a legitimate operation or introduce a rule that reads as arbitrary. What genuinely transfers is the principle — an empty list is a widening — and the authority check is where that principle belongs. (add_agentalso clamps to the minting agent's grant;edit_agenthas no minting agent, the actor is a person.)toolswhen the new set is a subset of the current effective grant). This makes the scope a one-way ratchet: a teammate scoped too tightly could never be loosened by anyone, and the only route back is delete-and-recreate — which orphans the workspace folder, budget row, desk memberships and inbox that this very route exists to preserve. A concrete harm, not a preference.editableis actor-dependent for the same reason (a gap not raised in review): it is the host stating the rule so the console does not re-derive it, so advertisingtoolsto a member whose save is a403is exactly the drift that list exists to remove. It resolves authority through the samemay_administerpredicate as the enforcement path, so the two cannot disagree.API Or Behavior Changes
reachableBynow reflects a scoped overlay teammate's real grant instead of always reporting the company's. Bug fix; an unscoped teammate is unchanged.add_agent's default scope is the minter's line, not the company's. For an unscoped minter — the overwhelmingly common case, and every company before workflows: authoring & validation gaps let broken graphs (incl. shipped seeds) save clean and fail silently #661 — behaviour is identical. It changes only for a scoped minter, which is the fix. Atoolsrequest outside the minter's grant is now narrowed, and one entirely outside it is a tool error.PATCH …/team/{agentId}acceptstools— admin-only, conditionally: an edit carryingtoolsrequires admin, one carrying onlyname/role/descriptionstays open to any member exactly as before.editablenames"tools"for an admin and omits it for a member. Manifest teammates still409.summarizeGrants().standardGrantrendering already distinguishes inherited from explicit, and now tells the truth for overlay teammates on every surface. Zero frontend files changed.Tests
Ten tests added or changed.
Rebased onto green
main(d1d01ae5), so nothing below is inherited:Every new test was revert-checked — its own fix removed, the named test re-run, confirmed to fail. Counts quoted so a run that silently executed nothing cannot pass for a pass:
mcpreads&[]againa_scoped_overlay_teammate_does_not_read_back_as_reaching_everythingadd_agentdefaults toVec::new()a_minted_teammate_is_bounded_by_its_minter_not_the_company["*"]an_explicit_scope_is_narrowed_to_what_the_minter_holdsa_scope_entirely_outside_the_minters_grant_is_refusedOVERLAY_EDITABLEback to 3the_host_states_which_fields_are_editablean_overlay_teammate_can_be_scoped_after_creationtrimmed_globsnever refusesa_blank_tool_glob_is_refusedrequire_adminguard removeda_member_cannot_widen_a_teammates_scopeeditablemade actor-independenteditable_names_tools_only_for_an_admina_member_may_still_edit_a_teammates_name_and_roleThe last row is deliberate: it pins the conditionality in both directions, so over-correcting into a blanket admin check — silently taking the name/role edit away from members — fails too. The widening test drives the route with two accounts; the harness signs every request in as an admin, so a check verified only as an admin passes identically against no check at all.
Not claimed as covered: the
tracing::info!observability has no revert-check. Nothing in the suite asserts tracing output, so I cannot prove that line fails when removed, and it is not in the table above.One of #661's own tests needed a fixture fix, and it is worth saying why rather than leaving it to look like a weakened assertion.
add_agent_tool_persists_a_tool_grantbegan failing under the new minter ceiling because my test helper modelled an "unscoped" minter with an arbitrary narrow grant (["fs:*", "web:*"]) — which is incoherent: an unscoped minter's effective grant is whatever the company allows, not some third list. The helper now passes["*"], whichgrant_matchesshort-circuits to true, so an unscoped minter has a genuinely open ceiling and a test about other behaviour is not accidentally a test about the #619 clamp.That distinction matters and would not be obvious on review: the fix is not "loosen the fixture until it passes", it is "model the thing the fixture claims to model". No assertion of #661's was weakened — and the two control tests (
add_agent_tool_empty_tools_is_the_standard_grant,an_unscoped_minter_mints_an_unscoped_teammate) pin that the common path is unchanged.cargo fmt --all -- --checkcargo clippy --all-targets -- -D warnings— run ascargo clippy --locked --no-deps --features openhuman,tinycortex --all-targets -- -D warnings;--no-depskeeps it off pre-existingvendor/tinyagentslints CI never seescargo build --all-targets— N/A — covered bycargo check --locked --all-features --all-targets, which is strictly wider (it compiles themongodb/sqliterow-builtCompanyRecordpaths the default feature set never reaches). Deliberately not run as a separate full build: the machine is at 31Gi free and a redundant build is ~15G oftarget/.cargo test— run ascargo test --features openhuman,tinycortexDocumentation
No doc change.
docs/spec/runtime/api.md's team section was updated by #661 to describe the field, the empty-means-inherit rule and scope-at-creation, and all of that remains accurate. This PR adds no new concept to document — it makes an existing documented rule true on a third reader, tightens a ceiling the docs never claimed was the company's, and adds aPATCHfield to a route whose patch semantics are already described.Issue linkage
Closes #740— the MCP third reader is entirely fixed here.add_agentmay set) plus the two gaps workflows: authoring & validation gaps let broken graphs (incl. shipped seeds) save clean and fail silently #661 left. Together they close it — but the foundation is someone else's work, so a maintainer should close it rather than have this PR auto-close an issue it only finishes.Supersedes #717
#717 was the original #619 build, opened before #661 merged. Roughly 60% of it is now duplicate. It is being closed rather than rebased: honest conflict resolution would mean taking
main's side on most of fifteen files, which is hand-deleting the PR. What carried over is exactly the four items above; what was dropped is the field, the fingerprint and its test, the harness carry, and the detail read — all now onmainvia #713.Summary by CodeRabbit
New Features
Bug Fixes