Skip to content

Commit 9dfc721

Browse files
DvirDukhanCopilot
andcommitted
fix(t17): address PR #675 review comments
- pyproject + uv.lock: drop pytest-anyio 0.0.0 placeholder; anyio's pytest plugin (already required via tests extra) provides the @pytest.mark.anyio marker. (Copilot review) - api/graph.py: when listing graphs, apply the internal-suffix filter to the *branch* component for parsed names (e.g. code:repo:main_git is hidden, but code:repo:release_tmp stays visible). Legacy bare names still filtered by full name. (CodeRabbit graph.py:99) - api/graph.py: Graph/AsyncGraphQuery constructors now coerce branch="" to DEFAULT_BRANCH alongside branch=None, so self.branch matches the actual graph key produced by compose_graph_name. (CodeRabbit graph.py:133) - api/project.py: same coercion for Project.__init__; an explicit empty-string branch now autodetects (or falls back to DEFAULT_BRANCH) instead of staying "". (CodeRabbit project.py:71) - api/index.py + api/llm.py: ChatRequest.branch is now threaded through ask() / _ask_sync() / _create_kg_agent() and used to compose the FalkorDB graph key. Previously it was accepted on the request but silently ignored. (CodeRabbit index.py:81) - api/migrations/per_branch.py: when both src and dst graphs exist (interrupted prior migration) the rename helper now deletes the leftover legacy src and reports success, so reruns can complete cleanly instead of skipping forever. Dry-run path preserved. (CodeRabbit per_branch.py:80) - api/git_utils/git_utils.py: rename CamelCase helpers GitRepoName/LegacyGitRepoName -> git_repo_name/legacy_git_repo_name (snake_case per api guidelines); keep CamelCase as deprecated aliases so the public surface stays backwards compatible. Update the in-module callers. switch_commit now guards against a None current_hash and raises a clear ValueError instead of silently walking off the end. (CodeRabbit git_utils.py:30) - api/info.py: get_repo_commit return type tightened to Optional[str] to reflect the documented None return. (CodeRabbit git_utils.py:30) All 24 tests in tests/test_per_branch_graphs.py still pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent a43b8a8 commit 9dfc721

8 files changed

Lines changed: 82 additions & 37 deletions

File tree

api/git_utils/git_utils.py

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414
# Configure logging
1515
logging.basicConfig(level=logging.DEBUG, format='%(filename)s - %(asctime)s - %(levelname)s - %(message)s')
1616

17-
def GitRepoName(repo_name, branch=None):
17+
def git_repo_name(repo_name, branch=None):
1818
""" Returns the git transitions graph key for ``(repo_name, branch)``.
1919
2020
Format: ``{repo_name}:{branch}_git``. Hash-tag stays on ``repo_name``
@@ -27,10 +27,15 @@ def GitRepoName(repo_name, branch=None):
2727
return "{" + repo_name + "}" + ":" + branch + "_git"
2828

2929

30-
def LegacyGitRepoName(repo_name):
30+
def legacy_git_repo_name(repo_name):
3131
"""Pre-T17 git graph key shape — kept for the migration helper."""
3232
return "{" + repo_name + "}_git"
3333

34+
35+
# Backwards-compatible CamelCase aliases (deprecated, will be removed).
36+
GitRepoName = git_repo_name
37+
LegacyGitRepoName = legacy_git_repo_name
38+
3439
def is_ignored(file_path: str, ignore_list: List[str]) -> bool:
3540
"""
3641
Checks if a file should be ignored based on the ignore list.
@@ -108,7 +113,7 @@ def build_commit_graph(path: str, analyzer: SourceAnalyzer, repo_name: str, igno
108113
g = source.clone(tmp_name)
109114
g.enable_backlog()
110115

111-
git_graph = GitGraph(GitRepoName(repo_name, branch))
116+
git_graph = GitGraph(git_repo_name(repo_name, branch))
112117
supported_types = analyzer.supported_types()
113118

114119
# Initialize with the current commit
@@ -298,12 +303,22 @@ def switch_commit(repo: str, to: str, branch: Optional[str] = None):
298303

299304
# Initialize the graph and GitGraph objects
300305
g = Graph(repo, branch=branch)
301-
git_graph = GitGraph(GitRepoName(repo, branch))
306+
git_graph = GitGraph(git_repo_name(repo, branch))
302307

303308
# Get the current commit hash of the graph
304309
current_hash = get_repo_commit(repo, branch)
305310
logging.info(f"Current graph commit: {current_hash}")
306311

312+
if current_hash is None:
313+
logging.error(
314+
"Cannot switch commit: no recorded commit for repo=%s branch=%s",
315+
repo, branch,
316+
)
317+
raise ValueError(
318+
f"No recorded commit for repo '{repo}' (branch={branch}); "
319+
"the repository must be analyzed before switch_commit can run."
320+
)
321+
307322
if current_hash == to:
308323
logging.debug("Current commit: {current_hash} is the requested commit")
309324
# No change remain at the current commit

api/graph.py

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -93,15 +93,21 @@ def get_repos() -> list[dict]:
9393

9494
repos = []
9595
for g in db.list_graphs():
96-
if _is_internal_suffix(g):
97-
continue
9896
parsed = parse_graph_name(g)
9997
if parsed is None:
100-
# Legacy graph (pre-T17): synthesize a virtual entry so it stays
101-
# discoverable until the migration helper promotes it.
98+
# Legacy graph (pre-T17) or internal helper graph: skip when
99+
# the bare name carries an internal suffix; otherwise synthesize
100+
# a virtual entry so it stays discoverable.
101+
if _is_internal_suffix(g):
102+
continue
102103
repos.append({"project": g, "branch": DEFAULT_BRANCH, "graph": g})
103104
else:
104105
project, branch = parsed
106+
# Hide per-branch internal companion graphs (e.g. ``branch_git``,
107+
# ``branch_schema``, ``branch_tmp``); their suffix lives on the
108+
# branch component, so check that explicitly.
109+
if _is_internal_suffix(branch):
110+
continue
105111
repos.append({"project": project, "branch": branch, "graph": g})
106112
return repos
107113

@@ -129,7 +135,9 @@ def __init__(self, name: str, branch: Optional[str] = None) -> None:
129135
self.name = name
130136
else:
131137
self.project = name
132-
self.branch = branch if branch is not None else DEFAULT_BRANCH
138+
# Normalize empty / None to DEFAULT_BRANCH so the stored
139+
# branch matches the key actually used by compose_graph_name.
140+
self.branch = branch or DEFAULT_BRANCH
133141
self.name = compose_graph_name(self.project, self.branch)
134142

135143
self.db = FalkorDB(host=os.getenv('FALKORDB_HOST', 'localhost'),
@@ -766,13 +774,15 @@ async def async_get_repos() -> list[dict]:
766774
try:
767775
repos = []
768776
for g in await db.list_graphs():
769-
if _is_internal_suffix(g):
770-
continue
771777
parsed = parse_graph_name(g)
772778
if parsed is None:
779+
if _is_internal_suffix(g):
780+
continue
773781
repos.append({"project": g, "branch": DEFAULT_BRANCH, "graph": g})
774782
else:
775783
project, branch = parsed
784+
if _is_internal_suffix(branch):
785+
continue
776786
repos.append({"project": project, "branch": branch, "graph": g})
777787
return repos
778788
finally:
@@ -796,7 +806,7 @@ def __init__(self, name: str, branch: Optional[str] = None) -> None:
796806
self.name = name
797807
else:
798808
self.project = name
799-
self.branch = branch if branch is not None else DEFAULT_BRANCH
809+
self.branch = branch or DEFAULT_BRANCH
800810
self.name = compose_graph_name(self.project, self.branch)
801811
self.db = _async_db()
802812
self.g = self.db.select_graph(self.name)

api/index.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -223,7 +223,7 @@ async def chat(data: ChatRequest, _=Depends(public_or_auth)):
223223
"""Chat with the CodeGraph language model."""
224224

225225
try:
226-
answer = await ask(data.repo, data.msg)
226+
answer = await ask(data.repo, data.msg, branch=data.branch)
227227
except Exception as e:
228228
logging.exception("Chat error for repo '%s': %s", data.repo, e)
229229
return JSONResponse({"status": "error", "response": "Internal server error"},

api/info.py

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -67,8 +67,12 @@ def set_repo_commit(repo_name: str, commit_hash: str, branch: Optional[str] = No
6767
raise
6868

6969

70-
def get_repo_commit(repo_name: str, branch: Optional[str] = None) -> str:
71-
"""Get the current commit the repo is at for ``(repo_name, branch)``."""
70+
def get_repo_commit(repo_name: str, branch: Optional[str] = None) -> Optional[str]:
71+
"""Get the current commit the repo is at for ``(repo_name, branch)``.
72+
73+
Returns ``None`` when no commit has been recorded yet (e.g. the
74+
repository has not been analyzed under the given branch).
75+
"""
7276

7377
try:
7478
r = get_redis_connection()

api/llm.py

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import os
22
import asyncio
33
import logging
4+
from typing import Optional
45

56
from graphrag_sdk.models.litellm import LiteModel
67
from graphrag_sdk import (
@@ -235,14 +236,23 @@ def _define_ontology() -> Ontology:
235236
# Global ontology
236237
ontology = _define_ontology()
237238

238-
def _create_kg_agent(repo_name: str):
239+
def _create_kg_agent(repo_name: str, branch: Optional[str] = None):
240+
from .graph import compose_graph_name, DEFAULT_BRANCH
241+
239242
model_name = os.getenv('MODEL_NAME', 'gemini/gemini-flash-lite-latest')
240243

241244
model = LiteModel(model_name)
242245

246+
# Resolve the actual FalkorDB graph key: callers may pass either a
247+
# bare project name + branch or an already-composed ``code:p:b`` name.
248+
if repo_name.startswith("code:") and ":" in repo_name[5:]:
249+
graph_name = repo_name
250+
else:
251+
graph_name = compose_graph_name(repo_name, branch or DEFAULT_BRANCH)
252+
243253
#ontology = _define_ontology()
244254
code_graph_kg = KnowledgeGraph(
245-
name=repo_name,
255+
name=graph_name,
246256
ontology=ontology,
247257
model_config=KnowledgeGraphModelConfig.with_model(model),
248258
host=os.getenv('FALKORDB_HOST', 'localhost'),
@@ -257,8 +267,8 @@ def _create_kg_agent(repo_name: str):
257267

258268
return code_graph_kg.chat_session()
259269

260-
def _ask_sync(repo_name: str, question: str) -> str:
261-
chat = _create_kg_agent(repo_name)
270+
def _ask_sync(repo_name: str, question: str, branch: Optional[str] = None) -> str:
271+
chat = _create_kg_agent(repo_name, branch=branch)
262272

263273
logging.debug(f"Question: {question}")
264274
print(f"Question: {question}")
@@ -268,6 +278,6 @@ def _ask_sync(repo_name: str, question: str) -> str:
268278
return response['response']
269279

270280

271-
async def ask(repo_name: str, question: str) -> str:
281+
async def ask(repo_name: str, question: str, branch: Optional[str] = None) -> str:
272282
loop = asyncio.get_running_loop()
273-
return await loop.run_in_executor(None, _ask_sync, repo_name, question)
283+
return await loop.run_in_executor(None, _ask_sync, repo_name, question, branch)

api/migrations/per_branch.py

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,22 @@ def _rename_graph(db: FalkorDB, src: str, dst: str, *, dry_run: bool) -> bool:
6868
"""
6969

7070
if db.connection.exists(dst):
71+
# Recovery path: a previous migration may have copied ``src`` to
72+
# ``dst`` but crashed before deleting ``src``. If both still exist,
73+
# finish the job rather than skipping forever.
74+
if db.connection.exists(src):
75+
if dry_run:
76+
logger.info(
77+
"[dry-run] would delete leftover legacy graph %s (dst %s already exists)",
78+
src, dst,
79+
)
80+
return True
81+
db.select_graph(src).delete()
82+
logger.info(
83+
"Deleted leftover legacy graph %s (dst %s already exists)",
84+
src, dst,
85+
)
86+
return True
7187
logger.warning("Target graph %s already exists — skipping rename of %s", dst, src)
7288
return False
7389
if dry_run:

api/project.py

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,10 @@ def __init__(self, name: str, path: Path, url: Optional[str], branch: Optional[s
6666
self.name = name
6767
self.path = path
6868
# Auto-detect branch from the working tree when not explicitly given.
69-
if branch is None:
69+
# Treat the empty string the same as ``None`` so the project never
70+
# reports a branch that disagrees with the graph/info keys it writes
71+
# (which coerce empty branch to DEFAULT_BRANCH).
72+
if not branch:
7073
branch = detect_branch(path) if path is not None and Path(path).exists() else DEFAULT_BRANCH
7174
self.branch = branch
7275
self.graph = Graph(name, branch=self.branch)

uv.lock

Lines changed: 2 additions & 15 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)