From e6bfd30da3fbd80a3a6855910c6d8710749c1dda Mon Sep 17 00:00:00 2001 From: Tabish Bidiwale Date: Thu, 25 Sep 2025 12:59:38 +1000 Subject: [PATCH 1/3] feat: add CLI e2e testing improvement plan ## Summary - Add phased approach to stabilize CLI spawn testing - Expand cross-shell/OS matrix coverage when stable - Optional packaging validation for CI environments --- openspec/changes/improve-cli-e2e-plan/proposal.md | 12 ++++++++++++ openspec/changes/improve-cli-e2e-plan/tasks.md | 14 ++++++++++++++ 2 files changed, 26 insertions(+) create mode 100644 openspec/changes/improve-cli-e2e-plan/proposal.md create mode 100644 openspec/changes/improve-cli-e2e-plan/tasks.md diff --git a/openspec/changes/improve-cli-e2e-plan/proposal.md b/openspec/changes/improve-cli-e2e-plan/proposal.md new file mode 100644 index 00000000..9d4786d2 --- /dev/null +++ b/openspec/changes/improve-cli-e2e-plan/proposal.md @@ -0,0 +1,12 @@ +## Why +Recent cross-shell regressions for `openspec` commands revealed that our existing unit/integration tests do not exercise the packaged CLI or shell-specific behavior. The prior attempt at Vitest spawn tests stalled because it coupled e2e coverage with `pnpm pack` installs, which fail in network-restricted environments. With those findings incorporated, we now need an approved plan to realign the work. + +## What Changes +- Adopt a phased strategy that first stabilizes direct spawn testing of the built CLI (`node dist/cli.js`) using lightweight fixtures and shared helpers. +- Expand coverage to cross-shell/OS matrices once the spawn harness is stable, ensuring both `dist/cli.js` and the bin shim are exercised with non-TTY defaults and captured diagnostics. +- Treat packaging/install validation as an optional CI safeguard: when a runner has registry access, run a simple pnpm-based pack→install→smoke-test flow; otherwise document it as out of scope while closing remaining hardening items. + +## Impact +- Tests: add `test/cli-e2e` spawn suite, helpers, and fixture usage updates; adjust `vitest.setup.ts` as needed. +- Tooling: update GitHub Actions workflows to add shell/OS matrices and (optionally) a packaging install check where network is available. +- Docs: keep `CROSS-SHELL-PLAN.md` aligned with the phased rollout and record any limitations called out during execution. diff --git a/openspec/changes/improve-cli-e2e-plan/tasks.md b/openspec/changes/improve-cli-e2e-plan/tasks.md new file mode 100644 index 00000000..5bca72df --- /dev/null +++ b/openspec/changes/improve-cli-e2e-plan/tasks.md @@ -0,0 +1,14 @@ +## 1. Phase 1 – Stabilize Local Spawn Coverage +- [ ] 1.1 Update `vitest.setup.ts` and helpers so the CLI build runs once and `runCLI` executes `node dist/cli.js` with non-TTY defaults. +- [ ] 1.2 Reuse the minimal fixture set (`tmp-init` or copy) to seed initial spawn tests for help/version, a happy-path `validate`, and a representative error flow. +- [ ] 1.3 Document the Phase 1 coverage details in `CROSS-SHELL-PLAN.md`, noting any outstanding gaps. + +## 2. Phase 2 – Expand Cross-Shell Validation +- [ ] 2.1 Exercise both entry points (`node dist/cli.js`, `bin/openspec.js`) in the spawn suite and add diagnostics for shell/OS context. +- [ ] 2.2 Extend GitHub Actions to run the spawn suite across a matrix of shells (bash, zsh, fish, pwsh, cmd) on macOS, Linux, and Windows runners. +- [ ] 2.3 Update `CROSS-SHELL-PLAN.md` with the matrix details, keeping non-TTY defaults and noting any targeted TTY scenarios for prompts/spinners. + +## 3. Phase 3 – Package Validation (Optional) +- [ ] 3.1 Add a simple CI job on runners with registry access that runs `pnpm pack`, installs the tarball into a temp workspace (e.g., `pnpm add --no-save`), and executes `pnpm exec openspec --version`. +- [ ] 3.2 If network-restricted environments can’t exercise installs, document the limitation in `CROSS-SHELL-PLAN.md` and skip the job there. +- [ ] 3.3 Close out remaining hardening items from the original cross-shell plan (e.g., `.gitattributes`, chmod enforcement, SIGINT follow-ups) and update the plan accordingly. From a32bb65d45f5ebc4acd15eb666126ee0efb0edf0 Mon Sep 17 00:00:00 2001 From: Tabish Bidiwale Date: Thu, 25 Sep 2025 14:40:17 +1000 Subject: [PATCH 2/3] feat: add markdown parser CRLF fix proposal and update e2e plan - Add comprehensive proposal for fixing CRLF parsing issues on Windows - Update CLI path references from dist/cli.js to dist/cli/index.js - Streamline e2e testing tasks based on refined approach --- .../changes/improve-cli-e2e-plan/proposal.md | 4 ++-- .../changes/improve-cli-e2e-plan/tasks.md | 1 - .../update-markdown-parser-crlf/proposal.md | 19 +++++++++++++++++++ .../specs/cli-validate/spec.md | 9 +++++++++ .../update-markdown-parser-crlf/tasks.md | 11 +++++++++++ 5 files changed, 41 insertions(+), 3 deletions(-) create mode 100644 openspec/changes/update-markdown-parser-crlf/proposal.md create mode 100644 openspec/changes/update-markdown-parser-crlf/specs/cli-validate/spec.md create mode 100644 openspec/changes/update-markdown-parser-crlf/tasks.md diff --git a/openspec/changes/improve-cli-e2e-plan/proposal.md b/openspec/changes/improve-cli-e2e-plan/proposal.md index 9d4786d2..a2a2ace5 100644 --- a/openspec/changes/improve-cli-e2e-plan/proposal.md +++ b/openspec/changes/improve-cli-e2e-plan/proposal.md @@ -2,8 +2,8 @@ Recent cross-shell regressions for `openspec` commands revealed that our existing unit/integration tests do not exercise the packaged CLI or shell-specific behavior. The prior attempt at Vitest spawn tests stalled because it coupled e2e coverage with `pnpm pack` installs, which fail in network-restricted environments. With those findings incorporated, we now need an approved plan to realign the work. ## What Changes -- Adopt a phased strategy that first stabilizes direct spawn testing of the built CLI (`node dist/cli.js`) using lightweight fixtures and shared helpers. -- Expand coverage to cross-shell/OS matrices once the spawn harness is stable, ensuring both `dist/cli.js` and the bin shim are exercised with non-TTY defaults and captured diagnostics. +- Adopt a phased strategy that first stabilizes direct spawn testing of the built CLI (`node dist/cli/index.js`) using lightweight fixtures and shared helpers. +- Expand coverage to cross-shell/OS matrices once the spawn harness is stable, ensuring both the direct `node dist/cli/index.js` invocation and the bin shim are exercised with non-TTY defaults and captured diagnostics. - Treat packaging/install validation as an optional CI safeguard: when a runner has registry access, run a simple pnpm-based pack→install→smoke-test flow; otherwise document it as out of scope while closing remaining hardening items. ## Impact diff --git a/openspec/changes/improve-cli-e2e-plan/tasks.md b/openspec/changes/improve-cli-e2e-plan/tasks.md index 5bca72df..710884dd 100644 --- a/openspec/changes/improve-cli-e2e-plan/tasks.md +++ b/openspec/changes/improve-cli-e2e-plan/tasks.md @@ -6,7 +6,6 @@ ## 2. Phase 2 – Expand Cross-Shell Validation - [ ] 2.1 Exercise both entry points (`node dist/cli.js`, `bin/openspec.js`) in the spawn suite and add diagnostics for shell/OS context. - [ ] 2.2 Extend GitHub Actions to run the spawn suite across a matrix of shells (bash, zsh, fish, pwsh, cmd) on macOS, Linux, and Windows runners. -- [ ] 2.3 Update `CROSS-SHELL-PLAN.md` with the matrix details, keeping non-TTY defaults and noting any targeted TTY scenarios for prompts/spinners. ## 3. Phase 3 – Package Validation (Optional) - [ ] 3.1 Add a simple CI job on runners with registry access that runs `pnpm pack`, installs the tarball into a temp workspace (e.g., `pnpm add --no-save`), and executes `pnpm exec openspec --version`. diff --git a/openspec/changes/update-markdown-parser-crlf/proposal.md b/openspec/changes/update-markdown-parser-crlf/proposal.md new file mode 100644 index 00000000..751673e9 --- /dev/null +++ b/openspec/changes/update-markdown-parser-crlf/proposal.md @@ -0,0 +1,19 @@ +# Update Markdown Parser CRLF Handling + +## Problem +Windows users report that `openspec validate` raises “Change must have a Why section” even when the section exists (see GitHub issue #77). The CLI currently splits markdown on `\n` and compares headers without stripping `\r`, so files saved with CRLF line endings keep a trailing carriage return in the header token. As a result the parser fails to detect `## Why`/`## What Changes`, triggering false validation errors and breaking the workflow on Windows-default editors. + +## Solution +- Normalize markdown content inside the parser so CRLF and lone-CR inputs are treated as `\n` before section detection, trimming any carriage returns from titles and content comparisons. +- Reuse the normalized reader everywhere `MarkdownParser` is constructed to keep behavior consistent for validation, view, spec, and list flows. +- Add regression coverage that reproduces the failure (unit test around `parseChange` and a CLI spawn/e2e test that writes a CRLF change then runs `openspec validate`). +- Update the `cli-validate` spec to codify the expectation that required sections are recognized regardless of line-ending style. + +## Benefits +- Restores correct validation behavior for Windows editors without requiring manual line-ending conversion. +- Locks in the fix with targeted tests so future parser refactors keep cross-platform support. +- Clarifies the spec so downstream work (e.g., cross-shell e2e plan) understands the non-negotiable behavior. + +## Risks +- Low: parser normalization touches shared code paths that parse specs and changes; need to ensure no regressions in other command consumers (mitigated by existing parser tests plus the new CRLF fixtures). + diff --git a/openspec/changes/update-markdown-parser-crlf/specs/cli-validate/spec.md b/openspec/changes/update-markdown-parser-crlf/specs/cli-validate/spec.md new file mode 100644 index 00000000..aeeb0ad1 --- /dev/null +++ b/openspec/changes/update-markdown-parser-crlf/specs/cli-validate/spec.md @@ -0,0 +1,9 @@ +## MODIFIED Requirements +### Requirement: Validation SHALL provide actionable remediation steps +Validation output SHALL include specific guidance to fix each error, including expected structure, example headers, and suggested commands to verify fixes. + +#### Scenario: Required sections parsed with CRLF line endings +- **GIVEN** a change proposal markdown saved with CRLF line endings +- **AND** the document contains `## Why` and `## What Changes` +- **WHEN** running `openspec validate ` +- **THEN** validation SHALL recognize the sections and NOT raise the "Change must have a Why section" error diff --git a/openspec/changes/update-markdown-parser-crlf/tasks.md b/openspec/changes/update-markdown-parser-crlf/tasks.md new file mode 100644 index 00000000..a1c6706b --- /dev/null +++ b/openspec/changes/update-markdown-parser-crlf/tasks.md @@ -0,0 +1,11 @@ +## 1. Guard the regression +- [ ] 1.1 Add a unit test that feeds a CRLF change document into `MarkdownParser.parseChange` and asserts `Why`/`What Changes` are detected. +- [ ] 1.2 Add a CLI spawn/e2e test that writes a CRLF change, runs `openspec validate`, and expects success. + +## 2. Normalize parsing +- [ ] 2.1 Normalize line endings when constructing `MarkdownParser` so headers and content comparisons ignore `\r`. +- [ ] 2.2 Ensure all CLI entry points (validate, view, spec conversion) reuse the normalized parser path. + +## 3. Document and verify +- [ ] 3.1 Update the `cli-validate` spec with a scenario covering CRLF line endings. +- [ ] 3.2 Run the parser and CLI test suites (`pnpm test`, relevant spawn tests) to confirm the fix. From 77cef014cff8526422e8de9e6278de5223d3a05a Mon Sep 17 00:00:00 2001 From: Tabish Bidiwale Date: Thu, 25 Sep 2025 14:50:39 +1000 Subject: [PATCH 3/3] fix: correct spec delta to add parsing requirement instead of modifying remediation - Change from MODIFIED to ADDED Requirements for cross-platform line ending parsing - Create focused requirement for parser behavior rather than validation messages - Maintain logical coherence between requirement and scenario --- .../specs/cli-validate/spec.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/openspec/changes/update-markdown-parser-crlf/specs/cli-validate/spec.md b/openspec/changes/update-markdown-parser-crlf/specs/cli-validate/spec.md index aeeb0ad1..55d70865 100644 --- a/openspec/changes/update-markdown-parser-crlf/specs/cli-validate/spec.md +++ b/openspec/changes/update-markdown-parser-crlf/specs/cli-validate/spec.md @@ -1,9 +1,9 @@ -## MODIFIED Requirements -### Requirement: Validation SHALL provide actionable remediation steps -Validation output SHALL include specific guidance to fix each error, including expected structure, example headers, and suggested commands to verify fixes. +## ADDED Requirements +### Requirement: Parser SHALL handle cross-platform line endings +The markdown parser SHALL correctly identify sections regardless of line ending format (LF, CRLF, CR). #### Scenario: Required sections parsed with CRLF line endings - **GIVEN** a change proposal markdown saved with CRLF line endings - **AND** the document contains `## Why` and `## What Changes` - **WHEN** running `openspec validate ` -- **THEN** validation SHALL recognize the sections and NOT raise the "Change must have a Why section" error +- **THEN** validation SHALL recognize the sections and NOT raise parsing errors