Name the server instead of printing its URL (#391) - #412
Conversation
`hyp remote login` printed `forwarding logs to https://hypaware.hyperparam.app`, and the first-sync privacy block printed the same origin. Terminals autolink any bare `https://` run and there is no escape sequence that suppresses it, so both reliably rendered as clickable links to a service endpoint that answers `{"error":"unknown_path"}` in a browser. Print the configured target name instead: `forwarding logs to the 'prod' server`, `first sync to the 'prod' server is 11:59pm tonight`. The name is what the user typed to reach the server, and `hyp remote list` stays the place that maps a name back to a URL. `hyp remote add` and `hyp remote list` still show URLs - there the URL is the content, not a destination being offered. LLP 0100 R1 needs no change: it requires the deadline, the backfill statement, and the skill hint, and LLP 0100 #flow already writes the example as `first sync to <server>`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review round 1 -
|
philcunliffe
left a comment
There was a problem hiding this comment.
Reviewed at 8d64c99. Full record above.
The diagnosis and the fix are both right. There is no ANSI escape that opts out of heuristic autolinking, so this was never a rendering quirk, and a dead-link click inside a privacy notice damages the trust that notice exists to build. Swapping the URL for the name is the correct call. Completeness within the privacy and first-sync path is verified complete, no degenerate name produces a crash or an empty label from any reachable path (12 variants driven through the real enrolling fork), and the tests are load-bearing rather than decorative, confirmed by mutation.
Requesting changes on one finding only. The PR removed a verifiable fact without leaving a signposted way back to it. The docstring justifies the swap with "hyp remote list is where name to URL lives", but nothing in the output says so, and the commands it does name cannot recover the destination: hyp status prints no URL, DURABLE_HINT points at hyp policy set, the privacy block points at the skill, and hyp remote status does not exist. On a bare hyp remote login the name comes from effectiveDefaultRemote(ctx.config), so the user is told a name they never typed, told their backfilled history ships tonight, and given no way to find out where.
One line after remote_commands.js:632 closes it, e.g. (run 'hyp remote list' to see its URL), or surface the central URL in hyp status.
Findings 2 to 4 in the record are yours to weigh and may be better as follow-ups: three sibling sites in the same file still print bare origins to stderr (defensible, since the origin is the load-bearing fact on an error path, but the docstring states the rule as general), remote add never validates the name so a URL-shaped name is interpolated into the trust notice, and the durable rule lives only in code comments rather than an LLP clause.
Advisory as always: neutral does not ready or merge contributor PRs.
Review finding 1: naming the server instead of its URL removed a verifiable
fact without signposting the way back to it. Nothing in the login output said
where to look, and none of the commands it does name can recover the URL - `hyp
status` names no server, DURABLE_HINT points at `hyp policy set`, the privacy
block points at the skill, and `hyp remote status` does not exist. A bare `hyp
remote login` resolves its target from `effectiveDefaultRemote`, so the user
could be shown a name they never typed, told their backfilled history ships
tonight, and given no way to find out where.
Both destination surfaces now carry the lookup. Each stream gets its own,
because the forwarding line is stdout and the privacy block is stderr, and
redirecting either must leave the other standing on its own:
forwarding logs to the 'hyperparam' server
(run 'hyp remote list' to see its URL)
...
first sync to the 'hyperparam' server is Jul 27, 2026, 11:59 PM
and includes your backfilled history
(run 'hyp remote list' to see that server's URL)
Review finding 4: the rule lived only in code comments, which is how a durable
constraint regresses silently. Pinned as LLP 0100 R1a, with @refs on both
surfaces.
Review finding 2, partial: the forwarding-line comment stated the rule as
general when three sibling error paths still print bare origins. Scoped it to
the success surfaces and said why the error paths differ - there the origin is
the fact the user must act on to decide which server to `hyp leave`.
Tests: pin the pointer on both streams, and drive a bare `hyp remote login`
through the enrolling fork - the default-name path the review noted was
unpinned, and the case that makes the pointer load-bearing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks - finding 1 is right, and it's the half of the trade I missed. Addressed in Finding 1 (blocking) - fixedVerified each of your claims before changing anything: Both surfaces now carry the lookup: Deliberately in both rather than once after The framing I took from your finding, now in the code and the LLP: withholding the URL is a readability choice, withholding the way to see it makes a consent surface unauditable. Finding 4 - donePinned as LLP 0100 R1a, with R1a binds the success surfaces, requires the lookup pointer, states why the pointer is not optional (the Finding 2 - scoped, not fixedTook the scope-note option. The comment stated the rule as general while Happy to file the follow-up if you'd still like those changed; my read is that they're correct as-is and the honesty gap was the comment, not the behavior. Finding 3 - not taken hereAgreed it's real - TestsAdded the two you'd expect: the pointer pinned on both streams separately, and a bare Your note about the whole-output Verification
One discrepancy worth flagging: I can't reproduce your 8 failures in |
Review round 2 -
|
| # | Item | Result |
|---|---|---|
| 1 | BLOCKING - no signposted way back to the URL | PASS |
| 2 | Sibling bare-origin sites | PASS, deferred with documented rationale (see finding 2) |
| 3 | runRemoteAdd does not validate the name |
PASS, deferred to issue #414 |
| 4 | Durable rule lived only in code comments | PASS, normative R1a added |
Finding 1, in detail. Both surfaces carry the pointer, and your stdout/stderr reasoning is factually correct rather than assumed: remote_commands.js:650-651 writes the forwarding line plus its pointer to stdout, :659 writes the privacy block carrying its pointer at :219 to stderr. A driver capturing the two streams separately confirmed each pointer lands on the claimed stream, so redirecting either genuinely leaves the other self-sufficient. Putting it in both rather than once was the right call.
hyp remote list really does surface the URL, including the hard case. readConfiguredRemotes (:861-874) seeds BUILTIN_REMOTES under the user's query.remotes, so on a bare login with an empty config it prints hyperparam https://hypaware.hyperparam.app token: stored - the exact name the privacy block just showed. Ran it.
Every path that prints these lines carries the pointer. Exercised: a named login (prod), a bare login with no remotes configured (resolving hyperparam through effectiveDefaultRemote, which was round 1's specific worry), a bare login with query.default_remote, and --no-daemon. All four print both pointers, and the --no-daemon and daemonCode !== 0 early returns sit at :663/:668, after the writes at :650-660, so neither can skip them. The wizard join lane goes through the identical code path (wizard/join.js:179 calls runRemoteLogin([], teed, {}) with a write-through tee). The already-enrolled re-login path correctly prints neither line, so the absence of a pointer there is right rather than a gap.
Regressions: none. Box formatting is intact (rule 62 chars, new pointer line 50, longest line is the pre-existing deadline line at 65, and the block has no side borders so nothing misaligns). The existing doesNotMatch(/https?:\/\//) invariant at remote-login-command.test.js:944 does cover the new lines, since it runs over out + err. No brittle whole-output equality assertions apply. Conventions clean: no U+2014 added anywhere in the delta (the em dashes present in both files are byte-identical at base ae63a02), no semicolons, no @typedef, no inline import() types. @ref LLP 0100#requirements [implements]: R1a at :206 and :649 resolve.
Master moved to dd3b438 (#408 and #407 landed) and this branch is behind. A trial merge is conflict-free, master's commits touch none of remote_commands.js, and the merged tree passes: 2745 tests, 8 failures, all the pre-existing leave-command.test.js ones.
Findings
1. NON-BLOCKING, but worth fixing before merge. The PR description is stale against the head and now contradicts what ships. The body was written for 8d64c99 and never updated for 0714482:
- Its
## Changeblock shows the privacy block without either pointer line, which is the exact copy round 1 rejected. A maintainer reading the body to decide whether to merge sees the version that was blocked. - Its
## LLPsection says verbatim "No doc change needed ... the destination's spelling is ours to choose." The head commit adds normative R1a atllp/0100-enrollment-privacy-review.spec.md:109-120doing precisely the opposite: it pins the spelling and mandates the lookup command. I checked both texts directly. The body actively misdescribes the design decision that lands. - Line citations drifted (
:626is now:650,:493is now:513), and## Verificationclaims2666 passing, 0 failingwhere the real numbers at this head are 2659 pass / 8 fail (the pre-existing failures, not this PR's fault, but the stated number is not reproducible).
Failure scenario: the squash commit message and the permanent PR record describe a design that is the opposite of the code and the LLP clause actually landing. This is a body edit, no code change.
2. NON-BLOCKING, carried forward. The residual bare-origin sites are scoped out honestly, but two of them fall outside the carve-out. The delta documents the boundary in two places (remote_commands.js:640-643 and the R1a clause at llp/0100:118-120), both saying the rule binds the success surfaces and that error paths still print origins, where the origin is the fact the user must act on. That is a good, honest scoping.
The gap: commands/central.js:384 (leaving <url>) and :524 (left <url> - this machine no longer forwards…) are hyp leave success surfaces, not error paths, so the documented carve-out does not cover them, and R1a is scoped to "the enrolling login's destination surfaces" so it does not reach hyp leave either. Since the body says Closes #391, and #391's scope really is narrowly the enrollment forwarding line, closing it is legitimate - but once this merges nothing tracks those two. A one-line follow-up issue closes the loop, the way #414 did for finding 3.
One nuance noted for completeness, not raised as a finding: for a user-added target, hyp remote list shows the configured query URL (https://hyp.internal/mcp) while forwarding goes to its origin (https://hyp.internal, :610). Since targetOrigin = originOf(entry.url) by construction the host is always identical and only a path suffix differs, so the user still learns the correct server, and for the built-in the two are byte-identical.
Verification run
node --test test/core/remote-login-command.test.js: 61 pass, 0 fail. npm test at this head: 2668 tests, 2659 pass, 8 fail. On the merged-with-master tree: 2745 tests, 2736 pass, 8 fail. npm run typecheck clean on both. The failure count did not grow and leave-command.test.js is still the only failing file, proved by running it at base ae63a02 in a throwaway worktree and getting the identical 3 pass / 8 fail with the same test names.
|
Round 2 reviewed at Approving. The blocking finding is genuinely fixed, verified by running the real code path across four enrolling scenarios plus the wizard join lane, not by reading it. Your stdout/stderr reasoning checks out, so putting the pointer on both surfaces rather than once was the right call. The risk I was most worried about (a pointer to a command that shows nothing) does not materialize: One thing to fix before you merge, which is a body edit and not code: the PR description is stale against the head and now contradicts what ships. Its One carried-forward item, yours to weigh: Advisory as always: neutral does not ready or merge contributor PRs, so landing this is your call. |
Closes #391.
Problem
hyp remote loginprinted the destination as a bare URL, in two places:Warp, iTerm2, VS Code's terminal and Ghostty all autolink any
https://run in output. There is no ANSI escape that suppresses this - OSC 8 exists to add an explicit hyperlink, not to opt out of heuristic detection. So the link in #391 wasn't a rendering quirk on the reporter's machine; it is what every modern terminal does with that line. The click lands on a service endpoint that returns{"error":"unknown_path"}.That is worst in the privacy block, where an invitation to a dead page sits in the one message asking the user to trust where their data is going.
Change
Print the configured target name instead of the URL, and pair each destination surface with the command that maps that name back:
The name is whatever the target is called -
hyperparamfor the built-in inbuiltin_remotes.js, orprod, or whatever was passed tohyp remote add <name> <url>.The lookup pointer is load-bearing, not decoration. A bare
hyp remote loginresolves its target fromeffectiveDefaultRemote, so the name shown can be one the user never typed, and nothing else this login prints recovers the URL:hyp statusnames no server,DURABLE_HINTpoints athyp policy set, the privacy block points at the skill, andhyp remote statusdoes not exist. Withholding the URL is a readability choice; withholding the way to see it would make a consent surface unauditable.Each surface carries its own pointer because the forwarding line is stdout and the privacy block is stderr - redirect either and the other has to stand on its own.
src/core/cli/remote_commands.js:650-651- the forwarding line and its pointerfirstSyncHoldMessage(remote_commands.js:211) - paramcentralUrl→serverName, call site passesname, pointer line at:219centralUrlis still computed and still used forenroll(); it just no longer reaches a printed string.Not changed
hyp remote addandhyp remote liststill show URLs. There the URL is the content, not a destination being offered.remote_commands.js:513(this machine is connected to <origin>) and its sibling error paths still print bare origins. There the origin is the fact the user must act on to decide which server tohyp leave, and those paths read origins back off sink config with no target name in scope, so naming them needs a reverse lookup - a separate change. The R1a scope note and the code comment both say so explicitly.On the server-side fix
This takes #391's suggested fix 2, in its stronger form: rather than relabelling the URL
API endpoint:, drop it. Labelling doesn't stop anyone clicking.Making
/a real landing page (suggested fix 1) is still worth doing and is the better end state - it would let the CLI print a URL that rewards the click, and it's the natural home for the hosted explorer in #392. That's a server-repo change; this PR is the client half that stops the dead click today. Both the code comment and R1a say to revisit if the root ever becomes browsable.LLP
Adds R1a to LLP 0100 §requirements plus a matching entry in §annotations.
The first round of this PR argued no doc change was needed - R1 asks for the deadline, the backfill statement and the skill hint, so the destination's spelling looked like ours to choose. Review finding 4 rejected that: the rule was living only in code comments, which is how a durable constraint regresses silently. R1a now pins it normatively - name the server by its configured target name, never print its URL, and name the command that maps the name back - along with the reason (terminals autolink, the root is a service endpoint), the scope (success surfaces only; error paths still print origins), and the revisit condition. Both surfaces carry
@ref LLP 0100#requirements [implements]: R1a.Verification
npm test- 2744 passing, 1 skipped, 0 failingnpm run typecheck- cleanTest changes:
forwarding logs toassertions and the threefirst sync toassertions were updated for the new wording.https?://at all, rather than pinning the two lines individually - so any future line that reintroduces a bare origin trips it.hyp remote logintest driving the enrolling fork with no positional target and noquery.default_remote: theeffectiveDefaultRemotepath the review noted was unpinned, and the case that makes the pointer load-bearing.Note:
node_moduleson my machine was stale after the #406 dep bump (hyparquetmissing, 139 spurious failures).npm installfixed it;package-lock.jsonis unchanged by this PR.🤖 Generated with Claude Code