Merge pull request #94 from ls1intum/code-smells

fix: correctness bugs in struggle detection pipeline

Author: Predixx

CODE_SMELL_REPORT.md

@@ -69,7 +69,7 @@ export class InterventionDecisionEngine {
         }
 
         // 2. Map EQ to intervention level
-        const level = this._mapEQToLevel(eq);
+        const level = this.mapEQToLevel(eq);
 
         if (level === 'none') {
             return {
@@ -98,8 +98,9 @@ export class InterventionDecisionEngine {
 
     /**
      * Map EQ score to recommended action level.
+     * Single source of truth for EQ-to-action mapping.
      */
-    private _mapEQToLevel(eq: number): RecommendedAction {
+    public mapEQToLevel(eq: number): RecommendedAction {
         if (eq >= this._thresholds.proactive) {
             return 'proactive';
         }

extension/src/extension/services/telemetry/decision/interventionDecisionEngine.ts

@@ -252,7 +252,13 @@ export function classifyBuildResult(result: ResultDTO): BuildResultClassificatio
         }
     }
 
-    // 3. Success
+    // 3. Fallback: if server reports failure but buildFailed/testCases are missing,
+    //    treat as test-failure to avoid silent false-success classification
+    if (result.successful === false) {
+        return 'test-failure';
+    }
+
+    // 4. Success
     return 'success';
 }
 

extension/src/extension/services/telemetry/eventPipeline/compileEquivalentEmitter.ts

@@ -100,6 +100,7 @@ export class InterventionFilter implements SessionResettable {
      */
     public onSessionStart(_context: SessionStartContext): void {
         this.setExerciseStartTime();
+        this._lastProgressTime = 0;
     }
 
 }

extension/src/extension/services/telemetry/interventionFilter.ts

@@ -384,10 +384,6 @@ export class TelemetryManager implements vscode.Disposable, WebSocketMessageHand
         const decision = this._decisionEngine.evaluate(eq, confidence, triggerType, state);
 
         if (!decision.shouldIntervene) {
-            // Still show subtle hint if warranted
-            if (decision.level === 'subtle') {
-                this._interventionService.showSubtleHintEQ(decision);
-            }
             return;
         }
 
@@ -415,7 +411,7 @@ export class TelemetryManager implements vscode.Disposable, WebSocketMessageHand
         const level = this._getRecommendedAction(eq, confidence);
 
         return {
-            isStruggling: confidence !== 'insufficient' && eq >= 0.15,
+            isStruggling: confidence !== 'insufficient' && level !== 'none',
             eq,
             eqConfidence: confidence,
             triggerType: this._lastTriggerType,
@@ -463,16 +459,7 @@ export class TelemetryManager implements vscode.Disposable, WebSocketMessageHand
         if (confidence === 'insufficient') {
             return 'none';
         }
-        if (eq >= 0.60) {
-            return 'proactive';
-        }
-        if (eq >= 0.35) {
-            return 'notification';
-        }
-        if (eq >= 0.15) {
-            return 'subtle';
-        }
-        return 'none';
+        return this._decisionEngine.mapEQToLevel(eq);
     }
 
     // ==================== DEBUG FEATURES ====================

extension/src/extension/services/telemetry/telemetryManager.ts

@@ -0,0 +1,108 @@
+/**
+ * classifyBuildResult — Unit Tests
+ *
+ * Verifies correct classification of Artemis build results into
+ * compiler-error, test-failure, or success for EQ pipeline consumption.
+ *
+ * Covers the NEW-3 fix: result.successful === false fallback prevents
+ * genuinely failed builds from being silently classified as 'success'.
+ */
+
+import * as assert from 'assert';
+import { classifyBuildResult } from '../../../src/extension/services/telemetry/eventPipeline/compileEquivalentEmitter';
+import type { ResultDTO } from '../../../src/extension/domain/submissions';
+
+function makeResult(overrides: Partial<ResultDTO> = {}): ResultDTO {
+    return {
+        id: 1,
+        ...overrides,
+    };
+}
+
+suite('classifyBuildResult', () => {
+
+    // ── Standard cases ──────────────────────────────────────────────
+
+    test('buildFailed === true → compiler-error', () => {
+        const result = makeResult({ submission: { buildFailed: true } });
+        assert.strictEqual(classifyBuildResult(result), 'compiler-error');
+    });
+
+    test('test failures with counts → test-failure', () => {
+        const result = makeResult({
+            testCaseCount: 5,
+            passedTestCaseCount: 3,
+            submission: { buildFailed: false },
+        });
+        assert.strictEqual(classifyBuildResult(result), 'test-failure');
+    });
+
+    test('all tests pass → success', () => {
+        const result = makeResult({
+            testCaseCount: 5,
+            passedTestCaseCount: 5,
+            successful: true,
+            submission: { buildFailed: false },
+        });
+        assert.strictEqual(classifyBuildResult(result), 'success');
+    });
+
+    test('no failure signals → success', () => {
+        const result = makeResult({ successful: true });
+        assert.strictEqual(classifyBuildResult(result), 'success');
+    });
+
+    // ── NEW-3 fix: result.successful === false fallback ─────────────
+
+    test('successful === false with missing buildFailed and no test counts → test-failure (not success)', () => {
+        const result = makeResult({
+            successful: false,
+            // submission.buildFailed is undefined
+            // testCaseCount is undefined
+        });
+        assert.strictEqual(
+            classifyBuildResult(result),
+            'test-failure',
+            'must not classify genuinely failed build as success when buildFailed field is missing',
+        );
+    });
+
+    test('successful === false with buildFailed === undefined and empty submission → test-failure', () => {
+        const result = makeResult({
+            successful: false,
+            submission: { buildFailed: undefined },
+        });
+        assert.strictEqual(classifyBuildResult(result), 'test-failure');
+    });
+
+    test('successful === undefined (truly unknown) with no other signals → success', () => {
+        const result = makeResult({
+            // successful is undefined
+            // no buildFailed, no test counts
+        });
+        assert.strictEqual(
+            classifyBuildResult(result),
+            'success',
+            'when server provides no success/failure signal, default to success',
+        );
+    });
+
+    // ── Priority: buildFailed takes precedence over successful ──────
+
+    test('buildFailed === true takes precedence over successful === false', () => {
+        const result = makeResult({
+            successful: false,
+            submission: { buildFailed: true },
+        });
+        assert.strictEqual(classifyBuildResult(result), 'compiler-error');
+    });
+
+    test('test counts take precedence over successful === false', () => {
+        const result = makeResult({
+            successful: false,
+            testCaseCount: 10,
+            passedTestCaseCount: 3,
+        });
+        assert.strictEqual(classifyBuildResult(result), 'test-failure');
+    });
+});

extension/test/unit/struggle-detection/classifyBuildResult.test.ts

@@ -525,6 +525,118 @@ suite('EQ Threshold Boundary Tests (InterventionDecisionEngine)', () => {
     });
 });
 
+// =========================================================================
+// Guardrail Enforcement Tests (Fix Verification)
+// =========================================================================
+
+suite('Guardrail Enforcement — Subtle Hints Respect Filter (NEW-1 fix)', () => {
+
+    let filter: InterventionFilter;
+    let decisionEngine: InterventionDecisionEngine;
+
+    setup(() => {
+        filter = new InterventionFilter();
+        decisionEngine = new InterventionDecisionEngine(filter);
+    });
+
+    test('subtle level blocked during exercise warmup (< 5 min)', () => {
+        // Exercise just started — warmup not elapsed
+        filter.setExerciseStartTime(Date.now());
+        const state: InterventionState = {
+            sessionInterventionCount: 0,
+            lastInterventionTime: 0,
+            lastDismissed: false,
+            lastAccepted: false,
+        };
+        const result = decisionEngine.evaluate(0.20, 'sufficient', 'idle', state);
+        assert.strictEqual(result.level, 'subtle');
+        assert.strictEqual(result.shouldIntervene, false, 'subtle hint must be blocked during warmup');
+    });
+
+    test('subtle level blocked when recent progress recorded', () => {
+        filter.setExerciseStartTime(Date.now() - 10 * 60 * 1000); // 10 min ago
+        filter.recordProgress(); // progress just now
+        const state: InterventionState = {
+            sessionInterventionCount: 0,
+            lastInterventionTime: 0,
+            lastDismissed: false,
+            lastAccepted: false,
+        };
+        const result = decisionEngine.evaluate(0.20, 'sufficient', 'idle', state);
+        assert.strictEqual(result.level, 'subtle');
+        assert.strictEqual(result.shouldIntervene, false, 'subtle hint must be blocked during progress grace period');
+    });
+
+    test('subtle level blocked after dismiss', () => {
+        filter.setExerciseStartTime(Date.now() - 10 * 60 * 1000);
+        const state: InterventionState = {
+            sessionInterventionCount: 0,
+            lastInterventionTime: 0,
+            lastDismissed: true,
+            lastAccepted: false,
+        };
+        const result = decisionEngine.evaluate(0.20, 'sufficient', 'idle', state);
+        assert.strictEqual(result.level, 'subtle');
+        assert.strictEqual(result.shouldIntervene, false, 'subtle hint must be blocked after dismiss');
+    });
+
+    test('EQ below 0.15 produces no intervention at all', () => {
+        filter.setExerciseStartTime(Date.now() - 10 * 60 * 1000);
+        const state: InterventionState = {
+            sessionInterventionCount: 0,
+            lastInterventionTime: 0,
+            lastDismissed: false,
+            lastAccepted: false,
+        };
+        const result = decisionEngine.evaluate(0.10, 'sufficient', 'idle', state);
+        assert.strictEqual(result.level, 'none');
+        assert.strictEqual(result.shouldIntervene, false);
+    });
+});
+
+suite('InterventionFilter Session Isolation (NEW-4 fix)', () => {
+
+    let filter: InterventionFilter;
+    let decisionEngine: InterventionDecisionEngine;
+
+    const defaultState: InterventionState = {
+        sessionInterventionCount: 0,
+        lastInterventionTime: 0,
+        lastDismissed: false,
+        lastAccepted: false,
+    };
+
+    setup(() => {
+        filter = new InterventionFilter();
+        decisionEngine = new InterventionDecisionEngine(filter);
+    });
+
+    test('progress from session A does not suppress interventions in session B', () => {
+        // Session A: exercise running > 5min, student makes progress
+        filter.setExerciseStartTime(Date.now() - 10 * 60 * 1000);
+        filter.recordProgress();
+
+        // Verify: progress grace period is active
+        const resultA = decisionEngine.evaluate(0.20, 'sufficient', 'idle', defaultState);
+        assert.strictEqual(resultA.shouldIntervene, false, 'session A should be suppressed by progress');
+
+        // Session B starts
+        filter.onSessionStart({ exerciseId: 999 });
+        // Fast-forward past warmup
+        (filter as any)._exerciseStartTime = Date.now() - 10 * 60 * 1000;
+
+        // Verify: progress grace period from session A is gone
+        const resultB = decisionEngine.evaluate(0.20, 'sufficient', 'idle', defaultState);
+        assert.strictEqual(resultB.shouldIntervene, true, 'session B must not be affected by session A progress');
+    });
+
+    test('_lastProgressTime is 0 after onSessionStart', () => {
+        filter.recordProgress();
+        filter.onSessionStart({ exerciseId: 123 });
+        assert.strictEqual((filter as any)._lastProgressTime, 0);
+    });
+});
+
 // =========================================================================
 // Helpers
 // =========================================================================