fix(ok): recover ACP agents when the npx/uvx interpreter is broken, and log agent failures (#3065) - #1206
Merged
Merged
Conversation
…nd log agent failures (#3065)
* fix(ok): recover ACP agents when the npx/uvx interpreter is broken
Preflight proved only that the interpreter file exists and is executable,
which a fatally broken one also does. A Homebrew node whose icu4c was
upgraded out from under it aborts under dyld the instant it runs, so the
agent died before the ACP handshake and the user saw only "initialize
failed: ACP connection closed" (PRD-7708).
Health-probe npx/uvx with --version before spawning and route a broken
interpreter to the same consent-gated managed runtime a missing one already
gets. Healthy verdicts memoize per command + PATH so the probe costs one
spawn per interpreter, not one per thread.
Also put the agent's own last words in the server log: the failure-status
warn now carries machineDetail (the stderr tail — the dyld text, the stack
trace), and an unexpected agent exit logs code/signal/tail. Neither reached
any log before, so an ACP crash left nothing in a diagnostic bundle.
* test(ok): cover the interpreter health probe and the new failure logs
Probe unit tests (healthy, non-zero exit, SIGABRT-with-linker-message,
silent failure, unspawnable, hang→timeout with no leaked process group)
plus the hint wording. Integration: a resolvable-but-crashing npx routes to
the consent flow and installs; declining reports the broken interpreter
rather than a missing one; a healthy interpreter is probed once across two
thread starts. Log coverage: the failure-status line carries machineDetail,
and an agent that dies after going ready logs its exit.
* style(ok): biome format the ACP launch tests
* docs(ok): drop tracker keys from the new ACP test comments
md-audit comment-discipline: ticket keys belong in the PR, not in source.
Substance kept.
* refactor(ok): make the interpreter health-memo key unambiguous
A space-joined command + PATH could collide across two launches. Encode
both fields instead.
* docs(ok): describe the health-memo key as it is actually built
* test(ok): assert the timed-out probe's group STOPPED, not that it vanished
The CI failure was the assertion, not the kill. Two defects in the check:
`pgrep -f <pattern>` also matches the shell running the pgrep, which never
dies; and a process-existence check reports a zombie as alive, which is what
an orphan becomes wherever init doesn't reap it (a container's PID 1).
The grandchild now proves it stopped by ceasing to write a heartbeat.
Verified on real Linux (docker node:24-slim): the group kill lands, the
heartbeat stops after ~10 ticks — the semantics were always correct.
* fix(ok): address review — uv hint, managed-runtime probe, public-mirror leak
Three findings from the PR review, all real:
- brokenInterpreterHint pasted the Homebrew/icu4c cause into the uv message,
sending a uv user to inspect a Node they may not even have. Switched whole
per kind, like missingCommandHint. The old test missed it because the
clause spells it `node`, not `Node.js`; the test now rejects both.
- The managed-runtime launch got preflight only, never the health probe --
and findManagedRuntime's already-installed fast path answers with the same
exists-plus-execute-bit check this change exists to replace. A runtime left
damaged by an interrupted extraction sailed through into the opaque failure
with no fallback left. Now probed as a terminal check (nowhere left to
route) with its own hint: OK's copy is not the user's to repair, so the
remedy is a fresh download, not a Homebrew fix.
- A test comment named the bug reporter. packages/** mirrors to the public
repo, so that shipped publicly.
The exit-after-ready log test was also flaky: a timed exit could land before
session setup finished, failing the pending session/new and putting the
thread in 'error' -- whose exit is a deliberate echo and is not logged. It
now goes ready first and dies on an out-of-band trigger with no request in
flight. 3x green; the managed-runtime test is mutation-checked (red with the
probe removed).
* style(ok): format the damaged-managed-runtime test
CI caught this because the local gate never reached lint: `pnpm check`
short-circuits at its first pre-flight failure, and a stray gitignored file
in this working tree fails the knip drift guard ahead of it.
* fix(ok): deterministic damaged-runtime test + review follow-ups
CI red: the damaged-managed-runtime test was timing-dependent. Its first
thread probed the freshly installed runtime, cached it healthy, and only then
did the test damage the launcher -- so on a slower runner the second thread
hit the cache and never probed. Now it installs, damages, and only then
starts a manager, which is also the real shape: a server booting into a
runtime an earlier process left behind.
Review follow-ups:
- exit-handler log uses stderrTailDetail(), the shared full-tail helper every
other failure path uses, instead of a local 10-line slice
- drop change-narration from the managed-probe comment
- call-site comment described a two-branch model that this no longer matches
- brokenInterpreterHint's doc claimed it only reaches a host with no managed
runtime; it also reaches a decline
- probe timeout now leaves a debug breadcrumb -- it answers "healthy enough"
and was otherwise indistinguishable from a clean run, so an interpreter
that hangs every launch burned the budget invisibly
New test for the retry cache-clear, after the obvious version proved to be
theater: the suggested shape (broken -> repair -> retry) passes with
clear() deleted, because a FAILING probe is never cached. What clear()
actually protects is the reverse -- an interpreter that breaks after being
cached healthy, where a retry would otherwise respawn it forever and never
offer the runtime. Mutation-checked: red without clear(), green with it.
* docs(ok): drop change-narration from test comments, cover the uv hint branch
Comment discipline applies to test comments too: "until these lines existed"
and "the check this change exists to replace" both dissolve once merged, and
the 84 MB-bundle anecdote is audit-trail narrative. Substance kept, framing
made durable.
brokenManagedRuntimeHint was only tested on its npx branch — the same
one-branch coverage that let Node-specific advice reach uv users in the
sibling hint.
* fix(ok): tell the user their interpreter is broken, not missing
Manual QA of the real UI caught what the tests and both reviewers missed.
The download offer is the sentence a user in this state actually reads, and
it said: "This agent runs through npx, which isn't installed." For the case
this PR exists to fix that is false and actively harmful — npx IS installed,
and following the advice installs a second Node that the broken one still
shadows on PATH.
The consent request now carries why it appeared (missing | broken), and the
card picks its sentence off that. The field is optional: threads persisted
before it existed replay as `missing`, which is what they were.
Translated into all ten non-source locales in the same change, per the
subtree's i18n rule. Each was written from that locale's existing sibling
string so vocabulary and register match rather than being re-derived.
Verified end to end against a real dev server with a crashing `npx` first on
PATH: the card now reads "installed but won't run — its Node.js looks
broken".
* fix(ok): flush TipTap's deferred destroy before the jsdom teardown
test:dom went red with 3954 passing tests and zero failures: @tiptap/react
defers editor.destroy() into a 0 ms timer on unmount, and when the last test
in a file leaves one pending, vitest tears the jsdom environment down first.
The destroy then runs with no `document`, throwing a ReferenceError nothing
is awaiting — which vitest can only report as an unhandled error that fails
the whole run.
One macrotask at file end lets it land while the DOM still exists. Latent
until now; this branch's app-side change shifted suite timing enough to pull
it inside the window.
afterAll, not afterEach: a tick between tests also lets deferred work render
against a tree whose providers cleanup just removed, which fails honest tests
(EditorArea's terminal-placement test died with "usePageList must be used
within <PageListProvider />" when I tried that first).
* fix(ok): ask about the managed runtime per launch, don't remember the answer
The consent card's "Remember this for future agents" checkbox defaulted to
on and persisted the decision to ~/.ok/acp-runtime-consent.json. A
remembered decline removed the offer with no UI to undo it; a remembered
grant authorized a download the user never saw. An installed runtime is
adopted before consent is ever read, so the remembered grant bought almost
nothing.
The frame parser now IGNORES a `remember` field rather than rejecting it —
a pre-removal renderer against a newer server would otherwise have its
grant dropped and the launch would sit parked forever.
* fix(ok): replace a damaged managed runtime instead of prescribing rm -rf
A copy of OK's own Node/uv that installs but won't run used to end the
launch with "delete that directory and OK will download a fresh copy on
the next launch" — terminal homework for a copy the user never installed.
OK now discards it and offers a fresh download through the same consent
card, one attempt per launch.
The quarantine reports whether the tree actually moved. A Windows rename
fails with EBUSY while another agent holds the launcher open, and a silent
failure there would let the re-download adopt the SAME damaged copy off the
install fast path — then blame the machine for a fresh copy that never
landed. That branch gets its own message and its own test.
* chore(ok): drop files a merge swept in from the working tree
The merge resolution staged the whole subtree, which pulled in 47
untracked local files — scratch notes, session logs, and internal
reports/specs — that belong to neither parent and to nothing in this PR.
They stay on disk, just not on the branch.
* ci(ok): give the integration shards room for their own retry
shard1 died at its 25-minute cap on this branch and the required gate went
green anyway. Three things compounded:
- A shard1 pass measures ~646s of vitest, so the one flake retry the step
already performs doubles the test phase to ~21.5 min. That fits under 25
only while the turbo cache is warm; cold, install + `^build` alone measured
~14 min, which puts a retried run at ~35 min.
- A job killed by `timeout-minutes` reports `cancelled`, and the gate
tolerates `cancelled` by design (merge-queue supersede, enforced by
check-gate-cancel-resilient) — so the check passed with the suite never
having finished.
- The kill skips the job's cache Post steps, so the turbo cache is never
saved and the next attempt starts equally cold and dies the same way.
Raise both shards to 35 so a cold install + build + both attempts fit, and
pass `--log-order=stream` on both attempts of the non-`test` branch: turbo's
default resolves to `grouped` in CI, so the killed job lost its entire
buffered log and read as ~11 minutes of silence.
* test(ok): stop template-history asserting the PUT always yields two commits
`template-history-attribution` failed twice in a merge_group run and ejected
the PR; it also failed twice on main at 00:41Z. Reproduced locally at 3/12.
A PUT produces two writes — the route's typed `template-create:` folder event
and the store's debounced content commit. The test assumed they always land as
two git commits, so the doc's own history is all `wip:`. They only do when the
writes straddle the 100ms debounce window. Dumping both histories shows the two
shapes:
separate: doc [c005ae7e wip: auto-save]
folder [072344a0 template-create: …]
coalesced: doc [90dc558f template-create: …]
folder [90dc558f template-create: …] ← same sha
Coalesced, the one commit carries the typed subject and appears in the doc's
history, so `putDoc.every(/^wip:/)` fails. The other interleaving — the typed
commit landing *after* the putDoc snapshot — makes it read as the "new" row and
fails the edit-entry assertion instead. That is the shape CI hit.
Both assertions now admit at most one typed entry and hold every other entry to
`wip:`, and the edit poll skips the typed event so it waits for the editor's own
commit. `<= 1` on both sides rather than equality, because the late-landing path
legitimately shows 0 typed entries at snapshot time and 1 afterwards.
The guard survives: the store minting a typed subject per content edit would push
the typed count past one, and the folder timeline is still held to exactly one
`template-` event. 22/22 clean locally against a 25% base rate.
---------
GitOrigin-RevId: 9fdbca9500886980c15e517c12e4baa4c8df2a7a
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.
Copybara-translated 1 Inkeep OSS change. Rebase-merge this PR so the prepared commit lands directly on public main.