Skip to content

[AI-1460] Claude SessionStart memory-index behavioral baseline + live-cert scaffold - #359

Merged
alexeyzimarev merged 5 commits into
mainfrom
tonyyoung/ai-1460-claude-memory-baseline
Jul 24, 2026
Merged

alexeyzimarev merged 5 commits into
mainfrom
tonyyoung/ai-1460-claude-memory-baseline

Conversation

@realtonyyoung

Copy link
Copy Markdown
Collaborator

Implements AI-1460 — the behavioral baseline + certification for Claude Code's SessionStart memory-index injection.

What

The ClaudeHookCommand migration onto the shared SessionStartMemory provider already landed with the AI-1458 foundation (#350) — so this PR is test-only, no production change. It pins the no-regression baseline the design requires and adds the live model-receipt gate.

Tests

  • 9 characterization tests in ClaudeHookCommandTests: 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 (lease dedup); 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 (new): env-gated (KCAP_CURSOR_MEMORY_LIVE=1 + reachable KCAP_URL) nonce-via-additionalContext model-receipt cert + disabled negative control (manual release gate, mirrors the Cursor AI-1461 precedent), plus ungated ExtractAssistantAnswer parser coverage.
  • ClaudeHookCommandTests 35/35, parser 4/4 green; check-linear-ids clean.

Notes

  • No production/behavioral/output/latency/scope change vs AI-1165 — that's the acceptance.
  • The live cert is a manual gate (env-gated, skipped in CI); its --print parser is defensive and to be confirmed on first live run (same posture as AI-1461).

🤖 Generated with Claude Code

…t 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>
@linear-code

linear-code Bot commented Jul 24, 2026

Copy link
Copy Markdown

AI-1460

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Add Claude SessionStart memory-index baseline tests + gated live certification

🧪 Tests 🕐 40+ Minutes

Grey Divider

AI Description

• Add characterization tests pinning Claude SessionStart additionalContext ordering and fail-open
 behavior.
• Extend ClaudeHookCommand test fixture to fake /api/memories/index and track request dedup.
• Add env-gated live model-receipt certification using claude --print plus defensive output parsing.
Diagram

graph TD
  UT["Unit tests"] --> CH["ClaudeHookCommand"] --> AC["SessionStartAdditionalContext"] --> OUT["stdout envelope"]
  CH --> MEM["SessionStart memory"] --> IDX["GET /api/memories/index"] --> SVC[("kcap server")]
  LIVE["Live cert tests"] --> CLAUDE["claude --print"] --> CH
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Golden-file snapshot tests for additionalContext
  • ➕ Simplifies assertions for byte-exact output (one snapshot per scenario)
  • ➕ Makes ordering/format regressions very obvious in diffs
  • ➖ Harder to localize failures to specific fragment sources
  • ➖ Can be noisy if formatting changes intentionally (requires snapshot churn)
2. HTTP replay (VCR) integration tests against recorded server responses
  • ➕ Exercises more of the real HTTP stack without depending on a live server/model
  • ➕ More CI-friendly than subprocess-based live cert
  • ➖ Still cannot prove the model actually received/used the injected context
  • ➖ Recordings can become stale and mask real-world auth/network behaviors
3. Move the live-cert to a dedicated manual test project/command
  • ➕ Keeps unit test suite purely hermetic; reduces accidental invocation risk
  • ➕ Can ship with clearer operator UX (flags, output artifacts)
  • ➖ Adds discoverability/maintenance overhead vs keeping the cert near related tests
  • ➖ Still requires manual gating and external dependencies

Recommendation: Current approach is appropriate: hermetic characterization tests lock in the behavioral baseline (ordering, fail-open semantics, budget/dedup), while the env-gated live certification is the only practical way to prove end-to-end “model receipt” without polluting CI. Consider snapshot testing only if byte-exact assertions become too verbose.

Files changed (2) +588 / -0

Tests (2) +588 / -0
ClaudeHookCommandTests.csAdd memory-index characterization baselines and enhance HTTP fixture +257/-0

Add memory-index characterization baselines and enhance HTTP fixture

• Adds a suite of SessionStart memory-index behavioral baseline tests covering fragment ordering, empty/204/5xx fail-open behavior, timeout/budget joining semantics, exactly-once fetch via lease dedup, and POST-failure short-circuiting. Enhances the test Fixture to stub GET /api/memories/index separately (status/body/delay) and track request counts, plus utilities for temporary profile overrides and stdout capture.

test/Capacitor.Cli.Tests.Unit/ClaudeHookCommandTests.cs

ClaudeMemoryIndexLiveCertTests.csIntroduce env-gated Claude live memory-index certification + output parser tests +331/-0

Introduce env-gated Claude live memory-index certification + output parser tests

• Adds a manual, env-gated live certification that saves a nonce memory, runs 'claude --print' in a temp worktree, and asserts the nonce is reproduced via SessionStart additionalContext injection; includes a negative control by toggling disable_memory_index via 'kcap config set' with restore-on-finally. Includes CI-safe parsing tests for extracting assistant output from either plain text or JSON/JSONL stdout formats.

test/Capacitor.Cli.Tests.Unit/ClaudeMemoryIndexLiveCertTests.cs

@qodo-code-review

qodo-code-review Bot commented Jul 24, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Context used
⚠️ Tickets: not configured — ticket URL found in PR but could not be fetched — check ticket provider credentials

Grey Divider


Remediation recommended

1. AppConfig state leak ✓ Resolved 🐞 Bug ☼ Reliability
Description
disabled_memory_index_does_not_construct_the_lease_store mutates the process-global AppConfig
resolved state but restores it to a fresh default Profile instead of restoring the previous resolved
values, creating order-dependent test behavior. The test is also missing a NotInParallel guard
despite mutating global state, so it can race other tests that read/write AppConfig.
Code

test/Capacitor.Cli.Tests.Unit/ClaudeHookCommandTests.cs[R36-41]

    public async Task disabled_memory_index_does_not_construct_the_lease_store() {
        using var fx = new Fixture();
+        fx.MemoryIndexBody = """[{"memory_id":"m1","slug":"s","audience":"org","description":"d","kind":"preference"}]"""; // decoy — must never be fetched
        var storeConstructed = false;
        AppConfig.SetResolvedState("http://localhost", "default", new Profile { DisableMemoryIndex = true });
        try {
Evidence
The test sets AppConfig resolved state and then unconditionally resets it to a new Profile, which
does not restore whatever was resolved previously. The same file adds WithProfileAsync that
explicitly captures and restores the original resolved state, showing the intended safe pattern.

test/Capacitor.Cli.Tests.Unit/ClaudeHookCommandTests.cs[35-57]
test/Capacitor.Cli.Tests.Unit/ClaudeHookCommandTests.cs[266-282]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
A unit test mutates process-global `AppConfig` resolved state and resets it to a default profile rather than restoring the original values, which can make the test suite order-dependent and flaky under parallel execution.

### Issue Context
This file already introduces `WithProfileAsync(...)`, which captures and restores `AppConfig.ResolvedServerUrl`/`ResolvedProfile` safely; the `disabled_memory_index_does_not_construct_the_lease_store` test should use the same pattern and be serialized.

### Fix Focus Areas
- test/Capacitor.Cli.Tests.Unit/ClaudeHookCommandTests.cs[35-57]
- test/Capacitor.Cli.Tests.Unit/ClaudeHookCommandTests.cs[266-282]

### Suggested change
- Add `[NotInParallel]` to `disabled_memory_index_does_not_construct_the_lease_store`.
- Replace the manual `AppConfig.SetResolvedState(...); try/finally { ... }` with `WithProfileAsync(new Profile { DisableMemoryIndex = true }, () => ...)` (or inline the same capture/restore logic).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Verbose live-cert file header ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
A very long, multi-paragraph XML/file-level comment was added to live-cert and hook command test
files, duplicating operational/runbook guidance and detailed behavioral contracts inside the tests
rather than keeping commentary concise. This reduces readability, makes the files harder to scan,
and increases maintenance burden and edit risk over time.
Code

test/Capacitor.Cli.Tests.Unit/ClaudeMemoryIndexLiveCertTests.cs[R9-43]

+/// <summary>
+/// The manual, recorded release-gate certification that the shared SessionStart memory index
+/// actually reaches the model for Claude Code itself — mirrors
+/// <see cref="Cursor.CursorMemoryIndexLiveCertTests"/>. Everything else in this feature (routing,
+/// parallel-fetch/budget math, lifecycle, envelope ordering, output bytes) is proven by the unit
+/// suite in <see cref="ClaudeHookCommandTests"/> against fakes; THIS file is the one place that
+/// asserts the real end-to-end claim — a fresh, high-entropy nonce placed only in a saved memory
+/// reaches the model's own answer via the SessionStart <c>hookSpecificOutput.additionalContext</c>
+/// envelope — because that claim can't be certified any other way.
+///
+/// <b>Gated</b> behind <see cref="LiveGateEnvVar"/> (unset by default) so CI, and any ordinary
+/// local test run, never executes this: it spends a real <c>claude --print</c> turn against a
+/// real, reachable kcap server. Requires:
+///   • <c>claude</c> (the Claude Code CLI) on PATH, logged in to its own account — separate from
+///     kcap's own login below.
+///   • Claude Code's SessionStart hook already configured to invoke <c>kcap</c> (e.g. via
+///     <c>kcap setup</c> / the Claude plugin installer) somewhere this test's throwaway worktree
+///     resolves settings from — a fresh temp dir carries no project-level override, so it
+///     inherits whatever is configured at the USER level (<c>~/.claude/settings.json</c>).
+///   • <see cref="ServerUrlEnvVar"/> pointing at a reachable kcap server exposing
+///     <c>GET /api/memories/index</c>.
+///   • <c>kcap login</c> already done against that server (this process's own token store is
+///     reused — see <see cref="HttpClientExtensions.CreateAuthenticatedClientAsync"/>) so a memory
+///     can be saved and later read back.
+///
+/// This is a MANUAL release-gate run by a human/controller before certifying (or re-certifying
+/// after a change to) Claude's SessionStart memory wiring — it is intentionally NOT wired into any
+/// CI job and this implementation does not attempt to run it. Claude Code's own <c>--print</c>
+/// output shape (plain text by default; no <c>--output-format</c> flag is passed here, mirroring
+/// <see cref="ClaudeBorrowedReviewLiveTests"/>'s own invocation) has not been independently
+/// verified against a live process for THIS purpose in this repo; <see cref="ExtractAssistantAnswer"/>
+/// parses defensively (JSON-lines OR plain text) — whoever runs this live for the first time
+/// should confirm/tighten that parsing and record the observed Claude Code version
+/// (<see cref="RecordClaudeVersionAsync"/>) alongside the result.
+/// </summary>
Evidence
PR Compliance ID 5 calls for brief comments that add only necessary context and prefers clarity
through code over extensive narrative. The cited blocks in ClaudeMemoryIndexLiveCertTests.cs and
ClaudeHookCommandTests.cs are lengthy, multi-page explanations mixing rationale, prerequisites,
operational/run instructions, behavioral baselines, and historical/migration context—material that
the tests and existing gating/helpers already convey—so the evidence supports reducing the in-code
commentary to a short intent statement (optionally with a link) and moving runbook-level detail to
external documentation.

CLAUDE.md: Keep Comments Concise and Prefer Self-Explanatory Code
test/Capacitor.Cli.Tests.Unit/ClaudeMemoryIndexLiveCertTests.cs[9-43]
test/Capacitor.Cli.Tests.Unit/ClaudeHookCommandTests.cs[60-72]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The test files `ClaudeMemoryIndexLiveCertTests.cs` and `ClaudeHookCommandTests.cs` contain overly verbose, multi-paragraph header/comment blocks that embed operational runbook guidance and detailed behavioral/historical narratives inside the test code. Update these comments to comply with the guidance that comments should be brief and that tests/code should express behavior, moving extensive documentation to external docs and leaving only a short intent summary (and optionally a link).

## Issue Context
PR Compliance ID 5 prefers concise comments that add only necessary context and emphasizes clarity through code/tests rather than long narrative commentary. The live-cert test already contains explicit gating (e.g., `KCAP_CLAUDE_MEMORY_LIVE`, `KCAP_URL`) and helper methods that communicate execution requirements, and the hook command tests already encode the behavioral baseline; the current long comments largely restate these requirements/behaviors and include additional rationale, prerequisites, operational steps, and migration/history that would be better maintained in dedicated documentation (e.g., CONTRIBUTING/testing docs) with only a brief in-code summary.

## Fix Focus Areas
- test/Capacitor.Cli.Tests.Unit/ClaudeMemoryIndexLiveCertTests.cs[9-43]
- test/Capacitor.Cli.Tests.Unit/ClaudeHookCommandTests.cs[60-72]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Timeout leaves orphan process ✓ Resolved 🐞 Bug ☼ Reliability
Description
RunProcessAsync cancels WaitForExitAsync/ReadToEndAsync using a timeout token but never kills the
spawned process on timeout, so a hung claude/kcap subprocess can be left running. This can wedge
manual live-cert runs and leak processes on a developer machine.
Code

test/Capacitor.Cli.Tests.Unit/ClaudeMemoryIndexLiveCertTests.cs[R278-295]

+    static async Task<(int ExitCode, string Stdout, string Stderr)> RunProcessAsync(
+            string fileName, IReadOnlyList<string> args, string? workingDirectory) {
+        var psi = new ProcessStartInfo(fileName) {
+            UseShellExecute        = false,
+            RedirectStandardOutput = true,
+            RedirectStandardError  = true,
+            WorkingDirectory       = workingDirectory ?? Environment.CurrentDirectory
+        };
+        foreach (var arg in args) psi.ArgumentList.Add(arg);
+
+        using var process = Process.Start(psi)
+            ?? throw new InvalidOperationException($"Failed to start '{fileName}'.");
+        using var timeoutCts = new CancellationTokenSource(ProcessTimeout);
+        var stdoutTask = process.StandardOutput.ReadToEndAsync(timeoutCts.Token);
+        var stderrTask = process.StandardError.ReadToEndAsync(timeoutCts.Token);
+        await process.WaitForExitAsync(timeoutCts.Token);
+        return (process.ExitCode, await stdoutTask, await stderrTask);
+    }
Evidence
The implementation uses a CancellationTokenSource timeout for ReadToEndAsync and
WaitForExitAsync, but has no cleanup path that kills the OS process when cancellation occurs.

test/Capacitor.Cli.Tests.Unit/ClaudeMemoryIndexLiveCertTests.cs[278-295]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`RunProcessAsync` applies a timeout via cancellation but does not terminate the child process when the timeout triggers.

### Issue Context
Because these are gated/manual tests, failures are more likely to be encountered by a human running them locally; leaving a hung process behind is disruptive.

### Fix Focus Areas
- test/Capacitor.Cli.Tests.Unit/ClaudeMemoryIndexLiveCertTests.cs[278-295]

### Suggested change
- Wrap `WaitForExitAsync` in a `try/catch (OperationCanceledException)` (or `finally`) and call `process.Kill(entireProcessTree: true)` (where supported) when the timeout is hit.
- After killing, optionally await `process.WaitForExitAsync()` with a short secondary timeout, then rethrow a clearer `TimeoutException` including `fileName` and args.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


View more (1)
4. CRLF breaks config parsing ✓ Resolved 🐞 Bug ☼ Reliability
Description
ReadDisableMemoryIndexAsync splits kcap config show output using "\n\n", which will not match
Windows CRLF blank lines ("\r\n\r\n"), causing JSON parsing to fail and returning null. In the
negative-control test this null then defaults the restore value to false, potentially overwriting a
machine where disable_memory_index was originally true.
Code

test/Capacitor.Cli.Tests.Unit/ClaudeMemoryIndexLiveCertTests.cs[R137-149]

+    static async Task<bool?> ReadDisableMemoryIndexAsync() {
+        var (exitCode, stdout, _) = await RunProcessAsync("kcap", ["config", "show"], workingDirectory: null);
+        if (exitCode != 0) return null;
+
+        try {
+            var jsonPart      = stdout.Split("\n\n", 2)[0];
+            var root          = JsonNode.Parse(jsonPart);
+            var activeProfile = root?["active_profile"]?.GetValue<string>();
+            if (activeProfile is null) return null;
+
+            return root?["profiles"]?[activeProfile]?["disable_memory_index"]?.GetValue<bool>();
+        } catch {
+            return null;
Evidence
The parser extracts JSON via stdout.Split("\n\n", 2), which fails to separate the JSON block on
CRLF output; when it returns null, the finally-block restore expression treats null as false and
writes that value back.

test/Capacitor.Cli.Tests.Unit/ClaudeMemoryIndexLiveCertTests.cs[137-151]
test/Capacitor.Cli.Tests.Unit/ClaudeMemoryIndexLiveCertTests.cs[101-128]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`ReadDisableMemoryIndexAsync` assumes Unix newlines when extracting the JSON block from `kcap config show`, which can fail on Windows and make the restore logic fall back to an incorrect value.

### Issue Context
The negative-control test restores `disable_memory_index` based on the `bool?` returned by `ReadDisableMemoryIndexAsync`; if parsing fails and returns `null`, cleanup restores `false` even if the setting was originally `true`.

### Fix Focus Areas
- test/Capacitor.Cli.Tests.Unit/ClaudeMemoryIndexLiveCertTests.cs[101-128]
- test/Capacitor.Cli.Tests.Unit/ClaudeMemoryIndexLiveCertTests.cs[137-151]

### Suggested change
- Normalize line endings first (e.g., `var normalized = stdout.Replace("\r\n", "\n");`).
- Then split on `"\n\n"`, or alternatively locate the first blank line via `IndexOf` supporting both `"\n\n"` and `"\r\n\r\n"`.
- Consider logging a warning when the original value can’t be parsed so the operator knows cleanup may be approximate.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment thread test/Capacitor.Cli.Tests.Unit/ClaudeMemoryIndexLiveCertTests.cs
Comment thread test/Capacitor.Cli.Tests.Unit/ClaudeHookCommandTests.cs Outdated
Comment thread test/Capacitor.Cli.Tests.Unit/ClaudeMemoryIndexLiveCertTests.cs Outdated
Comment thread test/Capacitor.Cli.Tests.Unit/ClaudeMemoryIndexLiveCertTests.cs
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.
…odo 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.
…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.
…ding 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.
@alexeyzimarev
alexeyzimarev merged commit 2e3d135 into main Jul 24, 2026
6 checks passed
@alexeyzimarev
alexeyzimarev deleted the tonyyoung/ai-1460-claude-memory-baseline branch July 24, 2026 11:09
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.

2 participants