Propose extending config injection to apply and archive.#1062
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>
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR extends project-level ChangesConfig Injection Extension Proposal
Sequence Diagram(s)sequenceDiagram
participant UserCLI as CLI
participant InstrMod as InstructionsModule
participant ProjectCfg as ProjectConfig
participant Validator as Validator
UserCLI->>InstrMod: request apply/archive instructions (--json)
InstrMod->>ProjectCfg: read config (context, rules)
InstrMod->>Validator: filter workflow keys (apply, archive)
InstrMod->>InstrMod: build template + attach optional context/rules
InstrMod->>UserCLI: return { template, context?, rules? }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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}
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>
|
Updated the proposal to include the config structure, validation expectations, and docs/migration impact |
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.
|
@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
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
|
Addressed — removed the type assertion for workflow rule targets, and added a comment clarifying the optional archive change validation |
|
@TabishB @alfred-openspec Follow-up changes are pushed and the latest feedback is addressed. Would appreciate a review when you have a moment |
2 participants
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