fix(cli): remove stale RED PHASE docblocks and redundant cast in symbol matcher#123
fix(cli): remove stale RED PHASE docblocks and redundant cast in symbol matcher#123
Conversation
…nt cast Agent-Logs-Url: https://github.com/Looted/kibi/sessions/b3910a2c-8deb-4e7a-bdf1-1706eca63b17 Co-authored-by: Looted <6255880+Looted@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Cleans up the TypeScript symbol coordinate matcher guardrail tests and matcher implementation by removing draft-phase language and simplifying a now-unnecessary type cast, keeping the documentation and code aligned with the current exported-first matching behavior.
Changes:
- Updated two test docblocks to describe the current exported-first and fail-closed behaviors (removing stale “RED PHASE” wording).
- Removed a redundant
as unknown ascast when recording class method candidates in the TS symbol matcher.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/cli/tests/extractors/symbols-ts.test.ts | Updates docblocks to reflect current matcher behavior and priorities. |
| packages/cli/src/extractors/symbols-ts.ts | Removes redundant cast when pushing method candidates. |
| * The matcher now scans non-exported top-level functions, but when multiple | ||
| * internal declarations share the same title and there is no exported | ||
| * declaration to disambiguate, it must treat the match as ambiguous and | ||
| * emit no coordinates. This test locks in that fail-closed behaviour. |
There was a problem hiding this comment.
The updated docblock says that when multiple internal declarations share the same title (and no export exists) the matcher must emit no coordinates. In the current implementation, ambiguity among non-exported top-level functions can still fall through to the class-method scan, which may return coordinates if there is exactly one matching method. Consider tightening the wording to explicitly describe ambiguity handling for non-exported top-level functions (and/or mention the method fallback) so the comment matches the actual priority rules.
| * The matcher now scans non-exported top-level functions, but when multiple | |
| * internal declarations share the same title and there is no exported | |
| * declaration to disambiguate, it must treat the match as ambiguous and | |
| * emit no coordinates. This test locks in that fail-closed behaviour. | |
| * The matcher now scans non-exported top-level functions before falling | |
| * back to class or instance methods. When multiple non-exported top-level | |
| * declarations share the same title and there is no exported declaration | |
| * to disambiguate, that top-level match is treated as ambiguous and MUST | |
| * emit no coordinates, even if a unique method with that title exists. | |
| * This test locks in that fail-closed behaviour for top-level functions. |
Three review comments on the symbol coordinate matcher that were left over from the planning/draft phase: two test docblocks still described hypothetical future behavior using "RED PHASE" language, and one push call carried a double-cast that became redundant once the type union was settled.
Changes
symbols-ts.test.ts—exportedWinsOverInternaldocblock: Replaced "RED PHASE: The current implementation only scans exported declarations" with accurate present-tense description of the live exported-first priority rule.symbols-ts.test.ts—ambiguousInternalTitlesFailCloseddocblock: Replaced "RED PHASE: The current scanner ignores non-exported declarations" with accurate description of the now-live fail-closed behaviour when multiple internal declarations share the same title.symbols-ts.ts:248— redundant cast removed:MethodDeclarationextendsNode, which is already a member of theNamedDeclarationCandidateunion — theas unknown asescape hatch was unnecessary.⌨️ Start Copilot coding agent tasks without leaving your editor — available in VS Code, Visual Studio, JetBrains IDEs and Eclipse.