hyp leave's assetless-marker drop was never taught about refused markers (#627) by philcunliffe · Pull Request #630 · hyparam/hypaware

philcunliffe · 2026-08-05T04:23:51Z

The inconsistency

LLP 0186 (PR #622) added refused as a terminal marker state that, like failed, records no effect of its own: an attach refusal stops before it touches the client's settings. The reconciler's reverse gap was taught to treat the two alike. hyp leave's parallel assetless-drop was not, and still read one status name:

if (!marker || (marker.status === 'failed' && installedAssets.length === 0)) {

So an assetless refused marker fell through to the full detachClientViaCore reversal instead of being dropped the way the reverse gap drops it.

This is not data loss: detachClientFromDisk no-ops ({ changed: false }) because a refusal never wrote the client's settings, and the marker is cleared afterwards. It is not silent either, which is what the regression test pins: the pointless disk probe prints

No HypAware marker found in /home/you/.codex/config.toml; nothing to do.

in the middle of hyp leave, naming a settings file the refusal never wrote and the user never had.

The fix

Both gates now share one predicate, markerRecordsNoEffect, placed beside readInstalledAssets in src/core/config/action_reconciler.js for the reason that accessor gives for living there: the markers' droppers are not all handlers, and two gates deciding "nothing to undo" for themselves are two chances to disagree.

The predicate deliberately expresses the property, not a status name. This codebase has repeatedly been bitten by keying on a named state as a proxy for a property (config_missing for "absent", firstLookRan for "actually printed"), so the naive || marker.status === 'refused' the issue sketches is exactly the shape to avoid. What both gates actually ask is "does this marker record any on-disk effect?", and that needs two independent records to say no:

a status that never wrote the handler's effect (failed, refused), and
no installed_assets.

Neither half is sufficient alone. "Carries no assets" cannot be the whole test, because a done attach that copied no files still owns the settings edit it wrote. And "status is failed/refused" cannot be the whole test, because a marker that went done and was later rewritten carries the earlier attach's file list forward.

An unrecognized status counts as recording an effect, so a fifth marker state added later routes to the real reversal rather than being silently dropped by a gate nobody remembered to update. That defaulting is the actual anti-drift property, and it is why this is a shared predicate rather than a second copy of a boolean.

The reconciler's own behaviour is unchanged: its inline condition is replaced by the identical shared call.

Regression test

test/core/leave-command.test.js, two tests:

leave drops an assetless refused attach marker the way it drops an assetless failed one — a mixed store (claude done, codex refused assetless, openclaw failed assetless). Asserts stdout never mentions No HypAware marker found or .codex, that no ~/.codex/config.toml is conjured, that done still reverses on disk and failed is still dropped, and that all three markers are gone.

Verified failing on b56ab21 with the source change reverted and the test kept:
```
not ok 10 - leave drops an assetless refused attach marker the way it drops an assetless failed one
  error: The input was expected to not match the regular expression /No HypAware marker found/
```
Passes after the fix. The other 12 tests in the file pass in both states, so the failure isolates the change.
leave still reverses a refused attach marker that carries installed assets — the overshoot guard. A refused marker carrying installed_assets (a done attach that later re-perform()ed into a refusal) must still take the real reversal, never be dropped, or the marker's file list is orphaned (LLP 0138 #marker-undo). Passes before and after, by design.

Checks

npm test: 3429 tests, 3428 pass, 0 fail, 1 skipped
npm run typecheck: clean

Finding 2 is deliberately unaddressed

Issue #627 carries two deferred findings. Only the hyp leave gate is fixed here. The other one, a reverse() returning refused falling into the reverse gap's generic failure else and being retried forever, is not touched, and closing #627 on this PR should not be read as having resolved it:

It is dead code today. ActionOutcome is one type across perform() and reverse(), so widening it made refused expressible on the reverse hook, but action_attach.js holds the only reverse() in the tree and it returns only done or failed. Nothing can currently reach that branch.
Fixing it would mean settling terminal-undo semantics, a design decision, not a consistency cleanup. Two settled LLPs point in opposite directions: LLP 0138 #refusal-is-not-failure (name what you leave behind and release the marker) versus client attach: probe-less contributes.client can attach but reverse() silently no-ops, orphaning settings #212 / LLP 0138 #marker-undo (never destroy the only record naming files that are still on disk). A wrong terminal decision about an undo is far more expensive than a retried one, which is why the current fallback is the safe half of the pair on purpose.
It remains recorded as an explicit out-of-scope constraint in LLP 0186 § Explicitly out of scope, which already states that a reverse() genuinely needing to refuse requires its own branch, its own answer to "what happens to the marker", and its own request extending LLP 0186. That is where it should be settled, when a real reverse-refusal need appears.

Fixes #627

…ers (#627) LLP 0186 added `refused` as a terminal marker state that, like `failed`, records no effect of its own: an attach refusal stops before it touches the client's settings. The reconciler's reverse gap was taught to treat the two alike. `hyp leave`'s parallel gate was not, and still asked `marker.status === 'failed'`, so an assetless `refused` marker fell through to the full `detachClientViaCore` reversal instead of being dropped. The fallthrough is not data loss, but it is not silent either: the disk probe finds nothing to reverse and prints "No HypAware marker found in <path>; nothing to do." during `hyp leave`, naming a settings file the refusal never wrote. Both gates now share one predicate, `markerRecordsNoEffect`, beside `readInstalledAssets` in action_reconciler.js. It asks the property the gates actually care about (this marker records no on-disk effect), which needs both a status that never wrote one and an empty `installed_assets`: a `done` attach that copied no files still owns the settings edit it wrote, so "carries no assets" alone would be the wrong test. An unrecognized status counts as recording an effect, so a marker state added later routes to the real reversal instead of being dropped by a gate nobody updated. The reconciler's own behaviour is unchanged; its inline condition is replaced by the identical shared call. Co-Authored-By: Claude <noreply@anthropic.com>

…ared predicate exists for The predicate's two halves are both already covered: the reverse-gap and `hyp leave` tests fail if the asset check goes away, and the `done` cases fail if the status check goes away. Its third property was not covered at all: mutating the status test from an allowlist ("failed or refused, else an effect") to a denylist ("done or applied, else no effect") left the whole suite green. That defaulting is the anti-drift property the shared predicate was introduced for. Inverted, a fifth marker state added later is silently dropped by both gates instead of routed to the real reversal, which is the data-loss shape of the #627 bug rather than its harmless one. Pin it, plus the truth table for a marker whose `installed_assets` is malformed rather than absent. Co-Authored-By: Claude <noreply@anthropic.com>

philcunliffe · 2026-08-05T05:06:37Z

Verdict: approve, with one gap fixed on the branch

The fix is correct, the refactor is genuinely pure, and the predicate does avoid the
proxy trap this repo keeps falling into. One finding was actionable and is fixed in
d898a88: the anti-drift property the PR is built around had no test.

Does the predicate actually avoid the proxy trap?

Yes, and for a reason worth naming precisely, because it is not the one the PR body
leads with.

The status half is still an enumeration of named states. What makes it not a proxy
is that it is an allowlist with a safe default rather than a denylist:
status !== 'failed' && status !== 'refused' -> false. Everything the predicate does
not recognize is treated as effect-bearing. That does not remove the need to update
the predicate when a state is added; it makes forgetting cost a wasted disk probe
(exactly the #627 symptom, harmless) instead of a silent drop over a real effect
(data loss). Inverting that one comparison is the difference between the two, and
until this round nothing tested it. See finding 1.

I verified the "a refusal wrote nothing" premise rather than taking it from LLP 0186,
since that is the load-bearing claim and the exact place a status-as-proxy would
diverge. Both in-tree refusal sites return before any write:

hypaware-core/plugins-workspace/openclaw/src/attach.js:159 refuses after
readOpenclawConfig, ahead of the single atomicWriteFile at line 176. The
invariant is stated on the read at line 134 ("pure read-then-decide, no partial
write" / R2).
hypaware-core/plugins-workspace/claude/src/settings.js:329 throws
markActionRefused from inside readSettings, during JSON.parse.

So refused implies "wrote nothing" by an explicit documented invariant, not by
accident. failed is weaker in principle (a perform() that threw after a partial
settings write would be a failed assetless marker recording a real effect), but
openclaw's only fail(..., 'write') sits behind an mtime-guarded atomic write, and
in any case the failed half is unchanged by this PR: both gates already dropped
assetless failed markers on master. Recorded as an observation below, not a
finding.

Byte-identity of the refactor: confirmed

The action_reconciler.js reverse gap on master:

!marker || ((marker.status === 'failed' || marker.status === 'refused') &&
  readInstalledAssets(marker).length === 0)

is exactly markerRecordsNoEffect, term for term. Pure refactor. hyp leave now
evaluates the identical expression, so it matches the reconciler rather than
introducing a third behaviour. installedAssets is still live in central.js (used
by the missing-descriptor branch at :470), so nothing went dead.

Status set verified independently: ActionMarkerStatus is
'done' | 'failed' | 'refused' | 'applied' (src/core/config/types.d.ts:305), while
ActionOutcome.status is only 'done' | 'failed' | 'refused' (:394). applied is
declared but never written as an action marker (the four 'applied' hits in-tree are
ConfigStageResult.action, unrelated). It correctly returns false from the
predicate regardless. A marker with no status at all routes to the real reversal,
which is the right default for a truncated write.

Test strength: verified by mutation, not by report

Reverting src/ and keeping the tests reproduces the claimed failure exactly:

not ok 10 - leave drops an assetless refused attach marker the way it drops an assetless failed one
# tests 13 / pass 12 / fail 1

The overshoot guards genuinely guard. Two mutants against markerRecordsNoEffect:

mutant	killed by
drop the asset check (`return true`)	`leave-command` 11; `action-reconciler` 10 and 17
drop the status check (assets alone)	`leave-command` 4, 8, 10, 12; `action-reconciler` 8, 9

Both halves are load-bearing and covered.

Findings

1. (medium, fixed) src/core/config/action_reconciler.js:537 - the defaulting
direction, the one property the shared predicate exists for, was untested.

A third mutant survived the entire suite:

if (marker.status === 'done' || marker.status === 'applied') return false

That is the same predicate with the allowlist turned into a denylist. npm test
stayed at 3429/3428/0. Nothing anywhere pinned which way an unrecognized status
falls, so the property the PR body calls "the actual anti-drift property" could be
inverted by a later edit without a single test noticing - and inverted, it is the
data-loss shape of #627 rather than the harmless one.

Fixed in d898a88, test/core/action-reconciler.test.js:885-957: two unit tests over
the predicate. The first asserts applied, an invented future state, '', and
case-variant spellings all count as recording an effect, plus a marker with no
status field. The second pins the full truth table, including the malformed-store
cases (installed_assets as a string, as an array of junk, as junk-with-one-real-path)
that readInstalledAssets normalizes. Re-running mutant C against the new tests now
gives not ok 18 ... # fail 1.

2. (informational, no action) Fixes #627 closes an issue with finding 2 open.

Confirmed the PR does not implement finding 2: the reverse gap's failure else
(action_reconciler.js:340-356) is untouched by the diff, and action_attach.js is
still the only reverse() in the tree, returning only done or failed. The PR body
states the omission explicitly under its own heading, with the LLP 0186 §Explicitly
out of scope backing. That is the right disclosure; flagging only that the issue will
close carrying an unresolved item, so if it should stay tracked it needs a fresh issue
after merge. Not editing the PR body.

3. (informational) @refs check out.

LLP 0186#how-the-reconciler-distinguishes-it-from-done resolves (§117), and that
section really does settle the reverse-gap rule the annotation claims. Extending it
to hyp leave is grounded separately by LLP 0138#marker-undo (§92 {#marker-undo}
names hyp detach and hyp leave as droppers that read the field through one
accessor), so the pairing on the new predicate is honest. No LLP file is edited; the
diff is three files. Minor nit, not worth a commit: {@link markerRecordsNoEffect}
inside // line comments at central.js:435 renders nowhere, since only the /** */
occurrence is JSDoc.

Checks

npm test: 3431 tests, 3430 pass, 0 fail, 1 skipped (was 3429/3428/0/1 at
aa31fc7; +2 from this round)
npm run typecheck: clean
No em dashes, no semicolons introduced.

Residual findings

failed is a weaker guarantee than refused. refused is backed by an
explicit no-partial-write invariant (openclaw R2, claude's read-path throw);
failed is not - it means "perform() threw", which in principle can happen after
a settings write, and such a marker would be assetless and get dropped. Unchanged
by this PR (both gates already did this on master) and not reachable through any
in-tree adapter today. Worth a real fix only if an adapter ever grows a non-atomic
settings write.
The allowlist still has to be updated. A future state that genuinely records no
effect will be routed to a pointless disk reversal until someone edits the
predicate. That is deliberate and now tested, but it is a maintenance obligation
moved to one place, not removed.
Finding 2 of Follow-up: deferred review findings from PR #622 #627 stays unimplemented by design (see finding 2 above).

A marker rewritten from `done` to `failed` or `refused` carries the earlier attach's asset list forward but nothing carries its settings write forward, so an attach that landed a settings write while installing no assets reads as "recorded nothing" and is dropped over settings still on disk. The predicate's doc claimed the two records were sufficient. Say what they miss instead, and where the fix belongs. Co-Authored-By: Claude <noreply@anthropic.com>

philcunliffe · 2026-08-05T06:12:08Z

Verdict: do not merge as-is. Round 1's residual 1 resolves to a live data-loss path, and `hyp leave` regresses against `master` because of it.

Round 1 verified the refactor's byte-identity and killed the defaulting mutant. Round 2
was asked to settle the one thing it left open: whether a marker can be dropped over a
real on-disk effect. It can, it is reachable with in-tree adapters, and this PR moves
hyp leave from the side that cleans it up to the side that does not.

Residual 1, resolved: not a partial write, a carried write

I audited every in-tree perform() for a throw following a partial on-disk write, and
that specific shape does not exist. All three registered clients funnel their entire
settings edit through one atomicWriteFile at the end of a pure read-then-decide:

hypaware-core/plugins-workspace/claude/src/settings.js:120-263 mutates only the
in-memory value, single writeAtomic at :252, nothing after it but result
construction.
hypaware-core/plugins-workspace/codex/src/settings.js:28-43 read, prepare, write.
hypaware-core/plugins-workspace/openclaw/src/attach.js:104-204 single
atomicWriteFile at :178, every refusal returns before it (the documented R2).
src/core/util/fs_atomic.js:85-111 is temp-then-rename with renamed tracking, so a
throw anywhere before the rename leaves the target byte-identical, and nothing throws
after it.

So round 1's stated worry is closed. But the audit surfaced a different and worse
route to the same drop, which the marker rewrite creates rather than the handler:

action_reconciler.js:238-259 (the failed branch) and :213-224 (the refused
branch) carry installed_assets forward across a rewrite, and nothing carries the
settings write forward. A marker that reached done (settings written) and is later
re-perform()ed into failed/refused is byte-indistinguishable from one whose first
perform() applied nothing, if that attach installed no assets. markerRecordsNoEffect
then answers true over a real effect, and both gates drop it.

That "if" is not exotic. It is the normal case for one shipped client and a designed-for
case for the others:

openclaw contributes no skills and no subagents at all. Every in-tree
ctx.skills.register / ctx.agents.register targets clients: ['claude']
(claude/src/index.js:276,285) or ['codex'] (codex/src/index.js:241), and
expandAssetClients (src/core/runtime/client_assets.js:312) only widens on 'all'.
So every successful openclaw attach writes models.providers into openclaw.json
and records a done marker with no installed_assets.
materializeAttachedAssets (action_attach.js:514-545) returns [] on any copy
failure by design ("a copy failure must not churn the marker to failed"), and
planClientAssets continues past each unusable asset. A claude/codex attach whose
copies all failed lands in the same state.
Any pre-LLP-0138 marker, which action_attach.js:303 already documents as existing.

And the re-perform that rewrites it is routine: isCurrent returns false on every
gateway rebind to a new ephemeral port (LLP 0086) and on any asset-set change (LLP 0107).
The re-perform then refuses (openclaw ownership conflict after a user hand-edit;
claude markActionRefused on JSONC, claude/src/settings.js:329) or fails (unreadable
or concurrently-edited settings file).

Reproduced against the real reconciler, not reasoned:

marker after drift re-perform: {"status":"refused","request_key":"claude",
  "reason":"settings.json is JSONC","at":"..."}
markerRecordsNoEffect(marker) = true
reverseCalls = 0
markers left = undefined
client settings still on disk = true

The same probe with failed instead of refused behaves identically.

Finding 1 (blocker): `hyp leave` now strands a client's settings where `master` cleaned them

src/core/commands/central.js:451. Same fixture in both trees: ~/.claude/settings.json
holding a real _hypaware block and env.ANTHROPIC_BASE_URL, plus an assetless
refused marker (the drift-rewrite above). Then hyp leave.

	`_hypaware` after leave	`ANTHROPIC_BASE_URL` after leave	leave said anything?
`master` (`b56ab21`)	removed	removed	yes
this PR (`d898a88`)	still there	still `http://127.0.0.1:4388`	no

master's gate was marker.status === 'failed' && assets.length === 0, so an assetless
refused marker fell through to detachClientViaCore, whose disk-driven undo reads the
settings file's own marker and cleans it. markerRecordsNoEffect drops it instead. The
user runs the one command whose whole job is to undo everything, is told it succeeded,
and is left with Claude Code pointed at a port that no longer binds, with no marker left
naming it and no line of output mentioning it.

Scope of the underlying defect vs. what this PR adds, stated precisely:

gate	`failed` assetless	`refused` assetless
reconciler reverse gap	drops on `master` (pre-existing)	drops on `master` (pre-existing, #622)
`hyp leave`	drops on `master` (pre-existing)	reversed on `master`, drops after this PR

Three of the four cells are inherited. The fourth is this PR, and it is the one on the
user-facing command. "Now consistent with the reconciler" is true, and the consistency
is consistently wrong.

This also contradicts the PR body's central claim. The body says the predicate expresses
"the property, not a status name", and that two independent records both have to say
nothing happened. They do not: the asset half covers the asset effect, and the settings
effect of a prior done is covered by nothing. The predicate is a proxy for the
property after all, in exactly the way the body sets out to avoid, and being shared now
makes every future gate inherit the wrong answer instead of one of them.

What I did and did not change

I did not change behaviour. The fix is a marker-schema question (the rewrite has to
record the effect it overwrites, e.g. carrying a prior-done bit forward through the
failed/refused branches, which markerRecordsNoEffect then reads), and CLAUDE.md
reserves settled marker semantics (LLP 0138 #marker-undo, LLP 0186) for a new LLP rather
than a reviewer's edit in the last round.

What I did land, in 7b598df, is honesty about it: the predicate's JSDoc asserted the
two records were sufficient, and the next caller would have believed it. It now names the
uncovered case and where the fix belongs (src/core/config/action_reconciler.js:529-538,
verified via git show HEAD:src/core/config/action_reconciler.js). Also folded in round
1's declined nit: the {@link markerRecordsNoEffect} inside // line comments at
central.js:436 renders nowhere, now plain backticks.

An alternative worth putting in front of triage: #627's actual symptom is a noisy
no-op probe, not a harmful one. Making the idempotent disk-driven reversal quiet when it
finds nothing would fix #627 without ever using marker status as a proxy for what is on
disk. That is a design pivot, not a review edit, so I am not making it.

Finding 2 (informational): round 1's mutants re-verified, plus three of mine

Round 1's fix holds, and both call sites are pinned.

mutant	result
A (round 1's): allowlist to denylist, `status === 'done' \|\| status === 'applied'`	dies - `not ok 66`, 3431/3429/1
B: `if (!marker) return false` (invert the missing-marker default)	dies - `not ok 67` only. Nothing but round 1's new unit test covers a missing marker
C: invert the asset half, `.length !== 0`	dies - 5 tests, `not ok 58/65/67/998/999`
D: reverse-gap call site weakened to `!marker`	dies - `not ok 58/65`
E: `hyp leave` call site weakened to `!marker`	dies - `not ok 998`

No surviving mutant. Round 1's tests pin what they claim.

Finding 3 (informational): finding 2 of #627 is still unimplemented, and still disclosed

Confirmed unchanged: the reverse gap's failure else at
src/core/config/action_reconciler.js:339-365 is not in the diff, and action_attach.js:315
is still the only reverse() in the tree, returning only done or failed. The PR body
still carries its "Finding 2 is deliberately unaddressed" section. Fixes #627 will close
the issue over it, so it needs a fresh issue after merge or it leaves tracking. Not
editing the PR body.

Checks

npm test: 3431 tests, 3430 pass, 0 fail, 1 skipped (unchanged from d898a88)
npm run typecheck: clean
No em dashes, no semicolons introduced.
Pushed 7b598df to fix/issue-627.

Residual findings

Finding 1 is unresolved in code and is the blocker. An assetless marker rewritten
from done is dropped over a settings write that is still on disk, by both gates. Three
of the four gate/status cells are inherited from master, the fourth (refused at
hyp leave) is introduced here and is a regression against master on the user-facing
undo command. Fixing it means the failed/refused rewrite recording the effect it
overwrites, which is a marker-schema decision for a new LLP extending LLP 0138.
The allowlist still has to be updated for a future state that genuinely records no
effect. Deliberate, tested since round 1, centralised rather than removed.
Finding 2 of Follow-up: deferred review findings from PR #622 #627 stays unimplemented by design, and will be closed unresolved by
Fixes #627 unless a fresh issue is filed.

philcunliffe · 2026-08-05T06:52:49Z

Neutral triage: parked, blocker confirmed by independent reproduction

I am the triage rung the fix-loop escalates to after 2 review rounds. Round 2's review
(comment on this PR, d898a88... findings) reported a blocker: hyp leave's reverse
gate, migrated by this PR to the shared markerRecordsNoEffect predicate, drops an
assetless failed/refused marker even when that marker was rewritten from a done
state whose settings write is still on disk. My job was to verify that claim myself
before acting on it, since parking a correct PR is also a cost. I reproduced it. The
blocker is real.

My own reproduction (not inherited from round 2)

Built the exact fixture independently in two fresh worktrees (origin/fix/issue-627
at 7b598df and origin/master at b56ab21): a real ~/.claude/settings.json
carrying _hypaware and env.ANTHROPIC_BASE_URL, plus a client-actions.json attach
marker for claude with status: 'refused' and no installed_assets (simulating
a done attach, whose only effect was the settings write, later re-perform()ed into
a refusal). Then ran hyp leave against each tree, script at
/tmp/claude-1001/-work-hypaware/3832ec93-3811-4502-bd86-c3a74a70d863/scratchpad/repro.mjs.

	`_hypaware` after `leave`	`ANTHROPIC_BASE_URL` after `leave`	`leave` mentions claude?
`origin/master` (`b56ab21`)	removed	removed	yes: `✓ Detached claude (...); Removed http://127.0.0.1:4388`
this PR (`7b598df`)	still present	still `http://127.0.0.1:4388`	no

Both runs exit 0 and print the same overall "left https://central.example" success
banner. On the PR head, the marker is silently deleted
(markers.attach -> undefined) and nothing on stdout names claude or the settings
file at all. This is src/core/commands/central.js:451
(if (markerRecordsNoEffect(marker)) { ... clearClientActionMarker(...); continue }),
compared with master's gate at the same call site,
!marker || (marker.status === 'failed' && installedAssets.length === 0), which lets
a refused marker fall through to detachClientViaCore and get properly reversed.

The root cause, confirmed by reading src/core/config/action_reconciler.js:180-260:
the done -> failed/refused marker rewrite carries installed_assets forward
(:184, :224) but carries no bit at all for "this marker's handler previously wrote
the client's settings." markerRecordsNoEffect (:548-552) can only see the two
records it's given, so it returns true over a real, undocumented-by-the-marker
effect. This is the exact "status as proxy for disk state" anti-pattern round 1's
own review named and the PR body promises to avoid, reappearing across the rewrite
seam rather than in the predicate itself.

Reachability: independently verified, not theoretical

hypaware-core/plugins-workspace/openclaw/src/index.js has no ctx.skills.register
or ctx.agents.register call at all (confirmed by grep across all three client
plugins: only claude/src/index.js:276,285 and codex/src/index.js:241 register
assets, targeting clients: ['claude'] and ['codex'] respectively, never
'openclaw' or 'all'). So every successful openclaw attach is a done marker
with zero installed_assets, and its attach()
(hypaware-core/plugins-workspace/openclaw/src/attach.js:104-204) writes
models.providers via a single atomicWriteFile unconditionally on success.
materializeAttachedAssets (src/core/config/action_attach.js:514-545) returns
[] on any copy failure "by design" per its own doc comment ("a copy failure is a
degraded install ... rather than an attach to redo"), so a claude/codex attach with
fully-failed copies is the same shape.
The re-perform that triggers the rewrite is routine, not contrived:
markerIsCurrent/handler.isCurrent (action_reconciler.js:409-416) documents the
LLP 0086 ephemeral-port-rebind drift path, and openclaw/src/attach.js:73,225
cross-references the same isCurrent() re-attach-on-drift behavior by name.

So: any openclaw attach, or any client attach with failed asset copies, that later
drifts (routine port rebind) and re-perform()s into a refusal or failure, hits this
path. Confirmed, not hypothetical.

The quiet-the-probe alternative: looks viable, flagging for the human

Round 2 floated, without implementing, fixing #627's actual symptom (a noisy no-op
disk probe) by always running the real disk-driven reversal and making it quiet when
it finds nothing, rather than skipping it based on marker status. I checked the
shape of that fix and it looks structurally sound:

detachClientFromDisk (src/core/config/client_detach_disk.js) is already
idempotent and already returns { changed: false } at multiple guard points when
there is nothing on disk to reverse (absent settings file at :152/:766/:1103,
no marker found at :172, no probe at :128).
The noisy line 627 complains about (No HypAware marker found ...; nothing to do.)
lives at the CLI-reporting layer, src/core/commands/clients.js:1280, not inside
the disk-driven reversal itself.
So the fix would be: stop consulting markerRecordsNoEffect to decide whether to
call the reversal at all, always call it, and suppress/soften that one reporting
line (or gate it on changed) when the probe legitimately finds nothing. That
never uses marker status as a proxy for what's on disk, which sidesteps the whole
marker-schema question. I did not implement or test this; it needs its own design
pass and its own review, but nothing I found rules it out.

Non-blocking findings, confirmed, not acted on (this PR is parked, not merged)

The allowlist still needs updating for a future genuinely-no-effect state.
markerRecordsNoEffect (action_reconciler.js:548) is deliberate and now tested
(round 1 pinned the defaulting direction), but it is a maintenance obligation moved
to one place, not removed. Not a blocker on its own.
Fixes #627 will close Follow-up: deferred review findings from PR #622 #627 carrying finding 2 unresolved. Finding 2 (a
reverse() returning refused falling into the reverse gap's generic failure
else, action_reconciler.js:339-365) is genuinely dead code today: I confirmed
action_attach.js:315 (async reverse(requestKey, ctx, marker)) is still the only
reverse() in the tree and only ever returns status: 'done' or status: 'failed'
(grep across src/core/config/action_attach.js, no refused return anywhere in
it). The PR body discloses this itself. If it should stay tracked, it needs a fresh
issue; I am not filing one while this PR is parked, since the eventual fix (or
re-scope) may change what that issue should say.

The decision this needs

Fix the marker schema: teach the done -> failed/refused rewrite to carry
forward whether the prior state wrote the client's settings (not just its asset
list), so markerRecordsNoEffect can see it. This is a marker-schema decision per
CLAUDE.md's LLP rules and needs a new LLP extending LLP 0138, not a review-round
edit.
Take the quiet-the-probe alternative instead: always run the real reversal,
quiet the reporting when it finds nothing. Sidesteps the marker-schema question
entirely; looks viable per the check above but is unimplemented and unreviewed.
Close this PR and re-scope Follow-up: deferred review findings from PR #622 #627 if neither direction is wanted right now.

I am not implementing any of these: this is the design call CLAUDE.md reserves for a
new LLP or an explicit human steer, not something a review round should decide by
editing.

How to unstick

Reply on this PR thread with the direction to take (1, 2, or 3 above, or something
else). Neutral monitors this thread and re-engages on the next tick.

What I did not do

I did not merge, mark ready, or push to master. I made no code changes; the
worktrees I used for reproduction were temporary and are removed. npm test on the
PR head (7b598df, real run, not the truncated no-node_modules subset) still
reports 3431 tests, 3430 pass, 0 fail, 1 skipped, matching round 2's report.

test and others added 2 commits August 5, 2026 04:23

philcunliffe added the neutral:stuck neutral attempted this but cannot complete it autonomously — needs a human label Aug 5, 2026

Navigation Menu

Uh oh!

hyp leave's assetless-marker drop was never taught about refused markers (#627) - #630

hyp leave's assetless-marker drop was never taught about refused markers (#627)#630
philcunliffe wants to merge 3 commits into
masterfrom
fix/issue-627

philcunliffe commented Aug 5, 2026

Uh oh!

philcunliffe commented Aug 5, 2026

Uh oh!

philcunliffe commented Aug 5, 2026

Uh oh!

philcunliffe commented Aug 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

philcunliffe commented Aug 5, 2026

The inconsistency

The fix

Regression test

Checks

Finding 2 is deliberately unaddressed

Uh oh!

philcunliffe commented Aug 5, 2026

Verdict: approve, with one gap fixed on the branch

Does the predicate actually avoid the proxy trap?

Byte-identity of the refactor: confirmed

Test strength: verified by mutation, not by report

Findings

Checks

Residual findings

Uh oh!

philcunliffe commented Aug 5, 2026

Verdict: do not merge as-is. Round 1's residual 1 resolves to a live data-loss path, and hyp leave regresses against master because of it.

Residual 1, resolved: not a partial write, a carried write

Finding 1 (blocker): hyp leave now strands a client's settings where master cleaned them

What I did and did not change

Finding 2 (informational): round 1's mutants re-verified, plus three of mine

Finding 3 (informational): finding 2 of #627 is still unimplemented, and still disclosed

Checks

Residual findings

Uh oh!

philcunliffe commented Aug 5, 2026

Neutral triage: parked, blocker confirmed by independent reproduction

My own reproduction (not inherited from round 2)

Reachability: independently verified, not theoretical

The quiet-the-probe alternative: looks viable, flagging for the human

Non-blocking findings, confirmed, not acted on (this PR is parked, not merged)

The decision this needs

How to unstick

What I did not do

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Verdict: do not merge as-is. Round 1's residual 1 resolves to a live data-loss path, and `hyp leave` regresses against `master` because of it.

Finding 1 (blocker): `hyp leave` now strands a client's settings where `master` cleaned them