docs: what gitnexus impact does not see, and why a zero is not an answer - #814
Merged
Merged
Conversation
…wer (#810) `impact` reports 0 callers and risk LOW for most Go symbols here. releasePort has 40 callers, setHealth 31, downstreamHub 13, allocPort 12 -- all four come back 0/LOW. It is not reporting a small blast radius; it is reporting nothing, in the shape of a small one, and CLAUDE.md makes running it mandatory before every edit. THE CAUSE, which #810 lacked when filed: a Go method body contributes no call edges. Pinned to single nodes by filePath, New in engine.go carries 19 outgoing edges and Open in clips.go carries 7, while startPreviewLocked, reconcileClips, allocPort, pump, setHealth and Start carry none between them. Most of this codebase is methods, so most of its call graph is absent. hashStrings returning 8 of 16 is the same fact from the other side -- the 8 found are calls from top-level functions. releasePort's one recorded caller is a test function; its forty callers inside Engine methods are not edges. PINNING IS WHAT MADE THE MEASUREMENT MEAN ANYTHING, and the note says so. An unpinned query reported `Start` with 24 outgoing edges and appeared to refute the whole finding. `Start` names many methods across many packages and the query was aggregating them; adding filePath returned 0. I came close to publishing that as a refutation. IT TOOK THREE TRIES TO FIND A DURABLE HOME, which is worth recording because the next person will look in the same two places first. CLAUDE.md and AGENTS.md are generated end to end -- every line is inside the gitnexus block and the next analyze overwrites it. .claude/skills/ is gitignored, so a note there reaches nobody else; I wrote one there before noticing. docs/ is the only tracked, shared surface, and check-build.mjs then requires the file to be declared published or withheld, which is why the manifest changes too. No CI guard, and that is a limit rather than an omission: .gitnexus/ is gitignored and no workflow runs gitnexus, so a test measuring the tool's accuracy would have no index to measure. Claude-Session: https://claude.ai/code/session_01A8N3W5ct9SZtHK9sCDD9cL
There was a problem hiding this comment.
🟡 Changes recommended
Correct the documentation inaccuracies and reconcile the reported caller count before approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds contributor documentation explaining GitNexus’s incomplete Go call graph and withholds it from the public website.
Changes:
- Adds
docs/GITNEXUS-IMPACT.md. - Registers the document as withheld in the web manifest.
File summaries
| File | Summary |
|---|---|
web/src/data/docs.mjs |
Marks the new document as withheld. |
docs/GITNEXUS-IMPACT.md |
Documents GitNexus limitations and interim guidance; requires factual corrections. |
Review details
Suppressed comments (3)
docs/GITNEXUS-IMPACT.md:18
- This is a text-search measurement, not a real-caller count: the search includes non-call mentions in comments. For example,
internal/engine/engine.gohas comment occurrences ofdownstreamHub()alongside the actual calls, so the13row is overstated as “real callers.” Please label this as text references (or switch to a call-aware count) so readers do not treat comment mentions as blast-radius evidence.
Callers counted by text search in `internal/`, excluding `_test.go` and the
definition itself.
docs/GITNEXUS-IMPACT.md:65
CLAUDE.mddoes not contain the quoted “never substitute grep for graph analysis” rule; it says to usegitnexus_queryinstead of grepping when exploring unfamiliar code (CLAUDE.md:13). Quoting a stronger rule makes this note factually inaccurate, so please paraphrase the generated guidance accurately.
- `CLAUDE.md` says "never substitute grep for graph analysis". For Go symbols
here, grep is currently the more accurate of the two. Use both and prefer
the larger answer.
docs/GITNEXUS-IMPACT.md:25
- The
downstreamHubrow reports 13 callers, but #810's measurement—described with the same text-search scope here—reports 10. Please reconcile the count and document any changed matching rule/index, or restore 10; otherwise this durable note contradicts its source measurement.
| `downstreamHub` | 13 | **0** | LOW |
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+61
to
+65
| - Treat `0` and `LOW` on a Go symbol exactly as `CLAUDE.md` says to treat | ||
| `UNKNOWN`: **unresolved**. Confirm with a text search. | ||
| - `CLAUDE.md` says "never substitute grep for graph analysis". For Go symbols | ||
| here, grep is currently the more accurate of the two. Use both and prefer | ||
| the larger answer. |
|
rainmanjam
added a commit
that referenced
this pull request
Sep 16, 2026
* ci: two checks that could only ever report, and now block `workflow lint` has run on every pull request since it was added -- no `needs: changes`, no `if:`, nothing path-filtered -- and could not block one. The job whose whole purpose is to catch a broken workflow file was advisory about workflow files, which is the one place that is worth least: a workflow that does not parse takes its own checks down with it, and the checks it takes down are the ones protection was relying on. Listing it was the entire fix. `build web/ and publish to Cloudflare Pages` needed a workflow change first, and that is why it was not required from the start. pages.yml carried a `paths:` filter on `pull_request:`, and a workflow-level filter skips the WORKFLOW, not the job -- so on a pull request touching neither web/ nor docs/ the check did not fail, it did not exist. Requiring an absent name is the 2026-09-02 outage that required_checks_test.go was written after: #650, #656 and #659 unmergeable at once, #659 forty of forty green, nothing red anywhere. So the filter moved inside, to a `changes` job, with a shim job carrying the same name for the not-applicable case -- the same partition ci.yml already uses for `container: <suite>` and container-not-applicable. The context is now always answered rather than sometimes absent. What that gates: `npm run build` in web/ is `astro check && astro build && scripts/check-build.mjs`, and check-build.mjs is the only thing in the repository asserting about the shipped CSS, the resolvability of every internal link, and that every docs/*.md is declared published or withheld. All of it was red-but-mergeable until now, #814 included -- that PR added a docs page and a manifest row, and had the row been forgotten it would have merged green and broken the deploy on main. Claude-Session: https://claude.ai/code/session_01A8N3W5ct9SZtHK9sCDD9cL * ci: name the site gate for what it gates, not for `changes` The first version named it `changes`, which made docsgate_test.go apply its rule to pages.yml: any job whose `needs` names `changes` must carry `needs.changes.outputs.code` on every one of its steps. Both platforms failed. That rule is flat on purpose and the right answer is not to weaken it. It is also a rule this file cannot follow: pagesdeploy_test.go requires the Build step to carry no `if:` at all, which is the exact opposite of "gate every step", and for a good reason -- `npm run build` is the only CI web/ has, so it must run whenever the job runs. The two rules are not in conflict; they are about different gates. docsgate is about the documentation gate shared by ci.yml's five required jobs, whose hazard is a skipped MATRIX job never expanding its per-leg contexts. This gate decides whether the site needs rebuilding, applies to a job with no matrix, and has its complement answered by a same-named sibling -- which is the container-suites -> container / container-not-applicable shape already in ci.yml. Naming it `changes` claimed membership of the wrong rule. Renamed to `site-scope`, with the distinction written down beside it. Found by running the package instead of the three test names I had guessed at with -run; the guard I tripped is not called anything containing "docsgate". Claude-Session: https://claude.ai/code/session_01A8N3W5ct9SZtHK9sCDD9cL
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses #810 as far as this repository can — the remaining work is upstream in gitnexus.
The root cause, which #810 lacked when filed
A Go method body contributes no call edges to the graph. Pinned to single nodes by
filePath:New(engine.go)Open(clips.go)startPreviewLocked,reconcileClips,allocPort(engine.go)pump,setHealth(youtube.go)Start(srtserver.go)Most of this codebase is methods, so most of its call graph is absent. That explains every row of the original measurement —
releasePort40 callers → 0,setHealth31 → 0 — andhashStringsreturning 8 of 16 is the same fact from the other side, the 8 being calls from top-level functions.releasePort's only recorded caller is a test function. Its forty callers insideEnginemethods are not edges.It also explains the relationship drop from #794: those edges are genuinely missing.
A near-miss worth recording
An unpinned
MATCH (a) WHERE a.name = 'Start'reported 24 outgoing edges and appeared to refute the whole finding.Startnames many methods across many packages and the query was aggregating them; addingfilePathreturned 0.Pin cypher queries by file, or the numbers lie. I came close to posting that 24 as a correction to my own issue.
Three tries to find a durable home
Worth knowing, because the next person will look in the same two places first:
CLAUDE.md,AGENTS.mdanalyzeoverwrites every line.claude/skills/docs/check-build.mjsthen requires the file to be declared published or withheld, which is why the manifest changes too. It's registered as withheld — it's about the tooling this repo is developed with, not about running polyemesis.No CI guard
.gitnexus/is gitignored and no workflow runs gitnexus, so a test measuring the tool's accuracy would have no index to measure. A limit, not an omission — I'd rather say that than add a test that asserts something adjacent and looks like coverage.https://claude.ai/code/session_01A8N3W5ct9SZtHK9sCDD9cL