diff --git a/architecture/features/traceability-validation.md b/architecture/features/traceability-validation.md index 93c04949..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 @@ -447,7 +448,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 @@ -456,19 +457,27 @@ 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` 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` - [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..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") @@ -58,6 +61,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 +81,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/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 4552c533..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 @@ -24,8 +36,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 @@ -84,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 @@ -95,6 +111,120 @@ 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, 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 [] + 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 + 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(hash_text.encode("utf-8")).hexdigest(), + "summary": None, + }) + return 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. @@ -102,15 +232,16 @@ 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 - # 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) @@ -126,6 +257,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,12 +266,35 @@ 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: + # 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 + + # @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: @@ -162,22 +318,30 @@ 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(): 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() 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: @@ -230,6 +394,122 @@ 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, 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 + + 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* + 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": [...]}``, 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), 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 + 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 + + fresh_sections = _compute_fresh_retrieval_sections(path) + if fresh_sections is None: + return None + + old_sections = cached["retrieval_sections"] + if len(old_sections) != len(fresh_sections): + return { + "structural_change": True, + "unchanged": [], + "changed": [_position_entry(s) for s in fresh_sections], + } + + 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 + + +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. @@ -238,20 +518,46 @@ 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. - """ - 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: + 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. + + 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 - 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/skills/studio/scripts/studio/utils/toc.py b/skills/studio/scripts/studio/utils/toc.py index daf6a711..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 @@ -892,11 +896,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``), 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 888b101d..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 @@ -15,10 +16,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" @@ -37,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) @@ -67,41 +94,152 @@ 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 + 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) + 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) - call_order = [] - - real_compute_etag = doc_index_module._compute_etag - real_read_text = Path.read_text + 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 _tracked_compute_etag(path): - call_order.append("etag") - return real_compute_etag(path) + 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"] + + 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): + 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 - 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) +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 - doc_index_module.build_doc_index(f) - assert call_order == ["etag", "read"] + 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_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" + 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 {(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) + 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"] == [{"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) - index = build_doc_index(f) - assert [s["heading"] for s in index["sections"]] == ["Title", "Real", "Also Real"] + 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) + 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"] == [] + assert {e["heading"] for e in diff["changed"]} == {"Section A", "Section B", "Section C"} class TestCachePersistence: @@ -110,6 +248,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) @@ -173,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): + 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)) @@ -181,7 +358,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 @@ -244,18 +426,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, 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 - 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) @@ -327,6 +514,120 @@ 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"]) + + 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): + """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): @@ -335,21 +636,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 ): @@ -378,6 +664,62 @@ 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_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 + + 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..4e80f995 100644 --- a/tests/test_toc.py +++ b/tests/test_toc.py @@ -985,23 +985,149 @@ 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" + "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_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_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" @@ -1122,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( 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