Skip to content

Commit 0a68539

Browse files
committed
refactor(validation): add io module, improve validation flow and tests
- Add src/pb_spec/validation/io.py for reusable validation I/O helpers - Refactor build.py and plan.py validation logic - Expand test coverage in tests/test_validate.py - Update pyproject.toml and uv.lock for dependency changes - Update pb-plan/pb-improve skill docs and configs
1 parent 61fcbac commit 0a68539

12 files changed

Lines changed: 392 additions & 77 deletions

File tree

.rumdl.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
# Global configuration options
44
[global]
55
# List of rules to disable (uncomment and modify as needed)
6-
disable = ["MD013", "MD033", "MD041", "MD036"]
6+
disable = ["MD013", "MD033", "MD041", "MD036", "MD031", "MD040"]
77

88
# List of rules to enable exclusively (if provided, only these rules will run)
99
# enable = ["MD001", "MD003", "MD004"]

pyproject.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ build-backend = "uv_build"
44

55
[project]
66
name = "pb-spec"
7-
version = "1.1.2"
7+
version = "1.1.3"
88
description = "Plan-Build Spec (pb-spec): A CLI tool for managing AI coding assistant skills"
99
readme = "README.md"
1010
license = "Apache-2.0"

skills/pb-improve/SKILL.md

Lines changed: 40 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -326,19 +326,56 @@ Write ONE `tasks.md` with tasks from ALL findings numbered sequentially. **Tasks
326326
- RED step: run scenario, capture failing output
327327
- GREEN step: implement, run scenario, capture passing output
328328
- REFACTOR step: clean up, re-run to confirm
329-
- Each task includes: Context, Verification, Scenario Coverage, Loop Type (BDD+TDD or TDD-only), Behavioral Contract, Simplification Focus, Status (🔴 TODO), BDD Verification, Advanced Test Verification, Runtime Verification
330329
- Ordered: infrastructure/scaffolding first (Phase 1), then findings in priority order
331330
- Verification commands with expected results
332331

333332
**Cross-finding dependencies:** If Finding B depends on Finding A, place Finding A's tasks first and note the dependency in the task Context.
334333

335-
**Task Quality Checklist:**
336-
334+
> **⚠️ CRITICAL — Required Fields Contract (MUST follow for every task):**
335+
>
336+
> Every `### Task X.Y:` block MUST include ALL 10 of these fields, or pb-build will reject the spec:
337+
>
338+
> 1. `Context:` — why this task exists
339+
> 2. `Verification:` — how to verify the task is complete
340+
> 3. `Scenario Coverage:` — feature file + scenario name, or `N/A — [reason]`
341+
> 4. `Loop Type:``BDD+TDD` or `TDD-only`
342+
> 5. `Behavioral Contract:` — what behavior must be preserved
343+
> 6. **`Simplification Focus:`** — what to simplify, or `N/A — [reason]`
344+
> 7. `Status:``🔴 TODO`
345+
> 8. **`BDD Verification:`** — concrete scenario check command, or `N/A — [reason]`
346+
> 9. `Advanced Test Verification:` — command or `N/A — [reason]`
347+
> 10. `Runtime Verification:` — command or `N/A — [reason]`
348+
>
349+
> **Template for every task block:**
350+
>
351+
> ```markdown
352+
> ### Task X.Y: [Name]
353+
>
354+
> > **Context:** [why]
355+
> > **Verification:** [how]
356+
> > **Scenario Coverage:** [feature/scenario or N/A with reason]
357+
>
358+
> - **Loop Type:** `BDD+TDD` or `TDD-only`
359+
> - **Behavioral Contract:** `Preserve existing behavior` or `[change]`
360+
> - **Simplification Focus:** `[what]` or `N/A — [reason]`
361+
> - **Status:** 🔴 TODO
362+
> - [ ] Step 1: ...
363+
> - [ ] BDD Verification: [command or N/A — reason]
364+
> - [ ] Advanced Test Verification: [command or N/A — reason]
365+
> - [ ] Runtime Verification: [command or N/A — reason]
366+
> ```
367+
368+
**Task Quality Checklist (verify EVERY task before writing):**
369+
370+
- [ ] **ALL 10 required fields present** (Context, Verification, Scenario Coverage, Loop Type, Behavioral Contract, Simplification Focus, Status, BDD Verification, Advanced Test Verification, Runtime Verification)
371+
- [ ] **Simplification Focus** is filled in (not missing) — this is the #1 field pb-improve forgets
372+
- [ ] **BDD Verification** is filled in (not missing) — this is the #2 field pb-improve forgets
337373
- [ ] Each BDD+TDD task maps to ONE scenario
338374
- [ ] RED evidence is required (failing output captured)
339375
- [ ] GREEN evidence is required (passing output captured)
340376
- [ ] No task can be marked done without GREEN evidence
341377
- [ ] Tasks are ordered by dependency across findings, not just within findings
378+
- [ ] Run `uv run pb-spec validate --plan <spec-dir>` before finishing to catch missing fields
342379
343380
Finish by writing `specs/README.md` with the consolidated spec entry:
344381

skills/pb-improve/references/plan-template.md

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -169,10 +169,22 @@ For the human/agent who owns this code after the change lands:
169169

170170
## tasks.md Template
171171

172-
> **CRITICAL:** Every task block MUST include all 10 required fields or pb-build validation will fail.
173-
> Required fields: `Context:`, `Verification:`, `Scenario Coverage:`, `Loop Type:`,
174-
> `Behavioral Contract:`, `Simplification Focus:`, `Status:`, `BDD Verification:`,
175-
> `Advanced Test Verification:`, `Runtime Verification:`
172+
> **⚠️ CRITICAL — pb-build REJECTS tasks.md if ANY of these 10 fields is missing:**
173+
>
174+
> | # | Field | Common Mistake |
175+
> |---|-------|----------------|
176+
> | 1 | `Context:` ||
177+
> | 2 | `Verification:` ||
178+
> | 3 | `Scenario Coverage:` ||
179+
> | 4 | `Loop Type:` ||
180+
> | 5 | `Behavioral Contract:` ||
181+
> | 6 | **`Simplification Focus:`** | **#1 forgotten field** — always add it |
182+
> | 7 | `Status:` ||
183+
> | 8 | **`BDD Verification:`** | **#2 forgotten field** — always add it |
184+
> | 9 | `Advanced Test Verification:` ||
185+
> | 10 | `Runtime Verification:` ||
186+
>
187+
> **Before writing tasks.md, verify each task block has ALL 10 fields.**
176188
177189
```markdown
178190
# <Feature Name> — Tasks

skills/pb-plan/SKILL.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -285,7 +285,7 @@ Write a **compact** design doc to `specs/<spec-dir>/design.md`. Only include sec
285285

286286
> Map critical business flows from Given-When-Then to Mermaid state diagrams. Gherkin is essentially state transitions: `Given` (initial state) → `When` (triggering event) → `Then` (result state).
287287
288-
```mermaid
288+
```
289289
stateDiagram-v2
290290
[*] --> InitialState : Given [precondition]
291291
InitialState --> ActionState : When [action/event]

src/pb_spec/config.py

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
import logging
66
import os
7+
import threading
78
from dataclasses import dataclass
89

910
logger = logging.getLogger(__name__)
@@ -31,24 +32,29 @@ class TimeoutConfig:
3132

3233

3334
_timeout_config: TimeoutConfig | None = None
35+
_config_lock = threading.Lock()
3436

3537

3638
def get_timeout_config() -> TimeoutConfig:
3739
"""Get timeout configuration from environment variables with defaults.
3840
39-
Cached after first call. Call _reset_timeout_config_cache() to refresh.
41+
Cached after first call with thread-safe double-checked locking.
42+
Call _reset_timeout_config_cache() to refresh.
4043
"""
4144
global _timeout_config
4245
if _timeout_config is None:
43-
_timeout_config = TimeoutConfig(
44-
git_ls_files=_parse_int_env("PB_SPEC_GIT_TIMEOUT", 60),
45-
rumdl_check=_parse_int_env("PB_SPEC_RUMDL_CHECK_TIMEOUT", 10),
46-
rumdl_format=_parse_int_env("PB_SPEC_RUMDL_FORMAT_TIMEOUT", 30),
47-
)
46+
with _config_lock:
47+
if _timeout_config is None:
48+
_timeout_config = TimeoutConfig(
49+
git_ls_files=_parse_int_env("PB_SPEC_GIT_TIMEOUT", 60),
50+
rumdl_check=_parse_int_env("PB_SPEC_RUMDL_CHECK_TIMEOUT", 10),
51+
rumdl_format=_parse_int_env("PB_SPEC_RUMDL_FORMAT_TIMEOUT", 30),
52+
)
4853
return _timeout_config
4954

5055

5156
def _reset_timeout_config_cache() -> None:
5257
"""Reset the cached config. For testing only."""
5358
global _timeout_config
54-
_timeout_config = None
59+
with _config_lock:
60+
_timeout_config = None

src/pb_spec/validation/__init__.py

Lines changed: 5 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
from __future__ import annotations
44

55
from pb_spec.validation.build import validate_build, validate_task
6+
from pb_spec.validation.io import read_file_content, read_spec_file
67
from pb_spec.validation.parser import (
78
ALLOWED_LOOP_TYPES,
89
ALLOWED_TASK_STATUSES,
@@ -21,11 +22,7 @@
2122
validate_contract_blocks,
2223
)
2324
from pb_spec.validation.plan import (
24-
_CONTRACT_CONFIG,
25-
_FULL_MODE_REQUIRED,
26-
_LIGHTWEIGHT_MODE_REQUIRED,
27-
_NA_REASON_FIELDS,
28-
_TASK_REQUIRED_FIELDS,
25+
_load_contract_config,
2926
validate_plan,
3027
)
3128
from pb_spec.validation.result import (
@@ -54,12 +51,9 @@
5451
"ValidationError",
5552
"ValidationMode",
5653
"ValidationResult",
57-
"_CONTRACT_CONFIG",
58-
"_FULL_MODE_REQUIRED",
59-
"_LIGHTWEIGHT_MODE_REQUIRED",
60-
"_NA_REASON_FIELDS",
61-
"_TASK_REQUIRED_FIELDS",
6254
"make_validation_error",
55+
"read_file_content",
56+
"read_spec_file",
6357
"validate_build",
6458
"validate_plan",
6559
"validate_task",
@@ -70,4 +64,5 @@
7064
"required_sections_for_contract_block",
7165
"task_display_name",
7266
"validate_contract_blocks",
67+
"_load_contract_config",
7368
]

src/pb_spec/validation/build.py

Lines changed: 23 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -7,13 +7,14 @@
77

88
from pb_spec.exceptions import FileReadError
99
from pb_spec.git_utils import get_git_modified_files
10+
from pb_spec.validation.io import read_file_content, read_spec_file
1011
from pb_spec.validation.parser import (
1112
TASK_CHECKBOX_RE,
1213
UNCHECKED_TASK_CHECKBOX_RE,
14+
TaskBlock,
1315
parse_task_blocks,
1416
task_display_name,
1517
)
16-
from pb_spec.validation.plan import read_file_content, read_spec_file
1718
from pb_spec.validation.result import (
1819
ErrorSeverity,
1920
ValidationError,
@@ -51,23 +52,24 @@ def validate_feature_scenarios(spec_dir: Path) -> list[ValidationError]:
5152
return errors
5253

5354

54-
def run_codebase_scan(mode: ValidationMode) -> ScanResult:
55+
def run_codebase_scan(mode: ValidationMode, root_dir: Path | str = ".") -> ScanResult:
5556
"""Scan codebase for code quality issues.
5657
5758
Args:
5859
mode: ValidationMode.TASK for subagent self-check (scoped to git-modified
5960
files), ValidationMode.BUILD for full scan.
61+
root_dir: Root directory to scan. Defaults to current working directory.
6062
6163
Returns:
6264
Pure ScanResult without side effects. Callers are responsible
6365
for presenting the results.
6466
"""
6567
target_files: set[Path] | None = None
6668
if mode == ValidationMode.TASK:
67-
target_files = get_git_modified_files()
69+
target_files = get_git_modified_files(root_dir)
6870

6971
scanner = CodeScanner(
70-
root_dir=".",
72+
root_dir=root_dir,
7173
check_skipped_tests=True,
7274
check_not_implemented=True,
7375
check_todos=True,
@@ -99,7 +101,7 @@ def _scan_result_to_errors(scan_result: ScanResult) -> list[ValidationError]:
99101
return errors
100102

101103

102-
def _validate_task_completion(task_blocks: list, content: str) -> list[ValidationError]:
104+
def _validate_task_completion(task_blocks: list[TaskBlock], content: str) -> list[ValidationError]:
103105
"""Validate that all tasks are marked DONE with complete steps.
104106
105107
Args:
@@ -176,7 +178,7 @@ def _validate_task_completion(task_blocks: list, content: str) -> list[Validatio
176178
return errors
177179

178180

179-
def _validate_task_completion_warnings(task_blocks: list) -> list[str]:
181+
def _validate_task_completion_warnings(task_blocks: list[TaskBlock]) -> list[str]:
180182
"""Collect warnings for non-DONE tasks (SKIPPED, OBSOLETE).
181183
182184
Args:
@@ -193,9 +195,9 @@ def _validate_task_completion_warnings(task_blocks: list) -> list[str]:
193195
return warnings
194196

195197

196-
def _validate_codebase_scan() -> list[ValidationError]:
198+
def _validate_codebase_scan(root_dir: Path | str = ".") -> list[ValidationError]:
197199
"""Run codebase scan and return errors."""
198-
scan_result = run_codebase_scan(mode=ValidationMode.BUILD)
200+
scan_result = run_codebase_scan(mode=ValidationMode.BUILD, root_dir=root_dir)
199201
if not scan_result.has_issues:
200202
return []
201203

@@ -231,7 +233,6 @@ def validate_build(spec_dir: Path) -> ValidationResult:
231233
],
232234
)
233235

234-
tasks_file = spec_dir / "tasks.md"
235236
content, error_result = read_spec_file(tasks_file)
236237
if error_result is not None:
237238
return error_result
@@ -250,19 +251,27 @@ def validate_build(spec_dir: Path) -> ValidationResult:
250251
task_blocks = parse_task_blocks(content)
251252
warnings.extend(_validate_task_completion_warnings(task_blocks))
252253
errors.extend(_validate_task_completion(task_blocks, content))
253-
errors.extend(_validate_codebase_scan())
254+
255+
# Determine project root: spec_dir is typically specs/xxx, so root is two levels up
256+
project_root = spec_dir.parent.parent if spec_dir.parent.name == "specs" else spec_dir.parent
257+
errors.extend(_validate_codebase_scan(root_dir=project_root))
258+
254259
errors.extend(validate_feature_scenarios(spec_dir))
255260

256261
return ValidationResult(is_valid=len(errors) == 0, errors=errors, warnings=warnings)
257262

258263

259-
def validate_task() -> ValidationResult:
264+
def validate_task(root_dir: Path | str = ".") -> ValidationResult:
260265
"""Subagent self-check before signaling READY_FOR_EVAL.
261266
262-
Returns a pure ValidationResult without side effects.
263-
Callers are responsible for presenting the results.
267+
Args:
268+
root_dir: Root directory to scan. Defaults to current working directory.
269+
270+
Returns:
271+
A pure ValidationResult without side effects.
272+
Callers are responsible for presenting the results.
264273
"""
265-
scan_result = run_codebase_scan(mode=ValidationMode.TASK)
274+
scan_result = run_codebase_scan(mode=ValidationMode.TASK, root_dir=root_dir)
266275
if not scan_result.has_issues:
267276
return ValidationResult(is_valid=True)
268277

src/pb_spec/validation/io.py

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
"""File I/O utilities for validation module."""
2+
3+
from __future__ import annotations
4+
5+
from pathlib import Path
6+
7+
from pb_spec.exceptions import FileReadError
8+
from pb_spec.validation.result import (
9+
ErrorSeverity,
10+
ValidationResult,
11+
make_validation_error,
12+
)
13+
14+
15+
def read_file_content(file_path: Path) -> str:
16+
"""Read file content with error handling.
17+
18+
Raises:
19+
FileReadError: If the file cannot be read.
20+
"""
21+
try:
22+
return file_path.read_text(encoding="utf-8")
23+
except (OSError, UnicodeDecodeError) as e:
24+
raise FileReadError(f"Cannot read file {file_path}: {e}") from e
25+
26+
27+
def read_spec_file(file_path: Path) -> tuple[str, None] | tuple[None, ValidationResult]:
28+
"""Read a spec file, returning (content, None) on success or (None, error_result) on failure."""
29+
try:
30+
return read_file_content(file_path), None
31+
except FileReadError as e:
32+
return None, ValidationResult(
33+
is_valid=False,
34+
errors=[
35+
make_validation_error(
36+
message=str(e),
37+
file_path=str(file_path),
38+
severity=ErrorSeverity.CRITICAL,
39+
)
40+
],
41+
)

0 commit comments

Comments
 (0)