RTOP-316: Tighten over-256-char comments and remove in-repo development plans

.claude/review-guidelines.md

@@ -1,6 +1,6 @@
 # Review Guidelines
 
-Apply the full checklist in `docs/pr-reviews/PR_REVIEW_CHECKLIST.md`. Priorities:
+Apply the shared review process maintained outside the repo. Priorities for this runtime:
 
 - Dual-process boundaries hold: PLC core (C/C++) and webserver (Python) communicate only via the documented IPC commands (`core/src/plc_app/unix_socket.c`).
 - Real-time safety in the scan cycle: no blocking calls, allocation, or logging in the hot path.

.github/pull_request_template.md

@@ -15,4 +15,4 @@
 - [ ] `bash scripts/run-pytest.sh` passes
 - [ ] `pre-commit run` clean
 - [ ] Docs updated if behavior changed (README, CLAUDE.md, docs/)
-- [ ] Follows `docs/pr-reviews/PR_REVIEW_CHECKLIST.md`
+- [ ] Follows the shared review process (summary in `.claude/review-guidelines.md`)

CONTRIBUTING.md

@@ -24,4 +24,5 @@ If your change alters documented behavior (commands, endpoints, env vars, archit
 
 ## Review
 
-PRs are reviewed against `docs/pr-reviews/PR_REVIEW_CHECKLIST.md` (summary in `.claude/review-guidelines.md`).
+PRs are reviewed against the shared review process maintained outside the repo. The in-tree
+summary lives at `.claude/review-guidelines.md`.

bootloader/internal/api/authz_test.go

@@ -12,12 +12,9 @@ import (
 	"github.com/Autonomy-Logic/openplc-runtime/bootloader/internal/runtimeauth"
 )
 
-// The routes that can change what this device runs are admin-only.
-//
-// The runtime treats `user` as a restricted role, but the bootloader checked
-// only the signature: any runtime account could change the runtime version or
-// self-update the bootloader -- and a self-update starts a container with the
-// Docker socket bound, which is host root.
+// Routes that change what the device runs are admin-only. Self-update
+// starts a container with the Docker socket bound (host root), so a
+// signature-only check is not enough.
 func TestARestrictedAccountCannotChangeWhatTheDeviceRuns(t *testing.T) {
 	cases := []struct {
 		name, method, path, body string

bootloader/internal/api/server.go

@@ -2,18 +2,10 @@
 // Copyright (c) 2026 Autonomy®
 
 // Package api is the bootloader's control API on port 8445.
-//
-// Deliberately small. This is the interface to the component that recovers a
-// device, so its surface is the shortest list that does the job: say what
-// state you are in, show me the runtime's logs, restart it, change its
-// version, wipe its data. It accepts no programs and does not control the PLC
-// -- those belong to the runtime, and a bootloader that could do them would be
-// a second, less-reviewed path to the same capability.
-//
-// Every route except login and capabilities requires a token from the
-// runtime's own account set. Capabilities is unauthenticated for the same
-// reason the runtime's is: a client has to be able to tell what it is talking
-// to before it has credentials.
+// Surface is intentionally small: state, runtime logs, restart,
+// version change, wipe. No program upload, no PLC control — those
+// belong to the runtime. Every route except login and capabilities
+// requires a token from the runtime's own account set.
 package api
 
 import (
@@ -65,34 +57,23 @@ type Updater interface {
 	Progress() updater.Progress
 }
 
-// SelfUpdater replaces the bootloader with a newer version of itself.
-//
-// Start returns once the helper that performs the swap is running: this
-// process is about to be stopped by it, so there is no completion to report
-// and nothing to poll -- the client reconnects and reads the new version from
-// capabilities.
+// SelfUpdater replaces the bootloader with a newer version. Start returns
+// once the helper is running; this process is about to be stopped by it,
+// so the client reconnects and reads the new version from capabilities.
 type SelfUpdater interface {
 	Start(ctx context.Context, version string) error
 }
 
-// HostReporter answers for the machine the runtime runs on.
-//
-// The bootloader is the right place for this. It exists on every device that
-// can be updated from an editor, including one running a runtime far older
-// than these endpoints -- so a Runtime Status screen fed from here is
-// populated regardless of which runtime version is installed, which is not
-// true of anything served by the runtime itself.
+// HostReporter answers for the machine the runtime runs on. Lives in the
+// bootloader so the Runtime Status screen is populated independent of
+// which runtime version (or none) is installed.
 type HostReporter interface {
 	SystemInfo(ctx context.Context) (*dockerapi.Info, error)
 }
 
-// Authenticator resolves credentials against the runtime's account set, and
-// serves the signing secret behind the tokens it issues.
-//
-// Secrets() is read per request rather than snapshotted at start-up: on a
-// fresh install the runtime writes .env and restapi.db AFTER the bootloader is
-// already running, and a snapshot taken before that left every authenticated
-// route answering 503 until the container was restarted.
+// Authenticator resolves credentials against the runtime's accounts and
+// serves the token signing secret. Secrets() is read per request so a
+// first-install .env written after start-up does not leave routes at 503.
 type Authenticator interface {
 	Authenticate(ctx context.Context, username, password, pepper string) (*runtimeauth.User, error)
 	CountUsers(ctx context.Context) (int, error)
@@ -219,12 +200,9 @@ func (s *Server) ListenAndServe(ctx context.Context) error {
 
 // --- middleware ----------------------------------------------------------
 
-// authenticated wraps a handler with bearer-token verification.
-//
-// It also enforces the no-users rule: with no accounts on the device the
-// bootloader accepts nothing at all. First-user bootstrap is a sensitive flow
-// that lives in the runtime alone, and a bootloader that could mint the first
-// admin would be a second path to owning the device.
+// authenticated wraps a handler with bearer-token verification. With no
+// accounts on the device, every authenticated route is refused:
+// first-user bootstrap belongs to the runtime, never here.
 func (s *Server) authenticated(next func(http.ResponseWriter, *http.Request)) http.HandlerFunc {
 	return func(w http.ResponseWriter, r *http.Request) {
 		count, err := s.cfg.Users.CountUsers(r.Context())
@@ -277,18 +255,9 @@ func subjectFrom(ctx context.Context) string {
 // device. Matched against the runtime's own value (webserver/restapi.py).
 const RoleAdmin = "admin"
 
-// adminOnly restricts a route to administrators.
-//
-// Applied to the routes that can change what this device runs. Without it any
-// runtime account -- including one the runtime itself treats as restricted --
-// could change the runtime version or self-update the bootloader, and a
-// self-update starts a container with the Docker socket bound, which is host
-// root. The runtime distinguishes these roles; the component that can replace
-// the runtime must not be the one that ignores the distinction.
-//
-// The role is read from the database per request. The token carries none, and
-// a role claim would mean a demotion did not take effect until the token
-// expired.
+// adminOnly restricts a route to administrators. Role is read from the DB
+// per request (not from a token claim) so a demotion takes effect
+// immediately rather than at token expiry.
 func (s *Server) adminOnly(next func(http.ResponseWriter, *http.Request)) http.HandlerFunc {
 	return s.authenticated(func(w http.ResponseWriter, r *http.Request) {
 		subject := subjectFrom(r.Context())
@@ -326,20 +295,8 @@ func bearerToken(r *http.Request) (string, bool) {
 
 // --- handlers ------------------------------------------------------------
 
-// handleDeviceInfo reports the machine the runtime runs on.
-//
-// Sourced from the Docker daemon, which runs on the host and answers for it.
-// The obvious alternative -- have the runtime report on itself -- is what this
-// replaces: that endpoint exists only in runtimes new enough to have it, so
-// every device in the field today answered it with a catch-all body and the
-// screen had nothing to show. The bootloader is present wherever an update is
-// possible at all, which makes it the one source that is always there.
-//
-// Deliberately only facts that VARY between devices. "This runtime runs in a
-// container" and "this device updates itself" were both here at one point and
-// are neither: a client reaching this handler at all has already learned them
-// from the bootloader answering, so reporting them again was a field that
-// could only ever hold one value.
+// handleDeviceInfo reports the machine the runtime runs on. Sourced from
+// the Docker daemon. Only facts that VARY between devices are reported.
 func (s *Server) handleDeviceInfo(w http.ResponseWriter, r *http.Request) {
 	payload := map[string]any{
 		"bootloaderVersion": s.cfg.Version,
@@ -583,16 +540,9 @@ func (s *Server) handleUpdateProgress(w http.ResponseWriter, r *http.Request) {
 	writeJSON(w, http.StatusOK, s.cfg.Updater.Progress())
 }
 
-// handleSelfUpdate replaces the bootloader itself.
-//
-// Separate from the runtime update on purpose: they change different things
-// and fail differently. A bootloader that will not come back costs the ability
-// to manage the device; a runtime that will not come back stops the plant. The
-// runtime container is untouched here, so a PLC keeps running throughout.
-//
-// There is no progress to poll. This process is replaced as part of the
-// operation, so the client's connection ends with it -- reconnecting and
-// reading /capabilities is how you learn the outcome.
+// handleSelfUpdate replaces the bootloader itself. The runtime container
+// is untouched so a running PLC survives. No progress endpoint: this
+// process ends; reconnect and read /capabilities for the outcome.
 func (s *Server) handleSelfUpdate(w http.ResponseWriter, r *http.Request) {
 	if s.cfg.SelfUpdater == nil {
 		writeError(w, http.StatusNotImplemented, "this bootloader cannot update itself")

bootloader/internal/api/server_test.go

@@ -218,10 +218,6 @@ func TestProtectedRoutesRejectATokenSignedWithAnotherSecret(t *testing.T) {
 }
 
 func TestATokenTheBootloaderIssuedIsAccepted(t *testing.T) {
-	// The bootloader owns its own sessions: the editor logs in here with the
-	// credentials it already holds, and this token is only ever presented
-	// back to the bootloader. Cross-service acceptance is deliberately not a
-	// contract -- the two services may resolve different .env files.
 	srv := newTestServer(t, &fakeUsers{count: 1}, healthySupervisor(), &fakeLogs{})
 	resp, _ := get(t, srv, "/api/bootloader/status", validToken(t))
 	if resp.StatusCode != http.StatusOK {

bootloader/internal/api/throttle.go

@@ -10,21 +10,9 @@ import (
 	"time"
 )
 
-// Login throttling.
-//
-// Every attempt, including one for a username that does not exist, runs a full
-// 600k-iteration PBKDF2 -- deliberately, so response timing does not enumerate
-// accounts. That makes the endpoint expensive by design, and this component
-// runs on the host network with no CPU limit, beside a PLC whose real-time
-// headroom must not be eaten. A loop of login POSTs from any host on the LAN
-// was therefore both a brute-force path to Docker-socket access and a cheap
-// denial of service against the scan cycle.
-//
-// Two independent limits, because they address different things: a global
-// concurrency cap bounds the CPU an attacker can command at any instant, and
-// per-source backoff makes sustained guessing impractical. Neither replaces
-// the other -- one attacker with two connections defeats a cap alone, and a
-// distributed source set defeats backoff alone.
+// Login throttling. Every attempt runs 600k PBKDF2 iterations (timing-safe
+// against enumeration), so the endpoint needs both a global concurrency cap
+// and per-source backoff to resist brute force and DoS.
 const (
 	// maxConcurrentVerifications is small on purpose. Two verifications in
 	// flight is more than a legitimate operator ever needs, and it leaves the
@@ -142,12 +130,8 @@ func (t *loginThrottle) recordSuccess(source string) {
 	delete(t.sources, source)
 }
 
-// requestSource identifies the caller for backoff purposes.
-//
-// The remote address only. There is no proxy in front of this: it is reached
-// directly on the LAN, or through the orchestrator agent on the same host, so
-// an X-Forwarded-For here would be attacker-controlled and trusting it would
-// hand out a way to reset someone else's backoff.
+// requestSource identifies the caller by remote address only. No proxy
+// sits in front, so X-Forwarded-For would be attacker-controlled.
 func requestSource(r *http.Request) string {
 	host, _, err := net.SplitHostPort(r.RemoteAddr)
 	if err != nil {

bootloader/internal/api/tls.go

@@ -19,19 +19,9 @@ import (
 	"time"
 )
 
-// The bootloader serves HTTPS with its own self-signed certificate, generated
-// once into its state directory and reused thereafter.
-//
-// Its own, rather than the runtime's: the runtime generates its certificate
-// inside its image (webserver/certOPENPLC.pem), so it is not in the shared
-// volume and there is nothing to share. Reusing it would also mean the
-// bootloader could not serve TLS at all before the runtime had ever started,
-// which is exactly the case recovery exists for.
-//
-// Self-signed is the same posture the runtime already has, so the editor's
-// handling is unchanged. Persisting it matters: regenerating on every boot
-// would change the fingerprint each time the device restarted, training
-// operators to click through certificate warnings.
+// Self-signed HTTPS cert, generated once into the state dir and persisted
+// so the fingerprint stays stable. Owned by the bootloader because
+// recovery must work before the runtime has ever started.
 const (
 	certFileName = "bootloader-cert.pem"
 	keyFileName  = "bootloader-key.pem"
@@ -72,11 +62,8 @@ func LoadOrCreateCertificate(stateDir string) (tls.Certificate, error) {
 	return cert, nil
 }
 
-// generateSelfSigned writes a new P-256 certificate and key.
-//
-// ECDSA rather than RSA: a 2048-bit RSA keygen on a Pi-class CPU takes long
-// enough to notice at first boot, and P-256 is both faster and universally
-// supported by anything that will talk to this port.
+// generateSelfSigned writes a new P-256 ECDSA certificate and key.
+// ECDSA because 2048-bit RSA keygen is slow enough on a Pi to notice.
 func generateSelfSigned(certPath, keyPath string) error {
 	key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
 	if err != nil {

bootloader/internal/discovery/responder.go

@@ -1,23 +1,11 @@
 // SPDX-License-Identifier: MIT
 // Copyright (c) 2026 Autonomy®
 
-// Package discovery answers the editor's LAN discovery probe while the runtime
-// is not running.
-//
-// The runtime has its own responder (webserver/discovery/network_discovery.py)
-// and normally owns this port. The bootloader's exists for one situation: the
-// runtime is down, so nothing is answering, and a device that cannot be found
-// cannot be repaired. Without this, a failed update makes a device vanish from
-// the editor's list at exactly the moment somebody needs to reach it.
-//
-// It runs ONLY in recovery mode, which is what keeps the two responders from
-// ever competing. Recovery is defined as "the runtime container is stopped" --
-// the supervisor stops it before entering that state -- so exclusivity holds
-// by construction rather than by coordination. Two services answering the same
-// broadcast would give the editor two different answers for one device.
-//
-// The protocol is the runtime's, byte for byte: a fixed magic string in, one
-// JSON datagram back, unicast to the sender.
+// Package discovery answers the editor's LAN discovery probe while
+// the runtime is down. Runs ONLY in recovery mode (the supervisor
+// stops the runtime first), so it never races the runtime's own
+// responder. Protocol is byte-for-byte the runtime's: fixed magic in,
+// one JSON datagram back, unicast to the sender.
 package discovery
 
 import (
@@ -51,13 +39,8 @@ const (
 	perIPRateLimit = 100 * time.Millisecond
 )
 
-// Reply is what a probing editor receives.
-//
-// service says "openplc-bootloader", not "openplc-runtime". Being honest here
-// costs an older editor the ability to see a device in recovery -- but an
-// older editor could not have done anything about it either, and the
-// alternative is a client that thinks it is talking to a working runtime and
-// then fails against every endpoint it tries.
+// Reply is what a probing editor receives. service says
+// "openplc-bootloader" so a client cannot mistake it for a working runtime.
 type Reply struct {
 	Service         string `json:"service"`
 	ProtocolVersion int    `json:"protocol_version"`
@@ -109,24 +92,19 @@ func New(port int, provider ReplyProvider, log *slog.Logger) *Responder {
 	}
 }
 
-// Enable starts answering probes. Safe to call when already enabled.
-//
-// A bind failure is logged and swallowed. Discovery is a convenience: losing
-// it must not stop the bootloader serving its control API, which is the
-// primary way in. The most likely cause is the runtime still holding the port,
-// and in that case the device is findable anyway.
+// Enable starts answering probes; idempotent. A bind failure is logged
+// and swallowed: discovery is a convenience, losing it must not stop the
+// control API from serving.
 func (r *Responder) Enable() {
 	r.mu.Lock()
 	if r.conn != nil {
 		r.mu.Unlock()
 		return
 	}
 	var conn *net.UDPConn
-	// SO_REUSEADDR and SO_REUSEPORT, matching how the runtime binds the same
-	// port. Linux shares a UDP port only when EVERY socket asked to, so
-	// without these a lingering bootloader socket makes the runtime's own bind
-	// fail -- and the runtime does not retry. The release now happens before
-	// the runtime starts; this is the safety net for a race in between.
+	// SO_REUSEADDR/REUSEPORT must match the runtime; Linux shares a UDP
+	// port only when every socket asked to, otherwise a lingering socket
+	// here blocks the runtime's bind.
 	listener := net.ListenConfig{Control: reusePort}
 	generic, err := listener.ListenPacket(
 		context.Background(), "udp", ":"+strconv.Itoa(r.port))

bootloader/internal/discovery/reuse_linux.go

@@ -11,12 +11,8 @@ import (
 	"golang.org/x/sys/unix"
 )
 
-// reusePort sets SO_REUSEADDR and SO_REUSEPORT on the listening socket.
-//
-// The runtime sets both when it binds the discovery port. Linux shares a UDP
-// port only when every socket involved asked to, so the bootloader has to ask
-// too -- otherwise its lingering socket makes the runtime's bind fail, and the
-// runtime binds once at start-up and never retries.
+// reusePort sets SO_REUSEADDR and SO_REUSEPORT so the port can be shared
+// with the runtime, which also sets both.
 func reusePort(_, _ string, c syscall.RawConn) error {
 	var setErr error
 	err := c.Control(func(fd uintptr) {