The stranded-attach warning repeats where the run ends (#614) - #625
The stranded-attach warning repeats where the run ends (#614)#625philcunliffe wants to merge 7 commits into
Conversation
Re-running `hyp init` and unchecking a previously attached client regenerates a config without that client's adapter while the client's own settings still route through the HypAware gateway. The finale only attaches, and the action reconciler's reverse lane undoes only config-named (org/central) keys, so a wizard attach on the local layer is never reversed: capture stops and the client's requests can fail. Picking only OpenTelemetry composes no gateway at all, and every still-attached client points at a dead port after the finale's restart. The finale now names the clients it left stranded (attach marker on disk, not picked, enabled by neither the written config nor the central layer) and prints the `hyp detach --client <name>` that clears each. It does not detach: rewriting a client's settings file is not what unchecking a picker row asked for, and that call is the maintainer's to make (LLP 0185). `hyp status` gains the matching `client_attached_not_configured` diagnostic as the after-the-fact backstop, gated to solo hosts because on a joined host that shape is a reconciler pass that has not run yet. The org/central attach markers are untouched throughout. Co-Authored-By: Claude <noreply@anthropic.com>
…604) Three defects in the #604 fix, found in neutral review. An unreadable local config emptied the active-plugin set for a reason that has nothing to do with what the operator enabled, and the new `client_attached_not_configured` branch read that as "nothing enables this adapter": `hyp status` answered a parse failure with a `hyp detach` for every attached client, stacked on top of the `config_unreadable` error that is the actual repair. The branch now stands down when the local layer is present but did not parse. `findAttachedNotConfiguredClients` counted a plugin entry with `enabled: false` as configured, while `hyp status` builds the same set from `activePlugins`, which skips it. A switched-off adapter collects exactly as little as an absent one, so the two surfaces disagreed about the same file. LLP 0185 records both gates, and the forward-refs the corpus convention wants land on the docs it extends: LLP 0180 (the finale's attach lane) and LLP 0086 (the client drift diagnostic family). The status test's `@ref` carried an empty gloss; it now says what it tests. Tests: an unreadable local config draws no detach advice; a disabled plugin entry strands its client; and the central-layer suppression LLP 0185 #scope promises for the finale, which had no coverage on the walkthrough side. Both new gates fail without the src change. npm test: 3386 tests / 3385 pass / 0 fail / 1 skipped. npm run typecheck clean. Smokes status_diagnostics and walkthrough_picker_to_first_query ok. Co-Authored-By: Claude <noreply@anthropic.com>
…ad unfiltered (#604) The status gate stands down on a local layer that is present but does not parse, and deliberately does not stand down when there is no config at all: nothing collects then, so a marker still on disk really is stranded. That boundary (`errorKind !== 'config_missing'`, not `!ok`) had no test, so widening the gate would have dropped the case silently. Adds one. `readCentralPluginNames` is deliberately not filtered by `enabled`, unlike the local set the caller builds: LLP 0185 #scope settles the org lane on the name alone, and filtering would hand the operator a detach for an org-owned client. Says so at the read, and scopes #not-configured-means-not-active to the local layer so the two sections cannot be read against each other. Co-Authored-By: Claude <noreply@anthropic.com>
The finale names the clients this run left attached but no longer collects, before the daemon restart (LLP 0185). `runInitWizard` then writes the run summary, a first look that is roughly sixty lines of real query output, and on the team path the privacy narration, none of which pause, so on a real terminal the warning is gone by the time setup ends. It is buried worst where it is the only signal there is: on a managed host `hyp status`'s mirror diagnostic is gated to hosts with no central layer. The wizard now repeats it, short, after the first look and before the privacy narration. The repeat is the caller's rather than the shared `writeWalkthroughRunSummary`'s, because only the caller knows whether anything came between: `runPickerWalkthrough` writes a short summary and stops, so it keeps its single print instead of showing the same paragraph twice within a few lines of itself. LLP 0188 records the print contract and carries the forward-ref on LLP 0185. Co-Authored-By: Claude <noreply@anthropic.com>
The gate was `firstLookRan || pathway === 'team'`. A `pathway` is only ever resolved on an interactive run, so a team run that is neither cancelled nor a dry run already has `firstLookRan`; the disjunct added exactly the runs where the first look did *not* run. Those are the runs that wrote nothing between the finale and here, so on a team run cancelled at the backfill consent the repeat landed three lines under the finale's own full warning, which is the same double print the shared run summary was rejected for. The gate is now `firstLookRan` alone. LLP 0188 #when says so and says why, and a new test pins the cancelled team run (fails on the old gate). Co-Authored-By: Claude <noreply@anthropic.com>
Neutral review, round 1 of 2Verdict: approve after the one fix below, which is pushed. Reviewed as Finding 1 (medium, fixed): the
|
…mpting `firstLookRan` says the closing block was attempted. It is documented to degrade to a silent skip rather than fail a finished install (LLP 0135 #first-look), so an unregistered `ai_gateway_messages` dataset, an unreadable cache, or a render that throws leaves an attended run that wrote nothing between the finale's warning and the closing repeat. Probed against the real orchestrator with no overview dataset: stdout was the config-write line and the repeat, with the finale's own six-line warning directly above it in production. That is the same-screen double print LLP 0188 says no path produces, reached through the first look's designed degradation instead of through the pathway disjunct round 1 removed. Gate on the result: `runWizardFirstLook` already returns `shown`, which is exactly "the block reached the screen". LLP 0188 §when restated to match (the condition is the closing sequence having *written* something), and the `**Extends:**` header folded into `Related:`, which is how the corpus's other 167 docs name what they extend. Co-Authored-By: Claude <noreply@anthropic.com>
Neutral review, round 2 of 2Verdict: approve after the one fix below, which is pushed ( Finding 1 (low, fixed):
|
PR #608 was squash-merged as 6ab0d99, so this branch's copies of its commits conflicted with the same content under new SHAs. Every conflicted hunk that was #608's takes master; the branch keeps only its own work (LLP 0188, writeAttachedNotConfiguredReminder, the wizard's firstLookShown gated repeat, and its tests). Co-Authored-By: Claude <noreply@anthropic.com>
philcunliffe
added
the
neutral:stuck
|
Neutral triage on What I was checkingThe task was to re-verify, with the same skepticism that caught the first two Conflict resolution: clean
|
1 participant
Base note: this is stacked on #608, not on master
Issue #614 is a deferred review finding about PR #608, which is still open
and unmerged. Every symbol the finding names (
FinaleSummary.attachedNotConfigured,findAttachedNotConfiguredClients, the finale warning, LLP 0185) exists only onfix/issue-604. Basing this onmasterwould have meant re-carrying #608's threecommits under a second PR number, so it is stacked instead, matching the pattern
this repo already uses (#611-#621 on
integration/reconciler-refused-marker).Merge #608 first; this then rebases onto master cleanly. The diff below is just
this change.
Root cause
runPickerFinaleprints the stranded-attach warning and returns. What happensnext depends entirely on which entry point called it, and nothing in the code
accounted for that:
runPickerWalkthroughwrites its short run summary and stops. The warning iswithin a handful of lines of the end.
runInitWizardwrites the run summary, thenrunWizardFirstLook(roughlysixty lines of real query output, by its own design in LLP 0135 #first-look),
then on the team path the privacy narration. Nothing pauses between them, so
on a real terminal the warning has scrolled away by the time
hyp initfinishes, and a user who does not scroll back never reads it.
writeWalkthroughRunSummarynever readfinaleSummary.attachedNotConfigured,which is what the issue reports, but that is the symptom rather than the cause:
the cause is that the finale's placement is correct for the daemon restart and
wrong for the end of the run.
It is buried worst exactly where it matters most. On a managed host
hyp status's mirror diagnostic (client_attached_not_configured) is gated to hostswith no central layer (LLP 0185 #status-backstop), so the finale's print is the
only signal a wizard-attached client the reconfigure run left unpicked ever
gets. A joined host has no backstop to scroll back to.
The fix
The finale's print stays exactly where LLP 0185 put it, after the config
write and before the daemon restart.
runInitWizardadditionally writes a shortrepeat after the first look and before the privacy narration, naming the same
clients and the same
hyp detach --client <name>lines.writeAttachedNotConfiguredReminder(new export,src/core/cli/walkthrough.js) is the compact form: names plus one repaircommand each, with the same dry-run tag rule. The finale's own
writeAttachedNotConfiguredWarning, which explains the consequence in full,stays private and unchanged.
src/core/cli/wizard/index.jscalls it only when its closing sequenceactually ran: the first look ran (attended, uncancelled, not a dry run) or the
pathway is
team. A scripted--yes/--dry-runwizard run writes nothingbetween the finale and its summary, so its output is unchanged.
narratePrivacyIfTeamPath, so the privacy narration isstill the wizard's last words (LLP 0135, LLP 0100/0101). Seven lines of
narration is a reminder that survives on one screen; sixty lines of query
output is not, and that is the difference this turns on.
The double-print hazard, and how it is avoided
Round 2 of #608's review flagged that echoing
attachedNotConfiguredfromwriteWalkthroughRunSummarywould print the warning twice within a few lines ofitself on the
runPickerWalkthroughpath, where the summary follows the finalewith nothing in between. That function is shared by both entry points, so the
naive echo is wrong twice over: it double-prints on one path and still lands
before the first look on the other, which is the complaint it was meant to fix.
So
writeWalkthroughRunSummaryis untouched. The repeat belongs to the callerthat buried the first print, because only the caller knows whether anything
came between.
runPickerWalkthroughdoes not repeat;runInitWizarddoes, andonly when its own closing output ran. No path prints the warning twice on one
screen.
This is a print-contract change to an Accepted LLP, so it is recorded as an
extension rather than an edit: LLP 0188 (new, Accepted) with an
Extended-by:forward-ref and an inline note added to LLP 0185§status-backstop. LLP 0185's decision itself is unchanged.
Ground truth
New regression test,
test/core/cli/wizard/index.test.js:runInitWizard: an attended run repeats the stranded-attach warning after the first look.Pre-fix (
git stash push -- src/, thennode --test test/core/cli/wizard/index.test.js):Post-fix, plus the double-print guard:
Two more tests pin the hazard rather than the feature, and pass on both sides
because they assert what must not change:
runInitWizard: a scripted run does not repeat the stranded-attach warning(no closing sequence, no repeat).
the finale names a still-attached client the new config no longer collectsnow counts occurrences: exactly onehyp detach --client codexand oneStill attached, no longer collectedonthe
runPickerWalkthroughpath. That assertion fails under the naiveshared-summary echo.
Full suite (
npm test):# tests 3389 / # pass 3388 / # fail 0 / # skipped 1.npm run typecheck: clean.Fixes #614