fix(#3309): applyRepairs must not count a failed repair as applied
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.
This commit is contained in:
@@ -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 };
|
||||
|
||||
@@ -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');
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user