Skip to content

feat(memory): add safe dream consolidation and review - #609

Merged
JAORMX merged 3 commits into
mainfrom
feat/dream-consolidation
Aug 18, 2026
Merged

JAORMX merged 3 commits into
mainfrom
feat/dream-consolidation

Conversation

@JAORMX

@JAORMX JAORMX commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Summary

  • harden scheduled dream consolidation with strict bounded plans, fair candidate rotation, version binding, and atomic exact-duplicate retirement
  • add an inspectable /dream workflow for project memory and the user model, with explicit token-spend confirmation, synthesized replacement review, whole-plan apply/dismiss, and honest receipts
  • retain authoritative plans behind bounded opaque expiring IDs and expose additive gRPC/HTTP APIs plus capability negotiation
  • document the safety boundary, process-local limitations, ownership posture, and operator workflow in ADRs 0226 and 0227

Safety properties

  • scheduled consolidation remains off by default and can only auto-apply byte-identical duplicates
  • reviewed synthesis atomically writes the displayed survivor replacement and retires all displayed sources per operation
  • clients never submit operations, keys, versions, or replacement content when deciding a plan
  • stale participants conflict instead of overwriting; independent operations may report partial outcomes honestly
  • manual dreaming is unavailable under ownership enforcement in v1
  • pending plans are bounded, expire after ten minutes, and are intentionally lost across restart or replica changes

Verification

  • GOFLAGS=-p=1 task test
  • task lint
  • task api:check
  • task docs
  • task site:build
  • go run ./cmd/mecademo
  • independent spec, architecture, security, UX, reuse, and QA review with repair iterations

The serial test setting was needed only to stay within the local temporary-build quota.

JAORMX added 3 commits August 18, 2026 08:40
Separate planning from application, rotate bounded candidate windows fairly, and restrict unattended consolidation to atomic retirement of exact duplicates. Preserve lifecycle history and report partial outcomes without leaking memory content.

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>
Expose bounded dream plans over gRPC and HTTP, retain authoritative plans behind opaque expiring IDs, and add a /dream TUI workflow for reviewing exact duplicate and synthesized replacement operations before application.

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>
Reconcile the root and OIDC standalone module graphs after rebasing onto the latest dependency bumps so readonly ko builds resolve consistently.

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>
@JAORMX
JAORMX force-pushed the feat/dream-consolidation branch from 9dc8507 to 4dfe4a0 Compare August 18, 2026 06:02
@JAORMX
JAORMX merged commit b3cfd5c into main Aug 18, 2026
26 of 27 checks passed
@JAORMX
JAORMX deleted the feat/dream-consolidation branch August 18, 2026 06:43
jhrozek added a commit that referenced this pull request Aug 19, 2026
manual_dream (already shipped on main, #609) and the new storage_health
field both landed on field 19, with manual_dream pushed to 23. That's a
wire-compatibility break: a mismatched client/server pair would misread
one field as the other (message vs bool, different json_name). Restore
manual_dream = 19 and give storage_health the next free number (23)
instead. Verified with `buf breaking --against origin/main` (now clean)
and regenerated code.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
jhrozek added a commit that referenced this pull request Aug 19, 2026
* docs(acceptance): plan session storage continuity

Define the verification contract for bounded snapshots, indexed inventory, migration, cleanup, retention, and legacy adoption.

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* chore(plan): decompose session storage waves

Map every acceptance criterion to an issue-aligned worker task and dependency wave.

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* chore(plan): start session storage wave one

Record the first worker branch and move the acceptance plan into progress.

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* chore(plan): split snapshot wave for bounded workers

Separate snapshot format, durability, and cross-process temp ownership into dependency-sized subagent tasks.

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* chore(plan): record blocked snapshot worker

Repeated focused subagents exhausted their runtime before producing a test or implementation edit.

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* chore(plan): retry snapshot worker without budget cap

Reset the infrastructure-only failure and dispatch with the inherited unbounded worker budget.

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* feat(jsonlstore): bound current session snapshots

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* chore(plan): complete snapshot core task

Record the merged v2 snapshot core and release the first dependent durability task.

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* chore(plan): dispatch snapshot durability task

Start the second wave-one subagent on crash and disk-failure guarantees.

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* fix(jsonlstore): make snapshot replacement crash durable

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* chore(plan): complete snapshot durability task

Record the merged crash-durable replacement implementation and release cross-process temp locking.

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* chore(plan): dispatch snapshot locking task

Start the final issue-586 worker on cross-process temp ownership and recovery.

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* fix(jsonlstore): coordinate snapshot temporary recovery

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* chore(plan): complete snapshot locking task

Record the merged cross-process family lock and orphan-temp recovery implementation.

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* chore(plan): split indexed inventory wave

Separate catalog rebuild, cursor-bounded pagination, and lock decomposition into subagent-sized tasks.

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* chore(plan): dispatch metadata catalog task

Start the catalog foundation subagent after the snapshot format wave completed.

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* feat(jsonlstore): add rebuildable inventory catalog

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* chore(plan): complete metadata catalog task

Record the merged rebuildable catalog and release bounded pagination.

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* chore(plan): dispatch bounded pagination task

Start the cursor and page-work subagent over the rebuilt catalog.

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* feat(storage): bound metadata pagination work

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* chore(plan): complete bounded pagination task

Record the merged generation-bound cursor and work-bounded catalog paging implementation.

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* chore(plan): dispatch inventory locking task

Start the final indexed-inventory worker on lock decomposition and reconciliation.

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* fix(jsonlstore): decompose inventory locking

Coordinate same-family mutations through the stable family flock while isolating catalog rebuild and generation reconciliation from session operations.

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* chore(plan): complete inventory locking task

Record the merged lock decomposition and release progressive UI plus maintenance server waves.

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* fix(jsonlstore): preserve durability and cancellation truth

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* fix(jsonlstore): detect appended v1 snapshots

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* fix(storage): make metadata continuation opaque

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* fix(redisstore): index bounded session metadata

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* docs(adr): renumber session storage maintenance

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* test(jsonlstore): prove cross-process family locking

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* test(jsonlstore): cover legacy snapshot artifacts

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* test(redisstore): prove bounded owner paging contract

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* fix(storage): confine catalog replacements

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* fix(storage): confine resolver file operations

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* chore(plan): dispatch progressive UX server wave

Start independent subagents for progressive sessions, storage health, and legacy adoption.

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* feat(mecatui): page session inventory progressively

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* feat(storage): expose bounded health status

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* feat(session): add authenticated legacy adoption

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* chore(plan): complete progressive UX server wave

Record progressive inventory, bounded storage health, and authenticated legacy adoption after integration repairs.

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* chore(plan): dispatch legacy adoption TUI

Start the capability-driven Adopt-as-chat workflow over progressive Sessions.

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* feat(mecatui): adopt eligible legacy sessions

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* chore(plan): complete legacy adoption TUI

Record the merged capability-driven Adopt-as-chat workflow.

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* chore(plan): dispatch maintenance server wave

Start independent subagents for resumable migration and the shared retention planner.

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* feat(storage): add resumable session migration jobs

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* feat(storage): add safe retention cleanup planner

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* chore(plan): complete maintenance server wave

Record merged migration and shared retention cleanup capabilities after integration repair.

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* chore(plan): dispatch operator maintenance wave

Start retention configuration and the TUI migration/cleanup workflows.

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* feat(storage): configure automatic retention

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* feat(mecatui): add session maintenance workflows

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* chore(plan): complete operator maintenance wave

Record configurable retention and the merged optimize/cleanup TUI workflows.

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* chore(plan): dispatch storage operations docs

Start the final worker on tested systemd, launchd, backup, migration, and restore guidance.

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* docs(storage): add local operations runbook

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* chore(plan): complete storage operations docs

Record the tested systemd, launchd, backup, migration, and restore runbook.

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* docs(acceptance): mark session storage continuity landed

All task waves and aggregate gates are complete; ac-trace and final panel remain as terminal verification.

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* fix(storage): restrict management and repair health lifecycle

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* fix(storage): serialize migration job checkpoints

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* fix(storage): require safe maintenance exclusion

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* fix(storage): unify cleanup candidate deletion

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* fix(redis): rebuild legacy metadata index

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* fix(app): own retention worker lifecycle

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* fix(storage): require real maintenance exclusion

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* fix(redis): fence migration ownership and publication

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* fix(storage): report stopped retention health truthfully

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* fix(redis): complete migration fencing and coverage

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* fix(storage): protect live children from retention

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* fix(storage): close final liveness ownership gaps

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* fix(storage): lease delegation child lifecycles

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* fix(redis): prove exact owner index coverage

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* docs(acceptance): close session storage plan

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* fix(perf): remove session persistence allocation regression

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* fix(memstore): cache metadata size estimates

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>

* fix(proto): stop reusing field 19 in ServerCapabilities

manual_dream (already shipped on main, #609) and the new storage_health
field both landed on field 19, with manual_dream pushed to 23. That's a
wire-compatibility break: a mismatched client/server pair would misread
one field as the other (message vs bool, different json_name). Restore
manual_dream = 19 and give storage_health the next free number (23)
instead. Verified with `buf breaking --against origin/main` (now clean)
and regenerated code.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* fix(grpcdriver): tolerate legacy metadata cursors without generation/scope/continuation

SessionMetadataCursor's new Generation/Scope/Continuation fields were
required non-empty on both the client and server side whenever a cursor
was present at all, with no capability negotiation. That breaks
pagination in both directions during a mixed-version deployment: a
version-skewed peer that predates these fields never populates them, so
its cursor was rejected outright as invalid instead of degrading.

The domain layer already models an empty-generation cursor as legitimate
(port.PaginateSessionMetadata's unbound path), and a bound pager already
turns a genuine mismatch into ErrSessionMetadataCursorRestart rather than
a hard failure. Add legacyOrBoundCursorFields and use it everywhere a
cursor triple is checked: accept all-empty (legacy) or all-non-empty
(bound), reject only a partial mix (which would indicate real
corruption). A version-skewed peer now degrades to a pagination restart
instead of a rejected request.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* test(retention): replace the tautological AC5.5 parity test with a real cross-check

PlanAutomaticRetention (server.cleanup.go) was a same-package passthrough
to PlanManualRetention with zero production callers — the real automatic
sweep (internal/app/childgc.go's childGC.sweep) calls
sessionretention.Plan directly and never touches it. The parity test
that was supposed to back AC5.5 ("automatic sweeps and manual plans
select the same candidates") called PlanManualRetention under two names
and asserted it equals itself; it could never catch a real divergence
between the two call sites.

Delete the unused shim and its self-referential test. Add a genuine
cross-check in internal/app/childgc_test.go that reads the store's rows,
runs the REAL childGC.sweep, then separately asks
server.PlanManualRetention over the same pre-sweep rows/policy/liveness
and asserts the two selections match.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* docs(server): fix classification rationale for management-authority boundaries

PlanSessionCleanup and 9 sibling migration/health/cleanup methods were
classified KindCallerOwned with rationale implying per-principal data
scoping. In fact these are store-wide admin-authority operations gated
by an allowlist (PlanSessionCleanup plans with owner=nil over the whole
store, for example) — only the resulting job/plan/token handle is
caller-bound, not the underlying data. The access control itself is
sound; the self-documentation an auditor would consult to reason about
blast radius was not. Rewrite the 10 rationale strings to say what is
actually decided, and add a doc-comment note on KindCallerOwned naming
this management-authority sub-case explicitly (a genuinely separate
AccessKind would need its own ADR per ADR 0212's closed 4-kind taxonomy;
not done here).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* docs(redisstore): document the Cluster key-declaration gap and the generation-increment invariant

Comment-only. Two things a future maintainer needs to know before
touching these Lua scripts:

- saveMetadataScript/deleteMetadataScript/conditionalDeleteMetadataScript/
  adoptMetadataScript all build the owner-index key by string
  concatenation inside the script rather than declaring it in KEYS[].
  Harmless today (only a single-node/Sentinel redis.Client exists), but
  a Redis Cluster upgrade can't be a client swap — these four scripts
  need hash-tagged keys or KEYS-array key building first.
- The exact-coverage proof in migration.go's verify*Coverage functions
  depends on every mutator of the metadata sorted sets either
  incrementing metadataRebuildGenerationKey or being a deliberate,
  reviewed exemption (adoptMetadataScript, whose writes happen under the
  fenced migration lock before the verification window opens). Nothing
  structurally enforces this today; a future fifth mutator that skips
  it would silently degrade the proof to "probably fine."

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* refactor(storage): dedupe port/jsonlstore/memstore reuse findings

Five reuse findings from the code-duplication review, all in the
storage-core area:

- memstore's encodedBytesLen now calls base64.StdEncoding.EncodedLen
  instead of reimplementing the formula.
- port.CompareSessionMetadataOrder replaces three independent hand-rolled
  "ModifiedAt desc, ID asc" comparators (port/store.go, jsonlstore's
  inventory_catalog.go and metalist.go) with one shared comparator.
  Verified algebraically equivalent at all three sites.
- session.PrincipalScopeHash replaces three duplicated
  sha256(issuer+subject) computations with one shared raw-hash primitive;
  each call site keeps its own nil-check/prefix/truncation logic on top,
  so on-disk/wire output is byte-identical to before.
- jsonlstore's writeInventoryFileWithPattern now delegates to the
  existing replaceCurrentSnapshot durable-write helper (generalized to
  take a temp pattern, an optional zero modifiedAt, and an optional
  *os.Root) instead of duplicating the atomic-write dance. Uses
  defaultSnapshotOps(), not the store's fault-injectable ops: the
  inventory-catalog write must stay invisible to snapshot-write fault
  injection, since advanceInventoryGeneration runs before the snapshot
  write it's meant to test in both Save and MigrateSessionFamily — using
  the injectable ops here made an injected post-rename fault abort
  before the snapshot rename it's supposed to exercise, and
  misclassified ENOSPC during migration as backend_failure instead of
  insufficient_space. Caught by an independent review pass.
- storage_health.go's isSessionStorageFile now uses the existing named
  suffix constants instead of hardcoded string literals.

port.CompareSessionMetadataOrder and session.PrincipalScopeHash are new
exported engine API surface (Added/minor per COMPATIBILITY.md) —
engine/api/{port,session}.txt regenerated via task api:update and
engine/CHANGELOG.md updated.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* refactor(redisstore,grpcdriver): dedupe result-decoding and cursor-validation reuse findings

- schedulestore.go's toString gained a []byte case; redisResultString
  (a near-identical duplicate) is deleted and its ~12 call sites in
  metadata_index.go/migration.go repointed to toString.
- grpcdriver's pageMetadataRequest now calls the existing
  validPortMetadataCursor instead of duplicating its check inline; the
  separate timestamp-validity check is untouched.
- gofmt -w on metadata_index.go (a trailing-blank-line violation from
  the redisResultString deletion, plus a pre-existing const-block
  alignment issue picked up as a side effect).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* refactor(sessionretention): use slices.SortStableFunc/ContainsFunc

planner.go used an unstable sort.Slice comparator (also a latent
correctness gap: the equivalent code this package's diff replaced in
childgc.go deliberately used SortStableFunc for deterministic cap
eviction on tied timestamps) and a hand-rolled linear scan for
membership. Replace with slices.SortStableFunc + cmp.Or/cmp.Compare
(same ascending ModifiedAt/ID ordering, now genuinely stable) and
slices.ContainsFunc. Also switches two plain-string sorts
(payload.Live/Leased) to slices.Sort.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* refactor(mecatui): reuse scrollWindow and widen humanizeBytes instead of duplicating them

- Sessions list reused the existing scrollWindow helper (already shared
  by the slash palette, @-mention menu, and /models picker) instead of
  a hand-rolled persisted-scroll field + keepSessionCursorVisible. Note:
  this changes the actual scroll behavior from sticky-edge to
  cursor-centered, matching models.go's existing convention — a real UX
  change, not purely internal.
- Deleted the duplicate binary-unit formatBytes (KiB/MiB) in favor of
  widening the existing decimal-unit humanizeBytes (int -> int64, added
  GB/TB steps); its one existing call site and test updated. Storage
  panels now render decimal (MB) instead of binary (MiB) units — also
  user-visible, not purely internal, but consistent with
  humanizeBytes's existing "reconciles with how sizes are reported
  everywhere" rationale.
- Refreshed 2 golden files (verified the diffs are byte-format text
  only, e.g. "6.0 MiB" -> "6.3 MB", arithmetic checked) and fixed 2 test
  assertions that hardcoded the old KiB output.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* fix(port): stop hashing the entire filtered row set on every metadata page

PaginateSessionMetadataBound computed its cursor-restart "generation" by
JSON-marshalling and SHA-256-hashing the whole filtered row set on every
call, making page N cost O(total matching sessions) instead of O(page
size) -- a real violation of AC2.1's page-work-bounded contract on
memstore, the one adapter that calls this helper (jsonlstore and
redisstore each maintain their own real persistent generation counter
and don't use it).

Change the signature to take the caller's own cheap, monotonic
generation signal instead of deriving one from row content. memstore
now tracks a real counter, bumped on Save/Delete, matching how
jsonlstore's inventory generation and redisstore's
metadataRebuildGenerationKey already work (a single global counter,
not per-owner -- owner-scope mismatches are still caught separately by
the Scope field).

Verified against the existing RunMetadataPager conformance test, which
already exercises restart-on-mutation. New exported engine API surface
change (Added/minor per COMPATIBILITY.md) -- engine/api/port.txt
regenerated, engine/CHANGELOG.md updated.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* test(storeconformance): add a shared SessionMigrationStore conformance suite

Neither jsonlstore nor redisstore's port.SessionMigrationStore
implementation was checked against a shared cross-adapter conformance
suite, unlike the other optional capabilities (RunMetadataPager,
RunConditionalPrunable already exist). Each adapter's migration tests
were written independently, with real risk of behavioral drift on
edge cases neither adapter's own author thought to test.

RunSessionMigration exercises the common contract: acquisition-gated
mutation (every mutating/ownership call rejects on an unbound
context), cross-acquisition exclusion (a second Acquire on the same
job id fails while the first is held), tampered-fingerprint handling
(a "changed" skip, never a silent overwrite), and job-checkpoint
round-tripping via Save/Load.

seedMigratable is a caller-supplied hook rather than something the
suite derives generically: neither adapter's ordinary Save produces a
migratable ("not yet current format") candidate, so each adapter's own
test supplies its existing internal seeding mechanism (jsonlstore's
seedMigrationV1; redisstore seeding directly into miniredis before
construction, since its metadata-index-ready state is fixed at open
time -- a real adapter difference the test comment documents).

Along the way: found and worked around a real behavior worth knowing --
jsonlstore's flock-based AcquireSessionMigrationJob retries until the
CALLER's context is done rather than failing fast on contention, so
the suite's contended-acquire check needs a bounded context (matching
what a real caller should already be doing).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* refactor(server,permconfig): dedupe ID generators and duration parsers

Two low-risk mechanical findings from the code-duplication review:

- newCleanupID (cleanup.go) and newMigrationHandle (migration.go), both
  in internal/adapter/server, generated a 16-byte crypto/rand hex ID
  identically. Extracted one shared randomHexID(n int) helper; both
  callers preserve their exact 16-byte length.
- internal/app's parseRetentionDuration and permconfig's
  validateRetentionDuration implemented identical duration-parsing
  logic (empty/"0" = disabled, else a non-negative time.ParseDuration
  value). internal/app already depends on internal/adapter/permconfig,
  so permconfig now exports ParseRetentionDuration and both callers use
  it -- validateRetentionDuration discards the parsed value,
  internal/app's wrapper keeps its existing path-qualified error text.

No behavior change; no test pinned the discarded internal error
strings.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* docs: regenerate llms.txt

Stale relative to the engine/CHANGELOG.md edits in the two preceding
review-fix commits (task docs:llms wasn't re-run after those).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* fix(lint): silence unused-parameter on the seed closure's unused *testing.T

CI's golangci-lint (revive unused-parameter) caught what my local go
vet/gofmt sweep didn't: the redisstore seed hook's *testing.T param is
genuinely unused (the row is seeded before construction, per the
comment above it). Rename to _.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* fix(port,storeconformance): address findings from an independent review of the last stretch

Three findings from a fresh Opus review pass over the memstore-pager
fix and the new SessionMigrationStore conformance suite -- none are
correctness bugs in the product, but each is real:

- engine/adapter/storeconformance/storeconformance.go: the two
  post-release assertions in RunSessionMigration used the same
  boundCtx that release() may itself cancel (redisstore's release
  cancels the acquisition's operation context), so a passing test
  proved nothing about the adapter's OWN ownership fence -- it could
  pass on ctx-cancellation alone. Verified by mutation: with BOTH of
  redisstore's real fences (the Go-side lost check and the Lua token
  compare) temporarily removed, the test still passed before this fix,
  and correctly fails after it. Wrap both assertions in
  context.WithoutCancel(boundCtx).
- engine/port/store.go: PaginateSessionMetadataBound silently degraded
  to unbound pagination when handed an empty generation, which would
  let a stale cursor mix rows instead of restarting -- exactly the
  failure ErrSessionMetadataCursorRestart exists to prevent. Reject an
  empty generation with an error instead.
- Softened the "O(total rows) instead of O(page size)" claim in the
  doc comment and CHANGELOG: prepareSessionMetadataRows's row
  copy/sort is still O(rows) per page either way, so this was a large
  constant-factor win (dropping a JSON encode + SHA-256 of every row),
  not an asymptotic one.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

---------

Co-authored-by: Jakub Hrozek <jakub@stacklok.com>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant