Repository navigation
fix(mobile): show a no-access state for an organization the reader lost - #8493
Conversation
The Profile agent rows resolved a stored organization that the settled membership list no longer contains to the personal scope, so the Security Agent landing offered its personal first-run GitHub setup under a lost organization's name. Entering that organization's scope directly showed only a generic load error. - Keep the stored organization once the membership list settles, so both agent rows reach the organization scope instead of silently changing scope. - Classify organization scope access from the membership list and the tRPC error codes of the scope's own queries, so a denial outranks cached data, and render one no-access surface: access-denied or unavailable copy, Retry (which re-resolves membership) and Back to profile. - Use it in the Security Agent and Code Reviewer organization landings.
iscekic
added
drain
|
bot: Android emulator against the local dev stack, with an owned fake account and a seeded organization plus GitHub integration. The selected organization's membership was removed, the app cold-started so the membership list refetched, then Profile → Security Agent.
Unit and mounted suites cover the state precedence, the denial over cached data, the deleted-organization variant, the Retry fan-out, and the personal scope staying unaffected.
|
…cope A personal scope has no membership to resolve, so the access check must not add an active membership observer there.
iscekic
added
merge:blocked:kilobot-not-ready
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryThe only new changes since Assumptions: Files Reviewed (9 files)
Previous Review Summaries (3 snapshots, latest commit 05aa8ae)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 05aa8ae)Status: No Issues Found | Recommendation: Merge Executive SummaryThe incremental commit Assumptions: Files Reviewed (1 file)
Previous review (commit 99b6244)Status: 1 Issues Found | Recommendation: Address before merge Executive SummaryThe access classifier was correctly rewritten to trust the server's denial, but the now-unused Assumptions: Overview
Issue Details (click to expand)CRITICAL
Files Reviewed (8 files)
Fix these issues in Kilo Cloud Previous review (commit 8d49aa7)Status: 5 Issues Found | Recommendation: Address before merge Executive SummaryThe new no-access flow is well structured, but inferring "lost access" from a membership-id list that omits inherited child organizations (and admins) can replace a landing the server would still authorize. Assumptions: Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (14 files)
Reviewed by deepseek-flash-latest · Input: 148.5K · Output: 12.7K · Cached: 1.2M Review guidance: REVIEW.md from base branch |
…mbership list The membership list is not the access rule: a Kilo admin reaches any organization without a membership row, and a parent owner, admin or billing manager reaches each child organization, so a scope missing from the list can still be fully accessible. Inferring denial there hid landings the server would have served. Judge access from the scope's own query errors instead, through the shared tRPC error readers: a permission loss (FORBIDDEN, or a procedure-level UNAUTHORIZED) denies, NOT_FOUND reports the unavailable state, and a session-level UNAUTHORIZED stays an ordinary load failure. Cached data cannot outvote the denial because the error is read whether or not data is present. The permission-denial reader moves to lib/trpc-error.ts so the cache and the landing share one definition.
iscekic
removed
merge:human
Access is decided from the scope's own query errors now, so the hook that read the membership list is gone and both landings call the pure classifier.
|
bot: Re-ran the Android verification on the final head (
The earlier images in this pull request were captured on an intermediate commit. The surface is unchanged; these are the final-head captures.
|
…lost-org-access # Conflicts: # apps/mobile/src/components/security-agent/scope-entry-screen.test.ts
iscekic
removed
the
merge:human
|
bot: Re-ran the Android verification on the merged head (
Captures from that run.
|
…omits it Access is decided by the organization scope's own query errors, so a list that no longer contains the organization must not produce the no-access state for an admin or an inherited child scope.
What changed
A reader whose membership in the selected organization is gone saw a first-run GitHub setup instead of a statement that access was lost. Two paths produced it:
getProfileAgentScopesubstituted the personal scope when the settled membership list no longer contained the stored organization, so Profile → Security Agent opened the personal landing and offered "Connect GitHub to get started".Changes, all mobile:
apps/mobile/src/lib/profile-agent-navigation.ts— the stored organization is kept once the membership list settles, even when the list no longer contains it. Both agent rows now reach the organization scope, which explains the loss instead of switching product scope. Loading/refreshing still yieldsundefined(rows disabled, unchanged).apps/mobile/src/lib/organization-scope-access.ts(classifyOrganizationScopeAccess) — one shared admission, decided by the organization scope's own query errors rather than by the client's membership list: a permission loss denies,NOT_FOUNDreports the unavailable state, anything else keeps the caller's own loading or retryable-error state. The server is the authority on access (a Kilo admin reaches any organization without a membership row, and a parent owner, admin or billing manager reaches each child organization), so absence from the list is never treated as proof. The error is read from each query'serror, not fromisError, so cached data cannot outvote a denial. A personal scope is never denied. The permission-loss definition moves toapps/mobile/src/lib/trpc-error.ts(isPermissionDeniedError) and is shared with the query cache, so a session-levelUNAUTHORIZED(authRequired) stays an ordinary load failure, and shaped server errors are read too.apps/mobile/src/components/organization/scope-access-denied-screen.tsx— the shared surface:QueryErrorwith thepermissionvariant ("Access denied"), or thenot-foundvariant for a deleted organization, plus two recoveries: Retry re-resolves membership and the scope's queries in place, and Back to profile leaves the stale selection. Copy reuses the existingorganization.boundary.*andqueryErrorstrings, so no locale catalog changes.apps/mobile/src/components/security-agent/scope-entry-screen.tsx+scope-entry-render.ts— newdeniedlanding state, placed after loading and before the generic error and every setup gate. No install-state token is minted for a denied scope.apps/mobile/src/components/code-reviewer/platform-list-screen.tsx— the same admission for the Code Reviewer organization landing; Retry refetches membership and all three platform statuses.apps/mobile/src/components/platform-error-screen.tsx— accepts a caller-composed action row (the two-button recovery).States covered by the Security Agent landing: first run without an integration still gets the setup, loading still gets the skeleton, a denied scope gets the no-access surface, a session-level unauthorized response and other failures keep the retryable server-error screen, and a connected enabled organization still gets the dashboard. The Code Reviewer landing keeps the organization name as its header when the membership list still supplies it and falls back to the generic "Organization" title once it does not.
Verification
Unit and mounted suites (VM, local Postgres):
organization-scope-access,scope-entry-render,scope-entry-screen(mounted, mocked tRPC transport: denied scope instead of setup, a single denied query overriding cached permission/config data, Retry after access is restored, personal scope unaffected, session-level unauthorized staying a load failure),scope-access-denied-screen(variant selection, Retry, Back to profile destination),platform-list-screen(denied surface, title fallback, Retry fan-out, granted rows),profile-agent-navigation, and the existingprofile-screensuites. The full mobile suite passes (1259 files, 18637 tests, on the head with main merged in).tsc --noEmitclean, oxlint clean on the changed files, knip clean.End-to-end on the Android emulator against the local dev stack, with an owned fake account and a seeded organization plus GitHub integration:
Visual proof is attached in a comment on this pull request.
After merging main into the branch, the Android run was repeated on the merged head with the same fixture: the denied state appears exactly as above, and Retry after the membership was restored returns the organization landing. The merged head also keeps a regression test for the class invariant: an organization missing from the membership list alone still reaches its landing, because access is decided by the organization's own queries.
Unproven by this run: the
not-foundvariant against a soft-deleted organization (unit-tested only), and iOS (no iOS-specific code in this change; proof is on Android).Proxy answers
The owner's proxy gave these answers in the owner's place.
Answer: Fallback for both products
Confidence: 0.50 · Precedent: Fix the root cause and the class of the issue on every affected surface: "fix this for all shared services model pickers on all surfaces"; the shared getProfileAgentScope silently switches Code Reviewer to the personal scope too
Answer: QueryError permission variant + Retry, plus Back to profile
Confidence: 0.80 · Precedent: Make plans cover the happy, loading, retryable-error, non-retryable-error, and empty states: listed the four states; the brief also asks for QueryError with the permission variant plus Retry
Answer: Yes: denial overrides cached data
Confidence: 0.80 · Precedent: Fix the root cause and the class of the issue, not the single symptom: "fix class of issue preventing progress"; the brief also says not to show org state after access is lost
Answer: Keep the lost org id
Confidence: 0.85 · Precedent: Fix the root cause and the class of the issue: "if anything repeats the same mistake over and over again, fix the class"; the personal fallback leaves the explorer's exact path broken.
Answer: Both landings
Confidence: 0.60 · Precedent: Fix the class on every affected surface: "fix this for all shared services model pickers on all surfaces"; and 'if you saw a gap, fix it'. The shared helper change creates the Code Reviewer gap, so fix it here.
Answer: Reuse existing keys
Confidence: 0.75 · Precedent: Least work with no drop in quality: "do the least amount of work possible, with no drop in quality"; the existing boundary strings are accurate and consistent across surfaces.
Answer: Same surface, not-found variant
Confidence: 0.80 · Precedent: Fix the class of the issue: "fix class of issue preventing progress"; a Retry button that can never succeed is a gap to fix.
Answer: Push a real follow-up commit
Confidence: 0.72 · Precedent: Autonomy: never accept blocked as an end state; a work item must converge, and the commit must carry a genuine test rather than filler.