From bc8b32f2bc9e4ae064f41258ec3ce720e6489fa9 Mon Sep 17 00:00:00 2001 From: xeonvs <11463419+xeonvs@users.noreply.github.com> Date: Mon, 24 Aug 2026 20:17:48 +0200 Subject: [PATCH 1/8] Plan v0.8.1 provider failure handling --- PLANS.md | 37 ++++++++++++++++++++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-) diff --git a/PLANS.md b/PLANS.md index ca171a2..6955220 100644 --- a/PLANS.md +++ b/PLANS.md @@ -4,4 +4,39 @@ Use this file for active or blocked repository work. Update it before implementa ## Active Work -No active or blocked repository work. +### Release 0.8.1: completion cap and safe LLM provider failures + +Status: active, `release-required`. Target stable version: `0.8.1`. + +#### Outcome and scope + +- Add optional `OCR_LLM_MAX_COMPLETION_TOKENS`; its exact default is unset and therefore inherits the qualified OCR behavior. It controls only the per-request completion/output cap through the protocol-specific `llm.extra_body` key and does not change OCR prompt/context `max_tokens`, `OCR_MAX_TOKENS_BUDGET`, receipt v5, DLP, telemetry, severity, findings, or approval contracts. +- Canonicalize provider URL, protocol, headers, auxiliary `/models` URL, and request-body controls in one provider-neutral runtime owner shared by `configure` and `preflight`. +- Project private OCR retry diagnostics into a closed provider-neutral failure reason and toolkit-authored GitLab guidance. Raw provider bodies, messages, codes, URLs, models, request IDs, paths, warnings, credentials, and stderr remain private. +- Keep OCR `1.9.10` as the exact qualified dependency. Do not promote unreleased upstream defaults or derive a completion cap from `/models` metadata. + +#### Trust and data flow + +1. Operator environment enters a single provider-config parser. It accepts an explicit closed protocol, bounded positive decimal completion cap, valid header objects, and absolute HTTPS URLs without credentials or fragments. The parser normalizes API root and terminal inference endpoint, preserves an unambiguous query for inference, and requires explicit `OCR_LLM_MODELS_URL` when an auxiliary URL cannot be derived safely. +2. `configure` projects only the validated inference settings into the private OCR configuration. A completion cap maps to `max_completion_tokens` for `openai`, `max_output_tokens` for `openai-responses`, and `max_tokens` for `anthropic`. An equal value already present in `OCR_LLM_EXTRA_BODY` is deduplicated; a different value fails closed. +3. `preflight` consumes the same canonical configuration and derives `/models` only from its normalized API root. Explicit protocol remains authoritative; a terminal endpoint for another protocol is rejected rather than changing protocol implicitly. +4. On a non-zero OCR exit, posting hostile-reads the bounded result artifact only for `ocr.llm-retry-report/v1`. A strict parser admits only allowlisted error class, failure phase, terminal outcome, and HTTP status into a closed reason. Normal findings and every raw/provider-controlled field are ignored. +5. The GitLab note is generated entirely from toolkit-owned static text. Classified failures suppress `OCR_POST_ERROR_DETAILS`; malformed, oversized, missing, or ambiguous diagnostics retain the existing generic note. Previous successful review notes remain, no normal findings are posted, and auto-approval is unreachable. + +#### Logical slices and commit gates + +1. **Plan and coordination.** Create `codex/v0.8.1-provider-failures`, this planning commit, milestone `v0.8.1`, a completion-cap issue linked with #129, updated #129 acceptance criteria, and a Draft PR. Before commit: plan/self-review, requirement and boundary mapping, `git diff --check`. +2. **Completion-cap contract.** Implement parsing, protocol mapping, conflict behavior, environment/generated-config/installed-artifact tests, and an exact checksum-verified OCR 1.9.10 no-LLM wire probe proving inherited `58888` and explicit `4096`. If the probe does not prove the override, omit the public setting and continue only the diagnostics work. Before commit: focused tests, full diff and trust review, `git diff --check`. +3. **Canonical provider configuration.** Share one provider-neutral owner between configure and preflight; cover API roots, terminal endpoints, trailing slash, query, credentials, fragments, protocol mismatch, and explicit models URL. Before commit: focused tests, URL/header/data-flow review, `git diff --check`. +4. **Failure projection and GitLab.** Add the bounded retry-report parser, closed reason mapping, static hints, and one renderer for non-zero retry reports and existing successful-result billing/quota warnings. Cover HTTP 400/401/402/403/404/408/409/413/422/429/5xx/529, timeout, network, decode, mixed, malformed, oversized, raw-data absence, previous-review preservation, no findings/approval, and strict/advisory behavior. Before commit: focused tests, privacy/approval/rollback review, `git diff --check`. +5. **Documentation and release handoff.** Document exact defaults, mappings, conflicts, the `4096` workaround, the three distinct token ceilings, possible provider cost reservation, and the limits of `/models`; add separate feature and bug-fix Towncrier fragments. Reconcile strategy/roadmap only where the implemented outcome changes them. Before commit: documentation/version consistency, Towncrier draft, full diff review, `git diff --check`. + +#### Validation and delivery + +- Focused contract, configuration, preflight, result, posting, approval, environment, installed-artifact, and compatibility tests. +- Full `scripts/quality.sh check` including combined and risk-group coverage floors; `PYTHONPATH=src python scripts/ocr_compat.py validate`; lock/manifest tests; `scripts/gitleaks.sh`; Towncrier draft; reproducible wheel/sdist, Twine, and clean installs on Python 3.12-3.14. +- Overall requirements, provider-boundary, privacy, DLP/approval, telemetry, rollback, and documentation self-review before push. +- Push the complete feature history to the Draft PR, wait for hosted checks, address evidence-driven failures through the same commit gate, then mark ready and merge through protected review. +- Verify the deterministic TestPyPI development build, then prepare and merge the protected `Release v0.8.1` PR. Monitor stable TestPyPI/PyPI publication, tag, immutable GitHub Release, provenance, attestations, supported-Python installs, and immutable receipt; close tracked issues only after independent external reconciliation. + +Resume point: create the feature branch and signed planning commit, then perform the exact OCR 1.9.10 wire probe before exposing `OCR_LLM_MAX_COMPLETION_TOKENS`. From 54d2f619992f87bf78948f449560112f41bdda1f Mon Sep 17 00:00:00 2001 From: xeonvs <11463419+xeonvs@users.noreply.github.com> Date: Mon, 24 Aug 2026 20:25:29 +0200 Subject: [PATCH 2/8] Add protocol-aware LLM completion cap --- PLANS.md | 8 +- docs/configuration.md | 1 + scripts/ocr_compat.py | 70 +++++++++++++ src/ocr_toolkit/configure.py | 108 +++----------------- src/ocr_toolkit/provider_config.py | 152 +++++++++++++++++++++++++++++ tests/test_environment_contract.py | 1 + tests/test_installed_policy_e2e.py | 27 ++++- tests/test_runtime_helpers.py | 71 ++++++++++++++ 8 files changed, 340 insertions(+), 98 deletions(-) create mode 100644 src/ocr_toolkit/provider_config.py diff --git a/PLANS.md b/PLANS.md index 6955220..9086b90 100644 --- a/PLANS.md +++ b/PLANS.md @@ -39,4 +39,10 @@ Status: active, `release-required`. Target stable version: `0.8.1`. - Push the complete feature history to the Draft PR, wait for hosted checks, address evidence-driven failures through the same commit gate, then mark ready and merge through protected review. - Verify the deterministic TestPyPI development build, then prepare and merge the protected `Release v0.8.1` PR. Monitor stable TestPyPI/PyPI publication, tag, immutable GitHub Release, provenance, attestations, supported-Python installs, and immutable receipt; close tracked issues only after independent external reconciliation. -Resume point: create the feature branch and signed planning commit, then perform the exact OCR 1.9.10 wire probe before exposing `OCR_LLM_MAX_COMPLETION_TOKENS`. +Resume point: complete canonical provider URL/configuration ownership, then implement the private retry-report failure projection. + +#### Current implementation evidence + +- Coordination: milestone `v0.8.1`, completion-cap issue #130, provider-diagnostics issue #129, and Draft PR #131 are open. +- Exact OCR 1.9.10 Darwin arm64 asset SHA-256 `c626347bafcdbf25cf058af403d16568a3a9ffa1814046ff7c9d1e6becaf60d2` was verified before execution. The isolated production-config-path probe observed `max_completion_tokens=58888` when unset and `max_completion_tokens=4096` when explicitly configured; all temporary binary, config, repository, HOME, and receipt paths were removed. +- Completion-cap parsing, protocol mapping, collision rules, environment defaults, generated config, wheel/sdist installed paths, and the reusable exact wire probe are implemented and focused-green. Resume with canonical provider URL/configuration ownership. diff --git a/docs/configuration.md b/docs/configuration.md index d430271..e79cb97 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -15,6 +15,7 @@ These are the complete supported toolkit-owned runtime inputs. `Required` is sco | `OCR_LLM_AUTH_HEADER` | Operator / configure and preflight | No | `Authorization` | Valid HTTP header name used for the bearer credential. | | `OCR_LLM_EXTRA_HEADERS` | Operator / configure and preflight | No | Empty object | JSON object of additional string headers; cannot duplicate the auth header. | | `OCR_LLM_EXTRA_BODY` | Operator / `ocr-ci configure` | No | Unset | JSON object merged into the OCR LLM request configuration. | +| `OCR_LLM_MAX_COMPLETION_TOKENS` | Operator / `ocr-ci configure` | No | Unset (inherits OCR) | Positive decimal integer from `1` through `1000000`; sets the protocol-specific completion/output cap without changing prompt/context or aggregate review budgets. | | `OCR_ANTHROPIC_DISABLE_THINKING` | Operator / `ocr-ci configure` | No | `false` | With the Anthropic protocol, exact `true` adds `thinking.type=disabled`. | | `OCR_REVIEW_LANGUAGE` | Operator / shared language resolver | No | `English` | Allowed language label or BCP-47 tag used for the review. | | `OCR_LLM_VALIDATE_MODEL` | Operator / `ocr-ci preflight` | No | `false` | `true` validates through `/models`; `auto` may use the offline allowlist; false values skip validation. | diff --git a/scripts/ocr_compat.py b/scripts/ocr_compat.py index e3cb6fd..c4b547b 100644 --- a/scripts/ocr_compat.py +++ b/scripts/ocr_compat.py @@ -614,6 +614,7 @@ class _StubHandler(http.server.BaseHTTPRequestHandler): request_count = 0 tokens_per_request = 2 + completion_caps: list[object] = [] def do_POST(self) -> None: if self.path != "/v1/chat/completions": @@ -631,6 +632,7 @@ def do_POST(self) -> None: if not isinstance(request, dict): self.send_error(400) return + type(self).completion_caps.append(request.get("max_completion_tokens")) messages = request.get("messages") if not isinstance(messages, list): self.send_error(400) @@ -731,6 +733,7 @@ def _stub_gateway(*, tokens_per_request: int = 2) -> Iterator[str]: _fail("stub gateway token usage must be at least two") _StubHandler.request_count = 0 _StubHandler.tokens_per_request = tokens_per_request + _StubHandler.completion_caps = [] server = http.server.ThreadingHTTPServer(("127.0.0.1", 0), _StubHandler) thread = threading.Thread(target=server.serve_forever, daemon=True) thread.start() @@ -864,6 +867,71 @@ def _budget_result_probe(binary: Path, directory: Path) -> dict[str, object]: } +def _completion_cap_probe(binary: Path, directory: Path) -> dict[str, object]: + """Observe the real OCR chat-completions output cap with and without an override.""" + + probe_root = directory / "completion-cap-probe" + probe_root.mkdir() + git_env = _isolated_probe_environment(probe_root / "git-home") + repo, base, head = _synthetic_repo(probe_root, git_env) + observed: dict[str, int] = {} + for label, expected in (("inherited", 58_888), ("explicit", 4_096)): + env = _isolated_probe_environment(probe_root / f"{label}-home") + with _stub_gateway() as gateway_url: + env.update( + { + "OCR_LLM_URL": gateway_url, + "OCR_LLM_TOKEN": "synthetic-token", + "OCR_LLM_MODEL": "synthetic-model", + "OCR_LLM_PROTOCOL": "openai", + "OCR_TELEMETRY_ENABLED": "false", + } + ) + from ocr_toolkit.config_writer import write_ocr_config + + llm_config: dict[str, object] = { + "auth_token": "synthetic-token", + "model": "synthetic-model", + "protocol": "openai", + "url": gateway_url, + "use_anthropic": False, + } + if label == "explicit": + llm_config["extra_body"] = {"max_completion_tokens": expected} + write_ocr_config( + {"llm": llm_config, "telemetry": {"enabled": False}}, + Path(env["HOME"]) / ".opencodereview" / "config.json", + ) + _run( + [ + str(binary), + "review", + "--from", + base, + "--to", + head, + "--format", + "json", + "--audience", + "agent", + "--concurrency", + "1", + ], + cwd=repo, + env=env, + ) + caps = _StubHandler.completion_caps + if not caps or any(value != expected for value in caps): + _fail(f"OCR completion-cap {label} probe expected {expected}, observed {caps!r}") + observed[label] = expected + return { + "explicit": observed["explicit"], + "inherited": observed["inherited"], + "result": "passed", + "wire_field": "max_completion_tokens", + } + + def _preview_file_selection(payload: dict[str, Any] | str, path: str) -> tuple[bool, object]: """Return one preview file's selected state and closed exclusion reason.""" @@ -1110,6 +1178,8 @@ def run_contracts(binary: Path, version: str, directory: Path) -> dict[str, Any] "result": "passed", }, } + if _version(version) >= (1, 9, 10): + contracts["completion_cap_probe"] = _completion_cap_probe(binary, directory) if thinking_probe is not None: contracts["comment_thinking_probe"] = thinking_probe return contracts diff --git a/src/ocr_toolkit/configure.py b/src/ocr_toolkit/configure.py index 3816e36..af69542 100644 --- a/src/ocr_toolkit/configure.py +++ b/src/ocr_toolkit/configure.py @@ -2,9 +2,7 @@ from __future__ import annotations -import json import os -import re import sys from typing import Any from urllib.parse import urlsplit @@ -12,9 +10,10 @@ from ocr_toolkit.common.language import resolve_review_language from ocr_toolkit.common.redaction import redact_sensitive from ocr_toolkit.config_writer import OCRConfigError, update_ocr_config - -HEADER_NAME_RE = re.compile(r"^[!#$%&'*+.^_`|~0-9A-Za-z-]+$") -LLM_PROTOCOLS = {"anthropic", "openai", "openai-responses"} +from ocr_toolkit.provider_config import ( + ProviderConfigError, + request_controls_from_environment, +) class OCRRuntimeConfigError(Exception): @@ -36,79 +35,6 @@ def _required_env(name: str) -> str: return value -def _parse_extra_headers(value: str) -> dict[str, str]: - if not value: - return {} - try: - parsed = json.loads(value) - except json.JSONDecodeError as exc: - raise OCRRuntimeConfigError("OCR_LLM_EXTRA_HEADERS must be a JSON object") from exc - if not isinstance(parsed, dict): - raise OCRRuntimeConfigError("OCR_LLM_EXTRA_HEADERS must be a JSON object") - headers: dict[str, str] = {} - for key, raw_value in parsed.items(): - if not isinstance(key, str) or not HEADER_NAME_RE.fullmatch(key): - raise OCRRuntimeConfigError( - "OCR_LLM_EXTRA_HEADERS contains an invalid HTTP header name" - ) - if not isinstance(raw_value, str): - raise OCRRuntimeConfigError("OCR_LLM_EXTRA_HEADERS contains a non-string header value") - if "\n" in raw_value or "\r" in raw_value: - raise OCRRuntimeConfigError( - "OCR_LLM_EXTRA_HEADERS contains a header value with a line break" - ) - headers[key] = raw_value - return headers - - -def _parse_extra_body(value: str) -> Any: - if not value: - return None - try: - parsed = json.loads(value) - except json.JSONDecodeError as exc: - raise OCRRuntimeConfigError("OCR_LLM_EXTRA_BODY must be valid JSON") from exc - if not isinstance(parsed, dict): - raise OCRRuntimeConfigError("OCR_LLM_EXTRA_BODY must be a JSON object") - return parsed - - -def _llm_protocol() -> str: - """Resolve and validate the explicit OCR LLM wire protocol.""" - - if "OCR_USE_ANTHROPIC" in os.environ: - raise OCRRuntimeConfigError( - "OCR_USE_ANTHROPIC was removed; set OCR_LLM_PROTOCOL=anthropic explicitly" - ) - protocol = _env("OCR_LLM_PROTOCOL", "openai") or "openai" - if protocol not in LLM_PROTOCOLS: - allowed = ", ".join(sorted(LLM_PROTOCOLS)) - raise OCRRuntimeConfigError(f"OCR_LLM_PROTOCOL must be one of: {allowed}") - return protocol - - -def _llm_extra_body(protocol: str) -> dict[str, Any] | None: - """Return explicit OCR LLM extra body with safe Anthropic defaults merged.""" - - raw_env = os.environ.get("OCR_LLM_EXTRA_BODY") - raw = raw_env.strip() if raw_env is not None else "" - explicit_object = bool(raw) - extra_body = _parse_extra_body(raw) - if extra_body is None: - extra_body = {} - - if protocol == "anthropic" and _bool_env("OCR_ANTHROPIC_DISABLE_THINKING"): - existing = extra_body.get("thinking") - disabled = {"type": "disabled"} - if existing is not None and existing != disabled: - raise OCRRuntimeConfigError( - "OCR_ANTHROPIC_DISABLE_THINKING conflicts with OCR_LLM_EXTRA_BODY.thinking" - ) - extra_body["thinking"] = disabled - - return extra_body if explicit_object or extra_body else None - - def build_config_updates() -> dict[str, Any]: """Build OCR config updates from already-normalized CI environment.""" @@ -136,32 +62,28 @@ def build_config_updates() -> dict[str, Any]: "OCR_LLM_URL must be an absolute HTTPS URL without embedded credentials" ) llm_model = _required_env("OCR_LLM_MODEL") - llm_protocol = _llm_protocol() - auth_header = _env("OCR_LLM_AUTH_HEADER", "Authorization") or "Authorization" - if not HEADER_NAME_RE.fullmatch(auth_header): - raise OCRRuntimeConfigError("OCR_LLM_AUTH_HEADER is not a valid HTTP header name") + try: + request_controls = request_controls_from_environment() + except ProviderConfigError as exc: + raise OCRRuntimeConfigError(str(exc)) from exc updates: dict[str, Any] = { "language": review_language, "llm.url": llm_url, "llm.auth_token": llm_token, "llm.model": llm_model, - "llm.protocol": llm_protocol, - "llm.use_anthropic": llm_protocol == "anthropic", - "llm.auth_header": auth_header, + "llm.protocol": request_controls.protocol, + "llm.use_anthropic": request_controls.protocol == "anthropic", + "llm.auth_header": request_controls.auth_header, "telemetry.enabled": _bool_env("OCR_TELEMETRY_ENABLED"), "telemetry.content_logging": _bool_env("OCR_TELEMETRY_CONTENT_LOGGING"), } - extra_headers = _parse_extra_headers(_env("OCR_LLM_EXTRA_HEADERS")) - if any(header.casefold() == auth_header.casefold() for header in extra_headers): - raise OCRRuntimeConfigError("OCR_LLM_EXTRA_HEADERS must not duplicate OCR_LLM_AUTH_HEADER") - if extra_headers: - updates["llm.extra_headers"] = extra_headers + if request_controls.extra_headers: + updates["llm.extra_headers"] = request_controls.extra_headers - extra_body = _llm_extra_body(llm_protocol) - if extra_body is not None: - updates["llm.extra_body"] = extra_body + if request_controls.extra_body is not None: + updates["llm.extra_body"] = request_controls.extra_body if _bool_env("OCR_TELEMETRY_ENABLED"): updates["telemetry.exporter"] = _env("OCR_TELEMETRY_EXPORTER") diff --git a/src/ocr_toolkit/provider_config.py b/src/ocr_toolkit/provider_config.py new file mode 100644 index 0000000..371f72d --- /dev/null +++ b/src/ocr_toolkit/provider_config.py @@ -0,0 +1,152 @@ +"""Own provider-neutral LLM request controls derived from the environment.""" + +from __future__ import annotations + +import json +import os +import re +from collections.abc import Mapping +from dataclasses import dataclass +from typing import Any + +HEADER_NAME_RE = re.compile(r"^[!#$%&'*+.^_`|~0-9A-Za-z-]+$") +POSITIVE_DECIMAL_RE = re.compile(r"^[1-9][0-9]*$") +LLM_PROTOCOLS = frozenset({"anthropic", "openai", "openai-responses"}) +MAX_COMPLETION_TOKENS_LIMIT = 1_000_000 +COMPLETION_TOKEN_FIELDS = { + "anthropic": "max_tokens", + "openai": "max_completion_tokens", + "openai-responses": "max_output_tokens", +} + + +class ProviderConfigError(Exception): + """Provider settings from the operator environment are invalid.""" + + +@dataclass(frozen=True) +class ProviderRequestControls: + """Validated protocol, headers, and request-body overlay for OCR.""" + + protocol: str + auth_header: str + extra_headers: dict[str, str] + extra_body: dict[str, Any] | None + + +def _env(environment: Mapping[str, str], name: str, default: str = "") -> str: + return environment.get(name, default).strip() + + +def _parse_protocol(environment: Mapping[str, str]) -> str: + """Return the explicit closed OCR wire protocol.""" + + if "OCR_USE_ANTHROPIC" in environment: + raise ProviderConfigError( + "OCR_USE_ANTHROPIC was removed; set OCR_LLM_PROTOCOL=anthropic explicitly" + ) + protocol = _env(environment, "OCR_LLM_PROTOCOL", "openai") or "openai" + if protocol not in LLM_PROTOCOLS: + allowed = ", ".join(sorted(LLM_PROTOCOLS)) + raise ProviderConfigError(f"OCR_LLM_PROTOCOL must be one of: {allowed}") + return protocol + + +def _parse_extra_headers(value: str) -> dict[str, str]: + """Parse the optional JSON header map without admitting line breaks.""" + + if not value: + return {} + try: + parsed = json.loads(value) + except json.JSONDecodeError as exc: + raise ProviderConfigError("OCR_LLM_EXTRA_HEADERS must be a JSON object") from exc + if not isinstance(parsed, dict): + raise ProviderConfigError("OCR_LLM_EXTRA_HEADERS must be a JSON object") + headers: dict[str, str] = {} + for key, raw_value in parsed.items(): + if not isinstance(key, str) or not HEADER_NAME_RE.fullmatch(key): + raise ProviderConfigError("OCR_LLM_EXTRA_HEADERS contains an invalid HTTP header name") + if not isinstance(raw_value, str): + raise ProviderConfigError("OCR_LLM_EXTRA_HEADERS contains a non-string header value") + if "\n" in raw_value or "\r" in raw_value: + raise ProviderConfigError( + "OCR_LLM_EXTRA_HEADERS contains a header value with a line break" + ) + headers[key] = raw_value + return headers + + +def _parse_extra_body(value: str) -> tuple[dict[str, Any], bool]: + """Parse the optional JSON request overlay and retain explicit emptiness.""" + + if not value: + return {}, False + try: + parsed = json.loads(value) + except json.JSONDecodeError as exc: + raise ProviderConfigError("OCR_LLM_EXTRA_BODY must be valid JSON") from exc + if not isinstance(parsed, dict): + raise ProviderConfigError("OCR_LLM_EXTRA_BODY must be a JSON object") + return parsed, True + + +def _parse_completion_cap(value: str) -> int | None: + """Parse the optional bounded positive completion-token cap.""" + + if not value: + return None + if POSITIVE_DECIMAL_RE.fullmatch(value) is None: + raise ProviderConfigError( + "OCR_LLM_MAX_COMPLETION_TOKENS must be a positive decimal integer" + ) + parsed = int(value) + if parsed > MAX_COMPLETION_TOKENS_LIMIT: + raise ProviderConfigError( + f"OCR_LLM_MAX_COMPLETION_TOKENS must be at most {MAX_COMPLETION_TOKENS_LIMIT}" + ) + return parsed + + +def request_controls_from_environment( + environment: Mapping[str, str] | None = None, +) -> ProviderRequestControls: + """Build validated OCR request controls from one environment snapshot.""" + + values = os.environ if environment is None else environment + protocol = _parse_protocol(values) + auth_header = _env(values, "OCR_LLM_AUTH_HEADER", "Authorization") or "Authorization" + if HEADER_NAME_RE.fullmatch(auth_header) is None: + raise ProviderConfigError("OCR_LLM_AUTH_HEADER is not a valid HTTP header name") + + extra_headers = _parse_extra_headers(_env(values, "OCR_LLM_EXTRA_HEADERS")) + if any(header.casefold() == auth_header.casefold() for header in extra_headers): + raise ProviderConfigError("OCR_LLM_EXTRA_HEADERS must not duplicate OCR_LLM_AUTH_HEADER") + + extra_body, explicit_body = _parse_extra_body(_env(values, "OCR_LLM_EXTRA_BODY")) + completion_cap = _parse_completion_cap(_env(values, "OCR_LLM_MAX_COMPLETION_TOKENS")) + if completion_cap is not None: + field = COMPLETION_TOKEN_FIELDS[protocol] + existing = extra_body.get(field) + if field in extra_body and (type(existing) is not int or existing != completion_cap): + raise ProviderConfigError( + f"OCR_LLM_MAX_COMPLETION_TOKENS conflicts with OCR_LLM_EXTRA_BODY.{field}; " + "remove one setting or make the integer values equal" + ) + extra_body[field] = completion_cap + + if protocol == "anthropic" and _env(values, "OCR_ANTHROPIC_DISABLE_THINKING").lower() == "true": + existing = extra_body.get("thinking") + disabled = {"type": "disabled"} + if existing is not None and existing != disabled: + raise ProviderConfigError( + "OCR_ANTHROPIC_DISABLE_THINKING conflicts with OCR_LLM_EXTRA_BODY.thinking" + ) + extra_body["thinking"] = disabled + + return ProviderRequestControls( + protocol=protocol, + auth_header=auth_header, + extra_headers=extra_headers, + extra_body=extra_body if explicit_body or extra_body else None, + ) diff --git a/tests/test_environment_contract.py b/tests/test_environment_contract.py index ccee26e..1eb8bdb 100644 --- a/tests/test_environment_contract.py +++ b/tests/test_environment_contract.py @@ -26,6 +26,7 @@ "OCR_LLM_AUTH_HEADER": "Authorization", "OCR_LLM_EXTRA_HEADERS": "Empty object", "OCR_LLM_EXTRA_BODY": "Unset", + "OCR_LLM_MAX_COMPLETION_TOKENS": "Unset (inherits OCR)", "OCR_ANTHROPIC_DISABLE_THINKING": "false", "OCR_REVIEW_LANGUAGE": "English", "OCR_LLM_VALIDATE_MODEL": "false", diff --git a/tests/test_installed_policy_e2e.py b/tests/test_installed_policy_e2e.py index 746e9c8..c029e3a 100644 --- a/tests/test_installed_policy_e2e.py +++ b/tests/test_installed_policy_e2e.py @@ -8,7 +8,6 @@ import subprocess import sys import tarfile -import venv from pathlib import Path import pytest @@ -111,20 +110,25 @@ def test_installed_wheel_and_sdist_expose_target_policy_through_real_mcp( """Prove both package paths under hostile imports, private state, and stdio MCP.""" git_binary = shutil.which("git") + uv_binary = shutil.which("uv") assert git_binary is not None + assert uv_binary is not None for label, artifact in zip(("wheel", "sdist"), installed_artifacts, strict=True): root = tmp_path / label root.mkdir(mode=0o700) environment = root / "venv" - venv.EnvBuilder(with_pip=True).create(environment) + _run( + [uv_binary, "venv", "--python", sys.executable, "--no-project", str(environment)], + cwd=root, + ) binary_directory = environment / ("Scripts" if os.name == "nt" else "bin") python = binary_directory / ("python.exe" if os.name == "nt" else "python") cli = binary_directory / ("ocr-ci.exe" if os.name == "nt" else "ocr-ci") _run( - [str(python), "-m", "pip", "install", "--no-deps", str(artifact)], + [uv_binary, "pip", "install", "--python", str(python), "--no-deps", str(artifact)], cwd=root, ) - _run([str(python), "-m", "pip", "check"], cwd=root) + _run([uv_binary, "pip", "check", "--python", str(python)], cwd=root) installed_version = _run( [str(python), "-I", "-c", "import ocr_toolkit; print(ocr_toolkit.__version__)"], cwd=root, @@ -143,6 +147,21 @@ def test_installed_wheel_and_sdist_expose_target_policy_through_real_mcp( env={"HOME": str(root), "PATH": str(binary_directory)}, ) assert "review" in help_text and "post" in help_text + config_home = root / "config-home" + config_environment = { + "HOME": str(config_home), + "OCR_LLM_MAX_COMPLETION_TOKENS": "4096", + "OCR_LLM_MODEL": "openai/gpt-test", + "OCR_LLM_PROTOCOL": "openai", + "OCR_LLM_TOKEN": "installed-test-token", + "OCR_LLM_URL": "https://gateway.example/v1/chat/completions", + "PATH": str(binary_directory), + } + _run([str(cli), "configure"], cwd=root, env=config_environment) + generated_config = json.loads( + (config_home / ".opencodereview" / "config.json").read_text(encoding="utf-8") + ) + assert generated_config["llm"]["extra_body"] == {"max_completion_tokens": 4096} protocol_environment = { "HOME": str(root / "home"), "PATH": os.pathsep.join( diff --git a/tests/test_runtime_helpers.py b/tests/test_runtime_helpers.py index efef713..b14a222 100644 --- a/tests/test_runtime_helpers.py +++ b/tests/test_runtime_helpers.py @@ -411,6 +411,77 @@ def test_runtime_config_preserves_explicit_empty_extra_body(self) -> None: self.assertEqual(updates["llm.extra_body"], {}) + def test_runtime_config_maps_completion_cap_by_protocol(self) -> None: + expected = { + "openai": "max_completion_tokens", + "openai-responses": "max_output_tokens", + "anthropic": "max_tokens", + } + for protocol, field in expected.items(): + with ( + self.subTest(protocol=protocol), + patched_env( + OCR_LLM_URL="https://gateway.example/v1", + OCR_LLM_TOKEN="llm-secret", + OCR_LLM_MODEL="provider/model", + OCR_LLM_PROTOCOL=protocol, + OCR_LLM_MAX_COMPLETION_TOKENS="4096", + ), + ): + updates = ocr_configure.build_config_updates() + + self.assertEqual(updates["llm.extra_body"], {field: 4096}) + + def test_runtime_config_deduplicates_equal_completion_cap(self) -> None: + with patched_env( + OCR_LLM_URL="https://gateway.example/v1/chat/completions", + OCR_LLM_TOKEN="llm-secret", + OCR_LLM_MODEL="openai/gpt-test", + OCR_LLM_PROTOCOL="openai", + OCR_LLM_MAX_COMPLETION_TOKENS="4096", + OCR_LLM_EXTRA_BODY='{"temperature":0,"max_completion_tokens":4096}', + ): + updates = ocr_configure.build_config_updates() + + self.assertEqual( + updates["llm.extra_body"], + {"temperature": 0, "max_completion_tokens": 4096}, + ) + + def test_runtime_config_rejects_conflicting_completion_cap(self) -> None: + for conflicting in (8192, 4096.0, True, None, "4096"): + with ( + self.subTest(conflicting=conflicting), + patched_env( + OCR_LLM_URL="https://gateway.example/v1/chat/completions", + OCR_LLM_TOKEN="llm-secret", + OCR_LLM_MODEL="openai/gpt-test", + OCR_LLM_PROTOCOL="openai", + OCR_LLM_MAX_COMPLETION_TOKENS="4096", + OCR_LLM_EXTRA_BODY=json.dumps({"max_completion_tokens": conflicting}), + ), + self.assertRaisesRegex( + ocr_configure.OCRRuntimeConfigError, + "conflicts with OCR_LLM_EXTRA_BODY.max_completion_tokens", + ), + ): + ocr_configure.build_config_updates() + + def test_runtime_config_rejects_invalid_completion_caps(self) -> None: + for value in ("0", "-1", "+1", "1.5", "1000001"): + with ( + self.subTest(value=value), + patched_env( + OCR_LLM_URL="https://gateway.example/v1/chat/completions", + OCR_LLM_TOKEN="llm-secret", + OCR_LLM_MODEL="openai/gpt-test", + OCR_LLM_PROTOCOL="openai", + OCR_LLM_MAX_COMPLETION_TOKENS=value, + ), + self.assertRaises(ocr_configure.OCRRuntimeConfigError), + ): + ocr_configure.build_config_updates() + def test_runtime_config_merges_anthropic_disable_thinking_with_extra_body(self) -> None: with patched_env( OCR_REVIEW_LANGUAGE="English", From 63ef1b44eb26bc0829405dadc834c9fad7e41742 Mon Sep 17 00:00:00 2001 From: xeonvs <11463419+xeonvs@users.noreply.github.com> Date: Mon, 24 Aug 2026 20:30:20 +0200 Subject: [PATCH 3/8] Canonicalize LLM provider configuration --- PLANS.md | 1 + src/ocr_toolkit/configure.py | 39 ++------ src/ocr_toolkit/preflight.py | 87 ++++-------------- src/ocr_toolkit/provider_config.py | 142 ++++++++++++++++++++++++++++- tests/test_installed_policy_e2e.py | 1 + tests/test_runtime_helpers.py | 109 +++++++++++++++++++++- 6 files changed, 273 insertions(+), 106 deletions(-) diff --git a/PLANS.md b/PLANS.md index 9086b90..d3f6d2d 100644 --- a/PLANS.md +++ b/PLANS.md @@ -46,3 +46,4 @@ Resume point: complete canonical provider URL/configuration ownership, then impl - Coordination: milestone `v0.8.1`, completion-cap issue #130, provider-diagnostics issue #129, and Draft PR #131 are open. - Exact OCR 1.9.10 Darwin arm64 asset SHA-256 `c626347bafcdbf25cf058af403d16568a3a9ffa1814046ff7c9d1e6becaf60d2` was verified before execution. The isolated production-config-path probe observed `max_completion_tokens=58888` when unset and `max_completion_tokens=4096` when explicitly configured; all temporary binary, config, repository, HOME, and receipt paths were removed. - Completion-cap parsing, protocol mapping, collision rules, environment defaults, generated config, wheel/sdist installed paths, and the reusable exact wire probe are implemented and focused-green. Resume with canonical provider URL/configuration ownership. +- Canonical provider configuration now gives `configure` and `preflight` one environment snapshot and one owner for explicit protocol, API-root normalization, terminal-endpoint compatibility, secret-bearing headers, request-body controls, and auxiliary metadata URLs. Queried inference URLs require an explicit models URL; metadata-disabled preflight remains compatible. Resume with private retry-report failure projection. diff --git a/src/ocr_toolkit/configure.py b/src/ocr_toolkit/configure.py index af69542..c38b774 100644 --- a/src/ocr_toolkit/configure.py +++ b/src/ocr_toolkit/configure.py @@ -5,14 +5,13 @@ import os import sys from typing import Any -from urllib.parse import urlsplit from ocr_toolkit.common.language import resolve_review_language from ocr_toolkit.common.redaction import redact_sensitive from ocr_toolkit.config_writer import OCRConfigError, update_ocr_config from ocr_toolkit.provider_config import ( ProviderConfigError, - request_controls_from_environment, + provider_config_from_environment, ) @@ -28,44 +27,18 @@ def _bool_env(name: str) -> bool: return _env(name).lower() == "true" -def _required_env(name: str) -> str: - value = _env(name) - if not value: - raise OCRRuntimeConfigError(f"{name} is required") - return value - - def build_config_updates() -> dict[str, Any]: """Build OCR config updates from already-normalized CI environment.""" review_language = resolve_review_language() - llm_url = _required_env("OCR_LLM_URL") - llm_token = _required_env("OCR_LLM_TOKEN") - try: - parsed_llm_url = urlsplit(llm_url) - parsed_llm_port = parsed_llm_url.port - parsed_llm_hostname = parsed_llm_url.hostname - parsed_llm_username = parsed_llm_url.username - parsed_llm_password = parsed_llm_url.password - except ValueError as exc: - raise OCRRuntimeConfigError( - "OCR_LLM_URL must be an absolute HTTPS URL without embedded credentials" - ) from exc - if ( - parsed_llm_url.scheme.lower() != "https" - or not parsed_llm_hostname - or (parsed_llm_port is None and parsed_llm_url.netloc.endswith(":")) - or parsed_llm_username is not None - or parsed_llm_password is not None - ): - raise OCRRuntimeConfigError( - "OCR_LLM_URL must be an absolute HTTPS URL without embedded credentials" - ) - llm_model = _required_env("OCR_LLM_MODEL") try: - request_controls = request_controls_from_environment() + provider = provider_config_from_environment() + llm_url = provider.require_inference_url() + llm_token = provider.require_token() + llm_model = provider.require_model() except ProviderConfigError as exc: raise OCRRuntimeConfigError(str(exc)) from exc + request_controls = provider.request_controls updates: dict[str, Any] = { "language": review_language, diff --git a/src/ocr_toolkit/preflight.py b/src/ocr_toolkit/preflight.py index 9bbd0c3..90dcde5 100644 --- a/src/ocr_toolkit/preflight.py +++ b/src/ocr_toolkit/preflight.py @@ -15,11 +15,15 @@ from typing import Any from ocr_toolkit.common.redaction import redact_sensitive +from ocr_toolkit.provider_config import ( + HEADER_NAME_RE, + ProviderConfigError, + provider_config_from_environment, +) HTTP_TIMEOUT_SECONDS = 30 MAX_RESPONSE_BODY_BYTES = 2_000_000 RESPONSE_READ_CHUNK_BYTES = 64 * 1024 -HEADER_NAME_RE = re.compile(r"^[!#$%&'*+.^_`|~0-9A-Za-z-]+$") DEFAULT_REQUEST_HEADERS = { "Accept": "application/json", "User-Agent": "open-code-review-ci-preflight/1.0", @@ -209,63 +213,10 @@ def validate_ocr_binary() -> None: def _models_url() -> str: - explicit_url = _env("OCR_LLM_MODELS_URL") - if explicit_url: - return explicit_url - - base = "" - llm_url = _env("OCR_LLM_URL") - if not base: - normalized_llm_url = llm_url.rstrip("/") - for endpoint in ("/chat/completions", "/responses"): - if normalized_llm_url.endswith(endpoint): - base = normalized_llm_url[: -len(endpoint)] - break - else: - if normalized_llm_url.endswith("/v1"): - base = normalized_llm_url - - if not base: - raise PreflightError( - "Cannot derive LLM /models URL; set OCR_LLM_MODELS_URL or " - "set OCR_LLM_VALIDATE_MODEL=false" - ) - return f"{base.rstrip('/')}/models" - - -def _parse_extra_headers(value: str) -> dict[str, str]: - """Parse optional OCR LLM extra headers JSON for preflight metadata calls.""" - - if not value.strip(): - return {} try: - payload = json.loads(value) - except json.JSONDecodeError as exc: - raise PreflightError("OCR_LLM_EXTRA_HEADERS must be a JSON object") from exc - if not isinstance(payload, dict): - raise PreflightError("OCR_LLM_EXTRA_HEADERS must be a JSON object") - - headers: dict[str, str] = {} - for key, header_value in payload.items(): - if not isinstance(key, str) or not HEADER_NAME_RE.fullmatch(key.strip()): - raise PreflightError("OCR_LLM_EXTRA_HEADERS contains an invalid header name") - if not isinstance(header_value, str): - raise PreflightError("OCR_LLM_EXTRA_HEADERS contains a non-string header value") - if "\r" in header_value or "\n" in header_value: - raise PreflightError("OCR_LLM_EXTRA_HEADERS contains an invalid header value") - headers[key.strip()] = header_value - return headers - - -def _llm_headers(token: str) -> dict[str, str]: - """Build the same auth/header set used by OCR's llm.* configuration.""" - - auth_header = _env("OCR_LLM_AUTH_HEADER", "Authorization") or "Authorization" - if not HEADER_NAME_RE.fullmatch(auth_header): - raise PreflightError("OCR_LLM_AUTH_HEADER is not a valid HTTP header name") - headers = _parse_extra_headers(_env("OCR_LLM_EXTRA_HEADERS")) - headers[auth_header] = f"Bearer {token}" - return headers + return provider_config_from_environment().require_models_url() + except ProviderConfigError as exc: + raise PreflightError(str(exc)) from exc def _context_length(model: dict[str, Any]) -> int: @@ -320,9 +271,11 @@ def _validate_allowed_model(model_id: str) -> bool: def validate_llm_model() -> None: """Verify the selected model exists in OpenAI-compatible metadata.""" - model_id = _env("OCR_LLM_MODEL") - if not model_id: - raise PreflightError("OCR_LLM_MODEL is required") + try: + provider = provider_config_from_environment() + model_id = provider.require_model() + except ProviderConfigError as exc: + raise PreflightError(str(exc)) from exc allowlist_matched = _validate_allowed_model(model_id) validate_mode = _env("OCR_LLM_VALIDATE_MODEL", "false").lower() @@ -332,15 +285,15 @@ def validate_llm_model() -> None: if validate_mode not in {"true", "1", "yes", "on", "auto"}: raise PreflightError("OCR_LLM_VALIDATE_MODEL must be true, false, or auto") - token = _env("OCR_LLM_TOKEN") - if not token: - raise PreflightError("OCR_LLM_TOKEN is required") - headers = _llm_headers(token) + try: + headers = provider.request_headers() + except ProviderConfigError as exc: + raise PreflightError(str(exc)) from exc print("Validating OCR model against LLM gateway metadata") try: - models_url = _models_url() - except PreflightError as exc: + models_url = provider.require_models_url() + except ProviderConfigError as exc: if validate_mode == "auto" and allowlist_matched: print( "OCR LLM /models URL unavailable; continuing because " @@ -348,7 +301,7 @@ def validate_llm_model() -> None: file=sys.stderr, ) return - raise + raise PreflightError(str(exc)) from exc try: payload = _request_json(models_url, headers) except PreflightError as exc: diff --git a/src/ocr_toolkit/provider_config.py b/src/ocr_toolkit/provider_config.py index 371f72d..acb54a7 100644 --- a/src/ocr_toolkit/provider_config.py +++ b/src/ocr_toolkit/provider_config.py @@ -6,8 +6,9 @@ import os import re from collections.abc import Mapping -from dataclasses import dataclass +from dataclasses import dataclass, field from typing import Any +from urllib.parse import SplitResult, urlsplit, urlunsplit HEADER_NAME_RE = re.compile(r"^[!#$%&'*+.^_`|~0-9A-Za-z-]+$") POSITIVE_DECIMAL_RE = re.compile(r"^[1-9][0-9]*$") @@ -18,6 +19,11 @@ "openai": "max_completion_tokens", "openai-responses": "max_output_tokens", } +TERMINAL_ENDPOINTS = { + "anthropic": "/v1/messages", + "openai": "/chat/completions", + "openai-responses": "/responses", +} class ProviderConfigError(Exception): @@ -30,8 +36,58 @@ class ProviderRequestControls: protocol: str auth_header: str - extra_headers: dict[str, str] - extra_body: dict[str, Any] | None + extra_headers: dict[str, str] = field(repr=False) + extra_body: dict[str, Any] | None = field(repr=False) + + +@dataclass(frozen=True) +class ProviderConfig: + """Canonical provider configuration shared by configure and preflight.""" + + inference_url: str | None + api_root_url: str | None + models_url: str | None + model: str | None + token: str | None = field(repr=False) + request_controls: ProviderRequestControls = field(repr=False) + + def require_inference_url(self) -> str: + """Return the normalized OCR API root or reject missing review configuration.""" + + if self.inference_url is None: + raise ProviderConfigError("OCR_LLM_URL is required") + return self.inference_url + + def require_model(self) -> str: + """Return the configured model identifier.""" + + if self.model is None: + raise ProviderConfigError("OCR_LLM_MODEL is required") + return self.model + + def require_token(self) -> str: + """Return the configured provider credential without rendering it.""" + + if self.token is None: + raise ProviderConfigError("OCR_LLM_TOKEN is required") + return self.token + + def require_models_url(self) -> str: + """Return the safe auxiliary URL or explain how to make it explicit.""" + + if self.models_url is None: + raise ProviderConfigError( + "Cannot derive LLM /models URL; set OCR_LLM_MODELS_URL or " + "set OCR_LLM_VALIDATE_MODEL=false" + ) + return self.models_url + + def request_headers(self) -> dict[str, str]: + """Return metadata-request headers with the provider credential.""" + + headers = dict(self.request_controls.extra_headers) + headers[self.request_controls.auth_header] = f"Bearer {self.require_token()}" + return headers def _env(environment: Mapping[str, str], name: str, default: str = "") -> str: @@ -108,6 +164,64 @@ def _parse_completion_cap(value: str) -> int | None: return parsed +def _parse_https_url(value: str, name: str) -> SplitResult: + """Parse one absolute credential-free HTTPS URL with no fragment.""" + + try: + parsed = urlsplit(value) + port = parsed.port + except ValueError as exc: + raise ProviderConfigError( + f"{name} must be an absolute HTTPS URL without embedded credentials or a fragment" + ) from exc + if ( + parsed.scheme.lower() != "https" + or not parsed.hostname + or (port is None and parsed.netloc.endswith(":")) + or parsed.username is not None + or parsed.password is not None + or "#" in value + ): + raise ProviderConfigError( + f"{name} must be an absolute HTTPS URL without embedded credentials or a fragment" + ) + return SplitResult("https", parsed.netloc, parsed.path.rstrip("/"), parsed.query, "") + + +def _canonical_provider_urls( + llm_url: str, models_url: str, protocol: str +) -> tuple[str | None, str | None, str | None]: + """Return normalized inference, API-root, and auxiliary provider URLs.""" + + inference: str | None = None + api_root: str | None = None + derived_models: str | None = None + if llm_url: + parsed = _parse_https_url(llm_url, "OCR_LLM_URL") + path = parsed.path + matched_protocol: str | None = None + for candidate, suffix in TERMINAL_ENDPOINTS.items(): + if path.endswith(suffix): + matched_protocol = candidate + path = path[: -len(suffix)].rstrip("/") + break + if matched_protocol is not None and matched_protocol != protocol: + raise ProviderConfigError( + "OCR_LLM_URL terminal endpoint conflicts with OCR_LLM_PROTOCOL; " + f"use {protocol!r} endpoint semantics or provide the API root" + ) + root_parts = SplitResult(parsed.scheme, parsed.netloc, path, "", "") + api_root = urlunsplit(root_parts) + inference = urlunsplit(root_parts._replace(query=parsed.query)) + if not parsed.query: + derived_models = f"{api_root.rstrip('/')}/models" + + explicit_models: str | None = None + if models_url: + explicit_models = urlunsplit(_parse_https_url(models_url, "OCR_LLM_MODELS_URL")) + return inference, api_root, explicit_models or derived_models + + def request_controls_from_environment( environment: Mapping[str, str] | None = None, ) -> ProviderRequestControls: @@ -150,3 +264,25 @@ def request_controls_from_environment( extra_headers=extra_headers, extra_body=extra_body if explicit_body or extra_body else None, ) + + +def provider_config_from_environment( + environment: Mapping[str, str] | None = None, +) -> ProviderConfig: + """Build one canonical provider configuration from an environment snapshot.""" + + values: Mapping[str, str] = dict(os.environ) if environment is None else dict(environment) + request_controls = request_controls_from_environment(values) + inference_url, api_root_url, models_url = _canonical_provider_urls( + _env(values, "OCR_LLM_URL"), + _env(values, "OCR_LLM_MODELS_URL"), + request_controls.protocol, + ) + return ProviderConfig( + inference_url=inference_url, + api_root_url=api_root_url, + models_url=models_url, + model=_env(values, "OCR_LLM_MODEL") or None, + token=_env(values, "OCR_LLM_TOKEN") or None, + request_controls=request_controls, + ) diff --git a/tests/test_installed_policy_e2e.py b/tests/test_installed_policy_e2e.py index c029e3a..f6c68b6 100644 --- a/tests/test_installed_policy_e2e.py +++ b/tests/test_installed_policy_e2e.py @@ -161,6 +161,7 @@ def test_installed_wheel_and_sdist_expose_target_policy_through_real_mcp( generated_config = json.loads( (config_home / ".opencodereview" / "config.json").read_text(encoding="utf-8") ) + assert generated_config["llm"]["url"] == "https://gateway.example/v1" assert generated_config["llm"]["extra_body"] == {"max_completion_tokens": 4096} protocol_environment = { "HOME": str(root / "home"), diff --git a/tests/test_runtime_helpers.py b/tests/test_runtime_helpers.py index b14a222..204665c 100644 --- a/tests/test_runtime_helpers.py +++ b/tests/test_runtime_helpers.py @@ -17,7 +17,7 @@ from pathlib import Path from typing import Any -from ocr_toolkit import config_writer, mcp_config, preflight +from ocr_toolkit import config_writer, mcp_config, preflight, provider_config from ocr_toolkit import configure as ocr_configure from tests.support import ( cleared_env, @@ -265,7 +265,7 @@ def test_runtime_config_rejects_llm_url_with_invalid_port(self) -> None: def test_runtime_config_updates_parse_headers_body_and_language(self) -> None: with patched_env( OCR_REVIEW_LANGUAGE="English", - OCR_LLM_URL="https://gateway.example/v1/chat/completions", + OCR_LLM_URL="https://gateway.example", OCR_LLM_TOKEN="llm-secret", OCR_LLM_MODEL="openai/gpt-test", OCR_LLM_AUTH_HEADER="authorization", @@ -326,6 +326,108 @@ def test_runtime_config_supports_openai_responses_protocol(self) -> None: self.assertEqual(updates["llm.protocol"], "openai-responses") self.assertFalse(updates["llm.use_anthropic"]) + self.assertEqual(updates["llm.url"], "https://gateway.example/v1") + + def test_provider_config_normalizes_roots_endpoints_and_query(self) -> None: + cases = ( + ("openai", "https://gateway.example/v1/", "https://gateway.example/v1"), + ( + "openai", + "https://gateway.example/v1/chat/completions/", + "https://gateway.example/v1", + ), + ( + "openai-responses", + "https://gateway.example/v1/responses", + "https://gateway.example/v1", + ), + ( + "anthropic", + "https://gateway.example/proxy/v1/messages", + "https://gateway.example/proxy", + ), + ) + for protocol, raw_url, expected_root in cases: + with self.subTest(protocol=protocol, raw_url=raw_url): + config = provider_config.provider_config_from_environment( + {"OCR_LLM_PROTOCOL": protocol, "OCR_LLM_URL": raw_url} + ) + + self.assertEqual(config.api_root_url, expected_root) + self.assertEqual(config.inference_url, expected_root) + self.assertEqual(config.models_url, f"{expected_root}/models") + + queried = provider_config.provider_config_from_environment( + { + "OCR_LLM_PROTOCOL": "openai", + "OCR_LLM_URL": "https://gateway.example/v1/chat/completions?tenant=review", + } + ) + self.assertEqual(queried.api_root_url, "https://gateway.example/v1") + self.assertEqual(queried.inference_url, "https://gateway.example/v1?tenant=review") + self.assertIsNone(queried.models_url) + with self.assertRaisesRegex(provider_config.ProviderConfigError, "OCR_LLM_MODELS_URL"): + queried.require_models_url() + + def test_provider_config_uses_explicit_models_url_for_queried_inference(self) -> None: + config = provider_config.provider_config_from_environment( + { + "OCR_LLM_PROTOCOL": "openai", + "OCR_LLM_URL": "https://gateway.example/v1?tenant=review", + "OCR_LLM_MODELS_URL": "https://metadata.example/catalog?tenant=review", + } + ) + + self.assertEqual( + config.models_url, + "https://metadata.example/catalog?tenant=review", + ) + + def test_provider_config_rejects_protocol_mismatched_terminal_endpoints(self) -> None: + cases = ( + ("openai", "https://gateway.example/v1/responses"), + ("openai", "https://gateway.example/v1/messages"), + ("openai-responses", "https://gateway.example/v1/chat/completions"), + ("anthropic", "https://gateway.example/v1/responses"), + ) + for protocol, url in cases: + with ( + self.subTest(protocol=protocol, url=url), + self.assertRaisesRegex( + provider_config.ProviderConfigError, + "terminal endpoint conflicts with OCR_LLM_PROTOCOL", + ), + ): + provider_config.provider_config_from_environment( + {"OCR_LLM_PROTOCOL": protocol, "OCR_LLM_URL": url} + ) + + def test_provider_config_rejects_fragments_and_hides_secret_fields_from_repr(self) -> None: + for name in ("OCR_LLM_URL", "OCR_LLM_MODELS_URL"): + with ( + self.subTest(name=name), + self.assertRaisesRegex(provider_config.ProviderConfigError, "fragment"), + ): + provider_config.provider_config_from_environment( + { + "OCR_LLM_PROTOCOL": "openai", + name: "https://gateway.example/v1#private", + } + ) + + config = provider_config.provider_config_from_environment( + { + "OCR_LLM_EXTRA_HEADERS": '{"X-Secret":"private-header"}', + "OCR_LLM_EXTRA_BODY": '{"private-body":"value"}', + "OCR_LLM_PROTOCOL": "openai", + "OCR_LLM_TOKEN": "private-token", + "OCR_LLM_URL": "https://gateway.example/v1", + } + ) + rendered = repr(config) + self.assertNotIn("private-token", rendered) + self.assertNotIn("private-header", rendered) + self.assertNotIn("private-body", rendered) def test_runtime_config_rejects_removed_anthropic_switch_with_migration(self) -> None: for legacy_value in ("", "false", "true"): @@ -485,7 +587,7 @@ def test_runtime_config_rejects_invalid_completion_caps(self) -> None: def test_runtime_config_merges_anthropic_disable_thinking_with_extra_body(self) -> None: with patched_env( OCR_REVIEW_LANGUAGE="English", - OCR_LLM_URL="https://gateway.example/v1/chat/completions", + OCR_LLM_URL="https://gateway.example", OCR_LLM_TOKEN="llm-secret", OCR_LLM_MODEL="anthropic/claude-test", OCR_LLM_PROTOCOL="anthropic", @@ -1505,6 +1607,7 @@ def test_models_url_accepts_responses_endpoint(self) -> None: with patched_env( OCR_LLM_MODELS_URL="", OCR_LLM_API_BASE_REMOVED="", + OCR_LLM_PROTOCOL="openai-responses", OCR_LLM_URL="https://gateway.example/v1/responses", ): self.assertEqual(preflight._models_url(), "https://gateway.example/v1/models") From d796e7c4d19c7bff2e2c746dda756e250adc125a Mon Sep 17 00:00:00 2001 From: xeonvs <11463419+xeonvs@users.noreply.github.com> Date: Mon, 24 Aug 2026 20:41:59 +0200 Subject: [PATCH 4/8] Project safe LLM provider failures --- PLANS.md | 7 +- src/ocr_toolkit/posting/result.py | 14 +- src/ocr_toolkit/posting/workflow.py | 142 ++++++++++------ src/ocr_toolkit/provider_failure.py | 243 +++++++++++++++++++++++++++ src/ocr_toolkit/review_runner.py | 30 +++- tests/test_posting_helpers.py | 251 +++++++++++++++++++++++----- tests/test_provider_failure.py | 229 +++++++++++++++++++++++++ tests/test_review_runner.py | 57 +++++++ 8 files changed, 869 insertions(+), 104 deletions(-) create mode 100644 src/ocr_toolkit/provider_failure.py create mode 100644 tests/test_provider_failure.py diff --git a/PLANS.md b/PLANS.md index d3f6d2d..fb48557 100644 --- a/PLANS.md +++ b/PLANS.md @@ -39,11 +39,12 @@ Status: active, `release-required`. Target stable version: `0.8.1`. - Push the complete feature history to the Draft PR, wait for hosted checks, address evidence-driven failures through the same commit gate, then mark ready and merge through protected review. - Verify the deterministic TestPyPI development build, then prepare and merge the protected `Release v0.8.1` PR. Monitor stable TestPyPI/PyPI publication, tag, immutable GitHub Release, provenance, attestations, supported-Python installs, and immutable receipt; close tracked issues only after independent external reconciliation. -Resume point: complete canonical provider URL/configuration ownership, then implement the private retry-report failure projection. +Resume point: document the completed provider configuration and safe-failure contracts, add the separate Towncrier fragments, and complete the release handoff gates. #### Current implementation evidence - Coordination: milestone `v0.8.1`, completion-cap issue #130, provider-diagnostics issue #129, and Draft PR #131 are open. - Exact OCR 1.9.10 Darwin arm64 asset SHA-256 `c626347bafcdbf25cf058af403d16568a3a9ffa1814046ff7c9d1e6becaf60d2` was verified before execution. The isolated production-config-path probe observed `max_completion_tokens=58888` when unset and `max_completion_tokens=4096` when explicitly configured; all temporary binary, config, repository, HOME, and receipt paths were removed. -- Completion-cap parsing, protocol mapping, collision rules, environment defaults, generated config, wheel/sdist installed paths, and the reusable exact wire probe are implemented and focused-green. Resume with canonical provider URL/configuration ownership. -- Canonical provider configuration now gives `configure` and `preflight` one environment snapshot and one owner for explicit protocol, API-root normalization, terminal-endpoint compatibility, secret-bearing headers, request-body controls, and auxiliary metadata URLs. Queried inference URLs require an explicit models URL; metadata-disabled preflight remains compatible. Resume with private retry-report failure projection. +- Completion-cap parsing, protocol mapping, collision rules, environment defaults, generated config, wheel/sdist installed paths, and the reusable exact wire probe are implemented and focused-green. +- Canonical provider configuration now gives `configure` and `preflight` one environment snapshot and one owner for explicit protocol, API-root normalization, terminal-endpoint compatibility, secret-bearing headers, request-body controls, and auxiliary metadata URLs. Queried inference URLs require an explicit models URL; metadata-disabled preflight remains compatible. +- Provider failure projection now hostile-reads the bounded private result, validates retry-report v1 counters and terminal attempt facts, and emits only a closed provider-neutral reason. Non-zero classified runs use one static GitLab renderer, keep stderr/provider fields private, preserve the previous review, publish no findings, and never reach approval; legacy billing warnings use the same renderer. The focused gate passed Ruff, full package mypy, 302 tests, and 119 subtests. diff --git a/src/ocr_toolkit/posting/result.py b/src/ocr_toolkit/posting/result.py index ebea05f..4729c71 100644 --- a/src/ocr_toolkit/posting/result.py +++ b/src/ocr_toolkit/posting/result.py @@ -17,13 +17,14 @@ load_ocr_result, ) from ocr_toolkit.posting.comments import clean_text, compact_escaped_text +from ocr_toolkit.provider_failure import ProviderFailureReason from ocr_toolkit.result_contract import ReviewOutcome __all__ = [ "OcrResultMalformed", "OcrResultMissing", "OcrResultTooLarge", - "llm_billing_failure_warnings", + "llm_billing_failure_reason", "load_ocr_result", "normalize_coverage_diagnostics", "ocr_warning_text", @@ -199,12 +200,13 @@ def ocr_warning_text(warning: Any, *, _seen: set[int] | None = None) -> str: return clean_text(warning) -def llm_billing_failure_warnings(warnings: Sequence[Any]) -> list[str]: - """Return OCR warnings that indicate LLM provider billing/quota failure.""" +def llm_billing_failure_reason( + warnings: Sequence[Any], +) -> ProviderFailureReason | None: + """Map a legacy OCR billing warning to the shared safe provider reason.""" - matches: list[str] = [] for warning in warnings: text = ocr_warning_text(warning) if text and LLM_BILLING_FAILURE_RE.search(text): - matches.append(text) - return matches + return ProviderFailureReason.RATE_OR_SPENDING_LIMIT + return None diff --git a/src/ocr_toolkit/posting/workflow.py b/src/ocr_toolkit/posting/workflow.py index 143b7af..805b75a 100644 --- a/src/ocr_toolkit/posting/workflow.py +++ b/src/ocr_toolkit/posting/workflow.py @@ -15,7 +15,6 @@ from ocr_toolkit.common.git import isolated_git_environment, read_only_git_prefix from ocr_toolkit.common.markdown import markdown_code_block, neutralize_quick_actions -from ocr_toolkit.common.redaction import redact_sensitive from ocr_toolkit.evidence.artifacts import repository_artifacts from ocr_toolkit.ocr_result import ( TOOLKIT_RESULT_KEY, @@ -37,7 +36,6 @@ clean_text, code_text, comment_line, - compact_escaped_text, ) from ocr_toolkit.posting.formatting import ( format_fallback_comment_chunks, @@ -71,12 +69,11 @@ ) from ocr_toolkit.posting.payloads import build_marked_note_body from ocr_toolkit.posting.result import ( - llm_billing_failure_warnings, + llm_billing_failure_reason, normalize_coverage_diagnostics, ) from ocr_toolkit.posting.snapshot import ( BotCommentRefs, - cleanup_drafts_created_by_this_run, collect_previous_bot_comment_refs, delete_previous_bot_comments_if_collected, delete_previous_setup_notes, @@ -101,6 +98,10 @@ PreExecutionStatusError, read_pre_execution_status, ) +from ocr_toolkit.provider_failure import ( + ProviderFailureReason, + provider_failure_reason, +) from ocr_toolkit.result_contract import OcrResultContractError, ReviewOutcome, parse_result_outcome # Kept as a module-level compatibility seam for tests and external monkey-patching. @@ -662,19 +663,13 @@ def post_results(config: GitLabConfig, result: dict[str, Any]) -> int: token_usage_summary=token_usage_summary, ) - billing_warnings = llm_billing_failure_warnings(warnings) - if billing_warnings: + billing_reason = llm_billing_failure_reason(warnings) + if billing_reason is not None: print( "OCR reported an LLM provider billing/quota failure; refusing to publish normal review notes.", file=sys.stderr, ) - return post_llm_provider_failure( - config, - billing_warnings, - tool_calls_summary=tool_calls_summary, - mcp_usage_summary=mcp_usage_summary, - token_usage_summary=token_usage_summary, - ) + return post_llm_provider_failure(config, billing_reason) previous_bot_comment_refs = collect_previous_bot_comment_refs(config) transaction = PostingTransaction() @@ -1101,38 +1096,75 @@ def post_manifest_failure( def post_llm_provider_failure( config: GitLabConfig, - warnings: Sequence[str], - tool_calls_summary: str = "", - mcp_usage_summary: str = "", - token_usage_summary: str = "", + reason: ProviderFailureReason, ) -> int: - """Post a visible OCR provider failure and preserve previous review notes.""" + """Render one closed provider failure without publishing private diagnostics.""" transaction = PostingTransaction() - warning_items: list[str] = [] - for warning in warnings[:10]: - safe_warning = compact_escaped_text( - neutralize_quick_actions(redact_sensitive(warning)), - 1200, - ) - if safe_warning: - warning_items.append(f"- {safe_warning}") + summary, remediation = { + ProviderFailureReason.AUTHENTICATION: ( + "The LLM provider did not accept the configured credential.", + "Check the protected credential and its authentication header, then rerun the pipeline.", + ), + ProviderFailureReason.AUTHORIZATION: ( + "The LLM provider authenticated the request but did not authorize it.", + "Check the credential permissions and provider access policy, then rerun the pipeline.", + ), + ProviderFailureReason.RATE_OR_SPENDING_LIMIT: ( + "The LLM provider rejected the request under a rate or spending limit.", + "The cause may be ordinary throttling, an account or API-key spending limit, or cost reservation from the requested output cap. Retry later and check account limits. If short probes pass but a full review fails before generation, try an explicit `OCR_LLM_MAX_COMPLETION_TOKENS`, for example `4096`.", + ), + ProviderFailureReason.OVERLOADED: ( + "The LLM provider reported that it was overloaded.", + "Retry later or use another already-qualified provider deployment.", + ), + ProviderFailureReason.TIMEOUT: ( + "The LLM provider request timed out.", + "Check provider latency and the network path, then rerun the pipeline.", + ), + ProviderFailureReason.NETWORK: ( + "The LLM provider could not be reached reliably.", + "Check DNS, TLS, proxy, and runner connectivity, then rerun the pipeline.", + ), + ProviderFailureReason.ENDPOINT_OR_MODEL_NOT_FOUND: ( + "The LLM provider reported that the configured endpoint or model was not found.", + "Verify both the provider API root and model identifier; the safe diagnostic cannot distinguish which one was absent.", + ), + ProviderFailureReason.REQUEST_REJECTED: ( + "The LLM provider rejected the review request.", + "Check the explicit protocol, endpoint shape, model contract, and request controls, then rerun the pipeline.", + ), + ProviderFailureReason.PROVIDER_UNAVAILABLE: ( + "The LLM provider was unavailable while processing the review.", + "Retry later and check the provider service status.", + ), + ProviderFailureReason.INVALID_RESPONSE: ( + "The LLM provider returned a response that could not be consumed safely.", + "Check protocol compatibility and provider response health, then rerun the pipeline.", + ), + ProviderFailureReason.CANCELLED: ( + "The LLM provider request was cancelled before the review completed.", + "Check pipeline cancellation and deadline signals, then rerun when the job can complete.", + ), + ProviderFailureReason.MIXED: ( + "The review encountered more than one provider failure category.", + "Inspect the private CI diagnostics, correct every provider-side failure, then rerun the pipeline.", + ), + ProviderFailureReason.UNKNOWN: ( + "The LLM provider request failed for an unclassified safe reason.", + "Inspect the private CI diagnostics and provider health, then rerun the pipeline.", + ), + }[reason] body_parts = [ - "OCR could not complete the review because the LLM provider reported a billing, quota, or balance failure.", + summary, "", + f"- Safe classification: `{reason.value}`.", "- Normal review comments were not published.", "- Previous OCR review comments were preserved.", - "- Refill or rotate the LLM token, then rerun the pipeline.", + "- Automatic approval was not attempted.", + f"- {remediation}", ] - if warning_items: - body_parts.extend(["", "**Provider warnings:**", *warning_items]) - if tool_calls_summary: - body_parts.extend(["", tool_calls_summary]) - if mcp_usage_summary: - body_parts.append(mcp_usage_summary) - if token_usage_summary: - body_parts.append(token_usage_summary) response = post_review_note_bounded( config, @@ -1142,18 +1174,23 @@ def post_llm_provider_failure( ) if response is None: print("Failed to create OCR provider-failure note.", file=sys.stderr) - cleanup_drafts_created_by_this_run(config, transaction) - print_posting_failure_banner() - return 1 + return posting_failure_exit(config, None, transaction) if not finalize_posting(config, transaction): return publish_failure_exit(config, transaction) - print( - "Open Code Review did not complete because the LLM provider reported a billing/quota failure.", - file=sys.stderr, - ) - return 1 + print(f"Open Code Review provider failure: {reason.value}.", file=sys.stderr) + return 1 if strict_posting() else 0 + + +def _result_provider_failure_reason(result_path: Path) -> ProviderFailureReason | None: + """Hostile-read one bounded private result and return only its closed reason.""" + + try: + result = load_ocr_result(result_path) + except (OcrResultMalformed, OcrResultMissing, OcrResultTooLarge): + return None + return provider_failure_reason(result) def post_parse_error(config: GitLabConfig, stderr_path: Path) -> int: @@ -1194,11 +1231,18 @@ def post_parse_error(config: GitLabConfig, stderr_path: Path) -> int: return 1 if strict_posting() else 0 -def post_ocr_failure(config: GitLabConfig, stderr_path: Path, exit_code: int) -> int: +def post_ocr_failure( + config: GitLabConfig, + stderr_path: Path, + exit_code: int, + result_path: Path | None = None, +) -> int: """Post a safe failure note when OCR exits with a non-zero status. A non-zero OCR exit code means the JSON output may be partial or misleading, - so this script intentionally does not publish normal review comments. + so this script intentionally does not publish normal review comments. A + bounded retry-report v1 may contribute only a closed provider-failure + reason; its raw diagnostics and stderr remain private. Previous OCR bot notes are intentionally NOT cleaned up here: the last valid review must remain visible until a successful run replaces it. @@ -1222,6 +1266,10 @@ def post_ocr_failure(config: GitLabConfig, stderr_path: Path, exit_code: int) -> if status is not None: return post_pre_execution_status(config, status) + reason = _result_provider_failure_reason(result_path) if result_path is not None else None + if reason is not None: + return post_llm_provider_failure(config, reason) + transaction = PostingTransaction() details_enabled = os.environ.get("OCR_POST_ERROR_DETAILS") == "1" details = read_stderr_excerpt(stderr_path) if details_enabled else "" @@ -1342,7 +1390,7 @@ def main(argv: list[str] | None = None) -> int: exit_code = ocr_exit_code() if exit_code != 0: - return post_ocr_failure(config, stderr_path, exit_code) + return post_ocr_failure(config, stderr_path, exit_code, result_path) try: result = load_ocr_result(result_path) diff --git a/src/ocr_toolkit/provider_failure.py b/src/ocr_toolkit/provider_failure.py new file mode 100644 index 0000000..a89b7f9 --- /dev/null +++ b/src/ocr_toolkit/provider_failure.py @@ -0,0 +1,243 @@ +"""Project private OCR retry diagnostics into closed provider failure reasons.""" + +from __future__ import annotations + +from collections import Counter +from collections.abc import Mapping +from enum import StrEnum +from typing import Any + +RETRY_REPORT_SCHEMA = "ocr.llm-retry-report/v1" +MAX_RETRY_REQUESTS = 10_000 +MAX_ATTEMPTS_PER_REQUEST = 100 +ERROR_CLASSES = frozenset( + { + "authentication", + "cancelled", + "network", + "overloaded", + "provider", + "rate_limited", + "timeout", + "unknown", + } +) +FAILURE_PHASES = frozenset( + {"context", "http", "response_decode", "response_status", "stream", "transport"} +) +REQUEST_OUTCOMES = frozenset({"cancelled", "failed", "recovered", "succeeded"}) +ATTEMPT_OUTCOMES = frozenset({"error", "success"}) + + +class ProviderFailureReason(StrEnum): + """Closed provider-neutral failure reason safe for control-flow and rendering.""" + + AUTHENTICATION = "authentication" + AUTHORIZATION = "authorization" + RATE_OR_SPENDING_LIMIT = "rate-or-spending-limit" + OVERLOADED = "overloaded" + TIMEOUT = "timeout" + NETWORK = "network" + ENDPOINT_OR_MODEL_NOT_FOUND = "endpoint-or-model-not-found" + REQUEST_REJECTED = "request-rejected" + PROVIDER_UNAVAILABLE = "provider-unavailable" + INVALID_RESPONSE = "invalid-response" + CANCELLED = "cancelled" + MIXED = "mixed" + UNKNOWN = "unknown" + + +class RetryReportError(Exception): + """The private retry report is malformed or internally inconsistent.""" + + +def _bounded_count(value: object, field: str, *, maximum: int = MAX_RETRY_REQUESTS) -> int: + """Return one bounded non-negative JSON integer.""" + + if type(value) is not int or not 0 <= value <= maximum: + raise RetryReportError(f"retry report field {field!r} is invalid") + return value + + +def _http_reason(status_code: int) -> ProviderFailureReason: + """Map one observed non-success HTTP status to a closed reason.""" + + if status_code == 401: + return ProviderFailureReason.AUTHENTICATION + if status_code == 403: + return ProviderFailureReason.AUTHORIZATION + if status_code in {402, 429}: + return ProviderFailureReason.RATE_OR_SPENDING_LIMIT + if status_code == 404: + return ProviderFailureReason.ENDPOINT_OR_MODEL_NOT_FOUND + if status_code in {408, 504}: + return ProviderFailureReason.TIMEOUT + if status_code == 529: + return ProviderFailureReason.OVERLOADED + if 500 <= status_code <= 599: + return ProviderFailureReason.PROVIDER_UNAVAILABLE + return ProviderFailureReason.REQUEST_REJECTED + + +def _validate_http_class(status_code: int, error_class: str, failure_phase: str) -> None: + """Reject attempt fields that contradict OCR's status-derived v1 classifier.""" + + if failure_phase != "http": + raise RetryReportError("retry report HTTP status has a non-HTTP failure phase") + expected = "provider" + if status_code == 429: + expected = "rate_limited" + elif status_code == 529: + expected = "overloaded" + elif status_code in {401, 403}: + expected = "authentication" + elif status_code in {408, 504}: + expected = "timeout" + if error_class != expected: + raise RetryReportError("retry report HTTP status contradicts its error class") + + +def _attempt_reason(attempt: Mapping[str, Any]) -> ProviderFailureReason | None: + """Validate one attempt and return its closed error reason, if any.""" + + outcome = attempt.get("outcome") + if outcome not in ATTEMPT_OUTCOMES: + raise RetryReportError("retry report attempt outcome is invalid") + status_value = attempt.get("status_code", 0) + if type(status_value) is not int or not (status_value == 0 or 100 <= status_value <= 599): + raise RetryReportError("retry report attempt status_code is invalid") + error_class = attempt.get("error_class", "") + failure_phase = attempt.get("failure_phase", "") + if outcome == "success": + if error_class or failure_phase or not 200 <= status_value <= 299: + raise RetryReportError("retry report success attempt is inconsistent") + return None + if error_class not in ERROR_CLASSES or failure_phase not in FAILURE_PHASES: + raise RetryReportError("retry report error attempt classification is invalid") + + if status_value and not 200 <= status_value <= 299: + _validate_http_class(status_value, error_class, failure_phase) + return _http_reason(status_value) + if status_value: + if failure_phase not in {"response_decode", "response_status", "stream"}: + raise RetryReportError("retry report successful HTTP status has invalid failure phase") + if error_class not in {"network", "provider", "unknown"}: + raise RetryReportError("retry report successful HTTP status has invalid error class") + return ProviderFailureReason.INVALID_RESPONSE + if failure_phase == "http" or error_class in { + "authentication", + "overloaded", + "provider", + "rate_limited", + }: + raise RetryReportError("retry report transport failure has HTTP-only classification") + if error_class == "cancelled": + return ProviderFailureReason.CANCELLED + if error_class == "timeout": + return ProviderFailureReason.TIMEOUT + if error_class == "network": + if failure_phase in {"response_decode", "response_status", "stream"}: + return ProviderFailureReason.INVALID_RESPONSE + return ProviderFailureReason.NETWORK + return ProviderFailureReason.UNKNOWN + + +def _request_reason(request: Mapping[str, Any]) -> ProviderFailureReason | None: + """Validate one logical request and classify only its terminal failure.""" + + outcome = request.get("outcome") + if outcome not in REQUEST_OUTCOMES: + raise RetryReportError("retry report request outcome is invalid") + attempts = request.get("attempts") + if not isinstance(attempts, list) or not 0 < len(attempts) <= MAX_ATTEMPTS_PER_REQUEST: + raise RetryReportError("retry report request attempts are invalid") + reasons: list[ProviderFailureReason | None] = [] + for index, attempt in enumerate(attempts, start=1): + if not isinstance(attempt, dict) or attempt.get("attempt") != index: + raise RetryReportError("retry report attempt order is invalid") + reasons.append(_attempt_reason(attempt)) + if outcome == "succeeded": + if len(attempts) < 2 or any(reason is not None for reason in reasons): + raise RetryReportError("retry report succeeded request is inconsistent") + return None + if outcome == "recovered": + if reasons[-1] is not None or not any(reason is not None for reason in reasons[:-1]): + raise RetryReportError("retry report recovered request is inconsistent") + return None + if outcome == "cancelled": + return ProviderFailureReason.CANCELLED + if reasons[-1] is None: + raise RetryReportError("retry report failed request ends in success") + return reasons[-1] + + +def parse_retry_report_failure(result: object) -> ProviderFailureReason | None: + """Validate retry-report v1 and return one closed terminal failure reason.""" + + if not isinstance(result, dict): + raise RetryReportError("OCR result must be an object") + report = result.get("retry_report") + if report is None: + return None + if not isinstance(report, dict) or report.get("schema_version") != RETRY_REPORT_SCHEMA: + raise RetryReportError("OCR retry report schema is unsupported") + requests = report.get("requests") + if not isinstance(requests, list) or not 0 < len(requests) <= MAX_RETRY_REQUESTS: + raise RetryReportError("OCR retry report requests are invalid") + + total_requests = _bounded_count(report.get("total_requests"), "total_requests") + expected_counts = { + "retried_requests": sum( + 1 + for request in requests + if isinstance(request, dict) + and isinstance(request.get("attempts"), list) + and len(request["attempts"]) > 1 + ), + "total_retries": sum( + max(len(request.get("attempts", [])) - 1, 0) + for request in requests + if isinstance(request, dict) and isinstance(request.get("attempts"), list) + ), + "recovered_requests": sum( + 1 + for request in requests + if isinstance(request, dict) and request.get("outcome") == "recovered" + ), + "failed_requests": sum( + 1 + for request in requests + if isinstance(request, dict) and request.get("outcome") == "failed" + ), + "cancelled_requests": sum( + 1 + for request in requests + if isinstance(request, dict) and request.get("outcome") == "cancelled" + ), + } + if total_requests < len(requests): + raise RetryReportError("OCR retry report total_requests is inconsistent") + for field, expected in expected_counts.items(): + if _bounded_count(report.get(field), field, maximum=MAX_RETRY_REQUESTS * 100) != expected: + raise RetryReportError(f"OCR retry report {field} is inconsistent") + + reasons: list[ProviderFailureReason] = [] + for request in requests: + if not isinstance(request, dict): + raise RetryReportError("OCR retry report request is invalid") + reason = _request_reason(request) + if reason is not None: + reasons.append(reason) + if not reasons: + return None + counts = Counter(reasons) + return next(iter(counts)) if len(counts) == 1 else ProviderFailureReason.MIXED + + +def provider_failure_reason(result: object) -> ProviderFailureReason | None: + """Return a closed reason, degrading malformed private diagnostics to unavailable.""" + + try: + return parse_retry_report_failure(result) + except RetryReportError: + return None diff --git a/src/ocr_toolkit/review_runner.py b/src/ocr_toolkit/review_runner.py index 44f7968..bbb0710 100644 --- a/src/ocr_toolkit/review_runner.py +++ b/src/ocr_toolkit/review_runner.py @@ -81,6 +81,7 @@ OcrResultMissing, OcrResultTooLarge, inspect_ocr_result, + load_ocr_result, transform_ocr_result, ) from ocr_toolkit.posting.result import ocr_warning_text @@ -93,6 +94,7 @@ PreExecutionStatusError, write_pre_execution_status, ) +from ocr_toolkit.provider_failure import ProviderFailureReason, provider_failure_reason from ocr_toolkit.providers.gitlab import ( GitLabProviderError, acquire_review_snapshot, @@ -2040,6 +2042,16 @@ def read_stderr_excerpt(stderr_path: Path, max_chars: int = DEFAULT_DIAGNOSTIC_C return redact_sensitive(text)[:max_chars] +def _closed_provider_failure_reason(result_path: Path) -> ProviderFailureReason | None: + """Return only a validated retry-report reason from one private result artifact.""" + + try: + result = load_ocr_result(result_path) + except (OcrResultMalformed, OcrResultMissing, OcrResultTooLarge): + return None + return provider_failure_reason(result) + + def _resolve_ocr_binary() -> str: """Resolve one exact executable before entering the repository-owned process cwd.""" @@ -2104,10 +2116,18 @@ def run_review( if completed.returncode != 0: print(f"Open Code Review exited with code {completed.returncode}.", file=sys.stderr) - excerpt = read_stderr_excerpt(stderr_path) - if excerpt: - print("Safe OCR stderr excerpt:", file=sys.stderr) - print(excerpt, file=sys.stderr) + provider_reason = _closed_provider_failure_reason(result_path) + if provider_reason is not None: + print( + f"OCR provider failure classified as {provider_reason.value}; " + "private diagnostics remain in the owner-only artifacts.", + file=sys.stderr, + ) else: - print("OCR did not provide a readable stderr diagnostic.", file=sys.stderr) + excerpt = read_stderr_excerpt(stderr_path) + if excerpt: + print("Safe OCR stderr excerpt:", file=sys.stderr) + print(excerpt, file=sys.stderr) + else: + print("OCR did not provide a readable stderr diagnostic.", file=sys.stderr) return completed.returncode diff --git a/tests/test_posting_helpers.py b/tests/test_posting_helpers.py index 47ca93b..7f68cf6 100644 --- a/tests/test_posting_helpers.py +++ b/tests/test_posting_helpers.py @@ -41,6 +41,7 @@ PreExecutionStatus, write_pre_execution_status, ) +from ocr_toolkit.provider_failure import ProviderFailureReason from ocr_toolkit.result_contract import CoverageFailure, ReviewOutcome from tests.support import ( gitlab_config, @@ -1483,6 +1484,174 @@ def test_small_text_budgets_stay_within_budget(self) -> None: class PostingWorkflowTests(unittest.TestCase): + def test_nonzero_ocr_projects_retry_report_to_static_provider_note(self) -> None: + """Publish only a closed 429 reason and keep raw result and stderr data private.""" + + notes: list[str] = [] + private_values = ( + "private-provider", + "private-model", + "/private/repository/file.py", + "private-request-id", + "private-response-body", + "private-finding", + "private-stderr", + ) + payload = { + "comments": [{"content": "private-finding"}], + "retry_report": { + "schema_version": "ocr.llm-retry-report/v1", + "total_requests": 1, + "retried_requests": 0, + "total_retries": 0, + "recovered_requests": 0, + "failed_requests": 1, + "cancelled_requests": 0, + "requests": [ + { + "logical_request_id": "private-logical-id", + "provider": "private-provider", + "model": "private-model", + "file_path": "/private/repository/file.py", + "task_type": "main_task", + "request_no": 1, + "outcome": "failed", + "attempts": [ + { + "attempt": 1, + "outcome": "error", + "error_class": "rate_limited", + "failure_phase": "http", + "status_code": 429, + "request_id": "private-request-id", + "provider_body": "private-response-body", + } + ], + } + ], + }, + } + + def capture( + _config: Any, _title: str, body: str, _transaction: PostingTransaction + ) -> dict[str, int]: + notes.append(body) + return {"id": 1} + + with tempfile.TemporaryDirectory() as tmp: + result_path = Path(tmp) / "result.json" + stderr_path = Path(tmp) / "stderr.log" + result_path.write_text(json.dumps(payload), encoding="utf-8") + stderr_path.write_text("private-stderr\n/merge", encoding="utf-8") + stderr = io.StringIO() + with ( + patched_env( + OCR_EXIT_CODE="1", + OCR_POST_ERROR_DETAILS="1", + OCR_STRICT_POSTING="false", + ), + patched_attr(workflow, "load_gitlab_config", lambda: gitlab_config()), + patched_attr( + workflow, + "repository_artifacts", + lambda: (_ for _ in ()).throw(RuntimeError("unavailable")), + ), + patched_attr(workflow, "post_review_note_bounded", capture), + patched_attr(workflow, "finalize_posting", lambda *_args: True), + patched_attr( + workflow, + "read_stderr_excerpt", + lambda *_args: self.fail("classified failure read stderr"), + ), + patched_attr( + workflow, + "execute_approval", + lambda *_args: self.fail("provider failure reached approval"), + ), + redirect_stderr(stderr), + ): + exit_code = workflow.main([str(result_path), str(stderr_path)]) + + self.assertEqual(exit_code, 0) + published = "\n".join(notes) + controlled_log = stderr.getvalue() + self.assertIn("rate-or-spending-limit", published) + self.assertIn("Previous OCR review comments were preserved", published) + self.assertIn("Automatic approval was not attempted", published) + for private in private_values: + self.assertNotIn(private, published) + self.assertNotIn(private, controlled_log) + + def test_malformed_retry_report_retains_generic_failure_details_path(self) -> None: + """Fall back to the existing generic note when private diagnostics contradict v1.""" + + notes: list[str] = [] + payload = { + "retry_report": { + "schema_version": "ocr.llm-retry-report/v1", + "total_requests": 1, + "retried_requests": 0, + "total_retries": 0, + "recovered_requests": 0, + "failed_requests": 2, + "cancelled_requests": 0, + "requests": [], + } + } + + def capture( + _config: Any, title: str, body: str, _transaction: PostingTransaction + ) -> dict[str, int]: + notes.append(f"{title}\n{body}") + return {"id": 1} + + with tempfile.TemporaryDirectory() as tmp: + result_path = Path(tmp) / "result.json" + stderr_path = Path(tmp) / "stderr.log" + result_path.write_text(json.dumps(payload), encoding="utf-8") + stderr_path.write_text("safe bounded diagnostic", encoding="utf-8") + with ( + patched_env(OCR_POST_ERROR_DETAILS="1", OCR_STRICT_POSTING="false"), + patched_attr( + workflow, + "repository_artifacts", + lambda: (_ for _ in ()).throw(RuntimeError("unavailable")), + ), + patched_attr(workflow, "post_review_note_bounded", capture), + patched_attr(workflow, "finalize_posting", lambda *_args: True), + ): + exit_code = workflow.post_ocr_failure(gitlab_config(), stderr_path, 1, result_path) + + self.assertEqual(exit_code, 0) + self.assertIn("Open Code Review failure", notes[0]) + self.assertIn("safe bounded diagnostic", notes[0]) + self.assertNotIn("provider failure", notes[0].lower()) + + def test_legacy_billing_warning_uses_shared_provider_renderer(self) -> None: + """Route the pre-v1 warning shape to the same closed reason and renderer.""" + + reasons: list[ProviderFailureReason] = [] + with patched_attr( + workflow, + "post_llm_provider_failure", + lambda _config, reason: reasons.append(reason) or 0, + ): + exit_code = workflow.post_results( + gitlab_config(), + { + "comments": [{"content": "must not publish"}], + "warnings": [ + { + "message": "private provider response: 402 Payment Required", + "provider_body": "private-response-body", + } + ], + }, + ) + + self.assertEqual(exit_code, 0) + self.assertEqual(reasons, [ProviderFailureReason.RATE_OR_SPENDING_LIMIT]) + def test_missing_gitlab_configuration_fails_closed(self) -> None: with patched_attr(workflow, "load_gitlab_config", lambda: None): exit_code = workflow.main([]) @@ -2036,32 +2205,34 @@ def capture( self.assertNotIn("exit code", notes[0]) self.assertIn("Previous OCR review comments were preserved", notes[0]) - def test_provider_failure_redacts_hostile_warning_and_never_succeeds(self) -> None: - """Redact provider diagnostics and keep provider failure non-successful.""" + def test_provider_failure_renderer_is_static_and_respects_strict_mode(self) -> None: + """Render only closed guidance while retaining advisory versus strict behavior.""" - notes: list[str] = [] - secret = "provider-secret-value" + for strict, expected in (("false", 0), ("true", 1)): + notes: list[str] = [] - def capture( - _config: Any, _title: str, body: str, _transaction: PostingTransaction - ) -> dict[str, int]: - notes.append(body) - return {"id": 1} + def capture( + _config: Any, _title: str, body: str, _transaction: PostingTransaction + ) -> dict[str, int]: + notes.append(body) + return {"id": 1} - with ( - patched_env(OCR_LLM_TOKEN=secret), - patched_attr(workflow, "post_review_note_bounded", capture), - patched_attr(workflow, "finalize_posting", lambda *_args: True), - redirect_stderr(io.StringIO()), - ): - exit_code = workflow.post_llm_provider_failure( - gitlab_config(), [f"token={secret}\n/merge"] - ) + with ( + self.subTest(strict=strict), + patched_env(OCR_STRICT_POSTING=strict), + patched_attr(workflow, "post_review_note_bounded", capture), + patched_attr(workflow, "finalize_posting", lambda *_args: True), + redirect_stderr(io.StringIO()), + ): + exit_code = workflow.post_llm_provider_failure( + gitlab_config(), ProviderFailureReason.RATE_OR_SPENDING_LIMIT + ) - self.assertEqual(exit_code, 1) - self.assertNotIn(secret, notes[0]) - self.assertNotIn("\n/merge", notes[0]) - self.assertIn(r"token=\*\*\*", notes[0]) + self.assertEqual(exit_code, expected) + self.assertIn("rate-or-spending-limit", notes[0]) + self.assertIn("cost reservation", notes[0]) + self.assertIn("OCR_LLM_MAX_COMPLETION_TOKENS", notes[0]) + self.assertIn("4096", notes[0]) def test_previous_review_cleanup_depends_only_on_coverage_completeness(self) -> None: """Clean prior review state only after complete replacement coverage.""" @@ -4859,17 +5030,16 @@ def test_billing_classifier_ignores_file_name(self) -> None: {"file": "x.py", "type": "subtask_error", "message": "402 Payment Required"}, ] - matches = result.llm_billing_failure_warnings(warnings) + reason = result.llm_billing_failure_reason(warnings) - self.assertEqual(len(matches), 1) - self.assertIn("402 Payment Required", matches[0]) + self.assertEqual(reason, ProviderFailureReason.RATE_OR_SPENDING_LIMIT) def test_billing_classifier_ignores_generic_billing_text(self) -> None: warnings = [ {"file": "x.py", "type": "subtask_error", "message": "billing module failed tests"} ] - self.assertEqual(result.llm_billing_failure_warnings(warnings), []) + self.assertIsNone(result.llm_billing_failure_reason(warnings)) def test_billing_classifier_tolerates_cyclic_warning_objects(self) -> None: """Do not recurse forever if an in-memory caller supplies a cycle.""" @@ -4877,7 +5047,7 @@ def test_billing_classifier_tolerates_cyclic_warning_objects(self) -> None: warning: dict[str, Any] = {"message": "ordinary warning"} warning["error"] = warning - self.assertEqual(result.llm_billing_failure_warnings([warning]), []) + self.assertIsNone(result.llm_billing_failure_reason([warning])) def test_billing_classifier_reads_nested_provider_error_fields(self) -> None: warnings = [ @@ -4887,26 +5057,23 @@ def test_billing_classifier_reads_nested_provider_error_fields(self) -> None: } ] - matches = result.llm_billing_failure_warnings(warnings) + reason = result.llm_billing_failure_reason(warnings) - self.assertEqual(len(matches), 1) - self.assertIn("insufficient_quota", matches[0]) + self.assertEqual(reason, ProviderFailureReason.RATE_OR_SPENDING_LIMIT) def test_billing_classifier_matches_numeric_status_without_message(self) -> None: warnings = [{"file": "x.py", "status": 402, "type": "subtask_error"}] - matches = result.llm_billing_failure_warnings(warnings) + reason = result.llm_billing_failure_reason(warnings) - self.assertEqual(len(matches), 1) - self.assertIn("status: 402", matches[0]) + self.assertEqual(reason, ProviderFailureReason.RATE_OR_SPENDING_LIMIT) def test_billing_classifier_matches_nested_numeric_code_without_message(self) -> None: warnings = [{"file": "x.py", "error": {"code": 402}}] - matches = result.llm_billing_failure_warnings(warnings) + reason = result.llm_billing_failure_reason(warnings) - self.assertEqual(len(matches), 1) - self.assertIn("code: 402", matches[0]) + self.assertEqual(reason, ProviderFailureReason.RATE_OR_SPENDING_LIMIT) def test_billing_classifier_reads_embedded_provider_json_message(self) -> None: warnings = [ @@ -4917,11 +5084,9 @@ def test_billing_classifier_reads_embedded_provider_json_message(self) -> None: } ] - matches = result.llm_billing_failure_warnings(warnings) + reason = result.llm_billing_failure_reason(warnings) - self.assertEqual(len(matches), 1) - self.assertIn("insufficient_funds", matches[0]) - self.assertIn("Insufficient user balance", matches[0]) + self.assertEqual(reason, ProviderFailureReason.RATE_OR_SPENDING_LIMIT) def test_billing_classifier_matches_status_code_shapes(self) -> None: warnings = [ @@ -4931,9 +5096,9 @@ def test_billing_classifier_matches_status_code_shapes(self) -> None: "status_code: 402", ] - matches = result.llm_billing_failure_warnings(warnings) + reason = result.llm_billing_failure_reason(warnings) - self.assertEqual(len(matches), 4) + self.assertEqual(reason, ProviderFailureReason.RATE_OR_SPENDING_LIMIT) def test_billing_classifier_ignores_non_billing_status_code(self) -> None: warnings = [ @@ -4941,7 +5106,7 @@ def test_billing_classifier_ignores_non_billing_status_code(self) -> None: '{"status_code": 200, "message": "billing report generated"}', ] - self.assertEqual(result.llm_billing_failure_warnings(warnings), []) + self.assertIsNone(result.llm_billing_failure_reason(warnings)) def test_token_usage_mapping_is_depth_bounded(self) -> None: from ocr_toolkit.result_usage import token_usage_mapping diff --git a/tests/test_provider_failure.py b/tests/test_provider_failure.py new file mode 100644 index 0000000..5e80bb4 --- /dev/null +++ b/tests/test_provider_failure.py @@ -0,0 +1,229 @@ +"""Closed projection tests for private OCR provider retry diagnostics.""" + +from __future__ import annotations + +from typing import Any + +import pytest + +from ocr_toolkit.provider_failure import ( + MAX_ATTEMPTS_PER_REQUEST, + ProviderFailureReason, + RetryReportError, + parse_retry_report_failure, + provider_failure_reason, +) + + +def _error_attempt(*, status: int, error_class: str, phase: str, number: int = 1) -> dict[str, Any]: + """Return one v1 error attempt with hostile additive diagnostics.""" + + return { + "attempt": number, + "outcome": "error", + "error_class": error_class, + "failure_phase": phase, + "status_code": status, + "request_id": "private-request-id", + "provider_body": "Authorization: Bearer private-token", + } + + +def _success_attempt(number: int) -> dict[str, Any]: + """Return one v1 successful attempt.""" + + return {"attempt": number, "outcome": "success", "status_code": 200} + + +def _request(outcome: str, attempts: list[dict[str, Any]]) -> dict[str, Any]: + """Return one logical request with raw identity fields the parser must ignore.""" + + return { + "logical_request_id": "private-logical-id", + "provider": "private-provider", + "model": "private-model", + "file_path": "/private/repository/file.py", + "task_type": "main_task", + "request_no": 1, + "outcome": outcome, + "attempts": attempts, + } + + +def _result(requests: list[dict[str, Any]], *, total_requests: int | None = None) -> dict[str, Any]: + """Return a counter-consistent retry-report result.""" + + return { + "comments": [{"content": "private finding must not be consumed"}], + "retry_report": { + "schema_version": "ocr.llm-retry-report/v1", + "total_requests": len(requests) if total_requests is None else total_requests, + "retried_requests": sum(len(request["attempts"]) > 1 for request in requests), + "total_retries": sum(len(request["attempts"]) - 1 for request in requests), + "recovered_requests": sum(request["outcome"] == "recovered" for request in requests), + "failed_requests": sum(request["outcome"] == "failed" for request in requests), + "cancelled_requests": sum(request["outcome"] == "cancelled" for request in requests), + "requests": requests, + "provider_private_extension": "private-response-body", + }, + } + + +@pytest.mark.parametrize( + ("status", "error_class", "expected"), + [ + (400, "provider", ProviderFailureReason.REQUEST_REJECTED), + (401, "authentication", ProviderFailureReason.AUTHENTICATION), + (402, "provider", ProviderFailureReason.RATE_OR_SPENDING_LIMIT), + (403, "authentication", ProviderFailureReason.AUTHORIZATION), + (404, "provider", ProviderFailureReason.ENDPOINT_OR_MODEL_NOT_FOUND), + (408, "timeout", ProviderFailureReason.TIMEOUT), + (409, "provider", ProviderFailureReason.REQUEST_REJECTED), + (413, "provider", ProviderFailureReason.REQUEST_REJECTED), + (422, "provider", ProviderFailureReason.REQUEST_REJECTED), + (429, "rate_limited", ProviderFailureReason.RATE_OR_SPENDING_LIMIT), + (500, "provider", ProviderFailureReason.PROVIDER_UNAVAILABLE), + (503, "provider", ProviderFailureReason.PROVIDER_UNAVAILABLE), + (504, "timeout", ProviderFailureReason.TIMEOUT), + (529, "overloaded", ProviderFailureReason.OVERLOADED), + ], +) +def test_http_status_matrix_maps_only_validated_attempt_fields( + status: int, error_class: str, expected: ProviderFailureReason +) -> None: + """Map the complete required HTTP matrix without reading provider text.""" + + result = _result( + [_request("failed", [_error_attempt(status=status, error_class=error_class, phase="http")])] + ) + + assert parse_retry_report_failure(result) is expected + + +@pytest.mark.parametrize( + ("attempt", "expected"), + [ + ( + _error_attempt(status=0, error_class="timeout", phase="context"), + ProviderFailureReason.TIMEOUT, + ), + ( + _error_attempt(status=0, error_class="network", phase="transport"), + ProviderFailureReason.NETWORK, + ), + ( + _error_attempt(status=200, error_class="network", phase="response_decode"), + ProviderFailureReason.INVALID_RESPONSE, + ), + ( + _error_attempt(status=200, error_class="provider", phase="response_status"), + ProviderFailureReason.INVALID_RESPONSE, + ), + ( + _error_attempt(status=0, error_class="unknown", phase="transport"), + ProviderFailureReason.UNKNOWN, + ), + ], +) +def test_non_http_failure_matrix_uses_class_and_phase( + attempt: dict[str, Any], expected: ProviderFailureReason +) -> None: + """Classify transport and response failures from closed fields only.""" + + assert parse_retry_report_failure(_result([_request("failed", [attempt])])) is expected + + +def test_cancelled_and_mixed_terminal_outcomes_are_closed() -> None: + """Prefer terminal cancellation and collapse heterogeneous failures to mixed.""" + + cancelled = _request( + "cancelled", + [_error_attempt(status=0, error_class="cancelled", phase="context")], + ) + unavailable = _request( + "failed", + [_error_attempt(status=503, error_class="provider", phase="http")], + ) + + assert parse_retry_report_failure(_result([cancelled])) is ProviderFailureReason.CANCELLED + assert ( + parse_retry_report_failure(_result([cancelled, unavailable])) is ProviderFailureReason.MIXED + ) + + +def test_recovered_only_report_does_not_create_a_failure_reason() -> None: + """Keep successful recovery private and outside failure-note semantics.""" + + recovered = _request( + "recovered", + [ + _error_attempt(status=429, error_class="rate_limited", phase="http"), + _success_attempt(2), + ], + ) + + assert parse_retry_report_failure(_result([recovered], total_requests=2)) is None + + +@pytest.mark.parametrize( + "mutate", + [ + lambda report: report.update(schema_version="future"), + lambda report: report.update(requests=[], total_requests=1, failed_requests=0), + lambda report: report.update(failed_requests=2), + lambda report: report["requests"][0]["attempts"][0].update(status_code=True), + lambda report: report["requests"][0]["attempts"][0].update(error_class="future"), + lambda report: report["requests"][0]["attempts"][0].update( + error_class="provider", status_code=401 + ), + ], +) +def test_malformed_or_impossible_reports_degrade_to_generic( + mutate: Any, +) -> None: + """Reject unsupported and internally contradictory private diagnostics.""" + + result = _result( + [ + _request( + "failed", + [_error_attempt(status=429, error_class="rate_limited", phase="http")], + ) + ] + ) + mutate(result["retry_report"]) + + with pytest.raises(RetryReportError): + parse_retry_report_failure(result) + assert provider_failure_reason(result) is None + + +def test_attempt_and_request_bounds_fail_before_projection() -> None: + """Reject per-request and aggregate lists beyond their fixed parser bounds.""" + + attempts = [ + _error_attempt(status=503, error_class="provider", phase="http", number=index) + for index in range(1, MAX_ATTEMPTS_PER_REQUEST + 2) + ] + result = _result([_request("failed", attempts)]) + + with pytest.raises(RetryReportError, match="attempts"): + parse_retry_report_failure(result) + + +def test_raw_provider_identity_and_payload_fields_do_not_affect_reason() -> None: + """Ignore additive identity and payload data even when it contains secrets.""" + + result = _result( + [ + _request( + "failed", + [_error_attempt(status=404, error_class="provider", phase="http")], + ) + ] + ) + + reason = parse_retry_report_failure(result) + + assert reason is ProviderFailureReason.ENDPOINT_OR_MODEL_NOT_FOUND + assert "private" not in reason.value diff --git a/tests/test_review_runner.py b/tests/test_review_runner.py index 0b4db5d..38ee4ac 100644 --- a/tests/test_review_runner.py +++ b/tests/test_review_runner.py @@ -1738,6 +1738,63 @@ def fake_run(argv: list[str], **kwargs: object) -> subprocess.CompletedProcess[b assert "Authorization: ***" in output.getvalue() +def test_run_review_keeps_classified_provider_stderr_private() -> None: + """Log only the closed reason when a valid private retry report exists.""" + + def fake_run(argv: list[str], **kwargs: object) -> subprocess.CompletedProcess[bytes]: + payload = { + "retry_report": { + "schema_version": "ocr.llm-retry-report/v1", + "total_requests": 1, + "retried_requests": 0, + "total_retries": 0, + "recovered_requests": 0, + "failed_requests": 1, + "cancelled_requests": 0, + "requests": [ + { + "outcome": "failed", + "provider": "private-provider", + "model": "private-model", + "file_path": "/private/path.py", + "attempts": [ + { + "attempt": 1, + "outcome": "error", + "error_class": "rate_limited", + "failure_phase": "http", + "status_code": 429, + "request_id": "private-request-id", + } + ], + } + ], + } + } + kwargs["stdout"].write(json.dumps(payload).encode()) # type: ignore[union-attr] + kwargs["stderr"].write(b"private provider body and token\n") # type: ignore[union-attr] + return subprocess.CompletedProcess(argv, 1) + + output = io.StringIO() + with ( + TemporaryDirectory() as tmp, + patched_attr(review_runner.subprocess, "run", fake_run), + redirect_stderr(output), + ): + result_path = Path(tmp) / "result.json" + stderr_path = Path(tmp) / "stderr.log" + exit_code = review_runner.run_review( + result_path, stderr_path, ["--from", "base", "--to", "head"] + ) + + assert stderr_path.read_text(encoding="utf-8") == "private provider body and token\n" + + assert exit_code == 1 + assert "rate-or-spending-limit" in output.getvalue() + assert "private provider body" not in output.getvalue() + assert "private-provider" not in output.getvalue() + + @pytest.mark.skipif(os.name == "nt", reason="synthetic executable contract is POSIX-only") @pytest.mark.parametrize("budget", ["0", "120000"]) def test_run_review_crosses_real_subprocess_boundary_with_private_artifacts( From 0aa2ad94fe99a94ed72db810ff8f71267878bd45 Mon Sep 17 00:00:00 2001 From: xeonvs <11463419+xeonvs@users.noreply.github.com> Date: Mon, 24 Aug 2026 20:47:09 +0200 Subject: [PATCH 5/8] Document provider limits and safe failures --- PLANS.md | 3 +- changelog.d/129.bugfix.md | 6 ++++ changelog.d/130.feature.md | 7 +++++ docs/configuration.md | 38 +++++++++++++++++++----- docs/engineering/test_evidence_matrix.md | 9 ++++++ docs/gitlab.md | 6 ++-- docs/operations.md | 10 ++++++- docs/security.md | 4 ++- tests/test_operations_docs.py | 37 +++++++++++++++++++++++ 9 files changed, 108 insertions(+), 12 deletions(-) create mode 100644 changelog.d/129.bugfix.md create mode 100644 changelog.d/130.feature.md diff --git a/PLANS.md b/PLANS.md index fb48557..2331f0e 100644 --- a/PLANS.md +++ b/PLANS.md @@ -39,7 +39,7 @@ Status: active, `release-required`. Target stable version: `0.8.1`. - Push the complete feature history to the Draft PR, wait for hosted checks, address evidence-driven failures through the same commit gate, then mark ready and merge through protected review. - Verify the deterministic TestPyPI development build, then prepare and merge the protected `Release v0.8.1` PR. Monitor stable TestPyPI/PyPI publication, tag, immutable GitHub Release, provenance, attestations, supported-Python installs, and immutable receipt; close tracked issues only after independent external reconciliation. -Resume point: document the completed provider configuration and safe-failure contracts, add the separate Towncrier fragments, and complete the release handoff gates. +Resume point: run the complete local release-readiness matrix, perform the overall requirements/privacy/data-flow self-review, then push the complete signed feature history to Draft PR #131. #### Current implementation evidence @@ -48,3 +48,4 @@ Resume point: document the completed provider configuration and safe-failure con - Completion-cap parsing, protocol mapping, collision rules, environment defaults, generated config, wheel/sdist installed paths, and the reusable exact wire probe are implemented and focused-green. - Canonical provider configuration now gives `configure` and `preflight` one environment snapshot and one owner for explicit protocol, API-root normalization, terminal-endpoint compatibility, secret-bearing headers, request-body controls, and auxiliary metadata URLs. Queried inference URLs require an explicit models URL; metadata-disabled preflight remains compatible. - Provider failure projection now hostile-reads the bounded private result, validates retry-report v1 counters and terminal attempt facts, and emits only a closed provider-neutral reason. Non-zero classified runs use one static GitLab renderer, keep stderr/provider fields private, preserve the previous review, publish no findings, and never reach approval; legacy billing warnings use the same renderer. The focused gate passed Ruff, full package mypy, 302 tests, and 119 subtests. +- Public configuration, GitLab operations, security boundaries, and the test-evidence matrix now distinguish the per-request completion cap, OCR-owned prompt/context ceiling, and aggregate review budget; document the `/models` non-claim and the safe 404/429 projection; and preserve the generic fallback boundary. Separate #130 feature and #129 bug-fix fragments enumerate added, changed, migration, privacy, deployment, and unchanged contracts. The documentation gate passed 49 tests, Ruff, `git diff --check`, and the rendered 0.8.1 Towncrier section. diff --git a/changelog.d/129.bugfix.md b/changelog.d/129.bugfix.md new file mode 100644 index 0000000..56557a0 --- /dev/null +++ b/changelog.d/129.bugfix.md @@ -0,0 +1,6 @@ +Provider configuration and failed-review diagnostics now share one safe boundary: + +- **Fixed:** `ocr-ci configure` and `ocr-ci preflight` now use the same explicit `OCR_LLM_PROTOCOL`, normalized credential-free HTTPS API root, headers, request controls, and auxiliary models URL. Protocol-mismatched terminal endpoints, embedded credentials, fragments, and ambiguous queried `/models` derivation fail closed. +- **Changed:** a non-zero OCR result with a valid bounded `ocr.llm-retry-report/v1` now produces a toolkit-authored provider-neutral GitLab reason and remediation hint. Runtime `404` remains `endpoint-or-model-not-found`; `429` lists throttling, spending limits, and requested-output cost reservation as possibilities without claiming which occurred. +- **Privacy:** classified failures ignore normal findings and raw provider/model identities, response bodies, codes/messages, request IDs, paths, warnings, and stderr. `OCR_POST_ERROR_DETAILS=1` does not override this boundary. +- **Unchanged:** the previous successful review is preserved, automatic approval is not attempted, and receipt v5, DLP, telemetry, severity, finding, and posting-transaction contracts do not change. diff --git a/changelog.d/130.feature.md b/changelog.d/130.feature.md new file mode 100644 index 0000000..6ef2014 --- /dev/null +++ b/changelog.d/130.feature.md @@ -0,0 +1,7 @@ +Operators can now control the LLM request's completion/output cap independently of OCR context and aggregate review budgets: + +- **Added:** optional `OCR_LLM_MAX_COMPLETION_TOKENS`, accepting decimal integers from `1` through `1000000`. Its exact default is unset, so toolkit 0.8.1 inherits the qualified OCR behavior. +- **Protocol mapping:** `openai` writes `llm.extra_body.max_completion_tokens`, `openai-responses` writes `max_output_tokens`, and `anthropic` writes `max_tokens`. +- **Migration:** an equal JSON integer already owned by `OCR_LLM_EXTRA_BODY` is deduplicated; a different or non-integer value fails configuration. Remove the duplicate field or keep the same integer in both inputs. +- **Deployment:** toolkit 0.8.1 remains on OCR 1.9.10. For a gateway that accepts short probes but rejects a full review before generation, try an explicit value such as `4096`; the toolkit does not derive it from `/models.max_completion_tokens`. +- **Unchanged:** `OCR_MAX_TOKENS_BUDGET`, OCR prompt/context `max_tokens`, receipt v5, DLP, telemetry, review outcomes, severity, findings, and approval policy are unaffected. diff --git a/docs/configuration.md b/docs/configuration.md index e79cb97..e2f4120 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -8,18 +8,18 @@ These are the complete supported toolkit-owned runtime inputs. `Required` is sco | Variable | Source / owner | Required | Exact default | Behavior | | --- | --- | --- | --- | --- | -| `OCR_LLM_URL` | Operator / `ocr-ci configure` | Yes for review | None | Absolute HTTPS LLM endpoint passed to OCR. | +| `OCR_LLM_URL` | Operator / configure and preflight | Yes for review | None | Absolute credential-free HTTPS API root or compatible terminal inference endpoint; normalized through the shared provider owner. | | `OCR_LLM_TOKEN` | Operator secret / `ocr-ci configure` | Yes for review | None | LLM credential; never written into generated context or receipts. | | `OCR_LLM_MODEL` | Operator / configure and preflight | Yes for review | None | Exact model identifier passed to OCR and optional model validation. | | `OCR_LLM_PROTOCOL` | Operator / `ocr-ci configure` | No | `openai` | Closed protocol: `openai`, `openai-responses`, or `anthropic`. | | `OCR_LLM_AUTH_HEADER` | Operator / configure and preflight | No | `Authorization` | Valid HTTP header name used for the bearer credential. | | `OCR_LLM_EXTRA_HEADERS` | Operator / configure and preflight | No | Empty object | JSON object of additional string headers; cannot duplicate the auth header. | -| `OCR_LLM_EXTRA_BODY` | Operator / `ocr-ci configure` | No | Unset | JSON object merged into the OCR LLM request configuration. | +| `OCR_LLM_EXTRA_BODY` | Operator / `ocr-ci configure` | No | Unset | JSON object merged into the OCR LLM request configuration; completion-cap field conflicts are checked against the dedicated variable. | | `OCR_LLM_MAX_COMPLETION_TOKENS` | Operator / `ocr-ci configure` | No | Unset (inherits OCR) | Positive decimal integer from `1` through `1000000`; sets the protocol-specific completion/output cap without changing prompt/context or aggregate review budgets. | | `OCR_ANTHROPIC_DISABLE_THINKING` | Operator / `ocr-ci configure` | No | `false` | With the Anthropic protocol, exact `true` adds `thinking.type=disabled`. | | `OCR_REVIEW_LANGUAGE` | Operator / shared language resolver | No | `English` | Allowed language label or BCP-47 tag used for the review. | | `OCR_LLM_VALIDATE_MODEL` | Operator / `ocr-ci preflight` | No | `false` | `true` validates through `/models`; `auto` may use the offline allowlist; false values skip validation. | -| `OCR_LLM_MODELS_URL` | Operator / `ocr-ci preflight` | No | Derived from `OCR_LLM_URL` | Explicit absolute `/models` metadata URL when validation is enabled. | +| `OCR_LLM_MODELS_URL` | Operator / `ocr-ci preflight` | No | Derived from `OCR_LLM_URL` | Explicit absolute credential-free HTTPS metadata URL when validation is enabled or inference query parameters make derivation ambiguous. | | `OCR_LLM_ALLOWED_MODELS` | Operator / `ocr-ci preflight` | No | Empty list | Comma-separated exact model identifiers for offline or `auto` validation. | | `OCR_TELEMETRY_ENABLED` | Operator / `ocr-ci configure` | No | `false` | Exact `true` enables OCR telemetry configuration. | | `OCR_TELEMETRY_CONTENT_LOGGING` | Operator / `ocr-ci configure` | No | `false` | Exact `true` enables OCR content logging; keep disabled for private review data. | @@ -39,10 +39,26 @@ These are the complete supported toolkit-owned runtime inputs. `Required` is sco | `OCR_POST_ERROR_DETAILS` | Operator / posting | No | Unset (disabled) | Only exact `1` admits the bounded redacted OCR stderr excerpt into a failure note. | | `OCR_EXIT_CODE` | Review job handoff / posting | No | `0` | OCR process exit code passed from `ocr-ci review` to `ocr-ci post`. | -`OCR_USE_ANTHROPIC` is not a compatibility alias in 0.8.0. Any presence fails configuration with an explicit request to set `OCR_LLM_PROTOCOL=anthropic`, preventing a stale false value from silently selecting the default OpenAI protocol. +Since 0.8.0, `OCR_USE_ANTHROPIC` is not a compatibility alias. Any presence fails configuration with an explicit request to set `OCR_LLM_PROTOCOL=anthropic`, preventing a stale false value from silently selecting the default OpenAI protocol. `OCR_LLM_AUTH_TOKEN`, `OPENAI_API_KEY`, and `ANTHROPIC_API_KEY` are redaction sentinels, not supported toolkit configuration. They stay in secret filtering so inherited process values cannot leak. `HOME` and `PATH` are process inputs used only for the isolated OCR home and binary lookup; `LANG`, `LC_ALL`, and `TMPDIR` are child-process mechanics set by the toolkit rather than public configuration. +### Provider endpoint and completion-cap contract + +`OCR_LLM_PROTOCOL` is authoritative; the URL never selects a protocol. `OCR_LLM_URL` accepts an API root or the matching terminal endpoint: `/chat/completions` for `openai`, `/responses` for `openai-responses`, and `/v1/messages` for `anthropic`. Configure and preflight use the same normalized API root, reject a terminal endpoint belonging to another protocol, and reject credentials or fragments embedded in either provider URL. A query is preserved for inference. Because copying it to an auxiliary endpoint is ambiguous, model validation with a queried inference URL requires an explicit `OCR_LLM_MODELS_URL`. + +`OCR_LLM_MAX_COMPLETION_TOKENS` is optional and defaults to **unset**, which inherits the qualified OCR version's behavior. It accepts a positive decimal integer from `1` through `1000000` and writes one protocol-specific field: + +| `OCR_LLM_PROTOCOL` | Generated `llm.extra_body` field | +| --- | --- | +| `openai` | `max_completion_tokens` | +| `openai-responses` | `max_output_tokens` | +| `anthropic` | `max_tokens` | + +If `OCR_LLM_EXTRA_BODY` already owns that field, an exactly equal JSON integer is deduplicated. A different value, or a boolean, string, float, or null at that field, fails configuration with a migration error; remove the duplicate field or keep the same integer in both places. Other `OCR_LLM_EXTRA_BODY` members are preserved. For example, set `OCR_LLM_MAX_COMPLETION_TOKENS=4096` when a gateway accepts short probes but rejects a full review before generation because it reserves spending against the requested output cap. + +Toolkit 0.8.1 does not derive this value from `/models.max_completion_tokens`. That metadata is a model capability boundary, not an account spending limit or proof of how a gateway reserves request cost. + ## GitLab and provider variables GitLab supplies the `CI_*` values in merge-request pipelines. The operator supplies the dedicated API token. @@ -122,11 +138,13 @@ Complete DLP-admitted metadata, generic discussions, and dynamic records remain Posting requires `GITLAB_API_TOKEN`, `CI_SERVER_URL`, `CI_PROJECT_ID`, and `CI_MERGE_REQUEST_IID`. Inline discussions additionally use GitLab diff refs and merge-request source/base SHA variables. `CI_COMMIT_SHA` remains distinct from the merge-request source SHA and is never assumed to identify the reviewed branch head. -## Posting controls +## Token controls -`OCR_POST_MODE`, `OCR_STRICT_POSTING`, `OCR_EXIT_CODE`, `OCR_MAX_POST_COMMENTS`, `OCR_MAX_RESULT_BYTES`, `OCR_POST_ERROR_DETAILS`, `OCR_POST_EMOJI`, `OCR_POST_BADGES`, and `OCR_AUTO_APPROVE` control write behavior and bounded error reporting. Human replies to bot-created discussions prevent automated ownership actions on that discussion. +Three independent controls must not be substituted for one another: -`OCR_POST_EMOJI` defaults to `true`. Set it to `false`, `0`, `no`, or `off` to disable every emoji added by the toolkit to GitLab review-health and aggregate severity/category summaries. Inline severity/category fields remain text-only in both modes. This does not rewrite emoji already contained in upstream OCR finding text. +- `OCR_LLM_MAX_COMPLETION_TOKENS` sets the provider request's per-call completion/output cap through `llm.extra_body`; its default is unset and it does not reduce prompt input. +- OCR's own `max_tokens`/`--max-tokens` controls its prompt/context ceiling. The toolkit does not add an environment alias or change that OCR-owned default. +- `OCR_MAX_TOKENS_BUDGET` is an operator-owned cost ceiling for the aggregate diff review, not a quality profile or per-request limit. `OCR_MAX_TOKENS_BUDGET` is an operator-owned cost ceiling for one diff review, not a quality profile or telemetry setting. The complete GitLab pipeline passes @@ -137,6 +155,12 @@ The toolkit publishes that run as partial and never treats it as clean or eligib for automatic approval. The cap is approximate because already-running work may finish and OCR accounts the provider-reported input plus output tokens. +## Posting controls + +`OCR_POST_MODE`, `OCR_STRICT_POSTING`, `OCR_EXIT_CODE`, `OCR_MAX_POST_COMMENTS`, `OCR_MAX_RESULT_BYTES`, `OCR_POST_ERROR_DETAILS`, `OCR_POST_EMOJI`, `OCR_POST_BADGES`, and `OCR_AUTO_APPROVE` control write behavior and bounded error reporting. Human replies to bot-created discussions prevent automated ownership actions on that discussion. + +`OCR_POST_EMOJI` defaults to `true`. Set it to `false`, `0`, `no`, or `off` to disable every emoji added by the toolkit to GitLab review-health and aggregate severity/category summaries. Inline severity/category fields remain text-only in both modes. This does not rewrite emoji already contained in upstream OCR finding text. + `OCR_POST_BADGES` controls only category/severity presentation on individual findings. The default `text` mode renders local Markdown labels and makes no external image request. Set it to `shields` to render one static Shields.io diff --git a/docs/engineering/test_evidence_matrix.md b/docs/engineering/test_evidence_matrix.md index 55b5a6c..a6ceaf2 100644 --- a/docs/engineering/test_evidence_matrix.md +++ b/docs/engineering/test_evidence_matrix.md @@ -87,6 +87,14 @@ The M5 negative suite is an attacker matrix, not merely a replay of observed rev | Session leakage | `review_runner.run_evidence_review` owner-only OCR HOME, safe absolute non-repository executable resolution, termination deferral across `finally` cleanup, and atomic result transformation | orchestration and real stdio MCP prove HOME isolation/context removal, failure/interruption cleanup and cleanup-gated receipt; inode replacement rejects races. Final real OCR completed one pass and left no context/session artifact | deterministic success/failure/interruption containment proven; real OCR success cleanup proven at the reviewed pre-remediation head | | Second review engine/model semantics | `review_runner.run_evidence_review` invokes one exact OCR process; native OCR is required for any future separate adjudication | orchestration tests assert one process/model loop; the final real run completed once with 44 evidence calls, while expected finding/context-use scenarios remain model-dependent | one-engine execution proven; model semantics remain unqualified | +## v0.8.1 provider request and failure-projection evidence + +| Requirement or boundary | Production owner and entry point | Required observable result | Evidence | Double boundary and claim limit | State | +| --- | --- | --- | --- | --- | --- | +| Explicit completion/output cap reaches OCR 1.9.10 wire request | `provider_config.provider_config_from_environment` -> `configure.build_config_updates` -> generated OCR config -> actual `ocr review` | unset preserves OCR's observed `max_completion_tokens=58888`; explicit `4096` replaces only that wire field | checksum-verified Darwin arm64 OCR 1.9.10 no-LLM local-gateway probe; reusable compatibility probe; generated-config and installed wheel/sdist tests | controlled local gateway is beyond OCR's real HTTP client and observes request shape only; it does not prove model quality, spending policy, or that `4096` suits every provider | proven for exact qualified OCR wire contract | +| Configure/preflight provider boundary stays canonical and secret-safe | `provider_config.ProviderConfig` shared by configure and preflight | explicit protocol, normalized HTTPS root/terminal endpoint, query handling, auxiliary URL, headers, and request-body controls agree; credentials/fragments/mismatches fail closed | `test_runtime_helpers.py`, `test_environment_contract.py`, and installed-artifact tests | local metadata peers and environment fixtures prove parser/transport wiring, not a live provider's endpoint policy | proven | +| Non-zero retry diagnostics become a closed GitLab failure note | bounded result owner -> `provider_failure.parse_retry_report_failure` -> posting renderer | only validated class/phase/status/terminal outcome selects static text; classified raw result/stderr stays private; previous review remains; findings and approval are unreachable | `test_provider_failure.py`, `test_posting_helpers.py`, and `test_review_runner.py`, including the required status/failure/malformed/oversize/privacy matrices | result fixtures and mocked GitLab writes prove strict parsing, control flow, and rendered payload; no live GitLab write or provider semantic claim | proven for deterministic toolkit policy | + ## Complete suite module audit Every top-level test module is classified below. A module can contain more than one evidence class; the strongest class applies only to the named boundary, never to all tests in that file. @@ -121,6 +129,7 @@ Every top-level test module is classified below. A module can contain more than | `test_posting_approval.py` | approval policy, exact-SHA request construction, ordering and fail-closed workflow | API owners are replaced; no live GitLab approval integration is claimed because writes are unsafe in tests | | `test_posting_helpers.py` | pure formatting/workflow policy, real Git reads, real local HTTP create serialization and paginated reconciliation reads, real result-file boundaries | mocked GitLab API owner cases prove response/error/workflow behavior only; local peers prove transport/completeness mechanics, not live GitLab semantics | | `test_posting_suggestions.py` | pure proof-bound suggestion decisions | fake readers are collaborators beyond the pure decision owner; no Git blob integration claim | +| `test_provider_failure.py` | strict bounded retry-report v1 parsing and closed provider-neutral reason projection | fixture records prove the toolkit parser only; actual OCR request behavior is separately observed by the compatibility probe | | `test_python_support.py` | static metadata/CI support range | supported interpreters are proven by the quality matrix, not this test alone | | `test_quality_script.py` | real synthetic Git history for Gitleaks range plus static wrapper policy | fake scanner proves wrapper invocation/range, not secret-detection efficacy; pinned real Gitleaks runs before push | | `test_release_authorization.py` | pure authorization rules plus real bounded helper subprocess/filesystem behavior | API response fixtures do not prove GitHub state; release closure requires live readback | diff --git a/docs/gitlab.md b/docs/gitlab.md index 342f409..e3b7cae 100644 --- a/docs/gitlab.md +++ b/docs/gitlab.md @@ -18,7 +18,7 @@ The complete variable inventory, owner, requirement, exact default, and behavior The public pipeline stores the OCR binary checksum as the non-secret `OCR_SHA256` pin. Store actual credentials as masked, protected CI variables; do not place their values in YAML, command arguments, repository evidence, or the generated bootstrap. GitLab job tokens are not accepted for posting. -`OCR_REVIEW_LANGUAGE` defaults to `English`; `Russian` is one example of an explicit review language. The example passes `OCR_MAX_TOOLS=30`, matching OCR 1.9.10's per-file tool-round default; increase it deliberately only when a reviewed repository needs more tool interaction. `OCR_MAX_TOKENS_BUDGET` defaults to `0`, meaning unlimited; a positive budget may stop dispatch and produce an explicitly partial, automatic-approval-ineligible review. +`OCR_REVIEW_LANGUAGE` defaults to `English`; `Russian` is one example of an explicit review language. The example passes `OCR_MAX_TOOLS=30`, matching OCR 1.9.10's per-file tool-round default; increase it deliberately only when a reviewed repository needs more tool interaction. `OCR_MAX_TOKENS_BUDGET` defaults to `0`, meaning unlimited; a positive budget may stop dispatch and produce an explicitly partial, automatic-approval-ineligible review. `OCR_LLM_MAX_COMPLETION_TOKENS` defaults to unset and separately controls only the provider request's completion/output cap; `4096` is a practical explicit value for gateways that reserve spending against a larger requested maximum. ## Choose one operating mode @@ -42,7 +42,7 @@ Use a dedicated bot account that is not the merge-request author. Give its proje Begin with a manual advisory job and `OCR_AUTO_APPROVE=false`. Enable strict posting or approval only after the project has reviewed published results, bot permissions, exact receipt gates, and all source-data boundaries. Keep result, stderr, evidence, generated OCR configuration, context stores, adapter scratch space, and OCR sessions private to the runner and out of public artifacts. The local-only `ocr-ci review --preserve-private-artifacts` diagnostic is rejected by the validated GitLab merge-request profile and must not be added to a CI job. -Before treating the advisory job as a required gate, run `ocr llm test` with the same generated OCR configuration and protected credential path. `ocr-ci preflight` always checks that the required toolkit inputs exist and can optionally validate model metadata through `/models`, but that metadata read is not a full review request and cannot guarantee that a gateway credential, protocol, or deployment will accept the later conversation. Keep `allow_failure` only when a missing review is intentionally advisory; a green pipeline with an allowed-to-fail OCR job is not evidence that OCR produced a usable review. +Before treating the advisory job as a required gate, run `ocr llm test` with the same generated OCR configuration and protected credential path. `ocr-ci preflight` always checks that the required toolkit inputs exist and can optionally validate model metadata through `/models`, but that metadata read is not a full review request and cannot guarantee that a gateway credential, protocol, deployment, spending policy, or requested output cap will accept the later conversation. The toolkit never derives `OCR_LLM_MAX_COMPLETION_TOKENS` from `/models.max_completion_tokens`. Keep `allow_failure` only when a missing review is intentionally advisory; a green pipeline with an allowed-to-fail OCR job is not evidence that OCR produced a usable review. The toolkit authenticates the token owner with live `GET /user`. No configured bot ID or username is trusted. The returned ID owns note/fingerprint checks; the validated username owns exact mention-command parsing. @@ -56,6 +56,8 @@ The toolkit authenticates the token owner with live `GET /user`. No configured b Remediation text is untrusted review history. It may locate a claim that OCR must re-check against current code and tests, but it cannot change severity, prove a fix, suppress or resolve a finding, issue a lifecycle command, or authorize approval. Any admitted remediation record therefore makes the review comment-only. DLP-clean non-remediation context does not independently disable an otherwise eligible receipt; a DLP rejection cannot make approval easier. +Provider configuration is forge-neutral. Configure and preflight share one normalized absolute HTTPS API root, explicit protocol, headers, request-body controls, and optional models URL. A protocol-mismatched terminal endpoint, embedded credential, or fragment fails before OCR. On a classified provider failure, `post` publishes only a static safe reason and guidance; raw provider/model fields, response bodies, request IDs, paths, warnings, and stderr remain private. A `429` note lists rate, spending, and requested-cap reservation as possibilities and may suggest `OCR_LLM_MAX_COMPLETION_TOKENS=4096` without claiming a cause. The previous successful review remains visible, no failed-result findings are posted, and approval is not attempted. + When a merge request introduces a repository-owned OCR rules path absent from both trusted baselines, `review` stops before OCR and `post` may publish only the static setup-pending message after hostile identity validation. The source file never becomes policy evidence for its own merge request. ## Reviewer commands and no-commit reruns diff --git a/docs/operations.md b/docs/operations.md index 2171fe2..670b746 100644 --- a/docs/operations.md +++ b/docs/operations.md @@ -34,6 +34,8 @@ suppressed. `OCR_MAX_TOKENS_BUDGET` can set an aggregate input-plus-output token ceiling for the OCR diff review. The default `0` is unlimited. A positive ceiling is approximate rather than a hard billing cutoff because already-running work may complete; when it stops further dispatch, completed findings remain publishable and unreviewed files stay explicit as budget-attributed failed coverage. Such a run is partial and cannot automatically approve. +This aggregate budget is separate from both OCR's prompt/context `max_tokens` ceiling and the provider request's completion/output cap. The toolkit does not add an environment alias for OCR's prompt/context control. `OCR_LLM_MAX_COMPLETION_TOKENS` defaults to unset and, when set, overlays only the protocol-specific output field. A gateway may reserve cost against that requested maximum before generation even when the eventual response would be short. The `/models` capability value does not reveal an account spending limit or reservation policy, so the toolkit never selects the cap from it automatically. + The published GitLab example also passes `OCR_MAX_TOOLS=30`, matching OCR 1.9.10's maximum tool-request rounds per file. A file that reaches this bound without the model finishing is explicit failed coverage, so the review is partial. Raise the value only after inspecting the repository/model behavior; repeated exhaustion at a deliberately raised bound is a diagnostic signal, not a reason to increase the ceiling indefinitely. The outcome wording distinguishes skipped, complete, complete-with-warnings, incomplete, token-budget, and failed reviews while preserving the finding state in that same line. A complete clean review is visibly positive; a complete review with findings or only reviewer-suppressed findings is neutral; warning, partial, budget, and failed states never look clean. Findings withheld by the posting limit remain counted even when the limit allows no individual finding note. Recommended focus areas ranks only its copy of already-published findings by the closed severity, category, safe repository location, and stable-identity order before its existing display cap; inline and fallback discussion order, suppression, counts, security focus, and approval policy remain unchanged. OCR 1.8.5 and later manifest failures provide the canonical failed-file receipt; legacy warnings are a bounded fallback, and `summary.files_reviewed` is never treated as proof of successful coverage. Technical details label the aggregate as all OCR tool calls, separately label toolkit-verified MCP-server calls, and show built-in evidence `summary`/`list`/`get` counts only after exact reconciliation; unavailable attribution is not displayed as zero. Zero-valued token counters and configured-but-unused MCP servers are omitted. Token usage renders only validated input/output/cached/reasoning/total/derived-other buckets; malformed or contradictory counters are unavailable and unknown provider keys are not published. Status and aggregate semantic-category emoji are enabled by default and can be disabled together with `OCR_POST_EMOJI=false`; finding labels remain text unless their separate badge mode is enabled. @@ -126,7 +128,13 @@ Suppression checks both the recorded inline position and compatible fingerprints ## OCR diagnostics -Run OCR through `ocr-ci review --result PATH --stderr PATH -- ...`. This wrapper does not post to GitLab: it creates private artifacts, acquires enriched context when selected, asks the exact resolved and preflight-qualified OCR executable to preview the production refs/rules/selection/background without an LLM, then runs the model review only if OCR accepts that background. OCR owns the current warning and rejection thresholds; the toolkit has no threshold setting. A recognized soft warning appears in the CI log and result summary and makes automatic approval ineligible. A recognized hard character/file-size rejection stops before the model and lets `ocr-ci post` publish only a static numeric failure summary; the OCR path and raw diagnostic remain private. Unknown preview failures use the generic fail-closed diagnostic path. The ordinary review still validates the same background, the wrapper validates the complete output, and context/session/configuration data is removed. On ordinary failure it prints only a bounded redacted stderr excerpt to the runner log. Pass the paths and captured exit code to `ocr-ci post` afterward. Set `OCR_POST_ERROR_DETAILS=1` only when that safe excerpt should also appear in the merge-request failure note. Cleanup uncertainty blocks result publication. DLP atomically converts unsafe publication output into a safe `completed_with_errors` subset, but sanitizes unsafe private-only result fields without discarding an otherwise valid manifest or finding set. Safe findings are posted, unsafe finding content/warnings and unsafe optional fields are omitted, previous OCR comments remain, and matching prior findings are consumed one-for-one rather than duplicated. Receipt v5 and the `ocr.publication-dlp-signal/v2` marker distinguish `private-sanitized`, where the canonical published and approval-relevant projection is unchanged, from partial approval-ineligible `publication-filtered`. The same count-only JSON is logged as `OCR toolkit telemetry event` for optional CI collection/alerting. It is not an OTLP/network exporter and contains no rejected value or location. Never interpret a filtered subset as a full review. +Run OCR through `ocr-ci review --result PATH --stderr PATH -- ...`. This wrapper does not post to GitLab: it creates private artifacts, acquires enriched context when selected, asks the exact resolved and preflight-qualified OCR executable to preview the production refs/rules/selection/background without an LLM, then runs the model review only if OCR accepts that background. OCR owns the current warning and rejection thresholds; the toolkit has no threshold setting. A recognized soft warning appears in the CI log and result summary and makes automatic approval ineligible. A recognized hard character/file-size rejection stops before the model and lets `ocr-ci post` publish only a static numeric failure summary; the OCR path and raw diagnostic remain private. Unknown preview failures use the generic fail-closed diagnostic path. The ordinary review still validates the same background, the wrapper validates the complete output, and context/session/configuration data is removed. On an unclassified ordinary failure it prints only a bounded redacted stderr excerpt to the runner log; a classified provider failure keeps that excerpt private. Pass the paths and captured exit code to `ocr-ci post` afterward. Set `OCR_POST_ERROR_DETAILS=1` only when the generic path's safe excerpt should also appear in the merge-request failure note. Cleanup uncertainty blocks result publication. DLP atomically converts unsafe publication output into a safe `completed_with_errors` subset, but sanitizes unsafe private-only result fields without discarding an otherwise valid manifest or finding set. Safe findings are posted, unsafe finding content/warnings and unsafe optional fields are omitted, previous OCR comments remain, and matching prior findings are consumed one-for-one rather than duplicated. Receipt v5 and the `ocr.publication-dlp-signal/v2` marker distinguish `private-sanitized`, where the canonical published and approval-relevant projection is unchanged, from partial approval-ineligible `publication-filtered`. The same count-only JSON is logged as `OCR toolkit telemetry event` for optional CI collection/alerting. It is not an OTLP/network exporter and contains no rejected value or location. Never interpret a filtered subset as a full review. + +When OCR exits nonzero with a valid bounded `ocr.llm-retry-report/v1`, the toolkit reads only its closed error class, failure phase, terminal outcome, and HTTP status. It maps those facts to `authentication`, `authorization`, `rate-or-spending-limit`, `overloaded`, `timeout`, `network`, `endpoint-or-model-not-found`, `request-rejected`, `provider-unavailable`, `invalid-response`, `cancelled`, `mixed`, or `unknown`, then writes a completely toolkit-authored note. A runtime `404` remains `endpoint-or-model-not-found` because safely distinguishing the endpoint from the model would require trusting the raw response body. + +For `429`, the note says that ordinary throttling, an account or API-key spending limit, or cost reservation from the requested output cap are all possible. Retry later and check provider limits. If short probes pass while a full review fails before generation, try an explicit `OCR_LLM_MAX_COMPLETION_TOKENS`, for example `4096`; this is a diagnostic workaround, not a claim that the cap was the cause. + +Raw provider/model identities, response bodies, error codes and messages, request IDs, paths, warnings, and stderr remain in owner-only private artifacts for a classified provider failure. `OCR_POST_ERROR_DETAILS=1` cannot add them to that note. Normal findings from the failed result are ignored, the previous successful review is preserved, and automatic approval is not attempted. Missing, oversized, malformed, or internally contradictory retry reports keep the existing generic failure path instead of guessing a classification. For a local diagnosis, add `--preserve-private-artifacts` before the `--` separator. The command retains the isolated OCR home plus repository-local private evidence/context artifacts, prints only their paths, and deliberately leaves the raw OCR result without receipt v5; do not pass that result to `ocr-ci post`. It writes `.review-context/private-dlp-decisions.json` with value-free bounded JSON paths, closed reason and detector subtype, size units, and SHA-256 for up to 1,000 rejected keys/values, plus explicit truncation and omitted-decision counts. Use matching digests to identify one repeated technical value and inspect the retained raw result locally before deciding whether a conservative PII match is a false positive; the sidecar itself is not proof that content is safe. These owner-only files can contain source/provider context, prompts, model responses, tool arguments/results, and generated runtime configuration. Inspect them locally, keep them out of commits and shared artifacts, then delete them after extracting the needed evidence. Ordinary runs do not retain this attribution. The authoritative GitLab merge-request profile rejects the flag before OCR execution and performs normal cleanup; an arbitrary `CI=true` value neither grants nor blocks the local mode. diff --git a/docs/security.md b/docs/security.md index 1ad8955..902aed7 100644 --- a/docs/security.md +++ b/docs/security.md @@ -23,7 +23,7 @@ An operator controls CI configuration, direct MCP servers, adapter commands/endp 5. Typed repository evidence crosses into the compact bootstrap and mandatory read-only evidence MCP. During enriched OCR, only the same built-in process's fixed `context_list`/`context_get` can read committed local handles; adapter/provider network paths are absent from the model loop. 6. Persisted evidence, context, results, OCR configuration, private pre-execution status, and receipts re-enter hostile parsing. OCR runs in a fresh isolated home; session, configuration, adapter, and context data crosses deterministic cleanup before a result becomes publishable. 7. In the direct-MCP GitLab profile, operator-configured external MCP is remote HTTPS only; developer-local execution may pass explicit stdio command/setup configuration. Server-authored descriptions and schemas cross into plan and main model context; model-generated arguments cross to allowed tools; textual responses cross back to the model and OCR session. This path is separate from M5 adapters. -8. The complete OCR result crosses independent publication DLP, deterministic GitLab publication, suppression, and receipt-v5 approval policy. +8. The complete successful OCR result crosses independent publication DLP, deterministic GitLab publication, suppression, and receipt-v5 approval policy. On non-zero OCR exit with a valid bounded `ocr.llm-retry-report/v1`, only a closed provider-neutral reason may cross the separate strict parser into a toolkit-authored failure note; in that classified path raw result fields and stderr remain private, normal findings are ignored, and approval is unreachable. Missing or invalid retry diagnostics use the pre-existing generic failure boundary. 9. A release candidate crosses protected-base authorization, publication, provenance, and live readback. ### Security objectives @@ -37,6 +37,7 @@ An operator controls CI configuration, direct MCP servers, adapter commands/endp - Adapter credentials and services enforce tenant/object/operation/field authorization independently; reference syntax and authentication alone never authorize a resource. - Context policy cannot come from the source branch, context budgets cannot evict repository evidence, and model-facing context cannot add a network, arbitrary ID/URL, search, traversal, or write path. - Publication and retention are independent from retrieval/model egress. Cleanup uncertainty blocks publication. DLP selects exact posting sinks separately from private OCR metadata. Unsafe sinks produce an explicit safe partial result; unsafe private-only keys/values are removed or replaced before retention without discarding a still-valid manifest or safe findings. Both paths atomically destroy the rejected value/location and expose only closed counts. Publication filtering preserves prior review state, matches repeated fingerprints one-for-one, and blocks approval; private-only sanitization may continue through all existing approval gates only after exact canonical equivalence. +- Classified provider failure reporting consumes only validated retry class, phase, terminal outcome, and HTTP status. Provider/model identity, URL, response body, error code or message, request ID, path, warning text, token, and stderr cannot enter the static note or become a receipt, DLP, telemetry, severity, finding, or approval signal. - Provider mutations bind reviewed identity where supported; ambiguous inline creates use one author-bound readback without retry, and unresolved ambiguity preserves prior state. - Secrets remain outside repository-controlled context, public notes, fixtures, and release artifacts. @@ -65,6 +66,7 @@ Receipt v5 records the bounded configured capability inventory and positive call - Review context uses a closed `off|metadata|enriched` selector. `off` retains only validated source/protected-target/author identities; `metadata` admits bounded MR fields; `enriched` requires the immutable protected policy and admits only stable bounded discussion/adapter projections. Source policy, unknown fields/classes, raw display identities, arbitrary URLs/IDs, tokens, and ambient environment values cannot expand it. - Generated Markdown neutralizes controls and GitLab quick actions. Actionable suggestions require exact reviewed-head proof; unverifiable replacements retain prose only. - Result and provider reads have byte limits; notes enforce character and UTF-8 byte limits. Position-bearing inline creates reserve independent unguessable markers, classify closed outcomes, and perform at most one complete author-bound reconciliation read with no retry. +- LLM provider URLs are normalized by one forge-neutral owner shared by configuration and preflight; only credential-free absolute HTTPS roots or protocol-compatible terminal endpoints are accepted. Explicit protocol remains authoritative, and ambiguous auxiliary URL derivation fails closed. - Automatic approval binds the exact synchronized reviewed head and MR author from receipt v5, skips self-approval, and never removes an existing approval. Partial, warning, non-v5, publication-filtered, omitted, degraded metadata, DLP-rejected selected-source, required context degradation, admitted remediation-context, or direct external-MCP runs are ineligible. - Human replies are ownership boundaries. Merge-request source SHA, protected-target policy SHA, and merge-result SHA remain distinct. - The evidence engine stores recursively redacted typed facts/deltas in owner-only files and serves a closed bounded network-independent MCP. Absence supports a negative claim only for applicable complete scope. diff --git a/tests/test_operations_docs.py b/tests/test_operations_docs.py index a128a6b..5bad27e 100644 --- a/tests/test_operations_docs.py +++ b/tests/test_operations_docs.py @@ -165,6 +165,43 @@ def test_aggregate_review_budget_is_explicit_and_never_looks_complete() -> None: assert "approximate" in configuration +def test_completion_cap_and_provider_failure_boundaries_are_public() -> None: + """Keep token ownership, safe failure projection, and migration behavior explicit.""" + + operations = OPERATIONS.read_text(encoding="utf-8") + configuration = CONFIGURATION.read_text(encoding="utf-8") + gitlab = GITLAB_GUIDE.read_text(encoding="utf-8") + security = (PROJECT_ROOT / "docs" / "security.md").read_text(encoding="utf-8") + + for document in (configuration, operations, gitlab): + assert "OCR_LLM_MAX_COMPLETION_TOKENS" in document + assert "4096" in document + assert "OCR_MAX_TOKENS_BUDGET" in document + assert "/models" in document + for field in ("max_completion_tokens", "max_output_tokens", "max_tokens"): + assert field in configuration + for phrase in ( + "defaults to **unset**", + "positive decimal integer from `1` through `1000000`", + "exactly equal JSON integer is deduplicated", + "fails configuration with a migration error", + "The toolkit does not add an environment alias", + ): + assert phrase in configuration + + for phrase in ( + "endpoint-or-model-not-found", + "cost reservation from the requested output cap", + "not a claim that the cap was the cause", + "`OCR_POST_ERROR_DETAILS=1` cannot add them", + "the previous successful review is preserved", + "automatic approval is not attempted", + ): + assert phrase in operations + assert "closed provider-neutral reason may cross the separate strict parser" in security + assert "receipt, DLP, telemetry, severity, finding, or approval signal" in security + + def test_finding_badge_contract_is_opt_in_and_privacy_explicit() -> None: operations = OPERATIONS.read_text(encoding="utf-8") configuration = CONFIGURATION.read_text(encoding="utf-8") From df9bde6bae8cbe337a5b21d11722d9182e5f781a Mon Sep 17 00:00:00 2001 From: xeonvs <11463419+xeonvs@users.noreply.github.com> Date: Mon, 24 Aug 2026 20:53:17 +0200 Subject: [PATCH 6/8] Harden provider boundary parsers --- PLANS.md | 1 + src/ocr_toolkit/provider_config.py | 8 ++++++++ src/ocr_toolkit/provider_failure.py | 6 +++++- tests/test_provider_failure.py | 1 + tests/test_runtime_helpers.py | 18 +++++++++++++++++- 5 files changed, 32 insertions(+), 2 deletions(-) diff --git a/PLANS.md b/PLANS.md index 2331f0e..58da765 100644 --- a/PLANS.md +++ b/PLANS.md @@ -49,3 +49,4 @@ Resume point: run the complete local release-readiness matrix, perform the overa - Canonical provider configuration now gives `configure` and `preflight` one environment snapshot and one owner for explicit protocol, API-root normalization, terminal-endpoint compatibility, secret-bearing headers, request-body controls, and auxiliary metadata URLs. Queried inference URLs require an explicit models URL; metadata-disabled preflight remains compatible. - Provider failure projection now hostile-reads the bounded private result, validates retry-report v1 counters and terminal attempt facts, and emits only a closed provider-neutral reason. Non-zero classified runs use one static GitLab renderer, keep stderr/provider fields private, preserve the previous review, publish no findings, and never reach approval; legacy billing warnings use the same renderer. The focused gate passed Ruff, full package mypy, 302 tests, and 119 subtests. - Public configuration, GitLab operations, security boundaries, and the test-evidence matrix now distinguish the per-request completion cap, OCR-owned prompt/context ceiling, and aggregate review budget; document the `/models` non-claim and the safe 404/429 projection; and preserve the generic fallback boundary. Separate #130 feature and #129 bug-fix fragments enumerate added, changed, migration, privacy, deployment, and unchanged contracts. The documentation gate passed 49 tests, Ruff, `git diff --check`, and the rendered 0.8.1 Towncrier section. +- Overall parser-boundary review found and closed three narrow fail-closed gaps before publication: completion-cap length is bounded before integer conversion, embedded URL whitespace is rejected before `urllib` normalization, and JSON booleans cannot satisfy retry attempt numbering. Ruff, full package mypy, 135 focused tests, 100 subtests, and `git diff --check` pass for the correction. diff --git a/src/ocr_toolkit/provider_config.py b/src/ocr_toolkit/provider_config.py index acb54a7..0b3eaa6 100644 --- a/src/ocr_toolkit/provider_config.py +++ b/src/ocr_toolkit/provider_config.py @@ -152,6 +152,10 @@ def _parse_completion_cap(value: str) -> int | None: if not value: return None + if len(value) > len(str(MAX_COMPLETION_TOKENS_LIMIT)): + raise ProviderConfigError( + f"OCR_LLM_MAX_COMPLETION_TOKENS must be at most {MAX_COMPLETION_TOKENS_LIMIT}" + ) if POSITIVE_DECIMAL_RE.fullmatch(value) is None: raise ProviderConfigError( "OCR_LLM_MAX_COMPLETION_TOKENS must be a positive decimal integer" @@ -167,6 +171,10 @@ def _parse_completion_cap(value: str) -> int | None: def _parse_https_url(value: str, name: str) -> SplitResult: """Parse one absolute credential-free HTTPS URL with no fragment.""" + if any(character.isspace() for character in value): + raise ProviderConfigError( + f"{name} must be an absolute HTTPS URL without embedded credentials or a fragment" + ) try: parsed = urlsplit(value) port = parsed.port diff --git a/src/ocr_toolkit/provider_failure.py b/src/ocr_toolkit/provider_failure.py index a89b7f9..d8ae52f 100644 --- a/src/ocr_toolkit/provider_failure.py +++ b/src/ocr_toolkit/provider_failure.py @@ -153,7 +153,11 @@ def _request_reason(request: Mapping[str, Any]) -> ProviderFailureReason | None: raise RetryReportError("retry report request attempts are invalid") reasons: list[ProviderFailureReason | None] = [] for index, attempt in enumerate(attempts, start=1): - if not isinstance(attempt, dict) or attempt.get("attempt") != index: + if ( + not isinstance(attempt, dict) + or type(attempt.get("attempt")) is not int + or attempt.get("attempt") != index + ): raise RetryReportError("retry report attempt order is invalid") reasons.append(_attempt_reason(attempt)) if outcome == "succeeded": diff --git a/tests/test_provider_failure.py b/tests/test_provider_failure.py index 5e80bb4..930e51f 100644 --- a/tests/test_provider_failure.py +++ b/tests/test_provider_failure.py @@ -171,6 +171,7 @@ def test_recovered_only_report_does_not_create_a_failure_reason() -> None: lambda report: report.update(schema_version="future"), lambda report: report.update(requests=[], total_requests=1, failed_requests=0), lambda report: report.update(failed_requests=2), + lambda report: report["requests"][0]["attempts"][0].update(attempt=True), lambda report: report["requests"][0]["attempts"][0].update(status_code=True), lambda report: report["requests"][0]["attempts"][0].update(error_class="future"), lambda report: report["requests"][0]["attempts"][0].update( diff --git a/tests/test_runtime_helpers.py b/tests/test_runtime_helpers.py index 204665c..4959543 100644 --- a/tests/test_runtime_helpers.py +++ b/tests/test_runtime_helpers.py @@ -429,6 +429,22 @@ def test_provider_config_rejects_fragments_and_hides_secret_fields_from_repr(sel self.assertNotIn("private-header", rendered) self.assertNotIn("private-body", rendered) + def test_provider_config_rejects_embedded_url_whitespace(self) -> None: + """Reject URL characters that urllib would otherwise silently normalize.""" + + for raw_url in ( + "https://gate\nway.example/v1", + "https://gateway.example/v1\t/models", + "https://gateway.example/v1 /models", + ): + with ( + self.subTest(raw_url=raw_url), + self.assertRaisesRegex(provider_config.ProviderConfigError, "absolute HTTPS URL"), + ): + provider_config.provider_config_from_environment( + {"OCR_LLM_PROTOCOL": "openai", "OCR_LLM_URL": raw_url} + ) + def test_runtime_config_rejects_removed_anthropic_switch_with_migration(self) -> None: for legacy_value in ("", "false", "true"): with ( @@ -570,7 +586,7 @@ def test_runtime_config_rejects_conflicting_completion_cap(self) -> None: ocr_configure.build_config_updates() def test_runtime_config_rejects_invalid_completion_caps(self) -> None: - for value in ("0", "-1", "+1", "1.5", "1000001"): + for value in ("0", "-1", "+1", "1.5", "1000001", "9" * 5000): with ( self.subTest(value=value), patched_env( From 53c6374da216bf88aa27ba6dd146b3322d8c8463 Mon Sep 17 00:00:00 2001 From: xeonvs <11463419+xeonvs@users.noreply.github.com> Date: Mon, 24 Aug 2026 20:53:52 +0200 Subject: [PATCH 7/8] Reconcile v0.8.1 implementation plan --- PLANS.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/PLANS.md b/PLANS.md index 58da765..0216171 100644 --- a/PLANS.md +++ b/PLANS.md @@ -6,7 +6,7 @@ Use this file for active or blocked repository work. Update it before implementa ### Release 0.8.1: completion cap and safe LLM provider failures -Status: active, `release-required`. Target stable version: `0.8.1`. +Status: implementation complete, local exact-head validation pending; `release-required`. Target stable version: `0.8.1`. #### Outcome and scope @@ -39,7 +39,7 @@ Status: active, `release-required`. Target stable version: `0.8.1`. - Push the complete feature history to the Draft PR, wait for hosted checks, address evidence-driven failures through the same commit gate, then mark ready and merge through protected review. - Verify the deterministic TestPyPI development build, then prepare and merge the protected `Release v0.8.1` PR. Monitor stable TestPyPI/PyPI publication, tag, immutable GitHub Release, provenance, attestations, supported-Python installs, and immutable receipt; close tracked issues only after independent external reconciliation. -Resume point: run the complete local release-readiness matrix, perform the overall requirements/privacy/data-flow self-review, then push the complete signed feature history to Draft PR #131. +Resume point: run the complete local release-readiness matrix against this plan-reconciled head, then push the complete signed feature history to Draft PR #131 and wait for hosted checks. #### Current implementation evidence From 4bfd9fc760143cb707c43aeca4f56db2a3096c13 Mon Sep 17 00:00:00 2001 From: xeonvs <11463419+xeonvs@users.noreply.github.com> Date: Mon, 24 Aug 2026 21:06:55 +0200 Subject: [PATCH 8/8] Deduplicate protected validation stages --- .github/workflows/build.yml | 3 --- .github/workflows/ci.yml | 29 ++++++++++------------------- .github/workflows/codeql.yml | 2 -- .github/workflows/security.yml | 2 -- .github/workflows/testpypi.yml | 2 -- PLANS.md | 7 +++++-- changelog.d/132.maintenance.md | 5 +++++ docs/development.md | 6 +++--- docs/release.md | 2 +- tests/test_operations_docs.py | 24 ++++++++++++++++++++++-- tests/test_quality_script.py | 21 +++++++++++++++++++-- tests/test_testpypi_preview.py | 15 +++++++++++++-- 12 files changed, 78 insertions(+), 40 deletions(-) create mode 100644 changelog.d/132.maintenance.md diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 2c3b2bc..3ea2beb 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -4,9 +4,6 @@ on: workflow_dispatch: pull_request: branches: [main] - push: - branches: [main] - paths: ["pyproject.toml", "uv.lock", "src/**", "scripts/install_local_artifact.py", "README.md", "LICENSE"] permissions: contents: read diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index cc41f72..da8c839 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -2,8 +2,6 @@ name: CI on: pull_request: - push: - branches: [main] workflow_dispatch: permissions: @@ -20,11 +18,11 @@ jobs: fail-fast: false matrix: include: - - { os: ubuntu-latest, python: "3.12" } - - { os: ubuntu-latest, python: "3.13" } - - { os: ubuntu-latest, python: "3.14" } - - { os: macos-latest, python: "3.12" } - - { os: macos-latest, python: "3.14" } + - { os: ubuntu-latest, python: "3.12", coverage: false } + - { os: ubuntu-latest, python: "3.13", coverage: false } + - { os: ubuntu-latest, python: "3.14", coverage: true } + - { os: macos-latest, python: "3.12", coverage: false } + - { os: macos-latest, python: "3.14", coverage: false } runs-on: ${{ matrix.os }} steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -35,12 +33,16 @@ jobs: with: version: "0.12.0" enable-cache: true - save-cache: ${{ github.event_name == 'push' && github.ref == 'refs/heads/main' }} + save-cache: false - uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 with: python-version: ${{ matrix.python }} - run: uv sync --frozen + - name: Test + if: ${{ !matrix.coverage }} + run: uv run pytest -q - name: Test with boundary coverage gates + if: ${{ matrix.coverage }} run: | uv run pytest --cov=ocr_toolkit --cov-report=term-missing --cov-fail-under=85 uv run coverage report --include=src/ocr_toolkit/ocr_result.py,src/ocr_toolkit/preflight.py --fail-under=80 @@ -66,14 +68,3 @@ jobs: - run: uv run ruff format --check . - run: uv run ruff check . - run: uv run mypy src/ocr_toolkit - - run: uv run python -c "import shutil; shutil.rmtree('dist', ignore_errors=True)" - - run: uv run python -m build - - run: uv run twine check dist/* - - run: python -m venv /tmp/wheel-smoke && /tmp/wheel-smoke/bin/pip install dist/*.whl && /tmp/wheel-smoke/bin/ocr-ci --help - - run: | - python -m venv /tmp/sdist-smoke - python scripts/install_local_artifact.py \ - --python /tmp/sdist-smoke/bin/python \ - --artifact "$(find dist -maxdepth 1 -name '*.tar.gz' -print -quit)" \ - --requirements /tmp/sdist-smoke-requirements.txt - /tmp/sdist-smoke/bin/ocr-ci --help diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index ba9bf59..5f8d739 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -1,8 +1,6 @@ name: CodeQL on: - push: - branches: [main] pull_request: branches: [main] schedule: diff --git a/.github/workflows/security.yml b/.github/workflows/security.yml index 223f0dc..0fbba59 100644 --- a/.github/workflows/security.yml +++ b/.github/workflows/security.yml @@ -2,8 +2,6 @@ name: Security on: pull_request: - push: - branches: [main] schedule: - cron: "17 4 * * 1" workflow_dispatch: diff --git a/.github/workflows/testpypi.yml b/.github/workflows/testpypi.yml index 988ba6c..8d8768e 100644 --- a/.github/workflows/testpypi.yml +++ b/.github/workflows/testpypi.yml @@ -42,8 +42,6 @@ jobs: echo "version=${VERSION}" >> "${GITHUB_OUTPUT}" echo "source_date_epoch=${SOURCE_DATE_EPOCH}" >> "${GITHUB_OUTPUT}" - run: uv sync --frozen - - run: ./scripts/quality.sh check - - run: uv run pip-audit --skip-editable - name: Build deterministic development artifacts env: SETUPTOOLS_SCM_PRETEND_VERSION: ${{ steps.preview.outputs.version }} diff --git a/PLANS.md b/PLANS.md index 0216171..0f84541 100644 --- a/PLANS.md +++ b/PLANS.md @@ -6,7 +6,7 @@ Use this file for active or blocked repository work. Update it before implementa ### Release 0.8.1: completion cap and safe LLM provider failures -Status: implementation complete, local exact-head validation pending; `release-required`. Target stable version: `0.8.1`. +Status: implementation complete, final exact-head validation pending; `release-required`. Target stable version: `0.8.1`. #### Outcome and scope @@ -14,6 +14,7 @@ Status: implementation complete, local exact-head validation pending; `release-r - Canonicalize provider URL, protocol, headers, auxiliary `/models` URL, and request-body controls in one provider-neutral runtime owner shared by `configure` and `preflight`. - Project private OCR retry diagnostics into a closed provider-neutral failure reason and toolkit-authored GitLab guidance. Raw provider bodies, messages, codes, URLs, models, request IDs, paths, warnings, credentials, and stderr remain private. - Keep OCR `1.9.10` as the exact qualified dependency. Do not promote unreleased upstream defaults or derive a completion cap from `/models` metadata. +- Remove repeated validation that crosses no new owner or artifact boundary: one PR job owns coverage, one owns packages, protected-main TestPyPI owns development artifact publication/readback, and stable release keeps its exact trusted-boundary gates. #### Trust and data flow @@ -30,6 +31,7 @@ Status: implementation complete, local exact-head validation pending; `release-r 3. **Canonical provider configuration.** Share one provider-neutral owner between configure and preflight; cover API roots, terminal endpoints, trailing slash, query, credentials, fragments, protocol mismatch, and explicit models URL. Before commit: focused tests, URL/header/data-flow review, `git diff --check`. 4. **Failure projection and GitLab.** Add the bounded retry-report parser, closed reason mapping, static hints, and one renderer for non-zero retry reports and existing successful-result billing/quota warnings. Cover HTTP 400/401/402/403/404/408/409/413/422/429/5xx/529, timeout, network, decode, mixed, malformed, oversized, raw-data absence, previous-review preservation, no findings/approval, and strict/advisory behavior. Before commit: focused tests, privacy/approval/rollback review, `git diff --check`. 5. **Documentation and release handoff.** Document exact defaults, mappings, conflicts, the `4096` workaround, the three distinct token ceilings, possible provider cost reservation, and the limits of `/models`; add separate feature and bug-fix Towncrier fragments. Reconcile strategy/roadmap only where the implemented outcome changes them. Before commit: documentation/version consistency, Towncrier draft, full diff review, `git diff --check`. +6. **Validation ownership deduplication.** Keep all five OS/Python full-test jobs but instrument coverage only on Ubuntu 3.14; keep packaging only in `Build artifacts`; stop generic workflow reruns on protected-main push; and keep TestPyPI development focused on its distinct versioned artifact/publication/readback boundary. Release-PR and post-merge stable gates remain unchanged. Update workflow contract tests, development/release guidance, issue #132, and a maintenance fragment. Before commit: focused workflow tests, trigger/check-name and trusted-boundary review, actionlint-equivalent YAML/static validation through existing tests, rendered Towncrier, and `git diff --check`. #### Validation and delivery @@ -39,7 +41,7 @@ Status: implementation complete, local exact-head validation pending; `release-r - Push the complete feature history to the Draft PR, wait for hosted checks, address evidence-driven failures through the same commit gate, then mark ready and merge through protected review. - Verify the deterministic TestPyPI development build, then prepare and merge the protected `Release v0.8.1` PR. Monitor stable TestPyPI/PyPI publication, tag, immutable GitHub Release, provenance, attestations, supported-Python installs, and immutable receipt; close tracked issues only after independent external reconciliation. -Resume point: run the complete local release-readiness matrix against this plan-reconciled head, then push the complete signed feature history to Draft PR #131 and wait for hosted checks. +Resume point: run one final exact-head local gate, commit and push the reviewed validation-ownership slice, update Draft PR #131 and issue #132, then require the hosted owners to pass. #### Current implementation evidence @@ -50,3 +52,4 @@ Resume point: run the complete local release-readiness matrix against this plan- - Provider failure projection now hostile-reads the bounded private result, validates retry-report v1 counters and terminal attempt facts, and emits only a closed provider-neutral reason. Non-zero classified runs use one static GitLab renderer, keep stderr/provider fields private, preserve the previous review, publish no findings, and never reach approval; legacy billing warnings use the same renderer. The focused gate passed Ruff, full package mypy, 302 tests, and 119 subtests. - Public configuration, GitLab operations, security boundaries, and the test-evidence matrix now distinguish the per-request completion cap, OCR-owned prompt/context ceiling, and aggregate review budget; document the `/models` non-claim and the safe 404/429 projection; and preserve the generic fallback boundary. Separate #130 feature and #129 bug-fix fragments enumerate added, changed, migration, privacy, deployment, and unchanged contracts. The documentation gate passed 49 tests, Ruff, `git diff --check`, and the rendered 0.8.1 Towncrier section. - Overall parser-boundary review found and closed three narrow fail-closed gaps before publication: completion-cap length is bounded before integer conversion, embedded URL whitespace is rejected before `urllib` normalization, and JSON booleans cannot satisfy retry attempt numbering. Ruff, full package mypy, 135 focused tests, 100 subtests, and `git diff --check` pass for the correction. +- Issue #132 owns the user-requested validation deduplication. The retained boundaries are five full functional matrix jobs, one PR coverage owner, PR security, one PR package owner, protected-main development artifact publication/readback, release-PR review, and the unchanged post-merge stable release pipeline. Its focused workflow contract passed 46 tests and YAML parsing for all five edited workflows; the complete handoff gate passed 1,231 tests plus 306 subtests, 86.13% combined branch coverage, all four risk floors (84/82/85/87), Ruff, mypy, Bandit, lock and OCR-manifest validation, rendered Towncrier, and `git diff --check`. Live ruleset readback confirmed the required check contexts and strict protected-head policy remain aligned with the unchanged job names. diff --git a/changelog.d/132.maintenance.md b/changelog.d/132.maintenance.md new file mode 100644 index 0000000..4d6ba0c --- /dev/null +++ b/changelog.d/132.maintenance.md @@ -0,0 +1,5 @@ +Protected validation now assigns each repeated check to one explicit owner: + +- **Changed:** all five supported OS/Python pull-request jobs still run the complete functional suite, while Ubuntu on the newest supported Python is the single owner of combined and risk-group coverage floors. +- **Removed:** generic `main`-push reruns of CI, package build, Security, and CodeQL; duplicate wheel/sdist construction inside the CI quality job; and duplicate source quality and dependency-audit runs inside the TestPyPI development workflow. +- **Unchanged:** pull-request Security, CodeQL, Dependency Review, and `Build artifacts` gates; scheduled security analysis; TestPyPI artifact publication, provenance, bounded registry readback, and clean installs; release-pull-request checks; and the complete post-merge stable-release validation. diff --git a/docs/development.md b/docs/development.md index 5ad9d36..8cd3195 100644 --- a/docs/development.md +++ b/docs/development.md @@ -13,16 +13,16 @@ uv run python -m build uv run twine check dist/* ``` -For routine agent and contributor checks, prefer `scripts/quality.sh check`. It replaces the selected mode's prior log, captures current output under ignored `.quality-logs/`, and prints only a short status; on failure it prints the last 80 lines. Individual modes are `format`, `lint`, `test`, `coverage`, `types`, and `security`. The `coverage` and `check` modes reuse that single branch-aware test run, then enforce scoped floors for result/preflight and GitLab posting transactions at 80%, plus review/context/DLP/approval and MCP/provider/policy/result contracts at 85%; a high combined result cannot hide a weak risk group. The Bandit gate scans only the supported runtime package at medium-or-higher severity and confidence; tests and synthetic fixtures are intentionally outside that bounded gate. +For routine agent and contributor checks, run focused tests for each logical change and `scripts/quality.sh check` once on the completed handoff head. It replaces the selected mode's prior log, captures current output under ignored `.quality-logs/`, and prints only a short status; on failure it prints the last 80 lines. Individual modes are `format`, `lint`, `test`, `coverage`, `types`, and `security`. The `coverage` and `check` modes reuse that single branch-aware test run, then enforce scoped floors for result/preflight and GitLab posting transactions at 80%, plus review/context/DLP/approval and MCP/provider/policy/result contracts at 85%; a high combined result cannot hide a weak risk group. Hosted pull requests still run the complete suite on all five supported OS/Python combinations, while Ubuntu with the newest supported Python is the sole coverage owner. The Bandit gate scans only the supported runtime package at medium-or-higher severity and confidence; tests and synthetic fixtures are intentionally outside that bounded gate. Runtime code must remain compatible with Python 3.12-3.14 and standard-library-only. Tests must use synthetic data; public examples must use safe placeholder hosts and credentials while describing the real operating behavior rather than labelling the feature itself as synthetic. User-visible changes require a fragment in `changelog.d/`. Repository-only qualification tools and evidence live under `scripts/` and `compatibility/`; they are excluded from both published distributions. Validate the manifest with `PYTHONPATH=src python scripts/ocr_compat.py validate`. -For artifact smoke tests, install the wheel and sdist into separate temporary virtual environments and run `ocr-ci --help`. Generic secret scanning uses Gitleaks; dependency auditing uses `pip-audit`. Install the exact Gitleaks version printed by `scripts/gitleaks.sh --version`, then run `scripts/gitleaks.sh` before pushing and `scripts/quality.sh check` for the Python quality matrix. The wrapper fails closed when the scanner version or base ref is unavailable, scans the complete first-parent feature history, and is also the single source for the hosted security job's version pin. TestPyPI and stable-release workflows do not duplicate that dedicated security job. +For package, executable-integration, or release-machinery changes, install the wheel and sdist into separate temporary virtual environments and run `ocr-ci --help`; ordinary runtime changes rely on the pull request's single `Build artifacts` owner instead of rebuilding packages inside both CI jobs. Repeat deterministic builds locally only when reproducibility or release machinery is in scope. Generic secret scanning uses Gitleaks; dependency auditing uses `pip-audit`. Install the exact Gitleaks version printed by `scripts/gitleaks.sh --version`, then run `scripts/gitleaks.sh` before pushing and `scripts/quality.sh check` for the Python quality matrix. The wrapper fails closed when the scanner version or base ref is unavailable, scans the complete first-parent feature history, and is also the single source for the hosted security job's version pin. TestPyPI and stable-release workflows do not duplicate that dedicated security job. `tests/test_installed_policy_e2e.py` builds both the direct wheel path and the sdist-to-wheel path, installs each into a clean environment, and exercises target decisions and nested guidance through the real stdio MCP. It runs with a hostile repository shadow package, restricted `PATH`, owner-only artifacts, and the installed console entry point; keep package-boundary changes inside that test rather than replacing it with editable-install mocks. -GitHub Actions storage is repository-owned infrastructure. CI restores setup-uv caches on pull requests but saves them only from `main`; CodeQL TRAP caching and the separately controlled v4 overlay-database mode are disabled, so the small repository receives a full analysis without per-run CodeQL cache writes. Workflow artifacts use a seven-day handoff window. The weekly **Actions storage maintenance** workflow grants `actions: write` only to its cleanup job and deletes all CodeQL caches, non-main or superseded setup-uv caches, superseded Gitleaks caches, artifacts older than seven days, ordinary logs older than 14 days, and release/TestPyPI logs older than 30 days. It deletes completed TestPyPI preview runs after 14 days, TestPyPI development and ordinary completed runs after 30 days, and stable Release runs after 60 days; deleting a run removes that run's metadata, logs, and check metadata, so a workflow run is never removed before its separately promised log window. Active and newer runs remain untouched. The scheduled collector reads a closed 74-day UTC window in daily shards, retaining a fail-closed ten-page limit per day instead of applying that limit to the aggregate run history. Scheduled log cleanup uses a bounded 14-day retry window so immutable run history does not get scanned and retried forever. Manual dispatch is a dry run unless `execute` is selected; the same plan is available locally with `python scripts/actions_cleanup.py`, requires `--execute` for deletion, and accepts `--include-all-old-logs` for a deliberate one-time historical log cleanup. +GitHub Actions storage is repository-owned infrastructure. Pull-request CI restores setup-uv caches but does not save branch-specific entries; protected-main publication may refresh shared dependency state. CodeQL TRAP caching and the separately controlled v4 overlay-database mode are disabled, so the small repository receives a full analysis without per-run CodeQL cache writes. Workflow artifacts use a seven-day handoff window. The weekly **Actions storage maintenance** workflow grants `actions: write` only to its cleanup job and deletes all CodeQL caches, non-main or superseded setup-uv caches, superseded Gitleaks caches, artifacts older than seven days, ordinary logs older than 14 days, and release/TestPyPI logs older than 30 days. It deletes completed TestPyPI preview runs after 14 days, TestPyPI development and ordinary completed runs after 30 days, and stable Release runs after 60 days; deleting a run removes that run's metadata, logs, and check metadata, so a workflow run is never removed before its separately promised log window. Active and newer runs remain untouched. The scheduled collector reads a closed 74-day UTC window in daily shards, retaining a fail-closed ten-page limit per day instead of applying that limit to the aggregate run history. Scheduled log cleanup uses a bounded 14-day retry window so immutable run history does not get scanned and retried forever. Manual dispatch is a dry run unless `execute` is selected; the same plan is available locally with `python scripts/actions_cleanup.py`, requires `--execute` for deletion, and accepts `--include-all-old-logs` for a deliberate one-time historical log cleanup. ## Planning and documentation lifecycle diff --git a/docs/release.md b/docs/release.md index 8b8dd29..50ab536 100644 --- a/docs/release.md +++ b/docs/release.md @@ -46,7 +46,7 @@ and exact resume action. ## Development builds -Every non-release push to protected `main` runs the **TestPyPI development build** workflow. The immutable workflow run number produces `.devN` (for example `0.3.0.devN` after the 0.2.0 release); rerunning the same run reuses the version and succeeds only when the already-published filenames and SHA-256 values match the reviewed artifacts. The workflow uses TestPyPI Trusted Publishing, publishes attestations, verifies the exact `testpypi.yml` PEP 740 publisher and subjects plus bounded HTTPS downloads, and smoke-installs the exact wheel and sdist locally with `--no-deps`. +Every non-release push to protected `main` runs the **TestPyPI development build** workflow. The immutable workflow run number produces `.devN` (for example `0.3.0.devN` after the 0.2.0 release); rerunning the same run reuses the version and succeeds only when the already-published filenames and SHA-256 values match the reviewed artifacts. Source tests, coverage, security, CodeQL, dependency review, and the package smoke gate already bind the protected pull-request tree, so this main-push workflow does not repeat them. It owns the new boundary instead: TestPyPI Trusted Publishing, attestations, exact `testpypi.yml` PEP 740 publisher and subject verification, bounded HTTPS readback, and smoke-installation of the exact development wheel and sdist with `--no-deps`. Development builds never create tags or GitHub Releases and never publish to production PyPI. TestPyPI is public disclosure, so only reviewed pull requests may reach `main`. diff --git a/tests/test_operations_docs.py b/tests/test_operations_docs.py index 5bad27e..04d439f 100644 --- a/tests/test_operations_docs.py +++ b/tests/test_operations_docs.py @@ -370,6 +370,27 @@ def test_security_workflow_has_a_bounded_bandit_job() -> None: assert "# nosec B108" in security +def test_protected_pr_and_scheduled_checks_do_not_repeat_on_main_push() -> None: + """Assign source validation to the reviewed tree and publication to main.""" + + workflows = PROJECT_ROOT / ".github" / "workflows" + for name in ("ci.yml", "build.yml", "security.yml", "codeql.yml"): + workflow = (workflows / name).read_text(encoding="utf-8") + assert " push:" not in workflow + assert "pull_request:" in workflow + + security = (workflows / "security.yml").read_text(encoding="utf-8") + codeql = (workflows / "codeql.yml").read_text(encoding="utf-8") + testpypi = (workflows / "testpypi.yml").read_text(encoding="utf-8") + release = (workflows / "release.yml").read_text(encoding="utf-8") + + assert "schedule:" in security + assert "schedule:" in codeql + assert " push:\n branches: [main]" in testpypi + assert "./scripts/quality.sh check" in release + assert "uv run pip-audit --skip-editable" in release + + def test_threat_model_covers_remote_finding_image_boundary() -> None: security = (PROJECT_ROOT / "docs" / "security.md").read_text(encoding="utf-8") policy = (PROJECT_ROOT / "SECURITY.md").read_text(encoding="utf-8") @@ -445,8 +466,7 @@ def test_actions_storage_maintenance_bounds_completed_run_metadata() -> None: assert "actions: write" in workflow assert "scripts/actions_cleanup.py" in workflow assert "--execute" in workflow - assert "save-cache:" in ci - assert "refs/heads/main" in ci + assert "save-cache: false" in ci assert "trap-caching: false" in codeql assert "CODEQL_OVERLAY_DATABASE_MODE: none" in codeql assert "separately controlled v4 overlay-database mode are disabled" in development diff --git a/tests/test_quality_script.py b/tests/test_quality_script.py index 734522c..d4a944e 100644 --- a/tests/test_quality_script.py +++ b/tests/test_quality_script.py @@ -24,7 +24,7 @@ def test_quality_script_uses_an_isolated_ignored_environment() -> None: def test_quality_script_enforces_combined_and_boundary_coverage() -> None: - """Use one branch-aware test run followed by four scoped coverage reports.""" + """Use one branch-aware test run and one hosted coverage owner.""" script = SCRIPT.read_text(encoding="utf-8") workflow = CI_WORKFLOW.read_text(encoding="utf-8") @@ -41,9 +41,26 @@ def test_quality_script_enforces_combined_and_boundary_coverage() -> None: assert ( "uv run pytest --cov=ocr_toolkit --cov-report=term-missing --cov-fail-under=85" in workflow ) + assert workflow.count("coverage: true") == 1 + assert workflow.count("coverage: false") == 4 + assert "if: ${{ matrix.coverage }}" in workflow + assert "if: ${{ !matrix.coverage }}" in workflow + assert workflow.count("uv run pytest -q") == 1 for command in coverage_commands: assert command in script - assert f"uv run {command}" in workflow + assert workflow.count(f"uv run {command}") == 1 + + +def test_ci_quality_job_does_not_duplicate_the_package_gate() -> None: + """Leave distribution construction and clean-install smoke to Build artifacts.""" + + workflow = CI_WORKFLOW.read_text(encoding="utf-8") + quality_job = workflow.split(" quality:", 1)[1] + + assert "python -m build" not in quality_job + assert "twine check" not in quality_job + assert "wheel-smoke" not in quality_job + assert "sdist-smoke" not in quality_job def test_quality_script_runs_the_bounded_bandit_gate() -> None: diff --git a/tests/test_testpypi_preview.py b/tests/test_testpypi_preview.py index 9175c7f..7ae163d 100644 --- a/tests/test_testpypi_preview.py +++ b/tests/test_testpypi_preview.py @@ -166,6 +166,8 @@ def test_artifact_manifest_accepts_only_complete_trusted_release() -> None: def test_workflow_automates_one_idempotent_development_build_per_main_run() -> None: + """Keep the main-push workflow focused on its public artifact boundary.""" + workflow = WORKFLOW.read_text(encoding="utf-8") assert "workflow_dispatch:" not in workflow @@ -176,6 +178,8 @@ def test_workflow_automates_one_idempotent_development_build_per_main_run() -> N assert "overwrite: true" in workflow assert "needs.build.outputs.publish == 'true'" in workflow assert "needs.publish.result == 'skipped'" in workflow + assert "quality.sh check" not in workflow + assert "pip-audit" not in workflow def test_workflow_bounds_and_verifies_every_testpypi_download() -> None: @@ -251,16 +255,20 @@ def test_production_release_verifies_reviewed_registry_artifacts() -> None: ) assert "pip install --no-deps --index-url" not in verifier assert '"open-code-review-toolkit==${VERSION}"' not in workflow + assert "./scripts/quality.sh check" in workflow + assert "uv run pip-audit --skip-editable" in workflow def test_distribution_build_is_a_bounded_pull_request_gate() -> None: + """Build and smoke-test packages once on every protected pull request.""" + workflow = BUILD_WORKFLOW.read_text(encoding="utf-8") - pull_request_block = workflow.split(" pull_request:", 1)[1].split(" push:", 1)[0] + pull_request_block = workflow.split(" pull_request:", 1)[1].split("\n\npermissions:", 1)[0] assert "pull_request:" in workflow assert "branches: [main]" in workflow + assert " push:" not in workflow assert "paths:" not in pull_request_block - assert '"scripts/install_local_artifact.py"' in workflow assert "timeout-minutes: 15" in workflow assert "python -m build --no-isolation" in workflow assert workflow.count("pip install --no-deps") == 1 @@ -268,8 +276,11 @@ def test_distribution_build_is_a_bounded_pull_request_gate() -> None: def test_ci_matrix_covers_supported_python_minors_and_os_boundaries() -> None: + """Run all five functional combinations while collecting coverage once.""" + workflow = (PROJECT_ROOT / ".github" / "workflows" / "ci.yml").read_text(encoding="utf-8") + assert " push:" not in workflow assert workflow.count('python: "3.12"') == 2 assert workflow.count('python: "3.13"') == 1 assert workflow.count('python: "3.14"') == 2