From 68ebc7114a168257cf7abe02bc787ded71a8b56b Mon Sep 17 00:00:00 2001 From: Jer Miller Date: Fri, 5 Jun 2026 22:10:12 -0600 Subject: [PATCH] refactor(convey): route three read surfaces through respond_collection Bring the activities day list, settings sol-voice throttled log, and tokens daily series into line with docs/CONVEY.md HTTP API conventions: return the {items, total} envelope instead of bare top-level arrays, and replace the tokens /api/daily abort(400) calls with INVALID_REQUEST_VALUE error envelopes. Each producer ships with its in-repo HTML/JS consumer updated to read .items, so behavior is preserved. Drop the four now-cleared scripts/check_api_conventions.py allowlist entries, update the unit tests that asserted the old bare-array shape, add an app-local HTTP test for the activities day route, and refresh the two in-process API baselines (day-activities, daily) that make ci's test_api_baselines validates. Co-Authored-By: Claude Opus 4.8 (1M context) --- scripts/check_api_conventions.py | 4 - solstone/apps/activities/_day.html | 3 +- solstone/apps/activities/routes.py | 11 +- solstone/apps/activities/tests/conftest.py | 13 ++ solstone/apps/activities/tests/test_routes.py | 39 +++++ solstone/apps/settings/routes.py | 6 +- .../settings/tests/test_sol_voice_routes.py | 6 +- solstone/apps/settings/workspace.html | 4 +- solstone/apps/tokens/routes.py | 15 +- solstone/apps/tokens/tests/test_routes.py | 19 ++- solstone/apps/tokens/workspace.html | 2 +- .../api/activities/day-activities.json | 5 +- tests/baselines/api/tokens/daily.json | 147 +++++++++--------- tests/test_app_activities.py | 10 +- 14 files changed, 181 insertions(+), 103 deletions(-) create mode 100644 solstone/apps/activities/tests/test_routes.py diff --git a/scripts/check_api_conventions.py b/scripts/check_api_conventions.py index 1308bccbc..30ec8d30f 100644 --- a/scripts/check_api_conventions.py +++ b/scripts/check_api_conventions.py @@ -79,15 +79,11 @@ ROUTE_DECORATORS: frozenset[str] = frozenset( # (posix-relative-path, kind) -> allowed count. Ratchets toward empty: lower a # count as occurrences are fixed; never raise one to admit a new violation. ALLOWLIST: dict[tuple[str, str], int] = { - ("solstone/apps/activities/routes.py", "bare-array"): 1, ("solstone/apps/import/routes.py", "abort"): 2, ("solstone/apps/import/routes.py", "bare-array"): 1, ("solstone/apps/link/routes.py", "abort"): 1, ("solstone/apps/observer/routes.py", "bare-array"): 2, - ("solstone/apps/settings/routes.py", "bare-array"): 1, ("solstone/apps/todos/routes.py", "bare-return"): 5, - ("solstone/apps/tokens/routes.py", "abort"): 2, - ("solstone/apps/tokens/routes.py", "bare-array"): 1, ("solstone/convey/services_scout.py", "abort"): 1, ("solstone/convey/services_scout.py", "inline-error"): 2, } diff --git a/solstone/apps/activities/_day.html b/solstone/apps/activities/_day.html index 53c7982f0..fe7ad0073 100644 --- a/solstone/apps/activities/_day.html +++ b/solstone/apps/activities/_day.html @@ -976,7 +976,8 @@ button.activity-detail-back { async function loadData() { try { - const acts = await window.apiJson(`/app/activities/api/day/${day}/activities`); + const data = await window.apiJson(`/app/activities/api/day/${day}/activities`); + const acts = data?.items; if (!Array.isArray(acts)) { throw new window.ApiError({ status: 200, diff --git a/solstone/apps/activities/routes.py b/solstone/apps/activities/routes.py index d8d25989e..2d550357f 100644 --- a/solstone/apps/activities/routes.py +++ b/solstone/apps/activities/routes.py @@ -18,7 +18,12 @@ from solstone.convey.reasons import ( INVALID_MONTH, INVALID_PATH, ) -from solstone.convey.utils import DATE_RE, error_response, format_date +from solstone.convey.utils import ( + DATE_RE, + error_response, + format_date, + respond_collection, +) from solstone.think.activities import ( estimate_duration_minutes, get_activity_by_id, @@ -188,7 +193,7 @@ def activities_day_activities(day: str) -> Any: Returns enriched activity records: timing comes from ``start``/``end`` for anticipated records and from segment keys for realized records. - Returns JSON array of activity objects. + Returns JSON collection envelope of activity objects. """ if not DATE_RE.fullmatch(day): return error_response(INVALID_DAY, detail="Invalid day format") @@ -210,7 +215,7 @@ def activities_day_activities(day: str) -> Any: # Sort by start time (activities without times go last) enriched_records.sort(key=lambda a: a.get("startTime", "z")) - return jsonify(enriched_records) + return respond_collection(enriched_records) @activities_bp.route("/api/activity_output/") diff --git a/solstone/apps/activities/tests/conftest.py b/solstone/apps/activities/tests/conftest.py index d7d4ada6d..c81ea9c2d 100644 --- a/solstone/apps/activities/tests/conftest.py +++ b/solstone/apps/activities/tests/conftest.py @@ -23,6 +23,19 @@ def activities_env(tmp_path, monkeypatch): activities_dir = facet_dir / "activities" activities_dir.mkdir(parents=True, exist_ok=True) + config_dir = tmp_path / "config" + config_dir.mkdir(parents=True, exist_ok=True) + (config_dir / "journal.json").write_text( + json.dumps( + { + "convey": {"trust_localhost": True}, + "setup": {"completed_at": 1700000000000}, + } + ) + + "\n", + encoding="utf-8", + ) + (facet_dir / "facet.json").write_text( json.dumps({"title": f"Test {facet}", "description": "Test facet"}) + "\n", encoding="utf-8", diff --git a/solstone/apps/activities/tests/test_routes.py b/solstone/apps/activities/tests/test_routes.py new file mode 100644 index 000000000..c3dc8b5b6 --- /dev/null +++ b/solstone/apps/activities/tests/test_routes.py @@ -0,0 +1,39 @@ +# SPDX-License-Identifier: AGPL-3.0-only +# Copyright (c) 2026 sol pbc + +from __future__ import annotations + +from solstone.convey import create_app + + +def test_day_activities_returns_collection_envelope(activities_env): + journal, _facet, day, _day_path = activities_env( + [ + { + "id": "coding_090000_300", + "activity": "coding", + "title": "Focused coding", + "segments": ["090000_300"], + "created_at": 1, + } + ] + ) + client = create_app(journal=str(journal)).test_client() + + response = client.get(f"/app/activities/api/day/{day}/activities") + + assert response.status_code == 200 + payload = response.get_json() + assert set(payload) == {"items", "total"} + assert len(payload["items"]) == 1 + assert payload["total"] == len(payload["items"]) + + +def test_day_activities_empty_day_returns_empty_envelope(activities_env): + journal, _facet, day, _day_path = activities_env(None) + client = create_app(journal=str(journal)).test_client() + + response = client.get(f"/app/activities/api/day/{day}/activities") + + assert response.status_code == 200 + assert response.get_json() == {"items": [], "total": 0} diff --git a/solstone/apps/settings/routes.py b/solstone/apps/settings/routes.py index 8583727ed..09a402073 100644 --- a/solstone/apps/settings/routes.py +++ b/solstone/apps/settings/routes.py @@ -64,7 +64,7 @@ from solstone.convey.sol_initiated.settings import ( from solstone.convey.sol_initiated.settings import ( save_settings as save_sol_voice_settings, ) -from solstone.convey.utils import error_response +from solstone.convey.utils import error_response, respond_collection from solstone.think.models import LOCAL_MODEL from solstone.think.providers.google import validate_vertex_credentials from solstone.think.retention import ( @@ -629,11 +629,11 @@ def get_sol_voice_throttled() -> Any: log_path = Path(get_journal()) / "push" / "nudge_log.jsonl" if not log_path.exists(): - return jsonify([]) + return respond_collection([]) try: rows = _read_sol_voice_throttled_rows(log_path, limit) - return jsonify(rows) + return respond_collection(rows) except Exception: logger.exception("error loading sol voice throttled log") return error_response(FILE_READ_FAILED, detail="unable to load throttled log") diff --git a/solstone/apps/settings/tests/test_sol_voice_routes.py b/solstone/apps/settings/tests/test_sol_voice_routes.py index 269bb713e..22d83992c 100644 --- a/solstone/apps/settings/tests/test_sol_voice_routes.py +++ b/solstone/apps/settings/tests/test_sol_voice_routes.py @@ -217,7 +217,8 @@ def test_sol_voice_throttled_endpoint_filters_correctly(settings_env): response = client.get("/app/settings/api/sol_voice/throttled?limit=1") assert response.status_code == 200 - assert response.get_json() == [ + payload = response.get_json() + assert payload["items"] == [ { "ts": 4, "category": "commitment", @@ -225,6 +226,7 @@ def test_sol_voice_throttled_endpoint_filters_correctly(settings_env): "outcome": "rate_floor", } ] + assert payload["total"] == len(payload["items"]) def test_sol_voice_throttled_endpoint_handles_missing_file(settings_env): @@ -234,7 +236,7 @@ def test_sol_voice_throttled_endpoint_handles_missing_file(settings_env): response = client.get("/app/settings/api/sol_voice/throttled") assert response.status_code == 200 - assert response.get_json() == [] + assert response.get_json() == {"items": [], "total": 0} def test_sol_voice_settings_persist_through_load_settings(settings_env): diff --git a/solstone/apps/settings/workspace.html b/solstone/apps/settings/workspace.html index 0c8950dfe..9135685a7 100644 --- a/solstone/apps/settings/workspace.html +++ b/solstone/apps/settings/workspace.html @@ -4255,8 +4255,8 @@ async function loadSolVoiceThrottledLog() { const log = document.getElementById('sol-voice-throttled-log'); if (!log || log.hidden) return; try { - const rows = await window.apiJson('api/sol_voice/throttled?limit=50'); - renderSolVoiceThrottledLog(Array.isArray(rows) ? rows : []); + const data = await window.apiJson('api/sol_voice/throttled?limit=50'); + renderSolVoiceThrottledLog(Array.isArray(data?.items) ? data.items : []); } catch (err) { log.textContent = solVoiceCopy.throttledError; window.logError(err, { context: 'settings: loadSolVoiceThrottledLog failed' }); diff --git a/solstone/apps/tokens/routes.py b/solstone/apps/tokens/routes.py index a6e5c69dd..cb2687a67 100644 --- a/solstone/apps/tokens/routes.py +++ b/solstone/apps/tokens/routes.py @@ -10,12 +10,12 @@ from datetime import date, timedelta from pathlib import Path from typing import Any, Dict -from flask import Blueprint, abort, jsonify, render_template, request +from flask import Blueprint, jsonify, render_template, request from solstone.apps.tokens import copy as tokens_copy from solstone.convey import state -from solstone.convey.reasons import INVALID_DAY, INVALID_MONTH -from solstone.convey.utils import DATE_RE, error_response +from solstone.convey.reasons import INVALID_DAY, INVALID_MONTH, INVALID_REQUEST_VALUE +from solstone.convey.utils import DATE_RE, error_response, respond_collection from solstone.think.models import calc_token_cost, get_model_provider, iter_token_log tokens_bp = Blueprint( @@ -355,10 +355,13 @@ def api_daily(): try: days = int(raw_days) except (TypeError, ValueError): - abort(400) + return error_response(INVALID_REQUEST_VALUE, detail="days must be a number") if days < 1 or days > 90: - abort(400) + return error_response( + INVALID_REQUEST_VALUE, + detail="days must be between 1 and 90", + ) today = date.today() rows = [] @@ -381,7 +384,7 @@ def api_daily(): rows.append({"day": day, "cost": round(cost, 4), "tokens": tokens}) - return jsonify(rows) + return respond_collection(rows) @tokens_bp.route("/api/stats/") diff --git a/solstone/apps/tokens/tests/test_routes.py b/solstone/apps/tokens/tests/test_routes.py index 9d4c52341..6d527e24c 100644 --- a/solstone/apps/tokens/tests/test_routes.py +++ b/solstone/apps/tokens/tests/test_routes.py @@ -59,7 +59,9 @@ def test_api_daily_happy_path(tokens_env, monkeypatch): response = env.client.get("/app/tokens/api/daily?days=14") assert response.status_code == 200 - rows = response.get_json() + payload = response.get_json() + rows = payload["items"] + assert payload["total"] == len(rows) assert len(rows) == 14 assert all(set(row) == {"day", "cost", "tokens"} for row in rows) assert sorted(rows, key=lambda row: row["day"]) == rows @@ -88,7 +90,7 @@ def test_api_daily_zero_fills_missing_days(tokens_env, monkeypatch): response = env.client.get("/app/tokens/api/daily?days=7") assert response.status_code == 200 - rows = response.get_json() + rows = response.get_json()["items"] assert [row["day"] for row in rows] == [_day(offset) for offset in range(6, -1, -1)] by_day = {row["day"]: row for row in rows} assert by_day[_day(5)]["tokens"] == 1000 @@ -100,9 +102,18 @@ def test_api_daily_zero_fills_missing_days(tokens_env, monkeypatch): def test_api_daily_rejects_invalid_days(tokens_env): env = tokens_env({}) - for days in ["0", "-1", "91", "abc"]: + cases = { + "abc": "days must be a number", + "0": "days must be between 1 and 90", + "-1": "days must be between 1 and 90", + "91": "days must be between 1 and 90", + } + for days, expected_detail in cases.items(): response = env.client.get(f"/app/tokens/api/daily?days={days}") assert response.status_code == 400 + payload = response.get_json() + assert payload["reason_code"] == "invalid_request_value" + assert payload["detail"] == expected_detail def test_api_daily_cross_month_boundary(tokens_env, monkeypatch): @@ -124,7 +135,7 @@ def test_api_daily_cross_month_boundary(tokens_env, monkeypatch): response = env.client.get("/app/tokens/api/daily?days=7") assert response.status_code == 200 - rows = response.get_json() + rows = response.get_json()["items"] assert [row["day"] for row in rows] == [ "20260226", "20260227", diff --git a/solstone/apps/tokens/workspace.html b/solstone/apps/tokens/workspace.html index 9a42f9d7d..5201c7bee 100644 --- a/solstone/apps/tokens/workspace.html +++ b/solstone/apps/tokens/workspace.html @@ -835,7 +835,7 @@ function getModelDisplayName(model) { async function loadDailySeries(days) { try { - return await window.apiJson(`/app/tokens/api/daily?days=${days}`); + return (await window.apiJson(`/app/tokens/api/daily?days=${days}`))?.items ?? null; } catch (err) { console.error('Failed to load daily token series:', err); return null; diff --git a/tests/baselines/api/activities/day-activities.json b/tests/baselines/api/activities/day-activities.json index fe51488c7..d4e41a8fc 100644 --- a/tests/baselines/api/activities/day-activities.json +++ b/tests/baselines/api/activities/day-activities.json @@ -1 +1,4 @@ -[] +{ + "items": [], + "total": 0 +} diff --git a/tests/baselines/api/tokens/daily.json b/tests/baselines/api/tokens/daily.json index 363d1e1f7..96de455f3 100644 --- a/tests/baselines/api/tokens/daily.json +++ b/tests/baselines/api/tokens/daily.json @@ -1,72 +1,75 @@ -[ - { - "cost": 0.0, - "day": "20260401", - "tokens": 0 - }, - { - "cost": 0.0, - "day": "20260402", - "tokens": 0 - }, - { - "cost": 0.0, - "day": "20260403", - "tokens": 0 - }, - { - "cost": 0.0, - "day": "20260404", - "tokens": 0 - }, - { - "cost": 0.0, - "day": "20260405", - "tokens": 0 - }, - { - "cost": 0.0, - "day": "20260406", - "tokens": 0 - }, - { - "cost": 0.0, - "day": "20260407", - "tokens": 0 - }, - { - "cost": 0.0, - "day": "20260408", - "tokens": 0 - }, - { - "cost": 0.0, - "day": "20260409", - "tokens": 0 - }, - { - "cost": 0.0, - "day": "20260410", - "tokens": 0 - }, - { - "cost": 0.0, - "day": "20260411", - "tokens": 0 - }, - { - "cost": 0.0, - "day": "20260412", - "tokens": 0 - }, - { - "cost": 0.0, - "day": "20260413", - "tokens": 0 - }, - { - "cost": 0.0, - "day": "20260414", - "tokens": 0 - } -] +{ + "items": [ + { + "cost": 0.0, + "day": "20260401", + "tokens": 0 + }, + { + "cost": 0.0, + "day": "20260402", + "tokens": 0 + }, + { + "cost": 0.0, + "day": "20260403", + "tokens": 0 + }, + { + "cost": 0.0, + "day": "20260404", + "tokens": 0 + }, + { + "cost": 0.0, + "day": "20260405", + "tokens": 0 + }, + { + "cost": 0.0, + "day": "20260406", + "tokens": 0 + }, + { + "cost": 0.0, + "day": "20260407", + "tokens": 0 + }, + { + "cost": 0.0, + "day": "20260408", + "tokens": 0 + }, + { + "cost": 0.0, + "day": "20260409", + "tokens": 0 + }, + { + "cost": 0.0, + "day": "20260410", + "tokens": 0 + }, + { + "cost": 0.0, + "day": "20260411", + "tokens": 0 + }, + { + "cost": 0.0, + "day": "20260412", + "tokens": 0 + }, + { + "cost": 0.0, + "day": "20260413", + "tokens": 0 + }, + { + "cost": 0.0, + "day": "20260414", + "tokens": 0 + } + ], + "total": 14 +} diff --git a/tests/test_app_activities.py b/tests/test_app_activities.py index 7ad6a3dff..7179e98cc 100644 --- a/tests/test_app_activities.py +++ b/tests/test_app_activities.py @@ -40,7 +40,9 @@ class TestActivitiesDayRoutes: "/app/activities/api/day/20260214/activities?facet=full-featured" ) assert resp.status_code == 200 - data = resp.get_json() + payload = resp.get_json() + data = payload["items"] + assert payload["total"] == len(data) assert isinstance(data, list) assert len(data) >= 2 @@ -58,7 +60,7 @@ class TestActivitiesDayRoutes: resp = activities_client.get( "/app/activities/api/day/20260214/activities?facet=full-featured" ) - data = resp.get_json() + data = resp.get_json()["items"] coding = next(a for a in data if a["activity"] == "coding") assert coding["name"] != "" assert coding["icon"] != "" @@ -68,7 +70,7 @@ class TestActivitiesDayRoutes: "/app/activities/api/day/20260422/activities?facet=full-featured" ) assert resp.status_code == 200 - data = resp.get_json() + data = resp.get_json()["items"] by_id = {activity["id"]: activity for activity in data} assert set(by_id) == { "anticipated_meeting_090000_0422", @@ -217,7 +219,7 @@ class TestActivitiesDayRoutes: resp = activities_client.get( "/app/activities/api/day/20260214/activities?facet=full-featured" ) - data = resp.get_json() + data = resp.get_json()["items"] coding = next(a for a in data if a["activity"] == "coding") assert len(coding["outputs"]) >= 1 output = coding["outputs"][0] -- 2.51.2