Skip to content

One reader for the PHP files in a directory - #650

Draft
Firehed wants to merge 9 commits into
mainfrom
pipeline/3-directory-reader
Draft

Firehed wants to merge 9 commits into
mainfrom
pipeline/3-directory-reader

Conversation

@Firehed

@Firehed Firehed commented Sep 29, 2026

Copy link
Copy Markdown
Owner

Third of five stacked PRs, on top of the declaration source. #646 is the total view.

What this does

PhpDirectoryReader lists the PHP files and directories inside a directory, and walks a tree of them. ComposerMapBackend walks its autoload roots through it in place of its own private walk.

No behaviour changes.

Why

The stand-in file watcher, two PRs up, needs the same walk. This keeps it written once.

Enforcement edits

Each at Eric's direction, classified by docs/architecture/enforcement-edits.md.

Edit Class
phpstan.neon filesystem allowlist: add PhpDirectoryReader Loosen
deptrac.yaml: add the Filesystem layer Tighten
deptrac.yaml: Knowledge may use Filesystem Loosen

🤖 Generated with Claude Code

@codecov

codecov Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.65%. Comparing base (b5f69e1) to head (04fe907).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff            @@
##               main     #650   +/-   ##
=========================================
  Coverage     99.65%   99.65%           
- Complexity     1903     1910    +7     
=========================================
  Files           136      138    +2     
  Lines          4882     4898   +16     
=========================================
+ Hits           4865     4881   +16     
  Misses           17       17           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

@Firehed
Firehed added this pull request to stack #653 September 29, 2026 17:17

namespace Firehed\PhpLsp\Filesystem;

final class PhpDirectoryReader

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can probably leverage the native directory iteration tooling

Comment thread src/Filesystem/PhpDirectoryReader.php Outdated
@Firehed
Firehed force-pushed the pipeline/3-directory-reader branch from 706e677 to 66c7b9f Compare September 29, 2026 20:01
Base automatically changed from pipeline/2-declaration-source to main September 29, 2026 21:10
Firehed and others added 6 commits September 29, 2026 14:11
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@Firehed
Firehed force-pushed the pipeline/3-directory-reader branch from 66c7b9f to 60f9bb8 Compare September 29, 2026 21:11
Firehed and others added 3 commits September 29, 2026 14:22
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@Firehed

Firehed commented Sep 30, 2026

Copy link
Copy Markdown
Owner Author

Adversarial review of this stack (650/651/652) before landing. Findings verified against source.

Where the design fights itself

PR 650 doesn't simplify what it extracts. ComposerMapBackend::walkPhpFiles still exists as a private 4-line adapter over PhpDirectoryReader::walk(). The backend gained a constructor arg; it didn't lose logic. Both PhpDirectoryReaderTest and ComposerMapBackendTest (and downstream PollingFileWatcherTest) build real temp trees, so the extraction didn't buy testability either. new PhpDirectoryReader() is instantiated twice on branch 5 — KnowledgeStack::forProject and Server::forProject.

PR 651 gives each holder a second description of its own staleness, already drifting from the first.

  • ComposerAutoloadMapReader::invalidate drops on any path under vendor/composer/; watchedPaths() declares four specific files.
  • ComposerMapBackend::invalidate accepts any .php path; watchedPaths() declares only PSR-4/PSR-0 roots.
  • AutoloadFilesBackend repeats autoloadFiles() on both sides.

Two routes to "what makes this holder stale," in a design axiom that says one route.

PR 652 watches a different set than the route it stands in for. WatchedFilesRegistrar registers **/*.php; the polling stand-in watches declared roots only. Third definition of the invalidation set. It also costs 8 ms per message while typing on openemr, while the record itself lists a 30–35 ms search scan as unaddressed.

The gap is narrower than the record suggests

  • In-editor edits already reach the catalog through TextDocumentSyncHandler::handleDidClose → invalidate.
  • Hover/definition on a class new since server start already work with no index: ComposerMapBackend::lookup builds a fresh ClassLoader and calls findFile, hitting disk directly.
  • What actually remains: prefix-completion and namespace-listing staleness for classes that arrived via git pull or composer install, under clients that don't send didChangeWatchedFiles (ALE, effectively).

Simpler answer

ComposerAutoloadMapReader stats its four map files before each message. ~30 lines. No Watch layer, no Snapshot, no DirectoryListing, no PR 650. Catches composer install — the one out-of-editor event that rewrites the whole map. git pull adding classes under ALE costs a restart or a client-side ALE fix.

Where the design is right

  • Snapshot::mayDifferFrom's whole-second mtime handling is correct — most pollers get this wrong.
  • Reporting through the same InvalidatableInterface the handler uses, gated on watchedFilesDynamicRegistration, means holders never learn polling exists and conforming clients pay nothing.
  • Not watching content is the right call once the parse cache is text-keyed.
  • Controlled-clock tests and the end-to-end ServerTest case are good.
  • The interface+composite shape does conform to the project's rules.

Enforcement growth

Two filesystem allowlist entries and three deptrac layers grew to admit this. CLAUDE.md flags that as a signal, not a chore.

Verdict

650 only earns its keep if 652 lands. 651+652 are a correct, well-tested, oversized answer to an ALE-only completion-staleness gap that a four-file stat plus "restart after pull" covers at a thirtieth of the code.

Moving all three back to draft to reconsider.


Review generated by an adversarial pass with the Fable model, claims verified against source before posting.

@Firehed
Firehed marked this pull request as draft September 30, 2026 00:38
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.

1 participant