From edbd7ee42c105cd1d3bf3fa45b990dbfde6193bd Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 1 Feb 2026 19:18:15 +0000 Subject: [PATCH 01/15] Add design doc for LLM code review pipeline with Codex CLI integration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Comprehensive design covering: - Codex CLI as transport boundary (three-phase pattern) - Durability via idempotent CheckpointUpsert (not traditional checkpoints) - State-as-values flowing through DAG edges - Loop-carried state for multi-turn conversations - Storage layout and checkpoint ID strategy (content-hashed) - Full DAG structure: CodexUpsert → GatherContext → ReviewLoop → FormatOutput - CLI interface design and configuration - Mock specifications for testing https://claude.ai/code/session_01RZryjdBirzK2p6KVyGttju --- TODO/llm-code-review-pipeline.md | 613 +++++++++++++++++++++++++++++++ 1 file changed, 613 insertions(+) create mode 100644 TODO/llm-code-review-pipeline.md diff --git a/TODO/llm-code-review-pipeline.md b/TODO/llm-code-review-pipeline.md new file mode 100644 index 00000000000..71fc7f8fb44 --- /dev/null +++ b/TODO/llm-code-review-pipeline.md @@ -0,0 +1,613 @@ +# LLM Code Review Pipeline + +**Status**: Design +**Date**: 2026-02-01 + +## Goal + +Build a robust LLM-powered code review pipeline that: +1. Integrates with **Codex CLI** as the primary LLM interface +2. Supports **local execution** with durable state (resume after interruption) +3. Models state as **typed DAG values** (no external state machine) +4. Composes cleanly with existing DAG patterns (Upsert, Retry, Loop) + +Future extension: GitHub Actions integration for automated PR reviews. + +--- + +## Design Principles + +### 1. State-as-Values (Not Checkpoints) + +Following the gunbc philosophy, durability is achieved through: +- **Idempotent operations** via Upsert pattern (check → skip if done) +- **State flowing through edges** as typed values +- **Loop-carried state** for multi-turn conversations +- **Explicit checkpoint nodes** that persist/restore state to disk + +This avoids a separate checkpoint/restore mechanism — the DAG structure *is* the state. + +### 2. Codex as a Transport Boundary + +Codex CLI invocations are **I/O boundaries** following the three-phase pattern: +``` +PrepareCodexRequest (pure) → TransportOps::Execute (I/O) → ParseCodexResponse (pure) +``` + +This ensures: +- Dry-run testing with mocked responses +- Clear separation of request construction from execution +- Deterministic replay of recorded sessions + +### 3. Fractal Composition + +The pipeline composes from smaller patterns: +``` +CodeReviewPipeline +├── CodexUpsert (check installed → install → verify) +├── ContextGather (git diff, file reads) +├── ReviewLoop (multi-turn if needed) +│ ├── PrepareReviewRequest +│ ├── ExecuteCodex +│ ├── ParseResponse +│ └── CheckpointState (persist progress) +└── OutputFormat (structured findings) +``` + +--- + +## Codex CLI Integration + +### CodexToolDef + +```rust +/// Codex CLI tool definition +pub static CODEX: CliToolDef = CliToolDef { + id: "codex", + check_cmd: &["codex", "--version"], + install_cmd: Some(&["npm", "install", "-g", "@openai/codex"]), + run_cmd: &["codex"], + description: "OpenAI Codex CLI for code generation and review", + access_mode: AccessMode::Exclusive, // One Codex session at a time +}; +``` + +### CodexOps + +```rust +/// Operations for Codex CLI integration +#[derive(Debug, Clone)] +pub enum CodexOps { + /// Prepare a Codex CLI invocation (PURE) + /// + /// Inputs: + /// - `prompt`: String — the review/generation prompt + /// - `context`: String — code context (diff, files) + /// - `config`: CodexConfig — model, temperature, etc. + /// - `session_state`: Option — for multi-turn + /// + /// Outputs: + /// - `request`: ShellRequest — CLI invocation + /// - `session_id`: String — for state tracking + PrepareRequest, + + /// Parse Codex CLI output (PURE) + /// + /// Inputs: + /// - `stdout`: String — CLI output + /// - `stderr`: String — CLI errors + /// - `exit_code`: Int + /// - `session_id`: String + /// + /// Outputs: + /// - `response`: CodexResponse — structured response + /// - `session_state`: CodexSessionState — updated state + /// - `success`: Bool + ParseResponse, + + /// Extract structured review findings (PURE) + /// + /// Inputs: + /// - `response`: CodexResponse + /// + /// Outputs: + /// - `findings`: Vec + /// - `summary`: String + /// - `severity`: ReviewSeverity + ExtractFindings, +} +``` + +### CodexConfig & State Types + +```rust +/// Configuration for Codex invocations +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct CodexConfig { + pub model: String, // e.g., "gpt-4o", "o3" + pub temperature: f64, // 0.0-1.0 + pub max_tokens: Option, + pub system_prompt: Option, + pub output_format: OutputFormat, // Text | Json | Markdown +} + +impl Default for CodexConfig { + fn default() -> Self { + Self { + model: "gpt-4o".into(), + temperature: 0.3, // Lower for code review + max_tokens: Some(4096), + system_prompt: None, + output_format: OutputFormat::Markdown, + } + } +} + +/// Session state for multi-turn conversations +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct CodexSessionState { + pub session_id: String, + pub turn_count: u32, + pub messages: Vec, + pub context_files: Vec, + pub partial_findings: Vec, +} + +/// A single finding from code review +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct ReviewFinding { + pub file: String, + pub line: Option, + pub severity: Severity, + pub category: Category, + pub message: String, + pub suggestion: Option, +} + +#[derive(Debug, Clone, Serialize, Deserialize)] +pub enum Severity { Info, Warning, Error, Critical } + +#[derive(Debug, Clone, Serialize, Deserialize)] +pub enum Category { + Bug, + Security, + Performance, + Style, + Documentation, + Testing, +} +``` + +--- + +## Durability Model + +### Philosophy: Idempotent Resume via Upsert + +Instead of checkpointing execution state, we use the **Upsert pattern** at each durable step: + +``` +CheckStepComplete → [Skip if done] → ExecuteStep → MarkComplete +``` + +The "check" reads from persistent storage; "mark complete" writes to it. + +### CheckpointNode Pattern + +```rust +/// Checkpoint operations for durable state +#[derive(Debug, Clone)] +pub enum CheckpointOps { + /// Check if a checkpoint exists (PURE read) + /// + /// Inputs: + /// - `checkpoint_id`: String + /// - `storage_path`: String (local file path) + /// + /// Outputs: + /// - `exists`: Bool + /// - `state`: Option — the stored state if exists + CheckExists, + + /// Save checkpoint to storage (I/O) + /// + /// Inputs: + /// - `checkpoint_id`: String + /// - `storage_path`: String + /// - `state`: Value — any serializable state + /// + /// Outputs: + /// - `success`: Bool + /// - `path`: String — actual file path written + Save, + + /// Load checkpoint from storage (I/O) + /// + /// Inputs: + /// - `checkpoint_id`: String + /// - `storage_path`: String + /// + /// Outputs: + /// - `state`: Value + /// - `timestamp`: Int — when saved + Load, + + /// Clear checkpoint (I/O) + /// + /// Inputs: + /// - `checkpoint_id`: String + /// - `storage_path`: String + /// + /// Outputs: + /// - `success`: Bool + Clear, +} +``` + +### Storage Layout + +``` +~/.gunbc/ +├── checkpoints/ +│ └── code-review/ +│ ├── session-{id}.json # Session state +│ ├── findings-{id}.json # Partial findings +│ └── context-{id}.json # Cached context +├── cache/ +│ └── codex/ +│ ├── responses/ # Response cache (by hash) +│ └── context/ # Context cache +└── config/ + └── codex.toml # User config +``` + +### Checkpoint ID Strategy + +Checkpoints are keyed by **content hash** of the operation inputs: +```rust +fn checkpoint_id(operation: &str, inputs: &[Value]) -> String { + let mut hasher = blake3::Hasher::new(); + hasher.update(operation.as_bytes()); + for input in inputs { + hasher.update(&serde_json::to_vec(input).unwrap()); + } + format!("{}-{}", operation, hasher.finalize().to_hex()[..12]) +} +``` + +This means: +- Same inputs → same checkpoint → skip re-execution +- Changed inputs → new checkpoint → fresh execution +- No manual cache invalidation needed + +--- + +## DAG Structure + +### Top-Level Pipeline + +``` +┌─────────────────────────────────────────────────────────────────────┐ +│ CodeReviewPipeline │ +│ │ +│ ┌──────────────┐ ┌───────────────┐ ┌────────────────────┐ │ +│ │ CodexUpsert │───▶│ GatherContext │───▶│ ReviewLoop │ │ +│ │ (install) │ │ (git diff) │ │ (multi-turn) │ │ +│ └──────────────┘ └───────────────┘ └─────────┬──────────┘ │ +│ │ │ +│ ┌─────────▼──────────┐ │ +│ │ FormatOutput │ │ +│ │ (findings) │ │ +│ └────────────────────┘ │ +└─────────────────────────────────────────────────────────────────────┘ +``` + +### Entrypoints (Unconnected Inputs) + +| Port | Type | Description | +|------|------|-------------| +| `target` | `String` | What to review: file path, git ref, or "staged" | +| `config` | `CodexConfig` | Review configuration | +| `storage_path` | `String` | Where to persist state (~/.gunbc/checkpoints) | + +### Boundaries (Unconnected Outputs) + +| Port | Type | Description | +|------|------|-------------| +| `findings` | `Vec` | Structured review results | +| `summary` | `String` | Human-readable summary | +| `session_id` | `String` | For resuming/referencing | + +### SubDag: GatherContext + +``` +┌─────────────────────────────────────────────────────────────────┐ +│ GatherContext │ +│ │ +│ ┌─────────────┐ │ +│ │ ParseTarget │──┬──▶ [GitDiff] ──┐ │ +│ │ (route) │ │ │ ┌────────────────┐ │ +│ └─────────────┘ ├──▶ [ReadFile] ──┼───▶│ MergeContext │ │ +│ │ │ │ (combine) │ │ +│ └──▶ [GitStaged]──┘ └────────────────┘ │ +│ │ +│ Branch on target type: │ +│ - "staged" → git diff --cached │ +│ - "HEAD~N" → git diff HEAD~N │ +│ - file path → read file directly │ +└─────────────────────────────────────────────────────────────────┘ +``` + +### SubDag: ReviewLoop (with Durability) + +``` +┌─────────────────────────────────────────────────────────────────────┐ +│ ReviewLoop │ +│ │ +│ ┌────────────────────────────────────────────────────────────┐ │ +│ │ WhileBuilder │ │ +│ │ condition: !response.is_complete │ │ +│ │ max_iterations: 5 │ │ +│ │ │ │ +│ │ ┌────────────────────────────────────────────────────┐ │ │ +│ │ │ Loop Body (per turn) │ │ │ +│ │ │ │ │ │ +│ │ │ ┌─────────────────┐ │ │ │ +│ │ │ │ CheckpointUpsert│ ◀── Check if turn done │ │ │ +│ │ │ │ (idempotent) │ │ │ │ +│ │ │ └───────┬─────────┘ │ │ │ +│ │ │ │ [skip if checkpoint exists] │ │ │ +│ │ │ ▼ │ │ │ +│ │ │ ┌─────────────────┐ │ │ │ +│ │ │ │ PrepareRequest │ ◀── Build Codex CLI args │ │ │ +│ │ │ │ (pure) │ │ │ │ +│ │ │ └───────┬─────────┘ │ │ │ +│ │ │ ▼ │ │ │ +│ │ │ ┌─────────────────┐ │ │ │ +│ │ │ │ ExecuteCodex │ ◀── I/O boundary │ │ │ +│ │ │ │ (transport) │ │ │ │ +│ │ │ └───────┬─────────┘ │ │ │ +│ │ │ ▼ │ │ │ +│ │ │ ┌─────────────────┐ │ │ │ +│ │ │ │ ParseResponse │ ◀── Extract structured data │ │ │ +│ │ │ │ (pure) │ │ │ │ +│ │ │ └───────┬─────────┘ │ │ │ +│ │ │ ▼ │ │ │ +│ │ │ ┌─────────────────┐ │ │ │ +│ │ │ │ SaveCheckpoint │ ◀── Persist for resume │ │ │ +│ │ │ │ (I/O) │ │ │ │ +│ │ │ └─────────────────┘ │ │ │ +│ │ │ │ │ │ +│ │ └────────────────────────────────────────────────────┘ │ │ +│ │ │ │ +│ │ loop_state: CodexSessionState (carried across turns) │ │ +│ └────────────────────────────────────────────────────────────┘ │ +│ │ +└─────────────────────────────────────────────────────────────────────┘ +``` + +### SubDag: CheckpointUpsert + +The idempotent checkpoint pattern: + +```rust +/// Build an upsert that checks/restores checkpoint before execution +pub fn build_checkpoint_upsert( + name: &str, + checkpoint_id_expr: &str, // How to compute checkpoint ID + body_dag: Dag, // The actual work +) -> Result, BuilderError> +where + T: From + From, +{ + UpsertBuilder::new(name) + // Check: Does checkpoint exist? + .check_op(CheckpointOps::CheckExists) + .check_output("exists", "Bool") + .check_output("state", "Option") + + // Create: Execute body (only if no checkpoint) + .create_dag(body_dag) + + // Resolve: Return state (from checkpoint or fresh execution) + .resolve_op(|check_result, create_result| { + if check_result.exists { + check_result.state // Use cached + } else { + create_result // Use fresh + } + }) + .build() +} +``` + +--- + +## CLI Interface + +### Commands + +```bash +# Review staged changes +gunbc review staged + +# Review specific files +gunbc review src/lib.rs src/main.rs + +# Review git range +gunbc review HEAD~3..HEAD + +# Resume interrupted session +gunbc review --resume session-abc123 + +# With custom config +gunbc review staged --model gpt-4o --temperature 0.2 + +# Output formats +gunbc review staged --format json > findings.json +gunbc review staged --format markdown > review.md +``` + +### Configuration File + +```toml +# ~/.gunbc/config/codex.toml + +[codex] +model = "gpt-4o" +temperature = 0.3 +max_tokens = 4096 + +[codex.review] +system_prompt = """ +You are an expert code reviewer. Focus on: +- Correctness and potential bugs +- Security vulnerabilities +- Performance issues +- Code clarity +Provide specific, actionable feedback with line numbers. +""" + +[storage] +checkpoint_dir = "~/.gunbc/checkpoints" +cache_responses = true +cache_ttl_hours = 24 +``` + +--- + +## Mock Specification + +```rust +/// Mock spec for testing code review pipeline +pub fn code_review_pipeline_mock_spec() -> MockSpec { + MockSpec::new("code-review-pipeline") + // Codex CLI execution mock + .boundary( + "execute_codex", + "stdout", + Value::Str(mock_codex_review_output()), + ) + .boundary("execute_codex", "stderr", Value::Str("".into())) + .boundary("execute_codex", "exit_code", Value::Int(0)) + + // Git diff mock + .boundary( + "git_diff", + "stdout", + Value::Str(mock_git_diff()), + ) + + // Checkpoint mocks + .boundary("check_checkpoint", "exists", Value::Bool(false)) + .boundary("save_checkpoint", "success", Value::Bool(true)) + + // Input expectations + .expects_input("target", InputConstraint::NonEmpty) + .expects_input("config", InputConstraint::NonEmpty) +} + +fn mock_codex_review_output() -> String { + r#"{ + "findings": [ + { + "file": "src/lib.rs", + "line": 42, + "severity": "warning", + "category": "bug", + "message": "Potential panic: unwrap() on Result that may be Err", + "suggestion": "Use `?` operator or handle the error explicitly" + } + ], + "summary": "Found 1 issue. Overall code quality is good.", + "complete": true + }"#.into() +} +``` + +--- + +## Implementation Tasks + +### Phase 1: Core Infrastructure + +- [ ] Define `CodexOps` enum in `lib/codex/src/ops.rs` +- [ ] Define state types (`CodexConfig`, `CodexSessionState`, `ReviewFinding`) +- [ ] Implement `Executable` for `CodexOps` +- [ ] Add `CODEX` to `CliToolDef` registry +- [ ] Create `CheckpointOps` for durable state + +### Phase 2: DAG Patterns + +- [ ] Build `GatherContext` SubDag (git diff, file read routing) +- [ ] Build `ReviewLoop` with `WhileBuilder` and loop-carried state +- [ ] Build `CheckpointUpsert` helper for idempotent steps +- [ ] Wire up `CodeReviewPipeline` top-level DAG +- [ ] Add mock specs for all boundaries + +### Phase 3: CLI Integration + +- [ ] Add `review` subcommand to gunbc CLI +- [ ] Implement config file loading (`~/.gunbc/config/codex.toml`) +- [ ] Add `--resume` flag for session recovery +- [ ] Add output format options (text, json, markdown) + +### Phase 4: Testing & Polish + +- [ ] Generate tests via testgen for pipeline DAG +- [ ] Add integration tests with mocked Codex responses +- [ ] Add real Codex integration test (requires API key) +- [ ] Documentation and examples + +### Future: GitHub Actions Integration + +- [ ] `GatherContext` variant that fetches PR diff via GitHub API +- [ ] Output formatting as PR comment +- [ ] GitHub Actions workflow template +- [ ] Rate limiting and error handling for API + +--- + +## Open Questions + +1. **Multi-file chunking**: Large diffs may exceed context window. Strategy? + - Option A: Chunk by file, review each, merge findings + - Option B: Summarize first, then deep-dive on flagged areas + - Option C: Use file-level priority (changed lines count) + +2. **Response parsing robustness**: Codex output may vary. How strict? + - Option A: Require JSON output format, fail on parse error + - Option B: LLM-assisted parsing (use another call to structure output) + - Option C: Regex/heuristic extraction with fallback + +3. **Session scope**: What defines a "session" for checkpointing? + - Option A: Per-invocation (new session each `gunbc review`) + - Option B: Per-target (same file = same session, resume by default) + - Option C: Explicit (`--session-id` flag) + +4. **Concurrent reviews**: Multiple reviews in parallel? + - Current: `AccessMode::Exclusive` prevents concurrent Codex + - Future: Could allow with separate session IDs + +--- + +## Related Patterns + +- **Upsert**: `core/ir/src/patterns/upsert.rs` — idempotent create/update +- **While/Retry**: `core/ir/src/patterns/repeat.rs` — iteration with state +- **CLI Tools**: `core/ir/src/transport/cli.rs` — CliToolDef, CliToolOp +- **LLM Ops**: `lib/llm-ops/src/` — existing chat completion patterns + +--- + +## Notes + +- The durability model is intentionally **not** a full checkpoint/restore system. + Instead, it leverages DAG structure: each step is idempotent, and resumption + is "re-run from start, skip completed steps." + +- Codex CLI is treated as a transport boundary, not a special case. This means + all existing testing infrastructure (DryRun, mocks, testgen) works automatically. + +- Loop-carried state in `WhileBuilder` is the key to multi-turn conversations. + The `CodexSessionState` accumulates messages and findings across turns. From c7ed3eed79d6f7f67a81e43b4bed42e8e45bd3ea Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 1 Feb 2026 19:36:42 +0000 Subject: [PATCH 02/15] Restructure LLM code review design with fractal workflow architecture MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Major restructuring around two composable phases: **Part 1: Fractal Workflow Architecture** - Meta-DAG showing full dev cycle: Requirements → Design → Implement → Review → Test → Commit - ReviewPhase appears at multiple points (reusable SubDag) - Maps existing primitives: GitOps, CargoOp, LlmOps, patterns **Part 2: ImplementationPhase (Codex Session)** - Step-level checkpointing for crash resilience - Content-addressed step IDs (same inputs = skip) - ImplementationSession tracks conversation state, artifacts, completed steps - StepUpsert pattern: CheckStepDone → Guard → Execute → SaveResult **Part 3: ReviewPhase (General-Purpose)** - Artifact + Rubric → Findings abstraction - Works for code, designs, tests, docs (same shape) - Predefined rubrics: code_review(), design_review(), test_review() - Multi-source review with parallel execution and MergeResults **Part 4: Composition** - ImplementAndReview workflow composing both phases - LoopBuilder for per-artifact review - BranchBuilder for pass/fail → iterate decision https://claude.ai/code/session_01RZryjdBirzK2p6KVyGttju --- TODO/llm-code-review-pipeline.md | 1255 +++++++++++++++++++----------- 1 file changed, 802 insertions(+), 453 deletions(-) diff --git a/TODO/llm-code-review-pipeline.md b/TODO/llm-code-review-pipeline.md index 71fc7f8fb44..aacbcbfc717 100644 --- a/TODO/llm-code-review-pipeline.md +++ b/TODO/llm-code-review-pipeline.md @@ -5,523 +5,887 @@ ## Goal -Build a robust LLM-powered code review pipeline that: -1. Integrates with **Codex CLI** as the primary LLM interface -2. Supports **local execution** with durable state (resume after interruption) -3. Models state as **typed DAG values** (no external state machine) -4. Composes cleanly with existing DAG patterns (Upsert, Retry, Loop) +Build a fractal DAG-based development workflow with: +1. **ImplementationPhase** — Codex CLI session management with crash-resilient durability +2. **ReviewPhase** — General-purpose, reusable review process for any artifact type -Future extension: GitHub Actions integration for automated PR reviews. +These phases compose into larger workflows (Requirements → Design → Implement → Review → Test → Commit) but are independently useful. --- -## Design Principles +## Part 1: Fractal Workflow Architecture -### 1. State-as-Values (Not Checkpoints) +### The Meta-Workflow -Following the gunbc philosophy, durability is achieved through: -- **Idempotent operations** via Upsert pattern (check → skip if done) -- **State flowing through edges** as typed values -- **Loop-carried state** for multi-turn conversations -- **Explicit checkpoint nodes** that persist/restore state to disk +A complete development cycle is a DAG of phases, where **ReviewPhase** appears multiple times: -This avoids a separate checkpoint/restore mechanism — the DAG structure *is* the state. +``` +┌─────────────────────────────────────────────────────────────────────────────┐ +│ DevWorkflow (Meta-DAG) │ +│ │ +│ ┌───────────────┐ ┌─────────────┐ ┌─────────────────────────────┐ │ +│ │ Requirements │───▶│ Design │───▶│ ImplementationPhase │ │ +│ │ (future) │ │ (future) │ │ (Codex session) │ │ +│ └───────────────┘ └──────┬──────┘ └──────────────┬──────────────┘ │ +│ │ │ │ +│ ▼ ▼ │ +│ ┌─────────────┐ ┌─────────────┐ │ +│ │ ReviewPhase │ │ ReviewPhase │ │ +│ │ (design) │ │ (code) │ │ +│ └─────────────┘ └──────┬──────┘ │ +│ │ │ +│ ▼ │ +│ ┌─────────────┐ │ +│ │ TestPhase │ │ +│ │ (cargo) │ │ +│ └──────┬──────┘ │ +│ │ │ +│ ▼ │ +│ ┌─────────────┐ │ +│ │ ReviewPhase │ │ +│ │ (tests) │ │ +│ └──────┬──────┘ │ +│ │ │ +│ ▼ │ +│ ┌─────────────┐ │ +│ │ CommitPhase │ │ +│ │ (git) │ │ +│ └─────────────┘ │ +└─────────────────────────────────────────────────────────────────────────────┘ +``` -### 2. Codex as a Transport Boundary +### Existing Primitives We Can Reuse -Codex CLI invocations are **I/O boundaries** following the three-phase pattern: -``` -PrepareCodexRequest (pure) → TransportOps::Execute (I/O) → ParseCodexResponse (pure) +| Primitive | Location | Purpose | +|-----------|----------|---------| +| `GitOps::PrepareDiff` | `lib/git-ops/` | Get diff for review context | +| `GitOps::PrepareCurrentBranch` | `lib/git-ops/` | Branch context | +| `CargoOp::test` | `lib/tools/cargo/` | Run tests | +| `CargoOp::check` | `lib/tools/cargo/` | Type checking | +| `LlmOps::PrepareChatRequest` | `lib/llm-ops/` | LLM invocation | +| `CliToolOp::Run` | `core/ir/src/transport/cli.rs` | CLI execution | +| `UpsertBuilder` | `core/ir/src/patterns/` | Idempotent operations | +| `WhileBuilder` | `core/ir/src/patterns/` | Loop with state | +| `BranchBuilder` | `core/ir/src/patterns/` | Conditional paths | + +### Focus: Implementation + Review + +For this iteration, we're building: +1. **ImplementationPhase** — Manage a Codex CLI session to generate/modify code +2. **ReviewPhase** — General-purpose review that works on any artifact + +--- + +## Part 2: ImplementationPhase (Codex Session) + +### Design Goals + +1. **Durability**: Survive crashes, resume seamlessly +2. **Statelessness**: All state flows through DAG edges (no hidden state) +3. **Idempotency**: Re-running from start skips completed work + +### The Challenge: Codex is Stateful + +Codex CLI maintains conversation state. Our challenge is to: +- Model this state explicitly in the DAG +- Persist it durably for crash recovery +- Resume without duplicating work or losing context + +### Session State Model + +```rust +/// Complete state of a Codex implementation session +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct ImplementationSession { + /// Unique session identifier (content-addressed) + pub session_id: String, + + /// The task being implemented + pub task: ImplementationTask, + + /// Completed steps (for idempotent resume) + pub completed_steps: Vec, + + /// Current Codex conversation state + pub conversation: ConversationState, + + /// Files created/modified by this session + pub artifacts: Vec, + + /// Session lifecycle + pub status: SessionStatus, +} + +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct ImplementationTask { + pub description: String, + pub context_files: Vec, // Files to read for context + pub target_files: Vec, // Files to create/modify + pub constraints: Vec, // Requirements to satisfy +} + +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct StepRecord { + pub step_id: String, // Content hash of step inputs + pub step_type: StepType, + pub completed_at: u64, // Unix timestamp + pub outputs: HashMap, // Step outputs for downstream +} + +#[derive(Debug, Clone, Serialize, Deserialize)] +pub enum StepType { + GatherContext, + CodexInvocation { turn: u32 }, + ApplyChanges, + Checkpoint, +} + +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct ConversationState { + pub messages: Vec, + pub turn_count: u32, + pub codex_session_file: Option, // Codex's own session file if applicable +} + +#[derive(Debug, Clone, Serialize, Deserialize)] +pub enum SessionStatus { + InProgress, + AwaitingReview, + Completed, + Failed { error: String }, +} ``` -This ensures: -- Dry-run testing with mocked responses -- Clear separation of request construction from execution -- Deterministic replay of recorded sessions +### Durability Strategy: Step-Level Checkpointing -### 3. Fractal Composition +Instead of checkpointing the entire session, we checkpoint **per step**: -The pipeline composes from smaller patterns: ``` -CodeReviewPipeline -├── CodexUpsert (check installed → install → verify) -├── ContextGather (git diff, file reads) -├── ReviewLoop (multi-turn if needed) -│ ├── PrepareReviewRequest -│ ├── ExecuteCodex -│ ├── ParseResponse -│ └── CheckpointState (persist progress) -└── OutputFormat (structured findings) +┌─────────────────────────────────────────────────────────────────────────────┐ +│ ImplementationPhase (Durable) │ +│ │ +│ For each step: │ +│ ┌─────────────────────────────────────────────────────────────────────┐ │ +│ │ StepUpsert Pattern │ │ +│ │ │ │ +│ │ ┌─────────────────┐ ┌─────────────────┐ │ │ +│ │ │ CheckStepDone │────▶│ Guard │ │ │ +│ │ │ (read storage) │ │ (skip if done) │ │ │ +│ │ └─────────────────┘ └────────┬────────┘ │ │ +│ │ │ │ │ +│ │ ┌─────────▼─────────┐ │ │ +│ │ │ ExecuteStep │ │ │ +│ │ │ (actual work) │ │ │ +│ │ └─────────┬─────────┘ │ │ +│ │ │ │ │ +│ │ ┌─────────▼─────────┐ │ │ +│ │ │ SaveStepResult │ │ │ +│ │ │ (write storage) │ │ │ +│ │ └───────────────────┘ │ │ +│ └─────────────────────────────────────────────────────────────────────┘ │ +│ │ +│ Steps execute in sequence, each wrapped in StepUpsert: │ +│ │ +│ 1. GatherContext ──▶ 2. CodexInvoke ──▶ 3. ApplyChanges ──▶ 4. Repeat? │ +│ │ │ +│ └─── (loop until complete or max turns) │ +└─────────────────────────────────────────────────────────────────────────────┘ ``` ---- - -## Codex CLI Integration +### Step ID: Content-Addressed -### CodexToolDef +Each step has a deterministic ID based on its inputs: ```rust -/// Codex CLI tool definition -pub static CODEX: CliToolDef = CliToolDef { - id: "codex", - check_cmd: &["codex", "--version"], - install_cmd: Some(&["npm", "install", "-g", "@openai/codex"]), - run_cmd: &["codex"], - description: "OpenAI Codex CLI for code generation and review", - access_mode: AccessMode::Exclusive, // One Codex session at a time -}; +fn step_id(step_type: &StepType, inputs: &HashMap) -> String { + let mut hasher = blake3::Hasher::new(); + hasher.update(&serde_json::to_vec(step_type).unwrap()); + + // Sort keys for determinism + let mut keys: Vec<_> = inputs.keys().collect(); + keys.sort(); + for key in keys { + hasher.update(key.as_bytes()); + hasher.update(&serde_json::to_vec(&inputs[key]).unwrap()); + } + + hasher.finalize().to_hex()[..16].to_string() +} +``` + +**Implications**: +- Same inputs → same step ID → skip if already done +- Changed inputs (e.g., new context) → new step ID → re-execute +- No manual invalidation needed + +### Crash Recovery Flow + ``` +┌──────────────────────────────────────────────────────────────────────────┐ +│ Resume After Crash │ +│ │ +│ 1. Load session manifest from ~/.gunbc/sessions/{session_id}.json │ +│ │ +│ 2. For each step in the workflow: │ +│ ┌────────────────────────────────────────────────────────────────┐ │ +│ │ step_id = compute_step_id(step_type, current_inputs) │ │ +│ │ │ │ +│ │ if session.completed_steps.contains(step_id): │ │ +│ │ # Step already done │ │ +│ │ outputs = load_step_outputs(step_id) │ │ +│ │ feed outputs to downstream steps │ │ +│ │ else: │ │ +│ │ # Step not done (or inputs changed) │ │ +│ │ outputs = execute_step(step_type, current_inputs) │ │ +│ │ save_step_result(step_id, outputs) │ │ +│ │ update session.completed_steps │ │ +│ └────────────────────────────────────────────────────────────────┘ │ +│ │ +│ 3. Continue from where we left off — no duplicate LLM calls │ +└──────────────────────────────────────────────────────────────────────────┘ +``` + +### Storage Layout -### CodexOps +``` +~/.gunbc/ +├── sessions/ +│ └── impl-{session_id}/ +│ ├── manifest.json # ImplementationSession +│ ├── steps/ +│ │ ├── {step_id}.json # StepRecord with outputs +│ │ └── ... +│ └── artifacts/ +│ ├── {file_path}.patch # Changes to apply +│ └── ... +├── cache/ +│ └── codex/ +│ └── context-{hash}.json # Cached context gathering +└── config/ + └── codex.toml +``` + +### ImplementationOps ```rust -/// Operations for Codex CLI integration #[derive(Debug, Clone)] -pub enum CodexOps { - /// Prepare a Codex CLI invocation (PURE) +pub enum ImplementationOps { + /// Initialize or resume a session + /// + /// Inputs: + /// - `task`: ImplementationTask + /// - `storage_path`: String + /// + /// Outputs: + /// - `session`: ImplementationSession + /// - `is_resume`: Bool + InitSession, + + /// Check if a step is already complete + /// + /// Inputs: + /// - `session`: ImplementationSession + /// - `step_type`: StepType + /// - `step_inputs`: HashMap + /// + /// Outputs: + /// - `done`: Bool + /// - `cached_outputs`: Option> + CheckStepDone, + + /// Record step completion + /// + /// Inputs: + /// - `session`: ImplementationSession + /// - `step_id`: String + /// - `step_type`: StepType + /// - `outputs`: HashMap + /// + /// Outputs: + /// - `session`: ImplementationSession (updated) + RecordStep, + + /// Prepare Codex CLI invocation /// /// Inputs: - /// - `prompt`: String — the review/generation prompt - /// - `context`: String — code context (diff, files) - /// - `config`: CodexConfig — model, temperature, etc. - /// - `session_state`: Option — for multi-turn + /// - `conversation`: ConversationState + /// - `prompt`: String + /// - `config`: CodexConfig /// /// Outputs: - /// - `request`: ShellRequest — CLI invocation - /// - `session_id`: String — for state tracking - PrepareRequest, + /// - `request`: ShellRequest + PrepareCodexRequest, - /// Parse Codex CLI output (PURE) + /// Parse Codex CLI response /// /// Inputs: - /// - `stdout`: String — CLI output - /// - `stderr`: String — CLI errors + /// - `stdout`: String + /// - `stderr`: String /// - `exit_code`: Int - /// - `session_id`: String + /// - `conversation`: ConversationState /// /// Outputs: - /// - `response`: CodexResponse — structured response - /// - `session_state`: CodexSessionState — updated state - /// - `success`: Bool - ParseResponse, + /// - `response`: CodexResponse + /// - `conversation`: ConversationState (updated) + /// - `complete`: Bool + ParseCodexResponse, - /// Extract structured review findings (PURE) + /// Extract file changes from Codex response /// /// Inputs: /// - `response`: CodexResponse /// /// Outputs: - /// - `findings`: Vec - /// - `summary`: String - /// - `severity`: ReviewSeverity - ExtractFindings, + /// - `artifacts`: Vec + /// - `needs_followup`: Bool + ExtractArtifacts, } ``` -### CodexConfig & State Types +### CodexConfig ```rust -/// Configuration for Codex invocations #[derive(Debug, Clone, Serialize, Deserialize)] pub struct CodexConfig { - pub model: String, // e.g., "gpt-4o", "o3" - pub temperature: f64, // 0.0-1.0 - pub max_tokens: Option, - pub system_prompt: Option, - pub output_format: OutputFormat, // Text | Json | Markdown + /// Model to use (e.g., "gpt-4o", "o3") + pub model: String, + + /// Approval mode for file changes + pub approval_mode: ApprovalMode, + + /// Working directory for Codex + pub working_dir: Option, + + /// Additional CLI flags + pub extra_args: Vec, +} + +#[derive(Debug, Clone, Serialize, Deserialize)] +pub enum ApprovalMode { + /// Auto-approve all changes (for automation) + AutoApprove, + /// Require explicit approval (for interactive) + RequireApproval, + /// Auto-approve safe changes, prompt for risky ones + SafeAutoApprove, } impl Default for CodexConfig { fn default() -> Self { Self { model: "gpt-4o".into(), - temperature: 0.3, // Lower for code review - max_tokens: Some(4096), - system_prompt: None, - output_format: OutputFormat::Markdown, + approval_mode: ApprovalMode::SafeAutoApprove, + working_dir: None, + extra_args: vec![], } } } +``` + +--- + +## Part 3: ReviewPhase (General-Purpose) + +### Design Goals + +1. **Reusability**: Same ReviewPhase works for code, designs, tests, docs +2. **Modularity**: ReviewPhase is a self-contained SubDag +3. **Composability**: Easily plugs into any workflow point + +### Abstraction: Artifact + Rubric → Findings + +The key insight is that **all reviews share the same shape**: + +``` +┌─────────────────────────────────────────────────────────────────────────┐ +│ ReviewPhase (Generic) │ +│ │ +│ Inputs: │ +│ ┌────────────────┐ ┌────────────────┐ ┌────────────────────────┐ │ +│ │ artifact │ │ rubric │ │ context (optional) │ │ +│ │ (what) │ │ (criteria) │ │ (background info) │ │ +│ └───────┬────────┘ └───────┬────────┘ └───────────┬────────────┘ │ +│ │ │ │ │ +│ └───────────────────┴───────────────────────┘ │ +│ │ │ +│ ▼ │ +│ ┌─────────────────┐ │ +│ │ PrepareReview │ │ +│ │ (build prompt) │ │ +│ └────────┬────────┘ │ +│ │ │ +│ ▼ │ +│ ┌─────────────────┐ │ +│ │ ExecuteReview │ ◀── LLM / Human / Automated │ +│ │ (I/O boundary) │ │ +│ └────────┬────────┘ │ +│ │ │ +│ ▼ │ +│ ┌─────────────────┐ │ +│ │ ParseFindings │ │ +│ │ (extract) │ │ +│ └────────┬────────┘ │ +│ │ │ +│ Outputs: ▼ │ +│ ┌────────────────┐ ┌────────────────┐ ┌────────────────────────┐ │ +│ │ findings │ │ verdict │ │ suggestions │ │ +│ │ (issues) │ │ (pass/fail) │ │ (improvements) │ │ +│ └────────────────┘ └────────────────┘ └────────────────────────┘ │ +└─────────────────────────────────────────────────────────────────────────┘ +``` + +### Core Types + +```rust +/// What is being reviewed +#[derive(Debug, Clone, Serialize, Deserialize)] +pub enum Artifact { + /// Source code (diff or full file) + Code { + content: String, + file_path: Option, + language: Option, + is_diff: bool, + }, + /// Design document + Design { + content: String, + format: DocFormat, // Markdown, PlainText, Structured + }, + /// Test results or test code + TestOutput { + stdout: String, + stderr: String, + exit_code: i32, + test_count: Option, + }, + /// Generic text artifact + Text { + content: String, + artifact_type: String, // e.g., "commit_message", "pr_description" + }, +} -/// Session state for multi-turn conversations +/// Review criteria #[derive(Debug, Clone, Serialize, Deserialize)] -pub struct CodexSessionState { - pub session_id: String, - pub turn_count: u32, - pub messages: Vec, - pub context_files: Vec, - pub partial_findings: Vec, +pub struct Rubric { + /// What aspects to evaluate + pub criteria: Vec, + + /// Severity thresholds + pub fail_on: FailureCondition, + + /// Domain-specific instructions + pub domain_prompt: Option, } -/// A single finding from code review #[derive(Debug, Clone, Serialize, Deserialize)] -pub struct ReviewFinding { - pub file: String, - pub line: Option, +pub struct Criterion { + pub name: String, // e.g., "correctness", "security" + pub description: String, // What to look for + pub weight: f32, // Importance (0.0-1.0) + pub examples: Vec, // Example issues +} + +#[derive(Debug, Clone, Serialize, Deserialize)] +pub enum FailureCondition { + /// Fail if any finding of this severity or higher + OnSeverity(Severity), + /// Fail if score below threshold + OnScore(f32), + /// Never auto-fail, always advisory + Advisory, +} + +/// Review output +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct ReviewResult { + pub findings: Vec, + pub verdict: Verdict, + pub summary: String, + pub score: Option, // 0.0-1.0 if scored + pub reviewer: ReviewerType, +} + +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct Finding { + pub id: String, pub severity: Severity, - pub category: Category, + pub category: String, // Maps to Criterion.name + pub location: Option, pub message: String, pub suggestion: Option, + pub confidence: f32, // 0.0-1.0 } #[derive(Debug, Clone, Serialize, Deserialize)] -pub enum Severity { Info, Warning, Error, Critical } +pub struct Location { + pub file: Option, + pub line_start: Option, + pub line_end: Option, + pub column: Option, +} #[derive(Debug, Clone, Serialize, Deserialize)] -pub enum Category { - Bug, - Security, - Performance, - Style, - Documentation, - Testing, +pub enum Verdict { + Pass, + Fail { blocking_findings: Vec }, // Finding IDs + NeedsWork { suggested_changes: u32 }, } -``` ---- +#[derive(Debug, Clone, Serialize, Deserialize)] +pub enum Severity { + Info, // Informational only + Suggestion, // Nice to have + Warning, // Should address + Error, // Must fix + Critical, // Blocking +} -## Durability Model +#[derive(Debug, Clone, Serialize, Deserialize)] +pub enum ReviewerType { + Llm { model: String }, + Automated { tool: String }, // e.g., "clippy", "eslint" + Human { reviewer: String }, +} +``` -### Philosophy: Idempotent Resume via Upsert +### Predefined Rubrics -Instead of checkpointing execution state, we use the **Upsert pattern** at each durable step: +```rust +impl Rubric { + /// Standard code review rubric + pub fn code_review() -> Self { + Self { + criteria: vec![ + Criterion { + name: "correctness".into(), + description: "Logic errors, bugs, edge cases".into(), + weight: 1.0, + examples: vec![ + "Off-by-one error".into(), + "Null pointer dereference".into(), + ], + }, + Criterion { + name: "security".into(), + description: "Vulnerabilities, unsafe practices".into(), + weight: 1.0, + examples: vec![ + "SQL injection".into(), + "Hardcoded secrets".into(), + ], + }, + Criterion { + name: "performance".into(), + description: "Inefficiencies, scaling issues".into(), + weight: 0.7, + examples: vec![ + "N+1 query".into(), + "Unnecessary allocation in loop".into(), + ], + }, + Criterion { + name: "maintainability".into(), + description: "Readability, structure, naming".into(), + weight: 0.5, + examples: vec![ + "Unclear variable name".into(), + "Function too long".into(), + ], + }, + ], + fail_on: FailureCondition::OnSeverity(Severity::Error), + domain_prompt: None, + } + } -``` -CheckStepComplete → [Skip if done] → ExecuteStep → MarkComplete -``` + /// Design document review rubric + pub fn design_review() -> Self { + Self { + criteria: vec![ + Criterion { + name: "completeness".into(), + description: "All necessary sections present".into(), + weight: 1.0, + examples: vec![], + }, + Criterion { + name: "clarity".into(), + description: "Clear, unambiguous language".into(), + weight: 0.8, + examples: vec![], + }, + Criterion { + name: "feasibility".into(), + description: "Technically achievable".into(), + weight: 1.0, + examples: vec![], + }, + Criterion { + name: "consistency".into(), + description: "No contradictions".into(), + weight: 0.9, + examples: vec![], + }, + ], + fail_on: FailureCondition::Advisory, + domain_prompt: None, + } + } -The "check" reads from persistent storage; "mark complete" writes to it. + /// Test coverage/quality review + pub fn test_review() -> Self { + Self { + criteria: vec![ + Criterion { + name: "coverage".into(), + description: "Key paths tested".into(), + weight: 1.0, + examples: vec![], + }, + Criterion { + name: "edge_cases".into(), + description: "Boundary conditions handled".into(), + weight: 0.9, + examples: vec![], + }, + Criterion { + name: "clarity".into(), + description: "Test intent is clear".into(), + weight: 0.6, + examples: vec![], + }, + ], + fail_on: FailureCondition::OnSeverity(Severity::Warning), + domain_prompt: None, + } + } +} +``` -### CheckpointNode Pattern +### ReviewOps ```rust -/// Checkpoint operations for durable state #[derive(Debug, Clone)] -pub enum CheckpointOps { - /// Check if a checkpoint exists (PURE read) +pub enum ReviewOps { + /// Build review prompt from artifact + rubric /// /// Inputs: - /// - `checkpoint_id`: String - /// - `storage_path`: String (local file path) + /// - `artifact`: Artifact + /// - `rubric`: Rubric + /// - `context`: Option /// /// Outputs: - /// - `exists`: Bool - /// - `state`: Option — the stored state if exists - CheckExists, + /// - `prompt`: String + /// - `system_prompt`: String + PrepareReviewPrompt, - /// Save checkpoint to storage (I/O) + /// Parse LLM response into structured findings /// /// Inputs: - /// - `checkpoint_id`: String - /// - `storage_path`: String - /// - `state`: Value — any serializable state + /// - `response`: String (LLM output) + /// - `rubric`: Rubric (for category mapping) /// /// Outputs: - /// - `success`: Bool - /// - `path`: String — actual file path written - Save, + /// - `result`: ReviewResult + ParseReviewResponse, - /// Load checkpoint from storage (I/O) + /// Determine verdict from findings /// /// Inputs: - /// - `checkpoint_id`: String - /// - `storage_path`: String + /// - `findings`: Vec + /// - `fail_condition`: FailureCondition /// /// Outputs: - /// - `state`: Value - /// - `timestamp`: Int — when saved - Load, + /// - `verdict`: Verdict + /// - `blocking_count`: Int + DetermineVerdict, - /// Clear checkpoint (I/O) + /// Merge findings from multiple review sources /// /// Inputs: - /// - `checkpoint_id`: String - /// - `storage_path`: String + /// - `results`: Vec /// /// Outputs: - /// - `success`: Bool - Clear, -} -``` - -### Storage Layout - -``` -~/.gunbc/ -├── checkpoints/ -│ └── code-review/ -│ ├── session-{id}.json # Session state -│ ├── findings-{id}.json # Partial findings -│ └── context-{id}.json # Cached context -├── cache/ -│ └── codex/ -│ ├── responses/ # Response cache (by hash) -│ └── context/ # Context cache -└── config/ - └── codex.toml # User config -``` - -### Checkpoint ID Strategy - -Checkpoints are keyed by **content hash** of the operation inputs: -```rust -fn checkpoint_id(operation: &str, inputs: &[Value]) -> String { - let mut hasher = blake3::Hasher::new(); - hasher.update(operation.as_bytes()); - for input in inputs { - hasher.update(&serde_json::to_vec(input).unwrap()); - } - format!("{}-{}", operation, hasher.finalize().to_hex()[..12]) + /// - `merged`: ReviewResult + MergeResults, } ``` -This means: -- Same inputs → same checkpoint → skip re-execution -- Changed inputs → new checkpoint → fresh execution -- No manual cache invalidation needed - ---- - -## DAG Structure - -### Top-Level Pipeline +### ReviewPhase DAG ``` -┌─────────────────────────────────────────────────────────────────────┐ -│ CodeReviewPipeline │ -│ │ -│ ┌──────────────┐ ┌───────────────┐ ┌────────────────────┐ │ -│ │ CodexUpsert │───▶│ GatherContext │───▶│ ReviewLoop │ │ -│ │ (install) │ │ (git diff) │ │ (multi-turn) │ │ -│ └──────────────┘ └───────────────┘ └─────────┬──────────┘ │ -│ │ │ -│ ┌─────────▼──────────┐ │ -│ │ FormatOutput │ │ -│ │ (findings) │ │ -│ └────────────────────┘ │ -└─────────────────────────────────────────────────────────────────────┘ +┌─────────────────────────────────────────────────────────────────────────────┐ +│ ReviewPhase SubDag │ +│ │ +│ Entrypoints: │ +│ ┌──────────┐ ┌──────────┐ ┌──────────┐ ┌──────────┐ │ +│ │ artifact │ │ rubric │ │ context │ │ config │ │ +│ └────┬─────┘ └────┬─────┘ └────┬─────┘ └────┬─────┘ │ +│ │ │ │ │ │ +│ └────────────┴────────────┴────────────┘ │ +│ │ │ +│ ▼ │ +│ ┌─────────────────────────┐ │ +│ │ PrepareReviewPrompt │ ◀── PURE: build LLM prompt │ +│ │ (ReviewOps) │ │ +│ └────────────┬────────────┘ │ +│ │ │ +│ ▼ │ +│ ┌─────────────────────────┐ │ +│ │ PrepareChatRequest │ ◀── PURE: build transport req │ +│ │ (LlmOps) │ │ +│ └────────────┬────────────┘ │ +│ │ │ +│ ▼ │ +│ ┌─────────────────────────┐ │ +│ │ Execute │ ◀── I/O BOUNDARY │ +│ │ (TransportOps) │ │ +│ └────────────┬────────────┘ │ +│ │ │ +│ ▼ │ +│ ┌─────────────────────────┐ │ +│ │ ParseChatResponse │ ◀── PURE: extract content │ +│ │ (LlmOps) │ │ +│ └────────────┬────────────┘ │ +│ │ │ +│ ▼ │ +│ ┌─────────────────────────┐ │ +│ │ ParseReviewResponse │ ◀── PURE: structure findings │ +│ │ (ReviewOps) │ │ +│ └────────────┬────────────┘ │ +│ │ │ +│ ▼ │ +│ ┌─────────────────────────┐ │ +│ │ DetermineVerdict │ ◀── PURE: pass/fail decision │ +│ │ (ReviewOps) │ │ +│ └────────────┬────────────┘ │ +│ │ │ +│ Boundaries: ▼ │ +│ ┌──────────┐ ┌──────────┐ ┌──────────┐ │ +│ │ findings │ │ verdict │ │ summary │ │ +│ └──────────┘ └──────────┘ └──────────┘ │ +└─────────────────────────────────────────────────────────────────────────────┘ ``` -### Entrypoints (Unconnected Inputs) - -| Port | Type | Description | -|------|------|-------------| -| `target` | `String` | What to review: file path, git ref, or "staged" | -| `config` | `CodexConfig` | Review configuration | -| `storage_path` | `String` | Where to persist state (~/.gunbc/checkpoints) | - -### Boundaries (Unconnected Outputs) - -| Port | Type | Description | -|------|------|-------------| -| `findings` | `Vec` | Structured review results | -| `summary` | `String` | Human-readable summary | -| `session_id` | `String` | For resuming/referencing | - -### SubDag: GatherContext - -``` -┌─────────────────────────────────────────────────────────────────┐ -│ GatherContext │ -│ │ -│ ┌─────────────┐ │ -│ │ ParseTarget │──┬──▶ [GitDiff] ──┐ │ -│ │ (route) │ │ │ ┌────────────────┐ │ -│ └─────────────┘ ├──▶ [ReadFile] ──┼───▶│ MergeContext │ │ -│ │ │ │ (combine) │ │ -│ └──▶ [GitStaged]──┘ └────────────────┘ │ -│ │ -│ Branch on target type: │ -│ - "staged" → git diff --cached │ -│ - "HEAD~N" → git diff HEAD~N │ -│ - file path → read file directly │ -└─────────────────────────────────────────────────────────────────┘ -``` +### Multi-Source Review (Parallel) -### SubDag: ReviewLoop (with Durability) +For comprehensive review, run multiple reviewers in parallel: ``` -┌─────────────────────────────────────────────────────────────────────┐ -│ ReviewLoop │ -│ │ -│ ┌────────────────────────────────────────────────────────────┐ │ -│ │ WhileBuilder │ │ -│ │ condition: !response.is_complete │ │ -│ │ max_iterations: 5 │ │ -│ │ │ │ -│ │ ┌────────────────────────────────────────────────────┐ │ │ -│ │ │ Loop Body (per turn) │ │ │ -│ │ │ │ │ │ -│ │ │ ┌─────────────────┐ │ │ │ -│ │ │ │ CheckpointUpsert│ ◀── Check if turn done │ │ │ -│ │ │ │ (idempotent) │ │ │ │ -│ │ │ └───────┬─────────┘ │ │ │ -│ │ │ │ [skip if checkpoint exists] │ │ │ -│ │ │ ▼ │ │ │ -│ │ │ ┌─────────────────┐ │ │ │ -│ │ │ │ PrepareRequest │ ◀── Build Codex CLI args │ │ │ -│ │ │ │ (pure) │ │ │ │ -│ │ │ └───────┬─────────┘ │ │ │ -│ │ │ ▼ │ │ │ -│ │ │ ┌─────────────────┐ │ │ │ -│ │ │ │ ExecuteCodex │ ◀── I/O boundary │ │ │ -│ │ │ │ (transport) │ │ │ │ -│ │ │ └───────┬─────────┘ │ │ │ -│ │ │ ▼ │ │ │ -│ │ │ ┌─────────────────┐ │ │ │ -│ │ │ │ ParseResponse │ ◀── Extract structured data │ │ │ -│ │ │ │ (pure) │ │ │ │ -│ │ │ └───────┬─────────┘ │ │ │ -│ │ │ ▼ │ │ │ -│ │ │ ┌─────────────────┐ │ │ │ -│ │ │ │ SaveCheckpoint │ ◀── Persist for resume │ │ │ -│ │ │ │ (I/O) │ │ │ │ -│ │ │ └─────────────────┘ │ │ │ -│ │ │ │ │ │ -│ │ └────────────────────────────────────────────────────┘ │ │ -│ │ │ │ -│ │ loop_state: CodexSessionState (carried across turns) │ │ -│ └────────────────────────────────────────────────────────────┘ │ -│ │ -└─────────────────────────────────────────────────────────────────────┘ -``` - -### SubDag: CheckpointUpsert - -The idempotent checkpoint pattern: - -```rust -/// Build an upsert that checks/restores checkpoint before execution -pub fn build_checkpoint_upsert( - name: &str, - checkpoint_id_expr: &str, // How to compute checkpoint ID - body_dag: Dag, // The actual work -) -> Result, BuilderError> -where - T: From + From, -{ - UpsertBuilder::new(name) - // Check: Does checkpoint exist? - .check_op(CheckpointOps::CheckExists) - .check_output("exists", "Bool") - .check_output("state", "Option") - - // Create: Execute body (only if no checkpoint) - .create_dag(body_dag) - - // Resolve: Return state (from checkpoint or fresh execution) - .resolve_op(|check_result, create_result| { - if check_result.exists { - check_result.state // Use cached - } else { - create_result // Use fresh - } - }) - .build() -} +┌─────────────────────────────────────────────────────────────────────────────┐ +│ MultiReview (Parallel + Merge) │ +│ │ +│ ┌─────────────────────────────────────────────┐ │ +│ │ artifact + rubric │ │ +│ └─────────┬───────────────┬──────────────┬────┘ │ +│ │ │ │ │ +│ ┌─────────▼─────┐ ┌───────▼───────┐ ┌────▼────────┐ │ +│ │ LLM Review │ │ Clippy/Lint │ │ Type Check │ │ +│ │ (ReviewPhase) │ │ (CargoOp) │ │ (CargoOp) │ │ +│ └───────┬───────┘ └───────┬───────┘ └──────┬──────┘ │ +│ │ │ │ │ +│ └─────────────────┴────────────────┘ │ +│ │ │ +│ ┌──────────▼──────────┐ │ +│ │ MergeResults │ │ +│ │ (ReviewOps) │ │ +│ └──────────┬──────────┘ │ +│ │ │ +│ ┌───────▼───────┐ │ +│ │ unified result│ │ +│ └───────────────┘ │ +└─────────────────────────────────────────────────────────────────────────────┘ ``` --- -## CLI Interface - -### Commands - -```bash -# Review staged changes -gunbc review staged - -# Review specific files -gunbc review src/lib.rs src/main.rs - -# Review git range -gunbc review HEAD~3..HEAD +## Part 4: Composition Example -# Resume interrupted session -gunbc review --resume session-abc123 +### Implement-and-Review Workflow -# With custom config -gunbc review staged --model gpt-4o --temperature 0.2 +Compose ImplementationPhase + ReviewPhase into a complete workflow: -# Output formats -gunbc review staged --format json > findings.json -gunbc review staged --format markdown > review.md ``` - -### Configuration File - -```toml -# ~/.gunbc/config/codex.toml - -[codex] -model = "gpt-4o" -temperature = 0.3 -max_tokens = 4096 - -[codex.review] -system_prompt = """ -You are an expert code reviewer. Focus on: -- Correctness and potential bugs -- Security vulnerabilities -- Performance issues -- Code clarity -Provide specific, actionable feedback with line numbers. -""" - -[storage] -checkpoint_dir = "~/.gunbc/checkpoints" -cache_responses = true -cache_ttl_hours = 24 +┌─────────────────────────────────────────────────────────────────────────────┐ +│ ImplementAndReview Workflow │ +│ │ +│ ┌─────────────────────────────────────────────────────────────────────┐ │ +│ │ ImplementationPhase │ │ +│ │ (Codex session with durability) │ │ +│ └─────────────────────────────┬───────────────────────────────────────┘ │ +│ │ │ +│ │ artifacts: Vec │ +│ ▼ │ +│ ┌─────────────────────────────────────────────────────────────────────┐ │ +│ │ LoopBuilder │ │ +│ │ (review each artifact) │ │ +│ │ │ │ +│ │ ┌─────────────────────────────────────────────────────────────┐ │ │ +│ │ │ ReviewPhase │ │ │ +│ │ │ rubric: Rubric::code_review() │ │ │ +│ │ └─────────────────────────────────────────────────────────────┘ │ │ +│ └─────────────────────────────┬───────────────────────────────────────┘ │ +│ │ │ +│ │ review_results: Vec │ +│ ▼ │ +│ ┌─────────────────────────────────────────────────────────────────────┐ │ +│ │ BranchBuilder │ │ +│ │ condition: all_passed(review_results) │ │ +│ │ │ │ +│ │ ┌─────────────────┐ ┌─────────────────┐ │ │ +│ │ │ True: Complete │ │ False: Iterate │ │ │ +│ │ │ → output result │ │ → back to Impl │ │ │ +│ │ └─────────────────┘ └─────────────────┘ │ │ +│ └─────────────────────────────────────────────────────────────────────┘ │ +└─────────────────────────────────────────────────────────────────────────────┘ ``` ---- - -## Mock Specification +### Builder API (Proposed) ```rust -/// Mock spec for testing code review pipeline -pub fn code_review_pipeline_mock_spec() -> MockSpec { - MockSpec::new("code-review-pipeline") - // Codex CLI execution mock - .boundary( - "execute_codex", - "stdout", - Value::Str(mock_codex_review_output()), - ) - .boundary("execute_codex", "stderr", Value::Str("".into())) - .boundary("execute_codex", "exit_code", Value::Int(0)) - - // Git diff mock - .boundary( - "git_diff", - "stdout", - Value::Str(mock_git_diff()), - ) - - // Checkpoint mocks - .boundary("check_checkpoint", "exists", Value::Bool(false)) - .boundary("save_checkpoint", "success", Value::Bool(true)) - - // Input expectations - .expects_input("target", InputConstraint::NonEmpty) - .expects_input("config", InputConstraint::NonEmpty) -} - -fn mock_codex_review_output() -> String { - r#"{ - "findings": [ - { - "file": "src/lib.rs", - "line": 42, - "severity": "warning", - "category": "bug", - "message": "Potential panic: unwrap() on Result that may be Err", - "suggestion": "Use `?` operator or handle the error explicitly" - } - ], - "summary": "Found 1 issue. Overall code quality is good.", - "complete": true - }"#.into() +/// Build a complete implement-and-review workflow +pub fn build_implement_and_review( + task: ImplementationTask, + rubric: Rubric, + config: WorkflowConfig, +) -> Result, BuilderError> { + let mut builder = DagBuilder::new("implement-and-review"); + + // Phase 1: Implementation + let impl_phase = ImplementationPhaseBuilder::new() + .with_task(task) + .with_durability(config.storage_path.clone()) + .build()?; + + let impl_node = builder.add_subdag("implement", impl_phase)?; + + // Phase 2: Review each artifact + let review_phase = ReviewPhaseBuilder::new() + .with_rubric(rubric) + .build()?; + + let review_loop = LoopBuilder::new("review-artifacts") + .with_body(review_phase) + .build()?; + + let review_node = builder.add_subdag_after("review", review_loop, &impl_node)?; + builder.connect(impl_node.output("artifacts"), review_node.input("items"))?; + + // Phase 3: Check verdict and potentially loop back + let check_node = builder.add_node_after( + "check-verdict", + WorkflowOps::CheckAllPassed, + &review_node, + )?; + builder.connect(review_node.output("results"), check_node.input("results"))?; + + // Set boundaries + builder.mark_entrypoint("task", impl_node.input("task"))?; + builder.mark_boundary("result", check_node.output("final_result"))?; + + builder.build() } ``` @@ -529,85 +893,70 @@ fn mock_codex_review_output() -> String { ## Implementation Tasks -### Phase 1: Core Infrastructure +### Phase 1: Core Types & Operations -- [ ] Define `CodexOps` enum in `lib/codex/src/ops.rs` -- [ ] Define state types (`CodexConfig`, `CodexSessionState`, `ReviewFinding`) -- [ ] Implement `Executable` for `CodexOps` -- [ ] Add `CODEX` to `CliToolDef` registry -- [ ] Create `CheckpointOps` for durable state +- [ ] Define `ImplementationSession` and related types in `lib/codex/src/types.rs` +- [ ] Define `Artifact`, `Rubric`, `ReviewResult` in `lib/review/src/types.rs` +- [ ] Implement `ImplementationOps` with `Executable` trait +- [ ] Implement `ReviewOps` with `Executable` trait -### Phase 2: DAG Patterns +### Phase 2: Durability Infrastructure -- [ ] Build `GatherContext` SubDag (git diff, file read routing) -- [ ] Build `ReviewLoop` with `WhileBuilder` and loop-carried state -- [ ] Build `CheckpointUpsert` helper for idempotent steps -- [ ] Wire up `CodeReviewPipeline` top-level DAG -- [ ] Add mock specs for all boundaries +- [ ] Implement step-level checkpoint storage +- [ ] Implement `CheckStepDone` / `RecordStep` operations +- [ ] Add content-addressed step ID computation +- [ ] Test crash recovery with simulated failures -### Phase 3: CLI Integration +### Phase 3: ReviewPhase SubDag -- [ ] Add `review` subcommand to gunbc CLI -- [ ] Implement config file loading (`~/.gunbc/config/codex.toml`) -- [ ] Add `--resume` flag for session recovery -- [ ] Add output format options (text, json, markdown) +- [ ] Build `ReviewPhaseBuilder` +- [ ] Implement predefined rubrics (code, design, test) +- [ ] Add multi-source review with `MergeResults` +- [ ] Generate mock specs for testing -### Phase 4: Testing & Polish +### Phase 4: ImplementationPhase SubDag -- [ ] Generate tests via testgen for pipeline DAG -- [ ] Add integration tests with mocked Codex responses -- [ ] Add real Codex integration test (requires API key) -- [ ] Documentation and examples +- [ ] Build `ImplementationPhaseBuilder` +- [ ] Integrate Codex CLI invocation +- [ ] Implement conversation state tracking +- [ ] Add artifact extraction from Codex responses -### Future: GitHub Actions Integration +### Phase 5: Composition & CLI -- [ ] `GatherContext` variant that fetches PR diff via GitHub API -- [ ] Output formatting as PR comment -- [ ] GitHub Actions workflow template -- [ ] Rate limiting and error handling for API +- [ ] Build `ImplementAndReviewBuilder` +- [ ] Add `gunbc implement` command +- [ ] Add `gunbc review` command +- [ ] Add `--resume` support for interrupted sessions --- ## Open Questions -1. **Multi-file chunking**: Large diffs may exceed context window. Strategy? - - Option A: Chunk by file, review each, merge findings - - Option B: Summarize first, then deep-dive on flagged areas - - Option C: Use file-level priority (changed lines count) +1. **Codex session files**: Does Codex CLI have its own session persistence? If so, how do we coordinate? -2. **Response parsing robustness**: Codex output may vary. How strict? - - Option A: Require JSON output format, fail on parse error - - Option B: LLM-assisted parsing (use another call to structure output) - - Option C: Regex/heuristic extraction with fallback +2. **Multi-file implementation**: When Codex generates changes across many files, should we review: + - All files together (single review)? + - Each file separately (parallel reviews)? + - Changed "modules" (heuristic grouping)? -3. **Session scope**: What defines a "session" for checkpointing? - - Option A: Per-invocation (new session each `gunbc review`) - - Option B: Per-target (same file = same session, resume by default) - - Option C: Explicit (`--session-id` flag) +3. **Feedback loop**: When review finds issues, how do we feed findings back to Codex? + - Option A: Append findings to conversation, ask for fixes + - Option B: New session with findings as context + - Option C: Human-in-the-loop decides -4. **Concurrent reviews**: Multiple reviews in parallel? - - Current: `AccessMode::Exclusive` prevents concurrent Codex - - Future: Could allow with separate session IDs +4. **Automated vs LLM review**: When should we use Clippy/lint vs LLM? + - Option A: Always run both in parallel, merge + - Option B: Clippy first, LLM for things Clippy can't catch + - Option C: Configurable per-rubric --- ## Related Patterns -- **Upsert**: `core/ir/src/patterns/upsert.rs` — idempotent create/update +- **Upsert**: `core/ir/src/patterns/upsert.rs` — idempotent operations - **While/Retry**: `core/ir/src/patterns/repeat.rs` — iteration with state +- **Loop**: `core/ir/src/patterns/loop_pattern.rs` — per-item processing +- **Branch**: `core/ir/src/patterns/branch.rs` — conditional paths - **CLI Tools**: `core/ir/src/transport/cli.rs` — CliToolDef, CliToolOp - **LLM Ops**: `lib/llm-ops/src/` — existing chat completion patterns - ---- - -## Notes - -- The durability model is intentionally **not** a full checkpoint/restore system. - Instead, it leverages DAG structure: each step is idempotent, and resumption - is "re-run from start, skip completed steps." - -- Codex CLI is treated as a transport boundary, not a special case. This means - all existing testing infrastructure (DryRun, mocks, testgen) works automatically. - -- Loop-carried state in `WhileBuilder` is the key to multi-turn conversations. - The `CodexSessionState` accumulates messages and findings across turns. +- **Git Ops**: `lib/git-ops/src/` — diff, branch operations From 6f27ad09fa600e669a7d6f804d4ef5420a93cedf Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 1 Feb 2026 20:02:26 +0000 Subject: [PATCH 03/15] Add Query/Command transport classification aligned with AGENT.md and SPEC.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reconciles new transport distinction with existing design principles: **Query vs Command Transport**: - Query: Read-like, no world mutation (LLM calls, file reads, git diff) - Allowed inside DAGs (reasoning zone) - Command: Write-like, mutates state (file writes, git commit, patches) - Only at DAG boundaries (action zone) **Scope Purity**: - Aligns with SPEC.md §2.7 "No side channels" - SubDags are hermetic — internals can't reach outside - Parent only sees entrypoints/boundaries, not internal transport **Applied to phases**: - ReviewPhase: Query only (LLM observes, outputs findings as data) - ImplementationPhase: Query for Codex, Command for applying changes - Reasoning → Acting pattern: decide first, act at boundaries References AGENT.md §4 "Nodes are Pure, Boundaries are Structural" https://claude.ai/code/session_01RZryjdBirzK2p6KVyGttju --- TODO/llm-code-review-pipeline.md | 129 ++++++++++++++++++++++++++++++- 1 file changed, 127 insertions(+), 2 deletions(-) diff --git a/TODO/llm-code-review-pipeline.md b/TODO/llm-code-review-pipeline.md index aacbcbfc717..110ba9d7fa2 100644 --- a/TODO/llm-code-review-pipeline.md +++ b/TODO/llm-code-review-pipeline.md @@ -13,6 +13,93 @@ These phases compose into larger workflows (Requirements → Design → Implemen --- +## Design Principles: Transport Classification & Scope Purity + +This section reconciles with AGENT.md ("Nodes are Pure, Boundaries are Structural") and SPEC.md (§2.7 "No side channels", §6 "Node Contract"). + +### Transport Classification: Query vs Command + +Not all I/O is equal. We distinguish: + +| Class | Nature | Where Allowed | Examples | +|-------|--------|---------------|----------| +| **Query** | Read-like, no world mutation | Inside DAGs | LLM calls, file reads, git diff, HTTP GET | +| **Command** | Write-like, mutates state | DAG boundaries only | File writes, git commit, apply patch, HTTP POST with side effects | + +**Rationale**: A Query is "observation" — it doesn't change the world, so the DAG remains **reasoning-focused**. A Command is "action" — it mutates state and should happen only after the DAG has decided what to do. + +``` +┌─────────────────────────────────────────────────────────────────────┐ +│ DAG Execution Model │ +│ │ +│ ┌─────────────────────────────────────────────────────────────┐ │ +│ │ Reasoning Zone (Queries OK) │ │ +│ │ │ │ +│ │ Pure nodes + Query transport (LLM, reads) │ │ +│ │ Produces: Decision / Intent / Plan │ │ +│ └─────────────────────────────────────────────────────────────┘ │ +│ │ │ +│ ▼ │ +│ ┌─────────────────────────────────────────────────────────────┐ │ +│ │ Action Zone (Commands) │ │ +│ │ │ │ +│ │ File writes, commits, patches applied │ │ +│ │ Executes the intent produced above │ │ +│ └─────────────────────────────────────────────────────────────┘ │ +└─────────────────────────────────────────────────────────────────────┘ +``` + +**Alignment with existing principles**: +- AGENT.md §4: "Nodes are Pure, Boundaries are Structural" — Queries are "less impure" than Commands +- AGENT.md §7: "No Escape Hatches" — Commands can't bypass DAG boundaries +- SPEC.md §6: "Port honesty" — Commands must flow through declared output ports + +### Scope Purity: SubDag Encapsulation + +A SubDag is **hermetic** — its internal nodes cannot reach outside: + +``` +SPEC.md §2.7: "An opaque node's observable effects on the graph are +limited to its declared output ports. Any communication between nodes +that bypasses edges is a contract violation." +``` + +Applied to SubDags: + +1. **Inputs**: SubDag internals see only what's passed through entrypoints +2. **Outputs**: Results leave only through declared boundaries +3. **No leakage**: Inner nodes cannot reference sibling nodes in parent DAG +4. **Resolution order**: SubDag must fully resolve before parent sees outputs + +``` +┌─────────────────────── Parent DAG ───────────────────────────┐ +│ │ +│ ┌─────────┐ ┌─────────────────────────────┐ │ +│ │ NodeA │────────▶│ SubDag (Review) │ │ +│ └─────────┘ in │ │ out │ +│ ──────▶│ [inner nodes isolated] │───────▶│ +│ ┌─────────┐ │ │ │ +│ │ NodeB │ ✗ │ Cannot see NodeA or NodeB │ │ +│ └─────────┘ ──────▶│ directly, only via inputs │ │ +│ └─────────────────────────────┘ │ +└───────────────────────────────────────────────────────────────┘ +``` + +**Implication for ReviewPhase**: The parent doesn't know ReviewPhase uses an LLM internally. It only sees: `(artifact, rubric) → (findings, verdict)`. + +### How This Applies to Our Phases + +| Phase | Transport Type | Reasoning | +|-------|----------------|-----------| +| **ReviewPhase** | Query only | LLM calls observe code, don't mutate it. Output is data (findings), not action. | +| **ImplementationPhase** | Query internally, Command at boundary | Codex queries produce artifacts. Applying artifacts is a Command. | +| **CommitPhase** | Command | Git commit mutates the repository. | +| **TestPhase** | Query | Running tests observes behavior, doesn't mutate code. | + +**The pattern**: Phases that "decide" use Queries. Phases that "act" use Commands. Most DAGs should be reasoning-heavy, with Commands concentrated at the edges. + +--- + ## Part 1: Fractal Workflow Architecture ### The Meta-Workflow @@ -83,6 +170,7 @@ For this iteration, we're building: 1. **Durability**: Survive crashes, resume seamlessly 2. **Statelessness**: All state flows through DAG edges (no hidden state) 3. **Idempotency**: Re-running from start skips completed work +4. **Query/Command separation**: Codex invocations are Queries; file mutations are Commands at the boundary ### The Challenge: Codex is Stateful @@ -91,6 +179,36 @@ Codex CLI maintains conversation state. Our challenge is to: - Persist it durably for crash recovery - Resume without duplicating work or losing context +### Query/Command Boundary in Implementation + +``` +┌─────────────────────────────────────────────────────────────────────┐ +│ ImplementationPhase │ +│ │ +│ ┌───────────────────────────────────────────────────────────────┐ │ +│ │ REASONING ZONE (Queries) │ │ +│ │ │ │ +│ │ GatherContext ──▶ CodexInvoke ──▶ ParseResponse ──▶ Decide │ │ +│ │ (file reads) (LLM query) (pure) (pure) │ │ +│ │ │ │ +│ │ Output: Artifacts (proposed changes, not yet applied) │ │ +│ └───────────────────────────────────────────────────────────────┘ │ +│ │ │ +│ ▼ │ +│ ┌───────────────────────────────────────────────────────────────┐ │ +│ │ ACTION ZONE (Commands) │ │ +│ │ │ │ +│ │ ApplyArtifacts ──▶ WriteFiles │ │ +│ │ (at DAG boundary, after reasoning completes) │ │ +│ └───────────────────────────────────────────────────────────────┘ │ +└─────────────────────────────────────────────────────────────────────┘ +``` + +The key insight: **Codex invocation is a Query** (asking "what should I do?"). **Applying the changes is a Command** (doing it). This separation means we can: +- Test the reasoning without mutating files +- Review proposed changes before applying +- Roll back decisions without undoing file mutations + ### Session State Model ```rust @@ -893,12 +1011,19 @@ pub fn build_implement_and_review( ## Implementation Tasks +### Phase 0: Core Architecture (if generalizing) + +- [ ] Consider adding Query/Command transport classification to `core/ir/src/transport/` +- [ ] Consider updating AGENT.md with Query/Command principle +- [ ] Consider updating SPEC.md §6 (Node Contract) with transport classification + ### Phase 1: Core Types & Operations - [ ] Define `ImplementationSession` and related types in `lib/codex/src/types.rs` - [ ] Define `Artifact`, `Rubric`, `ReviewResult` in `lib/review/src/types.rs` -- [ ] Implement `ImplementationOps` with `Executable` trait -- [ ] Implement `ReviewOps` with `Executable` trait +- [ ] Implement `ImplementationOps` with `Executable` trait (Query operations) +- [ ] Implement `ReviewOps` with `Executable` trait (Query operations) +- [ ] Define `ApplyOps` for Command operations (file writes, commits) ### Phase 2: Durability Infrastructure From 173dff23699902e21ba0e11408b4639c9d9764a5 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 1 Feb 2026 20:19:30 +0000 Subject: [PATCH 04/15] Address review feedback: structural transport, terminology, V0 scope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Based on detailed review feedback, major updates: **Boundary terminology** (fixes conflation): - Transport boundary = ExecuteQuery/Journal/Command nodes (I/O happens here) - Workflow boundary = unconnected output ports (DAG interface) **Transport classification now structural** (not rhetorical): - ExecuteQuery: read external state (LLM, file reads) - ExecuteJournal: write tool-owned state (checkpoints, cache) - ExecuteCommand: mutate user artifacts (file writes, commits) - Enables "this SubDag contains no ExecuteCommand" verification **Type fixes** (aligns with repo principles): - Removed Finding.confidence (kills confidence heuristics) - Added Finding.evidence for traceability - Renamed Severity → FindingLevel (Info|Nit|Concern|Blocker) - Avoids "Warning" which conflicts with "No Warnings" invariant **NextStep for fractal loop**: - ReviewResult now includes NextStep - Enables invariant loop: state → Review → NextStep → Branch → Implement/Done **V0 scope defined** (minimal useful pipeline): - Review current branch diff → JSON report - Reuses existing: GitOps, LlmOps, CargoOp - Explicitly embeds lib/llm-ops subdag (don't re-implement) - Does NOT include durability, Codex, apply commands **V1 scope**: Codex follow-up loop with patch artifacts https://claude.ai/code/session_01RZryjdBirzK2p6KVyGttju --- TODO/llm-code-review-pipeline.md | 268 ++++++++++++++++++++----------- 1 file changed, 173 insertions(+), 95 deletions(-) diff --git a/TODO/llm-code-review-pipeline.md b/TODO/llm-code-review-pipeline.md index 110ba9d7fa2..ad8af7a28d5 100644 --- a/TODO/llm-code-review-pipeline.md +++ b/TODO/llm-code-review-pipeline.md @@ -13,46 +13,70 @@ These phases compose into larger workflows (Requirements → Design → Implemen --- -## Design Principles: Transport Classification & Scope Purity +## Design Principles: Boundary Terminology & Transport Classification This section reconciles with AGENT.md ("Nodes are Pure, Boundaries are Structural") and SPEC.md (§2.7 "No side channels", §6 "Node Contract"). -### Transport Classification: Query vs Command +### Boundary Terminology (Two Distinct Concepts) -Not all I/O is equal. We distinguish: +gunbc has **two kinds of boundaries** that must not be conflated: -| Class | Nature | Where Allowed | Examples | -|-------|--------|---------------|----------| -| **Query** | Read-like, no world mutation | Inside DAGs | LLM calls, file reads, git diff, HTTP GET | -| **Command** | Write-like, mutates state | DAG boundaries only | File writes, git commit, apply patch, HTTP POST with side effects | - -**Rationale**: A Query is "observation" — it doesn't change the world, so the DAG remains **reasoning-focused**. A Command is "action" — it mutates state and should happen only after the DAG has decided what to do. +| Term | Definition | How Detected | Purpose | +|------|------------|--------------|---------| +| **Transport Boundary** | Node that executes `TransportOps::Execute*` | By op type | Where I/O happens; DryRun intercepts here | +| **Workflow Boundary** | Unconnected output port | By graph structure | DAG interface; signature for composition | ``` ┌─────────────────────────────────────────────────────────────────────┐ -│ DAG Execution Model │ +│ Example DAG │ │ │ -│ ┌─────────────────────────────────────────────────────────────┐ │ -│ │ Reasoning Zone (Queries OK) │ │ -│ │ │ │ -│ │ Pure nodes + Query transport (LLM, reads) │ │ -│ │ Produces: Decision / Intent / Plan │ │ -│ └─────────────────────────────────────────────────────────────┘ │ -│ │ │ -│ ▼ │ -│ ┌─────────────────────────────────────────────────────────────┐ │ -│ │ Action Zone (Commands) │ │ -│ │ │ │ -│ │ File writes, commits, patches applied │ │ -│ │ Executes the intent produced above │ │ -│ └─────────────────────────────────────────────────────────────┘ │ +│ ┌─────────────┐ ┌─────────────┐ ┌─────────────┐ │ +│ │ Prepare │────▶│ Execute │────▶│ Parse │ │ +│ │ (pure) │ │ (TRANSPORT │ │ (pure) │ │ +│ └─────────────┘ │ BOUNDARY) │ └──────┬──────┘ │ +│ └─────────────┘ │ │ +│ ▼ │ +│ ┌───────────┐ │ +│ │ output │ (WORKFLOW │ +│ │ (unconnected) BOUNDARY)│ +│ └───────────┘ │ └─────────────────────────────────────────────────────────────────────┘ ``` -**Alignment with existing principles**: -- AGENT.md §4: "Nodes are Pure, Boundaries are Structural" — Queries are "less impure" than Commands -- AGENT.md §7: "No Escape Hatches" — Commands can't bypass DAG boundaries -- SPEC.md §6: "Port honesty" — Commands must flow through declared output ports +### Transport Classification: Query / Journal / Command (Structural) + +Transport is classified by **typed execute ops**, not flags. This makes enforcement structural. + +| Class | Execute Op | Scope | Examples | +|-------|------------|-------|----------| +| **Query** | `TransportOps::ExecuteQuery` | Read external state | LLM calls, file reads, git diff, HTTP GET | +| **Journal** | `TransportOps::ExecuteJournal` | Write tool-owned cache/state | Checkpoints, session state, response cache | +| **Command** | `TransportOps::ExecuteCommand` | Mutate user artifacts / external systems | File writes, git commit, apply patch | + +**Why three categories?** +- `cargo test` writes to `target/` but we consider it "query-like" — it's tool-owned state +- Durability checkpoints write to `~/.gunbc/` — also tool-owned, not user artifacts +- Only Commands mutate things the user controls + +**Structural enforcement** (leverages existing `AccessMode` precedent): + +```rust +/// Transport execute operations — typed for structural enforcement +pub enum TransportOps { + /// Read-only access to external state + ExecuteQuery, + + /// Write to tool-owned cache/journal (durability, caching) + /// Does NOT mutate user-controlled artifacts + ExecuteJournal, + + /// Mutate user-controlled artifacts or external systems + /// Only allowed at workflow boundaries + ExecuteCommand, +} +``` + +**The rule**: A SubDag can be verified to contain "no `ExecuteCommand` nodes" — that's what makes ReviewPhase structurally query-only. ### Scope Purity: SubDag Encapsulation @@ -67,7 +91,7 @@ that bypasses edges is a contract violation." Applied to SubDags: 1. **Inputs**: SubDag internals see only what's passed through entrypoints -2. **Outputs**: Results leave only through declared boundaries +2. **Outputs**: Results leave only through declared workflow boundaries 3. **No leakage**: Inner nodes cannot reference sibling nodes in parent DAG 4. **Resolution order**: SubDag must fully resolve before parent sees outputs @@ -85,18 +109,19 @@ Applied to SubDags: └───────────────────────────────────────────────────────────────┘ ``` -**Implication for ReviewPhase**: The parent doesn't know ReviewPhase uses an LLM internally. It only sees: `(artifact, rubric) → (findings, verdict)`. +**Implication for ReviewPhase**: The parent doesn't know ReviewPhase uses an LLM internally. It only sees: `(artifact, rubric) → (findings, verdict, next_step)`. ### How This Applies to Our Phases -| Phase | Transport Type | Reasoning | -|-------|----------------|-----------| +| Phase | Transport Types Allowed | Reasoning | +|-------|-------------------------|-----------| | **ReviewPhase** | Query only | LLM calls observe code, don't mutate it. Output is data (findings), not action. | -| **ImplementationPhase** | Query internally, Command at boundary | Codex queries produce artifacts. Applying artifacts is a Command. | +| **ImplementationPhase** | Query + Journal | Codex queries produce artifacts. Checkpoints use Journal. Applying artifacts happens outside. | +| **TestPhase** | Query + Journal | Running tests observes behavior. Test caching uses Journal. | | **CommitPhase** | Command | Git commit mutates the repository. | -| **TestPhase** | Query | Running tests observes behavior, doesn't mutate code. | +| **ApplyPhase** | Command | File writes, patch application. | -**The pattern**: Phases that "decide" use Queries. Phases that "act" use Commands. Most DAGs should be reasoning-heavy, with Commands concentrated at the edges. +**The pattern**: Phases that "decide" use Query (+ Journal for durability). Phases that "act" use Command. Commands are concentrated at workflow boundaries. --- @@ -590,7 +615,7 @@ pub struct Rubric { /// What aspects to evaluate pub criteria: Vec, - /// Severity thresholds + /// Level thresholds for auto-fail pub fail_on: FailureCondition, /// Domain-specific instructions @@ -608,7 +633,7 @@ pub struct Criterion { #[derive(Debug, Clone, Serialize, Deserialize)] pub enum FailureCondition { /// Fail if any finding of this severity or higher - OnSeverity(Severity), + OnLevel(FindingLevel), /// Fail if score below threshold OnScore(f32), /// Never auto-fail, always advisory @@ -621,19 +646,30 @@ pub struct ReviewResult { pub findings: Vec, pub verdict: Verdict, pub summary: String, - pub score: Option, // 0.0-1.0 if scored pub reviewer: ReviewerType, + pub next_step: NextStep, // What to do next (fractal loop input) +} + +/// Canonical "what to do next" — enables fractal loop shape +/// Every review produces this, making the loop invariant: +/// state → ReviewPhase → NextStep → Branch(should_iterate) → (ImplementPhase or Done) +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct NextStep { + pub should_iterate: bool, + pub suggested_task: Option, // Prompt for Codex follow-up + pub blocking_findings: Vec, // Finding IDs that block progress + pub stopping_reason: Option, // Why we stopped (if !should_iterate) } #[derive(Debug, Clone, Serialize, Deserialize)] pub struct Finding { pub id: String, - pub severity: Severity, + pub level: FindingLevel, pub category: String, // Maps to Criterion.name pub location: Option, pub message: String, pub suggestion: Option, - pub confidence: f32, // 0.0-1.0 + pub evidence: Vec, // What led to this finding (line refs, patterns matched) } #[derive(Debug, Clone, Serialize, Deserialize)] @@ -652,12 +688,12 @@ pub enum Verdict { } #[derive(Debug, Clone, Serialize, Deserialize)] -pub enum Severity { - Info, // Informational only - Suggestion, // Nice to have - Warning, // Should address - Error, // Must fix - Critical, // Blocking +/// Finding severity levels (avoids "Warning" to align with "No Warnings" principle) +pub enum FindingLevel { + Info, // Informational, no action needed + Nit, // Minor style/preference issue + Concern, // Should address, but not blocking + Blocker, // Must fix before proceeding } #[derive(Debug, Clone, Serialize, Deserialize)] @@ -713,7 +749,7 @@ impl Rubric { ], }, ], - fail_on: FailureCondition::OnSeverity(Severity::Error), + fail_on: FailureCondition::OnLevel(FindingLevel::Blocker), domain_prompt: None, } } @@ -775,7 +811,7 @@ impl Rubric { examples: vec![], }, ], - fail_on: FailureCondition::OnSeverity(Severity::Warning), + fail_on: FailureCondition::OnLevel(FindingLevel::Concern), domain_prompt: None, } } @@ -837,6 +873,8 @@ pub enum ReviewOps { ┌─────────────────────────────────────────────────────────────────────────────┐ │ ReviewPhase SubDag │ │ │ +│ NOTE: Reuses lib/llm-ops chat subdag — don't re-implement the LLM chain! │ +│ │ │ Entrypoints: │ │ ┌──────────┐ ┌──────────┐ ┌──────────┐ ┌──────────┐ │ │ │ artifact │ │ rubric │ │ context │ │ config │ │ @@ -851,22 +889,14 @@ pub enum ReviewOps { │ └────────────┬────────────┘ │ │ │ │ │ ▼ │ -│ ┌─────────────────────────┐ │ -│ │ PrepareChatRequest │ ◀── PURE: build transport req │ -│ │ (LlmOps) │ │ -│ └────────────┬────────────┘ │ -│ │ │ -│ ▼ │ -│ ┌─────────────────────────┐ │ -│ │ Execute │ ◀── I/O BOUNDARY │ -│ │ (TransportOps) │ │ -│ └────────────┬────────────┘ │ -│ │ │ -│ ▼ │ -│ ┌─────────────────────────┐ │ -│ │ ParseChatResponse │ ◀── PURE: extract content │ -│ │ (LlmOps) │ │ -│ └────────────┬────────────┘ │ +│ ┌─────────────────────────────────────────────────┐ │ +│ │ EMBED: lib/llm-ops chat_completion subdag │ │ +│ │ ┌─────────────────────────────────────────┐ │ │ +│ │ │ PrepareChatRequest → ExecuteQuery → │ │ │ +│ │ │ ParseChatResponse │ │ │ +│ │ └─────────────────────────────────────────┘ │ │ +│ │ (reuse existing graph, don't redefine) │ │ +│ └────────────┬────────────────────────────────────┘ │ │ │ │ │ ▼ │ │ ┌─────────────────────────┐ │ @@ -876,14 +906,14 @@ pub enum ReviewOps { │ │ │ │ ▼ │ │ ┌─────────────────────────┐ │ -│ │ DetermineVerdict │ ◀── PURE: pass/fail decision │ +│ │ DetermineVerdict │ ◀── PURE: verdict + next_step │ │ │ (ReviewOps) │ │ │ └────────────┬────────────┘ │ │ │ │ -│ Boundaries: ▼ │ -│ ┌──────────┐ ┌──────────┐ ┌──────────┐ │ -│ │ findings │ │ verdict │ │ summary │ │ -│ └──────────┘ └──────────┘ └──────────┘ │ +│ Workflow Boundaries: ▼ │ +│ ┌──────────┐ ┌──────────┐ ┌──────────┐ ┌───────────┐ │ +│ │ findings │ │ verdict │ │ summary │ │ next_step │ │ +│ └──────────┘ └──────────┘ └──────────┘ └───────────┘ │ └─────────────────────────────────────────────────────────────────────────────┘ ``` @@ -1017,41 +1047,89 @@ pub fn build_implement_and_review( - [ ] Consider updating AGENT.md with Query/Command principle - [ ] Consider updating SPEC.md §6 (Node Contract) with transport classification -### Phase 1: Core Types & Operations +### V0: Minimal Useful Pipeline (First Target) -- [ ] Define `ImplementationSession` and related types in `lib/codex/src/types.rs` -- [ ] Define `Artifact`, `Rubric`, `ReviewResult` in `lib/review/src/types.rs` -- [ ] Implement `ImplementationOps` with `Executable` trait (Query operations) -- [ ] Implement `ReviewOps` with `Executable` trait (Query operations) -- [ ] Define `ApplyOps` for Command operations (file writes, commits) +**Goal**: Review current branch diff + optional cargo checks → JSON report -### Phase 2: Durability Infrastructure +**What already exists** (reuse, don't rebuild): +- ✅ `GitOps::PrepareDiff` / `ParseDiff` — get diff context +- ✅ `LlmOps::PrepareChatRequest` / `ParseChatResponse` — LLM invocation +- ✅ `CargoOp::check` / `CargoOp::test` — cargo tooling +- ✅ `TransportOps::Execute` — I/O boundary +- ✅ Pattern builders: `UpsertBuilder`, `LoopBuilder`, `BranchBuilder` -- [ ] Implement step-level checkpoint storage -- [ ] Implement `CheckStepDone` / `RecordStep` operations -- [ ] Add content-addressed step ID computation -- [ ] Test crash recovery with simulated failures +**V0 flow**: +``` +1. GitOps::PrepareDiff → ExecuteQuery → GitOps::ParseDiff +2. (Optional) CargoOp::check/test/clippy +3. ReviewOps::PrepareReviewPrompt(diff + tool_outputs + rubric) +4. [lib/llm-ops chat subdag] ← reuse existing +5. ReviewOps::ParseReviewResponse → DetermineVerdict +6. Output: JSON ReviewResult with findings, verdict, next_step +``` + +**What to build for V0**: +- [ ] `lib/review/` crate with types (`Artifact`, `Rubric`, `Finding`, `ReviewResult`, `NextStep`) +- [ ] `ReviewOps::PrepareReviewPrompt` — assemble prompt from artifact + rubric +- [ ] `ReviewOps::ParseReviewResponse` — parse LLM response into findings +- [ ] `ReviewOps::DetermineVerdict` — compute verdict and next_step +- [ ] Simple CLI: `gunbc review --diff HEAD~1` → prints JSON + +**V0 explicitly does NOT include**: +- Durability / checkpointing +- Codex CLI integration +- Multi-turn conversations +- Apply/commit commands + +--- -### Phase 3: ReviewPhase SubDag +### V1: Codex Follow-up Loop -- [ ] Build `ReviewPhaseBuilder` -- [ ] Implement predefined rubrics (code, design, test) -- [ ] Add multi-source review with `MergeResults` -- [ ] Generate mock specs for testing +After V0 works: +- [ ] ReviewResult.next_step.suggested_task feeds into Codex +- [ ] ImplementationPhase runs Codex CLI (Query), outputs patch artifact +- [ ] ReviewPhase reviews the patch +- [ ] Iterate until verdict.pass or max iterations +- [ ] **Keep "apply patch" as explicit Command** (separate from reasoning loop) -### Phase 4: ImplementationPhase SubDag +--- + +### Full Implementation Phases + +#### Phase 1: Core Types (V0 blocker) + +- [ ] Create `lib/review/` crate +- [ ] Define types: `Artifact`, `Rubric`, `Finding`, `FindingLevel`, `ReviewResult`, `NextStep`, `Verdict` +- [ ] Implement `ReviewOps` with `Executable` trait +- [ ] Wire up `ReviewPhaseBuilder` using existing patterns + +#### Phase 2: Transport Classification (architectural) + +- [ ] Add `ExecuteQuery` / `ExecuteJournal` / `ExecuteCommand` variants to `TransportOps` +- [ ] Update executor to handle new variants +- [ ] Add SubDag validation: "contains no ExecuteCommand nodes" -- [ ] Build `ImplementationPhaseBuilder` -- [ ] Integrate Codex CLI invocation -- [ ] Implement conversation state tracking -- [ ] Add artifact extraction from Codex responses +#### Phase 3: ImplementationPhase (V1) + +- [ ] Create `lib/codex/` crate +- [ ] Define types: `ImplementationSession`, `StepRecord`, `ConversationState` +- [ ] Implement `ImplementationOps` +- [ ] Integrate Codex CLI invocation (Query transport) +- [ ] Add Journal transport for session state persistence + +#### Phase 4: Durability (V1+) + +- [ ] Implement `ExecuteJournal` for checkpoint storage +- [ ] Implement `CheckStepDone` / `RecordStep` operations +- [ ] Add content-addressed step ID computation +- [ ] Test crash recovery -### Phase 5: Composition & CLI +#### Phase 5: Composition & CLI -- [ ] Build `ImplementAndReviewBuilder` -- [ ] Add `gunbc implement` command -- [ ] Add `gunbc review` command -- [ ] Add `--resume` support for interrupted sessions +- [ ] Build `ImplementAndReviewBuilder` (composes V0 + V1) +- [ ] Add `gunbc review` command (V0) +- [ ] Add `gunbc implement` command (V1) +- [ ] Add `--resume` support --- From e44c377fe4834b705537d5c8308b25c90c8e978b Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 1 Feb 2026 20:33:38 +0000 Subject: [PATCH 05/15] Simplify ReviewPhase to pure reconciliation, add Review Cycle orchestration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Key changes: - ReviewPhase is now pure: (artifact, criteria) → findings - No verdict, severity levels, or next_step in ReviewPhase - Added Part 4: Review Cycle as orchestration layer - Three-stage pattern: Coherence → Quality → Wisdom - Updated all type references (Criteria, Check, ReviewOutput) - Updated V0/V1 scope to reflect simplified model https://claude.ai/code/session_01RZryjdBirzK2p6KVyGttju --- TODO/llm-code-review-pipeline.md | 502 +++++++++++++++---------------- 1 file changed, 235 insertions(+), 267 deletions(-) diff --git a/TODO/llm-code-review-pipeline.md b/TODO/llm-code-review-pipeline.md index ad8af7a28d5..6c33c3bf455 100644 --- a/TODO/llm-code-review-pipeline.md +++ b/TODO/llm-code-review-pipeline.md @@ -109,7 +109,7 @@ Applied to SubDags: └───────────────────────────────────────────────────────────────┘ ``` -**Implication for ReviewPhase**: The parent doesn't know ReviewPhase uses an LLM internally. It only sees: `(artifact, rubric) → (findings, verdict, next_step)`. +**Implication for ReviewPhase**: The parent doesn't know ReviewPhase uses an LLM internally. It only sees: `(artifact, criteria) → findings`. ### How This Applies to Our Phases @@ -527,56 +527,23 @@ impl Default for CodexConfig { --- -## Part 3: ReviewPhase (General-Purpose) +## Part 3: ReviewPhase (Pure Reconciliation) -### Design Goals - -1. **Reusability**: Same ReviewPhase works for code, designs, tests, docs -2. **Modularity**: ReviewPhase is a self-contained SubDag -3. **Composability**: Easily plugs into any workflow point +### Design Principle: Review = Reconciliation -### Abstraction: Artifact + Rubric → Findings +**ReviewPhase is maximally simple**: given an artifact and criteria, report where the artifact diverges from the criteria. That's it. -The key insight is that **all reviews share the same shape**: +- **No verdict** — that's orchestration +- **No severity levels** — either it diverges or it doesn't +- **No "what to do next"** — that's orchestration +- **No opinions** — just reconciliation ``` -┌─────────────────────────────────────────────────────────────────────────┐ -│ ReviewPhase (Generic) │ -│ │ -│ Inputs: │ -│ ┌────────────────┐ ┌────────────────┐ ┌────────────────────────┐ │ -│ │ artifact │ │ rubric │ │ context (optional) │ │ -│ │ (what) │ │ (criteria) │ │ (background info) │ │ -│ └───────┬────────┘ └───────┬────────┘ └───────────┬────────────┘ │ -│ │ │ │ │ -│ └───────────────────┴───────────────────────┘ │ -│ │ │ -│ ▼ │ -│ ┌─────────────────┐ │ -│ │ PrepareReview │ │ -│ │ (build prompt) │ │ -│ └────────┬────────┘ │ -│ │ │ -│ ▼ │ -│ ┌─────────────────┐ │ -│ │ ExecuteReview │ ◀── LLM / Human / Automated │ -│ │ (I/O boundary) │ │ -│ └────────┬────────┘ │ -│ │ │ -│ ▼ │ -│ ┌─────────────────┐ │ -│ │ ParseFindings │ │ -│ │ (extract) │ │ -│ └────────┬────────┘ │ -│ │ │ -│ Outputs: ▼ │ -│ ┌────────────────┐ ┌────────────────┐ ┌────────────────────────┐ │ -│ │ findings │ │ verdict │ │ suggestions │ │ -│ │ (issues) │ │ (pass/fail) │ │ (improvements) │ │ -│ └────────────────┘ └────────────────┘ └────────────────────────┘ │ -└─────────────────────────────────────────────────────────────────────────┘ +ReviewPhase: (artifact, criteria) → findings ``` +The reviewer doesn't decide what happens with findings. Different criteria produce different findings. The orchestration layer decides what to do. + ### Core Types ```rust @@ -593,83 +560,51 @@ pub enum Artifact { /// Design document Design { content: String, - format: DocFormat, // Markdown, PlainText, Structured + format: DocFormat, }, - /// Test results or test code + /// Test results TestOutput { stdout: String, stderr: String, exit_code: i32, - test_count: Option, }, - /// Generic text artifact + /// Generic content Text { content: String, - artifact_type: String, // e.g., "commit_message", "pr_description" + artifact_type: String, }, } -/// Review criteria +/// Criteria = what to check for #[derive(Debug, Clone, Serialize, Deserialize)] -pub struct Rubric { - /// What aspects to evaluate - pub criteria: Vec, - - /// Level thresholds for auto-fail - pub fail_on: FailureCondition, - - /// Domain-specific instructions - pub domain_prompt: Option, +pub struct Criteria { + pub name: String, // e.g., "coherence", "quality", "security" + pub description: String, // What this criteria is about + pub checks: Vec, // Specific things to verify } #[derive(Debug, Clone, Serialize, Deserialize)] -pub struct Criterion { - pub name: String, // e.g., "correctness", "security" - pub description: String, // What to look for - pub weight: f32, // Importance (0.0-1.0) - pub examples: Vec, // Example issues +pub struct Check { + pub id: String, // Unique identifier + pub question: String, // What to ask about the artifact + pub examples: Vec, // Example violations (optional) } +/// Review output = pure reconciliation #[derive(Debug, Clone, Serialize, Deserialize)] -pub enum FailureCondition { - /// Fail if any finding of this severity or higher - OnLevel(FindingLevel), - /// Fail if score below threshold - OnScore(f32), - /// Never auto-fail, always advisory - Advisory, -} - -/// Review output -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct ReviewResult { - pub findings: Vec, - pub verdict: Verdict, - pub summary: String, - pub reviewer: ReviewerType, - pub next_step: NextStep, // What to do next (fractal loop input) -} - -/// Canonical "what to do next" — enables fractal loop shape -/// Every review produces this, making the loop invariant: -/// state → ReviewPhase → NextStep → Branch(should_iterate) → (ImplementPhase or Done) -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct NextStep { - pub should_iterate: bool, - pub suggested_task: Option, // Prompt for Codex follow-up - pub blocking_findings: Vec, // Finding IDs that block progress - pub stopping_reason: Option, // Why we stopped (if !should_iterate) +pub struct ReviewOutput { + pub criteria_name: String, // Which criteria was applied + pub findings: Vec, // Where artifact diverges from criteria + pub summary: String, // Natural language summary } +/// Finding = divergence from criteria #[derive(Debug, Clone, Serialize, Deserialize)] pub struct Finding { - pub id: String, - pub level: FindingLevel, - pub category: String, // Maps to Criterion.name + pub check_id: String, // Which Check this relates to pub location: Option, - pub message: String, - pub suggestion: Option, - pub evidence: Vec, // What led to this finding (line refs, patterns matched) + pub observation: String, // What was found + pub suggested_fix: Option, } #[derive(Debug, Clone, Serialize, Deserialize)] @@ -677,157 +612,198 @@ pub struct Location { pub file: Option, pub line_start: Option, pub line_end: Option, - pub column: Option, } +``` -#[derive(Debug, Clone, Serialize, Deserialize)] -pub enum Verdict { - Pass, - Fail { blocking_findings: Vec }, // Finding IDs - NeedsWork { suggested_changes: u32 }, -} +**Note**: No `FindingLevel`, no `Verdict`, no `NextStep`. Those belong in orchestration. -#[derive(Debug, Clone, Serialize, Deserialize)] -/// Finding severity levels (avoids "Warning" to align with "No Warnings" principle) -pub enum FindingLevel { - Info, // Informational, no action needed - Nit, // Minor style/preference issue - Concern, // Should address, but not blocking - Blocker, // Must fix before proceeding -} +--- + +## Part 4: Review Cycle (Orchestration) + +### The Three-Phase Review Pattern + +Reviews happen in stages, each with different criteria. This is orchestration, not ReviewPhase's concern. -#[derive(Debug, Clone, Serialize, Deserialize)] -pub enum ReviewerType { - Llm { model: String }, - Automated { tool: String }, // e.g., "clippy", "eslint" - Human { reviewer: String }, -} +``` +┌─────────────────────────────────────────────────────────────────────────┐ +│ Review Cycle │ +│ │ +│ Stage 1: COHERENCE │ +│ "Does it make sense?" │ +│ ┌─────────────────────────────────────────────────────────────────┐ │ +│ │ • Is it internally consistent? │ │ +│ │ • Does the logic flow? │ │ +│ │ • Are there contradictions? │ │ +│ └─────────────────────────────────────────────────────────────────┘ │ +│ │ │ +│ findings → iterate or proceed │ +│ ▼ │ +│ Stage 2: QUALITY │ +│ "Is it well-crafted?" │ +│ ┌─────────────────────────────────────────────────────────────────┐ │ +│ │ • Is it clean and readable? │ │ +│ │ • Are there bugs or issues? │ │ +│ │ • Does it follow conventions? │ │ +│ └─────────────────────────────────────────────────────────────────┘ │ +│ │ │ +│ findings → iterate or proceed │ +│ ▼ │ +│ Stage 3: WISDOM (optional) │ +│ "Is it the right approach?" │ +│ ┌─────────────────────────────────────────────────────────────────┐ │ +│ │ • Is this the best way to solve the problem? │ │ +│ │ • Are there better patterns? │ │ +│ │ • Will this scale / maintain well? │ │ +│ └─────────────────────────────────────────────────────────────────┘ │ +│ │ │ +│ findings → iterate or done │ +└─────────────────────────────────────────────────────────────────────────┘ ``` -### Predefined Rubrics +### Predefined Criteria ```rust -impl Rubric { - /// Standard code review rubric - pub fn code_review() -> Self { +impl Criteria { + /// Stage 1: Does it make sense? + pub fn coherence() -> Self { Self { - criteria: vec![ - Criterion { - name: "correctness".into(), - description: "Logic errors, bugs, edge cases".into(), - weight: 1.0, - examples: vec![ - "Off-by-one error".into(), - "Null pointer dereference".into(), - ], - }, - Criterion { - name: "security".into(), - description: "Vulnerabilities, unsafe practices".into(), - weight: 1.0, - examples: vec![ - "SQL injection".into(), - "Hardcoded secrets".into(), - ], + name: "coherence".into(), + description: "Internal consistency and logical flow".into(), + checks: vec![ + Check { + id: "no_contradictions".into(), + question: "Are there any internal contradictions?".into(), + examples: vec![], }, - Criterion { - name: "performance".into(), - description: "Inefficiencies, scaling issues".into(), - weight: 0.7, - examples: vec![ - "N+1 query".into(), - "Unnecessary allocation in loop".into(), - ], + Check { + id: "logic_flow".into(), + question: "Does the logic flow make sense?".into(), + examples: vec![], }, - Criterion { - name: "maintainability".into(), - description: "Readability, structure, naming".into(), - weight: 0.5, - examples: vec![ - "Unclear variable name".into(), - "Function too long".into(), - ], + Check { + id: "completeness".into(), + question: "Is anything obviously missing?".into(), + examples: vec![], }, ], - fail_on: FailureCondition::OnLevel(FindingLevel::Blocker), - domain_prompt: None, } } - /// Design document review rubric - pub fn design_review() -> Self { + /// Stage 2: Is it well-crafted? + pub fn quality() -> Self { Self { - criteria: vec![ - Criterion { - name: "completeness".into(), - description: "All necessary sections present".into(), - weight: 1.0, - examples: vec![], + name: "quality".into(), + description: "Correctness, cleanliness, and craftsmanship".into(), + checks: vec![ + Check { + id: "correctness".into(), + question: "Are there bugs or logical errors?".into(), + examples: vec!["Off-by-one".into(), "Null dereference".into()], }, - Criterion { - name: "clarity".into(), - description: "Clear, unambiguous language".into(), - weight: 0.8, + Check { + id: "clarity".into(), + question: "Is the code/content clear and readable?".into(), examples: vec![], }, - Criterion { - name: "feasibility".into(), - description: "Technically achievable".into(), - weight: 1.0, - examples: vec![], - }, - Criterion { - name: "consistency".into(), - description: "No contradictions".into(), - weight: 0.9, + Check { + id: "conventions".into(), + question: "Does it follow established patterns?".into(), examples: vec![], }, ], - fail_on: FailureCondition::Advisory, - domain_prompt: None, } } - /// Test coverage/quality review - pub fn test_review() -> Self { + /// Stage 3: Is it the right approach? + pub fn wisdom() -> Self { Self { - criteria: vec![ - Criterion { - name: "coverage".into(), - description: "Key paths tested".into(), - weight: 1.0, + name: "wisdom".into(), + description: "Strategic fit and long-term thinking".into(), + checks: vec![ + Check { + id: "approach".into(), + question: "Is this the right approach to the problem?".into(), examples: vec![], }, - Criterion { - name: "edge_cases".into(), - description: "Boundary conditions handled".into(), - weight: 0.9, + Check { + id: "alternatives".into(), + question: "Are there simpler or better alternatives?".into(), examples: vec![], }, - Criterion { - name: "clarity".into(), - description: "Test intent is clear".into(), - weight: 0.6, + Check { + id: "maintainability".into(), + question: "Will this be easy to maintain and extend?".into(), examples: vec![], }, ], - fail_on: FailureCondition::OnLevel(FindingLevel::Concern), - domain_prompt: None, } } } ``` +### Orchestration Logic + +The orchestration layer (not ReviewPhase) handles: + +```rust +/// Orchestration decides what to do with findings +pub struct ReviewCycleConfig { + /// Which criteria to apply, in order + pub stages: Vec, + + /// How to decide whether to iterate + pub iterate_policy: IteratePolicy, + + /// Maximum iterations per stage + pub max_iterations: u32, +} + +pub enum IteratePolicy { + /// Iterate if any findings have suggested_fix + OnFixableFinding, + /// Never auto-iterate, always proceed + AlwaysProceed, + /// Custom logic + Custom(fn(&[Finding]) -> bool), +} + +/// What the orchestration layer produces +pub struct CycleResult { + /// All findings from all stages + pub all_findings: Vec<(String, Vec)>, // (criteria_name, findings) + + /// Final decision + pub outcome: CycleOutcome, +} + +pub enum CycleOutcome { + /// All stages passed (no findings, or findings were fixed) + Complete, + /// Stopped iterating (max iterations or no more fixes) + Stabilized { remaining_findings: usize }, + /// Needs human decision + NeedsHuman { reason: String }, +} +``` + +### Why This Separation Matters + +1. **ReviewPhase stays pure**: `(artifact, criteria) → findings` — nothing more +2. **Different review "types" = different criteria**: security review is just `Criteria::security()` +3. **Orchestration is explicit**: decisions about iterating, blocking, etc. are visible +4. **Composable**: can run single criteria or full cycle, orchestration's choice + ### ReviewOps ```rust #[derive(Debug, Clone)] pub enum ReviewOps { - /// Build review prompt from artifact + rubric + /// Build review prompt from artifact + criteria /// /// Inputs: /// - `artifact`: Artifact - /// - `rubric`: Rubric + /// - `criteria`: Criteria /// - `context`: Option /// /// Outputs: @@ -839,34 +815,25 @@ pub enum ReviewOps { /// /// Inputs: /// - `response`: String (LLM output) - /// - `rubric`: Rubric (for category mapping) + /// - `criteria`: Criteria (for check_id mapping) /// /// Outputs: - /// - `result`: ReviewResult + /// - `output`: ReviewOutput ParseReviewResponse, - /// Determine verdict from findings - /// - /// Inputs: - /// - `findings`: Vec - /// - `fail_condition`: FailureCondition - /// - /// Outputs: - /// - `verdict`: Verdict - /// - `blocking_count`: Int - DetermineVerdict, - /// Merge findings from multiple review sources /// /// Inputs: - /// - `results`: Vec + /// - `outputs`: Vec /// /// Outputs: - /// - `merged`: ReviewResult - MergeResults, + /// - `merged`: ReviewOutput + MergeOutputs, } ``` +**Note**: No `DetermineVerdict` in ReviewOps — that's orchestration (see Part 4). + ### ReviewPhase DAG ``` @@ -877,7 +844,7 @@ pub enum ReviewOps { │ │ │ Entrypoints: │ │ ┌──────────┐ ┌──────────┐ ┌──────────┐ ┌──────────┐ │ -│ │ artifact │ │ rubric │ │ context │ │ config │ │ +│ │ artifact │ │ criteria │ │ context │ │ config │ │ │ └────┬─────┘ └────┬─────┘ └────┬─────┘ └────┬─────┘ │ │ │ │ │ │ │ │ └────────────┴────────────┴────────────┘ │ @@ -904,16 +871,13 @@ pub enum ReviewOps { │ │ (ReviewOps) │ │ │ └────────────┬────────────┘ │ │ │ │ -│ ▼ │ -│ ┌─────────────────────────┐ │ -│ │ DetermineVerdict │ ◀── PURE: verdict + next_step │ -│ │ (ReviewOps) │ │ -│ └────────────┬────────────┘ │ -│ │ │ │ Workflow Boundaries: ▼ │ -│ ┌──────────┐ ┌──────────┐ ┌──────────┐ ┌───────────┐ │ -│ │ findings │ │ verdict │ │ summary │ │ next_step │ │ -│ └──────────┘ └──────────┘ └──────────┘ └───────────┘ │ +│ ┌───────────────┐ ┌──────────┐ │ +│ │ findings │ │ summary │ │ +│ │ Vec │ │ String │ │ +│ └───────────────┘ └──────────┘ │ +│ │ +│ NOTE: No verdict, no next_step — that's orchestration (Part 4) │ └─────────────────────────────────────────────────────────────────────────────┘ ``` @@ -926,7 +890,7 @@ For comprehensive review, run multiple reviewers in parallel: │ MultiReview (Parallel + Merge) │ │ │ │ ┌─────────────────────────────────────────────┐ │ -│ │ artifact + rubric │ │ +│ │ artifact + criteria │ │ │ └─────────┬───────────────┬──────────────┬────┘ │ │ │ │ │ │ │ ┌─────────▼─────┐ ┌───────▼───────┐ ┌────▼────────┐ │ @@ -937,23 +901,23 @@ For comprehensive review, run multiple reviewers in parallel: │ └─────────────────┴────────────────┘ │ │ │ │ │ ┌──────────▼──────────┐ │ -│ │ MergeResults │ │ +│ │ MergeOutputs │ │ │ │ (ReviewOps) │ │ │ └──────────┬──────────┘ │ │ │ │ │ ┌───────▼───────┐ │ -│ │ unified result│ │ +│ │ unified output│ │ │ └───────────────┘ │ └─────────────────────────────────────────────────────────────────────────────┘ ``` --- -## Part 4: Composition Example +## Part 5: Composition Example ### Implement-and-Review Workflow -Compose ImplementationPhase + ReviewPhase into a complete workflow: +Compose ImplementationPhase + Review Cycle (orchestration) into a complete workflow: ``` ┌─────────────────────────────────────────────────────────────────────────────┐ @@ -967,24 +931,27 @@ Compose ImplementationPhase + ReviewPhase into a complete workflow: │ │ artifacts: Vec │ │ ▼ │ │ ┌─────────────────────────────────────────────────────────────────────┐ │ -│ │ LoopBuilder │ │ -│ │ (review each artifact) │ │ +│ │ Review Cycle (Orchestration) │ │ +│ │ stages: [Coherence, Quality, Wisdom] │ │ │ │ │ │ +│ │ For each artifact, for each stage: │ │ │ │ ┌─────────────────────────────────────────────────────────────┐ │ │ │ │ │ ReviewPhase │ │ │ -│ │ │ rubric: Rubric::code_review() │ │ │ +│ │ │ criteria: Criteria::quality() │ │ │ +│ │ │ outputs: (findings, summary) │ │ │ │ │ └─────────────────────────────────────────────────────────────┘ │ │ +│ │ If findings → iterate (apply fix, re-review) or proceed │ │ │ └─────────────────────────────┬───────────────────────────────────────┘ │ │ │ │ -│ │ review_results: Vec │ +│ │ cycle_result: CycleResult │ │ ▼ │ │ ┌─────────────────────────────────────────────────────────────────────┐ │ │ │ BranchBuilder │ │ -│ │ condition: all_passed(review_results) │ │ +│ │ condition: cycle_result.outcome == Complete │ │ │ │ │ │ │ │ ┌─────────────────┐ ┌─────────────────┐ │ │ -│ │ │ True: Complete │ │ False: Iterate │ │ │ -│ │ │ → output result │ │ → back to Impl │ │ │ +│ │ │ True: Complete │ │ False: Stabilized│ │ │ +│ │ │ → apply changes │ │ → human decides │ │ │ │ │ └─────────────────┘ └─────────────────┘ │ │ │ └─────────────────────────────────────────────────────────────────────┘ │ └─────────────────────────────────────────────────────────────────────────────┘ @@ -996,38 +963,36 @@ Compose ImplementationPhase + ReviewPhase into a complete workflow: /// Build a complete implement-and-review workflow pub fn build_implement_and_review( task: ImplementationTask, - rubric: Rubric, - config: WorkflowConfig, + cycle_config: ReviewCycleConfig, + workflow_config: WorkflowConfig, ) -> Result, BuilderError> { let mut builder = DagBuilder::new("implement-and-review"); // Phase 1: Implementation let impl_phase = ImplementationPhaseBuilder::new() .with_task(task) - .with_durability(config.storage_path.clone()) + .with_durability(workflow_config.storage_path.clone()) .build()?; let impl_node = builder.add_subdag("implement", impl_phase)?; - // Phase 2: Review each artifact - let review_phase = ReviewPhaseBuilder::new() - .with_rubric(rubric) - .build()?; - - let review_loop = LoopBuilder::new("review-artifacts") - .with_body(review_phase) + // Phase 2: Review cycle (orchestration handles iteration) + let review_cycle = ReviewCycleBuilder::new() + .with_stages(cycle_config.stages) + .with_iterate_policy(cycle_config.iterate_policy) + .with_max_iterations(cycle_config.max_iterations) .build()?; - let review_node = builder.add_subdag_after("review", review_loop, &impl_node)?; - builder.connect(impl_node.output("artifacts"), review_node.input("items"))?; + let review_node = builder.add_subdag_after("review-cycle", review_cycle, &impl_node)?; + builder.connect(impl_node.output("artifacts"), review_node.input("artifacts"))?; - // Phase 3: Check verdict and potentially loop back + // Phase 3: Handle cycle outcome let check_node = builder.add_node_after( - "check-verdict", - WorkflowOps::CheckAllPassed, + "handle-outcome", + WorkflowOps::HandleCycleOutcome, &review_node, )?; - builder.connect(review_node.output("results"), check_node.input("results"))?; + builder.connect(review_node.output("cycle_result"), check_node.input("result"))?; // Set boundaries builder.mark_entrypoint("task", impl_node.input("task"))?; @@ -1062,19 +1027,20 @@ pub fn build_implement_and_review( ``` 1. GitOps::PrepareDiff → ExecuteQuery → GitOps::ParseDiff 2. (Optional) CargoOp::check/test/clippy -3. ReviewOps::PrepareReviewPrompt(diff + tool_outputs + rubric) +3. ReviewOps::PrepareReviewPrompt(diff + tool_outputs + criteria) 4. [lib/llm-ops chat subdag] ← reuse existing -5. ReviewOps::ParseReviewResponse → DetermineVerdict -6. Output: JSON ReviewResult with findings, verdict, next_step +5. ReviewOps::ParseReviewResponse +6. Output: JSON ReviewOutput with findings, summary ``` **What to build for V0**: -- [ ] `lib/review/` crate with types (`Artifact`, `Rubric`, `Finding`, `ReviewResult`, `NextStep`) -- [ ] `ReviewOps::PrepareReviewPrompt` — assemble prompt from artifact + rubric +- [ ] `lib/review/` crate with types (`Artifact`, `Criteria`, `Check`, `Finding`, `ReviewOutput`) +- [ ] `ReviewOps::PrepareReviewPrompt` — assemble prompt from artifact + criteria - [ ] `ReviewOps::ParseReviewResponse` — parse LLM response into findings -- [ ] `ReviewOps::DetermineVerdict` — compute verdict and next_step - [ ] Simple CLI: `gunbc review --diff HEAD~1` → prints JSON +**Note**: V0 does NOT include orchestration (Review Cycle) — just single-criteria review. + **V0 explicitly does NOT include**: - Durability / checkpointing - Codex CLI integration @@ -1083,13 +1049,14 @@ pub fn build_implement_and_review( --- -### V1: Codex Follow-up Loop +### V1: Review Cycle + Codex Integration After V0 works: -- [ ] ReviewResult.next_step.suggested_task feeds into Codex +- [ ] Add Review Cycle orchestration (Coherence → Quality → Wisdom stages) +- [ ] For findings with `suggested_fix`, feed back into Codex - [ ] ImplementationPhase runs Codex CLI (Query), outputs patch artifact -- [ ] ReviewPhase reviews the patch -- [ ] Iterate until verdict.pass or max iterations +- [ ] ReviewPhase reviews the patch with same criteria +- [ ] Iterate until no findings or max iterations - [ ] **Keep "apply patch" as explicit Command** (separate from reasoning loop) --- @@ -1099,7 +1066,8 @@ After V0 works: #### Phase 1: Core Types (V0 blocker) - [ ] Create `lib/review/` crate -- [ ] Define types: `Artifact`, `Rubric`, `Finding`, `FindingLevel`, `ReviewResult`, `NextStep`, `Verdict` +- [ ] Define types: `Artifact`, `Criteria`, `Check`, `Finding`, `Location`, `ReviewOutput` +- [ ] Define orchestration types: `ReviewCycleConfig`, `IteratePolicy`, `CycleResult`, `CycleOutcome` - [ ] Implement `ReviewOps` with `Executable` trait - [ ] Wire up `ReviewPhaseBuilder` using existing patterns From 1f1f42c2bf5e41b39ddcd043932cd1761522d5f9 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 1 Feb 2026 20:38:32 +0000 Subject: [PATCH 06/15] Remove hardcoded stage pattern from Review Cycle - Criteria are now fully configurable, not prescriptive - Removed Coherence/Quality/Wisdom stage definitions - Order is flexible (sequential usually makes sense, but not enforced) - Updated all references from `stages` to `criteria` https://claude.ai/code/session_01RZryjdBirzK2p6KVyGttju --- TODO/llm-code-review-pipeline.md | 152 ++++--------------------------- 1 file changed, 19 insertions(+), 133 deletions(-) diff --git a/TODO/llm-code-review-pipeline.md b/TODO/llm-code-review-pipeline.md index 6c33c3bf455..7eed65e0626 100644 --- a/TODO/llm-code-review-pipeline.md +++ b/TODO/llm-code-review-pipeline.md @@ -621,141 +621,27 @@ pub struct Location { ## Part 4: Review Cycle (Orchestration) -### The Three-Phase Review Pattern +### What Orchestration Does -Reviews happen in stages, each with different criteria. This is orchestration, not ReviewPhase's concern. +ReviewPhase is pure reconciliation. The **Review Cycle** is orchestration that: +- Decides which criteria to run (fully configurable) +- Decides whether to iterate on findings +- Decides when to stop -``` -┌─────────────────────────────────────────────────────────────────────────┐ -│ Review Cycle │ -│ │ -│ Stage 1: COHERENCE │ -│ "Does it make sense?" │ -│ ┌─────────────────────────────────────────────────────────────────┐ │ -│ │ • Is it internally consistent? │ │ -│ │ • Does the logic flow? │ │ -│ │ • Are there contradictions? │ │ -│ └─────────────────────────────────────────────────────────────────┘ │ -│ │ │ -│ findings → iterate or proceed │ -│ ▼ │ -│ Stage 2: QUALITY │ -│ "Is it well-crafted?" │ -│ ┌─────────────────────────────────────────────────────────────────┐ │ -│ │ • Is it clean and readable? │ │ -│ │ • Are there bugs or issues? │ │ -│ │ • Does it follow conventions? │ │ -│ └─────────────────────────────────────────────────────────────────┘ │ -│ │ │ -│ findings → iterate or proceed │ -│ ▼ │ -│ Stage 3: WISDOM (optional) │ -│ "Is it the right approach?" │ -│ ┌─────────────────────────────────────────────────────────────────┐ │ -│ │ • Is this the best way to solve the problem? │ │ -│ │ • Are there better patterns? │ │ -│ │ • Will this scale / maintain well? │ │ -│ └─────────────────────────────────────────────────────────────────┘ │ -│ │ │ -│ findings → iterate or done │ -└─────────────────────────────────────────────────────────────────────────┘ -``` - -### Predefined Criteria - -```rust -impl Criteria { - /// Stage 1: Does it make sense? - pub fn coherence() -> Self { - Self { - name: "coherence".into(), - description: "Internal consistency and logical flow".into(), - checks: vec![ - Check { - id: "no_contradictions".into(), - question: "Are there any internal contradictions?".into(), - examples: vec![], - }, - Check { - id: "logic_flow".into(), - question: "Does the logic flow make sense?".into(), - examples: vec![], - }, - Check { - id: "completeness".into(), - question: "Is anything obviously missing?".into(), - examples: vec![], - }, - ], - } - } - - /// Stage 2: Is it well-crafted? - pub fn quality() -> Self { - Self { - name: "quality".into(), - description: "Correctness, cleanliness, and craftsmanship".into(), - checks: vec![ - Check { - id: "correctness".into(), - question: "Are there bugs or logical errors?".into(), - examples: vec!["Off-by-one".into(), "Null dereference".into()], - }, - Check { - id: "clarity".into(), - question: "Is the code/content clear and readable?".into(), - examples: vec![], - }, - Check { - id: "conventions".into(), - question: "Does it follow established patterns?".into(), - examples: vec![], - }, - ], - } - } - - /// Stage 3: Is it the right approach? - pub fn wisdom() -> Self { - Self { - name: "wisdom".into(), - description: "Strategic fit and long-term thinking".into(), - checks: vec![ - Check { - id: "approach".into(), - question: "Is this the right approach to the problem?".into(), - examples: vec![], - }, - Check { - id: "alternatives".into(), - question: "Are there simpler or better alternatives?".into(), - examples: vec![], - }, - Check { - id: "maintainability".into(), - question: "Will this be easy to maintain and extend?".into(), - examples: vec![], - }, - ], - } - } -} -``` - -### Orchestration Logic +No hardcoded stages or order — criteria are user-defined. -The orchestration layer (not ReviewPhase) handles: +### Orchestration Types ```rust /// Orchestration decides what to do with findings pub struct ReviewCycleConfig { - /// Which criteria to apply, in order - pub stages: Vec, + /// Which criteria to apply (order is configurable, not mandated) + pub criteria: Vec, /// How to decide whether to iterate pub iterate_policy: IteratePolicy, - /// Maximum iterations per stage + /// Maximum iterations per criteria pub max_iterations: u32, } @@ -770,7 +656,7 @@ pub enum IteratePolicy { /// What the orchestration layer produces pub struct CycleResult { - /// All findings from all stages + /// All findings from all criteria runs pub all_findings: Vec<(String, Vec)>, // (criteria_name, findings) /// Final decision @@ -778,7 +664,7 @@ pub struct CycleResult { } pub enum CycleOutcome { - /// All stages passed (no findings, or findings were fixed) + /// All criteria passed (no findings, or findings were fixed) Complete, /// Stopped iterating (max iterations or no more fixes) Stabilized { remaining_findings: usize }, @@ -790,9 +676,9 @@ pub enum CycleOutcome { ### Why This Separation Matters 1. **ReviewPhase stays pure**: `(artifact, criteria) → findings` — nothing more -2. **Different review "types" = different criteria**: security review is just `Criteria::security()` +2. **Criteria are user-defined**: security, style, correctness — whatever you configure 3. **Orchestration is explicit**: decisions about iterating, blocking, etc. are visible -4. **Composable**: can run single criteria or full cycle, orchestration's choice +4. **Order is flexible**: sequential usually makes sense (hard to discuss design when basic correctness fails), but not enforced ### ReviewOps @@ -932,12 +818,12 @@ Compose ImplementationPhase + Review Cycle (orchestration) into a complete workf │ ▼ │ │ ┌─────────────────────────────────────────────────────────────────────┐ │ │ │ Review Cycle (Orchestration) │ │ -│ │ stages: [Coherence, Quality, Wisdom] │ │ +│ │ criteria: [configured by caller] │ │ │ │ │ │ -│ │ For each artifact, for each stage: │ │ +│ │ For each artifact, for each configured criteria: │ │ │ │ ┌─────────────────────────────────────────────────────────────┐ │ │ │ │ │ ReviewPhase │ │ │ -│ │ │ criteria: Criteria::quality() │ │ │ +│ │ │ criteria: │ │ │ │ │ │ outputs: (findings, summary) │ │ │ │ │ └─────────────────────────────────────────────────────────────┘ │ │ │ │ If findings → iterate (apply fix, re-review) or proceed │ │ @@ -978,7 +864,7 @@ pub fn build_implement_and_review( // Phase 2: Review cycle (orchestration handles iteration) let review_cycle = ReviewCycleBuilder::new() - .with_stages(cycle_config.stages) + .with_criteria(cycle_config.criteria) .with_iterate_policy(cycle_config.iterate_policy) .with_max_iterations(cycle_config.max_iterations) .build()?; @@ -1052,7 +938,7 @@ pub fn build_implement_and_review( ### V1: Review Cycle + Codex Integration After V0 works: -- [ ] Add Review Cycle orchestration (Coherence → Quality → Wisdom stages) +- [ ] Add Review Cycle orchestration (configurable criteria) - [ ] For findings with `suggested_fix`, feed back into Codex - [ ] ImplementationPhase runs Codex CLI (Query), outputs patch artifact - [ ] ReviewPhase reviews the patch with same criteria From a97723f017e57d654f0dcc559ef0cb873e9cdcf3 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 1 Feb 2026 20:41:04 +0000 Subject: [PATCH 07/15] Add Effect capabilities, RemediationPlan, CachePolicy, Codex native resume MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Incorporates feedback to make the design more operational: - Effect Capabilities: Fine-grained runtime enforcement (ReadRepo, WriteRepo, SpawnProcess, WriteTemp, WriteUserState, Network) underneath Query/Command - RemediationPlan: Structured output from ReviewPhase that makes the loop truly fractal - (artifact, criteria) → (findings, remediation_plan) - Finding identity: Hash-based id for deterministic merge/dedup - Location with DiffSide: Handle old vs new lines in diff reviews - CachePolicy: Explicit policy for nondeterministic LLM steps (Immutable, Refresh, RefreshIfOlderThan, RefreshIfModelChanged) - Codex native resume: Use codex_session_id + `codex resume ` instead of duplicating conversation state https://claude.ai/code/session_01RZryjdBirzK2p6KVyGttju --- TODO/llm-code-review-pipeline.md | 143 ++++++++++++++++++++++++++++--- 1 file changed, 129 insertions(+), 14 deletions(-) diff --git a/TODO/llm-code-review-pipeline.md b/TODO/llm-code-review-pipeline.md index 7eed65e0626..9d7b798ceb3 100644 --- a/TODO/llm-code-review-pipeline.md +++ b/TODO/llm-code-review-pipeline.md @@ -78,6 +78,31 @@ pub enum TransportOps { **The rule**: A SubDag can be verified to contain "no `ExecuteCommand` nodes" — that's what makes ReviewPhase structurally query-only. +### Effect Capabilities (Runtime Enforcement) + +Query/Journal/Command is the user-facing abstraction. Underneath, implement as a finer-grained **effect capability** system for runtime verification: + +```rust +enum Effect { + ReadRepo, // Read tracked files, git state + WriteRepo, // Write tracked files, git index, commits + SpawnProcess, // Fork external processes + WriteTemp, // Write to $TMPDIR, target/, .cache/ + WriteUserState, // Write to ~/.gunbc, ~/.codex, caches + Network, // HTTP calls, LLM APIs +} +``` + +This maps to our classifications: + +| Classification | Allowed Effects | +|----------------|-----------------| +| **Query** | `ReadRepo + SpawnProcess + WriteTemp + Network` | +| **Journal** | `WriteUserState` (durable checkpoints) | +| **Command** | `WriteRepo` | + +**Why this matters**: The runtime can *prove* "no WriteRepo occurred" instead of hoping. Running tests in a hermetic workspace (git worktree to temp dir) lets them write build artifacts without violating "no repo mutation". + ### Scope Purity: SubDag Encapsulation A SubDag is **hermetic** — its internal nodes cannot reach outside: @@ -273,21 +298,45 @@ pub struct StepRecord { pub step_type: StepType, pub completed_at: u64, // Unix timestamp pub outputs: HashMap, // Step outputs for downstream + pub cache_policy: CachePolicy, // When to rerun this step } #[derive(Debug, Clone, Serialize, Deserialize)] pub enum StepType { GatherContext, CodexInvocation { turn: u32 }, + ReviewInvocation, ApplyChanges, Checkpoint, } +/// LLMs are nondeterministic — need explicit cache policy +#[derive(Debug, Clone, Serialize, Deserialize)] +pub enum CachePolicy { + /// Never rerun if step_id exists (crash recovery) + Immutable, + /// Always rerun, ignore cache + Refresh, + /// Rerun if cached result is older than duration + RefreshIfOlderThan(u64), // millis + /// Rerun if model version changed + RefreshIfModelChanged { model_fingerprint: String }, +} + #[derive(Debug, Clone, Serialize, Deserialize)] pub struct ConversationState { - pub messages: Vec, + /// Use Codex's native session ID — don't duplicate conversation state + /// Resume via: `codex resume ` or `codex exec resume ` + pub codex_session_id: Option, + pub mode: CodexMode, pub turn_count: u32, - pub codex_session_file: Option, // Codex's own session file if applicable + pub last_prompt_hash: Option, +} + +#[derive(Debug, Clone, Serialize, Deserialize)] +pub enum CodexMode { + Interactive, // `codex` — human-in-the-loop + Exec, // `codex exec` — non-interactive } #[derive(Debug, Clone, Serialize, Deserialize)] @@ -595,27 +644,81 @@ pub struct Check { pub struct ReviewOutput { pub criteria_name: String, // Which criteria was applied pub findings: Vec, // Where artifact diverges from criteria + pub remediation: RemediationPlan, // Structured tasks to fix findings pub summary: String, // Natural language summary } /// Finding = divergence from criteria #[derive(Debug, Clone, Serialize, Deserialize)] pub struct Finding { + /// Stable identity for merging/dedup (hash of canonical fields) + pub id: String, pub check_id: String, // Which Check this relates to pub location: Option, pub observation: String, // What was found pub suggested_fix: Option, } +/// Stable finding ID = hash of canonical fields +fn finding_id(check_id: &str, location: &Option, observation: &str) -> String { + let mut hasher = blake3::Hasher::new(); + hasher.update(check_id.as_bytes()); + if let Some(loc) = location { + hasher.update(&serde_json::to_vec(loc).unwrap()); + } + // Normalize: strip numbers, collapse whitespace + let normalized = observation.split_whitespace().collect::>().join(" "); + hasher.update(normalized.as_bytes()); + hasher.finalize().to_hex()[..16].to_string() +} + +/// Structured remediation output (makes the loop truly fractal) #[derive(Debug, Clone, Serialize, Deserialize)] -pub struct Location { +pub struct RemediationPlan { + pub goals: Vec, // Acceptance criteria + pub tasks: Vec, // Concrete edits + pub constraints: Vec, // "do not change public API", etc. +} + +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct RemediationTask { + pub finding_id: String, // Links to Finding.id pub file: Option, - pub line_start: Option, - pub line_end: Option, + pub intent: String, // What to change + pub suggested_patch: Option, // Optional diff snippet + pub validation: Vec, // Commands or checks to pass +} + +/// Location in source — handles both files and diffs +#[derive(Debug, Clone, Serialize, Deserialize)] +pub enum Location { + /// Position in a file + FileLine { + file: String, + line: u32, + }, + /// Span in a file + Span { + file: String, + start: u32, + end: u32, + }, + /// Position in a diff (need to distinguish old vs new) + DiffLine { + file: String, + line: u32, + side: DiffSide, + }, +} + +#[derive(Debug, Clone, Serialize, Deserialize)] +pub enum DiffSide { + Old, // Line in the removed version + New, // Line in the added version } ``` -**Note**: No `FindingLevel`, no `Verdict`, no `NextStep`. Those belong in orchestration. +**Note**: No `FindingLevel`, no `Verdict`, no `NextStep`. Those belong in orchestration. RemediationPlan is pure data — orchestration decides whether to execute it. --- @@ -920,12 +1023,15 @@ pub fn build_implement_and_review( ``` **What to build for V0**: -- [ ] `lib/review/` crate with types (`Artifact`, `Criteria`, `Check`, `Finding`, `ReviewOutput`) +- [ ] `lib/review/` crate with types: + - `Artifact`, `Criteria`, `Check` + - `Finding` (with hash-based `id`), `Location` (with `DiffSide`) + - `ReviewOutput`, `RemediationPlan`, `RemediationTask` - [ ] `ReviewOps::PrepareReviewPrompt` — assemble prompt from artifact + criteria -- [ ] `ReviewOps::ParseReviewResponse` — parse LLM response into findings +- [ ] `ReviewOps::ParseReviewResponse` — parse LLM response into findings + remediation plan - [ ] Simple CLI: `gunbc review --diff HEAD~1` → prints JSON -**Note**: V0 does NOT include orchestration (Review Cycle) — just single-criteria review. +**Note**: V0 does NOT include orchestration (Review Cycle) — just single-criteria review. RemediationPlan is output but not executed. **V0 explicitly does NOT include**: - Durability / checkpointing @@ -952,7 +1058,10 @@ After V0 works: #### Phase 1: Core Types (V0 blocker) - [ ] Create `lib/review/` crate -- [ ] Define types: `Artifact`, `Criteria`, `Check`, `Finding`, `Location`, `ReviewOutput` +- [ ] Define types: + - `Artifact`, `Criteria`, `Check` + - `Finding` (hash-based id), `Location` (with `DiffSide`) + - `ReviewOutput`, `RemediationPlan`, `RemediationTask` - [ ] Define orchestration types: `ReviewCycleConfig`, `IteratePolicy`, `CycleResult`, `CycleOutcome` - [ ] Implement `ReviewOps` with `Executable` trait - [ ] Wire up `ReviewPhaseBuilder` using existing patterns @@ -960,21 +1069,23 @@ After V0 works: #### Phase 2: Transport Classification (architectural) - [ ] Add `ExecuteQuery` / `ExecuteJournal` / `ExecuteCommand` variants to `TransportOps` +- [ ] Add `Effect` enum for runtime enforcement (ReadRepo, WriteRepo, SpawnProcess, etc.) - [ ] Update executor to handle new variants - [ ] Add SubDag validation: "contains no ExecuteCommand nodes" #### Phase 3: ImplementationPhase (V1) - [ ] Create `lib/codex/` crate -- [ ] Define types: `ImplementationSession`, `StepRecord`, `ConversationState` +- [ ] Define types: `ImplementationSession`, `StepRecord`, `ConversationState`, `CodexMode` +- [ ] Add `CachePolicy` for nondeterministic LLM steps - [ ] Implement `ImplementationOps` -- [ ] Integrate Codex CLI invocation (Query transport) +- [ ] Integrate Codex CLI via `codex_session_id` + native resume (`codex resume `) - [ ] Add Journal transport for session state persistence #### Phase 4: Durability (V1+) - [ ] Implement `ExecuteJournal` for checkpoint storage -- [ ] Implement `CheckStepDone` / `RecordStep` operations +- [ ] Implement `CheckStepDone` / `RecordStep` operations with `CachePolicy` support - [ ] Add content-addressed step ID computation - [ ] Test crash recovery @@ -989,7 +1100,11 @@ After V0 works: ## Open Questions -1. **Codex session files**: Does Codex CLI have its own session persistence? If so, how do we coordinate? +1. ~~**Codex session files**~~: **Resolved** — Codex CLI has native session persistence: + - Interactive: `codex resume [SESSION_ID]` + - Non-interactive: `codex exec resume ` + - State stored under `CODEX_HOME` (default `~/.codex`) + - **Strategy**: Store `codex_session_id` as pointer, use native resume 2. **Multi-file implementation**: When Codex generates changes across many files, should we review: - All files together (single review)? From 761c43de8ad4ce17fe9b3999ad1493d62c68a8a8 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 1 Feb 2026 20:51:37 +0000 Subject: [PATCH 08/15] Simplify Effect Capabilities to future consideration The read/write axis is convenient for testing/purity reasoning but not structurally fundamental. The actual concern is risk/interest - we care more about some reads (secrets) than some writes (caches). For now: Query/Journal/Command at the structural level is sufficient. As project advances: domain-specific risk profiles, writes can structure their own risk categories. https://claude.ai/code/session_01RZryjdBirzK2p6KVyGttju --- TODO/llm-code-review-pipeline.md | 27 ++++++--------------------- 1 file changed, 6 insertions(+), 21 deletions(-) diff --git a/TODO/llm-code-review-pipeline.md b/TODO/llm-code-review-pipeline.md index 9d7b798ceb3..323febf0606 100644 --- a/TODO/llm-code-review-pipeline.md +++ b/TODO/llm-code-review-pipeline.md @@ -78,30 +78,15 @@ pub enum TransportOps { **The rule**: A SubDag can be verified to contain "no `ExecuteCommand` nodes" — that's what makes ReviewPhase structurally query-only. -### Effect Capabilities (Runtime Enforcement) +### Effect Capabilities (Future Consideration) -Query/Journal/Command is the user-facing abstraction. Underneath, implement as a finer-grained **effect capability** system for runtime verification: +**Current state**: The repo uses read/write as the primary discrimination axis. This is convenient for testing and reasoning about purity — it's easy to say "this phase only reads" — but it's not structurally fundamental. -```rust -enum Effect { - ReadRepo, // Read tracked files, git state - WriteRepo, // Write tracked files, git index, commits - SpawnProcess, // Fork external processes - WriteTemp, // Write to $TMPDIR, target/, .cache/ - WriteUserState, // Write to ~/.gunbc, ~/.codex, caches - Network, // HTTP calls, LLM APIs -} -``` - -This maps to our classifications: +**The honest framing**: Read/write isn't the real concern. We care more about *some* reads (secrets, credentials) than *some* writes (temp caches). The actual concern is **risk/interest** — what do we need to track for testing, isolation, and safety? -| Classification | Allowed Effects | -|----------------|-----------------| -| **Query** | `ReadRepo + SpawnProcess + WriteTemp + Network` | -| **Journal** | `WriteUserState` (durable checkpoints) | -| **Command** | `WriteRepo` | +**For now**: Query/Journal/Command at the structural level (which `TransportOps` variants exist) is sufficient. DryRun mode intercepts transport boundaries for mocking. -**Why this matters**: The runtime can *prove* "no WriteRepo occurred" instead of hoping. Running tests in a hermetic workspace (git worktree to temp dir) lets them write build artifacts without violating "no repo mutation". +**As the project advances**: We'll likely need domain-specific risk profiles. Writes can structure their own risk categories (e.g., "repo mutation" vs "cache update" vs "credential storage"). The current read/write split is a placeholder for that richer model. ### Scope Purity: SubDag Encapsulation @@ -1069,9 +1054,9 @@ After V0 works: #### Phase 2: Transport Classification (architectural) - [ ] Add `ExecuteQuery` / `ExecuteJournal` / `ExecuteCommand` variants to `TransportOps` -- [ ] Add `Effect` enum for runtime enforcement (ReadRepo, WriteRepo, SpawnProcess, etc.) - [ ] Update executor to handle new variants - [ ] Add SubDag validation: "contains no ExecuteCommand nodes" +- [ ] (Future) Domain-specific risk profiles — defer until we have concrete use cases #### Phase 3: ImplementationPhase (V1) From afc553293ce7e651ffc958004988b9670429cd76 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 1 Feb 2026 20:56:13 +0000 Subject: [PATCH 09/15] Fix purity issues and add enforcement mechanisms - Rename `suggested_fix` to `remediation_hint` (reconciliation, not advice) - Add `source` provenance to Finding for MultiReview merging - Change `ApplyChanges` to `PrepareArtifacts` (Query-only) - Add JournalScope contract (allowed paths prevent Command-in-disguise) - Add DryRunMode enum (Strict/Safe/Off) for preview runs - Add phase-level validation table (ReviewPhase: no Command/Journal) - Update IteratePolicy: OnRemediableFindings, add OnAnyFindings - Update implementation tasks in Phases 1 and 2 https://claude.ai/code/session_01RZryjdBirzK2p6KVyGttju --- TODO/llm-code-review-pipeline.md | 75 ++++++++++++++++++++++++++++---- 1 file changed, 67 insertions(+), 8 deletions(-) diff --git a/TODO/llm-code-review-pipeline.md b/TODO/llm-code-review-pipeline.md index 323febf0606..5143ee533f6 100644 --- a/TODO/llm-code-review-pipeline.md +++ b/TODO/llm-code-review-pipeline.md @@ -88,6 +88,46 @@ pub enum TransportOps { **As the project advances**: We'll likely need domain-specific risk profiles. Writes can structure their own risk categories (e.g., "repo mutation" vs "cache update" vs "credential storage"). The current read/write split is a placeholder for that richer model. +### Journal Scope Contract + +Journal is "tool-owned state" — but without enforcement, it becomes "Command in disguise." Define allowed roots: + +```rust +/// Journal writes are constrained to these paths +pub struct JournalScope { + /// Allowed filesystem roots (e.g., ["~/.gunbc/", "$TMPDIR/gunbc-"]) + pub allowed_paths: Vec, + /// Resource IDs for locking/concurrency + pub resource_ids: Vec, +} +``` + +The executor enforces: `ExecuteJournal` can only write within `allowed_paths`. Writes outside → contract violation. + +### DryRun Modes + +With Query/Journal/Command, DryRun becomes explicit: + +```rust +pub enum DryRunMode { + /// Intercept ALL transport (Query/Journal/Command) — pure mock + Strict, + /// Allow Query, optionally allow Journal, block Command + Safe { allow_journal: bool }, + /// No interception — real execution + Off, +} +``` + +| Mode | Query | Journal | Command | +|------|-------|---------|---------| +| **Strict** | mock | mock | mock | +| **Safe(journal=true)** | allow | allow | mock | +| **Safe(journal=false)** | allow | mock | mock | +| **Off** | allow | allow | allow | + +This makes "preview runs" and tests explicit about what's being exercised. + ### Scope Purity: SubDag Encapsulation A SubDag is **hermetic** — its internal nodes cannot reach outside: @@ -133,6 +173,15 @@ Applied to SubDags: **The pattern**: Phases that "decide" use Query (+ Journal for durability). Phases that "act" use Command. Commands are concentrated at workflow boundaries. +**Phase-level validation** (enforced by builder/executor): + +| Phase | Validation Rule | +|-------|-----------------| +| **ReviewPhase** | No `ExecuteCommand`, no `ExecuteJournal` | +| **ImplementationPhase** | No `ExecuteCommand` | +| **TestPhase** | No `ExecuteCommand` | +| **ApplyPhase** | `ExecuteCommand` allowed | + --- ## Part 1: Fractal Workflow Architecture @@ -291,7 +340,8 @@ pub enum StepType { GatherContext, CodexInvocation { turn: u32 }, ReviewInvocation, - ApplyChanges, + /// Prepare patch/plan (Query-only) — actual apply is a Command phase outside + PrepareArtifacts, Checkpoint, } @@ -641,7 +691,10 @@ pub struct Finding { pub check_id: String, // Which Check this relates to pub location: Option, pub observation: String, // What was found - pub suggested_fix: Option, + /// Example way to satisfy criteria (part of reconciliation, not "next step advice") + pub remediation_hint: Option, + /// Provenance for MultiReview merging (e.g., "llm:gpt-4o", "cargo:clippy") + pub source: String, } /// Stable finding ID = hash of canonical fields @@ -734,8 +787,10 @@ pub struct ReviewCycleConfig { } pub enum IteratePolicy { - /// Iterate if any findings have suggested_fix - OnFixableFinding, + /// Iterate if any findings have remediation_hint + OnRemediableFindings, + /// Iterate on any findings (not just those with hints) + OnAnyFindings, /// Never auto-iterate, always proceed AlwaysProceed, /// Custom logic @@ -1010,7 +1065,7 @@ pub fn build_implement_and_review( **What to build for V0**: - [ ] `lib/review/` crate with types: - `Artifact`, `Criteria`, `Check` - - `Finding` (with hash-based `id`), `Location` (with `DiffSide`) + - `Finding` (hash-based `id`, `source` provenance, `remediation_hint`), `Location` (with `DiffSide`) - `ReviewOutput`, `RemediationPlan`, `RemediationTask` - [ ] `ReviewOps::PrepareReviewPrompt` — assemble prompt from artifact + criteria - [ ] `ReviewOps::ParseReviewResponse` — parse LLM response into findings + remediation plan @@ -1045,7 +1100,7 @@ After V0 works: - [ ] Create `lib/review/` crate - [ ] Define types: - `Artifact`, `Criteria`, `Check` - - `Finding` (hash-based id), `Location` (with `DiffSide`) + - `Finding` (hash-based id, `source` provenance, `remediation_hint`), `Location` (with `DiffSide`) - `ReviewOutput`, `RemediationPlan`, `RemediationTask` - [ ] Define orchestration types: `ReviewCycleConfig`, `IteratePolicy`, `CycleResult`, `CycleOutcome` - [ ] Implement `ReviewOps` with `Executable` trait @@ -1054,8 +1109,12 @@ After V0 works: #### Phase 2: Transport Classification (architectural) - [ ] Add `ExecuteQuery` / `ExecuteJournal` / `ExecuteCommand` variants to `TransportOps` -- [ ] Update executor to handle new variants -- [ ] Add SubDag validation: "contains no ExecuteCommand nodes" +- [ ] Add `JournalScope` contract (allowed paths, resource IDs) +- [ ] Add `DryRunMode` enum (Strict / Safe / Off) +- [ ] Update executor to handle new variants + enforce Journal scope +- [ ] Add phase-level validation: + - ReviewPhase: no ExecuteCommand, no ExecuteJournal + - ImplementationPhase: no ExecuteCommand - [ ] (Future) Domain-specific risk profiles — defer until we have concrete use cases #### Phase 3: ImplementationPhase (V1) From 57ff0587b4f7e2396bbb30c153af7c49012171c4 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 1 Feb 2026 21:04:03 +0000 Subject: [PATCH 10/15] Move Query/Journal/Command classification to appendix User feedback: read/write classification isn't structurally fundamental. Will use fermi-style risk categories (low/medium/high/extreme) instead. - Simplified main Transport Classification section to "Future Consideration" - Moved JournalScope, DryRunMode, phase-level validation to appendix - Updated phase table to use risk levels instead of transport types - Updated implementation tasks to reference risk categories - Preserved full Q/J/C design in appendix for future reference https://claude.ai/code/session_01RZryjdBirzK2p6KVyGttju --- TODO/llm-code-review-pipeline.md | 192 ++++++++++++++----------------- 1 file changed, 84 insertions(+), 108 deletions(-) diff --git a/TODO/llm-code-review-pipeline.md b/TODO/llm-code-review-pipeline.md index 5143ee533f6..76617e62632 100644 --- a/TODO/llm-code-review-pipeline.md +++ b/TODO/llm-code-review-pipeline.md @@ -43,90 +43,15 @@ gunbc has **two kinds of boundaries** that must not be conflated: └─────────────────────────────────────────────────────────────────────┘ ``` -### Transport Classification: Query / Journal / Command (Structural) - -Transport is classified by **typed execute ops**, not flags. This makes enforcement structural. - -| Class | Execute Op | Scope | Examples | -|-------|------------|-------|----------| -| **Query** | `TransportOps::ExecuteQuery` | Read external state | LLM calls, file reads, git diff, HTTP GET | -| **Journal** | `TransportOps::ExecuteJournal` | Write tool-owned cache/state | Checkpoints, session state, response cache | -| **Command** | `TransportOps::ExecuteCommand` | Mutate user artifacts / external systems | File writes, git commit, apply patch | - -**Why three categories?** -- `cargo test` writes to `target/` but we consider it "query-like" — it's tool-owned state -- Durability checkpoints write to `~/.gunbc/` — also tool-owned, not user artifacts -- Only Commands mutate things the user controls - -**Structural enforcement** (leverages existing `AccessMode` precedent): - -```rust -/// Transport execute operations — typed for structural enforcement -pub enum TransportOps { - /// Read-only access to external state - ExecuteQuery, - - /// Write to tool-owned cache/journal (durability, caching) - /// Does NOT mutate user-controlled artifacts - ExecuteJournal, - - /// Mutate user-controlled artifacts or external systems - /// Only allowed at workflow boundaries - ExecuteCommand, -} -``` - -**The rule**: A SubDag can be verified to contain "no `ExecuteCommand` nodes" — that's what makes ReviewPhase structurally query-only. - -### Effect Capabilities (Future Consideration) +### Transport Classification (Future Consideration) **Current state**: The repo uses read/write as the primary discrimination axis. This is convenient for testing and reasoning about purity — it's easy to say "this phase only reads" — but it's not structurally fundamental. **The honest framing**: Read/write isn't the real concern. We care more about *some* reads (secrets, credentials) than *some* writes (temp caches). The actual concern is **risk/interest** — what do we need to track for testing, isolation, and safety? -**For now**: Query/Journal/Command at the structural level (which `TransportOps` variants exist) is sufficient. DryRun mode intercepts transport boundaries for mocking. - -**As the project advances**: We'll likely need domain-specific risk profiles. Writes can structure their own risk categories (e.g., "repo mutation" vs "cache update" vs "credential storage"). The current read/write split is a placeholder for that richer model. +**For V0/V1**: Use fermi-style risk categories (low/medium/high/extreme) rather than structural read/write classification. The Query/Journal/Command model below is a placeholder for future work when we have concrete use cases. -### Journal Scope Contract - -Journal is "tool-owned state" — but without enforcement, it becomes "Command in disguise." Define allowed roots: - -```rust -/// Journal writes are constrained to these paths -pub struct JournalScope { - /// Allowed filesystem roots (e.g., ["~/.gunbc/", "$TMPDIR/gunbc-"]) - pub allowed_paths: Vec, - /// Resource IDs for locking/concurrency - pub resource_ids: Vec, -} -``` - -The executor enforces: `ExecuteJournal` can only write within `allowed_paths`. Writes outside → contract violation. - -### DryRun Modes - -With Query/Journal/Command, DryRun becomes explicit: - -```rust -pub enum DryRunMode { - /// Intercept ALL transport (Query/Journal/Command) — pure mock - Strict, - /// Allow Query, optionally allow Journal, block Command - Safe { allow_journal: bool }, - /// No interception — real execution - Off, -} -``` - -| Mode | Query | Journal | Command | -|------|-------|---------|---------| -| **Strict** | mock | mock | mock | -| **Safe(journal=true)** | allow | allow | mock | -| **Safe(journal=false)** | allow | mock | mock | -| **Off** | allow | allow | allow | - -This makes "preview runs" and tests explicit about what's being exercised. +**See**: [Appendix: Transport Classification Details](#appendix-transport-classification-details) for the full Query/Journal/Command design if needed later. ### Scope Purity: SubDag Encapsulation @@ -163,24 +88,15 @@ Applied to SubDags: ### How This Applies to Our Phases -| Phase | Transport Types Allowed | Reasoning | -|-------|-------------------------|-----------| -| **ReviewPhase** | Query only | LLM calls observe code, don't mutate it. Output is data (findings), not action. | -| **ImplementationPhase** | Query + Journal | Codex queries produce artifacts. Checkpoints use Journal. Applying artifacts happens outside. | -| **TestPhase** | Query + Journal | Running tests observes behavior. Test caching uses Journal. | -| **CommitPhase** | Command | Git commit mutates the repository. | -| **ApplyPhase** | Command | File writes, patch application. | - -**The pattern**: Phases that "decide" use Query (+ Journal for durability). Phases that "act" use Command. Commands are concentrated at workflow boundaries. +| Phase | Risk Level | Reasoning | +|-------|------------|-----------| +| **ReviewPhase** | Low | LLM calls observe code, don't mutate it. Output is data (findings), not action. | +| **ImplementationPhase** | Medium | Codex queries produce artifacts. Durability writes are tool-owned. User artifacts are not mutated. | +| **TestPhase** | Medium | Running tests observes behavior. May write to tool-owned caches. | +| **CommitPhase** | High | Git commit mutates the repository. | +| **ApplyPhase** | High | File writes, patch application. | -**Phase-level validation** (enforced by builder/executor): - -| Phase | Validation Rule | -|-------|-----------------| -| **ReviewPhase** | No `ExecuteCommand`, no `ExecuteJournal` | -| **ImplementationPhase** | No `ExecuteCommand` | -| **TestPhase** | No `ExecuteCommand` | -| **ApplyPhase** | `ExecuteCommand` allowed | +**The pattern**: Phases that "decide" are low/medium risk. Phases that "act" are high risk. High-risk operations are concentrated at workflow boundaries. --- @@ -1037,9 +953,9 @@ pub fn build_implement_and_review( ### Phase 0: Core Architecture (if generalizing) -- [ ] Consider adding Query/Command transport classification to `core/ir/src/transport/` -- [ ] Consider updating AGENT.md with Query/Command principle -- [ ] Consider updating SPEC.md §6 (Node Contract) with transport classification +- [ ] Consider adding risk categories to transport operations (fermi-style: low/medium/high/extreme) +- [ ] Consider updating AGENT.md with risk classification principle +- [ ] (Future) Consider Query/Command structural classification — see appendix ### V0: Minimal Useful Pipeline (First Target) @@ -1054,7 +970,7 @@ pub fn build_implement_and_review( **V0 flow**: ``` -1. GitOps::PrepareDiff → ExecuteQuery → GitOps::ParseDiff +1. GitOps::PrepareDiff → Execute → GitOps::ParseDiff 2. (Optional) CargoOp::check/test/clippy 3. ReviewOps::PrepareReviewPrompt(diff + tool_outputs + criteria) 4. [lib/llm-ops chat subdag] ← reuse existing @@ -1106,16 +1022,11 @@ After V0 works: - [ ] Implement `ReviewOps` with `Executable` trait - [ ] Wire up `ReviewPhaseBuilder` using existing patterns -#### Phase 2: Transport Classification (architectural) +#### Phase 2: Risk Classification (future) -- [ ] Add `ExecuteQuery` / `ExecuteJournal` / `ExecuteCommand` variants to `TransportOps` -- [ ] Add `JournalScope` contract (allowed paths, resource IDs) -- [ ] Add `DryRunMode` enum (Strict / Safe / Off) -- [ ] Update executor to handle new variants + enforce Journal scope -- [ ] Add phase-level validation: - - ReviewPhase: no ExecuteCommand, no ExecuteJournal - - ImplementationPhase: no ExecuteCommand -- [ ] (Future) Domain-specific risk profiles — defer until we have concrete use cases +- [ ] Define fermi-style risk categories (low/medium/high/extreme) +- [ ] Add risk annotation to transport operations +- [ ] (Future) Consider Query/Journal/Command structural classification if needed — see appendix #### Phase 3: ImplementationPhase (V1) @@ -1176,3 +1087,68 @@ After V0 works: - **CLI Tools**: `core/ir/src/transport/cli.rs` — CliToolDef, CliToolOp - **LLM Ops**: `lib/llm-ops/src/` — existing chat completion patterns - **Git Ops**: `lib/git-ops/src/` — diff, branch operations + +--- + +## Appendix: Transport Classification Details + +**Status**: Future consideration. Preserved here for reference when we have concrete use cases. + +**Background conversation**: The repo uses read/write as the primary discrimination axis. This is convenient for testing and reasoning about purity, but it's not structurally fundamental. Read/write isn't the real concern — we care more about *some* reads (secrets, credentials) than *some* writes (temp caches). The actual concern is **risk/interest**. We'll likely need domain-specific risk profiles. Writes can structure their own risk categories (e.g., "repo mutation" vs "cache update" vs "credential storage"). The current read/write split is a placeholder for that richer model. + +### Query / Journal / Command Classification + +Transport classified by typed execute ops, not flags: + +| Class | Execute Op | Scope | Examples | +|-------|------------|-------|----------| +| **Query** | `ExecuteQuery` | Read external state | LLM calls, file reads, git diff, HTTP GET | +| **Journal** | `ExecuteJournal` | Write tool-owned cache/state | Checkpoints, session state, response cache | +| **Command** | `ExecuteCommand` | Mutate user artifacts / external systems | File writes, git commit, apply patch | + +```rust +pub enum TransportOps { + ExecuteQuery, + ExecuteJournal, + ExecuteCommand, +} +``` + +### Journal Scope Contract + +Journal is "tool-owned state" — but without enforcement, it becomes "Command in disguise." + +```rust +pub struct JournalScope { + pub allowed_paths: Vec, // e.g., ["~/.gunbc/", "$TMPDIR/gunbc-"] + pub resource_ids: Vec, +} +``` + +Executor enforces: `ExecuteJournal` can only write within `allowed_paths`. + +### DryRun Modes + +```rust +pub enum DryRunMode { + Strict, // mock all + Safe { allow_journal: bool }, // allow Query, optionally Journal, block Command + Off, // real execution +} +``` + +| Mode | Query | Journal | Command | +|------|-------|---------|---------| +| **Strict** | mock | mock | mock | +| **Safe(journal=true)** | allow | allow | mock | +| **Safe(journal=false)** | allow | mock | mock | +| **Off** | allow | allow | allow | + +### Phase-level Validation (if using this model) + +| Phase | Validation Rule | +|-------|-----------------| +| **ReviewPhase** | No `ExecuteCommand`, no `ExecuteJournal` | +| **ImplementationPhase** | No `ExecuteCommand` | +| **TestPhase** | No `ExecuteCommand` | +| **ApplyPhase** | `ExecuteCommand` allowed | From 3a2c51115a1cf79d11d2fb4eef2bf1a210c7d4c6 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 1 Feb 2026 21:32:57 +0000 Subject: [PATCH 11/15] Address design review feedback for conceptual clarity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Major clarifications based on detailed review: **Boundary terminology**: - Rename "Workflow Boundary" → "Interface Boundary" to avoid confusion - Add north star paragraph unifying the design philosophy **Phase taxonomy**: - Introduce Reasoning vs Action phases (replaces Query/Command at phase level) - Action phases (Apply/Commit) explicitly designated for mutation - Static validation: reasoning phases contain no high-risk transport ops **Transport classification**: - Add "ownership not security" framing - Add TransportMeta struct for future risk policies (domain, scope) - Preserve fermi-style risk levels (low/medium/high/extreme) **Finding stability**: - Add issue_key field for stable cross-iteration matching - Use issue_key instead of observation text for ID hashing - Add ReviewBundle for multi-source merge container **Remediation framing**: - Rename RemediationPlan → CandidateRemediations (proposals, not commands) - Rename suggested_fix → candidate_fix throughout - Review = reconciliation + candidate repairs (not "what to do next") **Session/step model**: - Split session_id (UUID identity) vs task_id (content hash) - Add world state inputs requirement for step caching - Fix statelessness → "no implicit state" contract **ImplementationPhase invariant**: - Explicit: Codex cannot mutate user artifacts - Changes emitted as patch artifacts, applied by separate ApplyPhase **V0 checklist**: - Add crispness checklist for architecture alignment acceptance https://claude.ai/code/session_01RZryjdBirzK2p6KVyGttju --- TODO/llm-code-review-pipeline.md | 195 ++++++++++++++++++++----------- 1 file changed, 126 insertions(+), 69 deletions(-) diff --git a/TODO/llm-code-review-pipeline.md b/TODO/llm-code-review-pipeline.md index 76617e62632..b31defb7c86 100644 --- a/TODO/llm-code-review-pipeline.md +++ b/TODO/llm-code-review-pipeline.md @@ -17,6 +17,8 @@ These phases compose into larger workflows (Requirements → Design → Implemen This section reconciles with AGENT.md ("Nodes are Pure, Boundaries are Structural") and SPEC.md (§2.7 "No side channels", §6 "Node Contract"). +> **North Star**: gunbc workflows are pure dataflow graphs whose only interaction with the outside world occurs at explicit transport nodes. Transport operations are classified by risk level (low/medium/high/extreme) to enable static validation (e.g., "review contains no high-risk ops") and runtime interception (DryRun). Composition happens through interface boundaries (unconnected ports). Higher-level workflows are built by composing subdags, concentrating mutation inside explicit **action phases** while keeping **reasoning phases** hermetic and replayable. + ### Boundary Terminology (Two Distinct Concepts) gunbc has **two kinds of boundaries** that must not be conflated: @@ -24,7 +26,7 @@ gunbc has **two kinds of boundaries** that must not be conflated: | Term | Definition | How Detected | Purpose | |------|------------|--------------|---------| | **Transport Boundary** | Node that executes `TransportOps::Execute*` | By op type | Where I/O happens; DryRun intercepts here | -| **Workflow Boundary** | Unconnected output port | By graph structure | DAG interface; signature for composition | +| **Interface Boundary** | Unconnected output port | By graph structure | DAG composition interface | ``` ┌─────────────────────────────────────────────────────────────────────┐ @@ -37,21 +39,36 @@ gunbc has **two kinds of boundaries** that must not be conflated: │ └─────────────┘ │ │ │ ▼ │ │ ┌───────────┐ │ -│ │ output │ (WORKFLOW │ +│ │ output │ (INTERFACE │ │ │ (unconnected) BOUNDARY)│ │ └───────────┘ │ └─────────────────────────────────────────────────────────────────────┘ ``` -### Transport Classification (Future Consideration) - -**Current state**: The repo uses read/write as the primary discrimination axis. This is convenient for testing and reasoning about purity — it's easy to say "this phase only reads" — but it's not structurally fundamental. +### Transport Classification **The honest framing**: Read/write isn't the real concern. We care more about *some* reads (secrets, credentials) than *some* writes (temp caches). The actual concern is **risk/interest** — what do we need to track for testing, isolation, and safety? -**For V0/V1**: Use fermi-style risk categories (low/medium/high/extreme) rather than structural read/write classification. The Query/Journal/Command model below is a placeholder for future work when we have concrete use cases. +**Ownership vs Security**: The appendix describes a Query/Journal/Command model. That's an **ownership classification** (who controls what's being read/written), not a security classification. A "Query" can still exfiltrate secrets; a "Journal" can still persist sensitive data; a "Query-like" CLI command can execute arbitrary code. + +**For V0/V1**: Use fermi-style risk categories (low/medium/high/extreme) rather than structural read/write classification. Risk is assessed per-transport based on domain and scope, not just ownership. -**See**: [Appendix: Transport Classification Details](#appendix-transport-classification-details) for the full Query/Journal/Command design if needed later. +**Future-proofing**: Even if not enforced in V0, transport ops should carry metadata for later policy: + +```rust +/// Metadata for risk assessment (V1+) +pub struct TransportMeta { + pub risk: RiskLevel, // Low | Medium | High | Extreme + pub domain: TransportDomain, // Git | Cargo | LLM | FS | HTTP | ... + pub scope: TransportScope, // RepoPath(...) | ToolHome(...) | NetworkHost(...) +} + +pub enum RiskLevel { Low, Medium, High, Extreme } +``` + +Later "risk profiles" become a policy over `(risk, domain, scope)` instead of invasive refactoring. + +**See**: [Appendix: Transport Classification Details](#appendix-transport-classification-details) for the full Query/Journal/Command ownership model if needed later. ### Scope Purity: SubDag Encapsulation @@ -66,7 +83,7 @@ that bypasses edges is a contract violation." Applied to SubDags: 1. **Inputs**: SubDag internals see only what's passed through entrypoints -2. **Outputs**: Results leave only through declared workflow boundaries +2. **Outputs**: Results leave only through declared interface boundaries 3. **No leakage**: Inner nodes cannot reference sibling nodes in parent DAG 4. **Resolution order**: SubDag must fully resolve before parent sees outputs @@ -86,17 +103,21 @@ Applied to SubDags: **Implication for ReviewPhase**: The parent doesn't know ReviewPhase uses an LLM internally. It only sees: `(artifact, criteria) → findings`. -### How This Applies to Our Phases +### Phase Taxonomy: Reasoning vs Action + +Phases are classified by whether they **decide** or **act**: -| Phase | Risk Level | Reasoning | -|-------|------------|-----------| -| **ReviewPhase** | Low | LLM calls observe code, don't mutate it. Output is data (findings), not action. | -| **ImplementationPhase** | Medium | Codex queries produce artifacts. Durability writes are tool-owned. User artifacts are not mutated. | -| **TestPhase** | Medium | Running tests observes behavior. May write to tool-owned caches. | -| **CommitPhase** | High | Git commit mutates the repository. | -| **ApplyPhase** | High | File writes, patch application. | +| Phase | Type | Risk Level | Reasoning | +|-------|------|------------|-----------| +| **ReviewPhase** | Reasoning | Low | LLM calls observe code, don't mutate it. Output is data (findings), not action. | +| **ImplementationPhase** | Reasoning | Medium | Codex queries produce artifacts (patches). Durability writes are tool-owned. User artifacts are not mutated. | +| **TestPhase** | Reasoning | Medium | Running tests observes behavior. May write to tool-owned caches. | +| **ApplyPhase** | Action | High | File writes, patch application. Mutates user artifacts. | +| **CommitPhase** | Action | High | Git commit mutates the repository. | -**The pattern**: Phases that "decide" are low/medium risk. Phases that "act" are high risk. High-risk operations are concentrated at workflow boundaries. +**The invariant**: High-risk mutation (Action Phases) only occurs in explicitly designated action subdags. Reasoning phases are hermetic and replayable — they produce *artifacts* (patches, plans, findings) but don't apply them. + +**Static validation**: A reasoning phase subdag can be verified to contain no high-risk transport ops. This makes "ReviewPhase is read-only" a structural property, not just a naming convention. --- @@ -168,9 +189,11 @@ For this iteration, we're building: ### Design Goals 1. **Durability**: Survive crashes, resume seamlessly -2. **Statelessness**: All state flows through DAG edges (no hidden state) +2. **No implicit state**: All dependencies are explicit as either (a) DAG edges, or (b) declared transport accesses (including journal/checkpoint reads) 3. **Idempotency**: Re-running from start skips completed work -4. **Query/Command separation**: Codex invocations are Queries; file mutations are Commands at the boundary +4. **Reasoning/Action separation**: Codex invocations produce artifacts (patches); applying them is a separate Action Phase + +**Codex invariant**: ImplementationPhase must invoke Codex in a mode that **cannot mutate user artifacts**. Any file changes must be emitted as artifacts (patches) and applied only by ApplyPhase. (Enforce via sandbox, read-only mount, or Codex flags that output patches without applying.) ### The Challenge: Codex is Stateful @@ -179,35 +202,36 @@ Codex CLI maintains conversation state. Our challenge is to: - Persist it durably for crash recovery - Resume without duplicating work or losing context -### Query/Command Boundary in Implementation +### Reasoning/Action Boundary in Implementation ``` ┌─────────────────────────────────────────────────────────────────────┐ -│ ImplementationPhase │ +│ ImplementationPhase (Reasoning) │ │ │ │ ┌───────────────────────────────────────────────────────────────┐ │ -│ │ REASONING ZONE (Queries) │ │ +│ │ REASONING ZONE (Low/Medium Risk) │ │ │ │ │ │ │ │ GatherContext ──▶ CodexInvoke ──▶ ParseResponse ──▶ Decide │ │ -│ │ (file reads) (LLM query) (pure) (pure) │ │ +│ │ (file reads) (LLM call) (pure) (pure) │ │ │ │ │ │ │ │ Output: Artifacts (proposed changes, not yet applied) │ │ │ └───────────────────────────────────────────────────────────────┘ │ │ │ │ │ ▼ │ │ ┌───────────────────────────────────────────────────────────────┐ │ -│ │ ACTION ZONE (Commands) │ │ +│ │ ACTION ZONE (High Risk) — SEPARATE PHASE │ │ │ │ │ │ -│ │ ApplyArtifacts ──▶ WriteFiles │ │ -│ │ (at DAG boundary, after reasoning completes) │ │ +│ │ ApplyArtifacts ──▶ WriteFiles (ApplyPhase, not here) │ │ +│ │ (explicit action phase, after reasoning completes) │ │ │ └───────────────────────────────────────────────────────────────┘ │ └─────────────────────────────────────────────────────────────────────┘ ``` -The key insight: **Codex invocation is a Query** (asking "what should I do?"). **Applying the changes is a Command** (doing it). This separation means we can: +The key insight: **Codex invocation is Reasoning** (asking "what should I do?" → produces patch artifacts). **Applying the changes is Action** (doing it → separate phase). This separation means we can: - Test the reasoning without mutating files - Review proposed changes before applying - Roll back decisions without undoing file mutations +- DryRun the entire reasoning phase with no side effects ### Session State Model @@ -215,9 +239,12 @@ The key insight: **Codex invocation is a Query** (asking "what should I do?"). * /// Complete state of a Codex implementation session #[derive(Debug, Clone, Serialize, Deserialize)] pub struct ImplementationSession { - /// Unique session identifier (content-addressed) + /// Unique session identity (UUID/random) — for parallel sessions of same task pub session_id: String, + /// Content hash of task inputs — for dedup/resume matching + pub task_id: String, + /// The task being implemented pub task: ImplementationTask, @@ -234,6 +261,12 @@ pub struct ImplementationSession { pub status: SessionStatus, } +// session_id vs task_id: +// - session_id: random UUID (identity) — allows parallel sessions for same task +// - task_id: content hash (dedup key) — identifies "same task" for resume/lookup +// This prevents: (a) collision when two developers run same task, (b) accidental +// resume of unrelated session, (c) inability to have parallel experiments + #[derive(Debug, Clone, Serialize, Deserialize)] pub struct ImplementationTask { pub description: String, @@ -361,6 +394,12 @@ fn step_id(step_type: &StepType, inputs: &HashMap) -> String { - Changed inputs (e.g., new context) → new step ID → re-execute - No manual invalidation needed +**World state inputs**: A step is cacheable only relative to a declared **input snapshot** of the world state it depends on. Step inputs should include: +- `GatherContext`: `git HEAD` (or diff hash) + list of files read +- `CargoOp::test`: tool version (cargo + rustc), relevant env vars, manifest path +- LLM steps: model name + fingerprint, system prompt hash, temperature/seed +Otherwise caching becomes incorrect when the repo or environment changes. + ### Crash Recovery Flow ``` @@ -529,20 +568,20 @@ impl Default for CodexConfig { ## Part 3: ReviewPhase (Pure Reconciliation) -### Design Principle: Review = Reconciliation +### Design Principle: Review = Reconciliation + Candidate Repairs -**ReviewPhase is maximally simple**: given an artifact and criteria, report where the artifact diverges from the criteria. That's it. +**ReviewPhase is maximally simple**: given an artifact and criteria, report where the artifact diverges from the criteria, and optionally propose candidate repairs. That's it. - **No verdict** — that's orchestration - **No severity levels** — either it diverges or it doesn't -- **No "what to do next"** — that's orchestration -- **No opinions** — just reconciliation +- **No decisions** — that's orchestration (whether to apply, iterate, block) +- **Candidate repairs, not commands** — proposals are data; orchestration chooses whether to apply ``` -ReviewPhase: (artifact, criteria) → findings +ReviewPhase: (artifact, criteria) → (findings, candidate_remediations) ``` -The reviewer doesn't decide what happens with findings. Different criteria produce different findings. The orchestration layer decides what to do. +The reviewer doesn't decide what happens with findings or whether repairs are applied. Different criteria produce different findings. The orchestration layer decides what to do. ### Core Types @@ -590,13 +629,21 @@ pub struct Check { pub examples: Vec, // Example violations (optional) } -/// Review output = pure reconciliation +/// Review output = pure reconciliation + candidate repairs #[derive(Debug, Clone, Serialize, Deserialize)] pub struct ReviewOutput { - pub criteria_name: String, // Which criteria was applied - pub findings: Vec, // Where artifact diverges from criteria - pub remediation: RemediationPlan, // Structured tasks to fix findings - pub summary: String, // Natural language summary + pub criteria_name: String, // Which criteria was applied + pub source: String, // Provenance: "llm:gpt-4o", "cargo:clippy", etc. + pub findings: Vec, // Where artifact diverges from criteria + pub candidate_remediations: Option, // Optional repair proposals + pub summary: String, // Natural language summary +} + +/// Container for merging multiple reviews (same artifact, different criteria/sources) +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct ReviewBundle { + pub artifact_id: String, // Hash of artifact being reviewed + pub reviews: Vec, // Each has criteria_name + source } /// Finding = divergence from criteria @@ -604,43 +651,44 @@ pub struct ReviewOutput { pub struct Finding { /// Stable identity for merging/dedup (hash of canonical fields) pub id: String, - pub check_id: String, // Which Check this relates to + pub check_id: String, // Which Check this relates to pub location: Option, - pub observation: String, // What was found - /// Example way to satisfy criteria (part of reconciliation, not "next step advice") - pub remediation_hint: Option, - /// Provenance for MultiReview merging (e.g., "llm:gpt-4o", "cargo:clippy") - pub source: String, + /// Stable canonical key for cross-iteration matching (reviewer-provided) + /// Should NOT include line numbers; should include symbol/file/invariant + pub issue_key: String, + pub observation: String, // What was found (may vary in wording) + /// Example way to satisfy criteria (candidate, not command) + pub candidate_fix: Option, } -/// Stable finding ID = hash of canonical fields -fn finding_id(check_id: &str, location: &Option, observation: &str) -> String { +/// Stable finding ID = hash of (check_id, location context, issue_key) +/// Using issue_key instead of observation text for stability across LLM wording changes +fn finding_id(check_id: &str, location: &Option, issue_key: &str) -> String { let mut hasher = blake3::Hasher::new(); hasher.update(check_id.as_bytes()); if let Some(loc) = location { + // For diffs: include hunk context, not line numbers hasher.update(&serde_json::to_vec(loc).unwrap()); } - // Normalize: strip numbers, collapse whitespace - let normalized = observation.split_whitespace().collect::>().join(" "); - hasher.update(normalized.as_bytes()); + hasher.update(issue_key.as_bytes()); hasher.finalize().to_hex()[..16].to_string() } -/// Structured remediation output (makes the loop truly fractal) +/// Candidate repairs (proposals, not commands) #[derive(Debug, Clone, Serialize, Deserialize)] -pub struct RemediationPlan { - pub goals: Vec, // Acceptance criteria - pub tasks: Vec, // Concrete edits - pub constraints: Vec, // "do not change public API", etc. +pub struct CandidateRemediations { + pub goals: Vec, // Acceptance criteria + pub tasks: Vec, // Proposed edits + pub constraints: Vec, // "do not change public API", etc. } #[derive(Debug, Clone, Serialize, Deserialize)] -pub struct RemediationTask { - pub finding_id: String, // Links to Finding.id +pub struct CandidateTask { + pub finding_id: String, // Links to Finding.id pub file: Option, - pub intent: String, // What to change - pub suggested_patch: Option, // Optional diff snippet - pub validation: Vec, // Commands or checks to pass + pub intent: String, // What to change + pub candidate_patch: Option, // Optional diff snippet (proposal) + pub validation: Vec, // Commands or checks to pass } /// Location in source — handles both files and diffs @@ -672,7 +720,7 @@ pub enum DiffSide { } ``` -**Note**: No `FindingLevel`, no `Verdict`, no `NextStep`. Those belong in orchestration. RemediationPlan is pure data — orchestration decides whether to execute it. +**Note**: No `FindingLevel`, no `Verdict`, no `NextStep`. Those belong in orchestration. `CandidateRemediations` is pure data (proposals) — orchestration decides whether to execute them. --- @@ -703,7 +751,7 @@ pub struct ReviewCycleConfig { } pub enum IteratePolicy { - /// Iterate if any findings have remediation_hint + /// Iterate if any findings have candidate_fix OnRemediableFindings, /// Iterate on any findings (not just those with hints) OnAnyFindings, @@ -981,13 +1029,13 @@ pub fn build_implement_and_review( **What to build for V0**: - [ ] `lib/review/` crate with types: - `Artifact`, `Criteria`, `Check` - - `Finding` (hash-based `id`, `source` provenance, `remediation_hint`), `Location` (with `DiffSide`) - - `ReviewOutput`, `RemediationPlan`, `RemediationTask` + - `Finding` (hash-based `id`, `issue_key`, `candidate_fix`), `Location` (with `DiffSide`) + - `ReviewOutput`, `ReviewBundle`, `CandidateRemediations`, `CandidateTask` - [ ] `ReviewOps::PrepareReviewPrompt` — assemble prompt from artifact + criteria - [ ] `ReviewOps::ParseReviewResponse` — parse LLM response into findings + remediation plan - [ ] Simple CLI: `gunbc review --diff HEAD~1` → prints JSON -**Note**: V0 does NOT include orchestration (Review Cycle) — just single-criteria review. RemediationPlan is output but not executed. +**Note**: V0 does NOT include orchestration (Review Cycle) — just single-criteria review. `CandidateRemediations` is output but not executed. **V0 explicitly does NOT include**: - Durability / checkpointing @@ -995,13 +1043,20 @@ pub fn build_implement_and_review( - Multi-turn conversations - Apply/commit commands +**V0 Crispness Checklist** (acceptance criteria for architecture alignment): +- [ ] **Structural**: The `ReviewPhase` subdag validates as "contains no high-risk transport ops" (recursively) +- [ ] **Behavioral**: DryRun can run the entire V0 graph without touching the repo +- [ ] **Output contract**: Review output is machine-parseable JSON with schema version +- [ ] **Provenance**: Each finding indicates `(check_id, location?, observation)` and optional `candidate_fix` +- [ ] **Stable IDs**: Findings have stable-ish IDs via `issue_key`; merging is possible + --- ### V1: Review Cycle + Codex Integration After V0 works: - [ ] Add Review Cycle orchestration (configurable criteria) -- [ ] For findings with `suggested_fix`, feed back into Codex +- [ ] For findings with `candidate_fix`, feed back into Codex - [ ] ImplementationPhase runs Codex CLI (Query), outputs patch artifact - [ ] ReviewPhase reviews the patch with same criteria - [ ] Iterate until no findings or max iterations @@ -1017,7 +1072,7 @@ After V0 works: - [ ] Define types: - `Artifact`, `Criteria`, `Check` - `Finding` (hash-based id, `source` provenance, `remediation_hint`), `Location` (with `DiffSide`) - - `ReviewOutput`, `RemediationPlan`, `RemediationTask` + - `ReviewOutput`, `ReviewBundle`, `CandidateRemediations`, `CandidateTask` - [ ] Define orchestration types: `ReviewCycleConfig`, `IteratePolicy`, `CycleResult`, `CycleOutcome` - [ ] Implement `ReviewOps` with `Executable` trait - [ ] Wire up `ReviewPhaseBuilder` using existing patterns @@ -1092,11 +1147,13 @@ After V0 works: ## Appendix: Transport Classification Details -**Status**: Future consideration. Preserved here for reference when we have concrete use cases. +**Status**: Future consideration. This is an **ownership classification** model (who controls what's being read/written), not a security classification. Preserved here for reference when we have concrete use cases. **Background conversation**: The repo uses read/write as the primary discrimination axis. This is convenient for testing and reasoning about purity, but it's not structurally fundamental. Read/write isn't the real concern — we care more about *some* reads (secrets, credentials) than *some* writes (temp caches). The actual concern is **risk/interest**. We'll likely need domain-specific risk profiles. Writes can structure their own risk categories (e.g., "repo mutation" vs "cache update" vs "credential storage"). The current read/write split is a placeholder for that richer model. -### Query / Journal / Command Classification +**When to use this model**: If you need structural enforcement ("this subdag contains no Commands") beyond risk-level annotation. The main document uses fermi-style risk levels (low/medium/high/extreme) which are sufficient for V0/V1. + +### Query / Journal / Command Classification (Ownership Model) Transport classified by typed execute ops, not flags: From ea016c425b9117ffb263f51431aed7fa30deab05 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 1 Feb 2026 21:57:08 +0000 Subject: [PATCH 12/15] Consolidate design to ~450 word overview Preserve core concepts, defer details to implementation: - North star (pure dataflow, risk levels, action vs reasoning) - Two boundary types (transport vs interface) - Phase taxonomy with risk levels - ReviewPhase = reconciliation + candidate repairs - ImplementationPhase = produces patches, doesn't apply - V0/V1 scope with crispness checklist Detailed type definitions preserved in git history. https://claude.ai/code/session_01RZryjdBirzK2p6KVyGttju --- TODO/llm-code-review-pipeline.md | 1219 ++---------------------------- 1 file changed, 43 insertions(+), 1176 deletions(-) diff --git a/TODO/llm-code-review-pipeline.md b/TODO/llm-code-review-pipeline.md index b31defb7c86..a42e706151a 100644 --- a/TODO/llm-code-review-pipeline.md +++ b/TODO/llm-code-review-pipeline.md @@ -3,1209 +3,76 @@ **Status**: Design **Date**: 2026-02-01 -## Goal +## North Star -Build a fractal DAG-based development workflow with: -1. **ImplementationPhase** — Codex CLI session management with crash-resilient durability -2. **ReviewPhase** — General-purpose, reusable review process for any artifact type +gunbc workflows are pure dataflow graphs whose only interaction with the outside world occurs at explicit **transport nodes**. Transport operations are classified by **risk level** (low/medium/high/extreme) to enable static validation and DryRun interception. Higher-level workflows concentrate mutation in explicit **action phases** while keeping **reasoning phases** hermetic and replayable. -These phases compose into larger workflows (Requirements → Design → Implement → Review → Test → Commit) but are independently useful. +## Two Boundary Types ---- - -## Design Principles: Boundary Terminology & Transport Classification - -This section reconciles with AGENT.md ("Nodes are Pure, Boundaries are Structural") and SPEC.md (§2.7 "No side channels", §6 "Node Contract"). - -> **North Star**: gunbc workflows are pure dataflow graphs whose only interaction with the outside world occurs at explicit transport nodes. Transport operations are classified by risk level (low/medium/high/extreme) to enable static validation (e.g., "review contains no high-risk ops") and runtime interception (DryRun). Composition happens through interface boundaries (unconnected ports). Higher-level workflows are built by composing subdags, concentrating mutation inside explicit **action phases** while keeping **reasoning phases** hermetic and replayable. - -### Boundary Terminology (Two Distinct Concepts) - -gunbc has **two kinds of boundaries** that must not be conflated: - -| Term | Definition | How Detected | Purpose | -|------|------------|--------------|---------| -| **Transport Boundary** | Node that executes `TransportOps::Execute*` | By op type | Where I/O happens; DryRun intercepts here | -| **Interface Boundary** | Unconnected output port | By graph structure | DAG composition interface | - -``` -┌─────────────────────────────────────────────────────────────────────┐ -│ Example DAG │ -│ │ -│ ┌─────────────┐ ┌─────────────┐ ┌─────────────┐ │ -│ │ Prepare │────▶│ Execute │────▶│ Parse │ │ -│ │ (pure) │ │ (TRANSPORT │ │ (pure) │ │ -│ └─────────────┘ │ BOUNDARY) │ └──────┬──────┘ │ -│ └─────────────┘ │ │ -│ ▼ │ -│ ┌───────────┐ │ -│ │ output │ (INTERFACE │ -│ │ (unconnected) BOUNDARY)│ -│ └───────────┘ │ -└─────────────────────────────────────────────────────────────────────┘ -``` - -### Transport Classification - -**The honest framing**: Read/write isn't the real concern. We care more about *some* reads (secrets, credentials) than *some* writes (temp caches). The actual concern is **risk/interest** — what do we need to track for testing, isolation, and safety? - -**Ownership vs Security**: The appendix describes a Query/Journal/Command model. That's an **ownership classification** (who controls what's being read/written), not a security classification. A "Query" can still exfiltrate secrets; a "Journal" can still persist sensitive data; a "Query-like" CLI command can execute arbitrary code. - -**For V0/V1**: Use fermi-style risk categories (low/medium/high/extreme) rather than structural read/write classification. Risk is assessed per-transport based on domain and scope, not just ownership. - -**Future-proofing**: Even if not enforced in V0, transport ops should carry metadata for later policy: - -```rust -/// Metadata for risk assessment (V1+) -pub struct TransportMeta { - pub risk: RiskLevel, // Low | Medium | High | Extreme - pub domain: TransportDomain, // Git | Cargo | LLM | FS | HTTP | ... - pub scope: TransportScope, // RepoPath(...) | ToolHome(...) | NetworkHost(...) -} - -pub enum RiskLevel { Low, Medium, High, Extreme } -``` - -Later "risk profiles" become a policy over `(risk, domain, scope)` instead of invasive refactoring. - -**See**: [Appendix: Transport Classification Details](#appendix-transport-classification-details) for the full Query/Journal/Command ownership model if needed later. - -### Scope Purity: SubDag Encapsulation - -A SubDag is **hermetic** — its internal nodes cannot reach outside: - -``` -SPEC.md §2.7: "An opaque node's observable effects on the graph are -limited to its declared output ports. Any communication between nodes -that bypasses edges is a contract violation." -``` - -Applied to SubDags: - -1. **Inputs**: SubDag internals see only what's passed through entrypoints -2. **Outputs**: Results leave only through declared interface boundaries -3. **No leakage**: Inner nodes cannot reference sibling nodes in parent DAG -4. **Resolution order**: SubDag must fully resolve before parent sees outputs - -``` -┌─────────────────────── Parent DAG ───────────────────────────┐ -│ │ -│ ┌─────────┐ ┌─────────────────────────────┐ │ -│ │ NodeA │────────▶│ SubDag (Review) │ │ -│ └─────────┘ in │ │ out │ -│ ──────▶│ [inner nodes isolated] │───────▶│ -│ ┌─────────┐ │ │ │ -│ │ NodeB │ ✗ │ Cannot see NodeA or NodeB │ │ -│ └─────────┘ ──────▶│ directly, only via inputs │ │ -│ └─────────────────────────────┘ │ -└───────────────────────────────────────────────────────────────┘ -``` - -**Implication for ReviewPhase**: The parent doesn't know ReviewPhase uses an LLM internally. It only sees: `(artifact, criteria) → findings`. - -### Phase Taxonomy: Reasoning vs Action - -Phases are classified by whether they **decide** or **act**: - -| Phase | Type | Risk Level | Reasoning | -|-------|------|------------|-----------| -| **ReviewPhase** | Reasoning | Low | LLM calls observe code, don't mutate it. Output is data (findings), not action. | -| **ImplementationPhase** | Reasoning | Medium | Codex queries produce artifacts (patches). Durability writes are tool-owned. User artifacts are not mutated. | -| **TestPhase** | Reasoning | Medium | Running tests observes behavior. May write to tool-owned caches. | -| **ApplyPhase** | Action | High | File writes, patch application. Mutates user artifacts. | -| **CommitPhase** | Action | High | Git commit mutates the repository. | - -**The invariant**: High-risk mutation (Action Phases) only occurs in explicitly designated action subdags. Reasoning phases are hermetic and replayable — they produce *artifacts* (patches, plans, findings) but don't apply them. - -**Static validation**: A reasoning phase subdag can be verified to contain no high-risk transport ops. This makes "ReviewPhase is read-only" a structural property, not just a naming convention. - ---- - -## Part 1: Fractal Workflow Architecture - -### The Meta-Workflow - -A complete development cycle is a DAG of phases, where **ReviewPhase** appears multiple times: - -``` -┌─────────────────────────────────────────────────────────────────────────────┐ -│ DevWorkflow (Meta-DAG) │ -│ │ -│ ┌───────────────┐ ┌─────────────┐ ┌─────────────────────────────┐ │ -│ │ Requirements │───▶│ Design │───▶│ ImplementationPhase │ │ -│ │ (future) │ │ (future) │ │ (Codex session) │ │ -│ └───────────────┘ └──────┬──────┘ └──────────────┬──────────────┘ │ -│ │ │ │ -│ ▼ ▼ │ -│ ┌─────────────┐ ┌─────────────┐ │ -│ │ ReviewPhase │ │ ReviewPhase │ │ -│ │ (design) │ │ (code) │ │ -│ └─────────────┘ └──────┬──────┘ │ -│ │ │ -│ ▼ │ -│ ┌─────────────┐ │ -│ │ TestPhase │ │ -│ │ (cargo) │ │ -│ └──────┬──────┘ │ -│ │ │ -│ ▼ │ -│ ┌─────────────┐ │ -│ │ ReviewPhase │ │ -│ │ (tests) │ │ -│ └──────┬──────┘ │ -│ │ │ -│ ▼ │ -│ ┌─────────────┐ │ -│ │ CommitPhase │ │ -│ │ (git) │ │ -│ └─────────────┘ │ -└─────────────────────────────────────────────────────────────────────────────┘ -``` - -### Existing Primitives We Can Reuse - -| Primitive | Location | Purpose | -|-----------|----------|---------| -| `GitOps::PrepareDiff` | `lib/git-ops/` | Get diff for review context | -| `GitOps::PrepareCurrentBranch` | `lib/git-ops/` | Branch context | -| `CargoOp::test` | `lib/tools/cargo/` | Run tests | -| `CargoOp::check` | `lib/tools/cargo/` | Type checking | -| `LlmOps::PrepareChatRequest` | `lib/llm-ops/` | LLM invocation | -| `CliToolOp::Run` | `core/ir/src/transport/cli.rs` | CLI execution | -| `UpsertBuilder` | `core/ir/src/patterns/` | Idempotent operations | -| `WhileBuilder` | `core/ir/src/patterns/` | Loop with state | -| `BranchBuilder` | `core/ir/src/patterns/` | Conditional paths | - -### Focus: Implementation + Review - -For this iteration, we're building: -1. **ImplementationPhase** — Manage a Codex CLI session to generate/modify code -2. **ReviewPhase** — General-purpose review that works on any artifact - ---- - -## Part 2: ImplementationPhase (Codex Session) - -### Design Goals - -1. **Durability**: Survive crashes, resume seamlessly -2. **No implicit state**: All dependencies are explicit as either (a) DAG edges, or (b) declared transport accesses (including journal/checkpoint reads) -3. **Idempotency**: Re-running from start skips completed work -4. **Reasoning/Action separation**: Codex invocations produce artifacts (patches); applying them is a separate Action Phase - -**Codex invariant**: ImplementationPhase must invoke Codex in a mode that **cannot mutate user artifacts**. Any file changes must be emitted as artifacts (patches) and applied only by ApplyPhase. (Enforce via sandbox, read-only mount, or Codex flags that output patches without applying.) - -### The Challenge: Codex is Stateful - -Codex CLI maintains conversation state. Our challenge is to: -- Model this state explicitly in the DAG -- Persist it durably for crash recovery -- Resume without duplicating work or losing context - -### Reasoning/Action Boundary in Implementation +| Term | Definition | Purpose | +|------|------------|---------| +| **Transport Boundary** | Node executing `TransportOps::Execute*` | Where I/O happens; DryRun intercepts here | +| **Interface Boundary** | Unconnected output port | DAG composition interface | -``` -┌─────────────────────────────────────────────────────────────────────┐ -│ ImplementationPhase (Reasoning) │ -│ │ -│ ┌───────────────────────────────────────────────────────────────┐ │ -│ │ REASONING ZONE (Low/Medium Risk) │ │ -│ │ │ │ -│ │ GatherContext ──▶ CodexInvoke ──▶ ParseResponse ──▶ Decide │ │ -│ │ (file reads) (LLM call) (pure) (pure) │ │ -│ │ │ │ -│ │ Output: Artifacts (proposed changes, not yet applied) │ │ -│ └───────────────────────────────────────────────────────────────┘ │ -│ │ │ -│ ▼ │ -│ ┌───────────────────────────────────────────────────────────────┐ │ -│ │ ACTION ZONE (High Risk) — SEPARATE PHASE │ │ -│ │ │ │ -│ │ ApplyArtifacts ──▶ WriteFiles (ApplyPhase, not here) │ │ -│ │ (explicit action phase, after reasoning completes) │ │ -│ └───────────────────────────────────────────────────────────────┘ │ -└─────────────────────────────────────────────────────────────────────┘ -``` - -The key insight: **Codex invocation is Reasoning** (asking "what should I do?" → produces patch artifacts). **Applying the changes is Action** (doing it → separate phase). This separation means we can: -- Test the reasoning without mutating files -- Review proposed changes before applying -- Roll back decisions without undoing file mutations -- DryRun the entire reasoning phase with no side effects - -### Session State Model - -```rust -/// Complete state of a Codex implementation session -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct ImplementationSession { - /// Unique session identity (UUID/random) — for parallel sessions of same task - pub session_id: String, - - /// Content hash of task inputs — for dedup/resume matching - pub task_id: String, - - /// The task being implemented - pub task: ImplementationTask, - - /// Completed steps (for idempotent resume) - pub completed_steps: Vec, - - /// Current Codex conversation state - pub conversation: ConversationState, - - /// Files created/modified by this session - pub artifacts: Vec, - - /// Session lifecycle - pub status: SessionStatus, -} - -// session_id vs task_id: -// - session_id: random UUID (identity) — allows parallel sessions for same task -// - task_id: content hash (dedup key) — identifies "same task" for resume/lookup -// This prevents: (a) collision when two developers run same task, (b) accidental -// resume of unrelated session, (c) inability to have parallel experiments - -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct ImplementationTask { - pub description: String, - pub context_files: Vec, // Files to read for context - pub target_files: Vec, // Files to create/modify - pub constraints: Vec, // Requirements to satisfy -} - -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct StepRecord { - pub step_id: String, // Content hash of step inputs - pub step_type: StepType, - pub completed_at: u64, // Unix timestamp - pub outputs: HashMap, // Step outputs for downstream - pub cache_policy: CachePolicy, // When to rerun this step -} - -#[derive(Debug, Clone, Serialize, Deserialize)] -pub enum StepType { - GatherContext, - CodexInvocation { turn: u32 }, - ReviewInvocation, - /// Prepare patch/plan (Query-only) — actual apply is a Command phase outside - PrepareArtifacts, - Checkpoint, -} +## Phase Taxonomy -/// LLMs are nondeterministic — need explicit cache policy -#[derive(Debug, Clone, Serialize, Deserialize)] -pub enum CachePolicy { - /// Never rerun if step_id exists (crash recovery) - Immutable, - /// Always rerun, ignore cache - Refresh, - /// Rerun if cached result is older than duration - RefreshIfOlderThan(u64), // millis - /// Rerun if model version changed - RefreshIfModelChanged { model_fingerprint: String }, -} +| Phase | Type | Risk | Notes | +|-------|------|------|-------| +| **ReviewPhase** | Reasoning | Low | Produces findings, not mutations | +| **ImplementationPhase** | Reasoning | Medium | Produces patch artifacts, doesn't apply them | +| **TestPhase** | Reasoning | Medium | Observes behavior | +| **ApplyPhase** | Action | High | Mutates user artifacts | +| **CommitPhase** | Action | High | Mutates repository | -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct ConversationState { - /// Use Codex's native session ID — don't duplicate conversation state - /// Resume via: `codex resume ` or `codex exec resume ` - pub codex_session_id: Option, - pub mode: CodexMode, - pub turn_count: u32, - pub last_prompt_hash: Option, -} - -#[derive(Debug, Clone, Serialize, Deserialize)] -pub enum CodexMode { - Interactive, // `codex` — human-in-the-loop - Exec, // `codex exec` — non-interactive -} - -#[derive(Debug, Clone, Serialize, Deserialize)] -pub enum SessionStatus { - InProgress, - AwaitingReview, - Completed, - Failed { error: String }, -} -``` - -### Durability Strategy: Step-Level Checkpointing - -Instead of checkpointing the entire session, we checkpoint **per step**: - -``` -┌─────────────────────────────────────────────────────────────────────────────┐ -│ ImplementationPhase (Durable) │ -│ │ -│ For each step: │ -│ ┌─────────────────────────────────────────────────────────────────────┐ │ -│ │ StepUpsert Pattern │ │ -│ │ │ │ -│ │ ┌─────────────────┐ ┌─────────────────┐ │ │ -│ │ │ CheckStepDone │────▶│ Guard │ │ │ -│ │ │ (read storage) │ │ (skip if done) │ │ │ -│ │ └─────────────────┘ └────────┬────────┘ │ │ -│ │ │ │ │ -│ │ ┌─────────▼─────────┐ │ │ -│ │ │ ExecuteStep │ │ │ -│ │ │ (actual work) │ │ │ -│ │ └─────────┬─────────┘ │ │ -│ │ │ │ │ -│ │ ┌─────────▼─────────┐ │ │ -│ │ │ SaveStepResult │ │ │ -│ │ │ (write storage) │ │ │ -│ │ └───────────────────┘ │ │ -│ └─────────────────────────────────────────────────────────────────────┘ │ -│ │ -│ Steps execute in sequence, each wrapped in StepUpsert: │ -│ │ -│ 1. GatherContext ──▶ 2. CodexInvoke ──▶ 3. ApplyChanges ──▶ 4. Repeat? │ -│ │ │ -│ └─── (loop until complete or max turns) │ -└─────────────────────────────────────────────────────────────────────────────┘ -``` - -### Step ID: Content-Addressed - -Each step has a deterministic ID based on its inputs: - -```rust -fn step_id(step_type: &StepType, inputs: &HashMap) -> String { - let mut hasher = blake3::Hasher::new(); - hasher.update(&serde_json::to_vec(step_type).unwrap()); - - // Sort keys for determinism - let mut keys: Vec<_> = inputs.keys().collect(); - keys.sort(); - for key in keys { - hasher.update(key.as_bytes()); - hasher.update(&serde_json::to_vec(&inputs[key]).unwrap()); - } - - hasher.finalize().to_hex()[..16].to_string() -} -``` - -**Implications**: -- Same inputs → same step ID → skip if already done -- Changed inputs (e.g., new context) → new step ID → re-execute -- No manual invalidation needed - -**World state inputs**: A step is cacheable only relative to a declared **input snapshot** of the world state it depends on. Step inputs should include: -- `GatherContext`: `git HEAD` (or diff hash) + list of files read -- `CargoOp::test`: tool version (cargo + rustc), relevant env vars, manifest path -- LLM steps: model name + fingerprint, system prompt hash, temperature/seed -Otherwise caching becomes incorrect when the repo or environment changes. - -### Crash Recovery Flow - -``` -┌──────────────────────────────────────────────────────────────────────────┐ -│ Resume After Crash │ -│ │ -│ 1. Load session manifest from ~/.gunbc/sessions/{session_id}.json │ -│ │ -│ 2. For each step in the workflow: │ -│ ┌────────────────────────────────────────────────────────────────┐ │ -│ │ step_id = compute_step_id(step_type, current_inputs) │ │ -│ │ │ │ -│ │ if session.completed_steps.contains(step_id): │ │ -│ │ # Step already done │ │ -│ │ outputs = load_step_outputs(step_id) │ │ -│ │ feed outputs to downstream steps │ │ -│ │ else: │ │ -│ │ # Step not done (or inputs changed) │ │ -│ │ outputs = execute_step(step_type, current_inputs) │ │ -│ │ save_step_result(step_id, outputs) │ │ -│ │ update session.completed_steps │ │ -│ └────────────────────────────────────────────────────────────────┘ │ -│ │ -│ 3. Continue from where we left off — no duplicate LLM calls │ -└──────────────────────────────────────────────────────────────────────────┘ -``` - -### Storage Layout - -``` -~/.gunbc/ -├── sessions/ -│ └── impl-{session_id}/ -│ ├── manifest.json # ImplementationSession -│ ├── steps/ -│ │ ├── {step_id}.json # StepRecord with outputs -│ │ └── ... -│ └── artifacts/ -│ ├── {file_path}.patch # Changes to apply -│ └── ... -├── cache/ -│ └── codex/ -│ └── context-{hash}.json # Cached context gathering -└── config/ - └── codex.toml -``` - -### ImplementationOps - -```rust -#[derive(Debug, Clone)] -pub enum ImplementationOps { - /// Initialize or resume a session - /// - /// Inputs: - /// - `task`: ImplementationTask - /// - `storage_path`: String - /// - /// Outputs: - /// - `session`: ImplementationSession - /// - `is_resume`: Bool - InitSession, - - /// Check if a step is already complete - /// - /// Inputs: - /// - `session`: ImplementationSession - /// - `step_type`: StepType - /// - `step_inputs`: HashMap - /// - /// Outputs: - /// - `done`: Bool - /// - `cached_outputs`: Option> - CheckStepDone, - - /// Record step completion - /// - /// Inputs: - /// - `session`: ImplementationSession - /// - `step_id`: String - /// - `step_type`: StepType - /// - `outputs`: HashMap - /// - /// Outputs: - /// - `session`: ImplementationSession (updated) - RecordStep, - - /// Prepare Codex CLI invocation - /// - /// Inputs: - /// - `conversation`: ConversationState - /// - `prompt`: String - /// - `config`: CodexConfig - /// - /// Outputs: - /// - `request`: ShellRequest - PrepareCodexRequest, - - /// Parse Codex CLI response - /// - /// Inputs: - /// - `stdout`: String - /// - `stderr`: String - /// - `exit_code`: Int - /// - `conversation`: ConversationState - /// - /// Outputs: - /// - `response`: CodexResponse - /// - `conversation`: ConversationState (updated) - /// - `complete`: Bool - ParseCodexResponse, - - /// Extract file changes from Codex response - /// - /// Inputs: - /// - `response`: CodexResponse - /// - /// Outputs: - /// - `artifacts`: Vec - /// - `needs_followup`: Bool - ExtractArtifacts, -} -``` - -### CodexConfig - -```rust -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct CodexConfig { - /// Model to use (e.g., "gpt-4o", "o3") - pub model: String, - - /// Approval mode for file changes - pub approval_mode: ApprovalMode, - - /// Working directory for Codex - pub working_dir: Option, - - /// Additional CLI flags - pub extra_args: Vec, -} - -#[derive(Debug, Clone, Serialize, Deserialize)] -pub enum ApprovalMode { - /// Auto-approve all changes (for automation) - AutoApprove, - /// Require explicit approval (for interactive) - RequireApproval, - /// Auto-approve safe changes, prompt for risky ones - SafeAutoApprove, -} - -impl Default for CodexConfig { - fn default() -> Self { - Self { - model: "gpt-4o".into(), - approval_mode: ApprovalMode::SafeAutoApprove, - working_dir: None, - extra_args: vec![], - } - } -} -``` - ---- +**Invariant**: Reasoning phases are hermetic — they produce *artifacts* (patches, findings) but don't apply them. Action phases are explicit and separate. -## Part 3: ReviewPhase (Pure Reconciliation) +## Transport Risk Classification -### Design Principle: Review = Reconciliation + Candidate Repairs +Risk is assessed per-transport by domain and scope, not just read/write ownership. For V0/V1: fermi-style categories (low/medium/high/extreme). Future: `TransportMeta { risk, domain, scope }` for policy enforcement. -**ReviewPhase is maximally simple**: given an artifact and criteria, report where the artifact diverges from the criteria, and optionally propose candidate repairs. That's it. +**Ownership model** (Query/Journal/Command) is in appendix — useful for structural enforcement but not the primary classification axis. -- **No verdict** — that's orchestration -- **No severity levels** — either it diverges or it doesn't -- **No decisions** — that's orchestration (whether to apply, iterate, block) -- **Candidate repairs, not commands** — proposals are data; orchestration chooses whether to apply +## ReviewPhase: Reconciliation + Candidate Repairs ``` ReviewPhase: (artifact, criteria) → (findings, candidate_remediations) ``` -The reviewer doesn't decide what happens with findings or whether repairs are applied. Different criteria produce different findings. The orchestration layer decides what to do. +- **No verdict, no decisions** — orchestration decides what to do with findings +- **Candidate repairs** are proposals, not commands +- `Finding` has stable `issue_key` for cross-iteration matching (not observation text) +- `ReviewBundle` merges multiple sources (LLM + clippy + cargo check) -### Core Types +## ImplementationPhase: Produces Patches -```rust -/// What is being reviewed -#[derive(Debug, Clone, Serialize, Deserialize)] -pub enum Artifact { - /// Source code (diff or full file) - Code { - content: String, - file_path: Option, - language: Option, - is_diff: bool, - }, - /// Design document - Design { - content: String, - format: DocFormat, - }, - /// Test results - TestOutput { - stdout: String, - stderr: String, - exit_code: i32, - }, - /// Generic content - Text { - content: String, - artifact_type: String, - }, -} +**Codex invariant**: Must invoke Codex in a mode that **cannot mutate user artifacts**. Changes emitted as patch artifacts, applied only by ApplyPhase. -/// Criteria = what to check for -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct Criteria { - pub name: String, // e.g., "coherence", "quality", "security" - pub description: String, // What this criteria is about - pub checks: Vec, // Specific things to verify -} +- `session_id` (UUID) for identity, `task_id` (content hash) for dedup +- Step caching requires **world state inputs** (git HEAD, tool versions, model fingerprint) -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct Check { - pub id: String, // Unique identifier - pub question: String, // What to ask about the artifact - pub examples: Vec, // Example violations (optional) -} +## V0 Scope -/// Review output = pure reconciliation + candidate repairs -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct ReviewOutput { - pub criteria_name: String, // Which criteria was applied - pub source: String, // Provenance: "llm:gpt-4o", "cargo:clippy", etc. - pub findings: Vec, // Where artifact diverges from criteria - pub candidate_remediations: Option, // Optional repair proposals - pub summary: String, // Natural language summary -} +**Goal**: Review current branch diff → JSON report with findings -/// Container for merging multiple reviews (same artifact, different criteria/sources) -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct ReviewBundle { - pub artifact_id: String, // Hash of artifact being reviewed - pub reviews: Vec, // Each has criteria_name + source -} - -/// Finding = divergence from criteria -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct Finding { - /// Stable identity for merging/dedup (hash of canonical fields) - pub id: String, - pub check_id: String, // Which Check this relates to - pub location: Option, - /// Stable canonical key for cross-iteration matching (reviewer-provided) - /// Should NOT include line numbers; should include symbol/file/invariant - pub issue_key: String, - pub observation: String, // What was found (may vary in wording) - /// Example way to satisfy criteria (candidate, not command) - pub candidate_fix: Option, -} - -/// Stable finding ID = hash of (check_id, location context, issue_key) -/// Using issue_key instead of observation text for stability across LLM wording changes -fn finding_id(check_id: &str, location: &Option, issue_key: &str) -> String { - let mut hasher = blake3::Hasher::new(); - hasher.update(check_id.as_bytes()); - if let Some(loc) = location { - // For diffs: include hunk context, not line numbers - hasher.update(&serde_json::to_vec(loc).unwrap()); - } - hasher.update(issue_key.as_bytes()); - hasher.finalize().to_hex()[..16].to_string() -} - -/// Candidate repairs (proposals, not commands) -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct CandidateRemediations { - pub goals: Vec, // Acceptance criteria - pub tasks: Vec, // Proposed edits - pub constraints: Vec, // "do not change public API", etc. -} - -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct CandidateTask { - pub finding_id: String, // Links to Finding.id - pub file: Option, - pub intent: String, // What to change - pub candidate_patch: Option, // Optional diff snippet (proposal) - pub validation: Vec, // Commands or checks to pass -} - -/// Location in source — handles both files and diffs -#[derive(Debug, Clone, Serialize, Deserialize)] -pub enum Location { - /// Position in a file - FileLine { - file: String, - line: u32, - }, - /// Span in a file - Span { - file: String, - start: u32, - end: u32, - }, - /// Position in a diff (need to distinguish old vs new) - DiffLine { - file: String, - line: u32, - side: DiffSide, - }, -} - -#[derive(Debug, Clone, Serialize, Deserialize)] -pub enum DiffSide { - Old, // Line in the removed version - New, // Line in the added version -} -``` - -**Note**: No `FindingLevel`, no `Verdict`, no `NextStep`. Those belong in orchestration. `CandidateRemediations` is pure data (proposals) — orchestration decides whether to execute them. - ---- - -## Part 4: Review Cycle (Orchestration) - -### What Orchestration Does - -ReviewPhase is pure reconciliation. The **Review Cycle** is orchestration that: -- Decides which criteria to run (fully configurable) -- Decides whether to iterate on findings -- Decides when to stop - -No hardcoded stages or order — criteria are user-defined. - -### Orchestration Types - -```rust -/// Orchestration decides what to do with findings -pub struct ReviewCycleConfig { - /// Which criteria to apply (order is configurable, not mandated) - pub criteria: Vec, - - /// How to decide whether to iterate - pub iterate_policy: IteratePolicy, - - /// Maximum iterations per criteria - pub max_iterations: u32, -} - -pub enum IteratePolicy { - /// Iterate if any findings have candidate_fix - OnRemediableFindings, - /// Iterate on any findings (not just those with hints) - OnAnyFindings, - /// Never auto-iterate, always proceed - AlwaysProceed, - /// Custom logic - Custom(fn(&[Finding]) -> bool), -} - -/// What the orchestration layer produces -pub struct CycleResult { - /// All findings from all criteria runs - pub all_findings: Vec<(String, Vec)>, // (criteria_name, findings) - - /// Final decision - pub outcome: CycleOutcome, -} - -pub enum CycleOutcome { - /// All criteria passed (no findings, or findings were fixed) - Complete, - /// Stopped iterating (max iterations or no more fixes) - Stabilized { remaining_findings: usize }, - /// Needs human decision - NeedsHuman { reason: String }, -} -``` - -### Why This Separation Matters - -1. **ReviewPhase stays pure**: `(artifact, criteria) → findings` — nothing more -2. **Criteria are user-defined**: security, style, correctness — whatever you configure -3. **Orchestration is explicit**: decisions about iterating, blocking, etc. are visible -4. **Order is flexible**: sequential usually makes sense (hard to discuss design when basic correctness fails), but not enforced - -### ReviewOps - -```rust -#[derive(Debug, Clone)] -pub enum ReviewOps { - /// Build review prompt from artifact + criteria - /// - /// Inputs: - /// - `artifact`: Artifact - /// - `criteria`: Criteria - /// - `context`: Option - /// - /// Outputs: - /// - `prompt`: String - /// - `system_prompt`: String - PrepareReviewPrompt, - - /// Parse LLM response into structured findings - /// - /// Inputs: - /// - `response`: String (LLM output) - /// - `criteria`: Criteria (for check_id mapping) - /// - /// Outputs: - /// - `output`: ReviewOutput - ParseReviewResponse, - - /// Merge findings from multiple review sources - /// - /// Inputs: - /// - `outputs`: Vec - /// - /// Outputs: - /// - `merged`: ReviewOutput - MergeOutputs, -} -``` - -**Note**: No `DetermineVerdict` in ReviewOps — that's orchestration (see Part 4). - -### ReviewPhase DAG - -``` -┌─────────────────────────────────────────────────────────────────────────────┐ -│ ReviewPhase SubDag │ -│ │ -│ NOTE: Reuses lib/llm-ops chat subdag — don't re-implement the LLM chain! │ -│ │ -│ Entrypoints: │ -│ ┌──────────┐ ┌──────────┐ ┌──────────┐ ┌──────────┐ │ -│ │ artifact │ │ criteria │ │ context │ │ config │ │ -│ └────┬─────┘ └────┬─────┘ └────┬─────┘ └────┬─────┘ │ -│ │ │ │ │ │ -│ └────────────┴────────────┴────────────┘ │ -│ │ │ -│ ▼ │ -│ ┌─────────────────────────┐ │ -│ │ PrepareReviewPrompt │ ◀── PURE: build LLM prompt │ -│ │ (ReviewOps) │ │ -│ └────────────┬────────────┘ │ -│ │ │ -│ ▼ │ -│ ┌─────────────────────────────────────────────────┐ │ -│ │ EMBED: lib/llm-ops chat_completion subdag │ │ -│ │ ┌─────────────────────────────────────────┐ │ │ -│ │ │ PrepareChatRequest → ExecuteQuery → │ │ │ -│ │ │ ParseChatResponse │ │ │ -│ │ └─────────────────────────────────────────┘ │ │ -│ │ (reuse existing graph, don't redefine) │ │ -│ └────────────┬────────────────────────────────────┘ │ -│ │ │ -│ ▼ │ -│ ┌─────────────────────────┐ │ -│ │ ParseReviewResponse │ ◀── PURE: structure findings │ -│ │ (ReviewOps) │ │ -│ └────────────┬────────────┘ │ -│ │ │ -│ Workflow Boundaries: ▼ │ -│ ┌───────────────┐ ┌──────────┐ │ -│ │ findings │ │ summary │ │ -│ │ Vec │ │ String │ │ -│ └───────────────┘ └──────────┘ │ -│ │ -│ NOTE: No verdict, no next_step — that's orchestration (Part 4) │ -└─────────────────────────────────────────────────────────────────────────────┘ -``` - -### Multi-Source Review (Parallel) - -For comprehensive review, run multiple reviewers in parallel: - -``` -┌─────────────────────────────────────────────────────────────────────────────┐ -│ MultiReview (Parallel + Merge) │ -│ │ -│ ┌─────────────────────────────────────────────┐ │ -│ │ artifact + criteria │ │ -│ └─────────┬───────────────┬──────────────┬────┘ │ -│ │ │ │ │ -│ ┌─────────▼─────┐ ┌───────▼───────┐ ┌────▼────────┐ │ -│ │ LLM Review │ │ Clippy/Lint │ │ Type Check │ │ -│ │ (ReviewPhase) │ │ (CargoOp) │ │ (CargoOp) │ │ -│ └───────┬───────┘ └───────┬───────┘ └──────┬──────┘ │ -│ │ │ │ │ -│ └─────────────────┴────────────────┘ │ -│ │ │ -│ ┌──────────▼──────────┐ │ -│ │ MergeOutputs │ │ -│ │ (ReviewOps) │ │ -│ └──────────┬──────────┘ │ -│ │ │ -│ ┌───────▼───────┐ │ -│ │ unified output│ │ -│ └───────────────┘ │ -└─────────────────────────────────────────────────────────────────────────────┘ -``` - ---- - -## Part 5: Composition Example - -### Implement-and-Review Workflow - -Compose ImplementationPhase + Review Cycle (orchestration) into a complete workflow: - -``` -┌─────────────────────────────────────────────────────────────────────────────┐ -│ ImplementAndReview Workflow │ -│ │ -│ ┌─────────────────────────────────────────────────────────────────────┐ │ -│ │ ImplementationPhase │ │ -│ │ (Codex session with durability) │ │ -│ └─────────────────────────────┬───────────────────────────────────────┘ │ -│ │ │ -│ │ artifacts: Vec │ -│ ▼ │ -│ ┌─────────────────────────────────────────────────────────────────────┐ │ -│ │ Review Cycle (Orchestration) │ │ -│ │ criteria: [configured by caller] │ │ -│ │ │ │ -│ │ For each artifact, for each configured criteria: │ │ -│ │ ┌─────────────────────────────────────────────────────────────┐ │ │ -│ │ │ ReviewPhase │ │ │ -│ │ │ criteria: │ │ │ -│ │ │ outputs: (findings, summary) │ │ │ -│ │ └─────────────────────────────────────────────────────────────┘ │ │ -│ │ If findings → iterate (apply fix, re-review) or proceed │ │ -│ └─────────────────────────────┬───────────────────────────────────────┘ │ -│ │ │ -│ │ cycle_result: CycleResult │ -│ ▼ │ -│ ┌─────────────────────────────────────────────────────────────────────┐ │ -│ │ BranchBuilder │ │ -│ │ condition: cycle_result.outcome == Complete │ │ -│ │ │ │ -│ │ ┌─────────────────┐ ┌─────────────────┐ │ │ -│ │ │ True: Complete │ │ False: Stabilized│ │ │ -│ │ │ → apply changes │ │ → human decides │ │ │ -│ │ └─────────────────┘ └─────────────────┘ │ │ -│ └─────────────────────────────────────────────────────────────────────┘ │ -└─────────────────────────────────────────────────────────────────────────────┘ -``` - -### Builder API (Proposed) - -```rust -/// Build a complete implement-and-review workflow -pub fn build_implement_and_review( - task: ImplementationTask, - cycle_config: ReviewCycleConfig, - workflow_config: WorkflowConfig, -) -> Result, BuilderError> { - let mut builder = DagBuilder::new("implement-and-review"); - - // Phase 1: Implementation - let impl_phase = ImplementationPhaseBuilder::new() - .with_task(task) - .with_durability(workflow_config.storage_path.clone()) - .build()?; - - let impl_node = builder.add_subdag("implement", impl_phase)?; - - // Phase 2: Review cycle (orchestration handles iteration) - let review_cycle = ReviewCycleBuilder::new() - .with_criteria(cycle_config.criteria) - .with_iterate_policy(cycle_config.iterate_policy) - .with_max_iterations(cycle_config.max_iterations) - .build()?; - - let review_node = builder.add_subdag_after("review-cycle", review_cycle, &impl_node)?; - builder.connect(impl_node.output("artifacts"), review_node.input("artifacts"))?; - - // Phase 3: Handle cycle outcome - let check_node = builder.add_node_after( - "handle-outcome", - WorkflowOps::HandleCycleOutcome, - &review_node, - )?; - builder.connect(review_node.output("cycle_result"), check_node.input("result"))?; - - // Set boundaries - builder.mark_entrypoint("task", impl_node.input("task"))?; - builder.mark_boundary("result", check_node.output("final_result"))?; - - builder.build() -} ``` - ---- - -## Implementation Tasks - -### Phase 0: Core Architecture (if generalizing) - -- [ ] Consider adding risk categories to transport operations (fermi-style: low/medium/high/extreme) -- [ ] Consider updating AGENT.md with risk classification principle -- [ ] (Future) Consider Query/Command structural classification — see appendix - -### V0: Minimal Useful Pipeline (First Target) - -**Goal**: Review current branch diff + optional cargo checks → JSON report - -**What already exists** (reuse, don't rebuild): -- ✅ `GitOps::PrepareDiff` / `ParseDiff` — get diff context -- ✅ `LlmOps::PrepareChatRequest` / `ParseChatResponse` — LLM invocation -- ✅ `CargoOp::check` / `CargoOp::test` — cargo tooling -- ✅ `TransportOps::Execute` — I/O boundary -- ✅ Pattern builders: `UpsertBuilder`, `LoopBuilder`, `BranchBuilder` - -**V0 flow**: -``` -1. GitOps::PrepareDiff → Execute → GitOps::ParseDiff -2. (Optional) CargoOp::check/test/clippy -3. ReviewOps::PrepareReviewPrompt(diff + tool_outputs + criteria) -4. [lib/llm-ops chat subdag] ← reuse existing -5. ReviewOps::ParseReviewResponse -6. Output: JSON ReviewOutput with findings, summary +GitOps::PrepareDiff → Execute → ReviewOps::PreparePrompt → LLM → ParseResponse → JSON ``` -**What to build for V0**: -- [ ] `lib/review/` crate with types: - - `Artifact`, `Criteria`, `Check` - - `Finding` (hash-based `id`, `issue_key`, `candidate_fix`), `Location` (with `DiffSide`) - - `ReviewOutput`, `ReviewBundle`, `CandidateRemediations`, `CandidateTask` -- [ ] `ReviewOps::PrepareReviewPrompt` — assemble prompt from artifact + criteria -- [ ] `ReviewOps::ParseReviewResponse` — parse LLM response into findings + remediation plan -- [ ] Simple CLI: `gunbc review --diff HEAD~1` → prints JSON +**V0 Crispness Checklist**: +- [ ] ReviewPhase subdag contains no high-risk transport ops +- [ ] DryRun can run entire graph without touching repo +- [ ] Output is machine-parseable JSON with schema version +- [ ] Findings have stable IDs via `issue_key` -**Note**: V0 does NOT include orchestration (Review Cycle) — just single-criteria review. `CandidateRemediations` is output but not executed. +**V0 does NOT include**: durability, Codex integration, multi-turn, apply/commit. -**V0 explicitly does NOT include**: -- Durability / checkpointing -- Codex CLI integration -- Multi-turn conversations -- Apply/commit commands +## V1 Scope -**V0 Crispness Checklist** (acceptance criteria for architecture alignment): -- [ ] **Structural**: The `ReviewPhase` subdag validates as "contains no high-risk transport ops" (recursively) -- [ ] **Behavioral**: DryRun can run the entire V0 graph without touching the repo -- [ ] **Output contract**: Review output is machine-parseable JSON with schema version -- [ ] **Provenance**: Each finding indicates `(check_id, location?, observation)` and optional `candidate_fix` -- [ ] **Stable IDs**: Findings have stable-ish IDs via `issue_key`; merging is possible +- Review Cycle orchestration (configurable criteria, iteration) +- ImplementationPhase with Codex CLI integration +- Feed findings with `candidate_fix` back into Codex +- Keep "apply patch" as explicit Action Phase --- -### V1: Review Cycle + Codex Integration - -After V0 works: -- [ ] Add Review Cycle orchestration (configurable criteria) -- [ ] For findings with `candidate_fix`, feed back into Codex -- [ ] ImplementationPhase runs Codex CLI (Query), outputs patch artifact -- [ ] ReviewPhase reviews the patch with same criteria -- [ ] Iterate until no findings or max iterations -- [ ] **Keep "apply patch" as explicit Command** (separate from reasoning loop) - ---- - -### Full Implementation Phases - -#### Phase 1: Core Types (V0 blocker) - -- [ ] Create `lib/review/` crate -- [ ] Define types: - - `Artifact`, `Criteria`, `Check` - - `Finding` (hash-based id, `source` provenance, `remediation_hint`), `Location` (with `DiffSide`) - - `ReviewOutput`, `ReviewBundle`, `CandidateRemediations`, `CandidateTask` -- [ ] Define orchestration types: `ReviewCycleConfig`, `IteratePolicy`, `CycleResult`, `CycleOutcome` -- [ ] Implement `ReviewOps` with `Executable` trait -- [ ] Wire up `ReviewPhaseBuilder` using existing patterns - -#### Phase 2: Risk Classification (future) - -- [ ] Define fermi-style risk categories (low/medium/high/extreme) -- [ ] Add risk annotation to transport operations -- [ ] (Future) Consider Query/Journal/Command structural classification if needed — see appendix - -#### Phase 3: ImplementationPhase (V1) - -- [ ] Create `lib/codex/` crate -- [ ] Define types: `ImplementationSession`, `StepRecord`, `ConversationState`, `CodexMode` -- [ ] Add `CachePolicy` for nondeterministic LLM steps -- [ ] Implement `ImplementationOps` -- [ ] Integrate Codex CLI via `codex_session_id` + native resume (`codex resume `) -- [ ] Add Journal transport for session state persistence - -#### Phase 4: Durability (V1+) - -- [ ] Implement `ExecuteJournal` for checkpoint storage -- [ ] Implement `CheckStepDone` / `RecordStep` operations with `CachePolicy` support -- [ ] Add content-addressed step ID computation -- [ ] Test crash recovery - -#### Phase 5: Composition & CLI - -- [ ] Build `ImplementAndReviewBuilder` (composes V0 + V1) -- [ ] Add `gunbc review` command (V0) -- [ ] Add `gunbc implement` command (V1) -- [ ] Add `--resume` support - ---- - -## Open Questions - -1. ~~**Codex session files**~~: **Resolved** — Codex CLI has native session persistence: - - Interactive: `codex resume [SESSION_ID]` - - Non-interactive: `codex exec resume ` - - State stored under `CODEX_HOME` (default `~/.codex`) - - **Strategy**: Store `codex_session_id` as pointer, use native resume - -2. **Multi-file implementation**: When Codex generates changes across many files, should we review: - - All files together (single review)? - - Each file separately (parallel reviews)? - - Changed "modules" (heuristic grouping)? - -3. **Feedback loop**: When review finds issues, how do we feed findings back to Codex? - - Option A: Append findings to conversation, ask for fixes - - Option B: New session with findings as context - - Option C: Human-in-the-loop decides - -4. **Automated vs LLM review**: When should we use Clippy/lint vs LLM? - - Option A: Always run both in parallel, merge - - Option B: Clippy first, LLM for things Clippy can't catch - - Option C: Configurable per-rubric - ---- - -## Related Patterns - -- **Upsert**: `core/ir/src/patterns/upsert.rs` — idempotent operations -- **While/Retry**: `core/ir/src/patterns/repeat.rs` — iteration with state -- **Loop**: `core/ir/src/patterns/loop_pattern.rs` — per-item processing -- **Branch**: `core/ir/src/patterns/branch.rs` — conditional paths -- **CLI Tools**: `core/ir/src/transport/cli.rs` — CliToolDef, CliToolOp -- **LLM Ops**: `lib/llm-ops/src/` — existing chat completion patterns -- **Git Ops**: `lib/git-ops/src/` — diff, branch operations - ---- - -## Appendix: Transport Classification Details - -**Status**: Future consideration. This is an **ownership classification** model (who controls what's being read/written), not a security classification. Preserved here for reference when we have concrete use cases. - -**Background conversation**: The repo uses read/write as the primary discrimination axis. This is convenient for testing and reasoning about purity, but it's not structurally fundamental. Read/write isn't the real concern — we care more about *some* reads (secrets, credentials) than *some* writes (temp caches). The actual concern is **risk/interest**. We'll likely need domain-specific risk profiles. Writes can structure their own risk categories (e.g., "repo mutation" vs "cache update" vs "credential storage"). The current read/write split is a placeholder for that richer model. - -**When to use this model**: If you need structural enforcement ("this subdag contains no Commands") beyond risk-level annotation. The main document uses fermi-style risk levels (low/medium/high/extreme) which are sufficient for V0/V1. - -### Query / Journal / Command Classification (Ownership Model) - -Transport classified by typed execute ops, not flags: - -| Class | Execute Op | Scope | Examples | -|-------|------------|-------|----------| -| **Query** | `ExecuteQuery` | Read external state | LLM calls, file reads, git diff, HTTP GET | -| **Journal** | `ExecuteJournal` | Write tool-owned cache/state | Checkpoints, session state, response cache | -| **Command** | `ExecuteCommand` | Mutate user artifacts / external systems | File writes, git commit, apply patch | - -```rust -pub enum TransportOps { - ExecuteQuery, - ExecuteJournal, - ExecuteCommand, -} -``` - -### Journal Scope Contract - -Journal is "tool-owned state" — but without enforcement, it becomes "Command in disguise." - -```rust -pub struct JournalScope { - pub allowed_paths: Vec, // e.g., ["~/.gunbc/", "$TMPDIR/gunbc-"] - pub resource_ids: Vec, -} -``` - -Executor enforces: `ExecuteJournal` can only write within `allowed_paths`. - -### DryRun Modes - -```rust -pub enum DryRunMode { - Strict, // mock all - Safe { allow_journal: bool }, // allow Query, optionally Journal, block Command - Off, // real execution -} -``` - -| Mode | Query | Journal | Command | -|------|-------|---------|---------| -| **Strict** | mock | mock | mock | -| **Safe(journal=true)** | allow | allow | mock | -| **Safe(journal=false)** | allow | mock | mock | -| **Off** | allow | allow | allow | - -### Phase-level Validation (if using this model) - -| Phase | Validation Rule | -|-------|-----------------| -| **ReviewPhase** | No `ExecuteCommand`, no `ExecuteJournal` | -| **ImplementationPhase** | No `ExecuteCommand` | -| **TestPhase** | No `ExecuteCommand` | -| **ApplyPhase** | `ExecuteCommand` allowed | +*See git history for detailed type definitions and appendix with Query/Journal/Command ownership model.* From a7daa86af54b0b06a8a5e449c51ff861ea205165 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 1 Feb 2026 22:04:13 +0000 Subject: [PATCH 13/15] Clarify I/O and risk as orthogonal classification axes - I/O type (read/write): for purity, testing, DryRun mocking - Risk level (low/medium/high/extreme): for safety/interest These are orthogonal: cache write is low-risk but still a write. Credential read is high-risk but still a read. https://claude.ai/code/session_01RZryjdBirzK2p6KVyGttju --- TODO/llm-code-review-pipeline.md | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/TODO/llm-code-review-pipeline.md b/TODO/llm-code-review-pipeline.md index a42e706151a..c35b218d84f 100644 --- a/TODO/llm-code-review-pipeline.md +++ b/TODO/llm-code-review-pipeline.md @@ -26,11 +26,19 @@ gunbc workflows are pure dataflow graphs whose only interaction with the outside **Invariant**: Reasoning phases are hermetic — they produce *artifacts* (patches, findings) but don't apply them. Action phases are explicit and separate. -## Transport Risk Classification +## Transport Classification: Two Orthogonal Axes -Risk is assessed per-transport by domain and scope, not just read/write ownership. For V0/V1: fermi-style categories (low/medium/high/extreme). Future: `TransportMeta { risk, domain, scope }` for policy enforcement. +**I/O type** (read/write) — for purity, testing, DryRun mocking: +- **Read**: file reads, git diff, LLM calls, HTTP GET +- **Write**: file writes, git commit, cache updates -**Ownership model** (Query/Journal/Command) is in appendix — useful for structural enforcement but not the primary classification axis. +**Risk level** (fermi-style) — for safety/interest: +- **Low**: temp cache, tool-owned state +- **Medium**: LLM calls (cost, latency) +- **High**: repo mutation, external API calls +- **Extreme**: credential access, destructive ops + +These are orthogonal: a cache write is low-risk but still a write (matters for purity). A credential read is high-risk but still a read. DryRun intercepts by I/O type; policy enforcement uses risk level. ## ReviewPhase: Reconciliation + Candidate Repairs From 375226fda038236e9ce45238509803e5222d4f39 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 1 Feb 2026 22:08:13 +0000 Subject: [PATCH 14/15] Separate I/O (intrinsic) from risk (context-dependent) - I/O type (read/write): stable domain knowledge, core model - Risk modeling: separate concern, context-dependent (same write is low-risk in test, extreme in prod) Updated phase taxonomy to use I/O instead of risk levels. Risk modeling deferred until we have concrete use cases. https://claude.ai/code/session_01RZryjdBirzK2p6KVyGttju --- TODO/llm-code-review-pipeline.md | 33 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/TODO/llm-code-review-pipeline.md b/TODO/llm-code-review-pipeline.md index c35b218d84f..58cfeb54043 100644 --- a/TODO/llm-code-review-pipeline.md +++ b/TODO/llm-code-review-pipeline.md @@ -16,29 +16,28 @@ gunbc workflows are pure dataflow graphs whose only interaction with the outside ## Phase Taxonomy -| Phase | Type | Risk | Notes | -|-------|------|------|-------| -| **ReviewPhase** | Reasoning | Low | Produces findings, not mutations | -| **ImplementationPhase** | Reasoning | Medium | Produces patch artifacts, doesn't apply them | -| **TestPhase** | Reasoning | Medium | Observes behavior | -| **ApplyPhase** | Action | High | Mutates user artifacts | -| **CommitPhase** | Action | High | Mutates repository | +| Phase | Type | I/O | Notes | +|-------|------|-----|-------| +| **ReviewPhase** | Reasoning | Read-only | Produces findings, not mutations | +| **ImplementationPhase** | Reasoning | Read-only | Produces patch artifacts, doesn't apply them | +| **TestPhase** | Reasoning | Read-only | Observes behavior | +| **ApplyPhase** | Action | Write | Mutates user artifacts | +| **CommitPhase** | Action | Write | Mutates repository | -**Invariant**: Reasoning phases are hermetic — they produce *artifacts* (patches, findings) but don't apply them. Action phases are explicit and separate. +**Invariant**: Reasoning phases are read-only — they produce *artifacts* (patches, findings) but don't apply them. Action phases do writes and are explicit/separate. -## Transport Classification: Two Orthogonal Axes +## Transport Classification -**I/O type** (read/write) — for purity, testing, DryRun mocking: +**I/O type** (read/write) — intrinsic, stable domain knowledge: - **Read**: file reads, git diff, LLM calls, HTTP GET - **Write**: file writes, git commit, cache updates -**Risk level** (fermi-style) — for safety/interest: -- **Low**: temp cache, tool-owned state -- **Medium**: LLM calls (cost, latency) -- **High**: repo mutation, external API calls -- **Extreme**: credential access, destructive ops +This is the core model. DryRun intercepts by I/O type. Purity reasoning uses I/O type. -These are orthogonal: a cache write is low-risk but still a write (matters for purity). A credential read is high-risk but still a read. DryRun intercepts by I/O type; policy enforcement uses risk level. +**Risk modeling** — separate concern, context-dependent: +- Same write: low-risk in test, extreme-risk in prod +- Risk depends on environment, what's being accessed, policies +- Will model separately when we have concrete use cases ## ReviewPhase: Reconciliation + Candidate Repairs @@ -67,7 +66,7 @@ GitOps::PrepareDiff → Execute → ReviewOps::PreparePrompt → LLM → ParseRe ``` **V0 Crispness Checklist**: -- [ ] ReviewPhase subdag contains no high-risk transport ops +- [ ] ReviewPhase subdag is read-only (no write ops) - [ ] DryRun can run entire graph without touching repo - [ ] Output is machine-parseable JSON with schema version - [ ] Findings have stable IDs via `issue_key` From c23b49583fc69ae80e6cbb17e7ef81bbfd435b5e Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 1 Feb 2026 22:22:10 +0000 Subject: [PATCH 15/15] Add implementation plan with split-able TODOs Four sections for V0: 1. Core Types (lib/review/) - Artifact, Criteria, Finding, etc. 2. ReviewOps - PreparePrompt, ParseResponse, MergeOutputs 3. DAG Wiring - ReviewPhaseBuilder using existing primitives 4. CLI Integration - gunbc review command Each section can be tackled independently. https://claude.ai/code/session_01RZryjdBirzK2p6KVyGttju --- TODO/llm-code-review-pipeline.md | 100 ++++++++++++++++++++++++++++++- 1 file changed, 99 insertions(+), 1 deletion(-) diff --git a/TODO/llm-code-review-pipeline.md b/TODO/llm-code-review-pipeline.md index 58cfeb54043..f387803ff50 100644 --- a/TODO/llm-code-review-pipeline.md +++ b/TODO/llm-code-review-pipeline.md @@ -82,4 +82,102 @@ GitOps::PrepareDiff → Execute → ReviewOps::PreparePrompt → LLM → ParseRe --- -*See git history for detailed type definitions and appendix with Query/Journal/Command ownership model.* +## Implementation Plan + +### Section 1: Core Types (`lib/review/`) + +Create `lib/review/` crate with domain types. + +**TODO 1.1: Artifact types** +- [ ] `Artifact` enum (Code, Design, TestOutput, Text variants) +- [ ] `DocFormat` enum for design docs + +**TODO 1.2: Criteria types** +- [ ] `Criteria` struct (name, description, checks) +- [ ] `Check` struct (id, question, examples) + +**TODO 1.3: Finding types** +- [ ] `Finding` struct (id, check_id, location, issue_key, observation, candidate_fix) +- [ ] `Location` enum (FileLine, Span, DiffLine with DiffSide) +- [ ] `finding_id()` hash function using issue_key + +**TODO 1.4: Output types** +- [ ] `ReviewOutput` struct (criteria_name, source, findings, candidate_remediations, summary) +- [ ] `ReviewBundle` struct for multi-source merging +- [ ] `CandidateRemediations` struct (goals, tasks, constraints) +- [ ] `CandidateTask` struct (finding_id, file, intent, candidate_patch, validation) + +**TODO 1.5: JSON schema** +- [ ] Derive Serialize/Deserialize for all types +- [ ] Add schema version field to ReviewOutput +- [ ] Basic validation tests + +--- + +### Section 2: ReviewOps (`lib/review/ops.rs`) + +Implement `ReviewOps` enum with `Executable` trait. + +**TODO 2.1: PrepareReviewPrompt** +- [ ] `ReviewOps::PrepareReviewPrompt` variant +- [ ] Input: artifact, criteria, optional context +- [ ] Output: prompt string, system_prompt string +- [ ] Prompt template that instructs LLM to output structured findings + +**TODO 2.2: ParseReviewResponse** +- [ ] `ReviewOps::ParseReviewResponse` variant +- [ ] Input: LLM response string, criteria (for check_id mapping) +- [ ] Output: `ReviewOutput` +- [ ] Parse JSON from LLM response, generate finding IDs + +**TODO 2.3: MergeOutputs** +- [ ] `ReviewOps::MergeOutputs` variant +- [ ] Input: `Vec` +- [ ] Output: `ReviewBundle` +- [ ] Dedup findings by id + +--- + +### Section 3: DAG Wiring (`lib/review/dag.rs`) + +Wire up ReviewPhase subdag using existing primitives. + +**TODO 3.1: Review existing primitives** +- [ ] Understand `GitOps::PrepareDiff` / `ParseDiff` in `lib/git-ops/` +- [ ] Understand `LlmOps::PrepareChatRequest` / `ParseChatResponse` in `lib/llm-ops/` +- [ ] Understand `TransportOps::Execute` pattern + +**TODO 3.2: ReviewPhase builder** +- [ ] `ReviewPhaseBuilder` struct +- [ ] Entrypoints: artifact, criteria, config +- [ ] Internal flow: PreparePrompt → LLM subdag → ParseResponse +- [ ] Interface boundaries: findings, summary + +**TODO 3.3: DiffReview convenience builder** +- [ ] Combines GitOps::PrepareDiff → ReviewPhase +- [ ] Input: git ref (e.g., "HEAD~1"), criteria +- [ ] Output: ReviewOutput as JSON + +--- + +### Section 4: CLI Integration + +Add `gunbc review` command. + +**TODO 4.1: CLI command** +- [ ] `gunbc review --diff ` subcommand +- [ ] Load criteria from config or use default +- [ ] Output JSON to stdout + +**TODO 4.2: Default criteria** +- [ ] Ship basic "code review" criteria +- [ ] Configurable via `~/.gunbc/review.toml` or similar + +**TODO 4.3: Integration test** +- [ ] Test against sample diff +- [ ] Verify JSON output schema +- [ ] DryRun mode test (no actual LLM calls) + +--- + +*See git history for detailed type definitions from earlier design iterations.*