feat(plan): sacred-tier review feedback durability (0.11.1) (#690)

Author: rsnodgrass

.claude-plugin/marketplace.json

@@ -12,7 +12,7 @@
       "name": "ox",
       "source": "./claude-plugin",
       "description": "Team context, session recording, and AI coworker coordination for collaborative development",
-      "version": "0.11.0"
+      "version": "0.11.1"
     }
   ]
 }

claude-plugin/.claude-plugin/plugin.json

@@ -1,7 +1,7 @@
 {
   "name": "ox",
   "description": "Team context, session recording, and AI coworker coordination for collaborative development",
-  "version": "0.11.0",
+  "version": "0.11.1",
   "author": {
     "name": "SageOx",
     "email": "hi@sageox.ai"

cmd/ox/plan_review.go

@@ -110,18 +110,59 @@ func runPlanReview(cmd *cobra.Command, slug string, noServe bool, idleTimeout ti
 		return reviewStaticFallback(cmd, gitRoot, slug, in, res, review)
 	}
 
-	ln, err := net.Listen("tcp", "127.0.0.1:0")
-	if err != nil {
-		cli.PrintHint("could not start review server, falling back to file export: " + err.Error())
-		in := plan.Parse(planMD)
-		review, _ := plan.AssembleReview(info.Dir)
-		return reviewStaticFallback(cmd, gitRoot, slug, in, res, review)
+	// Bind a STABLE address for this plan (persisted last port, then the
+	// deterministic per-plan port). Stability is a durability feature, not a
+	// nicety: the page's unsent marks live in origin-scoped localStorage, so a
+	// restarted server on a fresh ephemeral port would strand them and leave the
+	// old tab unable to ever reconnect. If a live server for this same plan is
+	// already up, reuse it instead of racing it for the port.
+	dirName := filepath.Base(info.Dir)
+	state, _ := plan.LoadReviewServerState(gitRoot, dirName)
+	var ln net.Listener
+	for _, p := range reviewPortCandidates(state.Port, dirName) {
+		l, lerr := net.Listen("tcp", fmt.Sprintf("127.0.0.1:%d", p))
+		if lerr == nil {
+			ln = l
+			break
+		}
+		if probeReviewServer(p, dirName) {
+			url := fmt.Sprintf("http://127.0.0.1:%d/", p)
+			fmt.Fprintf(cmd.OutOrStdout(), "%s %s\n", cli.StyleBold.Render("Review loop already open:"), url)
+			cli.PrintHint("Reusing the running server — feedback keeps flowing to the same session.")
+			if oerr := cli.OpenInBrowser(url); oerr != nil {
+				cli.PrintHint("open this URL to review: " + url)
+			}
+			return nil
+		}
+	}
+	if ln == nil {
+		l, lerr := net.Listen("tcp", "127.0.0.1:0")
+		if lerr != nil {
+			cli.PrintHint("could not start review server, falling back to file export: " + lerr.Error())
+			in := plan.Parse(planMD)
+			review, _ := plan.AssembleReview(info.Dir)
+			return reviewStaticFallback(cmd, gitRoot, slug, in, res, review)
+		}
+		ln = l
+		cli.PrintHint("Stable review ports are busy — serving on an ephemeral port; a previously open tab won't auto-reconnect to this one.")
 	}
 	addr := ln.Addr().String()
-	token, err := randomToken()
-	if err != nil {
-		_ = ln.Close()
-		return fmt.Errorf("review server: %w", err)
+	// Reuse the persisted token so a tab that outlived the last server
+	// authenticates against this one without a reload (same trust domain: the
+	// token only gates loopback callers and the state file is 0600).
+	token := state.Token
+	if token == "" {
+		t, terr := randomToken()
+		if terr != nil {
+			_ = ln.Close()
+			return fmt.Errorf("review server: %w", terr)
+		}
+		token = t
+	}
+	if tcp, ok := ln.Addr().(*net.TCPAddr); ok {
+		if serr := plan.SaveReviewServerState(gitRoot, dirName, plan.ReviewServerState{Port: tcp.Port, Token: token}); serr != nil {
+			slog.Debug("plan review: could not persist server state", "error", serr)
+		}
 	}
 	base := "http://" + addr
 
@@ -163,10 +204,13 @@ func runPlanReview(cmd *cobra.Command, slug string, noServe bool, idleTimeout ti
 			return nil
 		case <-idle.C:
 			fmt.Fprintln(out, "\nReview session idle — closing.")
-			cli.PrintHint("Re-open anytime with `ox plan review " + slug + "`; feedback is saved in the ledger.")
+			cli.PrintHint("Everything submitted is saved in the ledger. The open page flips to disconnected mode; " +
+				"unsent marks stay in the browser. Re-open anytime: `ox plan review " + slug + "` (same address — the page reconnects and restores them).")
 			return nil
 		case <-ctx.Done():
 			fmt.Fprintln(out, "\nReview session closed.")
+			cli.PrintHint("Everything submitted is saved in the ledger. The open page flips to disconnected mode; " +
+				"unsent marks stay in the browser. Re-open anytime: `ox plan review " + slug + "` (same address — the page reconnects and restores them).")
 			return nil
 		}
 	}
@@ -189,9 +233,35 @@ func liveReviewHandler(gitRoot, slug, planDir, base, token string, bc *broadcast
 			return
 		}
 		w.Header().Set("Content-Type", "text/html; charset=utf-8")
+		// no-store: offline serving is the service worker's job — the HTTP cache
+		// must never mask a dead server with a stale page that still looks live.
+		w.Header().Set("Cache-Control", "no-store")
 		_, _ = w.Write(html)
 	})
 
+	// /healthz: unauthenticated identity probe (loopback only). A second
+	// `ox plan review` uses it to detect this server and reuse it; the page's
+	// offline probe uses it to notice the server is back. Exposes nothing but
+	// which plan is being served.
+	mux.HandleFunc("/healthz", func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		w.Header().Set("Cache-Control", "no-store")
+		_ = json.NewEncoder(w).Encode(reviewHealth{App: "ox-plan-review", Slug: slug, Dir: filepath.Base(planDir)})
+	})
+
+	// /sw.js: the offline shell (assets/sw.js) — keeps the plan readable on a
+	// reload after this process exits; review.js registers it.
+	mux.HandleFunc("/sw.js", func(w http.ResponseWriter, r *http.Request) {
+		js, err := plan.ReviewServiceWorkerJS()
+		if err != nil {
+			http.NotFound(w, r)
+			return
+		}
+		w.Header().Set("Content-Type", "text/javascript; charset=utf-8")
+		w.Header().Set("Cache-Control", "no-store")
+		_, _ = w.Write(js)
+	})
+
 	// SSE: EventSource can't set headers, so the token rides as a query param.
 	mux.HandleFunc("/events", func(w http.ResponseWriter, r *http.Request) {
 		if r.URL.Query().Get("t") != token {
@@ -444,6 +514,53 @@ func reviewStaticFallback(cmd *cobra.Command, gitRoot, slug string, in plan.Inpu
 	return nil
 }
 
+// reviewHealth is the /healthz body — enough for a second `ox plan review`
+// (and the page's reconnect probe) to recognize a live server for a plan.
+type reviewHealth struct {
+	App  string `json:"app"`
+	Slug string `json:"slug"`
+	Dir  string `json:"dir"`
+}
+
+// reviewPortCandidates orders the ports to try binding: the last port this
+// plan actually served on (persisted state — the origin an open tab is parked
+// on), then the plan's deterministic stable port and a few slots after it for
+// same-machine collisions. Deduped, order-preserving.
+func reviewPortCandidates(persisted int, dirName string) []int {
+	stable := plan.StableReviewPort(dirName)
+	raw := []int{persisted, stable, stable + 1, stable + 2, stable + 3, stable + 4}
+	seen := map[int]bool{}
+	var out []int
+	for _, p := range raw {
+		if p <= 0 || p > 65535 || seen[p] {
+			continue
+		}
+		seen[p] = true
+		out = append(out, p)
+	}
+	return out
+}
+
+// probeReviewServer reports whether a live ox review server for THIS plan is
+// already listening on port — so a second `ox plan review` reuses it instead
+// of racing it for the port or silently serving a twin.
+func probeReviewServer(port int, dirName string) bool {
+	c := &http.Client{Timeout: 700 * time.Millisecond}
+	resp, err := c.Get(fmt.Sprintf("http://127.0.0.1:%d/healthz", port))
+	if err != nil {
+		return false
+	}
+	defer resp.Body.Close()
+	if resp.StatusCode != http.StatusOK {
+		return false
+	}
+	var h reviewHealth
+	if jerr := json.NewDecoder(io.LimitReader(resp.Body, 4096)).Decode(&h); jerr != nil {
+		return false
+	}
+	return h.App == "ox-plan-review" && h.Dir == dirName
+}
+
 // randomToken returns a fresh 128-bit hex token, or an error. It FAILS CLOSED —
 // no static fallback — so the server never starts with a guessable token in the
 // exact failure mode where entropy is unavailable.

cmd/ox/plan_review_resume_test.go

@@ -0,0 +1,141 @@
+package main
+
+// Tests for the review server's restart/resume durability surface: the
+// stable-identity endpoints and the port selection that keep an open review
+// tab alive across server death.
+
+import (
+	"encoding/json"
+	"io"
+	"net"
+	"net/http"
+	"strings"
+	"testing"
+
+	"github.com/sageox/ox/internal/plan"
+)
+
+// TestReviewServer_HealthzIdentifiesPlan verifies /healthz is served without a
+// token and names the plan being served. Failure prevented: a second
+// `ox plan review` can't recognize the running server, so it races it for the
+// port; the page's offline probe can't detect recovery.
+func TestReviewServer_HealthzIdentifiesPlan(t *testing.T) {
+	dir := t.TempDir()
+	srv, _, _ := newTestReviewServer(t, dir)
+	resp, err := http.Get(srv.URL + "/healthz")
+	if err != nil {
+		t.Fatalf("healthz: %v", err)
+	}
+	defer resp.Body.Close()
+	if resp.StatusCode != http.StatusOK {
+		t.Fatalf("healthz status = %d", resp.StatusCode)
+	}
+	var h reviewHealth
+	if err := json.NewDecoder(resp.Body).Decode(&h); err != nil {
+		t.Fatalf("decode: %v", err)
+	}
+	if h.App != "ox-plan-review" || h.Slug != "p" {
+		t.Errorf("healthz identity = %+v", h)
+	}
+}
+
+// TestReviewServer_ServesOfflineShell verifies /sw.js is served as JS. Failure
+// prevented: the page registers a 404 as its service worker, so a reload while
+// the server is down shows a browser error page instead of the cached plan.
+func TestReviewServer_ServesOfflineShell(t *testing.T) {
+	dir := t.TempDir()
+	srv, _, _ := newTestReviewServer(t, dir)
+	resp, err := http.Get(srv.URL + "/sw.js")
+	if err != nil {
+		t.Fatalf("sw.js: %v", err)
+	}
+	defer resp.Body.Close()
+	if resp.StatusCode != http.StatusOK {
+		t.Fatalf("sw.js status = %d", resp.StatusCode)
+	}
+	if ct := resp.Header.Get("Content-Type"); !strings.Contains(ct, "javascript") {
+		t.Errorf("sw.js content-type = %q", ct)
+	}
+	body, _ := io.ReadAll(resp.Body)
+	if !strings.Contains(string(body), "caches.match") {
+		t.Error("sw.js must serve the cache-fallback shell")
+	}
+}
+
+// TestReviewServer_PageIsNoStore verifies the live page forbids HTTP caching —
+// offline serving is the service worker's job. Failure prevented: the HTTP
+// cache masks a dead server with a stale page that still looks live, hiding
+// the disconnected state from the reviewer.
+func TestReviewServer_PageIsNoStore(t *testing.T) {
+	dir := t.TempDir()
+	srv, _, _ := newTestReviewServer(t, dir)
+	resp, err := http.Get(srv.URL + "/")
+	if err != nil {
+		t.Fatalf("GET /: %v", err)
+	}
+	defer resp.Body.Close()
+	// the temp dir has no plan.md, so the render may 500 — the header contract
+	// applies to the page route regardless of render success.
+	if cc := resp.Header.Get("Cache-Control"); resp.StatusCode == http.StatusOK && cc != "no-store" {
+		t.Errorf("page Cache-Control = %q, want no-store", cc)
+	}
+}
+
+// TestReviewPortCandidates_PersistedFirstThenStable verifies candidate order:
+// the persisted (tab-parked) port leads, the deterministic stable port and its
+// probe window follow, no dupes, no nonsense ports. Failure prevented: a
+// restart binds a fresh origin while the old one was free, stranding marks.
+func TestReviewPortCandidates_PersistedFirstThenStable(t *testing.T) {
+	const dirName = "2026-07-01-my-plan"
+	stable := plan.StableReviewPort(dirName)
+
+	got := reviewPortCandidates(51000, dirName)
+	if got[0] != 51000 {
+		t.Errorf("persisted port must lead: %v", got)
+	}
+	if got[1] != stable {
+		t.Errorf("stable port must follow persisted: %v", got)
+	}
+	seen := map[int]bool{}
+	for _, p := range got {
+		if p <= 0 || p > 65535 || seen[p] {
+			t.Fatalf("bad candidate list: %v", got)
+		}
+		seen[p] = true
+	}
+	// persisted == stable must dedupe, and zero persisted must drop out.
+	if got := reviewPortCandidates(stable, dirName); got[0] != stable || len(got) != 5 {
+		t.Errorf("dedupe failed: %v", got)
+	}
+	if got := reviewPortCandidates(0, dirName); got[0] != stable {
+		t.Errorf("zero persisted must be skipped: %v", got)
+	}
+}
+
+// TestProbeReviewServer_MatchesOnlyThisPlan verifies the probe recognizes a
+// live server for the SAME plan and rejects everything else (other plan,
+// non-ox listener, dead port). Failure prevented: `ox plan review` "reuses" a
+// stranger's server and never starts its own.
+func TestProbeReviewServer_MatchesOnlyThisPlan(t *testing.T) {
+	dir := t.TempDir()
+	srv, _, _ := newTestReviewServer(t, dir) // serves planDir with Dir=base(dir)
+	port := srv.Listener.Addr().(*net.TCPAddr).Port
+
+	dirName := dir[strings.LastIndex(dir, "/")+1:]
+	if !probeReviewServer(port, dirName) {
+		t.Error("probe must recognize a live server for the same plan")
+	}
+	if probeReviewServer(port, "2026-01-01-some-other-plan") {
+		t.Error("probe must reject a server for a different plan")
+	}
+	// a dead port must not match (bind-then-close to get a free one).
+	l, err := net.Listen("tcp", "127.0.0.1:0")
+	if err != nil {
+		t.Fatalf("listen: %v", err)
+	}
+	dead := l.Addr().(*net.TCPAddr).Port
+	_ = l.Close()
+	if probeReviewServer(dead, dirName) {
+		t.Error("probe must reject a dead port")
+	}
+}

cmd/ox/release_notes.md

@@ -5,6 +5,19 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## [0.11.1] - 2026-07-01
+
+Review feedback on plans is sacred — this release makes sure none of it can be lost, no matter what happens to the review server, the browser tab, or the plan itself.
+
+### Fixed
+
+- **The review page now tells you the moment feedback stops being saved** — if the review server exits (idle timeout, Ctrl-C, a crash), the page immediately shows a clear "offline — feedback is NOT being saved" banner with the exact restart command to copy, instead of looking live while submissions silently fail. Unsent marks stay safely in the browser and are restored on reconnect.
+- **Restarting a review picks up exactly where it left off** — `ox plan review` serves each plan at a stable address, so the tab you already had open reconnects on its own (marks intact) when you restart, and re-running the command against an already-open review reuses the running one instead of starting a stranded twin.
+- **Reloading the plan while the server is down still shows the plan** — the page keeps a local copy of itself, so a reload lands in clearly-marked disconnected mode instead of a browser error screen.
+- **Your marks follow the plan as it changes** — when an AI coworker updates a plan, open review notes are re-anchored onto the content they referred to, so a reworded heading no longer detaches your comment. Notes whose content truly disappeared stay visibly open and keep appearing in every digest — nothing is ever dropped.
+- **Ask about review feedback later** — reviewer notes on plans now appear in local search, so "what did Sam flag on the auth plan?" finds the reviewer's actual words, not just the plan.
+- **Two coworkers resolving review items at the same time no longer lose one of the updates.**
+
 ## [0.11.0] - 2026-07-01
 
 ### Added

go.mod

@@ -42,6 +42,7 @@ require (
 	go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp v1.43.0
 	go.opentelemetry.io/otel/sdk v1.43.0
 	go.opentelemetry.io/otel/trace v1.43.0
+	golang.org/x/net v0.52.0
 	golang.org/x/sync v0.20.0
 	golang.org/x/sys v0.42.0
 	golang.org/x/term v0.41.0
@@ -178,7 +179,6 @@ require (
 	go.yaml.in/yaml/v3 v3.0.4 // indirect
 	golang.org/x/arch v0.8.0 // indirect
 	golang.org/x/crypto v0.49.0 // indirect
-	golang.org/x/net v0.52.0 // indirect
 	google.golang.org/genproto/googleapis/api v0.0.0-20260401024825-9d38bb4040a9 // indirect
 	google.golang.org/genproto/googleapis/rpc v0.0.0-20260401024825-9d38bb4040a9 // indirect
 	google.golang.org/grpc v1.80.0 // indirect