feat(store): support separate schema stores - #1473
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (16)
🚧 Files skipped from review as they are similar to previous changes (12)
📝 WalkthroughWalkthroughAdds configurable ChangesSchema Store resolution and workflow integration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant resolveOpenSpecRoot
participant StoreRegistry
participant WorkflowCommand
participant resolveSchema
CLI->>resolveOpenSpecRoot: resolve planning root and schema context
resolveOpenSpecRoot->>StoreRegistry: resolve registered schema Store
StoreRegistry-->>resolveOpenSpecRoot: canonical checkout and visibility
resolveOpenSpecRoot-->>CLI: resolved root with schemaContext
CLI->>WorkflowCommand: execute with schemaTarget and projectConfig
WorkflowCommand->>resolveSchema: resolve workflow schema
resolveSchema-->>WorkflowCommand: schema path and Store provenance
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/core/completions/completion-provider.ts (1)
93-104: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKey the schema cache by resolution target.
A call with a different
schemaTargetwithin the TTL reusesschemaCache, returning schemas from the prior Store/visibility context. Invalidate on target changes or cache by a stable target key.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/completions/completion-provider.ts` around lines 93 - 104, Update getSchemaNames so schemaCache is associated with the effective schemaTarget (or this.projectRoot fallback), using a stable target key and only reusing cached data when that key matches; otherwise fetch fresh schemas and store the new key with the result.src/commands/schema.ts (1)
604-651: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGuard
schema fork/schema initwrites intosinkRootwhen a Store is active.When
schemaStoreis configured, writes stay atroot.consumerRoot(sinkRootin the workflow), but schema resolution lists the active project layer viaschemaContext.root. If a Store replaces the project schema layer, copying or creating a schema here leaves it invisible to subsequent lookups/listing. ReusesourceLocationthere too soschema fork --jsonreports whether the destination appears at the active project layer while still telling the user where the file was written.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/schema.ts` around lines 604 - 651, Update the schema fork/init destination handling after sourceLocation is determined to use the active schema layer represented by sourceLocation when resolving or reporting the destination, while continuing to write files under root.consumerRoot. Ensure schema fork --json distinguishes the actual write location from whether the destination is visible in the active project layer, including Store-backed configurations.
🧹 Nitpick comments (1)
src/core/artifact-graph/resolver.ts (1)
34-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate visibility logic between
resolver.tsandschema.ts.isVisibleFromPrimarySourceinresolver.tsis not exported, soschema.ts'scheckAllLocationsreimplements the identicalsource === 'project' || visibleSchemas === '*' || visibleSchemas.includes(name)expression inline. One root cause: the helper should be shared, not duplicated.
src/core/artifact-graph/resolver.ts#L34-L44: exportisVisibleFromPrimarySource.src/commands/schema.ts#L57-L79: import and reuse the exported helper instead of recomputingvisibleinline.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/artifact-graph/resolver.ts` around lines 34 - 44, Export isVisibleFromPrimarySource from src/core/artifact-graph/resolver.ts. In src/commands/schema.ts, import and reuse this helper within checkAllLocations instead of duplicating the visible expression inline; preserve the existing visibility behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/change.ts`:
- Around line 42-52: Update the constructor comment to reflect that deprecated
noun-form commands can receive root-aware context from the CLI. In
ChangeCommand.validate(), derive changesPath from this.rootPath rather than
process.cwd(), and construct Validator with this.schemaTarget so
schemaStore-backed schemas resolve skip specs consistently with the top-level
validation flow.
---
Outside diff comments:
In `@src/commands/schema.ts`:
- Around line 604-651: Update the schema fork/init destination handling after
sourceLocation is determined to use the active schema layer represented by
sourceLocation when resolving or reporting the destination, while continuing to
write files under root.consumerRoot. Ensure schema fork --json distinguishes the
actual write location from whether the destination is visible in the active
project layer, including Store-backed configurations.
In `@src/core/completions/completion-provider.ts`:
- Around line 93-104: Update getSchemaNames so schemaCache is associated with
the effective schemaTarget (or this.projectRoot fallback), using a stable target
key and only reusing cached data when that key matches; otherwise fetch fresh
schemas and store the new key with the result.
---
Nitpick comments:
In `@src/core/artifact-graph/resolver.ts`:
- Around line 34-44: Export isVisibleFromPrimarySource from
src/core/artifact-graph/resolver.ts. In src/commands/schema.ts, import and reuse
this helper within checkAllLocations instead of duplicating the visible
expression inline; preserve the existing visibility behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f475f1c3-bac5-42e5-a7a4-6f7ceab4beff
📒 Files selected for processing (43)
.changeset/share-schema-stores.mddocs/agent-contract.mddocs/cli.mddocs/customization.mddocs/stores-beta/user-guide.mdopenspec/changes/add-schema-store-sources/.openspec.yamlopenspec/changes/add-schema-store-sources/design.mdopenspec/changes/add-schema-store-sources/proposal.mdopenspec/changes/add-schema-store-sources/specs/config-loading/spec.mdopenspec/changes/add-schema-store-sources/specs/schema-resolution/spec.mdopenspec/changes/add-schema-store-sources/specs/schema-store-sources/spec.mdopenspec/changes/add-schema-store-sources/specs/schema-which-command/spec.mdopenspec/changes/add-schema-store-sources/tasks.mdsrc/cli/index.tssrc/commands/change.tssrc/commands/completion.tssrc/commands/schema.tssrc/commands/validate.tssrc/commands/workflow/instructions.tssrc/commands/workflow/new-change.tssrc/commands/workflow/schemas.tssrc/commands/workflow/shared.tssrc/commands/workflow/status.tssrc/commands/workflow/templates.tssrc/core/archive.tssrc/core/artifact-graph/index.tssrc/core/artifact-graph/instruction-loader.tssrc/core/artifact-graph/resolver.tssrc/core/completions/completion-provider.tssrc/core/list.tssrc/core/project-config.tssrc/core/root-selection.tssrc/core/validation/validator.tssrc/core/view.tssrc/utils/change-metadata.tssrc/utils/change-utils.tssrc/utils/task-progress.tstest/commands/completion.test.tstest/commands/store-root-selection.test.tstest/core/artifact-graph/resolver.test.tstest/core/project-config.test.tstest/core/root-selection.test.tstest/core/view.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/schema.ts`:
- Line 647: Update the schema fork output after getSchemaResolution to include
the resolved storeId in both JSON sourceLocation data and human-readable output,
while preserving the existing store source labeling and provenance behavior for
other source types.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e5884762-196d-4052-9001-cbc9dee27cc7
📒 Files selected for processing (43)
.changeset/share-schema-stores.mddocs/agent-contract.mddocs/cli.mddocs/customization.mddocs/stores-beta/user-guide.mdopenspec/changes/add-schema-store-sources/.openspec.yamlopenspec/changes/add-schema-store-sources/design.mdopenspec/changes/add-schema-store-sources/proposal.mdopenspec/changes/add-schema-store-sources/specs/config-loading/spec.mdopenspec/changes/add-schema-store-sources/specs/schema-resolution/spec.mdopenspec/changes/add-schema-store-sources/specs/schema-store-sources/spec.mdopenspec/changes/add-schema-store-sources/specs/schema-which-command/spec.mdopenspec/changes/add-schema-store-sources/tasks.mdsrc/cli/index.tssrc/commands/change.tssrc/commands/completion.tssrc/commands/schema.tssrc/commands/validate.tssrc/commands/workflow/instructions.tssrc/commands/workflow/new-change.tssrc/commands/workflow/schemas.tssrc/commands/workflow/shared.tssrc/commands/workflow/status.tssrc/commands/workflow/templates.tssrc/core/archive.tssrc/core/artifact-graph/index.tssrc/core/artifact-graph/instruction-loader.tssrc/core/artifact-graph/resolver.tssrc/core/completions/completion-provider.tssrc/core/list.tssrc/core/project-config.tssrc/core/root-selection.tssrc/core/validation/validator.tssrc/core/view.tssrc/utils/change-metadata.tssrc/utils/change-utils.tssrc/utils/task-progress.tstest/commands/completion.test.tstest/commands/store-root-selection.test.tstest/core/artifact-graph/resolver.test.tstest/core/project-config.test.tstest/core/root-selection.test.tstest/core/view.test.ts
🚧 Files skipped from review as they are similar to previous changes (32)
- openspec/changes/add-schema-store-sources/specs/config-loading/spec.md
- openspec/changes/add-schema-store-sources/specs/schema-resolution/spec.md
- src/commands/validate.ts
- openspec/changes/add-schema-store-sources/.openspec.yaml
- src/commands/workflow/status.ts
- src/core/artifact-graph/index.ts
- src/core/validation/validator.ts
- src/utils/change-utils.ts
- .changeset/share-schema-stores.md
- src/commands/workflow/schemas.ts
- docs/customization.md
- src/commands/completion.ts
- docs/cli.md
- openspec/changes/add-schema-store-sources/proposal.md
- openspec/changes/add-schema-store-sources/specs/schema-which-command/spec.md
- src/utils/task-progress.ts
- docs/agent-contract.md
- src/core/list.ts
- openspec/changes/add-schema-store-sources/specs/schema-store-sources/spec.md
- src/commands/workflow/shared.ts
- src/commands/change.ts
- src/core/artifact-graph/instruction-loader.ts
- src/commands/workflow/new-change.ts
- openspec/changes/add-schema-store-sources/design.md
- src/core/completions/completion-provider.ts
- src/core/view.ts
- docs/stores-beta/user-guide.md
- src/core/project-config.ts
- src/core/artifact-graph/resolver.ts
- src/core/root-selection.ts
- src/commands/workflow/instructions.ts
- src/core/archive.ts
|
|
||
| // Determine source location | ||
| const sourceResolution = getSchemaResolution(source, projectRoot); | ||
| const sourceResolution = getSchemaResolution(source, schemaContext); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Include the Store identifier in fork output.
Line 647 resolves storeId, but schema fork emits only sourceLocation: "store" and human (store). Include the Store ID in both outputs so users can identify the configured Store that supplied the forked schema.
The PR objective requires Store provenance in human-readable and JSON output.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/commands/schema.ts` at line 647, Update the schema fork output after
getSchemaResolution to include the resolved storeId in both JSON sourceLocation
data and human-readable output, while preserving the existing store source
labeling and provenance behavior for other source types.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
test/core/project-config.test.ts (1)
572-576: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate
writeConfighelper.An identical helper already exists at Lines 490-494; consider hoisting one copy to the enclosing
describeif the scopes allow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/core/project-config.test.ts` around lines 572 - 576, Remove the duplicate writeConfig helper in the later test scope and reuse the existing writeConfig defined earlier in the enclosing describe. Hoist the remaining helper only if needed so all affected tests retain access without changing their behavior.src/commands/completion.ts (1)
289-304: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider degrading to project-root schema listing for any root-selection failure.
Only
no_root_with_registered_storesis tolerated. Any otherRootSelectionError(e.g.invalid_schema_store_declarationfrom a malformedschemaStore, or an unresolvable Store checkout) propagates to the outercatchat Line 322, soopenspec complete schemasemits nothing at all instead of the locally available schemas. Shell completion is a best-effort surface; a config typo silently killing it is unhelpful.♻️ Suggested fallback
- } catch (error) { - if ( - !isRootSelectionError(error) || - error.diagnostic.code !== 'no_root_with_registered_stores' - ) { - throw error; - } - } + } catch (error) { + // Completion is best-effort: fall back to project-root listing + // rather than emitting nothing on any root-selection failure. + if (!isRootSelectionError(error)) { + throw error; + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/completion.ts` around lines 289 - 304, Update the schema completion flow around resolveOpenSpecRoot and getSchemaNames so any root-selection failure degrades to listing schemas from the project root, rather than rethrowing RootSelectionError variants such as invalid schema-store declarations or unresolved checkouts. Preserve normal resolved-root behavior, and ensure completion remains best-effort while only falling back when root resolution fails.test/core/artifact-graph/resolver.test.ts (1)
752-764: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test's key assertions are vacuous — the consumer root is never part of the resolution context.
consumerRootis only used to write a schema on disk; every lookup here is done withstoreContext(storeRoot), so the resolver has no path by which it could ever seeconsumer-only.not.toContain('consumer-only')andtoBeNull()would still pass if Store replacement were completely broken. To actually cover replacement, assert that the same schema name resolves to the Store copy while the consumer-root context resolves to the consumer copy.💚 Suggested strengthening
it('replaces consumer-local project schemas when a Store is configured', () => { process.env.XDG_DATA_HOME = path.join(tempDir, 'user-data'); const consumerRoot = path.join(tempDir, 'consumer'); const storeRoot = path.join(tempDir, 'schema-store'); - writeSchema(consumerRoot, 'consumer-only', 'consumer-version'); - writeSchema(storeRoot, 'store-only', 'store-version'); + const consumerDir = writeSchema(consumerRoot, 'shared-schema', 'consumer-version'); + const storeDir = writeSchema(storeRoot, 'shared-schema', 'store-version'); + writeSchema(consumerRoot, 'consumer-only', 'consumer-version'); + writeSchema(storeRoot, 'store-only', 'store-version'); + // Sanity: the consumer root really does own these schemas. + expect(getSchemaDir('shared-schema', consumerRoot)).toBe(consumerDir); + expect(listSchemas(consumerRoot)).toContain('consumer-only'); + const schemas = listSchemas(storeContext(storeRoot)); expect(schemas).toContain('store-only'); expect(schemas).not.toContain('consumer-only'); + expect(getSchemaDir('shared-schema', storeContext(storeRoot))).toBe(storeDir); expect(getSchemaDir('consumer-only', storeContext(storeRoot))).toBeNull(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/core/artifact-graph/resolver.test.ts` around lines 752 - 764, Strengthen the test around listSchemas and getSchemaDir by creating the resolution context with both consumerRoot and storeRoot, then use the same schema name in both locations with distinguishable versions. Assert the Store-backed context returns the Store schema while a consumer-root-only context returns the consumer schema, ensuring the replacement behavior is genuinely exercised rather than merely confirming the consumer path is absent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openspec/changes/add-schema-store-sources/tasks.md`:
- Line 6: Update task 1.4 to invoke Vitest with the documented `pnpm exec vitest
run test/core/project-config.test.ts` command, preserving the existing focused
test target and configuration-contract verification.
---
Nitpick comments:
In `@src/commands/completion.ts`:
- Around line 289-304: Update the schema completion flow around
resolveOpenSpecRoot and getSchemaNames so any root-selection failure degrades to
listing schemas from the project root, rather than rethrowing RootSelectionError
variants such as invalid schema-store declarations or unresolved checkouts.
Preserve normal resolved-root behavior, and ensure completion remains
best-effort while only falling back when root resolution fails.
In `@test/core/artifact-graph/resolver.test.ts`:
- Around line 752-764: Strengthen the test around listSchemas and getSchemaDir
by creating the resolution context with both consumerRoot and storeRoot, then
use the same schema name in both locations with distinguishable versions. Assert
the Store-backed context returns the Store schema while a consumer-root-only
context returns the consumer schema, ensuring the replacement behavior is
genuinely exercised rather than merely confirming the consumer path is absent.
In `@test/core/project-config.test.ts`:
- Around line 572-576: Remove the duplicate writeConfig helper in the later test
scope and reuse the existing writeConfig defined earlier in the enclosing
describe. Hoist the remaining helper only if needed so all affected tests retain
access without changing their behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e5884762-196d-4052-9001-cbc9dee27cc7
📒 Files selected for processing (43)
.changeset/share-schema-stores.mddocs/agent-contract.mddocs/cli.mddocs/customization.mddocs/stores-beta/user-guide.mdopenspec/changes/add-schema-store-sources/.openspec.yamlopenspec/changes/add-schema-store-sources/design.mdopenspec/changes/add-schema-store-sources/proposal.mdopenspec/changes/add-schema-store-sources/specs/config-loading/spec.mdopenspec/changes/add-schema-store-sources/specs/schema-resolution/spec.mdopenspec/changes/add-schema-store-sources/specs/schema-store-sources/spec.mdopenspec/changes/add-schema-store-sources/specs/schema-which-command/spec.mdopenspec/changes/add-schema-store-sources/tasks.mdsrc/cli/index.tssrc/commands/change.tssrc/commands/completion.tssrc/commands/schema.tssrc/commands/validate.tssrc/commands/workflow/instructions.tssrc/commands/workflow/new-change.tssrc/commands/workflow/schemas.tssrc/commands/workflow/shared.tssrc/commands/workflow/status.tssrc/commands/workflow/templates.tssrc/core/archive.tssrc/core/artifact-graph/index.tssrc/core/artifact-graph/instruction-loader.tssrc/core/artifact-graph/resolver.tssrc/core/completions/completion-provider.tssrc/core/list.tssrc/core/project-config.tssrc/core/root-selection.tssrc/core/validation/validator.tssrc/core/view.tssrc/utils/change-metadata.tssrc/utils/change-utils.tssrc/utils/task-progress.tstest/commands/completion.test.tstest/commands/store-root-selection.test.tstest/core/artifact-graph/resolver.test.tstest/core/project-config.test.tstest/core/root-selection.test.tstest/core/view.test.ts
🚧 Files skipped from review as they are similar to previous changes (28)
- openspec/changes/add-schema-store-sources/.openspec.yaml
- src/core/artifact-graph/index.ts
- openspec/changes/add-schema-store-sources/specs/schema-which-command/spec.md
- .changeset/share-schema-stores.md
- openspec/changes/add-schema-store-sources/specs/schema-resolution/spec.md
- openspec/changes/add-schema-store-sources/specs/config-loading/spec.md
- src/commands/workflow/schemas.ts
- openspec/changes/add-schema-store-sources/specs/schema-store-sources/spec.md
- src/core/validation/validator.ts
- src/commands/workflow/shared.ts
- src/core/view.ts
- src/utils/change-utils.ts
- docs/cli.md
- src/core/completions/completion-provider.ts
- docs/stores-beta/user-guide.md
- openspec/changes/add-schema-store-sources/proposal.md
- src/utils/task-progress.ts
- src/commands/validate.ts
- src/commands/workflow/templates.ts
- docs/agent-contract.md
- openspec/changes/add-schema-store-sources/design.md
- src/commands/change.ts
- src/utils/change-metadata.ts
- src/core/root-selection.ts
- src/cli/index.ts
- src/core/artifact-graph/resolver.ts
- src/commands/workflow/instructions.ts
- src/commands/schema.ts
Summary
schemaStoreproject configuration that selects a registered Store independently from the Planning Store*visibility by defaultschemaStoreis absentschemasandtemplatesresolveschemaStorefrom the nearest consumer project. This PR does not add--storeto those commands and does not modify generated OpenSpec skills.Motivation
Departments may want specs and changes to live in one shared Planning Store while workflow schemas and templates are maintained in a separate Store. This lets different teams reuse department-level schemas without coupling schema ownership to the repository that owns planning artifacts.
Example:
Schema Store synchronization remains user-managed through normal Git operations; OpenSpec reads the registered local checkout and performs no fetch, pull, clone, or push during schema resolution.
Validation
pnpm exec openspec validate add-schema-store-sources --strictpnpm run buildpnpm exec tsc --noEmitpnpm lintgit diff --checkSummary by CodeRabbit
New Features
schemaStoresupport to source workflow schemas and templates from a registered Store, independently from planning, with visibility controls (wildcards and allowlists) and Store precedence.schemaStoreis configured.Documentation
schemaStoreconfiguration, precedence/diagnostics, wildcard/allowlist semantics, and the fact that Stores are read from their local checkout.Tests