Repository navigation
AI-1458: add shared SessionStart memory foundation - #350
Conversation
PR Summary by QodoAdd shared SessionStart memory foundation with leases, adapters, and Claude wiring
AI Description
Diagram
High-Level Assessment
Files changed (19)
|
Code Review by Qodo
Context used 1.
|
…-cert scaffold (#359) * test: Claude SessionStart memory-index behavioral baseline + live-cert scaffold The ClaudeHookCommand migration onto the shared SessionStart memory provider already landed with the AI-1458 foundation (#350); this adds the behavioral baseline that pins no regression, plus a gated live model-receipt cert. - 9 characterization tests: lessons+nudge+memory fragment ordering; only-ready-memory; empty-array / 204 / 5xx memory-index responses emit nothing (hook never fails); no re-fetch on a second session-start for the same session; memory-GET timeout does not suppress lessons/nudge; exhausted budget never touches the provider; a ready memory index is discarded when the session-start POST fails. - ClaudeMemoryIndexLiveCertTests: env-gated (KCAP_CURSOR/CLAUDE live) nonce-via- additionalContext model-receipt cert + disabled negative control, plus ungated ExtractAssistantAnswer parser coverage. No production change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test: trim verbose live-cert header comments (Qodo finding 1) The ~30-line XML <summary> on ClaudeMemoryIndexLiveCertTests and the multi-paragraph banner comment above the SessionStart memory-index baseline tests in ClaudeHookCommandTests read like runbook prose. Trim both to a short intent summary — what the file/block covers, that the live tests are env-gated manual release gates, and the --print-parser-unverified caveat — without touching any parser or gating logic. * test: fix AppConfig leak in disabled-memory-index lease-store test (Qodo finding 2) disabled_memory_index_does_not_construct_the_lease_store mutated the process-global AppConfig resolved state directly and restored a fresh default Profile instead of whatever was resolved before it ran, and had no parallel guard against interleaving with the file's other AppConfig/Console.Out-mutating tests. Route it through the existing WithProfileAsync helper (which captures + restores the original ResolvedServerUrl/ResolvedProfile regardless of run order or a mid-test exception) and add [NotInParallel], matching this file's precedent for every other test that touches the same shared state. * test: normalize CRLF before splitting kcap config show's JSON block (Qodo finding 4) ReadDisableMemoryIndexAsync split stdout on "\n\n" to isolate the leading JSON block from kcap config show's "JSON, blank line, Path:" output shape. On Windows the blank line is "\r\n\r\n", so the split never matched, JsonNode.Parse threw on the whole CRLF blob, and ReadDisableMemoryIndexAsync silently returned null — which would make the negative-control test's restore step wrongly write "false" even when disable_memory_index was originally true. Extract the split into ExtractLeadingJsonBlock, normalizing \r\n to \n first, and add CI-safe coverage for both the LF and CRLF shapes. * test: kill spawned process on timeout instead of leaking it (Qodo finding 3) RunProcessAsync's timeout path let WaitForExitAsync/ReadToEndAsync throw OperationCanceledException without ever signaling the spawned child, leaving a hung `claude`/`kcap` process running past the test. Wrap the read/wait in a finally that kills the whole process tree whenever the process hasn't already exited, and add a CI-safe test (spawning a long-lived cross-platform process the same way Daemon.DummyProcess does) that confirms the child is actually gone after a timeout, not just that the await unblocked. --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Summary
Companion server contract PR: https://github.com/kurrent-io/kcap-server/pull/1169
Linear: https://linear.app/kurrent/issue/AI-1458/sessionstart-memory-index-shared-provider-lifecycle-marker-and
Verification
Review history
8ebc699d040b414aa1ada75bd0b467aaapproved round 5