Uninstalling the daemon detaches Claude and Codex too - #703
Conversation
Removing the service left both clients pointed at a local gateway port that no longer answers, which fails every request rather than falling back to the vendor. Uninstall now runs the same disk undo `hyp detach` uses, once the service is actually gone, and names each settings file it reverted. `stop` and `restart` still touch nothing: their port comes back.
773dfed to
44c1da2
Compare
platypii
added
the
neutral:review
Master's highest LLP is 0205. PR #703 (uninstall-detaches-clients) also mints llp/0206 for an unrelated decision, and PR #701 (fix/issue-700) already claims 0207, so this PR's compaction-file-size decision moves to 0208, the next free number. Mechanical renumber only (LLP 0156): the filename, the doc header, the Extended-by forward-ref on LLP 0199, and every @ref annotation in src/ and test/ move together; no meaning changes. Co-Authored-By: Claude <noreply@anthropic.com>
Review - head
|
philcunliffe
added
the
neutral:changes-requested
philcunliffe
left a comment
philcunliffe
left a comment
There was a problem hiding this comment.
Reviewed at head 44c1da2 under the neutral:review delegation (review-only: nothing pushed to this branch).
One blocker: the sweep cannot detach OpenClaw, because it runs after teardown. detachJsonPathProviders needs expectedBaseUrl, and all three rungs of resolveExpectedGatewayBaseUrl are dead once the service is gone (capabilities unbound in the CLI, hyp init writes no listen per LLP 0114, and the live-status rung is gated on processIsAlive). Reproduced: a staged OpenClaw provider yields failed: [openclaw], exit 1, config unchanged - so OpenClaw is left pointed at the dead port, which is the bug this PR exists to fix. The suggested remedy hyp detach openclaw is a dead end after uninstall for the same reason. Recommend resolving the base URL before uninstallDaemon and threading it into the sweep.
Plus three majors: a user who never attached OpenClaw gets a false uninstall failure (contradicting this PR LLP 0206 D1); quiet: true silently discards every undo warning, including the "overridden externally" and prev_malformed notices that only the sweep-time reader can act on; and the two headline claims (the teardown ordering gate, per-client failure collection) have zero test coverage - no test drives runDaemonUninstall at all, so deleting the return 1 would break nothing.
Full evidence, plus a minor/nit and an LLP 0206 numbering-collision note, in the review record comment above. The shared-undo, failure-collection and destructive-scope claims all verified correct, and LLP hygiene is clean.
…ery client The json_path undo judges provider ownership by the gateway's base URL, and every rung of that resolution dies with the service, so uninstall now captures it first and hands it to the sweep. A config whose providers carry no HypAware marker header is a no-op rather than a failure when the origin is unknown. The quiet sweep now carries each undo's warnings back to the uninstall output, help text stops enumerating clients, and the uninstall command itself is under test through a teardown seam.
|
Addressed in caf18b4:
|
Review - head
|
philcunliffe
left a comment
philcunliffe
left a comment
There was a problem hiding this comment.
Re-reviewed at head caf18b4 (review-only: nothing pushed to this branch).
All six earlier findings are resolved, verified by reproduction and by five mutations that are each now caught by a test. The OpenClaw blocker is genuinely fixed for the daemon-alive case, the never-attached false failure is gone, undo warnings surface, and runDaemonUninstall is finally driven by tests.
One major survivor (NEW-1): uninstall still cannot detach OpenClaw when the daemon is not running. Resolving the origin before teardown does not help if there was never a live pid to resolve from - rung 3 is gated on processIsAlive. Reproduced with status.json present and no pid file: exit 1, config unchanged, OpenClaw left pointed at the dead port, and the printed remedy hyp detach openclaw fails identically. README.md:370 lists hyp daemon stop directly above hyp daemon uninstall, so this sequence is ordinary - and it is a new loud failure, since master exits 0 there. Suggested fix: read the last-known port from status.json without the liveness gate (the file survives a stop, and status.js:768 already sets that precedent), or at minimum stop printing a remedy that cannot work.
Plus a minor CLAUDE.md violation (an inline import() type at daemon.js:250, the only one in src/), a now-false JSDoc on the shared ownership predicate, and a README recipe that still enumerates two of three clients.
Full evidence in the review record comment above.
…ient's undo The json_path undo no longer needs the gateway's live base URL: the marker header attach writes is HypAware's own name, and attach already trusted it alone. Ownership is now that signature on both sides (LLP 0210), so the expectedBaseUrl threading, the pre-teardown origin capture, and the EXPECTED_BASE_URL_UNKNOWN refusal are gone, and the uninstall sweep works whether the daemon is running, stopped, or long dead. A not-ours entry is left in place instead of moved to a backup key, the cache purge follows the settings disposal, and the README uninstall recipe stops enumerating clients.
|
b316c79 reworks the OpenClaw handling instead of patching NEW-1. Stepping back from the survivor finding: the root cause was that the json_path undo was the only one needing a fact from a running daemon (the gateway's live origin) to judge ownership, while the json and toml undos read everything off the settings file. That asymmetry is what forced the pre-teardown origin capture, and it still left a stopped-daemon machine unable to detach OpenClaw at all. The rework (LLP 0210, extending LLP 0169 and LLP 0172): ownership is the entry's own signature - the x-hypaware-upstream marker header naming its key plus the shape attach produces. Attach has always trusted that signature alone (it must, since a drift re-attach sees its own entry carrying the old origin), so detach now matches it. Consequences:
Net: 309 insertions, 342 deletions, and no client undo depends on daemon liveness anymore. |
Review - head
|
| fixture | outcome |
|---|---|
| user-authored provider, no marker | changed:false, byte-identical, no warning |
marker present, models hand-edited non-empty |
not ours, left in place |
| marker names the other key | not ours, left in place |
ours at an old port :59999 |
deleted (the fix) |
ours at openai plus user's own anthropic |
openai deleted, anthropic kept, warning names the path only |
forged: user's baseUrl and x-api-key, but carrying x-hypaware-upstream and models: [] |
deleted, no backup |
The forged case is the one real give-up. LLP 0210 D1 names and accepts it, and the symmetry argument holds under verification: conflictingProviderKeys calls the identical predicate, so attach would silently overwrite the same entry on the next drift pass. Not blocking - but note hyp detach --json signals such a deletion only as removed: <their baseUrl>, with no warning.
Blast radius on the shared undo
Reconciler reverse() now calls detach({ descriptor, env }) with ctx.endpoint untouched for perform(), and is strictly improved. hyp leave never passed an origin and no longer depends on resolving one (leave-command.test.js passes). hyp detach dry-run unchanged; no dead imports left behind.
Mutation coverage of the new logic
Seven mutations applied and reverted; six caught. M4 - ungating the cache purge from deleted.length > 0 - caught nothing, which is finding N7.
New findings
N5 (minor) - src/core/config/types.d.ts:733-745. The exported ClientDetachFromDisk type still declares expectedBaseUrl?: string, documented as "ownership can only be decided by comparing that entry's baseUrl against the URL attach would have written (LLP 0172 section 2.1)" - the exact clause LLP 0210 retires. typecheck passes because an extra optional property is structurally harmless, so nothing catches it. This is the detach seam's type in CreateAttachHandlerOptions and it ships in the published types/ build, so a plugin author reads a parameter no implementation consumes. Recommend deleting the field and its JSDoc.
N6 (minor) - the cache purge is no longer retryable. client_detach_disk.js:806-820. Gating the purge on deleted.length > 0 means a detach whose settings half landed but whose cache half failed cannot be finished by rerunning it. Verified: with models.providers already empty and agents/main/agent/models.json still holding a stale entry, caf18b4 purges to {} on rerun while b316c79 leaves the cache dirty with changed:false. This matters because purgeProviderCaches' own doc says these caches "do not self-heal ... leaving them keeps a detached client pointed at a dead gateway", every purge failure is a warning that invites a retry, and docs/ACCEPTANCE.md:398-406 greps for exactly this residue. Recommend keeping D2's never-attached guarantee but moving the gate per-row: purge a cache row when it carries the marker header (the same signature D1 already trusts) rather than gating the whole purge on the settings deletion. That also stops the current code deleting a user's cached row for a managed key absent from the settings.
N7 (minor) - no test for LLP 0210 D2's purge gate. Removing the deleted.length > 0 gate at client_detach_disk.js:812 fails nothing, because the never-attached test in test/core/client-detach-json-path.test.js stages no cache file. D2 states the rule explicitly ("a never-attached machine's derived caches are never edited") and it is the only new rule in this commit with zero coverage. Recommend staging a cache file in that test and asserting it is untouched.
N8 (nit) - README.md:262-270 still lists only hyp detach claude / hyp detach codex, omitting openclaw. Same class as NEW-4 but a different region, pre-existing on the branch base and outside this diff; noting only because LLP 0206's "names no fixed client list" consequence applies here equally.
For the next round
N5 and NEW-3 are one-line edits; N7 is a few lines of fixture. N6 is the only one needing a decision rather than an edit - if the gate is deliberate, LLP 0210 D2 should say that a failed cache purge is not recoverable by rerunning hyp detach.
philcunliffe
left a comment
philcunliffe
left a comment
There was a problem hiding this comment.
Re-reviewed at head b316c79 (review-only: nothing pushed to this branch).
The round-2 major is resolved, and your approach is better than the one I recommended. I suggested recovering the historical origin from status.json; you dropped expectedBaseUrl entirely and made ownership signature-only. That is the stronger design: the origin comparison never protected anything the signature does not already decide, and it created a "ours, but at a stale port" class the old code misclassified as not-ours and backed up rather than deleting. It also fixes a latent bug my recommendation would have missed - the reconcilers reverse()passedctx.endpoint`, which after an LLP 0086 ephemeral rebind is the new port, so reverse would have refused over its own entry from the old one.
Verified both directions through the real CLI: on b316c79 a stopped-daemon machine now detaches OpenClaw with exit 0; on caf18b4 the same fixture failed with "base URL is unknown". NEW-2 and NEW-4 are also resolved. Six of seven mutations on the new logic are caught by tests.
Remaining, none blocking: NEW-3 is not resolved - the JSDoc on the shared predicate swapped one false claim for two new ones (it still says core "backs up anyone elses", which LLP 0210 D2 retired, and says "three sides" where there are two callers). Plus three new minors: the exported ClientDetachFromDisktype still declaresexpectedBaseUrlwith a JSDoc citing the exact clause 0210 retires, and it ships in the publishedtypes/ build; the cache purge is no longer retryable after a partial detach; and D2s purge gate has zero test coverage (removing it fails nothing).
The retryability one is the only item needing a decision rather than an edit - if the gate is deliberate, LLP 0210 D2 should say a failed cache purge is not recoverable by rerunning hyp detach. Full evidence in the review record above.
…overage The cache purge now runs when the managed surface is already empty, not only when this pass deleted an entry, so a detach whose settings half landed can be rerun to finish the cache half; keys a user value holds in the settings still keep their cache rows. The published detach type drops the retired expectedBaseUrl field, the shared predicate's JSDoc matches the leave-in-place disposal, and the README attach/detach section stops enumerating clients.
|
1249a8e addresses the round-3 findings:
3911 tests pass, typecheck clean. |
Review - head
|
| item | status |
|---|---|
| NEW-3 (false JSDoc on the shared predicate) | resolved - now says "the two sides", drops the backup claim and the phantom third caller. Grep confirms exactly two importers. |
N5 (expectedBaseUrl in the published types) |
resolved - gone from types.d.ts; repo-wide it survives only in llp/ history. types.d.ts does ship in the tarball, so the removal really does clean the published surface. |
| N6 (non-retryable cache purge) | partially resolved, and it introduces a worse problem - see below. |
| N7 (no coverage on the purge gate) | not resolved - mutation M4 still passes 9/9. |
| N8 (README enumerated two of three clients) | resolved - now hyp detach <client> with # claude, codex, openclaw, .... |
The gate became deleted.length > 0 || present.length === 0 rather than the suggested per-row check. That buys retryability only when the managed surface is entirely empty, and it pays for it by reinstating unconditional purging in exactly the case round 3 warned about.
NEW-4 - major - a never-attached machine's own cache rows are silently deleted
src/core/config/client_detach_disk.js:822. When the settings file exists but holds no managed key, present.length === 0 fires and the purge deletes anthropic and openai from every agents/*/agent/models.json. Reproduced against the real detachClientFromDisk:
S1 never attached, settings = {theme, agents} (no models.providers key at all)
cache before: {anthropic:{baseUrl:"https://api.anthropic.com", models:[{id:"claude-sonnet-4"}]},
openai:{..., models:[{id:"gpt-5"}]}}
result: {"changed": false} <- no warning, no output line
cache after: {}
S2 never attached, models.providers = {google:{...}} -> same: cache after {}
models.providers is optional in openclaw.json, so S1 is the ordinary state of an OpenClaw user who never overrode a provider. LLP 0167 verify item 3 records that these caches run in merge mode and "carry every existing cache provider forward wholesale", so rows at those keys are expected to be there. And because detachAllClientsFromDisk sweeps every registered client with no attach-state precheck, hyp daemon uninstall - the whole point of this PR - does this on any machine that has OpenClaw installed and was never attached. It reports changed:false and prints nothing, so the deletion is entirely silent.
LLP 0210 D2's new justification is contradicted by this repo's own acceptance procedure. D2 now claims an orphaned cache row "is indistinguishable from that residue", but docs/ACCEPTANCE.md:398-406 greps agents/*/agent/models.json for x-hypaware-upstream and treats a match as un-purged residue - i.e. the repo already asserts the marker is present in the cache file and is the distinguishing signal. The user's rows in S1/S2 carry no marker; ours do. So D2's sentence "a never-attached machine's own providers are never edited in either file" is false as written, and the per-row check is well-founded on the repo's own documented cache shape.
Recommend: purge a cache row when it carries the marker header naming its key (D1's signature, the same thing ACCEPTANCE greps for); for a row with no marker, purge only when this run deleted the matching settings entry. That keeps D1's trust model, keeps the never-attached guarantee whole, and is retryable in every case rather than only the fully-empty one. If the real cache shape is not trusted to carry the marker, the safe fallback is reverting to deleted.length > 0 and accepting the retry gap: a residue the user is warned about is strictly better than silent deletion of their config.
NEW-5 - minor - the retryability the gate was added for does not hold in the mixed case
Same line. With a user entry at one managed key, a rerun never purges residue at the other:
S4 settings = {models.providers: {openai: <user's own>}}
cache = {anthropic: <ours, marker present>, openai: <user's own>}
result: {"changed": false}
cache after: anthropic residue still there, marker and all
present.length === 1, deleted.length === 0, so the gate blocks. That residue plainly carries x-hypaware-upstream - the same signature D1 trusts to delete a settings entry - and is left pointing at a dead gateway forever. The per-row fix closes this too.
N7 - minor, third round - the purge gate still has no coverage on its guard side
M4 (if (true)) survives the whole file. The new never-attached test stages a cache, but both managed keys are user-owned, so they land in left and providerKeys filters to [] - the assertion is guaranteed by the left filter, not by the gate. Confirmed by mutation: M5 (ignore left) fails 1, which is what that test actually pins; M4b (revert to deleted.length > 0) fails 1; M6 (present.length === 0 only) fails 3. A test that would catch M4 is the S4 fixture above, or S1 with a cache row at a managed key the settings do not mention.
NEW-6 - nit - stale @ref gloss
client_detach_disk.js:728 still says "only the deleted keys are purged from the derived caches". The purge covers every managed key not held by a user settings entry, including keys never present in the settings at all - which is the mechanism behind NEW-4. Per CLAUDE.md "keep refs honest", reword when the gate changes.
Checked and clean
LLP 0210 is Status: Draft, so editing D2 in place is permitted, and both #d1/#d2 anchors resolve. The types.d.ts removal breaks no consumer (tsc -p tsconfig.build.json emits, npm pack packs, and nothing references the removed names). No em dashes, no added semicolons. Round 3's headline judgement stands and is not relitigated: signature-only ownership beats the origin comparison, D1 and its two callers are consistent and correct. Everything above is confined to the cache-purge gate.
philcunliffe
left a comment
philcunliffe
left a comment
There was a problem hiding this comment.
Re-reviewed at head 1249a8e (review-only: nothing pushed).
NEW-3, N5 and N8 are resolved. But the N6 fix took deleted.length > 0 || present.length === 0 rather than the per-row check, and that introduces a worse problem than the one it solves.
NEW-4 (major): a never-attached machine now has its own cache rows silently deleted. When the settings file holds no managed key, present.length === 0 fires and the purge wipes anthropic and openai from every agents/*/agent/models.json. Reproduced: a user with models.providers absent entirely (the ordinary state for someone who never overrode a provider) loses both cached rows, with changed:false, no warning and no output line. Since detachAllClientsFromDisk sweeps every registered client with no attach-state precheck, hyp daemon uninstall does this on any machine that has OpenClaw installed and was never attached.
LLP 0210 D2 justifies this by saying an orphaned row "is indistinguishable from that residue" — but docs/ACCEPTANCE.md:398-406 greps those cache files for x-hypaware-upstream and treats a match as un-purged residue, so the repo already asserts the marker is present in the cache and is the distinguishing signal. Ours carry it; the user`s do not.
Recommend the per-row check: purge a row carrying the marker that names its key (D1s own signature), and for an unmarked row purge only when this run deleted the matching settings entry. That keeps the never-attached guarantee whole and is retryable in every case, not just the fully-empty one. If the cache shape is not trusted to carry the marker, reverting to deleted.length > 0` is the safe fallback — a residue the user is warned about beats silent deletion of their config.
Also: NEW-5, the retryability this gate was added for still does not hold in the mixed case (a marked residue at one key with a user entry at the other is never purged); and N7 for the third round — mutation if (true) on the gate still passes the whole file, because the new tests assertion is guaranteed by the left` filter rather than by the gate.
Round 3`s judgement that signature-only ownership beats the origin comparison stands and is not relitigated. Everything here is confined to the cache-purge gate. Full evidence and fixtures in the review record above.
The round-4 gate traded one bug for a worse one: with the managed surface empty it deleted a never-attached user's own cache rows, and in the mixed case it never retried our residue. The caches carry provider entries forward headers and all (ACCEPTANCE greps them for the marker), so each row is judged by the same signature as the settings undo: a row whose marker header names its key is ours and purges whenever seen, a marker-less row rides out only on this run's settings deletions, and everything else is the user's and stays. Tests pin the never-attached untouched cases and the mixed-case rerun.
|
037bc88 takes the per-row recommendation - you were right that the marker is available in the caches, and ACCEPTANCE.md's residue grep settles it. I had wrongly inferred the cache shape from the test fixtures, which wrote marker-less rows; the fixtures now carry the entries forward wholesale, marker included, matching LLP 0167 verify item 3.
3913 tests pass, typecheck clean. |
Review - head
|
| scenario | settings | cache | result |
|---|---|---|---|
S1 never attached, no models.providers key, user rows in cache |
byte-identical | byte-identical | changed:false |
S2 never attached, models.providers={google} |
byte-identical | byte-identical | changed:false |
S2b same, nested models.providers cache spelling |
byte-identical | byte-identical | changed:false |
At 1249a8e S1 and S2 deleted both rows. The gate is gone and safety moved into the row test, so a never-attached machine's rows fail that test and the property holds by construction rather than by a caller-side precheck. That is the stronger shape.
NEW-5 (minor) - resolved. The mixed case now works: a marked anthropic residue is purged while the user's openai row is kept and settings stay byte-identical.
N7 (minor, open three rounds) - resolved. The mutation that survived three rounds (if (false) continue, ungating the row test) now fails 4 tests, including "leaves a never-attached machine untouched". Five further mutations fail 3-5 tests each. The tests also lock out a tempting refactor: substituting isOwnedProviderEntry for the row test fails, which is correct, since the cache row's models is populated by OpenClaw's merge and the row test must be the marker alone rather than the full triple.
One mutation survives: headers[markerHeader] !== undefined in place of === key. It only loosens the "names its own key" half, and a row so marked cannot be produced by attach. Noted, not raised.
NEW-6 (nit) - resolved. The @ref gloss and the surrounding JSDoc now describe the implemented rule.
The load-bearing premise, probed
The recommendation assumed the marker lands in the cache row, so that assumption was tested rather than inherited:
- marker names its own key with a stale port: purged (correct per D1 - a rebound-port residue is still ours).
- marker names its own key with
modelspopulated by the cache merge: purged. This is the shape a real cache row has, and a full-triple test would have missed it. - marker naming a different key: left in place.
- older/renamed marker header: left in place - and no prior HypAware version wrote a different config-side marker (the pre-0167 era attached the header per request via the steering plugin, never into
openclaw.json), so no real residue is missed. headersas an array, or marker valuetrue: left in place, no throw.
Cross-checked against the repo's own assertions: docs/ACCEPTANCE.md:401 greps those cache files for x-hypaware-upstream with pass condition "no matches", and LLP 0167 #attach-detach records that the merge "carries every existing cache provider forward wholesale". Both support the premise. It degrades gracefully too: if a future OpenClaw stripped headers from the cache, an ordinary full detach still purges via the deletedKeys arm and only the retry/mixed case would quietly not purge - and the openclaw_capture acceptance grep is the standing check for exactly that.
Docs and blast radius
LLP 0210 is Status: Draft, so editing it here is within convention; D2 now states the implemented per-row rule including the marker-less fallback, and the contradicted "indistinguishable from that residue" text is gone. docs/ACCEPTANCE.md's residue grep is unchanged and still describes reality. openclaw is the only manifest declaring format: json_path/cache_glob, and all four consumers (hyp detach, hyp leave, the uninstall sweep, and the reconciler's reverse()) share detachClientFromDisk, which the probes exercised directly.
One observation, not a finding
In the retry and mixed cases the purge rewrites a cache file while result.changed stays false, so hyp detach openclaw prints "nothing to do" and the sweep omits the client from detached. master's unconditional purge had the same mismatch over a strictly wider set of cases, so this PR narrows it rather than introducing it. Not worth holding for.
Closing note
Five rounds, and every round's findings were answered. Two of them changed neutral's mind: the signature-only ownership design (round 3) and this per-row purge both ended up better than what the review originally recommended. Held for you to merge - neutral never merges a review-only delegation.
philcunliffe
added
neutral:approved
philcunliffe
left a comment
philcunliffe
left a comment
There was a problem hiding this comment.
Approved at head 037bc88 (review-only: nothing pushed to this branch across all five rounds).
All four open findings resolved. The per-row purge is verified by reproduction: a never-attached machine with user-owned anthropic/openai cache rows now comes through byte-identical in all three shapes that previously lost them. Safety moved from a caller-side gate into the row test, so the never-attached property holds by construction rather than by a precheck - the stronger shape.
N7, open three rounds, is finally closed: the mutation that survived all of them (ungating the row test) now fails four tests, and five further mutations fail 3-5 each. The tests also lock out substituting isOwnedProviderEntry for the row test, which is correct, since the cache rows models is populated by OpenClaws merge.
I probed the premise the recommendation rested on rather than assuming it: a marker with a stale port purges, a marker naming a different key does not, a populated models row still purges (the shape a real cache row has), and an older/renamed marker is left alone - with no real residue missed, since no prior version wrote a config-side marker. docs/ACCEPTANCE.md:401 and LLP 0167 both corroborate, and the behaviour degrades gracefully if a future OpenClaw ever stripped those headers.
Worth saying: two of the five rounds ended with your approach beating the review`s own recommendation - signature-only ownership, and this per-row purge. Held for you to merge.
2 participants
hyp daemon uninstallremoved the launchd / systemd service and nothing else, by design. That left~/.claude/settings.jsonstill settingANTHROPIC_BASE_URLtohttp://127.0.0.1:<port>and~/.codex/config.tomlstill naming thehypawareprovider on that port, with nothing listening. A client pointed at a dead local port does not fall back to talking to Anthropic directly; it fails every request.Uninstall now sweeps the clients after the service is gone:
hyp detachand the reconciler'sreverse()already share, so org-installed assets come off with it and there is no third implementation to drift.hyp detach <client>that finishes it and the command exits nonzero.hyp daemon stopandhyp daemon restartstill touch no client settings: their port comes back, and detaching there would break capture on every restart.This is a second exception to LLP 0063's "each connection level exits with its own verb", so the rationale lands as LLP 0206 with an
Extended-by:forward reference on 0063: an exit verb may cascade down when the lower-level state it would leave behind is broken rather than merely stale.Tests: three new cases covering both clients reverted, the no-attach no-op, and a one-client sweep not reporting the untouched one. Full suite and typecheck pass.