detach: the undo replays every backup the marker holds, and says what it restored (#500) - #506
Conversation
… it restored (#500) Three deferred findings from the review of #495 (LLP 0163, `claude attach` backs a malformed `env`/`hooks` block up onto the marker and repairs it). All three are the undo failing to say something, two of them while dropping a backup and reporting success. Finding 3 (silence). A successful `prev_malformed` restore set none of `removed`/`restoredValue`/`warning`, so `hyp detach` printed `✓ Detached claude` and stopped while it rewrote a block. `DetachFromDiskResult` gains `restoredPaths: string[]`; the command prints one line each and echoes `restored_paths` into `--json`. Paths, never values: a malformed `env` is exactly where an API key ends up, which is also why this cannot ride on `restoredValue` (rendered as a bare value by both consumers). Finding 2 (delete-then-detach resurrection) is deliberately NOT changed. Two reviewers and triage judged restoring the pre-attach value defensible, and reversing it would discard a value rather than keep one. It is now audible instead of silent, which was the objectionable half. Finding 1 (the legacy branch). `detachLegacyJsonMarker` is selected by `managed` not being a plain object, so a current-shape marker whose record has been damaged lands there still carrying its backups - and they were dropped without a word, while the marker holding the only copy was deleted in the same write. It now replays `prev_malformed` through the same helper and with the same words as the record-driven branch, and restores a recorded `prev_base_url` instead of deleting the key. The managed env keys the unreadable record named stay (nothing on disk proves they are ours, and never-clobber outranks tidiness) and the reversal reports itself partial; managed hooks are stripped, because matching `hyp claude-hook …` is proof of ownership rather than a guess. A genuine pre-record marker carries none of the triggering fields and is reversed, and stays silent, exactly as before. The shape finding 1 needs is only reachable by hand-editing `managed` out of a marker, so the fixtures construct it directly. Findings 4a (sidecar backup file) and 4b (a marker schema change for prior-wins between nested paths) are the maintainer's parked open questions and are left open. LLP 0163 updated in the same commit. Co-Authored-By: Claude <noreply@anthropic.com>
CI intermittently failed test/core/client-detach-disk.test.js: 3 assertions
did JSON.stringify(result).includes('sk-x') / out.includes('sk-x') to prove
the ANTHROPIC_API_KEY test secret never leaks into the detach result or CLI
output. But settingsPath is always present in that text, comes from
stageHome()'s fs.mkdtemp('.../hyp-detach-disk-'), and its random suffix
starts with 'x' about 1/62 of the time - which, glued onto the fixed prefix's
trailing "...disk-", spells "sk-x" with no secret involved. Strip settingsPath
out of the checked text before the leak check in all three assertions.
Reproduced deterministically by forcing a home dir suffix that starts with
'x' and confirming the false positive, then confirming the fix clears it.
Three review findings on #506. 1. `hyp detach --json` gained a `restored_paths` key but `ClientResult` (src/core/cli/types.d.ts) - the declared shape of that payload, and the return type of the published `integration.detach()` helper - was not extended, so a typed consumer sees a field the CLI emits but the contract denies. Declared, with the paths-never-values rule stated where a caller reads it. 2. The `--json` half of the new reporting had no test; only the prose line did. Added the sibling `--json` case, asserting `restored_paths` and the same no-secret-in-output check. 3. src/core/config/client_detach_disk.js imported `DetachFromDiskResult` as `from './types.d.ts'` - the only such specifier left in src/ (299 use the root-anchored form) and one CLAUDE.md forbids by name. It dangles in the published declaration build: tsc does not copy `.d.ts` inputs, so there is no `types/core/config/types.d.ts` for it to reach, and the interface this PR extends resolved to `any` for package consumers. Re-anchored. Co-Authored-By: Claude <noreply@anthropic.com>
Verdict: approve with fixes (3 findings, all fixed and pushed as
|
reverse() logged the failure half of the prev_malformed replay and not the success half, so an org config drop could rewrite a block of the user's settings file with nothing recorded anywhere. hyp detach prints a line per restored path; this path has nobody at a terminal, so the log is the only place it can be said. Paths, never values, as everywhere else. Also refreshes two JSDoc paragraphs that still described the legacy hook strip as session-context-only after this PR widened the pattern to classify-cwd. Co-Authored-By: Claude <noreply@anthropic.com>
Verdict: approve with fixes (1 finding + 1 nit, both fixed and pushed as
|
| before | after |
|---|---|
{"env":{"MY_OWN":"keep-me"},"permissions":{"allow":["x"]}} |
{"env":{"MY_OWN":"keep-me","ENABLE_TOOL_SEARCH":"true","_CLAUDE_CODE_ASSUME_FIRST_PARTY_BASE_URL":"1"},"permissions":{"allow":["x"]}} |
{} |
{"env":{"ENABLE_TOOL_SEARCH":"true","_CLAUDE_CODE_ASSUME_FIRST_PARTY_BASE_URL":"1"}} |
{"hooks":{"SessionStart":[{"hooks":[{"type":"command","command":"my-own-thing"}]}]}} |
identical, plus the same two env keys |
Every user-owned key survives byte-identical: MY_OWN, permissions, and the user's
own SessionStart hook. The marker, the gateway base URL and the managed hooks are
all correctly gone. The entire residue is the two keys hypaware itself added, and
the decisive fact is manageEnvAdditions
(hypaware-core/plugins-workspace/claude/src/settings.js:292):
if (!weOwnIt && Object.hasOwn(env, key)) continue - presence, not type - so attach
never writes over a value at those keys, and there is therefore no prior user value
for the partial reversal to fail to give back. Reaching this state also requires no
prev_base_url, which (per settings.js:199) means there was no prior base URL
either. So the partial reversal cannot lose a setting: there is provably nothing at
any of the affected keys that was ever the user's.
That is a stronger statement than round 1's "nothing is destroyed, only left behind",
and it is what makes this a leftover rather than a data-loss bug. I also agree it is
undetectable without guessing env-key names, which is the guess this PR is right to
refuse. Not a blocker. The leftovers are inert (ENABLE_TOOL_SEARCH=true and
_CLAUDE_CODE_ASSUME_FIRST_PARTY_BASE_URL=1 against a now-absent base URL, i.e. a
genuinely first-party endpoint) and sit in the file the user just hand-edited.
Also recorded, deliberately not raised as blockers
- An
envbackup is structurally unrecoverable down the damaged-marker branch.
The managed additions the branch may not delete keepenvnon-empty, so the replay
always hits "in use again" and the backup dies with the marker
(test/core/client-detach-disk.test.js:981pins exactly this). Real destruction,
but strictly better than the pre-PR behaviour, which dropped the same backup with
no notice at all, and the honest alternatives (keep the marker; write the backup to
a side file) are design changes well outside Follow-up: deferred review findings from PR #495 #500. LLP 0163 says it plainly. - A damaged marker whose
portis also corrupted leaves the dead gateway URL in
place and destroysprev_base_urlwith the marker. Corruption of corruption, and
the "overridden externally" notice fires, so it is audible. - Finding 2's resurrection and open questions 4a/4b are unchanged and correctly
left as Open Questions in LLP 0163.
No changes to the PR body, no labels, no merge.
|
TRIAGE rung (LLP 0017): the review fix-loop exhausted its 2 rounds at head
Every residual is non-blocking (leftover inert env keys from a hand-damaged |
philcunliffe
added
the
neutral:approved
1 participant
Deferred review findings from #495 (LLP 0163:
claude attachbacks a malformedenv/hooksblock up onto the_hypawaremarker and repairs it, rather thanrefusing). #495 has merged, so these are actionable. All three fixed findings
are the undo failing to say something, two of them while dropping a backup
and reporting a successful detach.
No maintainer decision was recorded in the issue thread (
gh issue view 500 --json commentsreturns an empty list), so nothing here overrides the filedfindings.
Finding 3 - a successful
prev_malformedrestore was completely silentVerified on
master: a detach that puts the user'senvblock back sets noneof
removed/restoredValue/warning, sohyp detachprints✓ Detached claudeand nothing else while rewriting the file.DetachFromDiskResultgainsrestoredPaths?: string[].hyp detachprintsRestored <path> from the marker's malformed-block backupper entry andechoes
restored_pathsinto--json.Paths, never values. LLP 0163 already forbids the attach-side notices from
echoing the displaced value (a malformed
envblock is exactly where an API keyends up, and these strings are printed and logged); the same rule governs the
way back. That is also why this cannot ride on
restoredValue, which bothconsumers render as a bare scalar. A list rather than a joined string, for the
reason LLP 0045 Part 3 already gives for
warnings: string[].FAIL on
master:PASS after:
Finding 2 - the delete-then-detach resurrection: behaviour deliberately unchanged
Reproduced on
master: hand-delete the repairedenvblock after attach, andhyp detachputs the original malformed value back, where the siblingprev_base_urlleaves a hand-deleted leaf deleted.Not changed. Two reviewers and triage judged the restore defensible (at the
moment of deletion the block held only hypaware's own repaired keys, so
restoring the pre-attach value completes a partial manual detach and nothing is
lost), and flipping it would discard a value rather than keep one. It does not
meet the blocker bar: nothing is made unrecoverable while the user is told it is
safe.
What is fixed is that it happened without a word. Finding 3's field covers it,
and the regression test is written in exactly that shape:
FAIL on
master/ PASS after:Finding 1 - the legacy branch dropped backups silently
detachLegacyJsonMarkeris selected bymarker.managednot being a plainobject, so a current-shape marker whose record has been damaged lands there
still carrying
prev_malformed/prev_base_url. Those were dropped without aword, while the marker holding the only copy was deleted in the same write. On
this document's own terms that is destruction reported as a successful detach.
The branch now:
prev_malformedthrough the same helper and the same words as therecord-driven branch (extracted as
replayPrevMalformed), reporting a backupit cannot put back rather than dropping it;
prev_base_urlinstead of deleting the key;classify-cwdhooks as well assession-contextones - matchinghyp claude-hook …is proof of ownership, not a guess, so widening thepattern cannot clobber a user's own hook;
(
ENABLE_TOOL_SEARCH,_CLAUDE_CODE_ASSUME_FIRST_PARTY_BASE_URL) in placeand reports the reversal as partial. Nothing on disk distinguishes a value
hypaware wrote from one the user did, and never-clobber-a-user-edit outranks
tidiness. This is the one half of the finding that is reported rather than
repaired, deliberately.
Reachability. As triage established by execution, ordinary
attach()alwayswrites
managedin the same write that ever setsprev_malformed, so thisshape is unreachable without a hand-edited (or otherwise corrupted) marker. The
fixtures therefore construct the state directly -
breakManagedRecord()deletes_hypaware.managedfrom a real post-attach file - and say so in a comment.A genuine pre-record marker carries none of the triggering fields, so its
reversal and its silence are unchanged; that is pinned by its own test, which
passes on both sides.
FAIL on
master:PASS after:
prev_base_urlin this branch is not one of the four filed findings. It is theidentical defect one field over, in the same branch, exposed by the same fixture
so fixing
prev_malformedalone would have left the branch half honest.Left open
LLP 0163 (
settings.json.hypaware-backup-<ts>would make the promiseunconditional at the cost of on-disk surface nobody cleans up). Untouched.
Note this change makes an unrestorable backup loudly discarded on both
branches rather than silently on one, which is a reason to revisit it, not a
resolution of it.
The other parked open question. Untouched; the shallowest-first tiebreak and
both recording sequences stay pinned by their existing tests.
Docs
LLP 0163 updated in the same commit: two new subsections under Detach restores
the backup (the reporting rule, and the legacy branch), two consequences
bullets, and the paragraph that accepted the silent drop as corrupt-input
behaviour is replaced. Both open questions are left standing.
@refanchorschecked; the new legacy-branch annotation points at the new subsection.
Checks
npm teston this branch: 3094 tests, 3085 pass, 8 fail, 1 skipped.origin/masterbaseline in a pristine worktree, run serially: 3087tests, 3078 pass, 8 fail, 1 skipped. The 8 are the pre-existing
test/core/leave-command.test.jsfailures, identical by name on both sides(
leave after join removes the seed and reports the server,leave clears an applied central slot, not just the seed,leave reverses org-driven attaches and drops the forward identity,leave after join also warns about a local central sink that keeps forwarding,leave is idempotent: a second leave is the not-connected no-op,leave still tears down when only a stale attach marker survives a prior partial leave,leave removes the assets its attach marker records, and leaves manual copies alone,leave self-heals an org attach whose plugin is gone: drops the marker, warns, stays clean). Delta is+7 tests, +7 passes.
npm run typecheck: clean.npm run smoke -- client_attach_idempotent: ok.npm run smoke -- claude_attach_detach: FAIL on both sides, at the samehook-command path assertion (the smoke expects the packaged
hypbin and getsthe worktree's
hypaware-core/smoke/index.js). Worktree artifact, disregarded.npm run smoke -- client_attach_on_join: FAIL on both sides, atno re-attach: the attach.claude marker timestamp is unchanged. Pre-existing,unrelated.
No file here is touched by #491 (
fix/issue-481) or #502 (fix/issue-421);their live file lists were checked with
gh pr diff --name-only.Fixes #500