Skip to content

fix(ast): Kotlin walker tests read valid Kotlin; tree-sitter-language-pack <1.24 - #939

Merged
github-actions[bot] merged 2 commits into
mainfrom
fix/ci-kotlin-grammar-language-pack-1-23
Nov 7, 2026
Merged

github-actions[bot] merged 2 commits into
mainfrom
fix/ci-kotlin-grammar-language-pack-1-23

Conversation

@cdeust

@cdeust cdeust commented Nov 7, 2026

Copy link
Copy Markdown
Owner

Summary

Unblocks #932 (Dependabot minor-and-patch group). That PR bumps tree-sitter-language-pack 1.22.1 → 1.23.1 and unit fails on two TestKotlin tests (run 42350563661, job 128337627651):
test_full_definition_set_in_document_order (('O', 'class') != ('o', 'function'), A.m missing) and test_object_declaration_emits_no_type_entry.

Root cause: the test fixture was not valid Kotlin. class A { fun m() {} class B { ... } } puts two declarations on one line with no newline or ;, so tree-sitter returns an ERROR node and the walker sees whatever error recovery produced. That recovery differs between grammar releases. The tests were pinning the grammar's recovery, not the walker's contract. Separately, the 1.22.1 grammar could not parse a bare object O { ... } (it came out as infix_expression + ERROR). That is the "surprise" one test pinned; 1.23.1 fixes it.

Measured locally with both releases (fresh venvs, same walker):

source 1.22.1 1.23.1
old one-line fixture has_error=True; o function, A.m, A.deep has_error=True; O, O.o, no A.m, A.deep
valid fixture (this PR) has_error=True (bare object) ; o function has_error=False; O, O.o, A, A.m, B, B.deep

Change:

  • The fixture is now valid Kotlin (members of A on their own lines). The new test test_fixture_is_valid_kotlin asserts the grammar parses it with no error, so recovery can't creep back into the expectations.
  • test_object_declaration_emits_no_type_entry becomes test_bare_object_declaration_emits_a_type_entry: O and O.o are reported.
  • The pin goes to <1.24. The uv.lock entry is set to 1.23.1, the same package block Dependabot resolved in deps: bump the minor-and-patch group with 4 updates #932. uv lock --check --offline passes.

No production code changes: _walk_kotlin was already right for valid source.

Refs #932. After this merges, Dependabot's rebase of #932 drops the language-pack part of its group.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Test plan

  • pytest tests_py/core/test_ast_extractor_walkers.py test_ast_extractor_depth.py test_wiki_stub_detector.py test_ast_extractors_multilang.py test_ast_extractor_imports.py test_ast_parser_language_contract.py with tree-sitter-language-pack 1.23.1: 97 passed.
  • ruff check / ruff format --check on the touched test file: clean. Repo-wide ruff check reports 8 E501 in scripts/ and tests_py/scripts/, and the same 8 show on origin/main without this change, so the local ruff probably differs from CI's.
  • python scripts/check_craftsmanship.py: OK.
  • uv lock --check --offline: consistent.

Not verified locally: the full suite and pyright. uv sync can't reach download.pytorch.org from this environment, so I installed the locked dependencies without torch/sentence-transformers. In #932's run, typecheck passed with 1.23.1 in the lock, and unit failed only on these two tests.

Audit notes

  • Autopilot routine, no human review yet.
  • Outstanding: none.

Coding-standards compliance

  • §2.2 Layer dependency direction preserved (tests only).
  • §9 No dead code.

Reviewer checklist

  • CHANGELOG.md updated under the appropriate section.
  • No secrets / credentials / PII in the diff.
  • CI passes on the latest commit (pending).

Generated by Claude Code

@cdeust
cdeust marked this pull request as ready for review November 7, 2026 14:25
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 7, 2026 •

Copy link
Copy Markdown
Codex Review Summary: ✅ Completed

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-07T14:32:48.226482Z cef9222 New commits
🔒 Security Review ✅ Completed 2026-11-07T14:34:15.784657Z cef9222 New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

…-pack <1.24

The TestKotlin fixture `class A { fun m() {} class B { ... } }` is not valid
Kotlin (declarations need a newline or `;`), so its expected definitions were
the grammar's error recovery, not the walker's contract. Bumping
tree-sitter-language-pack 1.22.1 -> 1.23.1 (#932) changed that recovery and
`unit` failed on test_full_definition_set_in_document_order and
test_object_declaration_emits_no_type_entry (run 42350563661, job
128337627651).

The fixture is now valid Kotlin and a test asserts it parses without error.
Under 1.23.1 a bare `object O { ... }` is an `object_declaration` with a
`type_identifier`, so O and O.o are reported; the test that pinned the 1.22.1
behaviour (no entry for O) now pins the correct one. The pin moves to <1.24
and uv.lock to 1.23.1 (the same entry Dependabot resolved in #932).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018kyZYqc5kJqnz7haDpUyug
@cdeust
cdeust force-pushed the fix/ci-kotlin-grammar-language-pack-1-23 branch from 17bb517 to 61cbc19 Compare November 7, 2026 14:28
@claude

claude Bot commented Nov 7, 2026 •

Copy link
Copy Markdown

Claude finished @cdeust's task in 1m 46s —— View job


PR #939 review (1 of 3, ADR-1119)

  • Read the diff
  • Ran the five reviews: code quality, documentation, performance, security, tests
  • Checked the pin, the lock and CI myself
  • Posted the verdict

No Blockers.

  • Walker output matches: I traced _walk_kotlin (mcp_server/core/ast_extractors_jvm.py:93-126) on the new fixture. It produces I, I.x, O, O.o, A, A.m, B, B.deep, top, in the same order that test_full_definition_set_in_document_order expects. B is not qualified by A because the walker resets the qualifier for nested types, as the module docstring says.
  • Pin and lock agree: pyproject.toml:135 and the lock's requires-dist (uv.lock:1487) both say <1.24. The 1.23.1 block lists a sha256 for the sdist and all six wheels, all hosted on files.pythonhosted.org. I did not compare those hashes against PyPI because web access was denied here; uv sync --locked in CI would catch a mismatch.
  • CI cost: the new test gets its parser the same way _defs does, and CI already prefetches the Kotlin grammar. The new pyproject.toml hash makes the grammar cache miss once, on the first CI run after this change.

Notes, no action before merge

  • tests_py/core/test_ast_extractor_walkers.py:19-28: the module docstring still lists "Kotlin object O { ... } emits no entry for O" as one of "Three" pinned surprises. This PR reverses that, so the bullet should go and the count should become two. Fix this →
  • pyproject.toml:135 still allows >=1.12.5, which includes 1.22.x. By this PR's own docstring, 1.22.x fails the new Kotlin tests. CI resolves from uv.lock (1.23.1), so CI is not affected.
  • The bump changes every grammar in the pack, and seven other tree-sitter test files were not run locally (for example test_ast_parser_extra_langs.py and test_ast_extractor_definitions.py). The Pipeline run on this commit covers them and was still in progress when I posted this.

I could not post this with gh pr comment because the gh command was not approved in this run, so the review is in this comment instead.

Verdict: approve

cdeust commented Nov 7, 2026

Copy link
Copy Markdown
Owner Author

Replies to the review (1 of 3):

  • Stale docstring: fixed in cef9222 (fixup! on 61cbc19). I removed the Kotlin bullet and changed "Three" to "Two"; test_ast_extractor_walkers.py passes (25 passed).
  • The >=1.12.5 floor admits 1.22.x: left unchanged. ADR-0874 defines that floor as the API requirement (get_parser(...).parse(bytes)), not output fidelity. CI installs from uv.lock (1.23.1). Raising the floor to >=1.23 is an owner decision on what the floor means, outside this fix.
  • Other grammars: covered by the Pipeline on 17bb517, where unit, regression and e2e were green before the rebase. The Pipeline is running again on cef9222.

Generated by Claude Code

@claude

claude Bot commented Nov 7, 2026 •

Copy link
Copy Markdown

Claude finished @cdeust's task in 1m 0s —— View job


PR #939 review (2 of 3, ADR-1119)

  • Checked review 1 for Blockers. It raised none, so there was nothing to re-verify.
  • Found the commits pushed after 61cbc19: only cef9222 (fixup). git show --stat shows it changes only the module docstring of tests_py/core/test_ast_extractor_walkers.py (+1/−3).
  • Ran the five reviews on cef9222: code quality, documentation, performance, security, tests
  • Posted the verdict

No Blockers.

cef9222 applies review 1's docstring note. The docstring now says "Two measured behaviours" and no longer has the Kotlin bare-object bullet. Each of the two remaining surprises still has a pinning test:

  • Swift enum/struct reported as class: test_enum_and_struct_report_class_because_the_grammar_says_so, tests_py/core/test_ast_extractor_walkers.py:307.
  • Ruby def self.name not emitted: test_singleton_method_is_not_emitted, :261.

The Kotlin expectations at :167-168 and :195-196 (O as class, O.o as method) match the docstring, _walk_kotlin (mcp_server/core/ast_extractors_jvm.py:100-115) and the CHANGELOG entry. The commit has no runtime or security impact. I did not re-audit the unchanged code from 61cbc19.

Notes, no action before merge

  • tests_py/core/test_ast_extractor_walkers.py:199: in test_object_with_a_supertype_does_emit_a_type_entry, the "does" contrasts with bare objects, but bare objects now emit an entry too. test_object_with_a_supertype_emits_a_type_entry would read better.

I could not post this with gh pr comment because the gh command was not approved in this run (gh pr checks was blocked the same way), so the review is in this comment instead. I did not check CI on cef9222.

Verdict: approve

cdeust commented Nov 7, 2026

Copy link
Copy Markdown
Owner Author

Reply to review 2 of 3: the rename test_object_with_a_supertype_emits_a_type_entry is a naming note marked "no action before merge". It stays as it is unless another push is needed, in which case it goes into that push. A push just for this would restart the CI and the three reviews.


Generated by Claude Code

@github-actions
github-actions Bot merged commit 8042bcc into main Nov 7, 2026
18 checks passed
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