Add structured protocol schema validators

[wwind123]

Fixes #152

Summary

• add strict versioned structured response parsing helpers in protocol.py for v1 PR reviews, plan reviews, coder follow-ups, and plan revisions
• normalize PR/plan structured payloads back into existing parsed review dataclasses and add summary parity for legacy markdown parsing
• cover valid/invalid structured payloads, strict enums and item IDs, unknown-key rejection, and migration fallback behavior in unit tests

Testing

• python3 -m pytest tests/test_agent_loop.py
• python3 -m compileall src tests/test_agent_loop.py

[wwind123]

Implemented Phase 1 of the structured protocol layer in protocol.py.

Summary:

• added strict v1 structured JSON parsers/validators for pr_review, plan_review, coder_followup, and plan_revision
• kept legacy markdown parsing intact and added summary normalization parity for ParsedReview and ParsedPlanReview
• covered valid payloads, unsupported versions, kind mismatches, strict disposition enums, item ID validation, unknown-key rejection, and fallback-eligible malformed/schema-invalid payloads with unit tests

PR: #162

Tests: python3 -m pytest tests/test_agent_loop.py (passed); python3 -m compileall src tests/test_agent_loop.py (passed)

-- OpenAI Codex

[wwind123]

Prior unresolved plan item dispositions

• [item-14] summary field normalization → resolved. ParsedReview and ParsedPlanReview both now carry summary: str. Both parse_review() and parse_plan_review() call review_freeform_summary_text() on the legacy markdown path, and the orchestrator's resumed-record path also populates summary in both dataclasses. Parity is achieved.
• [item-15] item_id format validation → resolved. ITEM_ID_RE = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._-]*$") is defined and enforced in _expect_item_id(). The structured validators call this for every disposition entry and for addressed_items/remaining_items in coder_followup. The test test_parse_structured_pr_review_rejects_invalid_item_id_via_fallback confirms a space-containing ID correctly falls back to None.
• [item-16] Strict Disposition Enums → resolved. _parse_review_item_disposition_payload() uses an explicit allowed_statuses set ({"resolved", "blocking", allowed_same_status, "future"}) and hard-fails on any other string. No fuzzy regex normalization is applied. test_parse_structured_pr_review_requires_strict_disposition_enums confirms "still blocking" is rejected.
• [item-17] Schema Strictness (Unknown Keys) → resolved. _expect_exact_keys() enforces strict key checking at both the top-level and nested objects (e.g., human_requirements). Unknown keys at any level cause fallback to None. Tests confirm for both top-level unknowns and nested unknowns.

Blocking issues

validate_structured_plan_revision — summary validation is outside the try-except block.

protocol.py:830-836:

    except AgentLoopError:
        return None
    return StructuredPlanRevision(
        schema_version=1,
        kind="plan_revision",
        state="blocking",
        summary=_expect_non_empty_string(payload["summary"], context="plan_revision.summary"),  # ← uncaught
        plan_steps=plan_steps,
    )

_expect_non_empty_string(payload["summary"], ...) is called after the try: ... except AgentLoopError: return None block ends. If summary passes _expect_exact_keys (the key exists) but is an empty string or a non-string value, AgentLoopError propagates uncaught to the caller — violating the stated fallback contract that "valid JSON + supported version + invalid field value → return None."

This was confirmed with a live test: passing {"schema_version": 1, "kind": "plan_revision", "state": "blocking", "summary": "", "plan_steps": ["Step 1"]} raises AgentLoopError: plan_revision.summary must be a non-empty string. rather than returning None. The CI suite does not cover this case.

The fix is to move summary extraction inside the try block (matching the pattern used in validate_structured_coder_followup, parse_structured_pr_review, and parse_structured_plan_review):

    try:
        _expect_exact_keys(
            payload,
            context="plan_revision",
            required={"schema_version", "kind", "state", "summary", "plan_steps"},
        )
        state = _expect_non_empty_string(payload["state"], context="plan_revision.state")
        if state != "blocking":
            raise AgentLoopError("plan_revision.state must be `blocking`.")
        summary = _expect_non_empty_string(payload["summary"], context="plan_revision.summary")
        plan_steps = _expect_string_list(
            payload["plan_steps"],
            context="plan_revision.plan_steps",
            item_context="plan_revision.plan_steps",
        )
    except AgentLoopError:
        return None
    return StructuredPlanRevision(
        schema_version=1,
        kind="plan_revision",
        state="blocking",
        summary=summary,
        plan_steps=plan_steps,
    )

A corresponding test fixture (e.g., test_validate_structured_plan_revision_falls_back_on_empty_summary) should be added to maintain test coverage symmetry with the other validators.

-- Anthropic Claude

[wwind123]

Fixed the plan_revision fallback bug in validate_structured_plan_revision() by moving summary validation inside the try block, so invalid summary values now return None instead of leaking AgentLoopError.

Added a regression test covering the empty-summary case alongside the existing plan_revision validator coverage.

Tests: python3 -m pytest tests/test_agent_loop.py -k plan_revision (passed), python3 -m pytest tests/test_agent_loop.py (passed)

-- OpenAI Codex

[wwind123]

Prior unresolved item dispositions

• [item-1] Blocking issue from Anthropic Claude, round 1: ### Blocking issues -> resolved

-- Anthropic Claude

[wwind123]

Prior unresolved item dispositions

• [item-1] Blocking issue from Anthropic Claude, round 1: ### Blocking issues -> resolved

-- Google Gemini