detach: report every externally-overridden managed key, not just the last (issue #440 finding 2)

[philcunliffe]

Follow-up on the findings issue #438's triage deferred. One of the three findings is fixed here; the other two are deliberately deferred, with reasons below.

Fixed: finding 2 - warning overwritten when several managed keys were externally overridden

DetachFromDiskResult.warning was a single string | undefined reassigned inside the per-key loop in detachJsonMarker, so when the undo left two or more managed values in place because the user had re-pointed them, only the last key's notice survived. The on-disk protection was never affected (the user values were always kept); the operator was simply never told which keys stayed behind after a detach that otherwise reports success.

The identical pattern exists in detachJsonPathMarker's set-entry loop, so both now accumulate one notice per key and fold them through a shared joinWarnings helper.

The contract shape is unchanged.warning stays a single human-readable string (it has two consumers, not one: action_attach.js:346 logs it as a detail attribute on client_action.attach_reverse_warning, and clients.js:588 echoes it into the hyp detach --json payload); it now carries every key instead of one. That is deliberately not the array change the issue floated as a reason to defer - no caller has to change, and there is no published-type break.

Regression tests (fail before, pass after)

• test/core/client-detach-disk.test.js: claude undo names EVERY externally-overridden managed key in the warning, not just the last - attach, override both ANTHROPIC_BASE_URL and ENABLE_TOOL_SEARCH, detach; asserts both key names appear in warning and that both user values survive on disk.
• test/core/client-detach-json-path.test.js: json_path undo names EVERY externally-overridden set entry in the warning - a hand-written record with two set entries, both overridden; asserts both paths appear.
• Plus claude undo reports a single overridden key without the join separator, pinning the one-key string to its exact existing wording so the accumulation did not change the common case.

Verified: the two multi-key tests fail on master before the source change and pass after. npm test is green apart from the 8 known pre-existing test/core/leave-command.test.js failures, which are unrelated. npm run typecheck is clean.

LLP 0045 Part 3 gains a Never clobber a user edit: report every override, not just the last section, landed in the same commit, which the two new @refs point at.

Deferred: finding 1 - ownership gate silently adopts a non-string user value

Deferred on purpose, and it should be redone once #438 merges.

The correct fix is to gate on presence (key in env) rather than typeof env[key] === 'string'. On master that predicate lives on a single line inside attach():

constmanageToolSearch=weOwnToolSearch||typeofenv.ENABLE_TOOL_SEARCH!=='string'

PR #438deletes that exact line and replaces the whole block with the MANAGED_ENV_ADDITIONS table plus manageEnvAdditions(). Editing a line the held PR removes is a guaranteed merge conflict, and the fix would have to be rewritten against the new helper anyway - the very outcome that made triage defer it. #438's diff also rewrites 77 lines of test/plugins/claude-settings-attach.test.js, so the regression test would collide too.

Fixing it after #438 lands is strictly better: one edit to manageEnvAdditions's single if (!weOwnIt && typeof env[key] === 'string') continue guard covers both managed keys at once, rather than fixing ENABLE_TOOL_SEARCH here and _CLAUDE_CODE_ASSUME_FIRST_PARTY_BASE_URL again later. The data-loss risk is unchanged by waiting: it is pre-existing on master and #438 does not make an individual key any more exposed.

Deferred: finding 3 - verified-version discrepancy (2.1.220 vs 2.1.215)

Deferred because there is nothing to reconcile on master. Both version stamps were introduced by PR #438 and exist only on fix/issue-437:

• hypaware-core/plugins-workspace/claude/src/settings.js:61 (last verified 2.1.220) - added by Attach declares the gateway first-party so Claude keeps its real context window #438
• llp/0045-client-attach.design.md:291 (verified against 2.1.215) - added by Attach declares the gateway first-party so Claude keeps its real context window #438 round 2

git grep on origin/master finds neither string in either file. A "fix" here would have to invent the text it is correcting. It is a one-word edit to make after #438 merges (the triage worktree independently confirmed the installed bundle is 2.1.215, so 2.1.220 is the stamp to correct).

Finding 4 in the issue was already corrected in #438's body during triage and needs nothing here.

Interaction with PR #438

This branch is based on origin/master and touches no file #438 modifies except test/core/client-detach-disk.test.js, where #438 inserts around line 136 and this PR appends at the end of the file. src/core/config/client_detach_disk.js, src/core/config/types.d.ts and test/core/client-detach-json-path.test.js are untouched by #438. The LLP 0045 edit inserts a new section before #### ENABLE_TOOL_SEARCH, while #438 edits text inside and after that section. #438's review is not affected and needs no new commits.

Fixes#440

[philcunliffe]

Neutral review round - PR #441 @ 2ea163d

Verdict: approve the approach, with two fixes pushed. The bug is real, the diagnosis is right, both loop fixes are correct, and both deferrals hold up under checking. Two defects in the new code (one behavioural, one a load-bearing factual claim) are fixed in a891403 on this branch. One pre-existing sibling defect is reported unfixed.

Verification performed

• Reverted onlysrc/core/config/client_detach_disk.js to 3955700 and re-ran both test files: exactly the two named tests fail (claude undo names EVERY...[codex] Record AI gateway message context #20, json_path undo names EVERY...Feature ai-gateway-v2: Phase 4 — API tests + smokes refresh + ship gate (hy-1l4nl) #31), 37/39 pass. Restored: 39/39. The author's fail-before/pass-after claim is confirmed exactly.
• claude undo reports a single overridden key without the join separator passes on both sides of the revert, and the pinned string ANTHROPIC_BASE_URL was overridden externally; leaving in place is byte-for-byte the pre-PR message. The contract guard is genuine.
• npm test: 2795 pass / 8 fail, all 8 the known test/core/leave-command.test.js baseline. npm run typecheck clean.
• No em dash introduced anywhere in the diff; the one on client_detach_disk.js:183 was correctly converted to -.
• @ref LLP 0045#never-clobber-a-user-edit-report-every-override-not-just-the-last resolves to the real heading at llp/0045-client-attach.design.md:241.

Findings

1. MEDIUM (fixed) - the ; join separator is ambiguous by construction

src/core/config/client_detach_disk.js:214

Every notice already contains a ; : `${key} was overridden externally; leaving in place` (:184, :470). Joining those on ; produces four ; -delimited clauses for two keys, and nothing distinguishes a notice boundary from the punctuation inside a notice. Captured from a live run of the new multi-key test:

ANTHROPIC_BASE_URL was overridden externally; leaving in place; ENABLE_TOOL_SEARCH was overridden externally; leaving in place

This defeats the readability goal the LLP section states, at exactly the boundary that section makes load-bearing. Fixed: joinWarnings now joins on |; | cannot occur in an env key name or a dotted JSON path. The single-key string is untouched, so the contract-preservation test still passes unmodified.

2. MEDIUM (fixed in docs) - warning has two consumers, not one

The PR body, the commit message, LLP 0045 (its one consumer logs it as a detail attribute) and the joinWarnings JSDoc (callers log it as detail) all assert a single consumer. There are two:

• src/core/config/action_attach.js:346 - span detail attribute (the one named).
• src/core/commands/clients.js:588 - payload.warning = result.warning in the hyp detach --json output, plus :596 on the human path.

The second is a machine-readable CLI surface, which is precisely where "should this be an array?" bites hardest, so getting it wrong weakens the stated rationale for keeping a string.

The conclusion still holds - I checked every reference to .warning in src/, hypaware-core/, bin/, test/ and nothing splits, slices, regex-parses or otherwise destructures it; both consumers display it verbatim. So a single string is the right call and the array the issue floated would have been a gratuitous published-type break. But the reasoning has to rest on "no consumer parses it", not on "there is one consumer". Fixed in LLP 0045 and the JSDoc, which now name both consumers, state the display-only property, and record that a future need for the keys individually should add warnings: string[] alongside rather than re-type warning. (Per instruction the PR body was not edited - it still carries the wrong claim.)

3. LOW (fixed) - no test pinned the separator

Mutation-testing joinWarnings against the suite at 2ea163d: join(', ')survived - both multi-key tests only assert.match per key. Since ; vs | was the actual defect above, the character needs a test. (warnings[0], warnings[warnings.length - 1] and the zero-warnings-to-empty-string mutant were all correctly killed, so the rest of the helper is well covered.) Fixed: both multi-key tests now assert the whole joined string. All five mutants are now killed.

4. LOW (NOT fixed - reported for follow-up) - restoredValue has the identical reassign-in-loop defect, in the same loop

src/core/config/client_detach_disk.js:463 (post-fix line numbering), inside the very set loop this PR just fixed:

if(typeofentry.prev==='string')restoredValue=entry.prev

Two set entries that are both still ours and both have a prev are both restored on disk correctly, but restoredValue reports only the last - the same "only the last survives" shape as the fixed warning bug, one line away from it. Note the inconsistency with removed two lines below, which carries a deliberate first-wins guard (if (removed === undefined && ...)) that restoredValue does not.

I did not fix this, deliberately:

• Unlike warning, restoredValue is a value, not a message. Folding it with a separator would be wrong, and choosing first-wins vs last-wins vs a new plural field is a design decision, not a bug fix.
• It is unreachable today: OpenClaw, the only shipping json_path client, records exactly one set entry (hypaware-core/plugins-workspace/openclaw/src/settings.js:175). It is latent-only, exactly like the json_path half of this PR's own fix.
• Widening this deliberately narrow PR into a semantic change to a published --json field with no design decision behind it is the scope creep the author correctly avoided elsewhere.

Recommend a follow-up issue. It should not block this PR.

Checked and found correct (no action)

• Every warning path is accumulated. Both loops push on every branch that produces a notice; no branch was missed.
• No missed sibling. The file's other two warning producers are not loops and cannot clobber: detachLegacyJsonMarker:327 handles the single key ANTHROPIC_BASE_URL, and detachTomlManagedBlock:605 the single TOML root key. codex/src/toml-config.js:116 is likewise single-key. Correctly left alone.
• detachJsonMarker's removed/restoredValue are safe even though they sit in the loop, because both are gated on key === 'ANTHROPIC_BASE_URL', which can match at most once. Finding 4 applies only to the json_path branch.
• joinWarnings zero-case is right: undefined, so if (warning !== undefined) keeps the field absent rather than setting ''. A mutant returning '' is caught by the suite.
• Duplicates cannot arise: Object.entries keys are unique, and a duplicated set path would be a malformed record.

On the two deferrals - both sound, verified not assumed

Finding 1 (ownership gate): correctly deferred.git grep origin/master confirms the predicate is a single line, hypaware-core/plugins-workspace/claude/src/settings.js:120. gh pr diff 438 confirms #438deletes that exact line (- const manageToolSearch = weOwnToolSearch || typeof env.ENABLE_TOOL_SEARCH !== 'string') and replaces it with MANAGED_ENV_ADDITIONS + manageEnvAdditions(), whose guard is if (!weOwnIt && typeof env[key] === 'string') continue. Editing it here is a guaranteed conflict, and the post-#438 fix is one edit covering both managed keys instead of two edits covering one each. The author's reasoning is accurate on every point.

Finding 3 (version stamps): correctly deferred.git grep '2.1.220\|2.1.215' origin/master returns nothing. Both stamps exist only on fix/issue-437. There is literally no text on master to correct.

The no-conflict claim also checks out.#438 touches claude/src/settings.js, llp/0045, test/core/client-detach-disk.test.js, test/plugins/claude-settings-attach.test.js. The only overlaps are llp/0045 (#438 hunks at original lines 224, 252, 264; this PR inserts at original line 241 with context 238-243 - disjoint) and test/core/client-detach-disk.test.js (#438 at original line 136; this PR appends past 536 - disjoint). Both merge cleanly. Semantically the PRs reinforce each other: #438 adds a second managed env addition, which makes this multi-key warning more reachable, not less.

Pushed

a891403e965bb56034b5985306fdfc3fa8993a07 on fix/issue-440. Verified in the committed tree vs 2ea163d: warnings.join('; ') -> warnings.join(' | ') at client_detach_disk.js:220; the | doc in types.d.ts:652; the whole-string assertions in both test files; its one consumer gone from LLP 0045 (1 occurrence at 2ea163d, 0 at a891403). No em dash in the added lines. npm test and npm run typecheck re-run at the new head: same 8-failure leave-command baseline, typecheck clean.

Unresolved

• Finding 4 (restoredValue last-wins in the json_pathset loop) - latent, needs a follow-up issue.
• The PR body still carries the "its one consumer" claim (finding 2); the body was not edited per review protocol. Worth a one-line correction before this leaves draft.

[philcunliffe]

Neutral review — round 2 of 2 (final)

Reviewed a891403e965bb56034b5985306fdfc3fa8993a07 in a detached worktree. Round 1 (2ea163d, comment) found three items and fixed all three in a891403e. This round re-tested those fixes adversarially, independently verified the reachability claim round 1 used to justify deferring finding 4, and found one new LOW, now fixed.

Verdict: the core change is correct and I would approve the behavior. One documentation contract the PR itself introduced was demonstrably false and is now corrected. One item remains deliberately deferred, and the PR body carries a stale claim that only triage can fix.

---

Round 1's fixes — re-tested, all hold

Separator is genuinely pinned (round 1 LOW, confirmed fixed). Mutated warnings.join(' | ') → join(', ') at src/core/config/client_detach_disk.js:220 and ran the two suites. Exactly the two intended tests died, by name:

not ok 20 - claude undo names EVERY externally-overridden managed key in the warning, not just the last
not ok 31 - json_path undo names EVERY externally-overridden set entry in the warning
# pass 37 # fail 2

The join(', ') mutant that survived round 1 is dead. Good.

Zero-warning case yields an absent field.joinWarnings([]) returns undefined (client_detach_disk.js:220) and every call site is guarded if (warning !== undefined) (:198, :512). Pinned by test/core/client-detach-json-path.test.js:218 (assert.equal('warning' in result, false)).

Single-warning string is byte-identical to pre-PR. The template literals are unchanged from master — only the assignment became a push (:184, :476) — and test/core/client-detach-disk.test.js:593 pins the exact one-key string.

@ref anchors resolve. Both @ref LLP 0045#never-clobber-a-user-edit-report-every-override-not-just-the-last (:160, :455) slug-match the new heading at llp/0045-client-attach.design.md:241. No em dash introduced anywhere in the diff.

Both consumers confirmed (round 1 MEDIUM-docs). Re-swept: src/core/config/action_attach.js:346 (span detail) and src/core/commands/clients.js:588 + :596 (--json payload and stdout). Neither splits or parses. Exactly two, as round 1 said — not the one the PR body claims.

---

NEW — LOW: the | separator is not safe field-wide, and the PR documented that it is · FIXED

Round 1 fixed the ambiguity for the two branches that join. But the PR then wrote a field-level contract that overreaches:

• src/core/config/types.d.ts:654 — "the field is not safely splittable on anything but the | boundary"
• llp/0045-client-attach.design.md — "| cannot occur in an env key name or a dotted JSON path, so the boundary stays unambiguous."

Both statements invite a consumer to split on |. The warning field is shared with a third producer that does not join: detachTomlManagedBlock at src/core/config/client_detach_disk.js:611

warning=`${TOML_ROOT_RESTORE_KEY} was changed externally; leaving ${current} in place`

current is the user's live model_provider, read verbatim from their ~/.codex/config.toml (readRootModelProvider). It is an arbitrary TOML string. This needs no hand-edited marker and no adversary — just an ordinary config value with a pipe in it. Reproduced at runtime:

warning: 'model_provider was changed externally; leaving acme | prod in place'
split(' | '): [ 'model_provider was changed externally; leaving acme', 'prod in place' ]

One notice becomes two bogus notices. warning is currently reachable by hyp detach --client codex --json, so this string is already machine-visible even though nothing in-tree parses it.

The joined branches themselves are clean, and I checked concretely: managed.env keys come from two hardcoded literals in hypaware-core/plugins-workspace/claude/src/settings.js:138-139 (ANTHROPIC_BASE_URL, ENABLE_TOOL_SEARCH), and the only in-tree set path is the PRIMARY_PATH constant. So | is a fine readability choice — it is the contract framing that was wrong.

Fixed in 9613eb3. Corrected types.d.ts and LLP 0045 to state plainly that the field is display-only and that no separator is safe field-wide, with the toml case named as the reason; retitled | as the readability choice it is; and pointed callers who need the keys at a future warnings: string[]. Also added the codex regression test that branch never had (it had zero coverage of the "was changed externally" path), pinning the exact warning text and asserting that one notice splits into two.

---

Finding 4 (restoredValue last-wins) — independently verified, and I agree it stays deferred

src/core/config/client_detach_disk.js:468:

if(typeofentry.prev==='string')restoredValue=entry.prev

Reassigned inside the set loop with no guard, three lines above removed's explicit first-wins guard at :471 (if (removed === undefined && …)). The inconsistency is real.

I re-derived the reachability claim from scratch rather than taking round 1's word, and checked everyattach_probe declaration in the tree, not just OpenClaw. There are exactly four:

plugin	format	reaches the set loop?
claude	json	no — different function
codex	toml	no — different function
openclaw	json_path + marker_record	yes
claude-desktop	json_path, no marker_record	no — see below

• OpenClaw is the only real producer, and it emits exactly one entry, always: const set = [{ path: PRIMARY_PATH, value: managedPrimary, prev: primary }] (hypaware-core/plugins-workspace/openclaw/src/settings.js:175). I also checked the re-attach path round 1 did not mention — reattach() passes the original managed block through verbatim to attachRecord(...) and never grows set, so no number of re-attaches can produce a second entry.
• claude-desktop declares format: json_path (hypaware-core/plugins-workspace/claude-desktop/hypaware.plugin.json:22) but no marker_record, so recordPath === undefined → managed === undefined → detachJsonPathMarker throws MALFORMED_MARKER at :438, before the loop.

Verdict: round 1's claim holds — genuinely unreachable via any shipped producer. I'd sharpen the framing for the follow-up, though: it is unreached, not unreachable by construction. detachJsonPathMarker is documented as a format-generic record replayer (@ref LLP 0109#probe-and-detach-core-owned: "the record replay is format-generic, core knows json_path semantics, never 'OpenClaw'"), PluginAttachProbeManifest is in the published root kernel contract (hypaware-plugin-kernel-types.d.ts:163-181), and the repo has a real plugin-install path (src/core/plugin_install/, LLP 0007). Any installed plugin declaring json_path + marker_record with two set entries hits the defect on day one. It is one third-party plugin away, not impossible.

Even so, I agree it should not be fixed in this PR. Unlike warning, there is no obviously-correct answer: first-wins (matching removed), last-wins (status quo), or plural all encode different operator semantics, and silently picking one here would mint a contract nobody designed, in a PR scoped to messages. It wants its own issue.

---

Unresolved — for triage

1. The PR body still carries the false "its one consumer" claim. Body: "its one consumer, action_attach.js, logs it as a detail attribute". There are two: src/core/config/action_attach.js:346 and src/core/commands/clients.js:588, and the second is the more consequential one — it echoes warning into the machine-readable hyp detach --json payload, which is precisely the surface where the parseability question above bites. Round 1 flagged this; neither round is permitted to edit the PR body. Triage should correct it before merge, as it did for Attach declares the gateway first-party so Claude keeps its real context window #438. The in-repo docs are already right (LLP 0045 and types.d.ts both name both consumers) — the body is the only stale copy.
2. Finding 4 needs a follow-up issue: restoredValue last-wins at client_detach_disk.js:468, inconsistent with removed's first-wins guard at :471. Not reachable today; blocked on a deliberate first/last/plural decision.
3. Not this PR, noticed in passing:hyp detach --client claude-desktop looks like it always throws MALFORMED_MARKER — the manifest declares json_path with no marker_record (claude-desktop/hypaware.plugin.json:22), and detachJsonPathMarker:437 refuses any record-less marker. Pre-existing on master and unrelated to this change; worth its own look.

---

Checks

• npm test at 9613eb3: 2796 pass / 8 fail — the 8 known pre-existing test/core/leave-command.test.js failures, identical to the baseline at a891403e. No new failures.
• npm run typecheck: clean.
• Detach suites: 40 pass / 0 fail.
• Pushed 9613eb38954cf38f3b517fb8927340f0c0fc966f to fix/issue-440.

Round cap (2) reached; anything above under "Unresolved" goes to triage.

[philcunliffe]

Triage - PR #441 @ 9613eb38954cf38f3b517fb8927340f0c0fc966f

Review hit its 2-round cap. Independently re-verified the core change and every residual item against the code at head (not just the reviewers' prose) in a detached worktree: npm test at 9613eb3 is 2796 pass / 8 fail, all 8 the known pre-existing test/core/leave-command.test.js baseline; no new failures. Both loop fixes, the joinWarnings separator, and the "display-only, no separator is safe field-wide" contract correction all check out exactly as the round-2 comment describes.

Classification: no true blocker. All residuals are non-blocking follow-up material. This PR strictly adds information the user previously lost (every overridden key's notice, instead of just the last) and does not make anything on master worse.

• A - PR body's stale "its one consumer" claim. Confirmed there are two consumers: src/core/config/action_attach.js:346 (span detail) and src/core/commands/clients.js:588/:596 (the hyp detach --json payload and human output). The in-repo docs (types.d.ts, LLP 0045) were already corrected during review; the body was the only stale copy. Corrected directly in the PR body (this is the class of fix triage is permitted to make; review rounds are not).
• B - restoredValue last-wins in detachJsonPathMarker's set loop (client_detach_disk.js ~line 467-470, no first-wins guard unlike removed). Re-derived the reachability claim from scratch: all four attach_probe declarations checked (claude/codex never reach this function; claude-desktop throws MALFORMED_MARKER first, see below; OpenClaw always emits exactly one set entry and reattach() never grows it). Genuinely unreached today, though not unreachable by construction given the published kernel contract. It is a value field, not a message - folding it would be wrong, and first/last/plural is a design decision out of scope for a PR scoped to message accumulation. Filed as follow-up: Follow-up: deferred review findings from PR #441 #443.
• C - hyp detach --client claude-desktop always throws MALFORMED_MARKER. Confirmed: the manifest declares json_path with no marker_record, so recordPath is always undefined and the undo record can never be read - this is unconditional, not data-dependent. Confirmed pre-existing and untouched by this PR (git diff between origin/master and this head touches no line of the manifest or the MALFORMED_MARKER gate). Real bug, but independent of detach: report every externally-overridden managed key, not just the last (issue #440 finding 2) #441 - filed separately as hyp detach --client claude-desktop always fails with MALFORMED_MARKER #444, not bundled with the Follow-up: deferred review findings from PR #441 #443 follow-up.

Not moving this out of draft or merging - that stays a human/maintainer call. No blocking action needed from this PR's author.