Skip to content

refactor: layered ports-and-adapters design with enforced complexity and layer gates - #5

Merged
naerymdan merged 18 commits into
mainfrom
refactor/architecture
Oct 26, 2026
Merged

naerymdan merged 18 commits into
mainfrom
refactor/architecture

Conversation

@naerymdan

@naerymdan naerymdan commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Brings the layered ports-and-adapters design and the static analysis gates of kandev-plugin-forgejo#35 to this plugin. Behaviour is unchanged by the refactor itself; the fixes that review turned up are listed under Fixes found in review.

Design

docs/architecture.md has the layer table, what enforces each rule, and the practices a linter can't check. An import may only point to a lower layer:

Layer Packages
foundation internal/remote (quoting, command output, the admin login, SSH error kinds), internal/kandev (the late-bound Host, state documents, action responses)
model egress, profile, hosts, secrets, pki, msbctl (the protocol, run through its Runner port): pure
features hostcheck (Test's diagnosis, certificate care), sandbox (the provider's decisions), readiness, hostsetup (machine setup, plus the setup and upgrade jobs). No SDK
adapters sshx (keyring, pool, admin login), store (every Host state and secret key)
transport internal/plugin: one handler per surface; wire.go is the only file that names an adapter
entry server

What changed to get there:

  • Runtime is no longer a god object. Every action and provider RPC was a method on it, with access to every adapter. Each surface now has its own handler holding only the ports it uses: hosts, CA, secrets, egress, host jobs, readiness and the executor provider. Actions route through a table instead of a switch.
  • One store package. Host-state load and save was written out six times, for the registry, egress, secrets, status, CA and API URL. All of it is now in internal/store over one codec (kandev.LoadDocument/SaveDocument).
  • Decisions moved out of the transport:
    • the diagnosis and certificate care went to hostcheck;
    • placement, recovery, the bootstrap, the provision request and Kandev's states went to sandbox, which is SDK-free;
    • secret validation went to secrets;
    • the readiness items went to readiness;
    • the job table and the setup and upgrade flows went to hostsetup.
  • Features no longer import the SSH adapter. Host setup and the diagnosis matched on sshx error sentinels and used its Output type. Those now live in internal/remote.
  • Large files are split by concern. hostsetup.go (750 lines) is now one file per step. In the UI, the hosts page row (complexity 38), hostDetail (38), toActionError (17) and the egress page are split into hooks and render helpers. The UI's job view is no longer a module-level variable.

Measured

before after
Go average cyclomatic complexity 4.77 3.80
Go worst cyclomatic / cognitive 21 / 20 15 / 15
Go functions over cyclomatic 15 6 0
UI functions over complexity 12 / over 100 lines 3 / 2 0 / 0
golangci-lint issues (gate config) 16 0

Enforced

  • internal/archtest fails go test on an import that crosses a layer, or on a package missing from the table.
  • golangci-lint (.golangci.yml, same as the forgejo plugin) runs staticcheck, errorlint, unparam, gocyclo 15, gocognit 20, nestif 5, funlen 50 statements and dupl.
  • ESLint with typescript-eslint caps UI complexity, function length and file length.
  • dependency-cruiser forbids cycles, the model importing components, and imports of bundle.ts.
  • make lint runs all of them. CI installs golangci-lint from its checksummed release binary.
  • make metrics and make deadcode report without failing. deadcode currently finds nothing.
  • go.mod ignores node_modules, where flatted ships Go sources.

Verification

  • make lint, make test-race, make test-ui and make verify-package-host pass locally.
  • No test expectation changed. Test plumbing that named moved internals was updated:
    • the in-memory host moved to kandevtest;
    • the bootstrap and lease-header unit tests moved to internal/sandbox;
    • one placement assertion goes through sandbox.PlaceOrReuse.
  • New UI tests render the host row and the egress page against a stub host.
  • An independent review compared every old handler with its new home. These fixes from it are in the last commit:
    • a reworded base_url error string is restored;
    • Jobs.Stop takes the table lock again, so a job cannot start during Close;
    • two decisions still in the transport moved into hostcheck and sandbox.

Also bumps vitest to 4.1.12 for two moderate @vitest/mocker advisories, GHSA-qxg6-89mg-rqv8 and GHSA-xp26-3454-mjgp. The lockfile was made with npm 11, because npm 10 crashes resolving the linked SDK package. docs/development.md now says so. npm audit reports 0 vulnerabilities.

Fixes found in review

Every finding below was reproduced with a failing test first; each test is kept.

  • Regression from the refactor: a host write before the CA existed loaded, and on a fresh install generated, the plugin's SSH key. HealNow now connects only once a root exists (TestAddWithoutARootLeavesTheKeyAlone).

  • A root that exists but cannot be used (its key is gone from the secret store, unreadable, or does not match) was treated as "no root yet": hosts.add/update reported nothing and logged nothing, setup.status failed as a whole, and ca.create refused to replace it even with replace: true, which left no way out. pki.ErrRootUnusable now marks exactly these cases, which are now handled like this:

    • Test reports root_unusable (it used to blame the host with tls_install_failed).
    • hosts.add/update answer tls root_unusable.
    • The root readiness item is failing, with the replace step.
    • ca.create answers 409 naming replace: true, and replaces the root when that is set.

    A failure to read the store is kept separate: it stays a retryable 500 and never replaces the root (TestAnUnusableRootIsReportedAndReplaceable, TestAStoreOutageIsNotAnUnusableRoot).

  • The add-existing-host form let Add pin a host key that was scanned for one destination after the destination was edited. It now requires the scan to match, as the setup form already did (UI test in pages.test.ts).

  • Misnamed failures: an unreadable SSH key, or unreadable last-check results, were reported as the host registry's (TestAnUnreadableKeyIsNamed, TestUnreadableCheckResultsAreNamed), and a refused admin login was logged as "host refused the plugin's ssh key".

  • The maintainer connected, and so created the key, with no hosts registered (TestMaintainingNoHostsLeavesTheKeyAlone).

All but the first were already on main. CHANGELOG.md and docs/configuration.md are updated.

Checked and ruled out, so not changed:

  • the setup job's closure does not keep the admin login after the dial (heap-dump probe, with positive and negative controls);
  • JobView's stale onFinished is harmless;
  • the //nolint:errcheck directives on <-done are needed.

…nal/remote

Host setup and the plugin's diagnoses matched on sshx's error sentinels and
used its Output type, so a feature depended on the SSH adapter. They are the
vocabulary of running a command on a host, not SSH: internal/remote holds
them, sshx produces them.
…ace handlers

Every action and provider call was a method on one Runtime that held every
adapter, with Host-state load/save code repeated for each document and the
diagnosis, certificate care, placement and job logic inline beside the SDK
plumbing. Behaviour is unchanged; no test expectation changed.

- internal/kandev: the late-bound host, the plugin-wide state document
  codec, action responses. kandevtest is the in-memory host the tests used.
- internal/store: the one package that knows a state or secret key: the
  registry, egress policy, secrets, host status, root CA, API URL, SSH key.
- internal/hostcheck: Test's diagnosis and certificate care, over ports.
- internal/sandbox: resource state, bootstrap, provision request, placement
  and recovery decisions, Kandev's states, lease headers; SDK-free.
- internal/secrets: the secret index model and validation.
- internal/readiness: setup.status's items.
- internal/hostsetup: also the job table and the setup and upgrade jobs.
- internal/sshx: the keyring that loads or generates the plugin's key.
- internal/plugin: one handler per surface holding only the ports it uses,
  a routing table instead of a switch, and wire.go as composition root.
profile.Parse, the setup probe, the base URL check and the admin key
parsing are split into named steps; host setup's 750-line file is split
by step. Also: the unchecked <-done after a cancel is marked as deliberate,
the two capitalized error strings are reworded, an unused test helper is
gone, and go.mod ignores node_modules, where flatted ships Go sources.
…he transport

hosts.go scanned and fingerprinted keys with sshx directly, and Runtime held
the stores and the keyring by their concrete types. Both now go through
ports, so wire.go is the one transport file that names an adapter.
The hosts page's row (complexity 38), hostDetail (38), toActionError (17)
and the egress page were over the complexity and length limits. The row is
now a hook plus summary, actions and detail helpers in host-row.ts; the job
view and the setup forms have their own files, and the job view is no longer
a module-level variable set by the page. New tests render the row and the
egress page against a stub host.
- internal/archtest: the layer rules as a test, failing on an import that
  crosses a layer or on a package missing from the table.
- golangci-lint (.golangci.yml, as in kandev-plugin-forgejo): staticcheck,
  errorlint, unparam and the other correctness linters, and the caps:
  gocyclo 15, gocognit 20, nestif 5, funlen 50 statements, dupl.
- ESLint with typescript-eslint caps UI complexity, function and file
  length; dependency-cruiser forbids cycles, a model importing a
  component, and imports of bundle.ts.
- make lint runs them all; CI installs golangci-lint from its checksummed
  release binary. make metrics and make deadcode report without failing.
- The base_url refusals keep their original wording; ST1005 is waived on
  the line, since Kandev is a name and operators see the text.
- Jobs.Stop cancels under the job table's lock again, so a job started
  during Close is either waited for or refused.
- The root-scope check and heal-after-write move into hostcheck
  (OutsideRoot, HealNow), and a lost provision's outcome into sandbox
  (Recovered): decisions the transport was still making.
- The tests name the secret store keys through store's constants.
GHSA-qxg6-89mg-rqv8 and GHSA-xp26-3454-mjgp, moderate, in a devDependency.
Installed with npm 11: npm 10 crashes resolving the linked SDK package.
A host write before the CA exists loaded, and on a fresh install generated,
the plugin's SSH key, which the code before the refactor never did. Also
word ErrNotAuthorized for both logins it covers: an admin login refused
during setup was logged as the plugin's key being refused.
- hosts.add and hosts.update treated any failure to load the root, or to
  reach hosts, like having no root: no tls outcome and no log line. Only
  pki.ErrNoRoot skips certificate care now; anything else is logged and
  answered as failed/unavailable.
- setup.status blamed the host registry for every failure, including an
  unreadable root.
- The add-existing form let Add pin a host key scanned for one destination
  to another after the destination was edited. It now requires the scan to
  match, as the setup form already did.
- ca.create refused to replace a root that exists but cannot be loaded,
  even with replace: true, so a lost CA key had no way out. replace now
  covers it.
- setup.status failed as a whole on such a root, hiding the other items.
  The root item now reports it as failing, with the replace step.
- A failure to read the plugin's SSH key was reported as the host
  registry's on hosts.list, remove, test, setup and upgrade.
- The maintainer connected, and so created the key, with no hosts
  registered.
- Heal and HealNow share one classification of the root's state.
@naerymdan
naerymdan force-pushed the refactor/architecture branch from 5823d3c to 10b0652 Compare October 26, 2026 02:44
The previous commit read any failure to load the root as a broken root:
setup.status told the operator to replace it, and ca.create with replace
went ahead, even when only the secret store was briefly unreachable and the
root was fine. Following that advice would have destroyed a good root.

pki.ErrRootUnusable now marks the cases only a replace fixes: the key is
gone from the secret store, unreadable, or does not match the certificate.
Only those are a failing readiness item, a root_unusable Test problem and
TLS outcome (no longer reported as the host refusing a certificate), and
replaceable; without replace, ca.create answers 409 naming the way out. A
failure to read the store stays a 500 to retry.

hosts.list also named the host registry when the hosts' last check results
could not be read.
LoadRoot's unreadable-key and mismatch cases had no test asserting they are
ErrRootUnusable, so a revert to a plain error would have passed. The status
outage test named the Host state key itself; it now takes it from store, as
the secret keys already were.
A host write healed only once the root loaded, so with an unusable or
unreadable root every host was answered root_unusable or failed, even one
in tailscale serve mode that never uses the plugin's certificates. HealNow
now skips the host only when there is no root yet; otherwise it asks the
host first and lets Heal report the root for a proxy host alone.

ca.get answered an unusable root with a 500 to retry, while Test, hosts.add
and setup.status named it. It now answers exists: true, usable: false with
the reason, as only a replace fixes it.
One failed hosts.setup.status poll, such as a 503 during a plugin restart,
set the error and scheduled nothing, so the job view stayed on Running and
never called onFinished. A failure now schedules the next poll; a 404, a
job the plugin has forgotten, still stops it.
Runtime declared registry and connect alongside the embedded maintainer's,
so wire.go set each twice and a change to one copy missed the other; they
are now wire.go locals, and the maintainer's are the ones tests reach. The
dialer is a local too, handed to the pool, scans and setup alike rather
than read late by one of them.

Plan and the probe built SSH destinations with two helpers that bracketed
IPv6 differently; bracketHost now builds on bracket and net.JoinHostPort,
and both use it.
…again

A 4xx from hosts.setup.status, a job lost to a plugin restart, stopped the
polling but left the card on Running with only Close. The job now ends as
stopped, with the plugin's reason shown, and onFinished refreshes the host
list. Other failures keep polling.
@naerymdan
naerymdan merged commit 4819932 into main Oct 26, 2026
6 checks passed
@naerymdan
naerymdan deleted the refactor/architecture branch October 26, 2026 11:51
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