fix(github): unambiguous issue cache key, no search-qualifier injection, rate-limit vs scope (#956) (#1089)

* fix(github): unambiguous issue cache key, no search-qualifier injection, rate-limit vs scope (#956)

Three defects in the GitHub Issues integration:

1. Cache-key ambiguity — the browse cache keyed on
   f"{repo}|{page}|{per_page}|{search}|{label}|{user_id}", so a '|' typed
   into the search box shifted into the label field and served one filter's
   results for another (wrong data, no error). The key is now a native tuple,
   unambiguous by construction; invalidation matches components instead of
   string prefix/suffix, so a search term spelling the repo name can't be swept.

2. Search-qualifier injection — the user's search string was joined verbatim
   alongside repo:/is: qualifiers, so a search of `repo:other/thing` reached
   repositories outside the connected one; in a hosted deployment that makes
   the operator's PAT enumerable through a text field. Free text is now quoted
   per word (literal to GitHub, AND-of-terms semantics preserved); a term with
   nothing searchable left falls back to the plain list endpoint.

3. Misreported 403 — every 403 was reported as "missing issues:read scope",
   sending users to regenerate a PAT that was never the problem. 403/429 are
   now classified by Retry-After / X-RateLimit-Remaining / body message into a
   new RateLimitedError (-> HTTP 429, ErrorCodes.RATE_LIMITED, distinct text)
   vs. a genuine InsufficientScopeError (-> 403).

Also collapses get_issues' inline error chain into the existing shared
_map_github_error, deleting ~15 lines of duplicated mapping.

Closes #956

* docs: record the #956 GitHub-issues invariants in CLAUDE.md

* test: update tenant-isolation cache assertions to the tuple key (#956)

tests/ui/test_credential_tenant_isolation.py asserted on the old
'|'-joined string key ('acme/app|1|25|||1', k.endswith("|1")). Same
invariants, expressed against the tuple: user_id is k[-1].

Author: frankbria

CLAUDE.md

@@ -38,7 +38,7 @@ If you are an agent working in this repo: **do not improvise architecture**. Fol
 
 **Phase 5.5 is complete** — GitHub Issues import. Repo connection via PAT (#563) is **complete**: Settings → **Integrations** tab connects a GitHub repo with a Personal Access Token. Backend `POST/DELETE/GET /api/v2/integrations/github/{connect,disconnect,status}` (`ui/routers/github_integrations_v2.py`). Validation is headless in `core/github_connect_service.py` (httpx; verifies token, repo visibility, and issues-read access; typed errors → 400/404/403 — a bad PAT is 400 `UPSTREAM_AUTH_FAILED`, never 401, so the web UI doesn't treat it as session expiry, #734). The PAT is stored machine-wide via `CredentialManager` (`CredentialProvider.GIT_GITHUB`, the #555 pattern) and **never returned in any response**; non-secret repo metadata persists per-workspace in `.codeframe/github_integration.json` (`core/github_integration_config.py`). Frontend: `GitHubIntegrationCard` + `integrationsApi`.
 
-Issue **browse** (#564) is **complete**: `GET /api/v2/integrations/github/issues?page&per_page&search&label` on the same router lists the connected repo's **open** issues (PRs filtered out) — repo from `.codeframe/github_integration.json`, PAT from `CredentialManager`, **409** when not connected. Headless fetch in `core/github_issues_service.py` (`list_issues`): plain `/repos/{o}/{r}/issues` by default, routes to `/search/issues` for free-text search, `labels=` filter, `Link`-header pagination, 60s in-process TTL cache, typed errors → 502/403/502 (a rejected stored PAT is 502 `UPSTREAM_AUTH_FAILED`, never 401 — #734; `pr_v2.py` likewise remaps upstream GitHub 401s to 502 via `_github_error_http`). Frontend: `GitHubIssueImportModal` (paginated list, debounced search, label filter, multi-select that persists across pages, select-all-on-page, Import-Selected gated on ≥1) + `integrationsApi.getIssues`; an **Import from GitHub** button on `/tasks` (`TaskBoardView`) shown only when connected.
+Issue **browse** (#564) is **complete**: `GET /api/v2/integrations/github/issues?page&per_page&search&label` on the same router lists the connected repo's **open** issues (PRs filtered out) — repo from `.codeframe/github_integration.json`, PAT from `CredentialManager`, **409** when not connected. Headless fetch in `core/github_issues_service.py` (`list_issues`): plain `/repos/{o}/{r}/issues` by default, routes to `/search/issues` for free-text search, `labels=` filter, `Link`-header pagination, 60s in-process TTL cache, typed errors → 502/403/429/502 (a rejected stored PAT is 502 `UPSTREAM_AUTH_FAILED`, never 401 — #734; `pr_v2.py` likewise remaps upstream GitHub 401s to 502 via `_github_error_http`). Three #956 invariants hold here: the cache key is a **tuple** `(repo, page, per_page, search, label, user_id)` — never a `|`-joined string, which let a `|` in the search text collide with the label field; free-text search is quoted **per word** by `_sanitize_search` so it cannot inject `repo:`/`is:` qualifiers and escape the connected repo; and a 403/429 that is really throttling (`Retry-After` / `X-RateLimit-Remaining: 0` / a body message naming the limit) raises `RateLimitedError` → **429** `RATE_LIMITED` instead of being misreported as a missing `issues:read` scope. Frontend: `GitHubIssueImportModal` (paginated list, debounced search, label filter, multi-select that persists across pages, select-all-on-page, Import-Selected gated on ≥1) + `integrationsApi.getIssues`; an **Import from GitHub** button on `/tasks` (`TaskBoardView`) shown only when connected.
 
 Issue **import + traceability** (#565) is **complete**: `POST /api/v2/integrations/github/import` (same router) turns selected issues into tasks — title verbatim, body as description (+ a best-effort `**Labels:**` footer), linked via `github_issue_number` + `external_url`; PRs are rejected (`NotAnIssueError`→422), missing issues 404, fetch failures 502, malformed saved repo 409. Import is two-phase (fetch+dedupe all, then create) with rollback on a mid-create DB error; dedup is keyed on the full issue URL and backed by a `UNIQUE(workspace_id, external_url)` index (atomic across concurrent imports). Issue ops live in `core/github_issues_service.py` (`get_issue`, `close_issue`). **Auto-close**: marking an opted-in imported task DONE closes the linked issue — fired from core `tasks.update_status` so the web UI, CLI, and agent/batch paths all trigger it; the close targets the task's *source* repo parsed from `external_url` (not the live connection) and runs off the caller's path (event loop in the server, non-daemon thread in CLI). `TaskResponse` exposes the three traceability fields; `PATCH /api/v2/tasks/{id}` accepts `auto_close_github_issue` (persist-first + rollback-on-rejected-transition, with late opt-in on already-DONE tasks). Frontend: `GitHubIssueBadge`, import wiring in `TaskBoardView` (progress, in-modal error, summary banner), badge + auto-close checkbox in `TaskDetailModal`, `integrationsApi.importIssues` + `tasksApi.updateGitHubSettings`. **Known limitation**: auto-close uses the single machine-wide GIT_GITHUB PAT, so closing an older imported repo's issue after reconnecting to a different repo may fail if that PAT lacks access.
 

codeframe/core/github_issues_service.py

@@ -55,6 +55,16 @@ class IssueNotFoundError(Exception):
     callers map it to a 4xx rather than a 502.
     """
 
+
+class RateLimitedError(GitHubConnectError):
+    """GitHub throttled us — primary or secondary rate limit (#956).
+
+    GitHub answers a rate-limited request with 403 (sometimes 429), the same
+    status it uses for a token that lacks a scope. Reporting both as "missing
+    issues:read scope" sent users off to regenerate a PAT that was never the
+    problem, so the two are distinguished by headers/body here.
+    """
+
 # Parse the ``page=N`` query param out of a Link header's rel="last" URL.
 _LAST_PAGE_RE = re.compile(r'[?&]page=(\d+)[^>]*>;\s*rel="last"')
 
@@ -87,14 +97,53 @@ def _simplify(raw: dict) -> GitHubIssue:
     }
 
 
-def _raise_for_status(status_code: int, *, context: str) -> None:
-    """Map a GitHub HTTP status to a typed error. 2xx/410 are handled by callers."""
+def _rate_limit_retry_after(resp: httpx.Response) -> Optional[str]:
+    """Return the retry hint if ``resp`` is a rate-limit rejection, else ``None``.
+
+    GitHub signals throttling three ways, any one of which is sufficient: a
+    ``Retry-After`` header (secondary limit), an exhausted
+    ``X-RateLimit-Remaining``, or a body message naming the limit. The empty
+    string means "throttled, no hint" — distinct from ``None`` (not throttled).
+    Only ``Retry-After`` carries a delay; ``X-RateLimit-Reset`` is an absolute
+    unix timestamp and would read as a nonsense wait if echoed as seconds.
+    """
+    retry_after = resp.headers.get("retry-after")
+    if retry_after:
+        return retry_after
+    if resp.headers.get("x-ratelimit-remaining") == "0":
+        return ""
+    try:
+        body = resp.json()
+        message = str(body.get("message", "")) if isinstance(body, dict) else ""
+    except Exception:
+        message = ""
+    if "rate limit" in message.lower():
+        return ""
+    return None
+
+
+def _raise_403(resp: httpx.Response, scope_message: str) -> None:
+    """Raise the right error for a 403/429: throttling vs. a genuine scope gap."""
+    retry_after = _rate_limit_retry_after(resp)
+    if retry_after is None and resp.status_code != 429:
+        raise InsufficientScopeError(scope_message)
+    hint = f" Retry after {retry_after}s." if retry_after else ""
+    raise RateLimitedError(
+        "GitHub rate limit exceeded; the request was throttled, "
+        f"not rejected for missing permissions.{hint}"
+    )
+
+
+def _raise_for_status(resp: httpx.Response, *, context: str) -> None:
+    """Map a GitHub response to a typed error. 2xx/410 are handled by callers."""
+    status_code = resp.status_code
     if status_code == 401:
         raise InvalidTokenError("Invalid GitHub token.")
-    if status_code == 403:
-        raise InsufficientScopeError(
+    if status_code in (403, 429):
+        _raise_403(
+            resp,
             "Token cannot read issues for this repository "
-            "(missing issues:read scope)."
+            "(missing issues:read scope).",
         )
     if status_code >= 400:
         raise GitHubConnectError(
@@ -154,7 +203,8 @@ async def list_issues(
     Raises:
         ValueError: if ``repo`` is not a valid ``owner/repo`` string.
         InvalidTokenError: GitHub returned 401.
-        InsufficientScopeError: the token cannot read issues (403).
+        InsufficientScopeError: the token cannot read issues (403, not throttled).
+        RateLimitedError: GitHub throttled the request (403/429).
         GitHubConnectError: any other non-success response or network error.
     """
     owner, name = parse_repo(repo)
@@ -164,9 +214,10 @@ async def list_issues(
         client = httpx.AsyncClient(timeout=_TIMEOUT)
     try:
         headers = _headers(pat)
-        if search.strip():
+        term = _sanitize_search(search)
+        if term:
             return await _search_issues(
-                client, headers, owner, name, page, per_page, search, label
+                client, headers, owner, name, page, per_page, term, label
             )
         return await _list_issues(
             client, headers, owner, name, page, per_page, label
@@ -205,7 +256,7 @@ async def _list_issues(
     # 410 Gone == issues disabled on the repo: nothing to import, not an error.
     if resp.status_code == 410:
         return [], 0
-    _raise_for_status(resp.status_code, context="issues list")
+    _raise_for_status(resp, context="issues list")
 
     raw_items = resp.json()
     if not isinstance(raw_items, list):
@@ -255,7 +306,8 @@ async def get_issue(
     Raises:
         ValueError: if ``repo`` is not a valid ``owner/repo`` string.
         InvalidTokenError: GitHub returned 401.
-        InsufficientScopeError: the token cannot read issues (403).
+        InsufficientScopeError: the token cannot read issues (403, not throttled).
+        RateLimitedError: GitHub throttled the request (403/429).
         GitHubConnectError: any other non-success response or network error.
     """
     owner, name = parse_repo(repo)
@@ -288,10 +340,8 @@ async def get_issue(
                 raise GitHubConnectError("Could not reach GitHub. Try again later.")
             if repo_resp.status_code == 401:
                 raise InvalidTokenError("Invalid GitHub token.")
-            if repo_resp.status_code == 403:
-                raise InsufficientScopeError(
-                    "Token lacks access to this repository."
-                )
+            if repo_resp.status_code in (403, 429):
+                _raise_403(repo_resp, "Token lacks access to this repository.")
             if repo_resp.status_code == 404:
                 raise GitHubConnectError(
                     f"Repository '{repo}' is no longer accessible."
@@ -311,7 +361,7 @@ async def get_issue(
                     f"GitHub repo check returned status {repo_resp.status_code}."
                 )
             raise IssueNotFoundError(f"Issue #{number} was not found in '{repo}'.")
-        _raise_for_status(resp.status_code, context="get issue")
+        _raise_for_status(resp, context="get issue")
 
         raw = resp.json()
         if not isinstance(raw, dict):
@@ -370,7 +420,8 @@ async def close_issue(
     Raises:
         ValueError: if ``repo`` is not a valid ``owner/repo`` string.
         InvalidTokenError: GitHub returned 401.
-        InsufficientScopeError: the token cannot write issues (403).
+        InsufficientScopeError: the token cannot write issues (403, not throttled).
+        RateLimitedError: GitHub throttled the request (403/429).
         GitHubConnectError: any other non-success response or network error.
     """
     owner, name = parse_repo(repo)
@@ -409,7 +460,7 @@ async def close_issue(
             logger.warning("GitHub close issue failed: %s", type(exc).__name__)
             raise GitHubConnectError("Could not reach GitHub. Try again later.")
 
-        _raise_for_status(resp.status_code, context="close issue")
+        _raise_for_status(resp, context="close issue")
         # A redirect (3xx) — e.g. a moved/renamed/transferred repo — means the
         # PATCH was NOT applied (httpx does not follow redirects by default), so
         # the issue is still open. Treat it as a failure rather than reporting a
@@ -425,18 +476,43 @@ async def close_issue(
             await client.aclose()
 
 
+def _sanitize_search(search: str) -> str:
+    """Quote each free-text word so none can act as a qualifier (#956).
+
+    The term is joined into a ``q`` alongside ``repo:``/``is:`` qualifiers, so
+    raw text lets a user smuggle in their own: a search of ``repo:other/thing``
+    used to reach repositories outside the connected one, making the operator's
+    PAT enumerable through a text field in a hosted deployment. GitHub treats a
+    double-quoted string as literal, so quoting neutralises every qualifier.
+
+    Quoting *per word* rather than the whole string keeps the existing AND-of-
+    terms behaviour — one big phrase would silently turn "login bug" into an
+    exact-phrase search and drop results users used to get.
+
+    Embedded quotes are *removed*, not escaped — escaping semantics inside
+    GitHub's query language are version-dependent, and dropping them is the one
+    behaviour that cannot be talked into opening a second phrase.
+
+    Returns ``""`` when nothing searchable survives, so the caller falls back to
+    the plain list endpoint rather than sending an empty phrase.
+    """
+    words = search.replace('"', " ").split()
+    return " ".join(f'"{w}"' for w in words)
+
+
 async def _search_issues(
     client: httpx.AsyncClient,
     headers: dict[str, str],
     owner: str,
     name: str,
     page: int,
     per_page: int,
-    search: str,
+    term: str,
     label: str,
 ) -> tuple[list[GitHubIssue], int]:
+    """Search issues. ``term`` must already be ``_sanitize_search``-ed."""
     qualifiers = [
-        search.strip(),
+        term,
         f"repo:{owner}/{name}",
         "is:issue",
         "is:open",
@@ -454,7 +530,7 @@ async def _search_issues(
         logger.warning("GitHub issues search failed: %s", type(exc).__name__)
         raise GitHubConnectError("Could not reach GitHub. Try again later.")
 
-    _raise_for_status(resp.status_code, context="issues search")
+    _raise_for_status(resp, context="issues search")
 
     data = resp.json()
     if not isinstance(data, dict):

codeframe/ui/response_models.py

@@ -166,6 +166,10 @@ class ErrorCodes:
     # Distinct from UNAUTHORIZED so it is never carried on a 401 — the web UI
     # treats any 401 as CodeFRAME session expiry and logs the user out (#734).
     UPSTREAM_AUTH_FAILED = "UPSTREAM_AUTH_FAILED"
+    # An upstream service throttled us (GitHub rate limit). Distinct from
+    # FORBIDDEN so a transient 429 is never read as "your token lacks a
+    # scope" — that sent users to regenerate a working PAT (#956).
+    RATE_LIMITED = "RATE_LIMITED"
 
     # Server errors (5xx)
     INTERNAL_ERROR = "INTERNAL_ERROR"

codeframe/ui/routers/github_integrations_v2.py

@@ -40,6 +40,7 @@
     _TIMEOUT,
     IssueNotFoundError,
     NotAnIssueError,
+    RateLimitedError,
     get_issue,
     list_issues,
 )
@@ -160,9 +161,15 @@ class ImportResponse(BaseModel):
 # within the same server process. Bounded to
 # _ISSUE_CACHE_MAX_SIZE entries and swept on every set so search text (an
 # unbounded key space) can't accumulate stale payloads forever (#761).
+#
+# The key is a TUPLE, not a delimiter-joined string: joining on '|' meant a '|'
+# typed into the search box shifted into the label field and served one filter's
+# results for another — wrong data, no error (#956). A tuple is unambiguous by
+# construction and needs no encoding.
 _ISSUE_CACHE_TTL_SECONDS = 60.0
 _ISSUE_CACHE_MAX_SIZE = 256
-_ISSUE_CACHE: dict[str, tuple[float, Any]] = {}
+_IssueCacheKey = tuple  # (repo, page, per_page, search, label, user_id)
+_ISSUE_CACHE: dict[_IssueCacheKey, tuple[float, Any]] = {}
 
 
 def _evict_issue_cache() -> None:
@@ -178,7 +185,7 @@ def _evict_issue_cache() -> None:
             del _ISSUE_CACHE[key]
 
 
-def _issue_cache_get(key: str) -> Optional[Any]:
+def _issue_cache_get(key: _IssueCacheKey) -> Optional[Any]:
     entry = _ISSUE_CACHE.get(key)
     if entry is None:
         return None
@@ -189,7 +196,7 @@ def _issue_cache_get(key: str) -> Optional[Any]:
     return payload
 
 
-def _issue_cache_set(key: str, payload: Any) -> None:
+def _issue_cache_set(key: _IssueCacheKey, payload: Any) -> None:
     _ISSUE_CACHE[key] = (time.monotonic() + _ISSUE_CACHE_TTL_SECONDS, payload)
     _evict_issue_cache()
 
@@ -199,13 +206,12 @@ def _issue_cache_invalidate(repo: str, user_id: Optional[int]) -> None:
 
     Called after an import so reopening the browse modal doesn't keep offering
     just-imported issues as selectable (they would now be skipped as dupes).
-    Keys are ``repo|page|per_page|search|label|user_id``; only drop entries
-    belonging to the calling user so one tenant's import does not wipe another's
-    cache for the same repo (#790).
+    Keys are ``(repo, page, per_page, search, label, user_id)``; only drop
+    entries belonging to the calling user so one tenant's import does not wipe
+    another's cache for the same repo (#790). Matching is component-wise, so a
+    search term that happens to spell the repo name can't be swept (#956).
     """
-    prefix = f"{repo}|"
-    suffix = f"|{user_id}"
-    for key in [k for k in _ISSUE_CACHE if k.startswith(prefix) and k.endswith(suffix)]:
+    for key in [k for k in _ISSUE_CACHE if k[0] == repo and k[-1] == user_id]:
         _ISSUE_CACHE.pop(key, None)
 
 
@@ -410,9 +416,9 @@ async def get_issues(
     per_page = max(1, min(per_page, 100))
     repo = cfg["repo"]
 
-    # The user component keeps the repo|-prefixed invalidation in
-    # _issue_cache_invalidate working while separating tenants (#790).
-    cache_key = f"{repo}|{page}|{per_page}|{search}|{label}|{auth.get('user_id')}"
+    # Tuple key: repo first and user last so _issue_cache_invalidate can match
+    # both component-wise while keeping tenants separated (#790, #956).
+    cache_key = (repo, page, per_page, search, label, auth.get("user_id"))
     cached = _issue_cache_get(cache_key)
     if cached is not None:
         return cached
@@ -432,23 +438,10 @@ async def get_issues(
             status_code=409,
             detail=api_error(str(e), ErrorCodes.CONFLICT),
         )
-    except InvalidTokenError as e:
-        # 502, never 401: the stored PAT was rejected upstream — the caller's
-        # CodeFRAME session is fine (#734).
-        raise HTTPException(
-            status_code=502,
-            detail=api_error(str(e), ErrorCodes.UPSTREAM_AUTH_FAILED),
-        )
-    except InsufficientScopeError as e:
-        raise HTTPException(
-            status_code=403,
-            detail=api_error(str(e), ErrorCodes.VALIDATION_ERROR),
-        )
     except GitHubConnectError as e:
-        raise HTTPException(
-            status_code=502,
-            detail=api_error(str(e), ErrorCodes.EXECUTION_FAILED),
-        )
+        # Every typed GitHub error subclasses this; _map_github_error is the
+        # single mapping shared with the import path.
+        raise _map_github_error(e)
 
     response = GitHubIssuesResponse(
         issues=[GitHubIssueItem(**issue) for issue in issues],
@@ -493,6 +486,12 @@ def _map_github_error(e: Exception) -> HTTPException:
         return HTTPException(
             status_code=403, detail=api_error(str(e), ErrorCodes.VALIDATION_ERROR)
         )
+    if isinstance(e, RateLimitedError):
+        # 429, not 403: throttling is transient and retryable — reporting it as
+        # a scope gap sends users to regenerate a PAT that was fine (#956).
+        return HTTPException(
+            status_code=429, detail=api_error(str(e), ErrorCodes.RATE_LIMITED)
+        )
     return HTTPException(
         status_code=502, detail=api_error(str(e), ErrorCodes.EXECUTION_FAILED)
     )