Skip to content

Commit ecc224b

Browse files
aksOpsaksOps
authored
phase a/fixups spotbugs (#43)
* build(spotbugs): add exclude filter for generated parsers and known noise rules Reduces SpotBugs findings 1,492 -> 51 (96.6%) by excluding: - ANTLR-generated parser/lexer/listener/visitor classes (regenerated from .g4) - NM_METHOD_NAMING_CONVENTION (noise from generated code and ANTLR hooks) - SF_SWITCH_NO_DEFAULT (stylistic; SF_SWITCH_FALLTHROUGH still enforced) - EI_EXPOSE_REP / EI_EXPOSE_REP2 (internal DTOs, no trust boundary crossed) - MS_PKGPROTECT / MS_FINAL_PKGPROTECT (no same-package attacker model) Each exclusion carries a rationale comment in spotbugs-exclude.xml. * fix(spotbugs): address 4 priority-1 findings in core code - Analyzer.getGitHead: decode `git rev-parse HEAD` bytes as UTF-8 instead of platform default (DM_DEFAULT_ENCODING). - IndexCommand.call: narrow the cache-delete catch from blanket `Exception ignored` to `IOException` with a debug log line so silent data-loss is traceable (DE_MIGHT_IGNORE). - PluginsCommand.categoryDescription: remove dead `Set<String> frameworks` local that was assigned and never read (DLS_DEAD_LOCAL_STORE). - AntlrParserFactory.parse: replace identity-compare (`==`) of cache key with `.equals()`; cache behavior is unchanged because the parse tree is a deterministic function of content, so equal content yields an equivalent tree (ES_COMPARING_PARAMETER_STRING_WITH_EQ). Verified `mvn compile` clean on phase-a/fixups-spotbugs. * fix(spotbugs): concurrency and correctness priority-2 findings - AnalysisCache.removeFile: guarantee rwLock.writeLock().unlock() on all exception paths by wrapping the finally's conn.setAutoCommit(true) in its own try-finally. Previously, a non-SQLException thrown by setAutoCommit (RuntimeException/Error) would escape the finally block before unlock ran, leaking the write lock. (UL_UNRELEASED_LOCK_EXCEPTION_PATH) Note: the same setAutoCommit-in-finally-then-unlock pattern appears in two other methods in this class; SpotBugs only flagged removeFile, but they likely share the same risk. Out of scope for this PR; file a follow-up. - CodeIqApplication.main: collapse three identical else-if branches (isIndex / isEnrich / default) into one else with a combined rationale comment. (DB_DUPLICATE_BRANCHES) - BundleCommand: remove unused private 4-arg writeEntry(zos, name, content, lineEnding) that delegated to the 3-arg overload and silently discarded lineEnding. (UPM_UNCALLED_PRIVATE_METHOD) - PluginsCommand.SuggestSubcommand: iterate languageCounts.entrySet() instead of keySet() + get(). (WMI_WRONG_MAP_ITERATOR) - GitLabCiDetector.detect: iterate data.entrySet() instead of keySet() + get(). (WMI_WRONG_MAP_ITERATOR) - CSharpPreprocessorParserBase: replace reference-compare (== / !=) on expression values with Objects.equals(). The compared values are String literals 'true'/'false' produced by sibling methods, so logical equality is the intended semantic; using == only worked coincidentally via string interning. (ES_COMPARING_STRINGS_WITH_EQ x2) - VersionCommand.resolveVersion: narrow the outer catch from Exception to IOException since that is the only checked exception props.load can throw. Updated comment to document that the branch is an intentional fallthrough to the manifest-based lookup. (REC_CATCH_EXCEPTION) Verified mvn compile clean. * build(spotbugs): narrow-suppress 2 parser-base RCN + 1 BX finding, record triage result Added narrow exclude rules (class + pattern pair, not global) for: - RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE in CSharpParserBase and GoParserBase — hand-written ANTLR parser support classes carried over from antlr/grammars-v4. Defensive null checks are harmless; touching them risks divergence if we re-sync upstream. - BX_UNBOXING_IMMEDIATELY_REBOXED in CSharpPreprocessorParserBase — same origin, micro-perf, not correctness. Updated BASELINE.md to mark the SpotBugs gap as RESOLVED with final counts (1,492 -> 38; priority-1: 8 -> 0) and a pointer to the post-triage summary JSON. * fix(cache): extend nested-try-finally lock release to remaining AnalysisCache methods Follow-up to 798bccf. That commit fixed UL_UNRELEASED_LOCK_EXCEPTION_PATH in AnalysisCache.removeFile and flagged the same lock-leak pattern in two other methods that SpotBugs did not surface (likely because they differ subtly in what's inside the outer try body). Both methods — storeResults (the INSERT nodes/edges path) and the replace-with-enriched-data path — had: } finally { try { conn.setAutoCommit(true); } catch (SQLException ignored) {} rwLock.writeLock().unlock(); } If conn.setAutoCommit(true) throws anything other than SQLException (RuntimeException or Error), execution exits the finally before rwLock.writeLock().unlock() runs, leaving the write lock held and freezing all subsequent cache writers. Wrap setAutoCommit in a nested try-finally so unlock is always reached: } finally { try { try { conn.setAutoCommit(true); } catch (SQLException ignored) {} } finally { rwLock.writeLock().unlock(); } } All three lock-holding paths (removeFile, storeResults, replace-with- enriched) now share the same exception-safe pattern. mvn test -Dtest= AnalysisCacheTest passes.
1 parent caabd1d commit ecc224b

13 files changed

Lines changed: 152 additions & 37 deletions

File tree

‎docs/superpowers/baselines/2026-04-17/BASELINE.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -230,6 +230,7 @@ Ordered by severity. Each item cites the raw artifact it was derived from.
230230

231231
- **SpotBugs: 8 HIGH-priority findings (priority=1) + 1,484 at priority=2.** Total 1,492. HIGH findings must be triaged individually (read `raw/spotbugs.xml`). Noise-dominant rules (`NM_METHOD_NAMING_CONVENTION`=730, `SF_SWITCH_NO_DEFAULT`=448) should be filtered via a SpotBugs exclude file so real signal surfaces; real-concern patterns that deserve review now: `NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE` (26), `BC_UNCONFIRMED_CAST` (55), `UL_UNRELEASED_LOCK_EXCEPTION_PATH` (1), `WMI_WRONG_MAP_ITERATOR` (2), `ES_COMPARING_STRINGS_WITH_EQ` (2), `MT_CORRECTNESS` category (1).
232232
- Raw: `raw/spotbugs.xml`, `raw/spotbugs-summary.json`.
233+
- **RESOLVED (2026-04-17, branch `phase-a/fixups-spotbugs`)**: Added `spotbugs-exclude.xml` covering ANTLR-generated parsers and global noise rules (`NM_METHOD_NAMING_CONVENTION`, `SF_SWITCH_NO_DEFAULT`, `EI_EXPOSE_REP`/`EI_EXPOSE_REP2`, `MS_PKGPROTECT`/`MS_FINAL_PKGPROTECT`), wired via `pom.xml`. Fixed all 8 priority-1 findings in codeiq code (UTF-8 in `Analyzer.getGitHead`, narrowed catch in `IndexCommand`, dead-store removed in `PluginsCommand`, `.equals()` in `AntlrParserFactory` + `CSharpPreprocessorParserBase`, try-finally unlock in `AnalysisCache.removeFile`, merged duplicate branches in `CodeIqApplication`, removed dead `BundleCommand.writeEntry` overload, `entrySet()` iteration in `PluginsCommand` + `GitLabCiDetector`, narrowed `VersionCommand` catch). **Final: 1,492 → 38 (-97.5%); priority-1: 8 → 0.** Remaining 38 are priority-2 STYLE/BAD_PRACTICE; no CORRECTNESS/MT_CORRECTNESS/SECURITY left. Next-pass candidates: 26 `NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE`. Post-triage summary: `raw/spotbugs-summary-after-triage.json`.
233234

234235
### Medium
235236

‎pom.xml‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -323,6 +323,9 @@
323323
<groupId>com.github.spotbugs</groupId>
324324
<artifactId>spotbugs-maven-plugin</artifactId>
325325
<version>${spotbugs.version}</version>
326+
<configuration>
327+
<excludeFilterFile>spotbugs-exclude.xml</excludeFilterFile>
328+
</configuration>
326329
</plugin>
327330

328331
<plugin>

‎spotbugs-exclude.xml‎

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
<?xml version="1.0" encoding="UTF-8"?>
2+
<!--
3+
SpotBugs exclude filter for code-iq.
4+
5+
Philosophy: fail CI on real bugs, not on stylistic or generated-code noise.
6+
Each entry MUST have a rationale comment. If a rule here ever catches a
7+
genuine bug in new code, narrow or remove it — never silently live with it.
8+
9+
Docs: https://spotbugs.readthedocs.io/en/latest/filter.html
10+
-->
11+
<FindBugsFilter>
12+
13+
<!--
14+
ANTLR-generated parser sources.
15+
Regenerated on every `mvn generate-sources` from .g4 files in
16+
src/main/antlr4/. Fixing findings here is futile. Hand-written ANTLR
17+
support classes (name ends in `Base`, e.g. CSharpParserBase) are NOT
18+
excluded — those are ours to fix.
19+
-->
20+
<Match>
21+
<Class name="~io\.github\.randomcodespace\.iq\.grammar\..*(Parser|Lexer|Listener|Visitor)$"/>
22+
</Match>
23+
24+
<!--
25+
NM_METHOD_NAMING_CONVENTION — camelCase/PascalCase warnings.
26+
730 findings, nearly all from generated parsers and overridden ANTLR
27+
hooks (e.g. `OnPreprocessorExpressionConditionalEq`). Not actionable
28+
for new hand-written code; the compiler + review catch real typos.
29+
-->
30+
<Match>
31+
<Bug pattern="NM_METHOD_NAMING_CONVENTION"/>
32+
</Match>
33+
34+
<!--
35+
SF_SWITCH_NO_DEFAULT — stylistic.
36+
448 findings, dominated by generated ANTLR rule dispatch. Real
37+
fall-through/dead-store bugs are caught separately by
38+
SF_SWITCH_FALLTHROUGH and SF_DEAD_STORE_DUE_TO_SWITCH_FALLTHROUGH,
39+
which we DO enforce.
40+
-->
41+
<Match>
42+
<Bug pattern="SF_SWITCH_NO_DEFAULT"/>
43+
</Match>
44+
45+
<!--
46+
EI_EXPOSE_REP / EI_EXPOSE_REP2 — "constructor/getter stores or returns
47+
internal mutable state". 123 findings across detector result DTOs and
48+
graph-model records. No trust boundary is crossed here: these objects
49+
live inside a single JVM and are consumed by trusted pipeline code.
50+
Defensive copies would add GC cost for no security benefit. If we ever
51+
expose these across a security boundary, tighten here.
52+
-->
53+
<Match>
54+
<Bug pattern="EI_EXPOSE_REP"/>
55+
</Match>
56+
<Match>
57+
<Bug pattern="EI_EXPOSE_REP2"/>
58+
</Match>
59+
60+
<!--
61+
MS_PKGPROTECT / MS_FINAL_PKGPROTECT — "mutable non-final static field
62+
could be overwritten by a malicious subclass in the same package".
63+
80 findings. This JVM runs no untrusted bytecode; there is no
64+
same-package attacker model. Pure noise.
65+
-->
66+
<Match>
67+
<Bug pattern="MS_PKGPROTECT"/>
68+
</Match>
69+
<Match>
70+
<Bug pattern="MS_FINAL_PKGPROTECT"/>
71+
</Match>
72+
73+
<!--
74+
RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE in hand-written ANTLR parser
75+
support classes (CSharpParserBase, GoParserBase). Defensive null checks
76+
carried over verbatim from antlr/grammars-v4 grammar action files. Harmless;
77+
divergence risk if we "fix" them because upstream may re-sync.
78+
-->
79+
<Match>
80+
<Class name="~io\.github\.randomcodespace\.iq\.grammar\.(csharp\.CSharpParserBase|golang\.GoParserBase)"/>
81+
<Bug pattern="RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE"/>
82+
</Match>
83+
84+
<!--
85+
BX_UNBOXING_IMMEDIATELY_REBOXED in CSharpPreprocessorParserBase: grammar
86+
action code adapted from upstream. Micro-perf, not a correctness issue.
87+
-->
88+
<Match>
89+
<Class name="io.github.randomcodespace.iq.grammar.csharp.CSharpPreprocessorParserBase"/>
90+
<Bug pattern="BX_UNBOXING_IMMEDIATELY_REBOXED"/>
91+
</Match>
92+
93+
</FindBugsFilter>

‎src/main/java/io/github/randomcodespace/iq/CodeIqApplication.java‎

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -96,17 +96,12 @@ public static void main(String[] args) {
9696
// Point Neo4j config to the graph path (enriched or new empty db).
9797
// GraphBootstrapper will auto-load from H2 cache if no enriched graph exists.
9898
System.setProperty("codeiq.graph.path", graphDbPath.toString());
99-
} else if (isIndex) {
100-
app.setAdditionalProfiles("indexing");
101-
// Index command: no web server, no Neo4j
102-
app.setWebApplicationType(org.springframework.boot.WebApplicationType.NONE);
103-
} else if (isEnrich) {
104-
// Enrich command: no web server, Neo4j started programmatically
105-
app.setAdditionalProfiles("indexing");
106-
app.setWebApplicationType(org.springframework.boot.WebApplicationType.NONE);
10799
} else {
100+
// All non-serve commands (index, enrich, analyze, stats, ...) share the same
101+
// Spring setup: "indexing" profile, no web server. index/enrich open Neo4j
102+
// programmatically when needed. Previously split into three identical
103+
// branches — SpotBugs DB_DUPLICATE_BRANCHES.
108104
app.setAdditionalProfiles("indexing");
109-
// Disable web server for non-serve commands
110105
app.setWebApplicationType(org.springframework.boot.WebApplicationType.NONE);
111106
}
112107

‎src/main/java/io/github/randomcodespace/iq/analyzer/Analyzer.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
package io.github.randomcodespace.iq.analyzer;
22

3+
import java.nio.charset.StandardCharsets;
34
import io.github.randomcodespace.iq.analyzer.linker.Linker;
45
import io.github.randomcodespace.iq.cache.AnalysisCache;
56
import io.github.randomcodespace.iq.cache.FileHasher;
@@ -1608,7 +1609,7 @@ private String getGitHead(Path repoPath) {
16081609
.directory(repoPath.toFile())
16091610
.redirectErrorStream(true);
16101611
Process proc = pb.start();
1611-
String sha = new String(proc.getInputStream().readAllBytes()).trim();
1612+
String sha = new String(proc.getInputStream().readAllBytes(), StandardCharsets.UTF_8).trim();
16121613
int exitCode = proc.waitFor();
16131614
if (exitCode == 0 && sha.length() >= 7) {
16141615
return sha;

‎src/main/java/io/github/randomcodespace/iq/cache/AnalysisCache.java‎

Lines changed: 27 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -372,10 +372,16 @@ public void storeResults(String contentHash, String filePath, String language,
372372
log.warn("Failed to store cached results for hash {}", contentHash, e);
373373
} finally {
374374
try {
375-
conn.setAutoCommit(true);
376-
} catch (SQLException ignored) {
375+
try {
376+
conn.setAutoCommit(true);
377+
} catch (SQLException ignored) {
378+
// best-effort restore; the INSERTs have already been committed or rolled back.
379+
}
380+
} finally {
381+
// Guarantee unlock even if conn.setAutoCommit throws a non-SQLException
382+
// (RuntimeException / Error). Fixes SpotBugs UL_UNRELEASED_LOCK_EXCEPTION_PATH.
383+
rwLock.writeLock().unlock();
377384
}
378-
rwLock.writeLock().unlock();
379385
}
380386
}
381387

@@ -453,10 +459,16 @@ public void removeFile(String contentHash) {
453459
log.warn("Failed to remove cached file {}", contentHash, e);
454460
} finally {
455461
try {
456-
conn.setAutoCommit(true);
457-
} catch (SQLException ignored) {
462+
try {
463+
conn.setAutoCommit(true);
464+
} catch (SQLException ignored) {
465+
// best-effort restore; the DELETEs have already been committed or rolled back.
466+
}
467+
} finally {
468+
// Guarantee unlock even if conn.setAutoCommit throws a non-SQLException
469+
// (RuntimeException / Error). Fixes SpotBugs UL_UNRELEASED_LOCK_EXCEPTION_PATH.
470+
rwLock.writeLock().unlock();
458471
}
459-
rwLock.writeLock().unlock();
460472
}
461473
}
462474

@@ -599,10 +611,16 @@ public void replaceAll(List<CodeNode> nodes, List<CodeEdge> edges) {
599611
log.warn("Failed to replace cache with enriched data", e);
600612
} finally {
601613
try {
602-
conn.setAutoCommit(true);
603-
} catch (SQLException ignored) {
614+
try {
615+
conn.setAutoCommit(true);
616+
} catch (SQLException ignored) {
617+
// best-effort restore; the INSERTs have already been committed or rolled back.
618+
}
619+
} finally {
620+
// Guarantee unlock even if conn.setAutoCommit throws a non-SQLException
621+
// (RuntimeException / Error). Fixes SpotBugs UL_UNRELEASED_LOCK_EXCEPTION_PATH.
622+
rwLock.writeLock().unlock();
604623
}
605-
rwLock.writeLock().unlock();
606624
}
607625
}
608626

‎src/main/java/io/github/randomcodespace/iq/cli/BundleCommand.java‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -464,9 +464,4 @@ private void writeEntry(ZipOutputStream zos, String name, String content) throws
464464
zos.closeEntry();
465465
}
466466

467-
private void writeEntry(ZipOutputStream zos, String name, String content, String lineEnding)
468-
throws IOException {
469-
writeEntry(zos, name, content);
470-
}
471-
472467
}

‎src/main/java/io/github/randomcodespace/iq/cli/IndexCommand.java‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,8 +85,15 @@ public Integer call() {
8585
if (java.nio.file.Files.exists(cacheDir)) {
8686
try {
8787
try (var walk = java.nio.file.Files.walk(cacheDir)) {
88+
var logger = org.slf4j.LoggerFactory.getLogger(IndexCommand.class);
8889
walk.sorted(java.util.Comparator.reverseOrder())
89-
.forEach(p -> { try { java.nio.file.Files.deleteIfExists(p); } catch (Exception ignored) {} });
90+
.forEach(p -> {
91+
try {
92+
java.nio.file.Files.deleteIfExists(p);
93+
} catch (java.io.IOException ex) {
94+
logger.debug("Could not delete cache entry {}: {}", p, ex.getMessage());
95+
}
96+
});
9097
}
9198
CliOutput.info(" Deleted existing cache at " + cacheDir);
9299
} catch (Exception e) {

‎src/main/java/io/github/randomcodespace/iq/cli/PluginsCommand.java‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -100,8 +100,6 @@ public Integer call() {
100100
* or falls back to a sensible default.
101101
*/
102102
static String categoryDescription(String category, List<Detector> detectors) {
103-
// Collect unique frameworks from DetectorInfo if available
104-
Set<String> frameworks = new TreeSet<>();
105103
for (Detector d : detectors) {
106104
DetectorInfo info = d.getClass().getAnnotation(DetectorInfo.class);
107105
if (info != null && info.description() != null && !info.description().isEmpty()) {
@@ -406,8 +404,9 @@ public FileVisitResult visitFileFailed(Path file, IOException exc) {
406404
yaml.append("# Optimized for this project's detected languages\n\n");
407405

408406
yaml.append("languages:\n");
409-
for (String lang : languageCounts.keySet()) {
410-
yaml.append(" - ").append(lang).append(" # ").append(languageCounts.get(lang)).append(" files\n");
407+
for (Map.Entry<String, Integer> entry : languageCounts.entrySet()) {
408+
yaml.append(" - ").append(entry.getKey())
409+
.append(" # ").append(entry.getValue()).append(" files\n");
411410
}
412411

413412
yaml.append("\ndetectors:\n");

‎src/main/java/io/github/randomcodespace/iq/cli/VersionCommand.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,8 +38,8 @@ private static String resolveVersion() {
3838
String v = props.getProperty("build.version");
3939
if (v != null && !v.isBlank()) return v;
4040
}
41-
} catch (Exception ignored) {
42-
// intentionally empty
41+
} catch (java.io.IOException ignored) {
42+
// build-info.properties is optional; fall through to manifest lookup.
4343
}
4444
// Fallback: Implementation-Version from JAR manifest
4545
String v = VersionCommand.class.getPackage().getImplementationVersion();

0 commit comments

Comments
 (0)