Uh oh!
There was an error while loading. Please reload this page.
Unpicking an attached client is named, not silently left broken (#604) - #608
Conversation
Re-running `hyp init` and unchecking a previously attached client regenerates a config without that client's adapter while the client's own settings still route through the HypAware gateway. The finale only attaches, and the action reconciler's reverse lane undoes only config-named (org/central) keys, so a wizard attach on the local layer is never reversed: capture stops and the client's requests can fail. Picking only OpenTelemetry composes no gateway at all, and every still-attached client points at a dead port after the finale's restart. The finale now names the clients it left stranded (attach marker on disk, not picked, enabled by neither the written config nor the central layer) and prints the `hyp detach --client <name>` that clears each. It does not detach: rewriting a client's settings file is not what unchecking a picker row asked for, and that call is the maintainer's to make (LLP 0185). `hyp status` gains the matching `client_attached_not_configured` diagnostic as the after-the-fact backstop, gated to solo hosts because on a joined host that shape is a reconciler pass that has not run yet. The org/central attach markers are untouched throughout. Co-Authored-By: Claude <noreply@anthropic.com>
…604) Three defects in the #604 fix, found in neutral review. An unreadable local config emptied the active-plugin set for a reason that has nothing to do with what the operator enabled, and the new `client_attached_not_configured` branch read that as "nothing enables this adapter": `hyp status` answered a parse failure with a `hyp detach` for every attached client, stacked on top of the `config_unreadable` error that is the actual repair. The branch now stands down when the local layer is present but did not parse. `findAttachedNotConfiguredClients` counted a plugin entry with `enabled: false` as configured, while `hyp status` builds the same set from `activePlugins`, which skips it. A switched-off adapter collects exactly as little as an absent one, so the two surfaces disagreed about the same file. LLP 0185 records both gates, and the forward-refs the corpus convention wants land on the docs it extends: LLP 0180 (the finale's attach lane) and LLP 0086 (the client drift diagnostic family). The status test's `@ref` carried an empty gloss; it now says what it tests. Tests: an unreadable local config draws no detach advice; a disabled plugin entry strands its client; and the central-layer suppression LLP 0185 #scope promises for the finale, which had no coverage on the walkthrough side. Both new gates fail without the src change. npm test: 3386 tests / 3385 pass / 0 fail / 1 skipped. npm run typecheck clean. Smokes status_diagnostics and walkthrough_picker_to_first_query ok. Co-Authored-By: Claude <noreply@anthropic.com>
philcunliffe
commented
Aug 4, 2026
Neutral review of PR #608 (head |
| check | result |
|---|---|
npm test | 3386 tests / 3385 pass / 0 fail / 1 skipped |
npm run typecheck | clean |
npm run smoke -- status_diagnostics | ok |
npm run smoke -- walkthrough_picker_to_first_query | ok |
PR's own tests, src/ reverted to master | 6 tests / 2 pass / 4 fail (regression coverage confirmed) |
both new gates, src/ fix reverted | 9 tests / 7 pass / 2 fail (new coverage confirmed) |
Pushed as a70dc44.
PR #608 (fix/issue-604) already carries llp/0185-unpicked-client-stays-attached.decision.md and is further along, so this change set takes the next free number instead. LLP 0041's Extended-by forward-ref updated to match.
…ad unfiltered (#604) The status gate stands down on a local layer that is present but does not parse, and deliberately does not stand down when there is no config at all: nothing collects then, so a marker still on disk really is stranded. That boundary (`errorKind !== 'config_missing'`, not `!ok`) had no test, so widening the gate would have dropped the case silently. Adds one. `readCentralPluginNames` is deliberately not filtered by `enabled`, unlike the local set the caller builds: LLP 0185 #scope settles the org lane on the name alone, and filtering would hand the operator a detach for an org-owned client. Says so at the read, and scopes #not-configured-means-not-active to the local layer so the two sections cannot be read against each other. Co-Authored-By: Claude <noreply@anthropic.com>
philcunliffe
commented
Aug 4, 2026
Neutral review of PR #608, round 2 of 2 (head |
| check | result |
|---|---|
npm test (at a70dc44) | 3386 tests / 3385 pass / 0 fail / 1 skipped |
npm test (at d6eaa7d) | 3387 tests / 3386 pass / 0 fail / 1 skipped |
npm run typecheck | clean, both commits |
gate boundary, mutated to !localLoaded.ok | 4 pass / 1 fail (new test is load-bearing) |
| central read deleted from the finale | 4 pass / 1 fail (round 1's test is load-bearing) |
Pushed as d6eaa7d.
philcunliffe
commented
Aug 4, 2026
Neutral triage (fix-loop bound hit at 2 rounds, head
All residual findings are non-blocking. Opened #614 to track the deferred item so it doesn't get lost, since it does bear on the PR's stated purpose even though it isn't a merge blocker. Not merging or readying this PR; that is outside triage's scope. |
Uh oh!
There was an error while loading. Please reload this page.
PR #608 was squash-merged as 6ab0d99, so this branch's copies of its commits conflicted with the same content under new SHAs. Every conflicted hunk that was #608's takes master; the branch keeps only its own work (LLP 0188, writeAttachedNotConfiguredReminder, the wizard's firstLookShown gated repeat, and its tests). Co-Authored-By: Claude <noreply@anthropic.com>
… every boot (LLP 0184/0186/0187) (#622) * LLP 0184/0185: reconciler refused marker design, per issue #601 Routes the reconciler-retries-forever fix through the pipeline per the maintainer's guidance on #601: mints LLP 0184 (the request document, transcribed from the maintainer's pasted analysis) and LLP 0185 (the design), extending the Active LLP 0041 rather than editing it. LLP 0185 settles option 1 (a terminal `refused` marker state that short-circuits like `done` and surfaces as attention-needed in `hyp status`), widens ActionOutcome/ActionMarkerStatus/ClientActionState to carry it, and specifies how the transient/permanent bit crosses the kernel's throw-only attach() seam (markActionRefused/isActionRefused) so OpenClaw's ownership-conflict refusal and Claude's JSONC refusal both migrate without special-casing either adapter. Re-arm is the explicit `hyp attach` re-run only, per the maintainer's instruction; the isCurrent-style input-hash re-arm (LLP 0086 precedent) is recorded as a named follow-up candidate, not built. attempts-bounding on transient `failed` markers is left explicitly open. LLP 0041's Extended-by: line gets a mechanical forward-ref to LLP 0185; nothing it settled is edited. * Renumber the refused-marker design 0185 -> 0186 (collision with #608) PR #608 (fix/issue-604) already carries llp/0185-unpicked-client-stays-attached.decision.md and is further along, so this change set takes the next free number instead. LLP 0041's Extended-by forward-ref updated to match. * LLP 0187: reconciler-refused-marker executable plan (8 tasks) Turns LLP 0186's design (option 1, the terminal refused marker state per #601) into an eight-task graph: the widened ActionMarkerStatus/ActionOutcome type seam, the action_refusal.js marked-Error convention, the reconciler's unconditional refused short-circuit, the action_attach.js catch translation, the OpenClaw and Claude refusal-site migrations, the hyp status attention-needed surface, and the explicit hyp attach re-arm. * Widen action reconciler types for a terminal refused marker state Adds 'refused' to ActionMarkerStatus, ActionOutcome.status, and ReconcileActionResult.outcome, and adds the new ActionRefusalError interface (Error + required hypActionRefused: true) that lets a marked throw cross the kernel's throw-only attach() seam. Revises ActionMarker's doc comment to note a refused marker reuses `at` and `reason`, carries no `attempts`, and preserves `installed_assets` across rewrites the same way done/failed markers do. This is the type seam every other task in the reconciler-refused-marker change set (LLP 0186 / LLP 0187) builds on. Task-Id: T1 * T8: manual hyp attach re-arms a refused marker (LLP 0186/0187) A successful manual `hyp attach <client>` now clears whatever attach marker sits at that request key, mirroring the existing detach-side clearClientActionMarker call. This is the only re-arm a `refused` marker gets in this pass: with no marker left, the next reconcile pass treats the client as a fresh target and re-`perform()`s on its own. A failed manual attach never reaches the clear, so a refused marker survives a failed re-run unchanged. Adds test/core/attach-refused-rearm.test.js, mirroring test/core/detach-rejoin-recovery.test.js's pattern. Task-Id: T8 * The reconciler writes a terminal refused marker instead of retrying forever A precondition refusal only the user can fix (a conflicting provider entry, a JSONC settings file) came back as `failed`, so every reconcile pass re-performed it and bumped `attempts` forever (LLP 0184). The forward-gap loop now short-circuits a `refused` marker unconditionally, without consulting `markerIsCurrent()`: the freshness hook answers "did the input drift?", which says nothing about whether the refusal was resolved, and re-arming is the explicit `hyp attach` re-run only in this pass. A third outcome branch writes `{status, request_key, reason, at}` with no `attempts` (nothing will ever increment one) and carries `installed_assets` forward the way the `done` branch does, so a refusal on a re-perform never orphans what an earlier successful attach copied. The reverse gap treats a `refused` marker the way it treats a `failed` one: dropped when assetless, routed to `reverse()` when it recorded an effect. `runOutcome()` accepts the new shape, but only from a handler's return value: a bare throw stays `failed`. Task-Id: T3 * T7: hyp status surfaces terminal refused client actions (LLP 0186/0187) Widens ClientActionState with a refused member and renders it distinctly from failed in both hyp status --json (state/reason/at fall through existing spreads) and prose (a repair hint pointing at hyp attach), so a refused marker reads as attention-needed rather than a silent retry. Task-Id: T7 * Add action_refusal.js: markActionRefused/isActionRefused convention New sibling module to action_attach.js / action_backfill.js in src/core/config/. markActionRefused sets hypActionRefused: true on a thrown Error so it survives the kernel's throw-only attach(): Promise<void> seam; isActionRefused reads the marker back defensively, tolerant of any non-Error or unmarked throw. Realizes the ActionRefusalError interface LLP 0186/T1 already widened types.d.ts with. Task-Id: T2 * action_attach.js: translate a marked refusal into a refused outcome perform()'s catch now reads isActionRefused(err) to tell a permanent precondition refusal (LLP 0186) apart from a transient environmental failure, so the reconciler can short-circuit the former unconditionally instead of retrying it forever. Task-Id: T4 * T6: mark the Claude JSONC throw site as a permanent action refusal settings.js's JSONC detection throw site now wraps its ClaudeSettingsError with markActionRefused so the marked Error survives index.js's attach() rethrow, letting the reconciler treat it as a terminal refused outcome instead of retrying an edit attach can never safely make. Other ClaudeSettingsError throw sites are unchanged. Adds the first test coverage for this refusal path plus a companion case proving a plain malformed-JSON throw is not marked as refused. Task-Id: T6 * OpenClaw's ownership refusal is terminal, not a retried failure The `models.providers` ownership conflict (LLP 0167#attach-detach: a value HypAware did not write sits at a key attach owns) is a property of the user's config. No reconciler pass changes it, but the adapter reported it as `{status:'failed'}`, so the marker was retried on every pass with `attempts` climbing forever, which is LLP 0184's bug. `OpenclawAttachOutcome` gains `{status:'refused', reason}` and that one call site returns it. The other four `errorKind`s (`settings_path`, `endpoint`, `read`, `write`) are environmental and may well clear on their own, so they keep returning `{status:'failed'}` and keep retrying. Mechanically, `fail()`'s side effects split out into `reportAttachFailure()` and the refusal gets its own `refuse()` wrapper rather than a `status` argument on the shared five-call-site helper: there is then no parameter a future call site can pass wrong, and the four transient kinds cannot drift. Both surfaces `reportAttachFailure()` writes are unchanged, so `writeAttachOutput`'s `--json` payload and prose still say `status: 'failed'` for a refusal: the split is about how the reconciler schedules a retry, not about what the user's attach just did, and `--json` is a wire contract a scripted caller already parses. `index.js`'s wrapper throws `markActionRefused(new Error(reason))` for the refused outcome, alongside the unchanged bare-`Error` throw for `failed`. The kernel types the registered `attach()` as `Promise<void>`, so the mark is the only way the terminal/transient bit survives that seam to `perform()`'s catch. Tests: the three ownership-conflict assertions in openclaw-attach.test.js become `'refused'`; the missing- and malformed-`openclaw.json` cases still assert `'failed'`, proving the four environmental kinds did not migrate. At the wrapper seam, the rethrown refusal is asserted marked and a hard failure asserted unmarked. The `perform()`-level test stops asserting which non-`done` status the catch produces: that classification is action_attach.js's, tested there. Task-Id: T5 * Scope the refused-marker re-arm so it stops eating done markers and dry runs Review round 1 of PR #622. The re-arm added in T8 cleared the attach marker unconditionally after any successful manual `hyp attach`, which had two effects nothing in LLP 0186 asked for: - A `done` marker carrying `installed_assets` was deleted. That marker is the only record naming the files an org-driven attach installed, and `hyp detach` reads exactly it to know what to remove, so a manual re-attach followed by a detach left those files stranded (LLP 0138#marker-undo, the same invariant the reconciler's own carry-forward branches protect). - `hyp attach --dry-run` wrote to the marker store. `hyp detach --dry-run` returns before its own clear; the attach side did not. Gate the clear on the marker's prior status being `refused`, and skip it under --dry-run. That is exactly the re-arm the maintainer asked for on issue #601 and nothing more. Also strengthen the one end-to-end assertion over the whole OpenClaw chain back to the exact status (it had been weakened to `notEqual 'done'`, which would stay green if any link regressed to `failed`), and cover the Claude half of the seam at the registration level: the JSONC refusal only reaches the reconciler if `index.js`'s wrapper and `withSpan` rethrow the marked Error unchanged, which nothing tested. LLP 0186's re-arm and test-strategy sections are updated in the same commit to match. Co-Authored-By: Claude <noreply@anthropic.com> * Re-arm a refused marker without eating the undo record it carries Review round 2 of PR #622. Round 1 scoped the manual-attach re-arm to `refused` markers so a `done` one stops being collateral, but a `refused` marker is an undo record too: the reconciler's own `refused` write branch carries `installed_assets` forward across the rewrite precisely because a refusal on a re-`perform()` does not un-install what an earlier successful attach copied. Clearing such a marker is the same data loss round 1 fixed for `done`, one status over: `hyp detach` reads exactly that marker to know what to remove, so the org-installed files are stranded with nothing naming them. Reproduced against the real `attach()` command path (seeded a `refused` marker with `installed_assets`, ran attach, marker came back undefined). Re-arm now goes through `rearmRefusedActionMarker`, beside the store it rewrites. An assetless `refused` marker is still dropped outright, exactly as before. One that records assets is rewritten to `failed` with the record intact: `failed` is short-circuited by nothing, so it re-arms the forward gap exactly as a cleared marker does, the reverse gap and `hyp detach` keep reading the same undo record, and the next successful `perform()` unions the carried paths onto the fresh `done` marker. Also in this commit: - `hyp status` prints the refused repair hint unconditionally instead of from inside the reason guard, so a marker whose `reason` is empty or unreadable renders the next step rather than a bare `[refused]` (round 1's residual R1). - The reverse gap's failure arm now says why a `refused` reverse outcome deliberately lands there: `ActionOutcome` is one type across both hooks, so widening it made the status expressible on `reverse()`, but nothing produces it and dropping a marker over an unreversed effect is the more expensive wrong answer. Recorded as out of scope in LLP 0186 rather than left latent (round 1's residual R2). LLP 0186's re-arm, out-of-scope, and test-strategy sections are updated in the same commit. Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: test <test@test.com> Co-authored-by: test <test@example.com> Co-authored-by: Claude <noreply@anthropic.com>
Root cause
runPickerFinale(src/core/cli/walkthrough.js) only ever attaches pickedclients. Nothing detaches a client that a previous run attached and this run
left unchecked. The action reconciler's reverse lane
(
src/core/config/action_reconciler.js) undoes only config-named actionkeys, i.e. the org/central lane, so a solo machine's wizard-finale attach is
never reversed by it. Solo machines have been able to re-enter the picker via
Reconfigure since LLP 0129, so this is pre-existing on master.
Result, from the issue: unchecking Codex drops
@hypaware/codexand its gatewayupstream while
~/.codexstill routes through the gateway (capture stops,requests can fail); re-picking only OpenTelemetry composes no
@hypaware/ai-gatewayat all, so after the finale's daemon restart everystill-attached client points at a dead port.
What this implements
Option 2 (warn at finale time), plus option 3 (status diagnostic).
findAttachedNotConfiguredClients(src/core/cli/walkthrough.js): a client isstranded when its settings still carry a HypAware attach marker (read through
the same descriptor-driven probe
hyp statususes), it is not among this run'spicks, and neither the config the finale just wrote nor the org's central layer
enables its adapter. The finale prints the names, the consequence, and one
hyp detach --client <name>line each, before the daemon restart. Dry runscarry the finale's usual
(dry-run)tag.FinaleSummarygainsattachedNotConfigured?: string[]so the run's resultcarries what was printed.
hyp statusgainsclient_attached_not_configured: the mirror ofclient_attach_missing, repairing withhyp detach --client <name>. Gated tohosts with no central layer, because on a joined host the same shape is a
reconciler pass that has not run yet, not something an operator should undo by
hand. Known gap noted in the LLP: a local-layer strand on a managed host is
warned about by the finale but not by status.
Explicitly not option 1 (finale-side detach)
Auto-detaching on a menu confirm is a destructive, surprising behaviour change:
unchecking a picker row is not an instruction to rewrite
~/.codex/config.toml,and the picker has up to now only ever added. That is the maintainer's call to
make deliberately, not something a bug fix should decide on their behalf. The
warning is additive and reversible, and it makes the broken state visible without
committing to product behaviour. Options 1 and 3 stayed open; 3 is included here
because it was a small addition, 1 is not.
The warning surfaces the failure, it does not prevent it. A user who ignores
it is in exactly the state the issue describes. Option 1 is the complete fix if
you want it, and it is now cheap: the detection it needs is the function this PR
adds, and the undo it needs (
detachClientFromDisk) already exists.The org/central attach markers are untouched. A client whose adapter the
central layer names is never counted as stranded (the finale reads the central
layer read-only, for plugin names only), and the status diagnostic stays silent
on joined hosts. Nothing here writes to a settings file or a marker store.
Scope note: this PR does not touch
src/core/cli/wizard/pick.jsorcomposePickerConfig, which #603 covers in parallel.Ground truth
Pre-fix (
node --test test/core/walkthrough-unpicked-attached-clients.test.js test/core/status-client-attached-not-configured.test.js, withsrc/reverted tomaster):
Post-fix, same command:
Full suite (
npm test):# tests 3383 / # pass 3382 / # fail 0 / # skipped 1.npm run typecheck: clean.Smokes:
status_diagnostics,walkthrough_picker_to_first_queryandwalkthrough_backfill_client_historyare green.walkthrough_to_first_query,client_attach_idempotentandclaude_attach_detachfail in this environment, and fail identically on cleanmaster (verified by stashing the change), so they are pre-existing here.
One existing test needed a one-line fix:
status-first-sync-hold.test.jsasserted
diagnosticsdeep-equals[]while inheriting the runner's real$HOME, so the new diagnostic fired from the developer's own attached Claude. Itnow passes an isolated
homeDir, which is what that assertion always meant.Fixes#604