diff --git a/.abcd/development/brief/04-surfaces/11-history.md b/.abcd/development/brief/04-surfaces/11-history.md index a961378fe..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 @@ -210,7 +217,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 @@ -324,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 942cda5bd..431ff6de6 100644 --- a/.abcd/development/release/surface.json +++ b/.abcd/development/release/surface.json @@ -911,8 +911,15 @@ { "path": "abcd history capture", "hidden": false, - "sentence": "Redact and store one raw session transcript from a file or stdin: Writes one record into the store; refuses stdin without --session.", + "sentence": "Redact and store a session transcript, or a whole session with --all: Writes one record per transcript; refuses stdin or --all without --session.", "flags": [ + { + "name": "all", + "shorthand": "", + "type": "bool", + "required": false, + "hidden": false + }, { "name": "kind", "shorthand": "", 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/open/iss-2608291814572914-memory-store-containment-is-per-verb-not-structural.md b/.abcd/work/issues/open/iss-2608291814572914-memory-store-containment-is-per-verb-not-structural.md deleted file mode 100644 index 8e3b62b04..000000000 --- a/.abcd/work/issues/open/iss-2608291814572914-memory-store-containment-is-per-verb-not-structural.md +++ /dev/null @@ -1,12 +0,0 @@ ---- -schema_version: 1 -id: "iss-2608291814572914" -slug: "memory-store-containment-is-per-verb-not-structural" -severity: "minor" -category: "architectural-insight" -source: "impl-review" -found_during: "ultra-v0.6.8-followup" -found_at: "internal/core/memory/writer.go" ---- - -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. diff --git a/.abcd/work/issues/open/iss-2609251355497247-the-lifeboat-half-of-iss-2609020539188868-is-still-open.md b/.abcd/work/issues/open/iss-2609251355497247-the-lifeboat-half-of-iss-2609020539188868-is-still-open.md new file mode 100644 index 000000000..195dc30c5 --- /dev/null +++ b/.abcd/work/issues/open/iss-2609251355497247-the-lifeboat-half-of-iss-2609020539188868-is-still-open.md @@ -0,0 +1,14 @@ +--- +schema_version: 1 +id: "iss-2609251355497247" +slug: "the-lifeboat-half-of-iss-2609020539188868-is-still-open" +severity: "minor" +category: "security" +source: "review-followup" +found_during: "autonomous run A resumed 2026-09-25" +origin: researcher-authored +production_mode: hand-written +found_at: "internal/core/lifeboat" +--- + +The lifeboat half of iss-2609020539188868 is still open after the memory renderers were fixed: synthesis_review renders a finding id through termsafe.Sanitize alone, never CleanProse, so it can still carry an HTML comment opener or link syntax, and wraps a severity in its own bracket; the press-release subhead wraps a cleaned value in its own emphasis; synthesis_principles writes a cleaned principle as a bare paragraph with no leading-marker escape. The fix is the one applied to memory: every untrusted field on a markdown line through CleanProse, and no renderer adding delimiters around a cleaned value (termsafe.CodeSpan where a code span is wanted). diff --git a/.abcd/work/issues/open/iss-2609251358062952-the-ci-job-id-record-lint-in-github-workflows-ci-yml-runs.md b/.abcd/work/issues/open/iss-2609251358062952-the-ci-job-id-record-lint-in-github-workflows-ci-yml-runs.md new file mode 100644 index 000000000..1bae2f897 --- /dev/null +++ b/.abcd/work/issues/open/iss-2609251358062952-the-ci-job-id-record-lint-in-github-workflows-ci-yml-runs.md @@ -0,0 +1,14 @@ +--- +schema_version: 1 +id: "iss-2609251358062952" +slug: "the-ci-job-id-record-lint-in-github-workflows-ci-yml-runs" +severity: "nitpick" +category: "inconsistency" +source: "review-followup" +found_during: "autonomous run A resumed 2026-09-25" +origin: researcher-authored +production_mode: hand-written +found_at: ".github/workflows/ci.yml" +--- + +The CI job id record-lint in .github/workflows/ci.yml runs scripts/check-reviews-cases.sh and scripts/check-reviews.sh (the reviews-charter gate), while the real record-lint is a step of the check job, so a required status check is named for a gate it does not run. Renaming the job alone breaks the merge gate, because the main-protection ruleset (mirrored in .abcd/work/rulesets/main-protection.json) requires the context record-lint: the rename and the live ruleset edit must land together, which needs the forge ruleset changed by someone holding that permission. Carried over from iss-304 (d-12) when its other two halves were closed. diff --git a/.abcd/work/issues/open/iss-2609260948440803-local-tier-writes-by-path-memory-lint-writes-its-run-log.md b/.abcd/work/issues/open/iss-2609260948440803-local-tier-writes-by-path-memory-lint-writes-its-run-log.md new file mode 100644 index 000000000..d96cc058f --- /dev/null +++ b/.abcd/work/issues/open/iss-2609260948440803-local-tier-writes-by-path-memory-lint-writes-its-run-log.md @@ -0,0 +1,14 @@ +--- +schema_version: 1 +id: "iss-2609260948440803" +slug: "local-tier-writes-by-path-memory-lint-writes-its-run-log" +severity: "minor" +category: "security" +source: "review-followup" +found_during: "autonomous run A resumed 2026-09-25: review3-history item 6" +origin: researcher-authored +production_mode: hand-written +found_at: "internal/core/memory/lint.go" +--- + +Local-tier writes by path: memory lint writes its run-log report by path into the local tier and follows a symlinked ancestor out of the checkout. Lint (internal/core/memory/lint.go, lintReportDir and the write after it) joins .abcd/.work.local/logs/memory/lint- onto the repo root, os.MkdirAll-s it, and writes report.json and report.md with fsutil.WriteFileAtomic by path. Nothing vets .abcd/.work.local or logs/ first, and a committed symlink beats .gitignore (git add -f), so a checkout that ships .abcd/.work.local as a symlink gets the directory chain and both reports created at the link's target: probed at 35d5cf5f with .abcd/.work.local linked to a directory outside the repo, Lint returned nil and logs/memory/lint-/report.json and report.md were written in the outside directory. The contained pattern for this same tier already exists: mode.SetAt (internal/core/mode/store.go) opens an os.Root on the checkout, root.Lstat-refuses a .abcd/.work.local that is not a real directory, and writes with fsutil.WriteFileAtomicInRoot. Two sites the review named alongside were checked at 35d5cf5f and are NOT in this class: intent/audit.go's review request and dead-letter writes vet .abcd/.work.local/reviews level by level with fsutil.EnsureRealDirAll, and history/location.go's moveFile writes under a chain Resolve proved real with fsutil.EnsureRealDir, so both refuse a symlinked ancestor; each keeps only a vet-by-path-then-write-by-path swap window. 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..6d4d62f7d --- /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: "cdf5a434" +--- + +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 diff --git a/.abcd/work/issues/resolved/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 new file mode 100644 index 000000000..8c1826352 --- /dev/null +++ b/.abcd/work/issues/resolved/iss-2608291814572914-memory-store-containment-is-per-verb-not-structural.md @@ -0,0 +1,20 @@ +--- +schema_version: 1 +id: "iss-2608291814572914" +slug: "memory-store-containment-is-per-verb-not-structural" +severity: "minor" +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 and its headroom, QueryPages, the Ingest dedup and registry load, fileBack, the Lint crawl, residue and quotation checks, and the coverage crawl with its budget and stored fingerprint) goes through one os.Root store handle opened inside the repository root, and one Lint holds one handle for both passes; fileBack opens it before reading. The crawls moved in 2475b570; the config, registry and fingerprint reads that commit left by path moved in a8652c12. The coverage-index write goes through the handle too (iss-2609252100150846), and so does the --keep-original write of sources/, through a handle Ingest opens after WritePages returns, since the lock and its walk lapse there (iss-2609260908572219). The locked writer keeps validatedMemoryDir and writes by path, a residual behind the store lock; it is the only write into the store by path." +impact: fix +resolved_by: + commit: "a8652c12" +--- + +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, pinned by TestStoreHandleReadsOnlyTheDirectoryItOpened and by the store_swap_test.go set, which swaps the store at the moment openStore returns and drives Lint, Bare and Ingest over it; fileBack refuses a symlinked store before reading its registry; a memory read by path outside store.go and the locked writer reappearing would show it wrong diff --git a/.abcd/work/issues/open/iss-2609020539188868-three-markdown-renderers-re-escape-or-re-wrap-a-value-termsa.md b/.abcd/work/issues/resolved/iss-2609020539188868-three-markdown-renderers-re-escape-or-re-wrap-a-value-termsa.md similarity index 77% rename from .abcd/work/issues/open/iss-2609020539188868-three-markdown-renderers-re-escape-or-re-wrap-a-value-termsa.md rename to .abcd/work/issues/resolved/iss-2609020539188868-three-markdown-renderers-re-escape-or-re-wrap-a-value-termsa.md index 8f17dbca6..8a7b676d9 100644 --- a/.abcd/work/issues/open/iss-2609020539188868-three-markdown-renderers-re-escape-or-re-wrap-a-value-termsa.md +++ b/.abcd/work/issues/resolved/iss-2609020539188868-three-markdown-renderers-re-escape-or-re-wrap-a-value-termsa.md @@ -9,6 +9,14 @@ found_during: "autonomous-run-2026-09-01" origin: researcher-authored production_mode: hand-written found_at: "internal/core/memory/schema.go" +resolution: "The memory renderers (RenderIndex, RenderContradictions, Ask's cited and empty renders) wrap cleaned names through termsafe.CodeSpan and clean every untrusted field with CleanProse. The lifeboat half is captured separately as iss-2609251355497247 for a lifeboat lane." +impact: fix +resolved_by: + commit: "bf6df37b" --- Three markdown renderers re-escape or re-wrap a value termsafe already cleaned, which is the class the intent audit's renderEvidence just had fixed. termsafe's guarantees hold over the exact string CleanProse returned; a caller that adds its own delimiters is parsing a different string than the cleaner reasoned about. memory.RenderIndex and memory.RenderContradictions (internal/core/memory/schema.go, the two backtick-wrapped format strings) wrap a cleaned page filename in their own backticks, so a filename carrying a backtick shifts code-span parity in the committed .abcd/memory/index.md and contradictions.md and can move a sheltered angle bracket out of its span; memory.Ask's match render has the same shape over a value cleaned only by Sanitize; lifeboat's synthesis_review severity bracket and press-release subhead emphasis are the weaker form (neither delimiter affects span parity, and the artefact is not a committed record), and synthesis_review renders f.ID through Sanitize alone, never CleanProse, so it can still carry an HTML comment opener or link syntax; lifeboat's synthesis_principles writes a cleaned principle as a bare paragraph with no leading-marker escape. Out of scope for the code-span fix that found them (a different package, a different caller, and none is a defect that change introduced). What the fix DID close for all of them is the same-line embedding half: no cleaned field can carry an unpaired backtick run any more, so two cleaned values on one line cannot re-pair. The fix must establish that no renderer alters a cleaned value's bytes, and that every untrusted field on a committed markdown line goes through CleanProse rather than Sanitize alone. + +## Grounds + +- pursued: a page name carrying a balanced backtick pair keeps its sheltered content inside one code span in index.md, contradictions.md and the ask answer; raw HTML or link syntax from a page field appearing live in any of the three would show it wrong 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 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 diff --git a/.abcd/work/issues/open/iss-2609090951282192-memory-filename-hard-fail-bar-is-wider-than-secrets-only.md b/.abcd/work/issues/resolved/iss-2609090951282192-memory-filename-hard-fail-bar-is-wider-than-secrets-only.md similarity index 81% rename from .abcd/work/issues/open/iss-2609090951282192-memory-filename-hard-fail-bar-is-wider-than-secrets-only.md rename to .abcd/work/issues/resolved/iss-2609090951282192-memory-filename-hard-fail-bar-is-wider-than-secrets-only.md index 46fe1b9b2..6cc05cac4 100644 --- a/.abcd/work/issues/open/iss-2609090951282192-memory-filename-hard-fail-bar-is-wider-than-secrets-only.md +++ b/.abcd/work/issues/resolved/iss-2609090951282192-memory-filename-hard-fail-bar-is-wider-than-secrets-only.md @@ -10,6 +10,14 @@ origin: researcher-authored production_mode: hand-written found_at: "internal/core/memory/redact.go" related_issues: ["iss-2608291444328326"] +resolution: "The page-filename bar (write refusal and MR001 name check alike) selects hard_fail findings that are not identity kinds, so it holds to secret patterns as ruled." +impact: fix +resolved_by: + commit: "534c4005" --- The memory write boundary refuses a page filename carrying a hard-fail span, and the ruling that ordered it asked for a bar narrow enough that an ordinary slug is not refused: secrets only. The delivered bar selects on the scanner hard-fail severity, and the scanner puts three kinds there rather than one, a secret pattern, a banned real name, and the caller's own local machine account name. The function comment says so plainly, so the width is disclosed rather than hidden, but it is wider than the ruling. The consequence is a refusal the author cannot act on: on a machine whose account name is an ordinary word, an ordinary page whose slug carries that word at a hyphen boundary is refused at ingest with a message telling the author to repair the slug at the source, when what matched was the machine account rather than anything in the page. That collision is already recorded against another surface as iss-2608291444328326, where the same rule turned the release payload gate red on a pristine tree until the home directory was pointed at an alias. It matters because a memory write is the one path here with no workaround: the page cannot be written under the name the distiller chose, and the remedy the message names is not the remedy. Fix direction: hold the filename to the secret patterns alone, as the ruling asked, or keep the identity kinds and make the refusal name which kind matched so an author can tell a collision from a leak. Detector: with the caller account name set to an ordinary word, an ordinary page whose slug contains that word must still write, while a page name carrying a token body is still refused. + +## Grounds + +- pursued: with the machine account named garden, topic_home_garden-plan.md writes and is indexed, while a ghp_ token slug is still refused; an ordinary slug refused for local_username or real_name would show it wrong 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 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 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 diff --git a/.abcd/work/issues/resolved/iss-2609252100150846-the-memory-coverage-lint-writes-coverage-index-json-by-path.md b/.abcd/work/issues/resolved/iss-2609252100150846-the-memory-coverage-lint-writes-coverage-index-json-by-path.md new file mode 100644 index 000000000..ee590a5f9 --- /dev/null +++ b/.abcd/work/issues/resolved/iss-2609252100150846-the-memory-coverage-lint-writes-coverage-index-json-by-path.md @@ -0,0 +1,22 @@ +--- +schema_version: 1 +id: "iss-2609252100150846" +slug: "the-memory-coverage-lint-writes-coverage-index-json-by-path" +severity: "minor" +category: "security" +source: "review-followup" +found_during: "autonomous run A resumed 2026-09-25" +origin: researcher-authored +production_mode: hand-written +found_at: "internal/core/memory/coverage.go" +resolution: "The coverage lint writes .coverage_index.json through the store handle's os.Root (fsutil.WriteFileAtomicInRoot), so the write resolves inside the directory the handle vetted; Lint holds that one handle for the page lint and the coverage lint." +impact: fix +resolved_by: + commit: "113f8ab0" +--- + +The memory coverage lint writes .coverage_index.json by path (writeCoverageIndex over CoverageIndexPath) after openStore vetted the store, so a store directory swapped for a symlink between the open and the write lands the index outside the repository: the handle's containment covers its reads, not this write. It is not behind the store lock or validatedMemoryDir the way the page writer is, so the writer residual iss-2608291814572914 declares does not cover it. Fix: write the index through the store handle's os.Root (fsutil.WriteFileAtomicInRoot). + +## Grounds + +- pursued: a store swapped for a symlink after openStore leaves the index write inside the vetted directory, pinned by TestCoverageLintWritesOnlyThroughTheStoreHandle; a coverage-index write by path reappearing, or the test passing with the write reverted, would show it wrong diff --git a/.abcd/work/issues/resolved/iss-2609260908572219-the-kept-original-is-written-by-path-after-the-store-lock-lapses.md b/.abcd/work/issues/resolved/iss-2609260908572219-the-kept-original-is-written-by-path-after-the-store-lock-lapses.md new file mode 100644 index 000000000..240f172cf --- /dev/null +++ b/.abcd/work/issues/resolved/iss-2609260908572219-the-kept-original-is-written-by-path-after-the-store-lock-lapses.md @@ -0,0 +1,22 @@ +--- +schema_version: 1 +id: "iss-2609260908572219" +slug: "the-kept-original-is-written-by-path-after-the-store-lock-lapses" +severity: "minor" +category: "security" +source: "review-followup" +found_during: "autonomous run A resumed 2026-09-25" +origin: researcher-authored +production_mode: hand-written +found_at: "internal/core/memory/ingest.go" +resolution: "Ingest writes the kept original through a store handle it opens after WritePages returns: the open re-runs the segment walk, root.Lstat refuses a symlinked sources/, and fsutil.WriteFileAtomicInRoot writes sources/ inside the directory the handle opened, on the full ingest and the registry-hit fast path alike. The kept path is reported only while the store path still names that directory." +impact: fix +resolved_by: + commit: "27e45c54" +--- + +Ingest --keep-original writes the kept original by path after every vetting has lapsed: storeOriginal Lstats the leaf sources directory and calls fsutil.WriteFileAtomic on /.abcd/memory/sources/ after WritePages has released the store lock and its validatedMemoryDir walk, and the store handle Ingest opened is not used for it. The Lstat guard binds the leaf, not an ancestor, so a .abcd/memory swapped for a directory symlink between WritePages and storeOriginal lands redacted source material outside the repository while Ingest reports status=ingested with kept naming the in-repo path the file is not at. Neither the store-handle record iss-2608291814572914 (reads, plus the locked writer as the residual) nor iss-2609252100150846 (the coverage index) covers it. Fix: after WritePages, open a store handle and write sources/ through fsutil.WriteFileAtomicInRoot, keeping the symlink refusal as root.Lstat("sources"). + +## Grounds + +- pursued: a store swapped for a symlink between WritePages and the kept-original write, or after its handle opened, puts nothing outside the repository and reports no kept path, pinned by TestKeepOriginalIsNotWrittenThroughASwappedStore, TestKeepOriginalOnTheFastPathIsNotWrittenThroughASwappedStore and TestKeepOriginalRefusesAStoreSwappedAfterItsHandleOpened; a sources/ write by path reappearing in ingest.go, or those tests passing with it, would show it wrong diff --git a/.abcd/work/issues/open/iss-304-confirmed-nitpicks-deferred-from-bughunt-round-1-recorded-no.md b/.abcd/work/issues/resolved/iss-304-confirmed-nitpicks-deferred-from-bughunt-round-1-recorded-no.md similarity index 77% rename from .abcd/work/issues/open/iss-304-confirmed-nitpicks-deferred-from-bughunt-round-1-recorded-no.md rename to .abcd/work/issues/resolved/iss-304-confirmed-nitpicks-deferred-from-bughunt-round-1-recorded-no.md index 45e3b5838..fa0bd9139 100644 --- a/.abcd/work/issues/open/iss-304-confirmed-nitpicks-deferred-from-bughunt-round-1-recorded-no.md +++ b/.abcd/work/issues/resolved/iss-304-confirmed-nitpicks-deferred-from-bughunt-round-1-recorded-no.md @@ -7,6 +7,10 @@ category: "tech-debt" source: "agent-finding" found_during: "bughunt-round-1" found_at: "internal/core/memory/ingest.go" +resolution: "b-4: the generated CLI reference now lists Cobra's completion and help. a-4 was already closed at tip: materialFromLocal reads through fsutil.ReadGuarded, which caps the bytes actually read. d-12 (the record-lint job id) needs a coordinated live ruleset edit and continues as iss-2609251358062952." +impact: fix +resolved_by: + commit: "cb2c4c95" --- Confirmed nitpicks deferred from bughunt round 1 (recorded not fixed): (a-4) memory ingest --source reads its operand with os.Stat-then-unbounded os.ReadFile, a size TOCTOU the URL branch's io.LimitReader avoids; (b-4) the generated CLI reference claims to list every user-facing command but omits Cobra's completion/help commands; (d-12) the CI job id 'record-lint' actually runs scripts/check-reviews.sh (reviews-charter), a misleading required-check name whose safe rename needs a coordinated live-ruleset edit @@ -28,3 +32,7 @@ Confirmed nitpicks deferred from bughunt round 1 (recorded not fixed): (a-4) mem Each CONFIRMED (nitpick) by an independent refuter. Recorded not fixed this round: a-4/b-4 are below the fix bar for a substantive round, and d-12's rename cannot be applied without a coordinated live branch-ruleset change (unsafe autonomously). + +## Grounds + +- pursued: the reference lists abcd completion and abcd help and a local ingest source is read under the byte cap; a command the binary answers to missing from the reference would show it wrong diff --git a/commands/history.md b/commands/history.md index f35ededa3..2d5cf80a0 100644 --- a/commands/history.md +++ b/commands/history.md @@ -1,7 +1,7 @@ --- name: history description: "Keep session transcripts in the user-level store and read them back: Writes nothing bare, and redacts each one it stores; refuses an unknown sub-verb." -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 " block: agents --- @@ -189,6 +189,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 d345fbe2d..d980ef0b6 100644 --- a/docs/reference/cli/commands.md +++ b/docs/reference/cli/commands.md @@ -307,6 +307,128 @@ Preview the next release cut's version, records, and guardrail verdict: Writes n **Usage:** `abcd changelog` +### `abcd completion` + +Generate the autocompletion script for the specified shell + +**Usage:** `abcd completion` + +Generate the autocompletion script for abcd for the specified shell. +See each sub-command's help for details on how to use the generated script. + +#### `abcd completion bash` + +Generate the autocompletion script for bash + +**Usage:** `abcd completion bash` + +Generate the autocompletion script for the bash shell. + +This script depends on the 'bash-completion' package. +If it is not installed already, you can install it via your OS's package manager. + +To load completions in your current shell session: + + source <(abcd completion bash) + +To load completions for every new session, execute once: + +#### Linux: + + abcd completion bash > /etc/bash_completion.d/abcd + +#### macOS: + + abcd completion bash > $(brew --prefix)/etc/bash_completion.d/abcd + +You will need to start a new shell for this setup to take effect. + +**Flags:** + +``` + --no-descriptions disable completion descriptions +``` + +#### `abcd completion fish` + +Generate the autocompletion script for fish + +**Usage:** `abcd completion fish [flags]` + +Generate the autocompletion script for the fish shell. + +To load completions in your current shell session: + + abcd completion fish | source + +To load completions for every new session, execute once: + + abcd completion fish > ~/.config/fish/completions/abcd.fish + +You will need to start a new shell for this setup to take effect. + +**Flags:** + +``` + --no-descriptions disable completion descriptions +``` + +#### `abcd completion powershell` + +Generate the autocompletion script for powershell + +**Usage:** `abcd completion powershell [flags]` + +Generate the autocompletion script for powershell. + +To load completions in your current shell session: + + abcd completion powershell | Out-String | Invoke-Expression + +To load completions for every new session, add the output of the above command +to your powershell profile. + +**Flags:** + +``` + --no-descriptions disable completion descriptions +``` + +#### `abcd completion zsh` + +Generate the autocompletion script for zsh + +**Usage:** `abcd completion zsh [flags]` + +Generate the autocompletion script for the zsh shell. + +If shell completion is not already enabled in your environment you will need +to enable it. You can execute the following once: + + echo "autoload -U compinit; compinit" >> ~/.zshrc + +To load completions in your current shell session: + + source <(abcd completion zsh) + +To load completions for every new session, execute once: + +#### Linux: + + abcd completion zsh > "${fpath[1]}/_abcd" + +#### macOS: + + abcd completion zsh > $(brew --prefix)/share/zsh/site-functions/_abcd + +You will need to start a new shell for this setup to take effect. + +**Flags:** + +``` + --no-descriptions disable completion descriptions +``` + ### `abcd decide` Mint an ADR id and lay the record's empty skeleton: Writes one proposed record into the decisions store; refuses a missing or unusable title. @@ -650,6 +772,15 @@ exists. A workdir that is not a string, or holds a NUL byte, a control character or invalid UTF-8, or is over 4096 bytes, is refused with the blocking status and the reason. +### `abcd help` + +Help about any command + +**Usage:** `abcd help [command]` + +Help provides help for any command in the application. +Simply type abcd help [path to command] for full details. + ### `abcd history` Keep session transcripts in the user-level store and read them back: Writes nothing bare, and redacts each one it stores; refuses an unknown sub-verb. @@ -658,13 +789,14 @@ Keep session transcripts in the user-level store and read them back: Writes noth #### `abcd history capture` -Redact and store one raw session transcript from a file or stdin: Writes one record into the store; refuses stdin without --session. +Redact and store a session transcript, or a whole session with --all: Writes one record per transcript; refuses stdin or --all without --session. -**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/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/history/ingest.go b/internal/core/history/ingest.go index 5694b4b37..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) } @@ -275,15 +289,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 +597,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 +611,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 +631,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/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()) + } + } + } +} 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) + } +} 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..018be61dc --- /dev/null +++ b/internal/core/history/staging_shard_test.go @@ -0,0 +1,200 @@ +package history + +import ( + "os" + "path/filepath" + "strconv" + "sync" + "sync/atomic" + "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()) + } +} + +// TestStageWaitsOutARetiredLock holds, at the staging layer, the guarantee the +// per-key lock files lean on. A mutator that retires a staged file unlinks its +// lock while holding it (retireStagingLock), so a Stage queued on that lock +// file before the unlink is granted the flock on an orphaned inode once the +// retirer releases. It must notice and wait for whoever holds the fresh lock +// file at the same path, not run its critical section beside them. Only +// fsutil's inode revalidation makes that true; without it this Stage returns +// while the newcomer is still inside. +func TestStageWaitsOutARetiredLock(t *testing.T) { + repoRoot, home := setupStore(t) + const agent = "agent-retire" + meta := subAgentStage("sess-rt", agent) + if _, err := Stage(repoRoot, testRootSHA, meta, []byte("first\n")); err != nil { + t.Fatal(err) + } + sdir := stagingDir(home) + + holderIn := make(chan struct{}) + retired := make(chan struct{}) + newcomerIn := make(chan struct{}) + var newcomerInside atomic.Bool + overlapped := make(chan bool, 1) + var wg sync.WaitGroup + + wg.Add(1) + go func() { // A: a drain-shaped holder that retires the lock it holds + defer wg.Done() + if err := withStagingLock(sdir, agent, func() error { + close(holderIn) + time.Sleep(80 * time.Millisecond) // B opens the lock file and polls + retireStagingLock(sdir, agent) + close(retired) + <-newcomerIn + return nil + }); err != nil { + t.Error(err) + } + }() + <-holderIn + + wg.Add(1) + go func() { // B: a re-fired Stage queued on the lock file before it was retired + defer wg.Done() + _, err := Stage(repoRoot, testRootSHA, meta, []byte("second\n")) + overlapped <- newcomerInside.Load() + if err != nil { + t.Error(err) + } + }() + + <-retired + wg.Add(1) + go func() { // C: takes the fresh lock file at the same path and holds it + defer wg.Done() + if err := withStagingLock(sdir, agent, func() error { + newcomerInside.Store(true) + close(newcomerIn) + time.Sleep(300 * time.Millisecond) + newcomerInside.Store(false) + return nil + }); err != nil { + t.Error(err) + } + }() + wg.Wait() + if <-overlapped { + t.Fatal("a Stage queued on a retired lock file ran while a newcomer held the fresh one: two holders of one staging lock") + } +} diff --git a/internal/core/memory/ask.go b/internal/core/memory/ask.go index 68cfffb35..835f77305 100644 --- a/internal/core/memory/ask.go +++ b/internal/core/memory/ask.go @@ -4,14 +4,12 @@ import ( "bytes" "encoding/json" "fmt" - "os" - "path/filepath" + "math" "regexp" "sort" "strings" "time" - "github.com/intentdriven/abcd/internal/fsutil" "github.com/intentdriven/abcd/internal/termsafe" ) @@ -201,37 +199,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 +247,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, @@ -295,11 +279,32 @@ func QueryPages(repoRoot, question string, topN int) ([]MatchedPage, error) { // be told they invoked a plugin command they may not even have installed. const AskReportHeading = "abcd memory ask" +// citationTruncatedMarker ends a citation JSON cut at the page-value cap. The +// cap cuts mid-JSON, so the cut is said out loud rather than left to read as +// a whole value. It is plain text: nothing in it opens markdown or HTML. +const citationTruncatedMarker = " … (citation truncated)" + +// cleanCitationJSON is cleanPageField for the compacted citation JSON with the +// cut made visible: a value that cleans within the cap renders whole, and one +// that does not is cut short enough for the marker to fit and ends with it, so +// the field stays within the cap either way. The marker follows CleanProse's +// output, whose final span-aware pass leaves no span or escape open for it to +// fall into. +func cleanCitationJSON(raw string) string { + if whole := termsafe.CleanProse(raw, math.MaxInt); len(whole) <= maxPageValueBytes { + return whole + } + return termsafe.CleanProse(raw, maxPageValueBytes-len(citationTruncatedMarker)) + citationTruncatedMarker +} + // RenderCitedMatches is the default deterministic synthesizer — a // 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)), "", @@ -308,11 +313,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 @@ -322,17 +329,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 = cleanCitationJSON(compactJSONSorted(c.Citation)) } lines = append(lines, fmt.Sprintf(" - cites: class=%s | source_hash=%s | citation=%s", cls, sh, cj)) } @@ -340,12 +347,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" } @@ -409,14 +415,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/ask_termsafe_test.go b/internal/core/memory/ask_termsafe_test.go index dc69e880f..31b6d1e39 100644 --- a/internal/core/memory/ask_termsafe_test.go +++ b/internal/core/memory/ask_termsafe_test.go @@ -53,3 +53,47 @@ func TestRenderCitedMatchesSanitizesCitationFields(t *testing.T) { t.Fatalf("gh-250: sanitising must not drop the legitimate citation content:\n%s", out) } } + +// TestRenderCitedMatchesMarksATruncatedCitation: the citation JSON is capped at +// the page-value cap like every other field, and a cap cuts it mid-JSON. The +// cut must be visible — an explicit marker at the end, the whole field still +// inside the cap — so a reader never takes a cut citation for the whole one. +// A citation under the cap carries no marker. +func TestRenderCitedMatchesMarksATruncatedCitation(t *testing.T) { + render := func(title string) string { + t.Helper() + out := RenderCitedMatches("what tokens", []MatchedPage{{ + Filename: "topic_auth_tokens.md", + Score: 1, + Summary: "summary", + Citations: []AskCitation{{ + SourceClass: "knowledge", + SourceHash: strings.Repeat("a", 64), + Citation: map[string]any{"title": title, "type": "knowledge"}, + }}, + }}) + for _, line := range strings.Split(out, "\n") { + if _, cj, ok := strings.Cut(line, "| citation="); ok { + return cj + } + } + t.Fatalf("no citation line in:\n%s", out) + return "" + } + + long := render(strings.Repeat("x", maxPageValueBytes+500)) + if !strings.HasSuffix(long, citationTruncatedMarker) { + t.Fatalf("an over-cap citation renders cut with no marker; it ends %q", long[len(long)-40:]) + } + if len(long) > maxPageValueBytes { + t.Fatalf("the marked citation is %d bytes, over the %d-byte cap", len(long), maxPageValueBytes) + } + + short := render("Token rotation") + if strings.Contains(short, citationTruncatedMarker) { + t.Fatalf("a citation under the cap carries the truncation marker: %q", short) + } + if !strings.HasSuffix(short, "}") { + t.Fatalf("a citation under the cap must render whole: %q", short) + } +} diff --git a/internal/core/memory/bare.go b/internal/core/memory/bare.go index d1dfcff0c..370f1b21a 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(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(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,25 +178,12 @@ 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 } - budget := loadQuotationBudget(repoRoot) + budget := loadQuotationBudget(store) var referenced []string for _, p := range pages { block := coveragePageSourceBlock(p.text) @@ -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/class_shape_union_test.go b/internal/core/memory/class_shape_union_test.go index 9f4f85b41..1c97fda68 100644 --- a/internal/core/memory/class_shape_union_test.go +++ b/internal/core/memory/class_shape_union_test.go @@ -47,7 +47,7 @@ func TestDerivedClassesUnionsBothShapes(t *testing.T) { // Unit-level: the union is deduplicated, and a class named by both shapes // appears once. - l := newMemoryLinter("p.md", t.TempDir(), + l := newMemoryLinter("p.md", &storeHandle{dir: Dir(t.TempDir())}, "---\nsource:\n class: external_pdf\n classes: [external_pdf, session_memory]\n---\n# Body\n", nil) got := l.derivedClasses() want := []string{"external_pdf", "session_memory"} 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`