fix(devin-review): address 4 findings on PR #167
- Fix #1: _detect_existing_project now checks .claude/settings.json for "da sync" marker instead of deleted CLAUDE.md; update tests accordingly. - Fix #2: preview endpoint uses autoescape=False to match /setup rendering; align render_agent_prompt_banner in welcome_template.py to the same. - Fix #3: apply _sanitize_banner_html to override render path in setup_page so all render paths sanitize consistently. - Fix #4: move .setup-link-banner into the existing-user branch where account_details.last_sync_display is reachable; remove dead copy from new-user branch.
This commit is contained in:
parent
26dc367037
commit
61ef0d0eed
6 changed files with 62 additions and 39 deletions
|
|
@ -119,7 +119,9 @@ async def admin_preview_template(
|
||||||
"""Render arbitrary template content against the live context for the
|
"""Render arbitrary template content against the live context for the
|
||||||
calling admin, without persisting. Used by the /admin/agent-prompt editor's
|
calling admin, without persisting. Used by the /admin/agent-prompt editor's
|
||||||
Preview button so admins can see their edits before saving."""
|
Preview button so admins can see their edits before saving."""
|
||||||
env = Environment(undefined=StrictUndefined, autoescape=True)
|
# autoescape=False to match /setup rendering — the outer Jinja2 template
|
||||||
|
# applies escaping where needed.
|
||||||
|
env = Environment(undefined=StrictUndefined, autoescape=False)
|
||||||
try:
|
try:
|
||||||
template = env.from_string(payload.content)
|
template = env.from_string(payload.content)
|
||||||
ctx = build_context(
|
ctx = build_context(
|
||||||
|
|
|
||||||
|
|
@ -734,7 +734,7 @@ async def setup_page(
|
||||||
setup_instructions.resolve_lines() is used.
|
setup_instructions.resolve_lines() is used.
|
||||||
"""
|
"""
|
||||||
from src.repositories.welcome_template import WelcomeTemplateRepository
|
from src.repositories.welcome_template import WelcomeTemplateRepository
|
||||||
from src.welcome_template import compute_default_agent_prompt
|
from src.welcome_template import compute_default_agent_prompt, _sanitize_banner_html
|
||||||
from jinja2 import Environment, StrictUndefined, TemplateError
|
from jinja2 import Environment, StrictUndefined, TemplateError
|
||||||
|
|
||||||
base_url = str(request.base_url).rstrip("/")
|
base_url = str(request.base_url).rstrip("/")
|
||||||
|
|
@ -751,7 +751,7 @@ async def setup_page(
|
||||||
env = Environment(undefined=StrictUndefined, autoescape=False)
|
env = Environment(undefined=StrictUndefined, autoescape=False)
|
||||||
template = env.from_string(override_content)
|
template = env.from_string(override_content)
|
||||||
ctx_vars = _build_banner_ctx(user=user, server_url=base_url)
|
ctx_vars = _build_banner_ctx(user=user, server_url=base_url)
|
||||||
setup_script_text = template.render(**ctx_vars)
|
setup_script_text = _sanitize_banner_html(template.render(**ctx_vars))
|
||||||
except (TemplateError, Exception) as exc:
|
except (TemplateError, Exception) as exc:
|
||||||
logger.warning("setup_page: override render failed (%s); falling back to default", exc)
|
logger.warning("setup_page: override render failed (%s); falling back to default", exc)
|
||||||
setup_script_text = compute_default_agent_prompt(conn, user=user, server_url=base_url)
|
setup_script_text = compute_default_agent_prompt(conn, user=user, server_url=base_url)
|
||||||
|
|
|
||||||
|
|
@ -2334,6 +2334,12 @@
|
||||||
</div><!-- /right-column -->
|
</div><!-- /right-column -->
|
||||||
</div><!-- /dashboard-grid -->
|
</div><!-- /dashboard-grid -->
|
||||||
|
|
||||||
|
{% if account_details and account_details.last_sync_display %}
|
||||||
|
<div class="setup-link-banner">
|
||||||
|
Need to set up another machine? <a href="/setup">Open the setup page →</a>
|
||||||
|
</div>
|
||||||
|
{% endif %}
|
||||||
|
|
||||||
</main>
|
</main>
|
||||||
|
|
||||||
{% else %}
|
{% else %}
|
||||||
|
|
@ -2427,12 +2433,6 @@
|
||||||
</div>
|
</div>
|
||||||
</div>
|
</div>
|
||||||
|
|
||||||
{% if account_details and account_details.last_sync_display %}
|
|
||||||
<div class="setup-link-banner">
|
|
||||||
Need to set up another machine? <a href="/setup">Open the setup page →</a>
|
|
||||||
</div>
|
|
||||||
{% endif %}
|
|
||||||
|
|
||||||
<div class="support-banner" style="margin-top: 16px;">
|
<div class="support-banner" style="margin-top: 16px;">
|
||||||
{{ config.INSTANCE_NAME }} - need help? Contact your platform team.
|
{{ config.INSTANCE_NAME }} - need help? Contact your platform team.
|
||||||
</div>
|
</div>
|
||||||
|
|
|
||||||
|
|
@ -17,16 +17,16 @@ analyst_app = typer.Typer(help="Analyst workspace bootstrap and status")
|
||||||
# Helper: detect existing workspace
|
# Helper: detect existing workspace
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
|
|
||||||
_CLAUDE_MD_MARKER = "AI Data Analyst"
|
|
||||||
|
|
||||||
|
|
||||||
def _detect_existing_project(workspace: Path) -> bool:
|
def _detect_existing_project(workspace: Path) -> bool:
|
||||||
"""Return True if CLAUDE.md with the analyst identifier already exists."""
|
"""Return True if .claude/settings.json with the agnes da-sync hooks exists."""
|
||||||
claude_md = workspace / "CLAUDE.md"
|
settings = workspace / ".claude" / "settings.json"
|
||||||
if claude_md.exists():
|
if not settings.exists():
|
||||||
content = claude_md.read_text(encoding="utf-8")
|
|
||||||
return _CLAUDE_MD_MARKER in content
|
|
||||||
return False
|
return False
|
||||||
|
try:
|
||||||
|
content = settings.read_text(encoding="utf-8")
|
||||||
|
except OSError:
|
||||||
|
return False
|
||||||
|
return "da sync" in content
|
||||||
|
|
||||||
|
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
|
|
|
||||||
|
|
@ -7,7 +7,7 @@ script (TLS trust, CLI install, login, marketplace, skills). When an
|
||||||
override is saved it replaces the default everywhere: both the /setup page
|
override is saved it replaces the default everywhere: both the /setup page
|
||||||
display and the dashboard clipboard CTA.
|
display and the dashboard clipboard CTA.
|
||||||
|
|
||||||
Override content is a Jinja2 template (autoescape=True, StrictUndefined).
|
Override content is a Jinja2 template (autoescape=False, StrictUndefined).
|
||||||
Available placeholders: instance.{name,subtitle}, server.{url,hostname},
|
Available placeholders: instance.{name,subtitle}, server.{url,hostname},
|
||||||
user (may be None for anonymous visitors), now, today.
|
user (may be None for anonymous visitors), now, today.
|
||||||
|
|
||||||
|
|
@ -215,9 +215,11 @@ def render_agent_prompt_banner(
|
||||||
content = row.get("content")
|
content = row.get("content")
|
||||||
|
|
||||||
if content:
|
if content:
|
||||||
# Admin-authored override — render as Jinja2 HTML, sanitize.
|
# Admin-authored override — render as Jinja2, sanitize.
|
||||||
|
# autoescape=False to match /setup rendering — the outer Jinja2 template
|
||||||
|
# applies escaping where needed.
|
||||||
try:
|
try:
|
||||||
env = Environment(undefined=StrictUndefined, autoescape=True)
|
env = Environment(undefined=StrictUndefined, autoescape=False)
|
||||||
template = env.from_string(content)
|
template = env.from_string(content)
|
||||||
ctx = build_context(user=user, server_url=server_url)
|
ctx = build_context(user=user, server_url=server_url)
|
||||||
rendered = template.render(**ctx)
|
rendered = template.render(**ctx)
|
||||||
|
|
|
||||||
|
|
@ -34,45 +34,64 @@ def tmp_workspace(tmp_path, monkeypatch):
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
|
|
||||||
class TestDetectExistingProject:
|
class TestDetectExistingProject:
|
||||||
def test_no_claude_md_returns_false(self, tmp_workspace):
|
def test_no_settings_json_returns_false(self, tmp_workspace):
|
||||||
from cli.commands.analyst import _detect_existing_project
|
from cli.commands.analyst import _detect_existing_project
|
||||||
|
|
||||||
assert _detect_existing_project(tmp_workspace) is False
|
assert _detect_existing_project(tmp_workspace) is False
|
||||||
|
|
||||||
def test_claude_md_with_marker_returns_true(self, tmp_workspace):
|
def test_settings_json_with_da_sync_returns_true(self, tmp_workspace):
|
||||||
from cli.commands.analyst import _detect_existing_project
|
from cli.commands.analyst import _detect_existing_project
|
||||||
|
import json as _json
|
||||||
|
|
||||||
(tmp_workspace / "CLAUDE.md").write_text(
|
claude_dir = tmp_workspace / ".claude"
|
||||||
"# Acme — AI Data Analyst\n\nThis workspace is connected to http://localhost:8000.\n",
|
claude_dir.mkdir(parents=True, exist_ok=True)
|
||||||
encoding="utf-8",
|
settings = {
|
||||||
)
|
"hooks": {
|
||||||
|
"SessionStart": [{"hooks": [{"type": "command", "command": "da sync --quiet 2>/dev/null || true"}]}],
|
||||||
|
}
|
||||||
|
}
|
||||||
|
(claude_dir / "settings.json").write_text(_json.dumps(settings), encoding="utf-8")
|
||||||
assert _detect_existing_project(tmp_workspace) is True
|
assert _detect_existing_project(tmp_workspace) is True
|
||||||
|
|
||||||
def test_claude_md_without_marker_returns_false(self, tmp_workspace):
|
def test_settings_json_without_da_sync_returns_false(self, tmp_workspace):
|
||||||
from cli.commands.analyst import _detect_existing_project
|
from cli.commands.analyst import _detect_existing_project
|
||||||
|
import json as _json
|
||||||
|
|
||||||
(tmp_workspace / "CLAUDE.md").write_text(
|
claude_dir = tmp_workspace / ".claude"
|
||||||
"# Some Other Project\n\nNot an analyst workspace.\n",
|
claude_dir.mkdir(parents=True, exist_ok=True)
|
||||||
encoding="utf-8",
|
(claude_dir / "settings.json").write_text(
|
||||||
|
_json.dumps({"model": "sonnet"}), encoding="utf-8"
|
||||||
)
|
)
|
||||||
assert _detect_existing_project(tmp_workspace) is False
|
assert _detect_existing_project(tmp_workspace) is False
|
||||||
|
|
||||||
def test_setup_blocked_when_existing_without_force(self, tmp_workspace):
|
def test_setup_blocked_when_existing_without_force(self, tmp_workspace):
|
||||||
"""Setup must exit(1) when workspace exists and --force not supplied."""
|
"""Setup must exit(1) when workspace exists and --force not supplied."""
|
||||||
(tmp_workspace / "CLAUDE.md").write_text(
|
import json as _json
|
||||||
"# Acme — AI Data Analyst\nThis workspace is connected to http://localhost:8000.\n",
|
|
||||||
encoding="utf-8",
|
claude_dir = tmp_workspace / ".claude"
|
||||||
)
|
claude_dir.mkdir(parents=True, exist_ok=True)
|
||||||
|
settings = {
|
||||||
|
"hooks": {
|
||||||
|
"SessionStart": [{"hooks": [{"type": "command", "command": "da sync --quiet 2>/dev/null || true"}]}],
|
||||||
|
}
|
||||||
|
}
|
||||||
|
(claude_dir / "settings.json").write_text(_json.dumps(settings), encoding="utf-8")
|
||||||
result = runner.invoke(app, ["analyst", "setup", "--server-url", "http://localhost:8000"])
|
result = runner.invoke(app, ["analyst", "setup", "--server-url", "http://localhost:8000"])
|
||||||
assert result.exit_code == 1
|
assert result.exit_code == 1
|
||||||
assert "force" in result.output.lower() or "force" in (result.stderr or "").lower()
|
assert "force" in result.output.lower() or "force" in (result.stderr or "").lower()
|
||||||
|
|
||||||
def test_setup_proceeds_with_force(self, tmp_workspace):
|
def test_setup_proceeds_with_force(self, tmp_workspace):
|
||||||
"""--force bypasses existing-project detection."""
|
"""--force bypasses existing-project detection."""
|
||||||
(tmp_workspace / "CLAUDE.md").write_text(
|
import json as _json
|
||||||
"# Acme — AI Data Analyst\nThis workspace is connected to http://localhost:8000.\n",
|
|
||||||
encoding="utf-8",
|
claude_dir = tmp_workspace / ".claude"
|
||||||
)
|
claude_dir.mkdir(parents=True, exist_ok=True)
|
||||||
|
settings = {
|
||||||
|
"hooks": {
|
||||||
|
"SessionStart": [{"hooks": [{"type": "command", "command": "da sync --quiet 2>/dev/null || true"}]}],
|
||||||
|
}
|
||||||
|
}
|
||||||
|
(claude_dir / "settings.json").write_text(_json.dumps(settings), encoding="utf-8")
|
||||||
|
|
||||||
with patch("cli.commands.analyst._connect_to_instance", return_value="tok"), \
|
with patch("cli.commands.analyst._connect_to_instance", return_value="tok"), \
|
||||||
patch("cli.commands.analyst._download_metadata"), \
|
patch("cli.commands.analyst._download_metadata"), \
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue