docs(sdd): compress SDD skills and add filesystem path convention notes#1004
Open
glats wants to merge 15 commits into
Open
docs(sdd): compress SDD skills and add filesystem path convention notes#1004glats wants to merge 15 commits into
glats wants to merge 15 commits into
Conversation
Adds explicit `Filesystem path convention` notes to:
- `sdd-orchestrator.md`: after Artifact Store Policy section
- `sdd-explore/SKILL.md`: after Retrieving Context section
- `sdd-init/SKILL.md`: after Hard Rules section
- `_shared/sdd-phase-common.md`: appended to end of document
- `windsurf/workflows/sdd-new.md`: appended as clarifier note
The Engram topic key prefix `sdd/` (e.g. `sdd/{change-name}/proposal`)
is for memory organization only — it is NOT a filesystem path. Models
should use `openspec/` (OpenSpec) or `.sdd/` (Windsurf IDE) as the
canonical filesystem directories, never `sdd/`, `sdds/`, or `.sdd/`
outside their intended context.
…path convention notes
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR rewrites multiple SDD skill and workflow instruction files to standardize executor gating, memory persistence, output envelopes, and filesystem path conventions. It also adds a canonical ChangesSDD instruction and path convention updates
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Pull request overview
This PR updates SDD skill documentation to clarify execution/output expectations and (intended) filesystem-path conventions to prevent agents from confusing Engram topic keys (sdd/...) with on-disk artifact locations.
Changes:
- Added an Output Contract section to
sdd-exploredescribing the fields the sub-agent should return. - Added a Filesystem path convention note to
sdd-applystating SDD artifacts live underopenspec/(notsdd//.sdd//sdds/), plus minor Markdown spacing fixes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/assets/skills/sdd-explore/SKILL.md | Adds an Output Contract section; needs alignment with the shared return-envelope contract. |
| internal/assets/skills/sdd-apply/SKILL.md | Adds filesystem-path guidance for apply and fixes Markdown heading spacing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+144
to
+146
| Return status, executive_summary, artifacts, next_recommended, risks. | ||
| Include current state, affected areas, approaches, recommendation. | ||
| See `_shared/sdd-phase-common.md` Section D for envelope format. |
Comment on lines
+56
to
+58
| **Filesystem path convention**: The SDD artifact directory is `openspec/`. | ||
| Do NOT use `sdd/`, `.sdd/`, or `sdds/` as filesystem paths — these do not | ||
| exist. Engram topic keys use the `sdd/` prefix for memory organization only. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/assets/skills/sdd-explore/SKILL.md`:
- Around line 142-147: The Output Contract in SKILL.md is missing the required
skill_resolution field, so update the contract to list it alongside status,
executive_summary, artifacts, next_recommended, and risks. Make sure the wording
in this section matches the shared envelope requirements from
_shared/sdd-phase-common.md and the orchestrator gatekeeper, and verify any
nearby references to the contract or envelope format are consistent with the new
required field.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 98ecf415-a630-4c97-90f3-86399e8ce04b
📒 Files selected for processing (2)
internal/assets/skills/sdd-apply/SKILL.mdinternal/assets/skills/sdd-explore/SKILL.md
…compression) into main Resolves merge conflicts in sdd-apply and sdd-explore: - sdd-apply: combined compressed PR version with Filesystem path convention note - sdd-explore: combined compressed PR version with Output Contract section
openspec/changes/archive/YYYY-MM-DD-{change-name}/ instead of
abbreviated archive/YYYY-MM-DD-{name}/. Prevents agents from
creating archive folders at the repo root.
CodeRabbit fix from PR Gentleman-Programming#988.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
internal/assets/skills/sdd-apply/SKILL.md:39
- This file appears to be truncated: it stops immediately after the "## Status and Workspace Guard" heading and never closes the
<!-- section:model-capable -->block. As-is, the sdd-apply skill no longer contains the steps/rules it’s supposed to enforce, which will break downstream usage.
**Filesystem path convention**: The SDD artifact directory is `openspec/`.
Do NOT use `sdd/`, `.sdd/`, or `sdds/` as filesystem paths — these do not
exist. Engram topic keys use the `sdd/` prefix for memory organization only.
## Status and Workspace Guard
Comment on lines
+54
to
+58
| ## Output Contract | ||
|
|
||
| ### Ready for Proposal | ||
| {Yes/No — and what the orchestrator should tell the user} | ||
| ``` | ||
| Return status, executive_summary, artifacts, next_recommended, risks. | ||
| Include current state, affected areas, approaches, recommendation. | ||
| See `_shared/sdd-phase-common.md` Section D for envelope format. |
Comment on lines
18
to
+34
| ## Purpose | ||
|
|
||
| You are a sub-agent responsible for creating the TASK BREAKDOWN. You take the proposal, specs, and design, then produce a `tasks.md` with concrete, actionable implementation steps organized by phase. | ||
|
|
||
| ## What You Receive | ||
|
|
||
| From the orchestrator: | ||
| - Change name | ||
| - Artifact store mode (`engram | openspec | hybrid | none`) | ||
| - Delivery strategy (`ask-on-risk | auto-chain | single-pr | exception-ok`) | ||
| Create TASK BREAKDOWN from proposal, specs, and design. | ||
|
|
||
| ## Execution and Persistence Contract | ||
| > Follow `skills/_shared/sdd-phase-common.md`. | ||
|
|
||
| > Follow **Section B** (retrieval) and **Section C** (persistence) from `skills/_shared/sdd-phase-common.md`. | ||
|
|
||
| - **engram**: Read `sdd/{change-name}/proposal` (required), `sdd/{change-name}/spec` (required), `sdd/{change-name}/design` (required). Save as `sdd/{change-name}/tasks`. | ||
| - **engram**: Use `mem_get_observation` for Engram topics `sdd/{change-name}/proposal` (required), `sdd/{change-name}/spec` (required), `sdd/{change-name}/design` (required). Save with `mem_save(topic_key="sdd/{change-name}/tasks", ...)`. Do NOT read or write filesystem paths under `sdd/` — use `openspec/` for filesystem artifacts. | ||
| - **openspec**: Read and follow `skills/_shared/openspec-convention.md`. | ||
| - **hybrid**: Follow BOTH conventions — persist to Engram AND write `tasks.md` to filesystem. Retrieve dependencies from Engram (primary) with filesystem fallback. | ||
| - **none**: Return result only. Never create or modify project files. | ||
| - **hybrid**: Follow BOTH conventions. | ||
| - **none**: Return result only. | ||
|
|
||
| ## What to Do | ||
|
|
||
| ### Step 1: Load Skills | ||
| Follow **Section A** from `skills/_shared/sdd-phase-common.md`. | ||
|
|
||
| ### Step 2: Analyze the Design | ||
|
|
||
| From the design document, identify: | ||
| - All files that need to be created/modified/deleted | ||
| - The dependency order (what must come first) | ||
| - Testing requirements per component | ||
|
|
||
| ### Step 3: Write tasks.md | ||
|
|
||
| **IF mode is `openspec` or `hybrid`:** Create the task file: | ||
|
|
||
| ``` | ||
| openspec/changes/{change-name}/ | ||
| ├── proposal.md | ||
| ├── specs/ | ||
| ├── design.md | ||
| └── tasks.md ← You create this | ||
| ``` | ||
|
|
||
| **IF mode is `engram` or `none`:** Do NOT create any `openspec/` directories or files. Compose the tasks content in memory — you will persist it in Step 4. | ||
|
|
||
| #### Task File Format | ||
|
|
||
| ```markdown | ||
| # Tasks: {Change Title} | ||
|
|
||
| ## Review Workload Forecast | ||
|
|
||
| | Field | Value | | ||
| |-------|-------| | ||
| | Estimated changed lines | <rough estimate or range> | | ||
| | 400-line budget risk | Low / Medium / High | | ||
| | Chained PRs recommended | Yes / No | | ||
| | Suggested split | <single PR or PR 1 → PR 2 → PR 3> | | ||
| | Delivery strategy | <ask-on-risk / auto-chain / single-pr / exception-ok> | | ||
| | Chain strategy | <stacked-to-main / feature-branch-chain / size-exception / pending> | | ||
|
|
||
| Decision needed before apply: <Yes|No> | ||
| Chained PRs recommended: <Yes|No> | ||
| Chain strategy: <stacked-to-main|feature-branch-chain|size-exception|pending> | ||
| 400-line budget risk: <Low|Medium|High> | ||
|
|
||
| ### Suggested Work Units | ||
|
|
||
| | Unit | Goal | Likely PR | Notes | | ||
| |------|------|-----------|-------| | ||
| | 1 | <standalone deliverable> | PR 1 | <base branch; tests/docs included> | | ||
| | 2 | <standalone deliverable> | PR 2 | <immediate parent/base branch boundary; depends on PR 1 or independent> | | ||
|
|
||
| ## Phase 1: {Phase Name} (e.g., Infrastructure / Foundation) | ||
|
|
||
| - [ ] 1.1 {Concrete action — what file, what change} | ||
| - [ ] 1.2 {Concrete action} | ||
| - [ ] 1.3 {Concrete action} | ||
|
|
||
| ## Phase 2: {Phase Name} (e.g., Core Implementation) | ||
|
|
||
| - [ ] 2.1 {Concrete action} | ||
| - [ ] 2.2 {Concrete action} | ||
| - [ ] 2.3 {Concrete action} | ||
| - [ ] 2.4 {Concrete action} | ||
|
|
||
| ## Phase 3: {Phase Name} (e.g., Testing / Verification) | ||
|
|
||
| - [ ] 3.1 {Write tests for ...} | ||
| - [ ] 3.2 {Write tests for ...} | ||
| - [ ] 3.3 {Verify integration between ...} | ||
|
|
||
| ## Phase 4: {Phase Name} (e.g., Cleanup / Documentation) | ||
|
|
||
| - [ ] 4.1 {Update docs/comments} | ||
| - [ ] 4.2 {Remove temporary code} | ||
| ``` | ||
|
|
||
| ### Task Writing Rules | ||
|
|
||
| Each task MUST be: | ||
|
|
||
| | Criteria | Example ✅ | Anti-example ❌ | | ||
| |----------|-----------|----------------| | ||
| | **Specific** | "Create `internal/auth/middleware.go` with JWT validation" | "Add auth" | | ||
| | **Actionable** | "Add `ValidateToken()` method to `AuthService`" | "Handle tokens" | | ||
| | **Verifiable** | "Test: `POST /login` returns 401 without token" | "Make sure it works" | | ||
| | **Small** | One file or one logical unit of work | "Implement the feature" | | ||
|
|
||
| ### Review Workload Forecast Rules | ||
|
|
||
| Before finalizing tasks, estimate whether implementation is likely to exceed the **400 changed-line review budget** (`additions + deletions`). This is a planning guard, not an exact diff count. | ||
|
|
||
| Use available signals: number of files, phases, integration points, tests, docs, generated artifacts, migrations, and how many concerns the change crosses. | ||
|
|
||
| If the estimate is **High** or likely above 400 lines: | ||
|
|
||
| 1. Mark `Chained PRs recommended` as `Yes`. | ||
| 2. Split tasks into **work units** that can become chained or stacked PRs. | ||
| 3. Each suggested PR must have a clear start, clear finish, verification, and autonomous scope. | ||
| 4. **Ask the user which chain strategy to use** (this is a team decision): | ||
| - **Stacked PRs to main** — each PR merges to main in order. Fast iteration, fix on the go. Best for speed-first teams and independent slices. | ||
| - **Feature Branch Chain** — the feature/tracker branch accumulates the final integration; PR #1 targets the tracker branch, later PRs target the immediate previous PR branch so each child diff stays focused. Only the tracker merges to main. Best for rollback control and coordinated releases. | ||
| - **size:exception** — keep it as a single PR with maintainer approval. Best for generated code, migrations, or vendor diffs. | ||
| 5. Cache the user's choice and set `Decision needed before apply` from delivery strategy: | ||
| - `ask-on-risk`: `Yes` — orchestrator asks before apply. | ||
| - `auto-chain`: `No` — orchestrator proceeds with the first slice using the chosen chain strategy. | ||
| - `single-pr`: `Yes` — orchestrator must require `size:exception` before apply. | ||
| - `exception-ok`: `No` — maintainer has accepted `size:exception`. | ||
|
|
||
| Do not bury this in prose. Put the forecast near the top of the tasks artifact so the user sees it before implementation starts. | ||
|
|
||
| The forecast MUST include these exact plain-text lines so downstream guards can match them literally: | ||
|
|
||
| ```text | ||
| Decision needed before apply: Yes|No | ||
| Chained PRs recommended: Yes|No | ||
| Chain strategy: stacked-to-main|feature-branch-chain|size-exception|pending | ||
| 400-line budget risk: Low|Medium|High | ||
| ``` | ||
|
|
||
| You may keep the table for readability, but the plain-text lines are the guard contract. | ||
|
|
||
| For `feature-branch-chain`, suggested work units SHOULD name the intended base boundary: PR #1 base = feature/tracker branch; PR #2 base = PR #1 branch; PR #3 base = PR #2 branch. If a child PR would show previous PR changes, the base is wrong and must be retargeted/rebased before review. | ||
|
|
||
| ### Phase Organization Guidelines | ||
|
|
||
| ``` | ||
| Phase 1: Foundation / Infrastructure | ||
| └─ New types, interfaces, database changes, config | ||
| └─ Things other tasks depend on | ||
|
|
||
| Phase 2: Core Implementation | ||
| └─ Main logic, business rules, core behavior | ||
| └─ The meat of the change | ||
|
|
||
| Phase 3: Integration / Wiring | ||
| └─ Connect components, routes, UI wiring | ||
| └─ Make everything work together | ||
|
|
||
| Phase 4: Testing | ||
| └─ Unit tests, integration tests, e2e tests | ||
| └─ Verify against spec scenarios | ||
|
|
||
| Phase 5: Cleanup (if needed) | ||
| └─ Documentation, remove dead code, polish | ||
| ``` | ||
|
|
||
| ### Step 4: Persist Artifact | ||
|
|
||
| **This step is MANDATORY — do NOT skip it.** | ||
|
|
||
| Follow **Section C** from `skills/_shared/sdd-phase-common.md`. | ||
| - artifact: `tasks` | ||
| - topic_key: `sdd/{change-name}/tasks` | ||
| - type: `architecture` | ||
|
|
||
| ### Step 5: Return Summary | ||
|
|
||
| Return to the orchestrator: | ||
|
|
||
| ```markdown | ||
| ## Tasks Created | ||
|
|
||
| **Change**: {change-name} | ||
| **Location**: `openspec/changes/{change-name}/tasks.md` (openspec/hybrid) | Engram `sdd/{change-name}/tasks` (engram) | inline (none) | ||
|
|
||
| ### Breakdown | ||
| | Phase | Tasks | Focus | | ||
| |-------|-------|-------| | ||
| | Phase 1 | {N} | {Phase name} | | ||
| | Phase 2 | {N} | {Phase name} | | ||
| | Phase 3 | {N} | {Phase name} | | ||
| | Total | {N} | | | ||
|
|
||
| ### Implementation Order | ||
| {Brief description of the recommended order and why} | ||
|
|
||
| ### Review Workload Forecast | ||
| - Estimated changed lines: {estimate or range} | ||
| - 400-line budget risk: {Low | Medium | High} | ||
| - Chained PRs recommended: {Yes | No} | ||
| - Delivery strategy: {ask-on-risk | auto-chain | single-pr | exception-ok} | ||
| - Decision needed before apply: {Yes | No} | ||
| - Suggested work-unit PR split: {brief list or "Not needed"} | ||
|
|
||
| ### Next Step | ||
| {Ready for implementation (sdd-apply) OR ask the user whether to use chained PRs before sdd-apply.} | ||
| ``` | ||
| 1. Load Skills. | ||
| 2. Analyze design for files, dependencies, testing requirements. | ||
| 3. Write tasks.md with Review Workload Forecast, phases, checklist items. | ||
| 4. Persist: `mem_save(topic_key="sdd/{change-name}/tasks")` or write file. | ||
| 5. Return summary with phase breakdown, forecast, next step. |
Comment on lines
38
to
40
| ## What to Do | ||
| ## What to Do | ||
|
|
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@internal/assets/skills/_shared/sdd-phase-common.md`:
- Around line 111-113: The filesystem path convention in the shared SDD phase
protocol is too broad and conflicts with the Windsurf workflow. Update the
guidance in the shared SDD phase content so the ban on `sdd/`, `.sdd/`, and
`sdds/` applies only to non-Windsurf flows, while preserving `.sdd/` as the
valid artifact directory for Windsurf; keep the wording aligned with the
existing filesystem path convention section so it no longer contradicts
`sdd-new.md`.
In `@internal/assets/skills/sdd-design/SKILL.md`:
- Around line 27-30: Update the `sdd-design/SKILL.md` convention so `spec` is
required for the design phase instead of optional. In the `engram` guidance,
make `mem_get_observation` for `sdd/{change-name}/spec` mandatory alongside
`sdd/{change-name}/proposal`, and keep
`mem_save(topic_key="sdd/{change-name}/design", ...)` unchanged. Also adjust the
`hybrid` wording if needed so it inherits the same required-spec rule from the
`engram` convention.
In `@internal/assets/skills/sdd-explore/SKILL.md`:
- Line 36: The filesystem path guidance in SKILL.md is too broad: it should keep
openspec/ as the canonical SDD artifact directory while not declaring .sdd/
nonexistent in all contexts. Update the path-convention wording to scope the
.sdd/ restriction to non-Windsurf usage, and make sure the reference to the SDD
artifact directory clearly distinguishes Windsurf-specific paths from the
general filesystem convention.
In `@internal/assets/skills/sdd-init/SKILL.md`:
- Line 27: The filesystem path guidance in SKILL.md is too broad and incorrectly
says `.sdd/` does not exist. Update the path-convention text to keep `openspec/`
as the canonical SDD artifact directory while explicitly limiting the `.sdd/`
prohibition to non-Windsurf contexts; use the existing “Filesystem path
convention” section in SKILL.md to clarify that `.sdd/` is Windsurf-only rather
than globally invalid.
- Around line 46-47: The initialization output contract is missing the
orchestrator-required skill_resolution field, so update the Output Contract in
SKILL.md to include it alongside status, executive_summary, artifacts,
next_recommended, risks, and the other required metadata. Make sure the
initialization envelope and any related schema/docs reference the same field
name exactly so downstream validation accepts the returned result.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 71814701-8200-4b2c-8b6a-e1c4b46f6d9a
📒 Files selected for processing (11)
internal/assets/opencode/sdd-orchestrator.mdinternal/assets/skills/_shared/sdd-phase-common.mdinternal/assets/skills/sdd-apply/SKILL.mdinternal/assets/skills/sdd-archive/SKILL.mdinternal/assets/skills/sdd-design/SKILL.mdinternal/assets/skills/sdd-explore/SKILL.mdinternal/assets/skills/sdd-init/SKILL.mdinternal/assets/skills/sdd-propose/SKILL.mdinternal/assets/skills/sdd-spec/SKILL.mdinternal/assets/skills/sdd-tasks/SKILL.mdinternal/assets/windsurf/workflows/sdd-new.md
…Programming#1004 A: Scope .sdd/ ban to non-Windsurf contexts (3 files) B: Add skill_resolution to Output Contract (sdd-explore, sdd-init) C: Make spec required in sdd-design (was optional) D: Remove duplicate Output Contract + heading in sdd-explore
glats
changed the title
sdd/ ↔ openspec/ confusion
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
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
SDD orchestrator and sub-agents (sdd-explore, sdd-init, sdd-apply, etc.) sometimes confuse Engram topic keys (prefix
sdd/) with filesystem paths, generating globs like**/.sdd/**and**/sdd/**during exploration. These directories don't exist — the canonical filesystem path isopenspec/.Root cause: the orchestrator's Engram topic key table (
sdd/{change-name}/proposal, etc.) pattern-matches to a mental filesystem path because thesdd/prefix collides with the old.sdd/Windsurf IDE convention.Changes
SDD skill compression (~70% reduction): Condenses verbose step-by-step instructions into compact numbered lists across all 8 SDD phase skills (apply, archive, design, explore, init, propose, spec, tasks).
Filesystem path convention notes: Adds explicit
Filesystem path conventionnotes to 5 files:internal/assets/opencode/sdd-orchestrator.mdinternal/assets/skills/sdd-explore/SKILL.mdinternal/assets/skills/sdd-init/SKILL.mdinternal/assets/skills/_shared/sdd-phase-common.mdinternal/assets/windsurf/workflows/sdd-new.mdEach note clarifies:
sdd/prefix for memory organization only — NOT filesystem pathsopenspec/.sdd/is the Windsurf IDE artifact directory (allowed in Windsurf-only contexts)Code Review Fixes Applied
skill_resolutionto Output Contracts (sdd-explore, sdd-init)specrequired in sdd-design (was optional).sdd/ban to non-Windsurf contexts (sdd-phase-common, sdd-explore, sdd-init)openspec/changes/archive/formBackground
This change is Track 2 of a two-track fix. Track 1 (local Nix overrides) has been cleaned up in the nixos-hosts repo — all SDD skills now come from upstream. This PR provides the same guardrails upstream so downstream consumers don't need per-repo workarounds.
Summary by CodeRabbit
openspec/and tightened guidance to avoidsdd/,.sdd/, andsdds/as filesystem paths (sdd/is for Engram topic-key prefixes only).