mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-09-10 22:43:37 +00:00
Remove stale review docs, add spec DoD pipeline definitions
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Bryan Helmkamp
parent
4bb2de5a49
commit
d3e396753c
13 changed files with 906 additions and 3252 deletions
|
|
@ -1,243 +0,0 @@
|
|||
# Attractor Spec Compliance Review
|
||||
|
||||
Full review of `crates/attractor/` against `docs/specs/attractor-spec.md`.
|
||||
Reviewed 2026-02-20 by 5 parallel agents. Second-pass false-positive analysis applied.
|
||||
|
||||
---
|
||||
|
||||
## Section 1: Overview and Goals
|
||||
|
||||
| # | Sub-section | Verdict | Notes |
|
||||
|---|------------|---------|-------|
|
||||
| 1 | 1.1 Problem Statement | ALIGNED | Narrative; no code requirements |
|
||||
| 2 | 1.2 Why DOT Syntax | ALIGNED | Parser accepts DOT as specified |
|
||||
| 3 | 1.3 Design Principles | ALIGNED | Graph layer supports pluggable handlers, checkpoint, HITL, edge routing |
|
||||
| 4 | 1.4 Layering and LLM Backends | ALIGNED | Backend-agnostic; `CodergenBackend` trait exists |
|
||||
|
||||
---
|
||||
|
||||
## Section 2: DOT DSL Schema
|
||||
|
||||
| # | Sub-section | Verdict | Notes |
|
||||
|---|------------|---------|-------|
|
||||
| 5 | 2.1 Supported Subset | ALIGNED | Strict DOT subset enforced; one digraph per file (`parser/mod.rs:23-29`) |
|
||||
| 6 | 2.2 BNF Grammar | ALIGNED | All productions implemented in `grammar.rs` and `lexer.rs` |
|
||||
| 7 | 2.3 Key Constraints | ALIGNED | Directed-only, commas required, optional semicolons, comments stripped |
|
||||
| 8 | 2.4 Value Types | ALIGNED | String, Integer, Float, Boolean, Duration all supported |
|
||||
| 9 | 2.5 Graph Attributes | ALIGNED | `goal`, `model_stylesheet`, `default_max_retry`, `retry_target`, `fallback_retry_target`, `default_fidelity` all have accessors |
|
||||
| 10 | 2.6 Node Attributes | ALIGNED | All 17 node attributes have typed accessors in `graph/types.rs` |
|
||||
| 11 | 2.7 Edge Attributes | ALIGNED | All 7 edge attributes implemented |
|
||||
| 12 | 2.8 Shape-to-Handler Mapping | ALIGNED | Complete 9-shape mapping at `types.rs:72-85` with tests |
|
||||
| 13 | 2.9 Chained Edges | ALIGNED | `A -> B -> C` expanded via `windows(2)` in semantic analysis |
|
||||
| 14 | 2.10 Subgraphs | ALIGNED | Scoped defaults, class derivation from label |
|
||||
| 15 | 2.11 Default Blocks | ALIGNED | `node [...]` and `edge [...]` defaults applied correctly |
|
||||
| 16 | 2.12 Class Attribute | ALIGNED | Comma-separated, trimmed, deduplicated |
|
||||
| 17 | 2.13 Minimal Examples | ALIGNED | All 3 spec examples parse; tested |
|
||||
|
||||
**Minor gaps in Section 2:**
|
||||
- `Direction` values (`TB`/`LR`/`BT`/`RL`) not validated to allowed set
|
||||
- No explicit error for `strict` modifier or undirected `graph` keyword
|
||||
- `Graph` missing a `label()` convenience accessor
|
||||
- `Node::max_retries()` returns `Option<i64>` instead of defaulting to `0`
|
||||
- Subgraph class derivation only checks `GraphAttrDecl`, not `graph [label=...]` block form
|
||||
|
||||
---
|
||||
|
||||
## Section 3: Pipeline Execution Engine
|
||||
|
||||
| # | Sub-section | Verdict | Notes |
|
||||
|---|------------|---------|-------|
|
||||
| 18 | 3.1 Run Lifecycle | ALIGNED | 5 phases present; FINALIZE does not clean up resources (minor) |
|
||||
| 19 | 3.2 Core Execution Loop | GAP | `loop_restart` (step 7) not implemented -- just jumps to target |
|
||||
| 20 | 3.3 Edge Selection | ALIGNED | All 5 steps match spec; `normalize_label` handles prefixes |
|
||||
| 21 | 3.4 Goal Gate Enforcement | ALIGNED | 4-level retry_target fallback implemented |
|
||||
| 22 | 3.5 Retry Logic | GAP | `should_retry` predicate missing; retry counter not tracked; `reset_retry_counter` absent |
|
||||
| 23 | 3.6 Retry Policy | GAP | Presets defined but never selectable from node attributes; `default_max_retry=50` vs spec default `0` |
|
||||
| 24 | 3.7 Failure Routing | GAP | retry_target/fallback_retry_target not consulted on node FAIL -- only used for goal gates |
|
||||
| 25 | 3.8 Concurrency Model | ALIGNED | Single-threaded traversal; parallel handler manages branches |
|
||||
|
||||
---
|
||||
|
||||
## Section 4: Node Handlers
|
||||
|
||||
| # | Sub-section | Verdict | Notes |
|
||||
|---|------------|---------|-------|
|
||||
| 26 | 4.1 Handler Interface | ALIGNED | `Handler` trait matches spec signature |
|
||||
| 27 | 4.2 Handler Registry | ALIGNED | 3-step priority: explicit type > shape > default |
|
||||
| 28 | 4.3 Start Handler | ALIGNED | Returns SUCCESS immediately |
|
||||
| 29 | 4.4 Exit Handler | ALIGNED | Returns SUCCESS immediately |
|
||||
| 30 | 4.5 Codergen Handler | ALIGNED | Prompt expansion, backend call, artifact writes all present |
|
||||
| 31 | 4.6 Wait For Human | ALIGNED | Choices, freeform, accelerator keys, timeout/skip handling |
|
||||
| 32 | 4.7 Conditional Handler | ALIGNED | Pass-through SUCCESS; routing via edge selection |
|
||||
| 33 | 4.8 Parallel Handler | GAP | **Stub** -- no actual concurrent execution, no context cloning, no join/error policies |
|
||||
| 34 | 4.9 Fan-In Handler | GAP | Heuristic select works; no LLM-based evaluation path |
|
||||
| 35 | 4.10 Tool Handler | ALIGNED | Shell execution via `sh -c`; no command timeout (minor) |
|
||||
| 36 | 4.11 Manager Loop Handler | GAP | **Stub** -- always returns FAIL |
|
||||
| 37 | 4.12 Custom Handlers | ALIGNED | Trait + registry supports registration; panics not caught (minor) |
|
||||
|
||||
---
|
||||
|
||||
## Section 5: State and Context
|
||||
|
||||
| # | Sub-section | Verdict | Notes |
|
||||
|---|------------|---------|-------|
|
||||
| 38 | 5.1 PipelineContext | ALIGNED | Key-value store with get/set/merge; `internal.retry_count.<node_id>` never written (minor) |
|
||||
| 39 | 5.2 Outcome Model | ALIGNED | All fields and status values present |
|
||||
| 40 | 5.3 Checkpoint/Resume | GAP | `Checkpoint::save` works; `load` exists but **no resume logic in engine**; `node_retries` never populated |
|
||||
| 41 | 5.4 Context Fidelity | GAP | Attributes parsed/validated but **fidelity resolution precedence and session/thread management not implemented** in engine |
|
||||
| 42 | 5.5 Artifact Store | ALIGNED | Full implementation including file-backing |
|
||||
| 43 | 5.6 Run Directory | GAP | Missing `manifest.json`; per-node dirs only created by CodergenHandler, not other handlers |
|
||||
|
||||
---
|
||||
|
||||
## Section 6: Human-in-the-Loop (Interviewer Pattern)
|
||||
|
||||
| # | Sub-section | Verdict | Notes |
|
||||
|---|------------|---------|-------|
|
||||
| 44 | 6.1 Interviewer Trait | ALIGNED | `ask(Question) -> Answer` interface |
|
||||
| 45 | 6.2 Question Model | ALIGNED | Options, allow_freeform, timeout, stage |
|
||||
| 46 | 6.3 Answer Model | ALIGNED | AnswerValue variants cover spec cases |
|
||||
| 47 | 6.4 Built-in Interviewers | GAP | AutoApprove, Callback, Queue, Recording present; **ConsoleInterviewer missing** |
|
||||
| 48 | 6.5 Timeout Handling | GAP | Data model present but **no runtime timeout enforcement** in any interviewer |
|
||||
|
||||
---
|
||||
|
||||
## Section 7: Validation and Linting
|
||||
|
||||
| # | Sub-section | Verdict | Notes |
|
||||
|---|------------|---------|-------|
|
||||
| 49 | 7.1 Diagnostic Model | ALIGNED | Struct matches spec exactly (rule, severity, message, node_id, edge, fix) |
|
||||
| 50 | 7.2 Built-In Rules | GAP | 14 rules present; `start_node` and `terminal_node` only check shape, not ID fallback; `stylesheet_syntax` only checks brace balance |
|
||||
| 51 | 7.3 Validation API | GAP | No `extra_rules` parameter on `validate()`/`validate_or_raise()` |
|
||||
| 52 | 7.4 Custom Lint Rules | GAP | `LintRule` trait exists but **no registration mechanism** |
|
||||
|
||||
---
|
||||
|
||||
## Section 8: Model Stylesheet
|
||||
|
||||
| # | Sub-section | Verdict | Notes |
|
||||
|---|------------|---------|-------|
|
||||
| 53 | 8.1 Overview | ALIGNED | Stylesheet applied as transform after parsing |
|
||||
| 54 | 8.2 Grammar | ALIGNED | `*`, `.class`, `#id` selectors; ClassName accepts uppercase (minor) |
|
||||
| 55 | 8.3 Specificity | ALIGNED | Universal(0) < Class(1) < ID(2); explicit attrs never overridden |
|
||||
| 56 | 8.4 Recognized Properties | ALIGNED | `llm_model`, `llm_provider`, `reasoning_effort` |
|
||||
| 57 | 8.5 Application Order | ALIGNED | Explicit > stylesheet > default |
|
||||
| 58 | 8.6 Example | ALIGNED | Spec example validated by dedicated test |
|
||||
|
||||
**Minor gap:** No shape selector support (e.g., `box { ... }`) -- spec section 11.10 mentions it.
|
||||
|
||||
---
|
||||
|
||||
## Section 9: Transforms and Extensibility
|
||||
|
||||
| # | Sub-section | Verdict | Notes |
|
||||
|---|------------|---------|-------|
|
||||
| 59 | 9.1 AST Transforms | GAP | Trait takes `&mut Graph` (in-place) vs spec's "returns new Graph"; no `prepare_pipeline` function |
|
||||
| 60 | 9.2 Built-In Transforms | GAP | Variable expansion and stylesheet transforms present; **Preamble Transform missing** |
|
||||
| 61 | 9.3 Custom Transforms | GAP | Trait is public but **no `register_transform` API** on engine |
|
||||
| 62 | 9.4 Pipeline Composition | GAP | Manager loop stub; no graph merging transform |
|
||||
| 63 | 9.5 HTTP Server Mode | ALIGNED | Spec says "may expose" -- not required |
|
||||
| 64 | 9.6 Events | ALIGNED | All 16 event types defined and serializable |
|
||||
| 65 | 9.7 Tool Call Hooks | GAP | `tool_hooks.pre`/`tool_hooks.post` not read or executed |
|
||||
|
||||
**Minor gaps in 9.6:** Parallel events and interview events defined but never emitted by their handlers.
|
||||
|
||||
---
|
||||
|
||||
## Section 10: Condition Expression Language
|
||||
|
||||
| # | Sub-section | Verdict | Notes |
|
||||
|---|------------|---------|-------|
|
||||
| 66 | 10.1 Overview | ALIGNED | Dedicated condition module |
|
||||
| 67 | 10.2 Grammar | ALIGNED | `&&`-separated clauses, `=`/`!=` operators |
|
||||
| 68 | 10.3 Semantics | ALIGNED | AND-combined, exact case-sensitive string comparison |
|
||||
| 69 | 10.4 Variable Resolution | ALIGNED | `outcome`, `preferred_label`, `context.*`, missing-as-empty |
|
||||
| 70 | 10.5 Evaluation | GAP | Bare key truthiness check returns error instead of evaluating as truthy |
|
||||
| 71 | 10.6 Examples | ALIGNED | All spec examples verified by tests |
|
||||
| 72 | 10.7 Extended Operators | ALIGNED | Not implemented (spec says future) |
|
||||
|
||||
---
|
||||
|
||||
## Section 11: Definition of Done
|
||||
|
||||
| # | Sub-section | Verdict | Notes |
|
||||
|---|------------|---------|-------|
|
||||
| 73 | 11.1 DOT Parsing | ALIGNED | Integration tests verify all spec examples |
|
||||
| 74 | 11.2 Validation | ALIGNED | 14 rules, `validate_or_raise` blocks on errors |
|
||||
| 75 | 11.3 Execution Engine | ALIGNED | Full loop, edge selection, handler dispatch |
|
||||
| 76 | 11.4 Goal Gates | ALIGNED | Checked at terminal; retry_target fallback chain |
|
||||
| 77 | 11.5 Retry Logic | ALIGNED | Exponential backoff with jitter; `allow_partial` |
|
||||
| 78 | 11.6 Node Handlers | GAP | Manager loop stub; parallel stub |
|
||||
| 79 | 11.7 State and Context | GAP | Checkpoint resume not implemented |
|
||||
| 80 | 11.8 Human-in-the-Loop | GAP | No ConsoleInterviewer; no SINGLE_SELECT/MULTI_SELECT distinction |
|
||||
| 81 | 11.9 Conditions | ALIGNED | With bare-key gap noted above |
|
||||
| 82 | 11.10 Stylesheet | GAP | No shape selector |
|
||||
| 83 | 11.11 Transforms | GAP | No `register_transform` API; no preamble transform |
|
||||
| 84 | 11.12 Cross-Feature Matrix | GAP | Missing integration tests: retry-on-failure, checkpoint resume, 10+ node pipeline |
|
||||
| 85 | 11.13 Integration Smoke Test | GAP | No end-to-end test with real LLM callback |
|
||||
|
||||
---
|
||||
|
||||
## Gap Summary by Severity (after false-positive analysis)
|
||||
|
||||
9 of 36 original gaps were false positives. **27 legitimate gaps remain.**
|
||||
|
||||
### False Positives Removed
|
||||
|
||||
| Original # | Reason |
|
||||
|------------|--------|
|
||||
| 8 (Retry Presets) | Spec does not require presets be selectable from node attributes |
|
||||
| 9 (default_max_retry) | Spec attribute tables say default is 50; code matches |
|
||||
| 24 (Direction) | Direction is a Graphviz layout hint, not an Attractor semantic attribute |
|
||||
| 25 (strict/graph) | Implicit rejection via grammar is sufficient |
|
||||
| 26 (Graph::label()) | Attribute is stored and accessible via `attrs` map |
|
||||
| 27 (max_retries default) | Spec tables and code agree on graph-level default of 50 |
|
||||
| 29 (Variable Expansion) | Spec says only `$goal` from graph; code matches exactly |
|
||||
| 32 (Transform Signature) | `&mut Graph` is Rust-idiomatic; spec allows "modified graph" |
|
||||
| 34 (QuestionType) | Spec 6.2 defines the actual types; 11.8 uses inconsistent names |
|
||||
|
||||
### HIGH (5 gaps -- blocking or core functionality missing)
|
||||
|
||||
| # | Location | Gap |
|
||||
|---|----------|-----|
|
||||
| 1 | 3.7 Failure Routing | retry_target/fallback_retry_target not consulted on node FAIL (only on goal gate) |
|
||||
| 2 | 4.8 Parallel Handler | Stub -- no concurrent execution, no join/error policies |
|
||||
| 3 | 4.11 Manager Loop | Stub -- always returns FAIL |
|
||||
| 4 | 5.3 Checkpoint Resume | `Checkpoint::load` exists but engine has no resume-from-checkpoint logic |
|
||||
| 5 | 5.4 Context Fidelity | Fidelity resolution and session/thread management not implemented |
|
||||
|
||||
### MEDIUM (15 gaps -- functional but incomplete)
|
||||
|
||||
| # | Location | Gap |
|
||||
|---|----------|-----|
|
||||
| 6 | 3.2 loop_restart | Not implemented -- jumps to target instead of restarting run |
|
||||
| 7 | 3.5 should_retry | No retryable vs non-retryable error classification |
|
||||
| 10 | 4.9 Fan-In | No LLM-based evaluation path |
|
||||
| 11 | 4.12 Panic Safety | Handler panics not caught by engine |
|
||||
| 12 | 5.6 Run Directory | Missing `manifest.json`; per-node dirs only from CodergenHandler |
|
||||
| 13 | 6.4 ConsoleInterviewer | Not implemented |
|
||||
| 14 | 6.5 Timeout Handling | No runtime timeout enforcement |
|
||||
| 15 | 7.2 start_node rule | Only checks shape=Mdiamond, not ID-based fallback |
|
||||
| 16 | 7.2 terminal_node rule | Only checks shape=Msquare, not ID-based fallback |
|
||||
| 17 | 7.3/7.4 Custom Rules | LintRule trait exists but no registration or extra_rules param |
|
||||
| 18 | 8.2/11.10 Shape Selector | Stylesheet only supports `*`, `.class`, `#id` -- no shape selector |
|
||||
| 19 | 9.1 prepare_pipeline | No function chaining parse -> transforms -> validate |
|
||||
| 20 | 9.2 Preamble Transform | Not implemented |
|
||||
| 21 | 9.3 register_transform | No API on engine for registering transforms |
|
||||
| 22 | 9.6 Event Emission | Parallel and interview events defined but never emitted |
|
||||
| 23 | 9.7 Tool Call Hooks | pre/post hooks not read or executed |
|
||||
|
||||
### LOW (7 gaps -- minor, cosmetic, or optional)
|
||||
|
||||
| # | Location | Gap |
|
||||
|---|----------|-----|
|
||||
| 28 | 2.10 Subgraph label | Class derivation misses `graph [label=...]` block form |
|
||||
| 30 | 4.10 Tool Timeout | No command timeout on shell execution |
|
||||
| 31 | 8.2 ClassName | Accepts uppercase; spec says `[a-z0-9-]+` |
|
||||
| 33 | 10.5 Bare Key | Returns error instead of truthiness check |
|
||||
| 35 | 11.12 Test Coverage | Missing integration tests for retry, resume, 10+ nodes |
|
||||
| 36 | 11.13 Smoke Test | No real LLM end-to-end test |
|
||||
|
||||
---
|
||||
|
||||
**Overall: 58 of 85 items ALIGNED. 27 legitimate gaps (5 HIGH, 15 MEDIUM, 7 LOW).**
|
||||
|
|
@ -1,476 +0,0 @@
|
|||
# coding-agent-loop Simplification Analysis
|
||||
|
||||
Date: 2026-02-20
|
||||
|
||||
## Executive Summary
|
||||
|
||||
The `coding-agent-loop` crate is approximately 4,800 lines of production code and tests across 19 source files. The architecture is generally sound, but there are significant opportunities to reduce complexity, eliminate duplication, and improve maintainability. The most impactful findings center on massive test mock duplication, duplicated tool execution logic in `session.rs`, and the `ProviderProfile` trait being too wide.
|
||||
|
||||
---
|
||||
|
||||
## HIGH Severity Findings
|
||||
|
||||
### 1. Massive Mock `ExecutionEnvironment` Duplication Across Tests
|
||||
|
||||
**What:** The `ExecutionEnvironment` trait has 12 methods, and a full mock implementation is copy-pasted into nearly every test module. I count at least **11 separate mock implementations** of `ExecutionEnvironment` spread across:
|
||||
|
||||
- `execution_env.rs` (`MockEnv`)
|
||||
- `tool_registry.rs` (`DummyEnv`)
|
||||
- `tools.rs` (`ReadFileEnv`, `WriteFileEnv`, `EditFileEnv`, `ShellEnv`, `ShellCapturingEnv`, `GrepEnv`, `GlobEnv`)
|
||||
- `provider_profile.rs` (`TestEnv`)
|
||||
- `project_docs.rs` (`DocEnv`)
|
||||
- `profiles/mod.rs` (`TestEnv`)
|
||||
- `profiles/anthropic.rs` (`TestEnv`)
|
||||
- `profiles/gemini.rs` (`TestEnv`)
|
||||
- `profiles/openai.rs` (`TestEnv`, `MockFileEnv`)
|
||||
- `subagent.rs` (`MemoryExecutionEnvironment`)
|
||||
- `session.rs` (`MemoryExecutionEnvironment`)
|
||||
|
||||
Each one is 30-60 lines of boilerplate implementing every trait method. Most implementations are identical stubs returning empty/default values, with only 1-2 methods customized per mock.
|
||||
|
||||
**Where:** Every file with `#[cfg(test)]` modules.
|
||||
|
||||
**Simplification:** Create a single `MockExecutionEnvironment` in a shared test utility module (e.g., `src/test_support.rs` behind `#[cfg(test)]`) that provides sensible defaults. Specific tests can then wrap or override individual methods using composition or builder patterns. This would eliminate approximately **500-700 lines** of duplicated test code.
|
||||
|
||||
```rust
|
||||
// src/test_support.rs
|
||||
#[cfg(test)]
|
||||
pub struct MockExecutionEnvironment {
|
||||
pub files: std::collections::HashMap<String, String>,
|
||||
pub exec_result: Option<ExecResult>,
|
||||
pub grep_results: Vec<String>,
|
||||
pub glob_results: Vec<String>,
|
||||
// ...
|
||||
}
|
||||
```
|
||||
|
||||
**Impact:** HIGH -- this is the single largest source of unnecessary code in the crate. It also makes adding new methods to `ExecutionEnvironment` extremely painful since every mock must be updated.
|
||||
|
||||
---
|
||||
|
||||
### 2. Duplicated Tool Execution Logic Between Sequential and Parallel Paths
|
||||
|
||||
**What:** `session.rs` contains two nearly identical implementations of tool execution:
|
||||
|
||||
1. `execute_single_tool` + `emit_execute_and_truncate` (used by the sequential path)
|
||||
2. The inline closure in `execute_tool_calls_parallel` (lines 507-613)
|
||||
|
||||
Both paths:
|
||||
- Emit `ToolCallStart` events
|
||||
- Look up the tool in the registry
|
||||
- Validate arguments against the schema
|
||||
- Execute the tool
|
||||
- Handle success/error into `ToolResult`
|
||||
- Emit `ToolCallEnd` events with output data
|
||||
- Truncate the output for history
|
||||
|
||||
The parallel path duplicates all of this logic inside a closure, including identical `ToolResult` construction, identical event emission, and identical truncation.
|
||||
|
||||
**Where:** `/crates/coding-agent-loop/src/session.rs`, lines 425-668.
|
||||
|
||||
**Simplification:** Extract a shared `execute_one_tool` function that takes the necessary context (emitter, registry, env, config, session_id) and returns the truncated `ToolResult`. Both the sequential and parallel paths should call this same function. The parallel path simply runs multiple instances concurrently with `join_all`.
|
||||
|
||||
This would eliminate approximately **80-100 lines** of duplicated logic and ensure bug fixes apply to both paths.
|
||||
|
||||
**Impact:** HIGH -- duplicated business logic is a correctness risk; fixing a bug in one path but not the other is easy.
|
||||
|
||||
---
|
||||
|
||||
### 3. `ProviderProfile` Trait Is Too Wide (14 Methods)
|
||||
|
||||
**What:** The `ProviderProfile` trait requires implementing 14 methods:
|
||||
|
||||
```rust
|
||||
pub trait ProviderProfile: Send + Sync {
|
||||
fn id(&self) -> String;
|
||||
fn model(&self) -> String;
|
||||
fn tool_registry(&self) -> &ToolRegistry;
|
||||
fn tool_registry_mut(&mut self) -> &mut ToolRegistry;
|
||||
fn build_system_prompt(...) -> String;
|
||||
fn tools(&self) -> Vec<ToolDefinition>;
|
||||
fn provider_options(&self) -> Option<serde_json::Value>;
|
||||
fn supports_reasoning(&self) -> bool;
|
||||
fn supports_streaming(&self) -> bool;
|
||||
fn supports_parallel_tool_calls(&self) -> bool;
|
||||
fn context_window_size(&self) -> usize;
|
||||
fn knowledge_cutoff(&self) -> &str;
|
||||
}
|
||||
```
|
||||
|
||||
Several of these are pure data fields that don't need virtual dispatch. The `tools()` method is always just `self.registry.definitions()`. The `tool_registry()` and `tool_registry_mut()` methods exist only to allow external registration of subagent tools. This forces every test to implement all 14 methods even when only 1-2 matter.
|
||||
|
||||
**Where:** `/crates/coding-agent-loop/src/provider_profile.rs`
|
||||
|
||||
**Simplification:** Consider replacing the trait with a struct that holds data fields plus a closure/trait for the only truly polymorphic behavior (`build_system_prompt`). Alternatively, add default implementations where possible (e.g., `fn tools(&self) -> Vec<ToolDefinition> { self.tool_registry().definitions() }`). At minimum, `tools()` should have a default implementation since it's identical in all 3 profiles and every test profile.
|
||||
|
||||
The `supports_*` methods and `context_window_size` could be a `ProfileCapabilities` struct to reduce the trait surface.
|
||||
|
||||
**Impact:** HIGH -- affects every test file and every new profile implementation.
|
||||
|
||||
---
|
||||
|
||||
## MEDIUM Severity Findings
|
||||
|
||||
### 4. `register_subagent_tools` Is Copy-Pasted Across All Three Profiles
|
||||
|
||||
**What:** The `register_subagent_tools` method is identical in `AnthropicProfile`, `GeminiProfile`, and `OpenAiProfile`:
|
||||
|
||||
```rust
|
||||
pub fn register_subagent_tools(
|
||||
&mut self,
|
||||
manager: Arc<tokio::sync::Mutex<SubAgentManager>>,
|
||||
session_factory: SessionFactory,
|
||||
current_depth: usize,
|
||||
) {
|
||||
self.registry.register(make_spawn_agent_tool(manager.clone(), session_factory, current_depth));
|
||||
self.registry.register(make_send_input_tool(manager.clone()));
|
||||
self.registry.register(make_wait_tool(manager.clone()));
|
||||
self.registry.register(make_close_agent_tool(manager));
|
||||
}
|
||||
```
|
||||
|
||||
**Where:** `profiles/anthropic.rs:45-60`, `profiles/gemini.rs:45-60`, `profiles/openai.rs:45-60`
|
||||
|
||||
**Simplification:** Move this to a free function or a method on `ToolRegistry`:
|
||||
|
||||
```rust
|
||||
pub fn register_subagent_tools(
|
||||
registry: &mut ToolRegistry,
|
||||
manager: Arc<tokio::sync::Mutex<SubAgentManager>>,
|
||||
session_factory: SessionFactory,
|
||||
current_depth: usize,
|
||||
) { ... }
|
||||
```
|
||||
|
||||
Or add it as a default method on `ProviderProfile` since the trait already has `tool_registry_mut()`.
|
||||
|
||||
**Impact:** MEDIUM -- 3x duplication of 8 lines each. Easy to drift.
|
||||
|
||||
---
|
||||
|
||||
### 5. `build_system_prompt` Duplicated Structure Across Profiles
|
||||
|
||||
**What:** All three profiles' `build_system_prompt` methods share identical preamble and postamble logic:
|
||||
|
||||
```rust
|
||||
let env_block = build_env_context_block_with(env, env_context);
|
||||
let docs_section = if project_docs.is_empty() {
|
||||
String::new()
|
||||
} else {
|
||||
format!("\n\n{}", project_docs.join("\n\n"))
|
||||
};
|
||||
let user_section = match user_instructions {
|
||||
Some(instructions) => format!("\n\n# User Instructions\n{instructions}"),
|
||||
None => String::new(),
|
||||
};
|
||||
```
|
||||
|
||||
This identical block appears in `anthropic.rs:87-96`, `gemini.rs:87-96`, and `openai.rs:87-96`. Only the core prompt text differs.
|
||||
|
||||
**Where:** All three profile files.
|
||||
|
||||
**Simplification:** Extract a helper that takes the core prompt as a parameter:
|
||||
|
||||
```rust
|
||||
fn assemble_system_prompt(
|
||||
core_prompt: &str,
|
||||
env: &dyn ExecutionEnvironment,
|
||||
env_context: &EnvContext,
|
||||
project_docs: &[String],
|
||||
user_instructions: Option<&str>,
|
||||
) -> String { ... }
|
||||
```
|
||||
|
||||
Each profile would then only need to provide its unique prompt text.
|
||||
|
||||
**Impact:** MEDIUM -- reduces ~15 lines per profile, more importantly makes the structure consistent.
|
||||
|
||||
---
|
||||
|
||||
### 6. `SessionEvent.data` Uses `HashMap<String, serde_json::Value>` Instead of Typed Variants
|
||||
|
||||
**What:** Every event emitted throughout the codebase constructs a `HashMap<String, serde_json::Value>` manually:
|
||||
|
||||
```rust
|
||||
let mut data = HashMap::new();
|
||||
data.insert("tool_name".to_string(), serde_json::json!(&tc.name));
|
||||
data.insert("tool_call_id".to_string(), serde_json::json!(&tc.id));
|
||||
```
|
||||
|
||||
This pattern is repeated 15+ times across `session.rs`. The keys are stringly-typed and there's no compile-time guarantee about what data each event kind carries.
|
||||
|
||||
**Where:** `/crates/coding-agent-loop/src/session.rs` (throughout), `types.rs`
|
||||
|
||||
**Simplification:** Use typed event data enums:
|
||||
|
||||
```rust
|
||||
pub enum EventData {
|
||||
Empty,
|
||||
ToolCall { tool_name: String, tool_call_id: String },
|
||||
ToolCallEnd { tool_name: String, tool_call_id: String, output: serde_json::Value, is_error: bool },
|
||||
Error { error: String },
|
||||
ContextWarning { estimated_tokens: usize, context_window_size: usize, usage_percent: usize },
|
||||
}
|
||||
```
|
||||
|
||||
This removes all the `HashMap::new()` / `.insert()` boilerplate and provides type safety.
|
||||
|
||||
**Impact:** MEDIUM -- affects readability and correctness of event handling code.
|
||||
|
||||
---
|
||||
|
||||
### 7. `tools.rs` Exports `make_read_many_files_tool`, `make_list_dir_tool`, `make_web_search_tool`, `make_web_fetch_tool` But They Are Not Re-exported from `lib.rs`
|
||||
|
||||
**What:** `lib.rs` only re-exports:
|
||||
```rust
|
||||
pub use tools::{
|
||||
make_edit_file_tool, make_glob_tool, make_grep_tool, make_read_file_tool, make_shell_tool,
|
||||
make_shell_tool_with_config, make_write_file_tool,
|
||||
};
|
||||
```
|
||||
|
||||
But `tools.rs` also defines `make_read_many_files_tool`, `make_list_dir_tool`, `make_web_search_tool`, and `make_web_fetch_tool`. These are used internally by profiles (Gemini uses all of them, OpenAI uses `apply_patch`) but are not available to external consumers.
|
||||
|
||||
**Where:** `/crates/coding-agent-loop/src/lib.rs:31-34`, `/crates/coding-agent-loop/src/tools.rs`
|
||||
|
||||
**Simplification:** Either re-export all tools from `lib.rs` for consistency, or make the non-exported ones `pub(crate)` to clarify they're internal. The current state is ambiguous -- they're `pub` in `tools.rs` but not re-exported, suggesting an oversight.
|
||||
|
||||
**Impact:** MEDIUM -- confusing public API surface.
|
||||
|
||||
---
|
||||
|
||||
### 8. `TestProfile` / `MockLlmProvider` Duplicated Between `session.rs` and `subagent.rs`
|
||||
|
||||
**What:** Both `session.rs` and `subagent.rs` define their own:
|
||||
- `MockLlmProvider` (identical implementation)
|
||||
- `TestProfile` (identical implementation)
|
||||
- `MemoryExecutionEnvironment` (nearly identical)
|
||||
- `text_response` helper (identical)
|
||||
- `make_client` helper (identical)
|
||||
- `make_session` helper (identical)
|
||||
|
||||
**Where:** `session.rs` tests (lines 785-1135) and `subagent.rs` tests (lines 340-536).
|
||||
|
||||
**Simplification:** Extract these into a shared test support module. This would save approximately **200 lines** of duplicated test infrastructure.
|
||||
|
||||
**Impact:** MEDIUM -- significant duplication that makes maintenance harder.
|
||||
|
||||
---
|
||||
|
||||
### 9. `Io(String)` Error Variant Is Never Constructed
|
||||
|
||||
**What:** `AgentError::Io(String)` is defined and tested but never actually used anywhere in the production code. No code path constructs this variant.
|
||||
|
||||
**Where:** `/crates/coding-agent-loop/src/error.rs:18-19`
|
||||
|
||||
**Simplification:** Remove the variant (and its test) if it's truly unused. If it's intended for future use, add a `#[allow(dead_code)]` with a comment explaining when it will be needed.
|
||||
|
||||
**Impact:** MEDIUM -- dead code.
|
||||
|
||||
---
|
||||
|
||||
### 10. `History::new()` and `Default` Redundancy
|
||||
|
||||
**What:** `History` derives `Default` and also has a `new()` method that does the same thing. Both `new()` and `default()` return `Self { turns: Vec::new() }`.
|
||||
|
||||
**Where:** `/crates/coding-agent-loop/src/history.rs:4-11`
|
||||
|
||||
**Simplification:** Remove the manual `new()` and use `Default::default()` everywhere, or keep `new()` and remove the `Default` derive. The codebase uses `History::new()` everywhere, so keeping `new()` is fine, but having both is unnecessary. The `#[derive(Default)]` could be kept for flexibility since it's zero-cost.
|
||||
|
||||
**Impact:** LOW (but worth noting for consistency).
|
||||
|
||||
---
|
||||
|
||||
## LOW Severity Findings
|
||||
|
||||
### 11. `count_turns` Method Is Just `len()` by Another Name
|
||||
|
||||
**What:** `History::count_turns()` simply returns `self.turns.len()`. The name `count_turns` doesn't add semantic value over `len()` given the method already returns `&[Turn]` via `turns()`.
|
||||
|
||||
**Where:** `/crates/coding-agent-loop/src/history.rs:22-24`
|
||||
|
||||
**Simplification:** Replace `count_turns()` calls with `turns().len()` and remove the method, or rename to `len()` to follow Rust convention.
|
||||
|
||||
**Impact:** LOW.
|
||||
|
||||
---
|
||||
|
||||
### 12. `build_request` Calls `self.provider_profile.tools()` Twice
|
||||
|
||||
**What:** In `session.rs` `build_request()`:
|
||||
```rust
|
||||
let tools = self.provider_profile.tools();
|
||||
// ...
|
||||
tools: if tools.is_empty() { None } else { Some(tools) },
|
||||
tool_choice: if self.provider_profile.tools().is_empty() { // <-- second call
|
||||
None
|
||||
} else {
|
||||
Some(ToolChoice::Auto)
|
||||
},
|
||||
```
|
||||
|
||||
The second `self.provider_profile.tools()` call re-collects all tool definitions from the registry when it could just reuse the `tools` variable.
|
||||
|
||||
**Where:** `/crates/coding-agent-loop/src/session.rs:402-413`
|
||||
|
||||
**Simplification:**
|
||||
```rust
|
||||
let tools = self.provider_profile.tools();
|
||||
let has_tools = !tools.is_empty();
|
||||
// ...
|
||||
tools: if has_tools { Some(tools) } else { None },
|
||||
tool_choice: if has_tools { Some(ToolChoice::Auto) } else { None },
|
||||
```
|
||||
|
||||
**Impact:** LOW -- minor inefficiency and readability issue.
|
||||
|
||||
---
|
||||
|
||||
### 13. `EnvContext` Fields `git_status_short` and `git_recent_commits` Are Populated But Never Used
|
||||
|
||||
**What:** `Session::build_env_context()` populates `git_status_short` and `git_recent_commits` from git commands, but `build_env_context_block_with()` never reads these fields. They are stored in the `EnvContext` struct but have no effect on the system prompt or any other behavior.
|
||||
|
||||
**Where:** `/crates/coding-agent-loop/src/session.rs:97-119`, `/crates/coding-agent-loop/src/profiles/mod.rs:29-55`
|
||||
|
||||
**Simplification:** Either use these fields in the environment context block (which seems to be the intent), or remove them and the git commands that populate them. Currently they cause two unnecessary shell invocations on every session initialization.
|
||||
|
||||
**Impact:** LOW -- dead code causing unnecessary I/O.
|
||||
|
||||
---
|
||||
|
||||
### 14. `truncation.rs` Rebuilds Default Limit HashMaps on Every Call
|
||||
|
||||
**What:** `truncate_tool_output` calls `default_char_limits()`, `default_line_limits()`, and `default_truncation_modes()` which each allocate and populate a new `HashMap` on every invocation.
|
||||
|
||||
**Where:** `/crates/coding-agent-loop/src/truncation.rs:87-121`
|
||||
|
||||
**Simplification:** Use `LazyLock` (stable in Rust 1.80+) or `const` arrays with a lookup function to avoid repeated allocation:
|
||||
|
||||
```rust
|
||||
static DEFAULT_CHAR_LIMITS: LazyLock<HashMap<&str, usize>> = LazyLock::new(|| {
|
||||
// ...
|
||||
});
|
||||
```
|
||||
|
||||
Alternatively, replace the `HashMap` lookups with simple match statements since the key sets are small and fixed.
|
||||
|
||||
**Impact:** LOW -- minor allocation overhead per tool call, but tool calls are not in a hot path.
|
||||
|
||||
---
|
||||
|
||||
### 15. `GrepOptions` Uses `grep` CLI Fallback That Will Always Succeed (Hiding `rg` Not Found)
|
||||
|
||||
**What:** In `local_env.rs`, the `grep` method checks if `rg --version` succeeds. But `std::process::Command::new("rg").arg("--version").status().is_ok()` returns `Ok` as long as the process was *launched*, not necessarily that it succeeded. The `.is_ok()` check is on the `Result` from `status()`, not on the exit code.
|
||||
|
||||
**Where:** `/crates/coding-agent-loop/src/local_env.rs:246-251`
|
||||
|
||||
**Simplification:** Check the exit code:
|
||||
```rust
|
||||
let use_rg = std::process::Command::new("rg")
|
||||
.arg("--version")
|
||||
.stdout(std::process::Stdio::null())
|
||||
.stderr(std::process::Stdio::null())
|
||||
.status()
|
||||
.map(|s| s.success())
|
||||
.unwrap_or(false);
|
||||
```
|
||||
|
||||
**Impact:** LOW -- subtle correctness issue on systems where `rg` exists but returns an error.
|
||||
|
||||
---
|
||||
|
||||
### 16. `glob` Implementation Uses Shell Globbing via `ls -d` Which Is Fragile
|
||||
|
||||
**What:** The `glob` method in `local_env.rs` uses `sh -c "ls -d {pattern} 2>/dev/null"` to expand glob patterns. This is fragile because:
|
||||
- Filenames with spaces or special characters will break
|
||||
- The pattern is not shell-escaped
|
||||
- `ls -d` behaves differently across platforms
|
||||
|
||||
**Where:** `/crates/coding-agent-loop/src/local_env.rs:300-333`
|
||||
|
||||
**Simplification:** Use the `glob` crate (a Rust-native glob implementation) instead of shelling out. This would be more reliable, cross-platform, and avoid shell injection concerns.
|
||||
|
||||
**Impact:** LOW for now (this is local-only), but worth addressing before any security-sensitive use.
|
||||
|
||||
---
|
||||
|
||||
### 17. Types Tests Are Overly Trivial
|
||||
|
||||
**What:** `types.rs` contains tests that merely construct enum variants and check that `PartialEq` works:
|
||||
|
||||
```rust
|
||||
fn session_state_equality() {
|
||||
assert_eq!(SessionState::Idle, SessionState::Idle);
|
||||
assert_ne!(SessionState::Idle, SessionState::Closed);
|
||||
}
|
||||
```
|
||||
|
||||
These test the `#[derive(PartialEq)]` macro, which is guaranteed by the compiler.
|
||||
|
||||
**Where:** `/crates/coding-agent-loop/src/types.rs:67-184`
|
||||
|
||||
**Simplification:** Remove these tests. They add ~80 lines of code that test derived functionality and provide no value. The construction tests for `Turn` variants are slightly more useful as documentation but still marginal.
|
||||
|
||||
**Impact:** LOW -- no correctness value, just noise.
|
||||
|
||||
---
|
||||
|
||||
### 18. `apply_patch` Delete Operation Writes Empty String Instead of Deleting
|
||||
|
||||
**What:** `PatchOperation::Delete` is handled by writing an empty string to the file:
|
||||
|
||||
```rust
|
||||
PatchOperation::Delete { path } => {
|
||||
env.write_file(path, "").await?;
|
||||
results.push(format!("Deleted file: {path}"));
|
||||
}
|
||||
```
|
||||
|
||||
This leaves a zero-byte file on disk rather than actually deleting it.
|
||||
|
||||
**Where:** `/crates/coding-agent-loop/src/profiles/openai.rs:359-362`
|
||||
|
||||
**Simplification:** Add a `delete_file` method to `ExecutionEnvironment`, or use `exec_command("rm ...")`. Writing empty content and calling it "deleted" is misleading.
|
||||
|
||||
**Impact:** LOW -- the current behavior may be intentional to avoid adding a `delete_file` method to the trait, but it's semantically wrong.
|
||||
|
||||
---
|
||||
|
||||
## Structural Observations
|
||||
|
||||
### File Organization
|
||||
|
||||
The module structure is reasonable. A few observations:
|
||||
|
||||
1. **`profiles/openai.rs` contains the entire v4a patch parser** (~200 lines). This is OpenAI-specific tooling that could be its own module (`src/patch_v4a.rs`) for clarity, since it's a self-contained parser/applier.
|
||||
|
||||
2. **`tools.rs` and `subagent.rs` both define tool factories** (functions that return `RegisteredTool`). The tools in `tools.rs` are "standard" tools while `subagent.rs` has subagent-specific tools. This split makes sense but the non-standard tools (`make_list_dir_tool`, `make_read_many_files_tool`, etc.) are only used by specific profiles and could be co-located with those profiles.
|
||||
|
||||
3. **`provider_profile.rs` and `profiles/mod.rs`** -- the trait is in one file and the `EnvContext` struct + `build_env_context_block` functions are in another. These are tightly coupled and could be consolidated.
|
||||
|
||||
### Approximate Line Count Savings
|
||||
|
||||
| Finding | Estimated Lines Saved |
|
||||
|---------|----------------------|
|
||||
| #1 Shared test mock | 500-700 |
|
||||
| #2 Deduplicate tool execution | 80-100 |
|
||||
| #4 Shared subagent registration | 20 |
|
||||
| #5 Shared prompt assembly | 40 |
|
||||
| #8 Shared test infrastructure | 200 |
|
||||
| #9 Remove dead Io variant | 10 |
|
||||
| #17 Remove trivial tests | 80 |
|
||||
| **Total** | **~930-1150 lines** |
|
||||
|
||||
This represents roughly 20-25% of the crate's total size, with the vast majority coming from test deduplication.
|
||||
|
||||
---
|
||||
|
||||
## Recommended Priority Order
|
||||
|
||||
1. **Shared test mock for `ExecutionEnvironment`** (#1, #8) -- highest impact, eliminates the most duplication
|
||||
2. **Deduplicate tool execution in session.rs** (#2) -- correctness risk
|
||||
3. **Extract shared prompt assembly** (#5) + **shared subagent registration** (#4)
|
||||
4. **Narrow the `ProviderProfile` trait** (#3) -- architectural improvement
|
||||
5. **Fix unused `EnvContext` fields** (#13) -- removes unnecessary I/O
|
||||
6. **Type the event data** (#6) -- readability improvement
|
||||
7. **Clean up minor issues** (#9, #11, #12, #14, #15, #17)
|
||||
|
|
@ -1,378 +0,0 @@
|
|||
# coding-agent-loop Simplification Proposals
|
||||
|
||||
## 1. Duplicated Mock ExecutionEnvironment Implementations
|
||||
|
||||
### 1.1 Duplicate full-trait mocks in tools.rs tests
|
||||
|
||||
**File:** `crates/coding-agent-loop/src/tools.rs`, lines 419-644
|
||||
**Current:** Three separate structs (`ReadFileEnv`, `WriteFileEnv`, `EditFileEnv`, `ShellCapturingEnv`) each implement the full `ExecutionEnvironment` trait with 13 methods, where only 1-2 methods differ from the defaults. Each mock is ~60 lines of boilerplate.
|
||||
**Simplification:** Extract a `DelegatingMockEnv` that wraps `MockExecutionEnvironment` and allows overriding specific methods via closures or composition. Alternatively, use the existing `MockExecutionEnvironment` with additional optional fields (like `files` for read, `written` capture for write, `captured_timeout` for shell). The `MockExecutionEnvironment` in `test_support.rs` already supports `files` and `exec_result` -- extending it with `Mutex<Option<(String,String)>>` for write captures would eliminate `WriteFileEnv`, `EditFileEnv`, and `ShellCapturingEnv` entirely.
|
||||
**Why:** ~220 lines of near-identical boilerplate across four structs. Every time the `ExecutionEnvironment` trait changes, four mocks must be updated in addition to the ones in `test_support.rs` and `openai.rs`.
|
||||
|
||||
### 1.2 Duplicate MockFileEnv in openai.rs tests
|
||||
|
||||
**File:** `crates/coding-agent-loop/src/profiles/openai.rs`, lines 436-516
|
||||
**Current:** `MockFileEnv` reimplements the full `ExecutionEnvironment` trait to support `Mutex<HashMap>` for write/delete operations in apply_patch tests.
|
||||
**Simplification:** Consolidate into a single shared mock in `test_support.rs` that supports mutable file operations. The existing `MockExecutionEnvironment` already has a `files: HashMap` field -- wrapping it in `Mutex` (or making a `MutableMockEnv` variant) would replace `MockFileEnv`.
|
||||
**Why:** Another ~80 lines of duplicated trait implementation. This is the same problem as 1.1 but in a different file.
|
||||
|
||||
---
|
||||
|
||||
## 2. Duplicated Profile Boilerplate
|
||||
|
||||
### 2.1 linux_env() helper duplicated across 4 test modules
|
||||
|
||||
**Files:**
|
||||
- `crates/coding-agent-loop/src/profiles/mod.rs`, lines 96-103
|
||||
- `crates/coding-agent-loop/src/profiles/anthropic.rs`, lines 164-170
|
||||
- `crates/coding-agent-loop/src/profiles/gemini.rs`, lines 215-220
|
||||
- `crates/coding-agent-loop/src/profiles/openai.rs`, lines 425-432
|
||||
|
||||
**Current:** Each test module defines an identical `linux_env()` function that creates a `MockExecutionEnvironment` with `working_dir: "/home/test"`, `platform_str: "linux"`, `os_version_str: "Linux 6.1.0"`.
|
||||
**Simplification:** Add `MockExecutionEnvironment::linux()` as a named constructor in `test_support.rs`.
|
||||
**Why:** Four identical copies of the same 6-line function. If the mock struct changes, all four must be updated.
|
||||
|
||||
### 2.2 Repetitive ProviderProfile implementations across three profiles
|
||||
|
||||
**Files:**
|
||||
- `crates/coding-agent-loop/src/profiles/anthropic.rs`, lines 41-57
|
||||
- `crates/coding-agent-loop/src/profiles/gemini.rs`, lines 41-56
|
||||
- `crates/coding-agent-loop/src/profiles/openai.rs`, lines 43-57
|
||||
|
||||
**Current:** `id()`, `model()`, `tool_registry()`, and `tool_registry_mut()` have identical implementations in all three profile structs. The only differences are the string returned by `id()`.
|
||||
**Simplification:** Introduce a `BaseProfile` struct containing the common `model: String` and `registry: ToolRegistry` fields, then each profile delegates to it. This could be done with a macro or simple struct composition. For example:
|
||||
|
||||
```rust
|
||||
struct BaseProfile {
|
||||
id: &'static str,
|
||||
model: String,
|
||||
registry: ToolRegistry,
|
||||
}
|
||||
```
|
||||
|
||||
Each profile wraps `BaseProfile` and the four boilerplate methods delegate to it.
|
||||
**Why:** Removes ~15 lines of identical code per profile (45 lines total) and makes it impossible for them to drift.
|
||||
|
||||
### 2.3 ParallelTestProfile vs TestProfile duplication
|
||||
|
||||
**File:** `crates/coding-agent-loop/src/test_support.rs`, lines 126-420
|
||||
**Current:** `ParallelTestProfile` and `TestProfile` are nearly identical `ProviderProfile` implementations. The only difference is `supports_parallel_tool_calls` (false vs true) and an optional `context_window` field.
|
||||
**Simplification:** Merge into a single `TestProfile` with configurable fields:
|
||||
|
||||
```rust
|
||||
pub(crate) struct TestProfile {
|
||||
pub registry: ToolRegistry,
|
||||
pub parallel_tool_calls: bool,
|
||||
pub context_window: usize,
|
||||
}
|
||||
```
|
||||
|
||||
The `with_tools` constructor defaults `parallel_tool_calls` to `false` and `context_window` to `200_000`. A `.with_parallel()` builder method or a `TestProfileBuilder` enables the parallel variant.
|
||||
**Why:** Eliminates ~60 lines of duplicated trait implementation and makes the test intention clearer.
|
||||
|
||||
---
|
||||
|
||||
## 3. Redundant or Unused Code
|
||||
|
||||
### 3.1 History::new() is redundant with Default derive
|
||||
|
||||
**File:** `crates/coding-agent-loop/src/history.rs`, lines 10-12
|
||||
**Current:** `History::new()` manually creates `Self { turns: Vec::new() }`, and `#[derive(Default)]` is on the struct.
|
||||
**Simplification:** Remove the manual `new()` method entirely and use `History::default()` everywhere, or keep `new()` but implement it as `Self::default()`. Currently both exist and do the same thing.
|
||||
**Why:** Two ways to do the same thing is confusing. Pick one.
|
||||
|
||||
### 3.2 EventEmitter::new() is redundant with Default impl
|
||||
|
||||
**File:** `crates/coding-agent-loop/src/event.rs`, lines 12-15 and 34-38
|
||||
**Current:** `new()` and `default()` are both defined, `default()` just calls `new()`.
|
||||
**Simplification:** This is a standard Rust pattern and is fine, but `#[must_use]` on `new()` but not on `Default::default()` is inconsistent. Consider removing the manual `Default` impl and adding `#[must_use]` consistently, or just keep one constructor.
|
||||
**Why:** Minor, but reduces cognitive load.
|
||||
|
||||
---
|
||||
|
||||
## 4. Control Flow Simplifications
|
||||
|
||||
### 4.1 Simplify followup loop in process_input
|
||||
|
||||
**File:** `crates/coding-agent-loop/src/session.rs`, lines 193-208
|
||||
**Current:**
|
||||
```rust
|
||||
loop {
|
||||
self.run_single_input(¤t_input).await?;
|
||||
let next_followup = self.followup_queue.lock()
|
||||
.expect("followup queue lock poisoned")
|
||||
.pop_front();
|
||||
match next_followup {
|
||||
Some(followup) => { current_input = followup; }
|
||||
None => break,
|
||||
}
|
||||
}
|
||||
```
|
||||
**Simplification:** Use `while let`:
|
||||
```rust
|
||||
self.run_single_input(¤t_input).await?;
|
||||
while let Some(followup) = self.followup_queue.lock()
|
||||
.expect("followup queue lock poisoned")
|
||||
.pop_front()
|
||||
{
|
||||
self.run_single_input(&followup).await?;
|
||||
}
|
||||
```
|
||||
**Why:** Eliminates the mutable `current_input` variable and the `loop`/`match`/`break` pattern. The intent is clearer: process the initial input, then process followups until the queue is empty.
|
||||
|
||||
### 4.2 Simplify SubAgentManager::spawn success path
|
||||
|
||||
**File:** `crates/coding-agent-loop/src/subagent.rs`, lines 67-87
|
||||
**Current:**
|
||||
```rust
|
||||
let task = tokio::spawn(async move {
|
||||
let result = session.process_input(&task_prompt).await;
|
||||
let turns = session.history().turns();
|
||||
let turns_used = turns.len();
|
||||
let last_text = turns.iter().rev().find_map(|t| {
|
||||
if let Turn::Assistant { content, .. } = t {
|
||||
Some(content.clone())
|
||||
} else {
|
||||
None
|
||||
}
|
||||
});
|
||||
let success = result.is_ok();
|
||||
if let Err(e) = result {
|
||||
return Err(e);
|
||||
}
|
||||
Ok(SubAgentResult {
|
||||
output: last_text.unwrap_or_default(),
|
||||
success,
|
||||
turns_used,
|
||||
})
|
||||
});
|
||||
```
|
||||
**Simplification:** The `success` variable is computed from `result.is_ok()`, then `result` is checked for `Err` immediately after. Since `success` is always `true` when reaching the `Ok` path:
|
||||
```rust
|
||||
let task = tokio::spawn(async move {
|
||||
session.process_input(&task_prompt).await?;
|
||||
let turns = session.history().turns();
|
||||
let last_text = turns.iter().rev().find_map(|t| match t {
|
||||
Turn::Assistant { content, .. } => Some(content.clone()),
|
||||
_ => None,
|
||||
});
|
||||
Ok(SubAgentResult {
|
||||
output: last_text.unwrap_or_default(),
|
||||
success: true,
|
||||
turns_used: turns.len(),
|
||||
})
|
||||
});
|
||||
```
|
||||
**Why:** Eliminates the unnecessary `success` variable (always `true` on the Ok path) and the redundant `if let Err(e) = result { return Err(e) }` pattern which is just `result?`.
|
||||
|
||||
### 4.3 validate_tool_args empty schema check is overly complex
|
||||
|
||||
**File:** `crates/coding-agent-loop/src/session.rs`, lines 654-658
|
||||
**Current:**
|
||||
```rust
|
||||
if schema.is_null()
|
||||
|| (schema.is_object() && schema.as_object().map_or(true, |o| o.is_empty()))
|
||||
{
|
||||
return Ok(());
|
||||
}
|
||||
```
|
||||
**Simplification:** The `map_or(true, |o| o.is_empty())` is confusing because `as_object()` returns `None` when `is_object()` is false, but we already checked `is_object()`. So the `map_or(true, ...)` default of `true` is dead code. Simplify to:
|
||||
```rust
|
||||
if schema.is_null() {
|
||||
return Ok(());
|
||||
}
|
||||
if let Some(obj) = schema.as_object() {
|
||||
if obj.is_empty() {
|
||||
return Ok(());
|
||||
}
|
||||
}
|
||||
```
|
||||
**Why:** The original combines null check and empty-object check with boolean operators in a way that requires careful reading. The separated version is immediately clear.
|
||||
|
||||
---
|
||||
|
||||
## 5. Structural / Architectural Simplifications
|
||||
|
||||
### 5.1 extract_signatures_from_assistant should use if-let instead of match
|
||||
|
||||
**File:** `crates/coding-agent-loop/src/loop_detection.rs`, lines 14-22
|
||||
**Current:**
|
||||
```rust
|
||||
fn extract_signatures_from_assistant(turn: &Turn) -> Vec<u64> {
|
||||
match turn {
|
||||
Turn::Assistant { tool_calls, .. } => tool_calls
|
||||
.iter()
|
||||
.map(|tc| tool_call_signature(&tc.name, &tc.arguments))
|
||||
.collect(),
|
||||
_ => vec![],
|
||||
}
|
||||
}
|
||||
```
|
||||
**Simplification:** This function is only called in one place (line 35), where the result is immediately checked with `if !sigs.is_empty()`. The function could be inlined, but even if kept, consider using `if let`:
|
||||
```rust
|
||||
fn extract_signatures_from_assistant(turn: &Turn) -> Vec<u64> {
|
||||
let Turn::Assistant { tool_calls, .. } = turn else {
|
||||
return vec![];
|
||||
};
|
||||
tool_calls
|
||||
.iter()
|
||||
.map(|tc| tool_call_signature(&tc.name, &tc.arguments))
|
||||
.collect()
|
||||
}
|
||||
```
|
||||
**Why:** The `let-else` pattern makes the happy path less indented and immediately shows the function's purpose.
|
||||
|
||||
### 5.2 build_request constructs system prompt on every call
|
||||
|
||||
**File:** `crates/coding-agent-loop/src/session.rs`, lines 371-403
|
||||
**Current:** `build_request()` calls `build_system_prompt()` every iteration of the tool-call loop (called from `run_single_input` inside the `loop` at line 237). The system prompt, project docs, and environment context do not change during a single input processing cycle.
|
||||
**Simplification:** Compute the system prompt once at the start of `run_single_input` and pass it into `build_request`, or cache it as a field that's rebuilt only when `initialize()` or `process_input()` is called.
|
||||
**Why:** Avoids redundant string allocation and concatenation on every LLM round-trip. For sessions with many tool rounds, this is significant wasted work.
|
||||
|
||||
### 5.3 estimate_token_count also rebuilds the system prompt
|
||||
|
||||
**File:** `crates/coding-agent-loop/src/session.rs`, lines 514-553
|
||||
**Current:** `estimate_token_count()` calls `build_system_prompt()` again to get its length, duplicating the work already done in `build_request()` on the same iteration.
|
||||
**Simplification:** If the system prompt is cached per proposal 5.2, `estimate_token_count` can read from the cache. Alternatively, pass the already-built system prompt length to `check_context_usage`.
|
||||
**Why:** Double construction of the system prompt per LLM call is wasteful.
|
||||
|
||||
### 5.4 ProviderProfile trait returns owned Strings unnecessarily
|
||||
|
||||
**File:** `crates/coding-agent-loop/src/provider_profile.rs`, lines 20-21
|
||||
**Current:** `fn id(&self) -> String` and `fn model(&self) -> String` return owned `String` values. Every call allocates.
|
||||
**Simplification:** Return `&str` instead:
|
||||
```rust
|
||||
fn id(&self) -> &str;
|
||||
fn model(&self) -> &str;
|
||||
```
|
||||
All implementations store the model as a `String` field and the id as a string literal, so returning `&str` is straightforward.
|
||||
**Why:** Eliminates unnecessary heap allocation on every call. These methods are called frequently (every `build_request` call).
|
||||
|
||||
---
|
||||
|
||||
## 6. Naming and Clarity
|
||||
|
||||
### 6.1 EnvContext fields lack consistent naming
|
||||
|
||||
**File:** `crates/coding-agent-loop/src/profiles/mod.rs`, lines 13-21
|
||||
**Current:** The struct has fields `date`, `model_name`, `knowledge_cutoff` alongside `git_branch`, `is_git_repo`. The non-git fields use varying naming conventions -- `date` is vague (what date?), `model_name` is redundant (just `model` would match `ProviderProfile::model()`).
|
||||
**Simplification:** Rename `date` to `today` or `current_date`, and `model_name` to `model` for consistency with the trait method name.
|
||||
**Why:** Clearer intent and consistent naming.
|
||||
|
||||
### 6.2 Turn::Steering and Turn::System are semantically close
|
||||
|
||||
**File:** `crates/coding-agent-loop/src/types.rs`, lines 5-29
|
||||
**Current:** `Turn::System` and `Turn::Steering` both represent injected content. `System` maps to `Role::System` in the LLM message, while `Steering` maps to `Role::User`.
|
||||
**Simplification:** No code change needed, but adding a brief doc comment to each variant clarifying the distinction would help. Currently there's no documentation explaining when to use which.
|
||||
**Why:** A reader must trace through `convert_to_messages()` to understand the difference.
|
||||
|
||||
---
|
||||
|
||||
## 7. Tool Construction Boilerplate
|
||||
|
||||
### 7.1 Repeated parameter extraction pattern in tool executors
|
||||
|
||||
**Files:** `crates/coding-agent-loop/src/tools.rs` (lines 26-28, 61-66, 94-106, 168-175, 216-237, 263-269) and `crates/coding-agent-loop/src/subagent.rs` (lines 189-193, 237-244, 275-278, 312-315)
|
||||
|
||||
**Current:** Every tool executor manually extracts parameters with the same pattern:
|
||||
```rust
|
||||
let param = args.get("param")
|
||||
.and_then(|v| v.as_str())
|
||||
.ok_or_else(|| "Missing required parameter: param".to_string())?;
|
||||
```
|
||||
This pattern repeats ~15 times across the codebase with slight variations.
|
||||
**Simplification:** Introduce a small helper:
|
||||
```rust
|
||||
fn required_str<'a>(args: &'a serde_json::Value, key: &str) -> Result<&'a str, String> {
|
||||
args.get(key)
|
||||
.and_then(|v| v.as_str())
|
||||
.ok_or_else(|| format!("Missing required parameter: {key}"))
|
||||
}
|
||||
```
|
||||
**Why:** Reduces boilerplate and ensures consistent error messages across all tools.
|
||||
|
||||
---
|
||||
|
||||
## 8. Dead or Near-Dead Code
|
||||
|
||||
### 8.1 build_env_context_block (no-context variant) has limited use
|
||||
|
||||
**File:** `crates/coding-agent-loop/src/profiles/mod.rs`, lines 50-53
|
||||
**Current:** `build_env_context_block` wraps `build_env_context_block_with` with a default `EnvContext`. It's only used in one test.
|
||||
**Simplification:** Inline the default at the one test call site. Or keep it as a convenience but make it `#[cfg(test)]`.
|
||||
**Why:** Public API surface should be intentional. If this is only for tests, mark it as such.
|
||||
|
||||
### 8.2 SubAgent::id() and SubAgent::depth() are only used in tests
|
||||
|
||||
**File:** `crates/coding-agent-loop/src/subagent.rs`, lines 28-35
|
||||
**Current:** `SubAgent` has `id()` and `depth()` accessor methods.
|
||||
**Simplification:** Verify these are used outside tests. If they are only used in the test at line 348 (`manager.get(&agent_id).unwrap().depth()`), consider whether the `get()` method on the manager (and these accessors) serve a real purpose, or if they exist only to test internals.
|
||||
**Why:** Exposing internal state for testing purposes adds API surface that must be maintained.
|
||||
|
||||
### 8.3 Unused import: std::sync::Arc in profiles/openai.rs
|
||||
|
||||
**File:** `crates/coding-agent-loop/src/profiles/openai.rs`, line 9
|
||||
**Current:** `use std::sync::Arc;` is imported at the module level. It's used only in `make_apply_patch_tool()` for the executor closure.
|
||||
**Simplification:** This is fine for production code. Just noting it's not used in the profile implementation itself, only in the private tool factory.
|
||||
**Why:** Minor observation, no action needed.
|
||||
|
||||
---
|
||||
|
||||
## 9. Error Handling
|
||||
|
||||
### 9.1 Inconsistent error types: String vs AgentError
|
||||
|
||||
**Files:** Throughout the crate
|
||||
**Current:** The `ExecutionEnvironment` trait uses `Result<T, String>`, tool executors return `Result<String, String>`, while `Session` methods return `Result<(), AgentError>`. The `SubAgentManager` also uses `Result<T, String>`.
|
||||
**Simplification:** Consider using `AgentError` (or a dedicated `ToolError`) throughout instead of raw `String` errors. At minimum, `SubAgentManager` methods that return user-facing errors should use a typed error.
|
||||
**Why:** `String` errors lose the ability to match on error variants, making programmatic error handling impossible. This is a larger refactor but would significantly improve the API.
|
||||
|
||||
### 9.2 Lock poisoning panics could be handled
|
||||
|
||||
**Files:** `crates/coding-agent-loop/src/session.rs` (lines 143, 149, 199, 354) and `crates/coding-agent-loop/src/subagent.rs` (line 112)
|
||||
**Current:** `.expect("...lock poisoned")` is used on every mutex lock.
|
||||
**Simplification:** This is actually fine for most use cases -- a poisoned lock indicates a panic occurred while the lock was held, which is a serious bug. No change recommended, but documenting the deliberate choice would help.
|
||||
**Why:** No action needed. This is the standard Rust approach.
|
||||
|
||||
---
|
||||
|
||||
## 10. Test Organization
|
||||
|
||||
### 10.1 ProviderTestProfile in provider_profile.rs duplicates TestProfile
|
||||
|
||||
**File:** `crates/coding-agent-loop/src/provider_profile.rs`, lines 86-139
|
||||
**Current:** A `ProviderTestProfile` struct is defined with a custom `build_system_prompt` that includes platform info. It exists alongside `TestProfile` in `test_support.rs`.
|
||||
**Simplification:** If the test-specific behavior (including platform in prompt) matters, add a flag to `TestProfile` to enable it. If not, use `TestProfile` directly.
|
||||
**Why:** The comment on line 85 says "uses distinct id/model and a custom build_system_prompt", but examining the tests, the custom prompt is only checked for containing "linux" and "1" (docs count). These tests could use the shared `TestProfile` with a more flexible assertion.
|
||||
|
||||
### 10.2 Session tests define CapturingProvider inline
|
||||
|
||||
**File:** `crates/coding-agent-loop/src/session.rs`, lines 1225-1254 and 1411-1432
|
||||
**Current:** Two separate inline `CapturingProvider` structs are defined within test functions in the same module. Both capture request data for assertion.
|
||||
**Simplification:** Extract a single `CapturingLlmProvider` into `test_support.rs` that captures the full request, then tests can assert on whichever field they need. The first one (line 1225) captures `reasoning_effort`, the second (line 1411) captures messages. A single mock that captures the full `Request` would serve both.
|
||||
**Why:** Two near-identical mock providers in the same test module is unnecessary duplication.
|
||||
|
||||
---
|
||||
|
||||
## Summary
|
||||
|
||||
| Category | Count | Estimated Lines Saved |
|
||||
|----------|-------|-----------------------|
|
||||
| Duplicated mock implementations | 3 | ~350 |
|
||||
| Duplicated profile boilerplate | 3 | ~120 |
|
||||
| Redundant code | 2 | ~15 |
|
||||
| Control flow simplifications | 3 | ~20 |
|
||||
| Structural improvements | 4 | ~50 (plus perf gains) |
|
||||
| Naming and clarity | 2 | 0 (documentation) |
|
||||
| Tool construction boilerplate | 1 | ~30 |
|
||||
| Dead or near-dead code | 3 | ~20 |
|
||||
| Error handling | 2 | 0 (design decision) |
|
||||
| Test organization | 2 | ~80 |
|
||||
| **Total** | **25** | **~685** |
|
||||
|
||||
The highest-impact simplifications are:
|
||||
1. **Consolidating mock ExecutionEnvironment implementations** (proposals 1.1, 1.2) -- eliminates the most duplicated code
|
||||
2. **Merging TestProfile and ParallelTestProfile** (proposal 2.3) -- reduces test infrastructure
|
||||
3. **Caching system prompt per input cycle** (proposals 5.2, 5.3) -- both a clarity and performance win
|
||||
4. **Returning &str from ProviderProfile::id() and ::model()** (proposal 5.4) -- cleaner API
|
||||
5. **Helper for parameter extraction** (proposal 7.1) -- reduces the most pervasive boilerplate pattern
|
||||
|
|
@ -1,172 +0,0 @@
|
|||
# Coding Agent Loop: Spec Compliance Review
|
||||
|
||||
**Date:** 2026-02-20 (revised after manual verification)
|
||||
**Spec:** `docs/specs/coding-agent-loop-spec.md`
|
||||
**Implementation:** `crates/coding-agent-loop/src/`
|
||||
|
||||
---
|
||||
|
||||
## IMPORTANT: Review Corrections
|
||||
|
||||
The initial automated review by 5 agents contained **many false positives**. Manual
|
||||
verification of every source file revealed that the implementation is far more complete
|
||||
than originally reported. This revised review reflects the actual state of the code.
|
||||
|
||||
---
|
||||
|
||||
## Section 1: Overview and Goals
|
||||
|
||||
**1. (1.3) Architecture** ALIGNED. Session, ProviderProfile, ToolRegistry, ExecutionEnvironment are separate modules with event emission integrated into Session.
|
||||
|
||||
**2. (1.5) SDK Relationship** ALIGNED. Session calls `Client::complete()` directly and manages its own tool loop.
|
||||
|
||||
---
|
||||
|
||||
## Section 2: Agentic Loop
|
||||
|
||||
**3. (2.1) Session Record** ALIGNED. All fields present: `id`, `provider_profile`, `execution_env`, `history`, `event_emitter`, `config`, `state`, `llm_client`, `steering_queue` (Arc<Mutex<VecDeque>>), `followup_queue` (Arc<Mutex<VecDeque>>), `abort_flag`, `project_docs`, `env_context`.
|
||||
|
||||
**4. (2.2) SessionConfig** ALIGNED. All fields and defaults match spec: `max_turns=0`, `max_tool_rounds_per_input=200`, `default_command_timeout_ms=10_000`, `max_command_timeout_ms=600_000`, `reasoning_effort`, `tool_output_limits`, `tool_line_limits`, `enable_loop_detection=true`, `loop_detection_window=10`, `max_subagent_depth=1`, plus `user_instructions`.
|
||||
|
||||
**5. (2.3) SessionState** ALIGNED. All four states exist: `Idle`, `Processing`, `AwaitingInput`, `Closed`. Abort transitions to `Closed`. Auth errors transition to `Closed`. Note: `AwaitingInput` is defined but not auto-detected (host app sets it).
|
||||
|
||||
**6. (2.4) Turn Types** ALIGNED. All five variants exist: `User`, `Assistant` (with `content`, `tool_calls`, `reasoning`, `usage`, `response_id`, `timestamp`), `ToolResults`, `System`, `Steering`. `Steering` turns are converted to user-role messages in `History::convert_to_messages()`.
|
||||
|
||||
**7. (2.5) Core Agentic Loop** ALIGNED. Flow matches spec: append user turn -> `drain_steering()` before first LLM call -> LOOP (check limits -> build request -> call LLM -> record assistant turn -> if no tool calls break -> execute tools -> `drain_steering()` after tool execution -> loop detection).
|
||||
|
||||
**8. (2.6) Steering** ALIGNED. `steer()` queues messages into `steering_queue`. `follow_up()` queues into `followup_queue`. `drain_steering()` drains the queue into `Turn::Steering` entries and emits `SteeringInjected` events. Follow-up messages trigger new processing cycles after current input completes.
|
||||
|
||||
**9. (2.7) Reasoning Effort** ALIGNED. Passed through to LLM request. `set_reasoning_effort()` allows mid-session changes.
|
||||
|
||||
**10. (2.8) Stop Conditions** ALIGNED. All 5 present: natural completion, round limit, turn limit, abort (-> `Closed`), unrecoverable error (auth -> `Closed`).
|
||||
|
||||
**11. (2.9) Event System** ALIGNED. All EventKind variants defined: `SessionStart`, `SessionEnd`, `UserInput`, `AssistantTextStart`, `AssistantTextDelta`, `ToolCallOutputDelta`, `AssistantTextEnd`, `ToolCallStart`, `ToolCallEnd`, `SteeringInjected`, `TurnLimit`, `LoopDetection`, `ContextWindowWarning`, `Error`. `AssistantTextStart` emitted before LLM call. `ToolCallEnd` carries full untruncated output; truncation applied afterward for history. Note: `AssistantTextDelta` and `ToolCallOutputDelta` are defined but not emitted (requires streaming support in the loop, which uses `complete()` not `stream()`).
|
||||
|
||||
**12. (2.10) Loop Detection** ALIGNED. Checks repeating patterns of length 1, 2, 3. Injects `Turn::Steering` warning. Configurable window (default 10).
|
||||
|
||||
---
|
||||
|
||||
## Section 3: Provider-Aligned Toolsets
|
||||
|
||||
**13. (3.1) Provider Alignment** ALIGNED. Three distinct profiles with provider-specific tools and prompts.
|
||||
|
||||
**14. (3.2) ProviderProfile Interface** ALIGNED. All methods present: `id()`, `model()`, `tool_registry()`, `tool_registry_mut()`, `build_system_prompt()`, `tools()`, `provider_options()`, `supports_reasoning()`, `supports_streaming()`, `supports_parallel_tool_calls()`, `context_window_size()`, `knowledge_cutoff()`.
|
||||
|
||||
**15. (3.3) Shared Core Tools** ALIGNED. All six tools implemented: `read_file` (with `file_path`, `offset`, `limit`), `write_file`, `edit_file` (with `old_string`, `new_string`, `replace_all`), `shell` (with `command`, `timeout_ms`, `description`), `grep` (with `pattern`, `path`, `glob_filter`, `case_insensitive`, `max_results`), `glob` (with `pattern`, `path`).
|
||||
|
||||
**16. (3.4) OpenAI Profile** ALIGNED. Includes `apply_patch` (v4a format), `read_file`, `write_file`, `shell`, `grep`, `glob`, and all 4 subagent tools. System prompt mirrors codex-rs.
|
||||
|
||||
**17. (3.5) Anthropic Profile** ALIGNED. Uses `edit_file` (not `apply_patch`). Shell default timeout set to 120s via `make_shell_tool_with_config(&config)` where `config.default_command_timeout_ms = 120_000`. System prompt mirrors Claude Code including edit_file guidance, 120s timeout documentation, and coding best practices.
|
||||
|
||||
**18. (3.6) Gemini Profile** ALIGNED. All tools present: `read_file`, `read_many_files`, `write_file`, `edit_file`, `shell`, `grep`, `glob`, `list_dir`, `web_search`, `web_fetch`, plus subagent tools. System prompt mirrors gemini-cli with GEMINI.md/AGENTS.md conventions.
|
||||
|
||||
**19. (3.7) Custom Tool Registration** ALIGNED. Latest-wins for name collisions via `HashMap::insert`.
|
||||
|
||||
**20. (3.8) Tool Registry** ALIGNED. Has `register()`, `unregister()`, `get()`, `definitions()`, `names()`. Execution pipeline includes JSON Schema validation via `jsonschema` crate (`validate_tool_args`). Full pipeline: lookup -> validate -> execute -> emit (full output) -> truncate -> return (truncated).
|
||||
|
||||
---
|
||||
|
||||
## Section 4: Tool Execution Environment
|
||||
|
||||
**21. (4.1) ExecutionEnvironment Interface** ALIGNED. All methods present: `read_file(path, offset, limit)`, `write_file(path, content)`, `file_exists(path)`, `list_directory(path, depth)`, `exec_command(command, timeout_ms, working_dir, env_vars)`, `grep(pattern, path, options)`, `glob(pattern, path)`, `initialize()`, `cleanup()`, `working_directory()`, `platform()`, `os_version()`.
|
||||
|
||||
**22. (4.1) ExecResult** ALIGNED. All fields: `stdout`, `stderr`, `exit_code`, `timed_out`, `duration_ms`.
|
||||
|
||||
**23. (4.1) DirEntry** ALIGNED. All fields: `name`, `is_dir`, `size` (Option<u64>).
|
||||
|
||||
**24. (4.2) File Operations** ALIGNED. Direct filesystem via tokio::fs, paths resolved relative to working_directory.
|
||||
|
||||
**25. (4.2) Command Execution** ALIGNED. Spawns in new process group via `setpgid(0, 0)` in `pre_exec`. Uses `/bin/bash -c`. On timeout: SIGTERM to process group (negative PID), wait 2 seconds, then `child.kill()` (SIGKILL). Captures stdout/stderr separately. Records wall-clock `duration_ms`.
|
||||
|
||||
**26. (4.2) Environment Variable Filtering** ALIGNED. Excludes `*_api_key`, `*_secret`, `*_token`, `*_password`, `*_credential` (case-insensitive). Safelist includes: PATH, HOME, USER, SHELL, LANG, TERM, TMPDIR, GOPATH, CARGO_HOME, NVM_DIR. Note: configurable policy (inherit all/none/core) not yet exposed as a public API — filtering is hardcoded.
|
||||
|
||||
**27. (4.2) Search Operations** ALIGNED. Grep uses ripgrep with fallback to grep. Glob uses shell globbing with mtime sort.
|
||||
|
||||
**28. (4.3-4.4) Extension Points** ALIGNED. Trait-based (`#[async_trait]`), composable.
|
||||
|
||||
---
|
||||
|
||||
## Section 5: Tool Output and Context Management
|
||||
|
||||
**29. (5.1) Truncation Algorithm** ALIGNED. `head_tail` and `tail` modes. Warning messages include removed character count.
|
||||
|
||||
**30. (5.2) Default Output Size Limits** ALIGNED. All defaults match spec exactly: `read_file=50000`, `shell=30000`, `grep=20000`, `glob=20000`, `edit_file=10000`, `apply_patch=10000`, `write_file=1000`, `spawn_agent=20000`. Verified by test `default_char_limits_match_spec`.
|
||||
|
||||
**31. (5.3) Truncation Order** ALIGNED. `truncate_tool_output()` runs character-based truncation first, then line-based second. Default line limits: `shell=256`, `grep=200`, `glob=500`. Verified by test `default_line_limits_match_spec`.
|
||||
|
||||
**32. (5.4) Default Command Timeouts** ALIGNED. Matches spec.
|
||||
|
||||
**33. (5.5) Context Window Awareness** ALIGNED. `estimate_token_count()` uses 4-chars-per-token heuristic. `check_context_usage()` emits `ContextWindowWarning` at 80% threshold with `estimated_tokens`, `context_window_size`, and `usage_percent` data. Called after every assistant turn.
|
||||
|
||||
---
|
||||
|
||||
## Section 6: System Prompts and Environment Context
|
||||
|
||||
**34. (6.1) Layered System Prompt** ALIGNED. Five layers: (1) provider base, (2) environment context, (3) tool descriptions, (4) project docs, (5) user instruction overrides.
|
||||
|
||||
**35. (6.2) Provider-Specific Base Instructions** ALIGNED. Each profile has its own base prompt.
|
||||
|
||||
**36. (6.3) Environment Context Block** ALIGNED. `<environment>` block includes: working directory, is git repo, git branch, platform, OS version, today's date, model name, knowledge cutoff. All fields from `EnvContext` are rendered by `build_env_context_block_with()`.
|
||||
|
||||
**37. (6.4) Git Context** ALIGNED. Branch, short status, recent commits (last 10) captured at session start.
|
||||
|
||||
**38. (6.5) Project Document Discovery** ALIGNED. Walks from git root to cwd. Recognizes `AGENTS.md`, `CLAUDE.md`, `GEMINI.md`, `.codex/instructions.md`. Provider-filtered. `AGENTS.md` always loaded. 32KB budget enforced with `[Project instructions truncated at 32KB]` marker. Verified by test `truncates_at_budget`.
|
||||
|
||||
---
|
||||
|
||||
## Section 7: Subagents
|
||||
|
||||
**39. (7.2) Spawn Interface** ALIGNED. All 4 tools present: `spawn_agent` (task, working_dir, model, max_turns), `send_input` (agent_id, message), `wait` (agent_id), `close_agent` (agent_id).
|
||||
|
||||
**40. (7.3) SubAgent Lifecycle** ALIGNED. `SubAgentResult` has `output`, `success`, `turns_used`. Subagents share parent's `ExecutionEnvironment`, get independent history, depth limiting with `max_subagent_depth=1`. Default `max_turns=50` for subagents (set in `make_spawn_agent_tool`).
|
||||
|
||||
---
|
||||
|
||||
## Section 8: Out of Scope
|
||||
|
||||
**41. (8) Out of Scope items** ALIGNED. None implemented.
|
||||
|
||||
---
|
||||
|
||||
## Section 9: Definition of Done Summary
|
||||
|
||||
**42. (9.1) Core Loop** ALIGNED. 8/8 items pass.
|
||||
**43. (9.2) Provider Profiles** ALIGNED. 6/6 items pass.
|
||||
**44. (9.3) Tool Execution** ALIGNED. 5/5 items pass.
|
||||
**45. (9.4) Execution Environment** ALIGNED. 5/6 items pass. Minor gap: env var filtering policy not configurable via public API.
|
||||
**46. (9.5) Tool Output Truncation** ALIGNED. 6/6 items pass.
|
||||
**47. (9.6) Steering** ALIGNED. 4/4 items pass.
|
||||
**48. (9.7) Reasoning Effort** ALIGNED. 3/3 items pass.
|
||||
**49. (9.8) System Prompts** ALIGNED. 6/6 items pass.
|
||||
**50. (9.9) Subagents** ALIGNED. 6/6 items pass.
|
||||
**51. (9.10) Event System** ALIGNED. 3/4 items pass. Minor gap: streaming delta events defined but not emitted (loop uses `complete()` not `stream()`).
|
||||
**52. (9.11) Error Handling** ALIGNED. 5/5 items pass.
|
||||
|
||||
---
|
||||
|
||||
## Summary
|
||||
|
||||
| Category | Status |
|
||||
|---|---|
|
||||
| Section 1: Overview and Goals | ALIGNED |
|
||||
| Section 2: Agentic Loop (all subsections) | ALIGNED |
|
||||
| Section 3: Provider-Aligned Toolsets | ALIGNED |
|
||||
| Section 4: Tool Execution Environment | ALIGNED |
|
||||
| Section 5: Tool Output and Context Management | ALIGNED |
|
||||
| Section 6: System Prompts and Environment Context | ALIGNED |
|
||||
| Section 7: Subagents | ALIGNED |
|
||||
| Section 8: Out of Scope | ALIGNED |
|
||||
| Section 9: Definition of Done | ALIGNED |
|
||||
| **TOTALS** | **50 ALIGNED / 2 MINOR** |
|
||||
|
||||
### Remaining Minor Gaps
|
||||
|
||||
1. **Streaming delta events** — `AssistantTextDelta` and `ToolCallOutputDelta` event kinds are defined but not emitted. The loop uses `Client::complete()` (single-shot) rather than `Client::stream()`. Emitting deltas requires a streaming loop variant. This is a feature enhancement, not a spec violation, since the spec says streaming is optional (`supports_streaming` flag exists).
|
||||
|
||||
2. **Env var filtering policy** — The spec mentions configurable policy (inherit all / inherit none / inherit core). Currently hardcoded. The filtering itself matches spec patterns.
|
||||
|
||||
### Fixes Applied (2026-02-20)
|
||||
|
||||
1. Added `knowledge_cutoff()` method to `ProviderProfile` trait, implemented in all three profiles (Anthropic: "May 2025", Gemini: "January 2025", OpenAI: "April 2025"). Session now populates `EnvContext.knowledge_cutoff` from the profile.
|
||||
|
||||
2. Set subagent default `max_turns` to 50 in `make_spawn_agent_tool` (was using session factory default).
|
||||
|
|
@ -1,181 +0,0 @@
|
|||
# Attractor Spec: Confirmed Gaps Report
|
||||
|
||||
**Date:** 2026-02-21
|
||||
**Method:** 3 parallel agents investigated 19 claimed gaps, reading source code and citing exact file:line evidence.
|
||||
|
||||
**Result: 16 CONFIRMED / 2 REFUTED / 1 PARTIAL**
|
||||
|
||||
---
|
||||
|
||||
## Hard Gaps (3/3 confirmed)
|
||||
|
||||
### 1. Auto Status — engine never checks `auto_status` attribute
|
||||
|
||||
**CONFIRMED**
|
||||
|
||||
The `auto_status()` accessor exists at `graph/types.rs:193-194` but is never referenced in `engine.rs` or any handler. A grep for `auto_status` across all source returns only the accessor definition and its unit test — zero engine references. The spec (line 162, Appendix C line 2111) says: when `auto_status=true` and no `status.json` was written by the handler, the engine should synthesize `{"outcome": "success", "notes": "auto-status: handler completed without writing status"}`. This does not happen.
|
||||
|
||||
### 2. Timeout Enforcement — no deadline around handler execution
|
||||
|
||||
**CONFIRMED**
|
||||
|
||||
`tokio::time::timeout` appears in exactly two places: `handler/tool.rs:67` (subprocess timeout) and `interviewer/mod.rs:142` (human input timeout). The engine's `execute_with_retry` at `engine.rs:449-536` wraps handler execution in `catch_unwind` for panic safety (line 464) but has **no** `tokio::time::timeout` wrapper. A grep for `timeout` in `engine.rs` returns zero matches. All non-tool, non-interviewer handlers (codergen, manager_loop, parallel, etc.) can run indefinitely.
|
||||
|
||||
### 3. Manager Loop `child_autostart` — not implemented
|
||||
|
||||
**CONFIRMED**
|
||||
|
||||
The spec (lines 961-963) says: `IF node.attrs.get("stack.child_autostart", "true") == "true": start_child_pipeline(child_dotfile)`. A codebase-wide grep for `child_autostart`, `start_child_pipeline`, and `child_dotfile` returns matches only in the spec itself and the review document. `handler/manager_loop.rs` goes directly into its observation loop without any auto-start logic. The `ChildObserver` trait (lines 16-22) provides `observe` and `steer` but no launch capability. Sub-gap: `steer_cooldown_elapsed()` is also missing — zero matches for `steer_cooldown` anywhere.
|
||||
|
||||
---
|
||||
|
||||
## Minor Gaps (13/16 confirmed, 2 refuted, 1 partial)
|
||||
|
||||
### 4. Thread ID Resolution — only step 1 of 5
|
||||
|
||||
**CONFIRMED**
|
||||
|
||||
The spec (lines 1196-1206) defines 5-step thread resolution for `full` fidelity. `engine.rs:660-664` only handles step 1 (node `thread_id`). Missing:
|
||||
- Step 2: Edge `thread_id` — accessor exists at `graph/types.rs:262-264` but engine never reads it
|
||||
- Step 3: Graph-level default thread — no `default_thread` accessor (zero grep matches)
|
||||
- Step 4: Derived class from enclosing subgraph — engine never uses `classes` for thread resolution
|
||||
- Step 5: Fallback to previous node ID — not implemented
|
||||
|
||||
### 5. Checkpoint Resume — retry counters not restored
|
||||
|
||||
**CONFIRMED**
|
||||
|
||||
`Checkpoint.node_retries` field exists at `checkpoint.rs:17` but `Checkpoint::from_context()` at line 28 always initializes it as `HashMap::new()`. A grep for `node_retries` in `engine.rs` returns zero matches. The field is never populated during saves and never read during resume. The spec's `reset_retry_counter`/`increment_retry_counter` functions (lines 499-504) do not exist.
|
||||
|
||||
### 6. Checkpoint Resume — fidelity degradation missing
|
||||
|
||||
**CONFIRMED**
|
||||
|
||||
The spec (line 1165) says: "If the previous node used `full` fidelity, degrade to `summary:high` for the first resumed node." A grep for `fidelity.*degrad|summary.high.*resume` across all sources returns zero matches. The resume code at `engine.rs:591-608` performs no fidelity degradation.
|
||||
|
||||
### 7. ~~Retry Policy `should_retry` — too coarse~~ — RESOLVED
|
||||
|
||||
`AttractorError::is_retryable()` classifies errors by variant (`Handler`/`Engine`/`Io` = retryable; `Parse`/`Validation`/`Stylesheet`/`Checkpoint`/`Cancelled` = terminal). The `Handler` trait now has a `should_retry(&self, err: &AttractorError) -> bool` method (default delegates to `is_retryable()`). The engine's `execute_with_retry` calls the handler method directly. Handlers can override to customize retry behavior.
|
||||
|
||||
### 8. Direction type not validated
|
||||
|
||||
**CONFIRMED**
|
||||
|
||||
The BNF defines `Direction ::= 'TB' | 'LR' | 'BT' | 'RL'` (spec line 107). `grammar.rs:57-66` accepts any `identifier = value` as a graph attr declaration. `semantic.rs:178-179` inserts without validation. None of the 14 validation rules check direction values. `rankdir=XY` would be accepted silently.
|
||||
|
||||
### 9. Stylesheet `stylesheet_syntax` lint — brace balance only
|
||||
|
||||
**CONFIRMED**
|
||||
|
||||
`validation/rules.rs:320-348`: the rule counts `{` and `}` characters and errors only if counts differ. It does not call `parse_stylesheet()` from `stylesheet.rs:54-85` which performs full parsing (selector validation, declaration parsing, proper error messages). A stylesheet like `* { garbage garbage }` passes the lint.
|
||||
|
||||
### 10. Stylesheet — undocumented Shape selector
|
||||
|
||||
**CONFIRMED**
|
||||
|
||||
The spec grammar (line 1497) defines: `Selector ::= '*' | '#' Identifier | '.' ClassName` — three types. `stylesheet.rs:6-15` defines four: `Universal`, `Shape(String)`, `Class(String)`, `Id(String)`. The `Shape` selector is parsed at lines 117-131 (bare-word fallback). This shifts specificity: spec says `*`=0, `.class`=1, `#id`=2; impl has `*`=0, `Shape`=1, `.class`=2, `#id`=3. Relative ordering preserved but absolute values differ. Test at line 446 confirms `box { llm_model: opus; }` produces `Selector::Shape("box")`.
|
||||
|
||||
### 11. Fan-in — no score sort, all-fail returns SUCCESS
|
||||
|
||||
**CONFIRMED** (both sub-claims)
|
||||
|
||||
The spec (line 919) sorts by `(outcome_rank, -c.score, c.id)`. `fan_in.rs:103-109` sorts by `(status_rank, id)` — no `score` field on `Candidate` (lines 62-65), zero grep matches for "score". For all-fail: the spec (line 923) says "Only when all candidates fail does fan-in return FAIL." `fan_in.rs:41-58` always builds `Outcome::success()` at line 47 regardless of whether all candidates failed.
|
||||
|
||||
### 12. Preamble transform — applied at parse time, not execution time
|
||||
|
||||
**CONFIRMED**
|
||||
|
||||
The spec (line 1602): "Applied at execution time (not at parse time) since it depends on runtime state." `pipeline.rs:39` calls `PreambleTransform.apply(&mut graph)` in `prepare()` alongside other parse-time transforms. `transform.rs:28-49` reads `fidelity` from static node attributes but has no access to runtime state (e.g., edge-level fidelity overrides resolved at execution time in `engine.rs:656`). If a node has `fidelity="full"` but an incoming edge has `fidelity="truncate"`, the preamble would be incorrectly missing.
|
||||
|
||||
### 13. Pre-hook non-zero — returns fail instead of skip
|
||||
|
||||
**CONFIRMED**
|
||||
|
||||
The spec (line 1693): "non-zero means skip the tool call." `codergen.rs:98-103` returns `Outcome::fail("pre-hook failed, skipping LLM call")` — `StageStatus::Fail` is semantically stronger than skipping. Test at line 302 confirms the outcome status is `Fail`.
|
||||
|
||||
### 14. No parallel fan-out/fan-in integration test
|
||||
|
||||
**CONFIRMED**
|
||||
|
||||
`tests/integration.rs` (1181 lines) contains 11+ test functions covering linear, branching, human gate, goal gate, retry, stylesheet, checkpoint, and smoke test pipelines. None involve `component` (parallel) or `tripleoctagon` (fan-in) shape nodes. A grep for `parallel|fan_in|fan_out` in `crates/attractor/tests/` returns zero matches.
|
||||
|
||||
### 15. Manifest missing `goal` field
|
||||
|
||||
**CONFIRMED**
|
||||
|
||||
The spec (line 1260): "manifest.json -- Pipeline metadata (name, goal, start time)." `engine.rs:217-233` `write_manifest()` writes `pipeline_name`, `start_time`, `node_count`, `edge_count` — no `goal` field despite `graph.goal()` being available. Integration test at lines 1527-1532 confirms the four fields without `goal`.
|
||||
|
||||
### 16. ~~Error categories — no retryable/terminal classification~~ — RESOLVED
|
||||
|
||||
`AttractorError::is_retryable()` at `error.rs:38` classifies variants as retryable (`Handler`, `Engine`, `Io`) or terminal (`Parse`, `Validation`, `Stylesheet`, `Checkpoint`, `Cancelled`). The `Handler` trait's `should_retry` default impl delegates to this method. No `ErrorCategory` enum, but the classification is functionally equivalent.
|
||||
|
||||
### 17. Spec self-contradicts on `default_max_retry`
|
||||
|
||||
**CONFIRMED**
|
||||
|
||||
Spec line 138 (Section 2.5 table): `default_max_retry | Integer | 50`. Spec line 481 (Section 3.5): "Built-in default: 0 (no retries)." Implementation follows Section 2 at `graph/types.rs:349-353` with `.unwrap_or(50)`. Test at `engine.rs:931-936` confirms 51 max attempts (50 retries + 1 initial).
|
||||
|
||||
---
|
||||
|
||||
## Refuted Claims (2)
|
||||
|
||||
### R1. Missing variable handling — claimed as GAP
|
||||
|
||||
**REFUTED** — Correctly implemented.
|
||||
|
||||
`condition.rs:87-110` `resolve_key()` returns `String::new()` for all missing keys (lines 105, 109). Tests at lines 230-239 (`missing_key_compares_as_empty`) and 291-295 (`bare_key_falsy_when_empty`) confirm spec-compliant behavior. The spec (line 1724) says: "Missing keys compare as empty strings" — exactly what the implementation does.
|
||||
|
||||
### R2. Status File Contract — claimed "no implementation found"
|
||||
|
||||
**REFUTED** — Implemented at two layers.
|
||||
|
||||
**Engine-level:** `engine.rs:236-248` `write_node_status()` writes `{node_id}/status.json` with `status`, `notes`, `failure_reason`, `timestamp`. Called at line 702-703 for every node.
|
||||
**Handler-level:** `codergen.rs:111,150` writes richer `status.json` (full `Outcome` serialized).
|
||||
**Tests:** `engine.rs:1536-1551` and `integration.rs:189-198` verify status file existence and contents.
|
||||
|
||||
Minor sub-gap: the engine-level schema uses key `"status"` while the spec's Appendix C uses `"outcome"`, and the engine-level file omits `preferred_next_label`, `suggested_next_ids`, `context_updates`.
|
||||
|
||||
---
|
||||
|
||||
## Partial (1)
|
||||
|
||||
### P1. Checkpoint Resume — functional but incomplete
|
||||
|
||||
**PARTIAL**
|
||||
|
||||
`engine.rs:554-561` `run_from_checkpoint()` exists and is callable. Resume logic at lines 591-608 restores context, logs, completed_nodes, and continues from the next node.
|
||||
|
||||
**What works:** Basic resume for simple linear pipelines.
|
||||
|
||||
**What's missing:**
|
||||
1. `node_retries` ignored during resume (see gap 5)
|
||||
2. `node_outcomes` not restored — initialized as empty HashMap at line 586, causing goal gate checks to miss pre-checkpoint outcomes
|
||||
3. Edge selection during resume picks first outgoing edge (line 603-608), ignoring conditions — wrong successor for conditional graphs
|
||||
4. No integration test calls `run_from_checkpoint` — only `Checkpoint::save`/`load` is tested
|
||||
|
||||
---
|
||||
|
||||
## Summary Table
|
||||
|
||||
| # | Gap | Verdict | Severity |
|
||||
|---|-----|---------|----------|
|
||||
| 1 | Auto status not enforced | CONFIRMED | Hard |
|
||||
| 2 | Timeout not enforced in engine | CONFIRMED | Hard |
|
||||
| 3 | Manager loop child_autostart missing | CONFIRMED | Hard |
|
||||
| 4 | Thread ID resolution 1/5 steps | CONFIRMED | Moderate |
|
||||
| 5 | Checkpoint retry counters not persisted | CONFIRMED | Moderate |
|
||||
| 6 | Checkpoint fidelity degradation missing | CONFIRMED | Moderate |
|
||||
| 7 | ~~should_retry retries all errors~~ | RESOLVED | ~~Moderate~~ |
|
||||
| 8 | Direction values not validated | CONFIRMED | Low |
|
||||
| 9 | Stylesheet lint brace-balance only | CONFIRMED | Low |
|
||||
| 10 | Undocumented Shape selector | CONFIRMED | Low |
|
||||
| 11 | Fan-in: no score sort, all-fail=SUCCESS | CONFIRMED | Moderate |
|
||||
| 12 | Preamble at parse time not runtime | CONFIRMED | Moderate |
|
||||
| 13 | Pre-hook fail instead of skip | CONFIRMED | Low |
|
||||
| 14 | No parallel integration test | CONFIRMED | Low |
|
||||
| 15 | Manifest missing goal field | CONFIRMED | Low |
|
||||
| 16 | ~~No error retryable/terminal classification~~ | RESOLVED | ~~Moderate~~ |
|
||||
| 17 | Spec contradicts itself on default_max_retry | CONFIRMED | Low (spec bug) |
|
||||
| P1 | Checkpoint resume incomplete | PARTIAL | Moderate |
|
||||
| R1 | Missing variable handling | REFUTED | — |
|
||||
| R2 | Status file contract | REFUTED | — |
|
||||
|
|
@ -1,224 +0,0 @@
|
|||
# Attractor Spec Compliance Review
|
||||
|
||||
**Date**: 2026-02-21
|
||||
**Spec**: `docs/specs/attractor-spec.md`
|
||||
**Implementation**: `crates/attractor/src/`
|
||||
|
||||
---
|
||||
|
||||
## Section 1: Overview and Goals
|
||||
|
||||
| # | Subsection | Verdict |
|
||||
|---|-----------|---------|
|
||||
| 1 | 1.1 Problem Statement | ALIGNED |
|
||||
| 2 | 1.2 Why DOT Syntax | ALIGNED |
|
||||
| 3 | 1.3 Design Principles | ALIGNED |
|
||||
| 4 | 1.4 Layering and LLM Backends | ALIGNED |
|
||||
|
||||
**Details**: All five design principles are implemented: declarative pipelines (DOT parsed into `Graph`, engine handles execution), pluggable handlers (`HandlerRegistry`), checkpoint/resume (`checkpoint.rs`), human-in-the-loop (`interviewer/` module), edge-based routing (`select_edge()` in `engine.rs`). `CodergenBackend` trait decouples LLM integration. `EventEmitter` provides the event stream for frontends.
|
||||
|
||||
---
|
||||
|
||||
## Section 2: DOT DSL Schema
|
||||
|
||||
| # | Subsection | Verdict |
|
||||
|---|-----------|---------|
|
||||
| 5 | 2.1 Supported Subset | ALIGNED |
|
||||
| 6 | 2.2 BNF-Style Grammar | ALIGNED |
|
||||
| 7 | 2.3 Key Constraints | ALIGNED |
|
||||
| 8 | 2.4 Value Types | ALIGNED |
|
||||
| 9 | 2.5 Graph Attributes | ALIGNED |
|
||||
| 10 | 2.6 Node Attributes | ALIGNED |
|
||||
| 11 | 2.7 Edge Attributes | ALIGNED |
|
||||
| 12 | 2.8 Shape-to-Handler Mapping | ALIGNED |
|
||||
| 13 | 2.9 Chained Edges | ALIGNED |
|
||||
| 14 | 2.10 Subgraphs | ALIGNED |
|
||||
| 15 | 2.11 Node/Edge Default Blocks | ALIGNED |
|
||||
| 16 | 2.12 Class Attribute | ALIGNED |
|
||||
| 17 | 2.13 Minimal Examples | ALIGNED |
|
||||
|
||||
**Minor note**: The parser does not produce a specific error message when `strict digraph` is used — it simply fails to parse. Functionally correct but a UX gap for error messaging.
|
||||
|
||||
---
|
||||
|
||||
## Section 3: Pipeline Execution Engine
|
||||
|
||||
| # | Subsection | Verdict |
|
||||
|---|-----------|---------|
|
||||
| 18 | 3.1 Run Lifecycle (5 phases) | ALIGNED |
|
||||
| 19 | 3.2 Core Execution Loop | ALIGNED |
|
||||
| 20 | 3.3 Edge Selection Algorithm (5-step priority) | ALIGNED |
|
||||
| 21 | 3.4 Goal Gate Enforcement | ALIGNED |
|
||||
| 22 | 3.5 Retry Logic | ALIGNED |
|
||||
| 23 | 3.6 Retry Policies (5 presets) | ALIGNED |
|
||||
| 24 | 3.7 Failure Routing | ALIGNED |
|
||||
| 25 | 3.8 Concurrency Model | ALIGNED |
|
||||
|
||||
**Minor note**: The 5 named retry presets (none, standard, aggressive, linear, patient) exist as constructors on `RetryPolicy` but `build_retry_policy()` always constructs a custom policy from `max_retries` + default backoff. There is no mechanism for a node to select a preset by name (e.g. `retry_policy="aggressive"`).
|
||||
|
||||
---
|
||||
|
||||
## Section 4: Node Handlers
|
||||
|
||||
| # | Subsection | Verdict |
|
||||
|---|-----------|---------|
|
||||
| 26 | 4.1 Handler Interface | ALIGNED |
|
||||
| 27 | 4.2 Handler Registry | ALIGNED |
|
||||
| 28 | 4.3 Start Handler | ALIGNED |
|
||||
| 29 | 4.4 Exit Handler | ALIGNED |
|
||||
| 30 | 4.5 Codergen Handler | ALIGNED |
|
||||
| 31 | 4.6 Wait For Human Handler | ALIGNED |
|
||||
| 32 | 4.7 Conditional Handler | ALIGNED |
|
||||
| 33 | 4.8 Parallel Handler | ALIGNED |
|
||||
| 34 | 4.9 Fan-In Handler | ALIGNED |
|
||||
| 35 | 4.10 Tool Handler | ALIGNED |
|
||||
| 36 | 4.11 Manager Loop Handler | ALIGNED |
|
||||
| 37 | 4.12 Custom Handlers | ALIGNED |
|
||||
|
||||
**Minor note**: Manager loop reads `stack.child_dotfile` from node attrs rather than graph attrs as the spec pseudocode shows. Arguably better design (per-node child pipeline), but deviates from spec.
|
||||
|
||||
---
|
||||
|
||||
## Section 5: State and Context
|
||||
|
||||
| # | Subsection | Verdict |
|
||||
|---|-----------|---------|
|
||||
| 38 | 5.1 PipelineContext | GAP |
|
||||
| 39 | 5.2 Outcome | ALIGNED |
|
||||
| 40 | 5.3 Checkpoint | ALIGNED |
|
||||
| 41 | 5.4 Context Fidelity | GAP |
|
||||
| 42 | 5.5 Artifact Store | ALIGNED |
|
||||
| 43 | 5.6 Run Directory Structure | GAP |
|
||||
|
||||
**GAP 38 — `last_stage` / `last_response` context keys**: The spec defines these as engine-set context keys. The engine does not set them; only the `codergen` handler sets them. Other handler types do not propagate these keys. Additionally, `internal.retry_count.<node_id>` is tracked in a separate `node_retries` HashMap rather than as a context key.
|
||||
|
||||
**GAP 41 — Thread resolution step 3**: The spec lists "Graph-level default thread" as step 3 in thread ID resolution. The implementation uses the node's first CSS class instead. No graph-level default thread concept is implemented.
|
||||
|
||||
**GAP 43 — Per-node `prompt.md` / `response.md`**: The spec defines these as part of the run directory structure. The engine does not write them; only the `codergen` handler does. Other LLM-interacting handlers (fan_in with LLM evaluation) do not write these files.
|
||||
|
||||
---
|
||||
|
||||
## Section 6: Human-in-the-Loop (Interviewer Pattern)
|
||||
|
||||
| # | Subsection | Verdict |
|
||||
|---|-----------|---------|
|
||||
| 44 | 6.1 Interviewer Interface | ALIGNED |
|
||||
| 45 | 6.2 Question Model | ALIGNED |
|
||||
| 46 | 6.3 Answer Model | ALIGNED |
|
||||
| 47 | 6.4 Built-In Implementations (5) | ALIGNED |
|
||||
| 48 | 6.5 Timeout Handling | GAP |
|
||||
| 49 | 6.6 Gate Node Behavior | ALIGNED |
|
||||
|
||||
**GAP 48 — WaitHumanHandler bypasses timeout**: `ask_with_timeout()` exists as a utility function but `WaitHumanHandler` calls `interviewer.ask()` directly, so `timeout_seconds` on questions is not enforced for human gate interactions.
|
||||
|
||||
---
|
||||
|
||||
## Section 7: Validation and Linting
|
||||
|
||||
| # | Subsection | Verdict |
|
||||
|---|-----------|---------|
|
||||
| 50 | 7.1 Diagnostic Model | ALIGNED |
|
||||
| 51 | 7.2 Built-In Rules (14 rules) | ALIGNED |
|
||||
| 52 | 7.3 Validation API | ALIGNED |
|
||||
| 53 | 7.4 Custom Lint Rules | ALIGNED |
|
||||
|
||||
**Details**: All 14 spec rules implemented plus a bonus `direction_valid` rule (15 total). Error-severity diagnostics block execution. Custom rules supported via `extra_rules` parameter.
|
||||
|
||||
---
|
||||
|
||||
## Section 8: Model Stylesheet
|
||||
|
||||
| # | Subsection | Verdict |
|
||||
|---|-----------|---------|
|
||||
| 54 | 8.1 Purpose | ALIGNED |
|
||||
| 55 | 8.2 Grammar | ALIGNED |
|
||||
| 56 | 8.3 Selectors and Specificity | ALIGNED |
|
||||
| 57 | 8.4 Recognized Properties | ALIGNED |
|
||||
| 58 | 8.5 Application/Resolution Order | ALIGNED |
|
||||
|
||||
**Details**: Specificity correctly implemented (Universal=0, Class=1, Id=2). Explicit node attributes are never overridden. Stylesheet applied as a transform before validation.
|
||||
|
||||
---
|
||||
|
||||
## Section 9: Transforms and Extensibility
|
||||
|
||||
| # | Subsection | Verdict |
|
||||
|---|-----------|---------|
|
||||
| 59 | 9.1 AST Transforms | ALIGNED |
|
||||
| 60 | 9.2 Built-In Transforms (3) | ALIGNED |
|
||||
| 61 | 9.3 Custom Transforms | ALIGNED |
|
||||
| 62 | 9.4 Pipeline Composition | ALIGNED |
|
||||
| 63 | 9.5 HTTP Server Mode | GAP |
|
||||
| 64 | 9.6 Observability and Events | ALIGNED |
|
||||
| 65 | 9.7 Tool Call Hooks | GAP |
|
||||
|
||||
**GAP 63 — HTTP Server Mode**: No HTTP server implementation. The spec says "Implementations may expose" making this optional, but it is unimplemented.
|
||||
|
||||
**GAP 65 — Tool Call Hooks**: `tool_hooks.pre` and `tool_hooks.post` are defined in the spec for shell commands around LLM tool calls. Not implemented in any handler. Pre-hook should gate tool calls (non-zero exit = skip), post-hook for logging/auditing.
|
||||
|
||||
**Minor note**: Transform trait uses `&mut Graph` (in-place mutation) rather than returning a new graph as spec describes. Functionally equivalent.
|
||||
|
||||
---
|
||||
|
||||
## Section 10: Condition Expression Language
|
||||
|
||||
| # | Subsection | Verdict |
|
||||
|---|-----------|---------|
|
||||
| 66 | 10.1 Overview | ALIGNED |
|
||||
| 67 | 10.2 Grammar | ALIGNED |
|
||||
| 68 | 10.3 Semantics | ALIGNED |
|
||||
| 69 | 10.4 Variable Resolution | ALIGNED |
|
||||
| 70 | 10.5 Evaluation | ALIGNED |
|
||||
| 71 | 10.6 Examples | ALIGNED |
|
||||
| 72 | 10.7 Extended Operators (future) | ALIGNED |
|
||||
|
||||
**Details**: Full implementation with `=`, `!=`, `&&` conjunction, bare key truthiness, `context.*` double-lookup. Correctly does not implement future operators.
|
||||
|
||||
---
|
||||
|
||||
## Section 11: Definition of Done
|
||||
|
||||
| # | Subsection | Verdict |
|
||||
|---|-----------|---------|
|
||||
| 73 | 11.1 DOT Parsing | ALIGNED |
|
||||
| 74 | 11.2 Validation and Linting | ALIGNED |
|
||||
| 75 | 11.3 Execution Engine | ALIGNED |
|
||||
| 76 | 11.4 Goal Gate Enforcement | ALIGNED |
|
||||
| 77 | 11.5 Retry Logic | ALIGNED |
|
||||
| 78 | 11.6 Node Handlers | ALIGNED |
|
||||
| 79 | 11.7 State and Context | ALIGNED |
|
||||
| 80 | 11.8 Human-in-the-Loop | ALIGNED |
|
||||
| 81 | 11.9 Condition Expressions | ALIGNED |
|
||||
| 82 | 11.10 Model Stylesheet | ALIGNED |
|
||||
| 83 | 11.11 Transforms and Extensibility | ALIGNED |
|
||||
| 84 | 11.12 Cross-Feature Parity Matrix | ALIGNED |
|
||||
| 85 | 11.13 Integration Smoke Test | ALIGNED |
|
||||
|
||||
---
|
||||
|
||||
## Summary
|
||||
|
||||
| Category | Count |
|
||||
|----------|-------|
|
||||
| Total items reviewed | 85 |
|
||||
| ALIGNED | 78 |
|
||||
| GAP | 7 |
|
||||
| Alignment rate | 91.8% |
|
||||
|
||||
### All Gaps
|
||||
|
||||
| # | Section | Gap | Severity |
|
||||
|---|---------|-----|----------|
|
||||
| 38 | 5.1 | `last_stage`/`last_response` not set by engine; `internal.retry_count` not in context | Low |
|
||||
| 41 | 5.4 | Thread resolution missing graph-level default thread (step 3) | Low |
|
||||
| 43 | 5.6 | `prompt.md`/`response.md` only written by codergen, not other LLM handlers | Low |
|
||||
| 48 | 6.5 | WaitHumanHandler calls `ask()` directly, bypassing `ask_with_timeout()` | Medium |
|
||||
| 63 | 9.5 | HTTP server mode not implemented (spec marks as optional) | Low |
|
||||
| 65 | 9.7 | `tool_hooks.pre`/`tool_hooks.post` not implemented | Medium |
|
||||
|
||||
### Minor Notes (not gaps, but deviations)
|
||||
|
||||
- No specific error for `strict digraph` (parser just fails)
|
||||
- Named retry presets exist but no node-level attribute to select them
|
||||
- Manager loop reads `child_dotfile` from node attrs not graph attrs
|
||||
- Transform trait mutates in-place rather than returning new graph
|
||||
|
|
@ -1,701 +0,0 @@
|
|||
# Attractor Spec Compliance Review
|
||||
|
||||
**Date:** 2026-02-21
|
||||
**Spec:** `docs/specs/attractor-spec.md`
|
||||
**Implementation:** `crates/attractor/src/`
|
||||
**Reviewers:** 5 parallel agents, each covering distinct spec sections
|
||||
|
||||
---
|
||||
|
||||
## Section 1: Overview and Goals
|
||||
|
||||
### 1. Section 1.1 — Problem Statement
|
||||
**ALIGNED**
|
||||
The implementation delivers a DOT-based directed-graph pipeline runner as described. No code artifact required beyond the overall architecture.
|
||||
|
||||
### 2. Section 1.2 — Why DOT Syntax
|
||||
**ALIGNED**
|
||||
The parser (`parser/grammar.rs`) starts with `digraph` keyword and builds on directed graph primitives. DOT subset parser implemented from scratch.
|
||||
|
||||
### 3. Section 1.3 — Design Principles
|
||||
**ALIGNED**
|
||||
- Declarative pipelines: `.dot` files declare graph structure; engine traverses it (`parser/mod.rs:18`, `graph/types.rs:277-294`)
|
||||
- Pluggable handlers: `Node::handler_type()` at `graph/types.rs:203-209` resolves from `type` attr or shape mapping
|
||||
- Checkpoint and resume: `checkpoint` module (`lib.rs:2`), checkpoint save/load implemented
|
||||
- Human-in-the-loop: `interviewer` module (`lib.rs:10`), `wait.human` mapped at `graph/types.rs:77`
|
||||
- Edge-based routing: `Edge` struct has `condition()`, `weight()`, `label()` accessors at `graph/types.rs:241-274`
|
||||
|
||||
### 4. Section 1.4 — Layering and LLM Backends
|
||||
**ALIGNED**
|
||||
No LLM SDK dependency. `CodergenBackend` trait decouples LLM calls. Event stream module at `lib.rs:8`.
|
||||
|
||||
---
|
||||
|
||||
## Section 2: DOT DSL Schema
|
||||
|
||||
### 5. Section 2.1 — Supported Subset
|
||||
**ALIGNED**
|
||||
Parser accepts only `digraph` (`grammar.rs:140`). No code path for `graph` (undirected) or `strict`. Trailing content rejected at `parser/mod.rs:23-29`.
|
||||
|
||||
### 6. Section 2.2 — BNF Grammar
|
||||
**ALIGNED** (minor gap)
|
||||
All grammar productions verified against implementation:
|
||||
- `Graph`, `Statement`, `GraphAttrStmt`, `NodeDefaults`, `EdgeDefaults`, `GraphAttrDecl`, `SubgraphStmt`, `NodeStmt`, `EdgeStmt`, `AttrBlock`, `Attr` — all match at `grammar.rs:14-152`
|
||||
- `QualifiedId` (dotted keys) supported at `lexer.rs:90-113`
|
||||
- All value types including Duration with `ms/s/m/h/d` at `lexer.rs:229-246`
|
||||
- **Minor gap:** `Direction` type (`TB|LR|BT|RL`) parsed as bare `AstValue::Ident` — no validation restricting to valid values. Functionally works but invalid directions accepted silently.
|
||||
|
||||
### 7. Section 2.3 — Key Constraints
|
||||
**ALIGNED**
|
||||
- One digraph per file: trailing content check at `parser/mod.rs:23-29`
|
||||
- Bare identifiers: `[A-Za-z_][A-Za-z0-9_]*` at `lexer.rs:82-87`
|
||||
- Commas required: `separated_list1(preceded(ws, char(',')), attr)` at `grammar.rs:26-31`
|
||||
- Directed edges only: only `->` parsed at `lexer.rs:262-264`
|
||||
- Comments: `strip_comments()` at `lexer.rs:5-53` handles `//` and `/* */`
|
||||
- Semicolons optional: `opt_semi` at `grammar.rs:34-36`
|
||||
|
||||
### 8. Section 2.4 — Value Types
|
||||
**ALIGNED**
|
||||
All five types implemented with correct syntax:
|
||||
| Type | Implementation |
|
||||
|------|---------------|
|
||||
| String | `lexer.rs:121-177` with `\"`, `\n`, `\t`, `\\` escapes |
|
||||
| Integer | `lexer.rs:208-226` with sign and float-rejection |
|
||||
| Float | `lexer.rs:193-205` |
|
||||
| Boolean | `lexer.rs:180-190` |
|
||||
| Duration | `lexer.rs:229-246` with `ms/s/m/h/d` units |
|
||||
|
||||
### 9. Section 2.5 — Graph-Level Attributes
|
||||
**ALIGNED**
|
||||
All 7 attributes present with correct defaults:
|
||||
| Key | Evidence |
|
||||
|-----|----------|
|
||||
| `goal` | `graph/types.rs:333-338` — default `""` |
|
||||
| `label` | stored as generic attr |
|
||||
| `model_stylesheet` | `graph/types.rs:341-346` — default `""` |
|
||||
| `default_max_retry` | `graph/types.rs:349-354` — default `50` |
|
||||
| `retry_target` | `graph/types.rs:357-359` |
|
||||
| `fallback_retry_target` | `graph/types.rs:361-366` |
|
||||
| `default_fidelity` | `graph/types.rs:369-373` |
|
||||
|
||||
### 10. Section 2.6 — Node Attributes
|
||||
**ALIGNED**
|
||||
All 18 attributes present with correct defaults:
|
||||
| Key | Evidence |
|
||||
|-----|----------|
|
||||
| `label` | `graph/types.rs:119-121` — falls back to node ID |
|
||||
| `shape` | `graph/types.rs:124-126` — default `"box"` |
|
||||
| `type` | `graph/types.rs:129-131` |
|
||||
| `prompt` | `graph/types.rs:134-136` |
|
||||
| `max_retries` | `graph/types.rs:139-141` — `None` when unset |
|
||||
| `goal_gate` | `graph/types.rs:144-146` — default `false` |
|
||||
| `retry_target` | `graph/types.rs:149-151` |
|
||||
| `fallback_retry_target` | `graph/types.rs:153-156` |
|
||||
| `fidelity` | `graph/types.rs:159-161` |
|
||||
| `thread_id` | `graph/types.rs:164-166` |
|
||||
| `class` | `graph/types.rs:169-171` + parsing at `semantic.rs:103-117` |
|
||||
| `timeout` | `graph/types.rs:173-175` — `Option<Duration>` |
|
||||
| `llm_model` | `graph/types.rs:178-180` |
|
||||
| `llm_provider` | `graph/types.rs:183-185` |
|
||||
| `reasoning_effort` | `graph/types.rs:188-190` — default `"high"` |
|
||||
| `auto_status` | `graph/types.rs:193-195` — default `false` |
|
||||
| `allow_partial` | `graph/types.rs:198-200` — default `false` |
|
||||
|
||||
### 11. Section 2.7 — Edge Attributes
|
||||
**ALIGNED**
|
||||
All 7 attributes present:
|
||||
| Key | Evidence |
|
||||
|-----|----------|
|
||||
| `label` | `graph/types.rs:242-244` |
|
||||
| `condition` | `graph/types.rs:247-249` |
|
||||
| `weight` | `graph/types.rs:252-254` — default `0` |
|
||||
| `fidelity` | `graph/types.rs:257-259` |
|
||||
| `thread_id` | `graph/types.rs:262-264` |
|
||||
| `loop_restart` | `graph/types.rs:267-269` — default `false` |
|
||||
| `freeform` | `graph/types.rs:272-274` — default `false` |
|
||||
|
||||
### 12. Section 2.8 — Shape-to-Handler Mapping
|
||||
**ALIGNED**
|
||||
All 9 mappings present at `graph/types.rs:72-85`. Handler resolution with `type` override at `graph/types.rs:203-209`.
|
||||
|
||||
### 13. Section 2.9 — Chained Edges
|
||||
**ALIGNED**
|
||||
Parsed at `grammar.rs:86-101`, expanded via `windows(2)` at `semantic.rs:132-141`. Test at `semantic.rs:471-484` confirms correct desugaring.
|
||||
|
||||
### 14. Section 2.10-2.12 — Subgraphs, Defaults, Class Attribute
|
||||
**ALIGNED**
|
||||
- Subgraph scoping: `semantic.rs:145-235` saves/restores defaults per scope
|
||||
- Class derivation from subgraph label: `semantic.rs:51-58` (`derive_class_from_label`)
|
||||
- Comma-separated class parsing: `semantic.rs:103-117`, tested at `semantic.rs:487-500`
|
||||
- Node/edge default blocks: `grammar.rs:44-54`, applied in `semantic.rs:76-83`
|
||||
|
||||
---
|
||||
|
||||
## Section 3: Pipeline Execution Engine
|
||||
|
||||
### 15. Section 3.1 — Run Lifecycle
|
||||
**ALIGNED**
|
||||
Five phases implemented:
|
||||
- PARSE: `pipeline.rs:34` calls `crate::parser::parse(dot_source)`
|
||||
- VALIDATE: `pipeline.rs:46` calls `validation::validate(&graph, &[])`
|
||||
- INITIALIZE: `engine.rs:580-625` creates run directory, initializes context
|
||||
- EXECUTE: `engine.rs:627-771` main loop
|
||||
- FINALIZE: `engine.rs:773-784` emits `PipelineCompleted`, returns outcome
|
||||
|
||||
### 16. Section 3.2 — Core Execution Loop
|
||||
**ALIGNED**
|
||||
All 8 steps from spec implemented:
|
||||
1. Start node resolution: `engine.rs:620-624` via `graph.find_start_node()` at `graph/types.rs:310-320`
|
||||
2. Terminal check: `engine.rs:633-653`
|
||||
3. Execute with retry: `engine.rs:669-679`
|
||||
4. Record completion: `engine.rs:706-708`
|
||||
5. Apply context updates: `engine.rs:711-715`
|
||||
6. Save checkpoint: `engine.rs:718-730`
|
||||
7. Select next edge: `engine.rs:733-753`
|
||||
8. Loop restart: `engine.rs:760-767`, advance: `engine.rs:768`
|
||||
|
||||
### 17. Section 3.3 — Edge Selection Algorithm
|
||||
**ALIGNED**
|
||||
Five-step priority fully implemented at `engine.rs:299-356`:
|
||||
1. Condition matching: `engine.rs:311-321`
|
||||
2. Preferred label: `engine.rs:324-333` with `normalize_label` at `engine.rs:254-279`
|
||||
3. Suggested next IDs: `engine.rs:336-342`
|
||||
4. Weight + lexical tiebreak: `engine.rs:345-352` via `best_by_weight_then_lexical` at `engine.rs:282-295`
|
||||
5. Fallback: `engine.rs:355`
|
||||
|
||||
### 18. Section 3.4 — Goal Gate Enforcement
|
||||
**ALIGNED**
|
||||
- `check_goal_gates` at `engine.rs:362-377`: checks goal_gate nodes for SUCCESS/PARTIAL_SUCCESS
|
||||
- `get_retry_target` at `engine.rs:380-408`: four-level fallback chain (node → node fallback → graph → graph fallback)
|
||||
- `is_terminal` at `engine.rs:411-414`
|
||||
|
||||
### 19. Section 3.5 — Retry Logic
|
||||
**ALIGNED** (minor gaps)
|
||||
- `build_retry_policy` at `engine.rs:178-189`: node `max_retries` with `default_max_retry` fallback
|
||||
- `execute_with_retry` at `engine.rs:449-536`: full retry loop
|
||||
- **Minor:** Retry counters not persisted to `Checkpoint.node_retries` during execution (field exists at `checkpoint.rs:17` but not populated)
|
||||
- **Minor:** Spec self-contradicts on `default_max_retry` default (section 3.5 says 0, section 2 says 50). Implementation follows section 2 (50).
|
||||
|
||||
### 20. Section 3.6 — Retry Policy / Backoff
|
||||
**ALIGNED** (minor gap)
|
||||
- `BackoffConfig` at `engine.rs:28-44` with correct defaults
|
||||
- `delay_for_attempt` at `engine.rs:50-76`: formula matches spec exactly
|
||||
- All 5 presets match spec table (none/standard/aggressive/linear/patient)
|
||||
- **Minor:** Default `should_retry` at `engine.rs:101-103` retries ALL errors. Spec defines granular behavior (retry 429/5xx, fail 401/403/400).
|
||||
|
||||
### 21. Section 3.7 — Failure Routing
|
||||
**ALIGNED**
|
||||
All 4 priority steps at `engine.rs:733-753`: fail edge → retry_target → fallback_retry_target → pipeline termination.
|
||||
|
||||
### 22. Section 3.8 — Concurrency Model
|
||||
**ALIGNED**
|
||||
Single-threaded graph traversal at `engine.rs:627-771`. One node at a time.
|
||||
|
||||
### 23. Section 3.9 — Auto Status
|
||||
**GAP**
|
||||
`auto_status()` accessor exists at `graph/types.rs:193-194` but **engine never references it**. Engine always writes status.json itself at line 703. No auto-synthesis logic for when a handler writes no status.
|
||||
|
||||
### 24. Section 3.10 — Timeout Enforcement
|
||||
**GAP**
|
||||
Node `timeout` attribute parsed as `Option<Duration>` (item 10) but **not enforced during handler execution** in the engine. The tool handler (`tool.rs:61-78`) does use timeout for subprocess execution, but the engine does not wrap general handler execution with a timeout.
|
||||
|
||||
---
|
||||
|
||||
## Section 4: Node Handlers
|
||||
|
||||
### 25. Section 4.1 — Handler Trait
|
||||
**ALIGNED**
|
||||
Trait at `handler/mod.rs:22-31`: `async fn execute(&self, node, context, graph, logs_root) -> Result<Outcome>`. All four parameters match spec. `Result` wrapping for error propagation.
|
||||
|
||||
### 26. Section 4.2 — Handler Registry
|
||||
**ALIGNED**
|
||||
At `handler/mod.rs:34-74`: `HashMap<String, Box<dyn Handler>>`, `default_handler`, `register()` replaces existing, three-step `resolve()` (explicit type → shape → default). Tests confirm at lines 153-177.
|
||||
|
||||
### 27. Section 4.3 — Start Handler
|
||||
**ALIGNED**
|
||||
`handler/start.rs:13-26`: returns `Outcome::success()`. No-op.
|
||||
|
||||
### 28. Section 4.4 — Exit Handler
|
||||
**ALIGNED**
|
||||
`handler/exit.rs:13-26`: returns `Outcome::success()`. No goal gate logic (handled by engine).
|
||||
|
||||
### 29. Section 4.5 — Codergen Handler
|
||||
**ALIGNED**
|
||||
- Prompt building: `codergen.rs:87-91` — `node.prompt()` falling back to `node.label()`
|
||||
- `$goal` expansion: `codergen.rs:42-44`
|
||||
- Log writing: `codergen.rs:94-96` — `prompt.md`, `response.md`, `status.json`
|
||||
- Backend call: `codergen.rs:106-121` — handles `CodergenResult::Full` and `CodergenResult::Text`
|
||||
- Simulation mode when no backend: `codergen.rs:122+`
|
||||
- Context updates `last_stage`/`last_response`: `codergen.rs:139-146`
|
||||
- Tool hooks (pre/post): `codergen.rs:98-131` (enhancement beyond spec)
|
||||
- `CodergenBackend` trait at `codergen.rs:20-27` matches spec's `run(node, prompt, context) -> String | Outcome`
|
||||
|
||||
### 30. Section 4.6 — Wait Human Handler
|
||||
**ALIGNED**
|
||||
Comprehensive implementation at `wait_human.rs`:
|
||||
- Choice derivation from outgoing edges: lines 110-129
|
||||
- Freeform edge detection: line 115
|
||||
- No-edges failure: lines 131-133
|
||||
- Question building with `MultipleChoice`, `allow_freeform`, `stage`: lines 136-150
|
||||
- Timeout handling with default choice fallback: lines 162-179
|
||||
- Skipped handling: lines 183-185
|
||||
- Fixed-choice match with `suggested_next_ids` and context updates: lines 195-201
|
||||
- Freeform fallback: lines 204-221
|
||||
- First-choice fallback: lines 224-226
|
||||
- Accelerator key parsing `[K] Label`, `K) Label`, `K - Label`, first char: lines 32-71
|
||||
|
||||
### 31. Section 4.7 — Conditional Handler
|
||||
**ALIGNED**
|
||||
`handler/conditional.rs:14-28`: no-op returning SUCCESS with note. Routing handled by engine.
|
||||
|
||||
### 32. Section 4.8 — Parallel Handler
|
||||
**ALIGNED**
|
||||
Full implementation at `parallel.rs`:
|
||||
- 4 join policies: `wait_all`, `first_success`, `k_of_n(K)`, `quorum(fraction)` at lines 36-59
|
||||
- 3 error policies: `continue`, `fail_fast`, `ignore` at lines 62-75
|
||||
- `max_parallel` with `Semaphore` for bounded concurrency: lines 113-120
|
||||
- Context isolation via `clone_context()`: line 126
|
||||
- Join evaluation with all four policies: lines 249-286
|
||||
- Fail-fast behavior: lines 185-189
|
||||
- Context storage for fan-in (`parallel.results`, `parallel.branch_count`): lines 230-241
|
||||
|
||||
### 33. Section 4.9 — Fan-In Handler
|
||||
**ALIGNED** (minor gaps)
|
||||
- Reads `parallel.results`: `fan_in.rs:34-37`
|
||||
- LLM-based evaluation when prompt + backend present: lines 41-42
|
||||
- Heuristic selection by status rank: lines 77-115
|
||||
- Context updates `parallel.fan_in.best_id`/`best_outcome`: lines 48-55
|
||||
- **Minor:** No `score`-based sorting in heuristic (spec mentions `-c.score`)
|
||||
- **Minor:** Returns SUCCESS when results exist even if all candidates failed
|
||||
|
||||
### 34. Section 4.10 — Tool Handler
|
||||
**ALIGNED**
|
||||
At `tool.rs`:
|
||||
- Reads `tool_command` from attrs: lines 51-55
|
||||
- Empty command returns fail: lines 57-59
|
||||
- Runs via `sh -c` with timeout support: lines 61-78
|
||||
- Sets `tool.output` to stdout: lines 15-40
|
||||
|
||||
### 35. Section 4.11 — Manager Loop Handler
|
||||
**GAP** (partial)
|
||||
Core observe/steer/wait cycle implemented at `manager_loop.rs:59-157`:
|
||||
- Poll interval, max cycles, stop condition, actions parsing: lines 67-100
|
||||
- Observe/steer delegation: lines 105-115
|
||||
- Child status check: lines 119-132
|
||||
- Max cycles exceeded: lines 152-155
|
||||
- **GAP: `stack.child_autostart`/`start_child_pipeline` not implemented** — spec says autostart child pipeline, impl does not
|
||||
- **Minor:** `steer_cooldown_elapsed()` not implemented — steers every cycle
|
||||
|
||||
### 36. Section 4.12 — Custom Handlers
|
||||
**ALIGNED**
|
||||
Trait-based design inherently supports custom handlers via `register()`. `Send + Sync` bounds match spec contract.
|
||||
|
||||
---
|
||||
|
||||
## Section 5: State and Context
|
||||
|
||||
### 37. Section 5.1 — Context
|
||||
**ALIGNED**
|
||||
Thread-safe key-value store at `context.rs:1-11`: `Arc<RwLock<HashMap<String, Value>>>` with all spec methods:
|
||||
- `set()`: line 33-38
|
||||
- `get()`: line 46-52
|
||||
- `get_string()`: line 56-60
|
||||
- `append_log()`: line 67-72
|
||||
- `snapshot()`: line 80-85
|
||||
- `clone_context()`: line 98-106 (deep copy for parallel isolation)
|
||||
- `apply_updates()`: line 113-118
|
||||
|
||||
### 38. Section 5.2 — Outcome
|
||||
**ALIGNED**
|
||||
At `outcome.rs`:
|
||||
- `StageStatus` enum (lines 10-16): `Success`, `Fail`, `PartialSuccess`, `Retry`, `Skipped` — all 5 spec variants
|
||||
- `Outcome` struct (lines 48-60): `status`, `preferred_label`, `suggested_next_ids`, `context_updates`, `notes`, `failure_reason` — all match
|
||||
- Factory methods (lines 63-107): `success()`, `fail()`, `retry()`, `skipped()`
|
||||
|
||||
### 39. Section 5.3 — Checkpoint
|
||||
**ALIGNED** (minor gaps)
|
||||
At `checkpoint.rs:13-20`: `timestamp`, `current_node`, `completed_nodes`, `node_retries`, `context_values`, `logs` — all match spec.
|
||||
- `save()` at line 44-49, `load()` at line 56-61
|
||||
- Resume at `engine.rs:591-608`: restores context, logs, completed_nodes, resumes from next node
|
||||
- **Minor:** Retry counters not restored during resume
|
||||
- **Minor:** Fidelity degradation on resume (`full` → `summary:high`) not implemented per spec line 1166
|
||||
|
||||
### 40. Section 5.4 — Fidelity Modes
|
||||
**ALIGNED** (minor gap)
|
||||
`resolve_fidelity` at `engine.rs:199-211` implements full 4-level precedence:
|
||||
1. Edge `fidelity` attribute (line 201)
|
||||
2. Target node `fidelity` attribute (line 205)
|
||||
3. Graph `default_fidelity` attribute (line 208)
|
||||
4. Default: `"compact"` (line 211)
|
||||
|
||||
Tests confirm all four levels at `engine.rs:1462-1511`.
|
||||
|
||||
Thread tracking: `engine.rs:660-664` stores `thread.{tid}.current_node`.
|
||||
|
||||
**Minor gap:** Thread ID resolution only implements step 1 of 5 from spec (node `thread_id`). Missing: edge `thread_id`, graph default, subgraph class derivation, fallback to previous node ID.
|
||||
|
||||
### 41. Section 5.5 — Artifact Store
|
||||
**ALIGNED**
|
||||
Full implementation at `artifact.rs`:
|
||||
- `ArtifactStore` with `base_dir` and `RwLock<HashMap>`: lines 31-34
|
||||
- `store()` with file-backing above 100KB threshold: lines 62-96
|
||||
- `retrieve()` from memory or file: lines 107-130
|
||||
- `has()`, `list()`, `remove()`, `clear()`: lines 137-184
|
||||
- `ArtifactInfo` struct with all 5 fields: lines 16-22
|
||||
|
||||
### 42. Section 5.6 — Run Directory Structure
|
||||
**ALIGNED** (minor gap)
|
||||
All spec artifacts written: `checkpoint.json`, `manifest.json`, `{node_id}/status.json`, `{node_id}/prompt.md`, `{node_id}/response.md`, `artifacts/{artifact_id}.json`.
|
||||
- **Minor:** Manifest (`engine.rs:217-232`) includes `node_count`/`edge_count` but not `goal` field from spec.
|
||||
|
||||
---
|
||||
|
||||
## Section 6: Human-in-the-Loop (Interviewer Pattern)
|
||||
|
||||
### 43. Section 6.1 — Interviewer Interface
|
||||
**ALIGNED**
|
||||
Three methods at `interviewer/mod.rs:152-167`: `ask()`, `ask_multiple()` (with default sequential impl), `inform()` (with default no-op). Async via `async_trait`.
|
||||
|
||||
### 44. Section 6.2 — Question Model
|
||||
**ALIGNED**
|
||||
At `mod.rs:29-39`: `text`, `question_type` (4 variants: `YesNo`, `MultipleChoice`, `Freeform`, `Confirmation`), `options` (key + label), `allow_freeform`, `default`, `timeout_seconds`, `stage`, `metadata` — all 8 fields present.
|
||||
|
||||
### 45. Section 6.3 — Answer Model
|
||||
**ALIGNED**
|
||||
`AnswerValue` enum (lines 57-65): `Yes`, `No`, `Skipped`, `Timeout`, `Selected(String)`, `Text(String)`. `Answer` struct (lines 68-73): `value`, `selected_option`, `text`.
|
||||
|
||||
### 46. Section 6.4 — Built-In Interviewers
|
||||
**ALIGNED** (all 5)
|
||||
- **AutoApprove** (`auto_approve.rs:10-23`): YES for YesNo/Confirmation, first option for MultipleChoice, "auto-approved" for Freeform
|
||||
- **Console** (`console.rs:49-86`): `[?]` prefix, option display, freeform fallback, Y/N for YesNo, `>` prompt for Freeform
|
||||
- **Callback** (`callback.rs:6-22`): delegates to `Box<dyn Fn(Question) -> Answer>`
|
||||
- **Queue** (`queue.rs:9-28`): `Mutex<VecDeque<Answer>>`, returns SKIPPED when empty
|
||||
- **Recording** (`recording.rs:8-39`): wraps inner interviewer, records `(Question, Answer)` pairs
|
||||
|
||||
### 47. Section 6.5 — Timeout Handling
|
||||
**ALIGNED**
|
||||
At `mod.rs:133-149`: uses `tokio::time::timeout`, returns `default_answer.unwrap_or_else(Answer::timeout)`. Tests at lines 269-296.
|
||||
|
||||
---
|
||||
|
||||
## Section 7: Validation and Linting
|
||||
|
||||
### 48. Section 7.1 — Diagnostic Model
|
||||
**ALIGNED**
|
||||
At `validation/mod.rs:9-25`: `Severity` (Error/Warning/Info), `Diagnostic` with `rule`, `severity`, `message`, `node_id`, `edge`, `fix` — all fields match.
|
||||
|
||||
### 49. Section 7.2 — Built-In Lint Rules
|
||||
**ALIGNED** (14/14 rules implemented)
|
||||
At `validation/rules.rs`:
|
||||
| Rule | Severity | Location |
|
||||
|------|----------|----------|
|
||||
| `start_node` | ERROR | lines 30-69 |
|
||||
| `terminal_node` | ERROR | lines 73-104 |
|
||||
| `reachability` | ERROR | lines 108-155 (BFS) |
|
||||
| `edge_target_exists` | ERROR | lines 159-201 |
|
||||
| `start_no_incoming` | ERROR | lines 205-233 |
|
||||
| `exit_no_outgoing` | ERROR | lines 237-272 |
|
||||
| `condition_syntax` | ERROR | lines 276-316 |
|
||||
| `stylesheet_syntax` | ERROR | lines 320-348 |
|
||||
| `type_known` | WARNING | lines 352-392 |
|
||||
| `fidelity_valid` | WARNING | lines 396-471 |
|
||||
| `retry_target_exists` | WARNING | lines 475-548 |
|
||||
| `goal_gate_has_retry` | WARNING | lines 552-586 |
|
||||
| `prompt_on_llm_nodes` | WARNING | lines 590-624 |
|
||||
| `freeform_edge_count` | ERROR | lines 628-663 |
|
||||
|
||||
**Minor:** `stylesheet_syntax` only checks brace balance, not full parse.
|
||||
|
||||
### 50. Section 7.3 — Validation API
|
||||
**ALIGNED**
|
||||
`validate(graph, extra_rules)` at line 35, `validate_or_raise(graph, extra_rules)` at line 52.
|
||||
|
||||
### 51. Section 7.4 — Custom Lint Rules
|
||||
**ALIGNED**
|
||||
`LintRule` trait at `mod.rs:28-31` with `name()` and `apply()`. Custom rules via `extra_rules` parameter.
|
||||
|
||||
---
|
||||
|
||||
## Section 8: Model Stylesheet
|
||||
|
||||
### 52. Section 8.1 — CSS-like Syntax
|
||||
**ALIGNED**
|
||||
`parse_stylesheet` at `stylesheet.rs:54-85` parses selector blocks with property declarations.
|
||||
|
||||
### 53. Section 8.2 — Selectors and Specificity
|
||||
**ALIGNED** (minor extension)
|
||||
Implementation at `stylesheet.rs:18-27` adds a `Shape` selector beyond spec:
|
||||
| Selector | Specificity |
|
||||
|----------|-------------|
|
||||
| `*` (Universal) | 0 |
|
||||
| Shape (bare word) | 1 |
|
||||
| `.class` | 2 |
|
||||
| `#id` | 3 |
|
||||
|
||||
Spec defines `*`=0, `.class`=1, `#id`=2. Relative ordering preserved; the `Shape` selector is an undocumented extension. Cascading behavior correct.
|
||||
|
||||
### 54. Section 8.3 — Application Order
|
||||
**ALIGNED**
|
||||
At `stylesheet.rs:190-238`: sorts by specificity, higher overwrites lower, explicit node attributes always override. Test at `stylesheet.rs:395-442` verifies spec section 8.6 example exactly.
|
||||
|
||||
### 55. Section 8.4 — Recognized Properties
|
||||
**ALIGNED**
|
||||
`STYLESHEET_PROPERTIES` at line 182: `["llm_model", "llm_provider", "reasoning_effort"]`. Exact match.
|
||||
|
||||
---
|
||||
|
||||
## Section 9: Transforms and Extensibility
|
||||
|
||||
### 56. Section 9.1 — Transform Trait
|
||||
**ALIGNED**
|
||||
At `transform.rs:5-7`: `fn apply(&self, graph: &mut Graph)`. In-place mutation vs spec's return-new-graph — functionally equivalent.
|
||||
|
||||
### 57. Section 9.2 — Built-In Transforms
|
||||
**ALIGNED**
|
||||
Three built-in transforms:
|
||||
- **Variable Expansion:** `transform.rs:10-25` — expands `$goal` in prompts
|
||||
- **Stylesheet Application:** `transform.rs:52-65` — applies `model_stylesheet`
|
||||
- **Preamble:** `transform.rs:28-49` — prepends `[Context mode: {fidelity}]` for non-full fidelity
|
||||
- **Minor:** Preamble applied at parse time, not execution time; cannot incorporate runtime fidelity changes from edges
|
||||
|
||||
### 58. Section 9.3 — Custom Transforms
|
||||
**ALIGNED**
|
||||
`PipelineBuilder::register_transform()` at `pipeline.rs:24-26`. Custom transforms run after built-in, in registration order. Integration test at `pipeline.rs:149-169`.
|
||||
|
||||
### 59. Section 9.4 — Event Stream
|
||||
**ALIGNED**
|
||||
All spec event types implemented at `event.rs:5-74`:
|
||||
- Pipeline lifecycle: `PipelineStarted`, `PipelineCompleted`, `PipelineFailed`
|
||||
- Stage lifecycle: `StageStarted`, `StageCompleted`, `StageFailed`, `StageRetrying`
|
||||
- Parallel: `ParallelStarted`, `ParallelBranchStarted`, `ParallelBranchCompleted`, `ParallelCompleted`
|
||||
- Human: `InterviewStarted`, `InterviewCompleted`, `InterviewTimeout`
|
||||
- Checkpoint: `CheckpointSaved`
|
||||
|
||||
Observer pattern via `EventEmitter::on_event()` at line 106. Engine emits throughout execution.
|
||||
|
||||
### 60. Section 9.5 — Tool Call Hooks
|
||||
**ALIGNED** (minor discrepancy)
|
||||
Pre/post hooks at `codergen.rs:98-131`. `resolve_hook()` at lines 56-62 checks node-level then graph-level.
|
||||
- **Minor:** Pre-hook non-zero returns `Outcome::fail()` (stronger than spec's "skip the tool call")
|
||||
|
||||
### 61. Section 9.6 — HTTP Server Mode
|
||||
**N/A** — Spec says "Implementations may expose..." (optional). Not implemented.
|
||||
|
||||
---
|
||||
|
||||
## Section 10: Condition Expression Language
|
||||
|
||||
### 62. Section 10.1 — Grammar
|
||||
**ALIGNED**
|
||||
At `condition.rs`: `&&` conjunction (line 29), `!=` (lines 33-45), `=` (lines 46-58), bare key truthy (lines 59-72).
|
||||
|
||||
### 63. Section 10.2 — Semantics
|
||||
**ALIGNED**
|
||||
- Clauses AND-combined: `condition.rs:134` uses `.all()`
|
||||
- `outcome` resolves to status string: line 88-89
|
||||
- `preferred_label` resolves: lines 91-96
|
||||
- `context.*` lookup with fallback: lines 98-105
|
||||
- Missing keys = empty string: line 105
|
||||
- Empty condition = true: lines 130-132
|
||||
|
||||
### 64. Section 10.3 — Variable Resolution
|
||||
**ALIGNED**
|
||||
`resolve_key()` at lines 87-110 follows spec pseudocode exactly: `outcome` → `preferred_label` → `context.` prefix with qualified/unqualified fallback → direct context lookup → empty string.
|
||||
|
||||
### 65. Section 10.4 — Examples
|
||||
**ALIGNED**
|
||||
Tests cover all spec examples: `outcome=success` (line 169), `context.tests_passed=true` (line 206), `preferred_label=Fix` (line 189).
|
||||
|
||||
### 66. Section 10.5 — Extended Operators
|
||||
**ALIGNED**
|
||||
Correctly NOT implemented per spec: "documented as potential extensions... Implementations should not add them."
|
||||
|
||||
---
|
||||
|
||||
## Section 11: Definition of Done
|
||||
|
||||
### 67. Section 11.1 — DOT Parsing
|
||||
**ALIGNED**
|
||||
Integration tests parse all 3 spec examples at `integration.rs:30-143`.
|
||||
|
||||
### 68. Section 11.2 — Validation and Linting
|
||||
**ALIGNED**
|
||||
14 lint rules, `validate_or_raise()` used in integration tests.
|
||||
|
||||
### 69. Section 11.3 — Execution Engine
|
||||
**ALIGNED**
|
||||
Start node resolution, handler dispatch, outcome recording, edge selection, loop execution, terminal stop — all verified in integration tests at `integration.rs:158-203`.
|
||||
|
||||
### 70. Section 11.4 — Goal Gate Enforcement
|
||||
**ALIGNED**
|
||||
Integration tests at `integration.rs:432-608`.
|
||||
|
||||
### 71. Section 11.5 — Retry Logic
|
||||
**ALIGNED**
|
||||
Integration test at `integration.rs:828-902`.
|
||||
|
||||
### 72. Section 11.6 — Node Handlers
|
||||
**ALIGNED**
|
||||
All handler types exist. Custom handler registration works.
|
||||
|
||||
### 73. Section 11.7 — State and Context
|
||||
**ALIGNED**
|
||||
Context updates, checkpoint save/resume, artifacts — verified at `integration.rs:978-1016`.
|
||||
|
||||
### 74. Section 11.8 — Human-in-the-Loop
|
||||
**ALIGNED**
|
||||
All interviewer implementations present. Integration test with QueueInterviewer at `integration.rs:376-381`.
|
||||
|
||||
### 75. Section 11.9 — Condition Expressions
|
||||
**ALIGNED**
|
||||
All operators and variable types tested at `condition.rs:160-323`.
|
||||
|
||||
### 76. Section 11.10 — Model Stylesheet
|
||||
**ALIGNED**
|
||||
Integration tests at `integration.rs:677-822` verify selectors, specificity, cascading.
|
||||
|
||||
### 77. Section 11.11 — Transforms
|
||||
**ALIGNED**
|
||||
Transform interface, variable expansion, custom transforms — all tested.
|
||||
|
||||
### 78. Section 11.12 — Cross-Feature Parity Matrix
|
||||
**ALIGNED** (minor gap)
|
||||
22 of 23 matrix items pass. **Minor:** No dedicated integration test for parallel fan-out/fan-in (handlers exist, no end-to-end test).
|
||||
|
||||
### 79. Section 11.13 — Integration Smoke Test
|
||||
**ALIGNED**
|
||||
`integration.rs:1040-1180` implements mock-backend smoke test matching spec pattern.
|
||||
|
||||
---
|
||||
|
||||
## Appendices
|
||||
|
||||
### 80. Appendix A — Complete Attribute Reference
|
||||
**ALIGNED**
|
||||
All graph, node, and edge attributes have corresponding accessors. Dotted keys (`tool_hooks.pre/post`, `stack.*`) supported via `lexer.rs:314-315`.
|
||||
|
||||
### 81. Appendix B — Shape-to-Handler-Type Mapping
|
||||
**ALIGNED**
|
||||
All 9 mappings tested at `graph/types.rs:413-429`.
|
||||
|
||||
### 82. Appendix C — Status File Contract
|
||||
**GAP**
|
||||
`auto_status=true` synthesis not implemented in engine (see item 23). `Outcome` struct at `outcome.rs:48-60` matches contract fields, but the engine never checks `auto_status`.
|
||||
|
||||
### 83. Appendix D — Error Categories
|
||||
**ALIGNED** (minor gap)
|
||||
`AttractorError` at `error.rs:4-25` has 7 variants: `Parse`, `Validation`, `Engine`, `Handler`, `Checkpoint`, `Stylesheet`, `Io`.
|
||||
**Minor:** Spec defines 3 abstract categories (Retryable, Terminal, Pipeline). No explicit classification of which variants are retryable vs terminal; default `should_retry` retries all.
|
||||
|
||||
---
|
||||
|
||||
## Summary
|
||||
|
||||
| # | Section | Verdict |
|
||||
|---|---------|---------|
|
||||
| 1 | 1.1 Problem Statement | ALIGNED |
|
||||
| 2 | 1.2 Why DOT Syntax | ALIGNED |
|
||||
| 3 | 1.3 Design Principles | ALIGNED |
|
||||
| 4 | 1.4 Layering / LLM Backends | ALIGNED |
|
||||
| 5 | 2.1 Supported Subset | ALIGNED |
|
||||
| 6 | 2.2 BNF Grammar | ALIGNED (minor: Direction not validated) |
|
||||
| 7 | 2.3 Key Constraints | ALIGNED |
|
||||
| 8 | 2.4 Value Types | ALIGNED |
|
||||
| 9 | 2.5 Graph-Level Attributes | ALIGNED |
|
||||
| 10 | 2.6 Node Attributes | ALIGNED |
|
||||
| 11 | 2.7 Edge Attributes | ALIGNED |
|
||||
| 12 | 2.8 Shape-to-Handler Mapping | ALIGNED |
|
||||
| 13 | 2.9 Chained Edges | ALIGNED |
|
||||
| 14 | 2.10-2.12 Subgraphs/Defaults/Class | ALIGNED |
|
||||
| 15 | 3.1 Run Lifecycle | ALIGNED |
|
||||
| 16 | 3.2 Core Execution Loop | ALIGNED |
|
||||
| 17 | 3.3 Edge Selection Algorithm | ALIGNED |
|
||||
| 18 | 3.4 Goal Gate Enforcement | ALIGNED |
|
||||
| 19 | 3.5 Retry Logic | ALIGNED (minor: counters not persisted) |
|
||||
| 20 | 3.6 Retry Policy / Backoff | ALIGNED (minor: should_retry too coarse) |
|
||||
| 21 | 3.7 Failure Routing | ALIGNED |
|
||||
| 22 | 3.8 Concurrency Model | ALIGNED |
|
||||
| 23 | 3.9 Auto Status | **GAP** |
|
||||
| 24 | 3.10 Timeout Enforcement | **GAP** |
|
||||
| 25 | 4.1 Handler Trait | ALIGNED |
|
||||
| 26 | 4.2 Handler Registry | ALIGNED |
|
||||
| 27 | 4.3 Start Handler | ALIGNED |
|
||||
| 28 | 4.4 Exit Handler | ALIGNED |
|
||||
| 29 | 4.5 Codergen Handler | ALIGNED |
|
||||
| 30 | 4.6 Wait Human Handler | ALIGNED |
|
||||
| 31 | 4.7 Conditional Handler | ALIGNED |
|
||||
| 32 | 4.8 Parallel Handler | ALIGNED |
|
||||
| 33 | 4.9 Fan-In Handler | ALIGNED (minor: no score sort, all-fail case) |
|
||||
| 34 | 4.10 Tool Handler | ALIGNED |
|
||||
| 35 | 4.11 Manager Loop Handler | **GAP** (child_autostart missing) |
|
||||
| 36 | 4.12 Custom Handlers | ALIGNED |
|
||||
| 37 | 5.1 Context | ALIGNED |
|
||||
| 38 | 5.2 Outcome | ALIGNED |
|
||||
| 39 | 5.3 Checkpoint | ALIGNED (minor: retry counters, fidelity degradation on resume) |
|
||||
| 40 | 5.4 Fidelity Modes | ALIGNED (minor: thread_id resolution incomplete) |
|
||||
| 41 | 5.5 Artifact Store | ALIGNED |
|
||||
| 42 | 5.6 Run Directory | ALIGNED (minor: manifest missing goal) |
|
||||
| 43 | 6.1 Interviewer Interface | ALIGNED |
|
||||
| 44 | 6.2 Question Model | ALIGNED |
|
||||
| 45 | 6.3 Answer Model | ALIGNED |
|
||||
| 46 | 6.4 Built-In Interviewers | ALIGNED |
|
||||
| 47 | 6.5 Timeout Handling | ALIGNED |
|
||||
| 48 | 7.1 Diagnostic Model | ALIGNED |
|
||||
| 49 | 7.2 Built-In Lint Rules | ALIGNED (14/14) |
|
||||
| 50 | 7.3 Validation API | ALIGNED |
|
||||
| 51 | 7.4 Custom Lint Rules | ALIGNED |
|
||||
| 52 | 8.1 CSS-like Syntax | ALIGNED |
|
||||
| 53 | 8.2 Selectors/Specificity | ALIGNED (extra Shape selector) |
|
||||
| 54 | 8.3 Application Order | ALIGNED |
|
||||
| 55 | 8.4 Recognized Properties | ALIGNED |
|
||||
| 56 | 9.1 Transform Trait | ALIGNED |
|
||||
| 57 | 9.2 Built-In Transforms | ALIGNED |
|
||||
| 58 | 9.3 Custom Transforms | ALIGNED |
|
||||
| 59 | 9.4 Event Stream | ALIGNED |
|
||||
| 60 | 9.5 Tool Call Hooks | ALIGNED (minor: pre-hook behavior) |
|
||||
| 61 | 9.6 HTTP Server Mode | N/A (optional) |
|
||||
| 62 | 10.1 Grammar | ALIGNED |
|
||||
| 63 | 10.2 Semantics | ALIGNED |
|
||||
| 64 | 10.3 Variable Resolution | ALIGNED |
|
||||
| 65 | 10.4 Examples | ALIGNED |
|
||||
| 66 | 10.5 Extended Operators | ALIGNED |
|
||||
| 67 | 11.1 DOT Parsing | ALIGNED |
|
||||
| 68 | 11.2 Validation | ALIGNED |
|
||||
| 69 | 11.3 Execution Engine | ALIGNED |
|
||||
| 70 | 11.4 Goal Gates | ALIGNED |
|
||||
| 71 | 11.5 Retry Logic | ALIGNED |
|
||||
| 72 | 11.6 Node Handlers | ALIGNED |
|
||||
| 73 | 11.7 State/Context | ALIGNED |
|
||||
| 74 | 11.8 Human-in-the-Loop | ALIGNED |
|
||||
| 75 | 11.9 Conditions | ALIGNED |
|
||||
| 76 | 11.10 Stylesheet | ALIGNED |
|
||||
| 77 | 11.11 Transforms | ALIGNED |
|
||||
| 78 | 11.12 Parity Matrix | ALIGNED (minor: no parallel integration test) |
|
||||
| 79 | 11.13 Smoke Test | ALIGNED |
|
||||
| 80 | Appendix A — Attributes | ALIGNED |
|
||||
| 81 | Appendix B — Shape Mapping | ALIGNED |
|
||||
| 82 | Appendix C — Status File | **GAP** (auto_status not enforced) |
|
||||
| 83 | Appendix D — Error Categories | ALIGNED (minor: no retryable/terminal classification) |
|
||||
|
||||
---
|
||||
|
||||
## Totals
|
||||
|
||||
**79 ALIGNED / 3 GAP / 1 N/A** (+ 16 minor gaps within ALIGNED items)
|
||||
|
||||
### Hard Gaps (3)
|
||||
|
||||
1. **Auto Status (item 23/82):** `auto_status` accessor exists at `graph/types.rs:193-194` but engine never checks it. No auto-synthesis of SUCCESS when handler writes no status.
|
||||
|
||||
2. **Timeout Enforcement (item 24):** `timeout` attribute parsed as `Duration` but not enforced as a deadline around handler execution in the engine. Tool handler uses it for subprocess timeout, but no general enforcement.
|
||||
|
||||
3. **Manager Loop `child_autostart` (item 35):** `stack.child_autostart` / `start_child_pipeline` not implemented. The observe/steer/wait cycle exists but cannot auto-launch a child pipeline.
|
||||
|
||||
### Notable Minor Gaps (within ALIGNED items)
|
||||
|
||||
- Thread ID resolution: only step 1 of 5 implemented (node `thread_id`); missing edge, graph default, subgraph class, previous-node fallback
|
||||
- Checkpoint resume: retry counters not restored; fidelity degradation (`full` → `summary:high`) not applied
|
||||
- Retry policy: `should_retry` retries ALL errors; spec defines granular HTTP-status-based behavior
|
||||
- Stylesheet: adds undocumented `Shape` selector (functional, shifts specificity values)
|
||||
- Preamble transform: applied at parse time, not execution time
|
||||
- No dedicated parallel fan-out/fan-in integration test
|
||||
|
|
@ -1,119 +0,0 @@
|
|||
# Attractor Spec Gap Analysis
|
||||
|
||||
Comparison of the implementation in `crates/attractor/` against `docs/specs/attractor-spec.md`.
|
||||
|
||||
## Summary
|
||||
|
||||
The core pipeline engine, DOT parsing, edge selection, condition evaluation, retry logic, checkpoint/resume, validation, and all 10 handler types are implemented. The HTTP server with SSE is implemented. Context fidelity preamble synthesis, thread ID plumbing to backends, engine cancellation, recording/replay, and preset retry policies are all implemented. The `should_retry` predicate is customizable via the `Handler` trait. SVG graph rendering via `GET /pipelines/{id}/graph` is implemented. **No remaining gaps.**
|
||||
|
||||
---
|
||||
|
||||
## Implemented Features (Complete or Substantially Complete)
|
||||
|
||||
| Spec Section | Feature | Status |
|
||||
|---|---|---|
|
||||
| 2. DOT DSL | Parser, grammar, value types, chained edges, subgraphs, defaults, class attr | Done |
|
||||
| 3.1 | Run lifecycle (parse, validate, initialize, execute, finalize) | Done |
|
||||
| 3.2 | Core execution loop | Done |
|
||||
| 3.3 | Edge selection (5-step: condition, preferred label, suggested IDs, weight, lexical) | Done |
|
||||
| 3.4 | Goal gate enforcement with retry target fallback chain | Done |
|
||||
| 3.5-3.6 | Retry logic with backoff, jitter, preset policies, allow_partial | Done |
|
||||
| 3.6 | Preset retry policies selectable by name from DOT (`retry_policy` attr) | Done |
|
||||
| 3.6 | `should_retry` predicate customizable via `Handler` trait method | Done |
|
||||
| 3.7 | Failure routing (fail edge, retry_target, fallback, termination) | Done |
|
||||
| 3.8 | Single-threaded traversal with parallel handler isolation | Done |
|
||||
| 4.1-4.2 | Handler interface and registry (explicit type > shape > default) | Done |
|
||||
| 4.3-4.4 | Start/Exit handlers | Done |
|
||||
| 4.5 | Codergen handler with CodergenBackend, simulation mode, $goal expansion, log files | Done |
|
||||
| 4.5 | CodergenBackend receives thread_id for session reuse | Done |
|
||||
| 4.6 | Wait.human handler with accelerator keys, freeform edges, timeout, choice matching | Done |
|
||||
| 4.7 | Conditional handler (no-op, routing via edge selection) | Done |
|
||||
| 4.8 | Parallel handler (fan-out, join policies, error policies, bounded concurrency) | Done |
|
||||
| 4.9 | Fan-in handler (heuristic + LLM-based evaluation) | Done |
|
||||
| 4.10 | Tool handler (shell command, timeout) | Done |
|
||||
| 4.11 | Manager loop handler (observe/steer/wait, child autostart, stop condition) | Done |
|
||||
| 4.12 | Custom handler registration | Done |
|
||||
| 5.1 | Context (key-value store, thread-safe, snapshot, clone, apply_updates) | Done |
|
||||
| 5.2 | Outcome (all StageStatus values, context_updates, preferred_label, suggested_next_ids) | Done |
|
||||
| 5.3 | Checkpoint (save/load, resume from checkpoint, node_outcomes, retry counters) | Done |
|
||||
| 5.3 | Checkpoint resume fidelity degradation (full -> summary:high on first resumed node) | Done |
|
||||
| 5.4 | Fidelity resolution (edge > node > graph > default) | Done |
|
||||
| 5.4 | Thread ID resolution (5-level precedence) | Done |
|
||||
| 5.4 | Context fidelity preamble synthesis (truncate, compact, summary:low/medium/high) | Done |
|
||||
| 5.5 | Artifact store | Done |
|
||||
| 5.6 | Run directory structure (manifest.json, status.json, prompt.md, response.md) | Done |
|
||||
| 6.1 | Interviewer.inform() called at pipeline/stage lifecycle points | Done |
|
||||
| 6.1-6.5 | Interviewer interface + all implementations (AutoApprove, Console, Callback, Queue, Recording, Web) | Done |
|
||||
| 6.4 | RecordingInterviewer serialization (to_json/from_json, save/load file) | Done |
|
||||
| 6.4 | ReplayInterviewer for replaying recorded Q&A sessions | Done |
|
||||
| 7.1-7.4 | Validation with 15 lint rules, diagnostic model, custom rules | Done |
|
||||
| 8.1-8.6 | Model stylesheet (parse, selectors: *, .class, #id, specificity, application) | Done |
|
||||
| 9.1-9.3 | Transforms (variable expansion, stylesheet application, custom transforms) | Done |
|
||||
| 9.4 | Graph merging transform (namespace-prefixed node/edge merge) | Done |
|
||||
| 9.5 | HTTP server mode (POST /pipelines, GET status, SSE events, question answering, cancel) | Done |
|
||||
| 9.5 | GET /pipelines/{id}/checkpoint and GET /pipelines/{id}/context endpoints | Done |
|
||||
| 9.5 | Pipeline cancel with engine-level cancellation token (checked between nodes) | Done |
|
||||
| 9.6 | Event emitter (pipeline/stage/parallel/interview/checkpoint events) | Done |
|
||||
| 9.7 | Tool call hooks (pre/post hooks on codergen handler) | Done |
|
||||
| 10.1-10.6 | Condition expression language (=, !=, &&, outcome, preferred_label, context.*) | Done |
|
||||
| N/A | Sub-pipeline handler (inline DOT, context isolation with diff propagation) | Done (bonus) |
|
||||
| 2.7 | Edge `loop_restart` attribute | Done |
|
||||
| 2.6 | Node `auto_status` attribute | Done |
|
||||
| 2.6 | Node `timeout` attribute enforcement | Done |
|
||||
|
||||
---
|
||||
|
||||
## Gaps
|
||||
|
||||
### ~~1. GET /pipelines/{id}/graph (SVG Rendering) (Spec 9.5)~~ — RESOLVED
|
||||
|
||||
The endpoint is implemented. The DOT source is stored in `ManagedPipeline` and piped through `dot -Tsvg` on request, returning `image/svg+xml`. Returns 502 if graphviz is unavailable, 404 if pipeline not found.
|
||||
|
||||
### ~~2. `should_retry` Predicate Customization (Spec 3.6)~~ — RESOLVED
|
||||
|
||||
Handlers can now override `should_retry(&self, err: &AttractorError) -> bool` on the `Handler` trait. The default impl delegates to `err.is_retryable()`. The engine's `execute_with_retry` calls the handler method directly. The `ShouldRetryFn` type and `RetryPolicy.should_retry` field have been removed. There's no DOT-level mechanism for per-node retry predicate customization, which matches the spec (no DOT syntax defined for this).
|
||||
|
||||
Note: the spec's default predicate description references HTTP status codes (429, 5xx, 401, 403, 400) but the implementation classifies retryability by `AttractorError` variant (`Handler`/`Engine`/`Io` = retryable). Reasonable for Rust but not a 1:1 mapping.
|
||||
|
||||
---
|
||||
|
||||
## Spec Contradictions (not implementation gaps)
|
||||
|
||||
These items have conflicting definitions within the spec itself. The spec needs to be reconciled; no implementation changes are needed.
|
||||
|
||||
### Stylesheet Shape Selectors (Spec 8 vs 11.12)
|
||||
|
||||
The grammar (section 8.2) defines `Selector ::= '*' | '#' Identifier | '.' ClassName` — no shape selectors. But the DoD checklist (section 11.12) says "Selectors by shape name work (e.g., `box { ... }`)" and lists a 4-level specificity order including shape. The implementation follows the grammar.
|
||||
|
||||
### Orphan Node Severity (Spec 7 vs 11.12)
|
||||
|
||||
The validation table (section 7) defines `reachability` as **ERROR**. The DoD checklist (section 11.12) says "Validate: orphan node -> **warning**". The implementation uses ERROR, matching section 7.
|
||||
|
||||
---
|
||||
|
||||
## Cross-Feature Parity Matrix Status
|
||||
|
||||
Based on code review, these items from Spec 11.12 appear covered:
|
||||
|
||||
- [x] Parse simple linear pipeline
|
||||
- [x] Parse pipeline with graph-level attributes
|
||||
- [x] Parse multi-line node attributes
|
||||
- [x] Validate: missing start/exit node -> error
|
||||
- [x] Execute linear 3-node pipeline end-to-end
|
||||
- [x] Execute with conditional branching
|
||||
- [x] Execute with retry on failure
|
||||
- [x] Goal gate blocks exit when unsatisfied
|
||||
- [x] Goal gate allows exit when all satisfied
|
||||
- [x] Wait.human presents choices and routes on selection
|
||||
- [x] Wait.human with freeform edge routes free-text input
|
||||
- [x] Edge selection: condition match wins over weight
|
||||
- [x] Edge selection: weight breaks ties
|
||||
- [x] Edge selection: lexical tiebreak
|
||||
- [x] Context updates visible to next node
|
||||
- [x] Checkpoint save and resume
|
||||
- [x] Stylesheet applies model override by class/ID
|
||||
- [x] Prompt variable expansion ($goal)
|
||||
- [x] Parallel fan-out and fan-in
|
||||
- [x] Custom handler registration and execution
|
||||
- [x] Pipeline with 10+ nodes (via integration tests)
|
||||
- [x] Validate: orphan node -> error (spec contradiction: section 7 says ERROR, DoD says warning; implementation matches section 7)
|
||||
|
|
@ -1,250 +0,0 @@
|
|||
# Spec Compliance Review: Sections 5-6
|
||||
|
||||
## Section 5: State and Context
|
||||
|
||||
### 5.1 Context
|
||||
|
||||
**ALIGNED** (with minor gaps)
|
||||
|
||||
The `Context` struct in `/Users/bhelmkamp/p/brynary/attractor-rust/crates/attractor/src/context.rs` correctly implements:
|
||||
|
||||
- Thread-safe key-value store using `Arc<RwLock<HashMap<String, Value>>>` (context.rs:9)
|
||||
- Append-only logs using `Arc<RwLock<Vec<String>>>` (context.rs:10)
|
||||
- `set(key, value)` with write lock (context.rs:33-38)
|
||||
- `get(key)` with read lock, returns `Option<Value>` (context.rs:46-52) -- spec says `default=NONE` which maps to Rust's `Option::None`
|
||||
- `get_string(key, default)` with string coercion (context.rs:56-60)
|
||||
- `append_log(entry)` with write lock (context.rs:67-72)
|
||||
- `snapshot()` returning a cloned map (context.rs:80-85)
|
||||
- `clone_context()` for deep copy / parallel isolation (context.rs:99-106) -- called `clone_context` instead of `clone` to avoid conflict with Rust's `Clone` trait
|
||||
- `apply_updates(updates)` merging a map into context (context.rs:113-118)
|
||||
|
||||
**Minor gap**: `get()` does not accept a `default` parameter like the spec's `get(key, default=NONE)`. The Rust version returns `Option<Value>` instead, which is idiomatic but means callers must handle the default themselves. This is an acceptable Rust adaptation.
|
||||
|
||||
**Built-in context keys** set by the engine:
|
||||
|
||||
| Key | Status | Evidence |
|
||||
|-----|--------|----------|
|
||||
| `outcome` | ALIGNED | engine.rs:533 sets `context.set("outcome", ...)` |
|
||||
| `preferred_label` | ALIGNED | engine.rs:535 sets `context.set("preferred_label", ...)` |
|
||||
| `graph.goal` | ALIGNED | engine.rs:349-351 mirrors graph goal |
|
||||
| `current_node` | ALIGNED | engine.rs:492 sets `context.set("current_node", ...)` |
|
||||
| `last_stage` | ALIGNED | codergen.rs:100-103 sets `last_stage` via context_updates |
|
||||
| `last_response` | ALIGNED | codergen.rs:104-107 sets `last_response` (truncated to 200 chars) |
|
||||
| `internal.retry_count.<node_id>` | **GAP** | Not implemented anywhere. The engine tracks retry attempts locally in `execute_with_retry` but never writes `internal.retry_count.<node_id>` to the context. |
|
||||
|
||||
**Context key namespace conventions**: The code uses `graph.*` namespace (engine.rs:354) and `context.*` would be user-driven. No enforcement of namespaces exists (which is expected -- they're conventions).
|
||||
|
||||
### 5.2 Outcome
|
||||
|
||||
**ALIGNED**
|
||||
|
||||
The `Outcome` struct in `/Users/bhelmkamp/p/brynary/attractor-rust/crates/attractor/src/outcome.rs` matches the spec exactly:
|
||||
|
||||
- `status: StageStatus` (outcome.rs:49) -- all five values present: `Success`, `Fail`, `PartialSuccess`, `Retry`, `Skipped` (outcome.rs:10-16)
|
||||
- `preferred_label: Option<String>` (outcome.rs:51)
|
||||
- `suggested_next_ids: Vec<String>` (outcome.rs:53)
|
||||
- `context_updates: HashMap<String, Value>` (outcome.rs:55)
|
||||
- `notes: Option<String>` (outcome.rs:57)
|
||||
- `failure_reason: Option<String>` (outcome.rs:59)
|
||||
|
||||
Factory methods: `success()`, `fail(reason)`, `retry(reason)`, `skipped()` all present (outcome.rs:63-108).
|
||||
|
||||
Serialization with `serde` roundtrips correctly (outcome.rs:173-188).
|
||||
|
||||
### 5.3 Checkpoint
|
||||
|
||||
**ALIGNED** (with minor gaps)
|
||||
|
||||
The `Checkpoint` struct in `/Users/bhelmkamp/p/brynary/attractor-rust/crates/attractor/src/checkpoint.rs` implements:
|
||||
|
||||
- `timestamp: DateTime<Utc>` (checkpoint.rs:14)
|
||||
- `current_node: String` (checkpoint.rs:15)
|
||||
- `completed_nodes: Vec<String>` (checkpoint.rs:16)
|
||||
- `node_retries: HashMap<String, u32>` (checkpoint.rs:17)
|
||||
- `context_values: HashMap<String, Value>` (checkpoint.rs:18)
|
||||
- `logs: Vec<String>` (checkpoint.rs:19)
|
||||
- `save(path)` serializes to JSON (checkpoint.rs:44-49)
|
||||
- `load(path)` deserializes from JSON (checkpoint.rs:56-61)
|
||||
|
||||
**GAP: node_retries not populated by engine**: The `Checkpoint::from_context` (checkpoint.rs:24-37) always initializes `node_retries` to an empty map. The engine (engine.rs:539-543) never populates retry counts into the checkpoint. Tests manually set retries (checkpoint.rs:103) but the engine never does.
|
||||
|
||||
**GAP: Resume behavior not implemented**: Spec 5.3 describes a 6-step resume process (load checkpoint, restore context, restore completed_nodes, restore retry counters, determine next node, degrade fidelity). The engine has no `resume_from_checkpoint` method. The `Checkpoint::load` exists but nothing consumes it for resumption.
|
||||
|
||||
### 5.4 Context Fidelity
|
||||
|
||||
**GAP** (data model present, runtime not implemented)
|
||||
|
||||
The spec defines `FidelityMode` with values: `full`, `truncate`, `compact`, `summary:low`, `summary:medium`, `summary:high`.
|
||||
|
||||
- Graph types support `fidelity` attribute on nodes (graph/types.rs:159-160) and edges (graph/types.rs:257-258)
|
||||
- Graph supports `default_fidelity` (graph/types.rs:368-372)
|
||||
- Nodes and edges support `thread_id` attribute (graph/types.rs:164-165, 262-263)
|
||||
- Validation rule `fidelity_valid` validates fidelity modes (validation/rules.rs:381-446)
|
||||
|
||||
**However**:
|
||||
- No `FidelityMode` enum exists as a first-class type -- fidelity is only a string attribute
|
||||
- The fidelity resolution precedence (edge -> node -> graph default -> `compact`) is not implemented in the engine
|
||||
- Thread resolution for `full` fidelity is not implemented
|
||||
- The engine does not use fidelity to control context passing between nodes
|
||||
- No session reuse / thread management exists
|
||||
|
||||
### 5.5 Artifact Store
|
||||
|
||||
**ALIGNED**
|
||||
|
||||
The `ArtifactStore` in `/Users/bhelmkamp/p/brynary/attractor-rust/crates/attractor/src/artifact.rs` fully implements the spec:
|
||||
|
||||
- `store(id, name, data) -> ArtifactInfo` (artifact.rs:62-96) with file-backing for large artifacts
|
||||
- `retrieve(id) -> Value` (artifact.rs:107-130) reading from memory or disk
|
||||
- `has(id) -> bool` (artifact.rs:137-142)
|
||||
- `list() -> Vec<ArtifactInfo>` (artifact.rs:150-157)
|
||||
- `remove(id)` (artifact.rs:164-169) including disk cleanup
|
||||
- `clear()` (artifact.rs:176-184) including disk cleanup
|
||||
- `FILE_BACKING_THRESHOLD = 100 * 1024` (artifact.rs:12) matching spec's 100KB
|
||||
|
||||
`ArtifactInfo` fields match spec (artifact.rs:16-22):
|
||||
- `id`, `name`, `size_bytes`, `stored_at`, `is_file_backed`
|
||||
|
||||
Thread safety via `RwLock` (artifact.rs:33).
|
||||
|
||||
### 5.6 Run Directory Structure
|
||||
|
||||
**PARTIALLY ALIGNED**
|
||||
|
||||
Spec directory structure:
|
||||
```
|
||||
{logs_root}/
|
||||
checkpoint.json -- present (engine.rs:544)
|
||||
manifest.json -- MISSING
|
||||
{node_id}/
|
||||
status.json -- present (codergen.rs:82, 111)
|
||||
prompt.md -- present (codergen.rs:74)
|
||||
response.md -- present (codergen.rs:95)
|
||||
artifacts/
|
||||
{artifact_id}.json -- present (artifact.rs:75)
|
||||
```
|
||||
|
||||
**GAP: manifest.json**: The spec requires a `manifest.json` with pipeline metadata (name, goal, start time). This file is never written by the engine.
|
||||
|
||||
**GAP: Per-node directories only for codergen**: Only the `CodergenHandler` creates `{node_id}/` subdirectories with `status.json`, `prompt.md`, and `response.md`. Other handlers (start, exit, tool, parallel, etc.) do not write any per-node log files.
|
||||
|
||||
---
|
||||
|
||||
## Section 6: Human-in-the-Loop (Interviewer Pattern)
|
||||
|
||||
### 6.1 Interviewer Interface
|
||||
|
||||
**ALIGNED**
|
||||
|
||||
The `Interviewer` trait in `/Users/bhelmkamp/p/brynary/attractor-rust/crates/attractor/src/interviewer/mod.rs` matches the spec:
|
||||
|
||||
- `ask(question: Question) -> Answer` (mod.rs:133)
|
||||
- `ask_multiple(questions: Vec<Question>) -> Vec<Answer>` with default sequential implementation (mod.rs:135-140)
|
||||
- `inform(message, stage)` with default no-op (mod.rs:143-145)
|
||||
|
||||
The trait is `async` (using `#[async_trait]`) and requires `Send + Sync` (mod.rs:132), which is appropriate for Rust.
|
||||
|
||||
### 6.2 Question Model
|
||||
|
||||
**ALIGNED**
|
||||
|
||||
`Question` struct (mod.rs:29-38):
|
||||
- `text: String`
|
||||
- `question_type: QuestionType` (named `question_type` instead of `type` since `type` is a Rust keyword)
|
||||
- `options: Vec<QuestionOption>`
|
||||
- `allow_freeform: bool`
|
||||
- `default: Option<Answer>`
|
||||
- `timeout_seconds: Option<f64>`
|
||||
- `stage: String`
|
||||
- `metadata: HashMap<String, Value>`
|
||||
|
||||
`QuestionType` enum (mod.rs:13-18):
|
||||
- `YesNo`, `MultipleChoice`, `Freeform`, `Confirmation` -- all four spec variants present
|
||||
|
||||
`QuestionOption` (mod.rs:21-24):
|
||||
- `key: String`, `label: String` -- matches spec's `Option` (renamed to avoid Rust keyword collision)
|
||||
|
||||
### 6.3 Answer Model
|
||||
|
||||
**ALIGNED**
|
||||
|
||||
`Answer` struct (mod.rs:68-72):
|
||||
- `value: AnswerValue`
|
||||
- `selected_option: Option<QuestionOption>`
|
||||
- `text: Option<String>`
|
||||
|
||||
`AnswerValue` enum (mod.rs:57-64):
|
||||
- `Yes`, `No`, `Skipped`, `Timeout` -- matches spec
|
||||
- `Selected(String)` -- represents a multiple-choice selection (spec used `value: String`)
|
||||
- `Text(String)` -- represents freeform text
|
||||
|
||||
The spec uses a single `value` field that can be either an `AnswerValue` enum or a string. The Rust implementation cleanly separates these via enum variants, which is a good adaptation.
|
||||
|
||||
### 6.4 Built-In Interviewer Implementations
|
||||
|
||||
**AutoApproveInterviewer: ALIGNED**
|
||||
|
||||
`/Users/bhelmkamp/p/brynary/attractor-rust/crates/attractor/src/interviewer/auto_approve.rs`:
|
||||
- YesNo/Confirmation -> `Answer::yes()` (auto_approve.rs:12)
|
||||
- MultipleChoice -> first option or "auto-approved" text (auto_approve.rs:13-20)
|
||||
- Freeform -> `Answer::text("auto-approved")` (auto_approve.rs:21)
|
||||
- Matches spec pseudocode exactly
|
||||
|
||||
**ConsoleInterviewer: GAP (not implemented)**
|
||||
|
||||
No `ConsoleInterviewer` exists in the codebase. Grep for `ConsoleInterviewer` returns no matches. The spec describes a CLI-based interviewer that reads from stdin.
|
||||
|
||||
**CallbackInterviewer: ALIGNED**
|
||||
|
||||
`/Users/bhelmkamp/p/brynary/attractor-rust/crates/attractor/src/interviewer/callback.rs`:
|
||||
- Accepts a `Fn(Question) -> Answer` callback (callback.rs:7)
|
||||
- `ask()` delegates to callback (callback.rs:21)
|
||||
- Matches spec exactly
|
||||
|
||||
**QueueInterviewer: ALIGNED**
|
||||
|
||||
`/Users/bhelmkamp/p/brynary/attractor-rust/crates/attractor/src/interviewer/queue.rs`:
|
||||
- Pre-filled `VecDeque<Answer>` (queue.rs:10)
|
||||
- `ask()` dequeues or returns `Answer::skipped()` (queue.rs:24-26)
|
||||
- Thread-safe via `Mutex` (queue.rs:10)
|
||||
- Matches spec exactly
|
||||
|
||||
**RecordingInterviewer: ALIGNED**
|
||||
|
||||
`/Users/bhelmkamp/p/brynary/attractor-rust/crates/attractor/src/interviewer/recording.rs`:
|
||||
- Wraps an inner `Box<dyn Interviewer>` (recording.rs:9)
|
||||
- Records `(Question, Answer)` pairs in `Mutex<Vec<...>>` (recording.rs:10)
|
||||
- `ask()` delegates to inner, then records (recording.rs:32-38)
|
||||
- `recordings()` accessor (recording.rs:25-27)
|
||||
- Matches spec exactly
|
||||
|
||||
### 6.5 Timeout Handling
|
||||
|
||||
**GAP** (partially modeled, not implemented at runtime)
|
||||
|
||||
- The `Question` struct has a `timeout_seconds: Option<f64>` field (mod.rs:35)
|
||||
- The `Answer` has a `timeout()` factory (mod.rs:103-108) and `AnswerValue::Timeout` variant (mod.rs:61)
|
||||
- The `Question` struct has a `default: Option<Answer>` field (mod.rs:34)
|
||||
|
||||
**However**:
|
||||
- No interviewer implementation actually enforces timeouts (no tokio timeout wrapper)
|
||||
- The spec's timeout behavior (use default if available, else return Timeout) is not implemented in any interviewer
|
||||
- `wait.human` node's `human.default_choice` for timeout behavior is not checked at runtime
|
||||
|
||||
---
|
||||
|
||||
## Summary of Gaps
|
||||
|
||||
| Section | Status | Gap Description |
|
||||
|---------|--------|-----------------|
|
||||
| 5.1 | ALIGNED (minor) | `internal.retry_count.<node_id>` context key never written by engine |
|
||||
| 5.2 | ALIGNED | Fully matches spec |
|
||||
| 5.3 | GAP | Checkpoint save works but resume from checkpoint not implemented; node_retries never populated by engine |
|
||||
| 5.4 | GAP | Fidelity attributes parsed and validated, but fidelity resolution/application not in engine; no session/thread management |
|
||||
| 5.5 | ALIGNED | Fully matches spec |
|
||||
| 5.6 | GAP | Missing `manifest.json`; per-node directories only created by CodergenHandler, not other handlers |
|
||||
| 6.1 | ALIGNED | Trait matches spec interface |
|
||||
| 6.2 | ALIGNED | Question model complete |
|
||||
| 6.3 | ALIGNED | Answer model complete |
|
||||
| 6.4 | GAP | Missing `ConsoleInterviewer`; other four implementations aligned |
|
||||
| 6.5 | GAP | Timeout data model present but no runtime enforcement in any interviewer |
|
||||
|
|
@ -1,197 +0,0 @@
|
|||
# Spec Compliance Review: Sections 7-9 (Subagents + Definition of Done)
|
||||
|
||||
## Section 7: Subagents
|
||||
|
||||
### 7.1 Concept -- ALIGNED
|
||||
- `SubAgent` in `subagent.rs` spawns a child session via `SubAgentManager::spawn()` which takes a `Session` and runs `session.process_input()` in a tokio task.
|
||||
- The child session has its own conversation history (its own `History` instance).
|
||||
- The child session shares the parent's execution environment (passed through the `SessionFactory` / session construction).
|
||||
|
||||
### 7.2 Spawn Interface -- ALIGNED
|
||||
All four tools are implemented in `subagent.rs`:
|
||||
- `spawn_agent`: Correct params (`task` required, `working_dir`/`model`/`max_turns` optional). Returns agent ID.
|
||||
- `send_input`: Correct params (`agent_id`, `message` required). Returns acknowledgement.
|
||||
- `wait`: Correct params (`agent_id` required). Returns `SubAgentResult` (output, success, turns_used).
|
||||
- `close_agent`: Correct params (`agent_id` required). Returns final status.
|
||||
|
||||
**GAP**: `spawn_agent` tool executor does not use the `working_dir`, `model`, or `max_turns` optional parameters. They are defined in the schema but ignored in the executor at line 185-197. The session factory creates a default session regardless of these overrides.
|
||||
|
||||
### 7.3 SubAgent Lifecycle -- ALIGNED (with one minor gap)
|
||||
- `SubAgentResult` record: matches spec (`output: String`, `success: bool`, `turns_used: usize`).
|
||||
- `SubAgent` struct has `id` and `depth` fields but no explicit `status` enum (`"running" | "completed" | "failed"`). Status is implicit via whether the tokio task is running.
|
||||
- `SubAgentHandle` is not a separate record; `SubAgent` serves this role.
|
||||
- Depth limiting: Implemented in `SubAgentManager::spawn()` at line 56 (`depth >= self.max_depth`). Default `max_subagent_depth: 1` in `SessionConfig`.
|
||||
- Independent history: Each subagent gets its own `Session` with its own `History`.
|
||||
|
||||
**GAP**: No explicit `SubAgentHandle` record with a `status` field as spec defines. Status is implicit.
|
||||
|
||||
### 7.4 Use Cases -- ALIGNED
|
||||
The architecture supports all listed use cases (parallel exploration, focused refactoring, test execution, alternative approaches) through the spawn/wait/close interface. Subagents run as independent tokio tasks sharing the execution environment.
|
||||
|
||||
---
|
||||
|
||||
## Section 9: Definition of Done
|
||||
|
||||
### 9.1 Core Loop
|
||||
|
||||
| Item | Status | Evidence |
|
||||
|------|--------|----------|
|
||||
| Session created with ProviderProfile + ExecutionEnvironment | DONE | `Session::new(client, profile, env, config)` in `session.rs:37` |
|
||||
| `process_input()` runs agentic loop | DONE | `session.rs:118-157` -- LLM call -> tool exec -> loop |
|
||||
| Natural completion (text only, no tool calls) | DONE | `session.rs:259-261` -- breaks when `tool_calls.is_empty()` |
|
||||
| Round limits (`max_tool_rounds_per_input`) | DONE | `session.rs:184-191` -- checked each iteration |
|
||||
| Session turn limits (`max_turns`) | DONE | `session.rs:194-201` -- checked each iteration |
|
||||
| Abort signal -> CLOSED | DONE | `session.rs:204-207` -- checks `abort_flag`, transitions to Closed |
|
||||
| Loop detection -> warning SteeringTurn | DONE | `session.rs:278-290` -- calls `detect_loop`, injects Steering turn |
|
||||
| Multiple sequential inputs | DONE | Test `sequential_inputs` in `session.rs:1437-1457` confirms this works |
|
||||
|
||||
**Result: 8/8 DONE**
|
||||
|
||||
### 9.2 Provider Profiles
|
||||
|
||||
| Item | Status | Evidence |
|
||||
|------|--------|----------|
|
||||
| OpenAI profile with `apply_patch` (v4a) | DONE | `profiles/openai.rs` -- registers `apply_patch`, full v4a parser + applier |
|
||||
| Anthropic profile with `edit_file` (old_string/new_string) | DONE | `profiles/anthropic.rs` -- registers `edit_file` tool |
|
||||
| Gemini profile with gemini-cli-aligned tools | DONE | `profiles/gemini.rs` -- registers read/write/edit/shell/grep/glob |
|
||||
| Each profile has provider-specific system prompt | DONE | Each profile's `build_system_prompt()` includes identity + env context + tool guidance |
|
||||
| Custom tools can be registered | DONE | `tool_registry_mut()` exposed on `ProviderProfile` trait, `ToolRegistry::register()` available |
|
||||
| Tool name collisions resolved (override) | DONE | `ToolRegistry::register()` uses `HashMap::insert` which overwrites. Test `name_collision_overrides` in `tool_registry.rs:111` |
|
||||
|
||||
**Result: 6/6 DONE**
|
||||
|
||||
### 9.3 Tool Execution
|
||||
|
||||
| Item | Status | Evidence |
|
||||
|------|--------|----------|
|
||||
| Tool calls dispatched through ToolRegistry | DONE | `session.rs:352-353` -- `registry.get(tool_name)` |
|
||||
| Unknown tool -> error result to LLM | DONE | `session.rs:386-393` -- returns `is_error: true` with "Unknown tool" |
|
||||
| Tool argument JSON validated against schema | DONE | `session.rs:356-366` -- `validate_tool_args()` using `jsonschema` crate |
|
||||
| Tool execution errors caught and returned as error results | DONE | `session.rs:377-384` -- `Err(err)` mapped to `is_error: true` |
|
||||
| Parallel tool execution when `supports_parallel_tool_calls` | DONE | `session.rs:400-404` -- routes to `execute_tool_calls_parallel` when supported |
|
||||
|
||||
**Result: 5/5 DONE**
|
||||
|
||||
### 9.4 Execution Environment
|
||||
|
||||
| Item | Status | Evidence |
|
||||
|------|--------|----------|
|
||||
| `LocalExecutionEnvironment` implements all file/command ops | DONE | `local_env.rs` -- read/write/exists/list/exec/grep/glob all implemented |
|
||||
| Command timeout default is 10 seconds | DONE | `config.rs:23` -- `default_command_timeout_ms: 10_000` |
|
||||
| Command timeout overridable per-call via `timeout_ms` param | DONE | `tools.rs:180-184` -- shell tool reads `timeout_ms` from args |
|
||||
| Timed-out: SIGTERM then SIGKILL after 2 seconds | DONE | `local_env.rs:152-173` -- sends SIGTERM, waits 2s, then SIGKILL |
|
||||
| Env var filtering excludes sensitive variables | DONE | `local_env.rs:31-38` -- filters `*_API_KEY`, `*_SECRET`, `*_TOKEN`, `*_PASSWORD`, `*_CREDENTIAL` |
|
||||
| `ExecutionEnvironment` interface implementable by consumers | DONE | `execution_env.rs:27` -- `trait ExecutionEnvironment: Send + Sync` with all methods |
|
||||
|
||||
**Result: 6/6 DONE**
|
||||
|
||||
### 9.5 Tool Output Truncation
|
||||
|
||||
| Item | Status | Evidence |
|
||||
|------|--------|----------|
|
||||
| Character-based truncation runs FIRST | DONE | `truncation.rs:99-109` -- char truncation applied first |
|
||||
| Line-based truncation runs SECOND (shell:256, grep:200, glob:500) | DONE | `truncation.rs:111-121` -- line truncation after chars; `default_line_limits()` has correct values |
|
||||
| Truncation inserts visible marker | DONE | `truncation.rs:54-55, 62-63` -- `[WARNING: Output truncated...]` markers |
|
||||
| Full untruncated output in `TOOL_CALL_END` event | DONE | `session.rs:554-571` -- emits event with full output BEFORE truncation |
|
||||
| Default char limits match spec Section 5.2 | DONE | `truncation.rs:10-21` -- read_file:50k, shell:30k, grep:20k, glob:20k, edit_file:10k, write_file:1k |
|
||||
| Both char and line limits overridable via `SessionConfig` | DONE | `config.rs:10-11` -- `tool_output_limits` and `tool_line_limits` HashMaps; `truncation.rs:100-103, 112-116` checks config first |
|
||||
|
||||
**Result: 6/6 DONE**
|
||||
|
||||
### 9.6 Steering
|
||||
|
||||
| Item | Status | Evidence |
|
||||
|------|--------|----------|
|
||||
| `steer()` queues message injected after current tool round | DONE | `session.rs:80-85` -- pushes to `steering_queue`; `session.rs:275` -- `drain_steering()` called after tool execution |
|
||||
| `follow_up()` queues message processed after current input completes | DONE | `session.rs:87-92` -- pushes to `followup_queue`; `session.rs:136-146` -- processed after `run_single_input` |
|
||||
| Steering messages appear as SteeringTurn in history | DONE | `session.rs:304` -- `Turn::Steering` pushed to history |
|
||||
| SteeringTurns converted to user-role messages for LLM | DONE | `history.rs:75-81` -- `Turn::Steering` maps to `Role::User` message |
|
||||
|
||||
**Result: 4/4 DONE**
|
||||
|
||||
### 9.7 Reasoning Effort
|
||||
|
||||
| Item | Status | Evidence |
|
||||
|------|--------|----------|
|
||||
| `reasoning_effort` passed through to LLM SDK Request | DONE | `session.rs:340` -- `reasoning_effort: self.config.reasoning_effort.clone()` |
|
||||
| Changing mid-session takes effect on next LLM call | DONE | `session.rs:110-112` -- `set_reasoning_effort()` mutates config; test at line 1722 confirms |
|
||||
| Valid values: "low", "medium", "high", null | DONE | Stored as `Option<String>` and passed through to SDK; no validation in this layer (SDK handles it) |
|
||||
|
||||
**Result: 3/3 DONE**
|
||||
|
||||
### 9.8 System Prompts
|
||||
|
||||
| Item | Status | Evidence |
|
||||
|------|--------|----------|
|
||||
| Provider-specific base instructions | DONE | Each profile has distinct identity text ("You are Claude...", "You are a coding assistant", "powered by Gemini") |
|
||||
| Environment context (platform, git, working dir, date, model) | DONE | `profiles/mod.rs:21-48` -- `build_env_context_block` includes platform, working_directory, OS version, git branch, date, model |
|
||||
| Tool descriptions from active profile | DONE | `session.rs:323` -- `self.provider_profile.tools()` included in request |
|
||||
| Project docs (AGENTS.md + provider-specific) discovered and included | DONE | `project_docs.rs` -- discovers AGENTS.md plus provider-specific files |
|
||||
| User instruction overrides appended last | NOT DONE | No mechanism for user instruction overrides in the system prompt pipeline. The `build_system_prompt` method appends project docs but has no separate "user overrides" parameter. |
|
||||
| Only relevant project files loaded per provider | DONE | `project_docs.rs:13-18` -- filters by provider_id: anthropic gets CLAUDE.md, openai gets .codex/instructions.md, gemini gets GEMINI.md |
|
||||
|
||||
**Result: 5/6 DONE**
|
||||
|
||||
**GAP**: No explicit user instruction override mechanism in the system prompt. The spec says "User instruction overrides are appended last (highest priority)". The current `build_system_prompt` takes `project_docs` but has no separate parameter or config field for user-supplied instruction overrides.
|
||||
|
||||
### 9.9 Subagents
|
||||
|
||||
| Item | Status | Evidence |
|
||||
|------|--------|----------|
|
||||
| Subagents spawned with scoped task via `spawn_agent` tool | DONE | `subagent.rs:153-200` |
|
||||
| Subagents share parent's execution environment | DONE | Session factory creates session with shared env |
|
||||
| Subagents maintain independent conversation history | DONE | Each Session has its own History |
|
||||
| Depth limiting prevents recursive spawning (default max: 1) | DONE | `subagent.rs:56` and `config.rs:30` -- `max_subagent_depth: 1` |
|
||||
| Subagent results returned to parent as tool results | DONE | `wait` tool returns formatted result string |
|
||||
| `send_input`, `wait`, `close_agent` tools work correctly | DONE | All three tools implemented with correct params and tested |
|
||||
|
||||
**GAP (minor)**: The `spawn_agent` tool ignores `working_dir`, `model`, and `max_turns` optional parameters. They are in the schema but not wired to session creation.
|
||||
|
||||
**Result: 6/6 DONE** (core behavior works; optional param wiring is a gap but not a DoD blocker)
|
||||
|
||||
### 9.10 Event System
|
||||
|
||||
| Item | Status | Evidence |
|
||||
|------|--------|----------|
|
||||
| All event kinds from Section 2.9 emitted at correct times | PARTIAL | Most events are emitted. `ASSISTANT_TEXT_START` is defined in `EventKind` enum but never emitted in `session.rs`. `CONTEXT_WINDOW_WARNING` is emitted but not in the spec's enum (it's an extension). |
|
||||
| Events delivered via async iterator / equivalent | DONE | `event.rs` -- `tokio::sync::broadcast` channel with `subscribe()` returning `Receiver<SessionEvent>` |
|
||||
| `TOOL_CALL_END` events carry full untruncated output | DONE | `session.rs:554-571` -- emits before truncation |
|
||||
| Session lifecycle events (SESSION_START, SESSION_END) bracket session | DONE | `session.rs:123-127` emits SessionStart; `session.rs:150-154` emits SessionEnd |
|
||||
|
||||
**Result: 3/4 DONE, 1 PARTIAL**
|
||||
|
||||
**GAP**: `AssistantTextStart` event kind is defined in the enum but never emitted anywhere in the session code. The spec lists `ASSISTANT_TEXT_START` as a required event.
|
||||
|
||||
### 9.11 Error Handling
|
||||
|
||||
| Item | Status | Evidence |
|
||||
|------|--------|----------|
|
||||
| Tool execution errors -> error result sent to LLM | DONE | `session.rs:377-384` -- tool errors returned as `is_error: true` ToolResult |
|
||||
| LLM API transient errors -> retry with backoff (via SDK) | DONE | Spec explicitly says "handled by Unified LLM SDK layer" |
|
||||
| Authentication errors -> surface immediately, session CLOSED | DONE | `session.rs:226-229` -- `is_auth_error()` check, transitions to Closed |
|
||||
| Context window overflow -> emit warning event | DONE | `session.rs:629-654` -- `check_context_usage()` emits `ContextWindowWarning` |
|
||||
| Graceful shutdown: abort -> cancel -> kill -> flush -> SESSION_END | PARTIAL | Abort flag checked, returns `AgentError::Aborted`, transitions to Closed. But `SESSION_END` is NOT emitted on abort (the abort short-circuits before the `emit(SessionEnd)` call). Also no explicit process killing on abort -- the session just stops looping. |
|
||||
|
||||
**Result: 4/5 DONE, 1 PARTIAL**
|
||||
|
||||
**GAP**: On abort, `SESSION_END` event is not emitted. The abort at `session.rs:204-207` returns an `Err(AgentError::Aborted)` which skips the `SessionEnd` emit at line 150-154. Running processes are not explicitly killed on abort either (only the loop stops).
|
||||
|
||||
---
|
||||
|
||||
## Summary of All Gaps
|
||||
|
||||
### Functional Gaps (should fix):
|
||||
|
||||
1. **Spawn agent ignores optional params** (`subagent.rs:185-197`): `working_dir`, `model`, `max_turns` params are in the tool schema but the executor does not use them when creating the session. The session factory ignores these overrides.
|
||||
|
||||
2. **`AssistantTextStart` event never emitted** (`session.rs`): The event kind exists in the enum but is never emitted. Should be emitted before/when the LLM starts generating text.
|
||||
|
||||
3. **No `SESSION_END` event on abort** (`session.rs:204-207`): When abort triggers, the method returns early with `Err(AgentError::Aborted)` without emitting `SESSION_END`. The spec says graceful shutdown should "flush events -> emit SESSION_END".
|
||||
|
||||
4. **No user instruction overrides in system prompt** (`session.rs` / `ProviderProfile`): Spec 9.8 item 5 says "User instruction overrides are appended last (highest priority)". There is no mechanism to pass user instruction overrides into the system prompt pipeline. `SessionConfig` lacks an `instructions` or `user_overrides` field.
|
||||
|
||||
### Minor / Non-blocking Gaps:
|
||||
|
||||
5. **No explicit `SubAgentHandle` with `status` field**: Status is implicit based on tokio task state rather than an explicit enum field. Functionally equivalent but structurally different from spec.
|
||||
|
||||
6. **`SESSION_END` not emitted on abort path for running processes**: No explicit kill of running child processes on abort. The session just stops the loop, but any child process from `exec_command` may continue running. The `close()` method for subagents does handle this properly.
|
||||
|
|
@ -1,311 +0,0 @@
|
|||
# CLI Design
|
||||
|
||||
## Binary
|
||||
|
||||
The `attractor` crate (`crates/attractor/`) gains a `[[bin]]` target named `attractor` alongside its existing library. No separate CLI crate.
|
||||
|
||||
New files: `src/main.rs`, `src/cli/mod.rs`, `src/cli/run.rs`, `src/cli/validate.rs`.
|
||||
|
||||
New dependencies added to the `attractor` crate: `clap`, `anyhow`, `dotenvy`, `chrono` (all already in workspace).
|
||||
|
||||
## Command structure
|
||||
|
||||
```
|
||||
attractor run [OPTIONS] <pipeline.dot>
|
||||
attractor validate [OPTIONS] <pipeline.dot>
|
||||
attractor --version
|
||||
attractor --help
|
||||
```
|
||||
|
||||
## `attractor run`
|
||||
|
||||
```
|
||||
Usage: attractor run [OPTIONS] <PIPELINE>
|
||||
|
||||
Arguments:
|
||||
<PIPELINE> Path to a .dot pipeline file
|
||||
|
||||
Options:
|
||||
--logs-dir <DIR> Log/artifact directory [default: ./attractor-run-<YYYYMMDD-HHMMSS>]
|
||||
--dry-run Execute with a simulated LLM backend (no API calls)
|
||||
--auto-approve Auto-approve all human-in-the-loop gates
|
||||
--resume <CHECKPOINT> Resume from a checkpoint JSON file
|
||||
--model <MODEL> Override default LLM model for all nodes
|
||||
--provider <PROVIDER> Override default LLM provider (anthropic, openai, gemini)
|
||||
-v, --verbose... Verbosity level (-v summary, -vv full details)
|
||||
-h, --help Show help
|
||||
```
|
||||
|
||||
## `attractor validate`
|
||||
|
||||
```
|
||||
Usage: attractor validate [OPTIONS] <PIPELINE>
|
||||
|
||||
Arguments:
|
||||
<PIPELINE> Path to a .dot pipeline file
|
||||
|
||||
Options:
|
||||
-h, --help Show help
|
||||
```
|
||||
|
||||
Parse, transform, and validate only. Prints errors and warnings, exits 0 if no errors.
|
||||
|
||||
### Environment variables
|
||||
|
||||
| Variable | Purpose |
|
||||
|----------|---------|
|
||||
| `ANTHROPIC_API_KEY` | Anthropic API key |
|
||||
| `OPENAI_API_KEY` | OpenAI API key |
|
||||
| `GEMINI_API_KEY` | Google Gemini API key |
|
||||
|
||||
Loaded via `dotenvy::dotenv().ok()` at startup (`.env` file support, non-fatal if missing).
|
||||
|
||||
## Execution flow
|
||||
|
||||
### `attractor validate`
|
||||
|
||||
```
|
||||
validate_command(args):
|
||||
1. Read .dot file
|
||||
2. PipelineBuilder::new().prepare(&source) -> (Graph, Vec<Diagnostic>)
|
||||
3. Print "Parsed pipeline: {name} ({n} nodes, {m} edges)"
|
||||
4. Print errors (Severity::Error) to stderr
|
||||
5. Print warnings (Severity::Warning) to stderr
|
||||
6. If errors -> exit 1
|
||||
7. Print "Validation: OK", exit 0
|
||||
```
|
||||
|
||||
### `attractor run`
|
||||
|
||||
```
|
||||
main()
|
||||
1. dotenvy::dotenv()
|
||||
2. parse CLI args (clap)
|
||||
3. dispatch to run_command(args) or validate_command(args)
|
||||
|
||||
run_command(args):
|
||||
1. Read .dot file from args.pipeline
|
||||
2. Prepare pipeline
|
||||
PipelineBuilder::new().prepare(&source)
|
||||
-> (Graph, Vec<Diagnostic>)
|
||||
3. Print parsed summary to stdout
|
||||
"Parsed pipeline: {name} ({n} nodes, {m} edges)"
|
||||
"Goal: {goal}"
|
||||
4. Check for validation errors (Severity::Error)
|
||||
If any -> print to stderr, exit 1
|
||||
5. Print warnings (Severity::Warning) to stderr
|
||||
|
||||
6. Create logs directory
|
||||
args.logs_dir or generate ./attractor-run-<YYYYMMDD-HHMMSS>
|
||||
fs::create_dir_all()
|
||||
7. Build LLM client
|
||||
If --dry-run -> skip (no backend set on engine, handlers use dry-run stubs)
|
||||
Else -> unified_llm::Client::from_env()
|
||||
If no providers configured -> warn to stderr, continue as dry-run
|
||||
8. Resolve model/provider
|
||||
CLI --model/--provider override > graph-level defaults > auto-detect from available providers
|
||||
9. Build CodergenBackend
|
||||
Wire up LLM client + coding-agent-loop with ExecutionEnv rooted at cwd
|
||||
10. Build PipelineEngine
|
||||
Create HandlerRegistry, register built-in handlers
|
||||
Set backend on codergen handler
|
||||
Create EventEmitter
|
||||
If -v or -vv -> attach stderr logging callback (level determines format)
|
||||
Set interviewer:
|
||||
--auto-approve -> AutoApproveInterviewer
|
||||
else -> ConsoleInterviewer
|
||||
11. Execute or resume
|
||||
If --resume -> load Checkpoint from file, engine.run_from_checkpoint()
|
||||
Else -> engine.run()
|
||||
12. Print result
|
||||
"=== Pipeline Result ==="
|
||||
"Status: {SUCCESS|FAIL|PARTIAL_SUCCESS|...}"
|
||||
"Notes: ..." (if present)
|
||||
"Failure: ..." (if present)
|
||||
"Logs: {logs_dir}"
|
||||
13. Exit code
|
||||
0 if SUCCESS or PARTIAL_SUCCESS
|
||||
1 otherwise
|
||||
```
|
||||
|
||||
## Clap types
|
||||
|
||||
```rust
|
||||
#[derive(Parser)]
|
||||
#[command(name = "attractor", version, about = "DOT-based pipeline runner for AI workflows")]
|
||||
struct Cli {
|
||||
#[command(subcommand)]
|
||||
command: Command,
|
||||
}
|
||||
|
||||
#[derive(Subcommand)]
|
||||
enum Command {
|
||||
/// Launch a pipeline from a .dot file
|
||||
Run(RunArgs),
|
||||
/// Parse and validate a pipeline without executing
|
||||
Validate(ValidateArgs),
|
||||
}
|
||||
|
||||
#[derive(Args)]
|
||||
struct RunArgs {
|
||||
/// Path to the .dot pipeline file
|
||||
pipeline: PathBuf,
|
||||
|
||||
/// Log/artifact directory
|
||||
#[arg(long)]
|
||||
logs_dir: Option<PathBuf>,
|
||||
|
||||
/// Execute with simulated LLM backend
|
||||
#[arg(long)]
|
||||
dry_run: bool,
|
||||
|
||||
/// Auto-approve all human gates
|
||||
#[arg(long)]
|
||||
auto_approve: bool,
|
||||
|
||||
/// Resume from a checkpoint file
|
||||
#[arg(long)]
|
||||
resume: Option<PathBuf>,
|
||||
|
||||
/// Override default LLM model
|
||||
#[arg(long)]
|
||||
model: Option<String>,
|
||||
|
||||
/// Override default LLM provider
|
||||
#[arg(long)]
|
||||
provider: Option<String>,
|
||||
|
||||
/// Verbosity level (-v summary, -vv full details)
|
||||
#[arg(short, long, action = clap::ArgAction::Count)]
|
||||
verbose: u8,
|
||||
}
|
||||
|
||||
#[derive(Args)]
|
||||
struct ValidateArgs {
|
||||
/// Path to the .dot pipeline file
|
||||
pipeline: PathBuf,
|
||||
}
|
||||
```
|
||||
|
||||
## Verbosity levels
|
||||
|
||||
The `-v` flag uses `clap::ArgAction::Count` to support two levels. Both levels write to stderr.
|
||||
|
||||
### `-v` (verbose) -- one-line summary per event
|
||||
|
||||
Prints the event kind and key identifying fields on a single line. Matches the style of the C reference implementation (kind + node + attempt + optional data), extended with the richer fields available in the Rust `PipelineEvent` enum.
|
||||
|
||||
```
|
||||
[PIPELINE_STARTED] name=Deploy id=abc-123
|
||||
[STAGE_STARTED] name=build index=1
|
||||
[STAGE_COMPLETED] name=build index=1 duration=4523ms
|
||||
[STAGE_RETRYING] name=test index=2 attempt=2 delay=200ms
|
||||
[STAGE_FAILED] name=test index=2 error="assertion failed" will_retry=true
|
||||
[PARALLEL_STARTED] branches=3
|
||||
[PARALLEL_BRANCH_STARTED] branch=lint index=0
|
||||
[PARALLEL_BRANCH_COMPLETED] branch=lint index=0 duration=1200ms success=true
|
||||
[PARALLEL_COMPLETED] duration=5100ms succeeded=2 failed=1
|
||||
[INTERVIEW_STARTED] stage=review_gate question="Approve changes?"
|
||||
[INTERVIEW_COMPLETED] question="Approve changes?" answer="Approve" duration=12340ms
|
||||
[INTERVIEW_TIMEOUT] stage=review_gate duration=30000ms
|
||||
[CHECKPOINT_SAVED] node=test
|
||||
[PIPELINE_COMPLETED] duration=45230ms artifacts=3
|
||||
[PIPELINE_FAILED] error="goal gate unsatisfied" duration=32100ms
|
||||
```
|
||||
|
||||
No JSON, no multi-line output. Suitable for tailing in a terminal alongside normal output.
|
||||
|
||||
### `-vv` (very verbose) -- full event details
|
||||
|
||||
Prints every event as nicely formatted, multi-line output with all fields. Uses indented key-value pairs under a header line.
|
||||
|
||||
```
|
||||
── STAGE_COMPLETED ──────────────────────────
|
||||
name: build
|
||||
index: 1
|
||||
duration_ms: 4523
|
||||
|
||||
── STAGE_FAILED ─────────────────────────────
|
||||
name: test
|
||||
index: 2
|
||||
error: assertion failed
|
||||
will_retry: true
|
||||
|
||||
── PARALLEL_COMPLETED ───────────────────────
|
||||
duration_ms: 5100
|
||||
success_count: 2
|
||||
failure_count: 1
|
||||
|
||||
── INTERVIEW_COMPLETED ──────────────────────
|
||||
question: Approve changes?
|
||||
answer: Approve
|
||||
duration_ms: 12340
|
||||
```
|
||||
|
||||
Every field on the event variant is printed. This is the "dump everything" mode for debugging pipeline behavior.
|
||||
|
||||
## Output conventions
|
||||
|
||||
- **stdout**: Pipeline summary, result status.
|
||||
- **stderr**: Warnings, verbose events, progress messages, LLM provider warnings.
|
||||
|
||||
## Error handling
|
||||
|
||||
| Condition | Behavior |
|
||||
|-----------|----------|
|
||||
| .dot file not found / unreadable | Print error to stderr, exit 1 |
|
||||
| Parse failure | Print parse error to stderr, exit 1 |
|
||||
| Validation errors | Print each error to stderr, exit 1 |
|
||||
| No LLM providers (without --dry-run) | Warn to stderr, continue in dry-run mode |
|
||||
| Engine execution error | Print error to stderr, exit 1 |
|
||||
| Pipeline completes with FAIL | Print result, exit 1 |
|
||||
| Pipeline completes with SUCCESS/PARTIAL_SUCCESS | Print result, exit 0 |
|
||||
|
||||
All errors go through `anyhow` at the CLI boundary. The `main` function catches the result and formats it.
|
||||
|
||||
## File layout
|
||||
|
||||
```
|
||||
crates/attractor/
|
||||
Cargo.toml -- add [[bin]], clap, anyhow, dotenvy, chrono deps
|
||||
src/
|
||||
main.rs -- entry point: dotenvy, clap parse, dispatch
|
||||
cli/
|
||||
mod.rs -- re-exports
|
||||
run.rs -- run_command()
|
||||
validate.rs -- validate_command()
|
||||
lib.rs -- existing library (unchanged)
|
||||
...
|
||||
```
|
||||
|
||||
## Cargo.toml changes
|
||||
|
||||
Add to `crates/attractor/Cargo.toml`:
|
||||
|
||||
```toml
|
||||
[[bin]]
|
||||
name = "attractor"
|
||||
path = "src/main.rs"
|
||||
|
||||
[dependencies]
|
||||
# ... existing deps ...
|
||||
clap.workspace = true
|
||||
anyhow.workspace = true
|
||||
dotenvy.workspace = true
|
||||
chrono.workspace = true
|
||||
```
|
||||
|
||||
Add `anyhow` to workspace root `Cargo.toml` `[workspace.dependencies]`:
|
||||
|
||||
```toml
|
||||
anyhow = "1"
|
||||
```
|
||||
|
||||
(Already present in workspace deps.)
|
||||
|
||||
## What this design does NOT cover
|
||||
|
||||
- Web/SSE server mode (`attractor serve`) -- separate subcommand later
|
||||
- JSON/structured output mode (`--output json`) -- can be added later
|
||||
- Signal handling (Ctrl-C graceful shutdown) -- defer to follow-up
|
||||
- Config file support (e.g., `~/.config/attractor/config.toml`) -- not needed yet
|
||||
654
spec-dod-multimodel.dot
Normal file
654
spec-dod-multimodel.dot
Normal file
|
|
@ -0,0 +1,654 @@
|
|||
digraph SpecDoDMultiModel {
|
||||
graph [
|
||||
goal="Satisfy every Definition of Done checkbox across all three attractor-main specs (unified-llm-spec.md, coding-agent-loop-spec.md, attractor-spec.md). The implementation is in pure C11 under src/ and include/. Do NOT modify the spec files. Only modify implementation code. Uses multi-model consensus: Opus 4.6 and GPT-5.2 compete on audits and planning, GPT-5.2-codex and Opus 4.6 alternate on implementation.",
|
||||
default_max_retry="3",
|
||||
retry_target="triage_merge",
|
||||
default_fidelity="full",
|
||||
model_stylesheet="
|
||||
* { llm_model: claude-opus-4-6; llm_provider: anthropic; }
|
||||
.opus { llm_model: claude-opus-4-6; llm_provider: anthropic; reasoning_effort: high; }
|
||||
.gpt { llm_model: gpt-5.2; llm_provider: openai; reasoning_effort: high; }
|
||||
.codex { llm_model: gpt-5.2-codex; llm_provider: openai; reasoning_effort: high; }
|
||||
.merge { llm_model: claude-opus-4-6; llm_provider: anthropic; reasoning_effort: high; }
|
||||
"
|
||||
]
|
||||
|
||||
start [shape=Mdiamond]
|
||||
exit [shape=Msquare]
|
||||
|
||||
/*========================================================================
|
||||
* PHASE 1 — Dual Independent Audits (interleaved, fidelity-isolated)
|
||||
*
|
||||
* Each spec is audited by both models before moving to the next spec.
|
||||
* Audit nodes use fidelity="truncate" so they only see the graph goal
|
||||
* and NOT each other's responses — prevents anchoring bias.
|
||||
* Full responses are still stored as response.<node_id> for later use.
|
||||
*======================================================================*/
|
||||
|
||||
/* ---- LLM spec: both models ---- */
|
||||
|
||||
audit_llm_opus [
|
||||
label="Opus: Audit LLM DoD",
|
||||
shape=box,
|
||||
class="opus",
|
||||
fidelity="truncate",
|
||||
prompt="Read attractor-main/unified-llm-spec.md Section 8 (Definition of Done) in full. Then read every source file under src/llm/, src/util/, and include/llm/, include/util/.
|
||||
|
||||
For EACH checkbox in sections 8.1 through 8.10, evaluate whether the current C implementation satisfies it. Be strict — a checkbox is only checked if the feature is fully implemented and would work correctly at runtime.
|
||||
|
||||
Output a JSON object:
|
||||
{
|
||||
\"spec\": \"unified-llm\",
|
||||
\"model\": \"opus\",
|
||||
\"sections\": {
|
||||
\"8.1\": { \"title\": \"Core Infrastructure\", \"items\": [ {\"text\": \"...\", \"pass\": true/false, \"reason\": \"...\"} ] },
|
||||
...
|
||||
},
|
||||
\"total\": N,
|
||||
\"passed\": M,
|
||||
\"failed\": K,
|
||||
\"failed_items\": [ {\"section\": \"8.2\", \"text\": \"...\", \"reason\": \"...\"} ]
|
||||
}
|
||||
|
||||
Be thorough. Check every single checkbox."
|
||||
]
|
||||
|
||||
audit_llm_gpt [
|
||||
label="GPT-5.2: Audit LLM DoD",
|
||||
shape=box,
|
||||
class="gpt",
|
||||
fidelity="truncate",
|
||||
prompt="Read attractor-main/unified-llm-spec.md Section 8 (Definition of Done) in full. Then read every source file under src/llm/, src/util/, and include/llm/, include/util/.
|
||||
|
||||
For EACH checkbox in sections 8.1 through 8.10, evaluate whether the current C implementation satisfies it. Be strict — a checkbox is only checked if the feature is fully implemented and would work correctly at runtime.
|
||||
|
||||
Output a JSON object:
|
||||
{
|
||||
\"spec\": \"unified-llm\",
|
||||
\"model\": \"gpt-5.2\",
|
||||
\"sections\": {
|
||||
\"8.1\": { \"title\": \"Core Infrastructure\", \"items\": [ {\"text\": \"...\", \"pass\": true/false, \"reason\": \"...\"} ] },
|
||||
...
|
||||
},
|
||||
\"total\": N,
|
||||
\"passed\": M,
|
||||
\"failed\": K,
|
||||
\"failed_items\": [ {\"section\": \"8.2\", \"text\": \"...\", \"reason\": \"...\"} ]
|
||||
}
|
||||
|
||||
Be thorough. Check every single checkbox."
|
||||
]
|
||||
|
||||
/* ---- Agent spec: both models ---- */
|
||||
|
||||
audit_agent_opus [
|
||||
label="Opus: Audit Agent DoD",
|
||||
shape=box,
|
||||
class="opus",
|
||||
fidelity="truncate",
|
||||
prompt="Read attractor-main/coding-agent-loop-spec.md Section 9 (Definition of Done) in full. Then read every source file under src/agent/ and include/agent/.
|
||||
|
||||
For EACH checkbox in sections 9.1 through 9.13, evaluate whether the current C implementation satisfies it. Be strict.
|
||||
|
||||
Output a JSON object:
|
||||
{
|
||||
\"spec\": \"coding-agent-loop\",
|
||||
\"model\": \"opus\",
|
||||
\"sections\": { ... },
|
||||
\"total\": N,
|
||||
\"passed\": M,
|
||||
\"failed\": K,
|
||||
\"failed_items\": [ {\"section\": \"9.1\", \"text\": \"...\", \"reason\": \"...\"} ]
|
||||
}
|
||||
|
||||
Be thorough. Check every single checkbox."
|
||||
]
|
||||
|
||||
audit_agent_gpt [
|
||||
label="GPT-5.2: Audit Agent DoD",
|
||||
shape=box,
|
||||
class="gpt",
|
||||
fidelity="truncate",
|
||||
prompt="Read attractor-main/coding-agent-loop-spec.md Section 9 (Definition of Done) in full. Then read every source file under src/agent/ and include/agent/.
|
||||
|
||||
For EACH checkbox in sections 9.1 through 9.13, evaluate whether the current C implementation satisfies it. Be strict.
|
||||
|
||||
Output a JSON object:
|
||||
{
|
||||
\"spec\": \"coding-agent-loop\",
|
||||
\"model\": \"gpt-5.2\",
|
||||
\"sections\": { ... },
|
||||
\"total\": N,
|
||||
\"passed\": M,
|
||||
\"failed\": K,
|
||||
\"failed_items\": [ {\"section\": \"9.1\", \"text\": \"...\", \"reason\": \"...\"} ]
|
||||
}
|
||||
|
||||
Be thorough. Check every single checkbox."
|
||||
]
|
||||
|
||||
/* ---- Attractor spec: both models ---- */
|
||||
|
||||
audit_attractor_opus [
|
||||
label="Opus: Audit Attractor DoD",
|
||||
shape=box,
|
||||
class="opus",
|
||||
fidelity="truncate",
|
||||
prompt="Read attractor-main/attractor-spec.md Section 11 (Definition of Done) in full. Then read every source file under src/attractor/, src/main.c, and include/attractor/.
|
||||
|
||||
For EACH checkbox in sections 11.1 through 11.13, evaluate whether the current C implementation satisfies it. Be strict.
|
||||
|
||||
Output a JSON object:
|
||||
{
|
||||
\"spec\": \"attractor\",
|
||||
\"model\": \"opus\",
|
||||
\"sections\": { ... },
|
||||
\"total\": N,
|
||||
\"passed\": M,
|
||||
\"failed\": K,
|
||||
\"failed_items\": [ {\"section\": \"11.1\", \"text\": \"...\", \"reason\": \"...\"} ]
|
||||
}
|
||||
|
||||
Be thorough. Check every single checkbox."
|
||||
]
|
||||
|
||||
audit_attractor_gpt [
|
||||
label="GPT-5.2: Audit Attractor DoD",
|
||||
shape=box,
|
||||
class="gpt",
|
||||
fidelity="truncate",
|
||||
prompt="Read attractor-main/attractor-spec.md Section 11 (Definition of Done) in full. Then read every source file under src/attractor/, src/main.c, and include/attractor/.
|
||||
|
||||
For EACH checkbox in sections 11.1 through 11.13, evaluate whether the current C implementation satisfies it. Be strict.
|
||||
|
||||
Output a JSON object:
|
||||
{
|
||||
\"spec\": \"attractor\",
|
||||
\"model\": \"gpt-5.2\",
|
||||
\"sections\": { ... },
|
||||
\"total\": N,
|
||||
\"passed\": M,
|
||||
\"failed\": K,
|
||||
\"failed_items\": [ {\"section\": \"11.1\", \"text\": \"...\", \"reason\": \"...\"} ]
|
||||
}
|
||||
|
||||
Be thorough. Check every single checkbox."
|
||||
]
|
||||
|
||||
/*========================================================================
|
||||
* PHASE 2 — Cross-Critique (fidelity=full to see all response.* keys)
|
||||
*
|
||||
* Each model reviews the other's audit. Like megaplan's Compete phase:
|
||||
* independent work first, then adversarial review.
|
||||
*======================================================================*/
|
||||
|
||||
critique_by_gpt [
|
||||
label="GPT-5.2: Critique Opus Audits",
|
||||
shape=box,
|
||||
class="gpt",
|
||||
fidelity="full",
|
||||
prompt="You have all six audit reports available in context. The full outputs are in these context keys:
|
||||
|
||||
OPUS AUDITS:
|
||||
- response.audit_llm_opus — Opus's audit of unified-llm-spec.md Section 8
|
||||
- response.audit_agent_opus — Opus's audit of coding-agent-loop-spec.md Section 9
|
||||
- response.audit_attractor_opus — Opus's audit of attractor-spec.md Section 11
|
||||
|
||||
GPT AUDITS (your own):
|
||||
- response.audit_llm_gpt — your audit of unified-llm-spec.md Section 8
|
||||
- response.audit_agent_gpt — your audit of coding-agent-loop-spec.md Section 9
|
||||
- response.audit_attractor_gpt — your audit of attractor-spec.md Section 11
|
||||
|
||||
Compare them item by item. For every DoD checkbox where the two models DISAGREE (one says pass, the other says fail), re-read the relevant spec section and source file to determine who is correct.
|
||||
|
||||
Also identify items that one model flagged but the other missed entirely.
|
||||
|
||||
Output a JSON object:
|
||||
{
|
||||
\"agreements\": { \"both_pass\": N, \"both_fail\": N },
|
||||
\"disagreements\": [
|
||||
{
|
||||
\"spec\": \"...\", \"section\": \"...\", \"text\": \"...\",
|
||||
\"opus_says\": \"pass|fail\", \"gpt_says\": \"pass|fail\",
|
||||
\"verdict\": \"pass|fail\",
|
||||
\"reasoning\": \"...\"
|
||||
}
|
||||
],
|
||||
\"missed_by_opus\": [ {\"spec\": \"...\", \"section\": \"...\", \"text\": \"...\", \"reason\": \"...\"} ],
|
||||
\"missed_by_gpt\": [ {\"spec\": \"...\", \"section\": \"...\", \"text\": \"...\", \"reason\": \"...\"} ]
|
||||
}
|
||||
|
||||
Be rigorous. When in doubt, fail the checkbox — strictness prevents false confidence."
|
||||
]
|
||||
|
||||
critique_by_opus [
|
||||
label="Opus: Critique GPT-5.2 Audits",
|
||||
shape=box,
|
||||
class="opus",
|
||||
fidelity="full",
|
||||
prompt="You have all six audit reports available in context. The full outputs are in these context keys:
|
||||
|
||||
GPT AUDITS:
|
||||
- response.audit_llm_gpt — GPT-5.2's audit of unified-llm-spec.md Section 8
|
||||
- response.audit_agent_gpt — GPT-5.2's audit of coding-agent-loop-spec.md Section 9
|
||||
- response.audit_attractor_gpt — GPT-5.2's audit of attractor-spec.md Section 11
|
||||
|
||||
OPUS AUDITS (your own):
|
||||
- response.audit_llm_opus — your audit of unified-llm-spec.md Section 8
|
||||
- response.audit_agent_opus — your audit of coding-agent-loop-spec.md Section 9
|
||||
- response.audit_attractor_opus — your audit of attractor-spec.md Section 11
|
||||
|
||||
Compare them item by item. For every DoD checkbox where the two models DISAGREE (one says pass, the other says fail), re-read the relevant spec section and source file to determine who is correct.
|
||||
|
||||
Also identify items that one model flagged but the other missed entirely.
|
||||
|
||||
Output a JSON object:
|
||||
{
|
||||
\"agreements\": { \"both_pass\": N, \"both_fail\": N },
|
||||
\"disagreements\": [
|
||||
{
|
||||
\"spec\": \"...\", \"section\": \"...\", \"text\": \"...\",
|
||||
\"opus_says\": \"pass|fail\", \"gpt_says\": \"pass|fail\",
|
||||
\"verdict\": \"pass|fail\",
|
||||
\"reasoning\": \"...\"
|
||||
}
|
||||
],
|
||||
\"missed_by_opus\": [ {\"spec\": \"...\", \"section\": \"...\", \"text\": \"...\", \"reason\": \"...\"} ],
|
||||
\"missed_by_gpt\": [ {\"spec\": \"...\", \"section\": \"...\", \"text\": \"...\", \"reason\": \"...\"} ]
|
||||
}
|
||||
|
||||
Be rigorous. When in doubt, fail the checkbox — strictness prevents false confidence."
|
||||
]
|
||||
|
||||
/*========================================================================
|
||||
* PHASE 3 — Audit Consensus
|
||||
*
|
||||
* Merge all findings into a single agreed-upon truth.
|
||||
* Like megaplan's Merge phase: best ideas from both, disagreements resolved.
|
||||
*======================================================================*/
|
||||
|
||||
audit_consensus [
|
||||
label="Merge: Audit Consensus",
|
||||
shape=box,
|
||||
class="merge",
|
||||
fidelity="full",
|
||||
prompt="You have all prior audit and critique outputs in context. The key inputs are:
|
||||
|
||||
SIX AUDIT REPORTS (context keys response.audit_llm_opus, response.audit_agent_opus, response.audit_attractor_opus, response.audit_llm_gpt, response.audit_agent_gpt, response.audit_attractor_gpt)
|
||||
|
||||
TWO CROSS-CRITIQUES (context keys response.critique_by_gpt, response.critique_by_opus)
|
||||
|
||||
Produce a single definitive audit result. Resolution rules:
|
||||
1. If BOTH models agree a checkbox passes → pass
|
||||
2. If BOTH models agree a checkbox fails → fail
|
||||
3. If they DISAGREE, use the cross-critique verdicts. If the critiques also disagree, re-read the spec and code yourself and make the call. When in doubt, fail it.
|
||||
4. Include any items that were missed by one model but caught by the other.
|
||||
|
||||
Output a JSON object:
|
||||
{
|
||||
\"spec_results\": {
|
||||
\"unified-llm\": { \"total\": N, \"passed\": M, \"failed\": K, \"failed_items\": [...] },
|
||||
\"coding-agent-loop\": { ... },
|
||||
\"attractor\": { ... }
|
||||
},
|
||||
\"consensus_total\": N,
|
||||
\"consensus_passed\": M,
|
||||
\"consensus_failed\": K,
|
||||
\"disagreements_resolved\": N,
|
||||
\"all_failed_items\": [ {\"spec\": \"...\", \"section\": \"...\", \"text\": \"...\", \"reason\": \"...\", \"agreed_by\": \"both|opus_only|gpt_only|resolved\"} ]
|
||||
}"
|
||||
]
|
||||
|
||||
/*========================================================================
|
||||
* PHASE 4 — Dual Triage
|
||||
*
|
||||
* Both models independently prioritize the failures, then merge.
|
||||
* Different models weight different risks differently — consensus is stronger.
|
||||
*======================================================================*/
|
||||
|
||||
triage_opus [
|
||||
label="Opus: Triage & Prioritize",
|
||||
shape=box,
|
||||
class="opus",
|
||||
fidelity="full",
|
||||
prompt="The consensus audit results are in context key response.audit_consensus. Parse the all_failed_items list from that JSON and triage every failing DoD checkbox.
|
||||
|
||||
Group failures into:
|
||||
1. IMPLEMENTABLE — can be fixed by writing/modifying C code (functions, handlers, logic)
|
||||
2. STRUCTURAL — requires new files, new modules, or significant architecture changes
|
||||
3. DEFERRED — requires external resources (real API keys, network access, interactive testing) that cannot be done in a code-only pass
|
||||
|
||||
For each IMPLEMENTABLE item, identify the exact file(s) to modify and briefly describe the fix. Rank them by impact (most important first).
|
||||
|
||||
Output a JSON object:
|
||||
{
|
||||
\"model\": \"opus\",
|
||||
\"total_failing\": N,
|
||||
\"implementable\": [ {\"rank\": 1, \"spec\": \"...\", \"section\": \"...\", \"text\": \"...\", \"files\": [\"...\"], \"fix\": \"...\", \"impact\": \"high|medium|low\"} ],
|
||||
\"structural\": [ ... ],
|
||||
\"deferred\": [ ... ]
|
||||
}"
|
||||
]
|
||||
|
||||
triage_gpt [
|
||||
label="GPT-5.2: Triage & Prioritize",
|
||||
shape=box,
|
||||
class="gpt",
|
||||
fidelity="full",
|
||||
prompt="The consensus audit results are in context key response.audit_consensus. Parse the all_failed_items list from that JSON and triage every failing DoD checkbox.
|
||||
|
||||
Group failures into:
|
||||
1. IMPLEMENTABLE — can be fixed by writing/modifying C code (functions, handlers, logic)
|
||||
2. STRUCTURAL — requires new files, new modules, or significant architecture changes
|
||||
3. DEFERRED — requires external resources (real API keys, network access, interactive testing) that cannot be done in a code-only pass
|
||||
|
||||
For each IMPLEMENTABLE item, identify the exact file(s) to modify and briefly describe the fix. Rank them by impact (most important first).
|
||||
|
||||
Output a JSON object:
|
||||
{
|
||||
\"model\": \"gpt-5.2\",
|
||||
\"total_failing\": N,
|
||||
\"implementable\": [ {\"rank\": 1, \"spec\": \"...\", \"section\": \"...\", \"text\": \"...\", \"files\": [\"...\"], \"fix\": \"...\", \"impact\": \"high|medium|low\"} ],
|
||||
\"structural\": [ ... ],
|
||||
\"deferred\": [ ... ]
|
||||
}"
|
||||
]
|
||||
|
||||
triage_merge [
|
||||
label="Merge: Triage Consensus",
|
||||
shape=box,
|
||||
class="merge",
|
||||
fidelity="full",
|
||||
prompt="You have two triage reports in context: response.triage_opus and response.triage_gpt. Merge them into a single prioritized work plan.
|
||||
|
||||
Resolution rules:
|
||||
1. If both models classify an item the same way (IMPLEMENTABLE/STRUCTURAL/DEFERRED) → keep that classification
|
||||
2. If they disagree on classification → take the MORE ACTIONABLE classification (prefer IMPLEMENTABLE over STRUCTURAL over DEFERRED)
|
||||
3. For ranking, average the ranks and re-sort. If one model identified files/fixes the other didn't, include all suggestions.
|
||||
4. Deduplicate items that both models identified.
|
||||
|
||||
Output a JSON object:
|
||||
{
|
||||
\"total_failing\": N,
|
||||
\"implementable\": [ {\"spec\": \"...\", \"section\": \"...\", \"text\": \"...\", \"files\": [\"...\"], \"fix\": \"...\", \"opus_rank\": N, \"gpt_rank\": N} ],
|
||||
\"structural\": [ ... ],
|
||||
\"deferred\": [ ... ],
|
||||
\"classification_disagreements\": N,
|
||||
\"verdict\": \"all_clear\" | \"has_fixes\" | \"only_deferred\"
|
||||
}
|
||||
|
||||
If total_failing == 0 or verdict == \"only_deferred\", set preferred_next_label to \"Done\".
|
||||
Otherwise set preferred_next_label to \"Fix\"."
|
||||
]
|
||||
|
||||
/*========================================================================
|
||||
* PHASE 5 — Multi-Model Implementation
|
||||
*
|
||||
* Codex implements, Opus reviews and corrects, Codex validates.
|
||||
* Like megaplan's draft→critique→merge but for code.
|
||||
*======================================================================*/
|
||||
|
||||
fix_codex [
|
||||
label="Codex: Implement Fixes",
|
||||
shape=box,
|
||||
class="codex",
|
||||
goal_gate=true,
|
||||
fidelity="full",
|
||||
prompt="The merged triage report is in context key response.triage_merge. It contains a prioritized list of IMPLEMENTABLE DoD failures.
|
||||
|
||||
Pick the top 5 most impactful items (or all if fewer than 5) and implement the fixes in pure C11.
|
||||
|
||||
For each fix:
|
||||
1. Read the relevant source file(s)
|
||||
2. Make the minimal change needed to satisfy the DoD checkbox
|
||||
3. Write the modified file(s)
|
||||
4. Verify the fix compiles (mentally trace includes and types)
|
||||
|
||||
Constraints:
|
||||
- Do NOT modify any files under attractor-main/ (those are the specs)
|
||||
- Do NOT add external dependencies beyond what's already used (libcurl, pthreads, math)
|
||||
- Keep changes minimal and focused — one checkbox per fix
|
||||
- Maintain the existing code style
|
||||
|
||||
After implementing, output:
|
||||
{
|
||||
\"model\": \"codex\",
|
||||
\"fixes_applied\": [ {\"spec\": \"...\", \"section\": \"...\", \"text\": \"...\", \"files_changed\": [\"...\"], \"description\": \"...\"} ],
|
||||
\"count\": N,
|
||||
\"remaining_implementable\": M
|
||||
}"
|
||||
]
|
||||
|
||||
review_fix_opus [
|
||||
label="Opus: Review & Fix",
|
||||
shape=box,
|
||||
class="opus",
|
||||
goal_gate=true,
|
||||
fidelity="full",
|
||||
prompt="Codex just implemented a batch of fixes. Its report is in context key response.fix_codex.
|
||||
|
||||
PART A — Review Codex's work:
|
||||
1. Read every file that Codex modified (check the files_changed lists in response.fix_codex)
|
||||
2. For each fix, verify it actually satisfies the DoD checkbox it claims to address
|
||||
3. Check for: correctness, edge cases, style consistency, missing error handling
|
||||
4. If a fix is wrong or incomplete, rewrite it correctly
|
||||
|
||||
PART B — Implement additional fixes:
|
||||
5. From the remaining IMPLEMENTABLE items (see response.triage_merge for the full list), pick up to 5 more and implement them
|
||||
6. Follow the same constraints as Codex (pure C11, no new deps, minimal changes)
|
||||
|
||||
Output:
|
||||
{
|
||||
\"model\": \"opus\",
|
||||
\"codex_fixes_reviewed\": N,
|
||||
\"codex_fixes_correct\": N,
|
||||
\"codex_fixes_corrected\": [ {\"spec\": \"...\", \"section\": \"...\", \"issue\": \"...\", \"correction\": \"...\"} ],
|
||||
\"additional_fixes\": [ {\"spec\": \"...\", \"section\": \"...\", \"text\": \"...\", \"files_changed\": [\"...\"], \"description\": \"...\"} ],
|
||||
\"total_fixes_this_round\": N,
|
||||
\"remaining_implementable\": M
|
||||
}"
|
||||
]
|
||||
|
||||
review_codex [
|
||||
label="Codex: Validate All Changes",
|
||||
shape=box,
|
||||
class="codex",
|
||||
fidelity="full",
|
||||
prompt="Opus reviewed your fixes and implemented additional ones. Its report is in context key response.review_fix_opus. Your original report is in response.fix_codex.
|
||||
|
||||
Validate the full set of changes from this round:
|
||||
1. Read every file modified in this round (check files_changed in both response.fix_codex and response.review_fix_opus)
|
||||
2. Check each change for correctness: does it satisfy its DoD checkbox?
|
||||
3. Check for regressions: did any fix break something else?
|
||||
4. Check for consistency: do all the changes work together?
|
||||
|
||||
Output:
|
||||
{
|
||||
\"model\": \"codex\",
|
||||
\"total_changes_reviewed\": N,
|
||||
\"all_correct\": true/false,
|
||||
\"issues_found\": [ {\"file\": \"...\", \"issue\": \"...\", \"severity\": \"critical|minor\"} ],
|
||||
\"remaining_implementable\": M
|
||||
}
|
||||
|
||||
If issues_found contains any critical items, set preferred_next_label to \"More fixes needed\".
|
||||
If remaining_implementable > 0 and no critical issues, set preferred_next_label to \"More fixes needed\".
|
||||
Otherwise set preferred_next_label to \"Ready for build\"."
|
||||
]
|
||||
|
||||
/*========================================================================
|
||||
* PHASE 6 — Build Verification
|
||||
*======================================================================*/
|
||||
|
||||
build_check [
|
||||
label="Build & Smoke Test",
|
||||
shape=parallelogram,
|
||||
tool_command="cd /Users/justin.mccarthy/code/jm-attractor && make clean && make 2>&1 && echo '---BUILD OK---' && ./attractor --dry-run test/simple.dot 2>&1 && ./attractor --dry-run test/branching.dot 2>&1 && ./attractor --dry-run test/styled.dot 2>&1 && ./attractor --dry-run test/parallel.dot 2>&1 && ./attractor --dry-run test/conditions.dot 2>&1 && echo '---ALL TESTS PASSED---'",
|
||||
timeout="120s"
|
||||
]
|
||||
|
||||
build_fix [
|
||||
label="Opus: Fix Build Errors",
|
||||
shape=box,
|
||||
class="opus",
|
||||
fidelity="full",
|
||||
prompt="The build or smoke tests failed. The build output is in context key tool.output. Diagnose the compilation errors or test failures and fix them.
|
||||
|
||||
Read the relevant source files, identify the issue, and write corrected versions. Common issues:
|
||||
- Missing includes
|
||||
- Type mismatches
|
||||
- Undeclared functions
|
||||
- Linker errors
|
||||
|
||||
Output the fixes applied and ensure the code will compile cleanly with: cc -Wall -Wextra -std=c11"
|
||||
]
|
||||
|
||||
/*========================================================================
|
||||
* PHASE 7 — Dual Final Audit (interleaved, fidelity-isolated)
|
||||
*
|
||||
* Both models independently verify the fixes worked.
|
||||
* If either model finds a remaining failure, it counts.
|
||||
*======================================================================*/
|
||||
|
||||
final_audit_opus [
|
||||
label="Opus: Final Verification",
|
||||
shape=box,
|
||||
class="opus",
|
||||
fidelity="full",
|
||||
prompt="This is a verification pass. The items that were previously failing are listed in context key response.triage_merge (the implementable list). The fixes applied are in response.fix_codex and response.review_fix_opus.
|
||||
|
||||
Re-read all three spec DoD sections:
|
||||
- attractor-main/unified-llm-spec.md Section 8
|
||||
- attractor-main/coding-agent-loop-spec.md Section 9
|
||||
- attractor-main/attractor-spec.md Section 11
|
||||
|
||||
And re-read the implementation files that were changed in this iteration.
|
||||
|
||||
Check ONLY the items that were previously failing. Have they been fixed?
|
||||
|
||||
Output:
|
||||
{
|
||||
\"model\": \"opus\",
|
||||
\"verified_fixed\": [ {\"spec\": \"...\", \"section\": \"...\", \"text\": \"...\"} ],
|
||||
\"still_failing\": [ {\"spec\": \"...\", \"section\": \"...\", \"text\": \"...\", \"reason\": \"...\"} ],
|
||||
\"newly_broken\": [ ... ],
|
||||
\"remaining_total\": N
|
||||
}"
|
||||
]
|
||||
|
||||
final_audit_gpt [
|
||||
label="GPT-5.2: Final Verification",
|
||||
shape=box,
|
||||
class="gpt",
|
||||
fidelity="full",
|
||||
prompt="This is a verification pass. The items that were previously failing are listed in context key response.triage_merge (the implementable list). The fixes applied are in response.fix_codex and response.review_fix_opus.
|
||||
|
||||
Re-read all three spec DoD sections:
|
||||
- attractor-main/unified-llm-spec.md Section 8
|
||||
- attractor-main/coding-agent-loop-spec.md Section 9
|
||||
- attractor-main/attractor-spec.md Section 11
|
||||
|
||||
And re-read the implementation files that were changed in this iteration.
|
||||
|
||||
Check ONLY the items that were previously failing. Have they been fixed?
|
||||
|
||||
Output:
|
||||
{
|
||||
\"model\": \"gpt-5.2\",
|
||||
\"verified_fixed\": [ {\"spec\": \"...\", \"section\": \"...\", \"text\": \"...\"} ],
|
||||
\"still_failing\": [ {\"spec\": \"...\", \"section\": \"...\", \"text\": \"...\", \"reason\": \"...\"} ],
|
||||
\"newly_broken\": [ ... ],
|
||||
\"remaining_total\": N
|
||||
}"
|
||||
]
|
||||
|
||||
final_consensus [
|
||||
label="Merge: Final Consensus",
|
||||
shape=box,
|
||||
class="merge",
|
||||
fidelity="full",
|
||||
prompt="You have final audit results from both models in context: response.final_audit_opus and response.final_audit_gpt. Merge them into a definitive status.
|
||||
|
||||
Rules:
|
||||
1. An item is only \"verified_fixed\" if BOTH models agree it's fixed
|
||||
2. If EITHER model says an item is still failing, it counts as still failing
|
||||
3. Union all newly_broken items from both models
|
||||
|
||||
Output:
|
||||
{
|
||||
\"verified_fixed\": [ {\"spec\": \"...\", \"section\": \"...\", \"text\": \"...\", \"agreed_by\": \"both|opus_only|gpt_only\"} ],
|
||||
\"still_failing\": [ {\"spec\": \"...\", \"section\": \"...\", \"text\": \"...\", \"flagged_by\": \"both|opus_only|gpt_only\", \"reason\": \"...\"} ],
|
||||
\"newly_broken\": [ ... ],
|
||||
\"remaining_total\": N
|
||||
}
|
||||
|
||||
If remaining_total == 0 (ignoring DEFERRED items), set preferred_next_label to \"Complete\".
|
||||
Otherwise set preferred_next_label to \"More work needed\"."
|
||||
]
|
||||
|
||||
/*========================================================================
|
||||
* PHASE 8 — Human Gate
|
||||
*======================================================================*/
|
||||
|
||||
review_gate [
|
||||
label="A) Accept & finish\nB) Push for another round",
|
||||
shape=hexagon
|
||||
]
|
||||
|
||||
/*========================================================================
|
||||
* EDGES — Serial interleaved chain
|
||||
*
|
||||
* The engine is single-path, so we interleave model audits per-spec.
|
||||
* fidelity="truncate" on audit nodes prevents cross-model anchoring.
|
||||
*======================================================================*/
|
||||
|
||||
/* Phase 1: Interleaved audits (Opus then GPT for each spec) */
|
||||
start -> audit_llm_opus
|
||||
audit_llm_opus -> audit_llm_gpt
|
||||
audit_llm_gpt -> audit_agent_opus
|
||||
audit_agent_opus -> audit_agent_gpt
|
||||
audit_agent_gpt -> audit_attractor_opus
|
||||
audit_attractor_opus -> audit_attractor_gpt
|
||||
|
||||
/* Phase 2: Cross-critique (now sequential — GPT critiques Opus, then Opus critiques GPT) */
|
||||
audit_attractor_gpt -> critique_by_gpt
|
||||
critique_by_gpt -> critique_by_opus
|
||||
|
||||
/* Phase 3: Consensus */
|
||||
critique_by_opus -> audit_consensus
|
||||
|
||||
/* Phase 4: Dual triage (sequential — Opus then GPT then merge) */
|
||||
audit_consensus -> triage_opus
|
||||
triage_opus -> triage_gpt
|
||||
triage_gpt -> triage_merge
|
||||
|
||||
/* Triage decision */
|
||||
triage_merge -> exit [label="Done", condition="preferred_label=Done"]
|
||||
triage_merge -> fix_codex [label="Fix", condition="preferred_label=Fix", weight=10]
|
||||
triage_merge -> exit [label="Only deferred remain"]
|
||||
|
||||
/* Phase 5: Multi-model implementation (sequential alternation) */
|
||||
fix_codex -> review_fix_opus
|
||||
review_fix_opus -> review_codex
|
||||
|
||||
/* Implementation loop */
|
||||
review_codex -> fix_codex [label="More fixes needed", condition="preferred_label=More fixes needed", loop_restart=true]
|
||||
review_codex -> build_check [label="Ready for build", condition="preferred_label=Ready for build"]
|
||||
|
||||
/* Phase 6: Build */
|
||||
build_check -> final_audit_opus [label="Build OK", condition="outcome=success"]
|
||||
build_check -> build_fix [label="Build failed", condition="outcome=fail"]
|
||||
build_fix -> build_check
|
||||
|
||||
/* Phase 7: Dual final audit (sequential — Opus then GPT then consensus) */
|
||||
final_audit_opus -> final_audit_gpt
|
||||
final_audit_gpt -> final_consensus
|
||||
|
||||
/* Final decision */
|
||||
final_consensus -> review_gate [label="Complete", condition="preferred_label=Complete"]
|
||||
final_consensus -> triage_merge [label="More work needed", condition="preferred_label=More work needed"]
|
||||
|
||||
/* Phase 8: Human gate */
|
||||
review_gate -> exit [label="A) Accept"]
|
||||
review_gate -> triage_merge [label="B) Another round"]
|
||||
}
|
||||
252
spec-dod.dot
Normal file
252
spec-dod.dot
Normal file
|
|
@ -0,0 +1,252 @@
|
|||
digraph SpecDoD {
|
||||
graph [
|
||||
goal="Satisfy every Definition of Done checkbox across all three attractor specs (unified-llm-spec.md, coding-agent-loop-spec.md, attractor-spec.md). The implementation is in Rust under crates/. Do NOT modify the spec files. Only modify implementation code.",
|
||||
default_max_retry="3",
|
||||
retry_target="triage",
|
||||
model_stylesheet="
|
||||
* { llm_model: claude-opus-4-6; llm_provider: anthropic; }
|
||||
.audit { reasoning_effort: high; }
|
||||
.fix { llm_model: claude-opus-4-6; reasoning_effort: high; }
|
||||
#final_audit { reasoning_effort: high; }
|
||||
"
|
||||
]
|
||||
|
||||
start [shape=Mdiamond]
|
||||
exit [shape=Msquare]
|
||||
|
||||
/*------------------------------------------------------------------------
|
||||
* Phase 1: Baseline audit — read every DoD checkbox, check the code
|
||||
*----------------------------------------------------------------------*/
|
||||
|
||||
audit_llm [
|
||||
label="Audit: Unified LLM Client DoD",
|
||||
shape=box,
|
||||
class="audit",
|
||||
prompt="Read docs/specs/unified-llm-spec.md Section 8 (Definition of Done) in full. Then read every source file under crates/llm/src/.
|
||||
|
||||
For EACH checkbox in sections 8.1 through 8.10, evaluate whether the current Rust implementation satisfies it. Be strict — a checkbox is only checked if the feature is fully implemented and would work correctly at runtime.
|
||||
|
||||
Output a JSON object:
|
||||
{
|
||||
\"spec\": \"unified-llm\",
|
||||
\"sections\": {
|
||||
\"8.1\": { \"title\": \"Core Infrastructure\", \"items\": [ {\"text\": \"...\", \"pass\": true/false, \"reason\": \"...\"} ] },
|
||||
...
|
||||
},
|
||||
\"total\": N,
|
||||
\"passed\": M,
|
||||
\"failed\": K,
|
||||
\"failed_items\": [ {\"section\": \"8.2\", \"text\": \"...\", \"reason\": \"...\"} ]
|
||||
}
|
||||
|
||||
Be thorough. Check every single checkbox."
|
||||
]
|
||||
|
||||
audit_agent [
|
||||
label="Audit: Coding Agent Loop DoD",
|
||||
shape=box,
|
||||
class="audit",
|
||||
prompt="Read docs/specs/coding-agent-loop-spec.md Section 9 (Definition of Done) in full. Then read every source file under crates/agent/.
|
||||
|
||||
For EACH checkbox in sections 9.1 through 9.13, evaluate whether the current Rust implementation satisfies it. Be strict.
|
||||
|
||||
Output a JSON object:
|
||||
{
|
||||
\"spec\": \"coding-agent-loop\",
|
||||
\"sections\": { ... },
|
||||
\"total\": N,
|
||||
\"passed\": M,
|
||||
\"failed\": K,
|
||||
\"failed_items\": [ {\"section\": \"9.1\", \"text\": \"...\", \"reason\": \"...\"} ]
|
||||
}
|
||||
|
||||
Be thorough. Check every single checkbox."
|
||||
]
|
||||
|
||||
audit_attractor [
|
||||
label="Audit: Attractor Pipeline DoD",
|
||||
shape=box,
|
||||
class="audit",
|
||||
prompt="Read docs/specs/attractor-spec.md Section 11 (Definition of Done) in full. Then read every source file under crates/attractor/.
|
||||
|
||||
For EACH checkbox in sections 11.1 through 11.13, evaluate whether the current Rust implementation satisfies it. Be strict.
|
||||
|
||||
Output a JSON object:
|
||||
{
|
||||
\"spec\": \"attractor\",
|
||||
\"sections\": { ... },
|
||||
\"total\": N,
|
||||
\"passed\": M,
|
||||
\"failed\": K,
|
||||
\"failed_items\": [ {\"section\": \"11.1\", \"text\": \"...\", \"reason\": \"...\"} ]
|
||||
}
|
||||
|
||||
Be thorough. Check every single checkbox."
|
||||
]
|
||||
|
||||
/*------------------------------------------------------------------------
|
||||
* Phase 2: Triage — merge results, prioritize failures, decide next step
|
||||
*----------------------------------------------------------------------*/
|
||||
|
||||
triage [
|
||||
label="Triage & Prioritize",
|
||||
shape=box,
|
||||
prompt="You have three audit reports in context (from audit_llm, audit_agent, audit_attractor). Merge them into a single prioritized list of ALL failing DoD checkboxes.
|
||||
|
||||
Group failures into:
|
||||
1. IMPLEMENTABLE — can be fixed by writing/modifying Rust code (functions, handlers, logic)
|
||||
2. STRUCTURAL — requires new files, new modules, or significant architecture changes
|
||||
3. DEFERRED — requires external resources (real API keys, network access, interactive testing) that cannot be done in a code-only pass
|
||||
|
||||
For each IMPLEMENTABLE item, identify the exact file(s) to modify and briefly describe the fix.
|
||||
|
||||
Output a JSON object:
|
||||
{
|
||||
\"total_failing\": N,
|
||||
\"implementable\": [ {\"spec\": \"...\", \"section\": \"...\", \"text\": \"...\", \"files\": [\"...\"], \"fix\": \"...\"} ],
|
||||
\"structural\": [ ... ],
|
||||
\"deferred\": [ ... ],
|
||||
\"verdict\": \"all_clear\" | \"has_fixes\" | \"only_deferred\"
|
||||
}
|
||||
|
||||
If total_failing == 0 or verdict == \"only_deferred\", set preferred_next_label to \"Done\".
|
||||
Otherwise set preferred_next_label to \"Fix\"."
|
||||
]
|
||||
|
||||
/*------------------------------------------------------------------------
|
||||
* Phase 3: Fix — implement the highest-priority fixes
|
||||
*----------------------------------------------------------------------*/
|
||||
|
||||
fix_batch [
|
||||
label="Implement Fixes",
|
||||
shape=box,
|
||||
class="fix",
|
||||
goal_gate=true,
|
||||
prompt="The triage report identified IMPLEMENTABLE DoD failures. Pick the top 5 most impactful items (or all if fewer than 5) and implement the fixes in Rust.
|
||||
|
||||
For each fix:
|
||||
1. Read the relevant source file(s)
|
||||
2. Make the minimal change needed to satisfy the DoD checkbox
|
||||
3. Write the modified file(s)
|
||||
4. Verify the fix compiles (mentally trace includes and types)
|
||||
|
||||
Constraints:
|
||||
- Do NOT modify any files under docs/specs/ (those are the specs)
|
||||
- Do NOT add external dependencies beyond what's already used
|
||||
- Keep changes minimal and focused — one checkbox per fix
|
||||
- Maintain the existing code style
|
||||
|
||||
After implementing, output:
|
||||
{
|
||||
\"fixes_applied\": [ {\"spec\": \"...\", \"section\": \"...\", \"text\": \"...\", \"files_changed\": [\"...\"], \"description\": \"...\"} ],
|
||||
\"count\": N,
|
||||
\"remaining_implementable\": M
|
||||
}
|
||||
|
||||
If remaining_implementable > 0, set preferred_next_label to \"More fixes needed\".
|
||||
Otherwise set preferred_next_label to \"Re-audit\"."
|
||||
]
|
||||
|
||||
/*------------------------------------------------------------------------
|
||||
* Phase 4: Build verification
|
||||
*----------------------------------------------------------------------*/
|
||||
|
||||
build_check [
|
||||
label="Build & Smoke Test",
|
||||
shape=parallelogram,
|
||||
tool_command="cd /Users/bhelmkamp/p/brynary/attractor-rust && cargo build 2>&1 && echo '---BUILD OK---' && ./target/debug/attractor run --dry-run test/simple.dot 2>&1 && ./target/debug/attractor run --dry-run test/branching.dot 2>&1 && ./target/debug/attractor run --dry-run test/styled.dot 2>&1 && ./target/debug/attractor run --dry-run test/parallel.dot 2>&1 && ./target/debug/attractor run --dry-run test/conditions.dot 2>&1 && echo '---ALL TESTS PASSED---'",
|
||||
timeout="120s"
|
||||
]
|
||||
|
||||
/*------------------------------------------------------------------------
|
||||
* Phase 5: Build failure recovery
|
||||
*----------------------------------------------------------------------*/
|
||||
|
||||
build_fix [
|
||||
label="Fix Build Errors",
|
||||
shape=box,
|
||||
class="fix",
|
||||
prompt="The build or smoke tests failed. Read the build output from context (tool.output key). Diagnose the compilation errors or test failures and fix them.
|
||||
|
||||
Read the relevant source files, identify the issue, and write corrected versions. Common issues:
|
||||
- Missing includes
|
||||
- Type mismatches
|
||||
- Undeclared functions
|
||||
|
||||
Output the fixes applied and ensure the code will compile cleanly with: cargo build"
|
||||
]
|
||||
|
||||
/*------------------------------------------------------------------------
|
||||
* Phase 6: Final audit to confirm fixes worked
|
||||
*----------------------------------------------------------------------*/
|
||||
|
||||
final_audit [
|
||||
label="Final Verification Audit",
|
||||
shape=box,
|
||||
prompt="This is a verification pass. Re-read all three spec DoD sections:
|
||||
- docs/specs/unified-llm-spec.md Section 8
|
||||
- docs/specs/coding-agent-loop-spec.md Section 9
|
||||
- docs/specs/attractor-spec.md Section 11
|
||||
|
||||
And re-read the implementation files that were changed in this iteration.
|
||||
|
||||
Check ONLY the items that were previously failing. Have they been fixed?
|
||||
|
||||
Output:
|
||||
{
|
||||
\"verified_fixed\": [ {\"spec\": \"...\", \"section\": \"...\", \"text\": \"...\"} ],
|
||||
\"still_failing\": [ {\"spec\": \"...\", \"section\": \"...\", \"text\": \"...\", \"reason\": \"...\"} ],
|
||||
\"newly_broken\": [ ... ],
|
||||
\"remaining_total\": N
|
||||
}
|
||||
|
||||
If remaining_total == 0 (ignoring DEFERRED items), set preferred_next_label to \"Complete\".
|
||||
Otherwise set preferred_next_label to \"More work needed\"."
|
||||
]
|
||||
|
||||
/*------------------------------------------------------------------------
|
||||
* Phase 7: Human gate — confirm completion or push for another round
|
||||
*----------------------------------------------------------------------*/
|
||||
|
||||
review_gate [
|
||||
label="A) Accept & finish\nB) Push for another round",
|
||||
shape=hexagon
|
||||
]
|
||||
|
||||
/*------------------------------------------------------------------------
|
||||
* Edges
|
||||
*----------------------------------------------------------------------*/
|
||||
|
||||
start -> audit_llm
|
||||
|
||||
/* Sequential audit chain */
|
||||
audit_llm -> audit_agent
|
||||
audit_agent -> audit_attractor
|
||||
audit_attractor -> triage
|
||||
|
||||
/* Triage decision */
|
||||
triage -> exit [label="Done", condition="preferred_label=Done"]
|
||||
triage -> fix_batch [label="Fix", condition="preferred_label=Fix", weight=10]
|
||||
triage -> exit [label="Only deferred remain"]
|
||||
|
||||
/* Fix -> build check */
|
||||
fix_batch -> build_check
|
||||
|
||||
/* Build check outcomes */
|
||||
build_check -> final_audit [label="Build OK", condition="outcome=success"]
|
||||
build_check -> build_fix [label="Build failed", condition="outcome=fail"]
|
||||
|
||||
/* Build fix loops back to build check */
|
||||
build_fix -> build_check
|
||||
|
||||
/* Fix batch can loop for more fixes */
|
||||
fix_batch -> fix_batch [label="More fixes needed", condition="preferred_label=More fixes needed", loop_restart=true]
|
||||
|
||||
/* Final audit outcomes */
|
||||
final_audit -> review_gate [label="Complete", condition="preferred_label=Complete"]
|
||||
final_audit -> triage [label="More work needed", condition="preferred_label=More work needed"]
|
||||
|
||||
/* Human review gate */
|
||||
review_gate -> exit [label="A) Accept"]
|
||||
review_gate -> triage [label="B) Another round"]
|
||||
}
|
||||
Loading…
Add table
Reference in a new issue