Skip to content

Commit 7cea626

Browse files
ZviBaratzclaude
andauthored
fix(ego-lint): reduce Media Controls false positives (#23)
## Summary - **onDestroy() recognized as cleanup**: `build-resource-graph.py` now detects `onDestroy()` alongside `destroy()`, `disable()`, and `_destroy*()` — eliminates 45 FP WARNs from Media Controls - **Provenance threshold lowered 4→3**: Score 3 means strong hand-written indicators (domain vocabulary, nontrivial algorithms). AI code scores 1-2. Eliminates 173 FP R-SLOP-01/02 WARNs Closes #17 ## Test plan - [x] New fixture: `on-destroy-cleanup@test` with widget using `onDestroy()` via `this.connect("destroy", ...)` - [x] Assertion: no `resource-tracking/no-destroy-method` for widget with onDestroy - [x] `bash tests/run-tests.sh` passes (512 assertions) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 145b639 commit 7cea626

File tree

9 files changed

+85
-21
lines changed

9 files changed

+85
-21
lines changed

skills/ego-lint/references/rules-reference.md

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -831,7 +831,7 @@ Rules that detect patterns commonly found in AI-generated extensions. These are
831831
- **Rule**: JSDoc comments should not use TypeScript-style `@param {Type} name` annotations.
832832
- **Rationale**: GNOME Shell extensions are plain JavaScript (not TypeScript). JSDoc type annotations in the `{Type}` format are a strong signal that code was generated by an AI trained primarily on TypeScript codebases. EGO reviewers recognize this pattern.
833833
- **Fix**: Remove type annotations from JSDoc comments, or remove JSDoc entirely if the code is self-documenting. GJS does not process JSDoc types.
834-
- **Provenance suppression**: When `quality/code-provenance` reports a score >= 4 (strong hand-written indicators), R-SLOP-01 warnings are automatically suppressed. High-provenance code uses JSDoc as intentional documentation, not AI slop. A `provenance/jsdoc-suppressed` PASS line is emitted instead.
834+
- **Provenance suppression**: When `quality/code-provenance` reports a score >= 3 (moderate-to-high hand-written indicators), R-SLOP-01 warnings are automatically suppressed. Hand-written code uses JSDoc as intentional documentation, not AI slop. A `provenance/jsdoc-suppressed` PASS line is emitted instead.
835835

836836
#### Example
837837

@@ -862,7 +862,7 @@ _updateIndicator(level, charging, icon) {
862862
- **Rule**: JSDoc comments should not use TypeScript-style `@returns {Type}` annotations.
863863
- **Rationale**: Same as R-SLOP-01. `@returns {Type}` is a TypeScript convention that has no effect in GJS and signals AI-generated code.
864864
- **Fix**: Remove `@returns {Type}` annotations from JSDoc comments.
865-
- **Provenance suppression**: Same as R-SLOP-01 — suppressed when provenance score >= 4.
865+
- **Provenance suppression**: Same as R-SLOP-01 — suppressed when provenance score >= 3.
866866

867867
#### Example
868868

@@ -2011,8 +2011,8 @@ Rules for APIs removed or changed in specific GNOME Shell versions. These rules
20112011
### resource-tracking/no-destroy-method
20122012
- **Severity**: advisory
20132013
- **Checked by**: check-resources.py → build-resource-graph.py
2014-
- **Rule**: Module creates resources but has no `destroy()` or `disable()` method for cleanup.
2015-
- **Fix**: Add a `destroy()` method that cleans up all resources.
2014+
- **Rule**: Module creates resources but has no cleanup method (`destroy()`, `disable()`, or `onDestroy()`) for cleanup.
2015+
- **Fix**: Add a `destroy()` method that cleans up all resources, or use `onDestroy()` connected via `this.connect('destroy', this.onDestroy.bind(this))`.
20162016
20172017
### resource-tracking/destroy-not-called
20182018
- **Severity**: advisory

skills/ego-lint/scripts/build-resource-graph.py

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -329,6 +329,10 @@ def scan_file(file_path, ext_dir):
329329
has_private_destroy = bool(re.search(
330330
r'(?:^|\s)_destroy\w*\s*\([^)]*\)\s*\{', content, re.MULTILINE
331331
))
332+
# Detect onDestroy() — common pattern via this.connect("destroy", this.onDestroy.bind(this))
333+
has_on_destroy = bool(re.search(
334+
r'(?:^|\s)onDestroy\s*\([^)]*\)\s*\{', content, re.MULTILINE
335+
))
332336

333337
return {
334338
'rel': rel,
@@ -339,6 +343,7 @@ def scan_file(file_path, ext_dir):
339343
'has_destroy': has_destroy,
340344
'has_disable': has_disable,
341345
'has_private_destroy': has_private_destroy,
346+
'has_on_destroy': has_on_destroy,
342347
'child_refs': child_refs,
343348
'content': content,
344349
}
@@ -436,9 +441,9 @@ def detect_orphans(file_scans, ownership):
436441
"""Detect orphaned resources in files that are owned by other files.
437442
438443
An orphan is a resource created in a child module where:
439-
1. No matching cleanup exists in that module's destroy() method, OR
440-
2. The module has no destroy() method but creates resources, OR
441-
3. The module's destroy() is never called by its parent.
444+
1. The module has no cleanup method (destroy/disable/onDestroy) but creates resources, OR
445+
2. The module has a cleanup method but its parent never calls it, OR
446+
3. The cleanup method exists and is called, but specific resources aren't cleaned up.
442447
443448
Only flags resources in files that ARE owned by another file.
444449
"""
@@ -463,7 +468,8 @@ def detect_orphans(file_scans, ownership):
463468
has_destroy = scan['has_destroy']
464469
has_disable = scan['has_disable']
465470
has_private_destroy = scan['has_private_destroy']
466-
has_cleanup_method = has_destroy or has_disable or has_private_destroy
471+
has_on_destroy = scan['has_on_destroy']
472+
has_cleanup_method = has_destroy or has_disable or has_private_destroy or has_on_destroy
467473

468474
if not creates:
469475
continue
@@ -486,27 +492,27 @@ def detect_orphans(file_scans, ownership):
486492
parent_calls_destroy = True
487493
break
488494

489-
# Case 1: Module creates resources but has no destroy/disable method
495+
# Case 1: Module creates resources but has no cleanup method
490496
if not has_cleanup_method:
491497
for c in creates:
492498
orphans.append({
493499
'file': rel,
494500
'line': c['line'],
495501
'type': c['type'],
496502
'pattern': c['pattern'],
497-
'reason': f"no destroy()/disable() method in {rel}",
503+
'reason': f"no cleanup method (destroy/disable/onDestroy) in {rel}",
498504
})
499505
continue
500506

501-
# Case 2: Module has destroy() but parent never calls it
507+
# Case 2: Module has cleanup method but parent never calls it
502508
if not parent_calls_destroy:
503509
for c in creates:
504510
orphans.append({
505511
'file': rel,
506512
'line': c['line'],
507513
'type': c['type'],
508514
'pattern': c['pattern'],
509-
'reason': f"parent does not call destroy() on {rel}",
515+
'reason': f"parent does not call cleanup method on {rel}",
510516
})
511517
continue
512518

@@ -517,7 +523,7 @@ def detect_orphans(file_scans, ownership):
517523
# releasing references even when the resource auto-cleans itself
518524
nulled_refs = set()
519525
content = scan['content']
520-
for method_name in ('destroy', 'disable', '_destroy'):
526+
for method_name in ('destroy', 'disable', '_destroy', 'onDestroy'):
521527
mb = find_method_body(content, method_name)
522528
if mb:
523529
_, _, body = mb

skills/ego-lint/scripts/check-resources.py

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -52,13 +52,13 @@ def classify_orphan(orphan):
5252
file_path = orphan.get('file', '')
5353
line = orphan.get('line', '?')
5454

55-
# Case 1: No destroy/disable method at all
56-
if 'no destroy()/disable() method' in reason:
55+
# Case 1: No cleanup method at all (destroy/disable/onDestroy)
56+
if 'no cleanup method' in reason:
5757
detail = f"{file_path}:{line}{reason}"
5858
return 'resource-tracking/no-destroy-method', detail
5959

60-
# Case 2: Parent doesn't call destroy
61-
if 'parent does not call destroy()' in reason:
60+
# Case 2: Parent doesn't call cleanup method
61+
if 'parent does not call cleanup method' in reason:
6262
detail = f"{file_path}:{line}{reason}"
6363
return 'resource-tracking/destroy-not-called', detail
6464

skills/ego-lint/scripts/ego-lint.sh

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -583,8 +583,10 @@ run_subscript "$SCRIPT_DIR/check-package.sh"
583583
# ---------------------------------------------------------------------------
584584
# Provenance-gated WARN suppression
585585
# ---------------------------------------------------------------------------
586-
# When code provenance is high (score >= 4), JSDoc annotations (R-SLOP-01/02)
587-
# are intentional documentation, not AI slop signals. Suppress them post-hoc.
586+
# When code provenance is moderate-to-high (score >= 3), JSDoc annotations
587+
# (R-SLOP-01/02) are intentional documentation, not AI slop signals. Score 3
588+
# means strong hand-written indicators (domain vocabulary, nontrivial algorithms).
589+
# AI-generated code typically scores 1-2. Suppress JSDoc warnings post-hoc.
588590
# Future: move provenance awareness into apply-patterns.py for inline gating.
589591

590592
provenance_score=0
@@ -595,8 +597,8 @@ if provenance_line=$(grep 'quality/code-provenance' "$RESULTS_FILE" 2>/dev/null)
595597
fi
596598

597599
deferred_count=${#DEFERRED_SLOP_JSDOC[@]}
598-
if [[ "$provenance_score" -ge 4 && "$deferred_count" -gt 0 ]]; then
599-
# Suppress deferred R-SLOP-01/02 WARNs (high provenance = intentional JSDoc)
600+
if [[ "$provenance_score" -ge 3 && "$deferred_count" -gt 0 ]]; then
601+
# Suppress deferred R-SLOP-01/02 WARNs (moderate-to-high provenance = intentional JSDoc)
600602
WARN_COUNT=$((WARN_COUNT - deferred_count))
601603
PASS_COUNT=$((PASS_COUNT + 1))
602604
print_result "PASS" "provenance/jsdoc-suppressed" \
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
SPDX-License-Identifier: GPL-2.0-or-later
Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
import GLib from 'gi://GLib';
2+
import {Extension} from 'resource:///org/gnome/shell/extensions/extension.js';
3+
4+
import TestWidget from './widget.js';
5+
6+
export default class TestExtension extends Extension {
7+
enable() {
8+
this._widget = new TestWidget();
9+
}
10+
11+
disable() {
12+
this._widget.destroy();
13+
this._widget = null;
14+
}
15+
}
Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
{
2+
"uuid": "on-destroy-cleanup@test",
3+
"name": "Test onDestroy Cleanup",
4+
"description": "Test onDestroy() recognized as cleanup method",
5+
"shell-version": ["48"],
6+
"url": "https://example.com"
7+
}
Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
import GObject from 'gi://GObject';
2+
import GLib from 'gi://GLib';
3+
import St from 'gi://St';
4+
5+
class TestWidget extends St.BoxLayout {
6+
constructor() {
7+
super();
8+
this._sourceId = GLib.timeout_add_seconds(GLib.PRIORITY_DEFAULT, 5, () => {
9+
return GLib.SOURCE_CONTINUE;
10+
});
11+
this.connect('destroy', this.onDestroy.bind(this));
12+
}
13+
14+
onDestroy() {
15+
if (this._sourceId) {
16+
GLib.Source.remove(this._sourceId);
17+
this._sourceId = null;
18+
}
19+
}
20+
}
21+
22+
export default GObject.registerClass(
23+
{ GTypeName: 'TestWidget' },
24+
TestWidget,
25+
);

tests/run-tests.sh

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -626,6 +626,14 @@ assert_output_contains "metadata.json found in src/" "\[PASS\].*file-structure/m
626626
assert_output_not_contains "no metadata/exists FAIL" "\[FAIL\].*metadata/exists"
627627
echo ""
628628

629+
# --- on-destroy-cleanup (onDestroy recognized as cleanup) ---
630+
echo "=== on-destroy-cleanup ==="
631+
run_lint "on-destroy-cleanup@test"
632+
assert_exit_code "exits with 0 (no blocking issues)" 0
633+
assert_output_not_contains "no no-destroy-method for widget with onDestroy" "\[WARN\].*resource-tracking/no-destroy-method.*widget"
634+
assert_output_contains "resource tracking ran" "(PASS|WARN|FAIL).*resource-tracking"
635+
echo ""
636+
629637
# Extended assertion files (auto-sourced from assertions/ directory)
630638
ASSERTIONS_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/assertions"
631639
for assertion_file in "$ASSERTIONS_DIR"/*.sh; do

0 commit comments

Comments
 (0)