Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves symbol-coordinate refresh and documentation consistency around symbols.yaml, ensuring coordinates can be generated for additional TypeScript declaration shapes and preventing stale/duplicate symbol aliases from persisting in docs.
Changes:
- Extend TS coordinate matching to fall back to unique non-exported top-level functions and unique class methods when no exported declaration matches.
- Add regression tests across CLI, MCP, and VS Code extension to validate indexing, CodeLens placement, and coordinate refresh behavior.
- Update
documentation/symbols.yamlto remove a stale alias and refresh generated coordinate fields; add a CLI test to prevent regressions.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/vscode/tests/symbolIndex.test.ts | Adds tests for indexing symbols that represent internal helpers and class methods. |
| packages/vscode/tests/codeLens.test.ts | Adds CodeLens placement tests for internal helpers and class-method symbols. |
| packages/mcp/tests/tools/symbols.test.ts | Adds regression coverage for coordinate refresh on exported, non-exported, and class-method symbols. |
| packages/cli/tests/extractors/symbols-ts.test.ts | Adds matcher guardrail tests for exported-vs-internal priority, ambiguity fail-closed behavior, and unsupported shapes. |
| packages/cli/tests/documentation-consistency.test.ts | Adds regression assertions to prevent stale symbol aliases/duplicate titles in docs. |
| packages/cli/tests/commands/sync.test.ts | Adds an end-to-end sync regression test ensuring failed=0 for the newly supported symbol shapes. |
| packages/cli/src/extractors/symbols-ts.ts | Implements the fallback matching logic for internal functions and class methods. |
| documentation/symbols.yaml | Removes stale symbol alias entry and updates generated coordinate fields/timestamps. |
| .changeset/add-fallback-kibi-cli.md | Declares a patch release for kibi-cli reflecting the coordinate-refresh enhancement. |
| * `bar`, the matcher MUST return no coordinates (fail closed). Resolving | ||
| * ambiguously to one of them would produce silently wrong coordinates. | ||
| * | ||
| * RED PHASE: The current scanner ignores non-exported declarations, so it | ||
| * returns nothing today. This test locks that behaviour so that if the | ||
| * scanner is later extended to consider internal declarations, it must also | ||
| * implement the fail-closed rule before coordinates are emitted. |
There was a problem hiding this comment.
This docblock says the scanner "ignores non-exported declarations" and that internal resolution is a future extension, but the matcher now explicitly scans non-exported top-level functions (and fails closed on ambiguity). Please update the wording so it matches the current implementation and what the test is actually asserting.
| * `bar`, the matcher MUST return no coordinates (fail closed). Resolving | |
| * ambiguously to one of them would produce silently wrong coordinates. | |
| * | |
| * RED PHASE: The current scanner ignores non-exported declarations, so it | |
| * returns nothing today. This test locks that behaviour so that if the | |
| * scanner is later extended to consider internal declarations, it must also | |
| * implement the fail-closed rule before coordinates are emitted. | |
| * `bar`, the matcher MUST return no coordinates (fail closed). Resolving | |
| * ambiguously to one of them would produce silently wrong coordinates. | |
| * | |
| * 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. |
| if (method.getName() !== title) continue; | ||
| const nameNode = method.getNameNode(); | ||
| if (!nameNode) continue; | ||
| methodCandidates.push({ node: method as unknown as NamedDeclarationCandidate, getNameNode: () => nameNode }); |
There was a problem hiding this comment.
methodCandidates.push uses an as unknown as NamedDeclarationCandidate cast, but NamedDeclarationCandidate already includes Node, so the cast is redundant and weakens type-safety/readability. Prefer removing the cast (and/or tightening NamedDeclarationCandidate to the specific node types you actually handle).
| methodCandidates.push({ node: method as unknown as NamedDeclarationCandidate, getNameNode: () => nameNode }); | |
| methodCandidates.push({ node: method, getNameNode: () => nameNode }); |
| * RED PHASE: The current implementation only scans exported declarations. | ||
| * When an internal-then-exported pair exists, if the scanner is ever | ||
| * extended to look at internals first, this test locks the priority rule. |
There was a problem hiding this comment.
The comment says "RED PHASE: The current implementation only scans exported declarations", but this PR adds fallback scanning for unique non-exported functions and class methods in findNamedDeclaration. Please update this test docblock to reflect the current behavior (e.g., exported pass first, then internal/method fallback).
| * RED PHASE: The current implementation only scans exported declarations. | |
| * When an internal-then-exported pair exists, if the scanner is ever | |
| * extended to look at internals first, this test locks the priority rule. | |
| * The matcher now prefers exported declarations first, and only falls back | |
| * to unique internal functions or class methods when no matching export | |
| * is found. When an internal-then-exported pair exists, this test locks | |
| * in the priority rule that the exported declaration must win. |
|
@copilot apply changes based on the comments in this thread |
Applied all three changes from the review:
Changes are in commit |
|
@copilot apply changes based on the comments in this thread in a separate PR. |
…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>
fix(cli): remove stale RED PHASE docblocks and redundant cast in symbol matcher
exportedWinsOverInternaldocblock insymbols-ts.test.ts— replaced with accurate description of the live exported-first priority ruleambiguousInternalTitlesFailCloseddocblock insymbols-ts.test.ts— replaced with accurate description of the now-live fail-closed behaviour for ambiguous internal declarationsas unknown as NamedDeclarationCandidatedouble-cast onmethodCandidates.pushinsymbols-ts.ts—MethodDeclarationalready satisfiesNodewhich covers the union type🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.