diff --git a/.changeset/verify-removed-requirements.md b/.changeset/verify-removed-requirements.md new file mode 100644 index 0000000000..bd40bbbc93 --- /dev/null +++ b/.changeset/verify-removed-requirements.md @@ -0,0 +1,5 @@ +--- +"@fission-ai/openspec": patch +--- + +Stop `/opsx:verify` from reporting a correctly removed requirement as missing. Verify now reads which delta section each requirement sits under: ADDED and MODIFIED requirements are checked for an implementation as before, a REMOVED requirement passes once its behavior is gone and is flagged only while it is still present, and the old name of a RENAMED requirement is no longer reported as missing. diff --git a/openspec/specs/opsx-verify-skill/spec.md b/openspec/specs/opsx-verify-skill/spec.md index 740b5002c8..8e69096ffd 100644 --- a/openspec/specs/opsx-verify-skill/spec.md +++ b/openspec/specs/opsx-verify-skill/spec.md @@ -50,9 +50,10 @@ The agent SHALL verify that all required work has been completed. #### Scenario: Spec coverage check - **WHEN** verifying completeness - **AND** delta specs exist in `openspec/changes//specs/` -- **THEN** the agent extracts all requirements from delta specs -- **AND** searches codebase for implementation of each requirement -- **AND** reports which requirements appear to have implementation vs which are missing +- **THEN** the agent extracts all requirements from delta specs, noting the delta section each one sits under +- **AND** searches codebase for implementation of each ADDED or MODIFIED requirement +- **AND** reports which ADDED or MODIFIED requirements appear to have implementation vs which are missing +- **AND** checks REMOVED and RENAMED requirements as described in the Removed requirement and Renamed requirement scenarios #### Scenario: All tasks complete - **WHEN** all tasks are marked complete @@ -72,14 +73,14 @@ The agent SHALL verify that implementation matches the specifications. #### Scenario: Requirement implementation mapping - **WHEN** verifying correctness -- **THEN** for each requirement in delta specs: +- **THEN** for each ADDED or MODIFIED requirement in delta specs: - Search codebase for implementation - Identify relevant files and line numbers - Assess whether implementation satisfies the requirement #### Scenario: Scenario coverage check - **WHEN** verifying correctness -- **THEN** for each scenario in delta specs: +- **THEN** for each scenario under an ADDED or MODIFIED requirement in delta specs: - Check if the scenario's conditions are handled in code - Check if tests exist that cover the scenario - Report coverage status @@ -96,10 +97,33 @@ The agent SHALL verify that implementation matches the specifications. - **AND** suggest: either update implementation or update spec to match reality #### Scenario: Missing implementation -- **WHEN** no implementation found for a requirement +- **WHEN** no implementation found for an ADDED or MODIFIED requirement - **THEN** report as CRITICAL issue - **AND** suggest: "Implement requirement X" with guidance on what's needed +#### Scenario: Removed requirement +- **WHEN** a requirement sits under `## REMOVED Requirements` in a delta spec +- **THEN** the agent treats the absence of its implementation as the expected result +- **AND** does not report it as missing or suggest implementing it +- **AND** reports it as CRITICAL only if the removed behavior is still present in the codebase +- **AND** skips scenario coverage for it +- **AND** does not treat matches in OpenSpec artifacts or docs, or in code that serves only the Migration note or an ADDED requirement, as evidence by themselves +- **AND** still reports a code path that delivers the removed behavior, even when it is shared with an ADDED requirement + +#### Scenario: Renamed requirement +- **WHEN** a requirement is listed under `## RENAMED Requirements` in a delta spec +- **THEN** the agent does not report its FROM name as missing +- **AND** does not require code symbols or file names to be renamed +- **AND** unless the TO name also appears under MODIFIED, verifies that the behavior of the baseline requirement (its body and scenarios in the main spec, under the FROM name, or under the TO name only when the main spec is already synced) is still implemented +- **AND** reports CRITICAL "Renamed requirement not found" when that behavior is missing +- **AND** marks spec coverage as not verified for the entry when the baseline requirement cannot be found or read + +#### Scenario: Change that only removes or renames requirements +- **WHEN** the delta specs are readable and contain at least one REMOVED or RENAMED requirement but no ADDED or MODIFIED requirements +- **THEN** the agent reports requirement implementation mapping and scenario coverage as not applicable +- **AND** does not mark them as not verified or withhold readiness because of them +- **AND** a delta spec with no parseable requirements still marks them as not verified + ### Requirement: Coherence Verification The agent SHALL verify that implementation is sensible and follows design decisions. diff --git a/skills/openspec-verify-change/SKILL.md b/skills/openspec-verify-change/SKILL.md index d5f9d293f8..41be312432 100644 --- a/skills/openspec-verify-change/SKILL.md +++ b/skills/openspec-verify-change/SKILL.md @@ -71,7 +71,7 @@ In both branches, never create the root as a side effect: do not run `openspec i Verification is advisory. Respect intentional omissions such as `skip_specs: true`, optional design documents, and schemas without task tracking. Do not require or invent optional or intentionally omitted artifacts to obtain a clean report. `Not verified` describes a limit of this report, not a new archive prerequisite. Archive retains its own checks and user-confirmation behavior. - Mark checks the schema does not define, or artifacts the status reports as intentionally skipped, as **Not applicable**. Exclude them from skipped-check counts and the archive-readiness assessment. Reserve **Not verified** for applicable checks whose evidence is missing or unusable. + Mark checks the schema does not define, or artifacts the status reports as intentionally skipped, as **Not applicable**. The correctness checks of a change whose readable delta specs contain REMOVED or RENAMED requirements but no ADDED or MODIFIED requirements are also **Not applicable** (see step 6). Exclude them from skipped-check counts and the archive-readiness assessment. Reserve **Not verified** for applicable checks whose evidence is missing or unusable. If only task evidence is available for applicable checks, verify task completion only and mark the remaining applicable checks, including **Code Pattern Consistency**, as not verified with the reason "Only task evidence available". @@ -93,18 +93,35 @@ In both branches, never create the root as a side effect: do not run `openspec i - If status marks the spec artifact skipped by `skip_specs: true`, or the schema defines no spec artifact, report the spec-dependent checks as not applicable. - Otherwise, `contextFiles` is keyed by artifact id, and artifact ids come from the active schema. If `contextFiles.specs` is absent or empty, mark **Spec Coverage**, **Requirement Implementation Mapping**, and **Scenario Coverage** as not verified; do not treat any of them as clean. - If delta specs exist in `contextFiles.specs`: - - Extract all requirements (marked with "### Requirement:") - - For each requirement: + - Extract all requirements (marked with "### Requirement:", or listed as `FROM:`/`TO:` pairs under `## RENAMED Requirements`) and note the delta section each one sits under: `## ADDED`, `## MODIFIED`, `## REMOVED`, or `## RENAMED Requirements`. The section decides what the check looks for. + - For each ADDED or MODIFIED requirement (for MODIFIED, check the text in the delta, not the old wording): - Search codebase for keywords related to the requirement - Assess if implementation likely exists - - If requirements appear unimplemented: + - If ADDED or MODIFIED requirements appear unimplemented: - Add CRITICAL issue: "Requirement not found: " - Recommendation: "Implement requirement X: " + - For each REMOVED requirement, the change asks for the behavior to be gone, so invert the check: + - Search codebase for the removed behavior. Matches in `openspec/` artifacts or docs, or in code that serves only the Migration note or an ADDED requirement, are not evidence by themselves. Report any code path that still delivers the removed behavior, including one shared with an ADDED requirement. + - Finding no implementation is the expected result. Never report a REMOVED requirement as "Requirement not found" or recommend implementing it. + - If the behavior is still present: + - Add CRITICAL issue: "Removed requirement still implemented: " + - Recommendation: "Remove the remaining implementation at :, following the requirement's Migration note if it has one" + - For each RENAMED entry (`FROM:`/`TO:`), the name changes but the behavior stays, so check the TO requirement for that unchanged behavior: + - Do not report the FROM name as missing, and do not require code symbols, identifiers, or file names to be renamed. + - If the TO name also appears under MODIFIED, its behavior is checked there against the MODIFIED text; skip it here. + - Otherwise, read the baseline requirement in the main spec at `/openspec/specs//spec.md`, using the same capability path as the delta spec: the requirement under the FROM name, or under the TO name only when the FROM name is absent because the main spec is already synced. Its body and scenarios are the evidence for the behavior the TO requirement keeps. + - Search codebase for that behavior and assess if it is still implemented. + - If it appears unimplemented: + - Add CRITICAL issue: "Renamed requirement not found: " + - Recommendation: "Restore the behavior of (renamed from ); a rename must not change behavior" + - If the baseline requirement cannot be found or read, mark **Spec Coverage** as not verified for that entry with the reason. Never count an unchecked rename as passing. 6. **Verify Correctness** + If the delta specs are readable and contain at least one REMOVED or RENAMED requirement but no ADDED or MODIFIED requirements (the change only removes or renames requirements), report **Requirement Implementation Mapping** and **Scenario Coverage** as **Not applicable**. The REMOVED and RENAMED checks under Spec Coverage are the evidence for such a change (each RENAMED entry is checked there against its baseline behavior), so do not mark these two checks as not verified. A delta spec with no parseable requirements at all is unusable evidence, not a removal-only change: mark these checks as not verified. + **Requirement Implementation Mapping**: - - For each requirement from delta specs: + - For each ADDED or MODIFIED requirement from delta specs (REMOVED entries, and RENAMED entries without a MODIFIED block, were settled under Spec Coverage): - Search codebase for implementation evidence - If found, note file paths and line ranges - Assess if implementation matches requirement intent @@ -113,12 +130,13 @@ In both branches, never create the root as a side effect: do not run `openspec i - Recommendation: "Review : against requirement X" **Scenario Coverage**: - - For each scenario in delta specs (marked with "#### Scenario:"): + - For each scenario under an ADDED or MODIFIED requirement in delta specs (marked with "#### Scenario:"): - Check if conditions are handled in code - Check if tests exist covering the scenario - If scenario appears uncovered: - Add WARNING: "Scenario not covered: " - Recommendation: "Add test or implementation for scenario: " + - Skip scenarios under a REMOVED requirement; that behavior is meant to be gone. 7. **Verify Coherence** @@ -154,13 +172,15 @@ In both branches, never create the root as a side effect: do not run `openspec i | Coherence | Followed/Issues | ``` - In each Status cell, report the results of checks that ran and `Not verified ()` for every skipped check. If all checks in a dimension were skipped, start the cell with `Not verified`. Never score a skipped check as passing. Treat every not verified or partially verified check as skipped in the final assessment. + In each Status cell, report the results of checks that ran and `Not verified ()` for every skipped check. If all checks in a dimension were skipped, start the cell with `Not verified`. Never score a skipped check as passing. Treat every not verified or partially verified check as skipped in the final assessment. Count only ADDED and MODIFIED requirements in N, and report REMOVED and RENAMED requirements separately (for example, "1 removal confirmed, 1 rename verified"). For a change that only removes or renames requirements, the Correctness cell reads `Not applicable (no ADDED or MODIFIED requirements)`. **Issues by Priority**: 1. **CRITICAL** (Must fix before archive): - Incomplete tasks - Missing requirement implementations + - Removed requirements still implemented + - Renamed requirements whose behavior is no longer implemented - Each with specific, actionable recommendation 2. **WARNING** (Should fix): diff --git a/src/core/templates/workflows/verify-change.ts b/src/core/templates/workflows/verify-change.ts index 92c4b0cc2b..544e1da608 100644 --- a/src/core/templates/workflows/verify-change.ts +++ b/src/core/templates/workflows/verify-change.ts @@ -65,7 +65,7 @@ ${PROJECT_ROOT_GUARD} Verification is advisory. Respect intentional omissions such as \`skip_specs: true\`, optional design documents, and schemas without task tracking. Do not require or invent optional or intentionally omitted artifacts to obtain a clean report. \`Not verified\` describes a limit of this report, not a new archive prerequisite. Archive retains its own checks and user-confirmation behavior. - Mark checks the schema does not define, or artifacts the status reports as intentionally skipped, as **Not applicable**. Exclude them from skipped-check counts and the archive-readiness assessment. Reserve **Not verified** for applicable checks whose evidence is missing or unusable. + Mark checks the schema does not define, or artifacts the status reports as intentionally skipped, as **Not applicable**. The correctness checks of a change whose readable delta specs contain REMOVED or RENAMED requirements but no ADDED or MODIFIED requirements are also **Not applicable** (see step 6). Exclude them from skipped-check counts and the archive-readiness assessment. Reserve **Not verified** for applicable checks whose evidence is missing or unusable. If only task evidence is available for applicable checks, verify task completion only and mark the remaining applicable checks, including **Code Pattern Consistency**, as not verified with the reason "Only task evidence available". @@ -87,18 +87,35 @@ ${PROJECT_ROOT_GUARD} - If status marks the spec artifact skipped by \`skip_specs: true\`, or the schema defines no spec artifact, report the spec-dependent checks as not applicable. - Otherwise, \`contextFiles\` is keyed by artifact id, and artifact ids come from the active schema. If \`contextFiles.specs\` is absent or empty, mark **Spec Coverage**, **Requirement Implementation Mapping**, and **Scenario Coverage** as not verified; do not treat any of them as clean. - If delta specs exist in \`contextFiles.specs\`: - - Extract all requirements (marked with "### Requirement:") - - For each requirement: + - Extract all requirements (marked with "### Requirement:", or listed as \`FROM:\`/\`TO:\` pairs under \`## RENAMED Requirements\`) and note the delta section each one sits under: \`## ADDED\`, \`## MODIFIED\`, \`## REMOVED\`, or \`## RENAMED Requirements\`. The section decides what the check looks for. + - For each ADDED or MODIFIED requirement (for MODIFIED, check the text in the delta, not the old wording): - Search codebase for keywords related to the requirement - Assess if implementation likely exists - - If requirements appear unimplemented: + - If ADDED or MODIFIED requirements appear unimplemented: - Add CRITICAL issue: "Requirement not found: " - Recommendation: "Implement requirement X: " + - For each REMOVED requirement, the change asks for the behavior to be gone, so invert the check: + - Search codebase for the removed behavior. Matches in \`openspec/\` artifacts or docs, or in code that serves only the Migration note or an ADDED requirement, are not evidence by themselves. Report any code path that still delivers the removed behavior, including one shared with an ADDED requirement. + - Finding no implementation is the expected result. Never report a REMOVED requirement as "Requirement not found" or recommend implementing it. + - If the behavior is still present: + - Add CRITICAL issue: "Removed requirement still implemented: " + - Recommendation: "Remove the remaining implementation at :, following the requirement's Migration note if it has one" + - For each RENAMED entry (\`FROM:\`/\`TO:\`), the name changes but the behavior stays, so check the TO requirement for that unchanged behavior: + - Do not report the FROM name as missing, and do not require code symbols, identifiers, or file names to be renamed. + - If the TO name also appears under MODIFIED, its behavior is checked there against the MODIFIED text; skip it here. + - Otherwise, read the baseline requirement in the main spec at \`/openspec/specs//spec.md\`, using the same capability path as the delta spec: the requirement under the FROM name, or under the TO name only when the FROM name is absent because the main spec is already synced. Its body and scenarios are the evidence for the behavior the TO requirement keeps. + - Search codebase for that behavior and assess if it is still implemented. + - If it appears unimplemented: + - Add CRITICAL issue: "Renamed requirement not found: " + - Recommendation: "Restore the behavior of (renamed from ); a rename must not change behavior" + - If the baseline requirement cannot be found or read, mark **Spec Coverage** as not verified for that entry with the reason. Never count an unchecked rename as passing. 6. **Verify Correctness** + If the delta specs are readable and contain at least one REMOVED or RENAMED requirement but no ADDED or MODIFIED requirements (the change only removes or renames requirements), report **Requirement Implementation Mapping** and **Scenario Coverage** as **Not applicable**. The REMOVED and RENAMED checks under Spec Coverage are the evidence for such a change (each RENAMED entry is checked there against its baseline behavior), so do not mark these two checks as not verified. A delta spec with no parseable requirements at all is unusable evidence, not a removal-only change: mark these checks as not verified. + **Requirement Implementation Mapping**: - - For each requirement from delta specs: + - For each ADDED or MODIFIED requirement from delta specs (REMOVED entries, and RENAMED entries without a MODIFIED block, were settled under Spec Coverage): - Search codebase for implementation evidence - If found, note file paths and line ranges - Assess if implementation matches requirement intent @@ -107,12 +124,13 @@ ${PROJECT_ROOT_GUARD} - Recommendation: "Review : against requirement X" **Scenario Coverage**: - - For each scenario in delta specs (marked with "#### Scenario:"): + - For each scenario under an ADDED or MODIFIED requirement in delta specs (marked with "#### Scenario:"): - Check if conditions are handled in code - Check if tests exist covering the scenario - If scenario appears uncovered: - Add WARNING: "Scenario not covered: " - Recommendation: "Add test or implementation for scenario: " + - Skip scenarios under a REMOVED requirement; that behavior is meant to be gone. 7. **Verify Coherence** @@ -148,13 +166,15 @@ ${PROJECT_ROOT_GUARD} | Coherence | Followed/Issues | \`\`\` - In each Status cell, report the results of checks that ran and \`Not verified ()\` for every skipped check. If all checks in a dimension were skipped, start the cell with \`Not verified\`. Never score a skipped check as passing. Treat every not verified or partially verified check as skipped in the final assessment. + In each Status cell, report the results of checks that ran and \`Not verified ()\` for every skipped check. If all checks in a dimension were skipped, start the cell with \`Not verified\`. Never score a skipped check as passing. Treat every not verified or partially verified check as skipped in the final assessment. Count only ADDED and MODIFIED requirements in N, and report REMOVED and RENAMED requirements separately (for example, "1 removal confirmed, 1 rename verified"). For a change that only removes or renames requirements, the Correctness cell reads \`Not applicable (no ADDED or MODIFIED requirements)\`. **Issues by Priority**: 1. **CRITICAL** (Must fix before archive): - Incomplete tasks - Missing requirement implementations + - Removed requirements still implemented + - Renamed requirements whose behavior is no longer implemented - Each with specific, actionable recommendation 2. **WARNING** (Should fix): @@ -255,7 +275,7 @@ ${PROJECT_ROOT_GUARD} Each dimension can have CRITICAL, WARNING, or SUGGESTION issues. Verification is advisory. Respect intentional omissions such as \`skip_specs: true\`, optional design documents, and schemas without task tracking. Do not require or invent optional or intentionally omitted artifacts to obtain a clean report. \`Not verified\` describes a limit of this report, not a new archive prerequisite. Archive retains its own checks and user-confirmation behavior. - Mark checks the schema does not define, or artifacts the status reports as intentionally skipped, as **Not applicable**. Exclude them from skipped-check counts and the archive-readiness assessment. Reserve **Not verified** for applicable checks whose evidence is missing or unusable. + Mark checks the schema does not define, or artifacts the status reports as intentionally skipped, as **Not applicable**. The correctness checks of a change whose readable delta specs contain REMOVED or RENAMED requirements but no ADDED or MODIFIED requirements are also **Not applicable** (see step 6). Exclude them from skipped-check counts and the archive-readiness assessment. Reserve **Not verified** for applicable checks whose evidence is missing or unusable. If only task evidence is available for applicable checks, verify task completion only and mark the remaining applicable checks, including **Code Pattern Consistency**, as not verified with the reason "Only task evidence available". @@ -277,18 +297,35 @@ ${PROJECT_ROOT_GUARD} - If status marks the spec artifact skipped by \`skip_specs: true\`, or the schema defines no spec artifact, report the spec-dependent checks as not applicable. - Otherwise, \`contextFiles\` is keyed by artifact id, and artifact ids come from the active schema. If \`contextFiles.specs\` is absent or empty, mark **Spec Coverage**, **Requirement Implementation Mapping**, and **Scenario Coverage** as not verified; do not treat any of them as clean. - If delta specs exist in \`contextFiles.specs\`: - - Extract all requirements (marked with "### Requirement:") - - For each requirement: + - Extract all requirements (marked with "### Requirement:", or listed as \`FROM:\`/\`TO:\` pairs under \`## RENAMED Requirements\`) and note the delta section each one sits under: \`## ADDED\`, \`## MODIFIED\`, \`## REMOVED\`, or \`## RENAMED Requirements\`. The section decides what the check looks for. + - For each ADDED or MODIFIED requirement (for MODIFIED, check the text in the delta, not the old wording): - Search codebase for keywords related to the requirement - Assess if implementation likely exists - - If requirements appear unimplemented: + - If ADDED or MODIFIED requirements appear unimplemented: - Add CRITICAL issue: "Requirement not found: " - Recommendation: "Implement requirement X: " + - For each REMOVED requirement, the change asks for the behavior to be gone, so invert the check: + - Search codebase for the removed behavior. Matches in \`openspec/\` artifacts or docs, or in code that serves only the Migration note or an ADDED requirement, are not evidence by themselves. Report any code path that still delivers the removed behavior, including one shared with an ADDED requirement. + - Finding no implementation is the expected result. Never report a REMOVED requirement as "Requirement not found" or recommend implementing it. + - If the behavior is still present: + - Add CRITICAL issue: "Removed requirement still implemented: " + - Recommendation: "Remove the remaining implementation at :, following the requirement's Migration note if it has one" + - For each RENAMED entry (\`FROM:\`/\`TO:\`), the name changes but the behavior stays, so check the TO requirement for that unchanged behavior: + - Do not report the FROM name as missing, and do not require code symbols, identifiers, or file names to be renamed. + - If the TO name also appears under MODIFIED, its behavior is checked there against the MODIFIED text; skip it here. + - Otherwise, read the baseline requirement in the main spec at \`/openspec/specs//spec.md\`, using the same capability path as the delta spec: the requirement under the FROM name, or under the TO name only when the FROM name is absent because the main spec is already synced. Its body and scenarios are the evidence for the behavior the TO requirement keeps. + - Search codebase for that behavior and assess if it is still implemented. + - If it appears unimplemented: + - Add CRITICAL issue: "Renamed requirement not found: " + - Recommendation: "Restore the behavior of (renamed from ); a rename must not change behavior" + - If the baseline requirement cannot be found or read, mark **Spec Coverage** as not verified for that entry with the reason. Never count an unchecked rename as passing. 6. **Verify Correctness** + If the delta specs are readable and contain at least one REMOVED or RENAMED requirement but no ADDED or MODIFIED requirements (the change only removes or renames requirements), report **Requirement Implementation Mapping** and **Scenario Coverage** as **Not applicable**. The REMOVED and RENAMED checks under Spec Coverage are the evidence for such a change (each RENAMED entry is checked there against its baseline behavior), so do not mark these two checks as not verified. A delta spec with no parseable requirements at all is unusable evidence, not a removal-only change: mark these checks as not verified. + **Requirement Implementation Mapping**: - - For each requirement from delta specs: + - For each ADDED or MODIFIED requirement from delta specs (REMOVED entries, and RENAMED entries without a MODIFIED block, were settled under Spec Coverage): - Search codebase for implementation evidence - If found, note file paths and line ranges - Assess if implementation matches requirement intent @@ -297,12 +334,13 @@ ${PROJECT_ROOT_GUARD} - Recommendation: "Review : against requirement X" **Scenario Coverage**: - - For each scenario in delta specs (marked with "#### Scenario:"): + - For each scenario under an ADDED or MODIFIED requirement in delta specs (marked with "#### Scenario:"): - Check if conditions are handled in code - Check if tests exist covering the scenario - If scenario appears uncovered: - Add WARNING: "Scenario not covered: " - Recommendation: "Add test or implementation for scenario: " + - Skip scenarios under a REMOVED requirement; that behavior is meant to be gone. 7. **Verify Coherence** @@ -338,13 +376,15 @@ ${PROJECT_ROOT_GUARD} | Coherence | Followed/Issues | \`\`\` - In each Status cell, report the results of checks that ran and \`Not verified ()\` for every skipped check. If all checks in a dimension were skipped, start the cell with \`Not verified\`. Never score a skipped check as passing. Treat every not verified or partially verified check as skipped in the final assessment. + In each Status cell, report the results of checks that ran and \`Not verified ()\` for every skipped check. If all checks in a dimension were skipped, start the cell with \`Not verified\`. Never score a skipped check as passing. Treat every not verified or partially verified check as skipped in the final assessment. Count only ADDED and MODIFIED requirements in N, and report REMOVED and RENAMED requirements separately (for example, "1 removal confirmed, 1 rename verified"). For a change that only removes or renames requirements, the Correctness cell reads \`Not applicable (no ADDED or MODIFIED requirements)\`. **Issues by Priority**: 1. **CRITICAL** (Must fix before archive): - Incomplete tasks - Missing requirement implementations + - Removed requirements still implemented + - Renamed requirements whose behavior is no longer implemented - Each with specific, actionable recommendation 2. **WARNING** (Should fix): diff --git a/test/core/templates/skill-templates-parity.test.ts b/test/core/templates/skill-templates-parity.test.ts index 03fda1ae79..80d2670195 100644 --- a/test/core/templates/skill-templates-parity.test.ts +++ b/test/core/templates/skill-templates-parity.test.ts @@ -91,11 +91,11 @@ const EXPECTED_FUNCTION_HASHES: Record = { getArchiveChangeSkillTemplate: '71715f9d5899498942af03e182e6d1ac2c95952dde967950c2a9161084a53a8b', getBulkArchiveChangeSkillTemplate: '2a6ec08fea0f942158b4abe9c8d1af9622038e0c4dc684e7c73dad2fb8379a54', getOpsxSyncCommandTemplate: '60550b7bb9829421656d6324a9e4c951bc912f48f88882d1a07ce7f78397a5e7', - getVerifyChangeSkillTemplate: '40d955c703bd319c3d7ecbf8c6da3a04d7c85746587844ecc60656a89a2f47b3', + getVerifyChangeSkillTemplate: 'eecb063792075191b613978dec45f9f2fee247d2ff3003f2ebf17d632e54352e', getOpsxArchiveCommandTemplate: '3d2a330b46043fbb9f220831aa42ebbb62f411e9597b2bb491ad1ac1fa2d0873', getOpsxOnboardCommandTemplate: '0cf66e164c0e14c916c6d1ebb5d80ded07d7fb8e55d4eb34eba43e8ca9c28558', getOpsxBulkArchiveCommandTemplate: '4e2e39c4d634074f4a1ed67f076d5c4d0ead8b998f4d75218c33cdc6173719be', - getOpsxVerifyCommandTemplate: '9c8219820ee83a72d180a94c7504457a5fa54670386be266f8247cccd5c68fce', + getOpsxVerifyCommandTemplate: 'f47bc0c30cfa8e93b5e42026e9417636c5f15bd8505fb9138872e34af8906abb', getOpsxProposeSkillTemplate: '1aa2f2eb9c8cbc4dcab9d777bf8832b92ca04f9ef91d0494f1224a566aefdfe8', getOpsxProposeCommandTemplate: '3b7090ce5e79e879ab9b5bdaf4ff2b52e3c02211f71188838772d36ac337f96c', getFeedbackSkillTemplate: 'dabeb5e825b9349abc8156c3e7b8608f27987912a6d9bf47ef29addde6138133', @@ -112,7 +112,7 @@ const EXPECTED_GENERATED_SKILL_CONTENT_HASHES: Record = { 'openspec-sync-specs': '3909936a236a21a9a6d5bf495f90b396b3b68fc9220d7b2c1894668653beb2e4', 'openspec-archive-change': 'd01d9eeb06223ee89708b7963e82c5ebc11719c5b2dc62d4abb268ee016fcb7b', 'openspec-bulk-archive-change': '10f050ad5ef77084dc55a202427988b23903ee122f00985238a4eb9354a5dc3c', - 'openspec-verify-change': '1a9493b623da4f399ceac0be105c7a7248db421e83eb4c8b983ba3692c4a7453', + 'openspec-verify-change': '62c2d471a1ebc4be38df0d06393eb94d3d8b803719b6349b8a1d8e9231448275', 'openspec-onboard': '6993eff867d97d485e080078f9dfb80e968e242f3b17a924eeb077715fd548fa', 'openspec-propose': '66e3395adf9f2d93a09e8ef1d20e4efb010e5e8d4811f2d42a9316e4d1ca5a8b', 'openspec-update-change': '5f4ea19aa732b33d87a2120ec393ee34578e70678d97e8c3bb10f988c00cb4d3', diff --git a/test/core/templates/verify-change-delta-operations.test.ts b/test/core/templates/verify-change-delta-operations.test.ts new file mode 100644 index 0000000000..b0a1a19fe2 --- /dev/null +++ b/test/core/templates/verify-change-delta-operations.test.ts @@ -0,0 +1,129 @@ +import { readFileSync } from 'node:fs'; + +import { describe, expect, it } from 'vitest'; + +import { + getVerifyChangeSkillTemplate, + getOpsxVerifyCommandTemplate, +} from '../../../src/core/templates/skill-templates.js'; + +// #1959: verify treated every "### Requirement:" in a change's delta specs as +// behavior that must exist, whichever section it sat under. A REMOVED +// requirement that was removed correctly came back as CRITICAL "Requirement not +// found" with the recommendation to implement it, so an agent following the +// report restored what the change had just deleted. +const bodies: Array<[string, string]> = [ + ['skill', getVerifyChangeSkillTemplate().instructions], + ['command', getOpsxVerifyCommandTemplate().content], + // The committed skills.sh mirror is what `npx skills add` installs. + [ + 'committed skill file', + readFileSync(new URL('../../../skills/openspec-verify-change/SKILL.md', import.meta.url), 'utf8'), + ], +]; + +function section(body: string, start: string, end: string, label: string): string { + const from = body.indexOf(start); + const to = body.indexOf(end, from + start.length); + expect(from, `${label}: "${start}" not found`).toBeGreaterThanOrEqual(0); + expect(to, `${label}: "${end}" not found after "${start}"`).toBeGreaterThan(from); + return body.slice(from, to); +} + +describe('verify checks each requirement by its delta operation', () => { + it.each(bodies)('%s: classifies requirements by delta section before checking them', (label, body) => { + const coverage = section(body, '**Spec Coverage**', '6. **Verify Correctness**', label); + + // RENAMED entries carry no "### Requirement:" heading, so a rename-only + // delta must not read as empty. + expect(coverage, label).toContain('or listed as `FROM:`/`TO:` pairs under `## RENAMED Requirements`'); + for (const header of ['## ADDED', '## MODIFIED', '## REMOVED', '## RENAMED Requirements']) { + expect(coverage, label).toContain(header); + } + // The unscoped loop is what produced the bug. + expect(coverage, label).not.toMatch(/^\s*- For each requirement:$/m); + }); + + it.each(bodies)('%s: reports a missing requirement only for ADDED or MODIFIED', (label, body) => { + const coverage = section(body, '**Spec Coverage**', '6. **Verify Correctness**', label); + const addedOrModified = section(coverage, '- For each ADDED or MODIFIED requirement', '- For each REMOVED requirement', label); + + expect(addedOrModified, label).toContain('Add CRITICAL issue: "Requirement not found: "'); + }); + + it.each(bodies)('%s: inverts the check for a REMOVED requirement', (label, body) => { + const coverage = section(body, '**Spec Coverage**', '6. **Verify Correctness**', label); + const removed = section(coverage, '- For each REMOVED requirement', '- For each RENAMED entry', label); + + expect(removed, label).toContain('Finding no implementation is the expected result.'); + expect(removed, label).toContain('Never report a REMOVED requirement as "Requirement not found"'); + expect(removed, label).toContain('Add CRITICAL issue: "Removed requirement still implemented: "'); + expect(removed, label).not.toContain('Recommendation: "Implement'); + }); + + it.each(bodies)('%s: does not report the old name of a RENAMED requirement as missing', (label, body) => { + const renamed = section(body, '- For each RENAMED entry', '6. **Verify Correctness**', label); + + expect(renamed, label).toContain('Do not report the FROM name as missing'); + expect(renamed, label).toContain('do not require code symbols, identifiers, or file names to be renamed'); + }); + + // A rename keeps behavior, so a rename-only change must still prove the + // behavior exists before verify can call it ready. Its evidence is the + // baseline requirement in the main spec, not the RENAMED entry itself. + it.each(bodies)('%s: verifies the unchanged behavior of a RENAMED requirement against its baseline', (label, body) => { + const renamed = section(body, '- For each RENAMED entry', '6. **Verify Correctness**', label); + + expect(renamed, label).toContain('check the TO requirement for that unchanged behavior'); + expect(renamed, label).toContain('`/openspec/specs//spec.md`'); + expect(renamed, label).toContain('the requirement under the FROM name, or under the TO name only when the FROM name is absent because the main spec is already synced.'); + expect(renamed, label).toContain('Its body and scenarios are the evidence for the behavior the TO requirement keeps.'); + expect(renamed, label).toContain('Search codebase for that behavior and assess if it is still implemented.'); + expect(renamed, label).toContain('Add CRITICAL issue: "Renamed requirement not found: "'); + expect(body, label).toContain('- Renamed requirements whose behavior is no longer implemented'); + expect(renamed, label).toContain('If the TO name also appears under MODIFIED, its behavior is checked there'); + }); + + it.each(bodies)('%s: never counts an unchecked rename as passing', (label, body) => { + const renamed = section(body, '- For each RENAMED entry', '6. **Verify Correctness**', label); + + expect(renamed, label).toContain('If the baseline requirement cannot be found or read, mark **Spec Coverage** as not verified for that entry'); + expect(renamed, label).toContain('Never count an unchecked rename as passing.'); + expect(body, label).toContain('(each RENAMED entry is checked there against its baseline behavior)'); + }); + + it.each(bodies)('%s: maps implementation and scenarios only for ADDED or MODIFIED requirements', (label, body) => { + const correctness = section(body, '6. **Verify Correctness**', '7. **Verify Coherence**', label); + + expect(correctness, label).toContain('- For each ADDED or MODIFIED requirement from delta specs'); + expect(correctness, label).toContain('- For each scenario under an ADDED or MODIFIED requirement in delta specs'); + expect(correctness, label).toContain('Skip scenarios under a REMOVED requirement'); + expect(correctness, label).not.toContain('- For each requirement from delta specs:'); + expect(correctness, label).not.toContain('- For each scenario in delta specs'); + }); + // With #1732's "Not verified" rule, a change with nothing to add or modify + // left both correctness checks empty, which read as unverified and withheld + // readiness. That is the exact case #1959 reports. + it.each(bodies)('%s: treats the correctness checks of a removal-only change as not applicable', (label, body) => { + const correctness = section(body, '6. **Verify Correctness**', '**Requirement Implementation Mapping**:', label); + + expect(correctness, label).toContain('If the delta specs are readable and contain at least one REMOVED or RENAMED requirement but no ADDED or MODIFIED requirements'); + // An empty or unparseable delta must not pass as a removal-only change. + expect(correctness, label).toContain('A delta spec with no parseable requirements at all is unusable evidence, not a removal-only change: mark these checks as not verified.'); + expect(correctness, label).toContain('report **Requirement Implementation Mapping** and **Scenario Coverage** as **Not applicable**'); + expect(correctness, label).toContain('do not mark these two checks as not verified'); + expect(body, label).toContain('The correctness checks of a change whose readable delta specs contain REMOVED or RENAMED requirements but no ADDED or MODIFIED requirements are also **Not applicable** (see step 6).'); + }); + + it.each(bodies)('%s: does not treat artifacts or replacement code as the removed behavior', (label, body) => { + const removed = section(body, '- For each REMOVED requirement', '- For each RENAMED entry', label); + + expect(removed, label).toContain('Matches in `openspec/` artifacts or docs, or in code that serves only the Migration note or an ADDED requirement, are not evidence by themselves.'); + expect(removed, label).toContain('Report any code path that still delivers the removed behavior, including one shared with an ADDED requirement.'); + }); + + it.each(bodies)('%s: counts removals separately from covered requirements', (label, body) => { + expect(body, label).toContain('Count only ADDED and MODIFIED requirements in N, and report REMOVED and RENAMED requirements separately'); + expect(body, label).toContain('the Correctness cell reads `Not applicable (no ADDED or MODIFIED requirements)`'); + }); +});