feat(recall): extraction manager, scheduler, and CLI

[wesm]

Builds on #1159's extraction store and client with the manager layer that drives model-backed recall extraction end to end.

Manager (internal/recall/extract)

• ExtractCandidates selects eligible sessions in SQL: ended past the quiet period, not automated, not trashed, zero secret findings, scanned under the current rules versions, and non-empty. The privacy predicates are not configurable, and the same checks guard explicit single-session runs so no path can feed an excluded session to the model. Failed sessions retry behind an updated_at-indexed backoff arm.
• Manager.RunPass distills each candidate unit by unit, checkpointing a resumable cursor after every unit. Unit output commits under an in-transaction guard that re-verifies the session snapshot, eligibility, and absence of secret findings atomically with the insert; every Go-side check is advisory over that guard.
• Failures mark the session for retry after a per-session backoff instead of aborting the pass — except endpoint-scoped failures (401/403/404/405/415/501, refused redirects, schema-violating responses), which abort the pass with rows left pending so a broken endpoint cannot burn one doomed model call and a backoff per session. Failure transitions never advance the coverage stamp outside the transaction that records the failure.
• Units the model rejects as too large (context-overflow 400 or HTTP 413) or cannot answer completely (persistent truncation) are halved recursively down to the split floor.
• Model responses are bounded locally (entry count, field lengths, mirrored as maxItems/maxLength in the request schema) and persisted error text is capped, so a hostile endpoint cannot balloon the archive.
• Entry ids are deterministic (sha256 of generation fingerprint, session, unit, entry position), and the bulk insert skips existing ids, so replays after crashes or digest resets dedupe instead of duplicating.
• Entries carry evidence rows with the unit's message-ordinal range, session context (project, cwd, branch, agent), the generation fingerprint as source_run_id, and unreviewed_auto review state. While a generation is building, entries stage as archived and never serve.
• Privacy retraction runs on every scheduled pass, before any model work (so extraction failures cannot defer it) and again after the loop: sessions since trashed, flagged automated, or carrying findings get their generated entries deleted across all generations and their progress rows removed.

Activation

A generation auto-activates once everything eligible is done and it has produced entries; explicit Activate refuses an empty generation. The activation transaction re-verifies coverage — no eligible session pending/partial, unextracted, or with coverage stamped before its latest transcript write or under superseded scan rules — and aborts with a typed error rather than retiring the served corpus around a gap. It clears the staged output and progress of any session no longer fully eligible (trashed, reopened, awaiting rescan, or gone) so nothing stale serves and nothing strands archived, then promotes the rest atomically while retiring the previous generation.

Credential handling

Endpoint URLs can carry credentials in userinfo, query values, bare query tokens, fragments, and path segments. config.RedactedEndpoint masks all of these fail-closed (only api-version and known API-surface path vocabulary stay visible) for every error, log line, and stored failure row. Response bodies are attacker-influenced and can reflect the request: when the endpoint URL carries any credential material, all endpoint-provided diagnostic detail is withheld rather than scrubbed (re-encodings defeat literal replacement); credential-free endpoints keep a control-stripped, length-capped excerpt. Redirects are never followed — a redirect would replay transcript content to an attacker-chosen destination — and a refused redirect aborts the pass.

Secrets integration

Transcript mutations revoke the session's secret-scan stamp in-write (rulesAlgorithmVersion 7), so appended content cannot ride an older scan's approval. The manager additionally rescans outbound text against the full ruleset before sending and fails closed on any match despite a current stamp.

Config ([recall.extract])

Model identity (model, deployment), named servers (transport only — moving a deployment to a new address does not orphan the corpus), quiet_period, backstop_interval, failure_backoff, max_window_chars, max_tokens, prompt profile/override-dir selection, and request-shape overrides. Validated at load, and the resolved request shape is validated at manager construction, so a bad profile fails setup before any progress rows exist. Disabled section stays inert.

Daemon scheduler

Mirrors the embed scheduler: sync completions debounce into incremental passes; backstop ticks run full passes that revisit done sessions so grown transcripts are topped up via content-digest reset; with the backstop disabled a catchup ticker keeps incremental passes running. Every daemon lifetime starts with a full pass (deferred work survives daemon restarts), the pending startup pass and every running pass hold an idle-work lease so a detached daemon neither reaps itself mid-pass nor before its first pass, and no pass starts once the daemon is draining. Session-mutating server routes (trash, restore, delete, empty-trash, secret scan) notify the scheduler so eligibility changes are picked up without waiting for sync activity.

CLI

recall extract becomes a parent command:

• run [--session <id>] [--full] [--limit N] — one pass; --session bypasses the quiet period but never the privacy filters
• status — coverage per state, unit progress, entry count, generation list
• activate / retire <fingerprint> [--force] — with the store's refusal guards
• doctor — prints the resolved model/server/profile/fingerprint and makes one probe call whose deadline derives from the configured server timeout
• preview --session <id> — the previous --dry-run chunk preview; the legacy extract --session --dry-run flags still work as a silent fallback

Manual write commands refuse while a daemon owns the archive, since an enabled daemon runs passes itself.

Design notes live in docs/internal/recall-extraction.md.

Note: TestDoSyncConfiguredFullUnifiedHTTPUsesManifestDeltaAndOrderedProgress fails on this machine on clean origin/main as well; unrelated to this change.

🤖 Generated with Claude Code

[roborev-ci]

roborev: Combined Review (2cce9ad)

Extraction has two high-severity safety/correctness flaws and three medium-severity lifecycle and scalability issues.

High

• Stale or missing secret scans are treated as clean — internal/db/recall_extract.go:608, internal/recall/extract/manager.go:261
  Extraction checks only secret_leak_count == 0, without confirming that secrets_rules_version represents a current, completed scan. Unscanned or stale sessions may therefore send detectable credentials to the configured model endpoint. Require the current scan version, rescan or reject stale sessions, and apply this protection to batch and explicit-session extraction. Ideally, scan the complete message snapshot immediately before the first model request.

• Changed transcript units retain stale extracted entries — internal/recall/extract/manager.go:138
  Entry IDs omit the unit/content digest. After transcript changes reset progress, re-extracted entries at existing positions collide and are skipped, while obsolete entries remain. Atomically reconcile entries when a digest changes by replacing or archiving prior entries, with coverage for modified or expanded assistant units.

Medium

• Scheduled extraction work scales with the entire archive — internal/recall/extract/manager.go:238
  Every sync performs an archive-wide candidate query, and hourly passes reload every completed transcript to recompute digests. Propagate changed session IDs or versions, use bounded batches, and restrict backstop passes to sessions changed since extraction. Add small-versus-large archive scaling coverage.

• Inherited --server is silently ignored — cmd/agentsview/recall_extract.go:178
run, status, activate, retire, and doctor may operate on the local archive even when the user explicitly targets a remote daemon. Route these commands through remote endpoints or explicitly reject --server until supported.

• An empty generation can replace the active generation — internal/recall/extract/manager.go:450
  Activation checks completed sessions rather than produced entries, so valid zero-entry responses can activate an empty generation and retire the previous one. Require stats.Entries > 0 for automatic and manual activation, with zero-entry response tests.

---

Reviewers: 2 done | Synthesis: codex, 15s | Total: 13m56s

[wesm]

5fc6aba addresses the lint failure and all five review findings:

• Stale/missing secret scans — eligibility now requires secrets_rules_version to be one of secrets.ActiveRulesVersions() in both the SQL candidates query and the explicit single-session check. The candidates query errors if called without scan versions, so the boundary fails closed; unscanned sessions (leak count 0, no recorded scan) are excluded until secrets scan --backfill covers them.
• Stale entries after transcript changes — a content-digest change now rebuilds the session's generated corpus: unreviewed_auto entries under that generation+session are deleted before re-extraction, so changed units neither keep stale entries nor collide with replacements. Human-touched entries are preserved. Covered by a test where a grown assistant run re-packs into an existing unit.
• Archive-wide full passes — full passes now revisit only done sessions with local_modified_at > progress.updated_at (every transcript write stamps local_modified_at), so backstop ticks no longer reload unchanged transcripts. Incremental scans remain one indexed query.
• --server silently ignored — run/status/activate/retire/doctor now reject --server explicitly; preview keeps working over the service layer as before.
• Empty generation activation — both auto-activation and explicit activate now require at least one extracted entry, so a valid-but-empty generation can never replace the active corpus.
• nilaway — the nil flow from GetSession is checked explicitly at both call sites; make nilaway is clean.

Also adds documentation: docs/internal/recall-extraction.md (design contracts), an Automatic extraction section in docs/recall.md, and updated docs/commands.md/docs/configuration.md.

The e2e failure was a webkit-only session-timing.spec.ts chevron flake unrelated to this backend-only diff; this push re-runs it.

🤖 Generated with Claude Code

[roborev-ci]

roborev: Combined Review (5fc6aba)

Verdict: Changes requested — two high-severity privacy flaws could expose credentials to the configured model, with three additional lifecycle and scheduling issues.

High

• Candidate credentials bypass the extraction privacy gate — internal/db/recall_extract.go:633, internal/recall/extract/manager.go:247

  Eligibility relies on secret_leak_count = 0 and permits DefiniteRulesVersion, but candidate rules are excluded. JWT bearer tokens, basic-auth passwords, and high-entropy assignments can therefore be sent verbatim to the model.

  Fix: Require a full secrets.RulesVersion() scan and reject any session with current secret_findings, regardless of confidence. Apply the same validation to explicit single-session extraction.

• Secret-scan approval is not bound to the transcript sent — internal/recall/extract/manager.go:301, internal/recall/extract/manager.go:311

  Eligibility metadata and messages are loaded separately, while transcript mutations do not atomically invalidate the clean scan stamp. Newly appended secret-bearing messages can consequently be approved using stale metadata and sent to the model.

  Fix: Bind scan state to transcript_revision, invalidate it atomically on every transcript mutation, and require the scanned revision to match the message snapshot before sending. Load both from one consistent transaction or rescan the exact snapshot.

Medium

• Concurrent transcript changes can be permanently skipped — internal/db/recall_extract.go:643

  Detection uses local_modified_at > progress.updated_at. Cursor updates during extraction can overtake the source timestamp, permanently hiding changes; equal millisecond timestamps have the same problem.

  Fix: Persist and compare the extracted transcript_revision, then recheck it before marking extraction complete.

• Generation activation and retirement do not control serving — internal/recall/extract/manager.go:444

  Extracted entries are immediately stored as accepted, and Recall queries do not consult generation state. Building and retired generations therefore remain queryable together.

  Fix: Restrict queries to the active generation, or stage entries as non-served and atomically switch their status during activation.

• Disabling the backstop can prevent eventual extraction — cmd/agentsview/extract_scheduler.go:41

  The only post-sync pass runs after 30 seconds, before the default 30-minute quiet period expires. It skips newly ended sessions and never retries without unrelated sync activity.

  Fix: Schedule a follow-up at the earliest quiet-period expiration, or require a positive periodic backstop.

---

Reviewers: 2 done | Synthesis: codex, 19s | Total: 15m21s

[wesm]

Review round 2 addressed in 6570c21 — all five findings were real.

Candidate credentials bypass the privacy gate (high). Eligibility now requires the last scan to be the current full secrets.RulesVersion() — the definite-only inline sync version no longer qualifies — and any recorded row in secret_findings, regardless of confidence, excludes the session. Both enforced in the candidates SQL (NOT EXISTS over secret_findings) and in the explicit --session path (refuseSecretFindings, currentScanVersion). Practical consequence, now documented: sessions need secrets scan --backfill before they become eligible.

Scan approval not bound to the transcript sent (high). The manager brackets GetAllMessages with before/after session reads and compares message count, transcript revision, scan version, leak count, ended-at, and last local write (sessionSnapshotChanged). Sync writes messages, scan stamps, and counts in one transaction, so a stable bracket means the loaded transcript is the one the eligibility check approved; a mismatch skips silently and the next pass retries.

Mid-extraction writes permanently skipped (medium). New recall_extract_progress.content_stamped_at column records when the unit digest was derived; it moves only on insert and digest reset, never on cursor advances. The done-revisit gate is now local_modified_at >= content_stamped_at instead of > updated_at, so a write landing between unit derivation and the final cursor advance re-opens the session. Empty stamps (rows copied from pre-stamp archives) self-heal on the next same-digest upsert.

Activation/retirement don't control serving (medium). Entries extracted under a building generation are staged with status=archived; ActivateExtractGeneration promotes them to accepted and archives the retired generation's still-automatic entries in the same transaction, and RetireExtractGeneration (now transactional) archives its entries. Human-touched entries (review_state != 'unreviewed_auto') are never moved.

Disabled backstop prevents eventual extraction (medium). With backstop_interval disabled the scheduler now runs a catchup ticker of incremental passes, paced by the quiet period and floored at one minute, so a session whose quiet period elapses after the last sync still gets scanned.

Each fix landed test-first: new coverage in internal/db/recall_extract_test.go (any-confidence exclusion, content-stamp gate, stamp healing, activate/retire entry switching), internal/recall/extract/manager_test.go (definite-only refusal with backfill hint, candidate-finding refusal, snapshot comparator, staged-until-activation), and cmd/agentsview/extract_scheduler_test.go (catchup ticks, backstop supersedes catchup).

[roborev-ci]

roborev: Combined Review (6570c21)

The PR needs changes for three medium-severity correctness and scalability issues.

Medium

• internal/recall/extract/manager.go:360 — Snapshot guard misses concurrent changes. Both snapshots use GetSession, but sessionBaseCols does not load LocalModifiedAt, leaving it nil in both reads. Concurrent scans or metadata updates can therefore go undetected, potentially approving stale context or changed secret findings. Use GetSessionFull or a dedicated snapshot query including local_modified_at, and recheck secret findings after loading messages.

• internal/db/recall_extract.go:682 — Background extraction scales with the full archive. Every debounced sync and periodic pass queries the entire sessions/progress population, even when no sessions are eligible. Track changed session IDs or maintain an indexed work queue/watermark, reserve full reconciliation for recovery, and add a small-versus-large archive scaling regression.

• cmd/agentsview/recall_extract.go:162 — Manual extraction bypasses the writer lock. Opening the database directly with db.Open allows the multi-step extraction pass to overlap another offline writer or a full resync, risking writes being lost during a database swap. Use openWriteDB and hold the returned writeOwnerLock for the command’s lifetime.

---

Reviewers: 2 done | Synthesis: codex, 10s | Total: 20m6s

[wesm]

Review round 3 addressed in 8a3ff5d — all three findings were real.

Snapshot guard misses concurrent changes (medium). Confirmed: GetSession's column list omits local_modified_at, so both bracket reads carried nil and the comparison was blind to metadata-only writes (e.g. a findings replace under an unchanged rules version and leak count). The bracket reads now go through sessionSnapshot, which uses GetSessionFull and loads the full column set. A rescan that records findings also re-stamps local_modified_at in the same transaction as the session-row update, so the bracket now catches changed secret findings without a separate post-load findings query. Regression test: TestManagerSnapshotReadSeesMetadataOnlyWrites performs exactly the metadata-only write and asserts the snapshot comparison detects it.

Background extraction scales with the full archive (medium). The candidates query is now a union of two indexed arms: a discovery arm over sessions with no progress row, bounded by a new ChangedSince watermark against local_modified_at (new idx_sessions_local_modified), and a backlog arm over this generation's progress rows by state (existing state index). The manager watermarks incremental passes at the last completed unlimited pass minus the quiet period — the lag covers sessions that become eligible purely by time passing, since eligibility begins one quiet period after a session's final write. Explicit and limited passes never advance the watermark, and full backstop passes ignore it entirely, keeping them as the recovery path that reconciles anything a bounded scan missed. Sessions with a NULL local_modified_at remain discoverable. Scaling regression: TestExtractCandidatesChangedSinceAvoidsSessionScan runs EXPLAIN QUERY PLAN over the built query and fails if any plan step walks the whole sessions table; behavioral coverage in TestExtractCandidatesChangedSinceLimitsDiscovery / ...KeepsProgressBacklog and TestManagerWatermark*.

Manual extraction bypasses the writer lock (medium). openWritableExtractDB now delegates to openWriteDB after its extraction-specific transport refusals, returning the offline writer lock, which run/activate/retire hold for the command's lifetime. Regression test: TestRecallExtractRunRefusesWhileOfflineWriterHoldsLock.

All fixes landed test-first; suites, golangci-lint, and nilaway are clean apart from the pre-existing unrelated TestDoSyncConfiguredFullUnifiedHTTPUsesManifestDeltaAndOrderedProgress failure noted earlier.

[roborev-ci]

roborev: Combined Review (8a3ff5d)

High-severity issues could expose transcripts; three medium-severity correctness and scalability issues also need attention.

High

• Secret-scan freshness is not bound to the transcript revision
  Locations: internal/db/recall_extract.go:660, internal/recall/extract/manager.go:348
  Eligibility checks only that secrets_rules_version is current. Transcript writes advance transcript_revision without atomically invalidating the prior scan, allowing extraction to send newly appended credentials before a successful rescan.
  Fix: Persist the transcript revision covered by each secret scan, require it to match the current revision in every eligibility path, and atomically invalidate scan freshness on transcript mutation.

• Remote HTTP model endpoints expose transcripts in cleartext
  Location: internal/config/recall.go:197
  Arbitrary plaintext HTTP endpoints are permitted, so transcripts may be intercepted or modified in transit.
  Fix: Require HTTPS for non-loopback endpoints. If remote HTTP support is necessary, gate it behind an explicit insecure-transport option and reject HTTPS-to-HTTP redirects.

Medium

• Snapshot stamping can cause permanent full-history reloads
  Location: internal/db/recall_extract.go:331
content_stamped_at is written after the snapshot bracket but retained when the digest is unchanged. A concurrent update can be missed, while later metadata or secret-scan changes can leave the session eligible for full-history reloading on every pass.
  Fix: Capture a cutoff before reading the snapshot and advance it after every stable digest recheck, including unchanged digests.

• Hourly discovery scales with the complete session archive
  Location: internal/recall/extract/manager.go:277
  Each default hourly full pass clears ChangedSince, forcing a scan of all ended sessions rather than bounding work to changed records.
  Fix: Use a persisted watermark or indexed cursor for background discovery, reserving unbounded reconciliation for explicit maintenance.

• Activation and status checks ignore changed completed sessions
  Location: internal/recall/extract/manager.go:610
  Omitting IncludeDone treats completed sessions with changed transcripts as covered, allowing stale generation activation and incorrect no-backlog status.
  Fix: Include changed done rows in activation-readiness and backlog checks.

---

Reviewers: 2 done | Synthesis: codex, 17s | Total: 24m27s

[wesm]

Review round 4 addressed in fe18bf6 — both highs and all three mediums were real, with one nuance on the first high.

Scan freshness not bound to the transcript (high). Verified the exact window: the incremental sync path writes the session row, then the messages, then findings, then signals as separate writes, so appended content could briefly coexist with a still-current full-scan stamp. Fixed at the storage layer with the reviewer's suggested invalidation: bumpTranscriptRevisionTx — which already runs inside every transcript-mutation transaction (append, replace, diff, tool-call relink) — now clears secrets_rules_version in that same transaction, so any mutation atomically revokes scan freshness and extraction fails closed until a rescan re-stamps (the inline rescan restores only the definite-only version, which extraction already rejects; the atomic ReplaceSessionContent path re-stamps in-transaction). The manager additionally requires the loaded message count to equal the session row's, closing the mirror-image window where the row is written before the transcript. Ran the full sync suite against the merge-base and this change — identical results (the handful of local messages_fts failures predate the PR).

Cleartext HTTP endpoints (high). Validate now rejects plaintext HTTP to non-loopback hosts; a server entry can set allow_http = true to accept the risk explicitly. config.ValidateExtractTransport is shared with the model client's CheckRedirect, so a compliant endpoint cannot be redirected to non-compliant transport mid-request. Documented in docs/recall.md.

Stamp timing (medium). UpsertExtractProgress now takes the caller's transcript-read cutoff (captured before the first snapshot read) and stamps it on every stable upsert, same-digest revisits included — a zero cutoff is refused. This closes both directions: a write landing mid-derivation compares as after the stamp and re-opens the session, and a revisited-but-unchanged session settles its stamp forward instead of re-opening on every full pass.

Hourly full-pass scaling (medium). Full passes now bound discovery by the same ratcheted watermark; they differ from incremental passes only in revisiting changed done sessions, which the progress-state index bounds. Unbounded reconciliation is the fresh-manager path — a daemon restart or a manual recall extract run starts at a zero watermark and scans everything once.

Activation/status ignore changed done sessions (medium). Both maybeActivate and Status backlog queries now pass IncludeDone, so a completed session whose transcript changed since its unit snapshot blocks activation and shows in the backlog until re-extracted. Failed sessions still don't block activation.

All fixes landed test-first (TestTranscriptMutationInvalidatesSecretScanFreshness, TestReplaceSessionContentEndsScanStamped, TestManagerSkipsTranscriptOutOfStepWithSessionRow, transport validation + redirect cases, TestUpsertExtractProgressStampsCallerCutoff, TestManagerWatermarkLimitsScanDiscovery, TestManagerChangedDoneSessionBlocksActivation); lint and nilaway clean.

[roborev-ci]

roborev: Combined Review (fe18bf6)

Changes need revision: four medium-severity issues could cause unbounded archive scans, unnecessary reprocessing, missed retries, or extraction of ineligible sessions.

Medium

• internal/db/recall_extract.go:745 — Every full/backstop pass scans all completed progress rows because the changed-content predicate compares against a joined session timestamp. Work scales with the entire archive instead of the changed batch. Drive reconciliation from an indexed sessions.local_modified_at watermark, join matching progress rows, and add a small-versus-large archive scaling regression.

• internal/db/recall_extract.go:624 — Full-resync copying omits content_stamped_at, leaving restored progress rows with empty stamps. The next full pass consequently treats every completed session as changed and reprocesses the entire archive. Preserve the field when available, provide compatibility for older archives, and test progress preservation across resync.

• internal/recall/extract/manager.go:436, internal/recall/extract/manager.go:238 — A message-count mismatch produces an empty outcome while the pass still advances its discovery watermark. Persistent incomplete transcripts can therefore be excluded from future automatic passes. Persist a retryable state or prevent watermark advancement for candidates skipped due to incomplete transcripts.

• internal/recall/extract/manager.go:362 — The second snapshot comparison omits eligibility fields such as IsAutomated and DeletedAt, and eligibility is not rechecked. A concurrent update can make a session ineligible without changing compared fields, allowing it to be extracted. Revalidate eligibility against the second snapshot or include all eligibility-relevant fields in the comparison.

---

Reviewers: 2 done | Synthesis: codex, 16s | Total: 25m31s

[wesm]

Round-5 findings addressed in a588ed6.

1. Full/backstop passes scan all completed progress rows — The done-revisit predicate moved out of the progress-driven arm into its own UNION arm that drives from the sessions side of the join, so the planner bounds it with idx_sessions_local_modified. A second manager watermark (fullWatermark, passed as DoneChangedSince) restricts revisits to sessions written since the last completed unlimited full pass; it ratchets to the pass start (no quiet-period lag is needed — a write landing during pass N carries a local_modified_at at or after N's start, so pass N+1 still sees it), and only unlimited full passes advance it. A fresh manager (daemon restart, manual recall extract run) starts at zero and reconciles unbounded. TestExtractCandidatesDoneRevisitBoundedByWatermark covers the bound, TestExtractCandidatesFullScanPlanIsIndexBounded asserts via EXPLAIN QUERY PLAN that neither sessions nor recall_extract_progress is table-scanned, and TestManagerFullWatermarkBoundsDoneRevisits covers the manager wiring (advance on full passes only, no advance on limited passes, fresh-manager recovery).

2. Full-resync copy drops content_stamped_at — copyRecallExtractStateFromAttachedTx now copies the stamp. Older archives whose progress table predates the column are detected with pragma_table_info(?, 'old_db') and copy it as empty; those rows re-open once and settle on their first revisit. TestCopyRecallExtractStatePreservesContentStamp and TestCopyRecallExtractStateToleratesPreStampArchives cover both paths.

3. Message-count mismatch silently skipped while the watermark advances — A count mismatch under a stable snapshot bracket is no longer a silent skip: the manager upserts a progress row (which also handles digest-change corpus cleanup) and marks it failed with a descriptive error, so the session is visible in status and re-offered through the queue arm after the failure backoff — the queue arm is never gated by the discovery watermarks. The transcript still never reaches the model. An unstable bracket keeps the silent skip, since the concurrent write re-surfaces the session by itself. TestManagerMarksTranscriptOutOfStepRetryable covers the failure record and proves the retry flows through the queue arm with both watermarks set far in the future.

4. Second snapshot comparison misses concurrent ineligibility — The bracket recheck now goes through extractionBracketStable, which combines the field comparison with a full extractableSession eligibility recheck against the second read, so a session trashed or flagged automated between the two reads is discarded even though those flags touch no compared field. TestExtractionBracketStable covers trash, automation, moved local_modified_at, and vanished-row cases.

docs/internal/recall-extraction.md updated for all four (bounded done-revisit and the full watermark, resync stamp preservation, retryable out-of-step sessions, bracket eligibility recheck). Suites: internal/db, internal/recall/..., internal/config, internal/secrets, and cmd/agentsview pass except the pre-existing TestDoSyncConfiguredFullUnifiedHTTPUsesManifestDeltaAndOrderedProgress failure, which also fails on a clean origin/main checkout. golangci-lint clean; nilaway reports nothing in the changed files.

[roborev-ci]

roborev: Combined Review (a588ed6)

Overall verdict: Two medium-severity reconciliation bugs can leave Recall coverage inconsistent or stale.

Medium

• Completed rows bypass message-count mismatch handling — internal/recall/extract/manager.go:511
  If stored messages retain the previous digest but session.MessageCount differs, the completed-progress short-circuit runs before countMismatch is handled. The upsert preserves done, advances content_stamped_at, and incorrectly treats the inconsistent transcript as covered. Handle count mismatches before the done short-circuit, allow completed rows to transition safely into a retryable state, and add a regression test for an unchanged digest with a mismatched count.

• Metadata-only updates leave generated entries stale — internal/recall/extract/manager.go:492
  Reconciliation rebuilds entries only when transcript units change, even though entries also copy Project, CWD, GitBranch, and Agent. A metadata-only update keeps the same digest, advances the coverage stamp, and leaves entries with stale context that may match incorrect Recall filters. Synchronize contextual fields during same-digest revisits or include relevant metadata in the reconciliation identity, with a regression test covering a project or branch change.

---

Reviewers: 2 done | Synthesis: codex, 9s | Total: 18m29s

[wesm]

Round-6 findings addressed in fd5bc69.

1. Completed rows bypass message-count mismatch handling — Verified: with an unchanged digest, the progress upsert preserves done and settles content_stamped_at, and the done short-circuit returned before the mismatch check, so the inconsistent state was claimed as covered. The mismatch is now handled before the short-circuit, and MarkExtractProgressFailed gained a ReopenDone option that waives only the non-done guard — the optimistic digest and cursor checks still apply, so a reopen cannot clobber a row another writer moved. A reopened row's cursor resets to zero: AdvanceExtractCursor is strictly monotonic, so a reopened row left at cursor == total could never transition back to done; from zero the retry replays its units (entries dedupe by positional id) and converges. TestMarkExtractProgressFailedReopensDoneOnRequest covers the db semantics (including the stale-cursor refusal), and TestManagerReopensDoneSessionOnCountMismatch covers the manager end to end: same-digest count drift → retryable failure with no model calls, then heal → retry → done.

2. Metadata-only updates leave generated entries stale — Fixed by synchronizing rather than by widening the reconciliation identity: folding metadata into the digest would trigger full model re-extraction on every project or branch change. A new SyncExtractedEntryContext refreshes project, cwd, git branch, and agent on the session's generated entries, scoped to unreviewed_auto (mirroring the delete path, so human-touched entries stay as they were) with a no-op guard so an already-synchronized corpus touches nothing. The manager calls it on every same-digest revisit before the stamp settles. TestSyncExtractedEntryContextRefreshesGeneratedEntries covers scoping (other generations, other sessions, reviewed entries untouched) and idempotence; TestManagerRevisitSyncsEntryContext covers the revisit path: project/branch change with an unchanged transcript → entries updated with zero model calls.

One test-harness note: the bare UpsertSession helper does not stamp local_modified_at (the sync batch path does), so both new manager tests stamp it explicitly via BumpLocalModifiedAt to model a real sync write — without it the done-revisit gate never re-opens the session and the regression tests would pass vacuously.

docs/internal/recall-extraction.md updated for both. Suites: internal/db, internal/recall/..., internal/config, internal/secrets pass; cmd/agentsview passes except the pre-existing TestDoSyncConfiguredFullUnifiedHTTPUsesManifestDeltaAndOrderedProgress failure (also fails on clean origin/main). golangci-lint clean; nilaway reports nothing in the changed files.

[roborev-ci]

roborev: Combined Review (fd5bc69)

Verdict: Changes requested — one high-severity correctness issue and six medium-severity reliability/security issues remain.

High

• Eligibility can change during extraction — internal/recall/extract/manager.go:546
  Eligibility is checked only before the potentially long model-call loop. If a session is trashed, becomes automated, or gains secret findings during processing, later units may still reach the model and be persisted, with no subsequent reconciliation.
  Fix: Revalidate the session snapshot and secret findings before each model call and before persisting its result; discard generated entries and progress if eligibility is lost.

Medium

• Cross-origin redirects enable blind SSRF — cmd/agentsview/recall_extract.go:99
  A malicious extraction endpoint can return a 307/308 redirect to another HTTPS origin or a loopback HTTP address. Go may replay the extraction POST and body to that destination, potentially reaching localhost or internal services.
  Fix: Disable redirects or require the original scheme, hostname, and port. If failover is necessary, use an explicit origin allowlist.

• Retry setup incorrectly resets failure backoff — internal/db/recall_extract.go:366
  A same-digest upsert refreshes updated_at on failed rows before retry work begins. Cancellation then restarts the entire failure backoff instead of preserving resumable shutdown behavior.
  Fix: Preserve updated_at for same-digest failed rows and change it only when a genuine new failure is recorded.

• Coverage is stamped before context synchronization succeeds — internal/recall/extract/manager.go:499
  A same-digest revisit advances content_stamped_at before SyncExtractedEntryContext completes. If synchronization fails or is canceled, the row appears covered and stale metadata is never retried.
  Fix: Synchronize context before advancing the coverage stamp, or make both operations atomic.

• Legacy rows are needlessly reprocessed every full pass — internal/db/recall_extract.go:820
  Completed rows with local_modified_at IS NULL are revisited on every full pass, even after content_stamped_at is settled, causing repeated transcript loading and derivation across large archives.
  Fix: Revisit NULL rows only while their coverage stamp is empty.

• Zero-unit failed retries cannot return to done — internal/recall/extract/manager.go:543
  A reopened count-mismatch failure that derives zero units preserves the failed state, executes no loop iterations, and never transitions progress back to done.
  Fix: Explicitly mark verified zero-unit retries as done and add a regression test.

• Read-only status lacks extraction schema compatibility checks — cmd/agentsview/recall_extract.go:330
  The read-only compatibility probe omits extraction tables and content_stamped_at. Archives requiring migration therefore open successfully but later fail with raw missing-table or missing-column SQL errors.
  Fix: Add extraction storage to the compatibility probe or perform a status-specific preflight that returns standard upgrade guidance.

---

Reviewers: 2 done | Synthesis: codex, 21s | Total: 28m37s

[wesm]

Round-7 findings addressed in d089fd2.

High — eligibility can change during extraction — The bracket no longer ends when distillation starts. A new recheckExtraction (snapshot re-read + eligibility predicates + a fresh secret-findings query, since a scan under an unchanged rules version can land candidate findings without touching any snapshot field) runs before every model call and again before persisting each call's output. A concurrent write stops the pass silently as before; losing eligibility fails closed — generated entries are deleted and the progress row reopens at cursor zero as a retryable failure, so nothing extracted under the lost eligibility persists and a restored session re-extracts from scratch. TestManagerDiscardsSessionTrashedMidExtraction (trash lands during the second model call → later units never sent, zero entries persist, progress failed at cursor 0) and TestManagerStopsWhenSecretFindingAppearsMidExtraction (candidate finding under the same rules version) cover both routes.

Cross-origin redirects (SSRF) — CheckRedirect now requires the redirect target to match the configured endpoint's origin exactly (scheme, host, port); the previous transport-policy check allowed cross-origin HTTPS and loopback HTTP, both now refused. Same-origin path redirects still work. No allowlist added — nothing needs failover. TestResolveExtractDistillationRestrictsRedirectsToOrigin covers same-origin allowed, plaintext downgrade, cross-origin HTTPS, different port, and loopback.

Retry setup resets failure backoff — the upsert now preserves updated_at when the digest is unchanged and the stored state is failed, so the retry's opening upsert no longer restarts the backoff a cancelled retry would wait out again; a digest change still takes a fresh clock. TestUpsertExtractProgressPreservesFailedBackoff.

Stamp settles before context sync — the sync moved before the upsert that stamps coverage. The regression test (TestManagerRetriesContextSyncWhenItFails) installs a RAISE trigger on recall_entries through a side connection to make the sync fail, and verifies the next pass revisits and repairs the context — under the old order it reproduced the stale-forever behavior.

Legacy NULL rows reprocessed every full pass — the done-revisit predicate now treats local_modified_at IS NULL as revisitable only while content_stamped_at = '': every write path records a local write, so a stamped legacy row cannot have changed; pre-stamp archive copies still re-open once and settle. TestExtractCandidatesLegacyNullRowsSettleAfterStamp.

Zero-unit failed retries never return to done — a same-digest upsert now completes a zero-unit row whatever state it held (they are done by construction; the loop runs zero iterations, so no cursor advance would promote them). TestUpsertExtractProgressCompletesZeroUnitRows.

Read-only status compatibility — recall_extract_generations and recall_extract_progress joined readOnlyRequiredTables, so read-only opens of unmigrated archives return the standard schema-upgrade guidance (column checks, including content_stamped_at, derive automatically from the schema probe). Cases added to the existing table-driven read-only tests.

Also in this commit: ReopenDone renamed to Reopen, with the cursor reset now unconditional — the mid-extraction discard needs it on partial rows too, since a preserved cursor would skip units whose entries were deleted (TestMarkExtractProgressFailedReopenRestartsPartialRows).

Docs updated (recall.md transport paragraph, recall-extraction.md bracket/progress/reconciliation/scheduling sections). Suites: internal/db, internal/recall/..., internal/config, internal/secrets pass; cmd/agentsview passes except the pre-existing TestDoSyncConfiguredFullUnifiedHTTPUsesManifestDeltaAndOrderedProgress failure (also fails on clean origin/main). golangci-lint clean; nilaway reports nothing new in changed files.

[roborev-ci]

roborev: Combined Review (d089fd2)

High-severity correctness and medium-severity reconciliation, provenance, and performance issues remain.

High

• internal/recall/extract/manager.go:594 — The final eligibility recheck and entry insertion are not atomic. A secret scan, trash operation, or automation update can commit between them, allowing output from an ineligible session to persist.
  • Fix: In one database transaction, verify the expected session snapshot and absence of findings, insert entries, and advance progress.

Medium

• internal/db/recall_extract.go:811 — Candidate selection excludes ineligible sessions before reconciliation. Entries from sessions that later become trashed, automated, or associated with a secret finding are never removed, and partial progress may permanently block generation activation.

  • Fix: Add an eligibility-loss reconciliation path that deletes automatic entries and resets or removes progress without invoking the model.

• internal/recall/extract/manager.go:751 — Extracted entries set ProvenanceOK to true without storing the host-derived content digest and stable endpoint UUIDs. The evidence reconciler treats an empty digest as invalid, causing provenance to be trusted initially and revoked after resync or relevant transcript changes.

  • Fix: Bind each unit through the recall evidence-window APIs and persist the digest and source UUIDs before marking provenance valid.

• internal/db/recall_extract.go:1082 — Extraction statistics filter by source_run_id, but recall_entries has no index beginning with that column. Background generation passes will repeatedly scan the full recall corpus as it grows.

  • Fix: Add an index beginning with source_run_id, ideally covering the session and review-state predicates, plus a cardinality-scaling regression test.

---

Reviewers: 2 done | Synthesis: codex, 14s | Total: 18m33s

[wesm]

Round-8 findings addressed in 50e5858.

High — recheck and insert not atomic — Verified: a write could land between the out-of-band recheck and the entry insert. Each unit is now persisted through CommitExtractedUnit, a single transaction that re-reads the session row, re-runs the eligibility predicates, re-counts secret findings, verifies the snapshot guard (message count, transcript revision, local-modified stamp) against the state the unit was derived from, inserts the entries, and advances the cursor. The database mutex plus the transaction make this atomic against every in-process writer, and the offline writer lock excludes other processes. A guard failure returns ErrExtractSessionDrifted with nothing persisted; the manager classifies it (still eligible → concurrent write, silent retry next pass; ineligible → discard and reopen). The pre-call recheck remains only to avoid wasting a model call. TestCommitExtractedUnitRefusesDriftAndIneligibility covers stale snapshot, concurrent candidate finding under a matching snapshot, and trashed session; TestCommitExtractedUnitBindsEvidenceAndAdvances covers the happy path including partial→done transitions.

Post-extraction eligibility loss never reconciled — Verified, including the activation-blocking claim: maybeActivate refuses while any pending or partial row exists, and progress stats count rows regardless of session eligibility, so a session that went partial and was then trashed blocked activation forever. New ReconcileIneligibleExtractSessions (called on full passes, bounded by the full watermark — every ineligibility write records a local write, so the induction from done-revisits carries over; a fresh manager reconciles unbounded) deletes the unreviewed_auto entries and removes the progress rows of sessions since trashed, flagged automated, or carrying findings or leaks. Removing the row (rather than marking it failed) lets a restored session rediscover through the no-progress discovery arm and stops it counting in stats. Stale or missing scan versions deliberately do not qualify — they are transient (every transcript write clears the stamp until rescan) and deleting on them would rebuild the corpus on every sync. TestReconcileIneligibleExtractSessions covers all three causes, human-reviewed and other-generation entries surviving, and the watermark bound; TestManagerFullPassReconcilesIneligibleSessions covers the pass wiring end to end.

Provenance revoked for missing digest — Verified against the reconciler: it revokes provenance_ok entries whose evidence carries no content digest. Binding now happens inside the same commit transaction: each cited ordinal range is rebuilt via buildRecallEvidenceWindow + BindSelection and the resulting content digest and endpoint source UUIDs are stamped onto the evidence rows before insert (ranges bound once per unit and shared across its entries). TestCommitExtractedUnitBindsEvidenceAndAdvances asserts the digest and UUIDs; TestManagerProvenanceSurvivesTranscriptGrowth asserts entries keep a digest (and provenance) across a transcript append.

No index leading with source_run_id — Added idx_recall_entries_source_run on (source_run_id, source_session_id, review_state) in the base schema (all base columns, so it applies to existing archives on open), covering the stats count, the per-session delete, and the context-sync predicates. TestExtractStatsEntryCountPlanIsIndexBounded asserts via EXPLAIN QUERY PLAN that the count no longer scans recall_entries. One existing read-only compatibility test case moved from dropping review_state (now index-referenced, so SQLite refuses the DROP) to uncertainty.

Docs updated (recall-extraction.md: commit-time enforcement point, post-extraction reconciliation, evidence provenance binding). Suites: internal/db, internal/recall/..., internal/config, internal/secrets pass; cmd/agentsview passes except the pre-existing TestDoSync… failure (also fails on clean origin/main). golangci-lint clean; nilaway reports nothing in the changed files.

[roborev-ci]

roborev: Combined Review (50e5858)

Review verdict: Changes have multiple high-severity eligibility/privacy flaws and three medium-severity reliability/security issues.

High

• Eligibility reconciliation misses the active generation — internal/recall/extract/manager.go:243
  Reconciliation only covers the configured generation. When a new generation is building, the previous active generation continues serving entries that are not removed if their sessions become trashed, automated, or acquire secret findings. Reconcile both generations or make privacy retraction generation-independent.

• Eligibility retraction can be disabled indefinitely — internal/recall/extract/manager.go:242, cmd/agentsview/recall_extract.go:174
  Reconciliation runs only during full passes, but disabling backstop_interval schedules only incremental passes. Entries from sessions that lose eligibility can therefore remain accepted indefinitely. Run a separately watermarked eligibility-retraction pass regardless of the backstop setting.

• Activation can promote entries from known-ineligible sessions — internal/recall/extract/manager.go:813, internal/recall/extract/manager.go:835
  Both automatic and explicit activation can promote archived entries belonging to deleted, automated, or secret-bearing sessions. Atomically reconcile or reject ineligible entries within the activation transaction before promoting a generation.

Medium

• Entry deletion and cursor reset are non-atomic — internal/recall/extract/manager.go:690
  If cancellation or another error occurs after entries are deleted but before progress is reset, a later extraction can resume past units whose entries no longer exist. Perform both operations in one transaction with a guarded cursor reset.

• Large reconciliations can exceed SQLite’s parameter limit — internal/db/recall_extract.go:1249
  Materializing every matching session ID into two IN clauses can exceed the host-parameter limit, repeatedly preventing retraction. Use a set-based subquery or temporary table, or process bounded chunks in one transaction.

• Untrusted model errors permit terminal-control injection — cmd/agentsview/recall_extract.go:505
  A malicious model endpoint can return OSC/CSI sequences in an HTTP error body; recall extract doctor propagates the raw error to stderr, enabling deceptive output, hyperlinks, or clipboard manipulation. Apply sanitizeTerminal before returning the probe error, or escape untrusted response details in the model client, and test with terminal-control payloads.

---

Reviewers: 2 done | Synthesis: codex, 14s | Total: 22m28s

[wesm]

Round-9 findings addressed in 369ab33.

High — retraction misses other generations — ReconcileIneligibleExtractSessions no longer takes a fingerprint. It deletes ineligible sessions' unreviewed_auto entries whose source_run_id is any registered extraction generation and removes their progress rows across all generations, since a retired generation keeps serving until the next activation. Runs that are not extraction generations (imports) stay untouched. The shared ineligibility predicate is now a single SQL fragment (extractSessionIneligibleSQL) used by retraction and activation alike. Multi-generation cases added to TestReconcileIneligibleExtractSessions.

High — retraction schedulable away — Retraction now runs on every scheduled pass, not just full ones, bounded by its own reconcileWatermark that ratchets to the pass start on every completed unlimited scan pass (incremental included — the same induction as the other watermarks: every ineligibility write records a local write). With backstop_interval disabled, the catchup ticker's incremental passes therefore still retract. TestManagerIncrementalPassReconcilesIneligibleSessions covers it end to end.

High — activation promotes ineligible sessions' entries — The promotion UPDATE inside ActivateExtractGeneration's transaction now excludes entries whose source session matches the ineligibility predicate; both automatic and explicit activation route through it. Excluded entries stay archived (never served) and the retraction pass deletes them. TestActivateExtractGenerationSkipsIneligibleSessions.

Entry deletion and cursor reset non-atomic — New DiscardExtractedSessionOutput performs the entry delete and the guarded cursor reset in one transaction; a stale guard rolls the delete back too, so a resume can never skip units whose entries no longer exist. TestDiscardExtractedSessionOutputIsAtomic covers both the rollback-on-stale and the happy path. One behavioral note: the mid-extraction-trash test now expects the progress row to be gone after the pass rather than failed-at-zero — the mid-pass discard reopens it and the end-of-pass retraction then removes it, which is the intended end state (a restored session rediscovers through the no-progress discovery arm).

IN-clause parameter limit — Both retraction deletes are now set-based, with the ineligible-session SELECT inlined as a subquery; no per-session host parameters exist at all, so no limit to hit.

Terminal-control injection via doctor — The probe error (which embeds up to 200 bytes of the endpoint's HTTP error body) now passes through sanitizeTerminal before returning. TestRecallExtractDoctorSanitizesEndpointErrors probes with an OSC 8 hyperlink and a CSI clear-screen payload and asserts no ESC byte survives.

Docs updated (recall-extraction.md: retraction on every pass / generation independence / set-based deletes / atomic discard, and the activation promotion guard). Suites: internal/db, internal/recall/..., internal/config, internal/secrets pass; cmd/agentsview passes except the pre-existing TestDoSync… failure (also fails on clean origin/main). golangci-lint clean; nilaway reports nothing in the changed files.

[roborev-ci]

roborev: Combined Review (369ab33)

High-severity issues found; activation atomicity and redirect containment need correction before merge.

High

• Non-atomic corpus activation can retire valid data without a replacement
  internal/recall/extract/manager.go:807, internal/db/recall_extract.go:169
  A session can be trashed, gain findings, or change after the backlog check. Promotion may then skip its entries while still retiring the existing corpus. Transcript changes can also clear the scan stamp without entering the backlog, allowing stale entries through promotion. Recheck coverage, scan state, provenance, and promotable-entry existence inside the activation transaction, aborting on any mismatch.

• DNS rebinding bypasses redirect containment
  cmd/agentsview/recall_extract.go:108
  A permitted plaintext model endpoint can redirect to the same hostname and port, then rebind that hostname to a loopback or LAN address. The string-based scheme/host check passes, and Go may replay the transcript-bearing request to a local service. Disable extraction redirects, or pin resolved addresses and reject connections outside the approved address set or class.

Medium

• Ordinal gaps cause repeated extraction failures and block activation
  internal/recall/extract/segment.go:145, internal/recall/extract/manager.go:640
  Assistant messages are packed without checking ordinal continuity, while evidence requires gap-free ranges. Transcripts that preserve sequence gaps can repeatedly fail as “session drift,” leaving progress pending and triggering another model call on every pass. Split action units at ordinal discontinuities or represent evidence with contiguous ranges, and apply failure backoff to stable evidence-validation errors.

---

Reviewers: 2 done | Synthesis: codex, 14s | Total: 30m50s

[wesm]

Round 28 review response (as of 200edfd)

Verified all four findings against the code. Three fixed, one declined with rationale.

1. Endpoint outages handled per session (manager.go) — fixed. Confirmed: a dead endpoint made every queued session burn its own full retry ladder, so a large backlog's pass could stall for hours re-probing the same outage. extractSession's unit-loop error path now checks errors.As(err, &transientError) after marking the unit failed: an exhausted ladder aborts the whole pass. The failure mark lands before the abort, so the visited session sits behind its failure backoff and one pathological unit cannot re-abort every pass; the remaining backlog is untouched and resumes on the next pass. runPassLocked now counts the failed session before returning the error, so pass accounting survives the abort. Tests: TestManagerAbortsPassOnExhaustedTransientFailures (always-500 server, two queued sessions — pass errors after session A's ladder, session B has no progress row), and TestManagerRunPassRetriesFailedSessionFromCursor updated to the new contract while still pinning resume-from-cursor.

2. Quiet-period timestamps compared lexicographically (recall_extract.go:981) — declined, with the substance addressed by fix 3. The premise (mixed offsets breaking string comparison) doesn't hold here: every sessions.ended_at writer funnels through timeutil.Ptr, which formats RFC3339Nano in UTC with a Z suffix — offsets like +05:30 cannot reach the column. The residual anomaly is precision-only (…45Z sorts after …45.123Z), bounded to sub-second error against quiet periods measured in minutes. Moving to julianday(ended_at) on the predicate's left side would defeat the indexed range scans that the EXPLAIN-plan regression tests pin (round 24). Instead, the SQL selection stays advisory+indexed, and the new Go-side recheck (fix 3) parses timestamps exactly at the enforcement point — precision-correct where it matters.

3. Quiet-period eligibility can become stale (manager.go:508) — fixed. Confirmed: the backlog is materialized at pass start, so a session re-ended while queued (or during a long pass) could be extracted mid-settling. extractSession now rechecks via settledPastQuietPeriod against the freshly-read snapshot before any model work: time.Parse(time.RFC3339Nano, ended_at) compared to now - QuietPeriod, failing closed on an unparseable timestamp. Explicit single-session runs keep their bypass (explicit parameter). ended_at drift after the check is caught by the existing snapshot bracket: the content digest and eligibility re-verification at commit time. Test: TestManagerScheduledPassSkipsSessionReendedWithinQuietPeriod (session re-ended between backlog build and its turn — skipped without a progress row, no error, other session extracts normally).

4. Partial secret-scan commits don't notify (huma_routes_secrets.go:101) — fixed. Confirmed, plus a latent nil-deref: directBackend.ScanSecrets returned (nil, err) on error, discarding the engine's partial summary — so the handler couldn't even tell work had committed. The backend now returns the partial summary alongside the error, and the route notifies the extraction scheduler whenever summary.Scanned > 0, on both the success and error paths. Test: TestSecretScanNotifiesOnPartialCommit — 60 seeded sessions against the 50-session progress interval, with a ResponseWriter that cancels the request context on the first progress event, asserting exactly one mutation notification despite the error return.

Verification: extract/db/config/server/service suites green; golangci-lint 0 issues; nilaway 15→15 on touched packages (stash-compared). cmd/agentsview has its one known pre-existing failure and internal/sync its 9 pre-existing messages_fts failures — both counts identical on the pre-change tree via git stash.

[roborev-ci]

roborev: Combined Review (200edfd)

Medium-severity issues remain in scheduler recovery and secret-scan notification behavior.

Medium

• cmd/agentsview/extract_scheduler.go:179 — With the backstop disabled, a failed startup full pass leaves pendingFull set, but catch-up ticks always use Full: false, so they never consume it. Without another sync notification, changed completed sessions may remain unrevisited indefinitely. Run periodic passes with Full: tickFull || pendingFull, clear the carry only after a successful full pass, and add a regression test for a failed startup pass with the backstop disabled.

• internal/server/huma_routes_secrets.go:105 — Notification depends on summary.Scanned > 0, but Engine.ScanSecrets can persist the first session and then return on cancellation before incrementing Scanned. Eligibility may therefore change without notifying extraction, delaying retraction of generated entries. Notify after any scan error once scanning has been dispatched, or update the summary before returning a post-persist cancellation error.

---

Reviewers: 2 done | Synthesis: codex, 10s | Total: 18m8s

[wesm]

Round 29 review response (as of 27fd967)

Both findings verified against the code and fixed.

1. Stranded full-pass carry with the backstop disabled (extract_scheduler.go) — fixed. Confirmed: catchup ticks ran Full: false unconditionally and only the backstop branch consumed pendingFull, while the debounce timer only re-arms on sync activity — so a failed startup full pass left changed completed sessions unrevisited indefinitely. Periodic ticks now run with Full: tickFull || pendingFull, and the carry clears only after a full pass both starts and succeeds (a dropped or failed one stays carried). One behavior change this implies, now pinned by the updated TestExtractSchedulerCatchupTicksWhenBackstopDisabled: the first catchup tick consumes the startup full carry instead of waiting out the startup debounce. New regression test TestExtractSchedulerCatchupCarriesFailedStartupFullPass: startup full pass fails → next catchup tick is full → once it succeeds, later ticks are incremental. Observed red before the fix (the second call ran incremental).

2. Partial scan committing without incrementing Scanned (huma_routes_secrets.go / sync engine) — fixed at the engine. Confirmed the exact window: scanOneSession persists through a non-context-aware write, and the loop returned on ctx.Err() before counting the session it had just committed — a cancellation landing during the first session's scan reported Scanned == 0 with committed work, and the route's Scanned > 0 gate skipped the notification. Of the two suggested options I took the second (make the summary truthful) rather than notify-on-any-error: an unconditional error-path notification would also fire when nothing could have committed (e.g. the candidates query failed), while counting the persisted session keeps the existing gate precise. The loop now folds a persisted session into the summary before observing cancellation; the error contract is unchanged (cancellation still ends the run with an error, never a partial success). Regression test TestScanSecretsCountsSessionPersistedBeforeCancellation drives the window deterministically with a probing context whose Err() flips to Canceled the moment the session's secrets_rules_version commits — context-aware reads proceed normally (nil Done channel), so the first check to observe the cancellation is the one right after the persist. Observed red (Scanned 0, want 1) before the fix.

Verification: cmd/agentsview, internal/sync, internal/server, internal/service, internal/recall/extract suites run; only the pre-existing failures remain (1 in cmd/agentsview, 9 messages_fts in internal/sync — same counts as the pre-change baseline). golangci-lint 0 issues; nilaway 20→20 on the touched packages (stash-compared).

Note on the test placement: the new engine test lives in secret_scan_cancel_test.go rather than secret_scan_test.go — appending to the existing file re-uploads its blob, and GitHub push protection flags the pre-existing AKIA… scanner fixtures in it. The new test uses secret-free message content (the persist path under test runs regardless of findings).

[roborev-ci]

roborev: Combined Review (27fd967)

One medium-severity issue should be fixed before merge.

Medium

• internal/db/recall_extract.go:359 — Activation checks post-coverage modifications only for done rows. A failed row may retain staged entries, allowing stale context to be promoted if an eligible session is remapped to another project, CWD, or branch while its retry remains backed off.

  Suggested fix: Before promotion, refresh eligible failed rows’ entry context/provenance, or block/clear failed rows where local_modified_at >= content_stamped_at. Add coverage for activating a failed partial session after a context-only update.

---

Reviewers: 2 done | Synthesis: codex, 7s | Total: 22m51s

[wesm]

Round 30 review response (as of 7440393)

Finding verified and fixed.

Stale failed-row staged output can promote (recall_extract.go) — fixed. Confirmed: the activation transaction's stale gate filtered p.state = 'done' only, the building gate counts pending/partial, and the ineligible cleanup removes staged output only for sessions failing full eligibility — so a failed partial row whose still-eligible session was written after its coverage stamp (content change, or a remap to another project, cwd, or branch) promoted staged entries with context only the retry's same-digest refresh knows how to repair (RefreshExtractedSessionCoverage syncs entry context and rebinds evidence, but only when the retry actually runs — activation could beat the failure backoff).

Of the suggested options I took block-over-refresh: refreshing context at promotion time would still promote entries distilled from content written after the stamp (the same gate hole covers both), while blocking mirrors the existing done-stale semantics exactly and self-heals the same way — the retry either refreshes and re-stamps (same digest) or discards and re-extracts (changed digest), after which activation proceeds.

The new gate is scoped to what can actually promote stale output, as its own query using the full eligibility predicate:

• the session must be fully eligible — anything less (trashed, reopened, scan stamp cleared) is deleted by the ineligible cleanup later in the same transaction, preserving the transient-flux reset contract (TestActivateExtractGenerationResetsTransientlyIneligibleStagedOutput pins this and caught my first over-broad attempt, which reused the done-gate's hard-ineligible filter and blocked on flux sessions the cleanup owns);
• the row must hold staged entries — an outage-failed row with nothing staged promotes nothing and still never blocks, so activation isn't needlessly delayed by actively-written sessions that failed during an endpoint outage.

Tests, both written red-first: TestActivateExtractGenerationRefusesStaleFailedPartialCoverage (failed partial with staged entry + post-stamp write → ErrExtractActivationBlocked, generation stays building, entry stays archived; observed red — activation succeeded and would have promoted) and TestActivateExtractGenerationAllowsFreshFailedCoverage (bare stale-failed row and fresh failed partial → activation succeeds, fresh partial's staged output promotes as designed). docs/internal/recall-extraction.md's activation section updated to match.

Verification: internal/db full suite green (including all prior activation-gate tests), internal/recall/extract green, cmd/agentsview only its known pre-existing failure; golangci-lint 0 issues; nilaway 101→101 on internal/db (stash-compared).

[roborev-ci]

roborev: Combined Review (7440393)

High-severity security issue: credentialed endpoints can leak secrets through malformed redirect errors.

High

• internal/recall/extract/client.go:385 — The underlying url.Error.Err is emitted verbatim. A malformed redirect Location is parsed before CheckRedirect runs, and Go’s error can include the raw header. A credentialed endpoint could therefore expose credentials in CLI output, scheduler logs, and persisted failure text, bypassing existing redirect and response-body redaction.
  • Fix: Withhold transport-error details for credentialed endpoints. Otherwise, strip control characters and enforce a length bound. Add coverage for malformed redirect locations containing reflected credentials.

---

Reviewers: 2 done | Synthesis: codex, 8s | Total: 29m11s

[wesm]

Round 31 review response (as of 8641b26)

Finding verified and fixed.

Credential reflection through transport errors (client.go) — fixed. Confirmed, and the red test displayed the leak verbatim: distill's transport-error path wrapped url.Error.Err with %w, and Go's HTTP client quotes server-controlled bytes in several of those errors — a malformed redirect Location is parsed before the CheckRedirect policy runs, so its parse error echoes the raw header (failed to parse Location header "http://evil.example/%zz<reflected credential>"), and a malformed HTTP/1.x response is quoted the same way. A server that knows the endpoint credential could reflect it there, bypassing both the redirect-target redaction in RefuseRedirects and the response-body withholding, on the way to doctor stderr, scheduler logs, and persisted failure rows.

Transport-error detail now follows the established response-body policy via a new transportErrorDetail helper: withheld for credentialed endpoints (detailWithheld, same fail-closed credentialedEndpoint() check, which also treats an unparseable base URL as credentialed), control-stripped and bounded at 200 runes otherwise, per the finding's both suggestions. Two carve-outs, both safe by construction:

• context cancellation/deadline errors pass through — their text is fixed by the runtime and tells the operator why the request ended (timeout vs. outage) rather than a blanket "(withheld)";
• redirect refusals are unchanged — RefuseRedirects builds its own message with the target already run through config.RedactedEndpoint, and its sentinel must stay on the error chain for the endpoint-scoped classification in endpointScopedRejection.

Classification is unaffected: everything on this path except a redirect refusal was and remains transient, so dropping the foreign cause from the chain loses nothing any caller inspects (the retry loop watches ctx.Done() itself).

Tests, red-first with the reflected token visible in the failure output: TestClientWithholdsMalformedRedirectDetail (302 with a malformed Location reflecting a capability token against a path-token endpoint → error contains neither the token nor the header, does say withheld) and TestClientBoundsTransportErrorDetail (credential-free endpoint keeps the %zz malformed-Location diagnostic but a 600-byte header is truncated with the …(truncated) marker).

Verification: internal/recall/extract full suite green (including all prior redirect, withholding, and known-API-path diagnostic tests); golangci-lint 0 issues; nilaway 2→2 (stash-compared).

[roborev-ci]

roborev: Combined Review (8641b26)

High — Response-body read errors can expose credentials

• Location: internal/recall/extract/client.go:412
• A compromised model endpoint can reflect URL credentials into a malformed chunked-response trailer. The resulting body-read error is wrapped with %w without passing through credentialedEndpoint or transportErrorDetail. After retries, that error may be persisted or logged, exposing Basic-auth, query, or path credentials.
• Fix: Route body-read errors through transportErrorDetail, withholding details for credentialed endpoints and bounding/sanitizing them otherwise. Add a raw chunked-response regression test with credentials reflected in a malformed trailer.

Medium — Transient ineligibility can block generation activation indefinitely

• Locations: internal/recall/extract/manager.go:1057, internal/db/recall_extract.go:331
• Pending or partial progress for a session that becomes transiently ineligible, such as reopening or losing its full-scan stamp, remains counted as unfinished. Candidate selection excludes the session, reconciliation removes only hard-ineligible rows, and activation checks only hard ineligibility, making the later full-eligibility cleanup unreachable.
• Fix: Exclude rows that fail the full eligibility predicate from the unfinished activation count, and let maybeActivate rely on that transactional gate instead of rejecting all pending or partial statistics up front.

---

Reviewers: 2 done | Synthesis: codex, 9s | Total: 15m27s

[wesm]

Round 32 review response (as of 8efab41)

Both findings verified and fixed, with a threat-model note on the first.

1. Body-read errors quote raw wire bytes (client.go) — fixed, with the rationale reframed. The mechanism is real and I confirmed it empirically before writing the test: Go's chunked reader echoes a malformed trailer line whole — malformed MIME header: missing colon: "<raw line>" — so read errors carry server-chosen bytes into doctor output, scheduler logs, and persisted failure rows. On the threat model, though: a compromised endpoint reflecting the URL credential is a weak premise for credential theft — the endpoint already receives the credential on every request, so reflection steals nothing. What justifies the fix is narrower: secondary exposure (persisted failure rows and logs get shared in bug reports and CI output, and credential material must not transit into them) and plain hygiene (a benign broken proxy can put unbounded junk or terminal escapes into a trailer). Since the round-31 transportErrorDetail helper already encodes exactly that policy, the fix is one line: read errors route through it — withheld for credentialed endpoints, control-stripped and bounded otherwise, context errors passing through. Regression test TestClientSanitizesBodyReadErrorDetail drives a hijacked connection serving the malformed-trailer response raw: the credentialed case observed red with the reflected token in the error text; the bare-endpoint case pins that the malformed MIME header diagnostic survives.

2. Transient ineligibility can block activation indefinitely (manager.go / recall_extract.go) — fixed. Confirmed on all three legs: candidate selection applies full eligibility (skips the session), ReconcileIneligibleExtractSessions clears hard-ineligible rows only, and both maybeActivate (raw stats.Pending/Partial refusal) and the activation transaction's unfinished gate (NOT hard-ineligible filter) counted the row — so a pending row for a session that reopened and never ended again stalled activation with no code path that could ever settle it. As suggested: the unfinished gate now counts only fully eligible sessions (same predicate as candidate selection, so the two can't disagree), which makes the existing in-transaction cleanup reachable — it deletes the unfinishable row and its staged output, and rediscovery re-extracts once the session settles, matching the established transient-flux contract for failed rows. maybeActivate drops the raw-count refusal and defers to the eligibility-aware backlog probe (the candidates queue arm applies the same full-eligibility predicate) plus the transactional gates; the Done == 0 and entryless refusals stay. Tests, both red-first: TestActivateExtractGenerationResetsTransientlyIneligibleUnfinishedCoverage (pending twin of the failed-row flux test, both mutations: reopened, scan stamp cleared) and TestManagerActivatesOverTransientlyIneligibleUnfinishedSession (full pass over a done session plus a reopened pending one → activates, row cleared). All prior gate tests still pass — an eligible pending row still blocks (TestActivateExtractGenerationRefusesUnfinishedCoverage). docs/internal/recall-extraction.md activation section updated.

Verification: internal/db, internal/recall/extract, internal/server, internal/service suites green; cmd/agentsview only its known pre-existing failure; golangci-lint 0 issues; nilaway 103→103 (stash-compared).

[roborev-ci]

roborev: Combined Review (8efab41)

High-severity issues block approval: secret scanning can be bypassed, and credentialed redirects can leak secrets.

High

• internal/recall/extract/manager.go:582 — Secret detection scans messages individually, but adjacent assistant messages are later concatenated into one outbound unit. A multi-line secret split across messages, such as a PEM key, can evade both stored and boundary scans and then be sent to the model. Scan every constructed Unit.Text before any model request, and add a regression test covering a secret spanning adjacent messages.

• internal/recall/extract/client.go:394 — A credentialed extraction endpoint can redirect to a hostname containing the credential. Because config.RedactedEndpoint preserves hostnames, the redirect error can expose the secret in logs or CLI output. When the configured endpoint contains credentials, omit the redirect target entirely; otherwise, strip control characters and bound its length.

Medium

• internal/db/recall_extract.go:1036 — ended_at uses variable-precision RFC3339Nano strings, while eligibility compares them lexically with a fixed-millisecond cutoff. Equivalent timestamp formats can sort incorrectly, excluding quiet sessions and potentially stranding them behind the discovery watermark. Normalize timestamps to a fixed sortable representation or compare parsed numeric timestamps, with tests for mixed fractional precision and watermark advancement.

---

Reviewers: 2 done | Synthesis: codex, 10s | Total: 24m17s

[wesm]

Round 33 review response (as of 3e9560b)

All three findings verified and fixed. The second one exposed a latent hole worth its own mention, and the third overturns a round-28 decline.

1. Unit-straddling secrets evade per-message scans (manager.go) — fixed. Confirmed, and this one needs no hostile endpoint — it's the privacy boundary against accidental leakage, squarely in the threat model: units join adjacent assistant messages with \n\n before sending, and the private-key rule anchors on both BEGIN and END, so a PEM block split across two rows (agents do stream long output across messages) matches neither the stored per-message findings nor the extraction-time per-row rescan, while the joined unit text the model would receive contains the whole key. The manager now additionally scans every constructed Unit.Text — exactly the outbound payloads — and the existing secret-match path fails the session closed before any model call. Red test TestManagerRefusesUnitStraddlingSecret observed 2 model calls carrying the key pre-fix, 0 post-fix; the PEM literals are assembled at runtime so the test file never carries a key-shaped pattern (and push protection stays quiet).

2. Redirect target hostname can carry the credential (client.go) — fixed, and the policy hole it sat on too. Confirmed: RefuseRedirects redacts its target via config.RedactedEndpoint, which preserves hostnames. Credentialed endpoints now keep only the refusal sentinel (no target); credential-free ones keep the redacted target as the diagnostic, bounded at 200 runes. Writing the red test surfaced something bigger: the no-redirect policy lived solely in the daemon's wiring (cmd/agentsview sets CheckRedirect), so any Client built without an explicit HTTPClient followed redirects — replaying the extraction POST, transcript included, wherever the endpoint pointed. httpClient() now installs RefuseRedirects on its fallback and fills a missing CheckRedirect on caller-provided clients (on a copy, preserving their other settings). Tests: TestClientOmitsRedirectTargetForCredentialedEndpoints, TestClientBoundsRedirectTargetDetail — both exercised through the now-enforced default client.

3. Mixed-precision ended_at comparison (recall_extract.go) — fixed, reversing my round-28 decline. Round 28's version of this finding lacked the consequence that makes it matter; this round's has it. The anomaly is real but sub-second (RFC3339Nano trims trailing zeros, and a trimmed value in the cutoff's second sorts after it — 'Z' beats any digit or '.'). What round 28 missed: the discovery watermark advances to passStart − QuietPeriod, which during the anomaly window can land past the skipped session's local_modified_at, and every scheduled pass — full ones included — bounds discovery by that watermark. The mis-skip is therefore not "delayed one pass" but stranded until a daemon restart resets watermarks. The fix normalizes ended_at to fixed milliseconds via strftime inside the one shared eligibility predicate (extractEligibleSessionSQL), so every candidate arm and activation gate inherits it; the predicate is a filter, not the driving index range, so the pinned EXPLAIN plans are untouched (the round-28 concern about julianday on the range column doesn't apply here). One knock-on: the activation cleanup ran a second Sprintf pass over the rendered predicate, which mangled the strftime verbs into %!Y(MISSING) — caught by 13 manager tests going red — and is now plain concatenation. Test TestExtractCandidatesMixedPrecisionEndedAt covers trimmed-seconds and trimmed-millis inside the cutoff plus a same-second-past-cutoff negative.

Verification: internal/db, internal/recall/extract, internal/server, internal/service green; cmd/agentsview only its known pre-existing failure; golangci-lint 0 issues; nilaway 103→103 (stash-compared).

[roborev-ci]

roborev: Combined Review (3e9560b)

Potential privacy and lifecycle issues remain in Recall extraction, including one high-severity secret-exfiltration path.

High

• Secret material spanning extraction units bypasses the outbound privacy gate — internal/recall/extract/manager.go:967

  Secret detection scans each message or extraction unit independently. A secret whose identifying structure spans multiple units—such as a private key split across user messages—may evade every individual scan, even though the model endpoint can correlate the requests and reconstruct it.

  Fix: Before any model request, scan the aggregate outbound contents in transcript order in addition to the per-message and per-unit checks. Fail the session closed and discard staged output when the aggregate scan matches.

Medium

• Disabling extraction also disables reconciliation of generated entries — cmd/agentsview/recall_extract.go:143

  Previously accepted generated entries can remain queryable indefinitely after their source session becomes ineligible—for example, when it is trashed, classified as automated, or gains secret findings—because Recall queries filter only by entry status.

  Fix: Run retraction independently of model extraction, or enforce source-session eligibility when serving generated entries. Cover disabling extraction after activating a generation and subsequently making its source session ineligible.

• max_window_chars does not bound oversized individual messages or total session work — internal/recall/extract/segment.go:120

  A single oversized user or assistant message is sent whole. Overflow recovery can then recursively accumulate all leaf entries before committing, allowing one large transcript message to cause unbounded model calls and memory growth.

  Fix: Deterministically split individual messages at the configured rune limit, impose total per-session work bounds, and version the changed segmenter identity.

---

Reviewers: 2 done | Synthesis: codex, 11s | Total: 27m32s

[wesm]

Round 34 review response (as of 35c6d95)

All three findings verified and fixed. The first two were real gaps; the third I scoped to the concrete harm and declined the provenance-changing part with rationale.

1. Secret spanning units bypasses the outbound gate (high) — fixed, and the obvious fix doesn't work. Confirmed the round-33 per-unit scan still misses a key split across separate units (two user messages → two intent units → the endpoint correlates them). The natural fix — scan the concatenated unit texts — I implemented and it failed the red test: the scanner's private-key rule has a base64 payload-purity gate (≥99%), and the interposed unit formatting (USER MESSAGE (ordinal N):) drops a straddling block below it, so scanning formatted unit texts would silently miss real split keys. The rescan now runs over the raw message contents concatenated in transcript order — formatting-free, so a split PEM reconstructs to a clean high-purity body — which also subsumes the per-message and per-unit checks (any single-text match is a substring of the join). Verified empirically (raw-join matches, formatted-unit-join does not) before settling on it. Test TestManagerRefusesSecretSplitAcrossUnits: key BEGIN in one user message, END in another → 0 model calls, session failed.

2. Disabling extraction disables retraction (medium) — fixed via independent retraction. Confirmed: setupRecallExtraction returns nil when [recall.extract].enabled is false, so a generation activated while it was enabled keeps serving, and a source session later trashed/automated/secret-bearing is never reconciled (unreviewed_auto entries serve whenever a caller omits TrustedOnly). I took the "run retraction independently" option over serve-time enforcement: the serving path fans across multiple backends and query builders, so a serve-time eligibility join would be a large cross-cutting change, whereas reconciliation is the existing designed mechanism — the bug was purely the Enabled gate. A new Reconciler (no model client) is driven by the existing scheduler when extraction is disabled and a generation exists (nil otherwise, so the default-disabled daemon starts nothing), wired through the same startup pass, session-mutation notifier, and periodic ticks, with a short debounce so retraction is prompt. Tests: TestReconcilerRetractsIneligibleGeneratedEntries and TestSetupExtractReconcileOnlyWhenDisabled (nil without a generation; retracts a soft-deleted session's served entry with one).

3. Unbounded overflow-recovery work (medium) — bounded; message-splitting declined. Confirmed the concrete harm: one oversized message becomes one unit (user content is never packed, so unbounded in length), and distillSplit recursed with no cap — fanning out a model call per leaf and accumulating every leaf's entries. distillSplit now threads a shared call counter and fails the session closed with ErrSplitBudgetExceeded at a generous cap (256; no well-formed action unit approaches it). Test TestManagerBoundsOversizedUnitSplitWork drives a server that overflows above a threshold and succeeds below → without the bound the whole message distills via ~hundreds of leaf calls; with it, ≤256 calls, session failed, nothing persisted. I did not implement the finding's other two asks — splitting individual messages at the rune limit, and versioning the segmenter identity — because unit boundaries are the evidence-provenance unit (ordinal ranges); sub-dividing a message would require sub-ordinal evidence coordinates, a provenance-schema and fingerprint change beyond a review fix. The overflow path already exists to handle oversized content; the only defect was that it was unbounded, which the budget closes.

Verification: internal/recall/extract, internal/db green; cmd/agentsview only its known pre-existing failure; golangci-lint 0 issues; nilaway 17→17 (stash-compared). Docs updated (outbound aggregate rescan, reconcile-when-disabled, split budget).

On the round count: this slice has taken many rounds, and the remaining findings are getting narrower (spanning-secret variants, precision edges, lifecycle corners). I still think the branch is merge-worthy and would benefit from the squash before merge we discussed. Happy to keep going as long as findings keep landing.

[roborev-ci]

roborev: Combined Review (35c6d95)

High-severity credential-exposure issue remains in the extraction privacy gate, plus one medium-severity lifecycle bug.

High

• Message-boundary fragmentation bypasses outbound secret scanning — internal/recall/extract/manager.go:982

  Messages are aggregated with "\n", so fixed-format credentials split across adjacent messages remain undetected because regexes requiring contiguous characters cannot match. Both fragments may still be sent to the configured model endpoint, where they can be reconstructed.

  Scan both newline-preserving and separator-free aggregates—or overlapping boundary windows—and add regression tests for fixed-format credentials split across message boundaries, including splits within token prefixes and bodies.

Medium

• Generation reactivation promotes superseded entries — internal/db/recall_extract.go:286

  Activation promotes every archived unreviewed_auto entry, including obsolete entries archived after a reviewed replacement superseded them. Reactivating the generation can therefore serve both the obsolete entry and its replacement.

  Exclude entries with a non-empty superseded_by_entry_id from promotion and related extraction lifecycle operations. Add a regression test covering generation reactivation after supersession.

---

Reviewers: 2 done | Synthesis: codex, 11s | Total: 14m16s

[wesm]

Round 35 review response (as of 9cbd2dc)

Both findings verified and fixed.

1. Mid-token credential split across messages (high) — fixed. Confirmed: round 34's aggregate joined messages with "\n", which reconstructs multi-line secrets (PEM blocks) but breaks a single-token credential split mid-token — "AKIA…" in one message, the rest in the next, joined as "AKIA…\n…" — so the AWS-key regex (contiguous characters) can't match while both fragments still reach the endpoint. The rescan now also scans a separator-free join, which reconstructs the token; the newline join is kept because it preserves the structure of multi-line secrets. Scanning both (rather than only separator-free) is deliberate: [\s\S]-style multi-line rules match under either join, but a rule anchored on an explicit newline would only match the newline join, so both are needed for full coverage. Test TestManagerRefusesSecretSplitMidTokenAcrossMessages: an AWS key split mid-body across two user messages → 0 model calls, session failed; the key halves are separate string literals so the test file carries no contiguous key (and push protection stays quiet).

2. Reactivation promotes superseded entries (medium) — fixed. Confirmed the mechanism end to end: supersedeRecallEntryTx archives the obsolete entry with status='archived' and superseded_by_entry_id=<replacement>, and activation's promotion flipped every archived unreviewed_auto entry under the generation back to accepted — so reactivating would serve the obsolete entry alongside its reviewed replacement. Promotion now excludes superseded_by_entry_id != '', and the servable-count gate excludes them too (a generation whose only staged entries are superseded has nothing servable, so it must not activate on their strength). Test TestActivateExtractGenerationSkipsSupersededEntries: a live staged entry and a superseded one → activation promotes the live entry and leaves the superseded one archived.

On the finding's "and related extraction lifecycle operations": I scoped the exclusion to promotion and the servable gate, which are where a superseded entry would re-enter service. Reconciliation and the stale gates operate on ineligibility and coverage staleness, not supersession, and deleting/reopening a superseded entry's row there would be out of their remit — supersession is a serving-state concern, and the two enforcement points above cover it. Happy to widen if you had a specific operation in mind.

Verification: internal/recall/extract, internal/db green (all prior activation-gate and secret-scan tests included); golangci-lint 0 issues; nilaway 103→103 (stash-compared). Docs updated (two-join aggregate rescan).

[roborev-ci]

roborev: Combined Review (9cbd2dc)

The PR has one high-severity credential-exposure risk and one medium-severity CLI validation issue.

High

• Credential fragments can bypass the outbound secret gate — internal/recall/extract/manager.go:982

  aggregateTranscriptSecretMatches scans concatenated raw messages, while TurnsV1.Units omits system messages and trims model-visible content. Credentials split across user messages can therefore evade scanning when an intervening system message or boundary whitespace exists, yet be reconstructed by the remote extraction service.

  Build the aggregate from the exact normalized, model-visible sequence: exclude system and unsupported roles, trim and skip empty content, then scan both newline-preserving and separator-free joins. Add regression tests for ignored intervening rows and boundary whitespace.

Medium

• Negative extraction limits become unlimited processing — cmd/agentsview/recall_extract.go:305

  Negative --limit values pass validation and are interpreted as unlimited extraction, potentially processing the entire eligible archive and causing unexpected model usage.

  Reject negative limits at the CLI boundary and reserve zero for unlimited processing.

---

Reviewers: 2 done | Synthesis: codex, 11s | Total: 16m49s

[wesm]

Round 36 review response (as of 9dc7739)

Both findings verified and fixed.

1. Aggregate diverges from the model-visible sequence (high) — fixed. Confirmed the gap round 35 left: aggregateTranscriptSecretMatches joined the raw rows, but TurnsV1.Units drops system messages and unsupported roles and TrimSpaces each message. So a credential split across two user messages with an intervening system row, or with boundary whitespace, was broken in the scan's aggregate (the interposed system content or spaces separate the halves) while the endpoint receives the two user units adjacent and can reconstruct it. The scan now aggregates exactly the model-visible contents through a new shared VisibleContents filter — and, importantly, Units now routes its own skip decisions through the same visibleContent predicate, so the segmenter and the scan cannot drift about what is sent (a future role/whitespace rule change updates both at once). Both joins (newline-preserving and separator-free) are still scanned. The Units refactor is behavior-preserving: skipped rows still update the ordinal cursor, so run-contiguity across a dropped row is unchanged. Tests TestManagerRefusesSecretSplitAcrossSystemMessage and TestManagerRefusesSecretSplitAcrossBoundaryWhitespace, both mutation-verified (reverted to the raw-rows aggregate → both go red with 2 model calls, restored → green).

2. Negative --limit becomes unlimited (medium) — fixed. Confirmed: the candidate query reads limit <= 0 as unlimited (-1), and a negative also fails the opts.Limit == 0 watermark-advance guard, so --limit -5 would scan the whole eligible archive and skip advancing the watermark. The run command now rejects a negative limit at the boundary (before config load), with zero kept as the documented unlimited value. Test TestRecallExtractRunRejectsNegativeLimit.

Verification: internal/recall/extract, internal/db green; cmd/agentsview only its known pre-existing failure; golangci-lint 0 issues; nilaway 17→17 (stash-compared). Docs updated (model-visible aggregate). AWS-key halves in the tests are separate literals, so no contiguous key reaches the files.

[roborev-ci]

roborev: Combined Review (9dc7739)

High-severity privacy-gate gap allows secrets to reach the model.

High

• internal/recall/extract/manager.go:590 — Secret checks scan raw message content and concatenations, but not the formatted Unit.Text sent to the model. Because TurnsV1 adds ASSISTANT:\n, a bare high-entropy token may trigger the high-entropy-assignment rule only in the outbound payload, bypassing the privacy gate. Scan every derived unit’s text before model calls, and add a regression test proving an assistant message containing a bare high-entropy token results in no HTTP request.

---

Reviewers: 2 done | Synthesis: codex, 10s | Total: 18m38s

[wesm]

Round 37 review response (as of 6f6d249) — finding declined with evidence

Scanning the formatted Unit.Text (high) — declined. This one is an over-reach: the proposed fix would fail a large fraction of legitimate sessions while not closing a real leak. I verified empirically before deciding.

The high-entropy-assignment rule matches identifier:\s*<20+ char token>. TurnsV1 prepends [N] ASSISTANT:\n to each assistant unit, so ASSISTANT becomes a spurious assignment key for whatever token starts the message. Scanning the formatted text (secrets.Scan on the exact strings the segmenter builds):

content	raw scan	formatted ([0] ASSISTANT:\n…)
bare 40-hex git SHA	0	1
bare base64 blob	0	1
bare UUID-style token	0	1

Git SHAs, hashes, base64 blobs, and UUIDs are ubiquitous as the first line of assistant messages in coding transcripts. Scanning the formatted payload would flag every such message as a secret and fail the session — a high-impact false-positive regression. This is also the exact failure mode round 34 moved away from (formatting polluting the scan), here in the false-positive direction.

On the leak premise: a bare high-entropy token is not treated as a secret by design. The high-entropy-assignment rule is candidate confidence and requires a real assignment structure precisely because bare high-entropy strings are usually not secrets. The ASSISTANT: we prepend is our own formatting, not a user assignment — treating it as one manufactures a match rather than revealing a real one. A genuine bare secret with no vendor prefix and no assignment is, by the scanner's own definition, not identifiable as a secret, and adding a fake key doesn't change that. Vendor-prefixed secrets (AKIA/sk-/ghp_/PEM) and real key: value assignments in the content are already caught by the raw and aggregate scans regardless of formatting.

There's also a consistency argument: sync-time scanning, the secret_findings table, and search all define "secret" over raw content. If extraction alone flagged formatting artifacts, a session with a bare SHA would be clean-and-servable per secret_findings but poisoned per extraction — the two disagreeing on what a secret is.

I added TestManagerExtractsBenignHighEntropyAssistantToken (commit 6f6d249) pinning that a benign high-entropy token (a bare commit SHA opening an assistant message) extracts normally, so this decision is guarded against a future "scan the unit text" change reintroducing the false positives.

If the real concern is bare unprefixed secrets, the right place to address it is the shared secret rules (so sync, search, and extraction stay consistent), not extraction-only scanning of text we formatted — happy to open that separately if you want it.

No production change this round.

On the broader thread: I agree we're into diminishing returns — the last several rounds have been narrowing variants of the same secret-gate and lifecycle areas, and this round's finding was a false-positive trap rather than a leak. I'd suggest this is a good point to squash and merge; I'll keep responding to any genuinely new findings.