Skip to content

Login: wait for reconcile and report real capture state - #259

Merged
platypii merged 2 commits into
masterfrom
fix/login-capture-message-stale
Jul 7, 2026
Merged

Login: wait for reconcile and report real capture state#259
platypii merged 2 commits into
masterfrom
fix/login-capture-message-stale

Conversation

@platypii

@platypiiplatypii commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Problem

After enrollment, hyp remote login printed this unconditionally and synchronously - right after provisioning the sink, before the daemon's first config pull:

nothing is captured yet - run 'hyp attach <client>' to start

Two defects:

  1. It asserted a transient pre-reconcile state as terminal. The daemon pulls the org config moments later and, on any org that publishes one, auto-attaches its clients (LLP 0044). The claim was false within a poll cycle.
  2. It pushed manual work the daemon was about to do itself. LLP 0063 §login-config-pull only ever scoped that hint as interim, "until the server follow-up ships." Org default config (server LLP 0043) has now shipped and is deployed, so the interim premise no longer holds.

This repeatedly fooled operators into thinking a correctly configured, actively capturing fleet was broken. It also printed noisy internals (provisioned '@hypaware/central' sink 'central' - ...).

Fix

Two changes:

  1. Trim the noise. The sink line is now just forwarding logs to <url>.
  2. Stop guessing - wait and report the truth. After the daemon installs, the login command waits for its first reconcile (polling the on-disk attach markers via collectHypAwareStatus, a cross-process read) and reports ground truth:
    • capturing claude, codex once clients attach, or
    • no clients attached yet - check 'hyp status', or run 'hyp attach <client>' to capture on timeout (org publishes no config, or a slow pull).

--no-daemon has no reconcile to wait on, so it just prints the finish-enrolling note. A failed daemon install is reported and skips the wait.

Files

  • src/core/cli/remote_commands.js - trimmed message; waitForClientAttach helper (exported, injectable seam); daemon-installed path now waits then reports; enrollCentralSink made an injectable dep for testing.
  • test/core/remote-login-command.test.js - updated --no-daemon assertions; added attach-reported, timeout-fallback, failed-install, and two waitForClientAttach unit tests.
  • llp/0063-...decision.md - dated forward-ref note recording that the server follow-up shipped and the interim hint was replaced by waiting on the reconcile (old text kept as the record; the decision is not rewritten).

Checks

  • node --test test/core/remote-login-command.test.js - 41/41 pass
  • npm run typecheck - clean

@platypiiplatypii changed the title Fix stale 'nothing is captured yet' message on loginLogin: wait for reconcile and report real capture stateJul 6, 2026
The post-enrollment hint printed 'nothing is captured yet - run hyp
attach' unconditionally and synchronously, before the daemon's first
config pull. With org default config (server LLP 0043) shipped,
auto-attach (LLP 0044) is the primary path, so the hint was stale on
every login and pushed manual work the daemon was about to do.
Trim the sink line to 'forwarding logs to <url>', then wait for the
daemon's first reconcile (polling on-disk attach markers via
collectHypAwareStatus) and report the truth: 'capturing <clients>' on
attach, or point at 'hyp status' on timeout.
@platypii
platypiiforce-pushed the fix/login-capture-message-stale branch from bf9fe69 to 6f135f6CompareJuly 7, 2026 00:05
@philcunliffe

Copy link
Copy Markdown
Contributor

🔍 neutral review (Codex + Claude) — request_changes

Adopted into neutral's review at the user's request. Tests green (41/41, typecheck clean) and the LLP 0063 doc edit is proper (Draft doc; appends a dated forward-ref note, preserves the interim record — no settled decision reversed). Two majors worth addressing before merge:

Major 1 — false failure report on a login that actually succeeded

waitForClientAttach's poll loop (remote_commands.js:79-89) doesn't guard the collect() call, and collectHypAwareStatus is not actually throw-proof despite the "best-effort" docstring: measureCacheStats → walkForStats (status.js:741) re-throws any non-ENOENT fs error (EACCES/EMFILE/EIO), and the call at status.js:452 is unguarded. A transient fs error during the up-to-30 polls — after the sink is provisioned and the daemon installed — propagates out of runBrowserLogin and crashes the command / returns nonzero, discarding the successful enrollment and the intended no clients attached yet fallback. That's the same false-reporting class this PR sets out to fix.
Fix: wrap collect({env}) in try/catch inside the loop and treat a throw as "not attached this tick" (continue → timeout fallback); optionally also guard measureCacheStats in the collector.

Major 2 — up-to-30s silent hang on the no-config / slow-pull path

The attach-wait blocks the login for the full 30s default with zero output when the org publishes no config or the first pull is slow, then prints no clients attached yet. The old code answered instantly — this reads as a hang (amplified once an interactive prompt precedes it; see the note below).
Fix: print a waiting for the daemon to attach clients… line on stderr before polling, and/or shorten/parameterize the 30000ms default.

Minors

  • remote_commands.js:452 — each poll re-runs the FULL status collector (plugin discovery, catalog build, config validation, recursive cache walk) every 1s; it only needs the attach markers. That cache walk is also what creates the Major-1 throw path. Prefer a marker-only probe (probeClientAttachFromDescriptor per client).
  • remote_commands.js:87Math.min(intervalMs, remaining) busy-spins if a caller passes intervalMs ≤ 0 (not reachable in prod; exported seam only). Math.max(1, intervalMs).
  • test/core/remote-login-command.test.js:275 — comment says "deduped by the caller's join" but there's no dedup (harmless; Map keys). Drop "deduped".

Heads-up: interaction with the in-flight local-only-dir-selection change set (LLP 0080/0081)

That change set will wire an interactive enrollment picker (runLocalOnlyPicker) into this same runBrowserLogin. It's not coded on any branch yet, so nothing races this PR now. When it lands, the collision is deps-plumbing only (the { login, seed, enroll, waitForAttach } destructure + the two @param deps JSDoc lines + the runRemoteLogin forwarding block — all additive union merges); the picker body inserts cleanly before the seeded.length===0 fork, which #259 doesn't touch. Combined order is coherent: login → seed → picker (persists export exclusions) → enroll → #259 attach-wait (polls attach markers) — orthogonal state, and exclusions land before the daemon's first pull, which is correct. Note for whoever merges second: #259 renames the in-fork enrollCentralSink(...) call to enroll(...), and all deps seams must be unioned or a test silently loses its injection. (This also makes the Major-2 progress line more important — a silent 30s wait right after an interactive prompt reads as a freeze.)

Automated neutral review (Codex = independent second family). Findings only — the branch is yours; neutral hasn't modified or merged it.

… probe (PR #259 review round 1)
Round-1 review fixes for the login-time client attach wait:
- Major 1: guard the per-poll probe in waitForClientAttach so a transient fs
error (EACCES/EMFILE/EIO) mid-poll is swallowed as not-attached-this-tick
instead of crashing the command and discarding a successful enrollment.
Also guard measureCacheStats in collectHypAwareStatus so the collector
honors its best-effort docstring (walkForStats re-throws non-ENOENT).
- Major 2: print a 'waiting for the daemon to attach clients...' line on
stderr before polling so the up-to-30s wait no longer reads as a silent
hang; budget named ATTACH_WAIT_DEFAULT_MS so the line and loop agree.
- Minor A: replace the full-collector-per-poll with a marker-only probe
(loadClientDescriptors once + probeAttachedClients per tick) so each poll
is cheap and cannot throw on a cache walk.
- Minor B: floor the poll sleep at 1ms so a non-positive intervalMs (exported
seam only) cannot busy-spin.
- Minor C: fix the stale 'deduped' test comment (Map keys, no dedup).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@philcunliffe

Copy link
Copy Markdown
Contributor

🔍 neutral review round 2 (Codex + Claude) — approve

Round-2 review of the fixes for the round-1 findings — head 346d97d, additive on 6f135f6 (no force-push). Both independent families confirm the production code is correct and every round-1 finding is resolved. CI green (typecheck + test), mergeable.

Round-1 findings — all verified resolved

  • Major 1 (false failure report on a login that actually succeeded) ✓ — the per-poll probe throw is swallowed → timeout fallback; collectHypAwareStatus now guards measureCacheStats, so walkForStats re-throwing a non-ENOENT fs error can no longer crash the login. New regression test drives a throwing probe through runRemoteLogin and asserts exit 0 + the fallback line.
  • Major 2 (up-to-30s silent hang) ✓ — waiting for the daemon to attach clients (up to 30s)... now prints on stderr before polling; a test asserts it's present at the instant the wait begins.
  • Minor A ✓ — a marker-only probe (probeAttachedClients / probeClientAttachFromDescriptor) replaces the full status collector per poll, and maps ENOENT and other fs errors to not-attached, so the poll can't throw on a cache walk. Descriptor discovery is hoisted out of the loop.
  • Minor B ✓ — Math.max(1, …) floors the sleep; a non-positive interval can't busy-spin.
  • Minor C ✓ — the stale "deduped" test comment is gone.

Fresh bug scan (Claude): deadline/poll math correct, the collect → probe dep-seam rename is complete, the probe's return shape matches its callers, no timer leak — nothing ≥80. Style is clean (no-semicolon JS, JSDoc). The llp/0063 edit is a proper Draft-doc editorial forward-note (interim record preserved, no settled decision reversed).

One minor, non-blocking — test evidence

Codex (cat 9): the new login/wait tests stub the loadClientDescriptors / probeAttachedClients seams, so the marker-only probe's own logic (descriptor load + ENOENT/fs-error mapping) is not directly exercised — a regression there wouldn't fail the suite. Optional follow-up: add a focused probeAttachedClients test against real on-disk marker files (or a login-level test driving the default waitForClientAttach). Non-blocking — the behavior is covered indirectly and CI is green.

Verdict: approve. Round-1 findings resolved, no blockers or majors. Review rounds complete (N=2). Held for a human to merge — neutral does not merge.

Automated neutral review (Codex = independent second family). Findings only — the branch is yours; neutral hasn't modified or merged it.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@platypii@philcunliffe