claude attach: a non-string user env value is the user's, and one verified-version stamp

[philcunliffe]

Both findings deferred from PR #438 and dropped when #440 auto-closed. #438 has merged (a1100f0), so both are actionable on master.

Finding 1 - a non-string user value is clobbered and recorded as managed

Changed:hypaware-core/plugins-workspace/claude/src/settings.js (manageEnvAdditions), plus its JSDoc and the LLP 0045 ownership rule.

The gate tested the type of the existing value:

if(!weOwnIt&&typeofenv[key]==='string')continue// beforeif(!weOwnIt&&keyinenv)continue// after

A hand-written JSON boolean or number at a managed key fell straight through it. Reproduced end to end on master before touching anything:

BEFORE ATTACH : {"ENABLE_TOOL_SEARCH":true,"_CLAUDE_CODE_ASSUME_FIRST_PARTY_BASE_URL":0}
AFTER ATTACH : {"ENABLE_TOOL_SEARCH":"true","_CLAUDE_CODE_ASSUME_FIRST_PARTY_BASE_URL":"1", ...}
MARKER managed.env: {"ANTHROPIC_BASE_URL":"...","ENABLE_TOOL_SEARCH":"true","_CLAUDE_CODE_ASSUME_FIRST_PARTY_BASE_URL":"1"}
AFTER DETACH : null <- the whole env block is gone

Attach coerced the values and claimed both keys in the _hypaware marker's managed.env; the core undo then did exactly what that record authorized and deleted settings the user owned, pruning the emptied env block with them. The 0 case is the sharpest: that is the user deliberately turning the flag off, and attach reversed their intent before deleting it.

Coercion was never intended - the helper's JSDoc and LLP 0045 both already promised a user-set value is left untouched and stays out of the undo record. So the guard moves to match the doc, not the other way round. Ownership now turns on the key being present, whatever its JSON type. After the fix the same script reports RESULT: user values intact, and managed.env contains only ANTHROPIC_BASE_URL.

Regression tests (fail on master, pass here):

• test/core/client-detach-disk.test.js - claude attach + undo leave a non-string user-owned env value byte-for-byte intact. The round trip is the assertion that matters, since the bug needed attach and detach together to destroy data; it asserts the final file equals the user's original text byte-for-byte.
• test/plugins/claude-settings-attach.test.js - attach-level coverage for boolean / number / null, each asserting the value is untouched and absent from managed.env.

Verified by reverting only settings.js to origin/master with the new tests in place: exactly those 4 fail (not ok 4, 32, 33, 34); with the fix restored, 38/38 pass.

Finding 2 (issue calls it finding 3) - contradictory verified-version stamps

Changed: the stamp in hypaware-core/plugins-workspace/claude/src/settings.js and the one at llp/0045-client-attach.design.md:373. All three stamps now read 2.1.215.

Both values entered in the same commit (a1100f0, #438), so this was never an introduced-at vs last-verified distinction - just an unverified number that review round 2 corrected in one place of three. 2.1.215 is the release the isFirstPartyBaseUrl predicate was actually read off the shipped bundle, and it is what is installed in this environment: claude --version reports 2.1.215 (Claude Code) and the global npm tree shows @anthropic-ai/claude-code@2.1.215. Nobody has ever confirmed 2.1.220.

The stamp is load-bearing rather than cosmetic: the LLP sends a future reader to re-verify this undocumented, underscore-prefixed flag against a later release, and an ambiguous baseline defeats that mitigation. The LLP now also says explicitly that this one version is the baseline every stamp in the tree carries.

Documentation-only, no runtime effect, and it is not the sole content of this PR.

Checks

• npm test: 2848 pass / 8 fail - the 8 are the known pre-existing test/core/leave-command.test.js failures, unchanged in count and name from the stated baseline.
• npm run typecheck: clean (exit 0).
• The new @ref on manageEnvAdditions resolves; all three LLP 0045 anchors used in the file were checked against the document's headings.
• No hyp subcommand was run and no host state was touched.

Corrected during triage: this description stops at two findings and is now understated - the review fix-loop found two more instances of the same bug class (a type check standing in for a presence/ownership test) after this text was written, and the numbers above are the round-1 snapshot, not the head.

• client_detach_disk.js had the identical type gate on its never-clobber override notice, in both the record-driven undo (:182) and the legacy pre-record path (:338); round 1 fixed the former and declined the latter, round 2 judged that call wrong and fixed it too. Round 2 also swapped key in envObj for Object.hasOwn there, since that loop's key names come off disk rather than being in-tree literals.
• High:settings.js:123 gated the ANTHROPIC_BASE_URL backup itself on type. A non-string live value (a hand-written null or a stray number) read as nothing to back up, so attach recorded the key as managed with no prev_base_url to restore, and detach then deleted the user's setting outright - {"changed":true}, no warning, nothing to recover. Fixed across all three places the value passes through (attach's backup, the re-attach carry-forward, and the undo's read of prev_base_url); verified independently during triage by reverting just these two files against the new tests, which reproduces the loss (14 tests fail, including the byte-for-byte round trips for 8080/false/null and the two-attach carry-forward case) and confirming the pre-existing re-attach behaviour (recorded original, not the gateway URL, on re-attach) still holds with the fix restored.
• At head 3aa623b the suite is 2862 pass / 8 fail (same baseline, +14 tests over the number above), npm run typecheck remains clean, and no hyp subcommand was run at any point.

The two adjacent defects round 2 found in the same sweep (a different bug class: a schema check where a refusal belongs, in ensureObject for a non-object env and a non-array hooks.<event>) are filed separately as #454 rather than fixed here, since choosing refuse-vs-repair is a design call this PR should not make in passing.

Fixes#448

[philcunliffe]

Neutral review round - PR #449 @ de1873d

Verdict: approve the change as written. Both findings hold up under attack, every claim in the description reproduced exactly, and the new predicate does not introduce the re-attach regression its shape could have. One actionable finding, Low: the mirror-image type test in the undo, now fixed and pushed.

---

1. Attacking key in env (settings.js:204)

Attacked from five directions. The predicate survives all of them.

Attack	Result
Explicit "ENABLE_TOOL_SEARCH": null	'k' in env is true, so the user's null is left alone and stays out of managed.env. Covered by not ok 34 below.
Inherited prototype property	Cannot land.JSON.parse defines "__proto__" as an own data property and never mutates the prototype, so a parsed env is always plain Object.prototype. None of the three managed keys (ENABLE_TOOL_SEARCH, _CLAUDE_CODE_ASSUME_FIRST_PARTY_BASE_URL, ANTHROPIC_BASE_URL) exist on Object.prototype - verified [false, false, false]. Live probe with {"env":{"__proto__":{"ENABLE_TOOL_SEARCH":"sneaky"}}} attaches normally and claims the key, which is correct. Object.hasOwn is therefore exactly equivalent here, not safer: I ran it as a mutant and got 38/38, identical behaviour. in is fine.
Key present but undefined	Not expressible in JSON, so unreachable through readSettings. Mutant env[key] !== undefined also passes 38/38, confirming the two are indistinguishable on any real input.
RE-ATTACH: does the new predicate skip our own key and leave a stale value?	No.!weOwnIt && short-circuits first, so when a prior marker recorded the key we fall through to env[key] = value regardless of what is live. Probed directly: after attach, I drifted the live value to 'false', false, 0 and null in turn and re-attached. All four reclaim to live="true", managed="true". No stale value in any case.
Re-attach claiming a user key it skipped on attach #1	No. The key was never recorded, so weOwnIt stays false forever. Probed: user writes true, attach x2, then detach - file is byte-for-byte the original.

The re-attach path is also genuinely pinned, not just correct by luck. Mutating the guard to if (key in env) continue (dropping the ownership short-circuit) fails three named tests:

not ok 29 - re-attach keeps managing an ENABLE_TOOL_SEARCH it owns
not ok 35 - re-attach keeps managing a _CLAUDE_CODE_ASSUME_FIRST_PARTY_BASE_URL it owns
not ok 38 - the marker undo record is stable across re-attach (modulo attached_at)

2. Undo round trip - mixed case

Probed the exact mixed case: one key attach owns, one the user set as a non-string.

before attach : {"env":{"_CLAUDE_CODE_ASSUME_FIRST_PARTY_BASE_URL":0}}
after attach : {"_CLAUDE_CODE_ASSUME_FIRST_PARTY_BASE_URL":0,"ENABLE_TOOL_SEARCH":"true","ANTHROPIC_BASE_URL":"http://127.0.0.1:4123"}
managed.env : {"ANTHROPIC_BASE_URL":"...","ENABLE_TOOL_SEARCH":"true"} <- user key absent
after detach : byte-for-byte identical to the original, warning: undefined

Detach removes exactly what attach added and nothing else: detachJsonMarker only ever iterates Object.entries(managedEnv), so keeping the user's key out of the marker is the whole protection - and the test asserts the absence, which is the load-bearing half.

3. Test teeth - confirmed

Reverting onlysettings.js to origin/master, new tests in place:

not ok 4 - claude attach + undo leave a non-string user-owned env value byte-for-byte intact
not ok 32 - attach leaves a user-owned boolean env value untouched and unmanaged
not ok 33 - attach leaves a user-owned number env value untouched and unmanaged
not ok 34 - attach leaves a user-owned null env value untouched and unmanaged
# pass 34 / # fail 4

Exactly not ok 4, 32, 33, 34 as claimed; with the fix restored, 38/38. Mutants: Object.hasOwn(env, key) 38/38 (equivalent), env[key] !== undefined 38/38 (equivalent), key in env without the ownership guard fails the 3 re-attach tests above. Full suite at de1873d: 2848 pass / 8 fail, the 8 being the known test/core/leave-command.test.js baseline, unchanged in name and count. npm run typecheck clean.

4. Version stamps - confirmed

git grep -nE '2\.1\.[0-9]{2,3}' finds exactly three stamps, all now 2.1.215:

• hypaware-core/plugins-workspace/claude/src/settings.js:61
• llp/0045-client-attach.design.md:337
• llp/0045-client-attach.design.md:380

No fourth stamp anywhere. The only other 2.1.x in the tree is claude-cli/2.1.118 inside an example log string in hypaware-core/plugins-workspace/claude/agents/hypaware-analyst.md:58 - illustrative sample data, not a verification stamp, correctly left alone. Evidence independently reproduced in this environment: claude --version -> 2.1.215 (Claude Code), and npm ls -g -> @anthropic-ai/claude-code@2.1.215. The added LLP line ("that one version is the baseline every stamp in the tree carries, code comment included") is accurate as written, and is now the thing that keeps it accurate.

5. Conventions

All 5 @ref anchors in settings.js resolve against LLP 0044/0045 headings (checked with GitHub slug rules - the -- from the em-dashed ## Part 3 — heading is right). No U+2014 in any added line. No semicolons. JSDoc types only, @import at the top, no inline import('...'). The new @ref on manageEnvAdditions earns its place: the ownership rule it points at lives under the ENABLE_TOOL_SEARCH heading but binds every managed env key, and the gloss says so - not something the code or filename tells you.

---

Finding (Low, fixed and pushed)

src/core/config/client_detach_disk.js:182 - the undo's override notice carried the same type test this PR is removing from attach.

}elseif(typeofcurrent==='string'){warnings.push(`${key} was overridden externally; leaving in place`)}

This PR establishes that a hand-written JSON boolean at a managed key is a legitimate user value. The undo's never-clobber notice was still gated on the value being a string, so it stayed silent about exactly that value. Reproduced against a mkdtemp fixture - attach, then the user re-points our own ENABLE_TOOL_SEARCH:

override="manual-string" -> survived="manual-string" warning="ENABLE_TOOL_SEARCH was overridden externally; leaving in place"
override=false -> survived=false warning=undefined
override=0 -> survived=0 warning=undefined
override=null -> survived=null warning=undefined

Not data loss - the value correctly survives - which is why this is Low and not a blocker. But it is a documented-contract violation: LLP 0045 §Never clobber a user edit: report every override, not just the last says core "accumulates one message for each key it left in place", and that those keys "stay on disk after a detach that otherwise claims success, which is exactly the case an operator needs told". It is also inconsistent within the same file: the json_path undo at line 480 already tests presence (current !== undefined); only the json marker branch diverged.

Fixed in 871016e: gated on key in envObj, matching this PR's own "presence is the whole test" language. Deliberately not a bare else - a key the user deleted was not left in place and must not be reported. Two tests, both with teeth:

• reverting the predicate fails not ok 25 - claude undo reports a managed key the user overrode with a non-string
• widening it to else fails not ok 26 - claude undo stays silent about a managed key the user deleted outright

LLP 0045 is unchanged: the doc already said the right thing, so this is code-to-doc conformance, the same shape as Finding 1.

Noted, not fixed

client_detach_disk.js:338 (the legacy pre-record marker path) has the same typeof current === 'string' gate, but only for ANTHROPIC_BASE_URL, which must be a URL string to mean anything - a non-string there is not a plausible user setting, and that branch reverses markers that predate the undo record. Left alone deliberately rather than widening this PR.

---

After the fix:npm test 2850 pass / 8 fail (same 8 leave-command.test.js baseline, +2 from the new tests), npm run typecheck clean.

Reviewed in a detached worktree. No hyp subcommand was run and no host state was touched: every exercise was a library-level call against mkdtemp fixtures.

[philcunliffe]

Round 2 (final, cap 2) - reviewed 871016e, pushed fixes through 3aa623b

Verdict: the diagnosis in this PR is right and the tests are honest, but the sweep was too narrow. The bug class ("a type gate standing where a presence/ownership test belongs") had four instances, not one. Round 1 fixed the second; round 2 found the third and fourth. The fourth is a High: silent, unrecoverable destruction of a user setting, on ANTHROPIC_BASE_URL itself, i.e. the exact bug this PR is named after, on the one key it never swept.

---

1. Round 1's own fix at client_detach_disk.js:182 - correct, with one refinement

The else if chain is sound. I read the whole branch, not the line:

• key we own that is still ours -> restore-or-delete (:168),
• key we own that vanished -> current === undefined !== ourVal, falls to the presence test, absent, silent. Correct: nothing was left in place.
• key overridden with any JSON type -> reported. Correct, and the point of the fix.

No path produces a spurious "left in place" for a deleted key except one: key in envObj walks the prototype chain, and this loop's key names come off disk (Object.entries(managedEnv), from whatever managed.env a plugin recorded), not from in-tree literals. Reproduced on 871016e:

marker managed.env : {"toString": "x"}
settings env : {"ANTHROPIC_API_KEY": "sk-x"}
result.warning : "toString was overridden externally; leaving in place"

That is precisely the false report the presence test was introduced to prevent, and it fires for a key that is not on disk at all. Fixed to Object.hasOwn. Severity Low (no in-tree attach records such a key; a third-party plugin supplies this record and core should not trust its key names).

Note the asymmetry is real and worth keeping: the attach-side guard's keys are in-tree literals (MANAGED_ENV_ADDITIONS), so key in env was never wrong there. I moved it to Object.hasOwn anyway so one predicate means one thing across both halves; behaviour is unchanged and the existing tests confirm it.

2. client_detach_disk.js:338 (the legacy pre-record path) - round 1's call was wrong. Fixed.

Reachable today: yes. detachJsonMarker dispatches here for any marker without a managed record (:135), which is every pre-upgrade marker still on disk, and the branch has two existing tests.

The argument does not hold. "A non-string is not plausible for ANTHROPIC_BASE_URL" is exactly the reasoning the PR body already rejects for ENABLE_TOOL_SEARCH: settings.json is hand-edited, and null/false is how a user switches an override off. It is also the wrong branch to grant the exception to, because the legacy path reverses by convention rather than by a record, so it is the one that most often meets settings this tree never wrote. Reproduced on 871016e:

env before : {"ANTHROPIC_BASE_URL": false, "ANTHROPIC_API_KEY": "sk-x"}
result : {"changed": true} <- no warning
env after : {"ANTHROPIC_BASE_URL": false, ...} <- left in place, unreported

Never-clobber holds (the equality test at :345 correctly fails), so nothing is lost - only the notice, which violates LLP 0045 §"Never clobber a user edit" the same way. Severity Medium. Fixed to Object.hasOwn(envObj, 'ANTHROPIC_BASE_URL'), deliberately not a bare else. Round 1's commit message claim that "only the json marker branch diverged" was a survey miss.

3. HIGH - fourth instance: the base-URL backup is type-gated, and it destroys data

hypaware-core/plugins-workspace/claude/src/settings.js:123:

constliveBaseUrl=typeofenv.ANTHROPIC_BASE_URL==='string' ? env.ANTHROPIC_BASE_URL : undefined

This is the same bug as Finding 1 of this PR, 80 lines below its own fix, and it is strictly worse. The managed additions have somewhere safe to land when a key is not ours: attach skips them, so the user's value survives on disk. ANTHROPIC_BASE_URL has nowhere, because attach always repoints it. The backup is the never-clobber guard, so skipping it on JSON type does not lose a notice, it loses the value. End-to-end on 871016e, identically for 8080, false and null:

BEFORE ATTACH : {"ANTHROPIC_BASE_URL": false, "ANTHROPIC_API_KEY": "sk-x"}
MARKER : prev_base_url ABSENT, managed.env.ANTHROPIC_BASE_URL present
AFTER DETACH : {"ANTHROPIC_API_KEY": "sk-x"} <- user's value gone
result : {"changed":true,"removed":"http://127.0.0.1:4123"} <- no warning

Attach discards the only backup, records the key as managed anyway, and the undo does exactly what that record authorizes. Nothing to recover from. The string case round-trips byte-identically, so this is invisible until it isn't.

It needed fixing in all three places the value passes through, since any one alone still loses it:

• settings.js:123 attach takes the backup unconditionally (JSON cannot encode undefined, so undefined already means absent, and the existing !== undefined checks are the presence test the type test was standing in for),
• settings.js:129 re-attach carries the recorded prior forward by presence,
• client_detach_disk.js:149 the undo reads prev_base_url by presence, so a backup the marker is holding can no longer be discarded into the delete branch.

The marker keeps the real JSON value; only the display fields (prevValue / restoredValue) coerce to a string, which is the pattern :179 already used for removed.

LLP 0045 updated in the same commit: it stated presence-not-type for "every env key attach adds beside the base URL" and left the base URL's own backup unstated. That gap is what let this through, so the §prev_base_url rule now covers the backup and says why this key needs it more, not less.

4. Test teeth - both round-1 claims verified, plus the new ones

Every predicate is pinned in both directions, and no test fires for more than its own mutant:

mutation	result
:182 -> typeof current === 'string' (round 1's claim)	not ok 25 only
:182 -> bare else (round 1's claim)	not ok 26 only
:182 -> key in envObj	not ok 29 only
:348 -> typeof current === 'string'	not ok 27 only
:348 -> bare else	not ok 28 only
attach backup -> typeof ... === 'string'	8 fail (3 round-trips, re-attach, 4 attach-level)
undo prev_base_url -> typeof ... === 'string'	the 3 round-trips fail
re-attach prev_base_url -> typeof ... === 'string'	the two-attach round-trip fails

Round 1's two claims are exact. I also caught one of my own first-cut tests being toothless (an Object.hasOwn wrapper that was behaviourally identical to a plain read) and replaced the guard with the simpler honest expression plus a re-attach round-trip that does have teeth.

5. Further instances of the bug class

Swept hypaware-core/plugins-workspace/*/src/, src/core/config/, src/core/daemon/, plus the attach handler and client-asset undo. Beyond the four above, no more instances of this exact class. Cleared as legitimate: the JSON-record parse-hardening in detachJsonPathMarker (:479, :498), typeof marker.port === 'number' at :333 (fail-safe direction), the pre-regex typeof ... === 'string' guards, the codex TOML attach/detach (presence tests throughout, clean), and OpenClaw's marker-header parsing.

Two adjacent defects, different class (a schema check standing where a refusal belongs), left for triage:

• claude/src/settings.js:284ensureObject replaces a present-but-non-object env with {}. Confirmed: attach on {"env": "ANTHROPIC_API_KEY=sk-x"} silently writes the block away, no backup, no warning. The sibling openclaw/src/settings.js:756 does the same job correctly, throwing MALFORMED_CONFIG when the value is present but wrong-shaped. Claude's is that function minus the presence check. Medium.
• claude/src/settings.js:307installManagedHooks replaces a non-array hooks.<event> wholesale. Confirmed by the same probe. Low (already-broken config), same shape.

One Info: client_detach_disk.js:352's typeof current === 'string' ? current : String(current) is dead in its branch (reached only when current equals a template literal). Harmless, but it is the copy-paste that made :348 look reasonable.

---

Fixed and pushed

1ea12be presence tests in both undo branches; 3aa623b the base-URL backup across attach/re-attach/undo plus the LLP 0045 rule.

• npm test: 2862 pass / 8 fail; the 8 are the known test/core/leave-command.test.js baseline, unchanged in count and name.
• npm run typecheck: clean.
• 11 new tests, all mutation-verified. All four LLP 0045 anchors used in the touched files resolve against the document's headings. No em dash introduced; the file's 18 are pre-existing on master.
• No hyp subcommand run; every check used mkdtemp fixtures against the library entry points. Host state untouched.

For triage

The two ensureObject / installManagedHooks defects above. They are real and reproduced, but they are a different class from #448 (malformed config rather than a deliberate user value at a valid key) and fixing them means deciding whether attach should refuse rather than repair, which is a design call this PR should not make in passing.

[philcunliffe]

Triage - PR #449 @ 3aa623b (fix-loop cap, 2 review rounds)

Verdict: ship as one unit. No true blocker. All residuals are non-blocking and already tracked.

Independent verification of the HIGH (settings.js:123 base-URL backup)

Worked in a detached worktree off origin/master (74aea66), symlinked node_modules, ran no hyp subcommand.

1. The data-loss path is real on origin/master. Reverted only hypaware-core/plugins-workspace/claude/src/settings.js and src/core/config/client_detach_disk.js to origin/master with the PR's new tests left in place, and ran them directly:

   not ok 4 - claude attach + undo leave a non-string user-owned env value byte-for-byte intact
   not ok 25 - claude undo reports a managed key the user overrode with a non-string
   not ok 27 - claude undo of a LEGACY marker reports a base URL the user overrode with a non-string
   not ok 30 - claude attach + undo restore a 8080 base URL byte-for-byte
   not ok 31 - claude attach + undo restore a false base URL byte-for-byte
   not ok 32 - claude attach + undo restore a null base URL byte-for-byte
   not ok 33 - claude re-attach carries a non-string base URL backup forward, and undo restores it
   not ok 41-43 - attach leaves a user-owned boolean/number/null env value untouched and unmanaged
   not ok 48-51 - attach backs up a 8080/false/null/{"url":"x"} base URL into prev_base_url
   # pass 38 / # fail 14
   

   The 8080/false/null round-trip failures are the HIGH itself, reproduced exactly as described: attach discards the backup on type, records the key as managed anyway, and detach deletes the user's value. Restoring the two files: 52/52 pass.

2. The fix covers all three places the value passes through. Read the diff directly rather than trusting the description:

   • settings.js attach: liveBaseUrl is now the raw env.ANTHROPIC_BASE_URL (no type filter); presence is carried by the existing !== undefined checks below it, since JSON cannot encode undefined.
   • settings.js re-attach: prevBaseUrl is read off the prior marker via Object.hasOwn(priorMarker, 'prev_base_url') instead of a type test, so a non-string prior recorded by attach [codex] Add durable cache spool #1 is carried forward by attach [codex] Remove OpenTelemetry npm dependencies #2 rather than being dropped.
   • client_detach_disk.js:149: the undo's own read of marker.prev_base_url is the same Object.hasOwn swap, so a backup the marker is holding can no longer fall through to the delete branch.
   • Both attach's result.prevValue and detach's restoredValue coerce to a string only for the human-readable field; the value written to disk / carried in the marker is never reshaped. Confirmed by inspection, not just claim.

3. Re-attach interaction sanity-checked. The subtle case - a re-attach must keep the originalprev_base_url, not back up the gateway URL over it - is covered twice: the pre-existing idempotent re-attach keeps the original prev_base_url, not the gateway URL test (string case, unmodified by this PR, still passes) and the new claude re-attach carries a non-string base URL backup forward, and undo restores it test in client-detach-disk.test.js, which does two attaches then a detach and asserts the file lands back byte-for-byte. Both pass with the fix in place. I did not find a gap here.

Also independently confirmed: npm test at 3aa623b is 2862 pass / 8 fail, the 8 being exactly the known test/core/leave-command.test.js baseline (same 8 names as master); npm run typecheck clean; issue #454 exists and matches the two deferred defects verbatim (schema-check-vs-refusal on ensureObject, Medium + Low).

Scope judgement

This PR now fixes four instances of one bug class (type-check standing in for presence/ownership test) plus a version-stamp fix, one instance a High the filed issue never mentioned, discovered by the reviewer rather than planned upfront. That is a materially bigger diff than what was filed under #448.

Judgement: coherent enough to ship as one unit. All four fixes are the same predicate mistake, in the same small cluster of attach/detach code, touched by the same tests, and the High was found because review was reading the rest of this diff for the same pattern - it did not wander in from unrelated work. Splitting it now, post-fix-loop-cap with both rounds already reviewed and 52/52 green, would delay landing a live data-loss fix for no safety benefit; the fix is small, mutation-tested, and self-contained. Noting this rather than acting on it, per the rung's instructions.

Classification

• Fixes 1-4 (the type-vs-presence sweep): TRUE BLOCKER territory addressed - [codex] add PR checks #4 specifically removes a live, silent, unrecoverable data-loss path on master today. Verified above, not just taken on the PR's word.
• Residuals (claude attach silently replaces a malformed env/hooks block instead of refusing, unlike the openclaw sibling which throws MALFORMED_CONFIG #454's two schema-refusal defects): PREFERENCE. Different bug class (a check that's correct but whose response - repair vs refuse - is a design call), pre-existing on master, not made worse by this PR, and already filed with design options rather than dropped. Not filing a duplicate.
• No true blocker found in this PR as written.

PR body

The body was written when this was two findings (#448's original scope) and is now understated: it says nothing about the client_detach_disk.js notice fixes or the HIGH base-URL backup fix, and its npm test: 2848 pass line is the round-1 snapshot, not current. Corrected via gh pr edit, preserving the rest verbatim, per the #438/#441/#445/#447 precedent (an appended corrective block, not a rewrite).

No hyp subcommand was run; host $HOME, ~/.claude/settings.json, ~/.codex/config.toml, and ~/.config/systemd/ were untouched. All exercises were library-level calls in a detached mktemp worktree.