From 03258a07a2a3a1bf264cc1aea6b68f64b73e9eee Mon Sep 17 00:00:00 2001
From: sim <sim@local>
Date: Thu, 13 Aug 2026 02:56:30 -0400
Subject: [PATCH] fix(#3309): applyRepairs must not count a failed repair as
 applied
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

applyRepairs pushed a diagnostic's code onto applied unconditionally
after the try/catch around runRepairAction, even when the handler
threw (caught, recorded in details with success:false) or otherwise
failed — making applied mean "attempted" rather than "succeeded," with
no test exercising the failure path. Found by the Spec-axis orthogonal
review.

applied now only receives a code when the repair actually succeeded;
a failed attempt is still fully recorded in details (success:false,
the error message) but no longer misreported as applied. Adds a
regression test forcing addNyquistKey to throw (ENOENT on a config.json
that doesn't exist) and asserts it lands in details, not applied.
---
 src/health-diagnostic.cts        |  7 ++++++-
 tests/health-diagnostic.test.cjs | 24 ++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/src/health-diagnostic.cts b/src/health-diagnostic.cts
index 2410426c9..6079c9a02 100644
--- a/src/health-diagnostic.cts
+++ b/src/health-diagnostic.cts
@@ -443,6 +443,12 @@ function applyRepairs(
         ...(outcome.detail ? { detail: outcome.detail } : {}),
         ...(outcome.error ? { error: outcome.error } : {}),
       });
+      // `applied` means "the repair actually succeeded", not "was attempted"
+      // — a handler that returns `{success: false}` (or throws, caught
+      // below) is recorded in `details` with its failure, but must not be
+      // reported as applied. `refused` is reserved for the DESTRUCTIVE-risk
+      // gate above; a failed attempt is neither applied nor refused.
+      if (outcome.success) applied.push(code);
     } catch (err) {
       details.push({
         code,
@@ -451,7 +457,6 @@ function applyRepairs(
         error: err instanceof Error ? err.message : String(err),
       });
     }
-    applied.push(code);
   }
 
   return { applied, refused, details };
diff --git a/tests/health-diagnostic.test.cjs b/tests/health-diagnostic.test.cjs
index e8621da54..ff12e59ca 100644
--- a/tests/health-diagnostic.test.cjs
+++ b/tests/health-diagnostic.test.cjs
@@ -350,4 +350,28 @@ describe('applyRepairs — REAL diagnostics (rows 15-16)', () => {
     const diskConfig = JSON.parse(fs.readFileSync(configPath, 'utf-8'));
     assert.equal(diskConfig.model_profile, 'balanced');
   });
+
+  // Regression: a repair handler that THROWS (caught by applyRepairs's own
+  // try/catch) must be recorded in `details` with `success: false` and must
+  // NOT land in `applied` — `applied` means "succeeded", not "attempted".
+  // Forced here via ADD_NYQUIST_KEY against a config.json that is genuinely
+  // absent: `runRepairAction`'s `fs.readFileSync(configPath, ...)` throws
+  // ENOENT.
+  test('regression: a repair handler that throws is recorded in details with success:false and is NOT pushed to applied', (t) => {
+    const tmpDir = createTempProject();
+    t.after(() => cleanup(tmpDir));
+    setupHealthyProject(tmpDir);
+    fs.unlinkSync(path.join(tmpDir, '.planning', 'config.json'));
+    assert.equal(fs.existsSync(path.join(tmpDir, '.planning', 'config.json')), false);
+
+    const diagnostics = [fakeDiagnostic('W008', REMEDY_ACTION.ADD_NYQUIST_KEY, REMEDY_RISK.NONE)];
+    const result = applyRepairs(tmpDir, diagnostics, true, false);
+
+    assert.ok(!result.applied.includes('W008'), 'W008 must not be applied — the handler threw');
+    assert.ok(!result.refused.includes('W008'), 'a thrown handler is not a DESTRUCTIVE-risk refusal either');
+    const detail = result.details.find((d) => d.code === 'W008');
+    assert.ok(detail, 'a details row must still be recorded for the failed attempt');
+    assert.equal(detail.success, false);
+    assert.ok(detail.error, 'the details row must carry the thrown error message');
+  });
 });