fix(setup): default /setup to analyst, hide admin tile from non-admins
Three coupled UX fixes for the analyst-onboarding flow: 1. Dashboard "Setup a new Claude Code" CTA was rendering admin paste prompt for everyone (analysts couldn't actually execute the marketplace plugin install / skills setup steps). render_agent_prompt_banner now picks role based on user.is_admin — analysts get the analyst flow. 2. /setup default role changed from admin to analyst. Most visitors are analysts; admin layout is opt-in via the admin tile or ?role=admin. 3. Admin tile is admin-only on the role-tile nav. Non-admins see only the analyst tile. Server-side: non-admin requesting ?role=admin is silently downgraded to analyst (otherwise they'd see admin paste prompt despite no tile). Tests: - New: test_setup_page_admin_tile_hidden_for_non_admin (anonymous client can't see "Admin CLI" or role=admin link) - New: test_setup_page_admin_role_downgraded_for_non_admin (anonymous ?role=admin → analyst layout, no marketplace step in clipboard) - New: test_install_preview_default_role_is_analyst (admin signing in to bare /setup gets analyst clipboard by default) - Renamed: test_setup_page_default_role_is_admin → ..._is_analyst - Updated: test_setup_page_admin_clipboard_renders_admin_layout uses FastAPI dependency_overrides to inject admin user (admin layout is now admin-gated) - Updated: test_install_preview_visible_for_signed_in_user explicitly passes ?role=admin to exercise admin layout
This commit is contained in:
parent
d8dc7c7799
commit
92d477e422
5 changed files with 104 additions and 32 deletions
|
|
@ -717,18 +717,19 @@ async def activity_center(
|
||||||
@router.get("/setup", response_class=HTMLResponse)
|
@router.get("/setup", response_class=HTMLResponse)
|
||||||
async def setup_page(
|
async def setup_page(
|
||||||
request: Request,
|
request: Request,
|
||||||
role: Literal["analyst", "admin"] = Query(default="admin"),
|
role: Literal["analyst", "admin"] = Query(default="analyst"),
|
||||||
user: Optional[dict] = Depends(get_optional_user),
|
user: Optional[dict] = Depends(get_optional_user),
|
||||||
conn: duckdb.DuckDBPyConnection = Depends(_get_db),
|
conn: duckdb.DuckDBPyConnection = Depends(_get_db),
|
||||||
):
|
):
|
||||||
"""Setup instructions for the local agent (CLI + Claude Code).
|
"""Setup instructions for the local agent (CLI + Claude Code).
|
||||||
|
|
||||||
The `role` query param picks the layout:
|
The `role` query param picks the layout:
|
||||||
- `admin` (default) → full marketplace + skills + diagnose flow,
|
- `analyst` (default) → trimmed workspace-bootstrap flow rendered by
|
||||||
byte-identical to the pre-Task-4 page for any caller that doesn't
|
|
||||||
pass `?role=`.
|
|
||||||
- `analyst` → trimmed workspace-bootstrap flow rendered by
|
|
||||||
`_resolve_analyst_lines` (no marketplace, no plugins).
|
`_resolve_analyst_lines` (no marketplace, no plugins).
|
||||||
|
- `admin` → full marketplace + skills + diagnose flow. Admin-only:
|
||||||
|
non-admin callers asking for `?role=admin` are silently downgraded
|
||||||
|
to `analyst` so the page never shows them instructions they can't
|
||||||
|
execute.
|
||||||
|
|
||||||
When an admin override is saved, the override replaces the auto-generated
|
When an admin override is saved, the override replaces the auto-generated
|
||||||
setup_instructions output everywhere (both the /setup page display and the
|
setup_instructions output everywhere (both the /setup page display and the
|
||||||
|
|
@ -739,6 +740,13 @@ async def setup_page(
|
||||||
from src.welcome_template import compute_default_agent_prompt, _sanitize_banner_html
|
from src.welcome_template import compute_default_agent_prompt, _sanitize_banner_html
|
||||||
from jinja2 import Environment, StrictUndefined, TemplateError
|
from jinja2 import Environment, StrictUndefined, TemplateError
|
||||||
|
|
||||||
|
# Gate the admin layout on user.is_admin. Non-admins requesting `?role=admin`
|
||||||
|
# silently fall back to analyst — admin instructions reference admin-only
|
||||||
|
# endpoints (marketplace registration, skills install) that a non-admin
|
||||||
|
# PAT can't authenticate against.
|
||||||
|
if role == "admin" and not (user and user.get("is_admin")):
|
||||||
|
role = "analyst"
|
||||||
|
|
||||||
base_url = str(request.base_url).rstrip("/")
|
base_url = str(request.base_url).rstrip("/")
|
||||||
|
|
||||||
# Determine the script text: override (Jinja2-rendered) or live default.
|
# Determine the script text: override (Jinja2-rendered) or live default.
|
||||||
|
|
|
||||||
|
|
@ -704,6 +704,8 @@
|
||||||
<main class="main">
|
<main class="main">
|
||||||
|
|
||||||
<!-- ═══════════════ ROLE TILES ═══════════════ -->
|
<!-- ═══════════════ ROLE TILES ═══════════════ -->
|
||||||
|
{# Admin tile is admin-only — non-admins see the analyst tile alone. #}
|
||||||
|
{% set _show_admin_tile = user and user.is_admin %}
|
||||||
<nav class="role-tiles" aria-label="Choose setup role">
|
<nav class="role-tiles" aria-label="Choose setup role">
|
||||||
<a href="/setup?role=analyst"
|
<a href="/setup?role=analyst"
|
||||||
class="role-tile{% if role == 'analyst' %} is-active{% endif %}"
|
class="role-tile{% if role == 'analyst' %} is-active{% endif %}"
|
||||||
|
|
@ -711,12 +713,14 @@
|
||||||
<h3>Analyst workspace</h3>
|
<h3>Analyst workspace</h3>
|
||||||
<p>Bootstrap a workspace folder with CLAUDE.md, hooks, and synced data.</p>
|
<p>Bootstrap a workspace folder with CLAUDE.md, hooks, and synced data.</p>
|
||||||
</a>
|
</a>
|
||||||
|
{% if _show_admin_tile %}
|
||||||
<a href="/setup?role=admin"
|
<a href="/setup?role=admin"
|
||||||
class="role-tile{% if role != 'analyst' %} is-active{% endif %}"
|
class="role-tile{% if role != 'analyst' %} is-active{% endif %}"
|
||||||
{% if role != 'analyst' %}aria-current="page"{% endif %}>
|
{% if role != 'analyst' %}aria-current="page"{% endif %}>
|
||||||
<h3>Admin CLI</h3>
|
<h3>Admin CLI</h3>
|
||||||
<p>Install the CLI, register the marketplace, set up admin tooling.</p>
|
<p>Install the CLI, register the marketplace, set up admin tooling.</p>
|
||||||
</a>
|
</a>
|
||||||
|
{% endif %}
|
||||||
</nav>
|
</nav>
|
||||||
|
|
||||||
<!-- ═══════════════ ADMIN BANNER (optional) ═══════════════ -->
|
<!-- ═══════════════ ADMIN BANNER (optional) ═══════════════ -->
|
||||||
|
|
|
||||||
|
|
@ -244,4 +244,11 @@ def render_agent_prompt_banner(
|
||||||
# Fall through to default
|
# Fall through to default
|
||||||
|
|
||||||
# No override (or broken override) — return live default bash script.
|
# No override (or broken override) — return live default bash script.
|
||||||
return compute_default_agent_prompt(conn, user=user, server_url=server_url)
|
# Pick role by user identity: admins get the full CLI install + marketplace
|
||||||
|
# flow (existing behavior). Everyone else gets the analyst workspace
|
||||||
|
# bootstrap. The dashboard CTA hits this path; without role-by-identity,
|
||||||
|
# analysts would get admin instructions they can't actually execute.
|
||||||
|
role = "admin" if (user and user.get("is_admin")) else "analyst"
|
||||||
|
return compute_default_agent_prompt(
|
||||||
|
conn, user=user, server_url=server_url, role=role,
|
||||||
|
)
|
||||||
|
|
|
||||||
|
|
@ -32,19 +32,16 @@ def client(tmp_path, monkeypatch):
|
||||||
close_system_db()
|
close_system_db()
|
||||||
|
|
||||||
|
|
||||||
def test_setup_page_default_role_is_admin(client):
|
def test_setup_page_default_role_is_analyst(client):
|
||||||
"""No `role` query param → admin layout (default, preserves existing flow)."""
|
"""No `role` query param → analyst layout (most users are analysts;
|
||||||
|
admin layout is opt-in via the admin tile, which only renders to admins)."""
|
||||||
resp = client.get("/setup", follow_redirects=True)
|
resp = client.get("/setup", follow_redirects=True)
|
||||||
assert resp.status_code == 200
|
assert resp.status_code == 200
|
||||||
text = resp.text
|
text = resp.text
|
||||||
# Both tiles present in markup; admin tile is the active one.
|
# Analyst tile rendered; analyst layout is what the unauthenticated /
|
||||||
assert "role=analyst" in text
|
# non-admin caller gets by default.
|
||||||
assert "role=admin" in text or 'href="/setup"' in text
|
|
||||||
# Active state lives on the admin tile when role=admin (default).
|
|
||||||
# Asserting the tile labels are both rendered keeps the assertion
|
|
||||||
# robust against future styling tweaks.
|
|
||||||
assert "Analyst workspace" in text
|
assert "Analyst workspace" in text
|
||||||
assert "Admin CLI" in text
|
assert "role=analyst" in text
|
||||||
|
|
||||||
|
|
||||||
def test_setup_page_analyst_role(client):
|
def test_setup_page_analyst_role(client):
|
||||||
|
|
@ -53,12 +50,35 @@ def test_setup_page_analyst_role(client):
|
||||||
assert resp.status_code == 200
|
assert resp.status_code == 200
|
||||||
text = resp.text
|
text = resp.text
|
||||||
assert "Analyst workspace" in text
|
assert "Analyst workspace" in text
|
||||||
assert "Admin CLI" in text
|
|
||||||
# The page must reflect the analyst selection somewhere — either via
|
# The page must reflect the analyst selection somewhere — either via
|
||||||
# the active-state CSS class or the `role=analyst` link being rendered.
|
# the active-state CSS class or the `role=analyst` link being rendered.
|
||||||
assert "role=analyst" in text
|
assert "role=analyst" in text
|
||||||
|
|
||||||
|
|
||||||
|
def test_setup_page_admin_tile_hidden_for_non_admin(client):
|
||||||
|
"""Non-admin caller (anonymous in this test) must NOT see the admin tile —
|
||||||
|
the admin paste prompt references admin-only endpoints (marketplace
|
||||||
|
registration, skills install) that a non-admin PAT can't authenticate
|
||||||
|
against, so showing it would lead to a confusing failure.
|
||||||
|
"""
|
||||||
|
resp = client.get("/setup", follow_redirects=True)
|
||||||
|
assert resp.status_code == 200
|
||||||
|
assert "Admin CLI" not in resp.text
|
||||||
|
assert "role=admin" not in resp.text
|
||||||
|
|
||||||
|
|
||||||
|
def test_setup_page_admin_role_downgraded_for_non_admin(client):
|
||||||
|
"""Non-admin requesting `?role=admin` is silently downgraded to analyst.
|
||||||
|
The page must NOT render admin instructions (no `claude plugin marketplace
|
||||||
|
add` in the rendered prompt) for someone who can't execute them."""
|
||||||
|
resp = client.get("/setup?role=admin", follow_redirects=True)
|
||||||
|
assert resp.status_code == 200
|
||||||
|
# Admin-only steps must NOT appear (would surface admin paste prompt).
|
||||||
|
assert "claude plugin marketplace add" not in resp.text
|
||||||
|
# Analyst-only step IS present (downgrade landed on analyst layout).
|
||||||
|
assert "agnes init" in resp.text
|
||||||
|
|
||||||
|
|
||||||
def test_install_redirects_to_setup(client):
|
def test_install_redirects_to_setup(client):
|
||||||
"""`/install` legacy path keeps redirecting to `/setup` (302/307)."""
|
"""`/install` legacy path keeps redirecting to `/setup` (302/307)."""
|
||||||
resp = client.get("/install", follow_redirects=False)
|
resp = client.get("/install", follow_redirects=False)
|
||||||
|
|
@ -168,13 +188,30 @@ def test_setup_page_analyst_clipboard_renders_analyst_layout(client):
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
def test_setup_page_admin_clipboard_renders_admin_layout(client):
|
def test_setup_page_admin_clipboard_renders_admin_layout(client, monkeypatch):
|
||||||
"""Counterpart to the analyst test — admin tile MUST keep the existing
|
"""Counterpart to the analyst test — admin caller asking for `?role=admin`
|
||||||
full marketplace/plugins flow byte-equivalent (no regression from Task 4).
|
sees the full marketplace/plugins flow.
|
||||||
|
|
||||||
|
Admin layout is now admin-gated (non-admins are silently downgraded to
|
||||||
|
analyst). To exercise the admin path, monkeypatch `get_optional_user` to
|
||||||
|
return an admin user dict. This avoids spinning up a full session-cookie
|
||||||
|
fixture for one assertion.
|
||||||
"""
|
"""
|
||||||
import re
|
import re
|
||||||
|
from app.web.router import get_optional_user
|
||||||
|
from fastapi import Request
|
||||||
|
|
||||||
|
async def _admin_user(request: Request): # type: ignore[no-redef]
|
||||||
|
return {"id": "admin-1", "email": "admin@example.com",
|
||||||
|
"is_admin": True, "name": "Admin"}
|
||||||
|
|
||||||
|
# Override the FastAPI dependency on the running app.
|
||||||
|
client.app.dependency_overrides[get_optional_user] = _admin_user
|
||||||
|
try:
|
||||||
resp = client.get("/setup?role=admin", follow_redirects=True)
|
resp = client.get("/setup?role=admin", follow_redirects=True)
|
||||||
|
finally:
|
||||||
|
client.app.dependency_overrides.pop(get_optional_user, None)
|
||||||
|
|
||||||
assert resp.status_code == 200
|
assert resp.status_code == 200
|
||||||
text = resp.text
|
text = resp.text
|
||||||
|
|
||||||
|
|
@ -186,21 +223,14 @@ def test_setup_page_admin_clipboard_renders_admin_layout(client):
|
||||||
assert match, "SETUP_INSTRUCTIONS_TEMPLATE array not found in rendered HTML"
|
assert match, "SETUP_INSTRUCTIONS_TEMPLATE array not found in rendered HTML"
|
||||||
clipboard_block = match.group(1)
|
clipboard_block = match.group(1)
|
||||||
|
|
||||||
# Admin layout marker MUST be present. `agnes auth import-token` is the
|
# Admin layout marker MUST be present.
|
||||||
# admin login step (analyst replaces it with `agnes init`); `agnes skills`
|
|
||||||
# is admin-only post-auth setup. Either one anchors the admin layout
|
|
||||||
# without depending on RBAC plugin grants (which the unauthenticated
|
|
||||||
# TestClient won't have).
|
|
||||||
assert "agnes auth import-token" in clipboard_block, (
|
assert "agnes auth import-token" in clipboard_block, (
|
||||||
"Admin clipboard payload missing `agnes auth import-token` — "
|
"Admin clipboard payload missing `agnes auth import-token`"
|
||||||
"Task 4 should not have changed admin behavior."
|
|
||||||
)
|
)
|
||||||
assert "agnes skills" in clipboard_block, (
|
assert "agnes skills" in clipboard_block, (
|
||||||
"Admin clipboard payload missing the skills setup step"
|
"Admin clipboard payload missing the skills setup step"
|
||||||
)
|
)
|
||||||
# Analyst-only marker MUST NOT appear in admin layout. `agnes init` is
|
# Analyst-only marker MUST NOT appear in admin layout.
|
||||||
# the analyst-only auth + workspace bootstrap; admin uses
|
|
||||||
# `agnes auth import-token` instead.
|
|
||||||
assert "agnes init" not in clipboard_block, (
|
assert "agnes init" not in clipboard_block, (
|
||||||
"Admin clipboard block leaked the analyst `agnes init` step"
|
"Admin clipboard block leaked the analyst `agnes init` step"
|
||||||
)
|
)
|
||||||
|
|
|
||||||
|
|
@ -214,7 +214,9 @@ class TestClaudeSetupPreview:
|
||||||
"""
|
"""
|
||||||
|
|
||||||
def test_install_preview_visible_for_signed_in_user(self, web_client, admin_cookie):
|
def test_install_preview_visible_for_signed_in_user(self, web_client, admin_cookie):
|
||||||
resp = web_client.get("/setup", cookies=admin_cookie)
|
# /setup defaults to ?role=analyst (the common visitor case).
|
||||||
|
# Admin can switch to ?role=admin to see the marketplace + plugins flow.
|
||||||
|
resp = web_client.get("/setup?role=admin", cookies=admin_cookie)
|
||||||
assert resp.status_code == 200
|
assert resp.status_code == 200
|
||||||
body = resp.text
|
body = resp.text
|
||||||
# Preview card + placeholder token render
|
# Preview card + placeholder token render
|
||||||
|
|
@ -227,12 +229,33 @@ class TestClaudeSetupPreview:
|
||||||
# because it validates the PEP 427 filename in the URL before fetch).
|
# because it validates the PEP 427 filename in the URL before fetch).
|
||||||
assert "/cli/wheel/" in body
|
assert "/cli/wheel/" in body
|
||||||
assert "/cli/agnes.whl" not in body
|
assert "/cli/agnes.whl" not in body
|
||||||
# New numbered headers + agnes diagnose step
|
# Admin layout: numbered headers + diagnose step
|
||||||
assert "1) Install the CLI" in body
|
assert "1) Install the CLI" in body
|
||||||
assert "4) Run diagnostics" in body
|
assert "4) Run diagnostics" in body
|
||||||
assert "agnes diagnose" in body
|
assert "agnes diagnose" in body
|
||||||
assert "agnes auth whoami" in body
|
assert "agnes auth whoami" in body
|
||||||
|
|
||||||
|
def test_install_preview_default_role_is_analyst(self, web_client, admin_cookie):
|
||||||
|
"""Bare /setup defaults to analyst layout — even for admin users.
|
||||||
|
Admin opts in via the admin tile (?role=admin) when they want the
|
||||||
|
marketplace/plugins flow."""
|
||||||
|
import re
|
||||||
|
resp = web_client.get("/setup", cookies=admin_cookie)
|
||||||
|
assert resp.status_code == 200
|
||||||
|
body = resp.text
|
||||||
|
# The clipboard payload (SETUP_INSTRUCTIONS_TEMPLATE JS array)
|
||||||
|
# carries the analyst layout. Admin tile label "Admin CLI" still
|
||||||
|
# appears as a tile heading (admin user sees both tiles), but the
|
||||||
|
# default rendered prompt is the analyst flow.
|
||||||
|
match = re.search(
|
||||||
|
r"var\s+SETUP_INSTRUCTIONS_TEMPLATE\s*=\s*\[(.*?)\]\.join\(",
|
||||||
|
body, re.DOTALL,
|
||||||
|
)
|
||||||
|
assert match, "SETUP_INSTRUCTIONS_TEMPLATE array missing"
|
||||||
|
clipboard = match.group(1)
|
||||||
|
assert "agnes init" in clipboard
|
||||||
|
assert "claude plugin marketplace add" not in clipboard
|
||||||
|
|
||||||
def test_dashboard_setup_cta_links_to_setup(self, web_client, admin_cookie):
|
def test_dashboard_setup_cta_links_to_setup(self, web_client, admin_cookie):
|
||||||
"""Dashboard setup CTA shows env-setup-cta and a link to /setup instead
|
"""Dashboard setup CTA shows env-setup-cta and a link to /setup instead
|
||||||
of an inline collapsed preview."""
|
of an inline collapsed preview."""
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue