proposal: add artifact graph core query system #400
Conversation
Add OpenSpec change proposal for Slice 1 of the artifact POC - the core "What's Ready?" query system. This implements: - ArtifactGraph class for DAG-based dependency modeling - Filesystem-based state detection (file existence = completion) - Topological sort for build order calculation - Ready/blocked artifact queries This is a parallel module that will coexist with the current system.
📝 WalkthroughWalkthroughAdds a new artifact-graph core: YAML/Zod schema parsing and validation, XDG-aware schema resolution with global overrides, built-in schemas, a deterministic Kahn topological sort, filesystem/glob-based completion detection, and APIs for build order, ready/blocked queries, and schema listing. Changes
Sequence Diagram(s)sequenceDiagram
rect rgb(250,250,250)
participant User as User
participant Resolver as SchemaResolver
participant FS as FileSystem
participant Parser as SchemaParser
participant Graph as ArtifactGraph
participant State as StateDetector
end
User->>Resolver: resolveSchema(name)
Resolver->>FS: check XDG global override (.yaml/.yml)
alt override exists
FS-->>Resolver: file contents
Resolver->>Parser: parseSchema(contents)
Parser-->>Resolver: SchemaYaml or SchemaValidationError
else fallback
Resolver-->>Resolver: return BUILTIN_SCHEMAS[name]
end
Resolver-->>User: SchemaYaml or SchemaLoadError
User->>Graph: ArtifactGraph.fromSchema(schema)
Graph->>Parser: validate (Zod, duplicates, requires)
Parser-->>Graph: validated schema
Graph->>Graph: detect cycles (DFS)
Graph->>Graph: compute buildOrder (Kahn)
User->>State: detectCompleted(graph, changeDir)
State->>FS: check each artifact 'generates' (exists or fast-glob)
FS-->>State: match results
State-->>User: CompletedSet
User->>Graph: getNextArtifacts(CompletedSet)
Graph-->>User: ready artifact IDs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
- Add decision section for Zod schema validation in design.md - Update data structures to show Zod schemas with z.infer<> types - Update tasks to specify Zod usage for type definitions and parsing
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
openspec/changes/add-artifact-graph-core/tasks.md (2)
23-39: Incomplete test coverage for ready-calculation features.Tasks 5.1 (
getNextArtifacts) and 5.3 (getBlocked) are implementation features, but Section 7 (Testing) lacks explicit test tasks for both. While 7.3–7.4 implicitly validate readiness logic, tests 7.8 and 7.9 should explicitly verifygetNextArtifacts(graph, state)andgetBlocked(graph, state)behavior (including edge cases with complex dependency chains and partial completion).🔎 Suggested test tasks to add
- [ ] 7.7 Test: Cycle in schema throws clear error + [ ] 7.7 Test: Cycle in schema throws clear error + [ ] 7.8 Test: getNextArtifacts() returns correct set when some deps incomplete + [ ] 7.9 Test: getBlocked() identifies blocked artifacts and their blockers
5-8: Clarify error handling and validation scope in schema parsing.Task 2.1 mentions safeParse for validation but doesn't explicitly cover error recovery, reporting, or logging. Task 2.2 validates dependency references but doesn't specify what happens when a
requiresreference points to a non-existent artifact ID. Consider adding subtasks for:
- Error message clarity and format
- Logging or telemetry for parsing failures
- Recovery strategy (fail-fast vs. lenient)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
openspec/changes/add-artifact-graph-core/design.mdopenspec/changes/add-artifact-graph-core/tasks.md
🧰 Additional context used
📓 Path-based instructions (2)
openspec/changes/**/*.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
Scaffold proposal using
proposal.md,tasks.md, optionaldesign.md, and delta specs underopenspec/changes/<id>/
Files:
openspec/changes/add-artifact-graph-core/tasks.mdopenspec/changes/add-artifact-graph-core/design.md
openspec/changes/*/tasks.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
Ensure
tasks.mdcontains implementation checklist with numbered sections and checkbox items
Files:
openspec/changes/add-artifact-graph-core/tasks.md
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Applies to openspec/changes/**/*.md : Scaffold proposal using `proposal.md`, `tasks.md`, optional `design.md`, and delta specs under `openspec/changes/<id>/`
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Create `design.md` only when needed: cross-cutting changes, new external dependencies, significant data model changes, security/performance complexity, or pre-coding ambiguity
📚 Learning: 2025-11-25T01:08:19.004Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Applies to openspec/changes/**/*.md : Scaffold proposal using `proposal.md`, `tasks.md`, optional `design.md`, and delta specs under `openspec/changes/<id>/`
Applied to files:
openspec/changes/add-artifact-graph-core/tasks.mdopenspec/changes/add-artifact-graph-core/design.md
📚 Learning: 2025-11-25T01:08:19.004Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Create `design.md` only when needed: cross-cutting changes, new external dependencies, significant data model changes, security/performance complexity, or pre-coding ambiguity
Applied to files:
openspec/changes/add-artifact-graph-core/tasks.mdopenspec/changes/add-artifact-graph-core/design.md
📚 Learning: 2025-11-25T01:08:19.004Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Applies to openspec/changes/*/tasks.md : Ensure `tasks.md` contains implementation checklist with numbered sections and checkbox items
Applied to files:
openspec/changes/add-artifact-graph-core/tasks.md
🪛 markdownlint-cli2 (0.18.1)
openspec/changes/add-artifact-graph-core/design.md
124-124: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (3)
openspec/changes/add-artifact-graph-core/tasks.md (1)
1-39: Task checklist is well-structured and aligns with design decisions.The scaffold properly reflects the PR design: Zod-based schema validation (Section 1), Kahn's algorithm for topological sort (Section 3), filesystem-backed state detection with glob support (Section 4), and complete readiness query API (Section 5). Logical decomposition across sections is clear, and tasks flow naturally from types through integration and testing. Based on learnings, the numbered-section-with-checkbox format matches OpenSpec conventions.
openspec/changes/add-artifact-graph-core/design.md (2)
1-144: Comprehensive, well-justified design document.This design.md is appropriately scoped and justified per project learnings (new external dependency, significant data model changes, architectural pre-coding decisions). The document clearly articulates the problem (filesystem as database for stateless artifact tracking), documents five well-reasoned architectural decisions with alternatives, and provides concrete data structures and file organization. The Zod choice in particular demonstrates good dependency stewardship (reusing existing project dependencies rather than introducing new ones).
Minor fixes needed: language specifier on code block (line 124) and verification of Zod version claim (lines 66–67).
62-74: Zod dependency claim is accurate.The design document correctly states that Zod is already a project dependency (v4.0.17) with existing usage in
src/core/schemas/. The referenced files (base.schema.ts,config-schema.ts) and patterns are confirmed in the codebase, validating the rationale for the design decision.
| ``` | ||
| src/core/artifact-graph/ | ||
| ├── index.ts # Public exports | ||
| ├── types.ts # Type definitions | ||
| ├── graph.ts # ArtifactGraph class | ||
| ├── state.ts # State detection logic | ||
| └── schema.ts # YAML schema parser | ||
| ``` |
There was a problem hiding this comment.
Add language specifier to fenced code block.
The file structure code block should specify a language. Use text for file tree diagrams.
🔎 Proposed fix
-```
+```text
src/core/artifact-graph/
├── index.ts # Public exports
├── types.ts # Type definitions
├── graph.ts # ArtifactGraph class
├── state.ts # State detection logic
└── schema.ts # YAML schema parser
-```
+```🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
124-124: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In openspec/changes/add-artifact-graph-core/design.md around lines 124 to 131,
the fenced code block containing the file tree lacks a language specifier;
update the opening fence to use ```text so the file tree is treated as plain
text (keep the content unchanged and ensure the closing ``` remains present).
- Add decision for global → built-in schema resolution pattern - Add resolver.ts for schema lookup logic - Add built-in schemas directory (spec-driven.yaml, tdd.yaml) - Add schema resolution tests - Follows ESLint/Prettier/Git patterns (defaults baked in package)
Creates add-artifact-graph-core-v2 with requirements organized as vertical slices - each requirement file contains its spec, design decisions, and tasks bundled together for comparison.
Add getGlobalDataDir() function following XDG Base Directory Specification for storing user data like schema overrides: - XDG_DATA_HOME takes precedence on all platforms - Unix/macOS fallback: ~/.local/share/openspec/ - Windows fallback: %LOCALAPPDATA%/openspec/
Implement Slice 1 ("What's Ready?") of the artifact graph system:
- types.ts: Zod schemas for artifact definitions with derived TypeScript types
- schema.ts: YAML parsing with validation for duplicates, invalid refs, cycles
- graph.ts: ArtifactGraph class with Kahn's algorithm for topological sort
- state.ts: Filesystem-based completion detection with glob pattern support
- resolver.ts: Two-level schema resolution (global override → built-in)
- builtin-schemas.ts: spec-driven and tdd workflow definitions
Key design decisions:
- Filesystem as database (stateless, git-friendly)
- Cycle errors show full path (e.g., "A → B → C → A")
- Deterministic ordering via sorted queues
52 tests covering all artifact-graph functionality: - schema.test.ts: Parsing, validation errors, cycle detection - graph.test.ts: Build order, ready artifacts, blocked queries - state.test.ts: File existence, glob patterns, missing directories - resolver.test.ts: Schema resolution with global overrides
Archive completed change proposal and create artifact-graph spec with 6 requirements covering schema loading, build order, state detection, ready queries, completion checks, and blocked queries.
Clean up experimental vertical slice proposal that is no longer needed.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
openspec/changes/archive/2025-12-24-add-artifact-graph-core/design.md (1)
171-171: Add language specifier to fenced code block.The file structure code block should specify a language. Use
textfor file tree diagrams.🔎 Proposed fix
-``` +```text src/core/artifact-graph/ ├── index.ts # Public exports ├── types.ts # Zod schemas and type definitionsBased on past review comments and static analysis hints.
🧹 Nitpick comments (4)
openspec/specs/artifact-graph/spec.md (1)
3-4: Update the TBD Purpose section.The Purpose section is marked "TBD" and mentions updating after archive, but this proposal has already been archived. Please update the Purpose to describe the artifact-graph capability's role in the system.
src/core/artifact-graph/state.ts (1)
36-46: Consider path traversal edge cases.If
generatescontains..or an absolute path,path.joinmay resolve outsidechangeDir. For example,path.join('changeDir', '../secret')resolves tosecret. Since schemas are typically user-controlled in this context, the risk is low, but worth noting if schemas might come from less trusted sources in the future.🔎 Optional: Add path containment check
function isArtifactComplete(generates: string, changeDir: string): boolean { const fullPattern = path.join(changeDir, generates); + const resolved = path.resolve(fullPattern); + const resolvedChangeDir = path.resolve(changeDir); + if (!resolved.startsWith(resolvedChangeDir + path.sep) && resolved !== resolvedChangeDir) { + return false; // Path escapes changeDir + } // Check if it's a glob pattern if (isGlobPattern(generates)) { return hasGlobMatches(fullPattern); }src/core/artifact-graph/graph.ts (1)
33-38: Consider defensive validation forfromSchema.The docstring states this is for "pre-validated schema object", but if an unvalidated schema is passed (e.g., from BUILTIN_SCHEMAS which bypass parseSchema), invalid
requiresreferences could cause runtime errors at line 85. The built-in schemas are correct, but this is a potential footgun for future maintainers.🔎 Optional: Add minimal validation
static fromSchema(schema: SchemaYaml): ArtifactGraph { + // Minimal validation: ensure all requires references exist + const ids = new Set(schema.artifacts.map(a => a.id)); + for (const artifact of schema.artifacts) { + for (const req of artifact.requires) { + if (!ids.has(req)) { + throw new Error(`Invalid schema: artifact '${artifact.id}' requires unknown '${req}'`); + } + } + } return new ArtifactGraph(schema); }src/core/artifact-graph/resolver.ts (1)
64-78: LGTM!The function correctly combines built-in and global schemas, deduplicates via Set, and returns a sorted list. The extension normalization is consistent with
resolveSchema.Optionally, you could wrap the
readdirSynccall in a try-catch to handle edge cases where directory permissions change between theexistsSynccheck and thereaddirSynccall (TOCTOU), though this is unlikely in practice:🔎 Optional defensive coding
// Add user override schemas const globalDir = path.join(getGlobalDataDir(), 'schemas'); if (fs.existsSync(globalDir)) { - for (const file of fs.readdirSync(globalDir)) { - if (file.endsWith('.yaml') || file.endsWith('.yml')) { - schemas.add(file.replace(/\.ya?ml$/, '')); + try { + for (const file of fs.readdirSync(globalDir)) { + if (file.endsWith('.yaml') || file.endsWith('.yml')) { + schemas.add(file.replace(/\.ya?ml$/, '')); + } } + } catch { + // Directory became inaccessible - skip user overrides } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
openspec/changes/archive/2025-12-24-add-artifact-graph-core/design.mdopenspec/changes/archive/2025-12-24-add-artifact-graph-core/proposal.mdopenspec/changes/archive/2025-12-24-add-artifact-graph-core/specs/artifact-graph/spec.mdopenspec/changes/archive/2025-12-24-add-artifact-graph-core/tasks.mdopenspec/specs/artifact-graph/spec.mdpackage.jsonsrc/core/artifact-graph/builtin-schemas.tssrc/core/artifact-graph/graph.tssrc/core/artifact-graph/index.tssrc/core/artifact-graph/resolver.tssrc/core/artifact-graph/schema.tssrc/core/artifact-graph/state.tssrc/core/artifact-graph/types.tssrc/core/global-config.tssrc/core/index.tstest/core/artifact-graph/graph.test.tstest/core/artifact-graph/resolver.test.tstest/core/artifact-graph/schema.test.tstest/core/artifact-graph/state.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
openspec/specs/**/spec.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
Use SHALL/MUST for normative requirements in spec files; avoid should/may unless intentionally non-normative
Files:
openspec/specs/artifact-graph/spec.md
openspec/changes/**/*.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
Scaffold proposal using
proposal.md,tasks.md, optionaldesign.md, and delta specs underopenspec/changes/<id>/
Files:
openspec/changes/archive/2025-12-24-add-artifact-graph-core/design.mdopenspec/changes/archive/2025-12-24-add-artifact-graph-core/specs/artifact-graph/spec.mdopenspec/changes/archive/2025-12-24-add-artifact-graph-core/proposal.mdopenspec/changes/archive/2025-12-24-add-artifact-graph-core/tasks.md
openspec/changes/**/specs/**/spec.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
openspec/changes/**/specs/**/spec.md: Use## ADDED|MODIFIED|REMOVED|RENAMED Requirementsheaders in spec delta files
Include at least one#### Scenario:per requirement in spec delta files
Use#### Scenario: Nameformat (4 hashtags) for scenario headers, not bullets or bold text
Use## ADDED Requirementsfor new orthogonal capabilities that can stand alone; use## MODIFIED Requirementsfor behavior changes of existing requirements
When using MODIFIED Requirements, paste the full requirement block including header and all scenarios
Files:
openspec/changes/archive/2025-12-24-add-artifact-graph-core/specs/artifact-graph/spec.md
🧠 Learnings (9)
📓 Common learnings
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Applies to openspec/changes/**/*.md : Scaffold proposal using `proposal.md`, `tasks.md`, optional `design.md`, and delta specs under `openspec/changes/<id>/`
📚 Learning: 2025-11-25T01:08:19.004Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Applies to openspec/changes/**/specs/**/spec.md : Use `## ADDED|MODIFIED|REMOVED|RENAMED Requirements` headers in spec delta files
Applied to files:
openspec/specs/artifact-graph/spec.mdopenspec/changes/archive/2025-12-24-add-artifact-graph-core/specs/artifact-graph/spec.md
📚 Learning: 2025-11-25T01:08:19.004Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Applies to openspec/changes/**/*.md : Scaffold proposal using `proposal.md`, `tasks.md`, optional `design.md`, and delta specs under `openspec/changes/<id>/`
Applied to files:
openspec/specs/artifact-graph/spec.mdopenspec/changes/archive/2025-12-24-add-artifact-graph-core/design.mdopenspec/changes/archive/2025-12-24-add-artifact-graph-core/specs/artifact-graph/spec.mdopenspec/changes/archive/2025-12-24-add-artifact-graph-core/tasks.md
📚 Learning: 2025-11-25T01:08:19.004Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Applies to openspec/changes/**/specs/**/spec.md : Use `## ADDED Requirements` for new orthogonal capabilities that can stand alone; use `## MODIFIED Requirements` for behavior changes of existing requirements
Applied to files:
openspec/specs/artifact-graph/spec.mdopenspec/changes/archive/2025-12-24-add-artifact-graph-core/specs/artifact-graph/spec.md
📚 Learning: 2025-11-25T01:08:19.004Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Create `design.md` only when needed: cross-cutting changes, new external dependencies, significant data model changes, security/performance complexity, or pre-coding ambiguity
Applied to files:
openspec/changes/archive/2025-12-24-add-artifact-graph-core/design.mdopenspec/changes/archive/2025-12-24-add-artifact-graph-core/tasks.md
📚 Learning: 2025-11-25T01:08:19.004Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Use `file.ts:42` format for code location references and `specs/auth/spec.md` format for spec references
Applied to files:
openspec/changes/archive/2025-12-24-add-artifact-graph-core/design.md
📚 Learning: 2025-11-25T01:08:19.004Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Applies to openspec/changes/**/specs/**/spec.md : Include at least one `#### Scenario:` per requirement in spec delta files
Applied to files:
openspec/changes/archive/2025-12-24-add-artifact-graph-core/specs/artifact-graph/spec.md
📚 Learning: 2025-11-25T01:08:19.004Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Applies to openspec/changes/**/specs/**/spec.md : When using MODIFIED Requirements, paste the full requirement block including header and all scenarios
Applied to files:
openspec/changes/archive/2025-12-24-add-artifact-graph-core/specs/artifact-graph/spec.md
📚 Learning: 2025-11-25T01:08:19.004Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Applies to openspec/changes/*/tasks.md : Ensure `tasks.md` contains implementation checklist with numbered sections and checkbox items
Applied to files:
openspec/changes/archive/2025-12-24-add-artifact-graph-core/tasks.md
🧬 Code graph analysis (3)
src/core/artifact-graph/types.ts (1)
src/core/artifact-graph/index.ts (6)
ArtifactSchema(3-3)SchemaYamlSchema(4-4)Artifact(5-5)SchemaYaml(6-6)CompletedSet(7-7)BlockedArtifacts(8-8)
src/core/artifact-graph/state.ts (3)
src/core/artifact-graph/index.ts (3)
detectCompleted(18-18)ArtifactGraph(15-15)CompletedSet(7-7)src/core/artifact-graph/graph.ts (1)
ArtifactGraph(8-167)src/core/artifact-graph/types.ts (1)
CompletedSet(27-27)
src/core/artifact-graph/resolver.ts (2)
src/core/artifact-graph/types.ts (1)
SchemaYaml(22-22)src/core/artifact-graph/builtin-schemas.ts (1)
BUILTIN_SCHEMAS(81-84)
🪛 markdownlint-cli2 (0.18.1)
openspec/changes/archive/2025-12-24-add-artifact-graph-core/design.md
171-171: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (19)
openspec/changes/archive/2025-12-24-add-artifact-graph-core/specs/artifact-graph/spec.md (2)
1-103: Scenario count in spec exceeds PR objective.The PR objective states "6 requirements and 15 scenarios," but this spec file contains 21 scenarios across the 6 requirements (Schema Loading: 5, Build Order Calculation: 3, State Detection: 5, Ready Artifact Query: 3, Completion Check: 2, Blocked Query: 3). This expansion during implementation is common and acceptable; however, confirm whether the PR objective should be updated to reflect 21 scenarios, or whether some scenarios should be consolidated.
1-103: Spec format and structure fully compliant with coding guidelines.All requirements and scenarios follow the prescribed format:
- ✓ Uses
## ADDED Requirementsheader for new orthogonal capabilities- ✓ All 6 requirements include at least one scenario
- ✓ All 21 scenarios use
#### Scenario: Nameformat (4 hashtags)- ✓ Consistent WHEN/THEN structure with clear, observable outcomes
- ✓ Comprehensive coverage of happy path, error cases, and edge cases (e.g., cyclic dependencies, missing directories, glob patterns)
As per coding guidelines, the spec structure is correct and ready.
src/core/artifact-graph/types.ts (1)
1-33: LGTM! Well-structured type definitions.The Zod schemas and TypeScript types are well-defined with clear validation rules. The separation between runtime validation schemas and internal state types is clean and follows good practices.
package.json (1)
76-78: LGTM! Appropriate dependencies for artifact-graph functionality.The additions of
fast-glob(for glob pattern matching in state detection) andyaml(for schema parsing) are well-justified by the artifact-graph implementation.src/core/index.ts (1)
5-11: LGTM! Consistent public API expansion.The new exports for global data directory functionality follow the existing pattern established by
GLOBAL_CONFIG_DIR_NAMEandgetGlobalConfigDir(), maintaining API consistency.src/core/global-config.ts (1)
49-78: LGTM! Proper XDG Base Directory Specification compliance.The
getGlobalDataDir()implementation correctly follows the XDG Base Directory Specification, distinguishing between data and config directories. The platform-specific fallbacks are appropriate:
- Uses
LOCALAPPDATA(notAPPDATA) for Windows data storage- Uses
~/.local/share(not~/.config) for Unix/macOS data storagetest/core/artifact-graph/graph.test.ts (1)
1-268: LGTM! Comprehensive and well-structured test suite.The test suite thoroughly covers all ArtifactGraph functionality:
- Graph construction (from schema object and YAML)
- Artifact retrieval and listing
- Build order calculation (linear, diamond, independent scenarios)
- Ready artifact determination with various dependency states
- Completion checking
- Blocked artifact identification
The tests validate both happy paths and edge cases (undefined artifacts, empty states, partial completion).
src/core/artifact-graph/builtin-schemas.ts (1)
1-84: LGTM!The built-in schema definitions are well-structured with clear dependency relationships. Embedding them as compile-time constants is a good design choice to avoid runtime file resolution issues.
test/core/artifact-graph/resolver.test.ts (1)
1-145: LGTM!Comprehensive test coverage for the resolver module. Good isolation of environment variables and proper cleanup of temp directories. The tests cover important edge cases including extension normalization, global override precedence, deduplication, and sorted output.
test/core/artifact-graph/state.test.ts (1)
1-174: LGTM!Excellent test coverage for the state detection module. The tests thoroughly cover edge cases including non-existent directories, glob patterns (with empty/non-matching scenarios), nested paths, and mixed completion states. The
createSchemahelper keeps tests clean and readable.test/core/artifact-graph/schema.test.ts (1)
1-207: LGTM!Thorough test coverage for schema validation including Zod parsing errors, duplicate ID detection, invalid dependency references, and cycle detection with path reporting. The tests verify that error messages contain meaningful information for debugging.
src/core/artifact-graph/schema.ts (1)
1-124: LGTM!Well-implemented schema validation with clear separation of concerns. The DFS-based cycle detection correctly reconstructs the full cycle path for helpful error messages. The validation order (Zod → duplicates → references → cycles) is logical, ensuring earlier checks catch simpler errors before more complex analysis.
src/core/artifact-graph/index.ts (1)
1-24: LGTM!Clean and well-organized barrel file with logical grouping of exports. The comments improve discoverability of the public API surface.
src/core/artifact-graph/state.ts (1)
51-53: Note: Glob pattern detection is minimal but sufficient for stated use cases.The check covers
*,?, and[but doesn't detect brace expansion{a,b}or globstar**. This is fine if the documented schema patterns only use the covered syntax, but worth documenting if users expect full glob support.src/core/artifact-graph/graph.ts (2)
68-113: LGTM! Clean Kahn's algorithm implementation.The topological sort is correctly implemented with deterministic ordering through sorting at initial queue population (line 92) and when adding newly ready artifacts (line 109). This ensures consistent build order across runs.
115-166: LGTM!The
getNextArtifacts,isComplete, andgetBlockedmethods are cleanly implemented with appropriate sorting for determinism. The separation of concerns is good—state detection is external, and the graph only reasons about dependencies.src/core/artifact-graph/resolver.ts (2)
1-6: LGTM!Imports are appropriate and clearly support the two-level schema resolution approach (global overrides via filesystem, built-in schemas as fallback).
46-58: LGTM!The helper correctly checks both
.yamland.ymlextensions and returns the path if found. The synchronousexistsSynccheck is appropriate for this use case.openspec/changes/archive/2025-12-24-add-artifact-graph-core/tasks.md (1)
1-62: LGTM!The tasks file is well-structured and comprehensive:
- Clear numbered sections covering all implementation areas (types, parsing, graph core, state detection, ready calculation, schema resolution, built-in schemas, integration, testing)
- Explicit file paths for each component
- Detailed test coverage enumeration (19 test scenarios)
- All tasks appropriately marked complete for an archived proposal
The structure follows the coding guidelines for proposal scaffolding.
Based on learnings, the file correctly uses the prescribed
tasks.mdformat with numbered sections and checkbox items.
Global schema overrides are now validated through the same pipeline as built-in schemas, catching invalid schemas, cyclic dependencies, and invalid requires references at load time. Added SchemaLoadError for better error context with file paths.
Add end-to-end integration tests that exercise the full artifact-graph pipeline: resolveSchema → ArtifactGraph → detectCompleted → queries. Tests cover: - Complete spec-driven and tdd workflow progressions - Out-of-order file creation handling - Glob pattern matching with multiple files - Build order consistency - Edge cases (empty/missing directories, non-matching files)
Update custom error messages from string format to zod v4 object format
using `{ error: 'message' }` convention.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/core/artifact-graph/workflow.integration.test.ts (2)
52-74: Consider consistent use of normalizeBlocked.There's minor inconsistency in when
normalizeBlockedis applied:
- Line 52: Used (handles multi-dependency case)
- Line 63: Used (single dependency, defensive)
- Line 72: Not used (single dependency)
While this doesn't affect correctness, consistent usage (either always normalize or only for multi-dependency cases) would improve readability.
🔎 Optional: Apply normalizeBlocked consistently
expect(graph.getNextArtifacts(completed)).toEqual(['specs']); - expect(graph.getBlocked(completed)).toEqual({ + expect(normalizeBlocked(graph.getBlocked(completed))).toEqual({ tasks: ['specs'], });
59-65: Consider adding a comment explaining the sort.Line 62 is the only place where
getNextArtifacts()result is sorted before comparison. This is appropriate because both 'design' and 'specs' become ready simultaneously (parallel dependencies on 'proposal'), and the order between them may not be guaranteed. A brief comment would clarify this intent for future maintainers.🔎 Optional: Add explanatory comment
fs.writeFileSync(path.join(tempDir, 'proposal.md'), '# Proposal\n\nInitial proposal content.'); completed = detectCompleted(graph, tempDir); expect(completed).toEqual(new Set(['proposal'])); + // Both design and specs become ready simultaneously; sort for stable comparison expect(graph.getNextArtifacts(completed).sort()).toEqual(['design', 'specs']); expect(normalizeBlocked(graph.getBlocked(completed))).toEqual({
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/core/artifact-graph/types.tstest/core/artifact-graph/workflow.integration.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/artifact-graph/types.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Applies to openspec/changes/**/*.md : Scaffold proposal using `proposal.md`, `tasks.md`, optional `design.md`, and delta specs under `openspec/changes/<id>/`
🔇 Additional comments (9)
test/core/artifact-graph/workflow.integration.test.ts (9)
1-9: LGTM! Clean imports with modern conventions.The imports are well-organized and use the
node:prefix for built-in modules, which is a best practice. All necessary modules for integration testing are included.
10-20: LGTM! Well-documented helper function.The
normalizeBlockedhelper correctly sorts dependency arrays for stable assertions, and the JSDoc clearly explains why this is necessary. The implementation properly creates a new object and uses the spread operator to avoid mutating the input arrays.
25-35: LGTM! Proper test isolation and cleanup.Excellent use of unique temp directories per test with
mkdtempSyncfor isolation, and robust cleanup with existence checks and force removal. This ensures tests don't interfere with each other and leave no artifacts behind.
38-92: LGTM! Comprehensive workflow progression test.This test thoroughly exercises the complete spec-driven workflow, verifying graph state at each stage including completed artifacts, next available artifacts, blocked dependencies, and final completion status. The step-by-step progression with filesystem operations provides excellent integration coverage.
94-113: LGTM! Important edge case coverage.This test validates a critical aspect of the filesystem-based completion model: files created out of dependency order are still marked complete, but
getNextArtifacts()correctly respects the dependency graph. The explanatory comments clarify the expected behavior well.
115-131: LGTM! Good glob pattern coverage.This test validates that the artifact completion detection correctly handles multiple files matching a glob pattern, which is a realistic scenario for artifacts like specs that may contain multiple files.
134-172: LGTM! Excellent parallel workflow coverage.This test mirrors the spec-driven workflow test but exercises the TDD schema, providing validation that the system works with different workflow definitions. The explicit build order verification and realistic file types (
.test.ts,.ts,.md) add to the test's thoroughness.
174-186: LGTM! Important determinism validation.This simple test validates a critical property of the build order calculation: determinism. Ensuring consistent ordering across multiple calls is essential for reproducible builds.
188-221: LGTM! Thorough edge case coverage.These tests provide excellent coverage of edge cases:
- Empty directories (valid state with no files)
- Non-existent directories (robustness against missing paths)
- Non-matching files (strict glob pattern validation)
All scenarios correctly verify that the system doesn't produce false positives and handles unusual conditions gracefully.
- Add teardownTimeout (3s) to vitest config for forced cleanup - Add global teardown function to vitest.setup.ts - Call child.unref() to prevent child processes from blocking event loop - Explicitly destroy stdio streams on process close/error
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
test/helpers/run-cli.tsvitest.config.tsvitest.setup.ts
✅ Files skipped from review due to trivial changes (1)
- vitest.config.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Applies to openspec/changes/**/*.md : Scaffold proposal using `proposal.md`, `tasks.md`, optional `design.md`, and delta specs under `openspec/changes/<id>/`
🔇 Additional comments (1)
test/helpers/run-cli.ts (1)
93-94: LGTM! Test cleanup changes properly prevent hanging processes.The combination of
child.unref()and explicit stream destruction is the correct approach to fix hanging Vitest threads:
child.unref()allows the test process to exit without being blocked by the child process reference, while the Promise still keeps the test alive until completion.- Destroying streams in both error and close handlers ensures no dangling handles remain after the process completes.
- By the time the 'close' event fires, all data has already been collected via 'data' events, so destroying streams is safe and won't cause data loss.
Also applies to: 119-122, 128-131
| // Global teardown to ensure clean exit | ||
| export async function teardown() { | ||
| // Clear any remaining timers | ||
| // This helps prevent hanging handles from keeping the process alive | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for cleanup implementations mentioned in PR objectives
# Search for child.unref() calls
echo "=== Searching for child.unref() ==="
rg -n "\.unref\(\)" --type ts
# Search for stdio stream destruction
echo -e "\n=== Searching for stdio stream destruction ==="
rg -n "stdio.*destroy|destroy.*stdio" --type ts
# Check if teardown is called/defined anywhere
echo -e "\n=== Checking teardown usage ==="
rg -n "teardown" --type ts -C 2
# Check vitest config
echo -e "\n=== Checking vitest.config or setup file content ==="
cat vitest.setup.tsRepository: Fission-AI/OpenSpec
Length of output: 1110
Remove misleading comments or implement promised cleanup in the teardown function.
The teardown function body is empty, yet comments claim it will "clear any remaining timers" to "prevent hanging handles." The actual child process cleanup (child.unref() at line 94 in test/helpers/run-cli.ts) happens elsewhere, not in this function. Either remove the comments to reflect the function's actual no-op behavior, or implement the timer cleanup if it's genuinely needed.
🤖 Prompt for AI Agents
In vitest.setup.ts around lines 8–12, the teardown function contains misleading
comments about clearing timers but is a no-op; either remove those comments so
the function reflects that it intentionally does nothing, or implement the
promised cleanup by tracking and clearing any active timers
(clearTimeout/clearInterval) and other handles before exit while avoiding
duplicate child-process handling already done in test/helpers/run-cli.ts
(child.unref()). Make the change so the comments accurately describe the
implementation.
Summary
artifact-graphcapability spec with 6 requirements and 15 scenariosWhat This Proposal Covers
Key Design Decisions
src/core/artifact-graph/Files
proposal.md- Why and what changesdesign.md- Technical decisions and data structurestasks.md- 21 implementation tasks across 7 categoriesspecs/artifact-graph/spec.md- New capability specificationTest plan
openspec validate add-artifact-graph-core --strict🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.