From 38020f103db7012a749165295daed0b712d5cc17 Mon Sep 17 00:00:00 2001 From: TECK KEAT WILSON Date: Fri, 28 Aug 2026 15:15:41 +0800 Subject: [PATCH 1/5] feat(doc-index): infer real section granularity, hash sections for staleness Neither of these existed before: doc_index.py indexed every heading at every level, with no notion of "one retrievable section", and its staleness check was whole-file only -- any edit anywhere invalidated the entire cached index, making a real per-section partial rebuild impossible regardless of how small the actual edit was. infer_section_level() picks which heading level represents one real section, using the level's *frequency* as the signal: a document's real recurring structure (its chapters) shows up as the level used most often, while an occasional heading at an anomalous level -- exactly what PDF-to-Markdown conversion produces, since it assigns levels by font-size heuristics, not semantic depth -- is rare precisely because it's noise, not structure. Levels used only once are excluded as candidates outright. This is a direct, verified fix for a real failure found earlier building this feature: a real PDF-converted document put all 8 of its actual chapters on H5 and a single stray subsection on H3; treating H3 as "the" section level (or any fixed level) turned the entire back half of the document into one fake 6,601-line "section". Re-run against that same document with this change: 12 correctly-sized real sections, not one. build_doc_index() now also computes retrieval_sections -- headings grouped at exactly the inferred level (off-level stray headings stay inside whichever section they geographically fall under, rather than splitting one apart), each with a SHA-256 hash of its own text. diff_stale_sections() compares a file's current content against its last cached build at this granularity and reports which sections actually changed, matched by position (not heading text -- duplicate titles are real, see toc-heading-duplicate) -- the piece needed for a future caller to re-summarize only what changed instead of the whole document. Registered the three new instructions in traceability-validation.md; whitelisted diff_stale_sections in vulture_whitelist.py alongside annotate_section_summary (same "future caller, exercised by tests" situation). New code is 100% covered; existing sections/annotate/etag behavior is untouched and still passing. See constructorfabric/studio#104. Verified: pytest (test_doc_index.py + test_toc.py: 156 passed; full suite: 4815 passed, the same 12 pre-existing macOS-local/flaky failures as on main, none in the files touched here), pylint and vulture clean, cfs validate 0 errors, spec-coverage thresholds met, and infer_section_level re-run against the real PDF-converted document that originally exposed the bug. Signed-off-by: TECK KEAT WILSON --- .../features/traceability-validation.md | 5 +- .../studio/scripts/studio/utils/doc_index.py | 177 +++++++++++++++++- tests/test_doc_index.py | 113 +++++++++++ vulture_whitelist.py | 9 +- 4 files changed, 294 insertions(+), 10 deletions(-) diff --git a/architecture/features/traceability-validation.md b/architecture/features/traceability-validation.md index 93c04949..9c26eadf 100644 --- a/architecture/features/traceability-validation.md +++ b/architecture/features/traceability-validation.md @@ -447,7 +447,7 @@ Catches structural and traceability issues that AI agents miss or hallucinate **Input**: Markdown file path -**Output**: `cfs doc-index`'s JSON is `{file, cache_hit, total_lines, section_count, sections}`, each `sections[]` entry `{level, heading, line_start, line_end, summary}`. The underlying index dict additionally carries `schema_version`, `path`, and `etag`. +**Output**: `cfs doc-index`'s JSON is `{file, cache_hit, total_lines, section_count, sections, section_level, retrieval_section_count, retrieval_sections}` -- every heading's own line range in `sections[]` (`{level, heading, line_start, line_end, summary}`), plus a coarser "one chunk per real section" grouping in `retrieval_sections[]` at an inferred heading level, each with a content hash and a summary slot. The underlying cached index dict additionally carries `schema_version`, `path`, and `etag`. A cached, read-once-per-file structural index for Markdown JIT retrieval (see constructorfabric/studio#104): parsing a file's headings/section boundaries @@ -463,6 +463,9 @@ by requiring the read it's meant to save. 3. [x] - `p1` - Persist an index to its cache location atomically (temp file + `os.replace`, so a concurrent reader never observes a torn write); no-ops silently outside a Studio-adapted project - `inst-doc-index-save` 4. [x] - `p1` - Return the cached index or build-and-cache a fresh one; reports cache hit/miss for benchmarking - `inst-doc-index-get-or-build` 5. [x] - `p1` - Attach a one-line, LLM-authored summary to a cached section by its `line_start`, for a future per-section-summary caller - `inst-doc-index-annotate` +6. [x] - `p1` - Infer which heading level represents one retrievable section: the most-recurring level wins over a level that appears only once (however shallow), since PDF-conversion heading levels don't reliably encode true nesting depth — a fixed level assumption silently produces a degenerate mega-section on such documents - `inst-doc-index-infer-level` +7. [x] - `p1` - Group headings at exactly the inferred level into retrieval sections (off-level headings stay inside whichever section they fall under, never split one apart); hash each section's own text for section-granularity staleness detection - `inst-doc-index-retrieval-sections` +8. [x] - `p1` - Diff the current file against its last cached build at section granularity: which retrieval sections are unchanged vs. changed, or whether the section count itself changed (a structural change, matched by position not heading text, since duplicate titles are real) - `inst-doc-index-diff-stale` **Supporting**: - [x] - `p1` - Stat-based cache-validity fingerprint (`mtime_ns` + size); resolved from the file's own path, never a content hash - `inst-doc-index-etag` diff --git a/skills/studio/scripts/studio/utils/doc_index.py b/skills/studio/scripts/studio/utils/doc_index.py index 4552c533..84b97562 100644 --- a/skills/studio/scripts/studio/utils/doc_index.py +++ b/skills/studio/scripts/studio/utils/doc_index.py @@ -24,8 +24,9 @@ import logging import os import time +from collections import Counter from pathlib import Path -from typing import Any, Dict, List, Optional +from typing import Any, Dict, List, Optional, Tuple from .toc import parse_headings_with_lines @@ -95,6 +96,83 @@ def _index_cache_path(path: Path) -> Optional[Path]: # @cpt-end:cpt-studio-algo-traceability-validation-doc-index:p1:inst-doc-index-cache-path +# @cpt-begin:cpt-studio-algo-traceability-validation-doc-index:p1:inst-doc-index-infer-level +def infer_section_level(headings_with_lines: List[Tuple[int, str, int]]) -> Optional[int]: + """Infer which heading level represents one retrievable section. + + PDF-to-Markdown conversion assigns heading levels by font-size/style + heuristics, not semantic depth -- a document's real top-level chapters + can land on any level. A real document converted during this feature's + own development put all 8 of its actual chapters on H5, while a single + stray H3 subsection appeared once in the middle; a fixed-level + assumption (e.g. "H1-H3 is the chapter level") silently turned the back + half of that real document into one fake 6,601-line "section" bounded + by that one stray heading (see constructorfabric/studio#104). + + Heuristic: a document's real recurring structure shows up as the + heading level used *most often* -- real chapters repeat throughout a + document precisely because they're structure, not noise. A level used + only once is excluded as a candidate outright: a single occurrence + can't be "the" recurring section boundary by definition, and treating + it as one produces exactly the degenerate failure above. Ties (and the + all-singletons fallback) prefer the shallowest level, on the + conservative assumption that a coarser grouping beats fragmenting a + document into many tiny sections. + + Returns ``None`` for a headingless document. + """ + if not headings_with_lines: + return None + counts = Counter(level for level, _text, _line in headings_with_lines) + recurring = {level: count for level, count in counts.items() if count >= 2} + if not recurring: + return min(counts) + max_count = max(recurring.values()) + return min(level for level, count in recurring.items() if count == max_count) +# @cpt-end:cpt-studio-algo-traceability-validation-doc-index:p1:inst-doc-index-infer-level + + +# @cpt-begin:cpt-studio-algo-traceability-validation-doc-index:p1:inst-doc-index-retrieval-sections +def _build_retrieval_sections( + headings_with_lines: List[Tuple[int, str, int]], + lines: List[str], + section_level: Optional[int], +) -> List[Dict[str, Any]]: + """Group headings at exactly ``section_level`` into retrieval sections. + + Deliberately an *exact* level match, not "level <= section_level": the + same unreliable level-assignment this whole mechanism exists to work + around means a stray heading numerically shallower than the real + chapter level (like the H3 in the docstring above, sitting inside what + is structurally an H5 chapter) is not a trustworthy higher-level + boundary -- it's noise. Content under an off-level heading stays inside + whichever ``section_level`` section it falls under, rather than + splitting a real section apart. + + Each section's ``hash`` is a SHA-256 of its own text slice -- the + per-section granularity :func:`diff_stale_sections` needs to tell "this + one section changed" from "the whole file changed", which a whole-file + fingerprint structurally cannot do. + """ + if section_level is None: + return [] + line_count = len(lines) + marks = [(text, line_start) for level, text, line_start in headings_with_lines if level == section_level] + sections: List[Dict[str, Any]] = [] + for i, (text, line_start) in enumerate(marks): + line_end = marks[i + 1][1] - 1 if i + 1 < len(marks) else line_count + section_text = "\n".join(lines[line_start - 1:line_end]) + sections.append({ + "heading": text, + "line_start": line_start, + "line_end": line_end, + "hash": hashlib.sha256(section_text.encode("utf-8")).hexdigest(), + "summary": None, + }) + return sections +# @cpt-end:cpt-studio-algo-traceability-validation-doc-index:p1:inst-doc-index-retrieval-sections + + # @cpt-begin:cpt-studio-algo-traceability-validation-doc-index:p1:inst-doc-index-build def build_doc_index(path: Path) -> Dict[str, Any]: """Build a fresh structural index for a Markdown file. @@ -102,6 +180,13 @@ def build_doc_index(path: Path) -> Dict[str, Any]: Purely deterministic -- headings, section line ranges, and an etag. Contains no LLM-generated content; per-section ``summary`` fields start as ``None`` and are filled in later via :func:`annotate_section_summary`. + + ``sections`` lists *every* heading, any level (unchanged from before -- + still what :func:`annotate_section_summary` matches against by + ``line_start``). ``retrieval_sections`` is the coarser, inferred + "one chunk per real chapter" grouping a future TF-IDF/cascade/OKF + caller should read against instead -- see :func:`infer_section_level` + for why a fixed heading level can't be assumed. """ canonical_path = path.resolve() # Fingerprint before read: if a write lands between the two, the stored @@ -126,6 +211,8 @@ def build_doc_index(path: Path) -> Dict[str, Any]: "summary": None, }) + section_level = infer_section_level(headings) + return { "schema_version": _SCHEMA_VERSION, "path": str(canonical_path), @@ -133,10 +220,28 @@ def build_doc_index(path: Path) -> Dict[str, Any]: "built_at": time.strftime("%Y-%m-%dT%H:%M:%SZ", time.gmtime()), "total_lines": line_count, "sections": sections, + "section_level": section_level, + "retrieval_sections": _build_retrieval_sections(headings, lines, section_level), } # @cpt-end:cpt-studio-algo-traceability-validation-doc-index:p1:inst-doc-index-build +def _read_cache_file(cache_path: Path) -> Optional[Dict[str, Any]]: + """Read and parse a cache file, or ``None`` if missing/corrupt. + + No staleness check -- just "can this be read as JSON at all". Shared by + :func:`load_doc_index` (which layers the etag check on top) and + :func:`diff_stale_sections` (which deliberately reads a cache the + whole-file etag already considers stale, to compare it section by + section instead of discarding it outright). + """ + try: + return json.loads(cache_path.read_text(encoding="utf-8")) + except (json.JSONDecodeError, OSError) as exc: + logger.debug("doc-index cache unreadable at %s: %s", cache_path, exc) + return None + + # @cpt-begin:cpt-studio-algo-traceability-validation-doc-index:p1:inst-doc-index-load _REQUIRED_INDEX_FIELDS = ("total_lines", "sections") @@ -167,10 +272,8 @@ def load_doc_index(path: Path) -> Optional[Dict[str, Any]]: if cache_path is None or not cache_path.is_file(): return None - try: - cached = json.loads(cache_path.read_text(encoding="utf-8")) - except (json.JSONDecodeError, OSError) as exc: - logger.debug("doc-index cache unreadable for %s: %s", path, exc) + cached = _read_cache_file(cache_path) + if cached is None: return None canonical_path = path.resolve() @@ -230,6 +333,70 @@ def get_or_build_doc_index(path: Path, *, force_rebuild: bool = False) -> Dict[s # @cpt-end:cpt-studio-algo-traceability-validation-doc-index:p1:inst-doc-index-get-or-build +# @cpt-begin:cpt-studio-algo-traceability-validation-doc-index:p1:inst-doc-index-diff-stale +def diff_stale_sections(path: Path) -> Optional[Dict[str, Any]]: + """Compare the current file against its last cached build at *section* + granularity, not just "is the whole file's cache stale". + + This is what makes a real partial rebuild possible: :func:`load_doc_index` + answers "did anything change" (whole-file, via the etag); this answers + "which retrieval sections actually changed", so a caller doing expensive + per-section work (e.g. an LLM re-summarizing one section) can skip the + ones that didn't. + + Returns ``None`` when there's nothing to diff against -- never built, no + Studio directory, or the cached build predates ``retrieval_sections`` + (an older index format) -- callers should treat that as "everything is + new" and do a full build instead. + + Otherwise returns ``{"structural_change": bool, "unchanged": [...], + "changed": [...]}`` (heading-text lists, in document order). Sections + are matched by *position*, not heading text: duplicate heading titles + are real (see the ``toc-heading-duplicate`` check) and can't be told + apart by name, and a document that gained or lost a retrieval-level + heading shifts every position after it anyway. When the section + *count* itself differs, ``structural_change`` is ``True`` and + ``changed``/``unchanged`` aren't populated -- a position-based diff + across a changed count can't be safely narrowed to "which ones + changed" without guessing, so the caller should fall back to a full + rebuild rather than have this function guess for it. + """ + cache_path = _index_cache_path(path) + if cache_path is None or not cache_path.is_file(): + return None + + cached = _read_cache_file(cache_path) + if cached is None or "retrieval_sections" not in cached: + return None + + canonical_path = path.resolve() + try: + content = canonical_path.read_text(encoding="utf-8") + except OSError as exc: + logger.debug("doc-index section diff failed for %s: %s", path, exc) + return None + + lines = content.split("\n") + headings = parse_headings_with_lines(lines) + section_level = infer_section_level(headings) + fresh_sections = _build_retrieval_sections(headings, lines, section_level) + + old_sections = cached["retrieval_sections"] + if len(old_sections) != len(fresh_sections): + return { + "structural_change": True, + "unchanged": [], + "changed": [s["heading"] for s in fresh_sections], + } + + unchanged: List[str] = [] + changed: List[str] = [] + for old, new in zip(old_sections, fresh_sections): + (unchanged if old["hash"] == new["hash"] else changed).append(new["heading"]) + return {"structural_change": False, "unchanged": unchanged, "changed": changed} +# @cpt-end:cpt-studio-algo-traceability-validation-doc-index:p1:inst-doc-index-diff-stale + + # @cpt-begin:cpt-studio-algo-traceability-validation-doc-index:p1:inst-doc-index-annotate def annotate_section_summary(path: Path, line_start: int, summary: str) -> bool: """Attach a one-line summary to a cached section, keyed by its line_start. diff --git a/tests/test_doc_index.py b/tests/test_doc_index.py index 888b101d..fbdeb7bf 100644 --- a/tests/test_doc_index.py +++ b/tests/test_doc_index.py @@ -15,10 +15,13 @@ from studio.utils.doc_index import ( annotate_section_summary, build_doc_index, + diff_stale_sections, get_or_build_doc_index, + infer_section_level, load_doc_index, save_doc_index, ) +from studio.utils.toc import parse_headings_with_lines _SAMPLE = ( "# Title\n\n" @@ -103,6 +106,116 @@ def test_skips_headings_in_fenced_code(self, tmp_path: Path): index = build_doc_index(f) assert [s["heading"] for s in index["sections"]] == ["Title", "Real", "Also Real"] + def test_retrieval_sections_grouped_at_inferred_level(self, tmp_path: Path): + f = _write(tmp_path) + index = build_doc_index(f) + assert index["section_level"] == 2 + # "### A.1" (H3, off-level) stays inside "## Section A", not its own section. + assert [s["heading"] for s in index["retrieval_sections"]] == ["Section A", "Section B"] + + def test_headingless_document_has_no_retrieval_sections(self, tmp_path: Path): + f = _write(tmp_path, "Just a paragraph, no headings at all.\n") + index = build_doc_index(f) + assert index["section_level"] is None + assert index["retrieval_sections"] == [] + + def test_retrieval_section_hash_changes_only_for_the_edited_section(self, tmp_path: Path): + f = _write(tmp_path) + before = build_doc_index(f) + f.write_text(_SAMPLE.replace("Body of A.", "Body of A, edited."), encoding="utf-8") + after = build_doc_index(f) + by_heading_before = {s["heading"]: s["hash"] for s in before["retrieval_sections"]} + by_heading_after = {s["heading"]: s["hash"] for s in after["retrieval_sections"]} + assert by_heading_before["Section A"] != by_heading_after["Section A"] + assert by_heading_before["Section B"] == by_heading_after["Section B"] + + +class TestInferSectionLevel: + def test_uniform_level_is_chosen(self): + headings = [(2, "A", 1), (2, "B", 5), (2, "C", 9)] + assert infer_section_level(headings) == 2 + + def test_real_bug_regression_dominant_level_wins_over_a_stray_shallower_one(self): + """Reproduces the actual failure found developing this feature: a + PDF-converted document put its 8 real chapters on H5 and a single + subsection heading on H3. Picking the shallowest level present + (H3) -- or any fixed level -- turned the rest of the document into + one fake mega-section. The dominant (most-recurring) level must + win over a level that appears only once, however shallow.""" + headings = ( + [(5, f"Chapter {i}", i * 100) for i in range(1, 9)] + + [(3, "Stray Subsection", 250)] + ) + assert infer_section_level(headings) == 5 + + def test_no_headings_returns_none(self): + assert infer_section_level([]) is None + + def test_all_singleton_levels_falls_back_to_shallowest(self): + headings = [(4, "A", 1), (2, "B", 5), (6, "C", 9)] + assert infer_section_level(headings) == 2 + + def test_tie_between_recurring_levels_prefers_shallower(self): + headings = [(3, "A", 1), (3, "B", 5), (5, "C", 9), (5, "D", 13)] + assert infer_section_level(headings) == 3 + + def test_matches_real_parser_output(self, tmp_path: Path): + content = "##### Ch1\n\nbody\n\n##### Ch2\n\nbody\n\n### Odd\n\nbody\n\n##### Ch3\n\nbody\n" + f = _write(tmp_path, content) + lines = f.read_text(encoding="utf-8").split("\n") + headings = parse_headings_with_lines(lines) + assert infer_section_level(headings) == 5 + + +class TestDiffStaleSections: + def test_returns_none_when_no_cache_exists(self, tmp_path: Path, monkeypatch): + monkeypatch.setattr("studio.utils.files.find_studio_directory", lambda *_a, **_k: tmp_path) + f = _write(tmp_path) + assert diff_stale_sections(f) is None + + def test_returns_none_for_pre_retrieval_sections_cache_format(self, tmp_path: Path, monkeypatch): + monkeypatch.setattr("studio.utils.files.find_studio_directory", lambda *_a, **_k: tmp_path) + f = _write(tmp_path) + old_format = build_doc_index(f) + del old_format["retrieval_sections"] # simulate an index built before this field existed + save_doc_index(f, old_format) + assert diff_stale_sections(f) is None + + def test_no_edit_reports_everything_unchanged(self, tmp_path: Path, monkeypatch): + monkeypatch.setattr("studio.utils.files.find_studio_directory", lambda *_a, **_k: tmp_path) + f = _write(tmp_path) + save_doc_index(f, build_doc_index(f)) + diff = diff_stale_sections(f) + assert diff["structural_change"] is False + assert diff["changed"] == [] + assert set(diff["unchanged"]) == {"Section A", "Section B"} + + def test_editing_one_section_reports_only_that_one_changed(self, tmp_path: Path, monkeypatch): + monkeypatch.setattr("studio.utils.files.find_studio_directory", lambda *_a, **_k: tmp_path) + f = _write(tmp_path) + save_doc_index(f, build_doc_index(f)) + f.write_text(_SAMPLE.replace("Body of B.", "Body of B, edited."), encoding="utf-8") + diff = diff_stale_sections(f) + assert diff["structural_change"] is False + assert diff["changed"] == ["Section B"] + assert diff["unchanged"] == ["Section A"] + + def test_returns_none_when_file_deleted_after_caching(self, tmp_path: Path, monkeypatch): + monkeypatch.setattr("studio.utils.files.find_studio_directory", lambda *_a, **_k: tmp_path) + f = _write(tmp_path) + save_doc_index(f, build_doc_index(f)) + f.unlink() + assert diff_stale_sections(f) is None + + def test_adding_a_retrieval_level_heading_is_a_structural_change(self, tmp_path: Path, monkeypatch): + monkeypatch.setattr("studio.utils.files.find_studio_directory", lambda *_a, **_k: tmp_path) + f = _write(tmp_path) + save_doc_index(f, build_doc_index(f)) + f.write_text(_SAMPLE + "\n## Section C\n\nBody of C.\n", encoding="utf-8") + diff = diff_stale_sections(f) + assert diff["structural_change"] is True + assert diff["unchanged"] == [] + class TestCachePersistence: def test_load_returns_none_when_no_cache_exists(self, tmp_path: Path, monkeypatch): diff --git a/vulture_whitelist.py b/vulture_whitelist.py index a4dc6b7d..1c28afcb 100644 --- a/vulture_whitelist.py +++ b/vulture_whitelist.py @@ -12,7 +12,7 @@ from studio.commands.kit import _read_conf_version from studio.commands.resolve_vars import assemble_component from studio.utils.context import LoadedKit -from studio.utils.doc_index import annotate_section_summary +from studio.utils.doc_index import annotate_section_summary, diff_stale_sections from studio.utils.eval_harness import ReferencePresenceScorer, Scenario, ScorerKind, run_suite from studio.utils.eval_judge import Gold from studio.utils.manifest import ManifestLayerState @@ -40,11 +40,12 @@ _ = Gold.rules_assessed # part of the gold format; consumed by per-rule judge scoring (future) INCLUDE_ERROR = ManifestLayerState.INCLUDE_ERROR # valid enum value for future use -# doc-index summary annotation: written by an LLM caller during a one-time -# enrichment pass over a cached index's sections; not yet reached from -# production paths. Exercised by tests. See +# doc-index summary annotation and section-level staleness diff: called by +# a future partial-rebuild caller (an LLM re-summarizing only changed +# sections), not yet reached from production paths. Exercised by tests. See # skills/studio/scripts/studio/utils/doc_index.py. annotate_section_summary # noqa: B018 +diff_stale_sections # noqa: B018 # cfs map module — symbols retained for layout/configuration completeness. from studio.commands.map.layout import MAX_ROW_W # noqa: E402 From e62c30f3d46e57ac0bfefddb2812bacf277e2aa4 Mon Sep 17 00:00:00 2001 From: TECK KEAT WILSON Date: Mon, 31 Aug 2026 09:28:20 +0800 Subject: [PATCH 2/5] fix(doc-index): resolve CodeRabbit review findings on PR #109 - cmd_doc_index() built its output from the index but omitted retrieval_sections/section_level in both JSON and human output -- the new data #109 added was invisible through the CLI. Both are exposed now, and the human formatter lists retrieval sections the same way it already lists the finer-grained ones. - A write landing between read_text() and _compute_etag() in build_doc_index() could save headings parsed from the *old* content stamped with the *new* file's etag; load_doc_index() would then treat that stale index as valid until a later edit changed the etag again. _read_with_stable_etag() brackets the read with a stat snapshot on each side and retries on mismatch, so the saved etag is provably the one that matches what was actually parsed. - diff_stale_sections() reported changed/unchanged sections by heading text alone; two sections sharing a duplicate title (a real, already-flagged possibility -- see toc-heading-duplicate) couldn't be told apart. Each entry now carries line_start alongside the heading text, which is what a caller should actually use to address "this specific section" afterwards. - annotate_section_summary() updated only index["sections"], leaving the matching retrieval_sections entry at summary=None even on success -- a caller reading retrieval_sections (the more relevant list for a future per-section summarizer) couldn't see the annotation. Now updates both when both have an entry at line_start. - toc.py's _frontmatter_has_description() accepted `description: # TODO` and `description: ""` as satisfying the check, since `#` and `"` both match \S. Now parses the field's actual value and rejects comments and empty/whitespace-only quoted strings. Extracted _compute_fresh_retrieval_sections/_position_entry out of diff_stale_sections() to stay under pylint's local-variable limit after the line_start addition; registered the new instructions (stable-read, diff-stale-helpers) in traceability-validation.md. See constructorfabric/studio#104. Verified: pytest (test_doc_index.py + test_toc.py: 166 passed, 100% coverage on touched doc_index files); full suite: 4825 passed, the same 12 pre-existing macOS-local/flaky failures as on #108/#109, none in files touched here; pylint and vulture clean; cfs validate 0 errors; spec-coverage thresholds met; infer_section_level/retrieval_sections re-verified against the real PDF-converted document that originally exposed the granularity bug -- still 12 correct sections. Signed-off-by: TECK KEAT WILSON --- .../features/traceability-validation.md | 7 +- .../scripts/studio/commands/doc_index.py | 11 ++ .../studio/scripts/studio/utils/doc_index.py | 115 +++++++++---- skills/studio/scripts/studio/utils/toc.py | 7 + tests/test_doc_index.py | 152 ++++++++++++++---- tests/test_toc.py | 74 +++++++-- 6 files changed, 291 insertions(+), 75 deletions(-) diff --git a/architecture/features/traceability-validation.md b/architecture/features/traceability-validation.md index 9c26eadf..8046cace 100644 --- a/architecture/features/traceability-validation.md +++ b/architecture/features/traceability-validation.md @@ -456,7 +456,10 @@ size) changes. The cache-validity fingerprint is deliberately metadata-only (`mtime` + file size via `Path.stat()`), never a content hash — the point of the cache is to avoid reading the file at all on a hit, and a content hash would defeat that -by requiring the read it's meant to save. +by requiring the read it's meant to save. A build reads the content and +takes that fingerprint bracketed by a stat snapshot on each side, so the +fingerprint saved is provably the one that matches what was actually parsed +even if a write lands in the narrow window during the read. 1. [x] - `p1` - Build a fresh structural index: parse headings + line ranges from current content, compute the stat-based fingerprint, stamp the current schema version - `inst-doc-index-build` 2. [x] - `p1` - Load a cached index for a file, validated against current stat metadata (no content read on a hit) and against the required-field shape at the current schema version; returns `None` if missing, stale, corrupt, or an incomplete/outdated shape - `inst-doc-index-load` @@ -472,6 +475,8 @@ by requiring the read it's meant to save. - [x] - `p1` - Resolve the cache file location within the Studio directory owning the indexed file, resolved from the file's own path (not the process's working directory) - `inst-doc-index-cache-path` - [x] - `p1` - `cfs doc-index` CLI wrapper: parse arguments, build the JSON output payload, reporting a clean error for a missing or unreadable file - `inst-doc-index-cmd` - [x] - `p1` - Human-friendly formatter for `cfs doc-index` output - `inst-doc-index-cmd-format` +- [x] - `p1` - Read a file's content bracketed by an etag snapshot on each side, retrying on mismatch: closes the window where a write between the read and the fingerprint could save stale headings under a fresh-looking etag - `inst-doc-index-stable-read` +- [x] - `p1` - Re-parse a file's current content into retrieval sections for staleness comparison, and build the `(heading, line_start)` identity pair that disambiguates a duplicate heading title in a diff result - `inst-doc-index-diff-stale-helpers` ### Markdown Parsing Utilities diff --git a/skills/studio/scripts/studio/commands/doc_index.py b/skills/studio/scripts/studio/commands/doc_index.py index b71c493e..33275a60 100644 --- a/skills/studio/scripts/studio/commands/doc_index.py +++ b/skills/studio/scripts/studio/commands/doc_index.py @@ -58,6 +58,9 @@ def cmd_doc_index(argv: List[str]) -> int: "total_lines": index["total_lines"], "section_count": len(index["sections"]), "sections": index["sections"], + "section_level": index["section_level"], + "retrieval_section_count": len(index["retrieval_sections"]), + "retrieval_sections": index["retrieval_sections"], } ui.result(output, human_fn=_human_doc_index) return 0 @@ -75,4 +78,12 @@ def _human_doc_index(data: dict) -> None: summary = f" — {s['summary']}" if s.get("summary") else "" ui.substep(f" H{s['level']} [{s['line_start']}-{s['line_end']}] {s['heading']}{summary}") ui.blank() + + level = data["section_level"] + ui.step(f"Retrieval sections (level {level}, {data['retrieval_section_count']} section(s))" if level is not None + else "Retrieval sections (no headings — none inferred)") + for s in data["retrieval_sections"]: + summary = f" — {s['summary']}" if s.get("summary") else "" + ui.substep(f" [{s['line_start']}-{s['line_end']}] {s['heading']} ({s['hash'][:12]}){summary}") + ui.blank() # @cpt-end:cpt-studio-algo-traceability-validation-doc-index:p1:inst-doc-index-cmd-format diff --git a/skills/studio/scripts/studio/utils/doc_index.py b/skills/studio/scripts/studio/utils/doc_index.py index 84b97562..ac0d716b 100644 --- a/skills/studio/scripts/studio/utils/doc_index.py +++ b/skills/studio/scripts/studio/utils/doc_index.py @@ -173,6 +173,38 @@ def _build_retrieval_sections( # @cpt-end:cpt-studio-algo-traceability-validation-doc-index:p1:inst-doc-index-retrieval-sections +_MAX_READ_ATTEMPTS = 3 + + +# @cpt-begin:cpt-studio-algo-traceability-validation-doc-index:p1:inst-doc-index-stable-read +def _read_with_stable_etag(path: Path) -> Tuple[str, str]: + """Read a file's content together with an etag proven to match it. + + A write landing between reading the content and computing the etag + could otherwise save headings parsed from the *old* content stamped + with the *new* file's etag -- :func:`load_doc_index` would then treat + that stale index as valid until a later edit changes the etag again, + since nothing about the fingerprint itself would look wrong. + + Fixed by bracketing the read with a stat snapshot on each side: if they + match, the file didn't change during the read, so the etag genuinely + describes the content just read. If they don't, retry. After + ``_MAX_READ_ATTEMPTS`` under sustained contention, return the last read + anyway, stamped with its own trailing etag -- the safe direction to + fail in, since a file still being rewritten that fast will simply look + stale again on the very next check, never silently wrong. + """ + etag_after = _compute_etag(path) + for _ in range(_MAX_READ_ATTEMPTS): + etag_before = etag_after + content = path.read_text(encoding="utf-8") + etag_after = _compute_etag(path) + if etag_before == etag_after: + return content, etag_after + return content, etag_after +# @cpt-end:cpt-studio-algo-traceability-validation-doc-index:p1:inst-doc-index-stable-read + + # @cpt-begin:cpt-studio-algo-traceability-validation-doc-index:p1:inst-doc-index-build def build_doc_index(path: Path) -> Dict[str, Any]: """Build a fresh structural index for a Markdown file. @@ -189,13 +221,7 @@ def build_doc_index(path: Path) -> Dict[str, Any]: for why a fixed heading level can't be assumed. """ canonical_path = path.resolve() - # Fingerprint before read: if a write lands between the two, the stored - # etag describes content older than (never newer than) what got parsed, - # so a mismatch is always detected on the next load -- computing it - # after the read could instead capture a fingerprint newer than the - # content actually parsed, which a later stat comparison can't catch. - etag = _compute_etag(canonical_path) - content = canonical_path.read_text(encoding="utf-8") + content, etag = _read_with_stable_etag(canonical_path) lines = content.split("\n") line_count = len(lines) @@ -333,6 +359,32 @@ def get_or_build_doc_index(path: Path, *, force_rebuild: bool = False) -> Dict[s # @cpt-end:cpt-studio-algo-traceability-validation-doc-index:p1:inst-doc-index-get-or-build +# @cpt-begin:cpt-studio-algo-traceability-validation-doc-index:p1:inst-doc-index-diff-stale-helpers +def _compute_fresh_retrieval_sections(path: Path) -> Optional[List[Dict[str, Any]]]: + """Re-parse a file's current content into retrieval sections, for + comparison against a cached build. ``None`` on a read failure (e.g. the + file was deleted after it was cached).""" + canonical_path = path.resolve() + try: + content = canonical_path.read_text(encoding="utf-8") + except OSError as exc: + logger.debug("doc-index section diff failed for %s: %s", path, exc) + return None + + lines = content.split("\n") + headings = parse_headings_with_lines(lines) + section_level = infer_section_level(headings) + return _build_retrieval_sections(headings, lines, section_level) + + +def _position_entry(section: Dict[str, Any]) -> Dict[str, Any]: + """The (heading, line_start) pair identifying one retrieval section in + a :func:`diff_stale_sections` result -- ``line_start`` is what actually + disambiguates two sections sharing a duplicate heading title.""" + return {"heading": section["heading"], "line_start": section["line_start"]} +# @cpt-end:cpt-studio-algo-traceability-validation-doc-index:p1:inst-doc-index-diff-stale-helpers + + # @cpt-begin:cpt-studio-algo-traceability-validation-doc-index:p1:inst-doc-index-diff-stale def diff_stale_sections(path: Path) -> Optional[Dict[str, Any]]: """Compare the current file against its last cached build at *section* @@ -350,12 +402,15 @@ def diff_stale_sections(path: Path) -> Optional[Dict[str, Any]]: new" and do a full build instead. Otherwise returns ``{"structural_change": bool, "unchanged": [...], - "changed": [...]}`` (heading-text lists, in document order). Sections + "changed": [...]}``, where each entry is ``{"heading": str, "line_start": + int}`` -- the *current* (fresh) position, in document order. Sections are matched by *position*, not heading text: duplicate heading titles - are real (see the ``toc-heading-duplicate`` check) and can't be told - apart by name, and a document that gained or lost a retrieval-level - heading shifts every position after it anyway. When the section - *count* itself differs, ``structural_change`` is ``True`` and + are real (see the ``toc-heading-duplicate`` check), so heading text + alone can't tell two same-named sections apart -- ``line_start`` is + what a caller should actually use to address "this specific section" + afterwards (e.g. to call :func:`annotate_section_summary`), with the + heading text included only for human-readable logging. When the + section *count* itself differs, ``structural_change`` is ``True`` and ``changed``/``unchanged`` aren't populated -- a position-based diff across a changed count can't be safely narrowed to "which ones changed" without guessing, so the caller should fall back to a full @@ -369,30 +424,22 @@ def diff_stale_sections(path: Path) -> Optional[Dict[str, Any]]: if cached is None or "retrieval_sections" not in cached: return None - canonical_path = path.resolve() - try: - content = canonical_path.read_text(encoding="utf-8") - except OSError as exc: - logger.debug("doc-index section diff failed for %s: %s", path, exc) + fresh_sections = _compute_fresh_retrieval_sections(path) + if fresh_sections is None: return None - lines = content.split("\n") - headings = parse_headings_with_lines(lines) - section_level = infer_section_level(headings) - fresh_sections = _build_retrieval_sections(headings, lines, section_level) - old_sections = cached["retrieval_sections"] if len(old_sections) != len(fresh_sections): return { "structural_change": True, "unchanged": [], - "changed": [s["heading"] for s in fresh_sections], + "changed": [_position_entry(s) for s in fresh_sections], } - unchanged: List[str] = [] - changed: List[str] = [] - for old, new in zip(old_sections, fresh_sections): - (unchanged if old["hash"] == new["hash"] else changed).append(new["heading"]) + unchanged: List[Dict[str, Any]] = [] + changed: List[Dict[str, Any]] = [] + for old, new in zip(old_sections, fresh_sections, strict=True): + (unchanged if old["hash"] == new["hash"] else changed).append(_position_entry(new)) return {"structural_change": False, "unchanged": unchanged, "changed": changed} # @cpt-end:cpt-studio-algo-traceability-validation-doc-index:p1:inst-doc-index-diff-stale @@ -405,6 +452,15 @@ def annotate_section_summary(path: Path, line_start: int, summary: str) -> bool: pass, never generated inside this module. Returns ``False`` when no valid (non-stale) cached index exists or no section matches ``line_start`` -- callers should build the index first. + + Updates the matching entry in both ``sections`` (any heading level) and + ``retrieval_sections`` (the coarser grouping) when both have a section + starting at ``line_start`` -- a retriever reading ``retrieval_sections`` + needs the summary to show up there too, not just in the finer-grained + list. A ``line_start`` that only matches ``sections`` (an off-level + heading that isn't itself a retrieval section's start) updates only + that list, which is correct: there is no corresponding retrieval + section to update. """ index = load_doc_index(path) if index is None: @@ -419,6 +475,11 @@ def annotate_section_summary(path: Path, line_start: int, summary: str) -> bool: if not matched: return False + for retrieval_section in index.get("retrieval_sections", []): + if retrieval_section["line_start"] == line_start: + retrieval_section["summary"] = summary + break + save_doc_index(path, index) return True # @cpt-end:cpt-studio-algo-traceability-validation-doc-index:p1:inst-doc-index-annotate diff --git a/skills/studio/scripts/studio/utils/toc.py b/skills/studio/scripts/studio/utils/toc.py index daf6a711..753f0be9 100644 --- a/skills/studio/scripts/studio/utils/toc.py +++ b/skills/studio/scripts/studio/utils/toc.py @@ -892,6 +892,13 @@ def _frontmatter_has_description(lines: List[str], frontmatter_end: int) -> bool ``frontmatter_end`` is the index returned by :func:`_find_frontmatter_end` (one past the closing ``---``); the body being scanned is ``lines[1:frontmatter_end - 1]``, excluding both delimiter lines. + + A field that's present but carries no real value doesn't satisfy this: + a YAML comment (``description: # TODO``) or an empty quoted string + (``description: ""``) both parse as "no description" just as much as + the field being absent entirely would -- the point of this check is to + guarantee a caller gets something to actually read, not just a + matching key. """ for line in lines[1:frontmatter_end - 1]: match = _DESCRIPTION_FIELD_RE.match(line.strip()) diff --git a/tests/test_doc_index.py b/tests/test_doc_index.py index fbdeb7bf..61450f75 100644 --- a/tests/test_doc_index.py +++ b/tests/test_doc_index.py @@ -70,36 +70,6 @@ def test_etag_changes_when_content_changes(self, tmp_path: Path): idx2 = build_doc_index(f) assert idx1["etag"] != idx2["etag"] - def test_etag_is_computed_before_reading_content(self, tmp_path: Path, monkeypatch): - """CodeRabbit PR #108 (round 2): the etag must be captured before the - content read, not after -- a write landing between the two calls - would otherwise let the stored etag describe content newer than - what got parsed, and no later stat comparison could ever detect - that mismatch. Computing the etag first means a race can only make - it look older than the parsed content, which a later check always - catches.""" - from studio.utils import doc_index as doc_index_module - - f = _write(tmp_path) - call_order = [] - - real_compute_etag = doc_index_module._compute_etag - real_read_text = Path.read_text - - def _tracked_compute_etag(path): - call_order.append("etag") - return real_compute_etag(path) - - def _tracked_read_text(self, *args, **kwargs): - call_order.append("read") - return real_read_text(self, *args, **kwargs) - - monkeypatch.setattr(doc_index_module, "_compute_etag", _tracked_compute_etag) - monkeypatch.setattr(Path, "read_text", _tracked_read_text) - - doc_index_module.build_doc_index(f) - assert call_order == ["etag", "read"] - def test_skips_headings_in_fenced_code(self, tmp_path: Path): content = "# Title\n\n## Real\n\n```bash\n# not a heading\n```\n\n## Also Real\n" f = _write(tmp_path, content) @@ -188,7 +158,10 @@ def test_no_edit_reports_everything_unchanged(self, tmp_path: Path, monkeypatch) diff = diff_stale_sections(f) assert diff["structural_change"] is False assert diff["changed"] == [] - assert set(diff["unchanged"]) == {"Section A", "Section B"} + assert {(e["heading"], e["line_start"]) for e in diff["unchanged"]} == { + ("Section A", 3), + ("Section B", 11), + } def test_editing_one_section_reports_only_that_one_changed(self, tmp_path: Path, monkeypatch): monkeypatch.setattr("studio.utils.files.find_studio_directory", lambda *_a, **_k: tmp_path) @@ -197,8 +170,22 @@ def test_editing_one_section_reports_only_that_one_changed(self, tmp_path: Path, f.write_text(_SAMPLE.replace("Body of B.", "Body of B, edited."), encoding="utf-8") diff = diff_stale_sections(f) assert diff["structural_change"] is False - assert diff["changed"] == ["Section B"] - assert diff["unchanged"] == ["Section A"] + assert diff["changed"] == [{"heading": "Section B", "line_start": 11}] + assert diff["unchanged"] == [{"heading": "Section A", "line_start": 3}] + + def test_duplicate_headings_are_disambiguated_by_line_start(self, tmp_path: Path, monkeypatch): + """CodeRabbit PR #109: heading text alone can't tell two identically + named sections apart -- line_start must be returned so a caller + knows exactly which one changed.""" + monkeypatch.setattr("studio.utils.files.find_studio_directory", lambda *_a, **_k: tmp_path) + content = "## Details\n\nFirst.\n\n## Details\n\nSecond.\n" + f = _write(tmp_path, content) + save_doc_index(f, build_doc_index(f)) + f.write_text(content.replace("Second.", "Second, edited."), encoding="utf-8") + diff = diff_stale_sections(f) + assert diff["structural_change"] is False + assert diff["unchanged"] == [{"heading": "Details", "line_start": 1}] + assert diff["changed"] == [{"heading": "Details", "line_start": 5}] def test_returns_none_when_file_deleted_after_caching(self, tmp_path: Path, monkeypatch): monkeypatch.setattr("studio.utils.files.find_studio_directory", lambda *_a, **_k: tmp_path) @@ -215,6 +202,7 @@ def test_adding_a_retrieval_level_heading_is_a_structural_change(self, tmp_path: diff = diff_stale_sections(f) assert diff["structural_change"] is True assert diff["unchanged"] == [] + assert {e["heading"] for e in diff["changed"]} == {"Section A", "Section B", "Section C"} class TestCachePersistence: @@ -440,6 +428,73 @@ def test_returns_true_and_persists_on_match(self, tmp_path: Path, monkeypatch): cached = load_doc_index(f) assert cached["sections"][0]["summary"] == "The title." + def test_updates_matching_retrieval_section_too(self, tmp_path: Path, monkeypatch): + """CodeRabbit PR #109: annotate_section_summary() updated only + `sections`, leaving the matching `retrieval_sections` entry at + summary=None -- a caller reading retrieval_sections (the more + relevant list for a future OKF-style summarizer) couldn't see it.""" + monkeypatch.setattr("studio.utils.files.find_studio_directory", lambda *_a, **_k: tmp_path) + f = _write(tmp_path) + get_or_build_doc_index(f) + assert annotate_section_summary(f, line_start=3, summary="Covers A.") is True + index = load_doc_index(f) + retrieval_a = next(s for s in index["retrieval_sections"] if s["heading"] == "Section A") + assert retrieval_a["summary"] == "Covers A." + + def test_off_level_heading_leaves_retrieval_sections_untouched(self, tmp_path: Path, monkeypatch): + """line_start=7 is "### A.1" -- present in `sections` but not itself + a retrieval section's start (retrieval sections are at H2 here). + Only `sections` should be updated; there's no corresponding + retrieval section to touch.""" + monkeypatch.setattr("studio.utils.files.find_studio_directory", lambda *_a, **_k: tmp_path) + f = _write(tmp_path) + get_or_build_doc_index(f) + assert annotate_section_summary(f, line_start=7, summary="About A.1.") is True + index = load_doc_index(f) + a1 = next(s for s in index["sections"] if s["heading"] == "A.1") + assert a1["summary"] == "About A.1." + assert all(s["summary"] is None for s in index["retrieval_sections"]) + + +class TestReadWithStableEtag: + def test_retries_when_the_file_changes_mid_read(self, tmp_path: Path, monkeypatch): + """CodeRabbit PR #109: a write landing between reading content and + computing the etag could save headings from the *old* content + stamped with the *new* etag. Snapshotting before and after the + read, and retrying on mismatch, closes that window.""" + import studio.utils.doc_index as di + + f = _write(tmp_path) + etag_sequence = ["a", "b", "b"] # initial snapshot, then a mismatch, then a stable match + calls = {"n": 0} + + def fake_compute_etag(_path): + value = etag_sequence[calls["n"]] + calls["n"] += 1 + return value + + monkeypatch.setattr(di, "_compute_etag", fake_compute_etag) + content, etag = di._read_with_stable_etag(f) + assert content == _SAMPLE + assert etag == "b" + assert calls["n"] == 3 # one retry: initial snapshot + two read-and-check cycles + + def test_gives_up_after_max_attempts_under_sustained_contention(self, tmp_path: Path, monkeypatch): + import studio.utils.doc_index as di + + f = _write(tmp_path) + calls = {"n": 0} + + def always_different(_path): + calls["n"] += 1 + return f"etag-{calls['n']}" + + monkeypatch.setattr(di, "_compute_etag", always_different) + content, etag = di._read_with_stable_etag(f) + assert content == _SAMPLE # still returns a real read, not an error + assert calls["n"] == di._MAX_READ_ATTEMPTS + 1 + assert etag == f"etag-{calls['n']}" + class TestCmdDocIndex: def test_missing_file(self, tmp_path: Path, capsys): @@ -491,6 +546,37 @@ def test_basic(self, tmp_path: Path, capsys, monkeypatch): assert out["cache_hit"] is False assert out["section_count"] == 4 + def test_json_output_exposes_retrieval_sections(self, tmp_path: Path, capsys, monkeypatch): + """CodeRabbit PR #109: cmd_doc_index() built its output from `index` + but omitted retrieval_sections/section_level -- the new data this + PR adds was invisible through the CLI.""" + monkeypatch.setattr("studio.utils.files.find_studio_directory", lambda *_a, **_k: tmp_path) + f = _write(tmp_path) + rc = cmd_doc_index([str(f)]) + assert rc == 0 + out = json.loads(capsys.readouterr().out) + assert out["section_level"] == 2 + assert out["retrieval_section_count"] == 2 + assert [s["heading"] for s in out["retrieval_sections"]] == ["Section A", "Section B"] + assert "hash" in out["retrieval_sections"][0] + + def test_human_output_lists_retrieval_sections(self, tmp_path: Path, capsys, monkeypatch): + from studio.utils.ui import is_json_mode, set_json_mode + + monkeypatch.setattr("studio.utils.files.find_studio_directory", lambda *_a, **_k: tmp_path) + f = _write(tmp_path) + orig = is_json_mode() + set_json_mode(False) + try: + rc = cmd_doc_index([str(f)]) + finally: + set_json_mode(orig) + assert rc == 0 + out = capsys.readouterr().out + assert "Retrieval sections (level 2, 2 section(s))" in out + assert "Section A" in out + assert "Section B" in out + def test_second_invocation_is_cache_hit(self, tmp_path: Path, capsys, monkeypatch): monkeypatch.setattr("studio.utils.files.find_studio_directory", lambda *_a, **_k: tmp_path) f = _write(tmp_path) diff --git a/tests/test_toc.py b/tests/test_toc.py index 6d6e8865..f300f664 100644 --- a/tests/test_toc.py +++ b/tests/test_toc.py @@ -985,23 +985,14 @@ def test_frontmatter_without_description_field_still_warns(self): codes = [w["code"] for w in result["warnings"]] assert "toc-missing-description" in codes - @pytest.mark.parametrize( - "description_line", - [ - 'description: ""', - "description: ''", - "description: # TODO", - ], - ids=["double-quoted-empty", "single-quoted-empty", "comment-only"], - ) - def test_empty_or_comment_only_description_still_warns(self, description_line): - """CodeRabbit PR #108 (round 2): an empty quoted scalar or a bare - comment satisfies a naive "any character after the colon" check but - is not a real description -- each must still trigger the warning.""" + def test_comment_only_description_value_still_warns(self): + """CodeRabbit PR #109: `description: # TODO` matched the old regex + (`#` is non-whitespace) but is a YAML comment, not a value -- the + field is exactly as absent as if it weren't there at all.""" filler = "\n\n".join(f"Paragraph {i} of filler text." for i in range(60)) content = ( "---\n" - f"{description_line}\n" + "description: # TODO write this\n" "---\n\n" "# Title\n\n" "## Table of Contents\n\n" @@ -1013,6 +1004,61 @@ def test_empty_or_comment_only_description_still_warns(self, description_line): codes = [w["code"] for w in result["warnings"]] assert "toc-missing-description" in codes + def test_empty_quoted_description_value_still_warns(self): + """CodeRabbit PR #109: `description: ""` matched the old regex (the + opening quote is non-whitespace) but carries no actual text.""" + filler = "\n\n".join(f"Paragraph {i} of filler text." for i in range(60)) + content = ( + "---\n" + 'description: ""\n' + "---\n\n" + "# Title\n\n" + "## Table of Contents\n\n" + "1. [A](#a)\n\n" + "---\n\n" + f"## A\n\n{filler}\n" + ) + result = validate_toc(content, max_heading_level=2) + codes = [w["code"] for w in result["warnings"]] + assert "toc-missing-description" in codes + + def test_empty_single_quoted_description_value_still_warns(self): + """CodeRabbit PR #108 (round 2): single-quoted `description: ''` is + the same empty-scalar case as the double-quoted form and must warn + too -- not just the double-quoted variant.""" + filler = "\n\n".join(f"Paragraph {i} of filler text." for i in range(60)) + content = ( + "---\n" + "description: ''\n" + "---\n\n" + "# Title\n\n" + "## Table of Contents\n\n" + "1. [A](#a)\n\n" + "---\n\n" + f"## A\n\n{filler}\n" + ) + result = validate_toc(content, max_heading_level=2) + codes = [w["code"] for w in result["warnings"]] + assert "toc-missing-description" in codes + + def test_real_description_after_regex_tightening_still_suppresses_warning(self): + """Confirms the stricter check didn't overcorrect into rejecting a + genuinely populated, quoted description.""" + filler = "\n\n".join(f"Paragraph {i} of filler text." for i in range(60)) + content = ( + "---\n" + 'description: "A real, non-empty description."\n' + "---\n\n" + "# Title\n\n" + "## Table of Contents\n\n" + "1. [A](#a)\n\n" + "---\n\n" + f"## A\n\n{filler}\n" + ) + result = validate_toc(content, max_heading_level=2) + codes = [w["code"] for w in result["warnings"]] + assert "toc-missing-description" not in codes + def test_jit_readiness_warnings_are_never_errors(self): # All four signals are additive warnings; they must never appear # in `errors`, regardless of how badly a document scores. (This From e3daf450d4ecc2ba6856547cf0aae1145d35c979 Mon Sep 17 00:00:00 2001 From: TECK KEAT WILSON Date: Mon, 31 Aug 2026 09:42:08 +0800 Subject: [PATCH 3/5] fix(doc-index): resolve second CodeRabbit review round on PR #109 - load_doc_index() returned a cached index whenever its etag matched, with no check that the cached shape matched what this version of the code expects. A cache written before section_level/retrieval_sections existed can still have a matching etag if the file hasn't changed since -- cmd_doc_index() would then hit a KeyError reading those fields on a legacy cache instead of a clean rebuild. Now treated the same as a stale cache: rebuilt, not returned as-is. - _frontmatter_has_description() treated a YAML block-scalar marker (`description: |`, `description: >-`, ...) as a usable value, when the real content -- if any -- belongs on indented lines below it, not on the marker's own line. Now checks the first non-blank following line for real indentation before counting it as a description. See constructorfabric/studio#104. Verified: pytest (test_doc_index.py + test_toc.py: 172 passed, 100% coverage on touched doc_index files); full suite: 4831 passed, the same 12 pre-existing macOS-local/flaky failures as before, none in files touched here; pylint and vulture clean; cfs validate 0 errors; spec-coverage thresholds met; infer_section_level/retrieval_sections re-verified against the real PDF-converted document -- still 12 correct sections. Signed-off-by: TECK KEAT WILSON --- .../studio/scripts/studio/utils/doc_index.py | 9 ++- skills/studio/scripts/studio/utils/toc.py | 59 +++++++++----- tests/test_doc_index.py | 37 +++++++++ tests/test_toc.py | 80 +++++++++++++++++++ 4 files changed, 162 insertions(+), 23 deletions(-) diff --git a/skills/studio/scripts/studio/utils/doc_index.py b/skills/studio/scripts/studio/utils/doc_index.py index ac0d716b..f6960012 100644 --- a/skills/studio/scripts/studio/utils/doc_index.py +++ b/skills/studio/scripts/studio/utils/doc_index.py @@ -269,7 +269,7 @@ def _read_cache_file(cache_path: Path) -> Optional[Dict[str, Any]]: # @cpt-begin:cpt-studio-algo-traceability-validation-doc-index:p1:inst-doc-index-load -_REQUIRED_INDEX_FIELDS = ("total_lines", "sections") +_REQUIRED_INDEX_FIELDS = ("total_lines", "sections", "section_level", "retrieval_sections") def _has_schema_current_index(cached: Dict[str, Any]) -> bool: @@ -293,6 +293,13 @@ def load_doc_index(path: Path) -> Optional[Dict[str, Any]]: read (the property the whole cache exists to provide). Only a stale or absent cache falls through to :func:`build_doc_index`, which does the one real read. + + A matching etag alone isn't enough: a cache written by an older version + of this module (before ``section_level``/``retrieval_sections`` + existed) can have a matching etag if the file hasn't changed since, but + a caller reading those fields on it would hit a ``KeyError`` rather + than a clean rebuild. Treated the same as a stale cache -- rebuilt, + not crashed on. """ cache_path = _index_cache_path(path) if cache_path is None or not cache_path.is_file(): diff --git a/skills/studio/scripts/studio/utils/toc.py b/skills/studio/scripts/studio/utils/toc.py index 753f0be9..03ecfdb9 100644 --- a/skills/studio/scripts/studio/utils/toc.py +++ b/skills/studio/scripts/studio/utils/toc.py @@ -867,22 +867,26 @@ def _check_section_lengths( _DESCRIPTION_FIELD_RE = re.compile(r"^description\s*:\s*(.*)$") +_BLOCK_SCALAR_RE = re.compile(r"^[|>][+\-]?\d*$") -def _is_real_description_value(raw_value: str) -> bool: - """``True`` only if *raw_value* (the text after ``description:``) is an - actual description, not an empty quoted scalar (``""``/``''``) or a - comment-only value (``# TODO``) -- both look non-blank to a naive - "any character after the colon" check but carry no real content. - """ - value = raw_value.strip() - if not value or value.startswith("#"): - return False - if value[0] in "\"'": - quote = value[0] - end = value.find(quote, 1) - quoted = value[1:end] if end != -1 else value[1:] - return bool(quoted.strip()) +def _quoted_value_is_empty(value: str) -> bool: + """``value`` starts with a quote char -- True if the quoted text is empty.""" + quote = value[0] + closing = value.find(quote, 1) + inner = value[1:closing] if closing != -1 else value[1:] + return not inner.strip() + + +def _block_scalar_is_empty(body: List[str], start_index: int) -> bool: + """``value`` was a YAML block scalar marker (``|``, ``>``, ``|-``, ...) -- + its real content, if any, is on indented lines below it, not on the + marker's own line. True if the first non-blank following line isn't + indented under it (i.e. the block scalar has no content at all).""" + for line in body[start_index:]: + if not line.strip(): + continue + return not line[0].isspace() return True @@ -894,16 +898,27 @@ def _frontmatter_has_description(lines: List[str], frontmatter_end: int) -> bool ``lines[1:frontmatter_end - 1]``, excluding both delimiter lines. A field that's present but carries no real value doesn't satisfy this: - a YAML comment (``description: # TODO``) or an empty quoted string - (``description: ""``) both parse as "no description" just as much as - the field being absent entirely would -- the point of this check is to - guarantee a caller gets something to actually read, not just a - matching key. + a YAML comment (``description: # TODO``), an empty quoted string + (``description: ""``), or a block-scalar marker + (``description: |``) with nothing indented beneath it all parse as "no + description" just as much as the field being absent entirely would -- + the point of this check is to guarantee a caller gets something to + actually read, not just a matching key. """ - for line in lines[1:frontmatter_end - 1]: + body = lines[1:frontmatter_end - 1] + for i, line in enumerate(body): match = _DESCRIPTION_FIELD_RE.match(line.strip()) - if match and _is_real_description_value(match.group(1)): - return True + if not match: + continue + value = match.group(1).strip() + if not value or value.startswith("#"): + continue + if _BLOCK_SCALAR_RE.match(value): + if _block_scalar_is_empty(body, i + 1): + continue + elif value[0] in "\"'" and _quoted_value_is_empty(value): + continue + return True return False diff --git a/tests/test_doc_index.py b/tests/test_doc_index.py index 61450f75..f295a5c8 100644 --- a/tests/test_doc_index.py +++ b/tests/test_doc_index.py @@ -211,6 +211,43 @@ def test_load_returns_none_when_no_cache_exists(self, tmp_path: Path, monkeypatc f = _write(tmp_path) assert load_doc_index(f) is None + def test_legacy_cache_with_matching_etag_but_old_schema_is_rebuilt_not_returned( + self, tmp_path: Path, monkeypatch + ): + """CodeRabbit PR #109 (second round): a cache written before + section_level/retrieval_sections existed can have a matching etag + if the file hasn't changed since -- load_doc_index() must not + return it as-is, or a caller reading those fields hits a + KeyError instead of a clean rebuild.""" + monkeypatch.setattr("studio.utils.files.find_studio_directory", lambda *_a, **_k: tmp_path) + f = _write(tmp_path) + legacy = build_doc_index(f) + del legacy["section_level"] + del legacy["retrieval_sections"] + save_doc_index(f, legacy) + + assert load_doc_index(f) is None # not the legacy dict, and not a crash + + # The real caller path rebuilds cleanly rather than raising. + index = get_or_build_doc_index(f) + assert index["cache_hit"] is False + assert "section_level" in index + assert "retrieval_sections" in index + + def test_cmd_doc_index_does_not_crash_on_a_legacy_cache(self, tmp_path: Path, capsys, monkeypatch): + monkeypatch.setattr("studio.utils.files.find_studio_directory", lambda *_a, **_k: tmp_path) + f = _write(tmp_path) + legacy = build_doc_index(f) + del legacy["section_level"] + del legacy["retrieval_sections"] + save_doc_index(f, legacy) + + rc = cmd_doc_index([str(f)]) + assert rc == 0 + out = json.loads(capsys.readouterr().out) + assert out["cache_hit"] is False + assert "retrieval_sections" in out + def test_save_then_load_round_trips(self, tmp_path: Path, monkeypatch): monkeypatch.setattr("studio.utils.files.find_studio_directory", lambda *_a, **_k: tmp_path) f = _write(tmp_path) diff --git a/tests/test_toc.py b/tests/test_toc.py index f300f664..52ad526e 100644 --- a/tests/test_toc.py +++ b/tests/test_toc.py @@ -1059,6 +1059,86 @@ def test_real_description_after_regex_tightening_still_suppresses_warning(self): codes = [w["code"] for w in result["warnings"]] assert "toc-missing-description" not in codes + def test_empty_block_scalar_description_still_warns(self): + """CodeRabbit PR #109 (second round): `description: |` is a YAML + block-scalar marker -- the real content (if any) belongs on + indented lines below it, not on the marker line itself. With + nothing indented beneath it, this frontmatter has no real + description, immediately followed by the closing `---`.""" + filler = "\n\n".join(f"Paragraph {i} of filler text." for i in range(60)) + content = ( + "---\n" + "description: |\n" + "---\n\n" + "# Title\n\n" + "## Table of Contents\n\n" + "1. [A](#a)\n\n" + "---\n\n" + f"## A\n\n{filler}\n" + ) + result = validate_toc(content, max_heading_level=2) + codes = [w["code"] for w in result["warnings"]] + assert "toc-missing-description" in codes + + def test_populated_block_scalar_description_suppresses_warning(self): + """The other side of the block-scalar fix: real indented content + under `description: |` must still count as a real description.""" + filler = "\n\n".join(f"Paragraph {i} of filler text." for i in range(60)) + content = ( + "---\n" + "description: |\n" + " A real, multi-line\n" + " block-scalar description.\n" + "---\n\n" + "# Title\n\n" + "## Table of Contents\n\n" + "1. [A](#a)\n\n" + "---\n\n" + f"## A\n\n{filler}\n" + ) + result = validate_toc(content, max_heading_level=2) + codes = [w["code"] for w in result["warnings"]] + assert "toc-missing-description" not in codes + + def test_block_scalar_with_leading_blank_line_before_content_still_counts(self): + """A blank line immediately under the block-scalar marker (before + the real indented content) must be skipped, not mistaken for "no + content".""" + filler = "\n\n".join(f"Paragraph {i} of filler text." for i in range(60)) + content = ( + "---\n" + "description: |\n" + "\n" + " Real content after a leading blank line.\n" + "---\n\n" + "# Title\n\n" + "## Table of Contents\n\n" + "1. [A](#a)\n\n" + "---\n\n" + f"## A\n\n{filler}\n" + ) + result = validate_toc(content, max_heading_level=2) + codes = [w["code"] for w in result["warnings"]] + assert "toc-missing-description" not in codes + + def test_folded_block_scalar_marker_variant_is_recognized(self): + """`>` (folded) and modifiers like `|-`/`>+` are all valid YAML + block-scalar indicators, not just the bare `|`.""" + filler = "\n\n".join(f"Paragraph {i} of filler text." for i in range(60)) + content = ( + "---\n" + "description: >-\n" + "---\n\n" + "# Title\n\n" + "## Table of Contents\n\n" + "1. [A](#a)\n\n" + "---\n\n" + f"## A\n\n{filler}\n" + ) + result = validate_toc(content, max_heading_level=2) + codes = [w["code"] for w in result["warnings"]] + assert "toc-missing-description" in codes + def test_jit_readiness_warnings_are_never_errors(self): # All four signals are additive warnings; they must never appear # in `errors`, regardless of how badly a document scores. (This From 5e46011f6bc82460b55d1cf1bb880fc3ebafb5ea Mon Sep 17 00:00:00 2001 From: TECK KEAT WILSON Date: Tue, 1 Sep 2026 10:44:51 +0800 Subject: [PATCH 4/5] fix(doc-index,validate-toc): resolve CodeRabbit deep-review findings on PR #109 - doc_index.py: documented the additive-only schema contract (new fields need no version bump; only an existing field's meaning/shape changing does); non-UTF-8/binary input read failures are now caught and either degrade gracefully (diff paths) or propagate as a typed, catchable UnicodeDecodeError (build paths); per-section hashing now strips trailing whitespace per line, so an editor's "trim trailing whitespace on save" no longer looks like a real content edit; three genuinely abnormal cache fallback paths (directory lookup failure, corrupt cache file, source file vanishing mid-check) are now logged at WARNING, visible at the CLI's default log level, while the one path with a documented, legitimately expected trigger (diffing against a since- deleted file) stays at debug; annotate_section_summary's read-modify- write cycle now runs under an exclusive file lock (mirroring decision_log.py's established fcntl pattern), closing a lost-update race between two concurrent calls annotating different sections of the same document. - commands/doc_index.py: a non-UTF-8 file now reports a clean ERROR result instead of crashing with a raw traceback; --help now explains that section_level is inferred from heading frequency, not a fixed level. - commands/validate_toc.py: a read failure on one file in a multi-file batch (permission denied, binary content, a TOCTOU race) is now recorded as that file's own ERROR result instead of raising out of the loop and discarding every result already collected for files validated earlier in the same invocation; extracted _validate_one_file() to keep cmd_validate_toc's own local-variable count under pylint's threshold. Co-Authored-By: Claude Sonnet 5 Signed-off-by: TECK KEAT WILSON --- .../features/traceability-validation.md | 1 + .../scripts/studio/commands/doc_index.py | 5 +- .../scripts/studio/commands/validate_toc.py | 95 +++++++------ .../studio/scripts/studio/utils/doc_index.py | 129 ++++++++++++++---- tests/test_doc_index.py | 122 ++++++++++++++--- tests/test_toc.py | 28 ++++ 6 files changed, 290 insertions(+), 90 deletions(-) diff --git a/architecture/features/traceability-validation.md b/architecture/features/traceability-validation.md index 8046cace..25afc650 100644 --- a/architecture/features/traceability-validation.md +++ b/architecture/features/traceability-validation.md @@ -403,6 +403,7 @@ Catches structural and traceability issues that AI agents miss or hallucinate **Supporting**: - [x] - `p1` - Imports and module setup for validate-toc command - `inst-toc-imports` +- [x] - `p1` - Validate a single file, never raising: a missing file or a read failure (permission denied, binary/non-UTF-8 content, a TOCTOU race) is reported as its own ERROR result rather than aborting the whole batch and discarding results already collected for earlier files - `inst-toc-validate-one` - [x] - `p1` - Human-friendly formatter for validate-toc output: a WARN-only file prints its warnings the same way a FAIL file prints its errors, not just the bare status - `inst-toc-format` ### TOC Utilities diff --git a/skills/studio/scripts/studio/commands/doc_index.py b/skills/studio/scripts/studio/commands/doc_index.py index 33275a60..ba512b50 100644 --- a/skills/studio/scripts/studio/commands/doc_index.py +++ b/skills/studio/scripts/studio/commands/doc_index.py @@ -23,7 +23,10 @@ def cmd_doc_index(argv: List[str]) -> int: prog="cfs doc-index", description=( "Build or reuse a cached heading/section index for a Markdown file, " - "so navigation reads the file's structure once, not once per query." + "so navigation reads the file's structure once, not once per query. " + "section_level is inferred from the most frequently repeated heading " + "level (ties prefer the shallower level); a level used only once is " + "never chosen." ), ) p.add_argument("file", help="Markdown file path") diff --git a/skills/studio/scripts/studio/commands/validate_toc.py b/skills/studio/scripts/studio/commands/validate_toc.py index e0404141..d1e49ebd 100644 --- a/skills/studio/scripts/studio/commands/validate_toc.py +++ b/skills/studio/scripts/studio/commands/validate_toc.py @@ -14,10 +14,58 @@ from pathlib import Path from typing import List +from ..utils import error_codes as EC from ..utils.toc import DEFAULT_MAX_SECTION_LINES, add_toc_max_level_argument, validate_toc from ..utils.ui import ui # @cpt-end:cpt-studio-algo-traceability-validation-validate-toc:p1:inst-toc-imports +# @cpt-begin:cpt-studio-algo-traceability-validation-validate-toc:p1:inst-toc-validate-one +def _validate_one_file(filepath: Path, args: argparse.Namespace) -> dict: + """Validate a single file, returning its result dict. Never raises -- + a missing file or a read failure (permission denied, binary/non-UTF-8 + content, a TOCTOU race) is reported as an ERROR result instead, so one + bad file in a batch can't abort validation of the rest. + """ + if not filepath.is_file(): + return { + "file": str(filepath), + "status": "ERROR", + "message": "File not found", + "code": EC.FILE_LOAD_ERROR, + } + + try: + content = filepath.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError) as exc: + return { + "file": str(filepath), + "status": "ERROR", + "message": f"Could not read file: {exc}", + "code": EC.FILE_READ_ERROR, + } + + report = validate_toc( + content, + artifact_path=filepath, + max_heading_level=args.max_level, + max_section_lines=args.max_section_lines, + ) + errors = report.get("errors", []) + warnings = report.get("warnings", []) + file_result: dict = { + "file": str(filepath), + "status": "FAIL" if errors else ("WARN" if warnings else "PASS"), + "error_count": len(errors), + "warning_count": len(warnings), + } + if args.verbose or errors: + file_result["errors"] = errors + if args.verbose or warnings: + file_result["warnings"] = warnings + return file_result +# @cpt-end:cpt-studio-algo-traceability-validation-validate-toc:p1:inst-toc-validate-one + + def cmd_validate_toc(argv: List[str]) -> int: """Validate Table of Contents in markdown files.""" # @cpt-begin:cpt-studio-algo-traceability-validation-validate-toc:p1:inst-toc-parse-args @@ -46,50 +94,19 @@ def cmd_validate_toc(argv: List[str]) -> int: # @cpt-end:cpt-studio-algo-traceability-validation-validate-toc:p1:inst-toc-parse-args # @cpt-begin:cpt-studio-algo-traceability-validation-validate-toc:p1:inst-toc-resolve-files - results = [] - total_errors = 0 - total_warnings = 0 files_to_validate = [Path(f).resolve() for f in args.files] # @cpt-end:cpt-studio-algo-traceability-validation-validate-toc:p1:inst-toc-resolve-files # @cpt-begin:cpt-studio-algo-traceability-validation-validate-toc:p1:inst-toc-foreach-file - for filepath in files_to_validate: - - if not filepath.is_file(): - results.append({ - "file": str(filepath), - "status": "ERROR", - "message": "File not found", - }) + results = [_validate_one_file(filepath, args) for filepath in files_to_validate] + total_errors = 0 + total_warnings = 0 + for file_result in results: + if file_result["status"] == "ERROR": total_errors += 1 - continue - - content = filepath.read_text(encoding="utf-8") - report = validate_toc( - content, - artifact_path=filepath, - max_heading_level=args.max_level, - max_section_lines=args.max_section_lines, - ) - - errors = report.get("errors", []) - warnings = report.get("warnings", []) - total_errors += len(errors) - total_warnings += len(warnings) - - file_result: dict = { - "file": str(filepath), - "status": "FAIL" if errors else ("WARN" if warnings else "PASS"), - "error_count": len(errors), - "warning_count": len(warnings), - } - - if args.verbose or errors: - file_result["errors"] = errors - if args.verbose or warnings: - file_result["warnings"] = warnings - - results.append(file_result) + else: + total_errors += file_result["error_count"] + total_warnings += file_result["warning_count"] # @cpt-end:cpt-studio-algo-traceability-validation-validate-toc:p1:inst-toc-foreach-file # @cpt-begin:cpt-studio-algo-traceability-validation-validate-toc:p1:inst-toc-return diff --git a/skills/studio/scripts/studio/utils/doc_index.py b/skills/studio/scripts/studio/utils/doc_index.py index f6960012..f239b260 100644 --- a/skills/studio/scripts/studio/utils/doc_index.py +++ b/skills/studio/scripts/studio/utils/doc_index.py @@ -12,6 +12,18 @@ Scope: Markdown only. PDF/DOCX conversion is a separate concern (Layer 1); this module operates purely on already-plain-text content (Layer 2). +Schema contract: adding a new top-level key to the index dict is always +additive and does not require bumping ``_SCHEMA_VERSION`` -- an older +build of this module simply never wrote that key, which +``_has_schema_current_index``'s required-field check already treats as +"predates the current schema" and rebuilds. ``_SCHEMA_VERSION`` exists for +the other kind of change: an existing key's *meaning* or *shape* changing +incompatibly (e.g. what ``retrieval_sections`` entries contain), which a +field-presence check alone can't detect since the field is still there, +just holding something a new reader would misinterpret. Bump +``_SCHEMA_VERSION`` for that kind of change; a plain new field needs only +listing in ``_REQUIRED_INDEX_FIELDS`` if a consumer reads it unconditionally. + See constructorfabric/studio#104. @cpt-algo:cpt-studio-algo-traceability-validation-doc-index:p1 @@ -85,8 +97,11 @@ def _index_cache_path(path: Path) -> Optional[Path]: except OSError as exc: # A file whose parent can't be stat'd (permissions, a race) is not a # reason to fail the caller -- just an uncached build, like "no - # Studio directory found". - logger.debug("doc-index cache path lookup skipped for %s: %s", path, exc) + # Studio directory found". Warning, not debug: this is a genuine + # anomaly (unlike the ordinary, unlogged "no Studio directory" + # case below), and should be visible at the CLI's default log + # level rather than indistinguishable from a routine cache miss. + logger.warning("doc-index cache path lookup failed for %s: %s", path, exc) studio_dir = None if studio_dir is None: return None @@ -149,10 +164,15 @@ def _build_retrieval_sections( whichever ``section_level`` section it falls under, rather than splitting a real section apart. - Each section's ``hash`` is a SHA-256 of its own text slice -- the - per-section granularity :func:`diff_stale_sections` needs to tell "this - one section changed" from "the whole file changed", which a whole-file - fingerprint structurally cannot do. + Each section's ``hash`` is a SHA-256 of its own text slice, with each + line's trailing whitespace stripped before hashing -- a harmless + "trim trailing whitespace on save" edit (a common editor/IDE default) + changes no meaningful content and must not look like a real edit to + :func:`diff_stale_sections`, which is the whole point of hashing at + section granularity in the first place. The per-section granularity + :func:`diff_stale_sections` needs to tell "this one section changed" + from "the whole file changed", which a whole-file fingerprint + structurally cannot do. """ if section_level is None: return [] @@ -161,12 +181,12 @@ def _build_retrieval_sections( sections: List[Dict[str, Any]] = [] for i, (text, line_start) in enumerate(marks): line_end = marks[i + 1][1] - 1 if i + 1 < len(marks) else line_count - section_text = "\n".join(lines[line_start - 1:line_end]) + hash_text = "\n".join(line.rstrip() for line in lines[line_start - 1:line_end]) sections.append({ "heading": text, "line_start": line_start, "line_end": line_end, - "hash": hashlib.sha256(section_text.encode("utf-8")).hexdigest(), + "hash": hashlib.sha256(hash_text.encode("utf-8")).hexdigest(), "summary": None, }) return sections @@ -264,7 +284,12 @@ def _read_cache_file(cache_path: Path) -> Optional[Dict[str, Any]]: try: return json.loads(cache_path.read_text(encoding="utf-8")) except (json.JSONDecodeError, OSError) as exc: - logger.debug("doc-index cache unreadable at %s: %s", cache_path, exc) + # Reached only once the caller has already confirmed the cache file + # exists, so a failure here is real corruption or a permissions + # problem, not a routine cache miss -- warning, not debug, so it's + # visible at the CLI's default log level instead of masquerading + # as an ordinary first-time build. + logger.warning("doc-index cache unreadable at %s: %s", cache_path, exc) return None @@ -313,7 +338,10 @@ def load_doc_index(path: Path) -> Optional[Dict[str, Any]]: try: current_etag = _compute_etag(canonical_path) except OSError as exc: - logger.debug("doc-index staleness check failed for %s: %s", path, exc) + # The cache file was just confirmed to exist, so a stat() failure + # on the *source* file here means it vanished or became unreadable + # since -- a real anomaly, not a routine miss. + logger.warning("doc-index staleness check failed for %s: %s", path, exc) return None if cached.get("etag") != current_etag: @@ -374,7 +402,13 @@ def _compute_fresh_retrieval_sections(path: Path) -> Optional[List[Dict[str, Any canonical_path = path.resolve() try: content = canonical_path.read_text(encoding="utf-8") - except OSError as exc: + except (OSError, UnicodeDecodeError) as exc: + # Deliberately still debug, unlike this module's other fallback + # logs: this one path has a genuinely expected trigger ("the file + # was deleted after it was cached", per this function's own + # contract) alongside the anomalous ones, so promoting it would + # make a normal outcome noisy rather than making a real anomaly + # visible. logger.debug("doc-index section diff failed for %s: %s", path, exc) return None @@ -451,6 +485,31 @@ def diff_stale_sections(path: Path) -> Optional[Dict[str, Any]]: # @cpt-end:cpt-studio-algo-traceability-validation-doc-index:p1:inst-doc-index-diff-stale +def _with_cache_lock(cache_path: Path, fn): + """Run ``fn()`` -- a read-modify-write cycle against ``cache_path`` -- + under an exclusive lock on a sibling ``.lock`` file, serializing + concurrent callers so two overlapping read-modify-write cycles (e.g. + two :func:`annotate_section_summary` calls for different sections of + the same document, running from separate processes) can't each load + the same base index, mutate their own part, and have whichever writes + last silently discard the other's update. Mirrors + :func:`studio.utils.decision_log._append_locked`'s exact fallback: an + exclusive ``fcntl`` lock where available (POSIX), otherwise runs + ``fn()`` unlocked on platforms without it (e.g. Windows) -- atomicity + of each individual write is already guaranteed by :func:`save_doc_index` + regardless; only the cross-call serialization is best-effort there. + """ + try: + import fcntl # pylint: disable=import-outside-toplevel + except ImportError: + return fn() + lock_path = cache_path.with_name(f"{cache_path.name}.lock") + lock_path.parent.mkdir(parents=True, exist_ok=True) + with open(lock_path, "a", encoding="utf-8") as lock_fh: + fcntl.flock(lock_fh.fileno(), fcntl.LOCK_EX) + return fn() + + # @cpt-begin:cpt-studio-algo-traceability-validation-doc-index:p1:inst-doc-index-annotate def annotate_section_summary(path: Path, line_start: int, summary: str) -> bool: """Attach a one-line summary to a cached section, keyed by its line_start. @@ -468,25 +527,37 @@ def annotate_section_summary(path: Path, line_start: int, summary: str) -> bool: heading that isn't itself a retrieval section's start) updates only that list, which is correct: there is no corresponding retrieval section to update. - """ - index = load_doc_index(path) - if index is None: - return False - matched = False - for section in index["sections"]: - if section["line_start"] == line_start: - section["summary"] = summary - matched = True - break - if not matched: + The read-modify-write cycle (load, mutate one section, save) runs + under :func:`_with_cache_lock`, so two concurrent calls annotating + different sections of the same document don't race and silently drop + one side's update. + """ + cache_path = _index_cache_path(path) + if cache_path is None: return False - for retrieval_section in index.get("retrieval_sections", []): - if retrieval_section["line_start"] == line_start: - retrieval_section["summary"] = summary - break - - save_doc_index(path, index) - return True + def _read_modify_write() -> bool: + index = load_doc_index(path) + if index is None: + return False + + matched = False + for section in index["sections"]: + if section["line_start"] == line_start: + section["summary"] = summary + matched = True + break + if not matched: + return False + + for retrieval_section in index.get("retrieval_sections", []): + if retrieval_section["line_start"] == line_start: + retrieval_section["summary"] = summary + break + + save_doc_index(path, index) + return True + + return _with_cache_lock(cache_path, _read_modify_write) # @cpt-end:cpt-studio-algo-traceability-validation-doc-index:p1:inst-doc-index-annotate diff --git a/tests/test_doc_index.py b/tests/test_doc_index.py index f295a5c8..e7f47330 100644 --- a/tests/test_doc_index.py +++ b/tests/test_doc_index.py @@ -99,6 +99,19 @@ def test_retrieval_section_hash_changes_only_for_the_edited_section(self, tmp_pa assert by_heading_before["Section A"] != by_heading_after["Section A"] assert by_heading_before["Section B"] == by_heading_after["Section B"] + def test_trailing_whitespace_only_edit_does_not_change_the_hash(self, tmp_path: Path): + """CodeRabbit PR #109: a "trim trailing whitespace on save" editor + default changes no meaningful content and must not look like a + real edit to diff_stale_sections -- the whole point of hashing at + section granularity.""" + f = _write(tmp_path) + before = build_doc_index(f) + f.write_text(_SAMPLE.replace("Body of A.\n", "Body of A. \n"), encoding="utf-8") + after = build_doc_index(f) + by_heading_before = {s["heading"]: s["hash"] for s in before["retrieval_sections"]} + by_heading_after = {s["heading"]: s["hash"] for s in after["retrieval_sections"]} + assert by_heading_before["Section A"] == by_heading_after["Section A"] + class TestInferSectionLevel: def test_uniform_level_is_chosen(self): @@ -311,7 +324,7 @@ def _spy(start_path): assert seen_paths, "find_studio_directory was never called" assert seen_paths[0] == f.resolve().parent - def test_load_returns_none_on_corrupt_cache_file(self, tmp_path: Path, monkeypatch): + def test_load_returns_none_on_corrupt_cache_file(self, tmp_path: Path, monkeypatch, caplog): monkeypatch.setattr("studio.utils.files.find_studio_directory", lambda *_a, **_k: tmp_path) f = _write(tmp_path) save_doc_index(f, build_doc_index(f)) @@ -319,7 +332,12 @@ def test_load_returns_none_on_corrupt_cache_file(self, tmp_path: Path, monkeypat cache_dir = tmp_path / ".cache" / "doc-index" for cache_file in cache_dir.glob("*.json"): cache_file.write_text("{not valid json", encoding="utf-8") - assert load_doc_index(f) is None + with caplog.at_level("WARNING"): + assert load_doc_index(f) is None + # CodeRabbit PR #109: real corruption is a genuine anomaly, not a + # routine cache miss -- must be visible at the CLI's default log + # level (WARNING), not buried at debug. + assert any(r.levelname == "WARNING" for r in caplog.records) def test_cmd_doc_index_rebuilds_cleanly_after_corrupt_cache(self, tmp_path: Path, capsys, monkeypatch): """CodeRabbit PR #108: prove the corrupt-cache fallback at the @@ -382,18 +400,23 @@ def test_no_studio_directory_means_no_crash_and_always_none(self, tmp_path: Path assert load_doc_index(f) is None def test_studio_directory_lookup_error_means_no_crash_and_no_cache( - self, tmp_path: Path, monkeypatch + self, tmp_path: Path, monkeypatch, caplog ): """An OSError from find_studio_directory (e.g. an unreadable parent directory) must degrade to 'no cache', not raise -- and it must be - logged, not silently swallowed (see PR #108 review / pylint W9001).""" + logged, not silently swallowed (see PR #108 review / pylint W9001). + CodeRabbit PR #109: this is a genuine anomaly, distinct from the + ordinary (unlogged) "no Studio directory found" case, so it must + log at WARNING, visible at the CLI's default level, not DEBUG.""" def _raise(_start_path): raise OSError("permission denied") monkeypatch.setattr("studio.utils.files.find_studio_directory", _raise) f = _write(tmp_path) - save_doc_index(f, build_doc_index(f)) # must no-op, not raise - assert load_doc_index(f) is None + with caplog.at_level("WARNING"): + save_doc_index(f, build_doc_index(f)) # must no-op, not raise + assert load_doc_index(f) is None + assert any(r.levelname == "WARNING" for r in caplog.records) def test_load_returns_none_when_file_deleted_after_caching(self, tmp_path: Path, monkeypatch): monkeypatch.setattr("studio.utils.files.find_studio_directory", lambda *_a, **_k: tmp_path) @@ -492,6 +515,53 @@ def test_off_level_heading_leaves_retrieval_sections_untouched(self, tmp_path: P assert a1["summary"] == "About A.1." assert all(s["summary"] is None for s in index["retrieval_sections"]) + def test_concurrent_annotations_of_different_sections_do_not_lose_either_update( + self, tmp_path: Path, monkeypatch + ): + """CodeRabbit PR #109: two concurrent read-modify-write cycles + annotating different sections of the same document must not race -- + without the lock, thread B's load could happen before thread A's + save, so thread B's own save would overwrite thread A's summary + with a stale base index. Injects a delay inside the locked section + (between load and save) to force a real overlap window if the lock + weren't actually serializing the two calls.""" + import threading + import time as time_module + + from studio.utils import doc_index as di + + monkeypatch.setattr("studio.utils.files.find_studio_directory", lambda *_a, **_k: tmp_path) + f = _write(tmp_path) + index = get_or_build_doc_index(f) + line_a = index["sections"][1]["line_start"] # "Section A" + line_b = index["sections"][3]["line_start"] # "Section B" + + original_save = di.save_doc_index + + def slow_save(path, saved_index): + time_module.sleep(0.1) + original_save(path, saved_index) + + monkeypatch.setattr(di, "save_doc_index", slow_save) + + results: dict = {} + + def run(line_start: int, summary: str) -> None: + results[line_start] = di.annotate_section_summary(f, line_start, summary) + + t1 = threading.Thread(target=run, args=(line_a, "Summary A")) + t2 = threading.Thread(target=run, args=(line_b, "Summary B")) + t1.start() + t2.start() + t1.join(timeout=5) + t2.join(timeout=5) + + assert results == {line_a: True, line_b: True} + final = di.load_doc_index(f) + by_line = {s["line_start"]: s["summary"] for s in final["sections"]} + assert by_line[line_a] == "Summary A" + assert by_line[line_b] == "Summary B" + class TestReadWithStableEtag: def test_retries_when_the_file_changes_mid_read(self, tmp_path: Path, monkeypatch): @@ -540,21 +610,6 @@ def test_missing_file(self, tmp_path: Path, capsys): out = json.loads(capsys.readouterr().out) assert out["status"] == "ERROR" - def test_non_utf8_file_reports_a_clean_error_not_a_raw_traceback( - self, tmp_path: Path, capsys, monkeypatch - ): - """CodeRabbit PR #108 (round 2): a non-UTF-8 file raises - UnicodeDecodeError inside build_doc_index's read_text -- this must - surface as a clean exit-code-2 JSON error, the same contract a - missing file already gets, not an unhandled traceback.""" - monkeypatch.setattr("studio.utils.files.find_studio_directory", lambda *_a, **_k: tmp_path) - f = tmp_path / "bad.md" - f.write_bytes(b"# Title\n\xff\xfe not valid utf-8\n") - rc = cmd_doc_index([str(f)]) - assert rc == 2 - out = json.loads(capsys.readouterr().out) - assert out["status"] == "ERROR" - def test_unreadable_file_reports_a_clean_error_not_a_raw_traceback( self, tmp_path: Path, capsys, monkeypatch ): @@ -597,6 +652,31 @@ def test_json_output_exposes_retrieval_sections(self, tmp_path: Path, capsys, mo assert [s["heading"] for s in out["retrieval_sections"]] == ["Section A", "Section B"] assert "hash" in out["retrieval_sections"][0] + def test_non_utf8_file_reports_a_clean_error_not_a_raw_traceback(self, tmp_path: Path, capsys, monkeypatch): + """CodeRabbit PR #109: a binary/non-UTF-8 file used to crash with an + unhandled UnicodeDecodeError; must now report a clean ERROR result, + consistent with the existing "File not found" pattern.""" + monkeypatch.setattr("studio.utils.files.find_studio_directory", lambda *_a, **_k: tmp_path) + f = tmp_path / "binary.md" + f.write_bytes(b"\xff\xfe\x00\x01garbage") + rc = cmd_doc_index([str(f)]) + assert rc == 2 + out = json.loads(capsys.readouterr().out) + assert out["status"] == "ERROR" + assert "utf-8" in out["message"].lower() or "UTF-8" in out["message"] + + def test_help_explains_how_section_level_is_inferred(self): + """CodeRabbit PR #109: --help gave no indication that section_level + is a heuristic, not simply "H1/H2".""" + import io + from contextlib import redirect_stdout + + buf = io.StringIO() + with redirect_stdout(buf): + with pytest.raises(SystemExit): + cmd_doc_index(["--help"]) + assert "most frequently" in buf.getvalue() + def test_human_output_lists_retrieval_sections(self, tmp_path: Path, capsys, monkeypatch): from studio.utils.ui import is_json_mode, set_json_mode diff --git a/tests/test_toc.py b/tests/test_toc.py index 52ad526e..4e80f995 100644 --- a/tests/test_toc.py +++ b/tests/test_toc.py @@ -1248,6 +1248,34 @@ def test_multiple_files(self, tmp_path: Path, capsys): assert out["files_validated"] == 2 assert out["error_count"] == 1 + def test_a_read_failure_on_one_file_does_not_abort_the_batch(self, tmp_path: Path, capsys): + """CodeRabbit PR #109: an unhandled read failure on one file used to + raise out of the per-file loop, discarding results already + collected for files validated earlier in the same invocation and + never reaching the remaining files. A binary/non-UTF-8 file in the + middle of a batch must be recorded as its own ERROR result, and the + batch must still validate the file(s) after it.""" + good = tmp_path / "good.md" + good.write_text( + "# T\n\n## Table of Contents\n\n1. [A](#a)\n\n---\n\n## A\n", + encoding="utf-8", + ) + binary = tmp_path / "binary.md" + binary.write_bytes(b"\xff\xfe\x00\x01garbage") + good2 = tmp_path / "good2.md" + good2.write_text( + "# T\n\n## Table of Contents\n\n1. [B](#b)\n\n---\n\n## B\n", + encoding="utf-8", + ) + rc = cmd_validate_toc(["--max-level", "2", str(good), str(binary), str(good2)]) + assert rc == 2 + out = json.loads(capsys.readouterr().out) + assert out["files_validated"] == 3 + by_file = {r["file"]: r for r in out["results"]} + assert by_file[str(good)]["status"] == "PASS" + assert by_file[str(binary)]["status"] == "ERROR" + assert by_file[str(good2)]["status"] == "PASS" + def test_verbose_flag(self, tmp_path: Path, capsys): f = tmp_path / "doc.md" f.write_text( From dc3c8d30c5a42a3bf5fe26f62e43b4a0263175d6 Mon Sep 17 00:00:00 2001 From: TECK KEAT WILSON Date: Tue, 1 Sep 2026 10:47:23 +0800 Subject: [PATCH 5/5] fix(test-doc-index): make new caplog assertions robust to suite ordering Whichever CLI test runs first in the full suite triggers cli.py's own _configure_studio_logging(), which sets the "studio" logger's propagate to False for the rest of the process -- a real ambient global-state mutation that silently blocks pytest's caplog (listening on the root logger) from seeing any "studio.*" child logger's records for every test that runs afterward. The two new WARNING-level log assertions from the previous commit passed in isolation but failed as part of the full suite for exactly this reason. Added a studio_logger_propagates fixture that temporarily restores propagation for the duration of a test, and used it in both affected tests. Verified against the actual failure condition by forcing propagate=False before running them. Co-Authored-By: Claude Sonnet 5 Signed-off-by: TECK KEAT WILSON --- tests/test_doc_index.py | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/tests/test_doc_index.py b/tests/test_doc_index.py index e7f47330..5e9d7943 100644 --- a/tests/test_doc_index.py +++ b/tests/test_doc_index.py @@ -6,6 +6,7 @@ from __future__ import annotations import json +import logging import os from pathlib import Path @@ -40,6 +41,29 @@ def _write(tmp_path: Path, content: str = _SAMPLE, name: str = "doc.md") -> Path return f +@pytest.fixture +def studio_logger_propagates(): + """Force the "studio" logger to propagate for the duration of a test. + + Whichever CLI test runs first in the full suite triggers + cli.py's own _configure_studio_logging(), which sets + logging.getLogger("studio").propagate = False for the rest of the + process -- a real, ambient global-state mutation, not something this + test file controls. That silently blocks pytest's caplog (which + listens on the root logger) from ever seeing a child logger's records + for any test that runs after it. Tests asserting on log output for + "studio.utils.doc_index" opt into this fixture to stay correct + regardless of suite ordering, restoring the original value afterward. + """ + studio_logger = logging.getLogger("studio") + original = studio_logger.propagate + studio_logger.propagate = True + try: + yield + finally: + studio_logger.propagate = original + + class TestBuildDocIndex: def test_extracts_sections_with_line_ranges(self, tmp_path: Path): f = _write(tmp_path) @@ -324,7 +348,9 @@ def _spy(start_path): assert seen_paths, "find_studio_directory was never called" assert seen_paths[0] == f.resolve().parent - def test_load_returns_none_on_corrupt_cache_file(self, tmp_path: Path, monkeypatch, caplog): + def test_load_returns_none_on_corrupt_cache_file( + self, tmp_path: Path, monkeypatch, caplog, studio_logger_propagates + ): monkeypatch.setattr("studio.utils.files.find_studio_directory", lambda *_a, **_k: tmp_path) f = _write(tmp_path) save_doc_index(f, build_doc_index(f)) @@ -400,7 +426,7 @@ def test_no_studio_directory_means_no_crash_and_always_none(self, tmp_path: Path assert load_doc_index(f) is None def test_studio_directory_lookup_error_means_no_crash_and_no_cache( - self, tmp_path: Path, monkeypatch, caplog + self, tmp_path: Path, monkeypatch, caplog, studio_logger_propagates ): """An OSError from find_studio_directory (e.g. an unreadable parent directory) must degrade to 'no cache', not raise -- and it must be