diff --git a/REVIEW.md b/REVIEW.md index 7f474b9..1294734 100644 --- a/REVIEW.md +++ b/REVIEW.md @@ -713,3 +713,53 @@ Cancel → approved → `openclaw config get gateway.auth.mode` returned **token That is the whole chain — detect, propose, review, approve, apply, verify — fixing a real problem on a real OpenClaw install through the shipped interface. + + +--- + +# Fifth pass — more repairs, and honest finding titles + +## Repair catalog: 1 → 5 + +Three more config repairs, all built on `openclaw config set` with the read-back as verification, +all sharing one `configToggleRepair` contract so the guard rails cannot drift apart: + +| Repair | Key | Verified on a real install | +|---|---|---| +| `auto-update-enabled-warning` | `update.auto.enabled` → false | applied, finding gone | +| `gateway-loopback-no-auth` | `gateway.auth.mode` → token | applied, finding gone | +| `no-hybrid-search` | `agents.defaults.memorySearch.query.hybrid.enabled` → true | applied, `current:"true"`, finding gone | +| `no-memory-flush` | `agents.defaults.compaction.memoryFlush.enabled` → true | applied, `current:"true"`, finding gone | + +Four of the five now work without a service manager, so a containerised OpenClaw is no longer a +host where ClawFix can only look and shrug. `gateway-not-running` still needs systemd or launchd — +that is OpenClaw's restart path, not something ClawFix can route around. + +Each toggle refuses an unreadable key (`config_state_unknown`) rather than treating "cannot read" +as "false", and rollback restores the previous value. + +## Port conflict: deliberately still not repaired + +The obvious next repair is the port conflict — ClawFix already knows the squatting PID. It was not +added, because every version of it ends in killing a process ClawFix does not own. A repair that +terminates an unrelated service is not a guarded repair, whatever the preview says. If this is +wanted later, the safe shape is: refuse unless the listener is itself a stale OpenClaw process, +and even then prefer OpenClaw's own `--force` restart over a kill. + +## Two findings were shouting machine text at the user + +`[critical] timeout` came from `nativeStatus.gateway.error` being used directly as an issue title. +A probe that gave up was presented as a critical OpenClaw fault named "timeout". The same pattern +turned a validator message into the title `JSON5 parse failed: SyntaxError: JSON5: invalid +character 'i' at 1:8`. + +Both now carry an actionable headline with the upstream text as detail: + +``` +[high] OpenClaw config schema validation failed + detail: JSON5 parse failed: SyntaxError: JSON5: invalid character 'i' at 1:8 +``` + +Fixing the title broke the doctor-findings dedup, which had been matching on the raw message — so +the same problem briefly appeared twice, once well-titled and once raw. Caught on the real +machine, not in tests: the dedup now compares the detail as well as the headline. diff --git a/cli/core/diagnostics.js b/cli/core/diagnostics.js index 2ffcd57..006e70f 100644 --- a/cli/core/diagnostics.js +++ b/cli/core/diagnostics.js @@ -371,9 +371,14 @@ export function deriveIssues(collected) { } if (nativeConfig.available && nativeConfig.valid === false) { + // The headline stays a sentence the operator can act on; the validator's own text is + // detail, not the title. Surfacing it raw produced findings titled + // "JSON5 parse failed: SyntaxError: JSON5: invalid character 'i' at 1:8". + const configDetail = nativeConfig.errors[0]?.message || ''; issues.push({ severity: 'high', - text: nativeConfig.errors[0]?.message || 'OpenClaw config schema validation failed', + text: 'OpenClaw config schema validation failed', + description: configDetail || 'OpenClaw reported the config as invalid without detail.', source: 'openclaw-config', nativeCheckId: 'config/schema-invalid', path: nativeConfig.errors[0]?.path || null, @@ -385,9 +390,16 @@ export function deriveIssues(collected) { && nativeStatus.gateway.reachable === false && !issues.some((issue) => /gateway.*not running|gateway.*unreachable/i.test(issue.text)) ) { + // Same rule as the config case above. `gateway.error` is a machine string — when the probe + // timed out it was literally "timeout", which reached the user as a critical finding titled + // "timeout" and read like an OpenClaw fault rather than a probe that gave up. + const probeDetail = String(nativeStatus.gateway.error || '').trim(); issues.push({ severity: 'critical', - text: nativeStatus.gateway.error || 'OpenClaw gateway is unreachable', + text: 'OpenClaw gateway is unreachable', + description: probeDetail + ? `Gateway probe failed: ${probeDetail}` + : 'The gateway did not answer a status probe.', source: 'openclaw-status', nativeCheckId: 'status/gateway-unreachable', }); @@ -423,9 +435,14 @@ export function deriveIssues(collected) { } for (const finding of nativeDoctor.findings) { + // Match on the detail too, not just the headline. Findings that carry an upstream message as + // their detail (config/schema-invalid holds the validator's text) would otherwise be + // reported twice — once with an actionable title, once as the raw parser output. + const message = String(finding.message || '').toLowerCase(); const duplicate = issues.some((issue) => ( issue.nativeCheckId === finding.checkId - || issue.text.toLowerCase() === finding.message.toLowerCase() + || String(issue.text || '').toLowerCase() === message + || String(issue.description || '').toLowerCase() === message )); if (duplicate) continue; issues.push({ diff --git a/cli/core/repair-catalog.js b/cli/core/repair-catalog.js index 55ab7c5..4475ac1 100644 --- a/cli/core/repair-catalog.js +++ b/cli/core/repair-catalog.js @@ -108,60 +108,105 @@ function configFlag(value) { return null; } -const autoUpdateEnabled = Object.freeze({ +/** + * A repair that flips one boolean OpenClaw config key to `target`. + * + * Every step goes through the OpenClaw CLI as argv — read the current value, set it, read it + * back. Verification is the read-back, so a `config set` that reported success but changed + * nothing still fails. A key OpenClaw cannot report is refused rather than assumed: unreadable + * is not the same as false. + */ +function configToggleRepair({ id, key, target, title, description, blockedReason, risk = 'low' }) { + const targetText = String(target); + const previousText = String(!target); + + return Object.freeze({ + id, + key, + title, + description, + risk, + + async preflight(ctx) { + const current = await ctx.openclaw.configGet(key, { timeoutMs: 10_000 }); + const flag = configFlag(current); + if (flag === null) { + return Object.freeze({ ok: false, reason: 'config_state_unknown', evidence: { key, current } }); + } + if (flag === target) { + return Object.freeze({ ok: false, reason: blockedReason, evidence: { key, current } }); + } + return Object.freeze({ ok: true, evidence: { key, current } }); + }, + + async preview() { + return Object.freeze({ + steps: Object.freeze([ + `Read ${key} through the OpenClaw CLI (argv, no shell).`, + `Set ${key} to ${targetText}.`, + 'Read it back to confirm the change actually landed.', + ]), + summary: `openclaw config set ${key} ${targetText}`, + }); + }, + + async apply(ctx) { + const result = await ctx.openclaw.configSet(key, targetText, { timeoutMs: 30_000 }); + return Object.freeze({ + status: result.status, + timedOut: result.timedOut, + errorSummary: result.errorSummary, + }); + }, + + async verify(ctx) { + const current = await ctx.openclaw.configGet(key, { timeoutMs: 10_000 }); + return Object.freeze({ ok: configFlag(current) === target, evidence: { key, current } }); + }, + + async rollback(ctx) { + const result = await ctx.openclaw.configSet(key, previousText, { timeoutMs: 30_000 }); + return Object.freeze({ + rolledBack: result.status === 0, + note: result.status === 0 + ? `Restored ${key} to ${previousText}.` + : `Could not restore ${key}; check \`openclaw config get ${key}\`.`, + }); + }, + }); +} + +const autoUpdateEnabled = configToggleRepair({ id: 'auto-update-enabled-warning', + key: 'update.auto.enabled', + target: false, title: 'Disable OpenClaw auto-update', description: 'Auto-update restarts the gateway on its own schedule, which is the documented cause of ' + 'restart loops. This turns it off; updates then happen when you run them.', - risk: 'low', - - async preflight(ctx) { - const current = await ctx.openclaw.configGet('update.auto.enabled', { timeoutMs: 10_000 }); - const enabled = configFlag(current); - if (enabled === null) { - return Object.freeze({ ok: false, reason: 'auto_update_state_unknown', evidence: { current } }); - } - if (enabled === false) { - return Object.freeze({ ok: false, reason: 'auto_update_already_disabled', evidence: { current } }); - } - return Object.freeze({ ok: true, evidence: { current } }); - }, - - async preview() { - return Object.freeze({ - steps: Object.freeze([ - 'Read update.auto.enabled through the OpenClaw CLI (argv, no shell).', - 'Set update.auto.enabled to false.', - 'Read it back to confirm it is off.', - ]), - summary: 'openclaw config set update.auto.enabled false', - }); - }, - - async apply(ctx) { - const result = await ctx.openclaw.configSet('update.auto.enabled', 'false', { timeoutMs: 30_000 }); - return Object.freeze({ - status: result.status, - timedOut: result.timedOut, - errorSummary: result.errorSummary, - }); - }, + blockedReason: 'auto_update_already_disabled', +}); - async verify(ctx) { - const current = await ctx.openclaw.configGet('update.auto.enabled', { timeoutMs: 10_000 }); - return Object.freeze({ ok: configFlag(current) === false, evidence: { current } }); - }, +const hybridSearchDisabled = configToggleRepair({ + id: 'no-hybrid-search', + key: 'agents.defaults.memorySearch.query.hybrid.enabled', + target: true, + title: 'Enable hybrid memory search', + description: + 'Hybrid search combines keyword and semantic matching when the agent searches memory. ' + + 'OpenClaw recommends it; without it recall is keyword-only.', + blockedReason: 'hybrid_search_already_enabled', +}); - async rollback(ctx) { - const result = await ctx.openclaw.configSet('update.auto.enabled', 'true', { timeoutMs: 30_000 }); - return Object.freeze({ - rolledBack: result.status === 0, - note: result.status === 0 - ? 'Restored update.auto.enabled to true.' - : 'Could not restore update.auto.enabled; check `openclaw config get update.auto.enabled`.', - }); - }, +const memoryFlushDisabled = configToggleRepair({ + id: 'no-memory-flush', + key: 'agents.defaults.compaction.memoryFlush.enabled', + target: true, + title: 'Enable memory flush on compaction', + description: + 'Without a memory flush, anything the agent has not written down is lost when the context ' + + 'is compacted. This makes compaction persist memory first.', + blockedReason: 'memory_flush_already_enabled', }); const gatewayLoopbackNoAuth = Object.freeze({ @@ -234,4 +279,6 @@ export const repairCatalog = Object.freeze({ 'gateway-not-running': gatewayNotRunning, 'auto-update-enabled-warning': autoUpdateEnabled, 'gateway-loopback-no-auth': gatewayLoopbackNoAuth, + 'no-hybrid-search': hybridSearchDisabled, + 'no-memory-flush': memoryFlushDisabled, }); diff --git a/test/diagnostic-core.test.js b/test/diagnostic-core.test.js index 9d51964..42772f3 100644 --- a/test/diagnostic-core.test.js +++ b/test/diagnostic-core.test.js @@ -1844,7 +1844,9 @@ test('deriveIssues golden: comprehensive representative facts produce the exact }, { severity: 'high', - text: 'Gateway port must be an integer', + // The validator's own message is detail; the headline stays actionable. + text: 'OpenClaw config schema validation failed', + description: 'Gateway port must be an integer', source: 'openclaw-config', nativeCheckId: 'config/schema-invalid', path: 'gateway.port', @@ -2718,3 +2720,62 @@ test('non-Error external abort reasons use DiagnosticsAbortError', async () => { ); assert.equal(fake.clearCalls.length, 1); }); + +// ============================================================ +// Raw upstream error strings must not become finding titles +// ============================================================ + +test('a gateway probe timeout is reported as unreachable, not as a finding titled "timeout"', () => { + // A competing port owner suppresses the generic not-running issue, so the unreachable + // branch is the one under test here. + const issues = deriveIssues(fakeCollected({ + gateway: { gatewayStatus: 'not running', gatewayPort: 4321, gatewayPid: '' }, + portsGateway: { + valid: true, available: true, listening: true, process: 'some-other-process', pid: 777, collector: 'lsof', + }, + nativeStatus: { available: true, gateway: { reachable: false, error: 'timeout' } }, + })); + + const unreachable = issues.find(issue => issue.nativeCheckId === 'status/gateway-unreachable'); + assert.ok(unreachable, 'expected an unreachable finding'); + assert.equal(unreachable.text, 'OpenClaw gateway is unreachable'); + assert.match(unreachable.description, /Gateway probe failed: timeout/); + // The bare machine string must never be the headline. + assert.equal(issues.some(issue => issue.text === 'timeout'), false); +}); + +test('an invalid config keeps the parser message as detail, not as the title', () => { + const issues = deriveIssues(fakeCollected({ + nativeConfig: { + available: true, + valid: false, + errors: [{ message: "JSON5 parse failed: SyntaxError: JSON5: invalid character 'i' at 1:8", path: 'gateway' }], + }, + })); + + const invalid = issues.find(issue => issue.nativeCheckId === 'config/schema-invalid'); + assert.ok(invalid, 'expected a config finding'); + assert.equal(invalid.text, 'OpenClaw config schema validation failed'); + assert.match(invalid.description, /JSON5 parse failed/); + assert.equal(issues.some(issue => /^JSON5 parse failed/.test(issue.text)), false); +}); + +test('a doctor finding that repeats an already-reported detail is not listed twice', () => { + const parserMessage = "JSON5 parse failed: SyntaxError: JSON5: invalid character 'i' at 1:8"; + const issues = deriveIssues(fakeCollected({ + nativeConfig: { + available: true, + valid: false, + errors: [{ message: parserMessage, path: 'gateway' }], + }, + nativeDoctor: { + available: true, + findings: [{ checkId: 'core/doctor/config', severity: 'error', message: parserMessage, path: 'gateway' }], + }, + })); + + const mentions = issues.filter(issue => + issue.text === parserMessage || issue.description === parserMessage); + assert.equal(mentions.length, 1, 'the parser message must appear once'); + assert.equal(mentions[0].text, 'OpenClaw config schema validation failed'); +}); diff --git a/test/repair-catalog.test.js b/test/repair-catalog.test.js index 20d9cf6..e00a2a7 100644 --- a/test/repair-catalog.test.js +++ b/test/repair-catalog.test.js @@ -214,7 +214,7 @@ test('auto-update repair refuses to act on an unreadable flag rather than guessi const { ctx } = configCtx({}); const pre = await entry.preflight(ctx); assert.equal(pre.ok, false); - assert.equal(pre.reason, 'auto_update_state_unknown'); + assert.equal(pre.reason, 'config_state_unknown'); }); test('auto-update verify fails when the value did not actually change', async () => { @@ -283,3 +283,62 @@ test('the catalog exposes exactly the repairs the findings map can authorize', a assert.match(findings, new RegExp(`'${id.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}'`), `${id} is unmapped`); } }); + +// ============================================================ +// Config toggle repairs share one contract +// ============================================================ + +const TOGGLES = [ + { id: 'auto-update-enabled-warning', key: 'update.auto.enabled', from: 'true', to: 'false' }, + { id: 'no-hybrid-search', key: 'agents.defaults.memorySearch.query.hybrid.enabled', from: 'false', to: 'true' }, + { id: 'no-memory-flush', key: 'agents.defaults.compaction.memoryFlush.enabled', from: 'false', to: 'true' }, +]; + +for (const toggle of TOGGLES) { + test(`${toggle.id}: flips ${toggle.key} and verifies by reading it back`, async () => { + const entry = repairCatalog[toggle.id]; + const { ctx, store } = configCtx({ [toggle.key]: toggle.from }); + + assert.equal((await entry.preflight(ctx)).ok, true); + assert.equal((await entry.apply(ctx)).status, 0); + assert.equal(store[toggle.key], toggle.to); + assert.equal((await entry.verify(ctx)).ok, true); + }); + + test(`${toggle.id}: is blocked when the key is already correct`, async () => { + const entry = repairCatalog[toggle.id]; + const { ctx } = configCtx({ [toggle.key]: toggle.to }); + assert.equal((await entry.preflight(ctx)).ok, false); + }); + + test(`${toggle.id}: refuses an unreadable key instead of treating it as false`, async () => { + const entry = repairCatalog[toggle.id]; + const { ctx } = configCtx({}); + const pre = await entry.preflight(ctx); + assert.equal(pre.ok, false); + assert.equal(pre.reason, 'config_state_unknown'); + }); + + test(`${toggle.id}: verify fails when the set did not land`, async () => { + const entry = repairCatalog[toggle.id]; + const { ctx } = configCtx({ [toggle.key]: toggle.from }, { setStatus: 1 }); + await entry.apply(ctx); + assert.equal((await entry.verify(ctx)).ok, false); + }); + + test(`${toggle.id}: rollback restores the original value`, async () => { + const entry = repairCatalog[toggle.id]; + const { ctx, store } = configCtx({ [toggle.key]: toggle.from }); + await entry.apply(ctx); + assert.equal((await entry.rollback(ctx)).rolledBack, true); + assert.equal(store[toggle.key], toggle.from); + }); + + test(`${toggle.id}: preview names the exact command and has no side effects`, async () => { + const entry = repairCatalog[toggle.id]; + const { ctx, calls } = configCtx({ [toggle.key]: toggle.from }); + const preview = await entry.preview(ctx); + assert.match(preview.summary, new RegExp(`openclaw config set ${toggle.key.replace(/\./g, '\\.')} ${toggle.to}`)); + assert.equal(calls.some(([verb]) => verb === 'set'), false); + }); +}