From 4bed6111f018eaf547ebd17921e4a11e8e63335f Mon Sep 17 00:00:00 2001
From: Ed Snible <snible@us.ibm.com>
Date: Thu, 24 Sep 2026 10:34:06 -0400
Subject: [PATCH 1/6] fix: Self-heal corrupt session metadata and bound the
 lock wait
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

`abctl observe` showed no session titles at all, indefinitely, in two
states that never cleared themselves.

A corrupt ~/.cortex/session-metadata.json made claudeHarvester return
nil, so titles were off for the whole run — and every later launch read
the same bad file. Recovery needed `read-claude-sessions --merge=false`,
run by hand, by someone who knew the flag existed.

A wedged flock holder blocked lockMetadata's LOCK_EX forever. observe
harvests on a timer, so every attempt queued behind the same lock with
nothing on screen to say why.

Harvest now rebuilds a file that does not parse, and reports it on
Result.Rebuilt so a caller can say so — the counts cannot, since a
rebuild looks exactly like a first run. A file that could not be READ is
still refused: a permission or I/O failure says nothing about the
contents, and rebuilding over one would replace entries that may be
perfectly good. The two are told apart by a sentinel rather than by
matching ReadMetadata's message, because io.ReadAll can fail with EIO
mid-file and read like a truncated document.

lockMetadata polls LOCK_EX|LOCK_NB to a 2s deadline and then lets the
caller proceed unlocked, on the same best-effort footing as a filesystem
that cannot flock. Past the deadline two harvests can interleave, which
recoverConcurrentEntries narrows; unlocked is strictly worse than locked
and strictly better than wedged.

Two existing tests asserted the old policy and are inverted
deliberately. The destructive case — a rebuild over an unreadable file —
is guarded by tests at both layers, and each was mutation-checked to
confirm it fails when its guard is removed.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Ed Snible <snible@us.ibm.com>
---
 authbridge/authlib/observe/claude/harvest.go  |  86 ++++++++++--
 .../authlib/observe/claude/harvest_test.go    | 101 ++++++++++++--
 .../authlib/observe/claude/lock_unix.go       |  77 ++++++++++-
 .../authlib/observe/claude/lock_unix_test.go  | 129 ++++++++++++++++++
 authbridge/cmd/abctl/cmd_experimental.go      |  16 ++-
 authbridge/cmd/abctl/cmd_experimental_test.go |  60 +++++---
 authbridge/cmd/abctl/cmd_observe_test.go      |  62 +++++++--
 authbridge/cmd/abctl/main.go                  |  37 +++--
 8 files changed, 498 insertions(+), 70 deletions(-)
 create mode 100644 authbridge/authlib/observe/claude/lock_unix_test.go

diff --git a/authbridge/authlib/observe/claude/harvest.go b/authbridge/authlib/observe/claude/harvest.go
index d8406e05b..09be099aa 100644
--- a/authbridge/authlib/observe/claude/harvest.go
+++ b/authbridge/authlib/observe/claude/harvest.go
@@ -19,14 +19,43 @@ import (
 // tree. Read here so a user who has moved it is not told there are no sessions.
 const ConfigDirEnv = "CLAUDE_CONFIG_DIR"
 
-// ErrCorruptMetadata reports that an existing metadata file could not be trusted, so a
-// merge refused rather than rebuilding from scratch and dropping its entries.
+// ErrCorruptMetadata reports that an existing metadata file was there and could not be
+// read, so a merge refused rather than replacing a file whose contents are unknown.
+//
+// NARROWER THAN THE NAME SUGGESTS: a file that reads fine but does not parse no longer
+// comes back here, because Harvest rebuilds it instead. What is left is the file that
+// could not be read at all — a permission or I/O failure — where refusing is the only
+// safe answer, since a rebuild would replace entries that may be perfectly good.
 //
 // Exported because the repair is a CLI affordance: `abctl experimental
 // read-claude-sessions` names --merge=false as the way past, and only the command layer
 // knows its own flags. Callers discriminate with errors.Is.
 var ErrCorruptMetadata = errors.New("corrupt session metadata")
 
+// ErrLockTimeout reports that the metadata lock could not be acquired before its deadline,
+// so the harvest proceeded UNLOCKED rather than not at all.
+//
+// Its own error so a caller can tell "this filesystem cannot flock" from "another process
+// is holding it": both proceed unlocked, but only the second means a concurrent harvest is
+// real and a lost update is possible.
+//
+// Declared here rather than in lock_unix.go so it is part of the package API on every
+// platform — an errors.Is against it must compile where the lock is a no-op too, which is
+// the Windows cross-check lock_other.go exists to keep working.
+var ErrLockTimeout = errors.New("timed out waiting for the session metadata lock")
+
+// errMetadataNotJSON marks the one read failure Harvest can recover from by rebuilding.
+//
+// A SENTINEL RATHER THAN A STRING MATCH on ReadMetadata's message, and rather than
+// treating every non-permission error as a parse failure: io.ReadAll can fail with EIO
+// mid-file, which reads exactly like a truncated document and must not be rebuilt over.
+// Classifying on what the error IS, not on what it is not, keeps the destructive branch
+// reachable only from the one case that is provably a parse failure.
+//
+// Unexported: callers outside this package have no decision to make with it, since
+// Harvest acts on it before they see anything.
+var errMetadataNotJSON = errors.New("metadata is not valid JSON")
+
 // Options selects how much work one harvest does.
 type Options struct {
 	// ConfigDir is the agent config directory to read. Empty resolves via
@@ -47,8 +76,11 @@ type Options struct {
 	//     file is a cache with no eviction, and `--merge=false` is the only thing that prunes it.
 	//   - Merge:false DROPS every entry this harvest did not see, which includes entries from any
 	//     OTHER config dir and any session pruned since. That is what makes it the way to rebuild a
-	//     wrong file, and also why it is not the default: run it with a --dir narrower than the one
-	//     that produced the file and it discards the difference without asking.
+	//     file whose entries are WRONG — a file that parses but says the wrong thing, which nothing
+	//     else can fix — and also why it is not the default: run it with a --dir narrower than the
+	//     one that produced the file and it discards the difference without asking. It is no longer
+	//     needed for a file that does not parse; Harvest rebuilds that one itself, and reports it
+	//     on Result.Rebuilt.
 	Merge bool
 	// Incremental skips transcripts no newer than the entry already recorded for
 	// them, so only recently-touched sessions are parsed.
@@ -95,6 +127,13 @@ type Result struct {
 	// grow, so counting it as recovered drove Kept negative and the subcommand printed
 	// "-1 kept from the existing file". Not part of the Total identity below for the same reason.
 	Replaced int
+	// Rebuilt reports that the existing file did not parse, so this harvest replaced it from
+	// the transcripts instead of merging into it.
+	//
+	// Worth a field because the counts alone cannot show it: a rebuild looks exactly like an
+	// ordinary first run — everything Harvested, nothing Kept — and the entries it dropped
+	// (sessions whose transcripts are gone) leave no trace anywhere for a caller to notice.
+	Rebuilt bool
 	// Meta is what the run wrote: the merged whole under Merge, or just this harvest
 	// otherwise. Keyed by session id.
 	//
@@ -159,16 +198,38 @@ func Harvest(opts Options) (Result, error) {
 	var existing map[string]SessionMetadata
 	if opts.Merge {
 		if existing, err = ReadMetadata(path); err != nil {
-			// Wrapped in a sentinel rather than returned bare. A corrupt file read as
-			// absent would silently rebuild from scratch under the flag whose whole
-			// purpose is not losing entries — the same trap readState exists to close
-			// for claude-code-state.json. The caller names the repair.
-			return res, fmt.Errorf("%w: %w", ErrCorruptMetadata, err)
+			// A FILE THAT DOES NOT PARSE IS REBUILT; a file that could not be READ is
+			// refused. Refusing both is what this used to do, on the reasoning that a
+			// rebuild drops entries the flag exists to keep. What that reasoning missed is
+			// where the entries actually go: the only ones a rebuild loses are those whose
+			// transcripts are gone, and refusing did not preserve those either — it just
+			// deferred the choice onto a user who had to know --merge=false to make it,
+			// while `abctl observe` showed no titles at all until they did. Since every
+			// launch read the same bad file, that state never cleared itself.
+			//
+			// The read failure stays a refusal, and the distinction is the whole safety
+			// argument: a permission or I/O error says nothing about the contents, so
+			// replacing the file there would destroy entries that are very likely intact.
+			if !errors.Is(err, errMetadataNotJSON) {
+				return res, fmt.Errorf("%w: %w", ErrCorruptMetadata, err)
+			}
+			existing = map[string]SessionMetadata{}
+			res.Rebuilt = true
 		}
 	}
 
 	var since map[string]SessionMetadata
-	if opts.Incremental {
+	if opts.Incremental && !res.Rebuilt {
+		// NOT INCREMENTAL OVER A REBUILD, though today nothing observable turns on it: the
+		// rebuild replaced existing with an empty map, so every transcript looks new and the
+		// scan reads them all either way. NO TEST PINS THIS — a mutation removing the
+		// !res.Rebuilt term passes the suite, deliberately recorded here rather than guarded
+		// with a test that would only be asserting the coincidence.
+		//
+		// Kept because the equivalence is a property of the line above, not of this one: give
+		// the rebuild any non-empty baseline — salvaged entries, a defaulted map — and a set
+		// since starts skipping transcripts the rebuild exists to read. Saying "a rebuild is
+		// not incremental" directly costs one term and cannot come apart.
 		since = existing
 	}
 
@@ -1761,7 +1822,10 @@ func ReadMetadata(path string) (map[string]SessionMetadata, error) {
 	}
 	var m map[string]SessionMetadata
 	if uerr := json.Unmarshal(b, &m); uerr != nil {
-		return nil, fmt.Errorf("%s is not valid JSON: %w", path, uerr)
+		// Sentinel-wrapped so Harvest can tell a parse failure from a file it could not
+		// read: only the former is safe to rebuild over. The message keeps the path and
+		// the decoder's own detail, both of which reach the user.
+		return nil, fmt.Errorf("%w: %s is not valid JSON: %w", errMetadataNotJSON, path, uerr)
 	}
 	if m == nil {
 		return map[string]SessionMetadata{}, nil
diff --git a/authbridge/authlib/observe/claude/harvest_test.go b/authbridge/authlib/observe/claude/harvest_test.go
index 92c911758..4eb446dc3 100644
--- a/authbridge/authlib/observe/claude/harvest_test.go
+++ b/authbridge/authlib/observe/claude/harvest_test.go
@@ -222,13 +222,13 @@ func TestHarvest_IncrementalRequiresMerge(t *testing.T) {
 	}
 }
 
-// A corrupt existing file is refused under Merge, distinguishably, so the command layer
-// can name --merge=false as the way past.
-func TestHarvest_CorruptMetadataIsDistinguishable(t *testing.T) {
-	metadataHome(t)
-	cfg := filepath.Join(t.TempDir(), "claude")
-	writeSessionTranscript(t, filepath.Join(cfg, "projects", "-p"), "s1.jsonl",
-		`{"type":"ai-title","aiTitle":"t"}`)
+// writeMetadataFile puts raw bytes at the metadata path, creating the directory.
+//
+// Its own helper because every corrupt-file test needs the same four lines, and the mode
+// matters: SaveMetadata writes 0o600, so a fixture that differs would test a file the
+// product never produces.
+func writeMetadataFile(t *testing.T, body string) string {
+	t.Helper()
 	path, err := SessionMetadataPath()
 	if err != nil {
 		t.Fatal(err)
@@ -236,14 +236,97 @@ func TestHarvest_CorruptMetadataIsDistinguishable(t *testing.T) {
 	if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil {
 		t.Fatal(err)
 	}
-	if err := os.WriteFile(path, []byte("{not json"), 0o600); err != nil {
+	if err := os.WriteFile(path, []byte(body), 0o600); err != nil {
 		t.Fatal(err)
 	}
+	return path
+}
+
+// A file that does not parse is rebuilt from the transcripts rather than refused, so the
+// titles come back on their own instead of waiting for a hand-run --merge=false.
+func TestHarvest_CorruptMetadataIsRebuilt(t *testing.T) {
+	metadataHome(t)
+	cfg := filepath.Join(t.TempDir(), "claude")
+	writeSessionTranscript(t, filepath.Join(cfg, "projects", "-p"), "s1.jsonl",
+		`{"type":"ai-title","aiTitle":"t"}`)
+	path := writeMetadataFile(t, "{not json")
+
+	// Incremental too, which is how `abctl observe` calls it: the rebuild has to clear the
+	// baseline, or a skip test against an empty map is the only thing making this work.
+	res, err := Harvest(Options{ConfigDir: cfg, Merge: true, Incremental: true})
+	if err != nil {
+		t.Fatalf("Harvest: %v", err)
+	}
+	if !res.Rebuilt {
+		t.Error("Rebuilt = false, want true: the caller cannot tell a rebuild from a first run")
+	}
+	if got := res.Meta["s1"].Title; got != "t" {
+		t.Errorf("Meta[s1].Title = %q, want the harvested title", got)
+	}
+	if got := readMetadataFile(t, path)["s1"].Title; got != "t" {
+		t.Errorf("on disk Title = %q, want the file replaced with the rebuild", got)
+	}
+}
 
-	_, err = Harvest(Options{ConfigDir: cfg, Merge: true})
+// An UNREADABLE file is still refused, and left alone. This is the destructive case the
+// rebuild must not reach: a permission or I/O failure says nothing about the contents, so
+// replacing the file there would drop entries that are very likely intact.
+func TestHarvest_UnreadableMetadataIsRefusedNotRebuilt(t *testing.T) {
+	if os.Geteuid() == 0 {
+		t.Skip("root ignores the mode bits this test relies on")
+	}
+	metadataHome(t)
+	cfg := filepath.Join(t.TempDir(), "claude")
+	writeSessionTranscript(t, filepath.Join(cfg, "projects", "-p"), "s1.jsonl",
+		`{"type":"ai-title","aiTitle":"t"}`)
+	// VALID JSON, so the only thing making this unreadable is the mode. A corrupt body here
+	// would let the test pass for the wrong reason if the classification were inverted.
+	path := writeMetadataFile(t, `{"old":{"title":"keep me"}}`)
+	if err := os.Chmod(path, 0o000); err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() { _ = os.Chmod(path, 0o600) })
+
+	res, err := Harvest(Options{ConfigDir: cfg, Merge: true})
 	if !errors.Is(err, ErrCorruptMetadata) {
 		t.Fatalf("err = %v, want it to wrap ErrCorruptMetadata", err)
 	}
+	if res.Rebuilt {
+		t.Error("Rebuilt = true: an unreadable file must not be rebuilt over")
+	}
+	if err := os.Chmod(path, 0o600); err != nil {
+		t.Fatal(err)
+	}
+	if got := readMetadataFile(t, path)["old"].Title; got != "keep me" {
+		t.Errorf("Title = %q, want the untouched entry: the file was replaced", got)
+	}
+}
+
+// A rebuild is still a merge otherwise: it replaces the unparseable file, and the next
+// harvest keeps what it wrote.
+func TestHarvest_RebuildThenMergeKeepsEntries(t *testing.T) {
+	metadataHome(t)
+	cfg := filepath.Join(t.TempDir(), "claude")
+	dir := filepath.Join(cfg, "projects", "-p")
+	writeSessionTranscript(t, dir, "s1.jsonl", `{"type":"ai-title","aiTitle":"one"}`)
+	writeMetadataFile(t, "{not json")
+
+	if _, err := Harvest(Options{ConfigDir: cfg, Merge: true}); err != nil {
+		t.Fatalf("rebuild: %v", err)
+	}
+	writeSessionTranscript(t, dir, "s2.jsonl", `{"type":"ai-title","aiTitle":"two"}`)
+	res, err := Harvest(Options{ConfigDir: cfg, Merge: true})
+	if err != nil {
+		t.Fatalf("second harvest: %v", err)
+	}
+	if res.Rebuilt {
+		t.Error("Rebuilt = true on a file this package just wrote")
+	}
+	for id, want := range map[string]string{"s1": "one", "s2": "two"} {
+		if got := res.Meta[id].Title; got != want {
+			t.Errorf("Meta[%s].Title = %q, want %q", id, got, want)
+		}
+	}
 }
 
 // An entry written concurrently by another run survives this run's save.
diff --git a/authbridge/authlib/observe/claude/lock_unix.go b/authbridge/authlib/observe/claude/lock_unix.go
index 8ad314e08..7f05560c6 100644
--- a/authbridge/authlib/observe/claude/lock_unix.go
+++ b/authbridge/authlib/observe/claude/lock_unix.go
@@ -3,11 +3,35 @@
 package claude
 
 import (
+	"errors"
+	"fmt"
 	"os"
 	"path/filepath"
 	"syscall"
+	"time"
 )
 
+// lockTimeout bounds how long a harvest waits for the metadata lock before proceeding
+// without it.
+//
+// SIZED AGAINST A SCAN, not against a guess: a harvest holds the lock for the length of
+// one transcript scan, measured at ~3ms for a one-file tree and well under a second for
+// the ~180-session tree a laptop accumulates. Two seconds is therefore many times the
+// longest legitimate wait, so expiry means the holder is wedged rather than busy.
+//
+// The cost of it being too short is a lost update, which recoverConcurrentEntries
+// narrows; the cost of it being too long is the bug it exists to fix — no titles at all
+// while `abctl observe` polls a lock nobody will release.
+const lockTimeout = 2 * time.Second
+
+// lockPoll is how often acquisition is retried inside lockTimeout.
+//
+// Polled rather than blocking because the two are exclusive in this API: syscall.Flock
+// either blocks forever (LOCK_EX) or returns at once (LOCK_NB), and there is no
+// deadline variant. 20ms costs at most 100 cheap syscalls across the whole timeout and
+// adds at most 20ms to an uncontended handoff.
+const lockPoll = 20 * time.Millisecond
+
 // lockMetadata takes an exclusive advisory lock covering the read-modify-write of the metadata
 // file, and returns the release.
 //
@@ -24,13 +48,28 @@ import (
 // replaces the metadata file itself. Locking the metadata file would lock an inode the rename is
 // about to detach, protecting nothing.
 //
-// Blocking, with no timeout. A harvest holds this for the length of one scan, and the failure mode
-// of a timeout here is the lost update this exists to prevent; a caller that cannot wait should not
-// be harvesting. A crashed holder releases on process exit, since the kernel owns the lock.
+// BOUNDED BY lockTimeout, then it gives up and lets the caller proceed unlocked. This used to
+// block indefinitely, on the reasoning that a caller who cannot wait should not be harvesting.
+// The case that reasoning did not cover is a holder that never releases — not a crash, which the
+// kernel cleans up on process exit, but a process still alive and stuck. `abctl observe` harvests
+// on a timer, so every later attempt queued behind the same lock and the viewer showed no titles
+// at all, indefinitely, with nothing on screen to say why.
+//
+// What the bound costs: past the deadline two harvests can interleave, and the loser's entries can
+// be dropped by the winner's rename. That is the lost update this lock exists to prevent, now
+// possible again in the one case where the alternative was no titles ever.
+// recoverConcurrentEntries narrows it — it re-reads after saving and takes back what another run
+// wrote — but it is a single re-read, so it cannot converge against a run that renames inside its
+// window. Unlocked is strictly worse than locked and strictly better than wedged.
 //
 // Errors are returned rather than swallowed so the caller can proceed UNLOCKED instead of refusing
 // to harvest: a filesystem that cannot flock (some network mounts) should still get titles, on the
-// same best-effort footing as before this existed.
+// same best-effort footing as before this existed. A timeout joins that path, reported as
+// ErrLockTimeout so the two are distinguishable.
+//
+// The release func is nil on every error return, which the caller relies on to decide whether to
+// defer it — see Harvest's `if lerr == nil` gate. Returning a no-op func alongside an error would
+// make that gate look optional, and it is not.
 func lockMetadata(path string) (func(), error) {
 	lockPath := filepath.Join(filepath.Dir(path), "."+filepath.Base(path)+".lock")
 	if err := os.MkdirAll(filepath.Dir(lockPath), 0o700); err != nil {
@@ -40,9 +79,33 @@ func lockMetadata(path string) (func(), error) {
 	if err != nil {
 		return nil, err
 	}
-	if err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX); err != nil {
-		_ = f.Close()
-		return nil, err
+	// LOCK_NB polled to a deadline. EWOULDBLOCK (EAGAIN on Linux, where they are the same errno)
+	// is the one retryable answer: it means held, not broken. Every other errno is a real failure
+	// and returns at once, exactly as the blocking call used to.
+	//
+	// UNTESTED BRANCH, said out loud rather than left as a silent hole: no test distinguishes this
+	// discrimination from "retry on every errno", because provoking a non-EWOULDBLOCK flock error
+	// needs a filesystem that refuses flock outright (some network mounts) — not something a unit
+	// test can conjure. A mutation that retries every errno passes the whole suite. What it would
+	// cost in production is the timeout spent sleeping against an error that will never change,
+	// then reported as a lock timeout rather than as the ENOLCK it was.
+	deadline := time.Now().Add(lockTimeout)
+	for {
+		err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX|syscall.LOCK_NB)
+		if err == nil {
+			break
+		}
+		if !errors.Is(err, syscall.EWOULDBLOCK) {
+			_ = f.Close()
+			return nil, err
+		}
+		if !time.Now().Before(deadline) {
+			// Closed here too. The caller proceeds unlocked and never sees this descriptor, so
+			// leaking it would leak one per harvest — and `abctl observe` harvests on a timer.
+			_ = f.Close()
+			return nil, fmt.Errorf("%w after %s", ErrLockTimeout, lockTimeout)
+		}
+		time.Sleep(lockPoll)
 	}
 	return func() {
 		// Unlock before close, though closing the descriptor would release it anyway: being
diff --git a/authbridge/authlib/observe/claude/lock_unix_test.go b/authbridge/authlib/observe/claude/lock_unix_test.go
new file mode 100644
index 000000000..db567c174
--- /dev/null
+++ b/authbridge/authlib/observe/claude/lock_unix_test.go
@@ -0,0 +1,129 @@
+//go:build unix
+
+package claude
+
+import (
+	"errors"
+	"os"
+	"path/filepath"
+	"syscall"
+	"testing"
+	"time"
+)
+
+// metadataLockPath is where lockMetadata puts its sibling lock file.
+//
+// Duplicated from lock_unix.go on purpose: a test that derived the path by calling the
+// product code could not catch the path moving, which is the thing that would silently
+// stop the lock from serialising anything.
+func metadataLockPath(t *testing.T, path string) string {
+	t.Helper()
+	return filepath.Join(filepath.Dir(path), "."+filepath.Base(path)+".lock")
+}
+
+// holdMetadataLock takes the lock from the test process and releases it on cleanup.
+//
+// Flock is per-open-file-description, not per-process, so a second descriptor here does
+// contend with the product's — which is what makes an in-process test of this possible at
+// all, without the subprocess machinery TestHarvest_ConcurrentRunsLoseNothing needs.
+func holdMetadataLock(t *testing.T, path string) {
+	t.Helper()
+	lockPath := metadataLockPath(t, path)
+	if err := os.MkdirAll(filepath.Dir(lockPath), 0o700); err != nil {
+		t.Fatal(err)
+	}
+	f, err := os.OpenFile(lockPath, os.O_CREATE|os.O_RDWR, 0o600)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX|syscall.LOCK_NB); err != nil {
+		_ = f.Close()
+		t.Fatalf("could not take the lock the test depends on holding: %v", err)
+	}
+	t.Cleanup(func() {
+		_ = syscall.Flock(int(f.Fd()), syscall.LOCK_UN)
+		_ = f.Close()
+	})
+}
+
+// A held lock is waited for, then given up on — not waited for forever. This is the
+// wedged-holder case: before the timeout, `abctl observe` queued every harvest behind a
+// lock nobody would release and showed no titles at all, indefinitely.
+func TestLockMetadata_TimesOutAndReportsIt(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "session-metadata.json")
+	holdMetadataLock(t, path)
+
+	start := time.Now()
+	unlock, err := lockMetadata(path)
+	elapsed := time.Since(start)
+
+	if !errors.Is(err, ErrLockTimeout) {
+		if unlock != nil {
+			unlock()
+		}
+		t.Fatalf("err = %v, want it to wrap ErrLockTimeout", err)
+	}
+	// Nil, not a no-op: Harvest's `if lerr == nil` gate is what keeps this from being
+	// called, and a non-nil func here would make that gate look optional.
+	if unlock != nil {
+		t.Error("unlock is non-nil alongside an error")
+	}
+	if elapsed < lockTimeout {
+		t.Errorf("gave up after %s, before the %s deadline", elapsed, lockTimeout)
+	}
+	// Generous: this asserts it is bounded at all, not that the sleep is precise.
+	if elapsed > 4*lockTimeout {
+		t.Errorf("took %s, far past the %s deadline", elapsed, lockTimeout)
+	}
+}
+
+// An uncontended lock is still taken, and released, at once.
+func TestLockMetadata_UncontendedIsFast(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "session-metadata.json")
+
+	start := time.Now()
+	unlock, err := lockMetadata(path)
+	if err != nil {
+		t.Fatalf("lockMetadata: %v", err)
+	}
+	if unlock == nil {
+		t.Fatal("unlock is nil with no error")
+	}
+	if elapsed := time.Since(start); elapsed >= lockTimeout {
+		t.Errorf("uncontended acquisition took %s, want well under %s", elapsed, lockTimeout)
+	}
+	unlock()
+
+	// Released, so it can be taken again. Without this the test would pass against a
+	// release that does nothing.
+	again, err := lockMetadata(path)
+	if err != nil {
+		t.Fatalf("second lockMetadata after release: %v", err)
+	}
+	again()
+}
+
+// THE POINT OF THE TIMEOUT, at the level the bug was reported at: a wedged holder no
+// longer stops the titles from appearing. The harvest proceeds unlocked and writes.
+func TestHarvest_ProceedsWhenTheLockIsHeld(t *testing.T) {
+	metadataHome(t)
+	cfg := filepath.Join(t.TempDir(), "claude")
+	writeSessionTranscript(t, filepath.Join(cfg, "projects", "-p"), "s1.jsonl",
+		`{"type":"ai-title","aiTitle":"t"}`)
+	path, err := SessionMetadataPath()
+	if err != nil {
+		t.Fatal(err)
+	}
+	holdMetadataLock(t, path)
+
+	res, err := Harvest(Options{ConfigDir: cfg, Merge: true})
+	if err != nil {
+		t.Fatalf("Harvest: %v", err)
+	}
+	if got := res.Meta["s1"].Title; got != "t" {
+		t.Errorf("Meta[s1].Title = %q, want the harvest to have run unlocked", got)
+	}
+	if got := readMetadataFile(t, path)["s1"].Title; got != "t" {
+		t.Errorf("on disk Title = %q, want the harvest to have written", got)
+	}
+}
diff --git a/authbridge/cmd/abctl/cmd_experimental.go b/authbridge/cmd/abctl/cmd_experimental.go
index ea0c36ff6..cf4f58b1b 100644
--- a/authbridge/cmd/abctl/cmd_experimental.go
+++ b/authbridge/cmd/abctl/cmd_experimental.go
@@ -145,12 +145,11 @@ func runReadClaudeSessions(args []string, stdout, stderr io.Writer) int {
 	res, err := claude.Harvest(claude.Options{ConfigDir: *dir, Merge: *merge})
 	if err != nil {
 		if errors.Is(err, claude.ErrCorruptMetadata) {
-			// Refused rather than treated as empty. A corrupt file read as absent would
-			// silently rebuild from scratch under the flag whose whole purpose is not
-			// losing entries — the same trap readState exists to close for
-			// claude-code-state.json. Name the repair, and name the way past it.
+			// NOW ONLY THE UNREADABLE FILE reaches here: one that does not parse is rebuilt by
+			// Harvest itself. This one could not be read at all, so --merge=false is no longer
+			// the thing to suggest — it would hit the same read. Name what a human can do.
 			fmt.Fprintf(stderr, "abctl: %v\n"+
-				"  Fix or move the file, or re-run with --merge=false to rebuild it.\n", err)
+				"  Fix the file's permissions, or move it aside and re-run.\n", err)
 			return 1
 		}
 		fmt.Fprintf(stderr, "abctl: %v\n", err)
@@ -175,6 +174,13 @@ func runReadClaudeSessions(args []string, stdout, stderr io.Writer) int {
 		fmt.Fprintf(stderr, "abctl: merged %d entry(s) written concurrently by another run\n", res.Recovered)
 	}
 
+	// Said out loud because the counts cannot show it: a rebuild reports everything harvested
+	// and nothing kept, which is exactly what a first run reports. The entries it dropped —
+	// sessions whose transcripts are gone — leave no trace for the operator to notice.
+	if res.Rebuilt {
+		fmt.Fprintf(stderr, "abctl: the existing file could not be parsed; rebuilt it from %s\n", res.ConfigDir)
+	}
+
 	// Reported rather than silent: the count is the only way to notice that a wrong
 	// --dir found nothing, and zero is not an error — a machine that has never run
 	// Claude Code legitimately has no transcripts.
diff --git a/authbridge/cmd/abctl/cmd_experimental_test.go b/authbridge/cmd/abctl/cmd_experimental_test.go
index 04914b346..815374a33 100644
--- a/authbridge/cmd/abctl/cmd_experimental_test.go
+++ b/authbridge/cmd/abctl/cmd_experimental_test.go
@@ -381,10 +381,9 @@ func TestReadClaudeSessions_MergePrefersTheFreshHarvest(t *testing.T) {
 	}
 }
 
-// A corrupt existing file must not be read as empty under --merge: that would rebuild
-// from scratch under the flag whose purpose is not losing entries. Refuse, and name
-// the way past it.
-func TestReadClaudeSessions_MergeRefusesACorruptFile(t *testing.T) {
+// A file that does not parse is rebuilt in place, without --merge=false and without the
+// operator having to know that flag exists. Reported, because the counts cannot show it.
+func TestReadClaudeSessions_MergeRebuildsACorruptFile(t *testing.T) {
 	home := prefsHome(t)
 	path := filepath.Join(home, tui.SessionMetadataRel)
 	if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil {
@@ -399,25 +398,50 @@ func TestReadClaudeSessions_MergeRefusesACorruptFile(t *testing.T) {
 		`{"type":"user","cwd":"/w"}`)
 
 	var out, errb bytes.Buffer
-	if code := runExperimental([]string{"read-claude-sessions", "--dir", cfg}, &out, &errb); code != 1 {
-		t.Errorf("exit = %d, want 1", code)
+	if code := runExperimental([]string{"read-claude-sessions", "--dir", cfg}, &out, &errb); code != 0 {
+		t.Fatalf("exit = %d, want 0: %s", code, errb.String())
 	}
-	if !strings.Contains(errb.String(), "--merge=false") {
-		t.Errorf("stderr does not name the way past it: %q", errb.String())
+	if !strings.Contains(errb.String(), "rebuilt it") {
+		t.Errorf("stderr does not report the rebuild: %q", errb.String())
 	}
-	// The bad file is left alone rather than overwritten, so it can still be repaired.
-	b, err := os.ReadFile(path)
-	if err != nil || string(b) != "{not json" {
-		t.Errorf("the corrupt file was modified: %q, %v", b, err)
+	if _, ok := readMetadataFile(t, path)["fresh"]; !ok {
+		t.Error("the corrupt file was not rebuilt")
 	}
+}
 
-	// --merge=false is that way past it.
-	var out2, errb2 bytes.Buffer
-	if code := runExperimental([]string{"read-claude-sessions", "--dir", cfg, "--merge=false"}, &out2, &errb2); code != 0 {
-		t.Fatalf("--merge=false exit = %d: %s", code, errb2.String())
+// A file that could not be READ is still refused, and left alone. The rebuild must not
+// reach this case: a permission failure says nothing about the contents.
+func TestReadClaudeSessions_MergeRefusesAnUnreadableFile(t *testing.T) {
+	if os.Geteuid() == 0 {
+		t.Skip("root ignores the mode bits this test relies on")
 	}
-	if _, ok := readMetadataFile(t, path)["fresh"]; !ok {
-		t.Error("--merge=false did not rebuild the file")
+	home := prefsHome(t)
+	path := filepath.Join(home, tui.SessionMetadataRel)
+	if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil {
+		t.Fatal(err)
+	}
+	// Valid JSON, so the mode is the only thing making it unreadable.
+	if err := os.WriteFile(path, []byte(`{"old":{"title":"keep me"}}`), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.Chmod(path, 0o000); err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() { _ = os.Chmod(path, 0o600) })
+
+	cfg := filepath.Join(t.TempDir(), "claude")
+	writeSessionTranscript(t, filepath.Join(cfg, "projects", "-p"), "fresh.jsonl",
+		`{"type":"user","cwd":"/w"}`)
+
+	var out, errb bytes.Buffer
+	if code := runExperimental([]string{"read-claude-sessions", "--dir", cfg}, &out, &errb); code != 1 {
+		t.Errorf("exit = %d, want 1", code)
+	}
+	if err := os.Chmod(path, 0o600); err != nil {
+		t.Fatal(err)
+	}
+	if got := readMetadataFile(t, path)["old"].Title; got != "keep me" {
+		t.Errorf("Title = %q, want the untouched entry: the file was replaced", got)
 	}
 }
 
diff --git a/authbridge/cmd/abctl/cmd_observe_test.go b/authbridge/cmd/abctl/cmd_observe_test.go
index 2e7bc071e..e0539a5fc 100644
--- a/authbridge/cmd/abctl/cmd_observe_test.go
+++ b/authbridge/cmd/abctl/cmd_observe_test.go
@@ -319,16 +319,13 @@ func TestClaudeHarvester_SurvivesAnUnreadableConfigDir(t *testing.T) {
 	}
 }
 
-// A corrupt metadata file names the repair, and disables the harvest rather than running it.
+// A file that does not parse no longer disables the harvest: Harvest rebuilds it, and the
+// titles arrive on their own.
 //
-// Checked BEFORE the TUI starts precisely so the message can be printed at all: the harvest now
-// runs after the alt screen goes up, where a warning would corrupt the frame. It is also the one
-// failure that cannot clear itself, since every launch reads the same bad file. observe has to
-// name a different repair from the subcommand's --merge=false, which is not a flag it has.
-//
-// (This test existed before the async change and was lost in an edit; restored here in the
-// factory's shape, which is also where it belongs now that the check moved.)
-func TestClaudeHarvester_CorruptFileNamesTheRepair(t *testing.T) {
+// This is the reported bug at the level the user sees it. The pre-flight used to return nil
+// here, so `abctl observe` showed no titles at all for the whole run, and every later launch
+// read the same bad file — the state never cleared itself without a hand-run subcommand.
+func TestClaudeHarvester_CorruptFileIsRebuiltNotFatal(t *testing.T) {
 	home := prefsHome(t)
 	cfg := filepath.Join(t.TempDir(), "claude")
 	writeSessionTranscript(t, filepath.Join(cfg, "projects", "-p"), "s1.jsonl",
@@ -343,17 +340,58 @@ func TestClaudeHarvester_CorruptFileNamesTheRepair(t *testing.T) {
 		t.Fatal(err)
 	}
 
+	var warn bytes.Buffer
+	h := claudeHarvester(&warn)
+	if h == nil {
+		t.Fatal("a file that does not parse must not disable the harvest: Harvest rebuilds it")
+	}
+	// Asserted through the harvester rather than the file, because a harvester that runs but
+	// yields nothing would leave the TITLE column exactly as empty as no harvester at all.
+	got, err := h()
+	if err != nil {
+		t.Fatalf("harvest: %v", err)
+	}
+	if got["s1"].Title != "t" {
+		t.Errorf("Title = %q, want %q: the rebuild did not reach the transcripts", got["s1"].Title, "t")
+	}
+}
+
+// A file that cannot be READ still disables the harvest, and still names a repair. Harvest
+// refuses that one — a permission failure says nothing about the contents — so the pre-flight
+// is the only place it can be said before the alt screen goes up.
+func TestClaudeHarvester_UnreadableFileNamesTheRepair(t *testing.T) {
+	if os.Geteuid() == 0 {
+		t.Skip("root ignores the mode bits this test relies on")
+	}
+	home := prefsHome(t)
+	cfg := filepath.Join(t.TempDir(), "claude")
+	writeSessionTranscript(t, filepath.Join(cfg, "projects", "-p"), "s1.jsonl",
+		`{"type":"ai-title","aiTitle":"t"}`)
+	t.Setenv("CLAUDE_CONFIG_DIR", cfg)
+
+	path := filepath.Join(home, tui.SessionMetadataRel)
+	if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(path, []byte(`{"old":{"title":"keep me"}}`), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.Chmod(path, 0o000); err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() { _ = os.Chmod(path, 0o600) })
+
 	var warn bytes.Buffer
 	if h := claudeHarvester(&warn); h != nil {
-		t.Error("a corrupt file must disable the harvest, not be silently merged over")
+		t.Error("an unreadable file must disable the harvest, not be rebuilt over")
 	}
 	got := warn.String()
-	if !strings.Contains(got, "read-claude-sessions --merge=false") {
+	if !strings.Contains(got, "mv ") {
 		t.Errorf("the warning does not name the repair:\n%s", got)
 	}
 	// The command it names must be runnable as printed, on its own line.
 	for _, line := range strings.Split(got, "\n") {
-		if strings.Contains(line, "abctl experimental") && !strings.HasPrefix(strings.TrimSpace(line), "abctl experimental") {
+		if strings.Contains(line, "mv ") && !strings.HasPrefix(strings.TrimSpace(line), "mv ") {
 			t.Errorf("the repair command is not on a line of its own: %q", line)
 		}
 	}
diff --git a/authbridge/cmd/abctl/main.go b/authbridge/cmd/abctl/main.go
index fc9910198..8407d2584 100644
--- a/authbridge/cmd/abctl/main.go
+++ b/authbridge/cmd/abctl/main.go
@@ -13,9 +13,11 @@ package main
 
 import (
 	"context"
+	"errors"
 	"flag"
 	"fmt"
 	"io"
+	"io/fs"
 	"os"
 	"os/exec"
 	"os/signal"
@@ -239,14 +241,33 @@ func claudeHarvester(warn io.Writer) tui.HarvestFunc {
 		fmt.Fprintf(warn, "abctl: not naming sessions from Claude Code: %v\n", err)
 		return nil
 	} else if _, err := claude.ReadMetadata(path); err != nil {
-		// A corrupt file is the one failure that cannot clear itself: every launch reads the
-		// same bad file. ErrCorruptMetadata is exported so a caller can name the fix, and this
-		// caller has to name a different one from the subcommand's --merge=false, which is not
-		// a flag `abctl observe` has.
-		fmt.Fprintf(warn, "abctl: not naming sessions from Claude Code: %v\n"+
-			"  Fix or move the file, or rebuild it:\n"+
-			"    abctl experimental read-claude-sessions --merge=false\n", err)
-		return nil
+		// A FILE THAT DOES NOT PARSE IS NO LONGER CHECKED FOR HERE, because Harvest rebuilds it
+		// on the background goroutine and the titles arrive on their own. What is left is the
+		// file Harvest still refuses: one that could not be READ. That one repeats every launch
+		// and needs a human — a permission fixed, a disk looked at — so it is worth the one
+		// thing this position can still do, which is print before the alt screen goes up.
+		//
+		// Read-and-discard rather than a stat: the failure being checked for is the read itself,
+		// and Harvest's own read is the one that decides. Not a wasted read either way — it was
+		// already here, and what changed is only which of its errors is fatal.
+		if errors.Is(err, fs.ErrPermission) {
+			fmt.Fprintf(warn, "abctl: not naming sessions from Claude Code: %v\n"+
+				"  Fix the file's permissions, or move it aside:\n"+
+				"    mv %s %s.bad\n", err, path, path)
+			return nil
+		}
+		// EVERY OTHER READ ERROR FALLS THROUGH SILENTLY rather than disabling titles or warning
+		// here, because this position cannot tell which of them Harvest will recover from. The
+		// sentinel that says "this one does not parse" is unexported, and exporting it would
+		// widen authlib's API to let this pre-flight re-derive a decision Harvest makes a few
+		// lines later anyway.
+		//
+		// So the split is by what a human can DO, not by what went wrong: a permission failure
+		// names an action and repeats every launch, which is worth printing before the alt screen
+		// goes up. Not warning about the rest is the deliberate half — the common case among them
+		// is the file that does not parse, which heals itself moments later, so a line here would
+		// tell the operator titles are in trouble and then hand them titles. A harvest that does
+		// go on to fail reports itself through the viewer's own error path.
 	}
 	return func() (map[string]tui.SessionMetadata, error) {
 		// Incremental, unlike `abctl experimental read-claude-sessions`: that command's subject

From d9c5e1ecf5cc4b8d25f6af5085b95c7873582219 Mon Sep 17 00:00:00 2001
From: Ed Snible <snible@us.ibm.com>
Date: Thu, 24 Sep 2026 10:50:29 -0400
Subject: [PATCH 2/6] fix: Size the metadata lock deadline against a queue, not
 one scan
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The 2s deadline added in the previous commit fired on a healthy machine.
CI's authlib job failed TestHarvest_ConcurrentRunsLoseNothing with "file
holds 1 of 6": five of six concurrent harvests exceeded the deadline,
proceeded unlocked, and had their entries erased by the winner's rename.

The number was wrong, not the mechanism. 2s was derived as "many times
one scan" from a ~3ms single-file measurement, but the wait is the whole
queue ahead of you, not the holder alone. Six concurrent harvests of a
padded tree serialize to ~190ms on a developer laptop — a 10x margin,
which is why five local -count runs passed and said nothing useful — and
blow past 2s on a shared runner.

The two costs are not symmetric, and that is what sets the size. Too long
only delays a harvest that was already doomed: the wedged holder this
bound exists for, where no wait succeeds. Too short corrupts a HEALTHY
run, because expiry means proceeding unlocked. recoverConcurrentEntries
does not cover for that — it is a single re-read, and by its own docs
cannot converge when more than one rename lands in its window, which is
exactly the many-writer state a short deadline creates. So the deadline
must mean "no wait would have worked" and never "this machine is slow
today": 30s, far past any queue this file can produce.

Reproduced deterministically before changing it, by shrinking the
deadline to 40ms — same failure, same shape.

lockTimeout becomes a var so the two tests that wait it out can shrink it
to 40ms; the behaviour under test (expiry happens, is reported, returns
no unlock func) is identical at either value, and the real deadline would
add a minute to the package for no extra coverage. Nothing outside the
tests assigns it.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Ed Snible <snible@us.ibm.com>
---
 .../authlib/observe/claude/lock_unix.go       | 54 +++++++++++++------
 .../authlib/observe/claude/lock_unix_test.go  | 15 ++++++
 2 files changed, 53 insertions(+), 16 deletions(-)

diff --git a/authbridge/authlib/observe/claude/lock_unix.go b/authbridge/authlib/observe/claude/lock_unix.go
index 7f05560c6..7c6edbf93 100644
--- a/authbridge/authlib/observe/claude/lock_unix.go
+++ b/authbridge/authlib/observe/claude/lock_unix.go
@@ -14,22 +14,39 @@ import (
 // lockTimeout bounds how long a harvest waits for the metadata lock before proceeding
 // without it.
 //
-// SIZED AGAINST A SCAN, not against a guess: a harvest holds the lock for the length of
-// one transcript scan, measured at ~3ms for a one-file tree and well under a second for
-// the ~180-session tree a laptop accumulates. Two seconds is therefore many times the
-// longest legitimate wait, so expiry means the holder is wedged rather than busy.
+// THE TWO COSTS ARE NOT SYMMETRIC, which is what sets the size. Too long only delays a
+// harvest that was going to be wrong anyway — the wedged case this bound exists for, where
+// no wait of any length succeeds. Too short corrupts a HEALTHY run: expiry means proceeding
+// unlocked, and an unlocked harvest can have its whole contribution erased by another run's
+// rename. So the deadline has to sit far above the slowest legitimate wait, and the penalty
+// for overshooting is measured in seconds of a background goroutine nobody is watching.
 //
-// The cost of it being too short is a lost update, which recoverConcurrentEntries
-// narrows; the cost of it being too long is the bug it exists to fix — no titles at all
-// while `abctl observe` polls a lock nobody will release.
-const lockTimeout = 2 * time.Second
+// SIZED AGAINST A QUEUE, not a single scan, because the wait is the queue ahead of you and
+// not the holder alone. That was the sizing error in the first version of this: 2s was chosen
+// as "many times one scan" from a ~3ms one-file measurement, and it fell over as soon as
+// several harvests contended on a slow machine. Six concurrent harvests of a padded tree took
+// ~190ms serialized on a developer laptop and blew straight past 2s on a shared CI runner —
+// the package's own concurrency test failed, with five of six children losing every entry.
+// A runner is not an exotic environment; it is the slowest machine this code routinely runs on
+// and therefore the one that sets the number.
+//
+// A VAR RATHER THAN A CONST only so the tests can shrink it: waiting the real deadline twice
+// would add a minute to the package for no extra coverage, and a test that asserts the logic at
+// 40ms asserts exactly the same logic. Nothing outside the tests assigns it.
+//
+// 30s is deliberately far past any queue this file can produce. `abctl observe` harvests one
+// tree per tick, `read-claude-sessions` is one process, and the realistic worst case is a
+// handful of viewers plus a manual run — a queue of seconds, not minutes, even derated for a
+// loaded runner. What 30s buys is that reaching it means no wait would have worked.
+var lockTimeout = 30 * time.Second
 
 // lockPoll is how often acquisition is retried inside lockTimeout.
 //
 // Polled rather than blocking because the two are exclusive in this API: syscall.Flock
 // either blocks forever (LOCK_EX) or returns at once (LOCK_NB), and there is no
-// deadline variant. 20ms costs at most 100 cheap syscalls across the whole timeout and
-// adds at most 20ms to an uncontended handoff.
+// deadline variant. 20ms adds at most 20ms to an uncontended handoff, and costs 1500
+// cheap syscalls across a full 30s timeout — paid only by a harvest that is already
+// losing, since a lock that frees up is acquired on the next tick.
 const lockPoll = 20 * time.Millisecond
 
 // lockMetadata takes an exclusive advisory lock covering the read-modify-write of the metadata
@@ -55,12 +72,17 @@ const lockPoll = 20 * time.Millisecond
 // on a timer, so every later attempt queued behind the same lock and the viewer showed no titles
 // at all, indefinitely, with nothing on screen to say why.
 //
-// What the bound costs: past the deadline two harvests can interleave, and the loser's entries can
-// be dropped by the winner's rename. That is the lost update this lock exists to prevent, now
-// possible again in the one case where the alternative was no titles ever.
-// recoverConcurrentEntries narrows it — it re-reads after saving and takes back what another run
-// wrote — but it is a single re-read, so it cannot converge against a run that renames inside its
-// window. Unlocked is strictly worse than locked and strictly better than wedged.
+// What the bound costs, and why the deadline is generous rather than tight: past it two harvests
+// interleave, and the loser's entries can be erased wholesale by the winner's rename. That is the
+// very lost update this lock exists to prevent, so expiry must mean "no wait would have worked"
+// and never "this machine is slow today". recoverConcurrentEntries is a weaker backstop than it
+// looks — a SINGLE re-read after saving, which by its own documentation cannot converge when more
+// than one rename lands inside its window, i.e. exactly the many-writer case a short deadline
+// creates. It does not cover for a deadline that fires under ordinary contention.
+//
+// So: unlocked is strictly better than wedged, and strictly worse than locked — which makes the
+// bound worth having and worth sizing so that only the wedged case ever reaches it. See
+// lockTimeout for the measurements that set the number.
 //
 // Errors are returned rather than swallowed so the caller can proceed UNLOCKED instead of refusing
 // to harvest: a filesystem that cannot flock (some network mounts) should still get titles, on the
diff --git a/authbridge/authlib/observe/claude/lock_unix_test.go b/authbridge/authlib/observe/claude/lock_unix_test.go
index db567c174..bed51655c 100644
--- a/authbridge/authlib/observe/claude/lock_unix_test.go
+++ b/authbridge/authlib/observe/claude/lock_unix_test.go
@@ -46,10 +46,24 @@ func holdMetadataLock(t *testing.T, path string) {
 	})
 }
 
+// shortenLockTimeout shrinks the deadline for one test and restores it after.
+//
+// The deadline's production VALUE is a judgement about slow machines (see lockTimeout); what
+// these tests check is the behaviour at it — that expiry happens, is reported, and returns no
+// unlock func. That behaviour is identical at 40ms, and waiting the real 30s twice would add a
+// minute to the package to re-learn nothing.
+func shortenLockTimeout(t *testing.T) {
+	t.Helper()
+	was := lockTimeout
+	lockTimeout = 40 * time.Millisecond
+	t.Cleanup(func() { lockTimeout = was })
+}
+
 // A held lock is waited for, then given up on — not waited for forever. This is the
 // wedged-holder case: before the timeout, `abctl observe` queued every harvest behind a
 // lock nobody would release and showed no titles at all, indefinitely.
 func TestLockMetadata_TimesOutAndReportsIt(t *testing.T) {
+	shortenLockTimeout(t)
 	path := filepath.Join(t.TempDir(), "session-metadata.json")
 	holdMetadataLock(t, path)
 
@@ -106,6 +120,7 @@ func TestLockMetadata_UncontendedIsFast(t *testing.T) {
 // THE POINT OF THE TIMEOUT, at the level the bug was reported at: a wedged holder no
 // longer stops the titles from appearing. The harvest proceeds unlocked and writes.
 func TestHarvest_ProceedsWhenTheLockIsHeld(t *testing.T) {
+	shortenLockTimeout(t)
 	metadataHome(t)
 	cfg := filepath.Join(t.TempDir(), "claude")
 	writeSessionTranscript(t, filepath.Join(cfg, "projects", "-p"), "s1.jsonl",

From 63bda8e9732fd31f47c4785da4f0d624ac65990f Mon Sep 17 00:00:00 2001
From: Ed Snible <snible@us.ibm.com>
Date: Thu, 24 Sep 2026 11:05:28 -0400
Subject: [PATCH 3/6] fix: Refuse an oversized metadata file, and report a lock
 timeout
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Two review findings.

A valid metadata file over the 16 MiB read cap was silently destroyed.
ReadMetadata truncates at the cap, the truncated bytes fail to decode, the
previous commit classified that as errMetadataNotJSON, and Harvest rebuilds
over parse failures. Measured: a valid 18,496,001-byte file with 17,000
entries became 412 bytes with 1 entry, err=nil, Rebuilt=true. Pre-PR the
same file was refused.

The cap is now checked before the decode and reported as its own failure,
carrying neither sentinel — not errMetadataNotJSON, so it cannot be
rebuilt over; not a bare read error, because nothing is wrong with the
bytes. The stale comment claiming truncation was safe is replaced. The
guard is mutation-checked both ways: removing the check and re-classifying
it as errMetadataNotJSON each fail the new test.

Harvest also discarded the lock error, so ErrLockTimeout had no consumer
and a lost update was silent — the one failure the lock exists to prevent
was invisible from outside. It lands on Result.LockTimedOut, alongside
Rebuilt, and read-claude-sessions reports it. Tested both ways: set when
the lock is held, and not set when it is free.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Ed Snible <snible@us.ibm.com>
---
 authbridge/authlib/observe/claude/harvest.go  | 29 ++++++++--
 .../authlib/observe/claude/harvest_test.go    | 55 +++++++++++++++++++
 .../authlib/observe/claude/lock_unix_test.go  | 22 ++++++++
 authbridge/cmd/abctl/cmd_experimental.go      |  6 ++
 4 files changed, 106 insertions(+), 6 deletions(-)

diff --git a/authbridge/authlib/observe/claude/harvest.go b/authbridge/authlib/observe/claude/harvest.go
index 09be099aa..389e30a0a 100644
--- a/authbridge/authlib/observe/claude/harvest.go
+++ b/authbridge/authlib/observe/claude/harvest.go
@@ -134,6 +134,12 @@ type Result struct {
 	// ordinary first run — everything Harvested, nothing Kept — and the entries it dropped
 	// (sessions whose transcripts are gone) leave no trace anywhere for a caller to notice.
 	Rebuilt bool
+	// LockTimedOut reports that the metadata lock could not be taken before its deadline, so
+	// this harvest ran unlocked and a concurrent run's rename may erase its entries.
+	//
+	// A field because the alternative is silence: without it, the one failure mode this lock
+	// exists to prevent is invisible from outside, and "some titles vanished" has no diagnosis.
+	LockTimedOut bool
 	// Meta is what the run wrote: the merged whole under Merge, or just this harvest
 	// otherwise. Keyed by session id.
 	//
@@ -186,9 +192,15 @@ func Harvest(opts Options) (Result, error) {
 	// Best-effort: a filesystem that cannot flock still harvests, unlocked, on the footing every
 	// platform had before this. Not taken without Merge, where there is nothing to lose — that path
 	// replaces the file by definition.
+	// The error is recorded, not returned: proceeding unlocked is the deliberate fallback, so a
+	// caller that wants to report the risk can, and one that does not still harvests.
 	if opts.Merge {
-		if unlock, lerr := lockMetadata(path); lerr == nil {
+		unlock, lerr := lockMetadata(path)
+		switch {
+		case lerr == nil:
 			defer unlock()
+		case errors.Is(lerr, ErrLockTimeout):
+			res.LockTimedOut = true
 		}
 	}
 
@@ -1811,15 +1823,20 @@ func ReadMetadata(path string) (map[string]SessionMetadata, error) {
 	// file is readable, so an unbounded read stalls startup with nothing on screen to say why.
 	// Far past any real metadata file: the measured 192-session file is 74 KB.
 	//
-	// Truncation surfaces as a JSON error rather than as silent data loss, which is the right
-	// outcome here: this reader's caller refuses to merge over a file it cannot parse, so a
-	// file too large to read is treated like any other unreadable one instead of quietly
-	// becoming a smaller map.
+	// Truncation is reported as ITS OWN failure, not left to surface as a JSON error. A valid
+	// file over the cap decodes as a parse failure, and Harvest rebuilds over parse failures —
+	// which would replace a good 17 MB file with a 400-byte one. So the cap is checked before
+	// the decode, and the error deliberately carries neither sentinel: not errMetadataNotJSON,
+	// so it cannot be rebuilt over; not a bare read error, because nothing is wrong with the
+	// bytes. Read one over the cap to tell "exactly at the cap" from "larger".
 	const maxMetadataBytes = 16 << 20
-	b, err := io.ReadAll(io.LimitReader(f, maxMetadataBytes))
+	b, err := io.ReadAll(io.LimitReader(f, maxMetadataBytes+1))
 	if err != nil {
 		return nil, err
 	}
+	if len(b) > maxMetadataBytes {
+		return nil, fmt.Errorf("%s is larger than the %d byte read limit", path, maxMetadataBytes)
+	}
 	var m map[string]SessionMetadata
 	if uerr := json.Unmarshal(b, &m); uerr != nil {
 		// Sentinel-wrapped so Harvest can tell a parse failure from a file it could not
diff --git a/authbridge/authlib/observe/claude/harvest_test.go b/authbridge/authlib/observe/claude/harvest_test.go
index 4eb446dc3..d802c1511 100644
--- a/authbridge/authlib/observe/claude/harvest_test.go
+++ b/authbridge/authlib/observe/claude/harvest_test.go
@@ -304,6 +304,61 @@ func TestHarvest_UnreadableMetadataIsRefusedNotRebuilt(t *testing.T) {
 
 // A rebuild is still a merge otherwise: it replaces the unparseable file, and the next
 // harvest keeps what it wrote.
+// An oversized but VALID file is refused, not rebuilt over. The read cap makes such a file
+// fail json.Unmarshal, which would classify it as a parse failure and destroy it: measured at
+// 18.5MB/17000 entries replaced by 412 bytes/1 entry.
+func TestHarvest_OversizedValidMetadataIsRefusedNotRebuilt(t *testing.T) {
+	metadataHome(t)
+	path, err := SessionMetadataPath()
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil {
+		t.Fatal(err)
+	}
+	// Built as text rather than by marshalling a map: cheaper, and it keeps the fixture's size
+	// the point of the test rather than a side effect of the struct.
+	var b []byte
+	b = append(b, '{')
+	pad := strings.Repeat("x", 4<<10)
+	for i := 0; len(b) <= 16<<20; i++ {
+		if i > 0 {
+			b = append(b, ',')
+		}
+		b = append(b, fmt.Sprintf("%q:{\"title\":%q}", fmt.Sprintf("sess-%06d", i), pad)...)
+	}
+	b = append(b, '}')
+	if !json.Valid(b) {
+		t.Fatal("fixture is not valid JSON, so this would not test the rebuild path")
+	}
+	if err := os.WriteFile(path, b, 0o600); err != nil {
+		t.Fatal(err)
+	}
+
+	cfg := filepath.Join(t.TempDir(), "claude")
+	writeSessionTranscript(t, filepath.Join(cfg, "projects", "-p"), "fresh.jsonl",
+		`{"type":"ai-title","aiTitle":"new"}`)
+
+	res, err := Harvest(Options{ConfigDir: cfg, Merge: true})
+	if err == nil {
+		t.Fatalf("Harvest succeeded (Rebuilt=%v); want it refused rather than rebuilding", res.Rebuilt)
+	}
+	if !errors.Is(err, ErrCorruptMetadata) {
+		t.Errorf("err = %v, want it to wrap ErrCorruptMetadata", err)
+	}
+	if res.Rebuilt {
+		t.Error("Rebuilt is true for a file that was merely too large to read")
+	}
+	// The assertion that matters: the bytes are still there.
+	st, serr := os.Stat(path)
+	if serr != nil {
+		t.Fatal(serr)
+	}
+	if got := st.Size(); got != int64(len(b)) {
+		t.Errorf("file is %d bytes, was %d — it was rewritten", got, len(b))
+	}
+}
+
 func TestHarvest_RebuildThenMergeKeepsEntries(t *testing.T) {
 	metadataHome(t)
 	cfg := filepath.Join(t.TempDir(), "claude")
diff --git a/authbridge/authlib/observe/claude/lock_unix_test.go b/authbridge/authlib/observe/claude/lock_unix_test.go
index bed51655c..0c9f0c164 100644
--- a/authbridge/authlib/observe/claude/lock_unix_test.go
+++ b/authbridge/authlib/observe/claude/lock_unix_test.go
@@ -91,6 +91,23 @@ func TestLockMetadata_TimesOutAndReportsIt(t *testing.T) {
 	}
 }
 
+// An uncontended harvest does not claim a timeout. Guards the field against being set
+// unconditionally, which would make it useless as a signal.
+func TestHarvest_UncontendedDoesNotReportALockTimeout(t *testing.T) {
+	metadataHome(t)
+	cfg := filepath.Join(t.TempDir(), "claude")
+	writeSessionTranscript(t, filepath.Join(cfg, "projects", "-p"), "s1.jsonl",
+		`{"type":"ai-title","aiTitle":"t"}`)
+
+	res, err := Harvest(Options{ConfigDir: cfg, Merge: true})
+	if err != nil {
+		t.Fatalf("Harvest: %v", err)
+	}
+	if res.LockTimedOut {
+		t.Error("LockTimedOut is true with nothing holding the lock")
+	}
+}
+
 // An uncontended lock is still taken, and released, at once.
 func TestLockMetadata_UncontendedIsFast(t *testing.T) {
 	path := filepath.Join(t.TempDir(), "session-metadata.json")
@@ -141,4 +158,9 @@ func TestHarvest_ProceedsWhenTheLockIsHeld(t *testing.T) {
 	if got := readMetadataFile(t, path)["s1"].Title; got != "t" {
 		t.Errorf("on disk Title = %q, want the harvest to have written", got)
 	}
+	// Reported, not silent: an unlocked harvest can lose its entries to a concurrent rename, and
+	// without this the only failure the lock exists to prevent has no diagnosis.
+	if !res.LockTimedOut {
+		t.Error("LockTimedOut is false after the harvest ran unlocked")
+	}
 }
diff --git a/authbridge/cmd/abctl/cmd_experimental.go b/authbridge/cmd/abctl/cmd_experimental.go
index cf4f58b1b..0489436c1 100644
--- a/authbridge/cmd/abctl/cmd_experimental.go
+++ b/authbridge/cmd/abctl/cmd_experimental.go
@@ -177,6 +177,12 @@ func runReadClaudeSessions(args []string, stdout, stderr io.Writer) int {
 	// Said out loud because the counts cannot show it: a rebuild reports everything harvested
 	// and nothing kept, which is exactly what a first run reports. The entries it dropped —
 	// sessions whose transcripts are gone — leave no trace for the operator to notice.
+	// Said out loud because the entries it may have lost are gone without a trace: an unlocked
+	// harvest can have its whole contribution erased by a concurrent run's rename.
+	if res.LockTimedOut {
+		fmt.Fprintf(stderr, "abctl: timed out waiting for the lock on %s; harvested anyway, so a concurrent run may have overwritten this one\n", res.Path)
+	}
+
 	if res.Rebuilt {
 		fmt.Fprintf(stderr, "abctl: the existing file could not be parsed; rebuilt it from %s\n", res.ConfigDir)
 	}

From 050d076c9312ee62a69de7abec41a9df0b1d6985 Mon Sep 17 00:00:00 2001
From: Ed Snible <snible@us.ibm.com>
Date: Thu, 24 Sep 2026 11:42:41 -0400
Subject: [PATCH 4/6] fix: Give the oversized-metadata refusal its own sentinel
 and remedy

The 16 MiB read cap added in the previous commit created a third failure class
neither caller knew about: a file that is readable, valid, and simply too large.
Both callers got it wrong.

`read-claude-sessions` printed "Fix the file's permissions" for a file whose
permissions are fine. `abctl observe`'s pre-flight fell through silently,
because the error is not fs.ErrPermission -- so Harvest refused on every
launch with nothing said before the alt screen. That is the
never-clears-itself bug class this PR exists to fix, recreated.

So ErrMetadataTooLarge is exported and the cap error wraps it. Unlike
errMetadataNotJSON, whose outcome is already reported through Result.Rebuilt,
this condition is actionable and permanent, so callers genuinely need to
discriminate it. Each now branches on it first -- it also wraps
ErrCorruptMetadata, so the narrower sentinel has to be checked first -- and
names the remedy that fits: move the file aside, not chmod it.

Also in this commit:

- README: two sentences became false when parse failures started self-healing.
  A corrupt file no longer costs the TITLE column, and a parse failure
  deliberately prints nothing rather than "one line with the repair".

- SaveMetadata's lost tripwire is restored (gap 4 of the reported-not-fixed
  list). The writeAll seam is two lines and buys back the only way to reach
  the truncated-rename path: a write error that Close and Rename both survive
  is not something a real filesystem produces on demand. Mutation-checked by
  reintroducing the `err :=` shadow it guards.

Gap 3 (res.Partial dropped by the background harvester) is left as documented:
Partial carries formatted "path: err" strings rather than session ids, so
marking the affected rows needs a wider type and a per-entry flag threaded
into SessionMetadata and the TITLE cell. Gap 5 is closed by
TestLockMetadata_UncontendedIsFast.

New tests, each mutation-checked:
- the CLI names the right repair for an oversized file, and leaves its bytes
- the viewer's pre-flight disables titles and warns with its own remedy
- ErrMetadataTooLarge is pinned through both wraps
- a failed write keeps the old file and leaves no temp behind

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Ed Snible <snible@us.ibm.com>
---
 authbridge/authlib/observe/claude/harvest.go  | 50 +++++++++-----
 .../authlib/observe/claude/harvest_test.go    | 45 +++++++++++++
 authbridge/cmd/abctl/README.md                | 10 +--
 authbridge/cmd/abctl/cmd_experimental.go      | 14 +++-
 authbridge/cmd/abctl/cmd_experimental_test.go | 67 +++++++++++++++++++
 authbridge/cmd/abctl/cmd_observe_test.go      | 34 ++++++++++
 authbridge/cmd/abctl/main.go                  | 13 +++-
 7 files changed, 207 insertions(+), 26 deletions(-)

diff --git a/authbridge/authlib/observe/claude/harvest.go b/authbridge/authlib/observe/claude/harvest.go
index 389e30a0a..ceb3a42e3 100644
--- a/authbridge/authlib/observe/claude/harvest.go
+++ b/authbridge/authlib/observe/claude/harvest.go
@@ -23,9 +23,11 @@ const ConfigDirEnv = "CLAUDE_CONFIG_DIR"
 // read, so a merge refused rather than replacing a file whose contents are unknown.
 //
 // NARROWER THAN THE NAME SUGGESTS: a file that reads fine but does not parse no longer
-// comes back here, because Harvest rebuilds it instead. What is left is the file that
-// could not be read at all — a permission or I/O failure — where refusing is the only
-// safe answer, since a rebuild would replace entries that may be perfectly good.
+// comes back here, because Harvest rebuilds it instead. What is left is every file Harvest
+// refuses because a rebuild might replace good entries — one that could not be read (a
+// permission or I/O failure) and one too large to read whole (ErrMetadataTooLarge). The two
+// need different advice, so a caller printing a remedy must check the narrower sentinel
+// first rather than assuming this one means permissions.
 //
 // Exported because the repair is a CLI affordance: `abctl experimental
 // read-claude-sessions` names --merge=false as the way past, and only the command layer
@@ -44,6 +46,15 @@ var ErrCorruptMetadata = errors.New("corrupt session metadata")
 // the Windows cross-check lock_other.go exists to keep working.
 var ErrLockTimeout = errors.New("timed out waiting for the session metadata lock")
 
+// ErrMetadataTooLarge reports that the metadata file is larger than ReadMetadata's cap, so it
+// could not be read whole and was refused rather than rebuilt over.
+//
+// Exported, unlike errMetadataNotJSON, because the remedy differs and callers must not give the
+// wrong one: this file is intact and its permissions are fine, so "fix the permissions" is wrong
+// advice. It is also permanent — every launch refuses again — which a caller may want to say
+// before a full-screen viewer hides it.
+var ErrMetadataTooLarge = errors.New("session metadata file is too large to read")
+
 // errMetadataNotJSON marks the one read failure Harvest can recover from by rebuilding.
 //
 // A SENTINEL RATHER THAN A STRING MATCH on ReadMetadata's message, and rather than
@@ -1826,16 +1837,18 @@ func ReadMetadata(path string) (map[string]SessionMetadata, error) {
 	// Truncation is reported as ITS OWN failure, not left to surface as a JSON error. A valid
 	// file over the cap decodes as a parse failure, and Harvest rebuilds over parse failures —
 	// which would replace a good 17 MB file with a 400-byte one. So the cap is checked before
-	// the decode, and the error deliberately carries neither sentinel: not errMetadataNotJSON,
-	// so it cannot be rebuilt over; not a bare read error, because nothing is wrong with the
-	// bytes. Read one over the cap to tell "exactly at the cap" from "larger".
+	// the decode, and the error deliberately carries its own sentinel: not errMetadataNotJSON,
+	// so it cannot be rebuilt over; not a bare read error either, because nothing is wrong with the
+	// bytes — it carries ErrMetadataTooLarge, which the CLI and the viewer's pre-flight each
+	// branch on to give the right remedy. Read one over the cap to tell "exactly at the cap"
+	// from "larger".
 	const maxMetadataBytes = 16 << 20
 	b, err := io.ReadAll(io.LimitReader(f, maxMetadataBytes+1))
 	if err != nil {
 		return nil, err
 	}
 	if len(b) > maxMetadataBytes {
-		return nil, fmt.Errorf("%s is larger than the %d byte read limit", path, maxMetadataBytes)
+		return nil, fmt.Errorf("%w: %s is larger than the %d byte read limit", ErrMetadataTooLarge, path, maxMetadataBytes)
 	}
 	var m map[string]SessionMetadata
 	if uerr := json.Unmarshal(b, &m); uerr != nil {
@@ -1915,6 +1928,14 @@ func recoverConcurrentEntries(path string, meta map[string]SessionMetadata) (add
 	return added, replaced, nil
 }
 
+// writeAll writes body to w. A var, not a call, purely as a test seam.
+//
+// The bug it guards is the `err :=` shadow in SaveMetadata: a scoped error would be dropped,
+// and Close and Rename would then rename a TRUNCATED file over the good one. Nothing a test
+// can provoke on a working filesystem — a short write without an error is not something a
+// real file does — so the injection point is the only way to reach that path.
+var writeAll = func(w io.Writer, body []byte) (int, error) { return w.Write(body) }
+
 // SaveMetadata writes the map atomically, creating ~/.cortex if needed.
 //
 // Same mechanics as saveUserConfig, for the same reasons: CreateTemp rather than a
@@ -1960,16 +1981,11 @@ func SaveMetadata(path string, meta map[string]SessionMetadata) error {
 	// would then see success, renaming a truncated file over the good one. The bug
 	// saveUserConfig's comment records having made.
 	//
-	// TRIPWIRE LOST IN THE MOVE, recorded rather than left silent: in package main this was
-	// `writeAll(f, body)`, an indirected io.Writer.Write that a test could swap for a failing
-	// one — added because the shadowing bug above is invisible to every test that writes to a
-	// working filesystem. That var stays in cmd/abctl for saveUserConfig, which is what its
-	// own test swaps, and it cannot travel here without duplicating it across two modules. No
-	// test ever reached it through the harvest path (the write-failure test uses an
-	// unwritable directory instead), so nothing regressed today — but the injection point is
-	// gone, so a future edit that reintroduces the shadowing has one fewer way to be caught.
-	// Restoring it is three lines if that ever feels too thin.
-	_, err = f.Write(body)
+	// Through writeAll, not f.Write directly, so a test can make the write fail while Close
+	// and Rename still succeed — the only shape that catches the shadowing above, and one no
+	// real filesystem produces on demand. cmd/abctl has its own copy of this seam for
+	// saveUserConfig; the duplication is two lines and buys each module its own tripwire.
+	_, err = writeAll(f, body)
 	if cerr := f.Close(); err == nil {
 		err = cerr
 	}
diff --git a/authbridge/authlib/observe/claude/harvest_test.go b/authbridge/authlib/observe/claude/harvest_test.go
index d802c1511..613e8b8a2 100644
--- a/authbridge/authlib/observe/claude/harvest_test.go
+++ b/authbridge/authlib/observe/claude/harvest_test.go
@@ -5,6 +5,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"io"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -346,6 +347,11 @@ func TestHarvest_OversizedValidMetadataIsRefusedNotRebuilt(t *testing.T) {
 	if !errors.Is(err, ErrCorruptMetadata) {
 		t.Errorf("err = %v, want it to wrap ErrCorruptMetadata", err)
 	}
+	// Distinguishable through both wraps, so a caller can give the right remedy: this file's
+	// permissions are fine and "fix the permissions" would be wrong advice.
+	if !errors.Is(err, ErrMetadataTooLarge) {
+		t.Errorf("err = %v, want it to wrap ErrMetadataTooLarge", err)
+	}
 	if res.Rebuilt {
 		t.Error("Rebuilt is true for a file that was merely too large to read")
 	}
@@ -2847,3 +2853,42 @@ func TestTitleFromTranscript_CwdIsBounded(t *testing.T) {
 			got[max(0, len(got)-20):], "/the-leaf")
 	}
 }
+
+// A short write must not be renamed over the good file. This is the seam writeAll exists for:
+// the failure is a write error that Close and Rename both survive, which no real filesystem
+// produces, and the bug it guards — `err :=` shadowing inside SaveMetadata — is invisible to
+// every test that writes to a working disk.
+func TestSaveMetadata_AFailedWriteKeepsTheOldFile(t *testing.T) {
+	dir := t.TempDir()
+	path := filepath.Join(dir, "session-metadata.json")
+	const keep = `{"old":{"title":"keep me"}}`
+	if err := os.WriteFile(path, []byte(keep), 0o600); err != nil {
+		t.Fatal(err)
+	}
+
+	was := writeAll
+	writeAll = func(io.Writer, []byte) (int, error) { return 0, errors.New("disk on fire") }
+	t.Cleanup(func() { writeAll = was })
+
+	if err := SaveMetadata(path, map[string]SessionMetadata{"new": {Title: "t"}}); err == nil {
+		t.Fatal("SaveMetadata returned nil after the write failed")
+	}
+	// The point of the test: the old file, not a truncated new one.
+	b, err := os.ReadFile(path)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if string(b) != keep {
+		t.Errorf("file = %q, want the original %q", b, keep)
+	}
+	// And no temp file left behind to accumulate one per failure.
+	ents, err := os.ReadDir(dir)
+	if err != nil {
+		t.Fatal(err)
+	}
+	for _, e := range ents {
+		if strings.HasSuffix(e.Name(), ".tmp") {
+			t.Errorf("temp file %s was left behind", e.Name())
+		}
+	}
+}
diff --git a/authbridge/cmd/abctl/README.md b/authbridge/cmd/abctl/README.md
index 892acb96f..26d00d4f9 100644
--- a/authbridge/cmd/abctl/README.md
+++ b/authbridge/cmd/abctl/README.md
@@ -153,10 +153,12 @@ simply not be touched. It suppresses only the *scan*: the viewer still reads
 only sessions new or renamed since the last scan show as bare ids. There is no flag that
 hides titles already on disk — delete the file for that.
 
-A harvest that cannot run is never fatal — a missing, unreadable or corrupt file
-costs the `TITLE` column and nothing else, and the viewer still opens. The failures
-that can be known before the viewer starts, such as an unreadable metadata file, print
-one line to stderr with the repair; success says nothing.
+A harvest that cannot run is never fatal — the worst a missing or unreadable file
+costs is the `TITLE` column, and the viewer still opens. A file that does not parse is
+rebuilt from the transcripts rather than costing anything; the entries a rebuild cannot
+recover are sessions whose transcripts Claude Code has already pruned. The failures that
+need a human, such as an unreadable metadata file, print one line to stderr with the
+repair before the viewer starts; success says nothing.
 
 The config directory is `CLAUDE_CONFIG_DIR` when set, and `~/.claude`
 otherwise. To read a different directory, or to force a full re-read of every
diff --git a/authbridge/cmd/abctl/cmd_experimental.go b/authbridge/cmd/abctl/cmd_experimental.go
index 0489436c1..d58f78684 100644
--- a/authbridge/cmd/abctl/cmd_experimental.go
+++ b/authbridge/cmd/abctl/cmd_experimental.go
@@ -144,10 +144,18 @@ func runReadClaudeSessions(args []string, stdout, stderr io.Writer) int {
 	// `abctl observe` is the incremental caller.
 	res, err := claude.Harvest(claude.Options{ConfigDir: *dir, Merge: *merge})
 	if err != nil {
+		// Two refusals, two remedies. Checked narrowest first: the oversized file also wraps
+		// ErrCorruptMetadata, and its permissions are fine, so the permission advice is wrong
+		// for it. Neither suggests --merge=false any more — both would hit the same read.
+		if errors.Is(err, claude.ErrMetadataTooLarge) {
+			// The path is already in the error text, so it is not repeated here.
+			fmt.Fprintf(stderr, "abctl: %v\n"+
+				"  Move the file aside and re-run to start a fresh one.\n", err)
+			return 1
+		}
 		if errors.Is(err, claude.ErrCorruptMetadata) {
-			// NOW ONLY THE UNREADABLE FILE reaches here: one that does not parse is rebuilt by
-			// Harvest itself. This one could not be read at all, so --merge=false is no longer
-			// the thing to suggest — it would hit the same read. Name what a human can do.
+			// A file that does not parse is rebuilt by Harvest itself, so what reaches here
+			// could not be read at all. Name what a human can do.
 			fmt.Fprintf(stderr, "abctl: %v\n"+
 				"  Fix the file's permissions, or move it aside and re-run.\n", err)
 			return 1
diff --git a/authbridge/cmd/abctl/cmd_experimental_test.go b/authbridge/cmd/abctl/cmd_experimental_test.go
index 815374a33..db9cab478 100644
--- a/authbridge/cmd/abctl/cmd_experimental_test.go
+++ b/authbridge/cmd/abctl/cmd_experimental_test.go
@@ -3,6 +3,7 @@ package main
 import (
 	"bytes"
 	"encoding/json"
+	"fmt"
 	"os"
 	"path/filepath"
 	"runtime"
@@ -31,6 +32,40 @@ func writeSessionTranscript(t *testing.T, dir, name string, lines ...string) {
 }
 
 // readMetadataFile decodes what the command wrote.
+// oversizedMetadata builds a VALID metadata file just over Harvest's 16 MiB read cap.
+//
+// Raw text rather than marshalling a map: 4000-odd entries through encoding/json took seconds,
+// and what the test needs is the size and the validity, not realistic content.
+func oversizedMetadata(t *testing.T) []byte {
+	t.Helper()
+	var b []byte
+	b = append(b, '{')
+	pad := strings.Repeat("x", 4<<10)
+	for i := 0; len(b) <= 16<<20; i++ {
+		if i > 0 {
+			b = append(b, ',')
+		}
+		b = append(b, fmt.Sprintf("%q:{\"title\":%q}", fmt.Sprintf("sess-%06d", i), pad)...)
+	}
+	b = append(b, '}')
+	// Asserted, because an INVALID oversized file would exercise the rebuild path instead and
+	// the test would pass for the wrong reason.
+	if !json.Valid(b) {
+		t.Fatal("fixture is not valid JSON, so this would not test the refusal")
+	}
+	return b
+}
+
+// fileSize reports the file's size, for before/after comparison.
+func fileSize(t *testing.T, path string) int64 {
+	t.Helper()
+	st, err := os.Stat(path)
+	if err != nil {
+		t.Fatalf("stat %s: %v", path, err)
+	}
+	return st.Size()
+}
+
 func readMetadataFile(t *testing.T, path string) map[string]tui.SessionMetadata {
 	t.Helper()
 	b, err := os.ReadFile(path) //nolint:gosec // test-controlled path
@@ -445,6 +480,38 @@ func TestReadClaudeSessions_MergeRefusesAnUnreadableFile(t *testing.T) {
 	}
 }
 
+// An oversized but valid file is refused with its OWN remedy — not the permission advice, which
+// would be wrong: the file is intact and readable, just over the read cap.
+func TestReadClaudeSessions_OversizedFileNamesTheRightRepair(t *testing.T) {
+	home := prefsHome(t)
+	path := filepath.Join(home, tui.SessionMetadataRel)
+	if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(path, oversizedMetadata(t), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	size := fileSize(t, path)
+
+	cfg := filepath.Join(t.TempDir(), "claude")
+	writeSessionTranscript(t, filepath.Join(cfg, "projects", "-p"), "fresh.jsonl",
+		`{"type":"user","cwd":"/w"}`)
+
+	var out, errb bytes.Buffer
+	if code := runExperimental([]string{"read-claude-sessions", "--dir", cfg}, &out, &errb); code != 1 {
+		t.Errorf("exit = %d, want 1", code)
+	}
+	if got := errb.String(); !strings.Contains(got, "too large") {
+		t.Errorf("stderr does not say what is wrong:\n%s", got)
+	}
+	if got := errb.String(); strings.Contains(got, "permissions") {
+		t.Errorf("stderr gives the permission remedy for an intact file:\n%s", got)
+	}
+	if got := fileSize(t, path); got != size {
+		t.Errorf("file is %d bytes, was %d — it was rewritten", got, size)
+	}
+}
+
 // CLAUDE_CONFIG_DIR is honoured when --dir is absent, and --dir wins over it.
 func TestReadClaudeSessions_ConfigDirResolution(t *testing.T) {
 	t.Run("env var is used", func(t *testing.T) {
diff --git a/authbridge/cmd/abctl/cmd_observe_test.go b/authbridge/cmd/abctl/cmd_observe_test.go
index e0539a5fc..ac9faa796 100644
--- a/authbridge/cmd/abctl/cmd_observe_test.go
+++ b/authbridge/cmd/abctl/cmd_observe_test.go
@@ -397,6 +397,40 @@ func TestClaudeHarvester_UnreadableFileNamesTheRepair(t *testing.T) {
 	}
 }
 
+// An oversized-but-valid file also disables the harvest — Harvest refuses it, so running the
+// harvester anyway would fail on every launch with nothing said before the alt screen. The
+// remedy printed must be its own, not the permission advice: this file is readable and intact.
+func TestClaudeHarvester_OversizedFileNamesItsOwnRepair(t *testing.T) {
+	home := prefsHome(t)
+	cfg := filepath.Join(t.TempDir(), "claude")
+	writeSessionTranscript(t, filepath.Join(cfg, "projects", "-p"), "s1.jsonl",
+		`{"type":"ai-title","aiTitle":"t"}`)
+	t.Setenv("CLAUDE_CONFIG_DIR", cfg)
+
+	path := filepath.Join(home, tui.SessionMetadataRel)
+	if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(path, oversizedMetadata(t), 0o600); err != nil {
+		t.Fatal(err)
+	}
+
+	var warn bytes.Buffer
+	if h := claudeHarvester(&warn); h != nil {
+		t.Error("a file Harvest refuses must disable the harvest rather than fail every launch")
+	}
+	got := warn.String()
+	if !strings.Contains(got, "too large") {
+		t.Errorf("the warning does not say what is wrong:\n%s", got)
+	}
+	if strings.Contains(got, "permission") {
+		t.Errorf("the warning gives the permission remedy for an intact file:\n%s", got)
+	}
+	if !strings.Contains(got, "mv ") {
+		t.Errorf("the warning does not name the repair:\n%s", got)
+	}
+}
+
 // `abctl observe` wires the harvest to the flag: on by default, off with
 // --skip-claude-metadata.
 //
diff --git a/authbridge/cmd/abctl/main.go b/authbridge/cmd/abctl/main.go
index 8407d2584..1d52343f0 100644
--- a/authbridge/cmd/abctl/main.go
+++ b/authbridge/cmd/abctl/main.go
@@ -256,14 +256,23 @@ func claudeHarvester(warn io.Writer) tui.HarvestFunc {
 				"    mv %s %s.bad\n", err, path, path)
 			return nil
 		}
+		// Too large is the same shape of problem: Harvest refuses it, so it repeats every launch
+		// and needs a human. Different remedy — the file is intact and readable, just over the
+		// cap — so it gets its own line rather than the permission advice.
+		if errors.Is(err, claude.ErrMetadataTooLarge) {
+			fmt.Fprintf(warn, "abctl: not naming sessions from Claude Code: %v\n"+
+				"  Move it aside to start a fresh file:\n"+
+				"    mv %s %s.bak\n", err, path, path)
+			return nil
+		}
 		// EVERY OTHER READ ERROR FALLS THROUGH SILENTLY rather than disabling titles or warning
 		// here, because this position cannot tell which of them Harvest will recover from. The
 		// sentinel that says "this one does not parse" is unexported, and exporting it would
 		// widen authlib's API to let this pre-flight re-derive a decision Harvest makes a few
 		// lines later anyway.
 		//
-		// So the split is by what a human can DO, not by what went wrong: a permission failure
-		// names an action and repeats every launch, which is worth printing before the alt screen
+		// So the split is by what a human can DO, not by what went wrong: the two cases above
+		// name an action and repeat every launch, which is worth printing before the alt screen
 		// goes up. Not warning about the rest is the deliberate half — the common case among them
 		// is the file that does not parse, which heals itself moments later, so a line here would
 		// tell the operator titles are in trouble and then hand them titles. A harvest that does

From 7e0edc58b721abb77637b56df63b2969faa53301 Mon Sep 17 00:00:00 2001
From: Ed Snible <snible@us.ibm.com>
Date: Thu, 24 Sep 2026 13:06:12 -0400
Subject: [PATCH 5/6] fix: Restore the --merge=false remedy and correct five
 stale comments
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Review follow-ups, all five items.

1. cmd_experimental.go: --merge=false was removed from both refusal
   messages on the false premise that it "would hit the same read". It
   does not: Harvest reads the file only under Merge, and SaveMetadata
   renames over the path, so the old file never has to be readable.
   Verified against an unreadable file and a 17.7 MB one; both repair.
   The remedy is back in both messages, and now has a test that asserts
   it is present AND one that proves it works.

2. main.go: the fall-through claimed a failing harvest "reports itself
   through the viewer's own error path". No such path exists — harvestCmd
   does `meta, _ := h()` and drops it deliberately. Replaced with what
   actually happens, and why closing it is bigger than this change.

3. cmd_experimental.go: the Rebuilt doc was attached to the LockTimedOut
   branch. Moved onto `if res.Rebuilt`.

4. harvest_test.go: TestHarvest_RebuildThenMergeKeepsEntries's header had
   slid onto the oversized test. Reattached.

5. harvest.go: ReadMetadata's doc still stated the refuse-under-merge
   policy this PR inverts. Rewritten to describe the three outcomes a
   caller can now discriminate.

Also fixes an orphaned helper comment in cmd_experimental_test.go, the
same defect as (4) and introduced by an earlier commit in this PR.

Mutations: removing the restored remedy fails the new assertion (M18);
making --merge=false read the file fails the repair test (M19).

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Ed Snible <snible@us.ibm.com>
---
 authbridge/authlib/observe/claude/harvest.go  | 14 +++--
 .../authlib/observe/claude/harvest_test.go    |  4 +-
 authbridge/cmd/abctl/cmd_experimental.go      | 20 ++++---
 authbridge/cmd/abctl/cmd_experimental_test.go | 53 ++++++++++++++++++-
 authbridge/cmd/abctl/main.go                  | 14 ++++-
 5 files changed, 90 insertions(+), 15 deletions(-)

diff --git a/authbridge/authlib/observe/claude/harvest.go b/authbridge/authlib/observe/claude/harvest.go
index ceb3a42e3..230ed12dc 100644
--- a/authbridge/authlib/observe/claude/harvest.go
+++ b/authbridge/authlib/observe/claude/harvest.go
@@ -1812,10 +1812,16 @@ type contentBlock struct {
 // ReadMetadata reads the existing file, distinguishing absent from unreadable.
 //
 // An empty map with a nil error means genuinely no file yet — the first run, which is
-// not a problem. A non-nil error means a file was there and could not be trusted, and
-// the caller must say so out loud rather than proceeding: under merge, treating a
-// corrupt file as empty would discard exactly the entries the flag exists to keep.
-// Same distinction, for the same reason, as readState in cmd_claudecode.go.
+// not a problem. A non-nil error means a file was there and could not be trusted; what
+// the caller should DO about it depends on which error, and this function's job is only
+// to keep them apart. Same distinction, for the same reason, as readState in
+// cmd_claudecode.go.
+//
+// Three outcomes a caller can discriminate, because Harvest treats them differently:
+// errMetadataNotJSON for a file whose bytes are not JSON, which Harvest rebuilds over;
+// ErrMetadataTooLarge for a valid file past the read cap, which it refuses because
+// rebuilding would destroy intact entries; and a bare os/io error — permission, EIO —
+// which it also refuses, since nothing there says the contents are bad.
 //
 // A file holding JSON `null` decodes to a nil map, which is indistinguishable from an
 // empty object for merging purposes, so it is normalised rather than refused.
diff --git a/authbridge/authlib/observe/claude/harvest_test.go b/authbridge/authlib/observe/claude/harvest_test.go
index 613e8b8a2..3d7d747d1 100644
--- a/authbridge/authlib/observe/claude/harvest_test.go
+++ b/authbridge/authlib/observe/claude/harvest_test.go
@@ -303,8 +303,6 @@ func TestHarvest_UnreadableMetadataIsRefusedNotRebuilt(t *testing.T) {
 	}
 }
 
-// A rebuild is still a merge otherwise: it replaces the unparseable file, and the next
-// harvest keeps what it wrote.
 // An oversized but VALID file is refused, not rebuilt over. The read cap makes such a file
 // fail json.Unmarshal, which would classify it as a parse failure and destroy it: measured at
 // 18.5MB/17000 entries replaced by 412 bytes/1 entry.
@@ -365,6 +363,8 @@ func TestHarvest_OversizedValidMetadataIsRefusedNotRebuilt(t *testing.T) {
 	}
 }
 
+// A rebuild is still a merge otherwise: it replaces the unparseable file, and the next
+// harvest keeps what it wrote.
 func TestHarvest_RebuildThenMergeKeepsEntries(t *testing.T) {
 	metadataHome(t)
 	cfg := filepath.Join(t.TempDir(), "claude")
diff --git a/authbridge/cmd/abctl/cmd_experimental.go b/authbridge/cmd/abctl/cmd_experimental.go
index d58f78684..db05539d4 100644
--- a/authbridge/cmd/abctl/cmd_experimental.go
+++ b/authbridge/cmd/abctl/cmd_experimental.go
@@ -146,18 +146,26 @@ func runReadClaudeSessions(args []string, stdout, stderr io.Writer) int {
 	if err != nil {
 		// Two refusals, two remedies. Checked narrowest first: the oversized file also wraps
 		// ErrCorruptMetadata, and its permissions are fine, so the permission advice is wrong
-		// for it. Neither suggests --merge=false any more — both would hit the same read.
+		// for it.
+		//
+		// Both DO offer --merge=false, because it genuinely repairs both: Harvest reads the
+		// file only under Merge, and the save renames over the path, needing the directory
+		// writable rather than the old file readable. Verified on an unreadable file and on a
+		// 17.7 MB one. It is also the gentler of the two remedies — it leaves no .bad file to
+		// clean up — so it goes first, with mv kept for whoever wants the old bytes preserved.
 		if errors.Is(err, claude.ErrMetadataTooLarge) {
 			// The path is already in the error text, so it is not repeated here.
 			fmt.Fprintf(stderr, "abctl: %v\n"+
-				"  Move the file aside and re-run to start a fresh one.\n", err)
+				"  Re-run with --merge=false to rebuild it, or move the file aside first\n"+
+				"  to keep the old entries.\n", err)
 			return 1
 		}
 		if errors.Is(err, claude.ErrCorruptMetadata) {
 			// A file that does not parse is rebuilt by Harvest itself, so what reaches here
 			// could not be read at all. Name what a human can do.
 			fmt.Fprintf(stderr, "abctl: %v\n"+
-				"  Fix the file's permissions, or move it aside and re-run.\n", err)
+				"  Re-run with --merge=false to rebuild it, or fix the file's permissions\n"+
+				"  to keep the old entries.\n", err)
 			return 1
 		}
 		fmt.Fprintf(stderr, "abctl: %v\n", err)
@@ -182,15 +190,15 @@ func runReadClaudeSessions(args []string, stdout, stderr io.Writer) int {
 		fmt.Fprintf(stderr, "abctl: merged %d entry(s) written concurrently by another run\n", res.Recovered)
 	}
 
-	// Said out loud because the counts cannot show it: a rebuild reports everything harvested
-	// and nothing kept, which is exactly what a first run reports. The entries it dropped —
-	// sessions whose transcripts are gone — leave no trace for the operator to notice.
 	// Said out loud because the entries it may have lost are gone without a trace: an unlocked
 	// harvest can have its whole contribution erased by a concurrent run's rename.
 	if res.LockTimedOut {
 		fmt.Fprintf(stderr, "abctl: timed out waiting for the lock on %s; harvested anyway, so a concurrent run may have overwritten this one\n", res.Path)
 	}
 
+	// Said out loud because the counts cannot show it: a rebuild reports everything harvested
+	// and nothing kept, which is exactly what a first run reports. The entries it dropped —
+	// sessions whose transcripts are gone — leave no trace for the operator to notice.
 	if res.Rebuilt {
 		fmt.Fprintf(stderr, "abctl: the existing file could not be parsed; rebuilt it from %s\n", res.ConfigDir)
 	}
diff --git a/authbridge/cmd/abctl/cmd_experimental_test.go b/authbridge/cmd/abctl/cmd_experimental_test.go
index db9cab478..e0a7671f3 100644
--- a/authbridge/cmd/abctl/cmd_experimental_test.go
+++ b/authbridge/cmd/abctl/cmd_experimental_test.go
@@ -31,7 +31,6 @@ func writeSessionTranscript(t *testing.T, dir, name string, lines ...string) {
 	}
 }
 
-// readMetadataFile decodes what the command wrote.
 // oversizedMetadata builds a VALID metadata file just over Harvest's 16 MiB read cap.
 //
 // Raw text rather than marshalling a map: 4000-odd entries through encoding/json took seconds,
@@ -66,6 +65,7 @@ func fileSize(t *testing.T, path string) int64 {
 	return st.Size()
 }
 
+// readMetadataFile decodes what the command wrote.
 func readMetadataFile(t *testing.T, path string) map[string]tui.SessionMetadata {
 	t.Helper()
 	b, err := os.ReadFile(path) //nolint:gosec // test-controlled path
@@ -472,6 +472,11 @@ func TestReadClaudeSessions_MergeRefusesAnUnreadableFile(t *testing.T) {
 	if code := runExperimental([]string{"read-claude-sessions", "--dir", cfg}, &out, &errb); code != 1 {
 		t.Errorf("exit = %d, want 1", code)
 	}
+	// Asserted before the mode is restored, so the message is the one a user with a genuinely
+	// unreadable file sees.
+	if got := errb.String(); !strings.Contains(got, "--merge=false") {
+		t.Errorf("stderr does not name the remedy that repairs this:\n%s", got)
+	}
 	if err := os.Chmod(path, 0o600); err != nil {
 		t.Fatal(err)
 	}
@@ -480,6 +485,46 @@ func TestReadClaudeSessions_MergeRefusesAnUnreadableFile(t *testing.T) {
 	}
 }
 
+// AND THE REMEDY THE REFUSAL NAMES ACTUALLY WORKS. Without this, the advice in the message
+// above is only asserted to be PRESENT, not to be true — which is how it came to be deleted
+// as impossible in the first place: --merge=false skips the read entirely (Harvest reads the
+// file only under Merge) and the save renames over the path, so the old file never has to be
+// readable. Same fixture as the refusal, one flag different.
+func TestReadClaudeSessions_MergeFalseRepairsAnUnreadableFile(t *testing.T) {
+	if os.Geteuid() == 0 {
+		t.Skip("root ignores the mode bits this test relies on")
+	}
+	home := prefsHome(t)
+	path := filepath.Join(home, tui.SessionMetadataRel)
+	if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(path, []byte(`{"old":{"title":"keep me"}}`), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.Chmod(path, 0o000); err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() { _ = os.Chmod(path, 0o600) })
+
+	cfg := filepath.Join(t.TempDir(), "claude")
+	writeSessionTranscript(t, filepath.Join(cfg, "projects", "-p"), "fresh.jsonl",
+		`{"type":"ai-title","aiTitle":"new"}`)
+
+	var out, errb bytes.Buffer
+	if code := runExperimental([]string{"read-claude-sessions", "--dir", cfg, "--merge=false"}, &out, &errb); code != 0 {
+		t.Fatalf("exit = %d, want 0; stderr:\n%s", code, errb.String())
+	}
+	if got := readMetadataFile(t, path)["fresh"].Title; got != "new" {
+		t.Errorf("Meta[fresh].Title = %q, want the rebuild to have written it", got)
+	}
+	// The old entry is gone, which is what --merge=false MEANS. Asserted so the test cannot be
+	// read as claiming the unreadable entries were somehow recovered.
+	if _, ok := readMetadataFile(t, path)["old"]; ok {
+		t.Error("the old entry survived a --merge=false rebuild")
+	}
+}
+
 // An oversized but valid file is refused with its OWN remedy — not the permission advice, which
 // would be wrong: the file is intact and readable, just over the read cap.
 func TestReadClaudeSessions_OversizedFileNamesTheRightRepair(t *testing.T) {
@@ -507,6 +552,12 @@ func TestReadClaudeSessions_OversizedFileNamesTheRightRepair(t *testing.T) {
 	if got := errb.String(); strings.Contains(got, "permissions") {
 		t.Errorf("stderr gives the permission remedy for an intact file:\n%s", got)
 	}
+	// The remedy that actually works, asserted because it was once removed on the theory that
+	// --merge=false "would hit the same read". It does not: Harvest reads the file only under
+	// Merge, and the save renames over the path. Measured against a 17.7 MB file.
+	if got := errb.String(); !strings.Contains(got, "--merge=false") {
+		t.Errorf("stderr does not name the remedy that repairs this without deleting anything:\n%s", got)
+	}
 	if got := fileSize(t, path); got != size {
 		t.Errorf("file is %d bytes, was %d — it was rewritten", got, size)
 	}
diff --git a/authbridge/cmd/abctl/main.go b/authbridge/cmd/abctl/main.go
index 1d52343f0..3aeedf1d0 100644
--- a/authbridge/cmd/abctl/main.go
+++ b/authbridge/cmd/abctl/main.go
@@ -275,8 +275,18 @@ func claudeHarvester(warn io.Writer) tui.HarvestFunc {
 		// name an action and repeat every launch, which is worth printing before the alt screen
 		// goes up. Not warning about the rest is the deliberate half — the common case among them
 		// is the file that does not parse, which heals itself moments later, so a line here would
-		// tell the operator titles are in trouble and then hand them titles. A harvest that does
-		// go on to fail reports itself through the viewer's own error path.
+		// tell the operator titles are in trouble and then hand them titles.
+		//
+		// WHAT THAT COSTS, stated plainly because there is no other record of it: the rarer
+		// errors here — EIO, EISDIR, ELOOP — are ones Harvest also refuses, and nothing
+		// downstream reports them. harvestCmd does `meta, _ := h()` (tui/session_metadata.go)
+		// and drops the error on purpose, so those launch with no titles and no explanation,
+		// every time, exactly the shape of bug this fix exists to remove. It is narrower than
+		// what was fixed — a directory or a symlink loop where the metadata file belongs is not
+		// a state a user reaches by accident, whereas a truncated write is — and closing it
+		// properly means either exporting the parse sentinel or giving the TUI somewhere to put
+		// a late error. Both are bigger than this change; #1110 is about the file that does not
+		// parse and the wedged lock.
 	}
 	return func() (map[string]tui.SessionMetadata, error) {
 		// Incremental, unlike `abctl experimental read-claude-sessions`: that command's subject

From 3f56d0bb798f5b1ce6868508229ace3138ed3716 Mon Sep 17 00:00:00 2001
From: Ed Snible <snible@us.ibm.com>
Date: Thu, 24 Sep 2026 13:33:18 -0400
Subject: [PATCH 6/6] fix: Refuse an oversize metadata file in the viewer's
 reader too
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Three review items.

1. tui/session_metadata.go: the read was capped at exactly
   maxMetadataBytes with no +1, so an oversize file was decoded from a
   truncated buffer. Measured both outcomes: cut mid-token, Unmarshal
   fails and the result is the same empty map as before, no change; cut
   where the prefix is independently valid JSON (trailing whitespace past
   the cap), the prefix DECODED and the viewer returned a partial map as
   though it were the whole file, while claude.ReadMetadata refused the
   identical bytes. 16,777,238 B loaded 1 entry before, 0 after. That
   second shape is the bug: a partial read indistinguishable from a
   complete one.

   The review's framing (total silent loss at 191 B over the cap) does
   not reproduce — that boundary is the mid-token case and behaves the
   same before and after. Fixed anyway: the two readers of this file must
   agree on which files are too large.

2. harvest.go: the "both now cap it, and for the same reason" parity
   claim was false and mine, added last commit. The cap is shared; the
   contract at it is not, deliberately — ReadMetadata has a caller that
   can act and refuses loudly, LoadSessionMetadata has nowhere to report
   and returns empty. Documented as the asymmetry it is.

3. harvest.go: the lock switch had no default: arm, so a flock failure
   that was not a timeout proceeded unlocked with nothing recorded — the
   one state where a concurrent rename erases everything was the one
   state a caller could not report. Added Result.LockFailed, separate
   from LockTimedOut because "timed out" for an ENOLCK sends the reader
   hunting a process that does not exist, and reported by the CLI.

Mutations: reverting the cap fix fails the new boundary test (M20);
removing the default arm fails the lock test (M21). Windows cross-build
checked, since lock_other.go never sets the new field.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Ed Snible <snible@us.ibm.com>
---
 authbridge/authlib/observe/claude/harvest.go  | 38 +++++++++--
 .../authlib/observe/claude/lock_unix_test.go  | 65 +++++++++++++++++++
 authbridge/cmd/abctl/cmd_experimental.go      |  7 ++
 authbridge/cmd/abctl/tui/session_metadata.go  | 26 +++++++-
 .../cmd/abctl/tui/sessions_title_test.go      | 48 ++++++++++++++
 5 files changed, 178 insertions(+), 6 deletions(-)

diff --git a/authbridge/authlib/observe/claude/harvest.go b/authbridge/authlib/observe/claude/harvest.go
index 230ed12dc..1abb51ce1 100644
--- a/authbridge/authlib/observe/claude/harvest.go
+++ b/authbridge/authlib/observe/claude/harvest.go
@@ -151,6 +151,18 @@ type Result struct {
 	// A field because the alternative is silence: without it, the one failure mode this lock
 	// exists to prevent is invisible from outside, and "some titles vanished" has no diagnosis.
 	LockTimedOut bool
+	// LockFailed carries a lock failure that was NOT a timeout — flock refused for some other
+	// reason, e.g. the lock file could not be created or the filesystem rejected the call.
+	//
+	// Separate from LockTimedOut because the two need different words: a timeout means someone
+	// else holds it, while this means locking did not work at all, and reporting "timed out" for
+	// an EIO would send the reader looking for a process that does not exist. Empty when locking
+	// succeeded, was not attempted (no Merge), or is a no-op on this platform.
+	//
+	// Same consequence as a timeout, which is why it is reported at all: the harvest proceeds
+	// UNLOCKED, so a concurrent run's rename can erase everything it wrote. Before this, that
+	// state was the only one where wholesale loss was possible and nothing recorded it.
+	LockFailed string
 	// Meta is what the run wrote: the merged whole under Merge, or just this harvest
 	// otherwise. Keyed by session id.
 	//
@@ -212,6 +224,12 @@ func Harvest(opts Options) (Result, error) {
 			defer unlock()
 		case errors.Is(lerr, ErrLockTimeout):
 			res.LockTimedOut = true
+		default:
+			// ARMED, because the fallback is the same as a timeout's — harvest unlocked — but
+			// nothing said so. A flock that fails for any other reason left LockTimedOut false
+			// and no other trace, so the one state where a concurrent rename can erase every
+			// entry this run wrote was also the one state a caller could not report.
+			res.LockFailed = lerr.Error()
 		}
 	}
 
@@ -1834,11 +1852,21 @@ func ReadMetadata(path string) (map[string]SessionMetadata, error) {
 		return nil, err
 	}
 	defer f.Close() //nolint:errcheck // read-only
-	// BOUNDED, the same 16 MiB the viewer's own reader of this file uses. Both now cap it, and
-	// for the same reason: a stray large file at this path would otherwise be read whole and
-	// decoded before the viewer starts — `abctl observe` calls this synchronously to check the
-	// file is readable, so an unbounded read stalls startup with nothing on screen to say why.
-	// Far past any real metadata file: the measured 192-session file is 74 KB.
+	// BOUNDED at the same 16 MiB the viewer's own reader of this file uses, for the same reason:
+	// a stray large file at this path would otherwise be read whole and decoded before the viewer
+	// starts — `abctl observe` calls this synchronously to check the file is readable, so an
+	// unbounded read stalls startup with nothing on screen to say why. Far past any real metadata
+	// file: the measured 192-session file is 74 KB.
+	//
+	// THE CAP IS SHARED; THE CONTRACT AT IT IS NOT, and the difference is deliberate rather than
+	// an oversight to be unified. This function has a caller that can act — it returns an error,
+	// so the CLI and the pre-flight both name a remedy — and refuses loudly with
+	// ErrMetadataTooLarge. tui.LoadSessionMetadata has nowhere to report anything (it runs while
+	// the model is built, before tea.NewProgram owns the screen) and so returns an empty map,
+	// costing the TITLE column. What both must agree on is WHICH files are too large, which is
+	// why the constant and the read-one-past-it shape are duplicated there rather than eyeballed:
+	// they once disagreed, and the viewer silently decoded a truncated prefix of a file this one
+	// refused whole.
 	//
 	// Truncation is reported as ITS OWN failure, not left to surface as a JSON error. A valid
 	// file over the cap decodes as a parse failure, and Harvest rebuilds over parse failures —
diff --git a/authbridge/authlib/observe/claude/lock_unix_test.go b/authbridge/authlib/observe/claude/lock_unix_test.go
index 0c9f0c164..7f666d39d 100644
--- a/authbridge/authlib/observe/claude/lock_unix_test.go
+++ b/authbridge/authlib/observe/claude/lock_unix_test.go
@@ -164,3 +164,68 @@ func TestHarvest_ProceedsWhenTheLockIsHeld(t *testing.T) {
 		t.Error("LockTimedOut is false after the harvest ran unlocked")
 	}
 }
+
+// A flock failure that is NOT a timeout is recorded too, and the harvest still runs.
+//
+// The default: arm this covers was missing: a lock that failed for any reason other than the
+// deadline left LockTimedOut false and nothing else set, so the one state where a concurrent
+// rename can erase every entry a run wrote was also the one state a caller could not report.
+//
+// Provoked through a metadata directory that cannot be written, which makes the lock FILE
+// uncreatable — a real case (read-only home, restrictive mode) and the only non-timeout lock
+// failure a unit test can produce without a filesystem that refuses flock outright.
+func TestHarvest_NonTimeoutLockFailureIsRecorded(t *testing.T) {
+	if os.Geteuid() == 0 {
+		t.Skip("root ignores the directory mode this test relies on")
+	}
+	metadataHome(t)
+	cfg := filepath.Join(t.TempDir(), "claude")
+	writeSessionTranscript(t, filepath.Join(cfg, "projects", "-p"), "s1.jsonl",
+		`{"type":"ai-title","aiTitle":"t"}`)
+	path, err := SessionMetadataPath()
+	if err != nil {
+		t.Fatal(err)
+	}
+	dir := filepath.Dir(path)
+	if err := os.MkdirAll(dir, 0o700); err != nil {
+		t.Fatal(err)
+	}
+	// 0o500: readable and traversable, so the harvest can still try, but no new file can be
+	// created in it — which is what the lock needs.
+	if err := os.Chmod(dir, 0o500); err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() { _ = os.Chmod(dir, 0o700) })
+
+	res, _ := Harvest(Options{ConfigDir: cfg, Merge: true})
+	// The save fails too — the directory is unwritable, that is the point — so the error is not
+	// asserted. What must hold is that the lock failure was RECORDED rather than swallowed.
+	if res.LockFailed == "" {
+		t.Error("LockFailed is empty after the lock could not be created")
+	}
+	// Not mislabelled as a timeout: nobody was holding it, and saying so would send a reader
+	// looking for a process that does not exist.
+	if res.LockTimedOut {
+		t.Error("LockTimedOut is true for a failure that was not a timeout")
+	}
+}
+
+// The mirror: a lock that works reports neither field. Without this the pair above would pass
+// against code that set LockFailed unconditionally.
+func TestHarvest_SuccessfulLockReportsNeitherFailure(t *testing.T) {
+	metadataHome(t)
+	cfg := filepath.Join(t.TempDir(), "claude")
+	writeSessionTranscript(t, filepath.Join(cfg, "projects", "-p"), "s1.jsonl",
+		`{"type":"ai-title","aiTitle":"t"}`)
+
+	res, err := Harvest(Options{ConfigDir: cfg, Merge: true})
+	if err != nil {
+		t.Fatalf("Harvest: %v", err)
+	}
+	if res.LockFailed != "" {
+		t.Errorf("LockFailed = %q with a working lock", res.LockFailed)
+	}
+	if res.LockTimedOut {
+		t.Error("LockTimedOut is true with a working lock")
+	}
+}
diff --git a/authbridge/cmd/abctl/cmd_experimental.go b/authbridge/cmd/abctl/cmd_experimental.go
index db05539d4..14f65b435 100644
--- a/authbridge/cmd/abctl/cmd_experimental.go
+++ b/authbridge/cmd/abctl/cmd_experimental.go
@@ -196,6 +196,13 @@ func runReadClaudeSessions(args []string, stdout, stderr io.Writer) int {
 		fmt.Fprintf(stderr, "abctl: timed out waiting for the lock on %s; harvested anyway, so a concurrent run may have overwritten this one\n", res.Path)
 	}
 
+	// Same risk, different cause, so different words: a timeout means another run holds the lock,
+	// this means locking did not work at all. Reported for the same reason — the harvest ran
+	// unlocked either way.
+	if res.LockFailed != "" {
+		fmt.Fprintf(stderr, "abctl: could not lock %s (%s); harvested anyway, so a concurrent run may have overwritten this one\n", res.Path, res.LockFailed)
+	}
+
 	// Said out loud because the counts cannot show it: a rebuild reports everything harvested
 	// and nothing kept, which is exactly what a first run reports. The entries it dropped —
 	// sessions whose transcripts are gone — leave no trace for the operator to notice.
diff --git a/authbridge/cmd/abctl/tui/session_metadata.go b/authbridge/cmd/abctl/tui/session_metadata.go
index 7756ec1ff..c2464c527 100644
--- a/authbridge/cmd/abctl/tui/session_metadata.go
+++ b/authbridge/cmd/abctl/tui/session_metadata.go
@@ -58,16 +58,40 @@ func LoadSessionMetadata(path string) map[string]SessionMetadata {
 	// before the TUI starts, stalling startup with nothing on screen to say why — the load
 	// happens while the model is built, before tea.NewProgram, so there is nowhere to report
 	// it. 16 MiB is far past any real metadata file: the measured 109-session file is 42 KB.
+	//
+	// READ ONE PAST THE CAP, and bail on over-cap rather than decoding. Without the +1 the file
+	// was read to exactly the cap and the truncated buffer handed to Unmarshal, and what that
+	// produced depended on where the cut landed. Measured both ways:
+	//
+	//   - Cut mid-token, the common case: Unmarshal fails with "unexpected end of JSON input"
+	//     and the result is this same empty map. No behaviour change from the fix.
+	//   - Cut where the prefix is INDEPENDENTLY VALID JSON — trailing whitespace past the cap
+	//     does it — the prefix decoded and this function returned a PARTIAL map as though it
+	//     were the whole file, while claude.ReadMetadata refused the identical bytes. A
+	//     16,777,238-byte file loaded 1 entry before the fix and 0 after.
+	//
+	// The second is what the +1 is for: not the volume of titles lost, but that a partial read
+	// was indistinguishable from a complete one.
+	//
+	// Still returns the empty map, because this function has nowhere to report anything and
+	// says so above — but claude.ReadMetadata applies the SAME cap and DOES distinguish it,
+	// carrying ErrMetadataTooLarge, and `abctl observe`'s pre-flight calls that before the alt
+	// screen goes up. So the operator gets the one line naming the remedy from there; what
+	// this bail-out buys is that the two readers agree on which files are too large, instead
+	// of one silently reading a prefix the other refuses whole.
 	const maxMetadataBytes = 16 << 20
 	f, err := os.Open(path) //nolint:gosec // operator-supplied path
 	if err != nil {
 		return out
 	}
 	defer f.Close() //nolint:errcheck // read-only
-	b, err := io.ReadAll(io.LimitReader(f, maxMetadataBytes))
+	b, err := io.ReadAll(io.LimitReader(f, maxMetadataBytes+1))
 	if err != nil {
 		return out
 	}
+	if len(b) > maxMetadataBytes {
+		return out
+	}
 	var m map[string]SessionMetadata
 	if err := json.Unmarshal(b, &m); err != nil {
 		return out
diff --git a/authbridge/cmd/abctl/tui/sessions_title_test.go b/authbridge/cmd/abctl/tui/sessions_title_test.go
index 534be3827..0b8a8aa4e 100644
--- a/authbridge/cmd/abctl/tui/sessions_title_test.go
+++ b/authbridge/cmd/abctl/tui/sessions_title_test.go
@@ -2120,3 +2120,51 @@ func TestRefreshTick_FreshRowStillWaitsToSettle(t *testing.T) {
 			"exists because the title tiers read a transcript the agent has not finished writing")
 	}
 }
+
+// An oversized file must be REFUSED WHOLE, not read as a valid prefix of itself.
+//
+// What the cap does and does not protect against, measured rather than assumed. A file over the
+// limit was read to exactly maxMetadataBytes with no +1, and what happened next depended entirely
+// on where the cut landed:
+//
+//   - Cut mid-token — the overwhelmingly common case — Unmarshal fails with "unexpected end of
+//     JSON input" and the result is the empty map. Identical before and after this fix, and not
+//     data loss beyond the TITLE column this function is contracted to lose on any failure.
+//   - Cut where the prefix is INDEPENDENTLY VALID JSON, which trailing whitespace past the cap
+//     produces: the prefix decoded and the viewer loaded a partial map, believing it complete,
+//     while claude.ReadMetadata refused the very same file with ErrMetadataTooLarge. Measured: a
+//     16,777,238-byte file loaded 1 entry before the fix and 0 after.
+//
+// The second is the one worth a test, because it is the only shape where the two readers of this
+// file disagreed about its CONTENTS rather than merely about how loudly to fail.
+func TestLoadSessionMetadata_AtTheReadCap(t *testing.T) {
+	const cap = 16 << 20
+	path := filepath.Join(t.TempDir(), "session-metadata.json")
+
+	// A valid object followed by enough whitespace to push the file past the cap. The first
+	// cap bytes are therefore valid JSON on their own — which is exactly what makes a truncated
+	// read decode successfully and silently drop whatever followed.
+	body := `{"a":{"title":"real"}}`
+	over := []byte(body + strings.Repeat(" ", cap))
+	if len(over) <= cap {
+		t.Fatalf("fixture is %d bytes, within the %d cap: it cannot test the over case", len(over), cap)
+	}
+	if err := os.WriteFile(path, over, 0o600); err != nil {
+		t.Fatal(err)
+	}
+	// Zero, not one. One means a prefix decoded and the viewer is now showing a file it only
+	// partly read, with no error anywhere and claude.ReadMetadata refusing the same bytes.
+	if got := LoadSessionMetadata(path); len(got) != 0 {
+		t.Errorf("loaded %d entries from a %d byte file: a valid prefix of an oversize file decoded",
+			len(got), len(over))
+	}
+
+	// And the cap still admits everything under it, so the guard is a limit and not a wall.
+	under := []byte(body + strings.Repeat(" ", 1024))
+	if err := os.WriteFile(path, under, 0o600); err != nil {
+		t.Fatal(err)
+	}
+	if got := LoadSessionMetadata(path); len(got) != 1 || got["a"].Title != "real" {
+		t.Errorf("loaded %d entries from a %d byte file, want the 1 real entry", len(got), len(under))
+	}
+}