-
Notifications
You must be signed in to change notification settings - Fork 4.5k
fix(guidance): teach the spec-inventory verb to generated guidance #1700
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
clay-good
wants to merge
3
commits into
main
Choose a base branch
from
claude/openspec-issue-fixes-38e32e
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@fission-ai/openspec": patch | ||
| --- | ||
|
|
||
| Teach the generated guidance how to find and read a project's specs. `openspec list --specs` appeared in no generated skill, command, or artifact instruction, while `openspec list --json` (the in-flight *change* list) appeared throughout, so an agent asked to read the existing specs first enumerated changes instead and reported the step complete against the wrong object. The explore skill and command now list the spec inventory alongside the change list and say which is which, and the spec-driven `proposal` and `specs` instructions name the command where they ask for existing capabilities to be researched and for a delta's path to match an existing one. Both steps carry `--store "<id>"`, and capabilities are read with `openspec show "<spec-id>" --type spec --json --no-scenarios` so the read resolves against the same root the listing came from. `docs/cli.md` now documents the `--store` option on `list` and `show`, which both already accepted it. Fixes #1689. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,130 @@ | ||
| import path from 'path'; | ||
| import { fileURLToPath } from 'url'; | ||
| import { describe, expect, it } from 'vitest'; | ||
|
|
||
| import { | ||
| getSkillTemplates, | ||
| getCommandTemplates, | ||
| } from '../../../src/core/shared/skill-generation.js'; | ||
| import { | ||
| getExploreSkillTemplate, | ||
| getOpsxExploreCommandTemplate, | ||
| } from '../../../src/core/templates/skill-templates.js'; | ||
| import { loadSchema } from '../../../src/core/artifact-graph/schema.js'; | ||
|
|
||
| // #1689: 1.9.0 removed openspec/AGENTS.md, which carried the spec index, and | ||
| // nothing that replaced it ever named the verb that lists specs. Measured | ||
| // across one repo's generated surfaces: `openspec list --json` (the CHANGE | ||
| // list) appeared 10 times, `openspec list --specs` zero times. An agent told | ||
| // to "read the existing specs first" reaches for the one enumeration verb it | ||
| // was taught, gets the in-flight change list, and reports the step complete | ||
| // against the wrong object. | ||
| const SPEC_INVENTORY = 'openspec list --specs'; | ||
|
|
||
| // Assertions about the guidance attached to the command are scoped to a window | ||
| // after it rather than to the whole body, so an unrelated occurrence elsewhere | ||
| // in a long template cannot stand in for the passage under test. | ||
| const PASSAGE_WINDOW = 700; | ||
|
|
||
| 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; | ||
| } | ||
|
|
||
| const exploreBodies: Array<[string, string]> = [ | ||
| ['explore skill', getExploreSkillTemplate().instructions], | ||
| ['explore command', getOpsxExploreCommandTemplate().content], | ||
| ]; | ||
|
|
||
| describe('spec inventory vocabulary (#1689)', () => { | ||
| it('teaches the spec-inventory verb somewhere in the generated surfaces', () => { | ||
| const bodies = [ | ||
| ...getSkillTemplates().map(entry => entry.template.instructions), | ||
| ...getCommandTemplates().map(entry => entry.template.content), | ||
| ]; | ||
|
|
||
| const carriers = bodies.filter(body => body.includes(SPEC_INVENTORY)); | ||
| expect( | ||
| carriers.length, | ||
| `no generated skill or command names "${SPEC_INVENTORY}", so the spec inventory is unreachable by any path the tool teaches` | ||
| ).toBeGreaterThan(0); | ||
| }); | ||
|
|
||
| it('names the spec inventory in explore, where the agent orients', () => { | ||
| for (const [label, body] of exploreBodies) { | ||
| expect(body, label).toContain(SPEC_INVENTORY); | ||
| } | ||
| }); | ||
|
|
||
| it('distinguishes the change list from the spec inventory in explore', () => { | ||
| // Naming the command is not enough on its own: `openspec list` defaults to | ||
| // changes, so the two enumerations have to be told apart explicitly. | ||
| for (const [label, body] of exploreBodies) { | ||
| expect(body, label).toContain('openspec list --json'); | ||
| expect(body, label).toContain('`openspec list` on its own never shows it'); | ||
| } | ||
| }); | ||
|
|
||
| it('names the spec inventory where the proposal picks capabilities', () => { | ||
| // "Research existing specs before filling this in" named no command, which | ||
| // is how the Capabilities section ends up inventing a near-duplicate | ||
| // capability instead of reusing the existing one. | ||
| expect(instructionFor('proposal')).toContain(SPEC_INVENTORY); | ||
| }); | ||
|
|
||
| it('names the spec inventory where a delta must match an existing path', () => { | ||
| expect(instructionFor('specs')).toContain(SPEC_INVENTORY); | ||
| }); | ||
|
|
||
| // A bare `openspec list --specs` reads the local inventory, so under a | ||
| // selected store it confirms a capability path against the wrong root. | ||
| // Every site that names the command must carry the store qualifier with it. | ||
| it('carries the store qualifier everywhere it names the command', () => { | ||
| const sites: Array<[string, string]> = [ | ||
| ...exploreBodies, | ||
| ['proposal instruction', instructionFor('proposal')], | ||
| ['specs instruction', instructionFor('specs')], | ||
| ]; | ||
|
|
||
| for (const [label, body] of sites) { | ||
| const start = body.indexOf(SPEC_INVENTORY); | ||
| expect(start, label).toBeGreaterThanOrEqual(0); | ||
|
|
||
| // Scoped to the passage that names the command: every explore body | ||
| // already carries the store qualifier in its unrelated capture steps, | ||
| // so a whole-body match would pass even with the qualifier dropped here. | ||
| const passage = body.slice(start, start + PASSAGE_WINDOW); | ||
| expect(passage, `${label} names the command without its store qualifier`).toContain( | ||
| 'registered standalone store' | ||
| ); | ||
| expect(passage, label).toContain('--store "<id>"'); | ||
| } | ||
| }); | ||
|
|
||
| // Reading the inventory back by raw path defeats the fix under a store: the | ||
| // ids `list --specs --store <id>` returns are not present under the local | ||
| // `openspec/specs/`, so the read either fails or silently lands on a | ||
| // same-named local capability - the wrong-object failure #1689 is about. | ||
| // `openspec show` resolves against the same root the listing came from. | ||
| it('reads a listed capability with the store-aware command', () => { | ||
| const sites: Array<[string, string]> = [ | ||
| ...exploreBodies, | ||
| ['proposal instruction', instructionFor('proposal')], | ||
| ]; | ||
|
|
||
| for (const [label, body] of sites) { | ||
| const start = body.indexOf(SPEC_INVENTORY); | ||
| const passage = body.slice(start, start + PASSAGE_WINDOW); | ||
| expect(passage, `${label} does not name a store-aware read`).toContain('openspec show'); | ||
| // A change and a spec may share a name; without --type that is an | ||
| // ambiguous-item error rather than the spec the agent asked for. | ||
| expect(passage, `${label} omits the --type spec disambiguator`).toContain('--type spec'); | ||
| } | ||
| }); | ||
| }); | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Assert the optional store guidance.
The test verifies the inventory command and the change/spec distinction, but it does not verify the conditional
--store "<id>"guidance or the related--jsonwording. A future edit can remove store propagation while this test remains green. Add exact-content assertions for the proposal, modified-capability specs instruction, and both explore surfaces.Validate with
pnpm exec vitest run test/core/templates/spec-inventory.test.ts.🤖 Prompt for AI Agents