fix(change): resolve changes by directory instead of requiring proposal.md - #1433
Conversation
`openspec show <change>` and shell completion resolved a change only when `openspec/changes/<name>/proposal.md` existed. Every sibling command -- `list`, `status`, `instructions`, `validate` -- resolves a change by its directory (`getAvailableChanges`). The two rules disagree the moment a change is created: `openspec new change <name>` scaffolds only `.openspec.yaml`, so `list` showed the change while `show` reported `Unknown item`. A custom schema that defines no proposal artifact was never resolvable at all (#1161). Resolve by directory in `getActiveChangeIds`/`getArchivedChangeIds`, and report a change that exists without a proposal accurately -- pointing at `openspec status --change <name>` -- rather than as missing. The deprecated `openspec change list` keeps its own proposal-backed scan; its JSON output parses proposal.md per change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughChange discovery now resolves active and archived changes by directory rather than requiring ChangesProposal-less change resolution
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 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 `@src/commands/change.ts`:
- Around line 62-80: Update the no-name discovery path in the show command to
enumerate change directories directly, so scaffolded changes without proposal.md
appear in interactive selections and available-ID errors. Keep the deprecated
list command’s proposal-gated getActiveChanges() behavior unchanged, and add a
regression test covering no-name show discovery of a proposal-less change.
🪄 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 Plus
Run ID: 6d05d12f-08e7-48cf-94d4-8af53234e0ff
📒 Files selected for processing (5)
.changeset/show-resolves-proposalless-changes.mdsrc/commands/change.tssrc/utils/item-discovery.tstest/commands/show.test.tstest/utils/item-discovery.test.ts
`ChangeCommand.show` resolved a named proposal-less change but still built its no-name selector (and the non-interactive "Available IDs" hint) from the proposal-gated scan, so a scaffolded change could not be picked. Use directory-based discovery there as well. `list` keeps the local proposal-backed scan: its --json output parses proposal.md per change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed the review finding on The no-name selector in
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/commands/change.ts (1)
68-85: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve non-missing filesystem errors.
This catch treats permission errors and malformed paths as “no proposal.md yet.” It also considers any existing path a change directory, so a regular file named like the change receives the wrong guidance. Only handle
ENOENTas a missing artifact, and verifychangeDiris a directory before emitting the proposal-less message; rethrow other filesystem errors.🤖 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 68 - 85, The proposalPath error handling must only interpret ENOENT as a missing proposal. Update the catch around fs.access in the change command to rethrow other filesystem errors, then verify changeDir is an actual directory (not merely an existing path) before reporting a proposal-less change; otherwise preserve the not-found error.
🤖 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.
Outside diff comments:
In `@src/commands/change.ts`:
- Around line 68-85: The proposalPath error handling must only interpret ENOENT
as a missing proposal. Update the catch around fs.access in the change command
to rethrow other filesystem errors, then verify changeDir is an actual directory
(not merely an existing path) before reporting a proposal-less change; otherwise
preserve the not-found error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 753740e7-32c2-411e-bfec-42820744b034
📒 Files selected for processing (2)
src/commands/change.tstest/commands/show.test.ts
…honestly Adversarial review of the first two commits surfaced four defects. `change list` still used a private proposal-gated scan, so the deprecated alias reported a different set than `openspec list` -- the command its own deprecation warning tells you to use -- while the `show` selector beside it offered the wider set. Move it to `getActiveChangeIds` and drop the now unused helper and its ARCHIVE_DIR constant. Widening that list exposed three follow-on bugs, all fixed here: - Task counts were computed inside the proposal try block, so a change with tasks but no proposal.md reported 0/0. Task progress is independent of the proposal; resolve it first. - `--long` printed "(unable to read)" and `--json` "Unknown" for a change that is simply not written yet. Distinguish a missing proposal from an unreadable one by testing existence, not by sniffing error codes. - `show` reported a stray file under changes/, or a traversing name such as `../..`, as a change awaiting its proposal, pointing the user at a `status --change` call that cannot work. Require a directory that is a direct child of changes/. Also drops a duplicated proposal.md read in the --long path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
clay-good
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/commands/change.ts (1)
67-86: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReject non-child change paths before proposal access. The current validation occurs only in the failed-access path, so traversal can read an existing external
proposal.md.
src/commands/change.ts#L67-L86: validate the resolved direct child withlstat().isDirectory()before accessingproposal.md; reject traversal and symlink aliases.test/core/commands/change-command.show-validate.test.ts#L109-L112: seed the traversed target withtempRoot/proposal.md, usepath.join('..', '..'), and add an alias-path regression case.🤖 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 67 - 86, In src/commands/change.ts lines 67-86, update the change validation before proposalPath access to resolve the candidate, verify it is a direct child of changesPath, and use lstat().isDirectory() so traversal and symlink aliases are rejected before fs.access(proposalPath); preserve the existing missing-proposal handling for valid change directories. In test/core/commands/change-command.show-validate.test.ts lines 109-112, seed tempRoot/proposal.md, use path.join('..', '..') for the traversal case, and add regression coverage for an alias path.Source: Coding guidelines
🤖 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 `@src/commands/change.ts`:
- Around line 13-17: Update pathExists to return false only when fs.access fails
with an ENOENT error; rethrow or propagate EACCES and other I/O failures so the
existing read-error handling reports an unknown/unreadable proposal instead of
treating it as missing.
In `@test/core/commands/change-command.show-validate.test.ts`:
- Around line 109-112: Update the test around cmd.show in “does not treat a
traversing name as a change” to create tempRoot/proposal.md before asserting
rejection, and construct the traversal input with path.join('..', '..') rather
than a hard-coded separator. Add a separate symlink-alias regression case that
targets the reachable proposal path and verifies traversal aliases are not
accepted, covering Windows path identity behavior.
---
Outside diff comments:
In `@src/commands/change.ts`:
- Around line 67-86: In src/commands/change.ts lines 67-86, update the change
validation before proposalPath access to resolve the candidate, verify it is a
direct child of changesPath, and use lstat().isDirectory() so traversal and
symlink aliases are rejected before fs.access(proposalPath); preserve the
existing missing-proposal handling for valid change directories. In
test/core/commands/change-command.show-validate.test.ts lines 109-112, seed
tempRoot/proposal.md, use path.join('..', '..') for the traversal case, and add
regression coverage for an alias path.
🪄 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 Plus
Run ID: cb3f0d44-619b-4ddb-994c-85504be18e62
📒 Files selected for processing (4)
.changeset/show-resolves-proposalless-changes.mdsrc/commands/change.tstest/core/commands/change-command.list.test.tstest/core/commands/change-command.show-validate.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .changeset/show-resolves-proposalless-changes.md
| it('does not treat a traversing name as a change', async () => { | ||
| await expect(cmd.show('../..', { json: false })).rejects.toThrow(/not found at/); | ||
| await expect(cmd.show('../..', { json: false })).rejects.not.toThrow(/has no proposal\.md yet/); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Exercise the reachable traversal target.
Create tempRoot/proposal.md before this assertion; the current code then incorrectly accepts ../... Build the traversal input with path.join('..', '..'), and add a symlink-alias case to cover path identity on Windows. As per coding guidelines, “When touching path behavior, add coverage that would fail on Windows path separators” and “Add an alias-path regression test when touching path identity logic.”
🤖 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/core/commands/change-command.show-validate.test.ts` around lines 109 -
112, Update the test around cmd.show in “does not treat a traversing name as a
change” to create tempRoot/proposal.md before asserting rejection, and construct
the traversal input with path.join('..', '..') rather than a hard-coded
separator. Add a separate symlink-alias regression case that targets the
reachable proposal path and verifies traversal aliases are not accepted,
covering Windows path identity behavior.
Source: Coding guidelines
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Exact head e48e4fd cleanly unifies change discovery by directory across show, deprecated noun-form list/show/validate, and active/archived completion while preserving archive/hidden exclusions, direct-child containment, task counts, and structured JSON errors. Build, 63 focused discovery/show/list/completion tests, a real proposal-less CLI reproduction, and the full hosted Linux/macOS/Windows/security matrix pass; approved technically, with a real second-human approval still required before merge.
…oposals Second review round. `isDefinitelyMissing` replaces the plain existence check: fs.access can fail with EACCES or an I/O error, and treating that as "no proposal.md yet" hid a real read failure behind an ordinary-looking state. Only ENOENT counts as absent; anything else falls through to the existing unreadable handling. `show` now rejects a name that is not a direct child of changes/ before touching the filesystem. This closes a pre-existing traversal on main: `openspec change show ../..` resolved openspec/changes/../../proposal.md and printed a file from outside the changes directory. Both new tests fail without the guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Both findings from the second review round are addressed in 66cd50c, and the traversal one turned out to be a genuine (pre-existing) bug. 1. "Do not classify inaccessible proposals as missing" — valid, fixed. 2. "Exercise the reachable traversal target" — valid, and it exposed a real traversal. I verified this against Tests now use I skipped the suggested symlink-alias case: symlink creation is unreliable on Windows CI without elevation, and containment here is decided by Full suite: 2234 tests passing; |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/commands/change.ts (1)
88-100: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve unreadable-proposal errors.
Line 90 catches
EACCESand I/O failures, then Lines 97-105 report “has no proposal.md yet” whenever the directory is readable. OnlyENOENTshould trigger that message; propagate other access errors after retaining the stray-file directory check.Suggested fix
- } catch { + } catch (error) { // A change can exist without a proposal: `openspec new change` scaffolds // only .openspec.yaml, and a custom schema need not define a proposal // artifact. Say which of the two cases this is instead of reporting a @@ if (isChangeDirectory) { + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { + throw error; + } throw new Error( `Change "${changeName}" has no proposal.md yet. ` + `Run "openspec status --change ${changeName}" to see which artifact comes next.`🤖 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 88 - 100, Update the proposalPath access handling in the change command to treat only an ENOENT error as a missing proposal; preserve the existing isChangeDirectory stray-file check for that case. For EACCES and other filesystem failures, rethrow the original error instead of reporting that proposal.md is absent.
🤖 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.
Duplicate comments:
In `@src/commands/change.ts`:
- Around line 88-100: Update the proposalPath access handling in the change
command to treat only an ENOENT error as a missing proposal; preserve the
existing isChangeDirectory stray-file check for that case. For EACCES and other
filesystem failures, rethrow the original error instead of reporting that
proposal.md is absent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 470ce446-d750-4077-821d-e99c211cb33d
📒 Files selected for processing (2)
src/commands/change.tstest/core/commands/change-command.show-validate.test.ts
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Follow-up exact head 66cd50c closes the remaining safety gaps: change lookup now rejects traversal and nested names before any proposal read, and only labels a proposal missing on ENOENT, preserving unreadable-file reporting. Build, 64 focused discovery/show/list/completion tests including a real outside-proposal traversal regression, a structured JSON CLI reproduction, and the full hosted Linux/macOS/Windows/security matrix pass; approved technically, with a real second-human approval still required before merge.
2 participants
Status
LGTM. No behavior change for a change that already has a
proposal.md; the full suite (2234 tests) passes on Linux, macOS, and Windows.What was wrong
Change lookup used two different rules.
openspec list,status,instructions, andvalidateresolve a change by its directory.openspec show,openspec change list, and shell completion instead requiredopenspec/changes/<name>/proposal.md.Those rules disagree the moment you create a change, because
openspec new changewrites only.openspec.yaml:So
openspec listhands you a name thatopenspec showdenies. Tab-completion had the same blind spot.openspec change list— whose deprecation warning says "Useopenspec list" — reported a different set thanopenspec list. And for a custom schema that defines noproposalartifact, the change was never resolvable at all: the first defect listed in #1161.How it was fixed
getActiveChangeIds/getArchivedChangeIdsresolve a change by its directory, the rulegetAvailableChangesalready applies. Archive and hidden directories are still excluded.openspec change listmoves onto the same helper, so the deprecated alias now matches the command it points at.Reporting is honest about what is actually on disk:
This finishes a direction the repo already took: #1182 moved
validateoff theproposal.mdgate onto directory resolution, and #1375 relaxed--changelookup to accept any name that exists on disk. No schema, workflow, or artifact-graph behavior changes.Replication / proof
Reproduce on
main:After this branch both commands agree and
showexplains what is missing.Tests:
test/utils/item-discovery.test.ts(new) — scaffolded change, proposal-less schema, archive/hidden exclusion, missing directory.test/commands/show.test.ts— end-to-end:showresolves a scaffolded change;change showwith no name offers it.test/core/commands/change-command.list.test.ts—change listlists it, keeps[tasks 1/2], and labels it(no proposal.md yet)rather than(unable to read).test/core/commands/change-command.show-validate.test.ts— a stray file, a traversing name, and a nested name are not reported as changes; the traversal case writes the reachable target file so it fails without the containment guard, and builds the input withpath.join('..', '..')for Windows separators.eslint src/clean. No template changes, so the golden parity hashes are untouched.Review findings addressed
Three adversarial passes over the first two commits (blast radius, edge cases, consumer contracts) found four defects, all fixed in
e48e4fd:change listkept a private proposal-gated scan, disagreeing withopenspec listand with theshowselector directly above it. Unified; the orphaned helper and itsARCHIVE_DIRconstant are gone.tryblock, so a change with tasks but no proposal reported0/0.--long/--jsonlabelled a not-yet-written proposal(unable to read)/Unknown. Missing vs. unreadable is now decided by testing existence rather than sniffing error codes.showreported a stray file underchanges/as a change awaiting its proposal — pointing at astatus --changecall that cannot work. It now requires a directory.fs.accessfailing withEACCESwas read as "no proposal.md yet", hiding a real read failure. OnlyENOENTnow counts as absent.openspec change show ../..resolvedopenspec/changes/../../proposal.mdand printed a file from outside the changes directory. Verified reproducing identically onorigin/main, so it predates this PR; since this PR already added containment for the error message,shownow rejects any name that is not a direct child ofchanges/before touching the filesystem. Both new tests fail with the guard removed.Verified unchanged:
getActiveChangeIds/getArchivedChangeIdsare not part of the package's public export surface (src/index.tsexportscliandcoreonly);ChangeCommand.validateneeds only the change directory, so widening its selector cannot crash; every store root deriveschangesDiras<root>/openspec/changes(makeRoot), sogetActiveChangeIds(root.path)stays correct under--store; and an unreadablechanges/directory still degrades to an empty list.Notes / nits
changes/shares a name with a spec,openspec show <name>now reports the existing "Ambiguous item — pass--type change|spec" error even before the change has a proposal. That is the correct answer once the change is real tolist/status, and it was already the behavior the moment a proposal existed.show's not-found path can suggest it twice (Did you mean: auth, auth?). It is in the suggestion logic, unrelated to discovery.🤖 Generated with Claude Code
Summary by CodeRabbit
openspec show <change>now resolves changes by directory name (including scaffolded/proposal-less changes withoutproposal.md).(no proposal.md yet)and guidance toopenspec status --change <name>, avoids “Unknown item”, and keeps task counts accurate.