Security audit follow-up: resolve all deferred items - #86
Merged
Conversation
…amples/ Both were flagged as a deferred finding in the prior audit pass (AUDIT.md #15) pending confirmation they weren't serving some standalone-publish purpose that would make removing them unsafe. Checked both possibilities directly: - @shorthand/core is not published to npm (registry 404) despite shorthand/'s lockfile implying an independently-tracked dependency tree. - @smallchat/examples is published, but publishing doesn't consume a package-lock.json, so that's not a reason to keep one either. More importantly, `npm install` run directly inside either directory does not update its local lockfile at all — npm detects the ancestor workspace root and defers to the root package-lock.json entirely, so these files were not being maintained by any normal workflow. This had already caused real drift: shorthand/package-lock.json still resolved vitest@3.2.4, the exact version with the critical CVE fixed at the root in the prior audit pass (#85) — invisible to root-level `npm audit`/CI, the same class of blind spot as the packages/docs lockfile found in that same pass. A lockfile nobody can update and that silently reintroduces fixed CVEs is worse than no lockfile. Verified: fresh `npm install` from a clean node_modules, `npm test` (1153/1153), `npm run build`, and `npm run build:packages` all pass using only the root lockfile.
Second half of the deferred packages/docs dependency work: bumping just @docusaurus/preset-classic (via npm audit fix --force) while @docusaurus/core stayed pinned at 3.6.3 produced a mismatched, out-of-range pair that wasn't safe to ship. Bumped all four @docusaurus/* packages (core, preset-classic, module-type-aliases, types) together to the current latest, 3.10.2. - npm audit: 44 -> 24 remaining findings. All 24 are serialize-javascript/uuid/sockjs/webpack-dev-server, pulled in transitively via @docusaurus/bundler's own webpack toolchain, with "No fix available" per npm audit's own output even at Docusaurus's current latest release -- nothing further to do here until Docusaurus updates that dependency chain upstream. These are build/dev-time tooling deps, not shipped in the static site output. - Fixed the onBrokenMarkdownLinks config deprecation warning the bump surfaced (moved to markdown.hooks.onBrokenMarkdownLinks per the new Docusaurus config shape). - Aligned engines.node from >=18.0 to >=20.0 -- Docusaurus 3.10 itself now requires Node >=20, so the old floor was both already inconsistent with the rest of the monorepo and no longer accurate. Verified: `npm run build` (docusaurus build) succeeds cleanly with no warnings; root `npm test` (1153/1153) and `npm run build` unaffected (packages/docs is outside the root workspace/tsconfig).
Deferred item #13 from the security audit: ToolClass/ToolProxy are re-exported from `@smallchat/core/inference`, documented as the durable, transport-agnostic engine -- but ToolProxy statically imported MCPTransport/getTransport from src/mcp/transport.ts, ~550 lines of HTTP/JSON-RPC/SSE/gRPC wire-protocol code. Anyone importing `@smallchat/core/inference` expecting pure selection logic got concrete networking code pulled in transitively, contradicting the entry point's own stated contract in ARCHITECTURE.md. - src/core/types.ts: added ToolTransport (execute/executeStream/ executeInference), ToolTransportConnectionOptions, and a ToolTransportFactory type -- type-only, zero runtime import cost. - src/core/tool-class.ts: ToolProxy no longer imports mcp/transport.js at all. Its constructor takes an optional transportFactory; getTransport() is now a private method that uses the injected factory (lazily, same caching behavior as before) or returns null. execute()/ executeStream() return a clear "no transport configured" ToolResult error when no factory was injected, rather than silently reaching into a hardcoded implementation; executeInference() returns nothing, matching its existing "unsupported transport" no-op pattern. - src/compiler/compiler.ts (createIMP) and src/mcp/artifact.ts (hydrateRuntime) -- the only two places that construct ToolProxy -- now explicitly pass getTransport from mcp/transport.ts, preserving identical existing behavior at both call sites. - src/inference.ts and src/index.ts: export the three new types alongside the existing core type exports. Verified the fix is real, not just moved: grepped the built dist/inference.js, dist/core/*.js, and dist/runtime/*.js for any reference to mcp/transport -- none. Added src/inference.test.ts, a source-scan regression test that fails if any core/runtime file statically imports mcp/transport.ts again. Added constructor-injection tests to core/tool-class.test.ts (no-factory error path, and routing through an injected factory). Verified: npm run lint (tsc --noEmit), npm test (1156/1156), npm run build, and npm run build:packages all clean.
…tance Deferred item #21, tracked as a "Phase 0, do first" fix in docs/ecosystem/engineering-guide.md: PR #58 titled itself "Extract @shorthand/core package from compaction, CRDT, and importance modules," but only compaction and CRDT actually got extracted -- src/compaction/ and src/crdt/ don't exist as local directories, only as re-exports from @shorthand/core in src/index.ts. src/importance/ was left as a full duplicate copy of shorthand/src/importance/ instead, and it had already drifted: its types.ts defined a narrower local ConversationMessage (missing the 'tool' role option, timestamp: string | number, and normalizeTimestamp) instead of importing the canonical shared type shorthand/src/importance/types.ts already does. `diff -rq` on the two directories confirmed every other file was byte-identical -- this was purely a types.ts fork. Replaced src/importance/ with a thin re-export of @shorthand/core/importance, matching how compaction/CRDT already work, and deleted the five duplicated implementation files plus their tests (the underlying logic is exercised by @shorthand/core's own test suite). The public @smallchat/core/importance subpath is unchanged -- same exported names, same behavior -- so this is non-breaking for anyone consuming it. Making this change surfaced a real CI gap: shorthand/'s own ~260-spec test suite (compaction, CRDT, importance) was never run by root `npm test` or CI -- root vitest.config.ts only globs the root src/, and nothing wired shorthand's tests into either. This was true even before this change; deleting the local duplicate just made the gap visible rather than causing it. Added `npm test --workspace=shorthand` to CI's test job to close it. Adds src/importance/index.test.ts: a smoke test verifying the re-export barrel resolves and works end-to-end (not re-testing the underlying logic, which shorthand's own suite already covers). Verified: npm run lint (tsc --noEmit) clean, npm test (1106/1106), npm run build, npm run build:packages, and `npm test --workspace=shorthand` (260/260) all pass.
Root spec count dropped from ~1,150 to ~1,106 (removing the duplicated src/importance/ implementation files also removed their tests -- the underlying logic is now exercised by @shorthand/core's own suite instead). Also documents `npm test --workspace=shorthand`, which the audit follow-up wired into CI but wasn't mentioned anywhere for anyone running it locally.
Last deferred item from the security audit. commander@15 and better-sqlite3@13 (two majors ahead of what was pinned) both now require Node >=22 upstream -- a real breaking-change decision that wasn't this audit's to make unilaterally, so it was put to the maintainer directly: raise the floor, or hold at Node >=20 and skip the bumps. Decision: raise the floor. BREAKING CHANGE: engines.node is now >=22.0.0 (was >=20.0.0). Node 20 is no longer supported. - package.json: commander ^13.0.0 -> ^15.0.0, better-sqlite3 ^11.0.0 -> ^13.0.3, @types/better-sqlite3 ^7.6.12 -> ^9.6.0, engines.node -> >=22.0.0. - shorthand/package.json: better-sqlite3 bumped to match (it depends on the same package directly -- now deduped to one copy across the workspace instead of two divergent majors), plus vitest ^3.0.0 -> ^3.2.7 to match the critical-CVE fix applied to root earlier in this audit but missed here, and engines.node -> >=22.0.0. - packages/examples/package.json, packages/docs/package.json: engines.node -> >=22.0.0 for consistency. - src/cli/commands/init.ts: the package.json template `smallchat init` scaffolds for new projects also bumped to >=22.0.0 -- it depends on @smallchat/core, which now requires that floor anyway. - .github/workflows/ci.yml: test matrix ['20','22'] -> ['22','24']; the audit and docs jobs' single Node version '20' -> '22'. - README.md: "Requires Node.js >= 20" -> ">= 22". Verified beyond typecheck/tests (which a native-binding major bump like better-sqlite3 wouldn't necessarily catch): built and ran the actual CLI against the new versions -- `smallchat --help` renders correctly and `smallchat doctor` confirms "better-sqlite3 + sqlite-vec: working". Verified: npm run lint, npm test (1106/1106), npm run build, npm run build:packages, and npm test --workspace=shorthand (260/260) all pass on Node 22.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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.
Summary
Follow-up to #85 (merged). That PR's
AUDIT.mddeliberately deferred five findings rather than folding them into a security-audit branch without individual review. This PR resolves all five, plus one thing found along the way. Full detail inAUDIT.md's "Follow-up: Deferred Items Addressed" section and the correspondingCHANGELOG.mdentries.What's in this PR
shorthand/,packages/examples/). Neither was actually maintained by any normalnpm install— npm defers to the workspace root — andshorthand/package-lock.jsonhad already drifted back tovitest@3.2.4, the exact CVE fixed in Security audit: close MCP/channel auth gaps, fix critical CVE, add CI #85, invisible to rootnpm audit/CI.packages/docs: Docusaurus3.6.3→3.10.2(all four@docusaurus/*packages together), resolving 44 → 24npm auditfindings. The remaining 24 have no fix available yet upstream (confirmed vianpm audit's own output) — they're in@docusaurus/bundler's webpack toolchain, dev-time only, not shipped in the built site.ToolClass/ToolProxyhad — statically importingMCPTransportfromsrc/mcp/transport.ts, contradicting@smallchat/core/inference's "transport-agnostic durable core" contract.ToolProxynow takes an injectedtransportFactory; the two places that construct one (compiler.ts,mcp/artifact.ts) pass the concrete implementation, preserving identical behavior. Added a source-scan regression test (src/inference.test.ts) so this can't silently regress.src/importance/fork. It had drifted from@shorthand/core/importance(a narrower localConversationMessagetype, missing the'tool'role andnormalizeTimestamp) — tracked as a "Phase 0" fix indocs/ecosystem/engineering-guide.md. Replaced it with a thin re-export, matching how compaction/CRDT already work. Found and closed a real CI gap while making this change:shorthand/'s own ~260-spec test suite was never run by rootnpm testor CI.>=22, with an explicit decision rather than a unilateral one — I asked before doing this, since it's a real breaking change. Unblockedcommander(13→15) andbetter-sqlite3(11→13), applied consistently across everypackage.jsonin the repo (including thesmallchat initscaffold template) and CI's Node matrix.Test plan
npm run lint(tsc --noEmit) clean at every stepnpm test— 1106/1106 passing (down from 1153 in Security audit: close MCP/channel auth gaps, fix critical CVE, add CI #85; the importance-fork consolidation removed duplicated tests whose logic is now covered by@shorthand/core's own suite instead)npm test --workspace=shorthand— 260/260, newly wired into CInpm run buildandnpm run build:packagescleanpackages/docs:npm run build(docusaurus build) clean, no warningssmallchat --help,smallchat doctor) against the newcommander/better-sqlite3— a native-binding major bump a type-check wouldn't necessarily catchdist/inference.js/dist/core/*.js/dist/runtime/*.jsthat no trace ofmcp/transportremains after the layering fixGenerated by Claude Code