Review + upgrade pass: fix 4 blockers, harden repairs, make the gates able to fail - #21
Merged
Conversation
added 15 commits
July 25, 2026 04:37
The hosted agent emits repair.proposed {repairId, rationale} and never sends a
plan, but the bridge fed that event straight into planFromRaw(), which returns
null without a planId. Every remote repair proposal was silently dropped: no
card, no approval dialog, no error, and session.proposeRepair() was never called.
Server repair IDs are now resolved against the current scan's findings and the
local repair engine builds the plan, so planId, approval token, risk and preview
always come from this machine. A server-authored plan object can no longer reach
the approval dialog, and an unmatched repair ID surfaces an error instead of
vanishing.
Also:
- Ctrl+D quits and Ctrl+C quits when idle. The renderer holds stdin in raw mode
with exitOnCtrlC disabled, so SIGINT never arrived and the session could not be
exited at all. Key decisions move to a pure resolveGlobalKeyAction().
- A thrown scan no longer keeps the previous revision's findings alongside the
new revision and its scanError.
- The autoscan rejection is caught, so a failing first scan renders as a scan
error instead of tripping unhandledRejection and destroying the renderer.
/fix/sha256 tells users to compare the served hash against blob/main/SCRIPT_HASH, but the committed file was last touched in 6aa3908 while the served script has changed across several releases. It read 4470a867 while /fix serves c8f29823, so anyone performing the verification honestly got a mismatch — and the only lesson available from a check that always fails is to stop checking. Regenerates the file, exports DIAGNOSTIC_SCRIPT/SCRIPT_HASH from the route, adds `npm run hash:script`, and adds a test that fails when the two diverge.
…load Agreeing to a remote turn also POSTs the full redacted diagnostic to /api/diagnose before the agent call, but the consent dialog previewed only the agent-v2 body — with diagnosticId: null, a "pending-session" conversation id and a hardcoded risk of "medium" for every repair. The one screen whose whole job is to say what leaves the machine was showing something other than what is sent. The analyzer now describes its own outbound traffic, so the preview is built from the same conversationId and availableRepairs() the request uses and cannot drift. The dialog names the /api/diagnose endpoint explicitly and drops the "when present on a linked diagnostic" hedge on turns that create that diagnostic. Also replaces the fabricated disclosure in new-layout.test.tsx with the real builder, so the small-terminal test renders shipping copy.
cli/package.json allowlists bin/, adapters/, core/ and interfaces/ — never tui/ — so the published package contains zero TUI files and `npx clawfix --tui` spawned Bun against a cwd that does not exist. Node reported that as `spawn /path/to/bun ENOENT`, sending users to install a Bun they already had. Keeps the npm package a portable, Bun-free CLI (the standalone binary remains the way to get the chat UI) and detects the missing entry before spawning, pointing at the release download and at `npx clawfix --plain`. Site and --help no longer describe the OpenTUI session as what npx gives you. Also stops treating a non-zero TUI exit as "never launched", which dropped the user into a second full plain session after the TUI had already run.
… failures High/critical refusal lived only in the TUI approval handler, so it depended on which UI happened to be driving. It moves to applyPlan, the one layer every caller goes through — nothing runs, not even preflight. preflight, preview, verify and rollback were also unwrapped while apply was guarded, so a throw after the repair had already run rejected the promise and discarded applyResult — losing exactly the fact the caller needs most, that the system was already changed. Each stage now returns a structured outcome, and a failed rollback is reported without masking the verify failure it followed.
…onsent copy
The footer is `${aiLabel} · ${hints}` and aiLabel ranges from 10 to 24 chars, so
the single `width <= 64` threshold could not hold: the long hint wrapped to two
rows for every width from 65 to 91 — the entire 80-column band — eating a
transcript row. resolveModelLine() now picks the longest hint tier that fits the
measured width. Verified across 303 renders (widths 40..140 x 3 aiModes): a
single-row footer everywhere.
The frame sweep could not have caught it. It sampled 140/120/100/80/60 — never
the broken band — and every state used the default "local" label, the shortest
one. Adds 88/72/66 widths and consent-pending/remote states.
The privacy dialog was also truncating its own Included/Not-included lines on an
80x24 terminal, hiding the disclosure being consented to. Consent-critical lines
now precede contextual ones, the dialog gets a larger row budget, and the
redundant Destination line (the host is already in the endpoint URL) is dropped.
The frames harness renders the real buildDisclosureView output rather than a
hand-written disclosure naming an endpoint that does not exist.
…f Linux agent-v2 kept per-conversation history in a process-local Map keyed by a client-supplied id, recorded createdAt and never read it. Nothing evicted, so memory grew with every fresh conversation id for the life of the process. Adds a TTL sweep and a hard cap with oldest-first eviction, both env-tunable. Two tests could only ever pass on Linux, so `npm test` and `bun test` were red on macOS for reasons unrelated to the product: - install-script.test.js built its fixture tarball with GNU `tar --transform`, which BSD tar rejects. The run() helper ignored exit codes, so the failure surfaced later as a confusing ENOENT on the missing tarball. Stages a literal package/ directory instead and asserts tar's exit code at the point of failure. - app.test.tsx read @opentui/core-linux-x64 from node_modules, which is only installed on Linux. Reads the host's native package instead; the linux-x64 release pin is still asserted, from metadata rather than from disk. Suite is now green on macOS: 364 node tests, 81 TUI tests.
The smoke asserted bytes-on-stdout and an exit code, so a binary that started, emitted escape codes and rendered nothing would pass — and it ran with TERM=dumb, under which the app paints nothing at all. It also degraded silently to the plain path whenever node-pty was absent, which is always in CI, so the interactive-exit coverage never ran. Now asserts the session UI is actually painted, runs with a real TERM, says plainly when PTY coverage is skipped, and supports CLAWFIX_TUI_REQUIRE_PTY=1 to turn that skip into a failure. Covered by a regression test that feeds it a binary which starts, emits escapes and renders nothing.
…pped through scan -> consent -> remote chat -> repair proposal -> approval -> apply -> verify, with everything on the client real: session controller, findings normalization, repair catalog and engine, offline analyzer, SSE client and TUI bridge. Only the OpenClaw process boundary is faked. The test server validates request bodies with the real validateAgentV2Request and writes frames with the real formatSseEvent, so the wire contract cannot drift from src/routes/agent-v2.js without this failing. A live server is not used because emitting repair.proposed needs a configured AI provider. Asserts the parts that were broken or unproven: deterministic commands and the pre-consent turn upload nothing, the outbound body carries no banned fields, the server's repairId becomes a locally-built plan with a local approval token, the approval defaults to Cancel, apply runs exactly `gateway restart`, success copy appears only after a verified apply, and a rescan confirms the gateway is up. Confirmed to fail against the pre-fix bridge.
- Session messages carry an id. The TUI keys transcript rows by it and, with no id present, minted a fresh key for every message on every projection — new keys each publish, and an id counter that only grew. - Drops the nativeChecks argument the session passed into normalizeFindings, which never accepted it. Native checks reach findings folded into localIssues with a nativeCheckId; the comment claiming a separate path is corrected. - Consumed repair plans are pruned above a retention bound instead of living for the life of the process; recent tokens are still rejected on replay. - Status line slices by code point, so truncation cannot cut the banner emoji in half. - cli/package.json engine floor raised to Node 22, matching the server and the site's stated requirement. - Adds a parity test between lib/disclosure.ts and cli/core/privacy.js. Two copies of the text a user consents to is exactly what drifts silently — the frames harness had already drifted to an endpoint that does not exist.
…locker Adds what was fixed and verified, the one item only partially fixed (B-2, which lands for the source path but not the compiled binary), and B-23: the bun --compile build renders the UI but receives no keyboard input at all, so the shipped binary cannot be typed into, answered, or quit. Also records that an earlier round of this investigation reached wrong conclusions from a PTY harness that was not draining the master.
… input The standalone binary rendered a correct first frame and then ignored every keystroke — it could not be typed into, no dialog could be answered, no repair approved, and it could not be quit. Root cause: the Solid JSX transform is registered through bunfig.toml's `preload`, which Bun applies to `bun run` and `bun test` but not to a spawned `bun build`. The binary was compiled with the plain automatic JSX runtime, so components mounted once and nothing was reactive. Compiles in-process via Bun.build with createSolidTransformPlugin() passed to the bundler. Verified through a PTY on the released launcher: typed text is echoed (132 bytes rendered), Ctrl+D and idle Ctrl+C exit in ~0.1s with code 0 and the terminal restored. The pre-fix binary produces zero bytes for the same input. Adds scripts/smoke-tui-interactive.mjs, which drives the real binary through a PTY and asserts it renders, accepts input, and quits — confirmed to fail against the pre-fix binary — and wires it into the release workflow with CLAWFIX_TUI_REQUIRE_PTY=1. Every previous gate passed on the broken binary because rendering was exactly what still worked.
…ng pgrep
Found on a real OpenClaw 2026.6.11 install. `pgrep -f 'openclaw.*gateway'` was
wrong in both directions:
- A gateway started by `openclaw gateway run` re-execs with the bare argv
`openclaw`, so the pattern never matched a real running gateway.
- ClawFix's own `openclaw gateway status` probe — which checkGatewayRunning
launches concurrently with the pgrep, right there in the same Promise.all —
always did match.
So the check reported "running" whenever ClawFix looked, and never because a
gateway was actually up. Two consequences on a real machine, both reproduced:
- verify() returned {ok: true} with nothing listening on 18789, which means a
repair that changed nothing could be reported as `applied` and the UI would
print "Verification passed against local detectors." That is the truthfulness
invariant failing in the one place it matters most.
- preflight() returned gateway_already_running while the gateway was down, so
the only repair ClawFix ships could never be applied.
The port is now the verdict — the same evidence the diagnostics core already
reports as portListening. Status prose and PIDs are still collected as evidence
for the caller, never as the verdict, and the PID probe now excludes ClawFix's
own sub-commands and this process. Falls back to filtered PIDs only when the
adapter has no port probe.
Verified against a real gateway: down -> verify ok:false / preflight ok:true;
up -> verify ok:true with the real server PID / preflight blocks correctly. Full
repair run reports verify_failed truthfully when `gateway gateway restart` cannot
start the service, and a rescan agrees.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Full review of the CLI, TUI, web service and site copy, then the upgrade pass.
REVIEW.mdat the repo root has every finding with file:line evidence and a concrete fix.
Blockers fixed
Remote repair proposals were silently dropped. The server emits
repair.proposed {repairId, rationale}and never a plan, but the bridge fed that straight intoplanFromRaw(), which returnsnullwithout aplanId. No card, no dialog, no error, andsession.proposeRepair()was never called. Server repair IDs now resolve against the currentscan's findings through the local repair engine, so planId, approval token, risk and preview
always come from the user's machine.
The guarded repair reported success without a running gateway. Found on a real OpenClaw
2026.6.11 install.
pgrep -f 'openclaw.*gateway'never matched a real gateway (its argv is justopenclaw) and always matched ClawFix's own concurrentopenclaw gateway statusprobe. Soverify()returnedok:truewith nothing listening on 18789 — a repair that changed nothingwould be reported as
applied. The mirror image also held:preflightsaidgateway_already_runningwhile the gateway was down, so the only repair we ship could never beapplied. The listening port is now the verdict.
The standalone binary accepted no keyboard input.
bun build --compilewas spawned as a CLI,which cannot inherit the Solid JSX transform registered through
bunfig.toml'spreload. Thebinary rendered a correct first frame and then ignored every keystroke — nothing could be typed,
answered, approved, or quit. Now compiled in-process with the plugin passed to
Bun.build.There was no way to exit the TUI.
exitOnCtrlC: false,exitSignals: [], and Ctrl+C boundonly to cancel-scan. Ctrl+D quits; Ctrl+C quits when idle and still cancels active work.
Also fixed
Consent dialog previewed something other than what is sent (the
/api/diagnoseupload wasundisclosed) · stale
SCRIPT_HASHbroke the documented verification ritual · a thrown scan keptthe previous revision's findings · autoscan rejection tore down the renderer · footer wrapped
across the whole 65–91 column band · high-risk refusal lived only in the TUI, not the engine ·
stage exceptions escaped
applyPlanand lostapplyResult· unbounded conversation store ·npx clawfix --tuiblamed Bun for a missing directory (the TUI is binary-only; site copy nowsays so) · two tests could only pass on Linux.
Gates
npm testbun testbunx tsc --noEmitAlso run on a Blaxel sandbox with a real OpenClaw 2026.6.11 install: 378 tests,
clawfix --jsonfound 7 real issues, hostname hashed to 8 chars, and a real 48-char
gateway.auth.tokenappearszero times in the diagnostic.
New coverage
The scripted end-to-end session that was missing — scan → consent → remote chat → repair
proposal → approval → apply → verify — with the real session controller, findings, repair engine
and SSE client, and a test server that validates bodies with the real
validateAgentV2Request.Confirmed to fail against the pre-fix bridge. Plus
scripts/smoke-tui-interactive.mjs, whichdrives the real binary through a PTY; every earlier gate passed on the broken binary because
rendering was exactly what still worked.
Known limitation (not fixed)
The TUI cannot run on Alpine/musl:
@opentui/coreresolves its native library as@opentui/core-linux-x64with no musl detection, and the musl package does not satisfy it. TheCLI core is unaffected. Details in
REVIEW.md.