feat: support multi-level change domains and sibling archives#1367
feat: support multi-level change domains and sibling archives#1367terryzhong wants to merge 20 commits into
Conversation
📝 WalkthroughWalkthroughThe PR introduces domain-qualified change IDs, recursive change/spec discovery, sibling ChangesDomain-aware change and archive workflow
Estimated code review effort: 5 (Critical) | ~120 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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/commands/validate.ts (1)
143-147: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winScope nearest-match suggestions to the requested type.
When a
typeOverrideis provided but the item doesn't exist in that category (e.g., it exists as a change, but--type specwas passed), the current logic searches bothchangesandspecsfor suggestions. This can result in a confusing UX where the CLI suggests the exact same item name the user just typed (e.g.,Unknown item 'add-login'. Did you mean: add-login?).Scope the candidates to the requested type so suggestions are always relevant to what the user is trying to validate, and update the noun in the message to match the context.
🐛 Proposed fix to scope suggestions
- if (!type) { - const suggestions = nearestMatches(itemName, [...changes, ...specs]); - const message = suggestions.length - ? `Unknown item '${itemName}'. Did you mean: ${suggestions.join(', ')}?` - : `Unknown item '${itemName}'.`; + if (!type) { + const candidates = opts.typeOverride === 'change' ? changes : opts.typeOverride === 'spec' ? specs : [...changes, ...specs]; + const suggestions = nearestMatches(itemName, candidates); + const typeName = opts.typeOverride || 'item'; + const message = suggestions.length + ? `Unknown ${typeName} '${itemName}'. Did you mean: ${suggestions.join(', ')}?` + : `Unknown ${typeName} '${itemName}'.`;🤖 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/validate.ts` around lines 143 - 147, Update the unknown-item handling in the validation flow around the type lookup so nearestMatches only receives candidates from the requested type when typeOverride is provided, rather than combining changes and specs. Adjust the unknown-item message noun to reflect that type context, while preserving the existing combined-candidate behavior when no type override is specified.src/core/templates/workflows/onboard.ts (1)
189-198: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFetch status before referencing
changeRoot.The flow creates the change and immediately displays
changeRoot from status JSON, but never runs status. Resolve it before the SHOW step.Proposed fix
Set `<change-id>` to `<resolved-domain>/<name>` for a non-root domain, or `<name>` for root placement. Keep any selected `--store <id>` on every supported follow-up command. + +Run `openspec status --change "<change-id>" --json`, preserving `--store <id>` when selected. **SHOW:**🤖 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/onboard.ts` around lines 189 - 198, Update the onboarding flow before the SHOW step to run the status command and resolve its JSON response, ensuring the displayed “Created” value uses the fetched changeRoot. Preserve the existing change creation arguments and follow-up command behavior.
🧹 Nitpick comments (4)
src/core/list.ts (1)
104-109: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename variables for clarity.
The variable
changeDirsnow holds an array ofchangeIdsreturned bygetActiveChangeIds. UsingchangeDiras the loop variable name is slightly confusing since it represents an ID (which could be a nested path likePlatform/API/add-auth) rather than an absolute directory path. Consider renaming it tochangeIdto reflect its actual content.♻️ Proposed refactor
- for (const changeDir of changeDirs) { - const progress = await getTaskProgressForChange(changesDir, changeDir, targetPath); - const changePath = getChangePath(changesDir, changeDir); + for (const changeId of changeDirs) { + const progress = await getTaskProgressForChange(changesDir, changeId, targetPath); + const changePath = getChangePath(changesDir, changeId); const lastModified = await getLastModified(changePath); changes.push({ - name: changeDir, + name: changeId, completedTasks: progress.completed, totalTasks: progress.total, lastModified🤖 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/list.ts` around lines 104 - 109, Rename the `changeDirs` loop variable from `changeDir` to `changeId` in the iteration that processes results from `getActiveChangeIds`, and update all references within that loop, including calls to `getTaskProgressForChange` and `getChangePath`.docs/superpowers/specs/2026-06-04-loosen-domain-validation-design.md (1)
43-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSpecify a language for the fenced code block.
To satisfy markdown linting rules and improve syntax highlighting, consider adding a language identifier (such as
textorpseudo-code) to this code block.♻️ Proposed refactor
-``` +```text🤖 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 `@docs/superpowers/specs/2026-06-04-loosen-domain-validation-design.md` at line 43, Update the fenced code block in the specification to include an appropriate language identifier, such as text or pseudo-code, immediately after the opening fence while preserving its contents.Source: Linters/SAST tools
src/commands/change.ts (1)
155-160: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueReuse the already read
content.The file
proposal.mdis read intocontenton line 155, but then it is read again from disk on line 160 when instantiating theChangeParser. You can reuse the existingcontentvariable to avoid the redundant filesystem read.♻️ Proposed refactor
const content = await fs.readFile(proposalPath, 'utf-8'); const title = this.extractTitle(content, changeName); const { total, completed } = await getTaskProgressForChange(changesPath, changeName, process.cwd()); const taskStatusText = total > 0 ? ` [tasks ${completed}/${total}]` : ''; const changeDir = path.join(changesPath, changeName); - const parser = new ChangeParser(await fs.readFile(proposalPath, 'utf-8'), changeDir); + const parser = new ChangeParser(content, changeDir);🤖 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/change.ts` around lines 155 - 160, Update the ChangeParser construction in the change command to pass the already loaded content variable instead of reading proposal.md from disk again. Keep the existing extractTitle and task-progress flow unchanged.docs/stores-beta/user-guide.md (1)
77-82: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSpecify a language for the fenced code block.
This fenced code block is missing a language identifier. Consider adding
textto improve syntax highlighting and comply with Markdown linting rules.📝 Proposed refactor
-``` +```text Using OpenSpec root: team-plans (/Users/you/openspec/team-plans) Created change 'auth/add-login' at /Users/you/openspec/team-plans/openspec/changes/auth/add-login/ Schema: spec-driven Next: openspec status --change auth/add-login --store team-plans</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@docs/stores-beta/user-guide.mdaround lines 77 - 82, Update the fenced code
block in the user guide example to include the text language identifier, while
preserving its command output unchanged.</details> <!-- cr-comment:v1:251ba5489b43228e4c507d5f --> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>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@docs/concepts.md:
- Line 546: Update the archive diagram adjacent to the “Move to archive” step in
docs/concepts.md to show the sibling openspec/archive layout instead of
openspec/changes/archive, while preserving the date prefix and domain path
structure.In
@docs/superpowers/specs/2026-06-03-domain-archive-restructure-design.md:
- Line 42: Update the naming constraints and related domain documentation to
reflect the implementation: domain segments must allow case-preserving names and
characters such as those in Platform/API.v2/team_alpha, while only the final
change name remains subject to kebab-case validation. Preserve the existing
uniqueness requirement for change IDs within a planning home and update the
referenced sections consistently.In
@src/commands/spec.ts:
- Around line 160-163: Remove the existsSync(SPECS_DIR) early-return block in
the command flow so missing directories proceed through the normal spec ID
handling. Reuse getSpecIds()’s empty-array result, allowing JSON mode to emit []
and text mode to reach the existing No items found check.In
@src/commands/workflow/new-change.ts:
- Around line 262-270: Update the domain resolution flow around
validateExplicitDomain, promptForDomain, and changeId to compare each requested
path segment with existing sibling directories case-insensitively, rejecting
conflicting spellings or reusing the existing canonical spelling. Add the
required alias-path regression in
test/commands/workflow/new-change-domain.test.ts lines 104-115, covering an
existing Platform domain followed by a platform request.In
@src/core/archive.ts:
- Around line 231-238: Update the archive flow surrounding
pathExistsWithoutFollowingLinks and assertProspectivePathContained to initialize
or migrate archiveDir before validating archivePath containment. Ensure the
sibling directory exists before assertProspectivePathContained resolves the
archive root, while preserving the existing target-exists protection and later
archive creation behavior.In
@src/core/specs-apply.ts:
- Around line 75-81: Update the fs.readdir error handling in applySpecs so only
an ENOENT error for a missing changeSpecsDir returns updates unchanged; rethrow
permission, I/O, and all other filesystem errors. Preserve normal directory-read
behavior and apply the same handling to the corresponding error path noted in
the comment.In
@src/core/templates/workflows/archive-change.ts:
- Around line 86-90: The archive workflow must stop when the user selects Cancel
instead of proceeding unconditionally. Update both Cancel-handling branches in
src/core/templates/workflows/archive-change.ts at lines 86-90 and 194-198 to
return immediately before archive execution; apply the same behavior in both
template variants.- Around line 21-24: Replace the raw mkdir/mv archive steps in
src/core/templates/workflows/archive-change.ts lines 21-24 with a single
openspec archive invocation for the selected change, appending --store when
a store is selected. In src/core/templates/workflows/bulk-archive-change.ts
lines 17-20, invoke the same command once per selected change and record each
command result.In
@src/utils/change-path.ts:
- Around line 340-353: Update hasOwnChangeMarker to use fs.lstat instead of
fs.stat for .openspec.yaml and proposal.md, and require the result to be a
regular non-symlink file before returning true. Preserve the existing
missing-path handling and error propagation.In
@src/utils/change-utils.ts:
- Around line 16-47: The local canonicalizeProspectivePath and pathStaysWithin
logic duplicates the shared containment contract and may handle dangling links
or ancestor types differently. Remove or stop using these helpers, and update
createChange() to route prospective-path validation through
assertProspectivePathContained() from change-path.ts for all write paths.In
@test/utils/change-utils.test.ts:
- Around line 249-274: Add a positive alias-path regression test alongside the
existing escape test, creating a symlink/junction inside changesDir that
resolves to another directory also within changesDir and using it with
createChange. Assert the operation succeeds, the created path preserves the
alias spelling, and separately verify its canonical realpath matches the target
directory identity; keep the test compatible with platforms where link creation
is unavailable by using the existing skip handling.In
@test/vocabulary-sweep.test.ts:
- Around line 77-79: Standardize lifecycle documentation on the
change-id
terminology: update the vocabulary sweep regex in
test/vocabulary-sweep.test.tsto include/opsx:updateand detect stale
lifecycle argument names; indocs/commands.mdat lines 183, 277, 339, 376,
501, and 719, rename the affected argument rows, tips, syntax, and
troubleshooting references fromchange-nametochange-id, including
/opsx:updatesyntax.
Outside diff comments:
In@src/commands/validate.ts:
- Around line 143-147: Update the unknown-item handling in the validation flow
around the type lookup so nearestMatches only receives candidates from the
requested type when typeOverride is provided, rather than combining changes and
specs. Adjust the unknown-item message noun to reflect that type context, while
preserving the existing combined-candidate behavior when no type override is
specified.In
@src/core/templates/workflows/onboard.ts:
- Around line 189-198: Update the onboarding flow before the SHOW step to run
the status command and resolve its JSON response, ensuring the displayed
“Created” value uses the fetched changeRoot. Preserve the existing change
creation arguments and follow-up command behavior.
Nitpick comments:
In@docs/stores-beta/user-guide.md:
- Around line 77-82: Update the fenced code block in the user guide example to
include the text language identifier, while preserving its command output
unchanged.In
@docs/superpowers/specs/2026-06-04-loosen-domain-validation-design.md:
- Line 43: Update the fenced code block in the specification to include an
appropriate language identifier, such as text or pseudo-code, immediately after
the opening fence while preserving its contents.In
@src/commands/change.ts:
- Around line 155-160: Update the ChangeParser construction in the change
command to pass the already loaded content variable instead of reading
proposal.md from disk again. Keep the existing extractTitle and task-progress
flow unchanged.In
@src/core/list.ts:
- Around line 104-109: Rename the
changeDirsloop variable fromchangeDirto
changeIdin the iteration that processes results fromgetActiveChangeIds,
and update all references within that loop, including calls to
getTaskProgressForChangeandgetChangePath.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Path: .coderabbit.yaml **Review profile**: CHILL **Plan**: Pro **Run ID**: `fe8b746c-bf94-4cdf-87f4-f90db3581aa8` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 0a99f410457271aa773d8b106f03f637f7c6b3c0 and f6896fa95a6aa818cac688928f3293a774e0b7f6. </details> <details> <summary>📒 Files selected for processing (86)</summary> * `CHANGELOG.md` * `README.md` * `docs/agent-contract.md` * `docs/cli.md` * `docs/commands.md` * `docs/concepts.md` * `docs/customization.md` * `docs/examples.md` * `docs/faq.md` * `docs/getting-started.md` * `docs/glossary.md` * `docs/installation.md` * `docs/overview.md` * `docs/stores-beta/user-guide.md` * `docs/superpowers/plans/2026-06-03-domain-archive-restructure.md` * `docs/superpowers/plans/2026-06-04-loosen-domain-validation.md` * `docs/superpowers/plans/2026-07-15-domain-archive-upstream-reimplementation.md` * `docs/superpowers/specs/2026-06-03-domain-archive-restructure-design.md` * `docs/superpowers/specs/2026-06-04-loosen-domain-validation-design.md` * `docs/team-workflow.md` * `docs/troubleshooting.md` * `docs/workflows.md` * `src/cli/index.ts` * `src/commands/change.ts` * `src/commands/show.ts` * `src/commands/spec.ts` * `src/commands/store.ts` * `src/commands/validate.ts` * `src/commands/workflow/instructions.ts` * `src/commands/workflow/new-change.ts` * `src/commands/workflow/shared.ts` * `src/commands/workflow/status.ts` * `src/core/archive.ts` * `src/core/completions/command-registry.ts` * `src/core/init.ts` * `src/core/list.ts` * `src/core/openspec-root.ts` * `src/core/root-selection.ts` * `src/core/specs-apply.ts` * `src/core/templates/workflows/archive-change.ts` * `src/core/templates/workflows/bulk-archive-change.ts` * `src/core/templates/workflows/ff-change.ts` * `src/core/templates/workflows/new-change.ts` * `src/core/templates/workflows/onboard.ts` * `src/core/templates/workflows/propose.ts` * `src/core/templates/workflows/sync-specs.ts` * `src/core/view.ts` * `src/utils/change-path.ts` * `src/utils/change-utils.ts` * `src/utils/item-discovery.ts` * `test/cli-e2e/capstone-journeys.test.ts` * `test/cli-e2e/store-lifecycle.test.ts` * `test/commands/artifact-workflow.test.ts` * `test/commands/change-initiative-link.test.ts` * `test/commands/declared-store-fallback.test.ts` * `test/commands/legacy-groups-removed.test.ts` * `test/commands/show.test.ts` * `test/commands/spec.test.ts` * `test/commands/store-git.test.ts` * `test/commands/store-references.test.ts` * `test/commands/store-remote.test.ts` * `test/commands/store-root-selection.test.ts` * `test/commands/store.test.ts` * `test/commands/validate.test.ts` * `test/commands/workflow/new-change-domain.test.ts` * `test/commands/workflow/shared.test.ts` * `test/core/archive.test.ts` * `test/core/commands/change-command.list.test.ts` * `test/core/commands/change-command.show-validate.test.ts` * `test/core/completions/command-registry.test.ts` * `test/core/completions/completion-provider.test.ts` * `test/core/init.test.ts` * `test/core/list.test.ts` * `test/core/openspec-root.test.ts` * `test/core/relationship-health.test.ts` * `test/core/root-selection.test.ts` * `test/core/specs-apply.test.ts` * `test/core/templates/skill-templates-parity.test.ts` * `test/core/view.test.ts` * `test/core/working-set.test.ts` * `test/helpers/openspec-fixtures.ts` * `test/helpers/store-git.ts` * `test/utils/change-path.test.ts` * `test/utils/change-utils.test.ts` * `test/utils/item-discovery.test.ts` * `test/vocabulary-sweep.test.ts` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| 1. **Merge deltas.** Each delta spec section (ADDED/MODIFIED/REMOVED) is applied to the corresponding main spec. | ||
|
|
||
| 2. **Move to archive.** The change folder moves to `changes/archive/` with a date prefix for chronological ordering. | ||
| 2. **Move to archive.** The change folder moves to the sibling `archive/` tree with a date prefix for chronological ordering and its domain path preserved. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Update the adjacent archive diagram to the sibling layout.
Lines 527-539 still show openspec/changes/archive, contradicting this revised process and the PR’s new openspec/archive structure.
🤖 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 `@docs/concepts.md` at line 546, Update the archive diagram adjacent to the
“Move to archive” step in docs/concepts.md to show the sibling openspec/archive
layout instead of openspec/changes/archive, while preserving the date prefix and
domain path structure.
| auth/oauth/add-login # id with domain = auth/oauth | ||
| ``` | ||
|
|
||
| **Naming constraints:** Every segment (domain segments and change name) must satisfy the existing kebab-case validation rule. No two changes may share the same id within a planning home. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the domain naming constraints to match the implementation.
Domains are described as kebab-case here, but this PR intentionally accepts case-preserving segments such as Platform/API.v2/team_alpha. Reserve kebab-case for the final change name and document the actual domain character rules.
Also applies to: 63-74, 163-166
🤖 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 `@docs/superpowers/specs/2026-06-03-domain-archive-restructure-design.md` at
line 42, Update the naming constraints and related domain documentation to
reflect the implementation: domain segments must allow case-preserving names and
characters such as those in Platform/API.v2/team_alpha, while only the final
change name remains subject to kebab-case validation. Preserve the existing
uniqueness requirement for change IDs within a planning home and update the
referenced sections consistently.
| if (!existsSync(SPECS_DIR)) { | ||
| console.log('No items found'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix JSON contract break when specs directory is missing.
If --json is requested and the specs directory doesn't exist, the command currently outputs plain text (No items found), which breaks the JSON output contract.
Since getSpecIds() already gracefully handles missing directories and returns an empty array, you can safely remove this early return. This ensures that [] is correctly output in JSON mode, while text mode gracefully falls through to the existing No items found check on line 192.
💚 Proposed fix
- if (!existsSync(SPECS_DIR)) {
- console.log('No items found');
- return;
- }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!existsSync(SPECS_DIR)) { | |
| console.log('No items found'); | |
| return; | |
| } |
🤖 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/spec.ts` around lines 160 - 163, Remove the
existsSync(SPECS_DIR) early-return block in the command flow so missing
directories proceed through the normal spec ID handling. Reuse getSpecIds()’s
empty-array result, allowing JSON mode to emit [] and text mode to reach the
existing No items found check.
| let domain: string; | ||
| if (options.domain !== undefined) { | ||
| domain = validateExplicitDomain(options.domain); | ||
| } else if (!options.json && isInteractive()) { | ||
| domain = await promptForDomain(root.changesDir); | ||
| } else { | ||
| throw await domainRequiredError(root.changesDir); | ||
| } | ||
| const changeId = domain ? `${domain}/${name}` : name; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Prevent case-folding aliases in domain paths.
Domain casing is preserved, but no check prevents distinct Linux directories such as Platform and platform, which collapse on case-insensitive filesystems.
src/commands/workflow/new-change.ts#L262-L270: compare every requested segment against existing siblings case-insensitively and reject conflicting spelling or use the existing canonical spelling.test/commands/workflow/new-change-domain.test.ts#L104-L115: add an alias-path regression covering an existingPlatformdomain followed by aplatformrequest.
As per coding guidelines, “Add an alias-path regression test when touching path identity logic.”
📍 Affects 2 files
src/commands/workflow/new-change.ts#L262-L270(this comment)test/commands/workflow/new-change-domain.test.ts#L104-L115
🤖 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/new-change.ts` around lines 262 - 270, Update the
domain resolution flow around validateExplicitDomain, promptForDomain, and
changeId to compare each requested path segment with existing sibling
directories case-insensitively, rejecting conflicting spellings or reusing the
existing canonical spelling. Add the required alias-path regression in
test/commands/workflow/new-change-domain.test.ts lines 104-115, covering an
existing Platform domain followed by a platform request.
Source: Coding guidelines
| // Block before validation or spec preparation can mutate any target. | ||
| if (await pathExistsWithoutFollowingLinks(archivePath)) { | ||
| throw new ArchiveBlockedError( | ||
| 'archive_target_exists', | ||
| `Archive '${archivedAs}' already exists.` | ||
| ); | ||
| } | ||
| await assertProspectivePathContained(archiveDir, archivePath, 'Archive'); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C3 '\barchiveDir\b|mkdir.*archive' src/core src/commandsRepository: Fission-AI/OpenSpec
Length of output: 3997
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== root-selection ==\n'
sed -n '1,180p' src/core/root-selection.ts
printf '\n== archive helper ==\n'
rg -n -C4 'assertProspectivePathContained|pathExistsWithoutFollowingLinks|buildArchivePath' src/core/archive.ts src/core -g '!**/dist/**'
printf '\n== archive.ts around relevant lines ==\n'
sed -n '210,260p' src/core/archive.ts
printf '\n== archive.ts later lines ==\n'
sed -n '480,510p' src/core/archive.tsRepository: Fission-AI/OpenSpec
Length of output: 13176
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== containment helper definitions ==\n'
rg -n -C6 'function assertProspectivePathContained|const assertProspectivePathContained|export .*assertProspectivePathContained|pathExistsWithoutFollowingLinks|realpath\(' src/core
printf '\n== files mentioning archiveDir creation ==\n'
rg -n -C3 'archiveDir|openspec/archive|mkdir\(.*recursive: true\)' src -g '!**/dist/**'Repository: Fission-AI/OpenSpec
Length of output: 13822
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,180p' src/core/root-selection.tsRepository: Fission-AI/OpenSpec
Length of output: 5350
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== change-path helper ==\n'
sed -n '180,280p' src/utils/change-path.ts
printf '\n== archive command creation paths ==\n'
rg -n -C4 'openspec/archive|archiveDir|mkdir\(.*recursive: true\)|ensure.*archive|create.*archive' src -g '!**/dist/**'
printf '\n== open spec root/setup creation paths ==\n'
rg -n -C4 'openspec/(changes|specs|archive)|mkdir\(.*recursive: true\)' src/core src/commands src/utils -g '!**/dist/**'Repository: Fission-AI/OpenSpec
Length of output: 45766
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
const fs = require('fs');
const path = require('path');
const file = 'src/utils/change-path.ts';
const src = fs.readFileSync(file, 'utf8');
const start = src.indexOf('export async function pathExistsWithoutFollowingLinks');
const end = src.indexOf('export async function assertProspectivePathContained');
console.log('== pathExistsWithoutFollowingLinks ==');
console.log(src.slice(start, end));
const start2 = src.indexOf('export async function assertProspectivePathContained');
console.log('== assertProspectivePathContained ==');
console.log(src.slice(start2, src.indexOf('export function splitChangeId')));
JSRepository: Fission-AI/OpenSpec
Length of output: 501
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C3 'assertProspectivePathContained|pathExistsWithoutFollowingLinks|realpath|lstat|stat' src/utils/change-path.tsRepository: Fission-AI/OpenSpec
Length of output: 2416
Create archiveDir before containment checks. assertProspectivePathContained() resolves the archive root itself, so upgraded repos that still lack openspec/archive fail here before the recursive mkdir at line 495 can create it. Initialize or migrate the sibling directory first, then validate the final target.
🤖 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/archive.ts` around lines 231 - 238, Update the archive flow
surrounding pathExistsWithoutFollowingLinks and assertProspectivePathContained
to initialize or migrate archiveDir before validating archivePath containment.
Ensure the sibling directory exists before assertProspectivePathContained
resolves the archive root, while preserving the existing target-exists
protection and later archive creation behavior.
| **Prompt options:** | ||
| - If changes needed: "Sync now (recommended)", "Archive without syncing" | ||
| - If already synced: "Archive now", "Sync anyway", "Cancel" | ||
|
|
||
| If user chooses sync, use Task tool (subagent_type: "general-purpose", prompt: "Use Skill tool to invoke openspec-sync-specs for change '<name>'. Delta spec analysis: <include the analyzed delta spec summary>"). Proceed to archive regardless of choice. | ||
| If user chooses sync, use Task tool (subagent_type: "general-purpose", prompt: "Use Skill tool to invoke openspec-sync-specs for change '<change-id>'. Delta spec analysis: <include the analyzed delta spec summary>"). Proceed to archive regardless of choice. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not archive when the user selects Cancel.
Both variants offer Cancel and then instruct the agent to proceed regardless of the choice.
src/core/templates/workflows/archive-change.ts#L86-L90: return immediately on Cancel.src/core/templates/workflows/archive-change.ts#L194-L198: apply the same branch in the OPSX template.
📍 Affects 1 file
src/core/templates/workflows/archive-change.ts#L86-L90(this comment)src/core/templates/workflows/archive-change.ts#L194-L198
🤖 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/archive-change.ts` around lines 86 - 90, The
archive workflow must stop when the user selects Cancel instead of proceeding
unconditionally. Update both Cancel-handling branches in
src/core/templates/workflows/archive-change.ts at lines 86-90 and 194-198 to
return immediately before archive execution; apply the same behavior in both
template variants.
| async function hasOwnChangeMarker(changePath: string): Promise<boolean> { | ||
| for (const marker of ['.openspec.yaml', 'proposal.md']) { | ||
| try { | ||
| const stats = await fs.stat(path.join(changePath, marker)); | ||
| if (stats.isFile()) { | ||
| return true; | ||
| } | ||
| } catch (error) { | ||
| if (!isMissingPathError(error)) { | ||
| throw error; | ||
| } | ||
| } | ||
| } | ||
| return false; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
printf '\n== file outline ==\n'
ast-grep outline src/utils/change-path.ts --view expanded || true
printf '\n== relevant lines ==\n'
sed -n '300,390p' src/utils/change-path.ts
printf '\n== search for marker discovery ==\n'
rg -n "openspec\.yaml|proposal\.md|isSymbolicLink|lstat\(|stat\(" src/utils/change-path.ts src -g '!**/node_modules/**' || trueRepository: Fission-AI/OpenSpec
Length of output: 10135
🏁 Script executed:
set -euo pipefail
sed -n '300,390p' src/utils/change-path.tsRepository: Fission-AI/OpenSpec
Length of output: 2049
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path('src/utils/change-path.ts')
print(p.exists())
print('\n'.join(f"{i+1}: {line}" for i, line in enumerate(p.read_text().splitlines()[300:390])))
PYRepository: Fission-AI/OpenSpec
Length of output: 2292
🏁 Script executed:
set -euo pipefail
sed -n '120,190p' src/utils/change-path.tsRepository: Fission-AI/OpenSpec
Length of output: 2095
🏁 Script executed:
set -euo pipefail
rg -n "proposal\.md|\.openspec\.yaml" src/utils src/core src/commands -g '!**/node_modules/**' | sed -n '1,220p'Repository: Fission-AI/OpenSpec
Length of output: 4155
🏁 Script executed:
set -euo pipefail
sed -n '1,140p' src/utils/change-metadata.ts
printf '\n==\n'
sed -n '1,120p' src/utils/change-utils.tsRepository: Fission-AI/OpenSpec
Length of output: 8099
Reject symlinked change markers. fs.stat() follows .openspec.yaml and proposal.md links, so a symlink can make this resolver accept a change that discovery skips and can point metadata/proposal reads outside changesDir. Use lstat() and require a regular file.
🤖 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/utils/change-path.ts` around lines 340 - 353, Update hasOwnChangeMarker
to use fs.lstat instead of fs.stat for .openspec.yaml and proposal.md, and
require the result to be a regular non-symlink file before returning true.
Preserve the existing missing-path handling and error propagation.
| async function canonicalizeProspectivePath(targetPath: string): Promise<string> { | ||
| const missingSegments: string[] = []; | ||
| let currentPath = path.resolve(targetPath); | ||
|
|
||
| while (true) { | ||
| try { | ||
| const existingPath = await fs.realpath(currentPath); | ||
| return path.resolve(existingPath, ...missingSegments.reverse()); | ||
| } catch (error) { | ||
| if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { | ||
| throw error; | ||
| } | ||
|
|
||
| const parentPath = path.dirname(currentPath); | ||
| if (parentPath === currentPath) { | ||
| throw error; | ||
| } | ||
| missingSegments.push(path.basename(currentPath)); | ||
| currentPath = parentPath; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| function pathStaysWithin(basePath: string, targetPath: string): boolean { | ||
| const relativePath = path.relative(basePath, targetPath); | ||
| return ( | ||
| relativePath === '' || | ||
| (!path.isAbsolute(relativePath) && | ||
| relativePath !== '..' && | ||
| !relativePath.startsWith(`..${path.sep}`)) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Use the shared prospective-path containment helper.
This duplicates assertProspectivePathContained() from change-path.ts, but with different dangling-link and ancestor-type behavior. Route createChange() through the shared implementation so all write paths enforce one security contract.
Proposed direction
import {
+ assertProspectivePathContained,
splitChangeId,
validateChangeName,
validateDomainPath,
} from './change-path.js';
- const canonicalChangesDir = await canonicalizeProspectivePath(changesDir);
- const canonicalChangeDir = await canonicalizeProspectivePath(changeDir);
- if (!pathStaysWithin(canonicalChangesDir, canonicalChangeDir)) {
- throw new Error('Change path must stay within changesDir');
- }
+ await assertProspectivePathContained(changesDir, changeDir, 'Change path');Also applies to: 166-180
🤖 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/utils/change-utils.ts` around lines 16 - 47, The local
canonicalizeProspectivePath and pathStaysWithin logic duplicates the shared
containment contract and may handle dangling links or ancestor types
differently. Remove or stop using these helpers, and update createChange() to
route prospective-path validation through assertProspectivePathContained() from
change-path.ts for all write paths.
| it('should reject a link ancestor that escapes the changes directory', async (context) => { | ||
| const changesDir = path.join(testDir, 'selected-root', 'changes'); | ||
| const outsideDir = path.join(testDir, 'outside'); | ||
| const linkedDomain = path.join(changesDir, 'Linked'); | ||
| await fs.mkdir(changesDir, { recursive: true }); | ||
| await fs.mkdir(outsideDir, { recursive: true }); | ||
|
|
||
| try { | ||
| await fs.symlink( | ||
| outsideDir, | ||
| linkedDomain, | ||
| process.platform === 'win32' ? 'junction' : 'dir' | ||
| ); | ||
| } catch (error) { | ||
| if (isLinkCreationUnavailable(error)) { | ||
| context.skip(); | ||
| return; | ||
| } | ||
| throw error; | ||
| } | ||
|
|
||
| await expect( | ||
| createChange(testDir, 'Linked/add-auth', { changesDir }) | ||
| ).rejects.toThrow(/stay within changesDir/); | ||
| await expect(fs.access(path.join(outsideDir, 'add-auth'))).rejects.toThrow(); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add a positive in-root alias-path regression test.
The escape case is covered, but there is no test proving that a symlink/junction resolving within changesDir remains valid. Also assert canonical identity separately from preserved path spelling.
As per coding guidelines, “Add an alias-path regression test when touching path identity logic.” <coding_guidelines>
pnpm exec vitest run test/utils/change-utils.test.ts -t "alias"🤖 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 `@test/utils/change-utils.test.ts` around lines 249 - 274, Add a positive
alias-path regression test alongside the existing escape test, creating a
symlink/junction inside changesDir that resolves to another directory also
within changesDir and using it with createChange. Assert the operation succeeds,
the created path preserves the alias spelling, and separately verify its
canonical realpath matches the target directory identity; keep the test
compatible with platforms where link creation is unavailable by using the
existing skip handling.
Source: Coding guidelines
| if ( | ||
| /--change (?:"<name>"|<name>)|(?:\/opsx:(?:apply|archive|continue|verify)|openspec archive) \[?change-name/u.test(line) | ||
| ) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Enforce change-id consistently across all lifecycle documentation.
The vocabulary sweep omits /opsx:update, while command sections mix [change-id] syntax with change-name argument declarations.
test/vocabulary-sweep.test.ts#L77-L79: include/opsx:updateand cover stale lifecycle argument terminology.docs/commands.md#L183-L183: rename the/opsx:continueargument row tochange-id.docs/commands.md#L277-L277: rename the/opsx:applyargument row and related tip tochange-id.docs/commands.md#L339-L339: change/opsx:updatesyntax and its argument row tochange-id.docs/commands.md#L376-L376: rename the/opsx:verifyargument row tochange-id.docs/commands.md#L501-L501: rename the/opsx:archiveargument row tochange-id.docs/commands.md#L719-L719: align the surrounding troubleshooting terminology withchange-id.
📍 Affects 2 files
test/vocabulary-sweep.test.ts#L77-L79(this comment)docs/commands.md#L183-L183docs/commands.md#L277-L277docs/commands.md#L339-L339docs/commands.md#L376-L376docs/commands.md#L501-L501docs/commands.md#L719-L719
🤖 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 `@test/vocabulary-sweep.test.ts` around lines 77 - 79, Standardize lifecycle
documentation on the `change-id` terminology: update the vocabulary sweep regex
in `test/vocabulary-sweep.test.ts` to include `/opsx:update` and detect stale
lifecycle argument names; in `docs/commands.md` at lines 183, 277, 339, 376,
501, and 719, rename the affected argument rows, tips, syntax, and
troubleshooting references from `change-name` to `change-id`, including
`/opsx:update` syntax.
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Reviewed the domain/archive PR with focus on path resolution, archive moves, recursive discovery, store-root routing, generated workflow guidance, and traversal/symlink containment. I do not have blocking findings from that pass.\n\nVerification I ran in a temp checkout:\n\nbash\nnpm install\nnpm test -- --run test/utils/change-path.test.ts test/utils/change-utils.test.ts test/utils/item-discovery.test.ts test/core/archive.test.ts test/core/specs-apply.test.ts test/commands/workflow/new-change-domain.test.ts test/commands/show.test.ts test/commands/validate.test.ts\n\n\nResult: build passed during install; focused suite passed, 8 files / 146 tests. I did not rerun the full suite.
2 participants
Summary
openspec/archiveroot while preserving domain hierarchyVerification
node build.jsvitest run: 105 test files, 1961 passed, 22 skippednpm.cmd run lint: 0 errors (one pre-existing warning insrc/core/references.ts)Summary by CodeRabbit
New Features
--domainsupport, including nested domains and root changes.openspec/archive/, preserving domain structure and date-based naming.Bug Fixes
Documentation