feat: make apply instructions schema-aware #444
Conversation
The `generateApplyInstructions` function was hardcoded to check for `spec-driven` artifacts. This change makes it read artifact definitions from the schema's new `apply` block, enabling support for different workflows like TDD. Changes: - Add `ApplyPhaseSchema` Zod schema with `requires`, `tracks`, and `instruction` fields to types.ts - Update `SchemaYamlSchema` to include optional `apply` field - Add `apply` block to spec-driven and tdd schemas - Refactor `generateApplyInstructions` to: - Load schema via `resolveSchema()` - Read `apply.requires` for required artifacts - Check artifact existence dynamically (supports glob patterns) - Use `apply.tracks` for progress tracking (or skip if null) - Use `apply.instruction` for custom guidance - Build `contextFiles` from all existing artifacts in schema - Handle fallback when schema has no `apply` block (require all artifacts) - Add 8 new tests for schema-aware apply behavior
|
Warning Rate limit exceeded@TabishB has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 14 minutes and 41 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 Walkthrough📝 WalkthroughPre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
Review CompleteYour review story is ready! Comment !reviewfast on this PR to re-generate the story. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Fix all issues with AI Agents 🤖
In @src/commands/artifact-workflow.ts:
- Around line 426-447: artifactOutputExists currently treats any glob by only
checking the parent directory and splits generates on '/' which breaks on
Windows and yields false positives; update artifactOutputExists to normalize the
generates path and then resolve the full glob against changeDir using a proper
glob match (e.g., fast-glob or glob.sync) and return true only if at least one
file matches, falling back for non-glob generates to fs.existsSync(fullPath); if
you cannot add a glob dependency, compute dirPath using path.join/path.normalize
(use path.sep) and recursively read the directory entries under dirPath to
verify at least one file matches the expected pattern/extension before returning
true (refer to artifactOutputExists, generates, changeDir, dirPath).
- Around line 482-493: The loop building contextFiles has a redundant if/else:
both branches set contextFiles[artifact.id] = path.join(changeDir,
artifact.generates). Remove the conditional and simplify the body of the for
loop to always assign contextFiles[artifact.id] = path.join(changeDir,
artifact.generates) after verifying artifactOutputExists (keep the call to
artifactOutputExists, schema.artifacts, artifact.id, artifact.generates and
path.join as the referenced symbols).
- Around line 495-523: The code treats a configured tracksFile that doesn't
exist as "empty"; capture whether the tracks file actually exists when you read
it (e.g., set a fileExists flag when checking fs.existsSync(tracksPath) /
reading tasks via parseTasksFile) and then change the decision logic that
currently uses (tracksFile && total === 0): if tracksFile is configured but
fileExists is false, set state to 'blocked' with a message that the tracking
file is missing and must be created; if fileExists is true and total === 0, keep
the existing "file exists but contains no tasks" message (using
path.basename(tracksFile) as tracksFilename). Ensure references: tracksFile,
tracksPath, tasks, parseTasksFile, and the state/instruction assignment logic
are updated accordingly.
In @src/core/artifact-graph/types.ts:
- Around line 13-21: The validation on ApplyPhaseSchema's requires field uses {
message: 'At least one required artifact' } but the rest of the file uses the {
error: '...' } key; update the requires constraint to use { error: 'At least one
required artifact' } so it is consistent with other Zod validations (modify the
requires z.array(...).min(...) call inside ApplyPhaseSchema).
In @test/commands/artifact-workflow.test.ts:
- Around line 445-459: The test named "fallback: requires all artifacts when
schema has no apply block" doesn't exercise the fallback path because it uses
the spec-driven schema (which has an apply block); either modify the test to
create/use a minimal schema that lacks an apply block (e.g., add a helper like
createTestSchemaWithoutApply or extend createTestChange to accept a schema name
and pass a no-apply schema) and assert the fallback behavior, or simply rename
the test and its description to reflect that it verifies the spec-driven schema
uses its apply block (e.g., "spec-driven schema uses apply block
configuration"); update the test title and any helper call sites accordingly
(test name string, createTestChange invocation, and assertions around
json.state).
🧹 Nitpick comments (2)
src/core/artifact-graph/types.ts (1)
18-18: Consider simplifying nullable/optional chaining.The
tracksfield uses.nullable().optional(), which creates a tri-state type (string | null | undefined). Review whether both null and undefined states are necessary:
- If the field can be omitted from YAML, use
.optional()with a default value- If the field must be present but can be null, use
.nullable()only- If both states serve different purposes, document the distinction
Based on the schema usage (spec-driven uses a string, tdd uses null), it appears
.nullable()alone might suffice if the field is always present.test/commands/artifact-workflow.test.ts (1)
35-65: Consider extending test helper for multiple schemas.The
createTestChangehelper currently only supports spec-driven artifacts (proposal, design, specs, tasks). The tdd schema test (lines 426-443) creates artifacts manually because tdd uses different artifact names (spec, tests, implementation, docs).Consider adding an optional schema parameter to
createTestChangeto support multiple workflow schemas, reducing duplication in test setup. This would improve maintainability as more schemas are added.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
openspec/changes/make-apply-instructions-schema-aware/tasks.mdschemas/spec-driven/schema.yamlschemas/tdd/schema.yamlsrc/commands/artifact-workflow.tssrc/core/artifact-graph/types.tstest/commands/artifact-workflow.test.ts
🧰 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/make-apply-instructions-schema-aware/tasks.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/make-apply-instructions-schema-aware/tasks.md
🧠 Learnings (4)
📚 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:
schemas/tdd/schema.yamlopenspec/changes/make-apply-instructions-schema-aware/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/**/*.md : Scaffold proposal using `proposal.md`, `tasks.md`, optional `design.md`, and delta specs under `openspec/changes/<id>/`
Applied to files:
schemas/tdd/schema.yamlopenspec/changes/make-apply-instructions-schema-aware/tasks.mdsrc/commands/artifact-workflow.tsschemas/spec-driven/schema.yaml
📚 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:
schemas/tdd/schema.yamlopenspec/changes/make-apply-instructions-schema-aware/tasks.mdschemas/spec-driven/schema.yaml
📚 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/*/proposal.md : Ensure `proposal.md` includes sections: Why (1-2 sentences), What Changes (bullet list with breaking change markers), and Impact (affected specs and code)
Applied to files:
openspec/changes/make-apply-instructions-schema-aware/tasks.md
🧬 Code graph analysis (2)
test/commands/artifact-workflow.test.ts (1)
test/helpers/run-cli.ts (1)
runCLI(75-148)
src/commands/artifact-workflow.ts (1)
src/core/artifact-graph/index.ts (1)
resolveSchema(22-22)
🔇 Additional comments (5)
openspec/changes/make-apply-instructions-schema-aware/tasks.md (1)
1-35: LGTM!The task list is well-structured with numbered sections and checkbox items, following the coding guidelines. All tasks are marked complete, which aligns with the PR objectives indicating the feature is fully implemented and tested.
schemas/spec-driven/schema.yaml (1)
143-148: No action required. The YAML file contains only oneapplyblock (line 143), not duplicates. The AI summary referencing duplicate locations was inaccurate.Likely an incorrect or invalid review comment.
schemas/tdd/schema.yaml (1)
208-213: No duplicate apply blocks exist.The file contains only one top-level
applykey at line 208. The concern about silent YAML key overwrites does not apply here.src/commands/artifact-workflow.ts (2)
42-56: LGTM!The interface changes appropriately support the schema-aware design. The dynamic
Record<string, string>forcontextFilesand the addition ofschemaNameprovide the flexibility needed for different schema configurations.
575-625: LGTM!The print function correctly handles the new schema-aware structure. The dynamic iteration over
contextFilesentries and conditional progress display adapt well to different schema configurations.
- Fix artifactOutputExists to properly handle glob patterns cross-platform
by using path.sep and verifying actual file matches
- Remove redundant if/else in contextFiles loop
- Distinguish between missing tracks file vs empty tracks file
- Use consistent { error: } key in ApplyPhaseSchema validation
- Rename fallback test to accurately describe what it tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/commands/artifact-workflow.test.ts (1)
351-460: Test coverage looks solid, but fallback behavior remains untested.The test suite comprehensively covers the schema-driven apply instruction scenarios including blocked states, task completion, and schema-specific configurations. The test at lines 445-459 has been appropriately renamed to reflect what it actually validates.
However, there's still no test for the fallback path when a schema lacks an
applyblock (the logic at line 510 ofsrc/commands/artifact-workflow.tsthat falls back to requiring all artifacts). Consider adding a test with a minimal schema that has noapplyconfiguration to verify the fallback behavior works correctly.Suggested test for fallback behavior
You could create a minimal test schema or modify the test helper to support schemas without apply blocks:
it('falls back to requiring all artifacts when schema has no apply block', async () => { // Would require creating a test schema without an apply block // or mocking resolveSchema to return a schema without apply configuration // Then verify that all artifacts are required for the apply to be ready });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/commands/artifact-workflow.tssrc/core/artifact-graph/types.tstest/commands/artifact-workflow.test.ts
🧰 Additional context used
🧠 Learnings (1)
📚 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:
src/commands/artifact-workflow.ts
🧬 Code graph analysis (2)
test/commands/artifact-workflow.test.ts (1)
test/helpers/run-cli.ts (1)
runCLI(75-148)
src/commands/artifact-workflow.ts (1)
src/core/artifact-graph/resolver.ts (1)
resolveSchema(78-122)
🔇 Additional comments (5)
src/core/artifact-graph/types.ts (1)
13-21: LGTM! Well-structured apply phase schema.The
ApplyPhaseSchemadefinition is clear and follows the existing patterns in the file. The validation constraints use consistent{ error: '...' }parameters, and the comments effectively document each field's purpose.src/commands/artifact-workflow.ts (4)
428-488: LGTM! Glob handling and cross-platform concerns addressed.The
artifactOutputExistsfunction now properly handles both issues from the previous review:
- Cross-platform paths: Line 434 normalizes path separators using
path.sep- Accurate glob matching: Lines 462-484 recursively check for actual matching files with proper extension validation, not just directory existence
The implementation correctly handles
**patterns, validates file extensions, and only returnstruewhen matching files exist.
523-529: LGTM! Redundant branching removed.The context files construction has been simplified from the previous redundant if/else branches to a clean, straightforward implementation. Good refactoring.
531-564: LGTM! Missing vs. empty tracking file properly distinguished.The code now correctly handles three distinct cases with appropriate messaging:
- Tracking file missing (lines 555-559): "file is missing and must be created"
- Tracking file empty (lines 560-564): "exists but contains no tasks"
- Tracking file with incomplete tasks (line 573): Ready state with appropriate instruction
The
tracksFileExistsflag cleanly separates these concerns. Well done addressing the previous review feedback.
495-588: Excellent schema-aware refactoring!The
generateApplyInstructionsimplementation successfully transitions from hardcoded artifact checks to schema-driven configuration. Key strengths:
- Flexible configuration: Reads
apply.requires,apply.tracks, andapply.instructionfrom schema with sensible fallbacks (lines 504-512)- Dynamic validation: Uses
artifactOutputExiststo check required artifacts based on schema patterns (lines 514-521)- Context awareness: Builds
contextFilesdynamically from all existing schema artifacts (lines 523-529)- Comprehensive state logic: Properly handles blocked/ready/all_done states with clear, actionable messaging (lines 548-575)
- Schema exposure: Returns
schemaNamein the API surface for transparency (line 580)All previous review concerns have been addressed, and the implementation aligns well with the new
ApplyPhaseSchematype definitions.
Add two tests that verify the fallback logic when a schema lacks an apply block: - Blocks when not all artifacts exist (requires ALL artifacts) - Ready state with default instruction when all artifacts exist Uses XDG_DATA_HOME to create temporary user schemas for testing.
Summary
generateApplyInstructionsfunction now reads artifact definitions from the schema'sapplyblock instead of being hardcoded tospec-drivenartifactsapplyblock in schemas: Addedapplyconfiguration to bothspec-drivenandtddschemas withrequires,tracks, andinstructionfieldsspecs/**/*.md)Changes
Schema Format (
src/core/artifact-graph/types.ts)ApplyPhaseSchemaZod schema with:requires: Array of artifact IDs that must exist before applytracks: Path to file with checkboxes for progress (ornullif no tracking)instruction: Custom guidance for the apply phaseSchemaYamlSchemato include optionalapplyfieldApplyPhasetypeSchema Updates
requires: [tasks],tracks: tasks.md, with task-based instructionrequires: [tests],tracks: null, with TDD-specific instructionApply Instructions Refactor (
src/commands/artifact-workflow.ts)resolveSchema()to accessapplyconfigurationapply.requiresto determine required artifacts dynamicallygeneratespatterns (supports globs)apply.tracksfor progress tracking (skips if null)apply.instructionfor custom instruction textcontextFilesfrom all existing artifacts in schemaapplyblockTests
Added 8 new tests covering:
Test plan
openspec instructions apply --change <name>works with spec-driven schemaopenspec instructions apply --change <name> --schema tddworks with TDD schema🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.