fix: gate manager UI with capabilities

[an9xyz]

Summary

• fetch /v1/manager/me for manager identity and capability-driven UI gates
• hide system settings and protected manager routes unless the corresponding capability is present
• gate dashboard ETL, app version writes, user/group writes, and space write/destructive actions by capability
• add loading/retry handling so cached App Bot availability cannot render a partial empty shell before capabilities load

Tests

• npm test
• npm run build

Notes

• Existing PUT /v1/manager/download/versions/{id} usage is left in place; this PR only hides version write controls behind appversion.write. Backend endpoint ownership still needs separate confirmation.

Closes #92

[lml2468]

APPROVE — well-architected capability gating. The core model is sound (deny-by-default, normalized at the API boundary, not persisted so it can't be tampered via localStorage), gates are consistent with proper defense-in-depth, and there's a real unit test. One edge-case bug worth fixing (non-blocking) and a couple of notes. Verified against head 3e4c0770d0de: tsc clean, npm run build passes, capability tests green.

Architecture — correct

• normalizeManagerCapabilities forces every key through === true (so a truthy 1 from the backend is treated as false — strict, deny-by-default) and missing keys → false. Good, and the test covers exactly this.
• managerCapabilities is not in the persist partialize (store/auth.ts:119), so it's always re-fetched from /v1/manager/me on load — a stale/edited localStorage can't grant capabilities. Correct call.
• MainLayout gates the entire <Outlet/> behind a Spin while managerProfileStatus is pending and a retry UI on error — no child route renders before capabilities resolve. CapabilityRoute also returns null during idle/loading (belt-and-suspenders) and the menu is [] until loaded.
• Frontend gating is correctly treated as UX, not the security boundary (PR notes backend ownership still needs confirmation for the version endpoint). Every page pairs UI-hiding with a handler guard (if (!canWrite) return in Download/Users/Groups/Spaces handlers) — so a stale closure or devtools click is also stopped client-side.

🟡 Should-fix (non-blocking) — redirect loop for a manager with no readable section

firstManagerPath (capabilities.ts:45) falls back to /dashboard when no read capability matches, but the /dashboard route is itself wrapped in CapabilityRoute capability="dashboard.read". So a manager whose capability set has no *.read (e.g. a write/trigger-only or empty/misprovisioned account) gets: /dashboard → no dashboard.read → Navigate(firstManagerPath) → /dashboard → … an infinite redirect loop that bricks the console (the index route also points at /dashboard, compounding it). I reproduced the path logic for {}, {users.write}, and {dashboard.trigger} — all loop.

This requires an unusual capability config, so it's an edge case rather than a mainline break — but the failure mode is severe (whole app unusable, no escape). Suggest: when firstManagerPath finds nothing readable, return a dedicated "no access" path (or have CapabilityRoute render a "no permissions, contact admin" empty state instead of redirecting when the target equals the current path). A guard against target === currentPath would also break the cycle.

Notes (non-blocking)

• users.manage_admin is declared in MANAGER_CAPABILITY_KEYS but nothing consumes it yet (the SettingOutlined in Users is just a "system" display tag, not an admin-management action). Fine as a staged key, just flagging it's currently inert.
• Behavior change worth confirming with product: in super scope, member-role changes and member removal now require space.destructive (was unconditionally allowed for super-admin). Intended per the capability model, but it means an existing super-admin without that capability loses abilities they had pre-PR — make sure the backend /v1/manager/me grants the expected defaults so this isn't a silent regression on rollout.

Verification

• tsc --noEmit clean; npm run build passes (the >500 kB chunk warning is pre-existing).
• capabilities.test.ts (3 tests) passes and covers normalization strictness + path selection.
• The 9 feature.test.ts failures are pre-existing on main (local jsdom localStorage), and this PR touches no feature files — not a regression.
• Coverage gap (non-blocking): no tests for CapabilityRoute redirect behavior or the per-page canWrite gating — the redirect-loop case above is exactly what such a test would catch.

Solid, careful PR. Fixing the no-readable-section loop (or proving it's unreachable given backend-guaranteed defaults) is the one thing I'd want before this gates real accounts.

[Jerry-Xin]

The PR is relevant to octo-admin, but one capability-gating bug blocks approval.

🔴 Blocking

• 🔴 Critical: space.destructive is accidentally gated by space.write in the space detail drawer. buildSuperScope maps member removal and role changes to space.destructive in src/hooks/useSpaceScope.ts:113, but SpaceDetailDrawer sets readOnly using !scope.canUpdateSpaceProfile in src/pages/Spaces/SpaceDetailDrawer.tsx:48, where profile updates are tied to space.write. That same readOnly is passed to SpaceMembersPanel at src/pages/Spaces/SpaceDetailDrawer.tsx:79, and SpaceMembersPanel requires !readOnly before enabling removal/role changes in src/pages/Spaces/SpaceMembersPanel.tsx:51. Result: a manager with space.read + space.destructive but not space.write can see destructive list actions, but cannot remove members or change roles in the drawer. Split inactive-space read-only state from profile-edit permission, and let each panel’s own capability flags decide its actions.

💬 Non-blocking

• 🟡 Warning: firstManagerPath falls back to /dashboard even when no readable capability is present in src/auth/capabilities.ts:45. CapabilityRoute then redirects denied /dashboard access back to /dashboard in src/App.tsx:45, which can leave users with an empty/self-redirecting content area for an all-false capability set. Consider an explicit no-access route/state.

✅ Highlights

• Capability normalization is strict and covered by focused tests.
• The main route/menu gates are consistent for the core read capabilities.
• Write controls on dashboard ETL, versions, users, groups, and spaces are generally hidden and guarded in handlers.

[lml2468]

REQUEST CHANGES — correcting my earlier APPROVE. @Jerry-Xin's 🔴 is correct and I missed it: the readOnly propagation couples space.destructive actions to the space.write capability, defeating the fine-grained separation this PR exists to add. Good catch.

🔴 Blocking — space.write gates space.destructive actions via readOnly

• SpaceDetailDrawer.tsx:48: readOnly = space.status !== 1 || !scope.canUpdateSpaceProfile, and canUpdateSpaceProfile → space.write (useSpaceScope.ts:121).
• That single readOnly is passed to SpaceMembersPanel, where canRemove = !readOnly && scope.canRemoveMembers and canChangeRole = !readOnly && scope.canChangeMemberRoles (SpaceMembersPanel.tsx:52-53) — but canRemoveMembers/canChangeMemberRoles → space.destructive (useSpaceScope.ts:118,121).
• Net effect: a super-admin holding space.read + space.destructive but not space.write is forced fully read-only and cannot remove members or change roles, despite having the destructive capability. Reproduced the gate logic: read+destructive (no write) → has destructive cap true but panel canRemove/canChangeRole resolve to false.
• This is in the PR's core purpose (capability separation), so worth fixing rather than shipping cross-wired.
• Fix: decouple "inactive-space read-only" from "profile-edit (space.write)". Don't fold canUpdateSpaceProfile into the shared readOnly. Let each panel gate its own actions by its own capability:
  • SpaceInfoPanel gets the space.write/profile-editability flag (it already takes readOnly).
  • SpaceMembersPanel should derive remove/role purely from scope.canRemoveMembers/canChangeMemberRoles (i.e. space.destructive) AND the status-inactive condition — not from a readOnly that bakes in space.write.
  • Keep status !== 1 (inactive space) as a separate read-only signal applied to all panels.

💬 Non-blocking 🟡 — no-readable-section redirect loop (same as my prior note)

firstManagerPath (capabilities.ts:45) falls back to /dashboard, which is itself gated by dashboard.read, so an all-false capability set self-redirects /dashboard → /dashboard. Add an explicit no-access route / empty state, or guard target === currentPath. (Jerry-Xin and I both flagged this.)

Still correct (unchanged from my first pass)

• Capability normalization is strict (=== true, deny-by-default) with a targeted test; not persisted (can't be tampered via localStorage).
• Route + menu gates are consistent; write handlers have server-of-truth-independent client guards (if (!canWrite) return).
• tsc clean, npm run build passes, capabilities.test.ts green. The 9 feature.test.ts failures are pre-existing on main (jsdom localStorage), unrelated.
• Behavior-change note still stands: super-scope role/remove now require space.destructive — confirm /v1/manager/me grants expected defaults so existing super-admins don't silently lose abilities.

My reflection

I traced each page's canWrite/canDestructive gate in isolation and verified they mapped to the right capability — but didn't follow the readOnly prop across the component boundary from the drawer into the members panel, where a space.write-derived flag ends up gating space.destructive actions. The lesson: when one boolean (readOnly) is computed from one capability and passed into children that also gate on a different capability, trace the combined condition end-to-end. A capability-matrix test (read-only / write-only / destructive-only super-admin → which panel actions show) would catch exactly this and the redirect loop.

Happy to re-approve once the destructive/write decoupling lands.

[mochashanyao]

[Octo-Q · automated review]

Verdict: Approve — no blocking findings; notes below (data-flow traced).

---

octo-admin PR#91 Review Report — fix: gate manager UI with capabilities

Reviewer: Octo-Q (automated review)
Head SHA: 3e4c0770d0ded10e76a693e0912f09dc5d7e04c4
Files: 17 changed (+492 / −127)
Routing: complexity=security_sensitive

---

1. Verification Summary

Area	Status	Evidence
Capability normalization	✅	src/auth/capabilities.ts:28-34 — strict === true check, all keys defaulted to false
CapabilityRoute guard	✅	src/App.tsx:32-48 — blocks render during idle/loading/null; redirects to first allowed path
MainLayout profile fetch	✅	src/layouts/MainLayout.tsx:64-93 — useCallback + useEffect with proper dep array; error → retry UI
Store state transitions	✅	src/store/auth.ts:79-92 — setManagerMe sets loaded; setManagerProfileError sets error + null caps
Persist exclusion	✅	src/store/auth.ts:117-126 — partialize excludes managerCapabilities/managerProfileStatus (always re-fetched)
Menu gating	✅	src/layouts/MainLayout.tsx:99-133 — each item gated by hasManagerCapability; managerCapabilities === null → []
Page-level write guards	✅	Users/Groups/Spaces/Download/Dashboard — canWrite/canDestructive/canRunEtl selectors + handler early-returns
useSpaceScope super integration	✅	src/hooks/useSpaceScope.ts:113-126 — buildSuperScope(capabilities) maps space.write/space.destructive to scope flags
SpaceDetailDrawer readOnly	✅	src/pages/Spaces/SpaceDetailDrawer.tsx:48 — `space.status !== 1
SpaceMembersPanel canChangeRole	✅	src/pages/Spaces/SpaceMembersPanel.tsx:53 — uses scope.canChangeMemberRoles (super→space.destructive; user→role===2)
Unit tests	✅	src/auth/capabilities.test.ts — covers normalization, strict check, firstManagerPath

---

2. Findings

F1 — P2: Index route hard-codes /dashboard; firstManagerPath fallback creates theoretical redirect loop

Diff-scope: new (introduced by this PR)

App.tsx:134:

<Route index element={<Navigate to="/dashboard" replace />} />

capabilities.ts:48:

return '/dashboard'  // fallback when zero capabilities

Data-flow trace: If a manager has zero capabilities (backend returns all-false), visiting / → Navigate to /dashboard → CapabilityRoute checks dashboard.read → false → firstManagerPath(caps) returns /dashboard → Navigate to /dashboard → infinite redirect loop.

Severity rationale (R1/R2): This is new behavior introduced by this PR. However, the trigger condition (a manager with literally zero capabilities keys) is an edge case that implies a backend misconfiguration — any legitimate admin should have at least one read capability. React Router v6 also has built-in redirect-loop detection that would eventually halt the cycle. Not user-visible in normal operation → P2, not P1.

Suggestion: Change firstManagerPath fallback to a dedicated "no permissions" route or show an inline message, rather than defaulting to /dashboard. Alternatively, add a guard in CapabilityRoute: if firstManagerPath(caps) === currentPath, render an error state instead of redirecting.

---

F2 — P2: app-bots admin route not gated by capability

Diff-scope: pre-existing (not changed by this PR's route definition)

App.tsx:192-196:

<Route path="app-bots" element={<AppBotsGate fallback="/dashboard"><AppBots /></AppBotsGate>} />

The app-bots route is wrapped only in AppBotsGate (feature probe for backend availability), not in CapabilityRoute. Any super admin can access /app-bots regardless of manager capabilities, as long as the backend probe returns appBotsAvailable === true.

The sidebar menu item (MainLayout.tsx:130) is also gated only by appBotsAvailable, not by any capability key.

Severity rationale (R1/R2): This appears intentional — app-bots availability is driven by a backend feature probe, not a manager capability. No capability key like appbots.read exists in MANAGER_CAPABILITY_KEYS. If the backend intends capability-based gating here, a key needs to be added. If the feature probe is the intended gate, this is correct as-is. → P2 (design note, not a bug).

Suggestion: Confirm with backend whether app-bots should have a capability key. If yes, add it to MANAGER_CAPABILITY_KEYS and wrap the route + menu item. If the feature probe is the intended gate, add a brief code comment explaining the intentional asymmetry.

---

F3 — P2: AppBotsGate fallback path hard-codes /dashboard

Diff-scope: pre-existing

App.tsx:194:

<AppBotsGate fallback="/dashboard">

If appBotsAvailable becomes false while the user is on /app-bots, they are redirected to /dashboard. If the user also lacks dashboard.read, CapabilityRoute would redirect them to firstManagerPath(caps), which is correct as long as they have at least one capability. But the fallback should ideally use firstManagerPath(managerCapabilities) for consistency.

Suggestion: Use firstManagerPath(managerCapabilities) as the AppBotsGate fallback path instead of hard-coded /dashboard.

---

F4 — P2 (note): All write-action guards are client-side only

Diff-scope: new (this PR's design)

Every write guard in this PR (if (!canWrite) return, conditional button rendering) is a client-side UX gate. A user who bypasses the UI (e.g., via direct API calls or browser devtools) can still invoke write endpoints. The PR body explicitly acknowledges this: "Backend endpoint ownership still needs separate confirmation."

Not a blocking finding — this is the expected pattern for a UI-gating PR. Backend authorization enforcement is a separate concern.

---

F5 — P3: users.manage_admin capability key defined but never consumed

Diff-scope: new

capabilities.ts:10:

'users.manage_admin',

This key is included in MANAGER_CAPABILITY_KEYS and normalized by normalizeManagerCapabilities, but no route, menu item, or page-level guard references it. It is dead code.

Suggestion: If this capability is planned for a future PR, keep it with a // TODO comment. Otherwise, remove it to avoid confusion about what the backend contract actually covers.

---

3. Suggestions

1. F1 fix: Add a zero-capabilities guard — either a dedicated "no permissions" page or an inline error state in CapabilityRoute when firstManagerPath would redirect to the current path.
2. F2/F3 fix: Confirm whether app-bots needs a capability key; if not, add a code comment. Use firstManagerPath() for the AppBotsGate fallback.
3. F5 cleanup: Remove or annotate users.manage_admin.

---

4. Extra Findings

• Persist security is correct: managerCapabilities is intentionally excluded from partialize (auth.ts:117-126), ensuring capabilities are always freshly fetched after page reload. This prevents stale cached capabilities from granting access after a server-side permission revocation. ✅
• Error recovery is clean: On /v1/manager/me failure, the error screen with retry button is shown (MainLayout.tsx:325-345). The loadManagerProfile callback properly resets status to loading on retry. ✅
• normalizeManagerCapabilities is robust: Handles null, undefined, and non-true values correctly. Test coverage confirms strict === true semantics. ✅

---

5. Data-Flow Trace

Consumer	Data consumed	Upstream source	Verified flow
CapabilityRoute	managerCapabilities, managerProfileStatus	useAuthStore ← setManagerMe ← getManagerMe() ← GET /v1/manager/me	✅ MainLayout blocks <Outlet/> until loaded; CapabilityRoute has defensive null guard
MainLayout.menuItems	managerCapabilities	Same chain	✅ null → [] prevents stale menu; each item gated by hasManagerCapability
MainLayout.homePath	managerCapabilities	Same chain	✅ firstManagerPath(caps) selects first allowed route
Page write guards (canWrite, canDestructive, canRunEtl)	managerCapabilities via useAuthStore selector	Same chain	✅ Selector re-evaluates on store change; handler early-returns prevent action
buildSuperScope	capabilities param from useAuthStore(s => s.managerCapabilities)	Same chain	✅ Maps space.write → canManageInvites/canAddMembers/canUpdateSpaceProfile/canReviewApplies; space.destructive → canRemoveMembers/canChangeMemberRoles
SpaceDetailDrawer.readOnly	scope.canUpdateSpaceProfile	buildSuperScope ← space.write capability	✅ Combined with space.status !== 1; passed to SpaceInfoPanel and tab panels
SpaceMembersPanel.canChangeRole	scope.canChangeMemberRoles	buildSuperScope ← space.destructive (super) or role === 2 (user)	✅ Replaces old `scope.kind === 'super'

---

6. R5 Blind-Spot Checklist (security_sensitive)

C1 — Dual-path parity: N/A. This PR does not touch symmetric add/remove or subscribe/unsubscribe paths. The capability gating is applied consistently across all admin routes (except app-bots, noted in F2).

C2 — Control-flow ordering / nesting: Clear. CapabilityRoute checks loading state before capability check (correct ordering). normalizeManagerCapabilities runs before store write, so all consumers see normalized data. No nested/double-application risk.

C3 — Authorization boundary ≠ capability boundary: Acknowledged. This PR implements UI-level authorization gates only. The actual authorization boundary is at the backend API level (/v1/manager/* endpoints). The PR body explicitly notes backend verification is pending. The UI gates correctly prevent accidental/unauthorized use through the normal UI flow, but do not constitute a security boundary.

---

Verdict

No P0 or P1 findings. All issues are P2 (design notes / robustness improvements) or P3 (dead code). The capability normalization is strict and correct, the loading/error states are properly handled, the persist exclusion is security-appropriate, and the data flow from API → store → consumers is verified end-to-end.

[Octo-Q] verdict: APPROVE — with suggestions to address F1 (zero-capability redirect loop guard) and F2 (app-bots capability confirmation) in a follow-up.

[yujiawei]

Code Review — PR #91 (octo-admin)

Reviewed at head 3e4c0770d0ded10e76a693e0912f09dc5d7e04c4 against merge-base 0c1f5e9b. This is a security-sensitive PR (auth/capability gating) and was reviewed accordingly. Verdict: Changes requested — one blocking correctness defect that the gating system's own intended configurations can trigger, plus several items worth confirming before merge.

1. Summary

The change adds client-side capability gating for the super-manager admin UI: it fetches /v1/manager/me, stores a normalized ManagerCapabilities map in the auth store, and conditionally renders routes (CapabilityRoute), the sidebar menu, and write/destructive controls via hasManagerCapability(). The design is sound and consistently deny-by-default:

• ✅ normalizeManagerCapabilities / hasManagerCapability use strict === true, so any non-boolean or missing key fails closed (src/auth/capabilities.ts:33,42).
• ✅ managerCapabilities is intentionally not persisted (partialize in src/store/auth.ts:118-126), so a revoked capability cannot survive a reload — re-fetched each session.
• ✅ Null/loading/error states are guarded before <Outlet/> renders (src/layouts/MainLayout.tsx), with a spinner + retry panel.
• ✅ Menu, route, and per-action gating are layered (defense in depth on the client).

The implementation is clean and the new unit tests are a good start. The blocker below is not the design — it is an edge case in the redirect fallback that the partial-capability model explicitly invites.

2. Blocking issue (must fix before merge)

[P1] Infinite redirect loop / crash for a manager with no readable landing capability

Files: src/auth/capabilities.ts:44-53 (firstManagerPath), src/App.tsx:45-47 (CapabilityRoute), src/App.tsx:134 (index route)

firstManagerPath() returns /dashboard as its final fallback even when dashboard.read is false:

export function firstManagerPath(capabilities): string {
  if (hasManagerCapability(capabilities, 'dashboard.read')) return '/dashboard'
  ...
  return '/dashboard'   // fallback even when dashboard.read is false
}

CapabilityRoute redirects to that same path when the capability is missing:

if (!hasManagerCapability(managerCapabilities, capability))
  return <Navigate to={firstManagerPath(managerCapabilities)} replace />

So for a logged-in super manager whose capabilities have no read flag that firstManagerPath consults, the flow is:

/ → /dashboard → CapabilityRoute(dashboard.read) denies → Navigate(firstManagerPath()) → /dashboard → denies → … → React Router v6 throws "Maximum update depth exceeded" → blank/crashed Content with no in-app recovery (the breadcrumb home link also points at firstManagerPath, and menuItems is []).

Why this is in scope and not theoretical: firstManagerPath only inspects the seven *.read / system_setting / backup keys. It ignores dashboard.trigger, users.write, groups.write, space.write, space.destructive, appversion.write, users.manage_admin. This PR's entire purpose is to support partial capability sets, and dashboard.read vs dashboard.trigger are deliberately distinct keys — so a manager provisioned with, say, users.write but not users.read, or any write/destructive-only set, lands in the loop. The all-false set (freshly created super, unknown role) does too. The guard layer does not catch it: managerProfilePending/managerProfileFailed both require managerCapabilities === null, which is false once a non-null all-false object is loaded.

Suggested fix: make firstManagerPath return a neutral "no access" route when nothing matches (and add a small no-permission page), or have CapabilityRoute render a "no permission" panel instead of self-redirecting when the computed target equals the route that just denied. A regression test for the empty/all-false and write-without-read cases should accompany the fix.

3. Non-blocking — recommend confirming before/at merge (P2)

• Client-side gating is not enforcement (verify server-side authz). src/api/index.ts:23-30 attaches the same bearer token to every request regardless of capability; every write guard is a client-only if (!canWrite) return plus a hidden button (e.g. src/pages/Groups/index.tsx:92, src/pages/Spaces/index.tsx:191, src/pages/Download/index.tsx:145). This is correct as a UI layer, and this PR does not remove any pre-existing server check — but the gating is fully bypassable by editing JS, replaying a request, or tampering the /me response. Before relying on it, confirm every gated /v1/manager/* mutating endpoint returns 403 without the corresponding capability. This is the single most important thing for a human to manually verify on this PR.

• Space drawer couples destructive actions to space.write. src/pages/Spaces/SpaceDetailDrawer.tsx:48 computes readOnly from canUpdateSpaceProfile (= space.write) and passes it to all panels, so in SpaceMembersPanel (canRemove, canChangeRole, both = space.destructive) a manager with space.destructive but not space.write loses member-remove/role-change in the drawer. The PR's own list view (src/pages/Spaces/index.tsx) treats space.write and space.destructive as independent (dissolve/ban gated purely on canDestructive, create purely on canWrite), so this is an internal inconsistency. Suggest deriving the members panel's read-only state from space status only, letting canRemove/canChangeRole rely on the destructive scope.

• users.manage_admin is declared but never consumed (src/auth/capabilities.ts:10). No component reads it. Either a manage_admin-protected action is missing its client guard, or the key is stale and should be removed. Worth a one-line resolution so the capability surface doesn't imply coverage that doesn't exist.

• /app-bots is feature-gated, not capability-gated. It is the only super route still wrapped in AppBotsGate (feature probe) rather than CapabilityRoute (src/App.tsx:191-198); there is no app_bots key. This is pre-existing (unchanged from base) and likely intentional, but please confirm app-bots is meant to be open to all managers when the feature is enabled.

4. Minor / nits

• src/App.tsx:134 index route hardcodes /dashboard rather than firstManagerPath(); relies on a second redirect hop. Once the P1 fallback is fixed, pointing this at firstManagerPath() directly is cleaner.
• CapabilityRoute returns null on managerProfileStatus === 'error' (because managerCapabilities === null); this is currently harmless only because MainLayout intercepts the error state first. A short comment documenting that ordering dependency would prevent a future regression if CapabilityRoute is ever mounted outside MainLayout.
• loadManagerProfile surfaces the raw backend error via message.error((error as Error).message) (MainLayout.tsx:69-71), unlike the i18n'd inline panel. This matches existing house style across the codebase, so non-blocking — but consider an i18n'd message for consistency.
• capabilities.test.ts covers the happy paths but not the high-risk branches: the all-false firstManagerPath fallback (the loop above), the full precedence chain, unknown-key dropping, and null inputs. Adding these would have caught the P1.

5. Verification

• Diff is frontend-only (all 17 files under src/); no server changes.
• Build/test claims in the PR body (npm test, npm run build) were not re-run in this review; the new logic is type-safe and the gating reads are correct where present. The one functional defect is the redirect fallback (§2).

---

Once the §2 loop is fixed (and ideally the server-side enforcement in §3 confirmed), this is in good shape to merge.

[an9xyz]

Addressed the requested changes in dd50fb6:

• Decoupled inactive-space read-only state from space.write in SpaceDetailDrawer: profile editing still requires space.write, while members/invites/join-apply panels receive only the inactive-space read-only flag so space.destructive member actions are no longer blocked by missing space.write.
• Added a /no-access manager route and changed firstManagerPath fallback to it, so all-false or write-only capability sets no longer self-redirect through /dashboard.
• Changed the manager index route to use firstManagerPath() instead of hard-coded /dashboard.
• Changed manager AppBots fallback to firstManagerPath() instead of hard-coded /dashboard.
• Added a regression test for write/destructive-only capabilities falling back to no-access.
• Annotated users.manage_admin as reserved for the future admin-account management UI.

Verification rerun locally:

• npm test
• npm run build
• git diff --check

[lml2468]

APPROVE — re-review at dd50fb6efc0b. Both blockers from the last round are fixed correctly, with a new test for the redirect fallback. tsc clean, npm run build passes, capability tests green (now 4). Verified each fix against the new SHA.

🔴 → fixed: space.destructive no longer gated by space.write

SpaceDetailDrawer now splits the flag:

• inactiveSpace = space.status !== 1 (status-only)
• profileReadOnly = inactiveSpace || !scope.canUpdateSpaceProfile (status OR space.write)

SpaceInfoPanel gets profileReadOnly (profile edit = space.write); SpaceMembersPanel / SpaceInvitesPanel / SpaceJoinAppliesPanel now get inactiveSpace only. So member remove/role-change gate purely on scope.canRemoveMembers/canChangeMemberRoles (= space.destructive), independent of space.write. Verified the matrix:

• read+destructive (no write): profile editable false, member remove/role true ✅ (was false before — the bug)
• read+write (no destructive): profile editable true, member remove/role false ✅
• all three: both true ✅

🔴/🟡 → fixed: no-readable-section redirect loop

• firstManagerPath now returns a new MANAGER_NO_ACCESS_PATH = '/no-access' instead of /dashboard when nothing is readable (capabilities.ts:56).
• /no-access is a plain ungated route rendering <NoAccess/> (a "no available access" empty state) — confirmed it's not wrapped in CapabilityRoute, so no re-redirect.
• The index route now uses ManagerIndexRoute (resolves via firstManagerPath, null-safe during load) instead of a hardcoded /dashboard Navigate.
• app-bots fallback switched from hardcoded /dashboard to ManagerAppBotsGate → firstManagerPath (so it also lands on /no-access for a zero-read account instead of looping).
• Traced both entry paths for a zero-read capability set (index and a deep-link to a gated route → CapabilityRoute → firstManagerPath): both terminate at the ungated /no-access. No loop.
• New test covers it: firstManagerPath({users.write, space.destructive}) and firstManagerPath(null) → /no-access.

Notes addressed / remaining

• users.manage_admin now has an explicit "reserved for a future surface" comment — good, clears the inert-key ambiguity.
• app-bots is still feature-gated (appBotsAvailable), not capability-gated — unchanged from main and there's no app-bots capability key, so this is by design, not the bypass it might look like. If product wants app-bots behind a manager capability, that's a separate follow-up (raised by mochashanyao). Non-blocking.
• Still worth confirming on rollout: /v1/manager/me returns the expected capability defaults so existing super-admins don't silently lose role/remove (now space.destructive) or profile-edit (now space.write) abilities. This is a backend-contract check, not a code issue here.

Verification

• tsc --noEmit clean; npm run build passes (pre-existing chunk-size warning only).
• capabilities.test.ts: 4 tests pass, including the new no-access fallback.
• The 9 feature.test.ts failures remain pre-existing on main (jsdom localStorage), unrelated — PR touches no feature files.

Both fixes are minimal and correct, and the previously-missing capability-matrix edge is now test-covered. Thanks for the quick, precise turnaround. LGTM.

Both blocking items fixed in dd50fb6: (1) readOnly decoupled — SpaceMembersPanel now gates on inactiveSpace+space.destructive, not space.write; (2) firstManagerPath returns /no-access (ungated route) instead of /dashboard, eliminating the redirect loop. Re-reviewing on the new head.

[Jerry-Xin]

Re-review of dd50fb6 (was 3e4c077). Both items from my prior REQUEST_CHANGES are now genuinely fixed; verified by reading the changed code at HEAD, not the commit message. Approving.

✅ Resolved (previously blocking)

• 🔴 → fixed: space.destructive is no longer accidentally gated by space.write. SpaceDetailDrawer.tsx now separates inactiveSpace = space.status !== 1 from profileReadOnly = inactiveSpace || !scope.canUpdateSpaceProfile. Only SpaceInfoPanel (profile edit) receives profileReadOnly; SpaceMembersPanel/SpaceInvitesPanel/SpaceJoinAppliesPanel receive readOnly={inactiveSpace} (status-only). In SpaceMembersPanel.tsx:51-53, canRemove/canChangeRole gate on !readOnly && scope.canRemoveMembers/canChangeMemberRoles, which map to space.destructive in useSpaceScope.ts buildSuperScope. Net: an admin holding space.read + space.destructive but not space.write can now remove members / change roles on an active space. Decoupled correctly.

• 🟡 → fixed: the all-false / no-readable-capability redirect loop is gone. firstManagerPath (capabilities.ts) now returns MANAGER_NO_ACCESS_PATH = '/no-access' as the final fallback instead of /dashboard. App.tsx registers <Route path="no-access" element={<NoAccess />} /> outside any CapabilityRoute, so a manager with no readable caps lands on a real NoAccess screen rather than bouncing through /dashboard.

💬 Non-blocking follow-ups (do not block merge)

• 🟡 Stale /v1/manager/me write: loadManagerProfile (MainLayout.tsx) → setManagerMe (store/auth.ts:80) overwrites name/role/uid/managerCapabilities unconditionally when the request resolves. In a narrow window (slow response + logout or super→super account switch mid-flight) an earlier session's capabilities could be committed. Server-side token authorization still applies, so this is UI-state hardening rather than an authz bypass. Recommend guarding the commit with the current token/scope or a request id / AbortController. Suggested as a follow-up, not a blocker.

• 🟡 App Bots gating model (pre-existing, unchanged from main): /app-bots is feature-gated via AppBotsGate/appBotsAvailable, not capability-gated, and there is no app-bot key in MANAGER_CAPABILITY_KEYS. This is identical to main (verified at merge-base) and is out of scope for this PR. Only flag as a product decision if app-bots is intended to be capability-gated; track separately.

✅ Highlights

• Capability normalization is strict (=== true) and covered by focused tests.
• Write controls are both hidden in the UI and guarded in their event handlers — correct defense-in-depth for permission gates.
• CI: build (18) and build (20) green; only check-sprint (process gate) is red, which is not a code-quality blocker.

[Jerry-Xin]

APPROVE — re-review at dd50fb6efc0b. Both blockers from my prior review are correctly fixed.

✅ Resolved blockers

• 🔴 → fixed: readOnly is decoupled. SpaceDetailDrawer now derives profileReadOnly = inactiveSpace || !canUpdateSpaceProfile (space.write) and passes it only to SpaceInfoPanel, while SpaceMembersPanel / SpaceInvitesPanel / SpaceJoinAppliesPanel receive only inactiveSpace (status check). So canRemove/canChangeRole = !inactiveSpace && scope.canRemoveMembers/canChangeMemberRoles now gate purely on space.destructive — a manager with space.read + space.destructive but no space.write retains member-management. The cross-wiring is gone.
• 🟡 → fixed: firstManagerPath now returns MANAGER_NO_ACCESS_PATH (/no-access) instead of /dashboard. The /no-access route renders a plain <NoAccess/> and is NOT wrapped in any CapabilityRoute, so the all-false-capability redirect loop is eliminated. The index route also uses ManagerIndexRoute → firstManagerPath instead of hardcoding /dashboard.

💬 Non-blocking follow-up

• 🟡 App Bots is feature-gated (AppBotsGate/appBotsAvailable), not capability-gated. This is unchanged from main and there is no app-bot key in MANAGER_CAPABILITY_KEYS to gate it by — so it is not a regression introduced by this PR, and the fallback actually improved (/dashboard → firstManagerPath). Backend remains the enforcement boundary for app-bot write/token actions. If the product intends App Bots to become a per-manager capability surface, that is a new requirement: add an appbots.* key and gate menu + route + write controls. Tracking as follow-up, not blocking this PR.
• 🟡 Confirm /v1/manager/me defaults so existing super-admins are not silently downgraded now that role-change/removal require space.destructive.

✅ Highlights

• Strict deny-by-default capability normalization with unit tests; capabilities excluded from persistence (re-fetched, tamper-resistant).
• Shell-level gating blocks the outlet until capabilities load; per-page UI hiding paired with handler-level guards (defense in depth).

CI: build (18)/(20) pass. Only check-sprint fails — a sprint/board process gate, not a code-quality blocker.

[mochashanyao]

[Octo-Q · automated review]

Verdict: Approve — no blocking findings; notes below (data-flow traced).

---

octo-admin PR#91 Review Report — fix: gate manager UI with capabilities

Reviewer: Octo-Q (automated review)
Head SHA: dd50fb6efc0bcd7e4a2e4f2274efc24c4ca92ec0
Base: 0c1f5e9 (main, v1.5.2)
Commits: 2 (3e4c077 + dd50fb6)
Files changed: 17 (+548 / −133)
Routing: complexity=security_sensitive

---

1. Verification Summary

Area	Status	Evidence
Capability model definition	✅	src/auth/capabilities.ts — 14 keys, strict === true normalization, unit tests cover missing/non-boolean/fallback
Profile loading lifecycle	✅	MainLayout.tsx:64-93 — idle→loading→loaded/error; spinner + retry UI on failure
Route-level gating	✅	App.tsx:33-63 — CapabilityRoute returns null during loading, redirects on missing cap
Sidebar menu filtering	✅	MainLayout.tsx:102-131 — items built from capabilities; empty array when null
In-page write guards (Users/Groups/Download/Spaces/Dashboard)	✅	Each page derives canWrite/canDestructive/canRunEtl from store; handler early-returns + conditional button rendering
Space scope refinement	✅	useSpaceScope.ts:113-125 — buildSuperScope maps space.write/space.destructive to scope flags
SpaceDetailDrawer decomposition	✅	SpaceDetailDrawer.tsx:48-49 — inactiveSpace vs profileReadOnly correctly separated
Persist exclusion	✅	store/auth.ts:113-120 — partialize excludes managerCapabilities/managerProfileStatus (forces re-fetch on reload)
Unit tests	✅	capabilities.test.ts — 4 tests covering normalization, strict check, path selection, no-access fallback

2. Findings

P2-1: app-bots route has no capability gate

Diff-scope: new (this PR introduces capability gating for all other manager routes but omits app-bots)

The /app-bots route is wrapped in ManagerAppBotsGate (feature-availability gate) but not in CapabilityRoute. There is no appbots.read key in MANAGER_CAPABILITY_KEYS. Any manager can access app-bots when the backend feature probe returns true, regardless of their capability set.

• App.tsx:218-224 — route definition
• capabilities.ts:1-16 — no app-bots capability key defined

Assessment: Likely intentional (app-bots is a cross-cutting feature gated by backend availability, not per-manager capabilities). The sidebar menu also shows app-bots based solely on appBotsAvailable (MainLayout.tsx:128-129). Consistent between menu and route. Not a security issue if the backend enforces its own authorization on app-bots API endpoints.

Recommendation: If app-bots should be capability-gated, add an appbots.read key and wrap the route. If intentionally open to all managers, a brief code comment on the route explaining the design decision would help future maintainers.

P2-2: SystemSetting and Backup pages rely solely on route-level gating

Diff-scope: new (this PR adds route-level CapabilityRoute but no in-page guards)

Unlike Users, Groups, Download, Spaces, and Dashboard — which all have in-page canWrite/canDestructive/canRunEtl guards on their write handlers — SystemSetting and Backup have zero in-page capability checks. They rely entirely on CapabilityRoute preventing the page from rendering.

Assessment: Acceptable defense-in-depth posture since (a) CapabilityRoute blocks rendering, (b) sidebar hides the menu item, and (c) backend should enforce authorization on API endpoints. The other pages' in-page guards are an extra layer, not a requirement. No user-visible impact.

Recommendation: Consider adding in-page guards for consistency if these pages gain write operations that could be triggered via keyboard shortcuts or deep links in future refactors.

P2-3: buildSuperScope returns all-false permissions when capabilities are null

Diff-scope: new (this PR introduces the capabilities parameter to buildSuperScope)

useSpaceScope.ts:113-125 — when managerCapabilities is null (loading state), buildSuperScope(null) returns canWrite=false, canDestructive=false, etc. All space operations are locked down during the brief loading window.

Assessment: Not user-visible because MainLayout renders a <Spin /> spinner instead of <Outlet /> while managerProfilePending is true (MainLayout.tsx:327-331). The SpaceDetailDrawer can only be opened by user interaction after the layout has rendered, so capabilities are always loaded by then. Safe fail-closed behavior.

Recommendation: No action needed. The fail-closed + UI spinner combination is correct.

3. Suggestions

1. Consider a code comment on the app-bots route explaining why it lacks a CapabilityRoute wrapper (P2-1).
2. Optional: Add appbots.read to MANAGER_CAPABILITY_KEYS if backend already returns such a capability, or plan to add it when the capability model matures.

4. Extra Findings

• Persist exclusion is correct and intentional. managerCapabilities is excluded from partialize, forcing a fresh /v1/manager/me call on every page load. This prevents stale capability data from localStorage and ensures the server is the source of truth. Good security hygiene.
• Login/logout correctly resets capabilities. Both loginSuper() and logout() set managerCapabilities: null and managerProfileStatus: 'idle', triggering a fresh load on next MainLayout mount.
• normalizeManagerCapabilities strict === true check correctly rejects non-boolean truthy values (e.g., 1, "true"). Verified by unit test.
• SpaceMembersPanel role-change logic changed from scope.kind === 'super' || scope.role === 2 to scope.canChangeMemberRoles. For super scope, this is now gated by space.destructive capability. This is a behavioral change — super admins without space.destructive can no longer change member roles. Consistent with the PR's capability-based model.

5. Data-Flow Tracing

Consumed Data	Upstream Source	Flows Correctly?
managerCapabilities (store)	getManagerMe() → normalizeManagerCapabilities() → setManagerMe()	✅ API response normalized to strict boolean map, stored in zustand
managerProfileStatus (store)	setManagerProfileLoading() / setManagerMe() / setManagerProfileError()	✅ Three-state lifecycle (idle→loading→loaded/error) correctly managed
CapabilityRoute render decision	managerCapabilities from store + managerProfileStatus	✅ Returns null during loading, redirects on missing cap, renders children on match
menuItems (sidebar)	managerCapabilities from store	✅ Filtered by capability; empty when null (loading)
canWrite / canDestructive (pages)	hasManagerCapability(s.managerCapabilities, key) via useAuthStore selector	✅ Reactive to store changes; handler guards + conditional rendering
scope.canUpdateSpaceProfile	buildSuperScope(capabilities) → hasManagerCapability(capabilities, 'space.write')	✅ Flows from store → useSpaceScope → SpaceDetailDrawer → SpaceInfoPanel
scope.canChangeMemberRoles	buildSuperScope(capabilities) → hasManagerCapability(capabilities, 'space.destructive')	✅ Replaces hardcoded scope.kind === 'super' check
firstManagerPath(capabilities)	Used in ManagerIndexRoute, CapabilityRoute redirect, breadcrumb, ManagerAppBotsGate fallback	✅ Consistent priority ordering; /no-access fallback when no read caps
homePath (breadcrumb)	firstManagerPath(managerCapabilities)	✅ Can be /no-access when caps are null — but breadcrumb only renders inside MainLayout which gates on profile loaded

6. R5 Blind-Spot Checklist (security_sensitive)

C1 — Dual-path parity:

• login ↔ logout: Both reset managerCapabilities: null + managerProfileStatus: 'idle'. ✅ Clear.
• add ↔ remove members: Both gated by scope.canAddMembers / scope.canRemoveMembers (from space.write / space.destructive). Symmetric. ✅ Clear.
• create ↔ delete invites: Both gated by scope.canManageInvites (from space.write). ✅ Clear.
• No create-gate vs execute-path asymmetry found — this PR is UI-only gating; no authorization is created or delegated.

C2 — Control-flow ordering / nested reuse:

• normalizeManagerCapabilities called once in getManagerMe() API chain. Not reused in nested/composite calls. ✅ Clear.
• hasManagerCapability is a pure === true check. No ordering or nesting concerns. ✅ Clear.
• No security controls (regex/escape/sanitize) in this PR. N/A.

C3 — Authorization boundary ≠ capability boundary:

• /v1/manager/me endpoint: backend auth (token). Frontend only consumes the response. ✅ Clear.
• All manager API endpoints (ban/unban/forbid/create-space/etc.): frontend capability checks are UI-only. Backend must independently verify authorization. No new endpoints introduced. ✅ Clear.
• No new tool/capability exposure in this PR. N/A.

---

[Octo-Q] verdict: APPROVE

This PR is a well-structured capability-based access control implementation for the manager UI. The data flow from API → store → route/sidebar/page is consistent and correctly traced. Loading/error states are handled with appropriate UI (spinner + retry). The persist exclusion forces fresh capability data on every page load. Unit tests cover the core capability logic. The three P2 findings are design observations, not blockers.

Duplicate — superseded by consolidated APPROVE review on the same commit.

[yujiawei]

Code Review — PR #91 (octo-admin)

Reviewed at head dd50fb6efc0bcd7e4a2e4f2274efc24c4ca92ec0 against merge-base 0c1f5e9b. This is a security-sensitive PR (auth/capability gating) and was reviewed accordingly, including the two blockers raised in the prior round. Verdict: Approve — both prior blockers are correctly fixed, no new blocking defect found. One advisory coverage gap and a few non-blocking nits are listed below.

1. Summary

The change adds client-side capability gating for the super-manager admin UI: it fetches /v1/manager/me, stores a normalized ManagerCapabilities map in the auth store, and conditionally renders routes (CapabilityRoute), the sidebar menu, and write/destructive controls via hasManagerCapability(). The design is consistently deny-by-default and the layering (route + menu + per-action) is sound defense-in-depth on the client.

2. Prior-round blockers — both verified fixed ✅

• Infinite redirect loop for a manager with no readable landing capability — Fixed. firstManagerPath now returns MANAGER_NO_ACCESS_PATH = '/no-access' when no readable route matches (src/auth/capabilities.ts:48-57). I traced every navigation consumer: CapabilityRoute (src/App.tsx:46-48) and ManagerIndexRoute (src/App.tsx:52-60) only ever redirect to a path whose own gating capability was just confirmed true, or to /no-access, which is an ungated leaf route (src/App.tsx:229) and therefore terminates. Verified for all-false caps, write-without-read, only-system_setting, and cross-route redirect — none loops. Covered by a new test (src/auth/capabilities.test.ts:39-48, incl. the null case).

• space.destructive coupled to space.write in the space drawer — Fixed. SpaceDetailDrawer now splits inactiveSpace = space.status !== 1 (status-only) from profileReadOnly = inactiveSpace || !scope.canUpdateSpaceProfile (src/pages/Spaces/SpaceDetailDrawer.tsx:48-49). The members/invites/join-applies panels receive readOnly={inactiveSpace} and gate writes via scope; only the profile panel gets the capability-aware profileReadOnly. In src/hooks/useSpaceScope.ts:113-124, canRemoveMembers/canChangeMemberRoles derive from space.destructive while canManageInvites/canAddMembers/canUpdateSpaceProfile/canReviewApplies derive from space.write — the two are now independent.

3. Verification highlights

• Fail-closed normalization is solid. normalizeManagerCapabilities (src/auth/capabilities.ts:32-39) iterates a fixed allow-list and assigns key === true, so null/undefined/array/partial/non-boolean-truthy (1, "true") inputs all collapse to false, and unknown server keys are dropped. hasManagerCapability uses the same strict === true.
• No stale-capability leak. managerCapabilities and managerProfileStatus are intentionally excluded from partialize (src/store/auth.ts:117-126), so they never hydrate from localStorage and are re-fetched each session; they are also reset in loginSuper/loginSpace/logout. The error path (setManagerProfileError) nulls caps rather than keeping a prior set, and MainLayout renders the retry panel instead of <Outlet/> while caps are null — fail-closed at three independent layers.
• Per-action guard coverage is complete and capability-matched across Dashboard (dashboard.trigger), Download (appversion.write), Groups (groups.write), Users (users.write), and Spaces (space.write/space.destructive), with both the rendered control and the handler guarded in nearly all cases.

4. Advisory (non-blocking)

• /app-bots is feature-gated, not capability-gated. It is the only manager route wrapped in ManagerAppBotsGate (a backend feature probe) rather than CapabilityRoute (src/App.tsx:221-228), and the menu item appears whenever appBotsAvailable === true (src/layouts/MainLayout.tsx:129-131) with no capability check. So once caps load, any super-scope manager who passes the feature probe can reach the AppBots CRUD surface regardless of capability set. This is pre-existing (the base already used AppBotsGate fallback="/dashboard" with no capability gate) and client-side only — and there is currently no app_bot key in MANAGER_CAPABILITY_KEYS to gate it with. It does not regress anything in this PR, but it is the one surface left outside the new gating model. Recommend confirming whether an app_bot capability should exist (then add key + CapabilityRoute), or documenting that app-bots is intentionally open to all super-managers. Not a merge blocker.
• Server-side enforcement is the real boundary — please verify manually. Every guard here is a client-only hidden control plus if (!canX) return; all of it is bypassable by editing JS, replaying a request, or tampering the /me response. This PR is correct as a UI layer and removes no existing server check, but before relying on it, confirm every gated /v1/manager/* (and /v1/app_bot/*) mutating endpoint returns 403 without the corresponding capability. This is the single most important thing for a human to verify on this security-sensitive PR.

5. Nits (optional)

• Dashboard handleRunEtl lacks the defensive in-handler guard. It relies solely on the button being hidden (src/pages/Dashboard/index.tsx), unlike every other mutating page which also adds if (!canX) return. No exploit path (the button is the only trigger), but adding if (!canRunEtl) return would restore the consistency the PR otherwise establishes.
• Breadcrumb home href omits the router basename (src/layouts/MainLayout.tsx:230-238). Left-clicks are fine (preventDefault + navigate), but a middle/ctrl-click opens /dashboard instead of /admin/dashboard → 404. Cosmetic.
• users.manage_admin is declared but unused (src/auth/capabilities.ts:10-11). Intentional per the inline comment; harmless. Just ensure the future admin-account surface wires a gate to this key rather than re-deriving from users.write.
• Retry has no in-flight guard — rapid clicks can fire concurrent /v1/manager/me requests (src/layouts/MainLayout.tsx). Last-resolving response wins and is re-normalized fail-closed, so UX-only.
• npm test / npm run build were not re-run here (deps not installed in the review environment); the observed tsc errors were all missing-module resolution from the absent node_modules, not PR-introduced. CI should confirm green.

Both prior blockers are resolved and the gating model is coherent and fail-closed. Approving; please action the §4 server-side verification and consider the /app-bots coverage question before/after merge.

[lml2468]

APPROVE — re-review at 1fb2ae1d8f7c. This is an empty commit (chore: rerun sprint checks) whose tree is byte-for-byte identical to the previously-approved dd50fb6efc0b (git diff --quiet dd50fb6efc0b 1fb2ae1d8f7c → no changes; parent is dd50fb6). It only re-triggers the check-sprint process gate — no code change.

My prior approval carries over unchanged:

• 🔴 capability coupling (space.destructive gated by space.write) — fixed & matrix-verified at dd50fb6.
• 🔴/🟡 no-readable-section redirect loop — fixed via ungated /no-access + ManagerIndexRoute, test-covered.
• Capability core sound (deny-by-default, normalized at API boundary, not persisted, shell-gated until loaded, handler guards), menu correctly capability-filtered.

Re-confirmed the tree is identical, so no re-run of tsc/build/tests is warranted — the dd50fb6 verification (tsc clean, build passes, 4 capability tests green; the 9 feature.test.ts failures pre-existing on main) stands.

Non-blocking follow-ups remain as noted (App Bots → per-manager capability if product wants it; optional finer in-page guards for SystemSetting/Backup; confirm /v1/manager/me defaults on rollout). LGTM — re-approving the current head so the gate reflects it.

[mochashanyao]

[Octo-Q · automated review]

Verdict: Approve — no blocking findings; notes below (data-flow traced).

---

octo-admin PR#91 Review Report — fix: gate manager UI with capabilities

Reviewer: Octo-Q (automated review)
Head SHA: 1fb2ae1d8f7c81e347974d3a2da34f26e73ba771
Files changed: 17 (+548 / −133)
Routing: complexity=security_sensitive (automated review)

---

1. Verification Summary

Area	Status	Evidence
Capability type system	✅	src/auth/capabilities.ts — MANAGER_CAPABILITY_KEYS const tuple → ManagerCapabilityKey union → ManagerCapabilities record. normalizeManagerCapabilities strict === true check (rejects truthy non-boolean).
API → store data flow	✅	getManagerMe() → normalizeManagerCapabilities(res.data.capabilities) → setManagerMe() → store managerCapabilities. All keys normalized to boolean.
Route gating	✅	CapabilityRoute in src/App.tsx:33-49 blocks render until managerProfileStatus is 'loaded' and capabilities non-null. Redirects to firstManagerPath() on missing capability.
Sidebar menu gating	✅	MainLayout.tsx:102-131 — menuItems returns [] when managerCapabilities === null; each item gated by hasManagerCapability.
Loading/error UX	✅	MainLayout.tsx:328-354 — <Spin /> while pending; error state with retry button. managerProfilePending and managerProfileFailed correctly derived.
Persist middleware safety	✅	store/auth.ts:119-128 — partialize excludes managerCapabilities and managerProfileStatus. After refresh, capabilities re-fetched via useEffect.
Write-action guards in pages	✅	Users/Groups/Download/Spaces/Dashboard all add if (!canWrite) return guards on handlers AND conditionally render action columns/buttons.
Space scope capability mapping	✅	useSpaceScope.ts:113-125 — buildSuperScope maps space.write → write ops, space.destructive → destructive ops. buildUserScope adds canChangeMemberRoles: role === 2 and canUpdateSpaceProfile: isManager.
SpaceDetailDrawer readOnly split	✅	`profileReadOnly = inactiveSpace
Unit tests	✅	src/auth/capabilities.test.ts covers normalization, strict boolean check, firstManagerPath priority, and null/no-capabilities fallback to /no-access.

---

2. Findings

F1 — P2: /app-bots route and sidebar item lack capability gating

Diff-scope: new (PR introduced capability gating for all other routes but not app-bots)

src/App.tsx:218-222 wraps /app-bots in ManagerAppBotsGate which only checks useFeatureStore.appBotsAvailable, not any manager capability. Similarly, MainLayout.tsx:127-129 shows the app-bots menu item based solely on appBotsAvailable === true.

// App.tsx — no CapabilityRoute wrapper
<Route path="app-bots" element={<ManagerAppBotsGate><AppBots /></ManagerAppBotsGate>} />

// MainLayout.tsx — no capability check
return appBotsAvailable === true
  ? [...base, { key: '/app-bots', icon: <RobotOutlined />, label: t('nav:appBots') }, ...tail]
  : [...base, ...tail]

Every other manager route (dashboard, users, groups, spaces, system-setting, backup, download) is gated by both a CapabilityRoute and a capability-filtered menu item. App-bots is the sole exception.

Impact: Any super user can access /app-bots regardless of their capability set. If the backend /v1/app_bot/* endpoints are capability-restricted, the UI doesn't reflect this.

Note: No appbots.read key exists in MANAGER_CAPABILITY_KEYS, so this may be intentional pending backend capability design. PR description acknowledges: "Backend endpoint ownership still needs separate confirmation."

Recommendation: Either add an appbots.read capability key and gate accordingly, or add a code comment explaining the deliberate exclusion.

---

F2 — P2: setManagerMe overwrites login-time name/role/uid as side effect

Diff-scope: new (pre-PR, these fields were only set by loginSuper/loginSpace)

src/store/auth.ts:79-85:

setManagerMe: (profile) =>
  set({
    name: profile.name,
    role: profile.role,
    uid: profile.uid,
    managerCapabilities: profile.capabilities,
    managerProfileStatus: 'loaded',
  }),

This silently updates name, role, and uid — fields that are persisted via partialize. If /v1/manager/me returns different values than the login response (e.g., admin changed the user's display name between login and profile fetch), the UI will show the updated values. This is likely desirable but is an undocumented side effect.

Risk: Low. If the /v1/manager/me API ever returns stale or empty name/role/uid, the persisted state could be corrupted until next login. Consider only updating managerCapabilities and managerProfileStatus here, or add a comment documenting the intentional overwrite.

---

F3 — P2: Inconsistent capability key naming convention

Diff-scope: new (these keys are defined in this PR)

src/auth/capabilities.ts:1-15:

system_setting, backup          ← snake_case, no dot
appversion.read, dashboard.read ← resource.action dot notation
users.read, users.write         ← resource.action dot notation
space.read, space.write         ← resource.action dot notation

Two keys (system_setting, backup) use bare snake_case while the rest use resource.action dot notation. This is likely dictated by the backend API contract, but if the backend is still being designed, aligning to system.read/backup.read would improve maintainability.

---

3. Recommendations

1. App-bots gating (F1): Add a comment in App.tsx and MainLayout.tsx explaining why app-bots is excluded from capability gating, or add a capability key when the backend supports it.
2. setManagerMe side effect (F2): Either document the intentional overwrite of name/role/uid or narrow the setter to only update capability-related fields.
3. Key naming (F3): If backend is still flexible, align to resource.action convention. Low priority if backend contract is fixed.

---

4. Additional Observations

• /no-access route is correctly ungated within the super-only layout. Gating it would create a redirect loop when firstManagerPath() returns /no-access. ✅
• firstManagerPath priority order (dashboard → users → groups → spaces → system-setting → backup → download → no-access) matches the sidebar menu order. ✅
• Error retry flow is correct: clicking Retry calls loadManagerProfile() which sets status to 'loading', re-triggering the pending spinner via managerProfilePending. ✅
• buildSuperScope(null) safety: When capabilities are null (brief window during initial load or after API failure), all permissions default to false. Combined with MainLayout blocking <Outlet /> during pending state, this is safe. ✅
• Space-scope routes (/space/:spaceId) are outside MainLayout and don't block on capability loading, but buildSuperScope(null) returns all-false permissions, so no unsafe actions can be triggered during the brief loading window. ✅

---

5. Data-Flow Trace

Consumer	Data consumed	Source	Verified flow
CapabilityRoute	managerCapabilities, managerProfileStatus	useAuthStore	MainLayout useEffect → getManagerMe() → normalizeManagerCapabilities() → setManagerMe() → store. MainLayout blocks <Outlet /> until loaded. ✅
menuItems (MainLayout)	managerCapabilities	useAuthStore	Same as above. Returns [] when null. ✅
firstManagerPath()	managerCapabilities	passed from CapabilityRoute / ManagerIndexRoute / homePath	Handles null → returns /no-access. ✅
Page canWrite / canDestructive	managerCapabilities via hasManagerCapability	useAuthStore selector	Same store, same data. Loaded before pages render (MainLayout gate). ✅
buildSuperScope(capabilities)	managerCapabilities	useAuthStore via useSpaceScope	useMemo depends on [capabilities, scope, role]. Recalculates when capabilities load. ✅
SpaceInfoPanel.editable	readOnly prop + space.status	SpaceDetailDrawer computes profileReadOnly = inactiveSpace || !scope.canUpdateSpaceProfile	scope.canUpdateSpaceProfile ← buildSuperScope ← hasManagerCapability(capabilities, 'space.write'). ✅
SpaceMembersPanel.canChangeRole	scope.canChangeMemberRoles	useSpaceScope	Super: space.destructive. User: role === 2. ✅
normalizeManagerCapabilities output	API response capabilities field	GET /v1/manager/me → res.data.capabilities	Strict === true check rejects truthy non-booleans (tested in capabilities.test.ts). ✅

---

6. R5 Blind-Spot Checklist (security_sensitive)

C1 — Dual-path parity:

• login ↔ logout: Both correctly reset managerCapabilities: null and managerProfileStatus: 'idle'. ✅
• No add/remove or create/delete symmetric paths in this PR. N/A.
• No create-gate ↔ execute-path authorization symmetry applicable. N/A.

C2 — Control-flow ordering / nesting reuse:

• normalizeManagerCapabilities called once in getManagerMe() API layer. Not called from multiple sites. ✅
• hasManagerCapability is a pure read-only check. Safe under any ordering. ✅
• CapabilityRoute and MainLayout loading gate: MainLayout blocks <Outlet /> first, so CapabilityRoute never renders before capabilities load. No ordering hazard. ✅
• No security controls (regex/escape/sanitize) in this PR. N/A for non-canonical试穿.

C3 — Authorization boundary ≠ capability boundary:

• This PR is UI-only gating. Backend endpoint authorization is a separate concern (acknowledged in PR notes). The UI capability checks are defense-in-depth UX, not the authorization boundary. ✅
• No new endpoints or tools exposed. N/A.

---

Verdict

No P0 or P1 findings. All identified issues are P2 (design gaps and maintainability concerns). The capability system is correctly implemented with proper loading states, error handling, persist-safe behavior, and consistent write-action guards across all pages.

[Octo-Q] verdict: APPROVE — with recommendations to address F1 (app-bots gating) and F2 (setManagerMe side effect) in a follow-up.

[yujiawei]

Code Review — PR #91 (octo-admin)

Reviewed at head SHA 1fb2ae1d8f7c81e347974d3a2da34f26e73ba771 against base main (merge-base 0c1f5e9). Verified locally: npm test (29 passed) and npm run build both green. This review combines line-level analysis with two independent second-opinion passes.

Verdict: APPROVED

Every gate this PR adds is fail-closed, it introduces no regression, and it only adds restrictions to the admin console. Nothing in the diff requires a change before merge. There is one important system-level caveat and two follow-ups, called out below — none of them block this frontend change.

1. What this PR does well

• Centralized, type-safe capability contract (src/auth/capabilities.ts). MANAGER_CAPABILITY_KEYS as a const tuple driving ManagerCapabilityKey means route gates and per-page checks are compile-time checked against the same key set. I verified there are no key typos: every CapabilityRoute capability="..." and every hasManagerCapability(..., 'key') references a defined key.
• Fail-closed throughout. normalizeManagerCapabilities (capabilities.ts:36) coerces anything that isn't strictly true to false; null capabilities render nothing in CapabilityRoute/ManagerIndexRoute (App.tsx:41-48); per-page handlers short-circuit with if (!canWrite) return. Worst case is an over-restriction (visible immediately), never an over-grant.
• Defense in depth on writes. Each mutating page guards both the rendered control (button/column hidden) and the handler entry (Download/index.tsx:126,131,146, Groups/index.tsx:80,92,98,105, Users/index.tsx:113, Spaces/index.tsx:158,176,192,216). Hiding alone would be weaker.
• Loading/error UX is handled. MainLayout.tsx suspends <Outlet/> behind a spinner while the profile loads and offers a retry on error, so a partial shell can't render before capabilities resolve.

2. Findings

🔴 MUST-VERIFY (human) — backend enforcement is the real gate

src/App.tsx, all src/pages/* handlers — This PR is UI gating, not authorization. Hiding routes/buttons and short-circuiting handlers does not stop a user from calling /v1/manager/* directly with a valid token. This is inherent to any frontend change and the PR description already flags it ("Backend endpoint ownership still needs separate confirmation"). For a security_sensitive PR this must be stated explicitly: the backend must derive capabilities server-side and return 403 on every protected read/write/destructive endpoint. If the backend does not yet enforce space.destructive, groups.write, users.write, appversion.write, dashboard.trigger, etc., this PR gives the appearance of least-privilege without the substance. This is not a defect in the diff — it is a system requirement a human owner should confirm before relying on these gates as a security boundary.

P2 — /v1/manager/me wire format must be real JSON booleans

src/auth/capabilities.ts:36 (capabilities?.[key] === true). The strict equality is correct and deliberately fail-closed, but this codebase frequently models booleans as integers from the backend (status === 1, is_destroy === 1, role === 2). If /v1/manager/me returns 1/0 for capabilities instead of true/false, every capability normalizes to false and all managers are locked out of everything (fail-closed lockout, not a breach). Newer endpoints in this repo do return real booleans (backup.enabled, dashboard.is_active), so this is plausibly fine — but /v1/manager/me is brand new and unverifiable from the frontend. Confirm the contract returns JSON true/false before/at rollout. Not a blocker; worst case is immediately visible.

P2 — app-bots route is feature-flag gated, not capability gated

src/App.tsx:221-228, src/layouts/MainLayout.tsx menu. Unlike every other admin route, /app-bots is wrapped only in ManagerAppBotsGate (the appBotsAvailable feature probe), with no CapabilityRoute. Both second-opinion passes flagged this. However, I confirmed this is pre-existing, not introduced here: at the merge-base the same route was already AppBotsGate fallback="/dashboard" with no capability check, and there is no app_bots capability key in the contract to gate against. So this PR neither regresses nor can fully fix it without a backend contract addition. Recommend a follow-up: add an app_bots.read/app_bots.write key to MANAGER_CAPABILITY_KEYS, wrap the route in CapabilityRoute, and gate the menu item. Out of scope for this PR.

P2 — /no-access and breadcrumb edge cases

src/layouts/MainLayout.tsx breadcrumb href={homePath} + App.tsx:229 no-access route. For a manager with zero readable capabilities, firstManagerPath returns /no-access; the breadcrumb stays clickable and lands on the dead-end page, and /no-access does not redirect away once capabilities load for a user who does have access. Minor UX polish — consider disabling the breadcrumb when homePath === MANAGER_NO_ACCESS_PATH. Non-blocking.

3. Cross-check on edge cases (verified, no action needed)

• firstManagerPath correctly handles a system_setting-only manager → /system-setting.
• A manager with appversion.write but not appversion.read is fail-closed (cannot reach /download); acceptable as long as the backend never issues write without read.
• useSpaceScope now reacts to managerCapabilities and gates canUpdateSpaceProfile / canChangeMemberRoles / destructive space actions; the SpaceDetailDrawer split of inactiveSpace vs profileReadOnly is correct.
• Zustand persist intentionally does not persist managerCapabilities (only token/scope/role), so on reload capabilities are re-fetched from /v1/manager/me and cannot go stale across sessions. Good.

4. Coverage note (what this review could NOT verify)

• Backend enforcement of any capability — not in this repo; this is the single most important thing for a human to confirm (see MUST-VERIFY above).
• Exact /v1/manager/me JSON contract (boolean vs integer flags, key spelling server-side).
• Whether leaving app-bots ungated for all managers is an intentional product decision.
• Runtime behavior of files outside the diff.

Two independent second-opinion passes were run for this security-sensitive PR; both completed and their findings are absorbed above (the app-bots and frontend-only points are consensus; I downgraded app-bots from blocking after verifying it is pre-existing and has no contract key).