feat(instructions): add runtime context and operation guidance - #1062
Conversation
|
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)
✨ 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}
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
|
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 |
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 |
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.
|
@alfred-openspec Thanks for the review — I addressed the validation gap in this update. Changes made:
|
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
|
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. |
|
@alfred-openspec friendly ping when you have a chance. The validation-gap issue from the May 17 review has been fixed, and the latest follow-up is included in the current head commit from May 29. I also noticed the CI run on the latest commit is showing |
showms
changed the title
showms
changed the title
|
Thanks for pushing this forward. After digging into the design more, I think this PR has found a real user problem:
The dynamic part of this PR is useful. A skill should be able to ask OpenSpec for the latest project context when it runs. I think we should pause on the For example: rules:
specs:
- Every requirement must include a scenarioThis describes the spec we want the agent to produce. But this is different: operations:
apply:
guidance:
- Show a short test summary after each taskThis is extra advice about how the agent should work. The difference between guidance and a skill is important: A skill defines the main flow. Guidance helps the agent make choices inside that flow. There is one important limitation: to the model, the skill and guidance are both text. If guidance says “skip validation”, simply calling it guidance does not stop the agent from following it. Anything that must be protected should therefore be a real CLI check, not prompt text. Archive is a useful example of how these pieces fit together. We tried deterministic Markdown requirement merging before, but heading matching was too brittle. An agent is better at understanding the meaning and rewriting the final spec so it still reads well. That does not mean the agent needs to own the whole archive operation. A possible future flow is: This is the key artifact/skill relationship: The spec rules are not turned into archive rules. They are included because archive happens to produce a spec. We can think of artifact rules as “travelling with the artifact”. A possible project config could look like this: rules:
specs:
- Keep existing requirement IDs when the meaning has not changed
- Every requirement must include at least one scenario
operations:
archive:
guidance:
- Show a short summary of what changed after mergingThe user experience might be: My suggested direction is:
So I think this PR is valuable because it exposed a missing design boundary. The concern is not the quality of the implementation. I would rather settle the public model first than merge a config shape that may be hard to change later. |
- add typed apply and archive operation guidance - extend runtime instruction inputs for apply and archive - preserve existing archive execution and spec sync behavior
2378811 to
8bfe064
Compare
- carry artifact rules into archive-driven spec sync - reuse one config snapshot per instruction command - clarify that operation guidance is advisory - classify bulk archive skill as a new capability
- define owning artifact resolution for mixed schemas - apply artifact rules in archive and standalone sync flows - align archive and bulk guidance conflict semantics - clarify existing apply pause-on-blocker behavior
- scope delta discovery and artifact rules to the specs artifact - fail closed on invalid archive and specs instruction responses - clarify no-write and no-move behavior for single and bulk archive
|
Thanks for the detailed explanation and concrete archive example — it helped clarify the boundaries between artifact rules, operation guidance, skills, and CLI-enforced checks. I explored the proposed Today, A phased archive flow would likely require:
To keep the work focused and reviewable, would you prefer splitting it into two stages? 1. Runtime inputs and guidance in this PR
2. Archive execution redesign in a follow-up change
I have updated the current change artifacts around the first-stage contract, including fail-closed instruction lookup behavior and keeping spec sync scoped to the existing If you would rather settle the phased archive design as part of this PR before implementation, I’m also happy to expand the current change. |
|
Let’s split it. Keep #1062 scoped to runtime-fresh context and operations.*.guidance, plus carrying specs artifact rules into the existing apply/archive/sync workflows, with archive execution ownership and phases unchanged; the current proposal reflects that boundary well. Please open the prepare -> agent work -> validate -> confirm -> finalize redesign as a follow-up proposal, since it changes CLI/skill ownership, single/bulk atomicity, compatibility, and mixed-schema semantics and should be reviewed independently. |
- expose project context and operation guidance in apply/archive instructions - apply context and guidance across apply, archive, bulk archive, and spec sync - preserve workflow state, artifact-rule boundaries, and fail-closed behavior - update generated skills, documentation, tests, and parity hashes
|
Thanks — I’ve now implemented the agreed first-stage scope:
The Verification:
@TabishB @alfred-openspec, this should now be ready for review. Thanks! |
showms
changed the title
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Reviewed at d30183e. The final operation-guidance contract resolves my earlier validation concern; focused tests, lint, build, strict change validation, and CI are green.
|
Important Retracted — this verdict was wrong. See my current review for the LGTM. Two of the objections below don't hold, and I'm leaving the text intact rather than deleting it so the thread still reads:
What did hold: the skills.sh version-skew problem, fixed in Not LGTM — refactor and split before this can be consideredI reviewed this against four questions: is the need real, does it work, does it break anyone, and does it expand OpenSpec's core design. It works and it's well tested, but it breaks the archive workflow for skills.sh users, silently hijacks a reserved word, and expands the config architecture in a direction that conflicts with the open issue asking for this capability. Everything below was verified by running the branch (merge base = Verification performed
1. Is this a true need? Partially — but the mechanism diverges from the one that was asked forThe gap is real. I confirmed But there's an open issue asking for exactly this, and it asks for a different shape. #1383 "Proposal: Instructions for archive/sync in schema.yml" requests archive/sync instructions in # schemas/spec-driven/schema.yaml (already on main, lines 203-208)
apply:
requires: [tasks]
tracks: tasks.md
instruction: |
Read context files, work through pending tasks, mark complete as you go.The idiomatic, non-expanding extension is an Also: the PR description is badly stale. It still reads "This PR only adds the OpenSpec proposal scaffolding for discussion before implementation" and "No runtime code changes in this PR, so no tests were run." It is now 37 files and 2,424 additions of runtime code, and it describes a 2. Does the solution work? YesNo functional defects found. Verified live: $ openspec instructions archive --change demo-change --json
{
"changeName": "demo-change",
"context": "TypeScript project using pnpm.\n",
"operationGuidance": ["Summarize the archive outcome"],
"root": { "path": "/…/sandbox", "source": "nearest" }
}Malformed config degrades correctly and does not poison unrelated fields: $ openspec instructions apply --change demo-change --json # archive guidance is a string, plus a bogus op
Guidance for operation 'archive' must be an array of strings, ignoring this operation's guidance
Unknown operation ID 'bogusop' in config. Supported operation IDs: apply, archive
{ …valid apply payload, apply guidance intact… }Warnings go to stderr; stdout is pure JSON. Test coverage is genuinely good — the new cases in One part of this PR is an unambiguous win and should be kept regardless of what happens to the rest: threading a single 3. Breaking changes? Yes — three, one of them a blocker3a. BLOCKER — skill/CLI version skew hard-blocks archiving
Against the current released CLI: $ openspec instructions archive --change demo-change --json # v1.6.0
{ "status": [ { "severity": "error", "code": "change_error",
"message": "Artifact 'archive' not found in schema 'spec-driven'. Valid artifacts:\n proposal\n specs\n design\n tasks" } ]
$ echo $?
1So between merging this and shipping the next npm release, every skills.sh consumer's archive workflow is bricked — not degraded, stopped. Bulk archive is worse; its rule is "A failed archive/specs instruction lookup stops the whole batch atomically." Fix: any new CLI call a skill learns must degrade gracefully — treat a failed lookup as "no inputs configured" and continue the built-in workflow. Hard-stop semantics are only safe for commands that have already shipped. 3b. MAJOR —
|
LGTMUpdated 2026-07-27 after reading the full review history. This supersedes my earlier "Not LGTM" comment, which got two things wrong — see the corrections below. Corrections to my earlier review1.
@showms implemented exactly the shape the maintainer specified. My criticism was unfair and I withdraw it. 2. Carrying
And the non-goal I quoted no longer exists. I had diffed against a stale Verification against current main
@TabishB's seven-point direction, checked individually: points 1–4 implemented; point 5 (safety stays real CLI checks) respected — nothing enforceable moved into prompt text; points 6–7 deferred to follow-ups below. The one real issue found, and fixed
$ npx @fission-ai/openspec@1.6.0 instructions archive --change demo --json
{ "status": [ { "severity": "error", "code": "change_error", … } ] }
$ echo $?
1Fixed in General rule worth carrying forward: any time a generated skill learns a new CLI command, it must degrade when that command is absent — the skill can always be installed ahead of the CLI. Follow-ups filedBoth deferred items now have homes, so nothing is lost by merging:
Remaining non-blockers
Nice work @showms, and sorry for the noisy first pass. |
`openspec instructions archive` is introduced by this PR, so no released CLI has it. The archive and bulk-archive skills required a zero exit status from that lookup and told the agent to stop when it failed. `skills/` is installed standalone via `npx skills add Fission-AI/OpenSpec` and drives whatever CLI the user already has, so between merging this and publishing the next release every skills.sh consumer would have had archiving blocked outright — verified against @fission-ai/openspec@1.6.0, which exits 1 on that command. The lookup only supplies optional prompt inputs, so it now degrades: on a non-zero exit or invalid JSON the workflow continues with no context and no operation guidance. The `openspec instructions specs` lookup is an existing command and stays fail-closed, since a missing rule set there would silently change what gets written to main specs. Parity assertions updated to encode fail-open for archive inputs and fail-closed for specs rules. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Heads up @showms — I pushed one commit directly to your branch ( Happy to revert it if you'd rather take it yourself — just say so. What it changesOnly the failure semantics of the new - Keep the same selected-root flags on this command. Require a zero exit status
- and valid archive-instruction JSON for the selected change. If the lookup exits
- non-zero or returns invalid JSON, report the error and stop before inspecting or
- writing specs or moving the change. Do not treat a failed lookup as an empty
- successful response.
+ Keep the same selected-root flags on this command. This lookup is advisory and
+ optional: it only supplies extra prompt inputs, so it must never block archiving.
+ If it exits non-zero or returns invalid JSON — for example on an older CLI that
+ does not support this command yet — continue the archive workflow with no
+ context and no operation guidance. Do not report an error and do not stop.Why
$ npx @fission-ai/openspec@1.6.0 instructions archive --change demo-change --json
{ "status": [ { "severity": "error", "code": "change_error",
"message": "Artifact 'archive' not found in schema 'spec-driven'…" } ] }
$ echo $?
1Combined with the old "stop before inspecting or writing specs or moving the change", that meant every skills.sh user on What I deliberately did not changeThe
Verification
The general rule worth carrying forward: any time a generated skill learns a new CLI command, it has to degrade when that command is absent, because the skill can always be installed ahead of the CLI. With that in, this is an LGTM from me. |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Re-reviewed at bfe82b1. The archive-input compatibility fix preserves standalone skills on older CLIs while keeping specs-rule loading fail-closed; 137 focused tests, build, lint, strict change validation, and all CI checks pass.
Resolve conflicts between this branch's bulk-archive rework and main's runtime-context/artifact-rules guidance (Fission-AI#1062) by unioning both sides: - archive-change: keep main's specs-instruction fetch gate; keep this branch's /opsx:sync naming in the opsx command variant. - bulk-archive: step 8a syncs only includedDeltas AND carries main's specs-rule snapshot + artifact-rule scoping bullets; guidelines lists keep both sides' bullets. - Regenerated skills/ and parity hashes rather than hand-merging them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… description zcode was registered but missing from the adapters barrel (its test imported the module directly), and the completion registry still carried the pre-#1062 description for the instructions command. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…-audit follow-ups (Fission-AI#1475) * fix(archive): make the scenario-drift check fence-aware parseScenarioBlocks matched #### Scenario: headers on raw lines while the validator's countScenarios masks fenced code blocks (Fission-AI#1151). The drift check (Fission-AI#1391) inherited the raw scan, so a fenced scenario example in the current spec aborted an archive that validate had passed, and a fenced name in the MODIFIED block counted as keeping a scenario the block had actually dropped. Build the shared code-fence mask and skip masked lines in both the header scan and the block-end scan. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(update): tear down the redirected request when the budget expires The overall request budget was armed inside the first send() and its callback closed over that hop's request. After a redirect the timer destroyed the already-dead first request, so a redirect target that trickled bytes kept resetting its idle timeout and held the socket open until the body-size cap. Track the in-flight request and have the budget timer destroy whichever one is open. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(release): add changesets for user-facing changes missing from the 1.7.0 notes 18 feat/fix commits merged since v1.6.0 without a changeset, so the pending Version Packages PR would have released them silently: five tool integrations (ZCode, Hermes, CodeArts, Kimi Code rename, Codex skills-only), skills.sh distribution, symlinked schema dirs, nested spec discovery, drift multiplicity, checkbox markers, Windows welcome input, npx avoidance, doctor store drift, local dates, missing-core-workflows warning, store-aware main specs, open-questions guidance, and spec content guidance. Plus changesets for this branch's two fixes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(adapters): escape TOML-active characters in Gemini command files The gemini adapter interpolated the description into a TOML basic string and the body into a multiline basic string with no escaping. Every current template value happens to be safe; the first description with a double quote or backslash would silently produce invalid TOML for all Gemini command files. Escape both contexts (Fission-AI#1447 fixed the same class for the YAML adapters but scoped itself to YAML). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(update): harden install detection and redirect handling Three follow-ups from the release audit: - A path segment literally named volta (a user or project directory) classified the install as volta-managed and swallowed the upgrade offer. The undotted spelling now requires volta's own tools/image layout, matching how pnpm and yarn already demand corroboration. - The Windows npm-ownership fallback checked that the npm prefix exists, which is true of any X\node_modules\pkg tree, hand-copied ones included. Corroborate with the openspec.cmd shim npm actually writes. - A https registry redirecting to plain http was followed; a MITM on that reply controls the newer-version answer. Refuse the downgrade. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(cli): export zcodeAdapter from the barrel and sync a completion description zcode was registered but missing from the adapters barrel (its test imported the module directly), and the completion registry still carried the pre-Fission-AI#1062 description for the instructions command. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(parser): strip a UTF-8 BOM before parsing specs and deltas A BOM-prefixed delta spec (Windows editors, PowerShell Out-File) failed validate and archive with 'No delta sections found' because the first line never matched '## ADDED Requirements'. Strip the BOM in both normalizers, the same way tool detection already does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(cli): reject over-long change names with a validation message A 300-character change name surfaced two raw ENAMETOOLONG errno dumps from stat and mkdir. Bound the name at 200 characters in validateChangeName so the failure is a normal validation error. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(archive): finish the early-sync no-op rules for MODIFIED and RENAMED Two asymmetries left over from the Fission-AI#1376/Fission-AI#1386/Fission-AI#1437 no-op work: - MODIFIED counted every delta as applied even when the block was byte-equal to the main spec, so a fully early-synced change rewrote the file (normalization churn), printed '~ N modified', and reported specsUpdated: true where its ADDED/REMOVED/RENAMED twins print 'Specs already in sync; no files changed.' Count only real replacements. - RENAMED's already-synced skip (source gone, target present) had no near-miss guard: a case/whitespace variant of the source still in the spec means a typo'd header, and REMOVED already hard-aborts on that signal. Apply the same guard, excluding the target itself so a case-only rename still no-ops. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(validate): stop reporting an unreadable specs dir as 'no deltas' The delta-validation loop swallowed every error as 'if no specs dir, treat as no deltas', so an EACCES capability folder produced the misleading 'Change must have at least one delta' while archive let the same error propagate. Tolerate only ENOENT and ENOTDIR (a stray specs file); anything else stays loud, matching discoverSpecFiles' documented fail-loud contract. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(update): say when commands-only delivery leaves a tool with nothing Under delivery: commands, update removed the skills of adapterless skills-only tools (Hermes, Kimi Code, Vibe, CodeArts, ForgeCode) without a word — leaving zero OpenSpec artifacts while the tool's detection dir kept re-suggesting an init that would also generate nothing. Print the same per-tool configuration correction init already prints, pointing at 'openspec config set delivery both'. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(completion): honor $ZSH and $ZSH_CUSTOM for Oh My Zsh installs The installer used a set $ZSH only as an is-installed signal and then wrote to ~/.oh-my-zsh regardless, so a custom OMZ location got a freshly created ~/.oh-my-zsh tree that no shell ever loads — and isInstalled/uninstall looked in the same wrong place. Route every path through the $ZSH/$ZSH_CUSTOM-aware helpers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(init): make the static welcome screen wait for the Enter it asks for The static branch printed 'Press Enter to select tools...' and returned immediately, so the Enter landed in the tool picker and submitted the pre-selected set sight-unseen. Fission-AI#1462 routed reduced-motion, OPENSPEC_NO_ANIMATION, --no-animation, NO_COLOR, and narrow-terminal users onto this path. Wait in a TTY; drop the prompt line when there is no TTY to wait on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(feedback): keep the manual fallback on every gh failure Only missing-gh and unauthenticated flows showed the formatted feedback and pre-filled submission URL; issues-disabled, network, or rate-limit failures printed gh's stderr and discarded the path to submit what the user had already typed. Route those through the same manual fallback, preserving gh's exit code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(update): model the npm shim in the Windows prefix fixture The ownership corroboration now checks for the openspec.cmd shim npm writes beside node_modules; the Homebrew-prefix fixture built the layout without it, so the test failed on windows-pwsh. Write the shim in the fixture and pin the inverse: the same shape with nothing npm wrote (a hand-copied portable tree) is not an npm install. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(update): require volta's full tools/image layout for the undotted spelling The corroboration used has('tools', 'image'), which is some() — volta AND (tools OR image) — so /srv/volta/tools/apps/... still classified as a Volta install and swallowed the upgrade offer. Require both segments, matching the real %LOCALAPPDATA%\Volta\tools\image layout. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(adapters): escape control characters in Gemini multiline prompts escapeTomlMultilineBasicString handled backslashes and quote-triples but not the C0 controls that are as invalid in a multiline basic string as in a single-line one. Reuse TOML_CONTROL_CHARS, applied last so the escapes it introduces are not re-doubled. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(completion): finish the $ZSH_CUSTOM support and isolate it in tests The fpath verification advice still grepped the literal custom/completions, which a relocated $ZSH_CUSTOM need never contain — grep the actual directory instead. The installer tests cleared only $ZSH, so on a machine exporting $ZSH_CUSTOM they would have written into (and deleted from) the developer's real OMZ custom dir — the same leakage class Fission-AI#1400 fixed for $ZSH. Clear/restore both, and pin the custom-location paths with two new tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(release): correct the hermes and zcode changeset wording Hermes is skills-only (no command adapter), and zcode's namespaced commands register /opsx:<id>, not /opsx-* — the release notes must not reintroduce the invocation-spelling confusion Fission-AI#1471 removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(feedback): pin the manual fallback on a non-label gh failure The new reportGhFailure output (formatted feedback + pre-filled URL) had no coverage; the network-failure test now asserts it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(completion): match fpath entries as literal strings in the OMZ guidance The verification advice interpolated the completions dir into grep "<dir>" where regex metacharacters make the check unreliable and quotes could break the displayed command. Print one fpath entry per line and match with grep -F on a shell-quoted literal. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(adapters): never emit a bare carriage return in Gemini TOML prompts A lone CR is illegal in a multiline basic string — Python 3.13 tomllib rejects the file — and the control-char pass deliberately skipped it on the assumption it only appears as CRLF. Normalize CRLF to LF and escape any remaining CR as \r. The escaping guarantee is now parser-backed: smol-toml (new devDependency) round-trips every hostile body in the regression matrix (lone CR, CRLF, CR before a quote run, NUL/VT/FF, trailing backslash, four- and five-quote runs), and the same outputs were verified against Python tomllib. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * build(nix): update the pnpm deps hash for the smol-toml devDependency The lockfile changed, so the fixed-output derivation hash moved; value taken from the CI mismatch report. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
4 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