From 0c2dc8f88936bb1a1fe0e50d304117d5008b2819 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 12 Oct 2026 00:14:02 +0000 Subject: [PATCH 1/5] Add ndi.cloud.api.feedback (submitFeedback, deleteFeedback) Port of NDI-matlab#1023 (75f937a76): wrappers for the in-app problem report endpoints from ndi-cloud-node#158. - submitFeedback(app, category, description, *, appVersion, datasetId, documentId, environment, state, log, traceback) POSTs /feedback with schemaVersion 1. Optional fields keep MATLAB's camelCase names because they are the shared payload keys; None omits a field, as MATLAB's missing / struct.empty do. Returns an APIResponse whose data is the stored report id. - deleteFeedback(feedbackID) DELETEs /feedback/{feedbackId} (204). - FeedbackCategory validator in _validators.py (bug/performance/other). - Bridge entries for both; url bumped to 75f937a76 for the two new routes. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01B22A1gLVKa1E3uQm3tv4t8 --- src/ndi/cloud/api/__init__.py | 5 +- src/ndi/cloud/api/_validators.py | 9 +- src/ndi/cloud/api/feedback.py | 154 ++++++++++++++++++ .../cloud/api/ndi_matlab_python_bridge.yaml | 102 +++++++++++- 4 files changed, 265 insertions(+), 5 deletions(-) create mode 100644 src/ndi/cloud/api/feedback.py diff --git a/src/ndi/cloud/api/__init__.py b/src/ndi/cloud/api/__init__.py index 7ec3d946..ddd929ab 100644 --- a/src/ndi/cloud/api/__init__.py +++ b/src/ndi/cloud/api/__init__.py @@ -7,8 +7,9 @@ files — Presigned URL retrieval, file upload users — User creation, profile compute — Compute session management + feedback — In-app problem reports """ -from . import compute, datasets, documents, files, users +from . import compute, datasets, documents, feedback, files, users -__all__ = ["datasets", "documents", "files", "users", "compute"] +__all__ = ["datasets", "documents", "files", "users", "compute", "feedback"] diff --git a/src/ndi/cloud/api/_validators.py b/src/ndi/cloud/api/_validators.py index 01cdaf18..8a8d4ba3 100644 --- a/src/ndi/cloud/api/_validators.py +++ b/src/ndi/cloud/api/_validators.py @@ -9,6 +9,7 @@ PageSize -> (1,1) double (integer >= 1) Scope -> {iMustBeValidScope} (keyword, or dataset-id list) FilePath -> {mustBeFile} (file must exist on disk) + FeedbackCategory -> {mustBeMember(category, ["bug", "performance", "other"])} Usage:: @@ -25,7 +26,7 @@ def get_dataset(dataset_id: CloudId, *, client: CloudClient | None = None): import re from pathlib import Path -from typing import Annotated +from typing import Annotated, Literal from urllib.parse import urlparse from pydantic import AfterValidator, ConfigDict, Field @@ -82,6 +83,12 @@ def _check_file_exists(v: str) -> str: FilePath = Annotated[str, AfterValidator(_check_file_exists)] +# -- {mustBeMember}: feedback report category -------------------------------- +#: The categories POST /feedback accepts (ndi-cloud-node FEEDBACK_CATEGORIES). +FEEDBACK_CATEGORIES = ("bug", "performance", "other") + +FeedbackCategory = Literal["bug", "performance", "other"] + # -- Shared validate_call config --------------------------------------------- VALIDATE_CONFIG = ConfigDict(arbitrary_types_allowed=True) diff --git a/src/ndi/cloud/api/feedback.py b/src/ndi/cloud/api/feedback.py new file mode 100644 index 00000000..28f4d425 --- /dev/null +++ b/src/ndi/cloud/api/feedback.py @@ -0,0 +1,154 @@ +""" +ndi.cloud.api.feedback - In-app problem reports. + +All functions accept an optional ``client`` keyword argument. When omitted, +a client is created automatically from environment variables. + +The request body is a contract shared with NDI-matlab and the server +(ndi-cloud-node#158): field names, and which fields are left out when not +supplied, follow ``ndi.cloud.api.feedback.submitFeedback`` exactly. That is +why the optional arguments keep their camelCase payload names here rather +than the snake_case the sibling modules use for arguments. + +MATLAB equivalents: +ndi/+cloud/+api/+feedback/*.m, + +implementation/+feedback/*.m +""" + +from __future__ import annotations + +from typing import Annotated, Any + +from pydantic import SkipValidation, validate_call + +from ..client import APIResponse, CloudClient, _auto_client +from ._validators import VALIDATE_CONFIG, CloudId, FeedbackCategory, NonEmptyStr + +_Client = Annotated[CloudClient | None, SkipValidation()] + +#: The report schema this client writes. The server refuses any other. +FEEDBACK_SCHEMA_VERSION = 1 + + +@_auto_client +@validate_call(config=VALIDATE_CONFIG) +def submitFeedback( + app: NonEmptyStr, + category: FeedbackCategory, + description: NonEmptyStr, + *, + appVersion: str | None = None, + datasetId: str | None = None, + documentId: str | None = None, + environment: dict[str, Any] | None = None, + state: dict[str, Any] | None = None, + log: str | None = None, + traceback: str | None = None, + client: _Client = None, +) -> APIResponse: + """POST /feedback -- Submit an in-app problem report to NDI Cloud. + + The reporter is the logged-in user. The server redacts anything that + looks like a bearer token or a signed S3 URL credential before storing + the report, but callers should still scrub logs before sending. + + Args: + app: Name of the client sending the report + (e.g. ``"lightsheetZarr-viewer"``). + category: One of ``"bug"``, ``"performance"`` or ``"other"``. + description: The user's free-text description of the problem. + appVersion: Version of the client (e.g. ``"NDI-python 2.0"``). + datasetId: Cloud dataset id the report concerns. + documentId: Document id the report concerns. + environment: Dict describing the client environment (os, versions, ...). + state: Dict with client state useful for diagnosis. + log: Recent log lines. + traceback: Error stack, if any. + client: Authenticated cloud client. + + An optional field left at ``None`` is omitted from the request, as + MATLAB omits a ``missing`` string or an empty struct. An empty string + or an empty dict is sent, as MATLAB sends ``""`` and ``struct()``. + + Server limits: 1 MB per request; description 20,000 characters, log + 500,000, traceback 50,000; environment 20 kB and state 200 kB as JSON; + 10 reports per user per hour. Exceeding a size cap raises + :class:`~ndi.cloud.exceptions.CloudAPIError` with ``status_code`` 413; + the hourly limit, 429. + + Returns: + :class:`~ndi.cloud.client.APIResponse` whose ``data`` is the id of + the stored report (use it with :func:`deleteFeedback`), so that + ``b, feedbackID, status, url = submitFeedback(...)`` mirrors + MATLAB's ``[b, feedbackID, apiResponse, apiURL]``. + """ + body: dict[str, Any] = { + "schemaVersion": FEEDBACK_SCHEMA_VERSION, + "app": app, + "category": category, + "description": description, + } + optional = { + "appVersion": appVersion, + "datasetId": datasetId, + "documentId": documentId, + "log": log, + "traceback": traceback, + "environment": environment, + "state": state, + } + body.update({name: value for name, value in optional.items() if value is not None}) + return _submit(body, client=client) + + +def _submit(body: dict[str, Any], *, client: CloudClient) -> APIResponse: + """POST BODY to /feedback as-is and return the stored report's id. + + MATLAB equivalent: ``ndi.cloud.api.implementation.feedback.SubmitFeedback``, + which likewise sends whatever struct it is given, so tests can reach + the server's own validation past the wrapper's. + """ + result = client.post("/feedback", json=body) + data = result.data if hasattr(result, "data") else result + feedback_id = data.get("id", "") if isinstance(data, dict) else "" + return APIResponse( + feedback_id, + success=True, + status_code=getattr(result, "status_code", 201), + url=getattr(result, "url", ""), + headers=getattr(result, "headers", None), + reason=getattr(result, "reason", ""), + elapsed=getattr(result, "elapsed", None), + http_response=getattr(result, "http_response", None), + ) + + +@_auto_client +@validate_call(config=VALIDATE_CONFIG) +def deleteFeedback(feedbackID: CloudId, *, client: _Client = None) -> APIResponse: + """DELETE /feedback/{feedbackId} -- Withdraw one of your own reports. + + Only the user who submitted a report can delete it. A report that does + not exist, or that belongs to another user, raises + :class:`~ndi.cloud.exceptions.CloudNotFoundError` (HTTP 404); an id + that is not a well-formed ObjectId raises + :class:`~ndi.cloud.exceptions.CloudAPIError` with ``status_code`` 400. + + Args: + feedbackID: The id returned by :func:`submitFeedback`. + client: Authenticated cloud client. + + Returns: + :class:`~ndi.cloud.client.APIResponse` (HTTP 204) whose ``data`` is + MATLAB's confirmation message, ``"Feedback deleted successfully."``. + """ + result = client.delete("/feedback/{feedbackId}", feedbackId=feedbackID) + return APIResponse( + "Feedback deleted successfully.", + success=True, + status_code=getattr(result, "status_code", 204), + url=getattr(result, "url", ""), + headers=getattr(result, "headers", None), + reason=getattr(result, "reason", ""), + elapsed=getattr(result, "elapsed", None), + http_response=getattr(result, "http_response", None), + ) diff --git a/src/ndi/cloud/api/ndi_matlab_python_bridge.yaml b/src/ndi/cloud/api/ndi_matlab_python_bridge.yaml index 0f991cea..367b1ea0 100644 --- a/src/ndi/cloud/api/ndi_matlab_python_bridge.yaml +++ b/src/ndi/cloud/api/ndi_matlab_python_bridge.yaml @@ -1,7 +1,8 @@ # ndi_matlab_python_bridge.yaml — src/ndi/cloud/api/ # The Primary Contract for the ndi.cloud.api namespace. # -# Covers: datasets.py, documents.py, files.py, users.py, compute.py, _validators.py +# Covers: datasets.py, documents.py, files.py, users.py, compute.py, feedback.py, +# _validators.py project_metadata: bridge_version: "1.1" @@ -1426,6 +1427,99 @@ functions: misconfigured CI from silently deleting a real registered license). + # --- feedback.py --- + - name: submitFeedback + matlab_path: "+ndi/+cloud/+api/+feedback/submitFeedback.m" + matlab_last_sync_hash: "75f937a76" + python_path: "ndi/cloud/api/feedback.py" + input_arguments: + - name: app + type_matlab: "string {mustBeNonzeroLengthText}" + type_python: "NonEmptyStr" + - name: category + type_matlab: "string {mustBeMember(category, [\"bug\", \"performance\", \"other\"])}" + type_python: "FeedbackCategory" + - name: description + type_matlab: "string {mustBeNonzeroLengthText}" + type_python: "NonEmptyStr" + - name: appVersion + type_matlab: "string = missing" + type_python: "str | None" + default: "None" + - name: datasetId + type_matlab: "string = missing" + type_python: "str | None" + default: "None" + - name: documentId + type_matlab: "string = missing" + type_python: "str | None" + default: "None" + - name: environment + type_matlab: "struct {mustBeScalarOrEmpty} = struct.empty" + type_python: "dict[str, Any] | None" + default: "None" + - name: state + type_matlab: "struct {mustBeScalarOrEmpty} = struct.empty" + type_python: "dict[str, Any] | None" + default: "None" + - name: log + type_matlab: "string = missing" + type_python: "str | None" + default: "None" + - name: traceback + type_matlab: "string = missing" + type_python: "str | None" + default: "None" + - name: client + type_python: "_Client" + default: "None" + output_arguments: + - name: result + type_python: "APIResponse" + decision_log: > + Ported from NDI-matlab#1023 (75f937a76). POST /feedback with body + {schemaVersion: 1, app, category, description} plus each optional + field the caller supplied; the contract is ndi-cloud-node#158. + The optional arguments keep MATLAB's camelCase names (rather than the + snake_case the sibling modules use for arguments) because each one is + also the payload key, and the payload is shared with MATLAB and the + server. They are keyword-only, as MATLAB's are name-value options. + None plays the role of MATLAB's missing / struct.empty: the field is + omitted. "" and {} are sent, as MATLAB sends "" and struct(). + Returns an APIResponse whose data is the stored report's id, so + `b, feedbackID, status, url = submitFeedback(...)` unpacks like + MATLAB's [b, feedbackID, apiResponse, apiURL]. Where MATLAB returns + b = false on a non-201, Python raises (CloudAPIError with + status_code 400 / 413 / 429, CloudAuthError on 401 / 403), as every + other ndi.cloud.api function does. The implementation class + SubmitFeedback (sends any struct as-is) is the private helper + feedback._submit, used by the tests to reach server-side validation. + Tests: tests/test_cloud_api_feedback.py (offline) and + tests/test_cloud_feedback_live.py (port of feedbackTest.m). + + - name: deleteFeedback + matlab_path: "+ndi/+cloud/+api/+feedback/deleteFeedback.m" + matlab_last_sync_hash: "75f937a76" + python_path: "ndi/cloud/api/feedback.py" + input_arguments: + - name: feedbackID + type_matlab: "string" + type_python: "CloudId" + - name: client + type_python: "_Client" + default: "None" + output_arguments: + - name: result + type_python: "APIResponse" + decision_log: > + Ported from NDI-matlab#1023 (75f937a76). DELETE /feedback/{feedbackId}; + the server answers 204. data is MATLAB's confirmation message + "Feedback deleted successfully.". A report that does not exist or + belongs to another user raises CloudNotFoundError (MATLAB: b = false, + 404); a malformed id raises CloudAPIError with status_code 400. + The id is not checked for ObjectId shape client-side, matching MATLAB, + so the server's 400 stays reachable. + # --- compute.py --- - name: startSession matlab_path: "+ndi/+cloud/+api/+compute/startSession.m" @@ -1545,7 +1639,7 @@ not_yet_ported: - name: url matlab_path: "+ndi/+cloud/+api/url.m" - matlab_last_sync_hash: "3fce6cb" + matlab_last_sync_hash: "75f937a76" status: ported_differently decision_log: > MATLAB URL builder. Python uses CloudClient._build_url() @@ -1563,6 +1657,10 @@ not_yet_ported: bulk-upload status in api/files.py, /advance is what compute.advanceSession already posts to, and the signed-URL-set gap is recorded on batchSignedUrlLookup and downloadGenericFiles. + 2026-10-11: examined against 75f937a76 (was 3fce6cb). NDI-matlab#1023 + added submit_feedback (POST /feedback) and delete_feedback + (DELETE /feedback/{feedbackId}); both are interpolated at their call + sites in api/feedback.py, so there is no route table to update here. 2026-09-22: examined against 3fce6cb (was 6fdec7e7). NDI-matlab#1004 (6bf12fc0) added the list_dataset_files route (/datasets/{datasetId}/files) and made it keyset-paginated. As above, From ad7bd64084f9ae3854095b7e6db5a9b1011afcde Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 12 Oct 2026 00:14:03 +0000 Subject: [PATCH 2/5] Test ndi.cloud.api.feedback offline and live Port of NDI-matlab feedbackTest.m (75f937a76). - tests/test_cloud_api_feedback.py: real CloudClient over a stub transport; pins the exact payload, optional-field omission, URL and method, MATLAB-style unpacking, client-side validation, and the 400/401/403/404/413/429 error mapping. POST is not retried. - tests/test_cloud_feedback_live.py: the live cases, skipped without credentials or where the endpoint is not deployed (404 on a malformed id); every submitted report is deleted, with a teardown backstop. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01B22A1gLVKa1E3uQm3tv4t8 --- tests/test_cloud_api_feedback.py | 312 ++++++++++++++++++++++++++++++ tests/test_cloud_feedback_live.py | 266 +++++++++++++++++++++++++ 2 files changed, 578 insertions(+) create mode 100644 tests/test_cloud_api_feedback.py create mode 100644 tests/test_cloud_feedback_live.py diff --git a/tests/test_cloud_api_feedback.py b/tests/test_cloud_api_feedback.py new file mode 100644 index 00000000..f80730a0 --- /dev/null +++ b/tests/test_cloud_api_feedback.py @@ -0,0 +1,312 @@ +"""Unit tests for ndi.cloud.api.feedback -- no network required. + +Offline half of the port of NDI-matlab +``tests/+ndi/+unittest/+cloud/feedbackTest.m`` (75f937a76); the live half +is ``tests/test_cloud_feedback_live.py``. + +The request body is a contract shared with NDI-matlab and the server +(ndi-cloud-node#158), so these tests pin the exact payload: which keys are +always sent, which are left out when not supplied, and that none is +renamed. The client is a real :class:`CloudClient` over a stub transport, +so the URL, method and error mapping under test are the ones a caller +actually gets. +""" + +from __future__ import annotations + +import json +import types + +import pydantic +import pytest + +from ndi.cloud.api import feedback +from ndi.cloud.api.feedback import deleteFeedback, submitFeedback +from ndi.cloud.client import CloudClient +from ndi.cloud.config import CloudConfig +from ndi.cloud.exceptions import CloudAPIError, CloudAuthError, CloudNotFoundError + +API_URL = "https://api.example.test/v1" +APP = "NDI-python-unittest" +FEEDBACK_ID = "65f0c0ffee0000000000abcd" + + +def response(status=200, body=None, reason=""): + """Something shaped like a ``requests.Response``.""" + text = json.dumps(body) if body is not None else "" + + def _json(): + if body is None: + raise ValueError("no json") + return body + + return types.SimpleNamespace( + status_code=status, + reason=reason, + json=_json, + text=text, + content=text.encode(), + headers={}, + elapsed=None, + ) + + +class Transport: + """A stub ``requests.Session`` that records requests and replies RESP.""" + + def __init__(self, resp): + self.resp = resp + self.calls: list[dict] = [] + + def request(self, method, url, **kwargs): + self.calls.append({"method": method, "url": url, **kwargs}) + return self.resp + + +def make_client(resp, token="test-token"): + client = CloudClient.__new__(CloudClient) + client.config = CloudConfig(api_url=API_URL, token=token) + client._session = Transport(resp) + return client + + +def created(feedback_id=FEEDBACK_ID): + return make_client(response(201, {"id": feedback_id}, "Created")) + + +# --------------------------------------------------------------------------- +# submitFeedback +# --------------------------------------------------------------------------- + + +class TestSubmitFeedbackPayload: + def test_minimal_report_sends_only_the_required_fields(self): + client = created() + submitFeedback(APP, "other", "minimal report", client=client) + + (call,) = client._session.calls + assert call["json"] == { + "schemaVersion": 1, + "app": APP, + "category": "other", + "description": "minimal report", + } + + def test_post_to_feedback_with_bearer_token(self): + client = created() + submitFeedback(APP, "bug", "x", client=client) + + (call,) = client._session.calls + assert call["method"] == "POST" + assert call["url"] == f"{API_URL}/feedback" + assert call["headers"]["Authorization"] == "Bearer test-token" + + def test_full_report_sends_every_optional_field_under_its_matlab_name(self): + environment = {"os": "linux", "python": "3.12", "launchedFrom": "pytest"} + state = {"level": 3, "fetchStats": {"ok": 10, "failed": 2}} + client = created() + submitFeedback( + APP, + "bug", + "full report", + appVersion="NDI-python unittest", + datasetId="000000000000000000000000", + documentId="unittest-document", + environment=environment, + state=state, + log="line 1\nline 2", + traceback="no real traceback", + client=client, + ) + + (call,) = client._session.calls + assert call["json"] == { + "schemaVersion": 1, + "app": APP, + "category": "bug", + "description": "full report", + "appVersion": "NDI-python unittest", + "datasetId": "000000000000000000000000", + "documentId": "unittest-document", + "environment": environment, + "state": state, + "log": "line 1\nline 2", + "traceback": "no real traceback", + } + + @pytest.mark.parametrize( + "field, value", + [ + ("appVersion", "1.0"), + ("datasetId", "abc"), + ("documentId", "def"), + ("log", "a log"), + ("traceback", "a traceback"), + ("environment", {"os": "linux"}), + ("state", {"k": 1}), + ], + ) + def test_each_optional_field_is_sent_alone(self, field, value): + client = created() + submitFeedback(APP, "performance", "d", client=client, **{field: value}) + + body = client._session.calls[0]["json"] + assert body[field] == value + assert set(body) == {"schemaVersion", "app", "category", "description", field} + + def test_empty_values_are_sent_not_dropped(self): + # MATLAB omits only a missing string or struct.empty; "" and + # struct() go out as they are. None is Python's "missing". + client = created() + submitFeedback(APP, "bug", "d", log="", environment={}, client=client) + + body = client._session.calls[0]["json"] + assert body["log"] == "" + assert body["environment"] == {} + + @pytest.mark.parametrize("category", ["bug", "performance", "other"]) + def test_every_category_is_accepted(self, category): + client = created() + submitFeedback(APP, category, "d", client=client) + assert client._session.calls[0]["json"]["category"] == category + + +class TestSubmitFeedbackResult: + def test_returns_the_stored_report_id(self): + result = submitFeedback(APP, "bug", "d", client=created("abc123")) + assert result.data == "abc123" + assert result.success is True + assert result.status_code == 201 + assert result.url == f"{API_URL}/feedback" + + def test_unpacks_like_matlab(self): + b, feedbackID, status, url = submitFeedback(APP, "bug", "d", client=created("abc123")) + assert (b, feedbackID, status, url) == (True, "abc123", 201, f"{API_URL}/feedback") + + +class TestSubmitFeedbackValidation: + """Client-side argument validation: nothing reaches the network.""" + + def test_unknown_category_is_rejected(self): + client = created() + with pytest.raises(pydantic.ValidationError): + submitFeedback(APP, "feature", "never sent", client=client) + assert client._session.calls == [] + + def test_empty_description_is_rejected(self): + client = created() + with pytest.raises(pydantic.ValidationError): + submitFeedback(APP, "bug", "", client=client) + assert client._session.calls == [] + + def test_empty_app_is_rejected(self): + client = created() + with pytest.raises(pydantic.ValidationError): + submitFeedback("", "bug", "d", client=client) + assert client._session.calls == [] + + def test_environment_must_be_a_mapping(self): + client = created() + with pytest.raises(pydantic.ValidationError): + submitFeedback(APP, "bug", "d", environment=["not", "a", "dict"], client=client) + assert client._session.calls == [] + + def test_optional_fields_are_keyword_only(self): + client = created() + with pytest.raises(pydantic.ValidationError): + submitFeedback(APP, "bug", "d", "1.0", client=client) + assert client._session.calls == [] + + +class TestSubmitFeedbackErrors: + @pytest.mark.parametrize( + "status, error", + [ + (400, "category must be one of bug, performance, other"), + (413, "log exceeds 500000 characters"), + (429, "Too many feedback reports; the limit is 10 per hour"), + (500, "Failed to store feedback"), + ], + ) + def test_server_rejection_raises_with_status(self, status, error): + client = make_client(response(status, {"error": error})) + with pytest.raises(CloudAPIError) as excinfo: + submitFeedback(APP, "bug", "d", client=client) + assert excinfo.value.status_code == status + assert excinfo.value.response_body == {"error": error} + + @pytest.mark.parametrize("status", [401, 403]) + def test_unauthenticated_raises_auth_error(self, status): + client = make_client(response(status, {"error": "Unauthorized"})) + with pytest.raises(CloudAuthError): + submitFeedback(APP, "bug", "d", client=client) + + def test_post_is_not_retried(self): + # A repeated POST could store the report twice. + client = make_client(response(503, {"error": "unavailable"})) + with pytest.raises(CloudAPIError): + submitFeedback(APP, "bug", "d", client=client) + assert len(client._session.calls) == 1 + + +class TestSubmitRaw: + """feedback._submit mirrors the SubmitFeedback implementation class.""" + + def test_sends_the_body_as_is(self): + body = {"schemaVersion": 99, "app": APP, "category": "feature"} + client = created() + feedback._submit(body, client=client) + assert client._session.calls[0]["json"] == body + + +# --------------------------------------------------------------------------- +# deleteFeedback +# --------------------------------------------------------------------------- + + +class TestDeleteFeedback: + def test_delete_url_and_method(self): + client = make_client(response(204, None, "No Content")) + deleteFeedback(FEEDBACK_ID, client=client) + + (call,) = client._session.calls + assert call["method"] == "DELETE" + assert call["url"] == f"{API_URL}/feedback/{FEEDBACK_ID}" + assert call["headers"]["Authorization"] == "Bearer test-token" + + def test_success_unpacks_like_matlab(self): + client = make_client(response(204, None, "No Content")) + b, answer, status, url = deleteFeedback(FEEDBACK_ID, client=client) + assert b is True + assert answer == "Feedback deleted successfully." + assert status == 204 + assert url == f"{API_URL}/feedback/{FEEDBACK_ID}" + + def test_id_is_url_encoded(self): + client = make_client(response(204, None)) + deleteFeedback("a/b", client=client) + assert client._session.calls[0]["url"] == f"{API_URL}/feedback/a%2Fb" + + def test_missing_report_raises_not_found(self): + client = make_client(response(404, {"error": "Feedback not found"})) + with pytest.raises(CloudNotFoundError): + deleteFeedback("000000000000000000000000", client=client) + + def test_malformed_id_raises_400(self): + client = make_client(response(400, {"error": "Invalid feedback id"})) + with pytest.raises(CloudAPIError) as excinfo: + deleteFeedback("not-a-valid-id", client=client) + assert excinfo.value.status_code == 400 + + def test_empty_id_is_rejected_without_a_request(self): + client = make_client(response(204, None)) + with pytest.raises(pydantic.ValidationError): + deleteFeedback("", client=client) + assert client._session.calls == [] + + +def test_registered_in_package(): + import ndi.cloud.api as api + + assert "feedback" in api.__all__ + assert api.feedback.submitFeedback is submitFeedback diff --git a/tests/test_cloud_feedback_live.py b/tests/test_cloud_feedback_live.py new file mode 100644 index 00000000..1e16bc86 --- /dev/null +++ b/tests/test_cloud_feedback_live.py @@ -0,0 +1,266 @@ +""" +Live tests for the in-app problem report endpoints. + +Ported from NDI-matlab tests/+ndi/+unittest/+cloud/feedbackTest.m +(75f937a76, NDI-matlab#1023). Exercises ndi.cloud.api.feedback against +the real API (ndi-cloud-node#158): + + POST /feedback + DELETE /feedback/{feedbackId} + +Every report a test submits is deleted again -- by the test itself, or by +the ``submitted`` fixture's teardown if the test fails part-way -- so the +suite does not leave reports in the admin triage queue. Deleted reports do +not count against the server's limit of 10 reports per user per hour. + +Where MATLAB checks ``b == false`` and a status code, Python checks the +exception the call raises and its ``status_code``, since every +ndi.cloud.api function raises on a non-2xx response. + +The client-side validation tests of feedbackTest.m +(testWrapperRejectsUnknownCategory / testWrapperRejectsEmptyDescription) +need no network and live in tests/test_cloud_api_feedback.py. + +Environment variables consumed: + NDI_CLOUD_USERNAME / NDI_CLOUD_PASSWORD -- skip if absent (standard + live-test pattern, matches test_cloud_live.py). + CLOUD_API_ENVIRONMENT -- 'prod' (default) or 'dev'. The module skips + in an environment that does not have the feedback endpoint yet. +""" + +from __future__ import annotations + +import os +import platform +import sys + +import pytest + +_has_creds = bool(os.environ.get("NDI_CLOUD_USERNAME") and os.environ.get("NDI_CLOUD_PASSWORD")) +pytestmark = pytest.mark.skipif(not _has_creds, reason="NDI cloud credentials not set") + +APP_NAME = "NDI-python-unittest" +#: Valid 24-hex id that will not match any stored report. +NONEXISTENT_ID = "000000000000000000000000" + + +# --------------------------------------------------------------------------- +# Fixtures +# --------------------------------------------------------------------------- + + +@pytest.fixture(scope="module") +def cloud_config(): + """Authenticate once for the entire module and return config.""" + from ndi.cloud.auth import login + from ndi.cloud.config import CloudConfig + + config = CloudConfig.from_env() + config = login(config=config) + assert config.is_authenticated, "Login failed -- no token received" + return config + + +@pytest.fixture(scope="module") +def client(cloud_config): + """Return an authenticated CloudClient.""" + from ndi.cloud.client import CloudClient + + return CloudClient(cloud_config) + + +@pytest.fixture(scope="module", autouse=True) +def endpoint_deployed(client): + """Skip (rather than fail) where the feedback endpoint is not deployed. + + The cloud suite runs against both prod and dev. Once deployed, a + malformed id is answered 400 by the route; before that, the API's + catch-all answers 404. Mirrors feedbackTest.checkEndpointDeployed. + """ + from ndi.cloud.api.feedback import deleteFeedback + from ndi.cloud.exceptions import CloudAPIError, CloudNotFoundError + + try: + deleteFeedback("not-a-valid-id", client=client) + except CloudNotFoundError: + pytest.skip("The /feedback endpoint is not deployed in this API environment") + except CloudAPIError: + pass + + +@pytest.fixture() +def submitted(client): + """Collect ids of submitted reports; delete any still present at teardown. + + Mirrors feedbackTest.registerCleanup. A 404 here just means the test + already deleted the report itself. + """ + from ndi.cloud.api.feedback import deleteFeedback + from ndi.cloud.exceptions import CloudError + + ids: list[str] = [] + yield ids + for feedback_id in ids: + try: + deleteFeedback(feedback_id, client=client) + except CloudError: + pass + + +def _submit_raw(client, body): + """POST BODY as-is, bypassing the wrapper's own validation. + + MATLAB reaches the server's validation through the implementation class + ndi.cloud.api.implementation.feedback.SubmitFeedback; feedback._submit + is its counterpart. + """ + from ndi.cloud.api.feedback import _submit + + return _submit(body, client=client) + + +def _expect_rejected(client, submitted, body, status): + """Submit BODY raw and require the server to refuse it with STATUS.""" + from ndi.cloud.exceptions import CloudAPIError + + try: + result = _submit_raw(client, body) + except CloudAPIError as exc: + assert exc.status_code == status, exc + return + submitted.append(result.data) + pytest.fail(f"server accepted an invalid report (id {result.data}); expected HTTP {status}") + + +# --------------------------------------------------------------------------- +# Tests +# --------------------------------------------------------------------------- + + +class TestFeedbackLive: + def test_submit_and_delete(self, client, submitted): + """Submit a minimal report, delete it, and confirm a second delete is 404.""" + from ndi.cloud.api.feedback import deleteFeedback, submitFeedback + from ndi.cloud.exceptions import CloudNotFoundError + + b, feedback_id, status, url = submitFeedback( + APP_NAME, "other", "feedbackTest: minimal report (safe to ignore)", client=client + ) + assert b is True, url + assert status == 201, url + assert isinstance(feedback_id, str) and feedback_id, url + submitted.append(feedback_id) + + b_del, answer, status_del, url_del = deleteFeedback(feedback_id, client=client) + assert b_del is True, url_del + assert status_del == 204, url_del + assert answer == "Feedback deleted successfully." + + with pytest.raises(CloudNotFoundError): + deleteFeedback(feedback_id, client=client) + + def test_submit_full_report(self, client, submitted): + """Every optional field, including a log carrying credentials to redact. + + The reporter cannot read reports back, so redaction itself is + checked in the server's own tests. + """ + from ndi.cloud.api.feedback import deleteFeedback, submitFeedback + + environment = { + "os": platform.platform(), + "python": sys.version.split()[0], + "launchedFrom": "pytest", + } + state = {"level": 3, "fetchStats": {"ok": 10, "failed": 2}} + fake_token = "eyJhbGciOiJIUzI1NiJ9.eyJzdWIiOiJ1bml0dGVzdCJ9.c2lnbmF0dXJlX3VuaXR0ZXN0" + log_text = ( + "[lightsheet] GET https://bucket.s3.amazonaws.com/k?X-Amz-Signature=deadbeef01\n" + f"Authorization: Bearer {fake_token}" + ) + + result = submitFeedback( + APP_NAME, + "bug", + "feedbackTest: full report (safe to ignore)", + appVersion="NDI-python unittest", + datasetId=NONEXISTENT_ID, + documentId="unittest-document", + environment=environment, + state=state, + log=log_text, + traceback="feedbackTest: no real traceback", + client=client, + ) + assert result.success, result.url + assert result.status_code == 201, result.url + submitted.append(result.data) + + assert deleteFeedback(result.data, client=client).success + + def test_invalid_category_is_rejected(self, client, submitted): + body = { + "schemaVersion": 1, + "app": APP_NAME, + "category": "feature", + "description": "feedbackTest: invalid category", + } + _expect_rejected(client, submitted, body, 400) + + def test_missing_description_is_rejected(self, client, submitted): + body = {"schemaVersion": 1, "app": APP_NAME, "category": "bug"} + _expect_rejected(client, submitted, body, 400) + + def test_unsupported_schema_version_is_rejected(self, client, submitted): + body = { + "schemaVersion": 99, + "app": APP_NAME, + "category": "bug", + "description": "feedbackTest: bad schemaVersion", + } + _expect_rejected(client, submitted, body, 400) + + def test_oversize_log_is_rejected(self, client, submitted): + """A log over the 500,000-character field cap but under the 1 MB request cap.""" + from ndi.cloud.api.feedback import submitFeedback + from ndi.cloud.exceptions import CloudAPIError + + try: + result = submitFeedback( + APP_NAME, + "performance", + "feedbackTest: oversize log", + log="x" * 600_000, + client=client, + ) + except CloudAPIError as exc: + assert exc.status_code == 413, exc + return + submitted.append(result.data) + pytest.fail("server accepted a 600,000-character log; expected HTTP 413") + + def test_unauthenticated_submit_is_rejected(self, cloud_config): + """No token: refused. The API answers 403 throughout; 401 is accepted too.""" + from ndi.cloud.api.feedback import submitFeedback + from ndi.cloud.client import CloudClient + from ndi.cloud.config import CloudConfig + from ndi.cloud.exceptions import CloudAuthError + + anonymous = CloudClient(CloudConfig(api_url=cloud_config.api_url, token="")) + with pytest.raises(CloudAuthError): + submitFeedback(APP_NAME, "bug", "feedbackTest: no token", client=anonymous) + + def test_delete_nonexistent_report(self, client): + from ndi.cloud.api.feedback import deleteFeedback + from ndi.cloud.exceptions import CloudNotFoundError + + with pytest.raises(CloudNotFoundError): + deleteFeedback(NONEXISTENT_ID, client=client) + + def test_delete_malformed_id(self, client): + from ndi.cloud.api.feedback import deleteFeedback + from ndi.cloud.exceptions import CloudAPIError + + with pytest.raises(CloudAPIError) as excinfo: + deleteFeedback("not-a-valid-id", client=client) + assert excinfo.value.status_code == 400 From 03e5f268f645da0e0ffc1c853b3ea8a896a66ba7 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 12 Oct 2026 00:33:01 +0000 Subject: [PATCH 3/5] Bridge checks: verify MATLAB counterparts that live on an NDI-matlab branch NDI-python main records MATLAB files (the lightsheet OME-Zarr functions, LightsheetZarrManager, resolveOrganizationId, and six ndi_common document files) that exist only on VH-Lab/NDI-matlab claude/lightsheet-zarr-ndi-viewer-djp5vk, which has not merged. CI checks out NDI-matlab main, so the bridge job has been red on main since 2026-09-26, reporting them as stale paths. Add a matlab_branch field (and MATLAB_BRANCH_PENDING for ndi_common) that names the branch, and check the claim rather than skip it: - the branch must exist and the file be on it (ndi_common: byte-identical); - drift is measured against the branch tip instead of HEAD; - once the branch merges into NDI-matlab main, the marker must be removed. Helpers in tests/_matlab_branch.py, tested against a throwaway git repo in tests/test_matlab_branch_pending.py. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01B22A1gLVKa1E3uQm3tv4t8 --- tests/_matlab_branch.py | 71 +++++++++++++++++ tests/test_matlab_branch_pending.py | 99 ++++++++++++++++++++++++ tests/test_matlab_bridge_completeness.py | 97 ++++++++++++++++++++++- tests/test_matlab_bridge_hashes.py | 39 +++++++--- tests/test_ndi_common_matlab_sync.py | 62 ++++++++++++++- 5 files changed, 355 insertions(+), 13 deletions(-) create mode 100644 tests/_matlab_branch.py create mode 100644 tests/test_matlab_branch_pending.py diff --git a/tests/_matlab_branch.py b/tests/_matlab_branch.py new file mode 100644 index 00000000..6ff1c30e --- /dev/null +++ b/tests/_matlab_branch.py @@ -0,0 +1,71 @@ +"""Reading a named NDI-matlab branch, for counterparts not yet on its main. + +Some NDI-python work lands before its MATLAB half: the lightsheet OME-Zarr +documents and functions, and ``resolveOrganizationId``, were written as a +pair with VH-Lab/NDI-matlab ``claude/lightsheet-zarr-ndi-viewer-djp5vk`` and +merged here while that branch was still open. The bridge checks compare +against the checked-out NDI-matlab tree (its ``main`` unless a branch of the +same name exists), so those records looked stale -- a MATLAB file "that does +not exist" -- when the truth is "exists, not merged yet". + +A bridge entry says so with ``matlab_branch: ``, and an +``ndi_common`` file with an entry in ``MATLAB_BRANCH_PENDING``. The checks +then verify the claim against that branch instead of skipping it: the file +must be there, drift is measured against the branch tip, and once the branch +is merged the marker must go (see section 8 of +docs/developer_notes/ndi_matlab_python_bridge.yaml). + +These helpers read the branch through git, so they need the NDI-matlab +checkout's remote branches -- which ``actions/checkout`` provides with +``fetch-depth: 0``, and the bridge job already asks for that. +""" + +from __future__ import annotations + +import subprocess +from pathlib import Path +from typing import Any + + +def pending_branch(entry: dict[str, Any]) -> str: + """The entry's ``matlab_branch``, or ``""`` when it names none.""" + value = entry.get("matlab_branch") + return value.strip() if isinstance(value, str) else "" + + +def _git(repo: Path, *args: str) -> subprocess.CompletedProcess: + return subprocess.run(["git", "-C", str(repo), *args], capture_output=True, text=False) + + +def branch_ref(repo: Path, branch: str) -> str | None: + """The commit at the tip of BRANCH in REPO, or None if it is not there. + + Tries the remote-tracking ref first, since that is what a CI checkout + holds, then a local branch of that name. + """ + for ref in (f"refs/remotes/origin/{branch}", f"refs/heads/{branch}"): + result = _git(repo, "rev-parse", "--verify", "--quiet", f"{ref}^{{commit}}") + if result.returncode == 0: + return result.stdout.decode().strip() + return None + + +def read_on_branch(repo: Path, commit: str, repo_path: str) -> bytes | None: + """The bytes of REPO_PATH (relative to the repo root) at COMMIT, or None.""" + result = _git(repo, "show", f"{commit}:{repo_path}") + return result.stdout if result.returncode == 0 else None + + +#: NDI-matlab's default branch. "Merged" means merged here, not into whatever +#: happens to be checked out: CI checks out a same-named NDI-matlab branch +#: when one exists, and on the pending branch itself HEAD contains its own +#: tip by definition. +MAIN_BRANCH = "main" + + +def is_merged(repo: Path, commit: str) -> bool: + """True if COMMIT is already part of NDI-matlab main.""" + main = branch_ref(repo, MAIN_BRANCH) + if main is None: + return False + return _git(repo, "merge-base", "--is-ancestor", commit, main).returncode == 0 diff --git a/tests/test_matlab_branch_pending.py b/tests/test_matlab_branch_pending.py new file mode 100644 index 00000000..b0d3a2d6 --- /dev/null +++ b/tests/test_matlab_branch_pending.py @@ -0,0 +1,99 @@ +"""The ``matlab_branch`` helpers answer correctly, against a real git repo. + +MATLAB counterpart: none; this tests the bridge checks themselves. + +``matlab_branch`` lets a bridge entry (and ``MATLAB_BRANCH_PENDING`` an +``ndi_common`` file) say "the MATLAB side is on this NDI-matlab branch, not +yet on main" -- section 8 of docs/developer_notes/ndi_matlab_python_bridge.yaml. +The checks exempt such a record from "file must exist in the checked-out +tree" and verify it against the branch instead, so everything rests on +:mod:`tests._matlab_branch` telling the truth. A helper that always said +"found" would let any typo through; one that never said "merged" would let +the marker outlive its branch. + +The repo is built in a temporary directory, so this needs no NDI-matlab +checkout and runs in every job. +""" + +from __future__ import annotations + +import subprocess +from pathlib import Path + +import pytest + +from tests._matlab_branch import branch_ref, is_merged, pending_branch, read_on_branch + +FILE = "src/ndi/+ndi/+fun/+doc/+lightsheet/view.m" + + +def git(repo: Path, *args: str) -> str: + return subprocess.run( + ["git", "-C", str(repo), *args], capture_output=True, text=True, check=True + ).stdout.strip() + + +@pytest.fixture +def repo(tmp_path: Path) -> Path: + """main with one commit; ``feature`` one commit ahead, adding FILE.""" + git(tmp_path, "init", "-q", "-b", "main") + git(tmp_path, "config", "user.email", "test@example.invalid") + git(tmp_path, "config", "user.name", "test") + (tmp_path / "README").write_text("main\n") + git(tmp_path, "add", "README") + git(tmp_path, "commit", "-q", "-m", "main") + git(tmp_path, "checkout", "-q", "-b", "feature") + (tmp_path / FILE).parent.mkdir(parents=True) + (tmp_path / FILE).write_text("function view()\n") + git(tmp_path, "add", FILE) + git(tmp_path, "commit", "-q", "-m", "feature") + git(tmp_path, "checkout", "-q", "main") + return tmp_path + + +def test_a_branch_resolves_to_its_tip(repo): + assert branch_ref(repo, "feature") == git(repo, "rev-parse", "feature") + + +def test_a_missing_branch_resolves_to_nothing(repo): + assert branch_ref(repo, "no-such-branch") is None + + +def test_a_file_on_the_branch_is_read_from_it(repo): + tip = branch_ref(repo, "feature") + assert read_on_branch(repo, tip, FILE) == b"function view()\n" + assert not (repo / FILE).exists(), "the file is on the branch, not the checkout" + + +def test_a_file_not_on_the_branch_is_reported_absent(repo): + tip = branch_ref(repo, "feature") + assert read_on_branch(repo, tip, FILE.replace("view", "veiw")) is None + + +def test_an_unmerged_branch_is_not_merged(repo): + assert not is_merged(repo, branch_ref(repo, "feature")) + + +def test_checking_out_the_branch_does_not_make_it_merged(repo): + """CI checks out a same-named NDI-matlab branch when one exists. HEAD + then contains the branch tip, which must not read as "merged".""" + git(repo, "checkout", "-q", "feature") + assert not is_merged(repo, branch_ref(repo, "feature")) + + +def test_a_branch_merged_into_main_is_merged(repo): + git(repo, "merge", "-q", "--no-ff", "-m", "merge", "feature") + assert is_merged(repo, branch_ref(repo, "feature")) + + +@pytest.mark.parametrize( + "entry, expected", + [ + ({"matlab_branch": "feature"}, "feature"), + ({"matlab_branch": " feature "}, "feature"), + ({}, ""), + ({"matlab_branch": None}, ""), + ], +) +def test_pending_branch(entry, expected): + assert pending_branch(entry) == expected diff --git a/tests/test_matlab_bridge_completeness.py b/tests/test_matlab_bridge_completeness.py index 6a8af92a..ac72410b 100644 --- a/tests/test_matlab_bridge_completeness.py +++ b/tests/test_matlab_bridge_completeness.py @@ -50,6 +50,8 @@ import pytest import yaml +from tests._matlab_branch import branch_ref, is_merged, pending_branch, read_on_branch + #: Set by CI once NDI-matlab is checked out. See the module docstring. STRICT_ENV_VAR = "NDI_BRIDGE_CHECK_STRICT" @@ -364,15 +366,24 @@ def test_every_recorded_matlab_path_points_at_a_real_file(self, package: Bridged root = require_matlab_root() index = read_bridge_index(package) prefix = f"{package.matlab_dir}/" + # A path recorded with matlab_branch is checked against that branch + # by TestBranchPendingEntries instead -- its absence from the + # checked-out tree is the claim, not a defect. + pending = {path for _, path, _ in branch_pending_entries(package)} stale = sorted( - path for path in index.paths if path.startswith(prefix) and not (root / path).exists() + path + for path in index.paths + if path.startswith(prefix) and path not in pending and not (root / path).exists() ) assert not stale, ( f"{BRIDGE_FILENAME} entries under {package.python_dir} name MATLAB " "files that do not exist:\n " + "\n ".join(stale) + '\n\nPoint each at its new path, or set matlab_path: "N/A" and say ' - "in the decision_log that MATLAB removed it." + "in the decision_log that MATLAB removed it. If the file is on an " + "NDI-matlab branch that has not merged yet, add matlab_branch: " + " to the entry instead (section 8 of " + "docs/developer_notes/ndi_matlab_python_bridge.yaml)." ) @pytest.mark.parametrize("package", PACKAGES, ids=lambda p: p.id) @@ -396,6 +407,74 @@ def test_every_excluded_layer_carries_a_reason(self, package: BridgedPackage): assert len(reason.split()) >= 10, f"exclusion {prefix!r} needs a real reason" +def branch_pending_entries(package: BridgedPackage) -> list[tuple[str, str, dict[str, Any]]]: + """``(source, matlab_path, entry)`` for every entry carrying ``matlab_branch``. + + ``matlab_path`` is normalized; it is ``""`` when the entry names no file, + which :class:`TestBranchPendingEntries` reports. + """ + found = [] + for source in sorted((REPO_ROOT / package.python_dir).rglob(BRIDGE_FILENAME)): + data = yaml.safe_load(source.read_text(encoding="utf-8")) + for entry in _entries_with_a_matlab_branch(data): + path = normalize_matlab_path(entry.get("matlab_path") or "") + found.append((str(source.relative_to(REPO_ROOT)), path, entry)) + return found + + +class TestBranchPendingEntries: + """``matlab_branch`` is a claim, and the claim is checked. + + An entry may record a MATLAB file that is on a named NDI-matlab branch + but not yet on the tree this job checked out (section 8 of + docs/developer_notes/ndi_matlab_python_bridge.yaml). The stale-path + guard above exempts it, so this has to hold it to account instead, or + ``matlab_branch`` would be a way to silence that guard for good: + + * the branch must exist and the file must be on it -- a typo in either + is the stale path the exemption would otherwise hide; + * once the branch has merged into NDI-matlab main, the marker must go, + so an entry does not keep pointing at a branch after its file has + reached main (and the branch, typically, has been deleted). + """ + + @pytest.mark.parametrize("package", PACKAGES, ids=lambda p: p.id) + def test_every_branch_pending_file_is_on_its_branch(self, package: BridgedPackage): + root = require_matlab_root() + repo = root.parent.parent + offenders, unresolved = [], [] + for source, path, entry in branch_pending_entries(package): + name = entry.get("name", "") + branch = pending_branch(entry) + if not path: + offenders.append(f"{source}: {name} -- matlab_branch needs a real matlab_path") + continue + tip = branch_ref(repo, branch) + if tip is None: + unresolved.append(f"{source}: {name} -- branch {branch!r} not found") + continue + if read_on_branch(repo, tip, f"src/ndi/{path}") is None: + offenders.append(f"{source}: {name} -- {path} is not on {branch!r}") + elif is_merged(repo, tip): + offenders.append( + f"{source}: {name} -- {branch!r} has merged into NDI-matlab main; " + "remove matlab_branch" + ) + if unresolved and not os.environ.get(STRICT_ENV_VAR, "").strip(): + pytest.skip( + "this NDI-matlab checkout lacks branches the bridge names " + "(fetch them, or clone with full history): " + "; ".join(unresolved) + ) + problems = offenders + unresolved + assert not problems, ( + "bridge entries recorded against an NDI-matlab branch do not hold up:\n " + + "\n ".join(problems) + + "\n\nA branch that is not found was usually merged and deleted: check the " + "file is on NDI-matlab main and remove matlab_branch from the entry. See " + "section 8 of docs/developer_notes/ndi_matlab_python_bridge.yaml." + ) + + class TestTheDeferralsSayWhy: """``not_yet_ported`` and ``not_applicable`` are decisions, not labels. @@ -606,6 +685,20 @@ def _entries_with_a_matlab_path(node: Any) -> list[dict[str, Any]]: return found +def _entries_with_a_matlab_branch(node: Any) -> list[dict[str, Any]]: + """Every mapping in the tree that carries a ``matlab_branch:`` key.""" + found: list[dict[str, Any]] = [] + if isinstance(node, dict): + if isinstance(node.get("matlab_branch"), str): + found.append(node) + for value in node.values(): + found.extend(_entries_with_a_matlab_branch(value)) + elif isinstance(node, list): + for item in node: + found.extend(_entries_with_a_matlab_branch(item)) + return found + + def _entries_with_status(node: Any) -> list[dict[str, Any]]: """Every mapping in the tree that carries a ``status:`` key.""" found: list[dict[str, Any]] = [] diff --git a/tests/test_matlab_bridge_hashes.py b/tests/test_matlab_bridge_hashes.py index 7588036f..6427c034 100644 --- a/tests/test_matlab_bridge_hashes.py +++ b/tests/test_matlab_bridge_hashes.py @@ -81,6 +81,7 @@ import pytest import yaml +from tests._matlab_branch import branch_ref, pending_branch from tests.test_matlab_bridge_completeness import ( BRIDGE_FILENAME, REPO_ROOT, @@ -240,11 +241,28 @@ def drift_key(entry: dict[str, Any]) -> tuple[str, str]: return (matlab_file(entry), entry.get("name", "")) -def has_drifted(repo: Path, entry: dict[str, Any]) -> bool: +def drift_target(repo: Path, entry: dict[str, Any]) -> str | None: + """The NDI-matlab revision this entry's drift is measured against. + + ``HEAD`` when the file is in the checked-out tree. An entry whose file is + only on a not-yet-merged branch (``matlab_branch``, section 8 of + docs/developer_notes/ndi_matlab_python_bridge.yaml) is measured against + that branch's tip, so recording one does not exempt it from drift. None + when there is nothing to measure against -- a file on neither, which the + completeness guard reports. + """ + if (repo / "src" / "ndi" / matlab_file(entry)).exists(): + return "HEAD" + branch = pending_branch(entry) + return branch_ref(repo, branch) if branch else None + + +def has_drifted(repo: Path, entry: dict[str, Any], target: str = "HEAD") -> bool: """Have commits touching this entry's MATLAB file landed since its hash? Path-filtered on purpose: an unrelated commit elsewhere in NDI-matlab is - not this entry's problem and must not turn CI red. + not this entry's problem and must not turn CI red. TARGET is the + revision to measure up to; see :func:`drift_target`. """ full = f"src/ndi/{matlab_file(entry)}" result = subprocess.run( @@ -253,7 +271,7 @@ def has_drifted(repo: Path, entry: dict[str, Any]) -> bool: "-C", str(repo), "log", - f"{entry['matlab_last_sync_hash']}..HEAD", + f"{entry['matlab_last_sync_hash']}..{target}", "--oneline", "--", full, @@ -300,13 +318,14 @@ def test_no_entry_has_drifted(self): key = drift_key(entry) if key in allowed: continue - full = repo / "src" / "ndi" / key[0] - if not full.exists(): + target = drift_target(repo, entry) + if target is None: continue - if has_drifted(repo, entry): + if has_drifted(repo, entry, target): + where = "" if target == "HEAD" else f" on {pending_branch(entry)!r}" offenders.append( f"{display_path(source)}: {key[1]} ({key[0]}) -- MATLAB has " - f"moved since {entry['matlab_last_sync_hash']}" + f"moved{where} since {entry['matlab_last_sync_hash']}" ) assert not offenders, ( "these bridge entries have drifted -- NDI-matlab changed the file after " @@ -314,6 +333,7 @@ def test_no_entry_has_drifted(self): "known to match:\n " + "\n ".join(offenders) + "\n\n" "Read the diff:\n" " git -C ../NDI-matlab diff ..HEAD -- src/ndi/\n" + "(for an entry with matlab_branch, diff up to origin/ instead)\n" "then port the behavioural change and bump the hash, or -- if nothing on " "the Python side must follow -- just bump it. No decision_log is required " "for a regular port. See section 7 of " @@ -361,9 +381,10 @@ def test_the_allowlist_has_no_stale_entries(self): if not isinstance(entry.get("matlab_last_sync_hash"), str): live_nohash.add(key) continue - if not (repo / "src" / "ndi" / key[0]).exists(): + target = drift_target(repo, entry) + if target is None: continue - if has_drifted(repo, entry): + if has_drifted(repo, entry, target): live_drifted.add(key) fixed = sorted( [f"drifted: {n} ({p})" for p, n in drifted_allowed - live_drifted] diff --git a/tests/test_ndi_common_matlab_sync.py b/tests/test_ndi_common_matlab_sync.py index 779a4e96..32a5590d 100644 --- a/tests/test_ndi_common_matlab_sync.py +++ b/tests/test_ndi_common_matlab_sync.py @@ -36,6 +36,8 @@ import pytest +from tests._matlab_branch import branch_ref, is_merged, read_on_branch + NDI_COMMON_REL = Path("src/ndi/ndi_common") #: Set by CI once NDI-matlab is checked out. See the module docstring. @@ -125,6 +127,25 @@ def _require_matlab_repo() -> Path: ) +# Present in Python and on a named NDI-matlab branch that has not merged into +# the checked-out tree yet. Mapped to that branch. Not an exclusion: each file +# must match its copy on the branch byte-for-byte, and the entry must go once +# the branch merges (test_branch_pending_files_match_their_branch). Section 8 +# of docs/developer_notes/ndi_matlab_python_bridge.yaml. +_LIGHTSHEET_BRANCH = "claude/lightsheet-zarr-ndi-viewer-djp5vk" +MATLAB_BRANCH_PENDING: dict[str, str] = { + # The lightsheet OME-Zarr document types, landed here with the napari + # viewer (src/ndi/gui/app/lightsheetZarr) and authored as a pair with + # the MATLAB writer on this branch. + "database_documents/data/lightsheetZarrLevel.json": _LIGHTSHEET_BRANCH, + "database_documents/data/lightsheetZarrLevel.md": _LIGHTSHEET_BRANCH, + "database_documents/data/lightsheetZarrPyramid.json": _LIGHTSHEET_BRANCH, + "database_documents/data/lightsheetZarrPyramid.md": _LIGHTSHEET_BRANCH, + "schema_documents/data/lightsheetZarrLevel_schema.json": _LIGHTSHEET_BRANCH, + "schema_documents/data/lightsheetZarrPyramid_schema.json": _LIGHTSHEET_BRANCH, +} + + # Files present on both sides that intentionally do not match byte-for-byte. # Each entry pairs the relative path with a one-line reason so the exclusion # is not silent. When one of these converges, the test asks the caller to @@ -183,11 +204,48 @@ def test_no_undeclared_python_only_files(python_common, matlab_common): py = _list_files(python_common) ml = _list_files(matlab_common) extra = py - ml - unexpected = extra - PYTHON_ONLY_EXPECTED + unexpected = extra - PYTHON_ONLY_EXPECTED - MATLAB_BRANCH_PENDING.keys() assert not unexpected, ( "The following files exist in NDI-python/ndi_common but not in " "NDI-matlab. Either remove them, or add them to PYTHON_ONLY_EXPECTED " - "with a reason:\n " + "\n ".join(sorted(unexpected)) + "with a reason (or, if they are on an NDI-matlab branch that has not " + "merged yet, to MATLAB_BRANCH_PENDING):\n " + "\n ".join(sorted(unexpected)) + ) + + +def test_branch_pending_files_match_their_branch(python_common, matlab_common): + """Every MATLAB_BRANCH_PENDING file is on its branch, byte-for-byte. + + The map exempts these files from the Python-only check above, so this is + what keeps it honest: a file that is not on the branch, or differs from + it, is an undeclared divergence; and a branch that has merged into + NDI-matlab main means the entry is no longer needed. + """ + repo = matlab_common.parent.parent.parent + offenders, unresolved = [], [] + for rel, branch in sorted(MATLAB_BRANCH_PENDING.items()): + tip = branch_ref(repo, branch) + if tip is None: + unresolved.append(f"{rel}: branch {branch!r} not found") + continue + theirs = read_on_branch(repo, tip, f"{NDI_COMMON_REL.as_posix()}/{rel}") + if theirs is None: + offenders.append(f"{rel}: not on {branch!r}") + elif theirs != (python_common / rel).read_bytes(): + offenders.append(f"{rel}: differs from its copy on {branch!r}") + elif is_merged(repo, tip): + offenders.append(f"{rel}: {branch!r} has merged; remove it from MATLAB_BRANCH_PENDING") + if unresolved and not os.environ.get(STRICT_ENV_VAR, "").strip(): + pytest.skip( + "this NDI-matlab checkout lacks branches MATLAB_BRANCH_PENDING names " + "(fetch them, or clone with full history): " + "; ".join(unresolved) + ) + problems = offenders + unresolved + assert not problems, ( + "MATLAB_BRANCH_PENDING entries do not hold up:\n " + + "\n ".join(problems) + + "\n\nA branch that is not found was usually merged and deleted: check the " + "file is on NDI-matlab main and remove its MATLAB_BRANCH_PENDING entry." ) From 26f859a562b961ed9b1c09680fa306469afe5f10 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 12 Oct 2026 00:33:01 +0000 Subject: [PATCH 4/5] Bridge: record branch-pending MATLAB files; clear decodeJwt and lightsheet drift - matlab_branch on resolveOrganizationId, LightsheetZarrManager and the eight +ndi/+fun/+doc/+lightsheet entries. - fromOMEZarr / makePyramid / chooseTileShape: examined against 5c9e8a83 on that branch (default tile budget 8 MB -> 32 MB). No Python counterpart yet, nothing to follow; noted in each decision_log. - decodeJwt: examined against 4a571dce (NDI-matlab#1024, Java base64 decoder replaced by matlab.net.base64decode). Python uses the stdlib; nothing to follow. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01B22A1gLVKa1E3uQm3tv4t8 --- src/ndi/cloud/ndi_matlab_python_bridge.yaml | 10 ++++- .../ndi_matlab_python_bridge.yaml | 38 +++++++++++++++++-- .../ndi_matlab_python_bridge.yaml | 1 + 3 files changed, 45 insertions(+), 4 deletions(-) diff --git a/src/ndi/cloud/ndi_matlab_python_bridge.yaml b/src/ndi/cloud/ndi_matlab_python_bridge.yaml index 80046115..77f35ad2 100644 --- a/src/ndi/cloud/ndi_matlab_python_bridge.yaml +++ b/src/ndi/cloud/ndi_matlab_python_bridge.yaml @@ -272,7 +272,7 @@ functions: - name: decodeJwt matlab_path: "+ndi/+cloud/+internal/decodeJwt.m" - matlab_last_sync_hash: "7ab96514" + matlab_last_sync_hash: "4a571dce" python_path: "ndi/cloud/auth.py" input_arguments: - name: token @@ -284,6 +284,13 @@ functions: decision_log: > MATLAB places this in +internal; Python places it in auth.py since it is tightly coupled with authentication logic. Name exact match. + 2026-10-12: examined against 4a571dce (was 7ab96514), NDI-matlab#1024. + MATLAB swapped javax.xml.bind.DatatypeConverter.parseBase64Binary, + which recent MATLAB releases no longer ship, for + matlab.net.base64decode. Same base64url-to-base64 translation and + padding before it, same bytes out. Nothing to follow: Python decodes + with the standard library's base64 module and never had the Java + dependency. - name: getTokenExpiration matlab_path: "+ndi/+cloud/+internal/getTokenExpiration.m" @@ -1610,6 +1617,7 @@ not_yet_ported: - name: resolveOrganizationId matlab_path: "+ndi/+cloud/+internal/resolveOrganizationId.m" matlab_last_sync_hash: "f847d7c4" + matlab_branch: "claude/lightsheet-zarr-ndi-viewer-djp5vk" status: porting_deferred decision_log: > MATLAB helper introduced with NDI-matlab#979: given diff --git a/src/ndi/fun/doc_lightsheet/ndi_matlab_python_bridge.yaml b/src/ndi/fun/doc_lightsheet/ndi_matlab_python_bridge.yaml index b30f2eaf..b6545fc4 100644 --- a/src/ndi/fun/doc_lightsheet/ndi_matlab_python_bridge.yaml +++ b/src/ndi/fun/doc_lightsheet/ndi_matlab_python_bridge.yaml @@ -31,7 +31,8 @@ functions: - name: fromOMEZarr matlab_path: "+ndi/+fun/+doc/+lightsheet/fromOMEZarr.m" - matlab_last_sync_hash: "71d7387" + matlab_last_sync_hash: "5c9e8a83" + matlab_branch: "claude/lightsheet-zarr-ndi-viewer-djp5vk" status: porting_deferred decision_log: >- MATLAB probes an OME-Zarr store with ndr.format.omezarr.listPyramids, @@ -47,10 +48,19 @@ functions: database_add -- the Python port lands the same option and calls into ndr.format.omezarr.readArray equivalent. Ports with the other entries under this heading. + 2026-10-12: examined against 5c9e8a83 on + claude/lightsheet-zarr-ndi-viewer-djp5vk (was 71d7387). It raises + the default tile budget from 8*2^20 to 32*2^20 bytes + (signing/fetch/decode measurements on a whole-brain volume) and + updates the doc comments to match. Nothing on the Python side to + follow: this function has no Python counterpart yet, and the + Python viewer only reads the levels MATLAB wrote. The Python port + must use the 32 MB default when it lands. - name: makePyramid matlab_path: "+ndi/+fun/+doc/+lightsheet/makePyramid.m" - matlab_last_sync_hash: "8134a20" + matlab_last_sync_hash: "5c9e8a83" + matlab_branch: "claude/lightsheet-zarr-ndi-viewer-djp5vk" status: porting_deferred decision_log: >- The worker fromOMEZarr calls; writes one lightsheetZarrPyramid @@ -81,10 +91,19 @@ functions: is never touched). Container bytes are unchanged, so this side of the port stays the same when it lands: use numcodecs.Blosc directly. Ports with the other entries. + 2026-10-12: examined against 5c9e8a83 on + claude/lightsheet-zarr-ndi-viewer-djp5vk (was 8134a20). It raises + the default tile budget from 8*2^20 to 32*2^20 bytes + (signing/fetch/decode measurements on a whole-brain volume) and + updates the doc comments to match. Nothing on the Python side to + follow: this function has no Python counterpart yet, and the + Python viewer only reads the levels MATLAB wrote. The Python port + must use the 32 MB default when it lands. - name: chooseTileShape matlab_path: "+ndi/+fun/+doc/+lightsheet/chooseTileShape.m" - matlab_last_sync_hash: "1d11302" + matlab_last_sync_hash: "5c9e8a83" + matlab_branch: "claude/lightsheet-zarr-ndi-viewer-djp5vk" status: porting_deferred decision_log: >- Pure function that picks an OME-Zarr chunk shape landing near a @@ -102,10 +121,19 @@ functions: in axes_order.lower()]`. Ports with the other entries under this heading; the Python builder that mirrors makePyramid will call it directly. + 2026-10-12: examined against 5c9e8a83 on + claude/lightsheet-zarr-ndi-viewer-djp5vk (was 1d11302). It raises + the default tile budget from 8*2^20 to 32*2^20 bytes + (signing/fetch/decode measurements on a whole-brain volume) and + updates the doc comments to match. Nothing on the Python side to + follow: this function has no Python counterpart yet, and the + Python viewer only reads the levels MATLAB wrote. The Python port + must use the 32 MB default when it lands. - name: makeSourceFile matlab_path: "+ndi/+fun/+doc/+lightsheet/makeSourceFile.m" matlab_last_sync_hash: "1af66bb" + matlab_branch: "claude/lightsheet-zarr-ndi-viewer-djp5vk" status: porting_deferred decision_log: >- MATLAB creates the fileReference document the lightsheet documents' @@ -120,6 +148,7 @@ functions: - name: chooseLevel matlab_path: "+ndi/+fun/+doc/+lightsheet/chooseLevel.m" matlab_last_sync_hash: "5ab84f3" + matlab_branch: "claude/lightsheet-zarr-ndi-viewer-djp5vk" status: porting_deferred decision_log: >- Given a level table and a target per-axis voxel size, returns the @@ -130,6 +159,7 @@ functions: - name: levelTable matlab_path: "+ndi/+fun/+doc/+lightsheet/levelTable.m" matlab_last_sync_hash: "1af66bb" + matlab_branch: "claude/lightsheet-zarr-ndi-viewer-djp5vk" status: porting_deferred decision_log: >- Joins a lightsheetZarrPyramid and its lightsheetZarrLevel children @@ -145,6 +175,7 @@ functions: - name: viewCommand matlab_path: "+ndi/+fun/+doc/+lightsheet/viewCommand.m" matlab_last_sync_hash: "5ab84f3" + matlab_branch: "claude/lightsheet-zarr-ndi-viewer-djp5vk" status: porting_deferred decision_log: >- Pure command builder that ndi.gui.app.LightsheetZarrManager calls @@ -160,6 +191,7 @@ functions: - name: view matlab_path: "+ndi/+fun/+doc/+lightsheet/view.m" matlab_last_sync_hash: "a7ba07a" + matlab_branch: "claude/lightsheet-zarr-ndi-viewer-djp5vk" status: porting_deferred decision_log: >- MATLAB caller that pairs viewCommand + SYSTEM: builds the diff --git a/src/ndi/gui/app/lightsheetZarr/ndi_matlab_python_bridge.yaml b/src/ndi/gui/app/lightsheetZarr/ndi_matlab_python_bridge.yaml index 764100e6..849e37ca 100644 --- a/src/ndi/gui/app/lightsheetZarr/ndi_matlab_python_bridge.yaml +++ b/src/ndi/gui/app/lightsheetZarr/ndi_matlab_python_bridge.yaml @@ -36,6 +36,7 @@ functions: - name: LightsheetZarrManager matlab_path: "+ndi/+gui/+app/LightsheetZarrManager.m" matlab_last_sync_hash: "60a79cf0" + matlab_branch: "claude/lightsheet-zarr-ndi-viewer-djp5vk" status: matlab_only decision_log: >- matlab_only: napari is Python-only, so the manager class stays From 87750993b08dac4e65a0b6006852219bf84f8440 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 12 Oct 2026 00:33:01 +0000 Subject: [PATCH 5/5] Document matlab_branch in the bridge reference and porting guide Section 8 of docs/developer_notes/ndi_matlab_python_bridge.yaml defines when an entry may name an NDI-matlab branch and how the checks hold it to account; the field table and the porting guide point there. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01B22A1gLVKa1E3uQm3tv4t8 --- docs/developer_notes/PYTHON_PORTING_GUIDE.md | 2 + .../ndi_matlab_python_bridge.yaml | 57 +++++++++++++++++++ 2 files changed, 59 insertions(+) diff --git a/docs/developer_notes/PYTHON_PORTING_GUIDE.md b/docs/developer_notes/PYTHON_PORTING_GUIDE.md index 2b36e99b..01ea8e8b 100644 --- a/docs/developer_notes/PYTHON_PORTING_GUIDE.md +++ b/docs/developer_notes/PYTHON_PORTING_GUIDE.md @@ -31,6 +31,8 @@ To port or update a function, agents must follow these steps: **It must be a commit, never a blob.** `git hash-object ` returns the hash *of the file's contents*, which looks equally plausible in the YAML and is useless here: the freshness check works by walking history (`git log ..HEAD -- `), and a blob is not a point in history to walk from. 35 entries carried blob hashes before this was written down, because "the hash of the MATLAB file" reads naturally as either one. A test now rejects anything that is not a commit. Either kind of commit is acceptable: the file'''s own last-touching commit (what the command above gives), or a repo-wide commit such as NDI-matlab `HEAD` when you examined a batch of files together. Both are points in history, so both let the check ask "has this file moved since?" + + If the MATLAB file is on an NDI-matlab branch that has not merged into main yet, record a commit on that branch and add `matlab_branch: ` to the entry. The checks then verify the entry against that branch and ask for the field to be removed once it merges. Section 8 of `docs/developer_notes/ndi_matlab_python_bridge.yaml` defines when this applies; do not use it for anything else. 4. **Implement:** Write the Python code to satisfy the `input_arguments` and `output_arguments` defined in the YAML. If you are NOT porting it, the entry needs a `status` and a `decision_log` instead. The five permitted values, what each claims, and how to choose between them are defined in section 6 of `docs/developer_notes/ndi_matlab_python_bridge.yaml` — that is the normative list, and this guide deliberately does not repeat it. diff --git a/docs/developer_notes/ndi_matlab_python_bridge.yaml b/docs/developer_notes/ndi_matlab_python_bridge.yaml index 943d790c..82aa8d65 100644 --- a/docs/developer_notes/ndi_matlab_python_bridge.yaml +++ b/docs/developer_notes/ndi_matlab_python_bridge.yaml @@ -105,6 +105,9 @@ project_metadata: # | commit, or a repo-wide commit for a batch sync. Both are # | points in history. NEVER a blob hash (git hash-object): # | enforced by tests/test_matlab_bridge_hashes.py. +# matlab_branch | OPTIONAL. The NDI-matlab branch the MATLAB file lives on +# | when it has not merged into NDI-matlab main yet. Only +# | for that case, and removed once it merges. Section 8. # status | One of the five values in section 6 below, or ABSENT. # | Absent + a python_path is the normal case and means # | ported. Enforced by tests/test_matlab_bridge_status.py. @@ -335,3 +338,57 @@ project_metadata: # drifts, so a fix must delist in the same PR. Start with the 18 whose MATLAB # change touched a signature or an arguments block; that is where a stale hash # is most likely hiding a real defect. + +# ============================================================================= +# 8. MATLAB ON A BRANCH: a counterpart that has not reached NDI-matlab main +# ============================================================================= +# Some work lands here before its MATLAB half reaches NDI-matlab main. The +# lightsheet OME-Zarr feature is the case that forced this: the napari viewer, +# its two document types and the bridge entries for the MATLAB writer merged +# into NDI-python while VH-Lab/NDI-matlab claude/lightsheet-zarr-ndi-viewer-djp5vk +# was still open. CI checks out NDI-matlab main (or a branch named like this +# repo's), so every such entry read as a stale path -- "names a MATLAB file +# that does not exist" -- and the bridge job stayed red on main. Neither +# existing escape fits: `matlab_path: "N/A"` / `retired` say the MATLAB file is +# gone or never was, which is false, and would have to be undone by hand the +# day the branch merges. +# +# SO THE ENTRY SAYS WHERE THE FILE IS: +# +# - name: makePyramid +# matlab_path: "+ndi/+fun/+doc/+lightsheet/makePyramid.m" +# matlab_last_sync_hash: "5c9e8a83" # a commit ON that branch +# matlab_branch: "claude/lightsheet-zarr-ndi-viewer-djp5vk" +# status: porting_deferred +# +# Everything else about the entry -- status, decision_log, python_path -- means +# what it always does. An ndi_common file in the same position goes in +# MATLAB_BRANCH_PENDING in tests/test_ndi_common_matlab_sync.py, mapped to its +# branch. +# +# IT IS A CLAIM, AND IT IS CHECKED -- otherwise it would be a way to switch the +# stale-path guard off for good. Read from the branch with git, so it needs +# NDI-matlab's remote branches (fetch-depth: 0, which the bridge job has): +# * the branch must exist and the file must be on it +# (TestBranchPendingEntries in tests/test_matlab_bridge_completeness.py); +# an ndi_common file must also match its copy on the branch byte-for-byte +# (test_branch_pending_files_match_their_branch); +# * DRIFT STILL APPLIES (section 7), measured against the branch tip instead +# of HEAD: a commit on the branch touching the file after the recorded +# hash fails CI exactly as it would on main; +# * ONCE THE BRANCH MERGES INTO NDI-matlab main, the marker must go: the +# check fails, asking for matlab_branch (or the MATLAB_BRANCH_PENDING line) +# to be removed. A branch that has vanished fails the same way -- it was +# usually merged and deleted. Removing the marker is then all it takes: +# the file is on main and the ordinary checks take over. +# +# "Merged" means merged into NDI-matlab main, not into whatever is checked +# out. When CI checks out the pending branch itself (an NDI-python branch of +# the same name), the file is simply in the tree and is checked as usual. +# +# DO NOT USE IT FOR A FILE THAT IS ON MAIN, or to record an entry against a +# branch because main has moved and the drift is inconvenient. It answers one +# question -- "where is this MATLAB file, if not on main?" -- and nothing else. +# +# The checks are in tests/_matlab_branch.py and are themselves tested, against +# a throwaway git repo, in tests/test_matlab_branch_pending.py.