From d339525480462513fd3f841ec6fc11c417714b1a Mon Sep 17 00:00:00 2001 From: Clay Good Date: Wed, 19 Aug 2026 12:03:44 -0500 Subject: [PATCH 1/3] fix(schema): resolve main-spec reads against the store-aware root MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The spec-driven `specs` instruction named `openspec/specs//spec.md` — a cwd-relative path — for the two operations that touch a capability's main spec: step 1 of the MODIFIED workflow ("locate the existing requirement") and the edit that fixes a leftover TBD Purpose. When the change lives in a registered store, the main spec is under the store root. Verified against one: `openspec instructions specs --store mystore --json` returns `planningHome.root` pointing at the store while the instruction sent the read to the working repo, where the capability does not exist. Where a local capability happens to share the name it is worse than a miss — the read succeeds against a different capability and step 2 copies the wrong requirement block into the delta, silently. Both now use `/openspec/specs/...`, the root the same JSON already returns, matching what sync-specs.ts and archive-change.ts have said since they were written: use the store-aware root, not a hardcoded repo path. Guidance text only — no CLI, parser, or archive behavior changes. The two remaining `openspec/specs/` mentions describe the shape of a capability path rather than a file operation, and are left alone. Closes #1702 Co-Authored-By: Claude Opus 5 --- .changeset/store-aware-main-spec-paths.md | 5 ++ schemas/spec-driven/schema.yaml | 8 ++- test/core/templates/main-spec-paths.test.ts | 62 +++++++++++++++++++++ 3 files changed, 73 insertions(+), 2 deletions(-) create mode 100644 .changeset/store-aware-main-spec-paths.md create mode 100644 test/core/templates/main-spec-paths.test.ts diff --git a/.changeset/store-aware-main-spec-paths.md b/.changeset/store-aware-main-spec-paths.md new file mode 100644 index 0000000000..4ddbdd68cc --- /dev/null +++ b/.changeset/store-aware-main-spec-paths.md @@ -0,0 +1,5 @@ +--- +"@fission-ai/openspec": patch +--- + +Point the spec-driven `specs` instruction's main-spec read and edit at the store-aware root. It named `openspec/specs//spec.md`, a path relative to the current directory, for both step 1 of the MODIFIED workflow ("locate the existing requirement") and the edit that fixes a leftover `TBD` Purpose. When the change lives in a registered store the main spec is under the store root, so that read missed it — or, when a local capability happened to share the name, silently returned a different capability and the MODIFIED block was copied from the wrong requirement. Both now use `/openspec/specs/...`, the root already returned by `openspec instructions ... --json` and the same convention the sync and archive workflows use. Fixes #1702. diff --git a/schemas/spec-driven/schema.yaml b/schemas/spec-driven/schema.yaml index ae4d9eb336..aa92fea124 100644 --- a/schemas/spec-driven/schema.yaml +++ b/schemas/spec-driven/schema.yaml @@ -91,10 +91,14 @@ artifacts: 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; it is the store-aware root, so use it rather than a + repo-relative path - when a store is selected it points at the store, + not the current repository. 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) diff --git a/test/core/templates/main-spec-paths.test.ts b/test/core/templates/main-spec-paths.test.ts new file mode 100644 index 0000000000..bc94afda62 --- /dev/null +++ b/test/core/templates/main-spec-paths.test.ts @@ -0,0 +1,62 @@ +import path from 'path'; +import { fileURLToPath } from 'url'; +import { describe, expect, it } from 'vitest'; + +import { loadSchema } from '../../../src/core/artifact-graph/schema.js'; + +// #1702: the `specs` instruction sent main-spec reads and edits to +// `openspec/specs//spec.md`, a cwd-relative path. When the +// change lives in a registered store the main spec is under the store root, so +// the read either misses or - when a local capability shares the name - lands +// on a different capability and the MODIFIED workflow copies the wrong +// requirement block. The workflow templates already use the store-aware root +// (`sync-specs.ts`, `archive-change.ts`); the schema instruction was the site +// that was missed. +const STORE_AWARE_ROOT = ''; + +const repoRoot = path.resolve(fileURLToPath(new URL('.', import.meta.url)), '../../..'); +const defaultSchema = loadSchema(path.join(repoRoot, 'schemas', 'spec-driven', 'schema.yaml')); + +function instructionFor(artifactId: string): string { + const artifact = defaultSchema.artifacts.find(entry => entry.id === artifactId); + expect(artifact, `spec-driven has no "${artifactId}" artifact`).toBeDefined(); + const instruction = artifact?.instruction; + expect(instruction, `spec-driven "${artifactId}" has no instruction`).toBeDefined(); + return instruction as string; +} + +/** + * Lines that operate on a main spec file. A mention that only describes the + * shape of a capability path ("use the exact existing path under + * `openspec/specs/`") is not a file operation and is out of scope. + */ +function mainSpecOperations(instruction: string): string[] { + return instruction + .split('\n') + .filter(line => /openspec\/specs\/\/spec\.md/.test(line)) + .filter(line => /\b(edit|Locate)\b/.test(line)); +} + +describe('main spec paths in the specs instruction (#1702)', () => { + it('routes every main-spec operation through the store-aware root', () => { + const operations = mainSpecOperations(instructionFor('specs')); + + // Both the Purpose edit and step 1 of the MODIFIED workflow. + expect(operations.length, 'expected the main-spec read and edit to be present').toBe(2); + + for (const line of operations) { + expect( + line, + `main-spec operation uses a cwd-relative path: ${line.trim()}` + ).toContain(`${STORE_AWARE_ROOT}/openspec/specs/`); + } + }); + + it('explains where the store-aware root comes from', () => { + // Naming `planningHome.root` is not enough on its own: it is a field of the + // instructions JSON, and an agent that does not know that cannot use it. + const instruction = instructionFor('specs'); + expect(instruction).toContain('openspec instructions'); + expect(instruction).toContain('store-aware root'); + }); +}); From 0dd2326af52e2acd3502fc98fb8858d478965021 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Wed, 19 Aug 2026 12:17:35 -0500 Subject: [PATCH 2/3] fix(schema): make the store-aware root unconditional, and prove it resolves Two hardening findings. The wording said the root "points at the store when a store is selected." Verified across all four root configurations, that undersells it: a project `store:` pointer (source `declared`) and a global default store (source `global_default`) both resolve to the store with no `--store` flag passed. An agent reading the old sentence could conclude the case did not apply to it and fall back to a repo-relative path. It now says to always use the field and not to reason about which case applies. The test only pinned the placeholder text, which would still pass if `planningHome.root` were renamed or the suffix were wrong. Added a guard that substitutes the placeholder with a real resolved planning home and asserts the composed path lands on an actual main spec. Mutation-tested: inserting a path segment and renaming the field each fail it. Verified end to end that the composed path exists under all three store-selecting configurations, and under a plain local repo. Co-Authored-By: Claude Opus 5 --- .changeset/store-aware-main-spec-paths.md | 2 +- schemas/spec-driven/schema.yaml | 8 ++-- test/core/templates/main-spec-paths.test.ts | 43 ++++++++++++++++++++- 3 files changed, 48 insertions(+), 5 deletions(-) diff --git a/.changeset/store-aware-main-spec-paths.md b/.changeset/store-aware-main-spec-paths.md index 4ddbdd68cc..1ca82e9a33 100644 --- a/.changeset/store-aware-main-spec-paths.md +++ b/.changeset/store-aware-main-spec-paths.md @@ -2,4 +2,4 @@ "@fission-ai/openspec": patch --- -Point the spec-driven `specs` instruction's main-spec read and edit at the store-aware root. It named `openspec/specs//spec.md`, a path relative to the current directory, for both step 1 of the MODIFIED workflow ("locate the existing requirement") and the edit that fixes a leftover `TBD` Purpose. When the change lives in a registered store the main spec is under the store root, so that read missed it — or, when a local capability happened to share the name, silently returned a different capability and the MODIFIED block was copied from the wrong requirement. Both now use `/openspec/specs/...`, the root already returned by `openspec instructions ... --json` and the same convention the sync and archive workflows use. Fixes #1702. +Point the spec-driven `specs` instruction's main-spec read and edit at the store-aware root. It named `openspec/specs//spec.md`, a path relative to the current directory, for both step 1 of the MODIFIED workflow ("locate the existing requirement") and the edit that fixes a leftover `TBD` Purpose. When the change lives in a store — whether selected with `--store`, a project `store:` pointer, or a global default store — the main spec is under the store root, so that read missed it, or silently returned a different capability when a local one happened to share the name, and the MODIFIED block was then copied from the wrong requirement. Both operations now use `/openspec/specs/...`, the root already returned by `openspec instructions ... --json` and the same convention the sync and archive workflows use. Fixes #1702. diff --git a/schemas/spec-driven/schema.yaml b/schemas/spec-driven/schema.yaml index aa92fea124..ff12132d1e 100644 --- a/schemas/spec-driven/schema.yaml +++ b/schemas/spec-driven/schema.yaml @@ -93,9 +93,11 @@ artifacts: existing capability's Purpose - including a leftover `TBD` placeholder - edit `/openspec/specs//spec.md` directly. `planningHome.root` comes from the `openspec instructions ... - --json` response; it is the store-aware root, so use it rather than a - repo-relative path - when a store is selected it points at the store, - not the current repository. + --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` (the same store-aware root as above) diff --git a/test/core/templates/main-spec-paths.test.ts b/test/core/templates/main-spec-paths.test.ts index bc94afda62..5077021dc5 100644 --- a/test/core/templates/main-spec-paths.test.ts +++ b/test/core/templates/main-spec-paths.test.ts @@ -1,8 +1,11 @@ +import * as fs from 'node:fs'; +import * as os from 'node:os'; import path from 'path'; import { fileURLToPath } from 'url'; -import { describe, expect, it } from 'vitest'; +import { afterEach, describe, expect, it } from 'vitest'; import { loadSchema } from '../../../src/core/artifact-graph/schema.js'; +import { resolveCurrentPlanningHomeSync } from '../../../src/core/planning-home.js'; // #1702: the `specs` instruction sent main-spec reads and edits to // `openspec/specs//spec.md`, a cwd-relative path. When the @@ -59,4 +62,42 @@ describe('main spec paths in the specs instruction (#1702)', () => { expect(instruction).toContain('openspec instructions'); expect(instruction).toContain('store-aware root'); }); + + // The text guards above pin the placeholder. This pins the other half of the + // contract: that `planningHome.root` is a real field whose value, joined with + // the literal suffix the instruction spells out, lands on the main spec. A + // renamed field or a wrong suffix leaves the substitution pointing at nothing. + describe('the composed path resolves', () => { + const tempDirs: string[] = []; + + afterEach(() => { + for (const dir of tempDirs.splice(0)) { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it('lands on the main spec when the placeholder is substituted', () => { + const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'openspec-main-spec-path-')); + tempDirs.push(tempDir); + + const capability = 'identity/user-auth'; + const specDir = path.join(tempDir, 'openspec', 'specs', ...capability.split('/')); + fs.mkdirSync(specDir, { recursive: true }); + fs.mkdirSync(path.join(tempDir, 'openspec', 'changes'), { recursive: true }); + fs.writeFileSync(path.join(specDir, 'spec.md'), '# spec\n'); + + const planningHome = resolveCurrentPlanningHomeSync({ startPath: tempDir }); + expect(planningHome.root, 'planningHome has no root field').toBeTypeOf('string'); + + const [operation] = mainSpecOperations(instructionFor('specs')); + const template = operation.match(/\/\S*?spec\.md/)?.[0]; + expect(template, `no main-spec path found in: ${operation.trim()}`).toBeDefined(); + + const resolved = (template as string) + .replace('', planningHome.root) + .replace('', capability); + + expect(fs.existsSync(resolved), `composed path does not exist: ${resolved}`).toBe(true); + }); + }); }); From 9f47c024adbe3b66f63d23a15bf629243bdc7294 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Wed, 19 Aug 2026 12:27:17 -0500 Subject: [PATCH 3/3] test: compose the main-spec path from segments, not string substitution The guard substituted `planningHome.root` into a template spelled with forward slashes. On Windows that yields a mixed-separator path, so the assertion passed because Node accepts forward slashes there rather than because the path was built correctly. Windows CI was green either way; this makes the construction right instead of merely tolerated. The suffix is now captured on its own and joined to the root with path.join, so the assertion uses native separators everywhere. All three mutations (cwd-relative path, extra segment, renamed field) still fail the guard. Addresses CodeRabbit review on #1703. Co-Authored-By: Claude Opus 5 --- test/core/templates/main-spec-paths.test.ts | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/test/core/templates/main-spec-paths.test.ts b/test/core/templates/main-spec-paths.test.ts index 5077021dc5..3df8e72d0e 100644 --- a/test/core/templates/main-spec-paths.test.ts +++ b/test/core/templates/main-spec-paths.test.ts @@ -90,12 +90,17 @@ describe('main spec paths in the specs instruction (#1702)', () => { expect(planningHome.root, 'planningHome has no root field').toBeTypeOf('string'); const [operation] = mainSpecOperations(instructionFor('specs')); - const template = operation.match(/\/\S*?spec\.md/)?.[0]; - expect(template, `no main-spec path found in: ${operation.trim()}`).toBeDefined(); - - const resolved = (template as string) - .replace('', planningHome.root) - .replace('', capability); + const suffix = operation.match(/\/(\S*?spec\.md)/)?.[1]; + expect(suffix, `no main-spec path found in: ${operation.trim()}`).toBeDefined(); + + // Join as path segments rather than substituting into the string. The + // instruction spells its suffix with forward slashes while + // `planningHome.root` carries native separators, so a plain replace would + // hand Windows a mixed-separator path and lean on Node accepting it. + const resolved = path.join( + planningHome.root, + ...(suffix as string).replace('', capability).split('/') + ); expect(fs.existsSync(resolved), `composed path does not exist: ${resolved}`).toBe(true); });