Conversation
github-actions
Bot
added
the
maintainer:e2e:ok
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds documentation link and placement guards, integrates them into local and CI linting, adds behavioral and repository-invariant tests, and revises documentation for telemetry, uninstalling, tool parsing, migrations, links, and internal-content placement. ChangesDocumentation validation
Validation coverage
Documentation updates
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
LLxprt PR Review – PR #2686Issue AlignmentResolves the mechanical half of #2654. The PR fixes all 10 broken repo-relative links, relocates six internal docs from Side Effects
Code QualityNew guard scripts ( Tests and CoverageCoverage impact: increase. Five new test suites ( VerdictReady. The implementation is tightly aligned with the issue, the new CI guards are mechanically enforceable, and the test suite is behavioral and comprehensive. No remediation needed. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…guards (#2654) The documentation audit in #2654 found user pages asserting behavior the code does not implement, internal engineering records sitting in public navigation, and broken repo-relative links with nothing to catch them. Verifying the audit's own claims against source showed some were understated: it reported 3 broken links where there were 15, and its central telemetry claim was backwards. Correct the false documentation. Telemetry is not "commented out requiring source changes" -- initializeTelemetry is called unconditionally and starts a real SDK when enabled, exporting to file or console only, with no network exporter in the code path. Every documented event and metric name was wrong (llxprt_cli.* vs the actual llxprt_code.*), the disable instruction was wrong (persisted settings survive omitting --telemetry; --no-telemetry is the real override), and tracing was overstated. Reconcile configuration.md and enterprise.md, which still promised OTLP export the SDK cannot perform. docs/tool-parsing.md documented a /toolformat value the command rejects and presented two settings with no production consumer as working controls. Uninstall.md named the wrong product and led with deleting the entire npx cache. Relocate six internal documents to dev-docs/ with inbound links updated, and delete docs/cli/keyboard-shortcuts.md, a stale orphan no generator targets and nothing references. Add two guards so this cannot silently regress. lint:doc-links validates every repo-relative link and heading anchor across docs/, dev-docs/ and root-level Markdown; lint:doc-placement rejects internal-only directories and plan bookkeeping markers in docs/. Both use the marked lexer rather than hand-rolled parsing, so indented code, fence delimiters, titled and parenthesized destinations, and multi-byte anchors are handled per CommonMark, and both fail fast on an unreadable root instead of passing vacuously. The audit's remaining prose rewrites of large pages are tracked in #2685; this does not close #2654.
Contributor
OpenCodeReview — PR #2686
|
The vscode-ide-companion 'prepare' script regenerates NOTICES.txt on every install. The committed copy recorded ip-address's legacy git:// repository URL, while a fresh dependency resolution emits the normalized https:// form, so the regenerated file no longer matched what was checked in. This only surfaced now because touching package.json invalidates the CI install cache, which re-runs 'prepare'. PRs that leave package.json alone reuse the cache and never regenerate the file, hiding the drift.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
scripts/check-doc-links.ts (1)
192-200: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the dead
entryUrl === import.meta.urlcheck.fileURLToPath(...)returns a path, so that comparison can never pass; only theendsWith('check-doc-links.ts')branch gatesmain(). Mirrorcheck-doc-placement.tsor switch toimport.meta.main.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/check-doc-links.ts` around lines 192 - 200, Remove the dead entryUrl === import.meta.url comparison from the main execution guard in check-doc-links.ts, including any now-unused entryUrl conversion. Preserve the existing check-doc-links.ts filename gate, or use the established import.meta.main pattern consistent with check-doc-placement.ts.scripts/tests/doc-links-guard.test.ts (1)
401-418: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard symlink test against unprivileged Windows.
symlinkSync(..., 'dir')can throwEPERMon unprivileged Windows. Based on learnings, symlink-based fixtures elsewhere inscripts/testsare guarded withdescribe.skipIf(process.platform === 'win32'); this block should follow the same pattern for local-dev consistency.🔧 Proposed fix
- describe('symlink policy', () => { + describe.skipIf(process.platform === 'win32')('symlink policy', () => {Based on learnings (acoliver, PR 2260,
scripts/tests/verify-bun-workspace-links.test.ts:93-96): "avoid running them on unprivileged Windows because they commonly throwEPERM... Guard these tests withdescribe.skipIf(process.platform === 'win32')".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/tests/doc-links-guard.test.ts` around lines 401 - 418, Guard the symlink policy test suite by changing its describe wrapper to skip when process.platform is 'win32', while preserving the existing test and symlink validation behavior on other platforms.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/cli/skills.md`:
- Line 15: Update the LLXPRT context link in the skills documentation sentence
to use the corrected sibling target ./llxprt-md.md, or the repository’s
canonical equivalent, instead of ../tools/memory.md.
In `@docs/telemetry.md`:
- Around line 60-63: Update the disable-telemetry documentation to reference
settings.telemetry.enabled instead of settings.enabled, matching the
configuration builder and surrounding documentation.
In `@docs/tool-parsing.md`:
- Around line 118-121: Add the text language identifier to the fenced examples
in docs/tool-parsing.md, including the fences around the TOOL_REQUEST examples
at the referenced sections, so each opening fence uses ```text and documentation
lint passes.
- Around line 17-23: Reclassify `gemma` in the format table so it is listed
under Text-based formats alongside `hermes`, `xml`, and `llama`, rather than
Structured (JSON). Keep the existing format name and all other table entries
unchanged.
In `@docs/Uninstall.md`:
- Around line 37-42: Update the “npx does not install anything” section in
Uninstall.md to distinguish that npx creates no permanent or global installation
while still downloading package contents into the npm cache. Replace the
inaccurate “installs nothing on your system” wording and preserve the existing
guidance about optionally clearing the cache.
In `@scripts/check-doc-links.ts`:
- Around line 61-68: Update the link-handling function’s external-link branch so
the isIgnored result has an observable effect consistent with the intended
.lycheeignore filtering; remove the redundant unconditional return and ensure
ignored and non-ignored external links follow their distinct documented
outcomes.
- Around line 81-121: Update the directory branch in the local-link check to
resolve whichever supported entry file actually exists, index.md or README.md,
before passing it to checkFragment. Preserve the existing missing-index
violation and ensure directory links with fragments validate against the
resolved file without attempting to read a nonexistent index.md.
In `@scripts/doc-links/file-scanner.ts`:
- Around line 252-290: Update isFile, isDirectory, and pathExists in
file-scanner.ts to use statSync instead of lstatSync so symlinked files and
directories are followed. Keep dirHasIndex unchanged so its existing checks
inherit the symlink-aware behavior.
In `@scripts/doc-links/markdown-links.ts`:
- Around line 130-155: Update extractTokenLines to recurse through a token’s
nested tokens and items before falling back to its raw text, while preserving
CODE_TOKEN_TYPES filtering through walkNonCodeTokens. Ensure container tokens
such as paragraphs, blockquotes, and list items do not emit raw source
containing nested code spans or fenced blocks, while leaf tokens still
contribute their raw text.
---
Nitpick comments:
In `@scripts/check-doc-links.ts`:
- Around line 192-200: Remove the dead entryUrl === import.meta.url comparison
from the main execution guard in check-doc-links.ts, including any now-unused
entryUrl conversion. Preserve the existing check-doc-links.ts filename gate, or
use the established import.meta.main pattern consistent with
check-doc-placement.ts.
In `@scripts/tests/doc-links-guard.test.ts`:
- Around line 401-418: Guard the symlink policy test suite by changing its
describe wrapper to skip when process.platform is 'win32', while preserving the
existing test and symlink validation behavior on other platforms.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fb66230d-3801-4806-ac83-13557dfaa76a
⛔ Files ignored due to path filters (9)
dev-docs/architecture/message-bus.mdis excluded by!dev-docs/**dev-docs/documentation-style-guide.mdis excluded by!dev-docs/**dev-docs/hooks/architecture.mdis excluded by!dev-docs/**dev-docs/merge-notes/2026-01-06-batches21-25-skipped.mdis excluded by!dev-docs/**dev-docs/plans/archive/2026-01-03-welcome-onboarding.mdis excluded by!dev-docs/**dev-docs/providers/text-tool-call-parsing.mdis excluded by!dev-docs/**dev-docs/telemetry-internals.mdis excluded by!dev-docs/**dev-docs/tools/tool-output-format.mdis excluded by!dev-docs/**project-plans/issue2654/plan.mdis excluded by!project-plans/**
📒 Files selected for processing (36)
.github/workflows/ci.ymlCHANGELOG.mdCONTRIBUTING.mddocs/EMOJI-FILTER.mddocs/Uninstall.mddocs/agent-api.mddocs/cli/configuration.mddocs/cli/enterprise.mddocs/cli/google-cloud-auth.mddocs/cli/keyboard-shortcuts.mddocs/cli/profiles.mddocs/cli/skills.mddocs/cli/tutorials/skills-getting-started.mddocs/hooks/best-practices.mddocs/hooks/index.mddocs/message-bus.mddocs/migration/approval-mode-to-policies.mddocs/migration/stateless-provider-v2.mddocs/release-notes/2025Q4.mddocs/telemetry.mddocs/tool-parsing.mdpackage.jsonscripts/check-doc-links.tsscripts/check-doc-placement.tsscripts/doc-links/file-scanner.tsscripts/doc-links/heading-slugger.tsscripts/doc-links/lycheeignore.tsscripts/doc-links/markdown-links.tsscripts/legacy-paths/config.tsscripts/lint-all.shscripts/tests/doc-guard-helpers.tsscripts/tests/doc-links-guard.test.tsscripts/tests/doc-placement-guard.test.tsscripts/tests/doc-tree-invariants.test.tsscripts/tests/telemetry-doc-accuracy.test.tstsconfig.scripts.json
💤 Files with no reviewable changes (2)
- docs/cli/keyboard-shortcuts.md
- docs/EMOJI-FILTER.md
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/tests/telemetry-doc-accuracy.test.ts (1)
106-114: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftDerive these documentation claims from source behavior. These assertions only match prose, so a change to the telemetry default, CLI precedence, or hook-I/O behavior can leave stale docs green. Assert the relevant source contract first, then verify the documentation reflects it.
Also applies to: 143-152
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/tests/telemetry-doc-accuracy.test.ts` around lines 106 - 114, Update the telemetry documentation tests around the default and CLI precedence cases to first assert the corresponding source behavior, including telemetry hook I/O behavior where covered by the additional assertions, then verify docs/telemetry.md reflects those contracts. Anchor the source checks to the existing telemetry configuration and CLI-handling symbols, preserving the current documentation checks while ensuring stale prose cannot pass after behavior changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@scripts/tests/telemetry-doc-accuracy.test.ts`:
- Around line 106-114: Update the telemetry documentation tests around the
default and CLI precedence cases to first assert the corresponding source
behavior, including telemetry hook I/O behavior where covered by the additional
assertions, then verify docs/telemetry.md reflects those contracts. Anchor the
source checks to the existing telemetry configuration and CLI-handling symbols,
preserving the current documentation checks while ensuring stale prose cannot
pass after behavior changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 82d2046f-05e1-4f8a-bdff-8a20e2007a82
⛔ Files ignored due to path filters (9)
dev-docs/architecture/message-bus.mdis excluded by!dev-docs/**dev-docs/documentation-style-guide.mdis excluded by!dev-docs/**dev-docs/hooks/architecture.mdis excluded by!dev-docs/**dev-docs/merge-notes/2026-01-06-batches21-25-skipped.mdis excluded by!dev-docs/**dev-docs/plans/archive/2026-01-03-welcome-onboarding.mdis excluded by!dev-docs/**dev-docs/providers/text-tool-call-parsing.mdis excluded by!dev-docs/**dev-docs/telemetry-internals.mdis excluded by!dev-docs/**dev-docs/tools/tool-output-format.mdis excluded by!dev-docs/**project-plans/issue2654/plan.mdis excluded by!project-plans/**
📒 Files selected for processing (37)
.github/workflows/ci.ymlCHANGELOG.mdCONTRIBUTING.mddocs/EMOJI-FILTER.mddocs/Uninstall.mddocs/agent-api.mddocs/cli/configuration.mddocs/cli/enterprise.mddocs/cli/google-cloud-auth.mddocs/cli/keyboard-shortcuts.mddocs/cli/profiles.mddocs/cli/skills.mddocs/cli/tutorials/skills-getting-started.mddocs/hooks/best-practices.mddocs/hooks/index.mddocs/message-bus.mddocs/migration/approval-mode-to-policies.mddocs/migration/stateless-provider-v2.mddocs/release-notes/2025Q4.mddocs/telemetry.mddocs/tool-parsing.mdpackage.jsonpackages/vscode-ide-companion/NOTICES.txtscripts/check-doc-links.tsscripts/check-doc-placement.tsscripts/doc-links/file-scanner.tsscripts/doc-links/heading-slugger.tsscripts/doc-links/lycheeignore.tsscripts/doc-links/markdown-links.tsscripts/legacy-paths/config.tsscripts/lint-all.shscripts/tests/doc-guard-helpers.tsscripts/tests/doc-links-guard.test.tsscripts/tests/doc-placement-guard.test.tsscripts/tests/doc-tree-invariants.test.tsscripts/tests/telemetry-doc-accuracy.test.tstsconfig.scripts.json
💤 Files with no reviewable changes (2)
- docs/cli/keyboard-shortcuts.md
- docs/EMOJI-FILTER.md
🚧 Files skipped from review as they are similar to previous changes (29)
- docs/hooks/index.md
- docs/cli/tutorials/skills-getting-started.md
- docs/message-bus.md
- docs/migration/stateless-provider-v2.md
- docs/release-notes/2025Q4.md
- scripts/lint-all.sh
- scripts/legacy-paths/config.ts
- scripts/doc-links/lycheeignore.ts
- tsconfig.scripts.json
- docs/migration/approval-mode-to-policies.md
- CHANGELOG.md
- docs/cli/google-cloud-auth.md
- CONTRIBUTING.md
- .github/workflows/ci.yml
- docs/agent-api.md
- docs/cli/skills.md
- scripts/tests/doc-placement-guard.test.ts
- package.json
- scripts/doc-links/heading-slugger.ts
- docs/hooks/best-practices.md
- docs/cli/enterprise.md
- scripts/check-doc-links.ts
- docs/telemetry.md
- scripts/check-doc-placement.ts
- docs/cli/configuration.md
- docs/Uninstall.md
- scripts/tests/doc-tree-invariants.test.ts
- scripts/doc-links/file-scanner.ts
- scripts/tests/doc-links-guard.test.ts
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Guard correctness (these guards are this PR's anti-regression mechanism, so unsound guards defeat the purpose): - doc-placement no longer strips HTML comments before scanning. Issue #2654's core complaint was bookkeeping markers hidden in HTML comments in docs/agent-api.md; stripping comments meant re-adding that exact line would report PASSED. Hidden markers are the primary case to catch, not to exempt. This also removes the unsound sanitizing regex behind CodeQL alert 176 (js/incomplete-multi-character-sanitization) by deleting it rather than patching it. Removing 'html' from the code-token exclusion was also required, since marked emits HTML comments as html tokens. - Removed the 15 bookkeeping comment lines the fixed guard now correctly flags. - doc-links resolves whichever of index.md/README.md exists before validating a directory-link fragment. Previously a README.md-only directory plus an anchor threw an uncaught ENOENT that aborted the run and discarded every violation already collected. - Link-target existence checks now follow symlinks, so a symlinked page is no longer reported as missing. Directory-walk loop protection is unchanged. - Fenced code nested in lists and blockquotes is now stripped. Container tokens carry verbatim .raw, so recursing only after the raw fallback re-emitted fenced content and produced false positives against the guard's own advice to "put inside a code fence". - Removed dead .lycheeignore filtering whose result was discarded, and the now-unused module. External links are never fetched, so the filter could not affect any outcome. Documentation accuracy: - telemetry.md: settings.telemetry.enabled, not settings.enabled. - tool-parsing.md: gemma is hybrid, not JSON. Tool declarations are JSON but responses are text-delimited, which the page's own example already showed. - tool-parsing.md: label fenced examples so markdownlint MD040 passes. - Uninstall.md: npx creates no permanent install but does populate the npm cache; "installs nothing" was wrong. Allowlist the single intentional legacy ~/.llxprt reference in Uninstall.md, matching existing narrow entries for other migration-explanation docs. It exists so users can find and delete the pre-migration directory.
- extractConstantValues used '([^']+)' so a constant defined as '' would be skipped, letting a documented-but-empty name look absent. Use '*'. - The logPrompts assertion's alternation had no grouping, so JS read it as (logPrompts.*does.*not.*control) OR (does.*not.*govern). The second branch could match anywhere in the file and pass the test for unrelated prose. Group the alternation and make the gaps lazy so both branches stay anchored to logPrompts.
These tests assert the ABSENCE of bookkeeping markers, so a silently skipped directory or unreadable file made the assertion vacuously true -- a permissions problem would have looked like a clean tree. Narrow both catches to ENOENT (a path removed mid-walk) and rethrow everything else.
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-24.x-ubuntu-latest' artifact from the main CI run. |
The relocated-document invariants listed six new dev-docs paths but only five old docs/ paths, leaving the asymmetry unexplained and one case unverified. docs/tool-parsing.md is not simply deleted: its internals moved to dev-docs/providers/text-tool-call-parsing.md while the docs/ path was deliberately reused for a short user-facing settings page. Document why that path is absent from oldPaths and add a test asserting the page still exists and defers to the dev-docs internals page, so the reuse is an enforced invariant rather than a silent gap. Also record why the slug character class keeps underscore: GitHub preserves it, so dropping it would break existing anchors.
Review of the doc guards surfaced several real defects: The module entry check compared a filesystem path against a file: URL, so it could never match and the guard relied entirely on a filename suffix fallback. Compare like-to-like instead. assertRootExists used lstatSync, contradicting the module's documented policy of following symlinks, so a symlinked documentation root was rejected as "not a directory". processSymlink likewise tested the symlink's own name for a .md extension rather than the resolved target, silently skipping links inside a symlink named notes.txt. checkLocalFragment re-read the source file that scanFile had already read, and fragmentMatches re-lexed a document for every fragment link pointing at it. Thread the already-read content and an explicitly passed slug cache through instead; no module-level mutable state. Two assertions could pass for the wrong reason: the telemetry precedence test iterated a possibly-empty match list without asserting it was non-empty, and the startSpan check was unanchored so a mere mention in a comment would fail it. Also rename stripFencedBlocks to stripCodeTokens, since it strips inline code spans as well, correct the parseTarget contract comment to describe same-file fragment links, widen CI detection beyond the exact string "true", and drop an unreferenced test fixture.
Review flagged the default branch of tokenText as able to double-count, and it does. marked's inline tokens such as strong and em expose BOTH a flattened text property and a nested tokens array covering the same content, so taking both yielded "boldbold" for "**bold**". Every heading containing bold, italic, code, or a link therefore produced a corrupted slug, and valid anchors pointing at those headings were reported as broken. Prefer the nested tokens and fall back to text only for leaf tokens. Added a regression test covering all four markup kinds and confirmed by reverting the fix that it fails without it. Also tighten three assertions that could pass for the wrong reason: the multi-break test now requires every broken link on the line rather than any one of them; the instrumentation check asserts the exact contents of the instrumentations array, since the documentation claims HTTP is the only one; and the constant-extraction check asserts both lists are non-empty instead of comparing against an arbitrary count that ordinary refactoring would break. The malformed-destination case was split out and renamed. Its original name claimed both links were detected, but the unclosed parenthesis makes the first bracket pair ordinary text under CommonMark, so only the second is a link. The test now states what it actually verifies: that the scan continues past the malformed destination. Drops a duplicated docs/tool-parsing.md existence assertion.
Review flagged that the tokenText fallback lets raw markup into slugs, and it does. marked emits each inline tag as its own html token whose text is the literal markup, so "## Hello <span>world</span>" slugged to hello-spanworldspan and the guard rejected the valid anchor #hello-world. GitHub renders the HTML and slugs only the visible text. Skip html tokens; the surrounding text tokens already carry the visible content. Verified end-to-end before and after, and confirmed the new regression test fails when the html case is removed. Also make dirContains match directories only. It used readdirSync().includes(), so a plain file named docs/architecture would have satisfied the "internal directory is absent" assertions. Confirmed by creating such a file: the weakened helper passes, the fixed one fails. Documents that searchRepoForMarker inspects only .md files and excludes project-plans, and that extractConstantValues assumes the single-quoted literals Prettier enforces here.
Three narrow gaps in the test-side scanners, all confirmed against the real tree before changing anything. The fence-balance check counted only exactly three backticks, but docs in this repo use four-backtick fences to wrap examples that themselves contain fences, and tilde fences appear in dev-docs. Count any run of three or more backticks or tildes. The marker scanner matched .md case-sensitively while the guard's own isMarkdown() lowercases first, so the two disagreed about which files exist. Align the scanner with the guard. EVENT_ENHANCED_CONVERSATION_RESPONSE wraps onto a second line in constants.ts, so the extractor's newline-spanning behavior is load bearing rather than incidental. Assert that constant is extracted; verified the assertion fails when the regex stops spanning newlines.
acoliver
commented
Jul 25, 2026
stripCodeTokens drops both code lines and blank lines, so the returned array cannot be indexed to recover a source line number. The only current caller does a whole-document search and is unaffected, but the contract was undocumented, so a future caller could reasonably assume alignment and report wrong positions. Say so explicitly. Also rename a fragment test whose name and comment described a leading hash inside the fragment text, which is not what the test exercises. It covers cross-file fragment resolution against the target's headings.
…nces
Fixes the nine review findings on this PR.
Telemetry (docs/telemetry.md):
The page led with "telemetry is disabled by default", which was misleading in
both directions. It implied the always-on local session aggregation was gated
by telemetry.enabled (it is not - loggers.ts aggregateLocally() runs
unconditionally), while burying the far more important fact that no export
path exists at all. Now leads with "never sends telemetry anywhere", then
separates the always-on in-memory /stats layer from the opt-in OTEL
file/console layer. Also documents that --telemetry-target,
--telemetry-otlp-endpoint and --telemetry-otlp-protocol are accepted but inert,
since no exporter reads them.
Removed the corresponding false startup tip ("Send telemetry data to a local
file or GCP") and the tip advertising the inert OTLP endpoint setting.
Audience separation:
Removed seven docs/ -> dev-docs/ links. User-facing pages should not route
readers into contributor documentation; several of these were introduced by the
relocation earlier in this PR.
Moved docs/agent-api.md to dev-docs/ and added an explicit instability notice.
It was presented as a public embeddable API but carries no stability guarantee.
Deleted docs/migration/stateless-provider-v2.md and docs/release-notes/2025Q4.md
(both fully orphaned, no inbound links) and delinked the two historical
CHANGELOG references rather than rewriting released-version history.
Stale/incorrect references:
- configuration.md advertised a gemini-cli-sandbox image; the real image is
ghcr.io/vybestack/llxprt-code/sandbox.
- Replaced two absolute github.com blob/tree links, which escape the site when
docs are published to vybestack.dev.
- Renamed docs/EMOJI-FILTER.md to docs/emoji-filter.md to match its peers.
Guard coverage:
Added invariants so these cannot regress: no docs/ page may link into
dev-docs/, and no docs/ page may deep-link to a GitHub blob/tree URL. Both were
mutation-tested to confirm they fail on injected violations. The emoji filename
assertion reads the directory rather than using existsSync, which cannot detect
case on macOS. Doc guard suite: 101 tests passing.
6 tasks
The fence-balance check anchored at column 0, so a code block indented by one to three spaces (legal per CommonMark) matched zero fences and the modulo assertion passed vacuously. Allow the optional indent so indented fences are actually counted.
5 tasks
Four review findings that were real defects rather than style preferences:
- scripts/check-doc-links.ts: scanFile discarded the underlying read error
and reported a bare "unreadable file", making a CI failure impossible to
diagnose from the log. Include the error message.
- scripts/check-doc-placement.ts: dirExists treated every statSync error as
"directory absent". A forbidden directory that exists but is unreadable
(EACCES, ELOOP) would therefore pass the guard - a false negative, which is
the worst outcome for a guard. Only ENOENT is a real absence; rethrow the
rest.
- scripts/check-doc-placement.ts: hasBookkeepingPrefix would spin forever on
an empty prefix, since indexOf('') returns the same index and the loop never
advances. Latent today because PREFIX_MARKER_DISPLAYS is non-empty.
- scripts/tests/doc-tree-invariants.test.ts: the GitHub deep-link assertion
hardcoded the vybestack/llxprt-code slug, so a rename or fork would silently
turn it into a no-op. Derive owner/repo from package.json repository.url and
fail loudly if it cannot be parsed. Verified still catching violations by
injecting one. Also type `entries` as Dirent[] instead of implicit any.
The audience-separation assertions only matched inline `](dest)` links, so a reference definition (`[id]: ../dev-docs/...`) could reintroduce exactly the docs -> dev-docs leak this PR removes without failing the suite. Both checks now match either form; each was mutation-tested by injecting a reference definition into docs/index.md and confirming it fails. Also hoist repoBlobOrTreePattern() out of the filter callback (it re-reads and parses package.json on every call, and the pattern carries no /g state), and rename the PLAN_ prefix test, which claimed to exercise case-insensitivity while asserting an already-uppercase marker against a case-sensitive check.
Comment on lines
+94
to
+97
| // Containment check: resolved target must stay within the repository root | ||
| if (!isWithinRoot(absTarget, root)) { | ||
| return `${target} resolves outside the repository`; | ||
| } |
Contributor
There was a problem hiding this comment.
Potential path traversal via symlinks:
isWithinRootonly checks the path string prefix and does not resolve symlinks before containment. If a symlink exists insidedocs/ordev-docs/pointing outside the repository root (e.g.,docs/secret -> /etc), links likesecret/passwdwill resolve to a path string still within the repo prefix, but will actually access files outside the repo via the symlink. Consider canonicalizing the target withfs.realpathbefore performing the containment check, or rejecting symlink targets that resolve outside the root.
Comment on lines
+160
to
+163
| function relativePath(absPath: string): string { | ||
| const { root } = getRoots(); | ||
| return relative(root, absPath).replace(/\\/g, '/'); | ||
| } |
Contributor
There was a problem hiding this comment.
relativePath()callsgetRoots()on every invocation, which re-reads theDOC_GUARD_ROOTenvironment variable and re-resolves the root directory path. Since it is called for every broken link found, this causes redundant environment lookups and path resolution, and could yield inconsistent results if the environment variable were mutated during execution. RefactorrelativePathto accept the already-resolvedrootas a parameter, and callgetRoots()only once inmain().
Comment on lines
+147
to
+158
| function getFragmentContext( | ||
| filePath: string, | ||
| slugCache: SlugCache, | ||
| ): FragmentContext { | ||
| let ctx = slugCache.get(filePath); | ||
| if (ctx === undefined) { | ||
| const content = readFileText(filePath); | ||
| ctx = { content, slugs: extractHeadingSlugs(content) }; | ||
| slugCache.set(filePath, ctx); | ||
| } | ||
| return ctx; | ||
| } |
Contributor
There was a problem hiding this comment.
getFragmentContext()callsreadFileText()without error handling. If a target Markdown file is deleted, moved, or becomes unreadable (EACCES, EISDIR, encoding error) after the initial file list is collected, the script will throw an uncaught exception and crash instead of reporting a clean broken-link error. Wrap thereadFileTextcall in a try/catch and return aBreakentry describing the unreadable target, similar to the error handling inscanFile().
Comment on lines
+143
to
+149
| function checkMarkers(root: string): readonly Violation[] { | ||
| const docsDir = join(root, 'docs'); | ||
| const files = collectMarkdownFiles([docsDir]); | ||
| const violations: Violation[] = []; | ||
| for (const file of files) { | ||
| const content = readFileText(file); | ||
| const markers = findMarkerViolations(content); |
Contributor
There was a problem hiding this comment.
readFileText(file)can throw for unreadable files (e.g., permission changes, race conditions, transient I/O errors), butcheckMarkersdoes not handle that failure. If any single markdown file becomes unreadable during the scan, the entire script crashes with an unhandled exception instead of reporting a clean violation or continuing. Wrap the read in a try/catch and either report the unreadable file as a violation or skip it with a warning so the guard remains robust in CI.
Comment on lines
+111
to
+126
| function processSymlink(full: string, out: string[], seen: Set<string>): void { | ||
| const realFull = safeRealpath(full); | ||
| if (realFull === undefined) return; // broken symlink — skip | ||
| if (seen.has(realFull)) return; // cycle protection | ||
| seen.add(realFull); | ||
| try { | ||
| const stat = lstatSync(realFull); | ||
| if (stat.isDirectory()) { | ||
| walkDir(realFull, out, seen); | ||
| } else if (stat.isFile() && isMarkdown(realFull)) { | ||
| out.push(full); | ||
| } | ||
| } catch { | ||
| // broken or unreadable symlink target — skip | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Files inside symlinked directories are collected with their real filesystem paths (
walkDir(realFull, ...)), while symlinked files are collected with their symlink paths (out.push(full)). This inconsistency means relative links inside symlinked directories are resolved against the real directory structure instead of the symlinked structure. If a symlink points to a directory with a different layout, links that work through the symlink path may be falsely reported as broken. Consider tracking the original symlink path and resolving relative links from that path instead of the real path.
Comment on lines
+509
to
+514
| it('fails when docs/ root does not exist', async () => { | ||
| // beforeEach pre-creates docs/ and dev-docs/; remove docs/ to test | ||
| rmSync(join(fx.root(), 'docs'), { recursive: true, force: true }); | ||
| await runDocLinksGuard(fx.root(), 1); | ||
| // The guard must fail (non-zero), proving fail-fast works | ||
| }); |
Contributor
There was a problem hiding this comment.
The 'missing root (fail-fast)' test calls
await runDocLinksGuard(fx.root(), 1)without asserting the exit code. If the guard throws instead of returning, Vitest will report an unhandled error rather than a passing assertion. Destructure and assert the result to make the intent explicit, matching the pattern used by every other test in this file.
| // describe blocks even when useTempDir is called multiple times. | ||
| const ref: { root: string } = { root: '' }; | ||
| beforeEach(() => { | ||
| ref.root = mkdtempSync(join(tmpdir(), 'doc-guard-')); |
Contributor
There was a problem hiding this comment.
The template for
mkdtempSyncmust include at least 6 trailing 'X' characters. The current template'doc-guard-'doesn't meet this requirement and will throw an error. This breaks all tests usinguseTempDir().
Comment on lines
+116
to
+124
| const result = await execFileAsync(RUNTIME, [scriptPath], { | ||
| cwd: REPO_ROOT, | ||
| env, | ||
| encoding: 'utf8', | ||
| timeout: 30_000, | ||
| maxBuffer: 10 * 1024 * 1024, | ||
| }); | ||
| stdout = result.stdout; | ||
| stderr = result.stderr; |
Contributor
There was a problem hiding this comment.
util.promisify(execFile)resolves to a tuple[stdout, stderr], not an object withstdoutandstderrproperties. Accessingresult.stdoutwill always beundefined, causing the guard output to be lost and the return value to containundefinedinstead of the actual output.
Comment on lines
+43
to
+45
| const err = error as { code?: string }; | ||
| const isMissingOrDenied = | ||
| err.code === 'ENOENT' || err.code === 'EACCES' || err.code === 'ENOEXEC'; |
Contributor
There was a problem hiding this comment.
The error cast
error as { code?: string }is too narrow becauseexecFileSyncerrors can have a numericcode(non-zero exit code). Whenbun --versionexits with a non-zero code (e.g., corrupted installation),err.codeis a number, soisMissingOrDeniedis false and the error is re-thrown instead of returningfalse. This prevents graceful skipping of Bun-dependent tests when bun is installed but broken.
Comment on lines
+25
to
+27
| function readFile(relPath: string): string { | ||
| return readFileSync(join(repoRoot, relPath), 'utf8'); | ||
| } |
Contributor
There was a problem hiding this comment.
The local helper
readFile(relPath)shadows the conceptualfs.readFilenamespace and may momentarily suggest the async Promise-based API. Renaming it to something more specific, such asreadRepoFile, would make the synchronous, repo-root-scoped nature explicit and reduce cognitive friction.
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
2 participants
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 the documentation audit in #2654. Delivers the mechanically-verifiable half of that audit plus the CI enforcement that keeps it from regressing. The subjective prose rewrites of very large pages are tracked separately in #2685.
This PR does not close #2654 — see "Scope" below.
Why
The audit found user documentation asserting behavior the code does not implement, internal engineering records in public navigation, and broken repo-relative links with no CI to catch them.
I verified every audit claim against source rather than taking it at face value. Several were wrong or understated:
configConstructor.ts:499callsinitializeTelemetryunconditionally;sdk.ts:44early-returns only when disabled, then starts a realNodeSDKCorrectness fixes
docs/telemetry.mdwas wrong in four separate ways:llxprt_cli.*. Real values arellxprt_code.*(packages/telemetry/src/telemetry/constants.ts). All 29 identifiers now verified to exist as real constants.--telemetry. That does not disable it —configBuilder.ts:83isargv.telemetry ?? settings.telemetry.enabled, so persisted settings win. The real session override is--no-telemetry.HttpInstrumentationand there are nostartSpancalls.Also reconciled
docs/cli/configuration.mdanddocs/cli/enterprise.md, which still promised OTLP/gcpexport the SDK cannot perform — the audit explicitly required telemetry docs be non-contradictory.docs/tool-parsing.mddocumented/toolformat text, but the command rejects it (valid:openai|anthropic|deepseek|qwen|kimi|gemma|hermes|xml|llama|auto). It also presentedenableTextToolCallParsing/textToolCallModelsas working controls; those have no production consumer, so the page now says so plainly.docs/Uninstall.mdnamedgemini-cliand led with deleting the entire npx cache. Now leads with the supported channels (npm global, Homebrew), demotes cache clearing to optional troubleshooting with a collateral-deletion warning, derives the Windows cache path fromnpm config get cacheinstead of hard-coding it, and stops implying one directory removal deletes all state.docs/hooks/best-practices.mdhad an orphaned JavaScript fragment rendering outside any code fence, duplicated security guidance (SECRET_PATTERNSdefined twice), a broken link, and a false claim thattelemetry.logPromptsgates hook I/O logging (it does not —loggers.ts:274-290). The#using-hooks-securelyanchor is preserved becausedocs/extension.mdlinks to it.Placement
Relocated six internal documents to
dev-docs/with all inbound links updated:docs/architecture/message-bus-architecture.mddev-docs/architecture/message-bus.mddocs/hooks/architecture.mddev-docs/hooks/architecture.mddocs/tool-output-format.mddev-docs/tools/tool-output-format.mddocs/tool-parsing.md(internals)dev-docs/providers/text-tool-call-parsing.mddocs/merge-notes/batch21-25-skipped.mddev-docs/merge-notes/2026-01-06-batches21-25-skipped.mddocs/plans/2026-01-03-welcome-onboarding.mddev-docs/plans/archive/2026-01-03-welcome-onboarding.mdA short user-facing
docs/tool-parsing.mdremains, covering only the runtime command and troubleshooting.Deleted
docs/cli/keyboard-shortcuts.md: a stale orphan that carriesKEYBINDINGS-AUTOGENmarkers but is not the generator's target (generate-keybindings-doc.tswrites onlydocs/keyboard-shortcuts.md) and is referenced by nothing.Prevention
Two new guards, wired into
ci.ymlandscripts/lint-all.sh:npm run lint:doc-links— validates every repo-relative link and heading anchor acrossdocs/,dev-docs/, and root-level Markdown. Root coverage matters: it caught a bad link I introduced inCONTRIBUTING.mdplus 3 pre-existing ones that only-scanning-docs/ would have missed.npm run lint:doc-placement— rejects internal-only directories reappearing indocs/and plan/requirement bookkeeping markers outside code fences.Both use the
markedlexer (already a declared root dependency) rather than hand-rolled parsing. The first implementation was hand-rolled and had real bypasses — a 4-space-indented```was treated as a fence, silently hiding broken links after it. CommonMark compliance now covers indented code, fence delimiter/length matching, titled and parenthesized destinations, angle-bracket references, multi-backtick spans, setext headings, duplicate-heading suffixes, and non-ASCII anchors. Both guards fail fast on an unreadable root instead of passing vacuously, and target resolution is confined to the repository.82 behavioral tests run the real scripts against real fixture trees and assert real exit codes — no filesystem mocks. Includes negative controls for every bypass found in review.
Scope
In scope: correctness/safety fixes, the six relocations, and the prevention tooling. Deferred to #2685: prose rewrites and splits of
agent-api.md(1373 lines),mcp-server.md(730),approval-mode-to-policies.md(734),quick-reference.md(667), the hook-tutorial consolidation (1592 combined), plussandbox.mdtone and themessage-bus/todo-system/memport/deploymentsplits.This split keeps mechanically-verifiable fixes separate from prose judgement and keeps the diff reviewable. The placement guard already enforces the placement half of the deferred work mechanically. #2654 stays open until #2685 lands.
Verification
npm run lintnpm run lint:doc-links/lint:doc-placementnpm run lint:eslint-guardnpm run typechecknpx prettier --check .npm run buildBypass reproduction, before and after:
Pre-existing failures (not caused by this PR)
Two failures exist that this PR does not introduce, proven rather than assumed:
npm run test: 17 failures inpackages/cliandvscode-ide-companion. This PR changes zero files underpackages/. I created a clean worktree at base commit3a12f02e5and ran the same suites there: 12 failures reproduce with no changes applied.SyntaxError: Export named 'onExit' not found in module .../signal-exit/index.js. Reproduced on a fully clean tree (all changes stashed) with the identical error.inkdepends on CommonJSsignal-exit@3.0.7, which Bun's ESM interop cannot resolve a named export from. Worth its own issue.Also corrected during review: two cases where an emoji filter had mangled pre-existing content (
docs/EMOJI-FILTER.mdexamples and✓/✗glyphs inlint-all.sh).Relates to #2654. Follow-up: #2685.
Summary by CodeRabbit