Fix docref issues #1–#4 and open-review findings#5
Merged
Conversation
Issue #1 — bash symbol resolution crashed with "resolved is not a function" on any script whose parse reached the external scanner (a `[ … ]` test, extglob, or $'…'). tree-sitter-wasms' bash build imports a libc symbol (isalpha) no web-tree-sitter runtime exports, so the emscripten lazy stub blew up. Vendor a source-built bash grammar (pinned CLI, sha256-checked) like proto, wire it into VENDORED_GRAMMARS and the standalone binary's embed list. Issue #2 — `docref check <directory>` surfaced a raw EISDIR. docFiles now expands a directory argument to every scanned doc under it (a file is still taken verbatim; a missing path is kept so the read fails loudly). Issue #3 — the code-side anchor scan skipped dot-directories while the resolver read them by path, so an unused marker under .github/ was never flagged. anchorFiles globs with dot:true so both walk the same files. Findings: - gitcache SCP_LIKE let an option-shaped host/path through (git@-oProxyCommand=…:x, the CVE-2017-1000117 ssh RCE class). Reject a leading '-' on host and path, and whitespace in the path. - ops.update persisted docref.lock before refresh; a refresh failure left the lock advanced past the docs. Write the lock only after refresh. - ops.addRepo appended the [repos.<alias>] block before update fetched; an unreachable remote left a dangling block. Fetch first, persist last, roll back the in-memory alias on failure. - selfupdate.test evaluated assetName at describe scope, crashing the whole file on an unsupported platform. Pin a fixed supported platform for the injected-IO decision tests. - symbols.test's code() helper cast every error to DocrefError and read .code, masking a TypeError as undefined. Re-throw non-DocrefErrors. - install.sh: SC2015 (A && B || C) and SC2086 (unquoted $dir/docref). Tests cover each: bash external-scanner constructs, directory scoping, dot-directory anchors, scp-url host/path injection, findRootOrNull.
The extension loaded a single project from the first workspace folder (loadProject(findRoot(workspaceFolder))). In a multi-repo workspace the docref.toml files sit in subdirectories, so findRoot walked up and found none, loading a default config with no aliases and workspace-relative paths — 100+ false "broken"/"undeclared alias" diagnostics on green docs. Resolve each file's governing project by walking up from the file to its nearest docref.toml (ESLint/EditorConfig style), independent of the workspace folder: - core: findRootOrNull(from) — the nullable governing-root walk; findRoot now delegates to it. - rescan discovers every docref.toml under the workspace and scans each project against its own config; diagnostics and the actionable-at lookup are keyed by absolute path so two projects' docs never collide. - per-document commands (completion, code actions, drift, approve, refresh, add-repo, create/stage anchor) resolve the active file's project. - sidebar trees aggregate across projects with each node tagged with its root, so a node action targets the right repo; a single-repo workspace renders exactly as before. openLocation now opens absolute paths. The pure decision logic — tagRoot, mergeProjectTrees, aggregateStatusText — lives in logic.ts with unit tests, per the extension's thin-wiring rule. Also bound driftDocs: evict each virtual diff doc when its tab closes, so the map no longer grows for the life of the session.
open-review runs its `git diff origin/main...HEAD` inside the container
as root over the runner-owned checkout. git's dubious-ownership guard
then rejects the mount ("Not a git repository. Use --no-index"), so the
reviewer crashes with exit 2 before it can review — gating every PR.
Pass safe.directory=/repo via GIT_CONFIG_* env on the docker run, which
applies to every git call inside the container without a config file.
Reproduced the crash and verified the fix locally against the published
image (static scan then runs clean: 0 findings across the changed files).
The dogfood job doesn't hit this because `docref check` tolerates git
failing (it falls back to a glob); open-review hard-fails on the diff.
- vscode onFsEvent (WARNING, gating): the watcher's per-project match used `uri.fsPath.startsWith(s.root + '/')` with a hardcoded '/', so on Windows (backslash fsPaths) it always skipped and never rescanned on source/doc changes — only on-save and config events worked. Drop the buggy manual guard and rely on relPath, which normalizes separators and returns null outside root (the codebase's Windows-safe pattern, already unit-tested). - ops.update (NOTE): it advanced project.lock in-memory before refresh but, unlike addRepo's project.repos rollback, did not restore it if refresh threw — leaving a reused Project advanced past disk. Add the symmetric rollback, with a regression test (red-checked) that forces refresh to fail mid-update and asserts both the in-memory and on-disk lock are intact. The one discarded finding (tagRoot in-place mutation) was correctly dropped by the judge as speculative — the docstring documents the mutation and all callers pass freshly-built trees.
open-review flagged the synchronous statSync inside async docFiles as event-loop-blocking. Swap both statSync sites (docFiles and anchors) for `stat` from node:fs/promises; the anchors loop now does one stat per file instead of two. Pure internal change — no behavioral difference — so the anchors claim prose is re-approved unchanged.
open-review flagged two write-ordering gaps in the earlier rollback work: - update(): writeLock ran outside the try/catch, so a lock-write failure (disk full) left the in-memory lock advanced while the file stayed old. Move writeLock inside the guarded region — on failure the in-memory lock is rolled back to match the unwritten file. - addRepo(): appendRepoBlock ran after the lock-writing update, so an appendRepoBlock failure left a lock entry with no toml declaration. Reorder to: checkOnly update (reachability, no writes) → append the toml block → real update (lock + refresh). A failure now persists nothing, or at worst leaves a declared-but-unlocked alias (recoverable via update), never a lock without its declaration. Also reworded the docFiles stat-catch comment: it swallows any stat error (missing / permission / symlink), not only ENOENT, so it no longer claims the downstream read always reports ENOENT.
… nodes Regression from the multi-repo refactor: staged sidebar items (buildStageTree) carry no `root`, so clicking one called openAnchor(ref, undefined) and the new `if (!root || !refRaw) return` guard silently did nothing — where the old code used workspaceRoot(). Fall back to the active editor's project when no root is supplied, matching how the other staged-item actions resolve via projectForNode. open-review surfaced this but its cascade discarded it on a wrong-line-number technicality; re-verifying against the actual code confirmed the regression is real, so fixing it despite the discard.
The issue #2 directory-scoping built its match prefix from the raw CLI arg, but scanned docs are POSIX paths (tinyglobby). On Windows a `content\dir` argument would match nothing and the directory would silently expand to an empty set (exit 0, looking like a clean pass). Normalize the arg's backslashes to forward slashes before building the prefix. Surfaced (then discarded as low-risk) by open-review; it's the same hardcoded-'/' vs Windows-'\' class as the onFsEvent fix, in a feature this PR adds, so hardening it is in-scope.
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
1 participant
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.
Resolves the four open issues on this repo plus the actionable open-review findings.
Issues
resolved is not a function).tree-sitter-wasms' bash grammar imports a libc symbol (isalpha) noweb-tree-sitterruntime exports, so any script whose parse reached the external scanner ([ … ]tests, extglob,$'…') crashed the emscripten stub. Vendored a source-built bash grammar (pinned CLI, sha256-checked) like proto and wired it intoVENDORED_GRAMMARSand the standalone binary's embed list. Verified fixed in the compiled binary.check <directory>raw EISDIR.docFilesexpands a directory argument to every scanned doc under it; a file stays verbatim; a missing path is kept so the read fails loudly (ENOENT), not silently dropped.anchorFilesglobs withdot: trueso the code-side inventory walks.github/etc. like the reference resolver does — closing the unused-marker blind spot where CI configs live.docref.tomlfrom the workspace folder. Now each file's governingdocref.tomlis resolved by walking up from the file (ESLint/EditorConfig style, newfindRootOrNull), independent of the workspace folder.rescandiscovers and scans every project in a multi-repo workspace against its own config; diagnostics and nodes are keyed/tagged by project so nothing collides. Single-repo behavior is unchanged.Open-review findings
SCP_LIKEallowed an option-shaped host/path (git@-oProxyCommand=…:x, the CVE-2017-1000117 ssh RCE class). Reject a leading-on host and path, and whitespace in the path.updatepersisted the lock beforerefresh;addRepowrotedocref.tomlbefore validating reachability. Both reordered so disk writes happen only after the fetch/refresh succeed (with in-memory rollback inaddRepo).driftDocsmap — evict each virtual diff doc when its tab closes.selfupdate.testno longer crashes collection on unsupported platforms;symbols.test'scode()helper re-throws non-DocrefErrors instead of masking them.install.shSC2015 + SC2086.parserFordouble-load finding was verified as a false positive (synchronous get/set) and left unchanged.Verification
414 tests pass (regressions added for #1–#4,
findRootOrNull, scp-url injection, and the multi-project logic),tscclean, CLI/extension/standalone bundles build, and docref's own dogfoodcheckis green.