feat(validation): Add tasks.md validation with checkbox and content checks (#354) #576
Conversation
📝 WalkthroughWalkthroughAdds validation for Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "CLI (openspec validate)"
participant Validator as "Validator"
participant FS as "Filesystem"
participant Reporter as "Report Merger"
CLI->>Validator: request validate change <id>
Validator->>FS: read change delta/spec files
Validator->>FS: read `tasks.md`
alt tasks.md present
Validator->>Validator: validateTasksContent()
else tasks.md missing
Validator->>Validator: emit missing-file issue
end
Validator->>Reporter: send spec validation report
Validator->>Reporter: send tasks validation report
Reporter->>CLI: merged validation summary & issues
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile OverviewGreptile SummaryThis PR adds comprehensive validation for Key Changes:
Validation Rules Enforced:
Code Quality:
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant CLI as ValidateCommand
participant Val as Validator
participant FS as FileSystem
User->>CLI: openspec validate <change-id>
CLI->>CLI: validateByType('change', id)
CLI->>CLI: validateChangeReports(changeDir, validator)
par Parallel Validation
CLI->>Val: validateChangeDeltaSpecs(changeDir)
Val->>FS: Read spec delta files
FS-->>Val: Delta content
Val-->>CLI: Delta report
and
CLI->>Val: validateTasksFile(changeDir)
Val->>FS: readFile(tasks.md)
alt File exists
FS-->>Val: tasks.md content
Val->>Val: validateTasksContent(content)
Val->>Val: Check TASK_CHECKBOX_PATTERN
Val->>Val: Check EMPTY_TASK_PATTERN
Val-->>CLI: Tasks report (valid/errors)
else File missing
FS-->>Val: ENOENT error
Val-->>CLI: Tasks report (ERROR: required)
end
end
CLI->>CLI: Combine reports
CLI->>User: Display validation results
CLI->>User: Set exit code (0 or 1)
|
Review CompleteYour review story is ready! Comment !reviewfast on this PR to re-generate the story. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@openspec/specs/cli-validate/spec.md`:
- Around line 231-233: Update the spec's regex to match the implementation's
exported TASK_PATTERN used in task-progress.ts: change `/^[-*]\s+\[[xX\s]\]/` to
`/^[-*]\s+\[[\sx]\]/i` so the documented pattern and the TASK_PATTERN constant
(and its case-insensitive behavior) are identical.
In `@src/core/validation/validator.ts`:
- Around line 305-334: The checkbox regexes in validateTasksContent do not allow
leading whitespace so nested tasks like " - [ ] Subtask" are missed; update the
TASK_CHECKBOX_PATTERN and EMPTY_TASK_PATTERN to accept optional leading
whitespace (e.g., prepend \s* to the existing patterns) so validateTasksContent
correctly detects and validates indented checkbox lines (refer to
TASK_CHECKBOX_PATTERN, EMPTY_TASK_PATTERN and the validateTasksContent method).
🧹 Nitpick comments (1)
test/commands/validate.test.ts (1)
138-161: Good test coverage for missing tasks.md scenario.The test properly validates the error condition. Consider adding integration tests for other spec scenarios (empty task descriptions, no checkboxed tasks) for comprehensive coverage, though unit tests in
validation.test.tsmay already cover these.
| private validateTasksContent(content: string): ValidationIssue[] { | ||
| const issues: ValidationIssue[] = []; | ||
| const lines = content.split(/\r?\n/); | ||
| let hasCheckbox = false; | ||
|
|
||
| lines.forEach((line, index) => { | ||
| if (TASK_CHECKBOX_PATTERN.test(line)) { | ||
| hasCheckbox = true; | ||
| if (EMPTY_TASK_PATTERN.test(line)) { | ||
| const lineNumber = index + 1; | ||
| issues.push({ | ||
| level: 'ERROR', | ||
| path: `tasks.md:${lineNumber}`, | ||
| message: 'Empty task description', | ||
| line: lineNumber, | ||
| }); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| if (!hasCheckbox) { | ||
| issues.push({ | ||
| level: 'ERROR', | ||
| path: 'tasks.md', | ||
| message: 'tasks.md must contain at least one checkboxed task', | ||
| }); | ||
| } | ||
|
|
||
| return issues; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's find where TASK_CHECKBOX_PATTERN and EMPTY_TASK_PATTERN are defined
rg -n 'TASK_CHECKBOX_PATTERN|EMPTY_TASK_PATTERN' src/core/validation/validator.ts -A 1 -B 1Repository: Fission-AI/OpenSpec
Length of output: 392
🏁 Script executed:
#!/bin/bash
# Check if any tasks.md files exist and contain nested checkboxes
find . -name 'tasks.md' -type f | head -20Repository: Fission-AI/OpenSpec
Length of output: 1443
🏁 Script executed:
#!/bin/bash
# Search for nested checkbox patterns in the codebase
rg -n '^\s+[-*]\s+\[' --glob '*.md' | head -50Repository: Fission-AI/OpenSpec
Length of output: 7348
🏁 Script executed:
#!/bin/bash
# Look for test files related to validator
fd -e test.ts -e spec.ts -e test.js validatorRepository: Fission-AI/OpenSpec
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Find TASK_PATTERN definition
rg -n 'const TASK_PATTERN' src/core/validation/validator.ts -A 1 -B 1Repository: Fission-AI/OpenSpec
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Search for test files related to validator in the codebase
fd -type f -name '*validator*' | grep -E '\.(test|spec)\.'Repository: Fission-AI/OpenSpec
Length of output: 233
🏁 Script executed:
#!/bin/bash
# Check if there are any tests that validate nested tasks
rg -l 'validateTasksContent|TASK_CHECKBOX_PATTERN|nested.*task' --glob '*.test.ts' --glob '*.spec.ts'Repository: Fission-AI/OpenSpec
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Get more context around TASK_PATTERN definition in validator.ts
head -25 src/core/validation/validator.tsRepository: Fission-AI/OpenSpec
Length of output: 1020
🏁 Script executed:
#!/bin/bash
# Check test file for nested task test cases
cat test/core/validation.test.tsRepository: Fission-AI/OpenSpec
Length of output: 17346
🏁 Script executed:
#!/bin/bash
# Find the definition of TASK_PATTERN in task-progress.js
find . -name 'task-progress.js' -o -name 'task-progress.ts' | head -5Repository: Fission-AI/OpenSpec
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Get TASK_PATTERN definition
rg -n 'TASK_PATTERN' src/utils/ -B 2 -A 2Repository: Fission-AI/OpenSpec
Length of output: 793
Support nested checkbox tasks in validation patterns.
The current patterns (/^[-*]\s+\[[\sx]\]/i and /^[-*]\s+\[[xX\s]\]\s*$/) only match checkboxes at the line start. Nested tasks with leading whitespace (e.g., - [ ] Subtask) are actively used in the repository (e.g., openspec/changes/archive/2025-08-13-add-archive-command/tasks.md) but will not be detected or validated.
Update both patterns to allow optional leading whitespace:
Suggested changes
-export const TASK_PATTERN = /^[-*]\s+\[[\sx]\]/i;
-export const COMPLETED_TASK_PATTERN = /^[-*]\s+\[x\]/i;
+export const TASK_PATTERN = /^\s*[-*]\s+\[[\sx]\]/i;
+export const COMPLETED_TASK_PATTERN = /^\s*[-*]\s+\[x\]/i;And in validator.ts:
-const EMPTY_TASK_PATTERN = /^[-*]\s+\[[xX\s]\]\s*$/;
+const EMPTY_TASK_PATTERN = /^\s*[-*]\s+\[[xX\s]\]\s*$/;🤖 Prompt for AI Agents
In `@src/core/validation/validator.ts` around lines 305 - 334, The checkbox
regexes in validateTasksContent do not allow leading whitespace so nested tasks
like " - [ ] Subtask" are missed; update the TASK_CHECKBOX_PATTERN and
EMPTY_TASK_PATTERN to accept optional leading whitespace (e.g., prepend \s* to
the existing patterns) so validateTasksContent correctly detects and validates
indented checkbox lines (refer to TASK_CHECKBOX_PATTERN, EMPTY_TASK_PATTERN and
the validateTasksContent method).
There was a problem hiding this comment.
Since this regular expression is already used in existing code, the change will be put on hold for now. 🤔
There was a problem hiding this comment.
@atman-33, understood! Since TASK_PATTERN is shared across the codebase (including task-progress.ts), changing it would have broader implications. It makes sense to evaluate the impact on all consumers before making that adjustment.
Feel free to revisit this when you're ready to handle nested tasks uniformly across the validation and progress tracking features.
|
Thanks for reviewing! Let me know if you have any questions. |
1 participant
Overview
Implements comprehensive validation for
tasks.mdfiles in OpenSpec changes. Ensures all changes include properly formatted implementation checklists with checkboxes and non-empty descriptions.What Changed
Implementation (add-tasks-validation)
Validator Methods: Added
validateTasksFile()andvalidateTasksContent()insrc/core/validation/validator.tsCLI Integration: Updated
src/commands/validate.tsvalidateChangeReports()eliminates code duplicationShared Patterns: Unified checkbox validation with
task-progress.tsTASK_CHECKBOX_PATTERN:/^[-*]\s+\[[xX\s]\]/EMPTY_TASK_PATTERN:/^[-*]\s+\[[xX\s]\]\s*$/Testing
test/fixtures/tmp-init/openspec/changes/c1/tasks.mdDocumentation
openspec/specs/cli-validate/spec.mdwith 2 new requirementsValidation Rules
File Existence (ERROR)
tasks.mdmust exist in change directoryCheckbox Presence (ERROR)
- [ ],- [x],- [X]Task Descriptions (ERROR)
Related Issue
Closes #354 - Implement tasks.md checkbox validation
Checklist
Notes
task-progress.tsto ensure consistencyvalidateChangeReports()helper to eliminate duplication in CLI integrationSummary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.