Client attach on join: auto-attach clients when the daemon binds (LLP 0044/0045/0046)#179
Draft
philcunliffe wants to merge 24 commits into
Draft
Client attach on join: auto-attach clients when the daemon binds (LLP 0044/0045/0046)#179philcunliffe wants to merge 24 commits into
philcunliffe wants to merge 24 commits into
Conversation
New src/core/config/attach_policy.js — the backfill_policy.js twin and single source of truth for interpreting a client adapter plugin entry's `config.attach.on_join` block, shared by the attach handler (T6) and the declared-attach status surface (T9) so the two can never disagree. Tri-state, mirroring readBackfillPolicy: - absent block → default-on (onJoin: undefined) - on_join: true/false → that value - present-but-malformed (non-boolean on_join) → fail-safe opt-out (false); silently editing a user-owned client settings file on a malformed opt-out is the wrong default, so do not fail open. Pure; unit-tested over the full tri-state plus non-object attach blocks and the `onJoin !== false` off-switch contract both consumers rely on. @ref LLP 0044#where-attach-is-declared @ref LLP 0045 Task-Id: T2 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…oint) (LLP 0045) Extend ActionContext and ReconcileInput in src/core/config/types.d.ts with the three optional fields the attach handler reads, mirroring LLP 0045 §Part 1: - clientDescriptors?: Map<string, ClientDescriptor> — the static client→plugin map (from buildPluginCatalog) used to enumerate desired() and to hand the disk-driven undo each descriptor's attachProbe. - clients?: AiGatewayCapability — runtime gateway capability used only to invoke a client's attach effect (getClient(name).attach). - endpoint?: string — the local gateway base URL clients attach to. All optional and daemon-only by construction (a plain CLI boot leaves them unset). Type-only; no behaviour change until a handler (T6) or the daemon (T7) reads them. Imports AiGatewayCapability from the kernel types and ClientDescriptor from plugin_catalog.js. typecheck, lint, and the full test suite (1455 pass) stay green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Task-Id: T3
Add validateAttachSection beside validateBackfillSection in the claude and codex config.js, wired into validateClaudeConfig / validateCodexConfig. It validates the optional config.attach.on_join (boolean, default true) and rejects unknown attach keys, mirroring the backfill section validator. Plugin-local and additive: no top-level/core schema change. Realizes LLP 0045 Part 4 (attach.on_join rides the client adapter's own config block, validated by that plugin's config-section validator). Inert until the attach handler (T6) and daemon wiring (T7) read the policy. Tests cover accept/reject for both plugins, pointer mounting, and the LLP 0031 central-lock merge-drop (a local attach.on_join cannot flip a central-locked entry). Task-Id: T8 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Make each client adapter's attach marker carry everything the single core undo (task 4) needs to reverse the attach from disk alone, with the plugin unloaded. claude (`_hypaware` marker): record `prev_base_url` (the restore target) plus the managed `env.ANTHROPIC_BASE_URL` and session-context hook entries. An idempotent re-attach preserves the marker's recorded original `prev_base_url` instead of backing up our own gateway URL over it, and reports that original via the attach result. codex (`# BEGIN/END hypaware` marked block): already self-delimiting and already records the prior `model_provider` as `# previous_model_provider` — locked in with tests (block is self-describing; re-attach keeps the original pointer). Unit-tested via the marker contents (no plugin/gateway loaded). Adapter-local; no core contract change (the `json: true` one-line attach output shape is unchanged). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Task-Id: T1
…LP 0045/0046 T4) Add `src/core/config/client_detach_disk.js` — the one detach implementation. It reverses a client's attach from disk state alone: the descriptor's `attachProbe` locates the settings file and the settings-file marker is the self-describing undo record `attach()` wrote (LLP 0045 Part 3, the contract task T1 landed). Format-aware but plugin-agnostic, reusing the same `resolveClientSettingsPath` + `attachProbe` format dispatch (`json` marker-key / `toml` managed-block) the read-side `probeClientAttached` uses, and importing no plugin code (which would not survive the plugin being unloaded at reverse time): - json (claude `_hypaware`): restore-or-remove each managed `env` key from the recorded `prev_base_url`, strip the recorded managed hook entries (matched by event/matcher/exact command so no orphaned `hyp claude-hook` survives), and delete the marker. A pre-existing foreign base URL round-trips byte-for-byte; the no-pre-existing-URL fixture round-trips to empty; an externally-overridden value is left in place with a warning. - toml (codex `# BEGIN/END hypaware …`): strip the self-delimiting managed blocks and restore the prior `model_provider` recorded as `# previous_model_provider`. The managed-block convention is now a core-understood format contract, subsuming the codex adapter's old marked-block strip + model_provider restore. Atomic, mtime-gated writes preserve the adapters' concurrent-edit guarantee. Unit-tested on fixture settings files with no plugin loaded (incl. hand-written markers), proving reverse never depends on `ctx.clients`; cross-checked against `probeClientAttachFromDescriptor` for both formats. No second caller is wired yet (T5 reroutes `hyp detach`, T6 the reconciler `reverse()`), so this lands inert and green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Task-Id: T4
buildClientActionsReport (src/core/daemon/status.js) gains an attach declared-targets derivation symmetric to backfill's. It reuses the clientDescriptors-derived client-adapter plugin set and the shared readAttachPolicy (T2) so status can never disagree with the attach handler about what an `attach.on_join` block means. Interpretation, per LLP 0044 §status-surface: - enabled client adapter on a joined host, no marker -> pending - attach.on_join:false, or a non-joined host -> n/a - a `done` marker -> attached (the done rendering) Attach targets are keyed by *client* name (the handler's request key, descriptor.name) — not the owning plugin — so a `done` attach marker collapses with the declared target instead of doubling it. A failed/pending attach is its own status line and never flips `overall` to degraded (client-action state is not a diagnostic). It reads markers only; it never runs a reconcile pass. Tests: mixed done/failed/pending/n-a read cleanly; on_join:false and a non-joined explicit target both render n/a while a bare local install keeps the V1 surface; a failed attach stays healthy; a done marker collapses with its declared target (no double row). Task-Id: T9 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…) (LLP 0044/0045 T6)
The reversible instance of the client-action reconciler — the action_backfill.js
twin and the first handler to implement reverse(). New
src/core/config/action_attach.js exporting createAttachHandler(opts) and the
default attachHandler:
- desired() iterates ctx.clientDescriptors ∩ enabled plugins ∩ readAttachPolicy
(the attach_policy.js opt-out), guarded on ctx.clients.getClient(name) being
present so it never names a client perform() can't reach. Daemon-only by
construction: inert when clientDescriptors or clients is absent (a plain CLI
boot).
- perform() is in-process (a bounded settings write, not a subprocess): resolves
the runtime registration, calls attach({ endpoint: ctx.endpoint, config:{},
stdout, stderr, json:true }), parses the one-line JSON, and records the marker
detail (settings_path, prev_value). A throw becomes a failed outcome the
reconciler retries.
- reverse() is disk-driven, not adapter-driven: it runs after the staged restart
has unloaded the adapter, so it replays the single core undo
(detachClientFromDisk) from the descriptor's attachProbe + the self-describing
marker, never touching ctx.clients.
Adds the attach handler's type surface (ClientDetachFromDisk seam +
CreateAttachHandlerOptions) to config/types.d.ts, the direct analog of
CreateBackfillHandlerOptions. Unit-tested (22 cases) with injected fake
clientDescriptors + clients + fs, including a real fs round-trip proving reverse
runs with no adapter loaded. The reconciler is left untouched (threading the new
context fields onto ctx is T7's daemon wiring).
Task-Id: T6
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…core undo (LLP 0045 T5) Drop `detach` from `AiGatewayClientRegistration` (kernel type) and from the claude/codex `registerClient()` calls, and point `runClientLifecycle`'s detach branch at the single T4 core disk-driven undo (`detachClientFromDisk`), resolved per client via its `clientDescriptor`. There is now exactly one undo, shared by manual `hyp detach` and the daemon reconciler's `reverse()`, so the two cannot drift. - collectivus-plugin-kernel-types.d.ts: remove `detach()` from `AiGatewayClientRegistration` and the orphaned `AiGatewayClientDetachContext`; refresh the registerClient/getClient doc comments to attach-only. - claude/codex adapters: drop the registration `detach` property and the now-unused `writeDetachOutput`/type import (the settings-file `detach()` writers stay as exported utilities). - ai-gateway api.js: `registerClient` now requires only `attach()`. - core_commands.js: new `detachClientViaCore` + `writeCoreDetachOutput` emit the `client.detach` span and a plugin-agnostic `done`/no-op output via the core undo; the descriptor map is built once per detach invocation. - Tests/smoke updated for the rerouted path: ai-gateway-api (attach-only validation), command-dispatch + integration (detach is the disk undo, HOME isolated), and the claude_attach_detach smoke prose. claude_attach_detach / client_attach_idempotent smokes stay green. Task-Id: T5 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…Handler, backfillHandler] (LLP 0045 T7) Wire the attach handler into the daemon's client-action reconciler (LLP 0045 §Part 1 / Module-seam-breakdown item 7): - runtime.js: resolve the client-action seam from boot — clientDescriptors from the plugin catalog (now surfaced on BootKernelResult), and clients/endpoint from boot.runtime.capabilities when the ai-gateway capability is present, via localEndpoint() with the configuredGatewayEndpoint fallback. Thread all three onto each reconcile() input and register [attachHandler, backfillHandler] — attach first (exported DEFAULT_ACTION_HANDLERS so the order is a testable invariant). - action_reconciler.js: thread the new context fields (clientDescriptors / clients / endpoint) onto the ActionContext so a client handler can read them. - boot.js / runtime types: surface the boot catalog's clientDescriptors on BootKernelResult. - gateway_endpoint.js: extract configuredGatewayEndpoint/endpointFromListen into a shared module (was duplicated in core_commands.js and walkthrough.js; the daemon is the third consumer). Tests: daemon-reconcile.test.js gains the handler-order invariant and a gateway-enabled boot asserting clientDescriptors/clients/endpoint are threaded (and undefined on a non-gateway boot). New hermetic smoke client_attach_on_join drives join -> auto-attach -> no-re-attach -> config-drop reverse end to end. typecheck, lint, full test suite (1522 pass), and the new + existing attach/join smokes are green. (Pre-existing unrelated red: walkthrough_to_first_query, also red on the untouched integration branch.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Task-Id: T7
Marks the client-attach implementation design as built-and-merged now that the whole change set (T1-T9) has landed. Active = shipped marker per the project lifecycle (sibling design LLP 0041 is already Active). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Task-Id: T10
Contributor
Author
Dual-agent review —
|
| Finding | Codex | Claude | Verified | Severity |
|---|---|---|---|---|
Legacy Claude _hypaware marker (port-only, written by master) -> core undo deletes marker but orphans ANTHROPIC_BASE_URL + hooks, non-retryable |
#7 (cat 7, high) | lanes 2/3/6 (84) | yes -- git show origin/master + traced |
major |
Manual hyp detach gated on live gateway.getClient() -- cannot detach a dropped/unloaded adapter despite the disk-driven design |
#1 (cat 1, high) | -- | yes (core_commands.js:3277) | major |
| Detach descriptor map bundled-only vs boot/status bundled+installed -- installed client adapters attachable but not detachable | #2 (cat 2, med) | -- | yes, latent (claude/codex are bundled) | major |
@typedef DetachFromDiskResult violates the repo "no @typedef" rule |
-- | lane 1 (88) | yes | minor |
Prose Part 7 cites a non-existent LLP 0045 section |
-- | lane 5 (80) | yes | nit |
Codex TOML undo is symmetric/complete (writer emits # previous_model_provider, core reads the same key). Idempotency, both-format byte-for-byte round-trips, and attach-before-backfill ordering are all asserted in tests.
Codex review
Fix Validations
Silent gap: joined daemon did not auto-attach clients
- Status: correct
- Evidence: src/core/daemon/runtime.js:317, src/core/daemon/runtime.js:327, src/core/daemon/runtime.js:820, src/core/config/action_attach.js:73, src/core/config/action_attach.js:151
- Assessment: The daemon now registers the attach handler ahead of backfill, threads descriptors/gateway/endpoint into reconciliation, and
perform()invokes the adapterattach()with the resolved endpoint.
Findings
1) Behavioral Correctness
- Severity: major
- Confidence: high
- Evidence: src/core/cli/core_commands.js:3260, src/core/cli/core_commands.js:3277, src/core/cli/core_commands.js:3314, src/core/cli/core_commands.js:3350
- Why it matters: Manual
hyp detach <client>still requires the client to be live in the gateway registry, so it cannot use the new disk-driven undo for an attached client whose adapter was dropped or unloaded. - Suggested fix: For
action === 'detach', expand and validate targets from the descriptor map instead ofgateway.listClients()/gateway.getClient(). Keep the gateway registry requirement only forattach.
2) Contract & Interface Fidelity
- Severity: major
- Confidence: medium
- Evidence: src/core/runtime/boot.js:148, src/core/daemon/status.js:153, src/core/daemon/status.js:156, src/core/cli/core_commands.js:3677
- Why it matters: Boot/status build client descriptors from bundled plus installed plugins, but manual detach builds descriptors from bundled plugins only, so installed client adapters can attach but cannot be detached through the new core undo.
- Suggested fix: Build the CLI detach descriptor map with the same bundled + installed catalog inputs used by boot/status, using the command's resolved state root.
7) Resource Lifecycle & Cleanup
- Severity: major
- Confidence: high
- Evidence: src/core/config/client_detach_disk.js:128, src/core/config/client_detach_disk.js:133, src/core/config/client_detach_disk.js:145, hypaware-core/plugins-workspace/claude/src/settings.js:144, hypaware-core/plugins-workspace/claude/src/settings.js:146, hypaware-core/plugins-workspace/claude/src/settings.js:155
- Why it matters: Legacy Claude
_hypawaremarkers withoutmanaged.env/hookswill have the marker deleted while the gatewayANTHROPIC_BASE_URLand hooks remain, making cleanup non-retryable. - Suggested fix: Add a compatibility branch for old Claude markers: when
managedis absent butmarker.portexists, remove the matchingANTHROPIC_BASE_URLand strip the legacy session-context hooks before deleting the marker, or leave the marker intact and report an unsupported legacy undo.
No Finding
- Change Impact / Blast Radius
- Concurrency, Ordering & State Safety
- Error Handling & Resilience
- Security Surface
- Release Safety
- Test Evidence Quality
- Architectural Consistency
- Debuggability & Operability
Claude review
Claude review
Core disk-driven undo leaves a pre-upgrade (master-era) Claude _hypaware marker half-reversed — orphans ANTHROPIC_BASE_URL + hooks
- Severity: major
- Confidence: 84
- Evidence: src/core/config/client_detach_disk.js:128-163 (esp. 128-130, 133, 145)
- Why it matters:
origin/mastershipshyp attachwriting a Claude marker of shape{ attached_at, version, port, state_file }— nomanaged, noprev_base_url(confirmed viagit show origin/master:.../claude/src/settings.js). This PR's single undodetachJsonMarkerderives everything it reverses frommarker.managed.env/marker.managed.hooks/marker.prev_base_url; against a legacy marker those are all absent, somanagedEnv={}andhookEntries=[]— the env loop never runs andstripManagedHooksstrips nothing — yet line 133 deletes the marker unconditionally and reportschanged:true("✓ Detached"). Net:env.ANTHROPIC_BASE_URLstays pointed at a now-deadhttp://127.0.0.1:<oldport>(Claude Code is bricked), the fourhyp claude-hook session-contextentries remain orphaned, and because the marker is gone a re-runhyp detach/hyp statusreports "nothing to do" — unrecoverable without hand-editing. The retired adapterdetach()(still dead-present at claude/settings.js:135-160, 311-328) handled exactly this viamarker.port+ theclaude-hook session-contextpattern. Reached via manualhyp detachafter upgrade (and single-user/non-joined installs); the daemon auto-reverse path is shielded because theclient-actions.jsonstore is new so the daemon has no master-era attach to reverse, and a fresh auto-attach rewrites the marker to new format. Three independent review lanes and Codex (cat 7) converged on this. (Codex is unaffected: its# BEGIN/END hypawareblock +# previous_model_providerformat is unchanged by this PR, so the core undo parses old and new identically.) - Suggested fix: In
detachJsonMarker, whenmarker.managedis absent, fall back to the legacy reversal: removeenv.ANTHROPIC_BASE_URLwhen it equalshttp://127.0.0.1:${marker.port}and strip session-context hooks by theclaude-hook session-contextcommand pattern (the logic still present in the now-deaddetach()); add aclient-detach-disk.test.jscase seeding amanaged-less{ attached_at, version, port, state_file }marker asserting the env key and hooks are gone.
Shared type DetachFromDiskResult defined via forbidden @typedef instead of an interface in a .d.ts
- Severity: minor
- Confidence: 88
- Evidence: src/core/config/client_detach_disk.js:72 (
@typedef {object} DetachFromDiskResult); imported cross-file at src/core/config/types.d.ts (import type { DetachFromDiskResult } from './client_detach_disk.js') - Why it matters: The repo Code Style rule is explicit — "Do not use
@typedefin JSDoc. Define shared types asinterfaces in.d.tsfiles and import them via@import."DetachFromDiskResultis a cross-file shared type (it backs the exportedClientDetachFromDisktype), yet it is the lone@typedefin the PR while its siblings (ClientDetachFromDisk,CreateAttachHandlerOptions) are correctly interfaces intypes.d.ts. - Suggested fix: Move
DetachFromDiskResultintosrc/core/config/types.d.tsasexport interface DetachFromDiskResult {...}, delete the@typedefblock, and reference it via@importinclient_detach_disk.js.
Comments/test cite a non-existent "§Part 7" of LLP 0045
- Severity: nit
- Confidence: 80
- Evidence: src/core/config/action_attach.js:233 ("(LLP 0045 §Part 7)"); test/core/daemon-reconcile.test.js:359 and test title :362
- Why it matters: LLP 0045 has no Part 6/7 — its sections are Parts 1-5 plus "Module / seam breakdown"; the attach-first/live-capture-leads-backfill rationale these comments cite lives in "Module / seam breakdown", which the adjacent machine-checked
@refannotations correctly target. The prose label is internally inconsistent and points a reader at a section that does not exist. - Suggested fix: Replace "§Part 7" with "§Module / seam breakdown" (or "task T7" of LLP 0046) in those three spots so the prose matches the neighboring
@ref.
Lanes that cleared (no >=80 findings): contract & callers (full suite 1522 pass / 0 fail; every changed/removed symbol's call sites consistent; AiGatewayClientRegistration.detach fully retired with zero remaining call sites). Coverage confirmed for the new-format undo: byte-for-byte attach→detach round-trips for both claude _hypaware (hooks fully stripped, no orphans) and codex # BEGIN/END hypaware blocks, exactly-once idempotency (no_reattach marker+settings stability), and attach-before-backfill ordering asserted as a unit invariant. Daemon reinstall race (#147) and OTEL self-loop guard untouched and uninvolved.
Reviewed at head f77ed3f0c34f90943ad858df4f70953427e75be7. Generated by the dual-review skill (Codex gpt-5.5 + parallel Claude lanes).
…etach; bundled+installed descriptors; type+doc fixes (#179 review) Dual-review request_changes fixes for PR #179 (integration/client-attach): 1. Legacy-marker half-reversal: detachJsonMarker now falls back to the pre-self-describing convention when marker.managed is absent — removes ANTHROPIC_BASE_URL at the recorded port and strips claude-hook session-context hooks, so a manual `hyp detach` after upgrade fully reverses with nothing orphaned. Logic moved from the now-retired claude-adapter detach() into the one core undo. 2. Manual detach no longer gated on the live gateway: runClientLifecycle resolves/validates the detach target from the bundled+installed descriptor map, so a dropped/unloaded adapter's on-disk attach still reverses. 3. buildClientDescriptorMap now built from bundled+installed (matching boot/status), so installed (non-bundled) client adapters resolve. 4. DetachFromDiskResult moved from a @typedef to an interface in config/types.d.ts, imported via @import. 5. Relabel "§Part 7" prose to "§Module / seam breakdown item 7". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0178wfuBrVQHwPbsHAtcE7YS
Contributor
Author
Dual-agent review (round 2 — re-review) —
|
| Round-1 finding | Sev | Fix in this head | Test |
|---|---|---|---|
Legacy Claude _hypaware marker (no managed) orphaned ANTHROPIC_BASE_URL + hooks |
major | detachLegacyJsonMarker/stripLegacyClaudeHooks (client_detach_disk.js:278/318) — removes base URL only when == http://127.0.0.1:${port}, warn+leave on external override, strips claude-hook session-context hooks; faithful to the retired adapter detach() |
client-detach-disk.test.js:4439, :4483 |
Manual hyp detach resolved targets from live gateway |
major | runClientLifecycle detach branch → buildClientDescriptorMap + expandDetachClientNames + clientDescriptors.get (core_commands.js:3266-3322); no gateway.getClient on detach |
integration.test.js:4937 (dropped adapter) |
| Detach descriptor map bundled-only vs boot/status bundled+installed | major | buildClientDescriptorMap now buildPluginCatalog(bundled, installed) (core_commands.js:3709), byte-identical inputs to boot/status |
integration.test.js:4961 (INSTALLED adapter) |
@typedef DetachFromDiskResult violates no-@typedef rule |
minor | moved to interface in src/core/config/types.d.ts:582; @import-ed |
— |
Prose §Part 7 cites non-existent LLP 0045 section |
nit | replaced with §Module / seam breakdown item 7 (action_attach.js:233 + test title); zero Part 7/Part 6 remain |
— |
No regressions introduced by the fixes: the removed claude/src/settings.js#detach has zero remaining callers; the legacy-fallback hardcoding matches the retired adapter byte-for-byte and legacy JSON markers were Claude-only.
Why request_changes despite the round-1 fixes being clean
The mechanical verdict is driven entirely by 3 Codex majors, none of which is a regression from the round-1 fixes — all are false-positive or pre-existing:
- Codex [codex] Add durable cache spool #1 (desired() reverses all markers if
ctx.clientsis transiently unavailable): false positive — the daemon resolves the client seam once at boot (runtime.js:343, after the gateway source binds) and closes it over every pass, soctx.clientsis stable for the daemon’s life; there is no transient-empty window. - Codex [codex] Remove OpenTelemetry npm dependencies #2 (manual detach gated on the
@hypaware/ai-gatewaycapability): the capability gate (core_commands.js:3223) is pre-existing (commit946e0bf, outside every changed hunk) and unchanged by this PR. The round-1 bug — detaching a dropped/unloaded adapter while the gateway is present — is fixed. A gateway-less disk undo is a separate optional enhancement. - Codex [codex] add PR checks #4 (
readTextcaptures mtime after the content read — TOCTOU): thereadFile-then-statordering is inherited verbatim from the retired adapterreadSettings(origin/master settings.js:153/163); not introduced here. Tiny window, requires a concurrent settings.json edit.
Risk capstone
Cross-reference: reviewer findings vs high-risk surfaces
| Source | Finding (severity, evidence) | Intersects |
|---|---|---|
| Codex #1 | major — desired() reverses all markers if ctx.clients transiently unavailable (action_attach.js:72) | Concurrency surface — FALSE POSITIVE: seam resolved once at boot, stable for daemon life |
| Codex #2 | major — manual detach gated on ai-gateway capability (core_commands.js:3223) | Risks bullet 2 — pre-existing gate, unchanged by PR; round-1 target-resolution bug fixed |
| Codex #4 | major — readText stat-after-read TOCTOU (client_detach_disk.js:685/692) | Concurrency surface — pre-existing pattern inherited from retired adapter readSettings |
| Claude | minor — orphaned ClaudeDetach* types (claude/src/types.d.ts:84,88) | Cross-package usage — dead types, harmless |
| Claude | minor — new inline import() types (core_commands.js:3712,3714) | follows file's pre-existing convention |
Codex review
Fix Validations
Silent-gap auto attach on join
- Status: correct
- Evidence:
src/core/daemon/runtime.js:66,src/core/daemon/runtime.js:327,src/core/daemon/runtime.js:377,src/core/config/action_attach.js:151,hypaware-core/smoke/flows/client_attach_on_join.js:110 - Assessment: The daemon now registers attach before backfill, threads the client seam into reconciliation, and the smoke verifies the attach marker reaches
doneand client settings are written.
Disk-driven reverse after adapter drop
- Status: correct
- Evidence:
src/core/config/action_attach.js:187,src/core/config/action_attach.js:205,hypaware-core/smoke/flows/client_attach_on_join.js:200,hypaware-core/smoke/flows/client_attach_on_join.js:207 - Assessment: The daemon reverse path no longer calls adapter detach; it uses the descriptor-backed disk undo and the smoke covers post-restart reverse after
@hypaware/claudeis dropped.
Findings
1) Behavioral Correctness
- Severity: major
- Confidence: high
- Evidence:
src/core/config/action_attach.js:72,src/core/config/action_attach.js:96,src/core/config/action_reconciler.js:200,src/core/config/action_reconciler.js:211,test/core/action-attach.test.js:221 - Why it matters: For a reversible handler,
desired() === []means “reverse every existing marker”; the attach handler treats missingctx.clientsas inert, but the reconciler will detach previously attached clients if the gateway registry is transiently unavailable. - Suggested fix: Do not gate
desired()on the live client registry; derive desired targets from descriptors + config + policy, and letperform()fail/retry whenctx.clientsorendpointis missing.
2) Contract & Interface Fidelity
- Severity: major
- Confidence: high
- Evidence:
src/core/cli/core_commands.js:3223,src/core/cli/core_commands.js:3258,src/core/cli/core_commands.js:3266,test/core/integration.test.js:133 - Why it matters: Manual
hyp detachis documented as descriptor/disk-driven, but it still refuses to run unless@hypaware/ai-gatewayis activated, blocking recovery when the gateway plugin itself is removed or unavailable. - Suggested fix: Branch detach before the gateway capability check: build the descriptor map and call
detachClientViaCore; keep the gateway requirement only for attach.
4) Concurrency, Ordering & State Safety
- Severity: major
- Confidence: medium
- Evidence:
src/core/config/client_detach_disk.js:685,src/core/config/client_detach_disk.js:692,src/core/config/client_detach_disk.js:723,src/core/config/client_detach_disk.js:733 - Why it matters: The disk undo reads file contents before taking the mtime snapshot, so an edit between
readFileandstatbecomes the expected mtime and can be overwritten by stale transformed content. - Suggested fix: Capture
statbefore and after read, or open/read/fstat the same handle and only accept a stable mtime/inode snapshot; add an injected-fs regression test for the interleaving.
No Finding
-
- Change Impact / Blast Radius
-
- Error Handling & Resilience
-
- Security Surface
-
- Resource Lifecycle & Cleanup
-
- Release Safety
-
- Test Evidence Quality
-
- Architectural Consistency
-
- Debuggability & Operability
Evidence Bundle
- Changed hot paths:
runClientLifecycle,createAttachHandler,createActionReconciler,detachClientFromDisk, daemonDEFAULT_ACTION_HANDLERS, status client-action reporting. - Impacted callers:
hyp attach/detach/unattachviasrc/core/cli/core_commands.js:3216; daemon reconcile viasrc/core/daemon/runtime.js:327; attach reverse viasrc/core/config/action_attach.js:187; status viasrc/core/daemon/status.js:576. - Impacted tests:
test/core/action-attach.test.js:221,test/core/integration.test.js:113,test/core/daemon-reconcile.test.js:362,test/core/client-detach-disk.test.js:435,hypaware-core/smoke/flows/client_attach_on_join.js:110. - Unresolved uncertainty: I did not run the suite; review is based on the supplied diff plus targeted caller/contract traces.
Claude review
Claude review
Round-2 re-review of the five round-1 findings (all confirmed resolved with tests) plus a regression sweep of the fixes. Three parallel lanes (guidance / shallow-bug / contracts-callers). Round-1 majors verified fixed: legacy-marker detach fallback (detachLegacyJsonMarker/stripLegacyClaudeHooks, faithful to the retired adapter detach()), descriptor-map manual detach (expandDetachClientNames + clientDescriptors.get, no live-gateway read), bundled+installed descriptor map (buildClientDescriptorMap via buildPluginCatalog(bundled, installed)), DetachFromDiskResult moved to an interface, and the §Part 7 → §Module / seam breakdown label fix.
Orphaned ClaudeDetachOptions / ClaudeDetachResult left after the adapter detach() removal
- Severity: minor
- Confidence: 85
- Evidence: hypaware-core/plugins-workspace/claude/src/types.d.ts:84,88
- Why it matters: The PR deletes the only consumer of these types (the dead
detach()inclaude/src/settings.jsand its@import), so both are now dead exported types — grep finds only their definitions, zero references; this is the round-1 cleanup that was started (the@importwas trimmed) but not completed in the unmodifiedtypes.d.ts. Harmless (a dead.d.tstype has no runtime effect; no dangling import), but stale. - Suggested fix: Delete the
ClaudeDetachOptionsinterface andClaudeDetachResulttype alias fromclaude/src/types.d.ts.
New inline import('...') type annotations violate the "no inline import types" house rule
- Severity: minor
- Confidence: 80
- Evidence: src/core/cli/core_commands.js:3712,3714 (
/** @type {import('../manifest.js').LoadedManifest[]} */inbuildClientDescriptorMap) - Why it matters: CLAUDE.md is explicit — "Never use inline
import('...')types. Declare type imports at the top of the file with@importJSDoc comments, then reference the bare names." These are added by this PR. Caveat lowering confidence:core_commands.jsalready carries ~23 pre-existing inlineimport('...')usages, so the new lines follow the file's existing (rule-violating) local convention rather than introducing a new pattern. - Suggested fix: Add
@import { LoadedManifest } from '../manifest.js'to the top-of-file@importblock and change the two annotations to/** @type {LoadedManifest[]} */.
Reviewed at head 7432284a1da605093f71de362be7d3af4eeab2eb. Round-2 re-review by the dual-review skill (Codex gpt-5.5 + parallel Claude lanes).
philcunliffe
added
the
neutral:stuck
Contributor
Author
🤖 neutral:
|
philcunliffe
removed
the
neutral:stuck
… drop dead types; hoist import types; document stable client seam (#179 round-2) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0178wfuBrVQHwPbsHAtcE7YS
Contributor
Author
Dual-agent review —
|
| Source | Finding (severity, evidence) | Intersects |
|---|---|---|
| Codex #1 | major — localEndpoint fallback to configured listen (runtime.js:837/842, action_attach.js:133/150) | Config field chain (gateway listen); pre-existing, not round-3 |
| Codex #4 | major — transient registration → reverse gap (action_attach.js:90/96, action_reconciler.js:127/200/212) | Reverse-gap over-fire risk; round-2 #1 re-raise, moot per seam-stable design |
| Codex #2 | major — detach output contract drift (core_commands.js:3442/3472/3481) | Detach output contract risk; intentional/documented |
| Codex #7 | minor — tmp-file cleanup on partial write (client_detach_disk.js:748-758) | Temp-file cleanup risk; pre-existing |
| Claude | none (4 subagents clear; suite green) | — |
Codex review
Fix Validations
Silent-gap attach on join (#126)
- Status: incomplete
- Evidence:
src/core/daemon/runtime.js:317,src/core/daemon/runtime.js:373,src/core/config/action_attach.js:72,src/core/config/action_attach.js:150 - Assessment: The happy path is wired: daemon registers attach before backfill and threads client registry/endpoint into the reconciler. The edge cases below still violate the “attach when daemon binds” contract and can reverse a still-desired attach.
Findings
1) Behavioral Correctness
- Severity: major
- Confidence: high
- Evidence:
src/core/daemon/runtime.js:837,src/core/daemon/runtime.js:842,src/core/config/action_attach.js:133,src/core/config/action_attach.js:150 - Why it matters: If
clients.localEndpoint()throws because the gateway did not bind, the daemon still falls back to the configured listen URL and writes client settings to a potentially dead or occupied port. - Suggested fix: For daemon auto-attach, require the live
localEndpoint()result or a proven started gateway source; keepconfiguredGatewayEndpoint()fallback for manualhyp attach/init only.
4) Concurrency, Ordering & State Safety
- Severity: major
- Confidence: high
- Evidence:
src/core/config/action_attach.js:90,src/core/config/action_attach.js:96,src/core/config/action_reconciler.js:127,src/core/config/action_reconciler.js:200,src/core/config/action_reconciler.js:212 - Why it matters: A transient missing adapter registration makes
desired()omit a client even when config still enables its plugin, so an existing marker becomes a reverse gap and the reconciler detaches the client. - Suggested fix: Make attach desired-state depend on config policy/descriptors only; let
perform()fail when the live registration is unavailable, or add a non-reversing “unavailable” path.
2) Contract & Interface Fidelity
- Severity: major
- Confidence: medium
- Evidence:
src/core/cli/core_commands.js:3442,src/core/cli/core_commands.js:3472,src/core/cli/core_commands.js:3481,src/core/cli/core_commands.js:3482 - Why it matters: The new core detach output emits only generic
settings_pathand value-only prose, which changes the retired Codex JSON/human contract that exposedconfig_path,base_url=..., andmodel_provider=.... - Suggested fix: Preserve adapter-compatible aliases/labels from the descriptor or client name, especially
config_pathfor Codex JSON, or explicitly version/document the CLI JSON contract and update coverage.
7) Resource Lifecycle & Cleanup
- Severity: minor
- Confidence: high
- Evidence:
src/core/config/client_detach_disk.js:748,src/core/config/client_detach_disk.js:752,src/core/config/client_detach_disk.js:753,src/core/config/client_detach_disk.js:758 - Why it matters: If temp-file open/write/sync succeeds partially and then fails before rename, cleanup is skipped because
rm(tmpPath)only runs in the rename catch. - Suggested fix: Wrap the whole temp-file write/sync/close/rename sequence in a catch that removes
tmpPathwhen it was created, preserving the original error.
No Finding
- Change Impact / Blast Radius
- Error Handling & Resilience
- Security Surface
- Release Safety
- Test Evidence Quality
- Architectural Consistency
- Debuggability & Operability
Evidence Bundle
- Changed hot paths:
src/core/config/action_attach.js:72,src/core/config/action_attach.js:119,src/core/config/action_attach.js:187,src/core/config/client_detach_disk.js:85,src/core/daemon/runtime.js:317,src/core/cli/core_commands.js:3230,src/core/daemon/status.js:576 - Impacted callers: daemon reconcile pass
src/core/daemon/runtime.js:373; manual detachsrc/core/cli/core_commands.js:3230; status reportsrc/core/daemon/status.js:492 - Impacted tests:
hypaware-core/smoke/flows/client_attach_on_join.js:20;test/core/action-attach.test.js:239;test/core/action-attach.test.js:362;test/core/client-detach-disk.test.js:76;test/core/daemon-reconcile.test.js:362;test/core/integration.test.js:126;test/core/status-client-actions.test.js:307 - Unresolved uncertainty: I did not run the test suite; I also did not see coverage for gateway bind failure before auto-attach or “marker exists + plugin still configured + runtime registration missing.”
Claude review
Claude review
No issues found.
Four parallel review subagents (guidance-compliance, shallow bug scan,
contract & callers, tests & comments) each cleared the round-3 change set;
no candidate finding scored at/above the >=80 reporting bar. Highlights:
- Gate-move (C) is safe.
runClientLifecycleruns the disk-driven
detach branch ahead of thehypaware.ai-gatewaycapability gate; attach
stays gated. Both new tests use a real gateway-less kernel
(fakeKernelWithoutGateway()): the detach test asserts the disk undo ran
(status:ok, changed:false) with the capability absent; the attach test
asserts it still failscap_missing(exit 1). Not vacuous. - TOCTOU (D) correct.
readTextnow stats before reading, so the
captured mtime never post-dates the bytes; the write-time guard errs
toward CONCURRENT_EDIT rather than a silent clobber. - Seam (E) accurate.
clientSeamis resolved once and closed over by
runReconcilePass; never re-derived per pass.reverse()is disk-driven
and never readsctx.clients. The added comment matches the code. - Dead types (A) / hoisted imports (B) clean.
AiGatewayClientDetachContext
andAiGatewayClientRegistration.detach()have zero surviving references;
hoisting removed all inlineimport('...')types with no dangling refs; no
semicolons added; LLP refs all resolve. - Suite green: 1528 pass / 0 fail / 1 skip.
Reports: /Users/phil/workspace/hypaware/.git/worktrees/neutral-rereview3-179-XXXXXX.IhUxyenLoK/dual-review/pr-179
…requires a bound endpoint (#179 round-3 hardening) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0178wfuBrVQHwPbsHAtcE7YS
Contributor
Author
Dual-agent review —
|
| Finding | Reviewer | Severity | In round-4 surface? | Disposition |
|---|---|---|---|---|
Missing attach_probe -> reverse no-op drops marker, orphaning client settings |
Codex | major | NO — action_attach.js desired/reverse + probe contract untouched by cdab0e5 |
Pre-existing risk-class / latent contract gap. Not triggerable by shipped adapters (claude+codex both ship attach_probe). Valid follow-up; not a round-4 regression. |
client_attach_missing diagnostic still warns under attach.on_join:false |
Codex | minor | NO — status.js untouched by cdab0e5 |
Pre-existing diagnostic polish; not a round-4 regression. |
Stale endpoint JSDoc in types.d.ts contradicts round-4 contract |
Claude | minor | ADJACENT — behavior changed in round-4, parallel JSDoc not updated | Real round-4 completeness gap (stale comment on .d.ts contract surface). Doc-only; non-blocking. |
| "inert this pass" overstates the failed-marker-each-pass behavior | Claude | minor | YES — wording in round-4 runtime.js / new test | Real precision/operability nuance; safe + self-heals. Non-blocking. |
Codex review
Fix Validations
#126 silent-gap: joined daemon did not auto-attach enabled clients
- Status: correct
- Evidence: src/core/daemon/runtime.js:65, src/core/daemon/runtime.js:326, src/core/daemon/runtime.js:373, src/core/daemon/runtime.js:384, src/core/config/action_attach.js:89, src/core/config/action_attach.js:151
- Assessment: The daemon now registers attach before backfill, passes
clientDescriptors/clients/endpointinto the reconciler, andperform()calls the live adapter attach with JSON output. Existing handling was only a status warning (src/core/daemon/status.js:453), so it would not have closed the silent capture gap by itself.
Config-drop reverse after adapter unload
- Status: correct
- Evidence: src/core/config/action_reconciler.js:198, src/core/config/action_reconciler.js:210, src/core/config/action_reconciler.js:212, src/core/config/action_attach.js:203, src/core/config/client_detach_disk.js:85
- Assessment: The reverse path is disk-driven and does not require the dropped adapter to stay registered. This is correct for descriptors with a valid
attachProbe; see the first finding for the missing-probe hole.
Findings
2) Contract & Interface Fidelity
- Severity: major
- Confidence: high
- Evidence: src/core/plugin_catalog.js:23, src/core/plugin_catalog.js:97, src/core/config/action_attach.js:89, src/core/config/action_attach.js:96, src/core/config/client_detach_disk.js:85, src/core/config/client_detach_disk.js:87, src/core/config/action_attach.js:222
- Why it matters: A client contribution without
attach_probecan be auto-attached and marked done, but reverse will treat the missing probe as a successful no-op and drop the marker, leaving client settings orphaned. - Suggested fix: Make
attach_proberequired for anycontributes.clientthat can participate in attach-on-join, or filter/faildesired()whendescriptor.attachProbeis absent; also make reverse treat “missing attachProbe for an applied marker” as a failed reverse, notdone.
11) Debuggability & Operability
- Severity: minor
- Confidence: high
- Evidence: src/core/daemon/status.js:453, src/core/daemon/status.js:457, src/core/daemon/status.js:638, src/core/daemon/status.js:641, src/core/daemon/status.js:643
- Why it matters:
attach.on_join:falsecorrectly renders the attach action asn/a, but the older client-attach diagnostic still warns that the client is missing and tells the operator to runhyp attach. - Suggested fix: Gate
client_attach_missingon the same attach policy used by the declared attach report, and suppress the warning when the enabled plugin hasattach.on_join:false.
No Finding
-
- Behavioral Correctness
-
- Change Impact / Blast Radius
-
- Concurrency, Ordering & State Safety
-
- Error Handling & Resilience
-
- Security Surface
-
- Resource Lifecycle & Cleanup
-
- Release Safety
-
- Test Evidence Quality
-
- Architectural Consistency
Evidence Bundle
- Changed hot paths:
action_attach,action_reconciler,client_detach_disk, daemon runtime reconcile wiring, manualhyp attach/detach, status client-action reporting, ai-gatewayregisterClient. - Impacted callers: daemon reconcile pass
src/core/daemon/runtime.js:373; manual detachsrc/core/cli/core_commands.js:3230; manual attachsrc/core/cli/core_commands.js:3305; status reportsrc/core/daemon/status.js:499; gateway registrationhypaware-core/plugins-workspace/ai-gateway/src/api.js:53. - Impacted tests:
test/core/action-attach.test.js:146;test/core/action-attach.test.js:387;test/core/client-detach-disk.test.js:435;test/core/daemon-reconcile.test.js:362;test/core/status-client-actions.test.js:307;test/core/integration.test.js:169;hypaware-core/smoke/flows/client_attach_on_join.js:48. - Unresolved uncertainty: I did not run the test suite; review is based on the supplied diff plus targeted caller/contract tracing.
Claude review
Claude review
Four parallel review lanes (guidance/compliance, shallow-bug, contract-&-callers,
tests-&-comments) ran on head cdab0e5. The two headline round-4 items are correct
and genuinely tested; npm test is green (1532 pass / 0 fail / 1 skip) and the
client_attach_on_join smoke passes. ITEM-1's new test was mutation-verified
(disabling the fs.rm cleanup fails exactly that test). Two minor doc/contract
items survived the >=80 filter — neither blocks merge.
Stale endpoint JSDoc on the .d.ts contract surface contradicts the round-4 behavior
- Severity: minor
- Confidence: 90
- Evidence: src/core/config/types.d.ts:396-401 (ActionContext.endpoint) and :454-458 (ReconcileInput.endpoint)
- Why it matters: Both blocks (added earlier in this PR) still say endpoint is "resolved from
gateway.localEndpoint()with the configured-listenfallback the CLI uses. Set wheneverclientsis." Round-4 made both clauses false (daemon seam omits the fallback; endpoint is now deliberately undefined whileclientsis set —daemon-attach-seam.test.js:85-88,action_attach.js:133-136). A maintainer trusting "set whenever clients is" could delete theperform()endpoint guard as dead code, reintroducing the dead-port-URL failure round-4 closed. - Suggested fix: Update both JSDoc blocks to "resolved from
localEndpoint()only — no configured-listenfallback (that is the manual CLI path); may be undefined even whenclientsis set (gateway never bound), in which case the attach handler records a retryable failure."
"stays inert this pass" wording overstates the unbound-gateway path
- Severity: minor
- Confidence: 82
- Evidence: src/core/daemon/runtime.js:822-824 and test/core/daemon-attach-seam.test.js:84
- Why it matters:
desired()does not gate onendpoint, so when the gateway is enabled-but-never-bound (a non-fatal source bind failure;runtime.js:947-960) the client is still named andperform()returns{status:'failed', reason:'attach action missing gateway endpoint'}(action_attach.js:133-136), which the reconciler records as afailedmarker + aclient_action.failedERROR each pass until the gateway binds (action-attach.test.js:340-348pins this). It is safe (no crash, no bad URL) and self-heals, but it is "records a retryable failure," not literally "inert" — a recurring ERROR signal while the gateway is merely waiting to bind, which may feed the daemon's aggregate-state health derivation. - Suggested fix: Either make it genuinely inert (have
desired()omit the client whenclients && !ctx.endpoint, or return a non-failed "deferred" outcome) or soften the "inert this pass" wording inruntime.jsand the test name to "records a retryable failure until the gateway is proven-bound."
Reports: /Users/phil/workspace/hypaware/.git/worktrees/neutral-rereview4-179-XXXXXX.XXvzvfDRhl/dual-review/pr-179
philcunliffe
added
the
neutral:stuck
Contributor
Author
🤖 neutral: converged after 4 review rounds — human merge gateDriven through 4 fix↔review rounds (heads The residual mechanical
Two non-blocking doc-precision minors from round 4 (worth a quick cleanup before or after merge):
Process note: Recommendation: this is as clean as the change set will get — merge-quality, Claude-clean, residual is pre-existing/latent. Merge is a human act (neutral never merges); held for your call. Drop |
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.
Implements client attach on join — when a joined machine confirms a central config that enables a client adapter (claude/codex), the daemon performs that client's attach machine-effect and reverses it on leave. Closes the silent-gap failure (#126): daemon healthy + gateway bound but nothing recorded until a manual
hyp attach.First change set built via design-first intake (LLP 0016): the design (LLP 0045) was authored by hand and merged to master at
Status: Accepted; neutral planned (LLP 0046) and implemented it, flipping the design toActiveas the shipped marker.What landed (10 tasks, T1–T10):
_hypawaremarker + codex marked block (T1)attach_policy.js, tri-stateattach.on_join(T2)clientDescriptors/clients/endpoint(T3)client_detach_disk.js; adapters'detach()retired (T4/T5)action_attach.js(desired/perform/reverse) (T6)runtime.js: register[attachHandler, backfillHandler](T7) +client_attach_on_joinsmokeAccepted → Active(T10)Open question carried from LLP 0045 for review: undo-record completeness (per-format replay vs declarative manifest) — settled across T1/T4/T5; check for orphaned hook-entry risk on fleet-drop.
🤖 Generated with Claude Code
Change-Set: client-attach