Skip to content

nssh: codebase cleanup — split, dedupe, type the log schema#6

Merged
abizer merged 13 commits intomasterfrom
cleanup
May 6, 2026
Merged

nssh: codebase cleanup — split, dedupe, type the log schema#6
abizer merged 13 commits intomasterfrom
cleanup

Conversation

@abizer
Copy link
Copy Markdown
Owner

@abizer abizer commented May 5, 2026

Summary

11 commits of focused cleanup. No user-visible behavior changes except where explicitly called out.

What changed

Refactors / dedup

  • main.go (507 lines) split into main.go (CLI dispatch, 82 lines), session.go (orchestration + ntfy subscriber), oauth.go (callback proxy), and grew remote.go (SSH helpers) and infect.go (subcommand parser).
  • wire.Publish helper consolidates 3 sites that duplicated the marshal-envelope-and-pick-inline-vs-attachment dance.
  • runRemoteScript helper consolidates 5 sites that called ssh ... bash -l -s with a script over stdin.
  • LogEvent struct types the JSONL log schema — both writer (logEvent/logMessage/prepareRemote) and reader (status.go::formatEvent) marshal against the same type now.
  • Dead code removed: logTopic, logErr, shortPath.

Correctness

  • golang.org/x/mod/semver replaces hand-rolled looksLikeSemver and string-equality version compare. Fixes a latent false-positive: v1.2.3+build1 vs v1.2.3+build2 no longer triggers the upgrade nag. Behavior change worth noting: a release client connecting to a remote running a +dirty dev build of the same release no longer prompts to overwrite.
  • proxyOAuthCallback now caps Accept at 5 minutes via a TCPListener deadline. Previously, an abandoned OAuth flow leaked the listener + goroutine until session end.
  • latestReleaseTag parses the GitHub response with encoding/json instead of hunting for \"tag_name\":\" with strings.Index.

Cross-platform

  • internal/clipboard gated to GOOS=darwin via filename suffix; clipboard_other.go provides stub functions returning errUnsupported so the linux remote-shim build compiles cleanly.
  • runRemoteScript change: three infectRemote ssh sites now run with BatchMode=yes. Password-auth-only setups will see Permission denied (publickey) instead of repeated prompts. Pubkey auth that worked for the initial scp works for every subsequent ssh.

CI

  • New .github/workflows/test.yaml runs go vet + go test on PRs and master pushes; reusable via workflow_call.
  • release.yml now needs: test so goreleaser is gated on green tests.
  • flake.nix sets doCheck = true, so the cachix build runs go test as part of nix build.

Tests

  • New: wire.Publish (5 cases — inline / image-attachment / large-text / no-mutate / nil-data), LogEvent schema (4 cases — omit-zeros, preserve explicit zero/false, round-trip, wire subset).
  • cmd/nssh had zero tests; now has log_test.go. Broader test coverage tracked separately.

Test plan

  • go test ./... passes on darwin
  • go vet ./... and staticcheck ./... clean
  • GOOS=linux GOARCH=amd64 go build succeeds
  • nix build succeeds (test job runs under doCheck = true)
  • Smoke test on a real remote after merge: nssh devbox, paste image, xdg-open, OAuth callback flow

Files

.github/workflows/release.yml         |   4 +
.github/workflows/test.yaml           |  18 +
CLAUDE.md                             |   6 +-
cmd/nssh/{clipboard,infect,log,main}.go (refactored)
cmd/nssh/oauth.go                     |  98 ++++ (new)
cmd/nssh/remote.go                    |  97 ++++ (new — runRemoteScript + prepareRemote + resolveShortHost)
cmd/nssh/session.go                   | 303 ++++ (new — nsshMain + subscriber + handleMessage)
cmd/nssh/log_test.go                  |  95 ++++ (new)
flake.nix                             |   2 +-
go.mod, go.sum                        |   x/mod/semver
internal/clipboard/clipboard_darwin.go (renamed from clipboard.go)
internal/clipboard/clipboard_other.go |  15 + (new — stubs for !darwin)
internal/wire/publish.go              |  38 ++ (new)
internal/wire/publish_test.go         | 135 +++ (new)

🤖 Generated with Claude Code


Note

Medium Risk
Medium risk due to significant refactoring of session/infect/message-handling code and changes to version comparison and remote SSH invocation (BatchMode=yes), which could affect install/upgrade prompts and remote automation behavior.

Overview
Restructures the nssh CLI implementation by splitting the former large main.go into focused modules (session.go, oauth.go, remote.go) and moving infect argument parsing into infect.go, while keeping the same high-level flows.

Deduplicates wire/transport plumbing by introducing wire.Publish (central inline-vs-attachment logic) and runRemoteScript (shared ssh ... bash -l -s helper), and updates clipboard/shim code to reuse these helpers and avoid repeated base64/JSON handling.

Tightens correctness around versions, OAuth, and logging: replaces hand-rolled semver checks with golang.org/x/mod/semver (including semver-based remote/local mismatch checks), adds a 5-minute accept deadline to OAuth callback proxying, switches GitHub release tag parsing to encoding/json, and replaces map-based log events with a typed LogEvent schema that is used by both log writer and status --tail formatter (with new unit tests).

Improves build/CI signal and docs by adding a reusable test GitHub workflow and gating releases on it, enabling doCheck in flake.nix, adding non-darwin clipboard stubs for cross-compilation, and adding new docs/internals.md + docs/protocol.md references from README/CLAUDE.

Reviewed by Cursor Bugbot for commit d129354. Bugbot is set up for automated code reviews on this repo. Configure here.

abizer and others added 13 commits April 30, 2026 17:14
- new test.yaml: go vet + go test on PRs and master pushes, also
  callable as a reusable workflow.
- release.yml: gate goreleaser on the test workflow via needs.
- flake.nix: doCheck = true so cachix.yaml's nix build runs go test
  as part of the build (gates the cache push automatically).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The docs/ directory has been empty for a while. Remove the layout
entry until we write proper docs (tracked separately).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace strings.Index hunting for `"tag_name":"` with json.NewDecoder
into a typed struct. Same correctness, no quirks.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Renames clipboard.go to clipboard_darwin.go (file-suffix makes it
auto-darwin) and adds clipboard_other.go with stub functions returning
errUnsupported on every other GOOS. The remote-shim path on Linux
never calls into clipboard, so this is a no-op for current users while
unblocking a clean build matrix and a future Linux client backend.

Test file similarly renamed; the now-redundant runtime.GOOS skip is
removed.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Five sites repeated ssh + bash + script-via-stdin boilerplate:
prepareRemote, detectRemoteDesktop, and the trio in infectRemote
(mkdir, infect self, command -v xclip). Consolidate into one helper
that always uses BatchMode=yes and pipes stderr to os.Stderr so SSH
errors surface to the user.

Behavior change worth noting: the three infectRemote sites now use
BatchMode=yes — password-auth-only setups (rare) will see "Permission
denied (publickey)" instead of repeated password prompts. Pubkey auth
that works for the initial scp works for every subsequent ssh.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three sites duplicated the "marshal envelope, choose inline vs
attachment by inlineThreshold and image-mime, call ntfy.Publish*"
flow: clipboard.go's clip-read success and error responses, and
shim.go's clip-write request. Consolidate behind wire.Publish, which
takes the envelope by value so the inline-mode mutation of Body
doesn't leak back to the caller (callers immediately log the
envelope after publish, and the log shouldn't include base64 noise).

inlineThreshold moves into wire as the exported InlineThreshold —
the inline-vs-attachment decision is a protocol concern that lives
naturally with the envelope type. wire now imports ntfy; both packages
remain leaves of cmd/nssh.

Tests cover both transport paths, the large-text → attachment
threshold, the image → attachment unconditional rule, the nil-data
case (kind=open), and the no-mutation contract.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Until now, both the writer (logEvent + logMessage) and the reader
(status.go::formatEvent / formatWireMessage) handled events as
map[string]any with stringly-typed field names. Renaming a field
or adding one was grep-and-pray.

LogEvent in log.go is now the shared schema. omitempty drops fields
that aren't relevant to a given event, so the on-disk JSON shape is
unchanged. Exit and Mosh are *int / *bool so callers can record an
explicit zero/false (session-end exit=0 success) without being dropped.

Updated five logEvent call sites (main.go × 3, shim.go × 6 — wait,
six in shim.go, three in main.go) to pass typed LogEvent struct
literals. logMessage and the inline session-open emitter in
prepareRemote use the same type. Reader unmarshals straight into
LogEvent, with the per-event-detail fields rendered alphabetically
to preserve current `nssh status --tail` output.

Adds log_test.go covering: zero-fields omitted, explicit Exit=0 and
Mosh=true preserved, full round-trip including pointer fields, and
the wire-message subset emitted by logMessage.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
main.go was 507 lines mixing CLI dispatch, version printing, OAuth
proxy, ntfy subscriber loop, message routing, prepareRemote, and the
session exec lifecycle. Split into:

- main.go (82 lines): main, usage, printVersion, buildVersion — pure
  CLI dispatch.
- session.go (new, 288 lines): nsshMain plus the runtime guts —
  subscribeNtfy, deadlineConn, handleMessage, runSession,
  resetTerminal, remoteHasMosh.
- oauth.go (new, 83 lines): localhostRe, extractLocalhostPort,
  proxyOAuthCallback, handleOpen.
- remote.go: now also holds prepareRemote and resolveShortHost
  alongside runRemoteScript.
- infect.go: gains infectCmd (the subcommand parser belongs with
  infectSelf and infectRemote).

No behavior change. Each function moved verbatim; doc comments on
the moved-in functions were tightened but otherwise the implementations
are identical.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Four small follow-ups from self-review of the cleanup branch:

- oauth: cap proxyOAuthCallback's Accept at 5 minutes via a TCPListener
  deadline. Previously, an abandoned OAuth flow (user closes the tab)
  leaked the listener + goroutine until the nssh session ended.
- session: handleMessage now decodes the inline base64 body once and
  passes it to handleClipWrite; the handler's own decode block is
  gone. Cheap (≤3KB inline payload max) but the redundancy was
  visible.
- session: extract selectTransport from nsshMain — flag-driven
  ssh-vs-mosh choice plus locale env-setup is now a focused helper.
- clipboard: drop the local-task-id reference from clipboard_other.go's
  package doc; replace with a more durable forward-pointer.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Hand-rolled looksLikeSemver was a strict shape check (vX.Y.Z, no
prerelease, no build metadata) that doubled as the gate for the
session-start version-mismatch nag. The string-equality compare on
mismatch was a latent bug: v1.2.3+build1 vs v1.2.3+build2 would fire
even though they're the same release.

Drop in golang.org/x/mod/semver, rename the gate to isReleaseVersion
(more honest about its actual semantics — strictly stricter than
semver.IsValid because we exclude prereleases and build metadata),
and replace the !=  string compare with semver.Compare so build
metadata is ignored. Behavior change: a release client connecting to
a remote running a +dirty dev build of the same release tag no
longer prompts to overwrite.

Also relax CLAUDE.md's "stdlib only" rule to "minimize external
deps; prefer stdlib + golang.org/x/* over third-party." Pulls in
x/mod and (transitively) x/tools — both Go-team maintained.

flake.nix vendorHash flipped from null to the computed hash now
that go.mod has a real require.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two focused docs to capture the things that aren't obvious from the
code or the README:

- docs/internals.md (~260 lines): architecture, end-to-end flows for
  clipboard paste and OAuth callback, and the reasoning behind the
  unusual choices (ntfy as side channel, argv[0] dispatch, topic as
  secret, infect-self desktop refusal, what we deliberately don't do).
- docs/protocol.md (~190 lines): wire envelope schema, inline-vs-
  attachment rules, the four kinds, log event vocabulary, config
  precedence, state directory layout, ntfy endpoints touched.

CLAUDE.md regains the docs/ entry. README links both at the bottom.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Spell out exactly which kinds of changes need a docs/ touch (new
envelope kinds, new LogEvent fields, config keys, ntfy endpoints,
personas) and which don't (pure refactors). Goal: keep
internals.md/protocol.md from drifting by treating doc updates as
part of the change, not a follow-up.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@abizer abizer merged commit f66d205 into master May 6, 2026
2 checks passed
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.

1 participant