diff --git a/.abcd/development/specs/open/spc-12-disembark-grounds-a-lifeboat-s-open-questions-on-a-repo-s-to.md b/.abcd/development/specs/open/spc-12-disembark-grounds-a-lifeboat-s-open-questions-on-a-repo-s-to.md index 62e6fda..9ee07cd 100644 --- a/.abcd/development/specs/open/spc-12-disembark-grounds-a-lifeboat-s-open-questions-on-a-repo-s-to.md +++ b/.abcd/development/specs/open/spc-12-disembark-grounds-a-lifeboat-s-open-questions-on-a-repo-s-to.md @@ -83,16 +83,22 @@ The primitive therefore adds no second read path to audit. - **Recognised markers:** `TODO`, `FIXME`, `XXX`, `HACK`, `BUG` — uppercase only, matched by - `(^|[^A-Za-z0-9_-])(TODO|FIXME|XXX|HACK|BUG)(:|\(|\s|$)`. The leading class - is the word boundary that stops `TODO` matching inside `TODOS` or - `todo_list`; the trailing class admits the two conventional spellings - (`TODO:` and `TODO(alice):`) plus a bare word. The hyphen is excluded from - the leading class so the common redaction placeholder shape (`XXX-XXX-XXX`) - is rejected at every one of its triples — with the hyphen admitted, the last - triple matches on its leading `-` and a support phone number becomes a - fabricated open question. `NOTE` and `OPTIMIZE` are *not* recognised: `NOTE` - marks explanation rather than unfinished work, and `OPTIMIZE` is rare enough - that its false-positive cost exceeds its value. + `(^|[^A-Za-z0-9_-])(?:(TODO|FIXME)(?::|\()|(XXX|HACK|BUG)(?::|\(|\s|$))`. + The leading class is the word boundary that stops a marker matching inside a + longer identifier (`TODOS`, `todo_list`). The trailing boundary splits the + markers into two classes: `TODO` and `FIXME` require a trailing `:` or `(` + (the conventional `TODO:` and `TODO(alice):` spellings), because these are the + markers a project documents by name and their bare-word form is + indistinguishable from prose that merely mentions the marker (iss-111); `XXX`, + `HACK`, and `BUG` additionally admit a bare word (trailing whitespace or + end-of-line), because they are rarely written as bare uppercase words in prose + and the bare spelling is how they are conventionally written. The hyphen is + excluded from the leading class so the common redaction placeholder shape + (`XXX-XXX-XXX`) is rejected at every one of its triples — with the hyphen + admitted, the last triple matches on its leading `-` and a support phone + number becomes a fabricated open question. `NOTE` and `OPTIMIZE` are *not* + recognised: `NOTE` marks explanation rather than unfinished work, and + `OPTIMIZE` is rare enough that its false-positive cost exceeds its value. - **Binary files are skipped** by a NUL byte in the first 8 KiB — the conventional heuristic, and dependency-free. No extension allow-list is maintained. @@ -131,7 +137,7 @@ The primitive therefore adds no second read path to audit. | Question | Decision | |---|---| -| Which markers? | `TODO`, `FIXME`, `XXX`, `HACK`, `BUG`; uppercase only; word-boundary anchored, trailing `:`/`(`/space/EOL. `NOTE`, `OPTIMIZE` excluded. | +| Which markers? | `TODO`, `FIXME`, `XXX`, `HACK`, `BUG`; uppercase only; word-boundary anchored. `TODO`/`FIXME` require a trailing `:`/`(` (a bare word reads as prose that names the marker, iss-111); `XXX`/`HACK`/`BUG` also admit a bare word (trailing space/EOL). `NOTE`, `OPTIMIZE` excluded. | | Which tier — conventions or git? | **Conventions.** A working-tree file scan through the `SourceContext` file surface. The adapter never touches git, so it grounds a bare snapshot as readily as a working tree. | | Scan scope and the missing primitive | Option (a): add a bounded recursive-walk primitive, `WalkFiles`, to `SourceContext`. It is shared with itd-96, so the walk lands once. | | Which files to scan | Every regular file the walk yields, minus the skip set (`.git`, `node_modules`, `vendor`, `generated`), minus symlinks, minus binaries (NUL-byte heuristic), minus oversized files (`ReadFile`'s cap). | diff --git a/.abcd/work/issues/open/iss-111-the-open-questions-marker-pattern-admits-a-bare-uppercase-wo.md b/.abcd/work/issues/resolved/iss-111-the-open-questions-marker-pattern-admits-a-bare-uppercase-wo.md similarity index 55% rename from .abcd/work/issues/open/iss-111-the-open-questions-marker-pattern-admits-a-bare-uppercase-wo.md rename to .abcd/work/issues/resolved/iss-111-the-open-questions-marker-pattern-admits-a-bare-uppercase-wo.md index 6b712ad..a265ce2 100644 --- a/.abcd/work/issues/open/iss-111-the-open-questions-marker-pattern-admits-a-bare-uppercase-wo.md +++ b/.abcd/work/issues/resolved/iss-111-the-open-questions-marker-pattern-admits-a-bare-uppercase-wo.md @@ -7,6 +7,8 @@ category: "tech-debt" source: "impl-review" found_during: "itd-95 P1 review" found_at: "internal/core/lifeboat/sources_conventions.go" +resolution: "Marker pattern now splits by trailing boundary: TODO/FIXME require ':' or '(', XXX/HACK/BUG keep the bare word (option 2 of the two recorded). On this repo the durable-record documentation FP corpus (scope: .abcd/development/) dropped from 14 to 3 (the 3 survivors are prose that literally quotes the colon form, irreducible for this mechanism); 100% of the bare-word 'mentions a marker' FP class killed. All colon-form fixture markers preserved; only the bare 'FIXME check this' and '# TODO' pinning spellings reclassified to reject, correctly, as indistinguishable from prose. spc-12 pattern description amended in the same change." +impact: fix --- The open-questions marker pattern admits a bare uppercase word followed by whitespace, so prose that merely mentions a marker matches. Measured against this repository the adapter reports 42 markers across 11 files at medium confidence, and every one is documentation about markers rather than a work marker. The precision cost lands on repos that document their own conventions. Options: require the trailing colon or parenthesis, or drop the bare-word alternative for the ambiguous markers only. spc-12 fixes the current pattern, so this is a design revisit. \ No newline at end of file diff --git a/CHANGELOG.md b/CHANGELOG.md index bf8348b..1fe2b05 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -54,6 +54,19 @@ called out in a **Breaking** section. ### Fixed +- **The open-questions marker scan no longer reads documentation about markers as + open questions** (iss-111). The pattern that grounds `evidence/open-questions` + admitted a bare uppercase `TODO`/`FIXME` followed by whitespace, so on a + repository that documents its own conventions every prose mention of a marker + was cited as a work marker — 14 such false positives across the durable record + (`.abcd/development/`) on this repository, all documentation, none a real + marker, down to 3 after the fix (each an irreducible prose quotation of the + literal `TODO:` form). `TODO` and `FIXME` + now require a trailing `:` or `(` (the conventional `TODO:` / `TODO(alice):` + spellings), which is how genuine markers are almost always written; `XXX`, + `HACK`, and `BUG` still admit their conventional bare form, because they are + rarely written as bare words in prose and carry no measured false-positive + cost. - **Concurrent runs can no longer drop a repo registration or delete a just-committed issue file** (iss-101, iss-102). Two `abcd ahoy install` runs from different worktrees shared one `~/.abcd/history/index.json`, and its diff --git a/internal/core/lifeboat/sources_conventions.go b/internal/core/lifeboat/sources_conventions.go index cd33a5d..eeb08cf 100644 --- a/internal/core/lifeboat/sources_conventions.go +++ b/internal/core/lifeboat/sources_conventions.go @@ -697,18 +697,54 @@ func (convInternalsSource) probeLimited(ctx *SourceContext, walkLimit int) Evide // convMarkerNames are the in-code work markers recognised as open questions, // uppercase only. NOTE and OPTIMIZE are deliberately absent: NOTE marks // explanation rather than unfinished work, and OPTIMIZE is rare enough that its -// false-positive cost exceeds its value. -var convMarkerNames = []string{"TODO", "FIXME", "XXX", "HACK", "BUG"} +// false-positive cost exceeds its value. The set splits by how the pattern +// anchors each marker's trailing boundary (see convMarkerRe). +var convMarkerNames = append(append([]string{}, convDelimitedMarkers...), convBareMarkers...) + +// convDelimitedMarkers must carry a trailing ':' or '(' to be recognised. They +// are the markers a project documents by name, so their bare-word form (the +// marker followed by whitespace or end-of-line) is indistinguishable from prose +// that merely mentions the marker — the dominant false-positive class on a +// repository that documents its own conventions (iss-111). Requiring the +// conventional trailing delimiter — a colon or an open-paren, as in an authored +// marker naming its owner — keeps the genuine markers and drops the mentions. +// (This comment states the markers as slice values below rather than inline, so +// the scan does not cite its own prose.) +var convDelimitedMarkers = []string{"TODO", "FIXME"} + +// convBareMarkers additionally match a bare word (a trailing whitespace or +// end-of-line). These three are rarely written as bare uppercase words in running +// prose, so their bare form carries no measured false-positive cost, and that +// bare spelling is how they are conventionally written — requiring a delimiter +// would lose real markers for no precision gain. (Named only as slice values +// below, for the same reason as convDelimitedMarkers.) +var convBareMarkers = []string{"XXX", "HACK", "BUG"} // convMarkerRe matches one recognised marker on a line. The leading class is the -// word boundary that stops TODO matching inside TODOS or todo_list; the trailing -// class admits the two conventional spellings (TODO: and TODO(alice):) plus a -// bare word. The hyphen is excluded from the leading class so the redaction -// placeholder shape (XXX-XXX-XXX) is rejected at every one of its triples — -// without it the last triple matches on its leading hyphen and a support phone -// number becomes a fabricated open question. Built from convMarkerNames so the -// set and the pattern cannot drift, and compiled once. -var convMarkerRe = regexp.MustCompile(`(^|[^A-Za-z0-9_-])(` + strings.Join(convMarkerNames, "|") + `)(:|\(|\s|$)`) +// word boundary that stops a marker matching inside a longer identifier (TODOS, +// todo_list); the hyphen is excluded from it so the redaction placeholder shape +// (XXX-XXX-XXX) is rejected at every one of its triples — without that exclusion +// the last triple matches on its leading hyphen and a support phone number +// becomes a fabricated open question. The two marker classes differ only in their +// trailing boundary: convDelimitedMarkers (captured in group 2) require ':' or +// '('; convBareMarkers (group 3) also accept whitespace or end-of-line. Exactly +// one of the two marker groups is non-empty on a match — convMarkerName reads +// whichever fired. Built from the marker slices so the set and the pattern cannot +// drift, and compiled once. +var convMarkerRe = regexp.MustCompile( + `(^|[^A-Za-z0-9_-])` + + `(?:(` + strings.Join(convDelimitedMarkers, "|") + `)(?::|\()` + + `|(` + strings.Join(convBareMarkers, "|") + `)(?::|\(|\s|$))`) + +// convMarkerName returns the marker a convMarkerRe match captured. The pattern +// puts a delimited marker in group 2 and a bare-allowed marker in group 3, and +// exactly one is non-empty. +func convMarkerName(m []string) string { + if m[2] != "" { + return m[2] + } + return m[3] +} // maxMarkerCitations caps how many path:line citations the marker scan reports. // Beyond it the scan keeps counting — the headline stays truthful — but stops @@ -805,7 +841,7 @@ func (convOpenQuestionsSource) probeLimited(ctx *SourceContext, budget int) Evid hits++ markers++ if len(citations) < maxMarkerCitations { - citations = append(citations, fmt.Sprintf("%s:%d (%s)", p, i+1, m[2])) + citations = append(citations, fmt.Sprintf("%s:%d (%s)", p, i+1, convMarkerName(m))) } } if hits > 0 { diff --git a/internal/core/lifeboat/sources_conventions_test.go b/internal/core/lifeboat/sources_conventions_test.go index 47e2a86..0fb77d8 100644 --- a/internal/core/lifeboat/sources_conventions_test.go +++ b/internal/core/lifeboat/sources_conventions_test.go @@ -434,6 +434,31 @@ func TestConvOpenQuestionsIgnoresRedactionPlaceholders(t *testing.T) { } } +// TestConvOpenQuestionsIgnoresProseMentioningMarkers holds the iss-111 contract: +// a repository that documents its own conventions mentions TODO and FIXME by name +// in prose, and those mentions are not the team's open questions. A bare TODO or +// FIXME followed by whitespace reads exactly like such a mention, so the scan must +// come back blank rather than fabricate open questions out of the documentation. +func TestConvOpenQuestionsIgnoresProseMentioningMarkers(t *testing.T) { + dir := t.TempDir() + writeTree(t, dir, map[string]string{ + "go.mod": "module example.com/documented\n\ngo 1.22\n", + "CONTRIBUTING.md": "# Conventions\n\n" + + "Leave a TODO for unfinished work and a FIXME where a bug hides.\n" + + "We grep for TODO and FIXME markers before every release.\n", + }) + ctx, err := newSourceContext(dir) + if err != nil { + t.Fatal(err) + } + defer ctx.Close() + + ev := convSourceForSection(t, "evidence/open-questions").Probe(ctx) + if ev.Status != StatusBlank { + t.Fatalf("prose that merely names markers gives status %s, want blank; fabricated evidence %v", ev.Status, ev.Sources) + } +} + // TestConvMarkerRePinsTheRecognisedSpellings pins the marker pattern against the // spellings it must accept and the near-misses it must reject, so the word // boundary cannot be loosened or tightened without a test saying so. @@ -441,9 +466,7 @@ func TestConvMarkerRePinsTheRecognisedSpellings(t *testing.T) { match := []string{ "// TODO: handle the retry case", "//TODO: no space after the slashes", - "# TODO", "- TODO: a list item", - "* FIXME check this", "FIXME(alice): leaks a connection", "-- BUG --", "// HACK around the driver", @@ -455,6 +478,15 @@ func TestConvMarkerRePinsTheRecognisedSpellings(t *testing.T) { "XXX-XXX-XXX", "TODOS are not markers", "todo_list := nil", + // iss-111: TODO and FIXME require a trailing ':' or '('. A bare word is + // indistinguishable from prose that merely names the marker — the + // false-positive class that swamped a repo documenting its own + // conventions — so these documentation mentions must not match. + "# TODO", + "* FIXME check this", + "names TODO and FIXME markers as a read", + "a codebase dense with TODO markers", + "the TODO and FIXME conventions this project follows", } for _, line := range match { if convMarkerRe.FindStringSubmatch(line) == nil { @@ -463,7 +495,7 @@ func TestConvMarkerRePinsTheRecognisedSpellings(t *testing.T) { } for _, line := range reject { if m := convMarkerRe.FindStringSubmatch(line); m != nil { - t.Errorf("convMarkerRe matches %q as a %s marker", line, m[2]) + t.Errorf("convMarkerRe matches %q as a %s marker", line, convMarkerName(m)) } } }