diff --git a/src/memory.js b/src/memory.js index 7e2bb73..0e3751b 100644 --- a/src/memory.js +++ b/src/memory.js @@ -10,8 +10,10 @@ import { UserError } from "./logger.js"; * Memory files are the weights. To talk about them precisely, backpass parses each * file into addressable instruction units (design section 3.1): * - * - sections come from markdown headings - * - units come from list items and paragraphs inside those sections + * - sections come from markdown headings (ATX `#`, and a `---` setext underline) + * - units come from list items and paragraphs inside those sections; a thematic + * break (`---`, `***`, `___`) outside a fence ends the current unit and is never + * a unit itself * - each unit gets a content hash (survives cosmetic edits elsewhere in the file) * and a readable alias AG-001, AG-002, ... used in prompts and evidence * - eligible prose above ATTRIBUTION_SPLIT_TOKENS is conservatively split at @@ -25,6 +27,9 @@ import { UserError } from "./logger.js"; export const ATTRIBUTION_SPLIT_TOKENS = 120; const FENCE = /^\s*(```|~~~)/; +/** CommonMark thematic break: 3+ of one of `-*_`, optional spaces between, up to 3 leading spaces. */ +export const THEMATIC_BREAK = /^ {0,3}([-*_])(?:[ \t]*\1){2,}[ \t]*$/; +const LIST_ITEM = /^\s*([-*+]|\d+[.)])\s+/; function normalizeForHash(text) { return text @@ -100,7 +105,31 @@ export function parseMemoryUnits(text) { continue; } - const isListItem = /^\s*([-*+]|\d+[.)])\s+/.test(line); + // A break-only line is never an instruction. Directly under a plain paragraph a + // `---` run is a setext H2 underline instead, so that paragraph is a heading, as + // in CommonMark; under a list item, quote, indented code, or fence it stays a + // break. Checked before list items because `* * *` and `- - -` are breaks, not items. + if (THEMATIC_BREAK.test(line)) { + const setext = + /^ {0,3}-+[ \t]*$/.test(line) && + buffer.length && + /^ {0,3}\S/.test(buffer[0]) && + !LIST_ITEM.test(buffer[0]) && + !/^\s*>/.test(buffer[0]) && + !buffer.some((bufferLine) => FENCE.test(bufferLine)); + if (setext) { + const text = buffer.map((bufferLine) => bufferLine.trim()).join(" "); + buffer = []; + headings.length = Math.min(headings.length, 1); + headings[1] = text; + headings[0] = headings[0] ?? ""; + } else { + flush(i); + } + continue; + } + + const isListItem = LIST_ITEM.test(line); const isBlank = line.trim() === ""; if (isBlank) { @@ -108,7 +137,7 @@ export function parseMemoryUnits(text) { continue; } // A new list item starts a new unit; continuation lines stay with it. - if (isListItem && buffer.length && /^\s*([-*+]|\d+[.)])\s+/.test(buffer[0])) { + if (isListItem && buffer.length && LIST_ITEM.test(buffer[0])) { flush(i); } if (!buffer.length) bufferStart = i; diff --git a/src/state.js b/src/state.js index d7334c4..74ab4c9 100644 --- a/src/state.js +++ b/src/state.js @@ -248,7 +248,7 @@ function migrateEvidenceRecord(record, transcript, identity) { }; } -export const ANALYSIS_INDEX_VERSION = 3; +export const ANALYSIS_INDEX_VERSION = 4; /** * Cache key for a transcript's analysis: its content signature, the memory-surface hash, diff --git a/src/workspace.js b/src/workspace.js index 5537ee7..7cce147 100644 --- a/src/workspace.js +++ b/src/workspace.js @@ -4,7 +4,7 @@ import path from "node:path"; import { canonicalizeSearchPathRoots } from "./config.js"; import { anchoredHunks, countOccurrences, span } from "./diff.js"; import { warn } from "./logger.js"; -import { parseMemoryUnits, readOnlyResolvedPath, resolveMemoryPath } from "./memory.js"; +import { THEMATIC_BREAK, parseMemoryUnits, readOnlyResolvedPath, resolveMemoryPath } from "./memory.js"; import { isDirectoryEntry, parseFrontmatter, skillBody } from "./skills.js"; import { sha256 } from "./state.js"; @@ -401,7 +401,9 @@ export function splitRemovalHunk(hunk, { oldText, oldLines, recovered }) { if (remaining > 0) recovered.set(normalized, remaining - 1); } + const unitLines = new Set(); for (const unit of parseMemoryUnits(oldText)) { + for (let lineNo = unit.startLine; lineNo <= unit.endLine; lineNo += 1) unitLines.add(lineNo); const start = Math.max(unit.startLine, hunk.oldStart); const end = Math.min(unit.endLine, hunk.oldEnd); if (start > end) continue; @@ -412,12 +414,35 @@ export function splitRemovalHunk(hunk, { oldText, oldLines, recovered }) { if (kinds.size > 1) return null; } + const isAtxHeading = (lineNo) => /^#{1,6}\s+/.test(oldLines[lineNo - 1] || ""); + const isSetextTitle = (lineNo) => + lineNo >= 1 && + Boolean(normalizeRecoveryLine(oldLines[lineNo - 1])) && + !unitLines.has(lineNo) && + !isAtxHeading(lineNo) && + !THEMATIC_BREAK.test(oldLines[lineNo - 1]); + + // A heading (ATX, or a setext title with its dash underline) stays with the content it + // introduces; a plain thematic break introduces nothing. for (let lineNo = hunk.oldStart; lineNo <= hunk.oldEnd; lineNo += 1) { - if (!/^#{1,6}\s+/.test(oldLines[lineNo - 1] || "")) continue; + let headingStart = lineNo; + if (!isAtxHeading(lineNo)) { + if (unitLines.has(lineNo) || !/^ {0,3}-{3,}[ \t]*$/.test(oldLines[lineNo - 1]) || !isSetextTitle(lineNo - 1)) { + continue; + } + while (isSetextTitle(headingStart - 1)) headingStart -= 1; + } let contentLine = lineNo + 1; - while (contentLine <= hunk.oldEnd && !normalizeRecoveryLine(oldLines[contentLine - 1])) contentLine += 1; - if (contentLine > hunk.oldEnd || /^#{1,6}\s+/.test(oldLines[contentLine - 1] || "")) continue; - if (lineKinds.get(lineNo) !== lineKinds.get(contentLine)) return null; + while ( + contentLine <= hunk.oldEnd && + (!lineKinds.has(contentLine) || THEMATIC_BREAK.test(oldLines[contentLine - 1])) + ) { + contentLine += 1; + } + if (contentLine > hunk.oldEnd || !unitLines.has(contentLine)) continue; + for (let headingLine = headingStart; headingLine <= lineNo; headingLine += 1) { + if (lineKinds.has(headingLine) && lineKinds.get(headingLine) !== lineKinds.get(contentLine)) return null; + } } // Group the removed lines into maximal runs by recovery; blank lines never start a diff --git a/test/memory-units.test.js b/test/memory-units.test.js new file mode 100644 index 0000000..6cc0f7b --- /dev/null +++ b/test/memory-units.test.js @@ -0,0 +1,78 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +import { parseMemoryUnits } from "../src/memory.js"; +import { crossSurfaceDuplicates } from "../src/overlap.js"; + +const texts = (text) => parseMemoryUnits(text).map((unit) => unit.text); + +test("a thematic break is not a unit and keeps the text around it apart", () => { + const units = parseMemoryUnits( + "# Guide\n\nRun the linter before you commit.\n\n---\n\n## Tests\n\nRun the unit tests with `make test`.\n\n---\n\nKeep commits small.\n", + ); + assert.deepEqual( + units.map((unit) => [unit.id, unit.text]), + [ + ["AG-001", "Run the linter before you commit."], + ["AG-002", "Run the unit tests with `make test`."], + ["AG-003", "Keep commits small."], + ], + ); + assert.equal(units[2].section, "Guide > Tests"); +}); + +test("CommonMark thematic break variants are skipped and flush the current unit", () => { + for (const rule of ["***", "___", "- - -", "* * *", "_ _ _", " ----", "----- ", "*\t*\t*"]) { + assert.deepEqual(texts(`- First rule.\n${rule}\n- Second rule.\n`), ["- First rule.", "- Second rule."], rule); + assert.deepEqual(texts(`First rule.\n\n${rule}\nSecond rule.\n`), ["First rule.", "Second rule."], rule); + } +}); + +test("near-misses of a thematic break stay content", () => { + assert.deepEqual(texts("Rule one.\n\n--\n"), ["Rule one.", "--"]); + assert.deepEqual(texts("Rule one.\n\n ---\n"), ["Rule one.", " ---"], "four leading spaces is code"); + assert.deepEqual(texts("Rule one.\n\n-*-\n"), ["Rule one.", "-*-"], "mixed characters are not a break"); +}); + +test("a list item is still a unit", () => { + assert.deepEqual(texts("# Rules\n\n- item\n- another item\n"), ["- item", "- another item"]); +}); + +test("a thematic break inside a fence is preserved as content", () => { + const fenced = "Example frontmatter:\n```yaml\n---\nname: x\n---\n***\n```"; + assert.deepEqual(texts(`# Rules\n\n${fenced}\n`), [fenced]); +}); + +test("a dash run directly under a paragraph is a setext heading, not a unit", () => { + const units = parseMemoryUnits("# Guide\n\nRelease steps\n---\n\nTag the commit.\n\n## Other\n\nShip it.\n"); + assert.deepEqual( + units.map((unit) => [unit.text, unit.section]), + [ + ["Tag the commit.", "Guide > Release steps"], + ["Ship it.", "Guide > Other"], + ], + ); +}); + +test("a dash run under a list item, quote, indented code, or fence is a break, not a setext underline", () => { + assert.deepEqual(texts("- Keep it small.\n---\nNext.\n"), ["- Keep it small.", "Next."]); + assert.deepEqual(texts("> Quoted rule.\n---\nNext.\n"), ["> Quoted rule.", "Next."]); + assert.deepEqual(texts("```\ncode\n```\n---\nNext.\n"), ["```\ncode\n```", "Next."]); + assert.deepEqual(texts(" npm test\n---\nNext.\n"), [" npm test", "Next."], "indented code is not a title"); + assert.deepEqual(texts("\tnpm test\n---\nNext.\n"), ["\tnpm test", "Next."], "tab-indented code is not a title"); + assert.deepEqual(texts("Paragraph.\n***\nNext.\n"), ["Paragraph.", "Next."], "only dashes underline a heading"); +}); + +test("a shared thematic break is not a cross-surface overlap", () => { + const memoryFile = { + path: "AGENTS.md", + units: parseMemoryUnits("# Guide\n\nRun the linter.\n\n---\n\nKeep commits small.\n"), + }; + const skill = { + name: "demo", + path: ".agents/skills/demo/SKILL.md", + description: "Use when releasing.", + body: "# Release\n\nTag the commit, then push the tag.\n\n---\n\nWrite the changelog entry.", + }; + assert.deepEqual(crossSurfaceDuplicates(memoryFile, [skill]), []); +}); diff --git a/test/status.test.js b/test/status.test.js index 3ac08b8..0a8aebe 100644 --- a/test/status.test.js +++ b/test/status.test.js @@ -103,3 +103,19 @@ test("status identifies body overlap as triggered placement evidence", async () assert.match(text, /AG-001 restates database body.*body loads on trigger; weigh placement/); assert.doesNotMatch(text, /duplicated always loaded/); }); + +test("status neither counts nor reports thematic breaks as instructions (#192)", async () => { + const repo = makeRepo({ + "AGENTS.md": + "# Guide\n\nRun the linter before you commit.\n\n---\n\n## Tests\n\nRun the unit tests with `make test`.\n\n---\n\nKeep commits small.\n", + ".agents/skills/demo/SKILL.md": + "---\nname: demo\ndescription: Use when releasing.\n---\n\n# Release\n\nTag the commit, then push the tag.\n\n---\n\nWrite the changelog entry.\n", + }); + + const structured = JSON.parse((await captureStatus(repo, true)).join("\n")); + assert.deepEqual(structured.crossSurfaceDuplicates, []); + + const text = (await captureStatus(repo, false)).join("\n"); + assert.match(text, /ยท 3 instructions/); + assert.doesNotMatch(text, /restates demo/); +}); diff --git a/test/workspace.test.js b/test/workspace.test.js index 9ab2f6e..d186293 100644 --- a/test/workspace.test.js +++ b/test/workspace.test.js @@ -19,7 +19,9 @@ import { parseSkillFile, prepareWorkspace, repoFingerprint, + recoveredLineCounts, skillStagingRefusal, + splitRemovalHunk, workspacePathFor, } from "../src/workspace.js"; import { UserError } from "../src/logger.js"; @@ -872,3 +874,102 @@ test("project scope bills a skill symlinked out of the repo but never stages it" assert.equal(external.originals.get(".agents/skills/db/SKILL.md"), SKILL); assert.ok(external.originals.has(".agents/skills/solo.md")); }); + +test("a setext heading stays with the section it introduces when a removal is split", () => { + const oldText = [ + "# Guide", + "", + "Keep it small.", + "", + "Release steps", + "---", + "", + "Tag the commit.", + "", + "## Other", + "", + "Stay.", + "", + ].join("\n"); + const oldLines = oldText.split("\n"); + const skill = "---\nname: release\ndescription: Release steps.\n---\n\n# Release\n\nTag the commit.\n"; + const hunk = { oldStart: 5, oldEnd: 9 }; + assert.equal(splitRemovalHunk(hunk, { oldText, oldLines, recovered: recoveredLineCounts([skill]) }), null); +}); + +test("a plain thematic break never ties an extraction to the deletion after it", () => { + const skill = + "---\nname: release\ndescription: Release steps.\n---\n\n## Release\n\nTag the commit before release.\n"; + const split = (lines, hunk) => { + const oldText = lines.join("\n"); + return splitRemovalHunk(hunk, { oldText, oldLines: oldText.split("\n"), recovered: recoveredLineCounts([skill]) }); + }; + const afterParagraph = split( + [ + "# Guide", + "", + "Keep it small.", + "", + "Tag the commit before release.", + "", + "---", + "", + "Drop this stale note.", + "", + "## Other", + "", + "Stay.", + "", + ], + { oldStart: 5, oldEnd: 10 }, + ); + assert.deepEqual( + afterParagraph?.map((sub) => sub.find), + ["Tag the commit before release.\n\n---\n", "\nDrop this stale note.\n\n"], + ); + const afterHeading = split( + [ + "# Guide", + "", + "Keep it small.", + "", + "## Release", + "---", + "", + "Tag the commit before release.", + "", + "Drop this stale note.", + "", + "## Other", + "", + "Stay.", + "", + ], + { oldStart: 5, oldEnd: 11 }, + ); + assert.equal(afterHeading?.length, 2); +}); + +test("a thematic break between a heading and its section never lets an extraction orphan the heading", () => { + const oldText = [ + "# Guide", + "", + "Keep it small.", + "", + "## Notes", + "---", + "", + "Tag the commit before release.", + "", + "## Other", + "", + "Stay.", + "", + ].join("\n"); + const skill = "---\nname: release\ndescription: Release steps.\n---\n\n# Release\n\nTag the commit before release.\n"; + const hunk = { oldStart: 5, oldEnd: 9 }; + assert.equal( + splitRemovalHunk(hunk, { oldText, oldLines: oldText.split("\n"), recovered: recoveredLineCounts([skill]) }), + null, + ); +});