Skip to content

allow-specless-changes#977

Open
bsmedberg-xometry wants to merge 8 commits intoFission-AI:mainfrom
bsmedberg-xometry:allow-specless-changes
Open

allow-specless-changes#977
bsmedberg-xometry wants to merge 8 commits intoFission-AI:mainfrom
bsmedberg-xometry:allow-specless-changes

Conversation

@bsmedberg-xometry
Copy link
Copy Markdown

@bsmedberg-xometry bsmedberg-xometry commented Apr 15, 2026

Why

Teams using OpenSpec for product specs and high-level technical standards often need to track changes that don't modify spec-level requirements — bug fixes, documentation refactoring, dependency upgrades, infrastructure work, or implementation-only refactors. The openspec/changes/ directory is valuable as a repository for technical change descriptions and team review artifacts even when no spec requirements change.

Today, openspec validate hard-fails with CHANGE_NO_DELTAS when a change has zero delta specs, and the artifact dependency graph blocks tasks on specs completion (determined by file existence of specs/**/*.md). This forces teams to either (a) skip the openspec workflow for non-spec changes, fragmenting their process, or (b) create artificial spec changes just to pass validation. Neither is acceptable.

What Changes

Add requireSpecDeltas as a top-level tri-state setting in openspec/config.yaml:

  • "error" (default when omitted) — current behavior, hard-fail on missing deltas
  • "warn" — emit a WARNING but pass validation
  • false — completely suppress the check, no output at all

When set to "warn" or false, two things change:

  1. Validation: the CHANGE_NO_DELTAS check emits the configured level (or nothing) instead of ERROR
  2. Artifact graph: detectCompleted() synthetically marks specs as complete so that tasks is unblocked and the propose workflow can proceed without writing spec files

The proposal instruction/template in the schema is static and doesn't adapt to this config. Projects using this feature should add a rules.proposal entry in config.yaml to tell the AI that Capabilities sections are optional. This uses the existing rules injection mechanism — no code changes needed.

Example config for specless workflow:

schema: spec-driven
requireSpecDeltas: false

rules:
  proposal:
    - "The Capabilities section is optional. If the change has no spec-level requirement changes, leave New Capabilities and Modified Capabilities as 'None'."

Considered and deferred: per-change metadata

A skipSpecDeltas: true field in .openspec.yaml would allow per-change overrides of the project default. This was considered but deferred because:

  • The artifact graph fix (synthetic completion in detectCompleted()) would need to read change metadata, adding I/O to a currently simple function
  • The project-level config covers the primary use case (teams that routinely make non-spec changes)
  • Can be added later without breaking changes

Capabilities

New Capabilities

(none — this extends existing capabilities)

Modified Capabilities

  • config-loading: Add top-level requireSpecDeltas (tri-state: "error" | "warn" | false) to the ProjectConfig schema and resilient parsing
  • cli-validate: Respect requireSpecDeltas when evaluating the CHANGE_NO_DELTAS check
  • artifact-graph: Synthetically mark specs as complete in detectCompleted() when requireSpecDeltas is not "error"
  • cli-artifact-workflow: Add "skipped" status to status display for synthetically completed artifacts (indicator [~], dim color, "skipped" in JSON)

Non-goals

  • Making the specs artifact disappear from openspec status — it remains visible as [~] skipped
  • Changing the ChangeSchema Zod validation for deltas.min(1) — that validates the proposal's Capabilities section, not spec files
  • Dynamic instruction adaptation — the schema's proposal instruction is static; projects use rules config to adjust AI guidance
  • Per-change override via .openspec.yaml — deferred for a future change

Impact

  • src/core/project-config.ts — extend ProjectConfigSchema with optional requireSpecDeltas
  • src/core/validation/validator.tsvalidateChangeDeltaSpecs() respects the tri-state setting
  • src/commands/validate.ts — read project config and pass setting to the validator
  • src/core/artifact-graph/state.tsdetectCompleted() synthetically marks specs done when config allows
  • src/core/validation/constants.ts — add message for the warning case

Summary by CodeRabbit

  • New Features

    • Tri-state project setting to treat changes with no spec deltas as error/warn/ignored ("error" | "warn" | false).
    • New "skipped" artifact status/indicator; skipped artifacts count toward completion and appear as "skipped" in JSON.
    • Improved report printing utilities to standardize issue output.
  • Behavior

    • Validation, status output, and exit codes follow the tri-state setting; synthetically completed specs can unblock workflows.
  • Documentation

    • Added design, proposal, and CLI/spec docs for specless-change handling and status semantics.
  • Tests

    • New coverage for config parsing, validation modes, synthetic completion, status display, and JSON output.

Add top-level `requireSpecDeltas` tri-state setting to config.yaml
("error" | "warn" | false) that controls whether changes must include
spec deltas. When set to "warn" or false, validation passes without
spec deltas and the artifact graph synthetically completes the specs
artifact so downstream artifacts (tasks) are unblocked.

Status display shows synthetically completed artifacts as [~] skipped.
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 15, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds a tri-state project config requireSpecDeltas ("error" | "warn" | false) and threads it through config loading, validation, artifact-completion detection, and CLI status rendering; enables synthetic completion of the specs artifact and introduces a 'skipped' artifact status.

Changes

Cohort / File(s) Summary
Project config & manifest
src/core/project-config.ts, openspec/changes/allow-specless-changes/.openspec.yaml
Add RequireSpecDeltasSchema/type; extend ProjectConfigSchema with optional requireSpecDeltas; resilient parsing in readProjectConfig() with warnings for invalid values; add change-level .openspec.yaml.
Validator & messages
src/core/validation/validator.ts, src/core/validation/constants.ts
Introduce ValidatorConfig { strictMode?, requireSpecDeltas? }; change constructor to accept config object; validateChangeDeltaSpecs emits ERROR/WARNING/none depending on requireSpecDeltas; add CHANGE_NO_DELTAS_ALLOWED message.
Artifact completion & loader
src/core/artifact-graph/state.ts, src/core/artifact-graph/instruction-loader.ts
Add DetectCompletedOptions (requireSpecDeltas); detectCompleted() can synthetically add 'specs' when not 'error'; loadChangeContext() passes option; formatChangeStatus() marks completed-but-fileless artifacts as 'skipped'.
CLI wiring & commands
src/commands/validate.ts, src/commands/change.ts, src/commands/spec.ts
Commands read project config and instantiate Validator with { strictMode, requireSpecDeltas }; printing/exit-code logic adjusted to reflect issue levels (ERROR vs WARNING) and JSON/text output semantics; moved issue rendering to printReportIssues.
Status display helpers
src/commands/workflow/shared.ts, src/commands/workflow/status.ts
Extend status union to include 'skipped'; add color/indicator mapping ([~], dim); count 'skipped' toward completion totals in printed progress.
Report printing utilities
src/utils/report-printer.ts, src/utils/index.ts
New ReportIssue type, issueSeverityPrefix, and printReportIssues functions; re-exported from utils index for centralized report rendering.
Specs / design / proposal docs
openspec/changes/allow-specless-changes/design.md, .../proposal.md, openspec/changes/allow-specless-changes/specs/..., tasks.md
Add design, proposal, and multiple spec documents describing requireSpecDeltas semantics, synthetic specs completion, CLI status expectations, config-loading resilience, and validation guidance.
Tests
test/core/project-config.test.ts, test/core/validation.test.ts, test/core/artifact-graph/state.test.ts, test/core/artifact-graph/instruction-loader.test.ts, test/commands/validate.test.ts, test/commands/validate-warnings.test.ts
Add/adjust tests for parsing requireSpecDeltas, validator behavior across tri-state and strict mode, synthetic specs completion, 'skipped' status reporting, and end-to-end validate/status flows.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant CLI
  participant Config as ProjectConfigReader
  participant Validator
  participant Graph as ArtifactGraph
  participant FS as Filesystem

  CLI->>Config: readProjectConfig(projectRoot)
  Config-->>CLI: projectConfig (requireSpecDeltas)
  CLI->>Validator: new Validator({ strictMode, requireSpecDeltas })
  CLI->>Graph: detectCompleted(graph, changeDir, { requireSpecDeltas })
  Graph->>FS: scan generated outputs (globs)
  FS-->>Graph: foundFiles / none
  alt no files & requireSpecDeltas != "error"
    Graph-->>CLI: include 'specs' in completed (synthetic -> 'skipped')
  else files present
    Graph-->>CLI: include matching artifacts as completed ('done')
  end
  CLI->>Validator: validateChangeDeltaSpecs(changeDir)
  Validator-->>CLI: report (errors/warnings/ok)
  CLI->>CLI: render status (use 'skipped' for synthetic specs)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested reviewers

  • TabishB
  • alfred-openspec

Poem

"I’m a rabbit who hops through diffs so neat,
I let specless changes skip a beat,
requireSpecDeltas set, the specs go light,
Marked [~] and dim — yet counted right. 🐇✨"

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 53.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'allow-specless-changes' is directly related to the main feature being implemented: allowing changes without spec deltas via a new requireSpecDeltas configuration option.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (2)
openspec/changes/allow-specless-changes/specs/cli-validate/spec.md (2)

14-14: Include both non-blocking config options in remediation guidance.

Current note only points to false; consider mentioning "warn" too, so users can opt into visible-but-non-blocking behavior.

✏️ Proposed wording fix
-  - Note: "If this change intentionally has no spec deltas, set `requireSpecDeltas: false` in openspec/config.yaml"
+  - Note: "If this change intentionally has no spec deltas, set `requireSpecDeltas: "warn"` (visible, non-blocking) or `requireSpecDeltas: false` (silent) in openspec/config.yaml"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@openspec/changes/allow-specless-changes/specs/cli-validate/spec.md` at line
14, Update the guidance note that references the openspec/config.yaml
requireSpecDeltas setting to mention both non-blocking options: set
requireSpecDeltas: false to silence validation for intentionally spec-less
changes, or set requireSpecDeltas: "warn" to surface a visible non-blocking
warning; update the sentence in spec.md that currently only shows
`requireSpecDeltas: false` to include and briefly explain the `"warn"` option as
an alternative.

47-47: Minor wording trim for clarity.

At Line 47, “before listing scenarios” reads more directly than “prior to listing scenarios.”

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@openspec/changes/allow-specless-changes/specs/cli-validate/spec.md` at line
47, Replace the phrase "prior to listing scenarios" with the simpler wording
"before listing scenarios" in the introductory sentence that precedes the
scenario list (look for the sentence containing "prior to listing scenarios");
update that single line to read "before listing scenarios" and run a quick
proofread to ensure punctuation and flow remain correct.
🤖 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/allow-specless-changes/design.md`:
- Line 113: Remove the editorial artifact text "~5 lines added.
`loadChangeContext()` in `instruction-loader.ts` reads project config and passes
it through." from the design doc; open
openspec/changes/allow-specless-changes/design.md, locate the sentence that
begins with "~5 lines added." near the paragraph describing loadChangeContext()
and delete that patch-note residue so the finalized design contains only the
intended prose about loadChangeContext() and instruction-loader.ts.

In `@openspec/changes/allow-specless-changes/specs/cli-artifact-workflow/spec.md`:
- Around line 30-33: The scenario wording is contradictory: "no artifacts" (line
containing WHEN user runs `openspec status --change <id>` on a change with no
artifacts) conflicts with the subsequent THEN/AND steps that display artifacts
and their statuses; update the WHEN clause to clarify it refers to a change with
no generated/completed artifact files yet (suggest: "on a change with no
generated artifact files yet" or "no completed artifacts"), so the WHEN reads
e.g. `WHEN user runs openspec status --change <id> on a change with no generated
artifact files yet`, leaving the THEN/AND lines about displaying artifact
statuses unchanged.

In `@openspec/changes/allow-specless-changes/tasks.md`:
- Line 43: The open checklist item "6.3 Verify Windows CI passes (no
path-separator issues in config loading or state detection)" must be resolved
before merge: either run the Windows CI, confirm there are no path-separator
regressions in the config/state logic, and mark the checkbox complete, or
explicitly defer by replacing the checklist entry with a note linking to a newly
created follow-up issue (include the issue number/URL) that scopes the Windows
verification and assigns an owner; update the tasks.md entry to reflect the
chosen resolution so the PR no longer contains an unresolved Windows
verification TODO.

In `@src/commands/change.ts`:
- Line 218: The change validate command isn't honoring the project's
requireSpecDeltas because that flag isn't passed into the Validator; update the
Validator instantiation in the change validate flow to include requireSpecDeltas
(e.g., pass requireSpecDeltas: projectConfig.requireSpecDeltas or
options?.requireSpecDeltas) alongside strictMode so Validator receives the
project's setting; ensure you reference the existing Validator constructor call
and the project config/options variable used in the change command so behavior
matches openspec validate.

---

Nitpick comments:
In `@openspec/changes/allow-specless-changes/specs/cli-validate/spec.md`:
- Line 14: Update the guidance note that references the openspec/config.yaml
requireSpecDeltas setting to mention both non-blocking options: set
requireSpecDeltas: false to silence validation for intentionally spec-less
changes, or set requireSpecDeltas: "warn" to surface a visible non-blocking
warning; update the sentence in spec.md that currently only shows
`requireSpecDeltas: false` to include and briefly explain the `"warn"` option as
an alternative.
- Line 47: Replace the phrase "prior to listing scenarios" with the simpler
wording "before listing scenarios" in the introductory sentence that precedes
the scenario list (look for the sentence containing "prior to listing
scenarios"); update that single line to read "before listing scenarios" and run
a quick proofread to ensure punctuation and flow remain correct.
🪄 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: ed91a91c-b98f-474c-8f78-673774b5545f

📥 Commits

Reviewing files that changed from the base of the PR and between f529b25 and 2b838c7.

📒 Files selected for processing (23)
  • openspec/changes/allow-specless-changes/.openspec.yaml
  • openspec/changes/allow-specless-changes/design.md
  • openspec/changes/allow-specless-changes/proposal.md
  • openspec/changes/allow-specless-changes/specs/artifact-graph/spec.md
  • openspec/changes/allow-specless-changes/specs/cli-artifact-workflow/spec.md
  • openspec/changes/allow-specless-changes/specs/cli-validate/spec.md
  • openspec/changes/allow-specless-changes/specs/config-loading/spec.md
  • openspec/changes/allow-specless-changes/tasks.md
  • src/commands/change.ts
  • src/commands/spec.ts
  • src/commands/validate.ts
  • src/commands/workflow/shared.ts
  • src/commands/workflow/status.ts
  • src/core/artifact-graph/instruction-loader.ts
  • src/core/artifact-graph/state.ts
  • src/core/project-config.ts
  • src/core/validation/constants.ts
  • src/core/validation/validator.ts
  • test/commands/validate.test.ts
  • test/core/artifact-graph/instruction-loader.test.ts
  • test/core/artifact-graph/state.test.ts
  • test/core/project-config.test.ts
  • test/core/validation.test.ts

Comment thread openspec/changes/allow-specless-changes/design.md Outdated
Comment thread openspec/changes/allow-specless-changes/specs/cli-artifact-workflow/spec.md Outdated

- [x] 6.1 Add integration test: temp project with `requireSpecDeltas: false`, change with no specs → `openspec validate <change>` passes, `openspec status --change <change> --json` shows `specs: "skipped"` and `tasks: "ready"`
- [x] 6.2 Verify all existing tests pass (`pnpm test`)
- [ ] 6.3 Verify Windows CI passes (no path-separator issues in config loading or state detection)
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Unresolved Windows verification task should be closed before merge (or explicitly deferred).

Line 43 is still open; please either complete this CI verification or document deferral with a linked follow-up issue to avoid cross-platform regression risk.

If you want, I can draft a concise follow-up issue template for the Windows path-separator validation scope.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@openspec/changes/allow-specless-changes/tasks.md` at line 43, The open
checklist item "6.3 Verify Windows CI passes (no path-separator issues in config
loading or state detection)" must be resolved before merge: either run the
Windows CI, confirm there are no path-separator regressions in the config/state
logic, and mark the checkbox complete, or explicitly defer by replacing the
checklist entry with a note linking to a newly created follow-up issue (include
the issue number/URL) that scopes the Windows verification and assigns an owner;
update the tasks.md entry to reflect the chosen resolution so the PR no longer
contains an unresolved Windows verification TODO.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pending confirmation via github CI on this PR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
gh pr checks 977 --repo Fission-AI/OpenSpec

Length of output: 35


🏁 Script executed:

#!/bin/bash
# Check workflow runs on the PR branch
gh run list --repo Fission-AI/OpenSpec --branch allow-specless-changes --limit 10

Length of output: 123


🏁 Script executed:

#!/bin/bash
# Get jobs for the CI workflow run
gh run view 24478530371 --repo Fission-AI/OpenSpec --json jobs | jq '.jobs[] | {name: .name, status: .status, conclusion: .conclusion}'

Length of output: 0


🏁 Script executed:

#!/bin/bash
# Try viewing the run directly
gh run view 24478530371 --repo Fission-AI/OpenSpec

# Also try getting all jobs
gh api repos/Fission-AI/OpenSpec/actions/runs/24478530371/jobs --jq '.jobs[] | {name: .name, status: .status, conclusion: .conclusion}'

Length of output: 271


🏁 Script executed:

#!/bin/bash
# Try log-failed
gh run view 24478530371 --log-failed --repo Fission-AI/OpenSpec 2>&1 | head -50

# Try API with different endpoint
gh api repos/Fission-AI/OpenSpec/actions/runs/24478530371 --jq '{status: .status, conclusion: .conclusion, name: .name, event: .event, head_branch: .head_branch}'

Length of output: 164


@bsmedberg-xometry, understood — I've checked the CI run directly. The workflow run (#24478530371) shows a conclusion of action_required, meaning no jobs have actually executed yet — GitHub Actions is waiting for a maintainer to approve the workflow run before it starts. This means Windows CI (task 6.3) is still genuinely unconfirmed.

Once a maintainer approves the workflow run and CI completes — including the Windows job — task 6.3 can be marked [x]. In the meantime, the concern stands: the item is open and the cross-platform verification hasn't happened yet.

A few options to consider:

  1. Approve and run CI now — a repo maintainer can approve the pending workflow run so the full suite (including Windows) executes before merge.
  2. Defer explicitly — replace the open [ ] with a note and a linked follow-up issue so the PR can merge without the item appearing as an unresolved TODO. Would you like me to open that follow-up issue?

🧠 Learnings used
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

Comment thread src/commands/change.ts Outdated
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

218-241: ⚠️ Potential issue | 🟠 Major

Text-mode warn still behaves like false.

Line 227 treats every report.valid === true result as a clean pass. With requireSpecDeltas: 'warn' in non-strict mode, the validator returns a valid report that still contains warnings, so openspec change validate prints only Change "...\" is valid and never emits the warning this feature is supposed to surface.

Suggested fix
     const validator = new Validator({ strictMode: options?.strict || false, requireSpecDeltas });
     const report = await validator.validateChangeDeltaSpecs(changeDir);
+    const warnings = report.issues.filter(issue => issue.level === 'WARNING');
     
     if (options?.json) {
       console.log(JSON.stringify(report, null, 2));
     } else {
       if (report.valid) {
+        warnings.forEach(issue => {
+          console.error(`⚠ [WARNING] ${issue.path}: ${issue.message}`);
+        });
         console.log(`Change "${changeName}" is valid`);
       } else {
         console.error(`Change "${changeName}" has issues`);
         report.issues.forEach(issue => {
           const label = issue.level === 'ERROR' ? 'ERROR' : 'WARNING';
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/commands/change.ts` around lines 218 - 241, The code treats report.valid
as "no issues" but Validator can return report.valid === true while still
including WARNING-level issues when requireSpecDeltas === 'warn'; update the
printing branch that currently uses report.valid to instead consider
report.issues: print "Change ... is valid" only when report.issues is empty
(e.g. report.issues?.length === 0), otherwise always iterate and print
report.issues (using the existing formatting), and set process.exitCode = 1 only
if any issue has level 'ERROR' (e.g. report.issues.some(i => i.level ===
'ERROR')); keep the JSON output path unchanged and still call
this.printNextSteps() after printing issues.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@src/commands/change.ts`:
- Around line 218-241: The code treats report.valid as "no issues" but Validator
can return report.valid === true while still including WARNING-level issues when
requireSpecDeltas === 'warn'; update the printing branch that currently uses
report.valid to instead consider report.issues: print "Change ... is valid" only
when report.issues is empty (e.g. report.issues?.length === 0), otherwise always
iterate and print report.issues (using the existing formatting), and set
process.exitCode = 1 only if any issue has level 'ERROR' (e.g.
report.issues.some(i => i.level === 'ERROR')); keep the JSON output path
unchanged and still call this.printNextSteps() after printing issues.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 38942833-b8fd-4144-bd0f-3895f6972a5b

📥 Commits

Reviewing files that changed from the base of the PR and between 2b838c7 and a19539a.

📒 Files selected for processing (3)
  • openspec/changes/allow-specless-changes/design.md
  • openspec/changes/allow-specless-changes/specs/cli-artifact-workflow/spec.md
  • src/commands/change.ts
✅ Files skipped from review due to trivial changes (1)
  • openspec/changes/allow-specless-changes/specs/cli-artifact-workflow/spec.md

…f warnings. I'm proposing a spec for this case where the change is listed as valid but the warnings are still printed.

Thanks coderabbit for identifying the issue.
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/commands/validate.ts`:
- Around line 151-180: The code currently only treats ERROR-level issues as
failures; change the logic to treat report.valid === false as a failed
validation regardless of issue levels: define a boolean like isInvalid =
!report.valid (or set hasErrors = hasErrors || !report.valid), use that to set
process.exitCode = 1, and to drive the non-JSON output path so that when
isInvalid is true the block that logs "has issues" and prints report.issues is
executed (still rendering each issue by their level), while only when
report.valid is true should the code print the "is valid" branch; update
references to hasErrors/hasWarnings (and the branch that calls
this.printNextSteps) to use this new isInvalid condition so --strict/warn
semantics correctly fail single-item validations.

In `@test/commands/validate-warnings.test.ts`:
- Around line 45-170: The tests miss coverage for the single-item strict mode
where requireSpecDeltas: "warn" should cause warnings to fail; add tests
invoking runCLI with the --strict flag for both change validate <id> and
validate <id> flows (use the existing scaffoldSpeclessChange and writeConfig
helpers), asserting that when config contains "requireSpecDeltas: warn" the
result.exitCode is non-zero (fail), stderr contains 'WARNING' and 'has issues'
(or equivalent failure message), and that JSON output for --json + --strict
marks the item invalid; add these two test cases near the corresponding
single-item tests (mirror the existing 'prints "is valid"...' and 'includes
warning issues in JSON...' cases) so they exercise the single-item strict
reporting 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

Run ID: 251edd60-b68f-4973-bf93-80689c3caf0d

📥 Commits

Reviewing files that changed from the base of the PR and between a19539a and f065b8c.

📒 Files selected for processing (4)
  • openspec/changes/allow-specless-changes/specs/cli-validate/spec.md
  • src/commands/change.ts
  • src/commands/validate.ts
  • test/commands/validate-warnings.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/commands/change.ts

Comment thread src/commands/validate.ts Outdated
Comment thread test/commands/validate-warnings.test.ts
…sence of errors or warnings. Fixes --strict with warnings. Thanks coderabbit!
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/commands/change.ts (1)

230-245: Consider sharing the report formatter with src/commands/validate.ts.

This branch now mirrors the severity-prefix and warning-reporting logic in src/commands/validate.ts:167-175 almost exactly. Extracting a shared helper would reduce drift the next time output or exit-code semantics change.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/commands/change.ts` around lines 230 - 245, The change.ts block
duplicates the severity-prefix and issue-printing logic already used in
validate.ts (lines ~167-175); extract a shared helper (e.g.,
formatOrPrintReportIssues / printReportIssues) that accepts the report (or
report.issues), a changeName/context string, and a mode flag for using
console.error vs console.log or for returning whether to set exit code,
implement the prefix selection logic (✗/⚠/ℹ) and per-issue output there, export
it from a common helper module (utils/reportPrinter or similar), and replace the
duplicated loops in both src/commands/change.ts (around printNextSteps/exitCode
handling) and src/commands/validate.ts to call the shared helper so output and
exit-code semantics stay consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/commands/change.ts`:
- Around line 230-245: The change.ts block duplicates the severity-prefix and
issue-printing logic already used in validate.ts (lines ~167-175); extract a
shared helper (e.g., formatOrPrintReportIssues / printReportIssues) that accepts
the report (or report.issues), a changeName/context string, and a mode flag for
using console.error vs console.log or for returning whether to set exit code,
implement the prefix selection logic (✗/⚠/ℹ) and per-issue output there, export
it from a common helper module (utils/reportPrinter or similar), and replace the
duplicated loops in both src/commands/change.ts (around printNextSteps/exitCode
handling) and src/commands/validate.ts to call the shared helper so output and
exit-code semantics stay consistent.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 19034912-da1d-4af7-bc0d-ceb53a42cb20

📥 Commits

Reviewing files that changed from the base of the PR and between f065b8c and 828c79e.

📒 Files selected for processing (3)
  • src/commands/change.ts
  • src/commands/validate.ts
  • test/commands/validate-warnings.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • test/commands/validate-warnings.test.ts
  • src/commands/validate.ts

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/utils/report-printer.ts`:
- Around line 13-26: The JSDoc for printReportIssues is incorrect: it claims the
function "Returns true when the report is invalid" and mentions an "optional
next-steps callback" that doesn't exist; update the docstring to match the
actual signature and behavior (no return value) and remove any reference to a
return value or a non-existent callback, or alternatively if you intend a
callback/boolean return add them to the function signature—edit the comment
above printReportIssues (and references to label, report, ReportIssue) so the
description accurately states the three output behaviors and that the function
returns void (or implement the callback/boolean return consistently across the
function and its callers).
🪄 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: dde98a21-4f96-468b-bbbd-6dee529e8787

📥 Commits

Reviewing files that changed from the base of the PR and between 828c79e and 0971428.

📒 Files selected for processing (4)
  • src/commands/change.ts
  • src/commands/validate.ts
  • src/utils/index.ts
  • src/utils/report-printer.ts
✅ Files skipped from review due to trivial changes (1)
  • src/utils/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/commands/change.ts

Comment thread src/utils/report-printer.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant