feat(userspace): app_grants ledger feeds the capability broker#1404
Conversation
Per the reconciliation decision: the userspace broker stays the runtime enforcer for gated capabilities, and the per-user app_grants ledger feeds it. A capability the current user granted an app via the consent flow now authorises it on top of the per-app permissions_granted set. The merge is additive and best-effort: with no auth session or no ledger the broker falls back to the per-app granted set, so existing behaviour is unchanged. First-party apps keep their pre-authorised all-gated set. Init the lifespan-owned app_grants store in the two test fixtures that bypass the lifespan so the broker can consult it. New tests cover ledger-granted authorisation and ledger-absent denial.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
📝 WalkthroughWalkthroughThe broker endpoint for community (non-first-party) apps now unions per-user capability grants from an optional ChangesPer-user app_grants broker authorization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| # The userspace broker stays the runtime enforcer; the app_grants ledger | ||
| # feeds it (decision 24). A capability the current user granted this app | ||
| # via the consent flow also authorises it, on top of the per-app granted | ||
| # set. Additive and best-effort: no auth session or no ledger falls back | ||
| # to the per-app set, so nothing that worked before changes. | ||
| granted = set(app["permissions_granted"]) | ||
| uid = getattr(request.state, "user_id", None) | ||
| grants_store = getattr(request.app.state, "app_grants", None) | ||
| if uid and grants_store is not None: | ||
| granted |= await grants_store.granted_capabilities(uid, app_id) |
There was a problem hiding this comment.
💡 Edge Case: Ledger merge isn't truly best-effort; store errors propagate
The comment describes the merge as "additive and best-effort: no auth session or no ledger falls back to the per-app set, so nothing that worked before changes." In practice two of those fallbacks don't behave as described:
grants_storeisgetattr(request.app.state, 'app_grants', None), butapp_grantsis unconditionally assigned increate_app(tinyagentos/app.py), so it is effectively neverNonein production. The "no ledger" fallback therefore only triggers whenuidis falsy.- The broker route is auth-protected (not in
AuthMiddlewareexempt paths), souidis normally set. Theawait grants_store.granted_capabilities(uid, app_id)call is unguarded: if the store is uninitialized (granted_capabilitiesraisesRuntimeErrorwheninit()wasn't called) or the underlying query errors, the exception propagates and turns a previously-working broker call into a 500. That contradicts the "nothing that worked before changes" guarantee.
Production inits the store via the lifespan, so the practical risk is low, but a future client/test fixture that hits the broker without calling app_grants.init() will get a 500 instead of the intended per-app fallback. Consider wrapping the ledger lookup in a try/except (log + fall back to the per-app set) to make the merge genuinely best-effort.
Make the ledger merge genuinely best-effort by falling back to the per-app granted set if the store lookup fails.:
granted = set(app["permissions_granted"])
uid = getattr(request.state, "user_id", None)
grants_store = getattr(request.app.state, "app_grants", None)
if uid and grants_store is not None:
try:
granted |= await grants_store.granted_capabilities(uid, app_id)
except Exception:
logger.warning("app_grants lookup failed; falling back to per-app set", exc_info=True)
Was this helpful? React with 👍 / 👎
|
Note Your trial team has used its Gitar budget, so automatic reviews are paused. Upgrade now to unlock full capacity. Comment "Gitar review" to trigger a review manually. Code Review 👍 Approved with suggestions 0 resolved / 1 findingsImplements the app_grants ledger as an additive capability source for the broker. Wrap the ledger retrieval in a try-except block to ensure store errors propagate as intended for a best-effort fallback. 💡 Edge Case: Ledger merge isn't truly best-effort; store errors propagate📄 tinyagentos/routes/userspace_apps.py:250-259 The comment describes the merge as "additive and best-effort: no auth session or no ledger falls back to the per-app set, so nothing that worked before changes." In practice two of those fallbacks don't behave as described:
Production inits the store via the lifespan, so the practical risk is low, but a future client/test fixture that hits the broker without calling Make the ledger merge genuinely best-effort by falling back to the per-app granted set if the store lookup fails.🤖 Prompt for agentsOptionsDisplay: compact → Showing less information. Comment with these commands to change:
Important Your trial ends in 4 days — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more. Was this helpful? React with 👍 / 👎 | Gitar |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/test_routes_userspace_apps.py (1)
570-608: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd one regression test for the “ledger unavailable” fallback path.
These tests validate the positive/negative ledger-decision path, but not the contract that broker behavior falls back cleanly when
app_grantsis unavailable/uninitialized.🧪 Suggested test shape
`@pytest.mark.asyncio` async def test_broker_gated_cap_falls_back_when_app_grants_unavailable(client): app = client._transport.app await _init_userspace_stores(app, app.state.data_dir) store = app.state.userspace_apps await _install_test_app(store, permissions=["app.net"]) await store.set_permissions_granted("test-app", ["app.net"]) # Simulate unavailable ledger in lifespan-bypassed context. await app.state.app_grants.close() resp = await client.post( "/api/userspace-apps/test-app/broker", json={"capability": "app.net.fetch", "args": {"url": "http://example.com"}}, ) assert resp.status_code == 200 assert resp.json().get("error") != "permission_denied"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_routes_userspace_apps.py` around lines 570 - 608, Add a regression test for the broker fallback when app_grants is unavailable/uninitialized. In the userspace app broker tests near test_broker_gated_cap_allowed_via_app_grants_ledger, add a new async test that installs the app, grants the per-app permission, then closes or otherwise disables app.state.app_grants before calling the /api/userspace-apps/test-app/broker endpoint. Assert the broker still returns 200 and does not produce permission_denied, using the same app.state.userspace_apps, _install_test_app, and app.state.app_grants symbols to keep the test aligned with the existing ledger-path coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/conftest.py`:
- Around line 333-335: The test fixture initializes app_grants but never
releases it, so add a matching teardown step in the same fixture to close
app.state.app_grants after the test body completes. Locate the setup around
app.state.app_grants.init() and ensure the fixture cleanup path awaits the
corresponding close method on app.state.app_grants so SQLite handles are not
leaked and locking stays stable across tests.
In `@tests/userspace/conftest.py`:
- Around line 54-56: The userspace fixture initializes app_grants in setup but
never tears it down, so mirror the setup in the fixture cleanup by closing
app.state.app_grants during teardown. Update the conftest.py fixture that calls
app.state.app_grants.init() so it also performs the matching close on the same
app_grants lifecycle-managed object, keeping resource management consistent.
In `@tinyagentos/routes/userspace_apps.py`:
- Around line 257-259: The best-effort authorization path in the userspace app
handler can still raise a 500 when app_grants exists but has not been
initialized. Update the grant lookup around grants_store and
granted_capabilities() to verify the store is ready before calling it, or safely
skip/handle the call when initialization is incomplete so the fallback grant
calculation remains additive and broker requests do not fail unexpectedly.
---
Nitpick comments:
In `@tests/test_routes_userspace_apps.py`:
- Around line 570-608: Add a regression test for the broker fallback when
app_grants is unavailable/uninitialized. In the userspace app broker tests near
test_broker_gated_cap_allowed_via_app_grants_ledger, add a new async test that
installs the app, grants the per-app permission, then closes or otherwise
disables app.state.app_grants before calling the
/api/userspace-apps/test-app/broker endpoint. Assert the broker still returns
200 and does not produce permission_denied, using the same
app.state.userspace_apps, _install_test_app, and app.state.app_grants symbols to
keep the test aligned with the existing ledger-path coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 45617c49-8841-4432-9e26-93123a091bad
📒 Files selected for processing (4)
tests/conftest.pytests/test_routes_userspace_apps.pytests/userspace/conftest.pytinyagentos/routes/userspace_apps.py
| # app_grants ledger (per-app capability grants) is lifespan-owned; tests that | ||
| # bypass the lifespan must init it so the userspace broker can consult it. | ||
| await app.state.app_grants.init() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Close app.state.app_grants in fixture teardown.
This fixture now initializes app_grants, but teardown does not close it. Add a matching close to avoid leaked SQLite handles and flaky test locking.
🔧 Proposed fix
await office_docs.close()
await feedback_store.close()
+ await app.state.app_grants.close()
await app.state.qmd_client.close()
await app.state.http_client.aclose()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/conftest.py` around lines 333 - 335, The test fixture initializes
app_grants but never releases it, so add a matching teardown step in the same
fixture to close app.state.app_grants after the test body completes. Locate the
setup around app.state.app_grants.init() and ensure the fixture cleanup path
awaits the corresponding close method on app.state.app_grants so SQLite handles
are not leaked and locking stays stable across tests.
| # app_grants ledger is lifespan-owned; init it so the broker can consult it. | ||
| await app.state.app_grants.init() | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Mirror app_grants.init() with teardown close.
app_grants is now initialized in setup but never closed in teardown. Please close it to keep fixture resource management consistent.
🔧 Proposed fix
await userspace_data.close()
await userspace_apps.close()
+ await app.state.app_grants.close()
await app.state.secrets.close()
await app.state.qmd_client.close()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/userspace/conftest.py` around lines 54 - 56, The userspace fixture
initializes app_grants in setup but never tears it down, so mirror the setup in
the fixture cleanup by closing app.state.app_grants during teardown. Update the
conftest.py fixture that calls app.state.app_grants.init() so it also performs
the matching close on the same app_grants lifecycle-managed object, keeping
resource management consistent.
| grants_store = getattr(request.app.state, "app_grants", None) | ||
| if uid and grants_store is not None: | ||
| granted |= await grants_store.granted_capabilities(uid, app_id) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Uninitialized app_grants can turn best-effort authorization into a 500.
The current guard only checks for object presence, but granted_capabilities() raises when the store exists but is not initialized. That violates the additive fallback behavior and can fail broker calls unexpectedly.
🔧 Proposed fix
uid = getattr(request.state, "user_id", None)
grants_store = getattr(request.app.state, "app_grants", None)
if uid and grants_store is not None:
- granted |= await grants_store.granted_capabilities(uid, app_id)
+ try:
+ granted |= await grants_store.granted_capabilities(uid, app_id)
+ except RuntimeError:
+ # Best-effort path: preserve legacy per-app grants when
+ # the per-user ledger is unavailable/uninitialised.
+ pass📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| grants_store = getattr(request.app.state, "app_grants", None) | |
| if uid and grants_store is not None: | |
| granted |= await grants_store.granted_capabilities(uid, app_id) | |
| uid = getattr(request.state, "user_id", None) | |
| grants_store = getattr(request.app.state, "app_grants", None) | |
| if uid and grants_store is not None: | |
| try: | |
| granted |= await grants_store.granted_capabilities(uid, app_id) | |
| except RuntimeError: | |
| # Best-effort path: preserve legacy per-app grants when | |
| # the per-user ledger is unavailable/uninitialised. | |
| pass |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tinyagentos/routes/userspace_apps.py` around lines 257 - 259, The best-effort
authorization path in the userspace app handler can still raise a 500 when
app_grants exists but has not been initialized. Update the grant lookup around
grants_store and granted_capabilities() to verify the store is ready before
calling it, or safely skip/handle the call when initialization is incomplete so
the fallback grant calculation remains additive and broker requests do not fail
unexpectedly.
#1404 (broker best-effort): the app_grants ledger lookup in the userspace broker route was unguarded, so an uninitialised store or a query error turned a previously-working broker call into a 500, contradicting the best-effort comment. Wrap it in try/except that logs and falls back to the per-app granted set. #1405 (network origin validation): the grant API accepted any network: prefix, including an empty or malformed origin, so a typo'd origin produced a ledger row. Centralise the strict origin pattern as NET_ORIGIN_RE in capabilities.py (single source of truth, the package parser now reuses it) plus a is_valid_network_grant helper, and reject malformed network grants at the API with a 400. Tests: broker falls back instead of 500 on a raising ledger; malformed network origins rejected; package parser still validates origins via the shared pattern. 203 passed across the userspace + app-perms suites.
…1429) Backend for the app permission consent flow (#56), per Jay's answers (forks 31-34): - Decisions gain an optional metadata JSON column (guarded ALTER migration mirroring board_audit; in _JSON_FIELDS; create() passthrough; DecisionIn field). - capabilities.py: CAPABILITY_DESCRIPTIONS (one human line per known cap) + describe_capability() handling network:<origin> and unknowns. - app_permissions.app_grant_decision_payload(): builds the multi_select consent card (options = requested caps, metadata.kind = app_grant). Not wired into install yet (live-verify session). - answer route: _apply_app_grant side-effect writes granted (selected) / denied (rest) per capability to the existing app_grants ledger, best-effort. Reuses the existing AppGrantsStore (app.state.app_grants, #1404/#1405). Tests: metadata round-trip, metadata echo, app_grant answer writes grants, payload builder, full capability-description coverage. 61 related tests pass.
1 participant
Implements the app-permissions reconciliation (broker enforces, app_grants is the ledger feeding it).
What
The userspace broker stays the runtime enforcer for gated capabilities (app.net/agent/llm/memory). The per-user
app_grantsledger (built earlier this session) now feeds it: a capability the current user granted an app via the consent flow authorises it, on top of the per-apppermissions_grantedset.Why this shape
One vocabulary, one enforcer. The ledger records who-granted-what-when per (user, app, capability); the broker reads it at call time. The merge is additive and best-effort: with no auth session or no ledger, the broker falls back to the per-app granted set, so nothing that worked before changes. First-party apps keep their pre-authorised all-gated set.
Changes
routes/userspace_apps.py: broker route unions the per-user ledger grants intograntedfor community apps (readsrequest.state.user_idoptionally, no new 401).tests/conftest.py+tests/userspace/conftest.py: init the lifespan-ownedapp_grantsstore (these fixtures bypass the lifespan; production inits it in app.py).Tests
159 passed across userspace + app_grants + app_permissions suites.
Summary by CodeRabbit
New Features
Tests