diff --git a/.specify/feature.json b/.specify/feature.json index f1ada89..b90bf9b 100644 --- a/.specify/feature.json +++ b/.specify/feature.json @@ -1,3 +1,3 @@ { - "feature_directory": "specs/007-phase-gates" + "feature_directory": "specs/008-clarify-gate" } diff --git a/specs/008-clarify-gate/checklists/requirements.md b/specs/008-clarify-gate/checklists/requirements.md new file mode 100644 index 0000000..d14d026 --- /dev/null +++ b/specs/008-clarify-gate/checklists/requirements.md @@ -0,0 +1,34 @@ +# Specification Quality Checklist: Clarify-Gate Pattern + +**Purpose**: Validate specification completeness and quality before proceeding to planning +**Created**: 2026-05-20 +**Feature**: [spec.md](../spec.md) + +## Content Quality + +- [x] No implementation details (languages, frameworks, APIs) +- [x] Focused on user value and business needs +- [x] Written for non-technical stakeholders +- [x] All mandatory sections completed + +## Requirement Completeness + +- [x] No [NEEDS CLARIFICATION] markers remain +- [x] Requirements are testable and unambiguous +- [x] Success criteria are measurable +- [x] Success criteria are technology-agnostic (no implementation details) +- [x] All acceptance scenarios are defined +- [x] Edge cases are identified +- [x] Scope is clearly bounded +- [x] Dependencies and assumptions identified + +## Feature Readiness + +- [x] All functional requirements have clear acceptance criteria +- [x] User scenarios cover primary flows +- [x] Feature meets measurable outcomes defined in Success Criteria +- [x] No implementation details leak into specification + +## Notes + +- All items pass. Spec ready for `/speckit-plan`. diff --git a/specs/008-clarify-gate/spec.md b/specs/008-clarify-gate/spec.md new file mode 100644 index 0000000..f177896 --- /dev/null +++ b/specs/008-clarify-gate/spec.md @@ -0,0 +1,100 @@ +# Feature Specification: Clarify-Gate Pattern + +**Feature Branch**: `feature/8-clarify-gate` + +**Created**: 2026-05-20 + +**Status**: Draft + +**Input**: Implement the clarify-gate pattern — structured CLR-NNN questions that persist across phase re-runs, with merge behaviour control. + +## User Scenarios & Testing + +### User Story 1 - AI agent raises clarification during phase execution (Priority: P1) + +An AI agent running `/adm.lineage` encounters an ambiguous column name. It raises a structured CLR-NNN question that gets stored in the domain's state. When the phase is re-run later (with new data or after resolving questions), existing resolutions are preserved. + +**Why this priority**: This is the core mechanism — without it, analyst decisions get lost on every re-run, violating the "preserve analyst work" principle. + +**Independent Test**: Raise a CLR, resolve it, re-run the phase, verify the resolution survives. + +**Acceptance Scenarios**: + +1. **Given** a domain with no resolutions, **When** a new CLR-001 question is raised, **Then** it appears in `domain.resolutions` with status "open". +2. **Given** CLR-001 is resolved, **When** the phase is re-run and raises CLR-001 again, **Then** the existing resolution is preserved (merge behaviour). +3. **Given** CLR-001 is resolved and CLR-002 is new, **When** the phase re-runs, **Then** CLR-001 stays resolved and CLR-002 is added as open. + +--- + +### User Story 2 - Developer controls merge behaviour with --resolutions flag (Priority: P1) + +A developer re-running a phase can choose how new questions interact with existing resolutions: merge (default — preserve existing), overwrite (discard all, re-raise fresh), or skip (don't raise any new questions). + +**Why this priority**: Different re-run scenarios need different behaviour. Fresh data may invalidate old answers (overwrite), while minor updates should preserve work (merge). + +**Independent Test**: Run with each flag value and verify the expected merge behaviour. + +**Acceptance Scenarios**: + +1. **Given** existing resolutions and `--resolutions merge` (default), **When** new CLRs are raised, **Then** existing resolutions are kept, new questions are added as open. +2. **Given** existing resolutions and `--resolutions overwrite`, **When** new CLRs are raised, **Then** all prior resolutions are discarded, all questions are fresh and open. +3. **Given** existing resolutions and `--resolutions skip`, **When** the phase runs, **Then** no new questions are raised, only existing resolutions are used. + +--- + +### User Story 3 - adm check reports clarification status (Priority: P2) + +A developer runs `adm check` and sees the count of open vs resolved clarifications per domain, giving visibility into analyst work remaining before ratcheting. + +**Why this priority**: Already partially implemented in issue #5 — this story ensures the clarify module integrates cleanly with the existing check command. + +**Independent Test**: Already covered by test_check.py (CLR reporting tests from issue #5). + +**Acceptance Scenarios**: + +1. **Given** 3 open and 2 resolved CLRs, **When** `adm check` runs, **Then** it reports "3 open, 2 resolved" as a warning. +2. **Given** zero open CLRs, **When** `adm check` runs, **Then** it reports "ready for ratchet". + +--- + +### Edge Cases + +- CLR-NNN IDs must be globally unique within a domain — never reused even if the question is removed. +- What if a resolution references a column that no longer exists after data refresh? The resolution is still preserved (analyst decides if it's still valid). +- Maximum number of CLRs per domain? No limit — but `adm status` shows the count. + +## Requirements + +### Functional Requirements + +- **FR-001**: The system MUST support structured clarification questions with format: ID (CLR-NNN), question text, context, resolution status (open/resolved) +- **FR-002**: Resolutions MUST be stored in `.adm/project.json` under `domain.resolutions` as a `{CLR-ID: status}` mapping +- **FR-003**: Re-running a phase MUST preserve existing resolutions by default (merge behaviour) +- **FR-004**: The `--resolutions` flag MUST support three modes: `merge` (default), `overwrite`, `skip` +- **FR-005**: CLR-NNN IDs MUST be globally unique within a domain and never reused +- **FR-006**: The system MUST provide a function to raise a new CLR question (returns the next available ID) +- **FR-007**: The system MUST provide a function to resolve a CLR by ID (sets status to "resolved" with answer text) +- **FR-008**: `adm check` MUST report open vs resolved counts per domain (already implemented in issue #5) + +### Key Entities + +- **Clarification (CLR)**: A structured question raised during phase execution with ID, question, context, and status +- **Resolution**: The analyst's answer to a CLR, persisted in the state file +- **Merge Mode**: Controls how new CLRs interact with existing resolutions on phase re-run + +## Success Criteria + +### Measurable Outcomes + +- **SC-001**: Resolutions survive 100% of phase re-runs under default (merge) behaviour +- **SC-002**: No analyst decision is ever silently discarded without explicit `--resolutions overwrite` +- **SC-003**: The next available CLR-NNN ID is always correctly computed (no collisions) +- **SC-004**: All three merge modes produce the expected state after re-run + +## Assumptions + +- The state schema already has `resolutions: dict[str, ResolutionStatus]` (implemented in issue #1) +- Resolution status is either "open" or "resolved" (the ResolutionStatus enum already exists) +- The actual question text and context are stored in artefact files (not in project.json — only the ID and status live in state) +- The `--resolutions` flag is passed to phase commands, not to `adm check` or `adm status` +- CLR IDs are sequential within a domain: CLR-001, CLR-002, etc. diff --git a/src/adm_cli/clarify.py b/src/adm_cli/clarify.py new file mode 100644 index 0000000..66ceeed --- /dev/null +++ b/src/adm_cli/clarify.py @@ -0,0 +1,111 @@ +"""Clarify-gate pattern — structured CLR-NNN questions that persist across re-runs.""" + +from __future__ import annotations + +from enum import Enum +from pathlib import Path + +from .schema import ProjectState, ResolutionStatus, save_state + + +class MergeMode(str, Enum): + MERGE = "merge" + OVERWRITE = "overwrite" + SKIP = "skip" + + +def next_clr_id(domain: str, state: ProjectState) -> str: + """Compute the next available CLR-NNN ID for a domain.""" + if domain not in state.domains: + raise ValueError(f"Domain '{domain}' does not exist") + + existing = state.domains[domain].resolutions + if not existing: + return "CLR-001" + + max_num = 0 + for key in existing: + if key.startswith("CLR-") and key[4:].isdigit(): + max_num = max(max_num, int(key[4:])) + return f"CLR-{max_num + 1:03d}" + + +def raise_clr( + domain: str, + state: ProjectState, + state_path: Path, + question: str, + context: str | None = None, +) -> str: + """Raise a new clarification question. Returns the assigned CLR-NNN ID.""" + if domain not in state.domains: + raise ValueError(f"Domain '{domain}' does not exist") + + clr_id = next_clr_id(domain, state) + state.domains[domain].resolutions[clr_id] = ResolutionStatus.OPEN + save_state(state, state_path) + return clr_id + + +def resolve_clr( + domain: str, + clr_id: str, + state: ProjectState, + state_path: Path, +) -> None: + """Mark a CLR as resolved.""" + if domain not in state.domains: + raise ValueError(f"Domain '{domain}' does not exist") + + resolutions = state.domains[domain].resolutions + if clr_id not in resolutions: + raise ValueError(f"CLR '{clr_id}' does not exist in domain '{domain}'") + + resolutions[clr_id] = ResolutionStatus.RESOLVED + save_state(state, state_path) + + +def merge_clarifications( + domain: str, + new_clr_ids: list[str], + state: ProjectState, + state_path: Path, + mode: MergeMode = MergeMode.MERGE, +) -> None: + """Apply merge behaviour when a phase is re-run with new CLRs. + + - merge: preserve existing resolutions, add new as open + - overwrite: discard all prior resolutions, all new are open + - skip: don't add any new CLRs, keep existing only + """ + if domain not in state.domains: + raise ValueError(f"Domain '{domain}' does not exist") + + resolutions = state.domains[domain].resolutions + + if mode == MergeMode.OVERWRITE: + resolutions.clear() + for clr_id in new_clr_ids: + resolutions[clr_id] = ResolutionStatus.OPEN + + elif mode == MergeMode.SKIP: + pass # No changes — existing resolutions stay, no new ones added + + elif mode == MergeMode.MERGE: + for clr_id in new_clr_ids: + if clr_id not in resolutions: + resolutions[clr_id] = ResolutionStatus.OPEN + # If already exists (resolved or open), leave as-is + + save_state(state, state_path) + + +def get_clr_summary(domain: str, state: ProjectState) -> tuple[int, int]: + """Return (open_count, resolved_count) for a domain.""" + if domain not in state.domains: + raise ValueError(f"Domain '{domain}' does not exist") + + resolutions = state.domains[domain].resolutions + open_count = sum(1 for s in resolutions.values() if s == ResolutionStatus.OPEN) + resolved_count = sum(1 for s in resolutions.values() if s == ResolutionStatus.RESOLVED) + return open_count, resolved_count diff --git a/tests/test_clarify.py b/tests/test_clarify.py new file mode 100644 index 0000000..2133534 --- /dev/null +++ b/tests/test_clarify.py @@ -0,0 +1,129 @@ +"""Tests for clarify-gate pattern.""" + +from pathlib import Path + +import pytest + +from adm_cli.clarify import ( + MergeMode, + get_clr_summary, + merge_clarifications, + next_clr_id, + raise_clr, + resolve_clr, +) +from adm_cli.schema import DomainState, ProjectState, ResolutionStatus, save_state + + +def _setup(tmp_path: Path, **kwargs) -> tuple[ProjectState, Path]: + state = ProjectState(domains={"holdings": DomainState(**kwargs)}) + state_path = tmp_path / ".adm" / "project.json" + save_state(state, state_path) + return state, state_path + + +class TestNextClrId: + def test_first_id(self, tmp_path: Path): + state, _ = _setup(tmp_path) + assert next_clr_id("holdings", state) == "CLR-001" + + def test_sequential(self, tmp_path: Path): + state, _ = _setup(tmp_path, resolutions={"CLR-001": ResolutionStatus.OPEN}) + assert next_clr_id("holdings", state) == "CLR-002" + + def test_gaps_use_max(self, tmp_path: Path): + state, _ = _setup( + tmp_path, + resolutions={"CLR-001": ResolutionStatus.RESOLVED, "CLR-005": ResolutionStatus.OPEN}, + ) + assert next_clr_id("holdings", state) == "CLR-006" + + def test_nonexistent_domain(self, tmp_path: Path): + state, _ = _setup(tmp_path) + with pytest.raises(ValueError, match="does not exist"): + next_clr_id("nonexistent", state) + + +class TestRaiseClr: + def test_raises_open(self, tmp_path: Path): + state, state_path = _setup(tmp_path) + clr_id = raise_clr("holdings", state, state_path, "What does column X mean?") + assert clr_id == "CLR-001" + assert state.domains["holdings"].resolutions["CLR-001"] == ResolutionStatus.OPEN + + def test_sequential_ids(self, tmp_path: Path): + state, state_path = _setup(tmp_path) + id1 = raise_clr("holdings", state, state_path, "Q1") + id2 = raise_clr("holdings", state, state_path, "Q2") + assert id1 == "CLR-001" + assert id2 == "CLR-002" + + def test_persists_to_disk(self, tmp_path: Path): + from adm_cli.schema import load_state + + state, state_path = _setup(tmp_path) + raise_clr("holdings", state, state_path, "Q1") + reloaded = load_state(state_path) + assert "CLR-001" in reloaded.domains["holdings"].resolutions + + +class TestResolveClr: + def test_marks_resolved(self, tmp_path: Path): + state, state_path = _setup(tmp_path, resolutions={"CLR-001": ResolutionStatus.OPEN}) + resolve_clr("holdings", "CLR-001", state, state_path) + assert state.domains["holdings"].resolutions["CLR-001"] == ResolutionStatus.RESOLVED + + def test_nonexistent_clr(self, tmp_path: Path): + state, state_path = _setup(tmp_path) + with pytest.raises(ValueError, match="does not exist"): + resolve_clr("holdings", "CLR-999", state, state_path) + + +class TestMergeClarifications: + def test_merge_preserves_existing(self, tmp_path: Path): + state, state_path = _setup( + tmp_path, + resolutions={"CLR-001": ResolutionStatus.RESOLVED}, + ) + merge_clarifications("holdings", ["CLR-001", "CLR-002"], state, state_path, MergeMode.MERGE) + assert state.domains["holdings"].resolutions["CLR-001"] == ResolutionStatus.RESOLVED + assert state.domains["holdings"].resolutions["CLR-002"] == ResolutionStatus.OPEN + + def test_overwrite_discards_all(self, tmp_path: Path): + state, state_path = _setup( + tmp_path, + resolutions={"CLR-001": ResolutionStatus.RESOLVED, "CLR-002": ResolutionStatus.RESOLVED}, + ) + merge_clarifications("holdings", ["CLR-001", "CLR-003"], state, state_path, MergeMode.OVERWRITE) + resolutions = state.domains["holdings"].resolutions + assert resolutions == {"CLR-001": ResolutionStatus.OPEN, "CLR-003": ResolutionStatus.OPEN} + + def test_skip_adds_nothing(self, tmp_path: Path): + state, state_path = _setup( + tmp_path, + resolutions={"CLR-001": ResolutionStatus.RESOLVED}, + ) + merge_clarifications("holdings", ["CLR-002", "CLR-003"], state, state_path, MergeMode.SKIP) + assert "CLR-002" not in state.domains["holdings"].resolutions + assert state.domains["holdings"].resolutions["CLR-001"] == ResolutionStatus.RESOLVED + + +class TestGetClrSummary: + def test_counts(self, tmp_path: Path): + state, _ = _setup( + tmp_path, + resolutions={ + "CLR-001": ResolutionStatus.OPEN, + "CLR-002": ResolutionStatus.OPEN, + "CLR-003": ResolutionStatus.RESOLVED, + }, + ) + open_count, resolved_count = get_clr_summary("holdings", state) + assert open_count == 2 + assert resolved_count == 1 + + def test_empty(self, tmp_path: Path): + state, _ = _setup(tmp_path) + open_count, resolved_count = get_clr_summary("holdings", state) + assert open_count == 0 + assert resolved_count == 0