feat: inject config.yaml context and rules into apply/verify phases#887
Open
k1r3zz wants to merge 4 commits intoFission-AI:mainfrom
Open
feat: inject config.yaml context and rules into apply/verify phases#887k1r3zz wants to merge 4 commits intoFission-AI:mainfrom
k1r3zz wants to merge 4 commits intoFission-AI:mainfrom
Conversation
Previously, config.yaml rules only worked for artifact generation phases
(proposal, specs, design, tasks). The apply and verify phases had no access
to project-level rules, making it impossible to customize execution strategies
(e.g., subagent dispatching, parallel verification scans).
Changes:
- Apply: generateApplyInstructions() now reads config.yaml context + rules.apply
- Verify: new generateVerifyInstructions() reads config.yaml context + rules.verify
- CLI: `openspec instructions verify` is now a valid command
- Validation: apply/verify are accepted as valid rule keys in config.yaml
- Templates: generated command/skill prompts instruct AI to follow rules as
mandatory execution constraints
This enables teams to define custom execution strategies in config.yaml:
rules:
apply:
- "Use subagent for each task"
verify:
- "Run 4 parallel scan subagents"
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI
participant WF as "workflow/index"
participant Instr as "workflow/instructions"
participant Config as "project-config"
participant Schema as "artifact/schema"
participant Out as "stdout/json"
CLI->>WF: parse args (instructions verify ...)
WF->>Instr: invoke verifyInstructionsCommand(options)
Instr->>Config: readPhaseConfig(projectRoot, 'verify')
Instr->>Schema: load change & schema, build contextFiles, parse progress/tasks
Instr->>Out: output JSON OR printVerifyInstructionsText
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/commands/workflow/instructions.ts (1)
397-410: Consider a small helper for phase-config injection.The apply and verify paths now duplicate the same
readProjectConfig()/context/rules[phase]extraction. Centralizing that logic will make future config changes land in one place instead of two.Also applies to: 566-579
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/workflow/instructions.ts` around lines 397 - 410, Extract the duplicated project-config reading logic into a small helper function (e.g., getPhaseConfig or readPhaseConfig) that calls readProjectConfig(projectRoot) and returns an object with trimmed context and phase-specific rules (e.g., { context?: string, rules?: string[] }) based on a phase parameter like 'apply' or 'verify'; replace the duplicated blocks that populate configContext and configRules with calls to this helper in both the apply and verify paths so all config extraction (context trimming and rules['phase'] handling) is centralized.
🤖 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/cli/index.ts`:
- Around line 448-456: The CLI currently intercepts artifactId values 'apply'
and 'verify' in the action handler (the branch that calls
applyInstructionsCommand, verifyInstructionsCommand, or instructionsCommand),
which makes project-defined artifacts with those names unreachable; fix by
adding a schema-level reservation for those names in ArtifactSchema (in
src/core/artifact-graph/types.ts) so the validator rejects artifact IDs 'apply'
and 'verify' (or alternatively refactor the CLI to expose apply/verify as
explicit subcommands/flags instead of positional matches), and update any
relevant tests to assert the reserved-name validation and that
instructionsCommand only runs for non-reserved artifact IDs.
In `@src/commands/workflow/instructions.ts`:
- Around line 542-547: The loop that builds contextFiles stores
path.join(changeDir, artifact.generates) verbatim even though artifact.generates
may be a glob; update the logic in the block that iterates schema.artifacts (the
contextFiles map, the artifactOutputExists check, and the artifact.generates
usage) to expand the glob into concrete file paths before assigning: use the
same glob resolution used by artifactOutputExists to collect all matching files
under changeDir and either (a) set contextFiles[artifact.id] to an array of
resolved absolute/relative file paths, or (b) if you must keep a string shape,
join the resolved paths into a delimiter-separated string—ensure consumers are
updated accordingly so multi-file artifacts like "specs/*.md" return the actual
matched files rather than the wildcard.
---
Nitpick comments:
In `@src/commands/workflow/instructions.ts`:
- Around line 397-410: Extract the duplicated project-config reading logic into
a small helper function (e.g., getPhaseConfig or readPhaseConfig) that calls
readProjectConfig(projectRoot) and returns an object with trimmed context and
phase-specific rules (e.g., { context?: string, rules?: string[] }) based on a
phase parameter like 'apply' or 'verify'; replace the duplicated blocks that
populate configContext and configRules with calls to this helper in both the
apply and verify paths so all config extraction (context trimming and
rules['phase'] handling) is centralized.
🪄 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: 813da049-3643-4bb9-a7dc-9141e64380a3
📒 Files selected for processing (7)
src/cli/index.tssrc/commands/workflow/index.tssrc/commands/workflow/instructions.tssrc/commands/workflow/shared.tssrc/core/project-config.tssrc/core/templates/workflows/apply-change.tssrc/core/templates/workflows/verify-change.ts
Comment on lines
+542
to
+547
| // Build context files from all existing artifacts in schema | ||
| const contextFiles: Record<string, string> = {}; | ||
| for (const artifact of schema.artifacts) { | ||
| if (artifactOutputExists(changeDir, artifact.generates)) { | ||
| contextFiles[artifact.id] = path.join(changeDir, artifact.generates); | ||
| } |
Contributor
There was a problem hiding this comment.
Resolve globbed artifact outputs before returning contextFiles.
artifactOutputExists() already treats generates as a glob, but this branch stores path.join(changeDir, artifact.generates) verbatim. For multi-file artifacts like specs/*.md, verify output will hand consumers a wildcard string instead of the concrete files to read, so required context can be skipped. Please expand matches here, or change the field shape to hold arrays of resolved paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/workflow/instructions.ts` around lines 542 - 547, The loop that
builds contextFiles stores path.join(changeDir, artifact.generates) verbatim
even though artifact.generates may be a glob; update the logic in the block that
iterates schema.artifacts (the contextFiles map, the artifactOutputExists check,
and the artifact.generates usage) to expand the glob into concrete file paths
before assigning: use the same glob resolution used by artifactOutputExists to
collect all matching files under changeDir and either (a) set
contextFiles[artifact.id] to an array of resolved absolute/relative file paths,
or (b) if you must keep a string shape, join the resolved paths into a
delimiter-separated string—ensure consumers are updated accordingly so
multi-file artifacts like "specs/*.md" return the actual matched files rather
than the wildcard.
- Extract readPhaseConfig() helper to centralize config reading logic for apply/verify phases (eliminates duplication) - Add RESERVED_PHASE_IDS constant shared between schema validation and config validation - Add validateNoReservedIds() in schema parser to reject artifact IDs 'apply'/'verify' (prevents CLI routing conflicts) - Update parity test hashes to match template changes Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/schema.ts`:
- Around line 55-63: The reserved-ID check in validateNoReservedIds currently
does a case-sensitive lookup against RESERVED_PHASE_IDS using artifact.id;
change the check to normalize the artifact id to lowercase (e.g., use
artifact.id.toLowerCase() when calling RESERVED_PHASE_IDS.has) so IDs like
"Apply" or "VERIFY" are caught, and update the thrown SchemaValidationError
message to reference the original artifact.id while still checking the lowercase
form; key symbols: validateNoReservedIds, RESERVED_PHASE_IDS, Artifact.id,
SchemaValidationError.
🪄 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: 293f4a4f-0325-4c14-98ce-980bac156b90
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
src/commands/workflow/instructions.tssrc/commands/workflow/shared.tssrc/core/artifact-graph/schema.tssrc/core/project-config.tstest/core/templates/skill-templates-parity.test.ts
✅ Files skipped from review due to trivial changes (1)
- test/core/templates/skill-templates-parity.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core/project-config.ts
- src/commands/workflow/shared.ts
Comment on lines
+55
to
+63
| function validateNoReservedIds(artifacts: Artifact[]): void { | ||
| for (const artifact of artifacts) { | ||
| if (RESERVED_PHASE_IDS.has(artifact.id)) { | ||
| throw new SchemaValidationError( | ||
| `Artifact ID '${artifact.id}' is reserved for phase commands. ` + | ||
| `Reserved IDs: ${Array.from(RESERVED_PHASE_IDS).join(', ')}` | ||
| ); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the definition of RESERVED_PHASE_IDS
echo "=== RESERVED_PHASE_IDS definition ==="
rg -n -A5 'RESERVED_PHASE_IDS' src/core/project-config.ts
# Check how CLI routes the instructions command to see if it's case-sensitive
echo -e "\n=== CLI routing for instructions command ==="
rg -n -B2 -A5 'instructions' src/cli/index.ts | head -50Repository: Fission-AI/OpenSpec
Length of output: 1595
🏁 Script executed:
#!/bin/bash
# Check if artifact IDs are normalized to lowercase anywhere
echo "=== Artifact ID normalization ==="
rg -n -i 'toLowerCase|normalize' src/core/ | grep -i artifact | head -20
# Check the full schema validation file to understand artifact ID handling
echo -e "\n=== Artifact ID schema validation ==="
rg -n 'artifact.*id|id.*artifact' src/core/artifact-graph/schema.ts -i | head -20
# Check if there's any case-insensitive comparison or normalization
echo -e "\n=== Case-insensitive comparisons ==="
rg -n 'toLowerCase\|toUpperCase' src/ | head -20
# Look at the verifyInstructionsCommand and instructionsCommand handlers
echo -e "\n=== Verify instructions command handler ==="
rg -n -A10 'verifyInstructionsCommand' src/cli/ | head -40Repository: Fission-AI/OpenSpec
Length of output: 3150
🏁 Script executed:
#!/bin/bash
# Check the Artifact interface definition to see if case is preserved
echo "=== Artifact interface definition ==="
rg -n -B2 -A5 'interface Artifact\|type Artifact' src/ | head -40
# Check how instructionsCommand handler processes the artifactId
echo -e "\n=== Instructions command handler implementation ==="
rg -n -A15 'export.*instructionsCommand|async.*instructionsCommand' src/cli/ | head -50
# Check how the artifact ID is retrieved/validated in the commands
echo -e "\n=== Artifact retrieval in commands ==="
rg -n 'getArtifact|findArtifact' src/cli/ -A3 | head -40
# Check if there's any example schema with mixed-case artifact IDs
echo -e "\n=== Sample artifact definitions ==="
find . -name "*.yaml" -o -name "*.yml" | xargs grep -l "artifacts:" 2>/dev/null | head -5 | xargs head -100Repository: Fission-AI/OpenSpec
Length of output: 5139
🏁 Script executed:
#!/bin/bash
# Search more broadly for Artifact type
echo "=== Artifact type/interface definition ==="
rg -n 'Artifact' src/core/artifact-graph/schema.ts | head -20
# Check the imports and types in schema.ts
echo -e "\n=== Schema.ts imports and types ==="
head -30 src/core/artifact-graph/schema.ts
# Search for the actual type definition in artifact-graph directory
echo -e "\n=== Types in artifact-graph ==="
fd '\.ts' src/core/artifact-graph/ | xargs grep -l 'interface Artifact\|type Artifact'
# Find the commands implementation file
echo -e "\n=== Find commands file ==="
find src -name '*command*' -type f
# Check the instructions command more deeply
echo -e "\n=== Instructions command in commands file ==="
rg -n 'instructionsCommand' src/ -lRepository: Fission-AI/OpenSpec
Length of output: 1928
🏁 Script executed:
#!/bin/bash
# Read the Artifact type definition
echo "=== Artifact type definition in types.ts ==="
cat src/core/artifact-graph/types.ts
# Read the instructions command handler
echo -e "\n=== Instructions command handler ==="
cat src/commands/workflow/instructions.ts
# Check if there's any validation of artifact IDs in the resolver
echo -e "\n=== Artifact resolution/retrieval ==="
rg -n 'artifact\.id|artifactId' src/commands/workflow/ -A2 -B2 | head -80Repository: Fission-AI/OpenSpec
Length of output: 28155
🏁 Script executed:
#!/bin/bash
# Find and examine the getArtifact implementation
echo "=== getArtifact implementation ==="
rg -n 'getArtifact' src/core/artifact-graph/ -A5 | head -50
# Check if artifact IDs are used in case-sensitive lookups
echo -e "\n=== Artifact ID lookups in graph ==="
rg -n 'class.*Graph|getArtifact|find.*artifact' src/core/artifact-graph/ -B2 -A8 | head -100Repository: Fission-AI/OpenSpec
Length of output: 4805
Normalize artifact IDs to lowercase when validating reserved IDs.
The check uses exact string matching (RESERVED_PHASE_IDS.has(artifact.id)), but the schema allows artifact IDs with any casing. An artifact ID like Apply or VERIFY would bypass this validation since only lowercase apply and verify are checked. Normalize to lowercase during validation:
if (RESERVED_PHASE_IDS.has(artifact.id.toLowerCase())) {
🤖 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 55 - 63, The reserved-ID
check in validateNoReservedIds currently does a case-sensitive lookup against
RESERVED_PHASE_IDS using artifact.id; change the check to normalize the artifact
id to lowercase (e.g., use artifact.id.toLowerCase() when calling
RESERVED_PHASE_IDS.has) so IDs like "Apply" or "VERIFY" are caught, and update
the thrown SchemaValidationError message to reference the original artifact.id
while still checking the lowercase form; key symbols: validateNoReservedIds,
RESERVED_PHASE_IDS, Artifact.id, SchemaValidationError.
validateNoReservedIds now uses toLowerCase() to catch 'Apply', 'VERIFY' and other case variants, not just exact lowercase matches. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Author
|
Hi maintainers 👋 Just wanted to flag this PR for review when you get a chance. Summary: This adds support for custom All CodeRabbit feedback has been addressed across 3 commits, and all 1341 tests are passing ✅ Also fixes a bug where the verify template was incorrectly calling Happy to make any further changes if needed. Thanks! |
YAML parsers interpret unquoted strings containing colons as key-value
mappings. For example, a rule like:
- subagent_type: flutter-agent
becomes {subagent_type: "flutter-agent"} instead of a plain string.
This caused Zod's z.array(z.string()) validation to reject the entire
artifact's rules silently, making apply/verify phases ignore user-
defined rules from config.yaml.
Fix: before Zod validation, normalize array elements by converting
any objects back to "key: value" strings. Users no longer need to
quote rules containing colons.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Previously,
config.yamlrules only worked for artifact generation phases (proposal, specs, design, tasks). The apply and verify phases had no access to project-level rules, making it impossible to customize execution strategies (e.g., subagent dispatching, parallel verification scans).Changes
Core: Apply phase now reads config
generateApplyInstructions()readsconfig.yamlcontext+rules.applyprintApplyInstructionsText()outputs Project Context and Rules sectionsApplyInstructionsinterface gains optionalcontextandrulesfieldsCore: New verify instructions system
generateVerifyInstructions()readsconfig.yamlcontext+rules.verifyverifyInstructionsCommand()+printVerifyInstructionsText()VerifyInstructionsinterface inshared.tsopenspec instructions apply— now correctly callopenspec instructions verifyCLI
openspec instructions verifyis now a valid commandConfig validation
applyandverifyare accepted as valid rule keys invalidateConfigRules()(previously triggered unknown-artifact warnings)Templates
rulesare present, AI must follow them as mandatory execution constraints instead of default behaviorUsage
Teams can now define custom execution strategies in
config.yaml:Files Changed (7)
src/commands/workflow/shared.ts— typessrc/commands/workflow/instructions.ts— core logicsrc/commands/workflow/index.ts— exportssrc/cli/index.ts— CLI entrysrc/core/project-config.ts— validationsrc/core/templates/workflows/apply-change.ts— apply templatessrc/core/templates/workflows/verify-change.ts— verify templatesSummary by CodeRabbit
New Features
CLI
Validation
Workflows
Tests