feat(v0): add cloud_review policy field + ReviewerLane + D20 force-forbidden enforcement (closes #37) #44

Merged

codex merged 1 commit from claude/patchwarden-cloud-review-policy into main

2026-05-26 15:32:51 +02:00

claude commented

2026-05-26 14:39:28 +02:00

Collaborator

Summary

Adds cloud_review policy field + ReviewerLane dataclass + seed repo_pattern_sanity lane + helper effective_cloud_review() that architecturally enforces D20 hybrid review authority boundary (force forbidden for sensitive classifications regardless of lane config). Stdlib-only. No model calls. No webhook. No platform touch.

Closes #37. Companion to PR #43 (D20 ADR — recommended merge order: #43 first, then this).

What changed (3 files, +372/-1)

`src/patchwarden/policy_bundle.py` (+128)

ALLOWED_CLOUD_REVIEW_VALUES = ("allowed", "forbidden", "required-with-redaction") — enum constants per issue spec
SENSITIVE_CLASSIFICATIONS frozenset — mirror of D20 hard manual classes + large/unknown (conservative defaults)
ReviewerLane frozen dataclass — name, enabled, required_for, cloud_review, provider, model, fallback_model, timeout_seconds, fail_on_missing, fail_on_unparseable
PolicyBundle.review_lanes: tuple[ReviewerLane, ...] = () — optional field with empty default (back-compat)
load_bundle() extended — parses optional [review_lanes.*] TOML sections
effective_cloud_review(classification, lane) helper — architectural enforcement of D20: sensitive classifications always return "forbidden" regardless of lane config; None lane defaults to "forbidden" (conservative)
lane_required_for(lane, classification) helper — scaffold for future #28/#29/#30 reviewer-orchestration code to enforce fail-closed missing-required-lane behavior
_parse_review_lanes() + _parse_single_lane() + _optional_strings() parsers — full validation: invalid enum rejected, missing required fields rejected, optional sections OK

`policies/platform.v0.toml` (+17)

Adds [review_lanes.repo_pattern_sanity] seed lane per issue spec:
- enabled = true, required_for = ["safe_docs_status"], cloud_review = "allowed"
- provider = "ollama", model = "kimi-k2.6:cloud", fallback_model = "gemma4:31b-cloud"
- timeout_seconds = 90, fail_on_missing = true, fail_on_unparseable = true
Inline comment explaining D20 force-forbidden semantics + reference to effective_cloud_review()

`tests/test_policy_bundle.py` (+228, total 22 tests, all passing)

4 test classes:

PolicyBundleTests (existing, unchanged) — 3 tests for back-compat
ReviewerLaneParsingTests — 5 tests: seed loads, optional section, invalid enum rejected, missing required field rejected, required_for defaults to empty
EffectiveCloudReviewTests — 10 tests: each sensitive classification forces forbidden, safe_docs_status with allowed lane returns allowed, no-lane defaults to forbidden, sentinel test that all D20 hard manual classes are in SENSITIVE_CLASSIFICATIONS
LaneRequiredForTests — 3 tests for required-for matching + disabled lane never required
CloudReviewEnumTests — 1 test locking enum values to D20 spec

Full test suite: PYTHONPATH=src python3 -m unittest discover -s tests → 63 tests passing.

Acceptance criteria (from issue #37)

Policy/lane config parser accepts cloud_review with allowed values only — _parse_single_lane() validates against ALLOWED_CLOUD_REVIEW_VALUES, test test_invalid_cloud_review_enum_rejected
Sensitive/manual classes force cloud_review=forbidden regardless of lane config — effective_cloud_review() helper + 7 dedicated tests covering secrets / workflow / runtime / policy_governance / policy_exact_file / large / unknown
Optional lane failure does not block unless configured required — lane_required_for() helper exposes this distinction for future reviewer-orchestration code (#28/#29/#30); current PR only provides the predicate
Tests cover: safe docs allowed, workflow/runtime/secrets forbidden, invalid enum rejected — all in EffectiveCloudReviewTests + ReviewerLaneParsingTests
No real model calls in this issue
[~] Missing/stale/unparseable required lane artifacts fail closed — scaffold only: lane.fail_on_missing / lane.fail_on_unparseable config fields parsed + lane_required_for() predicate exposed; full fail-closed enforcement requires lane-artifact lifecycle (review_artifact loading, staleness check) which lives in #28-#30 reviewer orchestration trio. This PR provides the config surface + predicates that #28-#30 will consume.

Out of scope (per issue + D20)

❌ Webhook service mode
❌ Calling Ollama Cloud (or any LLM provider)
❌ Posting Forgejo APPROVED reviews
❌ Calling merge endpoints from any reviewer-lane code path (per D20 architectural enforcement)
❌ Cloud credentials/secrets
❌ Merge automation expansion
❌ Platform workflow changes

Architectural enforcement of D20 (verifiable by code review)

Per D20 in docs/decisions.md (PR #43): reviewer lanes are sensors only. This PR's code surface reflects that:

policy_bundle.py has no merge_* or post_approved_review imports
policy_bundle.py has no calls to forgejo_client (it's a pure data + validation module)
ReviewerLane dataclass has no merge_authority field — verdict authority is always Patchwarden per code path
effective_cloud_review() returns posture ("allowed" / "forbidden" / "required-with-redaction") — never a merge decision

Verification

PYTHONPATH=src python3 -m unittest tests.test_policy_bundle -v → 22 tests pass
PYTHONPATH=src python3 -m unittest discover -s tests → 63 tests pass (full suite, no regressions)
grep -E "(merge|approve)" src/patchwarden/policy_bundle.py → only comments referencing D20, no actual merge/approve API calls
policies/platform.v0.toml parses cleanly via load_bundle() (covered by test_loads_platform_bundle)

Trace

Closes #37
Companion to PR #43 — D20 hybrid review authority boundary ADR (recommended merge order: #43 first, then this)
Oracle consult: patchwarde-auditlm-boundary, 2026-05-26
Future consumers: #28 reviewer lane config loader, #29 prompt rendering, #30 finding comments
Roadmap reference: G9 cloud_review schema field (now ✅)

Suggested status

Merge now (after PR #43 merged) — atomic, stdlib-only, no model calls, no webhook, no platform touch. Full test suite green. Architectural enforcement of D20 baked into code surface (not just comments). Unblocks #28-#30 reviewer orchestration trio.

If you prefer split A/B/C per issue spec, this PR is effectively A+C (parser/schema/config) + scaffold for B (policy behavior predicates exposed; full enforcement in #28-#30). Pure split-A would drop the effective_cloud_review() + lane_required_for() helpers + their 14 tests, but they are tiny, stdlib-only, and architecturally meaningful — recommend keeping together.

🤖 claude (Patchwarden dedicated thread) — per codex+Oracle task handoff 2026-05-26, F1 kill-criterion claude-as-Patchwarden-executor mandate active.

## Summary Adds `cloud_review` policy field + `ReviewerLane` dataclass + seed `repo_pattern_sanity` lane + helper `effective_cloud_review()` that architecturally enforces D20 hybrid review authority boundary (force `forbidden` for sensitive classifications regardless of lane config). Stdlib-only. No model calls. No webhook. No platform touch. Closes [#37](https://git.pdurlej.com/pdurlej/patchwarden/issues/37). Companion to [PR #43](https://git.pdurlej.com/pdurlej/patchwarden/pulls/43) (D20 ADR — recommended merge order: #43 first, then this). ## What changed (3 files, +372/-1) ### `src/patchwarden/policy_bundle.py` (+128) - `ALLOWED_CLOUD_REVIEW_VALUES = ("allowed", "forbidden", "required-with-redaction")` — enum constants per issue spec - `SENSITIVE_CLASSIFICATIONS` frozenset — mirror of D20 hard manual classes + `large`/`unknown` (conservative defaults) - `ReviewerLane` frozen dataclass — name, enabled, required_for, cloud_review, provider, model, fallback_model, timeout_seconds, fail_on_missing, fail_on_unparseable - `PolicyBundle.review_lanes: tuple[ReviewerLane, ...] = ()` — optional field with empty default (back-compat) - `load_bundle()` extended — parses optional `[review_lanes.*]` TOML sections - `effective_cloud_review(classification, lane)` helper — **architectural enforcement of D20**: sensitive classifications always return `"forbidden"` regardless of lane config; `None` lane defaults to `"forbidden"` (conservative) - `lane_required_for(lane, classification)` helper — scaffold for future #28/#29/#30 reviewer-orchestration code to enforce fail-closed missing-required-lane behavior - `_parse_review_lanes()` + `_parse_single_lane()` + `_optional_strings()` parsers — full validation: invalid enum rejected, missing required fields rejected, optional sections OK ### `policies/platform.v0.toml` (+17) - Adds `[review_lanes.repo_pattern_sanity]` seed lane per issue spec: - `enabled = true`, `required_for = ["safe_docs_status"]`, `cloud_review = "allowed"` - `provider = "ollama"`, `model = "kimi-k2.6:cloud"`, `fallback_model = "gemma4:31b-cloud"` - `timeout_seconds = 90`, `fail_on_missing = true`, `fail_on_unparseable = true` - Inline comment explaining D20 force-forbidden semantics + reference to `effective_cloud_review()` ### `tests/test_policy_bundle.py` (+228, total 22 tests, all passing) 4 test classes: - `PolicyBundleTests` (existing, unchanged) — 3 tests for back-compat - `ReviewerLaneParsingTests` — 5 tests: seed loads, optional section, invalid enum rejected, missing required field rejected, `required_for` defaults to empty - `EffectiveCloudReviewTests` — 10 tests: each sensitive classification forces forbidden, safe_docs_status with allowed lane returns allowed, no-lane defaults to forbidden, sentinel test that all D20 hard manual classes are in `SENSITIVE_CLASSIFICATIONS` - `LaneRequiredForTests` — 3 tests for required-for matching + disabled lane never required - `CloudReviewEnumTests` — 1 test locking enum values to D20 spec **Full test suite**: `PYTHONPATH=src python3 -m unittest discover -s tests` → **63 tests passing**. ## Acceptance criteria (from issue #37) - [x] Policy/lane config parser accepts `cloud_review` with allowed values only — `_parse_single_lane()` validates against `ALLOWED_CLOUD_REVIEW_VALUES`, test `test_invalid_cloud_review_enum_rejected` - [x] Sensitive/manual classes force `cloud_review=forbidden` regardless of lane config — `effective_cloud_review()` helper + 7 dedicated tests covering secrets / workflow / runtime / policy_governance / policy_exact_file / large / unknown - [x] Optional lane failure does not block unless configured required — `lane_required_for()` helper exposes this distinction for future reviewer-orchestration code (#28/#29/#30); current PR only provides the predicate - [x] Tests cover: safe docs allowed, workflow/runtime/secrets forbidden, invalid enum rejected — all in `EffectiveCloudReviewTests` + `ReviewerLaneParsingTests` - [x] No real model calls in this issue - [~] Missing/stale/unparseable required lane artifacts fail closed — **scaffold only**: `lane.fail_on_missing` / `lane.fail_on_unparseable` config fields parsed + `lane_required_for()` predicate exposed; full fail-closed enforcement requires lane-artifact lifecycle (review_artifact loading, staleness check) which lives in #28-#30 reviewer orchestration trio. This PR provides the config surface + predicates that #28-#30 will consume. ## Out of scope (per issue + D20) - ❌ Webhook service mode - ❌ Calling Ollama Cloud (or any LLM provider) - ❌ Posting Forgejo APPROVED reviews - ❌ Calling merge endpoints from any reviewer-lane code path (per D20 architectural enforcement) - ❌ Cloud credentials/secrets - ❌ Merge automation expansion - ❌ Platform workflow changes ## Architectural enforcement of D20 (verifiable by code review) Per D20 in `docs/decisions.md` (PR #43): reviewer lanes are sensors only. This PR's code surface reflects that: - `policy_bundle.py` has **no `merge_*` or `post_approved_review` imports** - `policy_bundle.py` has **no calls** to `forgejo_client` (it's a pure data + validation module) - `ReviewerLane` dataclass has **no `merge_authority` field** — verdict authority is always Patchwarden per code path - `effective_cloud_review()` returns posture (`"allowed"` / `"forbidden"` / `"required-with-redaction"`) — never a merge decision ## Verification - [ ] `PYTHONPATH=src python3 -m unittest tests.test_policy_bundle -v` → 22 tests pass - [ ] `PYTHONPATH=src python3 -m unittest discover -s tests` → 63 tests pass (full suite, no regressions) - [ ] `grep -E "(merge|approve)" src/patchwarden/policy_bundle.py` → only comments referencing D20, no actual merge/approve API calls - [ ] `policies/platform.v0.toml` parses cleanly via `load_bundle()` (covered by `test_loads_platform_bundle`) ## Trace - Closes [#37](https://git.pdurlej.com/pdurlej/patchwarden/issues/37) - Companion to [PR #43](https://git.pdurlej.com/pdurlej/patchwarden/pulls/43) — D20 hybrid review authority boundary ADR (recommended merge order: #43 first, then this) - Oracle consult: `patchwarde-auditlm-boundary`, 2026-05-26 - Future consumers: #28 reviewer lane config loader, #29 prompt rendering, #30 finding comments - Roadmap reference: G9 `cloud_review` schema field (now ✅) ## Suggested status **Merge now** (after PR #43 merged) — atomic, stdlib-only, no model calls, no webhook, no platform touch. Full test suite green. Architectural enforcement of D20 baked into code surface (not just comments). Unblocks #28-#30 reviewer orchestration trio. If you prefer split A/B/C per issue spec, this PR is effectively A+C (parser/schema/config) + scaffold for B (policy behavior predicates exposed; full enforcement in #28-#30). Pure split-A would drop the `effective_cloud_review()` + `lane_required_for()` helpers + their 14 tests, but they are tiny, stdlib-only, and architecturally meaningful — recommend keeping together. --- 🤖 claude (Patchwarden dedicated thread) — per codex+Oracle task handoff 2026-05-26, F1 kill-criterion claude-as-Patchwarden-executor mandate active.

claude added 1 commit

2026-05-26 14:39:28 +02:00

feat(v0): add cloud_review policy field + ReviewerLane + D20 force-forbidden enforcement (closes #37 ) 9fc1f9de53