Skip to content

feat(phases 2-7): MCP tools, resources, transport, agent skills, EventBus SSE#392

Merged
jerry609 merged 82 commits into
devfrom
feat/mcp-phases-02-04
Mar 14, 2026
Merged

feat(phases 2-7): MCP tools, resources, transport, agent skills, EventBus SSE#392
jerry609 merged 82 commits into
devfrom
feat/mcp-phases-02-04

Conversation

@jerry609

@jerry609 jerry609 commented Mar 14, 2026

Copy link
Copy Markdown
Owner

Summary

  • Phase 2-3 (MCP Tools): Implement 8 MCP tools — paper search, analyze, judge, get research context, save to memory, search memory, scholar lookup, and tool listing
  • Phase 4 (MCP Resources): Add 4 MCP resources — scholars, track metadata, track papers, memory — with full registration in FastMCP server
  • Phase 5 (Transport Entry Point): Add paperbot-mcp serve CLI subcommand with stdio/SSE transport dispatch via serve.py
  • Phase 6 (Agent Skills): Create 4 SKILL.md agent skill files — literature review, paper reproduction, scholar monitoring, trend analysis
  • Phase 7 (EventBus + SSE Foundation): Implement EventBusEventLog (asyncio fan-out ring buffer) + GET /api/events/stream SSE endpoint for real-time event push to dashboard clients

Key Technical Details

  • EventBusEventLog: collections.deque(maxlen=200) ring buffer + per-client asyncio.Queue(maxsize=256) with drop-oldest backpressure — producer never blocks
  • SSE endpoint: StreamingResponse with 15s heartbeat, try/finally cleanup on disconnect, wired into CompositeEventLog as transparent backend
  • Zero breaking changes: All existing event_log.append() callers work unchanged; existing SSE endpoints untouched
  • Zero new dependencies: All primitives are stdlib or already in pyproject.toml

Test plan

  • 5 unit tests for EventBusEventLog (fan-out, ring buffer, backpressure, unsubscribe, composite wiring)
  • 2 integration tests for SSE endpoint (delivery within 1s, heartbeat on idle)
  • MCP tool/resource unit tests expanded (phases 2-4)
  • Existing e2e tests pass — no regressions
  • Manual: curl GET /api/events/stream during agent action — expect data: {...} within ~1s
  • Manual: Two simultaneous EventSource connections receive identical events
  • Manual: Idle SSE connection receives : keepalive after 15s

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Real-time SSE event stream and in-process event bus for live dashboard updates.
    • MCP server exposes nine tools (search, summarize, judge, relevance, trends, scholar lookup, research context, save-to-memory, export-to-Obsidian) and four read-only resources (track metadata, papers, memories, scholars).
    • CLI entry to run the MCP transport (stdio or HTTP).
  • Documentation

    • Expanded roadmap, milestones, architecture, migration plans, validation, and skill/workflow docs.
  • Tests

    • Broad unit and integration coverage for tools, resources, SSE, CLI, and audit logging.

jerry609 and others added 30 commits March 14, 2026 13:16
Plan 01 (W1): Audit helper + paper_search tool (R6.1, R6.2, R2.1)
Plan 02 (W2): LLM tools - paper_judge, paper_summarize, relevance_assess (R2.2-R2.4)
Plan 03 (W3): Integration tests + full regression check

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Create _audit.py with log_tool_call() for structured event logging
- Create paper_search.py wrapping PaperSearchService with audit integration
- Add 7 audit tests (event structure, run_id, graceful degradation, metrics, errors)
- Add 3 paper_search tests (dict results, empty results, audit logging)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…test

- Create server.py with FastMCP instance and paper_search registration
- Handle mcp package unavailability gracefully (Python 3.9 compat)
- Add bootstrap test verifying server imports and tool registration

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Create 02-01-SUMMARY.md with execution results and deviations
- Update STATE.md with phase position and decisions

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…vance_assess tools

- 4 tests for paper_judge: judgment dict, degraded mode, abstract->snippet mapping, audit logging
- 3 tests for paper_summarize: summary dict, degraded mode, audit logging
- 3 tests for relevance_assess: score/reason dict, fallback degraded, audit logging

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… MCP tools

- paper_judge wraps PaperJudge.judge_single() with abstract->snippet mapping
  and degraded detection when judge_model is empty
- paper_summarize wraps PaperSummarizer.summarize_item() with degraded
  detection on empty LLM output
- relevance_assess wraps RelevanceAssessor.assess() with fallback scoring
  detection when reason contains "Fallback"
- All three use anyio.to_thread.run_sync() for sync service calls
- All three log via log_tool_call() audit helper
- All 10 unit tests pass

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…in MCP server

- Server now registers all 4 tools: paper_search, paper_judge,
  paper_summarize, relevance_assess
- No circular imports, no regressions
- All 23 MCP tests pass (7 audit + 3 search + 4 judge + 3 summarize +
  3 relevance + 3 bootstrap)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- SUMMARY.md: 3 tools implemented with 10 TDD tests, all passing
- STATE.md: advanced to plan 03, recorded decisions and metrics

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…cation, and event logging

- 16 integration tests verifying all 4 tools are discoverable
- Schema tests validate parameter types, required/optional, defaults
- Invocation tests call tools through _impl layer with fake services
- Event logging tests verify workflow='mcp', stage='tool_call' on all 4 tools
- Comprehensive event structure test validates all tools in single run

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add 13 MCP requirements (MCP-01 through MCP-13) covering remaining
tools, resources, transport, and agent skills. Expand ROADMAP.md v1.0
section with phase details and success criteria.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…CP tools

- 3 tests for analyze_trends: normal, degraded, audit
- 3 tests for check_scholar: normal, degraded, audit
…ry, export_to_obsidian

- 3 tests for get_research_context: context pack dict, user_id/track_id passthrough, audit log
- 3 tests for save_to_memory: count return, invalid kind default, audit log
- 3 tests for export_to_obsidian: markdown key, frontmatter+title, audit log
- analyze_trends wraps sync TrendAnalyzer with anyio.to_thread.run_sync()
- analyze_trends returns degraded=True when LLM returns empty string
- check_scholar uses async SemanticScholarClient search_authors/get_author_papers
- check_scholar returns degraded=True when scholar not found
- Both tools use module-level lazy singleton pattern and log_tool_call audit
…o_obsidian MCP tools

- get_research_context: async wrapper around ContextEngine.build_context_pack() (offline=True default)
- save_to_memory: validates MemoryKind, wraps SqlAlchemyMemoryStore.add_memories() via anyio.to_thread
- export_to_obsidian: in-memory render via ObsidianFilesystemExporter._render_paper_note() + _yaml_frontmatter
- All three tools use module-level lazy singleton pattern and log_tool_call audit helper
- 9 tests now passing (3 per tool: normal, edge, audit)
- Add 03-01-SUMMARY.md for analyze_trends + check_scholar MCP tools
- Update STATE.md with decisions, metrics, and session info
- Update ROADMAP.md phase 3 progress (1/3 plans complete)
- Mark requirements MCP-01 and MCP-02 complete in REQUIREMENTS.md
…_obsidian plan

- 03-02-SUMMARY.md created: 3 MCP tools, 9 unit tests, TDD executed
- STATE.md updated: progress 63%, metrics recorded, decisions logged
- ROADMAP.md updated: phase 3 now 2/3 plans complete (In Progress)
- REQUIREMENTS.md: MCP-03, MCP-04, MCP-05 marked complete

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add imports for analyze_trends, check_scholar, get_research_context,
  save_to_memory, and export_to_obsidian
- Call register(mcp) for all 5 new tools after existing 4
- Total: 9 tools registered in single FastMCP server
- Update EXPECTED_TOOLS from 4 to 9 tools
- Add fakes for 5 new tools: _FakeTrendAnalyzer, _FakeS2Client,
  _FakeContextEngine, _FakeMemoryStore, _FakeExporter
- Replace test_all_four_tools_listed with test_all_nine_tools_listed
- Replace test_server_registers_all_four_tools with test_server_registers_all_nine_tools
- Add 5 schema tests (analyze_trends, check_scholar, get_research_context,
  save_to_memory, export_to_obsidian)
- Add 5 invocation tests via _impl functions
- Add 5 event logging tests
- Update test_all_tool_events_have_consistent_structure to 9 tools
- Total: 31 integration tests, all passing
- Add 03-03-SUMMARY.md: 9-tool FastMCP server wiring complete
- Update STATE.md: progress 75%, session record, decision logged
- Update ROADMAP.md: phase 03 marked Complete (3/3 plans)
jerry609 and others added 10 commits March 14, 2026 14:56
- 5 unit tests: fan-out, ring buffer catch-up, backpressure, unsubscribe, composite wiring
- 2 integration stubs (skipped): SSE delivery and heartbeat (Plan 07-02 wires endpoint)
- RED state confirmed: ModuleNotFoundError for event_bus_event_log module
- collections.deque(maxlen=200) ring buffer with configurable size
- asyncio.Queue(maxsize=256) per-client with configurable size
- drop-oldest backpressure: get_nowait then put_nowait on full queue
- subscribe() pre-loads queue with ring buffer catch-up burst
- unsubscribe() removes queue from fan-out set (idempotent via discard)
- _fan_out() iterates list(self._queues) snapshot for concurrency safety
- append() serializes AgentEventEnvelope via .to_dict() once only
- stream() returns iter(()) — no run_id replay supported
- close() clears _queues set
- No await inside append() or _fan_out() — pure synchronous
- All 5 unit tests pass (GREEN)
…kend

- SUMMARY.md: EventBusEventLog with drop-oldest backpressure + ring buffer catch-up
- STATE.md: plan metrics, 3 key decisions, session updated
- ROADMAP.md: phase 7 progress updated (1/2 plans complete)
- REQUIREMENTS.md: EVNT-04 marked complete
- APIRouter with prefix=/events; single route GET /stream
- _get_bus() helper: finds EventBusEventLog in _backends or directly on event_log
- _event_generator(): subscribe → drain queue with 15s heartbeat → unsubscribe in finally
- Heartbeat: yield sse_comment() on asyncio.TimeoutError (no events in 15s)
- Disconnect: checks request.is_disconnected() each iteration, catches CancelledError
- No wrap_generator(): events carry own AgentEventEnvelope fields already
- Late import of EventBusEventLog inside _get_bus() avoids circular import
…sts GREEN

main.py changes:
- Import EventBusEventLog from infrastructure.event_log
- Import events route module as events_route
- _startup_eventlog(): add EventBusEventLog as third CompositeEventLog backend
- Register events_route.router at prefix=/api with Events tag

tests/integration/test_events_sse_endpoint.py:
- Replace pytest.skip stubs with real passing tests
- test_event_delivered_within_1s: CompositeEventLog.append() → queue.get() < 1s
- test_heartbeat_on_idle: patches _HEARTBEAT_SECONDS=0.05s, confirms keepalive comment

All 7 tests pass (5 unit + 2 integration). e2e test unaffected.
- 07-02-SUMMARY.md: full plan summary with self-check PASSED
- STATE.md: metrics, decisions, session updated; progress recalculated
- ROADMAP.md: phase 7 marked Complete (2/2 plans)
Addresses review feedback on MCP phases 2-4: validate save_to_memory
confidence, align context route auth, harden resource contracts,
expand unit/integration test coverage for all MCP tools and resources.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@jerry609 jerry609 changed the title feat(mcp): implement phases 02-04 — tools, resources, server registration feat(phases 2-7): MCP tools, resources, transport, agent skills, EventBus SSE Mar 14, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds a first-class MCP server surface (tools + resources + CLI transport) and introduces an in-process EventBus + SSE endpoint to stream agent/events to dashboard clients in real time.

Changes:

  • Added MCP server package with tools/resources, audit logging, and paperbot mcp serve (stdio / streamable-http).
  • Implemented EventBusEventLog fan-out ring buffer and /api/events/stream SSE endpoint.
  • Updated tests to use authenticated user dependency overrides and added broad unit/integration coverage for MCP + SSE.

Reviewed changes

Copilot reviewed 104 out of 105 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tests/unit/test_research_context_route_explicit_track.py Updates tests to use get_required_user_id dependency override instead of passing user_id.
tests/integration/test_research_track_context_routes.py Same auth override pattern for integration routes.
tests/integration/test_events_sse_endpoint.py Adds SSE/EventBus integration tests (delivery + heartbeat).
src/paperbot/presentation/cli/main.py Adds paperbot mcp serve CLI dispatch.
src/paperbot/mcp/* Introduces MCP server, tools, resources, and transport entry points.
src/paperbot/infrastructure/event_log/event_bus_event_log.py Adds fan-out ring buffer event bus backend for SSE.
src/paperbot/api/routes/events.py Adds /api/events/stream SSE endpoint backed by EventBus.
src/paperbot/api/main.py Wires events router and adds EventBus backend to CompositeEventLog.
src/paperbot/application/services/llm_service.py Adds explicit fallback=True flag for relevance fallback path.
pyproject.toml / requirements.txt Adds mcp[fastmcp] dependency + script entry.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Comment thread pyproject.toml Outdated
Comment on lines +60 to +62
"bcrypt>=4.0.0",
"email-validator>=2.0.0",
"mcp[fastmcp]>=1.8.0,<2.0.0",
Comment thread src/paperbot/api/main.py Outdated
Comment on lines 110 to 118
bus = EventBusEventLog()
app.state.event_log = CompositeEventLog([
LoggingEventLog(),
SqlAlchemyEventLog(),
bus,
])
except Exception:
# If SQLAlchemy isn't available or DB init fails, fall back to logging only.
app.state.event_log = LoggingEventLog()
Comment on lines +108 to +115
q: asyncio.Queue = asyncio.Queue(maxsize=self._client_queue_size)

# Catch-up burst: deliver ring buffer in order (deque is oldest-first)
for event in list(self._ring):
self._put_nowait_drop_oldest(q, event)

self._queues.add(q)
return q

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Note

Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/paperbot/api/main.py (1)

122-124: ⚠️ Potential issue | 🟠 Major

Missing event_log.close() in shutdown handler.

The shutdown handler only calls obsidian.shutdown_obsidian_runtime() but does not close the event log. Per the EventBusEventLog.close() implementation (context snippet from event_bus_event_log.py:92-94), this method clears subscriber queues to properly disconnect SSE clients. Without calling it, subscriber queues may leak during graceful shutdown.

Proposed fix
 `@app.on_event`("shutdown")
 async def _shutdown_obsidian_runtime():
+    if hasattr(app.state, "event_log"):
+        app.state.event_log.close()
     obsidian.shutdown_obsidian_runtime(app)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/paperbot/api/main.py` around lines 122 - 124, The shutdown handler
_shutdown_obsidian_runtime currently only calls
obsidian.shutdown_obsidian_runtime(app) but must also close the event log to
clear subscriber queues; after calling obsidian.shutdown_obsidian_runtime(app)
invoke EventBusEventLog.close() on the running event log instance (e.g. call
event_log.close() or obtain it from the obsidian module/manager if it's stored
there) so subscriber queues are cleared during graceful shutdown.
♻️ Duplicate comments (2)
src/paperbot/mcp/tools/save_to_memory.py (1)

52-53: ⚠️ Potential issue | 🟠 Major

Require explicit user_id; avoid shared "default" identity.

Line 52 and Line 152 still default to "default", which can mix unrelated users’ memories in the same scope.

Suggested fix
 async def _save_to_memory_impl(
     content: str,
     kind: str = "note",
-    user_id: str = "default",
+    user_id: str = "",
@@
 ) -> Dict[str, Any]:
@@
+    if not user_id or user_id == "default":
+        raise ValueError("user_id is required and must not be 'default'")
@@
     async def save_to_memory(
         content: str,
         kind: str = "note",
-        user_id: str = "default",
+        user_id: str = "",
@@
     ) -> dict:

Also applies to: 152-153

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/paperbot/mcp/tools/save_to_memory.py` around lines 52 - 53, The function
in src/paperbot/mcp/tools/save_to_memory.py currently uses a shared literal
default user_id ("default") for the user_id parameter (and similarly for the
scope_type default), which can mix different users' memories; update the
function signature (the save_to_memory function or equivalent that declares
user_id: str = "default", scope_type: str = "global") to require an explicit
user_id (remove the "default" default or set user_id: Optional[str] = None) and
add runtime validation that raises a clear error (e.g., ValueError) if user_id
is missing or equals "default"; do the same for scope_type (avoid using "global"
as an implicit identity or validate allowed values) and fix the other occurrence
noted (the second default instance) so no code path uses the shared "default"
value.
.planning/STATE.md (1)

3-14: ⚠️ Potential issue | 🟡 Minor

YAML frontmatter still inconsistent with document body.

The frontmatter states milestone: v1.0 and total_phases: 15, but the body (line 24) says "Current focus: v1.1" and line 28 says "Phase: 7 of 17". Consider aligning these values:

 milestone: v1.0
-milestone_name: MCP Server
+milestone_name: Agent Orchestration Dashboard
 status: planning
 stopped_at: Completed 07-02-PLAN.md
 last_updated: "2026-03-14T06:52:51.697Z"
 last_activity: 2026-03-14 -- v2.0 roadmap created (phases 12-17)
 progress:
-  total_phases: 15
+  total_phases: 17
   completed_phases: 5
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.planning/STATE.md around lines 3 - 14, Frontmatter values (milestone,
total_phases, completed_phases, percent) are inconsistent with the document body
("Current focus" and "Phase"); update the YAML keys (milestone, total_phases,
completed_phases, percent) to match the body’s current focus and phase count (or
vice versa) so the frontmatter and body are synchronized—specifically reconcile
milestone with "Current focus" and total_phases/completed_phases/percent with
the "Phase: X of Y" and completed counts shown in the body.
🟡 Minor comments (20)
.planning/phases/06-agent-skills/06-RESEARCH.md-21-21 (1)

21-21: ⚠️ Potential issue | 🟡 Minor

Capitalize “Markdown” in docs text.

Use the proper noun form for consistency with the rest of the document and common style guides.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.planning/phases/06-agent-skills/06-RESEARCH.md at line 21, Update the
phrase "Phase 6 is purely a content-creation phase. No Python code is written.
The deliverable is four SKILL.md files placed in
`.claude/skills/{skill-name}/SKILL.md`." by capitalizing the word "Markdown"
wherever "markdown" appears in the document (specifically in the description of
the SKILL.md files and workflow body), so the proper noun form "Markdown" is
used consistently; edit the line containing "SKILL.md files" and any occurrences
in the surrounding paragraph to replace "markdown" with "Markdown".
.planning/phases/06-agent-skills/06-RESEARCH.md-531-540 (1)

531-540: ⚠️ Potential issue | 🟡 Minor

Replace machine-local absolute paths with repo-relative paths.

Hardcoded /home/master1/... paths are not reproducible for other contributors. Use repository-relative paths (or clearly label them as local-only examples) so the research artifact remains portable.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.planning/phases/06-agent-skills/06-RESEARCH.md around lines 531 - 540,
Replace the hardcoded machine-local absolute paths (entries starting with
"/home/master1/") in the research doc with repository-relative paths (e.g.,
src/paperbot/mcp/server.py, src/paperbot/mcp/tools/*.py) or explicitly mark each
as "local-only example" if you intend to keep them; update the lines that list
those absolute paths so they use portable repo-relative references or a clear
local-only label to make the artifact reproducible for other contributors.
.planning/phases/06-agent-skills/06-VALIDATION.md-59-62 (1)

59-62: ⚠️ Potential issue | 🟡 Minor

Manual verification needs explicit pass/fail acceptance criteria.

“Verify skill is loaded and workflow steps execute” is too subjective for consistent sign-off. Add concrete expected artifacts (e.g., specific log/event output, named skill ID loaded, and expected response sections) so different reviewers can produce the same verdict.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.planning/phases/06-agent-skills/06-VALIDATION.md around lines 59 - 62,
Update the "Verify skill is loaded and workflow steps execute" test instruction
to include explicit pass/fail criteria: specify the exact skill ID or filename
under `.claude/skills/` that must be loaded (e.g.,
"skill:paperbot-literature-review"), the expected log/event output (e.g.,
"Loaded skill: <skill-id>" and workflow step completion events such as "step 1:
fetch_sources completed"), and the expected response format sections (e.g.,
"Summary", "Key papers", "Notes"). Replace the subjective phrase in the table
cell with these concrete checks so reviewers can assert PASS only when the named
skill ID is observed in logs, the listed workflow events appear, and the agent
response contains the required sections.
.planning/phases/05-transport-entry-point/05-RESEARCH.md-17-17 (1)

17-17: ⚠️ Potential issue | 🟡 Minor

Fix malformed table termination in <phase_requirements>.

Line 17 is being parsed as a table row (MD055/MD056). Add a blank line before </phase_requirements> so the table closes cleanly.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.planning/phases/05-transport-entry-point/05-RESEARCH.md at line 17, The
closing tag </phase_requirements> is being parsed as a table row because the
Markdown table isn't terminated; open the file section containing
<phase_requirements> and insert a blank line immediately before the closing
</phase_requirements> tag so the table ends properly (i.e., ensure there is a
blank line between the last table row and </phase_requirements>).
.planning/phases/05-transport-entry-point/05-RESEARCH.md-56-56 (1)

56-56: ⚠️ Potential issue | 🟡 Minor

Address markdownlint structure issues for fences/tables.

Line 56 needs a language tag on the fenced block (MD040), and tables near Line 36, Line 458, and Line 466 need surrounding blank lines (MD058) for consistent rendering.

Also applies to: 36-36, 458-458, 466-466

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.planning/phases/05-transport-entry-point/05-RESEARCH.md at line 56, Add a
language tag to the fenced code block that starts with triple backticks on the
block around Line 56 (replace ``` with ```<language> e.g., ```text or ```bash as
appropriate) and ensure each Markdown table near the noted areas (the table
around Line 36 and the tables near Lines 458 and 466) has a blank line before
and after it so tables are separated by surrounding blank lines; update the
blocks in the document where you find the triple-backtick fence and the three
table blocks to satisfy MD040 and MD058 respectively.
.planning/phases/03-remaining-mcp-tools/03-01-SUMMARY.md-122-123 (1)

122-123: ⚠️ Potential issue | 🟡 Minor

Use the repo’s configured emphasis style for footer markers.

Line 122 and Line 123 use *...*, but markdownlint MD049 expects underscore emphasis.

Suggested fix
-*Phase: 03-remaining-mcp-tools*
-*Completed: 2026-03-14*
+_Phase: 03-remaining-mcp-tools_
+_Completed: 2026-03-14_
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.planning/phases/03-remaining-mcp-tools/03-01-SUMMARY.md around lines 122 -
123, Replace the asterisk-based emphasis used in the footer markers and change
"*Phase: 03-remaining-mcp-tools*" and "*Completed: 2026-03-14*" to underscore
emphasis (e.g., _Phase: 03-remaining-mcp-tools_ and _Completed: 2026-03-14_) so
the file conforms to the repo’s configured MD049 rule; update the two footer
marker instances in .planning/phases/03-remaining-mcp-tools/03-01-SUMMARY.md
accordingly.
.planning/phases/03-remaining-mcp-tools/03-UAT.md-14-21 (1)

14-21: ⚠️ Potential issue | 🟡 Minor

Update UAT tool name: relevance should be relevance_assess.

The decorated MCP tool in relevance.py (line 109) is named relevance_assess, not relevance. Lines 14 and 20 in the UAT file must be updated to list relevance_assess to match the actual registered tool name.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.planning/phases/03-remaining-mcp-tools/03-UAT.md around lines 14 - 21, The
UAT expects the tool name "relevance" but the actual decorated MCP tool in
relevance.py is named relevance_assess; update the UAT documentation and test
expectation to use relevance_assess instead of relevance (replace occurrences in
the tool list and the test description), and verify the MCP registration in
server.py still registers relevance_assess (search for the relevance_assess
symbol and the .register(mcp) calls to confirm all 9 tools are listed).
.planning/phases/03-remaining-mcp-tools/03-01-SUMMARY.md-80-81 (1)

80-81: ⚠️ Potential issue | 🟡 Minor

Escape underscored identifiers to avoid emphasis parsing errors.

Line 80 and Line 81 trigger markdownlint MD037 due bare _analyzer / _client tokens in prose.

Suggested fix
-- `src/paperbot/mcp/tools/analyze_trends.py` - analyze_trends MCP tool: lazy singleton _analyzer, anyio thread wrapping, degraded detection, log_tool_call audit
-- `src/paperbot/mcp/tools/check_scholar.py` - check_scholar MCP tool: lazy singleton _client, async S2 client calls, not-found degraded path, log_tool_call audit
+- `src/paperbot/mcp/tools/analyze_trends.py` - analyze_trends MCP tool: lazy singleton `_analyzer`, anyio thread wrapping, degraded detection, log_tool_call audit
+- `src/paperbot/mcp/tools/check_scholar.py` - check_scholar MCP tool: lazy singleton `_client`, async S2 client calls, not-found degraded path, log_tool_call audit
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.planning/phases/03-remaining-mcp-tools/03-01-SUMMARY.md around lines 80 -
81, The markdown contains bare underscored identifiers `_analyzer` and `_client`
causing MD037; update the prose in
.planning/phases/03-remaining-mcp-tools/03-01-SUMMARY.md to escape these tokens
(e.g., wrap `_analyzer` and `_client` in inline code backticks or escape the
underscores) where the analyze_trends MCP tool and check_scholar MCP tool are
described so the identifiers in the lines referencing
src/paperbot/mcp/tools/analyze_trends.py and
src/paperbot/mcp/tools/check_scholar.py render correctly without triggering lint
warnings.
.planning/research/PITFALLS.md-89-93 (1)

89-93: ⚠️ Potential issue | 🟡 Minor

Add fence languages for Markdown code blocks (MD040).

At Line 89 and Line 328, fenced blocks are missing language specifiers.

🔧 Proposed fix
-```
+```text
 psycopg2.errors.DatatypeMismatch: column "payload_json" is of type jsonb
 but expression is of type text.
 HINT: You might need to add an explicit cast.

...
- +text
SAWarning: Did not recognize type 'vector' of column 'embedding'

Also applies to: 328-330

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.planning/research/PITFALLS.md around lines 89 - 93, Two fenced code blocks
missing language specifiers; add a language tag (use "text") to each code fence
so Markdown lint MD040 is satisfied: update the first fenced block containing
the psycopg2.errors.DatatypeMismatch message to start with ```text instead of
``` and update the second fenced block containing the SAWarning about type
'vector' of column 'embedding' to start with ```text as well.
tests/unit/test_agent_skills.py-51-55 (1)

51-55: ⚠️ Potential issue | 🟡 Minor

Require both frontmatter delimiters when parsing SKILL.md.

At Line 51, the guard allows a single --- delimiter (len(parts) == 2), so malformed frontmatter can slip through parsing instead of failing structurally.

🔧 Proposed fix
-    if len(parts) < 2:
-        raise ValueError(f"{skill_name}/SKILL.md: no YAML frontmatter found (missing --- delimiters)")
+    if len(parts) < 3:
+        raise ValueError(
+            f"{skill_name}/SKILL.md: invalid YAML frontmatter (missing opening/closing --- delimiters)"
+        )
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/unit/test_agent_skills.py` around lines 51 - 55, The current parser
accepts a single '---' delimiter because it only checks if len(parts) < 2,
allowing malformed frontmatter to be treated as parts[1]; update the guard to
require both opening and closing frontmatter delimiters by checking that
len(parts) >= 3 and raising a ValueError (with the same "{skill_name}/SKILL.md:
no YAML frontmatter found (missing --- delimiters)" message) when not met, then
continue setting frontmatter = yaml.safe_load(parts[1]) and body = parts[2] if
present.
.planning/ROADMAP.md-303-304 (1)

303-304: ⚠️ Potential issue | 🟡 Minor

Fix misaligned progress-table values for Phases 6 and 7.

At Line 303 and Line 304, column values are shifted (milestone/plans/status are not in the header’s column order), which makes the status report ambiguous.

🔧 Proposed fix
-| 6. Agent Skills | 1/1 | Complete   | 2026-03-14 | - |
-| 7. EventBus + SSE Foundation | 2/2 | Complete   | 2026-03-14 | - |
+| 6. Agent Skills | v1.0 | 1/1 | Complete | 2026-03-14 |
+| 7. EventBus + SSE Foundation | v1.1 | 2/2 | Complete | 2026-03-14 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.planning/ROADMAP.md around lines 303 - 304, The two table rows for "6.
Agent Skills" and "7. EventBus + SSE Foundation" have their column values
shifted and do not match the table header order; update those rows so each
pipe-separated cell aligns with the header (move milestone/plans/status values
into their correct columns for the "6. Agent Skills" and "7. EventBus + SSE
Foundation" rows) so the Phase, Milestone/Plans, Status, Date and Notes cells
are in the same order as the table header.
tests/integration/test_research_track_context_routes.py-115-115 (1)

115-115: ⚠️ Potential issue | 🟡 Minor

Use .pop() to remove only the overridden dependency this test sets.

At Line 115 and Line 150, replace .clear() with .pop(auth_deps.get_required_user_id, None) to explicitly clean up only the override set by this test. This improves clarity and follows the principle of deterministic tests—each test should manage only its own state.

🔧 Proposed fix
-        app.dependency_overrides.clear()
+        app.dependency_overrides.pop(auth_deps.get_required_user_id, None)

As per coding guidelines tests/**/*.py: "Prefer deterministic tests and mock external APIs when possible."

Also applies to: 150-150

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/integration/test_research_track_context_routes.py` at line 115, Replace
the blanket teardown call app.dependency_overrides.clear() with removing only
the override this test sets by calling
app.dependency_overrides.pop(auth_deps.get_required_user_id, None); update both
occurrences around the test (the lines using app.dependency_overrides.clear) so
the test only cleans up its override for auth_deps.get_required_user_id and does
not wipe other tests' overrides. Ensure auth_deps.get_required_user_id is the
exact symbol referenced in the test when calling pop.
tests/unit/test_mcp_save_to_memory.py-127-127 (1)

127-127: ⚠️ Potential issue | 🟡 Minor

Escape regex metacharacters in match= for strict error assertion.

At Line 127, . in 0.0 and 1.0 are regex wildcards, so the assertion can pass on unintended messages like "confidence must be between 0x0 and 1x0".

🔧 Proposed fix
-            with pytest.raises(ValueError, match="confidence must be between 0.0 and 1.0"):
+            with pytest.raises(ValueError, match=r"confidence must be between 0\.0 and 1\.0"):
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/unit/test_mcp_save_to_memory.py` at line 127, The pytest.raises
assertion in tests/unit/test_mcp_save_to_memory.py uses a plain string for match
which treats '.' as regex wildcards; update the match to a properly escaped
regex (e.g., use r"confidence must be between 0\.0 and 1\.0" or generate the
pattern with re.escape("confidence must be between 0.0 and 1.0")) so the test
strictly matches the literal message in the ValueError raised by the code under
test.
.planning/REQUIREMENTS.md-210-213 (1)

210-213: ⚠️ Potential issue | 🟡 Minor

Coverage summary conflicts with checkbox states.

The summary states "v1.0 requirements: 17 total (4 shipped, 13 remaining)", but all v1.0 requirements (lines 12-41) are marked as complete with [x]. The traceability table (lines 150-168) also shows all MCP requirements as "Complete".

Consider updating the coverage summary to reflect the actual state:

 **Coverage:**
-- v1.0 requirements: 17 total (4 shipped, 13 remaining)
+- v1.0 requirements: 17 total (17 shipped, 0 remaining)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.planning/REQUIREMENTS.md around lines 210 - 213, The Coverage summary under
the "Coverage:" header is inconsistent with the v1.0 checklist and traceability
table: update the summary line "v1.0 requirements: 17 total (4 shipped, 13
remaining)" to match the actual state shown by the checkboxes and traceability
table (e.g., "v1.0 requirements: 17 total (17 shipped, 0 remaining)"), or
alternatively uncheck any requirements that are not actually shipped; ensure the
single summary string is corrected to reflect the true shipped/remaining counts
so the document is internally consistent.
.planning/STATE.md-62-67 (1)

62-67: ⚠️ Potential issue | 🟡 Minor

Broken markdown table structure.

Lines 65-67 appear to be table rows but are placed after a list item, breaking the table structure. They should either be part of the "By Phase" table above or formatted consistently.

 **Recent Trend:**
 - Last 3 plans: 3min, 2min, 3min
 - Trend: Stable
-| Phase 06-agent-skills P01 | 3 | 2 tasks | 5 files |
-| Phase 07-eventbus-sse-foundation P01 | 3 | 2 tasks | 3 files |
-| Phase 07 P02 | 4 | 2 tasks | 3 files |

Either move these rows into the "By Phase" table (lines 52-60) or convert them to list items.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.planning/STATE.md around lines 62 - 67, The markdown has table rows (Phase
06-agent-skills..., Phase 07-eventbus..., Phase 07 P02) placed after a list,
breaking the "By Phase" table; move those three rows into the existing "By
Phase" table block (the header and rows starting around the "By Phase" section)
so they become proper table rows, or if you prefer lists, convert each of the
three lines into list items under the preceding list — update the "By Phase"
table contents (or the list) accordingly to restore consistent markdown
table/list structure.
.planning/phases/05-transport-entry-point/05-01-PLAN.md-131-144 (1)

131-144: ⚠️ Potential issue | 🟡 Minor

Switch these indented snippets to fenced code blocks.

markdownlint is flagging MD046 on both sections, and nested code/examples inside the ordered list are easy for renderers to mangle. Using fenced python / toml / shell blocks here will keep the document lint-clean and render consistently.

Also applies to: 186-233

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.planning/phases/05-transport-entry-point/05-01-PLAN.md around lines 131 -
144, The markdown lists in
.planning/phases/05-transport-entry-point/05-01-PLAN.md contain indented
code/examples that trigger MD046; replace those indented snippets (including the
Python examples for src/paperbot/mcp/serve.py — descriptions of run_stdio() and
run_http() and the suggested from __future__ import annotations/docstrings — and
the TOML block describing the [project.scripts] entry and dependencies) with
fenced code blocks using the appropriate language tags (python and toml) so the
examples render consistently and pass markdownlint; ensure the fenced blocks
enclose the exact snippets referenced in the diff and also update the other
occurrence noted (lines ~186-233) similarly.
.planning/phases/07-eventbus-sse-foundation/07-02-PLAN.md-206-217 (1)

206-217: ⚠️ Potential issue | 🟡 Minor

Fence this sample code so Markdown stops treating it as syntax.

The [0] lookup and _event_generator identifier are currently being parsed as Markdown, which is why markdownlint is reporting MD052/MD037 here. Put this example in a fenced python block, or at least wrap the expressions in backticks.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.planning/phases/07-eventbus-sse-foundation/07-02-PLAN.md around lines 206 -
217, The markdown sample is being parsed as code causing lint errors; wrap the
example lines that reference _startup_eventlog(), the EventBusEventLog lookup
([0]), bus.subscribe(), bus.append(...), _event_generator and sse_comment() in a
fenced code block (```python ... ```) or at minimum inline backticks around
identifiers and expressions so the `[0]` index and `_event_generator` are not
interpreted as markdown/links; update the snippet in 07-02-PLAN.md to use a
fenced python block for the whole example (or backtick each identifier) to
resolve MD052/MD037.
.planning/phases/07-eventbus-sse-foundation/07-01-PLAN.md-186-186 (1)

186-186: ⚠️ Potential issue | 🟡 Minor

Use repo-relative paths in these verification commands.

The hardcoded /home/master1/PaperBot/... paths make the checks fail anywhere except one workstation. PYTHONPATH=src pytest tests/... (or $PWD-based paths) keeps the plan executable in CI and on other dev boxes.

Also applies to: 228-228

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.planning/phases/07-eventbus-sse-foundation/07-01-PLAN.md at line 186,
Replace the hardcoded absolute paths in the automated verification command (the
line containing "PYTHONPATH=/home/master1/PaperBot/src pytest
/home/master1/PaperBot/tests/unit/test_event_bus_event_log.py -q ...") with
repo-relative paths so the check runs in CI and other workstations; for example
change to "PYTHONPATH=src pytest tests/unit/test_event_bus_event_log.py -q ..."
(or use "$PWD" based paths) and make the same edit for the other occurrence
noted around line 228 to ensure all automated commands use repo-relative paths.
.planning/research/ARCHITECTURE.md-289-291 (1)

289-291: ⚠️ Potential issue | 🟡 Minor

Complete ContextVar lifecycle in ARQ job scoping.

Line 290 sets _db_session_context but no reset path is shown. Add on_job_end (or equivalent) to reset the token; otherwise context can leak across jobs in long-lived workers.

Also applies to: 299-300

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.planning/research/ARCHITECTURE.md around lines 289 - 291, The on_job_start
function sets the ContextVar _db_session_context but never resets it, which can
leak context across jobs; modify on_job_start to save the returned token (token
= _db_session_context.set(...)) and implement a matching on_job_end (or ensure
in the job teardown path) that calls _db_session_context.reset(token) to restore
previous state; reference the functions on_job_start and on_job_end and the
ContextVar _db_session_context so the token is stored and reset in the job
lifecycle.
.planning/research/ARCHITECTURE.md-507-513 (1)

507-513: ⚠️ Potential issue | 🟡 Minor

Phase A deliverable claim is too absolute for known feature gaps.

Line 513 says “PG works with existing sync stores,” but the doc also states FTS/vector behavior degrades on PG (Line 50–51). Reword this milestone to clarify “core CRUD parity” vs “search/embedding parity.”

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.planning/research/ARCHITECTURE.md around lines 507 - 513, Update the Phase
A milestone language under "Phase A — PostgreSQL + Schema (sync stays)" by
replacing the absolute claim "PG works with existing sync stores" with a scoped
statement that distinguishes core CRUD parity from search/embedding feature
gaps—for example: "PG provides core CRUD parity with existing sync stores; full
search (FTS5) and vector/embedding functionality may degrade or require
PG-specific solutions and will be addressed in later phases." Ensure the updated
sentence mentions both the core CRUD parity and the known FTS/vector limitations
so readers see the distinction.
🧹 Nitpick comments (6)
.planning/phases/06-agent-skills/06-RESEARCH.md (1)

483-485: Frontmatter parsing example is brittle with split("---", 2).

This sample parser can misparse when --- appears unexpectedly in content. Prefer extracting only the leading YAML block explicitly (e.g., anchored regex or a dedicated frontmatter parser) to avoid false parses in test code.

Proposed doc snippet update
-    # Strip leading/trailing --- delimiters
-    parts = content.split("---", 2)
-    frontmatter = yaml.safe_load(parts[1])
-    body = parts[2] if len(parts) > 2 else ""
+    # Parse only leading YAML frontmatter block
+    import re
+    m = re.match(r"^---\n(.*?)\n---\n?(.*)$", content, re.DOTALL)
+    assert m, f"{skill_name}: invalid or missing YAML frontmatter"
+    frontmatter = yaml.safe_load(m.group(1))
+    body = m.group(2)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.planning/phases/06-agent-skills/06-RESEARCH.md around lines 483 - 485, The
current frontmatter extraction using parts = content.split("---", 2) and
assigning frontmatter = yaml.safe_load(parts[1]) is brittle because it splits on
any '---' in the file; update the parser to explicitly extract only a leading
YAML frontmatter block (or use a dedicated frontmatter library) instead of blind
splitting. Replace the split-based logic that sets parts/frontmatter/body with a
targeted extraction: detect an opening '---' at the file start and capture up to
the next closing '---' (e.g., anchored, DOTALL non-greedy regex) or call a
frontmatter parser, then pass the captured YAML to yaml.safe_load and set body
to the remainder; ensure you handle the case where no leading frontmatter is
present so frontmatter stays None and body remains the full content.
.planning/phases/06-agent-skills/06-VALIDATION.md (1)

22-33: Prefer cross-platform test commands over inline PYTHONPATH=src.

These commands are shell-dependent and can fail on non-POSIX environments. Consider documenting python -m pytest ... and moving pythonpath = ["src"] into pyproject.toml to keep commands portable and consistent.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.planning/phases/06-agent-skills/06-VALIDATION.md around lines 22 - 33,
Replace the shell-dependent inline env commands (`PYTHONPATH=src pytest
tests/unit/test_agent_skills.py -q` and `PYTHONPATH=src pytest -q`) with
cross-platform invocations (`python -m pytest tests/unit/test_agent_skills.py
-q` and `python -m pytest -q`) in the Quick run, Full suite and Sampling Rate
sections, and add a persistent Python path setting to the project configuration
by adding pythonpath = ["src"] under the pytest config in pyproject.toml (or
equivalent [tool.pytest.ini_options]) so tests run portably across non-POSIX
environments.
.planning/research/FEATURES.md (1)

77-109: Add language specifier to fenced code block.

The dependency diagram code block is missing a language specifier. Adding text or plaintext would silence the MD040 warning and improve accessibility.

📝 Proposed fix
-```
+```text
 Docker Compose (PostgreSQL local dev)
   |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.planning/research/FEATURES.md around lines 77 - 109, The fenced dependency
diagram in the FEATURES.md document (the triple-backtick block that begins with
"Docker Compose (PostgreSQL local dev)") lacks a language specifier; fix it by
changing the opening fence from ``` to ```text (or ```plaintext) so the code
block becomes fenced with a language tag to satisfy MD040 and improve
accessibility.
src/paperbot/mcp/tools/save_to_memory.py (1)

36-47: Add explicit store typing for cleaner pyright checks.

_store and _get_store() are untyped. Adding concrete types improves static checking and call-site safety.

Suggested refactor
 from __future__ import annotations
 
 import math
 import logging
 import time
-from typing import Any, Dict
+from typing import Any, Dict, TYPE_CHECKING
@@
-_store = None
+if TYPE_CHECKING:
+    from paperbot.infrastructure.stores.memory_store import SqlAlchemyMemoryStore
+
+_store: "SqlAlchemyMemoryStore | None" = None
@@
-def _get_store():
+def _get_store() -> "SqlAlchemyMemoryStore":

As per coding guidelines, src/**/*.py: Use pyright in basic mode for Python type checking.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/paperbot/mcp/tools/save_to_memory.py` around lines 36 - 47, Annotate the
module-level _store and the _get_store() return type: add typing imports
(Optional, TYPE_CHECKING) and, under TYPE_CHECKING, import SqlAlchemyMemoryStore
from paperbot.infrastructure.stores.memory_store so you can use a forward
reference; change _store to _store: Optional["SqlAlchemyMemoryStore"] = None and
annotate def _get_store() -> "SqlAlchemyMemoryStore": so static checkers
(pyright) know the concrete type while keeping the runtime lazy import inside
_get_store() unchanged.
.planning/research/ARCHITECTURE.md (2)

665-667: Avoid making Docker a hard architectural blocker.

Line 665 frames Docker+PG as mandatory. Consider documenting alternatives (managed PG instance, local service, CI service container) to keep the plan environment-agnostic.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.planning/research/ARCHITECTURE.md around lines 665 - 667, Update the "1.
**Docker + PostgreSQL dev environment** — Nothing works without a PG target."
entry to remove Docker as a hard blocker and list alternatives: note that a
managed PostgreSQL instance, a locally installed PostgreSQL service, or a
CI-provided/Postgres container are valid options; rephrase to "A PostgreSQL
target is required, but Docker is optional" and add a short bulleted sublist of
the alternative approaches and when to prefer each to keep the architecture
environment-agnostic.

125-132: Make pool settings dialect-aware and configurable.

Line 127–128 applies pool_size/max_overflow unconditionally. That guidance can break or misbehave for sqlite+aiosqlite and is risky as a default architecture pattern. Gate pool args by dialect and read sizes from config/env.

For SQLAlchemy 2.x async engines, which create_async_engine pool arguments are valid for postgresql+asyncpg vs sqlite+aiosqlite, and what is the recommended pattern for conditional pool configuration?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.planning/research/ARCHITECTURE.md around lines 125 - 132, The current call
to create_async_engine passes pool_size and max_overflow unconditionally; make
pool settings dialect-aware and configurable by: detect the DB dialect from the
URL/engine config (e.g., the string contains "postgresql+asyncpg" vs
"sqlite+aiosqlite"), only include pool_size/max_overflow for dialects that
support them (postgresql+asyncpg) and omit or use NullPool/StaticPool options
for sqlite+aiosqlite, and read numeric values from a config/env (e.g.,
POOL_SIZE, MAX_OVERFLOW) with sensible defaults; update the create_async_engine
invocation to conditionally build connect/pool args based on that detection so
pool args aren't applied to sqlite+aiosqlite.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.planning/research/ARCHITECTURE.md:
- Around line 320-335: The current _run uses two separate await
connection.run_sync(...) calls and an outer async transaction; instead,
consolidate into a single synchronous callback passed to await
connection.run_sync(...) that performs context.configure(...), then calls
context.begin_transaction() and context.run_migrations() inside that sync
callback (so both configuration and migration execution run in the same sync
context); update _run to remove the outer async with connection.begin(): wrapper
and ensure you still use create_async_engine/asyncio.run as before while calling
connection.run_sync once with the combined callback.

In `@pyproject.toml`:
- Line 62: pyproject metadata incorrectly advertises support for Python 3.8/3.9
while shipping a hard dependency "mcp[fastmcp]>=1.8.0,<2.0.0" that requires
Python >=3.10; either raise requires-python to ">=3.10" and update/remove Python
3.8/3.9 classifiers, or make mcp an optional extra by removing
"mcp[fastmcp]>=1.8.0,<2.0.0" from the main dependencies and adding it under
optional-dependencies/extras (e.g., extras = { "fastmcp":
["mcp[fastmcp]>=1.8.0,<2.0.0"] }) and update classifiers accordingly; if you
keep the try/except ImportError workaround in code, ensure packaging marks mcp
as optional and update README/docs to instruct users to install the extra when
needed.

In `@src/paperbot/infrastructure/event_log/event_bus_event_log.py`:
- Around line 65-81: The append() method currently stores and fans out the same
mutable dict instance causing shared-mutation bugs; fix by making a deep copy of
the event before storing and before enqueueing so each subscriber and the ring
buffer hold independent objects—update append(), _fan_out(), and subscribe()
(and any other places around the event enqueue/dequeue logic such as the
handlers at the other mentioned blocks) to clone the dict (e.g., via deepcopy or
serialize/deserialize) immediately after creating or accepting the event and
again when putting into subscriber queues so callers, ring buffer, and consumers
never share the same mutable instance.

---

Outside diff comments:
In `@src/paperbot/api/main.py`:
- Around line 122-124: The shutdown handler _shutdown_obsidian_runtime currently
only calls obsidian.shutdown_obsidian_runtime(app) but must also close the event
log to clear subscriber queues; after calling
obsidian.shutdown_obsidian_runtime(app) invoke EventBusEventLog.close() on the
running event log instance (e.g. call event_log.close() or obtain it from the
obsidian module/manager if it's stored there) so subscriber queues are cleared
during graceful shutdown.

---

Minor comments:
In @.planning/phases/03-remaining-mcp-tools/03-01-SUMMARY.md:
- Around line 122-123: Replace the asterisk-based emphasis used in the footer
markers and change "*Phase: 03-remaining-mcp-tools*" and "*Completed:
2026-03-14*" to underscore emphasis (e.g., _Phase: 03-remaining-mcp-tools_ and
_Completed: 2026-03-14_) so the file conforms to the repo’s configured MD049
rule; update the two footer marker instances in
.planning/phases/03-remaining-mcp-tools/03-01-SUMMARY.md accordingly.
- Around line 80-81: The markdown contains bare underscored identifiers
`_analyzer` and `_client` causing MD037; update the prose in
.planning/phases/03-remaining-mcp-tools/03-01-SUMMARY.md to escape these tokens
(e.g., wrap `_analyzer` and `_client` in inline code backticks or escape the
underscores) where the analyze_trends MCP tool and check_scholar MCP tool are
described so the identifiers in the lines referencing
src/paperbot/mcp/tools/analyze_trends.py and
src/paperbot/mcp/tools/check_scholar.py render correctly without triggering lint
warnings.

In @.planning/phases/03-remaining-mcp-tools/03-UAT.md:
- Around line 14-21: The UAT expects the tool name "relevance" but the actual
decorated MCP tool in relevance.py is named relevance_assess; update the UAT
documentation and test expectation to use relevance_assess instead of relevance
(replace occurrences in the tool list and the test description), and verify the
MCP registration in server.py still registers relevance_assess (search for the
relevance_assess symbol and the .register(mcp) calls to confirm all 9 tools are
listed).

In @.planning/phases/05-transport-entry-point/05-01-PLAN.md:
- Around line 131-144: The markdown lists in
.planning/phases/05-transport-entry-point/05-01-PLAN.md contain indented
code/examples that trigger MD046; replace those indented snippets (including the
Python examples for src/paperbot/mcp/serve.py — descriptions of run_stdio() and
run_http() and the suggested from __future__ import annotations/docstrings — and
the TOML block describing the [project.scripts] entry and dependencies) with
fenced code blocks using the appropriate language tags (python and toml) so the
examples render consistently and pass markdownlint; ensure the fenced blocks
enclose the exact snippets referenced in the diff and also update the other
occurrence noted (lines ~186-233) similarly.

In @.planning/phases/05-transport-entry-point/05-RESEARCH.md:
- Line 17: The closing tag </phase_requirements> is being parsed as a table row
because the Markdown table isn't terminated; open the file section containing
<phase_requirements> and insert a blank line immediately before the closing
</phase_requirements> tag so the table ends properly (i.e., ensure there is a
blank line between the last table row and </phase_requirements>).
- Line 56: Add a language tag to the fenced code block that starts with triple
backticks on the block around Line 56 (replace ``` with ```<language> e.g.,
```text or ```bash as appropriate) and ensure each Markdown table near the noted
areas (the table around Line 36 and the tables near Lines 458 and 466) has a
blank line before and after it so tables are separated by surrounding blank
lines; update the blocks in the document where you find the triple-backtick
fence and the three table blocks to satisfy MD040 and MD058 respectively.

In @.planning/phases/06-agent-skills/06-RESEARCH.md:
- Line 21: Update the phrase "Phase 6 is purely a content-creation phase. No
Python code is written. The deliverable is four SKILL.md files placed in
`.claude/skills/{skill-name}/SKILL.md`." by capitalizing the word "Markdown"
wherever "markdown" appears in the document (specifically in the description of
the SKILL.md files and workflow body), so the proper noun form "Markdown" is
used consistently; edit the line containing "SKILL.md files" and any occurrences
in the surrounding paragraph to replace "markdown" with "Markdown".
- Around line 531-540: Replace the hardcoded machine-local absolute paths
(entries starting with "/home/master1/") in the research doc with
repository-relative paths (e.g., src/paperbot/mcp/server.py,
src/paperbot/mcp/tools/*.py) or explicitly mark each as "local-only example" if
you intend to keep them; update the lines that list those absolute paths so they
use portable repo-relative references or a clear local-only label to make the
artifact reproducible for other contributors.

In @.planning/phases/06-agent-skills/06-VALIDATION.md:
- Around line 59-62: Update the "Verify skill is loaded and workflow steps
execute" test instruction to include explicit pass/fail criteria: specify the
exact skill ID or filename under `.claude/skills/` that must be loaded (e.g.,
"skill:paperbot-literature-review"), the expected log/event output (e.g.,
"Loaded skill: <skill-id>" and workflow step completion events such as "step 1:
fetch_sources completed"), and the expected response format sections (e.g.,
"Summary", "Key papers", "Notes"). Replace the subjective phrase in the table
cell with these concrete checks so reviewers can assert PASS only when the named
skill ID is observed in logs, the listed workflow events appear, and the agent
response contains the required sections.

In @.planning/phases/07-eventbus-sse-foundation/07-01-PLAN.md:
- Line 186: Replace the hardcoded absolute paths in the automated verification
command (the line containing "PYTHONPATH=/home/master1/PaperBot/src pytest
/home/master1/PaperBot/tests/unit/test_event_bus_event_log.py -q ...") with
repo-relative paths so the check runs in CI and other workstations; for example
change to "PYTHONPATH=src pytest tests/unit/test_event_bus_event_log.py -q ..."
(or use "$PWD" based paths) and make the same edit for the other occurrence
noted around line 228 to ensure all automated commands use repo-relative paths.

In @.planning/phases/07-eventbus-sse-foundation/07-02-PLAN.md:
- Around line 206-217: The markdown sample is being parsed as code causing lint
errors; wrap the example lines that reference _startup_eventlog(), the
EventBusEventLog lookup ([0]), bus.subscribe(), bus.append(...),
_event_generator and sse_comment() in a fenced code block (```python ... ```) or
at minimum inline backticks around identifiers and expressions so the `[0]`
index and `_event_generator` are not interpreted as markdown/links; update the
snippet in 07-02-PLAN.md to use a fenced python block for the whole example (or
backtick each identifier) to resolve MD052/MD037.

In @.planning/REQUIREMENTS.md:
- Around line 210-213: The Coverage summary under the "Coverage:" header is
inconsistent with the v1.0 checklist and traceability table: update the summary
line "v1.0 requirements: 17 total (4 shipped, 13 remaining)" to match the actual
state shown by the checkboxes and traceability table (e.g., "v1.0 requirements:
17 total (17 shipped, 0 remaining)"), or alternatively uncheck any requirements
that are not actually shipped; ensure the single summary string is corrected to
reflect the true shipped/remaining counts so the document is internally
consistent.

In @.planning/research/ARCHITECTURE.md:
- Around line 289-291: The on_job_start function sets the ContextVar
_db_session_context but never resets it, which can leak context across jobs;
modify on_job_start to save the returned token (token =
_db_session_context.set(...)) and implement a matching on_job_end (or ensure in
the job teardown path) that calls _db_session_context.reset(token) to restore
previous state; reference the functions on_job_start and on_job_end and the
ContextVar _db_session_context so the token is stored and reset in the job
lifecycle.
- Around line 507-513: Update the Phase A milestone language under "Phase A —
PostgreSQL + Schema (sync stays)" by replacing the absolute claim "PG works with
existing sync stores" with a scoped statement that distinguishes core CRUD
parity from search/embedding feature gaps—for example: "PG provides core CRUD
parity with existing sync stores; full search (FTS5) and vector/embedding
functionality may degrade or require PG-specific solutions and will be addressed
in later phases." Ensure the updated sentence mentions both the core CRUD parity
and the known FTS/vector limitations so readers see the distinction.

In @.planning/research/PITFALLS.md:
- Around line 89-93: Two fenced code blocks missing language specifiers; add a
language tag (use "text") to each code fence so Markdown lint MD040 is
satisfied: update the first fenced block containing the
psycopg2.errors.DatatypeMismatch message to start with ```text instead of ```
and update the second fenced block containing the SAWarning about type 'vector'
of column 'embedding' to start with ```text as well.

In @.planning/ROADMAP.md:
- Around line 303-304: The two table rows for "6. Agent Skills" and "7. EventBus
+ SSE Foundation" have their column values shifted and do not match the table
header order; update those rows so each pipe-separated cell aligns with the
header (move milestone/plans/status values into their correct columns for the
"6. Agent Skills" and "7. EventBus + SSE Foundation" rows) so the Phase,
Milestone/Plans, Status, Date and Notes cells are in the same order as the table
header.

In @.planning/STATE.md:
- Around line 62-67: The markdown has table rows (Phase 06-agent-skills...,
Phase 07-eventbus..., Phase 07 P02) placed after a list, breaking the "By Phase"
table; move those three rows into the existing "By Phase" table block (the
header and rows starting around the "By Phase" section) so they become proper
table rows, or if you prefer lists, convert each of the three lines into list
items under the preceding list — update the "By Phase" table contents (or the
list) accordingly to restore consistent markdown table/list structure.

In `@tests/integration/test_research_track_context_routes.py`:
- Line 115: Replace the blanket teardown call app.dependency_overrides.clear()
with removing only the override this test sets by calling
app.dependency_overrides.pop(auth_deps.get_required_user_id, None); update both
occurrences around the test (the lines using app.dependency_overrides.clear) so
the test only cleans up its override for auth_deps.get_required_user_id and does
not wipe other tests' overrides. Ensure auth_deps.get_required_user_id is the
exact symbol referenced in the test when calling pop.

In `@tests/unit/test_agent_skills.py`:
- Around line 51-55: The current parser accepts a single '---' delimiter because
it only checks if len(parts) < 2, allowing malformed frontmatter to be treated
as parts[1]; update the guard to require both opening and closing frontmatter
delimiters by checking that len(parts) >= 3 and raising a ValueError (with the
same "{skill_name}/SKILL.md: no YAML frontmatter found (missing --- delimiters)"
message) when not met, then continue setting frontmatter =
yaml.safe_load(parts[1]) and body = parts[2] if present.

In `@tests/unit/test_mcp_save_to_memory.py`:
- Line 127: The pytest.raises assertion in tests/unit/test_mcp_save_to_memory.py
uses a plain string for match which treats '.' as regex wildcards; update the
match to a properly escaped regex (e.g., use r"confidence must be between 0\.0
and 1\.0" or generate the pattern with re.escape("confidence must be between 0.0
and 1.0")) so the test strictly matches the literal message in the ValueError
raised by the code under test.

---

Duplicate comments:
In @.planning/STATE.md:
- Around line 3-14: Frontmatter values (milestone, total_phases,
completed_phases, percent) are inconsistent with the document body ("Current
focus" and "Phase"); update the YAML keys (milestone, total_phases,
completed_phases, percent) to match the body’s current focus and phase count (or
vice versa) so the frontmatter and body are synchronized—specifically reconcile
milestone with "Current focus" and total_phases/completed_phases/percent with
the "Phase: X of Y" and completed counts shown in the body.

In `@src/paperbot/mcp/tools/save_to_memory.py`:
- Around line 52-53: The function in src/paperbot/mcp/tools/save_to_memory.py
currently uses a shared literal default user_id ("default") for the user_id
parameter (and similarly for the scope_type default), which can mix different
users' memories; update the function signature (the save_to_memory function or
equivalent that declares user_id: str = "default", scope_type: str = "global")
to require an explicit user_id (remove the "default" default or set user_id:
Optional[str] = None) and add runtime validation that raises a clear error
(e.g., ValueError) if user_id is missing or equals "default"; do the same for
scope_type (avoid using "global" as an implicit identity or validate allowed
values) and fix the other occurrence noted (the second default instance) so no
code path uses the shared "default" value.

---

Nitpick comments:
In @.planning/phases/06-agent-skills/06-RESEARCH.md:
- Around line 483-485: The current frontmatter extraction using parts =
content.split("---", 2) and assigning frontmatter = yaml.safe_load(parts[1]) is
brittle because it splits on any '---' in the file; update the parser to
explicitly extract only a leading YAML frontmatter block (or use a dedicated
frontmatter library) instead of blind splitting. Replace the split-based logic
that sets parts/frontmatter/body with a targeted extraction: detect an opening
'---' at the file start and capture up to the next closing '---' (e.g.,
anchored, DOTALL non-greedy regex) or call a frontmatter parser, then pass the
captured YAML to yaml.safe_load and set body to the remainder; ensure you handle
the case where no leading frontmatter is present so frontmatter stays None and
body remains the full content.

In @.planning/phases/06-agent-skills/06-VALIDATION.md:
- Around line 22-33: Replace the shell-dependent inline env commands
(`PYTHONPATH=src pytest tests/unit/test_agent_skills.py -q` and `PYTHONPATH=src
pytest -q`) with cross-platform invocations (`python -m pytest
tests/unit/test_agent_skills.py -q` and `python -m pytest -q`) in the Quick run,
Full suite and Sampling Rate sections, and add a persistent Python path setting
to the project configuration by adding pythonpath = ["src"] under the pytest
config in pyproject.toml (or equivalent [tool.pytest.ini_options]) so tests run
portably across non-POSIX environments.

In @.planning/research/ARCHITECTURE.md:
- Around line 665-667: Update the "1. **Docker + PostgreSQL dev environment** —
Nothing works without a PG target." entry to remove Docker as a hard blocker and
list alternatives: note that a managed PostgreSQL instance, a locally installed
PostgreSQL service, or a CI-provided/Postgres container are valid options;
rephrase to "A PostgreSQL target is required, but Docker is optional" and add a
short bulleted sublist of the alternative approaches and when to prefer each to
keep the architecture environment-agnostic.
- Around line 125-132: The current call to create_async_engine passes pool_size
and max_overflow unconditionally; make pool settings dialect-aware and
configurable by: detect the DB dialect from the URL/engine config (e.g., the
string contains "postgresql+asyncpg" vs "sqlite+aiosqlite"), only include
pool_size/max_overflow for dialects that support them (postgresql+asyncpg) and
omit or use NullPool/StaticPool options for sqlite+aiosqlite, and read numeric
values from a config/env (e.g., POOL_SIZE, MAX_OVERFLOW) with sensible defaults;
update the create_async_engine invocation to conditionally build connect/pool
args based on that detection so pool args aren't applied to sqlite+aiosqlite.

In @.planning/research/FEATURES.md:
- Around line 77-109: The fenced dependency diagram in the FEATURES.md document
(the triple-backtick block that begins with "Docker Compose (PostgreSQL local
dev)") lacks a language specifier; fix it by changing the opening fence from ```
to ```text (or ```plaintext) so the code block becomes fenced with a language
tag to satisfy MD040 and improve accessibility.

In `@src/paperbot/mcp/tools/save_to_memory.py`:
- Around line 36-47: Annotate the module-level _store and the _get_store()
return type: add typing imports (Optional, TYPE_CHECKING) and, under
TYPE_CHECKING, import SqlAlchemyMemoryStore from
paperbot.infrastructure.stores.memory_store so you can use a forward reference;
change _store to _store: Optional["SqlAlchemyMemoryStore"] = None and annotate
def _get_store() -> "SqlAlchemyMemoryStore": so static checkers (pyright) know
the concrete type while keeping the runtime lazy import inside _get_store()
unchanged.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: fafa1f34-ed5a-41d8-abc6-b4d69c22163b

📥 Commits

Reviewing files that changed from the base of the PR and between df35290 and 3789a6a.

📒 Files selected for processing (47)
  • .claude/skills/literature-review/SKILL.md
  • .claude/skills/paper-reproduction/SKILL.md
  • .claude/skills/scholar-monitoring/SKILL.md
  • .claude/skills/trend-analysis/SKILL.md
  • .planning/PROJECT.md
  • .planning/REQUIREMENTS.md
  • .planning/ROADMAP.md
  • .planning/STATE.md
  • .planning/phases/03-remaining-mcp-tools/03-01-SUMMARY.md
  • .planning/phases/03-remaining-mcp-tools/03-UAT.md
  • .planning/phases/04-mcp-resources/04-UAT.md
  • .planning/phases/05-transport-entry-point/05-01-PLAN.md
  • .planning/phases/05-transport-entry-point/05-01-SUMMARY.md
  • .planning/phases/05-transport-entry-point/05-RESEARCH.md
  • .planning/phases/05-transport-entry-point/05-VALIDATION.md
  • .planning/phases/05-transport-entry-point/05-VERIFICATION.md
  • .planning/phases/06-agent-skills/06-01-PLAN.md
  • .planning/phases/06-agent-skills/06-01-SUMMARY.md
  • .planning/phases/06-agent-skills/06-01-VERIFICATION.md
  • .planning/phases/06-agent-skills/06-RESEARCH.md
  • .planning/phases/06-agent-skills/06-VALIDATION.md
  • .planning/phases/07-eventbus-sse-foundation/07-01-PLAN.md
  • .planning/phases/07-eventbus-sse-foundation/07-01-SUMMARY.md
  • .planning/phases/07-eventbus-sse-foundation/07-02-PLAN.md
  • .planning/phases/07-eventbus-sse-foundation/07-02-SUMMARY.md
  • .planning/phases/07-eventbus-sse-foundation/07-VERIFICATION.md
  • .planning/phases/12-pg-infrastructure-schema/12-CONTEXT.md
  • .planning/research/ARCHITECTURE.md
  • .planning/research/FEATURES.md
  • .planning/research/PITFALLS.md
  • .planning/research/STACK.md
  • .planning/research/SUMMARY.md
  • pyproject.toml
  • requirements.txt
  • src/paperbot/api/main.py
  • src/paperbot/api/routes/events.py
  • src/paperbot/infrastructure/event_log/event_bus_event_log.py
  • src/paperbot/mcp/serve.py
  • src/paperbot/mcp/tools/save_to_memory.py
  • src/paperbot/presentation/cli/main.py
  • tests/integration/test_events_sse_endpoint.py
  • tests/integration/test_research_track_context_routes.py
  • tests/unit/test_agent_skills.py
  • tests/unit/test_event_bus_event_log.py
  • tests/unit/test_mcp_save_to_memory.py
  • tests/unit/test_mcp_serve_cli.py
  • tests/unit/test_research_context_route_explicit_track.py
✅ Files skipped from review due to trivial changes (4)
  • .planning/phases/07-eventbus-sse-foundation/07-VERIFICATION.md
  • .claude/skills/literature-review/SKILL.md
  • .claude/skills/scholar-monitoring/SKILL.md
  • .claude/skills/paper-reproduction/SKILL.md

Comment on lines +320 to +335
connectable = create_async_engine(async_url, poolclass=pool.NullPool)

async def _run():
async with connectable.connect() as connection:
await connection.run_sync(
lambda sync_conn: context.configure(
connection=sync_conn,
target_metadata=target_metadata,
compare_type=True,
render_as_batch=False, # PG supports native ALTER
)
)
async with connection.begin():
await connection.run_sync(context.run_migrations)

asyncio.run(_run())

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

find . -name "ARCHITECTURE.md" -type f

Repository: jerry609/PaperBot

Length of output: 97


🏁 Script executed:

head -n 335 ./.planning/research/ARCHITECTURE.md | tail -n 25

Repository: jerry609/PaperBot

Length of output: 966


🏁 Script executed:

# Let me check the broader context around these lines
sed -n '300,340p' ./.planning/research/ARCHITECTURE.md

Repository: jerry609/PaperBot

Length of output: 1437


🏁 Script executed:

web_search
Search for "Alembic async migration pattern canonical run_sync context.configure context.run_migrations"

Repository: jerry609/PaperBot

Length of output: 156


🌐 Web query:

Alembic async migration canonical pattern run_sync context.configure context.run_migrations best practices

💡 Result:

Canonical Alembic async env.py pattern (what -t async is based on)

The officially documented pattern is:

  • keep Alembic’s work synchronous in do_run_migrations(connection) (this is where you call context.configure(...) and context.run_migrations()),
  • but acquire the DB connection asynchronously and bridge into the sync function using await connection.run_sync(do_run_migrations),
  • and from the CLI entrypoint, call it via asyncio.run(...). [1]
# env.py (online path)
import asyncio
from sqlalchemy.ext.asyncio import async_engine_from_config
from sqlalchemy import pool
from alembic import context

def do_run_migrations(connection):
    context.configure(connection=connection, target_metadata=target_metadata)
    with context.begin_transaction():
        context.run_migrations()

async def run_async_migrations():
    connectable = async_engine_from_config(
        context.config.get_section(context.config.config_ini_section),
        prefix="sqlalchemy.",
        poolclass=pool.NullPool,
    )
    async with connectable.connect() as connection:
        await connection.run_sync(do_run_migrations)
    await connectable.dispose()

def run_migrations_online():
    asyncio.run(run_async_migrations())

(Offline mode remains the normal sync context.configure(url=..., literal_binds=True, ...) + context.run_migrations().) [1]

Best practices / gotchas

  1. Don’t “async-ify” Alembic itself (i.e., don’t make context.configure() / context.run_migrations() run directly in async code). The supported approach is specifically to use run_sync() to run Alembic’s sync API against an async connection. [1][2]

  2. If you already have a running event loop, don’t try to hack env.py with loop.create_task(...). People hit NoneType has no attribute configure / lost alembic.context state when doing that. Instead, use the cookbook’s programmatic async approach: run Alembic’s synchronous command.upgrade(...) inside AsyncConnection.run_sync(...), and in env.py support an externally-supplied connection via config.attributes["connection"]. [1][2]

  3. Consider a “hybrid” run_migrations_online() if you need both sync and async execution paths (common in test tooling). Pytest-Alembic documents a small re-org that decides sync vs async based on whether the engine/connection is an AsyncEngine, to avoid re-entrant asyncio.run() issues. [3]

Sources: Alembic Cookbook “Using Asyncio with Alembic” + “Programmatic API use (connection sharing) With Asyncio” [1]; Alembic discussion about failures when a loop already exists + recommended direction (run_sync, programmatic API) [2]; Pytest-Alembic asyncio notes and hybrid pattern [3].


Consolidate context.configure() and context.run_migrations() into a single sync callback per Alembic's canonical async pattern.

The current implementation splits these calls across separate run_sync() invocations and wraps the migration execution in an outer async transaction. The official Alembic async pattern consolidates both calls into a single synchronous callback wrapped with context.begin_transaction(), then bridges the async connection via a single await connection.run_sync() call. Separating Alembic's context operations across multiple calls risks losing context state and diverges from the documented best practices.

Proposed update
-    async def _run():
-        async with connectable.connect() as connection:
-            await connection.run_sync(
-                lambda sync_conn: context.configure(
-                    connection=sync_conn,
-                    target_metadata=target_metadata,
-                    compare_type=True,
-                    render_as_batch=False,  # PG supports native ALTER
-                )
-            )
-            async with connection.begin():
-                await connection.run_sync(context.run_migrations)
+    def _do_run_migrations(sync_conn):
+        context.configure(
+            connection=sync_conn,
+            target_metadata=target_metadata,
+            compare_type=True,
+            render_as_batch=False,
+        )
+        with context.begin_transaction():
+            context.run_migrations()
+
+    async def _run():
+        async with connectable.connect() as connection:
+            await connection.run_sync(_do_run_migrations)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
connectable = create_async_engine(async_url, poolclass=pool.NullPool)
async def _run():
async with connectable.connect() as connection:
await connection.run_sync(
lambda sync_conn: context.configure(
connection=sync_conn,
target_metadata=target_metadata,
compare_type=True,
render_as_batch=False, # PG supports native ALTER
)
)
async with connection.begin():
await connection.run_sync(context.run_migrations)
asyncio.run(_run())
connectable = create_async_engine(async_url, poolclass=pool.NullPool)
def _do_run_migrations(sync_conn):
context.configure(
connection=sync_conn,
target_metadata=target_metadata,
compare_type=True,
render_as_batch=False,
)
with context.begin_transaction():
context.run_migrations()
async def _run():
async with connectable.connect() as connection:
await connection.run_sync(_do_run_migrations)
asyncio.run(_run())
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.planning/research/ARCHITECTURE.md around lines 320 - 335, The current _run
uses two separate await connection.run_sync(...) calls and an outer async
transaction; instead, consolidate into a single synchronous callback passed to
await connection.run_sync(...) that performs context.configure(...), then calls
context.begin_transaction() and context.run_migrations() inside that sync
callback (so both configuration and migration execution run in the same sync
context); update _run to remove the outer async with connection.begin(): wrapper
and ensure you still use create_async_engine/asyncio.run as before while calling
connection.run_sync once with the combined callback.

Comment thread pyproject.toml Outdated
Comment thread src/paperbot/infrastructure/event_log/event_bus_event_log.py

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Implements a PaperBot MCP server (tools + resources), adds a CLI transport entrypoint, and introduces an in-process EventBus + /api/events/stream SSE foundation, while removing reliance on the legacy shared "default" user namespace across multiple subsystems.

Changes:

  • Add MCP server package with registered tools/resources plus paperbot mcp serve (stdio / streamable-http).
  • Add EventBusEventLog fan-out backend and new SSE endpoint for real-time event streaming.
  • Introduce user_identity helpers and migrate code/tests away from "default" user_id behavior.

Reviewed changes

Copilot reviewed 144 out of 145 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tests/unit/test_workflow_query_grounder.py Update tests to use non-legacy user_id.
tests/unit/test_wiki_route.py Update wiki route tests for authenticated user context override.
tests/unit/test_wiki_concept_store.py Update store tests to use non-legacy user_id.
tests/unit/test_wiki_concept_service.py Update service tests to use non-legacy user_id.
tests/unit/test_unified_topic_search_grounding.py Update grounding tests to use non-legacy user_id.
tests/unit/test_research_context_route_explicit_track.py Update route tests to override get_required_user_id instead of passing user_id in body.
tests/unit/test_paperscool_route.py Allow user_id=None in topic search fakes.
tests/unit/test_paper_judge_persistence.py Update persistence test user_id away from "default".
tests/unit/test_obsidian_sync_service.py Update Obsidian sync tests to use explicit user_id.
tests/unit/test_obsidian_sync.py Update Obsidian sync tests to use explicit user_id.
tests/unit/test_obsidian_cli.py Update CLI tests to pass explicit --user-id.
tests/unit/test_mcp_track_papers.py Add unit tests for MCP track_papers resource.
tests/unit/test_mcp_track_metadata.py Add unit tests for MCP track_metadata resource.
tests/unit/test_mcp_track_memory.py Add unit tests for MCP track_memory resource.
tests/unit/test_mcp_scholars.py Add unit tests for MCP scholars resource.
tests/unit/test_mcp_save_to_memory.py Add unit tests for MCP save_to_memory tool.
tests/unit/test_mcp_relevance.py Add unit tests for MCP relevance_assess tool.
tests/unit/test_mcp_paper_summarize.py Add unit tests for MCP paper_summarize tool.
tests/unit/test_mcp_paper_search.py Add unit tests for MCP paper_search tool.
tests/unit/test_mcp_export_to_obsidian.py Add unit tests for MCP export_to_obsidian tool.
tests/unit/test_mcp_check_scholar.py Add unit tests for MCP check_scholar tool.
tests/unit/test_mcp_bootstrap.py Add smoke tests for MCP server bootstrap/import surface.
tests/unit/test_mcp_analyze_trends.py Add unit tests for MCP analyze_trends tool.
tests/unit/test_llm_service.py Assert relevance fallback metadata is surfaced.
tests/unit/test_intelligence_store.py Update intelligence store tests to use non-legacy user_id.
tests/unit/test_intelligence_routes.py Update intelligence route tests for authenticated user context override.
tests/unit/test_intelligence_radar_service.py Update radar service test defaults away from "default".
tests/unit/test_api_main_eventlog_lifecycle.py Add tests for event log lifecycle + fallback behavior.
tests/unit/test_anchor_service.py Update anchor service tests for optional vs personalized user_id semantics.
tests/unit/test_agent_skills.py Add structural tests for .claude/skills/*/SKILL.md.
tests/integration/test_research_track_context_routes.py Update integration tests to use auth dependency override instead of query param user_id.
tests/integration/test_events_sse_endpoint.py Add integration tests for SSE heartbeat and event delivery via EventBus.
src/paperbot/utils/user_identity.py Add helpers to normalize/require user identity and reject legacy "default".
src/paperbot/repro/repro_agent.py Propagate optional user_id and avoid persisting experiences for legacy/anonymous flows.
src/paperbot/repro/orchestrator.py Normalize user_id in orchestrator context with optional identity.
src/paperbot/repro/nodes/verification_node.py Only persist verified structures when a real user_id is present.
src/paperbot/repro/nodes/generation_node.py Gate experience loading/persistence on real user_id.
src/paperbot/repro/memory/code_memory.py Require real user_id for DB-backed experience operations.
src/paperbot/repro/agents/debugging_agent.py Gate persistence of failure-reason experiences on real user_id.
src/paperbot/repro/agents/coding_agent.py Pass through user_id without forcing "default".
src/paperbot/presentation/cli/main.py Add paperbot mcp serve and require explicit user_id for Obsidian export.
src/paperbot/mcp/tools/save_to_memory.py Add MCP save_to_memory tool with audit logging.
src/paperbot/mcp/tools/relevance.py Add MCP relevance_assess tool wrapper with degraded detection.
src/paperbot/mcp/tools/paper_summarize.py Add MCP paper_summarize tool wrapper with degraded detection.
src/paperbot/mcp/tools/paper_search.py Add MCP paper_search tool wrapper + input validation + audit logging.
src/paperbot/mcp/tools/paper_judge.py Add MCP paper_judge tool wrapper with degraded detection.
src/paperbot/mcp/tools/check_scholar.py Add MCP check_scholar tool wrapper with degraded mode on not-found.
src/paperbot/mcp/tools/analyze_trends.py Add MCP analyze_trends tool wrapper with degraded detection.
src/paperbot/mcp/tools/_audit.py Add shared MCP tool-call audit logging with argument sanitization.
src/paperbot/mcp/tools/init.py Initialize MCP tools package.
src/paperbot/mcp/server.py Create FastMCP server and register tools/resources.
src/paperbot/mcp/serve.py Add transport dispatch for stdio vs streamable-http.
src/paperbot/mcp/resources/track_papers.py Add MCP track_papers resource wrapper.
src/paperbot/mcp/resources/track_metadata.py Add MCP track_metadata resource wrapper.
src/paperbot/mcp/resources/track_memory.py Add MCP track_memory resource wrapper.
src/paperbot/mcp/resources/scholars.py Add MCP scholars resource wrapper.
src/paperbot/mcp/resources/init.py Initialize MCP resources package.
src/paperbot/mcp/init.py Initialize MCP package.
src/paperbot/infrastructure/stores/research_store.py Remove legacy default user_id handling in get_paper_detail.
src/paperbot/infrastructure/stores/repro_experience_store.py Require explicit user_id for experience persistence/query.
src/paperbot/infrastructure/stores/models.py Remove server defaults for user_id columns in select models.
src/paperbot/infrastructure/services/intelligence_radar_service.py Require explicit user_id across radar service methods.
src/paperbot/infrastructure/obsidian/sync.py Make Obsidian sync treat "default" as no-op / anonymous.
src/paperbot/infrastructure/event_log/event_bus_event_log.py Add EventBus event log backend for SSE fan-out delivery.
src/paperbot/context_engine/engine.py Make optional components accessed via getattr for safer injection.
src/paperbot/application/workflows/unified_topic_search.py Treat user_id as optional and only ground queries when identity present.
src/paperbot/application/workflows/analysis/paper_judge.py Avoid provider metadata when parsing/judgment payload is missing.
src/paperbot/application/services/p2c/models.py Make P2C request user_id optional.
src/paperbot/application/services/p2c/context_bridge.py Skip enrichment when no real user identity is present.
src/paperbot/application/services/llm_service.py Add explicit fallback=True metadata on relevance fallback.
src/paperbot/application/services/enrichment_pipeline.py Make enrichment pipeline user_id optional.
src/paperbot/application/services/anchor_service.py Support anonymous/global mode and only personalize with real user_id.
src/paperbot/api/routes/wiki.py Switch wiki concepts route to authenticated user dependency.
src/paperbot/api/routes/repro_context.py Skip memory writes when user identity is absent.
src/paperbot/api/routes/paperscool.py Make request/user_id optional and normalize legacy default.
src/paperbot/api/routes/events.py Add /api/events/stream SSE endpoint backed by EventBus.
src/paperbot/api/main.py Wire EventBus into CompositeEventLog and include events router; close event log on shutdown.
src/paperbot/api/auth/dependencies.py Make get_user_id return Optional[str] (no "default" fallback).
requirements.txt Add MCP dependency line.
pyproject.toml Add MCP dependency and paperbot console script entry.
alembic/versions/0028_remove_legacy_user_defaults.py Add migration to remove server defaults for legacy user_id columns.
.planning/phases/07-eventbus-sse-foundation/07-VALIDATION.md Add phase validation strategy doc.
.planning/phases/07-eventbus-sse-foundation/07-CONTEXT.md Add phase context/decisions doc.
.planning/phases/06-agent-skills/06-VALIDATION.md Add phase validation strategy doc.
.planning/phases/06-agent-skills/06-01-VERIFICATION.md Add verification report doc.
.planning/phases/05-transport-entry-point/05-VALIDATION.md Add phase validation strategy doc.
.planning/phases/05-transport-entry-point/05-01-SUMMARY.md Add implementation summary doc.
.planning/phases/04-mcp-resources/04-VALIDATION.md Add phase validation strategy doc.
.planning/phases/04-mcp-resources/04-UAT.md Add UAT checklist doc.
.planning/phases/04-mcp-resources/04-02-SUMMARY.md Add implementation summary doc.
.planning/phases/03-remaining-mcp-tools/03-VALIDATION.md Add phase validation strategy doc.
.planning/phases/03-remaining-mcp-tools/03-UAT.md Add UAT checklist doc.
.planning/phases/03-remaining-mcp-tools/03-03-SUMMARY.md Add implementation summary doc.
.planning/phases/03-remaining-mcp-tools/03-01-SUMMARY.md Add implementation summary doc.
.planning/STATE.md Add planning state snapshot.
.planning/PROJECT.md Add planning project overview/requirements doc.
.claude/skills/trend-analysis/SKILL.md Add agent skill definition for trend analysis.
.claude/skills/scholar-monitoring/SKILL.md Add agent skill definition for scholar monitoring.
.claude/skills/paper-reproduction/SKILL.md Add agent skill definition for paper reproduction.
.claude/skills/literature-review/SKILL.md Add agent skill definition for literature review.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Comment on lines +169 to +171
return await _save_to_memory_impl(
content, kind, user_id, scope_type, scope_id, confidence, _run_id
)
Comment on lines +17 to +22
Thread-safety note:
append() is called from the async event loop only (uvicorn single-process).
put_nowait() is safe; no thread bridging needed for current architecture.
_fan_out() uses list(self._queues) snapshot to guard against concurrent
unsubscribe() calls inside the same event-loop tick.
"""
@jerry609
jerry609 merged commit cd0ba4c into dev Mar 14, 2026
17 checks passed
@jerry609
jerry609 deleted the feat/mcp-phases-02-04 branch March 14, 2026 14:37
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