diff --git a/.changeset/archive-waits-for-spec-sync.md b/.changeset/archive-waits-for-spec-sync.md new file mode 100644 index 0000000000..21e6f663b2 --- /dev/null +++ b/.changeset/archive-waits-for-spec-sync.md @@ -0,0 +1,7 @@ +--- +"@fission-ai/openspec": patch +--- + +### Bug Fixes + +- **Archive no longer races the spec sync, or reports a sync that never landed** — the generated `openspec-archive-change` skill (and the matching `opsx:archive` command) handed the spec sync to a background task and then moved the change folder immediately. The archive could move the delta specs out from under the running sync: the change ended up archived, `openspec/specs/` was never updated, and the summary still reported `Specs: ✓ Synced`. The sync now runs inline, and the archive only proceeds once every capability with a delta spec has been checked against it — ADDED present, MODIFIED changes applied, REMOVED gone, RENAMED under the new name and not the old. If the sync fails or a capability doesn't match, the archive stops and reports what differs instead of claiming success; nothing has moved, so you can fix it and retry. diff --git a/openspec/specs/opsx-archive-skill/spec.md b/openspec/specs/opsx-archive-skill/spec.md index 2dbb04c529..95ba9dc2d0 100644 --- a/openspec/specs/opsx-archive-skill/spec.md +++ b/openspec/specs/opsx-archive-skill/spec.md @@ -74,8 +74,11 @@ The skill SHALL prompt to sync delta specs before archiving if specs exist. - **WHEN** agent checks for delta specs - **AND** `specs/` directory exists in the change with spec files - **THEN** prompt user: "This change has delta specs. Would you like to sync them to main specs before archiving?" -- **AND** if user confirms, execute `/opsx:sync` logic -- **AND** proceed with archive regardless of sync choice +- **AND** if user cancels, stop without archiving +- **AND** if user confirms, execute `/opsx:sync` logic inline and wait for it to complete +- **AND** verify every capability that has a delta spec, not only those the sync reports it touched: ADDED requirements present, MODIFIED requirements carrying the changes named in the delta, REMOVED requirements absent, RENAMED requirements present under the new name and absent under the old one +- **AND** stop without archiving if the sync fails or any capability does not verify +- **AND** archive only after verification passes, or when the user explicitly chose to archive without syncing or to archive already-synced specs #### Scenario: No delta specs diff --git a/skills/openspec-archive-change/SKILL.md b/skills/openspec-archive-change/SKILL.md index 3c5cd0fadc..e198c0099c 100644 --- a/skills/openspec-archive-change/SKILL.md +++ b/skills/openspec-archive-change/SKILL.md @@ -66,7 +66,21 @@ Archive a completed change in the experimental workflow. - If changes needed: "Sync now (recommended)", "Archive without syncing" - If already synced: "Archive now", "Sync anyway", "Cancel" - If user chooses sync, use Task tool (subagent_type: "general-purpose", prompt: "Use Skill tool to invoke openspec-sync-specs for change ''. Delta spec analysis: "). If the user chooses "Cancel", stop — do not archive. For any other choice, proceed to archive. + Route on the answer: + - "Cancel" — stop, do not archive + - "Archive without syncing" or "Archive now" — proceed to archive + - "Sync now" or "Sync anyway" — sync, then verify (below) + - Anything else — ask again rather than archiving + + To sync, run the `openspec-sync-specs` workflow inline (agent-driven intelligent merge) for change '', passing the delta spec analysis from above, and wait for it to finish. Do not delegate it to a background task — step 5 would move `changeRoot` out from under a sync that is still reading it, leaving the change archived and the main specs never updated. If your agent can only run it by delegation, delegate synchronously and wait for the result. + + Then re-run the comparison from the top of this step against every capability that has a delta spec in `artifactPaths.specs.existingOutputPaths` — not only the ones the sync reports it touched. A successful sync leaves nothing left to apply, so each capability must now read as already synced: + - ADDED requirements present + - MODIFIED requirements carrying the scenario and description changes named in the delta, with their other scenarios intact + - REMOVED requirements gone + - RENAMED requirements present under the new name and absent under the old one + + If the sync failed, or any capability does not match, report what differs and stop — do not archive. Nothing has moved and `changeRoot` is intact, so the user can fix the mismatch or re-run the sync and start the archive again. 5. **Perform the archive** @@ -102,7 +116,7 @@ Archive a completed change in the experimental workflow. **Change:** **Schema:** **Archived to:** the archive path derived from `planningHome.changesDir`/YYYY-MM-DD-/ -**Specs:** ✓ Synced to main specs (or "No delta specs" or "Sync skipped") +**Specs:** <"✓ Synced to main specs" only if the step 4 verification passed; otherwise "No delta specs" or "Sync skipped"> <"All artifacts complete. All tasks complete." — or, if archived with warnings, list them instead (e.g. "Archived with 2 incomplete tasks")> ``` @@ -113,5 +127,6 @@ Archive a completed change in the experimental workflow. - Don't block archive on warnings - just inform and confirm - Preserve .openspec.yaml when moving to archive (it moves with the directory) - Show clear summary of what happened -- If sync is requested, use openspec-sync-specs approach (agent-driven) +- If sync is requested, run the `openspec-sync-specs` workflow inline (agent-driven) +- Never archive while a spec sync is still in flight — run the sync inline and verify the main specs before moving `changeRoot` - If delta specs exist, always run the sync assessment and show the combined summary before prompting diff --git a/src/core/templates/workflows/archive-change.ts b/src/core/templates/workflows/archive-change.ts index 36dc43b403..564a7fe0c7 100644 --- a/src/core/templates/workflows/archive-change.ts +++ b/src/core/templates/workflows/archive-change.ts @@ -68,7 +68,21 @@ ${STORE_SELECTION_GUIDANCE} - If changes needed: "Sync now (recommended)", "Archive without syncing" - If already synced: "Archive now", "Sync anyway", "Cancel" - If user chooses sync, use Task tool (subagent_type: "general-purpose", prompt: "Use Skill tool to invoke openspec-sync-specs for change ''. Delta spec analysis: "). If the user chooses "Cancel", stop — do not archive. For any other choice, proceed to archive. + Route on the answer: + - "Cancel" — stop, do not archive + - "Archive without syncing" or "Archive now" — proceed to archive + - "Sync now" or "Sync anyway" — sync, then verify (below) + - Anything else — ask again rather than archiving + + To sync, run the \`openspec-sync-specs\` workflow inline (agent-driven intelligent merge) for change '', passing the delta spec analysis from above, and wait for it to finish. Do not delegate it to a background task — step 5 would move \`changeRoot\` out from under a sync that is still reading it, leaving the change archived and the main specs never updated. If your agent can only run it by delegation, delegate synchronously and wait for the result. + + Then re-run the comparison from the top of this step against every capability that has a delta spec in \`artifactPaths.specs.existingOutputPaths\` — not only the ones the sync reports it touched. A successful sync leaves nothing left to apply, so each capability must now read as already synced: + - ADDED requirements present + - MODIFIED requirements carrying the scenario and description changes named in the delta, with their other scenarios intact + - REMOVED requirements gone + - RENAMED requirements present under the new name and absent under the old one + + If the sync failed, or any capability does not match, report what differs and stop — do not archive. Nothing has moved and \`changeRoot\` is intact, so the user can fix the mismatch or re-run the sync and start the archive again. 5. **Perform the archive** @@ -104,7 +118,7 @@ ${STORE_SELECTION_GUIDANCE} **Change:** **Schema:** **Archived to:** the archive path derived from \`planningHome.changesDir\`/YYYY-MM-DD-/ -**Specs:** ✓ Synced to main specs (or "No delta specs" or "Sync skipped") +**Specs:** <"✓ Synced to main specs" only if the step 4 verification passed; otherwise "No delta specs" or "Sync skipped"> <"All artifacts complete. All tasks complete." — or, if archived with warnings, list them instead (e.g. "Archived with 2 incomplete tasks")> \`\`\` @@ -115,7 +129,8 @@ ${STORE_SELECTION_GUIDANCE} - Don't block archive on warnings - just inform and confirm - Preserve .openspec.yaml when moving to archive (it moves with the directory) - Show clear summary of what happened -- If sync is requested, use openspec-sync-specs approach (agent-driven) +- If sync is requested, run the \`openspec-sync-specs\` workflow inline (agent-driven) +- Never archive while a spec sync is still in flight — run the sync inline and verify the main specs before moving \`changeRoot\` - If delta specs exist, always run the sync assessment and show the combined summary before prompting`, license: 'MIT', compatibility: 'Requires openspec CLI.', @@ -186,7 +201,21 @@ ${STORE_SELECTION_GUIDANCE} - If changes needed: "Sync now (recommended)", "Archive without syncing" - If already synced: "Archive now", "Sync anyway", "Cancel" - If user chooses sync, use Task tool (subagent_type: "general-purpose", prompt: "Use Skill tool to invoke openspec-sync-specs for change ''. Delta spec analysis: "). If the user chooses "Cancel", stop — do not archive. For any other choice, proceed to archive. + Route on the answer: + - "Cancel" — stop, do not archive + - "Archive without syncing" or "Archive now" — proceed to archive + - "Sync now" or "Sync anyway" — sync, then verify (below) + - Anything else — ask again rather than archiving + + To sync, run the \`openspec-sync-specs\` workflow inline (agent-driven intelligent merge) for change '', passing the delta spec analysis from above, and wait for it to finish. Do not delegate it to a background task — step 5 would move \`changeRoot\` out from under a sync that is still reading it, leaving the change archived and the main specs never updated. If your agent can only run it by delegation, delegate synchronously and wait for the result. + + Then re-run the comparison from the top of this step against every capability that has a delta spec in \`artifactPaths.specs.existingOutputPaths\` — not only the ones the sync reports it touched. A successful sync leaves nothing left to apply, so each capability must now read as already synced: + - ADDED requirements present + - MODIFIED requirements carrying the scenario and description changes named in the delta, with their other scenarios intact + - REMOVED requirements gone + - RENAMED requirements present under the new name and absent under the old one + + If the sync failed, or any capability does not match, report what differs and stop — do not archive. Nothing has moved and \`changeRoot\` is intact, so the user can fix the mismatch or re-run the sync and start the archive again. 5. **Perform the archive** @@ -280,7 +309,8 @@ Target archive directory already exists. - Don't block archive on warnings - just inform and confirm - Preserve .openspec.yaml when moving to archive (it moves with the directory) - Show clear summary of what happened -- If sync is requested, use the Skill tool to invoke \`openspec-sync-specs\` (agent-driven) +- If sync is requested, run the \`openspec-sync-specs\` workflow inline (agent-driven) +- Never archive while a spec sync is still in flight — run the sync inline and verify the main specs before moving \`changeRoot\` - If delta specs exist, always run the sync assessment and show the combined summary before prompting` }; } diff --git a/test/core/templates/skill-templates-parity.test.ts b/test/core/templates/skill-templates-parity.test.ts index 90cc5707ac..0d8b783852 100644 --- a/test/core/templates/skill-templates-parity.test.ts +++ b/test/core/templates/skill-templates-parity.test.ts @@ -49,11 +49,11 @@ const EXPECTED_FUNCTION_HASHES: Record = { getOpsxContinueCommandTemplate: 'f63964fab7720ede097aa48808baff196c391b962930ca960459205c724800e5', getOpsxApplyCommandTemplate: 'daeb507206707169de73c828e199648dde5732cbc17791ef2a027adffd028574', getOpsxFfCommandTemplate: 'b859b1955cda6012877ae7f9ec6980e468f2e949a3838dfcdebc17209d133749', - getArchiveChangeSkillTemplate: '81c0ef6794bc0e0b79342ea2a1814efb0d9bc8c7ebc9d7d63a16714d781ee804', + getArchiveChangeSkillTemplate: 'a8f1d9cb06c20c7335ac35826dd09bfadead75ef6d624d359912734f74232cbc', getBulkArchiveChangeSkillTemplate: 'f675122bce3ef583b245352abedecf50ff4043e45bea6bac091885f83c7b6362', getOpsxSyncCommandTemplate: '98b20e00da5c588ff83ed6e6f0e959dfc540349090fb3f5792ea030d099b8169', getVerifyChangeSkillTemplate: 'cab4db01b5d2b1243d63d90c53747d8b39e488c60f76eba3fe8b994467f69267', - getOpsxArchiveCommandTemplate: '871d9909e0e465fc98f07826c29183f4739c1d9fb79bd268ac5f8685f37f872d', + getOpsxArchiveCommandTemplate: '9d14e1ea23ae8be8971fafa1d6a4d4717a8a7b922b6e76c6fb07aa568a420632', getOpsxOnboardCommandTemplate: '0673f34a0f81fd173bcfb8c3ac83e2b1c617f7b7564e24e5298d3bd5665a05a9', getOpsxBulkArchiveCommandTemplate: 'd0d84040bcbd44e89ac525bb21100bee7befb3604e51095bfa65b8453d85290c', getOpsxVerifyCommandTemplate: 'f01c0c0cef53be0956de52363d955d4ace131b1b2d77adf902f35fead9a1486d', @@ -71,7 +71,7 @@ const EXPECTED_GENERATED_SKILL_CONTENT_HASHES: Record = { 'openspec-apply-change': '09c0e1cdf5ccc82416d0969d6bd715cc70616bdbc3531358a5c36057f78be55a', 'openspec-ff-change': '0c82830cd9bc98f86eb56b63ddaabe2bf5d35fe25b6c40a7059311aee2c8acac', 'openspec-sync-specs': 'b3f694ab81956d05126b089fe82dea78dec21788978bb9651485f996aee96740', - 'openspec-archive-change': '5efd666d9b13e3cb41346bc65829026325daaf0b8eaa0e924e12e7021f2ff15a', + 'openspec-archive-change': '4679a077d34016bf38f0d0aa5432b53ea83ae82c2c5fec6dcb7dc15571ee8ac6', 'openspec-bulk-archive-change': '545b9528df52fbb0b4898405b42a2ce10416678d469d20cf597d022fa6e16e3b', 'openspec-verify-change': '57693d22940f06080c6cf8d590ac2f48240d4a5e9ce7074dacd0f8d3c9945afa', 'openspec-onboard': 'b1b6fc9a1b3ff64dafe9b8c39a761ee1bd001b542d47b4e4deaf058e0aa21256', @@ -209,4 +209,33 @@ describe('skill templates split parity', () => { expect(content, dirName).not.toContain('Workspace guard'); } }); + + it('gates the archive on a completed spec sync (#1393)', () => { + const generatedSkill = generateSkillContent(getArchiveChangeSkillTemplate(), 'PARITY-BASELINE'); + const commandContent = getOpsxArchiveCommandTemplate().content; + + const variants: Array<[string, string]> = [ + ['skill', generatedSkill], + ['opsx command', commandContent], + ]; + + for (const [variant, content] of variants) { + // The sync must run inline: delegating it to a background task lets step 5 + // move changeRoot out from under a sync that is still reading it. + expect(content, variant).toContain('run the `openspec-sync-specs` workflow inline'); + expect(content, variant).toContain('Do not delegate it to a background task'); + expect(content, variant).toContain('Never archive while a spec sync is still in flight'); + + // Verification must follow delta semantics. Asserting presence alone would + // read a correct REMOVED-only sync as a failure, and would pass a no-op + // sync for a MODIFIED-only delta (those requirements already exist). + expect(content, variant).toContain('MODIFIED requirements carrying the scenario and description changes'); + expect(content, variant).toContain('REMOVED requirements gone'); + expect(content, variant).toContain('RENAMED requirements present under the new name and absent under the old one'); + + // Verification is bound to the delta specs on disk, not to whatever the + // sync reports it touched — a silently skipped capability must not escape. + expect(content, variant).toContain('not only the ones the sync reports it touched'); + } + }); });