Skip to content

Deduplicate filesystem existence-check helpers across kb-core and agents #690

Description

@williamthorsen

Problem

The codebase has several near-identical "stat a path, treat ENOENT as 'not found', re-throw everything else" helpers, each privately redefined. They share a shape but drift in subtle, behavior-affecting ways — the same class of divergence that #689 eliminated for the config loader.

The duplicated existence helpers

Function Location "Not found" codes Other errors Checks directory?
isExistingDirectory kb-retrieve/recall.ts ENOENT, ENOTDIR re-throws yes
pathExists kb-add/write-note.ts ENOENT only re-throws no
pathExists commands/generate-label-map.ts (all errors) swallows → false no
isDirectory kb-core/discovery/find-kb-root.ts (all errors) swallows → false yes

Across the four, three distinct absence policies exist — {ENOENT}, {ENOENT, ENOTDIR}, and {all errors} — and they disagree on what a permission error means: recall and write-note deliberately re-throw EACCES/EPERM; find-kb-root and generate-label-map swallow it. The swallow-all behavior is defensible for best-effort probes (an ancestor walk, a label-map existence check) but is undocumented and indistinguishable from the others at a glance.

Related, broader duplication

The error-code guard underneath recurs verbatim too. isEnoent is redefined in kb-add/write-note.ts, kb-edit/load-note.ts, commands/install.ts; isEnoentError variants live in derive-session-context/cli.ts and read-preferences.ts; and recall.ts carries a generic isErrorCode. kb-core/type-guards.ts defines a canonical isEnoent, but it is only a module-level export used internally — it is not surfaced through the package's public exports map, so the agents package cannot currently import it. Reusing it therefore requires first adding it to kb-core's public surface.

Context

  • Intentional divergence must be preserved, not flattened. find-kb-root's and generate-label-map's swallow-all and recall's re-throw-on-EACCES differ on purpose. The shared helper should express the policy as an explicit option, not force one policy on every caller. (Decision: find-kb-root keeps swallow-all — aborting an upward search because one ancestor is unreadable is worse than skipping it — now stated explicitly rather than implied by a bare catch.)
  • kb-core is the natural home, consistent with where Consolidate the duplicated kb.yaml loader and surface registry defects in kb-retrieve #689 places tryLoadKbConfig.
  • Public surface follows existing convention: fs helpers go behind a new ./filesystem subpath (matching kb-core's subpath-per-concern, full-word naming); the error-code guards are re-exported from the root barrel, mirroring the precedent already set by run-core, which exports its own isEnoent from its root.
  • Out of scope: mcp/src/staleness.ts and run-core carry their own copies but are not named here; run-core's is a deliberate, independently-exported peer. Left for a separate decision.
  • This is a maintainability cleanup, not a bug fix — no user-visible behavior changes.

Proposed solution

Add two filesystem-existence helpers to kb-corepathExists(path, options) and directoryExists(path, options) — parameterized on which stat-error codes count as "absent," plus an explicit option for the best-effort "treat any failure as absent" policy. Expose them through a new ./filesystem subpath. Point recall.ts, write-note.ts, find-kb-root.ts, and generate-label-map.ts at them, each passing its current policy explicitly.

Surface the canonical isEnoent guard (and a new generic isErrorCode) from kb-core's root barrel, and replace the redundant local isEnoent / isEnoentError / isErrorCode copies in agents with imports of the shared guards.

Acceptance criteria

Must have

  • Shared pathExists and directoryExists helpers live in kb-core behind a ./filesystem subpath, with the "which codes mean absent" / "swallow all" policy expressed as explicit options.
  • recall.ts, write-note.ts, find-kb-root.ts, and generate-label-map.ts call the shared helpers instead of private copies, each retaining its current observable behavior with the difference expressed explicitly rather than reimplemented.
  • New and modified behavior in this change is covered by tests.

Should have

  • kb-core re-exports isEnoent from its root barrel, and the redundant local isEnoent / isEnoentError copies in agents (write-note, load-note, install, derive-session-context cli + read-preferences) are replaced with imports of it.

Nice to have

  • A generic isErrorCode guard in kb-core (root barrel) absorbs recall.ts's local copy, and isEnoent is reimplemented in terms of it.

Metadata

Metadata

Assignees

No one assigned

    Labels

    refactoringImprovement to code without change in functionalityscope:agents

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions