chore: sweep the Batch B backlog — security, CI gates, ranking tests, honesty fixes - #46
Merged
Merged
Conversation
…h EVAL The 98.3% headline is carried by two of its seven rows (repo dump and blast radius). Only the repo-dump skew was disclosed. Adds the excl-D+F column to every run — 89.8% ai-chatbot, 96.4% zod, 73.1% taxonomy, 93.7% pooled — so the routine-case figure is readable without recomputing it. Also reconciles the two files that looked like they contradicted each other: RESULTS.md prices scenario D against an agent that cats all 65 dependents, EVAL.md prices --relates against grep -l. Both are true; the cheaper baseline is cheaper because it is 59.9% precise. Stated from both sides. Validates chars/4 against o200k_base for the first time on the three pinned eval repos: absolute error is 1.1-12.5% (wider than the +/-10% claimed), but the published saved-% moves only 0.03-0.08pp. Caveat corrected to match. The tokenizer was installed outside the repo; package.json is unchanged.
…h output Content-search lines are the only raw repository bytes agentmap echoes back; everything else it prints is its own metadata. git grep -I skips binary files, so what survives is a TEXT file with escapes embedded on purpose: ESC[2J blanks the terminal, and a cursor-up run can overwrite the file:line prefix so a hit appears to come from a file it did not. C0 controls and DEL are now replaced with U+FFFD inside contentSearch(), so prose and --json get the same sanitised bytes. Tab and newline are kept - stripping tabs would mangle every tab-indented repo. Doing it at the source rather than at the two print sites matters because a --json consumer that parses and echoes a line is exposed to the same trick; MCP's JSON.stringify protects the model, not that consumer. Test asserts both surfaces and is proven to fail with the sanitiser stubbed out. A second test pins the tab carve-out. Also documents the two content-search guards in SECURITY.md - the MCP data fence was implemented but never written down.
The 477 tests had only ever run on ubuntu, while the code carries real per-platform branches (win32 paths in skills/install.mjs, path separators through the resolver) and the docs target Windows. Node versions still fan out on Linux only; Windows and macOS get one pinned version each via include, so this adds two jobs rather than turning a 3-version matrix into nine. Guards the one file that cannot work there: post-commit-hook.test.mjs drives a POSIX /bin/sh script by spawning sh directly. Git for Windows runs that hook through its own bundled bash, so the hook is not what is uncovered on win32 - the harness is. Skipped with that reason attached rather than reshaped into something that passes without executing anything. install-smoke stays Linux-only and now says why in the file: every step is a bash script with absolute /tmp paths, and a half-ported PowerShell version that quietly skips a step is worse than a stated gap.
…installs Four separate correctness gaps, all of the same shape: the tool reported success while doing something other than what it said. --symbols N: map.json persists only the top 80, so --symbols 200 printed "top 200 ranked symbols" above at most 80 rows. n beyond what was persisted now re-ranks from the cached file map - the same ts-morph-free recompute --map --focus already does - so 200 returns 200 where the repo has them. Verified on zod: 200 requested, 200 returned. Where a repo genuinely ranks fewer, the header says so and --json carries requested/shown/truncated. Gemini global install on Windows wrote to ~/.agents/, which is the Amp/legacy convention; Gemini CLI resolves global config under homedir()/.gemini on every platform and never reads .agents. Two files written where nothing loads them, reported as installed. The win32 branch is gone - one path everywhere. settings.json with a non-object "hooks" threw ".some is not a function" partway through a multi-platform install, after earlier platforms were already on disk. Now validated with an error naming the file and the key, and installSkill() preflights every platform in dry-run mode before the first write, so a bad config leaves the repo untouched instead of half-installed. JSONC comments were silently deleted on rewrite, since JSON.stringify cannot round-trip them. Still deleted - a real splice needs a JSONC writer - but now the user is told which file to check. 5 tests, README updated for the --symbols contract change.
None of the three produced code, and two of them should not. Prune rankSymbols cross-product: refuted on five repos (49-409 files, including a Next.js app, the shape most likely to duplicate export names). Largest edge list is 2,533; the most definers any identifier attracts is 17. Pruning at defCount>20 drops zero edges everywhere, and pruning low enough to matter would delete 78.7% of zod's graph including real identifiers. Two of the item's premises were already false: default is excluded from references, and identMul's RARE_PENALTY already discounts the case it wanted removed. Memory ceiling: the rescoped heapUsed sampler was built, measured and reverted. Under a forced 150MB ceiling on zod, a probe firing every iteration logged exactly ONE sample before the OOM - the process dies inside a single getExportedDeclarations(), finer than one loop iteration - and that last sample read 42% of the limit, because V8 collects harder until it gives up. No rate and no threshold can catch it. A guard that never fires still reads as protection, so it is gone; agentmap.mjs is byte-identical. The measured envelope and the --max-old-space-size remedy go to README Troubleshooting instead. Post-commit hook: the lock half already shipped (atomic mkdir, 10-minute orphan clear, process-tree kill). The incremental half stays gated on Tier 2 becoming default-on - it is still experimental with a known reparse tail, and this hook runs on every commit in every consumer repo.
…ge gates Ranking quality had no guard. determinism.test.mjs proves two builds agree and that a known file appears somewhere in hubs — invert the hub comparator and both assertions still pass. Proven: with .sort() reversed, determinism reports 2/2 green while the new ranking-quality suite fails 3 of 5. Fixture is a constructed star (in-degrees 5/2/0) so first place, monotonicity and leaf-vs-hub are all knowable in advance, plus the same check one level down on --symbols. Env isolation: the two --global tests resolved paths against whatever $HOME the runner had, and now go through runWithHome() and assert the fake home appears in the output. git() no longer inherits the developer's ~/.gitconfig or /etc/gitconfig — GIT_CONFIG_GLOBAL/SYSTEM point at a path that does not exist, rather than /dev/null, which is not a path on the Windows runner just added. Typecheck gate: the roadmap assumed typescript already came via ts-morph. It does not - @ts-morph/common vendors it with no tsc bin - so this adds typescript and @types/node as DEV dependencies. The near-zero-deps rule governs runtime deps and the tarball; neither ships (package.json files is an allowlist, and npm pack --dry-run confirms). strict is off with a measured reason recorded in jsconfig.json: strict reports 505 errors, 272 of them 'annotate this parameter'. Non-strict reported 2, both real drift - a JSDoc @type for PLATFORMS that had fallen behind the object, and a dynamic import TS could not follow. Both fixed. Verified jsconfig.json does not perturb agentmap's own map: byte-identical with and without it. Coverage floor uses node --test's built-in coverage, not c8 - one less dep. Measured 93.77% lines / 76.12% branches; floors set at 90/70 so the gate is a regression alarm rather than a target that reddens on ordinary work. Own CI job pinned to Node 24 because the threshold flags do not exist on Node 20. ESLint deliberately NOT added - reasoning recorded in ROADMAP.
The onboarding matrix, uninstall table and troubleshooting section already existed - that roadmap item's anchors were stale. What was actually wrong or absent: The Gemini row claimed the nudge fires on AfterTool. It does not: the hook is registered on BeforeTool and emits a top-level systemMessage. What Gemini drops on BeforeTool is hookSpecificOutput.additionalContext, which is why the nudge used to vanish - the row had inverted the cause and the cure. Four troubleshooting rows added, each for a failure with no visible cause: nvm (the hook and the agent do not source nvm.sh, so node is absent from a PATH that works fine in your shell); the OOM, with the measured envelope showing repo size is the wrong axis - a 252-file app peaks at 683MB against 756MB for 4,000 dependency-free files, because the .d.ts closure dominates; stale skill detection, which --doctor already reports and nothing documented; and 0-files, stated precisely as git ls-files --cached --others --exclude-standard, so the gitignore case is obvious rather than mysterious. Competitor table: every row now links to its project, Repomix's agent-loop cell corrected from 'No' to 'MCP server (no auto-refresh, no nudge)', and an as-of date with an invitation to file drift as a bug.
The recursive tree walk existed twice - sourceFingerprint() and makeProject()'s non-git .vue fallback - identical except for what each does with a file, with every safety rule restated in both copies. Four of those rules are load-bearing and non-obvious (depth cap, per-directory try/catch, lstat-not-stat, skip symlinks), so two copies meant a fix could land in one and not the other. Extracted as walkSources(dir, onFile) with the reasoning written down once. Verified rather than assumed: 43 vue-sfc tests green (the fallback walk is what they exercise), a repo containing a circular AND a dangling symlink still terminates with exit 0, and a non-git .vue tree still resolves its import edge. Two other housekeeping items were already done and only the checkbox was stale: the dead statSync import is gone, and readPackageVersion is one definition with two callers, not the four divergent copies recorded. CI actions were already SHA-pinned. Adds CODE_OF_CONDUCT.md - links the Contributor Covenant rather than copying it, and states explicitly that being told a patch is wrong or that a number does not reproduce is the process working, since this repo closes roadmap items as refuted in writing. No FUNDING.yml: sponsorship handles are the maintainer's call, not something to invent.
ROADMAP proposed release-please or changesets to fix the recurring missing-CHANGELOG-entry problem and the lockfile-version drift. Both are directly checkable, so this closes the item with two tests instead of a dependency tree and a changeset file on every PR. The CHANGELOG gap was real and completely uncovered: nothing anywhere asserted that the shipped version has release notes. package.json always holds the LAST released version, so the check holds continuously rather than only at release time - bumping without writing the section now fails in npm test, not after the notes are already public. The lockfile check exists because npm ci does NOT catch this: it validates the dependency tree, not the root package's own version, which is how a lockfile declaring the previous version in two places shipped once already. Negative control: with package.json forced to 9.9.9, 5 of the 8 lockstep tests fail, including both new ones.
The status table said Batch 3 and Batch B were not started. Batch 3 had 4 of 7 already done; Batch B is now 20 done, 4 partial, 0 untouched. Records what the sweep actually found, because it changes how this file should be read. Several items were already fixed and only the checkbox was stale - the expanded denylist, the MCP injection fence, the dead statSync import, the four readPackageVersion copies, the SHA-pinned actions - and their file:line anchors had drifted, so a finding here can postdate its own fix. Two items were refuted by measurement rather than implemented, and one dependency premise was simply false: typescript does not come via ts-morph. Decisions recorded so they are not reopened from scratch: no changesets (two lockstep tests cover both named symptoms without a dependency tree or per-PR ceremony), no ESLint (checkJs covers the correctness class; the rest is style), no FUNDING.yml (the maintainer's call), no neutral .agentmap/ cache path (a cosmetic gain bought with a permanent dual-read path, on a tool whose central invariant is never serving a stale map), and no eval step in CI (it clones upstream repos, so it would make every PR depend on third-party availability). Each refuted or declined item carries its measurement or its reasoning, not just a verdict.
Adding windows-latest to the matrix is worthless if the suite cannot run there, and dishonest if the gaps are not named. bin-symlink tests now probe whether the platform can create a file symlink at all, by trying it, rather than branching on process.platform - an elevated Windows runner can, and there is no reason to give up the coverage when it can. Where it cannot, they skip with a reason: npm ships .cmd shims there, so argv[1] IS the target and the guard's fast path settles it. The realpathSync branch is then genuinely UNCOVERED on such a platform, and the comments now say so. That correction came from a negative control: disabling the realpath comparison fails the five symlink tests while the new non-canonical-path test still passes, because Node resolves argv[1] before setting it and the string-equality fast path answers first. The first draft of that test claimed to cover the branch. It does not, and a test file asserting coverage it does not have is the exact failure this whole sweep has been about. What the new test does cover, everywhere: invocation through a path a human or a wrapper composed rather than one npm generated. It asserts OUTPUT, never status, because exit 0 with empty stdout is how the original bug hid for 12 releases. Last POSIX-ism gone: core.hooksPath pointed at /dev/null, which works on Windows only by accident (git looks for a \dev\null directory and finds none). Now the same nonexistent-path sentinel used for GIT_CONFIG_GLOBAL/SYSTEM.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Both were caught by the gates added in this branch, on their first real run. CodeQL (high, js/incomplete-sanitization): the CHANGELOG check built a RegExp from VERSION escaping only '.', which leaves every other metacharacter and the backslash itself unescaped. Not exploitable - VERSION is pinned to bare semver by the test above it, from our own package.json - but a half-escape is exactly the thing that gets copied somewhere it does matter. Now parses each heading and compares strings, so nothing needs escaping. Negative control re-run: forcing 9.9.9 still fails it, and the diagnostic now lists real versions since (?!#) stops ### subheads reporting as '#'. Windows: 'importing agentmap.mjs still executes nothing' passed a bare drive path to dynamic import - ERR_UNSUPPORTED_ESM_URL_SCHEME, protocol 'd:'. The file:// template next to it was equally broken there. pathToFileURL handles the drive letter and separators everywhere. Pre-existing; it took a Windows runner to surface it, which is the point of adding one. Also worth recording: the runner CAN create symlinks, so all five symlink tests ran rather than skipping, and the new non-canonical-path test passed there too.
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.
Works the open backlog end to end. 11 commits, each independently reviewable.
What landed
Excl. D+Fcolumn: 89.8% / 96.4% / 73.1%, 93.7% pooled), reconcile the blast-radius row withEVAL.md, validatechars/4against a real tokenizer--symbols Nno longer claims a count it did not return; Gemini global install on Windows no longer writes to a path nothing reads; a malformed config can no longer leave a half-installed skill; JSONC comment loss is now reportedwalkSources()instead of two copies;CODE_OF_CONDUCT.md475 → 490 tests. Typecheck clean. Coverage 93.77% lines / 76.13% branches.
Two items were refuted, not implemented
rankSymbolscross-product — measured on 5 repos (49–409 files). Largest edge list 2,533; most definers for one identifier 17. Pruning atdefCount>20drops 0 edges on every repo; pruning low enough to matter would delete 78.7% of zod's graph including real identifiers.getExportedDeclarations(), finer than one loop iteration. No sampling rate or threshold reaches it.agentmap.mjsis byte-identical to before the attempt.Declined, with reasoning recorded in ROADMAP
changesets (two lockstep tests cover both named symptoms), ESLint (
checkJscovers the correctness class),FUNDING.yml(maintainer's call), a neutral.agentmap/cache path (permanent dual-read path for a cosmetic gain), and an eval step in CI (it clones upstream repos).Known gap, stated rather than papered over
Where a platform refuses file symlinks, the
realpathSyncbranch of the entry guard is uncovered. Verified by negative control: disabling that branch fails the five symlink tests while the non-canonical-path test still passes, because Node resolvesargv[1]before setting it. The comments say so.Why this is a PR
AGENTS.mdrequires it for code changes — and Windows has never run this suite, so CI here is the actual verification.