diff --git a/.claude/skills/convergence-review/SKILL.md b/.claude/skills/convergence-review/SKILL.md new file mode 100644 index 0000000..f90f8cb --- /dev/null +++ b/.claude/skills/convergence-review/SKILL.md @@ -0,0 +1,196 @@ +--- +name: convergence-review +description: Dispatch parallel review perspectives and enforce convergence (zero CRITICAL + zero IMPORTANT). Supports gate types — design doc (8), macro plan (8), PR plan (10), cross-system PR plan (5), PR code (10), cross-system PR code (4), hypothesis design (5), hypothesis code (5), hypothesis FINDINGS (10). +argument-hint: [artifact-path] [--reviewer-model X] [--fixer-model Y] +--- + +# Convergence Review Dispatcher + +Dispatch parallel review perspectives for gate **$0** and enforce convergence. + +## Gate Types + +| Gate | Perspectives | Artifact | Prompts Source | +|------|-------------|----------|----------------| +| `design` | 8 | Design doc at `$1` | [design-prompts.md](design-prompts.md) Section A | +| `macro-plan` | 8 | Macro plan at `$1` | [design-prompts.md](design-prompts.md) Section B | +| `x-macro-plan` | 8 | Cross-system macro plan at `$1` | [design-prompts.md](design-prompts.md) Section D | +| `g-macro-plan` | 8 | Generalized macro plan at `$1` | [design-prompts.md](design-prompts.md) Section E | +| `pr-plan` | 10 | Micro plan at `$1` | [pr-prompts.md](pr-prompts.md) Section A | +| `x-pr-plan` | 5 | Cross-system micro plan at `$1` | [pr-prompts.md](pr-prompts.md) Section C | +| `pr-code` | 10 | Current git diff | [pr-prompts.md](pr-prompts.md) Section B | +| `x-pr-code` | 4 | Current git diff (cross-system) | [pr-prompts.md](pr-prompts.md) Section D | +| `h-design` | 5 | Design doc at `$1` | `hypothesis-experiment/review-prompts.md` Section A | +| `h-code` | 5 | `run.sh` + `analyze.py` at `$1` | `hypothesis-experiment/review-prompts.md` Section B | +| `h-findings` | 10 | FINDINGS.md at `$1` | `hypothesis-experiment/review-prompts.md` Section C | + +--- + +## Instructions + +### Step 1: Validate + +1. Validate gate type `$0`. If not one of the gates above, report error and stop. +2. For all gates except `pr-code`: validate that `$1` exists (file or directory as appropriate). + +### Step 2: Run Orchestrator + +Run the Python orchestrator from this skill's directory: + +``` +python .claude/skills/convergence-review/orchestrator.py $0 $1 [flags] +``` + +**Available flags:** +- `--reviewer-model MODEL` — Model for parallel reviewers (default: haiku, resolved via model-aliases.json). Also: `CONVERGENCE_REVIEWER_MODEL` env var. +- `--fixer-model MODEL` — Model for assessment and fix phases (default: opus). Also: `CONVERGENCE_FIXER_MODEL` env var. +- `--max-rounds N` — Maximum convergence rounds (default: 10) +- `--base-url URL` — LiteLLM proxy endpoint (default: from `LITELLM_BASE_URL` or `ANTHROPIC_BASE_URL` env) +- `--state-dir PATH` — State directory (default: `.claude/convergence-state/-`) +- `--log-level LEVEL` — DEBUG, INFO, WARNING, ERROR (default: WARNING) +- `--reviewer-timeout SECS` — Per-reviewer timeout (default: 300) +- `--assessor-timeout SECS` — Assessment phase timeout (default: 600) +- `--fixer-timeout SECS` — Fix phase (claude -p) timeout (default: 900) +- `--force` — Disable early stall detection (still limited by --max-rounds) +- `--stall-window N` — Rounds without improvement *and* no fixes applied before early exit (default: 5) +- `--human` — Human-readable one-line progress to stderr + +**API key:** Sourced from environment only (not CLI) — checks `LITELLM_API_KEY`, `ANTHROPIC_AUTH_TOKEN`, `ANTHROPIC_API_KEY` in order. + +### Step 3: Report Progress + +Read stdout line by line. Each line is a JSON event. Report progress to the user: + +| Event | What to report | +|-------|---------------| +| `summary_path` | **Report immediately**: "Review summary: ``" — the user can check this file at any time for current progress. | +| `round_start` | "Round N/M: dispatching K reviewers..." | +| `reviewer_done` | (accumulate silently) | +| `reviewer_error` | "Warning: perspective XX-N timed out/failed" | +| `reviews_complete` | "N raw findings from K/M perspectives" | +| `assessment_complete` | "Assessment: X CRITICAL, Y IMPORTANT (Z dismissed, W deduped)" | +| `fix_start` | "Fixing N findings..." | +| `fixes_applied` | "Fixed: X CRITICAL, Y IMPORTANT (Z skipped)" | +| `round_complete` | "Round N complete: [status]" | +| `converged` | "CONVERGED in N rounds (Xs total)" | +| `early_stall` | "Warning: findings not decreasing over N rounds. Use --force to override." | +| `stalled` | "STALLED after N rounds: X CRITICAL, Y IMPORTANT remaining" | +| `assessment_fallback` | "Warning: assessment API failed, using raw findings (may contain duplicates)" | +| `interrupted` | "Review interrupted by user." | +| `error` | Report the error message | + +**Example user-facing output:** +``` +Round 1/10: 10 reviewers dispatched... 27 raw findings → assessment: 2 CRITICAL, 1 IMPORTANT (8 dismissed, 11 deduped) → fixing... 3 fixed +Round 2/10: 10 reviewers dispatched... 3 raw findings → assessment: 0 CRITICAL, 0 IMPORTANT → CONVERGED +``` + +### Step 4: Interpret Exit Code + +| Code | Meaning | Action | +|------|---------|--------| +| **0** | Converged | Report success. Proceed to next workflow step. | +| **1** | Stalled (max rounds or early stall) | AskUserQuestion: "Convergence stalled. Options: increase round limit / use --force / accept current state / abort" | +| **2** | Setup error | Report the error message from the `error` event. | +| **130** | Interrupted | Report "Review interrupted by user." | + +--- + +## Three-Phase Round Architecture + +Each round executes three phases: + +1. **REVIEW** — All perspectives dispatched as parallel async API calls (OpenAI-compatible via LiteLLM). Fast, cheap, any model. +2. **ASSESS** — Single API call to a strong model that deduplicates, validates, re-prioritizes, and dismisses findings. Convergence decided on assessed counts, not raw. +3. **FIX** — `claude -p` subprocess for sequential, context-aware file edits using only assessed CRITICAL + IMPORTANT findings. + +The round loop is a deterministic Python `for` loop — structurally immune to LLM completion bias. + +--- + +## Convergence Logic + +Convergence is decided after the ASSESS phase of each round. The loop exits successfully when the count of **actionable** CRITICAL + IMPORTANT findings reaches zero. + +### What counts as "actionable" + +Not all assessed C+I findings block convergence. The assessor assigns each finding a `cross_round_status` based on a prior-round ledger, and two statuses are excluded from the actionable count: + +| `cross_round_status` | Blocks convergence? | Meaning | +|---|---|---| +| `novel` | **Yes** | New finding not seen in prior rounds | +| `regression` | **Yes** | A prior fix introduced this new problem | +| `recurring-skipped` | **Yes** | Fixer skipped this before, but the assessor says the skip rationale was inadequate | +| `recurring-fixed` | **No** | Same theme as a prior finding that was already fixed — assessor cannot identify a specific deficiency in the fix | +| `recurring-escalate` | **No** | Same theme flagged in 3+ rounds — a persistent design trade-off, not a fixable defect | + +### Cross-round ledger + +From round 2 onward, the assessor receives a ledger of prior-round findings and their fix dispositions (fixed/skipped + rationale). This lets it distinguish genuinely new issues from recurring themes that have already been addressed or that represent inherent trade-offs. + +- **Recent rounds** (last 3): full finding detail with fix actions +- **Older rounds**: one-line summary with counts only (to bound prompt length) + +### Persistent themes + +Findings classified as `recurring-escalate` are accumulated into a "Persistent Themes" section in the summary file. These represent design trade-offs the automated loop cannot resolve — they are surfaced for human review but do not block convergence. + +### Stall detection + +If `--force` is not set, the orchestrator monitors finding counts over a sliding window (`--stall-window`, default 5). If actionable C+I counts are not decreasing **and** no fixes are being applied across the window, the loop exits early with code 1. The `--force` flag disables this, running all rounds up to `--max-rounds`. + +--- + +## Severity Classification + +| Severity | Definition | Blocks? | +|----------|-----------|---------| +| **CRITICAL** | Must fix. Missing control, contradicted status, silent data loss, cross-document contradiction. | Yes | +| **IMPORTANT** | Should fix. Would reader be misled? Would conclusion change? | Yes | +| **SUGGESTION** | Cosmetic. Off-by-one citations, style, terminology. | No | + +--- + +## Integration with Other Skills + +### From design process +``` +/convergence-review design docs/plans/archive/.md +``` + +### From macro planning process +``` +/convergence-review macro-plan docs/plans/.md +``` + +### From PR workflow (Steps 2.5 and 4.5) +``` +/convergence-review pr-plan docs/plans/pr--plan.md +/convergence-review x-pr-plan docs/plans/pr--plan.md # cross-system transfer plans +/convergence-review pr-code # inference-sim code review +/convergence-review x-pr-code # cross-system (sim2real) code review +``` + +### From hypothesis-experiment skill (Steps 2, 5, and 8) +``` +/convergence-review h-design hypotheses/h-/design.md +/convergence-review h-code hypotheses/h-/ +/convergence-review h-findings hypotheses/h-/FINDINGS.md +``` + +--- + +## Summary File + +The orchestrator writes a human-readable `-review.md` next to the artifact being reviewed (e.g., `docs/plans/roofline-design-review.md`). For `pr-code` (git diff) or directory artifacts, it falls back to the state directory. A copy is always kept in the state directory. The file is updated after each round — the user can check it at any time for current progress. + +## State Directory + +Runtime state is stored at `.claude/convergence-state/-/` (gitignored). Contains per-round reviewer outputs, assessments, fix prompts, progress files, and a copy of the summary file. + +## Dependencies + +- Python 3.11+ +- `openai` pip package (async client) +- `claude` CLI (fix phase only) +- LiteLLM proxy endpoint (configurable) diff --git a/.claude/skills/convergence-review/assessor.py b/.claude/skills/convergence-review/assessor.py new file mode 100644 index 0000000..714b542 --- /dev/null +++ b/.claude/skills/convergence-review/assessor.py @@ -0,0 +1,394 @@ +"""Assessment phase: dedup, validate, re-prioritize findings via OpenAI-compatible API.""" + +from __future__ import annotations + +import asyncio +import json +import logging +import time +from pathlib import Path + +from openai import APIConnectionError, APIStatusError, APITimeoutError, AsyncOpenAI + +from models import ( + AssessedFinding, + Assessment, + DismissedFinding, + ReviewerOutput, +) + +logger = logging.getLogger("convergence.assessor") + +ASSESSMENT_SYSTEM_PROMPT = """\ +You are a senior technical reviewer performing quality control on review findings. + +You will receive: +1. The review gate type and what it expects +2. Raw findings from multiple reviewer perspectives +3. The artifact being reviewed + +Your job is to produce a consolidated, high-quality assessment by: + +1. **GATE FIT**: First, check whether the artifact is appropriate for this gate type. \ +If the artifact is fundamentally the wrong type (e.g., a Python design doc reviewed \ +with Go code review prompts, or a macro plan reviewed as a design doc), flag this as a \ +single CRITICAL finding with id "F-0" and description starting with "GATE MISMATCH:". \ +Dismiss all other findings as downstream of the mismatch. + +2. **DEDUP**: Merge findings from different perspectives that describe the same issue. \ +Cite all originating perspectives. + +3. **CROSS-ROUND DEDUP**: If a prior-round finding ledger is provided, check each \ +current finding for thematic overlap with prior findings. Assign a cross_round_status: + - "novel": Not seen in any prior round. + - "regression": A prior fix introduced this new problem. + - "recurring-fixed": Same theme as a prior finding that was fixed. \ +DISMISS unless you can state a specific deficiency in the prior fix. + - "recurring-skipped": Same theme the fixer already skipped. \ +Keep only if you can explain what the skip rationale missed. + - "recurring-escalate": Same theme flagged in 3+ prior rounds. \ +This is a persistent design trade-off. DISMISS from the fix loop. + If no ledger is provided, set cross_round_status to "novel" for all findings. + +4. **VALIDATE**: Check whether evidence supports each claim. Dismiss findings that are: + - Factually wrong or unsupported by the artifact content + - Asking for content that is out of scope for this artifact type \ +(e.g., demanding Go code in a design doc, or DES event definitions in a tooling spec) + - Applying standards from a different domain than the artifact addresses + +5. **RE-PRIORITIZE**: Independently assign severity using these definitions: + - CRITICAL: Must fix. Missing control, contradicted status, silent data loss, \ +cross-document contradiction. + - IMPORTANT: Should fix. Would reader be misled? Would conclusion change? + - SUGGESTION: Cosmetic. Off-by-one citations, style, terminology. + +6. **NOTE GAPS**: Flag perspectives that timed out or errored as uncovered. + +Respond with ONLY a JSON object matching this schema: +{ + "assessed_findings": [ + { + "id": "F-1", + "severity": "CRITICAL", + "original_severity": "CRITICAL", + "description": "...", + "location": "file:line or null", + "evidence": "...", + "source_perspectives": ["XX-1", "XX-2"], + "assessment_rationale": "...", + "cross_round_status": "novel", + "prior_finding_ref": null + } + ], + "dismissed": [ + { + "description": "...", + "source_perspectives": ["XX-3"], + "dismissal_reason": "..." + } + ], + "uncovered_perspectives": [], + "summary": { + "critical": 0, + "important": 0, + "suggestion": 0, + "dismissed": 0, + "deduplicated": 0 + } +} +""" + + +_GATE_DESCRIPTIONS = { + "design": "Design document review. Artifact should be a behavioral specification describing what modules do and why. Reviewers check DES foundations, module architecture, invariants, etc.", + "macro-plan": "Macro plan review. Artifact should be a multi-PR implementation plan with PR decomposition, dependency DAG, and module contracts.", + "x-macro-plan": "Cross-system macro plan review. Artifact should be a multi-PR implementation plan spanning multiple systems/repos, with cross-system dependency DAG, component contracts, and integration points.", + "g-macro-plan": "Generalized macro plan review. Artifact should be a multi-PR implementation plan for a generalized/reusable pipeline, with PR decomposition, component contracts, and extension points.", + "pr-plan": "PR micro plan review. Artifact should be a single-PR implementation plan with TDD tasks and behavioral contracts.", + "x-pr-plan": "Cross-system PR micro plan review. Artifact should be a single-PR implementation plan for transferring logic between two codebases, with signal mappings, schema contracts, and cross-system integration steps.", + "pr-code": "PR code review. Artifact is a git diff of code changes.", + "x-pr-code": "Cross-system PR code review. Artifact is a git diff of code changes for a cross-system transfer pipeline (sim2real). Review focuses on cross-system contract integrity, artifact consistency, prompt template quality, and Python CLI + Go harness code correctness.", + "h-design": "Hypothesis experiment design review. Artifact should be a hypothesis sentence, classification, and experiment design.", + "h-code": "Hypothesis experiment code review. Artifact should be run.sh and analyze.py scripts.", + "h-findings": "Hypothesis FINDINGS.md review. Artifact should be experimental results with analysis.", +} + + +def build_prior_ledger(state_dir: Path, current_round: int, max_detail_rounds: int = 3) -> str: + """Build a text ledger of prior-round findings and their fix dispositions. + + Reads assessment-round-{1..N-1}.json and fix-summary-round-{1..N-1}.json + from the state directory. Recent rounds get full detail; older rounds get + a one-line summary to bound prompt length. + + Returns empty string for round 1 (no prior rounds). + """ + if current_round <= 1: + return "" + + parts = ["## Prior Round Findings\n"] + detail_cutoff = max(1, current_round - max_detail_rounds) + + for r in range(1, current_round): + assessment_path = state_dir / f"assessment-round-{r}.json" + fix_path = state_dir / f"fix-summary-round-{r}.json" + + if not assessment_path.exists(): + continue + + try: + assessment_data = json.loads(assessment_path.read_text()) + except (json.JSONDecodeError, OSError) as e: + logger.warning("Could not read assessment-round-%d.json: %s", r, e) + continue + + findings = assessment_data.get("assessed_findings", []) + summary = assessment_data.get("summary", {}) + c_count = summary.get("critical", 0) + i_count = summary.get("important", 0) + + # Load fix dispositions + fix_map: dict[str, dict] = {} # finding_id -> fix/skip info + if fix_path.exists(): + try: + fix_data = json.loads(fix_path.read_text()) + for fa in fix_data.get("fixes_applied", []): + fid = fa.get("finding_id", "") + fix_map[fid] = {"status": "FIXED", "detail": fa.get("action", "")} + for fs in fix_data.get("fixes_skipped", []): + fid = fs.get("finding_id", "") + fix_map[fid] = {"status": "SKIPPED", "detail": fs.get("reason", "")} + except (json.JSONDecodeError, OSError) as e: + logger.warning("Could not read fix-summary-round-%d.json: %s", r, e) + + applied = sum(1 for v in fix_map.values() if v["status"] == "FIXED") + skipped = sum(1 for v in fix_map.values() if v["status"] == "SKIPPED") + + if r < detail_cutoff: + # Older rounds: one-line summary only + parts.append( + f"### Round {r} (summary): {c_count}C {i_count}I — " + f"{applied} fixed, {skipped} skipped\n" + ) + continue + + # Recent rounds: full detail + parts.append(f"### Round {r} ({c_count}C {i_count}I — {applied} fixed, {skipped} skipped)\n") + for f in findings: + if f.get("severity") not in ("CRITICAL", "IMPORTANT"): + continue + fid = f.get("id", "?") + sev = f.get("severity", "?") + desc = f.get("description", "")[:200] + disposition = fix_map.get(fid) + if disposition: + disp_detail = disposition["detail"][:150] + parts.append(f"- {fid} [{sev}] {desc} → {disposition['status']}: {disp_detail}\n") + else: + parts.append(f"- {fid} [{sev}] {desc} → (no fix data)\n") + + parts.append("") + return "\n".join(parts) + + +def _format_assessment_input( + reviewer_outputs: list[ReviewerOutput], artifact_content: str, + gate: str = "", prior_ledger: str = "", +) -> str: + """Format reviewer outputs and artifact for the assessment call.""" + parts = [] + + if gate: + gate_desc = _GATE_DESCRIPTIONS.get(gate, gate) + parts.append(f"## Gate Type: {gate}\n{gate_desc}\n") + + if prior_ledger: + parts.append(prior_ledger) + + parts.append("## Reviewer Findings\n") + + for output in reviewer_outputs: + if output.error: + parts.append( + f"### {output.perspective} — ERROR: {output.error}\n" + ) + continue + + parts.append(f"### {output.perspective}\n") + for f in output.findings: + location = f" (at {f.location})" if f.location else "" + evidence = f"\n Evidence: {f.evidence}" if f.evidence else "" + parts.append( + f"- [{f.severity}] {f.description}{location}{evidence}\n" + ) + if not output.findings: + parts.append("No findings.\n") + + parts.append("\n## Artifact Being Reviewed\n") + parts.append(artifact_content) + + return "\n".join(parts) + + +def _parse_assessment(text: str) -> Assessment: + """Parse assessment JSON response.""" + try: + data = json.loads(text) + except json.JSONDecodeError: + # Lenient: find JSON object + start = text.find("{") + end = text.rfind("}") + if start >= 0 and end > start: + data = json.loads(text[start : end + 1]) + else: + raise + + assessed = [] + for i, f in enumerate(data.get("assessed_findings", []), 1): + assessed.append( + AssessedFinding( + id=f.get("id", f"F-{i}"), + severity=f.get("severity", "SUGGESTION"), + original_severity=f.get("original_severity", f.get("severity", "SUGGESTION")), + description=f.get("description", ""), + source_perspectives=f.get("source_perspectives", []), + assessment_rationale=f.get("assessment_rationale", ""), + location=f.get("location"), + evidence=f.get("evidence"), + cross_round_status=f.get("cross_round_status", "novel"), + prior_finding_ref=f.get("prior_finding_ref"), + ) + ) + + dismissed = [] + for d in data.get("dismissed", []): + dismissed.append( + DismissedFinding( + description=d.get("description", ""), + source_perspectives=d.get("source_perspectives", []), + dismissal_reason=d.get("dismissal_reason", ""), + ) + ) + + summary = data.get("summary", {}) + # Recompute summary from parsed data to ensure accuracy + computed_summary = { + "critical": sum(1 for f in assessed if f.severity == "CRITICAL"), + "important": sum(1 for f in assessed if f.severity == "IMPORTANT"), + "suggestion": sum(1 for f in assessed if f.severity == "SUGGESTION"), + "dismissed": len(dismissed), + "deduplicated": summary.get("deduplicated", 0), + } + + return Assessment( + assessed_findings=assessed, + dismissed=dismissed, + uncovered_perspectives=data.get("uncovered_perspectives", []), + summary=computed_summary, + ) + + +async def assess_findings( + client: AsyncOpenAI, + model: str, + reviewer_outputs: list[ReviewerOutput], + artifact_content: str, + timeout: float = 600, + gate: str = "", + prior_ledger: str = "", +) -> Assessment: + """Run assessment phase: dedup, validate, re-prioritize findings. + + Retries once on failure. If both attempts fail, returns an Assessment + with raw findings passed through (no dedup/validation). + """ + input_text = _format_assessment_input( + reviewer_outputs, artifact_content, gate=gate, prior_ledger=prior_ledger, + ) + + for attempt in range(2): + try: + messages = [ + {"role": "system", "content": ASSESSMENT_SYSTEM_PROMPT}, + {"role": "user", "content": input_text}, + ] + if attempt == 1: + messages.append( + { + "role": "user", + "content": "Important: respond with ONLY a valid JSON object, " + "no markdown or commentary.", + } + ) + + start = time.monotonic() + response = await asyncio.wait_for( + client.chat.completions.create( + model=model, + messages=messages, + temperature=0.0, + ), + timeout=timeout, + ) + elapsed = time.monotonic() - start + + if not response.choices: + raise ValueError("Assessment API returned empty choices list") + text = response.choices[0].message.content or "" + logger.debug( + "Assessment raw response (%d chars): %s", + len(text), + text[:500], + ) + assessment = _parse_assessment(text) + + logger.info( + "Assessment complete in %.1fs: %d assessed, %d dismissed", + elapsed, + len(assessment.assessed_findings), + len(assessment.dismissed), + ) + return assessment + + except ( + APIConnectionError, APIStatusError, APITimeoutError, + asyncio.TimeoutError, json.JSONDecodeError, ValueError, + ) as e: + if attempt == 0: + logger.warning("Assessment attempt 1 of 2 failed: %s, retrying", e) + else: + logger.error("Assessment failed after 2 attempts: %s", e) + + # Fallback: pass raw findings through without assessment + logger.error( + "ASSESSMENT FALLBACK: passing %d raw findings through without " + "deduplication or validation. Results may contain duplicates and false positives.", + sum(len(o.findings) for o in reviewer_outputs), + ) + raw_findings = [] + for i, output in enumerate(reviewer_outputs): + for finding in output.findings: + raw_findings.append( + AssessedFinding( + id=f"F-{len(raw_findings) + 1}", + severity=finding.severity, + original_severity=finding.severity, + description=finding.description, + source_perspectives=[finding.perspective], + assessment_rationale="Assessment failed — raw finding passed through", + location=finding.location, + evidence=finding.evidence, + ) + ) + + return Assessment( + assessed_findings=raw_findings, + fallback=True, + summary={ + "critical": sum(1 for f in raw_findings if f.severity == "CRITICAL"), + "important": sum(1 for f in raw_findings if f.severity == "IMPORTANT"), + "suggestion": sum(1 for f in raw_findings if f.severity == "SUGGESTION"), + "dismissed": 0, + "deduplicated": 0, + }, + ) diff --git a/.claude/skills/convergence-review/design-prompts.md b/.claude/skills/convergence-review/design-prompts.md new file mode 100644 index 0000000..daf7002 --- /dev/null +++ b/.claude/skills/convergence-review/design-prompts.md @@ -0,0 +1,900 @@ +# Design & Macro Plan Review Perspective Prompts + +Reference file for the convergence-review skill. Contains exact prompts for design document review (8 perspectives) and macro plan review (8 perspectives). + +**Canonical sources:** `docs/contributing/design-process.md` (Design Review Perspectives section) and `docs/contributing/macro-planning.md` (Macro Plan Review Perspectives section). The perspective names and checklist items below are expanded versions of the process doc checklists, with full prompt context for agent dispatch. If perspective names diverge, the process docs are authoritative. + +**Related documents:** +- Design doc process: `docs/contributing/design-process.md` +- Design guidelines: `docs/contributing/templates/design-guidelines.md` +- Macro plan process: `docs/contributing/macro-planning.md` +- Macro plan template: `docs/contributing/templates/macro-plan.md` + +**Dispatch pattern:** Launch each perspective as a parallel Task agent: +``` +Task(subagent_type="general-purpose", model="haiku", run_in_background=True, + prompt="\n\n") + +--- + +## Section A: Design Document Review (8 perspectives) — `design` gate + +### DD-1: Motivation & Scoping + +``` +You are reviewing a BLIS design document. + +DESIGN DOCUMENT: + + +YOUR FOCUS: Motivation & Scoping +- Are the analysis questions clear, specific, and testable? +- Does every pipeline stage trace back to at least one analysis question? Are there stages that don't contribute to answering any question? +- Is the scope boundary crisp? For each "out of scope" item, is it clear why it's excluded and what breaks if it's accidentally included? +- Are the abstraction gaps between source and target systems enumerated? For each gap, is the handling strategy stated (bridge faithfully / approximate / punt)? +- For each "approximate" gap: what fidelity is lost and what's the risk? +- Could a simpler pipeline (fewer stages, less validation) answer the same analysis questions? What's the minimum viable pipeline? + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### DD-2: Pipeline Correctness + +``` +You are reviewing a BLIS design document. + +DESIGN DOCUMENT: + + +YOUR FOCUS: Pipeline Correctness +- Are stage triggers and ordering well-defined? Which stages are sequential vs parallelizable? +- Is mutable state (retry counts, error context, generated files) clearly owned by a specific stage or orchestrator? +- Are failure modes specified for each stage? Is it clear what happens on failure (halt, retry, degrade, flag for human)? +- Is the pipeline deterministic where it claims to be? Where non-determinism is introduced (e.g., LLM generation), is it acknowledged and acceptable? +- Are timeouts specified? What prevents the pipeline from hanging indefinitely on an external dependency? +- How do you verify the pipeline itself works correctly, independent of any specific algorithm being transferred? + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### DD-3: Stage Contract Completeness + +``` +You are reviewing a BLIS design document. + +DESIGN DOCUMENT: + + +YOUR FOCUS: Stage Contract Completeness +- For each pipeline stage: are the inputs, outputs, and side effects (files created, PRs opened, comments posted) fully specified? +- Is state ownership clear? For each piece of mutable state (retry count, error context, generated code, signal coverage report), which stage or orchestrator owns it? +- Are stage boundaries testable? Could someone implement one stage without reading the design of other stages, working only from the input/output contract? +- Are failure modes specified per stage? What happens with bad input, external service down, LLM generates garbage, or ambiguous results? +- For artifacts passed between stages (e.g., signal coverage report, mapped algorithm spec): is the format/schema specified well enough to implement against? +- How hard is it to extend the pipeline? (add a new validation suite, support a new signal type, target a new production system) + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### DD-4: Integration Fit + +``` +You are reviewing a BLIS design document. + +DESIGN DOCUMENT: + + +YOUR FOCUS: Integration Fit +- Does the generated artifact (e.g., scorer plugin) follow the target system's conventions? Is it additive (new files only) or does it require modifying existing code? +- Is the no-op default specified? Does the target system work normally when the pipeline hasn't been run or the generated artifact is disabled? +- Can the generated artifact be rolled back or disabled without side effects? +- What's the coupling between the pipeline and the target system's internal APIs? How does the pipeline handle API evolution in the target system? +- Does the generated code access data through proper interfaces, or does it reach into internal state? +- Can the pipeline run against one version of the target system while that system's development continues on HEAD? + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### DD-5: Prohibited Content + +``` +You are reviewing a BLIS design document. + +DESIGN DOCUMENT: + + +YOUR FOCUS: Prohibited Content +Design docs describe WHAT the pipeline does and WHY, never HOW it's implemented. This doc spans two external systems (source simulator, target production system) whose internals will evolve. Check for: +- Concrete type names from either system (e.g., specific language types, struct field names) — belong in the mapping artifact, not the design +- Specific API method names, metric paths, or config field names from the target system — will stale as the target evolves +- Architecture pattern names tied to current implementation (e.g., naming a specific design pattern) — misleading if the target system changes its plugin mechanism +- Internal signal/field names from the source system — will stale if the source renames internals + +Apply the staleness test: Would any content in this doc mislead if either the source or target system changes their implementation? Content that would stale should live in the mapping artifact (pinned to a commit), not the design doc. + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### DD-6: Trade-off Quality + +``` +You are reviewing a BLIS design document. + +DESIGN DOCUMENT: + + +YOUR FOCUS: Trade-off Quality +- Does every non-obvious decision have alternatives listed with rationale? +- For each decision: what breaks if it's wrong? What's the cost of reversal? +- For numeric thresholds and magic numbers: why this value and not another? What's the sensitivity — does ±10% change the outcome? +- Are assumptions about the target system's architecture accurate and current? What happens when those assumptions change? +- Are there assumptions about the abstraction gap between source and target systems that this design gets wrong? +- For validation choices (metrics, test sizes, acceptance criteria): are alternatives discussed? Could a different metric or test design answer the same question more cheaply or reliably? + +THRESHOLD GUIDANCE: Design docs operate at a higher abstraction level than implementation specs. A threshold is JUSTIFIED at design level if the doc specifies: (a) the metric being used, (b) a provisional default, and (c) an actionable calibration protocol (when, how, and what data to collect). Do NOT flag a threshold as CRITICAL merely because it lacks empirical calibration data — that data doesn't exist yet at design time. Only flag as CRITICAL if: the metric choice itself is wrong, the calibration plan is missing or unactionable, or the provisional default has no stated rationale at all. + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### DD-7: Validation Strategy + +``` +You are reviewing a BLIS design document. + +DESIGN DOCUMENT: + + +YOUR FOCUS: Validation Strategy +- Are the right metrics chosen for each validation concern? Are there better alternatives? +- What properties must hold after each pipeline stage? Are these stated explicitly or only implied? +- Are success criteria quantitative and testable (not "looks correct")? +- What would prove the pipeline itself is unreliable? If a deliberately broken algorithm is transferred, does the pipeline catch it? If a good algorithm fails validation, can you diagnose why? +- What would falsify the design's core assumption (that algorithms can transfer across the abstraction gap at all)? How would you detect that the gap is too wide? +- Is there a plan for calibrating the pipeline's detection capability (false positive rate, false negative rate) before trusting its verdicts? + +THRESHOLD GUIDANCE: Design docs operate at a higher abstraction level than implementation specs. A threshold is JUSTIFIED at design level if the doc specifies: (a) the metric being used, (b) a provisional default, and (c) an actionable calibration protocol (when, how, and what data to collect). Do NOT flag a threshold as CRITICAL merely because it lacks empirical calibration data — that data doesn't exist yet at design time. Only flag as CRITICAL if: the metric choice itself is wrong, the calibration plan is missing or unactionable, or the provisional default has no stated rationale at all. + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### DD-8: Staleness Resistance + +``` +You are reviewing a BLIS design document. + +DESIGN DOCUMENT: + + +YOUR FOCUS: Staleness Resistance +- Apply the staleness test to every section: Would this content mislead if either the source system or target system changes their implementation? +- Is content described behaviorally (what each stage does and why) rather than structurally (specific types, API names, config formats from either system)? +- Implementation details that will stale should live in a mapping artifact pinned to a commit, not in the design doc. Are there details here that belong there instead? +- Does the design account for the target system evolving while the pipeline runs? What if breaking changes land between code generation and benchmarking? +- Does the design work across different target environments (different cluster sizes, hardware, metric collection intervals) or is it implicitly tied to one specific setup? +- Where the doc references external artifacts (mapping files, templates, repos): are those references stable, or do they break if the artifact moves or is restructured? + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +--- + +## Section B: Macro Plan Review (8 perspectives) — `macro-plan` gate + +## Section C: Generalized Transfer Pipeline Design Review (8 perspectives) — `g-design` gate + +These perspectives are system-agnostic. They apply to any cross-system transfer pipeline that moves algorithms or logic from a SOURCE system to a TARGET system. The SOURCE and TARGET are placeholders — the reviewer should identify the actual systems from the design document. + +### GDD-1: Motivation & Scoping + +``` +You are a design reviewer + +DESIGN DOCUMENT: + + +YOUR FOCUS: Motivation & Scoping +- Are the analysis questions clear, specific, and testable? Could you write a pass/fail test for each one? +- Does every pipeline stage trace back to at least one analysis question? Are there stages that don't contribute to answering any question? +- Is the scope boundary crisp? For each "out of scope" item, is it clear why it's excluded and what breaks if it's accidentally included? +- Are the abstraction gaps between SOURCE and TARGET enumerated? For each gap, is the handling strategy stated (bridge faithfully / approximate / punt)? +- For each "approximate" gap: what fidelity is lost and what's the risk? +- Could a simpler pipeline (fewer stages, less validation) answer the same analysis questions? What's the minimum viable pipeline? + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### GDD-2: Pipeline Correctness + +``` +You are a design reviewer + +DESIGN DOCUMENT: + + +YOUR FOCUS: Pipeline Correctness +- Are stage triggers and ordering well-defined? Which stages are sequential vs parallelizable? +- Is mutable state (retry counts, error context, generated files, intermediate artifacts) clearly owned by a specific stage or orchestrator? +- Are failure modes specified for each stage? Is it clear what happens on failure (halt, retry, degrade, flag for human)? +- Is the pipeline deterministic where it claims to be? Where non-determinism is introduced (e.g., LLM generation, external API calls), is it acknowledged and acceptable? +- Are timeouts specified? What prevents the pipeline from hanging indefinitely on an external dependency? +- How do you verify the pipeline itself works correctly, independent of any specific algorithm being transferred? + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### GDD-3: Stage Contract Completeness + +``` +You are a design reviewer + +DESIGN DOCUMENT: + + +YOUR FOCUS: Stage Contract Completeness +- For each pipeline stage: are the inputs, outputs, and side effects (files created, PRs opened, comments posted, deployments triggered) fully specified? +- Is state ownership clear? For each piece of mutable state, which stage or orchestrator owns it? +- Are stage boundaries testable? Could someone implement one stage without reading the design of other stages, working only from the input/output contract? +- Are failure modes specified per stage? What happens with bad input, external service down, generation produces garbage, or ambiguous results? +- For artifacts passed between stages: is the format/schema specified well enough to implement against? +- How hard is it to extend the pipeline? (add a new validation suite, support a new signal/feature type, target a new production system) + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### GDD-4: Integration Fit + +``` +You are a design reviewer + +DESIGN DOCUMENT: + + +YOUR FOCUS: Integration Fit +- Does the generated artifact follow the TARGET system's conventions? Is it additive (new files only) or does it require modifying existing TARGET code? +- Is the no-op default specified? Does the TARGET system work normally when the pipeline hasn't been run or the generated artifact is disabled? +- Can the generated artifact be rolled back or disabled without side effects? +- What's the coupling between the pipeline and the TARGET system's internal APIs? How does the pipeline handle API evolution in the TARGET? +- Does the generated code access data through proper TARGET interfaces, or does it reach into internal state? +- Can the pipeline run against one version of the TARGET while that system's development continues on HEAD? +- Does the pipeline similarly respect the SOURCE system's boundaries, or does it depend on SOURCE internals that may change? + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### GDD-5: Prohibited Content + +``` +You are a design reviewer + +DESIGN DOCUMENT: + + +YOUR FOCUS: Prohibited Content +Design docs describe WHAT the pipeline does and WHY, never HOW it's implemented. This doc spans two external systems whose internals will evolve independently. Check for: +- Concrete type names, struct definitions, or field names from either SOURCE or TARGET — these belong in a mapping artifact, not the design +- Specific API method names, metric paths, or config field names from the TARGET — will stale as the TARGET evolves +- Architecture pattern names tied to current implementation — misleading if either system changes its architecture +- Internal signal/field names from the SOURCE — will stale if the SOURCE renames internals + +Apply the staleness test: Would any content in this doc mislead if either the SOURCE or TARGET system changes their implementation? Content that would stale should live in a versioned mapping artifact pinned to a commit, not the design doc. + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### GDD-6: Trade-off Quality + +``` +You are a design reviewer + +DESIGN DOCUMENT: + + +YOUR FOCUS: Trade-off Quality +- Does every non-obvious decision have alternatives listed with rationale? +- For each decision: what breaks if it's wrong? What's the cost of reversal? +- For numeric thresholds and magic numbers: why this value and not another? What's the sensitivity — does ±10% change the outcome? +- Are assumptions about the SOURCE-to-TARGET abstraction gap accurate? What happens when those assumptions change? +- Are there assumptions about either system that this design gets wrong? +- For validation choices (metrics, test sizes, acceptance criteria): are alternatives discussed? Could a different metric or test design answer the same question more cheaply or reliably? + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### GDD-7: Validation Strategy + +``` +You are a design reviewer + +DESIGN DOCUMENT: + + +YOUR FOCUS: Validation Strategy +- Are equivalence metrics and acceptance thresholds justified? What evidence supports the chosen values? Has anyone calibrated them against known-good and known-bad transfers? +- What properties must hold after each pipeline stage? Are these stated explicitly or only implied? +- Are success criteria quantitative and testable (not "looks correct")? For each threshold: what's the sensitivity — does ±10% change the outcome? +- What would prove the pipeline itself is unreliable? If a deliberately broken algorithm is transferred, does the pipeline catch it? If a good algorithm fails validation, can you diagnose why? +- What would falsify the design's core assumption — that algorithms can transfer across the abstraction gap at all? How would you detect that the gap is too wide? +- Is there a plan for calibrating the pipeline's detection capability (false positive rate, false negative rate) before trusting its verdicts? + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### GDD-8: Staleness Resistance + +``` +You are a design reviewer + +DESIGN DOCUMENT: + + +YOUR FOCUS: Staleness Resistance +- Apply the staleness test to every section: Would this content mislead if either the SOURCE or TARGET system changes their implementation? +- Is content described behaviorally (what each stage does and why) rather than structurally (specific types, API names, config formats from either system)? +- Implementation details that will stale should live in a versioned mapping artifact pinned to a commit, not the design doc. Are there details here that belong there instead? +- Does the design account for the TARGET system evolving while the pipeline runs? What if breaking changes land between code generation and validation? +- Does the design work across different TARGET environments (different cluster sizes, hardware, configurations) or is it implicitly tied to one specific setup? +- Where the doc references external artifacts (mapping files, templates, repos): are those references stable, or do they break if the artifact moves or is restructured? + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +--- + +### MP-1: Objective Clarity + +``` +You are a BLIS macro-plan reviewer. + +DESIGN DOCUMENT: + + +YOUR FOCUS: Objective Clarity (macro-plan template Phase 1) +- Are 3-7 crisp objectives defined? +- Are non-goals explicitly listed? +- Is the model scoping table present (modeled / simplified / omitted / justification)? +- Are analysis questions specific enough to drive component selection? +- For each "Simplified" entry, is the lost real-system behavior documented? + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### MP-2: Concept Model Quality + +``` +You are a BLIS macro-plan reviewer. + +DESIGN DOCUMENT: + + +YOUR FOCUS: Concept Model Quality (macro-plan template Phase 2) +- Is the concept model under 80 lines? +- Does every building block have all 6 module contract aspects? (observes, controls, owns, invariants, events, extension friction) +- Is real-system correspondence documented (llm-d / vLLM / SGLang mapping table)? +- Is the state ownership map complete (exactly one owner per mutable state)? +- Are behavioral contracts testable with mocks? (Enables parallel development) + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### MP-3: PR Decomposition + +``` +You are a BLIS macro-plan reviewer. + +DESIGN DOCUMENT: + + +YOUR FOCUS: PR Decomposition Quality (macro-plan template Phase 6) +- Is each PR independently mergeable? (No PR requires another PR's uncommitted code) +- Does the dependency DAG have no cycles? +- Can module contracts be tested with mocks (parallel development enabled)? +- Does each PR identify its extension type? (policy template, subsystem module, backend swap, tier composition) +- Is each PR exercisable immediately after merge? (Via CLI or tests demonstrating new behavior) +- Are parallelizable workstreams identified? Is safe parallelism maximized? +- Are interface freeze points marked? (Which PRs unlock parallel development?) + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### MP-4: Abstraction Level + +``` +You are a BLIS macro-plan reviewer. + +DESIGN DOCUMENT: + + +YOUR FOCUS: Abstraction Level Compliance (macro-plan template Abstraction Level Rule) +The macro plan describes WHAT to build and in WHAT ORDER, not HOW. Check: +- Zero Go code in Sections A-F and H-K (only Section G may have frozen interface signatures)? +- Are all pre-freeze interfaces described behaviorally, not as Go code? +- Is every code snippet a FACT about merged code, not an ASPIRATION about unwritten code? +- Are module contracts using the template from Phase 2, not Go structs? +- Check the concept model (<80 lines?). Does it use behavioral descriptions or implementation details? + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### MP-5: Risk Register + +``` +You are a BLIS macro-plan reviewer. + +DESIGN DOCUMENT: + + +YOUR FOCUS: Risk Register Completeness (macro-plan template Phase 3) +For each entry, verify: +- DECISION: Is the choice clearly stated? +- ASSUMPTION: What must be true for this to work? +- VALIDATION: How to test cheaply? (Mock study, prototype, analysis, spike) +- COST IF WRONG: How many PRs of rework? (Count affected PRs) +- GATE: When must validation complete? (Before which PR) + +MANDATORY VALIDATION RULE: If cost-of-being-wrong >= 3 PRs, validation is MANDATORY. +- Are success criteria measurable (not "looks good")? +- Are abort plans specified (what changes if validation fails)? + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### MP-6: Cross-Cutting Infrastructure + +``` +You are a BLIS macro-plan reviewer. + +DESIGN DOCUMENT: + + +YOUR FOCUS: Cross-Cutting Infrastructure (macro-plan template Phase 5) +Verify Phase 5 is complete: +1. Shared Test Infrastructure — which PR creates it? Which consume it? Are invariant tests planned? +2. Documentation Maintenance — CLAUDE.md update triggers? README updates? Design guidelines updates? +3. CI Pipeline — new test packages added? New linter rules? Performance benchmarks? +4. Dependency Management — new external deps justified? Version pinning? +5. Interface Freeze Schedule — which PR freezes which interface? What must be validated before freezing? + +Check that NO item is left as "address when needed." Every cross-cutting concern must be assigned to a specific PR. + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### MP-7: Extension Friction + +``` +You are a BLIS macro-plan reviewer. + +DESIGN DOCUMENT: + + +YOUR FOCUS: Extension Friction (design guidelines Section 4.5) +- For each new module boundary, is the touch-point count for adding one more variant specified? +- Are touch-point counts within reference targets from design guidelines Section 4.5? +- If friction exceeds targets, is this acknowledged and justified? +- Does each building block map correctly to real inference system components? (llm-d, vLLM, SGLang) +- Are batching, KV cache, scheduling, and routing semantics accurate? + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### MP-8: Design Bug Prevention + +``` +You are a BLIS macro-plan reviewer. + +DESIGN DOCUMENT: + + +YOUR FOCUS: Design Bug Prevention (macro-plan template Phase 8) +Check that the plan prevents these failure modes: + +General: +- Scaffolding creep prevented (every struct/method/flag exercised by end of introducing PR)? +- Documentation drift prevented (CLAUDE.md updated in the same PR that causes the change)? +- Test infrastructure duplication prevented (shared packages created early)? +- Golden dataset staleness prevented (regeneration steps included)? +- Interface over-specification prevented (frozen only after 2+ implementations designed)? + +DES-specific: +- Type catalog trap prevented (behavioral descriptions, not Go structs)? +- Fidelity for its own sake prevented (every component traces to an analysis question)? +- Golden without invariant prevented (companion invariant tests for golden tests)? +- Exogenous/endogenous mixing prevented (separable workload inputs)? + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +--- + +## Section D: Cross-System Macro Plan Review (8 perspectives) — `x-macro-plan` gate + +These perspectives apply to macro plans written using the `macro-plan-cross-system.md` template — plans for pipelines that span multiple external systems and repositories. + +### CMP-1: Objective Clarity + +``` +You are a macro-plan reviewer. + +DESIGN DOCUMENT: + + +YOUR FOCUS: Objective Clarity (cross-system macro-plan template Phase 1) +- Are 3-7 crisp objectives defined? +- Are non-goals explicitly listed? +- Is the pipeline scoping table present (v1 / deferred / out of scope / justification)? +- Are external system version constraints specified? +- Are rollback/disable guarantees specified for each target system? +- Are analysis questions (from the design doc) referenced and traceable to pipeline components? + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### CMP-2: Component Model Quality + +``` +You are a BLIS macro-plan reviewer. + +DESIGN DOCUMENT: + + +YOUR FOCUS: Component Model Quality (cross-system macro-plan template Phase 2) +- Is the component model under 100 lines? +- Does every component have a full contract? (type, inputs, outputs, side effects, invariants, failure modes, external dependencies) +- Is the external system map present? (which components touch which external systems) +- Is data flow between stages clearly specified? (artifacts passed between components) +- Are user interaction points identified? (where the pipeline halts for human input) +- Is state ownership clear? (which component owns each artifact and mutable state) +- Are failure modes specified per component? (halt, retry, degrade) + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### CMP-3: PR Decomposition + +``` +You are a BLIS macro-plan reviewer. + +DESIGN DOCUMENT: + + +YOUR FOCUS: PR Decomposition Quality (cross-system macro-plan template Phase 6) +- Does each PR specify its target repository? +- Does each PR specify its PR type? (infrastructure, artifact, pipeline-stage, integration, validation, orchestration) +- Is each PR independently mergeable within its target repo? +- Does the dependency DAG have no cycles (including cross-repo edges)? +- Is each PR exercisable immediately after merge? +- Are cross-repo ordering constraints explicit? (which PRs in repo A must merge before PRs in repo B) +- Are parallelizable workstreams identified? Is safe parallelism maximized? + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### CMP-4: Abstraction Level + +``` +You are a BLIS macro-plan reviewer. + +DESIGN DOCUMENT: + + +YOUR FOCUS: Abstraction Level Compliance (cross-system macro-plan template Abstraction Level Rule) +The macro plan describes WHAT to build and in WHAT ORDER, not HOW. Check: +- Zero implementation code in Sections A-F and H-K? (No Go, Python, or other language code except in Section G for stable APIs) +- Are all component contracts described behaviorally, not as code? +- Is every code snippet a FACT about existing/stable systems, not an ASPIRATION about unwritten code? +- Are external system internals (types, struct fields, method bodies) absent? (Describe behaviorally instead) +- Check the component model (<100 lines?). Does it use behavioral descriptions or implementation details? + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### CMP-5: Risk Register + +``` +You are a BLIS macro-plan reviewer. + +DESIGN DOCUMENT: + + +YOUR FOCUS: Risk Register Completeness (cross-system macro-plan template Phase 3) +For each entry, verify: +- DECISION: Is the choice clearly stated? +- ASSUMPTION: What must be true for this to work? +- VALIDATION: How to test cheaply? +- COST IF WRONG: How many PRs of rework? +- GATE: When must validation complete? + +CROSS-SYSTEM RISK CATEGORIES — verify each is addressed: +- API drift (target system changes API) +- Version mismatch (pipeline vs. cluster versions) +- Schema evolution (artifact format changes) +- External availability (test infra down) +- Distributed partial failure (multi-repo inconsistency) +- Mock fidelity (test mocks diverge from reality) + +MANDATORY VALIDATION RULE: If cost-of-being-wrong >= 3 PRs, validation is MANDATORY. + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### CMP-6: Cross-Cutting Infrastructure + +``` +You are a BLIS macro-plan reviewer. + +DESIGN DOCUMENT: + + +YOUR FOCUS: Cross-Cutting Infrastructure (cross-system macro-plan template Phase 5) +Verify Phase 5 is complete: +1. Shared Test Infrastructure — which PR creates it? External system mocking strategy? +2. Documentation Maintenance — CLAUDE.md, README, artifact schema docs? +3. CI Pipeline — integration tests need external access? How handled? +4. Dependency Management — external system version pinning? Artifact compatibility? +5. Cross-Repo Coordination — PR ordering across repos? Branch naming? Review ownership? +6. Artifact Lifecycle — which artifacts are prerequisites? Who creates/maintains them? Version scheme? + +Check that NO item is left as "address when needed." Every cross-cutting concern must be assigned to a specific PR. + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### CMP-7: Extension & Integration Friction + +``` +You are a BLIS macro-plan reviewer. + +DESIGN DOCUMENT: + + +YOUR FOCUS: Extension & Integration Friction (cross-system macro-plan template Phase 2 Extension Points) +- For each extension point, is the cost of extension specified? (files to change, tests to add) +- How hard is it to add a new target system? (new production system to transfer algorithms to) +- How hard is it to add a new validation suite? (new equivalence test type) +- How hard is it to add a new signal type? (new category of signals to map) +- Are integration points with external systems loosely coupled? (Can the pipeline survive API changes with artifact updates only, or does pipeline code need changing?) +- Is there a clear boundary between what lives in the host repo vs. what lives in mapping artifacts? + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### CMP-8: Design Bug Prevention + +``` +You are a BLIS macro-plan reviewer. + +DESIGN DOCUMENT: + + +YOUR FOCUS: Design Bug Prevention (cross-system macro-plan template Phase 8) +Check that the plan prevents these failure modes: + +General: +- Scaffolding creep prevented (every file/function exercised by end of introducing PR)? +- Documentation drift prevented (docs updated in same PR that causes change)? +- Test infrastructure duplication prevented (shared packages created early)? + +Cross-system-specific: +- API contract drift prevented (version-pinned artifacts + staleness checks)? +- Mock divergence prevented (mocks derived from real artifacts, not hand-written)? +- Distributed partial failure handled (cleanup procedures for multi-repo inconsistency)? +- Artifact staleness prevented (compile + smoke test + version distance checks)? +- Cross-repo merge ordering enforced (dependency DAG includes cross-repo edges)? +- External system evolution handled (what happens when target system releases a new version)? + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +--- + +## Section E: Generalized Macro Plan Review (8 perspectives) — `g-macro-plan` gate + +These perspectives are system-agnostic. They apply to any macro plan for any project — single-repo or multi-repo, any language, any domain. They generalize the `macro-plan` gate (Section B) the same way `g-design` (Section C) generalizes `design` (Section A). + +### GMP-1: Objective Clarity + +``` +You are a BLIS macro-plan reviewer. + +DESIGN DOCUMENT: + + +YOUR FOCUS: Objective Clarity +- Are 3-7 crisp objectives defined? Is each objective testable (you could write a pass/fail check)? +- Are non-goals explicitly listed? For each, is it clear why it's excluded? +- Is a scoping table present that classifies capabilities as in-scope, deferred, or out of scope — with justification for each? +- Are the plan's objectives traceable to a design document or requirements source? Can every component trace back to an objective? +- Are version/compatibility constraints specified for external dependencies? +- Are rollback/disable guarantees specified? If the delivered code breaks something, how is it reverted? + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### GMP-2: Component/Concept Model Quality + +``` +You are a BLIS macro-plan reviewer. + +DESIGN DOCUMENT: + + +YOUR FOCUS: Component/Concept Model Quality +- Is the component or concept model concise (under 80-100 lines)? +- Does every component have a clear contract? At minimum: responsibility, inputs, outputs, side effects, invariants, and failure modes. +- Is data flow between components clearly specified? (What artifacts or state crosses each boundary?) +- Is state ownership unambiguous? (Exactly one component owns each piece of mutable state.) +- Are failure modes specified per component? (halt, retry, degrade, flag for human) +- Are extension points identified? (Where do new capabilities plug in, and what's the cost?) +- Are behavioral contracts testable with mocks? (Enables parallel development of components.) + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### GMP-3: PR Decomposition + +``` +You are a BLIS macro-plan reviewer. + +DESIGN DOCUMENT: + + +YOUR FOCUS: PR Decomposition Quality +- Is each PR independently mergeable? (No PR requires another PR's uncommitted code.) +- Does the dependency DAG have no cycles? +- Is each PR exercisable immediately after merge? (Via CLI, tests, or demonstrable new behavior.) +- Does each PR deliver one cohesive change? (Not a grab-bag of unrelated modifications.) +- Are parallelizable workstreams identified? Is safe parallelism maximized? +- Are interface boundaries between PRs clear? (Which PRs define interfaces that later PRs consume?) +- Does each PR specify its type? (infrastructure, feature, refactor, test, documentation, etc.) + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### GMP-4: Abstraction Level + +``` +You are a BLIS macro-plan reviewer. + +DESIGN DOCUMENT: + + +YOUR FOCUS: Abstraction Level Compliance +The macro plan describes WHAT to build and in WHAT ORDER, not HOW to implement each piece. Check: +- Zero implementation code (any language) in the plan body? (Only Section G / Stable API Reference may contain code, and only for already-frozen interfaces.) +- Are all component contracts described behaviorally, not as type definitions or code signatures? +- Is every code snippet a FACT about existing/stable systems, not an ASPIRATION about unwritten code? +- Does the component model use behavioral descriptions rather than implementation details? +- Are internal details of external systems absent? (Referenced behaviorally, not structurally.) + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### GMP-5: Risk Register + +``` +You are a BLIS macro-plan reviewer. + +DESIGN DOCUMENT: + + +YOUR FOCUS: Risk Register Completeness +For each entry, verify all five columns: +- DECISION: Is the choice clearly stated? +- ASSUMPTION: What must be true for this to work? +- VALIDATION: How to test cheaply? (Mock, prototype, spike, analysis) +- COST IF WRONG: How many PRs of rework? +- GATE: When must validation complete? (Before which PR) + +MANDATORY VALIDATION RULE: If cost-of-being-wrong >= 3 PRs, validation is MANDATORY. +- Are success criteria measurable (not "looks good")? +- Are abort plans specified (what changes if validation fails)? +- Are there obvious risks missing from the register? (External dependency changes, performance cliffs, security concerns, data migration issues) + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### GMP-6: Cross-Cutting Infrastructure + +``` +You are a BLIS macro-plan reviewer. + +DESIGN DOCUMENT: + + +YOUR FOCUS: Cross-Cutting Infrastructure +Verify these cross-cutting concerns are fully addressed: +1. Shared Test Infrastructure — which PR creates it? Which PRs consume it? Are test helpers, fixtures, and mocks planned? +2. Documentation Maintenance — when are CLAUDE.md, README, and user-facing docs updated? Is it the same PR that causes the change? +3. CI Pipeline — new test packages? New linter rules? Performance benchmarks? Integration test requirements? +4. Dependency Management — new external dependencies justified? Version pinning strategy? +5. Configuration — new config surfaces? Migration from old config? + +Check that NO item is left as "address when needed." Every cross-cutting concern must be assigned to a specific PR. + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### GMP-7: Extension Friction + +``` +You are a BLIS macro-plan reviewer. + +DESIGN DOCUMENT: + + +YOUR FOCUS: Extension Friction +- For each component boundary, is the cost of adding one more variant specified? (files to change, tests to add) +- Are touch-point counts reasonable? Adding a variant of an existing component should require changing a small, bounded set of files. +- If extension friction is high, is this acknowledged and justified? +- Are integration points between components loosely coupled? (Can one component change without cascading updates?) +- Is there a clear separation between stable interfaces and implementation details? +- Could a new contributor extend the system by reading only the component contracts, without understanding the full codebase? + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### GMP-8: Design Bug Prevention + +``` +You are a BLIS macro-plan reviewer. + +DESIGN DOCUMENT: + + +YOUR FOCUS: Design Bug Prevention +Check that the plan prevents these common failure modes: + +General: +- Scaffolding creep prevented? (Every file, function, and config exercised by end of introducing PR — no dead code "for later".) +- Documentation drift prevented? (Docs updated in the same PR that causes the change.) +- Test infrastructure duplication prevented? (Shared test packages created early and reused.) +- Premature abstraction prevented? (Abstractions introduced only after 2+ concrete uses exist.) + +Structural: +- Regression surfaces identified? (Which existing tests must keep passing?) +- State migration risks addressed? (If data formats change across PRs, is migration planned?) +- Invariants enumerated? (What must ALWAYS hold? What must NEVER happen?) +- Interface over-specification prevented? (Interfaces frozen only after sufficient design confidence.) + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` diff --git a/.claude/skills/convergence-review/fixer.py b/.claude/skills/convergence-review/fixer.py new file mode 100644 index 0000000..a777cad --- /dev/null +++ b/.claude/skills/convergence-review/fixer.py @@ -0,0 +1,395 @@ +"""Fix phase: claude -p subprocess for applying assessed findings.""" + +from __future__ import annotations + +import json +import logging +import os +import shutil +import subprocess +import time +from pathlib import Path + +from openai import OpenAI + +from models import AssessedFinding, FixResult +from progress import write_json_file + +logger = logging.getLogger("convergence.fixer") + +_FIX_PROMPT_TEMPLATE = """\ +You are fixing issues found during a convergence review. Apply each fix carefully. + +## Instructions + +1. Read the current state of each file mentioned in the findings below. +2. Fix all CRITICAL findings first, then IMPORTANT. +3. After each fix, verify the surrounding context still makes sense. +4. Do NOT re-review, dispatch evaluators, or add new findings. +5. When done, write a fix summary JSON to: {summary_path} + +## Fix Summary Schema + +Write this exact JSON structure to the summary path: +```json +{{ + "fixes_applied": [ + {{"finding_id": "F-1", "severity": "CRITICAL", "file": "path/to/file", "action": "Description of fix"}} + ], + "fixes_skipped": [ + {{"finding_id": "F-3", "severity": "IMPORTANT", "reason": "Why this was skipped"}} + ] +}} +``` + +## Findings to Fix + +{findings_text} + +## Artifact Path + +{artifact_path} +""" + + +def _format_findings(findings: list[AssessedFinding]) -> str: + """Format assessed findings for the fix prompt.""" + parts = [] + # Sort: CRITICAL first, then IMPORTANT + severity_order = {"CRITICAL": 0, "IMPORTANT": 1, "SUGGESTION": 2} + sorted_findings = sorted(findings, key=lambda f: severity_order.get(f.severity, 9)) + + for f in sorted_findings: + location = f" at {f.location}" if f.location else "" + evidence = f"\n Evidence: {f.evidence}" if f.evidence else "" + parts.append( + f"### {f.id} [{f.severity}]{location}\n" + f" {f.description}{evidence}\n" + f" Sources: {', '.join(f.source_perspectives)}\n" + f" Rationale: {f.assessment_rationale}" + ) + return "\n\n".join(parts) + + +def _parse_claude_output(stdout: str) -> FixResult | None: + """Try to extract fix results from claude -p --output-format json stdout. + + The claude JSON output contains a 'result' field with the assistant's + final text response. We look for fix/edit actions in that text. + """ + if not stdout or not stdout.strip(): + return None + + try: + data = json.loads(stdout) + except json.JSONDecodeError: + return None + + # claude --output-format json wraps output in {"type":"result","result":"..."} + result_text = data.get("result", "") + if not result_text: + return None + + # Try to find a JSON fix summary embedded in the result text. + # Use json.JSONDecoder.raw_decode from the opening brace before "fixes_applied". + marker = result_text.find('"fixes_applied"') + if marker >= 0: + brace_start = result_text.rfind("{", 0, marker) + if brace_start >= 0: + decoder = json.JSONDecoder() + try: + summary, _ = decoder.raw_decode(result_text, brace_start) + return FixResult( + fixes_applied=summary.get("fixes_applied", []), + fixes_skipped=summary.get("fixes_skipped", []), + ) + except json.JSONDecodeError: + logger.warning( + "Found 'fixes_applied' marker at position %d but could not " + "parse valid JSON from claude -p output (%d chars)", + marker, len(result_text), + ) + + # If no embedded JSON, count Edit/Write tool uses as proxy for fixes applied + # This is a rough heuristic but better than returning empty + edits = result_text.lower().count("edit") + result_text.lower().count("wrote") + if edits > 0: + logger.info( + "claude -p result mentions ~%d edit/write actions but no structured summary", + edits, + ) + + return None + + +_FIX_EXTRACTION_PROMPT = """\ +The following is the raw JSON output from a `claude -p` subprocess that was asked to fix \ +review findings in a file. The subprocess applied edits but its "result" field does not \ +contain a structured fix summary. Extract what was actually done. + +Examine the full JSON output (including tool use history, permission denials, and the \ +result text) to determine which findings were fixed, which were skipped, and why. + +You MUST respond with ONLY a JSON object — no markdown, no commentary: + +```json +{ + "fixes_applied": [ + {"finding_id": "F-1", "severity": "CRITICAL", "file": "path/to/file", "action": "Description of what was fixed"} + ], + "fixes_skipped": [ + {"finding_id": "F-3", "severity": "IMPORTANT", "reason": "Why this was skipped"} + ] +} +``` + +If you cannot determine what happened, return: {"fixes_applied": [], "fixes_skipped": []} + +--- BEGIN CLAUDE OUTPUT --- +""" + + +def _extract_fixes_fallback( + stdout: str, + base_url: str | None = None, + api_key: str | None = None, + model: str | None = None, + timeout: float = 120, +) -> FixResult | None: + """Fallback: ask an LLM to extract fix results from unparseable claude -p output.""" + base_url = base_url or os.environ.get("LITELLM_BASE_URL") or os.environ.get("ANTHROPIC_BASE_URL") + api_key = ( + api_key + or os.environ.get("LITELLM_API_KEY") + or os.environ.get("ANTHROPIC_AUTH_TOKEN") + or os.environ.get("ANTHROPIC_API_KEY") + or "" + ).strip() + + if not base_url or not api_key: + logger.warning("Cannot run fix extraction fallback: no API credentials") + return None + + # Truncate stdout to avoid blowing context — keep first and last portions + max_chars = 60_000 + if len(stdout) > max_chars: + half = max_chars // 2 + truncated = stdout[:half] + "\n\n[... truncated ...]\n\n" + stdout[-half:] + else: + truncated = stdout + + prompt = _FIX_EXTRACTION_PROMPT + truncated + "\n--- END CLAUDE OUTPUT ---" + + try: + client = OpenAI(base_url=base_url, api_key=api_key) + response = client.chat.completions.create( + model=model or "aws/claude-haiku-4-5", + messages=[{"role": "user", "content": prompt}], + temperature=0.0, + timeout=timeout, + ) + if not response.choices: + return None + text = response.choices[0].message.content or "" + + # Parse JSON from response + try: + data = json.loads(text) + except json.JSONDecodeError: + start = text.find("{") + end = text.rfind("}") + if start >= 0 and end > start: + data = json.loads(text[start : end + 1]) + else: + logger.warning("Fix extraction fallback returned unparseable response") + return None + + result = FixResult( + fixes_applied=data.get("fixes_applied", []), + fixes_skipped=data.get("fixes_skipped", []), + ) + logger.info( + "Fix extraction fallback recovered %d applied, %d skipped", + len(result.fixes_applied), + len(result.fixes_skipped), + ) + return result + + except Exception as e: + logger.warning("Fix extraction fallback failed: %s", e) + return None + + +def apply_fixes( + findings: list[AssessedFinding], + artifact_path: str, + state_dir: Path, + round_num: int, + fixer_model: str | None = None, + timeout: int = 900, + base_url: str | None = None, + api_key: str | None = None, + reviewer_model: str | None = None, +) -> FixResult: + """Apply fixes via claude -p subprocess. + + Only called when critical + important > 0. + """ + # Check claude CLI exists + if not shutil.which("claude"): + raise RuntimeError("claude CLI not found in PATH") + + summary_path = state_dir / f"fix-summary-round-{round_num}.json" + + # Only include CRITICAL and IMPORTANT for fixing + fixable = [f for f in findings if f.severity in ("CRITICAL", "IMPORTANT")] + findings_text = _format_findings(fixable) + + fix_prompt = _FIX_PROMPT_TEMPLATE.format( + summary_path=summary_path, + findings_text=findings_text, + artifact_path=artifact_path, + ) + + # Save prompt for debugging + prompt_file = state_dir / f"fix-prompt-round-{round_num}.md" + prompt_file.write_text(fix_prompt) + + cmd = [ + "claude", + "-p", + "-", + "--allowedTools", + "Read,Edit,Write,Glob,Grep", + "--output-format", + "json", + ] + if fixer_model: + cmd.extend(["--model", fixer_model]) + + # Clear entrypoint to avoid analytics interference in nested subprocess + env = {**os.environ, "CLAUDE_CODE_ENTRYPOINT": ""} + # Remove CLAUDECODE env var to allow nested claude -p subprocess + env.pop("CLAUDECODE", None) + + logger.info( + "Running claude -p for round %d (%d findings to fix)", + round_num, + len(fixable), + ) + start = time.monotonic() + + proc = None + try: + proc = subprocess.Popen( + cmd, + stdin=subprocess.PIPE, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + env=env, + ) + try: + stdout, stderr = proc.communicate(input=fix_prompt, timeout=timeout) + except subprocess.TimeoutExpired: + proc.terminate() + try: + proc.wait(timeout=10) + except subprocess.TimeoutExpired: + proc.kill() + elapsed = time.monotonic() - start + logger.error("claude -p timed out after %.1fs, terminated", elapsed) + return FixResult(error=f"fixer timed out after {elapsed:.0f}s") + + elapsed = time.monotonic() - start + logger.info("claude -p completed in %.1fs (exit %d)", elapsed, proc.returncode) + + if proc.returncode != 0: + logger.error("claude -p exited with code %d", proc.returncode) + + if stderr: + logger.debug("claude -p stderr:\n%s", stderr[:2000]) + if stdout: + logger.debug("claude -p stdout:\n%s", stdout[:2000]) + + # Save full stdout/stderr to state dir for post-mortem + stdout_path = state_dir / f"fix-stdout-round-{round_num}.txt" + stdout_path.write_text(stdout or "") + stderr_path = state_dir / f"fix-stderr-round-{round_num}.txt" + stderr_path.write_text(stderr or "") + + except KeyboardInterrupt: + if proc and proc.poll() is None: + logger.info("Interrupted — terminating claude -p subprocess") + proc.terminate() + try: + proc.wait(timeout=10) + except subprocess.TimeoutExpired: + proc.kill() + raise # Re-raise so orchestrator's KeyboardInterrupt handler catches it + + except OSError as e: + logger.error("claude -p subprocess OS error: %s", e) + if proc and proc.poll() is None: + proc.terminate() + return FixResult(error=f"claude -p subprocess OS error: {e}") + + # Parse results: try claude -p --output-format json stdout first, + # then fall back to the fix-summary file we asked it to write + fix_result = _parse_claude_output(stdout) + if fix_result: + if proc.returncode != 0: + fix_result.error = f"claude -p exited with code {proc.returncode} (partial success)" + # Save parsed summary for state dir consistency + write_json_file(summary_path, { + "fixes_applied": fix_result.fixes_applied, + "fixes_skipped": fix_result.fixes_skipped, + }) + return fix_result + + if summary_path.exists(): + try: + data = json.loads(summary_path.read_text()) + result = FixResult( + fixes_applied=data.get("fixes_applied", []), + fixes_skipped=data.get("fixes_skipped", []), + ) + if proc.returncode != 0: + result.error = f"claude -p exited with code {proc.returncode} (partial success)" + return result + except (json.JSONDecodeError, KeyError) as e: + logger.warning("Failed to parse fix summary file: %s", e) + + # Fallback: ask an LLM to extract fix results from raw claude output + if stdout and len(stdout.strip()) > 50: + logger.info( + "Attempting LLM fallback extraction from %d char claude -p output", + len(stdout), + ) + fallback_result = _extract_fixes_fallback( + stdout, + base_url=base_url, + api_key=api_key, + model=reviewer_model, + ) + if fallback_result and (fallback_result.fixes_applied or fallback_result.fixes_skipped): + if proc.returncode != 0: + fallback_result.error = ( + f"claude -p exited with code {proc.returncode} " + f"(fix summary recovered via LLM fallback)" + ) + write_json_file(summary_path, { + "fixes_applied": fallback_result.fixes_applied, + "fixes_skipped": fallback_result.fixes_skipped, + }) + return fallback_result + + if proc.returncode != 0: + return FixResult(error=f"claude -p exited with code {proc.returncode}") + + logger.warning( + "Could not parse fix results from claude -p output or summary file. " + "Check %s for raw output.", + stdout_path, + ) + return FixResult(error="could not parse fix results from output or summary file") diff --git a/.claude/skills/convergence-review/model-aliases.json b/.claude/skills/convergence-review/model-aliases.json new file mode 100644 index 0000000..90f70aa --- /dev/null +++ b/.claude/skills/convergence-review/model-aliases.json @@ -0,0 +1,13 @@ +{ + "haiku": "aws/claude-haiku-4-5", + "sonnet": "aws/claude-sonnet-4-6", + "opus": "aws/claude-opus-4-6", + "claude-haiku": "aws/claude-haiku-4-5", + "claude-sonnet": "aws/claude-sonnet-4-6", + "claude-opus": "aws/claude-opus-4-6", + "gpt-5": "Azure/gpt-5-2025-08-07", + "gpt-4o": "Azure/gpt-4o", + "gpt-4.1-mini": "Azure/gpt-4.1-mini", + "gemini-pro": "gcp/gemini-2.5-pro", + "gemini-flash": "GCP/gemini-2.5-flash" +} diff --git a/.claude/skills/convergence-review/models.py b/.claude/skills/convergence-review/models.py new file mode 100644 index 0000000..5f1131b --- /dev/null +++ b/.claude/skills/convergence-review/models.py @@ -0,0 +1,117 @@ +"""Data classes for the convergence review orchestrator.""" + +from __future__ import annotations + +from dataclasses import dataclass, field +from types import MappingProxyType + + +@dataclass +class GateConfig: + """Configuration for a review gate type.""" + + gate: str + prompt_file: str + prompt_section: str + artifact_type: str # "file", "git_diff", "directory" + + +GATE_CONFIGS: MappingProxyType[str, GateConfig] = MappingProxyType({ + "design": GateConfig("design", "design-prompts.md", "A", "file"), + "g-design": GateConfig("g-design", "design-prompts.md", "C", "file"), + "macro-plan": GateConfig("macro-plan", "design-prompts.md", "B", "file"), + "x-macro-plan": GateConfig("x-macro-plan", "design-prompts.md", "D", "file"), + "g-macro-plan": GateConfig("g-macro-plan", "design-prompts.md", "E", "file"), + "pr-plan": GateConfig("pr-plan", "pr-prompts.md", "A", "file"), + "x-pr-plan": GateConfig("x-pr-plan", "pr-prompts.md", "C", "file"), + "pr-code": GateConfig("pr-code", "pr-prompts.md", "B", "git_diff"), + "x-pr-code": GateConfig("x-pr-code", "pr-prompts.md", "D", "git_diff"), + "h-design": GateConfig( + "h-design", "hypothesis-experiment/review-prompts.md", "A", "file" + ), + "h-code": GateConfig( + "h-code", "hypothesis-experiment/review-prompts.md", "B", "directory" + ), + "h-findings": GateConfig( + "h-findings", "hypothesis-experiment/review-prompts.md", "C", "file" + ), +}) + + +@dataclass +class Finding: + """A single finding from a reviewer.""" + + perspective: str + severity: str # "CRITICAL", "IMPORTANT", "SUGGESTION" + description: str + location: str | None = None + evidence: str | None = None + + +@dataclass +class ReviewerOutput: + """Output from a single reviewer perspective.""" + + perspective: str + findings: list[Finding] = field(default_factory=list) + error: str | None = None + parse_warning: str | None = None # set when response was non-empty but unparseable + elapsed_sec: float = 0.0 + + +@dataclass +class AssessedFinding: + """A finding after assessment (dedup, validation, re-prioritization).""" + + id: str # "F-1", "F-2", etc. + severity: str + original_severity: str + description: str + source_perspectives: list[str] = field(default_factory=list) + assessment_rationale: str = "" + location: str | None = None + evidence: str | None = None + cross_round_status: str = "novel" # novel, regression, recurring-fixed, recurring-skipped, recurring-escalate + prior_finding_ref: str | None = None # e.g. "F-R1-3" for traceability + + +@dataclass +class DismissedFinding: + """A finding dismissed during assessment.""" + + description: str + source_perspectives: list[str] = field(default_factory=list) + dismissal_reason: str = "" + + +@dataclass +class Assessment: + """Result of the assessment phase.""" + + assessed_findings: list[AssessedFinding] = field(default_factory=list) + dismissed: list[DismissedFinding] = field(default_factory=list) + uncovered_perspectives: list[str] = field(default_factory=list) + summary: dict = field(default_factory=dict) + fallback: bool = False # True when assessment failed and raw findings passed through + + +@dataclass +class FixResult: + """Result of the fix phase.""" + + fixes_applied: list[dict] = field(default_factory=list) + fixes_skipped: list[dict] = field(default_factory=list) + error: str | None = None # e.g. "timed out", "claude -p failed" + + +@dataclass +class RoundResult: + """Result of a single convergence round.""" + + round_num: int + reviewer_outputs: list[ReviewerOutput] = field(default_factory=list) + assessment: Assessment | None = None + fix_result: FixResult | None = None + status: str = "error" # "converged", "fixed", "error" + elapsed_sec: float = 0.0 diff --git a/.claude/skills/convergence-review/orchestrator.py b/.claude/skills/convergence-review/orchestrator.py new file mode 100644 index 0000000..8080851 --- /dev/null +++ b/.claude/skills/convergence-review/orchestrator.py @@ -0,0 +1,799 @@ +"""Convergence review orchestrator: review → assess → fix round loop.""" + +from __future__ import annotations + +import argparse +import asyncio +import json +import logging +import os +import re +import shutil +import sys +import time +from dataclasses import asdict +from pathlib import Path + +from openai import AsyncOpenAI + +from assessor import assess_findings, build_prior_ledger +from fixer import apply_fixes +from models import GATE_CONFIGS, Assessment, FixResult, RoundResult +from progress import ( + emit_assessment_complete, + emit_converged, + emit_error, + emit_event, + emit_fix_start, + emit_fixes_applied, + emit_interrupted, + emit_reviews_complete, + emit_round_complete, + emit_round_start, + emit_stalled, + write_json_file, + write_progress_file, +) +from prompts import load_artifact_content, load_perspectives +from reviewer import run_all_reviewers +from summary import SummaryWriter + +logger = logging.getLogger("convergence") + +DEFAULT_REVIEWER_MODEL = "haiku" +DEFAULT_FIXER_MODEL = "opus" +DEFAULT_MAX_ROUNDS = 10 +DEFAULT_REVIEWER_TIMEOUT = 300 +DEFAULT_ASSESSOR_TIMEOUT = 600 +DEFAULT_FIXER_TIMEOUT = 900 + + +def _load_model_aliases() -> dict[str, str]: + """Load model alias mapping from model-aliases.json next to this file.""" + aliases_path = Path(__file__).parent / "model-aliases.json" + if not aliases_path.exists(): + logger.debug("No model-aliases.json found at %s", aliases_path) + return {} + try: + return json.loads(aliases_path.read_text()) + except json.JSONDecodeError as e: + raise RuntimeError(f"model-aliases.json is not valid JSON: {e}") from e + + +def _resolve_model(name: str) -> str: + """Resolve a short model name to its full LiteLLM name via aliases.""" + aliases = _load_model_aliases() + return aliases.get(name, name) + + +def _sanitize_path(path: str) -> str: + """Sanitize artifact path for use as directory name.""" + return re.sub(r"[^a-zA-Z0-9_-]", "-", path).strip("-")[:80] + + +def _default_state_dir(gate: str, artifact: str | None) -> Path: + """Compute default state directory path.""" + suffix = _sanitize_path(artifact) if artifact else "diff" + return Path(".claude") / "convergence-state" / f"{gate}-{suffix}" + + +def parse_args(argv: list[str] | None = None) -> argparse.Namespace: + """Parse CLI arguments.""" + parser = argparse.ArgumentParser( + description="Convergence review orchestrator", + prog="orchestrator.py", + ) + parser.add_argument("gate", choices=sorted(GATE_CONFIGS.keys()), help="Review gate type") + parser.add_argument("artifact", nargs="?", default=None, help="Artifact path (not needed for pr-code)") + parser.add_argument( + "--reviewer-model", + default=os.environ.get("CONVERGENCE_REVIEWER_MODEL", DEFAULT_REVIEWER_MODEL), + help=f"Model for reviewers (default: {DEFAULT_REVIEWER_MODEL})", + ) + parser.add_argument( + "--fixer-model", + default=os.environ.get("CONVERGENCE_FIXER_MODEL", DEFAULT_FIXER_MODEL), + help=f"Model for assessment and fixes (default: {DEFAULT_FIXER_MODEL})", + ) + parser.add_argument( + "--max-rounds", + type=int, + default=DEFAULT_MAX_ROUNDS, + help=f"Maximum convergence rounds (default: {DEFAULT_MAX_ROUNDS})", + ) + parser.add_argument( + "--base-url", + default=os.environ.get("LITELLM_BASE_URL") or os.environ.get("ANTHROPIC_BASE_URL"), + help="API base URL (checks LITELLM_BASE_URL then ANTHROPIC_BASE_URL)", + ) + # API key sourced from environment only (not CLI) to avoid process-list exposure. + parser.set_defaults( + api_key=( + os.environ.get("LITELLM_API_KEY") + or os.environ.get("ANTHROPIC_AUTH_TOKEN") + or os.environ.get("ANTHROPIC_API_KEY") + or "" + ).strip(), + ) + parser.add_argument( + "--state-dir", + default=None, + help="State directory (default: .claude/convergence-state/-)", + ) + parser.add_argument( + "--log-level", + default="WARNING", + choices=["DEBUG", "INFO", "WARNING", "ERROR"], + help="Log level (default: WARNING)", + ) + parser.add_argument( + "--reviewer-timeout", + type=int, + default=DEFAULT_REVIEWER_TIMEOUT, + help=f"Per-reviewer timeout in seconds (default: {DEFAULT_REVIEWER_TIMEOUT})", + ) + parser.add_argument( + "--assessor-timeout", + type=int, + default=DEFAULT_ASSESSOR_TIMEOUT, + help=f"Assessment phase timeout in seconds (default: {DEFAULT_ASSESSOR_TIMEOUT})", + ) + parser.add_argument( + "--fixer-timeout", + type=int, + default=DEFAULT_FIXER_TIMEOUT, + help=f"Fix phase (claude -p) timeout in seconds (default: {DEFAULT_FIXER_TIMEOUT})", + ) + parser.add_argument( + "--force", + action="store_true", + default=False, + help="Disable early stall detection (run all rounds even if not converging)", + ) + parser.add_argument( + "--stall-window", + type=int, + default=5, + help="Number of rounds without improvement before early exit (default: 5)", + ) + parser.add_argument( + "--human", + action="store_true", + default=False, + help="Human-readable one-line progress to stderr (suppresses JSON and httpx noise)", + ) + + args = parser.parse_args(argv) + + # Validate stall window + if args.stall_window < 2: + parser.error("--stall-window must be >= 2") + + # Resolve model aliases + try: + args.reviewer_model = _resolve_model(args.reviewer_model) + args.fixer_model = _resolve_model(args.fixer_model) + except RuntimeError as e: + parser.error(str(e)) + + # Validate artifact requirement + config = GATE_CONFIGS[args.gate] + if config.artifact_type != "git_diff" and args.artifact is None: + parser.error(f"Gate '{args.gate}' requires an artifact path") + + # Compute state dir + if args.state_dir is None: + args.state_dir = str(_default_state_dir(args.gate, args.artifact)) + + return args + + +def _human_print(msg: str) -> None: + """Print a human-readable progress line to stderr.""" + print(msg, file=sys.stderr, flush=True) + + +def build_prior_round_context( + assessment: Assessment, + fix_result: FixResult | None, +) -> str: + """Build a prior-round context section for reviewer prompts. + + Summarizes dismissed findings and fixed findings from the prior round + so reviewers can focus on genuinely new issues rather than re-raising + themes that have already been addressed or evaluated. + """ + lines = [ + "## Prior Round Context", + "", + "The following findings were evaluated in a prior review round. " + "Use this context to focus your review on genuinely NEW issues.", + "", + ] + + # Dismissed findings + dismissed = assessment.dismissed + if dismissed: + lines.append("### Dismissed (not actionable)") + lines.append("") + lines.append( + "These were reviewed and determined to be not actionable. " + "Do NOT re-raise these unless you have a SPECIFIC counter-argument " + "that addresses the dismissal rationale." + ) + lines.append("") + for d in dismissed: + reason = d.dismissal_reason or "no rationale provided" + desc = d.description[:150] + lines.append(f"- {desc} — *Dismissed: {reason}*") + lines.append("") + + # Fixed findings + fixed_descriptions = [] + if fix_result and fix_result.fixes_applied: + for f in fix_result.fixes_applied: + desc = f.get("description", f.get("id", ""))[:150] + if desc: + fixed_descriptions.append(desc) + + if fixed_descriptions: + lines.append("### Fixed (already addressed in artifact)") + lines.append("") + lines.append( + "These issues were fixed in the artifact you are now reading. " + "Do NOT re-raise these unless the fix is clearly incomplete " + "or introduced a new problem." + ) + lines.append("") + for desc in fixed_descriptions: + lines.append(f"- {desc}") + lines.append("") + + # Skipped findings (fixer chose not to fix) + skipped_descriptions = [] + if fix_result and fix_result.fixes_skipped: + for f in fix_result.fixes_skipped: + desc = f.get("description", f.get("id", ""))[:150] + rationale = f.get("rationale", "")[:100] + if desc: + skipped_descriptions.append((desc, rationale)) + + if skipped_descriptions: + lines.append("### Skipped (fixer declined to fix)") + lines.append("") + lines.append( + "These were identified as important but the fixer chose not to address them. " + "If you believe these are still valid issues, re-raise them with " + "a clear argument for why they should be fixed." + ) + lines.append("") + for desc, rationale in skipped_descriptions: + skip_note = f" — *Skip rationale: {rationale}*" if rationale else "" + lines.append(f"- {desc}{skip_note}") + lines.append("") + + return "\n".join(lines) + + +async def run_round( + client: AsyncOpenAI, + args: argparse.Namespace, + round_num: int, + artifact_content: str, + perspectives: list, + state_dir: Path, + summary: SummaryWriter, + prior_round_context: str | None = None, +) -> RoundResult: + """Execute one convergence round: review → assess → fix.""" + round_start = time.monotonic() + + emit_round_start( + round_num, + args.max_rounds, + args.gate, + args.artifact or "git-diff", + args.reviewer_model, + args.fixer_model, + len(perspectives), + ) + summary.write_round_start(round_num, len(perspectives)) + + if args.human: + _human_print( + f"Round {round_num}/{args.max_rounds}: Dispatching {len(perspectives)} reviewers..." + ) + + # Phase 1: Review + reviewer_outputs = await run_all_reviewers( + client, + args.reviewer_model, + perspectives, + artifact_content, + args.reviewer_timeout, + round_num, + state_dir, + prior_round_context, + ) + + total_raw = sum(len(o.findings) for o in reviewer_outputs) + ok_count = sum(1 for o in reviewer_outputs if o.error is None) + fail_count = len(reviewer_outputs) - ok_count + review_elapsed = time.monotonic() - round_start + + emit_reviews_complete(round_num, total_raw, ok_count, fail_count, review_elapsed) + summary.write_review_complete(reviewer_outputs, review_elapsed) + + if args.human: + _human_print( + f" Reviewed: {ok_count} reviewers → {total_raw} raw findings ({review_elapsed:.0f}s)" + ) + + # Guard: if all reviewers failed or returned 0 raw findings, that's + # suspicious — not convergence. + if ok_count == 0: + logger.error("All %d reviewers failed in round %d", len(reviewer_outputs), round_num) + elif total_raw == 0 and ok_count > 0: + # 0 raw findings from successful reviewers. If any have parse + # warnings, this is a parsing failure — abort the round. Otherwise + # treat as genuine convergence (all reviewers found nothing). + parse_warnings = [ + f" {o.perspective}: {o.parse_warning}" + for o in reviewer_outputs + if o.parse_warning + ] + + if parse_warnings: + diag = ( + f"Round {round_num}: {ok_count} reviewers succeeded but returned " + f"0 total findings. This is likely a parsing failure, not convergence." + f"\n\nParse warnings:\n" + "\n".join(parse_warnings) + ) + logger.error(diag) + emit_error(diag) + summary.write_error(diag) + + round_elapsed = time.monotonic() - round_start + emit_round_complete(round_num, "error", 0, 0, 0, 0, round_elapsed) + if args.human: + _human_print(f"\n ERROR: 0 raw findings from {ok_count} reviewers — aborting round") + for pw in parse_warnings: + _human_print(pw) + return RoundResult( + round_num=round_num, + reviewer_outputs=reviewer_outputs, + status="error", + elapsed_sec=round_elapsed, + ) + + # Phase 2: Assess (with cross-round ledger) + prior_ledger = build_prior_ledger(state_dir, round_num) + summary.write_assess_start(total_raw) + if args.human: + ledger_note = f" (with {round_num - 1}-round ledger)" if prior_ledger else "" + _human_print(f" Assessing {total_raw} findings{ledger_note}...") + assess_start = time.monotonic() + assessment = await assess_findings( + client, args.fixer_model, reviewer_outputs, artifact_content, + timeout=args.assessor_timeout, gate=args.gate, prior_ledger=prior_ledger, + ) + assess_elapsed = time.monotonic() - assess_start + + if assessment.fallback: + emit_event({ + "event": "assessment_fallback", + "round": round_num, + "message": "Assessment API failed. Using raw findings without dedup/validation.", + }) + if args.human: + _human_print( + " WARNING: Assessment failed — using raw findings (may contain duplicates)" + ) + + emit_assessment_complete( + round_num, + assessment.summary.get("critical", 0), + assessment.summary.get("important", 0), + assessment.summary.get("suggestion", 0), + assessment.summary.get("dismissed", 0), + assessment.summary.get("deduplicated", 0), + assess_elapsed, + ) + + summary.write_assessment_complete(assessment) + + if args.human: + s = assessment.summary + _human_print( + f" Assessment: {s.get('critical', 0)}C {s.get('important', 0)}I " + f"{s.get('suggestion', 0)}S " + f"({s.get('dismissed', 0)} dismissed, {s.get('deduplicated', 0)} deduped, " + f"{assess_elapsed:.0f}s)" + ) + + # Save assessment + write_json_file( + state_dir / f"assessment-round-{round_num}.json", + { + "assessed_findings": [asdict(f) for f in assessment.assessed_findings], + "dismissed": [asdict(d) for d in assessment.dismissed], + "uncovered_perspectives": assessment.uncovered_perspectives, + "summary": assessment.summary, + }, + ) + + # Compute actionable C+I: exclude recurring-fixed and recurring-escalate + # (those are persistent themes surfaced to the user, not fixable in the loop) + _NON_ACTIONABLE = {"recurring-fixed", "recurring-escalate"} + actionable = [ + f for f in assessment.assessed_findings + if f.severity in ("CRITICAL", "IMPORTANT") + and f.cross_round_status not in _NON_ACTIONABLE + ] + critical = sum(1 for f in actionable if f.severity == "CRITICAL") + important = sum(1 for f in actionable if f.severity == "IMPORTANT") + + # Check for failed round: all reviewers errored or none returned findings + if ok_count == 0: + round_elapsed = time.monotonic() - round_start + emit_round_complete(round_num, "error", 0, 0, 0, 0, round_elapsed) + return RoundResult( + round_num=round_num, + reviewer_outputs=reviewer_outputs, + assessment=assessment, + status="error", + elapsed_sec=round_elapsed, + ) + + # Phase 3: Fix (only if needed — only actionable findings) + fix_result = None + if critical + important > 0: + emit_fix_start(round_num, critical + important) + summary.write_fix_start(critical, important) + if args.human: + _human_print(f" Fixing {critical + important} findings ({critical}C {important}I)...") + + fix_start = time.monotonic() + fix_result = apply_fixes( + actionable, + args.artifact or ".", + state_dir, + round_num, + fixer_model=args.fixer_model, + timeout=args.fixer_timeout, + base_url=args.base_url, + api_key=args.api_key, + reviewer_model=args.reviewer_model, + ) + fix_elapsed = time.monotonic() - fix_start + + critical_fixed = sum( + 1 for f in fix_result.fixes_applied if f.get("severity") == "CRITICAL" + ) + important_fixed = sum( + 1 for f in fix_result.fixes_applied if f.get("severity") == "IMPORTANT" + ) + emit_fixes_applied( + round_num, critical_fixed, important_fixed, len(fix_result.fixes_skipped), fix_elapsed + ) + if args.human: + _human_print( + f" Fix: {critical_fixed}C {important_fixed}I fixed, " + f"{len(fix_result.fixes_skipped)} skipped ({fix_elapsed:.0f}s)" + ) + summary.write_fix_complete(fix_result) + summary.write_round_summary(assessment, fix_result) + status = "fixed" + else: + summary.write_fix_complete(None, converged=True) + summary.write_round_summary(assessment, None) + status = "converged" + critical_fixed = 0 + important_fixed = 0 + + round_elapsed = time.monotonic() - round_start + emit_round_complete( + round_num, status, critical, important, critical_fixed, important_fixed, round_elapsed + ) + + # Write progress + round_data = { + "round": round_num, + "status": status, + "critical": critical, + "important": important, + "suggestion": assessment.summary.get("suggestion", 0), + "dismissed": assessment.summary.get("dismissed", 0), + "fixes_applied": len(fix_result.fixes_applied) if fix_result else 0, + "elapsed_sec": round(round_elapsed, 1), + } + write_progress_file(state_dir, round_num, round_data) + summary.record_round(round_num, assessment=assessment, fix_result=fix_result) + + return RoundResult( + round_num=round_num, + reviewer_outputs=reviewer_outputs, + assessment=assessment, + fix_result=fix_result, + status=status, + elapsed_sec=round_elapsed, + ) + + +async def main(argv: list[str] | None = None) -> int: + """Main orchestrator entry point. Returns exit code.""" + args = parse_args(argv) + + # Configure logging + log_level = getattr(logging, args.log_level) + logging.basicConfig( + level=log_level, + format="%(asctime)s %(name)s %(levelname)s %(message)s", + stream=sys.stderr, + ) + + if args.human: + # Suppress noisy third-party loggers + logging.getLogger("httpx").setLevel(logging.WARNING) + logging.getLogger("httpcore").setLevel(logging.WARNING) + logging.getLogger("openai").setLevel(logging.WARNING) + + logger.info("Starting convergence review: gate=%s artifact=%s", args.gate, args.artifact) + + # Validate setup + if not args.base_url: + emit_error("No LiteLLM base URL configured. Set LITELLM_BASE_URL or use --base-url.") + return 2 + + if not args.api_key: + emit_error( + "No API key configured. Set LITELLM_API_KEY, ANTHROPIC_AUTH_TOKEN, " + "or ANTHROPIC_API_KEY." + ) + return 2 + + # Load perspectives + try: + perspectives = load_perspectives(args.gate) + except (ValueError, FileNotFoundError) as e: + emit_error(str(e)) + return 2 + + # Load artifact content + try: + artifact_content = load_artifact_content(args.gate, args.artifact) + except (ValueError, FileNotFoundError, RuntimeError) as e: + emit_error(str(e)) + return 2 + + # Prepare state directory (clean start) + state_dir = Path(args.state_dir).resolve() + cwd = Path.cwd().resolve() + if not state_dir.is_relative_to(cwd): + emit_error(f"State directory must be under current working directory: {state_dir}") + return 2 + if state_dir.exists(): + try: + shutil.rmtree(state_dir) + except OSError as e: + emit_error(f"Failed to clean state directory {state_dir}: {e}") + return 2 + state_dir.mkdir(parents=True, exist_ok=True) + + # Create summary writer + summary = SummaryWriter( + state_dir, args.gate, args.artifact, args.max_rounds, + reviewer_model=args.reviewer_model, fixer_model=args.fixer_model, + ) + logger.info("Summary file: %s", summary.path) + emit_event({"event": "summary_path", "path": str(summary.path)}) + + # Strip SOCKS proxy env vars that break httpx without socksio (process-global). + # Only ALL_PROXY/all_proxy are stripped; HTTPS_PROXY/HTTP_PROXY are kept. + for var in ("ALL_PROXY", "all_proxy"): + os.environ.pop(var, None) + + # Create OpenAI client + client = AsyncOpenAI(base_url=args.base_url, api_key=args.api_key) + + total_start = time.monotonic() + round_num = 0 + finding_history: list[int] = [] # actionable critical+important per round + fixes_history: list[int] = [] # fixes applied per round + recurring_history: list[int] = [] # recurring finding count per round + persistent_themes: list[dict] = [] # accumulated escalated themes for summary + prior_round_context: str | None = None # fed to reviewers from round 2+ + + try: + for round_num in range(1, args.max_rounds + 1): + result = await run_round( + client, args, round_num, artifact_content, perspectives, state_dir, summary, + prior_round_context, + ) + + if result.status == "converged": + total_elapsed = time.monotonic() - total_start + summary.write_persistent_themes(persistent_themes) + summary.write_converged(round_num, total_elapsed) + emit_converged(round_num, round_num, total_elapsed) + if args.human: + _human_print(f"\nCONVERGED in {round_num} rounds ({total_elapsed:.0f}s)") + logger.info("Converged in %d rounds (%.1fs)", round_num, total_elapsed) + return 0 + + if result.status == "error": + summary.write_error(f"Round {round_num} failed: all reviewers errored") + emit_error(f"Round {round_num} failed: all reviewers errored") + if args.human: + _human_print(f"\nERROR: Round {round_num} failed — all reviewers errored") + return 2 + + # Track actionable finding counts and fixes for stall detection + actionable_count = 0 + recurring = 0 + escalated_this_round = [] + if result.assessment: + _NA = {"recurring-fixed", "recurring-escalate"} + for f in result.assessment.assessed_findings: + if f.severity in ("CRITICAL", "IMPORTANT"): + if f.cross_round_status not in _NA: + actionable_count += 1 + if f.cross_round_status != "novel": + recurring += 1 + if f.cross_round_status == "recurring-escalate": + escalated_this_round.append({ + "description": f.description, + "severity": f.severity, + "first_seen_ref": f.prior_finding_ref, + "round_escalated": round_num, + }) + + finding_history.append(actionable_count) + fixes_history.append( + len(result.fix_result.fixes_applied) if result.fix_result else 0 + ) + recurring_history.append(recurring) + + # Accumulate persistent themes (deduplicate by description prefix) + for theme in escalated_this_round: + prefix = theme["description"][:100] + if not any(t["description"][:100] == prefix for t in persistent_themes): + persistent_themes.append(theme) + + total_ci = ( + result.assessment.summary.get("critical", 0) + + result.assessment.summary.get("important", 0) + ) if result.assessment else 0 + summary.write_recurring(recurring, total_ci, len(escalated_this_round)) + if recurring > 0: + logger.info( + "Round %d: %d/%d recurring, %d escalated as persistent themes", + round_num, recurring, total_ci, len(escalated_this_round), + ) + emit_event({ + "event": "recurring_findings", + "round": round_num, + "recurring": recurring, + "total": total_ci, + "actionable": actionable_count, + "escalated": len(escalated_this_round), + }) + if args.human: + _human_print( + f" Cross-round: {recurring}/{total_ci} recurring, " + f"{actionable_count} actionable, " + f"{len(escalated_this_round)} escalated as persistent" + ) + + # Early stall detection: exit early if findings aren't decreasing + # AND no fixes are being applied across the stall window. + # If fixes are being applied, the fixer is making progress even + # if new findings keep appearing — so we keep going. + if not args.force and len(finding_history) >= args.stall_window: + window = finding_history[-args.stall_window :] + fixes_window = fixes_history[-args.stall_window :] + no_fixes = sum(fixes_window) == 0 + if min(window) >= window[0] and window[0] > 0 and no_fixes: + logger.warning( + "Early stall: findings not decreasing and no fixes applied " + "over %d rounds: %s (fixes: %s)", + args.stall_window, + window, + fixes_window, + ) + emit_event({ + "event": "early_stall", + "round": round_num, + "finding_history": finding_history, + "recurring_history": recurring_history, + "message": ( + f"Finding counts not decreasing over {args.stall_window} rounds " + f"({window}). Use --force to override." + ), + }) + remaining_critical = result.assessment.summary.get("critical", 0) if result.assessment else 0 + remaining_important = result.assessment.summary.get("important", 0) if result.assessment else 0 + summary.write_persistent_themes(persistent_themes) + summary.write_stalled( + round_num, remaining_critical, remaining_important, + reason=f"findings not decreasing over {args.stall_window} rounds ({window})", + ) + emit_stalled(round_num, remaining_critical, remaining_important) + if args.human: + _human_print( + f"\nSTALLED (early): not converging after {args.stall_window} rounds " + f"({remaining_critical}C {remaining_important}I remaining). " + f"Use --force to override." + ) + return 1 + + # Build prior-round context for next round's reviewers + if result.assessment: + prior_round_context = build_prior_round_context( + result.assessment, result.fix_result + ) + logger.info( + "Built prior-round context for round %d (%d chars)", + round_num + 1, len(prior_round_context), + ) + + # Reload artifact content for next round (fixes may have changed it) + if result.status == "fixed": + reload_ok = False + for reload_attempt in range(3): + try: + artifact_content = load_artifact_content(args.gate, args.artifact) + reload_ok = True + break + except (ValueError, FileNotFoundError, RuntimeError) as e: + logger.warning( + "Artifact reload attempt %d failed: %s", + reload_attempt + 1, e, + ) + await asyncio.sleep(1) + if not reload_ok: + summary.write_error( + f"Failed to reload artifact after fixes (3 attempts). " + f"The fixer may have corrupted or deleted the file." + ) + emit_error("Failed to reload artifact after fixes") + return 2 + + # Max rounds reached + last_assessment = result.assessment + remaining_critical = last_assessment.summary.get("critical", 0) if last_assessment else 0 + remaining_important = last_assessment.summary.get("important", 0) if last_assessment else 0 + summary.write_persistent_themes(persistent_themes) + summary.write_stalled( + round_num, remaining_critical, remaining_important, + reason=f"max rounds ({args.max_rounds}) exhausted", + ) + emit_stalled(round_num, remaining_critical, remaining_important) + if args.human: + _human_print( + f"\nSTALLED: max rounds ({round_num}) reached. " + f"{remaining_critical}C {remaining_important}I remaining." + ) + logger.warning( + "Stalled after %d rounds: %d CRITICAL, %d IMPORTANT remaining", + round_num, + remaining_critical, + remaining_important, + ) + return 1 + + except KeyboardInterrupt: + summary.write_error("Interrupted by user") + emit_interrupted() + logger.info("Interrupted by user") + return 130 + + except Exception as e: + logger.exception("Unhandled error in convergence loop") + summary.write_error(f"Unhandled error: {e}") + emit_error(f"Unhandled error: {e}") + return 2 + + +if __name__ == "__main__": + sys.exit(asyncio.run(main())) diff --git a/.claude/skills/convergence-review/pr-prompts.md b/.claude/skills/convergence-review/pr-prompts.md new file mode 100644 index 0000000..0ca886f --- /dev/null +++ b/.claude/skills/convergence-review/pr-prompts.md @@ -0,0 +1,536 @@ +# PR Review Perspective Prompts + +Reference file for the convergence-review skill. Contains exact prompts for PR review perspectives across plan review, code review, and cross-system plan review gates. + +**Canonical source:** `docs/contributing/pr-workflow.md`. If prompts here diverge from pr-workflow.md, the process doc is authoritative. Sections A and B use inference-sim perspectives (v3.0). Section C uses sim2real perspectives (v1.0). + +**Dispatch pattern:** Launch each perspective as a parallel Task agent. **Do NOT paste the artifact content into the prompt** — this causes output generation to hang when dispatching 8+ agents in parallel. Instead, tell agents to read the file themselves: +``` +Task(subagent_type="general-purpose", model=REVIEW_MODEL, run_in_background=True, + prompt="") +``` +Each prompt below uses `ARTIFACT_PATH` as a placeholder. The dispatcher must replace it with the actual file path before launching agents. For `pr-code` gate, agents should run `git diff` themselves instead of reading a file. + +Model selection is controlled by the `--model` flag in the convergence-review skill (default: `haiku`). + +--- + +## Section A: PR Plan Review (10 perspectives) — Step 2.5 + +### PP-1: Substance & Design + +``` +Review this implementation plan for substance: Are the behavioral contracts logically sound? Are there mathematical errors, scale mismatches, or unit confusions? Could the design actually achieve what the contracts promise? Check formulas, thresholds, and edge cases from first principles — not just structural completeness. + +First, read the plan file at ARTIFACT_PATH using the Read tool. + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### PP-2: Cross-Document Consistency + +``` +Does this micro plan's scope match the source document? Are file paths consistent with the actual codebase? Does the deviation log account for all differences between what the source says and what the micro plan does? Check for stale references to completed PRs or removed files. + +First, read the plan file at ARTIFACT_PATH using the Read tool. + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### PP-3: Architecture Boundary Verification + +``` +Does this plan maintain architectural boundaries? Check: +(1) Individual instances don't access cluster-level state +(2) Types are in the right packages (sim/ vs sim/cluster/ vs cmd/) +(3) No import cycles introduced +(4) Does the plan introduce multiple construction sites for the same type? +(5) Does adding one field to a new type require >3 files? +(6) Does library code (sim/) call logrus.Fatalf anywhere in new code? +(7) Dependency direction: cmd/ → sim/cluster/ → sim/ (never reversed) + +First, read the plan file at ARTIFACT_PATH using the Read tool. + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### PP-4: Codebase Readiness + +``` +We're about to implement this PR. Review the codebase for readiness. Check each file the plan will modify for: +- Stale comments ("planned for PR N" where N is completed) +- Pre-existing bugs that would complicate implementation +- Missing dependencies +- Unclear insertion points +- TODO/FIXME items in the modification zone + +First, read the plan file at ARTIFACT_PATH using the Read tool. + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### PP-5: Structural Validation (perform directly, no agent) + +> **For Claude:** Perform these 4 checks directly. Do NOT dispatch an agent. + +**Check 1 — Task Dependencies:** +For each task, verify it can actually start given what comes before it. Trace the dependency chain: what files does each task create/modify? Does any task require a file or type that hasn't been created yet? + +**Check 2 — Template Completeness:** +Verify all sections from `docs/contributing/templates/micro-plan.md` are present and non-empty: Header, Part 1 (A-E), Part 2 (F-I), Part 3 (J), Appendix. + +**Check 3 — Executive Summary Clarity:** +Read the executive summary as if you're a new team member. Is the scope clear without reading the rest? + +**Check 4 — Under-specified Tasks:** +For each task, verify it has complete code. Flag any step an executing agent would need to figure out on its own. + +### PP-6: DES Expert + +``` +Review this plan as a discrete-event simulation expert. Check for: +- Event ordering bugs in the proposed design +- Clock monotonicity violations (INV-3) +- Stale signal propagation between event types (INV-7) +- Heap priority errors (cluster uses (timestamp, priority, seqID)) +- Event-driven race conditions +- Work-conserving property violations (INV-8) +- Incorrect assumptions about DES event processing semantics + +First, read the plan file at ARTIFACT_PATH using the Read tool. + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### PP-7: vLLM/SGLang Expert + +``` +Review this plan as a vLLM/SGLang inference serving expert. Check for: +- Batching semantics that don't match real continuous-batching servers +- KV cache eviction policies that differ from vLLM's implementation +- Chunked prefill behavior mismatches +- Preemption policy differences from vLLM +- Missing scheduling features that real servers have +- Flag any assumption about LLM serving that this plan gets wrong + +First, read the plan file at ARTIFACT_PATH using the Read tool. + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### PP-8: Distributed Inference Platform Expert + +``` +Review this plan as a distributed inference platform expert (llm-d, KServe, vLLM multi-node). Check for: +- Multi-instance coordination bugs +- Routing load imbalance under high request rates +- Stale snapshot propagation between instances +- Admission control edge cases at scale +- Horizontal scaling assumption violations +- Prefix-affinity routing correctness across instances + +First, read the plan file at ARTIFACT_PATH using the Read tool. + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### PP-9: Performance & Scalability + +``` +Review this plan as a performance and scalability analyst. Check for: +- Algorithmic complexity issues (O(n^2) where O(n) suffices) +- Unnecessary allocations in hot paths (event loop, batch formation) +- Map iteration in O(n) loops that could grow +- Benchmark-sensitive changes +- Memory growth patterns +- Changes that would degrade performance at 1000+ requests or 10+ instances + +First, read the plan file at ARTIFACT_PATH using the Read tool. + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### PP-10: Security & Robustness + +``` +Review this plan as a security and robustness reviewer. Check for: +- Input validation completeness (all CLI flags, YAML fields, config values) +- Panic paths reachable from user input (R3, R6) +- Resource exhaustion vectors (unbounded loops, unlimited memory growth) (R19) +- Degenerate input handling (empty, zero, negative, NaN, Inf) (R3, R20) +- Configuration injection risks +- Silent data loss paths (R1) + +First, read the plan file at ARTIFACT_PATH using the Read tool. + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +--- + +## Section B: PR Code Review (10 perspectives) — Step 4.5 + +### PC-1: Substance & Design + +``` +Review this diff for substance: Are there logic bugs, design mismatches between contracts and implementation, mathematical errors, or silent regressions? Check from first principles — not just structural patterns. Does the implementation actually achieve what the behavioral contracts promise? + +First, run `git diff` using the Bash tool to see the current code changes. Then review them. + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### PC-2: Code Quality + Antipattern Check + +``` +Review this diff for code quality. Check all of these: +(1) Any new error paths that use `continue` or early `return` — do they clean up partial state? (R1, R5) +(2) Any map iteration that accumulates floats — are keys sorted? (R2) +(3) Any struct field added — are all construction sites updated? (R4) +(4) Does library code (sim/) call logrus.Fatalf anywhere in new code? (R6) +(5) Any exported mutable maps — should they be unexported with IsValid*() accessors? (R8) +(6) Any YAML config fields using float64 instead of *float64 where zero is valid? (R9) +(7) Any division where the denominator derives from runtime state without a zero guard? (R11) +(8) Any new interface with methods only meaningful for one implementation? (R13) +(9) Any method >50 lines spanning multiple concerns (scheduling + latency + metrics)? (R14) +(10) Any changes to docs/contributing/standards/ files — are CLAUDE.md working copies updated? (DRY) + +First, run `git diff` using the Bash tool to see the current code changes. Then review them. + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### PC-3: Test Behavioral Quality + +``` +Review the tests in this diff. For each test, rate as Behavioral, Mixed, or Structural: +- Behavioral: tests observable behavior (GIVEN/WHEN/THEN), survives refactoring +- Mixed: some behavioral assertions, some structural coupling +- Structural: asserts internal structure (field access, type assertions), breaks on refactor + +Also check: +- Are there golden dataset tests that lack companion invariant tests? (R7) +- Do tests verify laws (conservation, monotonicity, causality) not just values? +- Would each test still pass if the implementation were completely rewritten? + +First, run `git diff` using the Bash tool to see the current code changes. Then review them. + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### PC-4: Getting-Started Experience + +``` +Review this diff for user and contributor experience. Simulate both journeys: +(1) A user doing capacity planning with the CLI — would they find everything they need? +(2) A contributor adding a new algorithm — would they know how to extend this? + +Check: +- Missing example files or CLI documentation +- Undocumented output metrics +- Incomplete contributor guide updates +- Unclear extension points +- README not updated for new features + +First, run `git diff` using the Bash tool to see the current code changes. Then review them. + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### PC-5: Automated Reviewer Simulation + +``` +The upstream community uses GitHub Copilot, Claude, and Codex to review PRs. Do a rigorous check so this will pass their review. Look for: +- Exported mutable globals +- User-controlled panic paths +- YAML typo acceptance (should use KnownFields(true)) +- NaN/Inf validation gaps +- Redundant or dead code +- Style inconsistencies +- Missing error returns + +First, run `git diff` using the Bash tool to see the current code changes. Then review them. + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### PC-6: DES Expert + +``` +Review this diff as a discrete-event simulation expert. Check for: +- Event ordering bugs in the implementation +- Clock monotonicity violations (INV-3) +- Stale signal propagation between event types (INV-7) +- Heap priority errors +- Work-conserving property violations (INV-8) +- Event-driven race conditions + +First, run `git diff` using the Bash tool to see the current code changes. Then review them. + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### PC-7: vLLM/SGLang Expert + +``` +Review this diff as a vLLM/SGLang inference serving expert. Check for: +- Batching semantics that don't match real continuous-batching servers +- KV cache eviction mismatches with vLLM +- Chunked prefill behavior errors +- Preemption policy differences +- Missing scheduling features +- Flag any assumption about LLM serving that this code gets wrong + +First, run `git diff` using the Bash tool to see the current code changes. Then review them. + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### PC-8: Distributed Inference Platform Expert + +``` +Review this diff as a distributed inference platform expert (llm-d, KServe, vLLM multi-node). Check for: +- Multi-instance coordination bugs +- Routing load imbalance +- Stale snapshot propagation +- Admission control edge cases +- Horizontal scaling assumption violations +- Prefix-affinity routing correctness + +First, run `git diff` using the Bash tool to see the current code changes. Then review them. + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### PC-9: Performance & Scalability + +``` +Review this diff as a performance and scalability analyst. Check for: +- Algorithmic complexity regressions (O(n^2) where O(n) suffices) +- Unnecessary allocations in hot paths +- Map iteration in O(n) loops +- Benchmark-sensitive changes +- Memory growth patterns +- Changes degrading performance at 1000+ requests or 10+ instances + +First, run `git diff` using the Bash tool to see the current code changes. Then review them. + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### PC-10: Security & Robustness + +``` +Review this diff as a security and robustness reviewer. Check for: +- Input validation completeness (CLI flags, YAML fields, config values) +- Panic paths reachable from user input +- Resource exhaustion vectors (unbounded loops, unlimited memory growth) +- Degenerate input handling (empty, zero, NaN, Inf) +- Configuration injection risks +- Silent data loss in error paths + +First, run `git diff` using the Bash tool to see the current code changes. Then review them. + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +--- + +## Section C: Cross-System PR Plan Review (5 perspectives) — x-pr-plan + +For sim2real cross-system transfer PRs. 5 perspectives aligned with `docs/contributing/pr-workflow.md` (v1.0). Not all perspectives are used for every PR category — see the pr-workflow's Perspective Assignment table. + +**Canonical source:** `docs/contributing/pr-workflow.md` (v1.0). If prompts here diverge from pr-workflow.md, the process doc is authoritative. + +### XPP-1: Cross-System Contract Integrity + +``` +Review this plan for cross-system contract integrity. Check: +(1) Do artifacts correctly describe the APIs in submodules? Read the actual submodule source files referenced in the plan and compare API signatures, types, and method names against what the plan documents. +(2) Are commit pins current? Run `git submodule status` and compare against any commit hashes referenced in the plan or mapping artifact. +(3) Do workspace artifact schemas chain correctly between stages? For each workspace artifact (algorithm_summary.json, signal_coverage.json, validation_results.json), verify that the fields written by the producing stage match the fields expected by consuming stages. +(4) Signal mapping accuracy: does each signal name in the mapping artifact match the actual field name in both the source system (inference-sim) and target system (llm-d-inference-scheduler)? Check for semantic drift, type mismatches (int vs float, absolute vs relative), unit mismatches (ms vs s), and temporal mismatches (point-in-time vs rolling average). +(5) Plugin interface compliance: do references to the target system's scorer interface, registration pattern, and config format match the actual code in the submodule? + +First, read the plan file at ARTIFACT_PATH using the Read tool. Then read the relevant submodule source files to verify claims. + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### XPP-2: Artifact Completeness & Consistency + +``` +Review this plan for artifact completeness and cross-artifact consistency. Check: +(1) Are all required sections present in each artifact the plan creates or modifies? (Mapping artifact: signal table, types, metric paths, staleness windows, fidelity ratings, commit pin. Scorer template: all 8 sections per design doc. Prompt templates: prerequisites, validation steps, halt conditions, expected outputs.) +(2) Do signal names match exactly across all artifacts? Grep for each signal name in the mapping artifact, JSON schemas, prompt templates, CLI code, and README. Any spelling or casing mismatch is a bug. +(3) Do field names match across workspace artifact schemas and the code that reads/writes them? +(4) Do file paths referenced in the plan exist or will be created by a preceding task? Flag any reference to a file that doesn't exist and isn't created by an earlier task. +(5) Is the deviation log current? If the plan deviates from the macro plan, is each deviation documented with rationale? +(6) Dead artifact check: is every file created by this plan consumed by a test, a later PR, or the pipeline runtime? If a file has no consumer, flag it. + +First, read the plan file at ARTIFACT_PATH using the Read tool. + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### XPP-3: Prompt Template Quality + +``` +Review this plan's prompt templates (or plans for prompt templates) for quality and completeness. Check: +(1) Does each prompt template specify prerequisites — which workspace artifacts must exist before this stage runs, and how to validate them (schema check + file existence)? +(2) Does each prompt template specify validation steps — how the operator or LLM verifies the stage's output before writing to workspace artifacts? +(3) Are halt conditions clear and unambiguous? For each condition that should stop the pipeline, is the trigger specific (not "if something looks wrong") and is the action explicit (exit code, error message, user decision options)? +(4) Are expected outputs fully specified — file names, JSON field names, format constraints? +(5) Does the prompt reference predecessor artifact checks? Each consuming stage should validate the predecessor artifact's schema before reading. +(6) Could the prompt cause LLM missteps? Look for ambiguous instructions, missing context, or steps where the LLM would need to guess. Each step should be self-contained enough that a capable LLM can execute it without external knowledge of the codebase. + +First, read the plan file at ARTIFACT_PATH using the Read tool. + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### XPP-4: Code Correctness (Python CLI + Go Harness) + +``` +Review this plan's code (Python CLI tools and Go test harness) for correctness. Check: +(1) Edge cases: empty input files, missing fields in JSON, zero-length signal lists, malformed YAML. +(2) Error handling: does every CLI command handle all three exit code paths (0 = success, 1 = validation failure, 2 = infrastructure error)? Are errors surfaced with actionable messages, not swallowed? +(3) JSON output contract: does each CLI command's actual JSON output match the schema documented in the macro plan's Component Model section? Check field names, types, and required vs optional. +(4) Go harness correctness: compilation against pinned submodule HEAD, test tuple handling, score comparison logic, timeout handling. +(5) Input validation: CLI argument parsing safety, path traversal prevention in file operations, subprocess invocation safety. +(6) Resource bounds: no unbounded file reads, no unlimited memory growth, no infinite retry loops without limits. + +First, read the plan file at ARTIFACT_PATH using the Read tool. + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### XPP-5: Plan Structural Validation (perform directly, no agent) + +> **For Claude:** Perform these 5 checks directly. Do NOT dispatch an agent. + +**Check 1 — Task Dependencies:** +For each task, verify it can actually start given what comes before it. Trace the dependency chain: what files does each task create/modify? Does any task require a file, type, or artifact that hasn't been created yet? + +**Check 2 — Template Completeness:** +Verify all sections from `docs/contributing/templates/micro-plan-cross-system.md` are present and non-empty: Header, Part 1 (A-E), Part 2 (F-I), Part 3 (J), Appendix. + +**Check 3 — Executive Summary Clarity:** +Read the executive summary as if you're a new team member unfamiliar with either codebase. Is the scope clear without reading the rest? Is the transfer direction obvious? + +**Check 4 — Under-specified Tasks:** +For each task, verify it has complete code or complete artifact content. Flag any step an executing agent would need to figure out on its own, especially cross-system integration steps. Verify each task uses the correct variant (Code Task / Artifact Task / Prompt Template Task). + +**Check 5 — Verification Gate Alignment:** +Does the plan's verification gate match the PR category from `docs/contributing/pr-workflow.md`? (Artifact → mapping validation + schema check. Pipeline Stage → pytest + go build + prompt check. Validation → pytest + go test + go build. Integration → end-to-end smoke test.) + +--- + +## Section D: Cross-System PR Code Review (4 perspectives) — x-pr-code + +For sim2real cross-system transfer PRs — post-implementation code/artifact review. 4 perspectives aligned with `docs/contributing/pr-workflow.md` (v1.0), minus plan structural validation (which only applies to plans, not diffs). Not all perspectives are used for every PR category — see the pr-workflow's Perspective Assignment table. + +**Canonical source:** `docs/contributing/pr-workflow.md` (v1.0). If prompts here diverge from pr-workflow.md, the process doc is authoritative. + +**Perspective assignment for code review by PR category:** + +| Category | XPC-1 Contracts | XPC-2 Artifacts | XPC-3 Prompts | XPC-4 Code | +|----------|:---:|:---:|:---:|:---:| +| **Artifact** | X | X | | | +| **Pipeline Stage** | X | X | X | | +| **Validation** | X | X | X | X | +| **Integration** | X | X | X | X | + +### XPC-1: Cross-System Contract Integrity + +``` +Review this diff for cross-system contract integrity. Check: +(1) Do any changes to artifacts (mapping docs, schemas, README) still correctly describe the APIs in submodules? Read the actual submodule source files and compare API signatures, types, and method names against what the changed artifacts document. +(2) Are commit pins still current? If the diff modifies files that reference submodule commit hashes, run `git submodule status` and verify they match. +(3) Do workspace artifact schema changes maintain the chain? If a schema field was added, renamed, or removed, verify that both the producing stage and all consuming stages are updated consistently. +(4) Signal mapping accuracy: if the diff touches signal names in any file (mapping artifact, schemas, CLI code, prompts), verify the name matches the actual field in both inference-sim and llm-d-inference-scheduler submodules. +(5) Plugin interface compliance: if the diff touches scorer template or generated code patterns, verify they still match the target system's actual interface. + +First, run `git diff` using the Bash tool to see the current code changes. Then read relevant submodule source files to verify claims. + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### XPC-2: Artifact Completeness & Consistency + +``` +Review this diff for artifact completeness and cross-artifact consistency. Check: +(1) If the diff adds or modifies an artifact, are all required sections present? (Mapping artifact: signal table, types, metric paths, staleness windows, fidelity ratings, commit pin. Scorer template: all 8 sections per design doc. Prompt templates: prerequisites, validation steps, halt conditions, expected outputs.) +(2) Do signal names match exactly across all files touched by this diff? Grep for each signal name in the mapping artifact, JSON schemas, prompt templates, CLI code, and README. Any spelling or casing mismatch is a bug. +(3) Do field names in changed JSON schemas match the code that reads/writes those fields? +(4) Do file paths referenced in changed documents actually exist in the repository? +(5) Dead artifact check: does this diff create any file that has no consumer (no test reads it, no later stage uses it, no pipeline runtime depends on it)? + +First, run `git diff` using the Bash tool to see the current code changes. + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### XPC-3: Prompt Template Quality + +``` +Review this diff's prompt templates for quality and completeness. Check: +(1) Does each new or modified prompt template specify prerequisites — which workspace artifacts must exist before this stage runs, and how to validate them (schema check + file existence)? +(2) Does each prompt specify validation steps — how the operator or LLM verifies the stage's output before writing to workspace artifacts? +(3) Are halt conditions clear and unambiguous? For each condition that should stop the pipeline, is the trigger specific (not "if something looks wrong") and is the action explicit (exit code, error message, user decision options)? +(4) Are expected outputs fully specified — file names, JSON field names, format constraints? +(5) Does the prompt reference predecessor artifact checks? Each consuming stage should validate the predecessor artifact's schema before reading. +(6) Could the prompt cause LLM missteps? Look for ambiguous instructions, missing context, or steps where the LLM would need to guess. + +If this diff does not touch any prompt templates, report "No prompt templates in diff — 0 CRITICAL, 0 IMPORTANT" and stop. + +First, run `git diff` using the Bash tool to see the current code changes. + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### XPC-4: Code Correctness (Python CLI + Go Harness) + +``` +Review this diff's code (Python CLI tools and Go test harness) for correctness. Check: +(1) Edge cases: empty input files, missing fields in JSON, zero-length signal lists, malformed YAML. +(2) Error handling: does every CLI command handle all three exit code paths (0 = success, 1 = validation failure, 2 = infrastructure error)? Are errors surfaced with actionable messages, not swallowed? +(3) JSON output contract: does each CLI command's JSON output match the schema documented in the macro plan? Check field names, types, and required vs optional. +(4) Go harness correctness: compilation against pinned submodule HEAD, test tuple handling, score comparison logic, timeout handling. +(5) Input validation: CLI argument parsing safety, path traversal prevention in file operations, subprocess invocation safety. +(6) Resource bounds: no unbounded file reads, no unlimited memory growth, no infinite retry loops without limits. +(7) Test quality: do tests verify observable behavior (not internal structure)? Do tests cover the contracts claimed in the plan? + +If this diff does not touch any Python or Go code, report "No code in diff — 0 CRITICAL, 0 IMPORTANT" and stop. + +First, run `git diff` using the Bash tool to see the current code changes. + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` diff --git a/.claude/skills/convergence-review/progress.py b/.claude/skills/convergence-review/progress.py new file mode 100644 index 0000000..2ed36d5 --- /dev/null +++ b/.claude/skills/convergence-review/progress.py @@ -0,0 +1,234 @@ +"""JSON progress event emitter and state file writer.""" + +from __future__ import annotations + +import json +import logging +from pathlib import Path +from typing import Any + +logger = logging.getLogger("convergence.progress") + + +def emit_event(event: dict[str, Any]) -> None: + """Emit a JSON progress event to stdout (one line).""" + try: + line = json.dumps(event, separators=(",", ":"), default=str) + except (TypeError, ValueError) as e: + logger.error("Failed to serialize event: %s — %s", event.get("event"), e) + # Emit a minimal fallback so the parent process knows something went wrong + fallback = json.dumps( + {"event": "internal_error", "message": f"event serialization failed: {e}"}, + separators=(",", ":"), + ) + try: + print(fallback, flush=True) + except BrokenPipeError: + pass + return + try: + print(line, flush=True) + except BrokenPipeError: + return + logger.debug("Event emitted: %s", event.get("event")) + + +def emit_round_start( + round_num: int, + max_rounds: int, + gate: str, + artifact: str, + reviewer_model: str, + fixer_model: str, + perspectives: int, +) -> None: + emit_event( + { + "event": "round_start", + "round": round_num, + "max_rounds": max_rounds, + "gate": gate, + "artifact": artifact, + "reviewer_model": reviewer_model, + "fixer_model": fixer_model, + "perspectives": perspectives, + } + ) + + +def emit_reviewer_done( + round_num: int, perspective: str, findings_count: int, elapsed_sec: float +) -> None: + emit_event( + { + "event": "reviewer_done", + "round": round_num, + "perspective": perspective, + "findings_count": findings_count, + "elapsed_sec": round(elapsed_sec, 1), + } + ) + + +def emit_reviewer_error( + round_num: int, perspective: str, error: str, elapsed_sec: float +) -> None: + emit_event( + { + "event": "reviewer_error", + "round": round_num, + "perspective": perspective, + "error": error, + "elapsed_sec": round(elapsed_sec, 1), + } + ) + + +def emit_reviews_complete( + round_num: int, + total_raw_findings: int, + perspectives_ok: int, + perspectives_failed: int, + elapsed_sec: float, +) -> None: + emit_event( + { + "event": "reviews_complete", + "round": round_num, + "total_raw_findings": total_raw_findings, + "perspectives_ok": perspectives_ok, + "perspectives_failed": perspectives_failed, + "elapsed_sec": round(elapsed_sec, 1), + } + ) + + +def emit_assessment_complete( + round_num: int, + critical: int, + important: int, + suggestion: int, + dismissed: int, + deduplicated: int, + elapsed_sec: float, +) -> None: + emit_event( + { + "event": "assessment_complete", + "round": round_num, + "critical": critical, + "important": important, + "suggestion": suggestion, + "dismissed": dismissed, + "deduplicated": deduplicated, + "elapsed_sec": round(elapsed_sec, 1), + } + ) + + +def emit_fix_start(round_num: int, findings_to_fix: int) -> None: + emit_event( + { + "event": "fix_start", + "round": round_num, + "findings_to_fix": findings_to_fix, + } + ) + + +def emit_fixes_applied( + round_num: int, + critical_fixed: int, + important_fixed: int, + skipped: int, + elapsed_sec: float, +) -> None: + emit_event( + { + "event": "fixes_applied", + "round": round_num, + "critical_fixed": critical_fixed, + "important_fixed": important_fixed, + "skipped": skipped, + "elapsed_sec": round(elapsed_sec, 1), + } + ) + + +def emit_round_complete( + round_num: int, + status: str, + critical: int, + important: int, + critical_fixed: int, + important_fixed: int, + elapsed_sec: float, +) -> None: + emit_event( + { + "event": "round_complete", + "round": round_num, + "status": status, + "critical": critical, + "important": important, + "critical_fixed": critical_fixed, + "important_fixed": important_fixed, + "elapsed_sec": round(elapsed_sec, 1), + } + ) + + +def emit_converged(round_num: int, total_rounds: int, total_elapsed_sec: float) -> None: + emit_event( + { + "event": "converged", + "round": round_num, + "total_rounds": total_rounds, + "total_elapsed_sec": round(total_elapsed_sec, 1), + } + ) + + +def emit_stalled( + round_num: int, remaining_critical: int, remaining_important: int +) -> None: + emit_event( + { + "event": "stalled", + "round": round_num, + "remaining_critical": remaining_critical, + "remaining_important": remaining_important, + } + ) + + +def emit_error(message: str) -> None: + emit_event({"event": "error", "message": message}) + + +def emit_interrupted() -> None: + emit_event({"event": "interrupted"}) + + +def write_progress_file(state_dir: Path, round_num: int, data: dict) -> None: + """Write progress.json (current) and progress-round-N.json (archive).""" + try: + state_dir.mkdir(parents=True, exist_ok=True) + current = state_dir / "progress.json" + current.write_text(json.dumps(data, indent=2)) + archive = state_dir / f"progress-round-{round_num}.json" + archive.write_text(json.dumps(data, indent=2)) + logger.debug("Progress written to %s", state_dir) + except OSError as e: + logger.error("Failed to write progress file in %s: %s", state_dir, e) + emit_event({"event": "internal_error", "message": f"Failed to write progress: {e}"}) + + +def write_json_file(path: Path, data: Any) -> None: + """Write arbitrary JSON data to a file.""" + try: + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(json.dumps(data, indent=2)) + except OSError as e: + logger.error("Failed to write JSON file %s: %s", path, e) + emit_event({"event": "internal_error", "message": f"Failed to write {path.name}: {e}"}) diff --git a/.claude/skills/convergence-review/prompts.py b/.claude/skills/convergence-review/prompts.py new file mode 100644 index 0000000..b4c20fd --- /dev/null +++ b/.claude/skills/convergence-review/prompts.py @@ -0,0 +1,185 @@ +"""Prompt loading and dynamic perspective discovery for convergence review.""" + +from __future__ import annotations + +import logging +import re +import subprocess +from dataclasses import dataclass +from pathlib import Path + +from models import GATE_CONFIGS, GateConfig + +logger = logging.getLogger("convergence.prompts") + +# Section letter to heading prefix mapping per gate +_SECTION_MAP: dict[str, dict[str, str]] = { + "design-prompts.md": {"A": "DD", "B": "MP", "C": "GDD", "D": "CMP", "E": "GMP"}, + "pr-prompts.md": {"A": "PP", "B": "PC", "C": "XPP", "D": "XPC"}, + "hypothesis-experiment/review-prompts.md": {"A": "DR", "B": "CR", "C": "FR"}, +} + + +@dataclass +class Perspective: + """A single review perspective with its ID and prompt text.""" + + id: str + name: str + prompt: str + + +def _find_skill_dir() -> Path: + """Return this file's parent directory (the convergence-review skill dir).""" + return Path(__file__).parent + + +def _extract_section_perspectives( + content: str, section_letter: str, prompt_file: str +) -> list[Perspective]: + """Extract perspectives from a specific section of a prompt markdown file. + + Parses ### XX-N: headings and extracts the fenced code block following each. + """ + prefix = _SECTION_MAP.get(prompt_file, {}).get(section_letter) + if prefix is None: + raise ValueError( + f"No heading prefix for section {section_letter} in {prompt_file}" + ) + + # Find section boundaries (## Section X: ...) + section_pattern = rf"^## Section {re.escape(section_letter)}:.*$" + section_match = re.search(section_pattern, content, re.MULTILINE) + if not section_match: + raise ValueError(f"Section {section_letter} not found in {prompt_file}") + + section_start = section_match.end() + + # Find next section (## Section ...) or end of file + next_section = re.search(r"^## Section [A-Z]:", content[section_start:], re.MULTILINE) + section_end = section_start + next_section.start() if next_section else len(content) + section_text = content[section_start:section_end] + + # Parse ### XX-N: headings and their code blocks + heading_pattern = rf"^### ({re.escape(prefix)}-\d+):\s*(.+)$" + headings = list(re.finditer(heading_pattern, section_text, re.MULTILINE)) + + perspectives = [] + for i, match in enumerate(headings): + perspective_id = match.group(1) + perspective_name = match.group(2).strip() + + # Extract text between this heading and the next (or section end) + start = match.end() + end = headings[i + 1].start() if i + 1 < len(headings) else len(section_text) + block_text = section_text[start:end] + + # Extract fenced code block content + code_match = re.search(r"```\n(.*?)```", block_text, re.DOTALL) + if code_match: + prompt_text = code_match.group(1).strip() + else: + logger.warning( + "No code block found for %s in %s", perspective_id, prompt_file + ) + prompt_text = block_text.strip() + + perspectives.append(Perspective(perspective_id, perspective_name, prompt_text)) + + logger.info( + "Loaded %d perspectives from %s section %s", + len(perspectives), + prompt_file, + section_letter, + ) + return perspectives + + +def load_perspectives(gate: str) -> list[Perspective]: + """Load all review perspectives for a gate type. + + Dynamically discovers perspectives by parsing ### XX-N: headings + from the gate's prompt file section. + """ + if gate not in GATE_CONFIGS: + raise ValueError( + f"Unknown gate '{gate}'. Valid gates: {', '.join(sorted(GATE_CONFIGS))}" + ) + + config = GATE_CONFIGS[gate] + skill_dir = _find_skill_dir() + prompt_path = skill_dir / config.prompt_file + + # If not found relative to convergence-review/, try relative to .claude/skills/ + if not prompt_path.exists(): + skills_root = skill_dir.parent + prompt_path = skills_root / config.prompt_file + + if not prompt_path.exists(): + raise FileNotFoundError(f"Prompt file not found: {prompt_path}") + + content = prompt_path.read_text() + return _extract_section_perspectives(content, config.prompt_section, config.prompt_file) + + +def load_artifact_content(gate: str, artifact_path: str | None) -> str: + """Load artifact content based on gate type. + + Returns the text content to be reviewed. + """ + config = GATE_CONFIGS[gate] + + if config.artifact_type == "git_diff": + # Use git diff HEAD to capture both staged and unstaged changes + try: + result = subprocess.run( + ["git", "diff", "HEAD"], + capture_output=True, + text=True, + check=False, + timeout=60, + ) + except FileNotFoundError: + raise RuntimeError("git is not installed or not in PATH") from None + if result.returncode != 0: + raise RuntimeError(f"git diff failed: {result.stderr}") + content = result.stdout.strip() + if not content: + raise ValueError("git diff is empty — nothing to review") + return content + + if artifact_path is None: + raise ValueError(f"Gate '{gate}' requires an artifact path") + + path = Path(artifact_path) + + if config.artifact_type == "file": + if not path.exists(): + raise FileNotFoundError(f"Artifact not found: {path}") + return path.read_text() + + if config.artifact_type == "directory": + # h-code: concatenate run.sh + analyze.py from artifact directory + parts = [] + for filename in ("run.sh", "analyze.py"): + file_path = path / filename + if file_path.exists(): + parts.append(f"=== {filename} ===\n{file_path.read_text()}") + else: + logger.warning("Expected file not found: %s", file_path) + if not parts: + raise FileNotFoundError( + f"No run.sh or analyze.py found in {path}" + ) + return "\n\n".join(parts) + + raise ValueError(f"Unknown artifact type: {config.artifact_type}") + + +def get_gate_config(gate: str) -> GateConfig: + """Get gate configuration, raising ValueError for unknown gates.""" + if gate not in GATE_CONFIGS: + raise ValueError( + f"Unknown gate '{gate}'. Valid gates: {', '.join(sorted(GATE_CONFIGS))}" + ) + return GATE_CONFIGS[gate] diff --git a/.claude/skills/convergence-review/reviewer.py b/.claude/skills/convergence-review/reviewer.py new file mode 100644 index 0000000..be8711e --- /dev/null +++ b/.claude/skills/convergence-review/reviewer.py @@ -0,0 +1,488 @@ +"""Async reviewer dispatch via OpenAI-compatible API.""" + +from __future__ import annotations + +import asyncio +import json +import logging +import re +import time +from dataclasses import asdict +from pathlib import Path + +from openai import APIConnectionError, APIStatusError, APITimeoutError, AsyncOpenAI + +from models import Finding, ReviewerOutput +from progress import emit_reviewer_done, emit_reviewer_error, write_json_file +from prompts import Perspective + +logger = logging.getLogger("convergence.reviewer") + +# Retry config for API errors +_MAX_RETRIES = 3 +_RETRY_DELAYS = [2, 4, 8] # seconds + +# Patterns in prompt templates that should be replaced with artifact content +_PLACEHOLDER_RE = re.compile(r"]+>|]+>") + + +_OUTPUT_FORMAT_SUFFIX = """ + +## Required Output Format + +You MUST respond with ONLY a JSON object. No markdown, no commentary, no explanation outside the JSON. + +```json +{ + "perspective": "", + "findings": [ + { + "severity": "CRITICAL", + "description": "Clear description of the issue", + "location": "file:line or section reference if applicable, else null", + "evidence": "Quote or reference from the artifact supporting this finding" + } + ] +} +``` + +Severity definitions: +- CRITICAL: Must fix. Missing control, contradicted status, silent data loss, cross-document contradiction. +- IMPORTANT: Should fix. Would reader be misled? Would a conclusion change? +- SUGGESTION: Cosmetic. Style, terminology, minor clarity improvements. + +If you find no issues, return: {"perspective": "", "findings": []} +""" + + +def _inject_artifact( + prompt: str, + artifact_content: str, + prior_round_context: str | None = None, +) -> str: + """Replace placeholder patterns in prompt with actual artifact content, + and append structured output format instructions. + + Handles three placeholder styles: + - / tags (legacy) + - ARTIFACT_PATH literal (pr-prompts.md convention) + - "First, read the plan file at ARTIFACT_PATH..." instruction lines + """ + # re.sub treats \w in replacement as backreference — escape it + filled = _PLACEHOLDER_RE.sub(lambda _: artifact_content, prompt) + + # Replace ARTIFACT_PATH and rewrite "read the file" instructions since + # API-based reviewers can't read files — inject the content directly. + if "ARTIFACT_PATH" in filled: + # Remove "First, read ... using the Read tool." instruction lines + filled = re.sub( + r"^First,.*(?:Read tool|Bash tool|git diff).*$\n?", + "", + filled, + flags=re.MULTILINE, + ) + filled = filled.replace("ARTIFACT_PATH", "") + + # Append prior round context if available (round 2+) + context_section = "" + if prior_round_context: + context_section = "\n\n" + prior_round_context + + # Append artifact content and output format + return ( + filled.rstrip() + + context_section + + "\n\n## Artifact Content\n\n" + + artifact_content + + _OUTPUT_FORMAT_SUFFIX + ) + + +def _parse_reviewer_response(perspective_id: str, text: str) -> list[Finding]: + """Parse reviewer output, trying JSON first then falling back to markdown. + + JSON format: + {"findings": [{"severity": "CRITICAL", "description": "..."}]} + + Markdown format (model's natural output): + 1. [CRITICAL] Description of finding + - **[CRITICAL]** Description of finding + **CRITICAL**: Description of finding + """ + # Try JSON first + findings = _try_parse_json(perspective_id, text) + if findings is not None: + return findings + + # Fall back to markdown/text parsing + findings = _parse_markdown_findings(perspective_id, text) + if findings: + logger.info( + "%s: parsed %d findings from markdown output", + perspective_id, + len(findings), + ) + else: + logger.warning( + "%s: no findings parsed from %d char response", + perspective_id, + len(text), + ) + return findings + + +def _fix_json_escapes(raw: str) -> str: + r"""Fix invalid JSON escape sequences produced by LLMs quoting code/regex. + + Models often embed regex patterns (e.g. \w, \d, \.) in JSON string values. + These are valid regex but invalid JSON escapes. We must handle mixed + content where some backslashes are already properly escaped (\\w) while + others are not (\w). + + Strategy: temporarily protect valid \\ pairs, fix remaining bad escapes, + then restore. + """ + _SENTINEL = "\x00BKSL\x00" + # Protect already-escaped backslashes + protected = raw.replace("\\\\", _SENTINEL) + # Fix remaining lone backslashes not followed by valid JSON escape chars + fixed = re.sub(r'\\(?!["\\/bfnrtu])', r'\\\\', protected) + # Restore protected backslashes + return fixed.replace(_SENTINEL, "\\\\") + + +def _try_parse_json(perspective_id: str, text: str) -> list[Finding] | None: + """Try to parse response as JSON. Returns None if not JSON.""" + # Try direct parse + try: + data = json.loads(text) + return _extract_json_findings(perspective_id, data) + except json.JSONDecodeError as e: + logger.debug("%s: direct JSON parse failed: %s", perspective_id, e) + + # Try finding a JSON code block + json_block = re.search(r"```(?:json)?\s*\n(\{.*\})\s*\n```", text, re.DOTALL) + if json_block: + raw = json_block.group(1) + for attempt_text in (raw, _fix_json_escapes(raw)): + try: + data = json.loads(attempt_text) + return _extract_json_findings(perspective_id, data) + except json.JSONDecodeError as e: + logger.debug("%s: code block JSON parse failed: %s", perspective_id, e) + + # Try direct parse with escape fix (no code block) + try: + data = json.loads(_fix_json_escapes(text)) + return _extract_json_findings(perspective_id, data) + except json.JSONDecodeError: + pass + + return None + + +def _extract_json_findings(perspective_id: str, data: dict) -> list[Finding]: + """Extract findings from a parsed JSON dict.""" + findings_data = data.get("findings", []) + findings = [] + for f in findings_data: + raw_severity = f.get("severity", "SUGGESTION") + severity = raw_severity.upper() + if severity not in ("CRITICAL", "IMPORTANT", "SUGGESTION"): + logger.warning( + "%s: unknown severity %r demoted to SUGGESTION", + perspective_id, raw_severity, + ) + severity = "SUGGESTION" + findings.append( + Finding( + perspective=perspective_id, + severity=severity, + description=f.get("description", ""), + location=f.get("location"), + evidence=f.get("evidence"), + ) + ) + return findings + + +# Patterns to match severity tags in various markdown formats +_SEVERITY_PATTERNS = [ + # 1. [CRITICAL] description or - [CRITICAL] description + re.compile( + r"^[\s\-\*\d.]*\[(?PCRITICAL|IMPORTANT|SUGGESTION)\]\s*(?P.+)", + re.MULTILINE, + ), + # **[CRITICAL]** description or **CRITICAL**: description + re.compile( + r"\*\*\[?(?PCRITICAL|IMPORTANT|SUGGESTION)\]?\*\*[:\s]+(?P.+)", + re.MULTILINE, + ), + # CRITICAL: description (standalone line) + re.compile( + r"^(?PCRITICAL|IMPORTANT|SUGGESTION):\s+(?P.+)", + re.MULTILINE, + ), + # Severity: CRITICAL — or — Rating: CRITICAL (followed by description on next context) + re.compile( + r"(?:Severity|Rating)[:\s]+\*?\*?(?PCRITICAL|IMPORTANT|SUGGESTION)\*?\*?", + re.MULTILINE, + ), +] + + +def _parse_markdown_findings(perspective_id: str, text: str) -> list[Finding]: + """Extract findings from markdown-formatted reviewer output.""" + findings = [] + seen_descriptions = set() + + for pattern in _SEVERITY_PATTERNS: + for match in pattern.finditer(text): + severity = match.group("sev").upper() + desc = match.group("desc").strip() if "desc" in match.groupdict() else "" + + # Skip if no description (severity-only pattern) + if not desc: + continue + + # Clean up markdown formatting + desc = re.sub(r"\*\*", "", desc).strip() + desc = re.sub(r"^\W+", "", desc).strip() + + # Deduplicate by first 80 chars of description + dedup_key = desc[:80].lower() + if dedup_key in seen_descriptions: + continue + seen_descriptions.add(dedup_key) + + if severity not in ("CRITICAL", "IMPORTANT", "SUGGESTION"): + severity = "SUGGESTION" + + findings.append( + Finding( + perspective=perspective_id, + severity=severity, + description=desc, + ) + ) + + return findings + + +_EXTRACTION_PROMPT = """\ +The following is a review response that could not be parsed as JSON. \ +Extract ALL findings from it and return them as a JSON object. + +You MUST respond with ONLY a JSON object — no markdown, no commentary. + +```json +{ + "findings": [ + { + "severity": "CRITICAL or IMPORTANT or SUGGESTION", + "description": "Clear description of the issue", + "location": "file:line or section reference if applicable, else null", + "evidence": "Quote or reference from the review supporting this finding" + } + ] +} +``` + +If the review found no issues, return: {"findings": []} + +--- BEGIN REVIEW RESPONSE --- +""" + + +async def _extract_findings_fallback( + client: AsyncOpenAI, + model: str, + perspective_id: str, + raw_text: str, + timeout: float, +) -> list[Finding] | None: + """Fallback: ask an LLM to extract findings from an unparseable response.""" + prompt = _EXTRACTION_PROMPT + raw_text + "\n--- END REVIEW RESPONSE ---" + try: + response = await asyncio.wait_for( + client.chat.completions.create( + model=model, + messages=[{"role": "user", "content": prompt}], + temperature=0.0, + ), + timeout=timeout, + ) + if not response.choices: + return None + extraction = response.choices[0].message.content or "" + findings = _try_parse_json(perspective_id, extraction) + if findings is not None: + logger.info( + "%s: fallback extraction recovered %d findings", + perspective_id, + len(findings), + ) + else: + logger.warning( + "%s: fallback extraction also failed to parse", + perspective_id, + ) + return findings + except (asyncio.TimeoutError, APIConnectionError, APIStatusError, APITimeoutError) as e: + logger.warning("%s: fallback extraction failed: %s", perspective_id, e) + return None + + +async def run_reviewer( + client: AsyncOpenAI, + model: str, + perspective: Perspective, + artifact_content: str, + timeout: float, + round_num: int, + prior_round_context: str | None = None, +) -> ReviewerOutput: + """Run a single reviewer perspective via OpenAI chat completion.""" + start = time.monotonic() + + # Inject artifact content into prompt placeholders + prompt_with_artifact = _inject_artifact( + perspective.prompt, artifact_content, prior_round_context + ) + + for attempt in range(_MAX_RETRIES): + try: + response = await asyncio.wait_for( + client.chat.completions.create( + model=model, + messages=[ + {"role": "user", "content": prompt_with_artifact}, + ], + temperature=0.0, + ), + timeout=timeout, + ) + elapsed = time.monotonic() - start + if not response.choices: + raise ValueError(f"{perspective.id}: API returned empty choices list") + text = response.choices[0].message.content or "" + logger.debug( + "%s raw response (%d chars): %s", + perspective.id, + len(text), + text[:500], + ) + findings = _parse_reviewer_response(perspective.id, text) + + # Detect non-empty response that yielded no findings (possible parse failure) + parse_warning = None + if not findings and len(text.strip()) > 50: + logger.warning( + "%s: non-empty response (%d chars) yielded 0 findings — trying fallback extraction", + perspective.id, + len(text), + ) + fallback = await _extract_findings_fallback( + client, model, perspective.id, text, timeout, + ) + if fallback: + findings = fallback + else: + parse_warning = ( + f"non-empty response ({len(text)} chars) yielded 0 findings" + ) + + logger.info( + "%s completed: %d findings in %.1fs", + perspective.id, + len(findings), + elapsed, + ) + emit_reviewer_done(round_num, perspective.id, len(findings), elapsed) + return ReviewerOutput( + perspective=perspective.id, + findings=findings, + parse_warning=parse_warning, + elapsed_sec=elapsed, + ) + + except asyncio.TimeoutError: + elapsed = time.monotonic() - start + logger.warning("%s timed out after %.1fs", perspective.id, elapsed) + emit_reviewer_error(round_num, perspective.id, "timeout", elapsed) + return ReviewerOutput( + perspective=perspective.id, + error="timeout", + elapsed_sec=elapsed, + ) + + except (APIConnectionError, APIStatusError, APITimeoutError) as e: + if attempt < _MAX_RETRIES - 1: + delay = _RETRY_DELAYS[attempt] + logger.warning( + "%s attempt %d failed: %s, retrying in %ds", + perspective.id, + attempt + 1, + e, + delay, + ) + await asyncio.sleep(delay) + else: + elapsed = time.monotonic() - start + error_msg = f"failed after {_MAX_RETRIES} attempts: {e}" + logger.error("%s %s", perspective.id, error_msg) + emit_reviewer_error(round_num, perspective.id, error_msg, elapsed) + return ReviewerOutput( + perspective=perspective.id, + error=error_msg, + elapsed_sec=elapsed, + ) + + # Unreachable, but satisfy type checker + elapsed = time.monotonic() - start + return ReviewerOutput(perspective=perspective.id, error="unknown", elapsed_sec=elapsed) + + +async def run_all_reviewers( + client: AsyncOpenAI, + model: str, + perspectives: list[Perspective], + artifact_content: str, + timeout: float, + round_num: int, + state_dir: Path, + prior_round_context: str | None = None, +) -> list[ReviewerOutput]: + """Dispatch all reviewers in parallel and collect results.""" + tasks = [ + run_reviewer( + client, model, p, artifact_content, timeout, round_num, + prior_round_context, + ) + for p in perspectives + ] + raw_results = await asyncio.gather(*tasks, return_exceptions=True) + + # Convert exceptions to error outputs + results: list[ReviewerOutput] = [] + for i, r in enumerate(raw_results): + if isinstance(r, BaseException): + pid = perspectives[i].id if i < len(perspectives) else f"unknown-{i}" + logger.error("%s raised unexpected exception: %s", pid, r) + results.append(ReviewerOutput(perspective=pid, error=f"internal error: {r}")) + else: + results.append(r) + + # Save raw reviewer outputs to state directory + reviewers_dir = state_dir / f"reviewers-round-{round_num}" + reviewers_dir.mkdir(parents=True, exist_ok=True) + for output in results: + data = { + "perspective": output.perspective, + "findings": [asdict(f) for f in output.findings], + "error": output.error, + "elapsed_sec": output.elapsed_sec, + } + write_json_file(reviewers_dir / f"{output.perspective}.json", data) + + return list(results) diff --git a/.claude/skills/convergence-review/summary.py b/.claude/skills/convergence-review/summary.py new file mode 100644 index 0000000..c5dcf82 --- /dev/null +++ b/.claude/skills/convergence-review/summary.py @@ -0,0 +1,331 @@ +"""Human-readable summary file writer for convergence reviews.""" + +from __future__ import annotations + +import logging +import shutil +from datetime import datetime +from pathlib import Path + +from models import Assessment, FixResult, ReviewerOutput + +logger = logging.getLogger("convergence.summary") + + +def _compute_summary_path(artifact: str | None, state_dir: Path) -> Path: + """Compute primary summary path: next to the artifact if possible, else state dir. + + For file artifacts: -review.md in the same directory. + For git_diff / directory / None: state_dir/summary.md. + """ + if artifact: + artifact_path = Path(artifact) + if artifact_path.is_file(): + return artifact_path.parent / f"{artifact_path.stem}-review.md" + return state_dir / "summary.md" + + +class SummaryWriter: + """Accumulates round results into a readable summary.md file.""" + + def __init__( + self, + state_dir: Path, + gate: str, + artifact: str | None, + max_rounds: int, + reviewer_model: str = "", + fixer_model: str = "", + ): + self.path = _compute_summary_path(artifact, state_dir) + self.state_dir_copy = state_dir / "summary.md" + self.gate = gate + self.artifact = artifact or "git-diff" + self.max_rounds = max_rounds + self._round_history: list[dict] = [] + + # Append header (preserves previous reviews) + self.path.parent.mkdir(parents=True, exist_ok=True) + timestamp = datetime.now().strftime("%Y-%m-%d %H:%M") + separator = "\n---\n\n" if self.path.exists() and self.path.stat().st_size > 0 else "" + with open(self.path, "a") as f: + f.write( + f"{separator}# Convergence Review: {gate} — {self.artifact}\n" + f"**Started**: {timestamp} | " + f"**Max rounds**: {max_rounds} | " + f"**Reviewer**: {reviewer_model} | " + f"**Fixer**: {fixer_model}\n\n" + ) + logger.info("Summary file: %s", self.path) + + def _sync_to_state_dir(self) -> None: + """Copy summary to state dir if primary path differs. Fails silently.""" + if self.path != self.state_dir_copy: + try: + shutil.copy2(self.path, self.state_dir_copy) + except OSError as e: + logger.warning("Failed to sync summary to state dir: %s", e) + + @staticmethod + def _ts() -> str: + """Return a compact timestamp for action steps.""" + return datetime.now().strftime("%H:%M:%S") + + def _append(self, text: str) -> None: + """Append text and sync to state dir. Degrades gracefully on I/O errors.""" + try: + with open(self.path, "a") as f: + f.write(text) + self._sync_to_state_dir() + except OSError as e: + logger.error("Failed to write summary file %s: %s", self.path, e) + + def write_round_start(self, round_num: int, num_perspectives: int) -> None: + """Append round header — written before reviewers start.""" + self._append( + f"## Round {round_num}/{self.max_rounds}\n\n" + f"- `{self._ts()}` **Dispatching** {num_perspectives} reviewers...\n\n" + ) + + def write_review_complete( + self, + reviewer_outputs: list[ReviewerOutput], + elapsed_sec: float, + ) -> None: + """Append review phase results.""" + ok = sum(1 for o in reviewer_outputs if o.error is None) + failed = len(reviewer_outputs) - ok + total_raw = sum(len(o.findings) for o in reviewer_outputs) + line = f"- `{self._ts()}` **Review complete**: {ok} reviewers, {total_raw} raw findings" + if failed: + line += f" ({failed} failed)" + line += f" ({elapsed_sec:.0f}s)\n\n" + self._append(line) + + def write_assess_start(self, total_raw: int) -> None: + """Append assessment start line.""" + self._append(f"- `{self._ts()}` **Assessing** {total_raw} raw findings...\n\n") + + def write_fix_start(self, critical: int, important: int) -> None: + """Append fix start line.""" + self._append(f"- `{self._ts()}` **Fixing** {critical}C {important}I...\n\n") + + def write_assessment_complete(self, assessment: Assessment) -> None: + """Append assessment phase results with finding list.""" + s = assessment.summary + fallback_note = " **[FALLBACK: raw findings, no dedup/validation]**" if assessment.fallback else "" + lines = [ + f"- `{self._ts()}` **Assessment**: {s.get('critical', 0)} CRITICAL, " + f"{s.get('important', 0)} IMPORTANT, " + f"{s.get('suggestion', 0)} SUGGESTION " + f"({s.get('dismissed', 0)} dismissed, " + f"{s.get('deduplicated', 0)} deduped){fallback_note}\n" + ] + for f in assessment.assessed_findings: + if f.severity in ("CRITICAL", "IMPORTANT"): + status_tag = "" + if f.cross_round_status and f.cross_round_status != "novel": + ref = f" see {f.prior_finding_ref}" if f.prior_finding_ref else "" + status_tag = f" *({f.cross_round_status}{ref})*" + lines.append( + f" - **{f.id}** [{f.severity}] {f.description}{status_tag}\n" + ) + lines.append("\n") + self._append("".join(lines)) + + def write_fix_complete(self, fix_result: FixResult | None, converged: bool = False) -> None: + """Append fix phase results.""" + if fix_result: + if fix_result.error: + self._append(f"- `{self._ts()}` **Fix**: FAILED — {fix_result.error}\n\n") + else: + c_fixed = sum( + 1 for f in fix_result.fixes_applied + if f.get("severity") == "CRITICAL" + ) + i_fixed = sum( + 1 for f in fix_result.fixes_applied + if f.get("severity") == "IMPORTANT" + ) + skipped = len(fix_result.fixes_skipped) + self._append(f"- `{self._ts()}` **Fix complete**: {c_fixed}C {i_fixed}I fixed, {skipped} skipped\n\n") + elif converged: + self._append(f"- `{self._ts()}` **Fix**: not needed (0 CRITICAL, 0 IMPORTANT)\n\n") + + def write_round_summary( + self, + assessment: Assessment | None, + fix_result: FixResult | None, + ) -> None: + """Append a compact round summary showing actionable count and delta.""" + if not assessment: + return + + ci = [ + f for f in assessment.assessed_findings + if f.severity in ("CRITICAL", "IMPORTANT") + ] + + non_actionable = {"recurring-fixed", "recurring-escalate"} + actionable_count = sum(1 for f in ci if f.cross_round_status not in non_actionable) + + if actionable_count == 0: + self._append(" > **Actionable: 0** — converged\n\n") + return + + # Compute delta from prior round + prev_actionable = None + if self._round_history: + prev_actionable = self._round_history[-1].get("actionable") + + # Headline: actionable count with delta + headline = f" > **Actionable: {actionable_count}**" + if prev_actionable is not None: + delta = actionable_count - prev_actionable + arrow = "↓" if delta < 0 else ("↑" if delta > 0 else "→") + headline += f" ({arrow}{abs(delta)} from round {self._round_history[-1]['round']})" + headline += "\n" + + # Breakdown by cross-round status + from collections import Counter + statuses = Counter(f.cross_round_status for f in ci) + breakdown_parts = [] + for s, label in [ + ("novel", "novel"), + ("regression", "regression"), + ("recurring-skipped", "recurring-skipped"), + ]: + if statuses.get(s, 0) > 0: + breakdown_parts.append(f"{statuses[s]} {label}") + breakdown = f" > Breakdown: {', '.join(breakdown_parts)}\n" if breakdown_parts else "" + + # Filtered count + filtered = sum(1 for f in ci if f.cross_round_status in non_actionable) + filtered_line = f" > Filtered: {filtered} recurring-fixed/escalated\n" if filtered > 0 else "" + + # Fixes + fix_line = "" + if fix_result and not fix_result.error: + applied = len(fix_result.fixes_applied) + skipped = len(fix_result.fixes_skipped) + fix_line = f" > Fixes: {applied} applied, {skipped} skipped\n" + elif fix_result and fix_result.error: + fix_line = f" > Fixes: FAILED — {fix_result.error}\n" + + self._append(f"{headline}{breakdown}{filtered_line}{fix_line}\n") + + def write_recurring(self, recurring: int, total: int, escalated: int = 0) -> None: + """Append recurring findings note after assessment.""" + if recurring > 0: + esc_note = f" ({escalated} escalated as persistent)" if escalated else "" + self._append( + f" - *Recurring*: {recurring}/{total} findings from prior rounds{esc_note}\n" + ) + + def record_round( + self, + round_num: int, + assessment: Assessment | None = None, + fix_result: FixResult | None = None, + ) -> None: + """Record round stats for the history table.""" + row: dict = {"round": round_num} + if assessment: + s = assessment.summary + row["critical"] = s.get("critical", 0) + row["important"] = s.get("important", 0) + row["suggestion"] = s.get("suggestion", 0) + row["dismissed"] = s.get("dismissed", 0) + + # Cross-round status breakdown from assessed findings + from collections import Counter + ci = [f for f in assessment.assessed_findings if f.severity in ("CRITICAL", "IMPORTANT")] + statuses = Counter(f.cross_round_status for f in ci) + non_actionable = {"recurring-fixed", "recurring-escalate"} + row["novel"] = statuses.get("novel", 0) + row["regression"] = statuses.get("regression", 0) + row["re_skip"] = statuses.get("recurring-skipped", 0) + row["re_fixed"] = statuses.get("recurring-fixed", 0) + row["escalated"] = statuses.get("recurring-escalate", 0) + row["actionable"] = sum(1 for f in ci if f.cross_round_status not in non_actionable) + if fix_result and not fix_result.error: + row["applied"] = len(fix_result.fixes_applied) + row["skipped"] = len(fix_result.fixes_skipped) + self._round_history.append(row) + + def _write_history_table(self) -> None: + """Append a progress table showing convergence trajectory across rounds.""" + if not self._round_history: + return + lines = [ + "\n## Progress\n\n", + "| Round | Actionable | Novel | Regress | Re-skip | Re-fixed | Escalated | Fixed | Skipped |\n", + "|------:|-----------:|------:|--------:|--------:|---------:|----------:|------:|--------:|\n", + ] + for row in self._round_history: + act = row.get("actionable", "-") + nov = row.get("novel", "-") + reg = row.get("regression", "-") + rsk = row.get("re_skip", "-") + rfx = row.get("re_fixed", "-") + esc = row.get("escalated", "-") + a = row.get("applied", "-") + sk = row.get("skipped", "-") + # Round 1 has no cross-round data; show "—" for inapplicable columns + if row["round"] == 1: + rfx = "—" + esc = "—" + lines.append( + f"| {row['round']} | {act} | {nov} | {reg} | {rsk} | {rfx} | {esc} | {a} | {sk} |\n" + ) + lines.append("\n") + self._append("".join(lines)) + + def write_persistent_themes(self, themes: list[dict]) -> None: + """Append persistent themes section — design trade-offs the loop couldn't resolve.""" + if not themes: + return + lines = [ + "\n## Persistent Themes (not blocking convergence)\n\n", + "These themes were flagged in 3+ rounds and represent design trade-offs or " + "scope limitations rather than fixable defects. Human review recommended.\n\n", + "| # | Severity | Theme | Escalated in Round |\n", + "|--:|----------|-------|-------------------:|\n", + ] + for i, t in enumerate(themes, 1): + desc = t.get("description", "")[:200] + sev = t.get("severity", "?") + rd = t.get("round_escalated", "?") + lines.append(f"| {i} | {sev} | {desc} | {rd} |\n") + lines.append("\n") + self._append("".join(lines)) + + def write_converged(self, round_num: int, total_elapsed: float) -> None: + """Append convergence result.""" + self._append( + f"## Result: CONVERGED — `{self._ts()}`\n\n" + f"Converged in {round_num} round(s) ({total_elapsed:.0f}s total). " + f"0 actionable findings remaining.\n" + ) + self._write_history_table() + + def write_stalled( + self, + round_num: int, + remaining_critical: int, + remaining_important: int, + reason: str = "max rounds reached", + ) -> None: + """Append stall result.""" + self._append( + f"## Result: STALLED — `{self._ts()}`\n\n" + f"**Reason**: {reason}\n\n" + f"{remaining_critical} CRITICAL, {remaining_important} IMPORTANT remaining " + f"after {round_num} round(s).\n" + ) + self._write_history_table() + + def write_error(self, message: str) -> None: + """Append error result.""" + self._append(f"## Result: ERROR\n\n{message}\n") + self._write_history_table() diff --git a/.claude/skills/convergence-review/tests/__init__.py b/.claude/skills/convergence-review/tests/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/.claude/skills/convergence-review/tests/conftest.py b/.claude/skills/convergence-review/tests/conftest.py new file mode 100644 index 0000000..7d0bf1f --- /dev/null +++ b/.claude/skills/convergence-review/tests/conftest.py @@ -0,0 +1,78 @@ +"""Shared pytest fixtures for convergence tests.""" + +from __future__ import annotations + +import json +import sys +from pathlib import Path + +import pytest + +# Ensure skill dir is on sys.path (also done in helpers.py but needed early) +SKILL_DIR = Path(__file__).resolve().parent.parent +if str(SKILL_DIR) not in sys.path: + sys.path.insert(0, str(SKILL_DIR)) + +# Ensure tests dir is on sys.path so `import helpers` works +TESTS_DIR = Path(__file__).resolve().parent +if str(TESTS_DIR) not in sys.path: + sys.path.insert(0, str(TESTS_DIR)) + +from helpers import FakePerspective + + +@pytest.fixture +def env_vars(monkeypatch): + """Set required env vars for the orchestrator.""" + monkeypatch.setenv("LITELLM_BASE_URL", "http://test.local/v1") + monkeypatch.setenv("LITELLM_API_KEY", "test-key-123") + + +@pytest.fixture +def work_dir(tmp_path, monkeypatch): + """Change cwd to tmp_path so state_dir is always under cwd.""" + monkeypatch.chdir(tmp_path) + return tmp_path + + +@pytest.fixture +def artifact_file(work_dir): + """Write a dummy artifact .md file under work_dir and return its path.""" + p = work_dir / "artifact.md" + p.write_text("# Test Artifact\n\nThis is a test document for review.\n") + return p + + +@pytest.fixture +def state_dir(work_dir): + """Return a state dir path under work_dir (not pre-created).""" + return work_dir / ".claude" / "convergence-state" / "test" + + +def harvest_events(capsys) -> list[dict]: + """Parse all JSON event lines from captured stdout. + + Fails if any non-empty line is not valid JSON — prevents silent event loss. + """ + out = capsys.readouterr().out + events = [] + bad_lines = [] + for line in out.strip().splitlines(): + line = line.strip() + if line: + try: + events.append(json.loads(line)) + except json.JSONDecodeError: + bad_lines.append(line) + assert not bad_lines, f"Unparseable JSON lines in stdout: {bad_lines}" + return events + + +@pytest.fixture +def default_perspectives(): + """Return a list of 3 fake perspectives for testing.""" + return [ + FakePerspective("DD-1", "Architecture"), + FakePerspective("DD-2", "Invariants"), + FakePerspective("DD-3", "Extension"), + ] diff --git a/.claude/skills/convergence-review/tests/helpers.py b/.claude/skills/convergence-review/tests/helpers.py new file mode 100644 index 0000000..0d0b22e --- /dev/null +++ b/.claude/skills/convergence-review/tests/helpers.py @@ -0,0 +1,128 @@ +"""Shared mock factories and helpers for convergence tests.""" + +from __future__ import annotations + +import asyncio +import json +import sys +from dataclasses import dataclass +from pathlib import Path +from unittest.mock import MagicMock + +# Add skill directory to sys.path so tests can import orchestrator, models, etc. +SKILL_DIR = Path(__file__).resolve().parent.parent +if str(SKILL_DIR) not in sys.path: + sys.path.insert(0, str(SKILL_DIR)) + + +@dataclass +class FakePerspective: + id: str + name: str + prompt: str = "" + + def __post_init__(self): + if not self.prompt: + self.prompt = f"[{self.id}] Review for issues." + + +def make_reviewer_response(perspective_id: str, findings: list[dict]) -> dict: + return {"perspective": perspective_id, "findings": findings} + + +def make_assessment_response( + assessed_findings: list[dict] | None = None, + dismissed: list[dict] | None = None, + uncovered: list[str] | None = None, + summary: dict | None = None, +) -> dict: + af = assessed_findings or [] + d = dismissed or [] + s = summary or { + "critical": sum(1 for f in af if f.get("severity") == "CRITICAL"), + "important": sum(1 for f in af if f.get("severity") == "IMPORTANT"), + "suggestion": sum(1 for f in af if f.get("severity") == "SUGGESTION"), + "dismissed": len(d), + "deduplicated": 0, + } + return { + "assessed_findings": af, + "dismissed": d, + "uncovered_perspectives": uncovered or [], + "summary": s, + } + + +def _make_chat_response(content: str) -> MagicMock: + msg = MagicMock() + msg.content = content + choice = MagicMock() + choice.message = msg + resp = MagicMock() + resp.choices = [choice] + return resp + + +def make_round_mock( + reviewer_responses: dict[str, dict], + assessor_response: dict, + reviewer_model: str = "openai/test-reviewer", + fixer_model: str = "openai/test-fixer", +): + async def _side_effect(*, model, messages, temperature=0.0, **kwargs): + if model == fixer_model: + return _make_chat_response(json.dumps(assessor_response)) + if model == reviewer_model: + user_msg = messages[-1]["content"] if messages else "" + for pid, resp in reviewer_responses.items(): + if pid in user_msg: + return _make_chat_response(json.dumps(resp)) + raise ValueError( + f"No reviewer response matched message content. " + f"Expected one of {list(reviewer_responses.keys())} in message: {user_msg[:200]!r}" + ) + raise ValueError( + f"Unexpected model in mock: {model!r} " + f"(expected {reviewer_model!r} or {fixer_model!r})" + ) + + return _side_effect + + +def make_error_side_effect( + error_perspectives: set[str] | None = None, + reviewer_responses: dict[str, dict] | None = None, + assessor_response: dict | None = None, + reviewer_model: str = "openai/test-reviewer", + fixer_model: str = "openai/test-fixer", +): + reviewer_responses = reviewer_responses or {} + assessor_response = assessor_response or make_assessment_response() + + async def _side_effect(*, model, messages, temperature=0.0, **kwargs): + if model == fixer_model: + return _make_chat_response(json.dumps(assessor_response)) + if model == reviewer_model: + user_msg = messages[-1]["content"] if messages else "" + for pid in (error_perspectives or set()): + if pid in user_msg: + raise Exception(f"Simulated failure for {pid}") + for pid, resp in reviewer_responses.items(): + if pid in user_msg: + return _make_chat_response(json.dumps(resp)) + raise ValueError( + f"No reviewer response matched message content. " + f"Expected one of {list(reviewer_responses.keys())} or " + f"error for {error_perspectives} in message: {user_msg[:200]!r}" + ) + raise ValueError( + f"Unexpected model in mock: {model!r} " + f"(expected {reviewer_model!r} or {fixer_model!r})" + ) + + return _side_effect + + +def run_main(argv: list[str]) -> int: + import orchestrator + return asyncio.run(orchestrator.main(argv)) diff --git a/.claude/skills/convergence-review/tests/test_assessment.py b/.claude/skills/convergence-review/tests/test_assessment.py new file mode 100644 index 0000000..b476ce9 --- /dev/null +++ b/.claude/skills/convergence-review/tests/test_assessment.py @@ -0,0 +1,149 @@ +"""BC-5, BC-6, BC-7: Assessment deduplication, severity independence, dismissal.""" + +from __future__ import annotations + +import json +from unittest.mock import patch, MagicMock + +import pytest + +from conftest import harvest_events +from helpers import ( + make_assessment_response, + make_reviewer_response, + make_round_mock, + run_main, +) +from models import FixResult + +REVIEWER_MODEL = "openai/test-reviewer" +FIXER_MODEL = "openai/test-fixer" + + +def _run_with_assessor( + artifact_file, state_dir, + default_perspectives, assessor_response, + reviewer_findings=None, +): + """Helper: run 1 round with specific assessor response, return exit code.""" + if reviewer_findings is None: + reviewer_findings = [{"severity": "CRITICAL", "description": "Some finding"}] + + reviewer_resp = { + p.id: make_reviewer_response(p.id, reviewer_findings) + for p in default_perspectives + } + + mock_client = MagicMock() + mock_client.chat.completions.create = make_round_mock( + reviewer_resp, assessor_response, + reviewer_model=REVIEWER_MODEL, fixer_model=FIXER_MODEL, + ) + + argv = [ + "design", str(artifact_file), + "--reviewer-model", REVIEWER_MODEL, + "--fixer-model", FIXER_MODEL, + "--max-rounds", "1", + "--state-dir", str(state_dir), + "--force", + ] + + with ( + patch("orchestrator.load_perspectives", return_value=default_perspectives), + patch("orchestrator.load_artifact_content", return_value="test content"), + patch("orchestrator.AsyncOpenAI", return_value=mock_client), + patch("orchestrator.apply_fixes", return_value=FixResult( + fixes_applied=[{"finding_id": "F-1", "severity": "CRITICAL", "action": "fixed"}], + )), + ): + return run_main(argv) + + +class TestDeduplication: + """BC-5: Assessor merges findings from multiple perspectives into one.""" + + def test_deduplication( + self, env_vars, artifact_file, state_dir, capsys, default_perspectives + ): + assessor = make_assessment_response( + assessed_findings=[{ + "id": "F-1", "severity": "CRITICAL", + "original_severity": "CRITICAL", + "description": "Missing invariant check", + "source_perspectives": ["DD-1", "DD-2"], + "assessment_rationale": "Both perspectives found the same issue", + }], + ) + + _run_with_assessor(artifact_file, state_dir, default_perspectives, assessor) + + assessment_files = list(state_dir.glob("assessment-round-*.json")) + assert len(assessment_files) >= 1 + data = json.loads(assessment_files[0].read_text()) + assessed = data["assessed_findings"] + assert len(assessed) == 1 + assert "DD-1" in assessed[0]["source_perspectives"] + assert "DD-2" in assessed[0]["source_perspectives"] + + +class TestSeverityIndependence: + """BC-6: Assessor can re-prioritize severity independently of reviewers.""" + + def test_severity_downgrade( + self, env_vars, artifact_file, state_dir, capsys, default_perspectives + ): + assessor = make_assessment_response( + assessed_findings=[{ + "id": "F-1", "severity": "SUGGESTION", + "original_severity": "CRITICAL", + "description": "Overstated issue", + "source_perspectives": ["DD-1"], + "assessment_rationale": "This is actually cosmetic", + }], + ) + + _run_with_assessor(artifact_file, state_dir, default_perspectives, assessor) + + assessment_files = list(state_dir.glob("assessment-round-*.json")) + assert len(assessment_files) >= 1 + data = json.loads(assessment_files[0].read_text()) + f = data["assessed_findings"][0] + assert f["severity"] == "SUGGESTION" + assert f["original_severity"] == "CRITICAL" + + +class TestDismissal: + """BC-7: Assessor dismisses findings with reason.""" + + def test_finding_dismissed( + self, env_vars, artifact_file, state_dir, capsys, default_perspectives + ): + assessor = make_assessment_response( + assessed_findings=[], + dismissed=[{ + "description": "Factually incorrect claim", + "source_perspectives": ["DD-3"], + "dismissal_reason": "The code actually handles this case correctly", + }], + ) + + _run_with_assessor( + artifact_file, state_dir, default_perspectives, assessor, + reviewer_findings=[{"severity": "CRITICAL", "description": "Factually incorrect claim"}], + ) + + assessment_files = list(state_dir.glob("assessment-round-*.json")) + assert len(assessment_files) >= 1 + data = json.loads(assessment_files[0].read_text()) + + assert len(data["assessed_findings"]) == 0 + assert len(data["dismissed"]) == 1 + d = data["dismissed"][0] + assert "DD-3" in d["source_perspectives"] + assert len(d["dismissal_reason"]) > 0 + + events = harvest_events(capsys) + assess_events = [e for e in events if e.get("event") == "assessment_complete"] + assert len(assess_events) == 1 + assert assess_events[0]["dismissed"] == 1 diff --git a/.claude/skills/convergence-review/tests/test_convergence.py b/.claude/skills/convergence-review/tests/test_convergence.py new file mode 100644 index 0000000..87a2e23 --- /dev/null +++ b/.claude/skills/convergence-review/tests/test_convergence.py @@ -0,0 +1,215 @@ +"""BC-1, BC-2, BC-3: Convergence loop outcomes (converged, stalled, early stall).""" + +from __future__ import annotations + +import json +from unittest.mock import patch, MagicMock + +import pytest + +from conftest import harvest_events +from helpers import ( + _make_chat_response, + make_assessment_response, + make_reviewer_response, + make_round_mock, + run_main, +) +from models import FixResult + +REVIEWER_MODEL = "openai/test-reviewer" +FIXER_MODEL = "openai/test-fixer" + + +def _make_argv(artifact_path, state_dir, max_rounds=10, force=False, stall_window=3): + argv = [ + "design", str(artifact_path), + "--reviewer-model", REVIEWER_MODEL, + "--fixer-model", FIXER_MODEL, + "--max-rounds", str(max_rounds), + "--state-dir", str(state_dir), + "--stall-window", str(stall_window), + ] + if force: + argv.append("--force") + return argv + + +class TestConvergence: + """BC-1: R1 finds 1 CRITICAL (fixed), R2 finds 0 → exit=0, converged event.""" + + def test_convergence_exit( + self, env_vars, artifact_file, state_dir, capsys, default_perspectives + ): + round_counter = {"n": 0} + + async def side_effect(*, model, messages, temperature=0.0, **kwargs): + if model == FIXER_MODEL: + if round_counter["n"] == 1: + resp = make_assessment_response( + assessed_findings=[{ + "id": "F-1", "severity": "CRITICAL", + "original_severity": "CRITICAL", + "description": "Missing invariant check", + "source_perspectives": ["DD-1"], + "assessment_rationale": "Valid finding", + }], + ) + else: + resp = make_assessment_response() + return _make_chat_response(json.dumps(resp)) + + if round_counter["n"] == 1: + resp = make_reviewer_response("DD-1", [{ + "severity": "CRITICAL", "description": "Missing invariant check", + }]) + else: + resp = make_reviewer_response("DD-1", []) + return _make_chat_response(json.dumps(resp)) + + load_call_count = {"n": 0} + + def mock_load(gate, path): + load_call_count["n"] += 1 + return "content v" + str(load_call_count["n"]) + + mock_client = MagicMock() + mock_client.chat.completions.create = side_effect + + def mock_apply_fixes(findings, artifact_path, sd, round_num, **kwargs): + return FixResult( + fixes_applied=[{"finding_id": "F-1", "severity": "CRITICAL", "action": "fixed"}], + ) + + import orchestrator as orch_mod + original_run_round = orch_mod.run_round + + async def tracking_run_round(client, args, round_num, *a, **kw): + round_counter["n"] = round_num + return await original_run_round(client, args, round_num, *a, **kw) + + with ( + patch("orchestrator.load_perspectives", return_value=default_perspectives), + patch("orchestrator.load_artifact_content", side_effect=mock_load), + patch("orchestrator.AsyncOpenAI", return_value=mock_client), + patch("orchestrator.apply_fixes", side_effect=mock_apply_fixes), + patch("orchestrator.run_round", side_effect=tracking_run_round), + ): + code = run_main(_make_argv(artifact_file, state_dir, max_rounds=5)) + + assert code == 0 + events = harvest_events(capsys) + event_names = [e["event"] for e in events if "event" in e] + assert "converged" in event_names + + # Summary file should contain "CONVERGED" + summary_files = list(state_dir.glob("summary.md")) + assert len(summary_files) == 1, f"Expected summary.md in {state_dir}" + assert "CONVERGED" in summary_files[0].read_text() + + +class TestStallMaxRounds: + """BC-2: CRITICAL persists every round, max-rounds reached → exit=1, stalled event.""" + + def test_stall_max_rounds( + self, env_vars, artifact_file, state_dir, capsys, default_perspectives + ): + reviewer_resp = { + p.id: make_reviewer_response(p.id, [{ + "severity": "CRITICAL", "description": "Persistent issue", + }]) for p in default_perspectives + } + assessor = make_assessment_response( + assessed_findings=[{ + "id": "F-1", "severity": "CRITICAL", + "original_severity": "CRITICAL", + "description": "Persistent issue", + "source_perspectives": ["DD-1"], + "assessment_rationale": "Real issue", + }], + ) + + mock_client = MagicMock() + mock_client.chat.completions.create = make_round_mock( + reviewer_resp, assessor, + reviewer_model=REVIEWER_MODEL, fixer_model=FIXER_MODEL, + ) + + def mock_apply_fixes(findings, artifact_path, sd, round_num, **kwargs): + return FixResult( + fixes_applied=[{"finding_id": "F-1", "severity": "CRITICAL", "action": "attempted"}], + ) + + with ( + patch("orchestrator.load_perspectives", return_value=default_perspectives), + patch("orchestrator.load_artifact_content", return_value="test content"), + patch("orchestrator.AsyncOpenAI", return_value=mock_client), + patch("orchestrator.apply_fixes", side_effect=mock_apply_fixes), + ): + code = run_main(_make_argv(artifact_file, state_dir, max_rounds=2, force=True)) + + assert code == 1 + events = harvest_events(capsys) + event_names = [e["event"] for e in events if "event" in e] + assert "stalled" in event_names + + stalled = [e for e in events if e.get("event") == "stalled"] + assert stalled[0]["remaining_critical"] > 0 + + summary_files = list(state_dir.glob("summary.md")) + assert len(summary_files) == 1, f"Expected summary.md in {state_dir}" + assert "STALLED" in summary_files[0].read_text() + + +class TestEarlyStall: + """BC-3: Finding counts don't decrease over stall window → early exit.""" + + def test_stall_early_no_improvement( + self, env_vars, artifact_file, state_dir, capsys, default_perspectives + ): + reviewer_resp = { + p.id: make_reviewer_response(p.id, [{ + "severity": "IMPORTANT", "description": "Persists", + }]) for p in default_perspectives + } + assessor = make_assessment_response( + assessed_findings=[{ + "id": "F-1", "severity": "IMPORTANT", + "original_severity": "IMPORTANT", + "description": "Persists", + "source_perspectives": ["DD-1"], + "assessment_rationale": "Valid", + }], + ) + + mock_client = MagicMock() + mock_client.chat.completions.create = make_round_mock( + reviewer_resp, assessor, + reviewer_model=REVIEWER_MODEL, fixer_model=FIXER_MODEL, + ) + + def mock_apply_fixes(findings, artifact_path, sd, round_num, **kwargs): + return FixResult( + fixes_applied=[], + fixes_skipped=[{"finding_id": "F-1", "severity": "IMPORTANT", "reason": "cannot fix"}], + ) + + with ( + patch("orchestrator.load_perspectives", return_value=default_perspectives), + patch("orchestrator.load_artifact_content", return_value="test content"), + patch("orchestrator.AsyncOpenAI", return_value=mock_client), + patch("orchestrator.apply_fixes", side_effect=mock_apply_fixes), + ): + code = run_main(_make_argv( + artifact_file, state_dir, max_rounds=10, stall_window=3, + )) + + assert code == 1 + events = harvest_events(capsys) + event_names = [e["event"] for e in events if "event" in e] + assert "early_stall" in event_names + + # No round 4 events + round_starts = [e for e in events if e.get("event") == "round_start"] + max_round = max(e["round"] for e in round_starts) + assert max_round <= 3 diff --git a/.claude/skills/convergence-review/tests/test_fix_phase.py b/.claude/skills/convergence-review/tests/test_fix_phase.py new file mode 100644 index 0000000..c75cba2 --- /dev/null +++ b/.claude/skills/convergence-review/tests/test_fix_phase.py @@ -0,0 +1,133 @@ +"""BC-11, BC-15: Conditional fix and prompt delivery.""" + +from __future__ import annotations + +import json +from unittest.mock import patch, MagicMock + +import pytest + +from conftest import harvest_events +from helpers import ( + make_assessment_response, + make_reviewer_response, + make_round_mock, + run_main, +) +from models import FixResult + +REVIEWER_MODEL = "openai/test-reviewer" +FIXER_MODEL = "openai/test-fixer" + + +class TestFixSkippedWhenClean: + """BC-11: 0 CRITICAL + 0 IMPORTANT → no fix_start, round converges.""" + + def test_fix_skipped_when_clean( + self, env_vars, artifact_file, state_dir, capsys, default_perspectives + ): + reviewer_resp = { + p.id: make_reviewer_response(p.id, [{ + "severity": "SUGGESTION", "description": "Minor style issue", + }]) for p in default_perspectives + } + assessor = make_assessment_response( + assessed_findings=[ + { + "id": "F-1", "severity": "SUGGESTION", + "original_severity": "SUGGESTION", + "description": "Style issue 1", + "source_perspectives": ["DD-1"], + "assessment_rationale": "Minor", + }, + { + "id": "F-2", "severity": "SUGGESTION", + "original_severity": "SUGGESTION", + "description": "Style issue 2", + "source_perspectives": ["DD-2"], + "assessment_rationale": "Minor", + }, + ], + ) + + mock_client = MagicMock() + mock_client.chat.completions.create = make_round_mock( + reviewer_resp, assessor, + reviewer_model=REVIEWER_MODEL, fixer_model=FIXER_MODEL, + ) + + argv = [ + "design", str(artifact_file), + "--reviewer-model", REVIEWER_MODEL, + "--fixer-model", FIXER_MODEL, + "--max-rounds", "1", + "--state-dir", str(state_dir), + ] + + with ( + patch("orchestrator.load_perspectives", return_value=default_perspectives), + patch("orchestrator.load_artifact_content", return_value="test content"), + patch("orchestrator.AsyncOpenAI", return_value=mock_client), + ): + code = run_main(argv) + + assert code == 0 + events = harvest_events(capsys) + event_names = [e["event"] for e in events if "event" in e] + + assert "fix_start" not in event_names + + round_completes = [e for e in events if e.get("event") == "round_complete"] + assert len(round_completes) == 1 + assert round_completes[0]["status"] == "converged" + + +class TestFixPromptViaStdin: + """BC-15: apply_fixes passes prompt via stdin (not CLI arg).""" + + def test_fix_uses_stdin(self, work_dir): + from models import AssessedFinding + + findings = [ + AssessedFinding( + id="F-1", severity="CRITICAL", original_severity="CRITICAL", + description="A" * 5000, + source_perspectives=["DD-1"], + assessment_rationale="Must fix", + ), + ] + + sd = work_dir / "state" + sd.mkdir() + + captured_stdin = {} + + captured_communicate = {} + + def mock_popen(cmd, **kwargs): + captured_stdin["cmd"] = cmd + captured_stdin["stdin_pipe"] = kwargs.get("stdin") + proc = MagicMock() + + def mock_communicate(input=None, timeout=None): + captured_communicate["input"] = input + return ('{"result": "done"}', "") + + proc.communicate = mock_communicate + proc.returncode = 0 + proc.poll.return_value = 0 + return proc + + import subprocess + with ( + patch("fixer.shutil.which", return_value="/usr/bin/claude"), + patch("fixer.subprocess.Popen", side_effect=mock_popen), + ): + from fixer import apply_fixes + apply_fixes(findings, "artifact.md", sd, 1, fixer_model=FIXER_MODEL) + + assert "-" in captured_stdin["cmd"] + assert captured_stdin["stdin_pipe"] == subprocess.PIPE + # Verify prompt content was actually passed via stdin + assert captured_communicate.get("input") is not None + assert len(captured_communicate["input"]) > 0 diff --git a/.claude/skills/convergence-review/tests/test_parse_fallback.py b/.claude/skills/convergence-review/tests/test_parse_fallback.py new file mode 100644 index 0000000..ca968b8 --- /dev/null +++ b/.claude/skills/convergence-review/tests/test_parse_fallback.py @@ -0,0 +1,137 @@ +"""Tests for reviewer parse fallback extraction.""" + +from __future__ import annotations + +import asyncio +import json +from unittest.mock import MagicMock + +import pytest + +from reviewer import ( + _extract_findings_fallback, + _fix_json_escapes, + _parse_reviewer_response, + run_reviewer, +) +from helpers import FakePerspective + + +def _make_async_chat_response(content: str) -> MagicMock: + """Build a mock that looks like an AsyncOpenAI chat completion response.""" + msg = MagicMock() + msg.content = content + choice = MagicMock() + choice.message = msg + resp = MagicMock() + resp.choices = [choice] + + async def _create(**kwargs): + return resp + + client = MagicMock() + client.chat.completions.create = _create + return client + + +class TestFixJsonEscapes: + """Test the sentinel-based JSON escape fixer.""" + + def test_valid_json_unchanged(self): + raw = '{"findings": [{"severity": "CRITICAL", "description": "test"}]}' + assert json.loads(_fix_json_escapes(raw)) == json.loads(raw) + + def test_fixes_bare_backslash_w(self): + # \w is invalid JSON escape + raw = r'{"evidence": "matches \w+ pattern"}' + fixed = _fix_json_escapes(raw) + data = json.loads(fixed) + assert "\\w+" in data["evidence"] + + def test_preserves_already_escaped(self): + # \\w should stay as \\w (literal backslash + w) + raw = '{"evidence": "matches \\\\w+ pattern"}' + fixed = _fix_json_escapes(raw) + data = json.loads(fixed) + assert "\\w+" in data["evidence"] + + def test_mixed_escapes(self): + # Mix of valid \\w and invalid \d in the same string + raw = r'{"evidence": "\\w and \d patterns"}' + fixed = _fix_json_escapes(raw) + data = json.loads(fixed) + assert "\\w" in data["evidence"] + assert "\\d" in data["evidence"] + + +class TestFallbackExtraction: + """Test the LLM fallback extraction for unparseable responses.""" + + def test_fallback_recovers_findings(self): + unparseable = "Here are my findings:\n1. [CRITICAL] Something is wrong\nBut this text has weird formatting..." + extraction_json = json.dumps({ + "findings": [ + {"severity": "CRITICAL", "description": "Something is wrong"} + ] + }) + client = _make_async_chat_response(extraction_json) + + findings = asyncio.run( + _extract_findings_fallback(client, "test-model", "XPP-1", unparseable, 60.0) + ) + assert findings is not None + assert len(findings) == 1 + assert findings[0].severity == "CRITICAL" + + def test_fallback_returns_none_on_failure(self): + """Fallback returns None if extraction also fails to parse.""" + client = _make_async_chat_response("I couldn't parse that either") + + findings = asyncio.run( + _extract_findings_fallback(client, "test-model", "XPP-1", "junk", 60.0) + ) + assert findings is None + + +class TestRunReviewerFallback: + """Integration: run_reviewer invokes fallback when primary parsing fails.""" + + def test_fallback_invoked_on_parse_failure(self): + """Non-empty unparseable response triggers fallback extraction.""" + # First call: reviewer returns unparseable text + # Second call: fallback extraction returns valid JSON + unparseable_response = "A" * 100 # 100 chars, no findings pattern + extraction_json = json.dumps({ + "findings": [ + {"severity": "IMPORTANT", "description": "Recovered finding"} + ] + }) + + call_count = {"n": 0} + + async def mock_create(*, model, messages, temperature=0.0, **kwargs): + call_count["n"] += 1 + if call_count["n"] == 1: + content = unparseable_response + else: + content = extraction_json + msg = MagicMock() + msg.content = content + choice = MagicMock() + choice.message = msg + resp = MagicMock() + resp.choices = [choice] + return resp + + client = MagicMock() + client.chat.completions.create = mock_create + + perspective = FakePerspective("XPP-1", "Test") + result = asyncio.run( + run_reviewer(client, "test-model", perspective, "artifact text", 60.0, 1) + ) + + assert call_count["n"] == 2 # reviewer + fallback + assert len(result.findings) == 1 + assert result.findings[0].severity == "IMPORTANT" + assert result.parse_warning is None # fallback succeeded, no warning diff --git a/.claude/skills/convergence-review/tests/test_protected_paths.py b/.claude/skills/convergence-review/tests/test_protected_paths.py new file mode 100644 index 0000000..4b4d69e --- /dev/null +++ b/.claude/skills/convergence-review/tests/test_protected_paths.py @@ -0,0 +1,309 @@ +"""Tests for protected paths feature across all phases.""" + +from __future__ import annotations + +import subprocess +from unittest.mock import MagicMock, patch + +import pytest + +from helpers import FakePerspective + + +# --------------------------------------------------------------------------- +# orchestrator: _check_protected_violations +# --------------------------------------------------------------------------- + +class TestCheckProtectedViolations: + """Unit tests for _check_protected_violations glob matching.""" + + def test_no_matches_returns_empty(self): + from orchestrator import _check_protected_violations + + fake_result = MagicMock(returncode=0, stdout="src/main.py\nREADME.md\n") + with patch("orchestrator.subprocess.run", return_value=fake_result): + violations = _check_protected_violations(["docs/contributing/**"]) + assert violations == [] + + def test_exact_glob_match(self): + from orchestrator import _check_protected_violations + + fake_result = MagicMock( + returncode=0, + stdout="src/main.py\ndocs/contributing/rules.md\nREADME.md\n", + ) + with patch("orchestrator.subprocess.run", return_value=fake_result): + violations = _check_protected_violations(["docs/contributing/**"]) + assert violations == ["docs/contributing/rules.md"] + + def test_multiple_patterns(self): + from orchestrator import _check_protected_violations + + fake_result = MagicMock( + returncode=0, + stdout="src/main.py\ndocs/contributing/rules.md\nCONTRIBUTING.md\n", + ) + with patch("orchestrator.subprocess.run", return_value=fake_result): + violations = _check_protected_violations( + ["docs/contributing/**", "CONTRIBUTING.md"] + ) + assert set(violations) == {"docs/contributing/rules.md", "CONTRIBUTING.md"} + + def test_nested_path_match(self): + from orchestrator import _check_protected_violations + + fake_result = MagicMock( + returncode=0, + stdout="docs/contributing/standards/rules.md\n", + ) + with patch("orchestrator.subprocess.run", return_value=fake_result): + violations = _check_protected_violations(["docs/contributing/**"]) + assert violations == ["docs/contributing/standards/rules.md"] + + def test_git_failure_returns_empty(self): + from orchestrator import _check_protected_violations + + fake_result = MagicMock(returncode=1, stdout="") + with patch("orchestrator.subprocess.run", return_value=fake_result): + violations = _check_protected_violations(["docs/contributing/**"]) + assert violations == [] + + def test_git_timeout_returns_empty(self): + from orchestrator import _check_protected_violations + + with patch( + "orchestrator.subprocess.run", + side_effect=subprocess.TimeoutExpired("git", 30), + ): + violations = _check_protected_violations(["docs/contributing/**"]) + assert violations == [] + + def test_os_error_returns_empty(self): + from orchestrator import _check_protected_violations + + with patch( + "orchestrator.subprocess.run", + side_effect=OSError("git not found"), + ): + violations = _check_protected_violations(["docs/contributing/**"]) + assert violations == [] + + def test_empty_diff_returns_empty(self): + from orchestrator import _check_protected_violations + + fake_result = MagicMock(returncode=0, stdout="") + with patch("orchestrator.subprocess.run", return_value=fake_result): + violations = _check_protected_violations(["docs/contributing/**"]) + assert violations == [] + + +# --------------------------------------------------------------------------- +# reviewer: protected-paths preamble in prompts +# --------------------------------------------------------------------------- + +class TestReviewerProtectedPreamble: + """Reviewer prompts include protected-file preamble when configured.""" + + def test_preamble_with_paths(self): + from reviewer import _build_protected_paths_preamble + + result = _build_protected_paths_preamble(["docs/contributing/**"]) + assert "Protected Reference Files" in result + assert "`docs/contributing/**`" in result + assert "ground truth" in result + + def test_preamble_empty_when_none(self): + from reviewer import _build_protected_paths_preamble + + assert _build_protected_paths_preamble(None) == "" + assert _build_protected_paths_preamble([]) == "" + + def test_preamble_multiple_patterns(self): + from reviewer import _build_protected_paths_preamble + + result = _build_protected_paths_preamble( + ["docs/contributing/**", "GOVERNANCE.md"] + ) + assert "`docs/contributing/**`" in result + assert "`GOVERNANCE.md`" in result + + def test_inject_artifact_includes_preamble(self): + from reviewer import _inject_artifact + + prompt = "Review for issues." + result = _inject_artifact( + prompt, + "artifact content", + protected_paths=["docs/contributing/**"], + ) + assert "Protected Reference Files" in result + assert "`docs/contributing/**`" in result + + def test_inject_artifact_no_preamble_when_none(self): + from reviewer import _inject_artifact + + prompt = "Review for issues." + result = _inject_artifact(prompt, "artifact content", protected_paths=None) + assert "Protected Reference Files" not in result + + +# --------------------------------------------------------------------------- +# assessor: protected paths in assessment input +# --------------------------------------------------------------------------- + +class TestAssessorProtectedPaths: + """Assessor dismisses findings targeting protected files.""" + + def test_format_includes_protected_section(self): + from assessor import _format_assessment_input + from models import ReviewerOutput + + outputs = [ + ReviewerOutput( + perspective="DD-1", + findings=[], + ) + ] + result = _format_assessment_input( + outputs, + "artifact content", + gate="design", + protected_paths=["docs/contributing/**"], + ) + assert "Protected Paths" in result + assert "`docs/contributing/**`" in result + assert "DISMISS" in result + + def test_format_no_protected_section_when_none(self): + from assessor import _format_assessment_input + from models import ReviewerOutput + + outputs = [ + ReviewerOutput( + perspective="DD-1", + findings=[], + ) + ] + result = _format_assessment_input( + outputs, "artifact content", gate="design", protected_paths=None, + ) + assert "Protected Paths" not in result + + def test_system_prompt_has_protected_files_rule(self): + from assessor import ASSESSMENT_SYSTEM_PROMPT + + assert "PROTECTED FILES" in ASSESSMENT_SYSTEM_PROMPT + assert "DISMISS" in ASSESSMENT_SYSTEM_PROMPT + + +# --------------------------------------------------------------------------- +# fixer: protected paths in fix prompt +# --------------------------------------------------------------------------- + +class TestFixerProtectedPaths: + """Fixer prompt includes protected paths section when configured.""" + + def test_build_protected_section_with_paths(self): + from fixer import _build_protected_section + + result = _build_protected_section(["docs/contributing/**"]) + assert "Protected Files (DO NOT MODIFY)" in result + assert "`docs/contributing/**`" in result + assert "read-only governance files" in result + + def test_build_protected_section_empty_when_none(self): + from fixer import _build_protected_section + + assert _build_protected_section(None) == "" + assert _build_protected_section([]) == "" + + def test_apply_fixes_includes_protected_section(self, tmp_path): + from models import AssessedFinding + + findings = [ + AssessedFinding( + id="F-1", + severity="CRITICAL", + original_severity="CRITICAL", + description="Test finding", + source_perspectives=["DD-1"], + assessment_rationale="Must fix", + ), + ] + + sd = tmp_path / "state" + sd.mkdir() + + captured_stdin = {} + + def mock_popen(cmd, **kwargs): + proc = MagicMock() + + def mock_communicate(input=None, timeout=None): + captured_stdin["input"] = input + return ('{"result": "done"}', "") + + proc.communicate = mock_communicate + proc.returncode = 0 + proc.poll.return_value = 0 + return proc + + with ( + patch("fixer.shutil.which", return_value="/usr/bin/claude"), + patch("fixer.subprocess.Popen", side_effect=mock_popen), + ): + from fixer import apply_fixes + + apply_fixes( + findings, + "artifact.md", + sd, + 1, + protected_paths=["docs/contributing/**"], + ) + + prompt = captured_stdin["input"] + assert "Protected Files (DO NOT MODIFY)" in prompt + assert "`docs/contributing/**`" in prompt + + def test_apply_fixes_no_protected_section_without_paths(self, tmp_path): + from models import AssessedFinding + + findings = [ + AssessedFinding( + id="F-1", + severity="CRITICAL", + original_severity="CRITICAL", + description="Test finding", + source_perspectives=["DD-1"], + assessment_rationale="Must fix", + ), + ] + + sd = tmp_path / "state" + sd.mkdir() + + captured_stdin = {} + + def mock_popen(cmd, **kwargs): + proc = MagicMock() + + def mock_communicate(input=None, timeout=None): + captured_stdin["input"] = input + return ('{"result": "done"}', "") + + proc.communicate = mock_communicate + proc.returncode = 0 + proc.poll.return_value = 0 + return proc + + with ( + patch("fixer.shutil.which", return_value="/usr/bin/claude"), + patch("fixer.subprocess.Popen", side_effect=mock_popen), + ): + from fixer import apply_fixes + + apply_fixes(findings, "artifact.md", sd, 1) + + prompt = captured_stdin["input"] + assert "Protected Files (DO NOT MODIFY)" not in prompt diff --git a/.claude/skills/convergence-review/tests/test_round_lifecycle.py b/.claude/skills/convergence-review/tests/test_round_lifecycle.py new file mode 100644 index 0000000..6456b4b --- /dev/null +++ b/.claude/skills/convergence-review/tests/test_round_lifecycle.py @@ -0,0 +1,208 @@ +"""BC-12, BC-13, BC-14: Artifact reload, summary incremental write, state isolation.""" + +from __future__ import annotations + +import json +from pathlib import Path +from unittest.mock import patch, MagicMock + +import pytest + +from conftest import harvest_events +from helpers import ( + _make_chat_response, + make_assessment_response, + make_reviewer_response, + make_round_mock, + run_main, +) +from models import FixResult + +REVIEWER_MODEL = "openai/test-reviewer" +FIXER_MODEL = "openai/test-fixer" + + +class TestArtifactReloadAfterFix: + """BC-12: Artifact is reloaded after a fix round (not cached).""" + + def test_artifact_reload_after_fix( + self, env_vars, artifact_file, state_dir, capsys, default_perspectives + ): + load_calls = [] + + def mock_load(gate, path): + load_calls.append(len(load_calls) + 1) + return f"content v{len(load_calls)}" + + round_counter = {"n": 0} + + async def side_effect(*, model, messages, temperature=0.0, **kwargs): + if model == FIXER_MODEL: + if round_counter["n"] == 1: + resp = make_assessment_response( + assessed_findings=[{ + "id": "F-1", "severity": "CRITICAL", + "original_severity": "CRITICAL", + "description": "Bug", "source_perspectives": ["DD-1"], + "assessment_rationale": "Real", + }], + ) + else: + resp = make_assessment_response() + return _make_chat_response(json.dumps(resp)) + + if round_counter["n"] == 1: + resp = make_reviewer_response("DD-1", [{ + "severity": "CRITICAL", "description": "Bug", + }]) + else: + resp = make_reviewer_response("DD-1", []) + return _make_chat_response(json.dumps(resp)) + + import orchestrator as orch_mod + original_run_round = orch_mod.run_round + + async def tracking_run_round(client, args, round_num, *a, **kw): + round_counter["n"] = round_num + return await original_run_round(client, args, round_num, *a, **kw) + + mock_client = MagicMock() + mock_client.chat.completions.create = side_effect + + def mock_apply_fixes(findings, artifact_path, sd, round_num, **kwargs): + return FixResult( + fixes_applied=[{"finding_id": "F-1", "severity": "CRITICAL", "action": "fixed"}], + ) + + with ( + patch("orchestrator.load_perspectives", return_value=default_perspectives), + patch("orchestrator.load_artifact_content", side_effect=mock_load), + patch("orchestrator.AsyncOpenAI", return_value=mock_client), + patch("orchestrator.apply_fixes", side_effect=mock_apply_fixes), + patch("orchestrator.run_round", side_effect=tracking_run_round), + ): + code = run_main([ + "design", str(artifact_file), + "--reviewer-model", REVIEWER_MODEL, + "--fixer-model", FIXER_MODEL, + "--max-rounds", "5", + "--state-dir", str(state_dir), + ]) + + assert code == 0 + assert len(load_calls) >= 2 + + +class TestSummaryIncremental: + """BC-13: Summary file updated incrementally (review+assessment before fix).""" + + def test_summary_written_before_fix( + self, env_vars, artifact_file, state_dir, capsys, default_perspectives + ): + summary_content_during_fix = {} + + reviewer_resp = { + p.id: make_reviewer_response(p.id, [{ + "severity": "CRITICAL", "description": "Must fix issue", + }]) for p in default_perspectives + } + assessor = make_assessment_response( + assessed_findings=[{ + "id": "F-1", "severity": "CRITICAL", + "original_severity": "CRITICAL", + "description": "Must fix issue", + "source_perspectives": ["DD-1"], + "assessment_rationale": "Valid", + }], + ) + + mock_client = MagicMock() + mock_client.chat.completions.create = make_round_mock( + reviewer_resp, assessor, + reviewer_model=REVIEWER_MODEL, fixer_model=FIXER_MODEL, + ) + + def mock_apply_fixes(findings, artifact_path, sd, round_num, **kwargs): + # Read summary file during fix phase + # Check both state dir copy and artifact-adjacent + for candidate in [ + sd / "summary.md", + artifact_file.parent / f"{artifact_file.stem}-review.md", + ]: + if candidate.exists(): + summary_content_during_fix["text"] = candidate.read_text() + break + return FixResult( + fixes_applied=[{"finding_id": "F-1", "severity": "CRITICAL", "action": "fixed"}], + ) + + with ( + patch("orchestrator.load_perspectives", return_value=default_perspectives), + patch("orchestrator.load_artifact_content", return_value="test content"), + patch("orchestrator.AsyncOpenAI", return_value=mock_client), + patch("orchestrator.apply_fixes", side_effect=mock_apply_fixes), + ): + run_main([ + "design", str(artifact_file), + "--reviewer-model", REVIEWER_MODEL, + "--fixer-model", FIXER_MODEL, + "--max-rounds", "1", + "--state-dir", str(state_dir), + "--force", + ]) + + assert "text" in summary_content_during_fix + text = summary_content_during_fix["text"] + assert "Round" in text + assert "Assessment" in text or "CRITICAL" in text + + +class TestStateIsolation: + """BC-14: Old state dir is cleaned before a new run.""" + + def test_state_isolation( + self, env_vars, artifact_file, state_dir, capsys, default_perspectives + ): + state_dir.mkdir(parents=True) + + leftover1 = state_dir / "assessment-round-1.json" + leftover1.write_text('{"old": true}') + leftover2 = state_dir / "old-progress.json" + leftover2.write_text('{"stale": true}') + + reviewer_resp = { + p.id: make_reviewer_response(p.id, []) for p in default_perspectives + } + assessor = make_assessment_response() + + mock_client = MagicMock() + mock_client.chat.completions.create = make_round_mock( + reviewer_resp, assessor, + reviewer_model=REVIEWER_MODEL, fixer_model=FIXER_MODEL, + ) + + with ( + patch("orchestrator.load_perspectives", return_value=default_perspectives), + patch("orchestrator.load_artifact_content", return_value="test content"), + patch("orchestrator.AsyncOpenAI", return_value=mock_client), + ): + code = run_main([ + "design", str(artifact_file), + "--reviewer-model", REVIEWER_MODEL, + "--fixer-model", FIXER_MODEL, + "--max-rounds", "1", + "--state-dir", str(state_dir), + ]) + + assert code == 0 + assert not leftover2.exists(), "Leftover files should be cleaned by rmtree" + + new_files = list(state_dir.iterdir()) + assert len(new_files) > 0 + + # assessment-round-1.json may be recreated by the new run, but must not have old content + if leftover1.exists(): + data = json.loads(leftover1.read_text()) + assert "old" not in data, "State dir should be fully cleaned before new run" + # The key invariant: old-progress.json (not recreated by the new run) must be gone + assert not (state_dir / "old-progress.json").exists() diff --git a/.claude/skills/convergence-review/tests/test_round_protocol.py b/.claude/skills/convergence-review/tests/test_round_protocol.py new file mode 100644 index 0000000..bbfc7fe --- /dev/null +++ b/.claude/skills/convergence-review/tests/test_round_protocol.py @@ -0,0 +1,142 @@ +"""BC-8, BC-9, BC-10: Round event ordering and failure tolerance.""" + +from __future__ import annotations + +from unittest.mock import patch, MagicMock + +import pytest + +from conftest import harvest_events +from helpers import ( + make_assessment_response, + make_error_side_effect, + make_reviewer_response, + make_round_mock, + run_main, +) + +REVIEWER_MODEL = "openai/test-reviewer" +FIXER_MODEL = "openai/test-fixer" + + +def _base_argv(artifact_path, state_dir): + return [ + "design", str(artifact_path), + "--reviewer-model", REVIEWER_MODEL, + "--fixer-model", FIXER_MODEL, + "--max-rounds", "1", + "--state-dir", str(state_dir), + ] + + +class TestEventOrdering: + """BC-8: Events within a round occur in deterministic order.""" + + def test_single_round_event_order( + self, env_vars, artifact_file, state_dir, capsys, default_perspectives + ): + reviewer_responses = { + p.id: make_reviewer_response(p.id, []) for p in default_perspectives + } + assessor = make_assessment_response() + + mock_client = MagicMock() + mock_client.chat.completions.create = make_round_mock( + reviewer_responses, assessor, + reviewer_model=REVIEWER_MODEL, fixer_model=FIXER_MODEL, + ) + + with ( + patch("orchestrator.load_perspectives", return_value=default_perspectives), + patch("orchestrator.load_artifact_content", return_value="test content"), + patch("orchestrator.AsyncOpenAI", return_value=mock_client), + ): + code = run_main(_base_argv(artifact_file, state_dir)) + + assert code == 0 + events = harvest_events(capsys) + event_names = [e["event"] for e in events if "event" in e] + + for required in ["round_start", "reviews_complete", "assessment_complete", "round_complete"]: + assert event_names.count(required) == 1, f"Expected 1 '{required}', got {event_names.count(required)}" + + ordered = ["round_start", "reviews_complete", "assessment_complete", "round_complete"] + indices = [event_names.index(e) for e in ordered] + assert indices == sorted(indices), f"Events out of order: {list(zip(ordered, indices))}" + + assert "converged" in event_names + + +class TestPartialReviewerFailure: + """BC-9: Partial reviewer failure doesn't kill the round.""" + + def test_one_of_three_fails( + self, env_vars, artifact_file, state_dir, capsys, default_perspectives + ): + good_responses = { + "DD-2": make_reviewer_response("DD-2", []), + "DD-3": make_reviewer_response("DD-3", []), + } + assessor = make_assessment_response() + + mock_client = MagicMock() + mock_client.chat.completions.create = make_error_side_effect( + error_perspectives={"DD-1"}, + reviewer_responses=good_responses, + assessor_response=assessor, + reviewer_model=REVIEWER_MODEL, + fixer_model=FIXER_MODEL, + ) + + with ( + patch("orchestrator.load_perspectives", return_value=default_perspectives), + patch("orchestrator.load_artifact_content", return_value="test content"), + patch("orchestrator.AsyncOpenAI", return_value=mock_client), + ): + code = run_main(_base_argv(artifact_file, state_dir)) + + assert code == 0 + events = harvest_events(capsys) + + reviews_complete = [e for e in events if e.get("event") == "reviews_complete"] + assert len(reviews_complete) == 1 + rc = reviews_complete[0] + assert rc["perspectives_failed"] >= 1 + assert rc["perspectives_ok"] >= 1 + + assert any(e.get("event") == "assessment_complete" for e in events) + + +class TestTotalReviewerFailure: + """BC-10: All reviewers fail → round status=error, no fix_start.""" + + def test_all_fail_gives_error( + self, env_vars, artifact_file, state_dir, capsys, default_perspectives + ): + all_ids = {p.id for p in default_perspectives} + assessor = make_assessment_response() + + mock_client = MagicMock() + mock_client.chat.completions.create = make_error_side_effect( + error_perspectives=all_ids, + reviewer_responses={}, + assessor_response=assessor, + reviewer_model=REVIEWER_MODEL, + fixer_model=FIXER_MODEL, + ) + + with ( + patch("orchestrator.load_perspectives", return_value=default_perspectives), + patch("orchestrator.load_artifact_content", return_value="test content"), + patch("orchestrator.AsyncOpenAI", return_value=mock_client), + ): + code = run_main(_base_argv(artifact_file, state_dir)) + + assert code == 2 + events = harvest_events(capsys) + + round_completes = [e for e in events if e.get("event") == "round_complete"] + assert len(round_completes) == 1 + assert round_completes[0]["status"] == "error" + + assert not any(e.get("event") == "fix_start" for e in events) diff --git a/.claude/skills/convergence-review/tests/test_setup_validation.py b/.claude/skills/convergence-review/tests/test_setup_validation.py new file mode 100644 index 0000000..734b6e0 --- /dev/null +++ b/.claude/skills/convergence-review/tests/test_setup_validation.py @@ -0,0 +1,65 @@ +"""BC-4: Setup validation errors exit with code 2, no round_start event.""" + +from __future__ import annotations + +from unittest.mock import patch + +import pytest + +from conftest import harvest_events +from helpers import FakePerspective, run_main + + +class TestSetupValidation: + """BC-4: All setup/arg errors → exit 2, no round_start event emitted.""" + + def test_invalid_gate(self, capsys): + """Unrecognized gate name → argparse error → SystemExit(2).""" + with pytest.raises(SystemExit) as exc_info: + run_main(["bogus", "file.md"]) + assert exc_info.value.code == 2 + + def test_missing_artifact_for_file_gate(self, env_vars, capsys): + """File gate without artifact path → argparse error → SystemExit(2).""" + with pytest.raises(SystemExit) as exc_info: + run_main(["design"]) + assert exc_info.value.code == 2 + + def test_missing_api_key(self, monkeypatch, work_dir, capsys): + """No API key env vars → exit 2, error event mentions key.""" + artifact = work_dir / "artifact.md" + artifact.write_text("# Test") + + monkeypatch.setenv("LITELLM_BASE_URL", "http://test.local/v1") + monkeypatch.delenv("LITELLM_API_KEY", raising=False) + monkeypatch.delenv("ANTHROPIC_AUTH_TOKEN", raising=False) + monkeypatch.delenv("ANTHROPIC_API_KEY", raising=False) + + with patch("orchestrator.load_perspectives", return_value=[FakePerspective("DD-1", "Arch")]): + code = run_main(["design", str(artifact)]) + + assert code == 2 + events = harvest_events(capsys) + assert not any(e.get("event") == "round_start" for e in events) + error_events = [e for e in events if e.get("event") == "error"] + assert len(error_events) >= 1 + assert "key" in error_events[0]["message"].lower() + + def test_missing_base_url(self, monkeypatch, work_dir, capsys): + """No base URL env vars → exit 2, error event mentions URL.""" + artifact = work_dir / "artifact.md" + artifact.write_text("# Test") + + monkeypatch.delenv("LITELLM_BASE_URL", raising=False) + monkeypatch.delenv("ANTHROPIC_BASE_URL", raising=False) + monkeypatch.setenv("LITELLM_API_KEY", "test-key") + + with patch("orchestrator.load_perspectives", return_value=[FakePerspective("DD-1", "Arch")]): + code = run_main(["design", str(artifact)]) + + assert code == 2 + events = harvest_events(capsys) + assert not any(e.get("event") == "round_start" for e in events) + error_events = [e for e in events if e.get("event") == "error"] + assert len(error_events) >= 1 + assert "url" in error_events[0]["message"].lower() diff --git a/.claude/skills/hypothesis-experiment/SKILL.md b/.claude/skills/hypothesis-experiment/SKILL.md new file mode 100644 index 0000000..82dd4d8 --- /dev/null +++ b/.claude/skills/hypothesis-experiment/SKILL.md @@ -0,0 +1,275 @@ +--- +name: hypothesis-experiment +description: Run the full hypothesis experiment workflow (Steps 0-10) — worktree, classify, design review (5 perspectives), human gate, implement, code review (5 perspectives), run, analyze, FINDINGS review (10 perspectives), self-audit, commit + PR. Enforces convergence protocol and hard gates. +argument-hint: +disable-model-invocation: true +--- + +# Hypothesis Experiment Workflow + +You are running the hypothesis experiment workflow for hypothesis **$ARGUMENTS**. + +**Canonical source:** `docs/contributing/hypothesis.md` (v2.0). Read it NOW before proceeding. +**Standards:** `docs/contributing/standards/experiments.md` (ED-1–ED-6, RCV-1–RCV-6). +**Template:** `docs/contributing/templates/hypothesis.md` (FINDINGS.md structure). +**Review prompts:** See [review-prompts.md](review-prompts.md) for exact agent dispatch prompts. + +--- + +## Hard Rules (non-negotiable) + +1. **Follow the steps in order.** Do not skip steps. Do not reorder. +2. **Human gate at Step 3 is a STOP.** Present the design and WAIT. Do not say "I'll proceed unless you stop me." +3. **Convergence = zero CRITICAL + zero IMPORTANT from ALL reviewers in a round.** Re-run the ENTIRE round after fixes. No exceptions. No shortcuts. +4. **NEVER trust agent self-reported convergence.** Independently verify every finding count. Agents have fabricated "0 CRITICAL, 0 IMPORTANT" when actual review found 3 CRITICAL + 18 IMPORTANT (#390). +5. **Code review BEFORE running experiments** (Step 5). Three of four major bugs in PR #310 would have been caught. +6. **Max 10 rounds per gate.** If not converged by round 10, suspend the experiment. +7. **Cross-gate regression:** If Code or FINDINGS Review finds a design flaw, loop back to Step 2. Max 2 regressions total. + +## Lessons Learned (encoded from MEMORY.md) + +- **`--stderr` must come BEFORE other flags** in `blis_run` calls — harness checks position 3 only +- **CLI defaults vs workload-spec YAML**: `--rate` mode uses CLI defaults (512/512 tokens). Workload YAMLs define own distributions. Capacity estimates MUST match the actual workload. +- **`--total-kv-blocks` default is context-dependent**: CLI default 1000000 but `defaults.yaml` overrides to 132139 for llama/H100/TP=2. Check defaults.yaml. +- **Conservation formula**: Always 4-term INV-1: `injected == completed + queued + running + dropped_unservable`. `parse_blis_output` does NOT extract `dropped_unservable` — parse it separately. +- **Analyzer verdict must match FINDINGS status**: If `analyze.py` produces a different verdict than FINDINGS.md, acknowledge the discrepancy explicitly. +- **Think before coding calibration**: Compute parameters analytically from alpha/beta coefficients FIRST, then validate with a tiny run. +- **Beta coefficients** (llama-3.1-8b, H100, TP=2): `[6910.42, 17.67, 2.84]` -> stepTime = beta0 + beta1*cacheMissTokens + beta2*decodeTokens +- **Alpha coefficients**: `[1601.35, 3.51, 1805.54]` -> queueDelay = alpha0 + alpha1*inputLen; outputProcessing = alpha2 +- **WorkloadSpec YAML format**: `id:` not `client_id:`, `process:` not `type:` in arrival, `aggregate_rate:` top-level, `rate_fraction:` per client, distribution params under `params:` with `std_dev` not `stdev` + +--- + +## Step 0: Create Worktree + +Create an isolated workspace FIRST, before any other work. + +``` +/superpowers:using-git-worktrees h-$ARGUMENTS +``` + +All subsequent steps happen in the worktree. Set your working directory there. + +--- + +## Step 1: Select and Classify + +1. **Check coverage gaps**: Read `hypotheses/README.md` for the coverage table +2. **Classify the hypothesis**: + - **Family**: Which of the 6 families? (See `docs/contributing/standards/experiments.md`) + - **VV&UQ**: Verification, Validation, or UQ? + - **Type**: Deterministic or Statistical? If statistical: dominance, monotonicity, equivalence, or Pareto? +3. **Write the hypothesis sentence** using the family-specific pattern from experiments.md +4. **Add diagnostic clause**: "If this fails, it would indicate..." + +**WARNING**: Pose the hypothesis WITHOUT reading the code first. Code-grounded hypotheses test implementation, not behavior. + +--- + +## Step 2: Design + Design Review + +### 2a: Design the Experiment + +Follow ED-1 through ED-6 (see `docs/contributing/standards/experiments.md`): +- ED-1: Controlled comparison (vary exactly ONE dimension) +- ED-2: Rate awareness (run where effect expected AND where it should vanish) +- ED-3: Precondition verification (in script, not just prose) +- ED-4: Workload seed independence (3 seeds minimum for statistical: 42, 123, 456) +- ED-5: Reproducibility (everything from `run.sh` alone) +- ED-6: Config diff against referenced experiments + +**Compute parameters analytically** from alpha/beta coefficients. Do NOT guess. + +### 2b: Design Review (5 perspectives) + +Dispatch 5 parallel review agents using the convergence-review skill: + +``` +/convergence-review h-design +``` + +Alternatively, dispatch manually. See [review-prompts.md](review-prompts.md) Section A for exact prompts. + +**Perspectives**: (1) Hypothesis Quality, (2) ED Rigor, (3) Parameter Calibration, (4) Control Completeness, (5) DES/Domain Fit + +The convergence-review skill enforces the protocol automatically. If dispatching manually, apply the **convergence protocol**: +1. Launch all 5 in parallel as background Task agents (subagent_type="general-purpose", model=REVIEW_MODEL from `--model` flag, default "haiku") +2. Collect all findings classified as CRITICAL / IMPORTANT / SUGGESTION +3. **Zero CRITICAL + zero IMPORTANT = converged** -> proceed to Step 3 +4. **Any CRITICAL or IMPORTANT** -> fix all, re-run ENTIRE round (not just failed perspectives) +5. **Independently count findings yourself.** Do not trust agent summaries. + +--- + +## Step 3: Human Approval Gate — STOP HERE + +**Present the experiment design to the user.** Include: +- Hypothesis sentence + classification (family, VV&UQ, type) +- Experiment design summary (configurations, controlled variables, seeds) +- Parameter choices with analytical derivation +- Planned controls (one per proposed mechanism) +- Expected outcomes and diagnostic implications + +**This is a hard gate. WAIT for explicit human approval before proceeding.** + +Use the AskUserQuestion tool: +``` +"Do you approve this experiment design?" +Options: "Approve — proceed to implementation", "Revise — I have feedback" +``` + +--- + +## Step 4: Implement + +Create `hypotheses/h-$ARGUMENTS/run.sh` and `hypotheses/h-$ARGUMENTS/analyze.py`. + +### Mandatory harness requirements + +**run.sh** MUST: +```bash +#!/bin/bash +set -euo pipefail +SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" +source "$SCRIPT_DIR/../lib/harness.sh" +setup_experiment "${1:-}" +``` +- Use `blis_run` for EVERY simulation call (not `$BINARY run` directly) +- Every `blis_run` needs a timeout tier: `$TIMEOUT_QUICK` (<100 req), `$TIMEOUT_STANDARD` (100-500), `$TIMEOUT_EXTENDED` (>500) +- If using `--total-kv-blocks`, call `preflight_kv_check` first +- Call `python3 "$SCRIPT_DIR/analyze.py" ...` at the end + +**analyze.py** MUST: +```python +#!/usr/bin/env python3 +import sys +from pathlib import Path +sys.path.insert(0, str(Path(__file__).resolve().parent.parent / "lib")) +from analyze_helpers import parse_blis_output, check_for_timeout +``` +- Use `parse_blis_output()` for all metric extraction +- Check `metrics["timed_out"]` before computing ratios +- Print warnings to stderr, results to stdout +- Verify INV-1 conservation (4-term formula) for every run + +--- + +## Step 5: Code Review (5 perspectives) — BEFORE running + +**Every run.sh and analyze.py must be code-reviewed BEFORE running.** + +Dispatch 5 parallel review agents using the convergence-review skill: + +``` +/convergence-review h-code hypotheses/h-$ARGUMENTS/ +``` + +Alternatively, dispatch manually. See [review-prompts.md](review-prompts.md) Section B for exact prompts. + +**Perspectives**: (1) Parser-Output Agreement, (2) CLI Flag Correctness, (3) YAML Field Validation, (4) Config Diff (ED-6), (5) Seed and Determinism + +Apply the convergence protocol (same rules as Step 2b). + +**Cross-gate regression**: If this review finds a design flaw, loop back to Step 2. + +--- + +## Step 6: Run Experiments + +After Code Review converges: +- **Deterministic**: Single seed sufficient +- **Statistical**: Minimum 3 seeds (42, 123, 456) per configuration +- Execute: `bash hypotheses/h-$ARGUMENTS/run.sh` +- Verify reproducibility: running twice produces identical output + +--- + +## Step 7: Analyze and Document + +1. **Review analyzer output** from Step 6 +2. **Trace every causal claim through code** (RCV-1: cite `file:line`) +3. **Compute expected values from first principles** for any "surprises" (RCV-2) +4. **Check mechanism AND direction** (RCV-3) +5. **Write FINDINGS.md** using `docs/contributing/templates/hypothesis.md` — ALL sections must be non-empty +6. **Update `hypotheses/README.md`** — add row to the validated hypotheses table + +--- + +## Step 8: FINDINGS Review (10 perspectives) + +Dispatch 10 parallel review agents using the convergence-review skill: + +``` +/convergence-review h-findings hypotheses/h-$ARGUMENTS/FINDINGS.md +``` + +Alternatively, dispatch manually. See [review-prompts.md](review-prompts.md) Section C for exact prompts. + +**Perspectives**: (1) Code Verifier, (2) Experiment Designer, (3) Statistical Rigor, (4) Control Auditor, (5) Standards Compliance, (6) Substance/Logic, (7) DES Mechanism, (8) Reproducibility, (9) Cross-Experiment, (10) User Guidance + +Expected: 1-5 rounds (10 perspectives = higher quality bar). + +**CRITICAL**: Independently read every agent's output and count findings yourself. Do NOT rely on agent self-reported totals. + +**Cross-gate regression**: If design flaw found, loop back to Step 2. Max 2 regressions total. + +--- + +## Step 9: Self-Audit (6 dimensions) + +**This is NOT an agent pass.** Stop. Think critically. Answer each question yourself. + +1. **Logic bugs in analyzer**: Trace through `analyze.py` mentally. Edge cases? Silent defaults to 0? Integer vs float? +2. **Reproducibility**: Would `./run.sh` again produce identical output? Any non-deterministic dependencies? +3. **FINDINGS.md consistency**: Does Status match Results data? Does Devil's Advocate actually argue against the conclusion? +4. **Cross-experiment contradictions**: Do findings contradict prior experiments or MEMORY.md knowledge? +5. **User guidance**: Would a BLIS user know what to do with these findings? +6. **Issue filing completeness**: Every actionable finding has a planned issue? + +Fix all issues found. Then proceed to Step 10. + +--- + +## Step 10: Verify + Commit + PR + +### If code fixes were discovered: +```bash +go build ./... +go test ./... -count=1 +golangci-lint run ./... +``` +All three must pass before committing. + +### Commit and PR: +``` +/commit-commands:commit-push-pr +``` + +PR description must include: +- Hypothesis sentence and status +- Key findings (1-3 bullets) +- `Fixes #NNN` for any issues addressed + +### Post-PR: File issues per the taxonomy +- **Bug**: `--label bug` — code defects discovered +- **Enhancement**: `--label enhancement` — improvements needed +- **New hypothesis**: `--label hypothesis` — follow-up experiments (use `.github/ISSUE_TEMPLATE/hypothesis.md`) +- **Design limitation**: `--label design` — documented limitations +- **Standards update**: `--label standards` — new rules/invariants + +**Issue title format for hypotheses**: behavioral prediction ("X should Y"), NOT a task ("test X"). + +--- + +## Convergence Protocol Quick Reference + +| Rule | Detail | +|------|--------| +| **Converged** | Zero CRITICAL + zero IMPORTANT from ALL reviewers in current round | +| **Not converged** | Fix all issues, re-run ENTIRE round | +| **Max rounds** | 10 per gate (Design, Code, FINDINGS each independent) | +| **SUGGESTION items** | Do not block convergence | +| **Agent timeout** | 5 min per reviewer; if exceeded, check output and restart | +| **Agent failure** | Fall back to performing that review directly | +| **Severity doubtful?** | If fixing it would change a conclusion → IMPORTANT. If only readability → SUGGESTION | +| **Model for reviewers** | Default: haiku (~2-3 min, thorough reviews). Override via `/convergence-review --model sonnet\|opus` | diff --git a/.claude/skills/hypothesis-experiment/review-prompts.md b/.claude/skills/hypothesis-experiment/review-prompts.md new file mode 100644 index 0000000..89f963f --- /dev/null +++ b/.claude/skills/hypothesis-experiment/review-prompts.md @@ -0,0 +1,419 @@ +# Review Agent Dispatch Prompts + +Reference file for the hypothesis-experiment skill. Contains exact prompts for all 20 reviewer perspectives across the three review gates. + +**Canonical source:** `docs/contributing/hypothesis.md` (v2.0). If prompts here diverge from hypothesis.md, the process doc is authoritative. + +**Usage**: Launch each perspective as a parallel Task agent: +``` +Task(subagent_type="general-purpose", model=REVIEW_MODEL, run_in_background=True, + prompt="") +``` +Model selection is controlled by the `--model` flag in the convergence-review skill (default: `haiku`). + +**After all agents complete**: Read each output file independently. Count CRITICAL and IMPORTANT findings yourself. Do NOT trust agent-reported totals. + +--- + +## Section A: Design Review (5 perspectives) — Step 2 + +### DR-1: Hypothesis Quality + +``` +You are reviewing a hypothesis experiment design for the BLIS discrete-event simulator. + +HYPOTHESIS AND DESIGN: + + +YOUR FOCUS: Hypothesis Quality +- Is the hypothesis behavioral, testable, and diagnostic? +- Does it follow the family-specific sentence pattern from docs/contributing/standards/experiments.md? +- Is the diagnostic clause present ("If this fails, it would indicate...")? +- Is it correctly classified (family, VV&UQ category, type)? +- Is the hypothesis conceptual (not code-grounded)? It should NOT reference specific files/lines. + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### DR-2: Experiment Design Rigor (ED-1–ED-6) + +``` +You are reviewing a hypothesis experiment design for the BLIS discrete-event simulator. + +HYPOTHESIS AND DESIGN: + + +YOUR FOCUS: Experiment Design Rigor +Check compliance with each standard: +- ED-1: Is exactly ONE dimension varied between configurations? Everything else constant? +- ED-2: Is there a rate where the effect should vanish, to confirm mechanism dependence? +- ED-3: Are preconditions verified in the script (not just prose)? +- ED-4: Is seed handling correct? (3+ seeds for statistical: 42, 123, 456) +- ED-5: Is the experiment reproducible from run.sh alone? +- ED-6: If reusing calibration from a prior experiment, is the config diff documented? + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### DR-3: Parameter Calibration + +``` +You are reviewing a hypothesis experiment design for the BLIS discrete-event simulator. + +HYPOTHESIS AND DESIGN: + + +YOUR FOCUS: Parameter Calibration +- Are parameters computed analytically from known coefficients, not guessed? + - Beta coefficients (llama-3.1-8b, H100, TP=2): [6910.42, 17.67, 2.84] + stepTime = 6910.42 + 17.67*cacheMissTokens + 2.84*decodeTokens (microseconds) + - Alpha coefficients: [1601.35, 3.51, 1805.54] + queueDelay = 1601.35 + 3.51*inputLen; outputProcessing = 1805.54 (microseconds) +- Are capacity estimates matched to the actual workload mode? + - CLI mode (--rate): uses defaults prompt=512, output=512 -> step ~17.4ms, capacity ~57.4 req/s + - Workload-spec YAML: uses distributions defined in the YAML (compute step time from YAML params) +- Is the operating point correct? (near saturation for queueing effects, sub-saturation for baseline) +- Is --total-kv-blocks appropriate? (CLI default is overridden by defaults.yaml to 132139 for llama/H100/TP=2) + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### DR-4: Control Completeness + +``` +You are reviewing a hypothesis experiment design for the BLIS discrete-event simulator. + +HYPOTHESIS AND DESIGN: + + +YOUR FOCUS: Control Completeness +- Does every proposed mechanism have a planned control experiment? (RCV-4) +- Does each control isolate exactly one variable? +- Is the baseline configuration clearly defined? +- Are there confounding variables that could explain results without the proposed mechanism? + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### DR-5: DES and Domain Fit + +``` +You are reviewing a hypothesis experiment design for the BLIS discrete-event simulator. + +HYPOTHESIS AND DESIGN: + + +YOUR FOCUS: DES and Domain Fit +- Will the experiment create the conditions needed for the hypothesis to be testable? +- Are there DES-specific subtleties that could confound results? + - Alpha overhead (~4.3ms/req) is non-blocking but inflates E2E metrics + - Step quantization: step time is per-batch, not per-request + - Clock granularity: microsecond ticks + - Event ordering: (timestamp, priority, seqID) at cluster level +- Is the experiment duration sufficient? Request count adequate for the intended effect? +- Is the warmup period adequate (or does the experiment need cold-start behavior)? + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +--- + +## Section B: Code Review (5 perspectives) — Step 5 + +### CR-1: Parser–Output Format Agreement + +``` +You are code-reviewing a BLIS hypothesis experiment BEFORE it runs. + +FILES TO REVIEW: + + +YOUR FOCUS: Parser–Output Format Agreement +For every regex or field extraction in analyze.py, verify the pattern matches actual output: +- Read cmd/root.go — what text does the CLI print? (format strings for "Preemption Rate: %.4f", etc.) +- Read sim/metrics_utils.go — what JSON fields exist in MetricsOutput? +- Match every regex in analyze.py against the format string in the producer code +- SILENT DEFAULTS: verify that when a regex matches nothing, analyze.py warns to stderr rather than silently defaulting to 0 +- Check parse_blis_output() from hypotheses/lib/analyze_helpers.py — does it extract what this experiment needs? (NOTE: it does NOT extract dropped_unservable) + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### CR-2: CLI Flag Correctness + +``` +You are code-reviewing a BLIS hypothesis experiment BEFORE it runs. + +FILES TO REVIEW: + + +YOUR FOCUS: CLI Flag Correctness +For every flag in run.sh: +- Verify the flag name exists in cmd/root.go +- Verify the value type matches (string, int, float) +- Check for typos that strict YAML parsing would reject +- Verify --stderr comes BEFORE other flags in blis_run calls (harness checks position 3 only) +- Cross-reference blis_run timeout tiers: TIMEOUT_QUICK (<100 req), TIMEOUT_STANDARD (100-500), TIMEOUT_EXTENDED (>500) + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### CR-3: YAML Field Validation + +``` +You are code-reviewing a BLIS hypothesis experiment BEFORE it runs. + +FILES TO REVIEW: + + +YOUR FOCUS: YAML Field Validation +If the experiment uses workload-spec YAML files: +- Verify field names against sim/workload/spec.go struct tags +- Key gotchas: `id:` not `client_id:`, `process:` not `type:` in arrival, `aggregate_rate:` top-level, `rate_fraction:` per client, distribution params under `params:` with `std_dev` not `stdev`, `prefix_group`+`prefix_length` not `prefix_tokens` +- KnownFields(true) will reject typos at runtime — catching them now saves a failed run +- If YAML is generated inline in run.sh (heredoc), verify the heredoc syntax is correct + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### CR-4: Config Diff (ED-6) + +``` +You are code-reviewing a BLIS hypothesis experiment BEFORE it runs. + +FILES TO REVIEW: + + +YOUR FOCUS: Config Diff (ED-6) +- If the experiment reuses calibration from a prior experiment, diff every CLI flag and YAML field between the two +- Explicitly list all differences +- Verify the # Reference: comment in run.sh points to the correct file +- Check: does any changed flag (routing policy, seed, rate, blocks) invalidate the calibration? +- Evidence: H10 used --routing-policy least-loaded while H8 used round-robin, shifting the preemption cliff. Caught only in post-publication review. + +If no prior experiment is referenced, report "N/A — no referenced experiment" and move on. + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### CR-5: Seed and Determinism + +``` +You are code-reviewing a BLIS hypothesis experiment BEFORE it runs. + +FILES TO REVIEW: + + +YOUR FOCUS: Seed and Determinism +- Verify --seed is passed correctly in every blis_run call +- Verify workload YAML seed: field doesn't conflict with --seed (CLI overrides YAML when explicit) +- Verify seeds vary across runs as intended (ED-4) +- Check that run.sh builds the binary (setup_experiment) and is fully self-contained (ED-5) +- Verify run.sh sources hypotheses/lib/harness.sh +- Verify every blis_run call has a timeout tier (no bare $BINARY run calls) + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +--- + +## Section C: FINDINGS Review (10 perspectives) — Step 8 + +### FR-1: Code Verifier + +``` +You are reviewing a FINDINGS.md for a BLIS hypothesis experiment. + +FINDINGS FILE: + +YOUR FOCUS: Code Verification +- READ the actual source files cited in FINDINGS.md. Verify every file:line citation. +- Does the code at the cited location actually produce the claimed behavior? +- Are there off-by-one errors in line citations? (Acceptable: +/-2 lines. Flag: >2 lines off.) +- Does the mechanism explanation match what the code does, not just what it's named? + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### FR-2: Experiment Designer + +``` +You are reviewing a FINDINGS.md for a BLIS hypothesis experiment. + +FINDINGS FILE: +RUN SCRIPT: + +YOUR FOCUS: Experiment Design Compliance +- ED-1 through ED-6 compliance (controlled comparison, rate awareness, preconditions, seeds, reproducibility, config diff) +- Are there missing control experiments or confound matrix cells? +- Are parameters properly calibrated? +- Cross-reference every CLI flag in run.sh against cmd/root.go flag definitions +- Cross-reference every YAML field name against sim/workload/spec.go struct tags + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### FR-3: Statistical Rigor + +``` +You are reviewing a FINDINGS.md for a BLIS hypothesis experiment. + +FINDINGS FILE: + +YOUR FOCUS: Statistical Rigor +- Are "surprises" computed from first principles? (RCV-2) +- Is the sample size adequate (seeds, operating points)? +- Are claims properly scoped (not over-generalized)? +- Is the evidence quality table complete and honest? +- Effect size thresholds: >20% for dominance (ALL seeds), <5% for equivalence (ALL seeds), <10% in any seed = inconclusive +- Is the status classification consistent with the data? + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### FR-4: Control Experiment Auditor + +``` +You are reviewing a FINDINGS.md for a BLIS hypothesis experiment. + +FINDINGS FILE: +RUN SCRIPT: + +YOUR FOCUS: Control Experiment Audit +- Does every proposed mechanism (RCV-3) have a control experiment (RCV-4)? +- Were controls EXECUTED, not just proposed? Look for past tense with data vs conditional language ("could be confirmed by") +- Does each control isolate exactly one variable? Diff CLI flags between treatment and control in run.sh +- Do control results confirm or refute the proposed mechanism? +- Do Evidence Quality table entries reflect the current round (not stale from prior rounds)? + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### FR-5: Standards Compliance + +``` +You are reviewing a FINDINGS.md for a BLIS hypothesis experiment. + +FINDINGS FILE: + +YOUR FOCUS: Standards Compliance +- Are ALL FINDINGS.md sections present and non-empty? (per docs/contributing/templates/hypothesis.md) +- Is the hypothesis correctly classified (family, VV&UQ, type)? +- Does the Devil's Advocate section argue both directions convincingly? (RCV-5) +- Are Scope and Limitations complete? (RCV-6) Operating point, dependencies, what was NOT tested, generalizability, UQ +- Does the Standards Audit check against docs/contributing/standards/rules.md and docs/contributing/standards/invariants.md? +- Are any new rules or invariants warranted? + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### FR-6: Substance and Logic + +``` +You are reviewing a FINDINGS.md for a BLIS hypothesis experiment. + +FINDINGS FILE: + +YOUR FOCUS: Substance and Logic +- Are there logical errors in the conclusions? +- Are there mathematical mistakes in effect size calculations or statistical claims? +- Does the evidence actually support the claims? (Not just "the numbers are close enough") +- Are alternative explanations adequately considered? +- Does the analyzer verdict in analyze.py match the FINDINGS.md status? If not, is the discrepancy acknowledged? + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### FR-7: DES Mechanism Expert + +``` +You are reviewing a FINDINGS.md for a BLIS hypothesis experiment. + +FINDINGS FILE: + +YOUR FOCUS: DES Mechanism Analysis +- Are there event-ordering subtleties that could explain results differently? +- Are assumptions about DES timing correct? + - Alpha overhead (~4.3ms/req) is non-blocking (delays enqueue, not step advance) + - Step quantization: step time is per-batch + - Clock: microsecond ticks +- Could the result be a simulation artifact rather than modeled system behavior? +- Are routing snapshot freshness assumptions correct? (INV-7: PendingRequests = synchronous, KVUtilization = stale) + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### FR-8: Reproducibility and Robustness + +``` +You are reviewing a FINDINGS.md for a BLIS hypothesis experiment. + +FINDINGS FILE: +RUN SCRIPT: + +YOUR FOCUS: Reproducibility and Robustness +- Can run.sh reproduce results from scratch on a clean checkout? +- Are results fragile to small parameter variations? (Would +/-10% on key params change the conclusion?) +- Are all intermediate files generated by the script, not checked in as stale artifacts? +- Does run.sh build the binary (setup_experiment call)? +- Are there any non-deterministic dependencies (timestamps, system load, file ordering)? + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### FR-9: Cross-Experiment Consistency + +``` +You are reviewing a FINDINGS.md for a BLIS hypothesis experiment. + +FINDINGS FILE: + +YOUR FOCUS: Cross-Experiment Consistency +- Do findings contradict any prior experiment? If so, is the contradiction acknowledged and explained? +- Check references to prior experiments — are specific claims accurate? (Read the referenced FINDINGS.md) +- Are there stale references to prior review rounds that should have been updated? +- Key prior findings to check against: + - H4: RR and LL mean-equivalent at low load; LL p99 worse from tie-breaking bias + - H7: Horizontal scaling is super-linear (7.4x TTFT p99 for 4->8 instances) + - H20: ParetoLogNormal produces FEWER preemptions than Gaussian (median drives KV pressure) + - H23: Uniform workloads eliminate policy differentiation even under overload + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` + +### FR-10: User Guidance and Actionability + +``` +You are reviewing a FINDINGS.md for a BLIS hypothesis experiment. + +FINDINGS FILE: + +YOUR FOCUS: User Guidance and Actionability +- Are "Implications for Users" practical and specific enough to act on? +- Are proposed issues (bugs, enhancements, follow-up hypotheses) well-scoped? +- Would a BLIS user reading this understand what to do differently? +- Are findings classified correctly in the Findings Classification table? +- Is the promotion assessment complete? (Should any confirmed findings become Go tests or formal invariants?) + +Rate each finding as CRITICAL, IMPORTANT, or SUGGESTION. +Report: (1) numbered list of findings with severity, (2) total CRITICAL count, (3) total IMPORTANT count. +``` diff --git a/convergence-review.pptx b/convergence-review.pptx new file mode 100644 index 0000000..b410e54 Binary files /dev/null and b/convergence-review.pptx differ