diff --git a/.changeset/tasks-per-group-tests-docs.md b/.changeset/tasks-per-group-tests-docs.md new file mode 100644 index 0000000000..db842203e6 --- /dev/null +++ b/.changeset/tasks-per-group-tests-docs.md @@ -0,0 +1,5 @@ +--- +"@fission-ai/openspec": patch +--- + +Task guidance now requires each task group to land its own tests and documentation updates instead of deferring them to a trailing group. The onboarding walkthrough teaches the same rule, and the published schema reference no longer quotes stale instruction text. diff --git a/docs-lab/reference/schemas/spec-driven/index.md b/docs-lab/reference/schemas/spec-driven/index.md index cf613bbf75..06d375792e 100644 --- a/docs-lab/reference/schemas/spec-driven/index.md +++ b/docs-lab/reference/schemas/spec-driven/index.md @@ -214,10 +214,16 @@ left with a `TBD ... Update Purpose after archive` placeholder to fill in by hand. Do NOT add `## Purpose` to a delta for an existing capability - that spec already has one and the delta's is ignored. To change an existing capability's Purpose - including a leftover `TBD` placeholder - -edit `openspec/specs//spec.md` directly. +edit `/openspec/specs//spec.md` +directly. `planningHome.root` comes from the `openspec instructions ... +--json` response. Always use it rather than a repo-relative path: it +resolves to the store whenever the change lives in one - whether that +came from `--store`, a project `store:` pointer, or a global default +store - and to the current repository otherwise. Do not try to work out +which case applies; the field already has. MODIFIED requirements workflow: -1. Locate the existing requirement in openspec/specs//spec.md +1. Locate the existing requirement in `/openspec/specs//spec.md` (the same store-aware root as above) 2. Copy the ENTIRE requirement block (from `### Requirement:` through all scenarios) 3. Paste under `## MODIFIED Requirements` and edit to reflect new behavior 4. Ensure header text matches exactly (whitespace-insensitive) @@ -362,6 +368,18 @@ Guidelines: - Each task MUST be a checkbox: `- [ ] X.Y Task description` - Tasks should be small enough to complete in one session - Order tasks by dependency (what must be done first?) +- Each task MUST state how to verify completion (a test, command, + observable behavior, or delivered artifact). Put the verification in + that task's checkbox description. Use a separate verification task only + when it checks broader integration or system behavior that spans + multiple implementation tasks. +- Each task group MUST land the tests and documentation its own work + calls for. Do NOT collect testing or documentation into a final group - + when a late group first exercises work from an early one, the failures + cascade back through every group in between and force rework. A group + whose work calls for neither, such as scaffolding or dependency setup, + carries neither. A final group is for integration checks only, not for + the tests and docs an earlier group owed. Example: ``` @@ -369,17 +387,17 @@ Example: ## 1. Setup -- [ ] 1.1 Create new module structure -- [ ] 1.2 Add dependencies to package.json +- [ ] 1.1 Create new module structure and verify expected files are present +- [ ] 1.2 Add dependencies to package.json and verify package installation succeeds ## 2. Core Implementation -- [ ] 2.1 Implement data export function -- [ ] 2.2 Add CSV formatting utilities +- [ ] 2.1 Implement data export function and verify the export test passes +- [ ] 2.2 Add CSV formatting utilities and verify unit tests cover quoting and delimiters +- [ ] 2.3 Document the export API in docs/export.md and verify the documented command runs as written ``` Reference specs for what needs to be built, design for how to build it. -Each task should be verifiable - you know when it's done. ```` ## Apply diff --git a/docs/concepts.md b/docs/concepts.md index 10106c5b78..824795b659 100644 --- a/docs/concepts.md +++ b/docs/concepts.md @@ -341,6 +341,8 @@ Tasks are the **implementation checklist** — concrete steps with checkboxes. - Group related tasks under headings - Use hierarchical numbering (1.1, 1.2, etc.) - Keep tasks small enough to complete in one session +- State how each task is verified (a test, command, or observable result) +- Land the tests and documentation each group's work calls for inside that group, not in a final catch-up group - Check tasks off as you complete them ## Delta Specs diff --git a/schemas/spec-driven/schema.yaml b/schemas/spec-driven/schema.yaml index 688c38a20b..51322aa03b 100644 --- a/schemas/spec-driven/schema.yaml +++ b/schemas/spec-driven/schema.yaml @@ -210,6 +210,13 @@ artifacts: that task's checkbox description. Use a separate verification task only when it checks broader integration or system behavior that spans multiple implementation tasks. + - Each task group MUST land the tests and documentation its own work + calls for. Do NOT collect testing or documentation into a final group - + when a late group first exercises work from an early one, the failures + cascade back through every group in between and force rework. A group + whose work calls for neither, such as scaffolding or dependency setup, + carries neither. A final group is for integration checks only, not for + the tests and docs an earlier group owed. Example: ``` @@ -224,6 +231,7 @@ artifacts: - [ ] 2.1 Implement data export function and verify the export test passes - [ ] 2.2 Add CSV formatting utilities and verify unit tests cover quoting and delimiters + - [ ] 2.3 Document the export API in docs/export.md and verify the documented command runs as written ``` Reference specs for what needs to be built, design for how to build it. diff --git a/skills/openspec-onboard/SKILL.md b/skills/openspec-onboard/SKILL.md index 966dcff2ad..a78845afb2 100644 --- a/skills/openspec-onboard/SKILL.md +++ b/skills/openspec-onboard/SKILL.md @@ -378,7 +378,7 @@ Save to the `resolvedOutputPath` from `openspec instructions design --change " { const example = tasks!.instruction.match(/```\s*([\s\S]*?)```/)?.[1]; expect(example).toBeDefined(); const numberedTasks = example!.split('\n').filter(line => /^- \[ \] \d+\.\d+ /.test(line)); - expect(numberedTasks).toHaveLength(4); + expect(numberedTasks).toHaveLength(5); expect(numberedTasks.every(line => /\bverify\b/i.test(line))).toBe(true); expect(numberedTasks[0]).toContain('expected files are present'); expect(numberedTasks[1]).toContain('package installation succeeds'); expect(numberedTasks[2]).toContain('export test passes'); expect(numberedTasks[3]).toContain('unit tests cover quoting and delimiters'); + expect(numberedTasks[4]).toContain('Document the export API'); expect(example).not.toMatch(/^- \[ \] \d+\.\d+ (?:verify|run (?:the )?verification)\b/im); }); + + // #1952: agents parked testing and documentation in one trailing group, so a + // failure seeded in group 1 only surfaced at the end and cascaded into rework. + it('keeps tests and documentation inside the group that does the work (#1952)', () => { + const tasks = defaultSchema.artifacts.find(artifact => artifact.id === 'tasks'); + expect(tasks).toBeDefined(); + expect(tasks!.instruction).toMatch( + /Each task group MUST land the tests and documentation its own work\s+calls for/ + ); + expect(tasks!.instruction).toMatch( + /Do NOT collect testing or documentation into a final group/ + ); + // The rule is scoped to what a group's work actually needs, so the worked + // example's scaffolding group can carry no tests or docs without + // contradicting it. + expect(tasks!.instruction).toMatch( + /A group\s+whose work calls for neither, such as scaffolding or dependency setup,\s+carries neither/ + ); + expect(tasks!.instruction).toMatch( + /A final group is for integration checks only, not for\s+the tests and docs an earlier group owed/ + ); + + // The worked example has to show a docs task inside the implementation + // group, not a trailing "testing and documentation" group of its own. + const example = tasks!.instruction.match(/```\s*([\s\S]*?)```/)?.[1]; + expect(example).toBeDefined(); + const headings = example! + .split('\n') + .filter(line => /^## /.test(line.trim())) + .map(line => line.trim()); + expect(headings).toHaveLength(2); + expect(headings.some(heading => /\b(test|testing|documentation|docs)\b/i.test(heading))).toBe( + false + ); + + const lastGroup = example!.slice(example!.lastIndexOf(headings[headings.length - 1])); + expect(lastGroup).toMatch(/^- \[ \] \d+\.\d+ Document the export API in docs\/export\.md/im); + }); }); describe('propose project context', () => { diff --git a/test/core/templates/schema-docs-instruction-parity.test.ts b/test/core/templates/schema-docs-instruction-parity.test.ts new file mode 100644 index 0000000000..88a05b2491 --- /dev/null +++ b/test/core/templates/schema-docs-instruction-parity.test.ts @@ -0,0 +1,63 @@ +import * as fs from 'node:fs'; +import * as path from 'node:path'; +import { describe, expect, it } from 'vitest'; + +import { parseSchema } from '../../../src/core/artifact-graph/schema.js'; + +// The published schema reference quotes every `spec-driven` instruction +// verbatim ("The instruction sent to the agent when it drafts this +// artifact"), so a reader can see exactly what their agent is told. Nothing +// regenerated that page, and it drifted: the `specs` block predated the +// store-aware main-spec paths (#1703) and the `tasks` block still taught the +// pre-#1660 rules, so the site contradicted the shipped instruction. This +// keeps the quoted blocks byte-identical to schema.yaml. +const REPO_ROOT = path.join(__dirname, '..', '..', '..'); +const SCHEMA_DIR = path.join(REPO_ROOT, 'schemas', 'spec-driven'); +const DOC_PATH = path.join( + REPO_ROOT, + 'docs-lab', + 'reference', + 'schemas', + 'spec-driven', + 'index.md' +); + +const normalize = (value: string): string => value.replace(/\r\n?/g, '\n').trim(); + +// The fence length varies per block because an instruction may contain its own +// ``` example, so the outer fence has to be longer. +const QUOTED_INSTRUCTION = + /### Instructions\n\n[^\n]*\n\n(`{3,})md\n([\s\S]*?)\n\1\n/g; + +describe('published schema reference', () => { + it('quotes every spec-driven instruction verbatim (#1952)', () => { + const schema = parseSchema( + fs.readFileSync(path.join(SCHEMA_DIR, 'schema.yaml'), 'utf-8') + ); + const doc = fs.readFileSync(DOC_PATH, 'utf-8').replace(/\r\n?/g, '\n'); + + const quoted = [...doc.matchAll(QUOTED_INSTRUCTION)].map(match => match[2]); + const expected: Array<[string, string]> = [ + ...schema.artifacts.map( + (artifact): [string, string] => [artifact.id, artifact.instruction ?? ''] + ), + ['apply', schema.apply?.instruction ?? ''], + ]; + + expect(quoted).toHaveLength(expected.length); + expected.forEach(([id, instruction], index) => { + expect(instruction, `${id} has no instruction to quote`).not.toBe(''); + expect(normalize(quoted[index]), `${id} instruction is stale in ${path.basename(DOC_PATH)}`).toBe( + normalize(instruction) + ); + }); + }); + + it('keeps the tasks guidance on the published page (#1952)', () => { + const doc = fs.readFileSync(DOC_PATH, 'utf-8').replace(/\r\n?/g, '\n'); + expect(doc).toContain( + 'Each task group MUST land the tests and documentation its own work' + ); + expect(doc).toContain('Each task MUST state how to verify completion'); + }); +}); diff --git a/test/core/templates/skill-templates-parity.test.ts b/test/core/templates/skill-templates-parity.test.ts index 4ab2bd8cb2..5bbcb847c3 100644 --- a/test/core/templates/skill-templates-parity.test.ts +++ b/test/core/templates/skill-templates-parity.test.ts @@ -82,7 +82,7 @@ const EXPECTED_FUNCTION_HASHES: Record = { getApplyChangeSkillTemplate: '04ae407c97b5f9cb0cc15199fe877ccc7cd1eff78bfe10ad70c16a112b10a661', getFfChangeSkillTemplate: 'd091600476a815ba99f69b446bcd46af5bf73d1c2810215a0c6196937d019cf6', getSyncSpecsSkillTemplate: 'bc80fe9b07eaa289e5eb8a3ce65eb7df722a16d864e37283c678220712e4f230', - getOnboardSkillTemplate: '8f4bb13c097c7c2dfca714c3f51765039008d87f844e62186e066bebd7637376', + getOnboardSkillTemplate: '84258a06c0ca88de708a23dd74e9a17efe11eff63a071b3864c781dcd5a0a4b7', getOpsxExploreCommandTemplate: 'f6cf22825643281d653355745623a6c1a4566db46cc2f262d2282243c6d8169a', getOpsxNewCommandTemplate: '6d504fef1e0d4ced7c423f4cc9d9d2cee11b1a6224edf685e06a3f0757e0ebff', getOpsxContinueCommandTemplate: 'ace5c9cc239c12b57dc86fd9a1c02a6ca467cb8e1245127340c07ab1b9d37c11', @@ -93,7 +93,7 @@ const EXPECTED_FUNCTION_HASHES: Record = { getOpsxSyncCommandTemplate: '60550b7bb9829421656d6324a9e4c951bc912f48f88882d1a07ce7f78397a5e7', getVerifyChangeSkillTemplate: '2e069a277dac23818b13bb50b66e806ab405bc3b7f535400e1ebf81b84153699', getOpsxArchiveCommandTemplate: '980109e5f8362610872c70fe0a0f1d48d3d2692275b2b17e2f4c91c3de89c2fd', - getOpsxOnboardCommandTemplate: '35332b79e943daefd4118513f03dc48926267af60348f781b8d92bbe5086a986', + getOpsxOnboardCommandTemplate: '0cf66e164c0e14c916c6d1ebb5d80ded07d7fb8e55d4eb34eba43e8ca9c28558', getOpsxBulkArchiveCommandTemplate: '3db03eadb764abd74c8c180656c3f64a8b9a4971056c91624d38df3209d7b446', getOpsxVerifyCommandTemplate: '938f52f20fb9a3b811ea47314baac1034cd550e8ab363ae878ccba4b6329348f', getOpsxProposeSkillTemplate: '1aa2f2eb9c8cbc4dcab9d777bf8832b92ca04f9ef91d0494f1224a566aefdfe8', @@ -113,7 +113,7 @@ const EXPECTED_GENERATED_SKILL_CONTENT_HASHES: Record = { 'openspec-archive-change': '305a21a9c76a925055f3bdbaac504f208660ef6948d78f73928de166250609bf', 'openspec-bulk-archive-change': '4bd638a50111d2ee3a667752a2355ed513f770695b137b93fc28848ca7bf60d2', 'openspec-verify-change': 'ad8a3098bd27d852721687c47a12db7107ed8b8dfc7f071406bb19961652e7ee', - 'openspec-onboard': '526bb7f9b8ceb8670b600ce33b0a62fe268393b7e778e120c796316da3a3cc3d', + 'openspec-onboard': '6993eff867d97d485e080078f9dfb80e968e242f3b17a924eeb077715fd548fa', 'openspec-propose': '66e3395adf9f2d93a09e8ef1d20e4efb010e5e8d4811f2d42a9316e4d1ca5a8b', 'openspec-update-change': '19163b8c1b40ccdc0840019aa8005877a90a3a1cd9f7aadb87f76ccce1342f19', }; @@ -499,6 +499,25 @@ describe('skill templates split parity', () => { } }); + // #1952: the onboarding walkthrough is where a user first meets task groups, + // so it has to say the same thing the tasks instruction does - tests and docs + // belong to the group that did the work, not to a trailing catch-up group. + it('teaches per-group tests and docs in the onboarding walkthrough (#1952)', () => { + const variants: Array<[string, string]> = [ + ['onboard skill', generateSkillContent(asDeployed(getOnboardSkillTemplate()), 'PARITY-BASELINE')], + ['onboard command', getOpsxOnboardCommandTemplate().content], + ]; + + for (const [label, content] of variants) { + expect(content, label).toContain( + 'Each group carries the tests and documentation for its own work - the last group is only for integration checks.' + ); + // The trailing group stays integration-only; it must not be renamed back + // into a general testing/documentation bucket. + expect(content, label).toContain('## 2. Integration Verification'); + } + }); + it('generates no workspace-planning residue in any workflow template (4.1)', () => { const allSkills: Array<[string, () => SkillTemplate]> = [ ['openspec-apply-change', getApplyChangeSkillTemplate],