fix(converge): do not re-append work an unchecked task already tracks (#4269) - #4330
fix(converge): do not re-append work an unchecked task already tracks (#4269)#4330ntdatt812 wants to merge 1 commit into
Conversation
The append step computed the next task ID but never asked whether a finding was already represented in tasks.md. So converge run twice, or run before implement had worked through the list, appended the same remediation work again under fresh IDs -- and the second run cannot tell its own previous output apart from the plan's original tasks, so traceability for one piece of work splits across two entries. Findings are now matched against existing unchecked tasks before anything is appended, on the work described and the paths or source-ref it names rather than on wording, and scanning outside code fences like the other commands do. A finding that is already tracked is reported as such rather than silently discarded, and an all-tracked run takes the converged path saying why -- so the operator reads "the work is known", not "the codebase is complete". Closes github#4269
There was a problem hiding this comment.
🟡 Changes recommended
The all-findings-tracked branch incorrectly reports full convergence, and substantive matching lacks regression coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Prevents /speckit-converge from duplicating remediation work already represented by unchecked tasks.
Changes:
- Adds substantive finding-to-task deduplication guidance.
- Excludes fenced checkbox examples and reports skipped findings.
- Adds template contract tests.
File summaries
| File | Description |
|---|---|
templates/commands/converge.md |
Defines deduplication and reporting behavior. |
tests/unit/test_converge_idempotency.py |
Tests the new template requirements. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| `F3 — already tracked by T017, not appended`. If **every** finding is already tracked, | ||
| there is nothing to append: take the `converged` path below and say why, so the operator | ||
| sees "the work is known" rather than "the codebase is complete". |
| def test_findings_are_compared_against_existing_unchecked_tasks(template_text: str) -> None: | ||
| assert re.search(r"unchecked task", template_text, re.IGNORECASE), ( | ||
| "converge no longer says to compare findings against existing unchecked tasks, so " | ||
| "a second run appends the same work again under new IDs" | ||
| ) | ||
| assert re.search(r"`- \[ \]`", template_text), ( | ||
| "the comparison should name the marker it scans for, so the rule is executable" | ||
| ) |
|
This PR addresses the duplication symptom, but the root issue is that Only after all tracked tasks are complete should Please adjust the PR around this lifecycle rather than routing already-tracked incomplete work through the Posted on behalf of @mnriem by GitHub Copilot (model: GPT-5.6 Sol). |
Closes #4269.
Step 7 knew how to compute the next task ID and nothing about whether a finding was already represented:
One item per finding, unconditionally. So the two cases in the issue both land the same way: run converge twice and gaps found both times are appended twice; run it before
/speckit-implementhas worked the list and it appends remediation for work the plan already tracks. The part that makes it hard to undo is that the second run cannot tell its own previous output apart from the plan's original tasks — both are just unchecked lines by then — so the duplicates accumulate and one piece of work ends up split across two IDs.What changed
Findings are matched against the existing unchecked tasks before anything is appended, and an already-covered finding is dropped rather than re-issued.
Three details that matter more than the rule itself:
<source-ref>named — converge writes its own descriptions, so a string match would never fire on its own previous output./speckit-clarifyalready applies and the one [Bug]: /speckit-implement counts checkbox markers inside fenced code blocks — example checkboxes can falsely block implementation #4272 just brought/speckit-implementin line with. A checklist that documents the checkbox format is not tracked work.F3 — already tracked by T017, not appended). A silently discarded finding is indistinguishable from one that was never found. And when every finding is already tracked, the run takes theconvergedpath saying why — so the operator reads "the work is known", not "the codebase is complete". That distinction is the whole value of the run.What I deliberately did not do
The issue's first bullet asks converge to verify a prerequisite implementation checkpoint. That needs a new piece of persisted state and a decision about what counts as "implement has run against this task list" — a design call for maintainers, not something to infer inside a dedup fix. This PR takes the second bullet only, which is self-contained and is what stops the duplicates.
Tests
tests/unit/test_converge_idempotency.py, four cases over the template:- [ ]is named so the rule is executableMutation-checked — weakening "unchecked task" to "existing tasks" and dropping the marker fails exactly one case, the comparison one, and leaves the other three green.
The two failures are
test_add_source_refuses_symlinked_specify_escapeandtest_save_records_refuses_symlinked_specify_escape, which fail identically onmainon this machine — creating a symlink on Windows needs elevation. Measured on a clean checkout, not assumed.