From 269dda9f6cb080e0a22dd104943789be52e8adcc Mon Sep 17 00:00:00 2001 From: Ilia Alshanetsky Date: Tue, 19 May 2026 12:18:47 -0400 Subject: [PATCH 1/2] refactor(github): split search_github / parse_github_response / enrich_with_comments search_github returned a normalized List[dict] directly while every other adapter follows search_X -> dict envelope, parse_X_response -> list[dict]. The github branch in pipeline._retrieve_stream was the only one that called search_* and returned (result, {}) without a parse step. This blocked fixture-driven testing: there was no parse function to feed a synthetic envelope to. Split into three: search_github(...) -> Dict[str, Any] HTTP fetch only. Returns {"items": [raw items], "context": {core, from_date, to_date, count}}. parse_github_response(response) -> List[Dict[str, Any]] Pure function. Normalizes, date-filters, sorts by relevance. enrich_with_comments(items, depth, token) -> List[Dict[str, Any]] Public extraction of the old private _enrich_top_items. Resolves the token via env / gh CLI fallback so callers don't have to. Pipeline now does the standard 3-call dance: response = github.search_github(...) items = github.parse_github_response(response) items = github.enrich_with_comments(items, depth=depth, token=token) Keeping enrich_with_comments in parse_github_response would make parse impure and force every fixture-driven test to either mock HTTP or skip enrichment. Splitting it out matches the YouTube adapter's pattern. --- skills/last30days/scripts/lib/github.py | 83 +++++++++++--- skills/last30days/scripts/lib/pipeline.py | 7 +- tests/test_github.py | 132 +++++++++++++++++++--- 3 files changed, 190 insertions(+), 32 deletions(-) diff --git a/skills/last30days/scripts/lib/github.py b/skills/last30days/scripts/lib/github.py index a48dd21..4638c4c 100644 --- a/skills/last30days/scripts/lib/github.py +++ b/skills/last30days/scripts/lib/github.py @@ -142,8 +142,14 @@ def search_github( to_date: str, depth: str = "default", token: Optional[str] = None, -) -> List[Dict[str, Any]]: - """Search GitHub Issues and PRs. +) -> Dict[str, Any]: + """Search GitHub Issues and PRs (HTTP fetch only). + + Returns a raw envelope shaped like every other adapter's ``search_X``: + ``{"items": [raw GitHub API items], "context": {core, from_date, + to_date, count}}``. Normalization, date filtering, and sorting move + to ``parse_github_response``; comment enrichment moves to + ``enrich_with_comments``. Args: topic: Search topic @@ -153,12 +159,12 @@ def search_github( token: Optional GitHub token (falls back to env/gh CLI) Returns: - List of normalized item dicts. Empty list on any failure. + Dict envelope. Empty ``items`` list on any failure. """ resolved_token = _resolve_token(token) if not resolved_token: _log("No GitHub token available (set GITHUB_TOKEN or install gh CLI)") - return [] + return {"items": [], "error": "no token"} count = DEPTH_LIMITS.get(depth, DEPTH_LIMITS["default"]) core = extract_core_subject(topic) @@ -176,12 +182,41 @@ def search_github( data = _fetch_json(url, token=resolved_token, timeout=30) if not data: - return [] + return {"items": [], "context": {"core": core, "from_date": from_date, + "to_date": to_date, "count": count}} raw_items = data.get("items", []) _log(f"Found {len(raw_items)} issues/PRs") - items = [] + return { + "items": raw_items, + "context": { + "core": core, + "from_date": from_date, + "to_date": to_date, + "count": count, + }, + } + + +def parse_github_response(response: Dict[str, Any]) -> List[Dict[str, Any]]: + """Normalize a ``search_github`` envelope into the skill's item shape. + + Pure function: no I/O, no token, no enrichment. Applies the date + filter using the search context and sorts by relevance. + """ + if not isinstance(response, dict): + return [] + raw_items = response.get("items") or [] + if not isinstance(raw_items, list): + return [] + context = response.get("context") or {} + core = context.get("core") or "" + from_date = context.get("from_date") or "" + to_date = context.get("to_date") or "" + count = context.get("count") or DEPTH_LIMITS["default"] + + items: List[Dict[str, Any]] = [] for i, item in enumerate(raw_items[:count]): html_url = item.get("html_url", "") repo = _parse_repo_from_url(html_url) @@ -224,20 +259,34 @@ def search_github( }, }) - # Enrich top items with comments - items = _enrich_top_items(items, depth, resolved_token) - # Date filter - filtered = [] - for item in items: - d = item.get("date") - if d is None or (from_date <= d <= to_date): - filtered.append(item) + if from_date and to_date: + items = [ + item for item in items + if item.get("date") is None or (from_date <= item["date"] <= to_date) + ] - # Sort by relevance - filtered.sort(key=lambda x: x.get("relevance", 0), reverse=True) + items.sort(key=lambda x: x.get("relevance", 0), reverse=True) + return items - return filtered + +def enrich_with_comments( + items: List[Dict[str, Any]], + depth: str = "default", + token: Optional[str] = None, +) -> List[Dict[str, Any]]: + """Fetch top comments for top-K items by reactions and attach to metadata. + + Mutates and returns ``items``. Resolves ``token`` via env/gh CLI when + not supplied, matching ``search_github``'s fallback chain. + """ + if not items: + return items + resolved_token = _resolve_token(token) + if not resolved_token: + _log("No GitHub token available for comment enrichment") + return items + return _enrich_top_items(items, depth, resolved_token) def _enrich_top_items( diff --git a/skills/last30days/scripts/lib/pipeline.py b/skills/last30days/scripts/lib/pipeline.py index 4d64edb..e45c1d6 100644 --- a/skills/last30days/scripts/lib/pipeline.py +++ b/skills/last30days/scripts/lib/pipeline.py @@ -1007,8 +1007,11 @@ def _retrieve_stream( result = polymarket.search_polymarket(subquery.search_query, from_date, to_date, depth=depth) return polymarket.parse_polymarket_response(result, topic=subquery.search_query), {} if source == "github": - result = github.search_github(subquery.search_query, from_date, to_date, depth=depth, token=config.get("GITHUB_TOKEN")) - return result, {} + token = config.get("GITHUB_TOKEN") + response = github.search_github(subquery.search_query, from_date, to_date, depth=depth, token=token) + items = github.parse_github_response(response) + items = github.enrich_with_comments(items, depth=depth, token=token) + return items, {} if source == "pinterest": result = pinterest.search_pinterest( subquery.search_query, from_date, to_date, diff --git a/tests/test_github.py b/tests/test_github.py index b13d435..bc57791 100644 --- a/tests/test_github.py +++ b/tests/test_github.py @@ -76,13 +76,14 @@ class TestParseDate(unittest.TestCase): class TestSearchGithub(unittest.TestCase): @patch.dict("os.environ", {}, clear=True) @patch("subprocess.run", side_effect=FileNotFoundError) - def test_no_token_returns_empty(self, mock_run): + def test_no_token_returns_empty_envelope(self, mock_run): result = github.search_github("react", "2026-03-01", "2026-03-31", token=None) - self.assertEqual(result, []) + self.assertEqual(result.get("items", []), []) + self.assertIn("error", result) @patch.object(github, "_fetch_json") @patch.object(github, "_resolve_token", return_value="test-token") - def test_search_returns_items(self, mock_token, mock_fetch): + def test_search_returns_raw_envelope(self, mock_token, mock_fetch): mock_fetch.return_value = { "total_count": 1, "items": [ @@ -99,9 +100,15 @@ class TestSearchGithub(unittest.TestCase): }, ], } - result = github.search_github("react", "2026-03-01", "2026-03-31") - self.assertEqual(len(result), 1) - item = result[0] + # Search returns raw envelope; parse normalizes. + response = github.search_github("react", "2026-03-01", "2026-03-31") + self.assertEqual(len(response["items"]), 1) + self.assertEqual(response["items"][0]["title"], "React Server Components bug") + self.assertEqual(response["context"]["from_date"], "2026-03-01") + + items = github.parse_github_response(response) + self.assertEqual(len(items), 1) + item = items[0] self.assertEqual(item["source"], "github") self.assertEqual(item["container"], "facebook/react") self.assertEqual(item["title"], "React Server Components bug") @@ -117,10 +124,11 @@ class TestSearchGithub(unittest.TestCase): @patch.object(github, "_fetch_json", return_value=None) @patch.object(github, "_resolve_token", return_value="test-token") - def test_rate_limit_returns_empty(self, mock_token, mock_fetch): - """403 rate limit returns empty list gracefully.""" - result = github.search_github("react", "2026-03-01", "2026-03-31") - self.assertEqual(result, []) + def test_rate_limit_returns_empty_envelope(self, mock_token, mock_fetch): + """403 rate limit returns envelope with empty items list.""" + response = github.search_github("react", "2026-03-01", "2026-03-31") + self.assertEqual(response["items"], []) + self.assertEqual(github.parse_github_response(response), []) @patch.object(github, "_fetch_json") @patch.object(github, "_resolve_token", return_value="test-token") @@ -142,9 +150,107 @@ class TestSearchGithub(unittest.TestCase): }, ], } - result = github.search_github("next.js", "2026-03-01", "2026-03-31") - self.assertEqual(len(result), 1) - self.assertTrue(result[0]["metadata"]["is_pr"]) + response = github.search_github("next.js", "2026-03-01", "2026-03-31") + items = github.parse_github_response(response) + self.assertEqual(len(items), 1) + self.assertTrue(items[0]["metadata"]["is_pr"]) + + +class TestParseGithubResponse(unittest.TestCase): + """Fixture-driven parse tests: feed a synthetic search_github envelope to + parse_github_response and assert normalized output. + + This contract (search returns dict envelope, parse turns it into a list) + matches every other source adapter. Before this refactor, search_github + returned a bare list and there was no parse step, blocking fixture tests. + """ + + _RAW_ENVELOPE = { + "items": [ + { + "html_url": "https://github.com/facebook/react/issues/42", + "title": "React Server Components bug", + "body": "There is a bug when using RSC with streaming...", + "created_at": "2026-03-15T10:00:00Z", + "state": "open", + "comments": 12, + "reactions": {"total_count": 8}, + "labels": [{"name": "bug"}, {"name": "rsc"}], + "user": {"login": "testuser"}, + }, + { + "html_url": "https://github.com/vercel/next.js/pull/99", + "title": "Add streaming support", + "body": "This PR adds...", + "created_at": "2026-03-20T10:00:00Z", + "state": "open", + "comments": 5, + "reactions": {"total_count": 3}, + "labels": [], + "user": {"login": "dev"}, + "pull_request": {"url": "..."}, + }, + ], + "context": { + "core": "react", + "from_date": "2026-03-01", + "to_date": "2026-03-31", + "count": 25, + }, + } + + def test_normalizes_items(self): + items = github.parse_github_response(self._RAW_ENVELOPE) + self.assertEqual(len(items), 2) + by_url = {i["url"]: i for i in items} + issue = by_url["https://github.com/facebook/react/issues/42"] + self.assertEqual(issue["source"], "github") + self.assertEqual(issue["container"], "facebook/react") + self.assertEqual(issue["title"], "React Server Components bug") + self.assertEqual(issue["date"], "2026-03-15") + self.assertEqual(issue["author"], "testuser") + self.assertEqual(issue["engagement"]["reactions"], 8) + self.assertEqual(issue["engagement"]["comments"], 12) + self.assertFalse(issue["metadata"]["is_pr"]) + + def test_detects_pr(self): + items = github.parse_github_response(self._RAW_ENVELOPE) + pr = next(i for i in items if "/pull/" in i["url"]) + self.assertTrue(pr["metadata"]["is_pr"]) + + def test_date_filter_drops_outside_window(self): + envelope = { + "items": [ + { + "html_url": "https://github.com/foo/bar/issues/1", + "title": "Too old", + "created_at": "2026-01-15T10:00:00Z", + "comments": 0, "reactions": {"total_count": 0}, + "labels": [], "user": {"login": "x"}, + }, + { + "html_url": "https://github.com/foo/bar/issues/2", + "title": "In window", + "created_at": "2026-03-15T10:00:00Z", + "comments": 0, "reactions": {"total_count": 0}, + "labels": [], "user": {"login": "x"}, + }, + ], + "context": {"core": "foo", "from_date": "2026-03-01", + "to_date": "2026-03-31", "count": 25}, + } + items = github.parse_github_response(envelope) + self.assertEqual(len(items), 1) + self.assertEqual(items[0]["title"], "In window") + + def test_sorts_by_relevance(self): + items = github.parse_github_response(self._RAW_ENVELOPE) + scores = [i.get("relevance", 0) for i in items] + self.assertEqual(scores, sorted(scores, reverse=True)) + + def test_empty_envelope(self): + self.assertEqual(github.parse_github_response({"items": []}), []) + self.assertEqual(github.parse_github_response({}), []) class TestComputeRelevance(unittest.TestCase): From c5c0239dc9cb90592535bea13c78f9ee7266a3a9 Mon Sep 17 00:00:00 2001 From: Ilia Alshanetsky Date: Tue, 19 May 2026 12:35:40 -0400 Subject: [PATCH 2/2] refactor(github): resolve token once at pipeline boundary; pad no-token envelope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- skills/last30days/scripts/lib/github.py | 27 +++++++++++++++++++---- skills/last30days/scripts/lib/pipeline.py | 5 ++++- tests/test_github.py | 15 +++++++++++++ 3 files changed, 42 insertions(+), 5 deletions(-) diff --git a/skills/last30days/scripts/lib/github.py b/skills/last30days/scripts/lib/github.py index 4638c4c..ccf5b8d 100644 --- a/skills/last30days/scripts/lib/github.py +++ b/skills/last30days/scripts/lib/github.py @@ -62,6 +62,17 @@ def _resolve_token(token: Optional[str] = None) -> Optional[str]: 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( url: str, token: Optional[str] = None, @@ -161,13 +172,21 @@ def search_github( Returns: 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) if not resolved_token: _log("No GitHub token available (set GITHUB_TOKEN or install gh CLI)") - return {"items": [], "error": "no token"} - - count = DEPTH_LIMITS.get(depth, DEPTH_LIMITS["default"]) - core = extract_core_subject(topic) + return { + "items": [], + "error": "no token", + "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})") # Build search query with date filter diff --git a/skills/last30days/scripts/lib/pipeline.py b/skills/last30days/scripts/lib/pipeline.py index e45c1d6..491432c 100644 --- a/skills/last30days/scripts/lib/pipeline.py +++ b/skills/last30days/scripts/lib/pipeline.py @@ -1007,7 +1007,10 @@ def _retrieve_stream( result = polymarket.search_polymarket(subquery.search_query, from_date, to_date, depth=depth) return polymarket.parse_polymarket_response(result, topic=subquery.search_query), {} 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) items = github.parse_github_response(response) items = github.enrich_with_comments(items, depth=depth, token=token) diff --git a/tests/test_github.py b/tests/test_github.py index bc57791..c872c6f 100644 --- a/tests/test_github.py +++ b/tests/test_github.py @@ -80,6 +80,21 @@ class TestSearchGithub(unittest.TestCase): result = github.search_github("react", "2026-03-01", "2026-03-31", token=None) self.assertEqual(result.get("items", []), []) 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, "_resolve_token", return_value="test-token")