Define SymbolSource / SymbolSink seam and delegating facade - #374
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #374 +/- ##
============================================
+ Coverage 98.12% 98.13% +0.01%
- Complexity 1696 1710 +14
============================================
Files 105 107 +2
Lines 4271 4297 +26
============================================
+ Hits 4191 4217 +26
Misses 80 80 ☔ View full report in Codecov by Harness. |
The comment claimed the null-AST guard skips class registration on a broken file. In fact ParserService reduces an unrecoverable body to an empty statement list ([], not null), so the guard is always true and registration/indexing both run over the empty AST and find nothing to add. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
5 tasks
Firehed
added a commit
that referenced
this pull request
Jul 28, 2026
## Slice S2.2 — Migrate `ClassCandidates` to `search` **Slice:** S2.2 (`docs/architecture/build-manifest.md`, Wave 1) **Plan step:** `docs/architecture/0002-execution-plan.md` Step 2 (§5.5 consumer-migration table; §5.4 lightweight `SymbolDefinition`) — a consumer migration onto the S2.1 seam, no behavior change. **Depends on:** S2.1 (`SymbolSource` / `SymbolSink` + delegating facade, #374) — merged. **RFC sections:** 0001 §4.2 (Symbol Discovery Authority), §4.4 (FQN-based knowledge queries). Routes `ClassCandidates`' workspace prefix search through `SymbolSource::searchClassLikes` instead of naming `SymbolIndex` directly. The one `DelegatingSymbolSource` facade is constructed in `Server.php` and shared; subsequent Step 2 slices route the remaining consumers onto the same instance (Plan §5.5). ### Behavior preservation The plan requires this migration be **identical (same backing)**. `searchClassLikes` searches the whole class-like namespace and returns each match with its `kind`, so the per-position kind narrowing that the index query used to do stays in the consumer, applied against each result's own `kind` — before the `ClassCandidateFilter::accepts()` predicate. That ordering matters: `accepts` resolves through the `ClassRepository`, which cannot vouch for a symbol whose declaration it cannot reach (e.g. an indexed interface with no resolvable file), so the index's authoritative kind must gate first. `testNewCompletionExcludesIndexedInterfaces` pins exactly this. Proven by the existing completion suite across every filter (Any / Instantiable / TypeHint / …) and by the frozen `PrefixSearchParityTest` golden, which stays unchanged because this slice does not touch the search surface itself. ### Acceptance criteria - [x] `ClassCandidates` reads class-like prefix search through `SymbolSource::searchClassLikes`, no longer naming `SymbolIndex` directly. - [x] Offered candidates are identical across all `ClassCandidateFilter` positions (existing `CompletionHandlerTest` coverage green, including the synthetic-interface and abstract-class exclusion cases). - [x] The `PrefixSearchParityTest` golden is unchanged (the search surface is untouched). - [x] The shared facade is wired once in `Server.php`; `NamespaceCandidates` still reads the raw catalog (its migration is S2.3). - [x] `composer test` green (PHPStan + tests + PHPCS). ### Not in this slice - The §4.2 enforcement rule that forbids naming `SymbolIndex` / `ClassRepository` / `NamespaceCatalog` outside a backend lands in **S2.6** (scoped-exempt), once the other consumers (S2.3–S2.5) have migrated. ### Candidate closes None — the manifest lists no `Closes` for S2.2. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
6 tasks
Firehed
added a commit
that referenced
this pull request
Jul 28, 2026
## Slice S2.3 — Migrate `NamespaceCandidates` to `childrenOf` **Slice:** S2.3 (`docs/architecture/build-manifest.md`, Wave 1) **Plan step:** `docs/architecture/0002-execution-plan.md` Step 2 (§5.5 consumer-migration table; §5.3 name-type model) — a consumer migration onto the S2.1 seam, no behavior change. **Depends on:** S2.1 (`SymbolSource` / `SymbolSink` + delegating facade, #374) — merged. **RFC sections:** 0001 §4.2 (Symbol Discovery Authority), §4.4 (FQN-based knowledge queries), §5.1 (typed identifiers). Routes `NamespaceCandidates`' namespace enumeration through `SymbolSource::childrenOf` instead of naming `NamespaceCatalog` directly. Each of the four call sites now wraps the namespace string in a `NamespaceName` (the typed identifier the manifest flagged as needed by this slice, already landed in S2.1). The consumer is constructed with the one shared `DelegatingSymbolSource` facade wired in `Server.php` in S2.2. ### Behavior preservation The plan requires this migration be **identical**. The facade's `childrenOf` unwraps `NamespaceName` and delegates to the same `NamespaceCatalog` the consumer used before, so no enumeration result changes. The frozen `ChildrenOfParityTest` golden (`children-of.json`) is unchanged — this slice does not touch the enumeration surface itself — and the full `NamespaceCandidates` unit suite plus the `CompletionHandler` namespace- navigation cases stay green across every path (absolute, `use`, bare descend, inline/node, multibyte range). ### Acceptance criteria - [x] `NamespaceCandidates` reads namespace enumeration through `SymbolSource::childrenOf`, no longer naming `NamespaceCatalog` directly. - [x] The string namespace is carried as a `NamespaceName` typed identifier at each call site (RFC 1 §5.1). - [x] Offered candidates are identical (existing `NamespaceCandidatesTest` and `CompletionHandlerTest` coverage green). - [x] The `ChildrenOfParityTest` golden is unchanged (the enumeration surface is untouched). - [x] The shared facade is reused; construction routes through the S2.2 `DelegatingSymbolSource` instance in `Server.php`. - [x] `composer test` green (PHPStan + tests + PHPCS). ### Not in this slice - The §4.2 enforcement rule forbidding direct `SymbolIndex` / `ClassRepository` / `NamespaceCatalog` names outside a backend lands in **S2.6** (scoped-exempt), once the remaining consumers (S2.4, S2.5) have migrated. ### Candidate closes (pending review verification) None — the manifest lists no `Closes` for S2.3. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
4 tasks
Firehed
added a commit
that referenced
this pull request
Jul 28, 2026
## Slice S2.4 — Migrate `SymbolResolver` class lookups to `lookupClassLike` **Slice:** S2.4 (`docs/architecture/build-manifest.md`, Wave 1) **Plan step:** `docs/architecture/0002-execution-plan.md` Step 2 (§5.5 consumer-migration table, `SymbolResolver` row; §5.2 interface JIT) — a consumer migration onto the S2.1 seam, no behavior change. **Depends on:** S2.1 (`SymbolSource` / `SymbolSink` + delegating facade, #374) — merged. **RFC sections:** 0001 §4.2 (Symbol Discovery Authority), §4.4 (FQN-based knowledge queries), §4.5 (type-graph traversal), §5.1 (typed identifiers). Routes `SymbolResolver`'s class knowledge through the `SymbolSource` seam instead of naming `ClassRepository` directly. The nine `ClassRepository::get()` sites (the position predicates `isInstantiable` / `isInterface` / `isThrowable` / … and the `self`/named class resolution) now call `SymbolSource::lookupClassLike`; `SymbolResolver` is constructed with the one shared `DelegatingSymbolSource` facade (wired in `Server.php` since S2.2). The facade construction moves above the resolver in `Server.php`, which is safe — the facade depends only on the repositories/index, never on the resolver. ### The `isSubclassOf` decision `SymbolResolver` also called `ClassRepository::isSubclassOf` at two sites (`isThrowable` and protected-visibility resolution) — a **transitive** subtype query that a single `lookupClassLike` cannot answer. The Step-2 illustrative interface (§5.2) lists only three methods, but §5.2 scopes the seam to "the surface today's migrated features actually need," and this migrated feature needs the subtype query. Leaving it on `ClassRepository` would keep `SymbolResolver` naming the repository directly, defeating the migration and forcing an extra exemption in S2.6's §4.2 rule (which is scoped to `FunctionRepository` only). So the seam gains `isSubclassOf`, implemented in `DelegatingSymbolSource` by pure delegation to `ClassRepository::isSubclassOf` — the same strangler pattern as `lookupClassLike`, no behavior change. `SymbolResolver` now names `ClassRepository` **nowhere**. ### Behavior preservation Both new seam calls delegate to the exact same backing methods, so no result changes. The frozen `ClassLikeLookupParityTest` golden — which covers **both** `get()` and `isSubclassOf` (`testIsSubclassOfTraversesTheGraph`) — is unchanged, and the full `SymbolResolver`, Hover, Definition, SignatureHelp, and Completion suites stay green. A new `DelegatingSymbolSourceTest` case pins the facade's `isSubclassOf` forwarding (both truth branches, and argument order). Test wiring for the five suites that construct `SymbolResolver` is de-duplicated behind a shared `BuildsSymbolSourceTrait` rather than re-assembling the facade by hand in each. ### Acceptance criteria - [x] `SymbolResolver` reads class-like lookup through `SymbolSource::lookupClassLike`, no longer naming `ClassRepository` directly. - [x] The transitive subtype check reads through the seam (`SymbolSource::isSubclassOf`, delegating to today's `ClassRepository::isSubclassOf`); `SymbolResolver`'s direct `ClassRepository` dependency is removed entirely. - [x] Behavior identical — the `ClassLikeLookupParityTest` golden (lookup + subtype) is unchanged and green. - [x] Full `composer test` green. Candidate closes (pending review verification): none (manifest `Closes` is `—`). --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
4 tasks
Firehed
added a commit
that referenced
this pull request
Jul 28, 2026
## Slice S2.5 — Migrate `TextDocumentSyncHandler` onto `SymbolSink` **Slice:** S2.5 (`docs/architecture/build-manifest.md`, Wave 1) **Plan step:** `docs/architecture/0002-execution-plan.md` Step 2 (§5.5 consumer-migration table, `TextDocumentSyncHandler` row; §5.2 `SymbolSink` write interface) — a consumer migration onto the S2.1 seam, no behavior change. **Depends on:** S2.1 (`SymbolSource` / `SymbolSink` + delegating facade, #374) — merged. **RFC sections:** 0001 §4.2 (Symbol Discovery Authority), §4.3 (one write path for symbol state), §5.2. Routes the document write path through the `SymbolSink` seam instead of naming the write-side collaborators directly. `TextDocumentSyncHandler` previously assembled the double write itself — parsing the document, registering its class-likes on `ClassRepository`, and calling `DocumentIndexer` — plus removing from both on close. It now depends on a single `SymbolSink` and calls `openDocument` / `updateDocument` / `closeDocument`; the parse, the class registration, and the index write live behind the `DelegatingSymbolSource` facade (which has reproduced this exact double write since S2.1). `Server.php` wires the one shared facade instance as the sink. The handler no longer names `DocumentIndexer`, `ClassInfoFactory`, `ClassRepository`, `ParserService`, or `ScopeFinder`; `ParserService` and the class-registration traversal leave the handler entirely (they already exist inside the facade). ### Behavior preservation The facade's write path is byte-for-byte the handler's old one (same parse, same `ClassRepository::updateDocument`, same `DocumentIndexer::index`, same close order), so no observable output changes. The frozen `WritePathParityTest` golden is unchanged and green, and the full sync-handler suite — which pins class registration on open/change, class replacement on change, class removal on close, and the Step-0 single-parse invariant — stays green against the migrated handler. Test wiring for the five downstream suites that also construct the sync handler now feeds it a `DelegatingSymbolSource` (via the shared `BuildsSymbolSourceTrait`, matching production's one-facade wiring) rather than re-assembling the write collaborators by hand; `CompletionHandlerTest` feeds a facade over its real shared `SymbolIndex` so opened documents still surface in completion. ### Acceptance criteria - [x] The document write path (open / change / close) flows through `SymbolSink`. - [x] `TextDocumentSyncHandler` no longer names `DocumentIndexer`, `ClassInfoFactory`, or `ClassRepository` directly (nor `ParserService` / `ScopeFinder`). - [x] Behavior identical — the `WritePathParityTest` golden is unchanged and green. - [x] Full `composer test` green. Candidate closes (pending review verification): none (manifest `Closes` is `—`). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Slice S2.1 — Define
SymbolSource/SymbolSink+ delegating facadeSlice: S2.1 (build-manifest.md, Wave 1)
Plan step: 0002-execution-plan.md Step 2 (§5.1–§5.6) — the seam-introducing slice of Step 2
RFC sections: 0001 §4.2 (Symbol Discovery Authority), §4.3 (Read/Write Segregation), §4.4 (Positional/Knowledge separation), §5.1 (read contract), §5.2 (write contract), §5.3 (absence is a bare
null)Introduces the read/write knowledge seam over today's collaborators, with no behavior change. No consumer is migrated onto it here — this slice only defines the interfaces and the delegating facade.
What's here
Firehed\PhpLsp\Knowledge\NamespaceName— typed identifier for a namespace path (RFC §5.1 "names MUST be typed identifiers"); the global namespace is the empty path.SymbolSource(read) andSymbolSink(write) — separate interfaces per RFC §4.3; FQN-based, no cursor/AST per §4.4.DelegatingSymbolSource— implements both by pure delegation toClassRepository(lookupClassLike),SymbolIndex(searchClassLikes),NamespaceCatalog(childrenOf), and the existing write paths (DocumentIndexer+ClassInfoFactory+ClassRepository). The write path reproduces today's double write behind one method (Plan §5.5); collapsing it is Step 3a.Acceptance criteria (this slice)
SymbolSource) and write interface (SymbolSink) exist as separate interfaces (RFC §4.3).ClassRepository/SymbolIndex/NamespaceCatalogand the existing write paths (Plan §5.5).SymbolSource; document open/update/close throughSymbolSink.tests/Parity/README.md, the facade rides the goldens unchanged (goldens assert surface output; the surfaces are the concrete classes, which this slice does not modify).tests/Knowledge/); 100% line/method coverage of the new classes.composer testgreen (PHPStan + tests + PHPCS).Resolved open decisions (Plan §7)
ClassLikeName= today'sClassName, reused as-is (Plan §5.3 lean; §7 open decision). No new class-name type;lookupClassLike(ClassName)keeps the diff minimal andClassName's dual role (identity +Type) intact.SymbolDefinition= today'sSymbol, reused (Plan §5.4).searchClassLikes(): list<Symbol>.searchClassLikes— searches the class-like namespace only; a kind-parameterizedsearchand per-kind function/constant lookup arrive in Step 3b (Plan §5.3).Firehed\PhpLsp\Knowledge— grounded in the RFC's own vocabulary (§3.2 "Knowledge layer (SymbolSource)", §5.1 "Symbol Knowledge").Deferred (later slices, per the manifest)
FunctionRepository) → S2.6, after the consumer migrations.ClassCandidates,NamespaceCandidates,SymbolResolverclass lookups,TextDocumentSyncHandler) → S2.2–S2.5.Server.phprides the migration slices, so no unused service is constructed here.TextDocumentSyncHandler::registerDocumentClassesis the ledger's "Step 2 facade hides today's double write" scaffold; the handler's copy is removed when it migrates ontoSymbolSink(S2.5).This PR body was written by AI and reviewed by a human.