#136: OpenAI grading provider (plan)

.claude/rules/llm-drafter.md

@@ -14,14 +14,32 @@ Every `# pyright: ignore[...]` and `# type: ignore[...]` comment for the Anthrop
 
 `call_llm(*, system, cached_block, dynamic_block, model, max_tokens, cache_ttl="5m", prompt_version, max_retries_*, provider="anthropic", client=None) -> LLMResult` (renamed from `call_anthropic` by #135, which dropped the old name) is the single shared seam for **both** the drafter and grader. It owns the generic machinery — retry loop + backoff (`2**attempt*_rand_uniform(0.75,1.25)`), per-class budgets, WARNING/INFO logs, the min/cap token validation, and `LLMResult` assembly — and dispatches the vendor-specific bits to a provider strategy resolved from a registry. When `client is None`, `call_llm` builds it via `strategy.make_client()` (DEC-006 — client construction lives at the seam, not the CLI).
 
-- **`LLMProvider` ABC + registry** in `signalforge.llm.providers`: `register_provider(provider)` / `provider_for(name) -> LLMProvider`; unknown name → `UnknownProviderError(LLMError)` listing available keys (CLI tier 2). A provider supplies `make_client`, `build_create_kwargs`, `build_count_tokens_kwargs`, `extract_text_blocks`, `extract_usage` (→ `UsageMetrics`), `classify_exception` (→ `ExceptionCategory`), plus capability flags `supports_prompt_caching` / `supports_token_count`. Wiring a new provider = that class + `register_provider` + a config enum value. `AnthropicProvider` (`name="anthropic"`, both flags `True`) is the only one registered in v0.x.
+- **`LLMProvider` ABC + registry** in `signalforge.llm.providers`: `register_provider(provider)` / `provider_for(name) -> LLMProvider`; unknown name → `UnknownProviderError(LLMError)` listing available keys (CLI tier 2). A provider supplies `make_client`, `build_create_kwargs`, `build_count_tokens_kwargs`, `extract_text_blocks`, `extract_usage` (→ `UsageMetrics`), `classify_exception` (→ `ExceptionCategory`), `estimate_input_tokens(model, text, *, system="", client=None) -> int` (#136 US-005; powers `--estimate`), plus capability flags `supports_prompt_caching` / `supports_token_count`. Wiring a new provider = that class + `register_provider` + a config enum value. Registered in v0.3: `AnthropicProvider` (`name="anthropic"`, both flags `True`); `OpenAIProvider` (`name="openai"`, both flags `False`, #136).
 - **Neutral value objects:** `UsageMetrics` + the `ExceptionCategory` enum (`AUTH`, `RATE_LIMIT`, `SERVER_ERROR`, `CONNECTION`, `NO_RETRY`) keep the orchestrator off vendor-shaped dicts.
 - **Capability-gated behaviour (DEC-008):** `supports_prompt_caching=False` ⇒ no `cache_control` marker, no `extended-cache-ttl` beta header, 0 cache tokens, no dual-zero anomaly WARNING. `supports_token_count=False` ⇒ skip the pre-send count gate (no pre-send `LLMCacheTooLargeError`). Anthropic sets both `True`, so its emitted bytes/control flow are unchanged — the byte-identity gate (fixtures + prompt-cache snapshot + drift detectors) is the regression guard.
 - **`provider` config field (DEC-007):** `DraftConfig.provider` (`llm:` block) and `GradeConfig.provider` (`grade:` block), both registry-validated `str` defaulting to `"anthropic"` — **deliberately NOT a `Literal`** (a registry is a plugin point that grows; #136/#137 register a provider instead of editing a Literal in two configs). The validator raises `UnknownProviderError` (an `LLMError`, so Pydantic v2 does NOT wrap it into `ValidationError` — it propagates raw with the available-keys remediation).
 
 **Gate the cache marker on BOTH capability flags, not just `supports_prompt_caching` (#135 QG lesson).** `call_llm` sets `cache_marker_active = supports_prompt_caching AND supports_token_count`. The pre-send count gate is what enforces the sub-minimum drop + the 8000-token oversize cap; attaching a `cache_control` marker without that gate having run would send an *unvalidated* marker (a sub-minimum block silently no-ops the marker — paying the input premium with no discount; an oversize block bypasses `LLMCacheTooLargeError`). Anthropic is `True/True` so the default path is unaffected, but a future provider that caches yet has no token-count API (`True/False`) must degrade to no-caching rather than send an unguarded marker. A new provider's capability flags are load-bearing — set them honestly, and don't assume "supports caching" alone is sufficient to attach a marker.
 
-When a new vendor lands (#136 OpenAI / #137 Gemini), add a `_<vendor>_client.py` shim + a `LLMProvider` subclass + `register_provider`; don't pool SDK ignores into a generic util module, and don't reach into `call_llm` — extend via the strategy.
+When a new vendor lands (#137 Gemini next), add a `_<vendor>_client.py` shim + a `LLMProvider` subclass + `register_provider`; don't pool SDK ignores into a generic util module, and don't reach into `call_llm` — extend via the strategy.
+
+### OpenAI provider shape (#136 — the second concrete provider, the no-cache precedent)
+
+`OpenAIProvider` ships under `provider="openai"` for both stages; the shim at `src/signalforge/llm/_openai_client.py` confines every `# pyright: ignore` / `# type: ignore` for the `openai` SDK (DEC-012 of #5 generalised). Three load-bearing patterns established by #136 that the next no-cache vendor should mirror:
+
+1. **`.messages.create` façade adapter.** The orchestrator hard-calls `llm_client.messages.create(**kwargs)`, but OpenAI's SDK exposes `client.chat.completions.create(...)`. `_OpenAIClientAdapter.messages` is a `SimpleNamespace` instance whose `.create` callable delegates to `chat.completions.create(**kwargs)` and whose `.count_tokens` raises `NotImplementedError` defensively (orchestrator never calls it when `supports_token_count=False`, but the protocol surface is uniform). Any vendor whose SDK uses a different call shape gets the same shim adaptation rather than a special-case branch in `call_llm`.
+2. **`response_format={"type":"json_object"}` belt-and-braces with the tolerant JSON parser (DEC-006 of #136).** `OpenAIProvider.build_create_kwargs` attaches the JSON-mode flag at every call. The grade + draft system prompts already contain "json" (case-insensitive), satisfying OpenAI's prompt-requirement check. The tolerant `extract_json_payload` parser (issue #144) remains the fallback if a future model strips the flag; server-side enforcement eliminates the prose-preamble drift class entirely for the providers that support it. **A future vendor with an equivalent JSON-mode (e.g. Gemini's `response_mime_type="application/json"`) MUST set it for the same reason — don't lean on the parser alone when the API exposes a server-side gate.**
+3. **`tiktoken` model-id fallback for `--estimate` (DEC-012 of #136).** `_count_openai_tokens(model, text)` calls `tiktoken.encoding_for_model(model)` inside a try/except, falling back to `tiktoken.get_encoding("cl100k_base")` on `KeyError` for unknown ids (newer model SKUs released after the installed `tiktoken` ship out of the registry). **Don't raise on unknown id** — `--estimate` is a calibration signal, not a billing guarantee (mirrors the `EXPLAIN`-based planner-estimate caveat in `warehouse-adapters.md`).
+
+### `estimate_input_tokens(*, system=...)` — separate the system envelope to preserve real-API byte-identity (#136 US-005 / DEC-013, refined by US-008 QG)
+
+The `--estimate` path generalised in US-005 via `LLMProvider.estimate_input_tokens(model, text, *, system="", client=None)`. The `system` parameter is keyword-only with an empty default, but **threading it as its own kwarg is load-bearing for Anthropic byte-identity:**
+
+- **AnthropicProvider** passes `system=system` to `messages.count_tokens(...)` when non-empty. The pre-refactor inline call did the same — and Anthropic's server-side tokenizer applies its own system-envelope tokens to that block. Dropping the kwarg (concatenating system into the user-content text) silently under-reports real-API counts by the system-envelope size, while a fake-driven byte-identity snapshot still passes because the fake returns canned `input_tokens` regardless of kwargs. **Lesson — fake-driven byte-identity tests pin rendered-output identity, NOT real-API call shape.** If a refactor changes the SDK call shape, the snapshot won't catch it; a real-API `@pytest.mark.anthropic` test against `--estimate` (or an explicit kwargs-shape assertion on the fake) is the only way. This is the same "snapshot/sqlglot tier vs live tier" gap documented in `warehouse-adapters.md` for the Snowflake compiler.
+- **OpenAIProvider** concatenates `system + text` before `tiktoken` — there's no system-envelope distinction at the BPE level, so every token contributes to the same total. Matches what OpenAI's chat-completion endpoint bills.
+- **Grade-side estimate (`_count_grade_criterion_tokens`) deliberately double-counts the rubric.** The pre-refactor shape passed `system=system_and_rubric` AND embedded `system_and_rubric` inside the cached user-content block, so the rubric was counted twice. This was already the behaviour; preserving byte-identity required reproducing it. **Don't "fix" this double-count in a future tidy-pass — it's the pre-refactor shape, and a quiet repair would silently shift estimate figures by ~the rubric size.** A real future cleanup would need a tied snapshot regeneration + operator-visible "estimate calibration changed" CHANGELOG entry.
+
+A new provider added under #137+ MUST implement `estimate_input_tokens` (it's `@abstractmethod`; missing impls fail at instantiation time). The implementation can ignore `system` (FakeNoCacheProvider stub concatenates for word-count; `_DummyProvider` returns `0`); but if the vendor's API distinguishes system from user tokens (like Anthropic), thread it separately or the same drift surfaces.
 
 ## Module-level `_sleep` / `_rand_uniform` aliases (DEC-004)
 
@@ -99,12 +117,15 @@ Same rule as the other layers (`safety-layer.md` DEC-022 / `prune-engine.md` DEC
 
 ## AST audit-completeness scans (DEC-013)
 
-`tests/test_audit_completeness.py` runs four AST scans:
+`tests/test_audit_completeness.py` runs five AST scans relevant to the LLM seam:
 
 - `LLMRequest` constructed only in `signalforge.safety.request` (existing from #4).
 - `AuditEvent` constructed only in `signalforge.safety.request`.
 - `anthropic.Anthropic(...)` constructed only in `signalforge.llm._anthropic_client` (DEC-012 — the SDK seam; module renamed from `_client` by #135).
 - `LLMResponseEvent` constructed only in `signalforge.draft.audit` — every event flows through `_build_response_event`.
+- **`openai.OpenAI(...)` constructed only in `signalforge.llm._openai_client`** (the 9th project AST scan, #136 US-001/DEC-010). Mirrors Scan 3 shape exactly — reuses `_AttributeCallFinder`, catches all three bypass patterns (bare via `from openai import OpenAI`, import-alias via `from openai import OpenAI as O`, module-attribute via `import openai; openai.OpenAI(...)`).
+
+When a new vendor lands (#137 Gemini bumps the tally to 10 for `genai.Client(...)`), add a **new** scan rather than extending an existing one — Scan 3 is Anthropic-specific (it hunts `anthropic.Anthropic`), so the OpenAI / Gemini scans are siblings, not extensions. The companion per-vendor `tests/llm/test_<vendor>_client_confinement.py` mirrors the Snowflake-shaped line-based scan for `# type: ignore` / `# pyright: ignore` confinement.
 
 If a new module legitimately needs to construct one of these gated names, update the scan's exclusion list AND document the audit-write seam. Don't suppress the test.
 
@@ -114,4 +135,4 @@ The drafter's config block is `{ llm: { provider, model, cheap_model, max_output
 
 ## Reference
 
-`plans/super/5-llm-draft-pipeline.md` — DEC-001 … DEC-027. `plans/super/135-provider-neutral-llm-seam.md` — DEC-001 … DEC-012 (the provider-neutral seam: `call_llm`, `LLMProvider` ABC + registry, capability flags, `provider` config field). `src/signalforge/llm/` (incl. `providers.py` + `_anthropic_client.py`), `src/signalforge/draft/` — current implementation. `tests/llm/_fake.py::FakeAnthropicClient` — `expect_*` API; `tests/llm/_fake_provider.py::FakeNoCacheProvider` + `tests/grade/test_provider_neutrality.py` — the no-cache provider-neutrality proof. `docs/draft-ops.md` — operational reference. `tests/fixtures/draft/llm_response_*.json` — fixture set exercising happy + each error path.
+`plans/super/5-llm-draft-pipeline.md` — DEC-001 … DEC-027. `plans/super/135-provider-neutral-llm-seam.md` — DEC-001 … DEC-012 (the provider-neutral seam: `call_llm`, `LLMProvider` ABC + registry, capability flags, `provider` config field). `plans/super/136-openai-grading-provider.md` — DEC-001 … DEC-014 (OpenAI as the second concrete provider: shim + `OpenAIProvider` + four pricing SKUs + `--estimate` strategy refactor + JSON-mode enforcement; the no-cache real-vendor precedent for #137 Gemini). `src/signalforge/llm/` (incl. `providers.py` + `_anthropic_client.py` + `_openai_client.py`), `src/signalforge/draft/` — current implementation. `tests/llm/_fake.py::FakeAnthropicClient` + `tests/llm/_fake_openai.py::FakeOpenAIClient` — `expect_*` API; `tests/llm/_fake_provider.py::FakeNoCacheProvider` + `tests/grade/test_provider_neutrality.py` + `tests/grade/test_provider_neutrality_openai.py` — the no-cache provider-neutrality proofs (synthetic + real OpenAI). `docs/draft-ops.md` / `docs/grade-ops.md` / `docs/cost-estimate-ops.md` — operational references. `tests/fixtures/draft/llm_response_*.json` / `tests/fixtures/estimate/anthropic_byte_identity_golden.txt` — fixture sets exercising happy + each error path + the DEC-013 Anthropic byte-identity floor.

CHANGELOG.md

@@ -4,7 +4,14 @@ All notable changes to SignalForge are documented here. The format is loosely ba
 
 ## [Unreleased]
 
-_Nothing yet — entries land here on `dev` and get promoted to a dated section at release time._
+### Added
+
+- **OpenAI as a grading + drafting provider (#136).** Set `grade.provider: openai` or `llm.provider: openai` in `signalforge.yml`; requires the `[openai]` install extra and `OPENAI_API_KEY`. Ships four pricing SKUs (`gpt-4o`, `gpt-4o-mini`, `gpt-4.1`, `gpt-4-turbo`); `--estimate` works via tiktoken (no extra API round-trip). Server-side JSON enforcement via `response_format={"type": "json_object"}`. v0.3 ships without prompt caching (no Anthropic-style cache discount); follow-up to evaluate OpenAI prompt caching.
+
+### Fixed
+
+- **`--estimate` grader-side token counts no longer double-count the rubric (#136 US-008 QG).** The pre-US-005 inline Anthropic call passed the rubric in BOTH the `system=` kwarg AND embedded in the cached user-content block, counting it twice per criterion. The first QG fix preserved that for Anthropic byte-identity, which then triple-counted the rubric for OpenAI (system→`system + text` concat → rubric prefix in text). Corrected to match the runtime grader call: rubric in `system=` once, artifact envelope in user content. Real-API `--estimate` figures for the grade-side shift down by ~one rubric per criterion (was: bug → over-report; now: matches what gets billed). Fake-driven byte-identity golden unchanged (canned token counts are call-shape-agnostic).
+- **`estimate(...)` engine parameter renamed `anthropic_client` → `client` (#136 US-008 QG).** Post-US-005 the slot was already typed `object | None` and forwarded verbatim to whichever provider strategy is active; the old name implied Anthropic-only and would mislead a future #137 Gemini wiring. CLI in `generate.py` already passed `None` for non-Anthropic providers; the rename surfaces that without behaviour change.
 
 ## [0.3.0] — 2026-05-27
 

CONTRIBUTING.md

@@ -34,10 +34,11 @@ uv run ruff check . && uv run ruff format --check . && uv run pyright && uv run
 ## Pre-release coverage audit
 
 The default `pytest` run — and therefore the coverage badge — measures only the
-default marker set. Tests gated behind `bigquery`, `anthropic`, `cli_subprocess`,
-`e2e`, and `wheel_smoke` are filtered out by `addopts` (see
-`.claude/rules/testing-signal.md` § "Known gap: excluded markers"), so the
-real-network and packaging paths are not instrumented in the badge number.
+default marker set. Tests gated behind `bigquery`, `anthropic`, `openai`,
+`cli_subprocess`, `e2e`, `snowflake`, and `wheel_smoke` are filtered out by
+`addopts` (see `.claude/rules/testing-signal.md` § "Known gap: excluded
+markers"), so the real-network and packaging paths are not instrumented in the
+badge number.
 
 Run this audit against the **matrix ceiling** (currently Python 3.13 — the
 highest version CI exercises) by prefixing `uv run --python 3.13`, so the
@@ -56,9 +57,11 @@ uv run pytest
 #    --cov-append combines with run 1 so the term report shows the COMBINED total.
 #    --cov-fail-under=0 overrides the 80% gate inherited from addopts — gated
 #    markers alone never clear it, and this is a measurement, not a gate.
-#    (bigquery/anthropic/e2e need creds; cli_subprocess/wheel_smoke do not.)
-SF_RUN_BQ=1 ANTHROPIC_API_KEY=sk-... GOOGLE_CLOUD_PROJECT=<billing-project> \
-  uv run pytest -m 'bigquery or anthropic or e2e or cli_subprocess or wheel_smoke' \
+#    (bigquery/anthropic/openai/snowflake/e2e need creds; cli_subprocess/wheel_smoke do not.)
+SF_RUN_BQ=1 ANTHROPIC_API_KEY=sk-... OPENAI_API_KEY=sk-... \
+  SF_RUN_OPENAI=1 SF_RUN_SNOWFLAKE=1 GOOGLE_CLOUD_PROJECT=<billing-project> \
+  SNOWFLAKE_ACCOUNT=... SNOWFLAKE_USER=... SNOWFLAKE_PASSWORD=... SNOWFLAKE_WAREHOUSE=... \
+  uv run pytest -m 'bigquery or anthropic or openai or snowflake or e2e or cli_subprocess or wheel_smoke' \
   --cov=signalforge --cov-append --cov-fail-under=0 --cov-report=term
 ```
 
@@ -129,3 +132,34 @@ SF_RUN_BQ)`.
 The tests query `bigquery-public-data.samples.shakespeare` (164K rows,
 free under the 1 TB/month BigQuery tier). They are maintainer-only for
 v0.1; no CI job runs them.
+
+## OpenAI live-API smoke tests
+
+Three tests gated by `@pytest.mark.openai` exercise the OpenAI provider
+end-to-end (issue #136):
+
+- `tests/grade/test_smoke_real_api_openai.py` — `grade_artifacts` against
+  a tiny in-test fixture, single criterion.
+- `tests/draft/test_smoke_real_api_openai.py` — `draft_schema` against a
+  small in-test manifest; honours DEC-005's "scope both stages" commitment.
+- `tests/cli/test_e2e_estimate_openai.py` — `signalforge generate
+  --estimate` with `llm.provider: openai` + `grade.provider: openai`.
+
+All three are skipped by default (filtered out by `addopts = -m 'not
+openai'`) and additionally self-skip via a runtime gate if either env var
+is missing — the belt-and-suspenders pattern from
+`.claude/rules/testing-signal.md` § "End-to-end gated tests".
+
+Run with credentials:
+
+```bash
+SF_RUN_OPENAI=1 OPENAI_API_KEY=sk-... uv run pytest -m openai --no-cov
+```
+
+The `--estimate` test additionally honours `GOOGLE_CLOUD_PROJECT` when
+present (lets the warehouse-bytes leg compute instead of degrading to
+`<unavailable: ...>`); absent, the warehouse half degrades cleanly per
+DEC-005 of #36 and the test still passes. They are maintainer-only; no
+CI job runs them. Each run hits the real OpenAI API and incurs a small
+cost (the grade smoke is 5 `gpt-4o` calls at ~$0.005 each; the draft
+smoke is 1 call; `--estimate` is local tiktoken only, no API call).

README.md

@@ -93,7 +93,7 @@ without adding it to a project environment.
 **Working from a clone (contributing)?** Install the dev toolchain with
 `uv sync --dev` — see [CONTRIBUTING.md](CONTRIBUTING.md) for the full workflow.
 
-### 2. Authenticate to BigQuery and Anthropic
+### 2. Authenticate to BigQuery and your LLM provider
 
 ```bash
 gcloud auth application-default login
@@ -104,6 +104,14 @@ export ANTHROPIC_API_KEY=sk-ant-...
 Use a fresh shell session (or `unset ANTHROPIC_API_KEY` after the
 run) so the key doesn't persist in your bash history.
 
+Anthropic is the default LLM provider. OpenAI is also supported —
+`pip install signalforge-dbt[openai]`, set `OPENAI_API_KEY`, and
+switch via `llm.provider: openai` / `grade.provider: openai` in
+`signalforge.yml`. See
+[docs/draft-ops.md § OpenAI provider](docs/draft-ops.md#openai-provider)
+and [docs/grade-ops.md § OpenAI provider](docs/grade-ops.md#openai-provider)
+for the per-stage config + cost notes.
+
 ### 3. Minimum `signalforge.yml`
 
 The fixture ships a working config; a minimum that exercises the full