fix(workflows): preserve nested spec paths - #1508
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (26)
🚧 Files skipped from review as they are similar to previous changes (23)
📝 WalkthroughWalkthroughThe change standardizes ChangesNested capability path support
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/core/templates/workflows/bulk-archive-change.ts (1)
20-21: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winKey bulk-archive conflicts by the full capability path.
The new definition supports nested paths, but the conflict step at Line 88 still names the key
capabilityand shows only leaf names. Define the key as the exact path relative tospecs/. Otherwiseidentity/user-authandbilling/user-authcan collide by leaf name. Bulk archive can then apply an incorrect conflict decision or ordering to one delta.Apply the wording to both template variants and regenerate
skills/openspec-bulk-archive-change/SKILL.md.Proposed wording
- Build a map of `capability -> [changes that touch it]`: + Build a map of `<capability-path>` (the exact path relative to `specs/`) -> [changes that touch it]:Please verify that conflict resolution uses this full path as its identity.
Based on learnings, committed
skills/<skill-name>/SKILL.mdfiles are generated output ofscripts/generate-skillssh.mjsand must match generator output.Also applies to: 359-360
🤖 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/templates/workflows/bulk-archive-change.ts` around lines 20 - 21, Update both bulk-archive workflow template variants so conflict resolution keys each delta by its full capability path relative to specs/, preserving nested segments instead of using the leaf name or capability key. Ensure conflict detection and ordering consistently use this path identity, then regenerate skills/openspec-bulk-archive-change/SKILL.md with scripts/generate-skillssh.mjs.Source: Learnings
🤖 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/specs/openspec-conventions/spec.md`:
- Line 227: Update the diff command in the command-line guidance to use the
defined <capability-path> placeholder consistently, replace the square-bracket
placeholders with angle-bracket placeholders, and quote both paths so shell
globbing cannot alter them before substitution.
In `@src/core/templates/workflows/explore.ts`:
- Around line 126-135: Synchronize the committed openspec-explore skill template
after the guidance change in the workflow template: regenerate
skills/openspec-explore/SKILL.md or update its parity hash using the project’s
established generator, so test/core/templates/skill-templates-parity.test.ts
matches the intentional content.
---
Nitpick comments:
In `@src/core/templates/workflows/bulk-archive-change.ts`:
- Around line 20-21: Update both bulk-archive workflow template variants so
conflict resolution keys each delta by its full capability path relative to
specs/, preserving nested segments instead of using the leaf name or capability
key. Ensure conflict detection and ordering consistently use this path identity,
then regenerate skills/openspec-bulk-archive-change/SKILL.md with
scripts/generate-skillssh.mjs.
🪄 Autofix
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 Plus
Run ID: 361c9c3b-da5b-4e30-b6b6-6ee0af39f029
📒 Files selected for processing (22)
docs/opsx.mddocs/troubleshooting.mddocs/writing-specs.mdopenspec/specs/cli-validate/spec.mdopenspec/specs/openspec-conventions/spec.mdopenspec/specs/specs-sync-skill/spec.mdschemas/spec-driven/schema.yamlschemas/spec-driven/templates/proposal.mdskills/openspec-archive-change/SKILL.mdskills/openspec-bulk-archive-change/SKILL.mdskills/openspec-explore/SKILL.mdskills/openspec-propose/SKILL.mdskills/openspec-sync-specs/SKILL.mdsrc/core/parsers/spec-structure.tssrc/core/templates/workflows/archive-change.tssrc/core/templates/workflows/bulk-archive-change.tssrc/core/templates/workflows/explore.tssrc/core/templates/workflows/propose.tssrc/core/templates/workflows/sync-specs.tssrc/core/validation/validator.tstest/core/artifact-graph/workflow.integration.test.tstest/core/templates/skill-templates-parity.test.ts
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Exact head cdeb9a7 is clean. Nested capability paths stay intact across schema guidance, generated workflows, sync/archive conflict identity, diagnostics, and docs; generated-skill parity, 86 focused tests, three strict living-spec validations, and the full hosted matrix all pass.
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Exact head 57c1594 is clean. The follow-up clarifies capability-path inputs and preserves caller-supplied absolute output paths without regressing nested-path identity across propose, onboard, sync, archive, validation, or generated guidance. A fresh build and 128 focused workflow, validation, loader, and parity tests pass, with hosted CI green.
57c1594 to
3a5ee27
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Re-reviewed the rebased head. Nested capability paths remain intact across schema guidance, generated workflows, conflict identity, validation, and diagnostics. A fresh build, 141 focused workflow/validation/loader/parity tests, strict validation of the three affected living specs, and the full hosted matrix pass.
2 participants
Status
LGTM.
What was wrong
OpenSpec already discovers, validates, syncs, and archives nested capability paths such as
identity/user-auth, but its generated workflow guidance still prescribed the flat-only pathspecs/<capability>/spec.md. Agents could therefore flatten an existing nested capability or create a delta in the wrong directory.How it was fixed
<capability-path>consistently across the spec-driven schema and the propose, explore, onboard, sync, archive, and bulk-archive workflows.existingOutputPathsentries verbatim, and require all bulk-archive examples to carry complete capability paths instead of ambiguous leaf-name shorthand.This changes guidance only. It does not change path resolution, storage, APIs, schema format, or runtime architecture.
Replication / proof
mainalready supports nested paths through recursive discovery, parsing, validation, sync, and archive coverage.node build.jseslint src/openspec-conventions,cli-validate, andspecs-sync-skillNotes / nits
PR #660 also proposes hierarchical specs, but it is a broad, conflicting runtime/configuration implementation from before recursive support landed on
main. This PR addresses only the remaining current-main guidance mismatch and does not adopt that PR's architecture.The issue suggests making
<domain>/<capability>mandatory. OpenSpec's current human documentation instead calls direct folders such asauth/andpayments/domains. This PR fixes the real flattening bug without introducing a new mandatory directory level or reorganizing existing projects.No changeset is included because this is a routine guidance and documentation correction.
Fixes #1459
Summary by CodeRabbit
Documentation
Bug Fixes
Tests