Four findings surfaced across two review rounds on PR #585 (integration/attach-prompts-to-enable, head 2943865) were triaged as non-blocking and deferred so that PR could ship. Tracking them here.
Three other findings from those rounds were fixed in the PR itself and are not tracked here: the --dry-run blocker, and two shapes of "the enable prompt offers what the additive write cannot deliver".
1. not_enabled error copy is wrong for three distinct states
File: src/core/commands/clients.js, resolveAttachEnablementState (~line 490-539)
classifyInactiveState() at ~line 515 is called with only descriptor.plugin (the adapter's own name), never the dependency closure. Three states therefore collapse into the same generic message, "add @hypaware/<plugin> to <configPath> and run hyp daemon restart":
- (a) entry present but
enabled: false locally (disabled-local)
- (b) adapter present and enabled, but a dependency (
@hypaware/ai-gateway) is disabled. The message names the wrong plugin.
- (c) all requested names present and enabled, but the plugin is not live in-process (e.g. an
activate() that threw at boot)
In every case the user is told to add something already in the file. enableWriteCannotDeliver (~line 850) correctly refuses to prompt in all three (it checks the full pluginNames closure), so the flow falls through to this stale message.
Why this was deferred rather than fixed: the same message also names hyp init, which does correctly repair all three states, so users are under-served rather than stuck. The text is settled by LLP 0174 / 0178, so changing it is a design change.
What a fix needs: mirror the existing precedent at src/core/cli/dispatch.js:266-278, which already branches disabled-central / disabled-local / absent into three distinct repair lines from the same classifyInactiveState() output. Additionally classify state (b) by checking the full dependency closure (pluginNames from resolveSingleSourceEnablement) rather than just descriptor.plugin, so the message names the plugin that is actually disabled. Wants a new LLP extending 0174.
2. Non-resolving @ref anchor: LLP 0139#seam-fresh-activation
Files: src/core/cli/dispatch.js:798 and :903-905 (new on that branch); pre-existing at dispatch.js:778 on master.
llp/0139-desktop-picker-consent.decision.md has no heading that slugs to seam-fresh-activation (only ## Context / ## Decision / ## Consequences). Zero runtime effect; master already carries the same break, so the branch is consistent with existing state rather than worse.
What a fix needs: a 4-site sweep. Either add the anchor to LLP 0139, or retarget all four @ref sites to the correct heading.
3. Activation-order drift in activatePluginDependencyClosure
File: src/core/cli/dispatch.js, activateSeamCommandPlugins (810-869) calling activatePluginDependencyClosure (907-1009)
activateSeamCommandPlugins previously bailed before calling activatePlugins when the command's owner plugin was eliminated by dependency resolution. The extracted closure now activates surviving members (line 989) before the caller's owner-check guard (line 847) runs, so closure members can be activated even when the owner never activates, and the dispatch.seam_activate success log (line 849) is skipped on that path because the early return happens first.
No correctness impact identified: every plugin activated this way is one the effective config already selects, so the eventual runtime state matches what boot would produce. Only reachable when resolution eliminates the owner but not its closure.
What a fix needs: either restore the pre-check, or add a telemetry line for the "activated some, not the owner" case so the path is observable.
4. Environment-dependent test fixture
File: test/core/attach-enable-resume.test.js:79-100 (installFakeDaemonService), used at ~line 206
The test relies on restartServiceDaemon() genuinely throwing (spawn ENOENT) because CI has no systemctl/launchctl reachable. Documented and deliberate, and it does make the failure real rather than simulated, but the test's meaning changes on a host that has a real service manager.
What a fix needs: make the "no service manager" precondition explicit (skip or adapt assertions) rather than implicit in the CI environment.
Deferred from PR #585 by triage after the review-round cap.
Four findings surfaced across two review rounds on PR #585 (
integration/attach-prompts-to-enable, head2943865) were triaged as non-blocking and deferred so that PR could ship. Tracking them here.Three other findings from those rounds were fixed in the PR itself and are not tracked here: the
--dry-runblocker, and two shapes of "the enable prompt offers what the additive write cannot deliver".1.
not_enablederror copy is wrong for three distinct statesFile:
src/core/commands/clients.js,resolveAttachEnablementState(~line 490-539)classifyInactiveState()at ~line 515 is called with onlydescriptor.plugin(the adapter's own name), never the dependency closure. Three states therefore collapse into the same generic message, "add@hypaware/<plugin>to<configPath>and runhyp daemon restart":enabled: falselocally (disabled-local)@hypaware/ai-gateway) is disabled. The message names the wrong plugin.activate()that threw at boot)In every case the user is told to add something already in the file.
enableWriteCannotDeliver(~line 850) correctly refuses to prompt in all three (it checks the fullpluginNamesclosure), so the flow falls through to this stale message.Why this was deferred rather than fixed: the same message also names
hyp init, which does correctly repair all three states, so users are under-served rather than stuck. The text is settled by LLP 0174 / 0178, so changing it is a design change.What a fix needs: mirror the existing precedent at
src/core/cli/dispatch.js:266-278, which already branchesdisabled-central/disabled-local/absentinto three distinct repair lines from the sameclassifyInactiveState()output. Additionally classify state (b) by checking the full dependency closure (pluginNamesfromresolveSingleSourceEnablement) rather than justdescriptor.plugin, so the message names the plugin that is actually disabled. Wants a new LLP extending 0174.2. Non-resolving
@refanchor:LLP 0139#seam-fresh-activationFiles:
src/core/cli/dispatch.js:798and:903-905(new on that branch); pre-existing atdispatch.js:778on master.llp/0139-desktop-picker-consent.decision.mdhas no heading that slugs toseam-fresh-activation(only## Context/## Decision/## Consequences). Zero runtime effect; master already carries the same break, so the branch is consistent with existing state rather than worse.What a fix needs: a 4-site sweep. Either add the anchor to LLP 0139, or retarget all four
@refsites to the correct heading.3. Activation-order drift in
activatePluginDependencyClosureFile:
src/core/cli/dispatch.js,activateSeamCommandPlugins(810-869) callingactivatePluginDependencyClosure(907-1009)activateSeamCommandPluginspreviously bailed before callingactivatePluginswhen the command's owner plugin was eliminated by dependency resolution. The extracted closure now activates surviving members (line 989) before the caller's owner-check guard (line 847) runs, so closure members can be activated even when the owner never activates, and thedispatch.seam_activatesuccess log (line 849) is skipped on that path because the early return happens first.No correctness impact identified: every plugin activated this way is one the effective config already selects, so the eventual runtime state matches what boot would produce. Only reachable when resolution eliminates the owner but not its closure.
What a fix needs: either restore the pre-check, or add a telemetry line for the "activated some, not the owner" case so the path is observable.
4. Environment-dependent test fixture
File:
test/core/attach-enable-resume.test.js:79-100(installFakeDaemonService), used at ~line 206The test relies on
restartServiceDaemon()genuinely throwing (spawnENOENT) because CI has nosystemctl/launchctlreachable. Documented and deliberate, and it does make the failure real rather than simulated, but the test's meaning changes on a host that has a real service manager.What a fix needs: make the "no service manager" precondition explicit (skip or adapt assertions) rather than implicit in the CI environment.
Deferred from PR #585 by triage after the review-round cap.