Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
201 changes: 201 additions & 0 deletions TODO/design-typed-mock-builder.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,201 @@
# Typed Mock Builder: Impossible by Construction

**Status:** Implemented ✅
**Depends on:** Phase 8 (done), Phase 9 concepts

## Implementation Summary

The typed mock builder pattern has been implemented and the first tool (gist) has been migrated.

**Core files:**
- `core/test/src/mock_requirements.rs` - MockRequirements type and extract_mock_requirements()
- `lib/tools/gist/src/graph_mock.rs` - First migrated MockSpec

## Problem (Solved)

Current mock specification was disconnected from DAG definition:

```rust
// DAG defines ports with types
let dag = build_gist_dag(); // execute_gist.url: String, execute_gist.response: Json

// MockSpec defined separately - can have wrong types or miss ports
let spec = MockSpec::new("gist")
.boundary("execute_gist", "url", Value::Int(42)) // WRONG TYPE
// forgot execute_gist.response - not caught until testgen
```

**Solution:** `extract_mock_requirements()` analyzes the DAG and creates typed slots that validate mocks at construction time.

## Implemented Design

### 1. MockRequirements from DAG

```rust
// In core/test/src/mock_requirements.rs
pub fn extract_mock_requirements<T>(dag: &Dag<T>, name: &str) -> MockRequirements {
let boundaries = detect_boundaries(dag);

// Analyze DAG structure to find:
// - Boundary ports (unconnected outputs)
// - Transport executor outputs (connected or not)
// - Resource/environment outputs (connected or not)

// Create MockRequirements with typed slots
}
```

### 2. Type-Checked Mock Setting

```rust
impl MockRequirements {
/// Set a boundary mock. Validates type at call site.
pub fn boundary(
mut self,
node: &str,
port: &str,
value: impl Into<Value>,
) -> Result<Self, MockTypeError> {
let slot = self.find_slot(node, port)?;
let value = value.into();
self.validate_type(&slot, &value)?;
// ... add to filled set and mock list
Ok(self)
}

// Typed helpers
pub fn boundary_str(self, node: &str, port: &str, value: &str) -> Result<Self, MockTypeError>
pub fn boundary_json(self, node: &str, port: &str, value: serde_json::Value) -> Result<Self, MockTypeError>
pub fn transport_response(self, node: &str, port: &str, response: TransportResponse) -> Result<Self, MockTypeError>
}
```

### 3. Migration Example (gist)

```rust
// Old pattern:
pub fn gist_mock_spec(mode: &GistMode) -> MockSpec {
MockSpec::new("gist")
.boundary("fs_env", "fs:write", mock_fs_handle()) // Could have wrong type
.boundary("execute_gist", "response", Value::Json(...)) // Could forget
}

// New pattern:
pub fn gist_mock_spec(mode: &GistMode) -> MockSpec {
let dag = build_gist_graph(mode.clone(), vec![], false)
.expect("gist graph should build");

extract_mock_requirements(&dag, "gist")
.boundary("fs_env", "fs:write", mock_fs_handle())
.expect("fs:write mock should match type") // Type-checked!
.transport_response("execute_gist", "response", mock_response())
.expect("execute_gist response should match type")
.build_unchecked()
}
```

## Type Compatibility

The following type compatibilities are implemented:

| Value Type | Port TypeId | Compatible |
|------------|-------------|------------|
| String | String | ✓ |
| Int | Int | ✓ |
| Int | Timestamp | ✓ (Timestamp serializes as Int) |
| Map | ToolHandle, AuthToken, FilesystemHandle, Platform | ✓ (Map-backed types) |
| Response | TransportResponse | ✓ |
| Json | Any | ✓ (Json is flexible) |
| Any | Any | ✓ |
| Skipped | Any | ✓ (Skipped is always compatible) |

## MockSlotKind

Four kinds of interceptable nodes are detected:

1. **Boundary** - Unconnected output ports (world writes) - Optional mocks
2. **Transport** - Transport executor outputs (consume TransportRequest) - Required mocks
3. **Resource** - Environment/resource node outputs (emit capability tokens) - Required mocks
4. **CliTool** - CLI tool nodes (consume ToolHandle but not TransportRequest) - Required mocks (e.g., `clippy_lint`)

All transport and CLI tool outputs require mocks because DryRun interception returns mocked values for the entire node.

## Error Handling

Errors at mock construction time (not testgen time):

```rust
pub enum MockTypeError {
UnknownSlot { node: String, port: String },
TypeMismatch { node: String, port: String, expected: String, actual: String },
CardinalityMismatch { node: String, port: String, expected: Cardinality, actual: usize },
}

pub struct MockIncompleteError {
dag_name: String,
missing: Vec<String>, // List of node.port that are unfilled
}
```

## Benefits Realized

1. **Impossible to forget mocks** - `build()` fails if required slots unfilled
2. **Type errors at definition site** - not at testgen, not at test runtime
3. **Co-location** - DAG is built first, mocks extracted from its structure
4. **IDE support** - typed helpers enable autocomplete
5. **Test validates pattern** - `test_typed_builder_catches_type_errors` proves type checking works
6. **Duplicate prevention** - Setting the same slot twice replaces instead of duplicating
7. **Unknown slot detection** - Testgen validates that all mocks reference existing nodes/ports

## Testgen Validations

Testgen (`core/codegen/src/testgen/codegen.rs`) now validates:

1. **Unknown mock slots** - Mocks referencing non-existent nodes/ports cause panics
2. **Type compatibility** - Mock value types must match port types (same rules as MockRequirements)
3. **Missing transport mocks** - Connected transport outputs without mocks cause panics

Type compatibility is implemented identically in both MockRequirements and testgen:
- `Int -> Timestamp` (Timestamp serializes as Int)
- `String -> Platform` (Platform serializes as String)
- `Map -> ToolHandle/AuthToken/FilesystemHandle/Platform` (Map-backed types)

## Migration Status

| Tool | Status | Notes |
|------|--------|-------|
| gist | ✅ Migrated | Full pattern: DAG → extract → type-check → build |
| deps | ✅ Migrated | Transport + resource mocks only |
| makegen | ✅ Migrated | Transport node with multiple outputs |
| bootstrap | ✅ Migrated | Multiple transport nodes (scan, makefile, gitignore) |
| ci | ✅ Migrated | Complex graph with transport + resource + CliTool nodes |
| review | ✅ Migrated | Two graphs: inline and diff, both with LLM transport |
| llm-ops | ⏸️ Skipped | No DAG builder - mock specs define expected patterns for testing |

## Relation to Phase 9 DagSpec

This is a stepping stone to DagSpec. Once all mock specs are migrated:

```rust
// Current approach
pub fn gist_mock_spec() -> MockSpec {
let dag = build_gist_dag();
extract_mock_requirements(&dag, "gist")
.boundary_str(...)
.build_unchecked()
}

// Future DagSpec
pub struct DagSpec<T> {
builder: fn() -> Dag<T>,
mock_spec: MockSpec, // Built from requirements, guaranteed complete
signature: Option<DagSignature>,
}
```

## Resolved Questions

1. **Error handling:** Using `Result` with `.expect()` at call sites - Rusty and explicit
2. **Cardinality validation:** Not yet implemented, but slot has cardinality info
3. **Optional slots:** All slots from boundaries are required; `build_unchecked()` panics if missing
4. **Compile-time vs runtime:** Runtime checking at mock construction is acceptable - error happens at definition site
26 changes: 14 additions & 12 deletions TODO/testgen-improvements.md
Original file line number Diff line number Diff line change
Expand Up @@ -404,28 +404,30 @@ tests for `ResourceType::Lease`.
Tests that DAG signature validates. Not yet generated by testgen.
Requires new testgen assertion (see TODO 8.2 below).

**TODO 8.1: Add transport-mock coverage assertion to testgen**
- [ ] Walk DAG analysis, find transport executor nodes
- [ ] Assert MockSpec provides mocks for all output ports used downstream
- [ ] This replaces per-tool boundary presence tests (Pattern A)
- [ ] Subsumes ~25 of the 49 tests
**TODO 8.1: Add transport-mock coverage assertion to testgen** ✅
- [x] Walk DAG analysis, find transport executor nodes
- [x] Assert MockSpec provides mocks for all output ports used downstream
- [x] This replaces per-tool boundary presence tests (Pattern A)
- [x] Subsumes ~25 of the 49 tests
- *Implemented in `codegen.rs:188-223` — panics if connected transport outputs lack mocks*

**TODO 8.2: Add signature validation assertion to testgen** ✅
- [x] If a `TestgenTargetDef` includes a signature, emit a test that
calls `signature.validate(&dag)`
- [ ] Optionally: `infer_signature(&dag)` matches declared signature
- [ ] This replaces per-tool signature tests (Pattern E, consolidation §7 Pattern 3)

**TODO 8.3: Add mock-value type compatibility assertion to testgen**
- [ ] For each mock value in MockSpec, assert `Value` type is compatible
**TODO 8.3: Add mock-value type compatibility assertion to testgen** ✅
- [x] For each mock value in MockSpec, assert `Value` type is compatible
with the DAG port's `type_id` (and cardinality)
- [ ] Contract-level check: "mock is Bool-typed" not "mock == Bool(true)"
- [ ] Catches type drift between MockSpec and DAG port definitions
- [x] Contract-level check: "mock is Bool-typed" not "mock == Bool(true)"
- [x] Catches type drift between MockSpec and DAG port definitions
- *Implemented in `codegen.rs:230-254` + `find_mock_type_mismatches()` helper*

**TODO 8.4: Delete redundant graph_mock.rs tests**
- [x] Delete Pattern A tests (boundary presence) — 10 tests deleted across 7 files
- [x] Delete Pattern C tests (self-chain) — 1 test deleted from llm-ops
- [x] Delete Pattern D tests (resource presence) — 4 tests deleted across 4 files
- [x] Delete Pattern A tests (boundary presence) — 12 tests deleted (10 earlier + 2 gist)
- [x] Delete Pattern C tests (self-chain) — 3 tests deleted (1 llm-ops + 2 gist)
- [x] Delete Pattern D tests (resource presence) — 5 tests deleted (4 earlier + 1 gist)
- [ ] Migrate Pattern B tests to NodeExamples — then delete
- [ ] Delete Pattern E tests — once TODO 8.2 lands
- [ ] Goal: graph_mock.rs files contain only `pub fn mock_spec()` + data
Expand Down
Loading