Skip to content

refactor(mcp): read devbook notes under one chapter spelling - #857

Merged
JSdotNet merged 1 commit into
mainfrom
schedule/issue-sweep/2026-09-30/574-canonical-annotation-paths
Oct 1, 2026
Merged

JSdotNet merged 1 commit into
mainfrom
schedule/issue-sweep/2026-09-30/574-canonical-annotation-paths

Conversation

@JSdotNet

Copy link
Copy Markdown
Owner

Closes #574

What changed

I finished item 4 of issue #574. The MCP tools read_knowledge_chapter (review mode) and list_annotations used to ask the annotation store twice, once under the repository-relative path and once under the area-relative path, and merge the results by id. They now ask once, with the normalized repository-relative path. The store's own DevbookChapterKey.Canonical handles every other spelling of the chapter.

Source changes:

  • ChapterReading.Spellings is gone, along with its "panels disagree" comment.
  • ChapterReading.Notes now takes one chapter path. It still drops drafts and still orders by BlockIndex, then CreatedAt.
  • In DevbookTools, list_annotations no longer calls ChapterPaths.Split. That call only existed to build the second spelling, so its comment was rewritten. read_knowledge_chapter passes normalized only.

Tests, written first and seen failing for the right reason (5 failures, each showing Listed with two entries such as [".arc42/adr/0012-mcp.md", "adr/0012-mcp.md"]):

  • FakeDevbookAnnotationStore now runs both the stored path and the query path through DevbookChapterKey.Canonical, using an optional Folders list. It matches the way the real store keys notes.
  • The old either-spelling test was replaced by single-query tests for both tools, plus a 5-row theory. It covers notes filed under .devbook/arc42/..., docs/arch/... and .arc42/... and asked for under another spelling.
  • DevbookChapterKeyTests gained two tests. One shows that a bare adr/x.md is claimed by no area wherever arc42 is pointed, so it is not migrated. The other loads the real Arc42DevbookReader catalog for the .arc42, .devbook/arc42, docs/arch and off-clone layouts, and shows that each document path carries its folder and canonicalizes to .arc42/adr/....
  • DevbookAnnotationStoreTests gained a test for a remark stored under a folderless key. After a reload it keeps its key and its stamps, and it adds nothing to ListChangedSince.

Why no stored note is lost: git history shows that the arc42 panel has filed remarks under the catalog's document path since the store was introduced (b1c3eba). The catalog has always spelled that path relative to the repository root, or to the folder's parent (ResolveRoot), so it always includes the folder. The bare adr/x.md spelling the issue and the old Spellings comment described was never actually written by the product.

Results:

  • Backlog.Infrastructure.Mcp.UnitTests: 160 of 160 passed.
  • Backlog.Infrastructure.FileSystem.UnitTests: 519 of 519 passed.
  • Backlog.Desktop.UI.UnitTests: 3243 of 3243 passed.
  • A --no-incremental build of the three test projects and their dependencies gave 0 warnings and 0 errors.
  • No code change to DevbookAnnotationStore, SessionDevbookAnnotationStore or DevbookChapterKey.

Nothing was committed.

Files: D:/iswp/574-canonical-annotation-paths/src/Infrastructure/Backlog.Infrastructure.Mcp/ChapterReading.cs, D:/iswp/574-canonical-annotation-paths/src/Infrastructure/Backlog.Infrastructure.Mcp/DevbookTools.cs, D:/iswp/574-canonical-annotation-paths/tests/Backlog.Infrastructure.Mcp.UnitTests/DevbookToolsTests.cs, D:/iswp/574-canonical-annotation-paths/tests/Backlog.Infrastructure.Mcp.UnitTests/FakeDevbook.cs, D:/iswp/574-canonical-annotation-paths/tests/Backlog.Desktop.UI.UnitTests/DevbookChapterKeyTests.cs, D:/iswp/574-canonical-annotation-paths/tests/Backlog.Infrastructure.FileSystem.UnitTests/DevbookAnnotationStoreTests.cs

Acceptance criteria

  • ⚠️ not proved by a test — 1. ChapterReading.Spellings no longer exists and no comment in Backlog.Infrastructure.Mcp says the panels file notes under different spellings
    • A property of the diff: the build compiles without Spellings, and a grep of src/Infrastructure/Backlog.Infrastructure.Mcp for 'disagree', 'Spellings', 'area-relative' and 'either spelling' found no remaining dual-spelling comment. No test asserts this.
  • ✅ 2. read_knowledge_chapter (review) and list_annotations each call List exactly once with the normalized repository-relative path; Listed shows one entry
    • DevbookToolsTests.Annotations_are_asked_for_once_with_the_chapter_path and DevbookToolsTests.A_review_read_asks_for_the_notes_once_with_the_chapter_path (both failed before the change with two Listed entries)
  • ✅ 3. Notes still drops drafts, returns tombstone-free results, orders by BlockIndex then CreatedAt; existing draft/tombstone/resolved tests stay green
    • DevbookToolsTests.Annotations_leave_out_drafts_and_tombstones, A_resolved_annotation_still_travels_and_says_so, A_review_read_interleaves_the_notes_at_their_blocks, A_note_anchored_out_of_range_still_travels (all green)
  • ✅ 4. A note filed under a relocated or .devbook/-prefixed spelling is returned by list_annotations when asked with any other spelling, through a canonicalizing store
    • DevbookToolsTests.A_note_filed_under_another_spelling_of_the_chapter_is_found (theory over .devbook/arc42, docs/arch and .arc42 spellings, with a fake store that calls DevbookChapterKey.Canonical)
  • ✅ 5. A test states what happens to a bare area-relative key such as adr/x.md: migrated on load, or shown unreachable
    • DevbookChapterKeyTests.A_path_with_no_folder_segment_is_claimed_by_no_area (not migrated), DevbookChapterKeyTests.Every_arc42_catalog_path_keys_under_the_conventional_folder (the arc42 catalog never emits a folderless path, so the panel cannot file one), DevbookAnnotationStoreTests.A_remark_keyed_without_its_folder_keeps_that_key_when_the_store_reads_it (kept, not lost, stamps unchanged). That no older build ever wrote a bare key rests on git history, not on a test.
  • ✅ 6. No change to persisted annotation stamps or ListChangedSince output
    • No store code was changed (a property of the diff). DevbookAnnotationStoreTests.A_remark_keyed_without_its_folder_keeps_that_key_when_the_store_reads_it and the existing Remarks_already_on_disk_take_the_canonical_name_when_the_store_reads_them and A_deleted_remark_stays_deleted_under_its_canonical_name assert that stamps are unchanged and ListChangedSince is empty.
  • ⚠️ not proved by a test — 7. Build warning-clean for touched projects; dotnet test passes for the three projects with counts checked
    • Verified by running, not by a test. A --no-incremental build of the three test projects gave 0 warnings and 0 errors. Mcp.UnitTests passed 160 of 160, FileSystem.UnitTests 519 of 519, Desktop.UI.UnitTests 3243 of 3243. The full Backlog.sln build, including the MAUI heads, was not run.

Verification

  • Command: cd "D:/iswp/574-canonical-annotation-paths"; dotnet build Backlog.sln; dotnet test Backlog.sln --no-build; then per project: dotnet build <proj> --no-incremental; dotnet test <proj> --no-build for tests/Backlog.Infrastructure.Mcp.UnitTests, tests/Backlog.Infrastructure.FileSystem.UnitTests, tests/Backlog.Desktop.UI.UnitTests
  • Result: 10295 passed, 0 failed. Build passed.
  • Repair attempts: 0
  • Notes: Worktree had uncommitted changes in ChapterReading.cs, DevbookTools.cs, DevbookChapterKeyTests.cs, DevbookAnnotationStoreTests.cs, DevbookToolsTests.cs, FakeDevbook.cs. The solution build (including the Backlog.Mobile android head and Backlog.Desktop) succeeded with 0 warnings and 0 errors. The full dotnet test Backlog.sln run reported: Passed!, failed 0, succeeded 10295, skipped 1. The skipped test was Backlog.EndToEndTests.ConferenceDayTests.A_conference_day_reaches_the_desktop_and_comes_back_as_my_day. Because incremental builds can hide warnings, I rebuilt the three named test projects with --no-incremental. Each had 0 warnings and 0 errors. Per-project counts: Backlog.Infrastructure.Mcp.UnitTests 160/160 passed, Backlog.Infrastructure.FileSystem.UnitTests 519/519 passed, Backlog.Desktop.UI.UnitTests 3243/3243 passed, none skipped. No project reported "Zero tests ran".

Review

  • minor, left open: src/Infrastructure/Backlog.Infrastructure.Mcp/DevbookTools.cs:186: The comment above list_annotations still says "the folder settings are read without resolving anything", but the change removed the only folder read on that path (the ChapterPaths.Split call). The new inline comment at line ~211 now says "no folder is resolved", so the two comments contradict each other.
    • Suggested fix: Change the attribute comment to say the notes come from Backlog's own store and the path reads no folder settings, so it matches the new inline comment.
  • minor, left open: src/Infrastructure/Backlog.Infrastructure.Mcp/DevbookTools.cs:188: The comment above ListAnnotationsOn still says the tool reads the folder settings ("the folder settings are read without resolving anything"). The change removed the only folder read in that method, the ChapterPaths.Split(folders.Folders(scope.Alias), ...) call, and its new inline comment says "no folder is resolved". The two comments now describe the method differently.
    • Suggested fix: Change the attribute-level comment to say the tool reads only Backlog's own annotation store, which canonicalizes the path itself, so nothing on this path can reach GitHub. Drop the phrase about reading folder settings.

What could not be proved

  • 2 acceptance criterion/criteria have no test proving them: "1. ChapterReading.Spellings no longer exists and no comment in Backlog.Infrastructure.Mcp says the panels file notes under different spellings"; "7. Build warning-clean for touched projects; dotnet test passes for the three projects with counts checked"
  • The run took 7 assumption(s) instead of asking: Can a bare area-relative key (e.g. adr/0012-....md, no folder segment) still exist in stored annotation JSON from builds before the arc42 reader spelled paths from the repository root? DevbookChapterKey.Canonical returns such a key unchanged, so the on-load migration would not fix it, and today only the MCP dual lookup finds it. Assumption: the implementer first checks git history of Arc42Devbook.cs / Arc42DevbookPanel.razor. If an area-relative spelling ever shipped, a mapping for it goes in DevbookChapterKey.Canonical, only when exactly one area plausibly claims it, keeping it idempotent and adding tests, before the MCP lookup is removed. If it never shipped, the removal goes ahead as-is.; Should the MCP test fake canonicalize like the real store, or should the tests use the real DevbookAnnotationStore? Assumption: the fake calls DevbookChapterKey.Canonical (the MCP test project already references Devbook.Abstractions), which keeps the tests free of file I/O.; The MCP tools echo the chapter path in their payloads as the caller spelled it (normalized), not in the canonical .arc42/... form. Assumption: keep echoing the caller's normalized path, so the tool contract for callers does not change.; I did not invoke delivery:flow-code myself. I took the orchestrating workflow (issue sweep / resolve-issue) to be the flow this stage runs inside, since the task told me to implement test first in the given worktree and to leave git and GitHub to the parent session.; I read the 'unreachable' option of criterion 5 as: no build of the product ever filed a bare area-relative key. Git history supports this. At b1c3eba, where IDevbookAnnotationStore was introduced, Arc42DevbookPanel already filed under document.Path, and Arc42Devbook.ResolveRoot already spelled paths relative to the repository root or the folder's parent. A key written by hand or by a non-product tool could still be bare, and the MCP tools would no longer find it.; Known edge case I left alone: if an area is pointed at a folder whose last segment is 'adr' (for example .tech at .arc42/adr), DevbookChapterKey claims bare 'adr/...' paths for that area. EveryConfiguration in DevbookChapterKeyTests already includes that layout, so the new test lists its own arc42-only layouts.; FakeDevbookAnnotationStore now canonicalizes at comparison time instead of on write. For List results this is equivalent to the real store, which canonicalizes on Put and on load.

Assumptions

  • Can a bare area-relative key (e.g. adr/0012-....md, no folder segment) still exist in stored annotation JSON from builds before the arc42 reader spelled paths from the repository root? DevbookChapterKey.Canonical returns such a key unchanged, so the on-load migration would not fix it, and today only the MCP dual lookup finds it. Assumption: the implementer first checks git history of Arc42Devbook.cs / Arc42DevbookPanel.razor. If an area-relative spelling ever shipped, a mapping for it goes in DevbookChapterKey.Canonical, only when exactly one area plausibly claims it, keeping it idempotent and adding tests, before the MCP lookup is removed. If it never shipped, the removal goes ahead as-is.
  • Should the MCP test fake canonicalize like the real store, or should the tests use the real DevbookAnnotationStore? Assumption: the fake calls DevbookChapterKey.Canonical (the MCP test project already references Devbook.Abstractions), which keeps the tests free of file I/O.
  • The MCP tools echo the chapter path in their payloads as the caller spelled it (normalized), not in the canonical .arc42/... form. Assumption: keep echoing the caller's normalized path, so the tool contract for callers does not change.
  • I did not invoke delivery:flow-code myself. I took the orchestrating workflow (issue sweep / resolve-issue) to be the flow this stage runs inside, since the task told me to implement test first in the given worktree and to leave git and GitHub to the parent session.
  • I read the 'unreachable' option of criterion 5 as: no build of the product ever filed a bare area-relative key. Git history supports this. At b1c3eba, where IDevbookAnnotationStore was introduced, Arc42DevbookPanel already filed under document.Path, and Arc42Devbook.ResolveRoot already spelled paths relative to the repository root or the folder's parent. A key written by hand or by a non-product tool could still be bare, and the MCP tools would no longer find it.
  • Known edge case I left alone: if an area is pointed at a folder whose last segment is 'adr' (for example .tech at .arc42/adr), DevbookChapterKey claims bare 'adr/...' paths for that area. EveryConfiguration in DevbookChapterKeyTests already includes that layout, so the new test lists its own arc42-only layouts.
  • FakeDevbookAnnotationStore now canonicalizes at comparison time instead of on write. For List results this is equivalent to the real store, which canonicalizes on Put and on load.

Opened as a draft by an unattended run of delivery-schedule:schedule-issue-sweep (delivery-schedule@1.13.0) on 2026-09-30. Validate what could not be proved, then mark it ready.

🤖 Generated with Claude Code

Checklist

  • Scope is focused and excludes unrelated changes
  • Documentation or instruction files were updated when governance or behavior changed (no chapter describes the MCP dual lookup; the store's canonical key is unchanged)
  • Running Aspire instances for this checkout were stopped before PR creation (none was started; the resolution ran unit tests only)
  • Follow-up work is captured when the implementation is intentionally partial (not partial; two minor comment findings are listed above)
  • Issues this PR closes are linked with a closing keyword

read_knowledge_chapter and list_annotations asked the annotation store
for two spellings of a chapter path and merged the results by id,
because the panels once filed notes under different spellings. Commit
120f2d5 made the store canonicalize every key on read, write and load,
so the MCP tools now make one List call with the normalized path and
ChapterReading.Spellings is gone. New tests pin that a note filed under
another spelling is still found, and what happens to a bare
area-relative key (kept as-is; no product build ever wrote one).

Refs #574

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@JSdotNet
JSdotNet marked this pull request as ready for review October 1, 2026 05:53
@JSdotNet
JSdotNet merged commit 4a5862f into main Oct 1, 2026
7 checks passed
@JSdotNet
JSdotNet deleted the schedule/issue-sweep/2026-09-30/574-canonical-annotation-paths branch October 1, 2026 05:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix inconsistent devbook annotation chapter paths

1 participant