create-implementation-plan: require unique identifiers (#1989)#2142
create-implementation-plan: require unique identifiers (#1989)#2142KomalSrinivasan wants to merge 3 commits into
Conversation
The skill template tells the agent to use REQ-, TASK-, GOAL-, and similar prefixed identifiers, but never says they have to be unique or how to check. @basilevs reported plans coming back with duplicate TASK IDs and proposed three POSIX one-liners that catch the two real collision modes (table rows and bullet declarations) plus a broad diagnostic scan. Document the uniqueness rule under the existing Template Validation Rules, then add a new "Identifier Uniqueness Check" section with all three bash commands and instructions on which must come back empty before the plan is finalized. DEP-* references intentionally allowed in multiple sections per the reporter's note. Closes github#1989.
🔒 PR Risk Scan ResultsScanned 2 changed file(s).
|
🔍 Skill Validator Results
Summary
Full validator output |
|
🟡 Contributor Reputation Check: MEDIUM risk
Maintainers: please review this contributor before merging. |
|
|
Upstream skills/create-implementation-plan/SKILL.md already uses GUD-001 in the template body. Codespell currently slips past it on word-boundary, but the regex alternation (GUD|RISK|...) added in the previous commit on this branch makes codespell flag it as a misspelling of GOOD. GUD is the documented "Guideline" identifier prefix alongside REQ, SEC, CON, PAT, etc. Add it to the ignore-words-list, matching the pattern every other technical-token exemption in .codespellrc uses.
|
@basilevs do you have any guidance on what could be done to improve the instruction? |
…1989 review) basilevs flagged that calling out DEP-* specifically was misleading, because any identifier can appear as a reference. A TASK body can cite a REQ, one TASK can cite another, and so on. The original phrasing made it sound like DEP-* was the only prefix allowed to recur. Rewrite the rule to lead with "uniquely declared": - Define declaration as the leading bullet/cell ID (e.g., the table row in Implementation Phase N, or '- **REQ-001**:'). - Say explicitly that references elsewhere in the plan are expected and not collisions, with concrete examples (TASK citing REQ, TASK citing TASK, Dependencies pointing at a DEP declared upstream). - Tighten the check intro to call (1) and (2) declaration-targeted gates and (3) a broad informational scan that will see references. Bash checks unchanged; they already encode the declaration-vs-reference distinction via the table-cell and bullet-prefix anchors.
|
Thanks @basilevs, fair point - DEP-* as the example made it sound like the only prefix allowed to recur. Reworded the rule in 24171ab to lead with "uniquely declared" and define declaration as the leading bullet/cell ID, then say any identifier can appear as a reference elsewhere (TASK body citing a REQ, one TASK citing another, etc.). Bash checks were already declaration-targeted via the table-cell and bullet-prefix anchors, so they didn't need to change. @aaronpowell ready for another look. |
Closes #1989.
@basilevs reported that the
create-implementation-planskill occasionally produces plans with duplicateTASK-identifiers (and could just as easily collide onREQ-,GOAL-, etc.). The skill template defines the prefix scheme but doesn't say identifiers must be unique, and gives the agent no way to verify.The reporter already worked out the right checks. This PR drops them into the skill.
Changes
skills/create-implementation-plan/SKILL.mdpicks up:A new bullet under
Template Validation Rules: identifiers must be declared exactly once across the plan.DEP-*is called out as the deliberate exception because it commonly appears as a reference in both Requirements and Dependencies.A new
Identifier Uniqueness Checksection with the reporter's three POSIX one-liners:TASK/GOALdeclarations in table rows.Checks 1 and 2 must return empty before finalizing. The section also notes the POSIX prereqs and what to substitute on Windows.
Why this shape
Template Validation Ruleskeeps it visible at the same scan depth as the other invariants.Verification
npm run skill:validatereportscreate-implementation-planas valid.npm run buildexits 0, regeneratesmarketplace.json.