Skip to content

feat(pipeline): link markdown file references into the graph - #1832

Open
cdeust wants to merge 3 commits into
DeusData:mainfrom
cdeust:feat/doclinks-markdown
Open

cdeust wants to merge 3 commits into
DeusData:mainfrom
cdeust:feat/doclinks-markdown

Conversation

@cdeust

@cdeust cdeust commented Aug 25, 2026

Copy link
Copy Markdown

Markdown docs reference other repo files constantly (a coding-standards doc links the modules it governs, a README points at entry points) but none of that surfaced as edges, so fan-in queries were blind to documentation hubs. In an A/B retrieval benchmark I ran in August on a docs-heavy repo, the graph's top fan-in answer was off by roughly 17x: the most-referenced file was a standards doc with 173 references and zero inbound edges.

New pre-dump pass, pass_doclinks.c, modeled on pass_configlink.c: three strategies emit REFERENCES_FILE edges between existing File nodes only (unresolvable targets are dropped, the pass never invents nodes): inline links, backtick paths, bare path mentions. Targets resolve against the referencing file's directory and the repo root; repeated references collapse into one edge carrying strategy, confidence and count. Registered in the pre-dump sequence after configlink and in the incremental post-passes; REFERENCES_FILE added to the skill's edge-type list and the structural/language contract tests.

Tests mirror test_configlink.c (real files in a tmpdir, File nodes in a gbuf, run the pass, assert edges): inline link, backtick, bare mention, http/anchor ignored, anchor-suffixed file link, dedupe with count, relative-vs-root resolution, unresolvable-target guard, NULL repo_path skip.

A companion change adds the same linking for shell files (source lines and script invocations); split out to keep each change reviewable.

Validation: scripts/build.sh clean; focused serial runner (doclinks, configlink, pipeline, edge_structural, lang_contract) 340 passed under ASan/UBSan; cppcheck clean on the new files with the repo's flags.

What does this PR do?

Checklist

  • Every commit is signed off (git commit -s) — required, CI rejects
    unsigned commits (DCO, see CONTRIBUTING.md)
  • Tests pass locally (make -f Makefile.cbm test)
  • Lint passes (make -f Makefile.cbm lint-ci)
  • New behavior is covered by a test (reproduce-first for bug fixes)

@cdeust
cdeust requested a review from DeusData as a code owner August 25, 2026 11:38
@github-actions

Copy link
Copy Markdown

Thanks for opening this — it has been seen, and it is queued.

This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence.

Current review status: working through a backlog. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

  • It will not be closed for inactivity. No stale bot touches pull requests here.
  • It may still sit a while before a human reads it. That is on us, not on you.
  • Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

  • Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
  • Get CI green, or say which failures you believe are pre-existing.
  • Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
  • Every commit needs a sign-off (git commit -s) — CI enforces DCO.

If this fixes a bug, a reproduction we can run is worth more than a description of the symptom.

Thanks for contributing, and sorry in advance for the wait.

@cdeust
cdeust force-pushed the feat/doclinks-markdown branch 3 times, most recently from 3f48c01 to d72a5c7 Compare August 25, 2026 12:27
@DeusData DeusData added enhancement New feature or request parsing/quality Graph extraction bugs, false positives, missing edges priority/normal Standard review queue; useful PR with ordinary maintainer urgency. labels Sep 1, 2026
@DeusData

DeusData commented Sep 1, 2026

Copy link
Copy Markdown
Owner

Thank you for the concrete documentation-link benchmark, the dedicated pass, and the negative and deduplication controls. This introduces new graph edge semantics and several source-text heuristics, so we need more time to review precision, incremental parity, and security boundaries carefully. The contribution queue is quite full, but we will return with grounded feedback as soon as possible.

@DeusData

DeusData commented Sep 1, 2026

Copy link
Copy Markdown
Owner

Maintainer decision: REFERENCES_FILE is accepted. A new edge type has to earn itself here — the default is to map a new construct onto an existing one — so it is worth saying what earned it.

None of the nine existing types means "mentions". DEPENDS_ON and CONFIGURES are causal claims, IMPORTS is language-level, USAGE is symbol-level. Forcing a documentation reference onto any of them would not just be imprecise — it would make that type mean less for everyone already querying it, and a fan-in query would silently mix two unlike relations. This is the case where a new type is cheaper than the alternative.

And the defect is measured rather than argued. A standards document with 173 references and zero inbound edges, and a top-fan-in answer off by roughly 17x, is a concrete way for the graph to be confidently wrong about which files matter. That is the kind of blindness that is invisible until someone benchmarks it.

What made this easy to accept

It emits edges between existing File nodes only. Dropping unresolvable targets rather than inventing nodes is the single most important property here — a doc-linking pass that minted File nodes for every path-shaped string would poison the graph rather than enrich it. Saying so explicitly in the description is what let me stop worrying about it early.

Modelling it on pass_configlink.c rather than inventing a pass shape means the next reader meets one pattern twice.

Carrying strategy alongside confidence and count is what de-risks the part I would otherwise have pushed back on. Bare path mentions in prose are the strategy most likely to produce a false edge, and because the strategy is on the edge, a consumer can filter to explicit links only without us having to decide that question up front. Keep that field — it is doing more work than it looks.

Splitting the shell-file companion out was the right call for reviewability.

Two things to do

Strip the Claude-Session: URL from the commit message. Attribution of tool use is welcome here and the Co-Authored-By style trailer is fine; the session URL is the part we do not carry in repository history. An amend and a force-push will do it.

Please rebase — this is DIRTY. main moved three times yesterday: broken by a duplicate-symbol merge, repaired by #1993, then #1703 landed.

Your checklist is entirely unticked, but your validation paragraph covers all of it — build clean, 340 passed under ASan/UBSan across the focused suites, cppcheck clean on the new files. Worth ticking so a reviewer does not have to reconcile the two.

Clearance is REVIEW(1) on Makefile.cbm — the established shape here for registering a new test file. It needs a maintainer marker, not a change from you.

@cdeust
cdeust force-pushed the feat/doclinks-markdown branch from d72a5c7 to 844a36b Compare September 2, 2026 01:44
@cdeust

cdeust commented Sep 2, 2026

Copy link
Copy Markdown
Author

Rebased on main (5fbab7b), one commit 844a36b, force-pushed. Conflicts were only in src/pipeline/pipeline.c and Makefile.cbm: kept main's version (sizeof-derived pass count, new ensemble_routing/importance passes) and inserted the doclinks pass / test_doclinks.c right after configlink. All other hunks are byte-identical to the previous commit d72a5c7 (checked by interdiff). The Claude-Session trailer is removed and the checklist is ticked. Locally under ASan/UBSan: doclinks, configlink, pipeline, edge_structural and lang_contract suites = 355 passed. clang-format reports no violation on the diff.

@DeusData ready for review on your side.

@DeusData DeusData left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eighteen days between "accepted" and this review is too long, and I am sorry for it — you did both things I asked within a day. Here is the detailed read. Short version: the design is right and safe, it still merges cleanly after 379 commits, and what stands between it and main is a handful of small, local changes — most of them caused by rules that landed after you opened the PR.

What is right, specifically

  • Safe by construction. There is exactly one file open and it goes through cbm_fopen; only File nodes ending in .md/.mdx are read. Paths found inside markdown are never opened or stat'ed — resolution is a pure string operation followed by a hash lookup on the file_path → File node map. ../../.., absolute paths, file://, drive letters and UNC paths can at worst form a key that misses, and doclink_normalize refuses to climb above the root. Over-long tokens, targets and paths are dropped, not truncated (with one exception, below).
  • No quadratic work. The File index is built once per pass, not once per markdown file — which is precisely the mistake this project keeps meeting — and nothing scans all nodes.
  • Disciplined about graph quality. Exact-path matching only: no basename or suffix guessing, so a prose README.md cannot bind to an arbitrary file; no invented nodes; self-references dropped; a consumed [text](target) span is blanked so one mention is not counted three times; strategy and confidence are on the edge so a consumer can filter to explicit links.
  • Incremental parity holds on today's closure route, which did not exist when you wrote this: cbm_delta_preseed loads every File node as a proxy, so your path map is built from the whole repository rather than from the changed files. I went looking for that specific bug because we found it in another PR today; it is not here.
  • The tests assert strategy and confidence, so each one binds; the suite is wired in both the Makefile and test_main.c; no sleeps, no wall clocks.

Changes needed

1. lint-ci will go red after a rebase: raw malloc/free in a new file (pass_doclinks.c:88, :447). scripts/lint-memory-core.py is a ratchet that landed after your PR: a file not in the baseline is allowed zero raw allocator sites. The function you modelled this on is grandfathered; a new file is not. Use cbm_alloc(CBM_MEM_CLASS_OTHER, …) / cbm_free from src/foundation/mem_core.h — today's pass_configlink.c shows the shape. This also puts a markdown buffer under the memory budget's accounting.

2. The 4 KiB line buffer can create a false edge (:400-409). A line longer than CBM_SZ_4K is cut at 4095 bytes and the rest is skipped. Paragraphs written on one line are common in markdown, so: if src/foo.cpp straddles byte 4095 it becomes src/foo.c, and if that file exists you emit a wrong edge (same for .json.js, .tsx.ts). Everything after the cut is lost, and on a final line without \n the remainder is re-scanned as a new line, splitting one token in two. source is already your private mutable copy — terminate lines in place (\n\0) and pass pointers: no copy, no cap, 4 KiB less stack.

3. DOCLINK_MAX_REFS = 256 decides graph content (:53, :210). A generated SUMMARY.md or file index with more than 256 distinct targets silently loses everything after the 256th, in document order. This project has an explicit ruling that no work cap may decide what is in the graph. Collect matches in a growable vector, sort by target id, collapse — O(M log M), deterministic, and it removes the linear dedupe scan along with the cap.

4. A rooted link still tries the directory first (doclink_resolve, :166-185). The comment says /docs/x.md is repo-root-relative, but the leading / is only stripped and the directory join still runs first. With docs/guide.md containing [home](/README.md) and both docs/README.md and README.md present, the edge goes to docs/README.md at 0.95. Remember that the reference was rooted and skip the join.

5. An explicit ./x falls back to the repo root (same function). [cfg](./config.md) in docs/a.md, with no docs/config.md but a root config.md, produces an edge to the root file at 0.95 — for what is really a broken link. No root fallback when the reference began with ./. Related and optional: a root-fallback hit on an inline link arguably should not carry the same 0.95 as a directory-relative one; recording "resolved":"dir"|"root" in the properties would let consumers tell them apart.

6. Silent skips and no cancellation check. Unreadable or oversized markdown files are dropped without a log line (:442-445) — the convention here is that a skip is reported, never silent. And the per-file loop (:432) never calls cbm_pipeline_check_cancel, so on a docs-heavy repository cancel latency is the whole pass.

7. Tests. One structural gap: every test calls the pass directly, so deleting the {predump_doclinks, …} registration line leaves all nine green. Please add one pipeline-level test that indexes a tiny repo and finds a REFERENCES_FILE edge. And negative controls for the cases above, since each is a way to get a false edge: the rooted /x and the ./x precedence cases, a line longer than 4 KiB with a straddling token, more than 256 targets, .. escaping the root, a bare http://host/path/file.c in prose, backtick main.c in docs/ not binding to src/main.c, and a CRLF file.

Smaller: a few comments still mention the shell strategy you split out (:461, pipeline_internal.h, the test_main.c comment, the test_doclinks.c header); README.md's edge-type list (~line 704) and the edge table in docs/index.html should gain the new type. Fenced code blocks are not tracked, so gcc src/main.c inside a fence links at 0.70 — defensible, but worth either a test that pins it or its own strategy label so consumers can filter it.

Two things that are ours, not yours

  • Closure inflation. The incremental planner treats every edge type except the containment ones as a dependency, so with this type every markdown file that references a changed file joins the re-parse closure and counts against its budget — docs-heavy repositories may see more full rebuilds. That same behaviour is also what keeps one parity corner correct, so excluding the type is not free. I would rather keep it and measure than guess; I will do that measurement on a docs-heavy corpus once this is rebased, alongside the before/after edge counts and a hand-sampled precision figure per strategy. Nothing for you to do.
  • Build clearance. Your Makefile.cbm hunk (registering the test file) needs a maintainer marker before I can build the branch on our machines — the established shape, needing nothing from you. That is why everything above comes from reading rather than running, and I have said so where it matters.

main currently carries two reds that are not yours (a linter ratchet and a worker-policy script, both fixed by #2257, about to land) — rebase after that and your CI will be showing only your own results. The Co-Authored-By trailer is fine as it is.

Thank you for the care in this one. It is a good pass, and the list above is what careful work looks like after the ground moved under it, not a judgement of it.

cdeust and others added 2 commits September 22, 2026 08:06
Markdown docs reference other repo files constantly (a coding-standards
doc links the modules it governs, a README points at entry points) but
none of that surfaced as edges, so fan-in queries were blind to
documentation hubs. In an A/B retrieval benchmark I ran in August on a
docs-heavy repo, the graph's top fan-in answer was off by roughly 17x:
the most-referenced file was a standards doc with 173 references and
zero inbound edges.

New pre-dump pass, pass_doclinks.c, modeled on pass_configlink.c: three
strategies emit REFERENCES_FILE edges between existing File nodes only
(unresolvable targets are dropped, the pass never invents nodes):
inline links, backtick paths, bare path mentions. Targets resolve
against the referencing file's directory and the repo root; repeated
references collapse into one edge carrying strategy, confidence and
count. Registered in the pre-dump sequence after configlink and in the
incremental post-passes; REFERENCES_FILE added to the skill's edge-type
list and the structural/language contract tests.

Tests mirror test_configlink.c (real files in a tmpdir, File nodes in a
gbuf, run the pass, assert edges): inline link, backtick, bare mention,
http/anchor ignored, anchor-suffixed file link, dedupe with count,
relative-vs-root resolution, unresolvable-target guard, NULL repo_path
skip.

A companion change adds the same linking for shell files (source lines
and script invocations); split out to keep each change reviewable.

Validation: scripts/build.sh clean; focused serial runner (doclinks,
configlink, pipeline, edge_structural, lang_contract) 340 passed under
ASan/UBSan; cppcheck clean on the new files with the repo's flags.

Signed-off-by: Clément Deust <clement.deust@gmail.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replace raw malloc/free with cbm_alloc/cbm_free (CBM_MEM_CLASS_EXTRACT)
and a growable CBM_DYN_ARRAY for matches. Remove the 4KiB line-copy
cap; lines are split in place instead of copied. Remove the
DOCLINK_MAX_REFS(256) cap; the ref vector now grows, is sorted by
target id, then collapsed. Track reference kind (bare, relative,
rooted) so /x never attempts a directory join and ./x never falls
back to root. Report per-file skips via cbm_pipeline_add_file_error
and cbm_log_warn, and check cbm_pipeline_check_cancel in the per-file
loop.

Add negative-control tests for all of the above, plus CRLF handling,
fenced-code-block pinning, bare URLs, backtick-basename exactness,
and ".." escaping.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Clément Deust <clement.deust@gmail.com>
@cdeust
cdeust force-pushed the feat/doclinks-markdown branch from 69a9c90 to 6ae6cd4 Compare September 22, 2026 06:51
@cdeust

cdeust commented Sep 22, 2026

Copy link
Copy Markdown
Author

Rebased onto main (e783f73), one commit 6ae6cd4, force-pushed. Addressing each item:

  1. Raw malloc/free replaced with cbm_alloc/cbm_free from mem_core.h at both sites (:114, :523 in the new diff). Used CBM_MEM_CLASS_EXTRACT rather than CBM_MEM_CLASS_OTHER since this pass extracts references from file content, matching the naming used elsewhere for extraction buffers. scripts/lint-memory-core.py passes: 3805 raw sites across 85 files, none grew.

  2. The 4 KiB line-copy cap is gone. source is the private mutable copy, so lines are now terminated in place (\n to \0, CRLF stripped) and scanned by pointer instead of being copied into a fixed buffer. No cap, no truncation, no stack buffer.

  3. DOCLINK_MAX_REFS(256) is gone. Matches are collected into a growable CBM_DYN_ARRAY, sorted by target id, then collapsed in one pass, so ordering no longer decides what survives past the 256th match.

  4. doclink_resolve now tracks whether a reference was bare, relative, or rooted. A rooted /x reference skips the directory join entirely and resolves against the root only.

  5. An explicit ./x reference no longer falls back to the root when the directory-relative path misses; only bare and relative-without-./ references get the root fallback.

  6. Unreadable and oversized files are now reported through cbm_pipeline_add_file_error plus cbm_log_warn instead of being dropped silently, and the per-file loop calls cbm_pipeline_check_cancel each iteration.

  7. Added a pipeline-level test (pipeline_doclinks_edge_lands_in_store) that indexes a small repo and asserts the REFERENCES_FILE edge lands in the store, plus direct-pass negative controls for: rooted /x vs ./x precedence, a line past the old 4 KiB cap with a straddling token, more than 256 distinct targets, ".." escaping the root, a bare URL in prose, backtick main.c not fuzzy-matching src/main.c, and CRLF line endings. Fenced code blocks are pinned by a dedicated test (doclinks_fenced_code_block_bare_mention_pinned) rather than given a separate strategy label, since the 0.70 confidence already lets consumers filter it.

Smaller items: the stale shell-strategy comments in pipeline_internal.h:668 and test_main.c are fixed. Checked README.md and docs/index.html: README.md already lists REFERENCES_FILE, and docs/index.html has no edge-type table to update.

Validation: build clean (make -f Makefile.cbm cbm), and the full ASan/UBSan suite (make -f Makefile.cbm build/c/test-runner) passed end to end: 8182 passed, 10 skipped, 0 failed. clang-format-20 reports no drift on any of the 5 changed files.

@DeusData ready for another look.

The full Edge Types list near the graph schema section still missed
the new REFERENCES_FILE type added by the doclinks pass. The
"selected" list earlier in the file is explicitly a subset and is
left as is.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Clément Deust <clement.deust@gmail.com>
@cdeust

cdeust commented Sep 22, 2026

Copy link
Copy Markdown
Author

Correction to my previous comment: README.md did not already list REFERENCES_FILE. The "selected" list near the top is intentionally a subset, but the full Edge Types table near the graph schema section (line 705) was missing it. Fixed in 9a9534c, pushed. docs/index.html genuinely has no edge-type table, that part of the previous comment stands.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request parsing/quality Graph extraction bugs, false positives, missing edges priority/normal Standard review queue; useful PR with ordinary maintainer urgency.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants