diff --git a/.changeset/purpose-marker-punctuation.md b/.changeset/purpose-marker-punctuation.md new file mode 100644 index 0000000000..ae4102edd4 --- /dev/null +++ b/.changeset/purpose-marker-punctuation.md @@ -0,0 +1,8 @@ +--- +'@fission-ai/openspec': patch +--- + +Fix `validate --strict` reporting `PURPOSE_IS_PLACEHOLDER` for a Purpose that opens with the ordinary word "Todo" followed by prose, as in Spanish ("Todo el…") and Portuguese ("Todo o…") specs ([#1897](https://github.com/Fission-AI/OpenSpec/issues/1897)). + +- Case now separates the marker from the word. `TBD`/`TODO` in capitals is still a placeholder marker whatever follows it, so `TODO write this later` is still reported. +- In any other case it counts as a marker only when followed by the end of the Purpose, a line break, or marker punctuation (`todo -`, `tbd.`), so an authored Spanish or Portuguese sentence is not reported. diff --git a/src/core/validation/purpose-placeholder.ts b/src/core/validation/purpose-placeholder.ts index 5fa3060df0..2f411a3dfc 100644 --- a/src/core/validation/purpose-placeholder.ts +++ b/src/core/validation/purpose-placeholder.ts @@ -39,15 +39,35 @@ export interface PurposePlaceholderIssue { } /** - * 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. + * A `TBD` or `TODO` opening the Purpose. + * + * `WORD_END` keeps the marker 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`. + * + * Case is what separates the marker from the word. `TODO` shouted in capitals + * is the marker whatever follows it, so `TODO write this later` is still an + * unwritten Purpose. Written in any other case it is only a marker when + * punctuation or the end of the line says so, because `todo` is an extremely + * frequent sentence opener in Spanish ("Todo el...") and Portuguese ("Todo + * o..."), and ordinary prose in those languages is not a placeholder. That + * keeps the lowercase forms an agent really does leave behind - `todo - write + * this later`, `tbd.` - reported, without reading a Spanish sentence as one. */ -const LEADING_MARKER = /^(?:TBD|TODO)(?![\p{L}\p{N}\p{M}_])/iu; +const WORD_END = '(?![\\p{L}\\p{N}\\p{M}_])'; +const MARKER_PUNCTUATION = '(?=[ \\t]*(?:$|\\n|[:\\-\u2013\u2014.,;()\\[\\]{}]))'; + +/** `TBD`/`TODO` in capitals: the marker, whatever follows it. */ +const LEADING_MARKER_SHOUTED = new RegExp(`^(?:TBD|TODO)${WORD_END}`, 'u'); + +/** Any other case: a marker only when punctuation or the line end says so. */ +const LEADING_MARKER_PUNCTUATED = new RegExp( + `^(?:TBD|TODO)${WORD_END}${MARKER_PUNCTUATION}`, + 'iu' +); const PURPOSE_HEADER = /^ {0,3}##(?!#)[ \t]+Purpose[ \t]*$/i; const TOP_LEVEL_HEADER = /^ {0,3}#{1,2}(?!#)[ \t]+/; @@ -104,7 +124,8 @@ export function findPurposePlaceholderIssue( // 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); + const leading = + LEADING_MARKER_SHOUTED.test(prose) || LEADING_MARKER_PUNCTUATED.test(prose); if (!leading && generatedPlaceholderPrefixIndex(prose) === undefined) 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 diff --git a/test/core/purpose-placeholder.test.ts b/test/core/purpose-placeholder.test.ts index aee7b9cbc8..cb78ce9e57 100644 --- a/test/core/purpose-placeholder.test.ts +++ b/test/core/purpose-placeholder.test.ts @@ -164,6 +164,43 @@ describe('findPurposePlaceholderIssue', () => { expect(findPurposePlaceholderIssue(' \n ', specWith(''))).toBeNull(); }); + it('does not report the Spanish word "Todo" opening authored prose', () => { + // `todo` is an extremely frequent sentence opener in Spanish ("Todo + // el…") and Portuguese ("Todo o…"). A marker is written `TODO:`, + // `TODO -`, or alone on its line - never `Todo` followed by prose. + for (const purpose of [ + 'Todo el conocimiento del producto vive del otro lado, en el repo hermano.', + 'Todo o catálogo é carregado a partir do repositório irmão.', + ]) { + expect(findPurposePlaceholderIssue(purpose, specWith(purpose))).toBeNull(); + } + }); + + it('still reports a marker alone on its line above placeholder prose', () => { + const purpose = 'TODO\nfill this in once the capability settles down.'; + expect(findPurposePlaceholderIssue(purpose, specWith(purpose))).not.toBeNull(); + }); + + it('still reports a shouted TODO opening placeholder prose without punctuation', () => { + // Case is what separates the marker from the Spanish word. In capitals + // it is the marker whatever follows it, so requiring punctuation must + // not let the plainest unwritten Purpose of all through. + for (const purpose of [ + 'TODO write this once the capability settles down.', + 'TBD pending the design review that has not happened yet.', + ]) { + expect(findPurposePlaceholderIssue(purpose, specWith(purpose))).not.toBeNull(); + } + }); + + it('does not report lowercase Spanish prose either', () => { + // The reporter is the capitals, not the position: `todo` uncapitalised + // opens a sentence just as often, and is just as much authored prose. + const purpose = + 'todo el conocimiento del producto vive del otro lado, en el repo hermano.'; + expect(findPurposePlaceholderIssue(purpose, specWith(purpose))).toBeNull(); + }); + it('does not report an ordinary short Purpose, which PURPOSE_TOO_BRIEF covers', () => { expect(findPurposePlaceholderIssue('Does stuff.', specWith('Does stuff.'))).toBeNull(); });