Add ndi.cloud.api.feedback; fix bridge CI for MATLAB files on an unmerged branch

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 <file>` 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 <hash>..HEAD -- <path>`), 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: <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.

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.

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"]

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)
 

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),
+    )