fix(tasks): count indented sub-tasks so a change with unfinished work isn't reported complete - #1486
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughTask checkbox parsing is centralized in ChangesTask parsing and progress
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant tasks.md
participant parseTaskLines
participant countTasksFromContent
participant generateApplyInstructions
tasks.md->>parseTaskLines: provide task content
parseTaskLines-->>countTasksFromContent: return ordered ParsedTask entries
parseTaskLines-->>generateApplyInstructions: return parsed tasks
countTasksFromContent-->>generateApplyInstructions: provide consistent progress counts
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Clean fix. Sharing one indentation-aware parser across progress, archive gating, and apply instructions removes the silent-completion mismatch; the focused tests and exact-head build pass.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/core/archive.test.ts`:
- Around line 257-261: Align both archive warning tests in
test/core/archive.test.ts:257-261 and test/core/archive.test.ts:309-313 with the
contract by removing { yes: true } when asserting warnings, or instead assert
that --yes suppresses them. Apply the same correction to the unterminated-fence
case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 15a32424-0c67-4843-b276-01f1ce24aae5
📒 Files selected for processing (10)
.changeset/count-indented-subtasks.mdsrc/commands/workflow/instructions.tssrc/core/parsers/code-fence.tssrc/utils/task-progress.tstest/commands/apply-instructions-tasks.test.tstest/core/archive.test.tstest/core/list.test.tstest/core/parsers/code-fence.test.tstest/core/view.test.tstest/utils/task-progress.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/utils/task-progress.ts
- test/utils/task-progress.test.ts
- src/commands/workflow/instructions.ts
| await archiveCommand.execute(changeName, { yes: true }); | ||
|
|
||
| expect(console.log).toHaveBeenCalledWith( | ||
| expect.stringContaining('Warning: 2 incomplete task(s) found') | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align --yes expectations with the stated archive contract.
The PR objective says incomplete-task warnings occur unless --yes is used, but both tests pass { yes: true } and require a warning. This locks in the opposite behavior.
test/core/archive.test.ts#L257-L261: exercise the warning path withoutyes, or assert thatyesbypasses the warning.test/core/archive.test.ts#L309-L313: apply the same correction for the unterminated-fence case.
📍 Affects 1 file
test/core/archive.test.ts#L257-L261(this comment)test/core/archive.test.ts#L309-L313
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/core/archive.test.ts` around lines 257 - 261, Align both archive warning
tests in test/core/archive.test.ts:257-261 and test/core/archive.test.ts:309-313
with the contract by removing { yes: true } when asserting warnings, or instead
assert that --yes suppresses them. Apply the same correction to the
unterminated-fence case.
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The code and focused suite look clean, and CodeRabbit's warning-test comment is not valid because --yes explicitly logs the warning before continuing. One blocking wording fix remains: the changeset and stale PR body say every old match is preserved and task totals can never fall, but closed-fence checkboxes are now intentionally excluded; please correct that claim and note that apply still omits bare checkboxes before re-review.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.changeset/count-indented-subtasks.md:
- Around line 5-7: Update the “Both surfaces now share one parser” wording in
the changeset to refer to all affected commands, such as “All affected
commands,” while preserving the rest of the explanation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2eb797ea-09ed-48a8-96a5-4a06931e774a
📒 Files selected for processing (5)
.changeset/count-indented-subtasks.mdsrc/utils/task-progress.tstest/commands/apply-instructions-tasks.test.tstest/core/archive.test.tstest/utils/task-progress.test.ts
💤 Files with no reviewable changes (2)
- test/commands/apply-instructions-tasks.test.ts
- test/core/archive.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/utils/task-progress.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/commands/workflow/instructions.ts (1)
328-349: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not derive apply progress from the filtered task list.
parseTasksFileremoves empty-description checkboxes, butgenerateApplyInstructionscalculatestotal,complete, andstatefrom that filtered array. A file containing one completed task and one unchecked blank checkbox can therefore reportall_done, whilelistandarchivestill report incomplete work. Keep progress accounting based on all parsed checkbox entries, and filter only the displayed/actionabletasks; add a regression test for this mixed case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/workflow/instructions.ts` around lines 328 - 349, Update generateApplyInstructions to calculate total, complete, and state from all checkbox entries returned by parseTaskLines, while filtering empty-description entries only from the displayed/actionable tasks array. Preserve existing instruction generation for non-empty tasks and add a regression test covering one completed task plus one unchecked blank checkbox, which must report incomplete progress.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/commands/workflow/instructions.ts`:
- Around line 328-349: Update generateApplyInstructions to calculate total,
complete, and state from all checkbox entries returned by parseTaskLines, while
filtering empty-description entries only from the displayed/actionable tasks
array. Preserve existing instruction generation for non-empty tasks and add a
regression test covering one completed task plus one unchecked blank checkbox,
which must report incomplete progress.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 394329bc-6bb4-411d-b409-ab96c3fd1773
📒 Files selected for processing (3)
.changeset/count-indented-subtasks.mdsrc/commands/workflow/instructions.tstest/commands/apply-instructions-tasks.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- test/commands/apply-instructions-tasks.test.ts
- .changeset/count-indented-subtasks.md
|
Review disposition (four adversarial review passes, all findings triaged): Fixed
Not changed, with reasons
|
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The copy/fence correction is clean and all exact-head checks pass, but the latest CodeRabbit finding is valid. I reproduced - [x] 1.1 Done plus an unchecked blank checkbox returning state: all_done, 1/1, while list/archive count 1/2; compute progress/state from all parsed checkboxes and filter only the displayed task rows, then add the mixed-case regression before re-review.
4252948 to
35309d3
Compare
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The force-squashed head still contains the blocker from the previous review. generateApplyInstructions calculates progress from parseTasksFile() after blank rows are filtered, and the new mixed test asserts only the displayed tasks plus list-side count; exact-head reproduction still returns all_done and 1/1 for one completed named task plus one unchecked blank task while list/archive report 1/2. Please compute progress/state from all parsed checkboxes, filter only returned display rows, and assert apply's progress/state in that mixed test.
35309d3 to
ea90878
Compare
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Clean now. Progress/state count every parsed checkbox while the apply list filters only blank display rows; the exact mixed-case reproduction returns 1/2 and ready, all 123 focused tests pass, and the hosted matrix plus CodeRabbit are green.
Both checkbox parsers anchored the bullet at column 0, so an indented sub-task was invisible to `openspec list`/`view` progress, to the apply task list, and to archive's incomplete-task check. A change whose sub-tasks were unfinished reported "✓ Complete" and archived with no warning. One shared `parseTaskLines()` now backs both surfaces and allows leading whitespace. It matches every line the two patterns it replaces matched, and more - including a tab or non-breaking space inside the brackets, which the old counting pattern accepted - so task counts can rise but never fall: no change starts reporting less work than before, and archive's gate can only get stricter. Checkboxes still count wherever they sit, including inside a code fence. Skipping fenced ones was implemented and dropped: every rule for deciding which fence is real has an input where a stray or unbalanced ``` swallows genuine tasks, which is the silent failure this fix exists to remove. Verified differentially against a build of main over hand-built fixtures and the repo's own 120 tasks.md files: 0 files count fewer tasks, 0 lose an incomplete-task warning. Closes #1485 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ea90878 to
2c727f5
Compare
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Clean wording-only follow-up. It preserves the verified progress/display split, clarifies the all-textless-checkbox blocked state, the exact-head build and apply tests pass, and the full hosted matrix plus CodeRabbit are green.
Deploying openspec-docs with
|
| Latest commit: |
48fc37c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c5173f54.openspec-docs.pages.dev |
| Branch Preview URL: | https://fix-parser-encoding-toleranc.openspec-docs.pages.dev |
2 participants
Status: Ready. Closes #1485. One commit. Full suite green (3405 tests, 116 files), lint and typecheck clean, verified end to end and differentially against a build of
main.What was wrong
A
tasks.mdwith indented sub-tasks reported the wrong progress: only checkboxes at column 0 were counted. A change whose sub-tasks were unfinished showed as done and archived without a warning.openspec list✓ Complete2/4 tasksopenspec archive --json(no--yes)archive_tasks_incomplete: 2 incomplete task(s)openspec instructions applyThe same blind spot hid the remaining work from agents resuming a change, since the apply task list is where they read what is left.
How it was fixed
Two regexes anchored at column 0 —
src/utils/task-progress.tsfor counting,src/commands/workflow/instructions.tsfor the apply task list — are replaced by one sharedparseTaskLines()that allows leading whitespace. Both surfaces now read the same lines of the same file, and after this change there is exactly one checkbox regex left insrc/.Why it can't take work away from a user
The new pattern matches a superset of what both old patterns matched — every part of it was relaxed, none tightened (
^→^\s*,\s+→\s*,[ xX]→[\sxX],(.+)$→(.*)). Keeping\sinside the brackets preserves the old counting pattern's tolerance for a tab or non-breaking space there.So task counts never fall, which pins down the surfaces that matter:
list/view;Evidence:
\r, NBSP, ideographic space and BOM) found zero cases where an old pattern matched and the new one did not, and zero description differences.mainover hand-built fixtures (nested, CRLF, BOM, bare checkbox, HTML comment, indented code block, fenced examples, stray unterminated fence, a 50k-line file) and the repo's own 120tasks.mdfiles: 0 files count fewer tasks, 0 lose an incomplete-task warning. 5 of the 120 count more, all of them genuine sub-tasks that were invisible.The one cost, stated plainly
Checkboxes are counted wherever they appear — inside a code fence, an HTML comment, or an indented block. That was already true on
mainfor lines at column 0; allowing indentation extends it to indented ones. So atasks.mdthat shows a checklist as a format example can now count that example as work and make archive ask for--yes.Fence-awareness was built to avoid this and then deliberately dropped. Every rule for deciding which fence is "real" has an input where a stray or unbalanced ``` swallows genuine tasks: the parity-based guard I first wrote let a mid-file unterminated fence be "closed" by the next fence opener, hiding every task between them —
✓ Completewith two unfinished tasks, on a file `main` handled correctly. Counting a documented example as work is a loud, bypassable false positive; losing a real task is a silent false negative that disables exactly the gate this PR exists to restore. The `parseTaskLines` docblock records that reasoning so the next person does not retry it.Proof
test/utils/task-progress.test.ts— sub-tasks at every depth, CRLF and trimming, bare checkboxes, a guard asserting every shape the old patterns accepted still counts, and the fenced/unterminated-fence behavior pinned as the documented limitation.test/commands/apply-instructions-tasks.test.ts— apply lists sub-tasks, agrees withgetTaskProgressForChange, and skips a checkbox carrying no text.test/core/archive.test.ts— the gate warns on unfinished sub-tasks (#1485), and prompts then cancels when the user declines.test/core/list.test.ts/test/core/view.test.ts— no✓ Complete, and the change stays in Active.Reverting
^\s*to^fails 10 tests across all five files.Notes
listand archive always agree on how much work is left. The displayed rows also decide apply's "nothing to work on" state, so a file of nothing but text-less checkboxes now asks to be rewritten instead of reporting itself done.mainlisted such a line when it happened to carry trailing whitespace, an artifact of(.+)backtracking; that row is now consistently hidden. Covered by tests asserting state and progress, not just the row list.applyreads one tracked file while progress globs the tracked-tasks artifact.X.Y.Znumbering the template prescribes; worth a follow-up if unnumbered task files become common.archive-change.ts,bulk-archive-change.ts,verify-change.ts) still tell agents to count- [ ]vs- [x]themselves and say nothing about sub-tasks. Worth a follow-up; it needs a golden-hash regen, so it is out of scope here.schemas/spec-driven/schema.yamlis untouched, so no generated skills or hashes move.tasks.mdfiles.🤖 Generated with Claude Code