Propose extending config injection to apply and archive.#1062
Open
showms wants to merge 9 commits into
Open
Conversation
Keep the existing context and rules model, but make it available on the apply and archive instruction surfaces for proposal-first upstream discussion. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughExtends project-level ChangesConfig Injection Extension
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI
participant InstrMod as InstructionsModule
participant ProjectCfg as ProjectConfig
participant Validator as ConfigValidator
CLI->>InstrMod: request apply/archive instructions (--json)
InstrMod->>ProjectCfg: read config (context, rules)
InstrMod->>Validator: emitConfigRuleWarnings(rules, validArtifactIds, schema)
InstrMod->>InstrMod: build template + attach optional context/rules
InstrMod->>CLI: return { template, context?, rules? }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
🧹 Nitpick comments (3)
openspec/changes/extend-config-injection-to-apply-archive/proposal.md (3)
9-10: ⚡ Quick winDocument the config structure for workflow-specific rules.
The proposal mentions extending
rulesto provide workflow-specific guidance, but doesn't specify how users will structure their config. The PR description mentionsrules.applyandrules.archiveas reserved targets, but this detail should be documented in the proposal itself for implementation clarity.📋 Suggested addition
Consider adding a subsection under "What Changes" that shows the config structure:
### Config Structure Users will specify workflow-specific rules using reserved targets: - `rules.apply`: guidance applied during `/opsx:apply` instruction generation - `rules.archive`: guidance applied during `/opsx:archive` instruction generation - Artifact keys (e.g., `rules.api`, `rules.workflow`) remain unchanged for backward compatibility🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openspec/changes/extend-config-injection-to-apply-archive/proposal.md` around lines 9 - 10, Add a short "Config Structure" subsection under "What Changes" that documents how to specify workflow-specific rules and context: explain that `rules.apply` and `rules.archive` are reserved targets for guidance applied during `/opsx:apply` and `/opsx:archive` instruction generation, that `context` injection will make the existing project context available to `apply` and `archive` workflows as well as artifact instructions, and note that existing artifact keys like `rules.api` and `rules.workflow` remain supported for backward compatibility.
7-14: 💤 Low valueConsider documenting validation and error handling expectations.
The proposal doesn't discuss how the system should handle invalid or conflicting rules (e.g., malformed
rules.apply, conflicts between artifact and workflow rules). While not essential for initial proposal review, documenting these expectations would help guide implementation and testing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openspec/changes/extend-config-injection-to-apply-archive/proposal.md` around lines 7 - 14, Add a short section to the proposal describing validation and error-handling expectations for the extended injection: specify schema/format checks for rules (e.g., rules.apply), how to detect and report malformed rules, precedence rules when workflow-level rules conflict with artifact-level rules, and the expected runtime behavior (reject/ignore/merge) and error messages for apply and archive instruction processing; reference the injected symbols `context`, the `rules` object and `rules.apply`/`rules.archive` and state that implementations must surface validation errors to callers and include unit/integration test coverage for these cases.
26-31: ⚡ Quick winConsider adding user-facing documentation to the impact list.
The impact section identifies implementation areas (config parsing, instruction generation, templates, tests) but doesn't mention user-facing documentation, examples, or migration guidance. Adding these would help users understand how to adopt the new workflow-specific rules.
📚 Suggested addition
Consider adding to the impact list:
- User documentation will need examples showing how to configure `rules.apply` and `rules.archive` alongside existing artifact rules. - Consider providing migration examples for teams currently restating guidance across artifact instructions and manual apply/archive steps.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openspec/changes/extend-config-injection-to-apply-archive/proposal.md` around lines 26 - 31, Add user-facing documentation and migration guidance to the Impact list: update the Impact section to explicitly include user documentation, examples, and migration notes showing how to configure rules.apply and rules.archive alongside artifact rules, and where to find/update docs (referencing changes in src/core/project-config.ts, src/commands/workflow/instructions.ts, archive templates, and src/core/config-prompts.ts); include example snippets and a short migration guide for teams converting artifact-based guidance to workflow-specific rules so docs, prompts, and tests reflect the new instruction surfaces.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@openspec/changes/extend-config-injection-to-apply-archive/proposal.md`:
- Around line 9-10: Add a short "Config Structure" subsection under "What
Changes" that documents how to specify workflow-specific rules and context:
explain that `rules.apply` and `rules.archive` are reserved targets for guidance
applied during `/opsx:apply` and `/opsx:archive` instruction generation, that
`context` injection will make the existing project context available to `apply`
and `archive` workflows as well as artifact instructions, and note that existing
artifact keys like `rules.api` and `rules.workflow` remain supported for
backward compatibility.
- Around line 7-14: Add a short section to the proposal describing validation
and error-handling expectations for the extended injection: specify
schema/format checks for rules (e.g., rules.apply), how to detect and report
malformed rules, precedence rules when workflow-level rules conflict with
artifact-level rules, and the expected runtime behavior (reject/ignore/merge)
and error messages for apply and archive instruction processing; reference the
injected symbols `context`, the `rules` object and `rules.apply`/`rules.archive`
and state that implementations must surface validation errors to callers and
include unit/integration test coverage for these cases.
- Around line 26-31: Add user-facing documentation and migration guidance to the
Impact list: update the Impact section to explicitly include user documentation,
examples, and migration notes showing how to configure rules.apply and
rules.archive alongside artifact rules, and where to find/update docs
(referencing changes in src/core/project-config.ts,
src/commands/workflow/instructions.ts, archive templates, and
src/core/config-prompts.ts); include example snippets and a short migration
guide for teams converting artifact-based guidance to workflow-specific rules so
docs, prompts, and tests reflect the new instruction surfaces.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9486a521-0d63-47ca-bf4f-329bc9133923
📒 Files selected for processing (2)
openspec/changes/extend-config-injection-to-apply-archive/.openspec.yamlopenspec/changes/extend-config-injection-to-apply-archive/proposal.md
Document reserved apply and archive rule targets, validation expectations, and user-facing docs impact for the proposal discussion. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Author
|
Updated the proposal to include the config structure, validation expectations, and docs/migration impact |
Collaborator
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
This is the right direction: workflow-phase guidance belongs on apply/archive, and proposal-only scope is a good way to settle the contract before implementation. I’d keep rules.apply and rules.archive as reserved workflow targets, but the implementation should avoid turning them into artifact-rule warnings and should keep the built-in apply/archive safety prompts higher priority than config guidance.
Author
|
@alfred-openspec noted, proposal already captures both constraints |
…kflows Add WORKFLOW_RULE_TARGETS constant for 'apply' and 'archive' Add openspec instructions archive command Extend generateApplyInstructions with context and rules.apply Update apply/archive/bulk-archive templates to consume injected config Add workflow rule examples to generated config.yaml Document rules.apply and rules.archive in opsx.md Refs: openspec/changes/extend-config-injection-to-apply-archive
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/core/artifact-graph/instruction-loader.ts (1)
246-251: ⚡ Quick winConsider eliminating the type assertion for better type safety.
The filtering logic is correct, but the type assertion
(WORKFLOW_RULE_TARGETS as Set<string>)on line 248 bypasses TypeScript's type checking. SinceWORKFLOW_RULE_TARGETSis defined asSet<WorkflowId>, the.has()method expects aWorkflowId, but receives an arbitrarystringfromObject.entries().A cleaner approach would be to define
WORKFLOW_RULE_TARGETSasSet<string>inproject-config.ts:export const WORKFLOW_RULE_TARGETS = new Set<string>(['apply', 'archive'] as const satisfies readonly WorkflowId[]);This maintains compile-time checking that the values are valid
WorkflowIdliterals while allowing runtime string comparison without type assertions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/artifact-graph/instruction-loader.ts` around lines 246 - 251, The code uses a type assertion (WORKFLOW_RULE_TARGETS as Set<string>) to allow .has(key) when filtering projectConfig.rules, which weakens type safety; change the declaration of WORKFLOW_RULE_TARGETS in project-config.ts to be Set<string> (e.g., construct it from readonly WorkflowId[] but typed Set<string>) so you can call WORKFLOW_RULE_TARGETS.has(key) without assertions, then remove the cast in instruction-loader.ts where artifactOnlyRules is built and keep the call into validateConfigRules unchanged.src/commands/workflow/instructions.ts (1)
497-499: 💤 Low valueClarify the purpose of optional change validation.
The command validates that the change exists when provided, but
generateArchiveInstructions(line 501) doesn't accept or use the change name. This validation appears to be either:
- A courtesy check to ensure the user is in the right context, or
- Future-proofing for change-specific archive logic not yet implemented.
If it's (1), consider adding a comment explaining the intent. If it's (2), consider whether the validation should be deferred until the feature is needed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/workflow/instructions.ts` around lines 497 - 499, The optional call to validateChangeExists(options.change, projectRoot) is ambiguous because generateArchiveInstructions does not use the change name; either add a one-line comment above this if-block clarifying that this is a courtesy/contextual validation (i.e., "ensure provided change name exists so the user is in the right context") or, if the intent is future change-specific logic, remove/defer the validation until that feature is implemented; refer to validateChangeExists, options.change and generateArchiveInstructions when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/commands/workflow/instructions.ts`:
- Around line 497-499: The optional call to validateChangeExists(options.change,
projectRoot) is ambiguous because generateArchiveInstructions does not use the
change name; either add a one-line comment above this if-block clarifying that
this is a courtesy/contextual validation (i.e., "ensure provided change name
exists so the user is in the right context") or, if the intent is future
change-specific logic, remove/defer the validation until that feature is
implemented; refer to validateChangeExists, options.change and
generateArchiveInstructions when making the change.
In `@src/core/artifact-graph/instruction-loader.ts`:
- Around line 246-251: The code uses a type assertion (WORKFLOW_RULE_TARGETS as
Set<string>) to allow .has(key) when filtering projectConfig.rules, which
weakens type safety; change the declaration of WORKFLOW_RULE_TARGETS in
project-config.ts to be Set<string> (e.g., construct it from readonly
WorkflowId[] but typed Set<string>) so you can call
WORKFLOW_RULE_TARGETS.has(key) without assertions, then remove the cast in
instruction-loader.ts where artifactOnlyRules is built and keep the call into
validateConfigRules unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 785f619e-962c-4b51-ac1c-a4d23b545921
📒 Files selected for processing (25)
docs/opsx.mdopenspec/changes/extend-config-injection-to-apply-archive/design.mdopenspec/changes/extend-config-injection-to-apply-archive/proposal.mdopenspec/changes/extend-config-injection-to-apply-archive/specs/cli-archive-instructions/spec.mdopenspec/changes/extend-config-injection-to-apply-archive/specs/cli-artifact-workflow/spec.mdopenspec/changes/extend-config-injection-to-apply-archive/specs/context-injection/spec.mdopenspec/changes/extend-config-injection-to-apply-archive/specs/opsx-archive-skill/spec.mdopenspec/changes/extend-config-injection-to-apply-archive/specs/opsx-bulk-archive-skill/spec.mdopenspec/changes/extend-config-injection-to-apply-archive/specs/rules-injection/spec.mdopenspec/changes/extend-config-injection-to-apply-archive/tasks.mdopenspec/config.yamlsrc/cli/index.tssrc/commands/workflow/index.tssrc/commands/workflow/instructions.tssrc/commands/workflow/shared.tssrc/core/artifact-graph/instruction-loader.tssrc/core/config-prompts.tssrc/core/project-config.tssrc/core/templates/workflows/apply-change.tssrc/core/templates/workflows/archive-change.tssrc/core/templates/workflows/bulk-archive-change.tstest/commands/apply-archive-instructions.test.tstest/commands/artifact-workflow.test.tstest/core/artifact-graph/instruction-loader.test.tstest/core/templates/skill-templates-parity.test.ts
✅ Files skipped from review due to trivial changes (7)
- src/commands/workflow/index.ts
- openspec/changes/extend-config-injection-to-apply-archive/specs/cli-archive-instructions/spec.md
- openspec/changes/extend-config-injection-to-apply-archive/specs/opsx-archive-skill/spec.md
- src/core/config-prompts.ts
- openspec/changes/extend-config-injection-to-apply-archive/specs/opsx-bulk-archive-skill/spec.md
- docs/opsx.md
- openspec/changes/extend-config-injection-to-apply-archive/tasks.md
Author
|
Addressed — removed the type assertion for workflow rule targets, and added a comment clarifying the optional archive change validation |
Author
|
@TabishB @alfred-openspec Follow-up changes are pushed and the latest feedback is addressed. Would appreciate a review when you have a moment |
alfred-openspec
requested changes
May 17, 2026
Collaborator
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Thanks for taking this from proposal to implementation, the overall shape looks coherent and the test suite passes. I found one validation gap to fix before merge: apply/archive read config directly and do not warn on unknown rules.* keys, so config typos can be missed on the new workflow surfaces.
…xtend-config-injection-to-apply-archive # Conflicts: # src/core/artifact-graph/instruction-loader.ts # test/core/templates/skill-templates-parity.test.ts
Author
|
@alfred-openspec Thanks for the review — I addressed the validation gap in this update. Changes made:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/core/project-config.ts (1)
50-53: ⚡ Quick winMake workflow targets immutable at the export boundary.
WORKFLOW_RULE_TARGETSis exported as a mutableSet, so any consumer can mutate validation behavior at runtime. Prefer exporting a read-only shape.♻️ Suggested change
-export const WORKFLOW_RULE_TARGETS = new Set<string>([ - 'apply', - 'archive', -] as const satisfies readonly WorkflowId[]); +const WORKFLOW_RULE_TARGETS_LIST = [ + 'apply', + 'archive', +] as const satisfies readonly WorkflowId[]; + +export const WORKFLOW_RULE_TARGETS: ReadonlySet<string> = new Set(WORKFLOW_RULE_TARGETS_LIST);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/project-config.ts` around lines 50 - 53, WORKFLOW_RULE_TARGETS is exported as a mutable Set; make the export read-only by replacing the exported Set with an immutable readonly tuple/array so consumers cannot mutate validation targets at runtime. Update WORKFLOW_RULE_TARGETS (the exported symbol) to a const readonly structure, e.g. export const WORKFLOW_RULE_TARGETS = ['apply', 'archive'] as const satisfies readonly WorkflowId[] (or a ReadonlyArray<WorkflowId>), and update any callers that relied on Set methods (use includes or construct a local Set where needed).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@openspec/changes/extend-config-injection-to-apply-archive/specs/rules-injection/spec.md`:
- Line 78: Update the warning example string to match the implementation/tests
by replacing single quotes with double quotes; locate the example line
containing "Unknown key in rules: 'unknownkey'. Valid keys for schema
'spec-driven': apply, archive (workflow), design, proposal, specs, tasks
(artifact)" and change it so the unknown key and schema use double quotes
instead of single quotes to align formats used in tests.
---
Nitpick comments:
In `@src/core/project-config.ts`:
- Around line 50-53: WORKFLOW_RULE_TARGETS is exported as a mutable Set; make
the export read-only by replacing the exported Set with an immutable readonly
tuple/array so consumers cannot mutate validation targets at runtime. Update
WORKFLOW_RULE_TARGETS (the exported symbol) to a const readonly structure, e.g.
export const WORKFLOW_RULE_TARGETS = ['apply', 'archive'] as const satisfies
readonly WorkflowId[] (or a ReadonlyArray<WorkflowId>), and update any callers
that relied on Set methods (use includes or construct a local Set where needed).
🪄 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: 14838602-9ce3-4b36-8b19-ce1dd8810d84
📒 Files selected for processing (22)
docs/opsx.mdopenspec/changes/extend-config-injection-to-apply-archive/design.mdopenspec/changes/extend-config-injection-to-apply-archive/specs/cli-archive-instructions/spec.mdopenspec/changes/extend-config-injection-to-apply-archive/specs/context-injection/spec.mdopenspec/changes/extend-config-injection-to-apply-archive/specs/opsx-archive-skill/spec.mdopenspec/changes/extend-config-injection-to-apply-archive/specs/opsx-bulk-archive-skill/spec.mdopenspec/changes/extend-config-injection-to-apply-archive/specs/rules-injection/spec.mdopenspec/changes/extend-config-injection-to-apply-archive/tasks.mdopenspec/config.yamlsrc/cli/index.tssrc/commands/workflow/instructions.tssrc/commands/workflow/shared.tssrc/core/artifact-graph/instruction-loader.tssrc/core/project-config.tssrc/core/templates/workflows/apply-change.tssrc/core/templates/workflows/archive-change.tssrc/core/templates/workflows/bulk-archive-change.tstest/commands/apply-archive-instructions.test.tstest/commands/artifact-workflow.test.tstest/core/artifact-graph/instruction-loader.test.tstest/core/project-config.test.tstest/core/templates/skill-templates-parity.test.ts
✅ Files skipped from review due to trivial changes (7)
- openspec/changes/extend-config-injection-to-apply-archive/specs/opsx-bulk-archive-skill/spec.md
- src/commands/workflow/shared.ts
- openspec/changes/extend-config-injection-to-apply-archive/specs/opsx-archive-skill/spec.md
- test/core/templates/skill-templates-parity.test.ts
- openspec/changes/extend-config-injection-to-apply-archive/specs/context-injection/spec.md
- openspec/changes/extend-config-injection-to-apply-archive/tasks.md
- docs/opsx.md
🚧 Files skipped from review as they are similar to previous changes (9)
- openspec/changes/extend-config-injection-to-apply-archive/specs/cli-archive-instructions/spec.md
- src/core/templates/workflows/apply-change.ts
- test/core/artifact-graph/instruction-loader.test.ts
- openspec/config.yaml
- src/cli/index.ts
- test/commands/apply-archive-instructions.test.ts
- test/commands/artifact-workflow.test.ts
- src/core/templates/workflows/bulk-archive-change.ts
- src/core/templates/workflows/archive-change.ts
Author
|
Follow-up update: the validation-gap fix is in, and I also addressed the latest CodeRabbit follow-ups. @alfred-openspec @TabishB would appreciate a re-review when convenient. |
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
2 participants
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.
Problem
openspec/config.yamlsupportscontextandrules, butrulesare only applied when generating artifact instructions. They are not surfaced inapply/archiveflows, so project-level guidance becomes inconsistent across the workflow.Why this matters
Users expect project constraints defined in config to apply throughout the workflow, not only while generating artifacts. This is especially important for:
applyarchiveIn practice, this would make the workflow much more flexible. For example:
apply, projects could specify execution guidance such as whether sub-agents are allowed, how much parallelization is appropriate, or what implementation constraints must still be followedarchive, projects could specify post-archive follow-up steps such as documentation cleanup, knowledge base updates, release note preparation, or other housekeeping actionsProposed change
Allow reserved
rulestargets for workflow phases:rules.applyrules.archiveKeep existing artifact keys unchanged.
Scope
This proposal would:
rules.applyinto apply instructionsrules.archiveinto archive workflow instructionsIt does not redesign the config format.
In this PR
This PR only adds the OpenSpec proposal scaffolding for discussion before implementation:
openspec/changes/extend-config-injection-to-apply-archive/.openspec.yamlopenspec/changes/extend-config-injection-to-apply-archive/proposal.mdTest plan
spec-drivenschemaSummary by CodeRabbit