Skip to content

feat(trust): scoped tokens — kb:read / kb:propose / kb:approve / kb:admin - #692

Closed
minion1227 wants to merge 5 commits into
vouchdev:testfrom
minion1227:feat/scoped-tokens
Closed

feat(trust): scoped tokens — kb:read / kb:propose / kb:approve / kb:admin#692
minion1227 wants to merge 5 commits into
vouchdev:testfrom
minion1227:feat/scoped-tokens

Conversation

@minion1227

Copy link
Copy Markdown
Contributor

Closes #608

Stacked on #669 — merge that first. The registry is where scopes are stored, so this branch is based on it. Until #669 lands the diff below also shows agents.py / test_agents.py; it shrinks to scopes.py, trust.py, jsonl_server.py, http_server.py, capabilities.py, models.py and test_scopes.py the moment it does.

a bearer token was all-or-nothing: hold it and you could call all 73 kb.* methods, kb.approve included. fine for a solo human, wrong for a CI job that should only read or a triage bot that should only propose. the config-level trusted-agent flag can only widen the gate — this is the first thing in vouch that can narrow it.

withholding kb:approve by default is the review gate, expressed as a credential.

$ vouch agents register ci-bot --subject 29e74dc6 --scope kb:read --scope kb:propose

# that credential, over http:
kb.search        -> ok
kb.propose_claim -> ok
kb.approve       -> permission_denied: kb.approve requires kb:approve;
                    this credential holds kb:read, kb:propose

four coarse scopes, not a per-method allowlist

kb:read / kb:propose / kb:approve / kb:admin. a per-method grammar is more flexible and makes every new kb.* method a config migration for every deployment; four buckets keep that cost at zero.

the two rules that make this safe to ship

an unscoped credential means all scopes. every token issued before this keeps working exactly as it did. an empty scope set is unrestricted, never denied — the opposite choice breaks every deployment on upgrade. test_unscoped_permits_everything walks all 73 methods.

every method must be classified. METHOD_SCOPES is exhaustive over capabilities.METHODS, enforced both ways: test_every_method_is_classified catches a new method with no scope, and test_the_table_has_no_entries_for_methods_that_do_not_exist catches a stale entry. an unclassified method is denied to a scoped caller — fails closed, because a deny-list in a trust-centric system fails open.

one implementation, three surfaces

enforcement is trust.require_scope, called from the two dispatch points that already exist: handle_request for JSONL/HTTP and wrap_tool_fn for MCP. the check runs before the handler, so a refused call cannot have side effects on its way to being refused — test_denial_happens_before_the_handler_runs spies on the handler and asserts it never ran.

a couple of details worth checking:

  • a wrong token never reaches the scope check, so it cannot probe the table.
  • an unknown method still reports method_not_found, not permission_denied — scoping must not turn a typo into a permissions puzzle.
  • _meta.vouch_trust only grows a scopes key when the credential is actually scoped, so unscoped callers see the byte-identical block they saw before.

discovery, not trial and error

kb.capabilities reports the caller's effective scopes and the exact method list they may call, so an agent finds out what it may do up front instead of failing method by method.

note on the VEP question

the issue says this "probably wants a VEP" as a surface change. i think the additive shape avoids needing one — kb.capabilities gains a field, _meta.vouch_trust gains an optional key, and unscoped behaviour is unchanged — but that is your call, and i would rather raise it than assume. happy to write one if you want the surface change recorded.

scope enforcement is here; #609's per-KB credential binding is not, and scopes stays plain recorded metadata on the registry row for anything else to build on.

tests

46 cases in tests/test_scopes.py: the exhaustiveness guards, representative methods per bucket, permits including the unscoped and unclassified paths, parse_scopes normalisation and its refusal of a typo'd scope, enforcement across all three credential shapes on the JSONL dispatch, the sync and async MCP wrappers, the trust block, kb.capabilities, and the MCP-over-HTTP ASGI middleware resolving registry scopes onto the request.

verification

pytest tests/ -q --ignore=tests/embeddings   green
mypy src                                     Success: no issues found in 121 source files
ruff check src tests                         All checks passed!
diff-cover --fail-under 100                  100%

…dmin

a bearer token was all-or-nothing: hold it and you could call all 73 kb.*
methods, kb.approve included. fine for a solo human, wrong for a ci job that
should only read or a triage bot that should only propose. the config-level
trusted-agent flag can only widen the gate; this is the first thing in vouch
that can narrow it — withholding kb:approve by default *is* the review gate,
expressed as a credential.

four coarse scopes over the method list rather than a per-method allowlist,
because a per-method grammar makes every new kb.* method a config migration
for every deployment.

two rules keep this safe to ship into existing deployments. an unscoped
credential means all scopes, so every token issued before this keeps working
exactly as it did — an empty scope set is "unrestricted", never "denied".
and every method must be classified: METHOD_SCOPES is exhaustive over
capabilities.METHODS with a test enforcing it, so a newly-added method cannot
silently land unreachable for scoped callers. an unclassified method is denied
to a scoped caller — fails closed, because a deny-list in a trust-centric
system fails open.

enforcement lives at the two dispatch points that already exist:
handle_request for jsonl and http, and wrap_tool_fn for mcp, both routed
through trust.require_scope so the three surfaces inherit one implementation.
the check runs before the handler, so a refused call cannot have side effects
on its way to being refused.

the agent registry supplies the scopes: a registered subject's scopes ride
onto VouchTrust at the http chokepoint, and an unregistered subject stays
unscoped. registration now validates scopes, so a typo cannot mint a
credential with powers nobody asked for.

kb.capabilities reports the caller's effective scopes and allowed methods, so
an agent discovers what it may do instead of failing method by method. the
trust block only grows a scopes key when the credential is actually scoped.

stacked on vouchdev#607: the registry is where scopes are stored.

Closes vouchdev#608
@minion1227
minion1227 requested a review from plind-junior as a code owner July 30, 2026 22:08
@github-actions github-actions Bot added docs documentation, specs, examples, and repo guidance cli command line interface mcp mcp, jsonl, and http surfaces storage kb storage, migrations, schemas, and proposals tests tests and fixtures size: XL 1000 or more changed non-doc lines labels Jul 30, 2026
the schema drift check compares schemas/ against what
scripts/gen_schemas.py emits from models.py. adding the per-credential
scopes block to Capabilities changed that output, so the committed
schema went stale.

this is the sixth registration site a new field can miss — CLAUDE.md's
four, plus hot_memory's coverage map, plus this. worth a line in the
contributor notes separately.
@github-actions github-actions Bot added the schemas json schemas and generated schema assets label Jul 30, 2026
@plind-junior

Copy link
Copy Markdown
Member

the two safety rules are the right two, and the direction on both is correct. an unscoped credential meaning all scopes is the only choice that does not break every deployment on upgrade, and an unclassified method being denied to a scoped caller is the only choice that fails closed — getting either backwards would be the bug that matters here, and test_unscoped_permits_everything walking all 73 methods plus the two-way exhaustiveness guard on METHOD_SCOPES pin both.

test_denial_happens_before_the_handler_runs is the test i would have asked for if it were not already there. a scope check that runs after the handler is not a scope check, and spying on the handler to prove it never ran is the only way to know.

the two small distinctions are also right and easy to get wrong: an unknown method still reporting method_not_found rather than permission_denied (scoping must not turn a typo into a permissions puzzle), and a wrong token never reaching the scope table so it cannot be used to probe it.

housekeeping — #669 merged this morning (05:47Z), so the stacking note at the top of the description is now stale and the diff needs a rebase. agents.py (+377) and test_agents.py (+481) are already on test, which is about 858 of the lines currently shown here. worth rebasing before review so what is left is the ~750 lines this PR is actually asking for.

on the VEP question you raise yourself: i think you are right that the additive shape avoids needing one — kb.capabilities gains a field, _meta.vouch_trust gains an optional key only when the credential is scoped, and unscoped behaviour is byte-identical. but raising it rather than assuming was the correct move, and it is a maintainer call rather than mine.

@github-actions github-actions Bot added size: L 500-999 changed non-doc lines and removed docs documentation, specs, examples, and repo guidance size: XL 1000 or more changed non-doc lines labels Jul 31, 2026
@plind-junior

Copy link
Copy Markdown
Member

closing this one reluctantly — the design is right and i want it back, but METHOD_SCOPES is stale by four methods and merging it as-is would silently lock them.

test now has 77 kb.* methods; this branch classifies 73. the four with no entry:

kb.capture_correction   (#679, merged 2026-07-31)
kb.list_goals           (#676)
kb.propose_goal         (#676)
kb.set_goal_status      (#676)

by this PR's own rule — "an unclassified method is denied to a scoped caller — fails closed" — every scoped credential would lose access to goal proposals and correction capture the moment this lands. that is the correct failure direction, but it is still a regression, and it is invisible from the diff.

your own test_every_method_is_classified is what catches it, which is the strongest possible argument for the guard you built. i rebased the branch onto current test to check, and all three matrix jobs go red on exactly those assertions:

E  AssertionError: assert 73 == 77

i also merged this into a local integration alongside the other eleven open PRs and ran the full suite — the eleven pass together; adding this one is what breaks it. so the objection is specific to the staleness, not to any interaction with the rest.

everything else i said stands. an unscoped credential meaning all scopes is the only choice that does not break deployments on upgrade; an unclassified method failing closed is the only safe direction for a deny-list; test_denial_happens_before_the_handler_runs is the test i would have asked for if it were not already there; and keeping method_not_found distinct from permission_denied is a real usability call most people get wrong. the #669 rebase also collapsed the diff to ~755 lines, which is now a reviewable size.

to bring it back: rebase onto test, add the four entries to METHOD_SCOPES (kb:propose for the three goal writes and kb:propose for kb.capture_correction would be my read, but that is your call), and reopen. i will merge it on green — no further review needed on the substance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cli command line interface mcp mcp, jsonl, and http surfaces schemas json schemas and generated schema assets size: L 500-999 changed non-doc lines storage kb storage, migrations, schemas, and proposals tests tests and fixtures

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(trust): scoped tokens — kb:read / kb:propose / kb:approve on a credential

2 participants