Client assets: one materializer, one install command (LLP 0138/0107) - #399
Conversation
platypii
added
the
neutral:adopt
Skills and subagents are two shapes of one thing: a plugin-contributed file tree copied into a client's config directory. They differed only in the copy (directory vs single .md) and in which manifest key names the destination, but shipped as parallel universes: two registries, two commands, and the same copy loop written four times (both CLI commands plus two more inline in the wizard finale). The four had already drifted - the CLI tolerated a failed copy and warned, the finale threw. Collapse them into `materializeClientAssets` (src/core/runtime/client_assets.js), which owns client targeting, containment, idempotent replace, tolerance, and reporting; only the copy branches on kind. `hyp skills install` now installs both kinds, and `hyp agents install` is removed rather than aliased - a hidden second spelling would preserve the confusion it was introduced to fix. This also lands LLP 0107, which was accepted 2026-07-13 and never implemented: `runAttach` dispatched to the adapter and never touched skills, and the reconciler's attach action had no notion of them, so the only path that materialized anything was the wizard finale. A machine enrolled by `hyp remote login`, or a client wired by a hand-run `hyp attach`, got capture and no helper skills - including the hypaware-privacy skill the LLP 0100 review flow depends on. Manual attach and the reconciler's attach now both materialize. Reversal follows LLP 0107 #reversal: an org-driven attach records the destination paths it wrote as `installed_assets` on its action marker and `reverse()` removes exactly those, so a user's own `hyp skills install` copies (which record no marker) survive a leave. What to remove cannot be re-derived from the live registries - they describe what the plugin set contributes now, not what that attach copied - so `ActionHandler.reverse()` receives the marker it is about to drop. A copy failure never fails the attach: the settings write applied, and a `failed` marker would re-attach every pass over a problem re-attaching cannot fix. Two behaviors fall out of the shared routine: a contribution naming the literal client `all` expands to every targeted client instead of warning about a client named "all", and a client with no directory for an asset kind (Codex has skills but no subagents) is a silent skip rather than a warning no one can act on. The finale keeps one `skills.install` span covering both kinds, which the release smoke battery asserts on; `agents.install` is gone.
8efd687 to
8170ec7
Compare
…dashes
Three findings from the review of this PR.
1. `hyp leave` left org-installed client assets on disk forever. Step 3
reversed each org-driven attach through `detachClientViaCore`, which
clears the action marker - and the marker's `installed_assets` was the
only record of what the attach had copied. The assets outlived the
leave with nothing left on disk naming them, which is exactly what
LLP 0107 §reversal says must not happen ("hyp leave removes the skills
they installed, exactly as it reverses settings edits").
`runLeave` now reads `installed_assets` off the marker before the
detach and removes those dests. Removal stays marker-driven, so a
manual `hyp skills install` copy (which records no marker) survives.
`detachClientViaCore` gains a `clearMarker` option, default true: leave
passes false when removal failed, so the undo record survives for a
re-run instead of being destroyed alongside the files it named.
`readInstalledAssets` is exported from action_attach.js rather than
copied, so the reconciler and the CLI cannot disagree about the field.
2. No containment re-check on removal. `removeClientAssets` recursively
force-rm'd every string the marker held, validated only as "non-empty
string". `client-actions.json` is a plain file on disk: one holding
"/" or a home directory was a recursive delete. The write side already
re-checks containment as defense in depth; the delete side has the
weaker input of the two and now does too. `removeClientAssets` takes
the allowed base dirs (`clientAssetBaseDirs`, the descriptor's own
skill_dir/agent_dir under HOME) and reports an escaping dest as failed
rather than removing it - which also keeps the marker for inspection.
3. Two em dashes this PR introduced, against the repo rule.
Tests: leave removes marker-recorded assets while an unmarked manual copy
survives; reverse() refuses a marker path outside the client's asset dirs
(escaping path, the skills dir itself, and "/") and leaves all three alone.
LLP 0138 #marker-undo gains the generalized rule the code now follows:
read the assets before dropping the marker, whoever drops it, and
re-check containment because the field is persisted JSON.
Co-Authored-By: Claude <noreply@anthropic.com>
neutral review round 1:
|
Round-2 review of 4752921 found that round 1 stopped one call site short and that the LLP paragraph it added overclaimed. Fixes, in the reviewer's order: 1. `hyp detach` orphaned org-installed assets. `detachClientViaCore` cleared the attach marker without ever reading `installed_assets`, so detaching on a joined machine reversed the settings and stranded the copies. The read-then-remove moves INTO that routine, next to the clear, so both verbs get it from one place: the duplicated block (and the `clearMarker` option it needed) is gone from `runLeave`. Manual copies record no marker and still survive. 2. `hyp leave`'s plugin-gone branch destroyed the undo record silently. Refusing to remove without a descriptor is right (no base dirs to bound a recursive rm), but the marker was the only thing naming those files, so it now prints them before dropping it. 3. A failed re-`perform()` dropped `installed_assets` when rewriting the marker, so a later reversal removed nothing. The field is carried forward, and `runLeave`'s "a failed marker never applied an effect" comment (now false for attach) is corrected: such a marker takes the normal reversal path. The reconciler's own reverse gap still drops a failed marker without reversing it; that is left open on purpose and documented, not papered over. 4. One HOME fallback on the removal side: `reverse()` used `ctx.env.HOME ?? ''` while the CLI undo used `?? os.homedir()`, the exact drift 4752921 set out to prevent. And an empty `baseDirs` now reports "no asset directories resolved" instead of blaming containment for a cause that is not containment. 5. LLP 0138 #marker-undo re-stated against what the code now does: the rule binds whoever drops the marker, degrades to naming the paths where removal is impossible, and names the one drop that still escapes it. Tests: `hyp detach` removes exactly the marker's assets while an unmarked manual copy survives, and keeps the marker when a recorded path cannot be removed. 2652 -> 2654 passing, tsc clean. Co-Authored-By: Claude <noreply@anthropic.com>
A machine-specific symlink (node_modules -> /work/hypaware/node_modules) was committed with the round-2 review fixes. .gitignore lists 'node_modules/' with a trailing slash, which matches a directory but not a symlink of the same name, so it was never ignored. Anyone checking out this branch would get a dangling symlink into a path on the machine that pushed it. Removing it; no source change.
neutral review round 2:
|
philcunliffe
added
the
neutral:changes-requested
philcunliffe
left a comment
philcunliffe
left a comment
There was a problem hiding this comment.
Neutral reviewed this PR twice at 2e203af and has reached its two-round limit, so it stops editing the branch here. The verdict is non-binding: you and the maintainer decide what happens next.
What neutral already fixed and pushed (details in the two review comments above): hyp leave and hyp detach both reverse the client assets their attach marker records, with a containment re-check on the recursive delete; the undo record survives a failed removal and a failed re-perform(); the plugin-gone branch names the files it cannot remove; and LLP 0138 #marker-undo now matches the code. Tests added for each. CI is green at this head.
Three things are yours to decide. None are defects neutral can settle without knowing your intent:
-
Undo semantics on a containment refusal. A recorded path that fails containment makes
hyp leaveexit 1 and keep the marker, and the message promises a re-run will retry. But that refusal is deterministic, so it fails identically forever with no escape but hand-editingclient-actions.json. Meanwhile the retained marker still readsstatus: doneafter its settings effect was reversed, which is the stale-marker state blocking re-attach after a laterhyp join(#217). An I/O failure is retryable and a containment refusal is not, and they probably should not share a message or a resolution. The reconciler's reverse gap, where afailedmarker carrying assets can still be dropped, is the same call. -
LLP 0107 §currency. This PR marks LLP 0107 implemented, but
isCurrentcompares onlymarker.endpoint, so a plugin the org adds later never re-materializes. That is exactly the scenario §currency promises and for which a login one-shot was rejected. Either add an asset-set signal toisCurrent, or narrow theExtended-by:claim and record in LLP 0138 that §currency is still open. -
hyp attachon a daemon-managed install. It still short-circuits at "already attached" before materializing, so it installs nothing. You raised this yourself in the PR body and asked for a call.
One error of neutral's own, flagged in the round-2 comment and already reverted: a node_modules symlink to an absolute path on the machine running neutral was briefly committed to this branch. .gitignore lists node_modules/ with a trailing slash, which does not match a symlink of that name.
Reply on this thread with your decisions and neutral will pick them up on its next tick.
…tics, attach short-circuit) A. A removal that failed and one that was refused are different outcomes. removeClientAssets marks each failure retryable or not: an fs.rm error keeps the marker and fails the command, a containment refusal names the paths and lets the marker go. Keeping a marker for a refusal made the undo permanently unfinishable and left the stale done marker that blocks re-attach (#217). Same split in reverse(). This also closes the reconciler's reverse gap: a failed marker carrying installed_assets now reverses instead of being dropped, which is safe now that a reverse which can never succeed completes. B. LLP 0107 currency is implemented, not narrowed. perform() records assets_key, a sorted digest of the planned copies, and isCurrent compares it beside the endpoint, so a plugin the org adds later re-materializes at an unchanged port. planClientAssets() is split out of materializeClientAssets() so the digest and the copy read one loop. C. hyp attach materializes before the already-attached short-circuit, so a daemon-managed install no longer installs nothing. readInstalledAssets moves to action_reconciler.js beside the marker store; the reverse gap needs it, and the generic reconciler should not import a specific handler.
neutral is stuck: its review budget is spent, and its next mechanical act would be a false signalWhat neutral was doing. Driving this adopted PR ( Why it cannot proceed. The rung the CLI computes for the new head is It also cannot approve. Its two-round budget is exhausted, so it has not reviewed What neutral did verify at
That is symbol-level confirmation that each item was addressed. It is not a judgment that the implementations are correct, and the roughly 625 added lines and four new test files have had no review pass. What neutral needs from you. One of:
Neutral has removed the now-inaccurate Worth flagging separately: the round counter being per-PR-lifetime rather than per-head-lineage is arguably a tooling bug. Every future contributor push to a cap-exhausted PR lands in this same position, where the only mechanical move available is a false one. How to unstick. Reply with a comment on this PR, or push to the branch. Neutral monitors this thread and will re-engage with your guidance on its next tick. |
philcunliffe
added
neutral:stuck
platypii
removed
the
neutral:stuck
neutral review round 3:
|
philcunliffe
added
the
neutral:changes-requested
…reverse-gap claim The blocking defect: the reconciler rebuilds a `done` marker from the outcome detail alone, and perform() reports what that pass copied. The two differ once the desired set shrinks, so an attach re-fired by a withdrawn plugin rewrote the marker with the shorter list while the withdrawn files stayed on disk with nothing naming them. The `failed` path already carried the field forward; the `done` path now unions it, in the same one place, for every handler. Covered by a shrink-direction currency test, which fails without the union. The previous commit message, a code comment, and LLP 0138 all claimed closing the reverse gap was safe because a reverse that can never succeed now completes. That is false: reverse() fails deterministically when the descriptor is gone or has no attach_probe, neither of which this branch touched. Such a marker now retries every pass rather than being dropped once. That is the accepted trade, not an oversight, and it is written down as one: a retained `failed` marker is visible and does not block re-attach, whereas dropping it destroys the record of files that are really there. Only the asset half degrades to naming-and-releasing; the settings half cannot. Also documents the currency limit: the digest covers the asset set, not its bytes, so an in-place skill rewrite under an unchanged set does not re-fire.
philcunliffe
added
neutral:approved
neutral verdict at
|
perform() inlined the same params-or-request-key resolution the new attachActionClient() helper exists to own. No behavior change; the emptiness guard stays, since it never was part of the resolution.
philcunliffe
added
the
neutral:adopted
…hold (LLP 0101/0100)
An onboarding session could not show data on the server. The enrolling
login writes a first-sync hold marker (LLP 0101), and the driver returns
`{ sinks: [], held: 'first_sync_hold' }` before it ever consults
`force: true`, so `hyp sink force` printed "no sinks instantiated;
nothing to do" - which is the opposite of the truth - and nothing could
end the window early. On an evening enrollment the 4-hour floor rolls the
deadline to the *next* day's 11:59pm, so "wait until midnight" could mean
28 hours.
LLP 0101 #no-release rejected exactly this verb, for three reasons. Two
survive: the printed message stays unconditionally true (reworded to "no
later than"), and release-on-review-completion still needs a completion
signal from the skill that a finished skill cannot honestly give. The
third - "costs only latency on a machine that was not forwarding
yesterday either" - is what this concedes. On an attended onboarding the
hold blocks the demonstration that the product works, and the window is
sized for a review (hypaware-privacy) too slow to run in the meeting the
hold was scoped to. #which holds the attended lane while unattended
`hyp join` forwards immediately, which is backwards for that case.
LLP 0069 R6 ("not forwarded, even once") is untouched: R6 forbids a
*silent* first forward, and an explicit confirmation naming the
destination is the consent it exists to obtain. The daemon never clears
the marker, so an unattended machine still waits out the full window.
`hyp sync [instance] [--yes] [--dry-run]` replaces `hyp sink force`
rather than aliasing it: both were one `driver.tick({force: true})`, and
a second spelling preserves the confusion it was introduced to fix
(the #399 doctrine). It always confirms - one rule beats a prompt whose
appearance the user has to predict - and prints what it is about to send
first: every destination named, annotated off-machine only on the
evidence of an http(s) URL and on-machine only on the evidence of a path,
plus the directories being withheld. "Are you sure?" with nothing to be
sure about is a keystroke, not a decision. `hyp sink maintain` stays;
snapshot expiration is not an export tick.
The three surfaces that print the deadline (login message, wizard privacy
narration, `hyp status`) now name the verb, and the formatted deadline
carries its time zone - ECMA-402 forbids mixing timeZoneName with
dateStyle/timeStyle, so the zone comes from a second formatter.
`purge` and `report delete` had grown one copy each of the same y/N
prompt; all three now share src/core/cli/confirm.js with their question
text unchanged.
LLP 0101 #no-release is amended with the original preserved as a quote,
and LLP 0100 R2 ("No export tick may run before the deadline") with it -
it was flatly false once this lands. R1 now requires the zone and the
release verb.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…hold (LLP 0101/0100) (#419) * hyp sync: one export verb, and a confirmed way out of the first-sync hold (LLP 0101/0100) An onboarding session could not show data on the server. The enrolling login writes a first-sync hold marker (LLP 0101), and the driver returns `{ sinks: [], held: 'first_sync_hold' }` before it ever consults `force: true`, so `hyp sink force` printed "no sinks instantiated; nothing to do" - which is the opposite of the truth - and nothing could end the window early. On an evening enrollment the 4-hour floor rolls the deadline to the *next* day's 11:59pm, so "wait until midnight" could mean 28 hours. LLP 0101 #no-release rejected exactly this verb, for three reasons. Two survive: the printed message stays unconditionally true (reworded to "no later than"), and release-on-review-completion still needs a completion signal from the skill that a finished skill cannot honestly give. The third - "costs only latency on a machine that was not forwarding yesterday either" - is what this concedes. On an attended onboarding the hold blocks the demonstration that the product works, and the window is sized for a review (hypaware-privacy) too slow to run in the meeting the hold was scoped to. #which holds the attended lane while unattended `hyp join` forwards immediately, which is backwards for that case. LLP 0069 R6 ("not forwarded, even once") is untouched: R6 forbids a *silent* first forward, and an explicit confirmation naming the destination is the consent it exists to obtain. The daemon never clears the marker, so an unattended machine still waits out the full window. `hyp sync [instance] [--yes] [--dry-run]` replaces `hyp sink force` rather than aliasing it: both were one `driver.tick({force: true})`, and a second spelling preserves the confusion it was introduced to fix (the #399 doctrine). It always confirms - one rule beats a prompt whose appearance the user has to predict - and prints what it is about to send first: every destination named, annotated off-machine only on the evidence of an http(s) URL and on-machine only on the evidence of a path, plus the directories being withheld. "Are you sure?" with nothing to be sure about is a keystroke, not a decision. `hyp sink maintain` stays; snapshot expiration is not an export tick. The three surfaces that print the deadline (login message, wizard privacy narration, `hyp status`) now name the verb, and the formatted deadline carries its time zone - ECMA-402 forbids mixing timeZoneName with dateStyle/timeStyle, so the zone comes from a second formatter. `purge` and `report delete` had grown one copy each of the same y/N prompt; all three now share src/core/cli/confirm.js with their question text unchanged. LLP 0101 #no-release is amended with the original preserved as a quote, and LLP 0100 R2 ("No export tick may run before the deadline") with it - it was flatly false once this lands. R1 now requires the zone and the release verb. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Review round 1: the hold releases only on unscoped, attended consent Six findings from neutral's review of e25b45b. 1. BLOCKER. `hyp sync <instance>` released the driver-wide hold after a plan built from the one named handle. `hyp sync parquet` on a machine with a central sink showed "parquet /home/u/exports (stays on this machine)", and a `y` there ended the window for `central` too - the next daemon tick then forwarded the whole backfilled history with nothing on screen. That is the silent first forward the hold exists to prevent (LLP 0101 #hold: "one missed forward hold is a permanent leak"), and it falsified this PR's own headline property. A scoped run now refuses while a hold is live and says why. Releasing is all-or-nothing because the hold is; rendering the full destination list from a command that ticks one sink would have been the other option, but "I asked for one sink and it released everything" is a surprise the refusal avoids entirely. 2. A failed unlink turned a confirmed sync into a silent exit 0: the marker survived, the driver held the tick, and the loop over `report.sinks` printed nothing. That is the defect this PR opens by describing, reintroduced one path over. The clear is now checked, the marker re-read, and `report.held` inspected after the tick; each is an error with the marker path, not a quiet success. 3. The release emitted no telemetry, and clearing the marker destroys the only on-disk evidence it happened, so nothing afterwards distinguished "the window expired" from "somebody ran hyp sync". Logs `sync.first_sync_hold_released` with the deadline, how early it was, and the destination counts. 4. `--yes` released a live hold, which the clause titled "Release only by confirmed, attended request" does not license. It now refuses while held. `--yes` still works for ordinary syncs; what it must not buy is somebody's review window. 5. Three prose surfaces still said the deadline was the only way the window ends: README, and the hypaware-privacy skill in both the claude and codex copies - the skill being the surface LLP 0100 designates for the review itself, telling a user mid-review that their window cannot end early. 6. The plan printed the raw server URL that R1a was landed to remove. R1a binds the login's surfaces by its text, but its reason is about terminals, and this prompt appears at the same onboarding moment: servers are now named from `query.remotes` by origin, with the `hyp remote list` pointer, falling back to the host when no name is configured. Both refusals run before the plan renders: a scoped plan is the misleading artifact finding 1 is about, and printing "syncing now ends it early" ahead of "you cannot end it this way" reads as a contradiction. `--dry-run` is exempt from both - it sends nothing, so showing a held machine what one destination would export is information, not consent. LLP 0101 #no-release and LLP 0100 R2 record the two shapes that cannot release and why, so the constraint is not left living in code comments. Tests: the instance-plus-hold combination the review identified as never exercised, plus --yes-plus-hold, an unclearable marker, and the two naming paths. 16 in the file, 2760 in the suite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
Skills and subagents are two shapes of one thing: a plugin-contributed file tree copied into a client's config directory. They differ in the copy (a directory tree vs a single
.md) and in which manifest key names the destination. Nothing else. They shipped as parallel universes anyway: two registries, two commands, and the same copy loop written four times (both CLI commands plus two more inline in the wizard finale). The four had already drifted - the CLI tolerated a failed copy and warned, the finale threw; the CLI warned about a client with no directory for the asset kind, the finale skipped silently.Along the way this lands LLP 0107, which was accepted 2026-07-13 and never implemented.
One materializer
New
src/core/runtime/client_assets.js.materializeClientAssets()owns client targeting, containment, idempotent replace, tolerance of a bad contribution, and what gets reported; only the copy itself branches on kind.removeClientAssets()is the reversal half. Every caller routes through it:hyp skills install, the wizard finale, manual attach, and the reconciler's attach action.One command
hyp skills installinstalls both kinds.hyp agents installis removed, not aliased - a hidden second spelling preserves the confusion it was introduced to fix. The registries stay two (plugins register two shapes); only the user-facing surface unifies.Attach installs, as LLP 0107 said
runAttachdispatched to the adapter'sattach()and never touched skills; the reconciler's attach action had no notion of them. The only path that materialized anything was the wizard finale. So a machine enrolled byhyp remote login, or a client wired by a hand-runhyp attach claude, got capture and no helper skills - including thehypaware-privacyskill the LLP 0100 review flow depends on.Both attach paths now materialize.
hyp skills installremains the manual path for re-copying after a local edit, without re-attaching.Reversal
Per LLP 0107 §reversal: an org-driven attach records the destination paths it wrote as
installed_assetson its action marker, andreverse()removes exactly those - so a user's ownhyp skills installcopies, which record no marker, survive a leave. What to remove cannot be re-derived from the live registries (they describe what the plugin set contributes now, not what that attach copied), soActionHandler.reverse()now receives the marker it is about to drop. Backfill is unaffected; attach is the only implementor.A copy failure never fails the attach: the settings write applied, and a
failedmarker would re-attach on every pass over a problem re-attaching cannot fix.Incidental improvements
all(legal perPluginSkillClient) now expands to every targeted client, instead of warning about a client named "all".Notes for review
ActionContext/ReconcileInputcarry the skill and agent registries, threaded by the daemon fromboot.runtime. Absent on a CLI boot, so the install half of attach is inert by construction, like the rest of the client seam.skills.installspan covering both kinds (the release smoke battery asserts it);agents.installis gone.installed_assets, so a leave that reverses it removes nothing - same outcome as a manual install. Self-healing: the next attach records the field.hyp attachon a daemon-managed install that is already attached at the live port still short-circuits with "nothing to do" and installs nothing. The reconciler covers that machine, so it is not a gap, but say the word if the manual command should always ensure assets.Checks
npm run typecheckcleannpm test: 2650 pass, 0 failwalkthrough_picker_to_first_query,client_attach_idempotent,package_bin_boot,cli_bundled_plugins_activated,status_diagnostics,daemon_foreground_start_stopall greenDesign rationale: LLP 0138, with an
Extended-by:forward-ref added to LLP 0107.