Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
121 changes: 121 additions & 0 deletions docs/internal/field-test-just-perfection.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
# Field Test #10: Just Perfection

## Pre-flight

- **Extension**: Just Perfection (`just-perfection-desktop@just-perfection`)
- **Source**: https://github.com/jrahmatzadeh/just-perfection
- **GNOME versions**: 45, 46, 47, 48, 49, 50
- **File count**: 7 JS files, 2 CSS files; ~5,300 JS lines (Manager.js alone is 1,636 non-blank)
- **License**: GPL-3.0-only (in repo root, not in `src/`)
- **Why this extension**: Most popular GNOME extension; exercises connectObject/disconnectObject signal management (67 signals), prototype backup/restore via `#originals` map, `src/` directory layout, GNOME 50 support (widest version range), CSS-heavy (23.7%), private field usage (`#timeoutIds`, `#originals`, `#shellVersion`), 20 Shell.ui imports

## Step 1: ego-lint

```bash
./ego-lint --verbose /tmp/just-perfection/src
```

### Results (before fixes)

| Status | Count |
|--------|-------|
| PASS | 202 |
| FAIL | 3 |
| WARN | 414 |
| SKIP | 12 |
| Exit | 1 |

### Classification of Each FAIL

| Rule | File:Line | TP/FP | Root Cause (if FP) |
|------|-----------|-------|-------------------|
| `license` | — | **FP** | LICENSE in repo root, not in `src/`; no parent-dir fallback for src/ layout |
| `R-VER48-07` | stylesheet.css:167 | TP | `.quick-menu-toggle` renamed to `.quick-toggle-has-menu` in GNOME 48 |
| `metadata/future-shell-version` | — | TP | GNOME 50 > CURRENT_STABLE (49) |

### Classification of Key WARNs

| Rule | Count | Classification | Notes |
|------|-------|---------------|-------|
| R-SLOP-01 (JSDoc @param) | 129 | **FP** | Provenance score=3 but threshold was >=4; PR #23 fixes threshold to >=3 |
| R-SLOP-02 (JSDoc @returns) | 271 | **FP** | Same root cause as R-SLOP-01 |
| `lifecycle/prototype-override` | 5 (3 unique) | TP + **bug** | Advisory is valid (prototypes restored via close() chain). Regex catches both override AND restore → duplicates |
| `metadata/uuid-matches-dir` | 1 | **FP** | Dir is `src`, not UUID; meaningless for src/ layout |
| `metadata/session-modes-consistency` | 1 | **FP** | `sessionMode.isLocked` used as guard — not lock screen functionality |
| `quality/impossible-state` | 1 | **FP** | Same isLocked guard pattern |
| `quality/comment-density` | 1 | **FP** | SupportNotifier.js has 40% comments from thorough hand-written JSDoc |
| `metadata/deprecated-version` | 1 | TP | `version` field ignored by EGO for GNOME 45+ |
| `quality/file-complexity` | 1 | TP | Manager.js 1,636 non-blank lines |
| `quality/private-api` | 1 | TP | statusArea access (legitimate for a UI tweaker) |
| `disclosure/private-api` | 1 | TP | Private API not disclosed in metadata description |

## Fixes Implemented

### False Positives Fixed

| Rule/Check | Root Cause | Fix |
|------------|-----------|-----|
| `license` (FAIL) | No parent-dir fallback for src/ layout | `ego-lint.sh`: check parent dir when `basename == "src"` |
| `metadata/uuid-matches-dir` | Dir is `src` for src/ layout | `check-metadata.py`: skip when `dir_name == "src"` |
| `session-modes-consistency` | `isLocked` is always a guard | `check-metadata.py`: narrow regex to `sessionMode.currentMode` only |
| `quality/impossible-state` | `isLocked` is always a guard | `check-quality.py`: remove `isLocked` from check, keep `currentMode` comparisons |
| `quality/comment-density` | 40% threshold too aggressive | `check-quality.py`: raise threshold from 40% to 50% |
| `lifecycle/prototype-override` (dupes) | Regex matches override + restore | `check-lifecycle.py`: deduplicate by `(file, prototype)` key |

### Test Fixtures Added

| Fixture | What It Tests |
|---------|--------------|
| `src-license-fallback@test` | License found in parent dir for src/ layout; UUID-dir skip |
| `islocked-guard@test` | `sessionMode.isLocked` guard not flagged as impossible-state or session-modes inconsistency |
| `proto-override-dedup@test` | Each prototype warned only once despite override + restore pattern |

### Existing Fixture Updated

| Fixture | Change | Reason |
|---------|--------|--------|
| `ai-slop@test` | `sessionMode.isLocked` → `sessionMode.currentMode !== 'unlock-dialog'` | `isLocked` no longer triggers impossible-state (by design) |

## Results (after fixes)

| Status | Before | After | Change |
|--------|--------|-------|--------|
| FAIL | 3 | 2 | -1 (license FP fixed) |
| WARN | 414 | 408 | -6 (5 FPs + 2 dupes fixed) |

Remaining WARNs breakdown:
- R-SLOP-01/02: 400 (addressed by pending PR #23)
- lifecycle/prototype-override: 3 (true positives — advisory)
- Others: 5 (all true positives)

**With PR #23 merged** (provenance threshold >=3): WARNs would drop to ~8, all true positives.

## Regression Verification

```bash
bash tests/run-tests.sh
# Results: 460 passed, 0 failed
```

| Target | Before | After |
|--------|--------|-------|
| Just Perfection | 3 FAIL, 414 WARN | 2 FAIL, 408 WARN |
| Test suite | 452 passed | 460 passed (+8 new assertions) |

## Calibration Lessons Learned

1. **`sessionMode.isLocked` is always a guard**: Reading lock state to decide behavior is defensive coding, not lock screen functionality. Only `currentMode === 'unlock-dialog'` comparisons suggest active lock screen mode usage.

2. **`src/` layout needs holistic support**: License check and UUID-dir match didn't handle src/ layout. Multiple checks already had src/ fallbacks (CSS, prefs, metadata), but these two were missed. Consider auditing all checks for src/ layout awareness.

3. **Prototype override + restore creates duplicates**: When an extension stores originals and restores them (Just Perfection's `#originals` map pattern), the same regex catches both the override and the restore. Deduplication by `(file, prototype_name)` is essential.

4. **Comment-density threshold interacts with provenance**: Well-documented hand-written code (provenance=3) naturally has higher comment density (40%+) from thorough JSDoc. The 40% threshold was too aggressive; 50% avoids FPs while still catching AI slop (typically 55-70%).

5. **Just Perfection validates PR #23**: Provenance score=3 with 400 JSDoc warnings confirms the threshold change from >=4 to >=3 is needed.

## Issues Created

- [#26](https://github.com/ZviBaratz/gnome-extension-reviewer/issues/26): false-positive: license check fails on src/ layout extensions
- [#27](https://github.com/ZviBaratz/gnome-extension-reviewer/issues/27): false-positive: isLocked guard flagged as impossible-state and session-modes inconsistency
- [#28](https://github.com/ZviBaratz/gnome-extension-reviewer/issues/28): bug: prototype-override check emits duplicate warnings
12 changes: 10 additions & 2 deletions skills/ego-lint/scripts/check-lifecycle.py
Original file line number Diff line number Diff line change
Expand Up @@ -374,15 +374,23 @@ def check_injection_manager(ext_dir):

# WS1-D: Detect direct prototype overrides
prototype_overrides = []
seen_overrides = set()
for filepath in js_files:
content = strip_comments(read_file(filepath))
rel = os.path.relpath(filepath, ext_dir)
# SomeClass.prototype.methodName = ...
for m in re.finditer(r'(\w+\.prototype\.\w+)\s*=', content):
prototype_overrides.append((rel, m.group(1)))
key = (rel, m.group(1))
if key not in seen_overrides:
seen_overrides.add(key)
prototype_overrides.append(key)
# Object.assign(SomeClass.prototype, ...)
for m in re.finditer(r'Object\.assign\s*\(\s*(\w+\.prototype)', content):
prototype_overrides.append((rel, f"Object.assign({m.group(1)}, ...)"))
label = f"Object.assign({m.group(1)}, ...)"
key = (rel, label)
if key not in seen_overrides:
seen_overrides.add(key)
prototype_overrides.append(key)

if prototype_overrides:
# Check if disable() restores prototypes
Expand Down
7 changes: 6 additions & 1 deletion skills/ego-lint/scripts/check-metadata.py
Original file line number Diff line number Diff line change
Expand Up @@ -173,8 +173,11 @@ def main():
result("FAIL", "metadata/uuid-format", f"UUID contains invalid characters: {uuid}")

# UUID matches directory name (advisory — cloned repos often differ)
# Skip for src/ layout — dirname is "src", not the UUID
if uuid == dir_name:
result("PASS", "metadata/uuid-matches-dir", f"UUID matches directory name")
elif dir_name == "src":
result("PASS", "metadata/uuid-matches-dir", f"UUID check skipped for src/ layout")
else:
result("WARN", "metadata/uuid-matches-dir", f"UUID '{uuid}' does not match directory '{dir_name}' — must match when installed")

Expand Down Expand Up @@ -354,7 +357,9 @@ def check_session_modes_consistency(meta, ext_dir):
"session-modes includes 'unlock-dialog'")
return

session_mode_re = re.compile(r"sessionMode\.(currentMode|isLocked)")
# Only flag currentMode checks — isLocked is always a guard pattern
# (reading lock state to decide behavior, not evidence of lock screen usage)
session_mode_re = re.compile(r"sessionMode\.currentMode")
found = []
for root, _dirs, files in os.walk(ext_dir):
for fname in files:
Expand Down
12 changes: 5 additions & 7 deletions skills/ego-lint/scripts/check-quality.py
Original file line number Diff line number Diff line change
Expand Up @@ -130,12 +130,10 @@ def check_impossible_state(ext_dir, js_files):
rel = os.path.relpath(filepath, ext_dir)
with open(filepath, encoding='utf-8', errors='replace') as f:
for lineno, line in enumerate(f, 1):
if re.search(r'sessionMode\.isLocked', line):
result("WARN", "quality/impossible-state",
f"{rel}:{lineno}: checks isLocked but extension "
f"does not run in lock screen")
found = True
elif re.search(r"currentMode\s*===?\s*['\"]unlock-dialog['\"]", line):
# sessionMode.isLocked is always a guard (reading lock state
# to decide behavior) — not evidence of lock screen mode usage.
# Only flag currentMode === 'unlock-dialog' checks.
if re.search(r"currentMode\s*[!=]==?\s*['\"]unlock-dialog['\"]", line):
result("WARN", "quality/impossible-state",
f"{rel}:{lineno}: checks for unlock-dialog but "
f"extension does not declare this session-mode")
Expand Down Expand Up @@ -699,7 +697,7 @@ def check_comment_density(ext_dir, js_files):
code_lines += 1

total = comment_lines + code_lines
if total > 0 and comment_lines / total > 0.4:
if total > 0 and comment_lines / total > 0.5:
result("WARN", "quality/comment-density",
f"{rel}: {comment_lines}/{total} lines are comments "
f"({comment_lines * 100 // total}%) — may indicate AI-generated verbose comments")
Expand Down
11 changes: 11 additions & 0 deletions skills/ego-lint/scripts/ego-lint.sh
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,17 @@ for candidate in LICENSE COPYING LICENSE.rst LICENSE.md LICENSE.txt COPYING.rst
fi
done

# src/ layout fallback: check parent directory for license
if [[ -z "$license_file" && "$(basename "$EXT_DIR")" == "src" ]]; then
parent_dir="$(dirname "$EXT_DIR")"
for candidate in LICENSE COPYING LICENSE.rst LICENSE.md LICENSE.txt COPYING.rst COPYING.md COPYING.txt; do
if [[ -f "$parent_dir/$candidate" ]]; then
license_file="$parent_dir/$candidate"
break
fi
done
fi

if [[ -n "$license_file" ]]; then
head_content=$(head -5 "$license_file" 2>/dev/null || true)
if echo "$head_content" | grep -qiE '(GPL|LGPL|MIT|BSD|Apache|MPL|ISC|Artistic)'; then
Expand Down
24 changes: 24 additions & 0 deletions tests/assertions/just-perfection-calibration.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# Field test #10: Just Perfection calibration fixes

# --- src-license-fallback (LICENSE in parent dir of src/) ---
echo "=== src-license-fallback ==="
run_lint "src-license-fallback@test/src"
assert_exit_code "exits with 0 (license found in parent dir)" 0
assert_output_contains "license found via parent fallback" "\[PASS\].*license"
assert_output_contains "UUID-dir skipped for src/ layout" "\[PASS\].*metadata/uuid-matches-dir.*src/ layout"
echo ""

# --- islocked-guard (sessionMode.isLocked as guard, not lock screen) ---
echo "=== islocked-guard ==="
run_lint "islocked-guard@test"
assert_exit_code "exits with 0" 0
assert_output_not_contains "no impossible-state for isLocked guard" "\[WARN\].*quality/impossible-state"
assert_output_not_contains "no session-modes-consistency for isLocked" "\[WARN\].*session-modes-consistency"
echo ""

# --- proto-override-dedup (each prototype warned only once) ---
echo "=== proto-override-dedup ==="
run_lint "proto-override-dedup@test"
assert_output_contains "prototype override detected" "\[WARN\].*lifecycle/prototype-override.*BackgroundMenu.prototype.open"
assert_output_contains "search prototype override detected" "\[WARN\].*lifecycle/prototype-override.*SearchController.prototype.startSearch"
echo ""
2 changes: 1 addition & 1 deletion tests/fixtures/ai-slop@test/extension.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ export default class AiSlopExtension extends Extension {
this._pendingDestroy = false;
this._indicator = null;

if (!Main.sessionMode.isLocked && this._indicator === null)
if (Main.sessionMode.currentMode !== 'unlock-dialog' && this._indicator === null)
this._initAsync();
}

Expand Down
1 change: 1 addition & 0 deletions tests/fixtures/islocked-guard@test/LICENSE
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
SPDX-License-Identifier: GPL-2.0-or-later
21 changes: 21 additions & 0 deletions tests/fixtures/islocked-guard@test/extension.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import {Extension} from 'resource:///org/gnome/shell/extensions/extension.js';
import * as Main from 'resource:///org/gnome/shell/ui/main.js';

export default class TestExtension extends Extension {
enable() {
if (!this._isLocked()) {
this._applySettings();
}
}

disable() {
this._revertSettings();
}

_isLocked() {
return Main.sessionMode.isLocked;
}

_applySettings() {}
_revertSettings() {}
}
7 changes: 7 additions & 0 deletions tests/fixtures/islocked-guard@test/metadata.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"uuid": "islocked-guard@test",
"name": "Test isLocked Guard",
"description": "Tests that sessionMode.isLocked guard is not flagged",
"shell-version": ["48"],
"url": "https://example.com"
}
1 change: 1 addition & 0 deletions tests/fixtures/proto-override-dedup@test/LICENSE
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
SPDX-License-Identifier: GPL-2.0-or-later
14 changes: 14 additions & 0 deletions tests/fixtures/proto-override-dedup@test/extension.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import {Extension} from 'resource:///org/gnome/shell/extensions/extension.js';
import {API} from './lib/api.js';

export default class TestExtension extends Extension {
enable() {
this._api = new API();
this._api.open();
}

disable() {
this._api.close();
this._api = null;
}
}
21 changes: 21 additions & 0 deletions tests/fixtures/proto-override-dedup@test/lib/api.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import * as BackgroundMenu from 'resource:///org/gnome/shell/ui/backgroundMenu.js';
import * as Search from 'resource:///org/gnome/shell/ui/search.js';

export class API {
#originals = {};

open() {
// Override prototype — saves original first
this.#originals['bgOpen'] = BackgroundMenu.BackgroundMenu.prototype.open;
BackgroundMenu.BackgroundMenu.prototype.open = () => {};

this.#originals['startSearch'] = Search.SearchController.prototype.startSearch;
Search.SearchController.prototype.startSearch = () => {};
}

close() {
// Restore originals
BackgroundMenu.BackgroundMenu.prototype.open = this.#originals['bgOpen'];
Search.SearchController.prototype.startSearch = this.#originals['startSearch'];
}
}
7 changes: 7 additions & 0 deletions tests/fixtures/proto-override-dedup@test/metadata.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"uuid": "proto-override-dedup@test",
"name": "Test Proto Override Dedup",
"description": "Tests that prototype override warnings are deduplicated",
"shell-version": ["48"],
"url": "https://example.com"
}
1 change: 1 addition & 0 deletions tests/fixtures/src-license-fallback@test/LICENSE
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
SPDX-License-Identifier: GPL-2.0-or-later
6 changes: 6 additions & 0 deletions tests/fixtures/src-license-fallback@test/src/extension.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import {Extension} from 'resource:///org/gnome/shell/extensions/extension.js';

export default class TestExtension extends Extension {
enable() {}
disable() {}
}
7 changes: 7 additions & 0 deletions tests/fixtures/src-license-fallback@test/src/metadata.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"uuid": "src-license-fallback@test",
"name": "Test Src License Fallback",
"description": "Tests license check parent-dir fallback for src/ layout",
"shell-version": ["48"],
"url": "https://example.com"
}