Double Diamond: data model scaffold + spec doc (#744)#752
Conversation
Phase 1 of the Double Diamond architecture: adds the domain types (Spec, SpecStatus, Complexity, TaskKind) and a full spec document, without wiring anything into dispatch yet. Existing behavior is unchanged — TaskKind defaults to Implement. Follow-on phases (Scout dispatch, Spec Queue, Builder slot, feedback loops) are described in spec/double-diamond.md §10.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR is clean — 84 tests green, build and clippy pass with no new warnings. The type surface matches the spec exactly and the test coverage (serde round-trips, status helpers, timestamp update) is solid for a scaffold phase.
One important finding and two suggestions noted inline. Summary below.
[IMPORTANT] set_status docstring vs. Task::set_state behaviour mismatch
set_status says it "mirrors the soft-validation pattern used by Task::set_state", but Task::set_state calls can_transition_to() and fires tracing::warn!; set_status does neither. For Phase 1 types-only this is harmless, but the inaccurate doc creates a false expectation for Phase 2 authors. Either adopt the warn pattern or fix the docstring — see the inline comment.
[SUGGESTION] thread::sleep in spec_set_status_updates_timestamp
The existing task tests explicitly note that Utc::now() has sub-microsecond resolution and no sleep is needed. This test is the only outlier. See inline comment.
[SUGGESTION] TaskKind is not yet a field on Task
The PR description says "TaskKind defaults to Implement" but Task has no kind field. The approach (types now, migration + struct field later) is coherent — you don't want a struct field that doesn't persist — but the phrasing is misleading. A note like "TaskKind is defined and ready; it will be added to Task when the migration lands in Phase 1b" would be clearer.
[SUGGESTION, Phase 3 heads-up] WorkType::Builder insertion will need care
The spec (§6.1) places Builder between MergeConflict and PrFeedback. WorkType derives Ord from declaration order, so insertion is fine at the Rust level. The risk is if any stored data encodes WorkType as an integer ordinal rather than as a string — the store currently uses as_str() / from_str(), so this looks safe, but the Phase 3 PR should verify nothing assumes a stable integer mapping.
[SUGGESTION] spec.md is not cross-referenced
double-diamond.md says it "supersedes the current 'N parallel implementers' model documented in spec.md §5 and §10", but spec.md itself is unchanged and still describes the old model. Someone reading spec.md alone gets an outdated picture. A one-line forward-reference in spec.md ("See spec/double-diamond.md for the Double Diamond execution model") would close the gap without requiring a full rewrite before the architecture ships.
References:
Reviewed by PR / Review
| /// Transition to a new status, updating `updated_at`. | ||
| /// | ||
| /// Does not enforce transitions — orchestrator logic does. This mirrors the | ||
| /// soft-validation pattern used by `Task::set_state`. |
There was a problem hiding this comment.
[IMPORTANT] Priority: Correctness
The docstring claims this "mirrors the soft-validation pattern used by Task::set_state", but Task::set_state actually calls can_transition_to() and emits tracing::warn! on invalid transitions. set_status does neither — it silently accepts any status regardless of whether the transition is legal.
This isn't a problem today (Phase 1 types only), but the docstring creates a false expectation. When Phase 2 code calls set_status, authors may assume the guard is in place and omit their own validation.
Two options:
Option A — add the same soft-validation pattern:
pub fn set_status(&mut self, status: SpecStatus) {
// (future: add can_transition_to check and tracing::warn! here, matching Task::set_state)
self.status = status;
self.updated_at = Utc::now();
}Option B — update the docstring to be accurate:
/// Transition to a new status, updating `updated_at`.
/// No transition enforcement at this layer — the orchestrator is responsible.
Either is fine; the claim needs to match the implementation.
| fn spec_set_status_updates_timestamp() { | ||
| let mut s = Spec::new("s", "t", "sc", "c", Complexity::Simple); | ||
| let before = s.updated_at; | ||
| std::thread::sleep(std::time::Duration::from_millis(2)); |
There was a problem hiding this comment.
[SUGGESTION] Priority: Code Quality
thread::sleep is unnecessary here. The task model tests explicitly note this: // Small delay not needed — Utc::now() has sub-microsecond resolution. chrono::Utc::now() advances within the same execution; s.updated_at (set in set_status) will be >= before (set in Spec::new()) even without the sleep. Using > (strictly greater) is what you want, and the two Utc::now() calls will differ at sub-microsecond granularity.
fn spec_set_status_updates_timestamp() {
let mut s = Spec::new("s", "t", "sc", "c", Complexity::Simple);
let before = s.updated_at;
// No sleep needed — Utc::now() has sub-microsecond resolution
s.set_status(SpecStatus::Approved);
assert_eq!(s.status, SpecStatus::Approved);
assert!(s.updated_at > before);
}The 2ms sleep accumulates when running the test suite. Drop it to stay consistent with the established pattern.
1 participant
Draft PR while @iamnbutler reviews the design. Refs #744.
Summary
spec/double-diamond.md— full spec document for the Double Diamond architecture, with concrete data model, phased rollout (4 phases), and proposed resolutions for every RFC open question.crates/models/src/spec.rs— Phase 1 types only:Spec,SpecStatus,Complexity,TaskKind. 17 new unit tests.crates/models/src/lib.rs— module registration and re-exports.No dispatch, prompt, storage, or API changes.
TaskKinddefaults toImplement, so existing tasks are unaffected.Key design choices (see issue comment on #744 for full reasoning)
Taskrow with aTaskKinddiscriminator, not three tables — reuses existing state machine, retry, dispatch, and accounting.fastpathlabel.Test plan
cargo test -p models --lib— 84 tests pass (17 new)cargo build --workspace— clean buildspec/double-diamond.md§10