Integrate 27 reviewed PRs: false-positive fixes, runner fixes, and one orphan-detector fix - #744
Open
awdemos wants to merge 61 commits into
Open
Integrate 27 reviewed PRs: false-positive fixes, runner fixes, and one orphan-detector fix#744awdemos wants to merge 61 commits into
awdemos wants to merge 61 commits into
Conversation
Kotlin resolves some imports by convention rather than by name, so the imported
symbol never appears in the file body:
import androidx.compose.runtime.getValue
import androidx.compose.runtime.setValue
...
var expanded by remember { mutableStateOf(false) }
The identifier search in detect_unused_imports never sees "getValue", so every
Compose file using property delegation is reported as having 2 unused imports.
Acting on the finding breaks the build. On a Compose Multiplatform project this
was 37 of 73 unused-import findings — over half the detector's output.
Adds a TreeSitterLangSpec.implicit_import_uses field: (name_pattern, body_pattern)
pairs declaring per-language conventions where a matching body pattern means the
import is used. Kotlin declares getValue/setValue/provideDelegate (guarded on the
`by` keyword) and componentN (guarded on destructuring declarations). Other
languages are unaffected — the field defaults to empty.
Genuinely dead delegation imports are still flagged: the guard requires the
convention's syntax to actually be present in the file.
The Kotlin plugin declared no zone_rules, so it fell back to the common rules, which look for a '/test/' or '/tests/' path segment. Kotlin projects are laid out by Gradle source set instead, and Kotlin Multiplatform names every test source set '<target>Test': shared/src/commonTest/kotlin/... sync/src/jvmTest/kotlin/... composeApp/src/androidUnitTest/kotlin/... None contain '/test/', so every test file was zoned production. On a KMP project that put all 241 files in the production zone, with 64 test files reported as orphaned (a test file has no importers by definition) and hardcoded test credentials raised as production security findings. Adds desloppify/languages/kotlin/_zones.py covering KMP and plain Gradle/Android test source sets, marking Gradle build scripts as config, and wires it into the plugin via zone_rules.
The kotlin plugin declared the ktlint tool with fmt="json", which maps
to the generic parse_json parser. That parser expects a flat list of
objects with top-level file/line/message keys (like most linters), but
ktlint's --reporter=json output nests violations per file:
[{"file": ..., "errors": [{"line", "column", "message", "rule"}]}]
Because parse_json looked for top-level "line"/"message" keys that
don't exist on ktlint's file objects, every entry was silently dropped,
so ktlint_violation always produced zero entries despite ktlint exiting
non-zero with real violations. This surfaced as a permanent
'Coverage reduced (ktlint_violation): ... tool_failed_unparsed_output'
warning and meant Kotlin projects never got real ktlint findings from
desloppify.
Add a dedicated parse_ktlint parser that understands the nested
file/errors shape, register it in PARSERS, and switch the kotlin plugin
to fmt="ktlint". Verified against a real multi-module Kotlin project:
ktlint_violation coverage is no longer reduced and the mechanical
'Code quality' score reflects real lint findings.
The csharp plugin discovered only `.cs`, so `.razor` and `.cshtml` were invisible. Code reachable only from markup therefore looked unreferenced: a `Widget.razor.cs` code-behind partial is reported as an orphaned file with zero importers and a suggestion to delete it, even though deleting it breaks the build. Views are parsed for edges but stay out of the scored extension set, mirroring how the typescript plugin already handles `.svelte`, `.vue` and `.astro`. Scores for existing projects are unchanged. Edges resolve by symbol name rather than by namespace. A view's `@using` says which namespaces are in scope, not which files it depends on, so linking a whole namespace would mark every file in it as live and hide genuinely dead code. Resolved per view: - types the view names, via a type-name index - extension methods it calls, which name no type at the call site - its own code-behind partial (`Widget.razor` -> `Widget.razor.cs`) - components it renders, including components declared in plain C# - partials and layouts referenced by string name - view components and tag helpers, which resolve by naming convention - `@page` marks a view as a routable root, like `Program.cs` - `_Imports.razor` and `_ViewImports.cshtml` usings apply to the subtree Also zones the ambient import files as config and `.razor.g.cs` / `.cshtml.g.cs` as generated, and stops `build_dep_graph` returning early on a project that is all views and no `.cs`. Measured on a ten-project Razor Pages solution: 334 views enter the graph, contributing 1052 edges across 200 `.cs` files, with 136 routable pages marked as roots. No file changed orphan status on that codebase, because the existing namespace matching already linked them.
ts_build_dep_graph discarded every import edge when file_list used relative paths, which made every file in the project look orphaned. resolve_import returns a path in the same space as the source_file it is handed, so a relative file_list produces relative results. The builder then unconditionally joined those onto the absolute scan_path and tested membership against the relative file_set, so the lookup never matched. Reproduced on a real React Router project: same file set, relative file_list gives 0 edges, absolute gives 142, and a db.server.js with 15 importers was reported as having zero. Running the scanner over that project, orphaned findings drop from 49 to 24 and code quality goes from 42.8% to 72.0%. Resolved paths are now matched against the file set in whichever space it uses, with the previous scan_path-relative interpretation kept as a fallback. The existing test only covered an absolute file_list with a scan_path-relative resolver, which is the one combination that worked. Added a regression test for the relative case.
Route modules and framework entry points have no importers by design -- the file-based router loads them from the filesystem -- so every one of them is reported as an orphaned file on every project of this shape. Mirrors the existing Next.js App Router handling rather than adding a new mechanism: detect the framework at the scan root, then exempt its convention files. Detection prefers a react-router.config.* or remix.config.* file and falls back to checking package.json for @react-router/* or @remix-run/*, because the framework's own template ships a Vite config rather than a react-router.config. Exempted: anything beneath an app/routes/ or src/routes/ directory, plus root, entry.client and entry.server beside it. An ordinary module such as app/db.server.js is deliberately still reported -- a genuinely orphaned file has to stay visible, which is what the negative tests cover. On a real React Router project this drops orphaned findings from 49 to 27.
The bash unused-import check treated a source directive as used only when the sourced file's basename reappeared in the script body. Scripts that source a function library (source ./lib.sh) and then call its functions or read its variables never repeat the basename, so every such script was flagged as an unused import. Resolve the sourced path relative to the sourcing script, parse it, and collect the symbols it defines: function definitions in both name() and 'function name' forms plus top-level variable assignments, including export/declare wrappers while excluding function-local declarations. The import counts as used when any defined symbol is referenced in the script. Unresolvable paths keep the existing basename heuristic, and per-run caching avoids reparsing a library shared by many scripts.
The unused-import detector took the last path segment as the in-code identifier. In Go that is frequently wrong, so every affected import was reported as unused. Scanning a real Go repo produced 20 false positives and no true positives: _ "github.com/joho/godotenv/autoload" -> "unused import: autoload" "gopkg.in/yaml.v3" -> "unused import: v3" "math/rand/v2" -> "unused import: v2" "github.com/go-playground/validator/v10"-> "unused import: v10" "github.com/anthropics/anthropic-sdk-go"-> "unused import: anthropic-sdk-go" These are used as yaml., rand., validator. and anthropic. respectively; the first is a blank import that exists only for its init() side effect. Note that a hyphenated segment can never be a Go identifier at all. There is also a general argument: go build fails on a genuinely unused import, so a Go module that compiles cannot have one, and any finding of this class on a compiling module is a false positive. Go now gets its own resolver that skips blank and dot imports, strips major-version segments (/v2, .v3), and returns None when the identifier cannot be determined from the path alone — callers treat None as "do not report", because a false negative is far cheaper than a false positive here. Verified: full suite passes (5825 passed, 3 skipped); scanning the repo that surfaced this drops its 17 unused-import findings to 0 with no new findings.
The GDScript dependency graph only followed `preload("res://x.gd")` and
`extends "res://x.gd"`. Godot code is rarely linked that way: scripts refer to
each other by their global `class_name`, and are attached to nodes through a
scene's `[ext_resource type="Script"]` block or a `project.godot` autoload
entry. None of those were resolved, so on a real project essentially every
`.gd` file reported zero importers.
`_find_project_root` also searched upward only. A Godot project is commonly one
directory inside a larger repository (engine plugins, build tooling and CI
beside it), and the scan root is that repository — so no `project.godot` was
found, every `res://` path failed to resolve, and even the preload edges that
were implemented produced nothing.
Measured on a 384-file Godot project: orphaned findings drop from 402 (one per
file, plus a matching false test_coverage finding each) to 7 genuine ones.
- follow `class_name` declarations and their use sites as graph edges, with
comments and string literals stripped first so a name mentioned in prose does
not fabricate a dependency
- add `find_gdscript_dynamic_imports`, reporting scripts a scene or autoload
attaches, via the existing `dynamic_import_finder` orphan hook
- search downward for `project.godot` when no ancestor holds one
- thread `dynamic_import_finder` through `run_coupling_phase`
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Godot's project templates and documentation use PascalCase directories, so a project's folders are `Scripts/`, `Scenes/` and `Tests/`. The zone rules matched only the lowercase spellings, so on a real project every suite in `Tests/` was classified production and scored as if it were shipping code, while the one file classified as a test was `test_scene_builder.gd` — production code that builds an in-game scene and merely starts with the Python/JS `test_` prefix. That prefix is not a Godot convention and is dropped; a Godot suite is `<name>_tests.gd`, `<name>_test.gd` or `<name>_suite.gd` inside a tests directory. Test-to-source mapping had the same import blindness as the dependency graph: it read only `preload`/`extends`, but a suite names what it exercises by global `class_name`, so nothing mapped and every file read as uncovered. Measured on a 384-file Godot project: test health 0.0% -> 42.1%, with the 23 suites now recognised as tests and credited against what they cover. - match Godot-cased test directories, and `_tests.gd`/`_suite.gd` suffixes - resolve a bare `class_name` in `resolve_import_spec` via a cached registry built from the production file set - report class names from `parse_test_import_specs`, with comments and strings stripped so a class merely described in prose is not counted as covered Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Resolving class_name references made the graph correct but reported the new edges as import cycles, including one spanning 86 files. A class_name reference cannot cycle at load time: Godot resolves the global registry lazily, so two scripts naming each other — a plug and its port, a cable and its socket — is ordinary and correct. Only preload and extends can cycle while parsing. Record class_name edges in `deferred_imports`, which `detect_cycles` already skips, leaving preload/extends cycles reported as before. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A registry or catalog commonly holds a script path in a data table and load()s
it later:
{"id": "atari_2600", "script": "res://Scripts/Objects/system_models/atari_2600_model.gd"}
No preload or extends pattern can see that, so every model registered this way
reported as orphaned even though the registry that names it is the only thing
that ever instantiates it. Comments are stripped first, so a path discussed in
prose is not counted as a reference.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… on Windows The opencode batch runner invoked the bare name 'opencode', which CreateProcess cannot resolve for npm-installed .cmd shims on Windows (WinError 2, exit 127). Mirror the codex runner: resolve the executable with shutil.which, wrap .cmd/.bat shims in 'cmd /c', and send the prompt via stdin instead of argv when the wrapper is used (cmd.exe re-parses argv and mangles prompts containing quotes or angle brackets).
Parse Cargo metadata from stdout without mixing diagnostic stderr. Scope check, Clippy, and rustdoc to the selected workspace member while preserving workspace-wide scans. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…list holds relative paths Source: peteromallet#696
…/destructuring imports as unused Source: peteromallet#676
…rce sets as test zone for Kotlin Source: peteromallet#677
…ter=json output correctly Source: peteromallet#680
…C++ review regex Source: peteromallet#729
…cktracking Source: peteromallet#733
…ted-worktree scan boundaries Source: peteromallet#741
…d async detection Source: peteromallet#713
…route modules as entry points Source: peteromallet#697
… into the C# dependency graph Source: peteromallet#682
…n versioned migrations Source: peteromallet#691
…y work items Source: peteromallet#692
…ty import entry points Source: peteromallet#690
… with discovered files Source: peteromallet#695
…ase output Source: peteromallet#689
…ne providers Source: peteromallet#687
…ension names Source: peteromallet#685
…plicate backlog items Source: peteromallet#743
…which and pipe prompts through stdin on Windows Source: peteromallet#737
…ose from opencode batch payloads Source: peteromallet#738
…, scene and autoload references Source: peteromallet#731
… from the package, not the path segment Source: peteromallet#703
…ry usage in unused-import detector Source: peteromallet#699
Follow-up to upstream PR peteromallet#682 (Razor/Blazor graph linking): views enter the C# dependency graph as edge sources, and detect_orphaned_files never filtered graph nodes by extension, so an unreferenced .razor/.cshtml view over 10 LOC was reported orphaned — a class of finding that did not exist before peteromallet#682. Apply the extension filter the function already receives, and strengthen the razor/orphan test to use a non-routable view so it actually exercises the filter instead of passing via the @page entrypoint path. Co-authored-by: Daniel Grimes <dan@dbhq.uk>
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
10 participants
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.
What this is
An integration of 27 open PRs, each individually reviewed (full diff read, applied to a worktree off current
main, affected test suites run against amainbaseline), merged as--no-ffmerge commits so original authorship is preserved, plus one follow-up fix. The goal: unstick the post-1.0 review queue by separating the verified-ready PRs from the ones needing rework, and giving maintainers a single tested integration to evaluate.Verification
tests/commands/test_cli.py::TestResolveLang::test_auto_detect_prefers_path_subtree_when_no_markers— environment-specific, fails identically on unmodifiedmainon machines with the go toolchain; unrelated to these changes).ruff check . --select E9,F63,F7,F82: clean.Included (27)
Core / cross-language
TypeScript / JS
Python
C++
Rust
Kotlin / Java
-f textC#
fcd2eff0) fixing a leak this exposed:detect_orphaned_filesnow applies the extension filter it always received, so linked views are no longer reported orphaned; the PR's own test was also passing for the wrong reason and was rewrittenOther languages
Runners
Intentionally excluded (with review comments posted on each)
step_completion,_extract_function_body) but itsunused.pyvariant was verified to regress Vite-root monorepos and its snapshot claim doesn't reproduce on main; consolidation path spelled out in the comments on fix: harden TypeScript scanning and plan resolution #721/fix: harden TypeScript project and async detection #713.Happy to split this into per-area PRs if that's easier to review — the merge commits are already structured that way.