Conversation
|
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. What that means for this PR, concretely:
Things that will genuinely speed it up whenever review does happen:
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. |
3f48c01 to
d72a5c7
Compare
|
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. |
|
Maintainer decision: None of the nine existing types means "mentions". 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 acceptIt 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 Carrying Splitting the shell-file companion out was the right call for reviewability. Two things to doStrip the Please rebase — this is 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 |
d72a5c7 to
844a36b
Compare
|
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
left a comment
There was a problem hiding this comment.
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/.mdxare read. Paths found inside markdown are never opened or stat'ed — resolution is a pure string operation followed by a hash lookup on thefile_path → File nodemap.../../.., absolute paths,file://, drive letters and UNC paths can at worst form a key that misses, anddoclink_normalizerefuses 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.mdcannot 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_preseedloads 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.cbmhunk (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.
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>
69a9c90 to
6ae6cd4
Compare
|
Rebased onto main (e783f73), one commit 6ae6cd4, force-pushed. Addressing each item:
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>
|
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. |
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
git commit -s) — required, CI rejectsunsigned commits (DCO, see CONTRIBUTING.md)
make -f Makefile.cbm test)make -f Makefile.cbm lint-ci)