Skip to content

wip: capture OpenViking repo state#1213

Draft
kal3w wants to merge 1 commit intovolcengine:mainfrom
kal3w:codex/openviking-cleanup-capture
Draft

wip: capture OpenViking repo state#1213
kal3w wants to merge 1 commit intovolcengine:mainfrom
kal3w:codex/openviking-cleanup-capture

Conversation

@kal3w
Copy link
Copy Markdown

@kal3w kal3w commented Apr 3, 2026

Preserves the current local OpenViking repo state in a dedicated capture branch.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 3, 2026

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🏅 Score: 78
🧪 No relevant tests
🔒 No security concerns identified
✅ No TODO sections
🔀 Multiple PR themes

Sub-PR theme: MCP Bridge Enhancements and Codex Setup

Relevant files:

  • examples/mcp-query/server.py
  • examples/common/recipe.py
  • examples/mcp-query/README.md
  • docs/en/guides/09-codex-setup.md
  • docs/en/guides/06-mcp-integration.md

Sub-PR theme: Claude Memory Plugin Offline Mode and Detached Commits

Relevant files:

  • examples/claude-memory-plugin/scripts/ov_memory.py
  • examples/claude-memory-plugin/hooks/common.sh
  • examples/claude-memory-plugin/hooks/session-start.sh
  • examples/claude-memory-plugin/hooks/session-end.sh
  • examples/claude-memory-plugin/hooks/stop.sh
  • examples/claude-memory-plugin/hooks/user-prompt-submit.sh
  • examples/claude-memory-plugin/hooks/hooks.json
  • examples/claude-memory-plugin/README.md
  • examples/claude-memory-plugin/.claude-plugin/marketplace.json

Sub-PR theme: Add Timestamps to MatchedContext

Relevant files:

  • openviking_cli/retrieve/types.py
  • openviking/retrieve/hierarchical_retriever.py

⚡ Recommended focus areas for review

Type Mismatch Bug

The _query_sync function returns a dict when recipe.query_ready is false, but the query MCP tool expects a string return type. This will cause a runtime error when the query tool is used without LLM config.

if not recipe.query_ready:
    return {
        "answer": (
            "Query is not configured on this MCP bridge. "
            "Provide a local ov.conf with vlm.api_base and vlm.model, "
            "or use the search tool and let Codex synthesize the answer."
        ),
        "context": [],
        "timings": {},
    }
N+1 Query Performance Issue

The search method calls self.client.stat() for each search result to retrieve timestamps, leading to N additional roundtrips. This can degrade performance when many results are returned.

Silent Exception Swallowing

The _replay_offline_sessions function uses broad except Exception: clauses without logging, which hides errors during offline session replay and makes debugging difficult.

except Exception:
    continue
if not turns:
    turns_file.unlink(missing_ok=True)
    continue
try:
    with OVClient(backend, ov_conf_path) as cli:
        sess = cli.create_session()
        sid = _as_text(sess.get("session_id"))
        if not sid:
            continue
        for t in turns:
            cli.add_message(sid, t["role"], t["content"])
        cli.commit_session(sid)
    turns_file.unlink(missing_ok=True)
    replayed += 1
except Exception:
    break  # server may have gone offline mid-replay
Set -e Removed

The set -e flag was removed from the common.sh script, which means the script will continue executing even if a command fails, potentially leading to unexpected behavior.

set -uo pipefail

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 3, 2026

PR Code Suggestions ✨

No code suggestions found for the PR.

@CLAassistant
Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

2 participants