From c3151b3f416a8072a73977c29af76cbfe76ebf68 Mon Sep 17 00:00:00 2001 From: Kodai Nakamura Date: Mon, 22 Jun 2026 01:33:56 +0900 Subject: [PATCH] fix(replace-links): align candidate scanner semantics Why: - Task 1 scanner drifted from replaceLinks in two places that affect AI ambiguity review. - Trie-hit namespace handling was filtering candidate sets differently from replacement-time behavior. - Protected-region scanning was still walking fenced code, callouts, and ignored headings that replaceLinks already shields. What: - make trie-hit scanning use the original candidate set and first-candidate scoped checks, matching current replaceLinks behavior - skip fenced code blocks, callouts, and ignored headings before protected-regex scanning while preserving original occurrence offsets - add regressions for trie-hit namespace semantics and protected-region skipping, plus AI-side coverage and append the fix report --- .superpowers/sdd/task-1-report.md | 169 ++++++++++++ .../__tests__/candidate-scanner.test.ts | 84 +++++- src/replace-links/candidate-scanner.ts | 240 ++++++++++++------ .../__tests__/resolve-ambiguities.test.ts | 36 +++ 4 files changed, 450 insertions(+), 79 deletions(-) create mode 100644 .superpowers/sdd/task-1-report.md diff --git a/.superpowers/sdd/task-1-report.md b/.superpowers/sdd/task-1-report.md new file mode 100644 index 0000000..1cdf26d --- /dev/null +++ b/.superpowers/sdd/task-1-report.md @@ -0,0 +1,169 @@ +# Task 1 Report: Candidate Scanning Module + +## Scope + +Implemented Task 1 from `.superpowers/sdd/task-1-brief.md`: + +- Created `src/replace-links/candidate-scanner.ts` +- Created `src/replace-links/__tests__/candidate-scanner.test.ts` +- Updated `src/replace-links/replace-links.ts` +- Updated `src/utils/resolve-ambiguities.ts` + +No AGENTS.md changes were needed. + +## TDD Evidence + +### RED + +Command: + +```bash +npx vitest run src/replace-links/__tests__/candidate-scanner.test.ts +``` + +Result: + +- Failed as expected. +- Failure cause: `Cannot find module '../candidate-scanner'`. + +### GREEN + +Command: + +```bash +npx vitest run src/replace-links/__tests__/candidate-scanner.test.ts +``` + +Result: + +- Passed: `4/4` tests. + +## Implementation Summary + +- Extracted shared scanning helpers from `replace-links.ts` into `candidate-scanner.ts`. +- Added `scanCandidateOccurrences()` for: + - unlinked candidate occurrences + - existing wikilink occurrences +- Added `getOccurrenceContext()` for bounded AI request context. +- Preserved `replaceLinks()` rendering behavior by importing the moved helpers back into `replace-links.ts`. +- Routed `resolveAmbiguities()` through the shared scanner instead of maintaining separate trie scanning logic. + +## Focused Verification + +Command: + +```bash +npx vitest run src/utils/__tests__/resolve-ambiguities.test.ts src/replace-links/__tests__/ai-disambiguation.test.ts src/replace-links/__tests__/replace-links.basic.test.ts +``` + +Result: + +- Passed: `19/19` tests across `3/3` files. + +## Full Stage Verification + +Commands: + +```bash +npm run test -- --reporter=dot +npm run tsc +npm run lint +``` + +Results: + +- `npm run test -- --reporter=dot`: passed, `320/320` tests across `38/38` files +- `npm run tsc`: passed +- `npm run lint`: passed + +Note: + +- The first full-suite run hit a transient failure in the namespace-resolution performance test. +- Re-running the full stage verification passed cleanly, including that test (`Namespace resolution processed in 290.97ms`, below the `300ms` threshold). + +## Commit + +Created commit: + +- `953688d` `refactor(replace-links): centralize candidate scanning` + +## Files Changed + +- `src/replace-links/candidate-scanner.ts` +- `src/replace-links/__tests__/candidate-scanner.test.ts` +- `src/replace-links/replace-links.ts` +- `src/utils/resolve-ambiguities.ts` + +## Brief Compliance Check + +- Used the exact Task 1 file set from the brief. +- Followed RED then GREEN before production code claims. +- Ran the focused tests from the brief. +- Ran the full verification commands from the brief. +- Used the specified Conventional Commit message with `Why` and `What`. + +## Review Fix 1 + +### Scope + +Fixed the Task 1 review findings for candidate scanning drift: + +- aligned trie-hit namespace handling in `scanCandidateOccurrences()` with current `replaceLinks()` behavior +- aligned scanner protected-region handling with current `replaceLinks()` block-level shielding for fenced code blocks, callouts, and ignored headings +- added regression coverage for both drift cases + +No AGENTS.md changes were needed. + +### TDD Evidence + +#### RED + +Commands: + +```bash +npx vitest run src/replace-links/__tests__/candidate-scanner.test.ts +npx vitest run src/utils/__tests__/resolve-ambiguities.test.ts +``` + +Results: + +- `candidate-scanner.test.ts` failed on the new trie-hit namespace semantics regression. +- `candidate-scanner.test.ts` failed on the new fenced-code/callout/ignored-heading regression. +- `resolve-ambiguities.test.ts` failed on the new protected-region regression. + +#### GREEN + +Commands: + +```bash +npx vitest run src/replace-links/__tests__/candidate-scanner.test.ts +npx vitest run src/utils/__tests__/resolve-ambiguities.test.ts src/replace-links/__tests__/ai-disambiguation.test.ts src/replace-links/__tests__/replace-links.callout.test.ts src/replace-links/__tests__/replace-links.headings.test.ts src/replace-links/__tests__/replace-links.code.test.ts +``` + +Results: + +- `candidate-scanner.test.ts`: passed `6/6` +- focused AI/replacement suite: passed `47/47` across `5/5` files + +### Implementation Summary + +- Reworked scanner block protection to skip the same fenced code blocks, callouts, and optional heading regions that `replaceLinks()` excludes before protected-regex scanning. +- Preserved absolute occurrence offsets while scanning only unprotected regions. +- Changed trie-hit scanner behavior to keep the original candidate set and use the first candidate for scoped/self-link checks, matching current `replaceLinks()` semantics. +- Updated scanner regressions to reflect current trie-hit behavior and added AI-side coverage to ensure protected regions are not scanned. + +### Full Verification + +Commands: + +```bash +npm run test -- --reporter=dot +npm run tsc +npm run lint +``` + +Results: + +- `npm run test -- --reporter=dot`: passed, `323/323` tests across `38/38` files +- `npm run tsc`: passed +- `npm run lint`: passed diff --git a/src/replace-links/__tests__/candidate-scanner.test.ts b/src/replace-links/__tests__/candidate-scanner.test.ts index 5aa386f..844afe8 100644 --- a/src/replace-links/__tests__/candidate-scanner.test.ts +++ b/src/replace-links/__tests__/candidate-scanner.test.ts @@ -1,4 +1,5 @@ import { describe, expect, it } from "vitest" +import { buildTrie, CandidateData } from "../../trie" import { buildCandidateTrieForTest } from "./test-helpers" import { getOccurrenceContext, @@ -80,7 +81,7 @@ describe("scanCandidateOccurrences", () => { ]) }) - it("filters scoped candidates outside the current namespace", () => { + it("preserves trie-hit candidate sets for scoped candidates in the current namespace", () => { const { candidateMap, trie } = buildCandidateTrieForTest({ files: [ { path: "pages/team-a/internal" }, @@ -108,6 +109,87 @@ describe("scanCandidateOccurrences", () => { expect(occurrences).toHaveLength(1) expect(occurrences[0].candidateData.candidates.map(c => c.canonical)).toEqual([ "pages/team-a/internal", + "pages/team-b/internal", + ]) + }) + + it("matches replaceLinks trie-hit namespace semantics", () => { + const candidateMap = new Map([ + [ + "internal", + { + candidates: [ + { + canonical: "pages/team-b/internal", + scoped: true, + namespace: "team-b", + }, + { + canonical: "pages/team-a/internal", + scoped: true, + namespace: "team-a", + }, + ], + }, + ], + ]) + const trie = buildTrie(["internal"], true) + + const occurrences = scanCandidateOccurrences({ + text: "internal", + filePath: "pages/team-a/today", + trie, + candidateMap, + settings: { + baseDir: "pages", + ignoreCase: true, + proximityBasedLinking: true, + }, + }) + + expect(occurrences).toEqual([]) + }) + + it("skips fenced code blocks, callouts, and ignored headings", () => { + const { candidateMap, trie } = buildCandidateTrieForTest({ + files: [{ path: "meeting" }], + settings: { + scoped: false, + baseDir: undefined, + ignoreCase: true, + }, + }) + + const occurrences = scanCandidateOccurrences({ + text: `# meeting +> [!note] +> meeting + +~~~ts +meeting +~~~ + +meeting`, + filePath: "notes/today", + trie, + candidateMap, + settings: { + ignoreCase: true, + ignoreHeadings: true, + proximityBasedLinking: true, + }, + }) + + expect(occurrences.map(o => ({ + start: o.start, + end: o.end, + text: o.text, + }))).toEqual([ + { + start: 50, + end: 57, + text: "meeting", + }, ]) }) }) diff --git a/src/replace-links/candidate-scanner.ts b/src/replace-links/candidate-scanner.ts index 2da65a3..27c7b59 100644 --- a/src/replace-links/candidate-scanner.ts +++ b/src/replace-links/candidate-scanner.ts @@ -144,27 +144,18 @@ export const extractLinkParts = ( export const escapeRegExp = (text: string): string => text.replace(/[.*+?^${}()|[\]\\]/g, "\\$&") -export const extractFencedCodeBlocks = ( +const findFencedCodeBlockRanges = ( body: string, -): { body: string, codeBlocks: Array<{ placeholder: string, content: string }> } => { +): Array<{ start: number, end: number }> => { if (!body.includes("```") && !body.includes("~~~")) { - return { body, codeBlocks: [] } + return [] } - const codeBlocks: Array<{ placeholder: string, content: string }> = [] - let result = "" - let cursor = 0 - let codeBlockIndex = 0 - + const ranges: Array<{ start: number, end: number }> = [] const openingFencePattern = /^ {0,3}(`{3,}|~{3,})[^\r\n]*(?:\r?\n|$)/gm let openingMatch: RegExpExecArray | null while ((openingMatch = openingFencePattern.exec(body)) !== null) { - const start = openingMatch.index - if (start < cursor) { - continue - } - const openingFence = openingMatch[1] const fenceChar = openingFence[0] const fenceLength = openingFence.length @@ -178,15 +169,35 @@ export const extractFencedCodeBlocks = ( ? closingMatch.index + closingMatch[0].length : body.length + ranges.push({ + start: openingMatch.index, + end, + }) + openingFencePattern.lastIndex = end + } + + return ranges +} + +export const extractFencedCodeBlocks = ( + body: string, +): { body: string, codeBlocks: Array<{ placeholder: string, content: string }> } => { + if (!body.includes("```") && !body.includes("~~~")) { + return { body, codeBlocks: [] } + } + + const ranges = findFencedCodeBlockRanges(body) + const codeBlocks: Array<{ placeholder: string, content: string }> = [] + let result = "" + let cursor = 0 + for (const [codeBlockIndex, range] of ranges.entries()) { const placeholder = `__CODE_BLOCK_${codeBlockIndex}__` codeBlocks.push({ placeholder, - content: body.slice(start, end), + content: body.slice(range.start, range.end), }) - result += body.slice(cursor, start) + placeholder - cursor = end - codeBlockIndex++ - openingFencePattern.lastIndex = end + result += body.slice(cursor, range.start) + placeholder + cursor = range.end } result += body.slice(cursor) @@ -345,20 +356,57 @@ const dedupeCandidates = (candidates: CandidateData["candidates"]): CandidateDat } } -const filterCandidateDataForNamespace = ( - candidateData: CandidateData, - currentNamespace: string, +const findProtectedBlockRanges = ( + text: string, settings: ReplaceLinksSettings, -): CandidateData => { - if (!settings.proximityBasedLinking) { - return dedupeCandidates(candidateData.candidates) +): Array<{ start: number, end: number }> => { + const ranges: Array<{ start: number, end: number }> = [ + ...findFencedCodeBlockRanges(text), + ] + + if (settings.ignoreHeadings) { + const headingPattern = /^#{1,6}\s+.*$/gm + let headingMatch: RegExpExecArray | null + + while ((headingMatch = headingPattern.exec(text)) !== null) { + ranges.push({ + start: headingMatch.index, + end: headingMatch.index + headingMatch[0].length, + }) + } } - const filteredCandidates = candidateData.candidates.filter((candidate) => { - return !(candidate.scoped && candidate.namespace !== currentNamespace) - }) + const calloutPattern = /^>[ \t]*\[![\w-]+\].*?(\n>.*?)*(?=\n(?!>)|$)/gm + let calloutMatch: RegExpExecArray | null - return dedupeCandidates(filteredCandidates) + while ((calloutMatch = calloutPattern.exec(text)) !== null) { + ranges.push({ + start: calloutMatch.index, + end: calloutMatch.index + calloutMatch[0].length, + }) + } + + ranges.sort((a, b) => a.start - b.start) + + const merged: Array<{ start: number, end: number }> = [] + for (const range of ranges) { + const lastRange = merged[merged.length - 1] + if (!lastRange || range.start > lastRange.end) { + merged.push({ ...range }) + continue + } + + lastRange.end = Math.max(lastRange.end, range.end) + } + + return merged +} + +const isIndexProtected = ( + index: number, + protectedRanges: Array<{ start: number, end: number }>, +): boolean => { + return protectedRanges.some(range => index >= range.start && index < range.end) } const collectExistingWikilinks = ( @@ -366,11 +414,16 @@ const collectExistingWikilinks = ( candidateMap: Map, occurrences: CandidateOccurrence[], settings: ReplaceLinksSettings, + protectedRanges: Array<{ start: number, end: number }>, ): void => { const existingLinkRegex = /\[\[([^|\]]+)(?:\|([^\]]+))?\]\]/g let match: RegExpExecArray | null while ((match = existingLinkRegex.exec(text)) !== null) { + if (isIndexProtected(match.index, protectedRanges)) { + continue + } + const fullMatch = match[0] const path = match[1] const alias = match[2] || path @@ -581,18 +634,7 @@ const collectUnlinkedOccurrences = ({ const candidateData = candidateMap.get(trieCandidateKey) if (candidateData) { - const filteredCandidateData = filterCandidateDataForNamespace( - candidateData, - currentNamespace, - settings, - ) - - if (filteredCandidateData.candidates.length === 0) { - i += candidate.length - continue - } - - if (isSelfLink(filteredCandidateData, filePath, settings)) { + if (isSelfLink(candidateData, filePath, settings)) { i += candidate.length continue } @@ -610,13 +652,23 @@ const collectUnlinkedOccurrences = ({ } } + if ( + settings.proximityBasedLinking + && candidateData.candidates.length > 0 + && candidateData.candidates[0].scoped + && candidateData.candidates[0].namespace !== currentNamespace + ) { + i += candidate.length + continue + } + occurrences.push({ kind: "unlinked", start: i, end: i + candidate.length, text: candidate, candidateKey: trieCandidateKey, - candidateData: filteredCandidateData, + candidateData: dedupeCandidates(candidateData.candidates), isInTable: isIndexInsideMarkdownTable(text, i), }) i += candidate.length @@ -655,60 +707,92 @@ export const scanCandidateOccurrences = ({ const normalizedText = text.normalize("NFC") const fallbackIndex = buildFallbackIndex(candidateMap, settings.ignoreCase) const currentNamespace = getCurrentNamespace(filePath, settings.baseDir) + const protectedRanges = findProtectedBlockRanges(normalizedText, settings) - collectExistingWikilinks(normalizedText, candidateMap, occurrences, settings) + collectExistingWikilinks( + normalizedText, + candidateMap, + occurrences, + settings, + protectedRanges, + ) - const protectedFreeOccurrences: CandidateOccurrence[] = [] - REGEX_PATTERNS.PROTECTED.lastIndex = 0 - let lastIndex = 0 - let match: RegExpExecArray | null - while ((match = REGEX_PATTERNS.PROTECTED.exec(normalizedText)) !== null) { - const segment = normalizedText.slice(lastIndex, match.index) - const segmentOccurrences: CandidateOccurrence[] = [] + const scanUnprotectedSegment = ( + segment: string, + offset: number, + segmentOccurrences: CandidateOccurrence[], + ): void => { + REGEX_PATTERNS.PROTECTED.lastIndex = 0 + let lastProtectedIndex = 0 + let match: RegExpExecArray | null + + while ((match = REGEX_PATTERNS.PROTECTED.exec(segment)) !== null) { + const unprotectedSegment = segment.slice(lastProtectedIndex, match.index) + const nestedOccurrences: CandidateOccurrence[] = [] + collectUnlinkedOccurrences({ + text: unprotectedSegment, + filePath, + trie, + candidateMap, + fallbackIndex, + currentNamespace, + settings, + occurrences: nestedOccurrences, + }) + for (const occurrence of nestedOccurrences) { + segmentOccurrences.push({ + ...occurrence, + start: occurrence.start + offset + lastProtectedIndex, + end: occurrence.end + offset + lastProtectedIndex, + }) + } + lastProtectedIndex = match.index + match[0].length + + if (match[0].length === 0) { + REGEX_PATTERNS.PROTECTED.lastIndex++ + } + } + + const nestedOccurrences: CandidateOccurrence[] = [] collectUnlinkedOccurrences({ - text: segment, + text: segment.slice(lastProtectedIndex), filePath, trie, candidateMap, fallbackIndex, currentNamespace, settings, - occurrences: segmentOccurrences, + occurrences: nestedOccurrences, }) - for (const occurrence of segmentOccurrences) { - protectedFreeOccurrences.push({ + for (const occurrence of nestedOccurrences) { + segmentOccurrences.push({ ...occurrence, - start: occurrence.start + lastIndex, - end: occurrence.end + lastIndex, + start: occurrence.start + offset + lastProtectedIndex, + end: occurrence.end + offset + lastProtectedIndex, }) } - lastIndex = match.index + match[0].length + } - if (match[0].length === 0) { - REGEX_PATTERNS.PROTECTED.lastIndex++ - } + const protectedFreeOccurrences: CandidateOccurrence[] = [] + let lastIndex = 0 + for (const range of protectedRanges) { + const segmentOccurrences: CandidateOccurrence[] = [] + scanUnprotectedSegment( + normalizedText.slice(lastIndex, range.start), + lastIndex, + segmentOccurrences, + ) + protectedFreeOccurrences.push(...segmentOccurrences) + lastIndex = range.end } const tailOccurrences: CandidateOccurrence[] = [] - collectUnlinkedOccurrences({ - text: normalizedText.slice(lastIndex), - filePath, - trie, - candidateMap, - fallbackIndex, - currentNamespace, - settings, - occurrences: tailOccurrences, - }) - for (const occurrence of tailOccurrences) { - protectedFreeOccurrences.push({ - ...occurrence, - start: occurrence.start + lastIndex, - end: occurrence.end + lastIndex, - }) - } - - occurrences.push(...protectedFreeOccurrences) + scanUnprotectedSegment( + normalizedText.slice(lastIndex), + lastIndex, + tailOccurrences, + ) + occurrences.push(...protectedFreeOccurrences, ...tailOccurrences) return occurrences.sort((a, b) => a.start - b.start) } diff --git a/src/utils/__tests__/resolve-ambiguities.test.ts b/src/utils/__tests__/resolve-ambiguities.test.ts index 36e7196..5dcafc3 100644 --- a/src/utils/__tests__/resolve-ambiguities.test.ts +++ b/src/utils/__tests__/resolve-ambiguities.test.ts @@ -73,4 +73,40 @@ describe("resolveAmbiguities", () => { ) expect(result.get("[[private/meeting|meeting]]")).toBe("work/meeting") }) + + it("should skip fenced code blocks, callouts, and ignored headings", async () => { + const text = `# meeting +> [!note] +> meeting + +~~~ts +meeting +~~~ + +meeting` + vi.mocked(aiClient.resolveAmbiguitiesBatch).mockClear() + vi.mocked(aiClient.resolveAmbiguitiesBatch).mockResolvedValue( + new Map([["meeting", "work/meeting"]]), + ) + + await resolveAmbiguities(text, candidateMap, trie, { + ...mockSettings, + ignoreHeadings: true, + proximityBasedLinking: true, + }) + + expect(aiClient.resolveAmbiguitiesBatch).toHaveBeenCalledTimes(1) + expect(aiClient.resolveAmbiguitiesBatch).toHaveBeenCalledWith( + expect.objectContaining({ + ignoreHeadings: true, + proximityBasedLinking: true, + }), + [ + expect.objectContaining({ + word: "meeting", + candidates: ["work/meeting", "private/meeting"], + }), + ], + ) + }) })