diff --git a/.superpowers/sdd/2026-09-22-control-panel-operational-ux-v1/task-3-report.md b/.superpowers/sdd/2026-09-22-control-panel-operational-ux-v1/task-3-report.md index 80beb11..2d68ef0 100644 --- a/.superpowers/sdd/2026-09-22-control-panel-operational-ux-v1/task-3-report.md +++ b/.superpowers/sdd/2026-09-22-control-panel-operational-ux-v1/task-3-report.md @@ -71,3 +71,61 @@ The warnings are existing third-party test-environment warnings from `pytest_asy - Live PostgreSQL and agent execution were not exercised; verification used the repository's offline SQLite test fixtures. - The blocker remains open until the agent's normal reprocess completion path reports resolution; this prevents marking it resolved before the bounded job succeeds. + +## Review Fixes + +### RED + +Added regression tests before changing production code for the four review findings: + +- Legacy blockers with `installation_id=NULL` must fail closed without selecting an Agent or creating work. +- Cross-tenant installation and Agent pointers must fail before context, job, or audit mutation. +- Oversized values/reasons and non-string fields must be rejected cleanly. +- Successful web resolution must redirect to the blocker detail page. + +Focused command before the fixes: + +```text +python -m pytest central/tests/test_blocker_resolution_panel.py -q +``` + +RED result: + +```text +4 failed, 5 passed +``` + +The failures reproduced the legacy Agent fallback, cross-tenant acceptance, unbounded/malformed input, and `/blockers` redirect behavior. + +### GREEN + +- Resolution now requires a non-null exact installation and verifies both installation and Agent tenant ownership before mutation. +- `value` is required to be a string of at most 512 characters; `reason` is required to be a string of at most 2000 characters. +- Boundary-sized audit data retains all required audit fields without whole-context truncation. +- Web success redirects to `/blockers/{blocker_id}`. + +Focused command: + +```text +python -m pytest central/tests/test_blocker_resolution_panel.py -q +``` + +Result: + +```text +9 passed, 1 warning +``` + +Full central command: + +```text +python -m pytest central/tests -q +``` + +Result: + +```text +265 passed, 1 warning +``` + +The existing `pytest_asyncio` configuration and `openpyxl` print-area warnings remain unchanged. diff --git a/central/app/blocker_resolution.py b/central/app/blocker_resolution.py index 8e34b6c..8403642 100644 --- a/central/app/blocker_resolution.py +++ b/central/app/blocker_resolution.py @@ -13,6 +13,8 @@ from .models import Agent, BlockedProduct, FiscalContextEntry, Installation, Ten WORKFLOW_SCOPES = {"PRODUCT", "GROUP", "COMPANY"} ENTRY_FIELDS = {"scope", "scope_key", "key", "value", "reason"} +MAX_VALUE_LENGTH = 512 +MAX_REASON_LENGTH = 2000 def validate_resolution_input(payload): @@ -22,6 +24,14 @@ def validate_resolution_input(payload): unexpected = set(payload) - ENTRY_FIELDS if unexpected: return None, f"unsupported resolution fields: {sorted(unexpected)}" + if not isinstance(payload.get("value"), str): + return None, "value must be a string" + if not isinstance(payload.get("reason"), str): + return None, "reason must be a string" + if len(payload["value"]) > MAX_VALUE_LENGTH: + return None, f"value must be at most {MAX_VALUE_LENGTH} characters" + if len(payload["reason"]) > MAX_REASON_LENGTH: + return None, f"reason must be at most {MAX_REASON_LENGTH} characters" normalized, error = validate_entry(payload) if error: @@ -69,15 +79,15 @@ def resolve_blocker_with_context(session, blocker_id, admin, payload, request_ip installation = session.get(Installation, blocker.installation_id) if installation is None: raise LookupError("blocker installation not found") + if installation.tenant_id != blocker.tenant_id: + raise LookupError("blocker installation tenant mismatch") agent = session.get(Agent, installation.agent_id) + if agent is None: + raise LookupError("blocker installation agent not found") + if agent.tenant_id != blocker.tenant_id: + raise LookupError("blocker agent tenant mismatch") else: - agent = session.scalar( - select(Agent) - .where(Agent.tenant_id == blocker.tenant_id, Agent.enabled.is_(True)) - .order_by(Agent.id.asc()) - ) - if agent is None: - raise LookupError("blocker agent not found") + raise LookupError("blocker installation is required") tenant = session.get(Tenant, blocker.tenant_id) company_key = _company_key(blocker, installation, tenant) diff --git a/central/app/web.py b/central/app/web.py index 5388f73..16a438e 100644 --- a/central/app/web.py +++ b/central/app/web.py @@ -149,7 +149,7 @@ def router(session_factory): except ValueError as error: detail = blocker_detail(session, blocker_id) return render(request, "blocker_detail.html", {"detail": detail, "auth": auth_data, "context_entries": [], "context_keys": sorted(ALLOWED_KEYS), "value_choices": sorted({value for values in ENUMS.values() for value in values}), "error": str(error)}, 422) - return RedirectResponse("/blockers", status_code=303) + return RedirectResponse(f"/blockers/{blocker_id}", status_code=303) finally: session.close() diff --git a/central/tests/test_blocker_resolution_panel.py b/central/tests/test_blocker_resolution_panel.py index b1585d3..241747e 100644 --- a/central/tests/test_blocker_resolution_panel.py +++ b/central/tests/test_blocker_resolution_panel.py @@ -1,10 +1,11 @@ from fastapi.testclient import TestClient +import pytest from sqlalchemy import select from central.app.blocked_products import upsert_blocker from central.app.blocker_resolution import resolve_blocker_with_context, validate_resolution_input from central.app.main import create_app -from central.app.models import AdminUser, AuditEvent, FiscalContextEntry, Job, Installation +from central.app.models import AdminUser, Agent, AuditEvent, BlockedProduct, FiscalContextEntry, Job, Installation, Tenant from central.tests.test_control_plane import seed, setup_db @@ -106,6 +107,78 @@ def test_resolve_blocker_with_context_persists_one_context_row_job_and_sanitized assert session.query(FiscalContextEntry).count() == 1 +def test_legacy_blocker_without_installation_fails_closed_without_job_or_audit(): + session_factory = setup_db() + blocker_id, admin_id, _ = _seed_blocker(session_factory) + + with session_factory() as session: + blocker = session.get(BlockedProduct, blocker_id) + blocker.installation_id = None + session.commit() + admin = session.get(AdminUser, admin_id) + + with pytest.raises(LookupError, match="installation"): + resolve_blocker_with_context(session, blocker_id, admin, {"scope": "PRODUCT", "scope_key": "P-001", "key": "product_role", "value": "RESALE", "reason": "reviewed"}) + + assert session.query(FiscalContextEntry).count() == 0 + assert session.query(Job).count() == 0 + assert session.query(AuditEvent).count() == 0 + + +def test_cross_tenant_installation_pointer_fails_before_mutation(): + session_factory = setup_db() + blocker_id, admin_id, _ = _seed_blocker(session_factory) + + with session_factory() as session: + blocker = session.get(BlockedProduct, blocker_id) + other_tenant = Tenant(name="Other Client") + session.add(other_tenant) + session.flush() + other_agent = Agent(tenant_id=other_tenant.id, name="other-agent", token_hash="other-agent-token".ljust(64, "x")) + session.add(other_agent) + session.flush() + other_installation = Installation(agent_id=other_agent.id, tenant_id=other_tenant.id, installation_id="22222222-2222-4222-8222-222222222222", company_code="OTHER-COMPANY") + session.add(other_installation) + session.flush() + blocker.installation_id = other_installation.id + session.commit() + admin = session.get(AdminUser, admin_id) + + with pytest.raises(LookupError, match="tenant"): + resolve_blocker_with_context(session, blocker_id, admin, {"scope": "PRODUCT", "scope_key": "P-001", "key": "product_role", "value": "RESALE", "reason": "reviewed"}) + + assert session.query(FiscalContextEntry).count() == 0 + assert session.query(Job).count() == 0 + assert session.query(AuditEvent).count() == 0 + + +def test_resolution_input_rejects_oversized_and_non_string_fields_cleanly(): + base = {"scope": "PRODUCT", "scope_key": "P-001", "key": "service_cfop", "value": "5102", "reason": "reviewed"} + for field, value in (("value", "v" * 513), ("reason", "r" * 2001), ("reason", 123)): + payload = {**base, field: value} + normalized, error = validate_resolution_input(payload) + assert normalized is None + assert error + + +def test_boundary_length_audit_keeps_required_fields_instead_of_truncating_context(): + session_factory = setup_db() + blocker_id, admin_id, _ = _seed_blocker(session_factory) + + with session_factory() as session: + admin = session.get(AdminUser, admin_id) + result = resolve_blocker_with_context( + session, + blocker_id, + admin, + {"scope": "PRODUCT", "scope_key": "P-001", "key": "service_cfop", "value": "v" * 512, "reason": "r" * 2000}, + ) + audit = session.get(AuditEvent, result["audit_id"]) + required = {"admin_user_id", "tenant_id", "client", "company_key", "installation_id", "product_code", "blocker_id", "key", "scope", "scope_key", "value", "reason", "before", "after", "reprocess_job_id", "request_ip"} + assert required <= audit.context.keys() + assert "truncated" not in audit.context + + def test_human_resolution_detail_requires_auth_csrf_and_renders_safe_form(): session_factory = setup_db() blocker_id, _, _ = _seed_blocker(session_factory) @@ -127,6 +200,14 @@ def test_human_resolution_detail_requires_auth_csrf_and_renders_safe_form(): ) assert missing_csrf.status_code == 403 + success = client.post( + f"/blockers/{blocker_id}/resolve-context", + data={"csrf_token": client.cookies.get("gti_csrf"), "scope": "PRODUCT", "scope_key": "P-001", "key": "product_role", "value": "RESALE", "reason": "reviewed"}, + follow_redirects=False, + ) + assert success.status_code == 303 + assert success.headers["location"] == f"/blockers/{blocker_id}" + def test_human_and_json_resolution_routes_use_the_same_validated_workflow(): session_factory = setup_db()