test(sync_middleware): sociable banner tests + decision_id alias fix

[jinhongkuan]

Summary

• Rewrites the get_session_start_banner tests in tests/test_sync_middleware.py from solitary (MagicMock ctx + AsyncMock ledger returning hand-crafted dicts) to sociable — they now seed a real SurrealDBLedgerAdapter over memory:// and exercise the actual get_decisions_by_status SQL.
• The first sociable run surfaced a latent UX bug: get_decisions_by_status selected decision_id from the decision table, but no such field is defined on that table (schema.py:114-154). Every banner row reached agents with decision_id: None. Fixed in ledger/adapter.py by aliasing type::string(id) AS decision_id — same pattern already used at queries.py:167, 228, 404, 512.
• Adds a "Sociable Testing for UX Paths" section to pilot/mcp/CLAUDE.md codifying the preference for handlers + ledger: SimpleNamespace ctx + real adapter; narrow seams only when the collaborator genuinely can't run in tests. Includes a checklist for tests-only PRs.

Why this matters

AI-authored tests skew solitary because mocks are easy to make pass. A solitary banner test stayed green for months while the production query returned null IDs. This PR is the worked example: same scenarios, same handler, real substrate — the bug fell out on the first run.

The non-banner tests (ensure_ledger_synced SHA-cache logic, repo_write_barrier concurrency primitives) intentionally keep their narrow seam patches: link_commit has its own end-to-end coverage, and the barrier tests are about pure asyncio behavior. The new CLAUDE.md guidance flags these as legitimate exceptions.

Test plan

• pytest tests/test_sync_middleware.py — 21/21 pass
• pytest tests/test_phase2_ledger.py tests/test_phase3_integration.py tests/test_link_commit_grounding.py tests/test_codegenome_continuity_service.py — 50/50 pass (no regression from the SQL alias)
• pytest tests/test_alpha_contract.py tests/test_alpha_flow.py — alpha_contract passes; two alpha_flow failures are pre-existing on main (verified by git stash + rerun) and unrelated to this PR
• CI green on PR

Out of scope

• Other solitary handler tests (test_ingest_size_limit.py, test_codegenome_adapter.py, etc.) — separate PRs, one at a time.
• Refactoring ensure_ledger_synced tests to use a real git repo — current narrow-seam approach is the right granularity for SHA-cache logic.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Fixed decision ID projection in session-start banner results to ensure proper status filtering and display.

• Tests

  • Refactored middleware tests from mock-based to real database environment for improved validation of decision retrieval, filtering, and banner generation.

• Documentation

  • Added testing standards for database-backed components, emphasizing real integration tests over mocks.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 99fd32a2-b922-4912-b251-4388284aee9b

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Introduces mandatory sociable testing policy via CLAUDE.md, fixes SurrealDB query aliasing in ledger adapter's get_decisions_by_status, and refactors sync_middleware tests from mock-based to real SurrealDB memory-backed end-to-end assertions.

Changes

Sociable Testing Refactor with Adapter Query Fix

Layer / File(s)	Summary
Testing Policy CLAUDE.md	Introduces mandatory "Sociable Testing for UX Paths" section defining rules: prefer real ledger adapter/client with seeded schema over MagicMock/AsyncMock, use SimpleNamespace for ctx, allow narrow patching only for hard-to-run boundaries, with validation checklist for tests-only PRs.
Adapter Query Fix ledger/adapter.py	get_decisions_by_status query now aliases SurrealDB record id as decision_id via type::string(id) AS decision_id instead of selecting non-existent field, ensuring proper decision-id values in banner results.
Test Infrastructure Setup tests/test_sync_middleware.py	Replaces mock-based substrate with real SurrealDB memory:// backend: _make_real_adapter() connects and initializes schema, _next_canonical() generates unique ids, _banner_ctx() and _ensure_ctx() provide lightweight real-ledger context, helpers seed drifted/ungrounded/proposal decisions.
Banner Test Suite tests/test_sync_middleware.py	All banner tests refactored to seed real ledger decisions and assert against actual query behavior: drifted/ungrounded decision counts, truncation at top 10 with drifted prioritized, per-session firing, exception swallowing, stale vs. fresh proposal detection. New test validates all open statuses surface in banner items.
Ensure Ledger Synced Tests tests/test_sync_middleware.py	Tests for ensure_ledger_synced refactored to use _ensure_ctx() with minimal SHA-cache state; validates calling/skipping handle_link_commit based on HEAD advancement and swallowing exceptions from handle_link_commit.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• BicameralAI/bicameral-mcp#56: Overlapping changes to ledger adapter's decision_id extraction and sync_middleware ledger interaction, moving tests to real SurrealDB and updating banner query behavior.

Poem

🐰 Hops through the test suite with joy!
Mock dittos fade, real queries stay,
The banner blooms with true SQLite dreams,
Each decision seeded, each assertion beams,
Sociable testing leads the way!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the two main changes: refactoring banner tests to use sociable testing approach and fixing a decision_id alias bug in the adapter.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/sociable-sync-middleware-tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/test_sync_middleware.py`:
- Around line 184-203: The test
test_banner_queries_each_open_status_actually_surfaces currently only seeds
drifted and ungrounded so it cannot verify context_pending reaches the banner;
either (A) seed a context_pending row before calling get_session_start_banner by
calling the existing helper (e.g. _seed_context_pending(client,
description="c1")) and update the final assertion to expect
{"drifted","ungrounded","context_pending"}, or (B) narrow the test claim by
changing the docstring and the final assertion to only assert the two seeded
statuses {"drifted","ungrounded"}; locate and modify the test function
test_banner_queries_each_open_status_actually_surfaces and the call to
get_session_start_banner to apply one of these fixes.
- Around line 18-29: The import block fails Ruff I001: reorder and format
imports into standard groups (stdlib, third-party, local) and alphabetize within
each group; specifically, group asyncio, datetime (UTC, datetime, timedelta),
pathlib.Path, types.SimpleNamespace, and unittest.mock (AsyncMock, MagicMock,
patch) as stdlib, then third-party pytest, and finally local imports from
handlers.sync_middleware (ensure_ledger_synced, get_session_start_banner) and
ledger modules (SurrealDBLedgerAdapter, LedgerClient, init_schema, migrate);
remove any unused imports and ensure the import style matches existing project
conventions so Ruff stops flagging I001.
- Around line 370-376: The test may short-circuit and never await
handle_link_commit because _LAST_SYNCED_SHA or the repo HEAD can equal the test
context's sha; update test_ensure_swallows_link_commit_exception to force the
"head advanced" branch by setting the module-level _LAST_SYNCED_SHA to a
different value than the repo HEAD (or otherwise mutate the test ctx so
ctx.repo.head.commit.hexsha[:7] != _LAST_SYNCED_SHA) before calling
ensure_ledger_synced; this guarantees ensure_ledger_synced will call and await
handlers.link_commit.handle_link_commit (the AsyncMock) so its RuntimeError
side_effect is exercised and swallowed.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3d1c2279-8485-44e9-ac44-726b6632f856

📥 Commits

Reviewing files that changed from the base of the PR and between b963dc8 and 0bc5356.

📒 Files selected for processing (3)

• CLAUDE.md
• ledger/adapter.py
• tests/test_sync_middleware.py

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Fix the import block so Ruff stops failing.

CI is already red on this block with I001, so this PR will not go green until the imports are re-sorted/formatted.

🧰 Tools

🪛 GitHub Actions: Lint & Type Check / 0_ruff + mypy.txt

[error] 16-29: Ruff check failed (I001): Import block is un-sorted or un-formatted. Organize imports; run with ruff check . --fix.

🪛 GitHub Actions: Lint & Type Check / ruff + mypy

[error] 16-29: Ruff import sorting/formatting failed (I001). Import block is un-sorted or un-formatted. Found 1 error. Help: Organize imports. Fixable with ruff check . --fix.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_sync_middleware.py` around lines 18 - 29, The import block fails
Ruff I001: reorder and format imports into standard groups (stdlib, third-party,
local) and alphabetize within each group; specifically, group asyncio, datetime
(UTC, datetime, timedelta), pathlib.Path, types.SimpleNamespace, and
unittest.mock (AsyncMock, MagicMock, patch) as stdlib, then third-party pytest,
and finally local imports from handlers.sync_middleware (ensure_ledger_synced,
get_session_start_banner) and ledger modules (SurrealDBLedgerAdapter,
LedgerClient, init_schema, migrate); remove any unused imports and ensure the
import style matches existing project conventions so Ruff stops flagging I001.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Either exercise context_pending here or narrow the test’s claim.

This test only seeds drifted and ungrounded, so it never proves the third queried “open” status still reaches the banner. If the context_pending path regresses, this stays green.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_sync_middleware.py` around lines 184 - 203, The test
test_banner_queries_each_open_status_actually_surfaces currently only seeds
drifted and ungrounded so it cannot verify context_pending reaches the banner;
either (A) seed a context_pending row before calling get_session_start_banner by
calling the existing helper (e.g. _seed_context_pending(client,
description="c1")) and update the final assertion to expect
{"drifted","ungrounded","context_pending"}, or (B) narrow the test claim by
changing the docstring and the final assertion to only assert the two seeded
statuses {"drifted","ungrounded"}; locate and modify the test function
test_banner_queries_each_open_status_actually_surfaces and the call to
get_session_start_banner to apply one of these fixes.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Force the head-advanced branch in the swallow test.

As written, this can pass without ever awaiting handle_link_commit if _LAST_SYNCED_SHA or the real repo HEAD short-circuits the function first. That makes the swallow path a false positive.

Suggested fix

-async def test_ensure_swallows_link_commit_exception():
+async def test_ensure_swallows_link_commit_exception(monkeypatch):
     ctx = _ensure_ctx()
+    monkeypatch.setattr("handlers.sync_middleware._LAST_SYNCED_SHA", "")

-    with patch("handlers.link_commit.handle_link_commit", new_callable=AsyncMock) as mock_lc:
+    with (
+        patch("handlers.link_commit._read_current_head_sha", return_value="new_sha"),
+        patch("handlers.link_commit.handle_link_commit", new_callable=AsyncMock) as mock_lc,
+    ):
         mock_lc.side_effect = RuntimeError("git not available")
         # Must not raise
         await ensure_ledger_synced(ctx)
+        mock_lc.assert_awaited_once()

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_sync_middleware.py` around lines 370 - 376, The test may
short-circuit and never await handle_link_commit because _LAST_SYNCED_SHA or the
repo HEAD can equal the test context's sha; update
test_ensure_swallows_link_commit_exception to force the "head advanced" branch
by setting the module-level _LAST_SYNCED_SHA to a different value than the repo
HEAD (or otherwise mutate the test ctx so ctx.repo.head.commit.hexsha[:7] !=
_LAST_SYNCED_SHA) before calling ensure_ledger_synced; this guarantees
ensure_ledger_synced will call and await handlers.link_commit.handle_link_commit
(the AsyncMock) so its RuntimeError side_effect is exercised and swallowed.