Hostile Code Review

Line 77 always divides by 10, even when a channel has fewer than 10 papers. If a channel has 3 papers with 100, 50, and 30 citations, the "mean of top 10" is (100+50+30)/10 = 18, not 60. The docstring says "mean citation count of top-10 papers (sum / 10, even if fewer exist)", so this is documented behavior — but it systematically penalizes channels with few papers, which may not be the intent.

# Line 77: always divides by 10
mean_cites = sum(p.get("cited_by_count", 0) for p in top_10) / 10

Line 77

This function divides by hardcoded 10. But in main() at line 289, a separate mean_top10 computation divides by len(top_papers_cites) — the actual number of papers in the slice (capped at 10). For a channel with 3 papers, this function computes sum/10 = 18 while main() computes sum/3 = 60. The composite score uses this function, but the JSON output reports the other value. A reader comparing citation_impact against mean_top10_citations will find they do not reconcile.

# Line 77 (compute_citation_impact): always /10
mean_cites = sum(...) / 10

# Line 289 (main): /len(top_papers_cites)
mean_top10 = sum(top_papers_cites) / len(top_papers_cites) if top_papers_cites else 0

Lines 77, 289

Lines 180–184 add primary_channel and secondary_channels keys directly to the input paper dicts. The function signature and docstring say "Returns: Tuple of (papers_with_assignments, ...)" which implies it returns new data, but the returned papers list is the same list with the same dict objects, now mutated.

This means the caller's original data is silently altered. In main() (line 259), the papers list loaded from JSON is passed in and permanently modified. Any subsequent code that inspects the raw paper data will see these injected keys. This is a side-effect contract violation: the function's return type suggests transformation, but the implementation is mutation.

# Lines 180-184: modifies the dict objects from the input list
paper["primary_channel"] = channels[0]
paper["secondary_channels"] = channels[1:] if len(channels) > 1 else []

Lines 180–184

This is the downstream half of the inconsistency noted above. At line 289, mean_top10 divides by len(top_papers_cites), which is the actual number of papers in the top-10 slice (could be 1–10). The citation_impact sub-score stored in the same output record was computed by compute_citation_impact which always divides by 10.

The JSON output therefore contains two numbers that claim to represent the same concept (citation_impact and mean_top10_citations) but use different denominators. For a channel with 3 papers, the reported mean_top10_citations will be 3.33x higher than the value that actually drove the composite score.

# Line 289
mean_top10 = sum(top_papers_cites) / len(top_papers_cites) if top_papers_cites else 0

Line 289

Line 125 constructs the filter as f">{min_citations - 1}" to mean "greater than or equal to min_citations". For min_citations=5, this produces ">4" which is correct. But the OpenAlex API actually supports the >= operator directly via ">4" or "5-" range syntax. The current approach works but is an unnecessarily clever encoding of >=.

f">{min_citations - 1}" if min_citations > 0 else None

Line 125

Line 42 sets self.last_request_time = time.time() after the potential sleep. This means the recorded time is the moment after the sleep, not the moment the request fires. If time.sleep overshoots (common on Windows), the next call's time_since_last will be measured from an inflated timestamp, making the actual delay shorter than min_delay. In practice, the overshoot is small (<1ms on modern systems), and the rate limit is 100ms between requests, so this is unlikely to cause API throttling. But for correctness, the timestamp should be set immediately before the request, not after the sleep.

def _rate_limit(self):
    current_time = time.time()
    time_since_last = current_time - self.last_request_time
    if time_since_last < self.min_delay:
        time.sleep(self.min_delay - time_since_last)
    self.last_request_time = time.time()  # set after sleep, not before request

Lines 36–42

Lines 77–81: on a 403 response, the code sleeps and retries but does not check whether this is the last attempt. If all 5 retries get 403, the loop exits and falls through to line 99: raise Exception(f"Failed after {max_retries} retries"). This is technically correct, but the generic exception message loses the context that the server consistently returned 403. The response body (which often contains rate-limit reset times or error details) is discarded.

Lines 77–81, 99

Line 68: urljoin(self.BASE_URL, endpoint) where BASE_URL = "https://api.openalex.org" and endpoint = "/works". Because BASE_URL has no trailing slash, urljoin replaces the path: urljoin("https://api.openalex.org", "/works") produces "https://api.openalex.org/works" which is correct. However, if endpoint were a relative path like "works" (no leading slash), urljoin would produce "https://api.openalex.org/works" — still correct by coincidence because the base has no path. But with a base like "https://api.openalex.org/v1", urljoin(..., "works") would produce "https://api.openalex.org/works", dropping /v1. Since all callers use leading-slash endpoints, this is fine today but fragile.

Line 68

The docstring says "up to 50 per batch" and the code processes in chunks of 50. However, if the API returns fewer results than requested (e.g., some IDs are invalid or deleted), the function silently returns fewer results than IDs were provided. There is no warning and no way for the caller to identify which IDs failed. For a lookup function, this is a surprising contract.

Lines 178–190

Even the large-sample path (lines 268–290) makes each request with per-page=200, but sets sample to up to 10,000. This means each request returns only 200 of the 10,000 sampled works. The remaining 9,800 are inaccessible because the /works?sample=N endpoint does not support pagination (each request is an independent random draw). So even the "large sample" path returns at most 200 results per iteration, and the loop runs (sample_size // 10000) + 1 times, yielding at most ~200 * (N/10000 + 1) results for a request of N > 10,000.

Lines 268–290

Line 321: response.get('group_by', []) assumes the API returns a group_by key. The OpenAlex API actually returns the grouped data under the key "group_by" when the request includes the group_by parameter. However, if the API changes its response structure or returns an error response (which would lack this key), the function silently returns an empty list with no indication of failure. This is a minor robustness concern.

Line 321