Conversation
|
Thanks @ntdatt812 for consolidating these related changes and incorporating the earlier feedback. The converge prerequisite follows the lifecycle I requested, and the legacy issue handling now asks rather than guessing which feature a bare task ID belongs to. The scope is suitable for review. Please complete the existing Claude Code disclosure with the model(s) and settings/mode used; the tool and extent are already documented. On my side, the next step is CI and review of this consolidated head. There’s no need to split it back into the original PRs. Drafted for @mnriem by GitHub Copilot (model: GPT-6 Astra). |
There was a problem hiding this comment.
🟡 Changes recommended
Pagination can misclassify legacy issues, and overlapping task and requirement findings can append duplicate remediation work.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Consolidates task-bookkeeping fixes across implement, taskstoissues, and converge.
Changes:
- Excludes fenced checkbox examples from task counts.
- Scopes issue deduplication by feature.
- Gates convergence on completed tasks and adds regression coverage.
File summaries
| File | Description |
|---|---|
templates/commands/implement.md |
Excludes fenced checkboxes. |
templates/commands/taskstoissues.md |
Adds feature-scoped issue titles and legacy handling. |
templates/commands/converge.md |
Adds prerequisite gate and task inventory. |
docs/reference/agentic-sdd.md |
Documents convergence lifecycle. |
tests/unit/test_checklist_scan_contract.py |
Tests checkbox scan rules. |
tests/unit/test_taskstoissues_feature_scope.py |
Tests feature-scoped deduplication. |
tests/unit/test_converge_prerequisite.py |
Tests convergence prerequisites. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| - **Task inventory**: every task in `tasks.md` — all of them checked by this point — with | ||
| the work it describes and the file paths it names. A task marked done whose work is | ||
| absent from the code, or only partly there, is a finding like any other, traced to | ||
| that task's ID. |
There was a problem hiding this comment.
Confirmed, and it was my own addition that opened it. T017 exists because FR-003 asked for it, so the task inventory reaches the same gap the requirements inventory already reaches. Two findings, two appended tasks, each carrying half the trace — and since neither is built, the next run finds the overlap again and appends both once more. That is #4269 one layer up from where the prerequisite gate stopped it.
Fixed in 42e8d4e. Step 4 now coalesces: two inventory items are the same work when one change to the same code would close both, and they collapse into a single Finding whose source-refs keeps every origin it was coalesced from. The record field is plural now (one or more), and Step 7 writes them comma-separated — per FR-003, T017 (missing) — in one checklist item, so the coalescing survives into tasks.md instead of being undone at the append step. Items that merely sit in the same file are explicitly not the same work and stay separate.
The task-inventory bullet in Step 3 now points forward at that rule, because it is what creates the second path to the gap; without the pointer the agent can reasonably report both.
Regression cover in tests/unit/test_converge_finding_coalescing.py (4 tests): the coalescing rule and its combined form, source-refs accepting more than one, the same-work criterion, the one-item append shape, and the Step 3 pointer. Reverting converge.md to the version you reviewed turns all four red — I checked rather than assumed.
AI assistance disclosed: written with Claude Code; I reviewed and ran the tests and lint myself.
| 1. **Fetch existing issues for deduplication**: Before creating anything, build the set of task IDs you are about to process from `tasks.md` (each is a `T` followed by **at least** three digits, e.g. `T001` — `__SPECKIT_COMMAND_CONVERGE__` assigns new IDs with `T{M+1:03d}`, which is a floor rather than a cap, so once a file has more than 999 tasks the IDs are four digits or longer). Then use the GitHub MCP server's `list_issues` tool to look for issues that already cover those IDs. Do not pass a `state` value, since omitting it makes the tool return both open and closed issues. Request `perPage: 100` to keep the number of calls down, and since the tool uses cursor-based pagination, request pages with the `after` parameter (using the `endCursor` from the previous response). For each issue title, match it against the task ID pattern `\bT\d{3,}\b` (the `{3,}` accepts four-digit and longer IDs — with `\d{3}` a title containing `T1000` would not match at all, because the trailing `\b` cannot fall between two digits, so that task would be silently neither deduplicated nor created; word boundaries still stop a token like `ST001` from matching, and force the whole digit run to be consumed so `T100` can never match inside `T1000`; this also recognises titles written as `T001 ...`, `T001: ...` or `[T001] ...`) and, when it matches one of your task IDs, mark that ID as already having an issue. Stop paginating as soon as every task ID has been matched, or when there are no more pages, so you do not keep fetching the whole repository's issue history once all task IDs are accounted for. This bounds the number of calls on repos with large issue histories and still prevents duplicates when the command is re-run after `tasks.md` is regenerated or the skill is re-invoked. | ||
| 1. For each task in the list, use the GitHub MCP server to create a new issue in the repository that is representative of the Git remote. Task lines in `tasks.md` start with a markdown checkbox, so first strip the leading `- [ ]` (and any `[P]` / `[US#]` markers) to recover the task ID and its description. Create the issue with a single canonical title of the form `T001: <description>`, with the ID written once followed by the task description (for example, the line `- [ ] T001 Create project structure` becomes the title `T001: Create project structure`). | ||
| - **Skip** any task whose ID is already present in the set of existing issues from the previous step, and report it (for example, `T001 already has an issue, skipping`). | ||
| 1. **Fetch existing issues for deduplication**: Before creating anything, build the set of task IDs you are about to process from `tasks.md` (each is a `T` followed by **at least** three digits, e.g. `T001` — `__SPECKIT_COMMAND_CONVERGE__` assigns new IDs with `T{M+1:03d}`, which is a floor rather than a cap, so once a file has more than 999 tasks the IDs are four digits or longer). Then use the GitHub MCP server's `list_issues` tool to look for issues that already cover those IDs. Do not pass a `state` value, since omitting it makes the tool return both open and closed issues. Request `perPage: 100` to keep the number of calls down, and since the tool uses cursor-based pagination, request pages with the `after` parameter (using the `endCursor` from the previous response). For each issue title, match it against the task ID pattern `\bT\d{3,}\b` (the `{3,}` accepts four-digit and longer IDs — with `\d{3}` a title containing `T1000` would not match at all, because the trailing `\b` cannot fall between two digits, so that task would be silently neither deduplicated nor created; word boundaries still stop a token like `ST001` from matching, and force the whole digit run to be consumed so `T100` can never match inside `T1000`; this also recognises titles written as `T001 ...`, `T001: ...` or `[T001] ...`) and, when it matches one of your task IDs, mark that ID as already having an issue **only if the title also carries this feature's identifier** (see below). Task IDs restart at `T001` in every feature's `tasks.md`, so an unscoped match means the first feature to reach the tracker permanently suppresses `T001` for every later feature -- a silent gap in exactly the multi-feature repos this command is for. Stop paginating as soon as every task ID has been matched, or when there are no more pages, so you do not keep fetching the whole repository's issue history once all task IDs are accounted for. This bounds the number of calls on repos with large issue histories and still prevents duplicates when the command is re-run after `tasks.md` is regenerated or the skill is re-invoked. |
There was a problem hiding this comment.
Correct, and it is an ordering bug I introduced by redefining "matched" without revisiting the exit that reads it. A scoped match is an answer; a bare match is a question. The exit fired on either, so one early page of bare T001: ... titles ended pagination and the scoped [002-billing] T001: ... issues on the next page were never fetched — then every ID went to the user as bare-only, prompting about issues that were in fact already matched, with a duplicate created for each wrong answer.
Fixed in 42e8d4e. The early exit now fires only when every task ID has a scoped match; if any ID is unmatched or matched only by a bare title, the pages are exhausted before anything is classified. Only "no more pages" makes bare-only a fact, and the rule says so where the classification happens as well as where pagination stops — the bare-only list is assembled "only once the pages have run out", so it cannot name an ID whose scoped issue was simply never fetched.
The bound this exit existed for is intact: on a repo whose tasks are all already scoped, it still stops at the first page that completes the set.
test_the_early_exit_requires_a_scoped_match_for_every_id pins both halves and pins the old wording out verbatim. Reverting taskstoissues.md to the reviewed version turns it red.
AI assistance disclosed: written with Claude Code; I reviewed and ran the tests and lint myself.
|
After deeper review, I’m revising the earlier consolidation guidance. These changes affect three different core commands and their behavior contracts, so please separate Changes to core commands and templates must meet a very high bar. Each proposal needs evidence of the current problem, justification for the smallest necessary correction, and reproducible before/after results covering existing supported behavior and relevant compatibility cases. For changes to agent instructions, tests asserting that particular wording exists are useful, but are not sufficient evidence that the intended behavior works. Include representative agent-run results and relevant artifacts. New lifecycle gates, issue-title conventions, and legacy migration behavior require explicit consideration; they should not be treated as incidental bookkeeping changes. Drafted for @mnriem with assistance from GitHub Copilot (model: GPT-6 Astra; interactive comment drafting). |
The checklist gate counted every `- [ ]` / `- [x]` line in every checklist file, fenced blocks included. A checklist that documents the checkbox format with an example fence therefore reported unchecked items nobody can ever tick, and /speckit-implement stops on a non-zero unchecked count -- so writing down the format blocked implementation. /speckit-clarify already scopes its scan to markers outside code fences, so this was also the two commands disagreeing about what a checklist item is. They now state the same rule. Closes github#4272
The scan-instruction pattern only recognised a definition whose first marker is unchecked, so implement.md's "Checked items: Lines matching `- [X]`" line was never parametrised. Removing its code-fence exclusion left the suite green. Match a checked or unchecked first marker: four definitions are now guarded instead of three, and that removal fails.
Task IDs are local to a feature -- every tasks.md restarts at T001 -- but the
dedup matched existing issues on the bare ID. So once feature 001-auth had an
issue titled T001, running the command for 002-billing saw "T001 exists" and
skipped it. The task was never created and nothing said so, which is a silent
gap in exactly the multi-feature repos this command targets.
The canonical title now carries the feature directory basename, and a task is
skipped only when an existing issue matches both that identifier and the ID.
The ID keeps its own word boundaries inside the prefixed title, so the
\bT\d{3,}\b matching from github#2968 is unchanged.
Issues filed before the prefix existed carry a bare `T001: ...`; those are
still recognised for their own feature, so upgrading does not re-create work
that is already tracked.
Closes github#4271
The sentence added in the previous commit — "so the `\bT\d{3,}\b` matching
above is unchanged by the prefix" — reached the file with two literal U+0008
BACKSPACE bytes where `\b` was meant. My editing pipeline interpreted the
escape rather than passing it through.
A control character cannot appear in a YAML block scalar, so the generator
fell back to a double-quoted flow scalar for the whole prompt, and
`test_yaml_has_prompt` failed on `speckit.taskstoissues.yaml`:
AssertionError: speckit.taskstoissues.yaml missing prompt block scalar
Reproduced and pinned locally: with the two bytes present the test fails with
that exact message, and with them written as `\b` the goose suite is 38/38.
The rendered sentence is unchanged — it was always meant to read `\b`.
The upgrade rule treated a bare `T001: ...` title as this feature's whenever no scoped title existed for that ID. That is exactly the state of every feature on its first run after upgrading, so a bare T001 filed for 001-auth still suppressed 002-billing's T001: the github#4271 skip, back. A bare title names no feature, and neither does the absence of a scoped one, so no inference from the tracker can settle it. Skipping drops a task silently; creating duplicates one an existing user already tracks. The command now lists every ID that matched only a bare title, with the issue and this feature's description for the task, and asks before creating anything. Confirmed issues are skipped, and retitled to the scoped form only if the user agrees, so the question does not recur.
…append them Converge is meant to assess a finished implementation, and nothing enforced that. Run while tasks.md still had open work, it assessed the code anyway, found that unbuilt work as new gaps and appended it again under fresh IDs, so every re-run duplicated its own remediation tasks (github#4269). And a run that found nothing new reported `converged`, whose report says the implementation satisfies the spec while tracked work is still open. Step 1 now checks for unchecked tasks (outside code fences, the rule implement counts by) and stops before any assessment, listing them and pointing to implement, with tasks.md untouched. Once every task is checked, each task joins the intent inventory, so one marked done but not built is a finding traced to its ID. Anything appended makes the next run stop again until it is implemented. The handoff and docs/reference/agentic-sdd.md describe the gate, and the tests pin the lifecycle. Closes github#4269
05dfd35 to
e2c0415
Compare
|
Rebased onto One conflict, in
AI assistance disclosed: this change and this comment were written with an AI coding agent, reviewed and verified by me before posting. |
|
Please address open Copilot feedback |
… a match bare-only Two faults the review found in this PR's own additions. converge: the task inventory reaches the same gap the requirements inventory already reaches -- `T017` exists because `FR-003` asked for it -- so absent code produced a finding under each, and Step 7 appended two remediation tasks for one piece of work, each carrying half the trace and both returning on the next run. Step 4 now coalesces items that one change to the same code would close, into a single finding whose `source-refs` keeps every origin, written comma-separated in one checklist item. Items that merely share a file stay separate. taskstoissues: scoping split "matched" into a scoped match and a bare-only match, but the early exit still fired on either. A first page of bare `T001: ...` titles therefore ended pagination before the scoped issues on a later page were seen, and every ID was handed to the user as bare-only -- a prompt for issues already matched, and a duplicate for every wrong answer. The exit now requires a scoped match for every ID, and the bare-only list is assembled only once the pages have run out. Contract tests over both templates. Reverting either template to the reviewed version turns the matching tests red.
|
@mnriem Both addressed in 42e8d4e — details in the two threads. Both were faults in this PR's own additions, not pre-existing:
Evidence: AI assistance disclosed: written with Claude Code; I reviewed and ran the tests and lint myself. |
|
Please address Copilot feedback and fix test & lint errors |
| 1. For each task in the list, use the GitHub MCP server to create a new issue in the repository that is representative of the Git remote. Task lines in `tasks.md` start with a markdown checkbox, so first strip the leading `- [ ]` (and any `[P]` / `[US#]` markers) to recover the task ID and its description. Create the issue with a single canonical title of the form `T001: <description>`, with the ID written once followed by the task description (for example, the line `- [ ] T001 Create project structure` becomes the title `T001: Create project structure`). | ||
| - **Skip** any task whose ID is already present in the set of existing issues from the previous step, and report it (for example, `T001 already has an issue, skipping`). | ||
| 1. **Fetch existing issues for deduplication**: Before creating anything, build the set of task IDs you are about to process from `tasks.md` (each is a `T` followed by **at least** three digits, e.g. `T001` — `__SPECKIT_COMMAND_CONVERGE__` assigns new IDs with `T{M+1:03d}`, which is a floor rather than a cap, so once a file has more than 999 tasks the IDs are four digits or longer). Then use the GitHub MCP server's `list_issues` tool to look for issues that already cover those IDs. Do not pass a `state` value, since omitting it makes the tool return both open and closed issues. Request `perPage: 100` to keep the number of calls down, and since the tool uses cursor-based pagination, request pages with the `after` parameter (using the `endCursor` from the previous response). For each issue title, match it against the task ID pattern `\bT\d{3,}\b` (the `{3,}` accepts four-digit and longer IDs — with `\d{3}` a title containing `T1000` would not match at all, because the trailing `\b` cannot fall between two digits, so that task would be silently neither deduplicated nor created; word boundaries still stop a token like `ST001` from matching, and force the whole digit run to be consumed so `T100` can never match inside `T1000`; this also recognises titles written as `T001 ...`, `T001: ...` or `[T001] ...`) and, when it matches one of your task IDs, mark that ID as already having an issue **only if the title also carries this feature's identifier** (see below). Task IDs restart at `T001` in every feature's `tasks.md`, so an unscoped match means the first feature to reach the tracker permanently suppresses `T001` for every later feature -- a silent gap in exactly the multi-feature repos this command is for. Stop paginating early **only** when every task ID has a scoped match; otherwise exhaust the pages before classifying any ID as bare-only. A bare match is not an answer, it is the question below, and the scoped issue that answers it may sit on a later page — one early page carrying bare `T001: ...` titles for every ID would otherwise end the search and send every one of them to the user as bare-only, prompting about issues that were already matched and risking a duplicate for each. Only "no more pages" makes bare-only a fact. So stop when every ID is scoped, or when the pages run out, so you do not keep fetching the whole repository's issue history once all task IDs are genuinely accounted for. This bounds the number of calls on repos with large issue histories and still prevents duplicates when the command is re-run after `tasks.md` is regenerated or the skill is re-invoked. | ||
| 1. For each task in the list, use the GitHub MCP server to create a new issue in the repository that is representative of the Git remote. Task lines in `tasks.md` start with a markdown checkbox, so first strip the leading `- [ ]` (and any `[P]` / `[US#]` markers) to recover the task ID and its description. Create the issue with a single canonical title of the form `[<feature>] T001: <description>`, where `<feature>` is the basename of FEATURE_DIR parsed in step 1 (the `NNN-name` spec directory, e.g. `002-billing`), followed by the ID written once and then the task description (for example, the line `- [ ] T001 Create project structure` in feature `002-billing` becomes the title `[002-billing] T001: Create project structure`). The ID keeps its own word boundaries, so the `\bT\d{3,}\b` matching above is unchanged by the prefix. |
|
Address Copilot feedback and fix test & lint errors |
Consolidates #4313, #4314 and #4330 into one pull request, as asked on #4460 (
author-over-cap). Those three will be closed in favour of this one. Each also had review feedback outstanding; that is addressed here, and noted per section below.The three share one subject: how the command templates read task bookkeeping in
tasks.mdand in the issue tracker. They stay separable. Each fix is its own run of commits, and each run was cherry-picked alone ontomainand passes its own tests there, so any one can be dropped without touching the others.Closes #4269, closes #4271, closes #4272.
1.
/speckit-implementcounts checkbox markers inside code fences (#4272), from #4313implement.mddefined its total, checked and unchecked counts on every- [ ]/- [x]line, so a checklist that documents the checkbox format in a fenced example reported items nobody can tick. A non-zero unchecked count stops implementation./speckit-clarifyalready scoped its scan to markers outside code fences;implementnow does the same.tests/unit/test_checklist_scan_contract.pyasserts that every line in a command template that defines a checkbox-marker scan also excludes code fences.Review feedback: Copilot pointed out that the test only recognised a definition whose first marker is unchecked, so the "Checked items: Lines matching
- [X]" definition was never parametrised; removing its exclusion left the suite green. Measured before changing it: with the old pattern that removal passes 4/4. The pattern now accepts a checked or unchecked first marker, four definitions are guarded instead of three, and the same removal fails.2.
/speckit-taskstoissuesdedup matches bare task IDs across features (#4271), from #4314Task IDs restart at
T001in every feature'stasks.md, and dedup matched existing issues on the ID alone. So the first feature to reach the tracker permanently suppressedT001for every later feature, and those tasks were silently never created. Issue titles now carry the feature ([002-billing] T001: ..., from theFEATURE_DIRbasename), and a task is skipped only when an issue matches both the feature and the ID.Review feedback, and backward compatibility: Copilot found that the upgrade rule brought the bug straight back. It treated a bare legacy
T001: ...title as this feature's whenever no scoped title existed for that ID, and that is the state of every feature on its first run after upgrading: a bareT001filed for001-authstill suppressed002-billing'sT001.A bare title names no feature, and neither does the absence of a scoped one, so nothing in the tracker can settle which feature it belongs to. Deciding either way breaks someone: skipping drops a task silently, and creating duplicates one an existing user already tracks. That second case is the compatibility concern raised on #4314. So the command now lists every ID that matched only a bare title, each with the issue and this feature's description for the task, and asks before creating anything. Confirmed issues are skipped. It then offers to retitle them to the scoped form, and does so only for the ones the user agrees to, so the question does not come back on later runs. Existing users therefore get no duplicates and no silent gaps; the cost is one question per legacy issue, once.
The U+0008 bytes Copilot flagged on the earlier commit were already fixed in that PR (
\bwritten as two characters);tests/integrations/test_integration_goose.py, which caught them, passes.3.
/speckit-convergeis not idempotent (#4269), from #4330#4330 dropped findings that an unchecked task already covered. Review there identified the root cause: converge never enforced its prerequisite. Run while
tasks.mdstill had open tasks, it assessed the code anyway, found that unbuilt work as new gaps and appended it again under fresh IDs. And a run whose findings were all already tracked ended inconverged, whose report says the implementation satisfies the spec while tracked work is still open.Reworked around the lifecycle instead of the dedup rule:
tasks.mdhas any unchecked task (outside code fences, the ruleimplementcounts by), converge stops before assessing anything. It lists the open task IDs, points to/speckit-implement, and leavestasks.mdbyte-for-byte unchanged. This is neitherconvergednortasks_appended.convergedis only reachable once the work is actually finished.docs/reference/agentic-sdd.mddescribe the gate.tests/unit/test_converge_prerequisite.pypins the lifecycle: the gate sits in Step 1 and stops before Step 2, namesimplement, lists the open IDs, leavestasks.mduntouched, counts outside code fences, is not reported asconverged, the inventory includes tasks, and the docs describe it. Copilot's other note on #4330, that the dedup test did not pin substance-based matching, no longer applies, since that rule is gone.Evidence
maintest_checklist_scan_contract.pytest_taskstoissues_feature_scope.pytest_converge_prerequisite.pymain: its own tests plus the goose integration tests pass (43, 43 and 45).STOPfails 1; removing the task inventory fails 1.uvx ruff@0.15.0 check src tests: clean. markdownlint reports the same number of findings on the four touched Markdown files as onmain(all pre-existing, in untouched lines).tests/unitandtests/integrationson Windows: the only failures are symlink-privilege errors (WinError 1314); the two whose names do not say so fail identically on a clean checkout ofmain.AI disclosure
Per CONTRIBUTING: this pull request (code, tests, measurements and this description) was developed with Claude Code as a coding agent.