test(admission): a cancelled real remember keeps its slot and lands its row in the Cortex store (#728, slice 4) - #943

Claude finished @cdeust's task in 2m 39s —— View job

I'll analyze this and get back to you.

Review 1 of 3 (ADR-1119): PR #943

Five reviews of the diff origin/main...4ef429d: code quality, test coverage, documentation accuracy, security, performance. No Blockers.

• The gate fixture patches the same store instance that remember._get_store() uses (same get_shared_store(DB_PATH, EMBEDDING_DIM) key, after the conftest singleton reset). The patched insert_memory_exact is the real write: remember_helpers.py:683 → memory_exact.insert_exact.
• The claimed mutant (sem.release() in the CancelledError branch of run_admitted) fails _assert_abandoned_and_held every time, in both tests.
• Isolation holds: tests_py/conftest.py redirects CORTEX_MEMORY_DB_PATH (and DATABASE_URL for PostgreSQL) before any mcp_server import. The diff contains no secrets or PII.
• Every wait is bounded by _GUARD, and the suite's timeout = 300 caps the worst failure path (about 120 to 150 s), so a failure cannot hang CI. The craftsmanship limits hold (181 lines, every function under 40 lines, # source: on the numeric constants).
• Not checked here: the CI result on this commit (gh pr checks was not permitted in this run), and the test was read, not run.

Notes, no action before merge

• The docstring says the handler "has run its write gate, embedding and curation", but with force=True the gate is bypassed (determine_bypass) and try_curation returns create at once (remember_helpers.py:448-449).
• The docstring and CHANGELOG say "isolated Cortex SQLite store", but the CI unit job runs with CORTEX_MEMORY_STORE_BACKEND=postgresql. The test holds on both backends, so "the suite's isolated Cortex store" would be accurate.
• If a test fails, the gate teardown releases the worker without waiting for it, so a late insert can race the conftest table purge. Only an already-red run is affected; waiting on an ended event in teardown would close it.

Verdict: approve

The three notes from review 1 are addressed in fixup b825723:

• Docstring: with force=True, the write gate and curation are bypassed (try_curation returns create). The docstring now says this. The handler has embedded the content and built the record when it blocks.
• "Isolated Cortex store": SQLite locally, PostgreSQL in the CI unit job (scripts/ci_unit.sh:58). The docstring and CHANGELOG say so.
• Teardown: _GatedInsert sets done in a finally. If the insert was reached, the fixture waits for it (bounded by _GUARD) before the conftest purge.

Local run: 2 passed. ruff 0.17.3 and craftsmanship: OK. CI not yet observed on b825723.

Generated by Claude Code

Claude finished @cdeust's task in 1m 50s —— View job

Review 2 of 3 (ADR-1119): PR #943

• Check the Blockers from review 1 (4ef429d): there were none.
• Review only the commit pushed since then, b825723 (fixup). Five reviews: code quality, test coverage, documentation accuracy, security, performance.
• Post the review: test(admission): a cancelled real remember keeps its slot and lands its row in the Cortex store (#728, slice 4) #943 (comment)

No Blockers. The fixup narrows the teardown race through a done event set in a finally, with bounded waits. It does not weaken any assertion, and the new docstring and CHANGELOG wording is accurate. The posted review has 3 Notes, none needing action before merge.

Not checked: CI on b825723, and the tests were read, not run.

Verdict: approve

Review 2 of 3 (ADR-1119): PR #943

Review 1 (4ef429d) found no Blockers. This review covers only the commit pushed since then, b825723 (fixup). I ran five reviews on it: code quality, test coverage, documentation accuracy, security and performance. No Blockers.

What the fixup changes

• The teardown race from review 1 is narrowed. done is set in a finally, so it fires on every path: success, the release assert timing out, or the real insert raising. Teardown sets release before it waits on done, and both waits are bounded by _GUARD.
  • Passing runs get no extra delay: done is already set before teardown.
  • The worst failing run still finishes well under the timeout = 300 limit.
• The assertions are unchanged. The test still pins all four properties: the slot is held, abandoned_work counts the worker and then records it completed, nothing is written before the release, and the row lands after it.
• The new docstring and CHANGELOG wording is accurate:
  • CI unit runs on PostgreSQL through scripts/ci_unit.sh.
  • force=True bypasses the write-gate decision (determine_bypass) and try_curation.
  • The handler embeds the content and builds the record before calling insert_memory_exact.
• Security: no secrets or PII, and no writes outside the isolated store.
• Not checked: CI on b825723, and the tests were read, not run.

Notes, no action before merge

• After a failed test, the late-write race is only partly closed. Teardown skips the wait when reached is unset. done also fires before the post-store writes (_run_post_store, mood EMA, wiki sync). The ignored done.wait(_GUARD) result times out silently. Waiting for abandoned_work.snapshot()["in_flight"] to drain would cover every case.
• The docstring says "tests_py/conftest.py redirects both before any import". That holds for SQLite, but DATABASE_URL is set at conftest.py:144, after _composition_root_wiring has been imported. It is harmless because ci_unit.sh exports the same URL and settings are read lazily, but "before any store is opened" would be exact.
• One reflowed line in CHANGELOG.md (line 92) is now about 130 columns; the surrounding lines wrap at about 80.

Verdict: approve

Review 2 notes:

• CHANGELOG line 92 (142 columns, introduced by b825723): rewrapped in fixup a514fcb.
• "redirects both before any import": now "points both at throwaway stores before any store is opened" (a514fcb).
• Teardown after a failed test: kept as is. The fixture is synchronous and runs after the client's loop has closed, so it cannot wait for abandoned_work to drain; that state is updated by callbacks on that loop. The remaining race only affects a run that is already red. A stronger guard would need an async fixture tied to the test's loop, which is not worth the code for a failure-only path.

Local: 2 passed; ruff 0.17.3 OK. CI not yet observed on a514fcb.

Generated by Claude Code

Claude finished @cdeust's task in 1m 30s —— View job

Review 3 of 3 (ADR-1119): PR #943

• Check the Blockers from review 2 (b825723): there were none.
• Find the commits pushed since then: only a514fcb, a fixup that changes the docstring and CHANGELOG wording.
• Run five reviews on a514fcb: code quality, test coverage, documentation accuracy, security, performance.
• Post the review: test(admission): a cancelled real remember keeps its slot and lands its row in the Cortex store (#728, slice 4) #943 (comment)

No Blockers. The fixup changes only the module docstring of the test file and rewraps one CHANGELOG line. No test logic or assertion changed. The new wording, "before any store is opened", matches tests_py/conftest.py (:62 and :144 run before the first connection at :178-181).

Accepted: the whole PR (CHANGELOG.md and tests_py/server/test_mcp_protocol_cancellation_remember.py, all three commits). No commit has an open Blocker, so nothing goes to a refinement PR.

The posted review has 2 Notes, neither needing action before merge.

Not checked: CI on a514fcb. The tests were not re-run.

Verdict: approve

Review 3 of 3 (ADR-1119): PR #943

Review 2 (b825723) found no Blockers. Since then one commit has been pushed, a514fcb (fixup). Only that commit was reviewed, through five reviews: code quality, test coverage, documentation accuracy, security and performance. No Blockers.

• Scope: I checked git diff b825723 a514fcb. It changes only the module docstring of tests_py/server/test_mcp_protocol_cancellation_remember.py (lines 1-15) and rewraps one paragraph in CHANGELOG.md, which fixes the ~130-column line from review 2. No test logic, assertion, constant or fixture changed.
• New wording: "points both at throwaway stores before any store is opened". In tests_py/conftest.py, CORTEX_MEMORY_DB_PATH is set at :62 and DATABASE_URL at :144, both when conftest is imported. The first connection is opened later, at :178-181, which is after both. The wording therefore answers review 2's ordering note.
• Security and performance: no secrets or PII. The change is documentation only, with no runtime cost.
• Not checked: CI on a514fcb. The tests were not re-run, which matters little for a docstring-only commit.

Accepted: the whole PR (4ef429d, b825723, a514fcb: CHANGELOG.md and tests_py/server/test_mcp_protocol_cancellation_remember.py). No file or commit has an open Blocker, so nothing needs a refinement PR.

Notes, no action before merge

• "Throwaway" is literal for SQLite, which gets a mkdtemp tree. For PostgreSQL the database is throwaway only because scripts/ci_unit.sh creates a fresh cluster, or because conftest creates a per-process database (conftest.py:138-142, which can be skipped with CORTEX_TEST_ISOLATE_DB=0 or CORTEX_TEST_DATABASE_URL).
• The source: quote at lines 25-26 still says "isolated Cortex SQLite store", while the docstring now says the CI store is PostgreSQL. It is accurate as a quote of the issue, but a reader may see the two as contradicting each other.

Verdict: approve