From 092da76e973742497b3df8c0f18147bd5d01f7e9 Mon Sep 17 00:00:00 2001 From: ibrog Date: Thu, 17 Sep 2026 15:07:31 +0300 Subject: [PATCH 1/2] fix(pipeline): sample the real GraphQL budget Generated-with: Codex --- README.md | 6 +- promptpilot/pipeline_insights.py | 264 +++++++++++++++++++++++++++---- tests/test_github_budget.py | 105 +++++++++++- tests/test_schedule_series.py | 41 ++++- 4 files changed, 371 insertions(+), 45 deletions(-) diff --git a/README.md b/README.md index 69d2876..d6274de 100644 --- a/README.md +++ b/README.md @@ -972,8 +972,10 @@ P2, question — P3. Каждые `aging_hours` ожидания эффекти снимать и повторно ставить `ship` из-за гонки между UI и завершением REVIEW. `github_budget` необязателен и включается только присутствием в профиле. Перед -GitHub-работой PromptPilot читает бесплатный для primary rate limit -`GET /rate_limit`. Поле `minimum_remaining` задаёт аварийный остаток, который +GitHub-работой PromptPilot читает бесплатный для primary REST rate limit +`GET /rate_limit`, а GraphQL-остаток и текущий `viewer` — коротким +GraphQL-запросом, обычно стоимостью в один GraphQL point. Поле +`minimum_remaining` задаёт аварийный остаток, который нельзя обещать ни одному запуску, а `costs` — консервативную верхнюю оценку расхода каждого маршрута. Допуск вычисляется как `remaining - резервы выполняющихся задач - cost маршрута >= minimum_remaining`. diff --git a/promptpilot/pipeline_insights.py b/promptpilot/pipeline_insights.py index d422dda..b64ef6c 100644 --- a/promptpilot/pipeline_insights.py +++ b/promptpilot/pipeline_insights.py @@ -83,6 +83,10 @@ class _GitHubScanLeaseUnavailable(_GitHubScanLeaseFailure): """SQLite temporarily prevented verification of an otherwise owned lease.""" +class _GitHubRateLimitUnavailable(RuntimeError): + """The authenticated GitHub budget cannot be proved from valid responses.""" + + class _GitHubScanLease: """Renew one SQLite-backed scan lease while external work is in flight.""" @@ -500,6 +504,41 @@ def _replace_admission_with_lease_failure( return admission +def _rate_limit_failure_decision( + profile: dict, exc: _GitHubRateLimitUnavailable, *, + status_revision: int | None = None) -> dict: + try: + policy = _github_budget_policy(profile) + if policy is not None: + policy = _with_shared_budget_floor(policy) + except (TypeError, ValueError): + policy = None + policy = policy or { + "minimum_remaining": dict(_DEFAULT_GITHUB_BUDGET_MINIMUM), + "unavailable_retry_seconds": 300, + "lease_scope": _GITHUB_SCAN_LEASE_SCOPE, + } + return _budget_denied( + policy, state="rate_limit_unavailable", + reason=f"GitHub API budget unavailable: {exc}", now=time.time(), + status_revision=status_revision) + + +def _replace_admission_with_rate_limit_failure( + admission: dict, profile: dict, + exc: _GitHubRateLimitUnavailable) -> dict: + """Keep the durable refresh denial aligned with a failed final sample.""" + lease = admission.get("_lease") + denied = _rate_limit_failure_decision( + profile, exc, + status_revision=_current_github_scan_status_revision()) + if lease is not None: + denied["_lease"] = lease + admission.clear() + admission.update(denied) + return admission + + def _evaluate_github_budget(policy: dict, limits: dict | None, *, now: float, status_revision: int | None = None, @@ -660,8 +699,12 @@ def _github_scan_admission(profile: dict, purpose: str, limits = _github_rate_limits() except _GitHubScanLeaseFailure: raise - except Exception: - limits = None + except _GitHubRateLimitUnavailable as exc: + decision = _rate_limit_failure_decision( + profile, exc, status_revision=lease.status_revision) + decision["_lease"] = lease + yield decision + return lease.ensure_owned(renew=True) if lease.lost: decision = _lease_failure_decision( @@ -800,8 +843,13 @@ def _reserve_execution_admission( limits = _github_rate_limits() except _GitHubScanLeaseFailure: raise - except Exception: - limits = None + except _GitHubRateLimitUnavailable as exc: + decision = _rate_limit_failure_decision( + profile, exc, status_revision=lease.status_revision) + decision["_lease"] = lease + admission.clear() + admission.update(decision) + return admission if not isinstance(limits, dict): decision = _evaluate_github_budget( policy, limits, now=time.time(), @@ -1277,40 +1325,181 @@ def _gh_api_json(args: list[str], input_value: dict | None = None): return json.loads(run.stdout) if run.stdout.strip() else None -def _github_rate_limits() -> dict | None: - """Return the authenticated GitHub budgets without spending core quota.""" +_GITHUB_LOGIN = re.compile(r"[A-Za-z0-9](?:[A-Za-z0-9-]{0,37}[A-Za-z0-9])?") +_GITHUB_RATE_LIMIT_QUERY = """query PromptPilotRateLimit { + viewer { login } + rateLimit { limit remaining resetAt used } +}""" + + +def _valid_github_login(value) -> bool: + return isinstance(value, str) and _GITHUB_LOGIN.fullmatch(value) is not None + + +def _trusted_github_accounts() -> set[str]: + """Return safe identity contracts for budget-enabled shared profiles.""" try: - payload = _gh_api_json(["rate_limit"]) - resources = payload.get("resources", {}) if isinstance(payload, dict) else {} - result = {} - for name in ("core", "search", "graphql"): - item = resources.get(name) - if not isinstance(item, dict): - continue - reset = item.get("reset") - result[name] = { - "limit": int(item.get("limit") or 0), - "used": int(item.get("used") or 0), - "remaining": int(item.get("remaining") or 0), - "reset": int(reset) if reset is not None else None, - "reset_at": datetime.fromtimestamp( - int(reset), timezone.utc).isoformat() - if reset is not None else None, - } - if result: - lease = getattr(_scan_lease_context, "lease", None) - if isinstance(lease, _GitHubScanLease): - if not db.record_pipeline_github_rate_snapshot( - lease.scope, result, observed_at=time.time(), - scan_lease_guard=lease.guard): - raise _GitHubScanLeaseLost( - "SQLite lease rejected GitHub rate snapshot") - return result or None + profiles = _profiles() + except Exception as exc: + raise _GitHubRateLimitUnavailable( + "pipeline profiles could not be read for identity validation") from exc + if not isinstance(profiles, dict): + raise _GitHubRateLimitUnavailable( + "pipeline profiles are invalid for identity validation") + accounts = set() + for profile in profiles.values(): + if not isinstance(profile, dict): + continue + budget = profile.get("github_budget") + if budget is None or budget is False: + continue + if isinstance(budget, dict) and budget.get("enabled", True) is False: + continue + control = profile.get("priority_control") + trusted = control.get("trusted_account") \ + if isinstance(control, dict) else None + if trusted is None: + continue + if not _valid_github_login(trusted): + raise _GitHubRateLimitUnavailable( + "configured trusted GitHub account is invalid") + accounts.add(trusted) + if len({account.casefold() for account in accounts}) > 1: + raise _GitHubRateLimitUnavailable( + "pipeline profiles configure different trusted GitHub accounts") + return accounts + + +def _rate_limit_integer(item: dict, field: str, source: str) -> int: + value = item.get(field) + if type(value) is not int or value < 0: + raise _GitHubRateLimitUnavailable( + f"GitHub {source} rate limit response is invalid") + return value + + +def _rest_rate_limit_resource(payload: dict, name: str) -> dict: + try: + item = payload["resources"][name] + except (KeyError, TypeError) as exc: + raise _GitHubRateLimitUnavailable( + f"GitHub REST /rate_limit response is invalid for {name}") from exc + if not isinstance(item, dict): + raise _GitHubRateLimitUnavailable( + f"GitHub REST /rate_limit response is invalid for {name}") + limit = _rate_limit_integer(item, "limit", f"REST {name}") + used = _rate_limit_integer(item, "used", f"REST {name}") + remaining = _rate_limit_integer(item, "remaining", f"REST {name}") + reset = _rate_limit_integer(item, "reset", f"REST {name}") + if used > limit or remaining > limit: + raise _GitHubRateLimitUnavailable( + f"GitHub REST /rate_limit response is invalid for {name}") + try: + reset_at = datetime.fromtimestamp(reset, timezone.utc).isoformat() + except (OverflowError, OSError, ValueError) as exc: + raise _GitHubRateLimitUnavailable( + f"GitHub REST /rate_limit response is invalid for {name}") from exc + return { + "limit": limit, "used": used, "remaining": remaining, + "reset": reset, "reset_at": reset_at, + } + + +def _graphql_rate_limit_resource(payload: dict) -> dict: + if not isinstance(payload, dict) or payload.get("errors"): + raise _GitHubRateLimitUnavailable( + "GitHub GraphQL rateLimit response is invalid") + try: + data = payload["data"] + item = data["rateLimit"] + login = data["viewer"]["login"] + except (KeyError, TypeError) as exc: + raise _GitHubRateLimitUnavailable( + "GitHub GraphQL rateLimit or viewer response is invalid") from exc + if not isinstance(item, dict): + raise _GitHubRateLimitUnavailable( + "GitHub GraphQL rateLimit response is invalid") + if not _valid_github_login(login): + raise _GitHubRateLimitUnavailable( + "GitHub GraphQL viewer response is invalid") + trusted_accounts = _trusted_github_accounts() + if trusted_accounts: + expected = next(iter(trusted_accounts)) + if login.casefold() != expected.casefold(): + raise _GitHubRateLimitUnavailable( + f"GitHub identity mismatch: authenticated as {login}, " + f"expected {expected}") + limit = _rate_limit_integer(item, "limit", "GraphQL") + used = _rate_limit_integer(item, "used", "GraphQL") + remaining = _rate_limit_integer(item, "remaining", "GraphQL") + if used > limit or remaining > limit: + raise _GitHubRateLimitUnavailable( + "GitHub GraphQL rateLimit response is invalid") + reset_at = item.get("resetAt") + if not isinstance(reset_at, str): + raise _GitHubRateLimitUnavailable( + "GitHub GraphQL rateLimit resetAt is invalid") + try: + reset_time = datetime.fromisoformat(reset_at.replace("Z", "+00:00")) + if reset_time.tzinfo is None: + raise ValueError("timezone is missing") + reset = int(reset_time.timestamp()) + normalized_reset = reset_time.astimezone(timezone.utc).isoformat() + except (OverflowError, OSError, TypeError, ValueError) as exc: + raise _GitHubRateLimitUnavailable( + "GitHub GraphQL rateLimit resetAt is invalid") from exc + if reset < 0: + raise _GitHubRateLimitUnavailable( + "GitHub GraphQL rateLimit resetAt is invalid") + return { + "limit": limit, "used": used, "remaining": remaining, + "reset": reset, "reset_at": normalized_reset, + } + + +def _github_rate_limits() -> dict: + """Read REST core/search and the authenticated GraphQL budget/identity. + + ``GET /rate_limit`` does not spend primary REST quota. The small GraphQL + query is necessary because the REST endpoint can report a stale/default + GraphQL bucket; its returned ``rateLimit`` includes that query's cost. + """ + try: + rest_payload = _gh_api_json(["rate_limit"]) except _GitHubScanLeaseFailure: raise - except (RuntimeError, OSError, OverflowError, TypeError, ValueError, - sqlite3.Error, json.JSONDecodeError): - return None + except Exception as exc: + raise _GitHubRateLimitUnavailable( + "GitHub REST /rate_limit request failed") from exc + core = _rest_rate_limit_resource(rest_payload, "core") + search = _rest_rate_limit_resource(rest_payload, "search") + try: + graphql_payload = _gh_api_json( + ["graphql"], {"query": _GITHUB_RATE_LIMIT_QUERY}) + except _GitHubScanLeaseFailure: + raise + except Exception as exc: + raise _GitHubRateLimitUnavailable( + "GitHub GraphQL rateLimit request failed") from exc + + result = { + "core": core, + "search": search, + "graphql": _graphql_rate_limit_resource(graphql_payload), + } + lease = getattr(_scan_lease_context, "lease", None) + if isinstance(lease, _GitHubScanLease): + try: + stored = db.record_pipeline_github_rate_snapshot( + lease.scope, result, observed_at=time.time(), + scan_lease_guard=lease.guard) + except (OSError, sqlite3.Error, TypeError, ValueError) as exc: + raise _GitHubRateLimitUnavailable( + "GitHub rate limit snapshot could not be stored") from exc + if not stored: + raise _GitHubScanLeaseLost( + "SQLite lease rejected GitHub rate snapshot") + return result def set_item_priority(profile_id: str, queue_id: str, kind: str, number: int, @@ -3465,6 +3654,11 @@ def analyze(profile_id: str, series: list[dict], *, use_cache: bool = True, profile_id, profile, series, _replace_admission_with_lease_failure( admission, profile, exc)) + except _GitHubRateLimitUnavailable as exc: + return _budget_blocked_cached( + profile_id, profile, series, + _replace_admission_with_rate_limit_failure( + admission, profile, exc)) if admission.get("enabled"): result["github_budget"] = _public_budget_decision(admission) cache = result.get("cache") diff --git a/tests/test_github_budget.py b/tests/test_github_budget.py index df76556..68efebd 100644 --- a/tests/test_github_budget.py +++ b/tests/test_github_budget.py @@ -1949,7 +1949,110 @@ def test_malformed_rate_limit_response_is_treated_as_unavailable(monkeypatch): }, }) - assert pipeline_insights._github_rate_limits() is None + with pytest.raises( + pipeline_insights._GitHubRateLimitUnavailable, + match="GitHub REST core rate limit response is invalid"): + pipeline_insights._github_rate_limits() + + +def test_graphql_rate_limit_errors_fail_closed_without_leaking_response( + monkeypatch): + secret = "ghp_not-for-diagnostics" + + def fake_api(args, _input_value=None): + if args == ["rate_limit"]: + return { + "resources": { + "core": {"limit": 5000, "used": 1, + "remaining": 4999, "reset": 2000}, + "search": {"limit": 30, "used": 0, + "remaining": 30, "reset": 2000}, + }, + } + return {"errors": [{"message": f"authorization failed: {secret}"}]} + + monkeypatch.setattr(pipeline_insights, "_gh_api_json", fake_api) + + with pytest.raises( + pipeline_insights._GitHubRateLimitUnavailable) as caught: + pipeline_insights._github_rate_limits() + + assert str(caught.value) == \ + "GitHub GraphQL rateLimit response is invalid" + assert secret not in str(caught.value) + + +def test_graphql_rate_limit_rejects_invalid_reset_at(monkeypatch): + def fake_api(args, _input_value=None): + if args == ["rate_limit"]: + return { + "resources": { + "core": {"limit": 5000, "used": 1, + "remaining": 4999, "reset": 2000}, + "search": {"limit": 30, "used": 0, + "remaining": 30, "reset": 2000}, + }, + } + return { + "data": { + "viewer": {"login": "owner"}, + "rateLimit": { + "limit": 5000, "used": 1, "remaining": 4999, + "resetAt": "not-a-timestamp", + }, + }, + } + + monkeypatch.setattr(pipeline_insights, "_gh_api_json", fake_api) + monkeypatch.setattr(pipeline_insights, "_profiles", lambda: {}) + + with pytest.raises( + pipeline_insights._GitHubRateLimitUnavailable, + match="GitHub GraphQL rateLimit resetAt is invalid"): + pipeline_insights._github_rate_limits() + + +def test_graphql_viewer_mismatch_blocks_scan_with_clear_diagnostic( + isolated_db, monkeypatch): + profile = _profile() + profile["priority_control"] = {"trusted_account": "expected-owner"} + monkeypatch.setattr( + pipeline_insights, "_profiles", lambda: {"example": profile}) + + def fake_api(args, _input_value=None): + if args == ["rate_limit"]: + return { + "resources": { + "core": {"limit": 5000, "used": 1, + "remaining": 4999, "reset": 2000}, + "search": {"limit": 30, "used": 0, + "remaining": 30, "reset": 2000}, + }, + } + return { + "data": { + "viewer": {"login": "other-owner"}, + "rateLimit": { + "limit": 5000, "used": 1, "remaining": 4999, + "resetAt": "2030-01-01T00:00:00Z", + }, + }, + } + + def forbidden(*_args, **_kwargs): + raise AssertionError("identity mismatch started a GitHub scan") + + monkeypatch.setattr(pipeline_insights, "_gh_api_json", fake_api) + monkeypatch.setattr(pipeline_insights, "_run_profile_health_check", forbidden) + monkeypatch.setattr(pipeline_insights, "_github_search", forbidden) + + result = pipeline_insights.analyze("example", [], use_cache=False) + + assert result["cache"]["refresh_blocked"] == "rate_limit_unavailable" + assert result["cache"]["refresh_blocked_reason"] == ( + "GitHub API budget unavailable: GitHub identity mismatch: " + "authenticated as other-owner, expected expected-owner") + assert result["generated_at"] is None def test_scan_exception_releases_lease(isolated_db, monkeypatch): diff --git a/tests/test_schedule_series.py b/tests/test_schedule_series.py index dac4e07..d1c0ed8 100644 --- a/tests/test_schedule_series.py +++ b/tests/test_schedule_series.py @@ -424,13 +424,35 @@ def test_health_check_failure_is_not_reported_as_broken_invariant(monkeypatch): def test_github_rate_limits_are_normalized(monkeypatch, no_live_github_rate_limit): - monkeypatch.setattr(pipeline_insights, "_gh_api_json", lambda _args: { - "resources": { - "core": {"limit": 5000, "used": 125, "remaining": 4875, "reset": 1}, - "search": {"limit": 30, "used": 2, "remaining": 28, "reset": 2}, - "graphql": {"limit": 5000, "used": 0, "remaining": 5000, "reset": 3}, - }, - }) + calls = [] + + def fake_api(args, input_value=None): + calls.append((args, input_value)) + if args == ["rate_limit"]: + return { + "resources": { + "core": {"limit": 5000, "used": 125, + "remaining": 4875, "reset": 1}, + "search": {"limit": 30, "used": 2, + "remaining": 28, "reset": 2}, + # This endpoint can report a different GraphQL bucket. + "graphql": {"limit": 5000, "used": 0, + "remaining": 5000, "reset": 3}, + }, + } + assert args == ["graphql"] + return { + "data": { + "viewer": {"login": "owner"}, + "rateLimit": { + "limit": 5000, "used": 679, "remaining": 4321, + "resetAt": "1970-01-01T00:00:04Z", + }, + }, + } + + monkeypatch.setattr(pipeline_insights, "_gh_api_json", fake_api) + monkeypatch.setattr(pipeline_insights, "_profiles", lambda: {}) limits = no_live_github_rate_limit() @@ -438,6 +460,11 @@ def test_github_rate_limits_are_normalized(monkeypatch, no_live_github_rate_limi assert limits["core"]["reset_at"] == "1970-01-01T00:00:01+00:00" assert limits["search"]["used"] == 2 assert limits["graphql"]["limit"] == 5000 + assert limits["graphql"]["remaining"] == 4321 + assert limits["graphql"]["reset"] == 4 + assert calls[1][0] == ["graphql"] + assert "viewer" in calls[1][1]["query"] + assert "rateLimit" in calls[1][1]["query"] def test_project_health_attention_overrides_warming_history(): From e1d503d618640cb1d341390c47466dbd40bec46e Mon Sep 17 00:00:00 2001 From: ibrog Date: Thu, 17 Sep 2026 15:12:12 +0300 Subject: [PATCH 2/2] fix(pipeline): validate rate bucket consistency Generated-with: Codex --- promptpilot/pipeline_insights.py | 17 ++++++++++--- tests/test_github_budget.py | 42 ++++++++++++++++++++++++++++++++ tests/test_workflow_api_cli.py | 5 ++++ 3 files changed, 61 insertions(+), 3 deletions(-) diff --git a/promptpilot/pipeline_insights.py b/promptpilot/pipeline_insights.py index b64ef6c..2919ff6 100644 --- a/promptpilot/pipeline_insights.py +++ b/promptpilot/pipeline_insights.py @@ -1391,7 +1391,10 @@ def _rest_rate_limit_resource(payload: dict, name: str) -> dict: used = _rate_limit_integer(item, "used", f"REST {name}") remaining = _rate_limit_integer(item, "remaining", f"REST {name}") reset = _rate_limit_integer(item, "reset", f"REST {name}") - if used > limit or remaining > limit: + # A smaller sum is conservative if GitHub adjusts quota mid-window. A + # larger sum claims overlapping spent and available quota, so ``remaining`` + # is not safe enough for admission. + if used + remaining > limit: raise _GitHubRateLimitUnavailable( f"GitHub REST /rate_limit response is invalid for {name}") try: @@ -1432,7 +1435,7 @@ def _graphql_rate_limit_resource(payload: dict) -> dict: limit = _rate_limit_integer(item, "limit", "GraphQL") used = _rate_limit_integer(item, "used", "GraphQL") remaining = _rate_limit_integer(item, "remaining", "GraphQL") - if used > limit or remaining > limit: + if used + remaining > limit: raise _GitHubRateLimitUnavailable( "GitHub GraphQL rateLimit response is invalid") reset_at = item.get("resetAt") @@ -3555,7 +3558,15 @@ def _analyze_without_budget(profile_id: str, series: list[dict], *, github_rate_limit = (cached[1].get("github_rate_limit") if cached else None) else: - github_rate_limit = _github_rate_limits() + try: + github_rate_limit = _github_rate_limits() + except _GitHubRateLimitUnavailable: + # Without an admission policy this is optional UI telemetry, + # not a safety gate. Budget-enabled profiles still propagate + # the error to analyze(), which records a fail-closed denial. + if _github_budget_policy(profile) is not None: + raise + github_rate_limit = None result = { "profile_id": profile_id, "title": profile["title"], "repository": profile["repository"], "queues": queues, diff --git a/tests/test_github_budget.py b/tests/test_github_budget.py index 68efebd..a28044e 100644 --- a/tests/test_github_budget.py +++ b/tests/test_github_budget.py @@ -2012,6 +2012,48 @@ def fake_api(args, _input_value=None): pipeline_insights._github_rate_limits() +@pytest.mark.parametrize("inconsistent_source", ["rest", "graphql"]) +def test_rate_limit_rejects_overlapping_used_and_remaining( + monkeypatch, inconsistent_source): + def fake_api(args, _input_value=None): + if args == ["rate_limit"]: + core = { + "limit": 5000, "used": 100, "remaining": 4900, + "reset": 2000, + } + if inconsistent_source == "rest": + core.update({"used": 4900, "remaining": 4900}) + return { + "resources": { + "core": core, + "search": {"limit": 30, "used": 0, + "remaining": 30, "reset": 2000}, + }, + } + graphql = { + "limit": 5000, "used": 100, "remaining": 4900, + "resetAt": "2030-01-01T00:00:00Z", + } + if inconsistent_source == "graphql": + graphql.update({"used": 4900, "remaining": 4900}) + return { + "data": { + "viewer": {"login": "owner"}, + "rateLimit": graphql, + }, + } + + monkeypatch.setattr(pipeline_insights, "_gh_api_json", fake_api) + monkeypatch.setattr(pipeline_insights, "_profiles", lambda: {}) + + with pytest.raises( + pipeline_insights._GitHubRateLimitUnavailable, + match=("GitHub REST /rate_limit response is invalid for core" + if inconsistent_source == "rest" + else "GitHub GraphQL rateLimit response is invalid")): + pipeline_insights._github_rate_limits() + + def test_graphql_viewer_mismatch_blocks_scan_with_clear_diagnostic( isolated_db, monkeypatch): profile = _profile() diff --git a/tests/test_workflow_api_cli.py b/tests/test_workflow_api_cli.py index d63067d..3423906 100644 --- a/tests/test_workflow_api_cli.py +++ b/tests/test_workflow_api_cli.py @@ -170,6 +170,11 @@ def test_ordinary_insights_api_does_not_enter_refresh_handler_after_restart( "count": 1, "items": [], "membership_complete": False, }) monkeypatch.setattr(pipeline_insights, "_run_profile_health_check", lambda _profile: None) + monkeypatch.setattr( + pipeline_insights, "_github_rate_limits", + lambda: (_ for _ in ()).throw( + pipeline_insights._GitHubRateLimitUnavailable( + "GitHub CLI is not authenticated"))) assert request( "GET", "/api/pipeline-insights/restart-api?refresh=true").status_code == 200 pipeline_insights._cache.clear()