Skip to content

A terminal marker rewrite records the effect it overwrites (#780 item 2) - #799

Merged
philcunliffe merged 4 commits into
masterfrom
fix/issue-780
Aug 18, 2026
Merged

A terminal marker rewrite records the effect it overwrites (#780 item 2)#799
philcunliffe merged 4 commits into
masterfrom
fix/issue-780

Conversation

@philcunliffe

@philcunliffephilcunliffe commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Issue #780 records four non-blocking follow-ups deferred during PR #630's review. This PR fixes item 2, the one behavioural defect among them. The other three are addressed below: one is already done, two are deliberately not actionable and are explained rather than touched.

Item 2 (fixed): a done to failed/refused rewrite recorded assets but not the settings write

The defect

src/core/config/action_reconciler.js's reverse gap drops a terminal marker for a request key the config stops naming, reading "no installed_assets" as "this key never applied anything". LLP 0138 #marker-undo states that reading directly: "failed normally means nothing was applied, and installed_assets is the evidence that something was."

That is true for the half of an attach that copies files. It says nothing about the half that writes the client's settings, and nothing in the marker schema recorded that half at all. So for a client whose attach copies no files (openclaw is the routine case: it writes models.providers into ~/.openclaw/openclaw.json and installs nothing):

  1. The reconciler attaches. perform() returns done; the marker records status: 'done' and no installed_assets, because there are none.
  2. The recorded input drifts (LLP 0086's isCurrent() reports the marker stale), the forward gap re-perform()s, and this pass refuses or fails. The marker is rewritten in place to refused/failed, carrying an empty asset list forward.
  3. The org drops the client from the fleet config. The reverse gap sees a terminal marker with no assets and deletes it without calling reverse().

The settings entry from step 1 is still in the file. After step 3 nothing names it: hyp status shows no action, and the reconciler has no marker left to reverse. That is exactly the orphaning #212 and LLP 0138 #marker-undo refuse to accept, reached by a route neither checked. The reverse gap's own comment already argues the opposite ("the settings half cannot [degrade to naming-and-releasing], because nothing else on disk would own the settings it left written") - the comment was right, the condition beneath it had nothing to read.

The fix

New LLP 0247, extending LLP 0138 #marker-undo and LLP 0186 #how-the-reconciler-distinguishes-it-from-done (both Active, neither edited beyond a mechanical Extended-by: forward-ref). This is the design pass the maintainer asked for on #630 ("The marker-schema question ... needs its own design pass and review").

  • ActionMarker gains one optional field, prior_done?: boolean. The reconciler's failed and refused rewrite branches set it when the marker they replace already recorded an applied effect. A done marker never carries it: its status already says the same thing, and two names for one fact are two chances to disagree.
  • It is carried, not recomputed, so it survives an arbitrary chain of later rewrites (done -> failed -> failed), the same way installed_assets survives one. Read through one exported accessor, markerRecordsPriorDone(), beside readInstalledAssets() and for the reason LLP 0138 gave for that one.
  • It is written after the outcome's detail spread: detail is handler-reported, prior_done is reconciler bookkeeping about an effect that is really on disk, and a handler that erased it would re-open this defect.
  • The reverse gap's drop now needs both halves of the evidence to be empty. Everything else is unchanged: a terminal marker that never reached done is still dropped, which is the whole reason LLP 0186 put refused in that gate.
  • No migration and no format break: the field is absent on every pre-existing marker, absent reads as "no prior done", and that is master's behaviour exactly.

hyp leave's sibling copy of the same unsound gate (src/core/commands/central.js) is deliberately not touched here: PR #630 removes that shortcut outright rather than teaching it a new field, so the two fixes are independent and this branch keeps central.js byte-identical to master to avoid conflicting with it.

Proof

Standalone reproduction against origin/master at 04330abb (three reconcile passes over a fake handler: done with no assets -> refused after isCurrent() drift -> key no longer desired):

reverseCalls = []
results = []
marker left = null
SYMPTOM: settings write dropped with the marker, nothing reversed it

After the fix, the same script:

reverseCalls = ["openclaw"]
results = [{"kind":"attach","requestKey":"openclaw","outcome":"reversed"}]
marker left = null

Two regression tests in test/core/action-reconciler.test.js, both driven end to end through reconcile() so nothing about the bit is hand-seeded:

  1. a settings-only attach rewritten from done to refused is reversed, not dropped (LLP 0247) - asserts the done marker carries neither installed_assets nor prior_done, that the rewrite to refused records prior_done: true, and that the reverse gap calls reverse() and reports reversed.
  2. the prior-done bit survives repeated failed rewrites, and a key that never applied anything is still dropped (LLP 0247) - two keys under one handler, one that reaches done then fails twice and one that fails from the first pass. Asserts the bit is set on the first and absent on the second, survives the second failing rewrite, that attempts still counts, and that only the first is reversed.

Both fail on the current tree (prior_done is undefined, and with that assertion removed reverseCalls is []) and pass after the fix. The existing LLP 0186 reverse-gap test - an assetless refused marker that never reached done is dropped; one carrying installed_assets is reversed - is unchanged and still passes; it is the control this must not break.

$ node --test test/core/action-reconciler.test.js
# tests 19 # pass 19 # fail 0 (before the fix: # pass 17 # fail 2)

Checks

  • npm run typecheck: clean.
  • npm test: 4219 tests, 4198 pass, 20 fail, 1 skipped. All 20 failures are pre-existing on origin/master and unrelated (query pushdown / NULL semantics: whereToParquetFilter ..., pushed-down comparisons do not leak NULL rows, etc.). The failing set was captured on the pristine checkout and on this branch and diffed: byte-identical, 20 lines each. This branch moves the suite from 4217 tests / 4197 pass to 4219 / 4198, and changes no failure.
  • No duplicate LLP numbers. 0245 and 0246 are already claimed on origin/integration/proxy-mode-capture and origin/integration/proxy-mode-default-attach, so this doc took 0247, the next number free across every branch.

Item 3 (already done, nothing in this PR)

PR #630's body was rewritten on 2026-08-15 to describe what actually merges, preserving the neutral-triage marker and the Fixes trailer. Recorded on #780 already.

Item 1 (not fixed, deliberately): reverse() returning refused falls into the generic failure branch

Unchanged and out of scope, for the reason the issue itself gives. It is dead code today (src/core/config/action_attach.js holds the only reverse() in the tree and returns only done or failed), and fixing it means settling terminal-undo semantics between two settled LLPs that point in opposite directions (LLP 0138 #refusal-is-not-failure versus #marker-undo / #212). LLP 0186 §Explicitly out of scope already requires that a reverse() genuinely needing to refuse brings its own branch and its own answer to "what happens to the marker", in its own request. Inventing that answer with no caller to test it against would be a design decision made by accident, which is what #780 exists to prevent. LLP 0247 restates it as still-open under Consequences so it stays visible.

Item 4 (not fixed, deliberately): hyp detach all narrates every unattached client

Marked "minor, optional / preference only" in the issue, and the ready-made mechanism it names - the quietNoop option on detachClientViaCore - exists only on PR #630's branch, not on master (verified: grep -rn quietNoop over this tree returns nothing). Implementing a second, independent quietNoop here would collide with #630 in src/core/commands/clients.js and send a currently mergeable, reviewed PR back through triage. It is a one-line gate once #630 lands.

Fixes#780

testand others added 2 commits August 17, 2026 20:46
The reconciler's reverse gap dropped an assetless failed/refused marker
for a request key the config stops naming, reading "no installed_assets"
as "this key never applied anything". That is only true for the half of
an attach that copies files. An attach that reached `done`, wrote the
client's settings and copied nothing (openclaw, routinely), then
re-performed into failed/refused, produced a marker indistinguishable
from one whose attach never touched the disk - and the drop stranded the
settings write with nothing naming it.
Record a `prior_done` bit on the rewrite and read it in the drop
condition, so such a marker is handed to reverse() instead.
Item 2 of #780, deferred from PR #630's review.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PR #797 (`fix/absolute-form-third-front-door`) claimed 0247 for
`llp/0247-absolute-form-third-front-door.decision.md` about a minute
before this branch's commit, so both PRs introduce an LLP 0247 and both
merge cleanly (different filenames). The result on master would be two
documents claiming one number, an ambiguous `@ref LLP 0247#the-bit`, and
a red `test/core/llp-ref-hygiene.test.js` ('no LLP number is claimed by
two documents').
LLP 0156#renumber settles this: the later claimant moves above the
highest number claimed anywhere. 0245-0249 are all claimed on unmerged
branches, so this doc takes 0250. Mechanical rename plus the reference
sweep in the same commit (the doc title, the two `Extended-by:`
forward-refs on LLP 0138/0186, four `@ref`s and two prose mentions in
`action_reconciler.js`, two in `types.d.ts`, and the test file's `@ref`
and two test names). Nothing the document decided changes.
@philcunliffe

Copy link
Copy Markdown
ContributorAuthor

Neutral review round: PR #799 (fix/issue-780, reviewed at head 30d33e3c)

Verdict: the change is correct and I would land it. The defect is real, the fix is the right shape, it is complete on every path the reconciler owns, and the two regression tests are genuine (verified independently: restoring origin/master's action_reconciler.js + types.d.ts under this branch's test file gives # pass 17 # fail 2, failing exactly the two new tests; the branch gives 19/19).

One finding is fixed and pushed. Three are recorded for a human, one of which is a design call I deliberately did not override.


1. medium - LLP number collision with PR #797 (fixed, pushed)

llp/0247-marker-records-the-effect-it-overwrites.decision.md (this branch) and llp/0247-absolute-form-third-front-door.decision.md (origin/fix/absolute-form-third-front-door, PR #797) both claimed LLP 0247, about a minute apart:

claimantcommit adding the docPR created
#7974120e77f 2026-08-17T20:45:18Z2026-08-17T20:45:42Z
#79930d33e3c 2026-08-17T20:46:40Z(this PR)

The body's "No duplicate LLP numbers ... 0247, the next number free across every branch" was true when written and is no longer. The two files have different names, so git merges both without conflict and neither branch's CI catches it. On master the result is two documents claiming one number (making @ref LLP 0247#the-bit formally ambiguous) and a red test/core/llp-ref-hygiene.test.js:458 (no LLP number is claimed by two documents) - the exact invariant LLP 0156 unskipped that test to enforce.

Fixed per LLP 0156 #renumber ("the later claimant moves to a fresh number above the highest number claimed anywhere, including branches without an open PR"). 0245-0249 are all claimed on unmerged branches, so this document is now LLP 0250, with the reference sweep in the same commit:

  • git mv to llp/0250-marker-records-the-effect-it-overwrites.decision.md plus its # LLP NNNN: title
  • the Extended-by: forward-refs on llp/0138-...:9 and llp/0186-...:8
  • four @refs and two prose mentions in src/core/config/action_reconciler.js (235, 274, 335, 339, 542, 549)
  • two prose mentions in src/core/config/types.d.ts (373, 374)
  • the @ref and both test names in test/core/action-reconciler.test.js (889, 891, 964)

Nothing the document decided changes; this is the mechanical renumbering CLAUDE.md explicitly permits on an Accepted doc. Verified on the pushed branch: git grep 0247 over src/, test/, llp/ returns nothing, all four @refs read LLP 0250, npm test 4219 tests / 4218 pass / 0 fail / 1 skipped, npm run typecheck clean, npm run smoke -- client_attach_on_join ok.

Residual race, for a human:#797 still claims 0247 for its own doc, and additionally double-claims 0246 with origin/integration/proxy-mode-default-attach, so it likely needs a sweep of its own. Whoever handles #797 should take 0251+ and leave 0250 here, or flip this one back and move #799 higher instead. Either resolution is fine; two 0247s is not.


2. medium - rearmRefusedActionMarker() still drops a marker carrying prior_done (recorded, deliberately not fixed)

src/core/config/action_reconciler.js:660-661. The re-arm decides by installed_assets alone, so an assetless refused marker carrying prior_done: true is deleted outright. A second reviewer reproduced the full chain end to end on this branch:

  1. org attach openclaw -> done (settings written, no assets)
  2. isCurrent() drift -> refused + prior_done: true
  3. user runs hyp attach openclaw; it succeeds and the re-arm deletes the marker
  4. next reconcile pass re-perform()s and fails transiently -> failed with noprior_done (its existing was undefined)
  5. org drops openclaw -> the reverse gap drops that marker without calling reverse() (reverseCalls: [], marker left: null)

So the new invariant does not survive an explicit hyp attach, and LLP 0250's Consequences bullet ("rearmRefusedActionMarker() is unchanged ... No effect is stranded") rests on a claim the chain above falsifies: it assumes the next pass always writes a done marker back, and a failing pass does not.

I did not fix it, and I think the decision is defensible on a reason the document states only in passing. The re-arm runs only after await client.attach(...) resolves (clients.js:451, call at :484), so at step 3 the settings have just been rewritten by the user's own explicit attach. A hand-attached client records no marker at all by design (clients.js:500; central.js's "a client the user attached by hand stays"), and hyp detach openclaw reverses settings from the client's own file with no marker needed. That is exactly why the with-assets branch has to preserve the record and this one does not: nothing but the marker names org-installed asset paths, whereas the settings name themselves. Nothing is stranded in the "no longer named on disk" sense that #212 and LLP 0138 #marker-undo are about; what is lost is the reconciler's automatic undo, for a client the user just explicitly attached by hand.

And the obvious fix is not free: preserving the bit would make an org config-drop silently undo a user's explicit hyp attach, which contradicts the documented "a client the user attached by hand stays" rule. That is a product tradeoff, not a bug fix, so overriding a decision this PR settled one commit ago is not a neutral reviewer's call.

What I would ask the author for: replace the "No effect is stranded" bullet's reasoning with the disk-names-itself argument above, since the current wording is falsifiable as written. That is an editorial correction to an unmerged doc, not a change to what it decided.


3. low - markerRecordsPriorDone() does not know the applied marker status (recorded, not fixed)

src/core/config/action_reconciler.js:554 recognises only status === 'done', but ActionMarkerStatus is 'done' | 'failed' | 'refused' | 'applied' (types.d.ts:305), with applied documented at :301 as "current applied state of a reconciled/reversible handler". An applied -> failed rewrite would record no prior_done and the reverse gap would drop it, which is precisely the bug this PR fixes.

Not fixed, because the gap is wider than the accessor and papering over one cell would misrepresent it: nothing anywhere writes status: 'applied' to a marker (git grep "'applied'" over src/, bin/, hypaware-core/ finds only apply.js's unrelated ConfigStageResult.action), and the rest of the reconciler does not know the state either - the forward gap's short-circuit at :155 tests status === 'done', so an applied marker would re-perform() on every pass, and the reverse gap at :343 would send it to reverse() unconditionally. Either applied should be dropped from the union or the reconciler should learn it end to end; a one-token disjunct here would create a false impression that it is supported. Worth an issue.


4. info - PR body staleness after the renumber

The body's "this doc took 0247" and "No duplicate LLP numbers" no longer describe the branch; the document is LLP 0250. Not edited, per this round's rules.


What I scrutinised and cleared

prior_done is set on every path that rewrites a marker to a terminal state. The forward gap is the only writer of a terminal marker, and both terminal branches set it: :237 (refused) and :276 (failed). The bit is carried, not recomputed, so arbitrary chains hold: done -> failed reads existing.status === 'done'; failed -> failed and failed -> refused read existing.prior_done === true. done -> refused is a sink, because a refused marker short-circuits unconditionally at :151. The only other in-tree marker writers are clearClientActionMarker (a drop after a completed reversal) and rearmRefusedActionMarker, whose asset branch spreads ...marker and carries the bit through (:664). No other module writes the store.

A handler's detail cannot clobber it.marker.prior_done = true is a statement after the object literal in both branches, so it wins over ...(outcome.detail ?? {}). Note the converse is open, and deliberately so: ActionOutcome.detail is a JsonObject (types.d.ts:416), so a handler returning { detail: { prior_done: true } } on a first-pass failure would pin that key's marker permanently un-droppable. No handler does this and the failure direction is the conservative one, so a guard would cost more than the hazard.

The absent case is master's behaviour exactly.markerRecordsPriorDone returns false for undefined, for a missing field, and for any non-true value (a hand-edited "yes" included). In the reverse gap the new clause is &&-ed onto a condition already gated on status === 'failed' || 'refused', so on a pre-upgrade store the gate evaluates identically; the status === 'done' arm of the accessor is dead in that position and live only at the two write sites. Empirically confirmed: the 17 pre-existing tests, including the LLP 0186 reverse-gap control, pass unchanged, and the test diff is 131 insertions(+), 0 deletions(-) - the control was not weakened.

Nothing reverses that should not. The only new reversal is a terminal marker that really did reach done, and reverse() is idempotent by construction: action_attach.js:316 runs detach({descriptor, env}) off the client's own settings file and returns done even when there is nothing left to undo, so re-reversing an effect a human already removed by hand drops the marker cleanly. The one genuinely new liability is the retained-forever case for a client whose descriptor is gone or that declares no attachProbe (action_attach.js:322-331 fails deterministically): such a marker is now kept and error-logged every pass instead of dropped once. LLP 0250 accepts that trade explicitly, and it is the same trade LLP 0138 #marker-undo already accepted for the asset half. Worth knowing: hyp status renders a field whitelist (status.js:271-282 JSON, :510-525 text), so prior_done surfaces nowhere - which is what the doc claims and I verified, but it also means an operator seeing a permanently-retained openclaw failed marker gets no hint why it is no longer dropped.

Backfill is unaffected.action_backfill.js declares no reverse, so the reverse gap is skipped for that kind entirely; a prior_done written onto a backfill marker is inert.

The LLP 0138 / 0186 edits rewrite nothing. The full diff of both files is one line each, and both are Extended-by: forward-refs (0138's an append onto its existing LLP 0219 (...) entry, 0186 gaining the header line it lacked). Both docs are Status: Active and neither body byte changes.

The src/ diff is surgical: one deleted line in the whole tree, the old drop condition's readInstalledAssets(marker).length === 0).

Style. No em dash anywhere in the diff (checked U+2014, and U+2013/U+2018/U+2019/U+201C/U+201D/U+2192); no trailing semicolons in added JS; the new field is an interface member in types.d.ts with JSDoc, not a @typedef; no inline import('...') types introduced.

Suite baseline note. In a fresh npm install --prefer-offline of a clean worktree of this branch, npm test was fully green (4219 / 4218 pass / 0 fail / 1 skipped) - the ~20 pre-existing query-pushdown failures the body reports did not reproduce for me, though they did for the second reviewer running a different glob against the shared checkout. Either way nothing here is attributable to this PR.


The central.js divergence: safe, leave it

src/core/commands/central.js:497 keeps the sibling shortcut if (!marker || (marker.status === 'failed' && installedAssets.length === 0)), dropping a marker without reversing on the same unsound reading of the evidence (and omitting refused, which happens to fall the safe way). Leaving it is correct here:

  • It is not a regression.central.js on this branch is byte-identical to master, so hyp leave behaves exactly as it does today. This PR takes the number of unsound gates from two to one; it creates no new wrong outcome.
  • hyp leave's assetless-marker drop was never taught about refused markers (#627) #630 deletes it, with this exact rationale. I read that diff: PR hyp leave's assetless-marker drop was never taught about refused markers (#627) #630 (open, MERGEABLE) removes the whole if block at central.js:497-508 and replaces it with a comment naming the same defect ("a marker that reached done, wrote the client's settings and installed no files ... is indistinguishable from one that never applied anything"), routing every marker to detachClientViaCore with quietNoop: true. Teaching that block about prior_done here would collide line-for-line with that removal and send a mergeable, green, reviewed PR back through triage.
  • The two fixes do not interact. After both land, the reconciler still drops only markers that genuinely recorded nothing, which is what LLP 0186 wanted from that gate.

The cost of the window is worth recording: until #630 lands the fix is half-deployed, and hyp leave and a config-drop reconcile disagree about the same marker (leave drops it, the reconciler reverses it). Now that markerRecordsPriorDone() is exported it is a one-clause change if #630 stalls.


Items 1 (a reverse() returning refused) and 4 (hyp detach all narration) of #780 are correctly out of scope, for reasons I agree with: item 1 needs terminal-undo semantics settled and has no caller to test against, and item 4's mechanism (quietNoop) exists only on #630's branch.

Pushed to fix/issue-780: one commit, Review fix: renumber the new decision to LLP 0250, the number is free (c31eaaa7).

@philcunliffe

Copy link
Copy Markdown
ContributorAuthor

Neutral review round 2 of 2: PR #799 (fix/issue-780, reviewed at head c31eaaa7)

Verdict: land it. Round 1's substantive review stands and I am not re-litigating it. Round 2 was owed because the head moved for round 1's renumber, so this round verified the renumber on the branch, re-examined the two deliberate deferrals, and looked for what round 1 missed. One new finding, fixed and pushed; the rest is recorded.


1. The renumber (round 1's fix) is correct and complete - verified on the branch

Checked on a fresh worktree of origin/fix/issue-780 at c31eaaa7, not from round 1's report:

  • git grep 0247 over the whole tracked tree returns nothing. No @ref, no prose, no test name, no forward-ref survives.
  • ls llp/02[45]* shows 0241, 0242, 0243, 0244, 0250. The file is renamed, and its # LLP 0250: title matches.
  • All four @refs read LLP 0250 (action_reconciler.js:235, 274, 339, 549), plus the prose mentions at :335 and types.d.ts:373, the two Extended-by: forward-refs (llp/0138-...:9, llp/0186-...:8), and the @ref and both test names in test/core/action-reconciler.test.js:889, 891, 964.
  • test/core/llp-ref-hygiene.test.js is green (11/11), so both new anchors (#the-bit, #the-drop-condition) resolve and no number is double-claimed.
  • Swept every remote branch for a competing llp/0250-*: only this branch claims it. 0245-0249 remain claimed elsewhere, so 0250 was the right landing spot.

Round 1's residual note stands unchanged and is still a human's call: #797 still claims 0247 for its own doc and additionally double-claims 0246 with origin/integration/proxy-mode-default-attach. Nothing on this PR can fix that; whoever handles #797 should take 0251+.


2. medium (new) - the re-arm's docstring asserted a premise this PR retired (fixed, pushed)

src/core/config/action_reconciler.js, rearmRefusedActionMarker(). The assetless branch justified its outright drop with:

No installed_assets. The marker records nothing that outlives it (an attach refuses before touching the client's settings), so it is dropped outright.

That parenthetical is exactly the reading LLP 0250 was written to retire. It is true of the refusal itself and false of a done pass the refusal was rewritten over: after this PR such a marker can carry prior_done: true, and prior_done is precisely the evidence the reverse gap thirty lines earlier now refuses to drop over. Two paths in one file, reading the same field, documented as if the field did not exist. CLAUDE.md's "keep refs honest" rule bites here: the comment is attached to @ref LLP 0138#marker-undo, and the section it points at is the one LLP 0250 extends.

Fixed in 99130b77: the docstring now states the reason the drop is actually safe (call-site ordering - the re-arm runs only after an explicit hyp attach that already succeeded and rewrote the settings, and a hand-attached client is undone by hyp detach reading the client's own file with no marker), says outright that such a marker may carry prior_done, and names the asymmetry: settings name themselves on disk, org-installed asset paths are named by nothing but the marker, so only the asset half outlives an explicit re-attach. An anchorless @ref LLP 0250 [constrained-by] records that leaving the re-arm reading only the asset half was a decision, not an oversight (anchorless @ref is an established convention in this tree, 47 instances under src/).

Behaviour is unchanged - the diff is 16 insertions / 5 deletions, all inside one JSDoc block, no executable line touched. Verified positively: git show HEAD:src/core/config/action_reconciler.js contains the new text at :633 and the new @ref at :661, and greps 0 occurrences of the retired premise. npm run typecheck clean; npm test4219 tests / 4218 pass / 0 fail / 1 skipped.

This is the editorial correction round 1 asked the author for, relocated from the LLP to the code. I deliberately did not touch LLP 0250's "No effect is stranded" bullet: the document is Status: Accepted, that bullet is what it settled about the re-arm, and CLAUDE.md forbids editing what an Accepted doc decided. The code comment carries no such immunity, and is where a future reader will actually be misled.


3. medium (carried from round 1, still deliberately not fixed) - rearmRefusedActionMarker() drops a marker carrying prior_done

Round 1 recorded the full falsifying chain and I reproduced its reasoning against the current tree; the behaviour is unchanged at c31eaaa7, and it has not become actionable. Two reasons, one stronger than round 1 had:

  • LLP 0250 settles it explicitly. §Consequences: "rearmRefusedActionMarker() is unchanged." The doc is Status: Accepted. Changing the re-arm would require rewriting that bullet, which CLAUDE.md and this round's rules both forbid; the route is a new request, not a review push.
  • The harm is not the harm client attach: probe-less contributes.client can attach but reverse() silently no-ops, orphaning settings #212 is about. Nothing is orphaned in the "no longer named on disk" sense: the settings were just rewritten by the user's own explicit attach, and hyp detach reverses them from the client's file with no marker. What is lost is the reconciler's automatic undo for a client the user hand-attached, and preserving the bit would make an org config-drop silently undo an explicit hyp attach. That is a product tradeoff, not a bug fix.

For the record, one nuance the author may want in the successor request: the with-assets branch already carries an org undo record across an explicit hyp attach (it rewrites to failed with assets intact), so "a client the user attached by hand stays" is not absolute today either. That weakens - it does not overturn - the "No effect is stranded" reasoning round 1 flagged. It is an argument for a follow-up issue, not for overriding a doc accepted one commit ago.

4. low (carried from round 1, not fixed) - markerRecordsPriorDone() does not know the applied marker status

Re-verified on this branch: ActionMarkerStatus still includes 'applied' (types.d.ts:305) and nothing anywhere writes it to a marker (grep "'applied'" over src/, bin/, hypaware-core/ finds only apply.js's unrelated ConfigStageResult.action and a span attribute). The forward gap's short-circuit (:155) and the reverse gap (:343) do not know the state either, so a one-token disjunct in the accessor would create a false impression that applied is supported end to end. Unchanged conclusion: either drop applied from the union or teach the reconciler about it, in its own issue.


5. info - the PR body is stale after the renumber

The body says the doc "took 0247", links llp/0247-marker-records-the-effect-it-overwrites.decision.md, quotes both test names with (LLP 0247), and asserts "No duplicate LLP numbers ... 0247, the next number free across every branch". The branch adds LLP 0250 and both test names read (LLP 0250). Not edited, per this round's rules. Whoever merges should refresh the body first, since it is the narrative the Fixes #780 trail lands on and it currently points at a document that does not exist.


What round 2 additionally checked and cleared

  • The code-review skill targeted the right diff. It reviewed origin/master...pr-799 in its own throwaway worktree, named LLP 0250 and the vacated 0247, and every file it discussed is in gh pr diff 799. Its output was usable and finding 2 above came from it.
  • The forward gap really is the only writer of a terminal marker. I specifically checked the case round 1's phrasing did not spell out: a failed reverse() does not rewrite the marker's status.action_reconciler.js:366-393 keeps the marker byte-identical and only logs, so a settings-only done marker whose reverse fails stays done and is re-reversed next pass. It never degrades into an assetless terminal marker the gate could drop. That was the most plausible remaining hole and it is closed.
  • Backfill is untouched by the new clause. The whole reverse gap is behind typeof reverse === 'function' (:298), and action_backfill.js declares none.
  • Nothing reverses that should not. The change only ever adds a conjunct to a drop condition, so no marker that master reverses stops being reversed, and no marker becomes newly reversible except one that genuinely reached done. The done-marker write path does not carry prior_done forward (:165), so the "two names for one fact" the doc rules out does not sneak in via the success branch.
  • Suite and types.npm run typecheck clean, npm test 4219 / 4218 pass / 0 fail / 1 skipped, on a fresh npm install --prefer-offline. The ~20 pre-existing query-pushdown failures the PR body reports did not reproduce here either, matching round 1.
  • central.js stays out of scope. The skill raised it as low; round 1 settled it and I agree. This branch keeps central.js byte-identical to master, so it is not a regression, and hyp leave's assetless-marker drop was never taught about refused markers (#627) #630 deletes the block outright with this exact rationale. Teaching it prior_done here would collide line-for-line with that removal. The residual window (until hyp leave's assetless-marker drop was never taught about refused markers (#627) #630 lands, hyp leave and a config-drop reconcile disagree about the same marker) is worth an explicit tracking issue if hyp leave's assetless-marker drop was never taught about refused markers (#627) #630 stalls; markerRecordsPriorDone() is exported, so it is a one-clause change.
  • Style. No em dash or other non-ASCII punctuation anywhere in the branch diff (checked U+2014, U+2013, U+2018/19, U+201C/D, U+2192).

Items 1 and 4 of #780 remain correctly out of scope, for the reasons round 1 gave.

Pushed to fix/issue-780: one commit, Review fix: the re-arm's assetless drop states why it is safe, not a premise LLP 0250 retired (99130b77). Round cap reached; the two carried findings above are recorded for a human rather than re-reviewed.

@philcunliffe

Copy link
Copy Markdown
ContributorAuthor

Triage result for this PR at head c9222390619ea6e016d7164169a2b3c293bf0518: every finding still unresolved at this head is non-blocking. The residuals (the rearmRefusedActionMarker() boundary LLP 0250 settles, the unwritten applied member of ActionMarkerStatus, the stale PR body after the 0247 -> 0250 renumber, and the LLP-number race on PR #797) are recorded with evidence in the follow-up issue: #827.

No content of this PR changed after review round 2; the head moved only for a master merge refreshing stale CI. Triage does not merge; this PR remains for a maintainer's decision.

@philcunliffephilcunliffe added the neutral:approved neutral reviewed this and holds it for a maintainer merge (own or adopted PR; LLP 0025/0030) label Aug 18, 2026
@philcunliffe
philcunliffe merged commit 9fc06e7 into masterAug 18, 2026
9 checks passed
@philcunliffe
philcunliffe deleted the fix/issue-780 branch August 18, 2026 19:26
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

neutral:approvedneutral reviewed this and holds it for a maintainer merge (own or adopted PR; LLP 0025/0030)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Follow-up: deferred review findings from PR #630

1 participant

@philcunliffe