From fcc3df07fb591a5f427b97c65ce25369291db7a4 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Fri, 25 Sep 2026 14:36:32 +0100 Subject: [PATCH 01/36] fix(history): stage each agent behind its own lock The staging directory had one lock, and a contended flock admits roughly ten writers a second (the helper's backoff ceiling sets the rate), so a burst of simultaneous sub-agent completions past about fifty hit the five-second staging timeout and lost the tail of the burst. Per the product thinker's M22 ruling (2026-09-23), each staged key (agent id, or the session id for a main thread) now has its own lock file under staging/locks/, taken by Stage, the drain's remove-if-unchanged, quarantine and Discard alike, each deriving the key from the staged file's own name. A lock file is retired with the staged file it guards, so the directory does not grow one empty file per sub-agent ever run. That is made safe by a change to the one lock primitive: fsutil.WithFileLock now revalidates, after the flock is granted, that the path still names the locked inode, and starts over on the current file when a holder unlinked it. Refs: iss-2609090828371674 Assisted-by: Claude:claude-opus-5-5 --- .../brief/04-surfaces/11-history.md | 7 +- internal/core/history/staging.go | 88 ++++++++++-- internal/core/history/staging_lifetime.go | 28 ++-- internal/core/history/staging_shard_test.go | 129 ++++++++++++++++++ internal/fsutil/flock.go | 60 ++++++-- internal/fsutil/flock_test.go | 78 +++++++++++ 6 files changed, 357 insertions(+), 33 deletions(-) create mode 100644 internal/core/history/staging_shard_test.go create mode 100644 internal/fsutil/flock_test.go diff --git a/.abcd/development/brief/04-surfaces/11-history.md b/.abcd/development/brief/04-surfaces/11-history.md index a961378fe..ede327c72 100644 --- a/.abcd/development/brief/04-surfaces/11-history.md +++ b/.abcd/development/brief/04-surfaces/11-history.md @@ -210,7 +210,12 @@ which is what it is. The handshake is locked and keyed on content per one carrying different bytes replaces the staged copy (the later snapshot of a session is the one worth keeping), and a drain removes a staged file only while it still holds the bytes it captured. One `(session, agent)` has one staged -copy, and a fresher copy is never lost (GHSA-xq36-hcgf-9wrj). +copy, and a fresher copy is never lost (GHSA-xq36-hcgf-9wrj). The lock is +per agent, never per repository: each staged file's key (its agent id, or the +session id for a main thread) has its own lock file under `staging/locks/`, so +a burst of simultaneous sub-agent completions stages in parallel instead of +queuing behind one lock whose timeout would refuse the tail of the burst. A +lock file is removed with the staged file it guards. Staging is also **the outcome record the store never had.** Before it, an absent record spanned "never ended", "ended before the store existed" and "ended and diff --git a/internal/core/history/staging.go b/internal/core/history/staging.go index 8a1fd3b6a..a5e5a9d03 100644 --- a/internal/core/history/staging.go +++ b/internal/core/history/staging.go @@ -75,15 +75,37 @@ const stageSidecarSuffix = ".stage.json" // corrupted file. const stageSidecarSchema = 1 -// stagingLockFilename is the per-repo staging lock, a sibling of the staged -// files (listStaged filters on stagedSuffix, so the lock is invisible to it). -// Every writer of the staging dir — Stage's list-compare-write and Drain's -// remove-if-unchanged — takes it through fsutil.WithFileLock, the one -// inter-process load-modify-write primitive, so the per-session idempotency +// stagingLocksDirName is the directory, under staging/, that holds the staging +// locks: one lock file per staged key, never one for the whole directory. +// +// The shape is the product thinker's ruling on iss-2609090828371674 (M22, +// 2026-09-23): each agent stages behind its own lock. A single per-repo lock +// admitted roughly ten writers a second under contention — the flock helper's +// backoff ceiling sets that rate, not the critical section — so a burst of +// simultaneous sub-agent completions past about fifty ran into the staging +// timeout, and a stage that times out is a transcript written nowhere. Keyed +// per agent, distinct agents never wait on each other, and the only contention +// left is the one the lock exists for: one agent's own re-fired stage against +// its own drain. +// +// The key is the staged filename's key (stagedKey: the agent id, or the session +// id for a main thread), so every mutator of one staged file — Stage's +// list-compare-write, Drain's remove-if-unchanged, quarantine's move and +// Discard — derives the same lock from the file's own name without reading its +// sidecar. Each goes through fsutil.WithFileLock, the one inter-process +// load-modify-write primitive, so the per-(session, agent) idempotency // guarantee holds across concurrent hooks and not just single-threaded -// (GHSA-xq36-hcgf-9wrj). It nests inside nothing: Drain releases it before -// Capture takes the store's repoLock, so the two can never wait on each other. -const stagingLockFilename = ".lock" +// (GHSA-xq36-hcgf-9wrj). A lock file is retired by whichever mutator retires +// the staged file it guards, which WithFileLock's inode revalidation makes safe, +// so the directory holds a lock only for a key that has something staged. It +// nests inside nothing: Drain releases it before Capture takes the store's +// repoLock, so the two can never wait on each other. It is a directory, so +// listStaged — which skips directories and filters on stagedSuffix — never sees +// a lock. +const stagingLocksDirName = "locks" + +// stagingLockSuffix ends every staging lock filename. +const stagingLockSuffix = ".lock" // stagingLockTimeout bounds how long a staging writer waits for the lock. It is // short because the SessionEnd hook must never wedge the session it is ending: @@ -384,7 +406,7 @@ func Stage(repoRoot, rootSHA string, meta StageMeta, raw []byte) (StageResult, e return StageResult{}, err } var res StageResult - err = withStagingLock(sdir, func() error { + err = withStagingLock(sdir, stagedKey(meta.Lineage), func() error { var err error res, err = stageLocked(sdir, meta, raw) return err @@ -395,17 +417,51 @@ func Stage(repoRoot, rootSHA string, meta StageMeta, raw []byte) (StageResult, e return res, nil } -// withStagingLock runs fn under the staging lock, naming the lock in the error -// when the primitive itself refuses (contention, or an unsafe lock path); fn's -// own error passes through unchanged. -func withStagingLock(sdir string, fn func() error) error { - err := fsutil.WithFileLock(filepath.Join(sdir, stagingLockFilename), stagingLockTimeout, fn) +// stagingLockPath is the lock file guarding every staged file whose filename +// key is key. +func stagingLockPath(sdir, key string) string { + return filepath.Join(sdir, stagingLocksDirName, key+stagingLockSuffix) +} + +// stagedLockKey is the lock key for a staged (or quarantined) file, read from +// its filename: the key stagedFilename wrote after the stamp. A name that does +// not carry the stamp shape falls back to the whole name without its suffix, +// which still names one file and still contains no separator (every caller +// has already refused one). +func stagedLockKey(path string) string { + name := filepath.Base(path) + if key := sessionIDFromStaged(name); key != "" && sessionIDRe.MatchString(key) { + return key + } + return strings.TrimSuffix(name, stagedSuffix) +} + +// withStagingLock runs fn under the lock for one staged key, naming the lock +// in the error when the primitive itself refuses (contention, or an unsafe +// lock path); fn's own error passes through unchanged. The locks directory is +// created 0o700 on demand and refused if it is anything but a real directory. +func withStagingLock(sdir, key string, fn func() error) error { + locks := filepath.Join(sdir, stagingLocksDirName) + if err := fsutil.EnsureRealDir(locks, storeDirPerm); err != nil { + return fmt.Errorf("history: staging lock: %w", storeDirFault(locks, err)) + } + err := fsutil.WithFileLock(stagingLockPath(sdir, key), stagingLockTimeout, fn) if errors.Is(err, fsutil.ErrLockContention) || errors.Is(err, fsutil.ErrLockPathUnsafe) { return fmt.Errorf("history: staging lock: %w", err) } return err } +// retireStagingLock removes the lock file for key. It is called only by a +// holder of that lock, inside fn, once the staged file the lock guards has +// left the staging directory. A waiter already queued on the retired file +// notices on acquisition and starts over on the current one (fsutil's inode +// revalidation), so removing it costs nobody their exclusion. Failure is +// ignored: a lock file left behind is an empty file, never a wrong answer. +func retireStagingLock(sdir, key string) { + _ = os.Remove(stagingLockPath(sdir, key)) +} + // stageLocked is Stage's critical section. listStaged is oldest-first, so when // a (session, agent) has several copies (a staging dir written before the lock // existed) the newest is the one compared and replaced; the drain retires the @@ -798,7 +854,8 @@ func refreshedFromSource(s Staged, stagedBytes []byte) (body []byte, extended bo // id where a session id belongs. func removeStagedIfUnchanged(sdir string, s Staged, read []byte) error { want := sha256.Sum256(read) - return withStagingLock(sdir, func() error { + key := stagedLockKey(s.Path) + return withStagingLock(sdir, key, func() error { current, err := fsutil.ReadGuarded(s.Path, maxTranscriptBytes) if err != nil { if errors.Is(err, os.ErrNotExist) { @@ -815,6 +872,7 @@ func removeStagedIfUnchanged(sdir string, s Staged, read []byte) error { if err := os.Remove(sidecarPathFor(s.Path)); err != nil && !errors.Is(err, os.ErrNotExist) { return err } + retireStagingLock(sdir, key) return nil }) } diff --git a/internal/core/history/staging_lifetime.go b/internal/core/history/staging_lifetime.go index a104b30b7..3e22aec1f 100644 --- a/internal/core/history/staging_lifetime.go +++ b/internal/core/history/staging_lifetime.go @@ -183,7 +183,8 @@ func quarantineStaged(sdir, qdir string, s Staged, read []byte, reason string) ( base := filepath.Base(s.Path) qpath := filepath.Join(qdir, base) var moved string - err := withStagingLock(sdir, func() error { + key := stagedLockKey(s.Path) + err := withStagingLock(sdir, key, func() error { current, err := fsutil.ReadGuarded(s.Path, maxTranscriptBytes) if err != nil { return err @@ -225,6 +226,7 @@ func quarantineStaged(sdir, qdir string, s Staged, read []byte, reason string) ( // invisible to listStaged, so it strands nothing. _ = os.Rename(s.SidecarPath, sidecarPathFor(qpath)) } + retireStagingLock(sdir, key) moved = qpath return nil }) @@ -371,15 +373,25 @@ func Discard(repoRoot, rootSHA, name string) (DiscardResult, error) { } return nil } - // The staging lock covers the staging directory's mutators; taking it - // for a quarantined file too is harmless (it is the same per-repo lock) - // and keeps a discard from racing a drain that is mid-move. When the - // staging directory does not exist there is no lock file to take and no - // drain to race — a drain requires it — so the removal runs unlocked - // rather than creating a directory in order to delete something else. + // The file's own staging lock (keyed by its filename, exactly as Stage + // and the drain key it) covers the staging directory's mutators; taking + // it for a quarantined file too is harmless and keeps a discard from + // racing a drain that is mid-move. When the staging directory does not + // exist there is no drain to race — a drain requires it — so the + // removal runs unlocked rather than creating a directory in order to + // delete something else. derr := remove if fsutil.IsRealDir(sdir) { - derr = func() error { return withStagingLock(sdir, remove) } + key := stagedLockKey(name) + derr = func() error { + return withStagingLock(sdir, key, func() error { + if err := remove(); err != nil { + return err + } + retireStagingLock(sdir, key) + return nil + }) + } } if err := derr(); err != nil { return DiscardResult{}, fmt.Errorf("history: discard %s: %w", name, err) diff --git a/internal/core/history/staging_shard_test.go b/internal/core/history/staging_shard_test.go new file mode 100644 index 000000000..83ace7243 --- /dev/null +++ b/internal/core/history/staging_shard_test.go @@ -0,0 +1,129 @@ +package history + +import ( + "os" + "path/filepath" + "strconv" + "sync" + "testing" + "time" + + "github.com/intentdriven/abcd/internal/fsutil" +) + +// TestStageFanOutBurstLosesNothing is the M22 ruling (iss-2609090828371674, +// 2026-09-23): each agent stages behind its own lock. With one shared staging +// lock a contended flock admits roughly ten writers a second (the helper's +// backoff ceiling, not the critical section, sets the rate), so a burst of +// simultaneous sub-agent completions past about fifty exceeds the staging +// timeout and every stage that times out is a transcript written nowhere. +// Distinct agents must not queue behind one another at all. +func TestStageFanOutBurstLosesNothing(t *testing.T) { + repoRoot, _ := setupStore(t) + const agents = 64 + start := make(chan struct{}) + var wg sync.WaitGroup + errs := make(chan error, agents) + for i := 0; i < agents; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + <-start + agent := "agent-fan-" + strconv.Itoa(i) + if _, err := Stage(repoRoot, testRootSHA, subAgentStage("sess-fan", agent), []byte("branch "+agent+"\n")); err != nil { + errs <- err + } + }(i) + } + close(start) + wg.Wait() + close(errs) + for err := range errs { + t.Errorf("Stage in a %d-agent burst: %v", agents, err) + } + staged, err := ListStaged(repoRoot, testRootSHA) + if err != nil { + t.Fatal(err) + } + seen := map[string]int{} + for _, s := range staged { + seen[s.AgentID]++ + } + for i := 0; i < agents; i++ { + if n := seen["agent-fan-"+strconv.Itoa(i)]; n != 1 { + t.Errorf("agent-fan-%d: %d staged copies, want 1", i, n) + } + } +} + +// TestStageLockIsPerAgent pins the shape of the ruling rather than its rate: +// while one agent's staging lock is held, a different agent of the same +// session still stages, and the held agent's own stage is the one that waits. +func TestStageLockIsPerAgent(t *testing.T) { + repoRoot, home := setupStore(t) + if _, err := Stage(repoRoot, testRootSHA, subAgentStage("sess-lk", "agent-held"), []byte("first\n")); err != nil { + t.Fatal(err) + } + sdir := stagingDir(home) + held := make(chan struct{}) + release := make(chan struct{}) + done := make(chan error, 1) + go func() { + done <- fsutil.WithFileLock(stagingLockPath(sdir, "agent-held"), 5*time.Second, func() error { + close(held) + <-release + return nil + }) + }() + <-held + if _, err := Stage(repoRoot, testRootSHA, subAgentStage("sess-lk", "agent-free"), []byte("other\n")); err != nil { + t.Fatalf("a different agent's stage waited on a lock it does not share: %v", err) + } + blocked := make(chan error, 1) + go func() { + _, err := Stage(repoRoot, testRootSHA, subAgentStage("sess-lk", "agent-held"), []byte("second\n")) + blocked <- err + }() + select { + case err := <-blocked: + t.Fatalf("the held agent's own stage did not wait for its lock (err=%v)", err) + case <-time.After(150 * time.Millisecond): + } + close(release) + if err := <-done; err != nil { + t.Fatal(err) + } + if err := <-blocked; err != nil { + t.Fatalf("the held agent's stage after release: %v", err) + } +} + +// TestDrainRetiresTheLockWithTheStagedFile: a per-agent lock that outlived its +// staged file would leave one empty file per sub-agent ever run. Whoever +// retires the staged file retires its lock. +func TestDrainRetiresTheLockWithTheStagedFile(t *testing.T) { + repoRoot, home := setupStore(t) + for _, a := range []string{"agent-r1", "agent-r2"} { + if _, err := Stage(repoRoot, testRootSHA, subAgentStage("sess-r", a), []byte(`{"type":"user","message":{"content":"hi"}}`+"\n")); err != nil { + t.Fatal(err) + } + } + locks := filepath.Join(stagingDir(home), stagingLocksDirName) + if entries, _ := os.ReadDir(locks); len(entries) != 2 { + t.Fatalf("after two stages the locks dir holds %d entries, want 2", len(entries)) + } + res, err := Drain(repoRoot, testRootSHA, DrainBudget{}) + if err != nil { + t.Fatal(err) + } + if len(res.Failed) != 0 { + t.Fatalf("drain failures: %+v", res.Failed) + } + entries, err := os.ReadDir(locks) + if err != nil { + t.Fatal(err) + } + for _, e := range entries { + t.Errorf("lock %s outlived the staged file it guarded", e.Name()) + } +} diff --git a/internal/fsutil/flock.go b/internal/fsutil/flock.go index 7cb22c7d2..96feec4d2 100644 --- a/internal/fsutil/flock.go +++ b/internal/fsutil/flock.go @@ -28,19 +28,61 @@ var ( // The lock is advisory — every writer of the guarded state must take it — and its // scope is one lock file; callers that must not deadlock keep their acquisitions // unnested. +// +// A holder MAY unlink lockPath inside fn, which is how a caller retires a +// per-key lock file once the key has nothing left to guard. That is safe +// because acquisition is revalidated: after the flock is granted, the path must +// still name the inode that was locked. A waiter that opened the file before a +// holder unlinked it would otherwise be granted the flock on the orphaned inode +// while a newcomer holds the fresh file at the same path — two holders of one +// lock. On a mismatch the waiter releases, reopens the current file and waits +// again, all within the one timeout. func WithFileLock(lockPath string, timeout time.Duration, fn func() error) error { - fd, err := openLockFd(lockPath) - if err != nil { - return err + deadline := time.Now().Add(timeout) + for { + fd, err := openLockFd(lockPath) + if err != nil { + return err + } + if err := acquireFlock(fd, time.Until(deadline)); err != nil { + syscall.Close(fd) + return err + } + current, err := lockStillNamesFd(lockPath, fd) + if err != nil { + syscall.Flock(fd, syscall.LOCK_UN) + syscall.Close(fd) + return err + } + if !current { + syscall.Flock(fd, syscall.LOCK_UN) + syscall.Close(fd) + continue + } + defer syscall.Close(fd) + defer syscall.Flock(fd, syscall.LOCK_UN) + return fn() } - defer syscall.Close(fd) +} - if err := acquireFlock(fd, timeout); err != nil { - return err +// lockStillNamesFd reports whether lockPath still names the inode fd holds. An +// absent path (a holder retired it) is false, not an error; a path that is now +// a symlink is refused as ErrLockPathUnsafe rather than followed. +func lockStillNamesFd(lockPath string, fd int) (bool, error) { + var held, named syscall.Stat_t + if err := syscall.Fstat(fd, &held); err != nil { + return false, err } - defer syscall.Flock(fd, syscall.LOCK_UN) - - return fn() + if err := syscall.Lstat(lockPath, &named); err != nil { + if err == syscall.ENOENT { + return false, nil + } + return false, err + } + if named.Mode&syscall.S_IFMT == syscall.S_IFLNK { + return false, fmt.Errorf("%w: lock path is a symlink: %s", ErrLockPathUnsafe, lockPath) + } + return held.Dev == named.Dev && held.Ino == named.Ino, nil } // openLockFd opens lockPath with O_CREAT|O_RDWR|O_NOFOLLOW and verifies, on the diff --git a/internal/fsutil/flock_test.go b/internal/fsutil/flock_test.go new file mode 100644 index 000000000..1a24b55c8 --- /dev/null +++ b/internal/fsutil/flock_test.go @@ -0,0 +1,78 @@ +package fsutil + +import ( + "os" + "path/filepath" + "sync" + "sync/atomic" + "testing" + "time" +) + +// TestWithFileLockSurvivesAHolderUnlinkingThePath pins the inode revalidation +// that lets a holder retire its lock file. A waiter that opened the lock path +// before the holder unlinked it would otherwise acquire the flock on the +// orphaned inode while a newcomer holds the fresh file at the same path: two +// holders of one lock. After acquiring, a waiter must confirm the path still +// names the inode it locked, and start over on the current file when it does +// not. +func TestWithFileLockSurvivesAHolderUnlinkingThePath(t *testing.T) { + lock := filepath.Join(t.TempDir(), "k.lock") + var inside atomic.Int32 + var overlap atomic.Bool + critical := func(hold time.Duration) func() error { + return func() error { + if inside.Add(1) > 1 { + overlap.Store(true) + } + time.Sleep(hold) + inside.Add(-1) + return nil + } + } + + holderIn := make(chan struct{}) + unlinked := make(chan struct{}) + newcomerIn := make(chan struct{}) + var wg sync.WaitGroup + + wg.Add(1) + go func() { // A: holds the original inode, unlinks it, waits for C to hold the fresh one + defer wg.Done() + _ = WithFileLock(lock, 5*time.Second, func() error { + close(holderIn) + time.Sleep(80 * time.Millisecond) // B opens the original inode and starts polling + if err := os.Remove(lock); err != nil { + t.Error(err) + } + close(unlinked) + <-newcomerIn + return nil + }) + }() + <-holderIn + + wg.Add(1) + go func() { // B: opened the original inode before the unlink + defer wg.Done() + if err := WithFileLock(lock, 5*time.Second, critical(20*time.Millisecond)); err != nil { + t.Error(err) + } + }() + + <-unlinked + wg.Add(1) + go func() { // C: creates and holds the fresh file at the same path + defer wg.Done() + if err := WithFileLock(lock, 5*time.Second, func() error { + close(newcomerIn) + return critical(300 * time.Millisecond)() + }); err != nil { + t.Error(err) + } + }() + wg.Wait() + if overlap.Load() { + t.Fatal("two holders were inside the lock at once: a waiter locked the unlinked inode") + } +} From 4c34bb9d70042e18ae8a19ff354dff614599bd05 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Fri, 25 Sep 2026 14:38:54 +0100 Subject: [PATCH 02/36] fix: decode a URL's userinfo before deciding where the login ends git and net/http percent-decode a URL's userinfo, so the colon that separates a login from a password need not appear literally: ssh://user%3Apw@host carries a password exactly as ssh://user:pw@host does. Every site that decided by a literal colon misread it. - ahoy's scrubRemoteUserinfo kept it as a bare ssh login, so the encoded password went to rest in the history registry, and the history.credential_at_rest detector (the scrub disagreeing with its input) never fired. The test now runs on the decoded userinfo, one round as git applies it, and an undecodable userinfo fails closed. - memory ingest's refusal renderers: url.Parse reads user%3Apw as a username containing a colon with no password, so URL.Redacted echoed it whole; the textual fallback kept everything before a literal colon. Both now split the decoded username and mask the rest. Refs: iss-2609020630232658 Assisted-by: Claude:claude-opus-5-5 --- internal/core/ahoy/remote_userinfo.go | 26 +++++++++--- .../core/ahoy/remote_userinfo_encoded_test.go | 32 +++++++++++++++ internal/core/memory/ingest.go | 27 ++++++++++-- internal/core/memory/userinfo_encoded_test.go | 41 +++++++++++++++++++ 4 files changed, 117 insertions(+), 9 deletions(-) create mode 100644 internal/core/ahoy/remote_userinfo_encoded_test.go create mode 100644 internal/core/memory/userinfo_encoded_test.go diff --git a/internal/core/ahoy/remote_userinfo.go b/internal/core/ahoy/remote_userinfo.go index 3973a760b..a3e16a4f1 100644 --- a/internal/core/ahoy/remote_userinfo.go +++ b/internal/core/ahoy/remote_userinfo.go @@ -1,6 +1,9 @@ package ahoy -import "strings" +import ( + "net/url" + "strings" +) // scrubRemoteUserinfo drops any credential from a git remote URL before it // enters RepoIdentity — the one value every registry sink and every JSON @@ -35,9 +38,7 @@ func scrubRemoteUserinfo(s string) string { if at < 0 { return s } - userinfo := authority[:at] - hasPassword := strings.Contains(userinfo, ":") - if !hasPassword && scheme != "http" && scheme != "https" { + if !userinfoCarriesPassword(authority[:at]) && scheme != "http" && scheme != "https" { return s } return s[:i+3] + authority[at+1:] + rest[len(authority):] @@ -56,8 +57,23 @@ func scrubRemoteUserinfo(s string) string { } // A userinfo with no colon is a bare login (`git@host:path`): the transport // needs it and it is a route, not a secret. - if !strings.Contains(s[:at], ":") { + if !userinfoCarriesPassword(s[:at]) { return s } return s[at+1:] } + +// userinfoCarriesPassword reports whether a userinfo holds a login:password +// pair. The test runs on the DECODED userinfo, because git percent-decodes it +// before use: `user%3Apw` is the login "user" with the password "pw", and a +// literal-colon test read it as a bare login (iss-2609020630232658). One round +// of decoding is what git applies, so `%253A` decodes to the literal text +// `%3A` and is not a separator. A userinfo that does not decode is treated as +// carrying one: this function decides what may go to rest, so it fails closed. +func userinfoCarriesPassword(userinfo string) bool { + decoded, err := url.PathUnescape(userinfo) + if err != nil { + return true + } + return strings.Contains(decoded, ":") +} diff --git a/internal/core/ahoy/remote_userinfo_encoded_test.go b/internal/core/ahoy/remote_userinfo_encoded_test.go new file mode 100644 index 000000000..46b0cc31e --- /dev/null +++ b/internal/core/ahoy/remote_userinfo_encoded_test.go @@ -0,0 +1,32 @@ +package ahoy + +import "testing" + +// TestScrubRemoteUserinfoDecodesBeforeTheColonTest pins iss-2609020630232658. +// git percent-decodes a URL's userinfo, so the colon separating a login from a +// password need not appear literally: ssh://user%3Apw@host carries a password +// exactly as ssh://user:pw@host does. A literal-colon test read it as a bare +// login — a route under ssh, and kept — so the encoded password went to rest in +// the history store, and the at-rest detector (defined as this function +// disagreeing with its input) never fired either. One round of decoding is +// what git applies, so a double-encoded %253A is literal text, not a +// separator; an undecodable userinfo is treated as a credential (fail closed). +func TestScrubRemoteUserinfoDecodesBeforeTheColonTest(t *testing.T) { + for _, tc := range []struct{ in, want string }{ + {"ssh://user%3Apw@example.com/owner/repo.git", "ssh://example.com/owner/repo.git"}, + {"ssh://user%3apw@example.com/owner/repo.git", "ssh://example.com/owner/repo.git"}, + {"git+ssh://user%3Apw@example.com/owner/repo.git", "git+ssh://example.com/owner/repo.git"}, + {"user%3Apw@example.com:owner/repo.git", "example.com:owner/repo.git"}, + {"ssh://user%zzpw@example.com/owner/repo.git", "ssh://example.com/owner/repo.git"}, + // Bare logins stay, encoded or not, and a double encoding is not a colon. + {"ssh://git@example.com/owner/repo.git", "ssh://git@example.com/owner/repo.git"}, + {"ssh://first%20last@example.com/owner/repo.git", "ssh://first%20last@example.com/owner/repo.git"}, + {"ssh://user%253Apw@example.com/owner/repo.git", "ssh://user%253Apw@example.com/owner/repo.git"}, + {"git@example.com:owner/repo.git", "git@example.com:owner/repo.git"}, + {"us%65r@example.com:owner/repo.git", "us%65r@example.com:owner/repo.git"}, + } { + if got := scrubRemoteUserinfo(tc.in); got != tc.want { + t.Errorf("scrubRemoteUserinfo(%q) = %q, want %q", tc.in, got, tc.want) + } + } +} diff --git a/internal/core/memory/ingest.go b/internal/core/memory/ingest.go index 8096cc992..a0d1e0f65 100644 --- a/internal/core/memory/ingest.go +++ b/internal/core/memory/ingest.go @@ -642,6 +642,18 @@ func redactedSource(source string) string { return maskUserinfo(source) } dropCredentialQuery(u) + // url.Parse decodes the userinfo and splits it at the first LITERAL colon, + // so `user%3Apw@host` parses as a username "user:pw" with no password, and + // Redacted — which masks only a parsed password — would echo it whole + // (iss-2609020630232658). Split the decoded username the way the transport + // will read it. + if u.User != nil { + if _, has := u.User.Password(); !has { + if login, _, found := strings.Cut(u.User.Username(), ":"); found { + u.User = url.UserPassword(login, "xxxxx") + } + } + } return u.Redacted() } @@ -663,11 +675,18 @@ func maskUserinfo(source string) string { if slash := strings.IndexByte(rest, '/'); slash >= 0 && slash < at { return source } - user := rest[:at] - if colon := strings.IndexByte(user, ':'); colon >= 0 { - user = user[:colon] + // The login ends at the first colon of the DECODED userinfo, since the + // transport decodes before it splits: `user%3Apw` is the login "user" and + // the password "pw" (iss-2609020630232658). A userinfo that does not + // decode cannot be split safely, so none of it is kept. + user, err := url.PathUnescape(rest[:at]) + if err != nil { + user = "" + } + if login, _, found := strings.Cut(user, ":"); found { + user = login } - return source[:i+3] + user + ":xxxxx@" + rest[at+1:] + return source[:i+3] + url.PathEscape(user) + ":xxxxx@" + rest[at+1:] } // transportCause renders the CAUSE of a failed fetch without the transport's diff --git a/internal/core/memory/userinfo_encoded_test.go b/internal/core/memory/userinfo_encoded_test.go new file mode 100644 index 000000000..061523bfd --- /dev/null +++ b/internal/core/memory/userinfo_encoded_test.go @@ -0,0 +1,41 @@ +package memory + +import ( + "strings" + "testing" +) + +// TestRefusalRenderersMaskAPercentEncodedPassword is the memory half of the +// sweep for iss-2609020630232658. A URL's userinfo is percent-decoded before +// use, so https://user%3Apw@host carries the password "pw" behind an encoded +// colon. url.Parse reads that as a USERNAME containing a colon and no password, +// so url.URL.Redacted (which masks only a parsed password) echoed the encoded +// password into a refusal, and the textual fallback kept everything before a +// literal colon. Both must decode before they decide where the login ends. +func TestRefusalRenderersMaskAPercentEncodedPassword(t *testing.T) { + const secret = "pwSECRET" + for _, in := range []string{ + "https://user%3A" + secret + "@example.com/a", + "https://user%3a" + secret + "@example.com/a", + } { + got := redactedSource(in) + if strings.Contains(got, secret) { + t.Errorf("redactedSource(%q) = %q: the encoded password survived", in, got) + } + if !strings.Contains(got, "user") { + t.Errorf("redactedSource(%q) = %q: the login name the operator recognises was lost", in, got) + } + } + // The textual fallback, reached when url.Parse refuses the string. + for _, in := range []string{ + "https://user%3A" + secret + "@exa mple.com/%zz", + "https://user%zz" + secret + "@example.com/a", + } { + if got := maskUserinfo(in); strings.Contains(got, secret) { + t.Errorf("maskUserinfo(%q) = %q: the encoded password survived", in, got) + } + } + if got := maskUserinfo("https://user:" + secret + "@example.com/a"); got != "https://user:xxxxx@example.com/a" { + t.Errorf("maskUserinfo literal-colon form = %q", got) + } +} From 9b96e61a8a44e0228d169e7c161225cf4ba7a89e Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Fri, 25 Sep 2026 14:39:08 +0100 Subject: [PATCH 03/36] =?UTF-8?q?chore:=20resolve=20iss-2609090828371674?= =?UTF-8?q?=20=E2=80=94=20per-agent=20staging=20locks?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves: iss-2609090828371674 Assisted-by: Claude:claude-opus-5-5 --- ...d-file-lock-s-backoff-cap-limits-staging-to-roughly.md | 8 ++++++++ 1 file changed, 8 insertions(+) rename .abcd/work/issues/{open => resolved}/iss-2609090828371674-the-shared-file-lock-s-backoff-cap-limits-staging-to-roughly.md (81%) diff --git a/.abcd/work/issues/open/iss-2609090828371674-the-shared-file-lock-s-backoff-cap-limits-staging-to-roughly.md b/.abcd/work/issues/resolved/iss-2609090828371674-the-shared-file-lock-s-backoff-cap-limits-staging-to-roughly.md similarity index 81% rename from .abcd/work/issues/open/iss-2609090828371674-the-shared-file-lock-s-backoff-cap-limits-staging-to-roughly.md rename to .abcd/work/issues/resolved/iss-2609090828371674-the-shared-file-lock-s-backoff-cap-limits-staging-to-roughly.md index 9892c0724..8e2e9121e 100644 --- a/.abcd/work/issues/open/iss-2609090828371674-the-shared-file-lock-s-backoff-cap-limits-staging-to-roughly.md +++ b/.abcd/work/issues/resolved/iss-2609090828371674-the-shared-file-lock-s-backoff-cap-limits-staging-to-roughly.md @@ -11,6 +11,14 @@ production_mode: hand-written found_at: "internal/fsutil" deferred_after: "v0.9.0" deferral_reason: "Ruled by the product thinker at the 2026-09-23 run A interview (M22: sharded per-agent locks, each agent staging behind its own lock; not a timeout or backoff tune and not a lock-free append; a build lane owed, not holding the tag). Earlier deferral: The record states its own position plainly: the remedies are design-shaped and should be chosen rather than assumed. Raising the timeout, lowering the backoff ceiling, sharding the lock per agent, or moving to a lock-free append reconciled at drain are four different bargains between latency, contention and complexity, and the measurement that motivates them is a ceiling rather than a fault. Choosing among them is the maintainer's call and no reading of the evidence makes one of them obviously right." +resolution: "Each staged key (agent id, or session id for a main thread) stages behind its own lock under staging/locks/, per the M22 ruling; fsutil.WithFileLock revalidates the locked inode so a lock is retired with its staged file." +impact: fix +resolved_by: + commit: "fcc3df07" --- The shared file lock's backoff cap limits staging to roughly ten writers a second, so a burst of simultaneous sub-agent completions loses transcripts. The lock helper backs off exponentially to a hundred-millisecond ceiling, so the rate a contended lock admits is set by that ceiling and not by how short the critical section is. Measured here, sixteen simultaneous stages take 2.12 seconds, consistently across three runs. Extrapolating the same rate, a burst past roughly forty to fifty simultaneous completions begins exceeding the five-second staging lock timeout, and a stage that times out is refused: the transcript it carried is not written anywhere, which is the loss the capture work exists to prevent. Sessions that fan out widely are exactly the sessions whose delegated reasoning is most worth keeping, so the ceiling bites hardest where the value is highest. The condition is pre-existing in the locking helper rather than introduced by sub-agent capture, but nothing reached the lock concurrently before, so it was unreachable in practice until now. The remedies are design-shaped and should be chosen rather than assumed: raise the timeout, lower the backoff ceiling, sharded locks keyed per agent, or a lock-free append with reconciliation at drain. + +## Grounds + +- pursued: a burst of 64 simultaneous distinct-agent stages all succeed and a held agent lock blocks only that agent; a stage refused with lock contention during a fan-out, or an empty lock file per sub-agent accumulating in staging/locks, would show it wrong From b08621fae0339ec29370b38a1a7ba36690e8c1bf Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Fri, 25 Sep 2026 14:39:10 +0100 Subject: [PATCH 04/36] =?UTF-8?q?chore:=20resolve=20iss-2609020630232658?= =?UTF-8?q?=20=E2=80=94=20decoded=20userinfo=20colon=20test?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves: iss-2609020630232658 Assisted-by: Claude:claude-opus-5-5 --- ...teuserinfo-and-the-history-credential-at-rest-detec.md | 8 ++++++++ 1 file changed, 8 insertions(+) rename .abcd/work/issues/{open => resolved}/iss-2609020630232658-scrubremoteuserinfo-and-the-history-credential-at-rest-detec.md (68%) diff --git a/.abcd/work/issues/open/iss-2609020630232658-scrubremoteuserinfo-and-the-history-credential-at-rest-detec.md b/.abcd/work/issues/resolved/iss-2609020630232658-scrubremoteuserinfo-and-the-history-credential-at-rest-detec.md similarity index 68% rename from .abcd/work/issues/open/iss-2609020630232658-scrubremoteuserinfo-and-the-history-credential-at-rest-detec.md rename to .abcd/work/issues/resolved/iss-2609020630232658-scrubremoteuserinfo-and-the-history-credential-at-rest-detec.md index 8b1de3df5..16c251c90 100644 --- a/.abcd/work/issues/open/iss-2609020630232658-scrubremoteuserinfo-and-the-history-credential-at-rest-detec.md +++ b/.abcd/work/issues/resolved/iss-2609020630232658-scrubremoteuserinfo-and-the-history-credential-at-rest-detec.md @@ -9,6 +9,14 @@ found_during: "autonomous-run-2026-09-01" origin: researcher-authored production_mode: hand-written found_at: "internal/core/ahoy/remote_userinfo.go" +resolution: "Every userinfo colon test runs on the decoded userinfo: ahoy's scrub (and so the at-rest detector and its heal) and memory ingest's two refusal renderers; undecodable userinfo fails closed." +impact: fix +resolved_by: + commit: "4c34bb9d" --- scrubRemoteUserinfo and the history.credential_at_rest detector decide that a userinfo carries a password by a literal colon, but git percent-decodes userinfo, so a remote such as ssh://user%3Apw@host/owner/repo.git under a non-http scheme is neither scrubbed at the derivation site nor detected at rest: the encoded password is stored verbatim in index.json and meta.json and the heal never fires. Reachability is thin (no credential helper is known to write this form) so this is a coverage hole in the new detector rather than a demonstrated leak; the fix is to percent-decode the userinfo before the colon test, for every scheme. + +## Grounds + +- pursued: ssh://user%3Apw@host is scrubbed and detected at rest while a bare or double-encoded login is kept; an encoded password surviving any of the three sites would show it wrong From 6bd2e84186c630f7b71a57217133dbb9fc3a1441 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Fri, 25 Sep 2026 14:41:00 +0100 Subject: [PATCH 05/36] perf(history): let reconstruct's mode reach the thread loader The loader read and kept every record body of a session before the renderer consulted the mode, so spine mode, whose purpose is to reduce each delegate to its opening and closing turns, still held every delegate in full. Each body is now parsed as it is read and let go, and in spine mode a delegate that hosts no other agent of the session releases the decoded content of every turn outside the spine window as it parses. Telemetry is still counted over every line. The main thread and any delegate named as another record's parent keep every turn, because placement reads a host's turns. Refs: iss-2609091155497399 Assisted-by: Claude:claude-opus-5-5 --- internal/core/history/reconstruct.go | 62 ++++++++++++++---- .../core/history/reconstruct_loader_test.go | 65 +++++++++++++++++++ 2 files changed, 113 insertions(+), 14 deletions(-) create mode 100644 internal/core/history/reconstruct_loader_test.go diff --git a/internal/core/history/reconstruct.go b/internal/core/history/reconstruct.go index 785a72d57..c772282c1 100644 --- a/internal/core/history/reconstruct.go +++ b/internal/core/history/reconstruct.go @@ -322,7 +322,7 @@ func Reconstruct(repoRoot, rootSHA string, opts ReconstructOptions) (Reconstruct return Reconstruction{}, fmt.Errorf("history: no records for session %q under %s", opts.SessionID, rootSHA) } - threads, dropped := loadThreads(records) + threads, dropped := loadThreads(records, opts.Mode) s := &session{ rootSHA: rootSHA, @@ -349,14 +349,31 @@ func Reconstruct(repoRoot, rootSHA string, opts ReconstructOptions) (Reconstruct // Loading and choosing records // --------------------------------------------------------------------------- -// loadThreads reads every record's body and picks ONE per (session, agent). +// loadThreads reads every record's body, parses it, and picks ONE thread per +// (session, agent). // // The pick is longest body, then newest capture, then filename. Longest first // because that is the store's own notion of more complete — supersession // replaces a stored record when the new bytes strictly extend it — so // preferring the newest alone would let a truncated re-capture displace a whole // transcript. Everything not picked is reported, never dropped silently. -func loadThreads(records []Record) ([]*thread, []DroppedRecord) { +// +// The mode reaches the loader (iss-2609091155497399). Each body is parsed as it +// is read and then let go, so at most one record body is resident at a time, +// never the whole session's. In spine mode a delegate that hosts no other +// agent of this session keeps decoded content for its head and tail turns +// only — the ones the renderer shows — so a session of verbose delegates is +// not held in memory in full just to be elided. Its telemetry is still counted +// over every line. The main thread is never reduced, and neither is a delegate +// named as another record's parent, because placement reads a host's every +// turn to find where it spawned and joined its own agents. +func loadThreads(records []Record, mode ReconstructMode) ([]*thread, []DroppedRecord) { + hosts := map[string]bool{} + for _, r := range records { + if r.ParentAgentID != "" { + hosts[r.ParentAgentID] = true + } + } byAgent := map[string][]*thread{} for _, r := range records { data, err := fsutil.ReadGuarded(r.Path, maxTranscriptBytes) @@ -379,11 +396,13 @@ func loadThreads(records []Record) ([]*thread, []DroppedRecord) { continue } rec.Path = r.Path - byAgent[r.AgentID] = append(byAgent[r.AgentID], &thread{ + th := &thread{ record: rec, recordName: filepath.Base(r.Path), - body: body, - }) + bodyLen: len(body), + } + th.parse(body, mode == ModeSpine && rec.AgentID != "" && !hosts[rec.AgentID]) + byAgent[r.AgentID] = append(byAgent[r.AgentID], th) } var out []*thread @@ -394,8 +413,8 @@ func loadThreads(records []Record) ([]*thread, []DroppedRecord) { if a.unreadable != b.unreadable { return a.unreadable == "" // readable first } - if len(a.body) != len(b.body) { - return len(a.body) > len(b.body) + if a.bodyLen != b.bodyLen { + return a.bodyLen > b.bodyLen } if !a.record.CapturedAt.Equal(b.record.CapturedAt) { return a.record.CapturedAt.After(b.record.CapturedAt) @@ -409,7 +428,7 @@ func loadThreads(records []Record) ([]*thread, []DroppedRecord) { reason = "unreadable: " + c.unreadable } dropped = append(dropped, DroppedRecord{ - Record: c.recordName, AgentID: agentID, Bytes: len(c.body), Reason: reason, + Record: c.recordName, AgentID: agentID, Bytes: c.bodyLen, Reason: reason, }) } if candidates[0].unreadable != "" { @@ -427,7 +446,7 @@ func loadThreads(records []Record) ([]*thread, []DroppedRecord) { type thread struct { record Record recordName string - body string + bodyLen int // bytes of the record body; the body itself is not kept unreadable string turns []turn @@ -518,13 +537,20 @@ type turn struct { } // parse fills the thread from its stored body. -func (t *thread) parse() { +// +// spine reduces the thread as it is parsed: once a turn falls outside the +// window the spine renderer shows (the first spineHeadTurns and the last +// spineTailTurns), its decoded blocks and source lines are released, leaving +// the turn's index and role for the omission count. Every line is still +// decoded for the telemetry — tokens, tool calls, models, span — which is +// counted over the whole transcript in both modes. +func (t *thread) parse(body string, spine bool) { t.toolCalls = map[string]int{} seenUsage := map[string]bool{} seenToolUse := map[string]bool{} seenModel := map[string]bool{} - for _, line := range strings.Split(t.body, "\n") { + for _, line := range strings.Split(body, "\n") { line = strings.TrimSpace(line) if line == "" { continue @@ -558,6 +584,14 @@ func (t *thread) parse() { cur.blocks = append(cur.blocks, blocks...) cur.raw = append(cur.raw, line) } else { + if spine { + // The turn about to leave the tail window, unless it is a head + // turn. Only the last turn is ever extended by a continuation + // line, and it is never the one released. + if k := len(t.turns) - spineTailTurns; k >= spineHeadTurns { + t.turns[k].blocks, t.turns[k].raw = nil, nil + } + } t.turns = append(t.turns, turn{ index: len(t.turns) + 1, role: rl.Type, @@ -714,14 +748,14 @@ type session struct { telemetry Telemetry } -// order parses every thread and sorts the sub-agents into a stable reading +// order separates the main thread from the sub-agents (the loader has already +// parsed every thread) and sorts the sub-agents into a stable reading // order: by depth, then by start time, then by agent id. Start time rather than // spawn point, because the spawn point is not always recoverable and a section // order that changes with attribution quality would make two runs over the same // store disagree. func (s *session) order() { for _, t := range s.threads { - t.parse() if t.isMain() { s.main = t continue diff --git a/internal/core/history/reconstruct_loader_test.go b/internal/core/history/reconstruct_loader_test.go new file mode 100644 index 000000000..689d1fa18 --- /dev/null +++ b/internal/core/history/reconstruct_loader_test.go @@ -0,0 +1,65 @@ +package history + +import "testing" + +// middleTurnsRetained reports how many of a thread's turns outside the spine +// window (the first spineHeadTurns and last spineTailTurns) still hold decoded +// blocks or source lines. +func middleTurnsRetained(th *thread) int { + n := 0 + for i, tn := range th.turns { + if i < spineHeadTurns || i >= len(th.turns)-spineTailTurns { + continue + } + if len(tn.blocks) > 0 || len(tn.raw) > 0 { + n++ + } + } + return n +} + +// TestSpineModeReachesTheLoader pins iss-2609091155497399: the mode is a +// loader input, not only a renderer one. In spine mode a delegate that hosts +// no other agent is reduced to its head and tail turns as it is parsed, so a +// session of verbose delegates is never resident in full just to be elided; +// the main thread, and a delegate whose turns place a nested agent, stay +// whole, because placement reads them. No thread keeps its record body once it +// has been parsed, in either mode. +func TestSpineModeReachesTheLoader(t *testing.T) { + _, home := setupStore(t) + plantRecord(t, home, "20260901T100000.000000000Z-sess-nest.md", []string{ + "session_id: sess-nest", "captured_at: 2026-09-01T10:06:00Z", + }, mainThreadBody()) + plantRecord(t, home, "20260901T100500.000000000Z-sess-nest-agent-agenthost.md", []string{ + "session_id: sess-nest", "captured_at: 2026-09-01T10:05:00Z", + "agent_id: agenthost", "spawn_depth: 1", "lineage_source: hook", "spawn_attribution: sidecar", + }, subAgentBody()) + plantRecord(t, home, "20260901T100600.000000000Z-sess-nest-agent-agentleaf.md", []string{ + "session_id: sess-nest", "captured_at: 2026-09-01T10:06:00Z", + "agent_id: agentleaf", "parent_agent_id: agenthost", "spawn_depth: 2", + "lineage_source: hook", "spawn_attribution: sidecar", + }, subAgentBody()) + records, err := ListForSession("", testRootSHA, "sess-nest") + if err != nil { + t.Fatal(err) + } + for _, mode := range []ReconstructMode{ModeFull, ModeSpine} { + threads, dropped := loadThreads(records, mode) + if len(dropped) != 0 || len(threads) != 3 { + t.Fatalf("%s: %d threads, %d dropped; fixture drift", mode, len(threads), len(dropped)) + } + for _, th := range threads { + if th.bodyLen == 0 || len(th.turns) != th.turnCount.Total { + t.Fatalf("%s %s: parsed %d turns of %d, body %d bytes", mode, th.label(), len(th.turns), th.turnCount.Total, th.bodyLen) + } + retained := middleTurnsRetained(th) + reduce := mode == ModeSpine && th.label() == "agentleaf" + switch { + case reduce && retained != 0: + t.Errorf("spine: leaf delegate %s kept %d middle turn(s) decoded", th.label(), retained) + case !reduce && retained == 0: + t.Errorf("%s: %s lost its middle turns, which placement or the render reads", mode, th.label()) + } + } + } +} From 2351cc96b738a59acdd0bdb3935426b7b22d37b5 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Fri, 25 Sep 2026 14:41:06 +0100 Subject: [PATCH 06/36] =?UTF-8?q?chore:=20resolve=20iss-2609091155497399?= =?UTF-8?q?=20=E2=80=94=20spine=20mode=20reaches=20the=20loader?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves: iss-2609091155497399 Assisted-by: Claude:claude-opus-5-5 --- ...uct-loads-every-record-s-full-body-even-in-the-mode.md | 8 ++++++++ 1 file changed, 8 insertions(+) rename .abcd/work/issues/{open => resolved}/iss-2609091155497399-reconstruct-loads-every-record-s-full-body-even-in-the-mode.md (74%) diff --git a/.abcd/work/issues/open/iss-2609091155497399-reconstruct-loads-every-record-s-full-body-even-in-the-mode.md b/.abcd/work/issues/resolved/iss-2609091155497399-reconstruct-loads-every-record-s-full-body-even-in-the-mode.md similarity index 74% rename from .abcd/work/issues/open/iss-2609091155497399-reconstruct-loads-every-record-s-full-body-even-in-the-mode.md rename to .abcd/work/issues/resolved/iss-2609091155497399-reconstruct-loads-every-record-s-full-body-even-in-the-mode.md index f826713b1..41845a652 100644 --- a/.abcd/work/issues/open/iss-2609091155497399-reconstruct-loads-every-record-s-full-body-even-in-the-mode.md +++ b/.abcd/work/issues/resolved/iss-2609091155497399-reconstruct-loads-every-record-s-full-body-even-in-the-mode.md @@ -9,6 +9,14 @@ found_during: "sub-agent transcript capture branch review" origin: researcher-authored production_mode: hand-written found_at: "internal/core/history/reconstruct.go" +resolution: "The mode is a loader input: bodies are parsed as read and released, and in spine mode a non-host delegate keeps decoded content for its head and tail turns only; main thread and host delegates stay whole." +impact: internal +resolved_by: + commit: "6bd2e841" --- Reconstruct loads every record's full body even in the mode whose whole purpose is to not render them. The thread loader reads each record body into memory for every thread in a session before the renderer consults the mode, so the spine mode, which reduces each delegate to its opening instruction and closing turn, still pays the full memory cost of every delegate it is about to discard. It is bounded per file by the record read cap rather than unbounded, and the largest main thread observed is well under that cap, so this is a ceiling rather than a leak: a session with a few dozen verbose delegates near the cap could still hold most of a gigabyte resident before the first byte is elided. The ingest path in the same package takes the opposite approach deliberately, processing one transcript at a time and discarding the bytes after probing, because the corpus it walks is far larger than memory. The fix is to let the mode reach the loader, so a spine run decodes only the head and tail turns of a thread it will summarise. Not urgent while sessions stay at the observed fan-out, and worth doing before a session with wide delegation is reconstructed on a small machine. + +## Grounds + +- pursued: a spine run holds at most one record body at a time and no leaf delegate's middle turns, with artefact and telemetry unchanged; a leaf delegate's middle turns retained after loading, or a nested agent losing its placement, would show it wrong From 57acad25f2acfd040d93ca79498aa675547fdbb4 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Fri, 25 Sep 2026 14:42:14 +0100 Subject: [PATCH 07/36] refactor(history): give session ownership one definition The rule for which store owns a session had two homes: SessionOwner, exported with no caller, and an inline copy in ingest's session placement that was the one that actually ran, so a fix to the helper alone would have appeared to work and changed nothing. The rule is now ownerIndex.owner, the placement pass calls it over its once-per-run index, and the uncalled export is gone. Refs: iss-2609091911066372 Assisted-by: Claude:claude-opus-5-5 --- internal/core/history/ingest.go | 28 +++++++----- internal/core/history/session_owner_test.go | 47 +++++++++++++++++++++ 2 files changed, 65 insertions(+), 10 deletions(-) create mode 100644 internal/core/history/session_owner_test.go diff --git a/internal/core/history/ingest.go b/internal/core/history/ingest.go index 5694b4b37..40973f936 100644 --- a/internal/core/history/ingest.go +++ b/internal/core/history/ingest.go @@ -275,15 +275,18 @@ func placeSessions(probes []transcriptProbe) map[string]sessionPlacement { // needs it. Asking the store per session would re-read every record in // every store for every session that fell through, which on a populated // machine is the difference between a verb and a coffee break. - var index map[string][]string + var index ownerIndex storeOwner := func(sessionID string) string { if index == nil { index = storeSessionIndex() } - if shas := index[sessionID]; len(shas) == 1 { - return shas[0] + // A refusal (no store, or several) is no placement; the session falls + // through to its files' own directories. + sha, err := index.owner(sessionID) + if err != nil { + return "" } - return "" + return sha } mainCwds := map[string][]string{} for _, p := range probes { @@ -580,8 +583,13 @@ func probeTranscript(c candidate) (transcriptProbe, error) { return p, nil } -// SessionOwner returns the root-commit SHA of the store that already holds a -// session — through a session note, or through a stored record naming it. +// ownerIndex maps a session id to the root-commit SHAs of every store lane +// that already knows it (storeSessionIndex builds it once per run). +type ownerIndex map[string][]string + +// owner is THE session-ownership rule, and it has this one definition +// (iss-2609091911066372): the root-commit SHA of the store that already holds +// a session — through a session note, or through a stored record naming it. // // It is the placement this machine made earlier, recovered rather than // recomputed, and it is what lets a session whose directories are all gone @@ -589,11 +597,11 @@ func probeTranscript(c candidate) (transcriptProbe, error) { // guessed, for the same reason SessionRepo refuses one: a transcript filed // against the wrong repository is redacted by the wrong repository's scanner // configuration. -func SessionOwner(sessionID string) (string, error) { +func (idx ownerIndex) owner(sessionID string) (string, error) { if !safeIDSegment(sessionID) { return "", fmt.Errorf("history: sessionID must be non-empty, match [A-Za-z0-9._-]+ and not be a directory reference") } - switch found := storeSessionIndex()[sessionID]; len(found) { + switch found := idx[sessionID]; len(found) { case 1: return found[0], nil case 0: @@ -609,8 +617,8 @@ func SessionOwner(sessionID string) (string, error) { // // A store that cannot be listed is skipped rather than fatal: one unreadable // store is not a reason to refuse a placement every other store can make. -func storeSessionIndex() map[string][]string { - index := map[string][]string{} +func storeSessionIndex() ownerIndex { + index := ownerIndex{} root, err := userStoreBase() if err != nil { return index diff --git a/internal/core/history/session_owner_test.go b/internal/core/history/session_owner_test.go new file mode 100644 index 000000000..d1350ab7d --- /dev/null +++ b/internal/core/history/session_owner_test.go @@ -0,0 +1,47 @@ +package history + +import ( + "path/filepath" + "testing" +) + +// TestSessionOwnershipHasOneDefinition pins iss-2609091911066372. The rule +// "the one store that already claims a session owns it; none or several is no +// answer" had two homes: an exported helper nothing called, and an inline copy +// in ingest's session placement that was the one that ran. It has one now, +// ownerIndex.owner, and the placement pass reaches it: a session two stores +// claim is refused by the rule and left unplaced by ingest alike. +func TestSessionOwnershipHasOneDefinition(t *testing.T) { + const a, b = testRootSHA, "b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0" + idx := ownerIndex{"one": {a}, "two": {a, b}} + if sha, err := idx.owner("one"); err != nil || sha != a { + t.Errorf("owner(one) = %q, %v; want %s", sha, err, a) + } + for _, id := range []string{"two", "none", "../x"} { + if sha, err := idx.owner(id); err == nil { + t.Errorf("owner(%s) = %q with no error; want a refusal", id, sha) + } + } + + // The placement pass reads the same rule off the on-disk index. + repoRoot, _ := setupStore(t) + other := t.TempDir() + fakeRepos(t, map[string]string{repoRoot: a, other: b}) + for _, pair := range []struct{ root, sha, id string }{ + {repoRoot, a, "sess-one"}, {repoRoot, a, "sess-two"}, {other, b, "sess-two"}, + } { + if err := NoteSessionRepo(pair.root, pair.sha, pair.id); err != nil { + t.Fatal(err) + } + } + got := placeSessions([]transcriptProbe{ + {path: filepath.Join(other, "x.jsonl"), sessionID: "sess-one"}, + {path: filepath.Join(other, "y.jsonl"), sessionID: "sess-two"}, + }) + if p := got["sess-one"]; p.rootSHA != a || p.via != "store" { + t.Errorf("sess-one placed %+v, want the one claiming store", p) + } + if p := got["sess-two"]; p.rootSHA != "" { + t.Errorf("sess-two, claimed by two stores, placed %+v; want no placement", p) + } +} From 1edd3dfa30e600f1e81ad6e0cfa0b9af8c3d0b1a Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Fri, 25 Sep 2026 14:42:16 +0100 Subject: [PATCH 08/36] =?UTF-8?q?chore:=20resolve=20iss-2609091911066372?= =?UTF-8?q?=20=E2=80=94=20one=20session-ownership=20rule?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves: iss-2609091911066372 Assisted-by: Claude:claude-opus-5-5 --- ...ed-owner-resolution-helper-has-no-callers-and-its-l.md | 8 ++++++++ 1 file changed, 8 insertions(+) rename .abcd/work/issues/{open => resolved}/iss-2609091911066372-an-exported-owner-resolution-helper-has-no-callers-and-its-l.md (77%) diff --git a/.abcd/work/issues/open/iss-2609091911066372-an-exported-owner-resolution-helper-has-no-callers-and-its-l.md b/.abcd/work/issues/resolved/iss-2609091911066372-an-exported-owner-resolution-helper-has-no-callers-and-its-l.md similarity index 77% rename from .abcd/work/issues/open/iss-2609091911066372-an-exported-owner-resolution-helper-has-no-callers-and-its-l.md rename to .abcd/work/issues/resolved/iss-2609091911066372-an-exported-owner-resolution-helper-has-no-callers-and-its-l.md index 430f020bd..fb28fa10a 100644 --- a/.abcd/work/issues/open/iss-2609091911066372-an-exported-owner-resolution-helper-has-no-callers-and-its-l.md +++ b/.abcd/work/issues/resolved/iss-2609091911066372-an-exported-owner-resolution-helper-has-no-callers-and-its-l.md @@ -9,6 +9,14 @@ found_during: "fidelity audit of the recovery intent" origin: researcher-authored production_mode: hand-written found_at: "internal/core/history/ingest.go" +resolution: "Session ownership has one definition, ownerIndex.owner, called by ingest's placement pass; the uncalled exported SessionOwner is removed." +impact: internal +resolved_by: + commit: "57acad25" --- An exported owner-resolution helper has no callers and its logic is duplicated inline at the one place that needs it. The function resolves which store already claims a session and is exported from the history package, but nothing in the tree calls it: the ingest path reimplements the same walk inline instead. That is two copies of one rule, which is the shape this repository's one-canonical-primitive principle exists to prevent, and it is also dead scaffolding on a package boundary, which the wired-or-it-isn-t-done rule forbids. The duplication is the more expensive half: a later change to how a session's owner is resolved has two homes to find, and the inline copy is the one that actually runs, so a fix applied to the exported helper alone would appear to work and change nothing. Either make the inline site call the helper, or delete the helper and let the inline walk be the only definition. The spec already flags this as an uncertainty; it shipped unresolved. + +## Grounds + +- pursued: a change to the ownership rule now changes both the lookup and ingest placement; a second copy of the one-store rule reappearing in ingest.go would show it wrong From 8a61f1090b22c3803444a9d927350ed626541669 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Fri, 25 Sep 2026 14:46:49 +0100 Subject: [PATCH 09/36] feat(history): capture a whole session with capture --session --all A run capturing its own transcripts had to list the host's files by hand and invoke capture once per file (eight calls for one session and seven sub-agents in the pilot that reported it). `history capture --session --all [...]` is the write-side twin of `list --session`: an ingest scoped to one session, into this repository's store. Transcripts are found by the session their lines name under the paths given or the declared ingest_roots, never by a host's directory layout, and are placed exactly as ingest places them, so one another repository owns is reported rather than stored here. Decision taken (no record covers it): the flag requires --session; the record's "current session as the default when the harness exposes its id" is not built, because no harness interface abcd reads exposes the running session's id to a shell command. The core change is one IngestOptions.Session filter applied after probing. Surface: commands/history.md, the history brief chapter, the generated CLI reference and surface.json move with the flag. Refs: iss-2609202046145653 Assisted-by: Claude:claude-opus-5-5 --- .../brief/04-surfaces/11-history.md | 8 ++ .abcd/development/release/surface.json | 7 ++ commands/history.md | 19 ++++- docs/reference/cli/commands.md | 5 +- internal/core/history/ingest.go | 14 ++++ internal/surface/cli/history.go | 66 +++++++++++++++- .../surface/cli/history_capture_all_test.go | 76 +++++++++++++++++++ internal/surface/cli/history_recovery.go | 8 +- 8 files changed, 193 insertions(+), 10 deletions(-) create mode 100644 internal/surface/cli/history_capture_all_test.go diff --git a/.abcd/development/brief/04-surfaces/11-history.md b/.abcd/development/brief/04-surfaces/11-history.md index ede327c72..aa614a328 100644 --- a/.abcd/development/brief/04-surfaces/11-history.md +++ b/.abcd/development/brief/04-surfaces/11-history.md @@ -51,6 +51,13 @@ ahoy's registry stays under `~/.abcd/history/` and holds no transcripts. standard input, where there is no filename to read it from. The caller also says where the transcript came from, a session abcd captured itself or an import of a prior tool's transcripts, and it defaults to the first. + Asked for a whole session instead, capture stores the named session's main + thread and every sub-agent transcript it spawned in one call — the write-side + twin of listing a session — finding them under the paths given, or the + declared `ingest_roots`, by the session their lines name, and placing them + exactly as ingesting does, so a transcript another repository owns is + reported rather than stored here. It needs the session named; nothing infers + the running one. - **The staged listing** names transcripts that ended but are not yet redacted into the store. A non-empty list means unredacted transcript text is on disk. - **Draining** redacts and stores every staged transcript, then deletes the raw @@ -329,6 +336,7 @@ Sub-verbs: none. | Flag | Type | |---|---| +| `--all` | bool | | `--kind` | string | | `--session` | string | diff --git a/.abcd/development/release/surface.json b/.abcd/development/release/surface.json index 77ac43b01..742d6089d 100644 --- a/.abcd/development/release/surface.json +++ b/.abcd/development/release/surface.json @@ -782,6 +782,13 @@ "path": "abcd history capture", "hidden": false, "flags": [ + { + "name": "all", + "shorthand": "", + "type": "bool", + "required": false, + "hidden": false + }, { "name": "kind", "shorthand": "", diff --git a/commands/history.md b/commands/history.md index b8ae46945..7e313563e 100644 --- a/commands/history.md +++ b/commands/history.md @@ -1,7 +1,7 @@ --- name: history description: Manage the native session-transcript store for this repo by invoking the abcd binary. list, show and staged are read-only; capture, drain and ingest are the redacting write paths, migrate repairs records in place, reconstruct renders one session as an artefact plus telemetry, and discard permanently deletes one unredacted staged or quarantined transcript. list --session reaches one session's whole set — its main thread and every sub-agent it spawned. The store is user-level, keyed on the repo's root-commit SHA, and every stored transcript is redacted on write. -argument-hint: "list [--session ] | show | staged [--all-repos] | drain | discard --yes | capture | ingest [...] | migrate | reconstruct " +argument-hint: "list [--session ] | show | staged [--all-repos] | drain | discard --yes | capture | capture --session --all [...] | ingest [...] | migrate | reconstruct " --- # `/abcd:history` — session-transcript store @@ -188,6 +188,23 @@ session id defaults to the transcript filename; reading from stdin requires identical transcript already stored is a no-op. If any hard-fail secret or the caller's own home path survives redaction, capture refuses to write. +```bash +"${CLAUDE_PLUGIN_ROOT}/abcd" history capture --session --all [...] --json +``` + +`--all` captures a whole session in one call: the main thread and every +sub-agent transcript it spawned, into this repository's store. It is the +write-side twin of `list --session`, and it is how a run captures its own +transcripts without listing the host's files by hand. `--session` is required +with it. The transcripts are found by what their lines say, never by where a +host keeps them: the sources are the paths given, or the `ingest_roots` +declared in `.abcd/config/history.json`, walked exactly as `ingest` walks them, +and only the files whose lines name that one session are stored. Placement is +`ingest`'s too, so a transcript of the session that another repository owns is +reported as skipped, not stored here. The report has `ingest`'s four +populations — `captured`, `skipped`, `orphans` and `failed` — and a session +found nowhere under the paths says so. + ## Ingest ```bash diff --git a/docs/reference/cli/commands.md b/docs/reference/cli/commands.md index 5352c41f8..5341bbc90 100644 --- a/docs/reference/cli/commands.md +++ b/docs/reference/cli/commands.md @@ -600,13 +600,14 @@ Manage the native session-transcript store #### `abcd history capture` -Redact and store a raw session transcript (reads a file or stdin) +Redact and store a raw session transcript (reads a file or stdin), or a whole session with --all -**Usage:** `abcd history capture [|-] [flags]` +**Usage:** `abcd history capture [ | - | --session --all ...] [flags]` **Flags:** ``` + --all capture every transcript of the --session named — its main thread and each sub-agent — found under the paths given (default: ingest_roots) --kind string source kind: native | specstory-import (default native) --session string session id for the record (default: transcript filename; required for stdin) ``` diff --git a/internal/core/history/ingest.go b/internal/core/history/ingest.go index 40973f936..539fb0a59 100644 --- a/internal/core/history/ingest.go +++ b/internal/core/history/ingest.go @@ -140,6 +140,14 @@ type IngestOptions struct { Lineage LineageLookup // MaxDepth bounds a directory walk; zero means ingestDefaultDepth. MaxDepth int + // Session, when set, scopes the run to one session: only transcripts whose + // lines name exactly this session id — its main thread and every sub-agent + // it spawned — are placed and stored, and every other transcript found is + // out of scope and not reported. It is the write-side twin of a session + // listing (`history capture --session --all`, iss-2609202046145653). + // A file that could not be read is still reported as failed: it may have + // been one of the session's. + Session string } // Ingested is one transcript that entered the store. @@ -215,6 +223,9 @@ func Ingest(dest Destination, sources []string, opts IngestOptions) (IngestResul if len(sources) == 0 { return IngestResult{}, errors.New("history: ingest needs at least one source path; declare them in " + ConfigRelPath + " or name them on the command line") } + if opts.Session != "" && !sessionIDRe.MatchString(opts.Session) { + return IngestResult{}, errors.New("history: the session to ingest must match [A-Za-z0-9._-]+") + } // Resolving is what creates the destination store when it is absent, and // what migrates a corpus left at the legacy location into it (iss-95). It // is done here, before any source is read, so a destination that cannot be @@ -231,6 +242,9 @@ func Ingest(dest Destination, sources []string, opts IngestOptions) (IngestResul res.Failed = append(res.Failed, IngestFailure{Path: c.path, Err: err.Error()}) continue } + if opts.Session != "" && p.sessionID != opts.Session { + continue + } probes = append(probes, p) } diff --git a/internal/surface/cli/history.go b/internal/surface/cli/history.go index 89495a1b0..705375fc8 100644 --- a/internal/surface/cli/history.go +++ b/internal/surface/cli/history.go @@ -43,11 +43,20 @@ func newHistoryCommand(asJSON *bool) *cobra.Command { // (two-stage, fail-closed), and store the record. This is the ONLY path that // writes to the store; list/show never mutate. var session, kind string + var captureAll bool captureCmd := &cobra.Command{ - Use: "capture [|-]", - Short: "Redact and store a raw session transcript (reads a file or stdin)", - Args: cobra.MaximumNArgs(1), + Use: "capture [ | - | --session --all ...]", + Short: "Redact and store a raw session transcript (reads a file or stdin), or a whole session with --all", + Args: func(cmd *cobra.Command, args []string) error { + if captureAll { + return nil // --all takes any number of source paths + } + return cobra.MaximumNArgs(1)(cmd, args) + }, RunE: func(cmd *cobra.Command, args []string) error { + if captureAll { + return captureWholeSession(cmd, *asJSON, session, kind, args) + } repoRoot, rootSHA, err := historyStore(cmd) if err != nil { return err @@ -99,6 +108,8 @@ func newHistoryCommand(asJSON *bool) *cobra.Command { } captureCmd.Flags().StringVar(&session, "session", "", "session id for the record (default: transcript filename; required for stdin)") captureCmd.Flags().StringVar(&kind, "kind", "", "source kind: native | specstory-import (default native)") + captureCmd.Flags().BoolVar(&captureAll, "all", false, + "capture every transcript of the --session named — its main thread and each sub-agent — found under the paths given (default: ingest_roots)") historyCmd.AddCommand(captureCmd) // list — records newest-first for this repo, or one session's whole set. @@ -471,6 +482,55 @@ func newHistoryCommand(asJSON *bool) *cobra.Command { return historyCmd } +// captureWholeSession is `history capture --session --all`: the +// write-side twin of `list --session`, so a run captures its own session — the +// main thread and every sub-agent it spawned — in one call instead of listing +// the harness's files by hand (iss-2609202046145653). +// +// It is an ingest scoped to one session, into THIS repository's store. The +// transcripts are found by what their lines say, never by where a host keeps +// them: the sources are the paths given, or the ingest_roots this repository +// declares, exactly as for ingest. Placement is ingest's too, so a transcript of +// the session that some other repository owns is reported, not stored here. +func captureWholeSession(cmd *cobra.Command, asJSON bool, session, kind string, sources []string) error { + if session == "" { + return fmt.Errorf("history capture: --all captures one named session; pass --session ") + } + if kind != "" && kind != "native" { + return fmt.Errorf("history capture: --all stores native transcripts only; --kind %s cannot apply", termsafe.Sanitize(kind)) + } + repoRoot, rootSHA, err := historyStore(cmd) + if err != nil { + return err + } + cfg, err := history.LoadConfig(repoRoot) + if err != nil { + return err + } + if len(sources) == 0 { + sources = cfg.IngestRoots + } + if len(sources) == 0 { + return fmt.Errorf("history capture: --all needs the paths to look under; name them, or declare ingest_roots in %s", history.ConfigRelPath) + } + dest := history.Destination{RepoRoot: repoRoot, RootSHA: rootSHA} + res, err := history.Ingest(dest, sources, history.IngestOptions{ + Adopt: cfg.AdoptProjects, + Lineage: harnessLineage(sources), + Session: session, + }) + if err != nil { + return err + } + redactIngestPaths(&res) + return render(cmd.OutOrStdout(), asJSON, res, func(w io.Writer) { + renderHistoryIngest(w, "capture --all", dest, res) + if len(res.Captured) == 0 && len(res.Failed) == 0 && len(res.Skipped) == 0 && len(res.Orphans) == 0 { + fmt.Fprintf(w, "no transcript of session %s was found under the paths given\n", termsafe.Sanitize(session)) + } + }) +} + // historyRecords reads the whole repo's records, or one session's whole set when // a session was named. // diff --git a/internal/surface/cli/history_capture_all_test.go b/internal/surface/cli/history_capture_all_test.go new file mode 100644 index 000000000..efc0ac784 --- /dev/null +++ b/internal/surface/cli/history_capture_all_test.go @@ -0,0 +1,76 @@ +package cli + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/intentdriven/abcd/internal/core/history" +) + +// TestHistoryCaptureAllTakesTheWholeSession pins iss-2609202046145653: a run +// captures its own session in one call. `history capture --session --all` +// finds, under the sources given, every transcript whose lines name that +// session — the main thread and each sub-agent — and stores them in this +// repository's store; a different session's transcript beside them is left +// alone. +func TestHistoryCaptureAllTakesTheWholeSession(t *testing.T) { + repo, rootSHA := sessionEndRepo(t) + t.Chdir(repo) + src := t.TempDir() + write := func(rel, session, agent string) { + t.Helper() + line := `{"type":"user","sessionId":"` + session + `","cwd":"` + repo + `"` + if agent != "" { + line += `,"agentId":"` + agent + `"` + } + line += `,"message":{"role":"user","content":"hello"}}` + "\n" + p := filepath.Join(src, "proj", rel) + if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(p, []byte(line), 0o644); err != nil { + t.Fatal(err) + } + } + write("sess-mine.jsonl", "sess-mine", "") + write("sess-mine/subagents/agent-a1.jsonl", "sess-mine", "a1") + write("sess-mine/subagents/agent-a2.jsonl", "sess-mine", "a2") + write("sess-other.jsonl", "sess-other", "") + + out, errOut, err := runRecovery("", "history", "capture", "--session", "sess-mine", "--all", src) + if err != nil { + t.Fatalf("history capture --all: %v\n%s\n%s", err, out, errOut) + } + recs, err := history.List(repo, rootSHA) + if err != nil { + t.Fatal(err) + } + got := map[string]bool{} + for _, r := range recs { + if r.SessionID != "sess-mine" { + t.Errorf("a transcript of session %s was captured; --all names one session", r.SessionID) + } + got[r.AgentID] = true + } + for _, want := range []string{"", "a1", "a2"} { + if !got[want] { + t.Errorf("agent %q of the named session was not captured; records: %+v", want, recs) + } + } +} + +// TestHistoryCaptureAllNeedsASession: --all is the whole of ONE named session, +// so without --session it is refused rather than read as every session. +func TestHistoryCaptureAllNeedsASession(t *testing.T) { + repo, _ := sessionEndRepo(t) + t.Chdir(repo) + _, errOut, err := runRecovery("", "history", "capture", "--all", t.TempDir()) + if err == nil { + t.Fatal("history capture --all with no --session succeeded") + } + if !strings.Contains(err.Error()+errOut, "--session") { + t.Errorf("the refusal must name --session, got %v / %s", err, errOut) + } +} diff --git a/internal/surface/cli/history_recovery.go b/internal/surface/cli/history_recovery.go index 4db86c0fe..d671026b5 100644 --- a/internal/surface/cli/history_recovery.go +++ b/internal/surface/cli/history_recovery.go @@ -189,7 +189,7 @@ func newHistoryIngestCommand(asJSON *bool) *cobra.Command { } redactIngestPaths(&res) return render(cmd.OutOrStdout(), *asJSON, res, func(w io.Writer) { - renderHistoryIngest(w, dest, res) + renderHistoryIngest(w, "ingest", dest, res) }) }, } @@ -263,15 +263,15 @@ func askAdoptions(cmd *cobra.Command, orphans []history.Orphan) []string { // renderHistoryIngest writes the human report. The destination leads it, because the // destination is the fact an operator most needs to be sure of. -func renderHistoryIngest(w io.Writer, dest history.Destination, res history.IngestResult) { +func renderHistoryIngest(w io.Writer, verb string, dest history.Destination, res history.IngestResult) { var wrote int for _, c := range res.Captured { if c.Wrote { wrote++ } } - fmt.Fprintf(w, "abcd history ingest — into %s (root %s)\n", - termsafe.Sanitize(fsutil.RedactHome(dest.RepoRoot)), dest.RootSHA) + fmt.Fprintf(w, "abcd history %s — into %s (root %s)\n", + verb, termsafe.Sanitize(fsutil.RedactHome(dest.RepoRoot)), dest.RootSHA) fmt.Fprintf(w, " stored %d of %d owned transcript(s); %d skipped, %d orphaned, %d failed\n", wrote, len(res.Captured), len(res.Skipped), len(res.Orphans), len(res.Failed)) for _, c := range res.Captured { From 93835820be7537edf6c40adab65cc54f198f91ef Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Fri, 25 Sep 2026 14:46:50 +0100 Subject: [PATCH 10/36] =?UTF-8?q?chore:=20resolve=20iss-2609202046145653?= =?UTF-8?q?=20=E2=80=94=20capture=20--session=20--all?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves: iss-2609202046145653 Assisted-by: Claude:claude-opus-5-5 --- ...apture-has-no-current-session-mode-so-a-run-capture.md | 8 ++++++++ 1 file changed, 8 insertions(+) rename .abcd/work/issues/{open => resolved}/iss-2609202046145653-history-capture-has-no-current-session-mode-so-a-run-capture.md (70%) diff --git a/.abcd/work/issues/open/iss-2609202046145653-history-capture-has-no-current-session-mode-so-a-run-capture.md b/.abcd/work/issues/resolved/iss-2609202046145653-history-capture-has-no-current-session-mode-so-a-run-capture.md similarity index 70% rename from .abcd/work/issues/open/iss-2609202046145653-history-capture-has-no-current-session-mode-so-a-run-capture.md rename to .abcd/work/issues/resolved/iss-2609202046145653-history-capture-has-no-current-session-mode-so-a-run-capture.md index 22139cfc4..f09e0e012 100644 --- a/.abcd/work/issues/open/iss-2609202046145653-history-capture-has-no-current-session-mode-so-a-run-capture.md +++ b/.abcd/work/issues/resolved/iss-2609202046145653-history-capture-has-no-current-session-mode-so-a-run-capture.md @@ -9,6 +9,14 @@ found_during: "Dessau pilot run, session gropiusllm-64, relayed to abcd-17 on 20 origin: researcher-authored production_mode: hand-written found_at: "internal/surface/cli/cli.go" +resolution: "history capture --session --all [...] stores the named session's main thread and every sub-agent transcript found under the paths or ingest_roots, placed as ingest places them. The current-session default is not built: no harness interface abcd reads exposes the running session id." +impact: additive +resolved_by: + commit: "8a61f109" --- history capture has no current-session mode, so a run captures its own transcripts by listing files by hand. At v0.9.0 the verb takes one transcript path, and bare abcd history capture with no file answers "--session is required when reading from stdin"; nothing discovers the session that is running or the sub-agent transcripts it spawned. The Dessau pilot (session gropiusllm-64, 2026-09-20) captured its session and seven sub-agent transcripts with eight invocations after listing ~/.claude/projects///subagents/agent-*.jsonl by hand, which is exactly the path knowledge the store already has (list --session reaches a session and every sub-agent it spawned on the read side). Wanted: a write-side twin of that read, history capture --session --all (the main thread and every sub-agent), with the current session as the default when the harness exposes its id, so a loop captures its own run in one call. Evidence for the implement verb (itd-2609201916151817), which would call it at the end of every lane. + +## Grounds + +- pursued: one call captures a session's main thread and all its sub-agents and nothing of another session; a sub-agent transcript of the named session left uncaptured, or another session's transcript stored, would show it wrong From 2475b570de2da5fe83e803b33c502758e0579f47 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Fri, 25 Sep 2026 14:50:50 +0100 Subject: [PATCH 11/36] fix(memory): read the store through one os.Root handle Containment of .abcd/memory was a per-verb pre-check: five entry points called safeMemoryDir and then read by path, and fileBack in ask.go read the existing pages and the registry through Dir(root) with no check, so only the later write refused a symlinked store, after the registry beyond it had been read. A path read after the check could also be redirected by a swap. The store is now opened once per verb as a storeHandle: the same segment walk, then an os.Root reached through an os.Root on the repository and confirmed to be the directory the walk vetted. Every read (Bare, QueryPages, Ingest's dedup reads, fileBack, Lint's crawl and residue scan, the coverage crawl, the headroom render) goes through it, via fsutil.ReadGuardedInRoot, so containment is a property of the handle rather than a check each verb remembers. The writer keeps its own locked validatedMemoryDir path. Refs: iss-2608291814572914 Assisted-by: Claude:claude-opus-5-5 --- internal/core/memory/ask.go | 52 +++--- internal/core/memory/bare.go | 75 ++------ internal/core/memory/guarded_reads_test.go | 11 +- internal/core/memory/ingest.go | 42 ++--- internal/core/memory/lint.go | 130 ++++---------- internal/core/memory/provenance.go | 7 + internal/core/memory/store.go | 194 +++++++++++++++++++++ internal/core/memory/store_handle_test.go | 92 ++++++++++ 8 files changed, 393 insertions(+), 210 deletions(-) create mode 100644 internal/core/memory/store.go create mode 100644 internal/core/memory/store_handle_test.go diff --git a/internal/core/memory/ask.go b/internal/core/memory/ask.go index 68cfffb35..eac4ee414 100644 --- a/internal/core/memory/ask.go +++ b/internal/core/memory/ask.go @@ -4,14 +4,11 @@ import ( "bytes" "encoding/json" "fmt" - "os" - "path/filepath" "regexp" "sort" "strings" "time" - "github.com/intentdriven/abcd/internal/fsutil" "github.com/intentdriven/abcd/internal/termsafe" ) @@ -201,37 +198,23 @@ func QueryPages(repoRoot, question string, topN int) ([]MatchedPage, error) { for _, t := range tokens { tokenSet[t] = true } - // Refuse a symlinked store DIRECTORY up front (GHSA-72rp): the leaf-guarded - // reads below only bind the leaf, so a committed `.abcd/memory` symlink would - // otherwise be walked and its out-of-repo pages disclosed. - mem, present, err := safeMemoryDir(repoRoot) + // Every read goes through the store handle, so a symlinked store DIRECTORY + // is refused when it is opened (GHSA-72rp) and nothing below can read + // outside it (iss-2608291814572914). + store, err := openStore(repoRoot) if err != nil { return nil, err } - if !present { - return nil, nil - } - entries, err := os.ReadDir(mem) - if err != nil { - if os.IsNotExist(err) { - return nil, nil - } - return nil, err - } + defer store.Close() var matches []MatchedPage - for _, e := range entries { - if !e.Type().IsRegular() || !IsMemoryPageName(e.Name()) { - continue - } - // ReadGuarded re-checks regular-file on the open fd (closing the - // ReadDir→open symlink-swap TOCTOU) and caps the size. - raw, err := fsutil.ReadGuarded(filepath.Join(mem, e.Name()), maxMemoryPageBytes) + for _, name := range store.pageNames() { + raw, err := store.read(name, maxMemoryPageBytes) if err != nil { continue } text := string(raw) page := parsePage(text) - info := pageInfoOf(e.Name(), page) + info := pageInfoOf(name, page) if classFilter != "" { ok := false for _, c := range info.Classes { @@ -263,7 +246,7 @@ func QueryPages(repoRoot, question string, topN int) ([]MatchedPage, error) { continue } matches = append(matches, MatchedPage{ - Filename: e.Name(), + Filename: name, Score: score, Classes: info.Classes, Domain: info.Domain, @@ -409,14 +392,21 @@ func fileBack(root string, matches []MatchedPage, rawPage map[string]any, decide return FileBackResult{Status: "declined"}, nil } - mem := Dir(root) - existing := existingPageFrontmatter(mem) - plan, err := ResolveDistilledPages(existing, []DistilledPage{page}) + // The store handle, not Dir(root): file-back reads the existing pages and + // the registry before it writes, and those reads were the ones no per-verb + // check covered (iss-2608291814572914). A symlinked store is refused here, + // before anything is read. + store, err := openStore(root) if err != nil { return FileBackResult{}, err } - - registry, err := LoadRegistry(SourcesIndexPath(root)) + existing := existingPageFrontmatter(store) + registry, err := store.registry() + store.Close() + if err != nil { + return FileBackResult{}, err + } + plan, err := ResolveDistilledPages(existing, []DistilledPage{page}) if err != nil { return FileBackResult{}, err } diff --git a/internal/core/memory/bare.go b/internal/core/memory/bare.go index d1dfcff0c..38c72b75d 100644 --- a/internal/core/memory/bare.go +++ b/internal/core/memory/bare.go @@ -3,13 +3,8 @@ package memory import ( "encoding/json" "fmt" - "io/fs" - "os" - "path/filepath" "sort" "strings" - - "github.com/intentdriven/abcd/internal/fsutil" ) // bare.go — the SD001-non-mutating bare render: page count by class, @@ -36,15 +31,17 @@ type BareStatus struct { // Bare renders the read-only store status. func Bare(repoRoot string) (BareStatus, error) { - // Refuse a symlinked store DIRECTORY (GHSA-72rp): the leaf O_NOFOLLOW guards - // below do not contain a symlinked ancestor, so a committed `.abcd/memory` - // symlink would otherwise have its out-of-repo pages crawled and disclosed. - mem, present, err := safeMemoryDir(repoRoot) + // Every read goes through the store handle: a symlinked store DIRECTORY is + // refused when it is opened (GHSA-72rp), and no read below can leave it + // (iss-2608291814572914). + store, err := openStore(repoRoot) if err != nil { return BareStatus{}, err } + defer store.Close() + present := store.present() - infos := barePageInfos(mem) + infos := barePageInfos(store) // Seed the collections non-nil so an empty or contradiction-free store // marshals them as [] in --json, not bare null (every --json collection is an // empty list, never null; a healthy store keeps an empty contradictions list). @@ -77,12 +74,12 @@ func Bare(repoRoot string) (BareStatus, error) { }) registry := map[string]any{} - if r, err := LoadRegistry(SourcesIndexPath(repoRoot)); err == nil { + if r, err := store.registry(); err == nil { registry = r } status.LastIngest = bareLastIngest(registry) - if contrText, ok := readOrEmpty(filepath.Join(mem, "contradictions.md")); ok { + if contrText, ok := store.readText("contradictions.md"); ok { for _, line := range strings.Split(contrText, "\n") { t := strings.TrimSpace(line) if strings.HasPrefix(t, "- ") { @@ -98,7 +95,7 @@ func Bare(repoRoot string) (BareStatus, error) { } stale := map[string]bool{} for name, want := range desired { - current, ok := readOrEmpty(filepath.Join(mem, name)) + current, ok := store.readText(name) if !ok || sha256Hex(current) != sha256Hex(want) { stale[name] = true } @@ -111,25 +108,14 @@ func Bare(repoRoot string) (BareStatus, error) { } } - status.Headroom = bareHeadroomLines(repoRoot, mem) + status.Headroom = bareHeadroomLines(repoRoot, store) return status, nil } -func barePageInfos(mem string) []PageInfo { - entries, err := os.ReadDir(mem) - if err != nil { - return nil - } +func barePageInfos(store *storeHandle) []PageInfo { var infos []PageInfo - names := make([]string, 0, len(entries)) - for _, e := range entries { - if e.Type().IsRegular() && IsMemoryPageName(e.Name()) { - names = append(names, e.Name()) - } - } - sort.Strings(names) - for _, name := range names { - if text, ok := readOrEmpty(filepath.Join(mem, name)); ok { + for _, name := range store.pageNames() { + if text, ok := store.readText(name); ok { infos = append(infos, pageInfoFrom(name, text)) } } @@ -172,11 +158,10 @@ func fmtSignedPct(fraction float64) string { return fmt.Sprintf("+%.0f%%", pct) } -func bareHeadroomLines(repoRoot, mem string) []string { +func bareHeadroomLines(repoRoot string, store *storeHandle) []string { const header = "Quotation-budget headroom:" - indexPath := CoverageIndexPath(repoRoot) - raw, err := fsutil.ReadGuarded(indexPath, maxRegistryBytes) + raw, err := store.read(coverageIndexName, maxRegistryBytes) if err != nil { return []string{header + " coverage index not built yet — run `abcd memory lint`"} } @@ -193,21 +178,8 @@ func bareHeadroomLines(repoRoot, mem string) []string { } // Read-only crawl over the same typed pages the lint crawls. - var pages []crawledPage - _ = filepath.WalkDir(mem, func(path string, d fs.DirEntry, err error) error { - if err != nil || !d.Type().IsRegular() || !strings.HasSuffix(path, ".md") { - return nil - } - if !isTypedMemoryPagePath(mem, path) { - return nil - } - if b, err := fsutil.ReadGuarded(path, maxMemoryPageBytes); err == nil { - rel, _ := filepath.Rel(mem, path) - pages = append(pages, crawledPage{rel: filepath.ToSlash(rel), text: string(b)}) - } - return nil - }) - registry, regErr := LoadRegistry(SourcesIndexPath(repoRoot)) + pages := store.typedPages() + registry, regErr := store.registry() if regErr != nil { registry = nil } @@ -270,14 +242,3 @@ func bareHeadroomLines(repoRoot, mem string) []string { } return lines } - -// readOrEmpty reads one store file through the guarded primitive: the store -// sits inside the repo working tree — a trust boundary — so a committed -// symlink leaf is refused rather than followed, and the read is size-capped. -func readOrEmpty(path string) (string, bool) { - raw, err := fsutil.ReadGuarded(path, maxMemoryPageBytes) - if err != nil { - return "", false - } - return string(raw), true -} diff --git a/internal/core/memory/guarded_reads_test.go b/internal/core/memory/guarded_reads_test.go index 992132325..361a11c26 100644 --- a/internal/core/memory/guarded_reads_test.go +++ b/internal/core/memory/guarded_reads_test.go @@ -55,8 +55,15 @@ func TestLintSkipsSymlinkedTypedPage(t *testing.T) { } plantSymlink(t, outside, filepath.Join(mem, "fact_eng_injected.md")) - if isTypedMemoryPagePath(mem, filepath.Join(mem, "fact_eng_injected.md")) { - t.Fatal("a symlinked page was followed and classified as a typed memory page") + store, err := openStore(root) + if err != nil { + t.Fatal(err) + } + defer store.Close() + for _, p := range store.typedPages() { + if p.rel == "fact_eng_injected.md" { + t.Fatal("a symlinked page was followed and classified as a typed memory page") + } } } diff --git a/internal/core/memory/ingest.go b/internal/core/memory/ingest.go index a0d1e0f65..d33f6db09 100644 --- a/internal/core/memory/ingest.go +++ b/internal/core/memory/ingest.go @@ -113,9 +113,11 @@ func Ingest(req IngestRequest) (IngestResult, error) { // already refused at WritePages -> validatedMemoryDir, but that fires only // after these reads; guarding here closes the pre-write read. A missing store // is fine (present=false) — WritePages materialises it. - if _, _, err := safeMemoryDir(root); err != nil { + store, err := openStore(root) + if err != nil { return IngestResult{}, err } + defer store.Close() now := req.Now if now.IsZero() { now = time.Now().UTC() @@ -167,8 +169,6 @@ func Ingest(req IngestRequest) (IngestResult, error) { memoryConsumer, _ = consumers["memory"].(map[string]any) } } - mem := Dir(root) - // ---- Registry-hit fast path (validate BEFORE mutate) ------------------- var validRecorded []string var recorded []string @@ -176,7 +176,7 @@ func Ingest(req IngestRequest) (IngestResult, error) { recorded = anyToStrings(memoryConsumer["pages"]) allValid := len(recorded) > 0 for _, pageName := range recorded { - hashes, present := pageHashSet(mem, pageName) + hashes, present := pageHashSet(store, pageName) if present && contains(hashes, contentHash) { validRecorded = append(validRecorded, pageName) } else { @@ -295,13 +295,13 @@ func Ingest(req IngestRequest) (IngestResult, error) { } // ---- Existing pages + repair safety ------------------------------------ - existing := existingPageFrontmatter(mem) + existing := existingPageFrontmatter(store) if repairing { for _, pageName := range recorded { if contains(validRecorded, pageName) { continue } - hashes, present := pageHashSet(mem, pageName) + hashes, present := pageHashSet(store, pageName) if !present { continue // missing — re-distil writes fresh } @@ -472,12 +472,11 @@ func backlinkOtherHashes(registry map[string]any, plan WritePlan, contentHash st // fact; a few MiB is far more than any real one. const maxMemoryPageBytes = 4 << 20 // 4 MiB -func pageHashSet(mem, filename string) ([]string, bool) { +func pageHashSet(store *storeHandle, filename string) ([]string, bool) { if !IsMemoryPageName(filename) { return nil, false } - path := filepath.Join(mem, filename) - raw, err := fsutil.ReadGuarded(path, maxMemoryPageBytes) + raw, err := store.read(filename, maxMemoryPageBytes) if err != nil { if os.IsNotExist(err) { return nil, false @@ -490,30 +489,23 @@ func pageHashSet(mem, filename string) ([]string, bool) { return SourceHashes(pageSourceBlock(string(raw))), true } -func existingPageFrontmatter(mem string) map[string]map[string]any { +// existingPageFrontmatter reads every top-level page's frontmatter through the +// store handle, so a hostile page can neither redirect the read nor exhaust +// memory, and nothing is read from outside the store. +func existingPageFrontmatter(store *storeHandle) map[string]map[string]any { pages := map[string]map[string]any{} - entries, err := os.ReadDir(mem) - if err != nil { - return pages - } - for _, e := range entries { - if !e.Type().IsRegular() || !IsMemoryPageName(e.Name()) { - continue - } - // ReadGuarded re-checks regular-file on the open fd (closing the ReadDir→ - // open symlink-swap TOCTOU) and caps the size, so a hostile page cannot - // redirect the read or exhaust memory. - raw, err := fsutil.ReadGuarded(filepath.Join(mem, e.Name()), maxMemoryPageBytes) + for _, name := range store.pageNames() { + raw, err := store.read(name, maxMemoryPageBytes) if err != nil { - pages[e.Name()] = map[string]any{} + pages[name] = map[string]any{} continue } fm, err := parseFrontmatter(string(raw)) if err != nil { - pages[e.Name()] = map[string]any{} + pages[name] = map[string]any{} continue } - pages[e.Name()] = fm + pages[name] = fm } return pages } diff --git a/internal/core/memory/lint.go b/internal/core/memory/lint.go index a6acfac78..3f198064f 100644 --- a/internal/core/memory/lint.go +++ b/internal/core/memory/lint.go @@ -10,7 +10,6 @@ import ( "strings" "time" - "github.com/intentdriven/abcd/internal/fsutil" "github.com/intentdriven/abcd/internal/termsafe" ) @@ -76,37 +75,8 @@ func severityFor(code string) string { // Typed-page gate // --------------------------------------------------------------------------- -func isTypedMemoryPagePath(mem, path string) bool { - base := filepath.Base(path) - if siblingFiles[base] { - return false - } - if _, _, _, ok := ParsePageFilename(base); !ok { - return false - } - rel, err := filepath.Rel(mem, path) - if err != nil { - return false - } - segs := strings.Split(filepath.ToSlash(rel), "/") - for i := 0; i < len(segs)-1; i++ { - if segs[i] == "sources" { - return false - } - } - // Guarded read: the store is a trust boundary, so a committed symlink - // page is refused here (O_NOFOLLOW) rather than followed unbounded. - raw, err := fsutil.ReadGuarded(path, maxMemoryPageBytes) - if err != nil { - return false - } - fm, err := parseFrontmatter(string(raw)) - if err != nil { - return false - } - _, ok := fm["source"] - return ok -} +// The typed-page gate is isTypedMemoryPage (store.go), applied to a page the +// store handle has already read. // --------------------------------------------------------------------------- // Page-local linter @@ -338,21 +308,26 @@ func quotedLine(text, s string) int { // from the pages by reconcile, so a page finding covers them. A binary // kept-original (a PDF) cannot be scanned span-wise and is skipped, as the // write side declines to rewrite it; its distilled pages are scanned instead. -func residueOfStoreFiles(r *storeRedactor, repoRoot, mem string) []Finding { +func residueOfStoreFiles(r *storeRedactor, store *storeHandle) []Finding { var out []Finding - index := SourcesIndexPath(repoRoot) - if raw, err := fsutil.ReadGuarded(index, maxRegistryBytes); err == nil { + const indexName = ".sources_index.json" + index := store.path(indexName) + if raw, err := store.read(indexName, maxRegistryBytes); err == nil { links := storedBackLinks(raw) out = append(out, residueFindings(r, maskBackLinks(string(raw), links), index)...) for _, bl := range links { out = append(out, pageNameResidue(r, bl.name, index, bl.line)...) } } - sources := filepath.Join(mem, "sources") - if fi, err := os.Lstat(sources); err != nil || fi.Mode()&os.ModeSymlink != 0 || !fi.IsDir() { + if store.root == nil { return out } - entries, err := os.ReadDir(sources) + // ReadDir through the handle: a symlinked sources/ fails to open as a + // directory inside the root rather than being listed through. + if fi, err := store.root.Lstat("sources"); err != nil || fi.Mode()&os.ModeSymlink != 0 || !fi.IsDir() { + return out + } + entries, err := fs.ReadDir(store.root.FS(), "sources") if err != nil { return out } @@ -360,12 +335,12 @@ func residueOfStoreFiles(r *storeRedactor, repoRoot, mem string) []Finding { if !e.Type().IsRegular() { continue } - path := filepath.Join(sources, e.Name()) - raw, err := fsutil.ReadGuarded(path, maxFetchBytes) + rel := "sources/" + e.Name() + raw, err := store.read(rel, maxFetchBytes) if err != nil || !isRedactableText(raw) { continue } - out = append(out, residueFindings(r, string(raw), path)...) + out = append(out, residueFindings(r, string(raw), store.path(rel))...) } return out } @@ -500,40 +475,22 @@ func runMemoryCoverageLint(repoRoot string) ([]Finding, map[string]any, error) { "new_fingerprint": nil, "written": false, } - // Refuse a symlinked store DIRECTORY before the coverage index is written - // (GHSA-72rp): writeCoverageIndex would otherwise MkdirAll + write into the - // symlink target, escaping the repo. - mem, present, err := safeMemoryDir(repoRoot) + // The store handle refuses a symlinked store DIRECTORY before the coverage + // index is written (GHSA-72rp): writeCoverageIndex would otherwise MkdirAll + // + write into the symlink target, escaping the repo. The crawl reads + // through it (iss-2608291814572914). + store, err := openStore(repoRoot) if err != nil { return nil, report, err } - if !present { + defer store.Close() + if !store.present() { return nil, report, nil } - - var pages []crawledPage - err = filepath.WalkDir(mem, func(path string, d fs.DirEntry, err error) error { - if err != nil || !d.Type().IsRegular() || !strings.HasSuffix(path, ".md") { - return nil - } - if !isTypedMemoryPagePath(mem, path) { - return nil - } - raw, err := fsutil.ReadGuarded(path, maxMemoryPageBytes) - if err != nil { - return nil - } - rel, _ := filepath.Rel(mem, path) - pages = append(pages, crawledPage{rel: filepath.ToSlash(rel), text: string(raw)}) - return nil - }) - if err != nil { - return nil, report, err - } - sort.Slice(pages, func(i, j int) bool { return pages[i].rel < pages[j].rel }) + pages := store.typedPages() budget := loadQuotationBudget(repoRoot) - registry, regErr := LoadRegistry(SourcesIndexPath(repoRoot)) + registry, regErr := store.registry() if regErr != nil { registry = nil } @@ -618,17 +575,17 @@ func Lint(req LintRequest) (LintResult, error) { if now.IsZero() { now = time.Now().UTC() } - // Refuse a symlinked store DIRECTORY before any crawl or coverage write - // (GHSA-72rp): the leaf O_NOFOLLOW guards do not contain a symlinked ancestor, - // and runMemoryCoverageLint would otherwise write .coverage_index.json into - // the symlink target. - mem, present, err := safeMemoryDir(root) + // The store handle refuses a symlinked store DIRECTORY before any crawl or + // coverage write (GHSA-72rp), and every read below goes through it + // (iss-2608291814572914). + store, err := openStore(root) if err != nil { return LintResult{}, err } + mem := store.dir var findings []Finding - if present { + if store.present() { // The store redactor's read side (GHSA-xj89-cc2c-wgwr). A degraded // scanner is a blocker finding against the store rather than an error: // lint's contract is to always crawl and write its report, and the exit @@ -641,31 +598,14 @@ func Lint(req LintRequest) (LintResult, error) { Suggestion: "Repair or remove the per-repo scanner override at .abcd/config/pii.json and re-run lint.", }) } - var pagePaths []string - err := filepath.WalkDir(mem, func(path string, d fs.DirEntry, err error) error { - if err != nil || !d.Type().IsRegular() || !strings.HasSuffix(path, ".md") { - return nil - } - if isTypedMemoryPagePath(mem, path) { - pagePaths = append(pagePaths, path) - } - return nil - }) - if err != nil { - return LintResult{}, err - } - sort.Strings(pagePaths) - for _, path := range pagePaths { - raw, err := fsutil.ReadGuarded(path, maxMemoryPageBytes) - if err != nil { - continue - } - findings = append(findings, newMemoryLinter(path, root, string(raw), redactor).run()...) + for _, p := range store.typedPages() { + findings = append(findings, newMemoryLinter(store.path(p.rel), root, p.text, redactor).run()...) } if redactor != nil { - findings = append(findings, residueOfStoreFiles(redactor, root, mem)...) + findings = append(findings, residueOfStoreFiles(redactor, store)...) } } + store.Close() corpusFindings, coverageReport, err := runMemoryCoverageLint(root) if err != nil { diff --git a/internal/core/memory/provenance.go b/internal/core/memory/provenance.go index ad9e75ffe..233b2f60d 100644 --- a/internal/core/memory/provenance.go +++ b/internal/core/memory/provenance.go @@ -105,6 +105,13 @@ const maxRegistryBytes = 8 << 20 // 8 MiB // source metadata must fail loudly, never be silently replaced. func LoadRegistry(path string) (map[string]any, error) { raw, err := fsutil.ReadGuarded(path, maxRegistryBytes) + return decodeRegistry(raw, err, path) +} + +// decodeRegistry is LoadRegistry's meaning applied to a read that already +// happened, by path or through a store handle: absent is an empty registry, +// anything unreadable or malformed a *RegistryFormatError naming path. +func decodeRegistry(raw []byte, err error, path string) (map[string]any, error) { if err != nil { if os.IsNotExist(err) { return map[string]any{}, nil diff --git a/internal/core/memory/store.go b/internal/core/memory/store.go new file mode 100644 index 000000000..dc65cd77c --- /dev/null +++ b/internal/core/memory/store.go @@ -0,0 +1,194 @@ +package memory + +import ( + "errors" + "io/fs" + "os" + "path" + "path/filepath" + "sort" + "strings" + + "github.com/intentdriven/abcd/internal/fsutil" +) + +// storeHandle is an open memory store: the one way a verb reads +// .abcd/memory/ (iss-2608291814572914). +// +// Containment used to be a per-verb pre-check — each entry point called +// safeMemoryDir and then read by PATH — so a verb that forgot the check +// (fileBack did) read through a symlinked store, and even a verb that +// remembered it read by a path that a swap after the check could redirect. +// The handle holds the store directory open as an os.Root, reached through an +// os.Root on the repository, after the same segment walk refuses a symlinked +// or non-directory `.abcd` or `memory`. Every read then resolves inside that +// descriptor: a name cannot climb out of it, a symlink leaf is refused by +// fsutil.ReadGuardedInRoot, and replacing the directory after it was opened +// changes nothing the handle reads. A verb that has a handle cannot read the +// store any other way, which is what makes the containment structural rather +// than remembered. +// +// An absent store is a handle with no root: every read reports not-exist and +// every listing is empty, so callers need no separate branch for it. +type storeHandle struct { + dir string // the canonical store path, for messages and for the writer + root *os.Root // nil when the store is absent +} + +// openStore opens the repository's memory store for reading. It never creates +// anything; a symlinked or non-directory store segment is refused with an +// *UnsafeStorePathError, exactly as the per-verb check refused it. +func openStore(repoRoot string) (*storeHandle, error) { + mem, present, err := safeMemoryDir(repoRoot) + if err != nil { + return nil, err + } + h := &storeHandle{dir: mem} + if !present { + return h, nil + } + repo, err := os.OpenRoot(repoRoot) + if err != nil { + return nil, err + } + defer repo.Close() + // Opened THROUGH the repository root, so the store cannot resolve outside + // the repository even if a segment was swapped after the walk above. + root, err := repo.OpenRoot(filepath.Join(".abcd", "memory")) + if err != nil { + if errors.Is(err, fs.ErrNotExist) { + return h, nil + } + return nil, &UnsafeStorePathError{Msg: "memory store could not be opened inside the repository: " + mem} + } + // The directory opened must be the one the walk vetted: a symlink + // swapped in between the two resolves to a different directory. + opened, err := root.Stat(".") + if err != nil { + root.Close() + return nil, err + } + vetted, err := os.Lstat(mem) + if err != nil || vetted.Mode()&os.ModeSymlink != 0 || !os.SameFile(opened, vetted) { + root.Close() + return nil, &UnsafeStorePathError{Msg: "memory store changed while it was being opened: " + mem} + } + h.root = root + return h, nil +} + +// Close releases the store descriptor. It is safe on an absent store. +func (h *storeHandle) Close() { + if h != nil && h.root != nil { + h.root.Close() + } +} + +// present reports whether the store directory exists. +func (h *storeHandle) present() bool { return h.root != nil } + +// path is the absolute path of a store-relative name, for messages and +// findings only; nothing reads by it. +func (h *storeHandle) path(rel string) string { + return filepath.Join(h.dir, filepath.FromSlash(rel)) +} + +// read reads one store file by its store-relative, slash-separated name: +// contained in the store, a symlink leaf refused, size-capped. +func (h *storeHandle) read(rel string, limit int64) ([]byte, error) { + if h.root == nil { + return nil, &fs.PathError{Op: "open", Path: h.path(rel), Err: fs.ErrNotExist} + } + return fsutil.ReadGuardedInRoot(h.root, filepath.FromSlash(rel), limit) +} + +// readText is read for the small text files a status renders, reporting only +// whether the read succeeded. +func (h *storeHandle) readText(rel string) (string, bool) { + raw, err := h.read(rel, maxMemoryPageBytes) + if err != nil { + return "", false + } + return string(raw), true +} + +// pageNames lists the typed-page filenames at the store's top level, sorted. +// Only regular files are listed; a symlink entry is not a page. +func (h *storeHandle) pageNames() []string { + if h.root == nil { + return nil + } + entries, err := fs.ReadDir(h.root.FS(), ".") + if err != nil { + return nil + } + var names []string + for _, e := range entries { + if e.Type().IsRegular() && IsMemoryPageName(e.Name()) { + names = append(names, e.Name()) + } + } + sort.Strings(names) + return names +} + +// typedPages crawls the store for every typed memory page — a page-shaped +// .md file outside sources/ whose frontmatter carries a source block — and +// returns each once, read once, sorted by its store-relative name. The walk +// never descends a symlinked directory (fs.WalkDir does not follow them) and +// every read goes through the handle. +func (h *storeHandle) typedPages() []crawledPage { + if h.root == nil { + return nil + } + var pages []crawledPage + _ = fs.WalkDir(h.root.FS(), ".", func(rel string, d fs.DirEntry, err error) error { + if err != nil || !d.Type().IsRegular() || !strings.HasSuffix(rel, ".md") { + return nil + } + raw, err := h.read(rel, maxMemoryPageBytes) + if err != nil { + return nil + } + if isTypedMemoryPage(rel, string(raw)) { + pages = append(pages, crawledPage{rel: rel, text: string(raw)}) + } + return nil + }) + sort.Slice(pages, func(i, j int) bool { return pages[i].rel < pages[j].rel }) + return pages +} + +// isTypedMemoryPage is the typed-page gate over a page already read: a +// page-shaped filename that is not a sibling file, not under sources/, whose +// frontmatter parses and carries a source block. +func isTypedMemoryPage(rel, text string) bool { + base := path.Base(rel) + if siblingFiles[base] { + return false + } + if _, _, _, ok := ParsePageFilename(base); !ok { + return false + } + segs := strings.Split(rel, "/") + for i := 0; i < len(segs)-1; i++ { + if segs[i] == "sources" { + return false + } + } + fm, err := parseFrontmatter(text) + if err != nil { + return false + } + _, ok := fm["source"] + return ok +} + +// registry loads the sources index through the handle, with LoadRegistry's +// meaning: absent is an empty registry, and an unreadable or malformed one is +// a *RegistryFormatError. +func (h *storeHandle) registry() (map[string]any, error) { + const name = ".sources_index.json" + raw, err := h.read(name, maxRegistryBytes) + return decodeRegistry(raw, err, h.path(name)) +} diff --git a/internal/core/memory/store_handle_test.go b/internal/core/memory/store_handle_test.go new file mode 100644 index 000000000..6359117e8 --- /dev/null +++ b/internal/core/memory/store_handle_test.go @@ -0,0 +1,92 @@ +package memory + +import ( + "errors" + "os" + "path/filepath" + "testing" +) + +// TestStoreHandleReadsOnlyTheDirectoryItOpened pins iss-2608291814572914: +// containment is a property of the handle, not a check each verb remembers. +// A store directory replaced by a symlink AFTER the handle was opened must not +// redirect a single read: the handle keeps reading the directory it vetted. +func TestStoreHandleReadsOnlyTheDirectoryItOpened(t *testing.T) { + root := t.TempDir() + mem := filepath.Join(root, ".abcd", "memory") + if err := os.MkdirAll(mem, 0o755); err != nil { + t.Fatal(err) + } + page := "---\nsource: sha256:0000\n---\n\nbody\n" + if err := os.WriteFile(filepath.Join(mem, "fact_eng_inside.md"), []byte(page), 0o644); err != nil { + t.Fatal(err) + } + outside := filepath.Join(t.TempDir(), "elsewhere") + if err := os.MkdirAll(outside, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(outside, "fact_eng_outside.md"), []byte(page), 0o644); err != nil { + t.Fatal(err) + } + + store, err := openStore(root) + if err != nil { + t.Fatal(err) + } + defer store.Close() + // Swap the vetted directory for a symlink to one outside the repository. + if err := os.Rename(mem, filepath.Join(root, ".abcd", "memory.moved")); err != nil { + t.Fatal(err) + } + if err := os.Symlink(outside, mem); err != nil { + t.Fatal(err) + } + + for name := range existingPageFrontmatter(store) { + if name == "fact_eng_outside.md" { + t.Fatal("a read through the handle followed a store directory swapped in after it was opened") + } + } + if _, err := store.read("fact_eng_outside.md", maxMemoryPageBytes); err == nil { + t.Fatal("the handle read a page from outside the directory it opened") + } + if _, err := store.read("fact_eng_inside.md", maxMemoryPageBytes); err != nil { + t.Fatalf("the handle lost the directory it opened: %v", err) + } +} + +// TestFileBackRefusesASymlinkedStore: fileBack read the existing pages and the +// registry through Dir(root) with no check at all, so only the later write +// refused a symlinked store, after the registry beyond it had been read. It +// now opens the store handle first, and a symlinked store is refused before +// anything under it is read. +func TestFileBackRefusesASymlinkedStore(t *testing.T) { + repo := t.TempDir() + seedAskStore(t, repo) + matches, err := QueryPages(repo, "how does token rotation work?", AskTopN) + if err != nil || len(matches) == 0 { + t.Fatalf("fixture: QueryPages = %d matches, %v", len(matches), err) + } + // The store becomes a symlink to a directory whose registry does not parse: + // reaching it surfaces as a registry format error, not as the refusal. + outside := t.TempDir() + if err := os.WriteFile(filepath.Join(outside, ".sources_index.json"), []byte("{not json"), 0o644); err != nil { + t.Fatal(err) + } + mem := Dir(repo) + if err := os.Rename(mem, mem+".moved"); err != nil { + t.Fatal(err) + } + if err := os.Symlink(outside, mem); err != nil { + t.Fatal(err) + } + page := map[string]any{ + "type": "topic", "domain": "auth", "slug": "rotation-summary", + "body": "# Rotation summary\nRotate daily.\n", + } + _, err = fileBack(repo, matches, page, nil, fixedNow) + var unsafe *UnsafeStorePathError + if !errors.As(err, &unsafe) { + t.Fatalf("fileBack on a symlinked store = %v (%T); want *UnsafeStorePathError before any read", err, err) + } +} From 474f4dd679f2c2eaa5e0e88813bdefb0ad40914d Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Fri, 25 Sep 2026 14:50:52 +0100 Subject: [PATCH 12/36] =?UTF-8?q?chore:=20resolve=20iss-2608291814572914?= =?UTF-8?q?=20=E2=80=94=20structural=20memory=20store=20containment?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves: iss-2608291814572914 Assisted-by: Claude:claude-opus-5-5 --- ...memory-store-containment-is-per-verb-not-structural.md | 8 ++++++++ 1 file changed, 8 insertions(+) rename .abcd/work/issues/{open => resolved}/iss-2608291814572914-memory-store-containment-is-per-verb-not-structural.md (58%) diff --git a/.abcd/work/issues/open/iss-2608291814572914-memory-store-containment-is-per-verb-not-structural.md b/.abcd/work/issues/resolved/iss-2608291814572914-memory-store-containment-is-per-verb-not-structural.md similarity index 58% rename from .abcd/work/issues/open/iss-2608291814572914-memory-store-containment-is-per-verb-not-structural.md rename to .abcd/work/issues/resolved/iss-2608291814572914-memory-store-containment-is-per-verb-not-structural.md index 8e3b62b04..015a76244 100644 --- a/.abcd/work/issues/open/iss-2608291814572914-memory-store-containment-is-per-verb-not-structural.md +++ b/.abcd/work/issues/resolved/iss-2608291814572914-memory-store-containment-is-per-verb-not-structural.md @@ -7,6 +7,14 @@ category: "architectural-insight" source: "impl-review" found_during: "ultra-v0.6.8-followup" found_at: "internal/core/memory/writer.go" +resolution: "Every memory read (Bare, QueryPages, Ingest dedup, fileBack, Lint crawl and residue, coverage crawl, headroom) goes through one os.Root store handle opened inside the repository root; fileBack now opens it before reading. The locked writer keeps validatedMemoryDir for its writes." +impact: fix +resolved_by: + commit: "2475b570" --- ultra-v0.6.8 altitude 3: the memory store's symlink guard is a per-verb pre-check repeated at five entry points (Bare, QueryPages, Ingest, Lint, runMemoryCoverageLint) rather than a containment mechanism, and fileBack in ask.go reached Dir(root) and existingPageFrontmatter without it. The site package fixed the identical class (gh #487) by opening one os.Root and routing every read through fsutil.ReadGuardedInRoot. Deeper fix: memory holds a store-root handle the same way so containment is structural rather than remembered at each verb. + +## Grounds + +- pursued: a store swapped for a symlink after the handle opened redirects no read, and fileBack refuses a symlinked store before reading its registry; a memory read by path outside store.go reappearing would show it wrong From cdf5a4342b0da074aeda34a5df2e85087e834ae4 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Fri, 25 Sep 2026 14:51:27 +0100 Subject: [PATCH 13/36] fix(memory): compare the store lock's file type under S_IFMT The store-lock guard asserted a regular file with st.Mode&S_IFREG != 0, but the file type is an enumeration under S_IFMT: a socket and a symlink both carry the S_IFREG bit and passed. The type is now compared under the mask. The O_NOFOLLOW open and the Lstat before it already refused those shapes, so this restores the guard as a defence rather than changing an observed outcome; folding the lock onto fsutil.WithFileLock stays with iss-129. Refs: iss-2608261133210491, iss-129 Assisted-by: Claude:claude-opus-5-5 --- internal/core/memory/storelock_mode_test.go | 29 +++++++++++++++++++++ internal/core/memory/writer.go | 10 ++++++- 2 files changed, 38 insertions(+), 1 deletion(-) create mode 100644 internal/core/memory/storelock_mode_test.go diff --git a/internal/core/memory/storelock_mode_test.go b/internal/core/memory/storelock_mode_test.go new file mode 100644 index 000000000..764757eb1 --- /dev/null +++ b/internal/core/memory/storelock_mode_test.go @@ -0,0 +1,29 @@ +package memory + +import ( + "syscall" + "testing" +) + +// TestStoreLockModeGuardMasksTheFileType pins iss-2608261133210491. The +// store-lock guard tested st.Mode&S_IFREG != 0, but the file-type field is an +// enumeration, not a set of flags: a socket (S_IFSOCK) and a symlink (S_IFLNK) +// both carry the S_IFREG bit, so the "regular file" assertion admitted them. +// The type is compared under the S_IFMT mask. +func TestStoreLockModeGuardMasksTheFileType(t *testing.T) { + for _, tc := range []struct { + name string + mode uint32 + want bool + }{ + {"regular", syscall.S_IFREG | 0o600, true}, + {"socket", syscall.S_IFSOCK | 0o600, false}, + {"symlink", syscall.S_IFLNK | 0o777, false}, + {"directory", syscall.S_IFDIR | 0o700, false}, + {"fifo", syscall.S_IFIFO | 0o600, false}, + } { + if got := lockModeIsRegular(uint32(tc.mode)); got != tc.want { + t.Errorf("lockModeIsRegular(%s) = %v, want %v", tc.name, got, tc.want) + } + } +} diff --git a/internal/core/memory/writer.go b/internal/core/memory/writer.go index 8e0cfb57e..ad1b44293 100644 --- a/internal/core/memory/writer.go +++ b/internal/core/memory/writer.go @@ -67,7 +67,7 @@ func WithStoreLock(repoRoot string, fn func() error) error { if err := syscall.Fstat(fd, &st); err != nil { return err } - if st.Mode&syscall.S_IFREG == 0 { + if !lockModeIsRegular(uint32(st.Mode)) { return &UnsafeStorePathError{Msg: "memory store lock fd is not a regular file: " + path} } if st.Nlink < 1 { @@ -82,6 +82,14 @@ func WithStoreLock(repoRoot string, fn func() error) error { return fn() } +// lockModeIsRegular reports whether a stat mode names a regular file. The +// file-type field is an enumeration under S_IFMT, not a set of flags: a socket +// and a symlink both carry the S_IFREG bit, so testing that bit alone admitted +// them (iss-2608261133210491). +func lockModeIsRegular(mode uint32) bool { + return mode&syscall.S_IFMT == syscall.S_IFREG +} + // memoryDir is the ONE walk that resolves /.abcd/memory for every // entry point, read or write. Each owned segment is lstat-refused as a symlink // or non-directory, so a committed `.abcd/memory` DIRECTORY symlink (git mode From a88dea08ea23639a160dfaac94312321d912e309 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Fri, 25 Sep 2026 14:51:29 +0100 Subject: [PATCH 14/36] =?UTF-8?q?chore:=20resolve=20iss-2608261133210491?= =?UTF-8?q?=20=E2=80=94=20store-lock=20S=5FIFMT=20mask?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves: iss-2608261133210491 Assisted-by: Claude:claude-opus-5-5 --- ...210491-memory-storelock-wrong-ifmt-mask.md | 12 ----------- ...210491-memory-storelock-wrong-ifmt-mask.md | 20 +++++++++++++++++++ 2 files changed, 20 insertions(+), 12 deletions(-) delete mode 100644 .abcd/work/issues/open/iss-2608261133210491-memory-storelock-wrong-ifmt-mask.md create mode 100644 .abcd/work/issues/resolved/iss-2608261133210491-memory-storelock-wrong-ifmt-mask.md diff --git a/.abcd/work/issues/open/iss-2608261133210491-memory-storelock-wrong-ifmt-mask.md b/.abcd/work/issues/open/iss-2608261133210491-memory-storelock-wrong-ifmt-mask.md deleted file mode 100644 index cd971ce33..000000000 --- a/.abcd/work/issues/open/iss-2608261133210491-memory-storelock-wrong-ifmt-mask.md +++ /dev/null @@ -1,12 +0,0 @@ ---- -schema_version: 1 -id: "iss-2608261133210491" -slug: "memory-storelock-wrong-ifmt-mask" -severity: "nitpick" -category: "tech-debt" -source: "agent-finding" -found_during: "bughunt-round-8" -found_at: "internal/core/memory/writer.go:70" ---- - -the memory store-lock guard tests mode AND S_IFREG nonzero instead of masking with S_IFMT, so its regular-file assertion also accepts symlink and socket modes; dead defence shielded by O_NOFOLLOW, fold into the iss-129 flock consolidation \ No newline at end of file diff --git a/.abcd/work/issues/resolved/iss-2608261133210491-memory-storelock-wrong-ifmt-mask.md b/.abcd/work/issues/resolved/iss-2608261133210491-memory-storelock-wrong-ifmt-mask.md new file mode 100644 index 000000000..d15be4c27 --- /dev/null +++ b/.abcd/work/issues/resolved/iss-2608261133210491-memory-storelock-wrong-ifmt-mask.md @@ -0,0 +1,20 @@ +--- +schema_version: 1 +id: "iss-2608261133210491" +slug: "memory-storelock-wrong-ifmt-mask" +severity: "nitpick" +category: "tech-debt" +source: "agent-finding" +found_during: "bughunt-round-8" +found_at: "internal/core/memory/writer.go:70" +resolution: "The store-lock fstat guard compares the file type under S_IFMT (lockModeIsRegular), so socket and symlink modes are refused; the flock consolidation itself stays with iss-129." +impact: internal +resolved_by: + commit: "8744c210" +--- + +the memory store-lock guard tests mode AND S_IFREG nonzero instead of masking with S_IFMT, so its regular-file assertion also accepts symlink and socket modes; dead defence shielded by O_NOFOLLOW, fold into the iss-129 flock consolidation + +## Grounds + +- pursued: the guard admits only S_IFREG under the S_IFMT mask; a socket or symlink mode accepted by lockModeIsRegular would show it wrong From bf6df37ba87508743fa801c5b2ff39564f5c06db Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Fri, 25 Sep 2026 14:55:42 +0100 Subject: [PATCH 15/36] fix(memory): stop a page name re-pairing the code span a render wraps it in termsafe's guarantees hold over the exact string CleanProse returned, and a renderer that adds its own delimiters parses a different string than the cleaner reasoned about. RenderIndex and RenderContradictions wrapped a cleaned page name in their own backticks, so a name carrying a balanced backtick pair re-paired with the wrapper and the raw HTML the cleaner sheltered inside it went live in a committed record. Both now wrap through a new termsafe.CodeSpan, which fences one backtick longer than the value's longest run and pads per CommonMark, leaving the value's bytes unaltered. Ask's render moves every untrusted field on its markdown lines from Sanitize to CleanProse (Sanitize leaves HTML openers and link syntax live) and wraps its filename the same way. The approach follows commit 9ef2373e on the unmerged 2026-09-02 branch fix/security-sweep-continued, re-applied at the current tip with its tests watched failing here first. The lifeboat renderers named in the same record are left to a lifeboat lane and captured separately. Refs: iss-2609020539188868 Assisted-by: Claude:claude-opus-5-5 --- internal/core/memory/ask.go | 26 ++-- internal/core/memory/codespan_test.go | 200 ++++++++++++++++++++++++++ internal/core/memory/schema.go | 17 ++- internal/termsafe/prose.go | 53 +++++++ 4 files changed, 281 insertions(+), 15 deletions(-) create mode 100644 internal/core/memory/codespan_test.go diff --git a/internal/core/memory/ask.go b/internal/core/memory/ask.go index eac4ee414..59775d98c 100644 --- a/internal/core/memory/ask.go +++ b/internal/core/memory/ask.go @@ -282,7 +282,10 @@ const AskReportHeading = "abcd memory ask" // citation-renderer, not an LLM. Missing provenance renders as explicit (none). func RenderCitedMatches(question string, matches []MatchedPage) string { lines := []string{ - "# " + AskReportHeading + " — " + termsafe.Sanitize(question), + // Every untrusted field on the answer's markdown lines goes through + // CleanProse, not Sanitize alone, which leaves an HTML opener and link + // syntax live (iss-2609020539188868). + "# " + AskReportHeading + " — " + cleanPageField(question), "", fmt.Sprintf("Matched pages (%d, overlap-ranked):", len(matches)), "", @@ -291,11 +294,13 @@ func RenderCitedMatches(question string, matches []MatchedPage) string { // Filename and Summary are page-derived (repo content); sanitise each field // before it joins the multi-line answer — masking the whole answer wholesale // would clobber its legitimate newlines. - summary := termsafe.Sanitize(m.Summary) + summary := cleanPageField(m.Summary) if summary == "" { summary = "(no summary)" } - lines = append(lines, fmt.Sprintf("- `%s` (score %d) — %s", termsafe.Sanitize(m.Filename), m.Score, summary)) + // The filename's code span is termsafe.CodeSpan's, never a hand-written + // backtick pair the name could re-pair with. + lines = append(lines, fmt.Sprintf("- %s (score %d) — %s", termsafe.CodeSpan(cleanPageField(m.Filename)), m.Score, summary)) for _, c := range m.Citations { // Every citation field is page-derived content from the same untrusted // ingest boundary as Summary/Filename above, so each is sanitised before @@ -305,17 +310,17 @@ func RenderCitedMatches(question string, matches []MatchedPage) string { // masked here (gh-250). class/source_hash are charset-constrained upstream, // but sanitising them too matches the sibling treatment and defends the // render even if that constraint ever weakens. - cls := termsafe.Sanitize(c.SourceClass) + cls := cleanPageField(c.SourceClass) if cls == "" { cls = "(none)" } - sh := termsafe.Sanitize(c.SourceHash) + sh := cleanPageField(c.SourceHash) if sh == "" { sh = "(none)" } cj := "(none)" if len(c.Citation) > 0 { - cj = termsafe.Sanitize(compactJSONSorted(c.Citation)) + cj = cleanPageField(compactJSONSorted(c.Citation)) } lines = append(lines, fmt.Sprintf(" - cites: class=%s | source_hash=%s | citation=%s", cls, sh, cj)) } @@ -323,12 +328,11 @@ func RenderCitedMatches(question string, matches []MatchedPage) string { return strings.Join(lines, "\n") + "\n" } -// RenderNoMatches is the explicit empty-result render. It sanitises the -// question itself, as RenderCitedMatches does, so a direct caller is covered -// and the two renders cannot disagree on what reaches the terminal; Sanitize -// is idempotent, so the copy Ask already masked costs nothing here. +// RenderNoMatches is the explicit empty-result render. It cleans the question +// itself, as RenderCitedMatches does, so a direct caller is covered and the two +// renders cannot disagree on what reaches the terminal. func RenderNoMatches(question string) string { - return "# " + AskReportHeading + " — " + termsafe.Sanitize(question) + "\n\n" + + return "# " + AskReportHeading + " — " + cleanPageField(question) + "\n\n" + "No matching memory pages (token overlap found nothing; an empty or absent store matches nothing).\n" + "Try different terms, an explicit class: / domain: filter, or ingest a source first.\n" } diff --git a/internal/core/memory/codespan_test.go b/internal/core/memory/codespan_test.go new file mode 100644 index 000000000..20f2a5be7 --- /dev/null +++ b/internal/core/memory/codespan_test.go @@ -0,0 +1,200 @@ +package memory + +import ( + "strings" + "testing" + + "github.com/intentdriven/abcd/internal/termsafe" +) + +// TestRenderIndexCannotHaveItsCodeSpanBrokenByAPageName is +// iss-2609020539188868: termsafe's guarantees hold over the EXACT string +// CleanProse returned, and a renderer that adds its own delimiters is parsing a +// different string than the cleaner reasoned about. +// +// The cleaner shelters what sits inside the FIELD's own code spans — no raw HTML +// is parsed there — and escapes an unpaired backtick run so two cleaned fields on +// one line cannot re-pair. Wrapping the field in the renderer's own single +// backticks defeats the first half: with `a`