diff --git a/REVIEW.md b/REVIEW.md index 05e10b5..7f474b9 100644 --- a/REVIEW.md +++ b/REVIEW.md @@ -656,3 +656,60 @@ invest next — port-conflict and config-invalid are both detected precisely eno - Raw parser output is used as a user-facing title: `JSON5 parse failed: SyntaxError: JSON5: invalid character 'i' at 1:8`. Accurate, but it is a stack-trace fragment where a sentence belongs. + + +--- + +# Fourth pass — the repair catalog, and driving the real UI + +## Two repairs added: ClawFix now fixes what it finds + +The catalog held one repair that needed a service manager. Two more were added, both using +OpenClaw's own supported commands and both verified against a real OpenClaw 2026.6.11 install: + +| Repair | Does | Verified by | Risk | +|---|---|---|---| +| `auto-update-enabled-warning` | `openclaw config set update.auto.enabled false` | reads the key back through `config get` | low | +| `gateway-loopback-no-auth` | sets `gateway.auth.mode` to `token`, then `doctor --fix --generate-gateway-token` | reads the mode back; never reads the token itself | medium | + +Repairable findings went from 1 to 3. On a real install, broken deliberately: + +``` +auto-update: before "true" → applied, verify {"ok":true,"current":"false"} → finding gone +gateway auth: before "none" → applied, verify {"ok":true,"mode":"token"} → finding gone +``` + +`gateway-loopback-no-auth` is deliberately medium risk, not low: existing clients stop working +until they carry the new token, and the preview says so. + +Verification reads one config key through OpenClaw rather than parsing `openclaw.json`, so repair +evidence stays narrow — and the auth repair never pulls the token into a repair record. + +## Driving the real UI found two defects + +The compiled musl binary was driven through a PTY with a terminal emulator attached, on a real +broken install. Both of these were invisible to unit tests and frame captures. + +**The sidebar's numbers did not match the numbers `fix <#>` accepts.** The sidebar sorted findings +by severity and renumbered its own view 1–4, while `fix <#>` and `explain <#>` index the unsorted +findings list. Reading *"3. Auto-update enabled"* and typing `fix 3` reached an advisory finding +and answered "This finding has no reviewed automatic repair." The sidebar now carries each +finding's real position — criticals still lead, but the numbers are the ones the commands take. + +**The status line printed the revision twice**, which pushed the finding count off the end: + +``` +before: 🦞 ClawFix v0.11.2 · revision f3e9479b-… · Revision f3e9479b-… +after: 🦞 ClawFix v0.11.2 · Revision f3e9479b-b52c-456e-8d0f-f5f25ba3b2e4 · 6 findings · AI consent required +``` + +The bridge's status already begins with the revision; the extra prefix was redundant. + +## End to end through the UI, on a real machine + +Sidebar listed *"4. Gateway auth missing on loopback"* → typed `fix 4` → approval dialog showed +`Risk: medium · gateway-loopback-no-auth` with the composer locked and focus defaulting to +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. diff --git a/cli/adapters/openclaw.js b/cli/adapters/openclaw.js index 106f7b6..0a868cb 100644 --- a/cli/adapters/openclaw.js +++ b/cli/adapters/openclaw.js @@ -484,6 +484,26 @@ export function createOpenClawAdapter({ npmVersion(options = {}) { return successfulText('npm', ['--version'], options); }, + + /** + * Read one config key through OpenClaw itself. + * + * Repairs verify against this rather than parsing openclaw.json: it is the value OpenClaw + * resolves, and it keeps repair evidence to a single key instead of a whole config blob. + * Returns '' when the key is unset or the call fails — callers must not read that as false. + */ + async configGet(key, options = {}) { + if (typeof key !== 'string' || !/^[A-Za-z0-9_.-]{1,128}$/.test(key)) return ''; + return processText(await invoke(['config', 'get', key], options)); + }, + + /** Set one config key. Values are passed as literal argv, never through a shell. */ + async configSet(key, value, options = {}) { + if (typeof key !== 'string' || !/^[A-Za-z0-9_.-]{1,128}$/.test(key)) { + return Object.freeze({ status: 1, errorSummary: 'invalid config key' }); + } + return invoke(['config', 'set', key, String(value)], options); + }, /** * PIDs that plausibly belong to a running gateway *server*. * diff --git a/cli/core/findings.js b/cli/core/findings.js index 1cb5809..e39f8fe 100644 --- a/cli/core/findings.js +++ b/cli/core/findings.js @@ -31,6 +31,7 @@ const LOCAL_KNOWN_ISSUE_ID_REPAIR_MAP = new Map([]); // Explicit native checkId -> repairId map (exact checkId equality only). const NATIVE_CHECK_ID_REPAIR_MAP = new Map([ ['runtime/gateway-port-conflict', 'port-conflict'], + ['gateway.loopback_no_auth', 'gateway-loopback-no-auth'], ]); function slug(value) { diff --git a/cli/core/repair-catalog.js b/cli/core/repair-catalog.js index bb6e6f9..55ab7c5 100644 --- a/cli/core/repair-catalog.js +++ b/cli/core/repair-catalog.js @@ -100,6 +100,138 @@ const gatewayNotRunning = Object.freeze({ }, }); +/** Normalized truthiness of an `openclaw config get` result. '' means unset or unreadable. */ +function configFlag(value) { + const text = String(value ?? '').trim().toLowerCase(); + if (text === 'true') return true; + if (text === 'false') return false; + return null; +} + +const autoUpdateEnabled = Object.freeze({ + id: 'auto-update-enabled-warning', + 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, + }); + }, + + async verify(ctx) { + const current = await ctx.openclaw.configGet('update.auto.enabled', { timeoutMs: 10_000 }); + return Object.freeze({ ok: configFlag(current) === false, evidence: { current } }); + }, + + 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 gatewayLoopbackNoAuth = Object.freeze({ + id: 'gateway-loopback-no-auth', + title: 'Require a token on the gateway', + description: + 'The gateway accepts unauthenticated connections. This switches auth to token mode and has ' + + 'OpenClaw generate one. Clients will need that token to connect afterwards.', + // Medium, not low: existing clients stop working until they carry the new token. + risk: 'medium', + + async preflight(ctx) { + const mode = await ctx.openclaw.configGet('gateway.auth.mode', { timeoutMs: 10_000 }); + const current = String(mode || '').trim().toLowerCase(); + if (current === 'token' || current === 'password' || current === 'trusted-proxy') { + return Object.freeze({ ok: false, reason: 'gateway_auth_already_enabled', evidence: { mode: current } }); + } + return Object.freeze({ ok: true, evidence: { mode: current || '(unset)' } }); + }, + + async preview() { + return Object.freeze({ + steps: Object.freeze([ + 'Set gateway.auth.mode to token through the OpenClaw CLI (argv, no shell).', + 'Run `openclaw doctor --fix --generate-gateway-token` so OpenClaw generates the token.', + 'Read gateway.auth.mode back to confirm token auth is active.', + 'Restart the gateway yourself for it to take effect; existing clients need the new token.', + ]), + summary: 'openclaw config set gateway.auth.mode token + doctor --generate-gateway-token', + }); + }, + + async apply(ctx) { + const set = await ctx.openclaw.configSet('gateway.auth.mode', 'token', { timeoutMs: 30_000 }); + if (set.status !== 0) { + return Object.freeze({ status: set.status, stage: 'set-mode', errorSummary: set.errorSummary }); + } + const generated = await ctx.openclaw.invoke( + ['doctor', '--fix', '--generate-gateway-token'], + { timeoutMs: 120_000 }, + ); + return Object.freeze({ + status: generated.status, + stage: 'generate-token', + timedOut: generated.timedOut, + errorSummary: generated.errorSummary, + }); + }, + + async verify(ctx) { + // Evidence is the mode only — never read the token itself into a repair record. + const mode = String(await ctx.openclaw.configGet('gateway.auth.mode', { timeoutMs: 10_000 })).trim(); + return Object.freeze({ ok: mode.toLowerCase() === 'token', evidence: { mode } }); + }, + + async rollback(ctx, { applyResult } = {}) { + if (applyResult?.stage === 'set-mode') { + return Object.freeze({ rolledBack: false, note: 'Auth mode was never changed.' }); + } + return Object.freeze({ + rolledBack: false, + note: 'Gateway auth was switched to token mode. To undo it deliberately, run ' + + '`openclaw config set gateway.auth.mode none` — that returns the gateway to accepting ' + + 'unauthenticated connections.', + }); + }, +}); + export const repairCatalog = Object.freeze({ 'gateway-not-running': gatewayNotRunning, + 'auto-update-enabled-warning': autoUpdateEnabled, + 'gateway-loopback-no-auth': gatewayLoopbackNoAuth, }); diff --git a/cli/tui/src/app.tsx b/cli/tui/src/app.tsx index fcd082f..c0845c1 100644 --- a/cli/tui/src/app.tsx +++ b/cli/tui/src/app.tsx @@ -174,8 +174,9 @@ export function App(props: AppProps) { const statusLine = () => { const state = current() - const rev = state.revision ? ` · revision ${state.revision}` : "" - const full = `🦞 ClawFix v${cliPackage.version}${rev} · ${state.status}` + // The bridge's status already begins with "Revision · N findings", so adding the + // revision here printed it twice and pushed the finding count off the end of the line. + const full = `🦞 ClawFix v${cliPackage.version} · ${state.status}` const budget = dims().width - 4 if (budget <= 0) return "" if (full.length <= budget) return full diff --git a/cli/tui/src/components/sidebar.tsx b/cli/tui/src/components/sidebar.tsx index 0c18c20..e2f6a59 100644 --- a/cli/tui/src/components/sidebar.tsx +++ b/cli/tui/src/components/sidebar.tsx @@ -32,9 +32,14 @@ export function Sidebar(props: SidebarProps) { } return [...bySeverity.entries()].sort((a, b) => severityRank(a[0]) - severityRank(b[0])) } + // Carry each finding's real position before sorting. `fix <#>` and `explain <#>` index the + // unsorted findings list, so a sidebar that renumbered its own severity-sorted view sent + // users to the wrong finding — reading "3. Auto-update enabled" and typing `fix 3` hit an + // advisory finding and answered "this finding has no reviewed automatic repair". const top = () => - [...(props.findings || [])] - .sort((a, b) => severityRank(a.severity) - severityRank(b.severity)) + (props.findings || []) + .map((finding, index) => ({ finding, position: index + 1 })) + .sort((a, b) => severityRank(a.finding.severity) - severityRank(b.finding.severity)) .slice(0, 4) const aiLabel = () => props.aiMode === "remote" ? "Remote" : props.aiMode === "remote-pending" ? "Remote (pending)" : "Local only" @@ -69,8 +74,8 @@ export function Sidebar(props: SidebarProps) { {top().length > 0 && Top issues} - {top().map((f, i) => ( - {`${i + 1}. ${f.title}`} + {top().map((entry) => ( + {`${entry.position}. ${entry.finding.title}`} ))} {top().length > 0 && } diff --git a/cli/tui/test/sidebar-numbering.test.tsx b/cli/tui/test/sidebar-numbering.test.tsx new file mode 100644 index 0000000..4665ca1 --- /dev/null +++ b/cli/tui/test/sidebar-numbering.test.tsx @@ -0,0 +1,73 @@ +import { afterEach, describe, expect, test } from "bun:test" + +import { testRender } from "@opentui/solid" + +import { App, createFakeSession } from "../src/app" +import type { TuiSessionView } from "../src/session-bridge" + +const renderers: Array<{ destroy(): void }> = [] +afterEach(() => { while (renderers.length) renderers.pop()?.destroy() }) + +/** Findings in the order the session produces them — which is what `fix <#>` indexes. */ +const FINDINGS = [ + { id: "f1", title: "Gateway is not running", severity: "critical", repairable: true, repairId: "gateway-not-running" }, + { id: "f2", title: "Auto-update enabled", severity: "medium", repairable: true, repairId: "auto-update-enabled-warning" }, + { id: "f3", title: "Reverse proxy headers are not trusted", severity: "medium", repairable: false, repairId: null }, + { id: "f4", title: "Gateway auth missing on loopback", severity: "critical", repairable: true, repairId: "gateway-loopback-no-auth" }, +] + +async function frame(partial: Partial, width = 110, height = 34) { + const view = Object.freeze({ ...createFakeSession(), ...partial }) as TuiSessionView + const setup = await testRender(() => , { width, height }) + renderers.push(setup.renderer) + await setup.renderOnce() + return setup.captureCharFrame() +} + +describe("sidebar numbering matches the fix/explain selectors", () => { + test("each listed issue carries its position in the findings list, not its sorted rank", async () => { + const out = await frame({ findings: FINDINGS as any, revision: "rev-1" }) + + // Sorted by severity for prominence: the two criticals lead. But the numbers must stay + // the finding's real index, because `fix 4` has to reach "Gateway auth missing". + // The sidebar wraps, so match the numbered prefix rather than the full title. + expect(out).toContain("1. Gateway is not") + expect(out).toContain("4. Gateway auth") + expect(out).toContain("2. Auto-update enabled") + expect(out).toContain("3. Reverse proxy") + + // The old bug: severity-sorted renumbering labelled the auth finding "2". + expect(out).not.toContain("2. Gateway auth") + expect(out).not.toContain("3. Auto-update enabled") + }) + + test("severity ordering still puts criticals first", async () => { + const out = await frame({ findings: FINDINGS as any, revision: "rev-1" }) + const rows = out.split("\n") + const idx = (needle: string) => rows.findIndex((r) => r.includes(needle)) + expect(idx("4. Gateway auth")).toBeLessThan(idx("2. Auto-update enabled")) + }) +}) + +describe("status line", () => { + test("does not print the revision twice", async () => { + const out = await frame({ + findings: FINDINGS as any, + revision: "aee53dec-5ed0-46e0-a300-c44bf5b0aa28", + status: "Revision aee53dec-5ed0-46e0-a300-c44bf5b0aa28 · 4 findings", + }) + const status = out.split("\n").find((r) => r.includes("ClawFix v")) || "" + const occurrences = (status.match(/aee53dec/g) || []).length + expect(occurrences).toBe(1) + }) + + test("keeps the finding count visible instead of truncating it away", async () => { + const out = await frame({ + findings: FINDINGS as any, + revision: "aee53dec-5ed0-46e0-a300-c44bf5b0aa28", + status: "Revision aee53dec-5ed0-46e0-a300-c44bf5b0aa28 · 4 findings", + }) + const status = out.split("\n").find((r) => r.includes("ClawFix v")) || "" + expect(status).toContain("4 findings") + }) +}) diff --git a/test/repair-catalog.test.js b/test/repair-catalog.test.js index ac21f54..20d9cf6 100644 --- a/test/repair-catalog.test.js +++ b/test/repair-catalog.test.js @@ -161,3 +161,125 @@ test('without a port probe the filtered PID evidence is used', async () => { assert.equal(result.ok, true); assert.equal(result.evidence.listening, null); }); + +// ============================================================ +// auto-update-enabled-warning +// ============================================================ + +function configCtx(values, { setStatus = 0, invokeStatus = 0 } = {}) { + const store = { ...values }; + const calls = []; + return { + calls, + store, + ctx: { + openclaw: { + async configGet(key) { calls.push(['get', key]); return store[key] ?? ''; }, + async configSet(key, value) { + calls.push(['set', key, value]); + if (setStatus === 0) store[key] = String(value); + return { status: setStatus }; + }, + async invoke(argv) { calls.push(['invoke', argv.join(' ')]); return { status: invokeStatus }; }, + async gatewayStatusText() { return ''; }, + async gatewayProcesses() { return ''; }, + async gatewayListening() { return false; }, + }, + wait: async () => {}, + }, + }; +} + +test('auto-update repair turns the flag off and verifies by reading it back', async () => { + const entry = repairCatalog['auto-update-enabled-warning']; + const { ctx, store } = configCtx({ 'update.auto.enabled': 'true' }); + + assert.equal((await entry.preflight(ctx)).ok, true); + const applied = await entry.apply(ctx); + assert.equal(applied.status, 0); + assert.equal(store['update.auto.enabled'], 'false'); + assert.equal((await entry.verify(ctx)).ok, true); +}); + +test('auto-update repair is blocked when it is already disabled', async () => { + const entry = repairCatalog['auto-update-enabled-warning']; + const { ctx } = configCtx({ 'update.auto.enabled': 'false' }); + const pre = await entry.preflight(ctx); + assert.equal(pre.ok, false); + assert.equal(pre.reason, 'auto_update_already_disabled'); +}); + +test('auto-update repair refuses to act on an unreadable flag rather than guessing', async () => { + const entry = repairCatalog['auto-update-enabled-warning']; + const { ctx } = configCtx({}); + const pre = await entry.preflight(ctx); + assert.equal(pre.ok, false); + assert.equal(pre.reason, 'auto_update_state_unknown'); +}); + +test('auto-update verify fails when the value did not actually change', async () => { + const entry = repairCatalog['auto-update-enabled-warning']; + const { ctx } = configCtx({ 'update.auto.enabled': 'true' }, { setStatus: 1 }); + await entry.apply(ctx); + assert.equal((await entry.verify(ctx)).ok, false); +}); + +test('auto-update rollback restores the previous value', async () => { + const entry = repairCatalog['auto-update-enabled-warning']; + const { ctx, store } = configCtx({ 'update.auto.enabled': 'true' }); + await entry.apply(ctx); + const back = await entry.rollback(ctx); + assert.equal(back.rolledBack, true); + assert.equal(store['update.auto.enabled'], 'true'); +}); + +// ============================================================ +// gateway-loopback-no-auth +// ============================================================ + +test('gateway auth repair sets token mode and has OpenClaw generate the token', async () => { + const entry = repairCatalog['gateway-loopback-no-auth']; + const { ctx, calls, store } = configCtx({ 'gateway.auth.mode': 'none' }); + + assert.equal((await entry.preflight(ctx)).ok, true); + const applied = await entry.apply(ctx); + assert.equal(applied.status, 0); + assert.equal(store['gateway.auth.mode'], 'token'); + assert.deepEqual(calls.at(-1), ['invoke', 'doctor --fix --generate-gateway-token']); + assert.equal((await entry.verify(ctx)).ok, true); +}); + +test('gateway auth repair never reads the token into its evidence', async () => { + const entry = repairCatalog['gateway-loopback-no-auth']; + const { ctx, calls } = configCtx({ 'gateway.auth.mode': 'none' }); + await entry.apply(ctx); + const verify = await entry.verify(ctx); + + assert.equal(JSON.stringify(verify).includes('token.'), false); + assert.equal(calls.some(([, key]) => String(key).includes('auth.token')), false); +}); + +test('gateway auth repair is blocked when auth is already required', async () => { + const entry = repairCatalog['gateway-loopback-no-auth']; + for (const mode of ['token', 'password', 'trusted-proxy']) { + const { ctx } = configCtx({ 'gateway.auth.mode': mode }); + const pre = await entry.preflight(ctx); + assert.equal(pre.ok, false, mode); + assert.equal(pre.reason, 'gateway_auth_already_enabled'); + } +}); + +test('gateway auth repair carries medium risk and says clients need the new token', async () => { + const entry = repairCatalog['gateway-loopback-no-auth']; + assert.equal(entry.risk, 'medium'); + const preview = await entry.preview(); + assert.match(preview.steps.join(' '), /existing clients need the new token/i); +}); + +test('the catalog exposes exactly the repairs the findings map can authorize', async () => { + const { default: fs } = await import('node:fs/promises'); + const findings = await fs.readFile(new URL('../cli/core/findings.js', import.meta.url), 'utf8'); + for (const id of Object.keys(repairCatalog)) { + assert.match(findings, new RegExp(`'${id.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}'`), `${id} is unmapped`); + } +});