feat(schema): add extends field for single-level schema inheritance#1005
feat(schema): add extends field for single-level schema inheritance#1005jiehu5114 wants to merge 4 commits into
Conversation
Added optional 'extends' field for schema inheritance.
…s with extends Skip requires validation for schemas using 'extends' and export validateNoCycles function for external use.
Added cycle validation and updated schema loading logic.
📝 WalkthroughWalkthroughThis PR implements schema inheritance via an Changes
Sequence DiagramsequenceDiagram
participant Caller as Caller
participant Resolver as Resolver
participant Schema as Schema Parser
participant Parent as Parent File
participant Validator as Validator
Caller->>Resolver: resolveSchema(childPath)
Resolver->>Schema: parseSchema(childContent)
Schema-->>Resolver: schema (with extends field)
alt Schema has extends
Resolver->>Parent: Load parent schema file
Parent-->>Resolver: parentContent
Resolver->>Schema: parseSchema(parentContent)
Schema-->>Resolver: parentSchema
Resolver->>Resolver: Merge artifacts<br/>(parent base + child overrides)
Resolver->>Resolver: Select apply<br/>(child or parent)
Resolver->>Validator: validateNoCycles(mergedArtifacts)
Validator-->>Resolver: ✓ or error
alt Validation passes
Resolver-->>Caller: Merged schema
else Validation fails
Resolver-->>Caller: SchemaLoadError
end
else No extends
Resolver-->>Caller: Parsed schema
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
Is there a reason that using |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
test/core/artifact-graph/schema.test.ts (1)
207-234: Tests look good; consider adding one for the skipped-validation behavior.These two tests verify that
extendsis preserved/omitted, but the more interesting schema-level change atschema.tslines 38-47 is thatparseSchemanow skipsrequiresreference and cycle validation whenextendsis present. That contract isn't directly tested here. A small addition would lock it in:♻️ Suggested extra test
it('should skip cross-boundary requires validation when extends is set', () => { const yaml = ` name: child version: 1 extends: some-parent artifacts: - id: child-art generates: c.md description: Child artifact template: templates/c.md requires: - provided-by-parent `; // Reference to parent-provided ID must not throw at parse time; // it is validated post-merge in the resolver. expect(() => parseSchema(yaml)).not.toThrow(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/core/artifact-graph/schema.test.ts` around lines 207 - 234, Add a test that verifies parseSchema skips requires reference/cycle validation when an extends value is present: create a YAML string with name/version, extends: some-parent, and an artifact that has a requires entry referencing an ID not defined in the child, then assert that calling parseSchema(yaml) does not throw; reference parseSchema and the behavior introduced around the extends-handling logic to ensure this contract is covered by tests.src/core/artifact-graph/resolver.ts (2)
258-269: Consider extracting requires validation instead of duplicating it.This block re-implements the same logic as
validateRequiresReferencesinschema.ts, just with a different error type (SchemaLoadErrorvsSchemaValidationError). SincevalidateNoCycleswas already exported for exactly this purpose, exporting the requires validator too (or a small shared helper that returns the offending pair) would keep the two sites in sync and make future changes (e.g., better error messages) one-touch.♻️ Sketch
In
schema.ts:-function validateRequiresReferences(artifacts: Artifact[]): void { +export function validateRequiresReferences(artifacts: Artifact[]): void {In
resolver.ts:- const allIds = new Set(merged.artifacts.map(a => a.id)); - for (const artifact of merged.artifacts) { - for (const req of artifact.requires) { - if (!allIds.has(req)) { - throw new SchemaLoadError( - `Schema '${child.name}': artifact '${artifact.id}' requires '${req}', ` + - `which does not exist in the merged schema.`, - childPath - ); - } - } - } + try { + validateRequiresReferences(merged.artifacts); + } catch (err) { + const cause = err instanceof Error ? err : new Error(String(err)); + throw new SchemaLoadError( + `Schema '${child.name}' (extends '${parentName}'): ${cause.message}`, + childPath, + cause + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/artifact-graph/resolver.ts` around lines 258 - 269, The duplicate "requires" validation in resolver.ts should be replaced with a shared validator exported from schema.ts (or a small helper that returns the offending pair) so both sites stay in sync; add/export a function (e.g., validateRequiresReferences or findInvalidRequire -> returns {artifactId, missingRequire} | null) from schema.ts and call it from resolver.ts to detect missing requires on merged.artifacts, then convert its result into a SchemaLoadError using child and childPath (instead of reimplementing the loop), keeping error message formatting consistent with the new shared helper.
244-254: Merge ordering is correct; minor note onextendsandapplysemantics.The spread order (parent base → child overrides → explicit
artifacts/apply) is right, andextends: undefinedensures downstream consumers see a fully-resolved schema. Two small notes:
extends: undefinedis redundant with...rawParent(rawParent can't haveextendsby the check at line 223) and...child(which would spreadchild.extends= the parent name). Setting it toundefinedafterwards is correct, just be aware the resulting object still has theextendskey present with valueundefined— if any consumer usesinorObject.hasOwn, they'll see it.delete-after-construction or omitting via rest would drop the key entirely. Optional.child.apply ?? rawParent.apply: a child cannot opt out of an inheritedapply(e.g., "I don't want an apply section at all"). Probably fine for now but worth capturing in the docs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/artifact-graph/resolver.ts` around lines 244 - 254, The merged object currently leaves an explicit extends: undefined property and uses nullish coalescing for apply; to fix, construct merged so the extends key is removed entirely (omit it via destructuring/rest from rawParent/child or delete it from the result) so consumers don't see an explicit undefined key, and change the apply merge logic in the merged creation to prefer the child's explicit value when present (use an undefined-check: use child.apply if child has own apply, otherwise rawParent.apply) so a child can opt out; refer to the merged variable, rawParent, child, extends, apply, mergedArtifacts, newArtifacts and SchemaYaml to locate the code to update.src/core/artifact-graph/types.ts (1)
28-30: Add.min(1)to theextendsfield for consistency with other schema identifiers.The
name,id, andgeneratesfields all use.min(1)to reject empty strings. Without this constraint onextends, an empty string would pass schema validation and flow to the resolver, producing the awkward error:Schema '<child>' extends '', but '' was not found.Adding.min(1)catches this at parse time with a clear validation error.♻️ Proposed change
- extends: z.string().optional(), + extends: z.string().min(1, { error: 'extends must be a non-empty schema name' }).optional(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/artifact-graph/types.ts` around lines 28 - 30, The extends schema currently allows empty strings which passes validation and leads to confusing runtime errors; update the extends declaration (the extends field in src/core/artifact-graph/types.ts) to require a non-empty string by chaining .min(1) onto the z.string() before making it optional (e.g., z.string().min(1).optional()), matching the pattern used by name/id/generates so empty identifiers are rejected at parse time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/artifact-graph/resolver.ts`:
- Around line 233-237: The current merge in mergedArtifacts (resolver.ts)
blindly spreads override over parentArtifact so ArtifactSchema.requires (which
defaults to []) from the parsed child YAML will clobber the parent's requires
even when the child didn't explicitly set it; update the merge to preserve
parentArtifact.requires when the child did not explicitly provide requires —
either by (A) detecting explicit keys from the original parsed YAML/AST
(preferred) and only applying override.requires when present, or (B) as a
pragmatic fallback implement: requires: override.requires.length ?
override.requires : parentArtifact.requires — and update the inline comment near
mergedArtifacts to reflect the actual semantics (i.e., clarify whether this is
field-level merge with detection of explicit fields or artifact-level
replacement).
In `@src/core/artifact-graph/schema.ts`:
- Around line 38-47: The schema validate command currently only calls
parseSchema so extended schemas skip deferred checks; update the validate
command handler to detect when parsedSchema.extends is truthy and then call
resolveSchema(parsedSchema) so the resolver re-runs cross-boundary requires and
cycle checks and any errors are surfaced to the user (ensure you await/catch
resolveSchema and propagate/log its errors); alternatively, if resolveSchema
cannot be invoked in this context, make the validate command explicitly print a
clear message that validation for schemas with extends is deferred to the
resolve step.
---
Nitpick comments:
In `@src/core/artifact-graph/resolver.ts`:
- Around line 258-269: The duplicate "requires" validation in resolver.ts should
be replaced with a shared validator exported from schema.ts (or a small helper
that returns the offending pair) so both sites stay in sync; add/export a
function (e.g., validateRequiresReferences or findInvalidRequire -> returns
{artifactId, missingRequire} | null) from schema.ts and call it from resolver.ts
to detect missing requires on merged.artifacts, then convert its result into a
SchemaLoadError using child and childPath (instead of reimplementing the loop),
keeping error message formatting consistent with the new shared helper.
- Around line 244-254: The merged object currently leaves an explicit extends:
undefined property and uses nullish coalescing for apply; to fix, construct
merged so the extends key is removed entirely (omit it via destructuring/rest
from rawParent/child or delete it from the result) so consumers don't see an
explicit undefined key, and change the apply merge logic in the merged creation
to prefer the child's explicit value when present (use an undefined-check: use
child.apply if child has own apply, otherwise rawParent.apply) so a child can
opt out; refer to the merged variable, rawParent, child, extends, apply,
mergedArtifacts, newArtifacts and SchemaYaml to locate the code to update.
In `@src/core/artifact-graph/types.ts`:
- Around line 28-30: The extends schema currently allows empty strings which
passes validation and leads to confusing runtime errors; update the extends
declaration (the extends field in src/core/artifact-graph/types.ts) to require a
non-empty string by chaining .min(1) onto the z.string() before making it
optional (e.g., z.string().min(1).optional()), matching the pattern used by
name/id/generates so empty identifiers are rejected at parse time.
In `@test/core/artifact-graph/schema.test.ts`:
- Around line 207-234: Add a test that verifies parseSchema skips requires
reference/cycle validation when an extends value is present: create a YAML
string with name/version, extends: some-parent, and an artifact that has a
requires entry referencing an ID not defined in the child, then assert that
calling parseSchema(yaml) does not throw; reference parseSchema and the behavior
introduced around the extends-handling logic to ensure this contract is covered
by tests.
🪄 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
Run ID: b79e9f84-f896-4808-a502-0a15ddbca456
📒 Files selected for processing (4)
src/core/artifact-graph/resolver.tssrc/core/artifact-graph/schema.tssrc/core/artifact-graph/types.tstest/core/artifact-graph/schema.test.ts
| const mergedArtifacts = rawParent.artifacts.map(parentArtifact => { | ||
| const override = child.artifacts.find(a => a.id === parentArtifact.id); | ||
| // Field-level merge: start with parent, apply only the fields the child explicitly set | ||
| return override ? { ...parentArtifact, ...override } : parentArtifact; | ||
| }); |
There was a problem hiding this comment.
Field-level merge silently discards parent requires due to Zod defaults.
The comment claims "apply only the fields the child explicitly set," but the spread { ...parentArtifact, ...override } can't distinguish "explicitly set" from "defaulted." ArtifactSchema.requires uses .default([]), so any child override that omits requires in YAML arrives here as requires: [] after Zod parsing and overwrites the parent's requires.
Concrete regression:
# parent
artifacts:
- id: design
generates: design.md
description: Design doc
template: templates/design.md
requires: [proposal]# child — only wants to swap the template
extends: parent
artifacts:
- id: design
generates: design.md
description: Design doc
template: templates/design-v2.mdPost-merge, design.requires becomes [] (proposal dependency lost). The cycle/requires re-validation won't catch this because [] is trivially valid.
Three options to resolve:
- Document loudly that overrides replace the whole artifact and child must re-specify
requiresif it still applies — simplest, but contradicts the current inline comment and the PR's "field-level merge" claim. - Fall back when child
requiresis empty:requires: override.requires.length ? override.requires : parentArtifact.requires— pragmatic but hides a real empty-requires override. - Re-parse from raw YAML to know which keys the child explicitly set (the only way to correctly implement the documented semantics). Most work, cleanest result.
Given the PR description explicitly says "field-level merge (child wins)," option 1 alone is likely to surprise users. At minimum, please either adjust the behavior or update the inline comment and PR docs to say "artifact-level replacement with optional-field inheritance."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/artifact-graph/resolver.ts` around lines 233 - 237, The current
merge in mergedArtifacts (resolver.ts) blindly spreads override over
parentArtifact so ArtifactSchema.requires (which defaults to []) from the parsed
child YAML will clobber the parent's requires even when the child didn't
explicitly set it; update the merge to preserve parentArtifact.requires when the
child did not explicitly provide requires — either by (A) detecting explicit
keys from the original parsed YAML/AST (preferred) and only applying
override.requires when present, or (B) as a pragmatic fallback implement:
requires: override.requires.length ? override.requires : parentArtifact.requires
— and update the inline comment near mergedArtifacts to reflect the actual
semantics (i.e., clarify whether this is field-level merge with detection of
explicit fields or artifact-level replacement).
| // When a schema uses `extends`, child artifacts may reference IDs provided by the | ||
| // parent. Skip cross-boundary requires validation here — it runs post-merge in | ||
| // resolver.ts after the full artifact set is assembled. | ||
| if (!schema.extends) { | ||
| // Check that all requires references are valid | ||
| validateRequiresReferences(schema.artifacts); | ||
|
|
||
| // Check for cycles | ||
| validateNoCycles(schema.artifacts); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how the validate command handles schemas with `extends`.
rg -nP --type=ts -C5 '\bparseSchema\s*\(' src/commands/schema.ts
rg -nP --type=ts -C3 '\bresolveSchema\s*\(' src/commands/schema.tsRepository: Fission-AI/OpenSpec
Length of output: 914
🏁 Script executed:
# Get fuller context around the validate command
rg -nP --type=ts -B10 -A20 'Validating schema structure' src/commands/schema.tsRepository: Fission-AI/OpenSpec
Length of output: 926
🏁 Script executed:
# Check if resolveSchema is called at all in the validate command and surrounding context
rg -nP --type=ts 'resolveSchema' src/commands/schema.tsRepository: Fission-AI/OpenSpec
Length of output: 45
🏁 Script executed:
# Look for validation-related calls after the parseSchema call
rg -nP --type=ts -A30 'let schema: SchemaYaml' src/commands/schema.ts | grep -A30 'parseSchema(content)'Repository: Fission-AI/OpenSpec
Length of output: 1142
🏁 Script executed:
# Get the full validate function
rg -nP --type=ts -B5 -A50 'function validateSchema' src/commands/schema.ts | head -100Repository: Fission-AI/OpenSpec
Length of output: 1584
🏁 Script executed:
# Verify the conditional skip in schema.ts
cat -n src/core/artifact-graph/schema.ts | sed -n '35,50p'Repository: Fission-AI/OpenSpec
Length of output: 700
🏁 Script executed:
# Check for any messages about extended schemas in the validate function
rg -nP --type=ts -A100 'function validateSchema' src/commands/schema.ts | grep -i -E '(extends|deferred|validation|resolve)'Repository: Fission-AI/OpenSpec
Length of output: 361
🏁 Script executed:
# Get the end of the validate function
rg -nP --type=ts -A80 'Check template files exist' src/commands/schema.ts | head -60Repository: Fission-AI/OpenSpec
Length of output: 2104
🏁 Script executed:
# Search for resolveSchema definition
rg -nP --type=ts 'function resolveSchema|resolveSchema\s*=' src/Repository: Fission-AI/OpenSpec
Length of output: 256
🏁 Script executed:
# Check resolver.ts to see if it calls validateRequiresReferences or validateNoCycles
rg -nP --type=ts '(validateRequiresReferences|validateNoCycles)' src/core/artifact-graph/Repository: Fission-AI/OpenSpec
Length of output: 634
🏁 Script executed:
# Check resolveSchema function
cat -n src/core/artifact-graph/resolver.ts | sed -n '109,130p'Repository: Fission-AI/OpenSpec
Length of output: 1042
🏁 Script executed:
# Check what happens around the validateNoCycles call in resolver.ts
cat -n src/core/artifact-graph/resolver.ts | sed -n '270,280p'Repository: Fission-AI/OpenSpec
Length of output: 565
🏁 Script executed:
# Check if validate command mentions extended schemas
rg -nP --type=ts -B5 -A5 'extends' src/commands/schema.ts | head -40Repository: Fission-AI/OpenSpec
Length of output: 45
🏁 Script executed:
# Check if there's any warning or message about partial validation
rg -nP --type=ts 'extends\|deferred\|partial' src/commands/schema.tsRepository: Fission-AI/OpenSpec
Length of output: 45
Validate command does not fully validate extended schemas.
The skip in parseSchema is correct for the happy path (resolver re-runs checks post-merge), but the schema validate command (src/commands/schema.ts:175-196) only calls parseSchema and does not call resolveSchema. For a schema with extends, dangling requires references or cycles introduced by the child will pass validation silently without warning the user.
The comment at line 217-220 ("Dependency graph validation is already done by parseSchema") is misleading—this is only true for non-extended schemas.
Please either:
- Call
resolveSchemain the validate command for extended schemas to re-run the deferred checks, or - Surface an explicit message to the user indicating that validation for extended schemas is deferred to the resolve step.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/artifact-graph/schema.ts` around lines 38 - 47, The schema validate
command currently only calls parseSchema so extended schemas skip deferred
checks; update the validate command handler to detect when parsedSchema.extends
is truthy and then call resolveSchema(parsedSchema) so the resolver re-runs
cross-boundary requires and cycle checks and any errors are surfaced to the user
(ensure you await/catch resolveSchema and propagate/log its errors);
alternatively, if resolveSchema cannot be invoked in this context, make the
validate command explicitly print a clear message that validation for schemas
with extends is deferred to the resolve step.
2 participants
Closes #778
Problem
openspec schema forkcopies a schema flat — no link back to the original. If you want to change one artifact or add an extra step, you still have to duplicate the entire file. With multiple platform schemas (iOS, Android, HarmonyOS, etc.) sharing 80% of the same artifacts, this creates real maintenance pain: fix a typo in one, repeat it in all the others.Solution
Add an optional
extendsfield. A child schema names its parent and only declares what it wants to override or add.Merge rules
applyblock (if present) replaces the parent's; otherwise parent's is inherited.extendsis flattened away — callers get a normalSchemaYamlwith noextendsfield.Constraints
extends, resolution throwsSchemaLoadError. Keeping it shallow makes behavior easy to reason about.requires— validation runs post-merge against the full artifact set.extendsare completely unaffected.Changes
types.ts— add optionalextends: stringtoSchemaYamlSchemaschema.ts— exportvalidateNoCycles; skip cross-boundaryrequiresvalidation whenextendsis present (deferred to post-merge)resolver.ts— resolve inheritance inresolveSchemaviamergeWithParentresolver.test.ts— 9 new test cases covering inherit, override, append, apply fallback, missing parent, and two-level rejectionschema.test.ts— 2 new test cases forextendsfield parsingSummary by CodeRabbit
New Features
Tests