diff --git a/.changeset/validate-reports-purpose-placeholder.md b/.changeset/validate-reports-purpose-placeholder.md new file mode 100644 index 0000000000..d0cef003f3 --- /dev/null +++ b/.changeset/validate-reports-purpose-placeholder.md @@ -0,0 +1,11 @@ +--- +"@fission-ai/openspec": patch +--- + +`openspec validate` now reports a `## Purpose` that is still the placeholder archive writes for a new capability, instead of passing it. The placeholder is longer than the 50-character brevity floor, so until now the one check meant to catch a Purpose nobody wrote was satisfied by the exact text saying nobody wrote one — a spec whose Purpose read `Does stuff.` failed `--strict` while a spec whose Purpose said nothing at all passed. A capability could carry the placeholder indefinitely while every command reported success. + +It is a warning, so a project that already has placeholders on disk keeps validating by default and only `--strict` fails. The message says to edit the main spec directly, since a `## Purpose` in a delta is read only when the capability is created and cannot replace an existing one. + +Detection is narrow. The placeholder archive generates is recognised through the same definition that writes it, wherever it appears in the Purpose. Otherwise only a `TBD` or `TODO` opening the Purpose counts, so `The retry budget is TBD pending benchmarks` is still a valid Purpose and a word like `TBDs` is not a marker. Fenced code inside the Purpose is quoted material rather than the Purpose speaking, so a spec that documents the placeholder keeps passing. An empty Purpose is unchanged, and a Purpose reported as a placeholder is no longer also reported as too brief, so a bare `TBD` yields one finding rather than two. + +`openspec archive` is unaffected: it validates rebuilt specs without `--strict`, so a spec archive writes still passes the validation it would have passed before, and the text archive writes is unchanged. diff --git a/openspec/changes/warn-on-purpose-placeholder/.openspec.yaml b/openspec/changes/warn-on-purpose-placeholder/.openspec.yaml new file mode 100644 index 0000000000..0c73c8f54e --- /dev/null +++ b/openspec/changes/warn-on-purpose-placeholder/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-08-15 diff --git a/openspec/changes/warn-on-purpose-placeholder/design.md b/openspec/changes/warn-on-purpose-placeholder/design.md new file mode 100644 index 0000000000..393a10b1cd --- /dev/null +++ b/openspec/changes/warn-on-purpose-placeholder/design.md @@ -0,0 +1,162 @@ +## Context + +See proposal.md — Why. What shapes the approach here is where the two ends +already sit: + +- `buildSpecSkeleton` composes the placeholder inline, interpolating the change + name. It is generated text with no name of its own. +- `applySpecRules` is the single place both spec entry points converge — + `validateSpec` (a file) and `validateSpecContent` (a rebuilt spec, called by + archive). A rule added there reaches the CLI and archive at once, so the + blast radius on archive has to be answered rather than assumed. +- Strict mode is already defined as "warnings fail": `createReport` treats a + warning as invalid only when `strictMode` is set. Severity is therefore a + choice between two existing behaviors, not a new mechanism. +- `task-numbering.ts` establishes the shape for a check like this: a pure module + under `src/core/validation/` returning findings, mapped to issues at one call + site in the validator. + +## Goals / Non-Goals + +**Goals:** + +- Report the placeholder without changing what any command does today by default. +- Recognise placeholders already on disk, including ones written by earlier + versions, since those are the ones that have lingered longest. +- Keep the rule quiet on authored prose, so the warning stays worth reading. + +**Non-Goals:** + +- Changing what archive writes. The placeholder is a useful marker at the moment + it is written; this change is about reporting it afterwards. +- Reporting a `## Purpose` in a delta spec. Delta Purposes are only read when a + capability is created, and archive already warns when it ignores one. +- Filling the Purpose in automatically. Only the author knows what the capability + is for. + +## Decisions + +### Severity is a warning, not an error + +Strict mode already means "warnings are failures", so a warning gives both +behaviors from one severity: silent by default, failing under `--strict`. + +*Alternative — error:* every project with a placeholder on disk starts failing +`openspec validate` on upgrade. On the evidence that these linger for months, +that is a large and involuntary blast radius for a documentation defect. + +*Alternative — a dedicated opt-in flag:* adds a surface to learn and to document, +and duplicates what `--strict` is for. Rejected as a second mechanism for an +existing one. + +### The check lives in validation, not in archive + +Placed as a pure module beside `task-numbering.ts` and called from +`applySpecRules`, so it applies to every path that validates a main spec. + +*Alternative — report at archive time, when the placeholder is written:* archive +already prints at that moment, and a line in a terminal is exactly what did not +survive. The defect is what persists on disk, so the check belongs where disk +state is inspected, and it must keep working for a spec archived a year ago by a +version that no longer runs. + +### The generated sentence is recognised through a shared constant + +The placeholder is text this tool generates, so it gets a name: the template +moves into a constant that `buildSpecSkeleton` composes from and the check +recognises through. Detection is then anchored to the thing itself rather than to +a second, hand-copied spelling of it that can drift from the writer. + +The change name is interpolated, so recognition matches the constant's fixed +segments around it rather than the whole string. + +*Alternative — spell the sentence out in the detector:* two independent copies of +one string, and the check silently stops matching the day the writer is reworded +— the failure mode being a check that reports nothing and looks healthy. + +### A second, narrow marker rule covers what the constant cannot + +A placeholder is not always the generated one. The `specs` instruction tells +agents to write "a brief TBD placeholder" when a delta has none, and an agent +writes its own wording. So a `TBD` **opening** the Purpose is also reported. + +The rule is deliberately positional rather than a search: a Purpose that opens +with `TBD` is announcing it was not written, while "the retry budget is TBD +pending benchmarks" is a real Purpose with an open question in it. Reporting the +second would train people to ignore the warning, which costs more than the +findings it would add. A word that merely starts with those letters (`TBDs`) is +excluded for the same reason. + +This is the one place the change cannot use an explicit lookup — the text is +written by agents and authors, not generated here, so there is no list to consult. +It is kept to a single anchored marker at a known position precisely to stay as +close to a lookup as the input allows. + +It also covers a case the constant match cannot. A markdown formatter that +rewraps the generated sentence across two lines breaks the constant lookup, and +the marker rule still catches it, because every spelling of the placeholder opens +with `TBD`. So the fallback is not only for agent-written placeholders — it is +what keeps detection working when the generated one is reformatted. + +### The placeholder finding replaces the brevity finding + +A bare `TBD` is both a placeholder and under the length floor. Reporting both puts +two findings on one line where only one is actionable: "you left the placeholder +in" tells the author what to do, "your Purpose is under 50 characters" does not. +The placeholder check therefore runs first and the brevity check runs only when it +does not fire. + +### Locating the line follows the rule that matched + +The warning names the line carrying the placeholder, and which line that is +depends on which rule fired. A leading `TBD` is the section's first non-blank line +by definition. The generated sentence is not: it can sit below prose somebody +wrote, so it is located by its own text. + +Naming the first non-blank line in that second case points at the authored prose — +a line the reader can see is fine, which reads as the check being wrong rather +than the Purpose being unwritten. When both rules match the leading marker wins, +because it is the earlier of the two. + +When the placeholder cannot be located — no section header, or a generated +sentence no single line carries — the finding is reported without a line rather +than with a guessed one, since a wrong line number is worse than none. + +Line endings are normalised before counting, so a spec saved on Windows reports +the same line number as the same spec saved on macOS or Linux. + +## Risks / Trade-offs + +- **A project running `--strict` in CI starts failing on upgrade** → that is the + intended effect and the reason severity is not an error: the failure is opt-in, + arrives only where a stricter gate was already requested, and is fixed by + writing one sentence. The message names the file to edit. + +- **A legitimate Purpose that opens with "TBD" is reported** → accepted. A Purpose + whose first word is `TBD` is stating it was not written; reporting it is the + feature, not a false positive. + +- **The marker rule is a positional match on authored prose, against the project's + preference for explicit lookups** → confined to the one case where no list can + exist, and anchored at a single position so its behavior is enumerable. The + generated sentence, which *can* be looked up, is looked up. + +- **Wording of the generated placeholder changes later and old specs stop being + recognised by the constant** → the marker rule still catches them, since every + spelling used so far opens with `TBD`. + +- **Archive behavior changes unintentionally** → archive constructs its validators + without strict mode, so a warning cannot flip a rebuilt spec to invalid. Covered + by a test asserting the exact call archive makes. + +## Migration Plan + +None. No data, config, or spec files change. A project sees the new warning the +first time it validates after upgrading, and fixes it by writing the Purpose in +the main spec. + +## Open Questions + +- Should a `TODO` marker be treated the same as `TBD`? No tool or instruction + produces one today, so it is left out; adding it later is a one-line widening + that changes no scenario already written here. diff --git a/openspec/changes/warn-on-purpose-placeholder/proposal.md b/openspec/changes/warn-on-purpose-placeholder/proposal.md new file mode 100644 index 0000000000..7f5b1b161c --- /dev/null +++ b/openspec/changes/warn-on-purpose-placeholder/proposal.md @@ -0,0 +1,64 @@ +## Why + +When a delta introduces a capability without a usable `## Purpose`, archive writes +`TBD - created by archiving change . Update Purpose after archive.` into the +new main spec. Three places already tell authors to replace it — the `specs` +instruction ("including a leftover `TBD` placeholder — edit the main spec +directly"), the sync-specs summary step ("so it gets written now rather than +lingering"), and the archive contract itself — but nothing reports that it is +still there. + +`--strict` cannot reach it. The check meant to catch a Purpose nobody wrote is a +50-character floor, and the placeholder is 91 characters, so the one rule that +exists to catch a thin Purpose is satisfied by the exact text meaning "nobody +wrote one". A spec whose Purpose reads `Does stuff.` fails `--strict` today; a +spec whose Purpose says nothing at all passes. + +The result is a capability that carries a to-do indefinitely while every command +reports success, and a silent pass is indistinguishable from a clean run. +[#369](https://github.com/Fission-AI/OpenSpec/issues/369) reported agents leaving +the placeholder behind and stayed open for seven months; the remedies since have +been instructions, which is the mechanism that report described as unreliable. + +## What Changes + +- `openspec validate` reports a `## Purpose` that is still the archive + placeholder, as a warning on the spec's Purpose, naming the line to replace. +- The message says to edit the main spec directly, because a `## Purpose` in a + delta is read only when a capability is created and cannot replace an existing + one. +- Detection stays narrow: the sentence archive itself writes counts wherever it + appears in the Purpose, and otherwise only a `TBD` or `TODO` opening the + Purpose counts. A marker inside a sentence is authored prose and is left alone, + and so is anything inside a fenced code block, which is a Purpose quoting the + placeholder rather than carrying it. +- A Purpose reported as a placeholder is no longer also reported as too brief, so + a bare `TBD` yields one finding rather than two. +- Not breaking: the finding is a warning, so a project that already carries + placeholders keeps validating by default and only `--strict` fails. `openspec + archive` is unaffected — it validates rebuilt specs without `--strict`, so a + spec archive writes still passes the validation it would have passed before. + +## Capabilities + +### New Capabilities + +None. + +### Modified Capabilities + +- `cli-validate`: adds a requirement that spec validation report a Purpose left as + the archive placeholder, with the severity, detection boundary, and precedence + over the existing brevity warning stated as contract. + +## Impact + +- **Affected behavior**: `openspec validate` on main specs — `validate `, + `validate --specs`, and the bulk/interactive paths that share it. A project + carrying a placeholder sees a new warning; under `--strict` that project now + fails until the Purpose is written. +- **Unaffected**: `openspec archive`, which validates rebuilt specs non-strictly; + delta spec validation, which does not read a main spec's Purpose; and any spec + whose Purpose is authored prose. +- **Docs**: none required — the message carries its own remediation, and the + `specs` instruction already tells authors to edit the main spec directly. diff --git a/openspec/changes/warn-on-purpose-placeholder/specs/cli-validate/spec.md b/openspec/changes/warn-on-purpose-placeholder/specs/cli-validate/spec.md new file mode 100644 index 0000000000..0c00fe1b5b --- /dev/null +++ b/openspec/changes/warn-on-purpose-placeholder/specs/cli-validate/spec.md @@ -0,0 +1,112 @@ +## ADDED Requirements + +### Requirement: Spec validation SHALL report a Purpose left as the archive placeholder + +The `validate` command SHALL report, as a warning against the spec's Purpose, a +`## Purpose` that is still a placeholder rather than a Purpose someone wrote: +the sentence `openspec archive` writes for a new capability, or a marker left in +its place. The report SHALL name the line the placeholder is on when it can be +located, and SHALL omit the line rather than point at the wrong text when it +cannot. + +The remediation SHALL say to edit the main spec directly, because a `## Purpose` +in a delta is read only when a capability is created and therefore cannot replace +one that already exists. + +The finding SHALL be a warning. A project that already carries placeholders +therefore keeps validating by default, and only `--strict` fails — the +placeholder is worth keeping at the moment archive writes it, and worth reporting +once it has outlived that moment. + +Detection SHALL be narrow, because a Purpose is prose and prose that raises an +open question is not a placeholder: + +- the sentence archive itself writes SHALL be reported wherever it appears in the + Purpose, since nobody writes it by accident; +- otherwise only a `TBD` or `TODO` marker opening the Purpose SHALL be reported. + The two words SHALL be read the same way, because which one got typed says + nothing about whether the Purpose was written; +- a marker appearing inside a sentence SHALL NOT be reported; +- a longer word that merely begins with those letters SHALL NOT be reported, + in any script. + +Text inside a fenced code block SHALL NOT be read as the Purpose speaking, for +either rule. A Purpose that quotes the placeholder is documenting it rather than +carrying it, and a check that fails the document explaining the placeholder +teaches its readers to ignore the warning. + +An empty Purpose SHALL NOT be reported by this requirement, which the +empty-Purpose error already covers. A Purpose reported as a placeholder SHALL NOT +also be reported as too brief, so a bare `TBD` yields one finding and not two. + +Validation performed inside `openspec archive` SHALL be unaffected, because +archive validates a rebuilt spec without `--strict` and a warning does not change +that verdict: a spec archive writes SHALL still pass the validation it would have +passed before this requirement existed. + +#### Scenario: The placeholder passes by default and fails under strict + +- **GIVEN** a main spec whose Purpose is the placeholder archive wrote +- **WHEN** `openspec validate --specs` runs +- **THEN** report a warning against the Purpose, naming the line it is on and + saying to edit the main spec directly +- **AND** the spec is reported valid + +#### Scenario: Strict validation fails on the placeholder + +- **GIVEN** the same main spec +- **WHEN** `openspec validate --specs --strict` runs +- **THEN** the spec is reported invalid + +#### Scenario: An authored Purpose raising an open question is not reported + +- **GIVEN** a Purpose reading "Bounds how often a failed delivery is retried. The exact budget is TBD pending load tests." +- **WHEN** `openspec validate --specs --strict` runs +- **THEN** report no placeholder warning, because the marker does not open the Purpose +- **AND** the spec is reported valid + +#### Scenario: A Purpose left as a TODO is reported like a TBD + +- **GIVEN** a Purpose consisting only of "TODO" +- **WHEN** `openspec validate --specs --strict` runs +- **THEN** report the placeholder warning, the same finding a bare "TBD" reports + +#### Scenario: A Purpose quoting the placeholder inside a fence is not reported + +- **GIVEN** a Purpose that explains the placeholder and shows it inside a fenced + code block +- **WHEN** `openspec validate --specs --strict` runs +- **THEN** report no placeholder warning +- **AND** the spec is reported valid + +#### Scenario: A word beginning with the marker is not reported + +- **GIVEN** a Purpose opening "TBDs raised during design review are tracked in the linked issue.", or the same sentence opening with "TODOs" +- **WHEN** `openspec validate --specs --strict` runs +- **THEN** report no placeholder warning + +#### Scenario: A bare TBD is reported once + +- **GIVEN** a Purpose consisting only of "TBD" +- **WHEN** `openspec validate --specs --strict` runs +- **THEN** report exactly one finding against the Purpose, the placeholder warning + rather than the too-brief warning + +#### Scenario: A terse but authored Purpose still reports as too brief + +- **GIVEN** a Purpose reading "Does stuff." +- **WHEN** `openspec validate --specs --strict` runs +- **THEN** report the too-brief warning and no placeholder warning + +#### Scenario: Archive still writes the spec it would have written + +- **GIVEN** a change whose delta introduces a capability with no usable `## Purpose` +- **WHEN** `openspec archive` validates the rebuilt spec before writing it +- **THEN** that spec is reported valid and archive completes exactly as before + +#### Scenario: Line endings do not change what is reported + +- **GIVEN** two main specs with the same placeholder Purpose, one saved with LF + line endings and one with CRLF +- **WHEN** `openspec validate --specs` runs on each +- **THEN** both report the same warning against the same line number diff --git a/openspec/changes/warn-on-purpose-placeholder/tasks.md b/openspec/changes/warn-on-purpose-placeholder/tasks.md new file mode 100644 index 0000000000..0b82a3db3a --- /dev/null +++ b/openspec/changes/warn-on-purpose-placeholder/tasks.md @@ -0,0 +1,117 @@ +## 1. Name the generated placeholder + +- [x] 1.1 Extract the placeholder archive writes into a named constant, composed + of the fixed segments around the interpolated change name, so one + definition serves both the writer and the check +- [x] 1.2 Compose `buildSpecSkeleton`'s placeholder from that constant, and + confirm the existing archive tests still pass unchanged — the text written + to disk must be byte-identical to before + +## 2. Detection module + +- [x] 2.1 Add `src/core/validation/purpose-placeholder.ts`: a pure function that + takes the parsed Purpose plus the spec content and returns a finding or + nothing, following the shape of `task-numbering.ts` +- [x] 2.2 Recognise the generated sentence through the constant from 1.1, + wherever it appears in the Purpose +- [x] 2.3 Recognise a `TBD` marker opening the Purpose, excluding a longer word + that merely begins with those letters +- [x] 2.4 Return no finding for an empty Purpose, leaving it to the existing + empty-Purpose error +- [x] 2.5 Locate the first non-blank line of the `## Purpose` section for the + finding, normalising line endings first, and return the finding without a + line when the section cannot be located + +## 3. Wire it into validation + +- [x] 3.1 Add the warning message to `VALIDATION_MESSAGES`, naming the main spec + as the place to edit and why a delta cannot do it +- [x] 3.2 Call the check from `applySpecRules` so both `validateSpec` and + `validateSpecContent` are covered +- [x] 3.3 Run the brevity check only when the placeholder check does not fire, so + a bare `TBD` produces one finding + +## 4. Tests + +- [x] 4.1 Unit-test the module: the generated sentence, a bare `TBD`, a `TBD` + opening a longer sentence, mixed case, and the sentence appearing below an + authored line +- [x] 4.2 Unit-test what must stay silent: a `TBD` inside a sentence, a word + beginning with the marker, an empty Purpose, and an ordinary short Purpose +- [x] 4.3 Unit-test line location: text after blank lines, a Purpose section with + no body, and no content supplied +- [x] 4.4 Test through `Validator`: valid by default with one warning, invalid + under `--strict`, and an authored Purpose still passing `--strict` +- [x] 4.5 Test the gap this closes — the placeholder is over the length floor, so + assert it now fails `--strict` while a terse authored Purpose still fails + for brevity and not as a placeholder +- [x] 4.6 Test that a bare `TBD` yields exactly one finding against the Purpose +- [x] 4.7 Test the archive guarantee: the exact non-strict `validateSpecContent` + call archive makes still reports a placeholder spec as valid +- [x] 4.8 Test the real file path end to end, reading a spec off disk +- [x] 4.9 Test that a spec saved with CRLF endings reports the same warning and + the same line number as the LF version + +## 5. Verify + +- [x] 5.1 Run the full suite and confirm no existing test changes behavior — only + additions +- [x] 5.2 Run `openspec validate --specs --strict` on this repo and confirm it + still passes, including the two specs that mention `TBD` inside scenarios + rather than in a Purpose +- [x] 5.3 Run lint, typecheck, and the build +- [x] 5.4 Confirm the cross-platform CI matrix passes, since the check counts + lines in files that may carry either line ending + (green on linux-bash, macos-bash and windows-pwsh, plus lint & typecheck; + run by `workflow_dispatch` on the fork, so the upstream pull-request run + is still the gate that counts) +- [x] 5.5 Add a `.changeset/` entry describing the new warning, its severity, the + detection boundary, and that archive is unaffected + +## 6. Prove the tests hold the behaviour + +Added during implementation, not planned. A passing suite says the code works on +the cases someone thought to write; it does not say a guard is load-bearing. Two +were not, and only reverting them one at a time showed it. + +- [x] 6.1 Revert each guard in turn and record which tests die, so every guard is + known to be held by a test rather than assumed to be +- [x] 6.2 Fix the prefix/suffix test, which named the suffix guard but used a + Purpose containing neither half of the generated sentence — it passed + whether or not the suffix was required, so the mutation killed nothing +- [x] 6.3 Remove the empty-Purpose early return, which no test could hold: + neither rule matches empty text, so the branch changed no behaviour. + The requirement that an empty Purpose goes unreported is unchanged and + still asserted; it now falls out of the two rules instead of a third branch + +## 7. Answer the two questions the issue left open + +Issue #1670 asked whether the finding should be an error and whether `TODO` +should count. Warning stands, for the upgrade-safety reason in section 1. The rest is +what changed after review. + +- [x] 7.1 Read a `TODO` opening the Purpose as the same finding as a `TBD`. + Nothing OpenSpec writes produces one, but the marker an author leaves is + whichever word they reach for, and a Purpose reading `TODO: fill in` is as + unwritten as one reading `TBD`. The narrow rule is unchanged: only the + opening position counts, so `TODOs are tracked in the issue` and a `TODO` + raised mid-sentence are still authored prose +- [x] 7.2 Read fenced code in the Purpose as quoted material rather than as the + Purpose speaking, through the `buildCodeFenceMask` the requirement and + structure parsers already share. Without it a Purpose that documents the + placeholder is reported as being one, which fails the document that + explains the check to the person reading the check's output +- [x] 7.3 Skip fenced lines when locating the placeholder too, so a `## Purpose` + or `## Requirements` quoted in a fence can neither be mistaken for the + section header nor end the section early +- [x] 7.4 Widen the message to name both what archive writes and a marker left in + its place, since one message now covers both +- [x] 7.5 Mutation-check every new guard by reverting it in turn: dropping `TODO` + kills 3 tests, unmasking detection kills 2, unmasking the line locator + kills 3, and unmasking the header search kills 1 +- [x] 7.6 Make the marker boundary Unicode-aware, after review pointed out that + `\b` is ASCII and so read `TODOé` and `TBD١` as markers followed by + punctuation. A Purpose is prose, and prose is not always Latin script. + Held in both directions: loosening it back to `\b` kills 1 test, tightening + it to reject punctuation kills 4 + diff --git a/src/core/specs-apply.ts b/src/core/specs-apply.ts index f0a8ff3842..e97008d211 100644 --- a/src/core/specs-apply.ts +++ b/src/core/specs-apply.ts @@ -21,7 +21,11 @@ import { import { findMainSpecStructureIssues } from './parsers/spec-structure.js'; import { buildCodeFenceMask } from './parsers/code-fence.js'; import { MarkdownParser } from './parsers/markdown-parser.js'; -import { MIN_PURPOSE_LENGTH } from './validation/constants.js'; +import { + MIN_PURPOSE_LENGTH, + PURPOSE_PLACEHOLDER_PREFIX, + PURPOSE_PLACEHOLDER_SUFFIX, +} from './validation/constants.js'; import { discoverSpecFiles } from '../utils/spec-discovery.js'; import { FileSystemUtils } from '../utils/file-system.js'; @@ -1088,6 +1092,7 @@ function readableOverview(skeleton: string, specName: string): string | null { export function buildSpecSkeleton(specFolderName: string, changeName: string, purpose?: string): string { const titleBase = specFolderName; const purposeBody = - purpose?.trim() || `TBD - created by archiving change ${changeName}. Update Purpose after archive.`; + purpose?.trim() || + `${PURPOSE_PLACEHOLDER_PREFIX}${changeName}${PURPOSE_PLACEHOLDER_SUFFIX}`; return `# ${titleBase} Specification\n\n## Purpose\n${purposeBody}\n\n## Requirements\n`; } diff --git a/src/core/validation/constants.ts b/src/core/validation/constants.ts index d08cd47ebe..3c570255cd 100644 --- a/src/core/validation/constants.ts +++ b/src/core/validation/constants.ts @@ -11,6 +11,15 @@ export const MAX_WHY_SECTION_LENGTH = 1000; export const MAX_REQUIREMENT_TEXT_LENGTH = 500; export const MAX_DELTAS_PER_CHANGE = 10; +// The Purpose `openspec archive` writes into a main spec it creates when the +// delta introduced the capability without a usable `## Purpose`. Named here, and +// composed from these two halves at the write site, so validation recognises the +// placeholder through the same definition that produces it: a second, hand-copied +// spelling would stop matching the day the wording changed, and a check that +// matches nothing looks exactly like a check that found nothing. +export const PURPOSE_PLACEHOLDER_PREFIX = 'TBD - created by archiving change '; +export const PURPOSE_PLACEHOLDER_SUFFIX = '. Update Purpose after archive.'; + // Validation messages export const VALIDATION_MESSAGES = { // Required content @@ -38,6 +47,11 @@ export const VALIDATION_MESSAGES = { // Warnings PURPOSE_TOO_BRIEF: `Purpose section is too brief (less than ${MIN_PURPOSE_LENGTH} characters)`, + PURPOSE_IS_PLACEHOLDER: + 'Purpose section is still a placeholder rather than a Purpose anyone wrote (the sentence `openspec archive` ' + + 'writes for a new capability, or a `TBD`/`TODO` marker left in its place). Replace it with what this ' + + 'capability is for, editing the main spec directly: a `## Purpose` in a delta is read only when the ' + + 'capability is created, so it cannot replace this one.', REQUIREMENT_TOO_LONG: `Requirement text is very long (>${MAX_REQUIREMENT_TEXT_LENGTH} characters). Consider breaking it down.`, DELTA_DESCRIPTION_TOO_BRIEF: 'Delta description is too brief', DELTA_MISSING_REQUIREMENTS: 'Delta should include requirements', diff --git a/src/core/validation/purpose-placeholder.ts b/src/core/validation/purpose-placeholder.ts new file mode 100644 index 0000000000..210bef885e --- /dev/null +++ b/src/core/validation/purpose-placeholder.ts @@ -0,0 +1,140 @@ +import { buildCodeFenceMask } from '../parsers/code-fence.js'; +import { PURPOSE_PLACEHOLDER_PREFIX, PURPOSE_PLACEHOLDER_SUFFIX } from './constants.js'; + +/** + * Detects a `## Purpose` that is still a placeholder, rather than one somebody + * wrote. + * + * When a delta introduces a capability with no usable `## Purpose`, archive + * stamps the placeholder into the new main spec. That text is over + * `MIN_PURPOSE_LENGTH`, so the brevity check cannot reach it: the one rule that + * exists to catch a Purpose nobody wrote is satisfied by the exact string + * meaning nobody wrote one. Nothing else reads it afterwards, so the capability + * keeps a to-do in it while every command reports success. + * + * Two things count, and deliberately nothing else: + * + * - the placeholder this tool generates, recognised through the same constants + * the writer composes it from, wherever it sits in the Purpose - nobody types + * that sentence by accident; + * - a `TBD` or `TODO` **opening** the Purpose, which is the marker left behind + * when someone is told to leave "a brief TBD placeholder" and never comes + * back. Which of the two words got typed says nothing about whether the + * Purpose was written, so both are read the same way. + * + * A marker inside a sentence is left alone. "The retry budget is TBD pending + * benchmarks" is a real Purpose with an open question in it, and reporting it + * would teach people to ignore the warning - which costs more than the findings + * it would add. + * + * Fenced code inside the Purpose is quoted material rather than the Purpose + * speaking, so it is read out first. Without that, a Purpose documenting the + * sentence archive writes is reported as being that sentence: a document about + * the placeholder, failing for carrying one. + */ + +export interface PurposePlaceholderIssue { + /** 1-based line of the placeholder text, when it can be located. */ + line?: number; +} + +/** + * A `TBD` or `TODO` opening the Purpose. The lookahead keeps it off a longer + * word that merely begins with those letters, like "TBDs" or "TODOs", while + * still allowing the punctuation a marker is usually written with: `TODO:`, + * `TBD -`. It rejects any letter, digit or combining mark rather than only the + * ASCII ones `\b` knows about, because a Purpose is prose and prose is not + * always written in Latin script - `TBD` followed by an Arabic-Indic digit is + * as much a longer word as `TBDs` is. + */ +const LEADING_MARKER = /^(?:TBD|TODO)(?![\p{L}\p{N}\p{M}_])/iu; + +const PURPOSE_HEADER = /^ {0,3}##(?!#)[ \t]+Purpose[ \t]*$/i; +const TOP_LEVEL_HEADER = /^ {0,3}#{1,2}(?!#)[ \t]+/; + +/** + * The lines of `text` that sit outside a fenced code block, with line endings + * normalised first. + * + * `buildCodeFenceMask` is the masker the requirement and structure parsers + * already share, and its own reason for existing is that a second, private + * notion of what a fence is drifts from the first. This check reads Markdown + * for the same purpose they do, so it reads fences the same way they do. + */ +function unfencedLines(text: string): string[] { + const lines = text.replace(/\r\n?/g, '\n').split('\n'); + const fenced = buildCodeFenceMask(lines); + return lines.filter((_, index) => !fenced[index]); +} + +/** + * True when the text carries the sentence archive writes. Matched as its two + * fixed halves in order, because the change name between them varies - so the + * check follows the writer's own definition instead of a second copy of it. + */ +function containsGeneratedPlaceholder(text: string): boolean { + const prefixAt = text.indexOf(PURPOSE_PLACEHOLDER_PREFIX); + if (prefixAt === -1) return false; + return text.indexOf(PURPOSE_PLACEHOLDER_SUFFIX, prefixAt + PURPOSE_PLACEHOLDER_PREFIX.length) !== -1; +} + +/** + * Reports the Purpose of a main spec as an unwritten placeholder, or null when + * it reads as authored content. + * + * An empty Purpose is not reported here - `SPEC_PURPOSE_EMPTY` already covers + * it, and reporting both would put two findings on one line. That falls out of + * the two rules rather than needing a case of its own. + */ +export function findPurposePlaceholderIssue( + overview: string, + content?: string +): PurposePlaceholderIssue | null { + // An empty Purpose needs no branch of its own: neither rule matches empty + // text, so it falls through to null on the line below. An early return for it + // would be a guard no test could hold, which is worse than none. A Purpose + // that is nothing but a fenced block reduces to the same empty text here, and + // is left to the brevity and empty-Purpose rules for the same reason. + const prose = unfencedLines(overview).join('\n').trim(); + const leading = LEADING_MARKER.test(prose); + if (!leading && !containsGeneratedPlaceholder(prose)) return null; + // Which rule matched decides where the placeholder is, so the locator is told. + // When both match the leading marker wins: it sits at or above the generated + // sentence, and the earliest marker is the one a reader scanning down meets. + return { line: content === undefined ? undefined : findPlaceholderLine(content, leading) }; +} + +/** + * The line inside the `## Purpose` section carrying the placeholder, so the + * warning points at the text to replace rather than at the file. + * + * Which line that is depends on the rule that matched. A leading marker is the + * section's first non-blank line by definition. The generated sentence is not: + * it can follow prose somebody wrote, and naming the first non-blank line then + * points at that prose - a line the reader can see is fine, which reads as the + * check being wrong rather than the Purpose being unwritten. + * + * Fenced lines are skipped on the way, for the reason detection skips them, and + * so a `## Requirements` quoted inside a fence cannot end the section early. + * + * Undefined when the placeholder cannot be located - no section header, or a + * generated sentence no single line carries. The caller then reports the finding + * without a line rather than with a guessed one, since a wrong line number is + * worse than none. + * + * Line endings are normalised first, so the same spec reports the same line + * whether it was saved on Windows or on macOS/Linux. + */ +function findPlaceholderLine(content: string, leading: boolean): number | undefined { + const lines = content.replace(/\r\n?/g, '\n').split('\n'); + const fenced = buildCodeFenceMask(lines); + const headerIndex = lines.findIndex((line, index) => !fenced[index] && PURPOSE_HEADER.test(line)); + if (headerIndex === -1) return undefined; + + for (let i = headerIndex + 1; i < lines.length; i++) { + if (fenced[i]) continue; + if (TOP_LEVEL_HEADER.test(lines[i])) return undefined; + if (leading ? lines[i].trim() : lines[i].includes(PURPOSE_PLACEHOLDER_PREFIX)) return i + 1; + } + return undefined; +} diff --git a/src/core/validation/validator.ts b/src/core/validation/validator.ts index 56f771e1a8..2bcb75cc57 100644 --- a/src/core/validation/validator.ts +++ b/src/core/validation/validator.ts @@ -33,6 +33,7 @@ import { } from '../../utils/change-metadata.js'; import { resolveTaskFilesForChange } from '../../utils/task-progress.js'; import { findTaskNumberingIssues } from './task-numbering.js'; +import { findPurposePlaceholderIssue } from './purpose-placeholder.js'; import { getPackageSchemasDir, getSchemaDir } from '../artifact-graph/index.js'; export class Validator { @@ -649,7 +650,20 @@ export class Validator { }); } - if (spec.overview.length < MIN_PURPOSE_LENGTH) { + // The placeholder is longer than MIN_PURPOSE_LENGTH, so the brevity check + // below cannot reach it; it is reported on its own terms instead. Checked + // first because a hand-written "TBD" is both a placeholder and too brief, + // and only one of those two tells the author what to do. (A "TODO" opening + // the Purpose reads the same way, so it is the same finding.) + const placeholder = findPurposePlaceholderIssue(spec.overview, content); + if (placeholder) { + issues.push({ + level: 'WARNING', + path: 'overview', + line: placeholder.line, + message: VALIDATION_MESSAGES.PURPOSE_IS_PLACEHOLDER, + }); + } else if (spec.overview.length < MIN_PURPOSE_LENGTH) { issues.push({ level: 'WARNING', path: 'overview', diff --git a/test/core/purpose-placeholder.test.ts b/test/core/purpose-placeholder.test.ts new file mode 100644 index 0000000000..1d074c2f68 --- /dev/null +++ b/test/core/purpose-placeholder.test.ts @@ -0,0 +1,308 @@ +import { describe, expect, it } from 'vitest'; +import { findPurposePlaceholderIssue } from '../../src/core/validation/purpose-placeholder.js'; +import { + PURPOSE_PLACEHOLDER_PREFIX, + PURPOSE_PLACEHOLDER_SUFFIX, +} from '../../src/core/validation/constants.js'; + +/** Built the way archive builds it, so the test cannot drift from the writer. */ +const placeholderFor = (changeName: string) => + `${PURPOSE_PLACEHOLDER_PREFIX}${changeName}${PURPOSE_PLACEHOLDER_SUFFIX}`; + +const ARCHIVE_TEXT = placeholderFor('add-retry-budget'); + +const specWith = (purpose: string) => + [ + '# widgets Specification', + '', + '## Purpose', + purpose, + '', + '## Requirements', + '### Requirement: Retries are bounded', + 'The system SHALL stop retrying a delivery after the configured budget.', + '', + '#### Scenario: Budget exhausted', + '- **WHEN** the budget is exhausted', + '- **THEN** the delivery is abandoned', + '', + ].join('\n'); + +describe('findPurposePlaceholderIssue', () => { + describe('reports a placeholder', () => { + it('reports the sentence archive writes, and points at it', () => { + expect(findPurposePlaceholderIssue(ARCHIVE_TEXT, specWith(ARCHIVE_TEXT))).toEqual({ + line: 4, + }); + }); + + it('reports it for any change name, since the name is what varies', () => { + const purpose = placeholderFor('2026-01-15-my-change'); + expect(findPurposePlaceholderIssue(purpose, specWith(purpose))).toEqual({ line: 4 }); + }); + + it('reports a bare TBD an agent wrote instead of the archive wording', () => { + expect(findPurposePlaceholderIssue('TBD', specWith('TBD'))).toEqual({ line: 4 }); + }); + + it('reports a TBD opening a longer placeholder sentence', () => { + const purpose = 'TBD: fill this in once the capability settles down.'; + expect(findPurposePlaceholderIssue(purpose, specWith(purpose))).toEqual({ line: 4 }); + }); + + it('ignores case in the leading marker', () => { + const purpose = 'tbd - write this later'; + expect(findPurposePlaceholderIssue(purpose, specWith(purpose))).not.toBeNull(); + }); + + it('reports a bare TODO, the other word for the same non-answer', () => { + expect(findPurposePlaceholderIssue('TODO', specWith('TODO'))).toEqual({ line: 4 }); + }); + + it('reports a TODO opening a longer placeholder sentence', () => { + const purpose = 'TODO: describe what this capability is for.'; + expect(findPurposePlaceholderIssue(purpose, specWith(purpose))).toEqual({ line: 4 }); + }); + + it('ignores case in a leading TODO too', () => { + const purpose = 'todo - write this later'; + expect(findPurposePlaceholderIssue(purpose, specWith(purpose))).not.toBeNull(); + }); + + it('reports the archive sentence even when it does not open the Purpose', () => { + // Someone typed a line above the placeholder and left it in place. The + // sentence is archive's own output wherever it sits, so it still counts. + const purpose = `Handles widget retries.\n\n${ARCHIVE_TEXT}`; + expect(findPurposePlaceholderIssue(purpose, specWith(purpose))).not.toBeNull(); + }); + + it('names the placeholder line, not the prose above it', () => { + // The warning says "this Purpose is still the placeholder". Pointing at a + // line the reader can see is fine reads as the check being wrong. + const purpose = `Handles widget retries.\n\n${ARCHIVE_TEXT}`; + expect(findPurposePlaceholderIssue(purpose, specWith(purpose))).toEqual({ line: 6 }); + }); + + it('reports a placeholder the author padded with whitespace', () => { + expect(findPurposePlaceholderIssue(' TBD ', specWith(' TBD '))).not.toBeNull(); + }); + }); + + describe('leaves authored prose alone', () => { + it('does not report a Purpose that raises an open question mid-sentence', () => { + const purpose = + 'Bounds how often a failed delivery is retried. The exact retry budget is TBD pending the load tests.'; + expect(findPurposePlaceholderIssue(purpose, specWith(purpose))).toBeNull(); + }); + + it('does not report a word that merely starts with the marker', () => { + const purpose = 'TBDs raised during design review are tracked in the linked issue.'; + expect(findPurposePlaceholderIssue(purpose, specWith(purpose))).toBeNull(); + }); + + it('does not report a word that merely starts with TODO', () => { + const purpose = 'TODOs raised during design review are tracked in the linked issue.'; + expect(findPurposePlaceholderIssue(purpose, specWith(purpose))).toBeNull(); + }); + + it('does not report a longer word in a script `\\b` cannot see', () => { + // A Purpose is prose, and prose is not always Latin script. `\\b` is ASCII, + // so it reads the boundary between "TODO" and any non-ASCII letter as the + // end of the marker and reports a word nobody meant as one. + for (const purpose of ['TODOé is a word here, not a marker.', 'TBD١ names the first budget.']) { + expect(findPurposePlaceholderIssue(purpose, specWith(purpose))).toBeNull(); + } + }); + + it('still reads the punctuation a marker is written with', () => { + // The boundary must reject letters without rejecting `TODO:` or `TBD -`, + // which is how the marker actually gets typed. + for (const purpose of ['TODO(owner): describe this.', 'TBD.', 'TBD, pending a rewrite.']) { + expect(findPurposePlaceholderIssue(purpose, specWith(purpose))).not.toBeNull(); + } + }); + + it('does not report a TODO raised mid-sentence', () => { + const purpose = + 'Bounds how often a failed delivery is retried. Tuning the budget is a TODO for the load tests.'; + expect(findPurposePlaceholderIssue(purpose, specWith(purpose))).toBeNull(); + }); + + it('does not report the generated sentence when only its opening half is present', () => { + // Quoting the placeholder's opening is not carrying the placeholder. The + // suffix has to follow the prefix, or a Purpose that documents the message + // archive writes would be reported as being that message. + const purpose = `Explains the ${PURPOSE_PLACEHOLDER_PREFIX} message archive writes, and how to replace it.`; + expect(purpose).toContain(PURPOSE_PLACEHOLDER_PREFIX); + expect(purpose).not.toContain(PURPOSE_PLACEHOLDER_SUFFIX); + + expect(findPurposePlaceholderIssue(purpose, specWith(purpose))).toBeNull(); + }); + + it('does not report an empty Purpose, which SPEC_PURPOSE_EMPTY already covers', () => { + expect(findPurposePlaceholderIssue('', specWith(''))).toBeNull(); + expect(findPurposePlaceholderIssue(' \n ', specWith(''))).toBeNull(); + }); + + it('does not report an ordinary short Purpose, which PURPOSE_TOO_BRIEF covers', () => { + expect(findPurposePlaceholderIssue('Does stuff.', specWith('Does stuff.'))).toBeNull(); + }); + }); + + describe('reads fenced code as quoted material, not as the Purpose', () => { + const fenced = (body: string) => ['```', body, '```'].join('\n'); + + it('does not report a Purpose that quotes the generated sentence in a fence', () => { + // A spec documenting the placeholder carries the sentence without being + // it. Reporting that is the check failing the one document that explains + // what it is for. + const purpose = [ + 'Documents the Purpose `openspec archive` writes for a new capability:', + '', + fenced(ARCHIVE_TEXT), + ].join('\n'); + + expect(findPurposePlaceholderIssue(purpose, specWith(purpose))).toBeNull(); + }); + + it('does not report a fenced marker that opens the fence but not the Purpose', () => { + const purpose = ['Shows the marker an unfinished spec carries:', '', fenced('TBD')].join('\n'); + + expect(findPurposePlaceholderIssue(purpose, specWith(purpose))).toBeNull(); + }); + + it('does not report a tilde fence either, since both spellings are fences', () => { + const purpose = ['Documents the placeholder:', '', '~~~', ARCHIVE_TEXT, '~~~'].join('\n'); + + expect(findPurposePlaceholderIssue(purpose, specWith(purpose))).toBeNull(); + }); + + it('still reports a placeholder that sits outside the fence', () => { + // The exemption is for what a fence contains, not for a Purpose that + // happens to contain a fence. + const purpose = [fenced('an unrelated example'), '', ARCHIVE_TEXT].join('\n'); + + expect(findPurposePlaceholderIssue(purpose, specWith(purpose))).toEqual({ line: 8 }); + }); + + it('names the unfenced line, skipping a fenced copy above it', () => { + const purpose = [fenced(ARCHIVE_TEXT), '', ARCHIVE_TEXT].join('\n'); + + expect(findPurposePlaceholderIssue(purpose, specWith(purpose))).toEqual({ line: 8 }); + }); + + it('finds the real Purpose header, not one quoted in a fence above it', () => { + // A spec whose preamble shows what a spec looks like has a `## Purpose` + // in it that is not this spec's. Starting the scan there walks straight + // into the real header and reports no line at all. + const content = [ + '# widgets Specification', + '', + 'Every capability spec opens like this:', + '', + '```', + '## Purpose', + 'TBD', + '```', + '', + '## Purpose', + ARCHIVE_TEXT, + '', + '## Requirements', + '### Requirement: Retries are bounded', + 'The system SHALL stop retrying a delivery after the configured budget.', + '', + '#### Scenario: Budget exhausted', + '- **WHEN** the budget is exhausted', + '- **THEN** the delivery is abandoned', + '', + ].join('\n'); + + expect(findPurposePlaceholderIssue(ARCHIVE_TEXT, content)).toEqual({ line: 11 }); + }); + + it('does not let a fenced heading end the Purpose section early', () => { + // `## Requirements` inside a fence is an example of a spec, not the start + // of this one's requirements - so the locator must read past it. + const purpose = [fenced('## Requirements'), '', ARCHIVE_TEXT].join('\n'); + + expect(findPurposePlaceholderIssue(purpose, specWith(purpose))).toEqual({ line: 8 }); + }); + }); + + describe('locating the placeholder', () => { + it('skips blank lines between the header and the text', () => { + const content = ['# widgets Specification', '', '## Purpose', '', '', 'TBD', ''].join('\n'); + expect(findPurposePlaceholderIssue('TBD', content)).toEqual({ line: 6 }); + }); + + it('counts lines the same way with CRLF endings', () => { + const content = specWith(ARCHIVE_TEXT).replace(/\n/g, '\r\n'); + expect(findPurposePlaceholderIssue(ARCHIVE_TEXT, content)).toEqual({ line: 4 }); + }); + + it('reports without a line rather than guessing when the section is empty', () => { + // The parsed overview and the file disagree; report the finding, not a + // line number pointing at the wrong text. + const content = ['# widgets Specification', '', '## Purpose', '', '## Requirements', ''].join( + '\n' + ); + expect(findPurposePlaceholderIssue('TBD', content)).toEqual({ line: undefined }); + }); + + it('reports without a line when no content is supplied', () => { + expect(findPurposePlaceholderIssue('TBD')).toEqual({ line: undefined }); + }); + }); + + // Every position a placeholder can occupy in a Purpose, and the line the + // warning should name for it. `specWith` puts the Purpose body at line 4, so + // the expected line is 4 plus however many lines precede the placeholder + // inside the body. + describe('the reported line follows the placeholder, wherever it sits', () => { + const cases: Array<{ name: string; purpose: string; line: number }> = [ + { + name: 'generated sentence alone — the shape archive writes', + purpose: ARCHIVE_TEXT, + line: 4, + }, + { + name: 'generated sentence one blank line below a sentence of prose', + purpose: `Handles widget retries.\n\n${ARCHIVE_TEXT}`, + line: 6, + }, + { + name: 'generated sentence below two lines of prose', + purpose: `Handles widget retries.\nAcross every transport.\n\n${ARCHIVE_TEXT}`, + line: 7, + }, + { + name: 'bare TBD an agent left behind', + purpose: 'TBD', + line: 4, + }, + { + name: 'both markers — the earliest is what a reader meets first', + purpose: `TBD, pending a rewrite.\n\n${ARCHIVE_TEXT}`, + line: 4, + }, + { + name: 'generated sentence rewrapped across two lines by a formatter', + purpose: 'TBD - created by archiving\nchange c1. Update Purpose after archive.', + line: 4, + }, + ]; + + for (const { name, purpose, line } of cases) { + it(name, () => { + const content = specWith(purpose); + // Guard the fixture itself: the expected line must really carry the + // placeholder, or the test would pin a number rather than a behaviour. + const onThatLine = content.split('\n')[line - 1]; + expect(onThatLine.startsWith('TBD')).toBe(true); + + expect(findPurposePlaceholderIssue(purpose, content)).toEqual({ line }); + }); + } + }); +}); diff --git a/test/core/validation.purpose-placeholder.test.ts b/test/core/validation.purpose-placeholder.test.ts new file mode 100644 index 0000000000..1f64e3c664 --- /dev/null +++ b/test/core/validation.purpose-placeholder.test.ts @@ -0,0 +1,197 @@ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { promises as fs } from 'fs'; +import os from 'os'; +import path from 'path'; +import { Validator } from '../../src/core/validation/validator.js'; +import { + MIN_PURPOSE_LENGTH, + PURPOSE_PLACEHOLDER_PREFIX, + PURPOSE_PLACEHOLDER_SUFFIX, + VALIDATION_MESSAGES, +} from '../../src/core/validation/constants.js'; + +const ARCHIVE_TEXT = `${PURPOSE_PLACEHOLDER_PREFIX}add-retry-budget${PURPOSE_PLACEHOLDER_SUFFIX}`; + +const specWith = (purpose: string) => + [ + '# widgets Specification', + '', + '## Purpose', + purpose, + '', + '## Requirements', + '### Requirement: Retries are bounded', + 'The system SHALL stop retrying a delivery after the configured budget.', + '', + '#### Scenario: Budget exhausted', + '- **WHEN** the budget is exhausted', + '- **THEN** the delivery is abandoned', + '', + ].join('\n'); + +const REAL_PURPOSE = + 'Bounds how often a failed delivery is retried, so a permanently broken endpoint cannot occupy the worker pool forever.'; + +const purposeIssues = (issues: Array<{ path: string }>) => + issues.filter((issue) => issue.path === 'overview'); + +describe('Purpose placeholder validation', () => { + describe('severity is what --strict is for', () => { + it('passes by default, so a project carrying one keeps validating', async () => { + const report = await new Validator().validateSpecContent('widgets', specWith(ARCHIVE_TEXT)); + + expect(report.valid).toBe(true); + expect(report.summary.errors).toBe(0); + expect(report.summary.warnings).toBe(1); + }); + + it('fails under --strict, naming the placeholder and where it is', async () => { + const report = await new Validator(true).validateSpecContent( + 'widgets', + specWith(ARCHIVE_TEXT) + ); + + expect(report.valid).toBe(false); + expect(report.issues).toContainEqual({ + level: 'WARNING', + path: 'overview', + line: 4, + message: VALIDATION_MESSAGES.PURPOSE_IS_PLACEHOLDER, + }); + }); + + it('leaves an authored Purpose passing --strict', async () => { + const report = await new Validator(true).validateSpecContent( + 'widgets', + specWith(REAL_PURPOSE) + ); + + expect(report.valid).toBe(true); + expect(report.summary.warnings).toBe(0); + }); + }); + + describe('the gap this closes', () => { + it('is the case --strict could not reach: the placeholder outruns the length floor', async () => { + // The check that exists to catch a thin Purpose is a length floor, and the + // placeholder clears it - so before this rule the spec saying "nobody wrote + // a Purpose" passed --strict while a real but terse one failed. Both now + // fail, each for the reason that fits it. + expect(ARCHIVE_TEXT.length).toBeGreaterThan(MIN_PURPOSE_LENGTH); + + const placeholder = await new Validator(true).validateSpecContent( + 'widgets', + specWith(ARCHIVE_TEXT) + ); + const terse = await new Validator(true).validateSpecContent( + 'widgets', + specWith('Does stuff.') + ); + + expect(placeholder.valid).toBe(false); + expect(placeholder.issues.map((i) => i.message)).toContain( + VALIDATION_MESSAGES.PURPOSE_IS_PLACEHOLDER + ); + + expect(terse.valid).toBe(false); + expect(terse.issues.map((i) => i.message)).toContain(VALIDATION_MESSAGES.PURPOSE_TOO_BRIEF); + expect(terse.issues.map((i) => i.message)).not.toContain( + VALIDATION_MESSAGES.PURPOSE_IS_PLACEHOLDER + ); + }); + + it('reports a bare TBD once, as a placeholder rather than as too brief', async () => { + const report = await new Validator(true).validateSpecContent('widgets', specWith('TBD')); + + const found = purposeIssues(report.issues); + expect(found).toHaveLength(1); + expect(found[0].message).toBe(VALIDATION_MESSAGES.PURPOSE_IS_PLACEHOLDER); + }); + + it('reports a bare TODO the same way, since it is the same non-answer', async () => { + const report = await new Validator(true).validateSpecContent('widgets', specWith('TODO')); + + const found = purposeIssues(report.issues); + expect(found).toHaveLength(1); + expect(found[0].message).toBe(VALIDATION_MESSAGES.PURPOSE_IS_PLACEHOLDER); + }); + }); + + describe('a Purpose that documents the placeholder is not one', () => { + it('passes --strict while quoting the sentence archive writes inside a fence', async () => { + // OpenSpec's own docs are the population most likely to quote this text. + // A check that fails the document explaining what the placeholder is + // teaches people that the warning is noise, which costs more than the one + // finding it adds. + const purpose = [ + 'Documents the Purpose `openspec archive` writes for a capability a delta introduced', + 'without one, and what to replace it with:', + '', + '```', + ARCHIVE_TEXT, + '```', + ].join('\n'); + + const report = await new Validator(true).validateSpecContent('widgets', specWith(purpose)); + + expect(report.valid).toBe(true); + expect(purposeIssues(report.issues)).toEqual([]); + }); + }); + + describe('archive is unaffected', () => { + it('still reports a placeholder spec as valid to the validator archive runs', async () => { + // `openspec archive` validates every rebuilt spec before writing it, with a + // non-strict Validator. A spec archive writes must not fail validation it + // would have passed before, so the new rule has to stay a warning on + // exactly this call. + const report = await new Validator().validateSpecContent('widgets', specWith(ARCHIVE_TEXT)); + + expect(report.valid).toBe(true); + }); + }); + + describe('the real file path', () => { + let dir: string; + + beforeEach(async () => { + dir = await fs.mkdtemp(path.join(os.tmpdir(), 'openspec-purpose-')); + }); + + afterEach(async () => { + await fs.rm(dir, { recursive: true, force: true }); + }); + + it('reports a placeholder read off disk', async () => { + const file = path.join(dir, 'spec.md'); + await fs.writeFile(file, specWith(ARCHIVE_TEXT), 'utf-8'); + + const report = await new Validator(true).validateSpec(file); + + expect(report.valid).toBe(false); + expect(report.issues.map((i) => i.message)).toContain( + VALIDATION_MESSAGES.PURPOSE_IS_PLACEHOLDER + ); + }); + + it('reports the same line for a spec saved with CRLF endings', async () => { + const lf = path.join(dir, 'lf.md'); + const crlf = path.join(dir, 'crlf.md'); + await fs.writeFile(lf, specWith(ARCHIVE_TEXT), 'utf-8'); + await fs.writeFile(crlf, specWith(ARCHIVE_TEXT).replace(/\n/g, '\r\n'), 'utf-8'); + + const lfReport = await new Validator().validateSpec(lf); + const crlfReport = await new Validator().validateSpec(crlf); + + expect(purposeIssues(crlfReport.issues)).toEqual(purposeIssues(lfReport.issues)); + expect(purposeIssues(crlfReport.issues)).toEqual([ + { + level: 'WARNING', + path: 'overview', + line: 4, + message: VALIDATION_MESSAGES.PURPOSE_IS_PLACEHOLDER, + }, + ]); + }); + }); +});