Skip to content

feat: add code-explainer document kind to thurview - #22

Merged
LeTuR merged 4 commits into
mainfrom
feat/code-explainer
Sep 9, 2026
Merged

feat: add code-explainer document kind to thurview#22
LeTuR merged 4 commits into
mainfrom
feat/code-explainer

Conversation

@LeTuR

@LeTuR LeTuR commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Intent

Add a "code-explainer" kind of option to thurview. The operator, verbatim: "I want to add to thurview a code-explainer kind of option. Goal is to allow this tool to provide an overall explaination of code base, allowing user to detect architecture/design issues."

Design the shape first, then ship a first cut. Four decisions were required and are deliberate:

  1. NEW DOCUMENT KIND, NOT A REVIEW WITH RANGE "EVERYTHING". thurviews unit was a CHANGE (anchors pin to commits; diff, commits, interface delta and approve/request-changes all assume a range). The prior workaround was scaffold --base HEAD --head HEAD, which leaves Files reporting 0 files, Commits reporting none and the interface delta reporting "No interface moved" - claims about a change that does not exist. So an explainer is a second kind beside a review, sharing anchors, peeks, map, threads, revisions, theme and the publish/wait loop, and dropping what describes a range. kind is optional on ReviewState so every stored record keeps reading as a review. pins.base == pins.head deliberately, so anchor validation, git-show reads, peeks and symbol lookup are byte-identical to a reviews - that is the integrity property and it must not be weakened.

  2. TABS. Files and Commits are ABSENT in explainer mode, not empty - a tab that renders empty is worse than one that is absent. A new Coverage tab takes their place. A file browser at the pinned commit is explicitly OUT OF SCOPE for this PR and named as the next slice. The decision verbs keep the same state machine and change only their words ("Done reading" / "Send it back"), because a reader of an explainer decides whether the DOCUMENT is finished, not whether code should merge, and "Approve" would read as a verdict.

  3. COVERAGE IS DERIVED, NOT CLAIMED. A codebase does not fit in a short document, so publish accounts for every file in scope at the pinned commit as explained (an anchor points into it), placed (a map nodes globs own it) or not examined, and shows the counts above the document in the slot a review gives the interface delta. An explainer that anchors nothing is REJECTED (deliberate: a review may publish a stub because the reader has the diff meanwhile; an explainer has nothing). An explainer with no map only WARNS (deliberate: "everything unanchored was not examined" is a true statement, just rarely the one meant).

  4. OBSERVATION, NOT JUDGEMENT. The README thesis - "It does not review the code for you. It helps you understand it fast enough to review it yourself" - is a hard constraint. The tool surfaces structure and never grades it. Rule applied: every fact is a count or a list of named things at the pinned commit that the reader could re-derive with thurview graph. NO severities, NO scores, NO "issues found" list, NO thresholds that colour anything, NO recommendations. The Coverage palette deliberately avoids the added/changed/removed colours the review views use. Test-coverage counts were deliberately LEFT OUT of this cut even though they would pass the rule, because in a table beside file counts they read as a grade. "Both ways" is used instead of "cycle" on purpose. Shared-name detection filters to definitions a consumer outside the file could name (reusing graph.ts isNestedNonMethod), because unfiltered it surfaced onclick/draw/expand - the same word, not the same concept.

Also seeded Louvain in architecture(), which was unseeded and clustered the same commit differently between runs; coverage counts derived from it must be stable at a pinned commit.

An earlier run of this pipeline reviewed commit c131204, found 2 findings, fixed them in cbdcdc6 (separating a file the graph cannot read from one the repo-wide MAX_FILES cap dropped, and adding missing coverage badge CSS), and then crashed on a provider session limit before it could re-review. Both fixes are correct and kept. 56893b7 adds the e2e test locking that classification through the CLI and the HTTP payload, and fixes a singular/plural slip.

Constraints held: the review path is additive-only and its 34 pre-existing tests pass unchanged (43 now); every claim stays anchored and anchors still validate through the same code; nothing about the codebase is generated from recollection - it all comes from the code graph at the pinned commit.

The design

1. A second document kind, not a review whose range is "everything"

thurview explain [<path>] [--commit <ref>] creates an explainer: one
pinned commit, an optional path scope, and the same engine as a review.

A review's unit is a range. "Everything" is not a range — it is one commit —
and the workaround the skill documented until now (scaffold --base HEAD --head HEAD) proves why that matters. With base == head nothing breaks, and that is
the problem: the Files tab reports 0 changed files, Commits reports none, and
the interface delta above the document reports "No interface moved". Each of
those is a claim about a change rendered for a document that has no change.
The brief's rule — a tab that renders empty is worse than a tab that is absent —
is exactly the failure that shape produces. The verbs disagree too: Approve and
Request changes decide whether a change should land, and an explainer has
nothing to land.

What the two kinds share is the whole understanding machine, and none of it
depends on a range: anchors, peeks, the map, threads, revisions, the theme, and
the publish → wait → answer loop. So this is one engine with two units, and
the split is a kind field on the stored document (src/store.ts), absent on
every record written before today and therefore read as a review.

Crucially, pins.base == pins.head == <the pinned commit>. Anchor validation,
git show-at-a-commit reads, peeks, symbol lookup and the theme path are
byte-identical to a review's. thurview's integrity property is untouched: an
explainer's claims validate through exactly the code that validates a review's.

2. What each tab does in explainer mode

Tab Review Explainer
Review walkthrough + interface delta Explainer: the document + the coverage panel
Files split diff of changed files absent
Commits commits base..head absent
Map parts, marked added/changed/removed parts at the pinned commit; no marks, no "context" demotion
Coverage absent new: what the document reached, and what it did not
Threads ask / comment / decide same; the decision reads Done reading / Send it back
Revisions every publish sealed unchanged

Files and Commits are absent rather than empty. The reader reaches code through
anchors, peeks and the map's node → code path; a file browser at the pinned
commit is the obvious next slice, not this one.

The decision keeps the same state machine and changes only its words. A reader
of an explainer is deciding whether the document is finished, not whether code
should merge, and "Approve" on a page about existing code would read as the
verdict this product does not give.

3. Staying short while staying honest

Three layers, each carrying what it is good at:

  • System — the map carries breadth. Seeded from thurview graph architecture, which is now scoped to the explainer's path. Every part of the
    scope can appear here, including the parts prose will not reach.
  • Subsystem — the prose carries depth. Three to six sections, selected by
    structural weight from the graph (cluster size, hub fan-in, cross-cluster
    reference counts), and the document is told to state the basis it selected on.
  • File and symbol — anchors carry the proof. Progressive disclosure: the
    reader opens code where they want it.

Coverage is derived at publish, not claimed in prose. Every file in scope at
the pinned commit lands in one of three states:

  • explained — an anchor in the document points into it;
  • placed — a map node's files globs match it and no anchor does;
  • not examined — neither.

The counts sit above the document in the slot a review gives the interface
delta — the same kind of thing, a fact derived from the pinned commit standing
above prose the agent wrote — and the Coverage tab breaks them down per part of
the system, names the files that were skipped, and lists the files in languages
the code graph cannot read. thurview publish prints the same line and the
files it did not examine.

So the honesty is structural rather than diligent: the agent cannot forget to
state coverage, and to move a file out of not examined it has to actually
anchor it or actually place it. Inflation is visible too — the Coverage tab
lists every map node with the globs it owns and how many files they match, so a
**/* on one node cannot quietly cover the repository.

Two guards back this up in publish: an explainer that anchors nothing at all
is rejected (a review may publish a stub because the reader has the diff to read
meanwhile; an explainer has nothing), and an explainer with no map only warns,
because "everything the prose did not anchor was not examined" is a true
statement — just rarely the one the author meant.

4. Where the line between observation and judgement is

The test used, and written into the skill: every fact in an explainer is a
count, or a list of named things, at the pinned commit, that the reader could
re-derive with thurview graph.

Shipped as observations, all from the code graph at that commit:

  • files and symbols per cluster, and each cluster's most-referenced symbols;
  • references crossing between clusters, with counts and whether the pair
    references each other in both directions;
  • names defined in more than one cluster (this run of the tool on its own
    src/ui surfaces exactly one: statusClass, in app.ts and views/map.ts);
  • files in scope the graph cannot read, by extension, and whether its file list
    was capped;
  • the three coverage states themselves.

Deliberately not shipped: severities, scores, an "issues found" list, thresholds
that colour anything, "violates X", "should be split", or any recommendation
section. The Coverage tab's palette is neutral on purpose — it never borrows the
added/changed/removed colours the review views use — because covered/uncovered
is a fact about the document, not a grade on the code.

Two calls worth naming, since the brief asked for the argument rather than the
assumption:

  • "Reaches both ways" instead of "cycle". Stating that A→B and B→A both
    exist, with counts, is two facts. "Circular dependency" is a verdict with a
    remedy implied. The label is the neutral one.
  • Test coverage left out of this cut. "No test file reaches this cluster" is
    a count of zero and would pass the test above, but sitting in a table beside
    file counts it reads as a grade far more readily than the others. The skill
    permits an author to state it in prose; the derived table does not, for now.
  • Shared names are filtered to definitions a consumer outside the file could
    name.
    Before filtering, onclick, draw and expand dominated the list —
    file-local nested definitions that share a word, not a concept. Reusing the
    graph's own isNestedNonMethod rule turned 8 noisy rows into 1 real one.

5. One more thing this fixed

architecture() ran Louvain unseeded, so the same commit clustered differently
between runs. That was already loose for reviews seeding a map; for coverage it
would have moved every derived count on republish. It is seeded now, so a pinned
commit always yields the same structure.

What Changed

  • Add a DocumentKind (review | explainer) to ReviewState/Binding (kind optional, defaulting old records to review) and a new thurview explain CLI command that pins an explainer to a single commit (pins.base == pins.head), reusing anchors, peeks, map, threads and the publish/wait loop while dropping range-only concepts; publish rejects an explainer with no anchors and warns when it has no map.
  • Add src/coverage.ts to derive, at publish time, per-file coverage (explained via an anchor, placed via a map glob, or not examined) across the files in scope at the pinned commit, and a src/ui/views/coverage.ts view (plus app.css styling) that renders those counts in place of the interface-delta panel; explainer mode hides the Files/Commits tabs and relabels review decisions to "Done reading"/"Send it back" (src/ui/views/review.ts, src/ui/app.ts, src/ui/state.ts, src/ui/threads.ts, src/ui/api.ts, src/document/compile.ts, src/server/server.ts).
  • Seed Louvain clustering in graph.ts architecture() so coverage counts derived from it are stable across runs at the same pinned commit, and track unresolved references per file (unresolvedByFile) plus an exported isGraphLanguage helper to distinguish a file the graph cannot parse from one dropped by the repo-wide MAX_FILES cap.
  • Add test/coverage.test.ts and extend test/e2e.test.ts/test/graph.test.ts to cover the new explainer/coverage behavior; document the feature in README.md, skills/thurview/SKILL.md and the new skills/thurview/references/code-explainer.md.

Risk Assessment

✅ Low: All three previously-selected findings (unresolved counts not scoped, architecture truncated flag not scoped, missing capped-branch test coverage) are correctly and durably fixed: unresolvedByFile tracks unresolved references per originating file in buildGraph() and computeCoverage() sums only in-scope files; scopeTruncated() correctly reuses the uncapped allFiles list to compute a scope-aware truncation flag for the CLI's explainer architecture command, replacing the previously unscoped whole-repo flag; and the new test/coverage.test.ts exercises computeCoverage() and scopeTruncated() directly as pure functions with synthetic inputs that actually drive the capped/uncapped and scoped/unscoped branches, satisfying the test-quality bar. No new correctness bugs, intent contradictions, or regressions were found in the fix-round diff.

Testing

Baseline pnpm test && bats scripts/ci/*.bats already passed; on top of that I ran the three targeted files touched by this change's fixes (coverage.test.ts, graph.test.ts, e2e.test.ts — 38 tests, all passing) and then manually drove the real feature end-to-end: created an explainer for src/ on this repo, seeded its map from the actual code graph, published it, served it, and inspected the live rendered page in headless Chromium. The Coverage tab, tab-bar absence of Files/Commits, the reworded Decide dialog, the derived coverage sentence, and the previously-missing badge CSS all render and behave exactly as the user intent specifies. No issues found; the working tree and browser/server processes used for manual verification were cleaned up.

  • Evidence: Coverage tab rendered in browser — banner, by-part-of-system clusters with anchored/placed badges, both-ways reference links (local file: /home/magicletur/.no-mistakes/evidence/01M23FGA44QHJGAY4VQKN8RGBN/coverage-tab.png)
  • Evidence: Reviews list page showing the new 'src' explainer (src/** scope, awaiting-review) listed alongside pre-existing reviews, confirming additive-only integration (local file: /home/magicletur/.no-mistakes/evidence/01M23FGA44QHJGAY4VQKN8RGBN/coverage-tab-scrolled.png)
  • Evidence: Decide dialog on an explainer reading 'Send this back or finish' with 'Send it back' / 'Done reading' buttons, not Approve/Request changes (local file: /home/magicletur/.no-mistakes/evidence/01M23FGA44QHJGAY4VQKN8RGBN/decide-tab.png)

Pipeline

Updates from git push no-mistakes

✅ **intent** - passed

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

⚠️ **Review** - 1 info
  • ⚠️ src/coverage.ts:249 - computeCoverage() sets unresolved: input.graph.unresolved directly from the whole-repo, unscoped CodeGraph (src/cli.ts:958 passes g0 = graph.graphAt(...), built over the entire repository, straight into graph: without ever recomputing resolution against the scoped subset). scopeGraph() only filters files/symbols/edges after the fact; the unresolved counter is a scalar accumulated once during buildGraph() over every reference in the repository and is never split by originating file, so it cannot be scoped by filtering. For thurview explain &lt;subdir&gt; on a large repository, the Coverage tab and the references the graph could not place fact (src/ui/views/coverage.ts:86) will show the repository-wide unresolved-reference count attributed to a small subdirectory scope — a wrong, misleadingly large number that doesn't error. This is the same class of defect the round-1 finding 'coverage-cap-precedes-scope' already identified and explicitly named for both truncated and unresolved ('coverage.truncated/coverage.unresolved reflect the whole repository's cap state rather than the requested scope's'); the fix in cbdcdc6 addressed truncated/capped but left unresolved on the same unscoped value. Fixing it needs buildGraph() (src/graph.ts:239-271) to track unresolved-reference counts per originating file so computeCoverage can sum only the in-scope subset, the same shared boundary that already backs the capped fix.
  • ⚠️ src/cli.ts:1324 - The new explainer branch of thurview graph architecture --review &lt;id&gt; builds g0 = scopeGraph(head, scope) and returns truncated: truncated.head, which is graph.architecture(g0, g0).truncated.head — and scopeGraph() (src/coverage.ts:57-66) spreads ...g onto the scoped copy without recomputing .truncated, so this is the ORIGINAL unscoped head.truncated flag from capFiles() over the whole repository (src/graph.ts:181-184), not whether the requested scope's files were affected by the repo-wide MAX_FILES=4000 cap. In a monorepo with >4000 graph-language files, thurview explain src/small-subsystem followed by graph architecture --review &lt;id&gt; will report truncated: true even when every file in that small scope was fully parsed (none of it was actually dropped by the cap) — the exact kind of false claim that coverage.ts's own capped/truncated computation was specifically fixed in round 1 to avoid for the Coverage tab. This sibling call site, added in this same PR, reintroduces the identical failure mode the prior finding targeted, and the added e2e test only asserts arch[&#39;scope&#39;] (test/e2e.test.ts, added in cbdcdc6) without ever checking truncated, so nothing catches it. The fix belongs at the same shared boundary as the capped computation in computeCoverage — a scope-aware truncation check reused by both call sites — rather than a second bespoke patch here.
  • ⚠️ test/e2e.test.ts:713 - The regression test added in 56893b7 ('tells a file it cannot read apart from one the file cap dropped') only exercises the outsideGraph classification (a non-graph-language file, notes.md) and asserts cov[&#39;files&#39;][&#39;capped&#39;] is 0 and cov[&#39;truncated&#39;] is false throughout. It never drives a scenario where a graph-language file is actually dropped by the repo-wide MAX_FILES=4000 cap, so the capped branch of the classification this commit's own message claims to lock ('the two reasons a file is absent') is not exercised by any test — the isGraphLanguage(f) ? &#39;capped&#39; : &#39;outsideGraph&#39; logic in src/coverage.ts (the exact code the round-1 finding 'coverage-cap-precedes-scope' was about) could be reverted or broken and this test would still pass, since capped stays 0 throughout. Spinning up >4000 real files isn't practical for an e2e run, but computeCoverage() is a pure function directly unit-testable: a test/coverage.test.ts calling it with a synthetic CoverageInput whose allFiles includes a graph-language path absent from graph.files (simulating what capFiles drops) would exercise the actual classification without needing a giant fixture repo, and would give the round-1 fix real regression coverage.

🔧 Fix: Scope unresolved/truncated to explainer scope; test capped branch
1 info still open:

  • ℹ️ src/coverage.ts:264 - graphAt() (src/graph.ts:278-285) reuses a graph.json cached under the review's persistent dir keyed only by commit hash, with no schema check — readJson<CodeGraph> trusts whatever shape is on disk. unresolvedByFile is a brand-new required field; a review whose graph was cached by a pre-upgrade thurview binary (before this change) will load a cached CodeGraph missing unresolvedByFile entirely. The ?. guard here (input.graph.unresolvedByFile?.[f] ?? 0) prevents a crash but silently reports unresolved: 0 for every file instead of the true count, i.e. a wrong value with no error. This is a narrow, pre-existing gap in the cache (it was never schema-versioned for any prior field either, e.g. truncated), not something this diff needs to fully solve, but worth a follow-up: have graphAt() treat a cached record missing unresolvedByFile as stale and rebuild.
✅ **Test** - passed

✅ No issues found.

  • pnpm test && bats scripts/ci/*.bats
  • npx vitest run test/coverage.test.ts test/graph.test.ts test/e2e.test.ts — 38 passed, covering computeCoverage's capped/outsideGraph split, scopeTruncated's scope-aware truncation, buildGraph's unresolvedByFile, and the e2e explainer CLI/HTTP flow (create, reject-no-anchor, reject-no-change, publish, serve coverage payload)
  • Manual end-to-end run: thurview explain src on this repo at commit c9aec905, seeded map.yaml via thurview graph architecture --review &lt;id&gt; (confirmed truncated: false, correctly scope-aware for 36 in-scope files), added one real anchor, thurview publish succeeded with derived coverage sentence '36 files ... 1 anchored ... 33 placed ... 2 not examined'
  • Built the UI bundle (esbuild, transient, removed afterward) and served the review over HTTP; fetched GET /api/reviews/<id> directly and inspected the coverage JSON (states, clusters, unclustered reasons, bothWays links)
  • Loaded the actual rendered page in headless Chromium via chrome-devtools-axi: confirmed tab bar is Explainer/Map/Coverage/Threads/Decide with no Files/Commits tabs, confirmed the Decide dialog reads 'Send this back or finish' with 'Send it back'/'Done reading' buttons (not Approve/Request changes), confirmed the Coverage banner sits above the document in the interface-delta slot
  • eval in-page to read computed styles of .cov-b-explained/.cov-b-placed badges — confirmed each renders with its own background/border (previously placed/uncovered silently fell back to the plain badge look before the round-1 CSS fix)
✅ **Document** - passed

✅ No issues found.

✅ **Lint** - passed

✅ No issues found.

✅ **Push** - passed

✅ No issues found.

thurview's unit was a CHANGE: anchors pin to commits, and the diff, the
commits and the interface delta all assume a range. Explaining a codebase
was a workaround - scaffold with base == head - which leaves every one of
those surfaces rendering a claim about a change that does not exist.

Add an explainer as a second document kind beside a review, sharing the
whole understanding machine (anchors, peeks, map, threads, revisions,
publish/wait) and dropping what describes a range. `thurview explain
[<path>] [--commit <ref>]` pins one commit and an optional path scope;
Files and Commits are absent rather than empty, and a Coverage tab takes
their place.

Coverage is derived at publish rather than claimed in prose: every file
in scope at the pinned commit is explained (an anchor points into it),
placed (a map node owns it) or not examined, and the counts sit above the
document in the slot a review gives the interface delta. An explainer
that anchors nothing is rejected; one with no map is warned, because
"everything unanchored was not examined" is true but rarely meant.

Everything the Coverage tab states is a count or a list of named things
the reader can re-derive with `thurview graph` at the same commit -
cluster sizes and hubs, cross-cluster reference counts, names defined in
more than one cluster, files the graph cannot read. No severities, no
scores, no issues list: it surfaces structure and leaves the conclusion
to the reader.

Also seed Louvain in architecture(), which was unseeded and so clustered
the same commit differently between runs.

Claude-Session: https://claude.ai/code/session_01WW5sEutdtcbSUtgLQKBKpn
The review round's fix separated a file the code graph cannot read from
one the repo-wide file cap dropped before scoping saw it. Only the first
is "outside the languages the graph reads"; reporting the second that way
would state something false about the code, which is the one thing an
explainer must not do. Lock the classification through the CLI and the
HTTP payload, and fix a singular/plural slip in the shared-names copy.

Claude-Session: https://claude.ai/code/session_01WW5sEutdtcbSUtgLQKBKpn
@LeTuR
LeTuR merged commit ff05f94 into main Sep 9, 2026
7 checks passed
@LeTuR
LeTuR deleted the feat/code-explainer branch September 9, 2026 16:44
LeTuR added a commit that referenced this pull request Sep 9, 2026
## Intent

Follow-up defect fix on the code-explainer feature merged in PR #22
(Thurbeen/thurview).

The problem, exactly: PR #22 added `unresolvedByFile` to `CodeGraph` so
that a scoped code explainer can report only its own scope
unresolved-reference count instead of the whole repository one. But
`graphAt` (src/graph.ts) caches a built graph on disk under the review
directory keyed ONLY by commit hash, with no schema check -
`readJson<CodeGraph>` trusts whatever shape is on disk. So any user who
already has a cached graph from a pre-upgrade thurview binary loads a
CodeGraph with `unresolvedByFile` missing, and `computeCoverage` sums
`undefined` entries to 0. The Coverage tab then states "0 references the
graph could not place" - a WRONG NUMBER with no error, for a feature
whose entire thesis is that every fact it states is a count the reader
can re-derive. The reviewing pipeline raised this as an open info
finding on PR #22 and it merged with the finding open; this fixes it.

The fix is deliberately schema-versioned rather than field-specific:
`GRAPH_SCHEMA` is a constant on the module, `CodeGraph` carries
`schema`, and `graphAt` rebuilds when the cached record version does not
match. A field-specific `if (!cached.unresolvedByFile)` guard would rot
the next time a field is added - and the reviewer noted the cache was
never schema-versioned for any prior field either. The `?.` optional
chain in `computeCoverage` is dropped now that the field is guaranteed
present (the `?? 0` stays, because a file with no unresolved references
legitimately has no entry).

Test: test/graph.test.ts drives `graphAt` against a real scratch git
repository with a cache file written in the OLD shape (right commit,
missing fields) and asserts the graph is rebuilt with the true per-file
counts rather than trusting the stale record, plus that the second call
reuses the freshly written cache. This is behaviour through the real
function, not a source grep.

Scope: one commit, no behaviour change to reviews, no new CLI surface.
The full gate is green locally: pnpm typecheck, prettier, rumdl,
shellcheck, and 48 tests.

## What Changed
- Added a `GRAPH_SCHEMA` constant and a `schema` field on `CodeGraph`;
`graphAt` now rebuilds the graph instead of reusing a disk cache whose
`schema` doesn't match the current version, rather than trusting the
cached record's commit hash alone.
- `buildGraph` stamps every graph it produces with `GRAPH_SCHEMA`.
- Removed the now-unneeded `?.` optional chain on `unresolvedByFile`
access in `computeCoverage`, since the field is guaranteed present once
the schema check passes (the `?? 0` fallback stays for files with no
unresolved references).
- Added a `graphAt` test that seeds a cache file in the pre-schema shape
(missing fields) and asserts the graph is rebuilt with correct per-file
unresolved counts, plus that a subsequent call reuses the freshly
written cache; updated the coverage test fixture to include `schema`.

## Risk Assessment

✅ Low: The schema-version guard closes the exact reachable path
described (stale on-disk graph missing unresolvedByFile), all CodeGraph
reads go through the now-guarded graphAt, readJson already fails safe to
null on malformed cache so no new crash path is introduced, and the new
test exercises graphAt end-to-end against a scratch repo with a
pre-upgrade cache shape, correctly failing before the fix and passing
after.

## Testing

The provided baseline (`pnpm test && bats scripts/ci/*.bats`) already
passed; on top of that I ran the two targeted suites touched by the fix
(graph.test.ts, coverage.test.ts) and, to prove this is a real
regression test rather than incidental coverage, temporarily reverted
src/graph.ts and src/coverage.ts to their pre-fix shape and confirmed
the new `graphAt` test fails with exactly the reported symptom (stale
cache trusted, unresolvedByFile silently undefined), then restored the
fix and confirmed all tests pass again. The change matches the stated
intent precisely: GRAPH_SCHEMA constant, schema field on CodeGraph,
graphAt checking schema on cache-hit, and the `?.` guard dropped in
computeCoverage now that the field is guaranteed present. No source or
test files were left modified.

<details>
<summary>Evidence: Regression reproduced pre-fix, passes post-fix
(graphAt cache-schema test)</summary>

```text
Pre-fix (ff05f94 shape): AssertionError: expected undefined to deeply equal { 'src/a.ts': 1 } — test/graph.test.ts:55
Post-fix (a1a69dd, current worktree): Test Files 2 passed (2); Tests 11 passed (11) across test/graph.test.ts and test/coverage.test.ts
```
</details>

## One thing to decide before merging

This PR carries a `cog.toml` line it did not set out to carry. The gate
titled
the PR `fix(graph): ...`, `graph` is not in `cog.toml`'s scope list, so
the
`PR Title` check failed - and the fix round widened the scope list
rather than
retitling. It is left in because it looks right on its own merits
(`src/graph.ts`
is a ~590-line subsystem with its own test file, and both #20 and #21
touched
it), but adding a commit scope is a repo-convention call that belongs to
you,
not to a one-line bug fix. Drop that line and retitle to an existing
scope if
you would rather keep the vocabulary as it is.

## Pipeline

Updates from [git push
no-mistakes](https://github.com/kunchenguid/no-mistakes)

<!-- no-mistakes-pipeline-attestation:v1
{"head_sha":"a1a69dd965fbbab1ec49ed95e195be04e3a586cc","steps":[{"step":"intent","status":"completed"},{"step":"rebase","status":"completed"},{"step":"review","status":"completed"},{"step":"test","status":"completed"},{"step":"document","status":"completed"},{"step":"lint","status":"completed"},{"step":"push","status":"completed"},{"step":"pr","status":"running"},{"step":"ci","status":"pending"}]}
-->

<details>
<summary>✅ **intent** - passed</summary>

✅ No issues found.
</details>

<details>
<summary>✅ **Rebase** - passed</summary>

✅ No issues found.
</details>

<details>
<summary>✅ **Review** - passed</summary>

✅ No issues found.
</details>

<details>
<summary>✅ **Test** - passed</summary>

✅ No issues found.
- `pnpm test && bats scripts/ci/*.bats`
- <code>`npx vitest run test/graph.test.ts test/coverage.test.ts` at
target commit a1a69dd — 11/11 pass</code>
- <code>Regression reproduction: temporarily reverted src/graph.ts and
src/coverage.ts to the pre-fix (ff05f94) shape while keeping the new
test, reran `test/graph.test.ts` — the new `graphAt` test failed with
`expected undefined to deeply equal { &#39;src/a.ts&#39;: 1 }`,
confirming it fails for the exact reported reason (stale cache trusted,
unresolvedByFile undefined)</code>
- <code>Restored fixed sources and reran `npx vitest run
test/graph.test.ts test/coverage.test.ts` — 11/11 pass again, worktree
left clean (`git status` clean, no stray temp files)</code>
</details>

<details>
<summary>✅ **Document** - passed</summary>

✅ No issues found.
</details>

<details>
<summary>✅ **Lint** - passed</summary>

✅ No issues found.
</details>

<details>
<summary>✅ **Push** - passed</summary>

✅ No issues found.
</details>
LeTuR added a commit that referenced this pull request Sep 9, 2026
…tmatter parsing (#24)

## Intent

A user is blocked: `npx skills add Thurbeen/thurview --skill thurview`
refuses to install the skill with 'Skipped .../skills/thurview/SKILL.md
- YAML parse error: Nested mappings are not allowed in compact
mappings'. The cause was already diagnosed before this work started, and
re-deriving it was out of scope: in skills/thurview/SKILL.md's
frontmatter the unquoted `description` scalar contains `Two kinds: ` - a
colon followed by a space, which YAML reads as a mapping key nested
under `description`.

The goal: make the frontmatter load again WITHOUT losing what the
description says. The 'two kinds' wording arrived with the
code-explainer work (#22) and naming both document kinds - the review
and the code explainer - is the whole point of that rewrite. Shortening
the description back to the old one-kind wording would be a regression,
not a fix. So the deliberate choice was to rephrase the colon away (`Two
kinds - a review ... and a code explainer ...`, em-dash) rather than
quote the scalar: quoting leaves a scalar the next editor can break the
same way, and the value already contains double quotes so the quoting
would have to be single. `argument-hint` was already correctly quoted
and was deliberately left untouched.

The test is the actual deliverable, not the one-line fix: nothing in
this repo noticed that a SKILL.md frontmatter had stopped being loadable
YAML, which is the only reason this reached a user.
test/skill-frontmatter.test.ts loads the frontmatter of every SKILL.md
in the repo with the repo's real YAML parser (the `yaml` package the CLI
already uses) and asserts each names the skill; it also asserts the
broken `Two kinds: ` shape is rejected, so the check is proven to go red
on the defect it exists to catch. It was written first and watched fail
with the installer's exact message before the description was fixed. The
description assertion pins that both kinds survive.

Wiring, deliberately inside the existing gate rather than beside it: the
suite is already the repo gate (`pnpm test`, run by CI's 'check' job and
by the prek hooks), but CI's dorny/paths-filter 'code' filter did not
list skills/, so a skill-only edit would have skipped the very job that
now guards it - exactly the hole this defect walked through. Added
'skills/**/SKILL.md' to that filter. No second gate was introduced.

Deliberately NOT widened: a user is blocked right now, so anything else
noticed goes in a report, not this branch. Only one SKILL.md exists in
the repo and it was checked for any other unquoted ': '; there is none.

Verified: pnpm check green (51 tests), rumdl and shellcheck clean, and
the fixed frontmatter also loads under python yaml.safe_load - the same
load the installer's error came from. The full installer path (npx
skills add) clones from GitHub and can only be exercised once this
branch is pushed.

Note on provenance: the broken description came from the code-explainer
work whose own task is still open elsewhere; that is known and no
coordination is needed - the defect is already on main and this fixes
main.

## What Changed

- Rephrased the unquoted `description` scalar in
`skills/thurview/SKILL.md` frontmatter, replacing `Two kinds: ` (a
colon-space that YAML parses as a nested mapping key) with `Two kinds —
` (em-dash), preserving both named document kinds (review and code
explainer) without quoting the scalar.
- Added `test/skill-frontmatter.test.ts`, which parses every `SKILL.md`
frontmatter in the repo with the `yaml` package, asserts each has a
`name` and `description`, asserts the thurview description still names
both document kinds, and asserts the previously-broken `Two kinds: `
shape is rejected by the parser.
- Added `skills/**/SKILL.md` to the `code` path filter in
`.github/workflows/ci.yml` so skill-only edits still trigger the test
job that now guards frontmatter validity.

## Risk Assessment

✅ Low: A minimal, well-scoped fix: the diff only rephrases the offending
colon in SKILL.md's description (preserving the required "two kinds"
content), adds a CI paths-filter entry so skill-only edits run the
guarding suite, and adds a regression test that parses every SKILL.md's
frontmatter with the same `yaml` package the installer uses (a
legitimate contract test since no in-repo code path actually parses
SKILL.md frontmatter to call instead) and asserts the broken shape is
rejected.

## Testing

Baseline `pnpm test && bats scripts/ci/*.bats` already passed; targeted
rerun of the new test/skill-frontmatter.test.ts (4/4) confirmed the
fixed SKILL.md parses and keeps both document kinds, and reverting just
that file reproduced the installer's exact 'Nested mappings are not
allowed in compact mappings' error, proving the test is a genuine
regression guard rather than a tautology. Cross-checked the fixed
description under python's yaml.safe_load (the loader family behind the
installer's error) and confirmed the CI paths-filter now covers
skills/**/SKILL.md. No findings.

<details>
<summary>Evidence: test/skill-frontmatter.test.ts — pass on fixed
content, fail (installer error) on reverted content</summary>

```text
$ npx vitest run test/skill-frontmatter.test.ts
Test Files 1 passed (1)
Tests 4 passed (4)

Regression check (SKILL.md reverted to pre-fix content, test rerun):
YAMLParseError: Nested mappings are not allowed in compact mappings at line 2, column 14:
description: Author and publish a thurview document - a guided, evidence-anchor...
2 failed | 2 passed (4) — matches the installer's exact reported error.
```
</details>
<details>
<summary>Evidence: python yaml.safe_load of the fixed frontmatter&#39;s
description field (same loader the installer&#39;s error came from) —
both document kinds present, colon removed via em-dash</summary>

```text
Author and publish a thurview document - a guided, evidence-anchored explanation the reader opens in the browser, annotates, asks questions about, and approves or sends back. Two kinds — a review of a branch, pull request or commit range, and a code explainer of a whole codebase or one subsystem at a pinned commit. Use when the user asks to review a branch or PR, to explain or walk through a change, "review my branch against main", to explain how a codebase or subsystem works or where its design problems might be, or invokes /thurview. Not for a pass/fail bug hunt.
```
</details>

## Pipeline

Updates from [git push
no-mistakes](https://github.com/kunchenguid/no-mistakes)

<!-- no-mistakes-pipeline-attestation:v1
{"head_sha":"fd8d8e20e67b81e53339df7c1fac4056a17ff637","steps":[{"step":"intent","status":"completed"},{"step":"rebase","status":"completed"},{"step":"review","status":"completed"},{"step":"test","status":"completed"},{"step":"document","status":"completed"},{"step":"lint","status":"completed"},{"step":"push","status":"completed"},{"step":"pr","status":"running"},{"step":"ci","status":"pending"}]}
-->

<details>
<summary>✅ **intent** - passed</summary>

✅ No issues found.
</details>

<details>
<summary>✅ **Rebase** - passed</summary>

✅ No issues found.
</details>

<details>
<summary>✅ **Review** - passed</summary>

✅ No issues found.
</details>

<details>
<summary>✅ **Test** - passed</summary>

✅ No issues found.
- `pnpm test && bats scripts/ci/*.bats`
- <code>`npx vitest run test/skill-frontmatter.test.ts` — 4/4 pass on
the fixed frontmatter</code>
- `Reverted skills/thurview/SKILL.md to the pre-fix (main) content and
reran the same test file: it failed with the installer's exact error,
'Nested mappings are not allowed in compact mappings', confirming the
test is a real regression guard — then restored the fixed file (worktree
left clean)`
- <code>Manually parsed the fixed frontmatter&#39;s description with
`python3 -c &#34;import yaml; ...&#34;` (yaml.safe_load), the same
loader family the installer&#39;s reported error came from, and
confirmed both &#39;review of a branch&#39; and &#39;code explainer&#39;
wording survive</code>
- `Confirmed .github/workflows/ci.yml's 'code' paths-filter now lists
'skills/**/SKILL.md' (line 38), closing the gap that let a skill-only
edit skip the guarding CI job`
</details>

<details>
<summary>✅ **Document** - passed</summary>

✅ No issues found.
</details>

<details>
<summary>✅ **Lint** - passed</summary>

✅ No issues found.
</details>

<details>
<summary>✅ **Push** - passed</summary>

✅ No issues found.
</details>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant