refactor(github): resolve token once at pipeline boundary; pad no-token envelope
Greptile review (PR #438) flagged two issues: 1. search_github and enrich_with_comments both call _resolve_token, so when GITHUB_TOKEN is absent from config and env the gh-CLI subprocess (with its 5s timeout) fires twice per query. 2. The no-token early-return envelope `{"items": [], "error": "no token"}` was missing the `context` key that every other failure path includes, making the envelope shape inconsistent between the no-token and fetch-failure cases. Fix 1: add public github.resolve_token(token) wrapping the existing _resolve_token. Pipeline calls it once before search and enrich, so both downstream calls receive an already-resolved (or already-None) token and skip the fallback chain. Fix 2: thread core/from_date/to_date/count through the no-token envelope's `context` key, matching the fetch-failure envelope shape. parse_github_response was already tolerant of the missing key, but diagnostics callers that read response["context"]["..."] now get a consistent dict in both error paths. Reviewer's suggested code patch for issue 1 was a no-op (it kept the same _resolve_token(token) call inside enrich_with_comments); the underlying intent — resolve at the boundary — is what this commit implements.
This commit is contained in:
@@ -62,6 +62,17 @@ def _resolve_token(token: Optional[str] = None) -> Optional[str]:
|
|||||||
return None
|
return None
|
||||||
|
|
||||||
|
|
||||||
|
def resolve_token(token: Optional[str] = None) -> Optional[str]:
|
||||||
|
"""Public alias for ``_resolve_token``.
|
||||||
|
|
||||||
|
The pipeline calls this once before ``search_github`` and
|
||||||
|
``enrich_with_comments`` so the ``gh auth token`` subprocess fallback
|
||||||
|
only fires once per query when ``GITHUB_TOKEN`` is unset, instead of
|
||||||
|
twice (once per call site).
|
||||||
|
"""
|
||||||
|
return _resolve_token(token)
|
||||||
|
|
||||||
|
|
||||||
def _fetch_json(
|
def _fetch_json(
|
||||||
url: str,
|
url: str,
|
||||||
token: Optional[str] = None,
|
token: Optional[str] = None,
|
||||||
@@ -161,13 +172,21 @@ def search_github(
|
|||||||
Returns:
|
Returns:
|
||||||
Dict envelope. Empty ``items`` list on any failure.
|
Dict envelope. Empty ``items`` list on any failure.
|
||||||
"""
|
"""
|
||||||
|
count = DEPTH_LIMITS.get(depth, DEPTH_LIMITS["default"])
|
||||||
|
core = extract_core_subject(topic)
|
||||||
resolved_token = _resolve_token(token)
|
resolved_token = _resolve_token(token)
|
||||||
if not resolved_token:
|
if not resolved_token:
|
||||||
_log("No GitHub token available (set GITHUB_TOKEN or install gh CLI)")
|
_log("No GitHub token available (set GITHUB_TOKEN or install gh CLI)")
|
||||||
return {"items": [], "error": "no token"}
|
return {
|
||||||
|
"items": [],
|
||||||
count = DEPTH_LIMITS.get(depth, DEPTH_LIMITS["default"])
|
"error": "no token",
|
||||||
core = extract_core_subject(topic)
|
"context": {
|
||||||
|
"core": core,
|
||||||
|
"from_date": from_date,
|
||||||
|
"to_date": to_date,
|
||||||
|
"count": count,
|
||||||
|
},
|
||||||
|
}
|
||||||
_log(f"Searching for '{core}' (raw: '{topic}', since {from_date}, count={count})")
|
_log(f"Searching for '{core}' (raw: '{topic}', since {from_date}, count={count})")
|
||||||
|
|
||||||
# Build search query with date filter
|
# Build search query with date filter
|
||||||
|
|||||||
@@ -1007,7 +1007,10 @@ def _retrieve_stream(
|
|||||||
result = polymarket.search_polymarket(subquery.search_query, from_date, to_date, depth=depth)
|
result = polymarket.search_polymarket(subquery.search_query, from_date, to_date, depth=depth)
|
||||||
return polymarket.parse_polymarket_response(result, topic=subquery.search_query), {}
|
return polymarket.parse_polymarket_response(result, topic=subquery.search_query), {}
|
||||||
if source == "github":
|
if source == "github":
|
||||||
token = config.get("GITHUB_TOKEN")
|
# Resolve once at the pipeline boundary so search and enrich
|
||||||
|
# share the result; otherwise each call would re-run the env
|
||||||
|
# lookup and gh-CLI subprocess fallback (up to 5s timeout each).
|
||||||
|
token = github.resolve_token(config.get("GITHUB_TOKEN"))
|
||||||
response = github.search_github(subquery.search_query, from_date, to_date, depth=depth, token=token)
|
response = github.search_github(subquery.search_query, from_date, to_date, depth=depth, token=token)
|
||||||
items = github.parse_github_response(response)
|
items = github.parse_github_response(response)
|
||||||
items = github.enrich_with_comments(items, depth=depth, token=token)
|
items = github.enrich_with_comments(items, depth=depth, token=token)
|
||||||
|
|||||||
@@ -80,6 +80,21 @@ class TestSearchGithub(unittest.TestCase):
|
|||||||
result = github.search_github("react", "2026-03-01", "2026-03-31", token=None)
|
result = github.search_github("react", "2026-03-01", "2026-03-31", token=None)
|
||||||
self.assertEqual(result.get("items", []), [])
|
self.assertEqual(result.get("items", []), [])
|
||||||
self.assertIn("error", result)
|
self.assertIn("error", result)
|
||||||
|
# Envelope shape must match the fetch-failure path: context is
|
||||||
|
# always present so callers (and parse_github_response) can read
|
||||||
|
# diagnostic fields without branching on which failure mode hit.
|
||||||
|
self.assertIn("context", result)
|
||||||
|
self.assertEqual(result["context"]["from_date"], "2026-03-01")
|
||||||
|
self.assertEqual(result["context"]["to_date"], "2026-03-31")
|
||||||
|
|
||||||
|
def test_resolve_token_public_alias(self):
|
||||||
|
"""resolve_token is the public entry point pipeline uses; _resolve_token stays
|
||||||
|
private. Both should return the same value for the same input."""
|
||||||
|
self.assertEqual(
|
||||||
|
github.resolve_token("explicit-token"),
|
||||||
|
github._resolve_token("explicit-token"),
|
||||||
|
)
|
||||||
|
self.assertEqual(github.resolve_token("explicit-token"), "explicit-token")
|
||||||
|
|
||||||
@patch.object(github, "_fetch_json")
|
@patch.object(github, "_fetch_json")
|
||||||
@patch.object(github, "_resolve_token", return_value="test-token")
|
@patch.object(github, "_resolve_token", return_value="test-token")
|
||||||
|
|||||||
Reference in New Issue
Block a user