Follow-up: deferred review findings from PR #495

[philcunliffe]

Deferred, non-blocking findings from the two neutral-review rounds on #495
(claude attach backs a malformed env/hooks block up onto the marker and repairs it, fixing #454). Triage classified all four as design divergences
or pre-existing corrupt-input gaps, none of them a defect in the shipped
behaviour, and cleared the PR for merge. Recording them here so they aren't
lost.

1. The legacy detach branch drops prev_malformed silently, and orphans more than reported

src/core/config/client_detach_disk.js — detachLegacyJsonMarker, reached
from detachJsonMarker whenever marker.managed is not a plain object.

Round 2 found this branch also orphans ENABLE_TOOL_SEARCH,
_CLAUDE_CODE_ASSUME_FIRST_PARTY_BASE_URL, and every classify-cwd hook
entry — wider than round 1's original report, because the retired convention
this branch replays predates all three. Triage reproduced it directly: a real
attach() against a hand-broken env (recording prev_malformed.env),
followed by a hand-edit that deletes only _hypaware.managed from the
resulting marker (simulating corruption/edit), then detachClientFromDisk
against that fixture. Result: _hypaware is deleted, prev_malformed.env is
gone from the file with no warning, no removed/restoredValue field, and
changed: true, while ENABLE_TOOL_SEARCH, _CLAUDE_CODE_ASSUME_FIRST_PARTY_BASE_URL,
and the classify-cwd hooks on SessionStart/CwdChanged remain in the
file, now unmanaged.

Reachability, verified by execution: the current attach()
(hypaware-core/plugins-workspace/claude/src/settings.js:235) always writes
managed as a plain object in the same write that would ever set
prev_malformed — confirmed with a live attach→re-attach fixture, managed
is present every time prev_malformed is. No code path in this tree writes
prev_malformed without managed. A genuinely pre-upgrade marker (the
literal legacy shape {attached_at,version,port,state_file}) predates
managed entirely, and since ENABLE_TOOL_SEARCH /
_CLAUDE_CODE_ASSUME_FIRST_PARTY_BASE_URL were introduced alongside the
managed record (same LLP 0045), such a marker never had those keys written
in the first place — nothing to orphan there. So the branch is reachable only
via a hand-edited or externally-corrupted marker, never through ordinary
attach/re-attach/detach. Also confirmed pre-existing: detachLegacyJsonMarker
is untouched by PR #495's diff (git diff origin/master...c829325 -- src/core/config/client_detach_disk.js); the PR only adds one more field
(prev_malformed) to a branch that already silently dropped things before
this PR existed.

Not a production defect from ordinary use, but worth fixing: replaying
prev_malformed (and reporting when it can't) even down the legacy branch
would close the corrupt-input gap outright.

2. Detach resurrects a hand-deleted malformed backup, and does so silently

src/core/config/client_detach_disk.js:267-296 (prev_malformed replay in
detachJsonMarker).

If a user deletes the entire env (or hooks) block by hand after attach
repaired it, hyp detach puts the original malformed value back — verified
by execution: attach a malformed env: "SUPER-SECRET-ORIGINAL", hand-delete
the resulting env key from the file, detach; the file comes back with
env: "SUPER-SECRET-ORIGINAL". The sibling prev_base_url mechanism does
the opposite in the same situation — also verified: hand-delete a managed
ANTHROPIC_BASE_URL leaf after attach, detach leaves it deleted.

Two reviewers judged the resurrection itself defensible (the block, at the
moment of deletion, held only hypaware's own repaired keys, so restoring the
pre-attach value is arguably completing a partial manual detach, and nothing
is lost — the opposite of the destruction direction #454 was about). Triage
agrees this is a design divergence rather than a defect: no case here makes
the original unrecoverable while telling the user it is safe, which is the
bar that would make it a blocker.

One thing triage adds on top: a successful prev_malformed restore sets
none of removed/restoredValue/warning (verified — detach result: { changed: true, settingsPath }, nothing else), so hyp detach prints
nothing at all when this fires, unlike a successful prev_base_url restore
(Restored <value>). Worth aligning the reporting, independent of whatever
the resurrection-vs-leave-deleted call ends up being.

3. Sidecar-file backup for an unrestorable value

Recorded by round 1 as outside the maintainer's decision on #454 (which asked
for the marker-based back-up-then-repair scheme, not a second on-disk
artifact), and left as an Open Question in LLP 0163. A sidecar file
(settings.json.hypaware-backup-<ts>) would make the "detach restores it"
promise unconditional even when the marker's copy can't be replayed, at the
cost of on-disk surface nobody currently cleans up. Scope decision for the
maintainer, not a defect in what shipped.

4. Marker schema change to let nested backups follow prior-wins

src/core/config/client_detach_disk.js:256-266 and LLP 0163. When two nested
paths are both malformed across separate attaches (e.g. hooks and
hooks.SessionStart), only one backup can occupy the emptied slot on detach,
and the current replay order is shallowest-first. Round 2 proved by running
both recording sequences that depth is orthogonal to age — shallowest-first
keeps the later value in one sequence and the earlier value in its mirror —
so no fixed sort order implements "the earliest backup holds the user's
content." The record carries no age field today. An explicit per-entry
timestamp or order counter in prev_malformed would let the replay pick
correctly regardless of nesting depth. Both current-behaviour sequences are
pinned by tests either way, so nothing regresses if this stays open; it is an
enhancement, not a bug — the loser at the tiebreak is always honestly
reported as discarded, never silently dropped.

---

Backlink: #495