fix(core): report a hand-rolled in-memory store under $lib/server

[oekazuma]

Closes #354.

The gap

security/handler-state-write exempts .set()/.update() on imports resolving under the $lib server root. That exemption earns its keep — it is where database and KV clients live, and db.set(…) on one is persistence, not shared state. But the check was purely path-based, so this was exempt too:

// src/lib/server/store.ts
export const db = new Map();

// src/routes/+page.server.ts
import { db } from '$lib/server/store';
export async function load({ locals }) {
  db.set('user', locals.user); // previously not reported
}

One shared instance, overwritten by every request, readable by every other — the exact failure the rule exists to catch, hidden by the directory it sits in. The sibling rule cannot cover it either: collectKitModuleFacts deliberately does not scan src/lib/server/**, so security/server-module-state never sees the declaration.

Approach

The call shape cannot separate a hand-rolled store from a client, and parseKitModuleFacts is pure — it cannot read another file. So arbitration moves one level out, to the collector that already holds the Runtime:

1. parseKitModuleFacts stops silently dropping the call. It records a pendingServerStoreWrites entry carrying the resolved path and the exported name (so an aliased import { db as store } still resolves).
2. collectKitModuleFacts reads each targeted module once and promotes into importedStateWrites only the writes whose export is an in-memory container.

A consumer that ignores the new field sees exactly the old behaviour, so nothing changes for anything downstream that has not opted in.

Precision-first, like the rest of the rule

Reported: an export initialized to new Map/Set/WeakMap/WeakSet, or to an object or array literal.

Exempt: everything else — a client constructed from a package import, a re-export, a module that cannot be found or read. What the read cannot positively identify as a container stays silent. In a default-on security rule a missed finding is the cheaper failure than noise on every project using Drizzle.

Verified

Three cases, run against the built CLI on a real fixture:

	Before	After
new Map() under $lib/server, .set() from a handler	not reported	reported
drizzle(url) under $lib/server, .set() from a handler	not reported	not reported
aliased import of the hand-rolled store	not reported	reported

Plus six collector tests covering the index-form module path, the unreadable/missing target, and — asserted explicitly — that only the modules a handler actually writes to are read, so a project whose handlers never touch $lib/server does no extra I/O.

Property writes (store.user = …) were already reported wherever they appear and are untouched. Both the CLI and the Vite plugin go through this collector, so both surfaces gain it.

Docs

The exemption paragraph on the rule page now states the real condition and shows the two-line contrast, en and ja together. Changeset is a @svelte-vitals/core minor.

build, typecheck, test (1149 in core), lint, check:publish, smoke, blume check and the docs build all pass.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Improved detection of shared in-memory state accessed through server modules.
  • Reports writes to exported Map, Set, weak collections, objects, and arrays.
  • Continues to exclude recognized database and KV persistence clients.
  • Supports aliased and relative imports while avoiding uncertain, wrapped, re-exported, or unreadable modules.
  • Limits module inspection to server modules actually written to by handlers.

• Documentation

  • Clarified server-module state detection, persistence-client exemptions, and supported import patterns.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: ad7782ee-ce8e-4033-b810-38573e13b2fd

📥 Commits

Reviewing files that changed from the base of the PR and between 4bd49e1 and 3bdfda3.

📒 Files selected for processing (3)

• .changeset/server-store-arbitration.md
• packages/core/src/kit-module-collect.ts
• packages/core/test/kit-module-collect.test.ts

🚧 Files skipped from review as they are similar to previous changes (3)

• .changeset/server-store-arbitration.md
• packages/core/test/kit-module-collect.test.ts
• packages/core/src/kit-module-collect.ts

---

📝 Walkthrough

Walkthrough

The rule defers .set() and .update() calls on $lib/server imports. The collector resolves written modules and reports exports initialized as in-memory containers while exempting persistence clients and unresolved modules.

Changes

Server store arbitration

Layer / File(s)	Summary
Parse and defer server-store writes packages/core/src/kit-module.ts, packages/core/src/kit-module-parse.ts	The parser tracks imported names and records handler writes to $lib/server modules in pendingServerStoreWrites. It detects exported Map, Set, weak collection, object, and array initializers.
Resolve and arbitrate pending writes packages/core/src/kit-module-collect.ts, packages/core/test/kit-module-collect.test.ts, packages/core/test/correctness-rules.test.ts, packages/core/test/security-kit-rules.test.ts	The collector resolves module and index paths, reads only written modules, and promotes matching in-memory exports to importedStateWrites. Tests cover aliases, persistence clients, unresolved modules, unreadable modules, and supported source forms.
Document the classification behavior docs/src/content/docs/rules/security/handler-state-write.md, docs/src/content/docs/ja/rules/security/handler-state-write.md, .changeset/server-store-arbitration.md	The documentation and changeset describe container detection, persistence-client exemptions, conservative handling, and selective module reads.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Handler
  participant Parser
  participant Collector
  participant ServerModule
  participant Rule
  Handler->>Parser: parse .set()/.update() call
  Parser->>Collector: return pendingServerStoreWrites
  Collector->>ServerModule: resolve and read target module
  ServerModule-->>Collector: return exported bindings
  Collector->>Rule: promote in-memory container writes
  Rule-->>Handler: report imported state write

Loading

Possibly related PRs

• oekazuma/svelte-vitals#235: This PR extends the Kit-module state-write analysis introduced there.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the core fix: reporting hand-rolled in-memory stores under $lib/server.
Linked Issues check	✅ Passed	The changes address issue #354 by detecting in-memory containers while preserving persistence-client, re-export, and unresolved-module exemptions.
Out of Scope Changes check	✅ Passed	The code, documentation, tests, and changeset updates all support the linked issue and stated pull request objectives.
Docstring Coverage	✅ Passed	Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 3

🤖 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 @.changeset/server-store-arbitration.md:
- Around line 26-27: Update the sentence in the changeset so it clearly states
that an unreadable module remains exempt because an unseen wrapper is treated as
silence rather than a false positive; correct the grammatical error without
changing the documented conservative exemption behavior.

In `@packages/core/src/kit-module-collect.ts`:
- Around line 66-85: Update inMemoryExportsOf and MODULE_CANDIDATES so explicit
.js repoPath values are checked exactly first, then resolved with the .js suffix
removed using .ts, .js, and index forms, avoiding candidates such as
store.js.ts. Preserve existing resolution for paths without .js, and add
coverage for the NodeNext-style import mapping to the TypeScript source.

In `@packages/core/test/kit-module-collect.test.ts`:
- Around line 53-60: Rename the test describing the persistence-client exemption
to state the verified behavior only, removing the explanatory “reason the
exemption exists” clause; leave the test implementation unchanged.

🪄 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: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 774ccec9-c4f3-49a2-976e-e5704a4e4329

📥 Commits

Reviewing files that changed from the base of the PR and between d6b60c6 and 4bd49e1.

📒 Files selected for processing (9)

• .changeset/server-store-arbitration.md
• docs/src/content/docs/ja/rules/security/handler-state-write.md
• docs/src/content/docs/rules/security/handler-state-write.md
• packages/core/src/kit-module-collect.ts
• packages/core/src/kit-module-parse.ts
• packages/core/src/kit-module.ts
• packages/core/test/correctness-rules.test.ts
• packages/core/test/kit-module-collect.test.ts
• packages/core/test/security-kit-rules.test.ts

[oekazuma]

All three applied.

The .js specifier finding was a real miss, and I verified it before fixing rather than taking the description on trust. moduleCandidates appended extensions unconditionally, so from '$lib/server/store.js' produced store.js.ts / store.js.js, matched nothing, and left the write unarbitrated — silently exempt. Against a fixture:

	Before	After
new Map(), imported as '$lib/server/store.js'	not flagged	flagged
drizzle(url), imported the same way	not flagged	not flagged

A path that already carries an extension is now checked as written first, then a .js one is remapped to .ts. Extensionless paths keep the previous order. Test added.

Worth noting this is the failure mode the arbitration is designed around: an unresolvable target means unarbitrated, which means exempt — so the bug was a miss rather than noise. That is the right direction to fail, but it also means this class of gap does not announce itself, which is why the resolution list deserves the test it now has.

The test name was named after why it exists rather than what it verifies — the AGENTS.md rule I added in #347, broken in the same session. 'leaves a persistence client exempt — the reason the exemption exists' → 'leaves an export that is not an in-memory container exempt'.

The changeset sentence was ungrammatical; reworded.

build, typecheck, test, lint, check:publish and smoke all pass.