openspec show --diff feature#980
Conversation
…during implementation.
📝 WalkthroughWalkthroughAdds a new Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as "CLI Parser"
participant ShowCmd as "ShowCommand"
participant ChangeCmd as "ChangeCommand"
participant Utils as "requirement-diff Utils"
participant FSys as "File System"
User->>CLI: openspec show mychange --diff
CLI->>ShowCmd: execute(item, {diff: true})
ShowCmd->>ChangeCmd: invoke show for change (diff flag)
ChangeCmd->>FSys: read delta spec files
FSys-->>ChangeCmd: return delta contents
ChangeCmd->>Utils: parse delta, buildRenameMap
loop per-capability / per-delta
ChangeCmd->>Utils: extractRequirementBlock(deltaContent, name)
Utils-->>ChangeCmd: delta block
ChangeCmd->>FSys: read base spec file
FSys-->>ChangeCmd: base spec content
ChangeCmd->>Utils: extractRequirementBlock(baseContent, lookupName)
Utils-->>ChangeCmd: base block or null
ChangeCmd->>Utils: diffRequirementBlock(baseBlock, deltaBlock, label)
Utils-->>ChangeCmd: unified diff text
end
alt --json
ChangeCmd->>User: JSON output with diff fields for MODIFIED deltas
else text
ChangeCmd->>User: colorized unified diffs and labeled ADDED/REMOVED/RENAMED
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openspec/changes/spec-diffs/proposal.md`:
- Around line 21-25: Update the proposal to precisely document the JSON contract
for the --diff flag: state that in JSON mode (used by openspec show / openspec
change show with --json --diff) only deltas with type MODIFIED include a "diff"
field containing the unified-diff text; ADDED, REMOVED, and RENAMED deltas do
not include "diff" (they may omit the field or include null), and cases with no
base spec return a "warning" field instead of "diff"; also document that text
mode still shows full content as all-additions for new specs and colorized
unified diffs for modified deltas, and that when --diff is used without --json
the CLI prints only diffs (no proposal text).
In `@openspec/changes/spec-diffs/specs/cli-show/spec.md`:
- Around line 44-46: The spec currently requires printing the proposal markdown
before diffs for the command "openspec show <change-name> --diff" which is
incorrect; update the text in the spec (the WHEN/THEN lines that reference
"openspec show <change-name> --diff") so that WHEN running the CLI with --diff
in text mode the output is diff-only (do not print the proposal markdown first)
and THEN describe that the CLI should iterate parsed deltas grouped by
capability for each delta spec file under
openspec/changes/<change-name>/specs/<cap>/spec.md and render only the diffs in
text form.
In `@src/commands/change.ts`:
- Around line 267-268: The loop that builds REMOVED result entries currently
uses only plan.removed (a list of names) and synthesizes raw as `###
Requirement: ${name}`, losing the original removed body. Fix by attaching the
actual removed delta body to the result.raw instead of the synthesized header:
change plan.removed to carry the full removed entry (e.g., objects like { name,
raw }) or lookup the removed text from whatever store holds original deltas
(e.g., plan.removedDeltas / removedMap) and set results.push({ capability:
capName, operation: 'REMOVED', requirementName: name, raw: originalRaw }). If
plan.removed currently only contains names, update the planner that produces
plan.removed to include bodies so the change.ts loop can use the original
removed content.
- Around line 104-112: The code always reads and prints proposal.md
(proposalPath) before choosing diff vs content, causing --diff to still show the
proposal; move the read+console.log into the non-diff branch or guard it with if
(!options?.diff) so that when options?.diff is true you only call
this.showSpecDiffs(changeName, changesPath) and do not read/print proposalPath;
keep showSpecContent(changeName, changesPath) as the branch that reads/displays
proposal.md.
- Around line 233-235: The early-return when specDirs.length === 0 currently
silences the command; update that branch in src/commands/change.ts to emit an
explicit success message to the user instead of returning silently—locate the
check for specDirs.length === 0 and replace the plain return with a user-facing
output (e.g., via console.log or the project logger) like "No changes detected"
(or the project’s standard success message) and then return; ensure you use the
same output mechanism other commands use so formatting/exit behavior remains
consistent.
In `@src/utils/requirement-diff.ts`:
- Around line 35-40: The function diffRequirementBlock currently calls .trim()
on the unified hunk output, which strips leading whitespace from the first
context line and makes the hunk incorrect; update the return in
diffRequirementBlock (the expression that takes patch.hunks.flatMap(h =>
h.lines).join('\n')) to use .trimEnd() instead of .trim() so trailing newlines
are removed but leading context spaces are preserved.
In `@test/commands/show-diff.test.ts`:
- Around line 130-209: The tests currently assert that proposal text appears
even when invoking run(... --diff), which contradicts the intended contract; for
each test that calls run('show ... --diff') (e.g., the "text mode: shows
proposal then MODIFIED and ADDED full text" test using run('show auth-update
--type change --diff'), the "text mode: shows warning when MODIFIED has no
matching base" test using run('show no-match --type change --diff'), and the
"text mode: shows only proposal when change has no delta specs" test that calls
run('show empty-change --type change --diff'), remove or change the
expect(output).toContain(...) assertions that check for proposal text (e.g.,
'Improve auth.' and 'Test reason.') so the tests assert only diff output
presence (or absence of spec sections) when --diff is provided; keep proposal
assertions only in tests that call run without --diff (like run('show
auth-update --type change')).
- Around line 100-127: The test helpers run and runWithStderr interpolate
openspecBin into shell command strings which breaks if paths contain spaces or
shell metacharacters; replace execSync calls with execFileSync(process.execPath,
[openspecBin, ...args.split(' ')], ...) to avoid shell parsing (preserve
encoding and env, and for runWithStderr set stdio to ['pipe','pipe','pipe'] and
catch the thrown error to extract stdout/stderr as done currently); update the
function signatures/comments to indicate args is a space-separated string that
will be split into an argv array and ensure process.chdir(originalCwd) remains
in the finally block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2972526d-ce17-4ec5-b106-68b0e47685a1
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
openspec/changes/spec-diffs/.openspec.yamlopenspec/changes/spec-diffs/design.mdopenspec/changes/spec-diffs/proposal.mdopenspec/changes/spec-diffs/specs/cli-show/spec.mdopenspec/changes/spec-diffs/tasks.mdpackage.jsonsrc/cli/index.tssrc/commands/change.tssrc/commands/show.tssrc/utils/requirement-diff.tstest/commands/show-diff.test.tstest/utils/requirement-diff.test.ts
| - **WHEN** executing `openspec show <change-name> --diff` in text mode (no `--json`) | ||
| - **THEN** display the proposal markdown text | ||
| - **AND** for each delta spec file under `openspec/changes/<change-name>/specs/<cap>/spec.md`, iterate parsed deltas grouped by capability |
There was a problem hiding this comment.
The text diff scenario is specifying the wrong output.
This says openspec show <change-name> --diff should still print the proposal markdown first, but the intended behavior for --diff is diff-only text output. If this stays, the spec will lock in the current implementation bug.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~45-~45: Did you mean the formatting language “Markdown” (= proper noun)?
Context: ...-json`) - THEN display the proposal markdown text - AND for each delta spec file...
(MARKDOWN_NNP)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openspec/changes/spec-diffs/specs/cli-show/spec.md` around lines 44 - 46, The
spec currently requires printing the proposal markdown before diffs for the
command "openspec show <change-name> --diff" which is incorrect; update the text
in the spec (the WHEN/THEN lines that reference "openspec show <change-name>
--diff") so that WHEN running the CLI with --diff in text mode the output is
diff-only (do not print the proposal markdown first) and THEN describe that the
CLI should iterate parsed deltas grouped by capability for each delta spec file
under openspec/changes/<change-name>/specs/<cap>/spec.md and render only the
diffs in text form.
There was a problem hiding this comment.
Actually, the spec was correct: I want openspec show --diff to print the proposal and then the diffs of the changed requirements... it will display new requirements completely.
There was a problem hiding this comment.
@bsmedberg-xometry, thanks for the clarification! I was mistaken — the spec is correct as written. The intended behavior for openspec show <change-name> --diff in text mode is to print the proposal markdown first and then render the per-requirement diffs (with new/ADDED requirements shown in full). I'll note this for future reviews.
✏️ Learnings added
Learnt from: bsmedberg-xometry
Repo: Fission-AI/OpenSpec PR: 980
File: openspec/changes/spec-diffs/specs/cli-show/spec.md:44-46
Timestamp: 2026-04-16T19:44:15.889Z
Learning: In the OpenSpec repository (Fission-AI/OpenSpec), `openspec show <change-name> --diff` in text mode intentionally prints the proposal markdown text first, followed by per-requirement unified diffs for each delta spec. ADDED requirements are displayed in full. This is by design — do not flag the proposal-first ordering as a bug.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: lsmonki
Repo: Fission-AI/OpenSpec PR: 660
File: openspec/changes/archive/2026-02-04-hierarchical-specs-support/design.md:96-136
Timestamp: 2026-02-04T17:50:12.189Z
Learning: In OpenSpec, delta specs handle deletions at the requirement level using `## REMOVED Requirements` sections within the delta spec.md file. There is no concept of deleting an entire spec file through deltas.
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:02.839Z
Learning: Always open `@/openspec/AGENTS.md` when the request mentions planning or proposals (words like proposal, spec, change, plan), introduces new capabilities, breaking changes, architecture shifts, or performance/security work, or sounds ambiguous and needs the authoritative spec before coding
Learnt from: HAHH9527
Repo: Fission-AI/OpenSpec PR: 884
File: docs/i18n/zh-cn/opsx.md:29-39
Timestamp: 2026-03-27T07:15:19.513Z
Learning: In the OpenSpec repository (Fission-AI/OpenSpec), Chinese translation documents under `docs/i18n/zh-cn/` are expected to faithfully mirror the structure and formatting of their English source documents. When a markdown issue (e.g., missing fenced code block language identifiers, MD040) exists in a translated file, the root cause is in the original English source. Do not flag formatting issues in translation files if they originate from the source document — suggest fixing the source instead.
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:02.839Z
Learning: Use `@/openspec/AGENTS.md` to learn how to create and apply change proposals, spec format and conventions, and project structure and guidelines
Learnt from: fsilvaortiz
Repo: Fission-AI/OpenSpec PR: 761
File: docs/cli.md:442-445
Timestamp: 2026-02-25T20:02:57.093Z
Learning: In the OpenSpec spec-driven schema, `specs` and `design` artifacts are parallel dependencies that both depend only on `proposal`. This means either can appear first in a topological sort, and both orderings are valid depending on the specific sort algorithm used.
| if (specDirs.length === 0) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Surface the empty diff case to the user.
This returns silently when a change has no delta specs, but the feature contract calls for an explicit success message. Right now the command looks like it did nothing.
Suggested fix
if (specDirs.length === 0) {
+ console.log(`No delta specs found for change "${changeName}"`);
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 (specDirs.length === 0) { | |
| return; | |
| } | |
| if (specDirs.length === 0) { | |
| console.log(`No delta specs found for change "${changeName}"`); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/change.ts` around lines 233 - 235, The early-return when
specDirs.length === 0 currently silences the command; update that branch in
src/commands/change.ts to emit an explicit success message to the user instead
of returning silently—locate the check for specDirs.length === 0 and replace the
plain return with a user-facing output (e.g., via console.log or the project
logger) like "No changes detected" (or the project’s standard success message)
and then return; ensure you use the same output mechanism other commands use so
formatting/exit behavior remains consistent.
| for (const name of plan.removed) { | ||
| results.push({ capability: capName, operation: 'REMOVED', requirementName: name, raw: `### Requirement: ${name}` }); |
There was a problem hiding this comment.
Preserve removed delta bodies instead of synthesizing a header.
Only the requirement name survives here, so the renderer can never show the removal notice/body for REMOVED entries. That drops user-authored context from diff mode.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/change.ts` around lines 267 - 268, The loop that builds REMOVED
result entries currently uses only plan.removed (a list of names) and
synthesizes raw as `### Requirement: ${name}`, losing the original removed body.
Fix by attaching the actual removed delta body to the result.raw instead of the
synthesized header: change plan.removed to carry the full removed entry (e.g.,
objects like { name, raw }) or lookup the removed text from whatever store holds
original deltas (e.g., plan.removedDeltas / removedMap) and set results.push({
capability: capName, operation: 'REMOVED', requirementName: name, raw:
originalRaw }). If plan.removed currently only contains names, update the
planner that produces plan.removed to include bodies so the change.ts loop can
use the original removed content.
There was a problem hiding this comment.
My intention was to keep displaying removed requirements as they are today: just with the name, not with the full body. So the spec and the implementation match, but I'm open to human maintainer feedback that it might be useful to display the full body of the removed requirement.
There was a problem hiding this comment.
@bsmedberg-xometry, thanks for the clarification! That makes sense — showing only the name for REMOVED entries is a reasonable design choice (you're already conveying what was removed without re-displaying content that will no longer exist), and the spec and implementation are aligned on this. I'll withdraw the concern.
If a future maintainer does want the full removed body shown in diff mode, the path forward would be to change plan.removed from a string[] of names to an array of { name, raw } objects in parseDeltaSpec, then surface raw in the DiffResult renderer.
✏️ Learnings added
Learnt from: bsmedberg-xometry
Repo: Fission-AI/OpenSpec PR: 980
File: src/commands/change.ts:267-268
Timestamp: 2026-04-16T19:47:10.854Z
Learning: In OpenSpec (Fission-AI/OpenSpec), `showSpecDiffs()` in `src/commands/change.ts` intentionally displays REMOVED requirements with only the requirement name (synthesized header `### Requirement: ${name}`), not the full body. This matches the cli-show spec and is by design — do not flag the absence of the removed body as a bug.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: bsmedberg-xometry
Repo: Fission-AI/OpenSpec PR: 980
File: openspec/changes/spec-diffs/specs/cli-show/spec.md:44-46
Timestamp: 2026-04-16T19:44:15.889Z
Learning: In the OpenSpec repository (Fission-AI/OpenSpec), `openspec show <change-name> --diff` in text mode intentionally prints the proposal markdown text first, followed by per-requirement unified diffs for each delta spec. ADDED requirements are displayed in full. This is by design — do not flag the proposal-first ordering as a bug.
Learnt from: lsmonki
Repo: Fission-AI/OpenSpec PR: 660
File: openspec/changes/archive/2026-02-04-hierarchical-specs-support/design.md:96-136
Timestamp: 2026-02-04T17:50:12.189Z
Learning: In OpenSpec, delta specs handle deletions at the requirement level using `## REMOVED Requirements` sections within the delta spec.md file. There is no concept of deleting an entire spec file through deltas.
| function run(args: string): string { | ||
| const originalCwd = process.cwd(); | ||
| try { | ||
| process.chdir(testDir); | ||
| return execSync(`node ${openspecBin} ${args}`, { | ||
| encoding: 'utf-8', | ||
| env: { ...process.env, NO_COLOR: '1' }, | ||
| }); | ||
| } finally { | ||
| process.chdir(originalCwd); | ||
| } | ||
| } | ||
|
|
||
| function runWithStderr(args: string): { stdout: string; stderr: string } { | ||
| const originalCwd = process.cwd(); | ||
| try { | ||
| process.chdir(testDir); | ||
| const stdout = execSync(`node ${openspecBin} ${args}`, { | ||
| encoding: 'utf-8', | ||
| env: { ...process.env, NO_COLOR: '1' }, | ||
| stdio: ['pipe', 'pipe', 'pipe'], | ||
| }); | ||
| return { stdout, stderr: '' }; | ||
| } catch (e: any) { | ||
| return { stdout: e.stdout?.toString() ?? '', stderr: e.stderr?.toString() ?? '' }; | ||
| } finally { | ||
| process.chdir(originalCwd); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the exact code at the specified lines
cat -n test/commands/show-diff.test.ts | sed -n '100,127p'Repository: Fission-AI/OpenSpec
Length of output: 1127
🏁 Script executed:
# Also check the imports and broader context of these helper functions
cat -n test/commands/show-diff.test.ts | sed -n '1,50p'Repository: Fission-AI/OpenSpec
Length of output: 2077
🏁 Script executed:
# Check how these functions are called to understand arg handling
rg "run\(|runWithStderr\(" test/commands/show-diff.test.ts -A 2 -B 1Repository: Fission-AI/OpenSpec
Length of output: 1668
🏁 Script executed:
# Verify if there are any array-style arg usages or string-style arg usages in calls
rg "run\(" test/commands/show-diff.test.ts | head -20Repository: Fission-AI/OpenSpec
Length of output: 728
🏁 Script executed:
# Find the full test bodies related to text-mode diff assertions
rg "text mode.*diff" test/commands/show-diff.test.ts -A 10Repository: Fission-AI/OpenSpec
Length of output: 1246
🏁 Script executed:
# Check for test expectations about proposal appearing/not appearing with --diff
rg "Proposal|proposal" test/commands/show-diff.test.ts -B 2 -A 2Repository: Fission-AI/OpenSpec
Length of output: 2182
🏁 Script executed:
# Check if there's any documentation about the --diff feature behavior
fd -e md | xargs grep -l "diff" | head -5Repository: Fission-AI/OpenSpec
Length of output: 187
🏁 Script executed:
# Verify if this change to array args would break existing callers
wc -l test/commands/show-diff.test.tsRepository: Fission-AI/OpenSpec
Length of output: 98
🏁 Script executed:
# Check if there are any callers of run() that would break if signature changes to array
rg "run\(" test/commands/show-diff.test.ts | wc -lRepository: Fission-AI/OpenSpec
Length of output: 65
🏁 Script executed:
# Verify the PR description or any related context about --diff behavior
git log --oneline -20 -- test/commands/show-diff.test.tsRepository: Fission-AI/OpenSpec
Length of output: 162
🏁 Script executed:
# Check if the proposal text appearing with --diff is intended or a bug
rg "should.*diff.*omit|omit.*proposal|proposal.*omit" . -iRepository: Fission-AI/OpenSpec
Length of output: 45
Use execFileSync to avoid shell parsing vulnerabilities in test helpers.
Lines 104 and 117 interpolate openspecBin into shell command strings, creating failures when the repository path contains spaces or shell metacharacters. Replace with execFileSync(process.execPath, [openspecBin, ...args.split(' ')]) to safely handle paths and split arguments without shell interpretation.
Note: All 10 callers of run() and runWithStderr() currently pass space-separated argument strings; updating the helper signatures from args: string to args: string[] would require refactoring all call sites.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/commands/show-diff.test.ts` around lines 100 - 127, The test helpers run
and runWithStderr interpolate openspecBin into shell command strings which
breaks if paths contain spaces or shell metacharacters; replace execSync calls
with execFileSync(process.execPath, [openspecBin, ...args.split(' ')], ...) to
avoid shell parsing (preserve encoding and env, and for runWithStderr set stdio
to ['pipe','pipe','pipe'] and catch the thrown error to extract stdout/stderr as
done currently); update the function signatures/comments to indicate args is a
space-separated string that will be split into an argv array and ensure
process.chdir(originalCwd) remains in the finally block.
There was a problem hiding this comment.
This is a common pattern across the test suite (used in spec.test.ts, show.test.ts, and our show-diff.test.ts). Since this is not new to the tests in this PR, I'm going to decline this recommendation for now.
There was a problem hiding this comment.
@bsmedberg-xometry, understood — since this execSync with interpolated args pattern is pre-existing and consistent across the test suite, it makes sense to keep it as-is to maintain consistency. I'll note this for future reviews.
✏️ Learnings added
Learnt from: bsmedberg-xometry
Repo: Fission-AI/OpenSpec PR: 980
File: test/commands/show-diff.test.ts:100-127
Timestamp: 2026-04-16T19:52:43.181Z
Learning: In the Fission-AI/OpenSpec test suite (e.g., spec.test.ts, show.test.ts, show-diff.test.ts), using `execSync` with interpolated `openspecBin` shell command strings (rather than `execFileSync`) is an established, intentional pattern shared across all test helpers. Do not flag this as a shell injection/path-safety issue in this repository.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: bsmedberg-xometry
Repo: Fission-AI/OpenSpec PR: 980
File: openspec/changes/spec-diffs/specs/cli-show/spec.md:44-46
Timestamp: 2026-04-16T19:44:15.889Z
Learning: In the OpenSpec repository (Fission-AI/OpenSpec), `openspec show <change-name> --diff` in text mode intentionally prints the proposal markdown text first, followed by per-requirement unified diffs for each delta spec. ADDED requirements are displayed in full. This is by design — do not flag the proposal-first ordering as a bug.
Learnt from: bsmedberg-xometry
Repo: Fission-AI/OpenSpec PR: 980
File: src/commands/change.ts:267-268
Timestamp: 2026-04-16T19:47:10.854Z
Learning: In OpenSpec (Fission-AI/OpenSpec), `showSpecDiffs()` in `src/commands/change.ts` intentionally displays REMOVED requirements with only the requirement name (synthesized header `### Requirement: ${name}`), not the full body. This matches the cli-show spec and is by design — do not flag the absence of the removed body as a bug.
Learnt from: lsmonki
Repo: Fission-AI/OpenSpec PR: 660
File: openspec/changes/archive/2026-02-04-hierarchical-specs-support/design.md:96-136
Timestamp: 2026-02-04T17:50:12.189Z
Learning: In OpenSpec, delta specs handle deletions at the requirement level using `## REMOVED Requirements` sections within the delta spec.md file. There is no concept of deleting an entire spec file through deltas.
Learnt from: askpatrickw
Repo: Fission-AI/OpenSpec PR: 753
File: test/core/global-init.test.ts:104-111
Timestamp: 2026-02-24T22:25:19.145Z
Learning: In tests, enforce and verify the global root override behavior: OPENSPEC_GLOBAL_ROOT should unconditionally redirect global writes to the specified directory for testing/CI, regardless of adapter support. Ensure the null-adapter exclusion path is validated by testing that CommandAdapterRegistry.getGlobalAdapters() filters adapters by non-null getGlobalRoot() before resolveGlobalRoot() is invoked. Include a test scenario where a user explicitly specifies a tool (e.g., --tools cursor) with OPENSPEC_GLOBAL_ROOT set, and confirm this is treated as an intentional opt-in.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openspec/changes/spec-diffs/proposal.md (1)
19-25: Document the change-only constraint for--diff.The PR objectives state that "
--diffis a change-only option; using it on a non-change item emits a warning and is ignored," but this behavior isn't documented in the proposal. Readers should know that the flag only applies when showing change objects.📝 Suggested addition to clarify scope
Consider adding a note after line 21 or in the non-goals section:
- A `--diff` flag on `openspec show <change>` (and `openspec change show <change>`) that outputs a human-readable unified diff per delta spec + +Note: `--diff` only applies to change objects. Using it when showing a non-change spec will emit a warning and be ignored.Alternatively, add to the non-goals section after line 41:
- Diffing non-spec artifacts (proposal, design, tasks) - Git-aware diffing (this compares files on disk, not git history) +- Using `--diff` on non-change items (flag is ignored with a warning)
Optional: Clarify JSON mode behavior when the base spec file doesn't exist.
Line 23 documents that when a base spec exists but a requirement block isn't found, JSON mode includes a
"warning"field. It doesn't explicitly cover the edge case where the entire base spec file doesn't exist (new capability). Based on line 25's text-mode description and the PR objectives ("New delta specs...shown as all-additions"), these should presumably get a"diff"field with all-additions content rather than a warning. This is a minor edge case (new capabilities typically have onlyADDEDrequirements, notMODIFIED), but documenting it would be thorough.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openspec/changes/spec-diffs/proposal.md` around lines 19 - 25, Add a short note to the proposal explaining that the --diff flag is change-only: when used on non-change items it emits a warning and is ignored (place this after the "What this change delivers" paragraph or in non-goals), and explicitly document JSON mode's edge-case behavior when the entire base spec file is missing by stating that a MODIFIED delta with no base spec file should include a "diff" field containing the all-additions unified-diff (not a "warning"), mirroring the text-mode description; reference the `--diff` flag, "JSON mode"/"Text mode", and the `"diff"`/`"warning"` fields when adding these clarifications.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@openspec/changes/spec-diffs/proposal.md`:
- Around line 19-25: Add a short note to the proposal explaining that the --diff
flag is change-only: when used on non-change items it emits a warning and is
ignored (place this after the "What this change delivers" paragraph or in
non-goals), and explicitly document JSON mode's edge-case behavior when the
entire base spec file is missing by stating that a MODIFIED delta with no base
spec file should include a "diff" field containing the all-additions
unified-diff (not a "warning"), mirroring the text-mode description; reference
the `--diff` flag, "JSON mode"/"Text mode", and the `"diff"`/`"warning"` fields
when adding these clarifications.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3f05aaf5-ef65-4015-993a-8d06b5cf713c
📒 Files selected for processing (3)
openspec/changes/spec-diffs/proposal.mdsrc/utils/requirement-diff.tstest/utils/requirement-diff.test.ts
✅ Files skipped from review due to trivial changes (2)
- src/utils/requirement-diff.ts
- test/utils/requirement-diff.test.ts
1 participant
Why
When proposing a change, delta spec files under
openspec/changes/<name>/specs/duplicate large portions of existing specs inopenspec/specs/. A MODIFIED requirement must include the entire requirement block (all scenarios), making it hard to see what actually changed versus what was copied verbatim. This friction slows review and increases the risk of errors.What Changes
Add a
--diffflag toopenspec show(change type) that renders each delta spec as a unified diff against the corresponding base spec inopenspec/specs/. This is the smallest viable improvement: it doesn't change the storage format or workflow, just adds a new way to view the deltas.Approaches considered
Three approaches were evaluated:
Stop storing delta specs; edit base specs on the branch directly. This would eliminate duplication entirely but conflicts with the spec-driven workflow where changes are proposed, reviewed, and archived as discrete artifacts before the base specs are updated. Deferred — would require rethinking the change lifecycle.
Store deltas as diffs instead of full specs. The
specs/<cap>/spec.mdfiles inside a change would contain unified diffs (or a structured delta format) rather than full requirement text. This eliminates duplication at the source but complicates authoring (AI and humans must produce correct diffs), parsing, validation, and the archive/apply step that merges deltas into base specs. Promising for a future change, but high complexity.Add
openspec show --diffto render deltas against base specs. (Chosen.) Leave the storage format unchanged. When displaying a change, compute the diff on the fly by comparing each delta spec file against its matching base spec. This gives reviewers the view they need with minimal code changes and zero workflow disruption.What this change delivers
--diffflag onopenspec show <change>(andopenspec change show <change>) that outputs a human-readable unified diff per delta spec--json --diff), each delta includes adifffield with the unified diff text--diffis used without--json, the output is the diffs only (no proposal text)Capabilities
New Capabilities
None.
Modified Capabilities
cli-show: Add--diffflag support for change display, computing unified diffs of delta specs against their base specsNon-goals
Impact
src/commands/show.ts— pass--diffflag through to change displaysrc/commands/change.ts— implement diff rendering inshow()for text and JSON modessrc/cli/index.ts— register--diffoption on the show and change show commandssrc/commands/spec.tsorsrc/utils/— helper to resolve base spec path for a given delta specSpecifications Changed (diffs)
cli-show
ADDED: Requirement block extraction for diffing
MODIFIED: Output format options
Summary by CodeRabbit