diff --git a/app/web/router.py b/app/web/router.py index 842a7ce..2fd05fe 100644 --- a/app/web/router.py +++ b/app/web/router.py @@ -7,10 +7,10 @@ import logging import os from datetime import datetime from pathlib import Path -from typing import Literal, Optional +from typing import Optional from urllib.parse import quote -from fastapi import APIRouter, Depends, Query, Request, HTTPException +from fastapi import APIRouter, Depends, Request, HTTPException from fastapi.responses import HTMLResponse, RedirectResponse from fastapi.templating import Jinja2Templates import duckdb @@ -717,42 +717,31 @@ async def activity_center( @router.get("/setup", response_class=HTMLResponse) async def setup_page( request: Request, - role: Literal["analyst", "admin"] = Query(default="analyst"), user: Optional[dict] = Depends(get_optional_user), conn: duckdb.DuckDBPyConnection = Depends(_get_db), ): """Setup instructions for the local agent (CLI + Claude Code). - The `role` query param picks the layout: - - `analyst` (default) → trimmed workspace-bootstrap flow rendered by - `_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. + Single unified flow for everyone — admin-vs-analyst is no longer a + layout branch. The marketplace + plugins block appears iff the + caller has plugin grants in `resource_grants` (resolved inside + `compute_default_agent_prompt`). - When an admin override is saved, the override replaces the auto-generated - setup_instructions output everywhere (both the /setup page display and the - dashboard clipboard CTA). When no override is set, the live default from + When an admin override is saved, the override replaces the + auto-generated setup_instructions output everywhere (both the + /setup page display and the dashboard clipboard CTA). When no + override is set, the live default from setup_instructions.resolve_lines() is used. """ from src.repositories.welcome_template import WelcomeTemplateRepository from src.welcome_template import compute_default_agent_prompt, _sanitize_banner_html 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("/") # Determine the script text: override (Jinja2-rendered) or live default. - # The override is role-agnostic by design — admins who set an override are - # opting into the exact text they wrote, regardless of which tile the - # caller picked. Only the live default branches on `role`. + # The override is per-instance, applies to every caller — admins who set + # an override are opting into the exact text they wrote. row = WelcomeTemplateRepository(conn).get() override_content = row.get("content") if override_content: @@ -789,7 +778,6 @@ async def setup_page( # Override both variables so the partial and the JS array stay in sync. setup_instructions_lines=setup_instructions_lines, setup_script_text=setup_script_text, - role=role, ) return templates.TemplateResponse(request, "install.html", ctx) diff --git a/tests/test_setup_page_roles.py b/tests/test_setup_page_roles.py deleted file mode 100644 index adb44de..0000000 --- a/tests/test_setup_page_roles.py +++ /dev/null @@ -1,277 +0,0 @@ -"""Tests for /setup role query-param branching. - -Task 4 wires `?role=analyst|admin` through the /setup route handler so the -template can render two role tiles and the renderer can pick the right -layout (admin = full marketplace/skills/diagnose flow; analyst = trimmed -workspace-bootstrap flow). Default is `admin` to preserve existing behavior. -""" - -import pytest -from fastapi.testclient import TestClient - - -@pytest.fixture -def client(tmp_path, monkeypatch): - """TestClient against a freshly-built FastAPI app rooted at tmp_path. - - Mirrors the `web_client` fixture in tests/test_web_ui.py — we re-create - the app so the DuckDB singleton picks up the per-test DATA_DIR rather - than leaking state across tests on the same xdist worker. - """ - monkeypatch.setenv("DATA_DIR", str(tmp_path)) - monkeypatch.setenv("TESTING", "1") - monkeypatch.setenv("JWT_SECRET_KEY", "test-secret-key-min-32-characters!!") - (tmp_path / "state").mkdir() - (tmp_path / "analytics").mkdir() - (tmp_path / "extracts").mkdir() - from src.db import close_system_db - close_system_db() - from app.main import create_app - app = create_app() - yield TestClient(app) - close_system_db() - - -def test_setup_page_default_role_is_analyst(client): - """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) - assert resp.status_code == 200 - text = resp.text - # Analyst tile rendered; analyst layout is what the unauthenticated / - # non-admin caller gets by default. - assert "Analyst workspace" in text - assert "role=analyst" in text - - -def test_setup_page_analyst_role(client): - """`?role=analyst` → analyst tile is the active one.""" - resp = client.get("/setup?role=analyst", follow_redirects=True) - assert resp.status_code == 200 - text = resp.text - assert "Analyst workspace" in text - # The page must reflect the analyst selection somewhere — either via - # the active-state CSS class or the `role=analyst` link being rendered. - 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): - """`/install` legacy path keeps redirecting to `/setup` (302/307).""" - resp = client.get("/install", follow_redirects=False) - assert resp.status_code in (302, 307) - assert "/setup" in resp.headers["location"] - - -def test_setup_page_invalid_role_falls_back(client): - """Invalid role values must NOT 500 — either FastAPI's Literal - validation rejects with 422, or the route quietly falls back to admin. - Both are acceptable; what's not acceptable is an unhandled exception. - """ - resp = client.get("/setup?role=hacker", follow_redirects=True) - assert resp.status_code in (200, 422) - - -def test_setup_page_analyst_js_uses_bootstrap_scope(client): - """Analyst tile's setupNewClaude JS must mint bootstrap-analyst PATs. - - The JS PAT mint must be role-aware: analyst gets a short-TTL - bootstrap-analyst-scoped PAT (server clamps ttl ≤ 3600s), not the - historical 90-day general PAT. Asserts the wiring at the rendered - template level so we catch any regression in either the Jinja ctx - plumbing or the JS branching. - """ - resp = client.get("/setup?role=analyst", follow_redirects=True) - assert resp.status_code == 200 - text = resp.text - # The role variable must be set to analyst in JS scope. - assert ( - 'const ROLE = "analyst"' in text - or 'ROLE = "analyst"' in text - or 'data-role="analyst"' in text - ) - # The bootstrap-analyst scope must appear in the JS PAT-mint body. - assert "bootstrap-analyst" in text - assert "ttl_seconds" in text - - -def test_setup_page_admin_js_uses_general_scope(client): - """Admin tile's setupNewClaude JS must keep the existing 90-day - expires_in_days behavior — byte-identical PAT mint shape so existing - admin flows don't regress. - """ - resp = client.get("/setup?role=admin", follow_redirects=True) - assert resp.status_code == 200 - text = resp.text - assert "expires_in_days" in text # still present in the admin body - - -def test_setup_page_analyst_clipboard_renders_analyst_layout(client): - """The clipboard text the analyst tile produces must be the analyst layout - (`agnes init` + `agnes catalog`), NOT the admin layout (`claude plugin - marketplace add` + admin-only flow). - - The rendered HTML embeds the role-aware `setup_instructions_lines` text - into a JS array `SETUP_INSTRUCTIONS_TEMPLATE` (see - `_claude_setup_instructions.jinja`); `renderSetupInstructions` substitutes - `{server_url}` / `{token}` into that array at click time. So checking the - embedded array against the served HTML is sufficient — if it carries the - analyst layout, the clipboard payload will too. - - Pinning to the JS array block specifically (not the whole page) avoids - false positives from chrome / preview-mode
renders elsewhere.
- """
- import re
-
- resp = client.get("/setup?role=analyst", follow_redirects=True)
- assert resp.status_code == 200
- text = resp.text
-
- # Locate the JS array that holds the clipboard template body. The partial
- # emits `var SETUP_INSTRUCTIONS_TEMPLATE = [...].join("\n");` — match
- # everything between the `[` and the matching `]` so we can scope the
- # assertions to *just* what gets pasted into Claude Code, ignoring the
- # preview block that also embeds the lines.
- match = re.search(
- r"var\s+SETUP_INSTRUCTIONS_TEMPLATE\s*=\s*\[(.*?)\]\.join\(",
- text,
- re.DOTALL,
- )
- assert match, "SETUP_INSTRUCTIONS_TEMPLATE array not found in rendered HTML"
- clipboard_block = match.group(1)
-
- # Analyst layout markers MUST be in the clipboard block.
- assert "agnes init" in clipboard_block, (
- "Analyst clipboard payload missing `agnes init` — is "
- "compute_default_agent_prompt(role=role) being threaded into "
- "setup_instructions_lines on the /setup route?"
- )
- assert "agnes catalog" in clipboard_block, (
- "Analyst clipboard payload missing `agnes catalog` smoke verify"
- )
-
- # Admin-only markers MUST NOT be in the analyst clipboard block.
- # If they are, renderSetupInstructions is producing admin layout despite
- # the analyst tile being selected — the bug this test guards against.
- # `agnes auth import-token` is the admin login step (analyst uses
- # `agnes init` which bundles auth + workspace bootstrap).
- assert "agnes auth import-token" not in clipboard_block, (
- "Analyst clipboard block contains admin-only `agnes auth import-token` "
- "— role plumbing is broken; analyst sees admin instructions."
- )
- assert "agnes skills" not in clipboard_block, (
- "Analyst clipboard block contains admin-only skills setup step — "
- "analyst layout should not include skills management."
- )
-
-
-def test_setup_page_admin_clipboard_renders_admin_layout(client, monkeypatch):
- """Counterpart to the analyst test — admin caller asking for `?role=admin`
- 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
- 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)
- finally:
- client.app.dependency_overrides.pop(get_optional_user, None)
-
- assert resp.status_code == 200
- text = resp.text
-
- match = re.search(
- r"var\s+SETUP_INSTRUCTIONS_TEMPLATE\s*=\s*\[(.*?)\]\.join\(",
- text,
- re.DOTALL,
- )
- assert match, "SETUP_INSTRUCTIONS_TEMPLATE array not found in rendered HTML"
- clipboard_block = match.group(1)
-
- # Admin layout marker MUST be present.
- assert "agnes auth import-token" in clipboard_block, (
- "Admin clipboard payload missing `agnes auth import-token`"
- )
- assert "agnes skills" in clipboard_block, (
- "Admin clipboard payload missing the skills setup step"
- )
- # Analyst-only marker MUST NOT appear in admin layout.
- assert "agnes init" not in clipboard_block, (
- "Admin clipboard block leaked the analyst `agnes init` step"
- )
-
-
-def test_setup_page_role_is_json_escaped(client):
- """The `ROLE` JS const must be injected via Jinja `tojson` (defense in
- depth) — not as a bare `"{{ role }}"` string interpolation. This makes
- JS string-escaping explicit and removes the dependency on Jinja
- autoescape semantics for JS contexts.
- """
- resp = client.get("/setup?role=analyst", follow_redirects=True)
- assert resp.status_code == 200
- text = resp.text
- # tojson always emits double-quoted JSON: the rendered output is exactly
- # `const ROLE = "analyst";` (note: no extra space inside the quotes).
- assert 'const ROLE = "analyst";' in text
-
-
-def test_setup_page_js_ternary_keys_bootstrap_to_analyst(client):
- """Mutation-resistant assertion: the `bootstrap-analyst` scope must sit on
- the truthy branch of `ROLE === "analyst"`, not on the falsy branch.
-
- Without this, a silent inversion (`ROLE === "analyst"` ↔ `ROLE !== "analyst"`)
- would let analyst tile mint a general PAT and admin tile mint a
- bootstrap-scoped PAT — with both substrings still present in served HTML.
- The test passes whether the content is queried via either role URL since
- the JS ternary itself is identical across role responses.
- """
- import re
- resp = client.get("/setup?role=admin", follow_redirects=True) # JS body is role-independent
- assert resp.status_code == 200
- text = resp.text
- # Match the ternary `ROLE === "analyst" ? : `.
- # Allow whitespace/newlines, ensure `bootstrap-analyst` is in the truthy branch.
- pattern = re.compile(
- r'ROLE\s*===\s*"analyst"\s*\?\s*\{[^}]*bootstrap-analyst[^}]*\}\s*:\s*\{[^}]*expires_in_days[^}]*\}',
- re.DOTALL,
- )
- assert pattern.search(text), (
- "JS PAT-mint ternary must have `bootstrap-analyst` on the analyst (truthy) "
- "branch and `expires_in_days` on the admin (falsy) branch. A silent inversion "
- "would let the analyst tile mint a general PAT — exactly the regression we want to catch."
- )
diff --git a/tests/test_setup_page_unified.py b/tests/test_setup_page_unified.py
new file mode 100644
index 0000000..617da95
--- /dev/null
+++ b/tests/test_setup_page_unified.py
@@ -0,0 +1,114 @@
+"""Tests for the unified `/setup` route.
+
+The previous `?role=analyst|admin` query parameter is gone. The route
+renders a single layout for everyone — admin-vs-analyst is no longer a
+branch. The marketplace + plugins block is gated by per-user
+`resource_grants` resolved inside `compute_default_agent_prompt`.
+"""
+
+import pytest
+from fastapi.testclient import TestClient
+
+
+@pytest.fixture
+def client(tmp_path, monkeypatch):
+ """TestClient against a freshly-built FastAPI app rooted at tmp_path.
+
+ Mirrors the `web_client` fixture in tests/test_web_ui.py — we re-create
+ the app so the DuckDB singleton picks up the per-test DATA_DIR rather
+ than leaking state across tests on the same xdist worker.
+ """
+ monkeypatch.setenv("DATA_DIR", str(tmp_path))
+ monkeypatch.setenv("TESTING", "1")
+ monkeypatch.setenv("JWT_SECRET_KEY", "test-secret-key-min-32-characters!!")
+ (tmp_path / "state").mkdir()
+ (tmp_path / "analytics").mkdir()
+ (tmp_path / "extracts").mkdir()
+ from src.db import close_system_db
+ close_system_db()
+ from app.main import create_app
+ app = create_app()
+ yield TestClient(app)
+ close_system_db()
+
+
+def test_setup_page_renders_unified_layout(client):
+ """Bare `/setup` (no query param) renders the unified flow:
+
+ - `agnes init` is mandatory (subsumes the old admin-only
+ `agnes auth import-token` + `agnes auth whoami` pair).
+ - Anonymous visitors with no plugin grants get the no-marketplace
+ layout (Confirm = step 6).
+ """
+ resp = client.get("/setup", follow_redirects=True)
+ assert resp.status_code == 200
+ text = resp.text
+ # Unified flow markers.
+ assert "agnes init" in text
+ # Legacy admin-only login verbs are gone from the rendered prompt.
+ assert "agnes auth import-token" not in text
+ # No-marketplace layout: Confirm = step 6.
+ assert "6) Confirm:" in text
+
+
+def test_setup_page_ignores_role_query_param(client):
+ """`?role=...` is no longer accepted by the route signature. FastAPI
+ ignores unknown query params silently — `/setup?role=admin` still
+ serves the unified layout. No 422, no redirect, no behavior delta
+ vs. bare `/setup`."""
+ bare = client.get("/setup", follow_redirects=True)
+ with_role = client.get("/setup?role=admin", follow_redirects=True)
+ assert bare.status_code == 200
+ assert with_role.status_code == 200
+ # Both responses contain the unified-flow marker.
+ assert "agnes init" in bare.text
+ assert "agnes init" in with_role.text
+ # Legacy admin-only login verbs are gone from both.
+ assert "agnes auth import-token" not in bare.text
+ assert "agnes auth import-token" not in with_role.text
+
+
+def test_setup_page_renders_marketplace_for_user_with_grants(client, monkeypatch):
+ """When the caller has plugin grants in `resource_grants`, the
+ unified flow inserts the marketplace + plugins block (step 5) and
+ Confirm shifts to step 8.
+
+ Stub `marketplace_filter.resolve_allowed_plugins` to return a
+ plugin so we don't have to seed the full marketplace plumbing in
+ this test — we're verifying the layout switch, not the RBAC
+ resolver itself (covered by `test_marketplace_filter`)."""
+ from app.web.router import get_optional_user
+ from fastapi import Request
+ from src import marketplace_filter
+
+ async def _admin_user(request: Request): # type: ignore[no-redef]
+ return {"id": "admin-1", "email": "admin@example.com",
+ "is_admin": True, "name": "Admin", "groups": ["Admin"]}
+
+ monkeypatch.setattr(
+ marketplace_filter,
+ "resolve_allowed_plugins",
+ lambda conn, user: [{"manifest_name": "demo-plugin"}],
+ )
+
+ client.app.dependency_overrides[get_optional_user] = _admin_user
+ try:
+ resp = client.get("/setup", follow_redirects=True)
+ finally:
+ client.app.dependency_overrides.pop(get_optional_user, None)
+
+ assert resp.status_code == 200
+ text = resp.text
+ # Marketplace block markers.
+ assert "claude plugin install demo-plugin@agnes" in text
+ # Layout shift: Confirm is now step 8 (was 6 without marketplace).
+ assert "8) Confirm:" in text
+ # Pre-flight is in the rendered prompt at step 4.
+ assert "Make sure git and claude are installed" in text
+
+
+def test_install_legacy_path_redirects_to_setup(client):
+ """`/install` legacy path keeps redirecting to `/setup` (302/307)."""
+ resp = client.get("/install", follow_redirects=False)
+ assert resp.status_code in (302, 307)
+ assert "/setup" in resp.headers["location"]
diff --git a/tests/test_web_ui.py b/tests/test_web_ui.py
index 92aba84..bbcc6eb 100644
--- a/tests/test_web_ui.py
+++ b/tests/test_web_ui.py
@@ -214,9 +214,11 @@ class TestClaudeSetupPreview:
"""
def test_install_preview_visible_for_signed_in_user(self, web_client, 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)
+ # /setup is now a single unified flow regardless of caller's role.
+ # Admin sees the same layout as everyone else; the marketplace
+ # block appears iff the caller has plugin grants in
+ # `resource_grants` (the seeded admin in this fixture has none).
+ resp = web_client.get("/setup", cookies=admin_cookie)
assert resp.status_code == 200
body = resp.text
# Preview card + placeholder token render
@@ -229,24 +231,29 @@ class TestClaudeSetupPreview:
# because it validates the PEP 427 filename in the URL before fetch).
assert "/cli/wheel/" in body
assert "/cli/agnes.whl" not in body
- # Admin layout: numbered headers + diagnose step
+ # Unified layout: numbered headers + diagnose step
assert "1) Install the CLI" in body
assert "4) Run diagnostics" in body
assert "agnes diagnose" in body
- assert "agnes auth whoami" in body
+ # `agnes init` is now the mandatory bootstrap step.
+ assert "agnes init" in body
+ # The generated /setup prompt's "Log in" / "Verify the login"
+ # admin-only headers are gone (agnes init subsumes them).
+ # `agnes auth whoami` survives as a static manual-install
+ # example elsewhere on the page (not in the generated prompt).
+ assert "2) Log in" not in body
+ assert "3) Verify the login" not 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."""
+ def test_install_preview_unified_layout(self, web_client, admin_cookie):
+ """The clipboard payload (SETUP_INSTRUCTIONS_TEMPLATE JS array)
+ carries the unified layout for every caller — admin-vs-analyst
+ is no longer a layout branch. Without plugin grants, the
+ marketplace block is omitted (no `claude plugin marketplace
+ add` line)."""
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,
@@ -255,6 +262,12 @@ class TestClaudeSetupPreview:
clipboard = match.group(1)
assert "agnes init" in clipboard
assert "claude plugin marketplace add" not in clipboard
+ # Legacy admin-only auth verbs are gone from the generated prompt.
+ assert "agnes auth import-token" not in clipboard
+ # `agnes auth whoami` was the old admin step 3; subsumed by
+ # `agnes init` + `agnes catalog` smoke verify.
+ assert "3) Verify the login" not in clipboard
+ assert "2) Log in" not in clipboard
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