From d064fd2b3205f3d9d0f9aa4d22ec1128ac0c5cad Mon Sep 17 00:00:00 2001 From: kitimark Date: Sat, 15 Aug 2026 12:57:50 +0700 Subject: [PATCH 1/9] docs(openspec): propose warn-on-purpose-placeholder When a delta introduces a capability with no 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"), the sync-specs summary step ("so it gets written now rather than lingering"), and the cli-archive contract -- 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 Purpose reading "Does stuff." fails --strict today, while one saying nothing at all passes. Proposes reporting it as a warning against the spec's Purpose -- silent by default, failing under --strict, so a project already carrying placeholders keeps validating until it opts into the stricter gate. Detection is narrow: the generated sentence wherever it appears, and otherwise only a `TBD` opening the Purpose, so prose raising an open question is left alone. Planning artifacts only; no source changes. Refs #369 Co-Authored-By: Claude Opus 5 (1M context) --- .../.openspec.yaml | 2 + .../warn-on-purpose-placeholder/design.md | 147 ++++++++++++++++++ .../warn-on-purpose-placeholder/proposal.md | 62 ++++++++ .../specs/cli-validate/spec.md | 89 +++++++++++ .../warn-on-purpose-placeholder/tasks.md | 66 ++++++++ 5 files changed, 366 insertions(+) create mode 100644 openspec/changes/warn-on-purpose-placeholder/.openspec.yaml create mode 100644 openspec/changes/warn-on-purpose-placeholder/design.md create mode 100644 openspec/changes/warn-on-purpose-placeholder/proposal.md create mode 100644 openspec/changes/warn-on-purpose-placeholder/specs/cli-validate/spec.md create mode 100644 openspec/changes/warn-on-purpose-placeholder/tasks.md 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..24a6963021 --- /dev/null +++ b/openspec/changes/warn-on-purpose-placeholder/design.md @@ -0,0 +1,147 @@ +## 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. + +### 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 is best-effort + +The warning names the first non-blank line of the `## Purpose` section. When the +section cannot be located — the parsed Purpose and the file disagree — 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..280d39d5f3 --- /dev/null +++ b/openspec/changes/warn-on-purpose-placeholder/proposal.md @@ -0,0 +1,62 @@ +## 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` opening the Purpose counts. + A `TBD` inside a sentence is authored prose and is left alone. +- 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..c4152e6b37 --- /dev/null +++ b/openspec/changes/warn-on-purpose-placeholder/specs/cli-validate/spec.md @@ -0,0 +1,89 @@ +## 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 the placeholder `openspec archive` writes for a new +capability rather than a Purpose someone wrote. 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` marker opening the Purpose SHALL be reported; +- a `TBD` appearing inside a sentence SHALL NOT be reported; +- a word that merely begins with those letters SHALL NOT be reported. + +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 word beginning with the marker is not reported + +- **GIVEN** a Purpose opening "TBDs raised during design review are tracked in the linked issue." +- **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..ea3757fb15 --- /dev/null +++ b/openspec/changes/warn-on-purpose-placeholder/tasks.md @@ -0,0 +1,66 @@ +## 1. Name the generated placeholder + +- [ ] 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 +- [ ] 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 + +- [ ] 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` +- [ ] 2.2 Recognise the generated sentence through the constant from 1.1, + wherever it appears in the Purpose +- [ ] 2.3 Recognise a `TBD` marker opening the Purpose, excluding a longer word + that merely begins with those letters +- [ ] 2.4 Return no finding for an empty Purpose, leaving it to the existing + empty-Purpose error +- [ ] 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 + +- [ ] 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 +- [ ] 3.2 Call the check from `applySpecRules` so both `validateSpec` and + `validateSpecContent` are covered +- [ ] 3.3 Run the brevity check only when the placeholder check does not fire, so + a bare `TBD` produces one finding + +## 4. Tests + +- [ ] 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 +- [ ] 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 +- [ ] 4.3 Unit-test line location: text after blank lines, a Purpose section with + no body, and no content supplied +- [ ] 4.4 Test through `Validator`: valid by default with one warning, invalid + under `--strict`, and an authored Purpose still passing `--strict` +- [ ] 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 +- [ ] 4.6 Test that a bare `TBD` yields exactly one finding against the Purpose +- [ ] 4.7 Test the archive guarantee: the exact non-strict `validateSpecContent` + call archive makes still reports a placeholder spec as valid +- [ ] 4.8 Test the real file path end to end, reading a spec off disk +- [ ] 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 + +- [ ] 5.1 Run the full suite and confirm no existing test changes behavior — only + additions +- [ ] 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 +- [ ] 5.3 Run lint, typecheck, and the build +- [ ] 5.4 Confirm the cross-platform CI matrix passes, since the check counts + lines in files that may carry either line ending +- [ ] 5.5 Add a `.changeset/` entry describing the new warning, its severity, the + detection boundary, and that archive is unaffected From f81db746dbb65bda30d8e7f121a7c498e59b00c9 Mon Sep 17 00:00:00 2001 From: kitimark Date: Sat, 15 Aug 2026 13:10:57 +0700 Subject: [PATCH 2/9] fix(validate): report a Purpose left as the archive placeholder When a delta introduces a capability with no usable `## Purpose`, archive writes a placeholder into the new main spec. Nothing read it afterwards, so the capability kept a to-do in it while every command reported success. `--strict` could not reach it. The check that exists to catch a Purpose nobody wrote is a 50-character floor, and the placeholder clears it: a spec whose Purpose read "Does stuff." failed --strict, while a spec whose Purpose said nothing at all passed. #369 reported agents leaving the placeholder behind and stayed open seven months; every remedy since has been an instruction, which is the mechanism that report described as unreliable. validate now reports it as a warning against the Purpose, naming the line and saying to edit the main spec directly -- a delta's `## Purpose` is read only when the capability is created, so it cannot replace an existing one. Warning rather than error, because strict mode already means "warnings fail": a project carrying placeholders keeps validating by default and only --strict fails. Archive is untouched -- 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 byte-identical. The placeholder is recognised through the same constants the writer composes it from, so the check cannot drift from the sentence it looks for -- the failure mode of a second, hand-copied spelling being a check that matches nothing and looks exactly like a check that found nothing. The one case that cannot be a lookup is an agent-written placeholder, kept to a `TBD` opening the Purpose: "the retry budget is TBD pending benchmarks" is authored prose and is left alone. Verified: 209 archive tests pass unchanged (the placeholder text is asserted literally, so the output is provably identical); full suite 138 files / 3993 tests; 36/36 strict spec validations; build, lint and typecheck clean. Against a project carrying four real placeholders, default mode still exits 0 and --strict fails exactly those four. Cross-platform CI is not yet confirmed -- it needs a pushed branch. Line endings are covered by tests asserting a CRLF spec and an LF spec produce identical findings, and the module does no path handling. Refs #369 Co-Authored-By: Claude Opus 5 (1M context) --- .../validate-reports-purpose-placeholder.md | 11 ++ .../warn-on-purpose-placeholder/tasks.md | 46 ++--- src/core/specs-apply.ts | 9 +- src/core/validation/constants.ts | 13 ++ src/core/validation/purpose-placeholder.ts | 86 +++++++++ src/core/validation/validator.ts | 15 +- test/core/purpose-placeholder.test.ts | 123 +++++++++++++ .../validation.purpose-placeholder.test.ts | 167 ++++++++++++++++++ 8 files changed, 444 insertions(+), 26 deletions(-) create mode 100644 .changeset/validate-reports-purpose-placeholder.md create mode 100644 src/core/validation/purpose-placeholder.ts create mode 100644 test/core/purpose-placeholder.test.ts create mode 100644 test/core/validation.purpose-placeholder.test.ts diff --git a/.changeset/validate-reports-purpose-placeholder.md b/.changeset/validate-reports-purpose-placeholder.md new file mode 100644 index 0000000000..44f74264bd --- /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` 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. 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/tasks.md b/openspec/changes/warn-on-purpose-placeholder/tasks.md index ea3757fb15..7c342023e7 100644 --- a/openspec/changes/warn-on-purpose-placeholder/tasks.md +++ b/openspec/changes/warn-on-purpose-placeholder/tasks.md @@ -1,66 +1,66 @@ ## 1. Name the generated placeholder -- [ ] 1.1 Extract the placeholder archive writes into a named constant, composed +- [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 -- [ ] 1.2 Compose `buildSpecSkeleton`'s placeholder from that constant, and +- [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 -- [ ] 2.1 Add `src/core/validation/purpose-placeholder.ts`: a pure function that +- [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` -- [ ] 2.2 Recognise the generated sentence through the constant from 1.1, +- [x] 2.2 Recognise the generated sentence through the constant from 1.1, wherever it appears in the Purpose -- [ ] 2.3 Recognise a `TBD` marker opening the Purpose, excluding a longer word +- [x] 2.3 Recognise a `TBD` marker opening the Purpose, excluding a longer word that merely begins with those letters -- [ ] 2.4 Return no finding for an empty Purpose, leaving it to the existing +- [x] 2.4 Return no finding for an empty Purpose, leaving it to the existing empty-Purpose error -- [ ] 2.5 Locate the first non-blank line of the `## Purpose` section for the +- [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 -- [ ] 3.1 Add the warning message to `VALIDATION_MESSAGES`, naming the main spec +- [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 -- [ ] 3.2 Call the check from `applySpecRules` so both `validateSpec` and +- [x] 3.2 Call the check from `applySpecRules` so both `validateSpec` and `validateSpecContent` are covered -- [ ] 3.3 Run the brevity check only when the placeholder check does not fire, so +- [x] 3.3 Run the brevity check only when the placeholder check does not fire, so a bare `TBD` produces one finding ## 4. Tests -- [ ] 4.1 Unit-test the module: the generated sentence, a bare `TBD`, a `TBD` +- [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 -- [ ] 4.2 Unit-test what must stay silent: a `TBD` inside a sentence, a word +- [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 -- [ ] 4.3 Unit-test line location: text after blank lines, a Purpose section with +- [x] 4.3 Unit-test line location: text after blank lines, a Purpose section with no body, and no content supplied -- [ ] 4.4 Test through `Validator`: valid by default with one warning, invalid +- [x] 4.4 Test through `Validator`: valid by default with one warning, invalid under `--strict`, and an authored Purpose still passing `--strict` -- [ ] 4.5 Test the gap this closes — the placeholder is over the length floor, so +- [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 -- [ ] 4.6 Test that a bare `TBD` yields exactly one finding against the Purpose -- [ ] 4.7 Test the archive guarantee: the exact non-strict `validateSpecContent` +- [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 -- [ ] 4.8 Test the real file path end to end, reading a spec off disk -- [ ] 4.9 Test that a spec saved with CRLF endings reports the same warning and +- [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 -- [ ] 5.1 Run the full suite and confirm no existing test changes behavior — only +- [x] 5.1 Run the full suite and confirm no existing test changes behavior — only additions -- [ ] 5.2 Run `openspec validate --specs --strict` on this repo and confirm it +- [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 -- [ ] 5.3 Run lint, typecheck, and the build +- [x] 5.3 Run lint, typecheck, and the build - [ ] 5.4 Confirm the cross-platform CI matrix passes, since the check counts lines in files that may carry either line ending -- [ ] 5.5 Add a `.changeset/` entry describing the new warning, its severity, the +- [x] 5.5 Add a `.changeset/` entry describing the new warning, its severity, the detection boundary, and that archive is unaffected 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..718ec84041 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,10 @@ 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 the placeholder archive writes for a new capability, not a Purpose anyone wrote. ' + + '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..a9c2aec5df --- /dev/null +++ b/src/core/validation/purpose-placeholder.ts @@ -0,0 +1,86 @@ +import { PURPOSE_PLACEHOLDER_PREFIX, PURPOSE_PLACEHOLDER_SUFFIX } from './constants.js'; + +/** + * Detects a `## Purpose` that is still the placeholder archive writes, 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` **opening** the Purpose, which is what an agent writes when told to + * leave "a brief TBD placeholder". + * + * A `TBD` 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. + */ + +export interface PurposePlaceholderIssue { + /** 1-based line of the placeholder text, when it can be located. */ + line?: number; +} + +/** A `TBD` opening the Purpose. `\b` keeps it off words like "TBDs". */ +const LEADING_TBD = /^TBD\b/i; + +const PURPOSE_HEADER = /^ {0,3}##(?!#)[ \t]+Purpose[ \t]*$/i; +const TOP_LEVEL_HEADER = /^ {0,3}#{1,2}(?!#)[ \t]+/; + +/** + * 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. + */ +export function findPurposePlaceholderIssue( + overview: string, + content?: string +): PurposePlaceholderIssue | null { + const trimmed = overview.trim(); + if (!trimmed) return null; + if (!containsGeneratedPlaceholder(trimmed) && !LEADING_TBD.test(trimmed)) return null; + return { line: content === undefined ? undefined : findPlaceholderLine(content) }; +} + +/** + * The first non-blank line of the `## Purpose` section, so the warning points at + * the text to replace rather than at the file. Undefined when the section cannot + * be located: the caller 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): number | undefined { + const lines = content.replace(/\r\n?/g, '\n').split('\n'); + const headerIndex = lines.findIndex((line) => PURPOSE_HEADER.test(line)); + if (headerIndex === -1) return undefined; + + for (let i = headerIndex + 1; i < lines.length; i++) { + if (TOP_LEVEL_HEADER.test(lines[i])) return undefined; + if (lines[i].trim()) return i + 1; + } + return undefined; +} diff --git a/src/core/validation/validator.ts b/src/core/validation/validator.ts index 56f771e1a8..4e9024a4a7 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,19 @@ 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. + const placeholder = findPurposePlaceholderIssue(spec.overview, content); + if (placeholder) { + issues.push({ + level: 'WARNING', + path: 'overview', + ...(placeholder.line === undefined ? {} : { 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..9f19324303 --- /dev/null +++ b/test/core/purpose-placeholder.test.ts @@ -0,0 +1,123 @@ +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 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('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 half of the generated sentence', () => { + // The prefix alone is not the placeholder - the suffix must follow it, or + // any Purpose mentioning archiving a change would be reported. + const purpose = 'Explains what happens when archiving change my-change runs twice.'; + 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('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 }); + }); + }); +}); diff --git a/test/core/validation.purpose-placeholder.test.ts b/test/core/validation.purpose-placeholder.test.ts new file mode 100644 index 0000000000..7251c65e3a --- /dev/null +++ b/test/core/validation.purpose-placeholder.test.ts @@ -0,0 +1,167 @@ +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); + }); + }); + + 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, + }, + ]); + }); + }); +}); From 7961a28deca61da501352f71522c80bd2b6c5418 Mon Sep 17 00:00:00 2001 From: kitimark Date: Sat, 15 Aug 2026 13:26:11 +0700 Subject: [PATCH 3/9] test(validate): make every placeholder guard load-bearing A mutation pass over the seven guards -- revert one, see which tests die -- found two that no test held. The prefix/suffix test did not exercise the guard it named. Its Purpose read "Explains what happens when archiving change my-change runs twice", which contains neither half of the generated sentence, so it passed whether or not the suffix was required. Matching on the prefix alone killed nothing. The Purpose now embeds the real prefix constant and asserts the suffix is absent, so the case is the one the name claims; the mutation kills it. The empty-Purpose early return was genuinely dead. Neither rule matches empty text, so removing the branch changed no behaviour and failed no test. Rather than keep a guard nothing can hold, the branch is gone and the comment says why an empty Purpose still yields null. The tests asserting that behaviour are unchanged and still pass. Every guard now dies under mutation: whole check removed from applySpecRules ......... 6 tests brevity no longer suppressed (else -> if) ....... 1 word boundary dropped from the TBD marker ....... 1 generated placeholder matched on prefix alone ... 1 line-ending normalisation removed ............... 2 section-boundary guard removed from locator ..... 1 Full suite 138 files / 3993 tests, lint and typecheck clean. Refs #1670 Co-Authored-By: Claude Opus 5 (1M context) --- src/core/validation/purpose-placeholder.ts | 7 +++++-- test/core/purpose-placeholder.test.ts | 12 ++++++++---- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/src/core/validation/purpose-placeholder.ts b/src/core/validation/purpose-placeholder.ts index a9c2aec5df..5ac3be14f6 100644 --- a/src/core/validation/purpose-placeholder.ts +++ b/src/core/validation/purpose-placeholder.ts @@ -52,14 +52,17 @@ function containsGeneratedPlaceholder(text: string): boolean { * 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. + * 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. const trimmed = overview.trim(); - if (!trimmed) return null; if (!containsGeneratedPlaceholder(trimmed) && !LEADING_TBD.test(trimmed)) return null; return { line: content === undefined ? undefined : findPlaceholderLine(content) }; } diff --git a/test/core/purpose-placeholder.test.ts b/test/core/purpose-placeholder.test.ts index 9f19324303..9787fa2d1c 100644 --- a/test/core/purpose-placeholder.test.ts +++ b/test/core/purpose-placeholder.test.ts @@ -79,10 +79,14 @@ describe('findPurposePlaceholderIssue', () => { expect(findPurposePlaceholderIssue(purpose, specWith(purpose))).toBeNull(); }); - it('does not report half of the generated sentence', () => { - // The prefix alone is not the placeholder - the suffix must follow it, or - // any Purpose mentioning archiving a change would be reported. - const purpose = 'Explains what happens when archiving change my-change runs twice.'; + 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(); }); From d70a781c7e17001893eeb725fbcf542353dac947 Mon Sep 17 00:00:00 2001 From: kitimark Date: Sat, 15 Aug 2026 13:28:40 +0700 Subject: [PATCH 4/9] docs(openspec): record the mutation pass in the task list The mutation work changed the implementation -- a test rewritten and a dead branch removed -- but no task covered it, so the plan claimed less work than was done. Added as group 6, marked complete, with why it was not planned. 5.4 now says what blocks it. It needs a pushed branch for the cross-platform matrix, and the note records that line endings are covered locally by tests asserting a CRLF spec and an LF spec produce identical findings, so a reader can tell the difference between unverified and unverifiable-from-here. The specs, proposal and design are unchanged and were checked: the delta's empty-Purpose clause constrains behaviour, not structure, and that behaviour is the same -- the redundant branch went, the rule did not. 26 of 27 tasks complete; the change still validates --strict. Co-Authored-By: Claude Opus 5 (1M context) --- .../warn-on-purpose-placeholder/tasks.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/openspec/changes/warn-on-purpose-placeholder/tasks.md b/openspec/changes/warn-on-purpose-placeholder/tasks.md index 7c342023e7..5b485974a5 100644 --- a/openspec/changes/warn-on-purpose-placeholder/tasks.md +++ b/openspec/changes/warn-on-purpose-placeholder/tasks.md @@ -62,5 +62,23 @@ - [x] 5.3 Run lint, typecheck, and the build - [ ] 5.4 Confirm the cross-platform CI matrix passes, since the check counts lines in files that may carry either line ending + (blocked: needs a pushed branch — line endings are covered locally by + tests asserting a CRLF spec and an LF spec produce identical findings) - [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 From 7de04065ebb3d7b06074deb1f28a63f20a58d363 Mon Sep 17 00:00:00 2001 From: kitimark Date: Sat, 15 Aug 2026 13:37:11 +0700 Subject: [PATCH 5/9] docs(openspec): close 5.4 on a green cross-platform matrix CI dispatched on the fork against this branch: lint & typecheck, and the test suite on linux-bash, macos-bash and windows-pwsh -- all green. The Windows job installed, built and ran the suite rather than short-circuiting, which is the part 5.4 existed to check, since the placeholder locator counts lines in files that may carry either ending. Recorded as a workflow_dispatch run on the fork, not the upstream pull-request run, because those are not the same gate and the note should not let a reader assume otherwise. Nix Flake Validation and Validate Release Tracking skipped: this branch touches neither the flake nor release tracking. 27 of 27 tasks complete. Co-Authored-By: Claude Opus 5 (1M context) --- openspec/changes/warn-on-purpose-placeholder/tasks.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/openspec/changes/warn-on-purpose-placeholder/tasks.md b/openspec/changes/warn-on-purpose-placeholder/tasks.md index 5b485974a5..3f18fe69ab 100644 --- a/openspec/changes/warn-on-purpose-placeholder/tasks.md +++ b/openspec/changes/warn-on-purpose-placeholder/tasks.md @@ -60,10 +60,11 @@ 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 -- [ ] 5.4 Confirm the cross-platform CI matrix passes, since the check counts +- [x] 5.4 Confirm the cross-platform CI matrix passes, since the check counts lines in files that may carry either line ending - (blocked: needs a pushed branch — line endings are covered locally by - tests asserting a CRLF spec and an LF spec produce identical findings) + (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 From c0535beed727402dcd92109b61653e435cbc5874 Mon Sep 17 00:00:00 2001 From: kitimark Date: Sat, 15 Aug 2026 14:36:57 +0700 Subject: [PATCH 6/9] fix(validate): name the placeholder's line, not the prose above it The warning tells you which line to fix, and named the wrong one when the generated sentence did not open the Purpose: 3 ## Purpose 4 Handles widget retries. <- warning pointed here 5 6 TBD - created by archiving ... <- placeholder is here The locator asked "what is the first non-blank line after ## Purpose?" rather than "where is the placeholder?". Those are the same line in five of the six shapes a placeholder can take -- a leading TBD marker is the first non-blank line by definition, and archive writes the generated sentence as the section's only content -- so the two questions only diverge when a human types prose above a leftover placeholder. Pointing at that prose is worse than pointing nowhere: the reader sees a sentence that is plainly fine and concludes the check is broken. design.md already said a wrong line number is worse than none, and the delta already required naming the line the placeholder is on, so this is the implementation meeting a contract that was already written, not a change of contract. The locator is now told which rule matched. A leading marker keeps the first-non-blank behaviour, because that is where it sits; the generated sentence is located by its own text. When both match the leading marker wins, being the earlier of the two. Found by CodeRabbit on #1671. The finding was real despite its own "Addressed" marker, which only tracked the file changing in a later commit. Two test gaps let it through. The case that covered this input asserted only that something was reported, never which line -- so it now asserts the line, and a table pins every position a placeholder can occupy, each case first checking that the line it expects really carries the placeholder. The mutation pass could not have caught it either: mutation proves a test dies when a guard is broken, and cannot invent an assertion nobody wrote. Reverting the branch fails exactly the three new expectations. Full suite 4000 tests / 138 files, lint and typecheck clean. Refs #1670 Co-Authored-By: Claude Opus 5 (1M context) --- .../warn-on-purpose-placeholder/design.md | 25 ++++++-- src/core/validation/purpose-placeholder.ts | 29 +++++++--- test/core/purpose-placeholder.test.ts | 58 +++++++++++++++++++ 3 files changed, 99 insertions(+), 13 deletions(-) diff --git a/openspec/changes/warn-on-purpose-placeholder/design.md b/openspec/changes/warn-on-purpose-placeholder/design.md index 24a6963021..393a10b1cd 100644 --- a/openspec/changes/warn-on-purpose-placeholder/design.md +++ b/openspec/changes/warn-on-purpose-placeholder/design.md @@ -92,6 +92,12 @@ written by agents and authors, not generated here, so there is no list to consul 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 @@ -100,12 +106,21 @@ 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 is best-effort +### 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. -The warning names the first non-blank line of the `## Purpose` section. When the -section cannot be located — the parsed Purpose and the file disagree — the finding -is reported without a line rather than with a guessed one, since a wrong line -number is worse than none. +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. diff --git a/src/core/validation/purpose-placeholder.ts b/src/core/validation/purpose-placeholder.ts index 5ac3be14f6..2728b9be11 100644 --- a/src/core/validation/purpose-placeholder.ts +++ b/src/core/validation/purpose-placeholder.ts @@ -63,27 +63,40 @@ export function findPurposePlaceholderIssue( // 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. const trimmed = overview.trim(); - if (!containsGeneratedPlaceholder(trimmed) && !LEADING_TBD.test(trimmed)) return null; - return { line: content === undefined ? undefined : findPlaceholderLine(content) }; + const leading = LEADING_TBD.test(trimmed); + if (!leading && !containsGeneratedPlaceholder(trimmed)) 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 first non-blank line of the `## Purpose` section, so the warning points at - * the text to replace rather than at the file. Undefined when the section cannot - * be located: the caller reports the finding without a line rather than with a - * guessed one, since a wrong line number is worse than none. + * 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 `TBD` 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. + * + * 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): number | undefined { +function findPlaceholderLine(content: string, leading: boolean): number | undefined { const lines = content.replace(/\r\n?/g, '\n').split('\n'); const headerIndex = lines.findIndex((line) => PURPOSE_HEADER.test(line)); if (headerIndex === -1) return undefined; for (let i = headerIndex + 1; i < lines.length; i++) { if (TOP_LEVEL_HEADER.test(lines[i])) return undefined; - if (lines[i].trim()) return i + 1; + if (leading ? lines[i].trim() : lines[i].includes(PURPOSE_PLACEHOLDER_PREFIX)) return i + 1; } return undefined; } diff --git a/test/core/purpose-placeholder.test.ts b/test/core/purpose-placeholder.test.ts index 9787fa2d1c..dd3aae0192 100644 --- a/test/core/purpose-placeholder.test.ts +++ b/test/core/purpose-placeholder.test.ts @@ -62,6 +62,13 @@ describe('findPurposePlaceholderIssue', () => { 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(); }); @@ -124,4 +131,55 @@ describe('findPurposePlaceholderIssue', () => { 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 }); + }); + } + }); }); From ba88f2e7f4cbb81fb9acdd6c421d1827231119f7 Mon Sep 17 00:00:00 2001 From: kitimark Date: Sun, 16 Aug 2026 04:20:47 +0700 Subject: [PATCH 7/9] refactor(validate): set the placeholder line unconditionally ValidationIssue.line is optional and the project does not enable exactOptionalPropertyTypes, so a plain assignment typechecks and JSON output is unchanged - JSON.stringify drops undefined values. findPurposePlaceholderIssue already returns the key unconditionally, and the neighbouring push sites assign line plainly, so the conditional spread was the odd one out. --- src/core/validation/validator.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/validation/validator.ts b/src/core/validation/validator.ts index 4e9024a4a7..03754e8e72 100644 --- a/src/core/validation/validator.ts +++ b/src/core/validation/validator.ts @@ -659,7 +659,7 @@ export class Validator { issues.push({ level: 'WARNING', path: 'overview', - ...(placeholder.line === undefined ? {} : { line: placeholder.line }), + line: placeholder.line, message: VALIDATION_MESSAGES.PURPOSE_IS_PLACEHOLDER, }); } else if (spec.overview.length < MIN_PURPOSE_LENGTH) { From ee135660b8b20c5e8d36dec4eb5ff0edea39770c Mon Sep 17 00:00:00 2001 From: Clay Good Date: Thu, 20 Aug 2026 12:01:11 -0500 Subject: [PATCH 8/9] fix(validate): widen the placeholder check to TODO and read fences as quoted #1670 left two questions open. Both are answered here, against how OpenSpec already reads a spec. A `TODO` opening the Purpose now reports as the same finding as a `TBD`. Nothing OpenSpec writes produces one, but the marker an author leaves behind is whichever word they reached for, and a Purpose reading `TODO: fill this in` is as unwritten as one reading `TBD`. Only the opening position counts, as before, so `TODOs are tracked in the linked issue` is still authored prose. Fenced code inside a Purpose is now read as quoted material rather than as the Purpose speaking, through the `buildCodeFenceMask` the requirement and structure parsers already share. Without it a spec documenting the sentence archive writes is reported as carrying it, which is the check failing the one document that explains it - and a warning that fires on the docs teaches people to ignore the warning. Fenced lines are skipped 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. The message now names both what archive writes and a marker left in its place, since one message covers both. Severity is unchanged: still a warning, so a project carrying placeholders keeps validating and only --strict fails. Every new guard is mutation-checked: dropping `TODO` kills 3 tests, unmasking detection kills 2, unmasking the line locator kills 3, unmasking the header search kills 1. Co-Authored-By: Claude Opus 5 --- .../validate-reports-purpose-placeholder.md | 2 +- .../warn-on-purpose-placeholder/proposal.md | 6 +- .../specs/cli-validate/spec.md | 36 ++++-- .../warn-on-purpose-placeholder/tasks.md | 26 +++++ src/core/validation/constants.ts | 7 +- src/core/validation/purpose-placeholder.ts | 72 ++++++++---- src/core/validation/validator.ts | 3 +- test/core/purpose-placeholder.test.ts | 106 ++++++++++++++++++ .../validation.purpose-placeholder.test.ts | 30 +++++ 9 files changed, 255 insertions(+), 33 deletions(-) diff --git a/.changeset/validate-reports-purpose-placeholder.md b/.changeset/validate-reports-purpose-placeholder.md index 44f74264bd..d0cef003f3 100644 --- a/.changeset/validate-reports-purpose-placeholder.md +++ b/.changeset/validate-reports-purpose-placeholder.md @@ -6,6 +6,6 @@ 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` 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. 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. +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/proposal.md b/openspec/changes/warn-on-purpose-placeholder/proposal.md index 280d39d5f3..7f5b1b161c 100644 --- a/openspec/changes/warn-on-purpose-placeholder/proposal.md +++ b/openspec/changes/warn-on-purpose-placeholder/proposal.md @@ -28,8 +28,10 @@ been instructions, which is the mechanism that report described as unreliable. 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` opening the Purpose counts. - A `TBD` inside a sentence is authored prose and is left alone. + 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 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 index c4152e6b37..08186416fe 100644 --- a/openspec/changes/warn-on-purpose-placeholder/specs/cli-validate/spec.md +++ b/openspec/changes/warn-on-purpose-placeholder/specs/cli-validate/spec.md @@ -3,10 +3,11 @@ ### 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 the placeholder `openspec archive` writes for a new -capability rather than a Purpose someone wrote. 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. +`## 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 @@ -22,10 +23,17 @@ 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` marker opening the Purpose SHALL be reported; -- a `TBD` appearing inside a sentence SHALL NOT be reported; +- 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 word that merely begins with those letters SHALL NOT be reported. +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. @@ -56,9 +64,23 @@ passed before this requirement existed. - **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." +- **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 diff --git a/openspec/changes/warn-on-purpose-placeholder/tasks.md b/openspec/changes/warn-on-purpose-placeholder/tasks.md index 3f18fe69ab..1714952021 100644 --- a/openspec/changes/warn-on-purpose-placeholder/tasks.md +++ b/openspec/changes/warn-on-purpose-placeholder/tasks.md @@ -83,3 +83,29 @@ were not, and only reverting them one at a time showed it. 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 + +#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 diff --git a/src/core/validation/constants.ts b/src/core/validation/constants.ts index 718ec84041..3c570255cd 100644 --- a/src/core/validation/constants.ts +++ b/src/core/validation/constants.ts @@ -48,9 +48,10 @@ 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 the placeholder archive writes for a new capability, not a Purpose anyone wrote. ' + - '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.', + '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 index 2728b9be11..3f9906554c 100644 --- a/src/core/validation/purpose-placeholder.ts +++ b/src/core/validation/purpose-placeholder.ts @@ -1,8 +1,9 @@ +import { buildCodeFenceMask } from '../parsers/code-fence.js'; import { PURPOSE_PLACEHOLDER_PREFIX, PURPOSE_PLACEHOLDER_SUFFIX } from './constants.js'; /** - * Detects a `## Purpose` that is still the placeholder archive writes, rather - * than one somebody wrote. + * 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 @@ -14,15 +15,22 @@ import { PURPOSE_PLACEHOLDER_PREFIX, PURPOSE_PLACEHOLDER_SUFFIX } from './consta * 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 + * the writer composes it from, wherever it sits in the Purpose - nobody types * that sentence by accident; - * - a `TBD` **opening** the Purpose, which is what an agent writes when told to - * leave "a brief TBD placeholder". + * - 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 `TBD` inside a sentence is left alone. "The retry budget is TBD pending + * 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 + * 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 { @@ -30,15 +38,34 @@ export interface PurposePlaceholderIssue { line?: number; } -/** A `TBD` opening the Purpose. `\b` keeps it off words like "TBDs". */ -const LEADING_TBD = /^TBD\b/i; +/** + * A `TBD` or `TODO` opening the Purpose. `\b` 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 -`. + */ +const LEADING_MARKER = /^(?:TBD|TODO)\b/i; 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 + * 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 { @@ -51,7 +78,7 @@ function containsGeneratedPlaceholder(text: string): boolean { * 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 + * 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. */ @@ -61,10 +88,12 @@ export function findPurposePlaceholderIssue( ): 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. - const trimmed = overview.trim(); - const leading = LEADING_TBD.test(trimmed); - if (!leading && !containsGeneratedPlaceholder(trimmed)) return null; + // 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. @@ -75,13 +104,16 @@ export function findPurposePlaceholderIssue( * 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 `TBD` is the + * 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 + * 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. * - * Undefined when the placeholder cannot be located — no section header, or a + * 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. @@ -91,10 +123,12 @@ export function findPurposePlaceholderIssue( */ function findPlaceholderLine(content: string, leading: boolean): number | undefined { const lines = content.replace(/\r\n?/g, '\n').split('\n'); - const headerIndex = lines.findIndex((line) => PURPOSE_HEADER.test(line)); + 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; } diff --git a/src/core/validation/validator.ts b/src/core/validation/validator.ts index 03754e8e72..2bcb75cc57 100644 --- a/src/core/validation/validator.ts +++ b/src/core/validation/validator.ts @@ -653,7 +653,8 @@ export class Validator { // 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. + // 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({ diff --git a/test/core/purpose-placeholder.test.ts b/test/core/purpose-placeholder.test.ts index dd3aae0192..b5bad6497c 100644 --- a/test/core/purpose-placeholder.test.ts +++ b/test/core/purpose-placeholder.test.ts @@ -55,6 +55,20 @@ describe('findPurposePlaceholderIssue', () => { 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. @@ -86,6 +100,17 @@ describe('findPurposePlaceholderIssue', () => { 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 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 @@ -107,6 +132,87 @@ describe('findPurposePlaceholderIssue', () => { }); }); + 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'); diff --git a/test/core/validation.purpose-placeholder.test.ts b/test/core/validation.purpose-placeholder.test.ts index 7251c65e3a..1f64e3c664 100644 --- a/test/core/validation.purpose-placeholder.test.ts +++ b/test/core/validation.purpose-placeholder.test.ts @@ -107,6 +107,36 @@ describe('Purpose placeholder validation', () => { 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', () => { From 207cf25cb7140652b708c3bf26ae10d9ea6f7886 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Thu, 20 Aug 2026 12:11:27 -0500 Subject: [PATCH 9/9] fix(validate): read the marker boundary in any script, not just ASCII MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found `\b` reading `TODOé` and `TBD١` as a marker followed by punctuation, because `\b` only knows ASCII word characters. A Purpose is prose and prose is not always Latin script, so the rule that a longer word beginning with those letters is not a marker has to hold in any script. The lookahead rejects letters, digits, combining marks and `_`, and nothing else, so `TODO:`, `TBD -` and `TODO(owner):` are still the marker they look like. Held in both directions: loosening it back to `\b` kills 1 test, tightening it to reject punctuation kills 4. Also reworded a task line that opened with `#1670`, which markdownlint reads as a heading missing its space. Co-Authored-By: Claude Opus 5 --- .../specs/cli-validate/spec.md | 3 ++- .../warn-on-purpose-placeholder/tasks.md | 10 ++++++++-- src/core/validation/purpose-placeholder.ts | 12 ++++++++---- test/core/purpose-placeholder.test.ts | 17 +++++++++++++++++ 4 files changed, 35 insertions(+), 7 deletions(-) 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 index 08186416fe..0c00fe1b5b 100644 --- a/openspec/changes/warn-on-purpose-placeholder/specs/cli-validate/spec.md +++ b/openspec/changes/warn-on-purpose-placeholder/specs/cli-validate/spec.md @@ -27,7 +27,8 @@ open question is not a placeholder: 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 word that merely begins with those letters 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 diff --git a/openspec/changes/warn-on-purpose-placeholder/tasks.md b/openspec/changes/warn-on-purpose-placeholder/tasks.md index 1714952021..0b82a3db3a 100644 --- a/openspec/changes/warn-on-purpose-placeholder/tasks.md +++ b/openspec/changes/warn-on-purpose-placeholder/tasks.md @@ -86,8 +86,8 @@ were not, and only reverting them one at a time showed it. ## 7. Answer the two questions the issue left open -#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 +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`. @@ -109,3 +109,9 @@ what changed after review. - [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/validation/purpose-placeholder.ts b/src/core/validation/purpose-placeholder.ts index 3f9906554c..210bef885e 100644 --- a/src/core/validation/purpose-placeholder.ts +++ b/src/core/validation/purpose-placeholder.ts @@ -39,11 +39,15 @@ export interface PurposePlaceholderIssue { } /** - * A `TBD` or `TODO` opening the Purpose. `\b` 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 -`. + * 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)\b/i; +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]+/; diff --git a/test/core/purpose-placeholder.test.ts b/test/core/purpose-placeholder.test.ts index b5bad6497c..1d074c2f68 100644 --- a/test/core/purpose-placeholder.test.ts +++ b/test/core/purpose-placeholder.test.ts @@ -105,6 +105,23 @@ describe('findPurposePlaceholderIssue', () => { 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.';