fix: address project context PR review

mcp_servers/project_context/providers/issue.py

@@ -1,68 +1,8 @@
-"""Provider boundary owned with get_project_issue_context.
+"""Read-only Gitea adapter for ``get_project_issue_context``.
 
-Read-only adapter that maps a Gitea issue/pull-request onto the neutral
-``IssueContextOutput`` schema declared in ``tools/issue_context.py``. Nothing
-here changes the shared runtime, the registry, or the tool schema — this
-module only fills in ``build_provider`` so ``runtime.py``'s
-``PROVIDER_FACTORIES["get_project_issue_context"]`` resolves to a real,
-read-only Gitea client instead of :class:`UnconfiguredIssueProvider`.
-
-Design notes (documented here so a reviewer does not have to guess):
-
-* Credentials (``GITEA_TOKEN``) and the base URL are read INSIDE
-  ``build_provider``/``_resolve_target`` only — never at import time — because
-  ``build_provider`` is only ever invoked by
-  ``ProjectProviderResolver.resolve`` (see ``runtime.py``), which
-  ``server.py::dispatch`` calls strictly AFTER ``policy.decide`` returns
-  ``True``. Reading env vars at module scope would read them before the
-  policy check ever runs.
-* The provider is bound to exactly one project/repository at construction
-  time (``identity.project`` -> ``PROJECT_CONTEXT_REPO_MAP``). The
-  ``project_id`` argument received per call is only used to echo it back in
-  the response and as a defensive equality check — it is never used to pick
-  which repository to query. This means a caller can never redirect the
-  provider to an arbitrary repository by tampering with ``project_id``.
-* ``revision`` pins to the issue's own ``updated_at`` timestamp. Gitea issues
-  (unlike commits/PRs) have no natural commit SHA of their own, so the most
-  meaningful, verifiable "version marker" Gitea offers for an issue is its
-  last-modified time: given the same URL and the same ``revision`` string, a
-  reviewer can confirm whether the issue has changed since this was fetched.
-* ``related`` items are derived from BARE ``#<number>`` cross-references
-  found in the issue body — never from a ``#<number>`` that is only the
-  label text of an explicit Markdown link (``[other-repo PR #4](http://.../
-  other-repo/pulls/4)``). A real issue body regularly links to a pull
-  request or an issue in a DIFFERENT repository this way; re-guessing that
-  as "issue #4 in THIS repo" would silently point at the wrong resource
-  entirely, which is worse than omitting it — the author already gave the
-  correct URL right there, so extraction strips whole ``[label](url)``
-  spans, then any remaining bare URLs, before scanning for mentions. Titles
-  for the mentions that DO survive are NOT fetched with an extra API call
-  (kept deliberately simple and bounded) — each related item's title is a
-  generic, honest placeholder ("Referenced item #<n>"), and its ``url``
-  always resolves to a real, openable page in this same repository. This
-  is a known, documented simplification, not a bug.
-* ``acceptance_criteria`` looks for the issue's own "Acceptance Criteria"
-  Markdown heading (``#`` through ``######``, case-insensitive) and only
-  collects checklist items (``- [ ]``/``- [x]``) from inside that section,
-  up to the next heading of equal-or-shallower depth. This matters because
-  a real work-item body commonly has a SEPARATE "Definition of Done"
-  section that also uses checklist syntax — without scoping to the
-  matching heading, those unrelated items would be silently folded into
-  "acceptance criteria" and misrepresent the issue. When no such heading
-  exists at all (ad-hoc issues with no fixed template), this falls back to
-  scanning the whole body, so those issues still get a best-effort result
-  instead of an always-empty list.
-* ``truncated``/``returned``/``remaining``/``next_cursor`` describe pagination
-  over the ``related`` list only. ``description`` has its own independent,
-  generous hard safety cap (so the response stays bounded even for a
-  pathological issue body) and is never silently cut without a visible
-  notice appended.
-* The regexes that derive ``acceptance_criteria``/``related`` never scan more
-  than ``_MAX_SCAN_CHARS`` of the raw body, and always run on a copy with
-  ``http(s)://`` links stripped first — otherwise a `#42` inside a URL
-  fragment (e.g. a doc anchor) would be mis-detected as a cross-reference to
-  issue #42, and a pathologically large body would cost unbounded regex work
-  on every call regardless of the display cap on ``description``.
+Policy runs before ``build_provider``. Target and credential resolution stay
+separate so the pilot service account can later be replaced by on-behalf-of
+credentials without changing the tool or provider contract.
 """
 
 from __future__ import annotations
@@ -97,7 +37,11 @@ _URL_PATTERN = re.compile(r"https?://\S+")
 _MARKDOWN_LINK_PATTERN = re.compile(r"\[[^\]]*\]\([^)]*\)")
 # ATX heading line, e.g. "# Acceptance Criteria" / "## Acceptance Criteria".
 _HEADING_PATTERN = re.compile(r"^(#{1,6})[ \t]+(.+?)\s*$", re.MULTILINE)
-_ACCEPTANCE_HEADING_NAMES = ("acceptance criteria",)
+_ACCEPTANCE_HEADING_NAMES = (
+    "acceptance criteria",
+    "tiêu chí hoàn thành",
+    "tiêu chí chấp nhận",
+)
 
 
 def _extract_heading_section(text: str, heading_names: tuple[str, ...]) -> str | None:
@@ -105,10 +49,11 @@ def _extract_heading_section(text: str, heading_names: tuple[str, ...]) -> str |
     matches one of ``heading_names``, up to the next heading of equal or
     shallower depth (or the end of ``text``). Returns ``None`` when no such
     heading exists, so the caller can fall back to the whole body."""
-    wanted = {name.strip().lower() for name in heading_names}
+    wanted = {name.strip().casefold() for name in heading_names}
     headings = list(_HEADING_PATTERN.finditer(text))
     for index, match in enumerate(headings):
-        if match.group(2).strip().lower() not in wanted:
+        heading = match.group(2).strip().rstrip("#").strip().casefold()
+        if heading not in wanted:
             continue
         level = len(match.group(1))
         end = len(text)
@@ -138,10 +83,17 @@ class _GiteaRepoTarget:
     base_url: str
     owner: str
     repo: str
-    token: str
     project_id: str
 
 
+class GiteaTargetResolver(Protocol):
+    def resolve(self, identity: IdentityContext) -> _GiteaRepoTarget: ...
+
+
+class GiteaCredentialResolver(Protocol):
+    def resolve(self, identity: IdentityContext, target: _GiteaRepoTarget) -> str: ...
+
+
 def _load_repo_map() -> dict[str, str]:
     raw = os.environ.get("PROJECT_CONTEXT_REPO_MAP", "").strip()
     if not raw:
@@ -159,53 +111,73 @@ def _load_repo_map() -> dict[str, str]:
     ):
         raise ProviderError(
             "UNAVAILABLE",
-            "PROJECT_CONTEXT_REPO_MAP must be a JSON object of project_id -> 'owner/repo'.",
+            "PROJECT_CONTEXT_REPO_MAP must map identity or project keys to 'owner/repo'.",
             retryable=False,
         )
     return parsed
 
 
-def _resolve_target(identity: IdentityContext) -> _GiteaRepoTarget:
-    """Read-only credential/target resolution. Only ever called AFTER the
-    runtime's policy has already granted ALLOW for this identity/tool."""
-    base_url = os.environ.get("GITEA_BASE_URL", "").strip().rstrip("/")
-    token = os.environ.get("GITEA_TOKEN", "").strip()
-    if not base_url or not token:
-        raise ProviderError(
-            "UNAVAILABLE",
-            "GITEA_BASE_URL/GITEA_TOKEN are not configured for this environment.",
-            retryable=False,
+@dataclass(frozen=True)
+class EnvironmentTargetResolver:
+    def resolve(self, identity: IdentityContext) -> _GiteaRepoTarget:
+        base_url = os.environ.get("GITEA_BASE_URL", "").strip().rstrip("/")
+        if not base_url:
+            raise ProviderError(
+                "UNAVAILABLE",
+                "GITEA_BASE_URL is not configured for this environment.",
+                retryable=False,
+            )
+        repo_map = _load_repo_map()
+        identity_key = f"{identity.org_unit}/{identity.customer}/{identity.project}"
+        slug = repo_map.get(identity_key) or repo_map.get(identity.project, "")
+        parts = slug.split("/")
+        if len(parts) != 2 or not all(parts):
+            raise ProviderError(
+                "UNAVAILABLE",
+                "This identity is not mapped to an approved Gitea repository.",
+                retryable=False,
+            )
+        owner, repo = parts
+        return _GiteaRepoTarget(
+            base_url=base_url,
+            owner=owner,
+            repo=repo,
+            project_id=identity.project,
         )
-    repo_map = _load_repo_map()
-    slug = repo_map.get(identity.project, "")
-    if not slug or "/" not in slug:
-        raise ProviderError(
-            "UNAVAILABLE",
-            "This project is not mapped to an approved Gitea repository.",
-            retryable=False,
-        )
-    owner, _, repo = slug.partition("/")
-    if not owner or not repo:
-        raise ProviderError(
-            "UNAVAILABLE",
-            "This project's Gitea repository mapping is malformed.",
-            retryable=False,
-        )
-    return _GiteaRepoTarget(
-        base_url=base_url, owner=owner, repo=repo, token=token, project_id=identity.project,
-    )
 
 
-def build_provider(identity: IdentityContext) -> IssueProvider:
-    """Replace only this factory when wiring the approved read-only issue adapter."""
-    return GiteaIssueProvider(_resolve_target(identity))
+@dataclass(frozen=True)
+class ServiceAccountCredentialResolver:
+    def resolve(self, identity: IdentityContext, target: _GiteaRepoTarget) -> str:
+        del identity, target
+        token = os.environ.get("GITEA_TOKEN", "").strip()
+        if not token:
+            raise ProviderError(
+                "UNAVAILABLE",
+                "GITEA_TOKEN is not configured for this environment.",
+                retryable=False,
+            )
+        return token
+
+
+def build_provider(
+    identity: IdentityContext,
+    *,
+    target_resolver: GiteaTargetResolver | None = None,
+    credential_resolver: GiteaCredentialResolver | None = None,
+) -> IssueProvider:
+    """Compose routing and credentials only after the policy has allowed the call."""
+    target = (target_resolver or EnvironmentTargetResolver()).resolve(identity)
+    token = (credential_resolver or ServiceAccountCredentialResolver()).resolve(identity, target)
+    return GiteaIssueProvider(target, token)
 
 
 class GiteaIssueProvider:
     """Read-only adapter mapping one Gitea issue/PR onto the neutral schema."""
 
-    def __init__(self, target: _GiteaRepoTarget) -> None:
+    def __init__(self, target: _GiteaRepoTarget, token: str) -> None:
         self._target = target
+        self._token = token
 
     def get_issue_context(
         self,
@@ -244,8 +216,11 @@ class GiteaIssueProvider:
         # cost, independently of `description`'s own display-only cap.
         scan_text = body[:_MAX_SCAN_CHARS]
         acceptance_section = _extract_heading_section(scan_text, _ACCEPTANCE_HEADING_NAMES)
+        acceptance_text = acceptance_section
+        if acceptance_text is None:
+            acceptance_text = "" if _HEADING_PATTERN.search(scan_text) else scan_text
         acceptance_criteria = tuple(
-            _CHECKLIST_PATTERN.findall(acceptance_section if acceptance_section is not None else scan_text)
+            _CHECKLIST_PATTERN.findall(acceptance_text)
         )
         related_all = self._extract_related(scan_text, issue_key)
 
@@ -345,7 +320,7 @@ class GiteaIssueProvider:
             f"{self._target.base_url}/api/v1/repos/{self._target.owner}/"
             f"{self._target.repo}/issues/{issue_key}"
         )
-        headers = {"Authorization": f"token {self._target.token}"}
+        headers = {"Authorization": f"token {self._token}"}
         try:
             response = requests.get(url, headers=headers, timeout=_REQUEST_TIMEOUT_SECONDS)
         except requests.exceptions.Timeout as exc: