Skip to content

Add directory-entry listing and existence helpers to file discovery - #2355

Merged
kojiwakayama merged 2 commits into
mainfrom
feat/discovery-dir-helpers
Jun 11, 2026
Merged

Add directory-entry listing and existence helpers to file discovery#2355
kojiwakayama merged 2 commits into
mainfrom
feat/discovery-dir-helpers

Conversation

@kojiwakayama

Copy link
Copy Markdown
Contributor

Summary

Extracts the standalone file-discovery infrastructure from the multi-agent staging branch (#2354), per the agreed split (PR A): listDiscoveryDirectoryEntries (immediate, non-recursive directory listing) and discoveryFileExists (path existence), both fsAdapter-aware with Node fallback.

Purely additive — no behavior changes to existing discovery. First of the extraction series; delegates: support (PR B) follows separately.

Testing

  • deno test --no-check --allow-all src/discovery/file-discovery.test.ts — 3/3 pass
  • deno test --no-check --allow-all src/discovery — full suite green

Type of Change

  • New feature (non-breaking, additive infrastructure)
  • Test update

Extracted from the multi-agent staging branch (PR #2354) as standalone
infrastructure: listDiscoveryDirectoryEntries lists immediate entries of a
discovery directory and discoveryFileExists checks path existence, both
fsAdapter-aware with Node fallback. No behavior changes to existing discovery.
@ariskemper

Copy link
Copy Markdown
Contributor

Ran a structured review pass (quality / correctness / security) over this slice. Clean extraction overall — the fsAdapter/Node dual-path mirrors the existing findFilesByExtension conventions and the helpers are exactly the right primitive for the stack. A few findings, all minor:

  1. Redundant Boolean() coercion on the fsAdapter path (file-discovery.ts, listDiscoveryDirectoryEntries): DirEntry.isFile/isDirectory are declared boolean in src/platform/adapters/base.ts:161, so Boolean(entry.isFile) is a no-op — and worse, if an adapter ever wrongly exposed these as methods, Boolean(fn) would coerce to true and silently misclassify every entry as both file and directory. Using the properties directly (as findFilesByExtension does at line 26) keeps a type error surfaced instead of masked.

  2. Node fallback path is untested: all three tests inject a fake fsAdapter, so the readdirSync/Dirent branch (where isFile()/isDirectory() are method calls, not properties) never runs under test. That branch is exactly where the contract differs, so one real-tempdir test without an adapter would lock it in.

  3. (Optional) The fsAdapter-vs-Node branching + exists-check + swallow-errors scaffolding is now repeated a third time in this file. If a fourth helper appears, extracting a shared readDiscoveryDirEntries(dir, context) core would pay for itself — not blocking for this PR.

Nothing security-relevant in this slice. LGTM after #1.

The fsAdapter branch already exposes boolean entry properties, so pass them through directly and add a no-adapter tempdir test for the Node Dirent fallback where the contract differs.

Constraint: Review requested the fsAdapter path avoid masking contract mistakes and the Node fallback be covered.\nRejected: Keep Boolean() coercion | it hides adapter shape errors instead of surfacing them.\nConfidence: high\nScope-risk: narrow\nTested: deno test --no-check --allow-all src/discovery/file-discovery.test.ts\nNot-tested: Full repository suite
@kojiwakayama
kojiwakayama marked this pull request as ready for review June 11, 2026 12:34
@kojiwakayama

Copy link
Copy Markdown
Contributor Author

Addressed the sensible review items in 99699d3d: removed the Boolean() coercion so adapter contract drift does not get masked, and added a real tempdir test for the Node readdirSync fallback path. Focused discovery test and the pre-push suite both passed.

@kojiwakayama
kojiwakayama merged commit a9468b2 into main Jun 11, 2026
19 checks passed
@kojiwakayama
kojiwakayama deleted the feat/discovery-dir-helpers branch June 11, 2026 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants