From ec58d92664fcd1a963f7731b152172ae806978e8 Mon Sep 17 00:00:00 2001 From: Michael Lieberman Date: Tue, 11 Aug 2026 16:18:39 -0400 Subject: [PATCH 1/4] test(parity): add OpenAI Tier 2 backend + SkillInvocationBackend Protocol (Fixes #368) Extends feature 028's parity test suite with a second Tier 2 provider adapter. Introduces a shared `SkillInvocationBackend` Protocol so future adapters (Gemini, xAI, self-hosted) slot in without touching the shared runner, differ, parser, or artifact writer. Two-Environment governance: `parity-tier2` (Claude, feature 028) and `parity-tier2-openai` (OpenAI, this feature) each hold their own reviewer list and secret. `OPENAI_API_KEY` never appears in any workflow other than `parity-tier2-openai.yml`; `ANTHROPIC_API_KEY` never appears in the OpenAI workflow. Enforced by a workflow-config test that iterates `.github/workflows/*.yml` and asserts the exclusivity property. Protocol seam: - `tests/darnit/parity/tier2/backends/base.py` -- @runtime_checkable Protocol with `name`, async `invoke(fixture_dir, model, max_turns)`, classmethod `check_env()`. `SkillInvocationResult` frozen dataclass gains a `turn_cap_exhausted: bool` field (default False). - `tests/darnit/parity/tier2/backends/claude_agent_sdk.py` -- feature 028's `invoke_skill` refactored into a class satisfying the Protocol. Body unchanged. - `tests/darnit/parity/tier2/claude_agent_sdk_client.py` -- backwards- compat shim; feature 028's existing tests continue to import from this path without change. - `tests/darnit/parity/tier2/backends/openai_backend.py` -- new OpenAI Chat Completions API backend. Hand-rolled tool-call loop, stateless per invocation, `temperature=0.0` for reproducibility. Registers the `audit_openssf_baseline` MCP tool as an OpenAI function-callable tool; `_dispatch_tool_call` FORCES `local_path=str(fixture_dir)` so a rogue model cannot make the tool wander outside the fixture (contract B-17, verified by adversarial test). - `tests/darnit/parity/tier2/backends/noop.py` -- test-only NoopBackend used by conformance and extensibility tests. Not registered in BACKEND_REGISTRY by default; tests inject it via `run.main(backends={"noop": NoopBackend})`. - `tests/darnit/parity/tier2/backends/__init__.py` -- BACKEND_REGISTRY dict is the single source of truth for `--backend ` lookup. Runner extensions: - `--backend ` CLI flag, default `claude_agent_sdk` (preserves feature 028 behavior). - `--model ` and `--max-turns ` flags (each backend's workflow YAML supplies its provider-appropriate pinned defaults). - New outcome `turn_cap_exhausted` with exit code 5. Diagnostically separate from `unparseable` (exit 2) and `per_control_disagree` (exit 1). Maps to a different fix (adjust prompt / raise cap, not adjust parser). - `main(argv, backends=)` accepts an optional backends dict override for test-side injection (SC-007 -- proves the Protocol seam works without touching shared modules). - Preflight audit log includes backend + model alongside actor + SHA. - Artifact filename convention: `openai_final_message.md` for the OpenAI backend; feature 028's `skill_final_message.md` preserved for Claude so downstream analysis scripts don't churn. OpenAI workflow (`.github/workflows/parity-tier2-openai.yml`): - Manual dispatch only. - `environment: parity-tier2-openai` -- REVIEWER APPROVAL REQUIRED before OPENAI_API_KEY is exposed (configure in GitHub UI; NOT in this YAML). - `permissions: contents: read` only. - Pinned model default `gpt-4o-2024-08-06` -- version-suffixed. A moving alias like `gpt-4o` would fail the SC-010 workflow-config test that regexes the default for a versioned pattern. Bumping the default requires a PR editing the YAML -- reviewable, correlatable with any subsequent test-result changes. - Artifact upload with `if: always()`. Zero product package changes. Feature 028's `test_no_product_changes.py` guard continues to cover `packages/*/src/`. Added `openai>=1.50` to the workspace-level dev group ONLY; `packages/darnit/pyproject.toml` and `packages/darnit-baseline/pyproject.toml` untouched. Tests: 32 new (protocol conformance, extensibility, OpenAI adversarial including turn-cap-exhausted + local_path guard + shared-parser compatibility, workflow config for both YAMLs, shim exports). Full workspace sweep: 2621 passed, 15 skipped, 0 failures. Closes #368. Sibling to #367 (feature 027) and #370 (feature 028); stacks on #370. --- .github/workflows/parity-tier2-openai.yml | 93 +++++ .specify/feature.json | 2 +- CLAUDE.md | 3 +- pyproject.toml | 5 + .../checklists/requirements.md | 75 ++++ .../contracts/openai-workflow.md | 66 ++++ .../skill-invocation-backend-protocol.md | 83 ++++ specs/029-openai-parity-adapter/data-model.md | 310 +++++++++++++++ specs/029-openai-parity-adapter/plan.md | 117 ++++++ specs/029-openai-parity-adapter/quickstart.md | 170 ++++++++ specs/029-openai-parity-adapter/research.md | 229 +++++++++++ specs/029-openai-parity-adapter/spec.md | 139 +++++++ specs/029-openai-parity-adapter/tasks.md | 364 ++++++++++++++++++ tests/darnit/parity/tier2/artifact_writer.py | 11 +- .../darnit/parity/tier2/backends/__init__.py | 35 ++ tests/darnit/parity/tier2/backends/base.py | 76 ++++ .../parity/tier2/backends/claude_agent_sdk.py | 134 +++++++ tests/darnit/parity/tier2/backends/noop.py | 42 ++ .../parity/tier2/backends/openai_backend.py | 223 +++++++++++ .../parity/tier2/claude_agent_sdk_client.py | 140 ++----- tests/darnit/parity/tier2/run.py | 170 ++++++-- .../tier2/test_backend_extensibility.py | 138 +++++++ .../test_backend_protocol_conformance.py | 48 +++ .../tier2/test_openai_backend_adversarial.py | 236 ++++++++++++ .../darnit/parity/tier2/test_shim_exports.py | 48 +++ .../parity/tier2/test_workflow_config.py | 107 +++++ uv.lock | 33 ++ 27 files changed, 2947 insertions(+), 150 deletions(-) create mode 100644 .github/workflows/parity-tier2-openai.yml create mode 100644 specs/029-openai-parity-adapter/checklists/requirements.md create mode 100644 specs/029-openai-parity-adapter/contracts/openai-workflow.md create mode 100644 specs/029-openai-parity-adapter/contracts/skill-invocation-backend-protocol.md create mode 100644 specs/029-openai-parity-adapter/data-model.md create mode 100644 specs/029-openai-parity-adapter/plan.md create mode 100644 specs/029-openai-parity-adapter/quickstart.md create mode 100644 specs/029-openai-parity-adapter/research.md create mode 100644 specs/029-openai-parity-adapter/spec.md create mode 100644 specs/029-openai-parity-adapter/tasks.md create mode 100644 tests/darnit/parity/tier2/backends/__init__.py create mode 100644 tests/darnit/parity/tier2/backends/base.py create mode 100644 tests/darnit/parity/tier2/backends/claude_agent_sdk.py create mode 100644 tests/darnit/parity/tier2/backends/noop.py create mode 100644 tests/darnit/parity/tier2/backends/openai_backend.py create mode 100644 tests/darnit/parity/tier2/test_backend_extensibility.py create mode 100644 tests/darnit/parity/tier2/test_backend_protocol_conformance.py create mode 100644 tests/darnit/parity/tier2/test_openai_backend_adversarial.py create mode 100644 tests/darnit/parity/tier2/test_shim_exports.py diff --git a/.github/workflows/parity-tier2-openai.yml b/.github/workflows/parity-tier2-openai.yml new file mode 100644 index 0000000..aeb79a3 --- /dev/null +++ b/.github/workflows/parity-tier2-openai.yml @@ -0,0 +1,93 @@ +# Feature 029 Tier 2 (OpenAI): coding-agent skill vs raw MCP tool output +# parity for OpenAI-based invocations. +# +# Manual-dispatch only. Environment-gated so an authorized reviewer must +# approve each run BEFORE OPENAI_API_KEY is exposed. See contract at +# specs/029-openai-parity-adapter/contracts/openai-workflow.md +# and governance rationale at spec.md FR-005 / FR-007 / FR-007a-equivalent. +# +# Key properties (see contract OW-1..OW-16): +# - OW-1: workflow_dispatch is the ONLY trigger. No schedule; no push. +# - OW-2: two inputs -- fixture_glob AND model (pinned version-suffixed default). +# - OW-3: model default MUST be a version-suffixed string (SC-010). +# - OW-4: environment `parity-tier2-openai` (distinct from Claude's env). +# - OW-6: permissions: contents: read only. +# - OW-7/OW-8/OW-9: OPENAI_API_KEY only in this workflow; the +# Anthropic-scoped key MUST NOT appear here. +# - OW-10: preflight actor+SHA+model logged BEFORE the SDK step. +# - OW-13: no api_key workflow input (governance regression guard). +# - OW-14: artifact upload runs on any exit code (`if: always()`). + +name: Parity Tier 2 (OpenAI) + +on: + workflow_dispatch: + inputs: + fixture_glob: + description: "Glob to filter which fixtures are run (default: all)" + default: "*" + required: false + model: + description: "Pinned OpenAI model (versioned suffix required; e.g. gpt-4o-2024-08-06)" + default: "gpt-4o-2024-08-06" + required: false + +permissions: + contents: read + +jobs: + tier2: + runs-on: ubuntu-latest + environment: parity-tier2-openai # OW-4: gated Environment with required reviewers + permissions: + contents: read # OW-6: no write scope + + steps: + - name: Checkout + uses: actions/checkout@v4 + + - name: Set up Python + uses: actions/setup-python@v5 + with: + python-version: "3.12" + + - name: Install uv + uses: astral-sh/setup-uv@v6 + + - name: Sync dev environment + run: uv sync --extra dev + + - name: Preflight audit log (OW-10) + run: | + { + echo "## Tier 2 (OpenAI) Preflight" + echo "" + echo "- actor: ${{ github.actor }}" + echo "- sha: ${{ github.sha }}" + echo "- fixture_glob: ${{ inputs.fixture_glob }}" + echo "- model: ${{ inputs.model }}" + echo "- timestamp: $(date -u +%Y-%m-%dT%H:%M:%SZ)" + } >> "$GITHUB_STEP_SUMMARY" + + - name: Run Tier 2 parity check (OpenAI) + env: + # OW-7: OPENAI_API_KEY appears in this workflow only. + # OW-9: this workflow deliberately does NOT expose any other + # provider's key -- the Claude/Anthropic-scoped key is only + # reachable from the sibling workflow (parity-tier2.yml). + OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} + run: | + # Run as a module (`python -m`) so the `tests.` package imports + # inside run.py resolve. + uv run python -m tests.darnit.parity.tier2.run \ + --backend openai \ + --model "${{ inputs.model }}" \ + --fixture-glob "${{ inputs.fixture_glob }}" + + - name: Upload parity artifacts (OW-14) + if: always() + uses: actions/upload-artifact@v4 + with: + name: parity-artifacts-openai + path: parity-artifacts/ + retention-days: 30 diff --git a/.specify/feature.json b/.specify/feature.json index eb972db..bcf0ef9 100644 --- a/.specify/feature.json +++ b/.specify/feature.json @@ -1 +1 @@ -{"feature_directory": "specs/028-audit-parity-tests"} +{"feature_directory": "specs/029-openai-parity-adapter"} diff --git a/CLAUDE.md b/CLAUDE.md index 7137952..0f628d4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -369,6 +369,7 @@ else: - Filesystem only. Composition is resolved in-memory at framework-config load time; no new persistent state. (013-plugin-composition) ## Recent Changes +- 029-openai-parity-adapter: adds OpenAI as a second Tier 2 backend to feature 028's parity test suite. Introduces `SkillInvocationBackend` Protocol in `tests/darnit/parity/tier2/backends/base.py` (test-only seam; `@runtime_checkable`); refactors feature 028's `claude_agent_sdk_client.py` into `backends/claude_agent_sdk.py` (backwards-compat shim preserves old import path); adds `OpenAIBackend` using Chat Completions API with `tools=[...]` function-calling, `temperature=0.0`, and pinned version-suffixed model default (`gpt-4o-2024-08-06`). Runner gains `--backend`, `--model`, `--max-turns` flags; new outcome `turn_cap_exhausted` (exit code 5) distinguishes runaway tool-loops from unparseable output. Separate `parity-tier2-openai.yml` workflow with `environment: parity-tier2-openai` (its own reviewer list + `OPENAI_API_KEY` at Environment scope, no repo-level exposure); mechanically enforced by workflow-config test. Zero product-package changes. Closes #368. - 028-audit-parity-tests: two-tier parity test suite verifying the darnit audit's per-control output is consistent across consumers. Tier 1 (`tests/darnit/parity/tier1/`) runs on every PR: parametrized-per-fixture pytest that invokes both the direct `audit_openssf_baseline` MCP tool AND `HarnessRun` (with `MockLLMStep`) in-process, then diffs per-control status. Sole allowed drift is PENDING_LLM (MCP) -> non-PENDING_LLM (harness). Tier 2 (`tests/darnit/parity/tier2/`) is manual-dispatch only via `.github/workflows/parity-tier2.yml` -- Environment-gated with required reviewers, no repo-level `ANTHROPIC_API_KEY` exposure. Uses `claude-agent-sdk` (test-only dev dep) to invoke the `/darnit-audit` skill, parses its final assistant message, diffs against the raw MCP tool JSON. Fixture corpus at `tests/darnit/parity/fixtures/`; `parity.toml` per fixture declares expected shape + `control_ids` filter. Zero product-package changes (SC-006), enforced by a git-diff-based test. Closes #366. Follow-up issues: #368 (OpenAI SDK parity), #369 (scheduled cadence + governance-appropriate key sourcing). - 027-interactive-resolvers: adds `--interactive` flag to `darnit harness` and a new `QuestionResolver` Protocol (async, `@runtime_checkable`) that sits downstream of feature 026's `AnswerSource` chain. `InteractiveTerminalResolver` reference implementation prompts on `/dev/tty` (isolated from stdout report / stderr progress streams). Third-party resolvers register via Python entry points under group `darnit.question_resolvers` (mirrors `darnit.frameworks` discovery). Every `Answer` carries `authority: "asserted"` enforced at the model layer via `Literal["asserted"]` with a fixed default. Per-question `resolution_trail` in the report captures which resolvers were offered a question and how each responded (`answered`/`skipped`/`errored`). Fail-fast (<2s) when stdin is not a TTY OR /dev/tty is not openable under `--interactive`. Feature 026's "no re-audit after collect" MVP policy preserved. - 026-darnit-harness: adds `darnit harness` subcommand -- end-to-end audit driver with in-band LLM dispatch (fleet-operator + CI-integrated persona). Consumes `ANTHROPIC_API_KEY` from env; dispatches PENDING_LLM results via `PydanticAILLMStep`. Non-interactive by default; batch answers via pluggable `AnswerSource` Protocol with auto-discovery of `.project/project.yaml` + `--answers` override. Markdown + JSON reports. Four documented exit codes (0/1/2/3) plus grep-able stderr summary. New `darnit.harness` subpackage (`driver`, `answer_sources`, `report`, `exit_codes`). @@ -380,5 +381,5 @@ else: For additional context about technologies to be used, project structure, shell commands, and other important information, read the current plan: -[`specs/028-audit-parity-tests/plan.md`](specs/028-audit-parity-tests/plan.md) +[`specs/029-openai-parity-adapter/plan.md`](specs/029-openai-parity-adapter/plan.md) diff --git a/pyproject.toml b/pyproject.toml index 76e9b11..08742cd 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -57,6 +57,11 @@ dev = [ # explicitly. Not part of `[dev]` per PR #370 review feedback. parity-tier2 = [ "claude-agent-sdk>=0.1.0", + # Feature 029: Tier 2 parity check adds an OpenAI backend alongside the + # Claude Agent SDK path. TEST-ONLY dep; MUST NOT appear in any darnit + # product package's pyproject.toml (SC-006). Pinned >=1.50 for the + # Chat Completions tool-calling surface used by openai_backend.py. + "openai>=1.50", ] [tool.uv.workspace] diff --git a/specs/029-openai-parity-adapter/checklists/requirements.md b/specs/029-openai-parity-adapter/checklists/requirements.md new file mode 100644 index 0000000..765834e --- /dev/null +++ b/specs/029-openai-parity-adapter/checklists/requirements.md @@ -0,0 +1,75 @@ +# Specification Quality Checklist: OpenAI Tier 2 Parity Adapter + +**Purpose**: Validate specification completeness and quality before proceeding to planning + +**Created**: 2026-08-10 + +**Feature**: [spec.md](../spec.md) + +## Content Quality + +- [X] No implementation details (languages, frameworks, APIs) +- [X] Focused on user value and business needs +- [X] Written for non-technical stakeholders +- [X] All mandatory sections completed + +## Requirement Completeness + +- [X] No [NEEDS CLARIFICATION] markers remain +- [X] Requirements are testable and unambiguous +- [X] Success criteria are measurable +- [X] Success criteria are technology-agnostic (no implementation details) +- [X] All acceptance scenarios are defined +- [X] Edge cases are identified +- [X] Scope is clearly bounded +- [X] Dependencies and assumptions identified + +## Feature Readiness + +- [X] All functional requirements have clear acceptance criteria +- [X] User scenarios cover primary flows +- [X] Feature meets measurable outcomes defined in Success Criteria +- [X] No implementation details leak into specification + +## Notes + +- This feature closes issue #368 (opened during feature 028's clarify pass as the follow-up for provider-agnostic Tier 2 checks). +- Two spec-level decisions were made explicitly in the spec rather than deferred as [NEEDS CLARIFICATION]: + - **Separate workflow per provider** (FR-005): rather than one aggregate workflow with a `provider` input. This preserves per-provider governance -- the OpenAI Environment has its own reviewer list distinct from the Claude Environment. + - **Shared skill prompt snapshot** (FR-009): feature 028's `skill_prompt_snapshot.md` is used verbatim by both backends. If provider-specific transformations are required (e.g., differing tool-call syntax), those live in the adapter, not in a forked snapshot. +- Corners intentionally deferred to /speckit-clarify: + - Which specific OpenAI API surface (Assistants API vs Chat Completions with tools). Both work; the choice affects turn-loop implementation but not the spec's contract. + - Whether the `NoopBackend` used to prove SC-005 / SC-007 lives in the tests package or is a documented "how to write a backend" reference. Plan-phase decision. + - The exact CI cadence question (US3 aggregate reporting) -- Priority 3, out of scope for MVP, no clarify question needed. +- Constitution IV echo: the OpenAI adapter, like the Claude adapter, MUST NOT modify the `/darnit-audit` skill it diagnoses. Any prompt-shape transformation is adapter-internal. +- Feature dependencies: feature 028 (parity test suite) is hard-required. Its `SkillReport` parser, `Tier2DiffReport` differ, `write_fixture_artifacts` writer, and `run.py` runner CLI are all consumed by this feature -- extended, but not forked. + +## Clarification Session Log + +Five clarifications recorded during the 2026-08-10 clarify session: + +1. **OpenAI API surface** -> Chat Completions with `tools=[...]` and hand-rolled tool-call loop. Stateless per-invocation; symmetric with feature 028's Claude adapter. (FR-001) +2. **Backend registration mechanism** -> Simple factory dict in a shared module; no entry-point discovery. TEST-ONLY seam; distinct from feature 027's product-facing `QuestionResolver`. (FR-004) +3. **Turn cap exhausted** -> New distinct outcome `turn_cap_exhausted` with exit code `5`. Diagnostically separate from `unparseable` and `per_control_disagree`. (FR-010; SC-011 added) +4. **Model default** -> Pin a version-suffixed string in the workflow YAML (e.g., `gpt-4o-2024-08-06`). Reproducibility is load-bearing for a diagnostic; moving aliases forbidden. (SC-010 added) +5. **NoopBackend location** -> Test-only fixture at `tests/darnit/parity/tier2/backends/noop.py`; Protocol shape documented in `contracts/skill-invocation-backend-protocol.md` for real backend authors. + +Two new SCs surfaced from these decisions: SC-010 (pinned model check) and SC-011 (turn-cap adversarial test). Coverage after clarify: 17 FR + 11 SC = 28 requirements, all with concrete acceptance criteria. + +## /speckit-analyze findings applied + +The 2026-08-11 analyze pass surfaced 8 findings (0 CRITICAL, 0 HIGH, 4 MEDIUM, 4 LOW). Applied remediations: + +- **MC1 (FR-013 fixture-diff)**: New task T024a manually verifies no fixture files were modified in this PR. Documented as a soft-constraint pre-PR check rather than a test. +- **MC2 (FR-014 parser reuse test)**: T016 gains a subtest `test_openai_style_markdown_is_parseable_by_shared_parser` that feeds an OpenAI-shaped Markdown response through feature 028's `SkillReport.parse()` and asserts parseable. +- **MC3 (shim export inventory)**: New task T009a creates `test_shim_exports.py` that imports every public name from feature 028's original module surface via the shim path. +- **MC4 (rebase watch list)**: New "Rebase conflict watch list" section in tasks.md enumerates the 6 files most likely to conflict on rebase from feature 028's PR review. +- **LC1 (T007/T008 ordering)**: Deps chart updated to explicitly state T008 runs before T007. +- **LC3 (Environment UI callout)**: New "Before-merge maintainer actions" section (M1-M4) documents the manual GitHub UI configuration that no code task performs. +- **LC4 (T009 canary list)**: T009 gains a pointer to the feature-028 test files most likely to surface a shim regression. + +Not applied: + +- **LC2 (T018 split)**: Would renumber tasks; declined as churn without material benefit. + +Task count after remediation: 32 tasks (T001-T024a, T029 with T009a, T024a intercalated). Coverage after remediation: 27/28 requirements have a concrete task or automated check; 1/28 (SC-009 30-min corpus wall clock) remains manual verification post-merge, as designed. diff --git a/specs/029-openai-parity-adapter/contracts/openai-workflow.md b/specs/029-openai-parity-adapter/contracts/openai-workflow.md new file mode 100644 index 0000000..62f0d2f --- /dev/null +++ b/specs/029-openai-parity-adapter/contracts/openai-workflow.md @@ -0,0 +1,66 @@ +# Contract: `parity-tier2-openai.yml` Workflow + +**Feature**: 029-openai-parity-adapter | **Consumers**: maintainers configuring the `parity-tier2-openai` Environment; reviewers approving OpenAI dispatches; auditors verifying access-control compliance. + +Mirrors feature 028's `parity-tier2.yml` contract (`tier2-workflow.md`) for the OpenAI backend. Governance-critical properties are enforced identically. + +## 1. Trigger + +- **OW-1**: Triggered EXCLUSIVELY by `workflow_dispatch`. No `push`, no `pull_request`, no `schedule`. +- **OW-2**: Inputs: `fixture_glob` (default `"*"`) AND `model` (default `gpt-4o-2024-08-06` -- SC-010 requires a version-suffixed default). +- **OW-3**: The `model` input's default MUST be a version-suffixed string. A moving alias (e.g., `gpt-4o` alone) fails the `test_openai_workflow_pins_versioned_model` check (SC-010). + +## 2. Environment + +- **OW-4**: Job MUST declare `environment: parity-tier2-openai`. Distinct from feature 028's `parity-tier2`. +- **OW-5**: GitHub UI (NOT this YAML) MUST configure the `parity-tier2-openai` Environment with: + - A required-reviewer list of authorized maintainers. + - `OPENAI_API_KEY` stored at the ENVIRONMENT level. + - No other secrets in this Environment (blast-radius minimization). + +## 3. Permissions + +- **OW-6**: `permissions: contents: read` at the job level. No `write` scope granted to any resource. + +## 4. Key exclusivity + +- **OW-7**: No other workflow references `secrets.OPENAI_API_KEY`. Verifiable by `test_workflow_config.py::test_openai_key_only_in_openai_workflow` (SC-002). +- **OW-8**: `OPENAI_API_KEY` does NOT appear in the `parity-tier2.yml` file (feature 028's Claude workflow). +- **OW-9**: `ANTHROPIC_API_KEY` does NOT appear in `parity-tier2-openai.yml`. The two workflows have exclusive per-provider keys. + +## 5. Preflight audit + +- **OW-10**: A preflight step MUST log actor + SHA + fixture_glob + selected model to `$GITHUB_STEP_SUMMARY` BEFORE the SDK-invocation step consumes `OPENAI_API_KEY`. + +## 6. Runner invocation + +- **OW-11**: The SDK step invokes `uv run python -m tests.darnit.parity.tier2.run --backend openai --fixture-glob --model `. +- **OW-12**: The step's `env:` block sets `OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }}`. +- **OW-13**: `ANTHROPIC_API_KEY` is NOT set in the step's `env:` block. This is enforced by an assertion in `test_workflow_config.py` that greps `parity-tier2-openai.yml` for `ANTHROPIC_API_KEY` and asserts the count is zero. + +## 7. Artifact upload + +- **OW-14**: `actions/upload-artifact@v4` runs with `if: always()` so failure artifacts land on any exit code. +- **OW-15**: Upload path is `parity-artifacts/`, same as feature 028. The OpenAI backend writes `openai_final_message.md` per fixture (distinct filename from feature 028's `skill_final_message.md`) so both providers' artifacts can coexist under the same fixture directory across dispatches. + +## 8. Exit codes + +- **OW-16**: Runner exit codes for OpenAI backend match the extended set from feature 028 + feature 029: + - `0` -- success + - `1` -- per_control_disagree or counts_disagree + - `2` -- skill_unparseable + - `3` -- setup (missing `OPENAI_API_KEY`) + - `4` -- rate limit + - `5` -- turn_cap_exhausted (NEW in feature 029) + +## 9. Rate limit handling + +- **OW-17**: The runner MUST NOT retry API calls automatically on rate limit. Same policy as feature 028's Claude workflow. + +## 10. Reviewer checklist + +Before approving a dispatch of `parity-tier2-openai.yml`, the reviewer verifies: + +- The dispatcher (github.actor) is listed on the workflow run and matches an authorized maintainer. +- The `fixture_glob` input matches the intended investigation scope (`"*"` for full-corpus check, a specific fixture name for targeted debugging). +- The `model` input either uses the workflow's pinned default OR is an explicitly overridden version-suffixed string. If a moving alias (e.g., `gpt-4o`) is in the model input, decline the approval and instruct the dispatcher to specify a versioned model. diff --git a/specs/029-openai-parity-adapter/contracts/skill-invocation-backend-protocol.md b/specs/029-openai-parity-adapter/contracts/skill-invocation-backend-protocol.md new file mode 100644 index 0000000..5ece740 --- /dev/null +++ b/specs/029-openai-parity-adapter/contracts/skill-invocation-backend-protocol.md @@ -0,0 +1,83 @@ +# Contract: `SkillInvocationBackend` Protocol + +**Feature**: 029-openai-parity-adapter | **Consumers**: authors of future Tier 2 provider adapters (Gemini, xAI, self-hosted). Also consumed by `run.py` and by Protocol-conformance tests. + +## 1. Shape + +```python +@runtime_checkable +class SkillInvocationBackend(Protocol): + name: str + + async def invoke( + self, + fixture_dir: Path, + model: str, + max_turns: int, + ) -> SkillInvocationResult: + ... + + @classmethod + def check_env(cls) -> None: + ... +``` + +- **B-1**: A backend MUST expose a class-level or instance-level `name: str` attribute. Convention: snake_case, provider-name-first (e.g., `claude_agent_sdk`, `openai`, `gemini_generative_ai`). +- **B-2**: A backend MUST expose an async `invoke(fixture_dir, model, max_turns) -> SkillInvocationResult`. Model and max_turns are supplied by the runner (from CLI flags or workflow YAML); backends do NOT default them. +- **B-3**: A backend MUST expose `check_env()` as a classmethod so the runner can fail fast on missing credentials WITHOUT constructing an instance. `check_env()` raises `SetupError` (imported from `backends.base`) with a message identifying the missing environment variable(s). +- **B-4**: A backend MAY expose additional attributes/methods; the runner ignores them. +- **B-5**: `isinstance(instance, SkillInvocationBackend)` MUST return True for any conforming class instance (this is the SC-005 test target). + +## 2. `SkillInvocationResult` contract + +- **B-6**: `invoke()` returns a `SkillInvocationResult` frozen dataclass with the fields defined in `data-model.md` section 2. All new backends MUST populate `final_message`, `model`, `turn_count`, and `metadata` (may be empty dict). +- **B-7**: `turn_cap_exhausted: bool` MUST be True iff the model exhausted the turn cap without emitting a final text message. Backends where the concept of "turn cap" doesn't apply (e.g., a synchronous single-call backend) always set it False. +- **B-8**: `final_message` MUST be the string the parser will consume. Empty string is a legal value only when `turn_cap_exhausted=True`. + +## 3. Registration + +- **B-9**: A new backend registers itself in `tests/darnit/parity/tier2/backends/__init__.py` by adding an entry to `BACKEND_REGISTRY`: + + ```python + BACKEND_REGISTRY = { + "claude_agent_sdk": ClaudeAgentSdkBackend, + "openai": OpenAIBackend, + "my_new_provider": MyNewProviderBackend, # <- adds here + } + ``` + +- **B-10**: This is a TEST-ONLY registration mechanism. Entry-point-based discovery is EXPLICITLY OUT OF SCOPE for this Protocol (clarify Q2 established this). + +## 4. Credentials + env + +- **B-11**: `check_env()` MUST NOT make any network call; it only inspects environment variables (or, for a self-hosted backend, whatever local resource identifies "credentials present"). +- **B-12**: `check_env()` SHOULD be idempotent -- callable arbitrarily many times. +- **B-13**: If a backend's credentials are ABSENT, `check_env()` MUST raise `SetupError` naming the missing variable(s). The runner catches this and returns exit code 3. + +## 5. Turn count + budget + +- **B-14**: `max_turns` is the CALLER's contract with the backend. The backend MUST NOT exceed it. Exceeding is a bug (specifically, it would break the "runaway budget" governance property). +- **B-15**: `turn_count` in the returned result MUST equal the number of assistant turns actually taken. + +## 6. Tool invocation + +- **B-16**: A backend that supports tool calls MUST invoke the darnit MCP tools (specifically `audit_openssf_baseline`) by calling their Python functions directly. The backend does NOT go through an MCP protocol wrapper. +- **B-17**: The backend MUST force `local_path` to the `fixture_dir` argument on every tool call, to prevent a rogue model from wandering outside the fixture. + +## 7. Prompt + +- **B-18**: All backends consume the same skill prompt snapshot at `tests/darnit/parity/tier2/skill_prompt_snapshot.md`. Backends do NOT fork this file. Provider-specific transformations (e.g., prepending tool-choice guidance for a model that needs it) live in the backend's own module. + +## 8. Reproducibility + +- **B-19**: Backends SHOULD use `temperature=0.0` or the provider's equivalent low-variance setting when the API exposes it. This isn't strictly enforceable across all providers but is expected behavior. +- **B-20**: Backends MUST NOT introduce random behavior (no `random.random()` calls, no `time`-based seeding). Two invocations against the same fixture with the same model SHOULD produce byte-identical `final_message` output. + +## 9. Backwards compatibility + +- **B-21**: Feature 029 introduces the Protocol. Feature 028's Claude adapter is refactored to satisfy it; the old import path `tests.darnit.parity.tier2.claude_agent_sdk_client` remains via a shim. Third parties adding a backend after 029 do NOT need to touch feature 028's or feature 029's existing files -- they add a new module and register it in `BACKEND_REGISTRY`. + +## 10. Non-goals + +- **B-22**: This Protocol is NOT a general-purpose "abstract over LLM providers" abstraction. It's specifically for Tier 2 skill-drift measurement. Product code (harness, MCP tools, etc.) does NOT consume it. +- **B-23**: The Protocol does NOT include streaming, cost tracking, or usage aggregation. Those are backend-internal concerns. diff --git a/specs/029-openai-parity-adapter/data-model.md b/specs/029-openai-parity-adapter/data-model.md new file mode 100644 index 0000000..4950ce8 --- /dev/null +++ b/specs/029-openai-parity-adapter/data-model.md @@ -0,0 +1,310 @@ +# Phase 1 Data Model: OpenAI Tier 2 Parity Adapter + +**Feature**: 029-openai-parity-adapter | **Date**: 2026-08-10 + +All entities are TEST-side only. No product data model changes. + +## 1. `SkillInvocationBackend` (Protocol) + +Module: `tests/darnit/parity/tier2/backends/base.py` + +```python +@runtime_checkable +class SkillInvocationBackend(Protocol): + name: str + + async def invoke( + self, + fixture_dir: Path, + model: str, + max_turns: int, + ) -> SkillInvocationResult: + ... + + @classmethod + def check_env(cls) -> None: + """Raise SetupError if the provider's credentials are absent.""" + ... +``` + +**Conformance verified by**: SC-005 test (`test_backend_protocol_conformance.py`) enumerates every entry in `BACKEND_REGISTRY` and asserts `isinstance(instance, SkillInvocationBackend)`. + +**Alternative Protocol shape rejected**: an `abc.ABC` subclass was considered but rejected in R1 -- Protocol is more Pythonic for duck-typed adapters. + +## 2. `SkillInvocationResult` + +Module: `tests/darnit/parity/tier2/backends/base.py` + +```python +@dataclass(frozen=True) +class SkillInvocationResult: + final_message: str + model: str + turn_count: int + metadata: dict[str, Any] = field(default_factory=dict) + turn_cap_exhausted: bool = False # NEW in feature 029 +``` + +Feature 028's Claude adapter returned this same dataclass minus `turn_cap_exhausted`. Adding the field with a default preserves backwards compat: Claude adapter continues to construct results without setting it; runner treats absence as False. + +**Validation rules**: None at the dataclass level. `turn_count` MUST be non-negative but not enforced by validators (test-side; frozen dataclass suffices). + +## 3. `SetupError` + +Module: `tests/darnit/parity/tier2/backends/base.py` + +```python +class SetupError(RuntimeError): + """Raised when a backend lacks the credentials/env it needs to invoke.""" +``` + +Same class from feature 028's `claude_agent_sdk_client.py`; moved to `backends/base.py` and re-exported at the old path for backwards compat. `check_env()` classmethods raise this. `run.py` catches it and returns exit code 3 (setup). + +## 4. `BACKEND_REGISTRY` + +Module: `tests/darnit/parity/tier2/backends/__init__.py` + +```python +from .base import SkillInvocationBackend, SkillInvocationResult, SetupError +from .claude_agent_sdk import ClaudeAgentSdkBackend +from .openai_backend import OpenAIBackend +from .noop import NoopBackend + +BACKEND_REGISTRY: dict[str, type[SkillInvocationBackend]] = { + "claude_agent_sdk": ClaudeAgentSdkBackend, + "openai": OpenAIBackend, + # NoopBackend intentionally not registered here -- tests inject it. +} +``` + +**Lifecycle**: module-level. Cheap; no lazy loading. `openai` imports its SDK at module load time; if `openai` isn't installed, the registry itself fails to import -- this is fine because Tier 2 is a dev-dep-required test surface. + +**Test injection**: `run.py` accepts an optional `backends: dict[str, ...]` parameter that overrides `BACKEND_REGISTRY` for that invocation. Tests use this to inject `NoopBackend` without touching the module dict. + +## 5. `ClaudeAgentSdkBackend` (refactored) + +Module: `tests/darnit/parity/tier2/backends/claude_agent_sdk.py` + +Refactor of feature 028's `claude_agent_sdk_client.py::invoke_skill` into a class: + +```python +class ClaudeAgentSdkBackend: + name = "claude_agent_sdk" + + @classmethod + def check_env(cls) -> None: + if not os.environ.get("ANTHROPIC_API_KEY"): + raise SetupError(...) + + async def invoke( + self, fixture_dir: Path, model: str, max_turns: int, + ) -> SkillInvocationResult: + # Body is feature 028's invoke_skill() logic, unchanged. + ... +``` + +**Backwards-compat shim** at `tests/darnit/parity/tier2/claude_agent_sdk_client.py`: + +```python +"""Backwards-compat re-export shim; superseded by +`tests/darnit/parity/tier2/backends/claude_agent_sdk.py`. Kept as an import +path for one release cycle so existing feature-028 tests continue to work +without an update.""" + +from tests.darnit.parity.tier2.backends.base import ( + SetupError, + SkillInvocationResult, +) +from tests.darnit.parity.tier2.backends.claude_agent_sdk import ( + ClaudeAgentSdkBackend, +) + + +async def invoke_skill(fixture_dir, model="anthropic:claude-sonnet-5", max_turns=20): + """Deprecated: use ClaudeAgentSdkBackend.invoke() directly.""" + return await ClaudeAgentSdkBackend().invoke(fixture_dir, model, max_turns) + + +__all__ = ("SetupError", "SkillInvocationResult", "invoke_skill") +``` + +## 6. `OpenAIBackend` (new) + +Module: `tests/darnit/parity/tier2/backends/openai_backend.py` + +```python +class OpenAIBackend: + name = "openai" + + @classmethod + def check_env(cls) -> None: + if not os.environ.get("OPENAI_API_KEY"): + raise SetupError( + "Tier 2 OpenAI backend requires OPENAI_API_KEY. " + "Configure the `parity-tier2-openai` GitHub Environment or export " + "the var for a local run.", + ) + + async def invoke( + self, fixture_dir: Path, model: str, max_turns: int, + ) -> SkillInvocationResult: + # See research.md R3 for the loop shape. + # See R4 for the tool schema. + # Uses openai.AsyncOpenAI + client.chat.completions.create(...). + ... +``` + +**Model default**: The backend does NOT default the model; the CALLER (`run.py` or the workflow YAML) supplies it. The workflow pins `gpt-4o-2024-08-06`; `run.py --model ` can override for local dev. + +**Temperature**: `temperature=0.0` for reproducibility. + +**Tool schemas**: See section 8. + +## 7. `NoopBackend` (test fixture) + +Module: `tests/darnit/parity/tier2/backends/noop.py` + +```python +class NoopBackend: + """Test-only backend used by SC-005 (Protocol conformance) and SC-007 + (extensibility). NOT registered in BACKEND_REGISTRY by default; tests + inject it explicitly. + """ + + name = "noop" + + @classmethod + def check_env(cls) -> None: + # No credentials required. + return None + + async def invoke( + self, fixture_dir: Path, model: str, max_turns: int, + ) -> SkillInvocationResult: + return SkillInvocationResult( + final_message="# noop backend\n\nPassed: 0\nFailed: 0", + model=model, + turn_count=0, + metadata={"backend": "noop"}, + ) +``` + +**Not shipped as a template**: real backend authors read `contracts/skill-invocation-backend-protocol.md`, not this class. + +## 8. Tool schema for `audit_openssf_baseline` + +Module: `tests/darnit/parity/tier2/backends/openai_backend.py` (module-level constant) + +```python +_TOOL_SCHEMAS = [ + { + "type": "function", + "function": { + "name": "audit_openssf_baseline", + "description": ( + "Run darnit's OpenSSF Baseline audit on the repository at the " + "given local_path. Returns a JSON string with per-control results." + ), + "parameters": { + "type": "object", + "properties": { + "local_path": { + "type": "string", + "description": "Absolute path to the repository being audited.", + }, + "level": { + "type": "integer", + "enum": [1, 2, 3], + "default": 3, + }, + "output_format": { + "type": "string", + "enum": ["markdown", "json"], + "default": "json", + }, + }, + "required": ["local_path"], + }, + }, + }, +] +``` + +**Dispatch**: + +```python +def _dispatch_tool_call(call, fixture_dir): + if call.function.name == "audit_openssf_baseline": + args = json.loads(call.function.arguments) + # Force local_path to the fixture_dir; ignore the model's suggestion + # to prevent it from wandering outside the fixture. + args["local_path"] = str(fixture_dir) + return audit_openssf_baseline(**args) + return f"unknown tool: {call.function.name}" +``` + +## 9. Runner extension (`run.py` update) + +- Add `--backend ` argument, default `claude_agent_sdk`. +- Add `--model ` argument (both backends accept this; each backend's workflow YAML supplies its provider-appropriate pinned default). +- Add `--max-turns ` argument, default 20. +- Handle new outcome `turn_cap_exhausted` in the exit-code aggregation (exit 5). + +**`_run_skill()`** replaced by: + +```python +async def _run_skill(fixture_dir, backend_name, model, max_turns, dry_run): + if dry_run: + return SkillInvocationResult(final_message=_DRY_RUN_STUB, model="dry-run", turn_count=0, metadata={"dry_run": True}) + backend_cls = BACKEND_REGISTRY[backend_name] + backend_cls.check_env() # fail fast if credentials absent + backend = backend_cls() + return await backend.invoke(fixture_dir, model, max_turns) +``` + +## 10. `artifact_writer` provider extension + +Module: `tests/darnit/parity/tier2/artifact_writer.py` + +Add optional `provider` parameter: + +```python +def write_fixture_artifacts( + artifact_root, fixture_name, mcp_json, skill_markdown, diff_md, + metadata=None, provider: str = "claude", +): + fixture_dir = artifact_root / fixture_name + fixture_dir.mkdir(parents=True, exist_ok=True) + (fixture_dir / "mcp_tool_result.json").write_text(mcp_json) + + # Provider-specific filename for the final message artifact. + final_message_name = ( + "skill_final_message.md" if provider == "claude" + else f"{provider}_final_message.md" + ) + (fixture_dir / final_message_name).write_text(skill_markdown) + (fixture_dir / "diff_report.md").write_text(diff_md) + ... +``` + +## 11. State transitions + +Feature 029 introduces no persistent state. All state is per-run in memory. The `turn_cap_exhausted` bool moves through: + +``` +Backend loop starts (turn=0) + | + v +Turn N (N < max_turns): model returns tool_call -> execute tool, append result, continue + | + v +Turn N: model returns text content -> return SkillInvocationResult(turn_cap_exhausted=False) + | + v +Turn N == max_turns: loop exits without text -> return SkillInvocationResult(turn_cap_exhausted=True, final_message="") + | + v +runner.py sees turn_cap_exhausted=True -> outcome="turn_cap_exhausted" -> exit code 5 +``` + +Nothing persists between runs. diff --git a/specs/029-openai-parity-adapter/plan.md b/specs/029-openai-parity-adapter/plan.md new file mode 100644 index 0000000..d39e3d2 --- /dev/null +++ b/specs/029-openai-parity-adapter/plan.md @@ -0,0 +1,117 @@ +# Implementation Plan: OpenAI Tier 2 Parity Adapter + +**Branch**: `029-openai-parity-adapter` | **Date**: 2026-08-10 | **Spec**: [spec.md](spec.md) + +**Input**: Feature specification from `specs/029-openai-parity-adapter/spec.md` (with 5 clarifications from `/speckit-clarify` on 2026-08-10: OpenAI Chat Completions API with hand-rolled tool loop; factory-dict registry; `turn_cap_exhausted` outcome + exit 5; pinned version-suffixed model default; `NoopBackend` as test-only fixture). + +## Summary + +Adds a second Tier 2 provider adapter to feature 028's parity test suite. Extracts a `SkillInvocationBackend` Protocol during the refactor so both the existing Claude adapter (from feature 028) and the new OpenAI adapter satisfy the same shape. A backend factory dict + `--backend ` CLI flag select which adapter runs per dispatch. + +Ships: +- **`SkillInvocationBackend` Protocol** (`tests/darnit/parity/tier2/backends/base.py`): async `invoke(fixture_dir) -> SkillInvocationResult` + `check_env() -> None` + `name: str`. `@runtime_checkable`. +- **Claude adapter refactor**: existing `claude_agent_sdk_client.py` moves to `backends/claude_agent_sdk.py` and is refactored to a class satisfying the Protocol. Zero behavior change; the existing `invoke_skill()` function becomes a class method. +- **OpenAI adapter** (`backends/openai_backend.py`): Chat Completions API loop; darnit MCP tools registered as function-callable `tools=[...]`; caps at 20 turns by default; returns `SkillInvocationResult`. +- **Runner update** (`run.py`): `--backend ` flag; looks up `BACKEND_REGISTRY[name]`; unchanged runner behavior for `--backend claude_agent_sdk` (default preserves feature 028). +- **OpenAI-specific workflow** (`.github/workflows/parity-tier2-openai.yml`): manual-dispatch only, `environment: parity-tier2-openai` (separate from feature 028's `parity-tier2`), `OPENAI_API_KEY` at Environment level, pinned model default `gpt-4o-2024-08-06`. +- **New failure class**: `turn_cap_exhausted` outcome + exit code 5. +- **NoopBackend fixture**: `backends/noop.py`; used by conformance and extensibility tests only. + +Closes #368. Zero product-package changes -- feature 028's `test_no_product_changes.py` guard already covers `packages/*/src/` and it stays untouched. + +## Technical Context + +**Language/Version**: Python 3.11 / 3.12 (workspace targets, unchanged). + +**Primary Dependencies (new -- test-side only)**: +- `openai>=1.50` (Anthropic-agnostic OpenAI Python SDK, version pinned to a range that includes the current Chat Completions surface with tool-calling). Added to workspace-level `pyproject.toml` dev group, alongside feature 028's `claude-agent-sdk`. TEST-ONLY per SC-006. +- No new stdlib usage beyond what feature 028 already touched. + +**Primary Dependencies (in use)**: `pytest`, `PyYAML` (already a workspace dep for workflow-config tests), `claude-agent-sdk` (from feature 028), feature 028's own modules: `AuditResult`, `SkillReport`, `Tier2DiffReport`, `write_fixture_artifacts`. + +**Storage**: Filesystem only. Fixture corpus reused from feature 028 unchanged. Artifact bundles land at `parity-artifacts//` per-provider; different workflow dispatches (Claude vs OpenAI) can share the same artifact path (each dispatch overwrites) or write to distinct subdirs -- plan-phase detail below. + +**Testing**: pytest for offline tests (Protocol conformance, backend adversarial, workflow config). No live API calls in the test suite. Adversarial cases mock the `openai` client at the module level. + +**Target Platform**: `ubuntu-latest` GitHub-hosted runner for the workflow; any host for local development. + +**Project Type**: Test-suite addition. No product-package code changes. + +**Performance Goals**: SC-009 -- full corpus (4-6 fixtures) in under 30 minutes. Per fixture: capped by 20-turn budget * per-turn latency (5-30s) + audit cost (a few seconds). Realistic upper bound per fixture: ~10 minutes. Corpus: ~40 minutes worst case, ~15 minutes typical. + +**Constraints**: +- **SC-006 preservation**: no dep additions to `packages/darnit/pyproject.toml` or `packages/darnit-baseline/pyproject.toml`. Enforced by feature 028's `test_no_product_changes.py`. +- **SC-002 (FR-007)**: `OPENAI_API_KEY` MUST appear only in `parity-tier2-openai.yml`. Verifiable by iterating `.github/workflows/*.yml` and asserting `OPENAI_API_KEY` literal count is exactly 1 (in the OpenAI workflow file). Mirror of feature 028's SC-005a for `ANTHROPIC_API_KEY`. +- **SC-010**: model default in the workflow YAML MUST match a version-suffixed pattern (e.g., `gpt-4o-2024-08-06`). Moving aliases (`gpt-4o` alone) fail the workflow-config test. +- **Stateless per-invocation** (FR-001): no persistent thread/assistant objects. +- **FR-009**: shared skill prompt snapshot with feature 028. No fork; adapter-side transformation for OpenAI's tool-call syntax if the SDK requires it. + +**Scale/Scope**: MVP is the Protocol seam + OpenAI adapter + governance-gated workflow + adversarial tests. Expected size: ~600-800 lines net production (backend adapters + Protocol module + runner update) + ~500 lines tests + one workflow YAML + one contract doc. + +## Constitution Check + +Constitution v1.3.0. Five Core Principles evaluated as gates. + +| Principle | Applicable? | Verdict | Rationale | +|-----------|-------------|---------|-----------| +| I. Plugin Separation | Yes | PASS | The Protocol + backends live under `tests/darnit/parity/tier2/backends/`. Zero code added to `packages/darnit-core` or `packages/darnit-baseline`. The runner consumes `audit_openssf_baseline` as a callable (same as feature 028) but doesn't modify it. The OpenAI SDK is a test-only dev-group dep. | +| II. Conservative-by-Default | Yes | PASS + REINFORCED | This feature exists to protect Principle II across a second provider surface. If an OpenAI-backed coding assistant silently reclassifies a WARN as PASS in its summary, this test catches it. Extending the diagnostic surface strengthens the "WARN counts as FAIL" invariant against provider drift. | +| III. TOML-First Architecture | No | N/A | No control definitions. No TOML changes. | +| IV. Never Guess User Values | No (indirect) | PASS | The runner does not fabricate or heuristically fill any context value. Every value the OpenAI assistant might read comes from the fixture repo's own `.project/project.yaml`. The `NoopBackend` similarly has no context inference. Principle IV is preserved by construction. | +| V. Sieve Pipeline Integrity | No | N/A | This feature is downstream of the sieve. | + +**No violations.** No Complexity Tracking entries required. + +Governance observations (feature-028-lineage): +- Manual-dispatch only for MVP; FR-007 (issue-#369 follow-up) still applies: scheduled cadence + governance-appropriate key sourcing is deferred to a separate feature. +- Two independent Environments (`parity-tier2` for Claude, `parity-tier2-openai` for OpenAI). Each has its own reviewer list; approvals are provider-scoped. That gives per-provider accountability -- an OpenAI-tier reviewer can approve OpenAI runs without being trusted for Anthropic-cost runs, and vice versa. + +## Project Structure + +### Documentation (this feature) + +```text +specs/029-openai-parity-adapter/ ++-- spec.md # /speckit-specify + /speckit-clarify output ++-- plan.md # this file ++-- research.md # Phase 0 ++-- data-model.md # Phase 1 ++-- quickstart.md # Phase 1 ++-- contracts/ +| +-- skill-invocation-backend-protocol.md # Protocol shape for backend authors +| +-- openai-workflow.md # workflow_dispatch shape + Environment config ++-- checklists/ +| +-- requirements.md # spec-quality checklist (exists) ++-- tasks.md # /speckit-tasks output +``` + +### Source Code (repository root) + +Everything ships under `tests/` and `.github/workflows/`. Zero product changes. + +```text +tests/darnit/parity/tier2/ ++-- backends/ # NEW: the Protocol seam +| +-- __init__.py # BACKEND_REGISTRY dict + Protocol re-export +| +-- base.py # NEW: SkillInvocationBackend Protocol + SkillInvocationResult dataclass + SetupError +| +-- claude_agent_sdk.py # REFACTORED FROM claude_agent_sdk_client.py -- class satisfying Protocol +| +-- openai_backend.py # NEW: OpenAIBackend, Chat Completions loop +| +-- noop.py # NEW: test-only NoopBackend for conformance/extensibility tests ++-- claude_agent_sdk_client.py # DELETED (superseded by backends/claude_agent_sdk.py; imports re-exported for one release) ++-- run.py # UPDATED: --backend flag; BACKEND_REGISTRY lookup; new exit code 5 ++-- diff.py # UPDATED: recognize turn_cap_exhausted outcome + emit its diff report shape ++-- test_openai_backend_adversarial.py # NEW: adversarial tests for the OpenAI adapter ++-- test_backend_protocol_conformance.py # NEW: SC-005 protocol conformance check across all registered backends ++-- test_backend_extensibility.py # NEW: SC-007 -- NoopBackend registers/invokes without shared-module edits ++-- test_workflow_config.py # UPDATED: assertions extended to parity-tier2-openai.yml + SC-010 model-pin check + SC-002 OPENAI_API_KEY-exclusive-file check + +.github/workflows/ ++-- parity-tier2-openai.yml # NEW: manual-dispatch, Environment-gated, pinned model default ++-- parity-tier2.yml # UNCHANGED +``` + +**Structure Decision**: Additive-only. All feature-028 test files are extended (not forked); the one refactor (moving `claude_agent_sdk_client.py` into `backends/claude_agent_sdk.py`) preserves the module surface via a re-export line in the old path. Existing feature-028 tests are updated to import from the new location, but no test's behavior changes. + +## Complexity Tracking + +No violations. Section intentionally empty. diff --git a/specs/029-openai-parity-adapter/quickstart.md b/specs/029-openai-parity-adapter/quickstart.md new file mode 100644 index 0000000..eaeb255 --- /dev/null +++ b/specs/029-openai-parity-adapter/quickstart.md @@ -0,0 +1,170 @@ +# Quickstart: OpenAI Tier 2 Parity Adapter + +**Feature**: 029-openai-parity-adapter | **For**: authorized maintainers dispatching an OpenAI Tier 2 parity check, and future authors adding a third-provider backend adapter. + +## Dispatching the OpenAI Tier 2 workflow + +Tier 2 is manual-dispatch only. An authorized maintainer approves each run. + +```bash +gh workflow run parity-tier2-openai.yml \ + --repo darnitdevorg/darnit \ + -f fixture_glob="*" \ + -f model="gpt-4o-2024-08-06" +``` + +Or via GitHub UI: + +1. Actions -> "Parity Tier 2 (OpenAI)" -> Run workflow. +2. Pick fixture glob (default `"*"`). +3. Optionally override the model (default is the pinned version-suffixed default). +4. Click "Run workflow." +5. Wait for the approval-required badge to appear on the run. +6. An authorized reviewer approves the deployment to `parity-tier2-openai`. +7. The workflow proceeds; `OPENAI_API_KEY` is injected only into the SDK-invocation step. + +### Reviewer approval checklist + +Before clicking Approve: + +- Confirm the dispatcher (github.actor) is authorized. +- Confirm the `model` input is a version-suffixed pin, not a moving alias. +- Confirm the `fixture_glob` matches the intended scope. +- Confirm the workflow YAML has not been modified in the same PR as an unrelated feature (governance red flag). + +### Interpreting the exit code + +| Exit code | Meaning | +|---|---| +| 0 | Success -- OpenAI backend agrees with tool on every control per fixture | +| 1 | Per-control disagreement or count disagreement | +| 2 | Skill output unparseable | +| 3 | Setup error (missing `OPENAI_API_KEY`) | +| 4 | Rate limit exhausted | +| 5 | Turn cap exhausted (model kept calling tools, never summarized) | + +### Reviewing artifacts locally + +```bash +gh run download --repo darnitdevorg/darnit +cd parity-artifacts/mixed_repo/ +cat mcp_tool_result.json | jq '.results[] | {id, status}' +cat openai_final_message.md # OpenAI backend's final message +cat skill_final_message.md # Claude backend's final message (if a Claude dispatch also ran) +cat diff_report.md +``` + +## Adding a new backend (Gemini, xAI, self-hosted, etc.) + +The Protocol seam is designed so a third-party backend needs only three additions and zero edits to shared modules. + +### Step 1: Write the backend class + +```python +# tests/darnit/parity/tier2/backends/my_provider.py + +from pathlib import Path +from tests.darnit.parity.tier2.backends.base import ( + SetupError, + SkillInvocationResult, +) + + +class MyProviderBackend: + name = "my_provider" + + @classmethod + def check_env(cls) -> None: + import os + if not os.environ.get("MY_PROVIDER_API_KEY"): + raise SetupError( + "Tier 2 my_provider backend requires MY_PROVIDER_API_KEY.", + ) + + async def invoke( + self, fixture_dir: Path, model: str, max_turns: int, + ) -> SkillInvocationResult: + # Your provider-specific invocation loop. + # See backends/openai_backend.py for a reference implementation. + ... +``` + +### Step 2: Register in `BACKEND_REGISTRY` + +```python +# tests/darnit/parity/tier2/backends/__init__.py + +from .my_provider import MyProviderBackend + +BACKEND_REGISTRY = { + "claude_agent_sdk": ClaudeAgentSdkBackend, + "openai": OpenAIBackend, + "my_provider": MyProviderBackend, # <- new line +} +``` + +### Step 3: Add a workflow file + +Copy `.github/workflows/parity-tier2-openai.yml` to `.github/workflows/parity-tier2-my-provider.yml` and: + +- Change `environment: parity-tier2-openai` -> `environment: parity-tier2-my-provider`. +- Change `OPENAI_API_KEY` -> `MY_PROVIDER_API_KEY` in the SDK step's `env:` block. +- Change `--backend openai` -> `--backend my_provider` in the runner invocation. +- Pin the model default to a versioned string appropriate for the provider. + +Configure the new Environment in GitHub UI with a reviewer list and the provider's API key. + +### Step 4: Verify + +- `test_backend_protocol_conformance.py` will automatically include your backend on next test run (it iterates `BACKEND_REGISTRY`). +- Add adversarial tests in a new file `tests/darnit/parity/tier2/test_my_provider_backend_adversarial.py` following the shape of `test_openai_backend_adversarial.py`. +- Extend `test_workflow_config.py` with an entry for `parity-tier2-my-provider.yml` mirroring the OpenAI assertions. + +**No changes required to**: `run.py`, `diff.py`, `skill_markdown_parser.py`, `artifact_writer.py`, `skill_prompt_snapshot.md`, any fixture. This is SC-007 by construction. + +## Local development against the OpenAI backend + +```bash +# Dry-run first (no API call; canned response). +uv run python -m tests.darnit.parity.tier2.run \ + --backend openai \ + --fixture-glob "all_pass_repo" \ + --dry-run + +# Real run (requires OPENAI_API_KEY export). +export OPENAI_API_KEY="sk-..." +uv run python -m tests.darnit.parity.tier2.run \ + --backend openai \ + --fixture-glob "all_pass_repo" \ + --model gpt-4o-2024-08-06 \ + --max-turns 20 \ + --artifact-dir /tmp/parity-dev +``` + +Expected artifact layout on success: + +``` +/tmp/parity-dev/ ++-- all_pass_repo/ + +-- mcp_tool_result.json + +-- openai_final_message.md + +-- diff_report.md + +-- metadata.json +``` + +## Test suite + +```bash +# Offline tests only (no API): +uv run pytest tests/darnit/parity/tier2/ -q + +# Full Tier 2 workspace: +uv run pytest tests/darnit/parity/ -q +``` + +Expected test count after feature 029 lands: feature-028 baseline + about 10-15 new tests (Protocol conformance + OpenAI adversarial + turn-cap-exhausted + workflow config). + +## Related follow-ups + +- **Issue #369**: Add scheduled cadence + governance-appropriate key sourcing (applies to Claude AND OpenAI workflows; a single follow-up covers both). +- **Future features** for additional providers: Gemini, xAI, self-hosted -- each is a fresh feature reusing the Protocol. diff --git a/specs/029-openai-parity-adapter/research.md b/specs/029-openai-parity-adapter/research.md new file mode 100644 index 0000000..33e987c --- /dev/null +++ b/specs/029-openai-parity-adapter/research.md @@ -0,0 +1,229 @@ +# Phase 0 Research: OpenAI Tier 2 Parity Adapter + +**Feature**: 029-openai-parity-adapter | **Date**: 2026-08-10 + +The five clarify decisions locked the load-bearing choices (API surface, registry mechanism, turn-cap outcome, model pin, NoopBackend location). This file covers the residual technical decisions Phase 1 design needs to sit on. + +## R1. Protocol shape (`SkillInvocationBackend`) + +**Decision**: `@runtime_checkable` Protocol in `tests/darnit/parity/tier2/backends/base.py`: + +```python +@runtime_checkable +class SkillInvocationBackend(Protocol): + name: str + async def invoke(self, fixture_dir: Path, model: str, max_turns: int) -> SkillInvocationResult: ... + @classmethod + def check_env(cls) -> None: ... # raises SetupError if provider credentials absent +``` + +- `name` is the string key used by `BACKEND_REGISTRY` and by the `--backend` CLI flag. +- `invoke()` takes an explicit `model` and `max_turns` so the runner (or workflow YAML) supplies them, not the adapter's default. +- `check_env()` is a classmethod so the runner can validate credentials WITHOUT constructing a backend instance -- cheap fail-fast. +- `SkillInvocationResult` gets a new optional `turn_cap_exhausted: bool` field alongside the existing `final_message`, `model`, `turn_count`, `metadata` fields. + +**Rationale**: Matches feature 028's existing `SkillInvocationResult` shape and its `SetupError` exception with minimal disruption. `@runtime_checkable` enables `isinstance` conformance checks in SC-005 tests. + +**Alternatives considered**: +- Abstract base class (`abc.ABC`): rejected -- Protocol is more Pythonic for duck-typed adapters and lets `NoopBackend` be a plain class without explicit inheritance. +- Separate `credentials_check` module: rejected -- keeping the check on the backend class localizes the "how do I know this provider is ready" question. + +## R2. Refactoring feature 028's Claude client + +**Decision**: Move `claude_agent_sdk_client.py` -> `backends/claude_agent_sdk.py`. Convert `invoke_skill()` function into a class `ClaudeAgentSdkBackend` with `async def invoke(...)` + `classmethod check_env()`. Preserve the existing `invoke_skill()` and `SetupError` exports at the old path via a re-export module (`tests/darnit/parity/tier2/claude_agent_sdk_client.py` becomes a shim: `from tests.darnit.parity.tier2.backends.claude_agent_sdk import ...`). Feature-028 tests continue to work unchanged. + +**Rationale**: A refactor that keeps the old import path working is safer for a stacked-PR world where feature 028 is unmerged. If 028 lands first with the current shape, this refactor's diff is minimal. + +**Alternatives considered**: +- Delete `claude_agent_sdk_client.py` entirely and update every import: rejected -- requires touching more files; risks a mid-review merge conflict with 028 test edits that could still land. +- Leave feature 028's client as-is and have the OpenAI backend live separately without a shared Protocol: rejected -- forgoes SC-005 (protocol conformance) verification and SC-007 (extensibility). + +## R3. OpenAI Chat Completions loop shape + +**Decision**: The backend implements a classic Chat Completions tool-call loop: + +```python +messages = [{"role": "system", "content": _load_skill_prompt()}, + {"role": "user", "content": f"Audit the repository at {fixture_dir}. Summarize per your usual format."}] + +for turn in range(max_turns): + response = await client.chat.completions.create( + model=model, + messages=messages, + tools=_darnit_tool_schemas(), + tool_choice="auto", + temperature=0.0, + ) + msg = response.choices[0].message + if msg.tool_calls: + messages.append(msg.model_dump(exclude_none=True)) + for call in msg.tool_calls: + result = _dispatch_tool_call(call, fixture_dir) + messages.append({ + "role": "tool", + "tool_call_id": call.id, + "content": result, + }) + continue + if msg.content: + return SkillInvocationResult(final_message=msg.content, model=model, turn_count=turn+1, ...) +# Fell out of the loop without a final text message: +return SkillInvocationResult(final_message="", turn_cap_exhausted=True, ...) +``` + +- `temperature=0.0` for reproducibility (matches feature 028's Claude adapter policy). +- `tool_choice="auto"` lets the model decide whether to call tools or answer directly. +- `_darnit_tool_schemas()` produces OpenAI-format function schemas for the audit tools the skill uses (mainly `audit_openssf_baseline`). +- `_dispatch_tool_call()` maps a tool_call to a Python call and JSON-stringifies the result. + +**Rationale**: This is the canonical OpenAI tool-loop shape. Stateless per invocation matches Q1's clarify decision. `temperature=0.0` addresses the reproducibility rationale from Q4. + +**Alternatives considered**: +- Streaming responses: rejected -- adds complexity; the final message is what the parser reads, not incremental tokens. +- Function-calling with `strict=True` (structured outputs): considered; may not be worth the SDK-version coupling for this MVP. Deferred to a follow-up if the parser routinely mis-extracts. + +## R4. Tool schema for `audit_openssf_baseline` + +**Decision**: One OpenAI tool schema, matching the MCP tool's signature: + +```json +{ + "type": "function", + "function": { + "name": "audit_openssf_baseline", + "description": "Run darnit's OpenSSF Baseline audit on the repository at local_path.", + "parameters": { + "type": "object", + "properties": { + "local_path": {"type": "string", "description": "Absolute path to the repo"}, + "level": {"type": "integer", "enum": [1, 2, 3], "default": 3}, + "output_format": {"type": "string", "enum": ["markdown", "json"], "default": "json"} + }, + "required": ["local_path"] + } + } +} +``` + +Additional tools available to Claude via MCP (`list_available_checks`, `confirm_project_data`) are OMITTED for MVP -- the skill's primary path is a single audit call. + +**Rationale**: Minimal surface -- the diagnostic value is whether the model faithfully reports what one audit call returned. Adding tools not needed for the primary journey inflates the SDK-call cost per fixture without adding parity signal. + +**Alternatives considered**: +- Register every darnit MCP tool: rejected as scope creep. +- Auto-generate schemas from the Python function signatures via `inspect`: rejected -- too much machinery for a two-line hand-authored schema. + +## R5. Turn-cap-exhausted outcome semantics + +**Decision**: The runner and differ recognize a new `SkillInvocationResult.turn_cap_exhausted: bool = False` field. When True: + +- `run.py` reports `outcome = "turn_cap_exhausted"`. +- Aggregate exit code = 5 (per Q3 clarify + FR-010). +- `diff_report.md` explicitly reads: `"FAIL: model exhausted its turn cap ({max_turns}) without emitting a final message. This is DISTINCT from unparseable output -- the assistant kept calling tools instead of summarizing."` +- The final assistant message field is empty; the raw tool-call transcript is NOT captured (privacy + noise; not needed to diagnose "model didn't converge"). + +Diff outcome ordering (most severe first): `SKILL_UNPARSEABLE` (2) > `TURN_CAP_EXHAUSTED` (5) > `PER_CONTROL_DISAGREE` (1) > `COUNTS_DISAGREE` (1) > `SUCCESS` (0). Exit-code order doesn't have to match severity -- 5 slots in alongside the existing 0-4. + +**Rationale**: A distinct outcome makes debugging obvious. Exit code 5 doesn't collide with 4 (rate-limit) so a CI grep for "exit 5" is unambiguous. + +**Alternatives considered**: +- Lump into `errored`: rejected in clarify Q3 (option D). +- Include the tool-call transcript in the diff report: rejected -- noise, potential leak surface, and not needed for the runbook. + +## R6. Workflow structure + Environment configuration + +**Decision**: `.github/workflows/parity-tier2-openai.yml` mirrors feature 028's `parity-tier2.yml`: + +- `on: workflow_dispatch:` with inputs `fixture_glob` (default `"*"`) AND `model` (default `gpt-4o-2024-08-06`). +- `environment: parity-tier2-openai` (distinct from feature 028's `parity-tier2`). +- `permissions: contents: read`. +- Preflight audit step logs actor + SHA + fixture_glob + selected model to `$GITHUB_STEP_SUMMARY` BEFORE the SDK step. +- SDK step consumes `OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }}` (Environment-scoped secret, NOT repo-level). +- `actions/upload-artifact@v4` with `if: always()` uploads `parity-artifacts/`. + +Environment configuration (in GitHub UI, not YAML): +- Required-reviewer list authorized to approve OpenAI-cost runs. +- `OPENAI_API_KEY` stored at the Environment level. +- No other secrets in this Environment (avoids blast radius if compromised). + +**Rationale**: Parallel to feature 028; two Environments give per-provider accountability. + +**Alternatives considered**: +- One workflow with a `provider` input: rejected -- one Environment would have to hold BOTH keys, or the workflow would have to switch Environments dynamically (which GitHub doesn't support cleanly). Per-provider workflows are cleaner. + +## R7. Fixture corpus and artifact path per provider + +**Decision**: Both workflows write to `parity-artifacts//`. Feature 028's Claude workflow overwrites Claude-specific files; feature 029's OpenAI workflow overwrites OpenAI-specific files. To keep them from stomping, we introduce a `openai_final_message.md` filename for the OpenAI adapter's output vs `skill_final_message.md` for Claude's (feature 028's original name). + +Concretely, `artifact_writer.py` is extended to accept a `provider` parameter that determines the filename of the "final message" artifact: + +- Claude: `skill_final_message.md` (feature 028's name preserved for backwards compat with any existing analysis scripts). +- OpenAI: `openai_final_message.md`. + +Other files (`mcp_tool_result.json`, `diff_report.md`, `metadata.json`) are provider-neutral and are overwritten on subsequent dispatches of the same provider. + +**Rationale**: Same fixture path, per-provider filename. A local script diffing across providers reads both filenames from the same directory. + +**Alternatives considered**: +- Provider-subdirectory (`parity-artifacts///`): cleaner separation but requires updating feature 028's existing artifact-shape docs and might churn analysis scripts. Deferred. +- Two entirely separate artifact roots: rejected -- inflates artifact-storage overhead in CI. + +## R8. Model-pin verification (SC-010) + +**Decision**: The workflow-config test (extending `test_workflow_config.py` from feature 028) adds an assertion: + +```python +def test_openai_workflow_pins_versioned_model(): + workflow = yaml.safe_load(open(".github/workflows/parity-tier2-openai.yml")) + inputs = _get_dispatch_inputs(workflow) + default_model = inputs["model"]["default"] + # Match either date-suffix (gpt-4o-2024-08-06) or explicit version tag (gpt-4o.1) + assert re.match(r"^[a-z0-9\-]+-\d{4}-\d{2}-\d{2}$", default_model) or \ + re.match(r"^[a-z0-9\-]+\.\d+$", default_model), \ + f"OpenAI model default {default_model!r} must be a versioned pin, not a moving alias" +``` + +**Rationale**: Two version-shape families cover OpenAI's naming (`gpt-4o-2024-08-06` for date-suffixed models, `gpt-4o.1` if OpenAI later moves to semver-style). A moving alias like `gpt-4o` fails both patterns and thus fails the test. + +**Alternatives considered**: +- A hard-coded list of known-valid strings: rejected -- would fail every time OpenAI ships a new dated model. +- Regex against a schema doc: rejected as more machinery than the invariant needs. + +## R9. Adversarial-test mocking strategy + +**Decision**: The OpenAI backend's tests mock `openai.AsyncOpenAI` at the module level using `unittest.mock.patch`. Test cases construct canned `ChatCompletion`-shaped responses. Because the SDK's response objects are Pydantic models with well-defined fields, the tests use minimal `SimpleNamespace`-style stubs rather than importing the SDK's response types. + +For SC-011 (turn-cap exhausted), the mock returns responses that always emit tool_calls, never a text message, so the loop runs for `max_turns` iterations and hits the cap. + +**Rationale**: Fine-grained control over API responses; no live network. Tests run in <1 second per case. + +**Alternatives considered**: +- `respx` or `vcrpy` for HTTP-level mocking: rejected -- more setup for equivalent outcome; SDK-level mocking is closer to what we care about. +- Use OpenAI's official test helpers (if any): unclear whether they exist; not necessary for the adversarial cases we care about. + +## R10. Interaction with feature 028's stacked-PR reality + +**Decision**: Feature 029 is stacked on feature 028 (which is stacked on feature 026). Rebase order once ancestors merge: + +1. #365 (feature 026 + Stage 1 substrate) merges to `main`. +2. #367 (feature 027, sibling to 028) merges. Order between #367 and #370 (feature 028) doesn't matter; they're independent siblings on 026. +3. #370 (feature 028) merges. +4. This feature's PR (#371, say) rebases to `main` after #370 lands. Its diff collapses to just feature 029. + +If reviews on the ancestor PRs surface changes to the Tier 2 machinery (e.g., someone requests a rename of `claude_agent_sdk_client.py`), feature 029's refactor plan absorbs those changes on rebase. + +**Rationale**: Documented for clarity. The plan phase can't predict every rebase surprise; the property that matters is that feature 029's refactor of `claude_agent_sdk_client.py` is small and reviewable in isolation. + +**Alternatives considered**: +- Wait for #370 to merge before starting 029: rejected by user instruction ("assume it'll get merged"). +- Fork 028's client without touching it: rejected -- forgoes the shared Protocol seam that's the whole point of extracting the abstraction. + +## Summary of Phase 0 outcome + +- Every technical unknown for Phase 1 has a concrete decision above. +- No new production dependencies. `openai>=1.50` is a workspace dev-group addition only. +- Governance property (SC-002) mirrors feature 028's SC-005a for the OpenAI key. +- SC-010 (pinned model) enforceable via a regex-based workflow config test. +- SC-011 (turn cap exhausted) exit code 5 slots into the existing 0-4 exit-code table with no collision. +- Refactor of feature 028's Claude client preserves import compatibility via a shim module. +- Fixture corpus, skill prompt snapshot, and `Tier2DiffReport` shape from feature 028 are reused verbatim (only `provider` filename argument added to `artifact_writer.py`). diff --git a/specs/029-openai-parity-adapter/spec.md b/specs/029-openai-parity-adapter/spec.md new file mode 100644 index 0000000..9617c12 --- /dev/null +++ b/specs/029-openai-parity-adapter/spec.md @@ -0,0 +1,139 @@ +# Feature Specification: OpenAI Tier 2 Parity Adapter + +**Feature Branch**: `029-openai-parity-adapter` + +**Created**: 2026-08-10 + +**Status**: Draft + +**Input**: User description: "Add an OpenAI SDK adapter to feature 028's Tier 2 parity check so an OpenAI-based coding-assistant invocation can be diffed against raw MCP tool output alongside the existing Claude Agent SDK path (issue #368)." + +## Clarifications + +### Session 2026-08-10 + +- Q: Which OpenAI API surface does the adapter use? -> A: OpenAI Chat Completions API with `tools=[...]` and a hand-rolled tool-call loop. Stateless per invocation -- no server-side thread/assistant lifecycle to manage; the turn cap is a plain message count. Symmetric with feature 028's Claude adapter (also stateless per-fixture) so both adapters slot cleanly into the same shared `SkillInvocationBackend` Protocol. +- Q: How are backends registered with the runner? -> A: Simple factory dict `BACKEND_REGISTRY = {"claude_agent_sdk": ..., "openai": ...}` in a shared module. Runner reads via string lookup; tests inject a `MockBackend` by monkey-patching the registry or by passing an explicit `backends=` dict. TEST-ONLY seam -- no entry-point discovery. Matches the fact that no third-party outside darnit's own test suite extends this Protocol (unlike feature 027's `QuestionResolver`, which is product-facing). +- Q: What outcome does the runner report when the turn cap is exhausted without a final message? -> A: New distinct outcome `turn_cap_exhausted` with exit code `5`. Diagnostically separate from `unparseable` (model summarized but the parser missed the format) and from `per_control_disagree` (model summarized fine, but disagreed with the tool). Maps to a different fix: adjust prompt or raise cap, not a parser fix. The set of documented exit codes is now: 0 success, 1 disagreement, 2 unparseable, 3 setup, 4 rate-limit, 5 turn-cap-exhausted. +- Q: How is the default OpenAI model chosen and updated? -> A: Pin a specific model string with a version suffix (e.g., `gpt-4o-2024-08-06`) as the default in `parity-tier2-openai.yml`. Reproducibility is load-bearing for a diagnostic: if OpenAI silently reinterprets what a moving alias means and the parity test's baseline shifts, we cannot distinguish "model changed" from "darnit tool changed." Bumping requires a PR editing the workflow -- explicit, reviewable, correlatable with any subsequent test result changes. A workflow input overrides the pinned default for one-off investigations. +- Q: Where does the `NoopBackend` used to verify SC-005 and SC-007 live? -> A: Test-only fixture at `tests/darnit/parity/tier2/backends/noop.py`. Concrete class satisfying the Protocol; imported by conformance and extensibility tests. Not shipped as a third-party template (matches Q2 -- this is a test-only seam, not a product-facing Protocol). The Protocol's shape is documented in `contracts/skill-invocation-backend-protocol.md`; anyone writing a real backend reads that contract. + +## User Scenarios & Testing *(mandatory)* + +### User Story 1 - Maintainer runs the OpenAI Tier 2 parity check (Priority: P1) + +An authorized maintainer manually dispatches an OpenAI Tier 2 workflow to see whether an OpenAI-based coding-assistant invocation (using the same skill-shaped system prompt as the Claude path) preserves the darnit tool's raw per-control status in its user-facing summary. The workflow captures raw MCP tool JSON per fixture, invokes an OpenAI model via the OpenAI SDK with the darnit MCP tools registered as function-calling tools, parses the final assistant message, and diffs the two. Failure surfaces per-control drift as an artifact. + +**Why this priority**: This is the direct closer for issue #368. Feature 028's Tier 2 covers Claude Code + Claude models. Users of darnit through OpenAI-backed coding assistants (e.g., Cursor with GPT-4, ChatGPT-in-IDE integrations, any custom OpenAI agent) deserve the same drift-detection surface. Without this, a silent WARN-to-PASS reclassification by an OpenAI-based agent would go undetected while the Claude path stays honest. + +**Independent Test**: An authorized maintainer dispatches `parity-tier2-openai.yml` with `fixture_glob="*"`. The workflow pauses at the reviewer gate, is approved, runs to completion, and either exits 0 (skill agrees with tool) or exits 1 with a per-fixture diff artifact showing which controls the OpenAI assistant reclassified. + +**Acceptance Scenarios**: + +1. **Given** the OpenAI-based invocation reports every control's status identically to the raw tool output, **When** the workflow runs, **Then** it exits 0 and the summary line shows "0 drifts, 0 unparseable." +2. **Given** the OpenAI-based invocation silently reclassifies at least one control's status, **When** the workflow runs, **Then** it exits 1 with a `diff_report.md` naming the offending controls; both raw artifacts (tool JSON + OpenAI final message) are uploaded. +3. **Given** the OpenAI-based invocation's final message cannot be parsed for per-control claims, **When** the workflow runs, **Then** it exits 2 (a distinct failure class from disagreement); the raw final message is uploaded so a maintainer can inspect the format the parser missed. +4. **Given** `OPENAI_API_KEY` is not configured on the `parity-tier2-openai` GitHub Environment, **When** an authorized maintainer approves the dispatch, **Then** the workflow fails fast with a clear setup error naming the missing key; no other steps proceed. + +--- + +### User Story 2 - Author of a future provider adapter reuses the seam (Priority: P2) + +A future maintainer wants to add a Gemini or xAI Tier 2 check. They read the shared Protocol definition in the darnit parity test suite, write a new backend adapter satisfying the Protocol, register it with the runner, and add a new workflow file. No changes to the shared parser, differ, artifact writer, runner CLI, or fixture corpus are required. + +**Why this priority**: Feature 028 shipped a per-provider Tier 2 by wiring the Claude SDK directly into `run.py`. Adding OpenAI as a second inline path would make the runner harder to extend and force each future provider to duplicate scaffolding. Extracting a `SkillInvocationBackend` Protocol during feature 029 pays down the debt now, when we have two concrete examples to compare against, before it multiplies. + +**Independent Test**: A stub `NoopBackend` implementation lives under `tests/darnit/parity/tier2/backends/` and is used by the runner to verify the seam's shape (accepts a fixture, returns a `SkillInvocationResult`, raises `SetupError` on missing env). Adding the backend requires no edits to `run.py`, `diff.py`, `artifact_writer.py`, `skill_markdown_parser.py`, or any fixture. + +**Acceptance Scenarios**: + +1. **Given** the Protocol shape is documented in `contracts/skill-invocation-backend-protocol.md`, **When** a third-party author writes a backend that implements the Protocol, **Then** they can register it with the runner (via constructor injection or a factory dict) without editing any file in `tests/darnit/parity/tier2/` other than to add their own module. +2. **Given** two registered backends (`claude_agent_sdk`, `openai`), **When** the runner is invoked with `--backend openai`, **Then** only the OpenAI backend is used; the Claude backend is not loaded. + +--- + +### User Story 3 - Aggregate provider drift comparison (Priority: P3) + +A maintainer wants to know, across a single fixture, whether the Claude-based and OpenAI-based skill invocations agree with EACH OTHER (not just with the raw tool). This surfaces provider-specific bias: if Claude reads the tool as WARN and OpenAI reads it as PASS on the same fixture, that is a signal worth capturing even if neither is technically "wrong" against the tool. + +**Why this priority**: Genuinely useful but not urgent for closing #368. The MVP of feature 029 answers "does OpenAI drift from tool?" for each fixture individually. Cross-provider drift is a follow-up analysis a maintainer can perform manually on the two workflows' artifacts. Priority 3 because automating it adds workflow complexity for a nice-to-have report. + +**Independent Test**: Given artifact bundles from both a Claude Tier 2 dispatch and an OpenAI Tier 2 dispatch on the same commit, a maintainer can run a local script that diffs the two providers' final messages for the same fixture and reports any control-level status mismatch between them. Not part of any CI job. + +**Acceptance Scenarios**: + +1. **Given** two artifact bundles for the same commit, **When** the maintainer runs the aggregate script, **Then** it produces a Markdown table listing per-control (Claude status, OpenAI status, disagreement flag). + +--- + +### Edge Cases + +- **OpenAI model + system prompt cannot invoke the darnit MCP tool directly** (OpenAI does not speak MCP protocol): the adapter MUST provide a function-calling shim that translates OpenAI tool calls to direct Python invocations of `audit_openssf_baseline` and returns the tool's JSON output back into the assistant's context. The adapter is not "invoking the actual MCP protocol" -- it's simulating a coding-assistant environment where the model can call darnit's audit function. +- **OpenAI model refuses to run the audit or returns a "safety-refused" message**: the parser produces `parseable=False`; the workflow exits 2 with the raw message in the artifact so a maintainer can inspect why. +- **OpenAI rate limit hit mid-run**: capture partial results; fail with a clear "rate limited" message per fixture; do not auto-retry. Same policy as feature 028's Tier 2. +- **`OPENAI_API_KEY` present but no `ANTHROPIC_API_KEY`**: OpenAI Tier 2 runs cleanly. It does NOT need Anthropic credentials. Similarly, Claude Tier 2 does not need OpenAI credentials. The two workflows are independent. +- **OpenAI SDK version bump changes response shape**: the adapter isolates SDK-specific access; a version bump requires updating only the OpenAI adapter, not the runner or shared parser. +- **Model-name defaulting**: OpenAI's model landscape (`gpt-4o`, `gpt-5`, etc.) evolves. The spec fixes a PINNED, VERSION-SUFFIXED model string (e.g., `gpt-4o-2024-08-06`) as the default in the workflow YAML. Reproducibility is load-bearing: if OpenAI silently reinterprets a moving alias and the parity baseline shifts, we cannot distinguish "model changed" from "darnit tool changed." Bumping the pinned default requires a PR editing the workflow. A workflow input overrides the pinned default for one-off investigations. +- **Turn cap reached without final message**: distinct outcome `turn_cap_exhausted` (exit 5). Different diagnostic than `unparseable` (exit 2) or `per_control_disagree` (exit 1). Maps to a different fix: adjust prompt / raise cap, not a parser fix. See FR-010. +- **Unauthorized dispatch attempt**: as with feature 028's Claude Tier 2, the `parity-tier2-openai` Environment is reviewer-gated. No API budget is consumed until an authorized reviewer approves. +- **OpenAI response streams "function_call" but the tool implementation returns an error**: adapter catches, injects an error response into the assistant's context, lets the model decide what to do next; the final assistant message is what the parser reads. If the model gracefully reports the error, that's a fine outcome (parser sees "audit failed to run" and Tier 2 marks the fixture as errored, not a drift). +- **Multiple assistant turns**: the adapter caps at a maximum turn count (same policy as feature 028's Claude client, default 20). A runaway conversation cannot burn arbitrary budget. + +## Requirements *(mandatory)* + +### Functional Requirements + +- **FR-001**: The system MUST provide an OpenAI-based Tier 2 parity backend that, for a given fixture, invokes an OpenAI model via the Chat Completions API with `tools=[...]` function-calling, executes tool calls in a hand-rolled loop, captures the final assistant message, and returns a `SkillInvocationResult`-shaped value analogous to feature 028's Claude backend. The backend is STATELESS per invocation: no persistent thread/assistant objects are created; the turn cap is enforced as a message count. +- **FR-002**: The backend MUST use the OpenAI Python SDK (available on PyPI). It is a TEST-ONLY dependency; no darnit product package gains a runtime dep on it. +- **FR-003**: The system MUST define a `SkillInvocationBackend` Protocol shared across all Tier 2 provider adapters. Both the existing Claude adapter (feature 028) and the new OpenAI adapter MUST conform to it. Future adapters (Gemini, xAI, self-hosted) can conform without editing the shared runner, differ, parser, or artifact writer. +- **FR-004**: The runner (`tests/darnit/parity/tier2/run.py`) MUST accept a `--backend ` CLI flag defaulting to `claude_agent_sdk` (preserves feature 028 behavior). Values `claude_agent_sdk` and `openai` are supported in this feature; unknown values fail with a clear error naming the supported set. Backend lookup uses a `BACKEND_REGISTRY` dict in a shared module (`tests/darnit/parity/tier2/backends/__init__.py`); tests inject a `MockBackend` by monkey-patching the registry or by passing an explicit `backends=` dict to the runner (no entry-point discovery, since this is a test-only Protocol seam). +- **FR-005**: The system MUST provide a separate GitHub Actions workflow file `.github/workflows/parity-tier2-openai.yml` that is manual-dispatch only, uses a distinct GitHub Environment named `parity-tier2-openai`, and reads `OPENAI_API_KEY` (not `ANTHROPIC_API_KEY`) from that Environment's secrets. The Claude Tier 2 workflow from feature 028 remains unchanged. +- **FR-006**: The `parity-tier2-openai` Environment MUST be configured with a required-reviewer list. Same governance model as feature 028's `parity-tier2` Environment. +- **FR-007**: `OPENAI_API_KEY` MUST NOT appear in any workflow file other than `parity-tier2-openai.yml`. Verifiable by iterating `.github/workflows/*.yml` and asserting the key literal appears only in the OpenAI workflow file. Mirror of feature 028's SC-005a for the Anthropic key. +- **FR-008**: When `OPENAI_API_KEY` is absent, the OpenAI backend MUST fail fast with a clear setup error naming the missing key. Silent skip is forbidden. +- **FR-009**: The OpenAI adapter MUST NOT modify the `/darnit-audit` skill's prompt snapshot committed by feature 028. It uses the SAME snapshot (adapted for OpenAI's tool-call format if the SDK requires it) so the two Tier 2 paths are testing the same coding-assistant behavior on different providers. +- **FR-010**: The OpenAI adapter MUST cap the assistant's turn count via an explicit `max_turns` value (default 20, configurable via workflow input) so a runaway conversation cannot burn arbitrary budget. When the cap is reached before the model emits a final text message, the runner MUST report the outcome as `turn_cap_exhausted` with exit code `5` -- distinct from `unparseable` (exit 2) and `per_control_disagree` (exit 1). The full documented exit-code set for the OpenAI backend is: `0` success, `1` disagreement, `2` unparseable, `3` setup, `4` rate-limit, `5` turn-cap-exhausted. +- **FR-011**: Per-control status disagreement between the OpenAI final assistant message and the raw MCP tool output is a HARD failure regardless of authority level. The OpenAI model has no license to reinterpret the tool's verdicts, same as the Claude path in feature 028. +- **FR-012**: The runner MUST support `--dry-run` for the OpenAI backend, same as for the Claude backend, so the OpenAI code path is offline-testable via a canned response. +- **FR-013**: Fixture corpus is REUSED from feature 028. No fixture changes are required by this feature. Adding fixtures happens by editing the fixtures directory; both providers automatically pick them up. +- **FR-014**: The Tier 2 skill Markdown parser is REUSED from feature 028. If the OpenAI adapter's final message format matches the parser's regex heuristics, existing patterns apply. If it does not, the failure class is "skill output unparseable" (distinct from "skill and tool disagree"), same as feature 028's FR-006a. No parser fork. +- **FR-015**: The workflow MUST perform a preflight audit log (actor + SHA + fixture_glob + selected backend) BEFORE consuming `OPENAI_API_KEY`, so post-hoc cost attribution works. Mirror of feature 028's T2-7/T2-8. +- **FR-016**: The parity test suite MUST NOT modify any darnit product package as a side effect of this feature. Enforced mechanically by the `test_no_product_changes.py` guard added in feature 028 (which already covers `packages/*/src/`). +- **FR-017**: This feature MUST close issue #368 when merged. Follow-up provider adapters (Gemini, xAI, self-hosted) are tracked as separate issues after merge; those are NOT in scope for this feature. + +### Key Entities + +- **SkillInvocationBackend**: A Protocol that any provider adapter satisfies. Exposes a stable identifier (`name: str`), an async `invoke(fixture_dir) -> SkillInvocationResult` method, and a `check_env()` classmethod (or equivalent) that fails fast when the provider's credentials are absent. The Claude Agent SDK adapter from feature 028 is refactored to satisfy this Protocol; the OpenAI adapter is a new implementation. +- **OpenAIBackend**: The concrete implementation of `SkillInvocationBackend` that wraps the OpenAI SDK. Uses the shared skill prompt snapshot as the system prompt; registers the darnit MCP tools as OpenAI function-callable tools; loops on tool calls until the assistant emits a final text message or the turn cap is reached. +- **BackendRegistry**: A module-level dict `BACKEND_REGISTRY = {"claude_agent_sdk": ..., "openai": ...}` in `tests/darnit/parity/tier2/backends/__init__.py`. Tests inject a `MockBackend` or `NoopBackend` (see below) by monkey-patching the dict or passing an explicit `backends=` dict to the runner. Not a Protocol-with-entry-points -- this is a test-only seam. +- **NoopBackend**: Test-only fixture at `tests/darnit/parity/tier2/backends/noop.py`. A concrete class satisfying the `SkillInvocationBackend` Protocol; used by conformance tests (SC-005) and extensibility tests (SC-007). NOT shipped as a "how to write a backend" template -- a real backend author reads `contracts/skill-invocation-backend-protocol.md` instead. +- **`parity-tier2-openai` Environment**: A GitHub Actions Environment with a required-reviewer list and `OPENAI_API_KEY` at the Environment level. Distinct from feature 028's `parity-tier2` Environment (which holds `ANTHROPIC_API_KEY`). + +## Success Criteria *(mandatory)* + +### Measurable Outcomes + +- **SC-001**: A single command dispatches the OpenAI Tier 2 workflow, waits for reviewer approval, runs against the fixture corpus, and produces an artifact bundle with per-fixture `diff_report.md`, `mcp_tool_result.json`, and `openai_final_message.md`. Verified end-to-end by an authorized maintainer. +- **SC-002**: `OPENAI_API_KEY` never appears in any workflow file other than `.github/workflows/parity-tier2-openai.yml`. Verified by an automated test that iterates `.github/workflows/*.yml` and asserts the count is exactly 1. +- **SC-003**: A hand-built OpenAI-style final message reclassifying a WARN control as PASS is caught by the diff, regardless of the control's authority level. Verified by an adversarial test that mocks the OpenAI backend and asserts the diff outcome is `per_control_disagree`. +- **SC-004**: Missing `OPENAI_API_KEY` causes the runner to exit with a documented setup-error exit code (`3`) in under 2 seconds. Verified by a subprocess test with the env var stripped. +- **SC-005**: The `SkillInvocationBackend` Protocol is satisfied by both the refactored Claude adapter AND the new OpenAI adapter. A third `NoopBackend` used only in tests also satisfies it. Verified by `isinstance` checks against the Protocol. +- **SC-006**: The parity test suite adds NO runtime dependency to `packages/darnit/pyproject.toml` or `packages/darnit-baseline/pyproject.toml`. The OpenAI SDK is added to the workspace dev group only. Verified by a diff of the two files' dependency lists pre- and post-feature. +- **SC-007**: Adding a new backend (e.g., a scripted `GeminiBackend`) does NOT require modifying `run.py`, `diff.py`, `skill_markdown_parser.py`, `artifact_writer.py`, or any fixture. Verified by adding a `MockGeminiBackend` in the tests and asserting registration + invocation succeed without any edit to those files. +- **SC-008**: An issue #368 status check MUST show "Closed" within one working day of this feature's PR merging. Manual verification. +- **SC-009**: The OpenAI Tier 2 workflow's execution wall-clock time per fixture is bounded by the assistant's turn cap (default 20 turns), the per-turn OpenAI latency (typically 5-30 seconds), and the fixture's audit cost. A full corpus run (4-6 fixtures) SHOULD complete in under 30 minutes. Verified by measuring one production dispatch after merge. +- **SC-010**: The default OpenAI model is a PINNED, VERSION-SUFFIXED string in `parity-tier2-openai.yml`. Verified by a workflow-config test that greps the workflow for the model input's default and asserts it matches the pattern `---
` OR another explicit versioned form. A moving alias (e.g., `gpt-4o` without suffix) fails the test. +- **SC-011**: The `turn_cap_exhausted` outcome is caught by the runner and produces exit code `5` with a documented `diff_report.md` naming the offending fixture. Verified by an adversarial test that mocks the OpenAI backend to always return a tool call (never a text message), runs the runner, and asserts the exit code + report shape. + +## Assumptions + +- Feature 028 is either merged or its branch is used as this feature's base. This feature builds on 028's Tier 2 machinery -- the shared parser, differ, artifact writer, runner CLI, and workflow shape are prerequisites. +- The OpenAI Python SDK is a reputable, publicly-available Python package on PyPI. It becomes a test-only workspace dev-group dep. If the SDK's install path is more complex (e.g., requires build-from-source), that is a plan-phase problem, not a spec-phase one. +- OpenAI's Assistants / Chat Completions API supports function-calling with the fine-grained control needed to loop over tool invocations, capture the final assistant message, and cap turn count. If a particular API surface is more amenable than another, that is a plan-phase decision. +- The `/darnit-audit` skill's system prompt snapshot from feature 028 is provider-neutral enough that OpenAI can consume it. If OpenAI's tool-call syntax requires prompt adjustments distinct from Claude's, the adapter documents those differences via a small transformation function on top of the shared snapshot -- NOT by forking the snapshot. +- Governance-wise, the OpenAI API key belongs to a specific company (same as the Anthropic key situation from feature 028). Manual-dispatch-only is preserved. Scheduled cadence is a follow-up (tracked by issue #369 for the Claude path; a sibling follow-up covers OpenAI). +- The parity test suite does not itself decide "which provider is correct." All three (Claude Tier 2, OpenAI Tier 2, raw tool) can disagree; each disagreement is a separate finding. The diagnostic value is knowing WHICH pair disagrees on WHICH control -- fixing the disagreement is a separate feature. +- Aggregate reporting across providers (US3) is out of scope for the MVP. A follow-up feature or a maintainer-run local script handles it. +- The OpenAI adapter simulates a coding-assistant environment (system prompt + tool-calling loop). It is NOT a full "MCP over OpenAI" bridge. The purpose is measuring whether an OpenAI-based coding-assistant style invocation preserves tool verdicts, not building a general-purpose MCP-to-OpenAI shim. +- The default model at snapshot time is whichever OpenAI model is the current recommended default for tool-calling agents. The exact string is captured in the workflow YAML AND is a workflow input, so a maintainer can dispatch against a specific model in an investigation without editing YAML. +- Follow-up provider adapters (Gemini, xAI, self-hosted) are explicitly out of scope. Feature 029 delivers the Protocol seam + OpenAI as the first non-Claude backend. Adding a third provider is a new feature that reuses the Protocol. diff --git a/specs/029-openai-parity-adapter/tasks.md b/specs/029-openai-parity-adapter/tasks.md new file mode 100644 index 0000000..b88a379 --- /dev/null +++ b/specs/029-openai-parity-adapter/tasks.md @@ -0,0 +1,364 @@ +--- +description: "Tasks for feature 029: OpenAI Tier 2 Parity Adapter -- second provider backend + SkillInvocationBackend Protocol seam" +--- + +# Tasks: OpenAI Tier 2 Parity Adapter + +**Input**: Design documents from `specs/029-openai-parity-adapter/` + +**Prerequisites**: plan.md (loaded), spec.md (loaded, 5 clarifications), research.md (loaded, 10 decisions), data-model.md (loaded), contracts/{skill-invocation-backend-protocol,openai-workflow}.md (loaded), quickstart.md (loaded). + +**Tests**: Test tasks included. Every FR maps to a concrete pytest module or a workflow-config assertion. Load-bearing SCs: SC-002 (OPENAI_API_KEY exclusive to one workflow), SC-003 (adversarial: skill reclassification caught), SC-005 (Protocol conformance), SC-007 (extensibility: new backend without shared-module edits), SC-010 (pinned versioned model), SC-011 (turn-cap-exhausted outcome). + +**Organization**: Tasks grouped by user story. Feature 028 (audit parity tests) is a hard dependency. The refactor of feature 028's `claude_agent_sdk_client.py` into the new Protocol-conforming layout is done under Phase 2 (foundational) so it's a shared prerequisite for both P1 (OpenAI backend) and P2 (extensibility test). + +**Branch base**: `028-audit-parity-tests` (PR #370, still open). Rebase to `main` once #370 lands. Do NOT branch from `main` directly -- feature 029 depends on 028's Tier 2 machinery. + +**Closes**: #368 on merge. + +## Format: `[ID] [P?] [Story?] Description` + +- **[P]**: Parallelizable with other [P] tasks in the same phase +- **[Story]**: Which user story (US1, US2, US3) +- File paths are exact and repository-relative + +--- + +## Phase 1: Setup (Shared Infrastructure) + +**Purpose**: Add the OpenAI SDK to the workspace dev group. Zero product-package changes (SC-006). No new subpackage; the extension lives under `tests/darnit/parity/tier2/backends/`. + +- [X] T001 Add `openai>=1.50` to the workspace-level `pyproject.toml`'s `dev` extra, alongside `claude-agent-sdk` from feature 028. MUST NOT modify `packages/darnit/pyproject.toml` or `packages/darnit-baseline/pyproject.toml`. Include a comment identifying it as a Tier 2 test-only dep. Then run `uv sync --extra dev` and confirm `openai` is importable. + +- [X] T002 [P] Create the new directory `tests/darnit/parity/tier2/backends/` with an empty `__init__.py` placeholder (will be populated in Phase 2). Ensures test collection doesn't fail when subsequent phases add files. + +**Checkpoint**: `openai` is importable; `tests/darnit/parity/tier2/backends/` exists as a Python package. + +--- + +## Phase 2: Foundational (Blocking Prerequisites) + +**Purpose**: Extract the `SkillInvocationBackend` Protocol. Refactor feature 028's Claude client into a class conforming to the Protocol. Register both existing backends in a factory dict. Preserve backwards-compat via a shim at the old import path so feature 028's existing tests continue to work unchanged. + +**CRITICAL**: No user-story tasks can proceed until this phase is complete. + +- [X] T003 Create `tests/darnit/parity/tier2/backends/base.py` per data-model.md section 1-3: + - `SetupError` (RuntimeError subclass; docstring cites B-11..B-13 from `contracts/skill-invocation-backend-protocol.md`) + - `SkillInvocationResult` frozen dataclass with `final_message: str`, `model: str`, `turn_count: int`, `metadata: dict[str, Any] = field(default_factory=dict)`, `turn_cap_exhausted: bool = False` + - `SkillInvocationBackend` Protocol -- `@runtime_checkable`, `name: str`, async `invoke(fixture_dir, model, max_turns) -> SkillInvocationResult`, classmethod `check_env() -> None` (contract QR-1..QR-3 shape) + - `__all__` tuple exporting the four names + +- [X] T004 Refactor feature 028's `tests/darnit/parity/tier2/claude_agent_sdk_client.py` into `tests/darnit/parity/tier2/backends/claude_agent_sdk.py` per data-model.md section 5: + - Rename `invoke_skill` function to a method on new class `ClaudeAgentSdkBackend` + - `name = "claude_agent_sdk"` class attribute + - `check_env()` classmethod checking `ANTHROPIC_API_KEY` (raises SetupError from `backends/base.py`) + - `async def invoke(self, fixture_dir, model, max_turns) -> SkillInvocationResult` -- body is feature 028's existing `invoke_skill` logic, unchanged behavior + - Import `SetupError`, `SkillInvocationResult` from `backends.base` + - Preserve `PROMPT_SNAPSHOT_PATH` module constant + +- [X] T005 Convert `tests/darnit/parity/tier2/claude_agent_sdk_client.py` into a backwards-compat re-export shim per data-model.md section 5. Body: + ```python + """Backwards-compat shim; superseded by + tests/darnit/parity/tier2/backends/claude_agent_sdk.py.""" + + from tests.darnit.parity.tier2.backends.base import ( + SetupError, SkillInvocationResult, + ) + from tests.darnit.parity.tier2.backends.claude_agent_sdk import ( + ClaudeAgentSdkBackend, PROMPT_SNAPSHOT_PATH, + ) + + + async def invoke_skill(fixture_dir, model="anthropic:claude-sonnet-5", max_turns=20): + """Deprecated: use ClaudeAgentSdkBackend.invoke() directly.""" + return await ClaudeAgentSdkBackend().invoke(fixture_dir, model, max_turns) + + + __all__ = ("SetupError", "SkillInvocationResult", "invoke_skill", "PROMPT_SNAPSHOT_PATH") + ``` + This preserves feature 028's tests that import from the old path. + +- [X] T006 [P] Create `tests/darnit/parity/tier2/backends/noop.py` per data-model.md section 7. `NoopBackend` class satisfying the Protocol: `name = "noop"`, `check_env()` returns None, `invoke()` returns a canned `SkillInvocationResult` with `final_message="# noop backend\n\nPassed: 0\nFailed: 0"`, `model=model`, `turn_count=0`, `metadata={"backend": "noop"}`. Docstring documents its role as a test-only fixture (NOT a template). + +- [X] T007 [P] Populate `tests/darnit/parity/tier2/backends/__init__.py` with `BACKEND_REGISTRY` dict per data-model.md section 4: + ```python + from .base import ( + SetupError, SkillInvocationBackend, SkillInvocationResult, + ) + from .claude_agent_sdk import ClaudeAgentSdkBackend + from .openai_backend import OpenAIBackend + + BACKEND_REGISTRY: dict[str, type[SkillInvocationBackend]] = { + "claude_agent_sdk": ClaudeAgentSdkBackend, + "openai": OpenAIBackend, + } + + __all__ = ( + "SetupError", "SkillInvocationBackend", "SkillInvocationResult", + "ClaudeAgentSdkBackend", "OpenAIBackend", "BACKEND_REGISTRY", + ) + ``` + NOTE: this task depends on T008's `openai_backend.py` existing; do T008 first OR add a placeholder `OpenAIBackend` stub in `openai_backend.py` before running the import. + +- [X] T008 Create `tests/darnit/parity/tier2/backends/openai_backend.py` skeleton (implementation body deferred to Phase 3 T010). Just the class shell: + ```python + from tests.darnit.parity.tier2.backends.base import ( + SetupError, SkillInvocationResult, + ) + + + class OpenAIBackend: + name = "openai" + + @classmethod + def check_env(cls) -> None: ... # T010 implements + + async def invoke(self, fixture_dir, model, max_turns): ... # T010 implements + ``` + Skeleton exists so T007 can import it; T010 fills in the body. + +- [X] T009 [P] Run feature 028's existing Tier 2 tests unchanged: `uv run pytest tests/darnit/parity/tier2/ -q`. All 21 must still pass. This is the refactor's canary: if any feature-028 test breaks, the shim (T005) or the class (T004) is wrong. Fix before proceeding. **LC4 pointer**: feature-028 tests most likely to surface a shim regression: `test_diff_adversarial.py::TestFR010MissingApiKey` (imports `SetupError` + `invoke_skill` via the old shim path); `test_workflow_config.py` (adding `parity-tier2-openai.yml` will trigger its cross-file grep -- adjust the assertion if it now counts one extra workflow file); `test_skill_markdown_parser.py` (imports `SkillReport` -- unchanged path). If a rename slips in during #370's review, expect breakage in `test_diff_adversarial.py` first. + +- [X] T009a [P] **MC3 fix**: Create `tests/darnit/parity/tier2/test_shim_exports.py` -- a defensive inventory: + - Import every public name from feature 028's ORIGINAL public surface via the shim path: `from tests.darnit.parity.tier2.claude_agent_sdk_client import SetupError, SkillInvocationResult, invoke_skill, PROMPT_SNAPSHOT_PATH`. + - Assert each imported name is not None AND matches expected type (SetupError is a class subclass of RuntimeError; SkillInvocationResult is a class; invoke_skill is callable; PROMPT_SNAPSHOT_PATH is a Path). + - If a future refactor adds a new public name to feature 028's original module, this test catches when the shim silently drops it. Belt-and-suspenders alongside T009's full regression sweep. + +**Checkpoint**: Feature 028's Tier 2 tests pass unchanged; `BACKEND_REGISTRY` contains two entries; `ClaudeAgentSdkBackend` and `NoopBackend` satisfy the Protocol via `isinstance` check. + +--- + +## Phase 3: User Story 1 -- Maintainer runs OpenAI Tier 2 parity check (P1) 🎯 MVP + +**Goal**: A maintainer dispatches `parity-tier2-openai.yml`, the workflow invokes OpenAI's Chat Completions API with the darnit audit tool registered, captures the final assistant message, and diffs it against the raw MCP tool JSON. Any per-control disagreement is a hard failure with a diff artifact. + +**Independent Test**: An authorized maintainer runs `gh workflow run parity-tier2-openai.yml --repo darnitdevorg/darnit -f fixture_glob="all_pass_repo"`. The workflow pauses at the reviewer gate. Once approved, it runs to completion; artifact bundle at `parity-artifacts/all_pass_repo/` contains `mcp_tool_result.json`, `openai_final_message.md`, `diff_report.md`, `metadata.json`. + +### Implementation for US1 + +- [X] T010 [US1] Implement `OpenAIBackend.invoke()` per data-model.md section 6 and research.md R3: + - Import `openai.AsyncOpenAI`, `openai.APIError` locally inside `invoke()` (delayed import so `check_env()` doesn't need the SDK) + - Load system prompt from `PROMPT_SNAPSHOT_PATH` (imported from feature 028's snapshot path) + - Initialize `client = openai.AsyncOpenAI()` (SDK reads `OPENAI_API_KEY` from env automatically) + - Build `messages = [{"role": "system", ...}, {"role": "user", ...}]` with the user message telling the model to audit `fixture_dir` + - Loop `for turn in range(max_turns)`: + - Call `await client.chat.completions.create(model=model, messages=messages, tools=_TOOL_SCHEMAS, tool_choice="auto", temperature=0.0)` + - If response's `msg.tool_calls`: append the assistant message + dispatch each tool call via `_dispatch_tool_call` (data-model.md section 8), append each result as a `{"role": "tool", ...}` message, continue + - If `msg.content`: return `SkillInvocationResult(final_message=msg.content, model=model, turn_count=turn+1, metadata={"backend": "openai"})` + - Fell out of loop: return `SkillInvocationResult(final_message="", model=model, turn_count=max_turns, metadata={"backend": "openai"}, turn_cap_exhausted=True)` + - Implement `check_env()` classmethod raising `SetupError("Tier 2 OpenAI backend requires OPENAI_API_KEY...")` + - Implement `_TOOL_SCHEMAS` module constant + `_dispatch_tool_call` helper per data-model.md section 8. `_dispatch_tool_call` forces `local_path=str(fixture_dir)` to prevent the model from wandering outside the fixture (contract B-17). + +- [X] T011 [US1] Update `tests/darnit/parity/tier2/run.py`: + - Add `--backend ` argument (default `"claude_agent_sdk"`, choices from `BACKEND_REGISTRY.keys()`). + - Add `--model ` argument (no default -- required in production; the workflow YAML supplies it). + - Add `--max-turns ` argument (default 20). + - Replace `_run_skill()` per data-model.md section 9: on non-dry-run, look up `BACKEND_REGISTRY[args.backend]`, call `check_env()` (raises SetupError -> exit 3), instantiate, `await backend.invoke(fixture_dir, args.model, args.max_turns)`. + - Extend exit-code aggregation: if any fixture's outcome is `turn_cap_exhausted`, exit code becomes 5. Preserve existing codes 0/1/2/3/4. + - Preserve `--dry-run` behavior; dry-run stub does NOT go through the backend registry. + +- [X] T012 [US1] Update `tests/darnit/parity/tier2/diff.py`: + - Recognize `SkillInvocationResult.turn_cap_exhausted=True` in the `diff()` function -- BEFORE the parseability check. + - When true, return `Tier2DiffReport(outcome="turn_cap_exhausted", diff_markdown=)`. + - Diff markdown text: `"# Tier 2 parity: {fixture_name}\n\nFAIL: model exhausted its turn cap ({max_turns}) without emitting a final message. This is DISTINCT from unparseable output -- the assistant kept calling tools instead of summarizing.\n\nSee `metadata.json` for the turn count. The raw tool-call transcript is NOT captured (privacy + noise; not needed to diagnose 'model didn't converge')."` + - Add `"turn_cap_exhausted"` to the `Tier2Outcome` IntEnum (value `5`). Update the outcome-to-exit-code mapping in `run.py` accordingly. + +- [X] T013 [US1] Update `tests/darnit/parity/tier2/artifact_writer.py` per data-model.md section 10: + - Add optional `provider: str = "claude"` parameter. + - Compute final-message filename: `"skill_final_message.md"` when `provider == "claude"`, else `f"{provider}_final_message.md"`. + - No other behavior change. + +- [X] T014 [US1] Update `run.py`'s artifact-writing call site to pass `provider=args.backend` (or a mapped value; `claude_agent_sdk` maps to `"claude"` filename convention; `openai` maps to `"openai"`). The mapping lives in `run.py`; a helper `_provider_filename_prefix(backend_name)` returns the string used for the final-message filename. + +- [X] T015 [US1] Create `.github/workflows/parity-tier2-openai.yml` per contract `openai-workflow.md` (OW-1..OW-17): + - `on: workflow_dispatch:` with inputs `fixture_glob` (default `"*"`) AND `model` (default `gpt-4o-2024-08-06`). + - Job runs on `ubuntu-latest` with `environment: parity-tier2-openai` and `permissions: contents: read`. + - Steps: checkout, setup-python, uv sync --extra dev, preflight-log (actor + SHA + timestamp + fixture_glob + model to `$GITHUB_STEP_SUMMARY`), run `uv run python -m tests.darnit.parity.tier2.run --backend openai --fixture-glob "${{ inputs.fixture_glob }}" --model "${{ inputs.model }}"` with `OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }}` env, upload `parity-artifacts/` via `actions/upload-artifact@v4` with `if: always()`. + - Add YAML comments documenting each OW-* rule the line satisfies. + +### Tests for US1 + +- [X] T016 [P] [US1] Create `tests/darnit/parity/tier2/test_openai_backend_adversarial.py` covering SC-003, SC-011, FR-011, FR-014: + - `test_skill_reclassification_caught` (SC-003): mock `openai.AsyncOpenAI` to return a canned response where the assistant's final message reports a control as PASS while the raw tool output reports WARN. Run `run.py` (or `diff()` directly) and assert exit code / outcome is `per_control_disagree`. + - `test_suggestive_authority_no_license_to_reinterpret` (FR-011): same setup but the control's authority is `suggestive`. Assert diff STILL returns `per_control_disagree`. + - `test_turn_cap_exhausted` (SC-011): mock the SDK to return a response emitting a tool_call on EVERY turn (never a text message). With `max_turns=3`, assert the backend returns `SkillInvocationResult(turn_cap_exhausted=True, ...)`; running through `run.py` produces exit code 5. + - `test_local_path_forced_to_fixture_dir` (B-17): mock the SDK to return a tool_call whose arguments include `local_path="/malicious/path"`. Assert `_dispatch_tool_call` overrides with the fixture_dir and does NOT invoke `audit_openssf_baseline` on the malicious path. + - **`test_openai_style_markdown_is_parseable_by_shared_parser` (MC2 fix, FR-14)**: feed feature 028's `SkillReport.parse()` a canned Markdown response shaped like an OpenAI final message (e.g., "**OSPS-DO-01.01**: PASS\nSummary: 2 passed, 0 failed" or the shape a real GPT-4o response would emit for the skill's prompt). Assert `parseable == True`, `counts` and `controls` are extracted. Guards against a silent parser fork -- if OpenAI's Markdown format systematically requires different regexes, this test surfaces it as a distinct failure class ("parser needs OpenAI-shape support") rather than a runtime "unparseable" failure per fixture. + +- [X] T017 [P] [US1] Add `test_openai_missing_key_fails_fast` to `test_openai_backend_adversarial.py`: + - With `monkeypatch.delenv("OPENAI_API_KEY", raising=False)`, call `OpenAIBackend.check_env()` and assert `SetupError` with substring "OPENAI_API_KEY". + - Subprocess test: run `python -m tests.darnit.parity.tier2.run --backend openai --fixture-glob "all_pass_repo"` with env stripped; assert exit code 3. + +- [X] T018 [P] [US1] Extend `tests/darnit/parity/tier2/test_workflow_config.py` to cover the OpenAI workflow (contract OW-1..OW-15 + SC-002 + SC-010): + - `test_openai_only_workflow_dispatch_trigger`: parse `parity-tier2-openai.yml`; assert only `workflow_dispatch` trigger. + - `test_openai_environment_declared`: assert `environment: parity-tier2-openai`. + - `test_openai_permissions_read_only`: assert `permissions.contents: read`; no write scopes. + - `test_openai_key_only_in_openai_workflow` (SC-002): iterate `.github/workflows/*.yml`; assert `OPENAI_API_KEY` appears in `parity-tier2-openai.yml` only. Pure Python file iteration (no `grep`), same pattern as feature 028's SC-005a check. + - `test_no_anthropic_key_in_openai_workflow` (OW-9): assert `ANTHROPIC_API_KEY` does NOT appear in `parity-tier2-openai.yml`. + - `test_no_openai_key_in_claude_workflow` (OW-8): assert `OPENAI_API_KEY` does NOT appear in `parity-tier2.yml`. + - `test_openai_workflow_pins_versioned_model` (SC-010): parse the workflow's `model` input default; assert it matches `^[a-z0-9\-]+-\d{4}-\d{2}-\d{2}$` OR another explicit versioned pattern. `gpt-4o` alone (moving alias) fails. + - `test_openai_artifact_upload_always` (OW-14): assert `actions/upload-artifact` step has `if: always()`. + +**Checkpoint**: The OpenAI workflow is present, its config tests pass, adversarial tests pass. Manual dry-run: `uv run python -m tests.darnit.parity.tier2.run --backend openai --fixture-glob "all_pass_repo" --dry-run` writes artifacts to `parity-artifacts/`. US1 is independently shippable if we stop here. + +--- + +## Phase 4: User Story 2 -- Extensibility for future providers (P2) + +**Goal**: A future maintainer adds a Gemini/xAI/self-hosted backend by writing one file + one workflow YAML, no edits to shared modules. Mechanically enforceable. + +**Independent Test**: SC-005 (Protocol conformance) + SC-007 (extensibility). Run `test_backend_protocol_conformance.py`; every registered backend + the `NoopBackend` passes `isinstance(x, SkillInvocationBackend)`. Run `test_backend_extensibility.py`; registering a `NoopBackend` (or a temporary in-test mock backend) via the runner's `backends=` override invokes it without touching any shared file. + +### Tests for US2 + +- [X] T019 [P] [US2] Create `tests/darnit/parity/tier2/test_backend_protocol_conformance.py` covering SC-005: + - For each entry in `BACKEND_REGISTRY`: `assert isinstance(BackendClass(), SkillInvocationBackend)`. + - Also verify `NoopBackend` satisfies the Protocol (though it's not in `BACKEND_REGISTRY`). + - Assert every backend has a non-empty `name: str` attribute. + - Assert every backend has a `check_env` classmethod (via `inspect.ismethod` on the class). + +- [X] T020 [P] [US2] Create `tests/darnit/parity/tier2/test_backend_extensibility.py` covering SC-007: + - Import `NoopBackend` from `backends/noop.py`. + - Instantiate the runner (or call `_main_async` with a mocked args + injected `backends={"noop": NoopBackend}` dict). + - Assert the runner invokes `NoopBackend.invoke()` and writes artifacts. + - Second test: build an ad-hoc `_InlineExtBackend` class inside the test that satisfies the Protocol; inject it via `backends={"inline": _InlineExtBackend}`; assert the runner picks it up. + - Meta-assertion: with `git diff --name-only ...HEAD`, verify no file under `tests/darnit/parity/tier2/` OTHER than the two new test files was modified when adding the ad-hoc backend. Skipped when no base ref reachable (local dev). Guards SC-007's "no shared-module edits" property mechanically. + +- [X] T021 [US2] Update the `run.py` module to accept a `backends: dict[str, type[SkillInvocationBackend]] | None = None` parameter on `_main_async` and `main`; if provided, use it instead of `BACKEND_REGISTRY`. This enables T020's test-side backend injection without monkey-patching module globals. Preserve the CLI path -- `backends=None` uses the module-level registry. + +**Checkpoint**: A future author writing a new backend can add ONE file (`backends/my_provider.py`), ONE line to `BACKEND_REGISTRY`, and ONE workflow YAML file -- no other edits. Verified by T020. + +--- + +## Phase 5: User Story 3 -- Aggregate provider drift comparison (P3, deferred) + +**Goal**: Compare Claude and OpenAI final messages for the same fixture, surfacing provider-specific bias. + +**Priority 3 = out of MVP scope.** The spec's US3 says "genuinely useful but not urgent." Fixture-level cross-provider comparison is a maintainer-run local script; not automated in CI. + +### Implementation for US3 (optional, may slip) + +- [ ] T022 [P] [US3] Create `tests/darnit/parity/tier2/scripts/aggregate_provider_diff.py` (local maintainer script, NOT invoked by any pytest). Reads two artifact bundles (`parity-artifacts-claude/` + `parity-artifacts-openai/`) OR one `parity-artifacts/` directory containing both providers' final messages; for each fixture, parses both providers' summaries; emits a Markdown table `| control_id | claude_status | openai_status | disagreement |`. + - Not a runnable CI job; documented in `quickstart.md`. + - Skipped if T022 slips -- US3 is P3. + +**Checkpoint**: MVP for feature 029 is Phase 1+2+3+4. Phase 5 is nice-to-have; if it slips, feature 029 ships without it. + +--- + +## Phase 6: Polish & Cross-Cutting + +- [X] T023 [P] Update `CLAUDE.md`'s "Recent Changes" section with a one-paragraph 029 entry describing the OpenAI Tier 2 backend + Protocol seam + governance parity, closes #368. + +- [X] T024 [P] Run `uv run ruff check tests/darnit/parity/` and `uv run ruff format --check tests/darnit/parity/`. Fix any lint issues. + +- [X] T024a [P] **MC1 fix (FR-013 fixture-diff sanity)**: Run `git diff --name-only ...HEAD -- tests/darnit/parity/fixtures/` and confirm the output is empty. FR-013 says the fixture corpus is reused from feature 028 unchanged; if this PR modifies a fixture, that's out of scope for feature 029. Manual pre-PR check (not automated as a test because "shouldn't add a fixture" is a soft constraint, not a load-bearing invariant). If a fixture change is genuinely needed, split it into a separate PR. + +- [X] T025 [P] Run `uv run python scripts/validate_sync.py --verbose`. Feature 029 doesn't touch product code but keep the check honest. + +- [X] T026 [P] Full test sweep: `uv run pytest tests/ -q`. Expected: feature-028 baseline (2589 passed) + Phase-3+4 new tests, all pass, no regressions. + +- [X] T027 [P] Local dry-run smoke: `uv run python -m tests.darnit.parity.tier2.run --backend openai --fixture-glob "all_pass_repo" --dry-run --artifact-dir /tmp/parity-openai-dryrun`. Verify artifact layout matches data-model.md section 10 + OW-15. + +- [X] T028 [P] Manual grep sanity for SC-002: `grep -r "OPENAI_API_KEY" .github/workflows/` returns matches ONLY in `parity-tier2-openai.yml`. Complements T018's automated test. + +- [ ] T029 Write the PR description. Structure per project convention: no Co-Authored-By: Claude trailer, no Generated with Claude Code footer. Include a summary, the two-Environment governance rationale, test plan, links to spec/plan/contracts, cross-links to #368 (close) + #369 (related, for scheduled cadence). + +--- + +## Before-merge maintainer actions (LC3) + +These are NOT tasks in the code sense -- they are manual GitHub UI steps a maintainer MUST perform before dispatching Tier 2 OpenAI in production. Automated tests can verify the workflow YAML shape but cannot verify the Environment's UI-side configuration. + +- [ ] **M1**: Create GitHub Environment `parity-tier2-openai` under Settings -> Environments in `darnitdevorg/darnit`. +- [ ] **M2**: Configure the Environment with a required-reviewer list (authorized maintainers only). +- [ ] **M3**: Add secret `OPENAI_API_KEY` at the ENVIRONMENT level (NOT repo level). Confirm the secret does not appear under Settings -> Secrets and variables -> Actions at the repository level. +- [ ] **M4**: Verify no other GitHub Environment on the repo also holds `OPENAI_API_KEY` (blast-radius minimization, per OW-8). + +Feature 028's `parity-tier2` Environment (for `ANTHROPIC_API_KEY`) is a prerequisite and is documented in feature 028's quickstart -- both Environments coexist independently. + +--- + +## Rebase conflict watch list (MC4) + +Feature 029 stacks on feature 028's still-open PR (#370). If reviews on #370 change the Tier 2 machinery, feature 029's rebase will conflict specifically in these files. Order matters: resolve top-down; downstream files depend on upstream shape: + +1. **`tests/darnit/parity/tier2/backends/claude_agent_sdk.py`** (new -- feature 029). If #370 rename anything the shim re-exports, update `backends/claude_agent_sdk.py` first. +2. **`tests/darnit/parity/tier2/claude_agent_sdk_client.py`** (converted to shim by feature 029). If #370 adds a new public export to the original module, add a passthrough to the shim. +3. **`tests/darnit/parity/tier2/run.py`** (extended by feature 029). Any #370 change to argument parsing or the outcome-to-exit-code map has to be re-integrated with feature 029's `--backend`, `--model`, `--max-turns`, and exit 5 additions. +4. **`tests/darnit/parity/tier2/diff.py`** (extended by feature 029). Any #370 change to `Tier2Outcome` or `Tier2DiffReport` shape has to be re-integrated with feature 029's `turn_cap_exhausted` outcome. +5. **`tests/darnit/parity/tier2/artifact_writer.py`** (extended by feature 029). Any #370 change to the artifact-shape has to be re-integrated with feature 029's provider parameter. +6. **`.github/workflows/parity-tier2-openai.yml`** (new -- feature 029). Not conflict-prone (new file), but re-verify the workflow-config test in `test_workflow_config.py` still parses the OpenAI workflow after any #370 shape changes to the Claude workflow. + +Post-rebase: run `uv run pytest tests/darnit/parity/tier2/ -q` and confirm ALL tests pass. If feature 028's test file names changed during #370's review, T009's "regression check" invocation must be updated. + +--- + +## Dependencies & Story Completion Order + +``` +Phase 1 (T001-T002) --setup + openai SDK dep-- + | + v +Phase 2 (T003-T009) --foundational: Protocol + refactor + registry + shim + regression check-- + | + +-----+---------------------+ + v v v + Phase 3 (T010-T018) Phase 4 (T019-T021) + US1 -- MVP OpenAI US2 -- extensibility tests + | | + +-----+----------------+ + v + Phase 5 (T022) --US3 aggregate script (optional; deferred if it slips)-- + | + v + Phase 6 (T023-T029) --polish-- +``` + +- **Phase 1**: T001 first (openai SDK), T002 [P] parallel. +- **Phase 2**: T003 first (Protocol). T004 depends on T003. T005 depends on T004. T006, T008 depend on T003 -- and **T008 MUST run before T007** (T007 imports `OpenAIBackend` from `openai_backend.py`; T008 creates the skeleton class so the import resolves). T007 also depends on T004. T009 and T009a are regression checks that run after T003-T008. T004/T005 are file-based coordinations on the Claude client; must go in that order. +- **Phase 3**: T010 first (OpenAIBackend body). T011 depends on T010 + T007. T012, T013, T014 depend on T012/T013. T015 depends on T011 (references `run.py`). T016-T018 are [P] tests after their subjects exist. +- **Phase 4**: T019, T020 are [P] tests. T021 (`run.py` accept backends= param) is required for T020's injection test. +- **Phase 5**: T022 [P], optional. +- **Phase 6**: T023-T028 [P]. T026 (full sweep) depends on ALL previous. T029 last. + +## Parallel Execution Examples + +Once Phase 2 is done, run Phase 3+4 tests in parallel: + +```bash +uv run pytest tests/darnit/parity/tier2/test_openai_backend_adversarial.py \ + tests/darnit/parity/tier2/test_workflow_config.py \ + tests/darnit/parity/tier2/test_backend_protocol_conformance.py \ + tests/darnit/parity/tier2/test_backend_extensibility.py \ + -q -n auto +``` + +## Implementation Strategy + +**MVP-first order**: Phase 1 -> Phase 2 -> Phase 3 (US1 delivers issue #368's close). Phase 4 (extensibility guarantees) is P2 -- shipped in the same PR because it's small and the tests need Phase 2's Protocol. + +**Two-PR option**: Not recommended for feature 029. The refactor + Protocol + OpenAI adapter form a coherent unit; splitting them creates a middle state where the Protocol exists but no non-Claude backend does. Not worth the review-surface fragmentation. + +**Time boxing**: Phase 2 (refactor + Protocol) is the delicate part; Phase 3 (OpenAI adapter) is straightforward once the Protocol is locked. Total estimated size: ~500-700 lines net production + ~400-500 lines tests. Smaller than feature 028 because we're extending existing scaffolding. + +## Test coverage matrix + +| Success Criterion / FR | Test task(s) | +|---|---| +| SC-001 (workflow produces artifact bundle end to end) | Manual verification (T027 dry-run + one production dispatch after merge) | +| SC-002 (OPENAI_API_KEY exclusive to one workflow) | T018 (workflow config test) + T028 (manual grep sanity) | +| SC-003 (skill reclassification caught, any authority) | T016 (adversarial: PASS-over-WARN with suggestive + dispositive) | +| SC-004 (missing key fail-fast) | T017 (unit + subprocess exit code 3) | +| SC-005 (Protocol conformance across all backends) | T019 (registry-iteration isinstance) | +| SC-006 (no product deps added) | T001 (dev-group only) + feature 028's `test_no_product_changes.py` guard | +| SC-007 (extensibility -- no shared-module edits) | T020 (backend injection + git-diff meta-assertion) | +| SC-008 (issue #368 closed) | T029 (PR desc with `Fixes #368`) | +| SC-009 (30-min corpus wall clock) | Manual verification after first production dispatch | +| SC-010 (pinned versioned model default) | T018 (regex against workflow YAML input default) | +| SC-011 (turn_cap_exhausted outcome) | T016 (mocked infinite-tool-call response; asserts exit code 5) | +| FR-001 (Chat Completions loop) | T010 (implementation) + T016 (adversarial exercises the loop) | +| FR-004 (`--backend` CLI + registry dispatch) | T011 (impl) + T020 (extensibility test proves runtime lookup works) | +| FR-010 (turn cap + exit code 5) | T016 (SC-011 above) | +| FR-013 (fixture corpus reused, no changes) | T024a (manual pre-PR fixture-diff check, MC1 fix) + feature 028's `test_no_product_changes.py` guard | +| FR-014 (parser reused, no fork) | T016 (MC2 fix: `test_openai_style_markdown_is_parseable_by_shared_parser`) | diff --git a/tests/darnit/parity/tier2/artifact_writer.py b/tests/darnit/parity/tier2/artifact_writer.py index ab8c1d7..05ef2fd 100644 --- a/tests/darnit/parity/tier2/artifact_writer.py +++ b/tests/darnit/parity/tier2/artifact_writer.py @@ -19,15 +19,24 @@ def write_fixture_artifacts( skill_markdown: str, diff_md: str, metadata: dict[str, object] | None = None, + provider: str = "claude", ) -> Path: """Write the four per-fixture artifact files. Returns the fixture's artifact directory. + + Feature 029 addition: `provider` argument controls the filename of + the final-message artifact. `"claude"` preserves feature 028's + `skill_final_message.md` for backwards compat; any other value writes + `_final_message.md` (e.g., `openai_final_message.md`). Both + providers' artifacts can coexist under the same fixture directory + across separate dispatches. """ fixture_dir = artifact_root / fixture_name fixture_dir.mkdir(parents=True, exist_ok=True) (fixture_dir / "mcp_tool_result.json").write_text(mcp_json) - (fixture_dir / "skill_final_message.md").write_text(skill_markdown) + final_message_name = "skill_final_message.md" if provider == "claude" else f"{provider}_final_message.md" + (fixture_dir / final_message_name).write_text(skill_markdown) (fixture_dir / "diff_report.md").write_text(diff_md) meta_out: dict[str, object] = { diff --git a/tests/darnit/parity/tier2/backends/__init__.py b/tests/darnit/parity/tier2/backends/__init__.py new file mode 100644 index 0000000..21a6d36 --- /dev/null +++ b/tests/darnit/parity/tier2/backends/__init__.py @@ -0,0 +1,35 @@ +"""Tier 2 backend registry (feature 029 T007). + +BACKEND_REGISTRY is the source of truth for `--backend ` lookup. +Adding a new backend = one line here + a new module file. See +`contracts/skill-invocation-backend-protocol.md` for the Protocol shape. +""" + +from __future__ import annotations + +from tests.darnit.parity.tier2.backends.base import ( + SetupError, + SkillInvocationBackend, + SkillInvocationResult, +) +from tests.darnit.parity.tier2.backends.claude_agent_sdk import ( + ClaudeAgentSdkBackend, +) +from tests.darnit.parity.tier2.backends.openai_backend import OpenAIBackend + +BACKEND_REGISTRY: dict[str, type[SkillInvocationBackend]] = { + "claude_agent_sdk": ClaudeAgentSdkBackend, + "openai": OpenAIBackend, + # NoopBackend intentionally not registered here -- tests inject it via + # `run(backends={"noop": NoopBackend})`. +} + + +__all__ = ( + "SetupError", + "SkillInvocationBackend", + "SkillInvocationResult", + "ClaudeAgentSdkBackend", + "OpenAIBackend", + "BACKEND_REGISTRY", +) diff --git a/tests/darnit/parity/tier2/backends/base.py b/tests/darnit/parity/tier2/backends/base.py new file mode 100644 index 0000000..03f3392 --- /dev/null +++ b/tests/darnit/parity/tier2/backends/base.py @@ -0,0 +1,76 @@ +"""SkillInvocationBackend Protocol + shared types (feature 029 T003). + +Defines the Protocol every Tier 2 provider adapter conforms to. See: + - specs/029-openai-parity-adapter/data-model.md sections 1-3 + - specs/029-openai-parity-adapter/contracts/skill-invocation-backend-protocol.md +""" + +from __future__ import annotations + +from dataclasses import dataclass, field +from pathlib import Path +from typing import Any, Protocol, runtime_checkable + + +class SetupError(RuntimeError): + """Raised by a backend's `check_env()` when the provider's credentials + or environment are not configured. The runner catches this and returns + exit code 3 (setup). Contract B-11..B-13. + """ + + +@dataclass(frozen=True) +class SkillInvocationResult: + """Return type for `SkillInvocationBackend.invoke()`. + + Fields: + final_message: the string the parser will consume. Empty only when + `turn_cap_exhausted=True`. + model: the exact model identifier the backend used (e.g. the pinned + version-suffixed string from the workflow YAML). + turn_count: number of assistant turns actually taken. Non-negative. + metadata: provider-specific extras (backend name, usage stats, etc.). + turn_cap_exhausted: True iff the model exhausted its turn cap without + emitting a final text message. Feature 029 addition -- default False + so feature 028's Claude adapter can construct results without + setting it. See spec.md FR-010 and SC-011. + """ + + final_message: str + model: str + turn_count: int + metadata: dict[str, Any] = field(default_factory=dict) + turn_cap_exhausted: bool = False + + +@runtime_checkable +class SkillInvocationBackend(Protocol): + """Protocol every Tier 2 provider adapter conforms to. + + See `contracts/skill-invocation-backend-protocol.md` rules B-1..B-23. + + - `name`: stable identifier used as the `--backend` CLI value and the + `BACKEND_REGISTRY` key. + - `invoke()`: async; caller (runner) supplies `model` and `max_turns`. + - `check_env()`: classmethod so the runner can fail fast on missing + credentials without constructing an instance. + """ + + name: str + + async def invoke( + self, + fixture_dir: Path, + model: str, + max_turns: int, + ) -> SkillInvocationResult: ... + + @classmethod + def check_env(cls) -> None: ... + + +__all__ = ( + "SetupError", + "SkillInvocationBackend", + "SkillInvocationResult", +) diff --git a/tests/darnit/parity/tier2/backends/claude_agent_sdk.py b/tests/darnit/parity/tier2/backends/claude_agent_sdk.py new file mode 100644 index 0000000..0ba08c0 --- /dev/null +++ b/tests/darnit/parity/tier2/backends/claude_agent_sdk.py @@ -0,0 +1,134 @@ +"""Claude Agent SDK backend (feature 029 T004). + +Refactored from feature 028's `claude_agent_sdk_client.py::invoke_skill` +into a class satisfying the `SkillInvocationBackend` Protocol. Body is the +feature 028 logic unchanged; only the shape (class vs free function) is +different. + +The old import path (`tests.darnit.parity.tier2.claude_agent_sdk_client`) +continues to work via a shim module that re-exports names from here. +""" + +from __future__ import annotations + +import os +from pathlib import Path + +from tests.darnit.parity.tier2.backends.base import ( + SetupError, + SkillInvocationResult, +) + +# The skill prompt snapshot lives ONE level up from this module (in +# tests/darnit/parity/tier2/, alongside claude_agent_sdk_client.py). +PROMPT_SNAPSHOT_PATH = Path(__file__).parent.parent / "skill_prompt_snapshot.md" + + +def _load_skill_prompt() -> str: + if not PROMPT_SNAPSHOT_PATH.exists(): + raise SetupError( + f"skill prompt snapshot missing: {PROMPT_SNAPSHOT_PATH}. Run T022 to capture it before invoking Tier 2.", + ) + return PROMPT_SNAPSHOT_PATH.read_text() + + +class ClaudeAgentSdkBackend: + """Feature 028's Claude Agent SDK client, wrapped as a Protocol-conforming + class.""" + + name = "claude_agent_sdk" + + @classmethod + def check_env(cls) -> None: + """FR-008: fail fast when ANTHROPIC_API_KEY is absent.""" + if not os.environ.get("ANTHROPIC_API_KEY"): + raise SetupError( + "Tier 2 requires ANTHROPIC_API_KEY. Configure the " + "`parity-tier2` GitHub Actions Environment (or export the " + "var for a local run).", + ) + + async def invoke( + self, + fixture_dir: Path, + model: str, + max_turns: int, + ) -> SkillInvocationResult: + """Invoke the /darnit-audit skill against `fixture_dir`. + + Returns a `SkillInvocationResult`. The caller has already validated + env via `check_env()`; this method assumes the SDK is importable + and ANTHROPIC_API_KEY is set. + """ + skill_prompt = _load_skill_prompt() + + # Import here so unit tests exercising the SetupError path don't + # require the SDK to be importable. + from claude_agent_sdk import ( + AssistantMessage, + ClaudeAgentOptions, + ResultMessage, + TextBlock, + query, + ) + + user_prompt = ( + f"{skill_prompt}\n\n" + f"Run the audit against the repository at {fixture_dir}. " + f"Summarize the results per the skill's usual format." + ) + + # PR #370 review fix: wire the darnit MCP server, allow the audit + # tool, and lock the agent down so it can't reach for out-of-band + # settings. Previously ClaudeAgentOptions passed only model, cwd, + # and max_turns -- the agent had NO way to call + # audit_openssf_baseline and the parity test measured "skill does + # nothing" instead of "skill vs tool". + options = ClaudeAgentOptions( + # SDK expects a bare model name. + model=model.replace("anthropic:", ""), + max_turns=max_turns, + cwd=str(fixture_dir), + mcp_servers={ + "darnit": { + "type": "stdio", + "command": "darnit", + "args": ["serve", "--framework", "openssf-baseline"], + }, + }, + # Restrict to the specific MCP tool the parity test needs. + allowed_tools=["mcp__darnit__audit_openssf_baseline"], + # Auto-accept the tool call; parity CI is non-interactive. + permission_mode="acceptEdits", + # Isolate from the running host's Claude Code settings. + setting_sources=[], + ) + + final_text: str = "" + turn_count = 0 + result_meta: dict[str, object] = {} + + async for message in query(prompt=user_prompt, options=options): + if isinstance(message, AssistantMessage): + turn_count += 1 + for block in message.content: + if isinstance(block, TextBlock): + final_text = block.text # keep only the LAST turn's text + elif isinstance(message, ResultMessage): + result_meta = { + "duration_ms": getattr(message, "duration_ms", None), + "num_turns": getattr(message, "num_turns", turn_count), + } + + return SkillInvocationResult( + final_message=final_text, + model=model, + turn_count=turn_count, + metadata=result_meta, + ) + + +__all__ = ( + "ClaudeAgentSdkBackend", + "PROMPT_SNAPSHOT_PATH", +) diff --git a/tests/darnit/parity/tier2/backends/noop.py b/tests/darnit/parity/tier2/backends/noop.py new file mode 100644 index 0000000..65a3abf --- /dev/null +++ b/tests/darnit/parity/tier2/backends/noop.py @@ -0,0 +1,42 @@ +"""Test-only NoopBackend (feature 029 T006). + +Used by SC-005 (Protocol conformance) and SC-007 (extensibility). NOT +registered in `BACKEND_REGISTRY` by default; tests inject it explicitly +via `run(backends={"noop": NoopBackend})`. + +NOT shipped as a "how to write a backend" template -- a real backend +author reads `contracts/skill-invocation-backend-protocol.md` instead. +""" + +from __future__ import annotations + +from pathlib import Path + +from tests.darnit.parity.tier2.backends.base import SkillInvocationResult + + +class NoopBackend: + """No-op backend returning a canned Markdown summary. Zero API calls.""" + + name = "noop" + + @classmethod + def check_env(cls) -> None: + # No credentials required. + return None + + async def invoke( + self, + fixture_dir: Path, + model: str, + max_turns: int, + ) -> SkillInvocationResult: + return SkillInvocationResult( + final_message="# noop backend\n\nPassed: 0\nFailed: 0", + model=model, + turn_count=0, + metadata={"backend": "noop", "fixture_dir": str(fixture_dir)}, + ) + + +__all__ = ("NoopBackend",) diff --git a/tests/darnit/parity/tier2/backends/openai_backend.py b/tests/darnit/parity/tier2/backends/openai_backend.py new file mode 100644 index 0000000..8eded38 --- /dev/null +++ b/tests/darnit/parity/tier2/backends/openai_backend.py @@ -0,0 +1,223 @@ +"""OpenAI Tier 2 backend (feature 029 T008 skeleton + T010 body). + +Chat Completions API with a hand-rolled tool-call loop. Stateless per +invocation; the darnit MCP audit tool is registered as a function-callable +tool; the model is called with `temperature=0.0` for reproducibility. + +See: + - specs/029-openai-parity-adapter/data-model.md sections 6, 8 + - specs/029-openai-parity-adapter/research.md R3, R4 + - specs/029-openai-parity-adapter/contracts/skill-invocation-backend-protocol.md +""" + +from __future__ import annotations + +import json +import os +from pathlib import Path +from typing import Any + +from tests.darnit.parity.tier2.backends.base import ( + SetupError, + SkillInvocationResult, +) +from tests.darnit.parity.tier2.backends.claude_agent_sdk import ( + PROMPT_SNAPSHOT_PATH, +) + +# Data-model.md section 8 -- one tool schema for the audit function. +_TOOL_SCHEMAS: list[dict[str, Any]] = [ + { + "type": "function", + "function": { + "name": "audit_openssf_baseline", + "description": ( + "Run darnit's OpenSSF Baseline audit on the repository at the " + "given local_path. Returns a JSON string with per-control " + "results including status, authority, and level." + ), + "parameters": { + "type": "object", + "properties": { + "local_path": { + "type": "string", + "description": ("Absolute path to the repository being audited."), + }, + "level": { + "type": "integer", + "enum": [1, 2, 3], + "default": 3, + }, + "output_format": { + "type": "string", + "enum": ["markdown", "json"], + "default": "json", + }, + }, + "required": ["local_path"], + }, + }, + }, +] + + +def _dispatch_tool_call(call: Any, fixture_dir: Path) -> str: + """Execute a tool call from the OpenAI response. + + Contract B-17: `local_path` is forced to `fixture_dir` even if the + model's tool-call arguments named a different path -- prevents a + rogue model from wandering outside the fixture. + """ + if call.function.name == "audit_openssf_baseline": + try: + args = json.loads(call.function.arguments or "{}") + except json.JSONDecodeError: + args = {} + # Force local_path to the fixture dir. + args["local_path"] = str(fixture_dir) + args.setdefault("output_format", "json") + args.setdefault("level", 3) + # Force safe defaults. + args["auto_init_config"] = False + args["attest"] = False + args["prefer_upstream"] = False + + # Local import so unit tests exercising the SetupError path don't + # require darnit-baseline to be importable. + from darnit_baseline.tools import audit_openssf_baseline + + return audit_openssf_baseline(**args) + return json.dumps({"error": f"unknown tool: {call.function.name}"}) + + +def _load_skill_prompt() -> str: + if not PROMPT_SNAPSHOT_PATH.exists(): + raise SetupError( + f"skill prompt snapshot missing: {PROMPT_SNAPSHOT_PATH}. " + "Run T022 (feature 028) to capture it before invoking Tier 2.", + ) + return PROMPT_SNAPSHOT_PATH.read_text() + + +class OpenAIBackend: + """OpenAI Chat Completions API backend for Tier 2 parity checks.""" + + name = "openai" + + @classmethod + def check_env(cls) -> None: + """FR-008: fail fast when OPENAI_API_KEY is absent.""" + if not os.environ.get("OPENAI_API_KEY"): + raise SetupError( + "Tier 2 OpenAI backend requires OPENAI_API_KEY. Configure " + "the `parity-tier2-openai` GitHub Actions Environment (or " + "export the var for a local run).", + ) + + async def invoke( + self, + fixture_dir: Path, + model: str, + max_turns: int, + ) -> SkillInvocationResult: + """Invoke an OpenAI-based skill against `fixture_dir`. + + Implements research.md R3's Chat Completions loop. On completion: + - Final text message received -> `turn_cap_exhausted=False`. + - Turn cap reached without text -> `turn_cap_exhausted=True`, + empty `final_message`. + """ + # Local import so unit tests exercising the SetupError path don't + # require the openai SDK to be importable. + from openai import AsyncOpenAI + + client = AsyncOpenAI() # reads OPENAI_API_KEY from env + + skill_prompt = _load_skill_prompt() + messages: list[dict[str, Any]] = [ + {"role": "system", "content": skill_prompt}, + { + "role": "user", + "content": ( + f"Run the audit against the repository at {fixture_dir}. " + "Use the audit_openssf_baseline tool. Summarize the " + "results per the skill's usual format after the tool " + "returns." + ), + }, + ] + + turn_count = 0 + for _turn in range(max_turns): + turn_count += 1 + response = await client.chat.completions.create( + model=model, + messages=messages, + tools=_TOOL_SCHEMAS, + tool_choice="auto", + temperature=0.0, + ) + msg = response.choices[0].message + + if msg.tool_calls: + # Append the assistant's tool-call message and each result. + messages.append( + { + "role": "assistant", + "content": msg.content or "", + "tool_calls": [ + { + "id": call.id, + "type": "function", + "function": { + "name": call.function.name, + "arguments": call.function.arguments, + }, + } + for call in msg.tool_calls + ], + } + ) + for call in msg.tool_calls: + result = _dispatch_tool_call(call, fixture_dir) + messages.append( + { + "role": "tool", + "tool_call_id": call.id, + "content": result, + } + ) + continue + + if msg.content: + return SkillInvocationResult( + final_message=msg.content, + model=model, + turn_count=turn_count, + metadata={"backend": "openai"}, + ) + + # No tool calls AND no content -- unusual; treat as effectively + # done and let the parser decide. + return SkillInvocationResult( + final_message="", + model=model, + turn_count=turn_count, + metadata={"backend": "openai", "empty_response": True}, + ) + + # Fell out of the loop without a final text message. + return SkillInvocationResult( + final_message="", + model=model, + turn_count=max_turns, + metadata={"backend": "openai"}, + turn_cap_exhausted=True, + ) + + +__all__ = ( + "OpenAIBackend", + "_TOOL_SCHEMAS", + "_dispatch_tool_call", +) diff --git a/tests/darnit/parity/tier2/claude_agent_sdk_client.py b/tests/darnit/parity/tier2/claude_agent_sdk_client.py index ee474ba..373a200 100644 --- a/tests/darnit/parity/tier2/claude_agent_sdk_client.py +++ b/tests/darnit/parity/tier2/claude_agent_sdk_client.py @@ -1,56 +1,25 @@ -"""Thin wrapper around `claude_agent_sdk.query` for Tier 2 (T018). +"""Backwards-compat shim (feature 029 T005). -Invokes the /darnit-audit skill against a fixture and returns the final -assistant message. Deterministic invocation: no streaming interrupts, no -interactive turns; explicit prompt + model + max_turns. +Superseded by `tests/darnit/parity/tier2/backends/claude_agent_sdk.py`. +Kept as an import path for one release cycle so existing feature-028 tests +continue to work without an update. -Per research.md R7: - - Prompt content is loaded from skill_prompt_snapshot.md. - - Model defaults to anthropic:claude-sonnet-5 (matches feature 025/026). - - max_turns cap prevents runaway budget consumption. - - ANTHROPIC_API_KEY MUST be set (FR-010); absence raises SetupError. +Do NOT add new callers to this module. Direct new code at +`tests.darnit.parity.tier2.backends.claude_agent_sdk.ClaudeAgentSdkBackend`. """ from __future__ import annotations -import os -from dataclasses import dataclass from pathlib import Path - -class SetupError(RuntimeError): - """Raised when Tier 2 lacks the environment it needs to invoke the skill.""" - - -@dataclass(frozen=True) -class SkillInvocationResult: - """Return type for `invoke_skill`.""" - - final_message: str - model: str - turn_count: int - metadata: dict[str, object] - - -PROMPT_SNAPSHOT_PATH = Path(__file__).parent / "skill_prompt_snapshot.md" - - -def _load_skill_prompt() -> str: - if not PROMPT_SNAPSHOT_PATH.exists(): - raise SetupError( - f"skill prompt snapshot missing: {PROMPT_SNAPSHOT_PATH}. Run T022 to capture it before invoking Tier 2.", - ) - return PROMPT_SNAPSHOT_PATH.read_text() - - -def _check_env() -> None: - """FR-010: fail fast when ANTHROPIC_API_KEY is absent.""" - if not os.environ.get("ANTHROPIC_API_KEY"): - raise SetupError( - "Tier 2 requires ANTHROPIC_API_KEY. Configure the " - "`parity-tier2` GitHub Actions Environment (or export the var " - "for a local run).", - ) +from tests.darnit.parity.tier2.backends.base import ( + SetupError, + SkillInvocationResult, +) +from tests.darnit.parity.tier2.backends.claude_agent_sdk import ( + PROMPT_SNAPSHOT_PATH, + ClaudeAgentSdkBackend, +) async def invoke_skill( @@ -58,81 +27,15 @@ async def invoke_skill( model: str = "anthropic:claude-sonnet-5", max_turns: int = 20, ) -> SkillInvocationResult: - """Invoke the /darnit-audit skill against `fixture_dir`. + """Deprecated: use `ClaudeAgentSdkBackend().invoke()` directly. - Returns a `SkillInvocationResult`. Raises SetupError on missing env - (before any API call is made). + Preserves feature 028's fail-fast semantics: check env before invoking. + The new Backend Protocol splits check_env and invoke into separate + calls (so the runner can fail fast without constructing a backend). + This shim reunites them to keep feature 028 tests green. """ - _check_env() - skill_prompt = _load_skill_prompt() - - # Import here so unit tests exercising the SetupError path don't - # require the SDK to be importable. - from claude_agent_sdk import ( - AssistantMessage, - ClaudeAgentOptions, - ResultMessage, - TextBlock, - query, - ) - - user_prompt = ( - f"{skill_prompt}\n\n" - f"Run the audit against the repository at {fixture_dir}. " - f"Summarize the results per the skill's usual format." - ) - - # PR #370 review fix: wire the darnit MCP server, allow the audit - # tool, and lock the agent down so it can't reach for out-of-band - # settings. Previously ClaudeAgentOptions passed only model, cwd, - # and max_turns -- the agent had NO way to call - # audit_openssf_baseline and the parity test measured "skill does - # nothing" instead of "skill vs tool". - options = ClaudeAgentOptions( - model=model.replace("anthropic:", ""), # SDK expects bare model name - max_turns=max_turns, - cwd=str(fixture_dir), - mcp_servers={ - "darnit": { - "type": "stdio", - "command": "darnit", - "args": ["serve", "--framework", "openssf-baseline"], - }, - }, - # Restrict to the specific MCP tool the parity test needs. Any - # off-list tool call is rejected by the SDK. - allowed_tools=["mcp__darnit__audit_openssf_baseline"], - # Auto-accept the tool call; parity CI is non-interactive. - permission_mode="acceptEdits", - # Isolate from the running host's Claude Code settings so a - # local dev's project/user config can't influence the run. - setting_sources=[], - ) - - final_text: str = "" - turn_count = 0 - result_meta: dict[str, object] = {} - - async for message in query(prompt=user_prompt, options=options): - if isinstance(message, AssistantMessage): - turn_count += 1 - # Concatenate any text blocks from this assistant turn. - for block in message.content: - if isinstance(block, TextBlock): - final_text = block.text # keep only the LAST turn's text - elif isinstance(message, ResultMessage): - # ResultMessage carries final metadata (usage, cost, etc.) - result_meta = { - "duration_ms": getattr(message, "duration_ms", None), - "num_turns": getattr(message, "num_turns", turn_count), - } - - return SkillInvocationResult( - final_message=final_text, - model=model, - turn_count=turn_count, - metadata=result_meta, - ) + ClaudeAgentSdkBackend.check_env() + return await ClaudeAgentSdkBackend().invoke(fixture_dir, model, max_turns) __all__ = ( @@ -140,4 +43,5 @@ async def invoke_skill( "SkillInvocationResult", "invoke_skill", "PROMPT_SNAPSHOT_PATH", + "ClaudeAgentSdkBackend", ) diff --git a/tests/darnit/parity/tier2/run.py b/tests/darnit/parity/tier2/run.py index 549a35f..ed19a3d 100644 --- a/tests/darnit/parity/tier2/run.py +++ b/tests/darnit/parity/tier2/run.py @@ -1,12 +1,13 @@ -"""Tier 2 runner entrypoint (feature 028 T021). +"""Tier 2 runner entrypoint (feature 028 T021, feature 029 T011). Invoked from the manual-dispatch GitHub Actions workflow. For each fixture: 1. Capture MCP tool JSON via direct Python call. - 2. Invoke the /darnit-audit skill via the Claude Agent SDK. - 3. Parse the skill's final assistant message. + 2. Invoke the coding-agent skill via the selected backend (Claude Agent + SDK by default, OpenAI via `--backend openai`). + 3. Parse the backend's final assistant message. 4. Run `diff()` and write per-fixture artifacts. - 5. Aggregate outcomes into an exit code per contract T2-13. + 5. Aggregate outcomes into an exit code. Exit codes: 0 -- success (every fixture agrees) @@ -14,9 +15,10 @@ 2 -- at least one fixture had unparseable skill output 3 -- setup error (missing key, missing fixtures) 4 -- rate limit exhausted (not automated; documented for follow-up) + 5 -- at least one fixture had turn_cap_exhausted (feature 029) -`--dry-run` stubs the SDK client with a canned response so the runner -can be exercised offline (used by the config workflow test). +`--dry-run` stubs the backend invocation with a canned response so the +runner can be exercised offline. """ from __future__ import annotations @@ -30,8 +32,10 @@ from tests.darnit.parity.tier1.comparator import AuditResult from tests.darnit.parity.tier2.artifact_writer import write_fixture_artifacts -from tests.darnit.parity.tier2.claude_agent_sdk_client import ( +from tests.darnit.parity.tier2.backends import ( + BACKEND_REGISTRY, SetupError, + SkillInvocationBackend, SkillInvocationResult, ) from tests.darnit.parity.tier2.diff import diff @@ -41,6 +45,16 @@ DEFAULT_ARTIFACT_ROOT = Path("parity-artifacts") +def _provider_filename_prefix(backend_name: str) -> str: + """Map backend name to the filename prefix used for the final-message + artifact. Preserves feature 028's `skill_final_message.md` for Claude + so downstream analysis scripts don't churn; other providers use + `_final_message.md`.""" + if backend_name == "claude_agent_sdk": + return "claude" + return backend_name + + def _discover_fixtures(fixture_glob: str) -> list[Path]: if not FIXTURES_DIR.exists(): return [] @@ -69,19 +83,21 @@ def _run_mcp_tool(fixture_dir: Path) -> tuple[str, AuditResult]: async def _run_skill( fixture_dir: Path, + backend_cls: type[SkillInvocationBackend] | None, + model: str, + max_turns: int, dry_run: bool, ) -> SkillInvocationResult: if dry_run: - # Stub: return a canned Markdown summary that the parser can extract. return SkillInvocationResult( final_message=("# Dry-Run Skill Output\n\nPassed: 0\nFailed: 0\nWarned: 0\n\nNo controls to summarize."), model="dry-run", turn_count=0, metadata={"dry_run": True}, ) - from tests.darnit.parity.tier2.claude_agent_sdk_client import invoke_skill - - return await invoke_skill(fixture_dir=fixture_dir) + assert backend_cls is not None + backend = backend_cls() + return await backend.invoke(fixture_dir, model, max_turns) def _write_step_summary(text: str) -> None: @@ -93,11 +109,15 @@ def _write_step_summary(text: str) -> None: f.write(text + "\n") -def _preflight_summary(fixture_glob: str) -> None: - """T2-7/T2-8: preflight audit line BEFORE consuming the API key.""" +def _preflight_summary(fixture_glob: str, backend_name: str, model: str) -> None: + """Feature 028 T2-7/T2-8 + feature 029 preflight: log actor + SHA + + fixture_glob + backend + model BEFORE consuming the API key.""" actor = os.environ.get("GITHUB_ACTOR", "") sha = os.environ.get("GITHUB_SHA", "") - line = f"Tier 2 parity preflight: actor={actor} sha={sha} fixture_glob={fixture_glob!r}" + line = ( + f"Tier 2 parity preflight: actor={actor} sha={sha} " + f"fixture_glob={fixture_glob!r} backend={backend_name!r} model={model!r}" + ) print(line, file=sys.stderr) _write_step_summary(line) @@ -105,31 +125,85 @@ def _preflight_summary(fixture_glob: str) -> None: async def _run_one_fixture( fixture_dir: Path, artifact_root: Path, + backend_cls: type[SkillInvocationBackend] | None, + backend_name: str, + model: str, + max_turns: int, dry_run: bool, ) -> str: """Return the diff outcome string for this fixture.""" mcp_raw, mcp_result = _run_mcp_tool(fixture_dir) - skill_result = await _run_skill(fixture_dir, dry_run=dry_run) - skill_report = SkillReport.parse(skill_result.final_message) - diff_report = diff(mcp_result, skill_report, fixture_dir.name) + skill_result = await _run_skill( + fixture_dir, + backend_cls, + model, + max_turns, + dry_run, + ) + + # Feature 029 T012: turn_cap_exhausted is a distinct outcome that + # bypasses the parseability + agreement checks. + if skill_result.turn_cap_exhausted: + outcome = "turn_cap_exhausted" + diff_md = ( + f"# Tier 2 parity: {fixture_dir.name}\n\n" + f"FAIL: model exhausted its turn cap ({max_turns}) without " + "emitting a final message. This is DISTINCT from unparseable " + "output -- the assistant kept calling tools instead of " + "summarizing.\n\n" + f"See `metadata.json` for the turn count. The raw tool-call " + "transcript is NOT captured (privacy + noise; not needed to " + "diagnose 'model didn't converge')." + ) + else: + skill_report = SkillReport.parse(skill_result.final_message) + diff_report = diff(mcp_result, skill_report, fixture_dir.name) + outcome = diff_report.outcome + diff_md = diff_report.diff_markdown write_fixture_artifacts( artifact_root=artifact_root, fixture_name=fixture_dir.name, mcp_json=mcp_raw, skill_markdown=skill_result.final_message, - diff_md=diff_report.diff_markdown, + diff_md=diff_md, metadata={ "model": skill_result.model, "turn_count": skill_result.turn_count, "dry_run": dry_run, + "backend": backend_name, + "turn_cap_exhausted": skill_result.turn_cap_exhausted, }, + provider=_provider_filename_prefix(backend_name), ) - return diff_report.outcome + return outcome + + +async def _main_async( + args: argparse.Namespace, + backends: dict[str, type[SkillInvocationBackend]] | None = None, +) -> int: + registry = backends if backends is not None else BACKEND_REGISTRY + backend_name = args.backend + if not args.dry_run and backend_name not in registry: + print( + f"Tier 2: unknown backend {backend_name!r}. Supported: {sorted(registry)}", + file=sys.stderr, + ) + return 3 + + backend_cls = None if args.dry_run else registry[backend_name] + _preflight_summary(args.fixture_glob, backend_name, args.model) -async def _main_async(args: argparse.Namespace) -> int: - _preflight_summary(args.fixture_glob) + # Fail fast on missing credentials BEFORE running any fixture's audit. + if not args.dry_run: + try: + backend_cls.check_env() + except SetupError as exc: + print(f"Tier 2 setup error: {exc}", file=sys.stderr) + _write_step_summary(f"setup_error: {exc}") + return 3 artifact_root = Path(args.artifact_dir) fixtures = _discover_fixtures(args.fixture_glob) @@ -138,18 +212,27 @@ async def _main_async(args: argparse.Namespace) -> int: f"Tier 2: no fixtures matched {args.fixture_glob!r} under {FIXTURES_DIR}", file=sys.stderr, ) - return 3 # setup error + return 3 outcomes: dict[str, list[str]] = { "success": [], "per_control_disagree": [], "counts_disagree": [], "skill_unparseable": [], + "turn_cap_exhausted": [], } for fixture_dir in fixtures: try: - outcome = await _run_one_fixture(fixture_dir, artifact_root, args.dry_run) + outcome = await _run_one_fixture( + fixture_dir, + artifact_root, + backend_cls, + backend_name, + args.model, + args.max_turns, + args.dry_run, + ) except SetupError as exc: print(f"Tier 2 setup error: {exc}", file=sys.stderr) _write_step_summary(f"setup_error: {exc}") @@ -166,23 +249,50 @@ async def _main_async(args: argparse.Namespace) -> int: summary = ( f"Tier 2 parity check: {len(fixtures)} fixtures checked, " f"{len(outcomes['per_control_disagree']) + len(outcomes['counts_disagree'])} drifts, " - f"{len(outcomes['skill_unparseable'])} unparseable" + f"{len(outcomes['skill_unparseable'])} unparseable, " + f"{len(outcomes['turn_cap_exhausted'])} turn-cap-exhausted" ) print(summary, file=sys.stderr) _write_step_summary(summary) - # Compute exit code per T2-13. + # Exit-code aggregation. Order matters: setup > errored > disagree > + # turn_cap_exhausted > unparseable > success. if outcomes.get("errored"): return 3 if outcomes["per_control_disagree"] or outcomes["counts_disagree"]: return 1 + if outcomes["turn_cap_exhausted"]: + return 5 if outcomes["skill_unparseable"]: return 2 return 0 -def main(argv: list[str] | None = None) -> int: - parser = argparse.ArgumentParser(description="Feature 028 Tier 2 parity runner") +def main( + argv: list[str] | None = None, + backends: dict[str, type[SkillInvocationBackend]] | None = None, +) -> int: + parser = argparse.ArgumentParser(description="Feature 028+029 Tier 2 parity runner") + parser.add_argument( + "--backend", + default="claude_agent_sdk", + help="Which Tier 2 backend to invoke (default: claude_agent_sdk).", + ) + parser.add_argument( + "--model", + default="anthropic:claude-sonnet-5", + help=( + "Model identifier for the backend. Claude default: " + "anthropic:claude-sonnet-5. OpenAI: workflow YAML supplies " + "a pinned versioned string (see SC-010)." + ), + ) + parser.add_argument( + "--max-turns", + type=int, + default=20, + help="Cap on assistant turns per fixture invocation (default: 20).", + ) parser.add_argument( "--fixture-glob", default="*", @@ -196,11 +306,13 @@ def main(argv: list[str] | None = None) -> int: parser.add_argument( "--dry-run", action="store_true", - help="Stub the SDK invocation with a canned response (no API call)", + help="Stub the backend invocation with a canned response (no API call)", ) args = parser.parse_args(argv) - return asyncio.new_event_loop().run_until_complete(_main_async(args)) + return asyncio.new_event_loop().run_until_complete( + _main_async(args, backends=backends), + ) if __name__ == "__main__": diff --git a/tests/darnit/parity/tier2/test_backend_extensibility.py b/tests/darnit/parity/tier2/test_backend_extensibility.py new file mode 100644 index 0000000..9b6478b --- /dev/null +++ b/tests/darnit/parity/tier2/test_backend_extensibility.py @@ -0,0 +1,138 @@ +"""Backend extensibility tests (feature 029 T020). + +Covers SC-007: a new backend can be added via constructor injection +(runner's `backends=` parameter) without touching any shared file. +""" + +from __future__ import annotations + +import json +from pathlib import Path + +from tests.darnit.parity.tier2.backends.base import ( + SkillInvocationBackend, + SkillInvocationResult, +) +from tests.darnit.parity.tier2.backends.noop import NoopBackend +from tests.darnit.parity.tier2.run import main + + +class TestSC007ExtensibilityViaConstructorInjection: + def test_noop_backend_invoked_via_injected_registry( + self, + tmp_path: Path, + ) -> None: + """SC-007: `run.main()` with a `backends=` dict override picks up + NoopBackend and invokes it. No edit to BACKEND_REGISTRY required.""" + artifact_dir = tmp_path / "artifacts" + + # NoopBackend requires no env; a run should complete cleanly. + rc = main( + argv=[ + "--backend", + "noop", + "--model", + "noop-model", + "--fixture-glob", + "all_pass_repo", + "--artifact-dir", + str(artifact_dir), + "--max-turns", + "1", + ], + backends={"noop": NoopBackend}, + ) + + # NoopBackend's canned response is unparseable by the shared parser + # (only "Passed: 0\nFailed: 0" -- no per-control claims), so we + # expect exit 2 (unparseable), NOT exit 3 (setup) or exit 1 + # (disagree). This proves the NoopBackend WAS invoked -- the runner + # got past the check_env fail-fast and ran the mock's canned + # response through the parser. + assert rc == 2, f"expected exit 2 (unparseable), got {rc}" + + # Artifact bundle written to the fixture path. + fixture_artifacts = artifact_dir / "all_pass_repo" + assert fixture_artifacts.exists() + # NoopBackend's provider filename prefix defaults to 'noop'. + assert (fixture_artifacts / "noop_final_message.md").exists() + metadata = json.loads((fixture_artifacts / "metadata.json").read_text()) + assert metadata.get("backend") == "noop" + + def test_ad_hoc_inline_backend_registered_and_invoked( + self, + tmp_path: Path, + ) -> None: + """SC-007 stronger form: define a backend inline (no module), + inject it, and confirm the runner picks it up.""" + + class _InlineTestBackend: + """Inline backend defined at test-collection time. If SC-007 + holds, no edit to run.py, diff.py, or any shared file is needed + to make this work.""" + + name = "inline_test" + + @classmethod + def check_env(cls) -> None: + return None + + async def invoke( + self, + fixture_dir: Path, + model: str, + max_turns: int, + ) -> SkillInvocationResult: + return SkillInvocationResult( + final_message=("# Inline test backend\n\nPassed: 0\nFailed: 0\n\n- **OSPS-DO-01.01**: PASS\n"), + model=model, + turn_count=1, + metadata={"backend": "inline_test"}, + ) + + assert isinstance(_InlineTestBackend(), SkillInvocationBackend) + + artifact_dir = tmp_path / "artifacts" + rc = main( + argv=[ + "--backend", + "inline_test", + "--model", + "inline-model", + "--fixture-glob", + "all_pass_repo", + "--artifact-dir", + str(artifact_dir), + "--max-turns", + "1", + ], + backends={"inline_test": _InlineTestBackend}, + ) + + # Whatever the diff outcome (likely disagree because the inline + # backend's canned OSPS-DO-01.01 status is PASS but the audit's + # actual OSPS-DO-01.01 might be different), the runner DID invoke + # the inline backend. Return codes 0, 1, or 2 all indicate the + # backend was reached; 3 (setup) would mean the injection failed. + assert rc in (0, 1, 2), ( + f"expected 0/1/2 (backend invoked); got {rc} which suggests the injected backend was not picked up" + ) + + def test_unknown_backend_name_returns_setup_error( + self, + tmp_path: Path, + ) -> None: + """Guardrail: `--backend ` fails fast with exit 3.""" + rc = main( + argv=[ + "--backend", + "nonexistent", + "--model", + "x", + "--fixture-glob", + "all_pass_repo", + "--artifact-dir", + str(tmp_path / "artifacts"), + ], + ) + assert rc == 3 diff --git a/tests/darnit/parity/tier2/test_backend_protocol_conformance.py b/tests/darnit/parity/tier2/test_backend_protocol_conformance.py new file mode 100644 index 0000000..bfcf0dc --- /dev/null +++ b/tests/darnit/parity/tier2/test_backend_protocol_conformance.py @@ -0,0 +1,48 @@ +"""SkillInvocationBackend Protocol conformance tests (feature 029 T019). + +Covers SC-005: every registered backend + NoopBackend satisfies the +Protocol via `isinstance` check. Guards against a refactor that +accidentally breaks the Protocol shape. +""" + +from __future__ import annotations + +import inspect + +import pytest + +from tests.darnit.parity.tier2.backends import BACKEND_REGISTRY +from tests.darnit.parity.tier2.backends.base import SkillInvocationBackend +from tests.darnit.parity.tier2.backends.noop import NoopBackend + + +class TestSC005AllRegisteredBackendsConform: + @pytest.mark.parametrize("name", list(BACKEND_REGISTRY)) + def test_registered_backend_satisfies_protocol(self, name: str) -> None: + cls = BACKEND_REGISTRY[name] + instance = cls() + assert isinstance(instance, SkillInvocationBackend), ( + f"{name!r} ({cls.__name__}) does not satisfy SkillInvocationBackend" + ) + + def test_noop_backend_satisfies_protocol(self) -> None: + """NoopBackend isn't in BACKEND_REGISTRY but must still conform.""" + assert isinstance(NoopBackend(), SkillInvocationBackend) + + +class TestBackendShapeInvariants: + @pytest.mark.parametrize("name", list(BACKEND_REGISTRY)) + def test_backend_has_non_empty_name_attribute(self, name: str) -> None: + cls = BACKEND_REGISTRY[name] + assert isinstance(cls.name, str) + assert cls.name # non-empty + + @pytest.mark.parametrize("name", list(BACKEND_REGISTRY)) + def test_backend_has_check_env_classmethod(self, name: str) -> None: + cls = BACKEND_REGISTRY[name] + assert inspect.ismethod(cls.check_env), f"{name}.check_env must be a classmethod (contract B-3)" + + @pytest.mark.parametrize("name", list(BACKEND_REGISTRY)) + def test_backend_has_async_invoke(self, name: str) -> None: + cls = BACKEND_REGISTRY[name] + assert inspect.iscoroutinefunction(cls.invoke), f"{name}.invoke must be an async method (contract B-2)" diff --git a/tests/darnit/parity/tier2/test_openai_backend_adversarial.py b/tests/darnit/parity/tier2/test_openai_backend_adversarial.py new file mode 100644 index 0000000..35df8b1 --- /dev/null +++ b/tests/darnit/parity/tier2/test_openai_backend_adversarial.py @@ -0,0 +1,236 @@ +"""OpenAI backend adversarial tests (feature 029 T016 + T017). + +Covers: + - SC-003: skill reclassification (WARN -> PASS) caught, any authority + - SC-011: turn_cap_exhausted outcome + exit code 5 + - FR-008 + FR-010 + FR-011 + FR-14 (MC2 parser reuse) + B-17 (local_path guard) +""" + +from __future__ import annotations + +import asyncio +import json +import os +import subprocess +import sys +from pathlib import Path +from types import SimpleNamespace +from unittest.mock import AsyncMock, patch + +import pytest + +from tests.darnit.parity.tier1.comparator import AuditResult, Control +from tests.darnit.parity.tier2.backends.base import ( + SetupError, +) +from tests.darnit.parity.tier2.backends.openai_backend import ( + OpenAIBackend, + _dispatch_tool_call, +) +from tests.darnit.parity.tier2.diff import diff +from tests.darnit.parity.tier2.skill_markdown_parser import SkillReport + + +def _run(coro): + return asyncio.new_event_loop().run_until_complete(coro) + + +def _mcp_result(*controls: Control) -> AuditResult: + return AuditResult(controls=tuple(controls), source="mcp_tool") + + +def _canned_response(*, tool_calls=None, content=None): + """Build a minimal ChatCompletion-shaped SimpleNamespace stub.""" + msg = SimpleNamespace( + tool_calls=tool_calls, + content=content, + ) + choice = SimpleNamespace(message=msg) + return SimpleNamespace(choices=[choice]) + + +class TestSC003SkillReclassificationCaught: + def test_warn_reclassified_as_pass_is_caught(self) -> None: + """SC-003 shape at the diff layer: parsed skill claim (PASS) vs + tool (WARN) -> per_control_disagree.""" + skill_md = "Passed: 1\nFailed: 0\n\n**OSPS-DO-01.01**: PASS" + skill_report = SkillReport.parse(skill_md) + assert skill_report.parseable + mcp = _mcp_result( + Control(id="OSPS-DO-01.01", status="WARN", authority="suggestive"), + ) + report = diff(mcp, skill_report, "sc003") + assert report.outcome == "per_control_disagree" + + def test_dispositive_authority_still_caught(self) -> None: + """FR-008/FR-11: authority level does not license reinterpretation.""" + skill_md = "Passed: 1\nFailed: 0\n\n**OSPS-LE-03.01**: PASS" + skill_report = SkillReport.parse(skill_md) + mcp = _mcp_result( + Control(id="OSPS-LE-03.01", status="FAIL", authority="dispositive"), + ) + report = diff(mcp, skill_report, "auth") + assert report.outcome == "per_control_disagree" + + +class TestSC011TurnCapExhausted: + """SC-011 / FR-010: model that keeps calling tools without summarizing + produces `turn_cap_exhausted=True`; runner returns exit 5.""" + + def test_turn_cap_exhausted_returns_flag_set( + self, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + # Mock AsyncOpenAI so every response has tool_calls and no content. + fake_call = SimpleNamespace( + id="call_1", + function=SimpleNamespace( + name="audit_openssf_baseline", + arguments=json.dumps({"local_path": "/tmp"}), + ), + ) + fake_response = _canned_response(tool_calls=[fake_call], content=None) + + mock_client = SimpleNamespace( + chat=SimpleNamespace( + completions=SimpleNamespace( + create=AsyncMock(return_value=fake_response), + ), + ), + ) + + monkeypatch.setenv("OPENAI_API_KEY", "fake-key-for-mock") + + with ( + patch( + "tests.darnit.parity.tier2.backends.openai_backend.AsyncOpenAI", + create=True, + return_value=mock_client, + ), + patch( + "tests.darnit.parity.tier2.backends.openai_backend._dispatch_tool_call", + return_value='{"results": []}', + ), + ): + # Also stub the openai import (delayed inside invoke()). + import openai + + openai.AsyncOpenAI = lambda: mock_client # type: ignore[assignment] + + backend = OpenAIBackend() + result = _run( + backend.invoke( + fixture_dir=Path("/tmp/fake_fixture"), + model="gpt-4o-2024-08-06", + max_turns=3, + ), + ) + + assert result.turn_cap_exhausted is True + assert result.final_message == "" + assert result.turn_count == 3 + + +class TestLocalPathForcedToFixtureDir: + """B-17: a rogue model cannot make the audit tool wander outside the + fixture directory. `_dispatch_tool_call` overrides `local_path`.""" + + def test_malicious_local_path_ignored( + self, + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, + ) -> None: + """The model tries to audit /etc/passwd; the backend forces the + fixture_dir.""" + call = SimpleNamespace( + id="call_evil", + function=SimpleNamespace( + name="audit_openssf_baseline", + arguments=json.dumps({"local_path": "/etc/passwd"}), + ), + ) + captured_local_path: dict[str, str] = {} + + def _mock_audit(**kwargs): + captured_local_path["value"] = kwargs["local_path"] + return '{"results": []}' + + # Patch the audit function at the import site inside _dispatch_tool_call. + with patch( + "darnit_baseline.tools.audit_openssf_baseline", + side_effect=_mock_audit, + ): + _dispatch_tool_call(call, tmp_path / "safe_fixture") + + # The malicious /etc/passwd path was overridden to the fixture dir. + assert captured_local_path["value"] == str(tmp_path / "safe_fixture") + assert captured_local_path["value"] != "/etc/passwd" + + +class TestFR010MissingApiKey: + def test_check_env_raises_setup_error( + self, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + """FR-008 / SC-004: OpenAIBackend.check_env() with no OPENAI_API_KEY + raises SetupError naming the missing variable.""" + monkeypatch.delenv("OPENAI_API_KEY", raising=False) + with pytest.raises(SetupError, match="OPENAI_API_KEY"): + OpenAIBackend.check_env() + + def test_run_py_exits_3_without_openai_key( + self, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + """SC-004: run.py subprocess exit code 3 when OPENAI_API_KEY absent.""" + env = {k: v for k, v in os.environ.items() if k != "OPENAI_API_KEY"} + env["PYTHONPATH"] = str(Path.cwd()) + # Keep ANTHROPIC_API_KEY in env just to prove it's ignored -- the + # OpenAI backend only checks OPENAI_API_KEY. + env["ANTHROPIC_API_KEY"] = "anthropic-key-should-not-help-openai" + + rc = subprocess.run( + [ + sys.executable, + "-m", + "tests.darnit.parity.tier2.run", + "--backend", + "openai", + "--fixture-glob", + "all_pass_repo", + "--artifact-dir", + str(tmp_path / "artifacts"), + ], + env=env, + capture_output=True, + text=True, + timeout=30, + ) + assert rc.returncode == 3, f"expected exit 3, got {rc.returncode}\nstderr: {rc.stderr}" + assert "OPENAI_API_KEY" in rc.stderr + + +class TestFR14MC2SharedParserHandlesOpenAIMarkdown: + """MC2 fix: an OpenAI-shaped final message is parseable by feature 028's + shared parser. Guards against silent parser fork.""" + + def test_openai_style_markdown_is_parseable(self) -> None: + openai_style = ( + "# Audit Report\n\n" + "Passed: 3\nFailed: 1\nWarned: 0\n\n" + "## Details\n\n" + "- **OSPS-DO-01.01**: PASS\n" + "- **OSPS-LE-03.01**: PASS\n" + "- **OSPS-GV-01.01**: FAIL\n" + "- **OSPS-BR-06.01**: PASS\n" + ) + report = SkillReport.parse(openai_style) + assert report.parseable + assert report.counts is not None + assert report.counts.get("pass") == 3 + assert report.counts.get("fail") == 1 + assert report.controls is not None + ids = {c.id for c in report.controls} + assert "OSPS-GV-01.01" in ids + assert "OSPS-DO-01.01" in ids diff --git a/tests/darnit/parity/tier2/test_shim_exports.py b/tests/darnit/parity/tier2/test_shim_exports.py new file mode 100644 index 0000000..439e3cb --- /dev/null +++ b/tests/darnit/parity/tier2/test_shim_exports.py @@ -0,0 +1,48 @@ +"""Backwards-compat shim inventory (feature 029 T009a / MC3). + +Defensive: imports every public name from feature 028's original module +surface via the shim path. If a future rename drops one of these names, +this test surfaces it explicitly rather than waiting for a downstream test +to break at a distance. +""" + +from __future__ import annotations + +from pathlib import Path + + +class TestShimReexports: + def test_setup_error_reexported(self) -> None: + from tests.darnit.parity.tier2.claude_agent_sdk_client import ( + SetupError, + ) + + assert issubclass(SetupError, RuntimeError) + + def test_skill_invocation_result_reexported(self) -> None: + from tests.darnit.parity.tier2.claude_agent_sdk_client import ( + SkillInvocationResult, + ) + + # Frozen dataclass with the feature 028 field set at minimum. + instance = SkillInvocationResult( + final_message="x", + model="y", + turn_count=1, + ) + assert instance.final_message == "x" + + def test_invoke_skill_reexported_and_callable(self) -> None: + from tests.darnit.parity.tier2.claude_agent_sdk_client import ( + invoke_skill, + ) + + assert callable(invoke_skill) + + def test_prompt_snapshot_path_reexported_as_path(self) -> None: + from tests.darnit.parity.tier2.claude_agent_sdk_client import ( + PROMPT_SNAPSHOT_PATH, + ) + + assert isinstance(PROMPT_SNAPSHOT_PATH, Path) + assert PROMPT_SNAPSHOT_PATH.exists() diff --git a/tests/darnit/parity/tier2/test_workflow_config.py b/tests/darnit/parity/tier2/test_workflow_config.py index 21ec5ba..d7eb6f2 100644 --- a/tests/darnit/parity/tier2/test_workflow_config.py +++ b/tests/darnit/parity/tier2/test_workflow_config.py @@ -101,3 +101,110 @@ def test_sc_005a_no_stray_anthropic_key_references(self) -> None: if "ANTHROPIC_API_KEY" in wf.read_text(): offenders.append(str(wf)) assert not offenders, f"ANTHROPIC_API_KEY MUST only appear in parity-tier2.yml. Offenders: {offenders}" + + +# --------------------------------------------------------------------------- +# Feature 029: OpenAI Tier 2 workflow config tests (T018) +# --------------------------------------------------------------------------- + +import re # noqa: E402 + +TIER2_OPENAI_WORKFLOW = WORKFLOWS_DIR / "parity-tier2-openai.yml" + + +def _load_openai_workflow() -> dict: + """Parse the Tier 2 OpenAI workflow YAML.""" + try: + import yaml + except ImportError: # pragma: no cover + pytest.skip("PyYAML not installed") + if not TIER2_OPENAI_WORKFLOW.exists(): + pytest.fail(f"Tier 2 OpenAI workflow missing: {TIER2_OPENAI_WORKFLOW}") + with TIER2_OPENAI_WORKFLOW.open() as f: + return yaml.safe_load(f) + + +class TestOpenAIWorkflowGovernance: + def test_ow_1_only_workflow_dispatch(self) -> None: + """OW-1: workflow_dispatch is the ONLY trigger.""" + workflow = _load_openai_workflow() + triggers = workflow.get("on") or workflow.get(True) + assert isinstance(triggers, dict) + assert set(triggers) == {"workflow_dispatch"} + + def test_ow_4_environment_declared(self) -> None: + """OW-4: environment: parity-tier2-openai.""" + workflow = _load_openai_workflow() + assert workflow["jobs"]["tier2"].get("environment") == "parity-tier2-openai" + + def test_ow_6_permissions_read_only(self) -> None: + """OW-6: contents: read; no write scopes.""" + workflow = _load_openai_workflow() + perms = workflow["jobs"]["tier2"].get("permissions") + assert perms is not None + assert perms.get("contents") == "read" + for k, v in perms.items(): + assert v != "write", f"forbidden write scope: {k}" + + def test_ow_13_no_api_key_input(self) -> None: + """OW-13 (T2-10 equivalent): no api_key workflow input.""" + workflow = _load_openai_workflow() + triggers = workflow.get("on") or workflow.get(True) + dispatch = triggers["workflow_dispatch"] + inputs = dispatch.get("inputs", {}) if isinstance(dispatch, dict) else {} + for name in inputs or {}: + assert "api_key" not in name.lower(), f"forbidden input {name!r} (OW-13)" + + def test_ow_14_artifact_upload_always(self) -> None: + """OW-14: upload-artifact runs with if: always().""" + workflow = _load_openai_workflow() + steps = workflow["jobs"]["tier2"]["steps"] + upload_steps = [s for s in steps if isinstance(s, dict) and "uses" in s and "upload-artifact" in s["uses"]] + assert upload_steps, "no upload-artifact step found" + for step in upload_steps: + condition = step.get("if") or step.get(True) + assert condition in ("always()", True), f"upload step must be `if: always()`, got {condition!r}" + + def test_sc_010_openai_workflow_pins_versioned_model(self) -> None: + """SC-010: the default OpenAI model MUST be a version-suffixed + string (e.g. gpt-4o-2024-08-06); a moving alias like gpt-4o + fails this test.""" + workflow = _load_openai_workflow() + triggers = workflow.get("on") or workflow.get(True) + inputs = triggers["workflow_dispatch"]["inputs"] + assert "model" in inputs, "workflow must expose a `model` input" + default_model = inputs["model"].get("default") + assert default_model is not None, "model input must have a default" + + # Two accepted version-pin shapes: + # date-suffixed: gpt-4o-2024-08-06 + # dotted-version: gpt-4o.1 + date_shape = re.fullmatch(r"[a-z0-9\-]+-\d{4}-\d{2}-\d{2}", default_model) + dot_shape = re.fullmatch(r"[a-z0-9\-]+\.\d+", default_model) + assert date_shape or dot_shape, ( + f"OpenAI model default {default_model!r} must be a versioned pin " + "(e.g. gpt-4o-2024-08-06 or gpt-4o.1), not a moving alias" + ) + + +class TestOpenAIApiKeyExclusivity: + """SC-002 + OW-7: OPENAI_API_KEY MUST appear only in parity-tier2-openai.yml.""" + + def test_sc_002_no_stray_openai_key_references(self) -> None: + offenders: list[str] = [] + for wf in list(WORKFLOWS_DIR.glob("*.yml")) + list(WORKFLOWS_DIR.glob("*.yaml")): + if wf.name == TIER2_OPENAI_WORKFLOW.name: + continue + if "OPENAI_API_KEY" in wf.read_text(): + offenders.append(str(wf)) + assert not offenders, f"OPENAI_API_KEY MUST only appear in parity-tier2-openai.yml. Offenders: {offenders}" + + def test_ow_9_no_anthropic_key_in_openai_workflow(self) -> None: + """OW-9: the OpenAI workflow does NOT expose ANTHROPIC_API_KEY.""" + content = TIER2_OPENAI_WORKFLOW.read_text() + assert "ANTHROPIC_API_KEY" not in content, "parity-tier2-openai.yml must not reference ANTHROPIC_API_KEY (OW-9)" + + def test_ow_8_no_openai_key_in_claude_workflow(self) -> None: + """OW-8: the Claude workflow does NOT expose OPENAI_API_KEY.""" + content = TIER2_WORKFLOW.read_text() + assert "OPENAI_API_KEY" not in content, "parity-tier2.yml must not reference OPENAI_API_KEY (OW-8)" diff --git a/uv.lock b/uv.lock index ceec51f..5d8bfcf 100644 --- a/uv.lock +++ b/uv.lock @@ -579,6 +579,7 @@ dev = [ ] parity-tier2 = [ { name = "claude-agent-sdk" }, + { name = "openai" }, ] [package.dev-dependencies] @@ -602,6 +603,7 @@ requires-dist = [ { name = "darnit-core", extras = ["attestation"], marker = "extra == 'attestation'", editable = "packages/darnit" }, { name = "darnit-gittuf", editable = "packages/darnit-gittuf" }, { name = "darnit-reproducibility", editable = "packages/darnit-reproducibility" }, + { name = "openai", marker = "extra == 'parity-tier2'", specifier = ">=1.50" }, { name = "pre-commit", marker = "extra == 'dev'", specifier = ">=4.0.0" }, { name = "pytest", marker = "extra == 'dev'", specifier = ">=8.0.0" }, { name = "pytest-cov", marker = "extra == 'dev'", specifier = ">=4.0.0" }, @@ -1349,6 +1351,25 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/88/b2/d0896bdcdc8d28a7fc5717c305f1a861c26e18c05047949fb371034d98bd/nodeenv-1.10.0-py2.py3-none-any.whl", hash = "sha256:5bb13e3eed2923615535339b3c620e76779af4cb4c6a90deccc9e36b274d3827", size = 23438, upload-time = "2025-12-20T14:08:52.782Z" }, ] +[[package]] +name = "openai" +version = "2.53.0" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "anyio" }, + { name = "distro" }, + { name = "httpx" }, + { name = "jiter" }, + { name = "pydantic" }, + { name = "sniffio" }, + { name = "tqdm" }, + { name = "typing-extensions" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/ef/cf/36e3e7235fdf6d125c052acc0970924611b17a20a4fe580596faf4566a65/openai-2.53.0.tar.gz", hash = "sha256:baf5802ad08980e1d9d561e1b996e800c8bcd14af5847c6d0e7a5cc59e4d4116", size = 1099435, upload-time = "2026-08-03T21:42:01.664Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/78/0f/cc6afea3542a5142c5d8fc8211c5e059a8375105d004a41dfa2c7948dbb0/openai-2.53.0-py3-none-any.whl", hash = "sha256:c694ffc747a3c4d1663ef2b07b811315a476164ee5efa3a993967349ebca7618", size = 1659829, upload-time = "2026-08-03T21:41:59.581Z" }, +] + [[package]] name = "opentelemetry-api" version = "1.44.0" @@ -2283,6 +2304,18 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/7b/61/cceae43728b7de99d9b847560c262873a1f6c98202171fd5ed62640b494b/tomli-2.4.1-py3-none-any.whl", hash = "sha256:0d85819802132122da43cb86656f8d1f8c6587d54ae7dcaf30e90533028b49fe", size = 14583, upload-time = "2026-03-25T20:22:03.012Z" }, ] +[[package]] +name = "tqdm" +version = "4.70.0" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "colorama", marker = "sys_platform == 'win32'" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/21/3b/6c24bec5be5e743ffd99576daa5cc077722fc7d5bbc00bd133fa0c698dc6/tqdm-4.70.0.tar.gz", hash = "sha256:55b0b0dbd97462d06ebee91e4dac24ed4d4702be82b24f07e6c1d27e08cea220", size = 795438, upload-time = "2026-07-27T11:33:15.271Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/f9/1c/01bfd571a64e7f270e6bab5e33777debe0edc56759233ce84f27dec92d14/tqdm-4.70.0-py3-none-any.whl", hash = "sha256:7f585706bfddbdebf89daac705b2dfcc16890130727d3197ca62c732b4310953", size = 80184, upload-time = "2026-07-27T11:33:13.167Z" }, +] + [[package]] name = "tree-sitter" version = "0.25.2" From b6d68adca8bf70ed54c41f3ae5a660473ea5afe5 Mon Sep 17 00:00:00 2001 From: Michael Lieberman Date: Tue, 11 Aug 2026 16:24:13 -0400 Subject: [PATCH 2/4] test(parity): add cross-provider aggregate diff script (feature 029 T022 / US3) Local maintainer script (`tests/darnit/parity/tier2/scripts/aggregate_provider_diff.py`) that reads Tier 2 artifact bundles from both providers (Claude via `skill_final_message.md`, OpenAI via `openai_final_message.md`) and produces a Markdown table per fixture showing where the two providers' final assistant messages agree or disagree on per-control status. Not invoked by CI (US3 is P3 in the spec; the useful signal is a local investigation, not an every-run automated check). Documented in quickstart.md as a maintainer workflow. Ten unit tests cover the parsing + diff logic (agreement, disagreement, missing artifacts, discovery-across-roots, exit codes) so the script doesn't silently rot. Closes T022 in feature 029's task list. Completes US3. --- specs/029-openai-parity-adapter/quickstart.md | 20 ++ specs/029-openai-parity-adapter/tasks.md | 2 +- tests/darnit/parity/tier2/scripts/__init__.py | 0 .../tier2/scripts/aggregate_provider_diff.py | 234 ++++++++++++++++++ .../scripts/test_aggregate_provider_diff.py | 138 +++++++++++ 5 files changed, 393 insertions(+), 1 deletion(-) create mode 100644 tests/darnit/parity/tier2/scripts/__init__.py create mode 100644 tests/darnit/parity/tier2/scripts/aggregate_provider_diff.py create mode 100644 tests/darnit/parity/tier2/scripts/test_aggregate_provider_diff.py diff --git a/specs/029-openai-parity-adapter/quickstart.md b/specs/029-openai-parity-adapter/quickstart.md index eaeb255..c7c7bcb 100644 --- a/specs/029-openai-parity-adapter/quickstart.md +++ b/specs/029-openai-parity-adapter/quickstart.md @@ -164,6 +164,26 @@ uv run pytest tests/darnit/parity/ -q Expected test count after feature 029 lands: feature-028 baseline + about 10-15 new tests (Protocol conformance + OpenAI adversarial + turn-cap-exhausted + workflow config). +## Cross-provider drift comparison (US3) + +Once both Claude and OpenAI Tier 2 workflows have run against the same commit, a maintainer can locally diff their final messages to see where the two providers agree or disagree. + +```bash +# 1. Download both artifact bundles. +gh run download --repo darnitdevorg/darnit +mv parity-artifacts parity-artifacts-claude + +gh run download --repo darnitdevorg/darnit +mv parity-artifacts parity-artifacts-openai + +# 2. Run the aggregate script. +uv run python -m tests.darnit.parity.tier2.scripts.aggregate_provider_diff \ + --claude-artifacts parity-artifacts-claude \ + --openai-artifacts parity-artifacts-openai +``` + +Output is one Markdown table per fixture with columns `control_id | claude_status | openai_status | disagreement`. Exit codes: 0 success, 1 no fixtures found, 2 missing arguments. Not invoked by CI -- a local maintainer runs it when investigating provider drift. + ## Related follow-ups - **Issue #369**: Add scheduled cadence + governance-appropriate key sourcing (applies to Claude AND OpenAI workflows; a single follow-up covers both). diff --git a/specs/029-openai-parity-adapter/tasks.md b/specs/029-openai-parity-adapter/tasks.md index b88a379..1c559b8 100644 --- a/specs/029-openai-parity-adapter/tasks.md +++ b/specs/029-openai-parity-adapter/tasks.md @@ -238,7 +238,7 @@ description: "Tasks for feature 029: OpenAI Tier 2 Parity Adapter -- second prov ### Implementation for US3 (optional, may slip) -- [ ] T022 [P] [US3] Create `tests/darnit/parity/tier2/scripts/aggregate_provider_diff.py` (local maintainer script, NOT invoked by any pytest). Reads two artifact bundles (`parity-artifacts-claude/` + `parity-artifacts-openai/`) OR one `parity-artifacts/` directory containing both providers' final messages; for each fixture, parses both providers' summaries; emits a Markdown table `| control_id | claude_status | openai_status | disagreement |`. +- [X] T022 [P] [US3] Create `tests/darnit/parity/tier2/scripts/aggregate_provider_diff.py` (local maintainer script, NOT invoked by any pytest). Reads two artifact bundles (`parity-artifacts-claude/` + `parity-artifacts-openai/`) OR one `parity-artifacts/` directory containing both providers' final messages; for each fixture, parses both providers' summaries; emits a Markdown table `| control_id | claude_status | openai_status | disagreement |`. - Not a runnable CI job; documented in `quickstart.md`. - Skipped if T022 slips -- US3 is P3. diff --git a/tests/darnit/parity/tier2/scripts/__init__.py b/tests/darnit/parity/tier2/scripts/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/tests/darnit/parity/tier2/scripts/aggregate_provider_diff.py b/tests/darnit/parity/tier2/scripts/aggregate_provider_diff.py new file mode 100644 index 0000000..0c21e95 --- /dev/null +++ b/tests/darnit/parity/tier2/scripts/aggregate_provider_diff.py @@ -0,0 +1,234 @@ +"""Aggregate cross-provider drift script (feature 029 T022, US3). + +Reads Tier 2 artifact bundles for BOTH the Claude and OpenAI backends and +produces a Markdown table per fixture showing where the two providers' +final assistant messages agree or disagree on per-control status. + +This is a LOCAL MAINTAINER script -- NOT invoked by any pytest module and +NOT run by CI. Its inputs are workflow-run artifact bundles downloaded via +`gh run download`. Example workflow: + + # 1. Dispatch Claude tier 2 workflow, download its artifacts: + gh run download --repo darnitdevorg/darnit + mv parity-artifacts parity-artifacts-claude + + # 2. Dispatch OpenAI tier 2 workflow, download its artifacts: + gh run download --repo darnitdevorg/darnit + mv parity-artifacts parity-artifacts-openai + + # 3. Diff the two bundles: + uv run python -m tests.darnit.parity.tier2.scripts.aggregate_provider_diff \\ + --claude-artifacts parity-artifacts-claude \\ + --openai-artifacts parity-artifacts-openai + +Alternate single-directory mode (if the two providers wrote to the same +fixture dir on separate dispatches, per feature 029's provider-filename +convention): + + uv run python -m tests.darnit.parity.tier2.scripts.aggregate_provider_diff \\ + --artifacts parity-artifacts + +Output goes to stdout as one Markdown table per fixture. +""" + +from __future__ import annotations + +import argparse +import sys +from pathlib import Path + +from tests.darnit.parity.tier2.skill_markdown_parser import SkillReport + + +def _read_message(path: Path) -> str | None: + """Read a final-message file if it exists; return None otherwise.""" + if not path.exists() or not path.is_file(): + return None + return path.read_text() + + +def _discover_fixtures(*roots: Path) -> list[str]: + """Every subdirectory of any root that has at least one final-message + file counts as a fixture.""" + names: set[str] = set() + for root in roots: + if not root.exists(): + continue + for child in root.iterdir(): + if not child.is_dir(): + continue + for candidate in ( + "skill_final_message.md", + "claude_final_message.md", + "openai_final_message.md", + ): + if (child / candidate).exists(): + names.add(child.name) + break + return sorted(names) + + +def _find_message( + fixture_name: str, + provider: str, + roots: list[Path], +) -> str | None: + """Locate the final-message artifact for `provider` under any of the + given artifact roots.""" + candidates: list[str] = [] + if provider == "claude": + candidates = ["skill_final_message.md", "claude_final_message.md"] + elif provider == "openai": + candidates = ["openai_final_message.md"] + else: + candidates = [f"{provider}_final_message.md"] + + for root in roots: + fixture_dir = root / fixture_name + for candidate in candidates: + content = _read_message(fixture_dir / candidate) + if content is not None: + return content + return None + + +def _diff_one_fixture( + fixture_name: str, + claude_message: str | None, + openai_message: str | None, +) -> str: + """Produce a Markdown section (heading + table) for one fixture.""" + lines: list[str] = [f"## Fixture: {fixture_name}"] + + if claude_message is None and openai_message is None: + lines.append("") + lines.append("No final-message artifacts found for either provider.") + lines.append("") + return "\n".join(lines) + + if claude_message is None: + lines.append("") + lines.append("Claude final message NOT found; only OpenAI to report.") + if openai_message is None: + lines.append("") + lines.append("OpenAI final message NOT found; only Claude to report.") + + claude_report = SkillReport.parse(claude_message) if claude_message else None + openai_report = SkillReport.parse(openai_message) if openai_message else None + + claude_by_id: dict[str, str] = {} + openai_by_id: dict[str, str] = {} + if claude_report and claude_report.controls: + claude_by_id = {c.id: c.status for c in claude_report.controls} + if openai_report and openai_report.controls: + openai_by_id = {c.id: c.status for c in openai_report.controls} + + control_ids = sorted(set(claude_by_id) | set(openai_by_id)) + if not control_ids: + lines.append("") + lines.append("Neither provider's message was parseable at the per-control level.") + lines.append("") + return "\n".join(lines) + + lines.append("") + lines.append("| control_id | claude_status | openai_status | disagreement |") + lines.append("| --- | --- | --- | --- |") + + disagreements = 0 + for cid in control_ids: + claude_s = claude_by_id.get(cid, "-") + openai_s = openai_by_id.get(cid, "-") + disagrees = claude_s != openai_s and claude_s != "-" and openai_s != "-" + marker = "YES" if disagrees else "" + if disagrees: + disagreements += 1 + lines.append(f"| {cid} | {claude_s} | {openai_s} | {marker} |") + + lines.append("") + lines.append( + f"Summary: {len(control_ids)} controls compared, {disagreements} disagreements.", + ) + lines.append("") + return "\n".join(lines) + + +def _build_parser() -> argparse.ArgumentParser: + parser = argparse.ArgumentParser( + description="Cross-provider Tier 2 drift diff", + ) + parser.add_argument( + "--claude-artifacts", + type=Path, + help="Directory containing Claude Tier 2 artifact bundle (e.g. from `gh run download`)", + ) + parser.add_argument( + "--openai-artifacts", + type=Path, + help="Directory containing OpenAI Tier 2 artifact bundle", + ) + parser.add_argument( + "--artifacts", + type=Path, + help="Single artifact root containing both providers' messages " + "(alternate mode; overrides --claude-artifacts and " + "--openai-artifacts if both provided).", + ) + return parser + + +def main(argv: list[str] | None = None) -> int: + args = _build_parser().parse_args(argv) + + roots: list[Path] = [] + if args.artifacts: + roots.append(args.artifacts) + if args.claude_artifacts: + roots.append(args.claude_artifacts) + if args.openai_artifacts: + roots.append(args.openai_artifacts) + + if not roots: + print( + "Provide at least one of --artifacts, --claude-artifacts, --openai-artifacts", + file=sys.stderr, + ) + return 2 + + fixture_names = _discover_fixtures(*roots) + if not fixture_names: + print("No fixtures with final-message artifacts found.", file=sys.stderr) + return 1 + + print("# Cross-provider Tier 2 drift diff") + print() + print(f"Fixtures analyzed: {len(fixture_names)}") + print(f"Roots: {[str(r) for r in roots]}") + print() + + total_disagreements = 0 + for name in fixture_names: + claude_message = _find_message(name, "claude", roots) + openai_message = _find_message(name, "openai", roots) + section = _diff_one_fixture(name, claude_message, openai_message) + print(section) + if "disagreements" in section: + # Grep the summary line for the count. + for line in section.splitlines(): + if line.startswith("Summary:") and "disagreements." in line: + parts = line.split(",") + if len(parts) >= 2: + try: + total_disagreements += int( + parts[1].strip().split()[0], + ) + except (IndexError, ValueError): + pass + + print("---") + print(f"**Total disagreements across all fixtures: {total_disagreements}**") + + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tests/darnit/parity/tier2/scripts/test_aggregate_provider_diff.py b/tests/darnit/parity/tier2/scripts/test_aggregate_provider_diff.py new file mode 100644 index 0000000..4daab38 --- /dev/null +++ b/tests/darnit/parity/tier2/scripts/test_aggregate_provider_diff.py @@ -0,0 +1,138 @@ +"""Smoke tests for aggregate_provider_diff (feature 029 T022). + +The script itself is a local maintainer tool, not a CI-invoked check -- +but the parsing + diff logic are worth testing to prevent silent bit-rot. +""" + +from __future__ import annotations + +from pathlib import Path + +from tests.darnit.parity.tier2.scripts.aggregate_provider_diff import ( + _diff_one_fixture, + _discover_fixtures, + _find_message, + main, +) + + +def _write_msg(root: Path, fixture: str, filename: str, content: str) -> None: + d = root / fixture + d.mkdir(parents=True, exist_ok=True) + (d / filename).write_text(content) + + +class TestDiffOneFixture: + def test_two_providers_agree(self) -> None: + claude = "Passed: 1\n\n- **OSPS-DO-01.01**: PASS" + openai = "Passed: 1\n\n- **OSPS-DO-01.01**: PASS" + section = _diff_one_fixture("fx", claude, openai) + assert "OSPS-DO-01.01 | PASS | PASS" in section + assert "0 disagreements" in section + + def test_two_providers_disagree_flagged(self) -> None: + claude = "Passed: 0\nFailed: 1\n\n- **OSPS-DO-01.01**: FAIL" + openai = "Passed: 1\nFailed: 0\n\n- **OSPS-DO-01.01**: PASS" + section = _diff_one_fixture("fx", claude, openai) + assert "| YES |" in section + assert "1 disagreements" in section + + def test_missing_openai_reported(self) -> None: + claude = "Passed: 1\n\n- **OSPS-DO-01.01**: PASS" + section = _diff_one_fixture("fx", claude, None) + assert "OpenAI final message NOT found" in section + + def test_missing_claude_reported(self) -> None: + openai = "Passed: 1\n\n- **OSPS-DO-01.01**: PASS" + section = _diff_one_fixture("fx", None, openai) + assert "Claude final message NOT found" in section + + def test_both_missing_reported(self) -> None: + section = _diff_one_fixture("fx", None, None) + assert "No final-message artifacts found" in section + + +class TestDiscoveryAndLookup: + def test_discover_fixtures_across_roots(self, tmp_path: Path) -> None: + claude_root = tmp_path / "claude" + openai_root = tmp_path / "openai" + _write_msg(claude_root, "a", "skill_final_message.md", "x") + _write_msg(openai_root, "b", "openai_final_message.md", "x") + _write_msg(claude_root, "c", "skill_final_message.md", "x") + _write_msg(openai_root, "c", "openai_final_message.md", "x") + + found = _discover_fixtures(claude_root, openai_root) + assert found == ["a", "b", "c"] + + def test_find_message_prefers_first_matching_root( + self, + tmp_path: Path, + ) -> None: + _write_msg( + tmp_path / "openai", + "fx", + "openai_final_message.md", + "openai-side", + ) + _write_msg( + tmp_path / "claude", + "fx", + "skill_final_message.md", + "claude-side", + ) + + claude_msg = _find_message( + "fx", + "claude", + [tmp_path / "claude", tmp_path / "openai"], + ) + openai_msg = _find_message( + "fx", + "openai", + [tmp_path / "openai", tmp_path / "claude"], + ) + assert claude_msg == "claude-side" + assert openai_msg == "openai-side" + + +class TestMainExitCode: + def test_main_exits_2_with_no_roots(self) -> None: + rc = main(argv=[]) + assert rc == 2 + + def test_main_exits_1_when_no_fixtures_present( + self, + tmp_path: Path, + ) -> None: + rc = main(argv=["--artifacts", str(tmp_path)]) + assert rc == 1 + + def test_main_success_end_to_end( + self, + tmp_path: Path, + capsys, + ) -> None: + _write_msg( + tmp_path / "cl", + "fx", + "skill_final_message.md", + "Passed: 1\n\n- **OSPS-DO-01.01**: PASS", + ) + _write_msg( + tmp_path / "op", + "fx", + "openai_final_message.md", + "Passed: 1\n\n- **OSPS-DO-01.01**: PASS", + ) + rc = main( + argv=[ + "--claude-artifacts", + str(tmp_path / "cl"), + "--openai-artifacts", + str(tmp_path / "op"), + ], + ) + assert rc == 0 + captured = capsys.readouterr() + assert "## Fixture: fx" in captured.out + assert "OSPS-DO-01.01" in captured.out From c4bda26609a94cead739d9f86bace4a25a4f0a60 Mon Sep 17 00:00:00 2001 From: Michael Lieberman Date: Wed, 12 Aug 2026 13:57:07 -0400 Subject: [PATCH 3/4] ci(parity-tier2-openai): pin actions to SHAs + move context expressions into env: vars Addresses Kusari Inspector findings on PR #371 -- same class of issue as the sibling fix for parity-tier2.yml on the 028 branch: 1. Shell injection risk (HIGH impact / HIGH likelihood): `${{ github.actor }}`, `${{ github.sha }}`, `${{ inputs.fixture_glob }}`, and `${{ inputs.model }}` were interpolated directly into `run:` blocks that had OPENAI_API_KEY set in env. A crafted fixture_glob or model input containing shell metacharacters could execute arbitrary commands and exfiltrate the key. Fix: move each context expression into a step-level `env:` variable and reference it as a quoted shell variable inside the run: block. 2. Supply-chain risk from unpinned actions: Same four actions (actions/checkout, actions/setup-python, astral-sh/setup-uv, actions/upload-artifact) used mutable version tags. Fix: pin each action to the same 40-character commit SHA used by the Claude workflow (both workflows share the identical pinned versions to simplify supply-chain review). Pinned versions match the Claude workflow exactly: actions/checkout 11d5960a326750d5838078e36cf38b85af677262 v4.4.0 actions/setup-python a26af69be951a213d495a4c3e4e4022e16d87065 v5.6.0 astral-sh/setup-uv d0cc045d04ccac9d8b7881df0226f9e82c39688e v6.8.0 actions/upload-artifact ea165f8d65b6e75b540449e92b4886f43607fa02 v4.6.2 All feature-029 workflow-config tests continue to pass (15 tests covering both workflows' governance-critical structure). --- .github/workflows/parity-tier2-openai.yml | 39 ++++++++++++++++------- 1 file changed, 28 insertions(+), 11 deletions(-) diff --git a/.github/workflows/parity-tier2-openai.yml b/.github/workflows/parity-tier2-openai.yml index aeb79a3..cf83cff 100644 --- a/.github/workflows/parity-tier2-openai.yml +++ b/.github/workflows/parity-tier2-openai.yml @@ -17,6 +17,14 @@ # - OW-10: preflight actor+SHA+model logged BEFORE the SDK step. # - OW-13: no api_key workflow input (governance regression guard). # - OW-14: artifact upload runs on any exit code (`if: always()`). +# +# Kusari Inspector hardening (2026-08-12): +# - Actions pinned to full commit SHAs (mutable version tags would allow +# a compromised action owner to silently repoint a tag and execute +# arbitrary code in the runner while OPENAI_API_KEY is in env). +# - All `${{ ... }}` context expressions in `run:` blocks moved to +# step-level `env:` variables and referenced as quoted shell vars. +# Prevents shell injection via crafted fixture_glob or model inputs. name: Parity Tier 2 (OpenAI) @@ -44,28 +52,33 @@ jobs: steps: - name: Checkout - uses: actions/checkout@v4 + uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 - name: Set up Python - uses: actions/setup-python@v5 + uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5.6.0 with: python-version: "3.12" - name: Install uv - uses: astral-sh/setup-uv@v6 + uses: astral-sh/setup-uv@d0cc045d04ccac9d8b7881df0226f9e82c39688e # v6.8.0 - name: Sync dev environment run: uv sync --extra dev - name: Preflight audit log (OW-10) + env: + ACTOR: ${{ github.actor }} + SHA: ${{ github.sha }} + FIXTURE_GLOB: ${{ inputs.fixture_glob }} + MODEL: ${{ inputs.model }} run: | { echo "## Tier 2 (OpenAI) Preflight" echo "" - echo "- actor: ${{ github.actor }}" - echo "- sha: ${{ github.sha }}" - echo "- fixture_glob: ${{ inputs.fixture_glob }}" - echo "- model: ${{ inputs.model }}" + echo "- actor: $ACTOR" + echo "- sha: $SHA" + echo "- fixture_glob: $FIXTURE_GLOB" + echo "- model: $MODEL" echo "- timestamp: $(date -u +%Y-%m-%dT%H:%M:%SZ)" } >> "$GITHUB_STEP_SUMMARY" @@ -76,17 +89,21 @@ jobs: # provider's key -- the Claude/Anthropic-scoped key is only # reachable from the sibling workflow (parity-tier2.yml). OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} + FIXTURE_GLOB: ${{ inputs.fixture_glob }} + MODEL: ${{ inputs.model }} run: | # Run as a module (`python -m`) so the `tests.` package imports - # inside run.py resolve. + # inside run.py resolve. FIXTURE_GLOB and MODEL are passed via env + # (not interpolated directly into this shell block) to prevent + # shell injection. uv run python -m tests.darnit.parity.tier2.run \ --backend openai \ - --model "${{ inputs.model }}" \ - --fixture-glob "${{ inputs.fixture_glob }}" + --model "$MODEL" \ + --fixture-glob "$FIXTURE_GLOB" - name: Upload parity artifacts (OW-14) if: always() - uses: actions/upload-artifact@v4 + uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 with: name: parity-artifacts-openai path: parity-artifacts/ From db4a8c3115daa7e962ee4767bf1b56b8b4eff25c Mon Sep 17 00:00:00 2001 From: Michael Lieberman Date: Thu, 13 Aug 2026 11:55:29 -0400 Subject: [PATCH 4/4] review-fix(pr-371): guard SDK import + pin OpenAI tool args + mark tests integration Reviewer pxp928 flagged one blocker (rebase-driven) and six should-fix items on #371. This commit addresses all of them. Blocker: - Both SC-007 tests inherited the missing-fixture-config bug from #370. Rebasing onto the updated 028 pulls the tracked `.baseline.toml` fixtures + the pytest-marker hook forward, so those tests now collect and run. Should-fix: - `import openai` in the OpenAI-backend adversarial test now goes through `pytest.importorskip("openai")`, so a lean env without the `parity-tier2` extra installed skips cleanly instead of exploding with ImportError. - `_dispatch_tool_call` now pins `level=3` and `output_format="json"` the same way `local_path` is pinned. Previously `setdefault` let a rogue model ask for markdown at level 1 and defeat the comparison. - Added a `tier2/conftest.py` `pytest_collection_modifyitems` hook so every Tier 2 test collects with the `integration` marker. Same reasoning as PR #370's tier1 conftest: CI's `-m unit / -m integration` split would otherwise silently deselect the whole suite. - The cross-provider aggregate script no longer silently reports "0 disagreements" when either provider's message was unparseable. It labels the fixture UNPARSEABLE in the per-fixture summary, lists unparseable fixtures at the bottom, and exits non-zero so CI can tell a "clean run" from a "no signal" run. - The diff-report formatter now accepts a `final_message_filename` keyword so an OpenAI-provider run's failure report points at `openai_final_message.md` instead of hardcoded `skill_final_message.md`. Threaded from `run.py` via `_provider_filename_prefix`. - Same filename thread covers the per-control and unparseable branches, so the diff output is consistent regardless of which provider ran. Workspace sweep: 2635 pass, 15 skip. --- .../parity/tier2/backends/openai_backend.py | 9 +++-- tests/darnit/parity/tier2/conftest.py | 18 ++++++++++ tests/darnit/parity/tier2/diff.py | 20 ++++++++--- tests/darnit/parity/tier2/run.py | 15 +++++++- .../tier2/scripts/aggregate_provider_diff.py | 36 +++++++++++++++++++ .../tier2/test_openai_backend_adversarial.py | 5 ++- 6 files changed, 94 insertions(+), 9 deletions(-) create mode 100644 tests/darnit/parity/tier2/conftest.py diff --git a/tests/darnit/parity/tier2/backends/openai_backend.py b/tests/darnit/parity/tier2/backends/openai_backend.py index 8eded38..e7ec09a 100644 --- a/tests/darnit/parity/tier2/backends/openai_backend.py +++ b/tests/darnit/parity/tier2/backends/openai_backend.py @@ -73,10 +73,13 @@ def _dispatch_tool_call(call: Any, fixture_dir: Path) -> str: args = json.loads(call.function.arguments or "{}") except json.JSONDecodeError: args = {} - # Force local_path to the fixture dir. + # PR #371 review fix: pin level + output_format the same way + # local_path is pinned. Previous `setdefault` let a rogue model + # ask for `output_format="markdown"` and `level=1`, defeating + # the comparison against the tool's JSON @ level 3. args["local_path"] = str(fixture_dir) - args.setdefault("output_format", "json") - args.setdefault("level", 3) + args["output_format"] = "json" + args["level"] = 3 # Force safe defaults. args["auto_init_config"] = False args["attest"] = False diff --git a/tests/darnit/parity/tier2/conftest.py b/tests/darnit/parity/tier2/conftest.py new file mode 100644 index 0000000..918603d --- /dev/null +++ b/tests/darnit/parity/tier2/conftest.py @@ -0,0 +1,18 @@ +"""Tier 2 conftest (PR #371 review fix). + +Adds the `integration` pytest marker to every test collected under this +directory. Tier 2 tests exercise the coding-agent parity flow (skill +prompt + provider backend + parser), which never satisfies the `unit` +marker's contract. Without the mark, CI's `-m unit / -m integration` +split silently deselected the entire suite. +""" + +from __future__ import annotations + +import pytest + + +def pytest_collection_modifyitems(config: pytest.Config, items: list) -> None: + integration_mark = pytest.mark.integration + for item in items: + item.add_marker(integration_mark) diff --git a/tests/darnit/parity/tier2/diff.py b/tests/darnit/parity/tier2/diff.py index 822fe71..0d5e568 100644 --- a/tests/darnit/parity/tier2/diff.py +++ b/tests/darnit/parity/tier2/diff.py @@ -40,6 +40,8 @@ def diff( mcp_result: AuditResult, skill_report: SkillReport, fixture_name: str, + *, + final_message_filename: str = "skill_final_message.md", ) -> Tier2DiffReport: """Compare tool JSON vs skill summary. @@ -48,12 +50,18 @@ def diff( 2. Per-control status disagreement -> PER_CONTROL_DISAGREE. 3. Summary-count disagreement (per-control agrees) -> COUNTS_DISAGREE. 4. All-agree -> success. + + `final_message_filename` is threaded into the failure reports so an + OpenAI-provider run reports `openai_final_message.md` rather than + the default Claude filename. PR #371 review fix. """ if not skill_report.parseable: return Tier2DiffReport( fixture_name=fixture_name, outcome="skill_unparseable", - diff_markdown=_format_unparseable_report(fixture_name, skill_report), + diff_markdown=_format_unparseable_report( + fixture_name, skill_report, final_message_filename, + ), ) # Per-control comparison: iterate both directions so a skill that @@ -85,7 +93,9 @@ def diff( fixture_name=fixture_name, outcome="per_control_disagree", disagreeing_controls=tuple(d[0] for d in disagreements), - diff_markdown=_format_per_control_report(fixture_name, disagreements), + diff_markdown=_format_per_control_report( + fixture_name, disagreements, final_message_filename, + ), ) # Counts comparison. Compute tool counts from AuditResult. @@ -127,6 +137,7 @@ def _tool_counts(mcp_result: AuditResult) -> dict[str, int]: def _format_per_control_report( fixture_name: str, disagreements: list[tuple[str, str, str]], + final_message_filename: str = "skill_final_message.md", ) -> str: lines = [ f"# Tier 2 parity: {fixture_name}", @@ -140,7 +151,7 @@ def _format_per_control_report( lines.append(f"| {cid} | {tool_s} | {skill_s} |") lines.append("") lines.append( - "See `mcp_tool_result.json` and `skill_final_message.md` in this directory for the raw artifacts.", + f"See `mcp_tool_result.json` and `{final_message_filename}` in this directory for the raw artifacts.", ) return "\n".join(lines) @@ -168,6 +179,7 @@ def _format_counts_report( def _format_unparseable_report( fixture_name: str, skill_report: SkillReport, + final_message_filename: str = "skill_final_message.md", ) -> str: return ( f"# Tier 2 parity: {fixture_name}\n\n" @@ -175,7 +187,7 @@ def _format_unparseable_report( "disagreement -- the parser did not find the expected shape in the " "skill's final message.\n\n" f"Parser notes: {list(skill_report.parse_notes) or ['(none)']}\n\n" - "See `skill_final_message.md` in this directory for the raw output.\n" + f"See `{final_message_filename}` in this directory for the raw output.\n" ) diff --git a/tests/darnit/parity/tier2/run.py b/tests/darnit/parity/tier2/run.py index ed19a3d..65b9e1e 100644 --- a/tests/darnit/parity/tier2/run.py +++ b/tests/darnit/parity/tier2/run.py @@ -157,7 +157,20 @@ async def _run_one_fixture( ) else: skill_report = SkillReport.parse(skill_result.final_message) - diff_report = diff(mcp_result, skill_report, fixture_dir.name) + # PR #371 review fix: thread the provider's final-message filename + # into the diff so failure reports point at + # `openai_final_message.md` rather than the default + # `skill_final_message.md` on the OpenAI provider path. + provider = _provider_filename_prefix(backend_name) + final_message_filename = ( + "skill_final_message.md" if provider == "claude" else f"{provider}_final_message.md" + ) + diff_report = diff( + mcp_result, + skill_report, + fixture_dir.name, + final_message_filename=final_message_filename, + ) outcome = diff_report.outcome diff_md = diff_report.diff_markdown diff --git a/tests/darnit/parity/tier2/scripts/aggregate_provider_diff.py b/tests/darnit/parity/tier2/scripts/aggregate_provider_diff.py index 0c21e95..20e2981 100644 --- a/tests/darnit/parity/tier2/scripts/aggregate_provider_diff.py +++ b/tests/darnit/parity/tier2/scripts/aggregate_provider_diff.py @@ -123,13 +123,35 @@ def _diff_one_fixture( if openai_report and openai_report.controls: openai_by_id = {c.id: c.status for c in openai_report.controls} + # PR #371 review fix: distinguish "parseable and 0 disagreements" + # from "unparseable on one/both sides" -- previously both cases + # silently reported "0 disagreements" in the aggregate summary. + claude_parseable = bool(claude_report and claude_report.parseable) + openai_parseable = bool(openai_report and openai_report.parseable) + control_ids = sorted(set(claude_by_id) | set(openai_by_id)) if not control_ids: lines.append("") lines.append("Neither provider's message was parseable at the per-control level.") lines.append("") + # Mark the summary as UNPARSEABLE so downstream aggregation + # cannot count this as a clean "0 disagreements" run. + lines.append("Summary: 0 controls compared, UNPARSEABLE.") + lines.append("") return "\n".join(lines) + if not (claude_parseable and openai_parseable): + missing = [] + if not claude_parseable: + missing.append("claude") + if not openai_parseable: + missing.append("openai") + lines.append("") + lines.append( + f"Note: {', '.join(missing)} message(s) partially unparseable; " + "disagreement count is a lower bound.", + ) + lines.append("") lines.append("| control_id | claude_status | openai_status | disagreement |") lines.append("| --- | --- | --- | --- |") @@ -206,11 +228,19 @@ def main(argv: list[str] | None = None) -> int: print() total_disagreements = 0 + unparseable_fixtures: list[str] = [] for name in fixture_names: claude_message = _find_message(name, "claude", roots) openai_message = _find_message(name, "openai", roots) section = _diff_one_fixture(name, claude_message, openai_message) print(section) + # PR #371 review fix: recognize UNPARSEABLE explicitly and skip + # counting -- previously a fully unparseable fixture matched the + # "disagreements" branch with a zero and was silently added as + # a clean run. + if "UNPARSEABLE" in section: + unparseable_fixtures.append(name) + continue if "disagreements" in section: # Grep the summary line for the count. for line in section.splitlines(): @@ -226,6 +256,12 @@ def main(argv: list[str] | None = None) -> int: print("---") print(f"**Total disagreements across all fixtures: {total_disagreements}**") + if unparseable_fixtures: + print( + f"**Unparseable fixtures (excluded from disagreement count): " + f"{unparseable_fixtures}**", + ) + return 1 # non-zero exit so CI won't file an unparseable run as clean return 0 diff --git a/tests/darnit/parity/tier2/test_openai_backend_adversarial.py b/tests/darnit/parity/tier2/test_openai_backend_adversarial.py index 35df8b1..32443ce 100644 --- a/tests/darnit/parity/tier2/test_openai_backend_adversarial.py +++ b/tests/darnit/parity/tier2/test_openai_backend_adversarial.py @@ -113,7 +113,10 @@ def test_turn_cap_exhausted_returns_flag_set( ), ): # Also stub the openai import (delayed inside invoke()). - import openai + # PR #371 review fix: skip cleanly when the parity-tier2 extra + # is not installed (unit runs on a lean env), instead of + # blowing up with ImportError. + openai = pytest.importorskip("openai") openai.AsyncOpenAI = lambda: mock_client # type: ignore[assignment]