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

[frankbria]

Closes #956.

Three defects in the GitHub Issues integration, each with a test that fails on main.

1. Cache-key ambiguity — wrong data, no error

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:

search='a|b' label='c'   -> acme/app|1|25|a|b|c|7
search='a'   label='b|c' -> acme/app|1|25|a|b|c|7   # same key

The key is now a native tuple — unambiguous by construction, no encoding needed. _issue_cache_invalidate matches components (k[0] == repo and k[-1] == user_id) rather than string prefix/suffix, so an entry for a different repo whose search text happens to spell the connected repo name is no longer swept.

2. Search text could introduce qualifiers

The user's search string was joined verbatim alongside the 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 (GitHub treats a quoted string as literal). Per word rather than one big phrase so login bug stays AND-of-terms instead of silently becoming an exact-phrase search:

'repo:other/secret' -> q = "repo:other/secret" repo:acme/app is:issue is:open
'login bug'         -> q = "login" "bug" repo:acme/app is:issue is:open
'a" repo:evil/x "b' -> q = "a" "repo:evil/x" "b" repo:acme/app is:issue is:open

Embedded quotes are removed rather than escaped — escaping semantics inside GitHub's query language are version-dependent; dropping them is the one behaviour that can't be talked into opening a second phrase. A term with nothing searchable left ("") returns "" and falls back to the plain list endpoint rather than sending an empty phrase.

3. A throttled 403 was reported as a missing scope

Every 403 raised InsufficientScopeError("... missing issues:read scope"), sending users off to regenerate a PAT that was never the problem.

403/429 are now classified by Retry-After / X-RateLimit-Remaining: 0 / a body message naming the limit:

case	before	after
secondary rate limit (Retry-After: 60)	403 "missing issues:read scope"	429 "GitHub rate limit exceeded… Retry after 60s."
primary rate limit (X-RateLimit-Remaining: 0)	403 "missing issues:read scope"	429 "GitHub rate limit exceeded…"
HTTP 429	502	429 "GitHub rate limit exceeded…"
genuine scope gap	403 "missing issues:read scope"	403 "missing issues:read scope" (unchanged)

New RateLimitedError(GitHubConnectError) + ErrorCodes.RATE_LIMITED. It subclasses the existing base, so every broad except GitHubConnectError handler (reconciliation, auto-close) keeps working. X-RateLimit-Reset is deliberately not echoed as a retry hint — it's an absolute unix timestamp and would read as a nonsense wait.

Incidental cleanup

get_issues had its own inline InvalidTokenError/InsufficientScopeError/GitHubConnectError chain duplicating _map_github_error. Since all typed errors subclass GitHubConnectError, that collapses to one except delegating to the shared mapper — ~15 lines deleted, and the new 429 case only had to be written once.

Verification

• 12 new tests (7 core, 5 router); all 11 relevant ones fail on main.
• uv run pytest tests/core/test_github_issues_service.py tests/ui/test_github_integrations_v2.py tests/core/test_github_connect_service.py tests/core/test_github_pagination_940.py tests/core/test_github_issue_reconciliation_1032.py tests/core/test_task_github_traceability.py → 159 passed
• uv run ruff check codeframe/ tests/ → clean
• Third-party review: codex review --base main → no findings.
• Demo run of all three acceptance criteria against the real code paths (mocked transport): output in the PR discussion below.

Known limitations

• Quoting per word means a user who wants an exact-phrase search can no longer get one by typing quotes — quotes are stripped. No UI ever offered phrase search, so this is not a regression in practice.
• The frontend needs no change: GitHubIssueImportModal renders error.detail, and normalizeErrorDetail surfaces the new rate-limit message text as-is. The axios interceptor only special-cases 401, so a 429 displays rather than redirecting.
• Rate-limit classification is heuristic (headers first, body message as fallback). A future GitHub 403 that names no limit and sets no headers would still read as a scope gap.

[frankbria]

Demo — acceptance-criteria evidence

Ran the real code paths (only the HTTP transport is mocked):

=== AC1: cache key is unambiguous ===
  OLD key search='a|b' label='c'   -> acme/app|1|25|a|b|c|7
  OLD key search='a'   label='b|c' -> acme/app|1|25|a|b|c|7
  OLD collide? True   <-- wrong data served
  NEW key search='a|b' label='c'   -> ('acme/app', 1, 25, 'a|b', 'c', 7)
  NEW key search='a'   label='b|c' -> ('acme/app', 1, 25, 'a', 'b|c', 7)
  NEW collide? False
  after invalidate(acme/app, user 7): own entry gone=True, other user kept=True, other repo kept=True

=== AC2: search text cannot introduce qualifiers ===
  user typed 'repo:other/secret'
    -> q = "repo:other/secret" repo:acme/app is:issue is:open
       scoped to connected repo: True
  user typed 'is:closed'
    -> q = "is:closed" repo:acme/app is:issue is:open
       scoped to connected repo: True
  user typed 'login bug'
    -> q = "login" "bug" repo:acme/app is:issue is:open
       scoped to connected repo: True
  user typed 'a" repo:evil/x "b'
    -> q = "a" "repo:evil/x" "b" repo:acme/app is:issue is:open
       scoped to connected repo: True
  OLD behaviour for 'repo:other/secret' would have been:
    -> q = repo:other/secret repo:acme/app is:issue is:open   <-- leaks outside the repo

=== AC3: rate-limit 403 distinguished from scope 403 ===
  secondary rate limit   -> RateLimitedError    HTTP 429  GitHub rate limit exceeded; the request was throttled, not rejected for missing permissions. Retry after 60s.
  primary rate limit     -> RateLimitedError    HTTP 429  GitHub rate limit exceeded; the request was throttled, not rejected for missing permissions.
  429                    -> RateLimitedError    HTTP 429  GitHub rate limit exceeded; the request was throttled, not rejected for missing permissions.
  genuine scope gap      -> InsufficientScope   HTTP 403  Token cannot read issues for this repository (missing issues:read scope).
  OLD behaviour: all four -> InsufficientScopeError / HTTP 403
                 'Token cannot read issues ... (missing issues:read scope).'

Done.

[github-actions]

GLM bug-hunting review in progress

• Gather PR diff + context (gh pr view / gh pr diff)
• Check existing comments to avoid repeats
• Read changed files + callers/callees in checked-out repo
• Confirm concrete defects against real code
• Post inline findings + summary

Scope: concrete defects only (logic errors, null/error paths, security, data loss, races, contract breaks). Not reviewing style/architecture/naming/coverage.

View job run

[frankbria]

Third-party review (post-PR)

codex review --base main on the branch diff:

The changes correctly distinguish GitHub rate limiting from permission failures and replace ambiguous cache/search handling without introducing clear regressions in the reviewed diff.

No findings. (opencode/GLM was skipped as primary — on this repo it has a history of mutating the working tree and timing out with zero output; codex review is the documented fallback.)

[github-actions]

Claude finished @frankbria's task in 4m 48s —— View job

---

GLM bug-hunting review — complete ✅

• Gather PR diff + context (gh pr view / gh pr diff)
• Check existing comments to avoid repeats
• Read changed files + callers/callees in checked-out repo
• Confirm concrete defects against real code
• Post inline findings + summary

✅ GLM review: no defects found.

Scope was concrete defects only (logic errors, null/error paths, security, data loss, races, contract breaks). What I checked and why each holds:

Area	Verdict	Notes
RateLimitedError hierarchy	✅	Subclasses GitHubConnectError; _map_github_error isinstance order is InvalidToken → InsufficientScope → RateLimited → catch-all GitHubConnect, all distinct siblings — no shadowing.
Throttle-vs-scope classification (_raise_403)	✅	None + non-429 → scope gap; otherwise RateLimitedError. A 429 with no rate-limit headers still routes to RateLimitedError (429 is always throttling). Correct.
_rate_limit_retry_after None vs ""	✅	"" (throttled, no hint) is falsy, so the "Retry after {x}s." branch is skipped — no "Retry after s.". Distinct from None (not throttled).
Broad handlers keep working	✅	_safe_close_issue (except Exception) and reconciliation is_closed (except Exception) both still swallow the new subclass; auto-close/recon paths unaffected.
_sanitize_search qualifier injection	✅	All " removed before split, so no word can contain a quote → no word can close its own "…". "repo:other/secret" becomes a literal phrase; repo:owner/name is always appended outside quotes, so the query stays repo-scoped. No PAT-enumeration path.
Cache key → tuple + component-wise invalidate	✅	Every inserted key is a 6-tuple, so k[0]/k[-1] indexing is safe (no empty-tuple risk). _evict_issue_cache/get/set are key-agnostic and work with tuples. user_id=None compares consistently.
_raise_for_status signature change	✅	Module-private; all 4 call sites (list/get/close/search) updated to pass resp. No external callers.
get_issue repo-probe 403/429	✅	Handled before the generic >=400/>=300 fallthroughs on both the probe and the issue fetch.

One theoretical non-defect I considered and discarded (below the precision bar): a trailing backslash in the search term (e.g. foo\) yields "foo\"; if GitHub honored \" as an in-phrase escape the query would be malformed → 422 → surfaced as a 502. GitHub's documented search syntax has no backslash escaping inside phrases (it's literal), the author explicitly disclosed the escaping consideration, and in neither parser interpretation does it re-open a repo: qualifier — so there's no security impact and no confirmed failure. Not posted.

[claude]

Code Review

Reviewed the diff directly (couldn't run the local test suite in this sandbox — verified statically by reading the full modified files and tracing call sites instead of relying on the PR's own reported test run).

Overall: solid fix, well-scoped to the three defects described, and the evidence in the PR body/demo comment matches what the code actually does.

Correctness — traced through each defect

• Cache key (tuple): _issue_cache_invalidate matches k[0] == repo and k[-1] == user_id, which only works because cache_key is built in exactly one place (get_issues) with repo first and user_id last. That's a soft coupling but it's documented in the comment at the call site, and the tuple approach genuinely eliminates the |-collision bug — confirmed by re-deriving the AC1 collision example by hand.
• Search qualifier injection: _sanitize_search quoting per word is correct — GitHub treats a double-quoted string as literal, and stripping (not escaping) embedded quotes means a user can't smuggle a second phrase open. Traced the list_issues → term = _sanitize_search(search) → if term: gate and confirmed the empty-after-sanitizing case (e.g. '""') correctly falls back to the plain list endpoint rather than sending a blank phrase to the search API — this is also a behavior improvement over the old search.strip() check, since previously a lone '"' would have been sent to GitHub as a malformed literal query.
• Rate-limit vs. scope: _raise_403 correctly forces RateLimitedError whenever status_code == 429 (even with no distinguishing headers/body), and falls back to InsufficientScopeError only when neither headers nor body indicate throttling on a plain 403. RateLimitedError(GitHubConnectError) preserves every existing broad except GitHubConnectError handler — confirmed both core/tasks.py (_safe_close_issue) and core/reconciliation.py catch bare Exception around GitHub calls, so this subclassing choice doesn't change their behavior either way.

Consistency

• _map_github_error (shared by both get_issues and import_issues) now handles RateLimitedError → 429/RATE_LIMITED, so the classification is consistent across the browse and import endpoints.
• Frontend confirmed to need no change as claimed: GitHubIssueImportModal renders error.detail generically, and the axios interceptor only special-cases 401 — a 429 will just display, not trigger a false session-expiry redirect.

Minor observations (non-blocking)

• _rate_limit_retry_after parses the JSON body on every 403/429 even when the headers already answered the question — negligible cost given this is a low-volume, user-triggered browse/import path, not worth the complexity of short-circuiting.
• The known limitation called out in the PR description (quoting removes the ability to do an exact-phrase search) is accurate and reasonable — no UI ever exposed phrase search, so nothing regresses in practice.
• _raise_403's reused scope_message param ("Token lacks access to this repository." for the repo-probe path) is only ever surfaced on the InsufficientScopeError branch, never blended into the rate-limit message — confirmed no mismatched wording can leak through.

Test coverage

New tests (TestSearchQualifierInjection, TestRateLimitVsScope, TestIssueCacheKeyAmbiguity, plus the router-level cache/invalidation updates) directly exercise each AC, including the adversarial cases (embedded quotes, multiword AND-semantics, repo name colliding with another entry's search text). This is good coverage for a defect-fix PR — no gaps stood out.

No blocking issues found.

[claude]

Code Review

Traced the diff against codeframe/core/github_issues_service.py and codeframe/ui/routers/github_integrations_v2.py directly (this sandbox has the PR already merged into the working tree, so I read the post-merge files, not just the patch).

Agrees with the prior review on this PR ("No blocking issues found") — independently re-derived the same conclusions:

• Cache key: tuple key + component-wise k[0] == repo and k[-1] == user_id matching genuinely fixes the |-collision. Confirmed _raise_403/_raise_for_status always raise on every path into them (no silent fallthrough to the generic >= 400 branch), and _map_github_error checks RateLimitedError before the unconditional 502 fallback, so ordering is correct even though RateLimitedError and InsufficientScopeError are siblings under GitHubConnectError.
• Search sanitization: _sanitize_search correctly neutralizes repo:/is: injection by per-word quoting, strips embedded quotes (can't reopen a second phrase), and the empty-after-sanitize case correctly falls back to the list endpoint. Traced "0" as a Retry-After value — it's a non-empty string so if retry_after: is truthy, no falsy-string footgun there.
• Rate-limit vs. scope: _raise_403's if retry_after is None and status_code != 429 correctly forces every plain 429 to RateLimitedError regardless of headers/body, while a 403 with no throttle signal still falls through to InsufficientScopeError. RateLimitedError(GitHubConnectError) subclassing preserves existing broad except GitHubConnectError sites.

No new correctness issues found beyond what's already noted in the thread. One trivial, non-blocking nit not previously mentioned: _IssueCacheKey = tuple (line 171) is an unparameterized alias — it works fine at runtime but doesn't type-check the tuple shape any more precisely than tuple alone would. Not worth a diff for a 6-field internal cache key.

Test coverage for the three defects (cache collision, qualifier injection incl. embedded-quote and multiword AND-of-terms cases, rate-limit-vs-scope incl. both primary/secondary detection paths) looks complete.

[github-actions]

Claude finished @frankbria's task in 4m 30s —— View job

---

GLM bug-hunting review — complete ✅

• Gather PR diff + context (gh pr view / gh pr diff)
• Check existing comments to avoid repeats
• Read changed files + callers/callees in checked-out repo
• Confirm concrete defects against real code
• Post inline findings + summary

✅ GLM review: no defects found.

Re-ran independently on the final (synchronize) diff. Scope = concrete defects only (logic, null/error paths, security, data loss, races, contracts). Verified against the checked-out files, not the patch alone:

Area	Verdict	Why it holds
_map_github_error isinstance order	✅	InvalidToken → InsufficientScope → RateLimited → 502 fallback. All four are distinct siblings under GitHubConnectError (RateLimitedError subclasses the base directly, github_issues_service.py:59), so no clause shadows another; order between them is immaterial.
_raise_403 always raises	✅	retry_after is None and status_code != 429 → InsufficientScopeError; every other path → RateLimitedError. Both branches raise. Only ever called with 403/429 (_raise_for_status:142, get_issue probe:343).
"" vs None retry hint	✅	_rate_limit_retry_after returns "" for header/body-only throttling — falsy, so hint = … if retry_after else "" is skipped; no "Retry after s.". None is the sole value that routes to InsufficientScope.
Broad handlers absorb the new subclass	✅	Verified at the call sites: tasks._safe_close_issue (except Exception, tasks.py:613) and reconciliation.IssueState.is_closed (except Exception, reconciliation.py:175) both swallow RateLimitedError. Auto-close/recon behavior unchanged.
_sanitize_search qualifier injection	✅	All " removed before .split() (line 499), so no surviving word can contain a quote → each "word" is a self-closed phrase; the bare repo:owner/name is appended outside any quote. No way to inject a second repo:/is: qualifier. Empty/punctuation-only input → "" → if term: falls through to the list endpoint (line 218).
Tuple cache key + component invalidate	✅	Only get_issues inserts keys (line 421), always a 6-tuple with repo first / user_id last, so _issue_cache_invalidate (line 214) k[0]==repo and k[-1]==user_id is safe on every real key. Per-tenant separation preserved — PAT resolved per-user at line 404 before the keyed write.
/connect handler not affected	✅	The one except InsufficientScopeError clause (router:290) is on /connect, which calls validate_connection — that function never raises RateLimitedError (only github_issues_service.py does). No missed-handler regression.

Considered and discarded (below the precision bar): a search term ending in \ yields a word like foo\ → "foo\" in the query. Even under the (undocumented, unlikely) interpretation that GitHub honored \" as an in-phrase escape and the phrase stayed open, the only consequence would be an unscoped exact-phrase search returning ~0 results — no bare repo: qualifier is reintroduced, no PAT enumeration, no data the PAT couldn't already read. No concrete failure scenario with meaningful impact.

[claude]

Review

Solid, well-tested fix for the three defects described in #956 — the tuple cache key, the component-wise invalidation match, and the 403-vs-429 rate-limit classification all look correct and are backed by good regression tests (including the adversarial test_embedded_quotes_cannot_break_out case).

One real gap: the search-qualifier-injection fix doesn't cover label

codeframe/core/github_issues_service.py:520-521:

if label.strip():
    qualifiers.append(f'label:"{label.strip()}"')

_sanitize_search quotes/neutralizes the search term before it reaches _search_issues, but label (also a free-text query param — Query("", description="Filter by a single label name") in github_integrations_v2.py:390, no allowlist against the repo's actual labels) is interpolated into the same q string with only a naive f'label:"{label.strip()}"' wrap. It is never quote-stripped like search is.

Since the label value can contain a ", an attacker can close the quoted phrase early and inject a bare qualifier, e.g.:

search=x
label=x" repo:secret-org/secret-repo "

produces q = "x" repo:acme/app is:issue is:open label:"x" repo:secret-org/secret-repo "", re-opening exactly the cross-repo qualifier-injection hole this PR's item #2 claims to close (PAT enumeration through a text field in a hosted deployment) — just through the other free-text parameter that flows into the same query.

This only triggers on the _search_issues path (i.e. search must also be non-empty — the plain /repos/{o}/{r}/issues list path passes label as a real query param via httpx, which is safe), but that's a low bar since both are ordinary query params on the same endpoint.

Suggest applying the same treatment used for search — e.g. reuse _sanitize_search on label (stripping embedded quotes) before building label:"...", or at minimum strip " characters from label the way _sanitize_search does for the search term. Worth a test mirroring TestSearchQualifierInjection but keyed on label.

Everything else looks good

• _raise_403/_raise_for_status control flow is sound — every branch that should raise does, no silent fallthrough.
• Rate-limit detection order (Retry-After → X-RateLimit-Remaining: 0 → body message → plain 429) is sensible, and correctly treats retry_after == "" (indicator present, no delay value) as "throttled, no hint" rather than "not throttled."
• _map_github_error centralizing the mapping and being reused by get_issues is a nice simplification; RateLimitedError subclassing GitHubConnectError means existing broad except GitHubConnectError call sites (reconciliation, auto-close) keep working without modification, as claimed.
• Test coverage for the cache-key and invalidation fixes (TestIssueCacheKeyAmbiguity, test_invalidation_ignores_repo_named_like_a_search_term) is thorough and directly demonstrates the old bug would have failed these.
• CLAUDE.md update accurately reflects the new behavior.

No other correctness, performance, or test-coverage concerns found.