Skip to content

fix(discover): keep JVM packages named like build dirs (part of #2661) - #2724

Merged
DeusData merged 3 commits into
DeusData:mainfrom
tmonestudio:fix/java-skip-dirs-and-typed-calls
Oct 27, 2026
Merged

DeusData merged 3 commits into
DeusData:mainfrom
tmonestudio:fix/java-skip-dirs-and-typed-calls

Conversation

@tmonestudio

@tmonestudio tmonestudio commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

What does this PR do?

Part of #2661.

JVM packages named like build output (#2661). ALWAYS_SKIP_DIRS matches a directory by basename anywhere in the tree, so src/main/java/com/acme/target/Foo.java was never indexed and calls into it bound to whatever else shared the name. Below src/<set>/{java,kotlin,scala,groovy}/ every directory is a package, so a built-in skip name there (target, tmp, temp, vendor, deploy, env, dist, obj, coverage) is now kept.

  • Only directories strictly below the language root qualify: target/classes, src/main/target and src/main/java itself keep the existing behavior.
  • The safety core (.git, worktrees) is never un-skipped.
  • The same predicate (cbm_discover_path_in_jvm_package) is applied in both places that consult the built-in list: directory_verdict in discovery and cif_directory_reason in the inventory filter.
  • In moderate and fast mode the exception also covers the fast-mode skip list (generated, gen, tools, integration, examples, docs, ...): below a JVM source root those names are packages too, so they are kept (e.g. src/main/java/com/acme/integration/I.java), while the same directory at the top level (integration/) stays skipped. This is pinned by a CBM_MODE_MODERATE case in discover_keeps_jvm_packages_named_like_build_dirs_issue2661. Users who want the fast tiers to stay lean there can exclude such a package with a .cbmignore pattern.

The plain layout from the #2382 reproducer (src/com/example/target/TargetDTO.java, without the main/java level) is still skipped, so #2661 stays open for that case.

The GLSL stage mapping that was in this PR moved to its own PR, as requested.

Note on #2382

While reproducing #2382 I found that on current main the typed-parameter case resolves correctly on the parallel pipeline path and fails only on the sequential one (small repos). #2667 builds the Java cross registry on the sequential path too; with it merged, the #2382 reproducer resolves correctly on both paths. So this PR does not touch call resolution. Details in a comment on #2382.

Checklist

  • Every commit is signed off (git commit -s)
  • Rebased on current main
  • Tests: the earlier head passed discover, language, inventory_filter, gitignore, gitignore_checked, workspace locally (445/0/4 skipped); the new moderate-mode case has not been run locally, it relies on CI
  • Lint: not run (cppcheck/clang-format not installed here)
  • New behavior is covered by tests: jvm_package_path_predicate_issue2661, discover_keeps_jvm_packages_named_like_build_dirs_issue2661

End to end with the built binary: in a Maven layout with com/acme/target/Foo.java, Foo.java is now indexed and f.ping() binds to com.acme.target.Foo.ping. target/classes/*.java at the module root stays skipped.

🤖 Generated with Claude Code

@tmonestudio
tmonestudio requested a review from DeusData as a code owner October 20, 2026 14:47
@github-actions

Copy link
Copy Markdown

Thanks for opening this — it has been seen, and it is queued.

This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence.

Current review status: working through a backlog. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

  • It will not be closed for inactivity. No stale bot touches pull requests here.
  • It may still sit a while before a human reads it. That is on us, not on you.
  • Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

  • Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
  • Get CI green, or say which failures you believe are pre-existing.
  • Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
  • Every commit needs a sign-off (git commit -s) — CI enforces DCO.

If this fixes a bug, a reproduction we can run is worth more than a description of the symptom.

Thanks for contributing, and sorry in advance for the wait.

@DeusData DeusData left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thank you, @tmonestudio, this is a really nice fix, and thank you for the careful description. You found the real cause of #2661 (the basename match against ALWAYS_SKIP_DIRS), you kept the exception tight (only strictly below src/<set>/<lang>/, never the language root or its siblings, never the safety core), and you applied the same predicate in discovery and in the inventory filter so the two agree. The tests cover both the predicate and a real discovery walk, including the target/classes and .git cases that must stay skipped.

We built your branch and tried it on a real repository. In elasticsearch, the core org.elasticsearch.env package (22 files: Environment, NodeEnvironment, ShardLock and friends) is never indexed on main. With your change it is, and 1,465 calls that had no target before now bind into it. clang-format and cppcheck 2.20 (the version CI uses) are clean on your changed source files. The CI run on this head was cancelled by a hang in our checkout step, not by anything in your change; it runs again on your next push.

A few things before we can merge:

  1. Please move the GLSL commit (feat(language): map GLSL compute, geometry and tessellation stages) into its own PR. It is a good change, but it is a separate feature from #2661, and we keep one claim per PR so each can be reviewed, reverted and bisected on its own. It is already its own commit, so this should be quick, and we are happy to review the GLSL PR right away. One heads-up for it: we indexed a small compute, geometry and tessellation shader with your mapping, and the files now index, but our vendored GLSL grammar reads the qualifier-only layout(...) in; / layout(...) out; lines as parse errors, so a definition right after one of them can be missed. That is a limitation on our side, not something your PR needs to fix.

  2. Please change Fixes #2661 to Part of #2661. The issue's own reproducer (from #2382) keeps its class at src/com/example/target/TargetDTO.java, a plain layout without the main/java level, and with this change that package is still skipped (we checked with your branch). Keeping the issue open lets that layout be handled separately; if you would like to take it on in a follow-up PR, you are very welcome to.

  3. In moderate and fast mode, cbm_should_skip_dir also applies the fast-mode skip list (generated, gen, tools, integration, examples, docs, ...), so your exception keeps those package names too when they sit below a JVM source root. We think that is the right behavior, since they are packages there as well (in elasticsearch that is about 470 more files in moderate mode, for example org.elasticsearch.xpack.core.ml.integration), but it is not mentioned yet. Could you add a sentence about it to the description, and pin it with a CBM_MODE_MODERATE case in discover_keeps_jvm_packages_named_like_build_dirs_issue2661, for example a src/main/java/com/acme/integration/I.java that is kept while a top-level integration/ stays skipped? Users who want the fast tiers to stay lean there can already exclude such a package with a .cbmignore pattern (and a !dir/ negation re-includes a built-in skip), so no new setting is needed.

Thanks again. This closes a real hole in how Java and Kotlin projects are indexed.

tmonestudio and others added 2 commits October 21, 2026 22:56
…ta#2661)

Below src/<set>/{java,kotlin,scala,groovy}/ every directory is a package,
so a built-in skip name there (target, tmp, vendor, deploy, env, dist...)
is a package name, not build output. Discovery and the inventory filter
now keep such directories; the root itself, its siblings and the safety
core (.git, worktrees) stay skipped.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Leonardo trindade miranda <tmonestudio@gmail.com>
Below a JVM source root, a package named like a fast-mode skip dir
(integration/) is kept in moderate mode, while a top-level integration/
stays skipped.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: mbrl <mbrl@local>
@tmonestudio
tmonestudio force-pushed the fix/java-skip-dirs-and-typed-calls branch from 08b1267 to d57846c Compare October 22, 2026 01:57
@tmonestudio tmonestudio changed the title fix(discover): keep JVM packages named like build dirs (#2661); map GLSL stages fix(discover): keep JVM packages named like build dirs (part of #2661) Oct 22, 2026
@tmonestudio

Copy link
Copy Markdown
Contributor Author

Thanks for the review. Done: (1) the GLSL commit is now its own PR, #2761, with the grammar limitation noted; (2) the description now says Part of #2661; (3) the description now covers the moderate/fast-mode behavior, and discover_keeps_jvm_packages_named_like_build_dirs_issue2661 has a CBM_MODE_MODERATE case (src/main/java/com/acme/integration/I.java kept, top-level integration/ skipped). The branch is rebased on current main.

@DeusData DeusData added bug Something isn't working parsing/quality Graph extraction bugs, false positives, missing edges labels Oct 25, 2026
…eusData#2661)

The Java and Kotlin doc-link reason tests modelled "a package in a
directory discovery skips" with src/main/java/a/env/ and
src/main/kotlin/k/env/. Discovery now keeps packages below
src/<set>/<lang>/, so those files are indexed and the two not_indexed
assertions failed. Move both fixtures to a plain src/<pkg>/ layout,
which discovery still skips by name (the open part of DeusData#2661), and
reword the comments. Assertions unchanged.

Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
@DeusData

Copy link
Copy Markdown
Owner

Thank you, @tmonestudio, for working through all three points so carefully. The GLSL stage mapping now stands on its own and is merged as #2761, the description says exactly what the change does (Part of #2661, plus the moderate/fast-mode behavior), and the new CBM_MODE_MODERATE case in discover_keeps_jvm_packages_named_like_build_dirs_issue2661 pins that behavior. The production change is the same one we measured on elasticsearch, so nothing more is needed there.

The remaining red CI was not caused by a problem in your fix but by two of our older tests, so we pushed one small commit on top of your branch (f05ecce):

  1. test(doc-links): move JVM not_indexed fixtures to a skipped layout (A Java package named target, tmp, deploy or vendor is silently not indexed #2661). The Java and Kotlin doc-link tests in tests/test_doc_mentions_jvm.c need "a package in a directory discovery skips", and they modelled it with src/main/java/a/env/ and src/main/kotlin/k/env/. That is exactly the layout your change now correctly indexes, so the two not_indexed assertions failed. We moved both fixtures to a plain src// layout (src/a/env/Config.java and src/k/env/Config.kt), which discovery still skips by name (the open part of A Java package named target, tmp, deploy or vendor is silently not indexed #2661), and reworded the comments to match. The assertions are unchanged, and doc_mentions_jvm and discover pass locally with it.

Since we pushed to your branch, please pull before pushing again. Thanks again for closing this hole in how Java and Kotlin projects are indexed.

@DeusData
DeusData merged commit 4edc6b7 into DeusData:main Oct 27, 2026
26 checks passed
@DeusData

Copy link
Copy Markdown
Owner

Merged as 4edc6b7 - thank you, @tmonestudio! Java and Kotlin packages whose directory names look like build folders are no longer skipped, which closes a real hole in how JVM projects are indexed (part of #2661). Thank you for working through every review point so carefully.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working parsing/quality Graph extraction bugs, false positives, missing edges

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants