From 512975d94c742b768036bc35d8b4c06119c9276a Mon Sep 17 00:00:00 2001 From: TECK KEAT WILSON Date: Fri, 28 Aug 2026 15:15:41 +0800 Subject: [PATCH 1/3] 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 06d24504..286a8127 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**: A structural index (headings, section line ranges, per-section summary slots) cached per file, keyed by a stat-based fingerprint +**Output**: A structural index (every heading's line range, plus a coarser "one chunk per real section" grouping at an inferred heading level, each with a content hash and a summary slot) cached per file, keyed by a stat-based fingerprint A cached, read-once-per-file structural index for Markdown JIT retrieval (see constructorfabric/studio#104): parsing a file's headings/section boundaries @@ -462,6 +462,9 @@ by requiring the read it's meant to save. 3. [x] - `p1` - Persist an index to its cache location; 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 ccca4df7..19cdc71d 100644 --- a/skills/studio/scripts/studio/utils/doc_index.py +++ b/skills/studio/scripts/studio/utils/doc_index.py @@ -23,8 +23,9 @@ import json import logging 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 @@ -82,6 +83,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. @@ -89,6 +167,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() content = canonical_path.read_text(encoding="utf-8") @@ -107,16 +192,36 @@ def build_doc_index(path: Path) -> Dict[str, Any]: "summary": None, }) + section_level = infer_section_level(headings) + return { "path": str(canonical_path), "etag": _compute_etag(canonical_path), "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 def load_doc_index(path: Path) -> Optional[Dict[str, Any]]: """Load a cached index for ``path``, or ``None`` if missing/stale/absent. @@ -131,10 +236,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() @@ -184,6 +287,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 7bb3008a..060976e2 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" @@ -73,6 +76,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 ecf54484..75398638 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 66b9cc27ab38843920914db5b0b5c61a4fd66dfc Mon Sep 17 00:00:00 2001 From: TECK KEAT WILSON Date: Mon, 31 Aug 2026 09:28:20 +0800 Subject: [PATCH 2/3] 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 | 111 ++++++++++++---- skills/studio/scripts/studio/utils/toc.py | 28 +++- tests/test_doc_index.py | 122 +++++++++++++++++- tests/test_toc.py | 55 ++++++++ 6 files changed, 303 insertions(+), 31 deletions(-) diff --git a/architecture/features/traceability-validation.md b/architecture/features/traceability-validation.md index 286a8127..1f17a984 100644 --- a/architecture/features/traceability-validation.md +++ b/architecture/features/traceability-validation.md @@ -455,7 +455,10 @@ happens once, not once per query, until the file's content actually 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 - `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); returns `None` if missing, stale, or corrupt - `inst-doc-index-load` @@ -469,6 +472,8 @@ by requiring the read it's meant to save. **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` - [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` - 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 93f5e03b..5ea1633e 100644 --- a/skills/studio/scripts/studio/commands/doc_index.py +++ b/skills/studio/scripts/studio/commands/doc_index.py @@ -48,6 +48,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 @@ -62,3 +65,11 @@ 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() diff --git a/skills/studio/scripts/studio/utils/doc_index.py b/skills/studio/scripts/studio/utils/doc_index.py index 19cdc71d..b13b198a 100644 --- a/skills/studio/scripts/studio/utils/doc_index.py +++ b/skills/studio/scripts/studio/utils/doc_index.py @@ -160,6 +160,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. @@ -176,7 +208,7 @@ def build_doc_index(path: Path) -> Dict[str, Any]: for why a fixed heading level can't be assumed. """ canonical_path = path.resolve() - content = canonical_path.read_text(encoding="utf-8") + content, etag = _read_with_stable_etag(canonical_path) lines = content.split("\n") line_count = len(lines) @@ -196,7 +228,7 @@ def build_doc_index(path: Path) -> Dict[str, Any]: return { "path": str(canonical_path), - "etag": _compute_etag(canonical_path), + "etag": etag, "built_at": time.strftime("%Y-%m-%dT%H:%M:%SZ", time.gmtime()), "total_lines": line_count, "sections": sections, @@ -287,6 +319,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* @@ -304,12 +362,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 @@ -323,30 +384,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 @@ -359,6 +412,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: @@ -373,6 +435,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 1d65a6c4..fefccd01 100644 --- a/skills/studio/scripts/studio/utils/toc.py +++ b/skills/studio/scripts/studio/utils/toc.py @@ -838,7 +838,7 @@ def _check_section_lengths( return warnings -_DESCRIPTION_FIELD_RE = re.compile(r"^description\s*:\s*\S") +_DESCRIPTION_FIELD_RE = re.compile(r"^description\s*:\s*(.*)$") def _frontmatter_has_description(lines: List[str], frontmatter_end: int) -> bool: @@ -847,11 +847,29 @@ 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. """ - return any( - _DESCRIPTION_FIELD_RE.match(line.strip()) - for line in lines[1:frontmatter_end - 1] - ) + for line in lines[1:frontmatter_end - 1]: + match = _DESCRIPTION_FIELD_RE.match(line.strip()) + if not match: + continue + value = match.group(1).strip() + if not value or value.startswith("#"): + continue + if value[0] in "\"'": + quote = value[0] + closing = value.find(quote, 1) + inner = value[1:closing] if closing != -1 else value[1:] + if not inner.strip(): + continue + return True + return False def _check_missing_description( diff --git a/tests/test_doc_index.py b/tests/test_doc_index.py index 060976e2..62f15418 100644 --- a/tests/test_doc_index.py +++ b/tests/test_doc_index.py @@ -158,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) @@ -167,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) @@ -185,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: @@ -356,6 +374,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): @@ -373,6 +458,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 da5f17fa..d11e45b6 100644 --- a/tests/test_toc.py +++ b/tests/test_toc.py @@ -896,6 +896,61 @@ def test_frontmatter_without_description_field_still_warns(self): codes = [w["code"] for w in result["warnings"]] assert "toc-missing-description" in codes + 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" + "description: # TODO write this\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_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_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 9cf51f4c3ea4b5bba25cafe03279a6cce4d063f5 Mon Sep 17 00:00:00 2001 From: TECK KEAT WILSON Date: Mon, 31 Aug 2026 09:42:08 +0800 Subject: [PATCH 3/3] 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 | 13 +++ skills/studio/scripts/studio/utils/toc.py | 44 +++++++--- tests/test_doc_index.py | 37 +++++++++ tests/test_toc.py | 80 +++++++++++++++++++ 4 files changed, 163 insertions(+), 11 deletions(-) diff --git a/skills/studio/scripts/studio/utils/doc_index.py b/skills/studio/scripts/studio/utils/doc_index.py index b13b198a..6c3563b8 100644 --- a/skills/studio/scripts/studio/utils/doc_index.py +++ b/skills/studio/scripts/studio/utils/doc_index.py @@ -255,6 +255,9 @@ 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 = ("section_level", "retrieval_sections") + + def load_doc_index(path: Path) -> Optional[Dict[str, Any]]: """Load a cached index for ``path``, or ``None`` if missing/stale/absent. @@ -263,6 +266,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(): @@ -281,6 +291,9 @@ def load_doc_index(path: Path) -> Optional[Dict[str, Any]]: if cached.get("etag") != current_etag: return None + if any(field not in cached for field in _REQUIRED_INDEX_FIELDS): + logger.debug("doc-index cache for %s predates the current schema; rebuilding", path) + return None return cached # @cpt-end:cpt-studio-algo-traceability-validation-doc-index:p1:inst-doc-index-load diff --git a/skills/studio/scripts/studio/utils/toc.py b/skills/studio/scripts/studio/utils/toc.py index fefccd01..f9e6bf73 100644 --- a/skills/studio/scripts/studio/utils/toc.py +++ b/skills/studio/scripts/studio/utils/toc.py @@ -839,6 +839,27 @@ def _check_section_lengths( _DESCRIPTION_FIELD_RE = re.compile(r"^description\s*:\s*(.*)$") +_BLOCK_SCALAR_RE = re.compile(r"^[|>][+\-]?\d*$") + + +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 def _frontmatter_has_description(lines: List[str], frontmatter_end: int) -> bool: @@ -849,25 +870,26 @@ 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 not match: continue value = match.group(1).strip() if not value or value.startswith("#"): continue - if value[0] in "\"'": - quote = value[0] - closing = value.find(quote, 1) - inner = value[1:closing] if closing != -1 else value[1:] - if not inner.strip(): + 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 62f15418..55929ff5 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 d11e45b6..2fe647f7 100644 --- a/tests/test_toc.py +++ b/tests/test_toc.py @@ -951,6 +951,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