fix(run): stop double-wrapping startup URLs when node.hostname is a URL - #2219
Open
dawsontoth wants to merge 7 commits into
Open
fix(run): stop double-wrapping startup URLs when node.hostname is a URL#2219dawsontoth wants to merge 7 commits into
dawsontoth wants to merge 7 commits into
Conversation
The startup banner composed Operations-API and REST URLs as 'http://' + getThisNodeName() + ':' + port + '/'. When node.hostname is configured as a full URL (e.g. http://localhost:9926), getThisNodeName() returns that whole URL, so the banner wrapped the scheme twice and glued on the wrong port: http://http://localhost:9926:9925/. Add getThisNodeHostname(), which reduces the node name to a bare host (stripping any scheme and port) for URL composition, and use it at the three banner sites. Bare hosts pass through unchanged; IPv6 brackets are preserved. Fixes #2218 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
github-actions
Bot
requested review from
Ethan-Arrowood,
cb1kenobi,
heskew and
kriszyp
August 19, 2026 15:14
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a new helper function getThisNodeHostname in server/nodeName.ts to extract a bare hostname (without scheme or port) from the configured node name. This helper is then used in bin/run.ts to construct well-formed startup and REST URLs, preventing issues like double-wrapped schemes or incorrect ports when node.hostname is configured as a full URL. Comprehensive unit tests have also been added to verify various hostname formats, including bare hosts, full URLs, and IPv6 addresses. There are no review comments, and I have no feedback to provide.
Contributor
|
Reviewed; no blockers found. |
Replace string concatenation with template-literal interpolation for the Operations-API HTTP/HTTPS URL lines. No behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
dawsontoth
commented
Aug 19, 2026
Comment on lines
-324
to
+327
| logMsg += pad('') + 'http://' + getThisNodeName() + ':' + env.get(CONFIG_PARAMS.OPERATIONSAPI_NETWORK_PORT) + '/\n'; | ||
| logMsg += `${pad('')}http://${getThisNodeHostname()}:${env.get(CONFIG_PARAMS.OPERATIONSAPI_NETWORK_PORT)}/\n`; | ||
| } | ||
| if (env.get(CONFIG_PARAMS.OPERATIONSAPI_NETWORK_SECUREPORT)) { | ||
| logMsg += | ||
| '\n' + | ||
| pad('') + | ||
| 'https://' + | ||
| getThisNodeName() + | ||
| ':' + | ||
| env.get(CONFIG_PARAMS.OPERATIONSAPI_NETWORK_SECUREPORT) + | ||
| '/\n'; | ||
| logMsg += `\n${pad('')}https://${getThisNodeHostname()}:${env.get(CONFIG_PARAMS.OPERATIONSAPI_NETWORK_SECUREPORT)}/\n`; |
Contributor
Author
There was a problem hiding this comment.
Swapping getThisNodeName for getThisNodeHostname is the only meaningful change, the rest is pure formatting.
- Extract nodeNameToDisplayHost(name) as a pure function so it can be unit tested with plain assert (no new sinon), per AGENTS.md test-style rule. - Bracket a bare IPv6 literal (e.g. '::1') so composed banner URLs stay valid instead of emitting https://::1:9925/. - Compress the added comments to a terse durable rationale. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
cb1kenobi
reviewed
Aug 19, 2026
Address the round-2 cross-model review: - Compress the helper's comments to a single durable contract, per the zero-new-comments default. - Add a plain-assert test pinning that an unparseable node.hostname is returned unchanged rather than dropped (closes the surviving mutant the reviewer found: catch returning undefined). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
cb1kenobi
reviewed
Aug 19, 2026
The round-2 refactor tested only the pure nodeNameToDisplayHost and stopped exercising getThisNodeHostname — the wrapper bin/run.ts actually calls — so a regression dropping normalization from the wrapper would pass. Add a real-config test (via env.setProperty, no sinon) that a URL-valued node.hostname is normalized to a bare host. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The pure nodeNameToDisplayHost helper and the getThisNodeHostname wrapper are unit-tested, but nothing drove startupLog() itself, so a banner call site that switched back to getThisNodeName() could recreate the double-wrapped URL while those suites stayed green. Drive the real exported startupLog() with a URL-valued node.hostname (via env.setProperty + console.log capture, no sinon) and assert the Operations-API HTTP/HTTPS and REST URL lines are well-formed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address the round-5 review: - startupBanner test snapshots and restores node.hostname and the secure ops port in cleanup instead of blanking them, so it cannot leak config into other suites sharing the mocha process. - Trim the nodeNameToDisplayHost comment to the durable "why". Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
cb1kenobi
reviewed
Aug 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2218. When
node.hostnameis configured as a full URL (e.g.http://localhost:9926), the Operations-API and REST lines of the startup banner composed URLs likehttp://http://localhost:9926:9925/— the scheme was wrapped twice and the wrong port appended, because the banner assumedgetThisNodeName()returns a bare host. A newnodeNameToDisplayHost()reduces the node name to a bare host (stripping any scheme/port, and bracketing a bare IPv6 literal) before each banner URL is composed; the three banner URL sites now use it. Result:http://localhost:9925/(ops) andhttp://localhost:9926/(REST).For the human reviewer
getThisNodeName(). Chosen becausegetThisNodeName()is the node's identity (cert CN, replication), so rewriting it is risky, while the reported bug is display-only. The alternative — normalizing at the source — would also fixhostnameToUrl(), which shares the same bare-host assumption and would produce a corrupt replication URL (ws://http://host:port) for a URL-valuednode.hostname. That corruption is pre-existing and left out of scope here; the root-cause follow-up (validate/warn the bare-hostnode.hostnameinvariant ingetThisNodeName(), which also covershostnameToUrl()/replication and thesecurity/keys.tscert identity) is being handled as a separate change already in progress. Reversible either way.Hostname:banner line left verbatim. It still prints the raw configurednode.hostname(the URL), deliberately — so an operator who configured a URL where a bare host is expected sees that, rather than having it silently masked. Only the composed URLs are normalized.Verification
Route: unit tests (plain
assert, no stubbing) at two levels — the purenodeNameToDisplayHosthelper across bare host / full URL /host:port/ bracketed and bare IPv6, and an end-to-end test that drives the real exportedstartupLog()with a URL-valuednode.hostnameand asserts the composed Operations-API HTTP/HTTPS and REST banner lines are well-formed (nohttp://http://double-wrap). Executed locally:unitTests/server/nodeName.test.js15 passing andunitTests/bin/startupBanner.test.js3 passing (18 together, no cross-suite leakage);npm run lint:requiredandprettier --checkclean. The e2e test also fails as expected when the banner sites are reverted togetThisNodeName(), proving it catches the regression.Review coverage
Authored by Opus 4.8. Cross-model review ran 6 delta rounds (one after each round of fixes). Codex
gpt-5.6-solgave independent, graded outside-family coverage every round; Gemini viaagy(default model) ran on some rounds and failed others (agyflakiness); the Harper domain adjudicator and Cursor legs were auto-pruned as narrow low-risk deltas, so Codex's findings were not domain-adjudicated. Every substantive finding was fixed and re-verified: new Sinon usage (→ pure function + plainassert), a bare-IPv6 URL gap, missinggetThisNodeHostnamewrapper coverage, missing end-to-endstartupLogcoverage, and a test-isolation cleanup gap. One nit is left open by choice — a two-line "why" comment onnodeNameToDisplayHost, kept as durable rationale the house style permits. Receipt @d7b5c46bb.Human-Review-Need: 3 @ d7b5c46