Skip to content

Document node.hostname config option (must be a bare hostname) - #632

Merged
dawsontoth merged 4 commits into
mainfrom
claude/node-hostname-docs-33a776
Aug 25, 2026
Merged

Document node.hostname config option (must be a bare hostname)#632
dawsontoth merged 4 commits into
mainfrom
claude/node-hostname-docs-33a776

Conversation

@dawsontoth

@dawsontoth dawsontoth commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Documents the node configuration section, which was missing from Configuration Options even though that page bills itself as covering all harper-config.yaml top-level sections, and records the node-identity behavior change that shipped in HarperFast/harper#2223.

node.hostname is this node's identity, not a URL: it becomes the node's TLS certificate common name and the host replication advertises to peers and dials to reach the node. A URL or host:port value silently corrupted both — a node configured as http://host:9926 advertised and dialed a host literally named http (HarperFast/harper#2218).

Companion to HarperFast/harper#2223, which is now merged. That PR landed as a loud rejection at the config boundary, not the warn-only change it was originally scoped as, so this docs PR was rewritten to match what actually shipped.

What this documents

  • New ## node section in the config reference, placed immediately before ## replication (identity is defined right before the section that consumes it).
  • The bare-host requirement, as enforced: a bare hostname, IPv4 literal, or unbracketed IPv6 literal is valid; a scheme, port, path, credentials, query string, fragment, bracketed [::1], or non-string value is rejected and Harper fails to start.
  • replication.hostname too — harper#2223 put it under the same constraint (it previously accepted string|number), so its bullet was updated rather than left alone.
  • The full identity fallback chainreplication.hostname → host in replication.url → certificate common name → Operations API host → 127.0.0.1, skipping unusable derived sources.
  • release-notes/v5-lincoln/5.3.md — a new page carrying the breaking-change entry, since an install with a URL/host:port hostname will fail to boot after upgrading.

For the human reviewer

Two calls here are the author's judgment and are the places to push back:

  1. v5.3.0 is an assumption, not a derived fact. harper#2223 is merged to main but no tag contains it (latest is v5.2.4), so the ship version is not yet knowable from the repo. v5.3.0 was chosen on the reasoning that a breaking change won't ship in a patch. If it actually ships as v5.2.5, both <VersionBadge> values and the release-notes filename need renaming before release.
  2. This publishes a 5.3 release-notes page before 5.3 exists. The release-notes sidebar is autogenerated from the directory, so merging adds a visible "5.3" entry for an unreleased version. That is deliberate (it is where the breaking change belongs) but it does surface early — worth a second opinion on whether to hold this file until the release is cut.

Smaller notes:

  • No in-doc issue links in the reference page (per the earlier review round), but the release-notes page does link harper#2218 — that matches its own house style; 5.2.md links harper#2049 the same way.
  • node.url is left undocumented. It exists in the new Joi schema, but node_url appears only in hdbTerms.ts with no consumer, so documenting it would advertise dead surface.
  • replication.routes entries are explicitly called out as still accepting URLs in the release notes, because the route schema is not bare-host constrained and readers will otherwise over-apply the new rule.
  • No admonition was added to the config reference: that page has zero admonitions and is a flat quick-reference, so the boot-failure consequence is carried in bold inside the bullet instead.

Verification

  • npm run build — succeeds; document count 403 → 404 (the new 5.3 page). The two broken-anchor warnings it prints are pre-existing on unrelated pages (backups/overview, release-notes/v5-lincoln/5.1); the new #node / #replication anchors and the release-notes link into /reference/v5/configuration/options#node all resolve.
  • npm run format:write then npm run format:check — clean.
  • Rebased onto current main (12 commits) with no conflicts.
  • Every documented claim was read off the merged core commit 08531e344, not the PR description: utility/nodeIdentity.ts (bareHostViolation — the exact accept/reject set), validation/configValidator.ts (bareHostConstraints on both node.hostname and replication.hostname, and that route hostnames are unconstrained), and server/nodeName.ts (getThisNodeName fallback order).

Review coverage

Authored by Claude Opus 4.8, revised by Claude Opus 5 after harper#2223 merged with different behavior. Accuracy verified by reading the merged core implementation directly. gemini-code-assist reviewed the earlier revision; its two comments were addressed (in-doc issue links removed; its suggested v5.2.0 badge was declined as already-shipped and replaced with the v5.3.0 badge above, with the caveat in item 1).

@dawsontoth
dawsontoth requested a review from kriszyp August 19, 2026 17:28

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds documentation for the new node configuration block and its hostname option in reference/configuration/options.md. The feedback suggests removing direct links to internal GitHub issues and pull requests to maintain clean, user-facing documentation, and recommends using the <VersionBadge> format to properly document the behavior change regarding startup warnings for invalid hostnames.

Comment thread reference/configuration/options.md Outdated
@github-actions

Copy link
Copy Markdown

🚀 Preview Deployment

Your preview deployment is ready!

🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-632

This preview will update automatically when you push new commits.

@github-actions
github-actions Bot temporarily deployed to pr-632 August 19, 2026 17:41 Inactive
@github-actions

Copy link
Copy Markdown

🚀 Preview Deployment

Your preview deployment is ready!

🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-632

This preview will update automatically when you push new commits.

dawsontoth and others added 3 commits August 21, 2026 14:57
The node section was missing from the "all top-level sections" config
reference. Add it, documenting that node.hostname must be a bare
hostname (no scheme, no port) because it becomes this node's TLS
certificate common name and the host replication advertises and dials;
a scheme or port silently corrupts both (harper#2218). Cross-links the
startup-warning PR (harper#2223).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Remove the (see harper#2218 / harper#2223) links from the rendered
node.hostname bullet to keep the user-facing reference clean; the
requirement and rationale stand on their own. PR-to-PR cross-links
remain in both PR descriptions for team traceability.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
harper#2223 merged as a boot-time rejection, not a warning: an invalid
node.hostname or replication.hostname now fails config validation and
Harper does not start. Update both bullets accordingly, note that an IP
literal (unbracketed IPv6 included) is valid, list the rejected shapes,
and give the full identity fallback chain down to 127.0.0.1.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@dawsontoth
dawsontoth force-pushed the claude/node-hostname-docs-33a776 branch from d37512e to b4c8fe0 Compare August 21, 2026 19:00
@github-actions
github-actions Bot temporarily deployed to pr-632 August 21, 2026 19:03 Inactive
@github-actions

Copy link
Copy Markdown

🚀 Preview Deployment

Your preview deployment is ready!

🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-632

This preview will update automatically when you push new commits.

Tag both hostname bullets with a changed-in-v5.3.0 badge, and add the
5.3 release-notes page documenting the breaking change: an install
whose node.hostname or replication.hostname is a URL, host:port, or
non-string will fail to start until corrected.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@github-actions
github-actions Bot temporarily deployed to pr-632 August 21, 2026 19:24 Inactive
@github-actions

Copy link
Copy Markdown

🚀 Preview Deployment

Your preview deployment is ready!

🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-632

This preview will update automatically when you push new commits.

@dawsontoth
dawsontoth marked this pull request as ready for review August 21, 2026 19:24
@dawsontoth
dawsontoth requested a review from a team as a code owner August 21, 2026 19:24

@DavidCockerill DavidCockerill left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving — the content is excellent. One thing to settle before this merges, because for a change that fails startup the version is not a detail.

The v5.3.0 badge and the release-notes/v5-lincoln/5.3.md placement may both be a line too late. The implementing commit — 08531e34 "Reject a non-bare-host node identity and fix IPv6 replication URLs", authored 2026-08-19 — is on core origin/main, whose package.json reads 5.2.4, and it is in no tagged release yet. It also reaches harper-pro right now through the core sync in harper-pro#751.

In plain terms: if this ships in a 5.2.x patch, an operator checks the 5.2.x release notes, finds nothing about hostnames, upgrades, and the node will not start. Mis-versioned docs for a fail-to-start change are worse than absent docs, because they actively tell the reader they are safe.

To be fair, core main sitting at 5.2.4 does not prove the change releases as 5.2.x — it may be held for 5.3. The ask is only to confirm the target release and align the badge and the release-note file to it; if it is 5.2.x, this needs a 5.2.md entry rather than a 5.3.md one.

Everything else here is right, and unusually so:

  • The failure is enumerated, not summarised — scheme, port, path, credentials, query string, fragment, bracketed IPv6, non-string, each named, plus that the startup error reports the offending value and the reason. Someone hitting this matches their error to the page in one read.
  • The old behaviour is stated with its consequence: earlier versions accepted such values and silently corrupted certificate matching and replication, with the http://host:9926 → host literally named http example. That is the argument for making a breaking change, and the page makes it rather than asserting it.
  • The fallback chain is documented in order and matches the asBareHost(...) ?? asBareHost(...) chain in the implementation.
  • replication.hostname is covered with the precedence rule stated — the omission that would otherwise send half the affected readers to the wrong page.

— DAIvid (Claude Opus 5)

@dawsontoth
dawsontoth merged commit aa74b1c into main Aug 25, 2026
7 checks passed
@dawsontoth
dawsontoth deleted the claude/node-hostname-docs-33a776 branch August 25, 2026 14:30
@github-actions

Copy link
Copy Markdown

🧹 Preview Cleanup

The preview deployment for this PR has been removed.

Sign up for free to 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